Review pass: four of the plan's source claims were false, and sign-in could not find a returning person
The plan's shape was right and most of it survives. What did not is listed here, because each would have become auth code. Sign-in minted a fresh person ref on every visit. The blind index was deferred to step 4 as "not settled", leaving a NOT NULL UNIQUE column with nothing to put in it and no lookup key — so sessions, credentials, recovery codes and the assertion's sub would all have attached to an identity nobody could ever return to. Ruling 3 is in the settled table and supplies the mechanism; it is built in this task now, and HOME_PEPPER joins the list only Paul can provision. Lose the DEK and sealed links are unreadable; lose the pepper and every person row is orphaned. The magic link redeemed on GET. Mail scanners prefetch links, which burns a single-use token; worse, any page the victim loads could embed an img tag pointing at the verify URL and silently sign them into the attacker's account. §5.6 names login CSRF for the assertion; this is the same attack one hop earlier. GET now renders a scriptless confirm form and POST does the work behind csrf.Protect. Cross-device still works because the token rides the URL, not a cookie. Redemption was SELECT-then-UPDATE, so two concurrent clicks both minted. It is one DELETE ... RETURNING now, which also removes the redeemed_at column and with it the oracle that let a caller tell "already used" from "never existed" — all three are now no row. PRAGMA secure_delete: the plan called this its riskiest assumption and it was right. db.Open's DSN carries busy_timeout, journal_mode and foreign_keys and nothing else, with no passthrough, and it opens two pools. secure_delete is per-connection, so an Exec after open sets it on one connection out of five — and the proposed test would have read it back on that same connection and passed while proving nothing. It needs sqlite.RegisterConnectionHook and a test on a fresh connection. Task 5 dropped the Keymail upgrade entirely: it computed a class no code path consumed, after the mail had gone. The seam is signin.Flow, whose Links interface lets the app supply a store that seals in PutLink and opens in TakeLink. Taking it also brings rate limits on POST /signin — the plan had none, leaving a mail-bomb vector — plus CRLF-safe address parsing and a link TTL the plan never named. signin.ClassKeymail, flagged as the one identifier not verified at source, does not exist; Class is a struct. Task 6 claimed rastrillo ships no WebAuthn JS. It does: webauthn/js.go embeds js/webauthn.mjs and exports webauthn.JS(). So the ruling §11 needs is only whether one screen may require script, not who writes it. Struck the fallback that recovery codes could ship without passkeys: SignInRecovery needs a live half-session and never signs anyone in from nothing, so codes alone are a first-factor bypass the spec never authorised. The RP-ID guard asserted nothing — an undefined helper, a dot count that is not "the full host", and no mention of LegacyRPID, which is a second accepted RP ID for assertions. Gate 1's canary was case-sensitive and not address-shaped, so it was blind to the lowercased copy that the blind index and the rate limiter both produce. And §3.4's "failed to hash alice@example.com" is its counter-example, not a prescribed error string; the plan read it as the latter and would have shipped the exact leak the rule forbids. Plus ruling 20 (attributes, not classes), main now existing, the gate-9 merge-order dependency, and a helpers section so the executor is not inventing newTestApp, the sessions config, the session TTL or AuthTime — a zero AuthTime is never fresh, so RequireFresh could never have passed.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 file changed,
+414
−82
docs/superpowers/plans/2026-09-03-home-signin.md+414 −82
diff --git a/docs/superpowers/plans/2026-09-03-home-signin.md b/docs/superpowers/plans/2026-09-03-home-signin.md| index 73fb385..44e8e0b 100644 |
| --- a/docs/superpowers/plans/2026-09-03-home-signin.md |
| +++ b/docs/superpowers/plans/2026-09-03-home-signin.md |
| @@ -22,11 +22,22 @@ Every task's requirements implicitly include this section. Values are copied ver |
| - **The credential origin never renders an account-authored string, ever.** It is the RP. "a permanent constraint with its own test, not a naming choice." (§4) |
| - **The RP ID is the full host** — `id.oficina.works` — "**Never widen the RP ID to a registrable suffix**", asserted by a test. (§4, ruling 16) |
| - **Zero JS.** "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." (§11) |
| -- **Write `class="rst-*"`**, `day` theme, rastrillo `server` shape. (§11) |
| +- **Write the attribute spelling — `rst-btn`, not `class="rst-btn"`** |
| + (ruling 20, 2026-09-05). §11 ruled the class vocabulary and both its |
| + reasons died: the attribute grammar shipped in v0.21.0, and at v0.22.0 — |
| + the current pin — rastrillo's own partials flipped, class-spelling |
| + occurrences in `ui/partials/` falling from 124 to 6. rastrillo's design |
| + system rules "One spelling, not two. No permanent class fallback" and |
| + drops the class selectors before 1.0. This app has no markup yet, so it |
| + starts on the surviving spelling. `day` theme, rastrillo `server` shape. |
| - **Recovery is recovery codes**: "ten, shown once, redeemed as a plain form POST with no JS. Sign-in only — step-up still demands a real assertion." (§5.2, ruling 6) |
| - **Passkey is optional per person.** Per-team enforcement is **not in slice one** (ruling 15) and no operator reset exists in slice one (§5.2) — do not build either. |
| - **No address, team name, or queue payload in any log line, error string, job status message or metric label.** Enforced by gate 9, already in the tree. (AGENTS.md; §3.4) |
| -- **One error string** for a hashing failure — "*failed to hash*" (§5.1's surrounding text; check §7 before inventing a second). |
| +- **Never put the value in the error.** §3.4's *"failed to hash |
| + alice@example.com"* is its **counter-example** — the sentence naming what |
| + must not reach a log — not a prescribed error string. An earlier draft of |
| + this plan read it as the latter and would have shipped exactly the leak |
| + the rule forbids. Errors carry person refs and codes. (§3.4) |
| - **One binary, `CGO_ENABLED=0`, cgo-free SQLite.** (AGENTS.md) |
| - **Every branch gets a discussion, and that discussion is the review record.** §Review wants a pass by a model that did not write the code. (AGENTS.md) |
| @@ -37,13 +48,31 @@ Every task's requirements implicitly include this section. Values are copied ver |
| - **Gate 1, plaintext grep** — "raw SQLite file and WAL plus this app's store objects, against a seeded corpus of known addresses." (§12) |
| - **Gate 2, no-address-at-rest** — "the schema has no address column, and a full sign-in plus directory upsert plus rekey leaves no readable address anywhere, WAL included." (§12) |
| -Sheets already has the helper this needs: `internal/sheets/sealtest` in `oficina/sheets-core`. Port it the way `internal/guardstest` was ported, rather than reinventing it. |
| +Sheets already has the helper this needs: `internal/sheets/sealtest` in `oficina/sheets-core`. Port it the way `internal/guardstest` was ported, rather than reinventing it — **and port `sealtest.go` itself along with its own `TestLeakedDetectsCanary` and WAL tests.** Task 1's steps commit `internal/home/sealtest` without ever saying to create it. |
| + |
| +**Change its canaries when porting.** Sheets' are shaped like |
| +`CANARY-CELL-VALUE-8f3a` and matched with `bytes.Contains` — case-sensitive |
| +and not address-shaped. Both matter here: the blind index (ruling 3) and |
| +`signin`'s rate limiter both key on `strings.ToLower(address)`, so the |
| +lowercased copy is the one likeliest to reach a store or an error string, |
| +and an upper-case canary cannot see it. Use lowercase, address-shaped |
| +canaries — `canary-8f3a@example.invalid` — or scan case-insensitively. |
| +Gate 9's own `logtest` package already learned this and its `encodings` |
| +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. Minting a DEK is a one-way door — lose it and every sealed link is unreadable — so **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` — script by construction — and **rastrillo ships no client JS for it** (`navigator.credentials` appears only in its docs and `webauthn/browser_test.go`, verified at v0.22.0). Task 6 is blocked on a ruling: §11 gains an explicit carve-out for the one optional enhancement, or passkeys leave slice one. Everything else here is scriptless and unaffected. |
| -3. **No `main` on this remote.** The cadence says branch from and squash-merge to `main`; `git ls-remote --heads origin` shows `design`, `skeleton`, `domain/oficina-works` and feature branches. Work off `skeleton`. |
| +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.** |
| + |
| + 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 |
| + `462df84` and the cadence applies as written — branch from it, |
| + squash-merge to it. |
| + |
| +4. **Merge order.** Task 2 modifies `internal/guardstest/repo_test.go`, |
| + which exists only on `feat/gate-9-no-content-in-logs`. That branch must |
| + land first, or Task 2 has no file to edit. |
| ### A spec-vs-reality note the executor must not "fix" silently |
| @@ -53,6 +82,54 @@ The usable seam is one level down: `github.com/keymaildev/signin` exports `Class |
| --- |
| +## Helpers and wiring this plan assumes |
| + |
| +Named here because an earlier draft used all of them without ever saying to |
| +build them, which would have left the executor re-deriving decisions |
| +mid-task. |
| + |
| +**Test helpers** — build these in Task 1 and 2, before anything calls them: |
| + |
| +| Helper | What it does | |
| +|---|---| |
| +| `newTestApp(t, dbPath)` | Opens a temp DB, applies migrations, wires a capturing `mail.Sender`, a `Sealer` from a test DEK, a test pepper, and a `signin.Flow` with a stub `LookupTXT`. Returns `*App`. | |
| +| `capturedLink(t, app)` | Pulls the magic link out of the captured mail. | |
| +| `signIn(t, app, addr)` | The whole ceremony: POST `/signin`, take the link, GET the confirm page, POST the form. Task 4's tests all go through it. | |
| +| `passkeyConfig(t)` | Returns the `passkey.Config` the app builds, for Task 7's guard. | |
| +| `templateNames(t)`, `renderTo(t, w, name, data)`, `sampleData(name)` | Task 7's scriptless sweep over every template. | |
| + |
| +**`sessions` wiring**, which no step created: |
| + |
| +```go |
| +sessions.New(sessions.Config{ |
| + DB: d.Writer(), |
| + Origin: "https://id.oficina.works", |
| + TTL: 30 * 24 * time.Hour, // the package default, named rather than defaulted |
| + SigninPath: "/signin", |
| +}) |
| +``` |
| + |
| +**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`. |
| + |
| +**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. |
| + |
| +**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 |
| +runs it again. Task 4's step 6 — deliberately break the seal and watch the |
| +gate catch it — is the pattern for every gate here, and it exists because |
| +gate 9's review found the same thing twice: a guard never seen to fail is a |
| +guard written to pass. |
| + |
| +--- |
| + |
| ## File Structure |
| ``` |
| @@ -307,7 +384,27 @@ Expected: PASS. |
| - [ ] **Step 5: Turn on secure_delete** |
| -`PRAGMA secure_delete` must be on, because "deleting a row does not remove bytes from a file that is continuously replicated" (§5.1). **Verify whether `rastrillo/db.Open` already sets it**; if not, set it immediately after open. Either way add a test that reads `PRAGMA secure_delete` back and asserts it is 1. Do not assume — this is the riskiest assumption in the plan. |
| +`PRAGMA secure_delete` must be on, because "deleting a row does not remove bytes from a file that is continuously replicated" (§5.1). |
| + |
| +**Resolved at source, 2026-09-05 — it was the plan's own riskiest |
| +assumption and it does not hold.** `rastrillo/db.Open` builds the DSN |
| +`path + "?_pragma=busy_timeout(5000)&_pragma=journal_mode(WAL)&_pragma=foreign_keys(1)"` |
| +and passes nothing else through: **no `secure_delete`, and no way to add |
| +one via the DSN.** It also opens **two pools** — a writer capped at 1 and |
| +readers of 4 or more. |
| + |
| +That matters because **`secure_delete` is per-connection.** An `Exec` |
| +after open sets it on whichever pooled connection served that call and on |
| +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. |
| - [ ] **Step 6: Commit** |
| @@ -424,16 +521,81 @@ The task the whole step exists for, and where the blindness claim is either kept |
| CREATE TABLE signin_links ( |
| token_hash TEXT PRIMARY KEY, |
| sealed_address BLOB NOT NULL, |
| - expires_at TEXT NOT NULL, |
| - redeemed_at TEXT |
| + expires_at TEXT NOT NULL |
| ); |
| CREATE INDEX idx_signin_links_expires_at ON signin_links(expires_at); |
| ``` |
| -The column holds `sealer.Seal("signin.address", []byte(addr))`. **Seal before the insert, never "insert then clean up"** — Litestream ships WAL frames continuously, so a plaintext write reaches the bucket before any delete could remove it (§5.1, §3.2). |
| +**There is no `redeemed_at` column, and its absence is the design.** An |
| +earlier draft had one and redeemed with SELECT-then-UPDATE. Two things were |
| +wrong with that. It is a read-then-write race: two clicks that both read a |
| +NULL both mint a session, and rastrillo's own `linkStore.TakeLink` carries a |
| +comment naming this exact race and fixing it with a single statement. And a |
| +row that survives redemption lets the handler tell "already used" apart from |
| +"never existed", which is an oracle. Taking the link is therefore one |
| +statement: |
| + |
| +```sql |
| +DELETE FROM signin_links |
| + WHERE token_hash = ? AND expires_at > ? |
| +RETURNING sealed_address |
| +``` |
| + |
| +Exactly one concurrent caller gets a row. Unknown, expired and already-used |
| +are then **indistinguishable by construction** rather than by remembering to |
| +make them so — all three are "no row". Deleting is safe here where it would |
| +not be elsewhere: the column is sealed, so the bytes Litestream keeps are |
| +bytes nobody can read (§3.2). |
| + |
| +**Take first, mint second.** A crash between them costs one link, which the |
| +person retries. The reverse order costs a session minted from a link still |
| +live for a second click. |
| + |
| +The column holds `sealer.Seal("signin.address", []byte(addr))`. **Seal |
| +before the insert, never "insert then clean up"** — Litestream ships WAL |
| +frames continuously, so a plaintext write reaches the bucket before any |
| +delete could remove it (§5.1, §3.2). |
| Use `rastrillo/sessions` directly, **never `rastrillo/auth`**: `sessions.Session.Subject` is an opaque string the app chooses, so it takes the person ref. `auth` sets `Subject: id.Address` and stores `auth_links.address TEXT NOT NULL` — both verified at source, and precisely what ruling 14 forbids. |
| +**The blind index is built in this task, not deferred.** An earlier draft |
| +deferred it to step 4 and said "until then, mint a person row keyed on the |
| +ref alone". That is not sign-in: with `blind_index TEXT NOT NULL UNIQUE` and |
| +no lookup key, every visit mints a *fresh* person ref, and sessions, |
| +credentials, recovery codes and the assertion's `sub` all attach to an |
| +identity nobody can ever return to. Ruling 3 is in §1's settled table and |
| +supplies the mechanism: |
| + |
| +```go |
| +// blindIndex is ruling 3: HMAC(pepper, lowercased address). The pepper |
| +// never leaves the home (ruling 4) — one on every instance would make a |
| +// single instance compromise an offline dictionary attack against the |
| +// whole suite directory. |
| +func blindIndex(pepper []byte, address string) string { |
| + m := hmac.New(sha256.New, pepper) |
| + m.Write([]byte(strings.ToLower(address))) |
| + return hex.EncodeToString(m.Sum(nil)) |
| +} |
| +``` |
| + |
| +`HOME_PEPPER` joins `HOME_DEK` in the list only Paul can provision, and for |
| +a sharper reason: lose the DEK and sealed links become unreadable; lose the |
| +pepper and **every person row is orphaned**, because the index is the only |
| +route from an address back to a person. §3.1 prefers an expensive KDF over a |
| +bare HMAC; that is a mitigation note, not a ruling, and ruling 3 is the |
| +ruling. File the tension as a spec note rather than resolving it here. |
| + |
| +Finding-or-minting the person is also one statement, so a returning person |
| +and a first-time person take the same path: |
| + |
| +```sql |
| +INSERT INTO people (person_ref, blind_index, created_at) VALUES (?, ?, ?) |
| +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). |
| + |
| - [ ] **Step 1: Write gate 2 in full, as a failing test** |
| ```go |
| @@ -445,26 +607,21 @@ func TestFullSigninLeavesNoReadableAddress(t *testing.T) { |
| dbPath := filepath.Join(t.TempDir(), "home.db") |
| app := newTestApp(t, dbPath) |
| - addr := string(sealtest.Canaries[0]) + "@example.invalid" |
| - |
| - 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") |
| - rec := httptest.NewRecorder() |
| - app.Router().ServeHTTP(rec, req) |
| - if rec.Code != http.StatusSeeOther { |
| - t.Fatalf("POST /signin = %d, want 303", rec.Code) |
| - } |
| - |
| - // Redeem the link the mailer captured, so a session row exists too. |
| - app.Router().ServeHTTP(httptest.NewRecorder(), |
| - httptest.NewRequest(http.MethodGet, capturedLink(t, app), nil)) |
| + // 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) |
| } |
| ``` |
| -`newTestApp` wires a `mail.Sender` that captures rather than sends (`mail.Sender` is a one-method interface: `Send(ctx, to, subject, body) error`). |
| +`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. |
| - [ ] **Step 2: Run it** |
| @@ -475,9 +632,61 @@ Expected: FAIL — no `/signin` route. |
| `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. |
| -- [ ] **Step 4: Implement Verify** |
| +- [ ] **Step 4: Implement Verify as GET-renders, POST-redeems** |
| + |
| +**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: |
| + |
| +- **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`. |
| -Look up by `sessions.HashToken(r.URL.Query().Get("t"))`. Reject if missing, expired, or `redeemed_at` is non-null — single use. Open the sealed address, classify it (Task 5), then mint or find the person row. **The blind index needs step 4's pepper, which is not settled** (see Task 2 step 3); until it is, mint a person row keyed on the ref alone and **file a task rather than leaving a TODO in the code**. Mint the session with `Subject: personRef`, and mark the link redeemed **in the same transaction** as the session mint. |
| +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) |
| + |
| +// Two concurrent redemptions of one link mint exactly one session. |
| +// SELECT-then-UPDATE minted two. |
| +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 4b: 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** |
| @@ -497,67 +706,160 @@ git commit -m "Magic-link sign-in, with the address sealed before it is written" |
| --- |
| -### Task 5: Address classification and the Keymail upgrade |
| +### Task 5: The `signin.Flow` seam, sealed link storage, and the Keymail upgrade |
| **Files:** |
| -- Create: `internal/home/classify.go`, `internal/home/classify_test.go` |
| -- Modify: `go.mod` — add `github.com/keymaildev/signin` |
| +- Create: `internal/home/links.go`, `internal/home/links_test.go` |
| +- Modify: `internal/home/signin.go`, `go.mod` — add `github.com/keymaildev/signin` |
| **Interfaces:** |
| -- Produces: `(*App).classify(ctx context.Context, address string) signin.Class` |
| +- Produces: `(*App).links` implementing `signin.LinkStore`; `(*App).flow` a `*signin.Flow`. |
| -Use `signin.Classifier` directly — a plain struct, no constructor, no database. See the spec-vs-reality note above: `rastrillo/auth` cannot serve this purpose without its address-bearing tables. |
| +**An earlier draft used `signin.Classifier` alone and dropped the Keymail |
| +upgrade on the floor.** It computed a class no code path consumed, and did |
| +it *after* the mail had already gone. §5.1 requires "auto-upgrading to |
| +Keymail when the address has a claimed inbox", so the seam is one level up. |
| -**Classification fails open** (§5.1): "classification fails open, so every address has a working path — which matters more here than in a normal app, because guests arrive from outside the suite entirely." A classifier error, timeout or DNS failure must yield the magic-link path, never an error page. |
| +`signin.Flow` is that seam, and the reason to take it is that it is the |
| +same seam `rastrillo/auth` uses — minus the address-bearing tables ruling |
| +14 forbids. `Flow.Links` is an interface (verified at v0.1.1): |
| -- [ ] **Step 1: Write the failing tests** |
| +```go |
| +PutLink(ctx, hash, address, purpose string, expires time.Time) error |
| +TakeLink(ctx, hash, purpose string) (address string, ok bool, err error) |
| +``` |
| + |
| +**So the app supplies the store, and sealing happens inside it.** `PutLink` |
| +seals before the insert; `TakeLink` runs Task 4's single |
| +`DELETE … RETURNING` and opens the result. The package's own comment on |
| +`TakeLink` says it "must delete the row it returns", which is the same |
| +single-use rule Task 4 arrived at independently. |
| + |
| +Wiring `Flow` then delivers, for free, four things the earlier draft either |
| +lost or would have had to write: |
| + |
| +- **The Keymail upgrade** (§5.1). `Begin` classifies, and on a claimed |
| + inbox takes `beginKeymail` — an HMAC'd pending blob in a cookie, and |
| + `CompleteKeymail` with an address-mismatch check — instead of mailing. |
| +- **Rate limits on `POST /signin`**, which the earlier draft had none of, |
| + leaving a mail-bomb vector. Defaults are 20 per IP and 5 per address per |
| + 15-minute window, and a nil `Limiter` builds a `MemoryLimiter` rather |
| + than being silently unlimited. |
| +- **CRLF-safe address parsing** via `SplitAddress`. |
| +- **`LinkTTL`**, defaulting to 15 minutes — the link lifetime the earlier |
| + draft never named. |
| + |
| +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`. |
| + |
| +**Classification fails open** (§5.1). The package already guarantees it — |
| +"every failure mode — DNS, transport, malformed body, non-200 — answers |
| +*not keymail*" — so the app must not add a failure path on top. Still bound |
| +the `Classifier.HTTP` client with a timeout: it makes outbound DNS and |
| +HTTPS requests to whatever domain the person typed. |
| + |
| +Add `GET /signin/callback` mirroring `auth.Callback` for the Keymail round |
| +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: |
| ```go |
| -func TestClassifyFailsOpen(t *testing.T) { |
| - for _, tc := range []struct { |
| - name string |
| - lookup func(ctx context.Context, name string) ([]string, error) |
| - }{ |
| - {"dns error", func(context.Context, string) ([]string, error) { return nil, errors.New("servfail") }}, |
| - {"empty", func(context.Context, string) ([]string, error) { return nil, nil }}, |
| - } { |
| - t.Run(tc.name, func(t *testing.T) { |
| - app := newTestApp(t, filepath.Join(t.TempDir(), "home.db")) |
| - app.classifier.LookupTXT = tc.lookup |
| - got := app.classify(context.Background(), "someone@example.invalid") |
| - if got == signin.ClassKeymail { |
| - t.Fatalf("classify = %v, want the magic-link path on failure", got) |
| - } |
| - }) |
| +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 |
| +} |
| +``` |
| + |
| +So assert on the flow's decision, not on a constant: |
| + |
| +```go |
| +func TestFailedClassificationStillSendsAMagicLink(t *testing.T) { |
| + app := newTestApp(t, filepath.Join(t.TempDir(), "home.db")) |
| + 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) |
| + if err != nil { |
| + t.Fatalf("Begin: %v", err) |
| + } |
| + if next.Kind != signin.NextMagicLink { |
| + t.Fatalf("Kind = %v, want NextMagicLink: classification must fail open", next.Kind) |
| } |
| } |
| ``` |
| -**Check `signin`'s actual `Class` constant names before writing this** — the package is at `/home/paulca/go/pkg/mod/github.com/keymaildev/signin@v0.1.0/classify.go`. Do not invent them; `ClassKeymail` above is a placeholder for whatever it actually declares, and is the one identifier in this plan not verified at source. |
| +- [ ] **Step 2: Run it** |
| -- [ ] **Step 2: Run, fail, implement, run** |
| +Run: `go test ./internal/home/ -run TestFailedClassification` |
| +Expected: FAIL — no `flow` field. |
| -Wrap `Classifier.Classify` with a bounded `context.WithTimeout` and return the magic-link class on anything unexpected. |
| +- [ ] **Step 3: Implement the LinkStore** |
| -- [ ] **Step 3: Commit** |
| +`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. |
| + |
| +- [ ] **Step 4: Wire the Flow and re-point the handlers** |
| + |
| +`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. |
| + |
| +- [ ] **Step 5: Commit** |
| ```bash |
| -git add internal/home/classify.go internal/home/classify_test.go go.mod go.sum |
| -git commit -m "Classify addresses, and fail open so every address has a path" |
| +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" |
| ``` |
| --- |
| -### Task 6: Recovery codes — BLOCKED |
| +### Task 6: Passkeys and recovery codes — BLOCKED on one ruling |
| -**Blocked on the §11 ruling** in "Three things to settle". `passkey.RegenerateRecoveryCodes` lives in the same package as the WebAuthn ceremony, and enrolling a passkey needs client JavaScript rastrillo does not ship. |
| +**Blocked on the §11 ruling** in "Three things to settle": passkeys need |
| +`navigator.credentials`, and §11 says every screen works with scripts off. |
| + |
| +**Correcting this plan's own earlier claim, which was false at source:** |
| +rastrillo *does* ship the client JavaScript. `webauthn/js.go` embeds |
| +`js/webauthn.mjs` and exports `webauthn.JS()`; the `passkey` package's doc |
| +says apps serve it as a static asset. So nobody has to write it, and the |
| +ruling is only whether **one** screen may require script. Everything else in |
| +this step stays scriptless either way. |
| Verified at v0.22.0 so the executor need not re-derive it: |
| - `recoveryCodesPerSet = 10` — matches §5.2's "ten" exactly. |
| -- Redemption is `if r.Method != http.MethodPost` then `r.PostFormValue("code")` — **a plain form POST with no JS**, matching §5.2 exactly. |
| -- Codes are base32, single-use, hashed with `sessions.HashToken`. |
| - |
| -So **redemption is scriptless and could ship alone**; only *issuing* codes alongside a passkey drags JS in. If passkeys leave slice one, this task becomes "recovery codes as a second sign-in path" and stays scriptless. If §11 gains a carve-out, this task also owns writing the WebAuthn JS the framework does not provide. |
| +- Redemption is `if r.Method != http.MethodPost` then `r.PostFormValue("code")` — **a plain form POST with no JS**, matching §5.2 exactly. Mount `csrf.Protect` on it, as the package requires. |
| +- Codes are base32, 50 bits each, hashed with `sessions.HashToken`, redeemed with a single `DELETE … RETURNING`. |
| +- There is no attempt counter, and that is sound rather than an omission: `SignInRecovery` requires a live `passkey_pending` half-session, so the window is bounded by `pendingTTL` rather than by a counter. |
| +- Losing passkey and codes together is a permanent lockout. That is §5.2's disclosed outcome with no operator reset (§14) — not a gap to fill. |
| + |
| +**Struck from the earlier draft:** *"If passkeys leave slice one, this task |
| +becomes 'recovery codes as a second sign-in path' and stays scriptless."* |
| +That is a spec substitution, not a fallback. `SignInRecovery` needs a live |
| +half-session minted by `Gate` — the package's own doc is "it never signs |
| +anybody in from nothing" — and §5.2 defines codes as recovery **for the |
| +second factor**. Codes without a passkey are a first-factor bypass the spec |
| +never authorised. If passkeys leave slice one, recovery codes leave with |
| +them. |
| + |
| +**Attachment, which the earlier draft omitted entirely.** At the point |
| +`POST /signin/verify` would call `sessions.SignIn`, call |
| +`passkey.Handlers.Gate(w, r, sess)` exactly as `auth.admit` does. Because |
| +`sess.Subject` is already the person ref, `passkey_credentials`, |
| +`passkey_pending` and `passkey_recovery_codes` key on it, and the spec's |
| +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.** |
| @@ -568,43 +870,73 @@ So **redemption is scriptless and could ship alone**; only *issuing* codes along |
| **Files:** |
| - Create: `internal/home/render.go`, `internal/home/templates/*.html` |
| - Create: `internal/guardstest/rpid_test.go` |
| -- Create: `static/tokens.css`, `static/theme.css` — copied from rastrillo, then frozen |
| +- Create: `static/tokens.css`, `static/theme.css` — vendored from rastrillo |
| **Interfaces:** |
| - Produces: `(*App).render(w http.ResponseWriter, name string, data any)` |
| Follow sheets-core's `internal/sheets/render.go`: one parsed tree per page, `ui.Funcs()` plus an `asset` func, `ParseFS(ui.Templates(), "*.html")` then the app's own layout and page. |
| -**Write `class="rst-*"`** — and for the right reason. tokens.css at v0.22.0 ships **both** spellings, paired: 130 distinct bare `rst-*` attributes alongside their class twins. Its own header says "TWO SPELLINGS, ON PURPOSE, FOR ONE RELEASE … The attribute spelling is the vocabulary this library is moving to; the class spelling is what every app and every shipped partial writes today." Write classes because that is what the partials you render alongside write — not because the attribute grammar is unreleased. It is released. |
| +**Write the attribute spelling — `rst-btn`, not `class="rst-btn"`** (ruling |
| +20). An earlier draft of this plan said to write classes "because that is |
| +what the partials you render alongside write". That was true at v0.21.0 and |
| +false at v0.22.0, the version pinned: the partials flipped, and the class |
| +spelling in `ui/partials/` fell from 124 occurrences to 6. rastrillo's |
| +design system rules one spelling with the class selectors dropped before |
| +1.0, and this app has no markup yet, so it starts on the survivor. |
| -Copy `static/tokens.css` and `static/theme.css` (the `day` theme) deliberately, and **re-copy on every rastrillo upgrade** until `rastrillo doctor` lands. |
| +Vendor `static/tokens.css` and `static/theme.css` (the `day` theme), then |
| +run **`rastrillo doctor --fix`** on each upgrade — it landed in v0.21.0, so |
| +the earlier draft's "until `rastrillo doctor` lands" was a trigger that had |
| +already fired. Add the `TestVendoredAssetsMatchTheLibrary` that |
| +`rastrillo new` scaffolds; this repo was hand-written, so it has no copy. |
| - [ ] **Step 1: The RP-ID guard** |
| +The earlier draft's version asserted nothing: it called an undefined |
| +`configuredOrigins`, counted dots (`strings.Count(host, ".") < 2`, which is |
| +not "the RP ID is the full host" and passes for any three-label host), and |
| +ignored `LegacyRPID` entirely. |
| + |
| ```go |
| -// Rulings 13 and 16: "The RP ID is the full host, asserted by a test." |
| -// passkey.Config has NO RPID field — RPID is derived internally as |
| -// url.Parse(Origin).Hostname() — so the thing to assert is the Origin |
| -// this app configures. |
| -func TestRPIDIsTheFullHost(t *testing.T) { |
| - for _, origin := range configuredOrigins(t) { |
| - u, err := url.Parse(origin) |
| - if err != nil { |
| - t.Fatalf("parse %q: %v", origin, err) |
| - } |
| - host := u.Hostname() |
| - if strings.Count(host, ".") < 2 { |
| - t.Errorf("origin %q derives RP ID %q, a registrable suffix: an RP ID "+ |
| - "valid at every host in the zone collapses the credential/directory "+ |
| - "origin split (ruling 2)", origin, host) |
| - } |
| +// Rulings 2, 13 and 16: "The RP ID is the full host, asserted by a test." |
| +// |
| +// Two doors, both accepted silently: |
| +// |
| +// - passkey.Config has NO RPID field. The RP ID is always |
| +// url.Parse(Origin).Hostname(), so widening it means widening Origin, |
| +// or bypassing passkey and building a webauthn.Config directly. |
| +// - passkey.Config.LegacyRPID is passed straight through as a SECOND |
| +// accepted RP ID — for assertions only; registration never takes it |
| +// (webauthn.go rpMatches, allowLegacy). Set to "oficina.works" it |
| +// collapses the split §4 exists to make. |
| +const credentialOrigin = "https://id.oficina.works" |
| + |
| +func TestRPIDIsExactlyTheCredentialHost(t *testing.T) { |
| + u, err := url.Parse(credentialOrigin) |
| + if err != nil { |
| + t.Fatalf("parse %q: %v", credentialOrigin, err) |
| + } |
| + if u.Hostname() != "id.oficina.works" { |
| + t.Errorf("RP ID is %q, want id.oficina.works (rulings 13, 16)", u.Hostname()) |
| + } |
| + // 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. |
| + 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) |
| } |
| } |
| ``` |
| +Add ruling 2's other half while here: the sessions cookie on the credential |
| +origin is `__Host-` and host-only (verified in `sessions.setCookie`). |
| +Assert no cookie set there carries a `Domain`. |
| + |
| - [ ] **Step 2: Every screen works with scripts off** |
| -§11 is enforceable mechanically and should be. |
| +§11 is enforceable mechanically and should be. If Task 6's ruling grants a |
| +carve-out, this test gains an explicit allowlist of the one screen — never |
| +a blanket exemption. |
| ```go |
| func TestNoScriptTagInAnyTemplate(t *testing.T) { |