diff --git a/src/__tests__/api/auth/mfa/verify.test.ts b/src/__tests__/api/auth/mfa/verify.test.ts index b16904be3..094614336 100644 --- a/src/__tests__/api/auth/mfa/verify.test.ts +++ b/src/__tests__/api/auth/mfa/verify.test.ts @@ -31,6 +31,7 @@ vi.mock("@/lib/auth/login-response", () => ({ })); vi.mock("@/lib/auth/mfa/challenge", () => ({ loadActiveChallenge: vi.fn(), + reserveChallengeAttempt: vi.fn().mockResolvedValue(true), recordChallengeFailure: vi .fn() .mockResolvedValue({ exhausted: false, attempts: 1 }), @@ -53,6 +54,7 @@ import { prisma } from "@/lib/db"; import { finishLogin } from "@/lib/auth/login-response"; import { loadActiveChallenge, + reserveChallengeAttempt, recordChallengeFailure, claimChallenge, } from "@/lib/auth/mfa/challenge"; @@ -105,6 +107,16 @@ describe("POST /api/auth/mfa/verify", () => { expect(arg.source).toBe("mfa.verify"); }); + it("no attempt left to reserve → 401 before the factor is evaluated", async () => { + vi.mocked(loadActiveChallenge).mockResolvedValue(ACTIVE_CHALLENGE as never); + vi.mocked(reserveChallengeAttempt).mockResolvedValueOnce(false); + + const res = await POST(verifyRequest(GOOD_BODY)); + expect(res.status).toBe(401); + expect(verifyMfaFactor).not.toHaveBeenCalled(); + expect(finishLogin).not.toHaveBeenCalled(); + }); + it("invalid ticket → 401, no session", async () => { vi.mocked(loadActiveChallenge).mockResolvedValue(null); const res = await POST(verifyRequest(GOOD_BODY)); diff --git a/src/app/api/auth/mfa/verify/route.ts b/src/app/api/auth/mfa/verify/route.ts index cdd7b03af..94bd30393 100644 --- a/src/app/api/auth/mfa/verify/route.ts +++ b/src/app/api/auth/mfa/verify/route.ts @@ -32,6 +32,7 @@ import { checkAuthSurfaceRateLimit, rateLimitHeaders } from "@/lib/rate-limit"; import { finishLogin } from "@/lib/auth/login-response"; import { loadActiveChallenge, + reserveChallengeAttempt, recordChallengeFailure, claimChallenge, } from "@/lib/auth/mfa/challenge"; @@ -90,6 +91,14 @@ export const POST = apiHandler(async (request: NextRequest) => { return apiError("Invalid or expired challenge", 401); } + // Count this attempt before anything is verified. The reservation is one + // guarded UPDATE, so concurrent guesses cannot all slip under the cap the + // way a read-then-increment let them. + if (!(await reserveChallengeAttempt(challenge.id))) { + annotate({ action: { name: "auth.mfa.verify.attempts_exhausted" } }); + return apiError("Invalid or expired challenge", 401); + } + const user = await prisma.user.findUnique({ where: { id: challenge.userId }, select: { diff --git a/src/app/api/auth/mfa/webauthn/verify/__tests__/route.test.ts b/src/app/api/auth/mfa/webauthn/verify/__tests__/route.test.ts index 14815ff0c..fa86f7987 100644 --- a/src/app/api/auth/mfa/webauthn/verify/__tests__/route.test.ts +++ b/src/app/api/auth/mfa/webauthn/verify/__tests__/route.test.ts @@ -25,6 +25,7 @@ vi.mock("@/lib/rate-limit", () => ({ vi.mock("@/lib/auth/mfa/challenge", () => ({ loadActiveChallenge: vi.fn(), + reserveChallengeAttempt: vi.fn().mockResolvedValue(true), recordChallengeFailure: vi.fn().mockResolvedValue({ exhausted: false }), claimChallenge: vi.fn(), })); @@ -50,6 +51,7 @@ import { prisma } from "@/lib/db"; import { checkAuthSurfaceRateLimit } from "@/lib/rate-limit"; import { loadActiveChallenge, + reserveChallengeAttempt, claimChallenge, recordChallengeFailure, } from "@/lib/auth/mfa/challenge"; @@ -93,6 +95,7 @@ beforeEach(() => { username: "u", onboardingCompletedAt: new Date(), } as never); + vi.mocked(reserveChallengeAttempt).mockResolvedValue(true); vi.mocked(recordChallengeFailure).mockResolvedValue({ exhausted: false, attempts: 1, @@ -126,6 +129,14 @@ describe("POST /api/auth/mfa/webauthn/verify", () => { ); }); + it("refuses before the assertion runs when no attempt can be reserved", async () => { + vi.mocked(reserveChallengeAttempt).mockResolvedValue(false); + const res = await POST(makeRequest(VALID_BODY)); + expect(res.status).toBe(401); + expect(verifyMfaAuthentication).not.toHaveBeenCalled(); + expect(finishLogin).not.toHaveBeenCalled(); + }); + it("rejects with 401 and burns an attempt when the assertion fails", async () => { vi.mocked(verifyMfaAuthentication).mockResolvedValue(false as never); diff --git a/src/app/api/auth/mfa/webauthn/verify/route.ts b/src/app/api/auth/mfa/webauthn/verify/route.ts index 00df78104..9df1e93d7 100644 --- a/src/app/api/auth/mfa/webauthn/verify/route.ts +++ b/src/app/api/auth/mfa/webauthn/verify/route.ts @@ -30,6 +30,7 @@ import { checkAuthSurfaceRateLimit, rateLimitHeaders } from "@/lib/rate-limit"; import { finishLogin } from "@/lib/auth/login-response"; import { loadActiveChallenge, + reserveChallengeAttempt, recordChallengeFailure, claimChallenge, } from "@/lib/auth/mfa/challenge"; @@ -86,6 +87,14 @@ export const POST = apiHandler(async (request: NextRequest) => { return apiError("Invalid or expired challenge", 401); } + // Count this attempt before anything is verified. The reservation is one + // guarded UPDATE, so concurrent guesses cannot all slip under the cap the + // way a read-then-increment let them. + if (!(await reserveChallengeAttempt(challenge.id))) { + annotate({ action: { name: "auth.mfa.verify.attempts_exhausted" } }); + return apiError("Invalid or expired challenge", 401); + } + const user = await prisma.user.findUnique({ where: { id: challenge.userId }, select: { id: true, username: true, onboardingCompletedAt: true }, diff --git a/src/lib/auth/mfa/__tests__/challenge.test.ts b/src/lib/auth/mfa/__tests__/challenge.test.ts index 74e4bfc47..141a1607e 100644 --- a/src/lib/auth/mfa/__tests__/challenge.test.ts +++ b/src/lib/auth/mfa/__tests__/challenge.test.ts @@ -21,6 +21,7 @@ import { createMfaChallenge, loadActiveChallenge, recordChallengeFailure, + reserveChallengeAttempt, claimChallenge, MFA_CHALLENGE_ATTEMPT_CAP, } from "../challenge"; @@ -91,8 +92,41 @@ describe("mfa challenge", () => { expect(await loadActiveChallenge("nope")).toBeNull(); }); + it("reserves an attempt with one guarded update bounded by the cap", async () => { + vi.mocked(prisma.mfaChallenge.updateMany).mockResolvedValue({ + count: 1, + } as never); + expect(await reserveChallengeAttempt("ch-1")).toBe(true); + expect(prisma.mfaChallenge.updateMany).toHaveBeenCalledWith({ + where: { + id: "ch-1", + consumedAt: null, + attempts: { lt: MFA_CHALLENGE_ATTEMPT_CAP }, + expiresAt: { gt: expect.any(Date) }, + }, + data: { attempts: { increment: 1 } }, + }); + // No separate read decides the cap — the WHERE is the whole check. + expect(prisma.mfaChallenge.findUnique).not.toHaveBeenCalled(); + }); + + it("refuses the reservation once the guarded update matches nothing", async () => { + vi.mocked(prisma.mfaChallenge.updateMany).mockResolvedValue({ + count: 0, + } as never); + expect(await reserveChallengeAttempt("ch-1")).toBe(false); + }); + + it("settling a failure does not count the attempt a second time", async () => { + vi.mocked(prisma.mfaChallenge.findUnique).mockResolvedValue({ + attempts: 2, + } as never); + await recordChallengeFailure("ch-1"); + expect(prisma.mfaChallenge.update).not.toHaveBeenCalled(); + }); + it("burns the ticket when the attempt cap is reached", async () => { - vi.mocked(prisma.mfaChallenge.update).mockResolvedValue({ + vi.mocked(prisma.mfaChallenge.findUnique).mockResolvedValue({ attempts: MFA_CHALLENGE_ATTEMPT_CAP, } as never); vi.mocked(prisma.mfaChallenge.updateMany).mockResolvedValue({ @@ -108,7 +142,7 @@ describe("mfa challenge", () => { }); it("does not burn below the attempt cap", async () => { - vi.mocked(prisma.mfaChallenge.update).mockResolvedValue({ + vi.mocked(prisma.mfaChallenge.findUnique).mockResolvedValue({ attempts: 1, } as never); const res = await recordChallengeFailure("ch-1"); diff --git a/src/lib/auth/mfa/challenge.ts b/src/lib/auth/mfa/challenge.ts index 9e641982d..a816bc4c0 100644 --- a/src/lib/auth/mfa/challenge.ts +++ b/src/lib/auth/mfa/challenge.ts @@ -11,9 +11,13 @@ * * Guarantees enforced here: * - **TTL** (~5 min) — `expiresAt`; an expired ticket is never loadable. - * - **Attempt cap** — `attempts` is incremented on every wrong factor and - * the ticket is burned (`consumedAt` set) once the cap is hit, forcing a - * fresh password login (NIST throttle, not an account lock). + * - **Attempt cap** — every verification first RESERVES an attempt with one + * guarded UPDATE (`attempts < cap` in the WHERE), so the cap bounds the + * number of guesses even when they arrive concurrently. It used to be a + * read, a verification, then an increment: parallel requests all read the + * same count below the cap and every one of them got to guess. A failed + * verification then burns the ticket (`consumedAt` set) once the cap is + * hit, forcing a fresh password login (NIST throttle, not an account lock). * - **Claim-once** — consuming a ticket is an atomic guarded update * (`consumedAt: null` in the WHERE), so two concurrent verifications can * never both succeed and mint two sessions. The factor is verified first; @@ -96,26 +100,50 @@ export async function loadActiveChallenge( } /** - * Record a failed factor attempt. Increments `attempts`; when the cap is - * reached the ticket is burned (`consumedAt` set) so it cannot be retried. - * Returns whether the ticket is now exhausted. + * Reserve one verification attempt against the ticket. Must run BEFORE the + * factor is checked. The increment and the cap check are one conditional + * UPDATE, so of any number of concurrent callers at most + * `MFA_CHALLENGE_ATTEMPT_CAP` in total ever get `true`. A `false` means the + * ticket is spent, expired or out of attempts, and the caller answers with the + * same generic refusal as an unknown ticket. + */ +export async function reserveChallengeAttempt( + challengeId: string, +): Promise { + const reserved = await prisma.mfaChallenge.updateMany({ + where: { + id: challengeId, + consumedAt: null, + attempts: { lt: MFA_CHALLENGE_ATTEMPT_CAP }, + expiresAt: { gt: new Date() }, + }, + data: { attempts: { increment: 1 } }, + }); + return reserved.count === 1; +} + +/** + * Settle a failed attempt that `reserveChallengeAttempt` already counted. + * Does not increment again; when the reserved count has reached the cap the + * ticket is burned (`consumedAt` set) so it cannot be retried. Returns whether + * the ticket is now exhausted. */ export async function recordChallengeFailure( challengeId: string, ): Promise<{ exhausted: boolean; attempts: number }> { - const updated = await prisma.mfaChallenge.update({ + const row = await prisma.mfaChallenge.findUnique({ where: { id: challengeId }, - data: { attempts: { increment: 1 } }, select: { attempts: true }, }); - const exhausted = updated.attempts >= MFA_CHALLENGE_ATTEMPT_CAP; + const attempts = row?.attempts ?? MFA_CHALLENGE_ATTEMPT_CAP; + const exhausted = attempts >= MFA_CHALLENGE_ATTEMPT_CAP; if (exhausted) { await prisma.mfaChallenge.updateMany({ where: { id: challengeId, consumedAt: null }, data: { consumedAt: new Date() }, }); } - return { exhausted, attempts: updated.attempts }; + return { exhausted, attempts }; } /** diff --git a/tests/integration/mfa-login-flow.test.ts b/tests/integration/mfa-login-flow.test.ts index 47839f959..43e53ccfc 100644 --- a/tests/integration/mfa-login-flow.test.ts +++ b/tests/integration/mfa-login-flow.test.ts @@ -100,6 +100,7 @@ describe("MFA login flow (real Postgres)", () => { createMfaChallenge, loadActiveChallenge, claimChallenge, + reserveChallengeAttempt, recordChallengeFailure, MFA_CHALLENGE_ATTEMPT_CAP, } = await import("@/lib/auth/mfa/challenge"); @@ -123,10 +124,41 @@ describe("MFA login flow (real Postgres)", () => { const row = await loadActiveChallenge(second.ticket); let exhausted = false; for (let i = 0; i < MFA_CHALLENGE_ATTEMPT_CAP; i++) { + expect(await reserveChallengeAttempt(row!.id)).toBe(true); ({ exhausted } = await recordChallengeFailure(row!.id)); } expect(exhausted).toBe(true); expect(await loadActiveChallenge(second.ticket)).toBeNull(); + expect(await reserveChallengeAttempt(row!.id)).toBe(false); + }); + + it("concurrent guesses cannot exceed the attempt cap", async () => { + const { + createMfaChallenge, + loadActiveChallenge, + reserveChallengeAttempt, + MFA_CHALLENGE_ATTEMPT_CAP, + } = await import("@/lib/auth/mfa/challenge"); + const prisma = getPrismaClient(); + const user = await prisma.user.create({ + data: { username: "race-user", email: "race@example.test" }, + }); + const { ticket } = await createMfaChallenge(user.id, "login"); + const active = await loadActiveChallenge(ticket); + + // Every request read "0 attempts" under the old read-then-increment; fire + // four times the cap at once and count how many were allowed to guess. + const results = await Promise.all( + Array.from({ length: MFA_CHALLENGE_ATTEMPT_CAP * 4 }, () => + reserveChallengeAttempt(active!.id), + ), + ); + expect(results.filter(Boolean)).toHaveLength(MFA_CHALLENGE_ATTEMPT_CAP); + const row = await prisma.mfaChallenge.findUnique({ + where: { id: active!.id }, + select: { attempts: true }, + }); + expect(row?.attempts).toBe(MFA_CHALLENGE_ATTEMPT_CAP); }); it("recovery code is one-time-use and regeneration invalidates the set", async () => {