oficina / home Public

Fourth review pass: the gates would not compile, and three secrets were never loaded

The design cleared this round — the seal, the sealed link store, the
single DELETE ... RETURNING, GET-renders/POST-redeems, the
X-Forwarded-For fix, the Gate on both paths and the RP-ID rule were all
verified sound at the pin. What blocked was that the tests which ARE
the gates would not run, and the boot path never loaded most of what
it needs.

Gate 2's test could not compile. sealtest.Canaries is [][]byte, so
"canary-" + Canaries[0] is a type error; and since the ported canaries
are already address-shaped, concatenating "@example.invalid" onto one
gives a two-@ value SplitAddress rejects outright. The port now exports
CanaryAddress and the test uses it. The one test that is gate 2 has to
be the one test that runs.

Three of the four secrets were never loaded. HOME_PEPPER,
HOME_SIGNIN_KEY and the SMTP password were named in a table and created
by no step, and cmd/home/main.go was edited only to wire the database —
sheets' main.go calls LoadSealer, ours did not. There is now a
loadSecrets step, a main.go wiring step, and steps that build every
helper the later tasks use: newTestApp, signIn, capturedLink,
passkeyConfig, templateNames, renderTo, sampleData, blindIndex and the
find-or-mint upsert, plus the sealtest port the plan had already
noticed was missing and then not fixed.

The two boot refusals were prose. mail.Logged writes the address to
platform-held logs on every sign-in — §3.4's leak arriving through a
fallback nobody chose — and an empty DBPath makes SQLite open a private
temporary database, so the app works and loses every person on restart.
Both are steps now, each with its own test.

Task 7's no-script test forbade what Task 6 builds. The prose described
an allowlist by template AND by src; the code beneath it failed on any
<script> anywhere, and the executor copies the code. It now carries the
allowlist, rejects inline bodies explicitly, asserts the served CSP is
still rastrillo's default so the carve-out cannot widen into
unsafe-inline, and checks the confirm page's recovery form survives
script-stripping — which is the whole reason the exemption is the
ceremony button rather than the screen.

The RP-ID guard declared the origin as a const inside itself and
asserted it against itself, so it could not fail whatever the app
configured. By this plan's own rule that is a guard written to pass. It
reads passkeyConfig(t).Origin now, and uses HasSuffix, since Contains
accepts x.oficina.works.evil.

The renderer's owner was left as a question. Task 5 builds it minimally
— it has the first callers — and Task 7 extends it. As written neither
Task 5 nor Task 6 could execute.

Plus: static/ moves under internal/home/ because go:embed cannot reach
a parent; the schema merge targets models.go, since no db.go exists;
Task 6 gains the GET routes, ConfirmPath and a RequireFresh max age;
the link sweep drops "wake-driven", because AGENTS.md says the home
does not hibernate and there are no wakes to hang it on; the seven
utilities keep class here as they do in §11; and the merge-order note
is closed, since gate 9 and rulings 17-21 are both on main.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Paul Campbell pushed by paul@keymail.dev ecfe06db5b8f20c21d9f339f3f295290c37ad814 parent e55bd53
1 file changed, +173 −30
  • docs/superpowers/plans/2026-09-03-home-signin.md +173 −30
diff --git a/docs/superpowers/plans/2026-09-03-home-signin.md b/docs/superpowers/plans/2026-09-03-home-signin.md
index 138e290..7227787 100644
--- a/docs/superpowers/plans/2026-09-03-home-signin.md
+++ b/docs/superpowers/plans/2026-09-03-home-signin.md
@@ -77,9 +77,10 @@ helper folds case for the same reason.
`462df84` and the cadence applies as written — branch from it,
squash-merge to it.
-4. **Merge order.** Task 2 modifies `internal/guardstest/repo_test.go`,
- which exists only on `feat/gate-9-no-content-in-logs`. That branch must
- land first, or Task 2 has no file to edit.
+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
@@ -190,7 +191,8 @@ internal/home/
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. class="rst-*".
+ templates/ Scriptless screens, attribute spelling
+ (ruling 20; the seven utilities keep class).
logtest/ Gate 9. Already on feat/gate-9-no-content-in-logs.
internal/guardstest/
logsinks_test.go Already landed. Forbids non-slog sinks.
@@ -321,11 +323,67 @@ func LoadSealer() (*seal.Sealer, error) {
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** — 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`) alongside `Canaries`. 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).
+
- [ ] **Step 6: Commit**
```bash
-git add internal/home/seal internal/home/sealtest internal/home/keys.go internal/home/keys_test.go go.mod go.sum
-git commit -m "Seal values at rest, and the plaintext gate that proves it"
+git add internal/home/seal internal/home/sealtest internal/home/keys.go internal/home/keys_test.go internal/home/testapp_test.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"
```
---
@@ -455,7 +513,8 @@ it is the one that matters. A test asserting on "a fresh connection" would
pass while the writer never had the pragma set, which is the worst kind of
green.
-Assert on `d.Writer()` **and** one reader. `migrate.Apply` takes a
+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**
@@ -572,7 +631,8 @@ Where the blindness claim is either kept or quietly broken.
**Interfaces:**
- Consumes: `seal.Sealer`, `NewPersonRef`, `sessions.Sessions`, `mail.Sender`.
-- Produces: `(*App).links`, a `signin.LinkStore`. Consumes `seal.Sealer` (Task 1) and the database (Task 2). **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`.
+- 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`:
@@ -706,8 +766,11 @@ 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 < ?` on the same wake-driven schedule §6 uses for the
-outbox — **never a ticker**, since a hibernating app runs no timers.
+`WHERE expires_at < ?` **at boot and on each `PutLink`**, the shape
+`passkey.Sweep` suggests. (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**
@@ -728,10 +791,13 @@ git commit -m "A sign-in link store that seals before it writes and takes exactl
**Interfaces:**
- Produces: `(*App).flow` a `*signin.Flow`, and the four routes. Consumes `(*App).links` from Task 4.
-**`(*App).render` is Task 7's**, and these three templates are the first
-callers. Either land Task 7 first or build the minimal renderer here and let
-Task 7 extend it — but say which, because an earlier draft left three
-templates with nothing to render them.
+**`(*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
@@ -829,7 +895,11 @@ func TestFullSigninLeavesNoReadableAddress(t *testing.T) {
// Lowercase and address-shaped: the blind index and signin's rate
// limiter both key on strings.ToLower(address), so a canary that is
// only ever upper-case cannot detect a leak of the lowercased copy.
- signIn(t, app, "canary-"+sealtest.Canaries[0]+"@example.invalid")
+ // 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)
}
```
@@ -926,6 +996,18 @@ 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
@@ -966,8 +1048,8 @@ 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: `static/passkey.js` — the app's own glue
-- Modify: `internal/home/app.go`, `internal/home/db.go` (schema merge)
+- 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.
@@ -976,7 +1058,7 @@ 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.
-- A successful step-up mints `Method` as the first factor plus `"+passkey"`, with a fresh `AuthTime` — which is what `RequireFresh` checks.
+- `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
@@ -1003,6 +1085,9 @@ rediscovered.
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
@@ -1028,8 +1113,21 @@ POST /passkey/signin/begin | /finish
POST /passkey/signin/recovery
```
-All behind `csrf.Protect`. Set `ConfirmPath` (its default is
-`/passkey/confirm`).
+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
+```
+
+`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*
@@ -1037,9 +1135,15 @@ All behind `csrf.Protect`. Set `ConfirmPath` (its default is
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. Serve `.mjs` with an
-explicit `text/javascript` Content-Type or strict MIME checking blocks the
-module.
+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**
@@ -1056,7 +1160,7 @@ page to sit behind `sessions.RequireFresh`.
- [ ] **Step 7: Commit**
```bash
-git add internal/home/passkey.go internal/home/passkey_test.go internal/home/templates static/passkey.js internal/home/app.go internal/home/db.go
+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"
```
@@ -1067,7 +1171,7 @@ git commit -m "Passkeys as an optional second factor, gated on both sign-in path
**Files:**
- Create: `internal/home/render.go`, `internal/home/templates/*.html`
- Create: `internal/guardstest/rpid_test.go`
-- Create: `static/tokens.css`, `static/theme.css` — vendored from rastrillo
+- 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)`
@@ -1148,21 +1252,60 @@ any script at all on those screens. Constrain it:
exemption is the ceremony button rather than the screen.
```go
-func TestNoScriptTagInAnyTemplate(t *testing.T) {
+// 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.
+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))
- if bytes.Contains(bytes.ToLower(buf.Bytes()), []byte("<script")) {
- t.Errorf("%s renders a <script> tag; §11 says every screen works with scripts off", name)
+ srcs, allowed := scriptSrcs(t, buf.Bytes()) // parses, so an inline body has no src
+
+ if !allowed && 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.
+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)` and
+`passkeyConfig(t)` — all four 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.
+
- [ ] **Step 3: Commit**
```bash
-git add internal/home/render.go internal/home/templates static internal/guardstest/rpid_test.go
+git add internal/home/render.go internal/home/templates internal/home/static internal/guardstest/rpid_test.go
git commit -m "Scriptless sign-in screens, and the guard that the RP ID is a full host"
```
@@ -1173,7 +1316,7 @@ git commit -m "Scriptless sign-in screens, and the guard that the RP ID is a ful
| 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 | §3.1's "expensive KDF" vs ruling 3's `HMAC` is settled — task 2 on `skeleton` |
+| ~~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" |