From 0991d754e4867581c83a2d446775e5829fd029b2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Bombeck?= Date: Sun, 19 Jul 2026 13:48:46 +0200 Subject: [PATCH] fix(auth): make the re-proved factor decide what an elevation reaches MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A step-up elevation minted from the account password satisfied the fresh-factor routes. That is a second-factor bypass: a stolen token plus a known password could rotate the recovery codes — which take no factor in the body — and then spend one of them to disable the second factor outright. The web has never allowed that chain, because a password login does not stamp mfaVerifiedAt. The method is now an authorisation input. Only totp, webauthn, and passkey satisfy a fresh-factor gate, which is exactly the set of ceremonies the web stamps a session for; password reaches what a plain cookie session reaches and stops. The mint gains TOTP and security-key arms so a TOTP-only account can still manage its factor natively, and the response says which reach it bought. The gate also returns a discriminated context. session.id meant a session row on one arm and a token row on the other, and the disable route passed the token id to a session-scoped query — excluding nothing, so it deleted every browser session and revoked the caller's own device login. The transport is now in the type and the compiler refuses the confusion. Redemption splits into validate-then-commit so a 429, a 422, or a wrong code no longer burns a proof against a five-per-fifteen-minute mint ceiling. The claim is UPDATE ... RETURNING, so the statement that decides is the one that reports. The mint is transactional behind a partial unique index. Sign-out-everywhere and both password-reset paths clear elevations. The status read drops to plain requireAuth — it carries no credential material — and every mutation stays gated. --- .../migration.sql | 8 + prisma/schema.prisma | 7 + src/__tests__/step-up-elevation-guard.test.ts | 218 +++++-- src/app/api/auth/me/mfa/disable/route.ts | 17 +- .../me/mfa/recovery-codes/regenerate/route.ts | 5 +- src/app/api/auth/me/mfa/route.ts | 17 +- src/app/api/auth/me/mfa/totp/confirm/route.ts | 20 +- src/app/api/auth/me/mfa/totp/setup/route.ts | 6 +- .../api/auth/me/mfa/webauthn/[id]/route.ts | 12 +- .../me/mfa/webauthn/register/options/route.ts | 5 +- .../me/mfa/webauthn/register/verify/route.ts | 7 +- src/app/api/auth/me/sessions/route.ts | 5 +- src/app/api/auth/step-up/options/route.ts | 44 +- src/app/api/auth/step-up/route.ts | 72 ++- src/lib/api-handler.ts | 160 +++-- src/lib/auth/__tests__/step-up.test.ts | 178 ++++-- src/lib/auth/session.ts | 37 +- src/lib/auth/step-up.ts | 329 +++++++---- src/lib/idempotency.ts | 2 +- src/lib/jobs/reminder/cleanup-handlers.ts | 30 + src/lib/jobs/reminder/register-maintenance.ts | 15 + src/lib/validations/step-up.ts | 45 +- .../sessions-and-known-devices.test.ts | 5 +- tests/integration/step-up-elevation.test.ts | 557 +++++++++++++++++- 24 files changed, 1496 insertions(+), 305 deletions(-) rename prisma/migrations/{0256_step_up_elevation => 0260_step_up_elevation}/migration.sql (78%) diff --git a/prisma/migrations/0256_step_up_elevation/migration.sql b/prisma/migrations/0260_step_up_elevation/migration.sql similarity index 78% rename from prisma/migrations/0256_step_up_elevation/migration.sql rename to prisma/migrations/0260_step_up_elevation/migration.sql index c16c4f3da..bf827994a 100644 --- a/prisma/migrations/0256_step_up_elevation/migration.sql +++ b/prisma/migrations/0260_step_up_elevation/migration.sql @@ -19,6 +19,14 @@ CREATE TABLE "step_up_elevations" ( ); CREATE UNIQUE INDEX "step_up_elevations_token_hash_key" ON "step_up_elevations"("token_hash"); + +-- At most ONE redeemable elevation per token, enforced by the database rather +-- than by the mint's delete-then-create ordering. Without it two concurrent +-- mints can interleave so that both rows survive, or the second mint's delete +-- strands the value the first mint already handed to the client. +CREATE UNIQUE INDEX "step_up_elevations_live_per_token_key" + ON "step_up_elevations"("api_token_id") WHERE "consumed_at" IS NULL; + CREATE INDEX "step_up_elevations_user_id_idx" ON "step_up_elevations"("user_id"); CREATE INDEX "step_up_elevations_api_token_id_idx" ON "step_up_elevations"("api_token_id"); CREATE INDEX "step_up_elevations_expires_at_idx" ON "step_up_elevations"("expires_at"); diff --git a/prisma/schema.prisma b/prisma/schema.prisma index 9a645a2c4..a43118859 100644 --- a/prisma/schema.prisma +++ b/prisma/schema.prisma @@ -3159,6 +3159,13 @@ model StepUpElevation { @@index([userId]) @@index([apiTokenId]) @@index([expiresAt]) + // NOTE: migration 0260 additionally creates a PARTIAL unique index — + // `(api_token_id) WHERE consumed_at IS NULL` — so at most one elevation per + // token is ever redeemable. Prisma cannot express a partial index, and a + // plain `@@unique([apiTokenId])` would be wrong: it would also forbid minting + // a second elevation after the first was legitimately consumed. The + // constraint therefore lives only in the migration, and the mint below + // handles its unique violation explicitly. @@map("step_up_elevations") } diff --git a/src/__tests__/step-up-elevation-guard.test.ts b/src/__tests__/step-up-elevation-guard.test.ts index 89c4a71d1..7b0581c8f 100644 --- a/src/__tests__/step-up-elevation-guard.test.ts +++ b/src/__tests__/step-up-elevation-guard.test.ts @@ -7,10 +7,20 @@ * unable to grow by accident. Behavioural tests prove the mechanism works; * these prove nobody quietly pointed it somewhere else. * - * Like the Bearer-scope guards next door, these are tripwires rather than - * proofs. They cannot show the allowlist is correct — only that it has not - * changed without someone editing this file, which is where the reviewer's - * attention is supposed to land. + * MATCHING IS ON THE IMPORT, NOT THE CALL. An earlier revision grepped for + * `requireMfaManagementAuth(` and was evadable in one line: + * + * import { requireMfaManagementAuth as gate } from "@/lib/api-handler"; + * const auth = await gate(); + * + * A call-shaped regex sees nothing there. An import-shaped one cannot be dodged + * that way, because the binding has to be imported from the module under some + * local name before it can be called at all. The helpers below parse the import + * specifier — including its `as` alias — and check calls against the LOCAL name, + * so a rename is followed rather than lost. + * + * They remain tripwires, not proofs. They cannot show an allowlist is correct, + * only that it has not changed without someone editing this file. */ import { describe, it, expect } from "vitest"; import { readFileSync } from "node:fs"; @@ -34,19 +44,65 @@ function read(rel: string): string { return readFileSync(join(SRC, rel), "utf8"); } -function filesMatching(re: RegExp): string[] { - return sourceFiles().filter((rel) => re.test(read(rel))); +/** Local names bound for `exportName` from any module matching `moduleRe`. */ +function importedAs( + rel: string, + exportName: string, + moduleRe: RegExp, +): string[] { + const src = read(rel); + const names: string[] = []; + for (const m of src.matchAll( + /import\s*\{([\s\S]*?)\}\s*from\s*["']([^"']+)["']/g, + )) { + const [, clause, specifier] = m; + if (!moduleRe.test(specifier)) continue; + for (const part of clause.split(",")) { + const piece = part.trim().replace(/^type\s+/, ""); + if (!piece) continue; + const aliased = piece.match( + /^([A-Za-z_$][\w$]*)\s+as\s+([A-Za-z_$][\w$]*)$/, + ); + if (aliased) { + if (aliased[1] === exportName) names.push(aliased[2]); + } else if (piece === exportName) { + names.push(piece); + } + } + } + return names; +} + +const API_HANDLER = /api-handler$/; +const STEP_UP_MODULE = /auth\/step-up$/; + +/** Files that import `exportName` from the module AND call it. */ +function callers(exportName: string, moduleRe: RegExp): string[] { + return sourceFiles() + .filter((rel) => { + const locals = importedAs(rel, exportName, moduleRe); + if (locals.length === 0) return false; + const src = read(rel); + return locals.some((name) => new RegExp(`\\b${name}\\s*\\(`).test(src)); + }) + .sort(); } /** * Every route an elevation can unlock. This is the whole set — the second-factor - * management surface and nothing besides. + * management MUTATIONS and nothing besides. * * What is deliberately ABSENT is as much the point as what is present: + * - `GET /api/auth/me/mfa`. The status read is plain `requireAuth()`: it + * carries no credential material (no secret, no code, no public key, no + * credential id — only names, dates, and a count), the web shows the same + * screen to any authenticated session, and `/api/auth/me` already exposes + * `totpConfirmedAt` over Bearer. Gating it would force a factor re-proof to + * render a screen and buy nothing; * - every `/api/admin/*` route. `requireAdmin` is cookie-only and does not * consult this mechanism at all; - * - the trusted-device routes. "Remember this browser" is a browser concept; - * a native client has no equivalent to manage; + * - the trusted-device routes. "Remember this browser" is a browser concept + * with no native equivalent to manage; * - `requireFreshMfaIfEnrolled`'s callers (password change, account deletion, * data reset, encrypted export, encryption-key rotation). Those are * destructive account actions, not second-factor management, and letting an @@ -54,7 +110,6 @@ function filesMatching(re: RegExp): string[] { * exists to prevent. */ const ELEVATION_ROUTES = [ - "app/api/auth/me/mfa/route.ts", "app/api/auth/me/mfa/disable/route.ts", "app/api/auth/me/mfa/recovery-codes/regenerate/route.ts", "app/api/auth/me/mfa/totp/confirm/route.ts", @@ -65,56 +120,98 @@ const ELEVATION_ROUTES = [ ].sort(); describe("S1 — the elevation-accepting route set is frozen", () => { - it("only the known MFA-management routes call the gate", () => { - const callers = filesMatching(/requireMfaManagementAuth\(/).filter( - // The gate's own definition names itself. + it("only the known MFA-management mutations call the gate", () => { + const found = callers("requireMfaManagementAuth", API_HANDLER).filter( (rel) => rel !== "lib/api-handler.ts", ); - expect(callers).toEqual(ELEVATION_ROUTES); + expect(found).toEqual(ELEVATION_ROUTES); + }); + + it("follows an aliased import rather than losing it", () => { + // Guards the guard. Without this the allowlist above is one rename away + // from meaningless, which is precisely how the previous revision was + // evadable. + const probe = ` + import { requireMfaManagementAuth as gate } from "@/lib/api-handler"; + const auth = await gate(); + `; + const names: string[] = []; + for (const m of probe.matchAll( + /import\s*\{([\s\S]*?)\}\s*from\s*["']([^"']+)["']/g, + )) { + for (const part of m[1].split(",")) { + const aliased = part + .trim() + .match(/^([A-Za-z_$][\w$]*)\s+as\s+([A-Za-z_$][\w$]*)$/); + if (aliased && aliased[1] === "requireMfaManagementAuth") { + names.push(aliased[2]); + } + } + } + expect(names).toEqual(["gate"]); + expect(new RegExp(`\\b${names[0]}\\s*\\(`).test(probe)).toBe(true); }); it("no admin route calls the gate", () => { - const admin = filesMatching(/requireMfaManagementAuth\(/).filter((rel) => - rel.startsWith("app/api/admin/"), + const admin = callers("requireMfaManagementAuth", API_HANDLER).filter( + (rel) => rel.startsWith("app/api/admin/"), ); expect(admin).toEqual([]); }); it("the old cookie-only gates no longer linger on the MFA surface", () => { - // A route that still called `requireCookieAuth` / `requireFreshMfa` - // directly would be outside the single gate, so its posture could drift - // from the rest of the set without tripping the allowlist above. for (const rel of ELEVATION_ROUTES) { - const src = read(rel); - expect(src).not.toMatch(/await requireCookieAuth\(/); - expect(src).not.toMatch(/await requireFreshMfa\(/); + expect(importedAs(rel, "requireCookieAuth", API_HANDLER)).toEqual([]); + expect(importedAs(rel, "requireFreshMfa", API_HANDLER)).toEqual([]); + } + }); + + it("every gated route spends its elevation", () => { + // `commitElevation` is what makes the proof single-use. A route that gates + // but never commits would leave the elevation redeemable for the rest of + // its window — the one fail-open the two-phase split introduces, closed + // here. + for (const rel of ELEVATION_ROUTES) { + expect(read(rel)).toMatch(/\.commitElevation\(\)/); } }); }); -describe("S2 — redemption happens in exactly one place", () => { - it("only the gate redeems an elevation", () => { - const redeemers = filesMatching(/redeemStepUpElevation\(/).filter( +describe("S2 — mint and redeem happen in exactly one place each", () => { + it("only the gate claims an elevation", () => { + const found = callers("claimStepUpElevation", STEP_UP_MODULE).filter( (rel) => rel !== "lib/auth/step-up.ts", ); - expect(redeemers).toEqual(["lib/api-handler.ts"]); + expect(found).toEqual(["lib/api-handler.ts"]); + }); + + it("only the gate validates one", () => { + const found = callers("validateStepUpElevation", STEP_UP_MODULE).filter( + (rel) => rel !== "lib/auth/step-up.ts", + ); + expect(found).toEqual(["lib/api-handler.ts"]); }); it("only the mint route mints one", () => { - const minters = filesMatching(/mintStepUpElevation\(/).filter( + const found = callers("mintStepUpElevation", STEP_UP_MODULE).filter( (rel) => rel !== "lib/auth/step-up.ts", ); - expect(minters).toEqual(["app/api/auth/step-up/route.ts"]); + expect(found).toEqual(["app/api/auth/step-up/route.ts"]); }); - it("no route reads the elevation header for itself", () => { - // The header is the gate's private input. A route reading it directly - // would be hand-rolling an authorisation decision outside the one place - // that is reviewed for it. - const readers = sourceFiles() - .filter((p) => p.endsWith("/route.ts")) - .filter((rel) => /["']x-step-up["']/i.test(read(rel))); - expect(readers).toEqual([]); + it("no file outside the gate reads the elevation header", () => { + // Both spellings — the literal string AND the exported constant — across + // every source file, not only `route.ts`. A helper could read it too. + const offenders = sourceFiles() + .filter((rel) => rel !== "lib/api-handler.ts") + .filter((rel) => { + const src = read(rel); + if (/["']x-step-up["']/i.test(src)) return true; + return ( + importedAs(rel, "STEP_UP_ELEVATION_HEADER", API_HANDLER).length > 0 + ); + }); + expect(offenders).toEqual([]); }); }); @@ -126,9 +223,10 @@ describe("S3 — requireAdmin stays cookie-only", () => { src.indexOf("export async function requireCookieAuth("), ); expect(body).toContain("await getSession()"); - // No Bearer resolution, and no elevation redemption, inside the admin gate. expect(body).not.toMatch(/authenticateBearer|resolveBearerToken/); - expect(body).not.toMatch(/redeemStepUpElevation|STEP_UP_ELEVATION_HEADER/); + expect(body).not.toMatch( + /claimStepUpElevation|validateStepUpElevation|STEP_UP_ELEVATION_HEADER|requireMfaManagementAuth/, + ); }); }); @@ -138,9 +236,11 @@ describe("S4 — the mint surface refuses a cookie", () => { "app/api/auth/step-up/route.ts", "app/api/auth/step-up/options/route.ts", ]) { - const src = read(rel); - expect(src).toMatch(/await requireBearerAuth\(\)/); - expect(src).not.toMatch(/requireAuth\(|requireCookieAuth\(/); + expect(importedAs(rel, "requireBearerAuth", API_HANDLER)).toEqual([ + "requireBearerAuth", + ]); + expect(importedAs(rel, "requireAuth", API_HANDLER)).toEqual([]); + expect(importedAs(rel, "requireCookieAuth", API_HANDLER)).toEqual([]); } }); @@ -153,3 +253,39 @@ describe("S4 — the mint surface refuses a cookie", () => { expect(body).not.toContain("getSession("); }); }); + +describe("S5 — a password proof cannot satisfy a fresh-factor gate", () => { + it("the fresh-factor set excludes password", async () => { + // The B1 invariant, pinned at the definition. The web stamps + // `mfaVerifiedAt` only for a completed second factor or a primary passkey + // login; the Bearer arm must not be looser. + const { FRESH_FACTOR_METHODS, isFreshFactorMethod } = + await import("@/lib/auth/step-up"); + expect([...FRESH_FACTOR_METHODS].sort()).toEqual([ + "passkey", + "totp", + "webauthn", + ]); + expect(isFreshFactorMethod("password")).toBe(false); + }); + + it("the destructive routes all request the fresh-factor arm", () => { + // Miss this on one route and a password-proved elevation tears down the + // second factor there. + for (const rel of [ + "app/api/auth/me/mfa/disable/route.ts", + "app/api/auth/me/mfa/recovery-codes/regenerate/route.ts", + "app/api/auth/me/mfa/webauthn/[id]/route.ts", + ]) { + expect(read(rel)).toMatch(/freshFactor:\s*true/); + } + }); + + it("the claim re-checks the factor rather than trusting the validation", () => { + const src = read("lib/auth/step-up.ts"); + const claim = src.slice( + src.indexOf("export async function claimStepUpElevation("), + ); + expect(claim).toMatch(/requireFreshFactor\s*&&\s*!isFreshFactorMethod/); + }); +}); diff --git a/src/app/api/auth/me/mfa/disable/route.ts b/src/app/api/auth/me/mfa/disable/route.ts index 144eb4c1b..50d22d12a 100644 --- a/src/app/api/auth/me/mfa/disable/route.ts +++ b/src/app/api/auth/me/mfa/disable/route.ts @@ -38,9 +38,8 @@ const DISABLE_WINDOW_MS = 15 * 60 * 1000; export const POST = apiHandler(async (req: Request) => { // Step-up gate first — throws StepUpRequiredError (401 + errorCode) if the // session is not freshly second-factor-verified. - const { user, session } = await requireMfaManagementAuth({ - freshFactor: true, - }); + const auth = await requireMfaManagementAuth({ freshFactor: true }); + const { user } = auth; const rl = await checkRateLimit( `mfa:disable:${user.id}`, @@ -85,6 +84,11 @@ export const POST = apiHandler(async (req: Request) => { throw new HttpError(401, "Invalid code"); } + // Every cheap check has passed and the teardown is next, so spend the + // elevation now. Placing it here rather than at the gate means a wrong code or + // a 429 above costs the caller nothing. + await auth.commitElevation(); + await prisma.$transaction(async (tx) => { await tx.user.update({ where: { id: user.id }, @@ -102,7 +106,12 @@ export const POST = apiHandler(async (req: Request) => { // every "remember this device" trusted device (all handled by // destroyOtherSessions), keeping only the caller's current session. A stale // session or a trusted-device cookie must not survive the factor's removal. - await destroyOtherSessions(user.id, session.id); + await destroyOtherSessions( + user.id, + auth.transport === "cookie" + ? { kind: "session", sessionId: auth.session.id } + : { kind: "accessToken", accessTokenHash: auth.accessTokenHash }, + ); await auditLog("auth.mfa.disabled", { userId: user.id, diff --git a/src/app/api/auth/me/mfa/recovery-codes/regenerate/route.ts b/src/app/api/auth/me/mfa/recovery-codes/regenerate/route.ts index e6c97c3d5..341515dc8 100644 --- a/src/app/api/auth/me/mfa/recovery-codes/regenerate/route.ts +++ b/src/app/api/auth/me/mfa/recovery-codes/regenerate/route.ts @@ -24,7 +24,8 @@ const REGEN_RATE_LIMIT = 5; const REGEN_WINDOW_MS = 15 * 60 * 1000; export const POST = apiHandler(async (req: Request) => { - const { user } = await requireMfaManagementAuth({ freshFactor: true }); + const auth = await requireMfaManagementAuth({ freshFactor: true }); + const { user } = auth; // Defence in depth — regeneration only makes sense for an active factor. if (!user.totpConfirmedAt) { @@ -44,6 +45,8 @@ export const POST = apiHandler(async (req: Request) => { return res; } + await auth.commitElevation(); + const recoveryCodes = await regenerateRecoveryCodes(user.id); await auditLog("auth.mfa.recovery_regenerated", { diff --git a/src/app/api/auth/me/mfa/route.ts b/src/app/api/auth/me/mfa/route.ts index df6c775f4..d283a10d8 100644 --- a/src/app/api/auth/me/mfa/route.ts +++ b/src/app/api/auth/me/mfa/route.ts @@ -3,12 +3,21 @@ * * Second-factor status for the security settings hub: whether TOTP is active, * how many recovery codes remain, and the registered WebAuthn security keys. - * Gated by `requireMfaManagementAuth`: a cookie session, or a Bearer token - * presenting a single-use step-up elevation. A token alone is not enough. + * Plain `requireAuth()` — a cookie session or a cookie-equivalent token. This + * read is NOT elevation-gated, and deliberately so: it returns metadata only + * (whether TOTP is on, how many recovery codes remain, and each key's name and + * dates) and no credential material at all — no secret, no code, no public key, + * no credential id. The web shows the same screen to any authenticated session, + * and `/api/auth/me` already exposes `totpConfirmedAt` over Bearer, so gating it + * would only force a factor re-proof to render a screen and buy nothing. + * + * Every MUTATION on this surface stays behind `requireMfaManagementAuth`. The + * argument-less `requireAuth()` keeps the fail-closed scope default, so a + * narrow-scope token gains no reach here. * * Returns only metadata: no secret, no recovery-code plaintext, no public key. */ -import { apiHandler, requireMfaManagementAuth } from "@/lib/api-handler"; +import { apiHandler, requireAuth } from "@/lib/api-handler"; import { apiSuccess } from "@/lib/api-response"; import { annotate } from "@/lib/logging/context"; import { prisma } from "@/lib/db"; @@ -17,7 +26,7 @@ import { countRemainingRecoveryCodes } from "@/lib/auth/mfa/recovery-codes"; export const dynamic = "force-dynamic"; export const GET = apiHandler(async () => { - const { user } = await requireMfaManagementAuth(); + const { user } = await requireAuth(); const [recoveryCodesRemaining, webauthn] = await Promise.all([ countRemainingRecoveryCodes(user.id), diff --git a/src/app/api/auth/me/mfa/totp/confirm/route.ts b/src/app/api/auth/me/mfa/totp/confirm/route.ts index 8bad88724..8e701bf04 100644 --- a/src/app/api/auth/me/mfa/totp/confirm/route.ts +++ b/src/app/api/auth/me/mfa/totp/confirm/route.ts @@ -46,7 +46,8 @@ const CONFIRM_RATE_LIMIT = 10; const CONFIRM_WINDOW_MS = 15 * 60 * 1000; export const POST = apiHandler(async (req: Request) => { - const { user, session } = await requireMfaManagementAuth(); + const auth = await requireMfaManagementAuth(); + const { user } = auth; const rl = await checkRateLimit( `mfa:confirm:${user.id}`, @@ -103,6 +104,10 @@ export const POST = apiHandler(async (req: Request) => { const recoveryCodes = generateRecoveryCodes(); + // The code verified and the activation is next — spend the elevation here so + // a wrong code above costs the caller nothing. + await auth.commitElevation(); + await prisma.$transaction(async (tx) => { await tx.user.update({ where: { id: user.id }, @@ -115,11 +120,14 @@ export const POST = apiHandler(async (req: Request) => { await tx.mfaRecoveryCode.deleteMany({ where: { userId: user.id } }); await persistRecoveryCodes(tx, user.id, recoveryCodes); // Stamp the completing session so the user can immediately manage the - // factor under step-up without re-verifying. - await tx.session.updateMany({ - where: { id: session.id }, - data: { mfaVerifiedAt: new Date() }, - }); + // factor under step-up without re-verifying. Cookie transport only — a + // Bearer caller has no session row, and mints a fresh elevation per action. + if (auth.transport === "cookie") { + await tx.session.updateMany({ + where: { id: auth.session.id }, + data: { mfaVerifiedAt: new Date() }, + }); + } }); // v1.23 — the account now has an active second factor, so any diff --git a/src/app/api/auth/me/mfa/totp/setup/route.ts b/src/app/api/auth/me/mfa/totp/setup/route.ts index 662aa2206..a9ef6b5ed 100644 --- a/src/app/api/auth/me/mfa/totp/setup/route.ts +++ b/src/app/api/auth/me/mfa/totp/setup/route.ts @@ -33,7 +33,8 @@ const SETUP_RATE_LIMIT = 5; const SETUP_WINDOW_MS = 15 * 60 * 1000; export const POST = apiHandler(async (req: Request) => { - const { user } = await requireMfaManagementAuth(); + const auth = await requireMfaManagementAuth(); + const { user } = auth; const rl = await checkRateLimit( `mfa:setup:${user.id}`, @@ -56,6 +57,9 @@ export const POST = apiHandler(async (req: Request) => { throw new HttpError(409, "A second factor is already active"); } + // Rate limit and the already-active check have passed; the write is next. + await auth.commitElevation(); + const secret = generateTotpSecret(); const account = user.email ?? user.username; const otpauthUri = buildOtpauthUri(secret, account); diff --git a/src/app/api/auth/me/mfa/webauthn/[id]/route.ts b/src/app/api/auth/me/mfa/webauthn/[id]/route.ts index 6aab56136..61af3c671 100644 --- a/src/app/api/auth/me/mfa/webauthn/[id]/route.ts +++ b/src/app/api/auth/me/mfa/webauthn/[id]/route.ts @@ -28,7 +28,8 @@ export const PATCH = apiHandler( request: NextRequest, { params }: { params: Promise<{ id: string }> }, ) => { - const { user } = await requireMfaManagementAuth(); + const auth = await requireMfaManagementAuth(); + const { user } = auth; const { id } = await params; const { data: body, error: jsonError } = await safeJson(request, { @@ -49,6 +50,8 @@ export const PATCH = apiHandler( return apiError("Security key not found", 404); } + await auth.commitElevation(); + const updated = await prisma.webauthnMfaCredential.update({ where: { id }, data: { name: parsed.data.name }, @@ -67,7 +70,8 @@ export const DELETE = apiHandler( ) => { // Step-up gate first — throws StepUpRequiredError (401 + errorCode) if the // session is not freshly second-factor-verified. - const { user } = await requireMfaManagementAuth({ freshFactor: true }); + const auth = await requireMfaManagementAuth({ freshFactor: true }); + const { user } = auth; const { id } = await params; const existing = await prisma.webauthnMfaCredential.findUnique({ @@ -78,6 +82,10 @@ export const DELETE = apiHandler( return apiError("Security key not found", 404); } + // Ownership resolved and the row is about to go — spend the elevation now, + // so a 404 for someone else's key id does not burn it. + await auth.commitElevation(); + await prisma.webauthnMfaCredential.delete({ where: { id } }); await auditLog("auth.mfa.webauthn.remove", { diff --git a/src/app/api/auth/me/mfa/webauthn/register/options/route.ts b/src/app/api/auth/me/mfa/webauthn/register/options/route.ts index 109acbbf3..a472d8d55 100644 --- a/src/app/api/auth/me/mfa/webauthn/register/options/route.ts +++ b/src/app/api/auth/me/mfa/webauthn/register/options/route.ts @@ -15,7 +15,8 @@ import { createMfaRegistrationOptions } from "@/lib/auth/mfa/webauthn"; export const dynamic = "force-dynamic"; export const POST = apiHandler(async () => { - const { user } = await requireMfaManagementAuth(); + const auth = await requireMfaManagementAuth(); + const { user } = auth; const rl = await checkRateLimit( `mfa:webauthn:register:${user.id}`, @@ -30,6 +31,8 @@ export const POST = apiHandler(async () => { return res; } + await auth.commitElevation(); + const { options, challengeId } = await createMfaRegistrationOptions( user.id, user.email ?? user.username, diff --git a/src/app/api/auth/me/mfa/webauthn/register/verify/route.ts b/src/app/api/auth/me/mfa/webauthn/register/verify/route.ts index b3b651ca3..371e91174 100644 --- a/src/app/api/auth/me/mfa/webauthn/register/verify/route.ts +++ b/src/app/api/auth/me/mfa/webauthn/register/verify/route.ts @@ -24,7 +24,8 @@ import { setMfaEnrollCookie } from "@/lib/auth/mfa-enrollment"; export const dynamic = "force-dynamic"; export const POST = apiHandler(async (request: NextRequest) => { - const { user } = await requireMfaManagementAuth(); + const auth = await requireMfaManagementAuth(); + const { user } = auth; const { data: body, error: jsonError } = await safeJson(request, { maxBytes: 64 * 1024, @@ -46,6 +47,10 @@ export const POST = apiHandler(async (request: NextRequest) => { return apiError("Security key verification failed", 400); } + // The attestation verified and the credential is about to be stored — a + // failed ceremony above must not have cost the caller their elevation. + await auth.commitElevation(); + const { registrationInfo } = verification; const created = await prisma.webauthnMfaCredential.create({ data: { diff --git a/src/app/api/auth/me/sessions/route.ts b/src/app/api/auth/me/sessions/route.ts index 440f4d44a..24f41a095 100644 --- a/src/app/api/auth/me/sessions/route.ts +++ b/src/app/api/auth/me/sessions/route.ts @@ -88,7 +88,10 @@ export const GET = apiHandler(async () => { export const DELETE = apiHandler(async (request: NextRequest) => { const { user, session } = await requireAuth(); - const { sessionsRevoked } = await destroyOtherSessions(user.id, session.id); + const { sessionsRevoked } = await destroyOtherSessions(user.id, { + kind: "session", + sessionId: session.id, + }); await auditLog("auth.session.revoke_others", { userId: user.id, diff --git a/src/app/api/auth/step-up/options/route.ts b/src/app/api/auth/step-up/options/route.ts index f9caae99d..30392af0c 100644 --- a/src/app/api/auth/step-up/options/route.ts +++ b/src/app/api/auth/step-up/options/route.ts @@ -1,23 +1,30 @@ /** * POST /api/auth/step-up/options * - * Begin a primary-passkey assertion for the step-up mint below. Returns - * SimpleWebAuthn assertion options scoped to the calling account's registered - * passkeys, plus the server-issued challenge id to present back at - * `POST /api/auth/step-up`. + * Begin an assertion ceremony for the step-up mint. Two kinds: + * + * `passkey` — against the account's PRIMARY passkeys. At parity with + * `/api/auth/passkey/login-verify`, which the web treats as a + * second-factor-grade proof. + * `webauthn` — against the account's registered SECOND-FACTOR security keys. + * + * Returns SimpleWebAuthn assertion options plus the server-issued challenge id + * to present back at `POST /api/auth/step-up`. * * Bearer-only (`requireBearerAuth`) — a browser has no use for this surface; it * re-proves a factor at login and carries the result on its session row. The - * allow-list is never empty by construction: an account with no passkey gets a - * 409 telling the client to use the password arm instead. + * allow-list is never empty by construction: an account with no credential of + * the requested kind gets a 409 so the client can offer another arm. */ import { NextRequest } from "next/server"; import { apiHandler, requireBearerAuth } from "@/lib/api-handler"; -import { apiError, apiSuccess } from "@/lib/api-response"; +import { apiError, apiSuccess, safeJson } from "@/lib/api-response"; import { annotate } from "@/lib/logging/context"; import { prisma } from "@/lib/db"; import { checkAuthSurfaceRateLimit, rateLimitHeaders } from "@/lib/rate-limit"; import { createAuthenticationOptions } from "@/lib/auth/passkey"; +import { createMfaAuthenticationOptions } from "@/lib/auth/mfa/webauthn"; +import { stepUpOptionsSchema } from "@/lib/validations/step-up"; export const dynamic = "force-dynamic"; @@ -36,6 +43,29 @@ export const POST = apiHandler(async (request: NextRequest) => { }); } + const { data: body, error: jsonError } = await safeJson(request, { + maxBytes: 4 * 1024, + }); + if (jsonError) return jsonError; + + const parsed = stepUpOptionsSchema.safeParse(body); + if (!parsed.success) { + return apiError("Invalid request", 422); + } + + if (parsed.data.method === "webauthn") { + const result = await createMfaAuthenticationOptions(user.id); + if (!result) { + annotate({ action: { name: "auth.stepup.webauthn.no_key" } }); + return apiError("No security key registered", 409); + } + annotate({ action: { name: "auth.stepup.webauthn.options" } }); + return apiSuccess({ + options: result.options, + challengeId: result.challengeId, + }); + } + const passkeyCount = await prisma.passkey.count({ where: { userId: user.id }, }); diff --git a/src/app/api/auth/step-up/route.ts b/src/app/api/auth/step-up/route.ts index ec1e6e228..7744cc320 100644 --- a/src/app/api/auth/step-up/route.ts +++ b/src/app/api/auth/step-up/route.ts @@ -4,6 +4,15 @@ * Re-prove a factor and receive a single-use, token-bound step-up elevation for * one second-factor-management call. * + * WHICH factor is re-proved decides WHAT the elevation reaches. `password` + * reaches exactly what a plain cookie session reaches. `totp`, `webauthn`, and + * `passkey` additionally satisfy the fresh-factor routes — disable, recovery-code + * rotation, security-key removal — and that set is precisely the set of + * ceremonies for which the web stamps a session second-factor-verified. If a + * password-proved elevation opened those routes, a stolen token plus the account + * password would rotate the recovery codes (which take no factor in the body) + * and then spend one of them to disable the second factor outright. + * * This is the whole point of the mechanism, so it is worth being blunt about * what it is not: it is NOT a way to turn a token into a stronger credential. * Presenting the token gets you as far as this endpoint and no further — the @@ -37,8 +46,11 @@ import { } from "@/lib/rate-limit"; import { verifyPassword } from "@/lib/auth/password"; import { verifyAuthentication } from "@/lib/auth/passkey"; +import { verifyMfaAuthentication } from "@/lib/auth/mfa/webauthn"; +import { verifyMfaFactor } from "@/lib/auth/mfa/verify-factor"; import { mintStepUpElevation, + isFreshFactorMethod, STEP_UP_ELEVATION_TTL_SECONDS, type StepUpMethod, } from "@/lib/auth/step-up"; @@ -103,32 +115,62 @@ export const POST = apiHandler(async (request: NextRequest) => { return apiError("Invalid request", 422); } + const method: StepUpMethod = parsed.data.method; let proved = false; - let method: StepUpMethod = parsed.data.method; // Audit-only detail. Never reaches the response — see the file header. let failure = "invalid"; + /** + * Resolve a WebAuthn challenge and confirm it belongs to THIS account before + * any verification runs. Both ceremony helpers resolve a challenge by id + * alone, so without this a challenge minted for another account could be + * carried into the ceremony. + */ + const challengeBelongsToCaller = async (id: string): Promise => { + const row = await prisma.authChallenge.findUnique({ + where: { id }, + select: { userId: true }, + }); + return Boolean(row && row.userId === user.id); + }; + if (parsed.data.method === "password") { if (!user.passwordHash) { // An SSO-provisioned account has no password to re-prove. Same refusal as // a wrong one: the account's credential shape is not a token holder's to // enumerate. `/api/auth/step-up/options` is the honest discovery path — - // it 409s when there is no passkey either. + // it 409s when there is no credential of the requested kind either. failure = "no_password"; } else { proved = await verifyPassword(user.passwordHash, parsed.data.password); if (!proved) failure = "bad_password"; } + } else if (parsed.data.method === "totp") { + // The shared factor verifier, so the replay guard and the accepted-step + // burn behave exactly as they do at login and at MFA-disable. A code spent + // here cannot be replayed there. + const result = await verifyMfaFactor(user, "totp", parsed.data.code); + proved = result.ok; + if (!proved) failure = result.replay ? "totp_replay" : "bad_totp"; + } else if (parsed.data.method === "webauthn") { + if (!(await challengeBelongsToCaller(parsed.data.challengeId))) { + failure = "foreign_challenge"; + } else { + try { + // Scoped to the caller's own second-factor credentials by the verifier. + proved = await verifyMfaAuthentication( + parsed.data.challengeId, + user.id, + parsed.data.credential, + ); + if (!proved) failure = "bad_assertion"; + } catch { + // A malformed / expired challenge is a failed attempt, not a 500. + failure = "bad_assertion"; + } + } } else { - method = "passkey"; - // Bind the challenge to the caller BEFORE verifying. `verifyAuthentication` - // resolves a challenge by id alone; pinning ownership here means a challenge - // minted for another account cannot be carried into this ceremony. - const challenge = await prisma.authChallenge.findUnique({ - where: { id: parsed.data.challengeId }, - select: { userId: true }, - }); - if (!challenge || challenge.userId !== user.id) { + if (!(await challengeBelongsToCaller(parsed.data.challengeId))) { failure = "foreign_challenge"; } else { try { @@ -142,7 +184,6 @@ export const POST = apiHandler(async (request: NextRequest) => { result.verification.verified && result.passkey.userId === user.id; if (!proved) failure = "bad_assertion"; } catch { - // A malformed / expired challenge is a failed attempt, not a 500. failure = "bad_assertion"; } } @@ -184,5 +225,12 @@ export const POST = apiHandler(async (request: NextRequest) => { elevation: token, expiresAt: expiresAt.toISOString(), expiresInSeconds: STEP_UP_ELEVATION_TTL_SECONDS, + method, + // Whether this elevation reaches the fresh-factor routes (disable, + // recovery-code rotation, security-key removal). A password-proved + // elevation does not, exactly as a password login does not on the web. + // Surfaced so the client can pick the right ceremony up front instead of + // discovering the refusal after spending a proof. + satisfiesFreshFactor: isFreshFactorMethod(method), }); }); diff --git a/src/lib/api-handler.ts b/src/lib/api-handler.ts index df6d0bdd7..96b32e4a9 100644 --- a/src/lib/api-handler.ts +++ b/src/lib/api-handler.ts @@ -14,9 +14,11 @@ import { type ScopeRequirement, } from "./auth/bearer"; import { - redeemStepUpElevation, + claimStepUpElevation, + validateStepUpElevation, STEP_UP_ELEVATION_TTL_SECONDS, } from "./auth/step-up"; +import { hashToken } from "./auth/hmac"; import { AssistantDisabledError } from "./feature-flags"; import { ConsentRequiredError } from "./ai/consent-guard"; import { SCOPE_HEALTH_READ, SCOPE_HEALTH_WRITE } from "./mcp/oauth/config"; @@ -624,6 +626,27 @@ export async function requireFreshMfaIfEnrolled( return auth; } +/** + * A Bearer-resolved caller. + * + * `apiTokenId` and `accessTokenHash` are named rather than smuggled through + * `AuthContext.session.id`. That field means "the session row id" on the cookie + * path and "the ApiToken row id" on the Bearer path, and code that forgot which + * one it held passed a token id to a session-scoped query — deleting every one + * of the user's browser sessions and revoking the caller's own refresh token. + * Naming the fields is what stops that recurring. + */ +export interface BearerAuthContext extends AuthContext { + /** The `ApiToken` row id — the binding a step-up elevation is tied to. */ + apiTokenId: string; + /** + * HMAC of the presented access token, which is what `RefreshToken` + * cross-references in `accessTokenHash`. Lets a route identify the CALLER's + * own device login and spare it when revoking every other one. + */ + accessTokenHash: string; +} + /** * v1.30.34 — resolve a caller by Bearer token ONLY, refusing a cookie session. * @@ -639,7 +662,7 @@ export async function requireFreshMfaIfEnrolled( * a cookie-equivalent (`["*"]`) token is admitted. A narrow token — an MCP grant, * a medication-ingest grant — is refused 403 by the resolver. */ -export async function requireBearerAuth(): Promise { +export async function requireBearerAuth(): Promise { let authHeader: string | null = null; try { const headerList = await headers(); @@ -650,7 +673,13 @@ export async function requireBearerAuth(): Promise { if (!authHeader?.startsWith("Bearer ")) { throw new HttpError(401, "Not authenticated"); } - return await authenticateBearer(authHeader.slice(7), undefined); + const raw = authHeader.slice(7); + const auth = await authenticateBearer(raw, undefined); + return { + ...auth, + apiTokenId: auth.session.id, + accessTokenHash: hashToken(raw), + }; } /** @@ -660,10 +689,40 @@ export async function requireBearerAuth(): Promise { */ export const STEP_UP_ELEVATION_HEADER = "x-step-up"; +/** + * What an MFA-management route received, with the transport made explicit. + * + * The two arms carry DIFFERENT fields on purpose. A cookie caller has a session + * row and a Bearer caller does not, so `session` exists only on the cookie arm + * and the compiler refuses to read it on the other. This is the fix for a real + * defect: the MFA-disable route passed `session.id` to `destroyOtherSessions`, + * which on the Bearer path was an ApiToken id — matching no session row, so the + * "keep the current one" exclusion excluded nothing and the caller revoked its + * own device login. A comment would not have caught that; a type does. + */ +export type MfaManagementContext = { + user: User; + /** + * Spend the elevation. Call it immediately BEFORE the mutation, once every + * cheap validation has passed — a 429, a 422, or a wrong factor code must not + * burn a proof the user then has to mint again against a 5-per-15-minute + * ceiling. + * + * A no-op on the cookie arm (the session stamp is not consumable). On the + * Bearer arm it is the atomic single-use claim, and it THROWS + * `StepUpRequiredError` if the claim is lost — so a concurrent redemption + * still yields exactly one winner even though validation happened earlier. + */ + commitElevation: () => Promise; +} & ( + | { transport: "cookie"; session: { id: string; expiresAt: Date } } + | { transport: "bearer"; apiTokenId: string; accessTokenHash: string } +); + /** * v1.30.34 — the single gate for second-factor management. * - * Every MFA-management route goes through here and nothing else does. The set + * Every MFA-management MUTATION goes through here and nothing else does. The set * is frozen by `src/__tests__/step-up-elevation-guard.test.ts`, so a future * route cannot quietly join it: widening the reach of an elevation has to be a * visible edit to that allowlist. @@ -677,35 +736,48 @@ export const STEP_UP_ELEVATION_HEADER = "x-step-up"; * all. * * BEARER + ELEVATION — a token that resolves cleanly AND presents a valid, - * unconsumed elevation minted for that same token against a re-proved factor. - * The token alone is never enough: with no header, or a header that does not - * redeem, the caller gets the same `auth.stepup.required` a stale cookie - * session gets. + * unconsumed elevation minted for that same token against a re-proved factor + * of sufficient strength. The token alone is never enough. + * + * FRESH FACTOR IS ABOUT WHICH FACTOR, NOT JUST HOW RECENT. On the cookie path + * `requireFreshMfa` reads `Session.mfaVerifiedAt`, and only a completed second + * factor or a primary passkey login ever writes it — a password login does not. + * The Bearer arm holds the identical line through `FRESH_FACTOR_METHODS`: a + * password-proved elevation reaches what a plain cookie session reaches and + * stops there. Without that rule, a stolen token plus the account password could + * rotate the recovery codes and spend one to disable the second factor. * * `requireAdmin` is untouched and stays cookie-only. An elevation cannot reach * it — not because a check refuses one, but because `requireAdmin` resolves * through `getSession()` and never consults this function or the header. * - * @param options.freshFactor mirrors the cookie path's `requireFreshMfa`: the - * account must actually have a second factor enrolled. Set by the destructive - * routes (disable, recovery-code rotation, security-key removal). + * @param options.freshFactor mirrors the cookie path's `requireFreshMfa`. Set by + * the destructive routes (disable, recovery-code rotation, security-key + * removal). */ export async function requireMfaManagementAuth( options: { freshFactor?: boolean } = {}, -): Promise { +): Promise { + const freshFactor = options.freshFactor === true; + // Cookie first, and via the original helpers — the web path runs the same // code it always did. const sessionData = await getSession(); if (sessionData) { - return options.freshFactor + const resolved = freshFactor ? await requireFreshMfa(MFA_STEP_UP_MAX_AGE_SECONDS) : await requireCookieAuth(); + return { + transport: "cookie", + user: resolved.user, + session: resolved.session, + commitElevation: async () => {}, + }; } // Bearer path. Resolution first: an unknown, revoked, expired, or narrow-scope // token is refused here and never gets as far as presenting an elevation. const auth = await requireBearerAuth(); - const apiTokenId = auth.session.id; let raw: string | null = null; try { @@ -718,36 +790,43 @@ export async function requireMfaManagementAuth( if (!raw) { annotate({ action: { name: "auth.stepup.elevation.missing" }, - meta: { maxAgeSeconds: STEP_UP_ELEVATION_TTL_SECONDS }, + meta: { maxAgeSeconds: STEP_UP_ELEVATION_TTL_SECONDS, freshFactor }, }); throw new StepUpRequiredError(); } - const redeemed = await redeemStepUpElevation({ - rawToken: raw, - userId: auth.user.id, - apiTokenId, - }); - - if (!redeemed.ok) { + const refusal = (reason: string): StepUpRequiredError => { // One audit row with the machine reason, one generic refusal on the wire. // A prober learns only "not accepted" — never whether the elevation was - // unknown, already spent, expired, or minted for a different token. + // unknown, already spent, expired, minted for a different token, or minted + // from a factor too weak for this route. auditLog("auth.stepup.elevation.rejected", { userId: auth.user.id, - details: { reason: redeemed.reason }, + details: { reason, freshFactor }, }).catch(() => {}); annotate({ action: { name: "auth.stepup.elevation.rejected" }, - meta: { reason: redeemed.reason }, + meta: { reason, freshFactor }, }); - throw new StepUpRequiredError(); - } + // Returned rather than thrown so every call site reads `throw refusal(...)` + // and the compiler narrows the result union afterwards. + return new StepUpRequiredError(); + }; + + // Validate WITHOUT consuming. The route runs its own cheap checks next and + // spends the elevation only when it is about to act. + const validated = await validateStepUpElevation({ + rawToken: raw, + userId: auth.user.id, + apiTokenId: auth.apiTokenId, + requireFreshFactor: freshFactor, + }); + if (!validated.ok) throw refusal(validated.reason); // Parity with the cookie path: `requireFreshMfa` refuses an account with no // second factor enrolled, because a step-up-gated action is meaningless there. // The Bearer path holds the same line rather than becoming the softer route. - if (options.freshFactor) { + if (freshFactor) { let enrolled = Boolean(auth.user.totpConfirmedAt); if (!enrolled) { const keys = await prisma.webauthnMfaCredential.count({ @@ -760,12 +839,25 @@ export async function requireMfaManagementAuth( } } - annotate({ - action: { name: "auth.stepup.elevation.accepted" }, - meta: { method: redeemed.method }, - }); - - return auth; + return { + transport: "bearer", + user: auth.user, + apiTokenId: auth.apiTokenId, + accessTokenHash: auth.accessTokenHash, + commitElevation: async () => { + const claimed = await claimStepUpElevation({ + rawToken: raw, + userId: auth.user.id, + apiTokenId: auth.apiTokenId, + requireFreshFactor: freshFactor, + }); + if (!claimed.ok) throw refusal(claimed.reason); + annotate({ + action: { name: "auth.stepup.elevation.accepted" }, + meta: { method: claimed.method, freshFactor }, + }); + }, + }; } /** diff --git a/src/lib/auth/__tests__/step-up.test.ts b/src/lib/auth/__tests__/step-up.test.ts index 70a46d2b0..f37365ddb 100644 --- a/src/lib/auth/__tests__/step-up.test.ts +++ b/src/lib/auth/__tests__/step-up.test.ts @@ -1,23 +1,33 @@ /** * Unit-level checks on the elevation primitive that need no database. * - * The behavioural work — binding, single use, expiry, the concurrent race — - * lives in `tests/integration/step-up-elevation.test.ts`, because every one of - * those claims is a claim about what Postgres does with a WHERE clause and a - * mocked client cannot answer it. What is worth pinning here is the pair of - * constants that must not drift apart, and the shape of the value the mint - * hands out. + * The behavioural work — binding, single use, expiry, the concurrent race, the + * fresh-factor rule end to end — lives in + * `tests/integration/step-up-elevation.test.ts`, because every one of those + * claims is a claim about what Postgres does with a WHERE clause and a mocked + * client cannot answer it. What is worth pinning here is the arithmetic and the + * constants, both of which a mock CAN answer honestly. */ -import { describe, it, expect, vi } from "vitest"; +import { describe, it, expect, vi, beforeEach } from "vitest"; +import { readFileSync } from "node:fs"; + +const createMock = vi.fn().mockResolvedValue({ id: "elevation-1" }); vi.mock("@/lib/db", () => ({ prisma: { + $transaction: vi.fn(async (fn: (tx: unknown) => Promise) => + fn({ + stepUpElevation: { + deleteMany: vi.fn().mockResolvedValue({ count: 0 }), + create: createMock, + }, + }), + ), stepUpElevation: { - create: vi.fn().mockResolvedValue({ id: "elevation-1" }), deleteMany: vi.fn().mockResolvedValue({ count: 0 }), - updateMany: vi.fn().mockResolvedValue({ count: 0 }), findUnique: vi.fn().mockResolvedValue(null), }, + $queryRaw: vi.fn().mockResolvedValue([]), }, })); @@ -26,10 +36,32 @@ process.env.API_TOKEN_HMAC_KEY ??= const { mintStepUpElevation, - redeemStepUpElevation, + claimStepUpElevation, + validateStepUpElevation, + isFreshFactorMethod, + FRESH_FACTOR_METHODS, STEP_UP_ELEVATION_TTL_SECONDS, } = await import("@/lib/auth/step-up"); -const { MFA_STEP_UP_MAX_AGE_SECONDS } = await import("@/lib/api-handler"); +const { MFA_STEP_UP_MAX_AGE_SECONDS, STEP_UP_ELEVATION_HEADER } = + await import("@/lib/api-handler"); +const { hashToken } = await import("@/lib/auth/hmac"); + +beforeEach(() => { + createMock.mockClear(); +}); + +async function mint() { + return mintStepUpElevation({ + userId: "u1", + apiTokenId: "t1", + method: "password", + }); +} + +/** The `data` object the mint handed to Prisma. */ +function lastCreateData(): { tokenHash: string; expiresAt: Date } { + return createMock.mock.calls.at(-1)![0].data; +} describe("the two step-up windows stay pinned together", () => { it("matches the cookie path's freshness window exactly", () => { @@ -40,70 +72,114 @@ describe("the two step-up windows stay pinned together", () => { }); describe("the minted value", () => { - it("is a prefixed 32-byte CSPRNG secret, and the plaintext never reaches the row", async () => { - const { prisma } = await import("@/lib/db"); - const { token } = await mintStepUpElevation({ - userId: "u1", - apiTokenId: "t1", - method: "password", - }); - + it("is a prefixed 32-byte CSPRNG secret", async () => { + const { token } = await mint(); expect(token).toMatch(/^hle_[0-9a-f]{64}$/); + }); - const created = vi.mocked(prisma.stepUpElevation.create).mock.calls[0][0]; - const stored = (created.data as { tokenHash: string }).tokenHash; - expect(stored).not.toContain(token); - expect(stored).toMatch(/^[0-9a-f]{64}$/); + it("stores the HMAC and nothing derived from the plaintext", async () => { + const { token } = await mint(); + const { tokenHash } = lastCreateData(); + + // Asserted as an EQUALITY against the expected hash. The previous + // `not.toContain` comparison was vacuous — the stored value is 64 chars and + // the token is 68, so a 64-char string can never contain it, and storing + // `token.slice(4)` in cleartext would have passed. + expect(tokenHash).toBe(hashToken(token)); + expect(tokenHash).not.toBe(token); + expect(tokenHash).not.toBe(token.slice(4)); }); it("differs on every mint", async () => { - const a = await mintStepUpElevation({ - userId: "u1", - apiTokenId: "t1", - method: "password", - }); - const b = await mintStepUpElevation({ - userId: "u1", - apiTokenId: "t1", - method: "password", - }); + const a = await mint(); + const b = await mint(); expect(a.token).not.toBe(b.token); }); }); -describe("redemption rejects a wrong-shaped value before touching the database", () => { - it("refuses a value carrying another credential's prefix", async () => { - const { prisma } = await import("@/lib/db"); - vi.mocked(prisma.stepUpElevation.updateMany).mockClear(); +describe("the expiry that is actually written", () => { + it("lands five minutes ahead, not five minutes' worth of milliseconds", async () => { + // The returned `expiresInSeconds` is an echo of a constant and proves + // nothing about the row. Dropping the `* 1000` from the mint yields a + // 300-MILLISECOND elevation with every constant still reading 300, so the + // computed timestamp is what has to be asserted. + const before = Date.now(); + const { expiresAt } = await mint(); + const after = Date.now(); - // `hls_` is the session cookie secret and `hlk_` an access token. Neither - // is an elevation, and neither should cost a query to find that out. - for (const value of [ - `hls_${"a".repeat(64)}`, - `hlk_${"a".repeat(64)}`, - "", - ]) { - const result = await redeemStepUpElevation({ + const written = lastCreateData().expiresAt; + expect(written.getTime()).toBe(expiresAt.getTime()); + + const target = STEP_UP_ELEVATION_TTL_SECONDS * 1000; + expect(written.getTime() - before).toBeGreaterThanOrEqual(target - 1_000); + expect(written.getTime() - after).toBeLessThanOrEqual(target + 1_000); + // And unambiguously far from the millisecond-scale mistake. + expect(written.getTime() - before).toBeGreaterThan(60_000); + }); +}); + +describe("the fresh-factor rule", () => { + it("admits only the methods the web stamps a session for", () => { + expect([...FRESH_FACTOR_METHODS].sort()).toEqual([ + "passkey", + "totp", + "webauthn", + ]); + expect(isFreshFactorMethod("totp")).toBe(true); + expect(isFreshFactorMethod("webauthn")).toBe(true); + expect(isFreshFactorMethod("passkey")).toBe(true); + // The B1 invariant: a password is not a second factor. + expect(isFreshFactorMethod("password")).toBe(false); + }); +}); + +describe("a wrong-shaped value is refused before touching the database", () => { + it.each([ + ["session secret", `hls_${"a".repeat(64)}`], + ["access token", `hlk_${"a".repeat(64)}`], + ["empty", ""], + ])("refuses a %s without a query", async (_label, value) => { + const { prisma } = await import("@/lib/db"); + vi.mocked(prisma.$queryRaw).mockClear(); + vi.mocked(prisma.stepUpElevation.findUnique).mockClear(); + + for (const fn of [claimStepUpElevation, validateStepUpElevation]) { + const result = await fn({ rawToken: value, userId: "u1", apiTokenId: "t1", + requireFreshFactor: false, }); expect(result).toEqual({ ok: false, reason: "malformed" }); } - expect(prisma.stepUpElevation.updateMany).not.toHaveBeenCalled(); + expect(prisma.$queryRaw).not.toHaveBeenCalled(); + expect(prisma.stepUpElevation.findUnique).not.toHaveBeenCalled(); }); }); describe("the elevation header name is stable", () => { - it("is the lowercase form the gate reads", async () => { - const { STEP_UP_ELEVATION_HEADER } = await import("@/lib/api-handler"); - // Header lookups in the gate go through `headers().get()`, which is - // case-insensitive; the constant is lowercase so a direct Map-backed test - // jar matches it too. + it("is the lowercase form the gate reads", () => { expect(STEP_UP_ELEVATION_HEADER).toBe("x-step-up"); expect(STEP_UP_ELEVATION_HEADER).toBe( STEP_UP_ELEVATION_HEADER.toLowerCase(), ); }); }); + +describe("the credential prefix lists do not drift apart", () => { + it("the idempotency cache and the log redactor both know `hle_`", async () => { + // Two parallel denylists guard the same class of value. They were already + // one prefix out of step (`hls_` was in one and not the other), which is + // how the next one gets missed. + const idempotency = readFileSync("src/lib/idempotency.ts", "utf8"); + expect(idempotency).toContain("hle_"); + + const { redactSecrets } = await import("@/lib/logging/redact"); + const secret = `hle_${"a".repeat(64)}`; + expect(redactSecrets(`elevation=${secret}`)).not.toContain(secret); + expect(redactSecrets(`session=hls_${"b".repeat(64)}`)).toContain( + "[REDACTED]", + ); + }); +}); diff --git a/src/lib/auth/session.ts b/src/lib/auth/session.ts index 4cb7839b2..cdc22a74a 100644 --- a/src/lib/auth/session.ts +++ b/src/lib/auth/session.ts @@ -368,6 +368,16 @@ export async function destroyAllSessions(userId: string): Promise { ]); } +/** + * How the caller of a "revoke everything else" action authenticated. + * + * Exists so the caller's own credential can be spared without any code having + * to guess what kind of id it is holding — see `destroyOtherSessions`. + */ +export type CurrentCredential = + | { kind: "session"; sessionId: string } + | { kind: "accessToken"; accessTokenHash: string }; + /** * v1.23 — "sign out everywhere" for the user-facing active-session surface * (issue #64). Distinct from `destroyAllSessions`: this keeps the caller's @@ -382,20 +392,41 @@ export async function destroyAllSessions(userId: string): Promise { */ export async function destroyOtherSessions( userId: string, - currentSessionId: string, + current: CurrentCredential, ): Promise<{ sessionsRevoked: number }> { + // Which rows describe the CALLER depends on how the caller authenticated, and + // the two shapes are not interchangeable. A cookie caller is a `Session` row. + // A Bearer caller is an `ApiToken` whose device login is the `RefreshToken` + // carrying the matching `accessTokenHash` — it has no session row at all, so + // every session goes. Passing an ApiToken id in as a session id (which an + // earlier revision did) silently excluded nothing and revoked the caller's own + // refresh token, logging the calling device out at its next rotation. + const keptSessionId = + current.kind === "session" ? current.sessionId : undefined; + const keptRefreshWhere = + current.kind === "accessToken" + ? { accessTokenHash: { not: current.accessTokenHash } } + : {}; + const [deleted] = await prisma.$transaction([ prisma.session.deleteMany({ - where: { userId, id: { not: currentSessionId } }, + where: { + userId, + ...(keptSessionId ? { id: { not: keptSessionId } } : {}), + }, }), prisma.refreshToken.updateMany({ - where: { userId, revokedAt: null }, + where: { userId, revokedAt: null, ...keptRefreshWhere }, data: { revokedAt: new Date() }, }), // v1.23 — "sign out everywhere" also drops every trusted device so a // remembered browser can no longer skip the second factor (§1.7: a device // cookie is killed on sign-out-everywhere). prisma.trustedDevice.deleteMany({ where: { userId } }), + // v1.30.34 — and every step-up elevation. A user who signs out everywhere + // expects nothing to remain that could act on the account; a live elevation + // is exactly that. + prisma.stepUpElevation.deleteMany({ where: { userId } }), ]); return { sessionsRevoked: deleted.count }; } diff --git a/src/lib/auth/step-up.ts b/src/lib/auth/step-up.ts index 734c214c1..d3aa78834 100644 --- a/src/lib/auth/step-up.ts +++ b/src/lib/auth/step-up.ts @@ -16,36 +16,41 @@ * session; a token client has no session to stamp. An elevation is that proof, * made explicit and made disposable. * - * THE PROPERTIES, and why each one is load-bearing: + * WHICH FACTOR WAS PROVED IS PART OF THE PROOF. `method` is an authorisation + * input, not telemetry. The web stamps `mfaVerifiedAt` only for a completed + * SECOND factor (TOTP, security key) or a primary passkey login — never for a + * password. If a password-proved elevation satisfied the fresh-factor routes, + * a stolen token plus the account password would rotate the recovery codes + * (which take no factor in the body) and then spend one of them to disable the + * second factor outright. So the fresh-factor routes accept only + * `FRESH_FACTOR_METHODS`, and the password arm reaches exactly what a plain + * cookie session reaches — no more. + * + * THE OTHER PROPERTIES, and why each is load-bearing: * * Minted only against a re-proved factor. Presenting the token alone yields - * nothing — that is the entire point. A stolen token gains no more reach than - * it had before this module existed. + * nothing. A stolen token gains no more reach than it had before. * * Bound to one token (`apiTokenId`). An elevation minted by the phone cannot - * be redeemed by a second token, not even another of the same user's. Theft of - * the elevation in transit is therefore useless without also holding the exact - * token it was minted for. + * be redeemed by a second token, not even another of the same user's. * - * Single-use. Redemption is a conditional UPDATE (see `redeemStepUpElevation`) - * so two concurrent redemptions cannot both succeed. One management action per - * proof, by construction rather than by convention. + * Single-use. The claim is one conditional UPDATE, so two concurrent + * redemptions cannot both succeed. * - * Short-lived. `STEP_UP_ELEVATION_TTL_SECONDS` mirrors the cookie path's - * `MFA_STEP_UP_MAX_AGE_SECONDS` exactly, so both transports carry the same - * freshness promise and neither is the softer way in. + * Short-lived. `STEP_UP_ELEVATION_TTL_SECONDS` equals the cookie path's + * `MFA_STEP_UP_MAX_AGE_SECONDS`, so neither transport is the softer way in. * * Stored hashed. Only `hashToken(secret)` reaches the database — the same - * posture as ApiToken, RefreshToken, and the session secret. A table dump - * yields nothing redeemable, and the raw value exists only in the mint - * response and the client's memory. + * posture as ApiToken, RefreshToken, and the session secret. * - * INVALIDATION comes from two directions. A password change calls - * `revokeStepUpElevations` outright. A revoked or deleted token strands its - * elevations without any code here running: redemption sits behind Bearer - * resolution, which refuses a revoked token, and the FK cascades on delete. + * INVALIDATION comes from three directions. A password change (self-service or + * operator-forced) and a sign-out-everywhere both call + * `revokeStepUpElevations`. A revoked or deleted token strands its elevations + * without any code here running: redemption sits behind Bearer resolution, + * which refuses a revoked token, and the FK cascades on delete. */ import { randomBytes } from "node:crypto"; +import { Prisma } from "@/generated/prisma/client"; import { prisma } from "@/lib/db"; import { hashToken } from "@/lib/auth/hmac"; @@ -65,16 +70,48 @@ const ELEVATION_SECRET_BYTES = 32; * guarding the same actions should not offer two different freshness promises — * if they diverged, the looser one would simply become the way in. Five minutes * also sits at the tight end of the 5–15 minute band OWASP recommends for - * re-authentication, which is where destructive actions belong; it is long - * enough to type a code from an authenticator app and short enough that an - * elevation captured off a screen or a proxy log is stale before it is useful. + * re-authentication, which is where destructive actions belong. * * A test pins the two constants together so a future edit to one is caught. */ export const STEP_UP_ELEVATION_TTL_SECONDS = 5 * 60; -/** Which factor was re-proved. Recorded for audit; never an authorisation input. */ -export type StepUpMethod = "password" | "passkey"; +/** + * Which factor was re-proved. This is an AUTHORISATION INPUT — see + * `FRESH_FACTOR_METHODS` and the file header. + * + * password — the account password. Reaches what a plain cookie session + * reaches, and no further. + * totp — a current code from the enrolled authenticator. + * webauthn — an assertion from a registered second-factor security key. + * passkey — an assertion from a primary passkey. At parity with + * `/api/auth/passkey/login-verify`, which stamps `mfaVerifiedAt` + * on the web for the very same ceremony. + */ +export type StepUpMethod = "password" | "totp" | "webauthn" | "passkey"; + +/** + * The methods that satisfy a fresh-factor gate — exactly the set for which the + * web writes `mfaVerifiedAt`. `password` is deliberately absent: a password + * login has never satisfied step-up on the web and must not start here. + * + * A recovery code is also absent, and that IS a narrowing relative to the web, + * where `/api/auth/mfa/verify` accepts one and does stamp the session. The + * consequence is that an account which has lost its authenticator cannot + * disable its second factor from the app — only from the web. That asymmetry is + * deliberate and flagged rather than quietly closed, because admitting a + * recovery code here means spending one break-glass credential to authorise + * spending another. + */ +export const FRESH_FACTOR_METHODS: ReadonlySet = new Set([ + "totp", + "webauthn", + "passkey", +]); + +export function isFreshFactorMethod(method: StepUpMethod): boolean { + return FRESH_FACTOR_METHODS.has(method); +} export interface MintedElevation { /** The raw `hle_<64 hex>` value. Returned once; never persisted, never logged. */ @@ -89,10 +126,12 @@ export interface MintedElevation { * not check anything, it records a decision already made. Keep the two adjacent * so that stays obvious. * - * Any earlier unconsumed elevation for the same token is deleted first. A fresh - * proof supersedes an older one, so there is never more than one redeemable - * elevation per token; that both bounds the table and shrinks the window in - * which a captured elevation is worth anything. + * The delete-then-create runs in ONE transaction. Without it two concurrent + * mints can interleave so the second's delete removes the first's row after the + * first has already handed its value to the client, leaving a caller holding an + * elevation that can never redeem. The partial unique index from migration 0260 + * is the backstop: if two transactions still collide, one loses on the + * constraint and retries rather than both rows surviving. */ export async function mintStepUpElevation(opts: { userId: string; @@ -102,60 +141,135 @@ export async function mintStepUpElevation(opts: { const raw = `${ELEVATION_PREFIX}${randomBytes(ELEVATION_SECRET_BYTES).toString("hex")}`; const expiresAt = new Date(Date.now() + STEP_UP_ELEVATION_TTL_SECONDS * 1000); - await prisma.stepUpElevation.deleteMany({ - where: { apiTokenId: opts.apiTokenId, consumedAt: null }, - }); + const write = async () => + prisma.$transaction(async (tx) => { + // A fresh proof supersedes an older one, so there is never more than one + // redeemable elevation per token. That bounds the table and shrinks the + // window in which a captured elevation is worth anything. + await tx.stepUpElevation.deleteMany({ + where: { apiTokenId: opts.apiTokenId, consumedAt: null }, + }); + await tx.stepUpElevation.create({ + data: { + userId: opts.userId, + apiTokenId: opts.apiTokenId, + tokenHash: hashToken(raw), + method: opts.method, + expiresAt, + }, + select: { id: true }, + }); + }); - await prisma.stepUpElevation.create({ - data: { - userId: opts.userId, - apiTokenId: opts.apiTokenId, - tokenHash: hashToken(raw), - method: opts.method, - expiresAt, - }, - select: { id: true }, - }); - - // Opportunistic housekeeping — cheap, indexed, and keeps the table from - // accumulating spent rows without needing a scheduled job for a table that - // only ever sees a handful of rows per user per year. - prisma.stepUpElevation - .deleteMany({ where: { expiresAt: { lt: new Date() } } }) - .catch(() => {}); + try { + await write(); + } catch (err) { + // P2002 on the partial index: a concurrent mint won the race between our + // delete and our insert. One retry (whose delete removes that winner in + // turn) resolves it deterministically. + if ( + err instanceof Prisma.PrismaClientKnownRequestError && + err.code === "P2002" + ) { + await write(); + } else { + throw err; + } + } return { token: raw, expiresAt }; } -/** Why a redemption was refused. Audit detail only — never surfaced to a caller. */ -export type StepUpRedeemFailure = - "malformed" | "unknown" | "wrong_token" | "consumed" | "expired"; +/** Why an elevation was refused. Audit detail only — never surfaced to a caller. */ +export type StepUpRejection = + | "malformed" + | "unknown" + | "wrong_token" + | "consumed" + | "expired" + | "insufficient_factor"; -export type StepUpRedeemResult = - | { ok: true; method: StepUpMethod } - | { ok: false; reason: StepUpRedeemFailure }; +export type StepUpOutcome = + { ok: true; method: StepUpMethod } | { ok: false; reason: StepUpRejection }; + +/** + * Check an elevation WITHOUT consuming it. + * + * Split from the claim so a route can refuse a bad request — a 429, a malformed + * body, a wrong TOTP code — without burning the caller's elevation. Burning one + * on a validation failure was a self-inflicted lockout: the mint allows five + * attempts per fifteen minutes, so five fat-fingered codes would lock a user out + * of their own second-factor settings. + * + * This is NOT an authorisation decision on its own, and nothing may mutate on + * the strength of it. `claimStepUpElevation` still has to win the race before + * any effect happens; this only decides whether the request is worth processing. + */ +export async function validateStepUpElevation(opts: { + rawToken: string; + userId: string; + apiTokenId: string; + requireFreshFactor: boolean; +}): Promise { + if (!opts.rawToken.startsWith(ELEVATION_PREFIX)) { + return { ok: false, reason: "malformed" }; + } + + const row = await prisma.stepUpElevation.findUnique({ + where: { tokenHash: hashToken(opts.rawToken) }, + select: { + userId: true, + apiTokenId: true, + consumedAt: true, + expiresAt: true, + method: true, + }, + }); + + if (!row) return { ok: false, reason: "unknown" }; + if (row.userId !== opts.userId || row.apiTokenId !== opts.apiTokenId) { + return { ok: false, reason: "wrong_token" }; + } + if (row.consumedAt) return { ok: false, reason: "consumed" }; + if (row.expiresAt <= new Date()) return { ok: false, reason: "expired" }; + + const method = row.method as StepUpMethod; + if (opts.requireFreshFactor && !isFreshFactorMethod(method)) { + return { ok: false, reason: "insufficient_factor" }; + } + + return { ok: true, method }; +} /** * Consume an elevation, or refuse. * - * SINGLE-USE ATOMICITY. The claim is one conditional `UPDATE ... WHERE + * SINGLE-USE ATOMICITY. The claim is one conditional `UPDATE … WHERE * token_hash = $1 AND user_id = $2 AND api_token_id = $3 AND consumed_at IS NULL - * AND expires_at > $4` (Prisma's `updateMany`), and the caller proceeds only on - * `count === 1`. Postgres under READ COMMITTED serialises the two writers on the - * row lock: the loser re-evaluates the predicate against the winner's committed - * version, finds `consumed_at` no longer null, and matches zero rows. There is - * no read-then-write window to lose, which is exactly why the check-then-update + * AND expires_at > $4 RETURNING method`, and the caller proceeds only on exactly + * one returned row. Postgres under READ COMMITTED serialises the two writers on + * the row lock: the loser re-evaluates the predicate against the winner's + * committed version, finds `consumed_at` no longer null, and returns nothing. + * There is no read-then-write window to lose, which is why the check-then-update * shape was not used. * - * Both binding fields are in the predicate rather than checked afterwards, so a - * mismatched token cannot be claimed and then rejected — it is never claimed at - * all, and the legitimate holder's elevation survives someone else's attempt. + * `RETURNING method` rather than a follow-up SELECT: a separate read can miss, + * and any fallback for that miss would fabricate an authorisation-relevant + * value — logging a passkey-proved elevation as password-proved, or worse, + * deciding a gate on the invented one. The statement that decides is the + * statement that reports. + * + * The fresh-factor rule is enforced here, on the returned row, not only in + * `validateStepUpElevation`. That validation is advisory; this is the gate. When + * the method is insufficient the row stays consumed — it was a real elevation + * presented in a real redemption, so spending it is right. */ -export async function redeemStepUpElevation(opts: { +export async function claimStepUpElevation(opts: { rawToken: string; userId: string; apiTokenId: string; -}): Promise { + requireFreshFactor: boolean; +}): Promise { if (!opts.rawToken.startsWith(ELEVATION_PREFIX)) { return { ok: false, reason: "malformed" }; } @@ -163,56 +277,69 @@ export async function redeemStepUpElevation(opts: { const tokenHash = hashToken(opts.rawToken); const now = new Date(); - const claimed = await prisma.stepUpElevation.updateMany({ - where: { - tokenHash, - userId: opts.userId, - apiTokenId: opts.apiTokenId, - consumedAt: null, - expiresAt: { gt: now }, - }, - data: { consumedAt: now }, - }); + const claimed = await prisma.$queryRaw<{ method: string }[]>` + UPDATE step_up_elevations + SET consumed_at = ${now} + WHERE token_hash = ${tokenHash} + AND user_id = ${opts.userId} + AND api_token_id = ${opts.apiTokenId} + AND consumed_at IS NULL + AND expires_at > ${now} + RETURNING method + `; - if (claimed.count === 1) { - const row = await prisma.stepUpElevation.findUnique({ + if (claimed.length !== 1) { + // Classify for the audit row only. The caller returns one generic response + // for every branch, so this tells an operator what happened without telling + // a prober anything. + const existing = await prisma.stepUpElevation.findUnique({ where: { tokenHash }, - select: { method: true }, + select: { userId: true, apiTokenId: true, consumedAt: true }, }); - return { ok: true, method: (row?.method as StepUpMethod) ?? "password" }; + if (!existing) return { ok: false, reason: "unknown" }; + if ( + existing.userId !== opts.userId || + existing.apiTokenId !== opts.apiTokenId + ) { + return { ok: false, reason: "wrong_token" }; + } + if (existing.consumedAt) return { ok: false, reason: "consumed" }; + return { ok: false, reason: "expired" }; } - // Classify the refusal for the audit row only. The caller returns one generic - // response for every branch below, so this read tells an operator what - // happened without telling a prober anything. - const existing = await prisma.stepUpElevation.findUnique({ - where: { tokenHash }, - select: { - apiTokenId: true, - userId: true, - consumedAt: true, - expiresAt: true, - }, - }); - if (!existing) return { ok: false, reason: "unknown" }; - if ( - existing.userId !== opts.userId || - existing.apiTokenId !== opts.apiTokenId - ) { - return { ok: false, reason: "wrong_token" }; + const method = claimed[0].method as StepUpMethod; + if (opts.requireFreshFactor && !isFreshFactorMethod(method)) { + return { ok: false, reason: "insufficient_factor" }; } - if (existing.consumedAt) return { ok: false, reason: "consumed" }; - return { ok: false, reason: "expired" }; + + return { ok: true, method }; } /** * Drop every elevation for a user. * - * Called when the account password changes: an elevation is a statement about a - * credential that no longer exists, so it must not outlive it. Deliberately + * Called when the account password changes (self-service or operator-forced) + * and on sign-out-everywhere. An elevation is a statement about a credential and + * a moment, and neither survives either of those events. Deliberately * unconditional over the user's rows — consumed ones go too, since there is no * reason to keep them once the anchor moved. */ export async function revokeStepUpElevations(userId: string): Promise { await prisma.stepUpElevation.deleteMany({ where: { userId } }); } + +/** + * Delete every elevation whose window has closed. + * + * Runs on the queue, not on a request. The earlier fire-and-forget sweep on the + * mint path was a cross-tenant unbounded DELETE competing with the request for a + * pool connection, and recurring work belongs on pg-boss. An expired row is + * already unredeemable — the predicate in `claimStepUpElevation` refuses it — so + * this is hygiene, not enforcement, and a missed run breaks nothing. + */ +export async function pruneExpiredStepUpElevations(): Promise { + const { count } = await prisma.stepUpElevation.deleteMany({ + where: { expiresAt: { lt: new Date() } }, + }); + return count; +} diff --git a/src/lib/idempotency.ts b/src/lib/idempotency.ts index a10155bc2..ad0ba614a 100644 --- a/src/lib/idempotency.ts +++ b/src/lib/idempotency.ts @@ -426,7 +426,7 @@ export function withIdempotency< // or any other word containing `sk-` would otherwise // silently break idempotency for benign retries). const SECRET_PATTERN = - /(?:\b(?:hlk_|hlr_|hls_|hlv_)[A-Za-z0-9_-]+|\bsk-(?:ant-)?[A-Za-z0-9_-]{8,})/; + /(?:\b(?:hlk_|hlr_|hls_|hlv_|hle_)[A-Za-z0-9_-]+|\bsk-(?:ant-)?[A-Za-z0-9_-]{8,})/; const cloned = response.clone(); const text = await cloned.text(); if (!SECRET_PATTERN.test(text)) { diff --git a/src/lib/jobs/reminder/cleanup-handlers.ts b/src/lib/jobs/reminder/cleanup-handlers.ts index 105367bdb..bdbe007be 100644 --- a/src/lib/jobs/reminder/cleanup-handlers.ts +++ b/src/lib/jobs/reminder/cleanup-handlers.ts @@ -40,6 +40,10 @@ const PUSH_ATTEMPT_RETENTION_DAYS = 90; // the two never drift. Slots at 03:40 between push-attempt cleanup (03:35) // and the drain (03:45) inside the existing 03:xx maintenance window. +export interface StepUpElevationCleanupPayload { + _?: never; +} + export interface RateLimitCleanupPayload { triggeredAt: string; } @@ -193,6 +197,32 @@ export async function handleIdempotencyCleanup( }); } +/** + * v1.30.34 — sweep expired step-up elevations. + * + * Hygiene, not enforcement: an expired row is already unredeemable because + * `claimStepUpElevation`'s predicate refuses it, so a missed run costs nothing + * but table space. It lives here rather than fire-and-forget on the mint request + * because a cross-tenant unbounded DELETE has no business competing with a + * user's request for a pool connection. + */ +export async function handleStepUpElevationCleanup( + jobs: Job[], +) { + void jobs; + await withBackgroundEvent("job.step_up_elevation_cleanup", async (evt) => { + const p = getWorkerPrisma(); + try { + const { count } = await p.stepUpElevation.deleteMany({ + where: { expiresAt: { lt: new Date() } }, + }); + evt.addMeta("step_up_elevation_cleanup_deleted", count); + } catch (err) { + evt.addWarning(`step-up-elevation-cleanup failed: ${err}`); + } + }); +} + export async function handleAuditLogCleanup( jobs: Job[], ) { diff --git a/src/lib/jobs/reminder/register-maintenance.ts b/src/lib/jobs/reminder/register-maintenance.ts index c258a79ca..397ea802d 100644 --- a/src/lib/jobs/reminder/register-maintenance.ts +++ b/src/lib/jobs/reminder/register-maintenance.ts @@ -134,6 +134,7 @@ import { } from "./backup-handlers"; import { RateLimitCleanupPayload, + StepUpElevationCleanupPayload, IdempotencyCleanupPayload, AuditLogCleanupPayload, MoodReminderCleanupPayload, @@ -145,6 +146,7 @@ import { handleRateLimitCleanup, handleIdempotencyCleanup, handleAuditLogCleanup, + handleStepUpElevationCleanup, CoachMessageCleanupPayload, handleCoachMessageCleanup, McpTokenCleanupPayload, @@ -184,6 +186,12 @@ const IDEMPOTENCY_CLEANUP_QUEUE = "idempotency-cleanup"; const IDEMPOTENCY_CLEANUP_CRON = "0 3 * * *"; // daily at 03:00 (Europe/Berlin) +const STEP_UP_ELEVATION_CLEANUP_QUEUE = "step-up-elevation-cleanup"; + +// Every 15 minutes. An elevation lives five, so nothing lingers long; the +// cadence only has to be short enough that the table stays small. +const STEP_UP_ELEVATION_CLEANUP_CRON = "*/15 * * * *"; + const AUDIT_LOG_CLEANUP_QUEUE = "audit-log-cleanup"; const AUDIT_LOG_CLEANUP_CRON = "15 3 * * *"; // daily at 03:15 (Europe/Berlin) @@ -280,6 +288,7 @@ const allQueues = [ RATE_LIMIT_CLEANUP_QUEUE, IDEMPOTENCY_CLEANUP_QUEUE, AUDIT_LOG_CLEANUP_QUEUE, + STEP_UP_ELEVATION_CLEANUP_QUEUE, OFFHOST_BACKUP_QUEUE, RESTORE_DRILL_QUEUE, HOST_METRIC_QUEUE, @@ -380,6 +389,7 @@ const schedules: ScheduleEntry[] = [ [RATE_LIMIT_CLEANUP_QUEUE, RATE_LIMIT_CLEANUP_CRON], [IDEMPOTENCY_CLEANUP_QUEUE, IDEMPOTENCY_CLEANUP_CRON], [AUDIT_LOG_CLEANUP_QUEUE, AUDIT_LOG_CLEANUP_CRON], + [STEP_UP_ELEVATION_CLEANUP_QUEUE, STEP_UP_ELEVATION_CLEANUP_CRON], [OFFHOST_BACKUP_QUEUE, OFFHOST_BACKUP_CRON], [RESTORE_DRILL_QUEUE, RESTORE_DRILL_CRON], [HOST_METRIC_QUEUE, HOST_METRIC_CRON], @@ -554,6 +564,11 @@ export async function registerMaintenanceQueues( { localConcurrency: 1 }, handleAuditLogCleanup, ); + await boss.work( + STEP_UP_ELEVATION_CLEANUP_QUEUE, + { localConcurrency: 1 }, + handleStepUpElevationCleanup, + ); await boss.work( OFFHOST_BACKUP_QUEUE, { localConcurrency: 1 }, diff --git a/src/lib/validations/step-up.ts b/src/lib/validations/step-up.ts index c167bab42..96028cd6e 100644 --- a/src/lib/validations/step-up.ts +++ b/src/lib/validations/step-up.ts @@ -7,14 +7,20 @@ import { z } from "zod/v4"; import { webauthnCredentialSchema } from "@/lib/validations/mfa"; /** - * Re-prove a factor to mint an elevation. + * Re-prove a factor to mint an elevation. Four accepted proofs, discriminated on + * `method`, and the choice is an AUTHORISATION input rather than a preference: * - * Two accepted proofs, discriminated on `method`: - * - `password` — the account password. Unavailable on an account that signed - * in through OIDC SSO and never set one; such an account uses the passkey - * arm, or manages its second factor on the web. - * - `passkey` — an assertion against a primary passkey the device already - * holds, begun at `/api/auth/step-up/options`. + * - `password` — the account password. Reaches exactly what a plain cookie + * session reaches. Unavailable on an account provisioned through OIDC SSO + * that never set one. + * - `totp` — a current code from the enrolled authenticator. + * - `webauthn` — an assertion from a registered second-factor security key, + * begun at `/api/auth/step-up/options` with `method: "webauthn"`. + * - `passkey` — an assertion from a primary passkey, begun at the same + * endpoint with `method: "passkey"`. + * + * Only the last three satisfy the fresh-factor routes, mirroring the web, where + * a password login never stamps a session second-factor-verified. * * The password is length-bounded so a body cap is not the only thing standing * between a caller and an unbounded Argon2id verify. @@ -25,6 +31,18 @@ export const stepUpMintSchema = z method: z.literal("password"), password: z.string().min(1).max(512), }), + z.object({ + method: z.literal("totp"), + code: z + .string() + .trim() + .regex(/^\d{6}$/, "Enter the 6-digit code from your authenticator app"), + }), + z.object({ + method: z.literal("webauthn"), + challengeId: z.string().min(1), + credential: webauthnCredentialSchema, + }), z.object({ method: z.literal("passkey"), challengeId: z.string().min(1), @@ -34,5 +52,16 @@ export const stepUpMintSchema = z .meta({ id: "StepUpMintRequest", description: - "Re-prove the account password or a primary passkey to mint a single-use, token-bound step-up elevation.", + "Re-prove a factor to mint a single-use, token-bound step-up elevation. `password` reaches the same routes a plain cookie session reaches; `totp`, `webauthn`, and `passkey` additionally satisfy the fresh-factor routes (disable, recovery-code rotation, security-key removal), matching exactly the ceremonies for which the web stamps a session second-factor-verified.", }); + +/** Which assertion ceremony to begin at `POST /api/auth/step-up/options`. */ +export const stepUpOptionsSchema = z + .object({ + method: z + .enum(["passkey", "webauthn"]) + .describe( + "`passkey` begins an assertion against the account's primary passkeys; `webauthn` against its registered second-factor security keys.", + ), + }) + .meta({ id: "StepUpOptionsRequest" }); diff --git a/tests/integration/sessions-and-known-devices.test.ts b/tests/integration/sessions-and-known-devices.test.ts index fb9cbc7c5..c54e97f91 100644 --- a/tests/integration/sessions-and-known-devices.test.ts +++ b/tests/integration/sessions-and-known-devices.test.ts @@ -58,7 +58,10 @@ describe("destroyOtherSessions", () => { }, }); - const result = await destroyOtherSessions(user.id, current.id); + const result = await destroyOtherSessions(user.id, { + kind: "session", + sessionId: current.id, + }); expect(result.sessionsRevoked).toBe(2); const remaining = await prisma.session.findMany({ diff --git a/tests/integration/step-up-elevation.test.ts b/tests/integration/step-up-elevation.test.ts index f85eacd37..d62ac379e 100644 --- a/tests/integration/step-up-elevation.test.ts +++ b/tests/integration/step-up-elevation.test.ts @@ -15,6 +15,7 @@ */ import { beforeEach, describe, expect, it, vi } from "vitest"; import { NextRequest } from "next/server"; +import * as OTPAuth from "otpauth"; import { cookieJar, headerJar } from "./mock-next-headers"; import { getPrismaClient, truncateAllTables } from "./setup"; @@ -118,15 +119,79 @@ function req(path: string, method = "GET", body?: unknown): NextRequest { return new NextRequest(`https://health.example${path}`, init as never); } +/** Drive the mint endpoint with an arbitrary proof body. */ +async function mintWith( + body: Record, +): Promise<{ status: number; body: Record }> { + const { POST } = await import("@/app/api/auth/step-up/route"); + const res = await POST(req("/api/auth/step-up", "POST", body)); + return { status: res.status, body: (await res.json()) as never }; +} + /** Drive the mint endpoint with a password proof. */ async function mintElevation( password: string, ): Promise<{ status: number; body: Record }> { - const { POST } = await import("@/app/api/auth/step-up/route"); - const res = await POST( - req("/api/auth/step-up", "POST", { method: "password", password }), - ); - return { status: res.status, body: (await res.json()) as never }; + return mintWith({ method: "password", password }); +} + +/** + * Mint a TOTP-proved elevation the way a real client would: enrol a secret, + * then present a live code for it. This is the only fresh-factor arm reachable + * without a WebAuthn authenticator, so it carries the fresh-factor coverage. + */ +async function enrolTotp(): Promise { + const { generateTotpSecret } = await import("@/lib/auth/mfa/totp"); + const { encrypt } = await import("@/lib/crypto"); + const secret = generateTotpSecret(); + await getPrismaClient().user.update({ + where: { id: USER_ID }, + data: { + totpSecretEncrypted: encrypt(secret), + totpConfirmedAt: new Date(), + totpLastStep: null, + }, + }); + return secret; +} + +/** Mirrors the server's TOTP parameters — there is no code generator to import. */ +function currentTotpCode(secretBase32: string): string { + const totp = new OTPAuth.TOTP({ + issuer: "HealthLog", + label: "HealthLog", + algorithm: "SHA1", + digits: 6, + period: 30, + secret: OTPAuth.Secret.fromBase32(secretBase32), + }); + return totp.generate({ timestamp: Date.now() }); +} + +async function mintTotpElevation( + secret: string, +): Promise<{ status: number; body: Record }> { + return mintWith({ method: "totp", code: currentTotpCode(secret) }); +} + +/** + * Seed a known recovery-code batch and return the plaintext. + * + * The disable route wants a live factor in its BODY on top of the step-up. A + * TOTP code spent minting the elevation cannot be reused there — the shared + * verifier burns its time-step, by design — so within one 30-second window the + * body factor has to be something else. That is exactly how the web behaves + * too. + */ +async function seedRecoveryCodes(): Promise { + const { regenerateRecoveryCodes } = + await import("@/lib/auth/mfa/recovery-codes"); + return regenerateRecoveryCodes(USER_ID); +} + +/** Pull the raw elevation out of a successful mint. */ +function elevationOf(result: { body: Record }): string { + return (result.body.data as { elevation: string }).elevation; } /** @@ -136,7 +201,19 @@ async function mintElevation( */ type RouteFn = (request: NextRequest) => Promise; -/** GET /api/auth/me/mfa — the cheapest route in the elevation-accepting set. */ +/** + * POST /api/auth/me/mfa/totp/setup — the cheapest MUTATION in the + * elevation-accepting set, and the probe every redemption case drives. + * + * NOT `GET /api/auth/me/mfa`: that read is plain `requireAuth()` now, so using + * it here would have quietly stopped exercising the elevation path at all. + */ +async function callMfaSetup(): Promise { + const { POST } = await import("@/app/api/auth/me/mfa/totp/setup/route"); + return await POST(req("/api/auth/me/mfa/totp/setup", "POST")); +} + +/** The status read — plain Bearer, no elevation. */ async function callMfaStatus(): Promise { const { GET } = await import("@/app/api/auth/me/mfa/route"); return await (GET as unknown as RouteFn)(req("/api/auth/me/mfa")); @@ -160,7 +237,7 @@ describe("E1 — the token alone is not a step-up proof", () => { useToken(raw); useElevation(null); - const res = await callMfaStatus(); + const res = await callMfaSetup(); // 401 with the machine code the client branches on to launch a re-proof — // not a 200, and not the 403 an unauthorised token would get. @@ -183,7 +260,7 @@ describe("E1 — the token alone is not a step-up proof", () => { useToken(raw); useElevation(`hle_${"f".repeat(64)}`); - expect((await callMfaStatus()).status).toBe(401); + expect((await callMfaSetup()).status).toBe(401); await new Promise((r) => setTimeout(r, 100)); expect(await auditReasons("auth.stepup.elevation.rejected")).toContain( "unknown", @@ -207,7 +284,7 @@ describe("E2 — an elevation is not portable between tokens", () => { // thing standing between these two, which is exactly the point. useToken(b.raw); useElevation(elevation); - const res = await callMfaStatus(); + const res = await callMfaSetup(); expect(res.status).toBe(401); await new Promise((r) => setTimeout(r, 100)); @@ -221,7 +298,7 @@ describe("E2 — an elevation is not portable between tokens", () => { useToken(a.raw); useElevation(elevation); - expect((await callMfaStatus()).status).toBe(200); + expect((await callMfaSetup()).status).toBe(200); }); it("refuses redemption by another user's token", async () => { @@ -236,7 +313,7 @@ describe("E2 — an elevation is not portable between tokens", () => { useToken(theirs.raw); useElevation(elevation); - expect((await callMfaStatus()).status).toBe(401); + expect((await callMfaSetup()).status).toBe(401); }); }); @@ -251,8 +328,8 @@ describe("E3 — an elevation is consumed exactly once", () => { ).elevation; useElevation(elevation); - expect((await callMfaStatus()).status).toBe(200); - expect((await callMfaStatus()).status).toBe(401); + expect((await callMfaSetup()).status).toBe(200); + expect((await callMfaSetup()).status).toBe(401); await new Promise((r) => setTimeout(r, 100)); expect(await auditReasons("auth.stepup.elevation.rejected")).toContain( @@ -271,15 +348,16 @@ describe("E3 — an elevation is consumed exactly once", () => { // conditional UPDATEs on one row rather than between two HTTP handlers that // happen to interleave. A check-then-update implementation passes the // sequential case above and fails here. - const { redeemStepUpElevation } = await import("@/lib/auth/step-up"); + const { claimStepUpElevation } = await import("@/lib/auth/step-up"); const attempt = () => - redeemStepUpElevation({ + claimStepUpElevation({ rawToken: elevation, userId: USER_ID, apiTokenId: id, + requireFreshFactor: false, }); - const results = await Promise.all([ + const results: Awaited>[] = await Promise.all([ attempt(), attempt(), attempt(), @@ -309,7 +387,7 @@ describe("E4 — an expired elevation is refused", () => { }); useElevation(elevation); - expect((await callMfaStatus()).status).toBe(401); + expect((await callMfaSetup()).status).toBe(401); await new Promise((r) => setTimeout(r, 100)); expect(await auditReasons("auth.stepup.elevation.rejected")).toContain( @@ -434,9 +512,9 @@ describe("E7 — the cookie flow is unchanged", () => { return session.id; } - it("serves an MFA-management read on a plain cookie session", async () => { + it("serves an MFA-management mutation on a plain cookie session", async () => { await useCookieSession(); - expect((await callMfaStatus()).status).toBe(200); + expect((await callMfaSetup()).status).toBe(200); }); it("still refuses a step-up-gated route on a cookie with no fresh factor", async () => { @@ -479,7 +557,7 @@ describe("E7 — the cookie flow is unchanged", () => { await useCookieSession(); useElevation(elevation); - expect((await callMfaStatus()).status).toBe(200); + expect((await callMfaSetup()).status).toBe(200); const row = await getPrismaClient().stepUpElevation.findFirst({ where: { apiTokenId: id }, @@ -491,14 +569,85 @@ describe("E7 — the cookie flow is unchanged", () => { // ── E8 — invalidation on credential rotation ───────────────────────── describe("E8 — an elevation does not outlive its anchor", () => { - it("is dropped when the account password changes", async () => { + it("is dropped by a real password change, through the route", async () => { + // Driven through POST /api/auth/password rather than by calling the + // revocation helper. Calling the helper directly proved only that the helper + // deletes rows — deleting the call from BOTH password routes left the old + // version of this test green, so the invalidation story was uncovered. const { raw } = await mintToken("e8a"); useToken(raw); + const elevation = elevationOf(await mintElevation(PASSWORD)); + expect(await getPrismaClient().stepUpElevation.count()).toBe(1); + + const NEW_PASSWORD = "Zt7#qvbLm2xR!e9Wd4Kp"; + const { POST } = await import("@/app/api/auth/password/route"); + const res = await POST( + req("/api/auth/password", "POST", { + currentPassword: PASSWORD, + newPassword: NEW_PASSWORD, + confirmPassword: NEW_PASSWORD, + }), + ); + expect(res.status).toBe(200); + + expect(await getPrismaClient().stepUpElevation.count()).toBe(0); + + // And the value the client still holds is dead. The password route mints a + // replacement cookie session, which the shared jar would otherwise pick up + // and satisfy the gate on the cookie arm — clear it so this probes the + // Bearer arm, which is the one under test. + cookieJar.clear(); + useElevation(elevation); + expect((await callMfaSetup()).status).toBe(401); + }); + + it("is dropped by an operator-forced reset, through the admin route", async () => { + const { raw } = await mintToken("e8a2"); + useToken(raw); await mintElevation(PASSWORD); expect(await getPrismaClient().stepUpElevation.count()).toBe(1); - const { revokeStepUpElevations } = await import("@/lib/auth/step-up"); - await revokeStepUpElevations(USER_ID); + // The admin route is cookie-only, so the reset arrives on an ADMIN session + // while the elevation belongs to the target's token. + await getPrismaClient().user.update({ + where: { id: OTHER_USER_ID }, + data: { role: "ADMIN" }, + }); + const adminSession = await getPrismaClient().session.create({ + data: { + userId: OTHER_USER_ID, + expiresAt: new Date(Date.now() + 3_600_000), + }, + }); + cookieJar.set("healthlog_session", adminSession.id); + headerJar.delete("authorization"); + + const { POST } = + await import("@/app/api/admin/users/[id]/reset-password/route"); + const res = await POST( + req(`/api/admin/users/${USER_ID}/reset-password`, "POST", { + password: "operator chosen long passphrase 77", + }), + { params: Promise.resolve({ id: USER_ID }) }, + ); + expect(res.status).toBe(200); + + expect(await getPrismaClient().stepUpElevation.count()).toBe(0); + }); + + it("is dropped by sign-out-everywhere", async () => { + const { raw } = await mintToken("e8a3"); + useToken(raw); + await mintElevation(PASSWORD); + + const session = await getPrismaClient().session.create({ + data: { userId: USER_ID, expiresAt: new Date(Date.now() + 3_600_000) }, + }); + const { destroyOtherSessions } = await import("@/lib/auth/session"); + await destroyOtherSessions(USER_ID, { + kind: "session", + sessionId: session.id, + }); expect(await getPrismaClient().stepUpElevation.count()).toBe(0); }); @@ -518,7 +667,7 @@ describe("E8 — an elevation does not outlive its anchor", () => { useElevation(elevation); // Refused at Bearer resolution, before the elevation is even looked at — // which is why no code in the revoke path needs to know about elevations. - expect((await callMfaStatus()).status).toBe(401); + expect((await callMfaSetup()).status).toBe(401); const row = await getPrismaClient().stepUpElevation.findFirst(); expect(row?.consumedAt).toBeNull(); @@ -563,7 +712,7 @@ describe("E9 — a native client can manage its second factor", () => { }); expect(user?.totpSecretEncrypted).toBeTruthy(); expect(user?.totpConfirmedAt).toBeNull(); - expect((await callMfaStatus()).status).toBe(401); + expect((await callMfaSetup()).status).toBe(401); }); it("refuses the mint surface to a cookie session", async () => { @@ -585,3 +734,361 @@ describe("E9 — a native client can manage its second factor", () => { expect(await getPrismaClient().stepUpElevation.count()).toBe(0); }); }); + +// ── E10 — the fresh-factor rule (B1) ───────────────────────────────── + +describe("E10 — a password proof cannot reach the destructive routes", () => { + /** + * The attack this closes, in the order it ran before the fix: + * 1. mint an elevation with the account password (a stolen token plus a + * known password is the whole prerequisite); + * 2. POST recovery-codes/regenerate — which takes NO factor in its body — + * and receive ten plaintext recovery codes; + * 3. POST disable with one of them, satisfying the body check. + * The second factor is gone. On the web that chain is impossible because a + * password login never stamps `mfaVerifiedAt`. + */ + async function regenerate(): Promise { + const { POST } = + await import("@/app/api/auth/me/mfa/recovery-codes/regenerate/route"); + return POST(req("/api/auth/me/mfa/recovery-codes/regenerate", "POST")); + } + + async function disable(code: string, method = "recovery"): Promise { + const { POST } = await import("@/app/api/auth/me/mfa/disable/route"); + return POST(req("/api/auth/me/mfa/disable", "POST", { code, method })); + } + + async function removeKey(id: string): Promise { + const { DELETE } = + await import("@/app/api/auth/me/mfa/webauthn/[id]/route"); + return DELETE(req(`/api/auth/me/mfa/webauthn/${id}`, "DELETE"), { + params: Promise.resolve({ id }), + }); + } + + it("refuses recovery-code regeneration on a password-proved elevation", async () => { + await enrolTotp(); + const { raw } = await mintToken("e10a"); + useToken(raw); + useElevation(elevationOf(await mintElevation(PASSWORD))); + + const res = await regenerate(); + + expect(res.status).toBe(401); + // No codes were minted, so step 2 of the chain yields nothing. + expect(await getPrismaClient().mfaRecoveryCode.count()).toBe(0); + await new Promise((r) => setTimeout(r, 100)); + expect(await auditReasons("auth.stepup.elevation.rejected")).toContain( + "insufficient_factor", + ); + }); + + it("refuses disable on a password-proved elevation", async () => { + const secret = await enrolTotp(); + const { raw } = await mintToken("e10b"); + useToken(raw); + useElevation(elevationOf(await mintElevation(PASSWORD))); + + const res = await disable(currentTotpCode(secret), "totp"); + + expect(res.status).toBe(401); + const user = await getPrismaClient().user.findUnique({ + where: { id: USER_ID }, + }); + expect(user?.totpConfirmedAt).not.toBeNull(); + }); + + it("refuses security-key removal on a password-proved elevation", async () => { + const key = await getPrismaClient().webauthnMfaCredential.create({ + data: { + userId: USER_ID, + name: "key", + credentialId: "cred-e10c", + credentialPublicKey: Buffer.from([1, 2, 3]), + transports: ["internal"], + }, + }); + const { raw } = await mintToken("e10c"); + useToken(raw); + useElevation(elevationOf(await mintElevation(PASSWORD))); + + expect((await removeKey(key.id)).status).toBe(401); + expect(await getPrismaClient().webauthnMfaCredential.count()).toBe(1); + }); + + it("does NOT burn the elevation when the factor is too weak", async () => { + // The refusal is about reach, not validity. The same elevation must still + // work on the routes it was always entitled to. + await enrolTotp(); + const { raw } = await mintToken("e10d"); + useToken(raw); + const elevation = elevationOf(await mintElevation(PASSWORD)); + + useElevation(elevation); + expect((await regenerate()).status).toBe(401); + + const row = await getPrismaClient().stepUpElevation.findFirst(); + expect(row?.consumedAt).toBeNull(); + }); + + it("admits the SAME routes a plain cookie session reaches", async () => { + // The password arm is not useless — it is exactly cookie-equivalent. + const { raw } = await mintToken("e10e"); + useToken(raw); + useElevation(elevationOf(await mintElevation(PASSWORD))); + + expect((await callMfaSetup()).status).toBe(200); + }); + + it("admits the destructive routes on a TOTP-proved elevation", async () => { + const secret = await enrolTotp(); + const { raw } = await mintToken("e10f"); + useToken(raw); + + const minted = await mintTotpElevation(secret); + expect(minted.status).toBe(200); + expect( + (minted.body.data as { satisfiesFreshFactor: boolean }) + .satisfiesFreshFactor, + ).toBe(true); + + useElevation(elevationOf(minted)); + const res = await regenerate(); + + expect(res.status).toBe(200); + const body = (await res.json()) as { + data: { recoveryCodes: string[] }; + }; + expect(body.data.recoveryCodes.length).toBeGreaterThan(0); + }); + + it("marks a password-proved elevation as not fresh-factor in the mint response", async () => { + const { raw } = await mintToken("e10g"); + useToken(raw); + const minted = await mintElevation(PASSWORD); + const data = minted.body.data as { + method: string; + satisfiesFreshFactor: boolean; + }; + expect(data.method).toBe("password"); + expect(data.satisfiesFreshFactor).toBe(false); + }); + + it("refuses a stale TOTP code and does not mint", async () => { + const secret = await enrolTotp(); + const { raw } = await mintToken("e10h"); + useToken(raw); + + // Spend the code through the shared verifier first; the replay guard must + // then refuse the same code here, exactly as it does at login. + const code = currentTotpCode(secret); + const { verifyMfaFactor } = await import("@/lib/auth/mfa/verify-factor"); + const user = await getPrismaClient().user.findUnique({ + where: { id: USER_ID }, + }); + await verifyMfaFactor(user!, "totp", code); + + const res = await mintWith({ method: "totp", code }); + + expect(res.status).toBe(401); + expect(await getPrismaClient().stepUpElevation.count()).toBe(0); + }); +}); + +// ── E11 — the disable route keeps the caller signed in (B2) ────────── + +describe("E11 — disabling MFA over Bearer spares the calling device", () => { + it("revokes every OTHER refresh token but not the caller's own", async () => { + const secret = await enrolTotp(); + const { raw, id } = await mintToken("e11a"); + + const { hashToken } = await import("@/lib/auth/hmac"); + // The caller's own device login: the RefreshToken row cross-referencing the + // access token being presented. + const mine = await getPrismaClient().refreshToken.create({ + data: { + userId: USER_ID, + tokenHash: hashToken(`hlr_mine${"0".repeat(56)}`), + accessTokenHash: hashToken(raw), + expiresAt: new Date(Date.now() + 30 * 24 * 3_600_000), + }, + }); + // Another device. + const theirs = await getPrismaClient().refreshToken.create({ + data: { + userId: USER_ID, + tokenHash: hashToken(`hlr_theirs${"0".repeat(54)}`), + accessTokenHash: hashToken(`hlk_other${"0".repeat(55)}`), + expiresAt: new Date(Date.now() + 30 * 24 * 3_600_000), + }, + }); + // And a browser session, which has no "current" on the Bearer path. + const webSession = await getPrismaClient().session.create({ + data: { userId: USER_ID, expiresAt: new Date(Date.now() + 3_600_000) }, + }); + + const codes = await seedRecoveryCodes(); + useToken(raw); + useElevation(elevationOf(await mintTotpElevation(secret))); + + const { POST } = await import("@/app/api/auth/me/mfa/disable/route"); + const res = await POST( + req("/api/auth/me/mfa/disable", "POST", { + code: codes[0], + method: "recovery", + }), + ); + expect(res.status).toBe(200); + + // The defect: passing the ApiToken id where a Session id was expected made + // the "keep the current one" exclusion match nothing, so the caller's own + // refresh token was revoked and the app logged itself out at next rotation. + const mineAfter = await getPrismaClient().refreshToken.findUnique({ + where: { id: mine.id }, + }); + const theirsAfter = await getPrismaClient().refreshToken.findUnique({ + where: { id: theirs.id }, + }); + expect(mineAfter?.revokedAt).toBeNull(); + expect(theirsAfter?.revokedAt).not.toBeNull(); + + // Web sessions all go — none of them is the caller. + expect( + await getPrismaClient().session.findUnique({ + where: { id: webSession.id }, + }), + ).toBeNull(); + + // And the calling token itself is untouched, so the next request works. + const token = await getPrismaClient().apiToken.findUnique({ + where: { id }, + }); + expect(token?.revoked).toBe(false); + }); + + it("keeps the caller's own session on the cookie path, as before", async () => { + const secret = await enrolTotp(); + const current = await getPrismaClient().session.create({ + data: { + userId: USER_ID, + expiresAt: new Date(Date.now() + 3_600_000), + mfaVerifiedAt: new Date(), + }, + }); + const other = await getPrismaClient().session.create({ + data: { userId: USER_ID, expiresAt: new Date(Date.now() + 3_600_000) }, + }); + cookieJar.set("healthlog_session", current.id); + headerJar.delete("authorization"); + + const { POST } = await import("@/app/api/auth/me/mfa/disable/route"); + const res = await POST( + req("/api/auth/me/mfa/disable", "POST", { + code: currentTotpCode(secret), + method: "totp", + }), + ); + expect(res.status).toBe(200); + + expect( + await getPrismaClient().session.findUnique({ where: { id: current.id } }), + ).not.toBeNull(); + expect( + await getPrismaClient().session.findUnique({ where: { id: other.id } }), + ).toBeNull(); + }); +}); + +// ── E12 — validation failures do not burn the proof ────────────────── + +describe("E12 — a rejected request keeps the elevation spendable", () => { + it("survives a wrong TOTP code on disable", async () => { + const secret = await enrolTotp(); + const codes = await seedRecoveryCodes(); + const { raw } = await mintToken("e12a"); + useToken(raw); + const elevation = elevationOf(await mintTotpElevation(secret)); + + useElevation(elevation); + const { POST } = await import("@/app/api/auth/me/mfa/disable/route"); + const bad = await POST( + req("/api/auth/me/mfa/disable", "POST", { + code: "000000", + method: "totp", + }), + ); + expect(bad.status).toBe(401); + + // Unspent — otherwise five fat-fingered codes would exhaust the 5-per-15-min + // mint ceiling and lock the user out of their own security settings. + const row = await getPrismaClient().stepUpElevation.findFirst(); + expect(row?.consumedAt).toBeNull(); + + // And the retry goes through on the SAME elevation. + const good = await POST( + req("/api/auth/me/mfa/disable", "POST", { + code: codes[0], + method: "recovery", + }), + ); + expect(good.status).toBe(200); + }); + + it("survives a malformed body", async () => { + const { raw } = await mintToken("e12b"); + useToken(raw); + useElevation(elevationOf(await mintElevation(PASSWORD))); + + const { POST } = + await import("@/app/api/auth/me/mfa/webauthn/register/verify/route"); + const res = await POST( + req("/api/auth/me/mfa/webauthn/register/verify", "POST", { nope: 1 }), + ); + expect(res.status).toBe(422); + + const row = await getPrismaClient().stepUpElevation.findFirst(); + expect(row?.consumedAt).toBeNull(); + }); +}); + +// ── E13 — the status read is plain Bearer ──────────────────────────── + +describe("E13 — the second-factor status read needs no elevation", () => { + it("serves a token with no elevation header at all", async () => { + const { raw } = await mintToken("e13a"); + useToken(raw); + useElevation(null); + + const res = await callMfaStatus(); + + expect(res.status).toBe(200); + const body = (await res.json()) as { + data: { totp: { enabled: boolean }; webauthn: unknown[] }; + }; + expect(body.data.totp).toBeDefined(); + expect(Array.isArray(body.data.webauthn)).toBe(true); + }); + + it("carries no credential material", async () => { + await getPrismaClient().webauthnMfaCredential.create({ + data: { + userId: USER_ID, + name: "key", + credentialId: "cred-e13b-secret", + credentialPublicKey: Buffer.from([9, 9, 9]), + transports: ["internal"], + }, + }); + const { raw } = await mintToken("e13b"); + useToken(raw); + + const text = await (await callMfaStatus()).text(); + + // The justification for relaxing this route is that the payload is inert. + // Pin it: no credential id, no public key, no codes. + expect(text).not.toContain("cred-e13b-secret"); + expect(text).not.toContain("credentialPublicKey"); + expect(text).not.toContain("credentialId"); + }); +});