diff --git a/CHANGELOG.md b/CHANGELOG.md index d274ccd..fe78119 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,3 +18,5 @@ Entries are newest-last within a release, matching the order they were written. - a failing `claude -p` **reported no reason at all**. The CLI exits non-zero with an empty stderr and puts its explanation in the JSON envelope on stdout; the gateway read only stderr, so the error was `claude -p exited 1: ` — a sentence that stops at the colon. Since the wrong-model bug above presented itself exactly that way, the two hid each other. stdout is read first now, and the recovered text also feeds the transient-vs-deterministic classifier, which was previously deciding from `""`. - **repeating `--registry` walked a Slack user straight past the agent opt-in.** The gate reads a flag's value to decide admission and argparse's `store` action then runs the *last* occurrence, but `_flag_value` returned the *first* — so `plan … --registry grapharc.examples.plan_docs:build_registry --registry grapharc.stdlib:build_registry` was judged against the demo registry and executed against the one that builds agent kinds on the host, with `GRAPHARC_SLACK_ALLOW_AGENT` never consulted and the forced `--approve` skipped in the same step. No privilege and no special knowledge needed: typing the flag twice was the whole exploit. Repeats of any admitted flag are refused outright now — the fail-closed reading, which retires the entire first-vs-last family rather than the one flag that exposed it — with a carve-out for the options the CLI itself accumulates (`agent --allow`/`--deny`, argparse `action="append"`), where every occurrence reaches the run and nothing can diverge. A duplicated `--model` is refused on the same rule, opted in or not, and `_flag_value` reads the last occurrence regardless, so the two readers can no longer disagree. A sweep over the whole allowlist asserts the duplicated form of every gated flag, so a future gate cannot reopen the gap. - a **NUL byte in a path came back as silence**, the worst answer a chat bot can give: `Path(raw).resolve()` raises `ValueError`, `handle_text_live` catches only `SlackCommandError`, so `trace a\x00b` escaped the bolt listener as an unhandled exception and the requester saw no reply at all — indistinguishable from the bot being down. A NUL anywhere in the request is now a refusal in the same voice the core tools already use ("cannot name a file"), and `_confined` turns any `ValueError`/`OSError` out of the filesystem into a refusal too, for callers of its own. Folded in from the same report: the flag allowlist tested `token.startswith("--")`, so a single-dash token slipped it and was spent as a positional — `trace -h` was admitted with `-h` as the path. Any leading dash is a flag now, and one not on the list is refused like any other. +- the `/live` **token check crashed on the strangers it exists to refuse**. `secrets.compare_digest` rejects `str` outside ASCII, and `_authorized` handed it the raw query parameter, so `?token=café` raised `TypeError` through the handler: an unauthenticated 500 with a traceback in the log on all four `/live` routes, where every ASCII guess correctly got a 401. The 500-vs-401 split was itself an oracle about how the token is compared. Both sides are encoded to UTF-8 now, which drops the ASCII restriction and keeps the constant-time comparison that is the whole reason `compare_digest` is there. A NUL byte in `?trace=` was the same shape one function over — `resolve_trace` raises `ValueError`, not the `LivePathError` the route caught — and is a 404 like any other malformed path now. +- the `/live` **index advertised traces the reader refuses to serve**. `scan_traces` walked the live root with `rglob("*.jsonl")`, which matches a symlinked file by name, then parsed it and published its name, size, mtime and **run ids** on `GET /live/api/runs` and the HTML index — for a file outside the root that `/live/api/stream` then 404s, the 404 being the proof of intent. One contract, two code paths, and only the reader enforced it; the live root is documented as the Slack bot's working directory, i.e. somewhere other things write. `scan_traces` routes every candidate through `resolve_trace` now and skips symlinks outright, so a refactor of either check cannot reopen the leak. The reader's confinement — `../`, `%2e%2e%2f`, absolute paths, `sub/../../`, symlinked directories — is unchanged. diff --git a/grapharc/server/live.py b/grapharc/server/live.py index 3bf3eae..b2d67a2 100644 --- a/grapharc/server/live.py +++ b/grapharc/server/live.py @@ -178,10 +178,21 @@ def scan_traces(root: Path) -> list[dict[str, Any]]: Run ids are parsed only for the `SCAN_PARSE_LIMIT` newest files; older rows carry an empty `runs` list — their viewer pages still work (the run is resolved from the file when the page opens). + + Confined exactly as `resolve_trace` confines the reader: `rglob` matches a + symlinked file by name, so without this the index advertised names, sizes + and parsed run ids for files the stream then 404s. Two checks for one + contract — the shared `resolve_trace` call, and an outright skip of + symlinks — so a refactor of either cannot quietly reopen the leak. """ found = [] for path in root.rglob("*.jsonl"): - if not path.is_file(): + if path.is_symlink() or not path.is_file(): + continue + try: + rel = path.relative_to(root).as_posix() + resolve_trace(root, rel) + except (ValueError, LivePathError): continue try: stat = path.stat() @@ -189,7 +200,7 @@ def scan_traces(root: Path) -> list[dict[str, Any]]: continue found.append( { - "trace": path.relative_to(root).as_posix(), + "trace": rel, "size": stat.st_size, "mtime": stat.st_mtime, "runs": [], @@ -229,14 +240,24 @@ def _authorized(request: Request) -> None: header = request.headers.get("authorization", "") if header.startswith("Bearer "): supplied = supplied or header.removeprefix("Bearer ") - if supplied is None or not secrets.compare_digest(supplied, token): + # Compared as bytes: `compare_digest` refuses `str` outside ASCII, so + # comparing text turned a one-character guess into a 500 — the gate + # crashing on the strangers it exists to refuse. Encoding keeps the + # constant-time property, which is the reason it is here at all. + if supplied is None or not secrets.compare_digest( + supplied.encode("utf-8"), token.encode("utf-8") + ): raise HTTPException(status_code=401, detail="missing or wrong token") def _resolved(raw: str) -> str: - """Validate confinement; 404 on refusal (don't map what exists outside).""" + """Validate confinement; 404 on refusal (don't map what exists outside). + + `ValueError` too: a NUL byte in the name reaches the filesystem call + inside `resolve()`, and a malformed request is a 404 like any other. + """ try: resolve_trace(root_path, raw) - except LivePathError: + except (LivePathError, ValueError): raise HTTPException(status_code=404, detail="no such trace") from None return raw diff --git a/tests/test_server_live.py b/tests/test_server_live.py index 74f306c..7c04de4 100644 --- a/tests/test_server_live.py +++ b/tests/test_server_live.py @@ -271,6 +271,71 @@ def test_a_token_locks_every_live_route(tmp_path): assert client.get("/live/api/runs?token=wrong").status_code == 401 +def test_a_hostile_token_is_a_401_not_a_crash(tmp_path): + """The gate that refuses strangers must not be crashable by one. + + `secrets.compare_digest` refuses `str` outside ASCII, so a one-character + guess used to raise `TypeError` through the handler — a 500 that both + amplifies the log and tells the caller how the token is compared. + """ + write_run(tmp_path / "t.jsonl", "r1", done=True) + routes = ("/live", "/live/api/runs", "/live/view?trace=t.jsonl", + "/live/api/stream?trace=t.jsonl") + guesses = ("caf%C3%A9", "%C3%A9", "%F0%9F%94%91", "x" * 9000, "") + with live_client(tmp_path, token="s3cret") as client: + for route in routes: + sep = "&" if "?" in route else "?" + for guess in guesses: + response = client.get(f"{route}{sep}token={guess}") + assert response.status_code == 401, (route, guess) + # Also over the header, where the wire is bytes: starlette decodes + # them latin-1, so non-ASCII arrives as a non-ASCII `str` too. + assert client.get( + route, headers={"authorization": "Bearer café".encode()} + ).status_code == 401 + assert client.get(f"{route}{sep}token=s3cret").status_code == 200 + + +def test_a_non_ascii_token_still_authorizes_its_owner(tmp_path): + """Bytes comparison must widen what is accepted, not only what is refused.""" + with live_client(tmp_path, token="café-🔑") as client: + assert client.get("/live/api/runs?token=caf%C3%A9-%F0%9F%94%91").status_code == 200 + assert client.get("/live/api/runs?token=caf%C3%A9").status_code == 401 + + +def test_a_nul_byte_in_the_trace_is_404_not_500(tmp_path): + """`resolve_trace` raises `ValueError`, not `LivePathError`, on a NUL byte.""" + with live_client(tmp_path) as client: + for raw in ("%00.jsonl", "sub/%00/t.jsonl", "t%00.jsonl"): + assert client.get(f"/live/view?trace={raw}").status_code == 404 + assert client.get(f"/live/api/stream?trace={raw}").status_code == 404 + + +def test_the_index_hides_a_symlinked_trace_outside_the_root(tmp_path): + """The index must advertise only what the reader will serve.""" + secret = tmp_path / "OUTSIDE.jsonl" + write_run(secret, "SECRET-RUN", done=True) + root = tmp_path / "liveroot" + root.mkdir() + write_run(root / "run1.jsonl", "r1", done=True) + (root / "link_out.jsonl").symlink_to(secret) + (root / "sub").mkdir() + (root / "sub" / "link_out.jsonl").symlink_to(secret) + (root / "linkdir").symlink_to(tmp_path) # a symlinked *directory* too + + assert [t["trace"] for t in scan_traces(root)] == ["run1.jsonl"] + + with live_client(root) as client: + listed = client.get("/live/api/runs").json()["traces"] + assert [t["trace"] for t in listed] == ["run1.jsonl"] + page = client.get("/live").text + for leak in ("SECRET-RUN", "link_out.jsonl", "OUTSIDE.jsonl"): + assert leak not in page + # And the index still agrees with the reader, which refuses the link. + assert client.get("/live/api/stream?trace=link_out.jsonl").status_code == 404 + assert client.get("/live/view?trace=link_out.jsonl").status_code == 404 + + def test_the_index_lists_traces_and_links_the_viewer(tmp_path): write_run(tmp_path / "runs" / "t.jsonl", "r1") with live_client(tmp_path) as client: