fix(http): emit Connection: close exactly once and never crash on stubs

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.
This commit is contained in:
Rodrigo Gomes da Silva authored and n committed 2026-09-22 12:06:09 +00:00
1 parent 3c5569a30e
commit 9a181c0026
5 files changed
+185 -17

No files matched your search

@@ -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
+2 -8
View File
@@ -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()