From b022a79eef046a2dc5d1613721491a8b7ea21c70 Mon Sep 17 00:00:00 2001 From: Rodrigo Gomes da Silva Date: Sat, 15 Aug 2026 20:20:36 -0300 Subject: [PATCH] fix(http): reject every Transfer-Encoding, not just the ones we can't name `identity` was exempt as a coding that "frames nothing", so a request carrying it read back as body-less however many payload bytes followed. It frames nothing either: RFC 9112 6.3 says a request whose final transfer coding is not `chunked` has a body length no recipient can determine, RFC 7230 removed `identity` from the transfer codings outright, and `http.server` decodes none of them (`rfile` is the raw socket). Every present value has to reject-and-close. Reproduced on 8c326003b, pipelined down one socket: GET /api/extensions/probe/sidecar/ping `Transfer-Encoding: identity` {"stale": true} -> HTTP/1.1 403 Forbidden (no Connection: close) -> HTTP/1.1 400 Bad request syntax ('{"stale": true}GET /api/health/agent HTTP/1.1') POST /api/upload (and /upload/extract, /workspace/upload, /transcribe) -> HTTP/1.1 400 Bad Request ("No file field in request", no close) -> HTTP/1.1 400 Bad request syntax ('--x') Deletes the `_UNFRAMED_TRANSFER_CODINGS` allowlist and the fold-safety test that existed only to guard its membership check; `_BLANK_TRANSFER_CODING` stays, since a present-but-empty header still needs a named, non-falsy report. The identity rows of the framing tables and of `test_bodyless_framing_keeps_the_pooled_socket_alive` move to the closes-the-socket side, with real-socket coverage on the sidecar path and all four upload endpoints. The keep-alive half stays pinned by the cases that genuinely have no body: no framing header at all, `Content-Length: 0`, an OWS-padded zero, two agreeing zeroes. Swept the rest of the coding surface for the same class: every case spelling, OWS and non-OWS padding, `identity, chunked`, `chunked, identity`, `identity,identity`, a lone comma, an unknown coding, duplicated headers, and a coding beside both a real and a zero Content-Length. --- api/helpers.py | 76 +++--- api/upload.py | 18 +- tests/test_rejected_write_connection_close.py | 237 +++++++++++------- tests/test_rejected_write_pooled_client.py | 92 ++++++- 4 files changed, 281 insertions(+), 142 deletions(-) diff --git a/api/helpers.py b/api/helpers.py index 64dbafde..9de52ba5 100644 --- a/api/helpers.py +++ b/api/helpers.py @@ -220,15 +220,6 @@ def arm_connection_close(handler) -> None: pass -# Codings that frame nothing, so a rejection may keep the connection alive. -# Members MUST stay lowercase ASCII tokens: membership is tested through -# ``str.lower()``, which maps some non-ASCII characters ONTO ASCII ones -# (``'K'.lower() == 'k'``), so a coding here containing such a letter would -# be reachable by a spelling no HTTP token grammar allows. ``identity`` has no -# such letter -- ``test_unframed_transfer_codings_cannot_be_spelled_non_ascii`` -# proves it mechanically and will fail if a foldable coding is ever added. -_UNFRAMED_TRANSFER_CODINGS = frozenset({'identity'}) - # What ``unsupported_transfer_encoding()`` reports for a header that declares no # coding at all (``Transfer-Encoding:``). A placeholder rather than the raw '' so # the return value is never a falsy non-``None`` — callers test ``is not None``, @@ -349,11 +340,11 @@ def request_declares_body(handler) -> bool: kills healthy keep-alive on body-less writes. A blank, unparseable or self-contradicting ``Content-Length`` counts as a - declared body, as does a blank ``Transfer-Encoding``: a value we cannot - interpret does not prove the body's absence, and the safe assumption for - connection reuse is that bytes are pending. Only framing that positively says - "no body" -- no length header at all, or every declared length agreeing on - zero -- keeps the connection alive. + declared body, as does ANY ``Transfer-Encoding`` -- ``identity`` and a blank + value included: a value we cannot interpret does not prove the body's absence, + and the safe assumption for connection reuse is that bytes are pending. Only + framing that positively says "no body" -- no framing header at all, or every + declared length agreeing on zero -- keeps the connection alive. """ if unsupported_transfer_encoding(handler) is not None: return True @@ -366,38 +357,39 @@ def request_declares_body(handler) -> bool: def unsupported_transfer_encoding(handler) -> str | None: """Return the transfer coding this server cannot decode, or ``None``. - ``BaseHTTPRequestHandler`` never decodes ``Transfer-Encoding``: it hands - every reader a raw ``rfile``, so a chunked body reads back as its literal - chunk framing while a ``Content-Length``-based reader sees length 0 and - consumes nothing at all. Such a request can only be rejected -- and only - with the connection armed for close, because its chunk bytes are still - queued on the socket. + EVERY present value counts, with no exemption for any coding. + ``BaseHTTPRequestHandler`` decodes none of them: it hands every reader a raw + ``rfile``, so a chunked body reads back as its literal chunk framing while a + ``Content-Length``-based reader sees length 0 and consumes nothing at all. + RFC 9112 section 6.3 says the same thing normatively -- a request whose final + transfer coding is not ``chunked`` has a body length the recipient cannot + determine, so it must be refused and the connection closed. Such a request + can therefore only be rejected, and only with the connection armed for close, + because its payload bytes are still queued on the socket. - Every declared coding is inspected, not just the first: a request may repeat - the header, and one undecodable coding anywhere is enough to refuse. + ``identity`` used to be exempt as a coding that "frames nothing". It is not: + it names no framing either, and RFC 7230 removed it from the transfer codings + altogether. A sidecar rejection then answered 403 with keep-alive and the + payload was parsed as the next request line + (``400 Bad request syntax ('{...}GET /api/health/agent HTTP/1.1')``), and all + four upload handlers fell through to a length-0 read, answered 400 "No file + field in request" and left the multipart bytes on the socket + (``400 Bad request syntax ('--x')``). Nothing legitimate is lost: no client + sends a transfer coding this server could have honoured. - A BLANK ``Transfer-Encoding:`` is undecodable too, and used to read as - "unframed" alongside ``identity``: a header that names no coding cannot tell a - reader whether a body follows or how it is framed, and payload bytes behind it - poisoned the next pooled request exactly as chunked framing did. ``''`` only - ever reached the unframed set from an explicitly empty header -- an absent one - yields no values to loop over -- so treating it as a coding we cannot decode - costs a real client nothing. + A header present but EMPTY (``Transfer-Encoding:``) is reported through + ``_BLANK_TRANSFER_CODING`` rather than as ``''``, so callers testing + ``is not None`` and the 411 message both read right. Only an ABSENT header + yields no values at all -- absence is something only the wire can say. - Only SP/HTAB is trimmed, for the same reason the length parse trims only - those: ``str.strip()`` erases U+00A0/U+0085 as well, so ``\\xa0identity`` -- - which no token grammar allows -- read back as plain ``identity`` and its - payload poisoned the next pooled request (403 with no ``Connection: close``, - then the bad-syntax 400). A coding padded with a character the grammar does - not permit is a coding we cannot decode. + The first value is enough precisely because no value is acceptable; it is + reported as-is (SP/HTAB trimmed, the one padding RFC 9110 allows) purely so + the rejection message names what arrived. """ - for raw in _framing_header_values(handler, 'Transfer-Encoding'): - normalized = raw.strip(_FIELD_VALUE_OWS) - if not normalized: - return _BLANK_TRANSFER_CODING - if normalized.lower() not in _UNFRAMED_TRANSFER_CODINGS: - return normalized - return None + values = _framing_header_values(handler, 'Transfer-Encoding') + if not values: + return None + return values[0].strip(_FIELD_VALUE_OWS) or _BLANK_TRANSFER_CODING def arm_connection_close_if_body_pending(handler) -> bool: diff --git a/api/upload.py b/api/upload.py index 517ce835..4174b389 100644 --- a/api/upload.py +++ b/api/upload.py @@ -82,12 +82,15 @@ def _read_multipart_or_reject(handler): Framing is judged from the headers, not from a present ``Content-Length``: - * ``Transfer-Encoding: chunked`` carries no Content-Length, so the old code - read it as length 0, found no parts, and answered "No file field in - request" with keep-alive intact -- leaving every chunk byte on the socket - to poison the next pooled request. ``http.server`` cannot decode chunked - framing at all (``rfile`` is the raw socket), so 411 is the only honest - answer. + * ANY ``Transfer-Encoding`` is refused, ``identity`` included: ``http.server`` + decodes none of them (``rfile`` is the raw socket) and RFC 9112 section 6.3 + says a request whose final coding is not ``chunked`` has a length no + recipient can determine. A coded request carries no usable Content-Length, + so the old code read it as length 0, found no parts, and answered "No file + field in request" with keep-alive intact -- leaving every payload byte on + the socket to poison the next pooled request. A Content-Length sent BESIDE a + coding does not rescue it: that pairing is the smuggling shape 6.3 rejects, + and the coding still hides how the body is framed. * a non-numeric or oversized Content-Length cannot be read either, and ``parse_multipart()`` validates the boundary and the length before its first ``rfile.read()`` — so every raise below is still pre-read. @@ -100,7 +103,8 @@ def _read_multipart_or_reject(handler): coding = unsupported_transfer_encoding(handler) if coding is not None: raise _RejectBeforeRead( - f'Unsupported Transfer-Encoding: {coding} (uploads require a Content-Length)', + f'Unsupported Transfer-Encoding: {coding} ' + '(uploads must send an undecoded body with a Content-Length)', 411, ) if unreadable_content_length(handler): diff --git a/tests/test_rejected_write_connection_close.py b/tests/test_rejected_write_connection_close.py index 8fdfd7bf..f5e80ce5 100644 --- a/tests/test_rejected_write_connection_close.py +++ b/tests/test_rejected_write_connection_close.py @@ -199,11 +199,22 @@ def test_bodyless_write_method_rejection_keeps_connection(method, headers, monke ({"content-length": "7"}, True), ({"Transfer-Encoding": "chunked"}, True), ({"transfer-encoding": "Chunked"}, True), - ({"Transfer-Encoding": "identity"}, False), - ({"Transfer-Encoding": "IDENTITY "}, False), # a token is case-insensitive + # No coding is exempt: http.server decodes none of them (RFC 9112 6.3). + ({"Transfer-Encoding": "identity"}, True), + ({"Transfer-Encoding": "IDENTITY "}, True), + ({"Transfer-Encoding": "Identity"}, True), + ({"Transfer-Encoding": "\tidentity\t"}, True), + ({"Transfer-Encoding": "identity, chunked"}, True), + ({"Transfer-Encoding": "chunked, identity"}, True), + ({"Transfer-Encoding": "identity,identity"}, True), + ({"Transfer-Encoding": ","}, True), + ({"Transfer-Encoding": "banana"}, True), ({"Transfer-Encoding": ""}, True), ({"Transfer-Encoding": " "}, True), ({"Transfer-Encoding": "\xa0identity"}, True), # not the token `identity` + # A coding beside a length: the coding decides, whatever the length says. + ({"Transfer-Encoding": "identity", "Content-Length": "0"}, True), + ({"Transfer-Encoding": "identity", "Content-Length": "15"}, True), ], ) def test_request_declares_body_reads_the_framing_headers(headers, expected): @@ -268,15 +279,31 @@ def test_duplicate_agreeing_zero_content_length_declares_no_body(): assert request_declares_body(cast(Any, handler)) is False -def test_repeated_transfer_encoding_is_detected_beyond_the_first_value(): - """`identity` first must not mask a chunked coding behind it.""" - from api.helpers import unsupported_transfer_encoding +@pytest.mark.parametrize( + "raw", + [ + "Transfer-Encoding: identity\r\nTransfer-Encoding: chunked\r\n", + "Transfer-Encoding: chunked\r\nTransfer-Encoding: identity\r\n", + "Transfer-Encoding: identity\r\nTransfer-Encoding: identity\r\n", + "Transfer-Encoding: identity\r\ntransfer-encoding: identity\r\n", + ], + ids=["identity-then-chunked", "chunked-then-identity", "twice", "twice-mixed-case"], +) +def test_repeated_transfer_encoding_refuses_whatever_the_order(raw): + """A duplicated header cannot hide an undecodable coding behind an "unframed" one. - handler = _message_with( - "Host: x\r\nTransfer-Encoding: identity\r\nTransfer-Encoding: chunked\r\n" - ) + This used to have to look PAST a leading `identity` to find the chunked coding + (`Message.get()` returns only the first value, so it read as unframed and the + chunk bytes stayed queued). With no coding exempt the leading value already + refuses, so no ordering can mask anything -- which is what is pinned here, + rather than the specific coding a first-value read happened to miss. + """ + from api.helpers import request_declares_body, unsupported_transfer_encoding - assert unsupported_transfer_encoding(cast(Any, handler)) == "chunked" + handler = _message_with("Host: x\r\n" + raw) + + assert unsupported_transfer_encoding(cast(Any, handler)) is not None + assert request_declares_body(cast(Any, handler)) is True def test_unreadable_content_length_flags_conflict_and_garbage_only(): @@ -334,11 +361,12 @@ def test_sidecar_get_with_duplicate_content_length_closes_connection(monkeypatch # `Content-Length:\t` to the same '', so every blank spelling poisoned the socket # identically — as did a blank value duplicated, or paired with a real length. # -# A blank `Transfer-Encoding:` was the same hole one header over: '' sat in -# `_UNFRAMED_TRANSFER_CODINGS` beside `identity`, so a header naming no coding at -# all read as "no framing to worry about" and its payload poisoned the socket the -# same way (verified live: 403 then the same bad-syntax 400). Absence is -# something only the wire can say, and an absent header yields no values at all. +# A blank `Transfer-Encoding:` was the same hole one header over: '' reached the +# allowlist of codings that "framed nothing" (since deleted, along with the +# `identity` entry that was its last member), so a header naming no coding at all +# read as "no framing to worry about" and its payload poisoned the socket the same +# way (verified live: 403 then the same bad-syntax 400). Absence is something only +# the wire can say, and an absent header yields no values at all. _BLANK_FRAMING_HEADERS = [ @@ -390,10 +418,12 @@ def test_blank_transfer_encoding_is_reported_as_an_undecodable_coding(): assert coding is not None assert coding, "an empty return value would be a falsy non-None trap for callers" + # Only an ABSENT header frames nothing; a present one always names a coding + # this server cannot decode, `identity` included. assert unsupported_transfer_encoding(cast(Any, _message_with("Host: x\r\n"))) is None assert unsupported_transfer_encoding( cast(Any, _message_with("Host: x\r\nTransfer-Encoding: identity\r\n")) - ) is None + ) == "identity" @pytest.mark.parametrize("raw", _BLANK_FRAMING_HEADERS) @@ -427,14 +457,14 @@ def test_sidecar_get_with_blank_framing_value_closes_connection(raw, monkeypatch "Content-Length: 0\r\n", "Content-Length: 0 \r\n", "Content-Length: 0\r\nContent-Length: 0\r\n", - "Transfer-Encoding: identity\r\n", ], ) def test_blank_rule_does_not_over_close_a_bodyless_request(raw): """The other half of the contract: framing that says "no body" keeps keep-alive. - Two AGREEING zeroes, a padded zero and `identity` are honest statements that - nothing is queued, and none of them may be swept up by the blank rule. + No framing header at all, an OWS-padded zero and two AGREEING zeroes are + honest statements that nothing is queued, and none of them may be swept up by + the blank rule. """ from api.helpers import request_declares_body @@ -550,21 +580,69 @@ def test_absurdly_long_digit_run_is_unreadable_and_never_raises(): assert unreadable_content_length(cast(Any, handler)) is True -# ── The same audit one header over: spelling an undecodable coding ───────────── +# ── The same audit one header over: NO transfer coding is decodable here ─────── +# +# `identity` was exempt as a coding that "frames nothing", so `Transfer-Encoding: +# identity` plus payload bytes read as body-less: the sidecar rejection answered +# 403 with keep-alive and the payload became the next request line, and all four +# upload handlers fell through to a length-0 read. RFC 9112 6.3 is explicit -- +# a request whose final transfer coding is not `chunked` has a length the +# recipient cannot determine, so it must be refused and the connection closed -- +# and http.server decodes no coding at all, `chunked` included. Every spelling in +# the neighbourhood is swept here because three prior rounds each fixed one value +# of a framing rule and left an adjacent one poisoning the socket. +# +# The over-close half of this contract is pinned by the framing table above and by +# `test_blank_rule_does_not_over_close_a_bodyless_request` / +# `test_digit_grammar_does_not_over_close_an_honest_zero`: a request with NO +# framing header, or one whose declared lengths all agree on zero, keeps its +# keep-alive. An absent `Transfer-Encoding` is the only one that frames nothing. @pytest.mark.parametrize( "value", - ["\xa0identity", "identity\xa0", "\xa0identity\xa0", "\x85identity"], - ids=["nbsp-ahead", "nbsp-behind", "nbsp-both", "nel-ahead"], + [ + "identity", + "IDENTITY", + "Identity", + "identity ", + "\tidentity\t", + "\xa0identity", + "identity\xa0", + "\xa0identity\xa0", + "\x85identity", + "identity, chunked", + "chunked, identity", + "identity,identity", + ",", + "banana", + ], + ids=[ + "identity", + "upper", + "title", + "sp-padded", + "htab-padded", + "nbsp-ahead", + "nbsp-behind", + "nbsp-both", + "nel-ahead", + "identity-then-chunked", + "chunked-then-identity", + "identity-list", + "lone-comma", + "unknown-coding", + ], ) -def test_non_ascii_padded_identity_is_not_the_identity_coding(value): - """`str.strip()` erased U+00A0/U+0085, so `\\xa0identity` read back as `identity`. +def test_every_spelling_of_a_transfer_coding_declares_a_body(value): + """Case, OWS, non-OWS padding, comma lists, a bare comma, an unknown coding. - No token grammar allows the padding, so the value names no coding this server - can decode -- and it reached the wire: latin-1 request lines carry U+00A0 - fine. Live on the otherwise-fixed head, sidecar GET + payload: 403 with no - `Connection: close`, then `400 Bad request syntax ('{"stale": true}GET ...')`. + The ASCII `identity` spellings are the newly-closed hole (they normalized to + the exempt token); the U+00A0/U+0085-padded ones were the previous round's + (`str.strip()` erased the padding, so `\\xa0identity` read back as `identity`) + and they reach the wire because a request line decodes as latin-1. The comma + forms never matched the exempt token and already refused -- they are pinned so + a future normalizer that splits the list cannot reopen the hole. """ from api.helpers import request_declares_body, unsupported_transfer_encoding @@ -574,44 +652,23 @@ def test_non_ascii_padded_identity_is_not_the_identity_coding(value): assert request_declares_body(cast(Any, handler)) is True -@pytest.mark.parametrize( - "value", ["identity", "IDENTITY", "Identity", "identity ", "\tidentity\t"] -) -def test_ascii_case_and_ows_spellings_of_identity_stay_decodable(value): - """The over-close half of the coding audit: a real token still frames nothing.""" - from api.helpers import unsupported_transfer_encoding +@pytest.mark.parametrize("length", ["0", "15"], ids=["zero-length", "real-length"]) +def test_transfer_encoding_beside_a_content_length_still_declares_a_body(length): + """A coding plus a length is unframeable however honest the length looks. - handler = _message_with(f"Host: x\r\nTransfer-Encoding: {value}\r\n") - - assert unsupported_transfer_encoding(cast(Any, handler)) is None - - -def test_unframed_transfer_codings_cannot_be_spelled_non_ascii(): - """Pin WHY membership-by-`str.lower()` is safe, so a future coding can't break it. - - `str.lower()` maps some non-ASCII characters onto ASCII ones -- U+212A KELVIN - SIGN lowercases to `k`, so `'chunKed'.lower() == 'chunked'` -- which means a - coding in the unframed set containing such a letter would be reachable by a - spelling no HTTP token allows, and would read as "frames nothing" for a - request whose bytes are still queued. `identity` has no such letter. This - scans every codepoint rather than trusting the claim. + `Transfer-Encoding: identity` with `Content-Length: 0` was the worst of the + pair: both halves read as "no body", so the rejection kept the socket and the + payload behind them was parsed as the next request line. RFC 9112 6.3 refuses + the combination outright (it is the request-smuggling shape), and the coding + has to win -- a length cannot describe a body the server cannot decode. """ - from api.helpers import _UNFRAMED_TRANSFER_CODINGS + from api.helpers import request_declares_body - letters = set("".join(_UNFRAMED_TRANSFER_CODINGS)) - assert all( - coding.isascii() and coding.islower() for coding in _UNFRAMED_TRANSFER_CODINGS - ), _UNFRAMED_TRANSFER_CODINGS - foldable = [ - (hex(cp), chr(cp).lower()) - for cp in range(0x110000) - if not chr(cp).isascii() and chr(cp).lower() and set(chr(cp).lower()) <= letters - ] - assert not foldable, ( - f"a non-ASCII character now lowercases into {sorted(letters)}, so an " - f"unframed coding could be spelled outside the token grammar: {foldable}" + handler = _message_with( + f"Host: x\r\nTransfer-Encoding: identity\r\nContent-Length: {length}\r\n" ) - assert "chunKed".lower() == "chunked", "the fold this test guards against" + + assert request_declares_body(cast(Any, handler)) is True def test_csp_report_rate_limited_closes_connection(monkeypatch): @@ -808,37 +865,43 @@ def test_malformed_zero_content_length_multipart_rejects_before_read( @pytest.mark.parametrize("handler_name", _MULTIPART_HANDLERS) -def test_non_ascii_padded_identity_multipart_rejects_before_read(handler_name, monkeypatch): - """`Transfer-Encoding: \\xa0identity` is not the `identity` token either. +@pytest.mark.parametrize( + "raw", + [ + "Transfer-Encoding: identity", + "Transfer-Encoding: IDENTITY ", + "Transfer-Encoding: \xa0identity", + "Transfer-Encoding:", + "Transfer-Encoding: identity\r\nContent-Length: 0", + "Transfer-Encoding: identity\r\nContent-Length: 88", + "Transfer-Encoding: banana", + ], + ids=[ + "identity", + "identity-cased-and-padded", + "nbsp-padded-identity", + "blank", + "identity-plus-zero-length", + "identity-plus-real-length", + "unknown-coding", + ], +) +def test_any_transfer_encoding_multipart_rejects_before_read(handler_name, raw, monkeypatch): + """No coding is decodable by an upload handler, so none may reach `rfile`. - `str.strip()` erased the U+00A0, so it read as unframed, fell through to the - length-0 read and answered 400 "No file field in request" with the payload - still queued (live: 400 then `400 Bad request syntax ('--x')`). + `identity` (any case, any OWS padding) was exempt as a coding that "frames + nothing", so each handler fell through to the length-0 read, answered 400 "No + file field in request" with keep-alive and left the whole multipart payload on + the socket (live: 400 then `400 Bad request syntax ('--x')`). The `\\xa0`-padded + spelling and the blank value are the previous round's holes, and the two + Content-Length pairings are the RFC 9112 6.3 smuggling shape -- the coding has + to win over any length beside it. `_NeverRead` proves the preflight answers + before a byte is consumed. """ from email.parser import Parser headers = Parser().parsestr( - "Content-Type: multipart/form-data; boundary=x\r\nTransfer-Encoding: \xa0identity\r\n\r\n" - ) - handler, answered = _run_multipart_handler(monkeypatch, handler_name, headers, _NeverRead()) - - assert answered["status"] == 411, answered - assert "Transfer-Encoding" in answered["payload"]["error"], answered - assert handler.close_connection is True - - -@pytest.mark.parametrize("handler_name", _MULTIPART_HANDLERS) -def test_blank_transfer_encoding_multipart_rejects_before_read(handler_name, monkeypatch): - """`Transfer-Encoding:` names no coding, so it cannot frame a body either. - - It used to read as "unframed" beside `identity` and fell through to the - length-0 read: 400 "No file field in request", keep-alive, and the payload - left on the socket (live: 400 then `400 Bad request syntax ('--x')`). - """ - from email.parser import Parser - - headers = Parser().parsestr( - "Content-Type: multipart/form-data; boundary=x\r\nTransfer-Encoding:\r\n\r\n" + "Content-Type: multipart/form-data; boundary=x\r\n" + raw + "\r\n\r\n" ) handler, answered = _run_multipart_handler(monkeypatch, handler_name, headers, _NeverRead()) diff --git a/tests/test_rejected_write_pooled_client.py b/tests/test_rejected_write_pooled_client.py index d343a863..229059f9 100644 --- a/tests/test_rejected_write_pooled_client.py +++ b/tests/test_rejected_write_pooled_client.py @@ -210,8 +210,9 @@ def test_duplicate_content_length_upload_closes_and_cannot_poison_the_socket(pat # sidecar GET -> 403, then 400 Bad request syntax ('{"stale": true}GET /...') # /api/upload -> 400, then 400 Bad request syntax ('--x') # `Message` collapses `Content-Length:` and `Content-Length: ` to the same '', -# and a blank `Transfer-Encoding:` sat in the unframed set beside `identity`, so -# each spelling below poisoned the socket the same way. +# and a blank `Transfer-Encoding:` reached the allowlist of codings that "framed +# nothing" (since deleted — see the identity section below), so each spelling +# below poisoned the socket the same way. _BLANK_FRAMING_HEADER_LINES = ( b"Content-Length:\r\n", @@ -319,6 +320,89 @@ def test_malformed_zero_framing_upload_closes_and_cannot_poison_the_socket(path, _assert_single_closed_response(answered, status, _MULTIPART_PAYLOAD) +# ── `Transfer-Encoding: identity` frames nothing a reader can trust ─────────── +# +# `identity` sat in an allowlist of codings that "frame nothing", so a request +# carrying it read back as body-less however many payload bytes followed. RFC +# 9112 §6.3 leaves no room for that: a request whose final transfer coding is not +# `chunked` cannot be framed by a recipient, so it must be refused and the +# connection closed — and `http.server` decodes no transfer coding at all, so +# `identity` is no more readable here than `chunked` is. Reproduced live on the +# otherwise-fixed head, pipelined down one socket: +# +# GET /api/extensions/probe/sidecar/ping `Transfer-Encoding: identity` +# {"stale": true} +# -> HTTP/1.1 403 Forbidden (no Connection: close) +# -> HTTP/1.1 400 Bad request syntax +# ('{"stale": true}GET /api/health/agent HTTP/1.1') +# POST /api/upload `Transfer-Encoding: identity` --x... +# -> HTTP/1.1 400 Bad Request (no Connection: close, "No file field") +# -> HTTP/1.1 400 Bad request syntax ('--x') +# +# Only the spellings that normalize to the bare token were poisoned — every +# neighbouring value (`identity, chunked`, `identity,identity`, a lone comma, an +# unknown coding, a non-OWS-padded token, a blank value) already missed the +# allowlist and already closed. Those rows are in the sweep below anyway: this is +# the fourth round in which an adjacent spelling of one framing rule survived a +# fix, so the whole neighbourhood gets pinned rather than just the reported case. + +_IDENTITY_FRAMING_LINES = [ + b"Transfer-Encoding: identity\r\n", + b"Transfer-Encoding: IDENTITY\r\n", + b"Transfer-Encoding: Identity\r\n", + b"Transfer-Encoding: identity \r\n", + b"Transfer-Encoding: \tidentity\t\r\n", + b"Transfer-Encoding: identity\r\nTransfer-Encoding: identity\r\n", + b"Transfer-Encoding: identity\r\nContent-Length: 0\r\n", + b"Transfer-Encoding: identity\r\nContent-Length: " + str(len(_BODY)).encode() + b"\r\n", + b"Transfer-Encoding: identity, chunked\r\n", + b"Transfer-Encoding: chunked, identity\r\n", + b"Transfer-Encoding: identity,identity\r\n", + b"Transfer-Encoding: ,\r\n", + b"Transfer-Encoding: banana\r\n", +] + + +@pytest.mark.parametrize("framing", _IDENTITY_FRAMING_LINES) +def test_identity_framing_sidecar_get_closes_and_cannot_poison_the_socket(framing): + answered = _pipelined_after( + b"GET /api/extensions/probe/sidecar/ping HTTP/1.1\r\n" + b"Host: 127.0.0.1\r\n" + b"Content-Type: application/json\r\n" + framing + b"\r\n" + _BODY + ) + + _assert_single_closed_response(answered, b"403", _BODY) + + +@pytest.mark.parametrize("path", _MULTIPART_UPLOAD_PATHS) +@pytest.mark.parametrize( + "framing", + [ + b"Transfer-Encoding: identity\r\n", + b"Transfer-Encoding: IDENTITY \r\n", + b"Transfer-Encoding: identity\r\nContent-Length: 0\r\n", + ], + ids=["identity", "identity-cased-and-padded", "identity-plus-zero-length"], +) +def test_identity_framing_upload_closes_and_cannot_poison_the_socket(path, framing): + """All four multipart handlers, mirroring the chunked cases above. + + `identity` carries no Content-Length either, so the length-0 read found no + parts and each handler answered 400 "No file field in request" with the whole + multipart payload still queued on an open socket. + """ + answered = _pipelined_after( + f"POST {path} HTTP/1.1\r\n" + "Host: 127.0.0.1\r\n" + "Content-Type: multipart/form-data; boundary=x\r\n".encode() + + framing + + b"\r\n" + + _MULTIPART_PAYLOAD + ) + + _assert_single_closed_response(answered, b"411", _MULTIPART_PAYLOAD) + + @pytest.mark.parametrize( "framing", [ @@ -327,8 +411,6 @@ def test_malformed_zero_framing_upload_closes_and_cannot_poison_the_socket(path, b"Content-Length: 00\r\n", b"Content-Length: 0 \r\n", b"Content-Length: 0\r\nContent-Length: 0\r\n", - b"Transfer-Encoding: identity\r\n", - b"Transfer-Encoding: IDENTITY \r\n", ], ids=[ "no-length", @@ -336,8 +418,6 @@ def test_malformed_zero_framing_upload_closes_and_cannot_poison_the_socket(path, "double-zero-length", "ows-padded-zero", "two-agreeing-zeroes", - "identity", - "identity-cased-and-padded", ], ) def test_bodyless_framing_keeps_the_pooled_socket_alive(framing):