Match chunked as the final transfer coding, order-independently#2500
Merged
Conversation
is_chunked_transfer_encoding compared the whole Transfer-Encoding field
value against "chunked", so a message whose final coding is chunked but
which names another coding first ("gzip, chunked", valid under RFC 9112
6.1) was read as unframed. On a keep-alive server the body was then left
in the socket and parsed as a smuggled request; open_stream repeated the
check case-sensitively with a raw ==, desyncing the client stream the
same way.
Rework the helper to match the last coding token case-insensitively and
route open_stream through it so both paths agree. The codings may also be
split across multiple Transfer-Encoding lines (RFC 9110 5.3); since
Headers is an unordered_multimap whose duplicate-key iteration order is
not portable, the final coding of a multi-line field cannot be
determined reliably, so treat any such message that names chunked as
chunked (fail safe: a mis-parse only closes the connection, whereas the
opposite error enables smuggling). A unit test covers the helper,
including order-independent multi-line cases.
Based on #2487 by @metsw24-max.
Owner
Author
|
@metsw24-max thanks for sending #2487. This PR is the revised version that I made with Opus. I think this is a more comprehensive solution. Please let me know if you have any question. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Based on #2487 by @metsw24-max — reworked implementation, so opening separately to make the diff reviewable.
Problem
is_chunked_transfer_encodingcompared the wholeTransfer-Encodingfield value against"chunked", so a message whose final coding is chunked but which names another coding first ("gzip, chunked", valid under RFC 9112 §6.1 where chunked must be the final coding) was read as unframed. On a keep-alive server the body is then left in the socket and parsed as the next request (smuggling).open_streamrepeated the check case-sensitively with a raw==, desyncing the client stream the same way.Change
Transfer-Encodingvalue case-insensitively instead of comparing the whole field, and routeopen_streamthrough the same helper so server and streaming client agree.Transfer-Encodinglines rather than only index 0.Headersis anunordered_multimapwhose duplicate-key iteration order is not portable (this is what broke CI on the original PR: libstdc++ and libc++ disagree), so the final coding of a multi-line field cannot be determined reliably. In that ambiguous case we fail safe — any multi-line field that nameschunkedis treated as chunked. A mis-parse only closes the connection, whereas the opposite error enables smuggling.Tests
ChunkedTransferEncodingTestcovers single-coding (case-insensitive),chunkedas the final coding of a list, non-finalchunked, absent/empty, and order-independent multi-line cases. Builds and passes on both the header-only and split (test_split) builds.Difference from #2487
#2487 read only the first
Transfer-Encodingline viaget_header_value(..., 0)and dropped its multi-line test because of theunordered_multimapordering issue. This version handles the multiple-line case deterministically by failing safe, so no ordering assumption is needed.🤖 Generated with Claude Code