From bf63f6bdba26930b5a9f5ebc2886799df0c39e88 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Bombeck?= Date: Sat, 26 Sep 2026 20:22:12 +0200 Subject: [PATCH] fix(auth): claim a WebAuthn challenge in the statement that checks it The passkey sign-in, security-key sign-in and security-key registration verifiers read the challenge, verified the response, and deleted the challenge afterwards. Two requests carrying the same assertion could both read it before either deleted it, and both be accepted. A challenge is now claimed with one guarded DELETE ... RETURNING that also checks its ceremony, its account and its expiry, before anything is verified; a concurrent second request finds nothing. A passkey sign-in also refuses a challenge that was begun for a different account than the passkey belongs to. --- src/lib/auth/__tests__/passkey.test.ts | 4 + .../webauthn-library-contract.test.ts | 16 +++ src/lib/auth/mfa/webauthn.ts | 127 ++++++++---------- src/lib/auth/passkey.ts | 109 ++++++++++----- .../webauthn-challenge-claim.test.ts | 98 ++++++++++++++ 5 files changed, 249 insertions(+), 105 deletions(-) create mode 100644 tests/integration/webauthn-challenge-claim.test.ts diff --git a/src/lib/auth/__tests__/passkey.test.ts b/src/lib/auth/__tests__/passkey.test.ts index d7edcda31..e4877a0e9 100644 --- a/src/lib/auth/__tests__/passkey.test.ts +++ b/src/lib/auth/__tests__/passkey.test.ts @@ -14,6 +14,7 @@ import { describe, it, expect, vi, beforeEach } from "vitest"; vi.mock("@/lib/db", () => ({ prisma: { + $queryRaw: vi.fn(), authChallenge: { findUnique: vi.fn(), deleteMany: vi.fn(), @@ -57,6 +58,9 @@ const VALID_BODY = { }; beforeEach(() => { + vi.mocked(prisma.$queryRaw).mockResolvedValue([ + { challenge: "challenge", user_id: null }, + ] as never); vi.mocked(prisma.authChallenge.findUnique).mockResolvedValue({ id: "ch-1", challenge: "challenge", diff --git a/src/lib/auth/__tests__/webauthn-library-contract.test.ts b/src/lib/auth/__tests__/webauthn-library-contract.test.ts index a20e474a9..cfe07e234 100644 --- a/src/lib/auth/__tests__/webauthn-library-contract.test.ts +++ b/src/lib/auth/__tests__/webauthn-library-contract.test.ts @@ -63,6 +63,22 @@ vi.mock("@/lib/db", () => { ) => [...table.values()].find((r) => r.credentialId === credentialId) ?? null; return { prisma: { + // `claimAuthChallenge`'s guarded DELETE ... RETURNING, over the map: + // values are (id, type) or (id, type, userId). + $queryRaw: async (_sql: TemplateStringsArray, ...values: unknown[]) => { + const [id, type, userId] = values as [string, string, string?]; + const row = store.authChallenge.get(id); + if ( + !row || + row.type !== type || + (userId !== undefined && row.userId !== userId) || + !((row.expiresAt as Date) > new Date()) + ) { + return []; + } + store.authChallenge.delete(id); + return [{ challenge: row.challenge, user_id: row.userId ?? null }]; + }, authChallenge: { deleteMany: async () => ({ count: 0 }), create: async ({ data }: { data: Record }) => { diff --git a/src/lib/auth/mfa/webauthn.ts b/src/lib/auth/mfa/webauthn.ts index 24c6b9ac2..691a5bd90 100644 --- a/src/lib/auth/mfa/webauthn.ts +++ b/src/lib/auth/mfa/webauthn.ts @@ -31,6 +31,7 @@ import { } from "@simplewebauthn/server"; import { z } from "zod/v4"; import { prisma } from "@/lib/db"; +import { claimAuthChallenge } from "@/lib/auth/passkey"; import { RP_NAME as rpName, getRpId, @@ -146,38 +147,30 @@ export async function verifyMfaRegistration( userId: string, response: unknown, ): Promise { - const challenge = await prisma.authChallenge.findUnique({ - where: { id: challengeId }, - }); - // The challenge must be live, of the registration type, and bound to the // calling user — a challenge minted for another account can never be reused. - if ( - !challenge || - challenge.expiresAt < new Date() || - challenge.type !== "mfa_registration" || - challenge.userId !== userId - ) { + // Claimed (deleted) in the same statement that checks it, so two concurrent + // requests cannot both use it. + const challenge = await claimAuthChallenge({ + challengeId, + type: "mfa_registration", + userId, + }); + if (!challenge) { throw new Error("Challenge expired or not found"); } - try { - const parsed = registrationResponseSchema.safeParse(response); - if (!parsed.success) { - throw new Error("Registration response shape invalid"); - } - return await verifyRegistrationResponse({ - response: parsed.data as RegistrationResponseJSON, - expectedChallenge: challenge.challenge, - expectedOrigin: getExpectedOrigin(), - expectedRPID: getRpId(), - supportedAlgorithmIDs: REGISTRATION_ACCEPTED_ALGORITHM_IDS, - }); - } finally { - await prisma.authChallenge - .delete({ where: { id: challengeId } }) - .catch(() => {}); + const parsed = registrationResponseSchema.safeParse(response); + if (!parsed.success) { + throw new Error("Registration response shape invalid"); } + return verifyRegistrationResponse({ + response: parsed.data as RegistrationResponseJSON, + expectedChallenge: challenge.challenge, + expectedOrigin: getExpectedOrigin(), + expectedRPID: getRpId(), + supportedAlgorithmIDs: REGISTRATION_ACCEPTED_ALGORITHM_IDS, + }); } // ── Authentication (mid-login second factor) ───────────────────────── @@ -232,16 +225,14 @@ export async function verifyMfaAuthentication( userId: string, response: unknown, ): Promise { - const challenge = await prisma.authChallenge.findUnique({ - where: { id: challengeId }, + // Claimed (deleted) in the same statement that checks it: one assertion, + // one verification, even when two requests carry it at once. + const challenge = await claimAuthChallenge({ + challengeId, + type: "mfa_authentication", + userId, }); - - if ( - !challenge || - challenge.expiresAt < new Date() || - challenge.type !== "mfa_authentication" || - challenge.userId !== userId - ) { + if (!challenge) { throw new Error("Challenge expired or not found"); } @@ -251,43 +242,37 @@ export async function verifyMfaAuthentication( } const typedResponse = parsed.data as unknown as AuthenticationResponseJSON; - try { - // The presented credential MUST belong to the password-identified user — - // look it up scoped to `userId` so an assertion against another account's - // key can never satisfy this account's second factor. - const credential = await prisma.webauthnMfaCredential.findFirst({ - where: { credentialId: typedResponse.id, userId }, + // The presented credential MUST belong to the password-identified user — + // look it up scoped to `userId` so an assertion against another account's + // key can never satisfy this account's second factor. + const credential = await prisma.webauthnMfaCredential.findFirst({ + where: { credentialId: typedResponse.id, userId }, + }); + if (!credential) return false; + + const verification: VerifiedAuthenticationResponse = + await verifyAuthenticationResponse({ + response: typedResponse, + expectedChallenge: challenge.challenge, + expectedOrigin: getExpectedOrigin(), + expectedRPID: getRpId(), + credential: { + id: credential.credentialId, + publicKey: credential.credentialPublicKey, + counter: Number(credential.counter), + transports: credential.transports as Transport[], + }, }); - if (!credential) return false; - const verification: VerifiedAuthenticationResponse = - await verifyAuthenticationResponse({ - response: typedResponse, - expectedChallenge: challenge.challenge, - expectedOrigin: getExpectedOrigin(), - expectedRPID: getRpId(), - credential: { - id: credential.credentialId, - publicKey: credential.credentialPublicKey, - counter: Number(credential.counter), - transports: credential.transports as Transport[], - }, - }); - - if (verification.verified) { - await prisma.webauthnMfaCredential.update({ - where: { id: credential.id }, - data: { - counter: BigInt(verification.authenticationInfo.newCounter), - lastUsedAt: new Date(), - }, - }); - } - - return verification.verified; - } finally { - await prisma.authChallenge - .delete({ where: { id: challengeId } }) - .catch(() => {}); + if (verification.verified) { + await prisma.webauthnMfaCredential.update({ + where: { id: credential.id }, + data: { + counter: BigInt(verification.authenticationInfo.newCounter), + lastUsedAt: new Date(), + }, + }); } + + return verification.verified; } diff --git a/src/lib/auth/passkey.ts b/src/lib/auth/passkey.ts index d32ee3ad6..8598beb0d 100644 --- a/src/lib/auth/passkey.ts +++ b/src/lib/auth/passkey.ts @@ -201,6 +201,46 @@ export async function consumeRegistrationChallenge(input: { }; } +/** + * Claim a WebAuthn challenge: delete it and return its material in one + * statement, or return null when it is unknown, expired, of another ceremony, + * or (when `userId` is given) minted for another account. + * + * Every ceremony verifier goes through here. They used to read the challenge, + * verify, and delete it in a `finally`, so two requests carrying the same + * assertion could both read it before either deleted it and both be + * accepted. The guarded DELETE ... RETURNING gives exactly one of them the + * challenge; the other finds nothing. + */ +export async function claimAuthChallenge(input: { + challengeId: string; + type: string; + userId?: string; +}): Promise<{ challenge: string; userId: string | null } | null> { + const rows = input.userId + ? await prisma.$queryRaw< + Array<{ challenge: string; user_id: string | null }> + >` + DELETE FROM auth_challenges + WHERE id = ${input.challengeId} + AND type = ${input.type} + AND user_id = ${input.userId} + AND expires_at > NOW() + RETURNING challenge, user_id + ` + : await prisma.$queryRaw< + Array<{ challenge: string; user_id: string | null }> + >` + DELETE FROM auth_challenges + WHERE id = ${input.challengeId} + AND type = ${input.type} + AND expires_at > NOW() + RETURNING challenge, user_id + `; + if (rows.length !== 1) return null; + return { challenge: rows[0].challenge, userId: rows[0].user_id }; +} + export async function verifyRegistration( expectedChallenge: string, response: unknown, @@ -271,11 +311,13 @@ export async function verifyAuthentication( verification: VerifiedAuthenticationResponse; passkey: { userId: string }; }> { - const challenge = await prisma.authChallenge.findUnique({ - where: { id: challengeId }, + // Claimed before anything else: a challenge is good for one verification, + // successful or not, and a concurrent second request finds nothing. + const challenge = await claimAuthChallenge({ + challengeId, + type: "authentication", }); - - if (!challenge || challenge.expiresAt < new Date()) { + if (!challenge) { throw new Error("Challenge expired or not found"); } @@ -301,37 +343,36 @@ export async function verifyAuthentication( throw new Error("Passkey not found"); } - try { - const verification = await verifyAuthenticationResponse({ - response: typedResponse, - expectedChallenge: challenge.challenge, - expectedOrigin: getExpectedOrigin(), - expectedRPID: getRpId(), - credential: { - id: passkey.credentialId, - publicKey: passkey.credentialPublicKey, - counter: Number(passkey.counter), - transports: passkey.transports as Transport[], + // A challenge begun for one account (the re-proof arm names the account) + // is answered only by that account's passkey. + if (challenge.userId !== null && challenge.userId !== passkey.userId) { + throw new Error("Challenge expired or not found"); + } + + const verification = await verifyAuthenticationResponse({ + response: typedResponse, + expectedChallenge: challenge.challenge, + expectedOrigin: getExpectedOrigin(), + expectedRPID: getRpId(), + credential: { + id: passkey.credentialId, + publicKey: passkey.credentialPublicKey, + counter: Number(passkey.counter), + transports: passkey.transports as Transport[], + }, + }); + + if (verification.verified) { + // Update counter + stamp last-used so the management UI can surface + // when each passkey was last exercised (v1.23 passkey QoL). + await prisma.passkey.update({ + where: { id: passkey.id }, + data: { + counter: BigInt(verification.authenticationInfo.newCounter), + lastUsedAt: new Date(), }, }); - - if (verification.verified) { - // Update counter + stamp last-used so the management UI can surface - // when each passkey was last exercised (v1.23 passkey QoL). - await prisma.passkey.update({ - where: { id: passkey.id }, - data: { - counter: BigInt(verification.authenticationInfo.newCounter), - lastUsedAt: new Date(), - }, - }); - } - - return { verification, passkey: { userId: passkey.userId } }; - } finally { - // Invalidate challenge after first verification attempt (success or failure) - await prisma.authChallenge - .delete({ where: { id: challengeId } }) - .catch(() => {}); } + + return { verification, passkey: { userId: passkey.userId } }; } diff --git a/tests/integration/webauthn-challenge-claim.test.ts b/tests/integration/webauthn-challenge-claim.test.ts new file mode 100644 index 000000000..e3a31bb3e --- /dev/null +++ b/tests/integration/webauthn-challenge-claim.test.ts @@ -0,0 +1,98 @@ +/** + * A WebAuthn challenge is good for exactly one verification. + * + * The ceremony verifiers used to read the challenge, verify, and delete it + * afterwards, so two requests carrying the same assertion could both read it + * and both pass. `claimAuthChallenge` deletes and returns it in one guarded + * statement; this pins that against real Postgres. + */ +import { beforeEach, describe, expect, it } from "vitest"; + +import { getPrismaClient, truncateAllTables } from "./setup"; + +const { claimAuthChallenge } = await import("@/lib/auth/passkey"); + +const USER_ID = "user-challenge-claim"; + +async function challenge( + type: string, + opts: { userId?: string | null; expiresInMs?: number } = {}, +): Promise { + const row = await getPrismaClient().authChallenge.create({ + data: { + userId: opts.userId === undefined ? USER_ID : opts.userId, + challenge: `material-${type}`, + type, + expiresAt: new Date(Date.now() + (opts.expiresInMs ?? 60_000)), + }, + }); + return row.id; +} + +beforeEach(async () => { + await truncateAllTables(getPrismaClient()); + await getPrismaClient().user.create({ + data: { + id: USER_ID, + username: "claim-user", + email: "claim@example.test", + }, + }); +}); + +describe("claimAuthChallenge", () => { + it("hands the challenge to exactly one of two concurrent claims", async () => { + const id = await challenge("authentication"); + const results = await Promise.all( + Array.from({ length: 8 }, () => + claimAuthChallenge({ challengeId: id, type: "authentication" }), + ), + ); + expect(results.filter((r) => r !== null)).toHaveLength(1); + expect(await getPrismaClient().authChallenge.count({ where: { id } })).toBe( + 0, + ); + }); + + it("returns the material and the account it was begun for", async () => { + const id = await challenge("mfa_authentication"); + await expect( + claimAuthChallenge({ + challengeId: id, + type: "mfa_authentication", + userId: USER_ID, + }), + ).resolves.toEqual({ + challenge: "material-mfa_authentication", + userId: USER_ID, + }); + }); + + it("refuses another ceremony's challenge and leaves it in place", async () => { + const id = await challenge("mfa_authentication"); + await expect( + claimAuthChallenge({ challengeId: id, type: "authentication" }), + ).resolves.toBeNull(); + expect(await getPrismaClient().authChallenge.count({ where: { id } })).toBe( + 1, + ); + }); + + it("refuses a challenge begun for another account", async () => { + const id = await challenge("mfa_registration"); + await expect( + claimAuthChallenge({ + challengeId: id, + type: "mfa_registration", + userId: "someone-else", + }), + ).resolves.toBeNull(); + }); + + it("refuses an expired challenge", async () => { + const id = await challenge("authentication", { expiresInMs: -1_000 }); + await expect( + claimAuthChallenge({ challengeId: id, type: "authentication" }), + ).resolves.toBeNull(); + }); +});