luckberg/familysync
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

docs(12): code review — 1 critical, 5 warnings, 4 info

Browse Source 
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
Lucas Berger
2026-06-15 15:02:27 -04:00
co-authored by Claude Opus 4.8
parent d96dfae17b
commit d8b4c98592
1 changed files with 386 additions and 0 deletions
Download Patch File Download Diff File Expand all files Collapse all files
.planning/phases/12-initial-setup-wizard/12-REVIEW.md
+386
View File
@@ -0,0 +1,386 @@
---
phase: 12-initial-setup-wizard
reviewed: 2026-06-15T00:00:00Z
depth: standard
files_reviewed: 12
files_reviewed_list:
- 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
findings:
critical: 1
warning: 5
info: 4
total: 10
status: 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:
```tsx
// 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:
```typescript
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:
```typescript
// 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:
```typescript
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:
```typescript
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:**
```typescript
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):
```typescript
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:
```typescript
// 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:
```javascript
// 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:
```typescript
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:
```typescript
} 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):
```typescript
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:**
```typescript
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_