diff --git a/api/helpers.py b/api/helpers.py index 0073972f..64dbafde 100644 --- a/api/helpers.py +++ b/api/helpers.py @@ -220,6 +220,13 @@ 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 @@ -228,6 +235,14 @@ _UNFRAMED_TRANSFER_CODINGS = frozenset({'identity'}) # and an empty string would also render as a hole in the rejection message. _BLANK_TRANSFER_CODING = '(blank)' +# The only whitespace a field value may be padded with: RFC 9110 OWS is SP/HTAB +# and nothing else. ``str.strip()`` would also erase U+00A0 and U+0085 -- both +# reachable over the wire, because request lines decode as latin-1 -- and +# ``Content-Length: \xa00`` would then read back as a genuine ``0`` while its +# payload sat unread on the socket. Padding a value with a character the grammar +# does not allow makes it unreadable, not zero. +_FIELD_VALUE_OWS = ' \t' + def _framing_header_values(handler, name: str) -> list[str]: """Every value the wire carried for one framing header, tolerating stubs. @@ -281,11 +296,27 @@ def _declared_content_lengths(handler) -> set[int] | None: anything that *is* here and does not parse has to be treated as pending bytes, exactly like ``banana`` or two lengths that disagree. Note ``Message`` collapses ``Content-Length:`` and ``Content-Length: `` to the same ``''``. + + "Parses" means RFC 9110's ``Content-Length = 1*DIGIT``: an unsigned run of + ASCII digits, nothing else. ``int()`` is far more generous than the grammar -- + it accepts a leading sign, PEP 515 underscores and non-ASCII digits -- and + every such spelling of ZERO used to read back as an honest ``0`` and take the + keep-alive branch below with its payload still queued: ``Content-Length: +0`` + (also ``-0``, ``0_0``, ``+00``, ``\\xa00``) answered 403 with no + ``Connection: close``, then ``400 Bad request syntax ('{...}GET /api/...')`` + -- the same trace as a blank value. A sign is only invisible on a NON-zero + value, where the wrong parse happens to close anyway. Anything the grammar + rejects is unreadable framing, which means a body may be pending. + + The ``int()`` guard stays even though ``isdigit()`` has already vetted every + character: a digit run longer than ``sys.get_int_max_str_digits()`` (4300) + raises ``ValueError`` too, and this runs on rejection paths where an + exception becomes a spurious 500. """ parsed: set[int] = set() for raw in _framing_header_values(handler, 'Content-Length'): - stripped = raw.strip() - if not stripped: + stripped = raw.strip(_FIELD_VALUE_OWS) + if not (stripped.isascii() and stripped.isdigit()): return None try: parsed.add(int(stripped)) @@ -352,9 +383,16 @@ def unsupported_transfer_encoding(handler) -> str | None: 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. + + 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. """ for raw in _framing_header_values(handler, 'Transfer-Encoding'): - normalized = raw.strip() + normalized = raw.strip(_FIELD_VALUE_OWS) if not normalized: return _BLANK_TRANSFER_CODING if normalized.lower() not in _UNFRAMED_TRANSFER_CODINGS: diff --git a/tests/test_rejected_write_connection_close.py b/tests/test_rejected_write_connection_close.py index e09bf30b..8fdfd7bf 100644 --- a/tests/test_rejected_write_connection_close.py +++ b/tests/test_rejected_write_connection_close.py @@ -172,7 +172,9 @@ def test_bodyless_write_method_rejection_keeps_connection(method, headers, monke [ ({}, False), ({"Content-Length": "0"}, False), + ({"Content-Length": "00"}, False), # 1*DIGIT, so an honest zero ({"Content-Length": " 0 "}, False), + ({"Content-Length": "0\t"}, False), ({"Content-Length": ""}, True), ({"Content-Length": " "}, True), ({"Content-Length": "\t"}, True), @@ -181,12 +183,27 @@ def test_bodyless_write_method_rejection_keeps_connection(method, headers, monke ({"Content-Length": "banana"}, True), ({"Content-Length": "0x5"}, True), ({"Content-Length": "+5"}, True), + # int() is looser than 1*DIGIT, and every one of these reads as ZERO -- + # where the wrong parse is not merely wrong but silently keep-alive. + ({"Content-Length": "+0"}, True), + ({"Content-Length": "-0"}, True), + ({"Content-Length": "0_0"}, True), + ({"Content-Length": "+00"}, True), + ({"Content-Length": "0_0_0"}, True), + ({"Content-Length": "٠0"}, True), # ARABIC-INDIC ZERO ahead of an ASCII one + ({"Content-Length": "٠"}, True), # ARABIC-INDIC DIGIT ZERO: int() -> 0 + ({"Content-Length": "０"}, True), # FULLWIDTH DIGIT ZERO: int() -> 0 + ({"Content-Length": "٥"}, True), # ARABIC-INDIC DIGIT FIVE: int() -> 5 + ({"Content-Length": "\xa00"}, True), # NBSP-padded zero: int() strips it + ({"Content-Length": "0\x85"}, True), # NEL-padded zero: likewise ({"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 ({"Transfer-Encoding": ""}, True), ({"Transfer-Encoding": " "}, True), + ({"Transfer-Encoding": "\xa0identity"}, True), # not the token `identity` ], ) def test_request_declares_body_reads_the_framing_headers(headers, expected): @@ -424,6 +441,179 @@ def test_blank_rule_does_not_over_close_a_bodyless_request(raw): assert request_declares_body(cast(Any, _message_with("Host: x\r\n" + raw))) is False +# ── A length that only `int()` calls zero is unreadable, not zero ────────────── +# +# The blank rule above closed the "no value" hole; this one closes the "value +# `int()` is too generous about" hole beside it. RFC 9110 is +# `Content-Length = 1*DIGIT` -- an unsigned run of ASCII digits -- but `int()` +# also accepts a leading sign, PEP 515 underscores and non-ASCII digits, and +# strips non-ASCII whitespace. Every spelling of ZERO below therefore parsed to +# an honest `0`, took the "every declared length agrees on zero" keep-alive +# branch, and left the payload queued. Reproduced live on the otherwise-fixed +# head, pipelined down one socket: +# +# GET /api/extensions/probe/sidecar/ping `Content-Length: +0` {"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') +# +# byte-for-byte the trace the blank value produced, and on /api/upload a 400 +# then `400 Bad request syntax ('--x')`. +# +# A sign is only harmless on a NON-zero value (`+5` parses to 5, declares a body +# and closes anyway), which is exactly why the family looked safe: the mistake is +# invisible until the value is zero. +# +# Non-ASCII digits are unreadable by the same rule. An HTTP/1.1 request line +# decodes as latin-1, so U+0660 cannot cross the wire in a header value (only +# U+00B2/U+00B3/U+00B9 -- superscripts, which `int()` already rejects -- and the +# U+0085/U+00A0 whitespace `int()` strips can); these helpers take any headers +# mapping, though, so the grammar is enforced here rather than relying on the +# transport to filter. + +_MALFORMED_ZERO_LENGTHS = [ + "+0", + "-0", + "0_0", + "+00", + "0_0_0", + "\xa00", # NBSP-padded zero: str.strip()/int() erase U+00A0 + "0\x85", # NEL-padded zero: likewise U+0085 + "٠", # ARABIC-INDIC DIGIT ZERO -- isdigit() and int() both accept it + "０", # FULLWIDTH DIGIT ZERO + "𝟢", # MATHEMATICAL MONOSPACE DIGIT ZERO +] + + +@pytest.mark.parametrize("value", _MALFORMED_ZERO_LENGTHS) +def test_malformed_zero_content_length_declares_a_body(value): + """On a real email.message.Message, as the production handler carries.""" + from api.helpers import request_declares_body + + handler = _message_with(f"Host: x\r\nContent-Length: {value}\r\n") + + assert request_declares_body(cast(Any, handler)) is True + + +@pytest.mark.parametrize("value", _MALFORMED_ZERO_LENGTHS) +def test_malformed_zero_content_length_is_unreadable(value): + """The upload preflight gate must refuse it too -- it is not a length.""" + from api.helpers import unreadable_content_length + + handler = _message_with(f"Host: x\r\nContent-Length: {value}\r\n") + + assert unreadable_content_length(cast(Any, handler)) is True + + +@pytest.mark.parametrize("value", _MALFORMED_ZERO_LENGTHS) +def test_sidecar_get_with_a_malformed_zero_length_closes_connection(value, monkeypatch): + """Through the production provenance rejection, not just the helper.""" + handler = _rejected_sidecar_call( + monkeypatch, "GET", _message_with(f"Host: x\r\nContent-Length: {value}\r\n").headers + ) + + assert handler.close_connection is True + + +@pytest.mark.parametrize( + "value", + ["0", "00", "000", " 0 ", "0\t", "\t0", "0\r\nContent-Length: 0"], + ids=["zero", "double-zero", "triple-zero", "padded", "tab-behind", "tab-ahead", "two-agreeing"], +) +def test_digit_grammar_does_not_over_close_an_honest_zero(value): + """The over-close half: `1*DIGIT` zeroes, SP/HTAB padding and all, stay alive. + + `00` is as valid as `0` (`1*DIGIT` sets no leading-zero rule), and RFC 9110 + OWS around a field value is SP/HTAB -- so trimming those must not become + trimming everything `str.strip()` would. + """ + from api.helpers import request_declares_body, unreadable_content_length + + handler = _message_with(f"Host: x\r\nContent-Length: {value}\r\n") + + assert request_declares_body(cast(Any, handler)) is False + assert unreadable_content_length(cast(Any, handler)) is False + + +def test_absurdly_long_digit_run_is_unreadable_and_never_raises(): + """`1*DIGIT` is necessary but not sufficient: `int()` caps the digit count. + + A run longer than `sys.get_int_max_str_digits()` (4300) raises ValueError, and + this runs on rejection paths where an exception becomes a spurious 500 -- so + the parse stays guarded even though every character is now vetted first. + """ + from api.helpers import request_declares_body, unreadable_content_length + + handler = _message_with("Host: x\r\nContent-Length: " + "0" * 5000 + "\r\n") + + assert request_declares_body(cast(Any, handler)) is True + assert unreadable_content_length(cast(Any, handler)) is True + + +# ── The same audit one header over: spelling an undecodable coding ───────────── + + +@pytest.mark.parametrize( + "value", + ["\xa0identity", "identity\xa0", "\xa0identity\xa0", "\x85identity"], + ids=["nbsp-ahead", "nbsp-behind", "nbsp-both", "nel-ahead"], +) +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`. + + 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 ...')`. + """ + from api.helpers import request_declares_body, unsupported_transfer_encoding + + handler = _message_with(f"Host: x\r\nTransfer-Encoding: {value}\r\n") + + assert unsupported_transfer_encoding(cast(Any, handler)) is not None + 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 + + 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. + """ + from api.helpers import _UNFRAMED_TRANSFER_CODINGS + + 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}" + ) + assert "chunKed".lower() == "chunked", "the fold this test guards against" + + def test_csp_report_rate_limited_closes_connection(monkeypatch): """A rate-limited CSP report is dropped before its body is read.""" import api.routes as routes @@ -594,6 +784,49 @@ def test_blank_content_length_multipart_rejects_before_read(handler_name, raw, m assert handler.close_connection is True +@pytest.mark.parametrize("handler_name", _MULTIPART_HANDLERS) +@pytest.mark.parametrize("value", ["+0", "0_0", "\xa00", "٠"], ids=["sign", "underscore", "nbsp", "arabic-indic"]) +def test_malformed_zero_content_length_multipart_rejects_before_read( + handler_name, value, monkeypatch +): + """A length only `int()` calls zero slipped the gate into `int(...)` == 0. + + All four handlers then read an empty body, answered 400 "No file field in + request" with keep-alive, and left the whole payload on the socket. Live on + the otherwise-fixed head: 400 then `400 Bad request syntax ('--x')`. + """ + from email.parser import Parser + + headers = Parser().parsestr( + f"Content-Type: multipart/form-data; boundary=x\r\nContent-Length: {value}\r\n\r\n" + ) + handler, answered = _run_multipart_handler(monkeypatch, handler_name, headers, _NeverRead()) + + assert answered["status"] == 400, answered + assert answered["payload"]["error"] == "Invalid Content-Length", answered + assert handler.close_connection is True + + +@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. + + `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')`). + """ + 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. diff --git a/tests/test_rejected_write_pooled_client.py b/tests/test_rejected_write_pooled_client.py index 93d1a878..d343a863 100644 --- a/tests/test_rejected_write_pooled_client.py +++ b/tests/test_rejected_write_pooled_client.py @@ -256,15 +256,89 @@ def test_blank_framing_upload_closes_and_cannot_poison_the_socket(path, framing, _assert_single_closed_response(answered, status, _MULTIPART_PAYLOAD) +# ── Framing only `int()` reads as zero ──────────────────────────────────────── +# +# RFC 9110 is `Content-Length = 1*DIGIT`; `int()` also takes a sign, PEP 515 +# underscores and non-ASCII whitespace padding, so each value below parsed to an +# honest `0`, took the "every declared length agrees on zero" keep-alive branch +# and left the payload queued. Reproduced live on the otherwise-fixed head: +# sidecar GET -> 403 (no Connection: close), then +# 400 Bad request syntax ('{"stale": true}GET /api/health/agent HTTP/1.1') +# /api/upload -> 400, then 400 Bad request syntax ('--x') +# `Transfer-Encoding: \xa0identity` is the same mistake one header over: strip() +# erased the U+00A0 and it read back as the `identity` token. Everything here is +# wire-reachable -- a request line decodes as latin-1, so U+00A0 and U+0085 pass +# through untouched (a non-ASCII DIGIT such as U+0660 cannot, which is why that +# family is pinned at the helper level in test_rejected_write_connection_close.py). + +_MALFORMED_ZERO_FRAMING_LINES = [ + b"Content-Length: +0\r\n", + b"Content-Length: -0\r\n", + b"Content-Length: 0_0\r\n", + b"Content-Length: +00\r\n", + b"Content-Length: 0_0_0\r\n", + b"Content-Length: \xa00\r\n", + b"Content-Length: 0\x85\r\n", + b"Content-Length: 0\r\nContent-Length: +0\r\n", + b"Transfer-Encoding: \xa0identity\r\n", +] + + +@pytest.mark.parametrize("framing", _MALFORMED_ZERO_FRAMING_LINES) +def test_malformed_zero_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", "status"), + [ + (b"Content-Length: +0\r\n", b"400"), + (b"Content-Length: 0_0\r\n", b"400"), + (b"Content-Length: \xa00\r\n", b"400"), + (b"Transfer-Encoding: \xa0identity\r\n", b"411"), + ], + ids=["sign", "underscore", "nbsp-padded", "nbsp-padded-identity"], +) +def test_malformed_zero_framing_upload_closes_and_cannot_poison_the_socket(path, framing, status): + 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, status, _MULTIPART_PAYLOAD) + + @pytest.mark.parametrize( "framing", [ b"", b"Content-Length: 0\r\n", + 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", + "zero-length", + "double-zero-length", + "ows-padded-zero", + "two-agreeing-zeroes", + "identity", + "identity-cased-and-padded", ], - ids=["no-length", "zero-length", "two-agreeing-zeroes", "identity"], ) def test_bodyless_framing_keeps_the_pooled_socket_alive(framing): """The over-close half of the contract, on the wire.