amadan
Plan §13 step 2: sign-in, second factor, recovery codes
The implementation plan for hand-rolled sign-in, and the two binding
files it had to correct to be true.
Seven review passes. The first six each found something that would have
become authentication code, and the plan you are reading is the one
that survived them:
The magic link redeemed on GET, so a mail scanner's prefetch burned the
token and, worse, any page the victim loaded could sign them into an
attacker's account with an img tag — login CSRF, which §5.6 names for
the assertion, one hop earlier. GET now renders a scriptless confirm
form; POST redeems behind csrf.Protect.
Redemption was SELECT-then-UPDATE, so two clicks both minted a session.
One DELETE ... RETURNING now, which also removes the redeemed_at column
and with it the oracle that told "already used" from "never existed".
Sign-in could never find a returning person: the blind index was
deferred, leaving a NOT NULL UNIQUE column with no lookup key, so every
visit minted a fresh person ref and every session, credential and
recovery code would have attached to an identity nobody could return
to. Ruling 3 is settled, so it is built here.
The per-IP rate limit was a suite-wide DoS. Over a unix socket
RemoteAddr is "@", so clientIP returns "@" and every request in the
world shares one budget key: twenty POSTs to /signin lock out sign-in
for everyone for fifteen minutes, on the service §2.1 calls the single
point of failure. The client IP comes from X-Forwarded-For, which the
carlos edge sets and strips on unverified hops.
The passkey Gate was on the magic-link path only, so a Keymail-upgraded
account with a passkey enrolled would have signed in with one factor.
PRAGMA secure_delete is per-connection and db.Open opens the writer's
connection inside itself, so the hook has to be registered before it —
and the obvious test would have read the pragma back on the connection
that set it and passed while proving nothing.
Four source claims were false and are corrected in place, including two
in binding files: §5.1 and AGENTS.md both said classification comes
from rastrillo/auth, which cannot be done — auth.New hardwires
Links: &linkStore{db: cfg.DB} and that store writes auth_links.address,
the very thing ruling 14 exists to avoid. Both now say
keymaildev/signin, whose Flow takes a LinkStore the app supplies, so
the app seals what it stores. AGENTS.md's own rule is that such
disagreements are fixed in the same commit, and "filed" is not fixed.
Three of the corrections made along the way were themselves wrong and
were caught on the following pass: the tokens.css history, a claim that
a zero AuthTime is never fresh (Fresh falls back to At), and the first
diagnosis of the rate limit. They are recorded as wrong rather than
quietly replaced.
Blocked on Paul for four secrets before it can run: HOME_DEK,
HOME_PEPPER, HOME_SIGNIN_KEY and CARLOS_SMTP_PASS. Tests use test keys,
so Tasks 1-7 are not blocked; the deploy is.
Reviewed by Fable across seven passes; the seventh approved.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
3 files changed,
+1415
−10
AGENTS.md+12 −7docs/superpowers/plans/2026-09-03-home-signin.md+1394 −0docs/superpowers/specs/2026-08-30-home-design.md+9 −3
CI passed — run details
Intent
Human
<task-notification> <task-id>a94c1b48373efbd7d</task-id> <tool-use-id>toolu_017CNuc56Um5ZyD1Z84Efn8e</tool-use-id> <output-file>/home/paulca/.cache/tmp/claude-1001/-home-paulca-amadan-net-oficina-home/d63f5ac7-6b42-48c5-a13d-34e6faf8e44c/tasks/a94c1b48373efbd7d.output</output-file> <status>completed</status> <summary>Agent "Fable review 4: signin plan" 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>**VERDICT: CHANGES NEEDED** The security architecture is now right — I re-verified all of it at source (rastrillo v0.22.0, keymaildev/signin v0.1.1, modernc sqlite v1.55.0, the carlos edge at 97eb53f, sheets-core, the spec and AGENTS.md on `main` at 5a42f2d). What remains is not design. It is that several of the tests that *are* the gates would not compile or would not test anything, and the boot path that loads the secrets has no step. Those are blocking for a plan that becomes authentication code; the rest are small. ## Your findings 1–9, checked at source | # | Status | |---|---| | 1 Per-IP | **Verified.** `auth.clientIP` returns `r.RemoteAddr` unchanged on `SplitHostPort` failure; `Flow.Begin` skips the budget only for `""`. Carlos `setForwarded` keeps "the LAST element … the client" and `stripForwardedHeaders` deletes inbound `X-Forwarded-*` on unverified hops. Plan text (lines 785–805) is correct. | | 2 Task 6 | Exists with Files/Interfaces/7 steps. Correct against `passkey.go`/`recovery.go` (`recoveryCodesPerSet = 10`, `PostFormValue("code")`, `sessions.HashToken`, `DELETE … RETURNING`, `pendingTTL`, `ConfirmPath` default). Defects below. | | 3 Gate on both paths | **Verified**, step 3. | | 4 Task 5 step 3 | **Verified.** Cookie attributes match `auth.setCookie` (auth.go:236–241); `force` matches `Begin`'s doc. | | 5 `Flow.Key` | Named. **But "loadSecrets and the boot refusals given steps" is false at source** — see B1. | | 6 Task 4/5 | `sealtest.Canaries` named, `Keymail` non-nil, `sync.Once`, `Origin` header all present. The renderer question is *named* but not *answered* (B4). Task 4's Interfaces still contradict themselves (M4). | | 7 Blind index | Struck from Task 4, **but residue survives** at line 422 and in the Deferred table (M7). | | 8 Residue | Some remains (lines 193, 422, 1122–1124, item 4 of "Three things"). | | 9 Ruling-21 allowlist | Prose is right; **the test code has no allowlist** (B3). | Also verified correct, since they were the riskiest claims: `sqlite.RegisterConnectionHook` is a package-level function (sqlite.go:785); the upsert `… DO UPDATE … RETURNING person_ref` returns `pr_a` on both insert and conflict (ran it against modernc v1.55.0); `Classify` makes no HTTP call after a non-NXDOMAIN TXT failure, so the fail-open test is sound; `Serve` only opens a DB `if opts.DBPath != ""`. ## Blocking **B1. Secrets and helpers are used and never created.** Line 108: *"Build these in Task 1 (`newTestApp`, `loadSecrets`) and Task 5 (`signIn`), each with its own step and its own test."* Task 1's steps are round-trip test / run / port seal.go / run / LoadSealer / commit. Task 5's are gate-2 test / run / wire Flow / verify / properties / gate / commit. **No step anywhere creates** `newTestApp`, `loadSecrets`, `signIn`, `capturedLink`, `passkeyConfig`, `templateNames`/`renderTo`/`sampleData`, a `HOME_PEPPER` loader, a `HOME_SIGNIN_KEY` loader, `blindIndex`/find-or-mint (prose only in Task 4; absent from its `git add`), the two boot refusals (no mailer, empty `DBPath`), or the `sealtest` port — which the plan itself notes at line 51 (*"Task 1's steps commit `internal/home/sealtest` without ever saying to create it"*) and then does not fix. And **`cmd/home/main.go` never calls `LoadSealer`**: Task 2's snippet is the only `main.go` edit and it wires only the DB; sheets' `main.go:82` calls `sheets.LoadSealer()`. So the answer to "is a secret used before it is loaded" is yes — three of the four are never loaded on the boot path at all. **B2. Gate 2's test does not compile, and would not parse as an address if it did.** Line 832: `signIn(t, app, "canary-"+sealtest.Canaries[0]+"@example.invalid")`. `Canaries` is `[][]byte` (Interfaces line 219; sheets `sealtest.go:22`) — `string + []byte` is a type error. And the plan's own instruction (line 58–59) makes the canary `canary-8f3a@example.invalid`, so the composed value has two `@`; `SplitAddress` cuts at the first, sees `@` in the domain, returns `ErrBadAddress`. The one test that *is* gate 2 cannot run. **B3. Task 7's no-script test forbids what Task 6 builds.** Step 2's prose: *"The only `<script>` on an allowlisted template is a same-origin `src` from a named list."* The code beneath it (lines 1151–1159) fails on any `<script` in any template with no allowlist at all. Task 6 lands first with `passkey_confirm.html` loading `webauthn.mjs`. The executor copies the code, not the prose. The script-strip-then-assert-recovery-form check and the CSP assertion are prose only, too. **B4. The renderer's owner is still undecided.** Lines 730–734: *"Either land Task 7 first or build the minimal renderer here and let Task 7 extend it — but say which."* The plan does not say which. Task 5 step 1 (`signIn` GETs the confirm page), Task 6's three templates and Task 7 all need `(*App).render`, which is Task 7's. As ordered, Tasks 5 and 6 cannot execute. **B5. The RP-ID guard asserts a constant against itself.** Lines 1110–1119 define `const credentialOrigin = "https://id.oficina.works"` *inside the test* and check `u.Hostname() != "id.oficina.works"`. That cannot fail whatever the app configures; only the `LegacyRPID` half reads `passkeyConfig(t)`. By the plan's own rule (*"a guard never seen to fail is a guard written to pass"*) parse `passkeyConfig(t).Origin`. The comment *"Until Task 6 lands there is no passkey.Config"* is residue from when Task 6 was deferred. ## Must fix, mechanical - **M1. `static/` path.** Tasks 6 and 7 create and `git add` `static/passkey.js`, `static/tokens.css` at repo root, while saying "follow sheets-core's `render.go`" — which is `//go:embed all:static` *inside* `internal/sheets/`. `go:embed` cannot reach a parent directory. `rastrillo new` scaffolds `internal/<pkg>/static/` and its `TestVendoredAssetsMatchTheLibrary` reads `../<pkg>/static`. Should be `internal/home/static/`. - **M2.** Task 6 Files: *"Modify: … `internal/home/db.go` (schema merge)"* and its `git add`. Task 2 put `Schema`/`BootSchema` in `models.go`; no `db.go` exists. - **M3.** File Structure line 193: *"templates/ Scriptless screens. `class="rst-*"`."* Contradicts ruling 20. - **M4.** Task 4 Interfaces: *"Consumes: `seal.Sealer`, `NewPersonRef`, `sessions.Sessions`, `mail.Sender`."* next to *"needs no `sessions` or `mail.Sender`."* - **M5. The seven-utility carve-out is missing.** §11 and AGENTS.md on `main`: `rst-sr-only`, `rst-mono`, `rst-m-hide`, `rst-grow`, `rst-nm`, `rst-danger`, `rst-cell-mut` keep `class=` — *"Write `<span rst-sr-only>` and nothing matches, so screen-reader-only text renders visible."* The plan's ruling-20 bullet and Task 7 say attribute spelling with no exception. - **M6.** Task 4 step 5: *"on the same wake-driven schedule §6 uses for the outbox — never a ticker, since a hibernating app runs no timers."* §6's sentence is about instances; AGENTS.md: *"The home does not hibernate."* No wake-driven schedule exists in this step. Say: sweep at boot and on each `PutLink` (as `passkey.Sweep`'s doc suggests). - **M7.** Line 422: *"must be settled before the pepper is implemented in step 4"* and the Deferred table's *"The pepper and the blind index"* row — both contradict Task 4's *"The blind index is built in this task, not deferred."* - **M8.** Task 6 Interfaces name only the seven JSON routes. The GET pages (`ConfirmPath`, enrol, recovery codes) and the two script assets need routes; `ConfirmPath`'s value and `RequireFresh`'s `maxAge` are unspecified. - **M9.** "Three things" item 4 is stale: gate 9 (`internal/home/logtest`, `repo_test.go`) is on `main` at 9162ee5. ## Nits - Line 979 conflates the Gate finish with step-up: `SignInFinish` mints `method+"+passkey"`; `StepUpFinish` mints `"passkey"` alone. - `webauthn/authtest` exists (rastrillo's own `passkey_test.go` uses it to enrol); name it for Task 6 step 2. - `register()` needs `rpId`, `userId`, `userName`; `/begin` returns only `challenge`. Say userId = person ref and userName is not the address (the app cannot have it there anyway). - Go's `mime` already maps `.mjs` to `text/javascript; charset=utf-8`; the explicit Content-Type is harmless. - `strings.Contains(LegacyRPID, ".oficina.works")` passes `x.oficina.works.evil`; use `HasSuffix`. - `UNIQUE` on `blind_index` already indexes it; `idx_people_blind_index` is redundant. - `db.DB` has no reader accessor; asserting `secure_delete` "on one reader" needs `d.G.Clauses(dbresolver.Read)` — say how. - §5.1 and AGENTS.md (lines 176–178) both still say classification goes through `rastrillo/auth`; the plan's "filed" correction should name AGENTS.md too. ## Plainly Not mergeable yet. Nothing here reopens the design — the seal, the sealed link store, the `DELETE … RETURNING`, GET-renders/POST-redeems, the XFF fix, the Gate on both paths, the RP-ID rule are all sound and verified. But B1–B5 mean the executor would hit a compile error in gate 2, a red no-script test on the plan's own output, an undecided renderer before the first handler test, a guard that cannot fail, and a boot path that never loads three of four secrets. Fix those five and the mechanical list, and this is ready.</result> <usage><subagent_tokens>196203</subagent_tokens><tool_uses>39</tool_uses><duration_ms>449506</duration_ms></usage> </task-notification>
Attributed to this commit: 18.8 g CO₂e (2345034 tokens). This is an attribution, not a measurement — usage counted from this commit's own marker forward to the next one on main, never backward from it. A turn that produced more than one commit books all of its tokens to the last one, and a concurrent session on this branch can add to the same figure. Some of it was priced using the shared default rate, because no model-specific factor row exists yet for at least one model involved. The Carbon view has the repo total, which is never derived from this attribution.
diff --git a/AGENTS.md b/AGENTS.md| index d6889bb..dddc792 100644 |
| --- a/AGENTS.md |
| +++ b/AGENTS.md |
| @@ -169,13 +169,18 @@ quietly ignored). |
| any delete. `PRAGMA secure_delete` is on. |
| - **The session Subject is an opaque person ref, never an address.** |
| `rastrillo/auth` as shipped sets `Subject: id.Address` and stores readable |
| - addresses in `auth_links.address` and, via `Subject: id.Address`, |
| - in `sessions.subject` — `auth_sessions` is no longer written at v0.22.0, |
| - its rows having been migrated into the shared `sessions` table, so the |
| - leak moved rather than closed. Verified at source, filed |
| - upstream as `rastrillo/rastrillo` #2. We hand-roll session minting until |
| - that lands, and use `auth` only for address classification and the Keymail |
| - upgrade, which hold nothing. |
| + addresses in `auth_links.address` and, via `Subject: id.Address`, in |
| + `sessions.subject` — `auth_sessions` is no longer written at v0.22.0, its |
| + rows having been migrated into the shared `sessions` table, so the leak |
| + moved rather than closed. Verified at source, filed upstream as |
| + `rastrillo/rastrillo` #2. We hand-roll session minting until that lands, |
| + and take classification and the Keymail upgrade from **`keymaildev/signin` |
| + directly, not from `auth`** — corrected 2026-09-05: `auth`'s |
| + classification is reachable only through `auth.Begin`, which hardwires |
| + `Links: &linkStore{db: cfg.DB}` and writes `auth_links.address`, so "use |
| + `auth` only for the parts that hold nothing" was never a thing that could |
| + be done. `signin.Flow` takes a `LinkStore` the app supplies, so the app |
| + seals what it stores. |
| - **The home hashes; instances never hold the pepper.** A pepper on every |
| instance makes one compromise an offline dictionary attack against the |
| whole suite directory. |
diff --git a/docs/superpowers/plans/2026-09-03-home-signin.md b/docs/superpowers/plans/2026-09-03-home-signin.md| new file mode 100644 |
| index 0000000..3cc10c1 |
| --- /dev/null |
| +++ b/docs/superpowers/plans/2026-09-03-home-signin.md |
| @@ -0,0 +1,1394 @@ |
| +# Oficina home — §13 step 2: sign-in, second factor, recovery codes |
| + |
| +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. |
| + |
| +**Goal:** A person can sign in to the Oficina home with a magic link, optionally add a passkey as a second factor, and recover with a one-time code — with no readable email address anywhere at rest. |
| + |
| +**Architecture:** Hand-rolled sign-in on the credential origin. `keymaildev/signin` supplies classification and the Keymail upgrade; `rastrillo/auth` is not used at all (see below), never for its link or session stores, because those hold readable addresses (ruling 14). The session Subject is an opaque person ref minted at first sign-in; link state is sealed before it is written. This is the first step that opens a database, so it is also the step that makes gates 1 and 2 buildable — and therefore mandatory. |
| + |
| +**Tech Stack:** Go 1.25.0, rastrillo v0.22.0, chi v5, modernc.org/sqlite (cgo-free), zero JavaScript. |
| + |
| +**Spec:** `docs/superpowers/specs/2026-08-30-home-design.md` — §5.1, §5.2, §10, §12, and §13 step 2. Read it alongside this plan; every task argues from it. |
| + |
| +## Global Constraints |
| + |
| +Every task's requirements implicitly include this section. Values are copied verbatim from the spec or AGENTS.md. |
| + |
| +- **No address column exists.** `people` holds person ref, blind index, created_at — "**No address column.**" (§10) |
| +- **The session Subject is an opaque person ref, never an address.** Sessions, credentials and recovery codes all key on it. (§5.1) |
| +- **Link state is sealed before it is written** and short-lived — "never cleaned up afterwards", because deleting a row does not erase bytes from a continuously replicated file. (§5.1, §3.2) |
| +- **`PRAGMA secure_delete` is on.** (§5.1) |
| +- **Migrations are additive-only.** (§10) |
| +- **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 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) |
| +- **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) |
| + |
| +### The gate escalation this step triggers |
| + |
| +§13 step 1 is "Skeleton, `AGENTS.md`, gates 1, 2 and 9". Gate 9 landed in `feat/gate-9-no-content-in-logs`. Gates 1 and 2 could not be built because the app had no database. **This step opens the first database, so this step owes gates 1 and 2** — they are not optional extras at the end: |
| + |
| +- **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 — **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; 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.~~ **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 |
| + `462df84` and the cadence applies as written — branch from it, |
| + squash-merge to it. |
| + |
| +4. ~~Merge order: gate 9 must land first.~~ **Closed 2026-09-05:** gate 9 |
| + merged to `main` at `9162ee5`, so `internal/guardstest/repo_test.go` and |
| + `internal/home/logtest` are both there. Rulings 17-21 are on `main` too, |
| + at `5a42f2d`. |
| + |
| +### A spec-vs-reality note the executor must not "fix" silently |
| + |
| +§5.1 says "We still use `rastrillo/auth` for **address classification and Keymail upgrade** — the part that is hard to get right and holds nothing." **`auth` cannot be used that way.** Its classification is reachable only through `auth.Begin`, which writes `auth_links.address TEXT NOT NULL` — verified at source, and the very thing ruling 14 exists to avoid. |
| + |
| +**Land the correction, do not merely file it.** AGENTS.md's own rule is |
| +that where it disagrees with the spec the spec wins "and this file is wrong |
| +— fix it in the same commit". Both §5.1 and AGENTS.md's Data section say |
| +classification goes through `rastrillo/auth`; both are wrong for the reason |
| +below, and both are on `main`. Fix them in **this branch**, so the plan and |
| +the binding files stop disagreeing the moment it lands. |
| + |
| +The usable seam is one level down: `github.com/keymaildev/signin` exports `Classifier` as a plain struct with exported fields (`HTTP`, `LookupTXT`, `Scheme`, `Now`, `TTL`, `NegTTL`) and a `Classify(ctx, address) Class` method. No constructor, no database, no stores. **Depend on `signin` directly** (rastrillo already requires v0.1.1). This plan does that, and **the correction has landed**: spec §5.1 and AGENTS.md both say `keymaildev/signin` as of this branch. Not filed — fixed. |
| + |
| +--- |
| + |
| +## 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. Sends an `Origin` header, or `csrf.Protect` refuses. Task 5'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)`, `scriptSrcs(t, b)` | Task 7's scriptless sweep. `scriptSrcs` parses the HTML and returns each `<script>`'s `src`, with the empty string for an inline one. | |
| + |
| + |
| +`loadSecrets` is **not** in this table: it is the production loader |
| +`cmd/home/main.go` calls (Task 1 step 5b), not a test helper. |
| + |
| +**Build these in Task 1 (`newTestApp`) and Task 5 (`signIn`, |
| +`capturedLink`), each with its own step and its own test.** An earlier draft |
| +listed them in this table and then had no step create any of them. |
| + |
| +**`newTestApp` must wire a non-nil `Flow.Keymail`.** `Begin` skips |
| +classification entirely when it is nil, so |
| +`TestFailedClassificationStillSendsAMagicLink` would pass vacuously — |
| +green, and proving nothing. |
| + |
| +**`RegisterConnectionHook` is process-global**, so guard it with a |
| +`sync.Once`; `newTestApp` is called by every test. |
| + |
| +**`signIn` must send an `Origin` header**, or `csrf.Protect` refuses its |
| +POSTs and every test fails for the wrong reason. |
| + |
| +**There is a fourth secret: `Flow.Key`.** It seals the Keymail pending blob, |
| +`sealPending` refuses a zero key, and §5.1's "must come from config, never |
| +an instance data key" applies — sign-in runs before any session exists. So |
| +the list Paul provisions is `HOME_DEK`, `HOME_PEPPER`, `HOME_SIGNIN_KEY` |
| +and `CARLOS_SMTP_PASS`. |
| + |
| +**`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", |
| +}) |
| +``` |
| + |
| +**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 |
| +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 |
| + |
| +``` |
| +cmd/home/main.go Resolve -> db.Open -> migrate.Apply -> Serve. |
| +internal/home/ |
| + app.go App struct and wiring. Gains db, sealer, sessions, classifier. |
| + keys.go LoadSealer: HOME_DEK from env, base64, exactly 32 bytes, refuse to boot otherwise. |
| + seal/seal.go DEK, HKDF purpose keys, AES-GCM. Ported from sheets-core. |
| + sealtest/sealtest.go Gate 1: AssertNoPlaintext over raw file, WAL and SHM. Ported. |
| + models.go Schema. people, signin_links. No address column, anywhere. |
| + migrations/0001_people.sql |
| + migrations/0002_signin_links.sql |
| + personref.go Mint and validate the opaque person ref. |
| + signin.go Begin/Verify: sealed link state, one-time redemption, session mint. |
| + links.go signin.LinkStore. Seals on put, one |
| + DELETE ... RETURNING on take. |
| + render.go One parsed tree per page, ui.Funcs plus an asset func. |
| + templates/ Scriptless screens, attribute spelling |
| + (ruling 20; the seven utilities keep class). |
| + logtest/ Gate 9. Already on main (9162ee5). |
| + rpid_test.go Rulings 13/16: the RP ID is the full host. |
| + Package home, not guardstest: it reads |
| + the app's own passkey.Config. |
| + noscript_test.go §11 + ruling 21's allowlist. |
| +internal/guardstest/ |
| + logsinks_test.go Already landed. Forbids non-slog sinks. |
| + noaddress_test.go Gate 2's schema half. |
| +``` |
| + |
| +--- |
| + |
| +### Task 1: The seal package, the DEK, and gate 1 |
| + |
| +The gate goes in with the mechanism, as it did in Sheets. Once this lands, every later task is checked by it. |
| + |
| +**Files:** |
| +- Create: `internal/home/seal/seal.go`, `internal/home/seal/seal_test.go` |
| +- Create: `internal/home/sealtest/sealtest.go` |
| +- Create: `internal/home/keys.go`, `internal/home/keys_test.go` |
| + |
| +**Interfaces:** |
| +- Consumes: nothing. |
| +- Produces: |
| + - `seal.New(dek []byte) (*Sealer, error)` — DEK must be exactly 32 bytes. |
| + - `(*Sealer).Seal(purpose string, plaintext []byte) ([]byte, error)` |
| + - `(*Sealer).Open(purpose string, ciphertext []byte) ([]byte, error)` |
| + - `home.LoadSealer() (*seal.Sealer, error)` — reads `HOME_DEK`. |
| + - `sealtest.Canaries [][]byte`, `sealtest.AssertNoPlaintext(t *testing.T, dbPath string)` |
| + - Purposes: `"signin.address"`, `"team.name"`, `"queue.payload"`. |
| + |
| +Port `seal.go` from `oficina/sheets-core/internal/sheets/seal/seal.go` **unchanged except the HKDF info string**, which becomes `"oficina/home:"+purpose`. Do not redesign it: two apps disagreeing about their envelope format is a bug waiting to differ. Likewise `sealtest.go`, keeping its `leaked`/`findLeak` split — a failing subtest always fails its parent, so the scan must be unit-testable without a live `*testing.T`. |
| + |
| +- [ ] **Step 1: Write the failing round-trip test** |
| + |
| +```go |
| +package seal |
| + |
| +import ( |
| + "bytes" |
| + "testing" |
| +) |
| + |
| +func testDEK() []byte { |
| + d := make([]byte, 32) |
| + for i := range d { |
| + d[i] = byte(i) |
| + } |
| + return d |
| +} |
| + |
| +func TestSealOpenRoundTrip(t *testing.T) { |
| + s, err := New(testDEK()) |
| + if err != nil { |
| + t.Fatalf("New: %v", err) |
| + } |
| + ct, err := s.Seal("signin.address", []byte("person@example.invalid")) |
| + if err != nil { |
| + t.Fatalf("Seal: %v", err) |
| + } |
| + if bytes.Contains(ct, []byte("example.invalid")) { |
| + t.Fatal("ciphertext contains the plaintext") |
| + } |
| + pt, err := s.Open("signin.address", ct) |
| + if err != nil { |
| + t.Fatalf("Open: %v", err) |
| + } |
| + if string(pt) != "person@example.invalid" { |
| + t.Fatalf("Open = %q", pt) |
| + } |
| +} |
| + |
| +// A value sealed under one purpose must not open under another. These are |
| +// adjacent columns of the same row; a bug that swapped them would be |
| +// invisible without this. |
| +func TestWrongPurposeFails(t *testing.T) { |
| + s, _ := New(testDEK()) |
| + ct, _ := s.Seal("signin.address", []byte("secret")) |
| + if _, err := s.Open("team.name", ct); err == nil { |
| + t.Fatal("opened under the wrong purpose") |
| + } |
| +} |
| + |
| +func TestRejectsShortDEK(t *testing.T) { |
| + if _, err := New(make([]byte, 31)); err == nil { |
| + t.Fatal("accepted a 31-byte DEK") |
| + } |
| +} |
| +``` |
| + |
| +- [ ] **Step 2: Run it and watch it fail** |
| + |
| +Run: `go test ./internal/home/seal/` |
| +Expected: FAIL, build error — `undefined: New`. |
| + |
| +- [ ] **Step 3: Port seal.go** |
| + |
| +Copy `oficina/sheets-core/internal/sheets/seal/seal.go` to `internal/home/seal/seal.go`. Change only the package doc and the HKDF info string: |
| + |
| +```go |
| +r := hkdf.New(sha256.New, s.dek, nil, []byte("oficina/home:"+purpose)) |
| +``` |
| + |
| +Add `golang.org/x/crypto` with `go get golang.org/x/crypto`. |
| + |
| +- [ ] **Step 4: Run the tests** |
| + |
| +Run: `go test ./internal/home/seal/` |
| +Expected: PASS. |
| + |
| +- [ ] **Step 5: Write LoadSealer and its test** |
| + |
| +```go |
| +// LoadSealer reads HOME_DEK and returns the process Sealer. |
| +// |
| +// It refuses to boot rather than generating a key. A generated key would |
| +// let the app start with every previously sealed link unreadable, which |
| +// looks like data loss and is discovered late. |
| +func LoadSealer() (*seal.Sealer, error) { |
| + raw := os.Getenv("HOME_DEK") |
| + if raw == "" { |
| + return nil, errors.New("HOME_DEK is not set") |
| + } |
| + dek, err := base64.StdEncoding.DecodeString(raw) |
| + if err != nil { |
| + return nil, fmt.Errorf("HOME_DEK is not valid base64: %w", err) |
| + } |
| + return seal.New(dek) |
| +} |
| +``` |
| + |
| +Test that unset, malformed and 31-byte values each error, and a valid one returns a Sealer. Use `t.Setenv`. |
| + |
| +- [ ] **Step 5a: Port `sealtest` itself** |
| + |
| +The plan commits `internal/home/sealtest` and no step ever created it. |
| +Port `sealtest.go` from sheets-core **with its own `TestLeakedDetectsCanary` |
| +and WAL tests**, and **extend `TestLogtestOnlyImportedByTests` to cover |
| +`sealtest` too** — on `main` it checks a single `const logtestImport`, so this |
| +turns that constant into a slice of two. Like `logtest`, it imports |
| +`testing` from a non-test file, so importing it from production code would |
| +drag the testing package into the binary. — a detector nobody has watched detect anything is the |
| +thing gate 9's review found twice. |
| + |
| +Export **`CanaryAddress`** — a lowercase, address-shaped |
| +`canary-8f3a@example.invalid` — and **include it in `Canaries`**, or gate 2 |
| +signs in with a value `AssertNoPlaintext` never scans for and passes while |
| +proving nothing. (Task 5 step 6's deliberate break would expose it, which is |
| +what that step is for; say it anyway.) Sheets' `Canaries` is |
| +`[][]byte` and shaped like `CANARY-CELL-VALUE-8f3a`; both matter here. |
| +Case, because the blind index and `signin`'s limiter key on |
| +`strings.ToLower(address)`, so the lowercased copy is the one likeliest to |
| +reach a store. Shape, because gate 2 signs *in* with the canary, so it has |
| +to parse as an address — `SplitAddress` rejects a value with two `@`. |
| + |
| +- [ ] **Step 5b: `loadSecrets`, and the boot refusals** |
| + |
| +**All four secrets, in one place, refusing rather than degrading.** Three |
| +of them had no loader at all in an earlier draft, and the app would have |
| +booted without them. |
| + |
| +```go |
| +// loadSecrets reads the four, and refuses to boot on any of them. |
| +// HOME_DEK seals link state and queue payloads (Task 1) |
| +// HOME_PEPPER the blind index (ruling 3, Task 5) |
| +// HOME_SIGNIN_KEY seals signin.Flow's Keymail pending blob |
| +// CARLOS_SMTP_PASS the mailer |
| +``` |
| + |
| +Two refusals that are not about secrets, and both are silent failures |
| +sheets already guards: |
| + |
| +- **No mailer.** `mail.Logged`'s `Send` logs `"to", to` — an address into |
| + platform-held logs on **every** sign-in, §3.4's leak arriving through a |
| + fallback nobody chose. Sheets' `LoadAuthEnv` hard-exits; do the same. |
| +- **Empty `DBPath`.** `Resolve` sets it only from `-db`, and SQLite treats |
| + an empty filename as a **private temporary database** — the app comes up, |
| + works, and loses every person on restart. |
| + |
| +Test each refusal with `t.Setenv`. |
| + |
| +- [ ] **Step 5c: Wire `main.go`, and build `newTestApp`** |
| + |
| +`cmd/home/main.go` must call `loadSecrets` and hand the Sealer to the app — |
| +sheets' `main.go` calls `LoadSealer()` and an earlier draft of this plan |
| +edited `main.go` only to wire the database, so three of four secrets were |
| +never loaded on the boot path. |
| + |
| +`newTestApp(t, dbPath)` is the helper every later task's tests use: temp DB, |
| +migrations applied, capturing `mail.Sender`, Sealer from a test DEK, test |
| +pepper, and a `signin.Flow` with a stub `LookupTXT` **and a non-nil |
| +`Keymail`** — `Begin` skips classification entirely when `Keymail` is nil, |
| +so the fail-open test would pass vacuously without it. Register the |
| +`secure_delete` connection hook here too, under a `sync.Once`, and **before** |
| +`db.Open` (Task 2 step 5). |
| + |
| +**It starts small and grows.** `BootSchema` is Task 2's, the `signin` |
| +dependency is Task 4's and `App.flow` is Task 5's, so build it here with the |
| +database and the Sealer only, and let Tasks 2, 4 and 5 extend it as each |
| +lands. An earlier draft described the finished helper in the first task that |
| +needs one, which does not compile. |
| + |
| +**The stub `LookupTXT` must return a real error, not `nil, nil`.** An empty |
| +TXT result makes `signin` fall through to `delegate`, which returns the |
| +domain itself, and `domainInfo` then makes a live HTTPS GET to |
| +`https://<domain>/.well-known/keymail`. Every `signIn` in the suite would |
| +touch the network. |
| + |
| +- [ ] **Step 6: Commit** |
| + |
| +```bash |
| +git add internal/home/seal internal/home/sealtest internal/home/keys.go internal/home/keys_test.go internal/home/testapp_test.go internal/home/app.go cmd/home/main.go go.mod go.sum |
| +git commit -m "Seal values at rest, load the four secrets, and the plaintext gate that proves it" |
| +``` |
| + |
| +--- |
| + |
| +### Task 2: The database, the schema, and gate 2's schema half |
| + |
| +**Files:** |
| +- Modify: `cmd/home/main.go` |
| +- Create: `internal/home/models.go`, `internal/home/migrations/0001_people.sql` |
| +- Create: `internal/guardstest/noaddress_test.go` |
| +- Modify: `internal/guardstest/repo_test.go` (add `migrationFiles`, `readFile`) |
| + |
| +**Interfaces:** |
| +- Produces: `home.Schema *migrate.Set`, `home.BootSchema *migrate.Set`, and an `App` holding `*db.DB`. |
| + |
| +Follow sheets-core's boot shape exactly — `Resolve`, open the DB yourself, `migrate.Apply`, then blank `DBPath` before `Serve`, because `Serve` would otherwise open a second handle: |
| + |
| +```go |
| +opts, err := rastrillo.Resolve(rastrillo.Options{Logger: logger}) |
| +d, err := rdb.Open(opts.DBPath, logger) |
| +_, err = migrate.Apply(context.Background(), d, home.BootSchema) |
| +opts.Mux = mux |
| +opts.DBPath = "" |
| +rastrillo.Serve(opts) |
| +``` |
| + |
| +```go |
| +//go:embed migrations/*.sql |
| +var migrationsFS embed.FS |
| + |
| +var Schema = migrate.MustFromFS(migrationsFS, "home") |
| +var BootSchema = migrate.Merge(sessions.Schema, Schema) |
| +``` |
| + |
| +`auth.Schema` is **deliberately absent**: this app does not use `rastrillo/auth`, and its address-bearing tables must never be created. Migration files are `migrations/NNNN_lowercase_name.sql`, forward-only, ledgered by ID and checksum. |
| + |
| +- [ ] **Step 1: Write gate 2's schema half** |
| + |
| +```go |
| +package guards |
| + |
| +import ( |
| + "regexp" |
| + "strings" |
| + "testing" |
| +) |
| + |
| +// addressish matches a column name that would hold an email address. |
| +// §10: "people — person ref, blind index, created_at. No address column." |
| +var addressish = regexp.MustCompile(`(?i)\b(address|email|e_mail|mail_to|recipient)\b`) |
| + |
| +// TestNoAddressColumn reads every migration and fails on a column whose |
| +// name says it holds an address. This is the cheap half of gate 2; the |
| +// expensive half — a full sign-in leaves no readable address anywhere, |
| +// WAL included — is Task 4 and needs a running sign-in. |
| +func TestNoAddressColumn(t *testing.T) { |
| + root := repoRoot(t) |
| + for _, path := range migrationFiles(t, root) { |
| + rel := relPath(t, root, path) |
| + for i, line := range strings.Split(readFile(t, path), "\n") { |
| + // A sealed blob is not an address, and says so in its name. |
| + if strings.Contains(strings.ToLower(line), "sealed") { |
| + continue |
| + } |
| + if addressish.MatchString(line) { |
| + t.Errorf("%s:%d: %q names an address column; §10 says there is none", |
| + rel, i+1, strings.TrimSpace(line)) |
| + } |
| + } |
| + } |
| +} |
| +``` |
| + |
| +Add `migrationFiles` and `readFile` beside the existing helpers in `repo_test.go`, following their shape (`t.Helper()`, `t.Fatalf` on error). |
| + |
| +- [ ] **Step 2: Run it** |
| + |
| +Run: `go test ./internal/guardstest/ -run TestNoAddressColumn` |
| +Expected: PASS trivially — no migrations yet. That is correct; it is a tripwire, like gate 9. |
| + |
| +- [ ] **Step 3: Write the first migration** |
| + |
| +`internal/home/migrations/0001_people.sql`: |
| + |
| +```sql |
| +CREATE TABLE people ( |
| + person_ref TEXT PRIMARY KEY, |
| + blind_index TEXT NOT NULL UNIQUE, -- UNIQUE already indexes it; no second index |
| + created_at TEXT NOT NULL |
| +); |
| +``` |
| + |
| +No address column, no email column. The blind index is `HMAC(pepper, lowercased address)` per ruling 3 — **but §3.1 says the index "uses an expensive KDF rather than a bare HMAC", contradicting ruling 3 and §6.** Filed as task 2 on `skeleton`; it **must be settled before the pepper is implemented in step 4**. This task creates the column; nothing writes it yet. |
| + |
| +- [ ] **Step 4: Verify** |
| + |
| +Run: `go test ./...` |
| +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). |
| + |
| +**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 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. `db.DB` exposes no reader |
| +accessor, so reach one through `d.G.Clauses(dbresolver.Read)`. `migrate.Apply` takes a |
| +`*db.DB`, so bypassing `db.Open` is not an escape route. |
| + |
| +- [ ] **Step 6: Commit** |
| + |
| +```bash |
| +git add internal/home/models.go internal/home/migrations internal/home/app.go internal/home/testapp_test.go cmd/home/main.go internal/guardstest |
| +git commit -m "Open the database, and the schema that has no address column" |
| +``` |
| + |
| +--- |
| + |
| +### Task 3: The person ref |
| + |
| +**Files:** |
| +- Create: `internal/home/personref.go`, `internal/home/personref_test.go` |
| + |
| +**Interfaces:** |
| +- Produces: `home.NewPersonRef() (string, error)`, `home.ValidPersonRef(s string) bool` |
| + |
| +The person ref is the subject of every session, credential and recovery code, and later the assertion's `sub` and the directory's join key. Opaque, stable across an email change, and **never derived from the address** — a derived ref puts the address back at rest in a reversible form under another name. |
| + |
| +- [ ] **Step 1: Write the failing tests** |
| + |
| +```go |
| +func TestPersonRefIsOpaqueAndUnique(t *testing.T) { |
| + seen := map[string]bool{} |
| + for i := 0; i < 1000; i++ { |
| + ref, err := NewPersonRef() |
| + if err != nil { |
| + t.Fatalf("NewPersonRef: %v", err) |
| + } |
| + if seen[ref] { |
| + t.Fatalf("duplicate person ref %q after %d draws", ref, i) |
| + } |
| + seen[ref] = true |
| + if !ValidPersonRef(ref) { |
| + t.Fatalf("NewPersonRef produced %q, which ValidPersonRef rejects", ref) |
| + } |
| + } |
| +} |
| + |
| +func TestPersonRefDoesNotEmbedInput(t *testing.T) { |
| + ref, _ := NewPersonRef() |
| + if strings.Contains(ref, "@") { |
| + t.Fatalf("person ref %q contains an @", ref) |
| + } |
| +} |
| + |
| +func TestValidPersonRefRejectsRubbish(t *testing.T) { |
| + for _, bad := range []string{"", "person@example.invalid", "../etc", strings.Repeat("a", 500)} { |
| + if ValidPersonRef(bad) { |
| + t.Errorf("ValidPersonRef(%q) = true", bad) |
| + } |
| + } |
| +} |
| +``` |
| + |
| +- [ ] **Step 2: Run, watch it fail** |
| + |
| +Run: `go test ./internal/home/ -run PersonRef` |
| +Expected: FAIL — `undefined: NewPersonRef`. |
| + |
| +- [ ] **Step 3: Implement** |
| + |
| +```go |
| +// personRefBytes is 16 bytes of crypto/rand, base32 without padding. |
| +// Long enough that collisions are not a concern at any plausible number |
| +// of people; short enough to read in a log line, which it is allowed to |
| +// appear in — §3.4: "Errors carry person refs and codes." |
| +const personRefBytes = 16 |
| + |
| +func NewPersonRef() (string, error) { |
| + b := make([]byte, personRefBytes) |
| + if _, err := rand.Read(b); err != nil { |
| + return "", err |
| + } |
| + enc := base32.StdEncoding.WithPadding(base32.NoPadding) |
| + return "pr_" + strings.ToLower(enc.EncodeToString(b)), nil |
| +} |
| +``` |
| + |
| +`ValidPersonRef` checks the `pr_` prefix, the exact expected length, and the alphabet. |
| + |
| +- [ ] **Step 4: Run** |
| + |
| +Run: `go test ./internal/home/ -run PersonRef` |
| +Expected: PASS. |
| + |
| +- [ ] **Step 5: Commit** |
| + |
| +```bash |
| +git add internal/home/personref.go internal/home/personref_test.go |
| +git commit -m "The person ref: opaque, minted, never derived from an address" |
| +``` |
| + |
| +--- |
| + |
| +### 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. |
| + |
| +Where the blindness claim is either kept or quietly broken. |
| + |
| +**Files:** |
| +- Create: `internal/home/links.go`, `internal/home/links_test.go` |
| +- Create: `internal/home/migrations/0002_signin_links.sql` |
| +- Modify: `go.mod` — add `github.com/keymaildev/signin` (the `LinkStore` |
| + interface is defined there, so the dependency arrives with the store, |
| + not with the handlers) |
| + |
| +**Interfaces:** |
| +- Consumes: `seal.Sealer` (Task 1) and the database (Task 2) — and nothing else. |
| +- Produces: `(*App).links`, a `signin.LinkStore`. **Routes nothing, and needs no `sessions` or `mail.Sender`** — 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`: |
| + |
| +```sql |
| +CREATE TABLE signin_links ( |
| + token_hash TEXT PRIMARY KEY, |
| + sealed_address BLOB NOT NULL, |
| + expires_at TEXT NOT NULL |
| +); |
| +CREATE INDEX idx_signin_links_expires_at ON signin_links(expires_at); |
| +``` |
| + |
| +**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**. (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 the LinkStore's failing tests** |
| + |
| +Four properties, driven directly against the store rather than through a |
| +handler: |
| + |
| +```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 |
| +``` |
| + |
| +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 them** |
| + |
| +Run: `go test ./internal/home/ -run TestPutLink` |
| +Expected: FAIL — no store. |
| + |
| +- [ ] **Step 3: Implement PutLink and TakeLink** |
| + |
| +`PutLink` seals then inserts. `TakeLink` is the single `DELETE … RETURNING` |
| +above. |
| + |
| +**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. |
| + |
| +**`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.) |
| + |
| +- [ ] **Step 4: Prove the seal bites** |
| + |
| +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. |
| + |
| +- [ ] **Step 5: Sweep expired links** |
| + |
| +Sealed rows are not a leak, but unbounded growth is a bug. Delete |
| +`WHERE expires_at < ?` **at boot and on each `PutLink`**, the shape |
| +`passkey.Sweep` suggests. |
| + |
| +**And sweep the other two while building the habit**, because nobody else |
| +does: `sessions.Sweep` (rows live 30 days) and `passkey.Sweep` |
| +(`passkey_pending`, `passkey_challenges`). Same boot-plus-on-write shape. |
| +None of these leak — they are keyed on person refs — but a table nothing |
| +ever deletes from is a bug waiting for a big enough database. (Not "the wake-driven schedule §6 uses" — that |
| +sentence is about *instances*, and AGENTS.md says **the home does not |
| +hibernate**, so there are no wakes to hang it on. A ticker would work here |
| +and is simply not worth a goroutine for this.) |
| + |
| +- [ ] **Step 6: Commit** |
| + |
| +```bash |
| +git add internal/home/links.go internal/home/links_test.go internal/home/migrations/0002_signin_links.sql internal/home/testapp_test.go go.mod go.sum |
| +git commit -m "A sign-in link store that seals before it writes and takes exactly once" |
| +``` |
| + |
| +--- |
| + |
| +### Task 5: `signin.Flow`, the sign-in handlers, and the Keymail upgrade |
| + |
| +**Files:** |
| +- Create: `internal/home/signin.go`, `internal/home/signin_test.go`, `internal/home/render.go` |
| +- Create: `internal/home/templates/signin.html`, `check_your_mail.html`, `signin_confirm.html` |
| +- Modify: `internal/home/app.go` |
| + |
| +**Interfaces:** |
| +- Produces: `(*App).flow` a `*signin.Flow`, and the four routes. Consumes `(*App).links` from Task 4. |
| + |
| +**`(*App).render` is built HERE, minimally, and Task 7 extends it.** These |
| +three templates are its first callers, and Tasks 5 and 6 cannot execute |
| +without it — so the earlier draft's "either land Task 7 first or build it |
| +here, but say which" is answered: build it here. Minimal means one parsed |
| +tree per page over `ui.Templates()` plus the app's layout, no asset |
| +pipeline. Task 7 adds the vendored assets, the scriptless sweep and the |
| +RP-ID guard on top. |
| + |
| +**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. |
| + |
| +`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): |
| + |
| +```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` (rastrillo's `mail.Sender` and `signin.Mailer` have identical |
| +method sets, so the capturing test sender plugs straight in with no |
| +adapter), `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 collapses into a suite-wide sign-in DoS, and this is |
| +settled here rather than left to be found.** An earlier draft of this |
| +paragraph said `RemoteAddr` is `""` behind the unix socket and the budget is |
| +therefore skipped. **Measured, and it is worse than that:** over a Linux |
| +unix socket `r.RemoteAddr` is `"@"`. `net.SplitHostPort("@")` fails, `auth`'s |
| +`clientIP` returns `"@"` unchanged, and `Flow.Begin` skips the budget only |
| +when the key is exactly `""`. So **every request in the world shares the key |
| +`ip:@`, with a budget of 20 per 15 minutes** — twenty POSTs to `/signin` lock |
| +out sign-in for the entire suite for a quarter of an hour. rastrillo's own |
| +comment admits the shape ("collapsing the per-IP budget into a global one"). |
| +§2.1 already names this service the suite's single point of failure; this |
| +would make it a trivially reachable one. |
| + |
| +**Take the client IP from `X-Forwarded-For`.** The carlos edge sets it on |
| +every proxied request (`SetXForwarded`) and strips any inbound |
| +`Forwarded`/`X-Forwarded-*` on unverified hops (`stripForwardedHeaders`), so |
| +the header is trustworthy at the app and a client cannot forge it. Use the |
| +last element; fall back to `RemoteAddr` only when the header is absent, and |
| +**never pass `"@"` as a limiter key**. Test both paths — a request with the |
| +header gets its own budget, one without does not silently share a global |
| +one. |
| + |
| +**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 gate 2 in full, as a failing test** |
| + |
| +```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) |
| + |
| + // 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. |
| + // sealtest.CanaryAddress, not a composed string: the ported Canaries |
| + // is [][]byte, and the ported canaries are already address-shaped |
| + // (canary-8f3a@example.invalid), so concatenating an "@example.invalid" |
| + // onto one gives a two-@ value that SplitAddress rejects outright. |
| + signIn(t, app, sealtest.CanaryAddress) |
| + sealtest.AssertNoPlaintext(t, dbPath) |
| +} |
| +``` |
| + |
| +Plus the classification test, asserting on the flow's decision rather than |
| +on a constant. **`signin.ClassKeymail` 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) { |
| + app := newTestApp(t, filepath.Join(t.TempDir(), "home.db")) |
| + app.flow.Classifier.LookupTXT = func(context.Context, string) ([]string, error) { |
| + return nil, errors.New("servfail") |
| + } |
| + // 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) |
| + } |
| + if next.Kind != signin.NextMagicLink { |
| + t.Fatalf("Kind = %v, want NextMagicLink: classification must fail open", next.Kind) |
| + } |
| +} |
| +``` |
| + |
| +- [ ] **Step 2: Run them** |
| + |
| +Expected: FAIL — no `/signin` route, no `flow` field. |
| + |
| +- [ ] **Step 3: Wire the Flow and implement Begin** |
| + |
| +`POST /signin` calls `Flow.Begin` and then **branches on `next.Kind`** — |
| +an earlier draft said "303 to a check-your-mail page, always", which drops |
| +the Keymail upgrade this task exists to deliver: |
| + |
| +- `NextMagicLink` — the mail has already been sent by the time `Begin` |
| + returns. 303 to the check-your-mail page. |
| +- `NextKeymail` — **no mail was sent.** `Begin` returns `Redirect` and |
| + `Pending`; set the pending cookie and 303 to `next.Redirect`. |
| + |
| +**Both branches must be indistinguishable to an attacker probing for known |
| +addresses.** The Keymail redirect necessarily differs, but it is decided by |
| +the address's *domain*, which the submitter already knows — it discloses |
| +nothing about whether that person has an account here. |
| + |
| +**The pending cookie's attributes are load-bearing and `auth` keeps them |
| +unexported, so they must be replicated deliberately:** `__Host-` prefix, |
| +`SameSite=Lax`, `HttpOnly`, `Secure`, and **cleared on read**. They are |
| +exactly what the "callback may mint on GET" argument below rests on; get |
| +them wrong and that argument stops being true. |
| + |
| +**Carry the `force` escape hatch.** `auth.Callback` redirects to |
| +`?force=1&err=keymail` when the Keymail round trip fails, and without it a |
| +failed approval is a dead end — against §5.1's "every address has a working |
| +path". `Flow.Begin`'s last parameter is that override. |
| + |
| +- [ ] **Step 4: 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. 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 4a: Build `blindIndex`, find-or-mint, and the test helpers** |
| + |
| +The blind index and the find-or-mint upsert were prose in Task 4 and in |
| +nobody's `git add`. They belong here, with the person lookup that uses them: |
| +`blindIndex(pepper, address)`, the `INSERT … ON CONFLICT(blind_index) DO |
| +UPDATE … RETURNING person_ref`, and `people.created_at` written coarsened to |
| +the day. |
| + |
| +`signIn(t, app, addr)` and `capturedLink(t, app)` are built here too — every |
| +later test goes through them. **`signIn` must send an `Origin` header**, or |
| +`csrf.Protect` refuses its POSTs and every test fails for the wrong reason. |
| + |
| +- [ ] **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/signin.go internal/home/signin_test.go internal/home/render.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 |
| + |
| +**Unblocked 2026-09-05** by ruling 21: §11 has its carve-out, so the |
| +enrolment and ceremony screens may load script and nothing else may. |
| + |
| +**Files:** |
| +- Create: `internal/home/passkey.go`, `internal/home/passkey_test.go` |
| +- Create: `internal/home/templates/passkey_confirm.html`, `passkey_enrol.html`, `recovery_codes.html` |
| +- Create: `internal/home/static/passkey.js` — the app's own glue (under `internal/home/` so `go:embed` can reach it) |
| +- Modify: `internal/home/app.go`, `internal/home/models.go` (where Task 2 put `Schema`/`BootSchema` — there is no `db.go`) |
| + |
| +**Interfaces:** |
| +- Produces: `(*App).passkeys *passkey.Handlers`, mounted at the seven routes below. |
| + |
| +Verified at v0.22.0 so the executor need not re-derive it: |
| +- `recoveryCodesPerSet = 10` — matches §5.2's "ten" exactly. |
| +- `SignInRecovery` is `r.PostFormValue("code")` — **a plain form POST with no JS**, matching §5.2. The package says why: "recovery is exactly the moment WebAuthn isn't working." |
| +- Codes are base32, 50 bits each, hashed with `sessions.HashToken`, redeemed with a single `DELETE … RETURNING`. No attempt counter, and that is sound rather than an omission: `SignInRecovery` needs a live `passkey_pending` half-session, so the window is bounded by `pendingTTL` rather than by a counter. |
| +- `SignInFinish` (the Gate path) mints `Method` as the first factor plus `"+passkey"` — `"magiclink+passkey"`. `StepUpFinish` is the different one: it mints `"passkey"` alone. Both set a fresh `AuthTime`, which is what `RequireFresh` checks. |
| +- Losing passkey and codes together is a permanent lockout: §5.2's disclosed outcome with no operator reset (§14). Not a gap to fill. |
| + |
| +**Struck from an 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. The Gate never signs anybody in |
| +from nothing, and §5.2 defines codes as recovery **for the second factor**; |
| +codes alone are a first-factor bypass the spec never authorised. |
| + |
| +- [ ] **Step 1: Merge `passkey.Schema`** |
| + |
| +Task 2's `migrate.Merge(sessions.Schema, Schema)` omits it, and `Gate` |
| +INSERTs into `passkey_pending`, so without this the first enrolled sign-in |
| +fails at the database. All four passkey tables key on `subject`, which is |
| +already the person ref — so the spec's worry about inheriting `auth`'s |
| +address-bearing subject dissolves. Say so; do not leave it to be |
| +rediscovered. |
| + |
| +- [ ] **Step 2: Write the failing test that one factor is not enough** |
| + |
| +```go |
| +// An enrolled person must NOT obtain a full session from the magic link |
| +// alone. This is the whole point of the Gate, and the test that would |
| +// have caught the callback bypass below. |
| +func TestEnrolledPersonCannotSignInWithOneFactor(t *testing.T) |
| +``` |
| + |
| +Enrol with `webauthn/authtest`, which is what rastrillo's own |
| +`passkey_test.go` uses — do not hand-roll a credential. |
| + |
| +- [ ] **Step 3: Put `Gate` on BOTH sign-in paths** |
| + |
| +At the point each handler would call `sessions.SignIn`, call |
| +`passkey.Handlers.Gate(w, r, sess)` — exactly as `auth.admit` does, and |
| +note that in rastrillo **both `Verify` and `Callback` funnel through that |
| +one `admit`.** This plan has two entry points, so it needs the call twice: |
| + |
| +- `POST /signin/verify` — the magic-link path. |
| +- `GET /signin/callback` — **the Keymail path.** 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. |
| + |
| +An account with no passkey passes the Gate untouched, so the unenrolled |
| +path is unchanged. |
| + |
| +- [ ] **Step 4: Mount the seven routes and write the glue** |
| + |
| +``` |
| +POST /passkey/register/begin | /finish |
| +POST /passkey/stepup/begin | /finish |
| +POST /passkey/signin/begin | /finish |
| +POST /passkey/signin/recovery |
| +``` |
| + |
| +All behind `csrf.Protect`. **Plus the GET pages and assets those seven |
| +imply**, which an earlier draft left unrouted: |
| + |
| +``` |
| +GET /passkey/confirm ConfirmPath — the ceremony + the recovery form |
| +GET /passkey/enrol enrolment |
| +GET /passkey/codes the show-once page, behind RequireFresh |
| +GET /static/webauthn.mjs webauthn.JS(), text/javascript |
| +GET /static/passkey.js the app glue |
| +``` |
| + |
| +**Both on stable, unfingerprinted paths.** Everything else may go through |
| +rastrillo's fingerprinting asset helper; these two may not, because Task 7's |
| +allowlist has to name them, and a rule that cannot name its subject is not a |
| +rule. |
| + |
| + |
| +`ConfirmPath` is `/passkey/confirm` (its default, named rather than |
| +defaulted). `RequireFresh`'s `maxAge` for the codes page is **5 minutes** — |
| +it must be short, because the page shows ten codes once, and it must exceed |
| +the ceremony round trip. |
| + |
| +**Correcting this plan's own earlier claim that "nobody has to write it":** |
| +`webauthn.JS()` embeds `js/webauthn.mjs`, which only *exports* |
| +`register()`/`authenticate()` and makes no network call. The app writes the |
| +glue that imports it, calls it, POSTs to `/passkey/*` and follows the `to` |
| +that `SignInFinish` returns. Both files are served as **static same-origin |
| +assets** — rastrillo's default CSP forbids inline script, and ruling 21's |
| +carve-out is for static files, not for inline blocks. Go's `mime` already maps `.mjs` to `text/javascript; charset=utf-8`, so |
| +serving it from the embedded FS needs nothing special — set the header |
| +explicitly anyway, since a wrong MIME type blocks a module silently. |
| + |
| +`register()` needs `rpId`, `userId` and `userName`, while `/begin` returns |
| +only `challenge`. **`userId` is the person ref. `userName` must not be the |
| +address** — the credential origin never renders an account-authored string |
| +(§4), and the home does not hold the address anyway. A person ref reads |
| +poorly in a password manager; that is the cost of §0 and it is accepted. |
| + |
| +- [ ] **Step 5: The confirm page carries the recovery form** |
| + |
| +The same screen hosts the ceremony *and* `SignInRecovery`'s plain form. |
| +**Script-strip it and the `code` form must still work** — that is gate 6's |
| +requirement under ruling 21, and the reason the allowlist exempts the |
| +ceremony button rather than the page. |
| + |
| +- [ ] **Step 6: Show the codes once, behind `RequireFresh`** |
| + |
| +`RegenerateRecoveryCodes` mints ten, shown once. The package requires that |
| +page to sit behind `sessions.RequireFresh`. |
| + |
| +- [ ] **Step 7: Commit** |
| + |
| +```bash |
| +git add internal/home/passkey.go internal/home/passkey_test.go internal/home/templates internal/home/static internal/home/app.go internal/home/models.go |
| +git commit -m "Passkeys as an optional second factor, gated on both sign-in paths" |
| +``` |
| + |
| +--- |
| + |
| +### Task 7: The scriptless screens, and the RP-ID guard |
| + |
| +**Files:** |
| +- Modify: `internal/home/render.go` (Task 5 created it); create the remaining `internal/home/templates/*.html` |
| +- Create: `internal/home/rpid_test.go` — **package `home`, not `guardstest`**: it reads the app's own config through a helper over the unexported `App`, which a `guards` test cannot reach. |
| +- Create: `internal/home/noscript_test.go` |
| +- Create: `internal/home/static/tokens.css`, `internal/home/static/theme.css` — vendored from rastrillo. **Under `internal/home/`, not the repo root:** `go:embed` cannot reach a parent directory, sheets-core embeds `all:static` from inside its package, and `rastrillo new` scaffolds `internal/<pkg>/static/` with a `TestVendoredAssetsMatchTheLibrary` that reads it there. |
| + |
| +**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 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. |
| + |
| +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 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. |
| +// This test lives in package home, NOT internal/guardstest: it reads the |
| +// config the APP builds, via a helper over the unexported App, and a |
| +// guards-package test cannot call one. |
| +func TestRPIDIsExactlyTheCredentialHost(t *testing.T) { |
| + cfg := passkeyConfig(t) // what the app configures, never a literal |
| + |
| + u, err := url.Parse(cfg.Origin) |
| + if err != nil { |
| + t.Fatalf("parse Origin %q: %v", cfg.Origin, err) |
| + } |
| + if u.Hostname() != "id.oficina.works" { |
| + t.Errorf("RP ID is %q, want id.oficina.works: passkey derives the RP ID "+ |
| + "from Origin's hostname, so widening Origin widens it (rulings 13, 16)", |
| + u.Hostname()) |
| + } |
| + // LegacyRPID must be empty, or a full host, with a comment naming the |
| + // §5.7 drill that is running. HasSuffix, not Contains: Contains accepts |
| + // "x.oficina.works.attacker.example". |
| + if cfg.LegacyRPID != "" && !strings.HasSuffix(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. Ruling 21's carve-out is an |
| +allowlist, and **an allowlist of screens is not enough** — it would admit |
| +any script at all on those screens. Constrain it: |
| + |
| +- The only `<script>` on an allowlisted template is a **same-origin `src`** |
| + from a named list. No inline bodies. |
| +- Assert the served CSP is still rastrillo's default (`Options.CSP` |
| + untouched), so the carve-out cannot quietly widen into |
| + `unsafe-inline`. |
| +- Script-strip the confirm page and assert the recovery `code` form still |
| + posts to `/passkey/signin/recovery` — gate 6's shape, and the reason the |
| + exemption is the ceremony button rather than the screen. |
| + |
| +```go |
| +// scriptAllowed is ruling 21's allowlist, by TEMPLATE and by SRC. Naming |
| +// the screens is not enough — that would admit any script at all on them. |
| +// Paths, not fingerprints. rastrillo's NewAssets.Path fingerprints |
| +// (/static/passkey.<hash>.js), so if these templates used the asset func |
| +// the allowlist would never match and the carve-out screens would fail |
| +// their own sweep. Task 6 routes these two on STABLE paths for exactly |
| +// that reason — they are the only two assets that need to be nameable in |
| +// a security rule. |
| +var scriptAllowed = map[string][]string{ |
| + "passkey_confirm.html": {"/static/webauthn.mjs", "/static/passkey.js"}, |
| + "passkey_enrol.html": {"/static/webauthn.mjs", "/static/passkey.js"}, |
| +} |
| + |
| +func TestNoScriptTagOutsideTheAllowlist(t *testing.T) { |
| + for _, name := range templateNames(t) { |
| + var buf bytes.Buffer |
| + renderTo(t, &buf, name, sampleData(name)) |
| + srcs := scriptSrcs(t, buf.Bytes()) // parses, so an inline body has no src |
| + 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 { |
| + if src == "" { |
| + t.Errorf("%s has an INLINE <script>; the carve-out is for static "+ |
| + "same-origin files, and rastrillo's default CSP forbids inline", name) |
| + } else if !slices.Contains(permitted, src) { |
| + t.Errorf("%s loads %q, which is not in ruling 21's allowlist", name, src) |
| + } |
| + } |
| + } |
| +} |
| + |
| +// The carve-out must not widen into unsafe-inline behind our backs. |
| +// |
| +// Do NOT reference rastrillo's defaultCSP — it is an unexported const in |
| +// serve.go. Assert that the app leaves Options.CSP empty and that the |
| +// SERVED header's default-src and script-src carry no 'unsafe-inline'. |
| +// (§11 says the credential origin moves to script-src 'self'; rastrillo's |
| +// default already delivers that via default-src 'self', so there is |
| +// nothing to override — which is the point of asserting it stays empty.) |
| +func TestServedCSPIsRastrillosDefault(t *testing.T) |
| + |
| +// And the one screen that carries script must still work without it: |
| +// passkey.SignInRecovery is a plain form POST "because recovery is exactly |
| +// the moment WebAuthn isn't working". |
| +func TestConfirmPageRecoveryFormSurvivesScriptStripping(t *testing.T) |
| +``` |
| + |
| +- [ ] **Step 2a: Build the test helpers this task's guards need** |
| + |
| +`templateNames(t)`, `renderTo(t, w, name, data)`, `sampleData(name)`, |
| +`scriptSrcs(t, b)` (parse the HTML and return each `<script>`'s `src`, empty |
| +string for an inline one) and `passkeyConfig(t)` — all five are used by the two guards above and created |
| +by no step in an earlier draft. `passkeyConfig` must return the config the |
| +**app** builds, not one the test declares, or the RP-ID guard asserts a |
| +constant against itself. `passkey.Handlers` keeps its config unexported, so |
| +**the `App` must retain the `passkey.Config` it passed to `passkey.New`** |
| +for the helper to hand back — one field, and the executor otherwise loses an |
| +hour to it. |
| + |
| +- [ ] **Step 3: Commit** |
| + |
| +```bash |
| +git add internal/home/render.go internal/home/templates internal/home/static internal/home/rpid_test.go internal/home/noscript_test.go |
| +git commit -m "Scriptless sign-in screens, and the guard that the RP ID is a full host" |
| +``` |
| + |
| +--- |
| + |
| +## Deferred, with triggers |
| + |
| +| Thing | Trigger | |
| +|---|---| |
| +| ~~Passkey enrollment and step-up~~ | **Ruled 2026-09-05 (ruling 21): carve-out granted, Task 6 builds it.** | |
| +| ~~The pepper and the blind index~~ — **built in Task 5** (ruling 3 is settled) | §3.1's "expensive KDF" vs ruling 3's `HMAC` is settled — task 2 on `skeleton` | |
| +| Assertion minting, `jti`, the redirect protocol | §13 step 3 | |
| +| The directory, calls 2 and 3 | §13 step 4 | |
| +| Operator reset | §5.2: "the first real person locked out with passkey and codes both lost" | |
| + |
| +## Self-review notes |
| + |
| +- **Spec coverage:** §5.1 is Tasks 1–5; §5.2 is Task 6, unblocked by ruling 21; §10's `people`, `signin_links` and `sessions` are Tasks 2–4; §12 gates 1 and 2 are Tasks 1 and 5; §11 is Task 7. §5.3–§5.6 are step 3, out of scope and listed above. |
| +- **Deliberately not covered:** `prefs`, `directory`, `teams`, `provisioning_queue`, `rekey_journal`, `consumers`, `assertion_jti` — later steps. |
| +- ~~Two spec corrections needed~~ — **landed in this branch**, 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.~~ **It did not exist** — `Class` is a struct. Flagging it was the right instinct; writing it anyway was the wrong response. Verify, or do not write. Every other signature in this plan was read at source, and the 2026-09-05 review pass re-read them at the pin. |
| +- ~~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. |
diff --git a/docs/superpowers/specs/2026-08-30-home-design.md b/docs/superpowers/specs/2026-08-30-home-design.md| index d254bc7..b3ee301 100644 |
| --- a/docs/superpowers/specs/2026-08-30-home-design.md |
| +++ b/docs/superpowers/specs/2026-08-30-home-design.md |
| @@ -62,7 +62,6 @@ blind, replaceable*. |
| | 18 | **Activity is approved in principle, pulled at render, and sequenced last** — the front door shows a real feed with titles, the home holds none of it at rest, and its slot is a step 10 after step 4 — `design-activity-pull` | Push into the home; a contentless heat map; refusing the feature | Paul, 2026-09-05 | |
| | 19 | **A team name is reserved suite-wide. Both paths end in the drain — an existing instance settles the label question for the operator, it does not bypass them. The home answers only facts about its own records, never "may this person"** — §7 | Per-app first-come, which lets a stranger squat `acme.sheets` behind your `acme.docs` on a domain that reads as one suite | Paul, 2026-09-05 | |
| | 20 | **Write the attribute spelling — `rst-btn`, not `class="rst-btn"`. The seven unpaired utilities keep `class`** — §11 | The class vocabulary, which this section ruled on 2026-08-30 and whose every stated reason has since died; "follow whatever the pin's partials use", which changes this app's markup on a dependency bump | Paul, 2026-09-05 | |
| - |
| | 21 | **§11's zero-JS rule gains one named carve-out: passkey enrolment and ceremony screens may load script. Everything else stays scriptless** — §11 | Passkeys leaving slice one; a blanket exemption; a progressive-enhancement version, since the ceremony itself cannot happen without script either way | Paul, 2026-09-05 | |
| --- |
| @@ -316,8 +315,15 @@ a guarantee into a comment. |
| rest — sealed, enumerated in §3.2, never readable. |
| - **`PRAGMA secure_delete`**, because deleting a row does not remove bytes |
| from a file that is continuously replicated. |
| -- We still use `rastrillo/auth` for **address classification and Keymail |
| - upgrade** — the part that is hard to get right and holds nothing. |
| +- We use **`keymaildev/signin` directly** for classification and the Keymail |
| + upgrade — the part that is hard to get right and holds nothing. |
| + **Corrected 2026-09-05:** an earlier draft said `rastrillo/auth`, which |
| + cannot serve that purpose. Its classification is reachable only through |
| + `auth.Begin`, and `Begin` writes `auth_links.address TEXT NOT NULL` — the |
| + very thing ruling 14 exists to avoid. The seam is one level down: |
| + `signin.Flow` takes a `LinkStore` the app supplies, so the app seals in |
| + `PutLink` and opens in `TakeLink`, and classification, the Keymail round |
| + trip and the rate limits all come with it. |
| **Filed upstream as `rastrillo/rastrillo` #2**: a `Config.Subject` hook, a |
| sealed link store, and written `secure_delete`/WAL guidance. Every |