amadan
Answer the second review: two gates that could not fail, three claims that were false
Review verdict was CHANGES NEEDED on five blocking findings. The two
worst were gates green on the thing they were supposed to guard.
## B-1: the expiry predicate had no test that could fail
PutLink sweeps expired rows immediately after inserting, so every
"expired link" case written through PutLink was deleted by its own call
before TakeLink ever saw it. Those subtests were testing "never
existed". The reviewer proved it: replacing `expires_at > ?` with
`? IS NOT NULL` passed the entire suite.
Expired rows now go in directly, bypassing PutLink, in all three places
that need one. TestTakeLinkRefusesAnExpiredLink also takes a live link
afterwards, so it cannot pass on a store that refuses everything. The
same mutant now fails with "an expired link was redeemed".
signin's contract is explicit that this is ours: "enforcing expiry is
entirely the implementer's obligation; Flow performs no expiry check of
its own."
## B-2: the sealed exemption was abusable, and its narrowness test could not fail
The static guard exempted the whole LINE when "sealed" appeared anywhere
on it, as a substring. Three real leaks walked through, each verified
against an actual migration file:
unsealed_address TEXT NOT NULL "sealed" is a substring
sealed_note BLOB, address TEXT NOT NULL one line, two columns
address TEXT NOT NULL DEFAULT 'sealed' the word in a default
And the test claiming the exemption was narrow only asserted that
"sealed_address" contains "sealed" — it never exercised the exemption.
The commit that added it said "both exemptions now have tests asserting
they stay narrow", which was not true of the sealed half.
The guard now tokenises each identifier and exempts only a whole
"sealed" part, so it covers sealed_address and nothing standing beside
it. All four cases above are caught, verified by writing each into a
real migration and watching the guard fail.
The two guards' word lists also disagreed — the static one had no bare
"mail", so sender_mail passed it and was caught only at runtime. They
match now, and a test asserts every forbidden word is actually matched.
Separate implementations are the point; disagreeing about what they
forbid is not independence, it is a hole.
## Three claims false at source
B-3: db.go said secure_delete meant a deleted row was not "in every WAL
frame Litestream has already shipped". It cannot recall a shipped frame,
and measurement shows the value stays readable in the local -wal until a
checkpoint. What it does is zero freed content in the pages it rewrites.
The comment now says that, and says the sealing is what covers the rest.
B-4: with DBPath now defaulted in Options, three comments still told an
operator to pass a --db flag the unit cannot pass. The check guards a
caller that builds its own Options, not the deployed boot path, and says
so.
B-5: "a zero HOME_SIGNIN_KEY fails every single sign-in" is false. Flow
reads Key only on the keymail path, so it breaks keymail sign-in and
leaves the magic link working. The refusal is still right on the
placeholder argument; the stated consequence was not — and a silently
half-broken sign-in is if anything worse.
## Nits
- The boot sweep the plan asked for now exists. A process that put a
link and restarted left that row forever.
- The concurrency test cannot run "against both pools": a DELETE goes to
the writer, and that pool is capped at one connection. Said in the
test rather than left looking like an omission.
- links.go had reworded the "adjacent columns" claim into "adjacent
tables" rather than dropping it; there is no teams table here.
- §3.1 does not "prefer" an expensive KDF, it states the index uses one.
Quoting it as a preference softened a contradiction into a nuance.
## The spec note is now actually filed
The last commit said "FILED FOR PAUL" and filed nothing. The spec's §3.1
now carries an OPEN note: ruling 3 says HMAC, §3.1 says expensive KDF,
the code implements ruling 3, and that paragraph is therefore currently
false about the code. The note argues both sides — the KDF case is
stronger than it looks, because sign-in is already rate-limited and
call 2 fires on grant change rather than per request — and leaves the
ruling to Paul, because changing either sentence changes a §1 row.
Also recorded in db.go, from the reviewer, as a hazard nobody has hit
yet: rastrillo's GORM logger prints slow queries with their parameters
interpolated. Nothing address-bearing goes through GORM today. The first
thing that does will put an address in platform logs.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01E4uVkB6xnZJYuxTooPiUn4
cmd/home/main.go+3 −2docs/superpowers/specs/2026-08-30-home-design.md+21 −0internal/guardstest/noaddress_test.go+106 −70internal/home/app.go+11 −0internal/home/db.go+20 −4internal/home/db_test.go+6 −1internal/home/keys.go+22 −11internal/home/keys_test.go+8 −4internal/home/links.go+13 −7internal/home/links_test.go+69 −8
Intent
<task-notification> <task-id>a445ee3dd022f1ebc</task-id> <tool-use-id>toolu_017Av2A9B4xfxjjZjzh2xqtn</tool-use-id> <output-file>/home/paulca/.cache/tmp/claude-1001/-home-paulca-amadan-net-oficina-home/d63f5ac7-6b42-48c5-a13d-34e6faf8e44c/tasks/a445ee3dd022f1ebc.output</output-file> <status>completed</status> <summary>Agent "Review tasks 2-4 and the fixes" 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>## Review of `53430d0..e41d43c` (`step2/seal-and-secrets`) Everything builds, vets, and passes (`go test ./...` and `go test -race ./internal/home/`). I ran five mutants in a scratch copy under `$TMPDIR`, booted the built binary against the platform's invocation shape, and checked every source claim against rastrillo v0.22.0, signin v0.1.1, modernc sqlite v1.55.0, and `carlos/platform` at `97eb53f`. The worktree is untouched. ### Part 1 — the six earlier findings | | Status | Evidence | |---|---|---| | B1/B2/B3 | **Fixed** | `run.go:87-92`: `-db` overwrites `DBPath` only when passed; relative path joined to `$STATE_DIRECTORY`. `carlos-app@.service`: `ExecStart=/opt/carlos/live/%i serve`, `StateDirectory=carlos-app/%i`, `Restart=always` — all as claimed. Reproduced: binary with `serve --addr`, no `--db`, `STATE_DIRECTORY` set → `/healthz` 200, `/` 200, `home.db` created under `$STATE_DIRECTORY`, nothing in cwd. Missing `HOME_PEPPER` → exit 1. `opts.DBPath = ""` before `Serve` is valid (`serve.go:341-352`, Mux + empty DBPath). Nothing on the platform path overrides the default. With `STATE_DIRECTORY` unset the file lands in cwd — reproduced; that's the dev-checkout case and can't happen under the unit. But see B-4: the fix leaves stale claims behind. | | B4 | **Fixed** | `keys.go:89-91`, one test per key. See B-5 for the comment. | | B5 | **Fixed** | `app.go:50-61`; `TestNewRefusesIncompleteWiring` covers zero value, each nil, nil db. | | B6 | **Fixed in `seal.go`, regressed in wording** at `links.go:21-22` (N-2). | ### Part 2 — findings **B-1 BLOCKING — `TakeLink`'s expiry predicate has no test that can fail.** `internal/home/links.go:104` `WHERE token_hash = ? AND purpose = ? AND expires_at > ?`. I replaced `expires_at > ?` with `? IS NOT NULL`; **every test passed.** Cause: `PutLink` sweeps after each insert (`links.go:89`), so the "expired" row in `TestTakeLinkIsIndistinguishableAcrossFailures` (`links_test.go:74-76`) is deleted by its own `PutLink` before `TakeLink` runs — the `expired` subtest at `:85` actually tests "never existed". Same for `TestSweepExpiredLinksRemovesOnlyExpiredOnes`. The signin contract (`magiclink.go:26-28`) says "enforcing expiry is entirely the implementer's obligation; Flow performs no expiry check of its own." Fix: insert an already-expired row directly (bypassing `PutLink`), or make `TakeLink` take a `now` and test with a future-dated row and a later clock. **B-2 BLOCKING — the static guard's `sealed` exemption is abusable, and the test that claims it is narrow cannot fail.** `internal/guardstest/noaddress_test.go:44` `if strings.Contains(strings.ToLower(code), "sealed") { continue }` — per line, substring. A migration with `unsealed_address TEXT NOT NULL`, with `CREATE TABLE x (id INTEGER PRIMARY KEY, sealed_note BLOB, address TEXT NOT NULL);` on one line, and with `address TEXT NOT NULL DEFAULT 'sealed'` **passed `TestNoAddressColumn`**; the runtime twin (`db_test.go:113`) caught all three. Commit 65bafa8: "Both exemptions … now have tests asserting they stay narrow" — the sealed half of `TestGuardExemptionsAreNarrow` (`:101-107`) asserts only that `"sealed_address"` contains `"sealed"` and that `"address TEXT,"` matches the regex; neither touches the exemption. Commit e41d43c: "so both exemptions are stated in both places", `db_test.go:104-105` "two independent checks that agree" — they do not: `db_test.go:112` promises `unsealed_address` "does not slip through", and the static guard lets it through. The word lists also differ: static has no bare `mail` (so `mail`, `sender_mail` pass static, caught at runtime); both miss `emails` and `addr`. Fix: tokenise the identifier as `splitIdent` does and exempt only a whole `sealed` token, then add the three cases above to the narrowness test. **B-3 BLOCKING — `db.go` overstates what `secure_delete` does.** `internal/home/db.go:19-22`: "without it, a redeemed magic link's sealed row is gone from the table and still present in the file, and in every WAL frame Litestream has already shipped." `secure_delete` cannot touch frames already shipped, and it does not clear the WAL on disk either: on this driver with the hook active (`PRAGMA secure_delete` = 1), a plaintext value inserted then deleted was **still present in `x.db-wal`** after the DELETE, gone only after `wal_checkpoint(TRUNCATE)`. What it does is zero freed content in the pages it rewrites, so the post-checkpoint main file and the new frame don't carry it. The spec's sentence (§5.1, "does not remove bytes from a file") is narrower and fine; the comment should say what the control does, not imply it recalls the WAL. (Default here is 0 with no hook, so the hook is not theatre — it just isn't what the comment says.) **B-4 BLOCKING — the B2 fix left the "no --db" story stale in three places.** `cmd/home/main.go:69-70` "no --db and SQLite opens a private temporary database that loses every person on restart" — with `DBPath: "home.db"` at `:57`, that refusal can no longer fire on this path. `internal/home/keys.go:104-106` "rastrillo.Resolve owns it (it comes from -db)", `:112-117` "Resolve sets DBPath only from -db … pass --db" — DBPath now comes from the Options default, and the error tells an operator to pass a flag the unit cannot pass. `keys_test.go:145-147` repeats it. Same defect class as B1-B3. **B-5 BLOCKING — "fails every single sign-in" is false at source.** `keys.go:85-87` "the app boots, answers /healthz, and fails every single sign-in"; `keys_test.go:157-159`; commit e41d43c "failed every single sign-in". `Flow.Key` is read only at `flow.go:163` (`beginKeymail`) and `flow.go:213` (`CompleteKeymail`); `sendLink` and `CompleteMagicLink` never touch it, and `HashToken` is plain SHA-256. A zero `HOME_SIGNIN_KEY` breaks the Keymail-classified path only; magic-link sign-in works. The refusal is still right on the placeholder argument alone — the stated consequence is not. **N-1 NIT — undisclosed plan deviations.** Task 4 step 5: sweep "at boot and on each PutLink" — no boot sweep exists. Step 1: "Run the concurrency one against both pools" — the test only reaches `Writer()`; `TakeLink` can only use the writer, so it cannot be done, but say so. e41d43c discloses only the sessions/passkey deferral. **N-2 NIT — `links.go:21-22`** "an address and a team name — adjacent columns of adjacent tables": there is no teams table in this schema, and "adjacent tables" is B6's sentence reworded, not removed. `seal.go`'s corrected wording (a store handed the wrong purpose) is the right one. **N-3 NIT — `links.go:48-52`** "§3.1 prefers 'an expensive KDF rather than a bare HMAC'": §3.1:336 says the index *uses* one — a statement, not a preference; the quote is softened. And "Filed for Paul" / e41d43c "FILED FOR PAUL": nothing in this range files anything (the spec is untouched; the plan diff is 3 lines). Plan Task 4 asked to "File the tension as a spec note." Point at the filing or make one. **N-4 NIT — heads-up, not in this range.** `PutLink`'s sweep log (`links.go:90`) is clean: it binds only a timestamp and SQLite error text carries SQL/column names, never bound values. But rastrillo `db.Open` installs a GORM logger that prints slow (>200 ms) queries **with interpolated parameters** at Warn (`db.go:84-88`). Nothing here goes through `d.G` with an address; the first thing that does will put it in platform logs. Worth a sentence in `db.go` now. **N-5 NIT —** 9209153 "Three database-level tests beyond the plan … : that both table sets exist … and that the database as actually built has no address column" lists two; the third is a matcher unit test. Trivial. ### Verified true (so nobody re-checks them) - `TakeLink` atomicity: `Writer()` is the raw writer `*sql.DB` capped at 1 (`db.go:59,119`); `DELETE … RETURNING` applies on first step, `Row.Scan`'s close commits in autocommit; second take proves it. **The race test is real**: a SELECT-then-DELETE mutant failed 10/10 runs with 2-10 winners. - Expiry boundaries: take `>` / sweep `<=` agree at the boundary second; RFC3339 truncation refuses at most <1 s early. No gap, no overlap. - `findOrMintPerson`: DO UPDATE + RETURNING yields the existing ref (`TestFindOrMintPersonIsStable`); a `person_ref` conflict (2^-128) surfaces as a UNIQUE error, which is correct; refs are random so nothing is "burned". - Normalisation: `Begin` hands `PutLink` `TrimSpace`d `local@lower(domain)` (`flow.go:128-131`, `signin.go:61-74`); `TakeLink` returns that string; `blindIndex` lowercases the whole thing. One row per person; Keymail's own check is `EqualFold`. Consistent. - Gate 1 flush: the unsealed mutant fails with exactly `plaintext "canary-8f3a@example.invalid" found in home.db-wal` — data is on disk in the WAL at scan time. - `sync.Once`: hooks append (`driver.go:201`); `sqlite.go:785` line number correct; secure_delete default 0 without the hook, 1 on writer and reader with it. - `models.go` sessions columns match `sessions/migrations/0001_init.sql`. Sheets' info string is `oficina/sheets:`+purpose; `vectors_test.go` is a genuine port plus additions. Containment guard rename correct. `.amadan/ci.d` runs five make targets and deploys nothing. Restart=always and the activator's wedged-unit text (`backend_unit.go:205-213`) are as quoted. ### Verdict **CHANGES NEEDED.** Two gates that cannot fail on the thing they guard (B-1, B-2) and three claims false at source (B-3, B-4, B-5). The code underneath is largely right; the fixes are a real expiry test, a token-based static exemption with a test that bites, and three comment rewrites.</result> <usage><subagent_tokens>170005</subagent_tokens><tool_uses>46</tool_uses><duration_ms>556519</duration_ms></usage> </task-notification>
diff --git a/cmd/home/main.go b/cmd/home/main.go| index 85d5e6b..2e0c982 100644 |
| --- a/cmd/home/main.go |
| +++ b/cmd/home/main.go |
| @@ -66,8 +66,9 @@ func main() { |
| // well and is broken or leaking: no DEK and addresses persist in the |
| // clear, no pepper and every person row is orphaned, no SMTP host |
| // and magic links are written to platform-held logs instead of |
| - // emailed, no --db and SQLite opens a private temporary database |
| - // that loses every person on restart. |
| + // emailed. The database path cannot be empty on this path — it is |
| + // set below — but LoadSecrets checks it anyway, for the caller that |
| + // builds its own Options. |
| secrets, err := home.LoadSecrets(opts.DBPath, logger) |
| if err != nil { |
| logger.Error("config", "err", err) |
diff --git a/docs/superpowers/specs/2026-08-30-home-design.md b/docs/superpowers/specs/2026-08-30-home-design.md| index 4883df7..7119ace 100644 |
| --- a/docs/superpowers/specs/2026-08-30-home-design.md |
| +++ b/docs/superpowers/specs/2026-08-30-home-design.md |
| @@ -338,6 +338,27 @@ bare HMAC, pricing the dictionary run rather than preventing it. It does not |
| make the second model safe; it makes it expensive. Said plainly rather than |
| counted as a fix. |
| +> **OPEN — for Paul, raised 2026-09-07.** The sentence above and **ruling 3** |
| +> contradict each other, and the code can only implement one. Ruling 3 says |
| +> `HMAC(pepper, lowercased address)`; §7's directory row and §7's rekey |
| +> reasoning say HMAC too. This paragraph is the only place an expensive KDF |
| +> appears. `internal/home/links.go`'s `blindIndex` implements **ruling 3**, |
| +> on the ground that a settled §1 row outranks a threat-model note — so as |
| +> written this paragraph is currently false about the code. |
| +> |
| +> It is a real tradeoff, not a typo. A dictionary run against a leaked |
| +> pepper is exactly the second threat model's attack, and an HMAC prices it |
| +> at nothing. Against that: the index is computed on the sign-in path and on |
| +> every call-2 directory upsert, so the cost is paid per request. That cost |
| +> is probably affordable — sign-in is already rate-limited at 20 per IP and |
| +> 5 per address per 15 minutes, and call 2 fires on grant change rather than |
| +> per request — which makes the case for the KDF stronger than it first |
| +> looks. |
| +> |
| +> Changing either sentence changes a §1 row, so it is Paul's ruling. Until |
| +> then the code follows ruling 3 and this note records the gap rather than |
| +> hiding it. |
| + |
| ### 3.2 What is at rest, precisely |
| **Sealed:** team display names; provisioning queue payloads; the rekey |
diff --git a/internal/guardstest/noaddress_test.go b/internal/guardstest/noaddress_test.go| index 0883a77..220738a 100644 |
| --- a/internal/guardstest/noaddress_test.go |
| +++ b/internal/guardstest/noaddress_test.go |
| @@ -6,57 +6,104 @@ import ( |
| "testing" |
| ) |
| -// addressish matches a column name that would hold an email address. |
| +// addressWords are the identifier parts that name an address. |
| // §10: "people — person ref, blind index, created_at. No address column." |
| // |
| -// The boundary is "not a letter or digit" rather than \b, because \b |
| -// counts _ as a word character and SQL column names are separated by it: |
| -// \bemail\b does not match owner_email, which is exactly the column |
| -// somebody would add. TestAddressishMatches caught that on the first |
| -// run, which is the whole reason that test exists. |
| +// It matches the runtime twin's list in internal/home/db_test.go on |
| +// purpose. An earlier version of the two disagreed — this one had no |
| +// bare "mail", so a column called sender_mail passed the static guard |
| +// and was caught only at runtime — and two guards that disagree about |
| +// what they forbid are one guard plus a surprise. |
| +var addressWords = map[string]bool{ |
| + "address": true, "addresses": true, "addr": true, |
| + "email": true, "emails": true, "mail": true, |
| + "recipient": true, "recipients": true, |
| +} |
| + |
| +// identifier finds the word-shaped runs in a line: letters, digits and |
| +// the separators SQL identifiers use. |
| +var identifier = regexp.MustCompile(`[A-Za-z0-9_-]+`) |
| + |
| +// namesAnAddress reports whether a line of SQL declares a column whose |
| +// name says it holds an address, and which word gave it away. |
| +// |
| +// It works per identifier, not per line, and that is the whole |
| +// correction. The first version matched a regex against the line and |
| +// exempted the ENTIRE line when the word "sealed" appeared anywhere on |
| +// it. Three real leaks walked through, all found by a reviewer running |
| +// them against a migration: |
| // |
| -// Known limit, stated rather than papered over: a name with no separator |
| -// at all — useraddress — is not matched. Widening to a bare substring |
| -// search would flag emailed_at and every comment mentioning mail, and a |
| -// guard that cries wolf gets a // nolint faster than it gets a fix. |
| -var addressish = regexp.MustCompile(`(?i)(?:^|[^a-z0-9])(address|email|e_mail|mail_to|recipient)(?:$|[^a-z0-9])`) |
| +// unsealed_address TEXT NOT NULL -- "sealed" is a substring |
| +// sealed_note BLOB, address TEXT NOT NULL -- one line, two columns |
| +// address TEXT NOT NULL DEFAULT 'sealed' -- the word in a default |
| +// |
| +// The exemption is now a whole "sealed" part of one identifier, so it |
| +// covers sealed_address and nothing standing next to it. |
| +// |
| +// Splitting on _ and - also fixes the earlier \b problem: \b counts _ as |
| +// a word character, so \bemail\b never matched owner_email, which is |
| +// exactly the column somebody would add. |
| +func namesAnAddress(line string) (string, bool) { |
| + // Match on the code, not the comment. A migration explaining why it |
| + // has no address column is not an address column, and the first one |
| + // written tripped this guard on its own prose. Nothing hides here: a |
| + // column definition inside a SQL comment is not a column. |
| + code, _, _ := strings.Cut(line, "--") |
| + for _, word := range identifier.FindAllString(code, -1) { |
| + parts := strings.FieldsFunc(strings.ToLower(word), func(r rune) bool { |
| + return r == '_' || r == '-' |
| + }) |
| + if slicesContains(parts, "sealed") { |
| + // A sealed blob is not an address, and says so in its name. |
| + continue |
| + } |
| + for _, p := range parts { |
| + if addressWords[p] { |
| + return word, true |
| + } |
| + } |
| + } |
| + return "", false |
| +} |
| + |
| +func slicesContains(parts []string, want string) bool { |
| + for _, p := range parts { |
| + if p == want { |
| + return true |
| + } |
| + } |
| + return false |
| +} |
| // 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 — needs a running sign-in and lands with it. |
| +// WAL included — lives with the sign-in it needs. |
| // |
| -// It is a tripwire, like gate 9: it passes trivially while there are no |
| +// It is a tripwire, like gate 9: it would pass trivially with no |
| // migrations, and that is correct. A guard written after the schema it |
| // guards is a guard written to pass. |
| func TestNoAddressColumn(t *testing.T) { |
| root := repoRoot(t) |
| - for _, path := range migrationFiles(t, root) { |
| + files := migrationFiles(t, root) |
| + if len(files) == 0 { |
| + t.Fatal("no migrations found: the guard would pass by not looking") |
| + } |
| + for _, path := range files { |
| rel := relPath(t, root, path) |
| for i, line := range strings.Split(readFile(t, path), "\n") { |
| - // Match on the code, not the comment. A migration explaining |
| - // why it has no address column is not an address column, and |
| - // the first one written tripped this guard on its own prose. |
| - // Nothing can hide here: a column definition inside a SQL |
| - // comment is not a column. |
| - code, _, _ := strings.Cut(line, "--") |
| - // A sealed blob is not an address, and says so in its name. |
| - if strings.Contains(strings.ToLower(code), "sealed") { |
| - continue |
| - } |
| - if addressish.MatchString(code) { |
| + if word, bad := namesAnAddress(line); bad { |
| t.Errorf("%s:%d: %q names an address column; §10 says there is none", |
| - rel, i+1, strings.TrimSpace(line)) |
| + rel, i+1, word) |
| } |
| } |
| } |
| } |
| -// TestAddressishMatches proves the matcher fires, and fires on the |
| -// shapes a schema would actually use. A tripwire that passes trivially |
| -// today is only worth having if it would still fire tomorrow, and this |
| -// repo has twice shipped a detector that could not detect. |
| -func TestAddressishMatches(t *testing.T) { |
| +// TestNamesAnAddress proves the matcher fires, and fires on the shapes a |
| +// schema would actually use. A tripwire that passes today is only worth |
| +// having if it would still fire tomorrow. |
| +func TestNamesAnAddress(t *testing.T) { |
| shouldMatch := []string{ |
| " address TEXT NOT NULL,", |
| " email TEXT,", |
| @@ -65,10 +112,17 @@ func TestAddressishMatches(t *testing.T) { |
| " recipient TEXT,", |
| " EMAIL TEXT,", |
| " owner_email TEXT,", |
| + " sender_mail TEXT,", |
| + " emails TEXT,", |
| + " addr TEXT,", |
| + // The three the line-level exemption used to let through. |
| + " unsealed_address TEXT NOT NULL,", |
| + " sealed_note BLOB, address TEXT NOT NULL,", |
| + " address TEXT NOT NULL DEFAULT 'sealed',", |
| } |
| for _, line := range shouldMatch { |
| - if !addressish.MatchString(line) { |
| - t.Errorf("addressish missed %q", line) |
| + if _, bad := namesAnAddress(line); !bad { |
| + t.Errorf("namesAnAddress missed %q", line) |
| } |
| } |
| @@ -78,51 +132,33 @@ func TestAddressishMatches(t *testing.T) { |
| " created_at TEXT NOT NULL", |
| "CREATE TABLE people (", |
| " host TEXT NOT NULL,", |
| - // "mailer" and "emailed" are words, not columns: the \b anchors |
| - // are what keep them out, and removing them would make every |
| - // comment mentioning mail a failure. |
| - " -- the mailer sends this", |
| + // The one real exemption: a sealed blob is not an address, and |
| + // says so in its own name. |
| + " sealed_address BLOB NOT NULL,", |
| + // Prose about addresses is not a column. |
| + "-- there is no address column here and that is the point", |
| + " person_ref TEXT PRIMARY KEY, -- not the email address", |
| + // "mailer" and "emailed" are words, not column names. |
| + " mailer_id TEXT,", |
| " emailed_at TEXT,", |
| - // Stated limit: no separator, so not matched. Here so the limit |
| - // is a test somebody can read, not a surprise in review. |
| - " useraddress TEXT,", |
| } |
| for _, line := range shouldNotMatch { |
| - if addressish.MatchString(line) { |
| - t.Errorf("addressish wrongly matched %q", line) |
| + if word, bad := namesAnAddress(line); bad { |
| + t.Errorf("namesAnAddress wrongly matched %q in %q", word, line) |
| } |
| } |
| } |
| -// The two ways past this guard are the "sealed" name and the comment |
| -// strip. Both are narrow, and both are stated here as tests rather than |
| -// as trust. |
| -func TestGuardExemptionsAreNarrow(t *testing.T) { |
| - // The sealed exemption is per line, and only when the line says so. |
| - if !strings.Contains(strings.ToLower(" sealed_address BLOB NOT NULL,"), "sealed") { |
| - t.Fatal("the exemption test itself is wrong") |
| - } |
| - if !addressish.MatchString(" address TEXT,") { |
| - t.Fatal("a plain address column must still match, exemption or not") |
| - } |
| - |
| - // The comment strip takes the comment, never the code beside it. |
| - for _, line := range []string{ |
| - " address TEXT, -- not sealed, not allowed", |
| - " owner_email TEXT NOT NULL, -- explained away in a comment", |
| - } { |
| - code, _, _ := strings.Cut(line, "--") |
| - if !addressish.MatchString(code) { |
| - t.Errorf("a column commented out of trouble still slipped through: %q", line) |
| +// Every word this guard forbids must actually be matched by it. They are |
| +// two pieces of one guard, and a word in the list that the matcher never |
| +// sees is a rule nobody enforces. |
| +func TestEveryForbiddenWordIsMatched(t *testing.T) { |
| + for word := range addressWords { |
| + if _, bad := namesAnAddress(" " + word + " TEXT,"); !bad { |
| + t.Errorf("addressWords has %q but namesAnAddress does not match it", word) |
| } |
| - } |
| - for _, line := range []string{ |
| - "-- there is no address column here and that is the point", |
| - " person_ref TEXT PRIMARY KEY, -- not the email address", |
| - } { |
| - code, _, _ := strings.Cut(line, "--") |
| - if addressish.MatchString(code) { |
| - t.Errorf("prose about addresses was read as a column: %q", line) |
| + if _, bad := namesAnAddress(" owner_" + word + " TEXT,"); !bad { |
| + t.Errorf("addressWords has %q but namesAnAddress does not match owner_%s", word, word) |
| } |
| } |
| } |
diff --git a/internal/home/app.go b/internal/home/app.go| index 802209c..bbcad6e 100644 |
| --- a/internal/home/app.go |
| +++ b/internal/home/app.go |
| @@ -12,6 +12,7 @@ |
| package home |
| import ( |
| + "context" |
| "errors" |
| "log/slog" |
| "net/http" |
| @@ -104,6 +105,16 @@ func New(secrets Secrets, d *db.DB, logger *slog.Logger) (*App, error) { |
| if a.passkeys, err = newPasskeys(a); err != nil { |
| return nil, err |
| } |
| + |
| + // Sweep at boot as well as on each put. A process that put a link |
| + // and then restarted before the next one leaves that row behind |
| + // forever otherwise, and this app is not expected to restart often. |
| + // Logged rather than fatal: expired sealed rows are untidy, not |
| + // unsafe, and refusing to boot over housekeeping is the wrong trade. |
| + if err := a.links.sweep(context.Background(), time.Now()); err != nil { |
| + logger.Warn("sweeping expired sign-in links at boot", "err", err) |
| + } |
| + |
| return a, nil |
| } |
diff --git a/internal/home/db.go b/internal/home/db.go| index 8d3ae18..cd88e3f 100644 |
| --- a/internal/home/db.go |
| +++ b/internal/home/db.go |
| @@ -16,10 +16,19 @@ var secureDeleteOnce sync.Once |
| // RegisterSecureDelete turns on PRAGMA secure_delete for every |
| // connection this process opens. |
| // |
| -// It matters because deleting a row does not remove its bytes from a |
| -// file that is continuously replicated (§5.1): without it, a redeemed |
| -// magic link's sealed row is gone from the table and still present in |
| -// the file, and in every WAL frame Litestream has already shipped. |
| +// It matters because deleting a row does not remove its bytes from the |
| +// file (§5.1). What the pragma actually does, stated precisely because |
| +// an earlier version of this comment overstated it: SQLite zeroes freed |
| +// content in the pages it rewrites, so the page that lands in the main |
| +// file after a checkpoint, and the frame written for that change, do not |
| +// carry the deleted bytes. |
| +// |
| +// What it does NOT do: recall anything. A frame Litestream has already |
| +// shipped is gone from our reach, and measurement confirms the deleted |
| +// value is still readable in the local -wal until a checkpoint. The |
| +// control narrows the window; it does not close it. That is why the |
| +// column is sealed as well — the bytes that do survive are bytes nobody |
| +// can read. |
| // |
| // Three things make this the shape it is, and each was a way to get it |
| // wrong: |
| @@ -38,6 +47,13 @@ var secureDeleteOnce sync.Once |
| // 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. |
| +// A neighbouring hazard, recorded here because this is where somebody |
| +// will look: rastrillo/db.Open installs a GORM logger that prints slow |
| +// queries — over 200ms — WITH THEIR PARAMETERS INTERPOLATED, at Warn. |
| +// Nothing in this app sends an address through d.G today; everything |
| +// address-bearing goes through Writer() with bound parameters. The first |
| +// query that does go through GORM with an address in it will put that |
| +// address into platform-held logs on any slow night. |
| func RegisterSecureDelete() { |
| secureDeleteOnce.Do(func() { |
| sqlite.RegisterConnectionHook(func(conn sqlite.ExecQuerierContext, _ string) error { |
diff --git a/internal/home/db_test.go b/internal/home/db_test.go| index fc95e77..fd4d161 100644 |
| --- a/internal/home/db_test.go |
| +++ b/internal/home/db_test.go |
| @@ -119,7 +119,12 @@ func addressishColumn(col string) bool { |
| } |
| for _, part := range parts { |
| switch part { |
| - case "address", "email", "mail", "recipient": |
| + // The same list as internal/guardstest's addressWords. They are |
| + // separate implementations on purpose, but disagreeing about |
| + // WHAT they forbid is not independence, it is a hole — and they |
| + // did disagree: the static one had no bare "mail", so |
| + // sender_mail passed it and was caught only here. |
| + case "address", "addresses", "addr", "email", "emails", "mail", "recipient", "recipients": |
| return true |
| } |
| } |
diff --git a/internal/home/keys.go b/internal/home/keys.go| index 232ed1c..e810963 100644 |
| --- a/internal/home/keys.go |
| +++ b/internal/home/keys.go |
| @@ -82,10 +82,15 @@ func key32(name string) ([32]byte, error) { |
| // An all-zero key is the canonical placeholder — the value a |
| // generated config, a truncated copy-paste or a `head -c 32 |
| // /dev/zero` produces — and it is well-formed, so every other check |
| - // here passes it. signin.Flow refuses a zero Key with ErrNoKey, but |
| - // per request rather than at construction: the app boots, answers |
| - // /healthz, and fails every single sign-in. This loader is the only |
| - // place that refusal can happen at boot. |
| + // here passes it. |
| + // |
| + // The consequence, stated accurately: signin.Flow reads Key only on |
| + // the keymail path (beginKeymail and CompleteKeymail), so a zero |
| + // HOME_SIGNIN_KEY breaks sign-in for addresses at a keymail domain |
| + // and leaves the magic link working. Not "every sign-in", which an |
| + // earlier version of this comment claimed. A silently half-broken |
| + // sign-in is if anything a worse failure than a wholly broken one, |
| + // and this loader is the only place the refusal can happen at boot. |
| if subtle.ConstantTimeCompare(b, make([]byte, 32)) == 1 { |
| return out, fmt.Errorf("%s is 32 zero bytes, which is a placeholder rather than a key", name) |
| } |
| @@ -102,19 +107,25 @@ func key32(name string) ([32]byte, error) { |
| // CARLOS_SMTP_PASS the mailer's credential |
| // |
| // dbPath is passed in rather than read from the environment because |
| -// rastrillo.Resolve owns it (it comes from -db), and it is checked here |
| -// so that every boot refusal lives in one function an operator can read. |
| +// rastrillo.Resolve owns it, and it is checked here so that every boot |
| +// refusal lives in one function an operator can read. |
| +// |
| +// On the platform it is never empty: cmd/home sets Options.DBPath to a |
| +// relative "home.db", which Resolve joins to $STATE_DIRECTORY, and a |
| +// -db flag overrides it only if one is passed. This check therefore |
| +// guards a caller that constructs its own Options without one — a test, |
| +// a future command — rather than the deployed boot path. |
| func LoadSecrets(dbPath string, logger *slog.Logger) (Secrets, error) { |
| if logger == nil { |
| logger = slog.Default() |
| } |
| - // Resolve sets DBPath 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. There is no warning for this |
| - // anywhere below us, which is why it is checked here. |
| + // SQLite treats an empty filename as a private temporary database: |
| + // the app comes up, works, and loses every person on restart. There |
| + // is no warning for this anywhere below us, which is why it is |
| + // checked here. |
| if strings.TrimSpace(dbPath) == "" { |
| - return Secrets{}, errors.New("no database path: pass --db, or SQLite opens a private temporary database and every person is lost on restart") |
| + return Secrets{}, errors.New("no database path: set Options.DBPath or pass -db, or SQLite opens a private temporary database and every person is lost on restart") |
| } |
| var s Secrets |
diff --git a/internal/home/keys_test.go b/internal/home/keys_test.go| index c66f33d..3d0a6b5 100644 |
| --- a/internal/home/keys_test.go |
| +++ b/internal/home/keys_test.go |
| @@ -142,9 +142,11 @@ func TestLoadSecretsRefusesWithoutSMTPHostOrFrom(t *testing.T) { |
| } |
| } |
| -// Resolve sets DBPath 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. |
| +// SQLite treats an empty filename as a private temporary database: the |
| +// app comes up, works, and loses every person on restart. On the |
| +// platform the path is never empty — cmd/home sets a relative default |
| +// that Resolve joins to $STATE_DIRECTORY — so this guards a caller that |
| +// builds its own Options, not the deployed boot path. |
| func TestLoadSecretsRefusesEmptyDBPath(t *testing.T) { |
| setSecretEnv(t, nil) |
| if _, err := LoadSecrets("", quietLogger()); err == nil { |
| @@ -156,7 +158,9 @@ func TestLoadSecretsRefusesEmptyDBPath(t *testing.T) { |
| // other check passes it. It is also the canonical placeholder, and |
| // signin.Flow refuses a zero Key per request rather than at |
| // construction — so without this the app boots, answers /healthz, and |
| -// fails every single sign-in. |
| +// breaks sign-in for keymail addresses while leaving the magic link |
| +// working, since Flow reads Key only on the keymail path. A silently |
| +// half-broken sign-in is if anything worse than a wholly broken one. |
| func TestLoadSecretsRefusesAllZeroKeys(t *testing.T) { |
| zero := base64.StdEncoding.EncodeToString(make([]byte, 32)) |
| for _, name := range []string{"HOME_DEK", "HOME_PEPPER", "HOME_SIGNIN_KEY"} { |
diff --git a/internal/home/links.go b/internal/home/links.go| index 22e2109..b2ec1fc 100644 |
| --- a/internal/home/links.go |
| +++ b/internal/home/links.go |
| @@ -18,8 +18,10 @@ import ( |
| // sealPurposeAddress is the HKDF purpose a link's address is sealed |
| // under. A value sealed under one purpose cannot be opened under |
| -// another, which is what keeps an address and a team name — adjacent |
| -// columns of adjacent tables — from ever being swapped unnoticed. |
| +// another, so what this catches is a store handed the wrong purpose — |
| +// invisible to the type system, since every sealed value is just opaque |
| +// bytes, and invisible to any test that round-trips through the purpose |
| +// it wrote. |
| const sealPurposeAddress = "signin.address" |
| // linkTimeFormat pins how expires_at is written AND compared. |
| @@ -45,11 +47,15 @@ const dayFormat = "2006-01-02" |
| // would turn a single instance compromise into an offline dictionary |
| // attack against the whole suite directory. |
| // |
| -// A tension worth stating rather than hiding: §3.1 prefers "an expensive |
| -// KDF rather than a bare HMAC", pricing a dictionary run against a |
| -// leaked pepper. That is a mitigation note in the threat-model section; |
| -// ruling 3, in §1's settled table, says HMAC, and a settled ruling wins |
| -// over a note. Filed for Paul as a spec question, not resolved here. |
| +// A tension worth stating rather than hiding. §3.1 does not express a |
| +// preference, it makes a claim: "the index uses an expensive KDF rather |
| +// than a bare HMAC, pricing the dictionary run rather than preventing |
| +// it." Ruling 3, in §1's settled table, says HMAC. Both cannot be true |
| +// of this function, and it implements the ruling, so §3.1's sentence is |
| +// currently false about the code. |
| +// |
| +// Filed as an open question in the spec's §3.1 — see the note there — |
| +// because changing either one is a §1 row, which is Paul's to rule. |
| func blindIndex(pepper []byte, address string) string { |
| m := hmac.New(sha256.New, pepper) |
| m.Write([]byte(strings.ToLower(address))) |
diff --git a/internal/home/links_test.go b/internal/home/links_test.go| index 3dd64b0..5cd4932 100644 |
| --- a/internal/home/links_test.go |
| +++ b/internal/home/links_test.go |
| @@ -13,6 +13,59 @@ import ( |
| "amadan.net/oficina/home/internal/home/sealtest" |
| ) |
| +// putRawLink inserts a link row directly, bypassing PutLink. |
| +// |
| +// This exists because PutLink sweeps expired rows immediately after |
| +// inserting, so a link written through it with a past expiry is deleted |
| +// by its own call before TakeLink ever sees it. Every "expired" case |
| +// written through PutLink was therefore testing "never existed", and the |
| +// expiry predicate had no test that could fail — replacing |
| +// `expires_at > ?` with `? IS NOT NULL` passed the whole suite. |
| +func putRawLink(t *testing.T, app *testApp, hash, address, purpose string, expires time.Time) { |
| + t.Helper() |
| + sealed, err := app.secrets.Sealer.Seal(sealPurposeAddress, []byte(address)) |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + if _, err := app.db.Writer().Exec( |
| + `INSERT INTO signin_links (token_hash, sealed_address, purpose, expires_at) |
| + VALUES (?, ?, ?, ?)`, |
| + hash, sealed, purpose, expires.UTC().Format(linkTimeFormat)); err != nil { |
| + t.Fatal(err) |
| + } |
| +} |
| + |
| +// TakeLink must refuse a link past its expiry. signin's own contract is |
| +// explicit that this is the store's job: "enforcing expiry is entirely |
| +// the implementer's obligation; Flow performs no expiry check of its own |
| +// on the value this returns." |
| +func TestTakeLinkRefusesAnExpiredLink(t *testing.T) { |
| + app := newTestApp(t) |
| + ctx := context.Background() |
| + |
| + putRawLink(t, app, "expired", "person@example.invalid", signin.PurposeSignin, |
| + time.Now().Add(-time.Minute)) |
| + |
| + addr, ok, err := app.links.TakeLink(ctx, "expired", signin.PurposeSignin) |
| + if err != nil { |
| + t.Fatalf("TakeLink: %v", err) |
| + } |
| + if ok { |
| + t.Fatal("an expired link was redeemed") |
| + } |
| + if addr != "" { |
| + t.Fatalf("TakeLink returned %q for an expired link", addr) |
| + } |
| + |
| + // And a link that has NOT expired is still taken, or the test above |
| + // would pass on a store that refuses everything. |
| + putRawLink(t, app, "live", "person@example.invalid", signin.PurposeSignin, |
| + time.Now().Add(time.Minute)) |
| + if _, ok, err := app.links.TakeLink(ctx, "live", signin.PurposeSignin); err != nil || !ok { |
| + t.Fatalf("a live link was refused (ok=%v err=%v)", ok, err) |
| + } |
| +} |
| + |
| // errWrongAddress is reported from a racing goroutine, where t.Errorf |
| // would be called from a non-test goroutine. |
| var errWrongAddress = errors.New("TakeLink returned the wrong address") |
| @@ -71,10 +124,11 @@ func TestTakeLinkIsIndistinguishableAcrossFailures(t *testing.T) { |
| app := newTestApp(t) |
| ctx := context.Background() |
| - if err := app.links.PutLink(ctx, "expired", "person@example.invalid", |
| - signin.PurposeSignin, time.Now().Add(-time.Minute)); err != nil { |
| - t.Fatalf("PutLink: %v", err) |
| - } |
| + // Direct, not through PutLink: PutLink's own sweep would delete this |
| + // row before TakeLink ran, and the case would silently become |
| + // "never existed". |
| + putRawLink(t, app, "expired", "person@example.invalid", signin.PurposeSignin, |
| + time.Now().Add(-time.Minute)) |
| if err := app.links.PutLink(ctx, "wrong-purpose", "person@example.invalid", |
| "something-else", time.Now().Add(15*time.Minute)); err != nil { |
| t.Fatalf("PutLink: %v", err) |
| @@ -103,6 +157,12 @@ func TestTakeLinkIsIndistinguishableAcrossFailures(t *testing.T) { |
| // Two clicks on the same link, at the same moment, must mint one |
| // session and not two. That is a property of the single DELETE … |
| // RETURNING statement, so it is asserted rather than assumed. |
| +// |
| +// The plan asks for this "against both pools". It cannot be: a DELETE |
| +// goes to the writer, and rastrillo's writer pool is capped at one |
| +// connection, so there is no second pool for this statement to reach. |
| +// Said here rather than left looking like an omission. What the test |
| +// does exercise is real — a SELECT-then-DELETE implementation loses it. |
| func TestConcurrentTakeLinkYieldsOneWinner(t *testing.T) { |
| app := newTestApp(t) |
| ctx := context.Background() |
| @@ -156,10 +216,11 @@ func TestSweepExpiredLinksRemovesOnlyExpiredOnes(t *testing.T) { |
| app := newTestApp(t) |
| ctx := context.Background() |
| - if err := app.links.PutLink(ctx, "old", "a@example.invalid", |
| - signin.PurposeSignin, time.Now().Add(-time.Hour)); err != nil { |
| - t.Fatal(err) |
| - } |
| + // The expired row goes in directly, for the same reason: written |
| + // through PutLink it would be swept by its own call, and the sweep |
| + // under test would have nothing left to remove. |
| + putRawLink(t, app, "old", "a@example.invalid", signin.PurposeSignin, |
| + time.Now().Add(-time.Hour)) |
| if err := app.links.PutLink(ctx, "live", "b@example.invalid", |
| signin.PurposeSignin, time.Now().Add(time.Hour)); err != nil { |
| t.Fatal(err) |