amadan
Answer the third review: four gates that did not guard what they claimed
Verdict was CHANGES NEEDED on six blocking findings. The reviewer used
mutation testing throughout and it was worth every minute — four of the
six are gates whose comments promised more than the code delivered, and
each was proved by a mutant that survived with the suite green.
## 1. The Gate could be taken off the keymail path with the suite green
The only thing pinning it was a text grep for "a.sessions.SignIn(".
Writing the mint through a local variable instead removed the second
factor from the keymail door and every test still passed — verbatim the
bug Task 6 exists to prevent, arriving through the door the guard could
not see.
The grep's stated invariant was also false. The deployed app mints
sessions in FOUR places: this package's completeSignIn, plus
passkey.SignInFinish, SignInRecovery and StepUpFinish, all mounted on
this router. Those three are legitimate — each one IS the second factor,
so gating them would be circular — but "exactly one call site" was never
true of the app, only of this package's source. It is kept as a
tripwire, honestly described, and no longer described as a proof.
The real fix is a keymail federation STUB, so the path can be driven end
to end for the first time: well-known, federation lookup and token
exchange, with authorization codes redeemed once per code the way a real
server does. The reviewer's mutant now fails two tests with "the Gate
did not take over on this path, so one factor opened an enrolled
account".
## 2 and 3. The scriptless sweep could not see two whole classes of script
It rendered each page with a hand-written fixture and scanned the
output, claiming the fixture reached "every branch". On a map,
{{if .Debug}} resolves to nil with no error — so a script inside any
branch the fixture did not satisfy rendered as nothing and the sweep
passed by not looking. Any key a future template introduced was
invisible by construction.
And its regex required the literal "</script>", so "</script >" — valid
HTML every browser accepts — walked straight past it. The justification
for hand-rolling was "the shapes it has to survive are the ones we
write", which is exactly the assumption that failed: nobody writes
"</script >" on purpose, which is why nobody would notice.
The sweep now reads template SOURCE, which has no branches to miss,
with a whitespace-tolerant matcher and a count of opening tags against
complete elements so an unbalanced tag is refused rather than skipped.
All three mutants now fail.
## 4. The oracle test erased the oracle it was looking for
It compared bodies with each response's own token replaced by the EMPTY
string, so any difference that was a function of the token — a valid
token echoed twice, padded, truncated — normalised away to identical.
And it never compared headers at all.
Now: a fake token of the same length, each response normalised with a
same-length placeholder, and headers compared as well as status and
body. A stated limit, because it is real: a difference keyed only on
token LENGTH is still invisible, which is fine, because length separates
well-formed from malformed and not live from dead.
## 5. The keymail path had no success-path coverage, so two claims were unasserted
Deleting the pending cookie's clearing, and stripping every attribute
off it — no __Host-, a Domain, no HttpOnly, no Secure — both left the
suite green, because no test ever took that branch.
Writing the test found something better than the mutant: the claim
"cleared on read, so a blob is good for exactly one callback" is FALSE.
Clearing touches one browser's cookie jar; the blob stays a valid
self-contained value until it expires. What actually makes the round
trip single-use is the authorization code, redeemed once at the
federation server. The comment says that now, and the stub models it,
because a stub that did not was how the false claim survived.
Nit 8 fell out of the same rewrite: SameSite=Lax does NOT stop this
attack shape — it sends the cookie on exactly the cross-site top-level
navigation the callback is. What stops an attacker is the HMAC and the
sealed state.
## 6. A bug I introduced two commits ago
enrolFreshness (10m) against codesFreshness (5m), with passkey.js
sending the browser to /passkey/codes the instant registration finishes.
A session between five and ten minutes old could enrol a passkey and was
then bounced to re-authenticate — at the exact moment it held a passkey
and zero recovery codes. That is precisely the outcome codesFreshness's
own comment says the value was chosen to prevent; the comment was true
until I added enrolFreshness at a larger value and did not revisit it.
codesFreshness is now defined AS enrolFreshness, with the ordering
pinned by a test and by an end-to-end enrol-then-read-codes test at
seven minutes, the age that used to fall in the gap.
## The rest
- The package doc still said sign-in was not implemented, eleven commits
after it was.
- "It redirects and never renders" was false: a failed find-or-mint
renders signin_failed.html. It carries nothing from the round trip, so
§4 holds, but the sentence did not.
- The CSP check requested one path. A handler's own header REPLACES
rastrillo's default, so an unsafe-inline carve-out on the one page
that actually loads script — the tempting place — was invisible. It
now walks every route, and the mutant fails naming /passkey/confirm.
- The rate limiter had never been driven. Two tests now: 25 attempts
from one IP get refused, a second IP is not, and one address from
twelve IPs is still bounded.
- clientIP's carlos citation is correct, and now says what it does not
cover: run outside the edge, any client sets the header for itself.
- The classifier timeout comment framed the risk as a slow handler. The
real exposure is an outbound fetch to a host the submitter's DNS
chooses, unvalidated, possibly internal. Named.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
cmd/home/main.go+4 −4internal/home/app.go+8 −5internal/home/keymail_test.go+168 −0internal/home/keymailstub_test.go+181 −0internal/home/noscript_test.go+129 −55internal/home/passkey.go+10 −1internal/home/passkey_test.go+68 −8internal/home/signin.go+41 −11internal/home/signin_test.go+103 −6
CI passed — run details
Intent
<task-notification> <task-id>a32568a657269a4fc</task-id> <tool-use-id>toolu_01KkvsBK8VEqv7UJLfisLYik</tool-use-id> <output-file>/home/paulca/.cache/tmp/claude-1001/-home-paulca-amadan-net-oficina-home/d63f5ac7-6b42-48c5-a13d-34e6faf8e44c/tasks/a32568a657269a4fc.output</output-file> <status>completed</status> <summary>Agent "Review tasks 5-7" 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>I have everything I need. Report follows. ## Scope note The branch moved under me. The assigned range was `e41d43c..05376b0` (six commits); by the time I finished, `HEAD` was **f945e66** — eleven commits, adding `903eb7b` (enrol freshness), `3211100` (the X-Forwarded-For citation), `f945e66` (callback binding tests) and two step-3 plan commits. **Everything below is against f945e66**, verified against a byte-identical snapshot. Two of the things I would have filed (`/passkey/enrol` freshness; the untested callback binding) were fixed while I read, and I have not filed them; one of the fixes introduced a new defect (finding 6). Baseline: `go test ./...` green. All mutants below were confirmed to **compile** (`go vet ./...` clean) before being scored — my first pass mis-scored two build failures as survivors and I re-ran everything. --- # BLOCKING ## 1. The Gate can be removed from the keymail path and the whole suite still passes `internal/home/signin.go:245` `completeSignIn` is the only Gate call site, and the only thing that pins it is a text grep. **Mutant (survives, suite green):** in `handleCallback`, replace `a.completeSignIn(w, r, id)` with ```go sm := a.sessions if err := sm.SignIn(w, r, sessions.Session{Subject: "pr_bypass", Method: string(id.Method), AuthTime: id.AuthTime, At: id.At}); err != nil { ... } http.Redirect(w, r, "/", http.StatusSeeOther) ``` `go test ./internal/...` → **ok**. A keymail-upgraded account with a passkey enrolled now signs in with one factor. That is, verbatim, the bug the plan says this task exists to prevent (`docs/superpowers/plans/2026-09-03-home-signin.md`, Task 6 step 3: *"An earlier draft named only the first, which left a Keymail-upgraded account with a passkey enrolled signing in with a single factor. That is the bug this task exists to prevent, arriving through the door the task forgot."*). The same mutation written as `a.sessions.SignIn(` **is** caught — by `TestSessionsSignInHasExactlyOneCallSite` alone. So the only guard on the keymail path is a substring match on one receiver spelling. Renaming the receiver, taking a local, or mounting any other rastrillo handler defeats it. Two doc claims fall with it: - `internal/home/passkey_test.go:130` — *"The Gate is on both sign-in paths because there is exactly one place that mints a session… a future handler that calls sessions.SignIn directly would bypass the second factor silently, and this makes that loud."* It does not make it loud; it makes one spelling of it loud. - `internal/home/passkey_test.go:138` — *"sessions.SignIn is called in %d places (%v); it must be exactly one, or a caller can mint a session without passing passkey.Gate."* The deployed app already has **four** session mints: `signin.go`, plus `passkey.SignInFinish`, `SignInRecovery` and `StepUpFinish`, all three mounted on this router at `app.go:165-167`. Three of the four never pass the Gate. They are legitimately gated by other means, but the invariant the test states is not the invariant the app has, and the test cannot tell the difference. Also worth knowing: the grep counts occurrences in **comments and strings**. A comment quoting `"a.sessions.SignIn("` fails the test on its own. It is simultaneously trivially evadable and trivially false-positive. **Fix:** assert behaviour, not text. There is no test anywhere that a keymail callback for an enrolled person redirects to `confirmPath` and mints nothing — the mirror of `TestEnrolledPersonCannotSignInWithOneFactor` for the other door. ## 2. `sampleData` does not reach every script-bearing branch; the doc says it does `internal/home/noscript_test.go:77-79`: > *"sampleData is enough data for each page to render its script-bearing branches. Every branch matters: a page that only emits its `<script>` inside an `{{if}}` would otherwise slip past."* **Mutant (survives):** add to `signin.html` — ``` {{if .Debug}}<script src="/static/evil.js"></script>{{end}} ``` `sampleData` is a hand-written fixed key set (`RPID`, `LegacyRPID`, `UserID`, `UserName`, `Token`, `Remaining`, `Error`, `RecoveryFailed`, `Codes`). On a `map[string]any`, `.Debug` resolves to `nil` with no error, the branch never renders, and the sweep passes by not looking. Any key a future template introduces is invisible to the sweep by construction. The sentence quoted is false at source. ## 3. The `<script>` regex misses a shape that renders fine in every browser `internal/home/noscript_test.go:39` — `` `(?is)<script([^>]*)>(.*?)</script>` ``, justified at line 36 as *"Hand-rolled rather than pulling in an HTML parser as a dependency: the input is this repo's own templates, and the shapes it has to survive are the ones we write."* **Mutant (survives):** `<script src="/static/evil.js"></script >` — one space before the closing bracket. The regex requires the literal `</script>`, finds no match, `scriptSrcs` returns empty, and `TestNoScriptTagOutsideTheAllowlist` `continue`s past a page that loads an arbitrary script. HTML permits whitespace there and every browser accepts it. "The shapes it has to survive are the ones we write" is exactly the assumption that fails: nobody writes `</script >` on purpose, which is why nobody would notice. (The plan, Task 7 step 2a, specified `scriptSrcs` should *"parse the HTML"*. It does not.) ## 4. The oracle test's token-stripping does hide a real difference — and it only compares body + status `internal/home/signin_test.go:206-212`. The comment asserts the thing it cannot check: *"The bodies differ only where the token itself is echoed into the hidden field."* **Mutant R (survives):** make `handleVerifyForm` consult the store and, for a token that exists, render `Token: tok + tok`. ```go if a.db.Writer().QueryRow(`SELECT count(*) FROM signin_links WHERE token_hash = ?`, signin.HashToken(tok)).Scan(&n) == nil && n > 0 { shown = tok + tok // a VALID token renders differently } ``` Suite green. `ReplaceAll(good, real, "")` erases both copies and `ReplaceAll(bad, fake, "")` erases the one — the two stripped bodies are byte-identical. **Any oracle whose output is a function of the token string is erased by the comparison.** A prefetching mail scanner sees a page whose length differs by 43 bytes for a live token. **Mutant T (survives):** put the oracle in a header — `w.Header().Set("X-Link", "known")` when `len(tok) == 43`. The test reads `good.Code`, `bad.Code` and the two bodies. Headers are never compared. The handler is correct today. The test does not establish that it stays correct. ## 5. `handleCallback` still has no coverage of the success path — so the pending cookie's attributes and its clearing are unasserted `f945e66` added four cookie-less shapes and one bad-MAC blob. Good, and I confirmed at source that `openPending("")` returns `ErrPendingInvalid` before any network call and that `state` is compared against the sealed `p.State` (`signin@v0.1.1/pending.go:59-67`, `flow.go:217`). The comment at `signin.go:224-236` is true. But no test ever drives a *successful* keymail return, so: **Mutant O (survives):** delete the whole clearing `http.SetCookie` from `takePending` (`signin.go:323-328`). The blob is then good for unlimited callbacks. `signin.go:41` — *"And it is cleared on read, so a blob is good for exactly one callback"* — is unasserted. **Mutant P (survives):** rename `pendingCookie` to `"oficina-signin"` and set `Domain: "oficina.works"`, `HttpOnly: false`, `Secure: false`. Suite green. `TestNoCookieOnTheCredentialOriginCarriesADomain` inspects only the recorders from `signIn` and `redeem`, and tests never take the keymail branch (the classifier is stubbed to error), so it never sees this cookie. `signin.go:34` — *"Every attribute here is load-bearing"* — is asserted by nothing. On the ordering you asked about: writing `Set-Cookie` to `w` before `r.Cookie` is correct and safe on every path — they touch different header maps, and the clearing header is emitted whether the read succeeds, fails, or the handler later renders. That part is fine; it is just untested. ## 6. `enrolFreshness` (10m) and `codesFreshness` (5m) contradict each other, and `passkey.js` walks straight into the gap Introduced by `903eb7b`. `internal/home/passkey.go:41` `codesFreshness = 5 * time.Minute`, `:65` `enrolFreshness = 10 * time.Minute`, and `internal/home/static/passkey.js:54` sends the browser to `/passkey/codes` the instant registration finishes. Measured: ``` session 7m old: GET /passkey/enrol = 200 session 7m old: POST /passkey/register/begin = 200 session 7m old: GET /passkey/codes = 303 → /signin?reauth=1 ``` So a person whose session is 5-10 minutes old is allowed to enrol a passkey and is then bounced to re-authenticate — **at the one moment they hold a passkey and zero recovery codes.** That is precisely the outcome `codesFreshness`'s own doc comment (`passkey.go:25-27`) says the value was chosen to prevent: > *"Long enough to exceed a passkey ceremony round trip, or enrolling and then being asked for your codes would bounce straight back to a step-up."* That sentence was true when `enrolFreshness` did not exist. `903eb7b` made it false and did not revisit it. Either make `codesFreshness >= enrolFreshness`, or gate them at the same value, or state the ordering constraint and pin it with a test. --- # NIT ## 7. `internal/home/app.go:7-11` and `cmd/home/main.go:3` are false > *"This package is not yet the home: it wires the platform contract, validates the configuration sign-in needs (keys.go) and seals values at rest (seal/), but serves only one placeholder page. **Sign-in**, directory, assertions and provisioning **are not implemented yet**. See page.go."* Sign-in, passkeys, recovery codes, eight templates and eighteen routes are implemented in this very branch. `cmd/home/main.go:3` likewise still says *"It serves internal/home's one placeholder page."* Eleven commits went past both. ## 8. `signin.go:38` overstates what `SameSite=Lax` contributes > *"SameSite=Lax is what stops a cross-site request carrying it"* Lax **sends** the cookie on cross-site top-level GET navigations — which is exactly the shape of the attack on a GET callback (an attacker page does `location = /signin/callback?code=…&state=…`). Lax blocks cross-site POSTs and subresource loads, neither of which is the threat here. What actually stops the attack is that the attacker cannot obtain or forge the HMAC'd blob and cannot know the sealed `state`. The security holds; the stated reason for it does not, and it is listed among the attributes that "are the reason the callback is allowed to mint a session on a GET." ## 9. `signin.go:235` — "It redirects and never renders" is false `handleCallback` → `completeSignIn` → `a.signinFailed(w)` on a find-or-mint failure or a session-mint failure, which **renders** `signin_failed.html`. No §4 violation (the page carries no account-influenced string), but the sentence is wrong. ## 10. `TestServedCSPAllowsNoInlineScript` does not assert what a browser receives `internal/home/noscript_test.go:147` claims it does — *"the point is what a browser receives, not what a library declares."* It builds its own `rastrillo.Options{Mux: mux, Logger: …}`, not the options `cmd/home/main.go` builds, and it requests exactly one path. **Mutant L (survives):** `w.Header().Set("Content-Security-Policy", "default-src 'self' 'unsafe-inline'")` at the top of `handlePasskeyConfirm` — the one page that actually loads script, and the one place an `unsafe-inline` carve-out would be tempting. Suite green. rastrillo's own `serve.go:383-386` documents that a handler's header *replaces* the default, so this is the supported way to widen it. `TestAppNeverSetsItsOwnCSP` only greps `cmd/home/main.go`. Fix is cheap: loop the served-CSP assertion over every route the app mounts, not just `/signin`. ## 11. The rate limiter is never exercised end to end The plan (Task 5, step 4) asks: *"Test both paths — a request with the header gets its own budget, one without does not silently share a global one."* `TestClientIPNeverCollapsesToOneBucket` tests `clientIP()`'s return value only. No test drives 21 POSTs to `/signin`, so `handleSigninBegin`'s `ErrRateLimited` → 429 branch (`signin.go:169-174`) is dead code as far as the suite is concerned, and nothing proves the budget is actually per-IP rather than global. ## 12. `clientIP`'s carlos citation checks out; the residual is that nothing proves the app is behind the edge I verified `3211100`'s claims at `carlos/platform@97eb53f`: - `stripForwardedHeaders` (`internal/edge/homemap.go:806`) deletes `Forwarded` and `X-Forwarded-{For,Host,Proto}` — called on the failed-hop-verification path; its own doc says the ReverseProxy `Rewrite` deletion is the primary mechanism and this is "belt and braces". Accurate. - `setForwarded` (`homemap.go:910`) calls `pr.SetXForwarded()` first (single address: the dialling peer), and on a *verified* hop replaces it with the inbound chain **verbatim** — with the literal sentence *"Not appended to: the last element has to stay the client"* and the citation to `internal/console/authcli.go:715` (*"the LAST element is the one to take"*). Accurate. So taking the last element is right **behind the carlos edge**. Nothing in the app checks that it is. Run directly (local dev, an internal caller, a misconfigured unit), any client sets `X-Forwarded-For` per request and gets an unlimited budget. Worth one sentence in the comment; not worth code. ## 13. `next.Redirect` is an attacker-influenceable redirect target, and nothing bounds it `signin.go:184` 303s to `next.Redirect`, built in `signin@v0.1.1/flow.go:172` as `https://<c.Server>/oauth/authorize?…`, where `c.Server` is the raw `host=` value from the submitter's `_keymail.<domain>` TXT record (`classify.go:250-268`) — unvalidated, so it may contain `/`, `?`, `@` or a port. Same value becomes `Base: "https://" + server` at `signin.go:65` for the token exchange. Practically bounded: it requires the submitter to type an address at a domain the attacker controls, and `csrf.Protect` blocks the cross-origin POST, so it is a redirect the victim chose. I would not block on it. But the blind-SSRF acknowledgement at `app.go:130-136` and the `classifierTimeout` comment at `signin.go:50-54` both frame the risk as *holding a handler open*; neither says the outbound host is arbitrary and may be an internal address (`host=127.0.0.1:2379`). Say so where it is claimed to be understood. --- # Verified clean Reported so you know they were checked, not skipped: - **`handleVerifyForm` touches no state and does not look the token up.** True at source. `TestGetDoesNotRedeemTheLink` kills the "GET redeems" mutant (both the burn and the session). - **`CompleteKeymail` with `blob == ""`.** `openPending` finds no `.`, returns `ErrPendingInvalid` before any network call. `state` is checked against the sealed `p.State`, and the blob is HMAC'd under `Flow.Key`. A forged code/state with no cookie reaches nothing. Now tested by `f945e66`. - **`sessions.SafeReturn`** (`sessions.go:458`) is path-only: rejects anything not starting `/`, rejects `//`, `\`, and control characters. No open redirect through the Gate's `return_to`. - **`takePending`'s write-then-read ordering** is correct on every path, including error paths. - **Templates: no XSS.** I rendered every `{{…}}` with `" onload="alert(1)` and `<img src=x onerror=alert(1)>`. `bodyattrs` (`data-rpid`/`data-userid`/`data-username`), the hidden `Token` field and `{{range .Codes}}<code>` all escape correctly — html/template resolves the `<body{{block "bodyattrs" .}}>` context properly. - **`TestRPIDIsExactlyTheCredentialHost` is a real guard.** Killed all three mutants: `LegacyRPID: "oficina.works"`, `Origin` widened to the apex, and `rpID()` dropping the leading label. - **The enrol freshness gate works.** 24-hour session: `/passkey/enrol` → 303, `register/begin` → 403, `register/finish` → 403. And `903eb7b`'s claim *"Verified by breaking it: swapping RequireFresh for Require fails"* is **true** — `r.Use(a.sessions.Require)` fails `TestEnrolmentNeedsAFreshSession`. (My first attempt at this mutant did not compile and I mis-scored it; corrected.) - **`codesFreshness` widened to 30 days** is killed; **`enrolFreshness` widened to 30 days** is killed. - **Doc claims about other packages, checked at the pinned v0.22.0** (note: `go.mod` pins **v0.22.0**, not the v0.26.0 sitting beside it in the module cache — I re-read the relevant code at v0.22.0): - `rastrillo/auth` does store the address as the session subject (`auth/handlers.go:136` `Subject: id.Address`) — so `completeSignIn`'s justification at `signin.go:240-244` is true. - `rastrillo.Serve` does register `GET /healthz` on the outer mux (`serve.go:458`) and mounts `opts.Mux` at `/` (`serve.go:473`), so `app.go:139-145` is true. - `signin`'s defaults are as documented: `/auth/verify` (`flow.go:191`), `/auth/callback` (`keymail.go:36`), `PendingTTL` 10m (`flow.go:165`). - **`AGENTS.md` and the spec's §3.1 note** are honest: the AGENTS.md edit replaces a status sentence with a rule, and the new spec block openly records that `blindIndex` implements ruling 3 and that §3.1's expensive-KDF sentence is therefore currently false about the code. That is the right way to file it. - **`namesAnAddress`'s claim** at `guardstest/noaddress_test.go` that its word list *"matches the runtime twin's list in internal/home/db_test.go"* is true — both are `address, addresses, addr, email, emails, mail, recipient, recipients`. - Inline `onclick=` handlers are not caught by the sweep, but rastrillo's `default-src 'self'` makes them inert. Not filed. --- # CHANGES NEEDED Four gates do not guard what their comments say they guard (findings 1-4), one load-bearing code path has no success-path coverage at all (5), one freshness pair contradicts itself and the JS flow that walks it (6), and the package doc says sign-in is not implemented (7). Ranked by what I would fix first: 1. **Finding 1** — replace the grep with a behavioural test that an enrolled person's *keymail* callback lands on `confirmPath` and mints nothing. Right now a one-line spelling change removes the second factor from that door with the suite green. 2. **Finding 6** — the enrol/codes window ordering, because it hits real people at the moment they have a passkey and no recovery codes. 3. **Findings 2, 3, 4** — the three guards whose comments promise more than the code delivers. All three are cheap: template-source scanning rather than rendered-output scanning for the sweep; compare full responses (headers included) rather than token-stripped bodies for the oracle. 4. **Finding 5** — one happy-path keymail test would kill mutants O and P together. 5. **Finding 7** and the rest of the NITs.</result> <usage><subagent_tokens>223064</subagent_tokens><tool_uses>70</tool_uses><duration_ms>924285</duration_ms></usage> </task-notification>
diff --git a/cmd/home/main.go b/cmd/home/main.go| index 2e0c982..27df38a 100644 |
| --- a/cmd/home/main.go |
| +++ b/cmd/home/main.go |
| @@ -1,9 +1,9 @@ |
| // Command home is the Oficina suite home binary. |
| // |
| -// It serves internal/home's one placeholder page and, as of §13 step 2, |
| -// refuses to start without the configuration sign-in will need. See |
| -// docs/superpowers/specs/2026-08-30-home-design.md and |
| -// internal/home/page.go. |
| +// It serves sign-in, the passkey second factor and recovery codes (§13 |
| +// step 2), plus a placeholder front page, and refuses to start without |
| +// the configuration those need. See |
| +// docs/superpowers/specs/2026-08-30-home-design.md. |
| // |
| // The platform activation contract has no $PORT: rastrillo.Resolve and |
| // Serve own it end to end — activation argv, LISTEN_FDS, |
diff --git a/internal/home/app.go b/internal/home/app.go| index 05ba127..e5562cb 100644 |
| --- a/internal/home/app.go |
| +++ b/internal/home/app.go |
| @@ -4,11 +4,14 @@ |
| // docs/superpowers/specs/2026-08-30-home-design.md (the design) for |
| // what this service is and, just as importantly, is not. |
| // |
| -// This package is not yet the home: it wires the platform contract, |
| -// validates the configuration sign-in needs (keys.go) and seals values |
| -// at rest (seal/), but serves only one placeholder page. Sign-in, |
| -// directory, assertions and provisioning are not implemented yet. See |
| -// page.go. |
| +// §13 step 2 is built: hand-rolled sign-in on keymaildev/signin with the |
| +// Keymail upgrade, passkeys as an optional second factor with recovery |
| +// codes, sealed link state, the person ref and the blind index. The |
| +// front page is still a placeholder (page.go). |
| +// |
| +// Not built: the directory and its calls, assertion minting and the |
| +// redirect protocol, provisioning, the rekey journal. Those are §13 |
| +// steps 3 onwards. |
| package home |
| import ( |
diff --git a/internal/home/keymail_test.go b/internal/home/keymail_test.go| new file mode 100644 |
| index 0000000..4f46782 |
| --- /dev/null |
| +++ b/internal/home/keymail_test.go |
| @@ -0,0 +1,168 @@ |
| +package home |
| + |
| +import ( |
| + "net/http" |
| + "strings" |
| + "testing" |
| +) |
| + |
| +// The control, and the proof the stub actually drives the Keymail |
| +// upgrade: an unenrolled person completes the round trip and gets a |
| +// session whose method says keymail. |
| +func TestKeymailSignInMintsASession(t *testing.T) { |
| + app := newTestApp(t) |
| + const addr = "person@keymail.invalid" |
| + stub := newKeymailStub(t, app, addr) |
| + |
| + pending, state := beginKeymail(t, app, addr) |
| + w := callback(t, app, "code-1", state, pending) |
| + |
| + if sessionCookie(w) == nil { |
| + t.Fatalf("the keymail round trip minted no session (status %d, location %q)", |
| + w.Code, w.Header().Get("Location")) |
| + } |
| + |
| + var subject, method string |
| + if err := app.db.Writer(). |
| + QueryRow("SELECT subject, method FROM sessions").Scan(&subject, &method); err != nil { |
| + t.Fatal(err) |
| + } |
| + if !ValidPersonRef(subject) { |
| + t.Errorf("session subject is %q, not a person ref", subject) |
| + } |
| + if method != "keymail" { |
| + t.Errorf("session method is %q, want keymail", method) |
| + } |
| + _ = stub |
| +} |
| + |
| +// FINDING 1, and the reason this file exists. The Gate must cover the |
| +// keymail door as well as the magic-link one. |
| +// |
| +// The previous guard was a text grep for "a.sessions.SignIn(" in the |
| +// package. A reviewer removed the Gate from this path by writing the |
| +// mint with a local variable instead, and the entire suite stayed green |
| +// — so the second factor could be taken off the keymail door by a |
| +// one-line spelling change. This asserts the behaviour instead. |
| +func TestKeymailSignInIsGatedForAnEnrolledPerson(t *testing.T) { |
| + app := newTestApp(t) |
| + const addr = "person@keymail.invalid" |
| + newKeymailStub(t, app, addr) |
| + |
| + // Enrol first, through the magic-link path. |
| + pending, state := beginKeymail(t, app, addr) |
| + first := callback(t, app, "code-1", state, pending) |
| + cookie := sessionCookie(first) |
| + if cookie == nil { |
| + t.Fatalf("setup: no session from the first keymail sign-in (status %d)", first.Code) |
| + } |
| + enrol(t, app, cookie) |
| + |
| + // Now sign in again through keymail alone. |
| + pending2, state2 := beginKeymail(t, app, addr) |
| + w := callback(t, app, "code-2", state2, pending2) |
| + |
| + if got := w.Header().Get("Location"); got != confirmPath { |
| + t.Errorf("the keymail callback went to %q, want %q — the Gate did not take over "+ |
| + "on this path, so one factor opened an enrolled account", got, confirmPath) |
| + } |
| + if c := sessionCookie(w); c != nil { |
| + t.Fatal("the keymail callback minted a full session for an enrolled person") |
| + } |
| +} |
| + |
| +// FINDING 5, mutant O. "It is cleared on read, so a blob is good for |
| +// exactly one callback" was asserted in a comment and by nothing else. |
| +func TestPendingCookieIsClearedOnRead(t *testing.T) { |
| + app := newTestApp(t) |
| + const addr = "person@keymail.invalid" |
| + newKeymailStub(t, app, addr) |
| + |
| + pending, state := beginKeymail(t, app, addr) |
| + w := callback(t, app, "code-1", state, pending) |
| + |
| + var cleared bool |
| + for _, c := range (&http.Response{Header: w.Header()}).Cookies() { |
| + if c.Name == pendingCookie && c.MaxAge < 0 { |
| + cleared = true |
| + } |
| + } |
| + if !cleared { |
| + t.Error("the callback did not clear the pending cookie, so the blob is replayable") |
| + } |
| + |
| + // Replaying the whole request — same blob, same code — gets nothing. |
| + // Note what does the work: NOT the cookie clearing, which only |
| + // touches one browser's jar, but the federation server redeeming the |
| + // authorization code once. The first version of this test asserted |
| + // the clearing was what made the round trip single-use, and it |
| + // failed, because it is not. |
| + again := callback(t, app, "code-1", state, pending) |
| + if sessionCookie(again) != nil { |
| + t.Error("replaying the round trip minted a second session") |
| + } |
| + var n int |
| + if err := app.db.Writer().QueryRow("SELECT count(*) FROM sessions").Scan(&n); err != nil { |
| + t.Fatal(err) |
| + } |
| + if n != 1 { |
| + t.Errorf("%d sessions after a replayed round trip, want 1", n) |
| + } |
| +} |
| + |
| +// FINDING 5, mutant P. Every attribute on the pending cookie is |
| +// load-bearing — they are what the "the callback may mint on a GET" |
| +// argument rests on — and none of them was asserted, because no test |
| +// ever took the keymail branch. |
| +func TestPendingCookieAttributes(t *testing.T) { |
| + app := newTestApp(t) |
| + const addr = "person@keymail.invalid" |
| + newKeymailStub(t, app, addr) |
| + |
| + pending, _ := beginKeymail(t, app, addr) |
| + |
| + if !strings.HasPrefix(pending.Name, "__Host-") { |
| + t.Errorf("pending cookie is %q; the __Host- prefix is what pins it to this exact "+ |
| + "origin with no Domain, so no sibling host can set or read it", pending.Name) |
| + } |
| + if pending.Domain != "" { |
| + t.Errorf("pending cookie carries Domain=%q; it must be host-only", pending.Domain) |
| + } |
| + if pending.Path != "/" { |
| + t.Errorf("pending cookie Path=%q; __Host- requires /", pending.Path) |
| + } |
| + if !pending.HttpOnly { |
| + t.Error("pending cookie is not HttpOnly, so script can read the blob") |
| + } |
| + if !pending.Secure { |
| + t.Error("pending cookie is not Secure, which __Host- also requires") |
| + } |
| + if pending.SameSite != http.SameSiteLaxMode { |
| + t.Errorf("pending cookie SameSite=%v, want Lax: Strict would break the keymail "+ |
| + "return itself, which is a top-level navigation from another origin", |
| + pending.SameSite) |
| + } |
| +} |
| + |
| +// A callback carrying a valid blob but the wrong state must mint |
| +// nothing. State is the browser binding; without it an attacker could |
| +// fix their own completed round trip onto a victim's browser. |
| +func TestCallbackWithWrongStateMintsNothing(t *testing.T) { |
| + app := newTestApp(t) |
| + const addr = "person@keymail.invalid" |
| + newKeymailStub(t, app, addr) |
| + |
| + pending, _ := beginKeymail(t, app, addr) |
| + w := callback(t, app, "code-1", "not-the-sealed-state", pending) |
| + |
| + if sessionCookie(w) != nil { |
| + t.Fatal("a mismatched state still minted a session") |
| + } |
| + var n int |
| + if err := app.db.Writer().QueryRow("SELECT count(*) FROM sessions").Scan(&n); err != nil { |
| + t.Fatal(err) |
| + } |
| + if n != 0 { |
| + t.Fatalf("a mismatched state created %d sessions", n) |
| + } |
| +} |
diff --git a/internal/home/keymailstub_test.go b/internal/home/keymailstub_test.go| new file mode 100644 |
| index 0000000..90d7d4d |
| --- /dev/null |
| +++ b/internal/home/keymailstub_test.go |
| @@ -0,0 +1,181 @@ |
| +package home |
| + |
| +import ( |
| + "context" |
| + "encoding/json" |
| + "net" |
| + "net/http" |
| + "net/http/httptest" |
| + "net/url" |
| + "strings" |
| + "sync" |
| + "testing" |
| + "time" |
| + |
| + "github.com/keymaildev/signin" |
| +) |
| + |
| +// keymailStub is a federation server good enough to drive the whole |
| +// Keymail upgrade end to end: classification, the authorize redirect and |
| +// the token exchange. |
| +// |
| +// It exists because the Keymail path had NO test at all. Everything |
| +// about it was asserted in comments — that the Gate covers it, that the |
| +// pending cookie is cleared on read, that its attributes are |
| +// load-bearing — and a review proved each of those claims survivable by |
| +// mutation precisely because nothing ever drove the path. |
| +type keymailStub struct { |
| + srv *httptest.Server |
| + host string |
| + address string |
| + // authTime is what the server reports as the moment an |
| + // authenticator gesture happened. Zero means "no gesture", which |
| + // callers must treat as the weaker of the two methods. |
| + authTime time.Time |
| + |
| + mu sync.Mutex |
| + spent map[string]bool |
| +} |
| + |
| +// newKeymailStub wires app.flow to talk to a local federation server for |
| +// one address's domain. |
| +func newKeymailStub(t *testing.T, app *testApp, address string) *keymailStub { |
| + t.Helper() |
| + _, domain, err := signin.SplitAddress(address) |
| + if err != nil { |
| + t.Fatalf("SplitAddress(%q): %v", address, err) |
| + } |
| + |
| + k := &keymailStub{ |
| + address: address, |
| + authTime: time.Now().Truncate(time.Second), |
| + spent: map[string]bool{}, |
| + } |
| + |
| + mux := http.NewServeMux() |
| + mux.HandleFunc("/.well-known/keymail", func(w http.ResponseWriter, r *http.Request) { |
| + writeJSON(t, w, map[string]any{"version": "1", "reauth": false}) |
| + }) |
| + mux.HandleFunc("/api/federation/lookup", func(w http.ResponseWriter, r *http.Request) { |
| + // A claimed inbox is a 200; anything else is "not keymail". |
| + if r.URL.Query().Get("addr") != strings.ToLower(k.address) { |
| + http.Error(w, "no such inbox", http.StatusNotFound) |
| + return |
| + } |
| + writeJSON(t, w, map[string]any{"ok": true}) |
| + }) |
| + mux.HandleFunc("/api/oauth/token", func(w http.ResponseWriter, r *http.Request) { |
| + // A real federation server redeems an authorization code once, |
| + // and once PER CODE — a fresh round trip issues a fresh code. |
| + // The stub must model both halves: spending globally made a |
| + // second legitimate sign-in fail, and not spending at all let a |
| + // replay through. Both versions were written before this |
| + // comment. |
| + var body struct { |
| + Code string `json:"code"` |
| + } |
| + if err := json.NewDecoder(r.Body).Decode(&body); err != nil { |
| + http.Error(w, "bad request", http.StatusBadRequest) |
| + return |
| + } |
| + k.mu.Lock() |
| + used := k.spent[body.Code] |
| + k.spent[body.Code] = true |
| + k.mu.Unlock() |
| + if used { |
| + http.Error(w, "code already redeemed", http.StatusBadRequest) |
| + return |
| + } |
| + writeJSON(t, w, map[string]any{ |
| + "access_token": "stub-access-token", |
| + "token_type": "bearer", |
| + "address": k.address, |
| + "auth_time": k.authTime.Unix(), |
| + }) |
| + }) |
| + |
| + k.srv = httptest.NewServer(mux) |
| + t.Cleanup(k.srv.Close) |
| + k.host = strings.TrimPrefix(k.srv.URL, "http://") |
| + |
| + // The delegation record keymail's own dnsFedHost would publish. |
| + app.flow.Classifier.Scheme = "http" |
| + app.flow.Classifier.LookupTXT = func(_ context.Context, name string) ([]string, error) { |
| + if name == "_keymail."+domain { |
| + return []string{"v=1 host=" + k.host}, nil |
| + } |
| + return nil, &net.DNSError{Err: "no such host", Name: name, IsNotFound: true} |
| + } |
| + // newFlow builds Base as https://<server>; the stub is plain http. |
| + app.flow.Keymail = func(server string) *signin.Keymail { |
| + return &signin.Keymail{ |
| + Base: "http://" + server, |
| + Origin: Origin, |
| + RedirectPath: callbackPath, |
| + } |
| + } |
| + return k |
| +} |
| + |
| +func writeJSON(t *testing.T, w http.ResponseWriter, v any) { |
| + t.Helper() |
| + w.Header().Set("Content-Type", "application/json") |
| + if err := json.NewEncoder(w).Encode(v); err != nil { |
| + t.Errorf("stub encode: %v", err) |
| + } |
| +} |
| + |
| +// beginKeymail runs POST /signin for an address the stub claims, and |
| +// returns the pending cookie and the state the flow sealed into it. |
| +func beginKeymail(t *testing.T, app *testApp, address string) (*http.Cookie, string) { |
| + t.Helper() |
| + rec := signIn(t, app, address) |
| + if rec.Code != http.StatusSeeOther { |
| + t.Fatalf("POST /signin = %d, want 303", rec.Code) |
| + } |
| + loc := rec.Header().Get("Location") |
| + if !strings.Contains(loc, "/oauth/authorize") { |
| + t.Fatalf("sign-in went to %q, which is not the keymail authorize URL — "+ |
| + "classification did not take the keymail branch", loc) |
| + } |
| + if _, sent := app.mailer.last(); sent { |
| + t.Fatal("the keymail branch sent a magic link as well") |
| + } |
| + |
| + u, err := url.Parse(loc) |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + state := u.Query().Get("state") |
| + if state == "" { |
| + t.Fatal("the authorize URL carries no state") |
| + } |
| + |
| + var pending *http.Cookie |
| + for _, c := range (&http.Response{Header: rec.Header()}).Cookies() { |
| + if c.Name == pendingCookie { |
| + pending = c |
| + } |
| + } |
| + if pending == nil { |
| + t.Fatalf("no %s cookie was set", pendingCookie) |
| + } |
| + return pending, state |
| +} |
| + |
| +// callback drives GET /signin/callback with the given cookies. The code |
| +// is a parameter because a real authorization code is single use: two |
| +// round trips carry two codes, and a replay carries the same one twice. |
| +func callback(t *testing.T, app *testApp, code, state string, cookies ...*http.Cookie) *httptest.ResponseRecorder { |
| + t.Helper() |
| + req := httptest.NewRequest(http.MethodGet, |
| + callbackPath+"?code="+url.QueryEscape(code)+"&state="+url.QueryEscape(state), nil) |
| + for _, c := range cookies { |
| + if c != nil { |
| + req.AddCookie(c) |
| + } |
| + } |
| + w := httptest.NewRecorder() |
| + app.Router().ServeHTTP(w, req) |
| + return w |
| +} |
diff --git a/internal/home/noscript_test.go b/internal/home/noscript_test.go| index 1c89de5..abf53ff 100644 |
| --- a/internal/home/noscript_test.go |
| +++ b/internal/home/noscript_test.go |
| @@ -2,9 +2,11 @@ package home |
| import ( |
| "bytes" |
| + "io/fs" |
| "net/http" |
| "net/http/httptest" |
| "os" |
| + "path" |
| "regexp" |
| "slices" |
| "sort" |
| @@ -13,6 +15,7 @@ import ( |
| "github.com/carlosframework/rastrillo" |
| "github.com/carlosframework/rastrillo/ui" |
| + "github.com/go-chi/chi/v5" |
| ) |
| // scriptAllowed is ruling 21's carve-out, by TEMPLATE and by SRC. |
| @@ -32,11 +35,22 @@ var scriptAllowed = map[string][]string{ |
| "passkey_enrol.html": {"/static/passkey.js"}, |
| } |
| -// scriptTag matches a <script ...> opening tag and captures its |
| -// attributes. Hand-rolled rather than pulling in an HTML parser as a |
| -// dependency: the input is this repo's own templates, and the shapes it |
| -// has to survive are the ones we write. |
| -var scriptTag = regexp.MustCompile(`(?is)<script([^>]*)>(.*?)</script>`) |
| +// scriptTag matches a complete <script ...>...</script>. |
| +// |
| +// The closing pattern tolerates whitespace — `</script >` is valid HTML |
| +// that every browser accepts, and the first version of this regex |
| +// required the literal `</script>` and so walked straight past a page |
| +// loading an arbitrary script. The justification for hand-rolling was |
| +// "the shapes it has to survive are the ones we write", which is exactly |
| +// the assumption that failed: nobody writes `</script >` on purpose, |
| +// which is why nobody would notice. |
| +var scriptTag = regexp.MustCompile(`(?is)<script\b([^>]*)>(.*?)</\s*script\s*>`) |
| + |
| +// scriptOpen matches just the opening tag, so a <script> with no |
| +// matching close can be counted and refused rather than silently |
| +// skipped. |
| +var scriptOpen = regexp.MustCompile(`(?is)<script\b`) |
| + |
| var srcAttr = regexp.MustCompile(`(?i)\bsrc\s*=\s*"([^"]*)"`) |
| // scriptSrcs returns one entry per <script> in b: its src, or the empty |
| @@ -59,13 +73,18 @@ func scriptSrcs(b []byte) []string { |
| return out |
| } |
| -// templateNames is every page this app can render. |
| +// templateNames is every template file this app ships, including the |
| +// layout — the sweep reads SOURCE, so the layout is a page like any |
| +// other and can carry a script too. |
| func templateNames(t *testing.T) []string { |
| t.Helper() |
| - app := newTestApp(t) |
| - names := make([]string, 0, len(app.renderer.pages)) |
| - for name := range app.renderer.pages { |
| - names = append(names, name) |
| + files, err := fs.Glob(templatesFS, "templates/*.html") |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + names := make([]string, 0, len(files)) |
| + for _, f := range files { |
| + names = append(names, path.Base(f)) |
| } |
| sort.Strings(names) |
| if len(names) == 0 { |
| @@ -74,9 +93,24 @@ func templateNames(t *testing.T) []string { |
| return names |
| } |
| -// sampleData is enough data for each page to render its script-bearing |
| -// branches. Every branch matters: a page that only emits its <script> |
| -// inside an {{if}} would otherwise slip past. |
| +// templateSource is one template file's raw text. |
| +func templateSource(t *testing.T, name string) []byte { |
| + t.Helper() |
| + b, err := templatesFS.ReadFile("templates/" + name) |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + return b |
| +} |
| + |
| +// sampleData is enough data to render each page for the tests that need |
| +// a rendered page. |
| +// |
| +// It is deliberately NOT what the script sweep uses. A fixed key set |
| +// cannot reach a branch keyed on something it does not carry — on a map, |
| +// {{if .Debug}} resolves to nil with no error — so a sweep built on it |
| +// passes by not looking. The sweep reads template source instead. This |
| +// stays for the recovery-form test, which needs real output. |
| func sampleData(name string) []any { |
| common := map[string]any{ |
| "RPID": "id.oficina.works", "LegacyRPID": "", |
| @@ -96,32 +130,45 @@ func sampleData(name string) []any { |
| // TestNoScriptTagOutsideTheAllowlist is §11, made mechanical: every |
| // screen works with scripts off, and ruling 21 carves out only the |
| // passkey screens. |
| +// |
| +// 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 |
| +// reached "every branch". It could not: on a map, {{if .Debug}} |
| +// resolves to nil with no error, so a <script> inside a branch the |
| +// fixture never satisfies rendered as nothing and the sweep passed by |
| +// not looking. Any key a future template introduced was invisible to it |
| +// by construction. Source has no branches to miss. |
| func TestNoScriptTagOutsideTheAllowlist(t *testing.T) { |
| - app := newTestApp(t) |
| for _, name := range templateNames(t) { |
| - for _, data := range sampleData(name) { |
| - var buf bytes.Buffer |
| - if err := app.renderer.pages[name].ExecuteTemplate(&buf, "layout", data); err != nil { |
| - t.Fatalf("render %s: %v", name, err) |
| - } |
| - srcs := scriptSrcs(buf.Bytes()) |
| - if len(srcs) == 0 { |
| - continue // no script at all: the common case |
| - } |
| - permitted, ok := scriptAllowed[name] |
| - if !ok { |
| - t.Errorf("%s renders a <script>; §11 says every screen works with scripts "+ |
| - "off and ruling 21 carves out only the passkey screens", name) |
| - continue |
| - } |
| - for _, src := range srcs { |
| - switch { |
| - case src == "": |
| - t.Errorf("%s has an INLINE <script>; the carve-out is for static "+ |
| - "same-origin files, and the default CSP forbids inline anyway", name) |
| - case !slices.Contains(permitted, src): |
| - t.Errorf("%s loads %q, which is not in ruling 21's allowlist", name, src) |
| - } |
| + src := templateSource(t, name) |
| + |
| + // A <script> with no matching close is refused rather than |
| + // skipped: an unbalanced tag is either a typo or an evasion, and |
| + // neither should pass silently. |
| + if opens, full := len(scriptOpen.FindAll(src, -1)), len(scriptTag.FindAll(src, -1)); opens != full { |
| + t.Errorf("%s has %d <script opening tags but %d complete elements; "+ |
| + "an unbalanced tag cannot be swept", name, opens, full) |
| + continue |
| + } |
| + |
| + srcs := scriptSrcs(src) |
| + if len(srcs) == 0 { |
| + continue // no script at all: the common case |
| + } |
| + permitted, ok := scriptAllowed[name] |
| + if !ok { |
| + t.Errorf("%s carries a <script>; §11 says every screen works with scripts "+ |
| + "off and ruling 21 carves out only the passkey screens", name) |
| + continue |
| + } |
| + for _, srcAttr := range srcs { |
| + switch { |
| + case srcAttr == "": |
| + t.Errorf("%s has an INLINE <script>; the carve-out is for static "+ |
| + "same-origin files, and the default CSP forbids inline anyway", name) |
| + case !slices.Contains(permitted, srcAttr): |
| + t.Errorf("%s loads %q, which is not in ruling 21's allowlist", name, srcAttr) |
| } |
| } |
| } |
| @@ -135,8 +182,11 @@ func TestScriptSrcsSeesInlineAndSourced(t *testing.T) { |
| <script>alert(1)</script> |
| <script type="module" src="/static/other.js"></script> |
| <script src="/static/x.js">alert(2)</script> |
| + <script src="/static/spaced.js"></script > |
| + <SCRIPT SRC="/static/shouty.js"></SCRIPT> |
| `)) |
| - want := []string{"/static/passkey.js", "", "/static/other.js", ""} |
| + want := []string{"/static/passkey.js", "", "/static/other.js", "", |
| + "/static/spaced.js", "/static/shouty.js"} |
| if !slices.Equal(got, want) { |
| t.Errorf("scriptSrcs = %q, want %q", got, want) |
| } |
| @@ -160,27 +210,51 @@ func TestServedCSPAllowsNoInlineScript(t *testing.T) { |
| t.Cleanup(func() { cleanup() }) |
| } |
| - rec := httptest.NewRecorder() |
| - h.ServeHTTP(rec, httptest.NewRequest(http.MethodGet, "/signin", nil)) |
| - |
| - csp := rec.Header().Get("Content-Security-Policy") |
| - if csp == "" { |
| - t.Fatal("no Content-Security-Policy header was served") |
| + // EVERY route, not just one. rastrillo's securityHeaders sets the |
| + // default and a handler's own header REPLACES it, so an |
| + // unsafe-inline carve-out added to the one page that actually loads |
| + // script — the tempting place to put it — would never be seen by a |
| + // check that only requests /signin. |
| + routes, ok := app.Router().(chi.Routes) |
| + if !ok { |
| + t.Fatalf("Router() is %T, which does not implement chi.Routes", app.Router()) |
| } |
| - for _, directive := range strings.Split(csp, ";") { |
| - d := strings.TrimSpace(directive) |
| - name, _, _ := strings.Cut(d, " ") |
| - // Inline STYLES are allowed on purpose: ui's partials set style |
| - // attributes. Inline SCRIPT is what §11 and ruling 21 are about. |
| - if name == "style-src" { |
| + 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, "/*", "")) |
| + 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)) |
| + } |
| + |
| + for _, p := range paths { |
| + rec := httptest.NewRecorder() |
| + h.ServeHTTP(rec, httptest.NewRequest(http.MethodGet, p, nil)) |
| + |
| + csp := rec.Header().Get("Content-Security-Policy") |
| + if csp == "" { |
| + t.Errorf("%s served no Content-Security-Policy header", p) |
| continue |
| } |
| - if strings.Contains(d, "'unsafe-inline'") || strings.Contains(d, "'unsafe-eval'") { |
| - t.Errorf("CSP directive %q permits inline or eval'd script", d) |
| + for _, directive := range strings.Split(csp, ";") { |
| + d := strings.TrimSpace(directive) |
| + name, _, _ := strings.Cut(d, " ") |
| + // Inline STYLES are allowed on purpose: ui's partials set |
| + // style attributes. Inline SCRIPT is what §11 and ruling 21 |
| + // are about. |
| + if name == "style-src" { |
| + continue |
| + } |
| + if strings.Contains(d, "'unsafe-inline'") || strings.Contains(d, "'unsafe-eval'") { |
| + t.Errorf("%s: CSP directive %q permits inline or eval'd script", p, d) |
| + } |
| + } |
| + if !strings.Contains(csp, "default-src 'self'") { |
| + t.Errorf("%s: CSP %q does not restrict script to same-origin", p, csp) |
| } |
| - } |
| - if !strings.Contains(csp, "default-src 'self'") { |
| - t.Errorf("CSP %q does not restrict script to same-origin", csp) |
| } |
| } |
diff --git a/internal/home/passkey.go b/internal/home/passkey.go| index f5791a1..1e86abc 100644 |
| --- a/internal/home/passkey.go |
| +++ b/internal/home/passkey.go |
| @@ -38,7 +38,16 @@ const confirmPath = "/passkey/confirm" |
| // will accept a recovery code instead. Asserted in |
| // TestRecoveryCodeProducesAFreshSession so the fact cannot change |
| // silently. |
| -const codesFreshness = 5 * time.Minute |
| +// 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 |
| +// re-authenticate at the exact moment they hold a passkey and zero |
| +// recovery codes — the worst possible moment, and precisely the outcome |
| +// the sentence above says this value was chosen to avoid. That sentence |
| +// was true until enrolFreshness was added at a larger value and did not |
| +// revisit it. TestFreshnessWindowsDoNotStrandAnEnroller pins the |
| +// ordering. |
| +const codesFreshness = enrolFreshness |
| // enrolFreshness is how recent a session must be to ADD a passkey. |
| // |
diff --git a/internal/home/passkey_test.go b/internal/home/passkey_test.go| index 8eb6d65..fde7d45 100644 |
| --- a/internal/home/passkey_test.go |
| +++ b/internal/home/passkey_test.go |
| @@ -127,16 +127,34 @@ func redeemAfterSignIn(t *testing.T, app *testApp, addr string) *httptest.Respon |
| return redeem(t, app, capturedLink(t, app)) |
| } |
| -// The Gate is on both sign-in paths because there is exactly one place |
| -// that mints a session, not because two call sites were remembered. |
| -// This is the guard: a future handler that calls sessions.SignIn |
| -// directly would bypass the second factor silently, and this makes that |
| -// loud. |
| -func TestSessionsSignInHasExactlyOneCallSite(t *testing.T) { |
| +// A TRIPWIRE, not a proof, and the difference matters — an earlier |
| +// version of this comment claimed it was a proof and was wrong twice |
| +// over. |
| +// |
| +// It was wrong about what it catches: it is a substring match on one |
| +// spelling, and a reviewer removed the Gate from the keymail path by |
| +// assigning a.sessions to a local first. The suite stayed green. The |
| +// real guards are behavioural and live in keymail_test.go and |
| +// passkey_test.go: an enrolled person must land on confirmPath and mint |
| +// nothing, down BOTH doors. |
| +// |
| +// And it was wrong about the invariant: the deployed app mints sessions |
| +// in FOUR places, not one. This package's completeSignIn is one; |
| +// passkey.SignInFinish, SignInRecovery and StepUpFinish are the others, |
| +// all mounted on this router. Those three are legitimate — each one IS |
| +// the second factor, so gating them would be circular — but "exactly |
| +// one call site" was never true of the app, only of this package's own |
| +// source. |
| +// |
| +// What it still buys: if somebody adds a fifth mint to THIS package, it |
| +// says so. That is worth keeping as long as nobody mistakes it for the |
| +// thing that makes the Gate safe. |
| +func TestThisPackageMintsSessionsInOnePlace(t *testing.T) { |
| got := grepPackage(t, "a.sessions.SignIn(") |
| if len(got) != 1 { |
| - t.Errorf("sessions.SignIn is called in %d places (%v); it must be exactly one, "+ |
| - "or a caller can mint a session without passing passkey.Gate", len(got), got) |
| + t.Errorf("this package mints a session in %d places (%v), want 1. If that is "+ |
| + "deliberate, the new one needs its own behavioural test that passkey.Gate "+ |
| + "covers it — see TestKeymailSignInIsGatedForAnEnrolledPerson.", len(got), got) |
| } |
| gate := grepPackage(t, "a.passkeys.Gate(") |
| if len(gate) != 1 { |
| @@ -350,3 +368,45 @@ func TestEnrolmentNeedsAFreshSession(t *testing.T) { |
| t.Errorf("GET /passkey/enrol with a stale session = %d, want a redirect to re-auth", w.Code) |
| } |
| } |
| + |
| +// The codes window must be at least as wide as the enrolment window. |
| +// |
| +// passkey.js sends the browser to /passkey/codes the moment registration |
| +// finishes. If a session can enrol but cannot then read its codes, the |
| +// person is bounced to re-authenticate holding a passkey and zero |
| +// recovery codes. That was real: enrolFreshness was 10 minutes and |
| +// codesFreshness 5, so a 7-minute-old session enrolled and was then |
| +// refused. |
| +func TestFreshnessWindowsDoNotStrandAnEnroller(t *testing.T) { |
| + if codesFreshness < enrolFreshness { |
| + t.Fatalf("codesFreshness (%s) is shorter than enrolFreshness (%s): a session can "+ |
| + "enrol a passkey and then be refused its recovery codes", |
| + codesFreshness, enrolFreshness) |
| + } |
| +} |
| + |
| +// And the same thing end to end, at a session age that used to fall in |
| +// the gap: enrol, then read the codes page, without re-authenticating. |
| +func TestEnrolThenReadCodesWithoutReauthenticating(t *testing.T) { |
| + app := newTestApp(t) |
| + cookie := signInFully(t, app, "person@example.invalid") |
| + |
| + // Seven minutes old: inside the old enrol window, outside the old |
| + // codes window. |
| + aged := time.Now().Add(-7 * time.Minute).UTC().Format(time.RFC3339) |
| + if _, err := app.db.Writer().Exec( |
| + `UPDATE sessions SET auth_time = ?, created_at = ?`, aged, aged); err != nil { |
| + t.Fatal(err) |
| + } |
| + |
| + enrol(t, app, cookie) |
| + |
| + req := httptest.NewRequest(http.MethodGet, "/passkey/codes", nil) |
| + req.AddCookie(cookie) |
| + w := httptest.NewRecorder() |
| + app.Router().ServeHTTP(w, req) |
| + if w.Code != http.StatusOK { |
| + t.Errorf("a session that just enrolled got %d from /passkey/codes; it is holding "+ |
| + "a passkey and no recovery codes", w.Code) |
| + } |
| +} |
diff --git a/internal/home/signin.go b/internal/home/signin.go| index abbc298..ad29abe 100644 |
| --- a/internal/home/signin.go |
| +++ b/internal/home/signin.go |
| @@ -35,10 +35,22 @@ const callbackPath = "/signin/callback" |
| // callback is allowed to mint a session on a GET where the magic link is |
| // not. __Host- pins it to this exact origin with no Domain and Path=/, |
| // so no sibling host can set or read it. HttpOnly keeps script out. |
| -// SameSite=Lax is what stops a cross-site request carrying it — Strict |
| -// would break the keymail return itself, which is a top-level navigation |
| -// from another origin. And it is cleared on read, so a blob is good for |
| -// exactly one callback. |
| +// SameSite=Lax rather than Strict because Strict would break the keymail |
| +// return itself, which is a top-level navigation from another origin. |
| +// Lax does NOT stop this attack shape — it sends the cookie on exactly |
| +// that navigation — and an earlier version of this comment claimed it |
| +// did. What stops an attacker is that the blob is HMAC'd under a config |
| +// key they do not have, and that the sealed state must match. |
| +// |
| +// It is cleared on read. That is worth doing and it is NOT what makes a |
| +// blob single-use: clearing touches one browser's cookie jar and the |
| +// blob stays a valid self-contained value until it expires. An earlier |
| +// version of this comment said "cleared on read, so a blob is good for |
| +// exactly one callback", and a test written to check it found the |
| +// replay went through. What actually makes the round trip single-use is |
| +// the authorization code, which the federation server redeems once — |
| +// standard OAuth, and the test stub models it because a stub that did |
| +// not was how the false claim survived. |
| const pendingCookie = "__Host-oficina-signin" |
| // pendingTTL bounds how long a half-finished keymail round trip stays |
| @@ -47,10 +59,19 @@ const pendingCookie = "__Host-oficina-signin" |
| // cookie that outlives the blob it carries. |
| const pendingTTL = 10 * time.Minute |
| -// classifierTimeout bounds the classifier's outbound calls. It makes DNS |
| -// and HTTPS requests to whatever domain the person typed, so an |
| -// unbounded client is a way to hold a handler open on a host the |
| -// submitter chose. |
| +// classifierTimeout bounds the classifier's outbound calls. |
| +// |
| +// The exposure is wider than "slow", and worth naming precisely. The |
| +// classifier resolves _keymail.<domain> for whatever domain was typed |
| +// and then fetches from the host that TXT record names — a value the |
| +// submitter's DNS controls, unvalidated, which may be a port or an |
| +// internal address. So this is an outbound fetch to an attacker-chosen |
| +// host, and the same host becomes the token exchange's Base. It is |
| +// bounded by the fact that csrf.Protect blocks the cross-origin POST, |
| +// so a victim has to type the address themselves; the timeout bounds |
| +// how long one costs us. Neither makes the host safe, and a future |
| +// change that widens who can reach this endpoint should read this |
| +// first. |
| const classifierTimeout = 5 * time.Second |
| // newFlow wires signin.Flow. Exactly one per process: the rate limiter's |
| @@ -110,6 +131,12 @@ func newFlow(secrets Secrets, links signin.LinkStore) *signin.Flow { |
| // Returning "" is deliberate where no IP is knowable: no per-IP budget |
| // is strictly better than one global budget shared by everyone, and the |
| // per-address budget still applies. |
| +// |
| +// The residual, stated because nothing in the code checks it: all of |
| +// the above is true BEHIND THE CARLOS EDGE. Run directly — local |
| +// development, an internal caller, a misconfigured unit — any client |
| +// can set X-Forwarded-For per request and hand itself a fresh budget |
| +// every time. The per-address budget is what still holds there. |
| func clientIP(r *http.Request) string { |
| if xff := r.Header.Get("X-Forwarded-For"); xff != "" { |
| parts := strings.Split(xff, ",") |
| @@ -232,9 +259,12 @@ func (a *App) handleVerify(w http.ResponseWriter, r *http.Request) { |
| // magic link has no such binding — anyone holding the URL can replay it |
| // — which is why that one needs the form. |
| // |
| -// It redirects and never renders: the return host is chosen by the |
| -// address, and rendering an account-influenced string on the credential |
| -// origin is what §4 forbids. |
| +// 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. |
| 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 e275650..567cfca 100644 |
| --- a/internal/home/signin_test.go |
| +++ b/internal/home/signin_test.go |
| @@ -3,9 +3,12 @@ package home |
| import ( |
| "context" |
| "errors" |
| + "fmt" |
| + "maps" |
| "net/http" |
| "net/http/httptest" |
| "net/url" |
| + "slices" |
| "strings" |
| "sync" |
| "testing" |
| @@ -199,16 +202,46 @@ func TestVerifyPageDoesNotDistinguishTokens(t *testing.T) { |
| return w |
| } |
| - good, bad := get(real), get("not-a-real-token-at-all") |
| + // A fake token of exactly the same length and alphabet as the real |
| + // one. Length matters: each response is normalised by replacing its |
| + // OWN token with a placeholder, so if the handler echoed a valid |
| + // token twice — or padded it, or truncated it — the two normalised |
| + // bodies would differ in length and be caught. The first version of |
| + // this test replaced each token with the empty string, which erased |
| + // any oracle that was a function of the token and let exactly that |
| + // mutant through. |
| + fake := strings.Repeat("z", len(real)) |
| + if fake == real { |
| + t.Fatal("the fake token collided with the real one") |
| + } |
| + // A stated limit: because the fake is the same length by |
| + // construction, a response that differed only on token LENGTH would |
| + // be invisible here. That is deliberate and not a hole — length |
| + // separates well-formed from malformed, which a scanner already |
| + // knows from the URL it was handed, and not live from dead, which is |
| + // the only thing worth hiding. |
| + |
| + good, bad := get(real), get(fake) |
| + |
| if good.Code != bad.Code { |
| t.Errorf("status differs: %d vs %d", good.Code, bad.Code) |
| } |
| - // The bodies differ only where the token itself is echoed into the |
| - // hidden field, so compare with each one's own token removed. |
| - g := strings.ReplaceAll(good.Body.String(), real, "") |
| - b := strings.ReplaceAll(bad.Body.String(), "not-a-real-token-at-all", "") |
| + |
| + // Headers too. An oracle in a header — a Content-Length, a cache |
| + // directive, anything set only on one branch — is just as readable |
| + // to a prefetching scanner as one in the body, and comparing only |
| + // bodies missed it. |
| + if !maps.EqualFunc(good.Header(), bad.Header(), slices.Equal) { |
| + t.Errorf("headers differ for a valid token:\n valid: %v\n invalid: %v", |
| + good.Header(), bad.Header()) |
| + } |
| + |
| + placeholder := strings.Repeat("#", len(real)) |
| + g := strings.ReplaceAll(good.Body.String(), real, placeholder) |
| + b := strings.ReplaceAll(bad.Body.String(), fake, placeholder) |
| if g != b { |
| - t.Error("the confirmation page renders differently for a valid token") |
| + t.Errorf("the confirmation page renders differently for a valid token "+ |
| + "(%d bytes vs %d after normalising the token)", len(g), len(b)) |
| } |
| } |
| @@ -409,3 +442,67 @@ func TestForgedPendingBlobIsRefused(t *testing.T) { |
| t.Fatalf("a forged pending blob created %d sessions", n) |
| } |
| } |
| + |
| +// The rate limiter, driven end to end. TestClientIPNeverCollapsesToOneBucket |
| +// checks what clientIP returns; nothing checked that the budget it feeds |
| +// actually bites, so handleSigninBegin's 429 branch was dead code as far |
| +// as the suite was concerned. |
| +func TestSigninIsRateLimitedPerIPNotGlobally(t *testing.T) { |
| + app := newTestApp(t) |
| + |
| + post := func(ip, addr string) int { |
| + form := url.Values{"address": {addr}} |
| + req := httptest.NewRequest(http.MethodPost, "/signin", strings.NewReader(form.Encode())) |
| + req.Header.Set("Content-Type", "application/x-www-form-urlencoded") |
| + req.Header.Set("Origin", Origin) |
| + req.Header.Set("X-Forwarded-For", ip) |
| + w := httptest.NewRecorder() |
| + app.Router().ServeHTTP(w, req) |
| + return w.Code |
| + } |
| + |
| + // One IP, distinct addresses so the per-address budget (5) is not |
| + // what fires. The per-IP budget is 20 per 15 minutes. |
| + var limited bool |
| + for i := range 25 { |
| + if post("198.51.100.7", fmt.Sprintf("p%d@example.invalid", i)) == http.StatusTooManyRequests { |
| + limited = true |
| + break |
| + } |
| + } |
| + if !limited { |
| + t.Error("25 sign-in attempts from one IP were never rate limited") |
| + } |
| + |
| + // A different IP must have its own budget. If the key had collapsed |
| + // — which is what "@" over a unix socket would have done — this |
| + // request would already be refused. |
| + if code := post("203.0.113.9", "someone-else@example.invalid"); code == http.StatusTooManyRequests { |
| + t.Error("a second IP was refused on the first attempt: the budget is global, not per-IP") |
| + } |
| +} |
| + |
| +// And the per-address budget exists too, independently of the per-IP |
| +// one: the same address from many IPs is still bounded. |
| +func TestSigninIsRateLimitedPerAddress(t *testing.T) { |
| + app := newTestApp(t) |
| + |
| + var limited bool |
| + for i := range 12 { |
| + form := url.Values{"address": {"target@example.invalid"}} |
| + req := httptest.NewRequest(http.MethodPost, "/signin", strings.NewReader(form.Encode())) |
| + req.Header.Set("Content-Type", "application/x-www-form-urlencoded") |
| + req.Header.Set("Origin", Origin) |
| + req.Header.Set("X-Forwarded-For", fmt.Sprintf("198.51.100.%d", i+1)) |
| + w := httptest.NewRecorder() |
| + app.Router().ServeHTTP(w, req) |
| + if w.Code == http.StatusTooManyRequests { |
| + limited = true |
| + break |
| + } |
| + } |
| + if !limited { |
| + t.Error("12 attempts at one address from 12 IPs were never rate limited: " + |
| + "a mail bomb only needs a botnet") |
| + } |
| +} |