Skip to content

fix(common): reject malformed user headers during deserialization - #4670

Open
udaya-arka wants to merge 7 commits into
apache:masterfrom
udaya-arka:4604-http-send-message-validation-changes
Open

udaya-arka wants to merge 7 commits into
apache:masterfrom
udaya-arka:4604-http-send-message-validation-changes

Conversation

@udaya-arka

Copy link
Copy Markdown

Which issue does this PR address?

Closes #4604

Rationale

The HTTP produce endpoint accepted typed user headers that violate the wire protocol's TLV bounds (0-byte values, widths mismatched with fixed-size types like int32, or values > 255 bytes). When encoded by user_headers_to_wire, these headers corrupted the message TLV stream, causing consumers polling the message to fail header decoding (Err(InvalidHeaderValue)) or silently drop all headers on the message (Ok(None)).

What changed?

  • Marker-specific error mapping: Added a sealed HeaderFieldMarker trait on KeyMarker and ValueMarker returning IggyError::InvalidHeaderKey and IggyError::InvalidHeaderValue respectively.
  • Constructor validation: Parameterized HeaderField<T>::from_raw by T: HeaderFieldMarker, returning the marker-specific error on empty fields, lengths > 255 bytes, or fixed-kind width mismatches.
  • Serde deserialization validation: Implemented custom Deserialize for HeaderField<T> that deserializes raw base64 data and routes it through Self::from_raw(), rejecting malformed keys/values at the HTTP/JSON boundary (HTTP 400).
  • Defense-in-depth: Added a debug_assert! in user_headers_to_wire verifying that the encoded buffer is structurally valid TLV.
  • Tests: Enabled and passed the boundary rejection tests in send_messages.rs, and added unit tests for HeaderField, HeaderEntry, HeaderKey, HeaderValue, and IggyMessage deserialization.

Local Execution

  • Passed:
    • cargo test -p iggy_common
    • cargo test -p iggy
    • cargo test -p iggy-cli
    • cargo clippy --package iggy_common --all-targets
    • cargo fmt --check --package iggy_common
  • Pre-commit hooks ran: passed

AI Usage

  1. Which tools? : Gemini CLI
  2. Scope of Usage : Generating text for commit, PR description, generating unit tests for failure scenarios, and understanding scope of the changes for options suggested in the issue.

@github-actions

Copy link
Copy Markdown

Thanks for the PR. It is labeled S-waiting-on-review and queued for review.

If AI was used to create this PR, you must run the team-review skill before opening it.

Slash commands (own line, regular comment) move it around the queue:

  • /ready - back to S-waiting-on-review after addressing feedback
  • /author - flip to S-waiting-on-author while you finish changes
  • /request-review @user-or-team - request a reviewer
  • /pin - exempt the PR from the stale bot, /unpin to undo

See CONTRIBUTING.md for details.

@github-actions github-actions Bot added the S-waiting-on-review PR is waiting on a reviewer label Oct 28, 2026
@justinmclean

Copy link
Copy Markdown
Member

Thanks @udaya-arka, and welcome. Routing the JSON through from_raw closes the gap #4604 describes, and the tests cover each case in the issue.

The CI runs show "action required" because a maintainer has to approve workflows for a first-time contributor, so they will start once someone approves them.

One small thing: debug_assert! is already compiled out of release builds, so the #[cfg(debug_assertions)] above it in user_headers_to_wire can go.

Serde's derived Deserialize for HeaderField<T> bypassed the length and
fixed-size checks enforced by constructors (1..=255 bytes, exact width
for fixed kinds). This allowed malformed typed user headers on the HTTP
produce endpoint to be encoded into the wire format under the assumption
that HeaderField instances are valid by construction, corrupting headers
for readers.

Implement custom Deserialize for HeaderField<T> routing decoded bytes
through from_raw(), returning InvalidHeaderKey for keys and
InvalidHeaderValue for values via a sealed HeaderFieldMarker trait.
Add a debug_assert! in user_headers_to_wire to assert TLV validity on
encode.

Signed-off-by: Prabhath Nampally <prabhathn03@gmail.com>
Signed-off-by: Prabhath Nampally <prabhathn03@gmail.com>
@udaya-arka
udaya-arka force-pushed the 4604-http-send-message-validation-changes branch from 504fc06 to 26fb845 Compare October 29, 2026 03:35
@udaya-arka

Copy link
Copy Markdown
Author

Hi @justinmclean, I'm happy to contribute, and thanks for pointing out the redundant debug_assertions attribute!

I've removed it in a new commit. I've also signed off my previous commit using an interactive rebase, so the commit hashes may have changed.

@hubcio

hubcio commented Oct 29, 2026

Copy link
Copy Markdown
Contributor

/skill team-review

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary: The header validation matches the binary codec, and the raw base64 header form stays opaque by design. The Rust build and tests are still queued, so this review rests on source reading and flags an extra copy per decoded header field, a new IggyMessage test that cannot fail, and a key error code that differs between from_raw and TryFrom.

Counts: critical 0, warning 3, nit 0, simplification 1

Findings without an anchor on a changed line:

  • core/common/src/wire_conversions.rs simplification: headers_from_validated_slice() re-checks expected_size and builds fields with new_unchecked(), duplicating what from_raw() now enforces with the same error codes. Build each field with HeaderKey::from_raw() and HeaderValue::from_raw().

This review was generated by Claude Code 2.1.310 on deepseek-flash[1m]. Review the output before you act on it.

@github-actions github-actions Bot added S-waiting-on-author PR is waiting on author response and removed S-waiting-on-review PR is waiting on a reviewer labels Oct 29, 2026
@codecov

codecov Bot commented Oct 29, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 58.41270% with 131 lines in your changes missing coverage. Please review.
✅ Project coverage is 70.48%. Comparing base (a05636a) to head (527ec50).
⚠️ Report is 4 commits behind head on master.

Files with missing lines Patch % Lines
core/common/src/types/message/user_headers.rs 66.08% 77 Missing and 1 partial ⚠️
core/common/src/types/message/iggy_message.rs 0.00% 35 Missing ⚠️
core/common/src/http/messages/send_messages.rs 63.04% 17 Missing ⚠️
core/common/src/wire_conversions.rs 75.00% 0 Missing and 1 partial ⚠️

❗ There is a different number of reports uploaded between BASE (a05636a) and HEAD (527ec50). Click for more details.

HEAD has 1 upload less than BASE
Flag BASE (a05636a) HEAD (527ec50)
rust 3 2
Additional details and impacted files
@@              Coverage Diff              @@
##             master    #4670       +/-   ##
=============================================
- Coverage     80.72%   70.48%   -10.25%     
- Complexity     3104     3106        +2     
=============================================
  Files          1479     1477        -2     
  Lines        361399   353961     -7438     
  Branches     305269   297832     -7437     
=============================================
- Hits         291746   249473    -42273     
- Misses        62771    97309    +34538     
- Partials       6882     7179      +297     
Components Coverage Δ
Rust Core 67.60% <58.41%> (-12.40%) ⬇️
Java SDK 78.15% <ø> (+0.01%) ⬆️
C# SDK 83.54% <ø> (-0.06%) ⬇️
Python SDK 85.67% <ø> (ø)
PHP SDK 90.71% <ø> (ø)
Node SDK 96.44% <ø> (ø)
Go SDK 69.26% <ø> (-0.05%) ⬇️
C++ SDK 85.55% <ø> (ø)
Files with missing lines Coverage Δ
core/common/src/wire_conversions.rs 87.34% <75.00%> (-5.24%) ⬇️
core/common/src/http/messages/send_messages.rs 67.47% <63.04%> (-10.14%) ⬇️
core/common/src/types/message/iggy_message.rs 55.55% <0.00%> (-17.65%) ⬇️
core/common/src/types/message/user_headers.rs 60.45% <66.08%> (-14.15%) ⬇️

... and 571 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

udaya-arka and others added 2 commits October 29, 2026 23:46
   - Avoid extra allocation in HeaderField::deserialize by moving owned Bytes into from_raw_bytes()
   - Bound FromStr and TryFrom for HeaderField by HeaderFieldMarker to return InvalidHeaderKey for keys
   - Fix IggyMessage test payload to include the required header object and verify valid headers deserialize

Signed-off-by: Prabhath Nampally <prabhathn03@gmail.com>
@udaya-arka

Copy link
Copy Markdown
Author

/ready

@github-actions github-actions Bot added S-waiting-on-review PR is waiting on a reviewer and removed S-waiting-on-author PR is waiting on author response labels Oct 29, 2026
Simplify headers_from_validated_slice() by delegating field validation
and construction to HeaderKey::from_raw() and HeaderValue::from_raw(),
removing duplicated expected_size checks and new_unchecked() calls.

Signed-off-by: Prabhath Nampally <prabhathn03@gmail.com>
@udaya-arka

Copy link
Copy Markdown
Author

performed refactoring in core/common/src/wire_conversions.rs as suggested.
Now HeaderKey is constructed using from_raw.

/ready

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

S-waiting-on-review PR is waiting on a reviewer

Projects

None yet

Development

Successfully merging this pull request may close these issues.

HTTP send_messages accepts typed user headers that no reader can decode

3 participants