diff --git a/apps/api/src/lib/setupGuard.ts b/apps/api/src/lib/setupGuard.ts index b7ac3d4..9f24fb1 100644 --- a/apps/api/src/lib/setupGuard.ts +++ b/apps/api/src/lib/setupGuard.ts @@ -11,20 +11,33 @@ * event-loop tick. The per-call freshness pattern mirrors db.select() in health.ts. * * Two-branch lock logic (D-10): - * 1. Explicit: app_config.setup_complete === 'true' + * 1. Explicit: app_config.setup_complete === 'true' (unconditional — lock once written) * 2. Effective: a member_credentials row exists AND VAPID_PRIVATE_KEY + VAPID_PUBLIC_KEY env set + * EXCEPT when an unclaimed local wizard user exists (oidcIss IS NULL, claimed=false), + * which means the wizard is still in-progress and /complete has not yet run. * * Real implementation (Plan 02): reads app_config.setup_complete + checks * member_credentials + VAPID env (effective-config branch, D-10). + * + * CR-01 fix: the effective-config branch must not fire while the wizard is in-progress. + * After POST /credential writes a member_credentials row but before POST /complete writes + * setup_complete, any production container with VAPID env set would have the effective-config + * branch return true, permanently blocking /complete. The unclaimed-user sentinel prevents this: + * - During wizard (credential written, /complete not yet called): unclaimed user exists → false + * - After /complete: setup_complete='true' → Check 1 locks (unconditional) + * - Post-setup first OIDC login: first-login-claims sets claimed=true → no unclaimed user → + * effective-config branch fires correctly for legacy-recovery (no setup_complete key) instances + * - Env-only configured instance (no wizard): no wizard-created unclaimed user → + * effective-config lock fires as expected */ import { db } from '../db/client.js'; -import { appConfig, memberCredentials } from '../db/schema.js'; -import { eq } from 'drizzle-orm'; +import { appConfig, memberCredentials, users } from '../db/schema.js'; +import { eq, isNull, and } from 'drizzle-orm'; /** Returns true if the wizard is already locked. Re-evaluated fresh — NEVER cache at module level. */ export async function isSetupLocked(): Promise { - // Check 1: explicit setup_complete flag in app_config + // Check 1: explicit setup_complete flag in app_config (unconditional — always locks once written) const [flagRow] = await db .select({ value: appConfig.value }) .from(appConfig) @@ -32,7 +45,18 @@ export async function isSetupLocked(): Promise { .limit(1); if (flagRow?.value === 'true') return true; - // Check 2: effective configuration — member_credentials row exists AND VAPID env set (D-10) + // Check 2: effective-config (legacy recovery for pre-Phase-12 instances that have no + // setup_complete key but are fully configured). Only applies when the wizard is NOT + // in-progress. An unclaimed local user (oidcIss IS NULL, claimed=false) is the definitive + // "wizard still in-progress" signal: /credential creates it before /complete is called, + // and first-login-claims sets claimed=true after the first OIDC login post-setup. + const [unclaimedRow] = await db + .select({ id: users.id }) + .from(users) + .where(and(isNull(users.oidcIss), eq(users.claimed, false))) + .limit(1); + if (unclaimedRow) return false; // wizard in-progress — never effective-lock + const [credRow] = await db.select({ id: memberCredentials.id }).from(memberCredentials).limit(1); const vapidPresent = !!process.env.VAPID_PRIVATE_KEY && !!process.env.VAPID_PUBLIC_KEY; return !!credRow && vapidPresent; diff --git a/apps/api/tests/routes/setup.test.ts b/apps/api/tests/routes/setup.test.ts index bce06f1..ebe0fc5 100644 --- a/apps/api/tests/routes/setup.test.ts +++ b/apps/api/tests/routes/setup.test.ts @@ -306,6 +306,21 @@ describe('POST /api/setup/config', () => { ); expect(res.status).toBe(400); }); + + // IN-01: appExternalUrl must also require https:// — it is injected as OIDC_AUTH_EXTERNAL_URL + // (the redirect URI base) and Authelia rejects non-https redirect URIs in production. + it('IN-01: returns 400 when appExternalUrl is an http:// URL (must require https)', async () => { + const app = await getApp(); + const res = await app.fetch( + jsonRequest('POST', '/api/setup/config', { + oidcIssuer: 'https://auth.example.com', + oidcClientId: 'familysync-client', + vapidPublicKey: VAPID_PUBLIC_KEY, + appExternalUrl: 'http://insecure-app.example.com', + }), + ); + expect(res.status).toBe(400); + }); }); // =========================================================================== @@ -554,4 +569,41 @@ describe('/api/setup/* — 423 when effectively configured (D-10 effective-confi // Not locked — no credentials means effective-config condition is false expect(res.status).not.toBe(423); }); + + // CR-01 regression: reproduces the production lock-out scenario. + // With VAPID env PRESENT, after /credential creates an unclaimed user + credential row, + // POST /complete must still succeed (200, writes setup_complete). Only AFTER /complete + // runs does isSetupLocked() return true (via Check 1 / explicit flag) — so a second + // /complete call returns 423. + // + // The bug was that the effective-config branch (credRow + vapidPresent) fired during + // the credential→complete window, blocking /complete with 423 permanently. + // beforeEach masks this by clearing VAPID env — so we set it explicitly here. + it('CR-01: /complete succeeds when VAPID env is set AND unclaimed wizard user+credential exist (in-progress wizard)', async () => { + // Explicitly set VAPID env (do NOT rely on beforeEach clearing it) + process.env.VAPID_PRIVATE_KEY = VAPID_PRIVATE_KEY; + process.env.VAPID_PUBLIC_KEY = VAPID_PUBLIC_KEY; + + // Simulate POST /credential: creates unclaimed local user + credential (wizard in-progress) + const userId = await seedLocalUser('cr01-regression'); + await seedCredential(userId); + + const app = await getApp(); + + // /complete must succeed (200) — effective-config lock must NOT fire while wizard in-progress + const completeRes = await app.fetch(jsonRequest('POST', '/api/setup/complete')); + expect(completeRes.status).toBe(200); + + // Verify setup_complete was written to app_config + const [flagRow] = await db + .select({ value: appConfig.value }) + .from(appConfig) + .where(eq(appConfig.key, 'setup_complete')) + .limit(1); + expect(flagRow?.value).toBe('true'); + + // Second /complete must return 423 — explicit setup_complete flag now locks unconditionally + const secondRes = await app.fetch(jsonRequest('POST', '/api/setup/complete')); + expect(secondRes.status).toBe(423); + }); });