NextMyDay/docs/reviews/review-001-m0.md

# REVIEW-001 — M0 (correctness + adversarial security)

| Field | Value |
|---|---|
| **Date** | 2026-10-14 |
| **Scope** | M0 (SPEC-002…008) + M0.1 (SPEC-009), at commit `f5c6b95` |
| **Reviewers** | two independent passes — *correctness & spec-conformance* · *adversarial security* |
| **Outcome** | **not safe to expose** until **H1** and **H2** are fixed; **sound enough for a loopback human test** after that |
| **Fixes** | **2026-10-14** — H1 · H2 · M1c · M1s · L3s · L1c/L2s · L8c · M2s · M3s · M2c · L1s · L5s · L4s · L7c landed (see **FIXED** below). **L3c · L4c · L5c · L6c** remain open. |

> The reviewers had no shell in their sandbox, so the `build`/`vet`/`test -race` gate is taken from
> the builders' runs and re-verified by the fix slice. Everything below is a **source** finding.

---

## Findings

| # | Sev | Where | Defect / attack | Triage |
|---|---|---|---|---|
| **H2** | 🔴 HIGH | `server.go` `dispatch` (setup branch) | **Loopback is enforced only at *bind*, never per request.** With a reverse proxy on `127.0.0.1` relaying remote clients, an attacker reaches `/setup`, creates the top admin — **the exact "first-visitor-wins" D-025 claims to close "in code."** | **✅ FIXED** — per-request gate: a non-loopback *resolved* client IP is 404 before dispatch (SPEC-003 §7/AC-10; `TestSpec003SetupIsLoopbackOnlyPerRequest`). |
| **H1** | 🔴 HIGH | `httpx/clientip.go` `resolveForwarded` | **XFF resolved leftmost-untrusted** → an append-style proxy (nginx default) lets the client forge the leftmost entry ⇒ a fresh rate-limit bucket per request (**A3 bypass**), plus forged `client_ip`/`location` in the audit trail (**A11/D-031**). Walk **right→left**. | **✅ FIXED** — right→left, first untrusted entry wins, fall back to the peer (AC-12; `TestClientIPTrustedPeerResolvesRightmostUntrusted`). |
| **M1c** | 🟠 MED | `server.go` `serveSetupAction` | The action door is **not pinned to the `setup` module** — in setup mode `POST /setup?do=core&action=modules.toggle` reaches a core action, bypassing the admin-only `RoleGate`. Inert today (no optional modules); a real primitive at M1. | **✅ FIXED** — `serveSetupAction` rejects `Module != setupModule` (AC-11). |
| **M1s** | 🟠 MED | `main.go` + `config.go` | **Placeholder signing keys run the whole setup:** `csrf_key`/`cookie_secret` stay `CHANGE_ME…` until restart ⇒ the double-submit token and the ceremony cookie are signed with a **publicly-known key** (A9/A15). Generate at startup, like the TOTP key. | **✅ FIXED** — `prepareSigningKeys` at startup in setup mode; `finish` persists the same values. |
| **L3s** | 🟡 LOW | `server.go` (GUID action door) | `/{GUID}?do=setup&action=…` falls through to `serveGUID`; only the handler's own guards make it inert. Enforce `Workflow=="setup"` at the door. | **✅ FIXED** — any resolution naming the `setup` module outside `Workflow=="setup"` is 404 in every mode (AC-11). |
| **L1c/L2s** | 🟡 LOW | `config.go` · `setup.go` | `secrets.session_key` is **dead config** — validated and generated, never read. Remove it (KISS) or document it as reserved. | **✅ FIXED** — removed from `config.go`, `setup.go`, `config.example.yaml` (SPEC-003 D-2 amended). |
| **L8c** | 🟡 LOW | `README.md` | Says "planning / pre-alpha · nothing is built" — stale; also omits that `/setup` **persists `config.yaml`** and a **restart** adopts the generated secrets. | **✅ FIXED** — status + a `/setup` / restart-secrets note. |
| **M2s** | 🟠 MED | `recovery/flow.go` | `/access` **timing oracle**: a hit does two **synchronous** SMTP sends, a miss one `SELECT` ⇒ "is this email on record?" leaks (A3). Notify **asynchronously**. | **✅ FIXED** — notifications run off the request path via `Deps.Dispatch` (`go fn()` by default), detached from the request context; `TestAccessNotificationIsAsynchronous`. |
| **M3s** | 🟠 MED | `middleware/ratelimit.go` | The limiter's `state` map is **never pruned** ⇒ unbounded growth (memory DoS). Prune elapsed windows. | **✅ FIXED** — time-gated sweep of elapsed windows + a hard cap (`TestRateLimiterPrunesElapsedWindows` · `TestRateLimiterMapIsCapped`). |
| **M2c** | 🟠 MED | `cmd/myday` (no test) | The **D-1 TOTP-key-at-startup** invariant is only covered by a *manual* smoke — no automated restart-survival test. A regression silently destroys future TOTP secrets. | **✅ FIXED** — `TestTOTPKeySurvivesSetupAndRestart` generates the key, persists via the wizard, reopens the DB and asserts the secret still unseals. |
| **L1s** | 🟡 LOW | `httpx/cookie.go` | Session cookie lacks the **`__Host-`** prefix (sibling-subdomain cookie-shadow DoS). | **✅ FIXED** — `__Host-myday_session` over https (a `__Host-` cookie is invalid without `Secure`, so it is conditional). |
| **L5s** | 🟡 LOW | `auth/totp/totp.go` | **TOTP replay** not prevented (skew ±90s) — track the last accepted counter per identity. | **✅ FIXED** — the highest accepted counter per identity is tracked; a replayed or older step is `ErrInvalidCode` (`TestVerifyRejectsAReplay`). |
| **L4s** | 🟡 LOW | `httpx/headers.go` | Add `Permissions-Policy` · `Cross-Origin-Opener-Policy` · `Cross-Origin-Resource-Policy`. | **✅ FIXED** — all three on every response. |
| **L3c** | 🟡 LOW | `identities.go` `Mint` | Check-then-insert is **not atomic**; two concurrent Mints → two `pending_enrollment` rows. Add a `UNIQUE(user_id) WHERE status <> 'revoked'` index / wrap in a tx. | fix next |
| **L4c** | 🟡 LOW | `Sessions.GC` · `UpdateWebAuthnSignCount` | Mutate without an audit row; AC-7's test enumerates the audited methods and **omits** these. | fix next |
| **L5c** | 🟡 LOW | `spec009_web_pwa_test.go` | The no-inline-script assertion covers a fixed event list; the CSP header per screen is never asserted. | fix next |
| **L6c** | 🟡 LOW | `middleware/ratelimit.go` | All `/{GUID}/…` traffic shares one bucket; `RetryAfter` uses wall-clock while `Allow` uses the injected clock. | fix next |
| **L7c** | 🟡 LOW | `auth/flow/flow.go` | The nudge trusts the client `Host`; behind a proxy that rewrites `Host`, the nudge is wrongly suppressed. | **✅ FIXED** — the configured `base_url` host is the origin signal (fall back to the request `Host`); `TestSpec006_NudgeIgnoresAReWrittenHost`. |

## Verified sound (no action)

Routing cascade & empty-segment handling · **two doors, one destination** · identification ≠
authorisation (session↔GUID match) · identity rotation revokes all sessions & factors · the
enrolment window · deterministic migration/discovery order · module↔core boundary · **A2** empty
session rejected · **A15/A8** CSRF on every action (session-bound + pre-session double-submit) ·
session fixation (rotation) · **A9** AES-GCM with a fresh nonce, never logged · **A1/A10** bounded
window + GUID tombstoning · **A5** the service worker never caches authed/`/{GUID}` · **A4/A7/R-7**
chain-authority approval · SQL parameterised · `static` traversal-safe · **D-041** the nudge cannot
downgrade auth.

---

## Verdict & gate

**Not safe to expose.** No auth bypass, no session forgery, no CSRF gap, no SQLi, no traversal — the
core is solid. The holes are in the **perimeter** the register calls "enforced": **fix H2 then H1**
before the first client; **M1c/M1s/L3s/L1c** ride the same pass.

A **private, loopback, no-proxy** human test is acceptable **once H1 + H2 land**.

---

## Rulings (Architect, 2026-10-14)

The fix pass landed (`03a9db0..04c2394`), independently verified: **gate green** · static binary ·
`routing` pure · tree clean · `main` == `origin/main`.

| Item | Ruling |
|---|---|
| **Dev. 1** — `__Host-` prefix applied **only over https** | **Accepted.** A `__Host-` cookie *must* carry `Secure`; forcing it would break the documented **http loopback bootstrap**. The prefix applies in the real (https) deployment. |
| **Dev. 2** — the TOTP replay counter is **in-process**, not a DB column | **Accepted** for now (±90 s window bounds the exposure; no migration). A durable `totp_secrets.last_counter` is a later option. |
| **Dev. 3** — the `/access` notify is async; the insert stays synchronous | **Accepted** — the dominant SMTP timing signal is gone; a fully uniform response is deferred. |
| **Dev. 5** — the M2c test drives `setup.Actions.Finish` with a stub, not the whole binary | **Accepted** — it pins the invariant; the live smoke covers the true end-to-end. |
| **A6 residual** (builder, Class B) — a proxy forwarding a **client-supplied** XFF *verbatim* could present a loopback claim during setup | **Adopt the strict setup gate:** in setup mode require **a loopback peer AND no `X-Forwarded-For`** header — the bootstrap is a **direct** SSH tunnel, so a proxied setup is refused. *Fold into the next slice.* |

**Still open (scheduled, non-blocking):** **L3c** (`Mint` check-then-insert atomicity) · **L4c**
(`Sessions.GC` / `UpdateWebAuthnSignCount` unaudited — the reason **A11** stays open) · **L5c**
(per-screen CSP assertion) · **L6c** (per-GUID rate buckets + the `RetryAfter` clock).

**Status: M0 is safe for a private, loopback human test.**

---

## Fix pass (2026-10-14)

**H1 · H2 · M1c · M1s · L3s · L1c/L2s · L8c · M2s · M3s · M2c · L1s · L5s · L4s · L7c are fixed**
(the Triage column above records the code/spec/test for each). The register rows **A3 · A6 · A9 ·
A11 · A13 · A15** were re-checked in `PROJECT_PLAN.md` §10.

Still open (deferred, low): **L3c** · **L4c** · **L5c** · **L6c** — none blocks a loopback human test.

Gate after the fix: `go build` · `go vet` · `go test -race` green; static CGO-free binary;
`internal/core/routing` purity intact. Each headline fix has a test that **fails without it** (H2
verified by reverting the guard — the rejection test fails on `/setup` and on the trusted-proxy
relay).