diff --git a/api/helpers.py b/api/helpers.py index 495e4baa..7e9a4ccf 100644 --- a/api/helpers.py +++ b/api/helpers.py @@ -223,29 +223,70 @@ def arm_connection_close(handler) -> None: _UNFRAMED_TRANSFER_CODINGS = frozenset({'', 'identity'}) -def _framing_header(handler, name: str) -> str | None: - """Read one framing header, tolerating handler stubs and plain dicts. +def _framing_header_values(handler, name: str) -> list[str]: + """Every value the wire carried for one framing header, tolerating stubs. - A real handler carries an ``email.message.Message``, whose ``get()`` is - case-insensitive like the wire; the suite's ``SimpleNamespace(headers={...})`` + ALL values, not just the first: ``Message.get()`` returns only the first + occurrence, so a request carrying ``Content-Length: 0`` followed by + ``Content-Length: 42`` reads back as length zero. Its 42 body bytes are then + never consumed and get parsed as the next request line -- the same poisoning + this module exists to prevent, reached by duplicating a header rather than by + omitting one. Conflicting framing is only visible if every value is. + + A real handler carries an ``email.message.Message`` (case-insensitive + ``get_all()``, like the wire); the suite's ``SimpleNamespace(headers={...})`` doubles are plain dicts, which are not, so fall back to a case-insensitive scan for those. A missing or odd ``headers`` must never raise: every caller is on a rejection path where an exception becomes a spurious 500. """ headers = getattr(handler, 'headers', None) if headers is None: - return None + return [] try: - value = headers.get(name) - if value is None and isinstance(headers, dict): + get_all = getattr(headers, 'get_all', None) + if callable(get_all): + values = get_all(name) + return [] if values is None else [str(v) for v in values if v is not None] + if isinstance(headers, dict): lowered = name.lower() - for key, candidate in headers.items(): - if str(key).lower() == lowered: - value = candidate - break + return [ + str(v) for k, v in headers.items() + if str(k).lower() == lowered and v is not None + ] + value = headers.get(name) + return [] if value is None else [str(value)] except Exception: - return None - return None if value is None else str(value) + return [] + + +def _declared_content_lengths(handler) -> set[int] | None: + """Distinct declared ``Content-Length`` values, or ``None`` if any is unreadable. + + ``None`` means the framing cannot be trusted at all (a non-numeric value); + an empty set means no length was declared. + """ + parsed: set[int] = set() + for raw in _framing_header_values(handler, 'Content-Length'): + stripped = raw.strip() + if not stripped: + continue + try: + parsed.add(int(stripped)) + except ValueError: + return None + return parsed + + +def unreadable_content_length(handler) -> bool: + """True when ``Content-Length`` cannot be trusted to frame the body. + + Either a value that will not parse, or two or more values that disagree. + Neither can tell a reader how many bytes to drain, so such a request can + only be rejected -- with the connection armed for close, since bytes may + still be queued behind it. + """ + parsed = _declared_content_lengths(handler) + return parsed is None or len(parsed) > 1 def request_declares_body(handler) -> bool: @@ -259,20 +300,16 @@ def request_declares_body(handler) -> bool: "read" requests (whose unread bytes poison the next pooled request) and kills healthy keep-alive on body-less writes. - An unparseable ``Content-Length`` counts as a declared body: a value we - cannot interpret does not prove the body's absence, and the safe assumption - for connection reuse is that bytes are pending. + An unparseable or self-contradicting ``Content-Length`` counts as a declared + body: a value we cannot interpret does not prove the body's absence, and the + safe assumption for connection reuse is that bytes are pending. """ - coding = _framing_header(handler, 'Transfer-Encoding') - if coding is not None and coding.strip().lower() not in _UNFRAMED_TRANSFER_CODINGS: + if unsupported_transfer_encoding(handler) is not None: return True - raw_length = _framing_header(handler, 'Content-Length') - if raw_length is None or not raw_length.strip(): - return False - try: - return int(raw_length.strip()) != 0 - except ValueError: + parsed = _declared_content_lengths(handler) + if parsed is None: return True + return any(length != 0 for length in parsed) def unsupported_transfer_encoding(handler) -> str | None: @@ -284,14 +321,15 @@ def unsupported_transfer_encoding(handler) -> str | None: 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 declared coding is inspected, not just the first: a request may repeat + the header, and one undecodable coding anywhere is enough to refuse. """ - coding = _framing_header(handler, 'Transfer-Encoding') - if coding is None: - return None - normalized = coding.strip() - if normalized.lower() in _UNFRAMED_TRANSFER_CODINGS: - return None - return normalized + for raw in _framing_header_values(handler, 'Transfer-Encoding'): + normalized = raw.strip() + if normalized.lower() not in _UNFRAMED_TRANSFER_CODINGS: + return normalized + return None def arm_connection_close_if_body_pending(handler) -> bool: diff --git a/api/upload.py b/api/upload.py index 01a7f3f9..517ce835 100644 --- a/api/upload.py +++ b/api/upload.py @@ -8,7 +8,12 @@ import tempfile from pathlib import Path from api.config import MAX_UPLOAD_BYTES, STATE_DIR -from api.helpers import arm_connection_close, j, unsupported_transfer_encoding +from api.helpers import ( + arm_connection_close, + j, + unreadable_content_length, + unsupported_transfer_encoding, +) from api.models import get_session from api.profiles import _profiles_match, get_active_profile_name as _get_active_profile_name from api.workspace import ( @@ -86,6 +91,10 @@ def _read_multipart_or_reject(handler): * 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. + * two DISAGREEING Content-Length headers read back as whichever came first, + so ``Content-Length: 0`` ahead of the real length made the body invisible + and left every byte of it on the socket. There is no honest length to + drain, so the request is refused before ``rfile`` is touched. """ content_type = handler.headers.get('Content-Type', '') coding = unsupported_transfer_encoding(handler) @@ -94,6 +103,8 @@ def _read_multipart_or_reject(handler): f'Unsupported Transfer-Encoding: {coding} (uploads require a Content-Length)', 411, ) + if unreadable_content_length(handler): + raise _RejectBeforeRead('Invalid Content-Length', 400) try: content_length = int(handler.headers.get('Content-Length', 0) or 0) except (TypeError, ValueError): diff --git a/tests/test_rejected_write_connection_close.py b/tests/test_rejected_write_connection_close.py index 306a4408..29ef8e37 100644 --- a/tests/test_rejected_write_connection_close.py +++ b/tests/test_rejected_write_connection_close.py @@ -212,6 +212,89 @@ def test_request_declares_body_tolerates_a_handler_without_headers(): assert request_declares_body(cast(Any, SimpleNamespace(headers=object()))) is False +# ── A duplicated framing header hides the body from a first-value-only read ──── +# +# `Message.get()` returns only the FIRST occurrence, so `Content-Length: 0` +# ahead of the real length reads back as an empty body: nothing is drained and +# the real bytes are parsed as the next request line. Verified live against the +# running server before the fix -- 403 then +# `400 Bad request syntax ('{"stale": true}GET /api/health/agent HTTP/1.1')`, +# and on /api/upload a 400 then `400 Bad request syntax ('--x')`. + + +def _message_with(raw_headers: str): + """Build the email.message.Message a production handler really carries.""" + from email.parser import Parser + + return SimpleNamespace(headers=Parser().parsestr(raw_headers + "\r\n")) + + +def test_duplicate_conflicting_content_length_declares_a_body(): + from api.helpers import request_declares_body + + handler = _message_with("Host: x\r\nContent-Length: 0\r\nContent-Length: 15\r\n") + + assert request_declares_body(cast(Any, handler)) is True + + +def test_duplicate_agreeing_zero_content_length_declares_no_body(): + """Repetition alone is not a conflict -- two honest zeroes still frame nothing.""" + from api.helpers import request_declares_body + + handler = _message_with("Host: x\r\nContent-Length: 0\r\nContent-Length: 0\r\n") + + 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 + + handler = _message_with( + "Host: x\r\nTransfer-Encoding: identity\r\nTransfer-Encoding: chunked\r\n" + ) + + assert unsupported_transfer_encoding(cast(Any, handler)) == "chunked" + + +def test_unreadable_content_length_flags_conflict_and_garbage_only(): + from api.helpers import unreadable_content_length + + assert unreadable_content_length(cast(Any, _message_with("Content-Length: 5\r\n"))) is False + assert unreadable_content_length(cast(Any, _message_with("Host: x\r\n"))) is False + assert unreadable_content_length( + cast(Any, _message_with("Content-Length: 5\r\nContent-Length: 5\r\n")) + ) is False + assert unreadable_content_length( + cast(Any, _message_with("Content-Length: 0\r\nContent-Length: 9\r\n")) + ) is True + assert unreadable_content_length(cast(Any, _message_with("Content-Length: nope\r\n"))) is True + + +def test_sidecar_get_with_duplicate_content_length_closes_connection(monkeypatch): + """The conflicting-framing GET reaches the production provenance rejection.""" + import api.routes as routes + + from email.parser import Parser + + headers = Parser().parsestr("Host: x\r\nContent-Length: 0\r\nContent-Length: 15\r\n\r\n") + handler = SimpleNamespace( + path="/api/extensions/ext1/sidecar/proxy", + command="GET", + headers=headers, + close_connection=False, + ) + monkeypatch.setattr(routes, "_match_extension_sidecar_proxy_path", lambda _path: ("ext1", "/proxy")) + monkeypatch.setattr(routes, "_check_same_origin_browser_request", lambda _handler, **_: False) + monkeypatch.setattr(routes, "j", lambda _handler, _payload, status=200: None) + + routes._handle_extension_sidecar_proxy( + cast(Any, handler), SimpleNamespace(path=handler.path, query=""), "GET" + ) + + assert handler.close_connection is True + + 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 @@ -335,6 +418,28 @@ def test_non_numeric_content_length_still_closes(handler_name, monkeypatch): assert handler.close_connection is True +@pytest.mark.parametrize("handler_name", _MULTIPART_HANDLERS) +def test_duplicate_content_length_multipart_rejects_before_read(handler_name, monkeypatch): + """Disagreeing lengths give no honest byte count -- refuse without reading. + + Reading the first value (0) made the body invisible: the handler answered + "No file field in request" with keep-alive and left the real multipart bytes + on the socket, where the server parsed them as the next request line. + """ + from email.parser import Parser + + headers = Parser().parsestr( + "Content-Type: multipart/form-data; boundary=x\r\n" + "Content-Length: 0\r\n" + "Content-Length: 88\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 + + # ── Header serialization: exactly one Connection: close, never a crash ──────── # # The advertisement lives in Handler.end_headers() via diff --git a/tests/test_rejected_write_pooled_client.py b/tests/test_rejected_write_pooled_client.py index d1b348fd..e39eae2c 100644 --- a/tests/test_rejected_write_pooled_client.py +++ b/tests/test_rejected_write_pooled_client.py @@ -157,3 +157,40 @@ def test_sidecar_get_with_a_body_closes_and_cannot_poison_the_socket(): ) _assert_single_closed_response(answered, b"403", _BODY) + + +# ── Framing hidden behind a DUPLICATED header ───────────────────────────────── +# +# `Content-Length: 0` sent ahead of the real length: `Message.get()` returns only +# the first value, so every reader saw an empty body and drained nothing. Both +# cases below were reproduced live on the otherwise-fixed head, poisoning the +# socket exactly as the single-header cases used to: +# sidecar GET -> 403, then 400 Bad request syntax ('{"stale": true}GET /...') +# /api/upload -> 400, then 400 Bad request syntax ('--x') + + +def test_duplicate_content_length_sidecar_get_closes_and_cannot_poison_the_socket(): + answered = _pipelined_after( + "GET /api/extensions/probe/sidecar/ping HTTP/1.1\r\n" + "Host: 127.0.0.1\r\n" + "Content-Type: application/json\r\n" + "Content-Length: 0\r\n" + f"Content-Length: {len(_BODY)}\r\n" + "\r\n".encode() + _BODY + ) + + _assert_single_closed_response(answered, b"403", _BODY) + + +@pytest.mark.parametrize("path", _MULTIPART_UPLOAD_PATHS) +def test_duplicate_content_length_upload_closes_and_cannot_poison_the_socket(path): + 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" + "Content-Length: 0\r\n" + f"Content-Length: {len(_MULTIPART_PAYLOAD)}\r\n" + "\r\n".encode() + _MULTIPART_PAYLOAD + ) + + _assert_single_closed_response(answered, b"400", _MULTIPART_PAYLOAD)