From b8d500fcb07f756ae5d2bc4042b5593fcdc855f1 Mon Sep 17 00:00:00 2001 From: Elmehdi Aitbrahim Date: Tue, 13 Oct 2026 16:08:16 -0400 Subject: [PATCH] fix(web): refuse HTTP/0.9 before dispatch with a framed 505 (#989) A versionless request line left self.headers a plain dict, so every refusal raised AttributeError in _declared_body_length: a reset and a traceback on stderr. The explicit 'HTTP/0.9' form was admitted and answered unframed, which desynchronised a kept-alive connection. parse_request now refuses both with a status-lined 505 and closes. Co-Authored-By: Claude Opus 5.5 --- keel/web/server.py | 30 +++++++++++++ tests/web/test_server.py | 97 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 127 insertions(+) diff --git a/keel/web/server.py b/keel/web/server.py index ecb6825c..6a1be689 100644 --- a/keel/web/server.py +++ b/keel/web/server.py @@ -53,6 +53,7 @@ import time from collections.abc import Callable from dataclasses import dataclass, field, replace +from http import HTTPStatus from http.server import BaseHTTPRequestHandler, ThreadingHTTPServer from pathlib import Path from typing import Any @@ -663,6 +664,35 @@ def handle_one_request(self) -> None: if not self.close_connection and self.command in ("GET", "HEAD"): self._drain_request_body() + def parse_request(self) -> bool: + """The stdlib's parse, and then an HTTP/0.9 request is refused before any verb sees it + (#989). + + **No route here can answer 0.9 correctly.** A versionless request line (`GET /x`) gets no + header block parsed, so `self.headers` is a plain `dict` and `_declared_body_length` -- + reached by every `_refuse` -- raised on it: a reset instead of an answer, and a traceback + on the stderr `log_message` keeps quiet. The explicit form (`GET /x HTTP/0.9`) does get + headers, so it could be admitted, and was then answered in 0.9 framing: a body with no + status line and no length, which on a kept-alive connection sat in front of the next + response. Both are the same check -- the stdlib's own test for "answer without a status + line" -- so after this every request a verb sees has a parsed header block. + + **Answered, then closed.** A 505 with a status line, as the stdlib frames every other + request line it refuses, rather than a silent close: a client that sent a version this + server will not speak is told so. `request_version` is moved off 0.9 only so that the + refusal carries its status line; `send_error` marks the connection to close. No browser + sends 0.9, the console's `fetch` included, so the halt is never refused by this. + """ + if not super().parse_request(): + return False + if self.request_version != "HTTP/0.9": + return True + self.request_version = "HTTP/1.0" + self.send_error( + HTTPStatus.HTTP_VERSION_NOT_SUPPORTED, explain="keel speaks HTTP/1.0 and HTTP/1.1." + ) + return False + # -- shared admission -- def _admitted(self, *, outlives_the_lifetime: bool = False) -> bool: """Host header, then the remote session lifetime, then session cookie. `False` means a diff --git a/tests/web/test_server.py b/tests/web/test_server.py index eb8c7956..b5e77b39 100644 --- a/tests/web/test_server.py +++ b/tests/web/test_server.py @@ -2882,3 +2882,100 @@ def test_an_expired_remote_session_is_refused_everything_but_the_halt( assert status == 403, body assert json.loads(body)["error"]["title"] == "Session expired" assert _kill_switch(deployment[0]) is True + + +# -- an HTTP/0.9 request line is refused before dispatch (#989) ------------------------------------ + + +def _send_raw_and_read(cfg, request: bytes) -> bytes: + """Write `request` in one burst and return everything the server sends until it closes. + + Every wait is bounded by the socket's 5 s timeout: a server that never closes raises + `TimeoutError` here, which is a failure, never a hang. A reset is tolerated only after bytes + arrived (the server closing with ours unread is still a close); a reset with NOTHING sent back + is the #989 symptom itself and is re-raised.""" + received = b"" + with socket.create_connection((cfg.host, cfg.port), timeout=5) as sock: + sock.sendall(request) + while True: + try: + chunk = sock.recv(65536) + except ConnectionResetError: + if not received: + raise + break + if not chunk: + break + received += chunk + return received + + +def _status_line(received: bytes) -> bytes: + return received.split(b"\r\n", 1)[0] + + +def _status_lines(received: bytes) -> list[bytes]: + """Every HTTP/1.x status code in the stream. Not `_statuses`, which counts the stdlib's HTML + error page as a separate `ERR` -- and that page is the expected BODY of the 505 here.""" + return re.findall(rb"HTTP/1\.[01] (\d{3}) ", received) + + +@pytest.mark.parametrize( + "path", ["/api/config", "/", "/api/gates/halt"], ids=["api", "shell", "halt-path"] +) +def test_an_http09_request_line_is_answered_and_closed_with_nothing_on_stderr( + deployment, capfd, path +) -> None: + """**#989.** A request line with no version (`GET /api/config`) is HTTP/0.9: the stdlib parses + no header block for it and leaves `self.headers` a plain `dict`. `_refuse` then drained through + `_declared_body_length`, which raised `AttributeError` on `.defects` -- the client got a reset + with no answer, and a traceback of absolute source paths reached the stderr `log_message` + exists to keep quiet. It is refused with a STATED status line now, then closed.""" + with _serving(deployment) as cfg: + received = _send_raw_and_read(cfg, f"GET {path}\r\n".encode()) + + assert re.fullmatch(rb"HTTP/1\.[01] 505 .*", _status_line(received)), received[:300] + assert _status_lines(received) == [b"505"], received[:300] + assert capfd.readouterr().err == "" + + +def test_an_explicit_http09_request_with_a_session_is_refused_not_served_unframed( + deployment, capfd +) -> None: + """The three-word form, `GET /api/config HTTP/0.9`, DOES get its headers parsed -- so with a + valid Host and session it used to be admitted and answered with the stdlib's 0.9 framing: the + JSON body with no status line and no headers. Asked for keep-alive, the connection then stayed + open, so that unframed body sat in front of the next response on the same socket. + + It is refused before dispatch like the two-word form, and the pipelined GET behind it is never + answered: one status line, then the close.""" + with _serving(deployment) as cfg: + host = f"{cfg.host}:{cfg.port}" + request = ( + f"GET /api/config HTTP/0.9\r\nHost: {host}\r\nCookie: {_session(cfg)}\r\n" + "Connection: keep-alive\r\n\r\n" + f"GET /api/config HTTP/1.1\r\nHost: {host}\r\nCookie: {_session(cfg)}\r\n" + "Connection: close\r\n\r\n" + ).encode() + received = _send_raw_and_read(cfg, request) + + assert re.fullmatch(rb"HTTP/1\.[01] 505 .*", _status_line(received)), received[:300] + assert _status_lines(received) == [b"505"], received[:300] + assert b'"data"' not in received, received[:300] + assert capfd.readouterr().err == "" + + +@pytest.mark.parametrize("version", ["HTTP/1.0", "HTTP/1.1"]) +def test_an_honest_versioned_get_is_still_served(deployment, version) -> None: + """The #989 refusal is for HTTP/0.9 only: a 1.0 or 1.1 request with a Host and a session is + answered 200 with the config document, exactly as before.""" + with _serving(deployment) as cfg: + request = ( + f"GET /api/config {version}\r\nHost: {cfg.host}:{cfg.port}\r\n" + f"Cookie: {_session(cfg)}\r\nConnection: close\r\n\r\n" + ).encode() + received = _send_raw_and_read(cfg, request) + + assert _statuses(received) == [b"200"], received[:300] + _head, _, body = received.partition(b"\r\n\r\n") + assert json.loads(body)["data"] is not None