diff --git a/keel/cli.py b/keel/cli.py index 2f2ddc4..5add21d 100644 --- a/keel/cli.py +++ b/keel/cli.py @@ -161,6 +161,7 @@ from keel.commands.journal import journal_group from keel.commands.mcp import mcp_cmd from keel.commands.monitor import run_monitor +from keel.commands.open_console import open_cmd from keel.commands.orders import orders_cmd from keel.commands.pnl import build_pnl_report, render_pnl_report from keel.commands.posture import posture_group @@ -1446,6 +1447,9 @@ def simulate( # dashboard's autonomy action and the console's three gated actions went. Each of those four # mirrored a CLI command that is still here. cli.add_command(serve_cmd) +# #756: the way back to a console nobody watched start. See `keel/web/runtime.py` for why a +# detached server may leave its token on disk and an interactive one may not. +cli.add_command(open_cmd) # -- mcp (the read-only research-assistant surface over stdio) ----------------------------------- diff --git a/keel/commands/open_console.py b/keel/commands/open_console.py new file mode 100644 index 0000000..539f2ab --- /dev/null +++ b/keel/commands/open_console.py @@ -0,0 +1,86 @@ +"""`keel open` -- reach a console that is already running (#756). + +`keel serve` prints its URL once, at startup, with the session token in it. That is enough when a +human is watching the terminal and useless when nothing is: under `launchd` the line goes to +`StandardOutPath`, and the way back into your own console becomes `grep`-ing a log for a token. + +So a detached `serve` records how to reach itself (`keel/web/runtime.py`, which carries the +argument for why that is acceptable and how it is bounded), and this reads that record back. + +**It mints nothing and it extends nothing.** The token it prints is the one the running server +already minted; stopping keel still revokes it, and this command has no way to bring it back. + +**It refuses rather than guesses.** Three different absences look identical from here -- no server, +a server started from a terminal, and a server that was killed -- and each gets its own sentence, +because "not found" would send an operator hunting for a bug in the two cases where nothing is +wrong. +""" + +from __future__ import annotations + +import webbrowser + +import click + +from keel.commands.serve import DEFAULT_PORT +from keel.web import runtime + + +@click.command("open") +@click.option( + "--port", + default=DEFAULT_PORT, + show_default=True, + type=int, + help="Which console. One `keel serve` process per port; see `keel serve --port`.", +) +@click.option( + "--no-browser", + is_flag=True, + default=False, + help="Print the address without launching a browser.", +) +def open_cmd(port: int, no_browser: bool) -> None: + """Print (and open) the address of a running `keel serve`, token included.""" + record = runtime.live_record(port) + if record is None: + _refuse(port) + return + + url = runtime.url_for(record) + click.echo(f"Opening the keel console at:\n\n {url}\n") + click.echo("This address carries the running server's session token. Stopping keel revokes it.") + + if no_browser: + return + try: + # Best-effort, exactly as `keel serve` treats its own launch: the URL is already printed, + # so a headless machine, a broken BROWSER variable or a sandbox with no launcher costs the + # operator nothing they cannot recover by pasting. + webbrowser.open(url) + except Exception as exc: # noqa: BLE001 -- any launcher failure is survivable here + click.echo(f"(could not launch a browser: {exc})") + + +def _refuse(port: int) -> None: + """Say which of the three absences this is, and what to do about each. + + A stale record is reported as a STOPPED server rather than as no server: the file is evidence + that one ran, and telling an operator "nothing is serving" when a crashed process left its + record behind hides the thing they most need to know. + """ + stale = runtime.read_record(port) + if stale is not None: + raise click.ClickException( + f"a keel server was recorded on port {port} but its process is gone -- it crashed or " + "was killed. Its token died with it; start a new one with `keel serve` (or " + "`launchctl kickstart` the agent that runs it)." + ) + raise click.ClickException( + f"no recorded keel server on port {port}.\n\n" + " If one is running, it was started from a terminal -- an interactive `keel serve` " + "deliberately records nothing, and its URL is printed in that terminal. Only a detached " + "server (launchd, or any run whose stdout is not a terminal) leaves a record here, " + "because that is the case where nobody can read the printed line.\n\n" + f" If none is running, start one: `keel serve --port {port}`." + ) diff --git a/keel/web/runtime.py b/keel/web/runtime.py new file mode 100644 index 0000000..d3774e3 --- /dev/null +++ b/keel/web/runtime.py @@ -0,0 +1,275 @@ +"""Where a running `keel serve` says how to reach it, so `keel open` can answer (#756). + +── WHY THIS EXISTS AT ALL, GIVEN WHAT `security.py` SAYS ────────────────────────────────────── + +Layer 3 of `keel/web/security.py` rests on the session token being ephemeral: *"the token is +minted per process and never written to disk, so closing the server destroys it and every +outstanding cookie becomes a string that authenticates nothing."* `serve` prints that sentence to +the operator on every run. + +This module writes the token to disk, so it is bounded by one rule and the rule is the whole +design: + + THE RECORD EXISTS ONLY WHEN THE URL IS NOT BEING SHOWN TO A HUMAN. + +`sys.stdout.isatty()` decides. Attached to a terminal, nothing here runs and the posture is +byte-for-byte what it was; `serve` still prints the sentence, and the sentence is still true. + +Detached -- `launchd`, a pipe, a container -- the token is going to a file ANYWAY, because that is +what `StandardOutPath` is. An operator's only route back into their own console then is to `grep` +a log for a URL. So the choice at that point is not "token on disk or not"; it is "token in a log +file at the daemon's umask, or token in a `0600` file in a `0700` directory that is deleted on +shutdown and whose staleness is checked before it is offered". The second is strictly less +exposure than the first, and it is the only one that makes `keel serve` usable unattended. + +── WHAT THIS IS NOT ─────────────────────────────────────────────────────────────────────────── + +**Not an authority on whether a server is running.** A `SIGKILL` or a power cut leaves the file +behind, so `live_record` re-checks the pid before offering anything. The file is a hint. + +**Not a way to reissue or extend a token.** Nothing here mints; it records what `serve` already +minted, and `forget` drops it. Stopping keel still revokes every outstanding cookie, which is +the operator's revocation gesture and is unchanged. + +**Not a lock.** It does not arbitrate who may bind a port -- the OS already refuses a second +bind, and a lock file that could disagree with the kernel would be a second answer to a question +that already has one. +""" + +from __future__ import annotations + +import json +import os +import socket +import sys +import time +from pathlib import Path +from typing import Any + +from keel_core.paths import state_root + +#: The directory holding one record per serving port. Under the deployment root, beside the +#: database rather than in a global `/tmp`: two deployments on one machine are the ordinary case +#: (#756 counts four), and their consoles must not read each other's records. +RUN_DIR_NAME = "run" + +#: `0700` on the directory and `0600` on the file. Group and world get nothing: this holds a live +#: bearer token, and on a shared machine a readable one would be worse than the log line it +#: replaces. Both are asserted by `tests/web/test_runtime_record.py`. +DIR_MODE = 0o700 +FILE_MODE = 0o600 + +#: How long to wait for the recorded port to answer. Loopback, so a healthy server answers +#: immediately; anything slower is a machine in trouble worth reporting rather than waiting on. +PROBE_TIMEOUT_SECONDS = 0.5 + + +def run_dir(*, create: bool = False) -> Path | None: + """The run directory, or `None` when there is no deployment to put one in. + + `parents=False` deliberately (#759 review). `state_root`'s own contract is that "it never + creates a deployment folder, because a deployment folder that does not exist is not one this + function chose", and `mkdir(parents=True)` reached straight past it -- serving on a machine + with no deployment brought one into existence as a side effect. `server.ensure_schema` refuses + the identical hazard one directory over ("a read-only view would bring a deployment into + existence merely by being started"), and a first-run `keel serve` with no deployment is a + supported state, not an error. + + So: record into a deployment that exists, and record nothing into one that does not. The + operator on that path is being shown the setup page and has no console to reopen yet. + """ + root = state_root() + directory = root / RUN_DIR_NAME + if create: + if not root.is_dir(): + return None + directory.mkdir(parents=False, exist_ok=True) + # Set explicitly rather than trusting the umask: `mkdir`'s mode is masked, and a + # deployment running under a permissive umask would otherwise get a group-readable + # directory holding session tokens. + directory.chmod(DIR_MODE) + return directory + + +def record_path(port: int) -> Path: + """One file per port. The PORT names it and the token lives inside. + + Directory listings leak names, and a filename is readable by anything that can list the + directory even when the file itself is not. Pinned by + `test_the_token_is_never_in_the_filename`. + """ + directory = run_dir() + assert directory is not None # `create=False` always answers a path + return directory / f"serve-{int(port)}.json" + + +def record_serving(*, host: str, port: int, token: str, interactive: bool) -> Path | None: + """Record how to reach this server, unless a human is watching stdout. + + `interactive` is the caller's `sys.stdout.isatty()` -- passed in rather than read here so the + decision is visible at the call site in `serve`, where the operator-facing sentence about it + is also printed, and so a test can exercise both sides without touching the process's own + streams. + + Returns the path written, or `None` when the rule above says to write nothing. + """ + if interactive: + return None + directory = run_dir(create=True) + if directory is None: + return None + path = directory / f"serve-{int(port)}.json" + # Written through a per-port temporary file and then renamed, so a reader can never see a + # half-written record: `os.replace` is atomic within a directory. The temporary carries the + # same `0600`, because it holds the same token for the moment it exists. + staging = directory / f".serve-{int(port)}.json.tmp" + body = json.dumps( + { + "pid": os.getpid(), + "host": host, + "port": int(port), + "token": token, + "started_ts": int(time.time()), + } + ) + # CREATED at `0600`, not corrected to it (#759 review). `Path.write_text` creates at the + # process umask -- measured `0644` under the usual `022` -- and the `chmod` that followed left + # the token world-readable for the window between the two calls. The `0700` directory meant no + # other account could traverse in, so it was never exploitable; it was also an incidental + # mitigation for the one file whose entire purpose is holding a secret, and the fix is one + # argument to `os.open`. `O_EXCL` refuses a pre-existing path, so a symlink planted at the + # staging name cannot redirect the write either. + descriptor = os.open(staging, os.O_CREAT | os.O_EXCL | os.O_WRONLY, FILE_MODE) + try: + with os.fdopen(descriptor, "w") as handle: + handle.write(body) + except BaseException: + # A half-written staging file must not be left behind to collide with the next `O_EXCL`. + staging.unlink(missing_ok=True) + raise + os.replace(staging, path) + return path + + +def read_record(port: int) -> dict[str, Any] | None: + """The record for `port`, or `None` if there is none or it cannot be read. + + Deliberately does NOT check liveness -- `live_record` does, and keeping them apart means a + caller diagnosing a stale file can still see it. A corrupt or truncated file (a crash + mid-write) reads as absent rather than raising: `keel open` must not traceback at an operator + whose server has just died. + """ + path = record_path(port) + try: + raw = path.read_text() + except OSError: + return None + try: + parsed = json.loads(raw) + except ValueError: + return None + if not isinstance(parsed, dict) or not parsed.get("token"): + return None + return parsed + + +def _process_alive(pid: int) -> bool: + """Is `pid` a process this user could signal? + + `signal 0` is the standard existence check: it validates the pid and permissions without + delivering anything. `pid <= 0` is refused before the call, because `os.kill(0, 0)` signals + the whole process GROUP -- which on a bad record would be this process, and would report the + stale record as live. + """ + if pid <= 0: + return False + try: + os.kill(pid, 0) + except ProcessLookupError: + return False + except PermissionError: + # It exists and belongs to someone else. Not ours to open, and not ours to claim is gone. + return True + except OSError: + return False + return True + + +def _port_answers(host: str, port: int) -> bool: + """Can a loopback TCP connection be made to `host:port` right now? + + A connect, not a request: this must not send the token anywhere, and "something is bound" is + the whole question. `create_connection` resolves the family, so an IPv6 record needs no special + case -- and it takes the UNBRACKETED host, unlike `url_for`, because brackets are a URL + spelling rather than part of an address. + + A short timeout on purpose. This runs against loopback, where a healthy answer is immediate; + anything slower is a machine in trouble, and `keel open` should say so rather than hang. + """ + if not host: + return False + try: + with socket.create_connection((host, int(port)), timeout=PROBE_TIMEOUT_SECONDS): + return True + except OSError: + return False + + +def live_record(port: int) -> dict[str, Any] | None: + """The record for `port`, but only if the process that wrote it is still around. + + `forget` covers the clean exit. This covers every other kind: a `SIGKILL`, a panic, a power + cut. Without it `keel open` would hand an operator a URL that refuses them, which reads as + keel being broken rather than as keel being stopped. + """ + record = read_record(port) + if record is None: + return None + try: + pid = int(record.get("pid", 0)) + except TypeError, ValueError: + return None + if not _process_alive(pid): + return None + # AND something must answer on the port (#759 review). A pid check alone is not liveness: keel + # dies, the OS hands that pid to anything else, and the record reads as live -- so `keel open` + # prints a token the server no longer honours and the browser gets a 403, which is the "keel is + # broken rather than stopped" confusion this check exists to prevent. Both checks, because + # neither is sufficient alone: a listening port could belong to another program, and a live pid + # could be a recycled one. + return record if _port_answers(str(record.get("host", "")), port) else None + + +def forget(port: int) -> None: + """Drop the record. Called from `serve`'s `finally`, so a clean stop leaves nothing behind.""" + try: + record_path(port).unlink() + except OSError: + # Already gone, or a directory that never existed because the run was interactive. Both + # are the desired end state, and neither is worth failing a shutdown over. + pass + + +def url_for(record: dict[str, Any]) -> str: + """The address to open, rebuilt from a record. + + The bracketing rule for IPv6 is `ServeConfig.url`'s, restated because this side has no + `ServeConfig` to ask -- `keel open` reads a file written by a process that has since become + unreachable. `test_an_ipv6_host_is_bracketed_like_serve_prints_it` holds the two spellings + to the same output. + """ + host = str(record.get("host", "")) + bracketed = f"[{host}]" if ":" in host else host + return f"http://{bracketed}:{int(record.get('port', 0))}/?token={record.get('token', '')}" + + +def stdout_is_interactive() -> bool: + """Whether a human is watching this process's stdout. + + Wrapped rather than called inline so `serve` reads as the rule it implements, and because a + detached stream can raise on `isatty` rather than answering. + """ + try: + return bool(sys.stdout.isatty()) + except AttributeError, ValueError: + return False diff --git a/keel/web/security.py b/keel/web/security.py index cf6c1b1..d4047e4 100644 --- a/keel/web/security.py +++ b/keel/web/security.py @@ -21,9 +21,16 @@ user is already viewing. `keel serve` mints it per run, prints it in the URL, and the browser exchanges it for a `SameSite=Strict` cookie on first load. `Strict` (not `Lax`) is deliberate: `Lax` attaches the cookie to top-level navigations, so a link on a hostile page would arrive - authenticated. Nothing is persisted **on this side of the wire** -- the token is minted per - process and never written to disk, so closing the server destroys it and every outstanding - cookie becomes a string that authenticates nothing. Since #634 the cookie carries a `Max-Age` + authenticated. The token is minted per process, so closing the server destroys it and every + outstanding cookie becomes a string that authenticates nothing. + + **Whether it is written to disk depends on who is watching, and #756 is why.** Attached to a + terminal, nothing is persisted on this side of the wire -- unchanged. Detached (`launchd`, a + pipe), `keel/web/runtime.py` records the address in a `0600` file so `keel open` can hand it + back; read its module docstring before touching that, because the argument is not "it is fine + to persist a token" but "on that path the token is already in `StandardOutPath` at the daemon's + umask, and a mode-`0600` file deleted on shutdown is strictly less exposure than the log line + that would otherwise be the only way in". Since #634 the cookie carries a `Max-Age` so the BROWSER stops throwing away a token that is still valid; `SESSION_COOKIE_MAX_AGE_SECONDS` carries the whole argument for why that extends convenience and not authority. diff --git a/keel/web/server.py b/keel/web/server.py index 9169ce9..d7ad499 100644 --- a/keel/web/server.py +++ b/keel/web/server.py @@ -51,7 +51,7 @@ from typing import Any from urllib.parse import parse_qs, urlsplit -from keel.web import api, events, staticfiles +from keel.web import api, events, runtime, staticfiles from keel.web.security import ( CSRF_HEADER, REMOTE_SESSION_MAX_AGE_SECONDS, @@ -988,7 +988,21 @@ def serve(cfg: ServeConfig, *, echo: Callable[[str], None] = print) -> int: # way to sign out. Said here rather than only in a docstring, because the operator who needs # to revoke is the operator reading this terminal, and the gesture is the one they already # have: stopping keel invalidates the token, so every browser holding it is out. - echo("Stopping keel revokes it: the token is new every run and is never written to disk.") + # #756. The sentence an operator is owed depends on which of these two runs this is, and + # printing the wrong one would be a false safety assurance about a live credential -- the + # class of thing `payload._session_banner` refuses to do about mode. + interactive = runtime.stdout_is_interactive() + recorded = runtime.record_serving( + host=running.host, port=running.port, token=running.token, interactive=interactive + ) + if recorded is None: + echo("Stopping keel revokes it: the token is new every run and is never written to disk.") + else: + echo("Stopping keel revokes it: the token is new every run.") + echo( + f"Nothing is reading this output, so the address is also in {recorded} (0600) -- " + f"run `keel open --port {running.port}` to get it back. It is deleted on shutdown." + ) try: server.serve_forever() @@ -996,6 +1010,10 @@ def serve(cfg: ServeConfig, *, echo: Callable[[str], None] = print) -> int: echo("") echo("stopped.") finally: + # Before `server_close`, so a record never outlives the port it names by more than the + # instant between these two lines. A crash skips this entirely, which is what + # `runtime.live_record`'s pid check is for. + runtime.forget(running.port) server.server_close() return 0 diff --git a/tests/web/test_open_command.py b/tests/web/test_open_command.py new file mode 100644 index 0000000..af3b00f --- /dev/null +++ b/tests/web/test_open_command.py @@ -0,0 +1,248 @@ +"""`keel open`, and what `keel serve` has to leave behind for it to work (#756). + +Under `launchd` nobody reads stdout, so the URL carrying the session token lands in a log file and +an operator's only way back into their own console is `grep`. These tests hold both halves of the +fix: that `serve` records how to reach itself when — and only when — no human is watching, and +that `open` refuses clearly in every case where it cannot help. +""" + +from __future__ import annotations + +import os +from pathlib import Path + +import pytest +from click.testing import CliRunner + +from keel.cli import cli +from keel.web import runtime +from keel.web import server as web_server +from keel.web.security import new_session_token + + +@pytest.fixture() +def home(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> Path: + monkeypatch.setenv("KEEL_HOME", str(tmp_path)) + return tmp_path + + +@pytest.fixture() +def listening() -> object: + """A port with something really bound to it, yielded as an int. + + `live_record` now probes the port, so every test that expects `keel open` to SUCCEED needs a + real listener. Three of them used a bare 8765 and passed on the author's machine only because + a real `keel serve` happened to be running there -- they would have failed in CI, which is the + same class of environment-dependence as a fixture that reaches the network. + + Backlog room for several probes: nothing here ever `accept()`s, and a backlog of 1 refuses the + second connection. + """ + import socket + + sock = socket.socket() + sock.bind(("127.0.0.1", 0)) + sock.listen(8) + try: + yield int(sock.getsockname()[1]) + finally: + sock.close() + + +class _StubServer: + def __init__(self, address: tuple[str, int]) -> None: + self.server_address = address + self.RequestHandlerClass = type("H", (), {"cfg": None}) + self.closed = False + + def serve_forever(self) -> None: + raise KeyboardInterrupt + + def server_close(self) -> None: + self.closed = True + + +def _serve(monkeypatch: pytest.MonkeyPatch, home: Path, *, interactive: bool) -> list[str]: + stub = _StubServer(("127.0.0.1", 8765)) + monkeypatch.setattr(web_server, "ensure_schema", lambda _path: None) + monkeypatch.setattr(web_server, "build_server", lambda _cfg: stub) + monkeypatch.setattr(runtime, "stdout_is_interactive", lambda: interactive) + lines: list[str] = [] + cfg = web_server.ServeConfig( + host="127.0.0.1", + port=8765, + token=new_session_token(), + db_path=str(home / "keel.db"), + config_path=str(home / "config.yaml"), + ) + assert web_server.serve(cfg, echo=lines.append) == 0 + return lines + + +# -- what serve leaves behind ------------------------------------------------------------------- + + +def test_an_interactive_serve_writes_nothing_and_says_the_token_never_lands( + monkeypatch: pytest.MonkeyPatch, home: Path +) -> None: + """The posture `security.py` describes, unchanged, and the sentence still true.""" + lines = _serve(monkeypatch, home, interactive=True) + assert not (home / "run").exists() + assert any("never written to disk" in line for line in lines), lines + + +def test_a_detached_serve_records_itself_and_says_so( + monkeypatch: pytest.MonkeyPatch, home: Path +) -> None: + """And it must NOT print the never-written sentence, which would then be false. + + The operator is owed the difference: on this path the token is on disk, and a line claiming + otherwise is exactly the sort of false safety assurance `_session_banner` exists to refuse. + """ + lines = _serve(monkeypatch, home, interactive=False) + assert not any("never written to disk" in line for line in lines), lines + assert any("keel open" in line for line in lines), lines + + +def test_a_clean_stop_removes_the_record(monkeypatch: pytest.MonkeyPatch, home: Path) -> None: + """`serve`'s stub raises `KeyboardInterrupt` from `serve_forever`, which is the Ctrl-C path.""" + _serve(monkeypatch, home, interactive=False) + assert runtime.read_record(8765) is None, "the record outlived the server that wrote it" + + +# -- the command -------------------------------------------------------------------------------- + + +def test_open_is_registered_and_documents_itself() -> None: + result = CliRunner().invoke(cli, ["open", "--help"]) + assert result.exit_code == 0 + assert "--port" in result.output + + +def test_open_prints_the_url_when_a_server_is_recorded(home: Path, listening: int) -> None: + runtime.record_serving(host="127.0.0.1", port=listening, token="tok", interactive=False) + result = CliRunner().invoke(cli, ["open", "--port", str(listening), "--no-browser"]) + assert result.exit_code == 0, result.output + assert f"http://127.0.0.1:{listening}/?token=tok" in result.output + + +def test_open_refuses_when_nothing_is_recorded(home: Path) -> None: + """Non-zero, because a script that pipes this into a browser must be able to tell.""" + port = _a_closed_port() + result = CliRunner().invoke(cli, ["open", "--port", str(port), "--no-browser"]) + assert result.exit_code != 0 + assert str(port) in result.output + + +def test_open_explains_the_interactive_case_rather_than_just_failing(home: Path) -> None: + """The likeliest confusion this command will cause: a server IS running, started from a + terminal, and deliberately left no record. "not found" would send the operator looking for a + bug; naming the reason sends them to the terminal that has the URL.""" + result = CliRunner().invoke(cli, ["open", "--port", str(_a_closed_port()), "--no-browser"]) + assert "terminal" in result.output.lower(), result.output + + +def test_open_does_not_offer_a_url_for_a_process_that_has_gone(home: Path) -> None: + """A `SIGKILL` leaves the file. Handing over a URL that refuses the browser would read as keel + being broken rather than as keel being stopped.""" + # A value no English sentence contains. `"tok"` was the first choice and it matched the word + # "token" in the refusal itself -- a substring assertion failing on prose, which is the same + # trap in the other direction from a substring assertion PASSING on prose. + secret = "Z9-stale-secret-Z9" + port = _a_closed_port() + runtime.record_serving(host="127.0.0.1", port=port, token=secret, interactive=False) + path = runtime.record_path(port) + path.write_text(path.read_text().replace('"pid": ' + str(os.getpid()), '"pid": 0')) + result = CliRunner().invoke(cli, ["open", "--port", str(port), "--no-browser"]) + assert result.exit_code != 0 + assert secret not in result.output, "a stale token was printed anyway" + + +def test_open_launches_a_browser_by_default_and_can_be_told_not_to( + home: Path, monkeypatch: pytest.MonkeyPatch, listening: int +) -> None: + """Symmetric with `keel serve --no-open`: the URL is printed either way, so a machine with no + launcher loses nothing.""" + runtime.record_serving(host="127.0.0.1", port=listening, token="tok", interactive=False) + opened: list[str] = [] + import keel.commands.open_console as open_mod + + monkeypatch.setattr(open_mod.webbrowser, "open", lambda url: opened.append(url) or True) + + CliRunner().invoke(cli, ["open", "--port", str(listening), "--no-browser"]) + assert opened == [] + + CliRunner().invoke(cli, ["open", "--port", str(listening)]) + assert opened == [f"http://127.0.0.1:{listening}/?token=tok"] + + +def test_a_browser_that_will_not_launch_does_not_fail_the_command( + home: Path, monkeypatch: pytest.MonkeyPatch, listening: int +) -> None: + """`serve` treats the launch as best-effort for the same reason: the URL is already printed, + and typing it in is a complete fallback.""" + runtime.record_serving(host="127.0.0.1", port=listening, token="tok", interactive=False) + import keel.commands.open_console as open_mod + + def _boom(_url: str) -> bool: + raise RuntimeError("no browser here") + + monkeypatch.setattr(open_mod.webbrowser, "open", _boom) + result = CliRunner().invoke(cli, ["open", "--port", str(listening)]) + assert result.exit_code == 0, result.output + assert f"http://127.0.0.1:{listening}/?token=tok" in result.output + + +# -- review findings (#759) ---------------------------------------------------------------------- + + +def _a_closed_port() -> int: + """A port nothing is listening on: bound to get a free number, then released. + + NOT a hard-coded 8765. The first cut used it and failed on the author's own machine, where a + real `keel serve` was running -- a test that passes only where the feature is unused is worse + than no test. + """ + import socket + + probe = socket.socket() + probe.bind(("127.0.0.1", 0)) + port = int(probe.getsockname()[1]) + probe.close() + return port + + +def test_a_recycled_pid_does_not_make_a_dead_server_look_alive(home: Path) -> None: + """A pid check alone is not liveness. + + keel dies, the OS hands its pid to something else, and the record reads as live -- so `open` + prints a token the server no longer honours and the browser gets a 403. That is exactly the + "keel is broken rather than stopped" confusion the check exists to prevent, and the original + spec asked for a process active ON THE DESIGNATED PORT. + + This record's pid is THIS process, which is certainly alive and certainly not serving. + """ + port = _a_closed_port() + runtime.record_serving(host="127.0.0.1", port=port, token="tok", interactive=False) + assert runtime.live_record(port) is None, "a live pid was accepted with nothing on the port" + + +def test_a_server_that_is_actually_listening_is_offered(home: Path) -> None: + """The other half. A port check that refused everything would pass the test above and break + the feature outright, so the same record is accepted once something is really bound.""" + import socket + + listener = socket.socket() + listener.bind(("127.0.0.1", 0)) + # Backlog room for more than one probe: `live_record` connects once per call and this test + # calls it twice, and nothing here ever `accept()`s, so a backlog of 1 refused the second. + listener.listen(8) + port = int(listener.getsockname()[1]) + try: + runtime.record_serving(host="127.0.0.1", port=port, token="tok", interactive=False) + assert runtime.live_record(port) is not None + result = CliRunner().invoke(cli, ["open", "--port", str(port), "--no-browser"]) + assert result.exit_code == 0, result.output + assert "token=tok" in result.output + finally: + listener.close() diff --git a/tests/web/test_runtime_record.py b/tests/web/test_runtime_record.py new file mode 100644 index 0000000..fd035fa --- /dev/null +++ b/tests/web/test_runtime_record.py @@ -0,0 +1,169 @@ +"""The serve runtime record: a live session token on disk, and the rule that bounds it (#756). + +`keel serve` mints a session token per process and prints it in a URL. Under `launchd` nobody +reads stdout, so that URL -- token and all -- lands in `StandardOutPath`, a log file at whatever +umask the daemon runs under. An operator's only way back into their own console becomes `grep`. + +`keel open` fixes that, and the only way it can is by the server leaving the token somewhere +readable. That is a real weakening of layer 3 in `keel/web/security.py` ("the token is minted per +process and never written to disk"), so it is bounded by one rule, which these tests are about: + + THE RECORD EXISTS ONLY WHEN THE URL IS NOT BEING SHOWN TO A HUMAN. + +`sys.stdout.isatty()` is the test. Attached to a terminal, the operator already has the URL and +nothing is written -- today's posture, byte for byte. Not attached, the token is already going +somewhere persistent (a log, a pipe), and a `0600` file in a `0700` directory that is deleted on +shutdown is strictly *less* exposure than the log line that would otherwise be the only way in. +""" + +from __future__ import annotations + +import json +import os +import stat +from pathlib import Path + +import pytest + +from keel.web import runtime + + +@pytest.fixture() +def home(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> Path: + monkeypatch.setenv("KEEL_HOME", str(tmp_path)) + return tmp_path + + +def test_nothing_is_written_when_stdout_is_a_terminal(home: Path) -> None: + """The whole bound on this feature. An interactive `keel serve` keeps the posture its own + output claims -- and the sentence it prints stays true.""" + runtime.record_serving(host="127.0.0.1", port=8765, token="tok", interactive=True) + assert runtime.read_record(8765) is None + assert not (home / "run").exists(), "an interactive run created the runtime directory" + + +def test_the_record_is_written_when_nobody_is_reading_stdout(home: Path) -> None: + runtime.record_serving(host="127.0.0.1", port=8765, token="tok", interactive=False) + record = runtime.read_record(8765) + assert record is not None + assert record["token"] == "tok" + assert record["port"] == 8765 + assert record["pid"] == os.getpid() + + +def test_the_file_is_unreadable_by_anyone_else(home: Path) -> None: + """`0600` in a `0700` directory. It holds a live bearer token; group or world read would put + it within reach of every account on a shared machine, which is worse than the log line this + exists to replace.""" + runtime.record_serving(host="127.0.0.1", port=8765, token="tok", interactive=False) + path = runtime.record_path(8765) + assert stat.S_IMODE(path.stat().st_mode) == 0o600, oct(path.stat().st_mode) + assert stat.S_IMODE(path.parent.stat().st_mode) == 0o700, oct(path.parent.stat().st_mode) + + +def test_the_record_is_removed_on_shutdown(home: Path) -> None: + """A token that outlives its process authenticates nothing, but it still READS as a way in, + and an operator following a stale record would be told to open a URL that refuses them.""" + runtime.record_serving(host="127.0.0.1", port=8765, token="tok", interactive=False) + runtime.forget(8765) + assert runtime.read_record(8765) is None + + +def test_a_record_whose_process_is_gone_is_not_offered(home: Path) -> None: + """Removal on shutdown covers the clean exit. A `SIGKILL`, a panic or a power cut leaves the + file behind, so liveness is checked on the way OUT as well -- the file is a hint, never the + authority on whether a server is running.""" + runtime.record_serving(host="127.0.0.1", port=8765, token="tok", interactive=False) + path = runtime.record_path(8765) + stale = json.loads(path.read_text()) + # A pid this process can prove is not serving: its own parent's parent is not `keel serve`, + # and pid 0 is never a real process to signal. + stale["pid"] = 0 + path.write_text(json.dumps(stale)) + assert runtime.live_record(8765) is None + assert runtime.read_record(8765) is not None, "read_record must not silently drop a stale file" + + +def test_a_corrupt_record_is_ignored_rather_than_raising(home: Path) -> None: + """A truncated write (a crash mid-flush) must not make `keel open` traceback at an operator + who is already having a bad day.""" + runtime.record_serving(host="127.0.0.1", port=8765, token="tok", interactive=False) + runtime.record_path(8765).write_text("{not json") + assert runtime.read_record(8765) is None + + +def test_the_token_is_never_in_the_filename(home: Path) -> None: + """Directory listings leak names. The port identifies the record; the secret is inside.""" + runtime.record_serving(host="127.0.0.1", port=8765, token="s3cret-token", interactive=False) + assert "s3cret-token" not in str(runtime.record_path(8765)) + assert "s3cret-token" not in "".join(p.name for p in runtime.record_path(8765).parent.iterdir()) + + +def test_records_are_per_port_so_several_deployments_coexist(home: Path) -> None: + """The point of #756: four deployments, four consoles, four ports. One shared file would make + the last server to start the only one reachable.""" + runtime.record_serving(host="127.0.0.1", port=8765, token="live", interactive=False) + runtime.record_serving(host="127.0.0.1", port=8766, token="paper", interactive=False) + live = runtime.read_record(8765) + paper = runtime.read_record(8766) + assert live is not None and paper is not None + assert live["token"] == "live" + assert paper["token"] == "paper" + assert runtime.record_path(8765) != runtime.record_path(8766) + + +def test_the_url_is_rebuilt_from_the_record_and_carries_the_token(home: Path) -> None: + runtime.record_serving(host="127.0.0.1", port=8765, token="tok", interactive=False) + record = runtime.read_record(8765) + assert record is not None + assert runtime.url_for(record) == "http://127.0.0.1:8765/?token=tok" + + +def test_an_ipv6_host_is_bracketed_like_serve_prints_it(home: Path) -> None: + """`ServeConfig.url` brackets it; a second spelling of one rule is a second chance to get it + wrong, so this asserts the same shape.""" + runtime.record_serving(host="::1", port=8765, token="tok", interactive=False) + record = runtime.read_record(8765) + assert record is not None + assert runtime.url_for(record) == "http://[::1]:8765/?token=tok" + + +# -- review findings (#759) ---------------------------------------------------------------------- + + +def test_the_file_is_created_at_0600_rather_than_corrected_afterwards( + home: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """`write_text` then `chmod` leaves the token world-readable for the window between them. + + Measured under `umask 022`, a `write_text` file is born `0644`. The `0700` directory means no + other account can traverse in, so the window was not exploitable -- but that is an incidental + mitigation, and creating a secret file at the mode it needs is one argument to `os.open`. + CodeQL points at this same line. + + THE TEST WORKS BY REMOVING THE SAFETY NET: with `chmod` neutered, a file that is merely + corrected afterwards shows its umask mode, and one that is created correctly still shows + `0600`. + """ + monkeypatch.setattr(os, "chmod", lambda *args, **kwargs: None) + monkeypatch.setattr(os, "umask", lambda mask: 0o022) + runtime.record_serving(host="127.0.0.1", port=8765, token="tok", interactive=False) + mode = stat.S_IMODE(runtime.record_path(8765).stat().st_mode) + assert mode == 0o600, f"created at {oct(mode)}; the mode must not depend on a later chmod" + + +def test_recording_never_brings_a_deployment_root_into_existence( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """`state_root`: "it never creates a deployment folder, because a deployment folder that does + not exist is not one this function chose." + + `mkdir(parents=True)` did exactly that. It is the same hazard `server.ensure_schema` refuses + one directory over -- "a read-only view would bring a deployment into existence merely by + being started" -- and a first-run `keel serve` on a machine with no deployment is a supported + state, not an error. + """ + missing = tmp_path / "no-such-deployment" + monkeypatch.setenv("KEEL_HOME", str(missing)) + assert runtime.record_serving(host="127.0.0.1", port=8765, token="t", interactive=False) is None + assert not missing.exists(), "serving created a deployment root as a side effect"