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