The /live auth check crashed on non-ASCII, and the index listed what the reader refuses - #74
Merged
Merged
Conversation
…the reader refuses
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 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two defects in
grapharc/server/live.py, both the same shape: one function raises a type its caller does not catch, or enforces a contract its sibling does not.#65 — the token check crashed on the strangers it exists to refuse
secrets.compare_digestrejectsstrcontaining anything outside ASCII, and_authorizedhanded it the raw query parameter. A one-character request therefore raisedTypeErrorstraight through the handler — an unauthenticated 500, with a traceback in the log, on all four/liveroutes (_authorizedruns first on every one). 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 drops the ASCII restriction entirely and keeps the constant-time property, which is the only reason
compare_digestis there. A non-ASCII token now also works for its owner — something the old comparison could never have allowed.Verified against a real server (
grapharc serve --live-root liveroot --live-token s3cr3t --port 8734):/live?token=caf%C3%A9/live/api/runs?token=%C3%A9/live?token=wrong/live?token=s3cr3t#69 — the index advertised traces the reader refuses to serve
scan_traceswalked the root withrglob("*.jsonl"), which matches a symlinked file by name, then parsed it and published its name, size, mtime and run ids onGET /live/api/runsand the/liveHTML index — for a fileresolve_traceexplicitly 404s. One contract, two code paths, and only the reader enforced it; the 404 is the proof of intent, andresolve_trace's docstring already says traversal "symlinks included, viaresolve()" is refused.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_tracesnow routes every candidate throughresolve_traceand skips symlinks outright — two checks for one contract, so a refactor of either cannot quietly reopen it.With
ln -s ../OUTSIDE.jsonl liveroot/link_out.jsonl(holding run idSECRET-RUN):Also fixed: a NUL byte in
?trace=was a 500The same shape one function over.
resolve_traceraisesValueError— not theLivePathError_resolvedcatches — when the name holds a NUL byte, so?trace=%00.jsonlreturned 500 instead of 404._resolvedcatches both now; a malformed path is a 404 like every other one. Verified:/live/api/stream?trace=%00.jsonl&token=…500 → 404.Confinement is not weakened
Re-checked against the running server and in the suite — all still 404, before and after:
../OUTSIDE.jsonl,%2e%2e%2fOUTSIDE.jsonl,/etc/passwd,sub/../../OUTSIDE.jsonl, a symlinked directory, and non-.jsonlnames.Tests
Four new tests in
tests/test_server_live.py, all of which fail onmain:test_a_hostile_token_is_a_401_not_a_crash— non-ASCII (café,é, an emoji), a 9,000-character token and an empty one each get 401 on all four/liveroutes, over the query string and over a bytesAuthorizationheader (starlette decodes headers latin-1, so non-ASCII arrives as a non-ASCIIstrthere too); the valid token still gets 200.test_a_non_ascii_token_still_authorizes_its_owner— the bytes comparison widens what is accepted, not only what is refused.test_the_index_hides_a_symlinked_trace_outside_the_root— a planted symlink, one in a subdirectory, and a symlinked directory appear in neitherscan_traces,/live/api/runs, nor the HTML index, while the reader keeps 404ing them.test_a_nul_byte_in_the_trace_is_404_not_500.Status
1779 passed(full suite,pytest),ruff check .clean. No flaky failures observed on this run — the SIGALRM timing tests passed too.Fixes #65
Fixes #69
🤖 Generated with Claude Code