diff --git a/docs/api/openapi.yaml b/docs/api/openapi.yaml index 3417971df..8c86d7bba 100644 --- a/docs/api/openapi.yaml +++ b/docs/api/openapi.yaml @@ -2581,13 +2581,17 @@ paths: the account has no password — an account must keep at least one way in. - Step-up gated for accounts that can clear the gate, on the same mechanism as removing a second-factor security - key (DELETE /api/auth/me/mfa/webauthn/{id}). An account with a second factor enrolled — a confirmed TOTP secret - or a registered security key — must present a fresh factor proof: a cookie session that completed one within the - last five minutes, or, on Bearer, an `X-Step-Up` elevation minted at POST /api/auth/step-up against a re-proved - factor (a password-proved elevation is NOT enough). An account with no second factor enrolled is unaffected and - keeps the plain session-or-token contract, because a gate it could never satisfy would lock it out of its own - credential list. + Step-up gated, on the same mechanism and at the same strength as removing a second-factor security key (DELETE + /api/auth/me/mfa/webauthn/{id}). The caller must present a fresh possession proof: on the cookie path a session + that completed one within the last five minutes, on Bearer an `X-Step-Up` elevation minted at POST + /api/auth/step-up against a re-proved factor. A password-proved elevation is NOT enough. + + + A PASSKEY IS SUCH A PROOF here, unlike on the second-factor routes. An account whose only credential is a + passkey can satisfy this gate: a passkey login stamps the session, and POST /api/auth/step-up accepts `method: + "passkey"` against the account's primary passkeys. So its refusal is `meta.errorCode: auth.stepup.required` — + re-prove and retry — and not the dead-end `auth.stepup.mfa_not_enrolled`, which is reserved for an account + holding no credential this gate could ever accept. The elevation is spent only when the deletion is about to happen. A 404 for an unknown id, or the @@ -2613,9 +2617,11 @@ paths: schema: $ref: "#/components/schemas/ErrorEnvelope" "401": - description: Not authenticated, or the account has a second factor and the request carried no proof of it fresh enough - (`meta.errorCode` = `auth.stepup.required`). Re-prove the factor and retry — on the web by re-verifying, on - a native client by minting an elevation at POST /api/auth/step-up. Nothing was removed. + description: Not authenticated, or the request carried no fresh possession proof (`meta.errorCode` = + `auth.stepup.required`). Re-prove and retry — on a native client by minting an elevation at POST + /api/auth/step-up, on the web by signing in again, which is what stamps the session. + `auth.stepup.mfa_not_enrolled` is the other arm and means the account holds no credential this gate can + accept, so retrying will not help. Nothing was removed either way. content: application/json: schema: diff --git a/messages/de.json b/messages/de.json index f7dd21931..c3cc01318 100644 --- a/messages/de.json +++ b/messages/de.json @@ -6404,7 +6404,8 @@ "error": "Die Einstellung konnte nicht gespeichert werden." }, "dangerZoneDetail": "Dein Konto und deine Anmeldedaten bleiben erhalten.", - "deleteAccountCardDetail": "Dieser Vorgang kann nicht rückgängig gemacht werden." + "deleteAccountCardDetail": "Dieser Vorgang kann nicht rückgängig gemacht werden.", + "passkeyStepUpRequired": "Aus Sicherheitsgründen bitte erneut anmelden, bevor du einen Passkey entfernst." }, "admin": { "title": "Administration", diff --git a/messages/en.json b/messages/en.json index db7c97817..4e9238e5f 100644 --- a/messages/en.json +++ b/messages/en.json @@ -6404,7 +6404,8 @@ "error": "Could not save the setting." }, "dangerZoneDetail": "Your account and the credentials you sign in with are kept.", - "deleteAccountCardDetail": "This cannot be undone." + "deleteAccountCardDetail": "This cannot be undone.", + "passkeyStepUpRequired": "For your security, sign in again before removing a passkey." }, "admin": { "title": "Administration", diff --git a/messages/es.json b/messages/es.json index 3562ce5b7..bf5db0715 100644 --- a/messages/es.json +++ b/messages/es.json @@ -6404,7 +6404,8 @@ "error": "No se pudo guardar el ajuste." }, "dangerZoneDetail": "Tu cuenta y las credenciales con las que inicias sesión se conservan.", - "deleteAccountCardDetail": "Esto no se puede deshacer." + "deleteAccountCardDetail": "Esto no se puede deshacer.", + "passkeyStepUpRequired": "Por seguridad, vuelve a iniciar sesión antes de quitar una clave de acceso." }, "admin": { "title": "Administration", diff --git a/messages/fr.json b/messages/fr.json index 5faaecd7a..73fe2a65f 100644 --- a/messages/fr.json +++ b/messages/fr.json @@ -6404,7 +6404,8 @@ "error": "Le réglage n'a pas pu être enregistré." }, "dangerZoneDetail": "Votre compte et vos identifiants de connexion sont conservés.", - "deleteAccountCardDetail": "Cette action est irréversible." + "deleteAccountCardDetail": "Cette action est irréversible.", + "passkeyStepUpRequired": "Par sécurité, reconnecte-toi avant de supprimer une clé d'accès." }, "admin": { "title": "Administration", diff --git a/messages/it.json b/messages/it.json index 85ad8bcf1..d2c73dcf0 100644 --- a/messages/it.json +++ b/messages/it.json @@ -6404,7 +6404,8 @@ "error": "Non è stato possibile salvare l'impostazione." }, "dangerZoneDetail": "Il tuo account e le credenziali con cui accedi restano.", - "deleteAccountCardDetail": "Questa azione non può essere annullata." + "deleteAccountCardDetail": "Questa azione non può essere annullata.", + "passkeyStepUpRequired": "Per sicurezza, accedi di nuovo prima di rimuovere una passkey." }, "admin": { "title": "Administration", diff --git a/messages/pl.json b/messages/pl.json index 7e83719f0..d44095179 100644 --- a/messages/pl.json +++ b/messages/pl.json @@ -6404,7 +6404,8 @@ "error": "Nie udało się zapisać ustawienia." }, "dangerZoneDetail": "Konto i dane logowania zostają zachowane.", - "deleteAccountCardDetail": "Tej operacji nie można cofnąć." + "deleteAccountCardDetail": "Tej operacji nie można cofnąć.", + "passkeyStepUpRequired": "Ze względów bezpieczeństwa zaloguj się ponownie przed usunięciem klucza dostępu." }, "admin": { "title": "Administracja", diff --git a/src/__tests__/step-up-elevation-guard.test.ts b/src/__tests__/step-up-elevation-guard.test.ts index 6eabb721b..16589e1d1 100644 --- a/src/__tests__/step-up-elevation-guard.test.ts +++ b/src/__tests__/step-up-elevation-guard.test.ts @@ -94,9 +94,12 @@ function callers(exportName: string, moduleRe: RegExp): string[] { * That one is `DELETE /api/auth/passkeys/{id}`. A passkey is the PRIMARY * sign-in credential, and it sat behind a plain session while the second-factor * key beside it demanded a fresh proof — the softer gate on the more valuable - * credential. It joins the set on `freshFactor: "if-enrolled"`, so an account - * with a second factor pays the full price and an account without one keeps the - * contract it had rather than losing the ability to remove its own credential. + * credential. It joins the set on the same `freshFactor: true` as the rest, + * differing only in `proofSource: "any-possession"` — which widens the + * REACHABILITY pre-check to count a primary passkey and does not soften the gate + * itself. A passkey-only account clears it by re-proving that passkey: on Bearer + * through the mint's `passkey` method, on the web through a fresh sign-in, which + * is what stamps `mfaVerifiedAt`. * * The widening that admits is real and bounded: an elevation minted against a * re-proved factor can now also remove a passkey. That is the same class of act diff --git a/src/app/api/auth/passkeys/[id]/__tests__/route.test.ts b/src/app/api/auth/passkeys/[id]/__tests__/route.test.ts index cc8943d1f..c4e4adde5 100644 --- a/src/app/api/auth/passkeys/[id]/__tests__/route.test.ts +++ b/src/app/api/auth/passkeys/[id]/__tests__/route.test.ts @@ -167,24 +167,26 @@ describe("PATCH /api/auth/passkeys/{id} — the rename refusal", () => { }); /** - * The removal's gate, and what it must NOT be. + * The removal's gate, and the one option that keeps it from being a dead end. * * A passkey is the primary sign-in credential and it came off a plain session, * while the second-factor key next door demanded a fresh proof. The route now - * asks the same gate the second-factor removal asks — but with - * `freshFactor: "if-enrolled"`, not `true`. That distinction is the whole - * design: `true` refuses an account with no second factor outright, and most - * accounts holding a passkey have nothing else enrolled, so `true` would take - * their own credential list away from them. + * asks the same gate that one asks, at the same strength — `freshFactor: true`, + * not a softer mode. What it adds is `proofSource: "any-possession"`, and that + * is the whole design: the gate's reachability pre-check counts second factors, + * and a passkey-only account holds none, so the default question would have + * refused `mfa_not_enrolled` to an account that was holding the proof the gate + * accepts. A passkey stamps `mfaVerifiedAt` on login and is a `passkey`-method + * mint arm; the pre-check simply was not asking about it. * * The gate itself is behavioural and lives against a real database in * `tests/integration/step-up-elevation.test.ts` (E14). What is pinned here is - * what this file can prove without one: the route asks for the right mode, and - * it spends the proof only when it is actually about to delete. + * what this file can prove without one: the route asks both halves of the right + * question, and it spends the proof only when it is about to delete. * - * Mutation check: change the option to `true` and the first case goes red; move - * `commitElevation()` above the ownership check and the third goes red; drop it - * altogether and the second goes red. + * Mutation check: drop the `proofSource` option and the first case goes red; + * move `commitElevation()` above the ownership check and the third goes red; + * drop it altogether and the second goes red. */ describe("DELETE /api/auth/passkeys/{id} — the removal's gate", () => { const mkDelete = () => @@ -192,7 +194,7 @@ describe("DELETE /api/auth/passkeys/{id} — the removal's gate", () => { method: "DELETE", }); - it("asks for the step-up gate in its if-enrolled mode", async () => { + it("asks for a full step-up, against any possession factor", async () => { vi.mocked(prisma.passkey.findUnique).mockResolvedValue({ id: "pk-1", userId: USER.id, @@ -204,10 +206,12 @@ describe("DELETE /api/auth/passkeys/{id} — the removal's gate", () => { await (DELETE as unknown as Handler)(mkDelete(), { params }); expect(requireMfaManagementAuth).toHaveBeenCalledWith({ - freshFactor: "if-enrolled", + freshFactor: true, + proofSource: "any-possession", }); - // Not `true`: that arm refuses an account with no second factor, which is - // a lockout on this route rather than a safeguard. + // Not the bare second-factor question: it would refuse `mfa_not_enrolled` + // to a passkey-only account, which is every account this route mostly + // serves, over a proof it was already holding. expect(requireMfaManagementAuth).not.toHaveBeenCalledWith({ freshFactor: true, }); diff --git a/src/app/api/auth/passkeys/[id]/route.ts b/src/app/api/auth/passkeys/[id]/route.ts index b13a0550f..b8a79f6cd 100644 --- a/src/app/api/auth/passkeys/[id]/route.ts +++ b/src/app/api/auth/passkeys/[id]/route.ts @@ -76,28 +76,45 @@ export const PATCH = apiHandler( * nothing about a hijacked session quietly stripping the credentials the owner * signs in with. So this route goes through `requireMfaManagementAuth` too. * - * Two consequences worth stating rather than discovering: + * Two things about the gate that are easy to get wrong, and are not: * * BEARER IS NOT REFUSED. `requireFreshMfa` is cookie-only, so a gate built on * it alone would have told the native client to go and use the website. The * gate this route borrows has a Bearer arm — a step-up elevation minted - * against a re-proved factor, exactly what the app already does to remove a - * security key — so the app keeps the capability and pays the same price the - * web pays. + * against a re-proved factor — and `passkey` is both one of the mint's methods + * and one of `FRESH_FACTOR_METHODS`, so the app re-proves the passkey it + * already holds and pays the same price the web pays. * - * AN ACCOUNT WITH NO SECOND FACTOR IS NOT LOCKED OUT. `freshFactor: true` - * refuses those outright, which is right for a route that manages a second - * factor and wrong here: most accounts with a passkey have nothing else - * enrolled, and they would lose the ability to remove their own credential. - * `"if-enrolled"` gates the accounts that can clear the gate and leaves the - * rest exactly where they were. + * A PASSKEY-ONLY ACCOUNT CAN CLEAR THIS GATE, which is why the gate is the + * plain `freshFactor: true` and not something softer. It reads like a lockout + * — most accounts holding a passkey enrol no second factor beside it — and it + * is not, because a passkey IS a possession factor here, as this codebase + * already says in three places: a passkey login stamps `Session.mfaVerifiedAt` + * (`/api/auth/passkey/login-verify`, v1.23 M-review M1), `passkey` sits in + * `FRESH_FACTOR_METHODS`, and `resolveMfaEnrollmentRequired` counts a primary + * passkey as satisfying the enforcement policy outright. The only thing that + * would have refused such an account is the reachability pre-check inside + * `requireFreshMfa`, which counts second factors and nothing else; + * `proofSource: "any-possession"` asks the question this route actually means. + * + * WHAT A REFUSED PERSON DOES NEXT. On Bearer: mint an elevation with + * `method: "passkey"` at `POST /api/auth/step-up`. On the web: sign in again, + * which is what every other step-up refusal in this app tells people and what a + * passkey login already does. The web has no dedicated in-page re-proof for a + * live cookie session — the mint endpoint is Bearer-only by design — so the + * recovery there is a fresh sign-in rather than a dialog. That is a real limit, + * it is named here rather than discovered, and it is the same one the + * second-factor removal beside this route already lives with. */ export const DELETE = apiHandler( async ( request: NextRequest, { params }: { params: Promise<{ id: string }> }, ) => { - const auth = await requireMfaManagementAuth({ freshFactor: "if-enrolled" }); + const auth = await requireMfaManagementAuth({ + freshFactor: true, + proofSource: "any-possession", + }); const { user } = auth; const { id } = await params; diff --git a/src/components/settings/security-section/__tests__/passkey-removal-step-up.test.tsx b/src/components/settings/security-section/__tests__/passkey-removal-step-up.test.tsx new file mode 100644 index 000000000..af14ec769 --- /dev/null +++ b/src/components/settings/security-section/__tests__/passkey-removal-step-up.test.tsx @@ -0,0 +1,110 @@ +/** + * The removal's refusal has to say something true, in the person's language. + * + * `DELETE /api/auth/passkeys/{id}` is step-up gated now, and the server answers + * 401 with `meta.errorCode: auth.stepup.required` and the prose "Recent + * second-factor verification required". Rendering that prose would have been + * wrong twice over: it is English regardless of the operator's locale, and for + * the passkey-only account this card mostly serves there is no second factor — + * the credential to re-prove is the passkey itself. + * + * So the card maps the refusal to its own sentence. Pinned here: + * 1. a step-up refusal renders the card's sentence, not the server's prose, + * 2. the mfa_not_enrolled arm maps too — both codes mean "the gate stopped + * you", and handling only the first falls through to raw English, + * 3. an ordinary failure still shows its own message rather than being + * swallowed by the step-up sentence, + * 4. the sentence resolves in more than one locale, + * 5. the sentence names the recovery the web actually has. The mint endpoint + * is Bearer-only by design, so there is no in-page re-proof dialog for a + * cookie session; signing in again is the recovery, and a passkey login + * stamps `mfaVerifiedAt`. If that ever changes, this assertion is the + * reminder that the copy has to change with it. + * + * Mutation check: return `err.message` from the removal's `onError` and cases 1, + * 2 and 4 go red; narrow `describeStepUp` to the exact `auth.stepup.required` + * code and case 2 goes red; drop the fallback arm and case 3 goes red. + */ +import { describe, expect, it } from "vitest"; +import { readFileSync } from "node:fs"; +import { resolve } from "node:path"; + +import { ApiError } from "@/lib/api/api-fetch"; +import { describeStepUp } from "../security-keys-card"; +import en from "@/../messages/en.json"; +import de from "@/../messages/de.json"; + +const STEP_UP_SENTENCE = en.settings.passkeyStepUpRequired; + +function refusal(errorCode: string): ApiError { + return new ApiError("Recent second-factor verification required", 401, { + errorCode, + }); +} + +describe("passkey removal — the step-up refusal a person sees", () => { + it("replaces the server's prose on a step-up refusal", () => { + const shown = describeStepUp( + refusal("auth.stepup.required"), + "fallback", + STEP_UP_SENTENCE, + ); + + expect(shown).toBe(STEP_UP_SENTENCE); + expect(shown).not.toContain("second-factor verification required"); + }); + + it("maps the not-enrolled arm too", () => { + expect( + describeStepUp( + refusal("auth.stepup.mfa_not_enrolled"), + "fallback", + STEP_UP_SENTENCE, + ), + ).toBe(STEP_UP_SENTENCE); + }); + + it("leaves an ordinary refusal saying its own thing", () => { + const lastMethod = new ApiError( + "Cannot delete — at least one authentication method must remain", + 400, + ); + + expect(describeStepUp(lastMethod, "fallback", STEP_UP_SENTENCE)).toContain( + "at least one authentication method", + ); + }); + + it("carries the sentence in every locale the app offers", () => { + // The EN↔DE pair is the smoke test; `i18n-locale-integrity.test.ts` holds + // the other four to the same guarantee. + expect(de.settings.passkeyStepUpRequired).toBeTruthy(); + expect(de.settings.passkeyStepUpRequired).not.toBe( + en.settings.passkeyStepUpRequired, + ); + }); + + it("names the recovery the web actually has", () => { + // Not "verify to continue": there is no in-page re-proof for a cookie + // session, because the elevation mint is Bearer-only by design. Signing in + // again is the recovery, and a passkey login stamps the session. + expect(STEP_UP_SENTENCE.toLowerCase()).toContain("sign in again"); + }); + + it("routes the removal through the mapper rather than the raw message", () => { + const source = readFileSync( + resolve(__dirname, "../passkey-list-section.tsx"), + "utf8", + ); + // Bounded by the next top-level declaration rather than by the first + // `});`, which lands inside `mutationFn` and would slice the handler away. + const start = source.indexOf("const remove = useMutation"); + const end = source.indexOf("const DEVICE_TYPE_LABELS", start); + expect(start, "the removal mutation was renamed").toBeGreaterThan(-1); + expect(end, "the slice's end anchor is gone").toBeGreaterThan(start); + const body = source.slice(start, end); + + expect(body).toContain("describeStepUp"); + expect(body).toContain("settings.passkeyStepUpRequired"); + }); +}); diff --git a/src/components/settings/security-section/passkey-list-section.tsx b/src/components/settings/security-section/passkey-list-section.tsx index 087866754..ed6cd4b7b 100644 --- a/src/components/settings/security-section/passkey-list-section.tsx +++ b/src/components/settings/security-section/passkey-list-section.tsx @@ -40,6 +40,7 @@ import { apiPost, } from "@/lib/api/api-fetch"; import { describePasskeyError } from "@/lib/passkey-errors"; +import { describeStepUp } from "./security-keys-card"; interface PasskeyInfo { id: string; @@ -169,7 +170,14 @@ export function PasskeyListSection({ setMsg(null); queryClient.invalidateQueries({ queryKey: queryKeys.passkeys() }); }, - onError: (err: Error) => setMsg(err.message), + // Removal is step-up gated. The server's own prose says "second-factor", + // which is wrong for the passkey-only account this card mostly serves — the + // credential to re-prove is the passkey — so the refusal gets its own + // sentence naming the recovery that exists on the web: sign in again. + onError: (err: Error) => + setMsg( + describeStepUp(err, err.message, t("settings.passkeyStepUpRequired")), + ), }); const DEVICE_TYPE_LABELS: Record = { diff --git a/src/components/settings/security-section/security-keys-card.tsx b/src/components/settings/security-section/security-keys-card.tsx index bcaf55806..3dcda31d9 100644 --- a/src/components/settings/security-section/security-keys-card.tsx +++ b/src/components/settings/security-section/security-keys-card.tsx @@ -41,7 +41,21 @@ export interface WebauthnKeyInfo { lastUsedAt: string | null; } -function describeStepUp( +/** + * Turn a refusal into a sentence, with the step-up arm told apart from the rest. + * + * Exported because the passkey card next door needs the identical mapping since + * passkey removal became step-up gated too. The server's own prose there is + * "Recent second-factor verification required", which is English-only and, for a + * passkey-only account, not even accurate about which credential to re-prove — + * so the caller supplies its own sentence and this only decides when to use it. + * + * Matched on `meta.errorCode`, and on the whole `auth.stepup` prefix rather than + * one code: `auth.stepup.required` and `auth.stepup.mfa_not_enrolled` both mean + * "the gate stopped you", and a card that handled only the first would fall + * through to raw server prose on the second. + */ +export function describeStepUp( err: unknown, fallback: string, stepUpMsg: string, diff --git a/src/lib/__tests__/require-fresh-mfa.test.ts b/src/lib/__tests__/require-fresh-mfa.test.ts index 308df3044..c6b7dcf74 100644 --- a/src/lib/__tests__/require-fresh-mfa.test.ts +++ b/src/lib/__tests__/require-fresh-mfa.test.ts @@ -6,6 +6,7 @@ vi.mock("@/lib/db", () => ({ prisma: { session: { findUnique: vi.fn() }, webauthnMfaCredential: { count: vi.fn() }, + passkey: { count: vi.fn() }, }, })); @@ -113,6 +114,75 @@ describe("requireFreshMfa", () => { expect(ctx.user.id).toBe("user-1"); }); + /** + * The reachability pre-check, and the one route that asks it differently. + * + * `requireFreshMfa` refuses `mfa_not_enrolled` when the account holds nothing + * that could ever produce the stamp, because a gate nobody can clear is a dead + * end rather than a safeguard. Which credentials count is the `proofSource` + * parameter, and it changes nothing else: the freshness read below is the same + * read either way. + * + * A primary passkey is deliberately outside the default set and inside + * `"any-possession"`. It DOES stamp `mfaVerifiedAt` on login, so it can clear + * the gate — but counting it by default would pull every passkey-holding + * account into `requireFreshMfaIfEnrolled`'s gate on account deletion, the + * data reset, the password change, the encrypted export and key rotation, + * none of which have ever applied to them. The sibling suite + * `require-fresh-mfa-if-enrolled.test.ts` is what catches that if the two + * predicates are ever collapsed into one. + */ + it("refuses a passkey-only account when only a second factor counts", async () => { + mockSession({ ...MFA_USER, totpConfirmedAt: null }); + vi.mocked(prisma.webauthnMfaCredential.count).mockResolvedValue(0 as never); + vi.mocked(prisma.passkey.count).mockResolvedValue(1 as never); + + await expect(requireFreshMfa(5 * 60)).rejects.toMatchObject({ + errorCode: "auth.stepup.mfa_not_enrolled", + }); + }); + + it("admits that same account under any-possession, with a fresh stamp", async () => { + mockSession({ ...MFA_USER, totpConfirmedAt: null }); + vi.mocked(prisma.webauthnMfaCredential.count).mockResolvedValue(0 as never); + vi.mocked(prisma.passkey.count).mockResolvedValue(1 as never); + vi.mocked(prisma.session.findUnique).mockResolvedValue({ + mfaVerifiedAt: new Date(Date.now() - 60_000), + } as never); + + const ctx = await requireFreshMfa( + MFA_STEP_UP_MAX_AGE_SECONDS, + "any-possession", + ); + expect(ctx.user.id).toBe("user-1"); + }); + + it("still enforces the window under any-possession", async () => { + // The wider proof source decides who may attempt the gate, not who passes + // it. A stale stamp is refused with the re-provable code, not admitted. + mockSession({ ...MFA_USER, totpConfirmedAt: null }); + vi.mocked(prisma.webauthnMfaCredential.count).mockResolvedValue(0 as never); + vi.mocked(prisma.passkey.count).mockResolvedValue(1 as never); + vi.mocked(prisma.session.findUnique).mockResolvedValue({ + mfaVerifiedAt: new Date(Date.now() - 10 * 60_000), + } as never); + + await expect( + requireFreshMfa(MFA_STEP_UP_MAX_AGE_SECONDS, "any-possession"), + ).rejects.toMatchObject({ errorCode: "auth.stepup.required" }); + }); + + it("refuses an account with no credential at all, under either source", async () => { + mockSession({ ...MFA_USER, totpConfirmedAt: null }); + vi.mocked(prisma.webauthnMfaCredential.count).mockResolvedValue(0 as never); + vi.mocked(prisma.passkey.count).mockResolvedValue(0 as never); + + await expect( + requireFreshMfa(5 * 60, "any-possession"), + ).rejects.toMatchObject({ errorCode: "auth.stepup.mfa_not_enrolled" }); + expect(prisma.session.findUnique).not.toHaveBeenCalled(); + }); + it("rejects when there is no cookie session — Bearer can never satisfy it", async () => { // getSession is cookie-only; a Bearer header is irrelevant and never read. headersGet.mockImplementation((n) => diff --git a/src/lib/api-handler.ts b/src/lib/api-handler.ts index 0b111ba47..3c0a6629a 100644 --- a/src/lib/api-handler.ts +++ b/src/lib/api-handler.ts @@ -1046,10 +1046,38 @@ export const MFA_STEP_UP_MAX_AGE_SECONDS = 5 * 60; export type FreshMfaContext = AuthContext & { mfaVerifiedAt: Date }; +/** + * Which credentials count when asking "can this account produce a fresh-factor + * proof at all". + * + * The gate is the same either way — a `Session.mfaVerifiedAt` inside the window + * on the cookie path, an elevation minted from a `FRESH_FACTOR_METHODS` proof on + * the Bearer path. What differs is the pre-check that decides whether refusing + * would be a gate or a dead end, and the honest answer to that depends on what + * the route is protecting. + * + * `second-factor` — a confirmed TOTP secret or a registered second-factor + * security key. The default, and the right question for the routes that MANAGE + * a second factor: there is nothing there to protect if nothing is enrolled. + * + * `any-possession` — the above, OR a registered PRIMARY passkey. The right + * question for a route protecting the passkey itself, because a passkey is a + * possession factor in its own right and this codebase already treats it as + * one in three places: `/api/auth/passkey/login-verify` stamps `mfaVerifiedAt` + * on a passkey login (v1.23, M-review M1), `passkey` is in + * `FRESH_FACTOR_METHODS` so a passkey-proved elevation reaches the fresh-factor + * routes, and `resolveMfaEnrollmentRequired` counts a primary passkey as + * satisfying the enforcement policy outright. A passkey-only account can + * therefore clear the gate on both transports, and asking the narrower + * question would have thrown `mfa_not_enrolled` at an account that was holding + * the proof all along. + */ +export type FreshFactorProofSource = "second-factor" | "any-possession"; + /** * v1.23 — step-up gate. Passes only for a COOKIE session whose - * `Session.mfaVerifiedAt` is within `maxAgeSeconds` AND whose user has an - * active second factor (`totpConfirmedAt`). Throws `StepUpRequiredError` + * `Session.mfaVerifiedAt` is within `maxAgeSeconds` AND whose user holds a + * credential that can produce such a stamp. Throws `StepUpRequiredError` * (401, `errorCode: "auth.stepup.required"`) otherwise. * * Bearer tokens can NEVER satisfy this — exactly like `requireAdmin`, the @@ -1059,9 +1087,14 @@ export type FreshMfaContext = AuthContext & { mfaVerifiedAt: Date }; * * Consumed in Phase M by MFA disable + recovery-code regeneration; later * waves gate account deletion, key rotation, and passphrase export on it. + * + * @param proofSource which credentials count as "can prove a factor". Defaults + * to the second-factor set every existing caller means; see + * {@link FreshFactorProofSource}. */ export async function requireFreshMfa( maxAgeSeconds: number, + proofSource: FreshFactorProofSource = "second-factor", ): Promise { const sessionData = await getSession(); if (!sessionData) throw new HttpError(401, "Not authenticated"); @@ -1075,12 +1108,11 @@ export async function requireFreshMfa( }); } - // The user must actually have a second factor active. A single-factor - // account cannot produce a fresh-MFA proof, so step-up-gated actions are - // unreachable for it by design (the management UI gates enrolment first). - // Either factor counts: a confirmed TOTP secret OR a registered WebAuthn - // security key — both stamp `Session.mfaVerifiedAt` on a completed login. - if (!(await hasSecondFactorEnrolled(sessionData.user))) { + // The account must hold a credential that can stamp `mfaVerifiedAt` in the + // first place. Without one a step-up-gated action is unreachable by design + // rather than merely refused, so the refusal is named differently + // (`mfa_not_enrolled`) and the management UI gates enrolment ahead of it. + if (!(await canProveFreshFactor(sessionData.user, proofSource))) { throw new StepUpRequiredError("auth.stepup.mfa_not_enrolled"); } @@ -1127,7 +1159,7 @@ export async function requireFreshMfaIfEnrolled( } /** - * Does this account have a second factor at all? + * Does this account have a SECOND FACTOR at all? * * Either factor enrols it: a confirmed TOTP secret OR a registered WebAuthn * security key. A webauthn-only account must clear step-up too, so every @@ -1135,6 +1167,17 @@ export async function requireFreshMfaIfEnrolled( * rule rather than reading `totpConfirmedAt` alone. It was written out three * times before it was a function, which is two more places for the second arm * to be forgotten. + * + * DELIBERATELY DOES NOT COUNT A PRIMARY PASSKEY, even though a passkey login + * stamps `mfaVerifiedAt` and could therefore satisfy the gate. This predicate + * answers "is there a second factor", and that is the question + * `requireFreshMfaIfEnrolled` asks before deciding whether account deletion, the + * data reset, the password change, the encrypted export and key rotation demand + * a step-up at all. Counting a passkey here would silently pull every + * passkey-holding account into a gate those routes have never applied to them — + * a behaviour change to five destructive actions, made in passing, for the + * benefit of a sixth. {@link canProveFreshFactor} is where the wider question + * lives. */ async function hasSecondFactorEnrolled(user: { id: string; @@ -1147,6 +1190,27 @@ async function hasSecondFactorEnrolled(user: { return webauthnKeyCount > 0; } +/** + * Can this account produce a fresh-factor proof at all? + * + * The wider question, asked by the routes that gate on possession rather than + * on second-factor management. See {@link FreshFactorProofSource} for why a + * primary passkey answers yes and where the codebase already says so; see + * {@link hasSecondFactorEnrolled} for why the narrower predicate must not be + * widened into this one. + */ +async function canProveFreshFactor( + user: { id: string; totpConfirmedAt: Date | null }, + proofSource: FreshFactorProofSource, +): Promise { + if (await hasSecondFactorEnrolled(user)) return true; + if (proofSource === "second-factor") return false; + const passkeyCount = await prisma.passkey.count({ + where: { userId: user.id }, + }); + return passkeyCount > 0; +} + /** * A Bearer-resolved caller. * @@ -1274,34 +1338,28 @@ export type MfaManagementContext = { * * @param options.freshFactor mirrors the cookie path's `requireFreshMfa`. Set by * the destructive routes (disable, recovery-code rotation, security-key - * removal). - * - * `"if-enrolled"` is the third value and it is the one that needs explaining. - * `true` refuses an account with no second factor outright, which is correct - * for the routes that MANAGE a second factor — there is nothing to remove if - * nothing is enrolled. Passkey removal is not like that: the passkey is the - * primary sign-in credential, most accounts holding one have no second factor - * beside it, and a gate they cannot satisfy would not be a safeguard but a - * locked door in front of their own credential list. So `"if-enrolled"` - * applies the full gate to the accounts that can clear it and leaves the rest - * on the plain `requireAuth()` contract they had, which is the same line - * `requireFreshMfaIfEnrolled` draws for the destructive account actions. + * removal, passkey removal). + * @param options.proofSource which credentials count as "can prove a factor" + * when deciding whether the gate is reachable. Defaults to the second-factor + * set. Passkey removal passes `"any-possession"` because a primary passkey IS + * the proof there — see {@link FreshFactorProofSource}. It changes only the + * reachability pre-check; the gate itself is identical either way. */ export async function requireMfaManagementAuth( - options: { freshFactor?: boolean | "if-enrolled" } = {}, + options: { + freshFactor?: boolean; + proofSource?: FreshFactorProofSource; + } = {}, ): Promise { - const freshFactorMode = options.freshFactor ?? false; + const freshFactor = options.freshFactor === true; + const proofSource = options.proofSource ?? "second-factor"; // Cookie first, and via the original helpers — the web path runs the same // code it always did. const sessionData = await getSession(); if (sessionData) { - const freshFactor = - freshFactorMode === "if-enrolled" - ? await hasSecondFactorEnrolled(sessionData.user) - : freshFactorMode; const resolved = freshFactor - ? await requireFreshMfa(MFA_STEP_UP_MAX_AGE_SECONDS) + ? await requireFreshMfa(MFA_STEP_UP_MAX_AGE_SECONDS, proofSource) : await requireCookieAuth(); return { transport: "cookie", @@ -1315,24 +1373,6 @@ export async function requireMfaManagementAuth( // token is refused here and never gets as far as presenting an elevation. const auth = await requireBearerAuth(); - // Resolved BEFORE the header is read, because on `"if-enrolled"` an account - // with no second factor needs no elevation and must not be asked for one — - // demanding a header it can never mint would be the lockout this mode exists - // to avoid, wearing a 401. - const freshFactor = - freshFactorMode === "if-enrolled" - ? await hasSecondFactorEnrolled(auth.user) - : freshFactorMode; - if (freshFactorMode === "if-enrolled" && !freshFactor) { - return { - transport: "bearer", - user: auth.user, - apiTokenId: auth.apiTokenId, - accessTokenHash: auth.accessTokenHash, - commitElevation: async () => {}, - }; - } - let raw: string | null = null; try { const headerList = await headers(); @@ -1377,13 +1417,12 @@ export async function requireMfaManagementAuth( }); 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. - // Unreachable under `"if-enrolled"` — that mode returned above when the - // account had no factor, so arriving here with `freshFactor` true means the - // enrolment was already established. - if (freshFactor && !(await hasSecondFactorEnrolled(auth.user))) { + // Parity with the cookie path: `requireFreshMfa` refuses an account that holds + // no credential able to produce the proof, because a step-up-gated action is + // unreachable rather than merely refused there. The Bearer path asks the same + // question, with the same `proofSource`, rather than becoming the softer or + // the stricter route. + if (freshFactor && !(await canProveFreshFactor(auth.user, proofSource))) { throw new StepUpRequiredError("auth.stepup.mfa_not_enrolled"); } diff --git a/src/lib/openapi/routes/auth.ts b/src/lib/openapi/routes/auth.ts index 5b4dbfe3d..0c2715658 100644 --- a/src/lib/openapi/routes/auth.ts +++ b/src/lib/openapi/routes/auth.ts @@ -1529,7 +1529,8 @@ export const authPaths: NonNullable = { summary: "Remove a registered passkey", description: "Deletes one primary sign-in credential, scoped to the authenticated user. Refuses to remove the LAST one when the account has no password — an account must keep at least one way in.\n\n" + - "Step-up gated for accounts that can clear the gate, on the same mechanism as removing a second-factor security key (DELETE /api/auth/me/mfa/webauthn/{id}). An account with a second factor enrolled — a confirmed TOTP secret or a registered security key — must present a fresh factor proof: a cookie session that completed one within the last five minutes, or, on Bearer, an `X-Step-Up` elevation minted at POST /api/auth/step-up against a re-proved factor (a password-proved elevation is NOT enough). An account with no second factor enrolled is unaffected and keeps the plain session-or-token contract, because a gate it could never satisfy would lock it out of its own credential list.\n\n" + + "Step-up gated, on the same mechanism and at the same strength as removing a second-factor security key (DELETE /api/auth/me/mfa/webauthn/{id}). The caller must present a fresh possession proof: on the cookie path a session that completed one within the last five minutes, on Bearer an `X-Step-Up` elevation minted at POST /api/auth/step-up against a re-proved factor. A password-proved elevation is NOT enough.\n\n" + + 'A PASSKEY IS SUCH A PROOF here, unlike on the second-factor routes. An account whose only credential is a passkey can satisfy this gate: a passkey login stamps the session, and POST /api/auth/step-up accepts `method: "passkey"` against the account\'s primary passkeys. So its refusal is `meta.errorCode: auth.stepup.required` — re-prove and retry — and not the dead-end `auth.stepup.mfa_not_enrolled`, which is reserved for an account holding no credential this gate could ever accept.\n\n' + "The elevation is spent only when the deletion is about to happen. A 404 for an unknown id, or the last-credential refusal, leaves it unconsumed and reusable.", requestParams: { path: z.object({ id: z.string() }) }, responses: { @@ -1558,7 +1559,7 @@ export const authPaths: NonNullable = { // and the step-up contract disappears from the published operation. "401": { description: - "Not authenticated, or the account has a second factor and the request carried no proof of it fresh enough (`meta.errorCode` = `auth.stepup.required`). Re-prove the factor and retry — on the web by re-verifying, on a native client by minting an elevation at POST /api/auth/step-up. Nothing was removed.", + "Not authenticated, or the request carried no fresh possession proof (`meta.errorCode` = `auth.stepup.required`). Re-prove and retry — on a native client by minting an elevation at POST /api/auth/step-up, on the web by signing in again, which is what stamps the session. `auth.stepup.mfa_not_enrolled` is the other arm and means the account holds no credential this gate can accept, so retrying will not help. Nothing was removed either way.", content: { "application/json": { schema: errorEnvelope } }, }, }, diff --git a/tests/integration/step-up-elevation.test.ts b/tests/integration/step-up-elevation.test.ts index 888f14081..e6a82cbd4 100644 --- a/tests/integration/step-up-elevation.test.ts +++ b/tests/integration/step-up-elevation.test.ts @@ -1157,33 +1157,42 @@ describe("E13 — the second-factor status read needs no elevation", () => { }); }); -// ── E14 — passkey removal joins the gate, without locking anyone out ── +// ── E14 — passkey removal is gated, and a passkey satisfies the gate ── /** * `DELETE /api/auth/passkeys/{id}` used to take a plain cookie session or a * wildcard Bearer, while the second-factor key beside it demanded a fresh * proof. That was the softer gate on the PRIMARY sign-in credential, so it now - * goes through `requireMfaManagementAuth({ freshFactor: "if-enrolled" })`. + * goes through `requireMfaManagementAuth({ freshFactor: true, proofSource: + * "any-possession" })`. * - * The mode is the part worth pinning behaviourally, because it is the part that - * can go wrong in two opposite directions and a status-only test would miss - * both: + * THE PROOF SOURCE IS THE PART THAT NEEDS PROVING, and the cases split on the + * difference between the two refusals rather than on the status, because both + * are 401 and only one of them is survivable: * - * - gate everyone, and an account with no second factor — which is most - * accounts holding a passkey — can never remove its own credential again. - * That is a lockout dressed as a safeguard; - * - gate nobody, and the change did nothing. + * `auth.stepup.required` — re-prove and retry. A gate. + * `auth.stepup.mfa_not_enrolled` — the account holds nothing that could ever + * produce the proof. A dead end, and the reachability pre-check is what + * decides which one a caller gets. * - * The Bearer arm is here for the same reason. `requireFreshMfa` is cookie-only, - * so a gate built on it alone would have answered the native client with "use - * the website". These cases prove the app keeps the capability by minting the - * same elevation it already mints to remove a security key. + * A passkey-only account holds no SECOND factor, so the default pre-check would + * hand it the dead end over a proof it was already carrying — its passkey stamps + * `mfaVerifiedAt` on login and is one of the mint's arms. `"any-possession"` is + * what makes its refusal the survivable one, and E14b then shows it actually + * surviving: a session stamped the way `/api/auth/passkey/login-verify` stamps + * it goes straight through. * - * Mutation check: change the route's option to `true` and the two unenrolled - * cases go red; drop the option entirely and the two refusals go red; move - * `commitElevation()` above the 404 branch and the last case goes red. + * The Bearer happy path is driven with TOTP rather than a passkey assertion for + * the reason `enrolTotp` above already gives — it is the only fresh-factor arm + * reachable without a real authenticator. What it proves is the redemption and + * the spend; the passkey arm's admission is proved by E14a/E14b on the pre-check + * that governs both transports. + * + * Mutation check: drop `proofSource` from the route and E14a's errorCode + * assertion plus E14b go red; drop `freshFactor` and E14a, E14c and E14d's spend + * go red; move `commitElevation()` above the 404 branch and E14f goes red. */ -describe("E14 — removing a passkey is step-up gated for enrolled accounts", () => { +describe("E14 — removing a passkey needs a fresh possession proof", () => { async function seedPasskey(label: string): Promise { const row = await getPrismaClient().passkey.create({ data: { @@ -1214,42 +1223,64 @@ describe("E14 — removing a passkey is step-up gated for enrolled accounts", () return session.id; } - it("lets a Bearer client with no second factor remove one, as before", async () => { + async function errorCodeOf(res: Response): Promise { + const body = (await res.json()) as { meta?: { errorCode?: string } }; + return body.meta?.errorCode; + } + + it("refuses a passkey-only cookie session, and the refusal is re-provable", async () => { const passkeyId = await seedPasskey("e14a"); - const { raw } = await mintToken("e14a"); - useToken(raw); - useElevation(null); - - expect((await callDelete(passkeyId)).status).toBe(200); - expect(await getPrismaClient().passkey.count()).toBe(0); - }); - - it("lets a cookie session with no second factor remove one, as before", async () => { - const passkeyId = await seedPasskey("e14b"); + // No TOTP, no security key. Only the passkey it is trying to remove. await useCookieSession(); - expect((await callDelete(passkeyId)).status).toBe(200); - expect(await getPrismaClient().passkey.count()).toBe(0); - }); + const res = await callDelete(passkeyId); - it("refuses a Bearer client on an enrolled account with no elevation", async () => { - const passkeyId = await seedPasskey("e14c"); - await enrolTotp(); - const { raw } = await mintToken("e14c"); - useToken(raw); - useElevation(null); - - expect((await callDelete(passkeyId)).status).toBe(401); - // The refusal is only worth anything if the row survived it. + expect(res.status).toBe(401); + // The whole point: NOT `mfa_not_enrolled`. This account can re-prove. + expect(await errorCodeOf(res)).toBe("auth.stepup.required"); expect(await getPrismaClient().passkey.count()).toBe(1); }); - it("refuses a cookie session on an enrolled account with no fresh factor", async () => { - const passkeyId = await seedPasskey("e14d"); - await enrolTotp(); - await useCookieSession(); + it("admits that same account once its session carries a passkey login", async () => { + const passkeyId = await seedPasskey("e14b"); + const sessionId = await useCookieSession(); + // Exactly what `/api/auth/passkey/login-verify` writes on a passkey login + // (v1.23, M-review M1). Still no second factor anywhere on the account. + await getPrismaClient().session.update({ + where: { id: sessionId }, + data: { mfaVerifiedAt: new Date() }, + }); - expect((await callDelete(passkeyId)).status).toBe(401); + expect((await callDelete(passkeyId)).status).toBe(200); + expect(await getPrismaClient().passkey.count()).toBe(0); + }); + + it("refuses a stale passkey login, so the window is real", async () => { + const passkeyId = await seedPasskey("e14c"); + const sessionId = await useCookieSession(); + await getPrismaClient().session.update({ + where: { id: sessionId }, + data: { + mfaVerifiedAt: new Date(Date.now() - 6 * 60 * 1000), + }, + }); + + const res = await callDelete(passkeyId); + + expect(res.status).toBe(401); + expect(await errorCodeOf(res)).toBe("auth.stepup.required"); + expect(await getPrismaClient().passkey.count()).toBe(1); + }); + + it("refuses a Bearer client presenting no elevation", async () => { + const passkeyId = await seedPasskey("e14d"); + const { raw } = await mintToken("e14d"); + useToken(raw); + useElevation(null); + + const res = await callDelete(passkeyId); + + expect(res.status).toBe(401); expect(await getPrismaClient().passkey.count()).toBe(1); }); @@ -1285,4 +1316,17 @@ describe("E14 — removing a passkey is step-up gated for enrolled accounts", () }); expect(row?.consumedAt).toBeNull(); }); + + it("tells an account holding nothing that there is nothing to prove with", async () => { + // No passkey, no TOTP, no security key. There is no credential this gate + // could ever accept, and the refusal says so rather than inviting a + // re-verification that cannot happen. Reachable only with a stale id — an + // account with no passkey has nothing here to delete. + await useCookieSession(); + + const res = await callDelete("pk-does-not-exist"); + + expect(res.status).toBe(401); + expect(await errorCodeOf(res)).toBe("auth.stepup.mfa_not_enrolled"); + }); });