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 <rodrigogs@users.noreply.github.com>

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)

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

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}"
+    )

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)
 

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"
+    )