amadan
Answer the final review: the sweep now reaches the routes it claimed to
Two blocking items, and the first is worth naming for what it is. ## The retracted claim, re-asserted inside the function that retracts it keymail_test.go's failure message read "the callback did not clear the pending cookie, so the blob is replayable". That "so" is exactly the causal claim the comment thirteen lines below it spends a paragraph retracting — the clearing is hygiene, the authorization code is what is single use. Third time this branch has shipped a retraction and the claim it retracts in the same file. Now: "so this browser will re-send it". ## The CSP sweep reached 13 of 23 handlers while claiming "every route" Using chi.Walk's method fixed the GET-only problem and I called it "the whole fix". It was half. Eight POSTs still 403'd on session gates and two GETs 303'd on RequireFresh, so ten handlers never ran — and 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. Three mutants survived there: handleRegenerateRecoveryCodes, handleRecoveryCodesPage, handlePasskeyEnrol. Two changes, because neither alone is enough: - The sweep now carries a real fresh session cookie, so the gated routes execute. All three mutants now fail. - TestNoHandlerSetsItsOwnCSP reads source instead, so it covers the three ceremony endpoints that need a pending half-session and cannot be driven here — and any route added later. Each mutant now fails BOTH tests. The comment says all of that, including which routes the runtime sweep still cannot reach and why the source guard exists. ## The no-state snapshot could not see most writes TestVerifyPageWritesNothing read three columns of one table and row counts of three others. A write to sealed_address, an insert into any of the four unlisted tables, and any in-place update to a counted one were all invisible — proven by a surviving mutant that rewrote every sealed_address to a null byte. dumpDatabase now renders every row of every column of every table, discovered from sqlite_master rather than listed. A snapshot that has to be kept in step with the schema is a snapshot that will not be. The mutant now fails. ## Prose - handleCallback's sentence, on its third revision, listed two of the three paths that render as though that were all of them; the passkey Gate's error path was missing. It now says three, and records what each earlier version got wrong. - "the eight POST-only handlers" matched neither the ten POST routes nor the seven POST-only paths. - "true for two commits and then was not" inverted its own history: it was true when written and stood falsely for two commits AFTER codesFreshness was redefined. - srcAttr's comment explained the quoting alternatives and not the data-src= fix that was the actual change.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
internal/home/keymail_test.go+1 −1internal/home/noscript_test.go+61 −5internal/home/passkey.go+2 −1internal/home/signin.go+9 −6internal/home/signin_test.go+2 −31internal/home/testapp_test.go+69 −0
CI passed — run details
Intent
<task-notification> <task-id>a8faec5bd7e8c4936</task-id> <tool-use-id>toolu_01AdRzkiYQSHUPxyKNaamQGM</tool-use-id> <output-file>/home/paulca/.cache/tmp/claude-1001/-home-paulca-amadan-net-oficina-home/d63f5ac7-6b42-48c5-a13d-34e6faf8e44c/tasks/a8faec5bd7e8c4936.output</output-file> <status>completed</status> <summary>Agent "Final check before merge" 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>All mutants compiled before running. Full results below. --- ## 1. A — `enrolFreshness` prose — **FIXED** `internal/home/passkey.go:69-77`. The "codes page's five" clause is gone. `grep -ri '\bfive\b|5 \* time.Minute'` over `internal/` returns nothing stale — the only surviving "5" is `passkey_test.go:378`'s historical note ("enrolFreshness was 10 minutes and codesFreshness 5"), correctly past-tense. `codesFreshness = enrolFreshness = 10 * time.Minute`, and every sentence in both comments now matches. One NIT below on the new sentence's commit count. ## 2. B — `takePending` — **FIXED, but the retracted claim still ships elsewhere** `signin.go:356-366` now agrees with `pendingCookie` (`signin.go:45-53`) and with the code: clearing is hygiene, the authorization code is what is single-use. I grepped the whole package for repeats of every retracted claim on this branch (single-use/one callback, "cleared on read", SameSite=Lax as a defence, "never renders"). One hit: **BLOCKING —** `internal/home/keymail_test.go:91`: ```go t.Error("the callback did not clear the pending cookie, so the blob is replayable") ``` That `so` is the retracted causal claim verbatim, and the comment thirteen lines below it (`keymail_test.go:94-98`) says the opposite: "Note what does the work: NOT the cookie clearing … The first version of this test asserted the clearing was what made the round trip single-use, and it failed, because it is not." Same shape as finding B — a retracted claim standing inside the function that retracts it. One-line fix: drop the clause, or say "so this browser will re-send it". ## 3. C — CSP sweep — **named mutant FIXED; the sweep still does not reach 10 of 23 routes** Mutant L2 in `handleSigninBegin`: **FAILS** — `noscript_test.go:300: /signin: CSP directive "default-src 'self' 'unsafe-inline'" permits inline or eval'd script`. Same in `handleVerify` (`/signin/verify`) and `handlePasskeyConfirm`. 23 targets walked, matching the claimed count. **BLOCKING —** the same mutant **survives** (test reports `ok`) in all three of: | handler | route | sweep status | |---|---|---| | `handleRegenerateRecoveryCodes` | POST `/passkey/codes` | 403 | | `handleRecoveryCodesPage` | GET `/passkey/codes` | 303 | | `handlePasskeyEnrol` | GET `/passkey/enrol` | 303 | I instrumented the loop to log `method path -> status`. Of the 23 targets, **10 never reach a handler**: eight POSTs return 403 (all five `passkeys.*` ceremony endpoints, `register/begin`, `register/finish`, `codes`) and two GETs return 303 from `RequireFresh`. Origin+form-body cleared `csrf.Protect`, which is what the fix addressed, but the *auth* gates were never considered. The gap lands exactly on the passkey screens — the ones ruling 21 carves out for script, i.e. the tempting place to add `unsafe-inline`, which is the scenario the test's own opening comment names. So both claims in the new comment are false as written: - `noscript_test.go:229` "EVERY route, not just one" — 13 of 23. - `noscript_test.go:251` "chi.Walk already hands the method over; **using it is the whole fix**" — it is half of it; the handlers still need a session (and a fresh one) to run. - Consequently `noscript_test.go:150`'s "`TestServedCSPAllowsNoInlineScript` is what guards that" does not hold for those routes either. **No state or ordering pollution.** Each test builds its own `App`, DB (`t.TempDir`) and mailer; `testapp_test.go:125` forces `LookupTXT` to error, so the sweep's `POST /signin` classifies offline and takes the magic-link path against its own mailer. `make test`, `-race -count=2` and `-shuffle=on` are all green (see 7). ## 4. `TestVerifyPageWritesNothing` — **FIXED**, with a real blind spot Mutant V (`UPDATE signin_links SET expires_at='2099-…'` on a non-empty token in `handleVerifyForm`): **FAILS**, printing the before/after diff on `expires_at`. **Missed (mutant V2, survives — test reports `ok`):** ```go a.db.Writer().Exec(`UPDATE signin_links SET sealed_address = X'00'`) a.db.Writer().Exec(`INSERT INTO passkey_recovery_codes (subject, code_hash) VALUES ('leak','x')`) ``` The snapshot selects only `token_hash, purpose, expires_at` — `sealed_address` is not read — and counts only `people`, `sessions`, `passkey_pending`. The DB also holds `passkey_credentials`, `passkey_recovery_codes`, `passkey_challenges` and `rastrillo_migrations`, none of which is snapshotted at all. And because the other three are *counted*, not compared, an in-place update (extending a session's expiry, rewriting `auth_time`) is invisible; in this test all three counts are 0 both times, so they assert only that they stay 0. NIT — a `sqlite_master`-driven full-table dump would close it without much code. ## 5. `TestFlowUsesBoundedHTTPClients` — **FIXED** - Remove `HTTP: http` from `newFlow`'s keymail builder → `keymail_test.go:187: the keymail client is unbounded`. - Remove `HTTP: http` from the `Classifier` literal → `keymail_test.go:180: the classifier has no HTTP client, so it uses an unbounded default`. Both compiled clean before running. **The stub's client is genuinely used.** `github.com/keymaildev/signin@v0.1.1/keymail.go:105` reads `k.HTTP` for the exchange and falls back to `&http.Client{Timeout: 15 * time.Second}` — so the commit message's "library's 15s default" is exact, and the stub's `HTTP: &http.Client{Timeout: classifierTimeout}` now carries the exchanges. (`RedirectPath` default `/auth/callback` confirmed at `keymail.go:36`.) Nothing asserts the *stub's* client is bounded — removing it leaves the suite green — but that is a fidelity fix, not a guarded property, and the comment doesn't claim otherwise. ## 6. Changed doc comments Verified true: the two `noscript_test.go` stated limits (30 entries under `ui/partials`, zero `<script>` in any of them, `ui.Templates()` is `fs.Sub(partialsFS, "partials")` so the three `layouts/*.html` that *do* carry `<script>` are excluded); the `TestVerifyPageDoesNotDistinguishTokens` timing note (`handleVerifyForm` is branch-free on the token); both `013331d` comments. **NIT — `signin.go:266-271`, `handleCallback`, third revision of this sentence.** "The one path that renders is a downstream failure inside completeSignIn (find-or-mint, or minting the session)". There are **three** render paths in `completeSignIn`: `findOrMintPerson` error (`signin.go:295`), **`a.passkeys.Gate` error (`signin.go:318`)**, and `sessions.SignIn` error (`signin.go:330`). The parenthetical reads as exhaustive and omits the Gate. **NIT — `noscript_test.go:248`, "the eight POST-only handlers never ran".** There are 10 POST routes and 7 POST-only *paths*; under the old GET-only sweep all 10 POST handlers were skipped (GET `/signin` ran `handleSigninForm`, not `handleSigninBegin`). Eight matches neither count. **NIT — `passkey.go:75-77`, "which was true for two commits and then was not".** Introduced at `903eb7b`, falsified at `d39fc86` — four commits later; it then stood falsely for two (`d39fc86`, `099582e`). The sentence inverts its own history. **NIT — `noscript_test.go:54-59`, `srcAttr`.** The regex changed (`\bsrc` → `(?:^|\s)src`) but the comment above it still explains only the quoting alternatives and says nothing about `data-src=`. The fix itself is correct — I confirmed leftmost-match now picks the real `src` out of `data-src="a" src="b"`. ## 7. Test runs ``` make test PASS (vet, build, ./... , -race -count=1) go test -race -count=2 ./internal/home/ ok 21.154s go test -count=1 -shuffle=on ./internal/home/ ok 5.282s ``` No flakes, no data races, no ordering sensitivity. --- ## Verdict: **CHANGES NEEDED** Two blocking items, both small: 1. `keymail_test.go:91` — the failure message re-asserts the "clearing makes it single-use" claim that the same function retracts. 2. The CSP sweep — 10 of 23 handlers still never execute because of the session/`RequireFresh` gates, proven by three surviving mutants in `handleRegenerateRecoveryCodes`, `handleRecoveryCodesPage` and `handlePasskeyEnrol`. Either drive the gated routes with a fresh session cookie, or stop the comment claiming "EVERY route" and "the whole fix" and state the gate as a limit. Items A, B (in `takePending`), C (for the named mutant), 4 and 5 are all confirmed fixed by mutation.</result> <usage><subagent_tokens>104884</subagent_tokens><tool_uses>45</tool_uses><duration_ms>441991</duration_ms></usage> </task-notification>
diff --git a/internal/home/keymail_test.go b/internal/home/keymail_test.go| index ece31cc..52f0f62 100644 |
| --- a/internal/home/keymail_test.go |
| +++ b/internal/home/keymail_test.go |
| @@ -88,7 +88,7 @@ func TestPendingCookieIsClearedOnRead(t *testing.T) { |
| } |
| } |
| if !cleared { |
| - t.Error("the callback did not clear the pending cookie, so the blob is replayable") |
| + t.Error("the callback did not clear the pending cookie, so this browser will re-send it") |
| } |
| // Replaying the whole request — same blob, same code — gets nothing. |
diff --git a/internal/home/noscript_test.go b/internal/home/noscript_test.go| index e571ffd..f246893 100644 |
| --- a/internal/home/noscript_test.go |
| +++ b/internal/home/noscript_test.go |
| @@ -7,6 +7,7 @@ import ( |
| "net/http/httptest" |
| "os" |
| "path" |
| + "path/filepath" |
| "regexp" |
| "slices" |
| "sort" |
| @@ -57,6 +58,8 @@ var scriptOpen = regexp.MustCompile(`(?is)<script\b`) |
| // FAILED the guard, since fail-safe was the right default, but with a |
| // diagnosis naming the wrong problem. A guard that reports the wrong |
| // reason gets the wrong fix. |
| +// It also requires a whitespace boundary before src, not \b: \b matches |
| +// inside data-src=, so a tag carrying both reported the wrong one. |
| var srcAttr = regexp.MustCompile(`(?i)(?:^|\s)src\s*=\s*(?:"([^"]*)"|'([^']*)'|([^\s>]+))`) |
| // scriptSrcs returns one entry per <script> in b: its src, or the empty |
| @@ -245,11 +248,24 @@ func TestServedCSPAllowsNoInlineScript(t *testing.T) { |
| if !ok { |
| t.Fatalf("Router() is %T, which does not implement chi.Routes", app.Router()) |
| } |
| - // Method AND route. An earlier version issued GET on every path, |
| - // which meant the eight POST-only handlers never ran — and an |
| - // unsafe-inline header set in handleSigninBegin survived the whole |
| - // suite while this test claimed to cover "every route". chi.Walk |
| - // already hands the method over; using it is the whole fix. |
| + // Method AND route, AND a session. Getting here took three goes and |
| + // the history is the useful part: |
| + // |
| + // 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. |
| + // |
| + // So: a fresh session cookie as well. Three ceremony endpoints |
| + // (signin/begin, signin/finish, signin/recovery) need a pending |
| + // half-session rather than a session and still do not run here; |
| + // TestNoHandlerSetsItsOwnCSP covers those by reading source, which |
| + // is why both tests exist. |
| 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 { |
| @@ -271,6 +287,10 @@ func TestServedCSPAllowsNoInlineScript(t *testing.T) { |
| t.Fatal("walked no POST routes; the sweep would miss every state-changing handler") |
| } |
| + // A real, fresh session: the passkey routes want one, and the codes |
| + // and enrol pages want a recent one. |
| + cookie := signInFully(t, app, "sweeper@example.invalid") |
| + |
| for _, tg := range targets { |
| p := tg.path |
| req := httptest.NewRequest(tg.method, p, strings.NewReader("address=someone@example.invalid")) |
| @@ -279,6 +299,7 @@ func TestServedCSPAllowsNoInlineScript(t *testing.T) { |
| // the handler runs — and a handler that never runs sets no |
| // header to inspect. |
| req.Header.Set("Origin", Origin) |
| + req.AddCookie(cookie) |
| rec := httptest.NewRecorder() |
| h.ServeHTTP(rec, req) |
| @@ -373,3 +394,38 @@ func TestVendoredAssetsMatchTheLibrary(t *testing.T) { |
| t.Error("static/theme.css differs from ui.ThemeCSS(\"day\"); run `rastrillo doctor --fix`") |
| } |
| } |
| + |
| +// 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. |
| +func TestNoHandlerSetsItsOwnCSP(t *testing.T) { |
| + files, err := filepath.Glob("*.go") |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + var checked int |
| + for _, f := range files { |
| + if strings.HasSuffix(f, "_test.go") { |
| + continue |
| + } |
| + checked++ |
| + b, err := os.ReadFile(f) |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + if bytes.Contains(bytes.ToLower(b), []byte("content-security-policy")) { |
| + t.Errorf("%s mentions Content-Security-Policy; a handler's own header replaces "+ |
| + "rastrillo's default, which is how the passkey carve-out would quietly "+ |
| + "become a blanket one", f) |
| + } |
| + } |
| + if checked == 0 { |
| + t.Fatal("scanned no files; the guard would pass by not looking") |
| + } |
| +} |
diff --git a/internal/home/passkey.go b/internal/home/passkey.go| index 50fe6f1..b713368 100644 |
| --- a/internal/home/passkey.go |
| +++ b/internal/home/passkey.go |
| @@ -74,7 +74,8 @@ const codesFreshness = enrolFreshness |
| // not a shorter one — see codesFreshness, which is defined as this value |
| // precisely because a shorter one stranded people mid-flow. An earlier |
| // version of this sentence said "rather than the codes page's five", |
| -// which was true for two commits and then was not. |
| +// which was true when written and stood falsely for two commits after |
| +// codesFreshness was redefined as this value. |
| const enrolFreshness = 10 * time.Minute |
| // passkeyConfig is the config the app builds. |
diff --git a/internal/home/signin.go b/internal/home/signin.go| index b7ad4ae..4931bcf 100644 |
| --- a/internal/home/signin.go |
| +++ b/internal/home/signin.go |
| @@ -264,12 +264,15 @@ func (a *App) handleVerify(w http.ResponseWriter, r *http.Request) { |
| // credential origin is what §4 forbids. |
| // |
| // Both the success and the ordinary failure are redirects — a failed |
| -// round trip goes to /signin?force=1. The one path that renders is a |
| -// downstream failure inside completeSignIn (find-or-mint, or minting the |
| -// session), which shows a fixed page carrying nothing from the round |
| -// trip. An earlier version said "never renders" flatly, which was false; |
| -// its correction then implied rendering was the normal failure, which |
| -// is also wrong. |
| +// round trip goes to /signin?force=1. Rendering happens only on a |
| +// downstream failure inside completeSignIn, of which there are three: |
| +// find-or-mint, the passkey Gate, and minting the session. All three |
| +// show the same fixed page, carrying nothing from the round trip. |
| +// |
| +// This sentence is on its third revision. It said "never renders", |
| +// which was false; then implied rendering was the normal failure, which |
| +// was also wrong; then listed two of the three paths as though that |
| +// were all of them. |
| func (a *App) handleCallback(w http.ResponseWriter, r *http.Request) { |
| blob := a.takePending(w, r) |
| q := r.URL.Query() |
diff --git a/internal/home/signin_test.go b/internal/home/signin_test.go| index 14cdfb3..3a0d078 100644 |
| --- a/internal/home/signin_test.go |
| +++ b/internal/home/signin_test.go |
| @@ -524,41 +524,12 @@ func TestVerifyPageWritesNothing(t *testing.T) { |
| signIn(t, app, "person@example.invalid") |
| token := capturedLink(t, app) |
| - snapshot := func() string { |
| - rows, err := app.db.Writer().Query( |
| - `SELECT token_hash, purpose, expires_at FROM signin_links ORDER BY token_hash`) |
| - if err != nil { |
| - t.Fatal(err) |
| - } |
| - defer rows.Close() |
| - var out strings.Builder |
| - for rows.Next() { |
| - var h, p, e string |
| - if err := rows.Scan(&h, &p, &e); err != nil { |
| - t.Fatal(err) |
| - } |
| - fmt.Fprintf(&out, "%s|%s|%s\n", h, p, e) |
| - } |
| - if err := rows.Err(); err != nil { |
| - t.Fatal(err) |
| - } |
| - for _, table := range []string{"people", "sessions", "passkey_pending"} { |
| - var n int |
| - if err := app.db.Writer(). |
| - QueryRow("SELECT count(*) FROM " + table).Scan(&n); err != nil { |
| - t.Fatal(err) |
| - } |
| - fmt.Fprintf(&out, "%s=%d\n", table, n) |
| - } |
| - return out.String() |
| - } |
| - |
| - before := snapshot() |
| + before := dumpDatabase(t, app) |
| for _, tok := range []string{token, "not-a-real-token", ""} { |
| req := httptest.NewRequest(http.MethodGet, linkPath+"?token="+url.QueryEscape(tok), nil) |
| app.Router().ServeHTTP(httptest.NewRecorder(), req) |
| } |
| - if after := snapshot(); after != before { |
| + if after := dumpDatabase(t, app); after != before { |
| t.Errorf("GET %s changed stored state:\nbefore:\n%s\nafter:\n%s", linkPath, before, after) |
| } |
| } |
diff --git a/internal/home/testapp_test.go b/internal/home/testapp_test.go| index c059684..6d98dde 100644 |
| --- a/internal/home/testapp_test.go |
| +++ b/internal/home/testapp_test.go |
| @@ -3,6 +3,7 @@ package home |
| import ( |
| "context" |
| "errors" |
| + "fmt" |
| "os" |
| "path/filepath" |
| "strings" |
| @@ -153,3 +154,71 @@ func grepPackage(t *testing.T, needle string) []string { |
| } |
| return found |
| } |
| + |
| +// dumpDatabase renders every row of every table, so a test can assert |
| +// that an operation changed nothing at all. |
| +// |
| +// Every table and every column, discovered from sqlite_master rather |
| +// than listed. The first version of this listed three columns of one |
| +// table and row COUNTS of three others — so a write to sealed_address, |
| +// 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. |
| +func dumpDatabase(t *testing.T, app *testApp) string { |
| + t.Helper() |
| + var tables []string |
| + rows, err := app.db.Writer().Query( |
| + `SELECT name FROM sqlite_master WHERE type='table' ORDER BY name`) |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + for rows.Next() { |
| + var name string |
| + if err := rows.Scan(&name); err != nil { |
| + rows.Close() |
| + t.Fatal(err) |
| + } |
| + tables = append(tables, name) |
| + } |
| + rows.Close() |
| + if err := rows.Err(); err != nil { |
| + t.Fatal(err) |
| + } |
| + if len(tables) == 0 { |
| + t.Fatal("no tables found: the snapshot would compare nothing") |
| + } |
| + |
| + var out strings.Builder |
| + for _, table := range tables { |
| + fmt.Fprintf(&out, "== %s\n", table) |
| + // The identifier comes from sqlite_master, not from a caller. |
| + r, err := app.db.Writer().Query(`SELECT * FROM "` + table + `" ORDER BY 1`) |
| + if err != nil { |
| + t.Fatalf("dump %s: %v", table, err) |
| + } |
| + cols, err := r.Columns() |
| + if err != nil { |
| + r.Close() |
| + t.Fatal(err) |
| + } |
| + for r.Next() { |
| + cells := make([]any, len(cols)) |
| + for i := range cells { |
| + cells[i] = new(any) |
| + } |
| + if err := r.Scan(cells...); err != nil { |
| + r.Close() |
| + t.Fatal(err) |
| + } |
| + for i, c := range cells { |
| + fmt.Fprintf(&out, " %s=%v\n", cols[i], *(c.(*any))) |
| + } |
| + out.WriteString(" --\n") |
| + } |
| + r.Close() |
| + if err := r.Err(); err != nil { |
| + t.Fatal(err) |
| + } |
| + } |
| + return out.String() |
| +} |