From 8cb958bf275d6f0bcbf5450c4f62a100d6f4d353 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Bombeck?= Date: Sat, 26 Sep 2026 15:19:26 +0200 Subject: [PATCH] fix(auth): accept a TOTP code only from a confirmed secret A pending secret from an enrollment that was never confirmed counted as a second factor wherever a code was verified: step-up minting, the passkey enrollment re-proof, MFA disable and login completion. Anyone holding a session can create a pending secret, so a code from one proves nothing about the account owner. verifyMfaFactor now refuses both TOTP and recovery codes until totpConfirmedAt is set. --- .../auth/mfa/__tests__/verify-factor.test.ts | 89 +++++++++++++++++++ src/lib/auth/mfa/verify-factor.ts | 21 ++++- tests/integration/mfa-login-flow.test.ts | 8 +- 3 files changed, 114 insertions(+), 4 deletions(-) create mode 100644 src/lib/auth/mfa/__tests__/verify-factor.test.ts diff --git a/src/lib/auth/mfa/__tests__/verify-factor.test.ts b/src/lib/auth/mfa/__tests__/verify-factor.test.ts new file mode 100644 index 000000000..e701c07d0 --- /dev/null +++ b/src/lib/auth/mfa/__tests__/verify-factor.test.ts @@ -0,0 +1,89 @@ +/** + * `verifyMfaFactor` is the one place a TOTP code is accepted as proof of a + * second factor — login completion, step-up minting, MFA disable and the + * passkey-enrollment re-proof all call it. A secret that was never confirmed + * is a pending enrollment, not a factor: whoever holds a session can create + * one, so a code from it proves only that the caller started enrollment. + */ +import { describe, it, expect, vi, beforeEach } from "vitest"; + +vi.mock("@/lib/db", () => ({ + prisma: { user: { updateMany: vi.fn() } }, +})); +vi.mock("@/lib/crypto", () => ({ + decrypt: vi.fn(() => "SECRET"), +})); +vi.mock("@/lib/auth/mfa/totp", () => ({ + verifyTotp: vi.fn(() => ({ valid: true, replay: false, step: 100 })), +})); +vi.mock("@/lib/auth/mfa/recovery-codes", () => ({ + verifyAndConsumeRecoveryCode: vi.fn(async () => true), +})); + +import { verifyMfaFactor } from "../verify-factor"; +import { verifyTotp } from "@/lib/auth/mfa/totp"; +import { verifyAndConsumeRecoveryCode } from "@/lib/auth/mfa/recovery-codes"; +import { prisma } from "@/lib/db"; + +const baseUser = { + id: "u1", + totpSecretEncrypted: "enc", + totpLastStep: null, +}; + +beforeEach(() => { + vi.clearAllMocks(); + vi.mocked(prisma.user.updateMany).mockResolvedValue({ count: 1 } as never); +}); + +describe("verifyMfaFactor", () => { + it("accepts a valid code against a confirmed secret and burns the step", async () => { + const res = await verifyMfaFactor( + { ...baseUser, totpConfirmedAt: new Date() }, + "totp", + "123456", + ); + expect(res).toEqual({ ok: true, replay: false }); + expect(prisma.user.updateMany).toHaveBeenCalledTimes(1); + }); + + it("refuses a valid code against a pending (unconfirmed) secret", async () => { + const res = await verifyMfaFactor( + { ...baseUser, totpConfirmedAt: null }, + "totp", + "123456", + ); + expect(res).toEqual({ ok: false, replay: false }); + // The code is never even evaluated, and no step is burned. + expect(verifyTotp).not.toHaveBeenCalled(); + expect(prisma.user.updateMany).not.toHaveBeenCalled(); + }); + + it("refuses when no secret is stored at all", async () => { + const res = await verifyMfaFactor( + { ...baseUser, totpSecretEncrypted: null, totpConfirmedAt: new Date() }, + "totp", + "123456", + ); + expect(res.ok).toBe(false); + }); + + it("refuses a recovery code while no second factor is confirmed", async () => { + const res = await verifyMfaFactor( + { ...baseUser, totpConfirmedAt: null }, + "recovery", + "abcd-efgh", + ); + expect(res.ok).toBe(false); + expect(verifyAndConsumeRecoveryCode).not.toHaveBeenCalled(); + }); + + it("accepts a recovery code once the factor is confirmed", async () => { + const res = await verifyMfaFactor( + { ...baseUser, totpConfirmedAt: new Date() }, + "recovery", + "abcd-efgh", + ); + expect(res.ok).toBe(true); + }); +}); diff --git a/src/lib/auth/mfa/verify-factor.ts b/src/lib/auth/mfa/verify-factor.ts index 7344787e5..36536af2c 100644 --- a/src/lib/auth/mfa/verify-factor.ts +++ b/src/lib/auth/mfa/verify-factor.ts @@ -8,9 +8,18 @@ * - **Recovery code:** Argon2id-verify against the unused set and burn the * matched row. * - * Used by both the login-completion route (`/api/auth/mfa/verify`) and the - * step-up-gated MFA-disable route, so the factor semantics are identical - * wherever a current factor must be proven. + * Used by the login-completion route (`/api/auth/mfa/verify`), the Bearer + * step-up mint, the step-up-gated MFA-disable route and the credential + * re-proof in `existing-factor-proof.ts`, so the factor semantics are + * identical wherever a current factor must be proven. + * + * **Only a CONFIRMED secret is a factor.** `/me/mfa/totp/setup` writes a + * pending secret for anyone holding the session, so a code from a secret + * whose `totpConfirmedAt` is still null proves only that the caller started + * an enrollment — never that they hold a factor the account owner set up. + * Accepting it would let a stolen session mint its own "second factor" and + * spend it on every gate this function guards. Recovery codes follow the same + * rule: they exist only for a confirmed factor, and are refused otherwise. */ import { prisma } from "@/lib/db"; import { decrypt } from "@/lib/crypto"; @@ -23,6 +32,8 @@ export interface FactorUser { id: string; totpSecretEncrypted: string | null; totpLastStep: bigint | null; + /** Null while the secret is a pending enrollment — not yet a factor. */ + totpConfirmedAt: Date | null; } export interface FactorResult { @@ -36,6 +47,10 @@ export async function verifyMfaFactor( method: MfaMethod, code: string, ): Promise { + // A pending secret is not a factor, and recovery codes only ever belong to a + // confirmed one. Refuse both before any code is evaluated or step burned. + if (!user.totpConfirmedAt) return { ok: false, replay: false }; + if (method === "recovery") { const ok = await verifyAndConsumeRecoveryCode(user.id, code); return { ok, replay: false }; diff --git a/tests/integration/mfa-login-flow.test.ts b/tests/integration/mfa-login-flow.test.ts index f646d7ad5..47839f959 100644 --- a/tests/integration/mfa-login-flow.test.ts +++ b/tests/integration/mfa-login-flow.test.ts @@ -63,7 +63,12 @@ describe("MFA login flow (real Postgres)", () => { const code = codeAt(secret, Date.now()); const first = await verifyMfaFactor( - { id: user.id, totpSecretEncrypted: encrypt(secret), totpLastStep: null }, + { + id: user.id, + totpSecretEncrypted: encrypt(secret), + totpLastStep: null, + totpConfirmedAt: user.totpConfirmedAt, + }, "totp", code, ); @@ -81,6 +86,7 @@ describe("MFA login flow (real Postgres)", () => { id: user.id, totpSecretEncrypted: encrypt(secret), totpLastStep: afterFirst?.totpLastStep ?? null, + totpConfirmedAt: user.totpConfirmedAt, }, "totp", code,