oficina / home Public

<task-notification> <task-id>a0514e16630bf2352</task-id> <tool-use-id>toolu_011YGbSk5R4ghf3DxFgEwTww</tool-use-id> <output-file>/home/paulca/.cache/tmp/claude-1001/-home-paulca-amadan-net-oficina-home/d63f5ac7-6b42-48c5-a13d-34e6faf8e44c/tasks/a0514e16630bf2352.output</output-file> <status>completed</status> <summary>Agent "Fable review 3: signin plan" finished</summary> <note>A task-notification fires each time this agent stops with no live background children of its own. The user can send it another message and resume it, so the same task-id may notify more than once.</note> <result>**VERDICT: CHANGES NEEDED** I verified every claim below at source: the plan at `ce18452`, the spec at `fb42ef0`, AGENTS.md at `c7d4b6b`, rastrillo v0.22.0 and keymaildev/signin v0.1.1 from the module cache, modernc sqlite v1.55.0, the carlos edge, and sheets-core. ## A–I, checked at source | | Status | Note | |---|---|---| | A | Done, but the new text is inconsistent | See finding 6 | | B | Verified | `csrf.Protect` is pure origin evidence (`csrf.go:25-59`), GET passes untouched | | C | **Diagnosis false at source** | See finding 1 | | D | Verified | `db.Open` calls `w.Ping()` at `db/db.go:60` before returning; `RegisterConnectionHook` exists at `sqlite.go:785` | | E | Verified | `sessions.Fresh` falls back to `sess.At` (`sessions.go:364-371`) | | F | Verified | `RedirectPath` default `/auth/callback` (`keymail.go:28`); `?token=` (`flow.go:194`); `Method` is a string type | | G | Half done | Timestamp pinned; `purpose` is an instruction in Step 3 while the SQL block above it still lacks the column | | H | Text only | No task step, file or test builds either guard | | I | Not swept | See finding 8 | ## Findings **1. The per-IP claim is false at source, and the true behaviour is a suite-wide sign-in DoS.** The plan says *"`r.RemoteAddr` for the peer is `""`, `auth`'s `clientIP` returns it unchanged, and `Flow.Begin` skips the IP budget entirely when `clientIP == ""`."* I measured it: over a Linux unix socket `r.RemoteAddr` is `"@"`. `clientIP("@")` returns `"@"` (SplitHostPort fails, `handlers.go:259-265`), and `Flow.Begin` only skips when exactly `""` (`flow.go:122`). So every request shares the key `ip:@` with budget 20 per 15 minutes — twenty POSTs to `/signin` lock out the whole suite for a quarter hour. rastrillo's own comment admits the collapse ("collapsing the per-IP budget into a global one"). The plan then refuses to decide: *"either name the trusted platform header … or pass `""` deliberately … Decide it here rather than discovering it in production"* — and doesn't. The answer exists: the carlos edge runs `pr.SetXForwarded()` on every proxied request and strips inbound `X-Forwarded-For` unless hop-verified (`platform/internal/edge/homemap.go:808, 910-918`). The plan must say: take the last `X-Forwarded-For` element, fall back to `RemoteAddr` only when the header is absent, and test both. **2. Task 6 is still a stub.** It has zero `- [ ]` steps, no Files, no Interfaces, no tests, no commit. Every other task has all of them. What an executor cannot derive from it: - `passkey.Schema` must be merged into `BootSchema`; Task 2's `migrate.Merge(sessions.Schema, Schema)` omits it, and `Gate` INSERTs into `passkey_pending` (`passkey.go:206`). - The seven routes from the package doc (`passkey.go:20-27`), `ConfirmPath`, the enrolment page, and the show-codes-once page *"mounted behind sessions.RequireFresh"* (`passkey.go:56-57`). - *"So nobody has to write it"* is overstated. `webauthn.mjs` only **exports** `register()`/`authenticate()` (152 lines, no DOM wiring). The default CSP forbids inline script (`serve.go:381-382`), so the app must write and serve its own same-origin module that calls them and POSTs to `/passkey/*`. That is script the plan says nobody writes. - No test that an enrolled person cannot obtain a session from the first factor alone. **3. Handler-without-guard: the Keymail callback bypasses the second factor.** Task 6: *"At the point `POST /signin/verify` would call `sessions.SignIn`, call `passkey.Handlers.Gate`"*. Task 5: *"`GET /signin/callback` may mint on GET"*. Nothing puts `Gate` on the callback. In rastrillo both `Verify` and `Callback` funnel through the one `admit` (`handlers.go:75-111, 132-160`). As written, a Keymail-upgraded account with a passkey enrolled signs in with one factor. **4. Task 5 Step 3 drops the Keymail upgrade the task exists to deliver.** *"`POST /signin` calls `Flow.Begin`, then 303 to a "check your mail" page — always, and always the same page."* On `NextKeymail`, `Begin` returns `Redirect` and `Pending` and sends no mail (`flow.go:151-176`); the handler must set the pending cookie and 303 to `next.Redirect` (`handlers.go:52-57`). Also missing: the `force` escape hatch (`auth.Callback` redirects to `?force=1&amp;err=keymail`; without it a failed Keymail approval is a dead end, against §5.1's "every address has a working path"), and the pending cookie's attributes — `__Host-`, `SameSite=Lax`, `HttpOnly`, cleared on read — which are unexported in `auth` and are exactly what the GET-may-mint argument rests on. **5. Secrets and boot guards with no step.** `Flow.Key` — *"a non-zero `Key` from config"* — is a fourth secret nobody names; `sealPending` refuses a zero key (`pending.go:ErrNoKey`), so the first Keymail address errors. `HOME_PEPPER` loading has no file, test or step. The mailer and `DBPath` refusals (H) live in the preamble only. Under *"TDD is not optional"*, none of these get built. **6. The Task 4/5 restructure does not hang together.** Task 4's Files say `signin.go`/`signin_test.go`/templates but its commit adds `links.go`/`links_test.go`; Task 5's Files say create `links.go` — the file Task 4 just committed. Both tasks "Produce: `(*App).links`". Task 4's Interfaces name `signin.LinkStore` and `newTestApp` wires a `signin.Flow`, but the `signin` dependency is only added in Task 5. `sealtest.Token` (Task 5 Step 1) is produced nowhere (`sealtest` exports `Canaries` only). Task 5's `GET /signin/verify` *"renders a scriptless confirmation page"* using `(*App).render`, produced in Task 7, with a template no task names. `newTestApp`/`signIn` are in a table but no step builds them. `TestFailedClassificationStillSendsAMagicLink` is vacuous unless `newTestApp` wires a non-nil `Flow.Keymail` — `Begin` skips classification entirely when it is nil (`flow.go:145`). **7. The plan contradicts itself on the blind index.** Task 2 Step 3: *"it must be settled before the pepper is implemented in step 4 … nothing writes it yet."* Task 4: *"The blind index is built in this task, not deferred."* The Deferred table still lists it as blocked. Ruling 3 is settled in §1, so Task 4 is right; the other two sentences must go. **8. Residue not swept.** Deferred table: *"Passkey enrollment and step-up | Paul rules on §11's zero-JS absolute"*. Self-review: *"§5.2 is Task 6, blocked and said so"*, *"gates 1 and 2 are Tasks 1 and 4"*, *"The one unverified identifier: `signin.ClassKeymail`"*. Task 7 Step 2: *"If Task 6's ruling grants a carve-out"* — conditional on a ruling already made, and the test code has no allowlist. File Structure lists `classify.go` (no task creates it) and no `links.go`. Task 4 "Consumes: … `sessions.Sessions`, `mail.Sender`" though it builds only the store. **9. What ruling 21 now lets through that the plan should still forbid.** The allowlist as described exempts *screens*, which admits any script on them. It should constrain: the only `&lt;script&gt;` on an allowlisted template is a same-origin `src` from a named list; no inline bodies; a test that the served CSP is still rastrillo's default (`Options.CSP` untouched); and, since the confirm page will carry both the ceremony and the recovery form, a script-stripped render of it must still contain a working `code` form posting to `/passkey/signin/recovery` (gate 6's shape). Executor detail: serve `.mjs` with an explicit `text/javascript` Content-Type, or strict MIME checking blocks the module. **10. The GET-may-mint-on-callback argument is sound** — with the caveats in 3 and 4. `CompleteKeymail` requires the HMAC'd pending blob from the cookie and `p.State == state` (`flow.go:211-218`), then an address-mismatch check; the callback URL is never emailed, so the scanner argument does not apply. It holds only if the app replicates the `__Host-`/Lax/single-use cookie exactly and puts `Gate` on the path. Minor: `RegisterConnectionHook` is process-global, so `newTestApp` needs a `sync.Once`; the `signIn` helper must send an `Origin` header or `csrf.Protect` refuses its POSTs.</result> <usage><subagent_tokens>148224</subagent_tokens><tool_uses>34</tool_uses><duration_ms>391758</duration_ms></usage> </task-notification>