14 KiB
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-15T20:07:55Z | standard | 16 |
|
|
issues_found |
Phase 12: Code Review Report
Reviewed: 2026-06-15T20:07:55Z Depth: standard Files Reviewed: 16 Status: issues_found
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.
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.
The remaining prior items (WR-04, IN-01, IN-02, IN-03, IN-04) are re-evaluated in the disposition table at the end.
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:
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:
- If
credentialSchemais ever tightened with.strict()for defence-in-depth, this extra field triggers a 400 that surfaces to the user as the genericnoEchoHook"Invalid request" with no diagnostic path. - 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. - The
postSetupConfigcamelCase regression is covered by a dedicated contract test (setupClient.contract.test.ts). No equivalent test exists forpostSetupCredential— 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):
// 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:
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:
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:
// 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:
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:
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:
} 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:
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:
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:
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 Reviewer: Claude (gsd-code-reviewer) Depth: standard