fix(12): WR-01 narrow TOCTOU guard and set claimed=true for OIDC inserts
- apps/api/src/auth/user.ts: upsertUser step-5 insert now sets claimed=true for all OIDC-created users. An identity-bound OIDC user is never a pending wizard bootstrap user; explicit claimed=true prevents ambiguity with the (oidcIss IS NULL AND claimed=false) sentinel used by the TOCTOU guard and isSetupLocked. First-login-claims path is unaffected (it updates a pre-existing oidcIss=null row; this change only touches the fresh insert). - apps/api/src/routes/setup.ts: TOCTOU guard in POST /credential now queries WHERE oidc_iss IS NULL AND claimed = false FOR UPDATE, matching the exact definition of a pending wizard bootstrap user. This provides defense-in-depth against any future path that could produce claimed=false OIDC rows. - apps/api/tests/auth/user.test.ts: new WR-01 test asserts that the fresh OIDC insert sets claimed=true in the values passed to db.insert(). - apps/api/tests/routes/setup.test.ts: new WR-01 integration test seeds an OIDC user with claimed=false (oidcIss NOT NULL) and verifies POST /credential still succeeds (guard ignores the OIDC row, only counts local wizard rows). All 402 API tests, 253 PWA tests, and typecheck pass. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Sonnet 4.6
parent
22d1581484
commit
687f9dc9fa
@@ -156,6 +156,11 @@ export async function upsertUser(oidcIss: string, oidcSub: string, displayName?:
|
|||||||
const shouldBeAdmin = flagRow?.value !== 'true' && Number(count) === 0;
|
const shouldBeAdmin = flagRow?.value !== 'true' && Number(count) === 0;
|
||||||
|
|
||||||
// 5. Insert new user row
|
// 5. Insert new user row
|
||||||
|
// claimed=true: an OIDC-created user is identity-bound at insert time and is
|
||||||
|
// never a pending wizard bootstrap user. Setting this explicitly prevents the
|
||||||
|
// TOCTOU guard in POST /credential (which counts WHERE oidc_iss IS NULL AND
|
||||||
|
// claimed = false) from ever treating a fresh OIDC insert as an unclaimed
|
||||||
|
// wizard row (WR-01).
|
||||||
// mysql2 has no RETURNING clause — use $returningId() then re-select
|
// mysql2 has no RETURNING clause — use $returningId() then re-select
|
||||||
const [inserted] = await db
|
const [inserted] = await db
|
||||||
.insert(users)
|
.insert(users)
|
||||||
@@ -165,6 +170,7 @@ export async function upsertUser(oidcIss: string, oidcSub: string, displayName?:
|
|||||||
displayName: displayName ?? null,
|
displayName: displayName ?? null,
|
||||||
color,
|
color,
|
||||||
isAdmin: shouldBeAdmin,
|
isAdmin: shouldBeAdmin,
|
||||||
|
claimed: true,
|
||||||
})
|
})
|
||||||
.$returningId();
|
.$returningId();
|
||||||
|
|
||||||
|
|||||||
@@ -259,11 +259,15 @@ setupRouter.post('/credential', zValidator('json', credentialSchema, noEchoHook)
|
|||||||
let localUser: typeof users.$inferSelect | undefined;
|
let localUser: typeof users.$inferSelect | undefined;
|
||||||
try {
|
try {
|
||||||
localUser = await db.transaction(async (tx) => {
|
localUser = await db.transaction(async (tx) => {
|
||||||
// Serialise: at most one unclaimed admin row may exist (WR-02).
|
// Serialise: at most one unclaimed wizard bootstrap row may exist (WR-02).
|
||||||
|
// The filter is (oidc_iss IS NULL AND claimed = false) — the precise definition
|
||||||
|
// of a "pending wizard bootstrap user" — so that OIDC-created rows (which are
|
||||||
|
// born with claimed=true after WR-01 fix, but could theoretically be claimed=false
|
||||||
|
// on legacy data) are never counted here (WR-01 defense-in-depth).
|
||||||
// Cast through unknown — Drizzle mysql2 execute() returns [rows, fields] for SELECTs;
|
// Cast through unknown — Drizzle mysql2 execute() returns [rows, fields] for SELECTs;
|
||||||
// the generic type parameter on execute() is not sufficient to type the result correctly.
|
// the generic type parameter on execute() is not sufficient to type the result correctly.
|
||||||
const countRows = (await tx.execute(
|
const countRows = (await tx.execute(
|
||||||
sql`SELECT COUNT(*) AS count FROM users WHERE claimed = false FOR UPDATE`,
|
sql`SELECT COUNT(*) AS count FROM users WHERE oidc_iss IS NULL AND claimed = false FOR UPDATE`,
|
||||||
)) as unknown as [{ count: string | number }[], unknown];
|
)) as unknown as [{ count: string | number }[], unknown];
|
||||||
const unclaimedCount = Number(countRows[0][0]?.count ?? 0);
|
const unclaimedCount = Number(countRows[0][0]?.count ?? 0);
|
||||||
if (unclaimedCount > 0) {
|
if (unclaimedCount > 0) {
|
||||||
|
|||||||
@@ -441,6 +441,55 @@ describe('upsertUser', () => {
|
|||||||
// select must only have been called once (identity lookup, then early-return)
|
// select must only have been called once (identity lookup, then early-return)
|
||||||
expect(mockDb.select).toHaveBeenCalledTimes(1);
|
expect(mockDb.select).toHaveBeenCalledTimes(1);
|
||||||
});
|
});
|
||||||
|
|
||||||
|
// WR-01: upsertUser's fresh OIDC insert must set claimed=true.
|
||||||
|
// An OIDC-created user is identity-bound at insert time and must NOT be born
|
||||||
|
// with claimed=false, which would make it indistinguishable from a pending
|
||||||
|
// wizard bootstrap user (oidcIss IS NULL AND claimed=false) in the TOCTOU guard.
|
||||||
|
it('WR-01: fresh OIDC insert sets claimed=true (OIDC user is never a pending wizard user)', async () => {
|
||||||
|
const iss = 'https://auth.example.com';
|
||||||
|
const sub = 'sub-wr01-claimed';
|
||||||
|
|
||||||
|
let selectCallCount = 0;
|
||||||
|
mockDb.select.mockImplementation(() => {
|
||||||
|
selectCallCount++;
|
||||||
|
if (selectCallCount === 1) return makeSelectChain([]); // identity lookup — not found
|
||||||
|
if (selectCallCount === 2) return makeSelectChain([]); // setup_complete — not set
|
||||||
|
if (selectCallCount === 3) {
|
||||||
|
// Used-colors query
|
||||||
|
return { from: vi.fn().mockResolvedValue([]) };
|
||||||
|
}
|
||||||
|
if (selectCallCount === 4) {
|
||||||
|
// Admin COUNT
|
||||||
|
return makeSelectChain([{ count: 0 }]);
|
||||||
|
}
|
||||||
|
// Re-fetch after insert
|
||||||
|
return makeSelectChain([
|
||||||
|
{
|
||||||
|
id: 20,
|
||||||
|
oidcIss: iss,
|
||||||
|
oidcSub: sub,
|
||||||
|
displayName: null,
|
||||||
|
color: COLOR_PALETTE[0],
|
||||||
|
isAdmin: true,
|
||||||
|
claimed: true,
|
||||||
|
createdAt: new Date(),
|
||||||
|
},
|
||||||
|
]);
|
||||||
|
});
|
||||||
|
|
||||||
|
mockDb.insert.mockReturnValue(makeInsertChain([{ id: 20 }]));
|
||||||
|
|
||||||
|
await upsertUser(iss, sub);
|
||||||
|
|
||||||
|
// The insert values must include claimed: true
|
||||||
|
const insertValues = mockDb.insert.mock.results[0]?.value?.values.mock.calls[0]?.[0];
|
||||||
|
expect(insertValues).toBeDefined();
|
||||||
|
expect(insertValues.claimed).toBe(true);
|
||||||
|
// And it must carry a real oidcIss (not null — distinguishable from wizard row)
|
||||||
|
expect(insertValues.oidcIss).toBe(iss);
|
||||||
|
expect(insertValues.oidcSub).toBe(sub);
|
||||||
|
});
|
||||||
});
|
});
|
||||||
|
|
||||||
// ── Plan 12-03: D-08 first-login-claims (Wave-2 GREEN — full test implementations) ──
|
// ── Plan 12-03: D-08 first-login-claims (Wave-2 GREEN — full test implementations) ──
|
||||||
|
|||||||
@@ -478,6 +478,39 @@ describe('POST /api/setup/credential', () => {
|
|||||||
const body = JSON.parse(bodyText) as { error: string };
|
const body = JSON.parse(bodyText) as { error: string };
|
||||||
expect(body.error).toBe('Invalid request');
|
expect(body.error).toBe('Invalid request');
|
||||||
});
|
});
|
||||||
|
|
||||||
|
// WR-01: TOCTOU guard must ignore OIDC users that happen to have claimed=false
|
||||||
|
// (oidcIss IS NOT NULL). Only wizard bootstrap users (oidcIss IS NULL AND claimed=false)
|
||||||
|
// must trigger the DUPLICATE_UNCLAIMED guard. This prevents a partially-bootstrapped
|
||||||
|
// instance (where an OIDC user somehow exists pre-setup) from permanently blocking
|
||||||
|
// the wizard credential step with 409.
|
||||||
|
it('WR-01: TOCTOU guard ignores claimed=false OIDC users (oidcIss NOT NULL) — credential step still succeeds', async () => {
|
||||||
|
// Seed an OIDC user with claimed=false to simulate the latent bug scenario.
|
||||||
|
// After the WR-01 fix upsertUser always inserts claimed=true for OIDC users, but
|
||||||
|
// this tests that the guard's WHERE clause is narrowed correctly for defense-in-depth.
|
||||||
|
await db.insert(users).values({
|
||||||
|
oidcIss: 'https://auth.test.setup',
|
||||||
|
oidcSub: `sub-wr01-oidc-claimed-false-${Date.now()}`,
|
||||||
|
displayName: 'OIDC User With claimed=false',
|
||||||
|
color: '#4A90D9',
|
||||||
|
isAdmin: false,
|
||||||
|
claimed: false, // legacy/hypothetical — oidcIss is NOT NULL
|
||||||
|
});
|
||||||
|
|
||||||
|
mockValidateCredentialShouldThrow = false;
|
||||||
|
|
||||||
|
const app = await getApp();
|
||||||
|
const res = await app.fetch(
|
||||||
|
jsonRequest('POST', '/api/setup/credential', {
|
||||||
|
fastmailEmail: 'operator@fastmail.com',
|
||||||
|
appPassword: 'valid-app-password-wr01',
|
||||||
|
}),
|
||||||
|
);
|
||||||
|
// Must succeed — the OIDC-with-oidcIss row must NOT block the wizard
|
||||||
|
expect(res.status).toBe(200);
|
||||||
|
const body = (await res.json()) as { ok: boolean };
|
||||||
|
expect(body.ok).toBe(true);
|
||||||
|
});
|
||||||
});
|
});
|
||||||
|
|
||||||
// ===========================================================================
|
// ===========================================================================
|
||||||
|
|||||||
Reference in New Issue
Block a user