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/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..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,61 @@ 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). + + **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, 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. + """ + 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: + 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 9ff2d739..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 @@ -330,6 +344,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 +920,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 +937,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 +1240,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/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/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/server.py b/keel/web/server.py index 256a0e56..0520eb77 100644 --- a/keel/web/server.py +++ b/keel/web/server.py @@ -46,12 +46,13 @@ from __future__ import annotations import json +import re import signal import socket 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 @@ -83,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) @@ -395,7 +399,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 -- @@ -527,6 +531,48 @@ 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. + """ + # 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 + 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 + # 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]) + def _drain_request_body(self) -> None: """Consume an unread request body, or close the connection rather than consume it. @@ -553,16 +599,17 @@ 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 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 +825,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 @@ -927,11 +994,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 @@ -1177,15 +1243,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/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_console_reads_are_read_only.py b/tests/web/test_console_reads_are_read_only.py new file mode 100644 index 00000000..ad5e405a --- /dev/null +++ b/tests/web/test_console_reads_are_read_only.py @@ -0,0 +1,503 @@ +"""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 ast +import hashlib +import http.client +import json +import re +import sqlite3 +from collections.abc import Iterator +from pathlib import Path, PurePosixPath, PureWindowsPath +from typing import Any + +import pytest + +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 +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. 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" + 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"] + + +def _mode_ro_opens(db_path: str) -> bool: + ro = sqlite3.connect(f"file:{db_path}?mode=ro", uri=True) + try: + 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: + 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 + + +# -- 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 (`_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. + + +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", + # #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", + ] + + +#: (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") + ) + + +#: 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"} + ): + 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 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 -- 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()] + strings = calls = 0 + offenders: list[tuple[str, str | None, str]] = [] + for file in files: + 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_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 " + ] + # 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_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")) diff --git a/tests/web/test_server.py b/tests/web/test_server.py index 1f280fc2..3fc7d57a 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 @@ -2147,3 +2160,476 @@ 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 + + +# 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", + ), + # #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" + ), +] + + +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 behind.""" + 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" + + +@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 _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 + a stop over its envelope is the one failure this route must not have. + + 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 + + +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) ----------------------------------------------- + + +@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 + 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( + 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.""" + _engage_kill_switch(deployment[0]) + 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 + 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