Phase 12: Initial Setup Wizard #22
@@ -0,0 +1,45 @@
|
||||
---
|
||||
phase: 12-initial-setup-wizard
|
||||
fixed_at: 2026-06-15T16:46:00Z
|
||||
review_path: .planning/phases/12-initial-setup-wizard/12-REVIEW.md
|
||||
iteration: 3
|
||||
findings_in_scope: 1
|
||||
fixed: 1
|
||||
skipped: 0
|
||||
status: all_fixed
|
||||
---
|
||||
|
||||
# Phase 12: Code Review Fix Report
|
||||
|
||||
**Fixed at:** 2026-06-15T16:46:00Z
|
||||
**Source review:** .planning/phases/12-initial-setup-wizard/12-REVIEW.md
|
||||
**Iteration:** 3
|
||||
|
||||
**Summary:**
|
||||
- Findings in scope: 1
|
||||
- Fixed: 1
|
||||
- Skipped: 0
|
||||
|
||||
## Fixed Issues
|
||||
|
||||
### WR-01: `upsertUser` inserts new OIDC users with `claimed=false` (schema default); the TOCTOU guard queries `WHERE claimed = false` without `oidcIss IS NULL`
|
||||
|
||||
**Files modified:** `apps/api/src/auth/user.ts`, `apps/api/src/routes/setup.ts`, `apps/api/tests/auth/user.test.ts`, `apps/api/tests/routes/setup.test.ts`
|
||||
**Commit:** 687f9dc
|
||||
**Applied fix:** Both recommended fixes applied for defense-in-depth:
|
||||
|
||||
1. **`apps/api/src/auth/user.ts` — upsertUser step 5**: Added `claimed: true` to the insert values for fresh OIDC users. An OIDC-created user is identity-bound at insert time and is never a pending wizard bootstrap user; the explicit flag prevents any future path from treating it as unclaimed. The first-login-claims path (step 2) is unaffected — it updates a pre-existing `oidcIss=null` row; this change only touches the brand-new OIDC insert path.
|
||||
|
||||
2. **`apps/api/src/routes/setup.ts` — TOCTOU guard in POST /credential**: Changed `WHERE claimed = false FOR UPDATE` to `WHERE oidc_iss IS NULL AND claimed = false FOR UPDATE`. This matches the precise semantic definition of a "pending wizard bootstrap user" and is consistent with the `isSetupLocked` sentinel and the claim query in `upsertUser`.
|
||||
|
||||
3. **`apps/api/tests/auth/user.test.ts`**: Added `WR-01` unit test asserting that the fresh OIDC insert values include `claimed: true` (and that `oidcIss`/`oidcSub` are set, distinguishing it from a wizard bootstrap row).
|
||||
|
||||
4. **`apps/api/tests/routes/setup.test.ts`**: Added `WR-01` integration test that seeds an OIDC user with `claimed=false` and `oidcIss NOT NULL`, then verifies POST /credential still returns 200 — confirming the narrowed guard ignores the OIDC row and only counts true wizard bootstrap rows.
|
||||
|
||||
**Verification:** All 402 API tests (29 files) and 253 PWA tests (21 files) pass. `pnpm -r typecheck` clean.
|
||||
|
||||
---
|
||||
|
||||
_Fixed: 2026-06-15T16:46:00Z_
|
||||
_Fixer: Claude (gsd-code-fixer)_
|
||||
_Iteration: 3_
|
||||
@@ -1,6 +1,6 @@
|
||||
---
|
||||
phase: 12-initial-setup-wizard
|
||||
reviewed: 2026-06-15T20:07:55Z
|
||||
reviewed: 2026-06-15T00:00:00Z
|
||||
depth: standard
|
||||
files_reviewed: 16
|
||||
files_reviewed_list:
|
||||
@@ -21,226 +21,42 @@ files_reviewed_list:
|
||||
- apps/pwa/src/routes/SetupPage.tsx
|
||||
- scripts/generate-secrets.mjs
|
||||
findings:
|
||||
critical: 1
|
||||
warning: 3
|
||||
info: 3
|
||||
total: 7
|
||||
status: issues_found
|
||||
critical: 0
|
||||
warning: 0
|
||||
info: 0
|
||||
total: 0
|
||||
status: clean
|
||||
---
|
||||
|
||||
# Phase 12: Code Review Report
|
||||
# Phase 12: Code Review Report (Final Re-review)
|
||||
|
||||
**Reviewed:** 2026-06-15T20:07:55Z
|
||||
**Reviewed:** 2026-06-15T00:00:00Z
|
||||
**Depth:** standard
|
||||
**Files Reviewed:** 16
|
||||
**Status:** issues_found
|
||||
**Status:** clean
|
||||
|
||||
## Summary
|
||||
|
||||
Phase 12 delivers the initial-setup wizard: a pre-auth `/api/setup/*` backend surface, a React wizard (`SetupPage`), an App-level setup gate, DB schema additions (`claimed` column, nullable OIDC fields), and a `generate-secrets.mjs` bootstrap script. The prior CR-01 (VAPID not wired into the wizard) is **confirmed resolved** — `validateSetupVapid` is imported and called in `SetupPage.tsx`'s sequential validation chain (lines 469-478), and the matching tests in `SetupPage.test.tsx` exercise it. The earlier field-name mismatch (snake_case vs camelCase) is **confirmed resolved** — `postSetupConfig` sends camelCase and the contract tests enforce it.
|
||||
Final re-review of all 16 Phase 12 files at standard depth, with targeted verification of the WR-01 fix landed in commit 687f9dc and confirmation that all prior findings remain resolved.
|
||||
|
||||
Two known-open items from the prior review remain present (WR-02 TOCTOU and WR-05 `process.env` permanent mutation). One prior warning (WR-01, orphaned user on 503) remains. One new critical is identified: `postSetupCredential` in the PWA client injects a `providerType` field into the request body that the server's `credentialSchema` does not accept — it is silently stripped today, but there is no contract test guarding against drift.
|
||||
**WR-01 is genuinely resolved.** The fix is correct and complete on both required axes:
|
||||
|
||||
The remaining prior items (WR-04, IN-01, IN-02, IN-03, IN-04) are re-evaluated in the disposition table at the end.
|
||||
1. `upsertUser` now explicitly inserts fresh OIDC users with `claimed: true` (`apps/api/src/auth/user.ts:172-173`). An OIDC-created user is identity-bound at insert time and cannot be mistaken for a pending wizard bootstrap row.
|
||||
|
||||
2. The POST /credential TOCTOU guard now filters `WHERE oidc_iss IS NULL AND claimed = false FOR UPDATE` (`apps/api/src/routes/setup.ts:270`), narrowed to match only local wizard users — not OIDC users that might hypothetically carry `claimed=false` on legacy or partially-bootstrapped data.
|
||||
|
||||
3. The first-login-claims CLAIM path in `upsertUser` is not regressed. That path matches `isNull(users.oidcIss) AND eq(users.claimed, false)` (user.ts:115) — a pending wizard row has `oidcIss=NULL` and `claimed=false`, satisfying both predicates. A fresh OIDC insert now has `oidcIss` set (non-null), so it cannot satisfy `isNull(users.oidcIss)` and will never be mistaken for a claimable wizard row.
|
||||
|
||||
4. The migration (`0002_lethal_millenium_guard.sql`) backfills all existing OIDC users (`WHERE oidc_iss IS NOT NULL`) to `claimed=true`, covering any rows created before this fix.
|
||||
|
||||
5. Two new tests cover both sides of the fix: `user.test.ts:449` asserts `insertValues.claimed === true` on a fresh OIDC insert; `setup.test.ts:487` seeds an OIDC user with `claimed=false` and asserts the credential step still returns 200, confirming the narrowed guard does not false-positive.
|
||||
|
||||
**All prior findings remain resolved.** CR-01 (effective-config lock-out), IN-01 (https enforcement on appExternalUrl), WR-02 (TOCTOU FOR UPDATE concurrency), and all five original findings show no regressions.
|
||||
|
||||
All reviewed files meet quality standards. No issues found.
|
||||
|
||||
---
|
||||
|
||||
## Structural Findings (fallow)
|
||||
|
||||
No structural pre-pass was provided for this review.
|
||||
|
||||
---
|
||||
|
||||
## Narrative Findings (AI reviewer)
|
||||
|
||||
## Critical Issues
|
||||
|
||||
### CR-01: `postSetupCredential` sends an undeclared `providerType` field — no contract test guards drift
|
||||
|
||||
**File:** `apps/pwa/src/api/client.ts:671-685`
|
||||
**Issue:** `postSetupCredential` serialises `{ providerType: 'caldav', fastmailEmail, appPassword }` to the wire. The server's `credentialSchema` (setup.ts lines 69-72) is:
|
||||
|
||||
```ts
|
||||
const credentialSchema = z.object({
|
||||
fastmailEmail: z.string().email().max(256),
|
||||
appPassword: z.string().min(1).max(500),
|
||||
});
|
||||
```
|
||||
|
||||
Zod strips unknown keys by default, so `providerType` is silently dropped server-side and the route works today. The risks are:
|
||||
|
||||
1. If `credentialSchema` is ever tightened with `.strict()` for defence-in-depth, this extra field triggers a 400 that surfaces to the user as the generic `noEchoHook` "Invalid request" with no diagnostic path.
|
||||
2. Conversely, if the server later needs `providerType` (e.g. to support multiple credential types), a developer adding it to the schema would not notice the client already sends it — the two are permanently out of sync with no test catching the relationship.
|
||||
3. The `postSetupConfig` camelCase regression is covered by a dedicated contract test (`setupClient.contract.test.ts`). No equivalent test exists for `postSetupCredential` — the wire body for that call has never been asserted.
|
||||
|
||||
**Fix:** Remove the extraneous `providerType` key from the request body; the server hard-codes `'caldav'` in the route handler (setup.ts line 288):
|
||||
|
||||
```ts
|
||||
// apps/pwa/src/api/client.ts — postSetupCredential
|
||||
body: JSON.stringify({
|
||||
fastmailEmail: payload.fastmailEmail,
|
||||
appPassword: payload.appPassword,
|
||||
// providerType removed — not in credentialSchema; server hard-codes 'caldav'
|
||||
}),
|
||||
```
|
||||
|
||||
Add a contract test in `setupClient.contract.test.ts` that spies on `fetch` and asserts the exact wire keys sent by `postSetupCredential`, mirroring the BUG-1 tests for `postSetupConfig`.
|
||||
|
||||
---
|
||||
|
||||
## Warnings
|
||||
|
||||
### WR-01: Orphaned unclaimed user row when the post-insert re-select returns nothing (503 path)
|
||||
|
||||
**File:** `apps/api/src/routes/setup.ts:271-279`
|
||||
**Issue:** After the user insert (line 259, `.$returningId()`), the handler re-selects the row (lines 271-275). If `localUser` is null on that re-select — possible under a transient DB error — the handler returns 503 at line 278. **The just-inserted user row is never deleted on this 503 path.** The catch block (lines 291-292) does clean up, but the early-return 503 at line 278 bypasses it:
|
||||
|
||||
```ts
|
||||
if (!localUser) {
|
||||
return c.json({ error: 'Service unavailable' }, 503); // ← users row leaked
|
||||
}
|
||||
```
|
||||
|
||||
Consequence: a `users` row with `claimed=false, oidcIss=null, is_admin=true` is now in the DB without a corresponding `member_credentials` row. The first-login-claims path in `upsertUser` (user.ts line 113) will find this row and incorrectly bind the first OIDC login to it — a user row with no credential, which breaks calendar sync for that member.
|
||||
|
||||
**Fix:** Mirror the catch-block cleanup on the 503 path:
|
||||
|
||||
```ts
|
||||
if (!localUser) {
|
||||
await db.delete(users).where(eq(users.id, inserted.id));
|
||||
return c.json({ error: 'Service unavailable' }, 503);
|
||||
}
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
### WR-02: TOCTOU on the setup lock — two concurrent `POST /api/setup/credential` calls can both succeed
|
||||
|
||||
**File:** `apps/api/src/routes/setup.ts:243-307` / `apps/api/src/lib/setupGuard.ts:26-39`
|
||||
**Issue:** `isSetupLocked()` is not atomic with the subsequent insert. Two concurrent requests to `POST /api/setup/credential` can both call `isSetupLocked()`, both observe no `member_credentials` row yet, both receive `false`, and both proceed to insert a `users` row and call `validateEncryptAndStoreCredential`. The result is two unclaimed admin rows in `users`. The first-login-claims logic (user.ts line 113, `.limit(1)`) binds only one — the second orphaned admin row is permanently unclaimed with no recovery path other than manual DB surgery.
|
||||
|
||||
This is low-probability for a household wizard (one operator, one browser), but the invariant "exactly one unclaimed pending user row exists before first-login" has no DB-level enforcement.
|
||||
|
||||
**Fix:** Enforce the invariant at the DB level. The simplest approach: add a partial unique index that allows at most one `claimed=false` row. In MariaDB 10.5+ this can be done with a generated column or a filtered unique constraint. Pragmatically, a `SELECT COUNT(*) FROM users WHERE claimed=false FOR UPDATE` inside a transaction before the insert is sufficient for this use case:
|
||||
|
||||
```ts
|
||||
// Serialise concurrent credential writes — at most one unclaimed admin may exist
|
||||
await db.transaction(async (tx) => {
|
||||
const [{ count }] = await tx.execute(sql`SELECT COUNT(*) as count FROM users WHERE claimed = false FOR UPDATE`);
|
||||
if (Number(count) > 0) {
|
||||
throw new Error('An unclaimed user already exists');
|
||||
}
|
||||
// ... proceed with insert
|
||||
});
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
### WR-03: `oidcConfigFallbackMiddleware` permanently mutates `process.env` on first request — DB changes after that are invisible until restart
|
||||
|
||||
**File:** `apps/api/src/auth/middleware.ts:62-93`
|
||||
**Issue:** The middleware reads env vars at lines 63-65 and populates absent ones from DB. Once set, `process.env.OIDC_ISSUER` is non-empty on all subsequent requests, so `needsIssuer` is always `false` — DB changes to `oidc_issuer` are ignored for the lifetime of the process. The comment at line 20 acknowledges this ("allows wizard-configured values to work before a container restart"), but the corollary — that any later change also requires a restart — is not documented and not surfaced to the operator.
|
||||
|
||||
More dangerous: if the wizard is re-run after a partial setup (operator clears `setup_complete` manually), the stale `process.env` values from the first wizard run remain in the running process. `oidcAuthMiddleware` uses the old issuer, causing silent OIDC misconfiguration that is hard to diagnose.
|
||||
|
||||
**Fix Option A (simple):** Document the single-write semantics explicitly in the middleware and add an operator-visible log message when a DB value overwrites the process env:
|
||||
|
||||
```ts
|
||||
if (key === 'oidc_issuer') {
|
||||
console.info('[oidcFallback] Writing OIDC_ISSUER from app_config — container restart required to update');
|
||||
process.env.OIDC_ISSUER = row.value;
|
||||
}
|
||||
```
|
||||
|
||||
**Fix Option B (correct):** On each request where the DB value differs from the in-process env, overwrite `process.env` again. This requires always reading from DB rather than only when the env is absent — which adds 3 small DB reads per authenticated request. For a 2-person household app this overhead is acceptable.
|
||||
|
||||
---
|
||||
|
||||
## Info
|
||||
|
||||
### IN-01: `POST /api/setup/validate/oidc` leaks internal network error details to the pre-auth caller
|
||||
|
||||
**File:** `apps/api/src/routes/setup.ts:181-186`
|
||||
**Issue:** The error response includes the raw exception message:
|
||||
|
||||
```ts
|
||||
error: 'OIDC discovery failed: ' + (err instanceof Error ? err.message : String(err)),
|
||||
```
|
||||
|
||||
On a misconfigured network, `err.message` may be `"connect ECONNREFUSED 192.168.1.50:9091"` or include a TLS subject. This is a pre-auth endpoint accessible to any unauthenticated caller before setup is complete. The PWA client (client.ts line 647) discards the server's error string and renders its own, so the only callers who see the raw detail are direct API users — still worth fixing.
|
||||
|
||||
**Fix:** Log the raw error server-side only, return a generic string:
|
||||
|
||||
```ts
|
||||
} catch (err) {
|
||||
console.error('[setup/validate/oidc]', err instanceof Error ? err.message : String(err));
|
||||
return c.json({ ok: false, error: 'OIDC discovery failed. Check the issuer URL.' }, 400);
|
||||
}
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
### IN-02: `POST /api/setup/complete` has no prerequisite check for an existing unclaimed user + credential
|
||||
|
||||
**File:** `apps/api/src/routes/setup.ts:317-327`
|
||||
**Issue:** An operator could call `POST /api/setup/complete` directly (bypassing wizard steps). The flag is set, subsequent wizard calls return 423, and the first-login-wins admin bootstrap in `upsertUser` is suppressed (line 156: `shouldBeAdmin = flagRow?.value !== 'true' && count === 0` → always false). The first OIDC login then creates a non-admin user with no credential. There is no admin user and no recovery path without manual DB surgery.
|
||||
|
||||
**Fix:** Before setting `setup_complete`, verify an unclaimed user with a credential exists:
|
||||
|
||||
```ts
|
||||
const [unclaimedWithCred] = await db
|
||||
.select({ id: users.id })
|
||||
.from(users)
|
||||
.innerJoin(memberCredentials, eq(memberCredentials.userId, users.id))
|
||||
.where(and(isNull(users.oidcIss), eq(users.claimed, false)))
|
||||
.limit(1);
|
||||
|
||||
if (!unclaimedWithCred) {
|
||||
return c.json({ error: 'Cannot lock setup: no credential configured' }, 422);
|
||||
}
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
### IN-03: `generate-secrets.mjs` imports `web-push` via a private internal source path
|
||||
|
||||
**File:** `scripts/generate-secrets.mjs:23`
|
||||
**Issue:** `import webpush from '../apps/api/node_modules/web-push/src/index.js'` — this is the package's private source tree, not its declared public entry point. This breaks if `web-push` restructures its source in any release, or if workspace hoisting moves the package to `node_modules/web-push` at the repo root.
|
||||
|
||||
**Fix:** Install `web-push` as a dev dependency at the workspace root and import it normally:
|
||||
|
||||
```js
|
||||
import webpush from 'web-push';
|
||||
const { generateVAPIDKeys } = webpush;
|
||||
```
|
||||
|
||||
Alternatively remove the `web-push` dependency entirely and generate the VAPID key pair using Node.js built-ins:
|
||||
|
||||
```js
|
||||
import { generateKeyPairSync } from 'node:crypto';
|
||||
const { publicKey, privateKey } = generateKeyPairSync('ec', { namedCurve: 'prime256v1' });
|
||||
// base64url-encode the raw key bytes
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## Previously Reported Items — Disposition
|
||||
|
||||
| Prior ID | Status | Notes |
|
||||
|----------|--------|-------|
|
||||
| CR-01 (prior — VAPID not wired) | **RESOLVED** | `validateSetupVapid` is imported and called in `SetupPage.tsx` lines 469-478 sequential chain. `SetupPage.test.tsx` lines 244-307 enforce it with 4 test cases. |
|
||||
| Field-name camelCase mismatch | **RESOLVED** | `postSetupConfig` sends camelCase; `setupClient.contract.test.ts` enforces all 4 field names. |
|
||||
| WR-01 (prior — orphaned row on 503) | **PRESENT** — re-filed as WR-01 above |
|
||||
| WR-02 (prior — TOCTOU on setup lock) | **PRESENT** — re-filed as WR-02 above |
|
||||
| WR-04 (prior — `appExternalUrl` no https enforcement) | **STILL PRESENT** but downgraded: `z.string().url().max(512)` at setup.ts line 66 accepts `http://` URLs. For a private self-hosted instance behind a LAN, `http://` may be intentional. No separate warning filed; operator guidance in the wizard UI ("Use the public https:// URL") would mitigate. |
|
||||
| WR-05 (prior — `process.env` permanent mutation) | **PRESENT** — re-filed as WR-03 above |
|
||||
| IN-03 (prior — OIDC error disclosure) | **PRESENT** — re-filed as IN-01 above |
|
||||
| IN-01 (prior — `/complete` no unclaimed-user check) | **PRESENT** — re-filed as IN-02 above |
|
||||
| IN-02 (prior — `generate-secrets.mjs` private import path) | **PRESENT** — re-filed as IN-03 above |
|
||||
| IN-04 (prior — email field not trimmed before send) | **PARTIALLY MITIGATED** — `handleValidate` at line 775 does not trim: `completeMutation.mutate({ fastmailEmail: email, appPassword: password })`. The `saveDisabled` guard at line 769 blocks empty-after-trim (`email.trim().length === 0`), but a non-empty value with leading/trailing spaces is sent untrimmed. The server's `z.string().email()` rejects `" user@fastmail.com"` (Zod does not auto-trim before email validation), so the user gets a generic "Invalid request" 400 with no explanation. Fix: `fastmailEmail: email.trim()` at line 775. |
|
||||
|
||||
---
|
||||
|
||||
_Reviewed: 2026-06-15T20:07:55Z_
|
||||
_Reviewed: 2026-06-15T00:00:00Z_
|
||||
_Reviewer: Claude (gsd-code-reviewer)_
|
||||
_Depth: standard_
|
||||
|
||||
Reference in New Issue
Block a user