From 15f07835769b02e612456fe3918055b2f677275b Mon Sep 17 00:00:00 2001 From: Elmehdi Aitbrahim Date: Mon, 12 Oct 2026 11:30:34 -0400 Subject: [PATCH 01/11] server: every POST ends at a request boundary; serve() keeps every config field (#955, #971) #955: do_POST drains the request body in a finally (bounded by _drain_request_body, which closes instead of reading an oversized or unreadable length), so a halt sent with a body no longer leaves bytes that poison the next request on a kept-alive connection. Server tests pin that the halt reads and validates no body. #971: serve() rebuilt ServeConfig by hand after binding and dropped external_hosts and started_at. It now uses dataclasses.replace(cfg, port=...). Tests drive the real serve(): a declared external Host is admitted, the remote session lifetime is enforced, and a release on a declared-remote deployment is refused. Co-Authored-By: Claude Opus 5.5 --- keel/web/server.py | 45 +++++--- tests/web/test_server.py | 227 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 260 insertions(+), 12 deletions(-) diff --git a/keel/web/server.py b/keel/web/server.py index 256a0e56..0272e5ef 100644 --- a/keel/web/server.py +++ b/keel/web/server.py @@ -51,7 +51,7 @@ import sys import time from collections.abc import Callable -from dataclasses import dataclass, field +from dataclasses import dataclass, field, replace from http.server import BaseHTTPRequestHandler, ThreadingHTTPServer from pathlib import Path from typing import Any @@ -395,7 +395,7 @@ class KeelHandler(BaseHTTPRequestHandler): #: Per REQUEST, not per connection, and reset at the top of each verb: a handler instance #: serves every request on a keep-alive connection, so a flag left set by one write would make #: the next refusal skip its drain and re-create the desynchronisation this exists to prevent. - #: `_drain_request_body` reads it; `_read_json_object` sets it. + #: `_drain_request_body` reads and sets it; `_read_json_object` sets it. body_consumed: bool = False # -- logging -- @@ -563,6 +563,9 @@ def _drain_request_body(self) -> None: if length > _MAX_BODY_BYTES: self.close_connection = True return + # Before the read: whatever the read does, this request's body is no longer ours to read, + # and `do_POST`'s closing drain must not wait on it a second time. + self.body_consumed = True try: self.rfile.read(length) except OSError: # pragma: no cover - a client that vanished mid-body @@ -778,8 +781,28 @@ def do_POST(self) -> None: # noqa: N802 - stdlib's naming, not ours There is no dynamic dispatch here and no getattr on a user-supplied name. A write reaches keel through exactly two tables: `keel.commands.setup.ACTIONS` -- which contains only idempotent, non-destructive, `MECHANICAL` steps and cannot contain anything else without - failing a test -- or `gates.TIER1_ACTIONS` plus the halt, behind `_gated_action`.""" + failing a test -- or `gates.TIER1_ACTIONS` plus the halt, behind `_gated_action`. + + **Every POST ends at a request boundary (#955).** A branch that answers without reading + the body -- the halt, which takes no phrase and no values -- used to leave whatever a + client sent anyway in the socket, and on a kept-alive connection the stdlib parsed it as + the next request line (`{}GET /api/pulse`, answered 501). The drain is in a `finally` + here rather than in each branch, so a branch added later cannot forget it; it is bounded + and closes instead of reading an oversized or unreadable length (`_drain_request_body`). + Drain rather than always close: the console's next read rides the same connection, and + closing on every halt would cost each one a reconnect for a body nobody needs. + + AFTER the answer, on purpose: the halt has already engaged and been reported by then, so a + client that declares a body it never sends stalls only its own connection, never the stop + -- stopping is never slower than starting (`keel/web/gates.py`).""" self.body_consumed = False + try: + self._post() + finally: + self._drain_request_body() + + def _post(self) -> None: + """`do_POST`'s routing, minus the drain that every branch shares. Read `do_POST`.""" parsed = urlsplit(self.path) if not self._admitted(): return @@ -1177,15 +1200,13 @@ def serve(cfg: ServeConfig, *, echo: Callable[[str], None] = print) -> int: return 1 bound_port = server.server_address[1] - running = ServeConfig( - host=cfg.host, - port=int(bound_port), - token=cfg.token, - db_path=cfg.db_path, - config_path=cfg.config_path, - build=cfg.build, - build_info=cfg.build_info, - ) + # `replace`, never a hand-listed constructor: the port is the ONE thing binding learns, and + # every other field must reach the handler as `serve_cmd` resolved it. The constructor this + # replaced copied seven fields by name and dropped `external_hosts` and `started_at` (#971), + # so every `--external-host` run refused its own declared Host, never expired a remote + # session, and read "no declared remote origin" in the release locality check. A field added + # to `ServeConfig` later is carried here without anyone having to remember this line. + running = replace(cfg, port=int(bound_port)) server.RequestHandlerClass.cfg = running # type: ignore[attr-defined] if not running.host_policy.is_loopback: diff --git a/tests/web/test_server.py b/tests/web/test_server.py index 1f280fc2..c747c3d4 100644 --- a/tests/web/test_server.py +++ b/tests/web/test_server.py @@ -2147,3 +2147,230 @@ def test_the_missing_gate_token_refusal_names_that_routes_own_cli_command( assert status == 403, body assert re.findall(r"`(keel [a-z-]+)`", body) == [command], body + + +# -- a halt with a body (#955) --------------------------------------------------------------------- + + +def _keepalive_halt_then_get(cfg, body: bytes) -> tuple[int, str, int, str]: + """POST the halt with `body` and a `Content-Length`, then GET down the SAME socket. + + One connection for both, because that is the only way the desynchronisation is visible: a + body left unread is parsed by the stdlib as the start of the next request line.""" + conn = http.client.HTTPConnection(cfg.host, cfg.port, timeout=10) + host = f"{cfg.host}:{cfg.port}" + try: + conn.request( + "POST", + "/api/gates/halt", + body=body, + headers={ + "Host": host, + "Cookie": _session(cfg), + "X-Keel-Client": "1", + security.HALT_HEADER: security.halt_token(cfg.token), + "Content-Type": "application/json", + "Content-Length": str(len(body)), + }, + ) + first = conn.getresponse() + first_body = first.read().decode("utf-8", "replace") + conn.request("GET", "/api/config", headers={"Host": host, "Cookie": _session(cfg)}) + second = conn.getresponse() + second_body = second.read().decode("utf-8", "replace") + return first.status, first_body, second.status, second_body + finally: + conn.close() + + +@pytest.mark.parametrize( + "body", + [b"{}", b"xy", b"this is not json at all", b'{"phrase": ["nested"]}'], + ids=["empty-object", "two-bytes", "not-json", "nested-json"], +) +def test_a_halt_with_a_body_engages_and_leaves_the_connection_clean(deployment, body) -> None: + """**#955: the halt reads no body, and a body sent anyway must not poison the connection.** + + Two pins in one drive. The halt answers 200 and engages the switch whatever the body is -- + including bodies `_read_json_object` would refuse (not JSON, a nested value), so a halt that + started parsing its body and answering 400 fails here rather than only in the client suite. + And the GET after it, on the same kept-alive socket, answers 200: before the fix the body's + bytes were read as the next request line (`{}GET /api/config`) and answered 501.""" + with _serving(deployment) as cfg: + assert _kill_switch(deployment[0]) in (None, False) + halt_status, halt_body, get_status, get_body = _keepalive_halt_then_get(cfg, body) + + assert halt_status == 200, halt_body + assert _kill_switch(deployment[0]) is True + assert get_status == 200, get_body[:200] + assert json.loads(get_body)["data"] is not None + + +def test_a_halt_with_an_oversized_body_still_engages(deployment) -> None: + """A body past `_MAX_BODY_BYTES` is not drained (that is the memory-exhaustion primitive the + cap denies) -- the connection is closed instead. The halt itself must still happen: refusing + a stop over its envelope is the one failure this route must not have.""" + oversized = b"x" * (web_server._MAX_BODY_BYTES + 1) + with _serving(deployment) as cfg: + conn = http.client.HTTPConnection(cfg.host, cfg.port, timeout=10) + try: + conn.request( + "POST", + "/api/gates/halt", + body=oversized, + headers={ + "Host": f"{cfg.host}:{cfg.port}", + "Cookie": _session(cfg), + "X-Keel-Client": "1", + security.HALT_HEADER: security.halt_token(cfg.token), + "Content-Length": str(len(oversized)), + }, + ) + response = conn.getresponse() + response.read() + finally: + conn.close() + + assert response.status == 200 + assert _kill_switch(deployment[0]) is True + + +# -- `serve()` keeps every field it was given (#971) ----------------------------------------------- + + +@contextmanager +def _through_serve(deployment, monkeypatch, home: Path, **changes: object): + """Run the REAL `serve()` in a thread; yield the config it was given and the one its handler + answers with. + + Not `_serving`, which builds the bound config itself: #971 was `serve()` rebuilding the + config after binding and dropping `external_hosts` and `started_at`, and every test that + built its own `ServeConfig` walked straight past it. The server object is captured by + wrapping `build_server` (still the real one), so the test can stop it and read the config + `serve()` installed on the handler -- the one every request is answered against.""" + monkeypatch.setenv("KEEL_HOME", str(home)) + from keel.web import runtime + + # Interactive, so `record_serving` writes nothing (this test is not about the record). + monkeypatch.setattr(runtime, "stdout_is_interactive", lambda: True) + built: list[web_server.KeelServer] = [] + real_build = web_server.build_server + + def _capture(cfg: web_server.ServeConfig) -> web_server.KeelServer: + server = real_build(cfg) + built.append(server) + return server + + monkeypatch.setattr(web_server, "build_server", _capture) + db_path, config_path = deployment + cfg = web_server.ServeConfig( + host="127.0.0.1", + port=0, + token=new_session_token(), + db_path=db_path, + config_path=config_path, + **changes, # type: ignore[arg-type] + ) + announced = threading.Event() + lines: list[str] = [] + + def _echo(line: str) -> None: + lines.append(line) + if "Press Ctrl-C" in line: + announced.set() + + exit_codes: list[int] = [] + thread = threading.Thread( + target=lambda: exit_codes.append(web_server.serve(cfg, echo=_echo)), daemon=True + ) + thread.start() + try: + assert announced.wait(timeout=10), lines + server = built[0] + yield cfg, server.RequestHandlerClass.cfg # type: ignore[attr-defined] + finally: + if built: + built[0].shutdown() + thread.join(timeout=10) + assert exit_codes == [0], lines + + +def test_serve_keeps_every_field_it_was_given(deployment, monkeypatch, tmp_path) -> None: + """Every field but `port` survives the post-bind rebuild. Compared as WHOLE dataclasses, so a + field added to `ServeConfig` later cannot be dropped here without failing this test -- and + every optional field is given a non-default value, so a field that fell back to its default + cannot pass by coincidence.""" + remote = frozenset({"keel.example.com"}) + started = time.time() - 60 + with _through_serve( + deployment, + monkeypatch, + tmp_path, + external_hosts=remote, + started_at=started, + build="keel test", + build_info=object(), + ) as (given, installed): + assert installed.port != 0, "the port is the one thing binding learns" + assert dataclasses.replace(installed, port=given.port) == given + assert installed.session_expired_at is not None + defaults = { + spec.name + for spec in dataclasses.fields(web_server.ServeConfig) + if spec.name not in {"port", "screen_cache"} + and spec.default is not dataclasses.MISSING + and getattr(given, spec.name) == spec.default + } + assert defaults == set(), "every optional field must be set off its default here" + + +def test_serve_admits_a_declared_external_host(deployment, monkeypatch, tmp_path) -> None: + """#971 item 1: `keel serve --external-host keel.example.com` refused a request whose Host was + keel.example.com, because the declaration never reached the handler.""" + with _through_serve( + deployment, monkeypatch, tmp_path, external_hosts=frozenset({"keel.example.com"}) + ) as (_given, installed): + status, _headers, body = _request( + installed, "/api/config", cookie=_session(installed), host="keel.example.com" + ) + + assert status == 200, body + + +def test_serve_enforces_the_remote_session_lifetime(deployment, monkeypatch, tmp_path) -> None: + """#971 item 2: `started_at` was dropped, so `session_expired_at` was always `None` and the + #648 lifetime never bit. A valid token on a run older than the lifetime must be refused.""" + stale = time.time() - security.REMOTE_SESSION_MAX_AGE_SECONDS - 1 + with _through_serve( + deployment, + monkeypatch, + tmp_path, + external_hosts=frozenset({"keel.example.com"}), + started_at=stale, + ) as (_given, installed): + status, _headers, body = _request(installed, "/api/config", cookie=_session(installed)) + + assert status == 403, body + assert "lifetime" in body + + +def test_serve_refuses_a_release_on_a_declared_remote_deployment( + deployment, monkeypatch, tmp_path +) -> None: + """#971 item 3: the release locality check read an empty declaration, so a tunnel that + rewrote Host to loopback passed it. With the declaration kept, a release from loopback -- + which is what a tunnel looks like -- is refused before the gates token is even read.""" + with _through_serve( + deployment, monkeypatch, tmp_path, external_hosts=frozenset({"keel.example.com"}) + ) as (_given, installed): + status, _headers, body = _request( + installed, + "/api/gates/resume", + method="POST", + cookie=_session(installed), + gate=security.gates_token(installed.token), + raw_body=json.dumps({"phrase": "RESUME TRADING"}), + ) + + assert status == 403, body + assert "loopback session on a loopback bind" in body From a3356fe4fa420e2eb547a5d9c11636c0c260e494 Mon Sep 17 00:00:00 2001 From: Elmehdi Aitbrahim Date: Mon, 12 Oct 2026 11:48:58 -0400 Subject: [PATCH 02/11] console: HALT stays reachable when the Pulse report fails to build (#958) A failed /api/pulse (500, or a refusal from its read) now carries halt: {halt_token, resume_command} on the error envelope. The halt token is security.halt_token (never the gates token), and the kill switch is read on its own through pulse.halt_resume_command, apart from gather_pulse. A switch that cannot be read shows the button, never the HALTED line. Every other failure, and every pre-admission refusal, carries halt: null, so the key set stays constant. Client: readingFrom forwards halt off a failure document. When Pulse fails and carries it, paint hands it to rememberHaltToken and renders pulseFailedView, which is the same haltControl plus the error. No extra confirm and no phrase. Co-Authored-By: Claude Opus 5.5 --- keel/commands/pulse.py | 13 +++ keel/web/api.py | 40 ++++++++- keel/web/payload.py | 22 ++++- keel/web/static/js/api.js | 28 ++++++- keel/web/static/js/live.js | 1 + keel/web/static/js/main.js | 9 ++ keel/web/static/js/render.js | 27 ++++++ tests/web/test_api.py | 119 +++++++++++++++++++++++++++ tests/web/test_attestation_banner.py | 4 +- tests/web/test_pulse_view.py | 74 +++++++++++++++++ 10 files changed, 331 insertions(+), 6 deletions(-) diff --git a/keel/commands/pulse.py b/keel/commands/pulse.py index 619c3e11..20413251 100644 --- a/keel/commands/pulse.py +++ b/keel/commands/pulse.py @@ -263,6 +263,19 @@ def build_pulse( ) +def halt_resume_command(repo: Repository) -> str: + """`RESUME_COMMAND` when the kill switch reads engaged, else `""` -- the halt control's one + input besides its token, read ALONE (#958). + + The Pulse report reads the same fact through `gather_status`, and when any part of that + report fails the whole report fails with it. The HALT button must not: stopping is never + slower than starting (`keel/web/gates.py`). So the web layer reads this on the failure path, + where nothing else on the ledger is trusted to answer. The same reading as + `gather_status`'s, `default=True` included -- an unset switch is ENGAGED, as the guards treat + it.""" + return RESUME_COMMAND if bool(repo.get_state("kill_switch", default=True)) else "" + + def gather_pulse( repo: Repository, config: Config, feed: ActivityFeed, *, now_ts: int ) -> PulseReport: diff --git a/keel/web/api.py b/keel/web/api.py index 9ff2d739..2db175e1 100644 --- a/keel/web/api.py +++ b/keel/web/api.py @@ -330,6 +330,30 @@ def read_pulse(cfg: ServeConfig, _query: Query, _state: Any, now_ts: int) -> dic return payload.pulse_payload(report, halt_token=halt_token(cfg.token)) +def pulse_halt(cfg: ServeConfig) -> dict[str, str]: + """The HALT control's inputs when the Pulse report could not be built (#958). + + `halt_token` is minted from the session, so it cannot fail. The kill-switch reading is the one + ledger read here, through its own repo open and `pulse.halt_resume_command` -- never through + `gather_pulse`, whose failure is why this exists. **A switch that cannot be read reads as not + halted**, which shows the button: halting a halted engine is harmless and idempotent, while a + HALTED line over a running engine would hide the only stop the page has. Never `gates_token` + (#968), on this path as on the success one.""" + from keel.commands.pulse import halt_resume_command + + resume = "" + repo = None + try: + repo = open_repo(cfg.db_path) + resume = halt_resume_command(repo) + except Exception: # noqa: BLE001 - an unreadable switch must still answer with the button + resume = "" + finally: + if repo is not None: + close_repo(repo) + return payload.halt_fallback(halt_token=halt_token(cfg.token), resume_command=resume) + + def read_orders(cfg: ServeConfig, query: Query, _state: Any, now_ts: int) -> dict[str, Any]: """The audit trail: what keel actually bought and sold, and at what price (#659). @@ -882,6 +906,10 @@ class ApiRoute: needs_database: bool = True collection: str = "" sortable: tuple[str, ...] = field(default=()) + #: Whether a failed read still answers with the HALT control's inputs (#958). `/api/pulse` + #: only: it is the view that halts, and `pulse_halt` reads what the button needs apart from + #: the report, so a report that raises does not take the stop with it. + halt_on_failure: bool = False #: THE READ SURFACE, in full. Every one of the rendered routes in `server.ROUTES` appears here as @@ -895,6 +923,7 @@ class ApiRoute: "/api/pulse": ApiRoute( html_route="/pulse", read=read_pulse, + halt_on_failure=True, # No sortable collection: every table on the glance page is an intrinsic order -- # `rule_counts` by status, `data_freshness` in the config's product order, `live_rules` # in the ledger's, cycles newest first -- and a display sort would destroy each rather @@ -1197,18 +1226,27 @@ def respond(cfg: ServeConfig, path: str, query: Query) -> tuple[int, dict[str, A data = route.read(cfg, query, state, now_ts) except ApiRefusal as refusal: return refusal.status, payload.error_envelope( - now_ts, status=refusal.status, title=refusal.title, detail=refusal.detail + now_ts, + status=refusal.status, + title=refusal.title, + detail=refusal.detail, + halt=pulse_halt(cfg) if route.halt_on_failure else None, ) except Exception as exc: # A broken report is a stated failure, never a 200 with an empty payload -- that shape is # precisely the "blank view, or worse, stale figures" this envelope exists to prevent. The # detail carries the exception's TYPE and message and no traceback: a traceback in a # browser is a stack of file paths from someone else's machine. + # + # Pulse's failure still carries the HALT control's inputs (#958): the report failing is + # not a reason for the stop to disappear. Reached only after `_admitted`, like the + # success document that carries the same token. return 500, payload.error_envelope( now_ts, status=500, title="That report could not be built", detail=f"{type(exc).__name__}: {exc}", + halt=pulse_halt(cfg) if route.halt_on_failure else None, ) if column: diff --git a/keel/web/payload.py b/keel/web/payload.py index 5c1d40b6..4094b1c3 100644 --- a/keel/web/payload.py +++ b/keel/web/payload.py @@ -2490,7 +2490,12 @@ def envelope( def error_envelope( - now_ts: float | int | None, *, status: int, title: str, detail: str + now_ts: float | int | None, + *, + status: int, + title: str, + detail: str, + halt: dict[str, str] | None = None, ) -> dict[str, Any]: """One `GET /api/*` refusal or failure. @@ -2510,15 +2515,30 @@ def error_envelope( is always `null`: most refusals happen before the session cookie is checked, so filling it in would mean an unauthenticated request reading the deployment database -- the same argument that keeps `engine` out of this document. + + `halt` (#958) is `null` on every failure but one: Pulse's own, after admission, where it is + `halt_fallback`'s `{halt_token, resume_command}`. The HALT button lives on Pulse, and a stop + that vanished whenever the report beside it failed to build would make stopping slower than + starting. Present and `null` everywhere else for `attestations`' reason -- a constant key set. """ return { "as_of": iso(now_ts), "data": None, "attestations": None, "error": {"status": str(status), "title": title, "detail": detail}, + "halt": halt, } +def halt_fallback(*, halt_token: str, resume_command: str) -> dict[str, str]: + """What the HALT control needs when the Pulse report could not be built (#958): the halt + token and the resume command, the same two values `pulse_payload` carries as `halt_token` + and `halt.resume_command`, and nothing else. Bare strings, under the same names, so the + client hands them to the same two readers (`actions.rememberHaltToken`, `render.haltControl`) + rather than a second copy of either.""" + return {"halt_token": str(halt_token), "resume_command": str(resume_command)} + + # -- ordering (#534) ----------------------------------------------------------------------------- # # Server-side sort, ordered with `Decimal`. The spec's own words: "On loopback the round trip is diff --git a/keel/web/static/js/api.js b/keel/web/static/js/api.js index 2f416f2c..a7188f87 100644 --- a/keel/web/static/js/api.js +++ b/keel/web/static/js/api.js @@ -84,6 +84,11 @@ * `undefined`, which every consumer treats as * "nothing to show" rather than as a mistake. That * is exactly how the banner shipped dead. + * @property {any} halt `{halt_token, resume_command}` on a failed `/api/pulse`, else + * `null` (#958): the HALT control's inputs, delivered apart from + * the report so a report that failed to build keeps the stop. + * Read off a FAILURE document only -- a built Pulse carries the + * same values in its `data`. */ /** @@ -160,7 +165,17 @@ export function readingFrom(document_, ok, status) { const error = /** @type {ApiError} */ ( document_.error || { status: status, title: "Refused", detail: "" } ); - return { as_of: as_of, engine: stopped(error.title), data: null, error: error, sort: null, attestations: null }; + return { + as_of: as_of, + engine: stopped(error.title), + data: null, + error: error, + sort: null, + attestations: null, + // `?? null` like `data` below: a refusal or an older server sends no usable `halt`, and + // `undefined` here would be a key the page cannot tell from "nothing to offer". + halt: document_.halt ?? null, + }; } // Cases 1 and 2. `engine` is validated rather than trusted: this page is served from the same @@ -180,6 +195,7 @@ export function readingFrom(document_, ok, status) { error: null, sort: document_.sort ?? null, attestations: document_.attestations ?? null, + halt: null, }; } @@ -229,6 +245,7 @@ export async function read(endpoint, params) { }, sort: null, attestations: null, + halt: null, }; } @@ -247,6 +264,7 @@ export async function read(endpoint, params) { error: { status: String(response.status), title: "Unreadable answer", detail: String(cause) }, sort: null, attestations: null, + halt: null, }; } @@ -333,8 +351,10 @@ export async function runHalt(haltToken) { * * **A write whose route reads no body sends none.** The halt route never reads one, and on a * kept-alive connection bytes a handler leaves unread are parsed as the start of the NEXT request: - * the PR1 browser check saw the repaint's `GET /api/pulse` arrive as `{}GET` and refused 501. So - * `body` is optional, and the JSON content type travels only with a body. + * the PR1 browser check saw the repaint's `GET /api/pulse` arrive as `{}GET` and refused 501. The + * server now drains such bytes itself (#955, `server.do_POST`), and this still sends none: a body + * nobody reads is bytes on the wire for nothing. So `body` is optional, and the JSON content type + * travels only with a body. * * @param {URL} url * @param {Record} extraHeaders the token header for this write. @@ -364,6 +384,7 @@ async function post(url, extraHeaders, body) { error: { status: "0", title: "No answer", detail: String(cause) }, sort: null, attestations: null, + halt: null, }; } @@ -378,6 +399,7 @@ async function post(url, extraHeaders, body) { error: { status: String(response.status), title: "Unreadable answer", detail: String(cause) }, sort: null, attestations: null, + halt: null, }; } diff --git a/keel/web/static/js/live.js b/keel/web/static/js/live.js index 094b33bc..82bdaeb1 100644 --- a/keel/web/static/js/live.js +++ b/keel/web/static/js/live.js @@ -143,6 +143,7 @@ export function subscribe(onReading) { error: null, sort: null, attestations: null, + halt: null, }, "", ); diff --git a/keel/web/static/js/main.js b/keel/web/static/js/main.js index c99a9aac..7c58c65d 100644 --- a/keel/web/static/js/main.js +++ b/keel/web/static/js/main.js @@ -47,6 +47,7 @@ import { paperBanner, consoleSwitcher, sessionChip, + pulseFailedView, pulseView, refusedView, rulesView, @@ -640,6 +641,14 @@ async function paint(route, rebuild, force) { // keel printed is what admits this browser, and pasting it is the action either one needs. if (primary.error && primary.error.status === REFUSED_STATUS) { rebuildInto(refusedView(primary, reconnect), Boolean(force)); + } else if (route.name === "pulse" && primary.halt) { + // #958: Pulse's report failed to build, and the failure still carries the HALT control's + // inputs. The stop is offered anyway -- stopping is never slower than starting + // (`keel/web/gates.py`) -- through the same token holder `mount` uses and the same + // control `pulseView` draws. Not the stopped view: keel is running, and that view + // offers no stop. + rememberHaltToken(primary.halt); + rebuildInto(pulseFailedView(primary), Boolean(force)); } else { rebuildInto(stoppedView(primary, pathFor(SETUP_ROUTE)), Boolean(force)); } diff --git a/keel/web/static/js/render.js b/keel/web/static/js/render.js index 66a86bbb..d1ae96dc 100644 --- a/keel/web/static/js/render.js +++ b/keel/web/static/js/render.js @@ -625,6 +625,33 @@ export function refusedView(reading, onReconnect) { return fragment; } +/** + * Pulse when its report could not be built (#958): the halt control, then why nothing else is here. + * + * `reading.halt` is the failure document's `{halt_token, resume_command}`, read apart from the + * report on the server (`api.pulse_halt`), so a gather step that raised does not take the stop + * with it -- `keel/web/gates.py`'s rule that stopping must never be slower than starting. The + * control is `haltControl` itself, handed `reading.halt` in the shape it reads, so the button, + * the confirm `main.js` puts in front of it and the HALTED line are the working page's, unchanged. + * None of the report's sections are drawn: they have nothing to render, and a zero here would be + * a figure nobody read. + * + * @param {import("./api.js").Reading} reading a failed `/api/pulse` that carried `halt`. + * @returns {DocumentFragment} + */ +export function pulseFailedView(reading) { + const fragment = document.createDocumentFragment(); + fragment.append(el("h1", undefined, "Pulse")); + fragment.append(haltControl({ halt: reading.halt })); + const card = el("div", "card stopped"); + card.append(el("p", undefined, reading.error ? reading.error.title : "")); + if (reading.error && reading.error.detail) { + card.append(el("p", "detail", reading.error.detail)); + } + fragment.append(card); + return fragment; +} + /** * Pulse, the home view: `/api/pulse`'s payload -- what keel did today, what stopped it, what it * will do next, and the one control that can stop it (console PR1). diff --git a/tests/web/test_api.py b/tests/web/test_api.py index 31ded8ef..60aaf5d2 100644 --- a/tests/web/test_api.py +++ b/tests/web/test_api.py @@ -47,6 +47,7 @@ from keel.web import payload from keel.web import server as web_server from keel.web.security import ( + HALT_HEADER, SESSION_COOKIE, csrf_token, gates_token, @@ -1045,6 +1046,124 @@ def _raise(*_args: Any, **_kwargs: Any) -> Any: raise RuntimeError("the ledger is on fire") +# -- HALT survives a Pulse that cannot be built (#958) --------------------------------------------- + + +def _set_kill_switch(db_path: str, engaged: bool) -> None: + from keel.data.repository import Repository + + conn = connect(db_path) + try: + Repository(conn).set_state("kill_switch", engaged) + finally: + conn.close() + + +def _post_halt(cfg: web_server.ServeConfig, token: str) -> int: + conn = http.client.HTTPConnection(cfg.host, cfg.port, timeout=10) + try: + conn.request( + "POST", + "/api/gates/halt", + headers={ + "Host": f"{cfg.host}:{cfg.port}", + "Cookie": _session(cfg), + "X-Keel-Client": "1", + HALT_HEADER: token, + }, + ) + response = conn.getresponse() + response.read() + return response.status + finally: + conn.close() + + +@pytest.mark.parametrize( + ("engaged", "resume"), [(False, ""), (True, "keel resume")], ids=["running", "halted"] +) +def test_a_pulse_that_cannot_be_built_still_carries_the_halt( + running: web_server.ServeConfig, + deployment: tuple[str, str], + monkeypatch: pytest.MonkeyPatch, + engaged: bool, + resume: str, +) -> None: + """**#958: stopping must never be slower than starting, and that includes a broken report.** + + Pulse is where the HALT button lives, and the button needs the halt token and whether the + kill switch is engaged. Neither may depend on the rest of the report: with `gather_pulse` + raising, the 500 still carries `halt` -- the token, and the resume command exactly when the + switch reads engaged (so the client shows the HALTED line instead of the button). The served + token then halts, which is the point of serving it.""" + _set_kill_switch(deployment[0], engaged) + monkeypatch.setattr("keel.commands.pulse.gather_pulse", _raise, raising=True) + + status, _headers, document = _json(running, "/api/pulse") + + assert status == 500 + assert document["data"] is None, "a failed report still renders no figures" + assert "RuntimeError" in document["error"]["detail"] + assert document["halt"] == { + "halt_token": halt_token(running.token), + "resume_command": resume, + } + served = json.dumps(document) + for other in (gates_token(running.token), csrf_token(running.token)): + assert other not in served + assert _post_halt(running, document["halt"]["halt_token"]) == 200 + + +def test_an_unreadable_kill_switch_offers_the_button( + running: web_server.ServeConfig, monkeypatch: pytest.MonkeyPatch +) -> None: + """When the report failed because the ledger itself is unreadable, the switch may be too. + Unknown reads as NOT halted -- the button, never the HALTED line: pressing HALT on a halted + engine is harmless, while a HALTED line over a running one would hide the only stop.""" + from keel.commands import pulse as pulse_mod + + monkeypatch.setattr(pulse_mod, "gather_pulse", _raise, raising=True) + monkeypatch.setattr(pulse_mod, "halt_resume_command", _raise, raising=True) + + status, _headers, document = _json(running, "/api/pulse") + + assert status == 500 + assert document["halt"] == {"halt_token": halt_token(running.token), "resume_command": ""} + + +def test_only_pulse_carries_the_halt_on_a_failure( + running: web_server.ServeConfig, monkeypatch: pytest.MonkeyPatch +) -> None: + """Every route's read made to raise: Pulse's 500 carries `halt`, every other 500 carries it as + `null` -- the key set of a failure is constant, and the token rides the one view that halts, + on failure as on success (`test_pulse_carries_the_halt_token_and_only_pulse_does`).""" + import dataclasses + + for path, route in list(web_api.API_ROUTES.items()): + monkeypatch.setitem(web_api.API_ROUTES, path, dataclasses.replace(route, read=_raise)) + + carriers = [] + for path in web_api.API_ROUTES: + status, _headers, document = _json(running, path) + assert status == 500, path + assert "halt" in document, path + if document["halt"] is not None: + carriers.append(path) + assert carriers == ["/api/pulse"] + assert len(web_api.API_ROUTES) >= 10, "the sweep must cover the table, not an empty one" + + +def test_a_refused_pulse_carries_no_halt(running: web_server.ServeConfig) -> None: + """Before admission there is no session, so there is nothing to hand a halt token to: the + pre-admission refusal carries `halt: null` like every other refusal.""" + status, _headers, body = _get(running, "/api/pulse") + + assert status == 403 + document = json.loads(body) + assert document["halt"] is None + assert halt_token(running.token) not in body + + # -- headers and caching --------------------------------------------------------------------------- diff --git a/tests/web/test_attestation_banner.py b/tests/web/test_attestation_banner.py index 50e1bf18..e0a5170e 100644 --- a/tests/web/test_attestation_banner.py +++ b/tests/web/test_attestation_banner.py @@ -420,7 +420,9 @@ def test_every_reading_the_client_mints_carries_the_whole_envelope() -> None: # A literal reading `attestations: null` on the SUCCESS path carries the key and still # discards every alert the server sent -- the original defect exactly, one line further on. # Whatever the success literal does for `data` and `sort`, it must do for this. - (success,) = [literal for literal in literals if "document_." in literal] + # Picked by `document_.data`: since #958 the FAILURE literal forwards one key off its + # document too (`halt`), so "reads `document_` at all" no longer singles out the success. + (success,) = [literal for literal in literals if "document_.data" in literal] forwarded = set(re.findall(r"([a-z_]+): document_\.", success)) assert envelope_keys - {"engine"} <= forwarded | {"as_of"}, ( f"the success reading invents rather than forwards: {sorted(envelope_keys - forwarded)}" diff --git a/tests/web/test_pulse_view.py b/tests/web/test_pulse_view.py index a97052d7..0de0bc75 100644 --- a/tests/web/test_pulse_view.py +++ b/tests/web/test_pulse_view.py @@ -252,3 +252,77 @@ def test_the_manifest_starts_at_pulse() -> None: assert manifest["start_url"] == "/pulse" assert manifest["id"] == "/" + + +# -- HALT survives a Pulse that cannot be built (#958) --------------------------------------------- + + +def test_every_reading_carries_the_halt_key_and_only_a_failure_reads_it() -> None: + """A `Reading` is the client's whole picture of one answer, so the failure document's `halt` + must survive `readingFrom` -- a key dropped there is `undefined`, which the page treats as + "no halt to offer", and a broken Pulse would hide the stop again. Every Reading literal in + every module carries the key (paired with `attestations`, which every literal has), and the + only one that READS it off a document is `readingFrom`'s failure arm.""" + + def keys(name: str, key: str) -> int: + # A Reading key sits on a line of its own; `{ halt: reading.halt }` inline is an argument. + return len(re.findall(rf"^\s+{key}: ", _comments_stripped(_source(name)), re.M)) + + literals = {name: keys(name, "attestations") for name in _MODULES} + halts = {name: keys(name, "halt") for name in _MODULES} + assert sum(literals.values()) >= 7, "the scan found no Reading literals; it has stopped working" + assert halts == literals + + api = _comments_stripped(_source("api.js")) + reading_from = _function_body(_source("api.js"), "readingFrom") + failure_arm = reading_from[reading_from.index("if (!ok) {") : reading_from.index("\n }\n")] + assert api.count("document_.halt") == 1 + assert failure_arm.count("halt: document_.halt ?? null") == 1 + + +def _paint() -> str: + code = _comments_stripped(_source("main.js")) + start = code.index("async function paint(") + return code[start : code.index("\n}\n", start)] + + +def test_a_failed_pulse_paints_the_halt_before_the_stopped_view() -> None: + """In `paint`'s `data: null` branch, a Pulse failure that carries `halt` gets its own view -- + after the 403 branch (a refusal carries `halt: null` and needs the reconnect field), before + the stopped view (which offers no stop). The halt token is taken off that failure exactly as + `mount` takes it off a built report: through `rememberHaltToken`, the one holder.""" + paint = _paint() + start = paint.index("if (primary.data === null) {") + null_branch = paint[start : paint.index("} else {\n rebuildInto(mount(", start)] + + refused = null_branch.index("primary.error.status === REFUSED_STATUS") + failed = null_branch.index('} else if (route.name === "pulse" && primary.halt) {') + stopped = null_branch.index("stoppedView(primary") + assert refused < failed < stopped + arm = null_branch[failed:stopped] + assert arm.count("rememberHaltToken(primary.halt);") == 1 + assert arm.count("rebuildInto(pulseFailedView(primary), Boolean(force));") == 1 + assert arm.index("rememberHaltToken(") < arm.index("pulseFailedView(") + + main = _code_only(_source("main.js")) + assert len(re.findall(r"\brememberHaltToken\(", main)) == 2, "mount + the failure arm, no third" + + +def test_the_failed_pulse_view_is_the_halt_control_and_the_error_only() -> None: + """`pulseFailedView` reuses `haltControl` -- the SAME form, button and HALTED line, so the + stop on a broken page is the stop on a working one -- with the failure's own `halt` in the + shape `haltControl` reads. HALT first, then why the rest is missing; none of the report's + sections, which have nothing to render. No extra confirm and no phrase: stopping is never + slower than starting, and the one confirm lives in `main.js`.""" + body = _function_body(_source("render.js"), "pulseFailedView") + + assert body.count("haltControl(") == 1 + assert re.findall(r"haltControl\(([^)]*)\)", body) == ["{ halt: reading.halt }"] + assert body.count('el("h1"') == 1 + assert body.index("haltControl(") < body.index("reading.error") + inside = body[body.index("{") :] + assert not re.search(r"\bpulse[A-Z]\w*\(", inside), "no report section on a failed report" + assert "confirm" not in body + render = _code_only(_source("render.js")) + assert len(re.findall(r"\bhaltControl\(", render)) == 3, "declaration + pulseView + failed" + assert "export function pulseFailedView(" in _comments_stripped(_source("render.js")) From 158780c6e85321e80ccc146338b9177fa6583a22 Mon Sep 17 00:00:00 2001 From: Elmehdi Aitbrahim Date: Mon, 12 Oct 2026 12:06:06 -0400 Subject: [PATCH 03/11] console: reads open a sidecar-less WAL database and still cannot write (#976) setup.inspect opened the database mode=ro. A WAL file whose -wal/-shm are gone (a checkpointing last close, any copied backup) cannot be opened mode=ro, so a running deployment read as "keel isn't running here" and the stopped view hid HALT. New keel.data.db.connect_read_only: mode=rw (creates no database file) plus PRAGMA query_only = ON, so SQLite refuses every write on the connection. Every console read goes through it: api.open_repo (previously a read-WRITE connect), setup.inspect, setup._sleeve_sell_kind, venue_readiness._read_only_trade_scope. The halt and the gated releases keep a writable opener, api.open_repo_to_write. Tests on a scratch WAL copy with its sidecars removed: inspect, Pulse (+ HALT 200), the promote lookup and venue scope all read it; a row, schema and header write through open_repo are refused; every connection opened by a sweep of all JSON read routes is query_only; the halt's own connection is not. Co-Authored-By: Claude Opus 5.5 --- keel/commands/setup.py | 8 +- keel/data/db.py | 29 ++ keel/venue_readiness.py | 22 +- keel/web/api.py | 18 +- keel/web/gates.py | 4 +- tests/web/test_console_reads_are_read_only.py | 258 ++++++++++++++++++ 6 files changed, 323 insertions(+), 16 deletions(-) create mode 100644 tests/web/test_console_reads_are_read_only.py diff --git a/keel/commands/setup.py b/keel/commands/setup.py index c88a8df3..bd883fda 100644 --- a/keel/commands/setup.py +++ b/keel/commands/setup.py @@ -46,6 +46,7 @@ import click from keel.commands._common import default_config_path, default_db_path +from keel.data.db import connect_read_only #: The exact schema names this module reads. Named constants rather than string literals for a #: reason that is not style: I wrote `"subscriptions"` and `"state"` here first, and both are @@ -446,7 +447,10 @@ def inspect(config_path: str | Path, db_path: str | Path) -> DeploymentState: conn: sqlite3.Connection | None = None if db_file.exists(): try: - conn = sqlite3.connect(f"file:{db_file}?mode=ro", uri=True) + # `connect_read_only`, not `mode=ro` (#976): a WAL file whose sidecars are gone + # cannot be opened `mode=ro`, and this probe then reported a running deployment as + # having no usable database -- the console's stopped view, which has no HALT. + conn = connect_read_only(db_file) # `Repository` reads rows by COLUMN NAME (`row["value"]`, `row["state"]`, ...), which # `_scope_observation`/`_venue_interest_observation` now call straight into for the # same reason `_withdrawals_observation` stayed on raw SQL: one place to decode a @@ -1297,7 +1301,7 @@ def _sleeve_sell_kind(db_path: Path, rule_id: int) -> str | None: from keel.data.repository import Repository try: - conn = sqlite3.connect(f"file:{db_path}?mode=ro", uri=True) + conn = connect_read_only(db_path) # not `mode=ro`: see `inspect` (#976) except sqlite3.Error as exc: raise _UnreadableRule from exc conn.row_factory = sqlite3.Row diff --git a/keel/data/db.py b/keel/data/db.py index de08c247..16dc7914 100644 --- a/keel/data/db.py +++ b/keel/data/db.py @@ -1213,6 +1213,35 @@ def connect(path: str | Path = "keel.db") -> sqlite3.Connection: return conn +def connect_read_only(path: str | Path) -> sqlite3.Connection: + """Open an EXISTING database to read it, never to write it (#976). + + **Why not `mode=ro`.** A WAL database's `-wal`/`-shm` sidecars can be gone while the file is + perfectly healthy -- a checkpointing last close removes them, and every copied backup lacks + them. Opening such a file needs the `-shm` re-created, which `mode=ro` forbids, so the open + fails with "unable to open database file". The console read that as "keel isn't running + here" on a running deployment, and its stopped view has no HALT. + + So the file is opened `mode=rw` -- which may create the sidecars, and which, unlike a plain + `sqlite3.connect`, refuses to CREATE a missing database -- and `PRAGMA query_only = ON` is + set before anything is read. That makes SQLite itself refuse every write on this connection: + rows, schema, and header pragmas alike. Read-only holds at the engine, not by call discipline. + + Unlike `connect`, this sets no `journal_mode`: changing the file's journal is a write, and a + reader has no business deciding it. + """ + uri = Path(path).absolute().as_uri() + "?mode=rw" + conn = sqlite3.connect(uri, uri=True) + try: + conn.execute("PRAGMA query_only = ON") + conn.execute(f"PRAGMA busy_timeout = {BUSY_TIMEOUT_MS}") + except BaseException: + conn.close() + raise + conn.row_factory = sqlite3.Row + return conn + + def table_present(conn: sqlite3.Connection, name: str) -> bool: """Does `name` exist on this database? diff --git a/keel/venue_readiness.py b/keel/venue_readiness.py index ca711a54..b1e45d83 100644 --- a/keel/venue_readiness.py +++ b/keel/venue_readiness.py @@ -65,7 +65,6 @@ from __future__ import annotations -import sqlite3 from collections.abc import Mapping from dataclasses import dataclass from enum import Enum @@ -74,6 +73,8 @@ from keel_core.trade_scope import READ_ONLY, TradeScopeState, VenueTradeScope +from keel.data.db import connect_read_only + class VenueReadiness(str, Enum): """The five states, in the order `venue_readiness` checks them.""" @@ -322,9 +323,11 @@ def _read_only_trade_scope(db_path: str | None, venue: str) -> tuple[VenueTradeS an empty database that later commands would then find and believe in. `keel brokers list` is a read-only informational command and must litter nothing. - So this uses the `mode=ro` URI shape #610 promoted to a seam for exactly this hazard - (`keel/commands/_common.py::_open_repo_ro`), with the same explicit existence check in front - of it, because `mode=ro`'s own refusal is an `OperationalError` and this path wants an + So this uses `keel.data.db.connect_read_only` (#976): `mode=rw`, which creates no database + file, plus `PRAGMA query_only = ON`, which refuses every write. It replaced the `mode=ro` + URI shape (#610's seam, `keel/commands/_common.py::_open_repo_ro`), which cannot open a WAL + database whose `-shm` sidecar is absent and so reported a healthy `/api/venues` database as + unreadable. The explicit existence check stays in front of it, because this path wants an ANSWER, not an exception. Where it deliberately differs from that seam: `_open_repo_ro` REFUSES a database stamped @@ -343,15 +346,14 @@ def _read_only_trade_scope(db_path: str | None, venue: str) -> tuple[VenueTradeS try: from keel.data.repository import Repository - conn = sqlite3.connect(f"file:{db_path}?mode=ro", uri=True) - conn.row_factory = sqlite3.Row + conn = connect_read_only(db_path) return Repository(conn).get_venue_trade_scope(venue), False except Exception: # A file IS there and this process could not read it: a schema older than #233's - # migration, a permissions problem, or -- the case that motivated separating these two - # answers -- a WAL database whose `-shm` sidecar is absent, which `mode=ro` cannot open - # because SQLite would have to CREATE that file. Every copied or restored backup has - # that shape. Reported as unknown, never as "nothing is attested". + # migration, a permissions problem, a locked or damaged file. (A WAL database whose + # `-shm` sidecar is absent was the case that motivated separating these two answers; + # since #976 it is read, not reported.) Reported as unknown, never as "nothing is + # attested". return None, True finally: if conn is not None: diff --git a/keel/web/api.py b/keel/web/api.py index 2db175e1..c4ceea21 100644 --- a/keel/web/api.py +++ b/keel/web/api.py @@ -65,14 +65,28 @@ def open_repo(db_path: str) -> Any: - """A plain connection -- deliberately WITHOUT `migrate`. + """A READ-ONLY connection (`db.connect_read_only`) -- deliberately WITHOUT `migrate`. Every CLI command migrates on the way in, which is right for a command: it runs once, and a schema behind the code is a thing to fix rather than to fail on. It is wrong here. These pages auto-reload every 15 seconds, so migrating per request would have a view that calls itself read-only take a write lock on the deployment database four times a minute -- against a database the agent may be mid-cycle on. `server.ensure_schema` does it ONCE, at bind time, - before anything is served.""" + before anything is served. + + `query_only` since #976: every reader in this package goes through here, so SQLite itself + refuses a write through any of them. The halt is the one write, and it opens its own + connection (`open_repo_to_write`).""" + from keel.data.db import connect_read_only + from keel.data.repository import Repository + + return Repository(connect_read_only(db_path)) + + +def open_repo_to_write(db_path: str) -> Any: + """The read-write connection for `gates._perform` -- the halt and the gated releases -- and + nothing else. Not `query_only`, so not for a reader: a page that read through this could write + the trading database. Like `open_repo`, it does not migrate.""" from keel.data.db import connect from keel.data.repository import Repository diff --git a/keel/web/gates.py b/keel/web/gates.py index ba39dd7e..d3e48dc3 100644 --- a/keel/web/gates.py +++ b/keel/web/gates.py @@ -235,9 +235,9 @@ def _perform(cfg: Any, operation: Callable[[Repository], None], done: str) -> st A `finally`, because every reader in `keel/web/` closes in one and a writer that leaked a connection would hold a write lock on the deployment database until refcounting noticed. """ - from keel.web.api import close_repo, open_repo + from keel.web.api import close_repo, open_repo_to_write - repo = open_repo(cfg.db_path) + repo = open_repo_to_write(cfg.db_path) try: operation(repo) finally: diff --git a/tests/web/test_console_reads_are_read_only.py b/tests/web/test_console_reads_are_read_only.py new file mode 100644 index 00000000..dbd35e82 --- /dev/null +++ b/tests/web/test_console_reads_are_read_only.py @@ -0,0 +1,258 @@ +"""The console's reads open a WAL database whose sidecars are gone, and still cannot write (#976). + +keel's database is in WAL mode. Once the last connection to it closes and checkpoints, +`keel.db-wal` and `keel.db-shm` may be gone, and a `mode=ro` open then fails with "unable to open +database file": SQLite would have to CREATE the `-shm`, and `mode=ro` forbids it. `setup.inspect` +opened that way, so on a running, set-up deployment the console answered "keel isn't running +here" -- and the stopped view has no HALT. + +The fix is a read-write open (`mode=rw`, which creates no database file) with `PRAGMA query_only += ON`, which makes SQLite itself refuse every write on the connection. Both halves are pinned +here: the read succeeds in that state, and a write through the console's read connection fails. +The halt is the console's one write to the trading database, and it keeps its own opener. + +Every database in this module is a scratch copy under `tmp_path`. +""" + +from __future__ import annotations + +import http.client +import json +import sqlite3 +from collections.abc import Iterator +from pathlib import Path +from typing import Any + +import pytest + +from keel.data.db import connect +from keel.data.repository import Repository +from keel.web import api as web_api +from keel.web import server as web_server +from keel.web.security import HALT_HEADER, SESSION_COOKIE, halt_token + + +def _strip_sidecars(db_path: str) -> None: + """Leave the database the way a checkpointing last close leaves it: one file, no `-wal`, no + `-shm`. Some SQLite builds keep the sidecars on close (persistent WAL), so the state is made + explicitly rather than hoped for -- after a TRUNCATE checkpoint the WAL holds nothing, so + removing the files loses no committed row. + + It then proves the state reaches the bug: a `mode=ro` open must fail here, or every test below + would pass by vacuum.""" + conn = sqlite3.connect(db_path) + try: + assert conn.execute("PRAGMA journal_mode").fetchone()[0] == "wal" + conn.execute("PRAGMA wal_checkpoint(TRUNCATE)").fetchall() + finally: + conn.close() + for suffix in ("-wal", "-shm"): + Path(db_path + suffix).unlink(missing_ok=True) + assert sorted(p.name for p in Path(db_path).parent.glob("keel.db*")) == ["keel.db"] + ro = sqlite3.connect(f"file:{db_path}?mode=ro", uri=True) + try: + with pytest.raises(sqlite3.OperationalError, match="unable to open"): + ro.execute("SELECT 1 FROM rules").fetchall() + finally: + ro.close() + + +def _kill_switch(db_path: str) -> object: + conn = connect(db_path) + try: + return Repository(conn).get_state("kill_switch") + finally: + conn.close() + + +def _request( + cfg: web_server.ServeConfig, method: str, path: str, headers: dict[str, str] | None = None +) -> tuple[int, Any]: + conn = http.client.HTTPConnection(cfg.host, cfg.port, timeout=10) + sent = {"Host": f"{cfg.host}:{cfg.port}", "Cookie": f"{SESSION_COOKIE}={cfg.token}"} + sent.update(headers or {}) + try: + conn.request(method, path, headers=sent) + response = conn.getresponse() + body = response.read().decode("utf-8", "replace") + return response.status, json.loads(body) if body else None + finally: + conn.close() + + +# -- the read succeeds without the sidecars -------------------------------------------------------- + + +def test_inspect_reads_a_wal_database_whose_sidecars_are_gone( + deployment: tuple[str, str], +) -> None: + from keel.commands.setup import inspect + + db_path, config_path = deployment + _strip_sidecars(db_path) + + state = inspect(config_path, db_path) + + assert state.has_usable_database, state + + +def test_pulse_on_a_deployment_whose_sidecars_are_gone_offers_halt( + running: web_server.ServeConfig, +) -> None: + """The issue's symptom, end to end: Pulse is the report and its HALT, not the stopped view -- + and the served token still halts.""" + _strip_sidecars(running.db_path) + + status, document = _request(running, "GET", "/api/pulse") + + assert status == 200 + assert document["engine"]["value"] != "stopped", document["engine"] + assert document["data"] is not None, "the stopped view would hide HALT" + assert document["data"]["halt_token"] == halt_token(running.token) + + status, _ = _request( + running, + "POST", + "/api/gates/halt", + {"X-Keel-Client": "1", HALT_HEADER: document["data"]["halt_token"]}, + ) + assert status == 200 + assert _kill_switch(running.db_path) is True + + +def test_a_sleeve_sell_lookup_reads_a_wal_database_whose_sidecars_are_gone( + deployment: tuple[str, str], +) -> None: + """The setup page's promote path reads the rule's class over the same kind of open, and it + fails CLOSED -- so before #976 it refused every promote in this state.""" + from keel.commands.setup import _sleeve_sell_kind + + db_path, _config_path = deployment + _strip_sidecars(db_path) + + assert _sleeve_sell_kind(Path(db_path), 999) is None + + +def test_venue_readiness_reads_a_wal_database_whose_sidecars_are_gone( + deployment: tuple[str, str], +) -> None: + """`/api/venues` reads each venue's trade scope over the same kind of open; before #976 this + state read as "unreadable" on a database that was fine.""" + from keel.venue_readiness import _read_only_trade_scope + + db_path, _config_path = deployment + _strip_sidecars(db_path) + + assert _read_only_trade_scope(db_path, "coinbase") == (None, False) + + +# -- and it still cannot write --------------------------------------------------------------------- + + +def test_a_write_through_the_console_read_connection_fails( + deployment: tuple[str, str], +) -> None: + """The console's read seam is read-only at the ENGINE level, not by call discipline: a row + write, a schema write and a header write are each refused by SQLite, and the switch the row + write aimed at is unchanged.""" + db_path, _config_path = deployment + before = _kill_switch(db_path) + repo = web_api.open_repo(db_path) + try: + conn = repo._conn + with pytest.raises(sqlite3.OperationalError, match="readonly"): + repo.set_state("kill_switch", True) + for statement in ("CREATE TABLE intruder (x)", "PRAGMA user_version = 99"): + with pytest.raises(sqlite3.OperationalError, match="readonly"): + conn.execute(statement) + finally: + web_api.close_repo(repo) + assert _kill_switch(db_path) == before + + +def test_the_read_only_opener_creates_no_database(tmp_path: Path) -> None: + """A read of a database that is not there must not bring one into existence -- the console + would then report a healthy empty install instead of offering setup.""" + from keel.data.db import connect_read_only + + missing = tmp_path / "keel.db" + with pytest.raises(sqlite3.OperationalError): + connect_read_only(missing) + assert list(tmp_path.iterdir()) == [] + + +class _Recorded(sqlite3.Connection): + """A connection that remembers `query_only` as it stood when it was closed.""" + + closed_query_only: int | None = None + + def close(self) -> None: + try: + self.closed_query_only = int(self.execute("PRAGMA query_only").fetchone()[0]) + except sqlite3.ProgrammingError: # already closed + pass + super().close() + + +@pytest.fixture +def opened(monkeypatch: pytest.MonkeyPatch) -> Iterator[list[_Recorded]]: + """Every sqlite3 connection opened while the test runs, server thread included.""" + real = sqlite3.connect + seen: list[_Recorded] = [] + + def spy(*args: Any, **kwargs: Any) -> sqlite3.Connection: + conn = real(*args, factory=_Recorded, **kwargs) + seen.append(conn) + return conn + + monkeypatch.setattr(sqlite3, "connect", spy) + yield seen + + +def _query_only(conn: _Recorded) -> int: + if conn.closed_query_only is not None: + return conn.closed_query_only + return int(conn.execute("PRAGMA query_only").fetchone()[0]) + + +def test_every_connection_a_console_read_opens_refuses_writes( + running: web_server.ServeConfig, opened: list[_Recorded] +) -> None: + """Every JSON read route, `/api/config` and the setup inspection with it, each over a + sidecar-less WAL database: every connection the console opened to answer them is + `query_only`. A reader that slipped back to the read-write `connect` fails here by name.""" + _strip_sidecars(running.db_path) + opened.clear() + + for path in web_api.API_ROUTES: + status, document = _request(running, "GET", path) + assert status == 200, (path, document) + assert document["engine"]["value"] != "stopped", path + + assert len(web_api.API_ROUTES) >= 10, "the sweep must cover the table, not an empty one" + assert len(opened) >= len(web_api.API_ROUTES), "the spy must see the reads it judges" + writable = [conn for conn in opened if _query_only(conn) != 1] + assert writable == [] + + +def test_the_halt_still_writes_after_the_reads( + running: web_server.ServeConfig, opened: list[_Recorded] +) -> None: + """The halt is the console's one write to the trading database, and it is not routed through + the read seam: it engages the switch, over a connection that is not `query_only`.""" + _strip_sidecars(running.db_path) + _request(running, "GET", "/api/pulse") + opened.clear() + + status, _ = _request( + running, + "POST", + "/api/gates/halt", + {"X-Keel-Client": "1", HALT_HEADER: halt_token(running.token)}, + ) + + halt_connections = list(opened) + assert status == 200 + assert halt_connections, "the spy must see the halt's open" + assert all(_query_only(conn) == 0 for conn in halt_connections) + assert _kill_switch(running.db_path) is True From 61f88f07870e07678169e0dcd321fa499413556a Mon Sep 17 00:00:00 2001 From: Elmehdi Aitbrahim Date: Mon, 12 Oct 2026 12:13:24 -0400 Subject: [PATCH 04/11] test: #976 fixture no longer assumes every SQLite build refuses mode=ro Linux CI's SQLite opens a sidecar-less WAL database under mode=ro, so the fixture's built-in reproduction failed every test there. The reproduction is now its own test, skipped (and saying why) on a build that does not have the bug; the read and read-only tests run on every build. Co-Authored-By: Claude Opus 5.5 --- tests/web/test_console_reads_are_read_only.py | 29 +++++++++++++++---- 1 file changed, 23 insertions(+), 6 deletions(-) diff --git a/tests/web/test_console_reads_are_read_only.py b/tests/web/test_console_reads_are_read_only.py index dbd35e82..80d16e13 100644 --- a/tests/web/test_console_reads_are_read_only.py +++ b/tests/web/test_console_reads_are_read_only.py @@ -36,10 +36,8 @@ def _strip_sidecars(db_path: str) -> None: """Leave the database the way a checkpointing last close leaves it: one file, no `-wal`, no `-shm`. Some SQLite builds keep the sidecars on close (persistent WAL), so the state is made explicitly rather than hoped for -- after a TRUNCATE checkpoint the WAL holds nothing, so - removing the files loses no committed row. - - It then proves the state reaches the bug: a `mode=ro` open must fail here, or every test below - would pass by vacuum.""" + removing the files loses no committed row. Whether `mode=ro` can open this state depends on + the SQLite build; `test_the_scratch_state_is_the_one_976_reported` says which one this is.""" conn = sqlite3.connect(db_path) try: assert conn.execute("PRAGMA journal_mode").fetchone()[0] == "wal" @@ -49,12 +47,31 @@ def _strip_sidecars(db_path: str) -> None: for suffix in ("-wal", "-shm"): Path(db_path + suffix).unlink(missing_ok=True) assert sorted(p.name for p in Path(db_path).parent.glob("keel.db*")) == ["keel.db"] + + +def _mode_ro_opens(db_path: str) -> bool: ro = sqlite3.connect(f"file:{db_path}?mode=ro", uri=True) try: - with pytest.raises(sqlite3.OperationalError, match="unable to open"): - ro.execute("SELECT 1 FROM rules").fetchall() + ro.execute("SELECT 1 FROM rules").fetchall() + except sqlite3.OperationalError as exc: + assert "unable to open" in str(exc), exc + return False finally: ro.close() + return True + + +def test_the_scratch_state_is_the_one_976_reported(deployment: tuple[str, str]) -> None: + """The fixture reaches the bug: on a SQLite build like the one #976 was found on (macOS, + 3.51.0), `mode=ro` cannot open the sidecar-less file -- which is what every test below then + reads successfully. Some builds (Linux CI) create the `-shm` even under `mode=ro`; there the + old open worked too, the tests below still pin the read-only half, and this one says so + rather than passing by vacuum.""" + db_path, _config_path = deployment + _strip_sidecars(db_path) + if _mode_ro_opens(db_path): + pytest.skip("this SQLite build opens a sidecar-less WAL database under mode=ro") + assert not Path(db_path + "-shm").exists() def _kill_switch(db_path: str) -> object: From b885728da41a895f82d5932678062733192da640 Mon Sep 17 00:00:00 2001 From: Elmehdi Aitbrahim Date: Mon, 12 Oct 2026 12:24:28 -0400 Subject: [PATCH 05/11] fix(web): refuse and close on untrustworthy request framing; harden #955/#971 tests One parser (_declared_body_length) for the drain and _read_json_object: Transfer-Encoding, a non-digit/duplicate Content-Length close the connection. Halt still engages first. Co-Authored-By: Claude Opus 5.5 --- keel/web/server.py | 45 +++++++--- tests/web/test_server.py | 175 +++++++++++++++++++++++++++++++++------ 2 files changed, 185 insertions(+), 35 deletions(-) diff --git a/keel/web/server.py b/keel/web/server.py index 0272e5ef..28fea1d4 100644 --- a/keel/web/server.py +++ b/keel/web/server.py @@ -46,6 +46,7 @@ from __future__ import annotations import json +import re import signal import socket import sys @@ -527,6 +528,33 @@ def _refuse(self, code: int, heading: str, detail: str) -> None: return self._send(code, f"{heading}\n\n{detail}\n") + def _declared_body_length(self) -> int | None: + """The request body's length, or `None` when the framing cannot be trusted -- and then the + connection is already marked to close. + + **One parser, so the drain and the reader cannot disagree** about where a body ends (#955 + review). Only ONE shape is trusted: no `Transfer-Encoding`, and either no `Content-Length` + (a body of zero) or exactly one, made only of ASCII digits. Everything else is a request + this server cannot size, and the safe answer is to stop using the connection rather than + guess: `int()` happily reads `"+2"`, `"-2"` and `"1_0"` (as 10); a negative length reads + as "no body"; a chunked body has no length at all; and two `Content-Length` headers (or one + beside a `Transfer-Encoding`) are the request-smuggling shape, where this server and + whatever sits in front of it measure the body differently. Draining by a wrong measure + leaves body bytes to be parsed as the next request line. + + The halt never calls this to decide anything -- it reads no body and engages first. + """ + if self.headers.get("Transfer-Encoding") is not None: + self.close_connection = True + return None + values = self.headers.get_all("Content-Length") or [] + if not values: + return 0 + if len(values) > 1 or re.fullmatch(r"[0-9]+", values[0]) is None: + self.close_connection = True + return None + return int(values[0]) + def _drain_request_body(self) -> None: """Consume an unread request body, or close the connection rather than consume it. @@ -553,12 +581,10 @@ def _drain_request_body(self) -> None: # Reading it a second time blocks until the client's timeout, which is a hang rather # than an error and was exactly what the first version of this did. return - try: - length = int(self.headers.get("Content-Length") or 0) - except ValueError: - self.close_connection = True + length = self._declared_body_length() + if length is None: return - if length <= 0: + if length == 0: return if length > _MAX_BODY_BYTES: self.close_connection = True @@ -950,11 +976,10 @@ def _read_json_object(self) -> dict[str, str] | None: bodies at #540 along with the form that sent them; this is the one place that difference can be exploited, so it is the one place it is refused. """ - try: - length = int(self.headers.get("Content-Length") or 0) - except ValueError: - return None - if length < 0 or length > _MAX_BODY_BYTES: + length = self._declared_body_length() + if length is None or length > _MAX_BODY_BYTES: + # Untrusted framing has already closed the connection; an oversized length is closed + # by the drain that every refusal runs. return None if length == 0: # An action with no declared inputs is submitted with an empty body, and that is a diff --git a/tests/web/test_server.py b/tests/web/test_server.py index c747c3d4..d5cecc5b 100644 --- a/tests/web/test_server.py +++ b/tests/web/test_server.py @@ -19,6 +19,8 @@ import dataclasses import http.client import json +import re +import socket import threading import time from collections.abc import Iterator @@ -1983,6 +1985,17 @@ def test_the_halt_still_needs_the_halt_token(deployment) -> None: # -- the halt token is not a release token (#968) -------------------------------------------------- +def _engage_kill_switch(db_path: str) -> None: + from keel.data.db import connect + from keel.data.repository import Repository + + conn = connect(db_path) + try: + Repository(conn).set_state("kill_switch", True) + finally: + conn.close() + + def _kill_switch(db_path: str) -> object: from keel.data.db import connect from keel.data.repository import Repository @@ -2206,32 +2219,134 @@ def test_a_halt_with_a_body_engages_and_leaves_the_connection_clean(deployment, assert json.loads(get_body)["data"] is not None -def test_a_halt_with_an_oversized_body_still_engages(deployment) -> None: +# Framing a drain cannot trust: every case here declares a body the server cannot size by one plain +# non-negative decimal `Content-Length`, so the only safe answer is to stop using the connection. +_UNTRUSTWORTHY_FRAMING = [ + pytest.param( + [("Transfer-Encoding", "chunked")], b"2\r\n{}\r\n0\r\n\r\n", id="chunked" + ), + pytest.param([("Content-Length", "-2")], b"{}", id="negative-length"), + pytest.param([("Content-Length", "+2")], b"{}", id="plus-signed-length"), + pytest.param([("Content-Length", "1_0")], b"{}", id="underscore-length"), + pytest.param( + [("Content-Length", "2"), ("Content-Length", "2")], b"{}", id="duplicate-length" + ), + pytest.param( + [("Content-Length", "2"), ("Content-Length", "40")], b"{}", id="conflicting-lengths" + ), + pytest.param( + [("Content-Length", "2"), ("Transfer-Encoding", "chunked")], + b"{}", + id="length-and-transfer-encoding", + ), +] + + +def _raw_post_then_get( + cfg, path: str, headers: list[tuple[str, str]], framing: list[tuple[str, str]], body: bytes +) -> bytes: + """POST with hand-written framing, pipeline a GET behind it, and return everything the server + sends until it closes the socket. + + `http.client` would normalise the framing, so this speaks bytes. The GET is written in the same + burst: a server that keeps the connection and mis-sizes the body answers it (or 501s on the + leftover bytes), so a second `HTTP/1.` status line in the result is the desynchronisation.""" + lines = [f"POST {path} HTTP/1.1"] + lines += [f"{name}: {value}" for name, value in [*headers, *framing]] + request = ("\r\n".join(lines) + "\r\n\r\n").encode() + body + request += f"GET /api/config HTTP/1.1\r\nHost: {cfg.host}:{cfg.port}\r\n\r\n".encode() + received = b"" + with socket.create_connection((cfg.host, cfg.port), timeout=3) as sock: + sock.sendall(request) + while True: + try: + chunk = sock.recv(65536) + except ConnectionResetError: + # A reset after the first response was read is the server closing with our bytes + # unread -- still a close. A reset before any response is not tolerated. + assert received, "connection reset before any response" + break + if not chunk: + break + received += chunk + return received + + +def _statuses(received: bytes) -> list[bytes]: + """Every status code in a raw byte stream, found anywhere in it. + + Not line-anchored: the stdlib writes its 501 straight after the previous body, and a request + line it cannot parse is answered with an HTML error page and no status line at all -- which + is also reported, as `b"ERR"`, because it is the same desynchronisation.""" + found = re.findall(rb"HTTP/1\.[01] (\d{3})|(

Error response

)", received) + return [code or b"ERR" for code, _page in found] + + +def _halt_headers(cfg) -> list[tuple[str, str]]: + return [ + ("Host", f"{cfg.host}:{cfg.port}"), + ("Cookie", _session(cfg)), + ("X-Keel-Client", "1"), + (security.HALT_HEADER, security.halt_token(cfg.token)), + ] + + +@pytest.mark.parametrize(("framing", "body"), _UNTRUSTWORTHY_FRAMING) +def test_a_halt_with_untrustworthy_framing_engages_and_closes(deployment, framing, body) -> None: + """**#955 review: framing the server cannot size is not drained and not ignored -- it closes.** + + `int("1_0")` is 10, `int("+2")` is 2, a negative length is "no body", and a chunked body has + no length at all. Each used to be drained wrongly or skipped, leaving the body to be parsed as + the next request. The halt still engages and answers 200 FIRST (it reads no body, and stopping + is never slower than starting); then the connection is closed, so the pipelined GET behind it + gets no answer at all -- never a 501.""" + with _serving(deployment) as cfg: + received = _raw_post_then_get(cfg, "/api/gates/halt", _halt_headers(cfg), framing, body) + + assert _kill_switch(deployment[0]) is True + assert _statuses(received) == [b"200"], received[:300] + + +@pytest.mark.parametrize(("framing", "body"), _UNTRUSTWORTHY_FRAMING) +def test_a_json_post_with_untrustworthy_framing_is_refused_and_closes( + empty_machine, framing, body +) -> None: + """The same framing on a setup action is refused (400, the shape a bad length gets today) and + the connection closes: the body is not read, not guessed at, and not left for the next request.""" + cfg = empty_machine + headers = [ + ("Host", f"{cfg.host}:{cfg.port}"), + ("Cookie", _session(cfg)), + ("X-Keel-Client", "1"), + ("X-Keel-CSRF", _csrf(cfg)), + ] + received = _raw_post_then_get(cfg, "/api/setup/config", headers, framing, body) + + assert _statuses(received) == [b"400"], received[:300] + head, _, payload = received.partition(b"\r\n\r\n") + assert json.loads(payload.decode())["error"]["status"] == "400" + + +def test_a_halt_with_an_oversized_body_still_engages_and_closes(deployment) -> None: """A body past `_MAX_BODY_BYTES` is not drained (that is the memory-exhaustion primitive the cap denies) -- the connection is closed instead. The halt itself must still happen: refusing - a stop over its envelope is the one failure this route must not have.""" - oversized = b"x" * (web_server._MAX_BODY_BYTES + 1) - with _serving(deployment) as cfg: - conn = http.client.HTTPConnection(cfg.host, cfg.port, timeout=10) - try: - conn.request( - "POST", - "/api/gates/halt", - body=oversized, - headers={ - "Host": f"{cfg.host}:{cfg.port}", - "Cookie": _session(cfg), - "X-Keel-Client": "1", - security.HALT_HEADER: security.halt_token(cfg.token), - "Content-Length": str(len(oversized)), - }, - ) - response = conn.getresponse() - response.read() - finally: - conn.close() + a stop over its envelope is the one failure this route must not have. - assert response.status == 200 + Only the HEADERS declare the oversized length; the body is never sent. Sending it would let + the server's close reset the socket before the 200 is read, which says nothing about the + halt. Read to EOF: the 200, then the server hanging up, is the whole contract.""" + declared = web_server._MAX_BODY_BYTES + 1 + with _serving(deployment) as cfg: + lines = ["POST /api/gates/halt HTTP/1.1"] + lines += [f"{name}: {value}" for name, value in _halt_headers(cfg)] + lines += [f"Content-Length: {declared}"] + received = b"" + with socket.create_connection((cfg.host, cfg.port), timeout=3) as sock: + sock.sendall(("\r\n".join(lines) + "\r\n\r\n").encode()) + while chunk := sock.recv(65536): + received += chunk # EOF (b"") ends the loop: the server closed + + assert _statuses(received) == [b"200"], received[:300] assert _kill_switch(deployment[0]) is True @@ -2351,7 +2466,9 @@ def test_serve_enforces_the_remote_session_lifetime(deployment, monkeypatch, tmp status, _headers, body = _request(installed, "/api/config", cookie=_session(installed)) assert status == 403, body - assert "lifetime" in body + error = json.loads(body)["error"] + assert error["status"] == "403" + assert error["title"] == "Session expired" def test_serve_refuses_a_release_on_a_declared_remote_deployment( @@ -2360,6 +2477,7 @@ def test_serve_refuses_a_release_on_a_declared_remote_deployment( """#971 item 3: the release locality check read an empty declaration, so a tunnel that rewrote Host to loopback passed it. With the declaration kept, a release from loopback -- which is what a tunnel looks like -- is refused before the gates token is even read.""" + _engage_kill_switch(deployment[0]) with _through_serve( deployment, monkeypatch, tmp_path, external_hosts=frozenset({"keel.example.com"}) ) as (_given, installed): @@ -2373,4 +2491,11 @@ def test_serve_refuses_a_release_on_a_declared_remote_deployment( ) assert status == 403, body - assert "loopback session on a loopback bind" in body + error = json.loads(body)["error"] + assert error["status"] == "403" + assert error["title"] == "Refused" + assert error["detail"].startswith( + "Gated actions are restricted to a loopback session on a loopback bind." + ) + # The refusal fired BEFORE the release could act: an engaged switch is still engaged. + assert _kill_switch(deployment[0]) is True From a3b133a8ccee8f56970703146daeeebcc2ba5818 Mon Sep 17 00:00:00 2001 From: Elmehdi Aitbrahim Date: Mon, 12 Oct 2026 12:25:27 -0400 Subject: [PATCH 06/11] fix(data): console reader never checkpoints; URI built by a pure helper; read-path pin connect_read_only disables checkpoint-on-close, builds its URI with _read_only_uri (UNC, drive, reserved characters), and the docstring states what read-only rests on. AST pin over the console read path forbids query_only/attach/wal_checkpoint/vacuum. Co-Authored-By: Claude Opus 5.5 --- keel/data/db.py | 37 ++++- tests/web/test_console_reads_are_read_only.py | 139 +++++++++++++++++- tests/web/test_server.py | 8 +- 3 files changed, 171 insertions(+), 13 deletions(-) diff --git a/keel/data/db.py b/keel/data/db.py index 16dc7914..7e63436b 100644 --- a/keel/data/db.py +++ b/keel/data/db.py @@ -16,8 +16,9 @@ import json import sqlite3 import time +import urllib.parse from collections.abc import Callable -from pathlib import Path +from pathlib import Path, PurePath from typing import Any SCHEMA_VERSION = 22 @@ -1213,6 +1214,21 @@ def connect(path: str | Path = "keel.db") -> sqlite3.Connection: return conn +def _read_only_uri(path: PurePath) -> str: + """The `file:` URI `connect_read_only` opens, built from an ABSOLUTE path. + + **Not `Path.as_uri()`.** On Windows it yields `file:///C:/...` for a drive path but, for a UNC + path (`\\\\server\\share`), a form SQLite does not read as the same file; and anything built + by hand must percent-encode, because in a URI `#`, `?` and `%` stop being path characters. So: + `as_posix()`, a `file://` prefix when the path already starts with `/` (POSIX, and UNC after + `as_posix` -- which gives `//server/share`, hence the four slashes) and `file:///` when it + starts with a drive letter, and `quote` keeping only `/` and `:`. + """ + posix = path.as_posix() + prefix = "file://" if posix.startswith("/") else "file:///" + return prefix + urllib.parse.quote(posix, safe="/:") + "?mode=rw" + + def connect_read_only(path: str | Path) -> sqlite3.Connection: """Open an EXISTING database to read it, never to write it (#976). @@ -1224,15 +1240,26 @@ def connect_read_only(path: str | Path) -> sqlite3.Connection: So the file is opened `mode=rw` -- which may create the sidecars, and which, unlike a plain `sqlite3.connect`, refuses to CREATE a missing database -- and `PRAGMA query_only = ON` is - set before anything is read. That makes SQLite itself refuse every write on this connection: - rows, schema, and header pragmas alike. Read-only holds at the engine, not by call discipline. + set before anything is read, so SQLite refuses every ordinary write on this connection: rows, + schema, and header pragmas alike. + + **What that does and does not rest on.** `query_only` is per-connection and the connection can + switch it back off, and neither it nor `mode=rw` blocks `ATTACH` or the checkpoint pragmas. + Read-only here is SQLite refusing ordinary writes PLUS no console reader issuing + `PRAGMA query_only`, `ATTACH`, a checkpoint or `VACUUM`; a structural test walks the console's + read path for exactly those (`test_the_console_read_path_never_asks_sqlite_to_lift_read_only`). + + **Checkpoint-on-close is disabled** (`SQLITE_DBCONFIG_NO_CKPT_ON_CLOSE`). The last connection + to close a WAL database otherwise checkpoints it into the main file, and a console that closes + after the agent has gone would be writing the trading database. The `-wal` is left for the + writer, which checkpoints on its own schedule. Unlike `connect`, this sets no `journal_mode`: changing the file's journal is a write, and a reader has no business deciding it. """ - uri = Path(path).absolute().as_uri() + "?mode=rw" - conn = sqlite3.connect(uri, uri=True) + conn = sqlite3.connect(_read_only_uri(Path(path).absolute()), uri=True) try: + conn.setconfig(sqlite3.SQLITE_DBCONFIG_NO_CKPT_ON_CLOSE, True) conn.execute("PRAGMA query_only = ON") conn.execute(f"PRAGMA busy_timeout = {BUSY_TIMEOUT_MS}") except BaseException: diff --git a/tests/web/test_console_reads_are_read_only.py b/tests/web/test_console_reads_are_read_only.py index 80d16e13..2aab7b62 100644 --- a/tests/web/test_console_reads_are_read_only.py +++ b/tests/web/test_console_reads_are_read_only.py @@ -16,16 +16,18 @@ from __future__ import annotations +import ast +import hashlib import http.client import json import sqlite3 from collections.abc import Iterator -from pathlib import Path +from pathlib import Path, PurePosixPath, PureWindowsPath from typing import Any import pytest -from keel.data.db import connect +from keel.data.db import _read_only_uri, connect, connect_read_only, migrate from keel.data.repository import Repository from keel.web import api as web_api from keel.web import server as web_server @@ -273,3 +275,136 @@ def test_the_halt_still_writes_after_the_reads( assert halt_connections, "the spy must see the halt's open" assert all(_query_only(conn) == 0 for conn in halt_connections) assert _kill_switch(running.db_path) is True + + +# -- a reader closing last must not checkpoint the trading database (#977 review) --------------- + + +def _db_with_unckeckpointed_wal(path: Path) -> None: + """A database whose recent rows live ONLY in the `-wal`: the shape the agent leaves while it + runs. The writer turns auto-checkpoint and checkpoint-on-close off so the state is exact.""" + conn = connect(str(path)) + migrate(conn) + conn.execute("PRAGMA wal_checkpoint(TRUNCATE)") + conn.close() + writer = sqlite3.connect(path) + writer.execute("PRAGMA wal_autocheckpoint = 0") + writer.setconfig(sqlite3.SQLITE_DBCONFIG_NO_CKPT_ON_CLOSE, True) + Repository(writer).set_state("wal_only_row", {"n": 1}) + writer.close() + + +def _sha256(path: Path) -> str: + return hashlib.sha256(path.read_bytes()).hexdigest() + + +def test_a_console_reader_closing_last_leaves_the_database_file_untouched(tmp_path: Path) -> None: + """**`query_only` does not stop the checkpoint a last close performs.** A console reader that + closes after the agent has gone would fold the WAL into `keel.db`; that is a write to the + trading database from a reader. `connect_read_only` disables checkpoint-on-close, so the main + file is byte-identical and the `-wal` is left for the writer.""" + db = tmp_path / "keel.db" + _db_with_unckeckpointed_wal(db) + wal = Path(f"{db}-wal") + assert wal.exists() and wal.stat().st_size > 0, ( + "the scratch state must hold an un-checkpointed WAL" + ) + before = (_sha256(db), db.stat().st_size) + + conn = connect_read_only(db) + try: + assert Repository(conn).get_state("wal_only_row", default=None) == {"n": 1} + finally: + conn.close() + + assert (_sha256(db), db.stat().st_size) == before + assert wal.exists() and wal.stat().st_size > 0 + + +@pytest.mark.parametrize( + ("path", "uri"), + [ + (PureWindowsPath(r"\\server\share\keel.db"), "file:////server/share/keel.db?mode=rw"), + (PureWindowsPath(r"C:\x y\keel.db"), "file:///C:/x%20y/keel.db?mode=rw"), + (PurePosixPath("/a b/#?%/keel.db"), "file:///a%20b/%23%3F%25/keel.db?mode=rw"), + ], + ids=["unc", "drive-with-space", "posix-reserved-characters"], +) +def test_the_read_only_uri_is_built_for_every_path_shape(path: PurePosixPath, uri: str) -> None: + """A URI is the one place a path's `#`, `?` and `%` change meaning, and a Windows path has no + leading slash for `file://` to attach to. Pure paths, so no Windows is needed to pin it.""" + assert _read_only_uri(path) == uri + + +def test_a_database_under_a_directory_with_awkward_characters_opens(tmp_path: Path) -> None: + awkward = tmp_path / "a b#?%\u00e9" + awkward.mkdir() + db = awkward / "keel.db" + conn = connect(str(db)) + migrate(conn) + Repository(conn).set_state("k", {"v": 1}) + conn.close() + + reader = connect_read_only(db) + try: + assert Repository(reader).get_state("k", default=None) == {"v": 1} + finally: + reader.close() + + +_REPO = Path(__file__).resolve().parents[2] +#: What the console's read path must never send to SQLite: each of these either undoes +#: `query_only` (it is per-connection and reversible by the connection itself), reaches another +#: file, or writes the database file outside an ordinary statement. +_FORBIDDEN_SQL = ("query_only", "attach", "wal_checkpoint", "vacuum") + + +def _console_read_path_files() -> list[Path]: + return [ + *sorted((_REPO / "keel" / "web").glob("*.py")), + _REPO / "keel" / "commands" / "setup.py", + _REPO / "keel" / "commands" / "pulse.py", + _REPO / "keel" / "venue_readiness.py", + ] + + +def _execute_string_constants(tree: ast.AST) -> tuple[int, list[str]]: + """(number of `.execute`/`.executescript` calls, every string constant inside their args).""" + calls = 0 + strings: list[str] = [] + for node in ast.walk(tree): + if ( + isinstance(node, ast.Call) + and isinstance(node.func, ast.Attribute) + and node.func.attr in {"execute", "executescript"} + ): + calls += 1 + for arg in node.args: + strings += [ + n.value + for n in ast.walk(arg) + if isinstance(n, ast.Constant) and isinstance(n.value, str) + ] + return calls, strings + + +def test_the_console_read_path_never_asks_sqlite_to_lift_read_only() -> None: + """**`query_only` is per-connection and the connection can switch it off; ATTACH and the + checkpoint pragmas are not blocked by it.** Read-only therefore rests on SQLite refusing + ordinary writes PLUS no console reader issuing those statements. This is the second half, + pinned structurally over the AST of every module on the console's read path.""" + files = _console_read_path_files() + assert all(f.exists() for f in files), [str(f) for f in files if not f.exists()] + total = 0 + offenders: list[tuple[str, str]] = [] + for file in files: + calls, strings = _execute_string_constants(ast.parse(file.read_text())) + total += calls + offenders += [ + (file.name, text) + for text in strings + if any(word in text.lower() for word in _FORBIDDEN_SQL) + ] + + assert total > 0, "the walker found no execute calls: it would pass by vacuum" + assert offenders == [] diff --git a/tests/web/test_server.py b/tests/web/test_server.py index d5cecc5b..0628fa72 100644 --- a/tests/web/test_server.py +++ b/tests/web/test_server.py @@ -2222,15 +2222,11 @@ def test_a_halt_with_a_body_engages_and_leaves_the_connection_clean(deployment, # Framing a drain cannot trust: every case here declares a body the server cannot size by one plain # non-negative decimal `Content-Length`, so the only safe answer is to stop using the connection. _UNTRUSTWORTHY_FRAMING = [ - pytest.param( - [("Transfer-Encoding", "chunked")], b"2\r\n{}\r\n0\r\n\r\n", id="chunked" - ), + pytest.param([("Transfer-Encoding", "chunked")], b"2\r\n{}\r\n0\r\n\r\n", id="chunked"), pytest.param([("Content-Length", "-2")], b"{}", id="negative-length"), pytest.param([("Content-Length", "+2")], b"{}", id="plus-signed-length"), pytest.param([("Content-Length", "1_0")], b"{}", id="underscore-length"), - pytest.param( - [("Content-Length", "2"), ("Content-Length", "2")], b"{}", id="duplicate-length" - ), + pytest.param([("Content-Length", "2"), ("Content-Length", "2")], b"{}", id="duplicate-length"), pytest.param( [("Content-Length", "2"), ("Content-Length", "40")], b"{}", id="conflicting-lengths" ), From f0d1e9efa5b37348f755de7712f01d8696e79943 Mon Sep 17 00:00:00 2001 From: Elmehdi Aitbrahim Date: Mon, 12 Oct 2026 12:32:08 -0400 Subject: [PATCH 07/11] test(web): shorten a docstring line (review round 1) Co-Authored-By: Claude Opus 5.5 --- tests/web/test_server.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/web/test_server.py b/tests/web/test_server.py index 0628fa72..83bc9a2d 100644 --- a/tests/web/test_server.py +++ b/tests/web/test_server.py @@ -2308,7 +2308,7 @@ def test_a_json_post_with_untrustworthy_framing_is_refused_and_closes( empty_machine, framing, body ) -> None: """The same framing on a setup action is refused (400, the shape a bad length gets today) and - the connection closes: the body is not read, not guessed at, and not left for the next request.""" + the connection closes: the body is not read, not guessed at, and not left behind.""" cfg = empty_machine headers = [ ("Host", f"{cfg.host}:{cfg.port}"), From 90caaa08e8a2d22b3391adf3c3aa184a581b1cf0 Mon Sep 17 00:00:00 2001 From: Elmehdi Aitbrahim Date: Mon, 12 Oct 2026 12:58:36 -0400 Subject: [PATCH 08/11] fix(web): close on unsizeable or malformed Content-Length framing; widen the read-only SQL pin to the Repository Bound the Content-Length digit string before int() (a 5000-digit value raised out of the handler), treat stdlib header parse defects as untrustworthy framing (a space before the colon hid the length and desynchronised the connection), and walk repository.py/audit.py/db.py in the banned-SQL AST pin with a function-scoped allow-list. Co-Authored-By: Claude Opus 5.5 --- keel/web/server.py | 17 ++- tests/web/test_console_reads_are_read_only.py | 102 +++++++++++++----- tests/web/test_server.py | 41 +++++++ 3 files changed, 135 insertions(+), 25 deletions(-) diff --git a/keel/web/server.py b/keel/web/server.py index 28fea1d4..4400f283 100644 --- a/keel/web/server.py +++ b/keel/web/server.py @@ -84,6 +84,9 @@ #: primitive and there is no proxy in front of this server to impose a limit. A setup form carries #: an action key and a token. _MAX_BODY_BYTES = 8 * 1024 +# More digits than any honest `Content-Length` has; a longer one is closed on, never `int()`-ed +# (CPython raises ValueError past 4300 digits). +_MAX_CONTENT_LENGTH_DIGITS = 18 @dataclass(frozen=True) @@ -544,13 +547,25 @@ def _declared_body_length(self) -> int | None: The halt never calls this to decide anything -- it reads no body and engages first. """ + # A header line the stdlib parser could not read (`Content-Length : 2`, a space before the + # colon) ends the header parse early: the length never reaches `self.headers`, and what is + # left sits in the payload. Absence of a length is then NOT "a body of zero". + if self.headers.defects or self.headers.get_payload(): + self.close_connection = True + return None if self.headers.get("Transfer-Encoding") is not None: self.close_connection = True return None values = self.headers.get_all("Content-Length") or [] if not values: return 0 - if len(values) > 1 or re.fullmatch(r"[0-9]+", values[0]) is None: + # Bounded BEFORE `int()`, which raises ValueError past 4300 digits. A few leading zeros + # ("01") still work; an absurdly padded or huge value closes like any unsizeable length. + if ( + len(values) > 1 + or len(values[0]) > _MAX_CONTENT_LENGTH_DIGITS + or re.fullmatch(r"[0-9]+", values[0]) is None + ): self.close_connection = True return None return int(values[0]) diff --git a/tests/web/test_console_reads_are_read_only.py b/tests/web/test_console_reads_are_read_only.py index 2aab7b62..c10e9a02 100644 --- a/tests/web/test_console_reads_are_read_only.py +++ b/tests/web/test_console_reads_are_read_only.py @@ -365,27 +365,59 @@ def _console_read_path_files() -> list[Path]: _REPO / "keel" / "commands" / "setup.py", _REPO / "keel" / "commands" / "pulse.py", _REPO / "keel" / "venue_readiness.py", + # #977 round 2: the readers' own SQL lives HERE, not in the modules above (the web + # modules hold five execute calls between them; `Repository` holds ~70). A banned + # statement added to a Repository read method reaches the read-only connection. + _REPO / "keel" / "data" / "repository.py", + _REPO / "keel" / "data" / "audit.py", + _REPO / "keel" / "data" / "db.py", ] -def _execute_string_constants(tree: ast.AST) -> tuple[int, list[str]]: - """(number of `.execute`/`.executescript` calls, every string constant inside their args).""" - calls = 0 - strings: list[str] = [] - for node in ast.walk(tree): +#: (file, enclosing function) -> the exact SQL strings allowed there. `connect_read_only` is the +#: one place `query_only` is SET (to ON); allow-listed by function AND exact string, never by +#: substring, so `PRAGMA query_only = OFF` in the same function still fails. +_ALLOWED_SQL = {("db.py", "connect_read_only"): {"PRAGMA query_only = ON"}} + +#: db.py functions that are WRITE paths by construction and never run on a read-only connection: +#: `connect` opens the read-write connection, `migrate` and the `_migrate_*` steps change schema. +#: They are excluded by name (not by file), and none of them issues a banned statement today -- +#: the exclusion only keeps a future checkpoint-on-purpose there from tripping the reader ban. +_WRITE_ONLY_FUNCTIONS = {("db.py", "connect"), ("db.py", "migrate")} + + +def _is_write_only(file_name: str, function: str | None) -> bool: + return (file_name, function) in _WRITE_ONLY_FUNCTIONS or ( + file_name == "db.py" and function is not None and function.startswith("_migrate") + ) + + +def _execute_string_constants(tree: ast.AST) -> list[tuple[str | None, str]]: + """Every `.execute`/`.executemany`/`.executescript` call, as (enclosing function, SQL + string) -- one row per string constant inside its args, and one `(function, "")` row for a + call with none (so the call count is `len(result)` at least).""" + rows: list[tuple[str | None, str]] = [] + + def visit(node: ast.AST, function: str | None) -> None: + if isinstance(node, ast.FunctionDef | ast.AsyncFunctionDef): + function = node.name if ( isinstance(node, ast.Call) and isinstance(node.func, ast.Attribute) - and node.func.attr in {"execute", "executescript"} + and node.func.attr in {"execute", "executemany", "executescript"} ): - calls += 1 - for arg in node.args: - strings += [ - n.value - for n in ast.walk(arg) - if isinstance(n, ast.Constant) and isinstance(n.value, str) - ] - return calls, strings + strings = [ + n.value + for arg in node.args + for n in ast.walk(arg) + if isinstance(n, ast.Constant) and isinstance(n.value, str) + ] + rows.extend([(function, text) for text in strings] or [(function, "")]) + for child in ast.iter_child_nodes(node): + visit(child, function) + + visit(tree, None) + return rows def test_the_console_read_path_never_asks_sqlite_to_lift_read_only() -> None: @@ -396,15 +428,37 @@ def test_the_console_read_path_never_asks_sqlite_to_lift_read_only() -> None: files = _console_read_path_files() assert all(f.exists() for f in files), [str(f) for f in files if not f.exists()] total = 0 - offenders: list[tuple[str, str]] = [] + offenders: list[tuple[str, str | None, str]] = [] for file in files: - calls, strings = _execute_string_constants(ast.parse(file.read_text())) - total += calls - offenders += [ - (file.name, text) - for text in strings - if any(word in text.lower() for word in _FORBIDDEN_SQL) - ] - - assert total > 0, "the walker found no execute calls: it would pass by vacuum" + for function, text in _execute_string_constants(ast.parse(file.read_text())): + if _is_write_only(file.name, function): + continue + total += 1 + if text in _ALLOWED_SQL.get((file.name, function), set()): + continue + if any(word in text.lower() for word in _FORBIDDEN_SQL): + offenders.append((file.name, function, text)) + + assert total >= 50, f"the walker found only {total} SQL calls: it would pass by vacuum" assert offenders == [] + + +def test_the_forbidden_sql_pin_sees_a_banned_statement_in_a_repository_method() -> None: + """The walker itself, on a planted source: an `OFF` pragma beside the one allowed `ON`, and + one in a Repository-shaped method, are both reported.""" + src = ( + "def connect_read_only(c):\n" + " c.execute('PRAGMA query_only = ON')\n" + " c.execute('PRAGMA query_only = OFF')\n" + "class R:\n" + " def read(self):\n" + " self.c.executemany('VACUUM', [])\n" + ) + rows = _execute_string_constants(ast.parse(src)) + flagged = [ + (f, t) + for f, t in rows + if t not in _ALLOWED_SQL.get(("db.py", f), set()) + and any(w in t.lower() for w in _FORBIDDEN_SQL) + ] + assert flagged == [("connect_read_only", "PRAGMA query_only = OFF"), ("read", "VACUUM")] diff --git a/tests/web/test_server.py b/tests/web/test_server.py index 83bc9a2d..516e306c 100644 --- a/tests/web/test_server.py +++ b/tests/web/test_server.py @@ -2235,6 +2235,16 @@ def test_a_halt_with_a_body_engages_and_leaves_the_connection_clean(deployment, b"{}", id="length-and-transfer-encoding", ), + # #977 round 2: a digit string too long for `int()` (past the 4300-digit conversion limit) used + # to raise out of the handler; it must read as an unsizeable length and close. + pytest.param([("Content-Length", "0" * 5000 + "2")], b"{}", id="5000-digit-zero-padded-length"), + pytest.param([("Content-Length", "9" * 5000)], b"{}", id="5000-digit-huge-length"), + # A space before the colon stops the stdlib header parse: the length never reaches + # `self.headers`, so a drain that only looks there sees a body of zero bytes. + pytest.param([("Content-Length ", "2")], b"{}", id="space-before-colon-length"), + pytest.param( + [("Transfer-Encoding ", "chunked")], b"2\r\n{}\r\n0\r\n\r\n", id="space-before-colon-te" + ), ] @@ -2323,6 +2333,37 @@ def test_a_json_post_with_untrustworthy_framing_is_refused_and_closes( assert json.loads(payload.decode())["error"]["status"] == "400" +@pytest.mark.parametrize( + ("framing", "body"), + [ + pytest.param([], b"", id="no-length"), + pytest.param([("Content-Length", "0")], b"", id="zero-length"), + pytest.param([("content-length", "2")], b"{}", id="lowercase-name"), + pytest.param([("Content-Length", "02")], b"{}", id="leading-zero"), + ], +) +def test_a_halt_with_trustworthy_framing_keeps_the_connection_open( + deployment, framing, body +) -> None: + """#977 round 2: the new framing checks must not close an honest browser POST. The GET behind + the halt (its own `Connection: close` ends the read) is answered, so there are two 200s.""" + with _serving(deployment) as cfg: + lines = ["POST /api/gates/halt HTTP/1.1"] + lines += [f"{n}: {v}" for n, v in [*_halt_headers(cfg), *framing]] + request = ("\r\n".join(lines) + "\r\n\r\n").encode() + body + request += ( + f"GET /api/config HTTP/1.1\r\nHost: {cfg.host}:{cfg.port}\r\n" + f"Cookie: {_session(cfg)}\r\nX-Keel-Client: 1\r\nConnection: close\r\n\r\n" + ).encode() + received = b"" + with socket.create_connection((cfg.host, cfg.port), timeout=3) as sock: + sock.sendall(request) + while chunk := sock.recv(65536): + received += chunk + + assert _statuses(received) == [b"200", b"200"], received[:300] + + def test_a_halt_with_an_oversized_body_still_engages_and_closes(deployment) -> None: """A body past `_MAX_BODY_BYTES` is not drained (that is the memory-exhaustion primitive the cap denies) -- the connection is closed instead. The halt itself must still happen: refusing From 1cee1a10ff7650be57d9c406883fd2e282c0228a Mon Sep 17 00:00:00 2001 From: Elmehdi Aitbrahim Date: Mon, 12 Oct 2026 13:17:53 -0400 Subject: [PATCH 09/11] test: scan every SQL string for lift-read-only statements; pin browser-shaped POSTs keep the connection #977 round 3: the AST pin now scans every non-docstring string constant (variable- and constant-held SQL included) through one shared _forbidden_sql used by the pin and its self-test; add real-browser header halt and setup POST keep-alive cases. Co-Authored-By: Claude Opus 5.5 --- tests/web/test_console_reads_are_read_only.py | 129 ++++++++++++------ tests/web/test_server.py | 75 ++++++++++ 2 files changed, 159 insertions(+), 45 deletions(-) diff --git a/tests/web/test_console_reads_are_read_only.py b/tests/web/test_console_reads_are_read_only.py index c10e9a02..ad5e405a 100644 --- a/tests/web/test_console_reads_are_read_only.py +++ b/tests/web/test_console_reads_are_read_only.py @@ -20,6 +20,7 @@ import hashlib import http.client import json +import re import sqlite3 from collections.abc import Iterator from pathlib import Path, PurePosixPath, PureWindowsPath @@ -353,10 +354,9 @@ def test_a_database_under_a_directory_with_awkward_characters_opens(tmp_path: Pa _REPO = Path(__file__).resolve().parents[2] -#: What the console's read path must never send to SQLite: each of these either undoes +#: What the console's read path must never send to SQLite (`_FORBIDDEN_SQL_RE`): each undoes #: `query_only` (it is per-connection and reversible by the connection itself), reaches another #: file, or writes the database file outside an ordinary statement. -_FORBIDDEN_SQL = ("query_only", "attach", "wal_checkpoint", "vacuum") def _console_read_path_files() -> list[Path]: @@ -392,73 +392,112 @@ def _is_write_only(file_name: str, function: str | None) -> bool: ) -def _execute_string_constants(tree: ast.AST) -> list[tuple[str | None, str]]: - """Every `.execute`/`.executemany`/`.executescript` call, as (enclosing function, SQL - string) -- one row per string constant inside its args, and one `(function, "")` row for a - call with none (so the call count is `len(result)` at least).""" - rows: list[tuple[str | None, str]] = [] +#: Case-insensitive; `attach` on a word boundary so `attachment; filename=` (a header) is not SQL. +_FORBIDDEN_SQL_RE = re.compile(r"query_only|wal_checkpoint|vacuum|\battach\b", re.IGNORECASE) + + +def _docstring_nodes(tree: ast.AST) -> set[int]: + """ids of the first-statement docstring constants of modules, classes and functions.""" + ids: set[int] = set() + for node in ast.walk(tree): + if isinstance(node, ast.Module | ast.ClassDef | ast.FunctionDef | ast.AsyncFunctionDef): + first = node.body[0] if node.body else None + if ( + isinstance(first, ast.Expr) + and isinstance(first.value, ast.Constant) + and isinstance(first.value.value, str) + ): + ids.add(id(first.value)) + return ids + + +def _forbidden_sql( + source: str, path_label: str +) -> tuple[list[tuple[str, str | None, str]], int, int]: + """The ONE filter the pin and its self-test share. Scans EVERY non-docstring string constant + (parts of f-strings included) -- not just literals written inside an `execute` call, since + SQL held in a variable or a module constant reaches `execute` all the same -- and returns + (offenders as (file, enclosing function, text), strings scanned, execute calls seen).""" + file_name = Path(path_label).name + tree = ast.parse(source) + docstrings = _docstring_nodes(tree) + offenders: list[tuple[str, str | None, str]] = [] + counts = {"strings": 0, "calls": 0} def visit(node: ast.AST, function: str | None) -> None: if isinstance(node, ast.FunctionDef | ast.AsyncFunctionDef): function = node.name + if _is_write_only(file_name, function): + return if ( isinstance(node, ast.Call) and isinstance(node.func, ast.Attribute) and node.func.attr in {"execute", "executemany", "executescript"} ): - strings = [ - n.value - for arg in node.args - for n in ast.walk(arg) - if isinstance(n, ast.Constant) and isinstance(n.value, str) - ] - rows.extend([(function, text) for text in strings] or [(function, "")]) + counts["calls"] += 1 + if ( + isinstance(node, ast.Constant) + and isinstance(node.value, str) + and id(node) not in docstrings + ): + counts["strings"] += 1 + text = node.value + if text not in _ALLOWED_SQL.get((file_name, function), set()) and ( + _FORBIDDEN_SQL_RE.search(text) + ): + offenders.append((file_name, function, text)) for child in ast.iter_child_nodes(node): visit(child, function) visit(tree, None) - return rows + return offenders, counts["strings"], counts["calls"] def test_the_console_read_path_never_asks_sqlite_to_lift_read_only() -> None: """**`query_only` is per-connection and the connection can switch it off; ATTACH and the checkpoint pragmas are not blocked by it.** Read-only therefore rests on SQLite refusing ordinary writes PLUS no console reader issuing those statements. This is the second half, - pinned structurally over the AST of every module on the console's read path.""" + pinned structurally over the AST of every module on the console's read path -- over every + string constant, so SQL held in a variable or constant is covered too (#977 round 3).""" files = _console_read_path_files() assert all(f.exists() for f in files), [str(f) for f in files if not f.exists()] - total = 0 + strings = calls = 0 offenders: list[tuple[str, str | None, str]] = [] for file in files: - for function, text in _execute_string_constants(ast.parse(file.read_text())): - if _is_write_only(file.name, function): - continue - total += 1 - if text in _ALLOWED_SQL.get((file.name, function), set()): - continue - if any(word in text.lower() for word in _FORBIDDEN_SQL): - offenders.append((file.name, function, text)) - - assert total >= 50, f"the walker found only {total} SQL calls: it would pass by vacuum" + found, n_strings, n_calls = _forbidden_sql(file.read_text(), str(file)) + offenders += found + strings += n_strings + calls += n_calls + + assert calls >= 50, f"the walker found only {calls} SQL calls: it would pass by vacuum" + assert strings >= 1500, f"only {strings} strings scanned: it would pass by vacuum" assert offenders == [] -def test_the_forbidden_sql_pin_sees_a_banned_statement_in_a_repository_method() -> None: - """The walker itself, on a planted source: an `OFF` pragma beside the one allowed `ON`, and - one in a Repository-shaped method, are both reported.""" - src = ( - "def connect_read_only(c):\n" - " c.execute('PRAGMA query_only = ON')\n" - " c.execute('PRAGMA query_only = OFF')\n" - "class R:\n" - " def read(self):\n" - " self.c.executemany('VACUUM', [])\n" - ) - rows = _execute_string_constants(ast.parse(src)) - flagged = [ - (f, t) - for f, t in rows - if t not in _ALLOWED_SQL.get(("db.py", f), set()) - and any(w in t.lower() for w in _FORBIDDEN_SQL) +def test_the_forbidden_sql_pin_flags_what_it_must_and_spares_what_it_must() -> None: + """The REAL filter on planted sources. Variable-held and constant-held SQL must be caught + (the call-literal-only walker this replaced missed both).""" + + def flagged(src: str, label: str = "repository.py") -> list[tuple[str | None, str]]: + return [(f, t) for _, f, t in _forbidden_sql(src, label)[0]] + + assert flagged( + "class R:\n def m(self):\n self.c.execute('PRAGMA query_only = OFF')\n" + ) == [("m", "PRAGMA query_only = OFF")] + assert flagged( + "def m(self):\n q = 'PRAGMA wal_checkpoint(TRUNCATE)'\n self.c.execute(q)\n" + ) == [("m", "PRAGMA wal_checkpoint(TRUNCATE)")] + assert flagged("_SQL = \"ATTACH DATABASE 'x' AS y\"\n") == [(None, "ATTACH DATABASE 'x' AS y")] + assert [t for _, t in flagged("def m(self, t):\n self.c.execute(f'VACUUM {t}')\n")] == [ + "VACUUM " ] - assert flagged == [("connect_read_only", "PRAGMA query_only = OFF"), ("read", "VACUUM")] + # Negatives. + on = "PRAGMA query_only = ON" + assert flagged(f"def connect_read_only(c):\n c.execute('{on}')\n", "db.py") == [] + assert flagged(f"def other(c):\n c.execute('{on}')\n", "db.py") == [("other", on)] + assert flagged("H = 'attachment; filename=x'\n", "server.py") == [] + assert flagged( + "def connect_read_only(c):\n c.execute('PRAGMA query_only = OFF')\n", "db.py" + ) == [("connect_read_only", "PRAGMA query_only = OFF")] + # Docstrings are prose, not SQL; the same words in a real string are not exempt. + assert flagged('def m():\n """Never VACUUM here."""\n') == [] diff --git a/tests/web/test_server.py b/tests/web/test_server.py index 516e306c..52aa8541 100644 --- a/tests/web/test_server.py +++ b/tests/web/test_server.py @@ -2364,6 +2364,81 @@ def test_a_halt_with_trustworthy_framing_keeps_the_connection_open( assert _statuses(received) == [b"200", b"200"], received[:300] +def _browser_headers(cfg, extra: list[tuple[str, str]]) -> list[tuple[str, str]]: + """What Chrome sends for the console's `fetch` (api.js `post`): its own headers + (`X-Keel-Client`, `Accept`, the token header, `Content-Type` only with a body) plus the ones + the browser adds.""" + return [ + ("Host", f"{cfg.host}:{cfg.port}"), + ("Connection", "keep-alive"), + ( + "User-Agent", + "Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) AppleWebKit/537.36 Chrome/130", + ), + ("Accept", "application/json"), + ("Accept-Language", "en-US,en;q=0.9,fr;q=0.8"), + ("Origin", f"http://{cfg.host}:{cfg.port}"), + ("Sec-Fetch-Site", "same-origin"), + ("Sec-Fetch-Mode", "cors"), + ("Sec-Fetch-Dest", "empty"), + ("Cookie", f"theme=dark; {_session(cfg)}; other=1"), + ("X-Keel-Client", "1"), + *extra, + ] + + +def _post_then_keepalive_get(cfg, path: str, headers: list[tuple[str, str]], body: bytes) -> bytes: + lines = [f"POST {path} HTTP/1.1", *[f"{n}: {v}" for n, v in headers]] + request = ("\r\n".join(lines) + "\r\n\r\n").encode() + body + request += ( + f"GET /api/config HTTP/1.1\r\nHost: {cfg.host}:{cfg.port}\r\n" + f"Cookie: {_session(cfg)}\r\nX-Keel-Client: 1\r\nConnection: close\r\n\r\n" + ).encode() + received = b"" + with socket.create_connection((cfg.host, cfg.port), timeout=3) as sock: + sock.sendall(request) + while chunk := sock.recv(65536): + received += chunk + return received + + +def test_a_real_browser_halt_post_keeps_the_connection_open(deployment) -> None: + """#977 round 3: the exact header set api.js sends for the halt (no body, so the browser adds + `Content-Length: 0` and no `Content-Type`) with the headers Chrome adds on top and a + multi-pair Cookie. Pins existing behaviour: answered, and the follow-up GET on the SAME + socket is answered too.""" + with _serving(deployment) as cfg: + headers = _browser_headers( + cfg, [(security.HALT_HEADER, security.halt_token(cfg.token)), ("Content-Length", "0")] + ) + received = _post_then_keepalive_get(cfg, "/api/gates/halt", headers, b"") + + assert _kill_switch(deployment[0]) is True + assert _statuses(received) == [b"200", b"200"], received[:300] + + +def test_a_real_browser_setup_post_keeps_the_connection_open(empty_machine) -> None: + """The same for a JSON setup write: `Content-Type: application/json`, a small body with its + true Content-Length and the real CSRF header. The write itself may be refused on its merits + (the body is not a valid config); what is pinned is that it is ANSWERED and the connection + stays usable.""" + cfg = empty_machine + body = json.dumps({"padding": "x"}).encode() + headers = _browser_headers( + cfg, + [ + ("X-Keel-CSRF", _csrf(cfg)), + ("Content-Type", "application/json"), + ("Content-Length", str(len(body))), + ], + ) + received = _post_then_keepalive_get(cfg, "/api/setup/config", headers, body) + + statuses = _statuses(received) + assert len(statuses) == 2 and statuses[1] == b"200", received[:300] + assert statuses[0] != b"ERR" and statuses[0] != b"501", received[:300] + + def test_a_halt_with_an_oversized_body_still_engages_and_closes(deployment) -> None: """A body past `_MAX_BODY_BYTES` is not drained (that is the memory-exhaustion primitive the cap denies) -- the connection is closed instead. The halt itself must still happen: refusing From 4222618ca3e2e41a73d1697429b74ac2a98edf80 Mon Sep 17 00:00:00 2001 From: Elmehdi Aitbrahim Date: Mon, 12 Oct 2026 13:24:41 -0400 Subject: [PATCH 10/11] server: say why multipart/message POSTs close after answering (review round 4) Co-Authored-By: Claude Opus 5.5 --- keel/web/server.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/keel/web/server.py b/keel/web/server.py index 4400f283..0520eb77 100644 --- a/keel/web/server.py +++ b/keel/web/server.py @@ -550,6 +550,9 @@ def _declared_body_length(self) -> int | None: # A header line the stdlib parser could not read (`Content-Length : 2`, a space before the # colon) ends the header parse early: the length never reaches `self.headers`, and what is # left sits in the payload. Absence of a length is then NOT "a body of zero". + # By design this also closes (after answering) a `multipart/*` or `message/*` POST, whose + # header block the stdlib parses as a MIME container. The console sends only JSON or no + # body (`static/js/api.js`), and no route here takes a multipart upload. if self.headers.defects or self.headers.get_payload(): self.close_connection = True return None From 64c3a682f55444762bdbf10a90b815c68996f3f0 Mon Sep 17 00:00:00 2001 From: Elmehdi Aitbrahim Date: Mon, 12 Oct 2026 13:44:58 -0400 Subject: [PATCH 11/11] test(web): pin that the halt answers before draining a body that never arrives (review round 5) A halt that drained or read its body first passed every test; this one declares 100 bytes, sends none, and needs the 200 and the engaged switch while the server is still waiting. Kills both mutants (drain-first, read-first). Co-Authored-By: Claude Opus 5.5 --- tests/web/test_server.py | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/tests/web/test_server.py b/tests/web/test_server.py index 52aa8541..3fc7d57a 100644 --- a/tests/web/test_server.py +++ b/tests/web/test_server.py @@ -2462,6 +2462,28 @@ def test_a_halt_with_an_oversized_body_still_engages_and_closes(deployment) -> N assert _kill_switch(deployment[0]) is True +def test_a_halt_whose_body_never_arrives_answers_before_the_drain(deployment) -> None: + """**The drain runs AFTER the answer** -- stopping must never be slower than starting (#955). + + The headers declare a 100-byte body that is never sent, and the socket stays open. The 200 + and the engaged switch must both arrive while the server is still waiting for those bytes: + a halt that drained (or read) first would sit on the socket until the client gave up, with + the switch still off. The response is read up to its declared length, never to EOF -- the + server is still draining, so EOF only comes when this test hangs up.""" + with _serving(deployment) as cfg: + lines = ["POST /api/gates/halt HTTP/1.1"] + lines += [f"{name}: {value}" for name, value in _halt_headers(cfg)] + lines += ["Content-Length: 100"] + with socket.create_connection((cfg.host, cfg.port), timeout=3) as sock: + sock.sendall(("\r\n".join(lines) + "\r\n\r\n").encode()) + response = http.client.HTTPResponse(sock) + response.begin() + body = response.read() + assert response.status == 200, body[:300] + assert json.loads(body)["data"]["step_key"] == "halt" + assert _kill_switch(deployment[0]) is True, "engaged while the body is still owed" + + # -- `serve()` keeps every field it was given (#971) -----------------------------------------------