fix(probe): parse Transfer-Encoding instead of matching the whole line - #911
fix(probe): parse Transfer-Encoding instead of matching the whole line#911atoz96 wants to merge 2 commits into
Conversation
GeniePod#910) read_http_body detected chunk framing by comparing the entire header line against "transfer-encoding: chunked", so it only matched the single canonical spelling. RFC 7230 3.2 allows zero or more spaces after the colon and 3.3.1 permits a codings list whose last entry frames the body, so Transfer-Encoding:chunked and gzip, chunked both slipped past. a chunked response carries no Content-Length, so the miss fell through to the read-to-EOF path and returned the wire bytes verbatim — the caller got a 200 and a body with the chunk-size lines inlined ("5\r\nhello\r\n6\r\n world\r\n0" instead of "hello world"), with nothing surfacing an error. is_chunked now parses the header the way parse_content_length two functions below already did: split on the colon, compare the name case-insensitively, and require the final comma-separated coding to be chunked.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughUpdated HTTP body reading to parse Transfer-Encoding fields according to their coding lists and received order. Chunked decoding now depends on the final coding, while unsupported codings are rejected. Tests cover spacing, repeated fields, ordering, and supported or rejected combinations. ChangesTransfer-Encoding handling
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/genie-common/src/probe.rs`:
- Around line 746-776: Update the HTTP body handling exercised by
probe_http_get_body/read_http_body so a response declaring “gzip, chunked” is
either decoded through the supported transfer codings before producing a String
or explicitly rejected as unsupported. Replace the current invalid
fixture/assertion with behavior that uses valid gzip-compressed chunk contents
or verifies the rejection path, while preserving the existing plain chunked
cases.
- Around line 420-429: Update the Transfer-Encoding detection logic in the
surrounding probe function to combine all repeated Transfer-Encoding field
values in header order, then inspect only the final coding for chunked framing
instead of using .any(...). Ensure an earlier chunked value followed by another
coding is treated as close-delimited, and add coverage for repeated fields whose
final coding is not chunked.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 5ebd0e79-f93c-43ba-ae18-ff92eadcaa30
📒 Files selected for processing (1)
crates/genie-common/src/probe.rs
addresses both review findings on GeniePod#911, which were flaws in that patch rather than in the original code. is_chunked used .any() over the header lines, so an earlier `Transfer-Encoding: chunked` won even when a later field changed the combined value. repeated fields combine in order as one list (RFC 9110 5.3) and only the final coding frames the body (RFC 9112 6.1), so `chunked` followed by `gzip` is `chunked, gzip` — close-delimited, not chunk-framed. transfer_codings now collects the codings in order and is_chunked asks only whether the last one is chunked. the `gzip, chunked` case in the test was also wrong: it shipped uncompressed bytes inside the chunks and asserted plain text came back. on a conformant response those bytes are gzip, and read_http_body only removes chunk framing before a lossy utf-8 conversion — so the caller would get a plausible-looking String that is silently mangled. rather than add a gzip decoder, undecodable_codings reports any coding other than a trailing chunked (identity excepted) and read_http_body fails with "unsupported transfer coding". mirrors the inbound reader in http.rs, which already rejects transfer-encodings it cannot handle. the fixture is replaced by two tests: one pinning the ordering contract over repeated fields, one asserting `gzip, chunked` is refused while `identity, chunked` still decodes. both fail on the previous commit — the second returned "hello world" for a body that was never plain text.
|
Both findings were right, and both were flaws in my patch rather than in the original code. Fixed in 48620df. 1. Final coding, not any coding. 2. The Rather than pull in a gzip decoder, I took the reject path you offered as the alternative: The bad fixture is gone, replaced by:
The two plain non-canonical spellings ( Both new tests fail on the previous commit. The second is the clearest statement of the bug you found: — the old code cheerfully returned Verification path is unchanged from the PR body: x86_64 laptop, no Jetson — this is pure HTTP framing driven through a real loopback |
1 participant
Summary
read_http_bodydetected chunk framing by comparing the whole header line against"transfer-encoding: chunked", so only the single canonical spelling matched. Because a chunked response carries noContent-Length, a miss fell through to the read-to-EOF path and returned the wire bytes verbatim — chunk-size lines inlined into the body, with a 200 status and no error. Fixes #910.Changes
is_chunkedparses the header the wayparse_content_lengthtwo functions below already did: split on the colon, compare the name case-insensitively, and require the final comma-separated coding to bechunked(RFC 7230 §3.3.1 — chunked must be last).Transfer-Encoding:chunked,Transfer-Encoding: chunked,transfer-encoding: gzip, chunked.Real Behavior Proof
Tested profile / hardware (check all that apply):
jetsonraspberry_piportable_sbclaptopmacWhat I ran
x86_64 Linux laptop (kernel 6.8), no Jetson and no cross-compile toolchain here — so this is CONTRIBUTING route 3 plus route 4's disclosure, not route 1.
This is a pure HTTP-framing path with no hardware, audio, or Home Assistant dependency: the affected code reads bytes off an
AsyncRead, and the tests drive it through a realTcpListeneron loopback. That makes loopback an equivalent exercise of the changed path rather than a proxy for it. What I cannot verify from here is any Jetson-specific behaviour of the callers — none is expected, since the change is confined to header parsing.Named tests covering the changed path:
probe::tests::chunked_body_decoded_for_noncanonical_transfer_encoding(new — the regression)probe::tests::read_chunked_body_decodes_valid_chunks(existing — canonical framing still decodes)probe::tests::read_chunked_body_rejects_oversize_chunk_header(existing — the size guard still fires)probe::tests::parse_content_lengthcallers via the fixed-body tests around it (existing — the non-chunked branch is unchanged)What I observed
The new test fails on
mainwith the framing inlined into the payload:After the fix, all three spellings decode to
hello world.Full run on this branch:
Test plan
probe_http_get_body("127.0.0.1:8099", "/", false, ProbeTimeouts::default()).mainthe returnedStringcarries the chunk-size lines; on this branch it ishello world.Summary by CodeRabbit
Transfer-Encoding.Transfer-Encodingheader values (with chunking determined only by the final coding).gzip, chunkedinstead of returning corrupted text).