From 20f6a6862642b9914b4eea7d5bf2b4c7f07557fa Mon Sep 17 00:00:00 2001 From: Rodrigo Gomes da Silva Date: Fri, 31 Jul 2026 18:12:49 -0300 Subject: [PATCH 01/15] fix: close rejected HTTP writes before reuse --- api/routes.py | 4 ++ server.py | 7 ++- tests/test_rejected_write_connection_close.py | 55 +++++++++++++++++++ 3 files changed, 65 insertions(+), 1 deletion(-) create mode 100644 tests/test_rejected_write_connection_close.py diff --git a/api/routes.py b/api/routes.py index d5b4a891..a7d01953 100644 --- a/api/routes.py +++ b/api/routes.py @@ -5871,6 +5871,9 @@ def _csrf_rejection_error(handler) -> str: def _check_csrf(handler) -> bool: """Reject cross-origin or tokenless authenticated browser unsafe requests.""" if not _check_same_origin_browser_request(handler): + # CSRF checks run before read_body(). Close rather than reusing an + # HTTP/1.1 connection whose unread body would corrupt the next request. + handler.close_connection = True return False if not _is_browser_unsafe_request(handler): return True # non-browser clients (curl, MCP, agent) have no Origin/Referer @@ -5883,6 +5886,7 @@ def _check_csrf(handler) -> bool: submitted = handler.headers.get(CSRF_HEADER_NAME) or handler.headers.get("X-CSRF-Token") if verify_csrf_token(cookie_val or "", submitted or ""): return True + handler.close_connection = True return _set_csrf_failure_reason(handler, "token_mismatch") diff --git a/server.py b/server.py index 040d80c1..8205167b 100644 --- a/server.py +++ b/server.py @@ -404,7 +404,12 @@ class Handler(BaseHTTPRequestHandler): _is_csp_report_post = ( parsed.path == "/api/csp-report" and self.command == "POST" ) - if not _is_csp_report_post and not check_auth(self, parsed): return + if not _is_csp_report_post and not check_auth(self, parsed): + # Authentication can reject an unsafe request before its body is + # consumed. Reusing that HTTP/1.1 connection would make the + # unread body prefix the next request line. + self.close_connection = True + return result = route_func(self, parsed) if result is False: return j(self, {'error': 'not found'}, status=404) diff --git a/tests/test_rejected_write_connection_close.py b/tests/test_rejected_write_connection_close.py new file mode 100644 index 00000000..c3cd9fd7 --- /dev/null +++ b/tests/test_rejected_write_connection_close.py @@ -0,0 +1,55 @@ +"""Regression tests for HTTP/1.1 write rejection framing. + +A rejected POST can happen before its request body is consumed (authentication +or CSRF failure). Keeping that connection alive leaves the body bytes in +``rfile``; BaseHTTPRequestHandler then parses them as the next request method. +The connection must close after those early rejections. +""" +from types import SimpleNamespace +from typing import Any, cast + + +def test_auth_rejected_write_closes_connection(monkeypatch): + import server + + handler = SimpleNamespace( + path="/api/session/new", + command="POST", + close_connection=False, + ) + monkeypatch.setattr(server, "reset_trusted_auth_request_state", lambda _handler: None) + monkeypatch.setattr(server, "get_profile_cookie", lambda _handler: None) + monkeypatch.setattr(server, "clear_request_profile", lambda: None) + monkeypatch.setattr(server, "check_auth", lambda _handler, _parsed: False) + + def route_must_not_run(_handler, _parsed): + raise AssertionError("route ran after auth rejection") + + server.Handler._handle_write(cast(Any, handler), route_must_not_run) + + assert handler.close_connection is True + + +def test_origin_rejected_csrf_closes_connection(monkeypatch): + import api.routes as routes + + handler = SimpleNamespace(close_connection=False) + monkeypatch.setattr(routes, "_check_same_origin_browser_request", lambda _handler: False) + + assert routes._check_csrf(handler) is False + assert handler.close_connection is True + + +def test_token_rejected_csrf_closes_connection(monkeypatch): + import api.auth as auth + import api.routes as routes + + handler = SimpleNamespace(headers={}, close_connection=False) + monkeypatch.setattr(routes, "_check_same_origin_browser_request", lambda _handler: True) + monkeypatch.setattr(routes, "_is_browser_unsafe_request", lambda _handler: True) + monkeypatch.setattr(auth, "is_auth_enabled", lambda: True) + monkeypatch.setattr(auth, "parse_cookie", lambda _handler: "session-cookie") + monkeypatch.setattr(auth, "verify_csrf_token", lambda _cookie, _token: False) + + assert routes._check_csrf(handler) is False + assert handler.close_connection is True From 11f6b31d128cd59e5049890c7a865722903d0668 Mon Sep 17 00:00:00 2001 From: Hermes Agent Date: Sat, 1 Aug 2026 03:29:10 -0300 Subject: [PATCH 02/15] test: drive the rejected-write connection reuse over a real socket The unit tests in this PR assert `close_connection` is set on each rejection path. That pins the mechanism but not the consequence: they build a SimpleNamespace, never open a socket, and would still pass if BaseHTTPRequestHandler stopped honouring the flag. This sends a rejected write and then an ordinary GET down one connection. Reusing it makes the server read the unread body as a request line and answer HTTP/1.1 400 Bad request syntax ('{"a": "b"}GET /api/health HTTP/1.1') an error about a request the client never sent, which is what makes this class of bug expensive to diagnose. Both directions are pinned: the reusing handler must produce that 400, the closing handler must not. The two files are complementary. Reverting server.py fails the unit test; making the handler here reuse the connection fails this one. --- tests/test_rejected_write_socket_reuse.py | 110 ++++++++++++++++++++++ 1 file changed, 110 insertions(+) create mode 100755 tests/test_rejected_write_socket_reuse.py diff --git a/tests/test_rejected_write_socket_reuse.py b/tests/test_rejected_write_socket_reuse.py new file mode 100755 index 00000000..824e99f3 --- /dev/null +++ b/tests/test_rejected_write_socket_reuse.py @@ -0,0 +1,110 @@ +"""A rejected write must not leave its unread body on a reused connection. + +The unit tests beside this one assert `close_connection` is set on each rejection +path. That pins the mechanism but not the consequence: they build a +SimpleNamespace, never open a socket, and would still pass if +BaseHTTPRequestHandler stopped honouring the flag. + +This drives the actual failure over one socket: a POST that is rejected before its +body is read, then an ordinary GET on the same connection. Without the fix the +server parses the leftover body as the next request line and answers + + HTTP/1.1 400 Bad request syntax ('{"a": "b"}GET /api/health HTTP/1.1') + +which is an error about a request the client never sent - the reason this class of +bug is expensive to diagnose from the client side. With the fix the server closes +after the rejection, so the client sees a clean EOF and reconnects instead. + +The two files are complementary and neither replaces the other. The unit tests +call the production handlers, so they pin that `close_connection` is actually set +on each rejection path. This file uses a minimal handler of the same shape, so it +pins WHY that flag matters at the HTTP level - which no amount of asserting on a +boolean can show. Reverting server.py fails the unit test; changing the handler +here to reuse the connection fails this one. +""" + +import socket +import threading +import time +from http.server import BaseHTTPRequestHandler, ThreadingHTTPServer + +BODY = b'{"a": "b"}' +REJECTED_WRITE = ( + b"POST /api/session/new HTTP/1.1\r\n" + b"Host: localhost\r\n" + b"Content-Type: application/json\r\n" + b"Content-Length: " + str(len(BODY)).encode() + b"\r\n" + b"\r\n" + BODY +) +FOLLOWING_READ = b"GET /api/health HTTP/1.1\r\nHost: localhost\r\n\r\n" + + +class _Rejecting(BaseHTTPRequestHandler): + """The production shape: reject an unsafe write before reading its body.""" + + protocol_version = "HTTP/1.1" + closes_on_reject = True + + def log_message(self, *args): + pass # keep the test output quiet + + def do_POST(self): + # Deliberately does NOT read the body. This is the production path when + # auth or CSRF rejects a write before read_body() runs. + if self.closes_on_reject: + self.close_connection = True + self.send_response(403) + self.send_header("Content-Length", "0") + self.end_headers() + + def do_GET(self): + self.send_response(200) + self.send_header("Content-Length", "2") + self.end_headers() + self.wfile.write(b"ok") + + +def _second_response(*, closes_on_reject: bool) -> bytes: + """Send a rejected write then a GET down one socket; return the GET's answer.""" + handler = type("_H", (_Rejecting,), {"closes_on_reject": closes_on_reject}) + server = ThreadingHTTPServer(("127.0.0.1", 0), handler) + threading.Thread(target=server.serve_forever, daemon=True).start() + try: + sock = socket.create_connection(server.server_address, timeout=5) + try: + sock.sendall(REJECTED_WRITE) + time.sleep(0.3) + assert b"403" in sock.recv(4096), "the write should have been rejected" + + sock.sendall(FOLLOWING_READ) + time.sleep(0.3) + try: + return sock.recv(4096) + except (socket.timeout, ConnectionError): + return b"" + finally: + sock.close() + finally: + server.shutdown() + server.server_close() + + +def test_rejected_write_does_not_corrupt_the_next_request(): + """Closing means the client sees EOF, never a bogus error about its own GET.""" + answer = _second_response(closes_on_reject=True) + assert b"400" not in answer, f"the leftover body reached the parser: {answer!r}" + assert BODY not in answer, f"the leftover body reached the parser: {answer!r}" + # The connection is gone, which is the point: the client reconnects rather + # than being told its next request was malformed. + assert answer == b"", f"expected a closed connection, got {answer!r}" + + +def test_without_the_fix_the_unread_body_corrupts_the_next_request(): + """The bug itself, so the test above is known to be testing something. + + Reusing the connection makes the server read the unread body as a request + line, and the body text appears in the 400 it sends back. + """ + answer = _second_response(closes_on_reject=False) + assert b"400" in answer, f"expected a parse failure, got {answer!r}" + assert BODY in answer, f"expected the leftover body in the error, got {answer!r}" From 6a8011f98360dad9dc6ac819ff64abcd281b9494 Mon Sep 17 00:00:00 2001 From: Rodrigo Gomes da Silva Date: Fri, 7 Aug 2026 02:25:59 -0300 Subject: [PATCH 03/15] fix: keep server.py under the 750-line guard after auth-reject close Move the close-on-reject decision into api.auth.check_auth_or_close so the added lines land outside server.py, restoring it to 749 lines. Trim the inline comment to a one-line docstring on the helper, per review feedback. The rejected-write connection-close test now patches api.auth.check_auth (the dependency the helper calls) instead of server.check_auth. --- api/auth.py | 8 ++++++++ server.py | 9 ++------- tests/test_rejected_write_connection_close.py | 3 ++- 3 files changed, 12 insertions(+), 8 deletions(-) diff --git a/api/auth.py b/api/auth.py index 846c9945..3fa0e34d 100644 --- a/api/auth.py +++ b/api/auth.py @@ -1207,6 +1207,14 @@ def check_auth(handler, parsed) -> bool: return False +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 + + def _is_loopback(addr: str) -> bool: """Return True if *addr* is a loopback address (127.x.x.x, ::1, or ::ffff:127.x.x.x).""" import ipaddress as _ipaddress diff --git a/server.py b/server.py index 8205167b..fba5719e 100644 --- a/server.py +++ b/server.py @@ -100,7 +100,7 @@ from urllib.parse import urlparse logger = logging.getLogger(__name__) from api.request_logging import emit_request_log -from api.auth import check_auth, reset_trusted_auth_request_state +from api.auth import check_auth, check_auth_or_close, reset_trusted_auth_request_state from api.config import HOST, PORT, STATE_DIR, SESSION_DIR, DEFAULT_WORKSPACE from api.helpers import ( j, @@ -404,12 +404,7 @@ class Handler(BaseHTTPRequestHandler): _is_csp_report_post = ( parsed.path == "/api/csp-report" and self.command == "POST" ) - if not _is_csp_report_post and not check_auth(self, parsed): - # Authentication can reject an unsafe request before its body is - # consumed. Reusing that HTTP/1.1 connection would make the - # unread body prefix the next request line. - self.close_connection = True - return + if not _is_csp_report_post and not check_auth_or_close(self, parsed): return result = route_func(self, parsed) if result is False: return j(self, {'error': 'not found'}, status=404) diff --git a/tests/test_rejected_write_connection_close.py b/tests/test_rejected_write_connection_close.py index c3cd9fd7..ea004f2e 100644 --- a/tests/test_rejected_write_connection_close.py +++ b/tests/test_rejected_write_connection_close.py @@ -10,6 +10,7 @@ from typing import Any, cast def test_auth_rejected_write_closes_connection(monkeypatch): + import api.auth as auth import server handler = SimpleNamespace( @@ -20,7 +21,7 @@ def test_auth_rejected_write_closes_connection(monkeypatch): monkeypatch.setattr(server, "reset_trusted_auth_request_state", lambda _handler: None) monkeypatch.setattr(server, "get_profile_cookie", lambda _handler: None) monkeypatch.setattr(server, "clear_request_profile", lambda: None) - monkeypatch.setattr(server, "check_auth", lambda _handler, _parsed: False) + monkeypatch.setattr(auth, "check_auth", lambda _handler, _parsed: False) def route_must_not_run(_handler, _parsed): raise AssertionError("route ran after auth rejection") From 1bd3c72e17615c280a71a100e83336e04c136f42 Mon Sep 17 00:00:00 2001 From: Rodrigo Gomes da Silva Date: Fri, 7 Aug 2026 02:40:07 -0300 Subject: [PATCH 04/15] fix: close connection on extension-sidecar provenance rejection Same class as the auth/CSRF close-on-reject: the sidecar proxy's same-origin provenance check runs before read_body(), so an HTTP/1.1 connection reused after a 403 would parse the unread body as the next request line. Flag close_connection on this path too. Found by Greptile review of #6658; covered by a new unit test. --- api/routes.py | 4 ++++ tests/test_rejected_write_connection_close.py | 18 ++++++++++++++++++ 2 files changed, 22 insertions(+) diff --git a/api/routes.py b/api/routes.py index a7d01953..41aad267 100644 --- a/api/routes.py +++ b/api/routes.py @@ -6066,6 +6066,10 @@ 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 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/tests/test_rejected_write_connection_close.py b/tests/test_rejected_write_connection_close.py index ea004f2e..fa926ae6 100644 --- a/tests/test_rejected_write_connection_close.py +++ b/tests/test_rejected_write_connection_close.py @@ -54,3 +54,21 @@ def test_token_rejected_csrf_closes_connection(monkeypatch): assert routes._check_csrf(handler) is False assert handler.close_connection is True + + +def test_sidecar_provenance_rejected_closes_connection(monkeypatch): + """Sidecar proxy provenance rejection also runs before read_body().""" + import api.routes as routes + + handler = SimpleNamespace( + path="/api/extensions/ext1/sidecar/proxy", + command="POST", + 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=""), "POST") + + assert handler.close_connection is True 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 05/15] 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() From 9a181c0026b42a0469a4f1f66f15449d86a7c639 Mon Sep 17 00:00:00 2001 From: Rodrigo Gomes da Silva Date: Fri, 14 Aug 2026 20:53:05 -0300 Subject: [PATCH 06/15] fix(http): emit Connection: close exactly once and never crash on stubs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The first cut of the close advertisement had two defects, both caught by CI (9 red jobs) and by the deep-review gate's own MUST-FIX 1 wording. 1. Dedup was keyed off a private `_close_advertised` flag that end_headers() set itself. Every SSE endpoint already sends its own `Connection: close`, and BaseHTTPRequestHandler.send_header flips close_connection when it does — so end_headers() appended a SECOND copy to every stream response. Dedup now derives from the pending header buffer, which sees those site-level headers too. 2. `self.close_connection` was read unguarded. The attribute only exists once a request line has been parsed, so the many suite handlers built with `Handler.__new__(Handler)` raised AttributeError instead of emitting headers — and in _handle_write that AttributeError became a spurious 500. Both reads are now defensive, and check_auth_or_close() no longer invents the attribute on a handler that never had it (which would have left a bogus keep-alive verdict behind). The mechanism moves to api/helpers.advertise_connection_close(), which also puts server.py back under the original 750-line guard, so the test's ceiling bump to 760 is reverted rather than shipped. --- api/auth.py | 7 + api/helpers.py | 44 ++++++ server.py | 12 +- tests/test_rejected_write_connection_close.py | 129 ++++++++++++++++++ tests/test_sprint10.py | 10 +- 5 files changed, 185 insertions(+), 17 deletions(-) diff --git a/api/auth.py b/api/auth.py index 8c236fd4..87f6bc4f 100644 --- a/api/auth.py +++ b/api/auth.py @@ -1213,7 +1213,14 @@ def check_auth_or_close(handler, parsed) -> bool: 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. + + ``close_connection`` only exists once BaseHTTPRequestHandler has parsed a + request line, so partially-built handler stubs may not have it at all. Read + it defensively and only arm/restore when it was really there: inventing the + attribute on a stub would leave a bogus keep-alive verdict behind. """ + if not hasattr(handler, 'close_connection'): + return check_auth(handler, parsed) prior_close = handler.close_connection handler.close_connection = True if check_auth(handler, parsed): diff --git a/api/helpers.py b/api/helpers.py index 6e658f0d..2337bb70 100644 --- a/api/helpers.py +++ b/api/helpers.py @@ -202,6 +202,50 @@ def _security_headers(handler): ) +def arm_connection_close(handler) -> None: + """Mark the connection dead BEFORE a reject-before-read response is written. + + A rejection that answers without consuming the request body leaves those + bytes queued in ``rfile``; reusing the connection makes the next HTTP/1.1 + request parse mid-body (the classic ``{}GET ... -> 501``). Arming must + happen before the response is flushed so ``advertise_connection_close()`` + can serialize it into the response headers. + + Best-effort: handler stubs and read-only doubles may reject the attribute, + and a failure here must never turn a 4xx into a 500. + """ + try: + handler.close_connection = True + except Exception: + pass + + +def advertise_connection_close(handler) -> None: + """Serialize ``close_connection`` into exactly one ``Connection: close`` header. + + ``BaseHTTPRequestHandler`` tracks ``close_connection`` internally but never + writes it to the wire, so a server-side close is invisible to the client: a + pooled client reuses the socket and fails with BrokenPipeError instead of + opening a fresh connection. Call this from ``end_headers()``, before the + header buffer is flushed. + + Dedup is derived from the pending header buffer rather than a private flag, + because the SSE endpoints send their own ``Connection: close`` (and + ``send_header`` itself flips ``close_connection`` when they do) — keying off + a flag we set ourselves would emit the header twice on every stream. + + Both attribute reads are defensive: partially-built handler stubs in the + test suite carry neither ``close_connection`` nor ``_headers_buffer``, and + header emission must not depend on their presence. + """ + if not getattr(handler, 'close_connection', False): + return + for raw in getattr(handler, '_headers_buffer', None) or (): + if raw[:11].lower() == b'connection:': + return + handler.send_header('Connection', 'close') + + def flush_pending_auth_cookies(handler) -> None: pending = getattr(handler, '_pending_set_cookies', None) if not pending: diff --git a/server.py b/server.py index a41c28d7..39af7793 100644 --- a/server.py +++ b/server.py @@ -104,6 +104,7 @@ from api.auth import check_auth, check_auth_or_close, reset_trusted_auth_request from api.config import HOST, PORT, STATE_DIR, SESSION_DIR, DEFAULT_WORKSPACE from api.helpers import ( j, + advertise_connection_close, get_profile_cookie, _build_csp_report_only_policy, _CLIENT_DISCONNECT_ERRORS, @@ -332,12 +333,7 @@ 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 + advertise_connection_close(self) # tell the client when the socket dies super().end_headers() def log_message(self, fmt, *args): pass # suppress default Apache-style log @@ -407,9 +403,7 @@ class Handler(BaseHTTPRequestHandler): set_request_profile(cookie_profile) try: parsed = urlparse(self.path) - _is_csp_report_post = ( - parsed.path == "/api/csp-report" and self.command == "POST" - ) + _is_csp_report_post = parsed.path == "/api/csp-report" and self.command == "POST" if not _is_csp_report_post and not check_auth_or_close(self, parsed): return result = route_func(self, parsed) if result is False: diff --git a/tests/test_rejected_write_connection_close.py b/tests/test_rejected_write_connection_close.py index 90b08914..bd7ddb4f 100644 --- a/tests/test_rejected_write_connection_close.py +++ b/tests/test_rejected_write_connection_close.py @@ -135,3 +135,132 @@ def test_upload_oversize_rejects_before_read(monkeypatch): upload.handle_upload(handler) assert handler.close_connection is True + + +# ── Header serialization: exactly one Connection: close, never a crash ──────── +# +# The advertisement lives in Handler.end_headers() via +# api.helpers.advertise_connection_close(). Two things went wrong there and +# both are pinned below: +# +# 1. The first cut keyed dedup off a private `_close_advertised` flag it set +# itself. Every SSE endpoint already sends its own `Connection: close` +# (which BaseHTTPRequestHandler.send_header turns into +# close_connection=True), so end_headers() appended a SECOND copy to every +# stream response. +# 2. It read `self.close_connection` unguarded. That attribute only exists +# once a request line has been parsed, so the many suite handlers built +# with `Handler.__new__(Handler)` raised AttributeError instead of +# emitting headers. + + +def _render_response_headers(prepare=None) -> str: + """Drive the production Handler.end_headers() into a byte buffer.""" + import io + + from server import Handler + + handler = Handler.__new__(Handler) + handler.request_version = "HTTP/1.1" + handler.close_connection = False + handler.wfile = io.BytesIO() + handler._headers_buffer = [] + if prepare is not None: + prepare(handler) + Handler.end_headers(handler) + return handler.wfile.getvalue().decode("latin-1") + + +def test_armed_close_is_advertised_exactly_once(): + """A reject-before-read must TELL the client the socket is gone.""" + from api.helpers import arm_connection_close + + rendered = _render_response_headers(arm_connection_close) + + assert rendered.count("Connection: close") == 1, rendered + + +def test_sse_style_explicit_close_is_not_doubled(): + """SSE sends its own Connection: close; end_headers() must not repeat it. + + send_header('Connection', 'close') flips close_connection inside the + stdlib, so a self-keyed dedup flag would emit the header twice here. + """ + + def prepare(handler): + handler.send_header("Content-Type", "text/event-stream; charset=utf-8") + handler.send_header("Connection", "close") + + rendered = _render_response_headers(prepare) + + assert rendered.count("Connection: close") == 1, rendered + + +def test_keep_alive_response_advertises_no_close(): + """An ordinary response must keep its keep-alive.""" + rendered = _render_response_headers( + lambda handler: handler.send_header("Content-Type", "application/json") + ) + + assert "Connection: close" not in rendered, rendered + + +def test_end_headers_on_handler_stub_without_close_connection(monkeypatch): + """A partially-built handler stub has no close_connection — don't raise. + + Much of the suite builds handlers with `Handler.__new__(Handler)` and never + parses a request, so the attribute is simply absent. Reading it unguarded + turned every such test into an AttributeError. + """ + from http.server import BaseHTTPRequestHandler + + from server import Handler + + sent: list[tuple[str, str]] = [] + handler = Handler.__new__(Handler) + handler.send_header = lambda key, value: sent.append((key, value)) + monkeypatch.setattr(BaseHTTPRequestHandler, "end_headers", lambda self: None) + + assert not hasattr(handler, "close_connection") + Handler.end_headers(handler) # must not raise + + assert "Content-Security-Policy-Report-Only" in dict(sent) + assert "Connection" not in dict(sent), "nothing was armed, so nothing to advertise" + + +def test_check_auth_or_close_on_handler_stub_without_close_connection(monkeypatch): + """check_auth_or_close must not invent close_connection on a stub. + + Arming a handler that never had the attribute would leave a bogus + keep-alive verdict behind, and reading it unguarded raised AttributeError + (which _handle_write then converted into a spurious 500). + """ + import api.auth as auth + from server import Handler + + handler = Handler.__new__(Handler) + monkeypatch.setattr(auth, "check_auth", lambda _handler, _parsed: True) + + assert auth.check_auth_or_close(handler, SimpleNamespace(path="/api/x")) is True + assert not hasattr(handler, "close_connection") + + +def test_check_auth_or_close_restores_keep_alive_on_success(monkeypatch): + """A successful auth must not inherit the armed close.""" + import api.auth as auth + + handler = SimpleNamespace(close_connection=False) + monkeypatch.setattr(auth, "check_auth", lambda _handler, _parsed: True) + + assert auth.check_auth_or_close(cast(Any, handler), SimpleNamespace(path="/api/x")) is True + assert handler.close_connection is False + + +def test_arm_connection_close_tolerates_a_read_only_handler(): + """Arming must never turn a 4xx into a 500, even on an odd handler.""" + from api.helpers import arm_connection_close + + class Frozen: + __slots__ = () + + arm_connection_close(Frozen()) # must not raise diff --git a/tests/test_sprint10.py b/tests/test_sprint10.py index d65be191..1f9ddfc9 100644 --- a/tests/test_sprint10.py +++ b/tests/test_sprint10.py @@ -70,15 +70,9 @@ 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. - - 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. - """ + """server.py should be under 750 lines after the split.""" lines = len((REPO_ROOT / "server.py").read_text().splitlines()) - assert lines < 760, f"server.py is {lines} lines -- split may not have landed" + assert lines < 750, 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() From a93e8ba890450723cafb71a8d3310bc0f51f006c Mon Sep 17 00:00:00 2001 From: Rodrigo Gomes da Silva Date: Fri, 14 Aug 2026 20:53:13 -0300 Subject: [PATCH 07/15] refactor(http): route every reject-before-read through one arming helper Replaces the inline `handler.close_connection = True` at each reject-before-read site with api.helpers.arm_connection_close(), the shared mechanism the review asked for. Behaviour is unchanged at every real handler; the helper adds the same stub tolerance the read side just got, so a handler that refuses the attribute can never turn a 4xx into a 500. Sites: _check_csrf (origin + token), the sidecar provenance 403 (body-reading calls only), the rate-limited CSP report, /api/health/restart, the deprecated process-complete-ack 410, and all four upload handlers' pre-read length/boundary rejections. --- api/routes.py | 13 +++++++------ api/upload.py | 20 ++++++++++---------- 2 files changed, 17 insertions(+), 16 deletions(-) diff --git a/api/routes.py b/api/routes.py index ecd188ff..92e5c617 100644 --- a/api/routes.py +++ b/api/routes.py @@ -2974,6 +2974,7 @@ from api.helpers import ( require, bad, safe_resolve, + arm_connection_close, j, t, read_body, @@ -5873,7 +5874,7 @@ def _check_csrf(handler) -> bool: if not _check_same_origin_browser_request(handler): # CSRF checks run before read_body(). Close rather than reusing an # HTTP/1.1 connection whose unread body would corrupt the next request. - handler.close_connection = True + arm_connection_close(handler) return False if not _is_browser_unsafe_request(handler): return True # non-browser clients (curl, MCP, agent) have no Origin/Referer @@ -5886,7 +5887,7 @@ def _check_csrf(handler) -> bool: submitted = handler.headers.get(CSRF_HEADER_NAME) or handler.headers.get("X-CSRF-Token") if verify_csrf_token(cookie_val or "", submitted or ""): return True - handler.close_connection = True + arm_connection_close(handler) return _set_csrf_failure_reason(handler, "token_mismatch") @@ -6071,7 +6072,7 @@ def _handle_extension_sidecar_proxy( # 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 + arm_connection_close(handler) return j(handler, {"error": _csrf_rejection_error(handler)}, status=403) try: request_body = _read_body_bytes(handler) if read_request_body else None @@ -6516,7 +6517,7 @@ def _handle_csp_report(handler) -> bool: ) # 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 + arm_connection_close(handler) return _send_no_content(handler) payload = _read_csp_report_payload(handler) @@ -12910,7 +12911,7 @@ 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 + arm_connection_close(handler) outcome = restart_active_profile_gateway() if outcome.get("status") == "completed": @@ -15236,7 +15237,7 @@ def handle_post(handler, parsed) -> bool: # 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 + arm_connection_close(handler) j( handler, { diff --git a/api/upload.py b/api/upload.py index 942f8f7c..30abef3b 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 j +from api.helpers import arm_connection_close, j 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 ( @@ -47,7 +47,7 @@ _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 + arm_connection_close(handler) return j(handler, {'error': message}, status=status) @@ -217,7 +217,7 @@ def handle_upload(handler): 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 + 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) @@ -226,7 +226,7 @@ def handle_upload(handler): except ValueError: # parse_multipart validates Content-Length/boundary before reading; # a ValueError here means the body was never consumed. - handler.close_connection = True + arm_connection_close(handler) raise session_id = fields.get('session_id', '') if 'file' not in files: @@ -418,7 +418,7 @@ def handle_upload_extract(handler): 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 + 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) @@ -427,7 +427,7 @@ def handle_upload_extract(handler): except ValueError: # parse_multipart validates Content-Length/boundary before reading; # a ValueError here means the body was never consumed. - handler.close_connection = True + arm_connection_close(handler) raise session_id = fields.get('session_id', '') if 'file' not in files: @@ -461,7 +461,7 @@ def handle_transcribe(handler): 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 + 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) @@ -470,7 +470,7 @@ def handle_transcribe(handler): except ValueError: # parse_multipart validates Content-Length/boundary before reading; # a ValueError here means the body was never consumed. - handler.close_connection = True + arm_connection_close(handler) raise if 'file' not in files: return j(handler, {'error': 'No file field in request'}, status=400) @@ -656,7 +656,7 @@ def handle_workspace_upload(handler): 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 + 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) @@ -666,7 +666,7 @@ def handle_workspace_upload(handler): except ValueError: # parse_multipart validates Content-Length/boundary before reading; # a ValueError here means the body was never consumed. - handler.close_connection = True + arm_connection_close(handler) raise session_id = fields.get('session_id', '') subpath = fields.get('path', '') From 304815228ae5f76df6b0effd3670e77982e0c588 Mon Sep 17 00:00:00 2001 From: Rodrigo Gomes da Silva Date: Fri, 14 Aug 2026 21:16:56 -0300 Subject: [PATCH 08/15] fix(http): don't let a non-iterable header buffer crash end_headers advertise_connection_close() guarded its two attribute reads but not the dedup scan over _headers_buffer. A stub whose buffer is truthy and non-iterable (a bare Mock) raised TypeError from inside end_headers(), which _handle_write's generic except would swallow into a spurious 500 -- the same failure class as the missing close_connection guard. --- api/helpers.py | 9 +++++++-- tests/test_rejected_write_connection_close.py | 20 +++++++++++++++++++ 2 files changed, 27 insertions(+), 2 deletions(-) diff --git a/api/helpers.py b/api/helpers.py index 2337bb70..79a6c8f3 100644 --- a/api/helpers.py +++ b/api/helpers.py @@ -236,11 +236,16 @@ def advertise_connection_close(handler) -> None: Both attribute reads are defensive: partially-built handler stubs in the test suite carry neither ``close_connection`` nor ``_headers_buffer``, and - header emission must not depend on their presence. + header emission must not depend on their presence. The buffer is also + type-checked before it is scanned -- a stub whose ``_headers_buffer`` is a + truthy non-iterable (a bare ``Mock``) would otherwise raise ``TypeError`` + from inside ``end_headers()``, which ``_handle_write`` would swallow into a + spurious 500. """ if not getattr(handler, 'close_connection', False): return - for raw in getattr(handler, '_headers_buffer', None) or (): + buffered = getattr(handler, '_headers_buffer', None) + for raw in buffered if isinstance(buffered, (list, tuple)) else (): if raw[:11].lower() == b'connection:': return handler.send_header('Connection', 'close') diff --git a/tests/test_rejected_write_connection_close.py b/tests/test_rejected_write_connection_close.py index bd7ddb4f..a1bfc9a5 100644 --- a/tests/test_rejected_write_connection_close.py +++ b/tests/test_rejected_write_connection_close.py @@ -228,6 +228,26 @@ def test_end_headers_on_handler_stub_without_close_connection(monkeypatch): assert "Connection" not in dict(sent), "nothing was armed, so nothing to advertise" +def test_advertise_close_tolerates_a_non_iterable_header_buffer(): + """The dedup scan must be as defensive as the two attribute reads. + + A bare ``Mock`` answers every attribute, so ``_headers_buffer`` comes back + truthy but non-iterable. Iterating it unguarded raised + ``TypeError: 'Mock' object is not iterable`` from inside ``end_headers()``, + which ``_handle_write``'s generic ``except`` turns into a spurious 500 — + the same crash-inside-end_headers class as the missing + ``close_connection`` guard above. + """ + from unittest.mock import Mock + + from api.helpers import advertise_connection_close + + handler = Mock() + advertise_connection_close(handler) # must not raise + + handler.send_header.assert_called_once_with("Connection", "close") + + def test_check_auth_or_close_on_handler_stub_without_close_connection(monkeypatch): """check_auth_or_close must not invent close_connection on a stub. 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 09/15] 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) From fb7664bc4087dd537341da6ab0deb1fdaa642144 Mon Sep 17 00:00:00 2001 From: Rodrigo Gomes da Silva Date: Sat, 15 Aug 2026 15:37:20 -0300 Subject: [PATCH 10/15] fix(http): read framing from every header value, not just the first Adversarially probing the re-gated head found the same poisoning reachable by DUPLICATING a framing header instead of omitting one. `Message.get()` returns only the first occurrence, so `Content-Length: 0` sent ahead of the real length made the body invisible to every reader: nothing was drained and the real bytes were parsed as the next request line. Reproduced live on the otherwise-fixed head, over one pooled socket: HTTP/1.1 403 Forbidden <- sidecar provenance reject HTTP/1.1 400 Bad request syntax ('{"stale": true}GET /api/health/agent ...') HTTP/1.1 400 Bad Request <- /api/upload reject HTTP/1.1 400 Bad request syntax ('--x') api/helpers.py now reads framing through `_framing_header_values()`, which returns every value the wire carried. `request_declares_body()` treats disagreeing or unparseable lengths as a declared body (they cannot prove the body's absence), and `unsupported_transfer_encoding()` inspects every declared coding so `identity` cannot mask a `chunked` behind it. Two agreeing zeroes are still no body -- repetition alone is not a conflict. New `unreadable_content_length()` gates the multipart preflight: with no honest byte count there is nothing to drain, so all four handlers refuse with 400 "Invalid Content-Length" before touching rfile, connection armed for close. 13 regressions, each verified red without this commit: the helper predicates on a real email.message.Message, the production sidecar 403, all four multipart handlers, and five pipelined real-socket tests proving the follow-up request is never answered off a poisoned socket. --- api/helpers.py | 100 +++++++++++------ api/upload.py | 13 ++- tests/test_rejected_write_connection_close.py | 105 ++++++++++++++++++ tests/test_rejected_write_pooled_client.py | 37 ++++++ 4 files changed, 223 insertions(+), 32 deletions(-) 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) From 79f5b25f5f078198b4a8bc9a70a2b55698228a99 Mon Sep 17 00:00:00 2001 From: Rodrigo Gomes da Silva Date: Sat, 15 Aug 2026 17:34:24 -0300 Subject: [PATCH 11/15] fix(http): treat a blank framing value as a declared body, not an absent one `Content-Length:` with nothing after the colon reads back as '' from `Message.get_all()`, and `_declared_content_lengths()` SKIPPED the empty value. The result was "no length declared", so `request_declares_body()` reported no pending body and the sidecar provenance rejection answered 403 with keep-alive intact -- leaving the payload in rfile for the server to parse as the next request line. Reproduced live, pipelined down one socket: GET /api/extensions/probe/sidecar/ping `Content-Length:` {"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') A blank value cannot prove a body's absence, so it now joins `banana` and two disagreeing lengths as unreadable framing: bytes are assumed pending and the connection closes. Absence is something only the wire can say, and an absent header already yields no values at all. Three adjacent holes of the same class were found by probing and are fixed by the same rule (each reproduced live with the same bad-syntax 400): * whitespace-only `Content-Length: ` -- `Message` collapses it to the same '', as it does a duplicated blank or a blank paired with a real length. * the four multipart upload handlers: a blank length slipped past `unreadable_content_length()`, reached `int('' or 0)` == 0, and answered 400 "No file field in request" with the payload still queued. Now 400 "Invalid Content-Length" with the connection closed. * blank `Transfer-Encoding:` -- '' sat in `_UNFRAMED_TRANSFER_CODINGS` beside `identity`, so a header naming no coding read as "nothing to frame". A blank coding is undecodable, not unframed; uploads answer 411 and close. The no-body contract is unchanged and pinned in both directions: no length header, a padded zero, two AGREEING zeroes and `identity` all still keep the pooled connection alive. --- api/helpers.py | 52 +++-- tests/test_rejected_write_connection_close.py | 178 +++++++++++++++++- tests/test_rejected_write_pooled_client.py | 98 +++++++++- 3 files changed, 312 insertions(+), 16 deletions(-) diff --git a/api/helpers.py b/api/helpers.py index 7e9a4ccf..0073972f 100644 --- a/api/helpers.py +++ b/api/helpers.py @@ -220,7 +220,13 @@ def arm_connection_close(handler) -> None: pass -_UNFRAMED_TRANSFER_CODINGS = frozenset({'', 'identity'}) +_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``, +# and an empty string would also render as a hole in the rejection message. +_BLANK_TRANSFER_CODING = '(blank)' def _framing_header_values(handler, name: str) -> list[str]: @@ -262,14 +268,25 @@ def _framing_header_values(handler, name: str) -> list[str]: 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. + ``None`` means the framing cannot be trusted at all (a value that will not + parse); an empty set means no length was declared. + + A BLANK or whitespace-only value is unreadable, NOT absent. ``Content-Length:`` + with payload bytes behind it declares a body whose size the header refuses to + state; skipping the empty value reported "no length declared", so the + rejection kept the connection alive and those bytes were parsed as the next + request line (403 then ``400 Bad request syntax ('{...}GET /api/... HTTP/1.1')``). + Absence is something only the wire can say, and ``_framing_header_values()`` + already reports a header the request never carried as no value at all -- so + 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 ``''``. """ parsed: set[int] = set() for raw in _framing_header_values(handler, 'Content-Length'): stripped = raw.strip() if not stripped: - continue + return None try: parsed.add(int(stripped)) except ValueError: @@ -280,10 +297,10 @@ def _declared_content_lengths(handler) -> set[int] | None: 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. + Either a value that will not parse (``banana``, ``0x5``, or a BLANK one), or + two or more values that disagree. None of them 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 @@ -300,9 +317,12 @@ 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 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. + 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. """ if unsupported_transfer_encoding(handler) is not None: return True @@ -324,9 +344,19 @@ def unsupported_transfer_encoding(handler) -> str | None: Every declared coding is inspected, not just the first: a request may repeat the header, and one undecodable coding anywhere is enough to refuse. + + 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. """ for raw in _framing_header_values(handler, 'Transfer-Encoding'): normalized = raw.strip() + if not normalized: + return _BLANK_TRANSFER_CODING if normalized.lower() not in _UNFRAMED_TRANSFER_CODINGS: return normalized return None diff --git a/tests/test_rejected_write_connection_close.py b/tests/test_rejected_write_connection_close.py index 29ef8e37..e09bf30b 100644 --- a/tests/test_rejected_write_connection_close.py +++ b/tests/test_rejected_write_connection_close.py @@ -173,15 +173,20 @@ def test_bodyless_write_method_rejection_keeps_connection(method, headers, monke ({}, False), ({"Content-Length": "0"}, False), ({"Content-Length": " 0 "}, False), - ({"Content-Length": ""}, False), + ({"Content-Length": ""}, True), + ({"Content-Length": " "}, True), + ({"Content-Length": "\t"}, True), ({"Content-Length": "12"}, True), ({"Content-Length": "-1"}, True), ({"Content-Length": "banana"}, True), + ({"Content-Length": "0x5"}, True), + ({"Content-Length": "+5"}, True), ({"content-length": "7"}, True), ({"Transfer-Encoding": "chunked"}, True), ({"transfer-encoding": "Chunked"}, True), ({"Transfer-Encoding": "identity"}, False), - ({"Transfer-Encoding": ""}, False), + ({"Transfer-Encoding": ""}, True), + ({"Transfer-Encoding": " "}, True), ], ) def test_request_declares_body_reads_the_framing_headers(headers, expected): @@ -295,6 +300,130 @@ def test_sidecar_get_with_duplicate_content_length_closes_connection(monkeypatch assert handler.close_connection is True +# ── A BLANK framing value is unreadable, not absent ─────────────────────────── +# +# `Content-Length:` with nothing after the colon reads back as '' from +# `get_all()`, and the empty value was SKIPPED — so `_declared_content_lengths()` +# reported "no length declared", `request_declares_body()` said no body was +# pending, and the provenance rejection answered 403 with keep-alive intact. +# Reproduced live against the running server on the otherwise-fixed head: +# +# GET /api/extensions/probe/sidecar/ping `Content-Length:` {"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') +# +# `Message` collapses `Content-Length:`, `Content-Length: ` and +# `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. + + +_BLANK_FRAMING_HEADERS = [ + "Content-Length:", + "Content-Length: ", + "Content-Length:\t", + "Content-Length:\r\nContent-Length:", # duplicated blank + "Content-Length:\r\nContent-Length: 15", # blank ahead of a real length + "Content-Length: 15\r\nContent-Length:", # and behind one + "Content-Length: 0\r\nContent-Length:", # blank behind an honest zero + "Transfer-Encoding:", + "Transfer-Encoding: ", + "Transfer-Encoding:\r\nTransfer-Encoding: chunked", +] + + +@pytest.mark.parametrize("raw", _BLANK_FRAMING_HEADERS) +def test_blank_framing_value_declares_a_body(raw): + """On a real email.message.Message, as the production handler carries.""" + from api.helpers import request_declares_body + + handler = _message_with("Host: x\r\n" + raw + "\r\n") + + assert request_declares_body(cast(Any, handler)) is True + + +@pytest.mark.parametrize( + "raw", + [ + "Content-Length:", + "Content-Length: ", + "Content-Length:\r\nContent-Length:", + "Content-Length:\r\nContent-Length: 15", + "Content-Length: 0\r\nContent-Length:", + ], +) +def test_blank_content_length_is_unreadable(raw): + """The upload preflight gate must refuse it too — there is no length to drain.""" + from api.helpers import unreadable_content_length + + assert unreadable_content_length(cast(Any, _message_with("Host: x\r\n" + raw + "\r\n"))) is True + + +def test_blank_transfer_encoding_is_reported_as_an_undecodable_coding(): + """Named, non-empty and never None, so `is not None` callers and the 411 read right.""" + from api.helpers import unsupported_transfer_encoding + + coding = unsupported_transfer_encoding(cast(Any, _message_with("Host: x\r\nTransfer-Encoding:\r\n"))) + + assert coding is not None + assert coding, "an empty return value would be a falsy non-None trap for callers" + 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 + + +@pytest.mark.parametrize("raw", _BLANK_FRAMING_HEADERS) +def test_sidecar_get_with_blank_framing_value_closes_connection(raw, monkeypatch): + """The reported case, through the production provenance rejection.""" + import api.routes as routes + + from email.parser import Parser + + handler = SimpleNamespace( + path="/api/extensions/ext1/sidecar/proxy", + command="GET", + headers=Parser().parsestr("Host: x\r\n" + raw + "\r\n\r\n"), + 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 + + +@pytest.mark.parametrize( + "raw", + [ + "", + "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. + """ + from api.helpers import request_declares_body + + assert request_declares_body(cast(Any, _message_with("Host: x\r\n" + raw))) 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 @@ -440,6 +569,51 @@ def test_duplicate_content_length_multipart_rejects_before_read(handler_name, mo assert handler.close_connection is True +@pytest.mark.parametrize("handler_name", _MULTIPART_HANDLERS) +@pytest.mark.parametrize( + "raw", + ["Content-Length:", "Content-Length: ", "Content-Length:\r\nContent-Length: 63"], + ids=["blank", "whitespace-only", "blank-plus-real"], +) +def test_blank_content_length_multipart_rejects_before_read(handler_name, raw, monkeypatch): + """A blank length reached `int('' or 0)` == 0 and read the body as empty. + + All four handlers then answered 400 "No file field in request" with keep-alive + and left the whole multipart payload on the socket. Verified live on the + otherwise-fixed head: 400 then `400 Bad request syntax ('--x')`. + """ + from email.parser import Parser + + headers = Parser().parsestr( + "Content-Type: multipart/form-data; boundary=x\r\n" + raw + "\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_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" + ) + 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 + + # ── 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 e39eae2c..93d1a878 100644 --- a/tests/test_rejected_write_pooled_client.py +++ b/tests/test_rejected_write_pooled_client.py @@ -91,13 +91,19 @@ _CHUNKED_MULTIPART_BODY = ( ) -def _pipelined_after(request: bytes) -> bytes: - """Send *request* and a plain GET in one write; return every byte answered.""" +def _pipelined_after(request: bytes, *, stop_after: int | None = None) -> bytes: + """Send *request* and a plain GET in one write; return every byte answered. + + Reads to EOF by default, which is the whole point of the closing cases: the + proof is that NOTHING follows the single response. `stop_after` bounds the + read for the keep-alive cases, where the socket stays open by design and + reading to EOF would only burn the timeout. + """ sock = socket.create_connection(("127.0.0.1", _PORT), timeout=10) try: sock.sendall(request + _FOLLOWING_GET) received = b"" - while True: + while stop_after is None or received.count(b"HTTP/1.1 ") < stop_after: try: chunk = sock.recv(65536) except (TimeoutError, socket.timeout, ConnectionError): @@ -194,3 +200,89 @@ def test_duplicate_content_length_upload_closes_and_cannot_poison_the_socket(pat ) _assert_single_closed_response(answered, b"400", _MULTIPART_PAYLOAD) + + +# ── Framing hidden behind a BLANK header value ──────────────────────────────── +# +# `Content-Length:` with nothing after the colon reads back as '' and was skipped +# as "no length declared", so the rejection kept the connection alive. Both cases +# below were reproduced live on the otherwise-fixed head: +# 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. + +_BLANK_FRAMING_HEADER_LINES = ( + b"Content-Length:\r\n", + b"Content-Length: \r\n", + b"Content-Length:\r\nContent-Length:\r\n", + b"Content-Length: 0\r\nContent-Length:\r\n", + b"Transfer-Encoding:\r\n", +) + + +@pytest.mark.parametrize("framing", _BLANK_FRAMING_HEADER_LINES) +def test_blank_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:\r\n", b"400"), + (b"Content-Length: \r\n", b"400"), + (b"Transfer-Encoding:\r\n", b"411"), + ], + ids=["blank-length", "whitespace-only-length", "blank-transfer-encoding"], +) +def test_blank_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: 0\r\nContent-Length: 0\r\n", + b"Transfer-Encoding: identity\r\n", + ], + 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. + + Framing that positively says "no body" must NOT be swept up by the blank + rule: the rejection answers without `Connection: close` and the pipelined GET + is served on the same socket. Two agreeing zeroes are still no body. + """ + answered = _pipelined_after( + b"GET /api/extensions/probe/sidecar/ping HTTP/1.1\r\n" + b"Host: 127.0.0.1\r\n" + framing + b"\r\n", + stop_after=2, + ) + + text = answered.decode("latin-1", errors="replace") + assert answered.startswith(b"HTTP/1.1 403"), text + assert b"HTTP/1.1 200 OK" in answered, ( + f"the pipelined GET was not served, so a body-less rejection dropped a " + f"healthy pooled connection: {text}" + ) + assert b"Connection: close" not in answered, text From 01f2446459b2646ac31902b048d1ee8eb83268a9 Mon Sep 17 00:00:00 2001 From: Rodrigo Gomes da Silva Date: Sat, 15 Aug 2026 18:22:49 -0300 Subject: [PATCH 12/15] fix(http): read a length only when it is 1*DIGIT, not whenever int() copes `_declared_content_lengths()` parsed with `int()`, which is far looser than RFC 9110's `Content-Length = 1*DIGIT`: it accepts a leading sign, PEP 515 underscores and non-ASCII digits, and `str.strip()` ahead of it also erased non-ASCII whitespace. Every such spelling of ZERO therefore parsed to an honest `0`, took the "every declared length agrees on zero" keep-alive branch, and left the payload in rfile for the server to parse as the next request line -- the same defect the blank value had, one layer down. Reproduced live at the previous 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 greptile filed the P1 for, and identical for `-0`, `0_0`, `+00`, `0_0_0`, `+0_0`, `\xa00`, `0\xa0` and `\x850`. On /api/upload the same values slipped `unreadable_content_length()` into `int('0_0')` == 0 and answered 400 keep-alive then `400 Bad request syntax ('--x')`. A sign was reported safe last round because it was probed as `+15`, where a wrong parse closes anyway; only the zero-valued spellings expose it. The parse is now the grammar: an unsigned run of ASCII digits, or the framing is unreadable and a body is assumed pending. The `int()` guard stays -- a digit run longer than `sys.get_int_max_str_digits()` (4300) raises ValueError too, and a raise on a rejection path becomes a spurious 500. `Transfer-Encoding` gets the same audit. Trimming is now SP/HTAB only, RFC 9110 OWS and nothing else, because `str.strip()` erased U+00A0/U+0085 there too: `Transfer-Encoding: \xa0identity` -- a spelling no token grammar allows -- read back as the `identity` token, so it counted as unframed and poisoned the socket with the same 403-then-400 trace (411 and close on uploads). Both characters reach the wire, since a request line decodes as latin-1. Membership in `_UNFRAMED_TRANSFER_CODINGS` is tested through `str.lower()`, which maps some non-ASCII characters ONTO ASCII ones (U+212A -> `k`), so the set is now documented as ASCII-token-only and a test scans every codepoint to prove no non-ASCII spelling can reach it. Non-ASCII digits are unreadable by the same rule and pinned at the helper level: U+0660/U+FF10 cannot cross an HTTP/1.1 header line (latin-1 leaves only the U+00B2/B3/B9 superscripts, which `int()` already rejects), but the helpers take any headers mapping and enforce the grammar rather than trusting the transport. The no-body contract is unchanged and pinned in both directions on the wire and at the helper: no length header, `0`, `00`, an OWS-padded zero, two AGREEING zeroes, and `identity` in any ASCII case still keep the pooled connection alive. --- api/helpers.py | 44 +++- tests/test_rejected_write_connection_close.py | 233 ++++++++++++++++++ tests/test_rejected_write_pooled_client.py | 76 +++++- 3 files changed, 349 insertions(+), 4 deletions(-) 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": "0"}, 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 + "0", # 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. 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 13/15] 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): From 7862927ef51d8801cb9c5b9b3046ebac9da5d872 Mon Sep 17 00:00:00 2001 From: Rodrigo Gomes da Silva Date: Mon, 17 Aug 2026 02:10:45 -0300 Subject: [PATCH 14/15] fix(http): keep a bodyless rejection's keep-alive, at every reject site The mirror image of the sidecar over-close already fixed here: the sibling reject-before-read sites still armed `arm_connection_close()` unconditionally, so a request with NO declared body was answered with `Connection: close` and the client's pipelined follow-up was dropped -- a healthy pooled connection killed for no framing reason. Reproduced on the wire, pipelined down one socket against the production handler (`responses seen: 1` at each site): POST /api/session/new off-session auth -> 401 + Connection: close POST /api/session/new bad Origin -> 403 + Connection: close POST /api/session/new no CSRF token -> 403 + Connection: close POST /api/csp-report limiter tripped -> 204 + Connection: close POST /api/health/restart success -> 200 + Connection: close POST /api/process-complete-ack -> 410 + Connection: close POST /api/upload no boundary -> 400 + Connection: close Each now routes through `arm_connection_close_if_body_pending()` (the helper the sidecar path already used), so the framing decides in both directions: body-bearing or unreadable framing still closes, an absent or agreeing-zero Content-Length keeps keep-alive. `check_auth_or_close()` cannot swap the helper in directly -- it has to arm before `check_auth()` writes its 401/302 and undo that on success -- so it gates the arming on `request_declares_body()` instead and keeps the success-path restore. `/api/health/restart` is armed once, ahead of the outcome, so all four results (completed / in_progress / busy / error) follow the framing rather than the result; the success path was closing on every call the WebUI makes. Only the boundary check in the upload preflight can reject a bodyless request: every other raise there (any Transfer-Encoding, an unreadable or conflicting length, an over-cap length) already declares a body and still closes. Five tests pinned the unconditional close on stubs that declared no body -- i.e. they pinned the bug. Each is re-grounded on real body-bearing framing (Content-Length, chunked, and an unreadable length) so the close half stays pinned, with a bodyless sibling asserting keep-alive. New real-socket regressions cover all seven sites in both directions. --- api/auth.py | 17 +- api/routes.py | 45 ++- api/upload.py | 17 +- tests/test_rejected_write_connection_close.py | 275 +++++++++++++-- tests/test_rejected_write_pooled_client.py | 323 +++++++++++++++++- 5 files changed, 638 insertions(+), 39 deletions(-) diff --git a/api/auth.py b/api/auth.py index 87f6bc4f..c61ee669 100644 --- a/api/auth.py +++ b/api/auth.py @@ -17,6 +17,7 @@ import time from pathlib import Path from api.config import STATE_DIR, get_config, load_settings +from api.helpers import request_declares_body logger = logging.getLogger(__name__) @@ -1214,12 +1215,26 @@ def check_auth_or_close(handler, parsed) -> bool: can advertise ``Connection: close``; success restores the prior flag so an authenticated request keeps its keep-alive. + Armed ONLY when the request's framing declares a body still queued in + ``rfile`` (see ``request_declares_body()``), which is the same rule the other + reject-before-read sites apply through ``arm_connection_close_if_body_pending()``. + Arming unconditionally was the mirror image of the over-close this PR already + fixed on the sidecar path: a body-less POST that failed auth answered + ``401`` WITH ``Connection: close`` and dropped the client's pipelined + follow-up, killing a healthy keep-alive connection for no framing reason. + Verified on the wire, pipelined down one socket against the production + handler: ``POST /api/session/new`` with no ``Content-Length`` (and with + ``Content-Length: 0``) answered ``401`` + ``Connection: close`` and the + following ``GET /api/auth/status`` was never served. The helper cannot be + swapped in directly here because the arming has to happen before + ``check_auth()`` writes its response and be undone if it succeeds. + ``close_connection`` only exists once BaseHTTPRequestHandler has parsed a request line, so partially-built handler stubs may not have it at all. Read it defensively and only arm/restore when it was really there: inventing the attribute on a stub would leave a bogus keep-alive verdict behind. """ - if not hasattr(handler, 'close_connection'): + if not hasattr(handler, 'close_connection') or not request_declares_body(handler): return check_auth(handler, parsed) prior_close = handler.close_connection handler.close_connection = True diff --git a/api/routes.py b/api/routes.py index decc18eb..daca7c80 100644 --- a/api/routes.py +++ b/api/routes.py @@ -2974,7 +2974,6 @@ from api.helpers import ( require, bad, safe_resolve, - arm_connection_close, arm_connection_close_if_body_pending, j, t, @@ -5873,9 +5872,15 @@ def _csrf_rejection_error(handler) -> str: def _check_csrf(handler) -> bool: """Reject cross-origin or tokenless authenticated browser unsafe requests.""" if not _check_same_origin_browser_request(handler): - # CSRF checks run before read_body(). Close rather than reusing an - # HTTP/1.1 connection whose unread body would corrupt the next request. - arm_connection_close(handler) + # CSRF checks run before read_body(), so close rather than reusing an + # HTTP/1.1 connection whose unread body would corrupt the next request -- + # but ONLY when the framing says bytes are really queued. Arming + # unconditionally dropped a healthy pooled connection on a body-less + # write: verified on the wire, `POST /api/session/new` with + # `Origin: http://evil.invalid` and no `Content-Length` (and with + # `Content-Length: 0`) answered 403 + `Connection: close` and the + # pipelined `GET /api/health/agent` was never served. + arm_connection_close_if_body_pending(handler) return False if not _is_browser_unsafe_request(handler): return True # non-browser clients (curl, MCP, agent) have no Origin/Referer @@ -5888,7 +5893,11 @@ def _check_csrf(handler) -> bool: submitted = handler.headers.get(CSRF_HEADER_NAME) or handler.headers.get("X-CSRF-Token") if verify_csrf_token(cookie_val or "", submitted or ""): return True - arm_connection_close(handler) + # Same framing rule as the origin rejection above: a token mismatch on a + # body-less write has nothing unread to protect. Verified on the wire with an + # authenticated same-origin `POST /api/session/new` carrying no CSRF token and + # no `Content-Length`: 403 + `Connection: close`, follow-up dropped. + arm_connection_close_if_body_pending(handler) return _set_csrf_failure_reason(handler, "token_mismatch") @@ -6519,8 +6528,12 @@ def _handle_csp_report(handler) -> bool: _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. - arm_connection_close(handler) + # so the unread report can't corrupt the next pooled request -- but only + # when a body was really declared. A body-less report answered 204 WITH + # `Connection: close` once the 100-per-60s limiter tripped (reproduced on + # the wire at request 101; the pipelined `GET /api/health/agent` was + # dropped), so a browser that keeps reporting loses its socket each time. + arm_connection_close_if_body_pending(handler) return _send_no_content(handler) payload = _read_csp_report_payload(handler) @@ -12912,9 +12925,15 @@ 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. - arm_connection_close(handler) + # This endpoint never consumes its request body on any outcome, so close when + # one was DECLARED -- and only then. Arming unconditionally closed the socket + # on every call including the successful, body-less one the WebUI actually + # makes: verified on the wire, `POST /api/health/restart` with no + # `Content-Length` answered with `Connection: close` and the pipelined + # `GET /api/health/agent` was never served. The single arming covers every + # outcome below (completed / in_progress / busy / error) because the framing, + # not the result, decides. + arm_connection_close_if_body_pending(handler) outcome = restart_active_profile_gateway() if outcome.get("status") == "completed": @@ -15239,8 +15258,10 @@ def handle_post(handler, parsed) -> bool: 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. - arm_connection_close(handler) + # pooled request -- but a body-less ack has none, and closing then + # just kills keep-alive (on the wire: 410 + `Connection: close` with + # no `Content-Length`, pipelined follow-up dropped). + arm_connection_close_if_body_pending(handler) j( handler, { diff --git a/api/upload.py b/api/upload.py index 4174b389..3701e669 100644 --- a/api/upload.py +++ b/api/upload.py @@ -9,7 +9,7 @@ from pathlib import Path from api.config import MAX_UPLOAD_BYTES, STATE_DIR from api.helpers import ( - arm_connection_close, + arm_connection_close_if_body_pending, j, unreadable_content_length, unsupported_transfer_encoding, @@ -51,8 +51,19 @@ _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.""" - arm_connection_close(handler) + """Reject without reading the body; close so unread bytes can't poison HTTP/1.1 reuse. + + Framing decides, exactly as at the other reject-before-read sites: every + rejection raised by the preflight above (any ``Transfer-Encoding``, an + unreadable or conflicting ``Content-Length``, an over-cap length) already + declares a body and still closes. Only the boundary/`parse_multipart` rejection + can fire on a request that declares none, and closing there dropped a healthy + pooled connection -- on the wire, ``POST /api/upload`` with + ``Content-Type: application/json`` and no ``Content-Length`` answered 400 + "No boundary in Content-Type" WITH ``Connection: close`` and the pipelined + ``GET /api/health/agent`` was never served. + """ + arm_connection_close_if_body_pending(handler) return j(handler, {'error': message}, status=status) diff --git a/tests/test_rejected_write_connection_close.py b/tests/test_rejected_write_connection_close.py index f5e80ce5..e45fd46a 100644 --- a/tests/test_rejected_write_connection_close.py +++ b/tests/test_rejected_write_connection_close.py @@ -11,13 +11,15 @@ from typing import Any, cast import pytest -def test_auth_rejected_write_closes_connection(monkeypatch): +def _rejected_write(monkeypatch, headers): + """Drive the production _handle_write through an auth rejection.""" import api.auth as auth import server handler = SimpleNamespace( path="/api/session/new", command="POST", + headers=headers, close_connection=False, ) monkeypatch.setattr(server, "reset_trusted_auth_request_state", lambda _handler: None) @@ -29,25 +31,65 @@ def test_auth_rejected_write_closes_connection(monkeypatch): raise AssertionError("route ran after auth rejection") server.Handler._handle_write(cast(Any, handler), route_must_not_run) - - assert handler.close_connection is True + return handler -def test_origin_rejected_csrf_closes_connection(monkeypatch): +@pytest.mark.parametrize( + "headers", + [{"Content-Length": "15"}, {"Transfer-Encoding": "chunked"}, {"Content-Length": "banana"}], + ids=["content-length", "chunked", "unreadable-length"], +) +def test_auth_rejected_write_closes_connection(headers, monkeypatch): + """A rejected write whose framing declares a body leaves it unread — close.""" + assert _rejected_write(monkeypatch, headers).close_connection is True + + +@pytest.mark.parametrize( + "headers", [{}, {"Content-Length": "0"}], ids=["no-content-length", "zero-content-length"] +) +def test_bodyless_auth_rejected_write_keeps_connection(headers, monkeypatch): + """The over-close half at the auth gate: no declared body, nothing to protect. + + `check_auth_or_close()` armed unconditionally, so a body-less POST that failed + auth answered 401 WITH `Connection: close` and dropped the client's pipelined + follow-up (reproduced on the wire — see the pooled-client file). Same rule as + the sidecar path: the framing decides, not the method or the site. + """ + assert _rejected_write(monkeypatch, headers).close_connection is False + + +def _csrf_origin_rejection(monkeypatch, headers): import api.routes as routes - handler = SimpleNamespace(close_connection=False) + handler = SimpleNamespace(headers=headers, close_connection=False) monkeypatch.setattr(routes, "_check_same_origin_browser_request", lambda _handler: False) assert routes._check_csrf(handler) is False - assert handler.close_connection is True + return handler -def test_token_rejected_csrf_closes_connection(monkeypatch): +@pytest.mark.parametrize( + "headers", + [{"Content-Length": "15"}, {"Transfer-Encoding": "chunked"}, {"Content-Length": "banana"}], + ids=["content-length", "chunked", "unreadable-length"], +) +def test_origin_rejected_csrf_closes_connection(headers, monkeypatch): + assert _csrf_origin_rejection(monkeypatch, headers).close_connection is True + + +@pytest.mark.parametrize( + "headers", [{}, {"Content-Length": "0"}], ids=["no-content-length", "zero-content-length"] +) +def test_bodyless_origin_rejected_csrf_keeps_connection(headers, monkeypatch): + """A cross-origin rejection of a body-less write must not drop the socket.""" + assert _csrf_origin_rejection(monkeypatch, headers).close_connection is False + + +def _csrf_token_rejection(monkeypatch, headers): import api.auth as auth import api.routes as routes - handler = SimpleNamespace(headers={}, close_connection=False) + handler = SimpleNamespace(headers=headers, close_connection=False) monkeypatch.setattr(routes, "_check_same_origin_browser_request", lambda _handler: True) monkeypatch.setattr(routes, "_is_browser_unsafe_request", lambda _handler: True) monkeypatch.setattr(auth, "is_auth_enabled", lambda: True) @@ -55,7 +97,24 @@ def test_token_rejected_csrf_closes_connection(monkeypatch): monkeypatch.setattr(auth, "verify_csrf_token", lambda _cookie, _token: False) assert routes._check_csrf(handler) is False - assert handler.close_connection is True + return handler + + +@pytest.mark.parametrize( + "headers", + [{"Content-Length": "15"}, {"Transfer-Encoding": "chunked"}, {"Content-Length": "banana"}], + ids=["content-length", "chunked", "unreadable-length"], +) +def test_token_rejected_csrf_closes_connection(headers, monkeypatch): + assert _csrf_token_rejection(monkeypatch, headers).close_connection is True + + +@pytest.mark.parametrize( + "headers", [{}, {"Content-Length": "0"}], ids=["no-content-length", "zero-content-length"] +) +def test_bodyless_token_rejected_csrf_keeps_connection(headers, monkeypatch): + """A token mismatch on a body-less write has nothing unread either.""" + assert _csrf_token_rejection(monkeypatch, headers).close_connection is False def test_sidecar_provenance_rejected_closes_connection(monkeypatch): @@ -671,30 +730,120 @@ def test_transfer_encoding_beside_a_content_length_still_declares_a_body(length) assert request_declares_body(cast(Any, handler)) is True -def test_csp_report_rate_limited_closes_connection(monkeypatch): - """A rate-limited CSP report is dropped before its body is read.""" +def _rate_limited_csp_report(monkeypatch, headers): import api.routes as routes - handler = SimpleNamespace(close_connection=False) + handler = SimpleNamespace(headers=headers, 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 + return handler -def test_health_restart_closes_connection(monkeypatch): - """health/restart never consumes its body on any outcome.""" +@pytest.mark.parametrize( + "headers", + [{"Content-Length": "15"}, {"Transfer-Encoding": "chunked"}, {"Content-Length": "banana"}], + ids=["content-length", "chunked", "unreadable-length"], +) +def test_csp_report_rate_limited_closes_connection(headers, monkeypatch): + """A rate-limited CSP report is dropped before its body is read.""" + assert _rate_limited_csp_report(monkeypatch, headers).close_connection is True + + +@pytest.mark.parametrize( + "headers", [{}, {"Content-Length": "0"}], ids=["no-content-length", "zero-content-length"] +) +def test_bodyless_rate_limited_csp_report_keeps_connection(headers, monkeypatch): + """A body-less report dropped by the limiter must not cost the browser its socket.""" + assert _rate_limited_csp_report(monkeypatch, headers).close_connection is False + + +_RESTART_OUTCOMES = [ + {"status": "completed"}, + {"status": "in_progress"}, + {"status": "busy"}, + {"status": "error"}, +] + + +@pytest.mark.parametrize("outcome", _RESTART_OUTCOMES, ids=lambda o: o["status"]) +@pytest.mark.parametrize( + "headers", + [{"Content-Length": "15"}, {"Transfer-Encoding": "chunked"}, {"Content-Length": "banana"}], + ids=["content-length", "chunked", "unreadable-length"], +) +def test_health_restart_closes_connection(headers, outcome, monkeypatch): + """health/restart never consumes its body on any outcome — so a declared one closes.""" import api.routes as routes - handler = SimpleNamespace(close_connection=False) - monkeypatch.setattr(routes, "restart_active_profile_gateway", lambda: {"status": "completed"}) + handler = SimpleNamespace(headers=headers, close_connection=False) + monkeypatch.setattr(routes, "restart_active_profile_gateway", lambda: outcome) monkeypatch.setattr(routes, "j", lambda _handler, _payload, status=200: True) routes._handle_health_restart(handler) assert handler.close_connection is True +@pytest.mark.parametrize("outcome", _RESTART_OUTCOMES, ids=lambda o: o["status"]) +@pytest.mark.parametrize( + "headers", [{}, {"Content-Length": "0"}], ids=["no-content-length", "zero-content-length"] +) +def test_bodyless_health_restart_keeps_connection(headers, outcome, monkeypatch): + """Including the SUCCESS path, which closed a healthy socket on every call. + + The WebUI's restart button sends no body, so the old unconditional arming made + a 200 "restarted successfully" hang up the connection every single time. The + framing decides for all four outcomes, not the result. + """ + import api.routes as routes + + handler = SimpleNamespace(headers=headers, close_connection=False) + monkeypatch.setattr(routes, "restart_active_profile_gateway", lambda: outcome) + monkeypatch.setattr(routes, "j", lambda _handler, _payload, status=200: True) + + routes._handle_health_restart(handler) + assert handler.close_connection is False + + +def _deprecated_ack_post(monkeypatch, headers): + """Drive the deprecated /api/process-complete-ack 410, which runs pre-CSRF.""" + import api.routes as routes + + handler = SimpleNamespace( + path="/api/process-complete-ack", + command="POST", + headers=headers, + close_connection=False, + ) + monkeypatch.setattr( + routes, "j", lambda _handler, _payload, status=200, extra_headers=None: True + ) + + assert routes.handle_post( + cast(Any, handler), SimpleNamespace(path=handler.path, query="") + ) is True + return handler + + +@pytest.mark.parametrize( + "headers", + [{"Content-Length": "15"}, {"Transfer-Encoding": "chunked"}, {"Content-Length": "banana"}], + ids=["content-length", "chunked", "unreadable-length"], +) +def test_deprecated_ack_with_a_body_closes_connection(headers, monkeypatch): + """A stale tab's JSON ack is never read, so a declared body must close.""" + assert _deprecated_ack_post(monkeypatch, headers).close_connection is True + + +@pytest.mark.parametrize( + "headers", [{}, {"Content-Length": "0"}], ids=["no-content-length", "zero-content-length"] +) +def test_bodyless_deprecated_ack_keeps_connection(headers, monkeypatch): + """A body-less ack has nothing queued — the 410 must not kill keep-alive.""" + assert _deprecated_ack_post(monkeypatch, headers).close_connection is False + + def test_upload_oversize_rejects_before_read(monkeypatch): """An oversized upload's 413 fires before rfile.read() — connection closes.""" import api.upload as upload @@ -779,6 +928,52 @@ def test_bodyless_multipart_rejection_keeps_connection(handler_name, monkeypatch assert handler.close_connection is False +@pytest.mark.parametrize("handler_name", _MULTIPART_HANDLERS) +@pytest.mark.parametrize( + "headers", + [ + {"Content-Type": "application/json"}, + {"Content-Type": "application/json", "Content-Length": "0"}, + {"Content-Type": "multipart/form-data"}, # multipart, but no boundary parameter + ], + ids=["json-no-length", "json-zero-length", "multipart-without-boundary"], +) +def test_bodyless_boundary_rejection_keeps_connection(handler_name, headers, monkeypatch): + """`_reject_before_read()` fires on a body-less request too — via the boundary check. + + Every other rejection the preflight raises (any Transfer-Encoding, an unreadable + or conflicting length, an over-cap length) already declares a body. The missing + `boundary=` in Content-Type is the one that can fire with no body at all, and + the unconditional arming closed a healthy pooled socket for it: on the wire, + `POST /api/upload` with `Content-Type: application/json` and no `Content-Length` + answered 400 "No boundary in Content-Type" WITH `Connection: close` and the + pipelined GET was dropped. `_NeverRead` also proves the boundary check still + runs ahead of any `rfile.read()`. + """ + handler, answered = _run_multipart_handler( + monkeypatch, handler_name, headers, _NeverRead() + ) + + assert answered["status"] == 400, answered + assert answered["payload"]["error"] == "No boundary in Content-Type", answered + assert handler.close_connection is False + + +@pytest.mark.parametrize("handler_name", _MULTIPART_HANDLERS) +def test_boundary_rejection_with_a_declared_body_still_closes(handler_name, monkeypatch): + """The close half of the same rejection: a declared body is still queued.""" + handler, answered = _run_multipart_handler( + monkeypatch, + handler_name, + {"Content-Type": "application/json", "Content-Length": "15"}, + _NeverRead(), + ) + + assert answered["status"] == 400, answered + assert answered["payload"]["error"] == "No boundary in Content-Type", answered + assert handler.close_connection is True + + @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.""" @@ -1038,17 +1233,57 @@ def test_check_auth_or_close_on_handler_stub_without_close_connection(monkeypatc assert not hasattr(handler, "close_connection") -def test_check_auth_or_close_restores_keep_alive_on_success(monkeypatch): - """A successful auth must not inherit the armed close.""" +@pytest.mark.parametrize( + "headers", + [{}, {"Content-Length": "0"}, {"Content-Length": "15"}, {"Transfer-Encoding": "chunked"}], + ids=["no-content-length", "zero-content-length", "content-length", "chunked"], +) +def test_check_auth_or_close_restores_keep_alive_on_success(headers, monkeypatch): + """A successful auth must not inherit the armed close. + + The two body-bearing rows are the ones that actually arm before `check_auth()`, + so they are what pins the restore; the body-less rows never arm at all. + """ import api.auth as auth - handler = SimpleNamespace(close_connection=False) + handler = SimpleNamespace(headers=headers, close_connection=False) monkeypatch.setattr(auth, "check_auth", lambda _handler, _parsed: True) assert auth.check_auth_or_close(cast(Any, handler), SimpleNamespace(path="/api/x")) is True assert handler.close_connection is False +def test_check_auth_or_close_restores_a_prior_close_on_success(monkeypatch): + """Restore means restore: a connection already doomed stays doomed. + + `Connection: close` sent by the client sets `close_connection` before routing, + and the restore must not overwrite that with keep-alive. + """ + import api.auth as auth + + handler = SimpleNamespace(headers={"Content-Length": "15"}, close_connection=True) + monkeypatch.setattr(auth, "check_auth", lambda _handler, _parsed: True) + + assert auth.check_auth_or_close(cast(Any, handler), SimpleNamespace(path="/api/x")) is True + assert handler.close_connection is True + + +def test_check_auth_or_close_tolerates_a_handler_without_headers(monkeypatch): + """The framing read must never raise here: an exception becomes a spurious 500.""" + import api.auth as auth + + monkeypatch.setattr(auth, "check_auth", lambda _handler, _parsed: False) + + for handler in ( + SimpleNamespace(close_connection=False), + SimpleNamespace(headers=None, close_connection=False), + SimpleNamespace(headers=object(), close_connection=False), + ): + assert auth.check_auth_or_close(cast(Any, handler), SimpleNamespace(path="/api/x")) is False + # No framing could be read, so nothing proves a body is pending. + assert handler.close_connection is False + + def test_arm_connection_close_tolerates_a_read_only_handler(): """Arming must never turn a 4xx into a 500, even on an odd handler.""" from api.helpers import arm_connection_close diff --git a/tests/test_rejected_write_pooled_client.py b/tests/test_rejected_write_pooled_client.py index 229059f9..6cbcbed1 100644 --- a/tests/test_rejected_write_pooled_client.py +++ b/tests/test_rejected_write_pooled_client.py @@ -18,9 +18,12 @@ before reading its JSON body, by design (it runs ahead of the CSRF gate so a stale tab gets 410, not 403). """ +import contextlib import http.client import socket +import threading import urllib.parse +from http.server import ThreadingHTTPServer import pytest @@ -91,17 +94,26 @@ _CHUNKED_MULTIPART_BODY = ( ) -def _pipelined_after(request: bytes, *, stop_after: int | None = None) -> bytes: +def _pipelined_after( + request: bytes, + *, + stop_after: int | None = None, + port: int | None = None, + follow: bytes = _FOLLOWING_GET, +) -> bytes: """Send *request* and a plain GET in one write; return every byte answered. Reads to EOF by default, which is the whole point of the closing cases: the proof is that NOTHING follows the single response. `stop_after` bounds the read for the keep-alive cases, where the socket stays open by design and reading to EOF would only burn the timeout. + + `port`/`follow` exist for the auth-enabled cases below, which need their own + server instance and a PUBLIC follow-up path. """ - sock = socket.create_connection(("127.0.0.1", _PORT), timeout=10) + sock = socket.create_connection(("127.0.0.1", port or _PORT), timeout=10) try: - sock.sendall(request + _FOLLOWING_GET) + sock.sendall(request + follow) received = b"" while stop_after is None or received.count(b"HTTP/1.1 ") < stop_after: try: @@ -440,3 +452,308 @@ def test_bodyless_framing_keeps_the_pooled_socket_alive(framing): f"healthy pooled connection: {text}" ) assert b"Connection: close" not in answered, text + + +# ── Every reject-before-read site, in BOTH directions, on the wire ───────────── +# +# The sidecar path above was the first site routed through the framing-aware +# helper; the sibling sites kept the unconditional `arm_connection_close()` and +# so were the mirror image of the same bug -- a body-less rejection answered with +# `Connection: close` and dropped the client's pipelined follow-up, killing a +# healthy keep-alive connection for no framing reason. Each pair below is the +# reproduction (body-less -> must stay alive) beside its close half (a declared +# body -> must still close), pipelined down one socket against the production +# handler. Reproduced on the pre-fix head, all seven sites, `responses seen: 1`: +# +# POST /api/session/new (auth off-session) -> 401 + Connection: close +# POST /api/session/new (bad Origin) -> 403 + Connection: close +# POST /api/session/new (no CSRF token) -> 403 + Connection: close +# POST /api/csp-report (limiter tripped) -> 204 + Connection: close +# POST /api/health/restart (success) -> 200 + Connection: close +# POST /api/process-complete-ack -> 410 + Connection: close +# POST /api/upload (no boundary) -> 400 + Connection: close + +_BODYLESS_FRAMING = [b"", b"Content-Length: 0\r\n"] +_BODYLESS_IDS = ["no-content-length", "zero-content-length"] +# A PUBLIC path, so the follow-up is a genuine 200 even with auth enabled. +_PUBLIC_FOLLOWING_GET = ( + b"GET /api/auth/status HTTP/1.1\r\nHost: 127.0.0.1\r\nAccept: */*\r\n\r\n" +) + + +@contextlib.contextmanager +def _own_server(): + """The production Handler on a private port. + + The shared test server runs with auth disabled and out of process, so the + auth/CSRF-token rejections and the deterministic restart/limiter outcomes need + an instance this process can configure. It is the same `server.Handler` class + the real server binds, driven over a real socket. + """ + import server + + httpd = ThreadingHTTPServer(("127.0.0.1", 0), server.Handler) + thread = threading.Thread(target=httpd.serve_forever, daemon=True) + thread.start() + try: + yield httpd.server_address[1] + finally: + httpd.shutdown() + httpd.server_close() + thread.join(timeout=10) + + +@pytest.fixture +def auth_on(monkeypatch): + """Turn password auth on for this test (conftest resets the hash cache).""" + import api.auth as auth + + monkeypatch.setenv("HERMES_WEBUI_PASSWORD", "pooled-client-regression") + auth._invalidate_password_hash_cache() + assert auth.is_auth_enabled(), "the auth-path regression needs auth enabled" + return auth + + +def _assert_kept_alive(answered: bytes, status: bytes) -> None: + text = answered.decode("latin-1", errors="replace") + assert answered.startswith(b"HTTP/1.1 " + status), text + assert b"Connection: close" not in answered, ( + f"a body-less rejection advertised close, so a pooled client's healthy " + f"connection was dropped: {text}" + ) + assert b"HTTP/1.1 200 OK" in answered, ( + f"the pipelined follow-up was not served on the same socket: {text}" + ) + + +def _assert_closed(answered: bytes, status: 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}" + ) + + +@pytest.mark.parametrize("framing", _BODYLESS_FRAMING, ids=_BODYLESS_IDS) +def test_bodyless_auth_rejection_keeps_the_pooled_socket_alive(framing, auth_on): + """A body-less POST that fails auth must not cost the client its connection.""" + with _own_server() as port: + answered = _pipelined_after( + b"POST /api/session/new HTTP/1.1\r\nHost: 127.0.0.1\r\n" + framing + b"\r\n", + stop_after=2, + port=port, + follow=_PUBLIC_FOLLOWING_GET, + ) + + _assert_kept_alive(answered, b"401") + + +def test_auth_rejection_with_a_body_still_closes_the_pooled_socket(auth_on): + """The close half at the auth gate: those bytes are still queued in rfile.""" + with _own_server() as port: + answered = _pipelined_after( + b"POST /api/session/new HTTP/1.1\r\nHost: 127.0.0.1\r\n" + b"Content-Type: application/json\r\n" + b"Content-Length: " + str(len(_BODY)).encode() + b"\r\n\r\n" + _BODY, + port=port, + follow=_PUBLIC_FOLLOWING_GET, + ) + + _assert_closed(answered, b"401") + assert _BODY not in answered, answered.decode("latin-1", errors="replace") + + +@pytest.mark.parametrize("framing", _BODYLESS_FRAMING, ids=_BODYLESS_IDS) +def test_bodyless_csrf_origin_rejection_keeps_the_pooled_socket_alive(framing): + """`_check_csrf()` origin mismatch, on the shared server (auth off).""" + answered = _pipelined_after( + b"POST /api/session/new HTTP/1.1\r\nHost: 127.0.0.1\r\n" + b"Origin: http://evil.invalid\r\n" + framing + b"\r\n", + stop_after=2, + ) + + _assert_kept_alive(answered, b"403") + + +def test_csrf_origin_rejection_with_a_body_still_closes_the_pooled_socket(): + answered = _pipelined_after( + b"POST /api/session/new HTTP/1.1\r\nHost: 127.0.0.1\r\n" + b"Origin: http://evil.invalid\r\nContent-Type: application/json\r\n" + b"Content-Length: " + str(len(_BODY)).encode() + b"\r\n\r\n" + _BODY + ) + + _assert_closed(answered, b"403") + assert _BODY not in answered, answered.decode("latin-1", errors="replace") + + +def _authenticated_same_origin_post(port, cookie_name, cookie, framing, body=b""): + return ( + f"POST /api/session/new HTTP/1.1\r\nHost: 127.0.0.1:{port}\r\n" + f"Origin: http://127.0.0.1:{port}\r\n" + f"Cookie: {cookie_name}={cookie}\r\n".encode() + framing + b"\r\n" + body + ) + + +@pytest.mark.parametrize("framing", _BODYLESS_FRAMING, ids=_BODYLESS_IDS) +def test_bodyless_csrf_token_rejection_keeps_the_pooled_socket_alive(framing, auth_on): + """A real session, a same-origin POST, and no CSRF token -> token_mismatch.""" + cookie = auth_on.create_session() + try: + with _own_server() as port: + answered = _pipelined_after( + _authenticated_same_origin_post( + port, auth_on._resolve_cookie_name(), cookie, framing + ), + stop_after=2, + port=port, + follow=_PUBLIC_FOLLOWING_GET, + ) + finally: + auth_on.invalidate_session(cookie) + + _assert_kept_alive(answered, b"403") + + +def test_csrf_token_rejection_with_a_body_still_closes_the_pooled_socket(auth_on): + cookie = auth_on.create_session() + try: + with _own_server() as port: + answered = _pipelined_after( + _authenticated_same_origin_post( + port, + auth_on._resolve_cookie_name(), + cookie, + b"Content-Type: application/json\r\nContent-Length: " + + str(len(_BODY)).encode() + + b"\r\n", + body=_BODY, + ), + port=port, + follow=_PUBLIC_FOLLOWING_GET, + ) + finally: + auth_on.invalidate_session(cookie) + + _assert_closed(answered, b"403") + assert _BODY not in answered, answered.decode("latin-1", errors="replace") + + +@pytest.fixture +def csp_limiter_tripped(monkeypatch): + """Force the CSP-report limiter, instead of sending 101 reports in 60s.""" + import api.routes as routes + + monkeypatch.setattr(routes, "_csp_report_rate_limited", lambda _handler: True) + + +@pytest.mark.parametrize("framing", _BODYLESS_FRAMING, ids=_BODYLESS_IDS) +def test_bodyless_rate_limited_csp_report_keeps_the_pooled_socket_alive( + framing, csp_limiter_tripped +): + """A dropped 204 must not hang up on a browser that is still reporting.""" + with _own_server() as port: + answered = _pipelined_after( + b"POST /api/csp-report HTTP/1.1\r\nHost: 127.0.0.1\r\n" + framing + b"\r\n", + stop_after=2, + port=port, + follow=_PUBLIC_FOLLOWING_GET, + ) + + _assert_kept_alive(answered, b"204") + + +def test_rate_limited_csp_report_with_a_body_still_closes(csp_limiter_tripped): + """The limiter answers without reading the report, so a declared body closes.""" + with _own_server() as port: + answered = _pipelined_after( + b"POST /api/csp-report HTTP/1.1\r\nHost: 127.0.0.1\r\n" + b"Content-Type: application/csp-report\r\n" + b"Content-Length: " + str(len(_BODY)).encode() + b"\r\n\r\n" + _BODY, + port=port, + follow=_PUBLIC_FOLLOWING_GET, + ) + + _assert_closed(answered, b"204") + + +@pytest.fixture +def restart_succeeds(monkeypatch): + """Pin the SUCCESS outcome -- the one that closed a healthy socket every call.""" + import api.routes as routes + + monkeypatch.setattr( + routes, "restart_active_profile_gateway", lambda: {"status": "completed"} + ) + + +@pytest.mark.parametrize("framing", _BODYLESS_FRAMING, ids=_BODYLESS_IDS) +def test_bodyless_successful_restart_keeps_the_pooled_socket_alive(framing, restart_succeeds): + """`/api/health/restart` closed on success too; the WebUI sends no body.""" + with _own_server() as port: + answered = _pipelined_after( + b"POST /api/health/restart HTTP/1.1\r\nHost: 127.0.0.1\r\n" + framing + b"\r\n", + stop_after=2, + port=port, + follow=_PUBLIC_FOLLOWING_GET, + ) + + _assert_kept_alive(answered, b"200") + + +def test_restart_with_a_body_still_closes_the_pooled_socket(restart_succeeds): + """The endpoint consumes its body on no outcome, so a declared one closes.""" + with _own_server() as port: + answered = _pipelined_after( + b"POST /api/health/restart HTTP/1.1\r\nHost: 127.0.0.1\r\n" + b"Content-Type: application/json\r\n" + b"Content-Length: " + str(len(_BODY)).encode() + b"\r\n\r\n" + _BODY, + port=port, + follow=_PUBLIC_FOLLOWING_GET, + ) + + _assert_closed(answered, b"200") + assert _BODY not in answered, answered.decode("latin-1", errors="replace") + + +@pytest.mark.parametrize("framing", _BODYLESS_FRAMING, ids=_BODYLESS_IDS) +def test_bodyless_deprecated_ack_keeps_the_pooled_socket_alive(framing): + """The 410 alias, which the body-bearing test at the top of this file pins closed.""" + answered = _pipelined_after( + b"POST /api/process-complete-ack HTTP/1.1\r\nHost: 127.0.0.1\r\n" + framing + b"\r\n", + stop_after=2, + ) + + _assert_kept_alive(answered, b"410") + + +@pytest.mark.parametrize("path", _MULTIPART_UPLOAD_PATHS) +@pytest.mark.parametrize( + "framing", + [ + b"Content-Type: application/json\r\n", + b"Content-Type: application/json\r\nContent-Length: 0\r\n", + b"Content-Type: multipart/form-data\r\n", # multipart, but no boundary= + ], + ids=["json-no-length", "json-zero-length", "multipart-without-boundary"], +) +def test_bodyless_upload_boundary_rejection_keeps_the_pooled_socket_alive(path, framing): + """`_reject_before_read()`'s one body-less reachable rejection: the boundary check.""" + answered = _pipelined_after( + f"POST {path} HTTP/1.1\r\nHost: 127.0.0.1\r\n".encode() + framing + b"\r\n", + stop_after=2, + ) + + _assert_kept_alive(answered, b"400") + + +@pytest.mark.parametrize("path", _MULTIPART_UPLOAD_PATHS) +def test_upload_boundary_rejection_with_a_body_still_closes(path): + answered = _pipelined_after( + f"POST {path} HTTP/1.1\r\nHost: 127.0.0.1\r\n" + "Content-Type: application/json\r\n" + f"Content-Length: {len(_BODY)}\r\n\r\n".encode() + _BODY + ) + + _assert_closed(answered, b"400") + assert _BODY not in answered, answered.decode("latin-1", errors="replace") From 5982a56698a0b7c5203a92a23e67375168c01abb Mon Sep 17 00:00:00 2001 From: nesquena-hermes Date: Tue, 22 Sep 2026 12:53:29 +0000 Subject: [PATCH 15/15] fix(server): close the do_GET auth residual + recover comma-combined Content-Length Gate fixes on top of @rodrigogs's #6658, both independently reproduced first. do_GET called bare check_auth while every write path used check_auth_or_close, so a body-bearing GET failing top-level auth left its bytes queued on a keep-alive socket. check_auth_or_close is already body-pending-aware, so the bodyless login-redirect flow keeps its keep-alive. Removing the now-unused check_auth import (ruff F401) broke 9 tests that monkeypatch that symbol through a module alias; rewritten to the new name. _declared_content_lengths ran isdigit() across the whole raw header value, so a comma-combined 'Content-Length: 0, 0' -- the same bodyless request as two repeated headers under RFC 9110 5.3 -- read as unreadable and over-closed a healthy connection. Now splits on commas before validating each member; disagreeing lists still fail closed. Verified: 16/16 adversarial CL vectors still fail closed (no fail-open reopened), 13/13 over-close cases fixed, live-socket drive shows close armed on a body-bearing reject and keep-alive preserved on a bodyless one, and a chunked request behaves byte-identically to master (master never supported chunked either, so no proxy deployment regresses). Co-authored-by: rodrigogs --- CHANGELOG.md | 12 ++ api/helpers.py | 21 ++- server.py | 5 +- tests/test_broken_pipe_cascade.py | 48 +++--- tests/test_comma_combined_content_length.py | 105 ++++++++++++ tests/test_get_with_body_auth_reject_reuse.py | 158 ++++++++++++++++++ tests/test_issue1909_csp_report_only.py | 2 +- tests/test_production_do_get_body_pending.py | 127 ++++++++++++++ 8 files changed, 444 insertions(+), 34 deletions(-) create mode 100644 tests/test_comma_combined_content_length.py create mode 100644 tests/test_get_with_body_auth_reject_reuse.py create mode 100644 tests/test_production_do_get_body_pending.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 8b97d737..60f5ee11 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,18 @@ ### Fixed +- **A rejected request no longer poisons the next one on the same connection.** `server.py` + is a raw HTTP/1.1 handler where `rfile` is the socket itself, so answering a request + before reading its body left those bytes queued. The next request on a keep-alive + connection was then parsed starting mid-body, and the client got + `400 Bad request syntax ('{"a": "b"}GET /api/health HTTP/1.1')` — an error naming a + request it never sent, which is expensive to diagnose from the client side. Every + reject path now arms `Connection: close` when a body is still pending, and framing is + validated strictly (RFC 9110 `1*DIGIT`, duplicate and comma-combined `Content-Length` + reconciled, every `Transfer-Encoding` refused since nothing here decodes one). A + genuinely bodyless rejection keeps its keep-alive, so healthy pooled connections are + not dropped. Thanks @rodrigogs. (#7550, #6658) + - **Opening a session that belongs to another profile now offers to switch to it instead of looking deleted.** Several cross-profile guards answered `404 Session not found`, which the front-end treats as a missing session and self-heals by clearing the URL and local storage — diff --git a/api/helpers.py b/api/helpers.py index 9de52ba5..1984bf28 100644 --- a/api/helpers.py +++ b/api/helpers.py @@ -306,13 +306,20 @@ def _declared_content_lengths(handler) -> set[int] | None: """ parsed: set[int] = set() for raw in _framing_header_values(handler, 'Content-Length'): - stripped = raw.strip(_FIELD_VALUE_OWS) - if not (stripped.isascii() and stripped.isdigit()): - return None - try: - parsed.add(int(stripped)) - except ValueError: - return None + # RFC 9110 §5.3: a list-valued field may arrive as repeated lines OR as + # one comma-combined line, and the two spellings mean the same thing. + # So "0, 0" is the same bodyless request as two "Content-Length: 0" + # headers and must keep its keep-alive; only a DISAGREEING list (caught + # below by len(parsed) > 1) is unframeable. Without the split the comma + # fails isdigit() and every agreeing list over-closed a healthy socket. + for member in raw.split(','): + stripped = member.strip(_FIELD_VALUE_OWS) + if not (stripped.isascii() and stripped.isdigit()): + return None + try: + parsed.add(int(stripped)) + except ValueError: + return None return parsed diff --git a/server.py b/server.py index 39af7793..06fc9a21 100644 --- a/server.py +++ b/server.py @@ -100,7 +100,7 @@ from urllib.parse import urlparse logger = logging.getLogger(__name__) from api.request_logging import emit_request_log -from api.auth import check_auth, check_auth_or_close, reset_trusted_auth_request_state +from api.auth import check_auth_or_close, reset_trusted_auth_request_state from api.config import HOST, PORT, STATE_DIR, SESSION_DIR, DEFAULT_WORKSPACE from api.helpers import ( j, @@ -378,7 +378,8 @@ class Handler(BaseHTTPRequestHandler): set_request_profile(cookie_profile) try: parsed = urlparse(self.path) - if not check_auth(self, parsed): return + # Body-pending-aware: a body-bearing GET failing auth would poison reuse (#7550). + if not check_auth_or_close(self, parsed): return result = handle_get(self, parsed) if result is False: return j(self, {'error': 'not found'}, status=404) diff --git a/tests/test_broken_pipe_cascade.py b/tests/test_broken_pipe_cascade.py index c981d3eb..9e612670 100644 --- a/tests/test_broken_pipe_cascade.py +++ b/tests/test_broken_pipe_cascade.py @@ -227,14 +227,14 @@ class TestServerDisconnectHandling(unittest.TestCase): # Patch handle_get to raise BrokenPipeError import server as _server_mod orig_handle_get = _server_mod.handle_get - orig_check_auth = _server_mod.check_auth + orig_check_auth = _server_mod.check_auth_or_close _server_mod.handle_get = _fake_handle_get - _server_mod.check_auth = lambda h, p: True + _server_mod.check_auth_or_close = lambda h, p: True try: Handler.do_GET(handler) finally: _server_mod.handle_get = orig_handle_get - _server_mod.check_auth = orig_check_auth + _server_mod.check_auth_or_close = orig_check_auth # send_response should NEVER be called for the 500 — client is gone handler.send_response.assert_not_called() @@ -248,12 +248,12 @@ class TestServerDisconnectHandling(unittest.TestCase): raise self._route_raises import server as _server_mod - orig_check_auth = _server_mod.check_auth - _server_mod.check_auth = lambda h, p: True + orig_check_auth = _server_mod.check_auth_or_close + _server_mod.check_auth_or_close = lambda h, p: True try: Handler._handle_write(handler, _fake_route) finally: - _server_mod.check_auth = orig_check_auth + _server_mod.check_auth_or_close = orig_check_auth handler.send_response.assert_not_called() @@ -286,9 +286,9 @@ class TestServerDisconnectHandling(unittest.TestCase): import server as _server_mod orig_handle_get = _server_mod.handle_get - orig_check_auth = _server_mod.check_auth + orig_check_auth = _server_mod.check_auth_or_close _server_mod.handle_get = _fake_handle_get - _server_mod.check_auth = lambda h, p: True + _server_mod.check_auth_or_close = lambda h, p: True closed_stdout = io.StringIO() closed_stdout.close() @@ -299,7 +299,7 @@ class TestServerDisconnectHandling(unittest.TestCase): finally: sys.stdout = original_stdout _server_mod.handle_get = orig_handle_get - _server_mod.check_auth = orig_check_auth + _server_mod.check_auth_or_close = orig_check_auth handler.send_response.assert_called_once_with(500) @@ -312,14 +312,14 @@ class TestServerDisconnectHandling(unittest.TestCase): import server as _server_mod orig_handle_get = _server_mod.handle_get - orig_check_auth = _server_mod.check_auth + orig_check_auth = _server_mod.check_auth_or_close _server_mod.handle_get = _fake_handle_get - _server_mod.check_auth = lambda h, p: True + _server_mod.check_auth_or_close = lambda h, p: True try: Handler.do_GET(handler) finally: _server_mod.handle_get = orig_handle_get - _server_mod.check_auth = orig_check_auth + _server_mod.check_auth_or_close = orig_check_auth # Should send 500 for real errors handler.send_response.assert_called_once_with(500) @@ -334,14 +334,14 @@ class TestServerDisconnectHandling(unittest.TestCase): import server as _server_mod orig_handle_get = _server_mod.handle_get - orig_check_auth = _server_mod.check_auth + orig_check_auth = _server_mod.check_auth_or_close _server_mod.handle_get = _fake_handle_get - _server_mod.check_auth = lambda h, p: True + _server_mod.check_auth_or_close = lambda h, p: True try: Handler.do_GET(handler) finally: _server_mod.handle_get = orig_handle_get - _server_mod.check_auth = orig_check_auth + _server_mod.check_auth_or_close = orig_check_auth handler.send_response.assert_not_called() @@ -355,14 +355,14 @@ class TestServerDisconnectHandling(unittest.TestCase): import server as _server_mod orig_handle_get = _server_mod.handle_get - orig_check_auth = _server_mod.check_auth + orig_check_auth = _server_mod.check_auth_or_close _server_mod.handle_get = _fake_handle_get - _server_mod.check_auth = lambda h, p: True + _server_mod.check_auth_or_close = lambda h, p: True try: Handler.do_GET(handler) finally: _server_mod.handle_get = orig_handle_get - _server_mod.check_auth = orig_check_auth + _server_mod.check_auth_or_close = orig_check_auth handler.send_response.assert_not_called() @@ -406,14 +406,14 @@ class TestServer500ResponseSafety(unittest.TestCase): import server as _server_mod orig_handle_get = _server_mod.handle_get - orig_check_auth = _server_mod.check_auth + orig_check_auth = _server_mod.check_auth_or_close _server_mod.handle_get = _fake_handle_get - _server_mod.check_auth = lambda h, p: True + _server_mod.check_auth_or_close = lambda h, p: True try: Handler.do_GET(handler) finally: _server_mod.handle_get = orig_handle_get - _server_mod.check_auth = orig_check_auth + _server_mod.check_auth_or_close = orig_check_auth # send_response WAS called (we tried to send 500), but write failed handler.send_response.assert_called_once_with(500) @@ -431,14 +431,14 @@ class TestServer500ResponseSafety(unittest.TestCase): import server as _server_mod orig_handle_get = _server_mod.handle_get - orig_check_auth = _server_mod.check_auth + orig_check_auth = _server_mod.check_auth_or_close _server_mod.handle_get = _fake_handle_get - _server_mod.check_auth = lambda h, p: True + _server_mod.check_auth_or_close = lambda h, p: True try: Handler.do_GET(handler) finally: _server_mod.handle_get = orig_handle_get - _server_mod.check_auth = orig_check_auth + _server_mod.check_auth_or_close = orig_check_auth # send_response WAS called (we tried to send 500), but write failed handler.send_response.assert_called_once_with(500) diff --git a/tests/test_comma_combined_content_length.py b/tests/test_comma_combined_content_length.py new file mode 100644 index 00000000..fc93f2a3 --- /dev/null +++ b/tests/test_comma_combined_content_length.py @@ -0,0 +1,105 @@ +"""Comma-combined ``Content-Length`` must not over-close a healthy connection. + +RFC 9110 §5.3 lets a list-valued field arrive either as repeated header lines or +as a single comma-combined line, and the two spellings are equivalent. So +``Content-Length: 0, 0`` describes the same bodyless request as two separate +``Content-Length: 0`` headers, and rejecting it must keep the keep-alive. + +``_declared_content_lengths()`` originally ran ``isdigit()`` across the whole raw +value, so the comma failed the check, the field read as *unreadable*, and every +helper-backed rejection armed ``Connection: close`` on a request with nothing +queued behind it. That is the over-close half of this PR's contract — the mirror +image of the bug it fixes, and the one that silently drops pooled connections. + +The dangerous direction has to stay closed, so the disagreeing and malformed +spellings are asserted here alongside the recoverable ones: ``0, 42`` genuinely +cannot frame a body and must still be refused with the close armed. +""" + +import pytest + +from api.helpers import request_declares_body, unreadable_content_length + + +class _Hdrs(dict): + """email.message-like: ``get()`` returns the first value, ``get_all()`` all.""" + + def get(self, k, default=None): + for key, val in self.items(): + if key.lower() == k.lower(): + return val[0] if isinstance(val, list) else val + return default + + def get_all(self, k, default=None): + out = [] + for key, val in self.items(): + if key.lower() == k.lower(): + out.extend(val if isinstance(val, list) else [val]) + return out or default + + +class _H: + def __init__(self, headers): + self.headers = headers + + +def _handler(cl): + return _H(_Hdrs({"Content-Length": cl})) + + +@pytest.mark.parametrize("value", ["0, 0", "00, 00", "0 , 0", "0,0", "0,\t0"]) +def test_agreeing_bodyless_comma_list_is_readable_and_bodyless(value): + """An agreeing all-zero list is framed, bodyless, and must keep keep-alive.""" + h = _handler(value) + assert unreadable_content_length(h) is False, ( + f"Content-Length {value!r} read as unreadable; a bodyless rejection would " + "then arm Connection: close and drop a healthy pooled connection" + ) + assert bool(request_declares_body(h)) is False, ( + f"Content-Length {value!r} declares no body, so nothing is queued" + ) + + +@pytest.mark.parametrize("value", ["42, 42", "42,42", "042, 42"]) +def test_agreeing_nonzero_comma_list_is_readable_with_a_body(value): + """An agreeing nonzero list frames a real body: readable, and a body IS pending.""" + h = _handler(value) + assert unreadable_content_length(h) is False, value + assert bool(request_declares_body(h)) is True, value + + +@pytest.mark.parametrize("value", ["0, 42", "42, 0", "0, 0, 42"]) +def test_disagreeing_comma_list_stays_unreadable(value): + """Disagreeing members cannot frame a body — must stay refused, close armed.""" + h = _handler(value) + assert unreadable_content_length(h) is True, ( + f"Content-Length {value!r} disagrees; there is no honest length to drain, " + "so it must be refused with the connection armed for close" + ) + assert bool(request_declares_body(h)) is True, value + + +@pytest.mark.parametrize( + "value", + ["0, banana", "banana, 0", "0,", ",0", "0, ,0", "0, +0", "0, \xa00", "0, 0x5"], +) +def test_malformed_comma_members_stay_unreadable(value): + """One bad member poisons the whole field: fail closed, never treat as zero.""" + h = _handler(value) + assert unreadable_content_length(h) is True, ( + f"Content-Length {value!r} has an unparseable member and must fail closed" + ) + assert bool(request_declares_body(h)) is True, value + + +def test_repeated_headers_and_comma_list_agree(): + """The two RFC spellings of the same field must behave identically.""" + repeated = _H(_Hdrs({"Content-Length": ["0", "0"]})) + combined = _handler("0, 0") + assert unreadable_content_length(repeated) == unreadable_content_length(combined) + assert bool(request_declares_body(repeated)) == bool(request_declares_body(combined)) + + repeated_bad = _H(_Hdrs({"Content-Length": ["0", "42"]})) + combined_bad = _handler("0, 42") + assert unreadable_content_length(repeated_bad) is True + assert unreadable_content_length(combined_bad) is True diff --git a/tests/test_get_with_body_auth_reject_reuse.py b/tests/test_get_with_body_auth_reject_reuse.py new file mode 100644 index 00000000..8caac837 --- /dev/null +++ b/tests/test_get_with_body_auth_reject_reuse.py @@ -0,0 +1,158 @@ +"""A body-bearing GET that fails auth must not poison a keep-alive connection. + +The sibling files cover the WRITE paths: a rejected POST/PUT/PATCH/DELETE arms the +close through ``check_auth_or_close``. ``do_GET`` was the documented residual — it +called plain ``check_auth``, so a GET that *declares a body* and then fails auth +answered its 401/302 and left those bytes queued in ``rfile``. On a keep-alive +socket the next request is then parsed starting mid-body, and the client gets an +error about a request it never sent. + +A GET with a body is unusual but entirely legal, and it is what a proxy, a +``fetch`` with a body, or a scripted client can produce. + +Both halves of the contract are asserted here, because the naive fix (close every +auth-failed GET) would regress the body-less login-redirect flow: + +* body-bearing + auth-failed -> connection MUST close (framing unresynchronisable) +* body-less + auth-failed -> connection MUST stay alive (nothing queued) + +``test_unfixed_handler_reproduces_the_poisoning`` is the negative control: the same +socket exchange against a handler that answers without arming the close shows the +leftover body surfacing as a bogus ``400 Bad request syntax``, proving the exchange +really does exercise the bug rather than passing vacuously. +""" + +import socket +import threading +from http.server import BaseHTTPRequestHandler, ThreadingHTTPServer + +BODY = b'{"q": "search"}' + + +def _get_with_body(path=b"/api/sessions"): + return ( + b"GET " + path + b" HTTP/1.1\r\n" + b"Host: localhost\r\n" + b"Content-Type: application/json\r\n" + b"Content-Length: " + str(len(BODY)).encode() + b"\r\n" + b"\r\n" + BODY + ) + + +BODYLESS_GET = ( + b"GET /api/sessions HTTP/1.1\r\n" + b"Host: localhost\r\n" + b"\r\n" +) +FOLLOWING_GET = b"GET /api/health HTTP/1.1\r\nHost: localhost\r\n\r\n" + + +def _declares_body(handler): + """The production rule: framing declares a body, so it is queued in rfile.""" + raw = handler.headers.get("Content-Length") + if raw is not None: + stripped = raw.strip(" \t") + if not (stripped.isascii() and stripped.isdigit()): + return True # unreadable framing -> fail closed + try: + if int(stripped) > 0: + return True + except ValueError: + return True + return bool((handler.headers.get("Transfer-Encoding") or "").strip()) + + +class _AuthRejectingHandler(BaseHTTPRequestHandler): + """Mirrors do_GET's shape: reject before the body is ever read.""" + + protocol_version = "HTTP/1.1" + arm_close = True + + def do_GET(self): + if self.path == "/api/health": + payload = b'{"status": "ok"}' + self.send_response(200) + self.send_header("Content-Type", "application/json") + self.send_header("Content-Length", str(len(payload))) + self.end_headers() + self.wfile.write(payload) + return + # Auth failure BEFORE the body is read — the residual's exact shape. + if self.arm_close and _declares_body(self): + self.close_connection = True + payload = b'{"error": "unauthorized"}' + self.send_response(401) + self.send_header("Content-Type", "application/json") + self.send_header("Content-Length", str(len(payload))) + if self.arm_close and _declares_body(self): + self.send_header("Connection", "close") + self.end_headers() + self.wfile.write(payload) + + def log_message(self, *a): + pass + + +class _UnfixedHandler(_AuthRejectingHandler): + arm_close = False + + +def _exchange(handler_cls, request_bytes): + """Pipeline `request_bytes` + a follow-up GET down ONE socket; return the reply.""" + srv = ThreadingHTTPServer(("127.0.0.1", 0), handler_cls) + threading.Thread(target=srv.serve_forever, daemon=True).start() + try: + with socket.create_connection(srv.server_address, timeout=5) as s: + s.sendall(request_bytes + FOLLOWING_GET) + s.settimeout(5) + chunks = [] + while True: + try: + b = s.recv(4096) + except socket.timeout: + break + if not b: + break + chunks.append(b) + return b"".join(chunks) + finally: + srv.shutdown() + srv.server_close() + + +def test_body_bearing_get_that_fails_auth_closes_the_connection(): + raw = _exchange(_AuthRejectingHandler, _get_with_body()) + assert b"401" in raw, raw[:200] + assert b"Connection: close" in raw, f"401 did not advertise close: {raw[:200]}" + # The follow-up GET must NEVER be answered on this socket. + assert raw.count(b"HTTP/1.1") == 1, f"connection was reused: {raw[:300]}" + assert b'"status": "ok"' not in raw, f"follow-up GET was served: {raw[:300]}" + # And the leftover body must never surface as a bogus request line. + assert b"Bad request syntax" not in raw, raw[:300] + assert b"501" not in raw, raw[:300] + + +def test_bodyless_get_that_fails_auth_keeps_keep_alive(): + """The over-close half: nothing is queued, so the socket must stay usable.""" + raw = _exchange(_AuthRejectingHandler, BODYLESS_GET) + assert b"401" in raw, raw[:200] + assert b"Connection: close" not in raw, f"over-closed a bodyless reject: {raw[:200]}" + # The pipelined follow-up IS served, proving keep-alive survived. + assert b'"status": "ok"' in raw, f"keep-alive was dropped: {raw[:300]}" + assert raw.count(b"HTTP/1.1") == 2, raw[:300] + + +def test_unfixed_handler_reproduces_the_poisoning(): + """Negative control: without the arming, the leftover body corrupts the stream.""" + raw = _exchange(_UnfixedHandler, _get_with_body()) + assert b"401" in raw, raw[:200] + poisoned = ( + b"Bad request syntax" in raw + or b"400" in raw + or b"501" in raw + or b'"status": "ok"' not in raw + ) + assert poisoned, ( + "expected the unread body to poison the next request on this socket; " + f"got {raw[:300]!r}" + ) diff --git a/tests/test_issue1909_csp_report_only.py b/tests/test_issue1909_csp_report_only.py index a22be893..7ef947be 100644 --- a/tests/test_issue1909_csp_report_only.py +++ b/tests/test_issue1909_csp_report_only.py @@ -144,7 +144,7 @@ def test_server_bypasses_auth_for_csp_report(monkeypatch): called["path"] = parsed.path return True - monkeypatch.setattr("server.check_auth", fail_auth) + monkeypatch.setattr("server.check_auth_or_close", fail_auth) monkeypatch.setattr("server.clear_request_profile", lambda: None) monkeypatch.setattr("server.get_profile_cookie", lambda _handler: None) diff --git a/tests/test_production_do_get_body_pending.py b/tests/test_production_do_get_body_pending.py new file mode 100644 index 00000000..32929488 --- /dev/null +++ b/tests/test_production_do_get_body_pending.py @@ -0,0 +1,127 @@ +"""``Handler.do_GET`` itself must arm the close when a body is still queued. + +This is the production-wired companion to +``test_get_with_body_auth_reject_reuse.py``. That file proves the HTTP-level +consequence using a handler of the same shape; this one asserts the real +``server.Handler.do_GET`` routes its auth rejection through the body-pending-aware +path, so reverting ``server.py`` to plain ``check_auth`` fails a test. + +That distinction matters here: the PR's existing GET unit tests all pass with the +residual reverted, because none of them reach ``do_GET``'s auth call. +""" + +import types + +import pytest + + +def _handler_double(headers, path="/api/sessions"): + """A handler stub shaped like the one the reject paths receive.""" + h = types.SimpleNamespace() + h.path = path + h.headers = headers + h.close_connection = False + h.command = "GET" + h._headers_buffer = [] + h.responses = [] + h.sent = [] + return h + + +class _Hdrs(dict): + def get(self, k, default=None): + for key, val in self.items(): + if key.lower() == k.lower(): + return val[0] if isinstance(val, list) else val + return default + + def get_all(self, k, default=None): + out = [] + for key, val in self.items(): + if key.lower() == k.lower(): + out.extend(val if isinstance(val, list) else [val]) + return out or default + + +def test_do_get_source_uses_the_body_pending_aware_auth_call(): + """``do_GET`` must not call bare ``check_auth``. + + A source assertion is the right tool for this one: the call is a single line + inside a method whose surrounding machinery (socket, wfile, routing) would + have to be faked wholesale to reach it, and faking that is what let the + residual hide in the first place. + """ + import pathlib + + src = pathlib.Path(__file__).resolve().parents[1] / "server.py" + text = src.read_text() + + start = text.index("def do_GET") + end = text.index("def ", start + 10) + body = text[start:end] + + assert "check_auth_or_close(self, parsed)" in body, ( + "do_GET must route its auth rejection through check_auth_or_close so a " + "body-bearing GET cannot leave unread bytes on a keep-alive socket" + ) + assert "check_auth(self, parsed)" not in body, ( + "do_GET still calls bare check_auth — a body-bearing GET that fails auth " + "will poison the next request on the connection" + ) + + +def test_check_auth_or_close_arms_only_when_a_body_is_pending(): + """The over-close half, at the production helper.""" + from api.auth import check_auth_or_close + import api.auth as auth_mod + + calls = {} + + def _deny(handler, parsed): + calls["hit"] = True + return False + + real = auth_mod.check_auth + auth_mod.check_auth = _deny + try: + # Body pending -> close armed. + h = _handler_double(_Hdrs({"Content-Length": "15"})) + assert check_auth_or_close(h, None) is False + assert h.close_connection is True, "body-bearing reject must arm the close" + + # Body-less -> keep-alive preserved. + h2 = _handler_double(_Hdrs({})) + assert check_auth_or_close(h2, None) is False + assert h2.close_connection is False, ( + "a body-less reject must NOT close — that drops a healthy keep-alive" + ) + + # Unreadable framing -> fail closed. + h3 = _handler_double(_Hdrs({"Content-Length": "+0"})) + assert check_auth_or_close(h3, None) is False + assert h3.close_connection is True, ( + "unreadable Content-Length must fail closed, not be treated as zero" + ) + finally: + auth_mod.check_auth = real + + +@pytest.mark.parametrize( + "value", + ["\xa00", "0\x85", "+0", "-0", "0_0", "\u0660", "-5", ""], +) +def test_adversarial_zero_spellings_declare_a_body(value): + """Every spelling of "zero" that ``int()`` accepts must still fail closed. + + ``int()`` is far more generous than RFC 9110 ``Content-Length = 1*DIGIT``: + it tolerates unicode whitespace, a leading sign, PEP-515 underscores and + non-ASCII digits. Each of those parsed to an honest ``0`` would take the + keep-alive branch with the body still queued. + """ + from api.helpers import request_declares_body + + h = _handler_double(_Hdrs({"Content-Length": value})) + assert request_declares_body(h) is True, ( + f"Content-Length {value!r} was treated as no-body; a queued body would " + "then poison the next request on this connection" + )