familysync/.planning/phases/12-initial-setup-wizard/12-REVIEW.md

phase, reviewed, depth, files_reviewed, files_reviewed_list, findings, status

phase	reviewed	depth	files_reviewed	files_reviewed_list	findings	status
12-initial-setup-wizard	2026-06-15T00:00:00Z	standard	12	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/db/schema.ts apps/api/src/db/migrations/0002_lethal_millenium_guard.sql scripts/generate-secrets.mjs apps/pwa/src/api/client.ts apps/pwa/src/routes/SetupPage.tsx apps/pwa/src/App.tsx apps/api/tests/routes/setup.test.ts	critical warning info total 1 5 4 10	issues_found

Phase 12: Code Review Report

Reviewed: 2026-06-15 Depth: standard Files Reviewed: 12 Status: issues_found

Summary

Phase 12 adds the first-run setup wizard: a pre-auth /api/setup/* route surface, a 423 lock guard, schema migration making oidc_iss/oidc_sub nullable with a claimed marker, first-login-claims in upsertUser, a secret-generation helper script, and a multi-step React wizard. The overall architecture is sound and the security decisions (VAPID keys env-only, noEchoHook on credential endpoint, https-only for oidcIssuer, mounting before OIDC middleware) are correctly implemented.

One blocker was found: the wizard UI skips calling validateSetupVapid entirely, so an operator with invalid or missing VAPID keys can complete setup without any runtime feedback. Five warnings cover an orphaned-row leak path, a duplicate-unclaimed-user race in the credential endpoint, a misleading test mock comment that hides missing coverage, missing appExternalUrl https enforcement, and the oidcConfigFallbackMiddleware permanently mutating process.env with no mechanism to pick up DB changes after the first request. Four info items cover minor quality gaps.

---

Critical Issues

CR-01: VAPID Validation Never Called During Wizard Flow

File: apps/pwa/src/routes/SetupPage.tsx:27-34 and apps/pwa/src/routes/SetupPage.tsx:452-487

Issue: validateSetupVapid is exported from client.ts and has full backend implementation (POST /api/setup/validate/vapid) but is never imported or called in SetupPage.tsx. Step 2 calls only validateSetupDb then validateSetupOidc. The VAPID structural check (webpush.setVapidDetails() 32-byte / 65-byte decode) is entirely skipped.

An operator whose VAPID_PRIVATE_KEY / VAPID_PUBLIC_KEY env vars are absent, corrupted, or swapped will complete the entire wizard (HTTP 200 on every step including POST /api/setup/complete) with no error, then silently fail to send any push notification. There is no other point in the flow where the VAPID pair is validated before the app goes live.

Fix: Import validateSetupVapid in SetupPage.tsx and call it as a third validation row inside Step2Config.configMutation.onSuccess, after the OIDC check. Add a ValidationRow with state={validationRows.vapid} to surface the result. Both the validationRows state type and bothPassed guard should include the vapid row:

// In import block:
import {
  validateSetupDb,
  validateSetupOidc,
  validateSetupVapid,   // add
  ...
} from '../api/client.js';

// State:
const [validationRows, setValidationRows] = useState<Pick<ValidationRowStatus, 'db' | 'oidc' | 'vapid'>>({
  db: 'idle',
  oidc: 'idle',
  vapid: 'idle',
});

// In onSuccess after OIDC passes:
try {
  await validateSetupVapid();
  setValidationRows({ db: 'success', oidc: 'success', vapid: 'success' });
  setBothPassed(true);   // rename to allPassed if preferred
} catch (vapidErr) {
  setValidationRows((prev) => ({ ...prev, vapid: 'failure' }));
  setFieldError('VAPID validation failed. Check that VAPID_PRIVATE_KEY and VAPID_PUBLIC_KEY are set in your Docker environment.');
}

---

Warnings

WR-01: Orphaned User Row When Post-Insert Re-Select Returns Nothing

File: apps/api/src/routes/setup.ts:259-279

Issue: The /credential handler inserts the local user (lines 259-269), then re-selects by inserted.id (lines 271-275) to get the full typed row. If the re-select returns nothing (race/transient DB issue), the handler returns 503 at line 278 but never deletes the orphaned inserted row. The catch block at line 292 only executes when validateEncryptAndStoreCredential throws, not when the re-select fails.

An orphaned unclaimed row means isSetupLocked()'s effective-config check will not catch it (no member_credentials row), and a subsequent /credential call will insert a second unclaimed user. With two unclaimed users in the DB, first-login-claims in upsertUser will claim the first one it finds (nondeterministic by insert order without ORDER BY).

Fix: Roll back the insert when the re-select returns nothing:

const [localUser] = await db.select().from(users).where(eq(users.id, inserted.id)).limit(1);

if (!localUser) {
  // Roll back the orphaned insert — re-select missed it (transient or race)
  await db.delete(users).where(eq(users.id, inserted.id));
  return c.json({ error: 'Service unavailable' }, 503);
}

---

WR-02: Concurrent /credential Calls Create Multiple Unclaimed Admin Users

File: apps/api/src/routes/setup.ts:243-307

Issue: The guard (isSetupLocked) returns false until setup_complete='true' is set OR a member_credentials row exists. Two concurrent POST /api/setup/credential requests both pass isSetupLocked() (no credential rows yet), both insert a local user row with oidcIss=null, claimed=false, isAdmin=true, and both call validateEncryptAndStoreCredential with their respective localUser.id. The member_credentials table has UNIQUE(user_id) — but since the two users have different id values, both inserts succeed. The result is two unclaimed admin users in the DB.

first-login-claims in upsertUser uses LIMIT 1 with no ORDER BY, so which of the two unclaimed users gets claimed by the first OIDC login is non-deterministic. The second unclaimed user remains in the DB forever as an orphaned admin row.

For a self-hosted two-person household app this is a low-probability race. But there is no DB transaction or application-level uniqueness check preventing it.

Fix (simplest): Add a uniqueness check before inserting the local user. Query for existing unclaimed users and return a 409 (or reuse the existing one) if found:

// Before inserting the local user:
const [existingUnclaimed] = await db
  .select()
  .from(users)
  .where(and(isNull(users.oidcIss), eq(users.claimed, false)))
  .limit(1);

if (existingUnclaimed) {
  // Reuse the existing unclaimed local user row instead of creating a duplicate.
  // A prior /credential call created it; this call just re-validates and re-stores
  // the credential (onDuplicateKeyUpdate in validateEncryptAndStoreCredential handles this).
  const localUser = existingUnclaimed;
  // ... proceed to validateEncryptAndStoreCredential(localUser.id, ...)
}

Alternatively, wrap the insert + credential store in a DB transaction and add a table-level advisory lock, but the check-before-insert approach is sufficient for a 2-person household app.

---

WR-03: Misleading Test Mock Comment Hides Missing Coverage for Duplicate Credential Calls

File: apps/api/tests/routes/setup.test.ts:52-55

Issue: The mock for validateEncryptAndStoreCredential has a comment: "Success: write a mock credential row so the guard's effective-config check works" (line 53). The mock does NOT write any row — it returns undefined. After a successful /credential call in tests, the member_credentials table is empty. This means isSetupLocked()'s effective-config check (credential row + VAPID env) does not trigger on a second /credential call in the test environment — even though it would trigger in production once validateEncryptAndStoreCredential has actually run.

As a direct consequence, there is no test asserting that a second call to /credential returns 423 (the effective-config branch). The test suite has no coverage for the duplicate call scenario described in WR-02 above.

Fix: Either (a) have the mock write a real member_credentials row, or (b) add an explicit test:

it('returns 423 on a second /credential call when member_credentials row exists and VAPID env set', async () => {
  // First call: let validateEncryptAndStoreCredential mock write a real credential row
  // ... seed user + credential row + set VAPID env, then:
  const res = await app.fetch(jsonRequest('POST', '/api/setup/credential', { ... }));
  expect(res.status).toBe(423);
});

Also correct the comment to accurately describe what the mock does.

---

WR-04: appExternalUrl Accepts http:// URLs (No HTTPS Enforcement)

File: apps/api/src/routes/setup.ts:66

Issue: The configSchema validates oidcIssuer with an explicit .refine((v) => v.startsWith('https://'), ...) but appExternalUrl only uses .url() with no scheme restriction:

appExternalUrl: z.string().url().max(512),  // accepts http://

appExternalUrl is stored in app_config as app_external_url and consumed by oidcConfigFallbackMiddleware as OIDC_AUTH_EXTERNAL_URL. An http:// app URL means the OIDC redirect_uri will be http://…/callback. Many OIDC providers (including Authelia) reject non-HTTPS redirect URIs in production. An operator who accidentally enters http:// instead of https:// will complete setup successfully but then fail every OIDC login with a cryptic Authelia error.

Fix:

appExternalUrl: z
  .string()
  .url()
  .max(512)
  .refine((v) => v.startsWith('https://'), { message: 'appExternalUrl must be an https URL' }),

---

WR-05: oidcConfigFallbackMiddleware Permanently Mutates process.env With No Update Path

File: apps/api/src/auth/middleware.ts:63-93

Issue: oidcConfigFallbackMiddleware sets process.env.OIDC_ISSUER, process.env.OIDC_CLIENT_ID, and process.env.OIDC_AUTH_EXTERNAL_URL the first time a request arrives without those env vars set. Once written, needsIssuer = !process.env.OIDC_ISSUER evaluates to false on every subsequent request — the DB is never queried again for the lifetime of the process.

If the wizard is somehow re-run (DB edited to clear setup_complete, new values written to app_config), the old values remain in process.env and are never replaced until the container restarts. While re-running the wizard is not a supported workflow, this also creates a subtle silent failure mode during initial development and testing: a test that sets process.env.OIDC_ISSUER='' to simulate "no env var" will not work once another test or import has already caused the middleware to populate the var.

Fix: The simplest mitigation is to document the behavior explicitly and add a guard that re-reads from DB if the env var is an empty string (not just absent):

const needsIssuer = !process.env.OIDC_ISSUER || process.env.OIDC_ISSUER === '';

A stronger fix would avoid mutating process.env globally and instead read from a request-scoped context. But given that @hono/oidc-auth reads from process.env directly and a container restart always clears the mutations, the documentation + empty-string guard is sufficient for this household app.

---

Info

IN-01: POST /api/setup/complete Has No Prerequisite Validation

File: apps/api/src/routes/setup.ts:317-327

Issue: An operator can call POST /api/setup/complete without first completing POST /api/setup/credential. This writes setup_complete='true' to app_config with no unclaimed local user in the DB. On first OIDC login, upsertUser finds no unclaimed user to claim, falls through to the normal insert path, and sets shouldBeAdmin = false because flagRow?.value === 'true' (setup complete, so admin bootstrap is blocked). The first login creates a non-admin user; there is no way to recover admin access without a DB edit.

This requires deliberate misuse of the wizard (calling /complete while bypassing the UI), so it is not a realistic user scenario. The fix is to add a prerequisite check in /complete that verifies an unclaimed local user with a credential exists before locking:

// Before writing setup_complete:
const [unclaimed] = await db
  .select({ id: users.id })
  .from(users)
  .where(and(isNull(users.oidcIss), eq(users.claimed, false)))
  .limit(1);
if (!unclaimed) {
  return c.json({ error: 'No credential configured — complete the credential step first' }, 400);
}

---

IN-02: generate-secrets.mjs Imports Web-Push Source File Directly

File: scripts/generate-secrets.mjs:23

Issue: The script imports '../apps/api/node_modules/web-push/src/index.js' — the package's source file — rather than the package entry point. This bypasses the package.json "main" field and breaks if web-push restructures its src/ directory in a future update. The src/index.js path is a private implementation detail, not a stable API surface.

This works today (verified the file exists and exports generateVAPIDKeys), but it is brittle.

Fix: Import the package by name from the workspace root:

// Option A: if web-push is hoisted or linked via pnpm workspace
import { generateVAPIDKeys } from 'web-push';

// Option B: explicit workspace resolution (more robust across pnpm hoist strategies)
import { createRequire } from 'node:module';
const require = createRequire(import.meta.url);
const { generateVAPIDKeys } = require('../apps/api/node_modules/web-push');

---

IN-03: validate/oidc Endpoint Leaks Internal Network Error Details

File: apps/api/src/routes/setup.ts:181-186

Issue: The OIDC validation error response includes the raw error message:

error: 'OIDC discovery failed: ' + (err instanceof Error ? err.message : String(err))

On a misconfigured or unreachable Authelia instance, this returns messages such as: "OIDC discovery failed: connect ECONNREFUSED 10.0.0.5:9091" to the pre-auth client. While this is a self-hosted operator-facing tool where the context is appropriate, it exposes internal network topology (private IPs, port numbers) to the browser before authentication.

This is a bounded info item for a self-hosted app; the risk is low but the fix is trivial.

Fix: Return a generic message and log the detail server-side:

} catch (err) {
  console.error('[setup/validate/oidc] Discovery failed:', err instanceof Error ? err.message : String(err));
  return c.json({ ok: false, error: 'OIDC discovery failed. Check the issuer URL and that Authelia is reachable from the server.' }, 400);
}

---

IN-04: Email Field Not Trimmed Before Sending in Credential Submission

File: apps/pwa/src/routes/SetupPage.tsx:742-748

Issue: saveDisabled checks email.trim().length === 0 (line 742) but the email is sent to the API without trimming (line 748):

completeMutation.mutate({ fastmailEmail: email, appPassword: password });

A trailing/leading space in the email field passes the non-empty check but is sent verbatim to validateEncryptAndStoreCredential, which passes it to createFastmailClient. Fastmail CalDAV will reject the username user@fastmail.com (trailing space) with an authentication error. The operator sees a generic "CalDAV validation failed" message and must diagnose the trailing space themselves.

Fix:

completeMutation.mutate({ fastmailEmail: email.trim(), appPassword: password });

Note: appPassword should deliberately NOT be trimmed — some generated passwords could theoretically start/end with specific characters.

---

Reviewed: 2026-06-15 Reviewer: Claude (gsd-code-reviewer) Depth: standard