fix(auth): count a second-factor attempt before verifying it

The login challenge read its attempt count, verified the factor, and only
then incremented. Requests sent in parallel all read the same count below
the cap, so one ticket allowed more guesses than the cap. Each attempt is
now reserved by a single guarded UPDATE before the factor is evaluated,
and a failure only settles the reserved attempt.
This commit is contained in:
Marc-André Bombeck committed 2026-09-26 15:21:42 +02:00
1 parent 8cb958bf27
commit e1718e3c30
7 files changed
+147 -12

No files matched your search

+12
View File
@@ -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));
+9
View File
@@ -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: {
@@ -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);
@@ -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 },
+36 -2
View File
@@ -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");
+38 -10
View File
@@ -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<boolean> {
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 };
}
/**
+32
View File
@@ -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 () => {