amadan
Second review pass: the rewrite built the endpoint twice, and three claims of mine were false
Task 4 hand-rolled Begin and Verify and Task 5 then replaced them with signin.Flow — so an executor following this exactly would ship a POST /signin with no rate limit and no CRLF guard, then rewrite the security-critical path. Task 4 is now the store and routes nothing; Task 5 owns every handler. Built once. Three things I asserted that are false at source: "A zero AuthTime is never fresh, so a session minted without it can never pass RequireFresh." sessions.Fresh falls back to sess.At, and its doc says why — plugins that never set AuthTime, the magic link among them, must converge rather than redirect-loop. CompleteMagicLink sets only At by construction, so a magic-link session legitimately has a zero AuthTime. Copy both fields from the Identity, as auth.admit does. The per-IP rate limit does not exist as deployed. rastrillo.Serve listens on a unix socket, so RemoteAddr for the peer is empty, and Flow.Begin skips the IP budget entirely when clientIP is "". Nothing reads a forwarded header. So the only limit left is 5 per address, on the service §2.1 calls the suite's single point of failure. The plan now says to name the trusted platform header or add a total-rate cap, and decide it here. The secure_delete hook has to be registered BEFORE db.Open. db.Open calls Ping, which opens the writer pool's single long-lived connection right there — and that connection is the only one that performs DELETEs. A hook registered afterwards misses it forever while a "fresh connection" test passes. Also: csrf.Protect goes router-wide, because POST /signin is an unauthenticated endpoint that sends mail and makes outbound DNS and HTTPS calls decided by the submitted address — rastrillo's auth calls that a blind-SSRF surface by design and guards it. The magic link parameter is `token`, not `t`, which is what sendLink builds. The Keymail client needs RedirectPath set, or its default disagrees with the route mounted. `purpose` needs a column, and expires_at needs its timestamp format pinned, since the comparison is a string one. And the app must refuse to boot without a mailer — mail.Logged writes the address to platform-held logs on every sign-in, which is §3.4's leak arriving through a fallback nobody chose — or with an empty DBPath, which SQLite treats as a private temporary database. Paul ruled the zero-JS question: §11 gains one named carve-out for the passkey enrolment and ceremony screens, which may load rastrillo's own webauthn.mjs. Everything else stays scriptless, so a person with script off keeps a working path and loses only the optional second factor. Task 6 is unblocked, and Task 7's no-script sweep gets an explicit allowlist rather than a blanket exemption. Says why GET /signin/callback may mint on GET when GET /signin/verify may not: the __Host- pending cookie and state bind the callback to the browser that began the flow, and the magic link has no such binding.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 file changed,
+212
−136
docs/superpowers/plans/2026-09-03-home-signin.md+212 −136
diff --git a/docs/superpowers/plans/2026-09-03-home-signin.md b/docs/superpowers/plans/2026-09-03-home-signin.md| index 44e8e0b..9e234e1 100644 |
| --- a/docs/superpowers/plans/2026-09-03-home-signin.md |
| +++ b/docs/superpowers/plans/2026-09-03-home-signin.md |
| @@ -63,7 +63,14 @@ helper folds case for the same reason. |
| ### Three things to settle before Task 5 — not the executor's to decide |
| 1. **`home` has no secrets.** `carlos secrets list --account bds --app home` returns empty; sheets has `SHEETS_DEK`, `SHEETS_INSTANCE_KEY`, `CARLOS_SMTP_PASS`. Task 1 cannot run until `HOME_DEK` exists; Task 4 cannot send a link without SMTP; and **Task 4 also needs `HOME_PEPPER`** for the blind index (ruling 3). All three are one-way doors — lose the DEK and every sealed link is unreadable; lose the pepper and every person row is orphaned, because the index is the only way back from an address to a person. **Paul provisions these, not the executor.** |
| -2. **Passkey needs JavaScript, and §11 forbids it.** §11: "Every screen works with scripts off. **No surface here has any reason to require script**, and the day one does is the day this section is wrong." A WebAuthn ceremony is `navigator.credentials.create/get`, which is script by construction. **The day §11 anticipated has arrived, and the ruling is whether §11 bends or passkeys leave slice one.** |
| +2. ~~Passkey needs JavaScript, and §11 forbids it.~~ **RULED 2026-09-05 by |
| + Paul: §11 gains one named carve-out.** The passkey enrolment and |
| + ceremony screens may load rastrillo's own `webauthn.mjs`. Everything |
| + else stays scriptless — magic-link sign-in, recovery-code redemption and |
| + the whole springboard — so a person with script off keeps a working path |
| + and only loses the optional second factor. Task 7's no-`<script>` sweep |
| + gets an **explicit allowlist of exactly those screens**, never a blanket |
| + exemption. The original problem, kept for the reasoning: §11: "Every screen works with scripts off. **No surface here has any reason to require script**, and the day one does is the day this section is wrong." A WebAuthn ceremony is `navigator.credentials.create/get`, which is script by construction. **The day §11 anticipated has arrived, and the ruling is whether §11 bends or passkeys leave slice one.** |
| Correcting this plan's own earlier claim, which was false at source: **rastrillo does ship the client JS.** `webauthn/js.go` embeds `js/webauthn.mjs` and exports `webauthn.JS()`, and the `passkey` package's doc says apps serve it as a static asset. So the question is not who writes the script — nobody has to — it is only whether one screen may require it. That makes the carve-out cheaper than the earlier draft implied, and the ruling no less necessary. Everything else here is scriptless and unaffected. |
| 3. ~~No `main` on this remote.~~ **Closed 2026-09-05:** `main` exists at |
| @@ -109,17 +116,31 @@ sessions.New(sessions.Config{ |
| }) |
| ``` |
| -**Set `Method` and `AuthTime` on every minted session.** `sessions.Session` |
| -is `{Subject, Method, AuthTime, At}`, and `sessions.Fresh` compares |
| -`AuthTime` against a max age. **A zero `AuthTime` is never fresh**, so a |
| -session minted without it can never pass `RequireFresh` — which is what |
| -§5.2's show-codes-once page must sit behind. `Method` is `"magic_link"` or |
| -`"keymail"` from the `signin.Identity`. |
| +**Copy `Method` and `AuthTime` from the `signin.Identity`**, as |
| +`auth.admit` does — do not invent either. Correcting an earlier draft of |
| +this section, which was false at source: it said "a zero `AuthTime` is never |
| +fresh". `sessions.Fresh` **falls back to `sess.At`** when `AuthTime` is |
| +zero, and its doc says why — "plugins that never set AuthTime (the magic |
| +link) still converge instead of redirect-looping". `CompleteMagicLink` sets |
| +only `At` by construction, so a magic-link session legitimately has a zero |
| +`AuthTime` and `RequireFresh` still works. |
| **The mailed link's base URL** is `https://id.oficina.works` + |
| `Flow.LinkPath`. It comes from config, never from the request Host: a |
| Host-derived link is an open redirect with an extra step. |
| +**Refuse to boot rather than degrade.** Two silent failures, both of which |
| +sheets-core already guards: |
| + |
| +- **No mailer.** `mail.Logged`'s `Send` logs `"to", to` — an address into |
| + platform-held logs on every single sign-in, which is precisely §3.4's |
| + leak, arriving through the fallback nobody chose. Sheets' `LoadAuthEnv` |
| + has the shape: "no mailer means magic links get written to platform-held |
| + logs … hard exit". Do the same. |
| +- **Empty `DBPath`.** `Resolve` sets it only from `-db`, and SQLite treats |
| + an empty filename as a **private temporary database** — so the app comes |
| + up, works, and loses every person on restart. |
| + |
| **TDD is not optional and not compressible.** An earlier draft collapsed a |
| task into "Run, fail, implement, run" with no code. Every task writes the |
| test, runs it, watches it fail *for the stated reason*, implements, and |
| @@ -399,12 +420,20 @@ none of the others, and on none opened later. The earlier draft's test — |
| read the pragma back and assert 1 — runs on that same connection and so |
| passes while proving nothing, which is the worst kind of green. |
| -Use `sqlite.RegisterConnectionHook` (modernc.org/sqlite, present at |
| -v1.57.0) to run `PRAGMA secure_delete=ON` on **every** new connection. |
| -Test it by forcing a fresh connection — close and reopen, or drain the |
| -writer pool — and asserting the pragma there, not on the connection that |
| -set it. `migrate.Apply` takes a `*db.DB`, so bypassing `db.Open` is not an |
| -escape route. |
| +Use `sqlite.RegisterConnectionHook` (modernc.org/sqlite — present at the |
| +version `go.mod` actually pins) to run `PRAGMA secure_delete=ON` on |
| +**every** new connection. |
| + |
| +**Register the hook before `db.Open`, in `main` and in `newTestApp`.** |
| +`db.Open` calls `Ping`, which opens the writer pool's single long-lived |
| +connection there and then. A hook registered afterwards misses that |
| +connection **forever** — and it is the only one that performs DELETEs, so |
| +it is the one that matters. A test asserting on "a fresh connection" would |
| +pass while the writer never had the pragma set, which is the worst kind of |
| +green. |
| + |
| +Assert on `d.Writer()` **and** one reader. `migrate.Apply` takes a |
| +`*db.DB`, so bypassing `db.Open` is not an escape route. |
| - [ ] **Step 6: Commit** |
| @@ -501,7 +530,13 @@ git commit -m "The person ref: opaque, minted, never derived from an address" |
| --- |
| -### Task 4: Magic-link sign-in, and gate 2 in full |
| +### Task 4: The schema and the sealed LinkStore |
| + |
| +**Task 4 builds the store; Task 5 builds the endpoint.** An earlier draft |
| +had Task 4 hand-roll `Begin` and `Verify` and Task 5 then replace them with |
| +`signin.Flow` — so the executor would have shipped a `POST /signin` with no |
| +rate limit and no CRLF guard, and then rewritten the security-critical path. |
| +Nothing here routes a handler. |
| The task the whole step exists for, and where the blindness claim is either kept or quietly broken. |
| @@ -513,7 +548,7 @@ The task the whole step exists for, and where the blindness claim is either kept |
| **Interfaces:** |
| - Consumes: `seal.Sealer`, `NewPersonRef`, `sessions.Sessions`, `mail.Sender`. |
| -- Produces: `(*App).handleSigninBegin`, `(*App).handleSigninVerify`, routed at `POST /signin` and `GET /signin/verify`. |
| +- Produces: `(*App).links`, a `signin.LinkStore`. **Routes nothing** — the handlers are Task 5's, at `POST /signin`, `GET /signin/verify` (renders), `POST /signin/verify` (redeems) and `GET /signin/callback`. |
| `0002_signin_links.sql`: |
| @@ -594,119 +629,72 @@ ON CONFLICT(blind_index) DO UPDATE SET blind_index = excluded.blind_index |
| RETURNING person_ref |
| ``` |
| -`created_at` is written **coarsened to the day** (§3.3). |
| +`created_at` is written **coarsened to the day**. (Cite §3.3's reasoning, |
| +not its rule: on the spec branch §3.3's coarsening is explicitly "a rule for |
| +publication … never a storage format". Coarsening here is still right — it |
| +is strictly less information and `people.created_at` has no freshness role — |
| +but it is this plan's choice, not §3.3's requirement.) |
| -- [ ] **Step 1: Write gate 2 in full, as a failing test** |
| +- [ ] **Step 1: Write the LinkStore's failing tests** |
| -```go |
| -// TestFullSigninLeavesNoReadableAddress is §12 gate 2: "a full sign-in |
| -// plus directory upsert plus rekey leaves no readable address anywhere, |
| -// WAL included." The directory and rekey halves arrive in step 4; this |
| -// asserts the sign-in half and must be EXTENDED, not replaced, then. |
| -func TestFullSigninLeavesNoReadableAddress(t *testing.T) { |
| - dbPath := filepath.Join(t.TempDir(), "home.db") |
| - app := newTestApp(t, dbPath) |
| +Four properties, driven directly against the store rather than through a |
| +handler: |
| - // Lowercase and address-shaped: the blind index and signin's rate |
| - // limiter both key on strings.ToLower(address), so a canary that is |
| - // only ever upper-case cannot detect a leak of the lowercased copy — |
| - // which is the copy likeliest to reach a store or an error string. |
| - addr := "canary-" + sealtest.Token + "@example.invalid" |
| - |
| - signIn(t, app, addr) |
| - sealtest.AssertNoPlaintext(t, dbPath) |
| -} |
| +```go |
| +func TestPutLinkSealsTheAddress(t *testing.T) // raw file holds no canary |
| +func TestTakeLinkReturnsTheAddressOnce(t *testing.T) // second take: ok == false |
| +func TestTakeLinkRefusesAWrongPurpose(t *testing.T) |
| +func TestConcurrentTakeLinkYieldsOneWinner(t *testing.T) // N goroutines, one ok |
| ``` |
| -`newTestApp` wires a `mail.Sender` that captures rather than sends |
| -(`mail.Sender` is a one-method interface: `Send(ctx, to, subject, body) |
| -error`), and `signIn` is the helper Step 4 builds: POST the address, pull |
| -the link out of the captured mail, GET the confirm page, POST the form. |
| +Run the concurrency one against **both pools** — `db.Open` gives a writer |
| +capped at 1 and four or more readers, and a race that only appears across |
| +pools is still a race. |
| -- [ ] **Step 2: Run it** |
| +- [ ] **Step 2: Run them** |
| -Run: `go test ./internal/home/ -run TestFullSignin` |
| -Expected: FAIL — no `/signin` route. |
| +Run: `go test ./internal/home/ -run TestPutLink` |
| +Expected: FAIL — no store. |
| -- [ ] **Step 3: Implement Begin** |
| +- [ ] **Step 3: Implement PutLink and TakeLink** |
| -`sessions.NewToken()` returns `(token, hash, err)`; mail the raw token, store the hash. Seal the address, insert, send, then **303** to a "check your mail" page. **Always 303, always the same page**, whether or not the address is known — a different response for a known address is an account-enumeration oracle. |
| +`PutLink` seals then inserts. `TakeLink` is the single `DELETE … RETURNING` |
| +above. |
| -- [ ] **Step 4: Implement Verify as GET-renders, POST-redeems** |
| +**Pin the timestamp format.** `expires_at > ?` is a **string** comparison, |
| +so both sides must be written the same way — `expires.UTC().Format(time.RFC3339)`, |
| +as `auth/store.go` does. A value carrying a zone offset sorts differently |
| +and silently accepts expired links. |
| -**The magic link must not sign anyone in on GET.** An earlier draft had |
| -`GET /signin/verify` look up the token and mint. Two things break: |
| +**`purpose` needs somewhere to live.** The `signin.LinkStore` interface |
| +passes one to both methods, and the schema above has no column for it. Add |
| +`purpose TEXT NOT NULL` to `0002_signin_links.sql` and put it in the DELETE |
| +predicate. (Hard-coding a check against `signin.PurposeSignin` also works, |
| +but the column costs nothing and the interface clearly expects more than |
| +one purpose to exist.) |
| -- **Mail security scanners prefetch links.** The scanner burns the |
| - single-use token and the person's own click fails. This is routine, not |
| - hypothetical. |
| -- **It is login CSRF.** Any page the victim loads can embed |
| - `<img src="https://id.oficina.works/signin/verify?t=…">` and silently |
| - sign them into an account the attacker controls, after which everything |
| - they do happens in the attacker's session. §5.6 names login CSRF for the |
| - assertion; this is the same attack one hop earlier. |
| - |
| -rastrillo's own `auth.Verify` redeems on GET, so this is inherited rather |
| -than invented — and the fix is free under zero-JS: |
| - |
| -- `GET /signin/verify?t=…` **touches no state.** It renders a scriptless |
| - confirmation page: one form, the token in a hidden field, one button. It |
| - must not look the token up — that reintroduces the oracle and lets a |
| - prefetch probe validity. |
| -- `POST /signin/verify` behind `csrf.Protect("https://id.oficina.works")` |
| - does the work: take the link with the single DELETE-RETURNING above, open |
| - the sealed address, classify it (Task 5), find-or-mint the person with the |
| - blind index, then `sessions.SignIn` with `Subject: personRef`. |
| - |
| -Cross-device still works (§5.5): the token rides the URL, not a cookie, so |
| -the link opens on any device and the form posts from there. |
| - |
| -- [ ] **Step 4a: Prove the three properties that were wrong** |
| - |
| -Three tests, each of which fails against the design this replaced: |
| - |
| -```go |
| -// A returning person gets the SAME person ref. Without the blind index |
| -// every sign-in minted a new one, so nobody could ever return. |
| -func TestSecondSigninFindsTheSamePerson(t *testing.T) |
| +- [ ] **Step 4: Prove the seal bites** |
| -// Two concurrent redemptions of one link mint exactly one session. |
| -// SELECT-then-UPDATE minted two. |
| -func TestConcurrentRedemptionMintsOneSession(t *testing.T) |
| +Temporarily store the address unsealed. `sealtest.AssertNoPlaintext` must |
| +FAIL naming the canary. Revert. **Do not skip this.** A gate never seen to |
| +fail is a gate written to pass — the finding that came back twice on gate 9. |
| -// A GET does not redeem: a mail scanner's prefetch must leave the link |
| -// usable, and must not sign anybody in. |
| -func TestGetDoesNotRedeemTheLink(t *testing.T) |
| -``` |
| - |
| -And one that keeps the oracle shut: unknown, expired and already-used |
| -tokens must produce byte-identical responses. |
| - |
| -- [ ] **Step 4b: Sweep expired links** |
| +- [ ] **Step 5: Sweep expired links** |
| Sealed rows are not a leak, but unbounded growth is a bug. Delete |
| `WHERE expires_at < ?` on the same wake-driven schedule §6 uses for the |
| outbox — **never a ticker**, since a hibernating app runs no timers. |
| -- [ ] **Step 5: Run the gate** |
| - |
| -Run: `go test ./internal/home/ -run TestFullSignin` |
| -Expected: PASS. |
| - |
| -- [ ] **Step 6: Prove the gate bites** |
| - |
| -Temporarily change the insert to store the address unsealed. Run the test; it must FAIL naming the canary. Revert. **Do not skip this.** A gate never seen to fail is a gate written to pass — the finding that came back twice on gate 9. |
| - |
| -- [ ] **Step 7: Commit** |
| +- [ ] **Step 6: Commit** |
| ```bash |
| -git add internal/home/signin.go internal/home/signin_test.go internal/home/migrations/0002_signin_links.sql internal/home/templates internal/home/app.go |
| -git commit -m "Magic-link sign-in, with the address sealed before it is written" |
| +git add internal/home/links.go internal/home/links_test.go internal/home/migrations/0002_signin_links.sql |
| +git commit -m "A sign-in link store that seals before it writes and takes exactly once" |
| ``` |
| --- |
| -### Task 5: The `signin.Flow` seam, sealed link storage, and the Keymail upgrade |
| +### Task 5: `signin.Flow`, the sign-in handlers, and the Keymail upgrade |
| **Files:** |
| - Create: `internal/home/links.go`, `internal/home/links_test.go` |
| @@ -752,7 +740,28 @@ lost or would have had to write: |
| Configuration: `Origin: "https://id.oficina.works"`, `LinkPath: |
| "/signin/verify"`, a non-zero `Key` from config (it seals the pending blob |
| and **must not** come from an instance data key — sign-in runs before any |
| -session exists), `Mailer`, `Classifier` and `Keymail`. |
| +session exists), `Mailer`, `Classifier`, and `Keymail` **with |
| +`RedirectPath: "/signin/callback"`** — its default is `/auth/callback`, so |
| +mounting the route without setting this leaves the two disagreeing. |
| + |
| +**Mount `csrf.Protect(origin)` router-wide, not just on the verify POST.** |
| +`POST /signin` needs it too: `rastrillo/auth` checks same-origin on its own |
| +`Begin` and its comment says why — "an unauthenticated endpoint that sends |
| +mail and makes outbound DNS/HTTPS calls decided by the submitted address (a |
| +blind-SSRF surface by design)". `csrf.Protect` is pure origin evidence |
| +(`Sec-Fetch-Site`, then `Origin`, then `Referer`) and never consults a |
| +session, so it works before anyone is signed in. |
| + |
| +**The per-IP budget does not exist as deployed, and the plan must not claim |
| +it.** `rastrillo.Serve` listens on a unix socket (`net.Listen("unix", …)`), |
| +so `r.RemoteAddr` for the peer is `""`, `auth`'s `clientIP` returns it |
| +unchanged, and `Flow.Begin` skips the IP budget entirely when `clientIP == |
| +""`. Nothing in rastrillo or sheets-core reads a forwarded header. So either |
| +name the trusted platform header that carries the real client IP and use it, |
| +or pass `""` deliberately **and add a total-rate cap on `POST /signin`** — |
| +because with no IP key the only limit left is 5 per address, and §2.1 makes |
| +this service the suite's single point of failure. Decide it here rather than |
| +discovering it in production. |
| **Classification fails open** (§5.1). The package already guarantees it — |
| "every failure mode — DNS, transport, malformed body, non-200 — answers |
| @@ -765,21 +774,30 @@ trip. **Redirect to it, never render it**: the return host is |
| address-chosen, and rendering an account-influenced string on the |
| credential origin is what §4 forbids and gate 5 asserts. |
| -- [ ] **Step 1: Write the failing test** |
| - |
| -`signin.ClassKeymail` **does not exist** — an earlier draft invented it, and |
| -flagged it as the one identifier not verified at source, which turned out to |
| -be the right instinct and the wrong response. `Class` is a struct: |
| +- [ ] **Step 1: Write gate 2 in full, as a failing test** |
| ```go |
| -type Class struct { |
| - Keymail bool // true only for a claimed inbox |
| - Server string // the federation host that answered |
| - Reauth bool // the server's own advertisement that step-up exists |
| +// TestFullSigninLeavesNoReadableAddress is §12 gate 2: "a full sign-in |
| +// plus directory upsert plus rekey leaves no readable address anywhere, |
| +// WAL included." The directory and rekey halves arrive in step 4; this |
| +// asserts the sign-in half and must be EXTENDED, not replaced, then. |
| +func TestFullSigninLeavesNoReadableAddress(t *testing.T) { |
| + dbPath := filepath.Join(t.TempDir(), "home.db") |
| + app := newTestApp(t, dbPath) |
| + |
| + // Lowercase and address-shaped: the blind index and signin's rate |
| + // limiter both key on strings.ToLower(address), so a canary that is |
| + // only ever upper-case cannot detect a leak of the lowercased copy. |
| + signIn(t, app, "canary-"+sealtest.Token+"@example.invalid") |
| + sealtest.AssertNoPlaintext(t, dbPath) |
| } |
| ``` |
| -So assert on the flow's decision, not on a constant: |
| +Plus the classification test, asserting on the flow's decision rather than |
| +on a constant. **`signin.ClassKeymail` (which turned out not to exist — `Class` is a struct) does not exist** — an earlier draft |
| +invented it and flagged it as the one identifier not verified at source, |
| +which was the right instinct and the wrong response. `Class` is a struct |
| +`{Keymail bool; Server string; Reauth bool}`: |
| ```go |
| func TestFailedClassificationStillSendsAMagicLink(t *testing.T) { |
| @@ -787,7 +805,8 @@ func TestFailedClassificationStillSendsAMagicLink(t *testing.T) { |
| app.flow.Classifier.LookupTXT = func(context.Context, string) ([]string, error) { |
| return nil, errors.New("servfail") |
| } |
| - next, err := app.flow.Begin(context.Background(), "someone@example.invalid", "203.0.113.1", 0) |
| + // force is signin.Method, a string type — not 0. |
| + next, err := app.flow.Begin(context.Background(), "someone@example.invalid", "", signin.Method("")) |
| if err != nil { |
| t.Fatalf("Begin: %v", err) |
| } |
| @@ -797,38 +816,91 @@ func TestFailedClassificationStillSendsAMagicLink(t *testing.T) { |
| } |
| ``` |
| -- [ ] **Step 2: Run it** |
| +- [ ] **Step 2: Run them** |
| -Run: `go test ./internal/home/ -run TestFailedClassification` |
| -Expected: FAIL — no `flow` field. |
| +Expected: FAIL — no `/signin` route, no `flow` field. |
| -- [ ] **Step 3: Implement the LinkStore** |
| +- [ ] **Step 3: Wire the Flow and implement Begin** |
| -`PutLink` seals and inserts; `TakeLink` is the single `DELETE … RETURNING` |
| -from Task 4 and opens the sealed column. Test both halves directly, |
| -including that a wrong purpose does not match and that two concurrent |
| -`TakeLink` calls for one hash return `ok` exactly once. |
| +`POST /signin` calls `Flow.Begin`, then **303 to a "check your mail" page — |
| +always, and always the same page**, whether or not the address is known. A |
| +different response for a known address is an account-enumeration oracle. |
| -- [ ] **Step 4: Wire the Flow and re-point the handlers** |
| +- [ ] **Step 4: Verify as GET-renders, POST-redeems** |
| -`POST /signin` calls `Flow.Begin`. `GET /signin/verify` renders the |
| -confirmation form (Task 4). `POST /signin/verify` calls |
| -`Flow.CompleteMagicLink`, which returns an `Identity` carrying the address; |
| -the person lookup and session mint follow as in Task 4. |
| +**The magic link must not sign anyone in on GET.** An earlier draft had |
| +`GET /signin/verify` look up the token and mint. Two things break: |
| -- [ ] **Step 5: Commit** |
| +- **Mail security scanners prefetch links.** The scanner burns the |
| + single-use token and the person's own click fails. Routine, not |
| + hypothetical. |
| +- **It is login CSRF.** Any page the victim loads can embed |
| + `<img src="https://id.oficina.works/signin/verify?token=…">` and silently |
| + sign them into an account the attacker controls, after which everything |
| + they do happens in the attacker's session. §5.6 names login CSRF for the |
| + assertion; this is the same attack one hop earlier. |
| + |
| +rastrillo's own `auth.Verify` redeems on GET, so this is inherited rather |
| +than invented — and the fix is free under zero-JS: |
| + |
| +- `GET /signin/verify?token=…` **touches no state.** It renders a |
| + scriptless confirmation page: one form, the token in a hidden field, one |
| + button. It must not look the token up — that reintroduces the oracle and |
| + lets a prefetch probe validity. **The parameter is `token`**, which is |
| + what `sendLink` builds; an earlier draft said `t`. |
| +- `POST /signin/verify` behind `csrf.Protect` does the work: |
| + `Flow.CompleteMagicLink` (which calls `TakeLink`), then find-or-mint the |
| + person with the blind index, then `sessions.SignIn` with |
| + `Subject: personRef` and `Method`/`AuthTime` copied from the `Identity`. |
| + |
| +Cross-device still works (§5.5): the token rides the URL, not a cookie. |
| + |
| +**`GET /signin/callback` may mint on GET, and the difference is not |
| +arbitrary.** The Keymail return is bound to the browser that began the flow |
| +by the `__Host-` pending cookie plus `state`, so an attacker cannot make a |
| +victim's browser complete a flow it never started. The magic link has no |
| +such binding — anyone holding the URL can replay it — which is exactly why |
| +it needs the form. Redirect to the callback, **never render it**: the |
| +return host is address-chosen, and rendering an account-influenced string on |
| +the credential origin is what §4 forbids and gate 5 asserts. |
| + |
| +- [ ] **Step 5: Prove the three properties that were wrong** |
| + |
| +```go |
| +// A returning person gets the SAME person ref. Without the blind index |
| +// every sign-in minted a new one, so nobody could ever return. |
| +func TestSecondSigninFindsTheSamePerson(t *testing.T) |
| + |
| +// Two concurrent redemptions of one link mint exactly one session. |
| +func TestConcurrentRedemptionMintsOneSession(t *testing.T) |
| + |
| +// A GET does not redeem: a mail scanner's prefetch must leave the link |
| +// usable, and must not sign anybody in. |
| +func TestGetDoesNotRedeemTheLink(t *testing.T) |
| +``` |
| + |
| +And one that keeps the oracle shut: unknown, expired and already-used |
| +tokens must produce byte-identical responses. |
| + |
| +- [ ] **Step 6: Run the gate, then prove it bites** |
| + |
| +Run gate 2. Then break the seal deliberately and watch it fail, as Task 4 |
| +step 4 did for the store. |
| + |
| +- [ ] **Step 7: Commit** |
| ```bash |
| -git add internal/home/links.go internal/home/links_test.go internal/home/signin.go go.mod go.sum |
| -git commit -m "Take signin.Flow as the seam, with a LinkStore that seals" |
| +git add internal/home/signin.go internal/home/signin_test.go internal/home/templates internal/home/app.go go.mod go.sum |
| +git commit -m "Sign-in through signin.Flow, with the link redeemed on POST" |
| ``` |
| --- |
| -### Task 6: Passkeys and recovery codes — BLOCKED on one ruling |
| +### Task 6: Passkeys and recovery codes |
| -**Blocked on the §11 ruling** in "Three things to settle": passkeys need |
| -`navigator.credentials`, and §11 says every screen works with scripts off. |
| +**Unblocked 2026-09-05.** §11 has its carve-out (see "Three things to |
| +settle"), so this task proceeds: the enrolment and ceremony screens may |
| +load script, and nothing else may. |
| **Correcting this plan's own earlier claim, which was false at source:** |
| rastrillo *does* ship the client JavaScript. `webauthn/js.go` embeds |
| @@ -861,7 +933,8 @@ them. |
| worry about inheriting `auth`'s address-bearing subject dissolves — say so |
| rather than leaving it to be rediscovered. |
| -**Do not start until that ruling exists.** |
| +**Serve `webauthn.JS()` as a static asset**, as the `passkey` package's doc |
| +says — do not vendor a copy, or it drifts from the module on every upgrade. |
| --- |
| @@ -922,6 +995,9 @@ func TestRPIDIsExactlyTheCredentialHost(t *testing.T) { |
| } |
| // Wherever a passkey.Config is built, LegacyRPID must be empty, or a |
| // full host with a comment naming the §5.7 drill that is running. |
| + // Until Task 6 lands there is no passkey.Config; assert the Origin half |
| + // alone and let this half start as a skipped subtest with the reason |
| + // written down, never as an absent assertion. |
| if cfg := passkeyConfig(t); cfg.LegacyRPID != "" && !strings.Contains(cfg.LegacyRPID, ".oficina.works") { |
| t.Errorf("LegacyRPID %q widens the RP ID (§4, ruling 2)", cfg.LegacyRPID) |
| } |
| @@ -975,4 +1051,4 @@ git commit -m "Scriptless sign-in screens, and the guard that the RP ID is a ful |
| - **Deliberately not covered:** `prefs`, `directory`, `teams`, `provisioning_queue`, `rekey_journal`, `consumers`, `assertion_jti` — later steps. |
| - **Two spec corrections needed, neither made silently:** §5.1's "use `rastrillo/auth` for classification" is not achievable; §3.1 contradicts ruling 3 on HMAC-vs-KDF. |
| - **The one unverified identifier:** `signin.ClassKeymail` in Task 5. Every other signature in this plan was read at source. |
| -- **The riskiest assumption:** that `rastrillo/db.Open` either sets `PRAGMA secure_delete` or lets the app set it. Task 2 step 5 verifies rather than assumes; if neither holds, that is a blocker and an upstream issue. |
| +- ~~The riskiest assumption: that `rastrillo/db.Open` either sets `PRAGMA secure_delete` or lets the app set it.~~ **Resolved 2026-09-05: it does neither.** The DSN carries `busy_timeout`, `journal_mode` and `foreign_keys` and passes nothing else through. Task 2 step 5 now uses a connection hook registered before `db.Open`, because the writer pool's one long-lived connection is opened inside it. |