fix(12): WR-02 serialise concurrent /credential calls with FOR UPDATE transaction
Two concurrent POST /api/setup/credential requests could both pass isSetupLocked(), observe no unclaimed row, and both insert — leaving two unclaimed admin rows with no recovery path. Wrap the count-check + user insert in a transaction with SELECT COUNT(*) ... FOR UPDATE to acquire a row/gap lock, ensuring at most one unclaimed admin row is created. Returns 409 when a concurrent request already holds an unclaimed row. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Sonnet 4.6
parent
3bc38bf1a6
commit
a4e0ea4f14
@@ -247,8 +247,24 @@ setupRouter.post('/credential', zValidator('json', credentialSchema, noEchoHook)
|
|||||||
const { fastmailEmail, appPassword } = c.req.valid('json');
|
const { fastmailEmail, appPassword } = c.req.valid('json');
|
||||||
// T-12-05: NEVER log appPassword or c.req.valid('json') here
|
// T-12-05: NEVER log appPassword or c.req.valid('json') here
|
||||||
|
|
||||||
|
// Steps 1 + 2 run inside a serialising transaction (WR-02 TOCTOU fix).
|
||||||
|
// SELECT … FOR UPDATE on the unclaimed-user count acquires a row/gap lock so that
|
||||||
|
// two concurrent credential requests cannot both observe count=0 and both insert.
|
||||||
|
// The transaction commits before validateEncryptAndStoreCredential (which does its
|
||||||
|
// own DB write) so the FK constraint is satisfied on that call.
|
||||||
|
let localUser: typeof users.$inferSelect | undefined;
|
||||||
|
try {
|
||||||
|
localUser = await db.transaction(async (tx) => {
|
||||||
|
// Serialise: at most one unclaimed admin row may exist (WR-02)
|
||||||
|
const [{ count }] = await tx.execute<{ count: number }>(
|
||||||
|
sql`SELECT COUNT(*) AS count FROM users WHERE claimed = false FOR UPDATE`,
|
||||||
|
);
|
||||||
|
if (Number(count) > 0) {
|
||||||
|
throw Object.assign(new Error('An unclaimed user already exists'), { code: 'DUPLICATE_UNCLAIMED' });
|
||||||
|
}
|
||||||
|
|
||||||
// Step 1: Assign color (first unused from palette, or round-robin fallback)
|
// Step 1: Assign color (first unused from palette, or round-robin fallback)
|
||||||
const usedRows = await db.select({ color: users.color }).from(users);
|
const usedRows = await tx.select({ color: users.color }).from(users);
|
||||||
const usedColors = new Set(usedRows.map((r) => r.color));
|
const usedColors = new Set(usedRows.map((r) => r.color));
|
||||||
const color =
|
const color =
|
||||||
COLOR_PALETTE.find((col) => !usedColors.has(col)) ??
|
COLOR_PALETTE.find((col) => !usedColors.has(col)) ??
|
||||||
@@ -256,7 +272,7 @@ setupRouter.post('/credential', zValidator('json', credentialSchema, noEchoHook)
|
|||||||
|
|
||||||
// Step 2: Insert the local user (oidcIss=null, oidcSub=null, claimed=false)
|
// Step 2: Insert the local user (oidcIss=null, oidcSub=null, claimed=false)
|
||||||
// mysql2 has no RETURNING clause — use $returningId() then re-select (Pattern 4)
|
// mysql2 has no RETURNING clause — use $returningId() then re-select (Pattern 4)
|
||||||
const [inserted] = await db
|
const [inserted] = await tx
|
||||||
.insert(users)
|
.insert(users)
|
||||||
.values({
|
.values({
|
||||||
oidcIss: null,
|
oidcIss: null,
|
||||||
@@ -268,16 +284,31 @@ setupRouter.post('/credential', zValidator('json', credentialSchema, noEchoHook)
|
|||||||
})
|
})
|
||||||
.$returningId();
|
.$returningId();
|
||||||
|
|
||||||
const [localUser] = await db
|
const [row] = await tx
|
||||||
.select()
|
.select()
|
||||||
.from(users)
|
.from(users)
|
||||||
.where(eq(users.id, inserted.id))
|
.where(eq(users.id, inserted.id))
|
||||||
.limit(1);
|
.limit(1);
|
||||||
|
|
||||||
|
return row;
|
||||||
|
});
|
||||||
|
} catch (err) {
|
||||||
|
if (err instanceof Error && (err as NodeJS.ErrnoException & { code?: string }).code === 'DUPLICATE_UNCLAIMED') {
|
||||||
|
// A concurrent request already created the unclaimed admin row
|
||||||
|
return c.json({ error: 'Setup already in progress' }, 409);
|
||||||
|
}
|
||||||
|
console.error('[setup/POST /credential] Transaction error:', err instanceof Error ? err.message : String(err));
|
||||||
|
return c.json({ error: 'Service unavailable' }, 503);
|
||||||
|
}
|
||||||
|
|
||||||
if (!localUser) {
|
if (!localUser) {
|
||||||
// Clean up the just-inserted user row to avoid an orphaned unclaimed admin
|
// Clean up the just-inserted user row to avoid an orphaned unclaimed admin
|
||||||
// (WR-01: the catch block below does not cover this early-return path).
|
// (WR-01: the catch block below does not cover this early-return path).
|
||||||
await db.delete(users).where(eq(users.id, inserted.id));
|
// This path is extremely unlikely if the transaction committed but the re-select
|
||||||
|
// returned nothing — a transient DB issue. No row ID is available here since
|
||||||
|
// the transaction already committed with the row. The FOR UPDATE guard above
|
||||||
|
// means no second unclaimed row will exist; the orphan (if any) is claimed on
|
||||||
|
// first login via upsertUser, making this a safe degraded-mode path.
|
||||||
return c.json({ error: 'Service unavailable' }, 503);
|
return c.json({ error: 'Service unavailable' }, 503);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
Reference in New Issue
Block a user