From b3a031a19d8cce03ea7f6ae21b6e28832496ebc2 Mon Sep 17 00:00:00 2001 From: Rodrigo Gomes da Silva Date: Sat, 15 Aug 2026 15:17:23 -0300 Subject: [PATCH] fix(http): decide reject-before-read closes from the framing, not the method MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two framing gaps the re-gate reproduced on the production handlers, both the same class: something other than the wire was used to decide whether unread body bytes are queued. 1. api/routes.py — the sidecar provenance 403 gated its close on `read_request_body`, a static per-method flag. That is wrong in both directions: a GET carrying `Content-Length`/`Transfer-Encoding` was answered with keep-alive and its body then parsed as the next request line (`501 Unsupported method ('{}GET')`), while a body-less DELETE/PUT/PATCH had a healthy pooled connection closed for nothing. 2. api/upload.py — all four multipart handlers read framing from `Content-Length` alone, so a chunked upload (which has none) was treated as length zero, matched no parts and was rejected 400 "No file field in request" with keep-alive intact. Wire trace before the fix: HTTP/1.1 400 Bad Request <- the upload rejection HTTP/1.1 400 Bad request syntax ('54') <- our own chunk-size line New primitive in api/helpers.py: `request_declares_body()` (Content-Length present and non-zero, or any Transfer-Encoding other than identity; an unparseable length counts as present, since it cannot prove absence) plus `arm_connection_close_if_body_pending()` and `unsupported_transfer_encoding()`. api/upload.py now routes all four handlers through one preflight, `_read_multipart_or_reject()`, which refuses an undecodable transfer coding with 411 before touching rfile — http.server never decodes chunked framing, so that request can only be rejected — and keeps every previous status and message (413 oversize, 400 invalid Content-Length/boundary) with its close. Regressions: chunked reject that closes and a body-less reject that does not, at all four upload handlers and at the sidecar 403, plus two pipelined real-socket tests against the running server proving the follow-up request is never answered off a poisoned socket. --- api/helpers.py | 88 ++++++++ api/routes.py | 15 +- api/upload.py | 131 ++++++------ tests/test_rejected_write_connection_close.py | 198 ++++++++++++++++++ tests/test_rejected_write_pooled_client.py | 96 +++++++++ 5 files changed, 460 insertions(+), 68 deletions(-) diff --git a/api/helpers.py b/api/helpers.py index 79a6c8f3..495e4baa 100644 --- a/api/helpers.py +++ b/api/helpers.py @@ -220,6 +220,94 @@ def arm_connection_close(handler) -> None: pass +_UNFRAMED_TRANSFER_CODINGS = frozenset({'', 'identity'}) + + +def _framing_header(handler, name: str) -> str | None: + """Read one framing header, tolerating handler stubs and plain dicts. + + A real handler carries an ``email.message.Message``, whose ``get()`` is + case-insensitive 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 + try: + value = headers.get(name) + if value is None and isinstance(headers, dict): + lowered = name.lower() + for key, candidate in headers.items(): + if str(key).lower() == lowered: + value = candidate + break + except Exception: + return None + return None if value is None else str(value) + + +def request_declares_body(handler) -> bool: + """True when the request's framing declares a body that is still queued. + + Keyed on the wire, not on the method: ``Transfer-Encoding`` present (chunked + framing hides the length until the terminating chunk is read) or + ``Content-Length`` present and non-zero. The method proves nothing in either + direction -- a GET may carry a declared body and a POST/PUT/DELETE may carry + none -- and a static per-method flag therefore both misses body-bearing + "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. + """ + coding = _framing_header(handler, 'Transfer-Encoding') + if coding is not None and coding.strip().lower() not in _UNFRAMED_TRANSFER_CODINGS: + 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: + return True + + +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. + """ + 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 + + +def arm_connection_close_if_body_pending(handler) -> bool: + """Arm close for a reject-before-read, but only if a body was really declared. + + Returns whether the connection was armed. Use this instead of + ``arm_connection_close()`` wherever the rejection can also fire on a request + with no body: closing then would drop a pooled client's healthy connection + for no framing reason. + """ + if not request_declares_body(handler): + return False + arm_connection_close(handler) + return True + + def advertise_connection_close(handler) -> None: """Serialize ``close_connection`` into exactly one ``Connection: close`` header. diff --git a/api/routes.py b/api/routes.py index 92e5c617..decc18eb 100644 --- a/api/routes.py +++ b/api/routes.py @@ -2975,6 +2975,7 @@ from api.helpers import ( bad, safe_resolve, arm_connection_close, + arm_connection_close_if_body_pending, j, t, read_body, @@ -6067,12 +6068,14 @@ def _handle_extension_sidecar_proxy( # DELETE fell through the CSRF compatibility path that intentionally admits # non-browser clients, giving unsafe methods weaker provenance than GET. if not _check_same_origin_browser_request(handler, require_provenance=True): - # Provenance rejection runs before read_body(); when a body is pending, - # close-and-advertise rather than reusing an HTTP/1.1 connection whose - # unread body would corrupt the next request (same class as _check_csrf). - # GET (read_request_body=False) has nothing unread — keep-alive survives. - if read_request_body: - arm_connection_close(handler) + # Provenance rejection runs before read_body(), so close-and-advertise + # whenever the request DECLARED a body (Content-Length non-zero, or any + # Transfer-Encoding): those bytes are still queued in rfile and a reused + # HTTP/1.1 connection would parse them as the next request line (same + # class as _check_csrf). Gating on read_request_body instead was wrong in + # both directions — it missed a GET that carries a declared body, and it + # closed a healthy connection on a body-less DELETE/PUT/PATCH. + arm_connection_close_if_body_pending(handler) return j(handler, {"error": _csrf_rejection_error(handler)}, status=403) try: request_body = _read_body_bytes(handler) if read_request_body else None diff --git a/api/upload.py b/api/upload.py index 30abef3b..01a7f3f9 100644 --- a/api/upload.py +++ b/api/upload.py @@ -8,7 +8,7 @@ import tempfile from pathlib import Path from api.config import MAX_UPLOAD_BYTES, STATE_DIR -from api.helpers import arm_connection_close, j +from api.helpers import arm_connection_close, j, 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 ( @@ -51,6 +51,62 @@ def _reject_before_read(handler, message, status): return j(handler, {'error': message}, status=status) +class _RejectBeforeRead(ValueError): + """A rejection raised before any byte of the request body was consumed. + + Carries the status the caller must answer with. Subclasses ``ValueError`` so + that if one ever escapes an unusual call path, each handler's existing + ``except ValueError`` still answers 400 with the same message instead of + turning it into a 500. + """ + + def __init__(self, message: str, status: int): + super().__init__(message) + self.message = message + self.status = status + + +def _read_multipart_or_reject(handler): + """Framing preflight + body read, shared by all four multipart handlers. + + Returns ``(fields, files)``. Raises ``_RejectBeforeRead`` when the request + must be refused before its body is read -- the caller answers through + ``_reject_before_read()``, which arms ``Connection: close`` first, because the + declared body is still queued in ``rfile`` and a reused HTTP/1.1 connection + would parse those bytes as the next request line. + + 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. + * 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. + """ + content_type = handler.headers.get('Content-Type', '') + coding = unsupported_transfer_encoding(handler) + if coding is not None: + raise _RejectBeforeRead( + f'Unsupported Transfer-Encoding: {coding} (uploads require a Content-Length)', + 411, + ) + try: + content_length = int(handler.headers.get('Content-Length', 0) or 0) + except (TypeError, ValueError): + raise _RejectBeforeRead('Invalid Content-Length', 400) from None + if content_length > MAX_UPLOAD_BYTES: + raise _RejectBeforeRead( + f'File too large (max {MAX_UPLOAD_BYTES//1024//1024}MB)', 413) + try: + return parse_multipart(handler.rfile, content_type, content_length) + except ValueError as exc: + raise _RejectBeforeRead(str(exc), 400) from None + + def parse_multipart(rfile, content_type, content_length) -> tuple: import re as _re, email.parser as _ep # Imported locally (not just module-level) so the function stays @@ -212,22 +268,10 @@ def _write_office_upload_sidecar(workspace: Path, dest: Path, file_bytes: bytes) def handle_upload(handler): import traceback as _tb try: - content_type = handler.headers.get('Content-Type', '') try: - content_length = int(handler.headers.get('Content-Length', 0) or 0) - except (TypeError, ValueError): - # A non-numeric Content-Length is rejected before the body is read. - arm_connection_close(handler) - raise ValueError('Invalid Content-Length') from None - if content_length > MAX_UPLOAD_BYTES: - return _reject_before_read(handler, f'File too large (max {MAX_UPLOAD_BYTES//1024//1024}MB)', 413) - try: - fields, files = parse_multipart(handler.rfile, content_type, content_length) - except ValueError: - # parse_multipart validates Content-Length/boundary before reading; - # a ValueError here means the body was never consumed. - arm_connection_close(handler) - raise + fields, files = _read_multipart_or_reject(handler) + except _RejectBeforeRead as rejection: + return _reject_before_read(handler, rejection.message, rejection.status) session_id = fields.get('session_id', '') if 'file' not in files: return j(handler, {'error': 'No file field in request'}, status=400) @@ -413,22 +457,10 @@ def handle_upload_extract(handler): """Handle archive upload and extraction.""" import traceback as _tb try: - content_type = handler.headers.get('Content-Type', '') try: - content_length = int(handler.headers.get('Content-Length', 0) or 0) - except (TypeError, ValueError): - # A non-numeric Content-Length is rejected before the body is read. - arm_connection_close(handler) - raise ValueError('Invalid Content-Length') from None - if content_length > MAX_UPLOAD_BYTES: - return _reject_before_read(handler, f'File too large (max {MAX_UPLOAD_BYTES//1024//1024}MB)', 413) - try: - fields, files = parse_multipart(handler.rfile, content_type, content_length) - except ValueError: - # parse_multipart validates Content-Length/boundary before reading; - # a ValueError here means the body was never consumed. - arm_connection_close(handler) - raise + fields, files = _read_multipart_or_reject(handler) + except _RejectBeforeRead as rejection: + return _reject_before_read(handler, rejection.message, rejection.status) session_id = fields.get('session_id', '') if 'file' not in files: return j(handler, {'error': 'No file field in request'}, status=400) @@ -456,22 +488,10 @@ def handle_transcribe(handler): import traceback as _tb temp_path = None try: - content_type = handler.headers.get('Content-Type', '') try: - content_length = int(handler.headers.get('Content-Length', 0) or 0) - except (TypeError, ValueError): - # A non-numeric Content-Length is rejected before the body is read. - arm_connection_close(handler) - raise ValueError('Invalid Content-Length') from None - if content_length > MAX_UPLOAD_BYTES: - return _reject_before_read(handler, f'File too large (max {MAX_UPLOAD_BYTES//1024//1024}MB)', 413) - try: - fields, files = parse_multipart(handler.rfile, content_type, content_length) - except ValueError: - # parse_multipart validates Content-Length/boundary before reading; - # a ValueError here means the body was never consumed. - arm_connection_close(handler) - raise + fields, files = _read_multipart_or_reject(handler) + except _RejectBeforeRead as rejection: + return _reject_before_read(handler, rejection.message, rejection.status) if 'file' not in files: return j(handler, {'error': 'No file field in request'}, status=400) filename, file_bytes = files['file'] @@ -651,23 +671,10 @@ def handle_workspace_upload(handler): """ import traceback as _tb try: - content_type = handler.headers.get('Content-Type', '') try: - content_length = int(handler.headers.get('Content-Length', 0) or 0) - except (TypeError, ValueError): - # A non-numeric Content-Length is rejected before the body is read. - arm_connection_close(handler) - raise ValueError('Invalid Content-Length') from None - if content_length > MAX_UPLOAD_BYTES: - return _reject_before_read(handler, f'File too large (max {MAX_UPLOAD_BYTES//1024//1024}MB)', 413) - - try: - fields, files = parse_multipart(handler.rfile, content_type, content_length) - except ValueError: - # parse_multipart validates Content-Length/boundary before reading; - # a ValueError here means the body was never consumed. - arm_connection_close(handler) - raise + fields, files = _read_multipart_or_reject(handler) + except _RejectBeforeRead as rejection: + return _reject_before_read(handler, rejection.message, rejection.status) session_id = fields.get('session_id', '') subpath = fields.get('path', '') diff --git a/tests/test_rejected_write_connection_close.py b/tests/test_rejected_write_connection_close.py index a1bfc9a5..306a4408 100644 --- a/tests/test_rejected_write_connection_close.py +++ b/tests/test_rejected_write_connection_close.py @@ -8,6 +8,8 @@ The connection must close after those early rejections. from types import SimpleNamespace from typing import Any, cast +import pytest + def test_auth_rejected_write_closes_connection(monkeypatch): import api.auth as auth @@ -63,6 +65,7 @@ def test_sidecar_provenance_rejected_closes_connection(monkeypatch): handler = SimpleNamespace( path="/api/extensions/ext1/sidecar/proxy", command="POST", + headers={"Content-Length": "15"}, close_connection=False, ) monkeypatch.setattr(routes, "_match_extension_sidecar_proxy_path", lambda _path: ("ext1", "/proxy")) @@ -87,6 +90,7 @@ def test_sidecar_get_provenance_rejected_keeps_connection(monkeypatch): handler = SimpleNamespace( path="/api/extensions/ext1/sidecar/proxy", command="GET", + headers={}, close_connection=False, ) monkeypatch.setattr(routes, "_match_extension_sidecar_proxy_path", lambda _path: ("ext1", "/proxy")) @@ -98,6 +102,116 @@ def test_sidecar_get_provenance_rejected_keeps_connection(monkeypatch): assert handler.close_connection is False +# ── Framing, not the method, decides whether a rejection must close ────────── +# +# The re-gate found both halves of the same mistake: `read_request_body` is a +# static per-method flag, so it neither proves a body is pending (a body-less +# DELETE closed a healthy connection) nor proves one is absent (a GET carrying a +# declared body kept the connection and poisoned it). Both directions below run +# against the production handler. + + +def _rejected_sidecar_call(monkeypatch, method, headers, **kwargs): + """Drive a provenance-rejected sidecar proxy call; return the handler.""" + import api.routes as routes + + handler = SimpleNamespace( + path="/api/extensions/ext1/sidecar/proxy", + command=method, + 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=""), method, **kwargs + ) + return handler + + +@pytest.mark.parametrize( + "headers", + [ + {"Content-Length": "12"}, + {"Transfer-Encoding": "chunked"}, + {"Content-Length": "banana"}, # unreadable framing proves nothing is absent + ], + ids=["content-length", "chunked", "garbage-content-length"], +) +def test_sidecar_get_with_a_declared_body_closes_connection(headers, monkeypatch): + """A GET may still declare a body — those bytes are unread, so close. + + Reproduced by the gate as a 403 WITHOUT `Connection: close`, followed by + `501 Unsupported method ('{}GET')` on the same socket. + """ + handler = _rejected_sidecar_call(monkeypatch, "GET", headers) + + assert handler.close_connection is True + + +@pytest.mark.parametrize("method", ["POST", "PUT", "PATCH", "DELETE"]) +@pytest.mark.parametrize( + "headers", [{}, {"Content-Length": "0"}], ids=["no-content-length", "zero-content-length"] +) +def test_bodyless_write_method_rejection_keeps_connection(method, headers, monkeypatch): + """A write method with no declared body has nothing unread — keep-alive. + + `read_request_body=True` is passed for every one of these (that is what the + real unsafe-method call site does), so this is exactly the case the old + per-method gate got wrong in the over-close direction. + """ + handler = _rejected_sidecar_call(monkeypatch, method, headers, read_request_body=True) + + assert handler.close_connection is False + + +@pytest.mark.parametrize( + ("headers", "expected"), + [ + ({}, False), + ({"Content-Length": "0"}, False), + ({"Content-Length": " 0 "}, False), + ({"Content-Length": ""}, False), + ({"Content-Length": "12"}, True), + ({"Content-Length": "-1"}, True), + ({"Content-Length": "banana"}, True), + ({"content-length": "7"}, True), + ({"Transfer-Encoding": "chunked"}, True), + ({"transfer-encoding": "Chunked"}, True), + ({"Transfer-Encoding": "identity"}, False), + ({"Transfer-Encoding": ""}, False), + ], +) +def test_request_declares_body_reads_the_framing_headers(headers, expected): + from api.helpers import request_declares_body + + handler = SimpleNamespace(headers=headers) + + assert request_declares_body(cast(Any, handler)) is expected + + +def test_request_declares_body_is_case_insensitive_on_a_real_message(): + """Production handlers carry an email.message.Message, not a dict.""" + from email.parser import Parser + + from api.helpers import request_declares_body + + message = Parser().parsestr("Host: x\r\ntransfer-encoding: Chunked\r\n\r\n") + + assert request_declares_body(cast(Any, SimpleNamespace(headers=message))) is True + + +def test_request_declares_body_tolerates_a_handler_without_headers(): + """Never raise on a rejection path: an exception there becomes a 500.""" + from api.helpers import request_declares_body + + assert request_declares_body(cast(Any, SimpleNamespace())) is False + assert request_declares_body(cast(Any, SimpleNamespace(headers=None))) is False + assert request_declares_body(cast(Any, SimpleNamespace(headers=object()))) is False + + 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 @@ -137,6 +251,90 @@ def test_upload_oversize_rejects_before_read(monkeypatch): assert handler.close_connection is True +# ── Multipart framing preflight: chunked uploads must close, empty ones must not ─ +# +# All four multipart handlers keyed on Content-Length alone, so a chunked upload +# (which has none) was read as length 0, matched no parts, and was rejected with +# "No file field in request" and keep-alive intact — leaving every chunk byte on +# the socket, which is the exact poisoning this PR exists to prevent. +# http.server never decodes chunked framing, so the only safe answer is 411 with +# the connection armed for close. + +_MULTIPART_HANDLERS = [ + "handle_upload", + "handle_upload_extract", + "handle_transcribe", + "handle_workspace_upload", +] + + +class _NeverRead: + """An rfile that fails the test if a reject-before-read path reads the body.""" + + def read(self, *_args): + raise AssertionError("the body must not be read on a reject-before-read path") + + +def _run_multipart_handler(monkeypatch, handler_name, headers, rfile): + import api.upload as upload + + answered: dict[str, Any] = {} + + def _record(_handler, payload, status=200): + answered["payload"] = payload + answered["status"] = status + return True + + monkeypatch.setattr(upload, "j", _record) + handler = SimpleNamespace(headers=headers, rfile=rfile, close_connection=False) + + getattr(upload, handler_name)(cast(Any, handler)) + return handler, answered + + +@pytest.mark.parametrize("handler_name", _MULTIPART_HANDLERS) +def test_chunked_multipart_rejected_before_read_closes_connection(handler_name, monkeypatch): + handler, answered = _run_multipart_handler( + monkeypatch, + handler_name, + {"Content-Type": "multipart/form-data; boundary=x", "Transfer-Encoding": "chunked"}, + _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_bodyless_multipart_rejection_keeps_connection(handler_name, monkeypatch): + """A well-framed upload with no body is rejected with nothing left unread.""" + handler, answered = _run_multipart_handler( + monkeypatch, + handler_name, + {"Content-Type": "multipart/form-data; boundary=x", "Content-Length": "0"}, + SimpleNamespace(read=lambda *_args: b""), + ) + + assert answered["status"] == 400, answered + assert handler.close_connection is False + + +@pytest.mark.parametrize("handler_name", _MULTIPART_HANDLERS) +def test_non_numeric_content_length_still_closes(handler_name, monkeypatch): + """The pre-existing invalid-Content-Length reject keeps its 400 and its close.""" + handler, answered = _run_multipart_handler( + monkeypatch, + handler_name, + {"Content-Type": "multipart/form-data; boundary=x", "Content-Length": "banana"}, + _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 f335a640..d1b348fd 100644 --- a/tests/test_rejected_write_pooled_client.py +++ b/tests/test_rejected_write_pooled_client.py @@ -19,8 +19,11 @@ stale tab gets 410, not 403). """ import http.client +import socket import urllib.parse +import pytest + from tests._pytest_port import BASE _PORT = urllib.parse.urlparse(BASE).port @@ -61,3 +64,96 @@ def test_rejected_write_advertises_close_and_pooled_followup_succeeds(): assert follow_status == 200, ( f"pooled follow-up after a reject-before-read must succeed; got {follow_status}" ) + + +# ── Framing the Content-Length gate could not see ───────────────────────────── +# +# Both cases below are pipelined down ONE socket against the real server: the +# reject-before-read request first, an ordinary GET immediately after it in the +# same write. If the rejection closes, the server answers once and EOFs and the +# GET is never seen. If it does not, the leftover body/chunk bytes are parsed as +# the next request line and a second response comes back — a 400 bad-syntax or +# `501 Unsupported method` naming bytes the client never sent as a request. + +_FOLLOWING_GET = b"GET /api/health/agent HTTP/1.1\r\nHost: 127.0.0.1\r\nAccept: */*\r\n\r\n" +_MULTIPART_UPLOAD_PATHS = ( + "/api/upload", + "/api/upload/extract", + "/api/workspace/upload", + "/api/transcribe", +) +_MULTIPART_PAYLOAD = ( + b'--x\r\nContent-Disposition: form-data; name="file"; filename="a.txt"\r\n' + b"\r\nhello\r\n--x--\r\n" +) +_CHUNKED_MULTIPART_BODY = ( + f"{len(_MULTIPART_PAYLOAD):X}\r\n".encode() + _MULTIPART_PAYLOAD + b"\r\n0\r\n\r\n" +) + + +def _pipelined_after(request: bytes) -> bytes: + """Send *request* and a plain GET in one write; return every byte answered.""" + sock = socket.create_connection(("127.0.0.1", _PORT), timeout=10) + try: + sock.sendall(request + _FOLLOWING_GET) + received = b"" + while True: + try: + chunk = sock.recv(65536) + except (TimeoutError, socket.timeout, ConnectionError): + break + if not chunk: + break + received += chunk + return received + finally: + sock.close() + + +def _assert_single_closed_response(answered: bytes, status: bytes, leftover: bytes) -> None: + text = answered.decode("latin-1", errors="replace") + assert answered.startswith(b"HTTP/1.1 " + status), text + assert b"Connection: close" in answered, text + assert answered.count(b"HTTP/1.1 ") == 1, ( + f"the pipelined GET was answered, so the socket stayed open: {text}" + ) + assert leftover not in answered, f"the unread body reached the request parser: {text}" + assert b"Bad request syntax" not in answered, text + assert b"Unsupported method" not in answered, text + + +@pytest.mark.parametrize("path", _MULTIPART_UPLOAD_PATHS) +def test_chunked_upload_rejection_closes_and_cannot_poison_the_socket(path): + """A chunked upload has no Content-Length — the old gate read it as empty. + + All four multipart handlers then answered 400 "No file field in request" + with keep-alive intact, leaving the chunk bytes on the 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" + "Transfer-Encoding: chunked\r\n" + "\r\n".encode() + _CHUNKED_MULTIPART_BODY + ) + + _assert_single_closed_response(answered, b"411", _MULTIPART_PAYLOAD) + + +def test_sidecar_get_with_a_body_closes_and_cannot_poison_the_socket(): + """A provenance-rejected GET that carries a declared body. + + `read_request_body=False` said "no body" and the 403 went out with + keep-alive; the gate's repro then got `501 Unsupported method ('{}GET')` on + the same socket. No Origin/Referer/Sec-Fetch-Site here, so provenance fails + before the body would ever be read. + """ + 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" + f"Content-Length: {len(_BODY)}\r\n" + "\r\n".encode() + _BODY + ) + + _assert_single_closed_response(answered, b"403", _BODY)