From ae2f67d077705f80d3b6564670b8822204c55335 Mon Sep 17 00:00:00 2001 From: Shashank Shekhar Singh Date: Tue, 4 Aug 2026 00:35:57 +0530 Subject: [PATCH] The /live auth check crashed on non-ASCII, and the index listed what the reader refuses MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two defects in `grapharc/server/live.py`, both of the same shape: one function raises a type its caller does not catch, or enforces a contract its sibling does not. **`_authorized` turned an unauthenticated guess into a 500.** `secrets.compare_digest` refuses `str` containing anything outside ASCII, and the raw query parameter went straight in, so a one-character request crashed the gate that exists to refuse strangers: curl "…/live?token=caf%C3%A9" -> 500 (traceback in the log) curl "…/live/api/runs?token=%C3%A9" -> 500 curl "…/live?token=wrong" -> 401 (ASCII, handled correctly) `_authorized` runs first on all four `/live` routes, so every one of them was reachable this way, and the 500-vs-401 split was itself an oracle: it told an unauthenticated caller something about how the token is compared. Both sides are encoded to UTF-8 before the comparison now. That removes the ASCII restriction entirely and keeps the constant-time property, which is the only reason `compare_digest` is used at all. A non-ASCII token now also *works* for its owner, which the old comparison could never have allowed. The same shape one function over: `resolve_trace` raises `ValueError` — not the `LivePathError` the route catches — when the name holds a NUL byte, so `?trace=%00.jsonl` was a 500 rather than a 404. `_resolved` catches both now; a malformed path is a 404 like every other one. **`scan_traces` published files the reader 404s.** It walked the root with `rglob("*.jsonl")`, which matches a symlinked *file* by name, then parsed it and put its name, size, mtime and run ids on `GET /live/api/runs` and the HTML index: ln -s ../OUTSIDE.jsonl liveroot/link_out.jsonl # run_id "SECRET-RUN" /live/api/runs -> {"trace":"link_out.jsonl", "runs":["SECRET-RUN"], …} /live/api/stream?trace=link_out.jsonl -> 404 # the reader refuses what the index advertised `resolve_trace`'s docstring says traversal "symlinks included, via resolve()" is refused, and the 404 is the proof of intent — the confinement simply was not applied on the listing path. The leakage is bounded (names, sizes, mtimes, run ids; never `state_delta`), but the live root is documented as the Slack bot's working directory, i.e. a place other things write, and run ids are exactly the input the rest of the API takes. `scan_traces` now routes every candidate through `resolve_trace` and skips symlinks outright — two checks for one contract, so a refactor of either cannot quietly reopen it. The reader's confinement is untouched: `../`, `%2e%2e%2f`, absolute paths, `sub/../../` and symlinked directories all still 404, verified against a running server as well as in the suite. Tests cover each: non-ASCII, oversized and empty tokens get 401 on every `/live` route (over the query string and over a bytes `Authorization` header, which starlette decodes latin-1 into a non-ASCII `str`); a valid non-ASCII token gets 200; a planted symlink — plus one in a subdirectory and a symlinked directory — appears in neither `/live/api/runs` nor the HTML index while the reader keeps 404ing it; a NUL byte is a 404. All four fail on main. Co-Authored-By: Claude Fable 5 --- CHANGELOG.md | 2 ++ grapharc/server/live.py | 31 ++++++++++++++++--- tests/test_server_live.py | 65 +++++++++++++++++++++++++++++++++++++++ 3 files changed, 93 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a3cad53..aa432cd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,3 +16,5 @@ Entries are newest-last within a release, matching the order they were written. - a bracket anywhere in a model's **prose** hijacked JSON extraction, because only the first `{`/`[` was ever tried. `Based on the context [lines 3-5]: {…}` was rejected as unparseable, and — worse — `Analysis (note [1]): {"supported": false}` returned a perfectly valid `[1]`, substituting a fabricated value for the verifier's actual answer. Every opener is tried now, and length alone turned out not to be a safe rank — a citation list like `[101, 205, 309, …]` *longer* than the verdict still won — so object spans are tried before array spans, each longest-first; junk still returns `None`, so fail-closed is unchanged. - a **bare backend name was read as a model name**, because `split_spec` only consulted the backend list when the spec contained a slash. `--model claude-cli` — the backend `models --check` reports as `usable` — shelled out to `claude -p --model claude-cli` and was refused by the CLI on *every* call, and `--model mock` named the paid subscription backend and spawned the real binary, so the double documented as "never reaches a provider" reached for one. A bare backend name now resolves to that backend (`claude-cli` to its own default model, `mock` to the scripted double, which ignores the model segment anyway); `openrouter`, `openai` and `ollama` front catalogues rather than a model, so those are refused with an example spelling instead of a guess about what to bill you for. The slash forms and bare *model* names are unchanged. - 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 `""`. +- 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: