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).