fix(12): CR-01 guard effective-config branch during wizard in-progress

isSetupLocked() now checks for an unclaimed local wizard user
(oidcIss IS NULL, claimed=false) before firing the effective-config
branch. During the credential→complete window, this sentinel prevents
a production container with VAPID env set from blocking POST /complete
with 423. The explicit setup_complete flag (Check 1) still locks
unconditionally once written. Adds regression test that sets VAPID env
explicitly (no beforeEach clearing) to reproduce the production scenario.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This commit is contained in:
Lucas Berger
2026-06-15 16:36:52 -04:00
co-authored by Claude Sonnet 4.6
parent ed4e64a06a
commit 61a869ca7d
2 changed files with 81 additions and 5 deletions
+29 -5
View File
@@ -11,20 +11,33 @@
* event-loop tick. The per-call freshness pattern mirrors db.select() in health.ts. * event-loop tick. The per-call freshness pattern mirrors db.select() in health.ts.
* *
* Two-branch lock logic (D-10): * 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 * 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 * Real implementation (Plan 02): reads app_config.setup_complete + checks
* member_credentials + VAPID env (effective-config branch, D-10). * 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 { db } from '../db/client.js';
import { appConfig, memberCredentials } from '../db/schema.js'; import { appConfig, memberCredentials, users } from '../db/schema.js';
import { eq } from 'drizzle-orm'; import { eq, isNull, and } from 'drizzle-orm';
/** Returns true if the wizard is already locked. Re-evaluated fresh — NEVER cache at module level. */ /** Returns true if the wizard is already locked. Re-evaluated fresh — NEVER cache at module level. */
export async function isSetupLocked(): Promise<boolean> { export async function isSetupLocked(): Promise<boolean> {
// 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 const [flagRow] = await db
.select({ value: appConfig.value }) .select({ value: appConfig.value })
.from(appConfig) .from(appConfig)
@@ -32,7 +45,18 @@ export async function isSetupLocked(): Promise<boolean> {
.limit(1); .limit(1);
if (flagRow?.value === 'true') return true; 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 [credRow] = await db.select({ id: memberCredentials.id }).from(memberCredentials).limit(1);
const vapidPresent = !!process.env.VAPID_PRIVATE_KEY && !!process.env.VAPID_PUBLIC_KEY; const vapidPresent = !!process.env.VAPID_PRIVATE_KEY && !!process.env.VAPID_PUBLIC_KEY;
return !!credRow && vapidPresent; return !!credRow && vapidPresent;
+52
View File
@@ -306,6 +306,21 @@ describe('POST /api/setup/config', () => {
); );
expect(res.status).toBe(400); 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 // Not locked — no credentials means effective-config condition is false
expect(res.status).not.toBe(423); 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);
});
}); });