amadan
Answer the confirmation review: three claims that outlived what they described
All six findings from the previous round were confirmed killed by mutation. Three new blocking items, and two of them are the same defect this project keeps producing — a sentence that was true when written and was not updated when the thing it described changed. A. enrolFreshness still said "Ten minutes rather than the codes page's five". The commit two before this one made codesFreshness EQUAL to enrolFreshness, so the codes page's window is ten. Changed the value, left the sentence citing the old one. B. Worse, and worth naming plainly: takePending still said "clears it in the same breath, so one blob completes at most one callback" — the exact sentence the rewritten pendingCookie comment, three hundred lines above, spends a paragraph retracting as false. The retraction and the claim shipped in the same commit. C. The CSP sweep walked all 23 routes and issued GET to every one, so the eight POST-only handlers never ran. An unsafe-inline header set in handleSigninBegin survived the whole suite while the test's own comment claimed to cover "every route". chi.Walk already hands over the method; using it is the entire fix, and the mutant now fails naming /signin. The test also refuses to pass if it walks no POST routes at all. Beyond the three: - The oracle test had two survivors. One is now a test rather than a stated limit: TestVerifyPageWritesNothing snapshots signin_links and three row counts across a valid, an invalid and an empty token, so a GET that quietly extended a live link's expiry is caught — verified by writing exactly that mutant. The other, a timing oracle, is stated as a limit instead: it needs the handler to stay branch-free on the token, which is a thing to re-read when it changes rather than to test badly. - handleCallback's corrected sentence had over-corrected. Both success and ordinary failure are redirects; only a downstream failure inside completeSignIn renders. Said properly now, along with what the two earlier versions each got wrong. - The script sweep states two limits it does not cover: inline event handlers (onclick= is script and is not <script>; the CSP is what makes it inert), and rastrillo's ui partials, which are parsed into every page tree and are clean at the pinned version but are not scanned here. - srcAttr's \b also matched data-src=, so a page with both would report the wrong one. Anchored to a whitespace boundary. Confirmed by the reviewer and recorded so nobody re-derives it: the keymail stub leaks no goroutines, the rate-limit tests isolate the two budgets (forcing clientIP to "" kills only the per-IP one), chi.Walk does enumerate both r.Group blocks, and -race -count=2 plus two -shuffle=on runs are green. Known gap in the stub, stated rather than hidden: it rebuilds app.flow.Keymail without the HTTP field, so exchanges run on the library's 15s default client rather than the app's classifierTimeout one. The timeout wiring is therefore untested.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
internal/home/noscript_test.go+41 −8internal/home/passkey.go+7 −3internal/home/signin.go+20 −6internal/home/signin_test.go+56 −0
CI passed — run details
Intent
<task-notification> <task-id>ac69975687101d23e</task-id> <tool-use-id>toolu_01BgCpirWMvZ3yUdR9igByLo</tool-use-id> <output-file>/home/paulca/.cache/tmp/claude-1001/-home-paulca-amadan-net-oficina-home/d63f5ac7-6b42-48c5-a13d-34e6faf8e44c/tasks/ac69975687101d23e.output</output-file> <status>completed</status> <summary>Agent "Confirm the third review's fixes" 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>Repo untouched — all mutation work ran in `$TMPDIR` copies built from `git archive d39fc86`. Every mutant below was compile-checked before scoring. ## The six findings **1. Gate on the keymail path — KILLED.** Mutant (mint via `sm := a.sessions; sm.SignIn(...)` in `handleCallback`) compiles, and `TestKeymailSignInIsGatedForAnEnrolledPerson` fails: *"the keymail callback went to "/", want "/passkey/confirm"".* I re-ran it with the grep string scrubbed from my mutant's comment — my first attempt accidentally contained `a.sessions.SignIn(` verbatim and tripped the tripwire spuriously. The behavioural test alone is the killer, which is exactly what the commit claims. *Stub fidelity:* faithful for what these tests assert. `/api/federation/lookup`'s key matches `probe`'s `lower(local)+"@"+domain`; well-known field names match `domainInfo`'s struct (`version`, not `v`); zero-vs-nonzero `auth_time` matches `Exchange`. Named gaps, none load-bearing: the stub ignores `code_verifier` (no PKCE S256 check), `client_id` and `redirect_uri`; there is no `/oauth/authorize` handler, so codes are invented by the test rather than issued; and `app.flow.Keymail` is rebuilt **without** the `HTTP` field, so exchanges run on the library's 15s default client instead of the app's `classifierTimeout` one — the timeout wiring is untested. **2. Script sweep — KILLED** for `{{if .Debug}}`, `</script >`, unbalanced `<script`, a script in `layout.html`, one inside an HTML comment, and a dynamic `src="{{.X}}"` on an allowlisted page. `src='...'`: in committed `d39fc86` it falls to the inline branch — fails safe, wrong diagnosis. The uncommitted working-tree edit fixes it properly (verified: reports *loads "https://evil.invalid/x.js"*). Two survive. `<scr{{""}}ipt ...>` renders as `<scrZgotmplZipt>` — html/template's auto-escaping neutralises it, so not a hole. `onclick="..."` **is** unswept (CSP is the only thing stopping it) — NIT. **3. Oracle test — KILLED.** Mutant R (token echoed twice when live) fails on the body: *"676 bytes vs 633 after normalising"*. Mutant T′ (`Cache-Control` set only when live) fails on headers. **Two it still misses**, both compile and survive the *whole* suite: - **U (timing):** `time.Sleep(40ms)` when the token is live. The test compares status, headers and body only. A prefetching scanner times the response and learns liveness. - **V (state):** the GET does `UPDATE signin_links SET expires_at = now+24h` for a live token. `handleVerifyForm`'s comment says *"It TOUCHES NO STATE, and that is the whole point"* and *"It must not look the token up either"* — nothing enforces either. The test enforces response-indistinguishability, which is narrower than the comment claims. **4. Freshness ordering — KILLED.** Reverting to `5 * time.Minute` fails both `TestFreshnessWindowsDoNotStrandAnEnroller` and `TestEnrolThenReadCodesWithoutReauthenticating` (303 at 7 minutes). Day-old sessions still refused by both windows. `codesFreshness` has exactly one use site, so nothing else assumes the old ordering. `passkey.js` has only two redirect chains — enrol→`/passkey/codes` (the fixed one) and confirm→`/`, ungated — so no other route pair has the problem. **5. CSP — mutant L KILLED**, naming `/passkey/confirm` as promised. `chi.Walk` does enumerate all 23 routes including both `r.Group` blocks (they report `middlewares=2`). **But see blocking C below.** **6. Rate-limit tests — real.** Budgets of 1,000,000 kill both. Forcing `clientIP` to `""` kills *only* the per-IP test while the per-address one still passes, proving the two isolate different budgets. No shared state: `limiterOnce`/`defaultLimiter` are `Flow` fields, `newTestApp` builds a fresh `Flow` per test, and `NewMemoryLimiter` starts no goroutine or ticker. `-race -count=2` and two `-shuffle=on` runs are all green. ## New findings **BLOCKING A — `enrolFreshness`'s justification is now false** (`internal/home/passkey.go:71`). It still reads *"Ten minutes rather than the codes page's five"*. This commit made `codesFreshness = enrolFreshness`, so the codes page's window is ten. The commit changed the value and left the sentence that cites the old one. **BLOCKING B — the retracted claim survives verbatim** (`internal/home/signin.go:351`). `takePending`'s comment: *"clears it in the same breath, so one blob completes at most one callback."* That is the exact sentence the rewritten `pendingCookie` comment 300 lines above spends a paragraph retracting as false. **BLOCKING C — the CSP sweep probes every route with GET only.** `TestServedCSPAllowsNoInlineScript` walks all 23 routes but issues `httptest.NewRequest(http.MethodGet, p, nil)`, so the 8 POST-only handlers never run. Mutant L2 — the same `unsafe-inline` header set in `handleSigninBegin` — **survives the whole suite**. The comment says *"EVERY route, not just one"*, which overstates it. `chi.Walk` already hands the callback the method; using it is the fix. **NITs** - `handleCallback`'s new sentence *"On success it redirects. On a failure it renders signin_failed.html"* inverts the common case: a failed `CompleteKeymail` redirects to `/signin?force=1`; only a downstream `completeSignIn` failure renders. Technically defensible, misleading as written. - `onclick=` and other inline handlers are outside the sweep entirely. - The sweep reads only this app's `templates/*.html`; 30 rastrillo `ui` partials are parsed into every page tree and never scanned. Clean today (no `<script>` in `partials/`, and `ui.Templates()` excludes the `layouts/` files that do have one), but an upgrade could change that silently. - `\bsrc` matches `data-src=`, so `data-src="/evil.js" src="/static/passkey.js"` reports `/evil.js`. Fail-safe, wrong diagnosis — the defect the new `srcAttr` comment says it fixed. Pre-existing. - Missing blank comment line before *"It must be at least enrolFreshness"* (`passkey.go:41`) runs two paragraphs together. **Not a problem:** the keymail stub does **not** leak goroutines — `t.Cleanup(k.srv.Close)` handles it. The ~+2 goroutines/test I measured is `newTestApp` alone (identical delta with no stub and no HTTP at all), pre-existing and unrelated to this commit. Verified claims: the "four mint sites" count is right (`recovery.go:148`, `passkey.go:334`, `passkey.go:550`, plus `completeSignIn`); `PendingTTL` does default to 10 minutes. The `clientIP` carlos citation cites a repo not present here, so I could not check it. ## Verdict **CHANGES NEEDED.** All six original findings are genuinely fixed and their mutants killed — the work is sound. Three blocking items remain, all small: two false doc claims (A, B) that are the same defect class this commit set out to eliminate, and one guard (C) that still cannot fail on 8 of 23 routes while claiming to cover every one. Findings 3's survivors (timing, and a GET that writes) are worth a stated limit in the comment rather than more test machinery, since the comment currently promises more than the test delivers.</result> <usage><subagent_tokens>171697</subagent_tokens><tool_uses>55</tool_uses><duration_ms>758653</duration_ms></usage> </task-notification>
diff --git a/internal/home/noscript_test.go b/internal/home/noscript_test.go| index 827a507..e571ffd 100644 |
| --- a/internal/home/noscript_test.go |
| +++ b/internal/home/noscript_test.go |
| @@ -57,7 +57,7 @@ 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. |
| -var srcAttr = regexp.MustCompile(`(?i)\bsrc\s*=\s*(?:"([^"]*)"|'([^']*)'|([^\s>]+))`) |
| +var srcAttr = regexp.MustCompile(`(?i)(?:^|\s)src\s*=\s*(?:"([^"]*)"|'([^']*)'|([^\s>]+))`) |
| // scriptSrcs returns one entry per <script> in b: its src, or the empty |
| // string for an inline one. An inline body counts even when a src is |
| @@ -143,6 +143,17 @@ func sampleData(name string) []any { |
| // screen works with scripts off, and ruling 21 carves out only the |
| // passkey screens. |
| // |
| +// Two stated limits, so nobody reads more into a green run than is |
| +// there. It does not catch inline event handlers — onclick= and its |
| +// relatives are script and are not <script>; rastrillo's default CSP |
| +// (default-src 'self', no unsafe-inline) is what makes them inert, and |
| +// TestServedCSPAllowsNoInlineScript is what guards that. And it reads |
| +// only this app's templates/*.html, while roughly thirty rastrillo ui |
| +// partials are parsed into every page tree. Those are clean at the |
| +// pinned version — no <script> anywhere in partials/, and ui.Templates() |
| +// excludes the layouts/ files that do have one — but an upgrade could |
| +// change that without this sweep noticing. |
| +// |
| // It reads template SOURCE, not rendered output, and that is the |
| // correction. The first version rendered each page with a hand-written |
| // fixture and scanned the result, with a comment claiming the fixture |
| @@ -234,20 +245,42 @@ func TestServedCSPAllowsNoInlineScript(t *testing.T) { |
| if !ok { |
| t.Fatalf("Router() is %T, which does not implement chi.Routes", app.Router()) |
| } |
| - var paths []string |
| - if err := chi.Walk(routes, func(_ string, route string, _ http.Handler, _ ...func(http.Handler) http.Handler) error { |
| - paths = append(paths, strings.ReplaceAll(route, "/*", "")) |
| + // 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. |
| + 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 { |
| + targets = append(targets, target{method, strings.ReplaceAll(route, "/*", "")}) |
| return nil |
| }); err != nil { |
| t.Fatalf("walk routes: %v", err) |
| } |
| - if len(paths) < 5 { |
| - t.Fatalf("walked %d routes; the sweep would pass by not looking", len(paths)) |
| + if len(targets) < 5 { |
| + t.Fatalf("walked %d routes; the sweep would pass by not looking", len(targets)) |
| + } |
| + var posts int |
| + for _, tg := range targets { |
| + if tg.method == http.MethodPost { |
| + posts++ |
| + } |
| + } |
| + if posts == 0 { |
| + t.Fatal("walked no POST routes; the sweep would miss every state-changing handler") |
| } |
| - for _, p := range paths { |
| + for _, tg := range targets { |
| + p := tg.path |
| + req := httptest.NewRequest(tg.method, p, strings.NewReader("address=someone@example.invalid")) |
| + req.Header.Set("Content-Type", "application/x-www-form-urlencoded") |
| + // The POSTs are behind csrf.Protect, which short-circuits before |
| + // the handler runs — and a handler that never runs sets no |
| + // header to inspect. |
| + req.Header.Set("Origin", Origin) |
| rec := httptest.NewRecorder() |
| - h.ServeHTTP(rec, httptest.NewRequest(http.MethodGet, p, nil)) |
| + h.ServeHTTP(rec, req) |
| csp := rec.Header().Get("Content-Security-Policy") |
| if csp == "" { |
diff --git a/internal/home/passkey.go b/internal/home/passkey.go| index 1e86abc..50fe6f1 100644 |
| --- a/internal/home/passkey.go |
| +++ b/internal/home/passkey.go |
| @@ -38,6 +38,7 @@ const confirmPath = "/passkey/confirm" |
| // will accept a recovery code instead. Asserted in |
| // TestRecoveryCodeProducesAFreshSession so the fact cannot change |
| // silently. |
| +// |
| // It must be at least enrolFreshness. static/passkey.js sends the |
| // browser to /passkey/codes the instant registration finishes, so a |
| // window shorter than the one that let them enrol bounces them to |
| @@ -68,9 +69,12 @@ const codesFreshness = enrolFreshness |
| // reauth=1, so it is a detour and not a dead end. It costs an attacker |
| // the inbox, which is the whole point. |
| // |
| -// Ten minutes rather than the codes page's five: enrolling means reading |
| -// a page, deciding, and completing an authenticator gesture, where the |
| -// codes page is one click on a decision already made. |
| +// Ten minutes: enrolling means reading a page, deciding, and completing |
| +// an authenticator gesture. The codes page runs on the same window and |
| +// 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. |
| 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 ad29abe..b7ad4ae 100644 |
| --- a/internal/home/signin.go |
| +++ b/internal/home/signin.go |
| @@ -261,10 +261,15 @@ func (a *App) handleVerify(w http.ResponseWriter, r *http.Request) { |
| // |
| // It never renders anything derived from the return: the host is chosen |
| // by the address, and putting an account-influenced string on the |
| -// credential origin is what §4 forbids. On success it redirects. On a |
| -// failure it renders signin_failed.html, which is a fixed page carrying |
| -// nothing from the round trip — an earlier version of this sentence said |
| -// "never renders" flatly, which was false. |
| +// 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. |
| func (a *App) handleCallback(w http.ResponseWriter, r *http.Request) { |
| blob := a.takePending(w, r) |
| q := r.URL.Query() |
| @@ -348,8 +353,17 @@ func (a *App) setPending(w http.ResponseWriter, blob string) { |
| }) |
| } |
| -// takePending reads the pending blob and clears it in the same breath, |
| -// so one blob completes at most one callback. |
| +// takePending reads the pending blob and clears the cookie in the same |
| +// breath. |
| +// |
| +// The clearing is hygiene, NOT single use — see the note on |
| +// pendingCookie. It stops this browser re-submitting; it does not make |
| +// the blob itself spent, because the blob is a self-contained value |
| +// that stays valid until it expires. The authorization code is what is |
| +// single use, at the federation server. An earlier version of this |
| +// comment said "so one blob completes at most one callback", which is |
| +// the claim the pendingCookie note retracts — and it survived here, |
| +// verbatim, through the commit that retracted it. |
| func (a *App) takePending(w http.ResponseWriter, r *http.Request) string { |
| http.SetCookie(w, &http.Cookie{ |
| Name: pendingCookie, |
diff --git a/internal/home/signin_test.go b/internal/home/signin_test.go| index 567cfca..14cdfb3 100644 |
| --- a/internal/home/signin_test.go |
| +++ b/internal/home/signin_test.go |
| @@ -236,6 +236,14 @@ func TestVerifyPageDoesNotDistinguishTokens(t *testing.T) { |
| good.Header(), bad.Header()) |
| } |
| + // A further stated limit, because the handler's comment claims more |
| + // than any response comparison can check. "It touches no state" is |
| + // asserted separately, by TestVerifyPageWritesNothing. TIMING is |
| + // not asserted at all: a handler that slept only for a live token |
| + // would pass everything here, and a prefetching scanner could read |
| + // it. Closing that needs the handler to stay branch-free on the |
| + // token, which is what it is — and is worth re-reading rather than |
| + // re-testing whenever it changes. |
| placeholder := strings.Repeat("#", len(real)) |
| g := strings.ReplaceAll(good.Body.String(), real, placeholder) |
| b := strings.ReplaceAll(bad.Body.String(), fake, placeholder) |
| @@ -506,3 +514,51 @@ func TestSigninIsRateLimitedPerAddress(t *testing.T) { |
| "a mail bomb only needs a botnet") |
| } |
| } |
| + |
| +// The confirmation page's comment says it TOUCHES NO STATE. The |
| +// indistinguishability test cannot check that — a GET that quietly |
| +// extended a live link's expiry would render identically and pass — so |
| +// this checks the database directly. |
| +func TestVerifyPageWritesNothing(t *testing.T) { |
| + app := newTestApp(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() |
| + 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 { |
| + t.Errorf("GET %s changed stored state:\nbefore:\n%s\nafter:\n%s", linkPath, before, after) |
| + } |
| +} |