fix(h1): require chunked as the final transfer-coding, trim OWS - #19
Conversation
Chunked framing was selected by a suffix-only compare on the raw Transfer-Encoding value, with no OWS trim, so values like `chunked\t` (trailing tab), ` chunked ` (surrounding OWS), and `chunked, gzip` (chunked not the final coding) were not recognised as chunked, and `xchunked` false-positived as chunked. Per RFC 9112 §6.1 (chunked must be the final transfer-coding) and RFC 9110 §5.6.3 (OWS), select chunked only when the final comma-separated transfer-coding token equals `chunked` (case-insensitive) after trimming OWS from the value and each token. When a Transfer-Encoding is present but its final coding is not chunked, raise a parse error rather than leaving the framing unresolved. Content-Length elision and the multi-TE / CL+TE warnings are preserved when chunked is selected. Two small zero-copy helpers (trim_ows, ends_with_chunked_coding), no new deps. Tests in tests/edge_cases.rs cover the accepted and rejected forms and the Content-Length elision. Signed-off-by: Florentin Dubois <florentin.dubois@clever.cloud>
FlorentinDUBOIS
left a comment
FlorentinDUBOIS
left a comment
There was a problem hiding this comment.
Automated cross-review (review / review-code / guidelines / simplify / security-review + Codex), adversarially verified. 2 findings survived (3 refuted). Posted as comments (self-authored PR).
…Transfer-Encoding Addresses two review findings on the initial commit: - process_headers serves both requests and responses, so the unconditional error on a non-chunked-final Transfer-Encoding wrongly rejected spec-valid close-delimited responses (e.g. a `gzip` response). Gate the reject on kawa.kind == Kind::Request; a response keeps the body size Content-Length processing produced (close-delimited when Empty), matching pre-fix behaviour. - Repeated Transfer-Encoding field lines combine per RFC 9110 §5.3, so a split `gzip` then `chunked` is `gzip, chunked` (chunked is the final coding) and must not be rejected on the first line. Defer the reject via a pending flag that a later chunked-final line clears; decide once after the header-block loop. `chunked` then `identity` still rejects (request). Tests (tests/edge_cases.rs): split gzip+chunked accepted, split chunked+identity rejected, and a response with a non-chunked-final Transfer-Encoding no longer errors. Signed-off-by: Florentin Dubois <florentin.dubois@clever.cloud>
…ding The previous commit set body_size = Chunked immediately on a chunked Transfer-Encoding line and only deferred the reject decision, so a response whose split Transfer-Encoding lines were `chunked` then `identity` stayed mis-framed as chunked even though the combined final coding is `identity` (close-delimited). Resolve Transfer-Encoding once, up front: repeated field lines combine (RFC 9110 §5.3), so the combined final transfer-coding is the final coding of the last line. Pre-scan for presence and final-chunked, then process Content-Length knowing a present Transfer-Encoding overrides it (RFC 9110 §6.3 -> drop every Content-Length), and decide framing after the loop: - combined ends in chunked -> Chunked; - request, not chunked-final -> reject (RFC 9112 §6.3); - response, not chunked-final -> close-delimited (body_size stays Empty). This keeps the existing multiple_length_information behaviour (a differing Content-Length after chunked lines is dropped, not an inconsistency error, because Transfer-Encoding overrides Content-Length) while fixing the response mis-framing. Test (tests/edge_cases.rs): a response with split `chunked` then `identity` Transfer-Encoding lines is now close-delimited (body_size == Empty), not chunked; seen failing against the prior code. Signed-off-by: Florentin Dubois <florentin.dubois@clever.cloud>
|
Hi @FlorentinDUBOIS, this PR matches a vulnerability report I sent privately to Two things:
Happy to share the full write-up and the reproduction. I have published a technical |
2 participants
Summary
Make chunked-framing detection in
process_headersRFC-correct.Chunked framing was selected by a suffix-only compare on the raw
Transfer-Encodingvalue, with noOWS trim. Per RFC 9112 §6.1 (chunked must be the final transfer-coding) and RFC 9110 §5.6.3 (OWS),
this selects chunked only when the final comma-separated transfer-coding token equals
chunked(case-insensitive) after trimming OWS from the value and each token. When a
Transfer-Encodingispresent but its final coding is not
chunked, it now raises a parse error instead of leaving theframing unresolved. Existing behavior (Content-Length elision, the multi-TE / CL+TE warnings) is
preserved when chunked is selected.
Fixes two issues in the old suffix check: values such as
chunked\torchunkedwere notrecognised as chunked, and
xchunkedfalse-positived as chunked.Two small zero-copy helpers (
trim_ows,ends_with_chunked_coding), no new deps.Tests (
tests/edge_cases.rs)chunked,chunked\t,chunked,gzip, chunked→ Chunked;chunked, gzip,identity,xchunked→ parse error;Content-Length+chunked\t→ Chunked with Content-Length elided. All17 crate tests pass;
clippy --libandfmt --checkclean.Notes for reviewers
clippy --all-targets -- -D warningsfails on this branch, but identically on unmodifiedv0.6.8(pre-existing test-lint debt:unused_io_amount, etc.).clippy --libis clean.Transfer-Encoding: gzip+Transfer-Encoding: chunked(two separate lines) is now rejected on the first line, while thecombined
Transfer-Encoding: gzip, chunkedis accepted — flagging in case you'd prefer tocoalesce same-name TE headers first. The reject-on-non-chunked-final rule also applies to
responses (e.g. a close-delimited
Transfer-Encoding: gzipresponse is rejected).