diff --git a/docs/api/openapi.yaml b/docs/api/openapi.yaml index 70df81108..8c86d7bba 100644 --- a/docs/api/openapi.yaml +++ b/docs/api/openapi.yaml @@ -2581,9 +2581,21 @@ paths: the account has no password — an account must keep at least one way in. - No step-up: this endpoint takes a plain cookie session or a wildcard Bearer token, where removing a - second-factor security key (DELETE /api/auth/me/mfa/webauthn/{id}) requires a fresh factor proof. The asymmetry - is in the code as it stands, not a description of intent. + 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 + last-credential refusal, leaves it unconsumed and reusable. parameters: - in: path name: id @@ -2604,7 +2616,16 @@ paths: application/json: schema: $ref: "#/components/schemas/ErrorEnvelope" - "401": *a1 + "401": + 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: + $ref: "#/components/schemas/ErrorEnvelope" "404": description: No such passkey for this account. content: @@ -7701,18 +7722,27 @@ paths: summary: Reset one threshold override, or all of them description: >- With `?metric=` it drops that metric's override; WITHOUT the parameter it drops EVERY override on the account. - There is no confirmation step and no dry run, so a client that means to reset one metric must send the - parameter. + The wide form is not reachable by omitting the parameter alone — it requires `{ "confirm": "RESET_THRESHOLDS" }` + in the body, the same typed-confirmation shape account deletion and the data reset use. The `?metric=` form + takes no body and is unchanged. - Audit-logged with the before and after maps. Unlike the PUT this path runs no rate limit of its own. + Audit-logged with the before and after maps. Rate-limited 30 / 5 min per user, matching the PUT. parameters: - in: query name: metric schema: - description: The single metric to reset. Omit to reset every override on the account. + description: The single metric to reset. Omit to reset every override on the account — which then requires the + confirmation body. $ref: "#/components/schemas/ThresholdMetric" - description: The single metric to reset. Omit to reset every override on the account. + description: The single metric to reset. Omit to reset every override on the account — which then requires the + confirmation body. + requestBody: + required: false + content: + application/json: + schema: + $ref: "#/components/schemas/ThresholdsResetAllRequest" responses: "200": description: The remaining override map. @@ -7727,7 +7757,12 @@ paths: schema: $ref: "#/components/schemas/ErrorEnvelope" "401": *a1 - "422": *a4 + "422": + description: '`metric` was omitted and the body did not carry `{ "confirm": "RESET_THRESHOLDS" }`. Nothing was reset.' + content: + application/json: + schema: + $ref: "#/components/schemas/ErrorEnvelope" "429": *a5 /api/auth/me/devices: get: @@ -23030,6 +23065,17 @@ components: - TOTAL_BODY_WATER - BONE_MASS - OXYGEN_SATURATION + ThresholdsResetAllRequest: + type: object + properties: + confirm: + type: string + const: RESET_THRESHOLDS + description: Required when `metric` is omitted. Compared exactly; any other value is refused. + required: + - confirm + description: Confirmation body for the reset-everything form. Send no body at all when resetting a single metric with + `?metric=`. DevicePatchRequest: type: object properties: diff --git a/messages/de.json b/messages/de.json index a2d2e6d9e..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", @@ -6922,7 +6923,14 @@ "restoreSkippedPersonalRecordReference": "Herkunft einer Bestleistung", "restoreSkippedEcgReference": "Herkunft eines EKG-Streifens", "restoreSkippedMedicationTarget": "Zielwert-Analyt einer Medikation", - "restoreSkippedScheduleRevisionLink": "Abfolge der Schema-Zeiträume" + "restoreSkippedScheduleRevisionLink": "Abfolge der Schema-Zeiträume", + "restoreMissingTitle": "Wiederherstellung abgelehnt — die Datei ist unvollständig", + "restoreMissingDescription": "Das Manifest der Datei sagt, sie enthalte den folgenden Abschnitt, und sie enthält ihn nicht. Es wurde nichts geändert: Eine Wiederherstellung ersetzt eine ganze Datenklasse, ein fehlender Abschnitt hätte sie also geleert.", + "restoreMissingRemedy": "Nimm eine Notfall-Sicherung oder exportiere das Konto erneut und lade die neue Datei hoch. Ein Abschnitt, den das Manifest ausdrücklich als weggelassen ausweist, ist etwas anderes und wird normal wiederhergestellt.", + "restoreMissingDocuments": "Dokumente", + "restoreMissingWorkouts": "Workouts", + "restoreMissingMentalHealth": "Psychische Fragebögen", + "restoreMissingConsent": "Einwilligungsnachweise" }, "danger-zone": { "title": "Gefahrenzone", diff --git a/messages/en.json b/messages/en.json index ba0a5ac87..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", @@ -6922,7 +6923,14 @@ "restoreSkippedPersonalRecordReference": "Personal best provenance", "restoreSkippedEcgReference": "ECG strip provenance", "restoreSkippedMedicationTarget": "Medication target analyte", - "restoreSkippedScheduleRevisionLink": "Schedule era succession" + "restoreSkippedScheduleRevisionLink": "Schedule era succession", + "restoreMissingTitle": "Restore refused — the file is incomplete", + "restoreMissingDescription": "The file's own manifest says it carries the section below, and it does not. Nothing was changed: a restore replaces a whole class of records, so putting back a section that is not in the file would have emptied it.", + "restoreMissingRemedy": "Use a disaster-recovery snapshot, or export the account again and upload the new file. A section the manifest declares as omitted is a different thing and restores normally.", + "restoreMissingDocuments": "Documents", + "restoreMissingWorkouts": "Workouts", + "restoreMissingMentalHealth": "Mental-health screeners", + "restoreMissingConsent": "Consent receipts" }, "danger-zone": { "title": "Danger Zone", diff --git a/messages/es.json b/messages/es.json index 632a9f9ed..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", @@ -6922,7 +6923,14 @@ "restoreSkippedPersonalRecordReference": "Procedencia de un récord personal", "restoreSkippedEcgReference": "Procedencia de un trazado de ECG", "restoreSkippedMedicationTarget": "Analito objetivo de un medicamento", - "restoreSkippedScheduleRevisionLink": "Sucesión de periodos de pauta" + "restoreSkippedScheduleRevisionLink": "Sucesión de periodos de pauta", + "restoreMissingTitle": "Restauración rechazada: el archivo está incompleto", + "restoreMissingDescription": "El propio manifiesto del archivo dice que incluye la sección siguiente, y no la incluye. No se cambió nada: una restauración sustituye toda una clase de registros, así que una sección ausente la habría vaciado.", + "restoreMissingRemedy": "Usa una copia de recuperación ante desastres, o vuelve a exportar la cuenta y sube el archivo nuevo. Una sección que el manifiesto declara omitida es otra cosa y se restaura con normalidad.", + "restoreMissingDocuments": "Documentos", + "restoreMissingWorkouts": "Entrenamientos", + "restoreMissingMentalHealth": "Cuestionarios de salud mental", + "restoreMissingConsent": "Comprobantes de consentimiento" }, "danger-zone": { "title": "Zona de peligro", diff --git a/messages/fr.json b/messages/fr.json index 5e03401f1..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", @@ -6922,7 +6923,14 @@ "restoreSkippedPersonalRecordReference": "Provenance d'un record personnel", "restoreSkippedEcgReference": "Provenance d'un tracé ECG", "restoreSkippedMedicationTarget": "Analyte cible d'un médicament", - "restoreSkippedScheduleRevisionLink": "Succession des périodes de schéma" + "restoreSkippedScheduleRevisionLink": "Succession des périodes de schéma", + "restoreMissingTitle": "Restauration refusée — le fichier est incomplet", + "restoreMissingDescription": "Le manifeste du fichier indique qu'il contient la section ci-dessous, et il ne la contient pas. Rien n'a été modifié : une restauration remplace toute une classe d'enregistrements, une section absente l'aurait donc vidée.", + "restoreMissingRemedy": "Utilise une sauvegarde de reprise après sinistre, ou réexporte le compte et téléverse le nouveau fichier. Une section que le manifeste déclare omise est autre chose et se restaure normalement.", + "restoreMissingDocuments": "Documents", + "restoreMissingWorkouts": "Séances", + "restoreMissingMentalHealth": "Questionnaires de santé mentale", + "restoreMissingConsent": "Preuves de consentement" }, "danger-zone": { "title": "Zone de danger", diff --git a/messages/it.json b/messages/it.json index eb02d4511..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", @@ -6922,7 +6923,14 @@ "restoreSkippedPersonalRecordReference": "Provenienza di un primato personale", "restoreSkippedEcgReference": "Provenienza di un tracciato ECG", "restoreSkippedMedicationTarget": "Analita obiettivo di un farmaco", - "restoreSkippedScheduleRevisionLink": "Successione dei periodi di schema" + "restoreSkippedScheduleRevisionLink": "Successione dei periodi di schema", + "restoreMissingTitle": "Ripristino rifiutato — il file è incompleto", + "restoreMissingDescription": "Il manifesto del file dichiara di contenere la sezione qui sotto, e non la contiene. Non è stato modificato nulla: un ripristino sostituisce un'intera classe di record, quindi una sezione assente l'avrebbe svuotata.", + "restoreMissingRemedy": "Usa uno snapshot di disaster recovery, oppure esporta di nuovo l'account e carica il file nuovo. Una sezione che il manifesto dichiara omessa è un'altra cosa e si ripristina normalmente.", + "restoreMissingDocuments": "Documenti", + "restoreMissingWorkouts": "Allenamenti", + "restoreMissingMentalHealth": "Questionari sulla salute mentale", + "restoreMissingConsent": "Ricevute di consenso" }, "danger-zone": { "title": "Zona pericolosa", diff --git a/messages/pl.json b/messages/pl.json index 18514d304..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", @@ -6922,7 +6923,14 @@ "restoreSkippedPersonalRecordReference": "Pochodzenie rekordu osobistego", "restoreSkippedEcgReference": "Pochodzenie zapisu EKG", "restoreSkippedMedicationTarget": "Analit docelowy leku", - "restoreSkippedScheduleRevisionLink": "Następstwo okresów schematu" + "restoreSkippedScheduleRevisionLink": "Następstwo okresów schematu", + "restoreMissingTitle": "Przywracanie odrzucone — plik jest niekompletny", + "restoreMissingDescription": "Manifest pliku mówi, że zawiera on poniższą sekcję, a jej nie zawiera. Nic nie zostało zmienione: przywracanie zastępuje całą klasę rekordów, więc brakująca sekcja by ją opróżniła.", + "restoreMissingRemedy": "Użyj kopii do odtwarzania po awarii albo wyeksportuj konto ponownie i wgraj nowy plik. Sekcja, którą manifest jawnie deklaruje jako pominiętą, to co innego i przywraca się normalnie.", + "restoreMissingDocuments": "Dokumenty", + "restoreMissingWorkouts": "Treningi", + "restoreMissingMentalHealth": "Kwestionariusze zdrowia psychicznego", + "restoreMissingConsent": "Potwierdzenia zgody" }, "danger-zone": { "title": "Strefa zagrożenia", diff --git a/src/__tests__/step-up-elevation-guard.test.ts b/src/__tests__/step-up-elevation-guard.test.ts index a8c4ec8b6..16589e1d1 100644 --- a/src/__tests__/step-up-elevation-guard.test.ts +++ b/src/__tests__/step-up-elevation-guard.test.ts @@ -87,8 +87,25 @@ function callers(exportName: string, moduleRe: RegExp): string[] { } /** - * Every route an elevation can unlock. This is the whole set — the second-factor - * management MUTATIONS and nothing besides. + * Every route an elevation can unlock: the second-factor management MUTATIONS, + * plus the one credential removal that is not a second factor and belongs here + * anyway. + * + * 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 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 + * as the rest of the list — a credential the caller owns, removed after they + * prove they are the caller — and it is written down here rather than inferred, + * which is the whole point of this file. * * 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 @@ -108,6 +125,7 @@ function callers(exportName: string, moduleRe: RegExp): string[] { * exists to prevent. */ const ELEVATION_ROUTES = [ + "app/api/auth/passkeys/[id]/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", diff --git a/src/app/api/admin/backups/[id]/restore/route.ts b/src/app/api/admin/backups/[id]/restore/route.ts index f15f3bc8a..ec71b6817 100644 --- a/src/app/api/admin/backups/[id]/restore/route.ts +++ b/src/app/api/admin/backups/[id]/restore/route.ts @@ -40,6 +40,7 @@ import { import { recomputeUserRollups } from "@/lib/rollups/measurement-rollups"; import { restoreCycleData } from "@/lib/cycle/backup"; import { + findMissingBackupSections, recordUnknownKeys, summarizeRestoreSkips, type RestoreSkipLog, @@ -193,9 +194,14 @@ const handler = apiHandler( return apiError("Failed to decrypt backup payload", 500); } + // Parsed once and kept, because the schema's per-section `.default([])` + // erases the difference between a section that is absent and one that is + // empty — and that difference is what the completeness check below reads. + let raw: unknown; let payload; try { - payload = parseBackupPayload(plaintext); + raw = JSON.parse(plaintext); + payload = parseBackupPayload(raw); } catch (err) { await auditLog("admin.backups.restore.failed", { userId: admin.id, @@ -289,6 +295,39 @@ const handler = apiHandler( }, }); + // A section the file's own manifest says it carries, and does not. + // + // The restore's first act is to delete the class it is about to rebuild, so + // an absent section is not a gap in the file — it is an erasure of that part + // of the account, reported as a success. #237: a portable file whose + // document payload was gone emptied the vault and said nothing. The whole + // file is refused instead, above the transaction, for the same reason the + // metadata-only check below sits there. + // + // A DECLARED omission is not this. The portable export leaves the screener + // administrations and the consent receipts out on purpose and says so in the + // manifest; `findMissingBackupSections` reads the manifest and not the + // emptiness of an array, so those files stay restorable. So does every file + // written before the manifest existed, which declares nothing at all. + const missingSections = findMissingBackupSections(raw); + if (missingSections.length > 0) { + await auditLog("admin.backups.restore.failed", { + userId: admin.id, + ipAddress: getClientIp(request), + details: { + backupId: id, + ownerId, + reason: "section_missing", + sections: missingSections, + }, + }); + return apiError( + `Backup is missing ${missingSections.join(", ")} — the file's own manifest says it carries ${missingSections.length === 1 ? "that section" : "those sections"}. Nothing was changed. Use a disaster-recovery snapshot, or export the account again.`, + 422, + { errorCode: "backup.section.missing", sections: missingSections }, + ); + } + // Portable exports intentionally omit document ciphertext. They remain // valid upload/download artifacts, but cannot be used to manufacture an // InboundDocument row. A DR restore must fail before any delete rather 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 ee81a5d11..c4e4adde5 100644 --- a/src/app/api/auth/passkeys/[id]/__tests__/route.test.ts +++ b/src/app/api/auth/passkeys/[id]/__tests__/route.test.ts @@ -16,6 +16,7 @@ import { describe, it, expect, vi, beforeEach } from "vitest"; vi.mock("@/lib/api-handler", () => ({ apiHandler: (fn: unknown) => fn, requireAuth: vi.fn(), + requireMfaManagementAuth: vi.fn(), })); vi.mock("@/lib/db", () => ({ @@ -35,8 +36,8 @@ vi.mock("@/lib/auth/audit", () => ({ auditLog: vi.fn().mockResolvedValue(undefined), })); -import { PATCH } from "../route"; -import { requireAuth } from "@/lib/api-handler"; +import { DELETE, PATCH } from "../route"; +import { requireAuth, requireMfaManagementAuth } from "@/lib/api-handler"; import { prisma } from "@/lib/db"; const USER = { id: "user-1", username: "u", passwordHash: "argon2id$…" }; @@ -70,9 +71,18 @@ type Handler = ( ctx: { params: Promise<{ id: string }> }, ) => Promise; +const commitElevation = vi.fn().mockResolvedValue(undefined); + beforeEach(() => { vi.resetAllMocks(); vi.mocked(requireAuth).mockResolvedValue({ user: USER } as never); + commitElevation.mockClear().mockResolvedValue(undefined); + vi.mocked(requireMfaManagementAuth).mockResolvedValue({ + transport: "cookie", + user: USER, + session: { id: "sess-1", expiresAt: new Date(Date.now() + 3_600_000) }, + commitElevation, + } as never); }); describe("PATCH /api/auth/passkeys/{id} — the rename refusal", () => { @@ -155,3 +165,108 @@ describe("PATCH /api/auth/passkeys/{id} — the rename refusal", () => { expect(prisma.passkey.update).not.toHaveBeenCalled(); }); }); + +/** + * 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 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 both halves of the right + * question, and it spends the proof only when it is about to delete. + * + * 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 = () => + new Request("http://localhost/api/auth/passkeys/pk-1", { + method: "DELETE", + }); + + it("asks for a full step-up, against any possession factor", async () => { + vi.mocked(prisma.passkey.findUnique).mockResolvedValue({ + id: "pk-1", + userId: USER.id, + name: "Phone", + } as never); + vi.mocked(prisma.passkey.count).mockResolvedValue(2 as never); + vi.mocked(prisma.passkey.delete).mockResolvedValue({} as never); + + await (DELETE as unknown as Handler)(mkDelete(), { params }); + + expect(requireMfaManagementAuth).toHaveBeenCalledWith({ + freshFactor: true, + proofSource: "any-possession", + }); + // 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, + }); + }); + + it("spends the proof once, on the way to the delete", async () => { + vi.mocked(prisma.passkey.findUnique).mockResolvedValue({ + id: "pk-1", + userId: USER.id, + name: "Phone", + } as never); + vi.mocked(prisma.passkey.count).mockResolvedValue(2 as never); + vi.mocked(prisma.passkey.delete).mockResolvedValue({} as never); + + const res = await (DELETE as unknown as Handler)(mkDelete(), { params }); + + expect(res.status).toBe(200); + expect(commitElevation).toHaveBeenCalledTimes(1); + expect(prisma.passkey.delete).toHaveBeenCalledWith({ + where: { id: "pk-1" }, + }); + }); + + it("does not spend the proof on another account's passkey id", async () => { + vi.mocked(prisma.passkey.findUnique).mockResolvedValue({ + id: "pk-1", + userId: "someone-else", + name: "Not yours", + } as never); + + const res = await (DELETE as unknown as Handler)(mkDelete(), { params }); + + expect(res.status).toBe(404); + expect(commitElevation).not.toHaveBeenCalled(); + expect(prisma.passkey.delete).not.toHaveBeenCalled(); + }); + + it("does not spend the proof when the last credential is refused", async () => { + vi.mocked(prisma.passkey.findUnique).mockResolvedValue({ + id: "pk-1", + userId: USER.id, + name: "Only one", + } as never); + vi.mocked(prisma.passkey.count).mockResolvedValue(1 as never); + vi.mocked(requireMfaManagementAuth).mockResolvedValue({ + transport: "cookie", + user: { ...USER, passwordHash: null }, + session: { id: "sess-1", expiresAt: new Date(Date.now() + 3_600_000) }, + commitElevation, + } as never); + + const res = await (DELETE as unknown as Handler)(mkDelete(), { params }); + + expect(res.status).toBe(400); + expect(commitElevation).not.toHaveBeenCalled(); + expect(prisma.passkey.delete).not.toHaveBeenCalled(); + }); +}); diff --git a/src/app/api/auth/passkeys/[id]/route.ts b/src/app/api/auth/passkeys/[id]/route.ts index 5c7ad7788..b8a79f6cd 100644 --- a/src/app/api/auth/passkeys/[id]/route.ts +++ b/src/app/api/auth/passkeys/[id]/route.ts @@ -8,7 +8,11 @@ import { safeJson, } from "@/lib/api-response"; import { NextRequest } from "next/server"; -import { apiHandler, requireAuth } from "@/lib/api-handler"; +import { + apiHandler, + requireAuth, + requireMfaManagementAuth, +} from "@/lib/api-handler"; import { annotate } from "@/lib/logging/context"; import { passkeyRenameSchema } from "@/lib/validations/auth"; @@ -61,12 +65,57 @@ export const PATCH = apiHandler( }, ); +/** + * Remove a passkey — step-up gated, on the same mechanism the second-factor + * security-key removal uses. + * + * The two deletions were not equals. Removing a registered SECOND factor + * demanded a fresh factor proof; removing a passkey, which is the PRIMARY + * sign-in credential, took a plain cookie session or a wildcard Bearer. The + * last-method check below stops an account locking itself out, but it does + * nothing about a hijacked session quietly stripping the credentials the owner + * signs in with. So this route goes through `requireMfaManagementAuth` too. + * + * 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 — 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. + * + * 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 { user } = await requireAuth(); + const auth = await requireMfaManagementAuth({ + freshFactor: true, + proofSource: "any-possession", + }); + const { user } = auth; const { id } = await params; @@ -91,6 +140,12 @@ export const DELETE = apiHandler( ); } + // Ownership resolved and the last-method check passed — spend the elevation + // now, so a 404 for someone else's id or a refused last credential does not + // burn a proof the caller then has to mint again. Same ordering as the + // security-key removal. + await auth.commitElevation(); + await prisma.passkey.delete({ where: { id } }); await auditLog("auth.passkey.delete", { diff --git a/src/app/api/user/thresholds/__tests__/route.test.ts b/src/app/api/user/thresholds/__tests__/route.test.ts index dca481f73..c8524b19d 100644 --- a/src/app/api/user/thresholds/__tests__/route.test.ts +++ b/src/app/api/user/thresholds/__tests__/route.test.ts @@ -32,6 +32,9 @@ vi.mock("@/lib/analytics/effective-range", async (importOriginal) => { }; }); vi.mock("@/lib/logging/transports", () => ({ emitIfSampled: vi.fn() })); +vi.mock("@/lib/cache/invalidate", () => ({ + invalidateUserProfile: vi.fn(), +})); vi.mock("@/lib/db-compat", () => ({ ensureDbCompatibility: vi.fn().mockResolvedValue(undefined), })); @@ -44,7 +47,7 @@ vi.mock("next/headers", () => ({ })), })); -import { GET, PUT } from "../route"; +import { GET, PUT, DELETE } from "../route"; import { getSession } from "@/lib/auth/session"; import { checkRateLimit } from "@/lib/rate-limit"; import { prisma } from "@/lib/db"; @@ -63,6 +66,31 @@ function putReq(body: unknown): NextRequest { }); } +function deleteReq( + options: { metric?: string; body?: unknown } = {}, +): NextRequest { + const url = options.metric + ? `http://localhost/api/user/thresholds?metric=${options.metric}` + : "http://localhost/api/user/thresholds"; + return new NextRequest(url, { + method: "DELETE", + ...(options.body === undefined + ? {} + : { + headers: { "content-type": "application/json" }, + body: JSON.stringify(options.body), + }), + }); +} + +/** The stored override set the DELETE branches read before they write. */ +function armOverrides(overrides: Record) { + vi.mocked(prisma.user.findUnique).mockResolvedValue({ + thresholdsJson: overrides, + } as never); + vi.mocked(prisma.user.update).mockResolvedValue({} as never); +} + beforeEach(() => { vi.resetAllMocks(); vi.mocked(getSession).mockResolvedValue(SESSION_OK as never); @@ -111,6 +139,121 @@ describe("PUT /api/user/thresholds — 422 multi-issue (v1.4.43 W6)", () => { }); }); +/** + * The parameterless DELETE erases every band the account has tuned, and it + * used to do it on an omitted query parameter alone — no confirmation, no dry + * run, and no rate limit of its own while the PUT beside it had one. + * + * The confirmation is the same shape the other two wide deletes use + * (`/api/settings/account` wants `DELETE_ACCOUNT`, `/api/settings/data` wants + * `DELETE`): a literal in the body, compared exactly. What is pinned here is + * the pair of properties that make it worth anything — the refusal happens, + * AND nothing was written when it does. A status assertion on its own would + * stay green if the wipe ran first and the 422 came after it. + * + * The single-metric form is pinned as UNCHANGED in the same block, because the + * failure mode of a confirmation is that it spreads to the narrow path and + * breaks the targets sheet, which resets metrics in a loop. + * + * Mutation check: drop the `confirm !== RESET_ALL_CONFIRMATION` branch and the + * first two cases go red; drop the rate-limit call and the fourth goes red; + * extend the confirmation to the `?metric=` arm and the fifth goes red. + */ +describe("DELETE /api/user/thresholds — the wide form asks first", () => { + it("refuses the parameterless form with no body", async () => { + armOverrides({ WEIGHT: { min: 60, max: 80 } }); + + const res = await DELETE(deleteReq()); + + expect(res.status).toBe(422); + const body = (await res.json()) as { error: string }; + expect(body.error).toContain("RESET_THRESHOLDS"); + expect(prisma.user.update).not.toHaveBeenCalled(); + }); + + it("refuses a body whose confirmation is the wrong token", async () => { + armOverrides({ WEIGHT: { min: 60, max: 80 } }); + + const res = await DELETE(deleteReq({ body: { confirm: "DELETE" } })); + + expect(res.status).toBe(422); + expect(prisma.user.update).not.toHaveBeenCalled(); + }); + + it("wipes every override once the confirmation is present", async () => { + armOverrides({ WEIGHT: { min: 60, max: 80 }, PULSE: { min: 50, max: 90 } }); + + const res = await DELETE( + deleteReq({ body: { confirm: "RESET_THRESHOLDS" } }), + ); + + expect(res.status).toBe(200); + const body = (await res.json()) as { data: { overrides: unknown } }; + expect(body.data.overrides).toEqual({}); + expect(prisma.user.update).toHaveBeenCalledTimes(1); + }); + + it("runs the sibling's rate limit and honours a refusal", async () => { + armOverrides({ WEIGHT: { min: 60, max: 80 } }); + vi.mocked(checkRateLimit).mockResolvedValue({ allowed: false } as never); + + const res = await DELETE( + deleteReq({ body: { confirm: "RESET_THRESHOLDS" } }), + ); + + expect(res.status).toBe(429); + expect(checkRateLimit).toHaveBeenCalledWith( + "thresholds:reset:user-1", + 30, + 5 * 60 * 1000, + ); + expect(prisma.user.update).not.toHaveBeenCalled(); + }); + + /** + * The other end. A confirmation the server demands and the only client that + * sends the wide form does not send is not a safeguard, it is an outage — + * and it would look like a passing suite, because the route tests above + * construct their own requests. So the token is compared across the two + * files rather than assumed to match. + */ + it("is the same token the settings card sends", async () => { + const { readFileSync } = await import("node:fs"); + const { join } = await import("node:path"); + + const route = readFileSync( + join(process.cwd(), "src/app/api/user/thresholds/route.ts"), + "utf8", + ); + const client = readFileSync( + join( + process.cwd(), + "src/components/settings/thresholds-editor-section.tsx", + ), + "utf8", + ); + + const declared = route.match(/RESET_ALL_CONFIRMATION = "([^"]+)"/)?.[1]; + expect(declared, "the route no longer declares a confirmation").toBe( + "RESET_THRESHOLDS", + ); + expect(client).toContain(`confirm: "${declared}"`); + }); + + it("leaves the single-metric form exactly as it was", async () => { + armOverrides({ WEIGHT: { min: 60, max: 80 }, PULSE: { min: 50, max: 90 } }); + + // No body at all — the targets sheet sends none, and must keep working. + const res = await DELETE(deleteReq({ metric: "WEIGHT" })); + + expect(res.status).toBe(200); + const body = (await res.json()) as { + data: { overrides: Record }; + }; + expect(Object.keys(body.data.overrides)).toEqual(["PULSE"]); + }); +}); + /** * The docblock promised a key the handler has never returned. * diff --git a/src/app/api/user/thresholds/route.ts b/src/app/api/user/thresholds/route.ts index 5034ae027..22fcce714 100644 --- a/src/app/api/user/thresholds/route.ts +++ b/src/app/api/user/thresholds/route.ts @@ -9,7 +9,10 @@ * it is worth stating the absence rather than just deleting the word. * PUT accepts a partial map — only the provided metrics are updated, others * keep their existing override or remain on the default. - * DELETE with ?metric=... resets one metric to default. + * DELETE with ?metric=... resets one metric to default. WITHOUT the parameter + * it drops every override the account has, so it demands the typed + * confirmation the other wide-blast-radius deletes demand — see + * `RESET_ALL_CONFIRMATION` below. */ import { apiHandler, requireAuth } from "@/lib/api-handler"; import { @@ -118,6 +121,21 @@ export const PUT = apiHandler(async (request: NextRequest) => { return apiSuccess({ overrides: merged }); }); +/** + * What the parameterless DELETE has to be told before it wipes every override. + * + * Same shape as `/api/settings/account` (`DELETE_ACCOUNT`) and + * `/api/settings/data` (`DELETE`): a literal in the JSON body, compared + * exactly, with no default. The point is that the wide form cannot be reached + * by dropping a query parameter — a request that means "reset everything" has + * to say so in a second place. + * + * NOT applied to `?metric=`. That form resets one band the person can retype + * from the editor beside it, and the targets sheet fires it in a loop over the + * metrics it owns; a confirmation there would be ceremony without a decision. + */ +const RESET_ALL_CONFIRMATION = "RESET_THRESHOLDS"; + export const DELETE = apiHandler(async (request: NextRequest) => { const { user } = await requireAuth(); const url = new URL(request.url); @@ -128,6 +146,43 @@ export const DELETE = apiHandler(async (request: NextRequest) => { meta: { metric: metric ?? "all" }, }); + // The PUT's limit, on the path that can destroy more in one call than the + // PUT can. It had none at all: an omitted query parameter erased the whole + // override set, and nothing stopped that being repeated. + const rl = await checkRateLimit( + `thresholds:reset:${user.id}`, + 30, + 5 * 60 * 1000, + ); + if (!rl.allowed) { + return apiError("Too many requests, please slow down", 429); + } + + if (!metric) { + const raw = await request.text(); + if (raw.length > 64 * 1024) { + return apiError(`Request body exceeds ${64 * 1024} bytes`, 413); + } + let confirm = ""; + if (raw.length > 0) { + try { + const body = JSON.parse(raw) as { confirm?: unknown }; + confirm = typeof body?.confirm === "string" ? body.confirm : ""; + } catch { + return apiError("Invalid request", 422); + } + } + if (confirm !== RESET_ALL_CONFIRMATION) { + // Names the token rather than saying "confirmation missing", so a client + // that reached here by omitting `?metric=` by accident is told what the + // request it sent actually meant. + return apiError( + `Confirmation missing. Resetting every threshold override requires { confirm: '${RESET_ALL_CONFIRMATION}' }`, + 422, + ); + } + } + const existing = await prisma.user.findUnique({ where: { id: user.id }, select: { thresholdsJson: true }, diff --git a/src/components/admin/__tests__/backups-restore-refusal-notice.test.tsx b/src/components/admin/__tests__/backups-restore-refusal-notice.test.tsx new file mode 100644 index 000000000..d818af957 --- /dev/null +++ b/src/components/admin/__tests__/backups-restore-refusal-notice.test.tsx @@ -0,0 +1,111 @@ +/** + * A refused backup has to tell the operator what to do next. + * + * The server refuses a file whose manifest claims a section the file does not + * carry, before it deletes anything. That refusal reaches a person through the + * `meta.errorCode` on the error envelope, and a bare "restore failed" toast + * would waste it: the operator on the other end is frequently holding the only + * copy of an account, and the two facts they can act on — which section is + * missing, and that nothing was changed — have to stay on screen. + * + * Pinned here: + * 1. the missing sections are named, not counted, + * 2. the notice says nothing was changed and what to do instead, + * 3. no sections means no notice — a panel that appears after every restore + * trains the operator to ignore it, + * 4. the copy resolves in more than one locale, + * 5. `missingSectionsOf` keys on the errorCode and not on the message text, + * so rewording the server's prose cannot silently remove the panel. + * + * Mutation check: render a count instead of the list and case 1 goes red; drop + * the remedy line and case 2 goes red; return the notice unconditionally and + * case 3 goes red; match on the message string in `missingSectionsOf` and case + * 5 goes red. + */ +import { describe, expect, it, vi } from "vitest"; +import { renderToStaticMarkup } from "react-dom/server"; + +import { I18nProvider } from "@/lib/i18n/context"; +import { + RestoreRefusalNotice, + missingSectionsOf, +} from "@/components/admin/backups-section"; +import { ApiError } from "@/lib/api/api-fetch"; +import type { MissingBackupSection } from "@/lib/export/restore-skips"; + +vi.mock("next/navigation", () => ({ + useRouter: () => ({ push: vi.fn(), replace: vi.fn() }), + usePathname: () => "/admin/backups", +})); + +function render( + sections: MissingBackupSection[], + locale: "en" | "de" = "en", +): string { + return renderToStaticMarkup( + + {}} /> + , + ); +} + +describe("RestoreRefusalNotice", () => { + it("names every missing section", () => { + const html = render(["documents", "consent"]); + + expect(html).toContain('data-slot="restore-refusal-notice"'); + expect(html).toContain("Documents"); + expect(html).toContain("Consent receipts"); + }); + + it("says nothing was changed, and what to do instead", () => { + const html = render(["documents"]); + + expect(html).toContain("Nothing was changed"); + expect(html).toContain("disaster-recovery snapshot"); + }); + + it("renders nothing when no section was missing", () => { + expect(render([])).toBe(""); + }); + + it("speaks the operator's language", () => { + const german = render(["documents", "mentalHealth"], "de"); + + expect(german).toContain("Dokumente"); + expect(german).toContain("Psychische Fragebögen"); + expect(german).toContain("Es wurde nichts geändert"); + }); +}); + +describe("missingSectionsOf", () => { + it("reads the sections off the refusal envelope", () => { + const err = new ApiError("Backup is missing documents", 422, { + errorCode: "backup.section.missing", + sections: ["documents"], + }); + + expect(missingSectionsOf(err)).toEqual(["documents"]); + }); + + it("ignores an error whose message merely looks like one", () => { + // The message is prose the server may reword. Only the code counts. + const err = new ApiError("Backup is missing documents", 422, { + errorCode: "something_else", + sections: ["documents"], + }); + + expect(missingSectionsOf(err)).toEqual([]); + expect(missingSectionsOf(new Error("Backup is missing documents"))).toEqual( + [], + ); + }); + + it("survives a refusal with no section list", () => { + const err = new ApiError("Refused", 422, { + errorCode: "backup.section.missing", + }); + + expect(missingSectionsOf(err)).toEqual([]); + }); +}); diff --git a/src/components/admin/backups-section.tsx b/src/components/admin/backups-section.tsx index 598d6aab9..5e2243a91 100644 --- a/src/components/admin/backups-section.tsx +++ b/src/components/admin/backups-section.tsx @@ -49,11 +49,18 @@ import { useFormatters, useTranslations } from "@/lib/i18n/context"; import { queryKeys } from "@/lib/query-keys"; import type { BackupRow, BackupsList } from "@/types/backups"; import type { + MissingBackupSection, RestoreSkipSummary, SkippedCatalogue, } from "@/lib/export/restore-skips"; import { getApiErrorMessage } from "./_shared"; -import { apiFetch, apiFetchRaw, apiGet, apiPost } from "@/lib/api/api-fetch"; +import { + ApiError, + apiFetch, + apiFetchRaw, + apiGet, + apiPost, +} from "@/lib/api/api-fetch"; /** * Typed-confirmation dialog for restore. The destructive Restore button @@ -374,6 +381,114 @@ function catalogueLabel( return unlabelled; } +/** + * Which section a refused file claimed and did not carry, as a label. + * + * Spelled out with literal `t()` calls for the same reason as + * `catalogueLabel` above — an interpolated key is invisible to + * `i18n-call-site-coverage.test.ts`, and a renamed section would ship as raw + * dot notation. The `never` at the foot is what makes the compiler say the + * chain is exhaustive. + */ +function missingSectionLabel( + section: MissingBackupSection, + t: ReturnType["t"], +) { + if (section === "documents") { + return t("admin.section.backups.restoreMissingDocuments"); + } + if (section === "workouts") { + return t("admin.section.backups.restoreMissingWorkouts"); + } + if (section === "mentalHealth") { + return t("admin.section.backups.restoreMissingMentalHealth"); + } + if (section === "consent") { + return t("admin.section.backups.restoreMissingConsent"); + } + const unlabelled: never = section; + return unlabelled; +} + +/** + * Why a file was refused, and what to do about it. + * + * Held on screen rather than left to the toast, exactly like the skip report + * below it. The operator refused here is often holding the only copy of an + * account; "restore failed" for four seconds is not enough to act on, and the + * two facts that let them act — which section the file claimed and did not + * carry, and that nothing was changed — have to survive long enough to be read. + */ +/** + * The refused sections carried on an `ApiError`, or none. + * + * Keyed on `meta.errorCode` and not on the message text: the message is prose + * the server may reword, and matching on it would turn a copy edit into a + * silent loss of the panel. + */ +export function missingSectionsOf(err: unknown): MissingBackupSection[] { + if (!(err instanceof ApiError)) return []; + if (err.meta?.errorCode !== "backup.section.missing") return []; + const sections = err.meta?.sections; + if (!Array.isArray(sections)) return []; + return sections.filter( + (section): section is MissingBackupSection => typeof section === "string", + ); +} + +export function RestoreRefusalNotice({ + sections, + onDismiss, +}: { + sections: MissingBackupSection[]; + onDismiss: () => void; +}) { + const { t } = useTranslations(); + if (sections.length === 0) return null; + return ( +
+
+
+
+ +
+
+ ); +} + /** * What the last restore could not put back. * @@ -585,6 +700,13 @@ export function BackupsSection() { // until they dismiss it. const [skipped, setSkipped] = useState(null); + // Why the last restore was refused before it touched anything. Same reason + // the skip report is held rather than toasted: the operator has to be able to + // read it after the four seconds are up. + const [refusedSections, setRefusedSections] = useState< + MissingBackupSection[] + >([]); + // Restore: typed-confirmation dialog. The mutation is keyed by row id // and used inline by `` below — keeping the // mutation here lets the parent invalidate the list query on success. @@ -604,6 +726,7 @@ export function BackupsSection() { // A restore that dropped links is not a plain success and must not read // as one. The count and the exact keys go on screen; the same report is // in the audit row for anyone asking later. + setRefusedSections([]); const report = data.skipped; if (report && report.links > 0) { setSkipped(report); @@ -622,6 +745,16 @@ export function BackupsSection() { queryClient.invalidateQueries(); }, onError: (err) => { + // The server refuses an incomplete file before it deletes anything, and + // names the sections in `meta`. That is the one restore failure the + // operator can act on, so it gets the panel rather than only a toast. + const sections = missingSectionsOf(err); + if (sections.length > 0) { + setRefusedSections(sections); + toast.error(t("admin.section.backups.restoreMissingTitle")); + return; + } + setRefusedSections([]); toast.error( err instanceof Error && err.message ? err.message @@ -745,6 +878,11 @@ export function BackupsSection() { + setRefusedSections([])} + /> + {skipped ? ( { + 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/components/settings/thresholds-editor-section.tsx b/src/components/settings/thresholds-editor-section.tsx index 29a9689f1..0d638024b 100644 --- a/src/components/settings/thresholds-editor-section.tsx +++ b/src/components/settings/thresholds-editor-section.tsx @@ -108,10 +108,19 @@ export function ThresholdsEditorSection({ id }: { id: string }) { const resetMutation = useMutation({ mutationFn: async (metric: ThresholdMetric | null) => { - const url = metric - ? `/api/user/thresholds?metric=${metric}` - : "/api/user/thresholds"; - const res = await apiFetchRaw(url, { method: "DELETE" }); + // `metric === null` is the reset-everything arm behind the confirm + // dialog at the foot of the card. The server asks for the same decision + // in the body, so the dialog's answer is what gets sent — the wide form + // is not reachable by dropping the query parameter. + const res = metric + ? await apiFetchRaw(`/api/user/thresholds?metric=${metric}`, { + method: "DELETE", + }) + : await apiFetchRaw("/api/user/thresholds", { + method: "DELETE", + headers: { "Content-Type": "application/json" }, + body: JSON.stringify({ confirm: "RESET_THRESHOLDS" }), + }); if (!res.ok) throw new Error("reset failed"); }, onSuccess: () => { 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 d319a36de..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,18 +1108,12 @@ 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 (!sessionData.user.totpConfirmedAt) { - const webauthnKeyCount = await prisma.webauthnMfaCredential.count({ - where: { userId: sessionData.user.id }, - }); - if (webauthnKeyCount === 0) { - throw new StepUpRequiredError("auth.stepup.mfa_not_enrolled"); - } + // 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"); } // Read the freshness stamp off the live session row — `getSession`'s @@ -1125,22 +1152,65 @@ export async function requireFreshMfaIfEnrolled( maxAgeSeconds: number, ): Promise { const auth = await requireAuth(); - // Either factor enrols the account: a confirmed TOTP secret OR a registered - // WebAuthn security key. A webauthn-only user must clear step-up too, so the - // destructive-action boundary tracks `requireFreshMfa`'s either-factor rule. - let enrolled = Boolean(auth.user.totpConfirmedAt); - if (!enrolled) { - const webauthnKeyCount = await prisma.webauthnMfaCredential.count({ - where: { userId: auth.user.id }, - }); - enrolled = webauthnKeyCount > 0; - } - if (enrolled) { + if (await hasSecondFactorEnrolled(auth.user)) { await requireFreshMfa(maxAgeSeconds); } return auth; } +/** + * 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 + * boundary that asks this question tracks `requireFreshMfa`'s either-factor + * 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; + totpConfirmedAt: Date | null; +}): Promise { + if (user.totpConfirmedAt) return true; + const webauthnKeyCount = await prisma.webauthnMfaCredential.count({ + where: { userId: user.id }, + }); + 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. * @@ -1268,19 +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). + * 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 } = {}, + options: { + freshFactor?: boolean; + proofSource?: FreshFactorProofSource; + } = {}, ): Promise { 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 resolved = freshFactor - ? await requireFreshMfa(MFA_STEP_UP_MAX_AGE_SECONDS) + ? await requireFreshMfa(MFA_STEP_UP_MAX_AGE_SECONDS, proofSource) : await requireCookieAuth(); return { transport: "cookie", @@ -1338,20 +1417,13 @@ 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. - if (freshFactor) { - let enrolled = Boolean(auth.user.totpConfirmedAt); - if (!enrolled) { - const keys = await prisma.webauthnMfaCredential.count({ - where: { userId: auth.user.id }, - }); - enrolled = keys > 0; - } - if (!enrolled) { - throw new StepUpRequiredError("auth.stepup.mfa_not_enrolled"); - } + // 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"); } return { diff --git a/src/lib/export/__tests__/missing-backup-sections.test.ts b/src/lib/export/__tests__/missing-backup-sections.test.ts new file mode 100644 index 000000000..8ac6fc0d0 --- /dev/null +++ b/src/lib/export/__tests__/missing-backup-sections.test.ts @@ -0,0 +1,125 @@ +/** + * A missing section and a declared omission are not the same thing. + * + * `findMissingBackupSections` is the rule that separates them, and the whole + * value of the refusal it feeds rests on it getting the separation right in + * both directions: + * + * - too lax and a portable file whose documents section is gone still runs, + * which is the defect (#237) — the restore deletes the vault it is about to + * rebuild and rebuilds nothing; + * - too strict and it refuses the two files that legitimately leave a section + * out. The portable export omits the mental-health screeners and the consent + * receipts ON PURPOSE, declares both in the manifest as `included: + * "omitted"`, and has to stay restorable. So does every backup written + * before the manifest existed, which declares nothing at all. + * + * The manifest is the only discriminator, and the cases below say so by proving + * the two things it is NOT: not the emptiness of the array (an account with no + * documents honestly writes `[]`), and not a guess from the export's purpose, + * which the payload never states. + * + * Mutation check: treat `[]` as missing and the empty-section case goes red; + * drop the `included === "omitted"` skip and the declared-omission case goes + * red; return early on a present-but-manifest-less file and the legacy case + * stays green while the first case goes red. + */ +import { describe, expect, it } from "vitest"; + +import { findMissingBackupSections } from "../restore-skips"; + +/** A portable file's manifest, as `buildFullBackupPayload` writes it. */ +const PORTABLE_MANIFEST = { + documents: { included: "metadata-only", note: "…" }, + workouts: { included: "summary-only", note: "…" }, + mentalHealth: { included: "omitted", note: "…" }, + consent: { included: "omitted", note: "…" }, +}; + +/** The disaster-recovery writer's manifest — every section carried. */ +const DR_MANIFEST = { + documents: { included: "encrypted-content", note: "…" }, + workouts: { included: "summary-only", note: "…" }, + mentalHealth: { included: "full", note: "…" }, + consent: { included: "full", note: "…" }, +}; + +function portable(overrides: Record = {}) { + return { + schemaVersion: "2", + exportedAt: "2026-08-01T00:00:00.000Z", + userId: "user-1", + manifest: PORTABLE_MANIFEST, + documents: [], + workouts: [], + mentalHealthAssessments: [], + consentReceipts: [], + ...overrides, + }; +} + +describe("findMissingBackupSections", () => { + it("names the documents section a portable file claims and does not carry", () => { + const file = portable(); + delete (file as Record).documents; + + expect(findMissingBackupSections(file)).toEqual(["documents"]); + }); + + it("finds nothing in a complete portable file", () => { + expect(findMissingBackupSections(portable())).toEqual([]); + }); + + it("does not read an empty section as a missing one", () => { + // An account with no documents writes `[]`, and that file is fine. Refusing + // it would make the check worse than the silence it replaces. + expect(findMissingBackupSections(portable({ documents: [] }))).toEqual([]); + }); + + it("leaves a DECLARED omission alone", () => { + // The portable export omits both of these on purpose and says so. This is + // the boundary: the keys are gone, and the file is still restorable. + const file = portable(); + delete (file as Record).mentalHealthAssessments; + delete (file as Record).consentReceipts; + + expect(findMissingBackupSections(file)).toEqual([]); + }); + + it("names the same two sections when the manifest says they ARE carried", () => { + // Identical payload to the case above, one word different in the manifest. + // That word is the whole rule. + const file = portable({ manifest: DR_MANIFEST }); + delete (file as Record).mentalHealthAssessments; + delete (file as Record).consentReceipts; + + expect(findMissingBackupSections(file)).toEqual([ + "mentalHealth", + "consent", + ]); + }); + + it("claims nothing about a file with no manifest", () => { + // Every backup written before the manifest existed. It declares nothing, so + // nothing is missing from it, and it stays restorable. + const legacy = portable({ manifest: null }); + delete (legacy as Record).documents; + delete (legacy as Record).workouts; + + expect(findMissingBackupSections(legacy)).toEqual([]); + }); + + it("reports every missing section rather than the first", () => { + const file = portable({ manifest: DR_MANIFEST }); + delete (file as Record).documents; + delete (file as Record).workouts; + + expect(findMissingBackupSections(file)).toEqual(["documents", "workouts"]); + }); + + it("survives a payload that is not an object at all", () => { + expect(findMissingBackupSections(null)).toEqual([]); + expect(findMissingBackupSections("nope")).toEqual([]); + expect(findMissingBackupSections([])).toEqual([]); + }); +}); diff --git a/src/lib/export/restore-skips.ts b/src/lib/export/restore-skips.ts index ef8acb6c3..11ffe61d7 100644 --- a/src/lib/export/restore-skips.ts +++ b/src/lib/export/restore-skips.ts @@ -216,3 +216,91 @@ export function summarizeRestoreSkips(log: RestoreSkipLog): RestoreSkipSummary { links: catalogueKeys.reduce((total, entry) => total + entry.links, 0), }; } + +/** + * A section the file's own manifest says it carries, and does not. + * + * This is the other half of the honesty contract above, and it is deliberately + * the opposite answer. A skip drops one edge and names it, because the record + * it hangs off survives and giving the operator the rest is worth more than + * refusing them everything. A missing SECTION is not one edge — it is a whole + * class of the account, and the restore's first act is to delete the class it + * is about to rebuild. Restoring a file whose documents section is not there + * therefore does not lose a link. It empties the vault, tells the operator the + * restore succeeded, and leaves them to find out later. + * + * So this one refuses the file. Nothing is deleted, and the response names the + * section rather than saying "invalid backup", because an operator holding the + * only copy of an account needs to know whether the file is worthless or + * whether they picked the wrong one of two. + * + * ── The manifest is what tells a missing section from an omitted one ──────── + * + * A payload that omits a section on purpose is not broken, and refusing it + * would break the two exports that do it. `buildSensitiveBackupSection` leaves + * the screener administrations and the consent receipts out of a PORTABLE file + * — the first because the encrypted per-item answers include the PHQ-9 + * self-harm item, the second because a consent belongs to the operator it was + * given to — and says so in the manifest as `included: "omitted"`. That is a + * declared omission, and it stays restorable. + * + * The discriminator is therefore the manifest and nothing else. Not the emptiness + * of the array: an account with no documents honestly writes `[]`, and treating + * that as a loss would refuse a perfectly good file. Not the export's purpose + * either, which the payload does not state. Only "the file said it carries this, + * and the key is not there". + * + * A file with NO manifest at all declares nothing, so nothing is missing from + * it. Every backup written before the manifest existed is in that category and + * has to stay restorable. + */ +export type MissingBackupSection = + "documents" | "workouts" | "mentalHealth" | "consent"; + +/** + * Which payload key each manifest entry speaks for. + * + * `included: "omitted"` is the one value that means "not carried". Every other + * value the writers produce — `metadata-only`, `encrypted-content`, + * `summary-only`, `full` — describes HOW MUCH of the section travels, not + * whether the section is there, and all of them come with the key present. + */ +const MANIFEST_SECTIONS: ReadonlyArray<{ + manifestKey: MissingBackupSection; + payloadKey: string; +}> = [ + { manifestKey: "documents", payloadKey: "documents" }, + { manifestKey: "workouts", payloadKey: "workouts" }, + { manifestKey: "mentalHealth", payloadKey: "mentalHealthAssessments" }, + { manifestKey: "consent", payloadKey: "consentReceipts" }, +]; + +function isRecord(value: unknown): value is Record { + return typeof value === "object" && value !== null && !Array.isArray(value); +} + +/** + * Sections the raw file claims and does not carry, in manifest order. + * + * Takes the RAW parsed JSON, not a `BackupPayload`. Every section key in + * `backupPayloadSchema` carries `.default([])`, so by the time the payload has + * been through the schema an absent section and an empty one are the same + * value — which is exactly the distinction this function exists to make. Run it + * on the object that came out of `JSON.parse`. + */ +export function findMissingBackupSections( + raw: unknown, +): MissingBackupSection[] { + if (!isRecord(raw)) return []; + const manifest = raw.manifest; + if (!isRecord(manifest)) return []; + + const missing: MissingBackupSection[] = []; + for (const { manifestKey, payloadKey } of MANIFEST_SECTIONS) { + const entry = manifest[manifestKey]; + if (!isRecord(entry)) continue; + if (entry.included === "omitted") continue; + if (raw[payloadKey] === undefined) missing.push(manifestKey); + } + return missing; +} diff --git a/src/lib/openapi/routes/auth.ts b/src/lib/openapi/routes/auth.ts index 3d2694ab7..0c2715658 100644 --- a/src/lib/openapi/routes/auth.ts +++ b/src/lib/openapi/routes/auth.ts @@ -1529,7 +1529,9 @@ 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" + - "No step-up: this endpoint takes a plain cookie session or a wildcard Bearer token, where removing a second-factor security key (DELETE /api/auth/me/mfa/webauthn/{id}) requires a fresh factor proof. The asymmetry is in the code as it stands, not a description of intent.", + "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: { "200": { @@ -1553,6 +1555,13 @@ export const authPaths: NonNullable = { content: { "application/json": { schema: errorEnvelope } }, }, ...stdResponses, + // After `stdResponses`, or the generic 401 there overwrites this one + // and the step-up contract disappears from the published operation. + "401": { + 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: errorEnvelope } }, + }, }, }, }, diff --git a/src/lib/openapi/routes/profile.ts b/src/lib/openapi/routes/profile.ts index f3e50f971..f2d5b8eba 100644 --- a/src/lib/openapi/routes/profile.ts +++ b/src/lib/openapi/routes/profile.ts @@ -653,6 +653,27 @@ const thresholdsOverridesResponse = z .object({ overrides: thresholdOverrideMap }) .meta({ id: "ThresholdsOverridesResponse" }); +/** + * The typed confirmation the parameterless DELETE demands. + * + * Published because it is the difference between "reset one band" and "erase + * every band the account has tuned", and a client that learned the endpoint + * from the contract alone would otherwise discover the requirement as a 422. + */ +const thresholdsResetAllRequest = z + .object({ + confirm: z + .literal("RESET_THRESHOLDS") + .describe( + "Required when `metric` is omitted. Compared exactly; any other value is refused.", + ), + }) + .meta({ + id: "ThresholdsResetAllRequest", + description: + "Confirmation body for the reset-everything form. Send no body at all when resetting a single metric with `?metric=`.", + }); + const thresholdsUpdateRequest = thresholdsUpdateSchema.meta({ id: "ThresholdsUpdateRequest", description: @@ -1716,17 +1737,23 @@ export const profilePaths: NonNullable = { tags: ["Analytics"], summary: "Reset one threshold override, or all of them", description: - "With `?metric=` it drops that metric's override; WITHOUT the parameter it drops EVERY override on the account. There is no confirmation step and no dry run, so a client that means to reset one metric must send the parameter.\n\n" + - "Audit-logged with the before and after maps. Unlike the PUT this path runs no rate limit of its own.", + 'With `?metric=` it drops that metric\'s override; WITHOUT the parameter it drops EVERY override on the account. The wide form is not reachable by omitting the parameter alone — it requires `{ "confirm": "RESET_THRESHOLDS" }` in the body, the same typed-confirmation shape account deletion and the data reset use. The `?metric=` form takes no body and is unchanged.\n\n' + + "Audit-logged with the before and after maps. Rate-limited 30 / 5 min per user, matching the PUT.", requestParams: { query: z.object({ metric: thresholdMetricEnum .optional() .describe( - "The single metric to reset. Omit to reset every override on the account.", + "The single metric to reset. Omit to reset every override on the account — which then requires the confirmation body.", ), }), }, + requestBody: { + required: false, + content: { + "application/json": { schema: thresholdsResetAllRequest }, + }, + }, responses: { "200": { description: "The remaining override map.", @@ -1745,6 +1772,14 @@ export const profilePaths: NonNullable = { content: { "application/json": { schema: errorEnvelope } }, }, ...stdResponses, + // After `stdResponses` on purpose: the generic 422 there would + // otherwise overwrite this one and the confirmation contract would + // vanish from the published operation. + "422": { + description: + '`metric` was omitted and the body did not carry `{ "confirm": "RESET_THRESHOLDS" }`. Nothing was reset.', + content: { "application/json": { schema: errorEnvelope } }, + }, }, }, }, diff --git a/tests/integration/backup-missing-section-restore.test.ts b/tests/integration/backup-missing-section-restore.test.ts new file mode 100644 index 000000000..89702af11 --- /dev/null +++ b/tests/integration/backup-missing-section-restore.test.ts @@ -0,0 +1,238 @@ +/** + * A file that claims a section it does not carry is refused whole (#237). + * + * The restore's first act is to delete the class of records it is about to + * rebuild. So a payload whose documents section is simply absent does not + * "restore everything else": it empties the vault, reports success, and leaves + * the operator to discover the hole later. The maintainer's decision is to + * refuse the file instead — above the transaction, like the metadata-only + * check beside it, so the account survives the refusal. + * + * The hard part is not the refusal. It is the boundary, and both halves are + * asserted here against the real route and real rows: + * + * MISSING — the manifest says the file carries documents, and the key is not + * there. Refused, named, nothing deleted. + * + * DECLARED OMITTED — the manifest says the file does NOT carry the + * mental-health screeners or the consent receipts, and the keys are not + * there. That is what every portable export writes, on purpose, and it has to + * restore. The two cases below are the same builder, the same account and the + * same deleted-key trick; the only difference is what the manifest said, which + * is exactly the claim being made. + * + * Mutation check: drop the `included === "omitted"` skip in + * `findMissingBackupSections` and the second case goes red. Move the refusal + * below the `prisma.$transaction(...)` call and the first case's surviving-rows + * assertions go red while its status assertion stays green — which is why the + * rows are asserted separately rather than trusted to follow from the status. + */ +import { beforeEach, describe, expect, it, vi } from "vitest"; + +process.env.ENCRYPTION_KEY = + "0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef"; + +import { encrypt } from "@/lib/crypto"; +import { buildFullBackupPayload } from "@/lib/export/full-backup-payload"; +import { POST } from "@/app/api/admin/backups/[id]/restore/route"; +import { invalidateUserData } from "@/lib/cache/invalidate"; + +import { cookieJar, headerJar } from "./mock-next-headers"; +import { getPrismaClient, truncateAllTables } from "./setup"; + +vi.mock("next/headers", async () => { + const { cookieJar, headerJar } = await import("./mock-next-headers"); + return { + headers: vi.fn(async () => ({ + get: (name: string) => headerJar.get(name.toLowerCase()) ?? null, + })), + cookies: vi.fn(async () => ({ + get: (name: string) => { + const value = cookieJar.get(name); + return value ? { name, value } : undefined; + }, + set: (name: string, value: string) => cookieJar.set(name, value), + delete: (name: string) => cookieJar.delete(name), + })), + }; +}); + +vi.mock("@/lib/db-compat", () => ({ + ensureDbCompatibility: vi.fn().mockResolvedValue(undefined), +})); + +vi.mock("@/lib/cache/invalidate", () => ({ + invalidateUserData: vi.fn(), +})); + +beforeEach(async () => { + await truncateAllTables(getPrismaClient()); + cookieJar.clear(); + headerJar.clear(); + vi.mocked(invalidateUserData).mockClear(); +}); + +function makeRequest(id: string) { + return new Request(`http://localhost/api/admin/backups/${id}/restore`, { + method: "POST", + headers: { "content-type": "application/json" }, + body: JSON.stringify({ confirm: "RESTORE" }), + }); +} + +/** + * An admin account with one measurement and nothing else, signed in. + * + * The measurement is the witness. It references nothing the refusal is about, + * so it can only be lost to the wipe — which is what tells a refused restore + * apart from a destroyed account. + */ +async function seedOwner(username: string) { + const prisma = getPrismaClient(); + const owner = await prisma.user.create({ + data: { username, email: `${username}@example.test`, role: "ADMIN" }, + }); + const session = await prisma.session.create({ + data: { userId: owner.id, expiresAt: new Date(Date.now() + 60_000) }, + }); + cookieJar.set("healthlog_session", session.id); + await prisma.measurement.create({ + data: { + userId: owner.id, + type: "WEIGHT", + value: 79.2, + unit: "kg", + source: "MANUAL", + measuredAt: new Date("2026-07-11T06:15:00.000Z"), + }, + }); + return owner; +} + +/** + * A real portable export of that account, with the named keys cut out of the + * JSON before it is sealed. + * + * Cutting them from the RAW object rather than from a parsed payload is the + * whole point: every section in `backupPayloadSchema` carries `.default([])`, + * so a payload that has been through the schema no longer knows the difference + * between a section that was absent and one that was empty. + */ +async function sealPortableWithout( + ownerId: string, + keys: string[], +): Promise<{ backupId: string; manifest: Record }> { + const prisma = getPrismaClient(); + // No `purpose`, so the builder takes the portable branch — the file a person + // gets from "export my data". + const { payload } = await buildFullBackupPayload(prisma, ownerId); + const raw = JSON.parse(JSON.stringify(payload)) as Record; + for (const key of keys) delete raw[key]; + + const backup = await prisma.dataBackup.create({ + data: { + userId: ownerId, + type: "MISSING_SECTION_RESTORE", + data: encrypt(JSON.stringify(raw)), + }, + }); + return { + backupId: backup.id, + manifest: raw.manifest as Record, + }; +} + +async function restore(backupId: string) { + return await POST( + makeRequest(backupId) as unknown as Parameters[0], + { params: Promise.resolve({ id: backupId }) }, + ); +} + +describe("a backup whose manifest claims a section the file does not carry", () => { + it("refuses the whole file, names the section, and changes nothing", async () => { + const prisma = getPrismaClient(); + const owner = await seedOwner("missing-section-owner"); + const { backupId, manifest } = await sealPortableWithout(owner.id, [ + "documents", + ]); + + // Prove the premise rather than assume it: the file still SAYS it carries + // documents. Without this the test would keep passing if the builder ever + // stopped writing the manifest entry. + expect( + (manifest.documents as { included?: string } | undefined)?.included, + ).toBe("metadata-only"); + + const response = await restore(backupId); + + expect(response.status).toBe(422); + const body = (await response.json()) as { + error: string; + meta?: { errorCode?: string; sections?: string[] }; + }; + expect( + body.error, + "the message names the section, so the operator can act on it", + ).toContain("documents"); + expect( + body.error, + "and says the account is intact, because that is the decision they need", + ).toContain("Nothing was changed"); + expect(body.meta?.errorCode).toBe("backup.section.missing"); + expect(body.meta?.sections).toEqual(["documents"]); + + // The refusal is only worth anything above the wipe. + expect( + await prisma.measurement.count({ where: { userId: owner.id } }), + "a refused restore must not have run the wipe", + ).toBe(1); + // A second, independent witness: the invalidation only fires at the end of + // a restore that actually happened. + expect(vi.mocked(invalidateUserData)).not.toHaveBeenCalled(); + + const auditRow = await prisma.auditLog.findFirstOrThrow({ + where: { action: "admin.backups.restore.failed" }, + orderBy: { createdAt: "desc" }, + }); + const details = JSON.parse(auditRow.details ?? "{}") as { + reason?: string; + sections?: string[]; + }; + expect(details.reason).toBe("section_missing"); + expect(details.sections).toEqual(["documents"]); + }); + + it("restores a file whose manifest DECLARES the same keys omitted", async () => { + const prisma = getPrismaClient(); + const owner = await seedOwner("declared-omission-owner"); + // The two sections a portable export leaves out on purpose, cut from the + // file exactly the way `documents` was cut above. + const { backupId, manifest } = await sealPortableWithout(owner.id, [ + "mentalHealthAssessments", + "consentReceipts", + ]); + + expect( + (manifest.mentalHealth as { included?: string } | undefined)?.included, + ).toBe("omitted"); + expect( + (manifest.consent as { included?: string } | undefined)?.included, + ).toBe("omitted"); + + const response = await restore(backupId); + + expect( + response.status, + "a declared omission is not a missing section", + ).toBe(200); + const body = (await response.json()) as { data: { restored: boolean } }; + expect(body.data.restored).toBe(true); + // The restore genuinely ran: the measurement came back out of the file it + // was exported into, and the cache was evicted. + expect( + await prisma.measurement.count({ where: { userId: owner.id } }), + ).toBe(1); + expect(vi.mocked(invalidateUserData)).toHaveBeenCalledWith(owner.id); + }); +}); diff --git a/tests/integration/step-up-elevation.test.ts b/tests/integration/step-up-elevation.test.ts index e47f69546..e6a82cbd4 100644 --- a/tests/integration/step-up-elevation.test.ts +++ b/tests/integration/step-up-elevation.test.ts @@ -1156,3 +1156,177 @@ describe("E13 — the second-factor status read needs no elevation", () => { expect(text).not.toContain("credentialId"); }); }); + +// ── 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: true, proofSource: + * "any-possession" })`. + * + * 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: + * + * `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. + * + * 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. + * + * 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 needs a fresh possession proof", () => { + async function seedPasskey(label: string): Promise { + const row = await getPrismaClient().passkey.create({ + data: { + userId: USER_ID, + name: label, + credentialId: `cred-${label}`, + credentialPublicKey: Buffer.from([1, 2, 3]), + credentialDeviceType: "multiDevice", + transports: ["internal"], + }, + select: { id: true }, + }); + return row.id; + } + + async function callDelete(id: string): Promise { + const { DELETE } = await import("@/app/api/auth/passkeys/[id]/route"); + return await DELETE(req(`/api/auth/passkeys/${id}`, "DELETE"), { + params: Promise.resolve({ id }), + }); + } + + async function useCookieSession(): Promise { + const session = await getPrismaClient().session.create({ + data: { userId: USER_ID, expiresAt: new Date(Date.now() + 3_600_000) }, + }); + cookieJar.set("healthlog_session", session.id); + return session.id; + } + + 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"); + // No TOTP, no security key. Only the passkey it is trying to remove. + await useCookieSession(); + + const res = await callDelete(passkeyId); + + 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("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(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); + }); + + it("admits the native client on a factor-proved elevation, and spends it", async () => { + const passkeyId = await seedPasskey("e14e"); + const secret = await enrolTotp(); + const { raw, id: tokenId } = await mintToken("e14e"); + useToken(raw); + const elevation = elevationOf(await mintTotpElevation(secret)); + useElevation(elevation); + + expect((await callDelete(passkeyId)).status).toBe(200); + expect(await getPrismaClient().passkey.count()).toBe(0); + + const row = await getPrismaClient().stepUpElevation.findFirst({ + where: { apiTokenId: tokenId }, + }); + expect(row?.consumedAt).not.toBeNull(); + }); + + it("does not burn the elevation on a foreign passkey id", async () => { + await seedPasskey("e14f"); + const secret = await enrolTotp(); + const { raw, id: tokenId } = await mintToken("e14f"); + useToken(raw); + const elevation = elevationOf(await mintTotpElevation(secret)); + useElevation(elevation); + + expect((await callDelete("pk-does-not-exist")).status).toBe(404); + + const row = await getPrismaClient().stepUpElevation.findFirst({ + where: { apiTokenId: tokenId }, + }); + 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"); + }); +});