fix: merge duplicate Transfer-Encoding: chunked response headers - #1201
Open
drusc0 wants to merge 6 commits into
Open
fix: merge duplicate Transfer-Encoding: chunked response headers#1201drusc0 wants to merge 6 commits into
drusc0 wants to merge 6 commits into
Conversation
Some servers send a redundant, byte-identical Transfer-Encoding: chunked header line twice on the wire, which h11 correctly rejects per RFC 9112 but which requests and browsers tolerate. Normalize this narrow, safe case the same way h11 already tolerates duplicate identical Content-Length headers, before the bytes ever reach h11's parser. Anything else (differing values, other conflicts) still raises RemoteProtocolError exactly as before. Fixes pydantic#622
Merging this PR will not alter performance
Comparing 
Footnotes
|
![veria-ai[bot]](https://avatars.githubusercontent.com/in/1723776?s=60&v=4){kind=small}
PR overviewAll previously flagged issues have been addressed. No open security concerns remain on this pull request. Security reviewNo open security issues remain on this pull request. Fixed/addressed: 1 · PR risk: 0/10 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
1 issue found across 4 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/httpcore2/httpcore2/_async/http11.py">
<violation number="1" location="src/httpcore2/httpcore2/_async/http11.py:58">
P2: This predicate merges more than byte-identical lines: it strips and lowercases names and values, so malformed or differently encoded duplicates bypass h11's validation. Match the raw header line and remove only an exact repeated line.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| merged_lines = [] | ||
| for line in header_lines: | ||
| name, sep, value = line.partition(b":") | ||
| if sep and name.strip().lower() == b"transfer-encoding" and value.strip().lower() == b"chunked": |
Contributor
There was a problem hiding this comment.
P2: This predicate merges more than byte-identical lines: it strips and lowercases names and values, so malformed or differently encoded duplicates bypass h11's validation. Match the raw header line and remove only an exact repeated line.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/httpcore2/httpcore2/_async/http11.py, line 58:
<comment>This predicate merges more than byte-identical lines: it strips and lowercases names and values, so malformed or differently encoded duplicates bypass h11's validation. Match the raw header line and remove only an exact repeated line.</comment>
<file context>
@@ -38,6 +38,87 @@ class HTTPConnectionState(enum.IntEnum):
+ merged_lines = []
+ for line in header_lines:
+ name, sep, value = line.partition(b":")
+ if sep and name.strip().lower() == b"transfer-encoding" and value.strip().lower() == b"chunked":
+ if seen_chunked_transfer_encoding:
+ continue
</file context>
drusc0
marked this pull request as draft
September 11, 2026 15:52
The previous commit normalized duplicate Transfer-Encoding headers with a standalone byte-stream wrapper that re-implemented HTTP header parsing ahead of h11, using its own \r\n\r\n boundary and \r\n line-splitting assumptions. Two independent security reviews found this unsound: - h11 tolerates a bare \n (not just \r\n\r\n) as a header terminator and splits lines on \n, so a non-\r\n header block let normalization run past the real boundary into response body bytes, corrupting them and shifting message framing onto the next pooled response. - h11 supports obsolete line folding (a continuation line starting with whitespace); the wrapper's case/whitespace-insensitive matching could treat a folded continuation as a standalone duplicate and delete it, corrupting an unrelated header's value and, in one reproduction, fully desyncing a pooled connection so the next request received a response the server never sent for it. - The wrapper also silently stopped applying after any 1xx interim response (100 Continue / 103 Early Hints) ahead of the final response, and a case/whitespace-insensitive match let malformed duplicate lines bypass h11's own validation. This replaces that wrapper with normalization gated directly on `h11_state.their_state == h11.SEND_RESPONSE`, mirroring h11's actual boundary regex and line-splitting so a header block is parsed exactly the way h11 will parse it, and re-arming naturally across 1xx responses via h11's own state machine instead of manual bookkeeping. Only ever removes a line that is unfolded, not the status line, not itself followed by a fold continuation, and matches (case-insensitive name, OWS-stripped-and-lowered value) exactly `(transfer-encoding, chunked)` after an identical earlier line -- everything else is left untouched so h11 still raises for it exactly as before. A second review round of this replacement found one more real issue: the boundary search only looked inside this connection's own accumulator, not bytes h11 might already be holding unconsumed from a previous read (e.g. a pipelined response, or two responses landing in the same TCP read on a keep-alive connection -- an everyday occurrence, not an edge case). When the true boundary straddled that hidden junction, the search could lock onto a later, coincidental match inside the body. Fixed by skipping normalization whenever h11 already holds unparsed trailing data at the point a new header block would start, falling back to h11's pre-existing (safe) handling for that read. Both rounds' proof-of-concept payloads, plus the original issue-622 reproduction over a real TCP socket, are verified fixed. Full suite (2020 tests) passes with 100% coverage; mypy strict and ruff are clean. Fixes pydantic#622
A third security review of the previous commit's fix found two more issues, neither a correctness/desync bug like the earlier two rounds: - The response-header accumulator re-searched for the header/body boundary from byte 0 on every network read instead of resuming where the previous search left off, making header assembly O(n^2) in the header size. h11's own ReceiveBuffer avoids exactly this (its module docstring calls it out explicitly as a DoS concern) via a resumable search offset; the accumulator now does the same, tracking `_response_header_search_from` and resuming 2 bytes before the end of what's already been scanned (the terminator is at most 3 bytes). Isolated benchmarking confirms linear scaling after the fix, versus quadratic before (~65x slower at 100KB). - Merging duplicate `Transfer-Encoding: chunked` when a `Content-Length` header is also present let a Content-Length/Transfer-Encoding framing ambiguity through that h11 previously rejected outright -- exactly the shape of the classic conflicting-framing request-smuggling primitive. Issue pydantic#622's actual reproductions never combine the two, so the merge now bails out (deliberately broad, case-insensitive substring check) whenever anything resembling Content-Length is present in the header block, at no cost to the fix's actual purpose. Reproduced both issues before fixing (isolated O(n) vs O(n^2) timing comparison; a Content-Length + duplicate-Transfer-Encoding payload that previously merged and desynced a pooled connection), and re-verified every proof-of-concept from the first two review rounds stays fixed. Full suite (2026 tests) passes with 100% coverage; mypy strict and ruff are clean. Fixes pydantic#622
drusc0
marked this pull request as ready for review
September 12, 2026 02:11
Contributor
There was a problem hiding this comment.
All reported issues were addressed across 4 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Co-authored-by: cubic-dev-ai[bot] <191113872+cubic-dev-ai[bot]@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
All reported issues were addressed across 1 file (changes from recent commits).
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
The reviewer's suggested change (narrowing the Content-Length guard to check each header line's field name before the colon, rather than a substring search over the whole raw block -- avoiding a false positive when an unrelated header value or the reason phrase merely contains the text "content-length") was applied directly to the async source via GitHub's UI, which doesn't run this repo's formatting/unasync pipeline. This runs scripts/lint to reformat per ruff's style and regenerate the auto-generated sync mirror, which is what CI was failing on. Verified the precision fix itself is correct: a response whose reason phrase/header values merely mention "content-length" now merges the duplicate Transfer-Encoding header as it should, while a genuine Content-Length header still blocks the merge. Full suite (2026 tests) passes with 100% coverage; mypy strict and ruff are clean.
Contributor
There was a problem hiding this comment.
All reported issues were addressed across 2 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
The docstring still described the Content-Length bail-out as a "broad, case-insensitive substring" scan over the whole header block -- accurate for the original implementation, but stale after the precision fix that narrowed it to an exact per-line field-name match (avoiding a false positive when an unrelated header value or the reason phrase merely contains the text "content-length"). Update the docstring to describe the actual check, including the resulting narrow gap it shares with the Transfer-Encoding match: a Content-Length header expressed only via obsolete line folding won't be detected. No behavior change. Full suite (2026 tests) still passes with 100% coverage; mypy strict and ruff are clean.
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
1 participant
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.
Summary
Transfer-Encoding: chunkedheader twice on the wire. h11 correctly rejects this per RFC 9112, butrequestsand browsers tolerate it.h11_state.their_state == h11.SEND_RESPONSEinsideHTTP11Connection._receive_event(), mirroring h11's own header/body boundary regex and line-splitting exactly, so a header block is parsed the same way h11 will parse it. Re-arms naturally across 1xx interim responses via h11's own state machine. Only ever removes a line that is unfolded, isn't the status line, isn't itself followed by a fold continuation, has noContent-Lengthheader anywhere in the block, and matches (case-insensitive name, OWS-stripped value) exactly(transfer-encoding, chunked)after an identical earlier line — everything else is left untouched, so h11 still raises for it exactly as before.Revision history (kept for reviewer context)
This went through three rounds of independent security review before landing on the current design:
\nheader termination and obsolete line folding that the wrapper didn't account for, which could corrupt response body bytes, shift message framing, or in one reproduction fully desync a pooled connection so the next request received a response the server never sent for it. It also silently stopped applying after a 1xx interim response, and case/whitespace-insensitive matching let malformed duplicate lines bypass h11's validation.ReceiveBuffer; (b) merging duplicateTransfer-Encoding: chunkedwhenContent-Lengthwas also present let a conflicting-framing ambiguity through that h11 previously rejected — fixed by bailing out of the merge wheneverContent-Lengthis present anywhere in the header block (issue RemoteProtocolError: multiple Transfer-Encoding headers #622's real reproductions never combine the two, so this costs nothing).Test plan
tests/httpcore2/_async/test_http11.py(sync mirror auto-generated viascripts/unasync.py) covering: happy-path merge, merge split across reads, merge with bare-LF-terminated headers, merge after a 1xx interim response (both split-read and same-read timing), differing-values / malformed-line / obsolete-line-fold / Content-Length-present regression guards (must still raise), oversized-header regression guard.scripts/check(ruff format/check, mypy strict, unasync sync-check) passes.scripts/test— 2026 passed, 1 unrelated pre-existing skip.scripts/coverage— 100%.httpx2.get()stack.Fixes #622