From 01689f77bca2ff395105da1c9d2bb5a284f4fcb0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Bombeck?= Date: Sat, 27 Jun 2026 20:34:11 +0200 Subject: [PATCH] feat(security): require fresh verification before destructive account actions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Account deletion and the keep-account data reset now demand a recent second-factor re-verification for accounts that have a second factor active; single-factor accounts are unaffected and keep the typed- confirmation-only contract. The gate runs through requireFreshMfaIfEnrolled, which is cookie-only by construction — a Bearer transport for an MFA-enrolled account cannot satisfy step-up and surfaces the auth.stepup.required envelope so the UI can launch a re-verification flow. --- src/app/api/settings/account/route.ts | 13 +- src/app/api/settings/data/route.ts | 12 +- .../require-fresh-mfa-if-enrolled.test.ts | 117 ++++++++++++++++++ src/lib/api-handler.ts | 28 +++++ 4 files changed, 166 insertions(+), 4 deletions(-) create mode 100644 src/lib/__tests__/require-fresh-mfa-if-enrolled.test.ts diff --git a/src/app/api/settings/account/route.ts b/src/app/api/settings/account/route.ts index d5f0d3da3..28d6e584d 100644 --- a/src/app/api/settings/account/route.ts +++ b/src/app/api/settings/account/route.ts @@ -3,7 +3,11 @@ import { auditLog } from "@/lib/auth/audit"; import { apiSuccess, apiError, getClientIp } from "@/lib/api-response"; import { destroyAllSessions } from "@/lib/auth/session"; import { NextRequest } from "next/server"; -import { apiHandler, requireAuth } from "@/lib/api-handler"; +import { + apiHandler, + requireFreshMfaIfEnrolled, + MFA_STEP_UP_MAX_AGE_SECONDS, +} from "@/lib/api-handler"; import { annotate } from "@/lib/logging/context"; export const dynamic = "force-dynamic"; @@ -11,9 +15,14 @@ export const dynamic = "force-dynamic"; /** * Permanently delete the user account and ALL associated data. * Cascading deletes in the schema handle all related records. + * + * v1.23 — for an account with a second factor active this requires a fresh + * step-up (within MFA_STEP_UP_MAX_AGE_SECONDS) in addition to the typed + * confirmation, so a hijacked live session cannot erase the record. Accounts + * without MFA are unaffected and keep the typed-confirmation-only contract. */ export const DELETE = apiHandler(async (request: NextRequest) => { - const { user } = await requireAuth(); + const { user } = await requireFreshMfaIfEnrolled(MFA_STEP_UP_MAX_AGE_SECONDS); let confirm = ""; try { diff --git a/src/app/api/settings/data/route.ts b/src/app/api/settings/data/route.ts index b537b765e..e938a843b 100644 --- a/src/app/api/settings/data/route.ts +++ b/src/app/api/settings/data/route.ts @@ -2,7 +2,11 @@ import { prisma } from "@/lib/db"; import { auditLog } from "@/lib/auth/audit"; import { apiSuccess, apiError, getClientIp } from "@/lib/api-response"; import { NextRequest } from "next/server"; -import { apiHandler, requireAuth } from "@/lib/api-handler"; +import { + apiHandler, + requireFreshMfaIfEnrolled, + MFA_STEP_UP_MAX_AGE_SECONDS, +} from "@/lib/api-handler"; import { annotate } from "@/lib/logging/context"; import { invalidateUserData } from "@/lib/cache/invalidate"; @@ -10,9 +14,13 @@ export const dynamic = "force-dynamic"; /** * Delete all user-owned health/integration data while keeping the account. + * + * v1.23 — gated by a fresh step-up for MFA-enrolled accounts (see the account + * deletion route); single-factor accounts keep the typed-confirmation-only + * contract. */ export const DELETE = apiHandler(async (request: NextRequest) => { - const { user } = await requireAuth(); + const { user } = await requireFreshMfaIfEnrolled(MFA_STEP_UP_MAX_AGE_SECONDS); let confirm = ""; try { diff --git a/src/lib/__tests__/require-fresh-mfa-if-enrolled.test.ts b/src/lib/__tests__/require-fresh-mfa-if-enrolled.test.ts new file mode 100644 index 000000000..a05a523a2 --- /dev/null +++ b/src/lib/__tests__/require-fresh-mfa-if-enrolled.test.ts @@ -0,0 +1,117 @@ +import { describe, it, expect, vi, beforeEach } from "vitest"; + +// Mirror require-fresh-mfa.test.ts mocking so the api-handler module loads +// without a live DB / request scope. +vi.mock("@/lib/db", () => ({ + prisma: { + session: { findUnique: vi.fn() }, + apiToken: { findUnique: vi.fn(), update: vi.fn() }, + user: { findUnique: vi.fn() }, + }, +})); + +vi.mock("@/lib/auth/session", () => ({ getSession: vi.fn() })); +vi.mock("@/lib/auth/hmac", () => ({ hashToken: vi.fn(() => "hash") })); +vi.mock("@/lib/auth/audit", () => ({ + auditLog: vi.fn().mockResolvedValue(undefined), +})); +vi.mock("@/lib/logging/transports", () => ({ emitIfSampled: vi.fn() })); + +const headersGet = vi.fn<(name: string) => string | null>(); +vi.mock("next/headers", () => ({ + headers: vi.fn(async () => ({ get: headersGet })), + cookies: vi.fn(async () => ({ + get: () => undefined, + set: () => {}, + delete: () => {}, + })), +})); + +import { + requireFreshMfaIfEnrolled, + StepUpRequiredError, + HttpError, + MFA_STEP_UP_MAX_AGE_SECONDS, +} from "../api-handler"; +import { getSession } from "@/lib/auth/session"; +import { prisma } from "@/lib/db"; +import { headers } from "next/headers"; +import { auditLog } from "@/lib/auth/audit"; + +const MFA_USER = { + id: "user-1", + role: "USER" as const, + username: "u", + totpConfirmedAt: new Date("2020-01-01"), +}; +const PLAIN_USER = { ...MFA_USER, totpConfirmedAt: null }; + +beforeEach(() => { + vi.resetAllMocks(); + headersGet.mockReturnValue(null); + // resetAllMocks wipes the inline mock factory implementation too; restore + // the headers() wrapper so requireAuth's Bearer branch can read the header. + vi.mocked(headers).mockImplementation( + async () => ({ get: headersGet }) as never, + ); + // resetAllMocks wipes the factory mockResolvedValue; auditLog is awaited + // via `.catch(...)` in the Bearer path, so it must return a thenable. + vi.mocked(auditLog).mockResolvedValue(undefined); +}); + +function mockCookieSession(user: unknown) { + vi.mocked(getSession).mockResolvedValue({ + session: { id: "sess-1", expiresAt: new Date(Date.now() + 1e6) }, + user, + } as never); +} + +describe("requireFreshMfaIfEnrolled", () => { + it("passes a non-MFA user straight through without a freshness read", async () => { + mockCookieSession(PLAIN_USER); + const ctx = await requireFreshMfaIfEnrolled(MFA_STEP_UP_MAX_AGE_SECONDS); + expect(ctx.user.id).toBe("user-1"); + expect(prisma.session.findUnique).not.toHaveBeenCalled(); + }); + + it("passes an MFA user with a fresh verification", async () => { + mockCookieSession(MFA_USER); + vi.mocked(prisma.session.findUnique).mockResolvedValue({ + mfaVerifiedAt: new Date(Date.now() - 60_000), + } as never); + const ctx = await requireFreshMfaIfEnrolled(MFA_STEP_UP_MAX_AGE_SECONDS); + expect(ctx.user.id).toBe("user-1"); + }); + + it("blocks an MFA user without a fresh verification", async () => { + mockCookieSession(MFA_USER); + vi.mocked(prisma.session.findUnique).mockResolvedValue({ + mfaVerifiedAt: null, + } as never); + await expect( + requireFreshMfaIfEnrolled(MFA_STEP_UP_MAX_AGE_SECONDS), + ).rejects.toBeInstanceOf(StepUpRequiredError); + }); + + it("a Bearer token can never satisfy step-up for an MFA-enrolled account", async () => { + // requireAuth resolves the Bearer caller (no cookie), but the step-up read + // is cookie-only — getSession returns null inside requireFreshMfa. + vi.mocked(getSession).mockResolvedValue(null as never); + headersGet.mockImplementation((n) => + n.toLowerCase() === "authorization" ? "Bearer hlk_xyz" : null, + ); + vi.mocked(prisma.apiToken.findUnique).mockResolvedValue({ + id: "tok-1", + userId: "user-1", + permissions: ["*"], + revoked: false, + expiresAt: null, + } as never); + vi.mocked(prisma.user.findUnique).mockResolvedValue(MFA_USER as never); + vi.mocked(prisma.apiToken.update).mockResolvedValue({} as never); + + await expect( + requireFreshMfaIfEnrolled(MFA_STEP_UP_MAX_AGE_SECONDS), + ).rejects.toBeInstanceOf(HttpError); + }); +}); diff --git a/src/lib/api-handler.ts b/src/lib/api-handler.ts index d0761558f..e95b4ca80 100644 --- a/src/lib/api-handler.ts +++ b/src/lib/api-handler.ts @@ -557,6 +557,34 @@ export async function requireFreshMfa( return { ...sessionData, mfaVerifiedAt: verifiedAt }; } +/** + * v1.23 — conditional step-up for destructive account actions. + * + * Resolves the caller with the standard `requireAuth()` (cookie OR Bearer). + * For an account WITHOUT a confirmed second factor the caller passes straight + * through — a single-factor user is intentionally unaffected, so account + * deletion / data reset keeps its existing typed-confirmation-only contract. + * For an account WITH MFA active (`totpConfirmedAt` set) it additionally runs + * `requireFreshMfa`, which is cookie-only by construction: an MFA-enrolled + * account's Bearer transport carries no `mfaVerifiedAt` and therefore cannot + * satisfy step-up, surfacing `StepUpRequiredError` (401, + * `errorCode: "auth.stepup.required"`) so the UI launches a re-verification. + * + * Gating only the MFA-enrolled cohort keeps the boundary structural: a + * hijacked live cookie session for an MFA user cannot nuke the record without + * a fresh factor, while users who never opted into MFA are not forced through + * a flow they have no way to complete. + */ +export async function requireFreshMfaIfEnrolled( + maxAgeSeconds: number, +): Promise { + const auth = await requireAuth(); + if (auth.user.totpConfirmedAt) { + await requireFreshMfa(maxAgeSeconds); + } + return auth; +} + /** * Report unhandled errors to GlitchTip (fire-and-forget). * Uses dynamic import to avoid circular dependencies and startup cost.