docs: record the M0 review fixes and correct the README
REVIEW-001 triage updated (H1, H2, M1c, M1s, L3s, L1c/L2s, L8c, M2s, M3s, M2c, L1s, L5s, L4s, L7c marked FIXED; L3c, L4c, L5c, L6c stay open with the reason), and each fix is tied to its code and test. - SPEC-003: the door pipeline (XFF right→left, the per-request setup gate, the isolated hardening headers, the pinned setup module), §6 the `__Host-` cookie and the startup-generated signing keys, §7 loopback enforcement per request, new AC-10…AC-12, D-2 amended (session_key removed), and a new §11 that maps every REVIEW-001 finding to its fix. - SPEC-005 / SPEC-006 / SPEC-007: the same three points where they belong (the wizard's loopback + module pin, the nudge's configured-origin signal, the async /access notification ruled in). - PROJECT_PLAN §10 register: A13 re-stated as enforced (per request + pin); A2/A5/A8 re-stated as enforced where the review verified them; A3/A6/A9/A15 now enforced by this pass; A11 left open (L4c). §14 change log entries. - README: the status said "planning / pre-alpha · nothing is built" — it is stale. State M0+M0.1, note that /setup PERSISTS config.yaml and a restart adopts the generated secrets, and state the per-request loopback rule.
This commit is contained in:
parent
4d5d1f674e
commit
04c2394c34
7 changed files with 174 additions and 23 deletions
|
|
@ -502,24 +502,28 @@ the top admin (first user)
|
|||
|
||||
> **THE GATE — re-check every open item before the first real client / beta.** The threat model
|
||||
> flips from *"nobody has access"* to *"strangers do."*
|
||||
>
|
||||
> **2026-10-14 — REVIEW-001** (M0, independent): the identity/session/CSRF core was **sound**; the
|
||||
> **perimeter** was not. **H1 · H2 · M1c · M1s · L3s · L1c/L2s · M2s · M3s · L1s · L5s · L4s · L7c**
|
||||
> are fixed (see `docs/reviews/review-001-m0.md`); **L3c · L4c · L5c · L6c** remain open.
|
||||
|
||||
| # | Item | Sev | Status |
|
||||
|---|---|---|---|
|
||||
| A1 | **Enrollment window bounded** — un-enrolled GUID is a bearer; TTL + invite delivery + rate limit | HIGH | designed (D-006) |
|
||||
| A2 | Empty/deleted session id rejected on **every** path | HIGH | open |
|
||||
| A3 | `/{GUID}` and `/access` rate-limited; **generic** responses (no enumeration/no timing oracle) | HIGH | open |
|
||||
| A2 | Empty/deleted session id rejected on **every** path | HIGH | **enforced** (M0 review verified) |
|
||||
| A3 | `/{GUID}` and `/access` rate-limited; **generic** responses (no enumeration/no timing oracle) | HIGH | **enforced** — limiter pruned & capped (M3s); `/access` notification is async (M2s) |
|
||||
| A4 | Recovery requires **manager escalation**; approver above the requester; both audited | HIGH | designed (D-009) |
|
||||
| A5 | PWA service worker never caches authenticated responses or `/{GUID}` across sessions | MED | open |
|
||||
| A6 | Trusted-proxy client IP — XFF **set**, not appended | MED | open |
|
||||
| A5 | PWA service worker never caches authenticated responses or `/{GUID}` across sessions | MED | **enforced** (M0 review verified) |
|
||||
| A6 | Trusted-proxy client IP — XFF **set**, not appended | MED | **enforced** — XFF resolved **right→left**, first untrusted hop (H1) |
|
||||
| A7 | **Email = recovery initiation only**; a human must approve | HIGH | designed (D-009) |
|
||||
| A8 | **Actions (`do=`) are form state, not a control** — server-side authz + CSRF on every action | HIGH | open |
|
||||
| A9 | TOTP secret **encrypted at rest** (key from config) | HIGH | open |
|
||||
| A8 | **Actions (`do=`) are form state, not a control** — server-side authz + CSRF on every action | HIGH | **enforced** (M0 review verified) |
|
||||
| A9 | TOTP secret **encrypted at rest** (key from config) | HIGH | **enforced** — key generated at startup (D-1/R-6); replay refused (L5s) |
|
||||
| A10 | GUID **tombstoned, never recycled** | HIGH | designed (D-008) |
|
||||
| A11 | Every auth event audited (bootstrap, enroll, login, recovery request/approve/deny, revoke, `reset_admin`) | HIGH | open |
|
||||
| A11 | Every auth event audited (bootstrap, enroll, login, recovery request/approve/deny, revoke, `reset_admin`) | HIGH | open (L4c: `Sessions.GC` · `UpdateWebAuthnSignCount` unaudited) |
|
||||
| A12 | Changing the **email on record** is a privileged, audited operation | HIGH | open |
|
||||
| A13 | **Setup window hardened** — `/setup` served **only** in setup mode; **loopback-only (enforced)**; rate-limited; `/setup` → 404 once productive | HIGH | designed (D-025/D-026) |
|
||||
| A13 | **Setup window hardened** — `/setup` served **only** in setup mode; **loopback-only, enforced per request (H2)**; the `setup` module **pinned at the door (M1c/L3s)**; rate-limited; `/setup` → 404 once productive | HIGH | **enforced** |
|
||||
| A14 | **Top admin protected** — cannot be deleted; `reset_admin` rotations audited; startup warning while setup mode is reachable | HIGH | open |
|
||||
| A15 | **Pre-session CSRF** — the anonymous forms (`/setup` · `/access` · enroll/login) need a signed **double-submit** cookie (SPEC-005/006/007) | HIGH | open |
|
||||
| A15 | **Pre-session CSRF** — the anonymous forms (`/setup` · `/access` · enroll/login) need a signed **double-submit** cookie (SPEC-005/006/007) | HIGH | **enforced** — signing keys generated at startup in setup mode (M1s) |
|
||||
|
||||
---
|
||||
|
||||
|
|
@ -633,3 +637,6 @@ shell/layout lives in `core/web`; a module's views live with the module.
|
|||
| 2026-10-14 | **SPEC-009 ✅ IMPLEMENTED → M0.1 (human-usable).** `internal/core/web/` — the **shell** (`layout.html` · `NavLister`/`Render` · nav from `Registry.Nav()`, D-038) and the embedded assets (`app.css` · **`webauthn.js`** — the three ceremonies, vanilla, **no inline scripts** · `sw.js` + the `CacheablePrefixes` policy, **A5** · `manifest.webmanifest` with **`start_url: "/"`**, D-011 · icons) · the reserved **`static`** workflow (`core/server/static.go`) · every screen restyled inside the shell (`/setup` · enroll · login · nudge · dashboard · `/access` · Modules page · `/home`) · **Step 0 the module name-collision guard** (`module.CoreDispatchNames` · `ErrCoreDispatchName` — a colliding module now **fails to load**, error + log, SPEC-008 §9 D-9). AC-3…AC-7 asserted **without a browser**; live-smoked end to end from a copy of `config.example.yaml` (bootstrap → TOTP login → **nudge** → dashboard → restart). `-race` clean · static CGO-free binary · `routing` purity intact. **D-1 (flag):** the **TOTP key is now generated at startup** — D-041 enrolls TOTP *before* `finish` persists the R-6 secrets, so a fresh copy of the example could never complete `/setup` ("no TOTP sealer configured"); the ruling R-6 is unchanged, only the timing. **D-2:** `home` is excluded from the collision guard (the dashboard *is* the discovered reference module). **O-15 opened** (§9): should a module named for a reserved level-1 workflow be refused too? |
|
||||
| 2026-10-14 | **SPEC-009 rulings (§13)** — **D-1 accepted** (the TOTP key is generated at **startup**; it fixed a real blocker — a fresh `config.example.yaml` could never complete `/setup`); **D-2 accepted** (the collision guard = the core dispatch modules `auth·setup·recovery·core`; `home` is a level-1 *workflow*). **O-15 RESOLVED**: extend the guard to the reserved level-1 names, **except `home`**. Q-3 → a **README caveat** (reach loopback via `localhost`). Guard extension + README caveat folded into the next slice. |
|
||||
| 2026-10-13 | **SPEC-008 ✅ IMPLEMENTED + Step 0 (R-7)** — the **module registry** & the core **Modules page**: `core/module` (the contract · Registry · page) · `internal/modules/` (**build-time discovery**, D-028: a `go generate` tool writes `imports_gen.go` from the directories) · the reference required **`home`** dashboard module · `main` wiring (load → migrations → mount → log). **R-7 landed**: `users.CanApprove(approver, requester, approverIsAncestor)` — approval authority is the **chain** (manager upward) or the admin (`recovery.isAncestor`). AC-1..6 green, `-race` clean, static binary, live-smoked; `core/routing` purity intact. **M0 complete.** Next: **M1 — org structure**. |
|
||||
| 2026-10-14 | **REVIEW-001 (M0) written** — two independent passes (correctness · adversarial security). Verdict: the **identity/session/CSRF core is sound** (no auth bypass, no session forgery, no CSRF gap, no SQLi, no traversal) but the **perimeter is weak**: **H2** (loopback enforced only at *bind*, not per request) and **H1** (XFF resolved *leftmost*-untrusted, forgeable through an append-style proxy) are **HIGH**; plus M1c/M1s/M2s/M3s/M2c and L1c/L3s/L1s/L5s/L4s/L7c/L8c. `docs/reviews/review-001-m0.md`; register rows re-opened where the review contradicted "enforced". |
|
||||
| 2026-10-14 | **REVIEW-001 FIX PASS (security).** **H2** — a **per-request** loopback gate in `dispatch` while in setup mode (a non-loopback *resolved* client IP → 404; the bind check alone is defeated by a reverse proxy — the exact first-visitor-wins D-025 claims to close); SPEC-003 §7 + AC-10, A13 → **enforced**. **H1** — `ClientIP` walks XFF **right→left** and returns the first untrusted hop, falling back to the peer (A6/AC-12); the unit test that encoded the vulnerable expectation is rewritten. **M1c/L3s** — the `setup` **module is pinned at the door**: `serveSetupAction` rejects `Module != "setup"`, and any other resolution naming it 404s in every mode instead of falling through to `serveGUID`. **M1s** — the CSRF/cookie signing keys are **generated at startup** in setup mode (mirroring the TOTP key, D-1/R-6), so the wizard's ceremony is never signed with a shipped public placeholder; `finish` persists the same values. **M2s** — the `/access` notification runs **off the request path** (a `Dispatch` seam, `go fn()` by default, context detached) so a hit is not timed against a miss (A3). **M3s** — the rate limiter's state map is **pruned** (time-gated sweep at a size threshold) and **capped** (memory-DoS). **L1s** — the session cookie is **`__Host-myday_session`** over https (conditional: a browser rejects `__Host-` without `Secure`, which would break the http loopback bootstrap). **L5s** — **TOTP replay** refused: the highest accepted counter per identity is tracked (reset on re-enrollment). **L4s** — `Permissions-Policy` · `Cross-Origin-Opener-Policy` · `Cross-Origin-Resource-Policy`. **L7c** — the nudge's origin signal is the configured **`base_url` host** (falling back to the request `Host`), so a proxy rewriting `Host` cannot suppress it. SPEC-003 amended (§4/§6/§7/§9/§10 + §11 the fix table). |
|
||||
| 2026-10-14 | **REVIEW-001 FIX PASS (correctness & cleanup).** **L1c/L2s** — dead `secrets.session_key` **removed** (`config.go` · `setup.go` · `config.example.yaml`; SPEC-003 D-2 amended). **L8c** — README corrected (M0/M0.1 status; `/setup` **persists `config.yaml`** and a **restart adopts the generated secrets**; the per-request loopback rule stated). **M2c** — an automated **restart-survival test** (`cmd/myday/main_test.go`) generates the TOTP key at startup, enrolls a secret, persists via the wizard, reopens the DB with the persisted key and asserts the secret still unseals — the **D-1** invariant, previously covered only by a manual smoke. Gate green (`build` · `vet` · `test -race`), **static CGO-free** binary, `routing` purity intact; the H2 guard was reverted to confirm its test fails without it. **Still open (deferred, low): L3c · L4c · L5c · L6c.** |
|
||||
|
|
|
|||
27
README.md
27
README.md
|
|
@ -3,8 +3,11 @@
|
|||
> A self-hosted employee **time, absence & reporting** app by **Sechpoint**.
|
||||
> Delivered as a **PWA** for mobile and desktop. **Single-tenant** — one company per instance.
|
||||
|
||||
**Status:** 🚧 **planning / pre-alpha.** No application code yet.
|
||||
The design of record is [`PROJECT_PLAN.md`](PROJECT_PLAN.md).
|
||||
**Status:** 🚧 **M0 + M0.1 implemented** — the auth & routing foundation and the web/PWA layer
|
||||
(the SPEC-002…009 slices) are built: routing · identity/sessions · the door · `/setup` ·
|
||||
enroll/login (+ the passkey nudge) · roles/chain/recovery · the module registry · the web shell.
|
||||
Next milestone: **M1 — org structure**. The design of record is
|
||||
[`PROJECT_PLAN.md`](PROJECT_PLAN.md).
|
||||
|
||||
---
|
||||
|
||||
|
|
@ -24,8 +27,6 @@ drop-in **modules**.
|
|||
|
||||
## Running it
|
||||
|
||||
> **Not yet available** — nothing is built. These are the intended steps.
|
||||
|
||||
```bash
|
||||
# 1. configure
|
||||
cp config.example.yaml config.yaml # then edit; config.yaml is gitignored
|
||||
|
|
@ -48,6 +49,10 @@ ssh -L 8080:127.0.0.1:8080 user@server # from your workstation
|
|||
# then open http://127.0.0.1:8080/setup and complete the wizard
|
||||
```
|
||||
|
||||
> **Loopback is enforced per request, not just at bind.** While in setup mode the app answers **404**
|
||||
> to any request whose client is not loopback — so a reverse proxy must **not** sit in front of a
|
||||
> fresh instance, even one bound to `127.0.0.1`. Bootstrap over the SSH tunnel (above) first.
|
||||
|
||||
> **Reach the app over `localhost` / `127.0.0.1` — not a hostname.** With an `https://` `base_url`
|
||||
> the CSRF and session cookies are marked **`Secure`**, so they are **not** sent over plain
|
||||
> `http://`; and only a **loopback** origin counts as a trustworthy/secure context for the
|
||||
|
|
@ -59,6 +64,20 @@ Only **after** the top admin exists should you put MyDay behind a reverse proxy
|
|||
|
||||
---
|
||||
|
||||
## What `/setup` writes — and the restart
|
||||
|
||||
The wizard **persists `config.yaml`** (`setup: false`, the base URL, mail/locale, and the secrets)
|
||||
next to the loaded file — never over the committed `config.example.yaml`. If the file is not
|
||||
writable it **prints** the values for you to save by hand, and setup still completes.
|
||||
|
||||
The **secrets are generated at startup** when a value is still `CHANGE_ME…`, so the wizard's
|
||||
ceremony is never signed with a known key. The persisted file then holds **those same values** —
|
||||
so **a restart adopts them** and the admin's TOTP secret (sealed with the generated key) keeps
|
||||
working. Boot a productive instance from the **written `config.yaml`**, not from the example:
|
||||
`config.example.yaml`'s placeholders fail closed in productive mode by design.
|
||||
|
||||
---
|
||||
|
||||
## Documentation
|
||||
|
||||
| | |
|
||||
|
|
|
|||
73
docs/reviews/review-001-m0.md
Normal file
73
docs/reviews/review-001-m0.md
Normal file
|
|
@ -0,0 +1,73 @@
|
|||
# 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**.
|
||||
|
||||
---
|
||||
|
||||
## 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).
|
||||
|
|
@ -2,7 +2,7 @@
|
|||
|
||||
| Field | Value |
|
||||
|---|---|
|
||||
| **Status** | **Implemented** (2026-10-13) — see §10; `build`/`vet`/`test`/`-race` green · static binary · live-smoked |
|
||||
| **Status** | **Implemented** (2026-10-13) — see §10; `build`/`vet`/`test`/`-race` green · static binary · live-smoked. **Hardened 2026-10-14** — see §11 (REVIEW-001 H1/H2/M1c/M3s/L1s/L4s). |
|
||||
| **Created** | 2026-10-13 |
|
||||
| **Author** | OpenArchitect |
|
||||
| **Target milestone** | **M0** |
|
||||
|
|
@ -43,10 +43,13 @@ function, applying **identity / session / role** and all cross-cutting concerns.
|
|||
```
|
||||
request
|
||||
1. correlation id — generate a UUID; put it on the context and in every log line
|
||||
2. client IP — resolve from `server.trusted_proxies`; SET `X-Forwarded-For` (never append) [A6]
|
||||
2. client IP — resolve from `server.trusted_proxies`; walk XFF **right→left** and
|
||||
SET it to the one resolved value (never append) [A6/H1]
|
||||
3. security headers — X-Content-Type-Options · Referrer-Policy · a strict CSP · HSTS (when https)
|
||||
· Permissions-Policy · COOP · CORP [L4s]
|
||||
4. routing.Dispatch() — the pure model (SPEC-002)
|
||||
5. switch on Kind:
|
||||
5. setup-mode gate — in setup mode the **resolved client IP must be loopback**, else 404 (A13/H2)
|
||||
6. switch on Kind:
|
||||
Redirect → 302
|
||||
NotFound → 404
|
||||
Setup → the setup wizard, ONLY in setup mode; else 404
|
||||
|
|
@ -58,12 +61,14 @@ request
|
|||
c. missing/invalid → KindLogin (login screen)
|
||||
d. ROLE gate: does the user's role permit this function?
|
||||
e. build the RequestContext → dispatch to the module function
|
||||
6. audit — every auth event + every mutation (A11)
|
||||
7. rate limit — `/{GUID}`, `/access`, `/setup` (A3)
|
||||
7. audit — every auth event + every mutation (A11)
|
||||
8. rate limit — `/{GUID}`, `/access`, `/setup` (A3)
|
||||
```
|
||||
|
||||
- Before dispatching, the door attaches **`audit.Meta{ActorUserID · ClientIP · CorrelationID}`** to
|
||||
the `context.Context`, so services can write their audit rows (SPEC-004 §5, ruling **R-1**).
|
||||
- The **`setup` module is pinned at the door**: the wizard's functions are reachable **only**
|
||||
through the reserved `setup` workflow; any other resolution naming it 404s (M1c/L3s).
|
||||
|
||||
**The door renders no dynamic HTML** — it resolves, gates, and hands off.
|
||||
|
||||
|
|
@ -92,9 +97,17 @@ type RequestContext struct {
|
|||
## 6. Session cookie & CSRF
|
||||
|
||||
- Cookie `myday_session`: **`HttpOnly` · `Secure` · `SameSite=Lax` · `Path=/`**.
|
||||
- Over **https** the cookie carries the **`__Host-`** prefix (`__Host-myday_session`, L1s): a
|
||||
browser only accepts it with `Secure` · `Path=/` · no `Domain`, so the cookie is pinned to the
|
||||
exact origin and a sibling subdomain cannot shadow it. On plain-http loopback (the bootstrap /
|
||||
human test) the bare name is used — a browser **rejects** a `__Host-` cookie sent without
|
||||
`Secure`, so the prefix cannot be unconditional.
|
||||
- Opaque random id; the server-side row is authoritative (SPEC-004).
|
||||
- **CSRF:** a per-session token on every **action** (`do`/`action`) — hidden field, verified
|
||||
server-side (A8). The action fields are *not* a security control on their own.
|
||||
- The **pre-session** double-submit token (A15) covers every anonymous form (`/setup`, `/access`,
|
||||
enroll/login). In setup mode the CSRF + cookie signing keys are **generated at startup** when
|
||||
left at a placeholder, so the ceremony is never signed with a shipped, public key (M1s).
|
||||
- **Logout** revokes the session row.
|
||||
|
||||
---
|
||||
|
|
@ -103,6 +116,11 @@ type RequestContext struct {
|
|||
|
||||
- In **setup mode**, if `server.listen` is **not loopback**, the process **refuses to start**
|
||||
(fail-closed). The operator bootstraps over an **SSH tunnel** to `127.0.0.1:8080`.
|
||||
- **Per request (H2/A13):** the bind check alone is defeated by a reverse proxy on `127.0.0.1`
|
||||
relaying remote clients — so while in setup mode the door also rejects **any request whose
|
||||
resolved client IP is not loopback** (404, before dispatch). The client IP is the one resolved
|
||||
through `server.trusted_proxies` (§4 step 2), so a trusted proxy forwarding a remote client does
|
||||
**not** launder it into loopback. An empty/unparseable client IP **fails closed**.
|
||||
- Once the top admin exists (`setup: false`), a public bind is allowed.
|
||||
|
||||
---
|
||||
|
|
@ -129,6 +147,9 @@ type RequestContext struct {
|
|||
| AC-7 | In setup mode with a non-loopback `listen`, the process **exits with an error** (D-025) |
|
||||
| AC-8 | Every response carries a correlation id, echoed in the log; path credentials are redacted |
|
||||
| AC-9 | The rate limiter returns **429** on the auth-sensitive routes (A3) |
|
||||
| AC-10 | In setup mode a request whose **resolved client IP is not loopback** is **404** — even when relayed by a trusted proxy (H2/A13) |
|
||||
| AC-11 | The **`setup` module** is reachable only through `Workflow == "setup"`; any other resolution naming it is 404 (M1c/L3s) |
|
||||
| AC-12 | `X-Forwarded-For` is resolved **right→left**; a forged leftmost entry is ignored (H1/A6) |
|
||||
|
||||
---
|
||||
|
||||
|
|
@ -141,7 +162,7 @@ log · loopback enforcement · fail-closed secrets). `internal/core/routing` pur
|
|||
| # | Builder choice | Ruling |
|
||||
|---|---|---|
|
||||
| D-1 | `gopkg.in/yaml.v3` + `KnownFields(true)` (unknown config key = error) | **Accepted** — pure-Go, static-safe, fail-closed. |
|
||||
| D-2 | Secret strength: `session_key`/`cookie_secret`/`csrf_key` ≥ 32 bytes; `totp_encryption_key` = 32 bytes; `CHANGE_ME` rejected in productive mode | **Accepted** — to be documented in the config reference. |
|
||||
| D-2 | Secret strength: `cookie_secret`/`csrf_key` ≥ 32 bytes; `totp_encryption_key` = 32 bytes; `CHANGE_ME` rejected in productive mode | **Accepted** — to be documented in the config reference. **Amended 2026-10-14**: the dead `session_key` is **removed** (L1c/L2s). |
|
||||
| D-3 | CSRF = stateless `HMAC-SHA256(csrf_key, "myday.csrf.v1:"+session_id)` | **Accepted** — no extra table; dies with the session. |
|
||||
| D-4 | In-memory fixed-window rate limiter (30/min; classes `access`·`setup`·`guid`) | **Accepted.** |
|
||||
| D-5 | `RoleGate` **seam**, defaulting to *allow any authenticated user* | **Accepted** — the policy lands in **SPEC-007/008**. |
|
||||
|
|
@ -156,3 +177,26 @@ log · loopback enforcement · fail-closed secrets). `internal/core/routing` pur
|
|||
| **R-5** | **Pre-session CSRF** — `/setup`, `/access` and the enroll/login actions run before a session, so §6's session-bound CSRF doesn't cover them. | **Must be added** — a signed **double-submit** cookie for anonymous forms. Applies to **SPEC-005/006/007**; tracked as **A15**. |
|
||||
| — | `RequestContext.Location` always empty (D-031) | **Accepted** — the seam exists; the IP-geo lookup is deferred. |
|
||||
| — | `RedactPath` over-redacts a segment after a credential-like name | **Accepted** — log-only; no leak. |
|
||||
|
||||
---
|
||||
|
||||
## 11. M0 review hardening (2026-10-14 · REVIEW-001)
|
||||
|
||||
The independent M0 review found the identity/session/CSRF core sound but the **perimeter** weak.
|
||||
The fixes touching this spec:
|
||||
|
||||
| Finding | Fix |
|
||||
|---|---|
|
||||
| **H2** — `dispatch` checked loopback only at *bind* | §7: a **per-request** gate — in setup mode a non-loopback resolved client IP is 404 before dispatch (AC-10). |
|
||||
| **H1** — `resolveForwarded` returned the *leftmost* untrusted hop | `ClientIP` now walks **right→left** and returns the first untrusted entry, falling back to the peer (AC-12). An append-style proxy can no longer be forged past. |
|
||||
| **M1c** — the `/setup` action door was not pinned to the module | `serveSetupAction` rejects `Module != "setup"` (AC-11). |
|
||||
| **L3s** — `/{GUID}?do=setup&…` fell through to `serveGUID` | The door rejects any resolution naming the `setup` module outside `Workflow == "setup"`, in every mode (AC-11). |
|
||||
| **M1s** — placeholder signing keys ran the whole setup | `cmd/myday` generates the CSRF/cookie keys at **startup** in setup mode, mirroring the TOTP key (R-6/D-1); `finish` persists the same values. |
|
||||
| **M3s** — the rate limiter's state map was never pruned | `middleware.RateLimiter` sweeps elapsed windows (time-gated, amortised) and **caps** the map. |
|
||||
| **L1s** — no `__Host-` cookie prefix | §6: session cookie is `__Host-myday_session` over https. |
|
||||
| **L4s** — missing hardening headers | `Permissions-Policy` · `Cross-Origin-Opener-Policy` · `Cross-Origin-Resource-Policy` (§4 step 3). |
|
||||
| **L1c/L2s** — dead `secrets.session_key` | Removed (D-2 amended). |
|
||||
| **L7c** — the passkey nudge trusted the client `Host` | SPEC-006 §8: the configured `base_url` host is the origin signal, falling back to the request `Host`. |
|
||||
|
||||
`build` · `vet` · `test -race` green; the static CGO-free binary is unchanged;
|
||||
`internal/core/routing` purity intact.
|
||||
|
|
|
|||
|
|
@ -26,7 +26,11 @@ Module setup screens (each module owns its config). Multi-admin (there is **one*
|
|||
|
||||
- In setup mode the app serves **only** `/setup` (+ static assets); **every other route → 404**.
|
||||
- **Loopback-only, enforced (D-025):** if `server.listen` is **not** loopback while in setup mode,
|
||||
the process **refuses to start**.
|
||||
the process **refuses to start** — and, per request, a client whose **resolved** IP is not loopback
|
||||
is 404 **before** dispatch (H2/REVIEW-001 · SPEC-003 §7), so a reverse proxy on `127.0.0.1`
|
||||
relaying remote clients cannot reach the wizard.
|
||||
- The `setup` **module** is pinned at the door: the wizard's actions are reachable only through the
|
||||
reserved `setup` workflow (M1c/L3s · SPEC-003 §4).
|
||||
|
||||
## 4. The wizard
|
||||
|
||||
|
|
|
|||
|
|
@ -133,9 +133,12 @@ POST /{GUID}?do=auth&action=passkey.never → users.passkey_opt_out = true (au
|
|||
login redirects to `/{GUID}/auth/passkey` when `shouldNudge` holds; otherwise it lands on the
|
||||
app. `login.finish` (passkey login) never nudges.
|
||||
- **`shouldNudge`** — `Users.PasskeyOptOut` is **false** · the identity has **no** passkey
|
||||
(`Credentials.HasWebAuthnCredential`) · the request origin (`r.Host`) is **not** loopback
|
||||
(`httpx.IsLoopbackHost`) · the nudge is not suppressed (`Deps.SuppressNudge`, wired to setup
|
||||
mode). Any lookup error fails safe — **no** nudge.
|
||||
(`Credentials.HasWebAuthnCredential`) · the origin is **not** loopback (`httpx.IsLoopbackHost`) ·
|
||||
the nudge is not suppressed (`Deps.SuppressNudge`, wired to setup mode). Any lookup error fails
|
||||
safe — **no** nudge.
|
||||
- **The origin signal is the configured `base_url` host** (`Deps.BaseURL`), falling back to the
|
||||
request `Host` only when it is unset (L7c, REVIEW-001): a reverse proxy that rewrites `Host` must
|
||||
not be able to suppress — or enable — the nudge.
|
||||
- **The screen** — a session-gated **route** on the `auth` module:
|
||||
`GET /{GUID}/auth/passkey` (function `passkey.prompt`). The door already holds a session, so the
|
||||
forms carry the **session-bound** CSRF token (`httpx.TokenFromRequest`), not the pre-session one.
|
||||
|
|
|
|||
|
|
@ -86,5 +86,6 @@ F-2 `no-store`).
|
|||
| D-3 | Claim-then-rotate; on a rotation failure the request stays *approved + audited* (never double-rotates) | **Accepted** — safe. |
|
||||
| D-4 | A live pending request is **reused**, so the cap (3) is a hard bound only under concurrency | **Accepted.** |
|
||||
| D-5 | Admin-recovery flag clearing rewrites `config.yaml`; unwritable → logged, continue | **Accepted** (D-035). |
|
||||
| D-6 | The `/access` notification runs **off the request path** (a `Dispatch` seam; `go fn()` by default, context detached with `context.WithoutCancel`, bounded by a 30s timeout) | **Accepted 2026-10-14** (REVIEW-001 **M2s**): a hit must not be timed against a miss — a synchronous send was an enumeration oracle (A3). `Deps.Dispatch` lets tests run it synchronously for determinism. |
|
||||
|
||||
**Follow-up (deferred):** notify the **wider chain**, not only the direct manager.
|
||||
|
|
|
|||
Loading…
Reference in a new issue