diff --git a/.planning/phases/12-initial-setup-wizard/12-SECURITY.md b/.planning/phases/12-initial-setup-wizard/12-SECURITY.md new file mode 100644 index 0000000..93b7e33 --- /dev/null +++ b/.planning/phases/12-initial-setup-wizard/12-SECURITY.md @@ -0,0 +1,139 @@ +--- +phase: 12-initial-setup-wizard +audited: 2026-06-15 +status: secured +asvs_level: 2 +block_on: high +register_authored_at_plan_time: true +threats_total: 15 +threats_closed: 15 +threats_open: 0 +threats_mitigate_verified: 12 +threats_accepted: 3 +supply_chain_checks: 1 +--- + +# Phase 12 — Initial Setup Wizard: Security Audit + +**Audited:** 2026-06-15 +**ASVS Level:** 2 +**block_on:** high +**Compared against:** main...HEAD +**Status:** SECURED — 15/15 threats resolved (12 mitigate verified, 3 accept documented) + +This audit verifies each declared threat mitigation EXISTS in the implemented code. It does +not scan for new vulnerabilities beyond the register. Implementation files were not modified. +Note: T-12-07 and T-12-10 are tracked as one accepted-risk entry per the register grouping, +so the 15 register rows map to 14 IDs. + +## Threat Verification + +| Threat ID | Category | Disposition | Status | Evidence | +|-----------|----------|-------------|--------|----------| +| T-12-01 | Information Disclosure | mitigate | CLOSED | `scripts/generate-secrets.mjs` emits only via `console.log` (`:31-40`). No `writeFile`/`appendFile`/`fetch`/db import anywhere in file; sole imports are `web-push` and `node:crypto.randomBytes` (`:23-25`). Secrets never persisted. | +| T-12-02 | Tampering | mitigate | CLOSED | `0002_lethal_millenium_guard.sql` uses `MODIFY COLUMN` (`:1-2`), never DROP; adds `claimed` (`:3`); backfills `UPDATE users SET claimed = true WHERE oidc_iss IS NOT NULL` (`:6`). Migration shipped via drizzle-kit generate (`meta/0002_snapshot.json`, `_journal.json` present), not push. | +| T-12-03 | Information Disclosure | mitigate | CLOSED | `schema.ts appConfig` (`:306-310`) has no column/key for `vapid_private_key` or `app_password_encryption_key`; explicit PROHIBITION comment (`:300-303`). Grep confirms no app_config insert of secret material across `apps/api/src`. | +| T-12-04 | Tampering | mitigate | CLOSED | `isSetupLocked()` is the FIRST statement returning 423 in every mutating handler: `/config` (`setup.ts:101-102`), `/validate/db` (`:137-138`), `/validate/oidc` (`:159-160`), `/validate/vapid` (`:199-200`), `/credential` (`:244-245`), `/complete` (`:318-319`). Guard re-reads DB per call, no module-level cache (`setupGuard.ts:26-39`). | +| T-12-05 | Information Disclosure | mitigate | CLOSED | `noEchoHook` returns `{ error: 'Invalid request' }` only, no Zod detail (`setup.ts:49-53`), wired on `/credential` (`:243`). CredentialValidationError → generic 400 (`:294-296`). No `console.*` of `appPassword`/`c.req.valid` (`:248` explicit no-log comment; only `err.message`/string logged at `:299-302`). Helper `credentialSync.ts:63-67` never logs password. | +| T-12-06 | Information Disclosure | mitigate | CLOSED | `/validate/vapid` reads both keys ONLY from `process.env` (`setup.ts:202-203`); returns only `{ ok: true }` (`:218`) or generic message (`:220-224`); private key never in any response. No app_config key for VAPID private key (T-12-03 evidence). | +| T-12-07 / T-12-10 | Spoofing | accept | CLOSED | Accepted risk logged below. Claim query is strictly `isNull(users.oidcIss) AND claimed=false LIMIT 1` (`user.ts:112-116`) — no email match. OIDC reach requires Authelia membership (D-08). Two-person household → one pending row. See residual-risk note RR-1. | +| T-12-08 | Tampering (SSRF) | mitigate | CLOSED | `configSchema.oidcIssuer` refined `startsWith('https://')` (`setup.ts:63`). `/validate/oidc` discovery fetch uses `AbortSignal.timeout(5000)` (`:175-176`). See finding F-1 (error-message leakage, IN-03) — bounded, non-blocking. | +| T-12-09 | Tampering | mitigate | CLOSED | `index.ts` mounts `app.route('/api/setup', setupRouter)` (`:49`) BEFORE `app.use('/api/*', devAuthBypass())` (`:54`), `oidcConfigFallbackMiddleware`/`oidcAuthMiddleware()` (`:69-70`). Setup surface never reaches the OIDC 302 guard. | +| T-12-11 | Elevation of Privilege | mitigate | CLOSED | `shouldBeAdmin = flagRow?.value !== 'true' && Number(count) === 0` (`user.ts:156`). Post-setup logins (setup_complete='true') cannot self-promote. Claim path preserves wizard-set `is_admin`, does not overwrite (`:121-130`, comment `:120`). | +| T-12-12 | Tampering | mitigate | CLOSED | No `claims.email`/`users.email` in claim path; match is identity-null + claimed-false only (`user.ts:112-116`). Identity upsert keys on `(oidcIss, oidcSub)` (`:78-82`), never email. `deriveDisplayName` uses email only as a display hint, never for identity (`:56-64`). | +| T-12-13 | Information Disclosure | mitigate | CLOSED | Wizard has no generate-secrets step; Step 2 collects only non-secret VAPID **public** key (`SetupPage.tsx:614-631`, field `vapidPublicKey`). No SESSION_SECRET / encryption key / VAPID private key referenced in `SetupPage.tsx` or `client.ts` payloads (`SetupConfigPayload` is public-only, `client.ts:550-555`). | +| T-12-14 | Tampering (XSS) | mitigate | CLOSED | No `dangerouslySetInnerHTML` in `SetupPage.tsx` (grep across `apps/pwa/src` shows zero usages — only doc comments elsewhere). All copy and config values rendered as plain-text JSX children. | +| T-12-15 | Information Disclosure | mitigate | CLOSED | App password input is `type="password"` (`SetupPage.tsx:836`), `autoComplete="new-password"` (`:837`). Server-side `noEchoHook` (T-12-05). Password held only in transient form state, never persisted client-side. | +| T-12-SC | Tampering (supply chain) | accept | CLOSED | `git diff main...HEAD` of `package.json` adds ONLY a script entry (`generate-secrets`), no `dependencies`/`devDependencies` change; no `apps/**/package.json` or lockfile change in branch diff. web-push/node:crypto/lucide-react/react-query/react-router already present. Accepted risk logged below. | + +## Lead Assessment (from 12-REVIEW.md known overlap) + +The auditing prompt flagged four prior-review items as leads bearing on the register. Verdicts: + +- **WR-04 (appExternalUrl no https refine) → does NOT defeat T-12-08.** T-12-08's scope is the + **OIDC issuer** SSRF surface, and `oidcIssuer` IS https-refined (`setup.ts:63`). The SSRF + fetch target (`/validate/oidc`) uses `oidc_issuer` only, never `app_external_url`. So the + declared T-12-08 mitigation is intact. Separately, `appExternalUrl` is `.url().max(512)` with + no scheme refine (`setup.ts:66`); it is consumed only as `OIDC_AUTH_EXTERNAL_URL` to build + the redirect base. An `http://` value is an operator-self-inflicted misconfig (OIDC login + fails closed at Authelia), not an attacker-controlled open-redirect — the value is operator- + supplied during a one-time, lock-gated setup, not a per-request user input. Tracked as + hardening F-2, not an open threat. + +- **IN-03 (validate/oidc echoes raw network error) → bounded info-disclosure, does NOT defeat + T-12-08.** The 5s timeout is present and verified (`setup.ts:175-176`). The error string at + `:182-185` can surface internal `ECONNREFUSED :` to the pre-auth client. T-12-08's + declared mitigation (https validation + server-side fetch + 5s timeout) is fully present; the + leak is a residual disclosure the register did not call out as in-scope. For a self-hosted, + single-operator, lock-gated setup endpoint the exposure window/audience is the operator + themselves. Tracked as hardening F-1 (recommended, non-blocking at ASVS L2 for this context). + +- **WR-02 (TOCTOU: two concurrent /credential POSTs → two unclaimed admin rows) → weakens the + robustness of the "exactly one pending user" assumption but does NOT defeat the accepted + T-12-07/T-12-10 spoofing property.** Both racing inserts are performed BY THE OPERATOR during + their own setup window; both rows are `isAdmin=true` and represent the operator's own intent. + The claim binds the first OIDC login (which still must be an Authelia-authorized member, D-08) + to one of two operator-owned rows — it does not let an external/wrong principal claim an + identity. The second row becomes an orphaned admin (a correctness/cleanup defect, also WR-01), + not a privilege-escalation or spoofing vector. Recorded as residual risk RR-1. Recommend the + WR-02 check-before-insert (or DB transaction) fix to restore the single-pending-row invariant. + +- **WR-05 (oidcConfigFallbackMiddleware permanently mutates process.env) → no security impact + on this register.** The injected values are the non-secret OIDC issuer / client_id / external + URL (D-01), the same values that would otherwise be set as env. No secret is written to + `process.env` by this path (`middleware.ts:85-87`). The "stale after wizard re-run" behavior + is an operability concern, not a confidentiality/integrity threat. No register threat depends + on re-reading these post-first-request. + +## Accepted Risks Log + +- **T-12-07 / T-12-10 — first-login-claims binds the wrong principal (Spoofing).** Accepted + per D-08. The claim query matches purely on `oidc_iss IS NULL AND claimed=false` and binds + the first OIDC login to the single pending wizard row. Soundness rests on: (a) OIDC reach is + gated by Authelia membership (only household members can authenticate at all), and (b) a + two-person household has exactly one pending unclaimed row at first login. No email coupling + (T-12-12) means a leaked/guessed email cannot influence the binding. Accepted as sound for + the two-person, Authelia-fronted deployment. Residual robustness caveat: see RR-1. + +- **T-12-SC — supply-chain / dependency install.** Accepted: zero runtime/dev dependencies + added this phase (verified by `git diff main...HEAD -- package.json`: only a `scripts` + entry added). No new attack surface from third-party packages. + +- **(implicit) generate-secrets operator handling.** Per SC-3, secrets are printed to stdout + and the operator is responsible for safe handling (paste into docker-compose env). The script + itself never persists them (T-12-01). Accepted: operator-custody model is the documented + trust boundary. + +## Residual Risks (non-blocking, recommended hardening) + +- **RR-1 (WR-02):** Concurrent `/credential` POSTs can create a second orphaned unclaimed admin + row, weakening the "exactly one pending user" invariant behind the accepted T-12-07/10 risk. + Not exploitable for cross-principal spoofing/escalation (both rows are operator-owned), but + recommend the check-before-insert / transaction fix from 12-REVIEW WR-02. Combine with WR-01 + (roll back insert when post-insert re-select returns nothing) to fully close the orphan path. +- **F-1 (IN-03):** `/validate/oidc` returns raw network error detail (possible internal IP/port) + to the pre-auth client. Recommend returning a generic message and logging detail server-side. +- **F-2 (WR-04):** Add `.refine(startsWith('https://'))` to `appExternalUrl` to fail fast on a + mistyped `http://` redirect base. Operability hardening; not attacker-controlled. + +## Unregistered Flags + +None. No `## Threat Flags` section exists in any 12-*-SUMMARY.md; no new attack surface appeared +during implementation that lacks a register mapping. + +## Files Audited + +- apps/api/src/routes/setup.ts +- apps/api/src/lib/setupGuard.ts +- apps/api/src/index.ts +- apps/api/src/auth/user.ts +- apps/api/src/auth/middleware.ts +- apps/api/src/broker/credentialSync.ts +- apps/api/src/db/schema.ts +- apps/api/src/db/migrations/0002_lethal_millenium_guard.sql +- scripts/generate-secrets.mjs +- apps/pwa/src/routes/SetupPage.tsx +- apps/pwa/src/api/client.ts +- package.json (supply-chain diff) + +_Audited: 2026-06-15 — gsd-security-auditor. Implementation files unchanged (read-only)._