oficina / home Public

Review pass: two more ways to pass a line that leaks, and a route never driven

The previous pass closed three bypasses and claimed the set. Fable's
review found two more of the same shape and one hole in what the gate
drives at all. Every one fails in the same direction: readable in
production, unreadable to the scanner, so the gate goes green on a line
that leaks.

render asserted the []byte TYPE, which does not match a named byte
slice. json.RawMessage is the likeliest type a queue payload actually
has, and it read as "[67 65 78 ...]" here while slog's TextHandler
printed it in the clear. slog's own byteSlice check is a reflect kind
check for exactly this reason, so this one is now too.

A value implementing encoding.TextMarshaler leaked under both stock
handlers while the scanner saw its String(). TextHandler prefers
MarshalText; fmt — and so slog.Value.String — prefers Stringer. A
person type with a redacting String and a plain MarshalText is
therefore a leak that reads clean. json.Marshaler is the same trick
under JSONHandler. render now returns the union of every form any
stock handler could emit rather than picking one, because picking one
is what failed twice.

WithGroup discarded the group name. Values under a group were always
scanned; the name never was, and it is caller-supplied —
WithGroup(team.Slug) is a plausible line to write. Inline group keys
had the same gap.

The router test requested chi's route patterns literally, so
chi.URLParam returned "{team}" and a handler logging a team slug from
the path passed. Parameters are now filled with a canary. Routes are
also driven with the method they were DECLARED with: a route
registered only as DELETE answered 405 to the GET and POST this sent,
so its handler never ran — contradicting this test's own claim that a
route added in step 2 is covered without anyone remembering it.

The sink guard compared the identifier to the literal "os", so
`import goos "os"; goos.Stderr` walked past it, and a local variable
named log would have fired it. It now resolves identifiers through the
file's imports and keys on the import path, and refuses a dot-import
of a forbidden package outright, since a dot-import leaves no selector
to match.

Every new test was checked against the unfixed code first: all six in
logtest fail without the change, and the alias guard was proven by
planting an aliased sink and watching it fail.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Paul Campbell pushed by paul@keymail.dev 6f4945e54e60f84ff9b5a0a550241d27084e10c7 parent 1caabab
4 files changed, +275 −52
  • internal/guardstest/logsinks_test.go +45 −6
  • internal/home/logs_test.go +62 −25
  • internal/home/logtest/logtest.go +86 −21
  • internal/home/logtest/logtest_test.go +82 −0

CI passed — run details

Intent

Human

OK, let's stand up a continuously reviewd project that keeps going until home is live. It should offer single-signon against all the apps in the suite, and it should link out to all the apps. We can add features as we go, but at the very least it would have a springboard-style dashboard for me to launch into any one of the apps.

Human

Create `main` and stand it up as it should be. 1. Is home just a list of teams? Then do we need a separate springboard app? I'm confused! Maybe we do. Or would there be a second level in this app which is -> [account switcher] -> apps and activity ... maybe that's it?

Human

<task-notification> <task-id>abe5e8a6e1b4c3727</task-id> <tool-use-id>toolu_0143Scc9sb8TLPUFbXP6hCZY</tool-use-id> <output-file>/home/paulca/.cache/tmp/claude-1001/-home-paulca-amadan-net-oficina-home/d63f5ac7-6b42-48c5-a13d-34e6faf8e44c/tasks/abe5e8a6e1b4c3727.output</output-file> <status>completed</status> <summary>Agent "Fable review: gate 9 logs" 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 structure is right — separated detection primitive, mutation-checked tests, AST guard on non-slog sinks, honest doc comment about scope — and `make test` is green under `CGO_ENABLED=0` (nothing here touches cgo). But I found two ways a leaking line passes the scanner and one way a leaking handler is never driven. All three were confirmed by running code against Go 1.26.5 and chi v5.3.2, not by reading. The first is the "fourth way" you asked me to look for, and it is the same bug the review pass claimed to close, one type away. Required changes: 1. `internal/home/logtest/logtest.go:224-231` — `render` catches `[]byte` but not a *named* byte-slice type. `v.Any().([]byte)` fails for `json.RawMessage` or `type Payload []byte`, so it falls through to `v.String()` and the scanner sees `[67 65 78 ...]`, while slog's TextHandler (via its reflect-based `byteSlice`) prints the payload readable. Probe output, canary `CANARY-QUEUE-PAYLOAD-93bd` logged as `json.RawMessage`: - production TextHandler: `raw="CANARY-QUEUE-PAYLOAD-93bd"` - branch scanner: `raw=[67 65 78 65 82 89 45 ...]` `json.RawMessage` is the single most likely type for a queue payload to have. Fix by mirroring slog's own check (`reflect.TypeOf(a).Kind() == reflect.Slice &amp;&amp; Elem().Kind() == reflect.Uint8`), and add a `TestFoundCatchesANamedByteSlicePayload` using `json.RawMessage` in `logtest_test.go`, mutation-checked as the others were. 2. `internal/home/logtest/logtest.go:224-231` — a value implementing `encoding.TextMarshaler` leaks under *both* stock handlers while the scanner sees its `String()`. TextHandler prefers `MarshalText` over `Stringer`; JSONHandler goes through `json.Marshal`, which also honours `TextMarshaler`. `Value.String()` goes through `fmt`, which prefers `Stringer`. A person type with `String() = "person#42"` and `MarshalText() = address` printed `who=CANARY-ADDRESS-a41f@example.invalid` from both production handlers and `who=person#42` from the scanner. This is the LogValuer bug (already fixed) in a different interface. Recommended fix for both 1 and 2: make `render` return the *union* of what any production handler could print — `String()`, plus `MarshalText()` if `TextMarshaler`, plus `MarshalJSON()` if `json.Marshaler`, plus the bytes if byte-slice-kinded — and say so in the comment. Add a test with a redacting `String()` so it fails without the change, as `valuer` does. 3. `internal/home/logs_test.go:66-102` — route params are never driven with a canary, and only GET/POST are sent. `chi.Walk` yields the pattern `/t/{team}` and the test requests that string literally, so `chi.URLParam(r, "team")` is `"{team}"`; a handler that logs the team slug from the path passes the gate. A route registered only as DELETE/PUT/PATCH gets 405 and its handler never runs. Probe output with a planted `GET /t/{team}` handler logging the param and a `DELETE /t/{team}`: ``` handler logged team="{team}" GET /t/{team} -&gt; 200 POST /t/{team} -&gt; 405 ``` (the DELETE handler was never reached). This directly contradicts the doc comment at lines 29-31 ("a route added in step 2 is driven by this test without anyone remembering to add it"), and a team slug in a path is the most plausible shape for the directory routes. Fix: keep the `method` from `chi.Walk` and drive each route with its declared method (form body for non-GET) in addition to GET/POST; substitute every `{...}` segment with `url.PathEscape(logtest.Canaries[1])` (regex-constrained params like `{id:[0-9]+}` will then 404 — say so in a comment rather than let it be silent). Recommended, not blocking: - `internal/guardstest/logsinks_test.go:72-76` compares `pkg.Name` to the literal `"os"`/`"log"`, so `import goos "os"; goos.Stderr` (and a dot-import) sidesteps the rule, and a local variable named `log` could false-positive. Resolve the identifier through the file's import specs; `fileImports` already parses them. - `internal/home/logtest/logtest.go:192,200-207` drops group keys entirely (`WithGroup` ignores the name; `appendAttr` recurses without writing `a.Key` for groups). Writing the key costs nothing and closes "team slug as group name". - Nothing waits for handlers that log from a goroutine after returning; not an issue for step 2 unless something logs asynchronously, but worth one line in the "what has to stay true" list. What I checked and found sound: the three claimed fixes from `1caabab` are real — `[]byte` (plain) is rendered readable, percent-encoded and case-folded canaries are found, and `slog.SetDefault`'s `log.SetOutput`/`SetFlags(0)` side effect is restored in cleanup (verified against `log/slog.SetDefault` source; the `defaultHandler` skip is exactly as the comment says). `Enabled` is unconditionally true, `With`-bound attrs and groups are walked, `LogValuer` is resolved. The JSONHandler error-text correction is accurate. The chi `middleware.Logger`/`Recoverer` claims are accurate for v5.3.2. `log.Print*` calls would in any case be captured in-test because the recorder is installed as slog default. The guards run under `go test ./...` (package with only `_test.go` files, confirmed with `-v`), walk from `runtime.Caller` not cwd, and skip `.claude/worktrees`. The "rastrillo resolves `slog.Default()` at construction" claim is true (`serve.go:288-290`, `sidecar.go:56-58`). Comment style, `§` citations and the AGENTS.md-three-sinks/spec-four-sinks attribution all match the repo's conventions.</result> <usage><subagent_tokens>79650</subagent_tokens><tool_uses>14</tool_uses><duration_ms>218890</duration_ms></usage> </task-notification>

Human

<task-notification> <task-id>a6e4052fe1da1a275</task-id> <tool-use-id>toolu_01Pyy1rcgjF2T6N4yCXEbCbW</tool-use-id> <output-file>/home/paulca/.cache/tmp/claude-1001/-home-paulca-amadan-net-oficina-home/d63f5ac7-6b42-48c5-a13d-34e6faf8e44c/tasks/a6e4052fe1da1a275.output</output-file> <status>completed</status> <summary>Agent "Fable review: AGENTS.md spec sync" 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>I have everything I need; nothing further to fetch. Two more facts fell out of that last pass: `auth.Config.Subject` is the magic-link *email subject line*, not a subject-remapping hook, so the hand-roll rule still holds at v0.22.0 (though `auth_sessions` is no longer written at that version); and `rastrillo doctor` landed in **v0.21.0** — the first version this repo ever pinned. **VERDICT: CHANGES NEEDED** The four retractions the branch set out to carry are all present and all trace to the spec verbatim (list at the end). What blocks the merge is that the fourth commit (46ea045) introduces a new false reason of exactly the class it says it exists to remove, misses the rotted claim directly beneath the one it fixed, and leaves AGENTS.md disagreeing with spec §11 on a premise that does not hold. **Required changes** 1. **The Frontend bullet's stated cause is false.** Text: *"**Re-check this when the pin moves** — it is a claim about a dependency, and bumping the dependency is what falsified the last one."* And the commit message: *"commit 0d0262a moved the pin to v0.22.0 without re-checking."* Verified across every cached rastrillo version: `ui/tokens.css` has 0 distinct bare `[rst-*]` attributes and exactly 528 `.rst-` class occurrences through v0.20.x (so the spec's 2026-08-30 "zero bare / ~528" was *correct when written*); v0.21.0 introduced the twins (130 distinct, "TWO SPELLINGS" header present); v0.22.0 is also 130. This repo's first `go.mod` (ef8a30f, 2026-08-31) already pinned v0.21.0. The bump in 0d0262a changed the count from 130 to 130 and falsified nothing. What falsified the claim was rastrillo v0.21.0 landing between the spec's verification and the skeleton's first pin. The rule ("re-check on every pin move") is right; its evidence sentence is wrong, and the branch's own thesis is that a false reason in a binding file is worse than none. Rewrite the reason to match the record (e.g. the claim was verified against a version this repo never pinned). 2. **The next bullet is a rotted dependency claim the same commit failed to re-check.** Text: *"Adopt `rastrillo doctor` the day it lands and delete this step."* `rastrillo doctor` exists at v0.21.0 and v0.22.0 (`cmd/rastrillo/doctor.go`; help: "compare the app's vendored static/ files with this binary's", with `--fix`). The trigger fired at the very pin 46ea045 examined, one line below the sentence it rewrote. Also: there is **no `static/tokens.css` in the tree** (`internal/home/page.go` says no stylesheet is linked, deliberately), so "re-copy it on every upgrade" is an instruction with no object. Restate as: when the app vendors `static/`, run `rastrillo doctor --fix` on each upgrade (and note `rastrillo new` scaffolds `TestVendoredAssetsMatchTheLibrary` as the standing gate, per doctor.go's own doc). 3. **AGENTS.md now disagrees with spec §11, and the reason given for not fixing it is untrue.** Spec §11: *"`ui/tokens.css` has **zero bare `rst-*` attribute selectors**… `rastrillo doctor`… remain in zero files."* AGENTS.md says 130. The preamble rule is "fix it in the same commit." Commit 46ea045 defers because *"that file has uncommitted changes in the main checkout and is not mine to edit."* I checked: `git status` for `docs/` is clean and the main checkout's spec is byte-identical to `main`. The branch is docs-only already; fix §11 here (both the selector count and the doctor claim), or the branch leaves the file it was cut to reconcile still in disagreement. 4. **Ruling 16 / §4's test-backed RP-ID rule is not carried.** Spec §4: *"**Never widen the RP ID to a registrable suffix**… `oficina.works` as an RP ID is valid at every host in the zone… which collapses the split this very section exists to make. **The RP ID is the full host**, asserted by a test."* Ruling 16 was on `main` (462df84) before this branch, and the Identity section already carries the parallel origin-separation constraint "with its own test". An agent configuring `passkey.Config` with a suffix RP ID would be accepted by WebAuthn and defeat ruling 2 silently — the exact "reads like an improvement" case this file exists to block. Add one bullet; the spec supplies the wording. **Non-blocking, but fix while here** 5. *"gate 9 does not, and is not there yet"* is a status line with no basis in the spec, and `origin/feat/gate-9-no-content-in-logs` already exists; it will be false the day that merges. State the rule (§13 step 1 puts gate 9 in the skeleton; it needs no database) rather than the current state. 6. Data bullet: *"stores readable addresses in `auth_links` and `auth_sessions`"* — at v0.22.0 `auth/store.go` says `auth_sessions` "is no longer written to" (sessions moved to a shared table). The substantive claim survives (`admit()` still sets `Subject: id.Address`, `auth_links.address TEXT NOT NULL`, and `Config.Subject` is just the email subject line), but the table detail is stale at the pin. 7. The quoted header's *"(design spec §6-v3)"* is rastrillo's design spec; in a file whose own §6 is "The directory" it should say so. 8. **Spec-internal drift** (flag for the spec's owner): §15's last bullet *"A real suite domain — triggers §5.7"* is dead after ruling 16 — §5.7 is titled "not needed for the `oficina.works` move"; §12 gate 9 scans "log lines, error strings and metric labels" while §3.4's rule (which AGENTS.md now correctly carries) adds "job status message"; §11's tokens.css and doctor claims per item 3. 9. Pre-existing claims with no spec basis but no conflict, left alone deliberately: `CGO_ENABLED=0`/cgo-free SQLite, "a promote is not a deploy", the Review and Shipping-cadence rulings (process, attributed to Paul, outside the spec's scope), Authorship. Consider adding §2.2's note that ruling 15 (per-team 2FA, §14) is the worked example of "never hold an authorization model". 10. Squash note: b19f0fa's trailer *"Reviewed-By: Fable (dispatched; found the divergence)"* predates any review; the squash message should not inherit it. The commit claim "a main this remote does not have" is also stale — `refs/heads/main` is on origin at 462df84. **Overstatement check**: nothing in AGENTS.md asserts §2.3's divergence as settled, builds anything from §14, or closes a §15 item; "we hand-roll until [rastrillo #2] lands" correctly keeps that open. **Claims traced and confirmed against the spec** (verbatim or equivalent): the one rule and falsifiable form (§0); "readable" retraction with §3.2 sealing; "some person" via team-set fingerprint plus one outside fact (§3.3); gates 1, 2, 9 wording and §13 step 1 grouping; gate 9 needs no database (§12.9 is a string scan; `cmd/home/main.go:44`, `:57` already emit log lines; skeleton sets no `DBPath`); the four never-dos (§2.2) and the §6 member/guest quotation; credential origin never renders account-authored strings, with a test (§4, gate 5); the two-sided boundary test, theme/role/"which documents" examples, the "behave identically" retraction and CSS-injection point, identity exempt by construction (§5.4); 120-second expiry, cold-wake reason, `jti` stops replay (§5.3); audience binding (§5.3); slowest-consumer prune rule (§9.1); no address column, sealed-before-written, `secure_delete`, Litestream WAL (§3.2, §5.1, §10); opaque Subject and rastrillo #2 (§5.1, ruling 14 — re-verified at v0.22.0); home hashes, no pepper on instances (ruling 4, §6); additive migrations (§10); log rule including job status messages (§3.4); zero JS (§11); `X-Carlos-Version` after the watch (§12); the home does not hibernate (§2.1); restore drill before launch (§10).</result> <usage><subagent_tokens>95134</subagent_tokens><tool_uses>15</tool_uses><duration_ms>272321</duration_ms></usage> </task-notification>

Human

<task-notification> <task-id>a8a6f5afa932b8c04</task-id> <tool-use-id>toolu_01Vm6ZrpKuZgyRLWZCnKHUC8</tool-use-id> <output-file>/home/paulca/.cache/tmp/claude-1001/-home-paulca-amadan-net-oficina-home/d63f5ac7-6b42-48c5-a13d-34e6faf8e44c/tasks/a8a6f5afa932b8c04.output</output-file> <status>completed</status> <summary>Agent "Fable review: step 2 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>I have everything needed. Nothing further to fetch. **VERDICT: CHANGES NEEDED** The plan gets the big shape right — no address column, Subject is a minted person ref, `rastrillo/auth` and its `auth_links`/`auth_sessions` tables are excluded, the address is sealed before the insert, gate 1 goes in with the mechanism and is made to fail on purpose. But four of its load-bearing source claims are false at v0.22.0, one step produces a sign-in that cannot recognise a returning person, and the single-use redemption has the race rastrillo's own store documents fixing. Executed as written, this becomes auth code with real holes. ## Required changes **1. Sign-in cannot find a returning person (Task 4, step 4).** Plan: *"The blind index needs step 4's pepper, which is not settled ... until it is, mint a person row keyed on the ref alone."* With `blind_index TEXT NOT NULL UNIQUE` and no lookup key, every sign-in mints a fresh person ref (and the plan never says what goes in the NOT NULL column). Everything §5.1/§10 keys on the ref — sessions, credentials, recovery codes — and §5.3's `sub` ("stable across email change") attach to an identity nobody can return to. That is not sign-in. Ruling 3 (`HMAC(pepper, lowercased address)`) is in §1's settled table; §3.1's "expensive KDF" is a mitigation note, not a ruling. Implement the index in this step behind one `blindIndex(addr)` function per ruling 3, pepper from platform secrets (`HOME_PEPPER`, added to "Three things to settle" item 1 — losing it orphans every person row), file the §3.1 tension as a spec note, and extend gate 2 to sign in twice with one address and assert one `people` row. **2. The seam is `signin.Flow` with an app-owned `LinkStore`; the Keymail upgrade is silently dropped.** Plan: *"Depend on `signin` directly ... Produces: `(*App).classify`"*, and in Verify: *"Open the sealed address, classify it (Task 5)"*. §5.1 requires "auto-upgrading to Keymail when the address has a claimed inbox". Nothing in the plan performs the upgrade — Task 5 computes a class no code path consumes, and Verify classifies after the mail has already gone. Verified at `signin@v0.1.1`: `Flow.Links` is an interface (`PutLink(ctx, hash, address, purpose, expires)` / `TakeLink(ctx, hash, purpose) (address, ok, err)`), so the app supplies a store that seals in `PutLink` and opens in `TakeLink`, over `signin_links`. `Flow.Begin` then delivers classification, the Keymail path (`beginKeymail` → HMAC'd pending blob in a cookie, `CompleteKeymail` with the address-mismatch check), per-IP (20) and per-address (5) budgets on a 15-minute window, CRLF-safe address parsing (`SplitAddress`), and `LinkTTL`. That is the spec's "classification and Keymail upgrade — which hold nothing", one package down, and it also closes the plan's missing rate limit on `POST /signin` (a mail-bomb vector today). Rewrite Task 5 as the sealed `LinkStore` plus `Flow` wiring (`Origin: "https://id.oficina.works"`, `LinkPath: "/signin/verify"`, non-zero `Key`), add `GET /signin/callback` mirroring `auth.Callback` (redirect to `next.Redirect`, never render it — the host is address-chosen). Fix the test: `signin.ClassKeymail` does not exist; `Class` is `struct{Keymail bool; Server string; Reauth bool}` — assert `Next.Kind == NextMagicLink` from `Flow.Begin` with a failing `LookupTXT`. **3. Single-use redemption is a read-then-write race, and the "same transaction" cannot exist (Task 4, step 4).** Plan: *"Reject if missing, expired, or `redeemed_at` is non-null — single use ... mark the link redeemed **in the same transaction** as the session mint."* `sessions.SignIn`/`Mint` write through their own `*sql.DB` (`create` → `s.cfg.DB.Exec`, verified) — there is no transaction to join, so the executor will improvise. And SELECT-then-UPDATE lets two concurrent clicks both mint; rastrillo's own `linkStore.TakeLink` comment names exactly this race and fixes it with one `DELETE ... RETURNING`. Require a single-statement take (`... WHERE token_hash=? AND redeemed_at IS NULL AND expires_at &gt; ? RETURNING sealed_address`), take first, mint second (a crash between costs one link, never a double use), one indistinguishable refusal for unknown/used/expired, and a two-goroutine test asserting exactly one session row. (§5.1 "short-lived", §3.4 no oracle.) **4. The magic link redeems on GET.** Plan: *"`GET /signin/verify`"* ... *"Look up by `sessions.HashToken(r.URL.Query().Get("t"))`"* → mint. Mail-security scanners prefetch links and burn the single-use token; and a session-minting GET lets any page the victim loads (`&lt;img src=...&gt;`) sign them into the attacker's account — login CSRF, the class §5.6 names for the assertion, one hop earlier. Rastrillo's `auth.Verify` does the same, so this is inherited, not invented — but the home is the suite's only identity authority and the fix is free under zero-JS: GET renders a scriptless confirm form with the token in a hidden field; POST behind `csrf.Protect("https://id.oficina.works")` takes the link. Cross-device still works (§5.5): the token rides the URL, not a cookie. **5. `PRAGMA secure_delete` — resolved now, not "verify whether" (Task 2, step 5).** Plan: *"Verify whether `rastrillo/db.Open` already sets it; if not, set it immediately after open."* Verified: `db.Open` builds `path + "?_pragma=busy_timeout(5000)&amp;_pragma=journal_mode(WAL)&amp;_pragma=foreign_keys(1)"` — no `secure_delete`, no DSN passthrough, two pools (writer capped at 1, readers ≥ 4). `secure_delete` is per-connection: an `Exec` after open lands on one pooled connection and on none opened later. The plan's "read it back and assert 1" passes on that same connection and proves nothing. Use modernc's `sqlite.RegisterConnectionHook` (exists at v1.55.0) to run `PRAGMA secure_delete=ON` on every new connection, and test it by draining a connection from the writer pool after a forced close/reopen. `migrate.Apply` takes `*db.DB`, so bypassing `db.Open` is not the escape. (§5.1, §3.2.) **6. Task 6's source claim is false, and one of its two branches is a spec substitution.** Plan: *"rastrillo ships no client JS for it (`navigator.credentials` appears only in its docs and `webauthn/browser_test.go`, verified at v0.22.0)"* and *"this task also owns writing the WebAuthn JS the framework does not provide."* False: `webauthn/js.go` embeds `js/webauthn.mjs` and exports `webauthn.JS()`; `passkey`'s package doc says apps serve it as a static asset. The ruling request itself is legitimate — §5.2 requires passkeys and §11 forbids script, and only Paul can resolve that — but it must be re-asked on correct facts. Strike *"If passkeys leave slice one, this task becomes 'recovery codes as a second sign-in path' and stays scriptless"*: `SignInRecovery` requires a live `passkey_pending` half-session minted by `Gate` (the package: "it never signs anybody in from nothing"), and §5.2 defines codes as recovery for the second factor. Codes without a passkey are a first-factor bypass the spec never authorised. Add what the plan omits about attachment: at the point Verify would call `sessions.SignIn`, call `passkey.Handlers.Gate(w, r, sess)` exactly as `auth.admit` does; because `sess.Subject` is the person ref, `passkey_credentials`/`passkey_pending`/`passkey_recovery_codes` key on it and the spec's "inherit" worry dissolves — say so, and mount `csrf.Protect` on the recovery POST as the package requires. Recovery soundness I verified at source and the plan should record: ten codes, 50 bits each, SHA-256 via `sessions.HashToken`, one `DELETE ... RETURNING`, no attempt counter justified by the half-session requirement bounded by `pendingTTL`; lockout with codes and passkey both lost is exactly §5.2's disclosed outcome with no operator reset (§14). **7. The RP-ID guard asserts nothing and ignores `LegacyRPID` (Task 7, step 1).** Plan: *"passkey.Config has NO RPID field"*, `configuredOrigins(t)` (undefined; with Task 6 blocked there is no `passkey.Config` to read), and `strings.Count(host, ".") &lt; 2`. `passkey.Config.LegacyRPID` exists (§5.7 cites it) and is a second RP ID accepted for assertions — set to `oficina.works` it widens exactly what §4/ruling 13 forbid. Dot-counting is not "the RP ID is the full host". Assert the app's one credential-origin constant has hostname exactly `id.oficina.works`, and, whenever a `passkey.Config` is built, `LegacyRPID == ""` (or a full host, with a comment naming the drill). Also add the ruling-2 half: the sessions cookie is `__Host-` and host-only (verified in `sessions.setCookie`) — keep an assertion that no cookie on the credential origin carries a `Domain`. **8. Gate 1 is blind to the lowercased address (Task 1).** `sealtest.leaked` is `bytes.Contains` on `CANARY-CELL-VALUE-8f3a`; the blind index (ruling 3) and `signin`'s limiter key use `strings.ToLower(address)`. Code that wrote the lowercased address in the clear passes the gate. Use lowercase, address-shaped canaries in the port (e.g. `canary-8f3a@example.invalid`) or scan case-insensitively. Also, Task 1's steps never port `sealtest.go` or its `TestLeakedDetectsCanary`/WAL tests, yet step 6 commits `internal/home/sealtest` — add the step. **9. Executability gaps that force the executor to re-derive decisions.** - Undefined helpers with no step that creates them: `newTestApp`, `capturedLink`, `configuredOrigins`, `templateNames`, `renderTo`, `sampleData`. - No `sessions.New(sessions.Config{DB: d.Writer(), Origin: "https://id.oficina.works", TTL: ?, SigninPath: "/signin"})` step, no session TTL, and the minted `sessions.Session` has no `Method`/`AuthTime` — `RequireFresh` (which the show-codes-once page must sit behind, per `passkey` docs) checks `AuthTime`; a zero value is never fresh. - Task 5 step 2 *"Run, fail, implement, run"* collapses the TDD cycle into one line with no code. - *"No `main` on this remote ... Work off `skeleton`"* is stale: `refs/heads/main` exists at 462df84. AGENTS.md says cut from `main`. - Task 2 modifies `internal/guardstest/repo_test.go`, which exists only on `feat/gate-9-no-content-in-logs` (not in `main`). State the merge-order dependency. - No link TTL number (falls out of `Flow.LinkTTL` if change 2 is taken), no mailed-link base URL, no sweep for expired sealed rows (sweeping sealed bytes is fine; unbounded growth is not). **10. §3.3 timestamps.** `people.created_at` should be written coarsened to the day; rastrillo's `sessions` rows carry exact RFC3339 `created_at`/`expires_at`, which §3.2 claims are coarsened — cannot be, expiry needs precision. File as a spec correction alongside the other two. **11. Two small fidelity errors.** *"One error string for a hashing failure — 'failed to hash' (§5.1's surrounding text; check §7 ...)"* misreads §3.4, where `"failed to hash alice@example.com"` is the counter-example. And *"It is released"* (Task 7) silently corrects AGENTS.md's and §11's "zero bare `rst-*` attribute selectors" — I count 130 `[rst-` selectors in `ui/tokens.css` at v0.22.0, so the plan is right and AGENTS.md says such disagreements are fixed in the same commit; list it as the third filed correction. ## Verified and sound (keep as-is) - `sessions` schema is `token_hash, subject, method, auth_time, created_at, expires_at` — address-free; `auth.Schema` correctly excluded (`auth_links.address` / `auth_sessions.address` confirmed). - `sessions.NewToken()`/`HashToken` signatures, `mail.Sender` shape, `migrate.Merge`/`MustFromFS`, the sheets-core `seal.go` HKDF line and `LoadSealer` shape all match the plan. - Sealing before insert, refusing to boot without `HOME_DEK`, wrong-purpose test, "prove the gate bites" step, always-303 on begin, person ref from `crypto/rand` never derived from the address, and "per-team enforcement not built" (ruling 15) are all correct — nothing per-team leaks in. - The DEK/SMTP provisioning escalation to Paul is right; add the pepper to it. One non-blocking note: `signin.Classifier` makes outbound DNS and HTTPS requests to whatever domain the person typed. Bound its `HTTP` client with a timeout; the plan's `context.WithTimeout` wrapper covers it if change 2 keeps it.</result> <usage><subagent_tokens>141386</subagent_tokens><tool_uses>22</tool_uses><duration_ms>459082</duration_ms></usage> </task-notification>

diff --git a/internal/guardstest/logsinks_test.go b/internal/guardstest/logsinks_test.go
index cf69df5..b55dafc 100644
--- a/internal/guardstest/logsinks_test.go
+++ b/internal/guardstest/logsinks_test.go
@@ -4,6 +4,7 @@ import (
"go/ast"
"go/parser"
"go/token"
+ "strconv"
"strings"
"testing"
)
@@ -21,9 +22,15 @@ import (
// step 2, and either one would carry an address past a green gate.
const chiMiddleware = "github.com/go-chi/chi/v5/middleware"
-// forbiddenSelectors are package-qualified names that write outside
-// slog. os.Stdout/os.Stderr are the raw sinks; the log package's own
-// printers go to its default writer, which is not the recorder either.
+// forbiddenSelectors are names that write outside slog, keyed by IMPORT
+// PATH rather than by the identifier they happen to be spelled with.
+// os.Stdout/os.Stderr are the raw sinks; the log package's own printers
+// go to its default writer, which is not the recorder either.
+//
+// Keying on the path is what makes `import goos "os"; goos.Stderr` fail
+// too. An earlier version compared the identifier to the literal "os",
+// which an alias walks straight past — and which also fired on a local
+// variable that happened to be named log.
var forbiddenSelectors = map[string]map[string]string{
"os": {
"Stdout": "writes outside slog, so gate 9 cannot see it",
@@ -64,17 +71,49 @@ func TestNoLogSinkOutsideSlog(t *testing.T) {
if err != nil {
t.Fatalf("guards: parse %s: %v", rel, err)
}
+
+ // Local identifier -> import path, so an alias resolves to the
+ // package it actually names.
+ local := map[string]string{}
+ for _, imp := range f.Imports {
+ ipath, err := strconv.Unquote(imp.Path.Value)
+ if err != nil {
+ t.Fatalf("guards: unquote import in %s: %v", rel, err)
+ }
+ name := ipath[strings.LastIndex(ipath, "/")+1:]
+ if imp.Name != nil {
+ name = imp.Name.Name
+ }
+ // A dot-import puts the package's names in file scope
+ // unqualified, so os.Stderr becomes a bare Stderr with no
+ // selector to match. Refuse the import rather than try to
+ // resolve it.
+ if name == "." {
+ if _, forbidden := forbiddenSelectors[ipath]; forbidden {
+ t.Errorf("%s:%d: dot-imports %q, which hides its sinks from this guard",
+ rel, fset.Position(imp.Pos()).Line, ipath)
+ }
+ continue
+ }
+ local[name] = ipath
+ }
+
ast.Inspect(f, func(n ast.Node) bool {
sel, ok := n.(*ast.SelectorExpr)
if !ok {
return true
}
- pkg, ok := sel.X.(*ast.Ident)
+ ident, ok := sel.X.(*ast.Ident)
if !ok {
return true
}
- if why, bad := forbiddenSelectors[pkg.Name][sel.Sel.Name]; bad {
- t.Errorf("%s:%d: %s.%s — %s", rel, fset.Position(sel.Pos()).Line, pkg.Name, sel.Sel.Name, why)
+ ipath, isPackage := local[ident.Name]
+ if !isPackage {
+ return true
+ }
+ if why, bad := forbiddenSelectors[ipath][sel.Sel.Name]; bad {
+ t.Errorf("%s:%d: %s.%s (%s) — %s",
+ rel, fset.Position(sel.Pos()).Line, ident.Name, sel.Sel.Name, ipath, why)
}
return true
})
diff --git a/internal/home/logs_test.go b/internal/home/logs_test.go
index e3eb564..c5662a9 100644
--- a/internal/home/logs_test.go
+++ b/internal/home/logs_test.go
@@ -6,6 +6,7 @@ import (
"net/http"
"net/http/httptest"
"net/url"
+ "regexp"
"strings"
"testing"
@@ -38,6 +39,10 @@ import (
// its own machinery is outside this gate.
// - A sink that is not slog is invisible. internal/guardstest forbids
// the ones that exist today; a new one needs a new rule there.
+// - A route parameter constrained by a regex — {id:[0-9]+} — will not
+// match the canary substituted into it, so its route 404s and its
+// handler is never driven. There are none today. The day there is
+// one, fillParams is where a satisfying value goes.
func TestNoContentInLogs(t *testing.T) {
rec := logtest.NewRecorder()
@@ -62,9 +67,30 @@ func TestNoContentInLogs(t *testing.T) {
"this test enumerates routes so new ones are covered automatically", router)
}
- var targets []string
- err := chi.Walk(routes, func(_ string, route string, _ http.Handler, _ ...func(http.Handler) http.Handler) error {
- targets = append(targets, route)
+ // Each route is driven with the method it was DECLARED with, not
+ // just GET and POST. A route registered only as DELETE or PATCH
+ // answers 405 to both, so its handler never runs and whatever it
+ // logs is never seen.
+ type target struct{ method, path string }
+ var targets []target
+ seen := map[target]bool{}
+ add := func(method, path string) {
+ tg := target{method: method, path: path}
+ if !seen[tg] {
+ seen[tg] = true
+ targets = append(targets, tg)
+ }
+ }
+
+ err := chi.Walk(routes, func(method string, route string, _ http.Handler, _ ...func(http.Handler) http.Handler) error {
+ // chi route patterns can carry wildcards; make them fetchable.
+ // Parameters are filled with a canary so a handler that logs
+ // chi.URLParam logs content rather than the literal "{team}" —
+ // which is what it logged before, passing the gate.
+ path := fillParams(strings.ReplaceAll(route, "/*", "/x"))
+ add(method, path)
+ add(http.MethodGet, path)
+ add(http.MethodPost, path)
return nil
})
if err != nil {
@@ -74,32 +100,43 @@ func TestNoContentInLogs(t *testing.T) {
t.Fatal("walked zero routes: the gate would pass by not looking")
}
// A path that matches nothing, plus a path segment carrying content.
- targets = append(targets, "/no-such-route", "/"+url.PathEscape(logtest.Canaries[1]))
-
- for _, target := range targets {
- // chi route patterns can carry wildcards; make them fetchable.
- target = strings.ReplaceAll(target, "/*", "/x")
+ for _, p := range []string{"/no-such-route", "/" + url.PathEscape(logtest.Canaries[1])} {
+ add(http.MethodGet, p)
+ add(http.MethodPost, p)
+ }
- for _, method := range []string{http.MethodGet, http.MethodPost} {
- // Sign-in will be a POST with the address in the form body,
- // so the body is driven, not just the URL.
- body := strings.NewReader(url.Values{
- "address": {logtest.Canaries[0]},
- "team": {logtest.Canaries[1]},
- }.Encode())
+ for _, tg := range targets {
+ // Sign-in will be a POST with the address in the form body, so
+ // the body is driven, not just the URL.
+ body := strings.NewReader(url.Values{
+ "address": {logtest.Canaries[0]},
+ "team": {logtest.Canaries[1]},
+ }.Encode())
- req := httptest.NewRequest(method, target+"?q="+url.QueryEscape(logtest.Canaries[0]), body)
- req.Header.Set("Content-Type", "application/x-www-form-urlencoded")
- // The headers most likely to be logged wholesale by a future
- // request logger, each carrying content that must never appear.
- req.Header.Set("Referer", "https://"+logtest.Canaries[1]+".example.invalid")
- req.Header.Set("User-Agent", logtest.Canaries[0])
- req.Header.Set("X-Forwarded-For", logtest.Canaries[0])
- req.AddCookie(&http.Cookie{Name: "team", Value: logtest.Canaries[2]})
+ req := httptest.NewRequest(tg.method, tg.path+"?q="+url.QueryEscape(logtest.Canaries[0]), body)
+ req.Header.Set("Content-Type", "application/x-www-form-urlencoded")
+ // The headers most likely to be logged wholesale by a future
+ // request logger, each carrying content that must never appear.
+ req.Header.Set("Referer", "https://"+logtest.Canaries[1]+".example.invalid")
+ req.Header.Set("User-Agent", logtest.Canaries[0])
+ req.Header.Set("X-Forwarded-For", logtest.Canaries[0])
+ req.AddCookie(&http.Cookie{Name: "team", Value: logtest.Canaries[2]})
- router.ServeHTTP(httptest.NewRecorder(), req)
- }
+ router.ServeHTTP(httptest.NewRecorder(), req)
}
rec.AssertNoContent(t)
}
+
+// chiParam matches one chi route parameter, {team} or {id:[0-9]+}.
+var chiParam = regexp.MustCompile(`\{[^{}]*\}`)
+
+// fillParams substitutes a canary for every route parameter.
+//
+// Without it the test requests the pattern literally, so chi.URLParam
+// returns "{team}" and a handler that logs the team slug from the path
+// passes the gate — while in production it logs a real team name. A team
+// slug in a path is the likeliest shape for the directory's own routes.
+func fillParams(pattern string) string {
+ return chiParam.ReplaceAllLiteralString(pattern, url.PathEscape(logtest.Canaries[1]))
+}
diff --git a/internal/home/logtest/logtest.go b/internal/home/logtest/logtest.go
index 3d2d255..347c2f6 100644
--- a/internal/home/logtest/logtest.go
+++ b/internal/home/logtest/logtest.go
@@ -54,8 +54,11 @@ package logtest
import (
"context"
+ "encoding"
+ "encoding/json"
"log/slog"
"net/url"
+ "reflect"
"strings"
"sync"
"testing"
@@ -153,8 +156,9 @@ func (r *Recorder) AssertNoContent(t *testing.T) {
// reader, and the gate needs a form it can scan, including values whose
// printed form differs from their scanned form (see render).
type handler struct {
- rec *Recorder
- attrs []slog.Attr
+ rec *Recorder
+ attrs []slog.Attr
+ groups []string
}
// Enabled answers true at every level. A handler that filtered by level
@@ -165,6 +169,14 @@ func (h *handler) Enabled(context.Context, slog.Level) bool { return true }
func (h *handler) Handle(_ context.Context, r slog.Record) error {
var b strings.Builder
b.WriteString(r.Message)
+ // Open groups are scanned too. A group name is a key, and a key is
+ // as capable of carrying a team slug as a value is —
+ // logger.WithGroup(team.Slug) is a plausible thing for someone to
+ // write, and it would otherwise never be looked at.
+ for _, g := range h.groups {
+ b.WriteByte(' ')
+ b.WriteString(g)
+ }
for _, a := range h.attrs {
appendAttr(&b, a)
}
@@ -181,15 +193,24 @@ func (h *handler) Handle(_ context.Context, r slog.Record) error {
func (h *handler) WithAttrs(as []slog.Attr) slog.Handler {
n := &handler{rec: h.rec}
n.attrs = append(append([]slog.Attr{}, h.attrs...), as...)
+ n.groups = append([]string{}, h.groups...)
return n
}
-// WithGroup keeps the handler unchanged. Grouping only nests keys, and
-// this handler scans values: a canary under a group still arrives in the
-// Record (or in attrs) and is still found. Returning h itself would be
-// wrong if the group name ever mattered — it does not here, and the day
-// it does this method needs writing properly.
-func (h *handler) WithGroup(string) slog.Handler { return h }
+// WithGroup records the name. An earlier version returned h unchanged,
+// reasoning that grouping only nests keys while this handler scans
+// values. That is true of the values and false of the name: the name is
+// caller-supplied, so WithGroup(team.Slug) puts content in the one place
+// the scanner was not looking. Keeping it costs a string.
+func (h *handler) WithGroup(name string) slog.Handler {
+ if name == "" {
+ return h
+ }
+ n := &handler{rec: h.rec}
+ n.attrs = append([]slog.Attr{}, h.attrs...)
+ n.groups = append(append([]string{}, h.groups...), name)
+ return n
+}
// appendAttr writes one attribute's value, recursing into groups.
//
@@ -200,6 +221,11 @@ func (h *handler) WithGroup(string) slog.Handler { return h }
func appendAttr(b *strings.Builder, a slog.Attr) {
v := a.Value.Resolve()
if v.Kind() == slog.KindGroup {
+ // Write the group's own key before recursing, for the reason
+ // WithGroup gives: the name is caller-supplied and can carry
+ // content on its own.
+ b.WriteByte(' ')
+ b.WriteString(a.Key)
for _, g := range v.Group() {
appendAttr(b, g)
}
@@ -211,21 +237,60 @@ func appendAttr(b *strings.Builder, a slog.Attr) {
b.WriteString(render(v))
}
-// render returns the value as production would show it, which is not
-// always what slog.Value.String returns.
+// render returns the union of every form a production handler could
+// print, which is not always what slog.Value.String returns.
+//
+// Three ways a value reads differently in production than through
+// Value.String, all failing in the same dangerous direction — readable
+// in the log, unreadable to the scanner, so the gate passes on a line
+// that leaks:
+//
+// - A byte slice. slog's TextHandler quotes it as a string, so a queue
+// payload logged as []byte is READABLE in production, while
+// Value.String goes through fmt.Sprint and yields "[67 65 78 ...]".
+// The test is on the KIND, not on the []byte type: json.RawMessage
+// is the likeliest type a queue payload actually has, and a type
+// assertion to []byte does not match it. slog's own byteSlice check
+// is a reflect kind check for precisely this reason, and an earlier
+// version of this function asserted the type instead.
+// - An encoding.TextMarshaler. TextHandler prefers MarshalText over
+// Stringer; Value.String goes through fmt, which prefers Stringer.
+// A type whose String() redacts and whose MarshalText() does not
+// therefore leaks in production and reads clean here. This is the
+// LogValuer bug wearing a different interface.
+// - A json.Marshaler, the same trick under JSONHandler, which reaches
+// MarshalJSON through json.Marshal.
//
-// A []byte is the case that matters and it fails in the dangerous
-// direction: slog's text handler quotes it as a string, so a queue
-// payload logged as []byte is READABLE in production, while
-// Value.String goes through fmt.Sprint and yields "[67 65 78 ...]".
-// Scanning the numeric form finds nothing, so the gate would pass on a
-// line that leaks. Queue payloads are exactly the sink most likely to
-// arrive as a byte slice.
+// So take the union rather than choose: any form ANY stock handler could
+// emit is a form the scanner has to see. Concatenating them is safe
+// because the result is only ever substring-searched, never read.
func render(v slog.Value) string {
- if v.Kind() == slog.KindAny {
- if bs, ok := v.Any().([]byte); ok {
- return string(bs)
+ if v.Kind() != slog.KindAny {
+ return v.String()
+ }
+ a := v.Any()
+ var b strings.Builder
+ b.WriteString(v.String())
+
+ if rv := reflect.ValueOf(a); rv.IsValid() &&
+ rv.Kind() == reflect.Slice && rv.Type().Elem().Kind() == reflect.Uint8 {
+ b.WriteByte(' ')
+ b.Write(rv.Bytes())
+ }
+ // Marshal errors are ignored on purpose: a value that fails to
+ // marshal prints nothing in production either, so there is nothing
+ // for the scanner to miss.
+ if tm, ok := a.(encoding.TextMarshaler); ok {
+ if t, err := tm.MarshalText(); err == nil {
+ b.WriteByte(' ')
+ b.Write(t)
+ }
+ }
+ if jm, ok := a.(json.Marshaler); ok {
+ if j, err := jm.MarshalJSON(); err == nil {
+ b.WriteByte(' ')
+ b.Write(j)
}
}
- return v.String()
+ return b.String()
}
diff --git a/internal/home/logtest/logtest_test.go b/internal/home/logtest/logtest_test.go
index a63fcca..f1fe621 100644
--- a/internal/home/logtest/logtest_test.go
+++ b/internal/home/logtest/logtest_test.go
@@ -1,6 +1,7 @@
package logtest
import (
+ "encoding/json"
"errors"
"fmt"
"log/slog"
@@ -169,3 +170,84 @@ func TestFoundCatchesALowercasedCanary(t *testing.T) {
t.Fatalf("found() = %q, want %q", got, Canaries[1])
}
}
+
+// payloadBytes is a NAMED byte slice with no marshaller of its own —
+// the plain form of the type a queue payload has in practice. It exists
+// because v.Any().([]byte) does not match it while slog's own
+// reflect-kind check does, so an assertion-based render read it as
+// "[67 65 78 ...]" and the gate passed on a readable line.
+type payloadBytes []byte
+
+func TestFoundCatchesANamedByteSlicePayload(t *testing.T) {
+ r := NewRecorder()
+ slog.New(r.Handler()).Info("enqueue", "payload", payloadBytes(Canaries[2]))
+ if got := r.found(); got != Canaries[2] {
+ t.Fatalf("found() = %q, want %q", got, Canaries[2])
+ }
+}
+
+// json.RawMessage is the single likeliest type for a queue payload, and
+// it is a named byte slice, so it failed the same way.
+func TestFoundCatchesAJSONRawMessagePayload(t *testing.T) {
+ r := NewRecorder()
+ raw := json.RawMessage(`"` + Canaries[2] + `"`)
+ slog.New(r.Handler()).Info("enqueue", "payload", raw)
+ if got := r.found(); got != Canaries[2] {
+ t.Fatalf("found() = %q, want %q", got, Canaries[2])
+ }
+}
+
+// redactingID redacts through String and leaks through MarshalText.
+// slog's TextHandler prefers MarshalText; fmt — and so slog.Value.String
+// — prefers Stringer. A type shaped like this is not contrived: giving a
+// person type a redacting String is the obvious defensive move, and
+// MarshalText is then the method someone adds later for serialisation.
+type redactingID struct{ address string }
+
+func (r redactingID) String() string { return "person#42" }
+func (r redactingID) MarshalText() ([]byte, error) { return []byte(r.address), nil }
+
+func TestFoundCatchesATextMarshalerThatOutlivesItsString(t *testing.T) {
+ r := NewRecorder()
+ slog.New(r.Handler()).Info("signin", "who", redactingID{address: Canaries[0]})
+ if got := r.found(); got != Canaries[0] {
+ t.Fatalf("found() = %q, want %q", got, Canaries[0])
+ }
+}
+
+// The same trick under JSONHandler, which reaches MarshalJSON through
+// json.Marshal.
+type redactingJSONID struct{ address string }
+
+func (r redactingJSONID) String() string { return "person#42" }
+func (r redactingJSONID) MarshalJSON() ([]byte, error) {
+ return json.Marshal(r.address)
+}
+
+func TestFoundCatchesAJSONMarshalerThatOutlivesItsString(t *testing.T) {
+ r := NewRecorder()
+ slog.New(r.Handler()).Info("signin", "who", redactingJSONID{address: Canaries[0]})
+ if got := r.found(); got != Canaries[0] {
+ t.Fatalf("found() = %q, want %q", got, Canaries[0])
+ }
+}
+
+// A group NAME is caller-supplied, so it can carry a team slug just as a
+// value can. WithGroup previously discarded it.
+func TestFoundCatchesACanaryInAGroupName(t *testing.T) {
+ r := NewRecorder()
+ slog.New(r.Handler()).WithGroup(Canaries[1]).Info("saved", "rows", 3)
+ if got := r.found(); got != Canaries[1] {
+ t.Fatalf("found() = %q, want %q", got, Canaries[1])
+ }
+}
+
+// And an inline group's key, which arrives as an attr rather than
+// through WithGroup.
+func TestFoundCatchesACanaryInAnInlineGroupKey(t *testing.T) {
+ r := NewRecorder()
+ slog.New(r.Handler()).Info("saved", slog.Group(Canaries[1], "rows", 3))
+ if got := r.found(); got != Canaries[1] {
+ t.Fatalf("found() = %q, want %q", got, Canaries[1])
+ }
+}