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

docs(12): add code review report

Browse Source
This commit is contained in:
Lucas Berger
2026-06-15 16:11:50 -04:00
parent 836cb38934
commit ef3e9810a9
1 changed files with 161 additions and 301 deletions
Download Patch File Download Diff File Expand all files Collapse all files
.planning/phases/12-initial-setup-wizard/12-REVIEW.md
+160 -300
View File
@@ -1,129 +1,112 @@
--- ---
phase: 12-initial-setup-wizard phase: 12-initial-setup-wizard
reviewed: 2026-06-15T00:00:00Z reviewed: 2026-06-15T20:07:55Z
depth: standard depth: standard
files_reviewed: 12 files_reviewed: 16
files_reviewed_list: 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/auth/middleware.ts
- apps/api/src/db/schema.ts - apps/api/src/auth/user.ts
- apps/api/src/db/migrations/0002_lethal_millenium_guard.sql - apps/api/src/db/migrations/0002_lethal_millenium_guard.sql
- scripts/generate-secrets.mjs - apps/api/src/db/schema.ts
- apps/pwa/src/api/client.ts - apps/api/src/index.ts
- apps/pwa/src/routes/SetupPage.tsx - apps/api/src/lib/setupGuard.ts
- apps/pwa/src/App.tsx - apps/api/src/routes/setup.ts
- apps/api/tests/auth/user.test.ts
- apps/api/tests/routes/setup.test.ts - apps/api/tests/routes/setup.test.ts
- apps/pwa/src/api/client.ts
- apps/pwa/src/api/setupClient.contract.test.ts
- apps/pwa/src/App.test.tsx
- apps/pwa/src/App.tsx
- apps/pwa/src/routes/SetupPage.test.tsx
- apps/pwa/src/routes/SetupPage.tsx
- scripts/generate-secrets.mjs
findings: findings:
critical: 1 critical: 1
warning: 5 warning: 3
info: 4 info: 3
total: 10 total: 7
status: issues_found status: issues_found
--- ---
# Phase 12: Code Review Report # Phase 12: Code Review Report
**Reviewed:** 2026-06-15 **Reviewed:** 2026-06-15T20:07:55Z
**Depth:** standard **Depth:** standard
**Files Reviewed:** 12 **Files Reviewed:** 16
**Status:** issues_found **Status:** issues_found
## Summary ## Summary
Phase 12 adds the first-run setup wizard: a pre-auth `/api/setup/*` route surface, a 423 lock 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.
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 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.
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 The remaining prior items (WR-04, IN-01, IN-02, IN-03, IN-04) are re-evaluated in the disposition table at the end.
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.
--- ---
## Structural Findings (fallow)
No structural pre-pass was provided for this review.
---
## Narrative Findings (AI reviewer)
## Critical Issues ## Critical Issues
### CR-01: VAPID Validation Never Called During Wizard Flow ### CR-01: `postSetupCredential` sends an undeclared `providerType` field — no contract test guards drift
**File:** `apps/pwa/src/routes/SetupPage.tsx:27-34` and `apps/pwa/src/routes/SetupPage.tsx:452-487` **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:
**Issue:** `validateSetupVapid` is exported from `client.ts` and has full backend ```ts
implementation (`POST /api/setup/validate/vapid`) but is never imported or called in const credentialSchema = z.object({
`SetupPage.tsx`. Step 2 calls only `validateSetupDb` then `validateSetupOidc`. The VAPID fastmailEmail: z.string().email().max(256),
structural check (`webpush.setVapidDetails()` 32-byte / 65-byte decode) is entirely skipped. appPassword: z.string().min(1).max(500),
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.');
}
``` ```
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 ## Warnings
### WR-01: Orphaned User Row When Post-Insert Re-Select Returns Nothing ### WR-01: Orphaned unclaimed user row when the post-insert re-select returns nothing (503 path)
**File:** `apps/api/src/routes/setup.ts:259-279` **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:
**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);
```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) { if (!localUser) {
// Roll back the orphaned insert — re-select missed it (transient or race)
await db.delete(users).where(eq(users.id, inserted.id)); await db.delete(users).where(eq(users.id, inserted.id));
return c.json({ error: 'Service unavailable' }, 503); return c.json({ error: 'Service unavailable' }, 503);
} }
@@ -131,256 +114,133 @@ if (!localUser) {
--- ---
### WR-02: Concurrent `/credential` Calls Create Multiple Unclaimed Admin Users ### 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` **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.
**Issue:** The guard (`isSetupLocked`) returns false until `setup_complete='true'` is set OR a 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.
`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 **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:
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 ```ts
transaction or application-level uniqueness check preventing it. // Serialise concurrent credential writes — at most one unclaimed admin may exist
await db.transaction(async (tx) => {
**Fix (simplest):** Add a uniqueness check before inserting the local user. Query for existing const [{ count }] = await tx.execute(sql`SELECT COUNT(*) as count FROM users WHERE claimed = false FOR UPDATE`);
unclaimed users and return a 409 (or reuse the existing one) if found: if (Number(count) > 0) {
throw new Error('An unclaimed user already exists');
```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, ...)
} }
``` // ... proceed with insert
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) ### WR-03: `oidcConfigFallbackMiddleware` permanently mutates `process.env` on first request — DB changes after that are invisible until restart
**File:** `apps/api/src/routes/setup.ts:66` **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.
**Issue:** The `configSchema` validates `oidcIssuer` with an explicit 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.
`.refine((v) => v.startsWith('https://'), ...)` but `appExternalUrl` only uses `.url()` with
no scheme restriction:
```typescript **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:
appExternalUrl: z.string().url().max(512), // accepts http://
```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;
}
``` ```
`appExternalUrl` is stored in `app_config` as `app_external_url` and consumed by **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.
`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 ## Info
### IN-01: `POST /api/setup/complete` Has No Prerequisite Validation ### IN-01: `POST /api/setup/validate/oidc` leaks internal network error details to the pre-auth caller
**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` **File:** `apps/api/src/routes/setup.ts:181-186`
**Issue:** The error response includes the raw exception message:
**Issue:** The OIDC validation error response includes the raw error message: ```ts
error: 'OIDC discovery failed: ' + (err instanceof Error ? err.message : String(err)),
```typescript
error: 'OIDC discovery failed: ' + (err instanceof Error ? err.message : String(err))
``` ```
On a misconfigured or unreachable Authelia instance, this returns messages such as: 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.
`"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:** Log the raw error server-side only, return a generic string:
**Fix:** Return a generic message and log the detail server-side: ```ts
```typescript
} catch (err) { } catch (err) {
console.error('[setup/validate/oidc] Discovery failed:', err instanceof Error ? err.message : String(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 and that Authelia is reachable from the server.' }, 400); return c.json({ ok: false, error: 'OIDC discovery failed. Check the issuer URL.' }, 400);
} }
``` ```
--- ---
### IN-04: Email Field Not Trimmed Before Sending in Credential Submission ### IN-02: `POST /api/setup/complete` has no prerequisite check for an existing unclaimed user + credential
**File:** `apps/pwa/src/routes/SetupPage.tsx:742-748` **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.
**Issue:** `saveDisabled` checks `email.trim().length === 0` (line 742) but the email is sent **Fix:** Before setting `setup_complete`, verify an unclaimed user with a credential exists:
to the API without trimming (line 748):
```typescript ```ts
completeMutation.mutate({ fastmailEmail: email, appPassword: password }); 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);
}
``` ```
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_ ### 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_
_Reviewer: Claude (gsd-code-reviewer)_ _Reviewer: Claude (gsd-code-reviewer)_
_Depth: standard_ _Depth: standard_