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.
This commit is contained in:
1 parent
1bd3c72e17
commit
3c5569a30e
7 files changed
+231
-24
No files matched your search
@@ -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
|
||||
@@ -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}"
|
||||
)
|
||||
@@ -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()
|
||||
|
||||
Reference in new issue
Block a user