amadan
Say what the CSP sweep actually covers, measured rather than reasoned
The code was right; the prose describing it was wrong for the fourth round running, and in both directions this time. It claimed the three passkey ceremony endpoints "still do not run here". They do. Their 403 is written by passkey.Handlers.refuse from INSIDE the handler, after the pending half-session lookup misses — so a header set at the top of one is observable. Verified directly: wrapping /passkey/signin/begin to set an unsafe-inline header before calling the real handler fails the sweep, naming that route. With the session cookie the sweep now reaches and runs all twenty-three targets. Probe-verified, not inferred from status codes — inferring from status codes is exactly how the previous commit got this wrong in the other direction, because a 303 can mean either "RequireFresh refused" or "the handler succeeded". It also claimed TestNoHandlerSetsItsOwnCSP "covers those by reading source". It does not: those handlers live in rastrillo's passkey package, and that test globs *.go in THIS package, so it never reads them. The runtime sweep is what covers them. The source guard's real justification is a route added later and not yet swept, plus a code path the sweep's one canned request does not take. And the before-state was overstated: five handlers never ran, not ten, all five stopped by RequireFresh and none by csrf — so the sweep reached eighteen of twenty-three, not thirteen. "The gap landed exactly on the passkey screens" was also too strong: of the five, only the enrolment screen is one of the two ruling 21 lets load script. Two smaller admissions recorded where they belong: the source grep is evadable by building the header key with concatenation (acceptable — the sweep reaches every route), and dumpDatabase sees no schema changes or pragmas and orders by column 1, which is deterministic only because every current table's first column is its primary key.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
internal/home/noscript_test.go+39 −28internal/home/testapp_test.go+12 −0
CI passed — run details
Intent
<task-notification> <task-id>af83c692dca252f93</task-id> <tool-use-id>toolu_01Fo8q9jTB2TJ6DdQJDFzF2X</tool-use-id> <output-file>/home/paulca/.cache/tmp/claude-1001/-home-paulca-amadan-net-oficina-home/d63f5ac7-6b42-48c5-a13d-34e6faf8e44c/tasks/af83c692dca252f93.output</output-file> <status>completed</status> <summary>Agent "Last check on the final commit" finished</summary> <note>A task-notification fires each time this agent stops with no live background children of its own. The user can send it another message and resume it, so the same task-id may notify more than once.</note> <result>Review of `4d6f923` complete. All work done in `$TMPDIR` copies made with `git archive`; the worktree was not touched. ## 1. CSP sweep now carries a session — FIXED (mechanism), claim about coverage is FALSE All three mutants (`w.Header().Set("Content-Security-Policy", "…'unsafe-inline'")` at the top of the handler) compile clean (`go vet` OK) and now fail **both** tests: | mutant | `TestServedCSPAllowsNoInlineScript` | `TestNoHandlerSetsItsOwnCSP` | |---|---|---| | `handlePasskeyEnrol` | FAIL `/passkey/enrol` | FAIL `passkey.go` | | `handleRecoveryCodesPage` | FAIL `/passkey/codes` | FAIL `passkey.go` | | `handleRegenerateRecoveryCodes` | FAIL `/passkey/codes` | FAIL `passkey.go` | **How many of the 23 targets reach a handler: 23 of 23.** I did not infer this from status codes — I built the tree with a `replace` onto a local copy of rastrillo v0.22.0, set `X-Reached: <funcname>` as the first statement of every `*App.handle*` and every `passkey.Handlers` ceremony method, and logged it in the sweep. Every one of the 23 targets came back with a non-empty `X-Reached` (the four `/static/*`, `/healthz` and `/` are closures/`handleIndex` with a `_ *http.Request` signature the probe regex skipped; all returned 200, so they plainly ran). **The comment's list is neither true nor complete.** `noscript_test.go:264-267` says the three `signin/*` ceremony endpoints "still do not run here". They do run. Their 403 is written by `h.refuse(w)` *inside* the handler, after `h.pendingFrom(r)` misses — `passkey/passkey.go:255-266` (`SignInBegin`), `:275-280` (`SignInFinish`), `recovery.go:114` (`SignInRecovery`), `refuse` at `:589`. Nothing short-circuits ahead of them; `csrf.Protect` passes on every POST in the sweep. A CSP header set at the top of those handlers would be caught by the runtime sweep today. The second half of the sentence is false too: `TestNoHandlerSetsItsOwnCSP` does **not** cover those three. It globs `*.go` in `internal/home`; those routes bind `a.passkeys.SignInBegin/Finish/Recovery`, which live in rastrillo's `passkey` package and are never read by the glob. Same claim repeated verbatim at `noscript_test.go:403-406` and in the commit message. The honest justification for the source guard is routes added later, and handler code paths the sweep's canned request does not take — not "three endpoints the sweep cannot reach". ## 2. `TestNoHandlerSetsItsOwnCSP` — OK, with a real but low-cost evasion It does fail on any non-test `internal/home/*.go` mentioning the string (proven three times above), and `checked == 0` is guarded. The grep is **not** sound on its own. `w.Header()["Content-Security-Policy"] = …` still contains the literal, so that one is caught. But this compiles and evades it: ```go const hdr = "Content-Security" + "-Policy" w.Header()[hdr] = []string{"default-src 'self'; script-src 'self' 'unsafe-inline'"} ``` Verified: `TestNoHandlerSetsItsOwnCSP` **passes**, `TestServedCSPAllowsNoInlineScript` **fails**. `textproto.CanonicalMIMEHeaderKey` on a split constant would do the same. Given the runtime sweep reaches 23/23 routes, this barely matters — the grep is the belt, the sweep is the braces, and the braces hold. NIT only. Not covered by either test: a `<meta http-equiv="Content-Security-Policy">` in `templates/*.html`. `noscript_test.go` scans templates for `<script`, never for `http-equiv`. ## 3. `dumpDatabase` — FIXED The `UPDATE signin_links SET sealed_address = X'00'` mutant in `handleVerifyForm` now fails `TestVerifyPageWritesNothing`, with a readable before/after diff. Eight tables discovered from `sqlite_master`; empty tables emit a `== name` header, so an insert into one is visible — verified with an `INSERT INTO people` mutant (CAUGHT). Answering the specific questions: - **`ORDER BY 1` stability:** stable for every table *today*, and only by luck — the first column is the PRIMARY KEY in all eight (`people.person_ref`, `signin_links.token_hash`, `sessions.token_hash`, `passkey_credentials.id`, `passkey_challenges.challenge`, `passkey_pending.token_hash`, `passkey_recovery_codes.code_hash`, `rastrillo_migrations.id`). A future table whose first column ties gives an order SQLite does not guarantee — that is a flake risk, not a missed write. - **`sqlite_sequence`/internal tables:** `type='table'` would include `sqlite_sequence` if it existed; no schema uses AUTOINCREMENT, so it does not. Views, indexes and triggers are excluded. - **WAL:** no gap. The dump reads through `app.db.Writer()`, so WAL content is visible. **What it still misses** (each proven with a mutant in `handleVerifyForm`): - `CREATE INDEX IF NOT EXISTS zzz_probe ON people(created_at)` — MISSED (only `name` is read from `sqlite_master`, never `sql`; no DDL change is visible). - `PRAGMA user_version = 42` — MISSED. - Delete-every-row-and-reinsert-identical on `signin_links` (new rowids, rewritten WAL pages, `secure_delete` zeroing) — MISSED. `SELECT *` omits the rowid. None of these are the class of bug the test exists to catch. Worth one sentence in the comment; not worth code. ## 4. Session cookie in the sweep — OK, no pollution `newTestApp(t)` is per-test, so nothing leaks across tests. Within the sweep, the POST to `/passkey/codes` does not rotate the session — `/passkey/register/begin` still returns 200 afterwards. - `go test -race -count=2 ./internal/home/` → `ok 13.210s` - `go test -count=1 -shuffle=on ./internal/home/` ×2 → `ok 3.773s`, `ok 3.768s` - `go test -count=1 ./...` → all six packages ok ## 5. Prose Verified **correct**: - `keymail_test.go:91` — "so this browser will re-send it" no longer asserts replayability. Clean. - `signin.go` `handleCallback`, three render paths — accurate. `completeSignIn` has exactly three `a.signinFailed(w)` calls: `findOrMintPerson` (`signin.go:299`), `passkeys.Gate` error (`:323`), `sessions.SignIn` (`:332`). The `gated == true` branch is a redirect (`passkey/passkey.go:214`), not a render, so it correctly is not counted. - `passkey.go:74-77` history — accurate. `903eb7b` wrote "rather than the codes page's five" while `codesFreshness = 5 * time.Minute` (true when written); `d39fc86` redefined it to `enrolFreshness`; the sentence then stood false through `d39fc86` and `099582e` — two commits — until `3634208` corrected it. "True when written and stood falsely for two commits after" checks out. - `noscript_test.go:255-257` — "all ten POST handlers were skipped": there are exactly 10 POST routes. The commit message's "seven POST-only paths" is also exactly right. - `noscript_test.go:61-63` srcAttr — `\b` does match inside `data-src=` (`-` non-word, `s` word). Accurate. Verified **false**: - `noscript_test.go:264-267` and `:403-406` — the unreachable-routes claim. See item 1. Two places, plus the commit message. - `noscript_test.go:258-260` — "eight POSTs still 403'd on **csrf**/session gates … so **ten** of twenty-three handlers still never ran". I ran the same `X-Reached` probe against parent `013331d`. **Five** handlers never ran, not ten: `GET/POST /passkey/codes`, `GET /passkey/enrol`, `POST /passkey/register/begin`, `POST /passkey/register/finish` — all five short-circuited by `RequireFresh`. The other five 403s (`signin/begin`, `signin/finish`, `signin/recovery`, `stepup/begin`, `stepup/finish`) all reported `X-Reached`; they ran and refused from inside. So the sweep reached **18 of 23**, not 13. And **no** POST was stopped by csrf — `Origin` is set, as the comment ten lines below says itself. ## Findings **BLOCKING — 1.** `internal/home/noscript_test.go:264-267` and `:403-406`: the claim that `signin/begin`, `signin/finish` and `signin/recovery` "still do not run here" is false (probe-verified: all three enter their handler), and the claim that `TestNoHandlerSetsItsOwnCSP` "covers those by reading source" is false (they are in rastrillo's `passkey` package; the `*.go` glob never sees it). The commit message repeats both. This is the exact failure mode of the last three rounds: the fix landed, the sentence describing it did not. The sweep now reaches every route — say that, and justify the source guard on routes added later plus code paths the canned request misses. **BLOCKING — 2.** `internal/home/noscript_test.go:258-260`: "eight POSTs still 403'd on csrf/session gates … ten of twenty-three handlers still never ran" and the commit message's "reached 13 of 23". The true before-state is 18 of 23 reached, 5 never run, all five stopped by `RequireFresh`, none by csrf. **NIT.** `noscript_test.go:261-263` / commit message: "the gap landed exactly on the passkey screens, which are the ones ruling 21 lets load script." `scriptAllowed` names `passkey_confirm.html` and `passkey_enrol.html`. `passkey_confirm` was always reachable; two of the three gap handlers render `recovery_codes.html`, which loads no script. Only `enrol` is in both sets. "Exactly" overstates. **NIT.** `testapp_test.go` `dumpDatabase`: `ORDER BY 1` is deterministic only because every current table's first column is its primary key — worth naming, since the comment's whole argument is that a snapshot must not depend on the schema staying as it is. Blind spots worth one sentence: DDL, pragmas, rowids. **NIT.** The source grep is defeated by string concatenation (proven). Fine as defence in depth; not worth hardening. ## CHANGES NEEDED The code is right — all three CSP mutants die, the DB mutant dies, and the suite is clean under `-race -count=2` and `-shuffle=on`. What is wrong is prose, in the same way as the previous three rounds: two comments assert a coverage gap that the commit itself closed, and one of them names a test that demonstrably does not cover what it claims. Three sentence edits in `noscript_test.go` (lines 258-267, 403-406) and the corresponding commit-message paragraph, and this is done.</result> <usage><subagent_tokens>98320</subagent_tokens><tool_uses>46</tool_uses><duration_ms>466415</duration_ms></usage> </task-notification>
diff --git a/internal/home/noscript_test.go b/internal/home/noscript_test.go| index 84264f9..d463d20 100644 |
| --- a/internal/home/noscript_test.go |
| +++ b/internal/home/noscript_test.go |
| @@ -248,31 +248,29 @@ func TestServedCSPAllowsNoInlineScript(t *testing.T) { |
| if !ok { |
| t.Fatalf("Router() is %T, which does not implement chi.Routes", app.Router()) |
| } |
| - // Method AND route, AND a session. Getting here took three goes and |
| - // the history is the useful part: |
| + // Method AND route, AND a session. It took three goes, and the |
| + // history is the useful part because each version's comment claimed |
| + // more coverage than it had. |
| // |
| // It first issued GET on every path, so all ten POST handlers were |
| // skipped and an unsafe-inline header in handleSigninBegin survived |
| - // the whole suite. Using chi.Walk's method fixed that — and the |
| - // comment then claimed it was "the whole fix", which was wrong |
| - // again: eight POSTs still 403'd on csrf/session gates and two GETs |
| - // 303'd on RequireFresh, so ten of twenty-three handlers still |
| - // never ran. The gap landed exactly on the passkey screens, which |
| - // are the ones ruling 21 lets load script and therefore the |
| - // tempting place to widen a CSP. |
| + // the whole suite. Using chi.Walk's method fixed that, and the |
| + // comment then called it "the whole fix" — wrong again: five |
| + // handlers still never ran, all five stopped by RequireFresh |
| + // (GET and POST /passkey/codes, GET /passkey/enrol, and both |
| + // register endpoints). None was stopped by csrf; the Origin header |
| + // below sees to that. So the sweep reached eighteen of |
| + // twenty-three, and one of the five it missed renders the enrolment |
| + // screen, which is one of the two ruling 21 lets load script. |
| // |
| - // So: a fresh session cookie as well. Measured after that change, |
| - // twenty of the twenty-three targets reach their handler and run it |
| - // to completion. The three that do not are the ceremony endpoints — |
| - // signin/begin, signin/finish, signin/recovery — which want a |
| - // pending half-session rather than a session, and refuse with a 403 |
| - // before doing anything. Precisely: they DO run, they just return |
| - // early, so a header set after that point would not be observed |
| - // here. |
| - // |
| - // That is why TestNoHandlerSetsItsOwnCSP exists alongside this one: |
| - // it reads source, so it covers those three, anything they might set |
| - // before refusing, and any route added later. |
| + // So: a fresh session cookie as well. With it, all twenty-three |
| + // targets reach and run their handler — probe-verified rather than |
| + // inferred from status codes, which is how an earlier version of |
| + // this comment got it wrong in the other direction. The three |
| + // ceremony endpoints answer 403, but that 403 is written by |
| + // passkey.Handlers.refuse from INSIDE the handler after the pending |
| + // half-session lookup misses, so a header set at the top of one |
| + // would be caught here. |
| type target struct{ method, path string } |
| var targets []target |
| if err := chi.Walk(routes, func(method string, route string, _ http.Handler, _ ...func(http.Handler) http.Handler) error { |
| @@ -295,7 +293,8 @@ func TestServedCSPAllowsNoInlineScript(t *testing.T) { |
| } |
| // A real, fresh session: the passkey routes want one, and the codes |
| - // and enrol pages want a recent one. |
| + // and enrol pages want a recent one. Without it this sweep silently |
| + // covered eighteen routes while claiming twenty-three. |
| cookie := signInFully(t, app, "sweeper@example.invalid") |
| for _, tg := range targets { |
| @@ -405,12 +404,24 @@ func TestVendoredAssetsMatchTheLibrary(t *testing.T) { |
| // No handler in this package may set its own Content-Security-Policy. |
| // |
| // rastrillo's securityHeaders sets the default on every response, and a |
| -// handler's own header REPLACES it — that is the one move that turns |
| -// ruling 21's carve-out into a blanket exemption, and it would show up |
| -// in no rendered page. TestServedCSPAllowsNoInlineScript catches it on |
| -// every route it can actually reach; three ceremony endpoints need a |
| -// pending half-session and it cannot. This reads source, so it covers |
| -// all of them, including any route added later. |
| +// handler's own header REPLACES it — the one move that turns ruling |
| +// 21's carve-out into a blanket exemption, and it would show up in no |
| +// rendered page. |
| +// |
| +// What this adds over TestServedCSPAllowsNoInlineScript, which does now |
| +// reach all twenty-three routes: a route added later and not yet swept, |
| +// and a code path inside a handler that the sweep's one canned request |
| +// does not take — a branch behind a form value, an error path. |
| +// |
| +// What it does NOT add, contrary to an earlier version of this comment: |
| +// coverage of the three passkey ceremony endpoints. Those handlers live |
| +// in rastrillo's passkey package, and this globs *.go in THIS package, |
| +// so it never reads them. The runtime sweep is what covers those. |
| +// |
| +// It is also evadable by anyone trying: a header key built by |
| +// concatenation does not contain the literal string. That is acceptable |
| +// — this is the belt and the sweep is the braces, and the braces reach |
| +// every route. |
| func TestNoHandlerSetsItsOwnCSP(t *testing.T) { |
| files, err := filepath.Glob("*.go") |
| if err != nil { |
diff --git a/internal/home/testapp_test.go b/internal/home/testapp_test.go| index 6d98dde..89b0cf1 100644 |
| --- a/internal/home/testapp_test.go |
| +++ b/internal/home/testapp_test.go |
| @@ -164,6 +164,18 @@ func grepPackage(t *testing.T, needle string) []string { |
| // an insert into any unlisted table, and any in-place update to a |
| // counted one were all invisible. A snapshot that has to be kept in |
| // step with the schema is a snapshot that will not be. |
| +// |
| +// Two things it still does not see, stated so a future reader does not |
| +// assume otherwise: schema changes (only `name` is read from |
| +// sqlite_master, never `sql`, so a CREATE INDEX is invisible) and |
| +// pragmas such as user_version. Neither is the class of write this |
| +// exists to catch, which is a handler quietly touching a row. |
| +// |
| +// And ORDER BY 1 is deterministic here only because every current |
| +// table's first column is its primary key. A future table whose first |
| +// column can tie would make this flaky rather than wrong — worth |
| +// knowing, given the whole argument above is that a snapshot must not |
| +// depend on the schema staying as it is. |
| func dumpDatabase(t *testing.T, app *testApp) string { |
| t.Helper() |
| var tables []string |