mirror of
https://github.com/MBombeck/HealthLog.git
synced 2026-11-10 21:32:18 +00:00
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.
This commit is contained in:
1 parent
4af6b87a2a
commit
8cb958bf27
3 files changed
+114
-4
No files matched your search
@@ -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);
|
||||
});
|
||||
});
|
||||
@@ -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<FactorResult> {
|
||||
// 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 };
|
||||
|
||||
@@ -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,
|
||||
|
||||
Reference in new issue
Block a user