From 0692017f345a90b56e83d573b16b4c86e22d4b2c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Bombeck?= Date: Sat, 26 Sep 2026 22:15:10 +0200 Subject: [PATCH 1/9] fix(auth): adding a credential beside a second factor needs that factor On an account with a second factor, TOTP setup, security-key registration and passkey registration no longer accept the password, a password re-proof or a session younger than five minutes. A remembered browser skips the second factor at sign-in and still creates a fresh session, so the password alone could add a key that then satisfies every gate the factor protected. Only a TOTP code, a security key, a passkey or a completed second factor within five minutes opens enrollment there; the Bearer arm of TOTP setup and security-key registration asks the same of its elevation. Accounts without a second factor keep the previous rules. The refusal names the proofs the account can give, and the enrollment dialogs offer exactly those. A proof that verifies now gives its attempt back to the shared re-proof budget, including the current password on a password change. --- src/app/api/auth/me/mfa/totp/setup/route.ts | 8 +- .../me/mfa/webauthn/register/options/route.ts | 7 +- .../auth/passkey/register-options/route.ts | 26 ++ .../api/auth/password/__tests__/route.test.ts | 6 +- src/app/api/auth/password/route.ts | 5 + src/app/api/auth/reproof/route.ts | 3 + .../existing-factor-reauth-dialog.tsx | 22 ++ .../security-section/security-keys-card.tsx | 12 +- .../settings/security-section/totp-card.tsx | 11 +- .../__tests__/require-recent-proof.test.ts | 47 +++- src/lib/api-handler.ts | 84 ++----- .../enrollment-proof-second-factor.test.ts | 228 ++++++++++++++++++ src/lib/auth/existing-factor-proof.ts | 82 ++++++- src/lib/auth/second-factor.ts | 66 +++++ 14 files changed, 519 insertions(+), 88 deletions(-) create mode 100644 src/lib/auth/__tests__/enrollment-proof-second-factor.test.ts create mode 100644 src/lib/auth/second-factor.ts diff --git a/src/app/api/auth/me/mfa/totp/setup/route.ts b/src/app/api/auth/me/mfa/totp/setup/route.ts index e1ac624ea..9c94d919e 100644 --- a/src/app/api/auth/me/mfa/totp/setup/route.ts +++ b/src/app/api/auth/me/mfa/totp/setup/route.ts @@ -16,8 +16,10 @@ * factor that later satisfies every step-up gate, so a stolen session must not * be able to mint one: the cookie arm also passes `checkCookieEnrollmentProof` * — a sign-in or second factor inside the last five minutes, or a password / - * factor proof in the body (the step-up mint's shapes). The Bearer arm already - * carries that proof in its elevation. The recovery-code batch is + * factor proof in the body (the step-up mint's shapes); on an account that + * already has a second factor, only that factor or a passkey. The Bearer arm + * carries the proof in its elevation, under the same rule: on an account with + * a second factor the elevation must come from one. The recovery-code batch is * issued at `/confirm` (after the factor is proven), not here, so an abandoned * setup never persists codes. */ @@ -41,7 +43,7 @@ const SETUP_RATE_LIMIT = 5; const SETUP_WINDOW_MS = 15 * 60 * 1000; export const POST = apiHandler(async (req: Request) => { - const auth = await requireMfaManagementAuth(); + const auth = await requireMfaManagementAuth({ freshFactorIfEnrolled: true }); const { user } = auth; const rl = await checkRateLimit( diff --git a/src/app/api/auth/me/mfa/webauthn/register/options/route.ts b/src/app/api/auth/me/mfa/webauthn/register/options/route.ts index 998c2f3b5..ff09e4f79 100644 --- a/src/app/api/auth/me/mfa/webauthn/register/options/route.ts +++ b/src/app/api/auth/me/mfa/webauthn/register/options/route.ts @@ -8,7 +8,10 @@ * * The cookie arm additionally passes `checkCookieEnrollmentProof`: a session * alone cannot add a security key, for the same reason it cannot enroll TOTP — - * the new key would satisfy every step-up gate from then on. + * the new key would satisfy every step-up gate from then on. On an account that + * already has a second factor, both arms want that factor (or a passkey): a + * password, or a session that skipped the factor on a remembered browser, does + * not add a key. */ import { apiHandler, requireMfaManagementAuth } from "@/lib/api-handler"; import { apiError, apiSuccess, getClientIp } from "@/lib/api-response"; @@ -20,7 +23,7 @@ import { createMfaRegistrationOptions } from "@/lib/auth/mfa/webauthn"; export const dynamic = "force-dynamic"; export const POST = apiHandler(async (req: Request) => { - const auth = await requireMfaManagementAuth(); + const auth = await requireMfaManagementAuth({ freshFactorIfEnrolled: true }); const { user } = auth; const rl = await checkRateLimit( diff --git a/src/app/api/auth/passkey/register-options/route.ts b/src/app/api/auth/passkey/register-options/route.ts index ac848880c..b72bfff13 100644 --- a/src/app/api/auth/passkey/register-options/route.ts +++ b/src/app/api/auth/passkey/register-options/route.ts @@ -5,10 +5,16 @@ import { } from "@/lib/auth/passkey"; import { createMfaAuthenticationOptions } from "@/lib/auth/mfa/webauthn"; import { + isSecondFactorProof, recordReproofFailure, + refundReproof, throttleReproof, verifyExistingFactorProof, } from "@/lib/auth/existing-factor-proof"; +import { + hasSecondFactorEnrolled, + recentProofMethods, +} from "@/lib/auth/second-factor"; import { prisma } from "@/lib/db"; import { apiError, @@ -55,6 +61,25 @@ export const POST = apiHandler(async (request: NextRequest) => { const parsed = stepUpMintSchema.safeParse(body); if (!parsed.success) return proofRequired(); + // On an account with a second factor a password does not add a sign-in + // credential, exactly as it does not satisfy the second factor at sign-in: + // a passkey registered on the strength of the password alone would from then + // on sign in without the second factor at all. Refused before a guess is + // spent, with the proofs that would work. + if ( + !isSecondFactorProof(parsed.data.method) && + (await hasSecondFactorEnrolled(user)) + ) { + annotate({ + action: { name: "auth.reproof.method_refused" }, + meta: { stage: "passkey_enroll", method: parsed.data.method }, + }); + return apiError("Confirm with your second factor or a passkey", 422, { + errorCode: "auth.reproof.too_weak", + methods: await recentProofMethods(user, true), + }); + } + // Every proof counts against the account's shared re-proof bucket and every // refusal is audited. Without both, a stolen session could guess the account // password or a TOTP code here as fast as it could send requests. @@ -73,6 +98,7 @@ export const POST = apiHandler(async (request: NextRequest) => { ); return proofRequired(); } + await refundReproof(user.id); // A strong existing factor refreshes the session's MFA stamp. Password proof // deliberately clears it: registration is authorized by this single-use, diff --git a/src/app/api/auth/password/__tests__/route.test.ts b/src/app/api/auth/password/__tests__/route.test.ts index e13ecff29..557ac34e8 100644 --- a/src/app/api/auth/password/__tests__/route.test.ts +++ b/src/app/api/auth/password/__tests__/route.test.ts @@ -50,6 +50,7 @@ vi.mock("@/lib/auth/password", () => ({ vi.mock("@/lib/rate-limit", () => ({ checkRateLimit: vi.fn(), + refundRateLimit: vi.fn(), })); vi.mock("@/lib/i18n/server-locale", () => ({ @@ -77,7 +78,7 @@ vi.mock("next/headers", () => ({ import { POST } from "../route"; import { prisma } from "@/lib/db"; import { getSession } from "@/lib/auth/session"; -import { checkRateLimit } from "@/lib/rate-limit"; +import { checkRateLimit, refundRateLimit } from "@/lib/rate-limit"; import { verifyPassword, checkPasswordStrength } from "@/lib/auth/password"; import { requireFreshMfaIfEnrolled, @@ -146,6 +147,9 @@ describe("POST /api/auth/password — MFA step-up (v1.25)", () => { expect(res.status).toBe(200); expect(requireFreshMfaIfEnrolled).toHaveBeenCalledWith(expect.any(Number)); expect(prisma.user.update).toHaveBeenCalled(); + // The right current password was not a guess: the shared re-proof + // bucket gets its slot back. + expect(refundRateLimit).toHaveBeenCalledWith("auth:step-up:user-1"); }); }); diff --git a/src/app/api/auth/password/route.ts b/src/app/api/auth/password/route.ts index 47906bc9f..a8935d912 100644 --- a/src/app/api/auth/password/route.ts +++ b/src/app/api/auth/password/route.ts @@ -16,6 +16,7 @@ import { } from "@/lib/api-response"; import { recordReproofFailure, + refundReproof, throttleReproof, } from "@/lib/auth/existing-factor-proof"; import { @@ -72,6 +73,10 @@ export const POST = apiHandler(async (request: NextRequest) => { ); return apiError("Current password is incorrect", 401); } + // The current password was right, so this attempt was not a guess. Give the + // shared re-proof budget its slot back; otherwise a password change followed + // by a few confirmations elsewhere runs the owner out of attempts. + await refundReproof(user.id); if (currentPassword === newPassword) { return apiError("New password must differ from current password", 422); diff --git a/src/app/api/auth/reproof/route.ts b/src/app/api/auth/reproof/route.ts index 0f39e7d13..f01203d96 100644 --- a/src/app/api/auth/reproof/route.ts +++ b/src/app/api/auth/reproof/route.ts @@ -52,6 +52,7 @@ import { annotate } from "@/lib/logging/context"; import { REPROOF_FAILED_CODE, recordReproofFailure, + refundReproof, throttleReproof, verifyExistingFactorProof, } from "@/lib/auth/existing-factor-proof"; @@ -110,6 +111,8 @@ export const POST = apiHandler(async (request: NextRequest) => { }); } + await refundReproof(user.id); + const now = new Date(); await prisma.session.update({ where: { id: session.id }, diff --git a/src/components/settings/security-section/existing-factor-reauth-dialog.tsx b/src/components/settings/security-section/existing-factor-reauth-dialog.tsx index 88fd9c3b7..553fba98f 100644 --- a/src/components/settings/security-section/existing-factor-reauth-dialog.tsx +++ b/src/components/settings/security-section/existing-factor-reauth-dialog.tsx @@ -44,6 +44,28 @@ export function isReproofRequired(err: unknown): boolean { ); } +const REAUTH_METHODS: readonly ReauthMethod[] = [ + "password", + "totp", + "passkey", + "webauthn", +]; + +/** + * The proofs the refusal says this account can give, or null when it names + * none. On an account with a second factor the server leaves the password out, + * so the dialog does not offer a choice that can only be refused. + */ +export function offeredReauthMethods(err: unknown): ReauthMethod[] | null { + if (!(err instanceof ApiError)) return null; + const raw = err.meta?.methods; + if (!Array.isArray(raw)) return null; + const offered = raw.filter((m): m is ReauthMethod => + REAUTH_METHODS.includes(m as ReauthMethod), + ); + return offered.length > 0 ? offered : null; +} + /** * Collect a fresh proof of a credential the account already holds, before a * new factor is added. A passkey or security-key assertion begins at the diff --git a/src/components/settings/security-section/security-keys-card.tsx b/src/components/settings/security-section/security-keys-card.tsx index deeaca438..7dcb374f2 100644 --- a/src/components/settings/security-section/security-keys-card.tsx +++ b/src/components/settings/security-section/security-keys-card.tsx @@ -36,6 +36,7 @@ import { formatDate } from "@/lib/format"; import { ExistingFactorReauthDialog, isReproofRequired, + offeredReauthMethods, type ExistingFactorProof, type ReauthMethod, } from "./existing-factor-reauth-dialog"; @@ -98,8 +99,14 @@ export function SecurityKeysCard({ // options call is retried with it from the dialog. const [reauthOpen, setReauthOpen] = useState(false); const [reauthError, setReauthError] = useState(null); - const reauthMethods: ReauthMethod[] = [ - "password", + // An account with a second factor confirms with that factor or a passkey; + // the server refuses a password there and names what it will take. + const hasSecondFactor = totpEnabled || keys.length > 0; + const [offeredMethods, setOfferedMethods] = useState( + null, + ); + const reauthMethods: ReauthMethod[] = offeredMethods ?? [ + ...(hasSecondFactor ? [] : (["password"] as const)), ...(totpEnabled ? (["totp"] as const) : []), "passkey", ...(keys.length > 0 ? (["webauthn"] as const) : []), @@ -126,6 +133,7 @@ export function SecurityKeysCard({ }, onError: (err, proof) => { if (isReproofRequired(err)) { + setOfferedMethods(offeredReauthMethods(err)); setError(null); setReauthError(null); setReauthOpen(true); diff --git a/src/components/settings/security-section/totp-card.tsx b/src/components/settings/security-section/totp-card.tsx index e35a43d40..7495a9c93 100644 --- a/src/components/settings/security-section/totp-card.tsx +++ b/src/components/settings/security-section/totp-card.tsx @@ -36,7 +36,9 @@ import { RecoveryCodesPanel } from "./recovery-codes-panel"; import { ExistingFactorReauthDialog, isReproofRequired, + offeredReauthMethods, type ExistingFactorProof, + type ReauthMethod, } from "./existing-factor-reauth-dialog"; interface SetupData { @@ -128,6 +130,11 @@ export function TotpCard({ // call is retried with it from the dialog. const [reauthOpen, setReauthOpen] = useState(false); const [reauthError, setReauthError] = useState(null); + const [reauthMethods, setReauthMethods] = useState([ + "password", + "passkey", + "webauthn", + ]); const beginSetup = useMutation({ mutationFn: async (proof?: ExistingFactorProof) => { @@ -148,6 +155,8 @@ export function TotpCard({ }, onError: (err, proof) => { if (isReproofRequired(err)) { + const offered = offeredReauthMethods(err); + if (offered) setReauthMethods(offered); setError(null); setReauthError(null); setReauthOpen(true); @@ -278,7 +287,7 @@ export function TotpCard({ setReauthOpen(open); if (!open) setReauthError(null); }} - methods={["password", "passkey", "webauthn"]} + methods={reauthMethods} pending={beginSetup.isPending} error={reauthError} onProof={(proof) => beginSetup.mutate(proof)} diff --git a/src/lib/__tests__/require-recent-proof.test.ts b/src/lib/__tests__/require-recent-proof.test.ts index 5f3716fb2..8c0d09fd1 100644 --- a/src/lib/__tests__/require-recent-proof.test.ts +++ b/src/lib/__tests__/require-recent-proof.test.ts @@ -38,7 +38,11 @@ vi.mock("next/headers", () => ({ })), })); -import { requireRecentProof, StepUpRequiredError } from "../api-handler"; +import { + requireMfaManagementAuth, + requireRecentProof, + StepUpRequiredError, +} from "../api-handler"; import { getSession } from "@/lib/auth/session"; import { prisma } from "@/lib/db"; import { headers } from "next/headers"; @@ -219,3 +223,44 @@ describe("Bearer", () => { await refusal(requireRecentProof({ bearer: "elevation" })); }); }); + +describe("adding a factor over Bearer (`freshFactorIfEnrolled`)", () => { + const passwordElevation = { + userId: "user-1", + apiTokenId: "tok-1", + consumedAt: null, + expiresAt: new Date(Date.now() + 60_000), + method: "password", + }; + + it("a password-proved elevation still adds a first factor", async () => { + bearer(PLAIN_USER, `hle_${"a".repeat(64)}`); + vi.mocked(prisma.stepUpElevation.findUnique).mockResolvedValue( + passwordElevation as never, + ); + const auth = await requireMfaManagementAuth({ + freshFactorIfEnrolled: true, + }); + expect(auth.transport).toBe("bearer"); + }); + + it("a password-proved elevation does not add a factor beside an existing one", async () => { + bearer(MFA_USER, `hle_${"a".repeat(64)}`); + vi.mocked(prisma.stepUpElevation.findUnique).mockResolvedValue( + passwordElevation as never, + ); + await refusal(requireMfaManagementAuth({ freshFactorIfEnrolled: true })); + }); + + it("a second-factor elevation does", async () => { + bearer(MFA_USER, `hle_${"a".repeat(64)}`); + vi.mocked(prisma.stepUpElevation.findUnique).mockResolvedValue({ + ...passwordElevation, + method: "totp", + } as never); + const auth = await requireMfaManagementAuth({ + freshFactorIfEnrolled: true, + }); + expect(auth.transport).toBe("bearer"); + }); +}); diff --git a/src/lib/api-handler.ts b/src/lib/api-handler.ts index db1087f5a..cf464dea2 100644 --- a/src/lib/api-handler.ts +++ b/src/lib/api-handler.ts @@ -12,6 +12,13 @@ import { annotate, eventStorage, getEvent } from "./logging/context"; import { emitIfSampled } from "./logging/transports"; import { redactOptional, redactSecrets } from "./logging/redact"; import { getSession } from "./auth/session"; +import { + hasSecondFactorEnrolled, + recentProofMethods, + type RecentProofMethod, +} from "./auth/second-factor"; + +export { recentProofMethods, type RecentProofMethod }; import { auditLog, recordDelegatedAccess } from "./auth/audit"; import { resolveBearerToken, @@ -1388,13 +1395,6 @@ export async function requireFreshMfaOrElevationIfEnrolled( return { ...auth, commitElevation }; } -/** - * A proof the recent-proof gate can ask for. `password` and `passkey` on an - * account without a second factor; `totp`, `webauthn` and `passkey` on one - * with a second factor, where a password alone has never been enough. - */ -export type RecentProofMethod = "password" | "totp" | "webauthn" | "passkey"; - /** * How the recent-proof gate treats a Bearer caller. * @@ -1508,63 +1508,6 @@ export async function assertRecentCookieProof( ); } -/** - * The proofs that would satisfy {@link assertRecentCookieProof} for this - * account. Also what `POST /api/auth/reproof` accepts, so the dialog and the - * endpoint cannot disagree. - */ -export async function recentProofMethods( - user: Pick, - enrolled?: boolean, -): Promise { - const hasSecond = enrolled ?? (await hasSecondFactorEnrolled(user)); - const [keys, passkeys] = await Promise.all([ - prisma.webauthnMfaCredential.count({ where: { userId: user.id } }), - prisma.passkey.count({ where: { userId: user.id } }), - ]); - const methods: RecentProofMethod[] = []; - if (hasSecond) { - if (user.totpConfirmedAt) methods.push("totp"); - if (keys > 0) methods.push("webauthn"); - } else if (user.passwordHash) { - methods.push("password"); - } - if (passkeys > 0) methods.push("passkey"); - return methods; -} - -/** - * 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? * @@ -1719,10 +1662,17 @@ export type MfaManagementContext = { * 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. + * @param options.freshFactorIfEnrolled the Bearer arm of ADDING a factor + * (TOTP setup, security-key registration). On an account that already has a + * second factor, the elevation must come from a second-factor or passkey + * proof, the rule `checkCookieEnrollmentProof` holds on the cookie arm; on + * one without, a password-proved elevation is still enough. The cookie arm + * is not changed by it: those routes run their own enrollment proof there. */ export async function requireMfaManagementAuth( options: { freshFactor?: boolean; + freshFactorIfEnrolled?: boolean; proofSource?: FreshFactorProofSource; } = {}, ): Promise { @@ -1747,9 +1697,13 @@ export async function requireMfaManagementAuth( // Bearer path. Resolution first: an unknown, revoked, expired, or narrow-scope // token is refused here and never gets as far as presenting an elevation. const auth = await requireBearerAuth(); + const bearerFreshFactor = + freshFactor || + (options.freshFactorIfEnrolled === true && + (await hasSecondFactorEnrolled(auth.user))); const commitElevation = await resolveBearerElevation( { user: auth.user, apiTokenId: auth.apiTokenId }, - { freshFactor, proofSource }, + { freshFactor: bearerFreshFactor, proofSource }, ); return { diff --git a/src/lib/auth/__tests__/enrollment-proof-second-factor.test.ts b/src/lib/auth/__tests__/enrollment-proof-second-factor.test.ts new file mode 100644 index 000000000..5cdabf23a --- /dev/null +++ b/src/lib/auth/__tests__/enrollment-proof-second-factor.test.ts @@ -0,0 +1,228 @@ +/** + * Adding a credential to an account that already has a second factor. + * + * A password, a young session or a password re-proof used to be enough to + * add a security key, a TOTP authenticator or a passkey even when the account + * was protected by a second factor. A session that signed in on a remembered + * browser skips the factor and is still young, so the password alone could + * add a key that then satisfies every gate the factor protected. On such an + * account only possession of a factor counts now; the account without one is + * unchanged. + */ +import { describe, it, expect, vi, beforeEach } from "vitest"; + +vi.mock("@/lib/db", () => ({ + prisma: { + session: { findUnique: vi.fn(), update: vi.fn() }, + webauthnMfaCredential: { count: vi.fn() }, + passkey: { count: vi.fn() }, + authChallenge: { findUnique: vi.fn() }, + }, +})); +vi.mock("@/lib/rate-limit", () => ({ + checkRateLimit: vi.fn(), + rateLimitHeaders: vi.fn(() => ({})), + refundRateLimit: vi.fn(), +})); +vi.mock("@/lib/auth/audit", () => ({ + auditLog: vi.fn().mockResolvedValue(undefined), +})); +vi.mock("@/lib/logging/context", () => ({ annotate: vi.fn() })); +vi.mock("@/lib/auth/password", () => ({ verifyPassword: vi.fn() })); +vi.mock("@/lib/auth/passkey", () => ({ + verifyAuthentication: vi.fn(), + createAuthenticationOptions: vi.fn(), + createRegistrationOptions: vi.fn(), +})); +vi.mock("@/lib/auth/mfa/webauthn", () => ({ + verifyMfaAuthentication: vi.fn(), + createMfaAuthenticationOptions: vi.fn(), +})); +vi.mock("@/lib/auth/mfa/verify-factor", () => ({ verifyMfaFactor: vi.fn() })); +vi.mock("@/lib/api-handler", () => ({ + apiHandler: (fn: unknown) => fn, + requireCookieAuth: vi.fn(), +})); + +import { prisma } from "@/lib/db"; +import { checkRateLimit, refundRateLimit } from "@/lib/rate-limit"; +import { verifyPassword } from "@/lib/auth/password"; +import { verifyMfaFactor } from "@/lib/auth/mfa/verify-factor"; +import { createRegistrationOptions } from "@/lib/auth/passkey"; +import { requireCookieAuth } from "@/lib/api-handler"; +import { checkCookieEnrollmentProof } from "../existing-factor-proof"; +import { POST as PASSKEY_REGISTER_OPTIONS } from "@/app/api/auth/passkey/register-options/route"; + +const NOW = Date.now(); +const fresh = new Date(NOW - 60_000); +const stale = new Date(NOW - 60 * 60_000); + +const ENROLLED: { + id: string; + username: string; + email: string; + passwordHash: string; + totpSecretEncrypted: string | null; + totpLastStep: null; + totpConfirmedAt: Date | null; +} = { + id: "user-1", + username: "u", + email: "u@example.com", + passwordHash: "hash", + totpSecretEncrypted: "enc", + totpLastStep: null, + totpConfirmedAt: new Date("2026-01-01"), +}; +const PLAIN = { ...ENROLLED, totpSecretEncrypted: null, totpConfirmedAt: null }; + +function req(body?: unknown): Request { + return new Request("http://localhost/api/auth/me/mfa/totp/setup", { + method: "POST", + headers: body === undefined ? {} : { "content-type": "application/json" }, + body: body === undefined ? undefined : JSON.stringify(body), + }); +} + +function session(stamps: { + mfaVerifiedAt?: Date | null; + createdAt?: Date; + reproofAt?: Date | null; +}) { + vi.mocked(prisma.session.findUnique).mockResolvedValue({ + mfaVerifiedAt: stamps.mfaVerifiedAt ?? null, + createdAt: stamps.createdAt ?? stale, + reproofAt: stamps.reproofAt ?? null, + } as never); +} + +function gate(user: typeof ENROLLED | typeof PLAIN, request: Request) { + return checkCookieEnrollmentProof({ + user: user as never, + sessionId: "sess-1", + request, + ipAddress: "203.0.113.9", + stage: "totp_enroll", + }); +} + +beforeEach(() => { + vi.resetAllMocks(); + vi.mocked(prisma.webauthnMfaCredential.count).mockResolvedValue(0 as never); + vi.mocked(prisma.passkey.count).mockResolvedValue(0 as never); + vi.mocked(checkRateLimit).mockResolvedValue({ + allowed: true, + limit: 5, + remaining: 4, + resetAt: NOW + 1000, + }); +}); + +describe("account with a second factor", () => { + it("a session signed in a minute ago is not enough (remembered browser)", async () => { + session({ createdAt: fresh }); + const res = await gate(ENROLLED, req()); + expect(res?.status).toBe(401); + const body = await res!.json(); + expect(body.meta.errorCode).toBe("auth.reproof.required"); + expect(body.meta.methods).toEqual(["totp"]); + }); + + it("a password re-proof stamp is not enough", async () => { + session({ reproofAt: fresh }); + const res = await gate(ENROLLED, req()); + expect(res?.status).toBe(401); + }); + + it("a password in the body is refused before it is verified", async () => { + session({ createdAt: fresh }); + const res = await gate( + ENROLLED, + req({ method: "password", password: "correct horse" }), + ); + expect(res?.status).toBe(401); + expect(verifyPassword).not.toHaveBeenCalled(); + expect(checkRateLimit).not.toHaveBeenCalled(); + }); + + it("a completed second factor within five minutes passes", async () => { + session({ mfaVerifiedAt: fresh }); + expect(await gate(ENROLLED, req())).toBeNull(); + }); + + it("a TOTP code in the body passes and gives the attempt back", async () => { + session({}); + vi.mocked(verifyMfaFactor).mockResolvedValue({ ok: true } as never); + expect(await gate(ENROLLED, req({ method: "totp", code: "123456" }))).toBe( + null, + ); + expect(refundRateLimit).toHaveBeenCalledWith("auth:step-up:user-1"); + }); + + it("a security key alone makes the account enrolled", async () => { + vi.mocked(prisma.webauthnMfaCredential.count).mockResolvedValue(1 as never); + session({ createdAt: fresh }); + const res = await gate(PLAIN, req()); + expect(res?.status).toBe(401); + expect((await res!.json()).meta.methods).toEqual(["webauthn"]); + }); +}); + +describe("account without a second factor (unchanged)", () => { + it("a session signed in a minute ago passes", async () => { + session({ createdAt: fresh }); + expect(await gate(PLAIN, req())).toBeNull(); + }); + + it("a password in the body passes", async () => { + session({}); + vi.mocked(verifyPassword).mockResolvedValue(true); + expect( + await gate(PLAIN, req({ method: "password", password: "pw" })), + ).toBeNull(); + expect(verifyPassword).toHaveBeenCalled(); + }); +}); + +describe("POST /api/auth/passkey/register-options", () => { + function passkeyReq(body: unknown) { + return new Request("http://localhost/api/auth/passkey/register-options", { + method: "POST", + headers: { "content-type": "application/json" }, + body: JSON.stringify(body), + }); + } + + it("refuses a password on an account with a second factor, before verifying it", async () => { + vi.mocked(requireCookieAuth).mockResolvedValue({ + user: ENROLLED, + session: { id: "sess-1" }, + } as never); + const res = await PASSKEY_REGISTER_OPTIONS( + passkeyReq({ method: "password", password: "correct horse" }) as never, + ); + expect(res.status).toBe(422); + const body = await res.json(); + expect(body.meta.errorCode).toBe("auth.reproof.too_weak"); + expect(body.meta.methods).toEqual(["totp"]); + expect(verifyPassword).not.toHaveBeenCalled(); + expect(createRegistrationOptions).not.toHaveBeenCalled(); + }); + + it("still takes a password on an account without one", async () => { + vi.mocked(requireCookieAuth).mockResolvedValue({ + user: PLAIN, + session: { id: "sess-1" }, + } as never); + vi.mocked(verifyPassword).mockResolvedValue(true); + vi.mocked(createRegistrationOptions).mockResolvedValue({ + options: {}, + challengeId: "ch-1", + } as never); + const res = await PASSKEY_REGISTER_OPTIONS( + passkeyReq({ method: "password", password: "pw" }) as never, + ); + expect(res.status).toBe(200); + expect(refundRateLimit).toHaveBeenCalledWith("auth:step-up:user-1"); + }); +}); diff --git a/src/lib/auth/existing-factor-proof.ts b/src/lib/auth/existing-factor-proof.ts index 64915503d..e44c3b53c 100644 --- a/src/lib/auth/existing-factor-proof.ts +++ b/src/lib/auth/existing-factor-proof.ts @@ -22,7 +22,15 @@ import { apiError } from "@/lib/api-response"; import { REPROOF_REQUIRED_CODE } from "@/lib/api-errors"; import { auditLog } from "@/lib/auth/audit"; import { annotate } from "@/lib/logging/context"; -import { checkRateLimit, rateLimitHeaders } from "@/lib/rate-limit"; +import { + checkRateLimit, + rateLimitHeaders, + refundRateLimit, +} from "@/lib/rate-limit"; +import { + hasSecondFactorEnrolled, + recentProofMethods, +} from "@/lib/auth/second-factor"; import { verifyPassword } from "@/lib/auth/password"; import { verifyAuthentication } from "@/lib/auth/passkey"; import { verifyMfaAuthentication } from "@/lib/auth/mfa/webauthn"; @@ -73,6 +81,16 @@ export async function throttleReproof( }); } +/** + * Give back the attempt a proof charged once it verified. The bucket exists to + * stop guessing; a proof that verified was not a guess, and without the refund + * an owner who confirms a few actions in a row (an export, a share link, a + * password change) runs the shared budget dry and is told to wait. + */ +export async function refundReproof(userId: string): Promise { + await refundRateLimit(reproofBucket(userId)); +} + export type ProofOutcome = { ok: true } | { ok: false; reason: string }; /** @@ -184,14 +202,15 @@ export const RECENT_PROOF_MAX_AGE_MS = 5 * 60 * 1000; * session could enroll their own authenticator, and from then on hold a factor * that satisfies every step-up gate, registers a passkey and survives the * owner changing the password. So the caller must show that the person at the - * keyboard can authenticate as the account owner right now, in one of three - * ways: + * keyboard can authenticate as the account owner right now. * - * - the session completed a second factor or passkey sign-in within the last - * five minutes (`Session.mfaVerifiedAt`); + * What counts depends on whether the account already has a second factor. + * + * Without one (password, passkey or single sign-on only), any of: + * - the session completed a passkey sign-in or re-proof within the last five + * minutes (`Session.mfaVerifiedAt`); * - the session re-proved a credential at `POST /api/auth/reproof` within - * the last five minutes (`Session.reproofAt`), which accepts the same - * proofs this gate takes in the body; + * the last five minutes (`Session.reproofAt`); * - the session itself was created by a sign-in within the last five * minutes (`Session.createdAt`, which no later write touches) — this is * also what lets an SSO-only account with nothing else to re-prove enroll @@ -200,6 +219,13 @@ export const RECENT_PROOF_MAX_AGE_MS = 5 * 60 * 1000; * `{ method: "totp", code }` or a passkey / security-key assertion, the * same shapes the step-up mint accepts. * + * With one, only a proof of possession of a factor: `Session.mfaVerifiedAt` + * within five minutes, or a TOTP code, security-key or passkey assertion in + * the body. A password is not enough and neither is a young session: signing + * in on a remembered browser skips the second factor and still creates a + * fresh session, so `createdAt` would let the password alone add a new key, + * which then satisfies every gate the second factor was protecting. + * * Returns the refusal to send, or null to proceed. Proofs in the body are * throttled and audited through the shared re-proof bucket. */ @@ -212,6 +238,7 @@ export async function checkCookieEnrollmentProof(args: { }): Promise { const { user, sessionId, request, ipAddress, stage } = args; + const enrolled = await hasSecondFactorEnrolled(user); const row = await prisma.session.findUnique({ where: { id: sessionId }, select: { mfaVerifiedAt: true, createdAt: true, reproofAt: true }, @@ -219,20 +246,25 @@ export async function checkCookieEnrollmentProof(args: { const now = Date.now(); const recent = (d: Date | null | undefined) => d != null && now - d.getTime() <= RECENT_PROOF_MAX_AGE_MS; - if ( - recent(row?.mfaVerifiedAt) || - recent(row?.createdAt) || - recent(row?.reproofAt) - ) { + const sessionSatisfies = enrolled + ? recent(row?.mfaVerifiedAt) + : recent(row?.mfaVerifiedAt) || + recent(row?.createdAt) || + recent(row?.reproofAt); + if (sessionSatisfies) { annotate({ meta: { enrollment_proof: "recent_session" } }); return null; } const proof = await readProof(request); if (proof === "absent") { - annotate({ action: { name: "auth.reproof.required" }, meta: { stage } }); + annotate({ + action: { name: "auth.reproof.required" }, + meta: { stage, second_factor: enrolled }, + }); return apiError("Confirm it is you before adding a sign-in method", 401, { errorCode: REPROOF_REQUIRED_CODE, + methods: await recentProofMethods(user, enrolled), }); } if (proof === "invalid") { @@ -240,6 +272,18 @@ export async function checkCookieEnrollmentProof(args: { errorCode: REPROOF_REQUIRED_CODE, }); } + // Refused before a guess is spent: a password that verified would still not + // open this gate on an account with a second factor. + if (enrolled && !isSecondFactorProof(proof.method)) { + annotate({ + action: { name: "auth.reproof.method_refused" }, + meta: { stage, method: proof.method }, + }); + return apiError("Confirm with your second factor or a passkey", 401, { + errorCode: REPROOF_REQUIRED_CODE, + methods: await recentProofMethods(user, enrolled), + }); + } const limited = await throttleReproof(user.id, ipAddress, stage); if (limited) return limited; @@ -257,10 +301,22 @@ export async function checkCookieEnrollmentProof(args: { errorCode: REPROOF_FAILED_CODE, }); } + await refundReproof(user.id); annotate({ meta: { enrollment_proof: proof.method } }); return null; } +/** + * The proofs that stand for possession of a factor rather than knowledge of + * the password. On an account with a second factor these are the only ones + * that may add a credential. + */ +export function isSecondFactorProof( + method: ExistingFactorProof["method"], +): boolean { + return method === "totp" || method === "webauthn" || method === "passkey"; +} + async function readProof( request: Request, ): Promise { diff --git a/src/lib/auth/second-factor.ts b/src/lib/auth/second-factor.ts new file mode 100644 index 000000000..48634c9fa --- /dev/null +++ b/src/lib/auth/second-factor.ts @@ -0,0 +1,66 @@ +import type { User } from "@/generated/prisma/client"; +import { prisma } from "@/lib/db"; + +/** + * 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. `canProveFreshFactor` in `api-handler.ts` is where the wider question + * lives. + */ +export 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; +} + +/** + * A proof the recent-proof gate can ask for. `password` and `passkey` on an + * account without a second factor; `totp`, `webauthn` and `passkey` on one + * with a second factor, where a password alone has never been enough. + */ +export type RecentProofMethod = "password" | "totp" | "webauthn" | "passkey"; + +/** + * The proofs that would satisfy `assertRecentCookieProof` for this + * account. Also what `POST /api/auth/reproof` accepts, so the dialog and the + * endpoint cannot disagree. + */ +export async function recentProofMethods( + user: Pick, + enrolled?: boolean, +): Promise { + const hasSecond = enrolled ?? (await hasSecondFactorEnrolled(user)); + const [keys, passkeys] = await Promise.all([ + prisma.webauthnMfaCredential.count({ where: { userId: user.id } }), + prisma.passkey.count({ where: { userId: user.id } }), + ]); + const methods: RecentProofMethod[] = []; + if (hasSecond) { + if (user.totpConfirmedAt) methods.push("totp"); + if (keys > 0) methods.push("webauthn"); + } else if (user.passwordHash) { + methods.push("password"); + } + if (passkeys > 0) methods.push("passkey"); + return methods; +} From a4d27c618ecb25e8ca2c64818ea11492f54e7ab7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Bombeck?= Date: Sat, 26 Sep 2026 22:21:43 +0200 Subject: [PATCH 2/9] fix(export): every whole-record export asks for a fresh proof GET /api/export with type=all (the default), POST /api/export/health-record and GET /api/fhir/Patient/$everything hand over the whole record but took a live session alone, so a stolen browser session could still walk off with everything the full-backup gate was meant to protect. A browser session now needs a sign-in or re-proof within five minutes, with the second factor on an account that has one, and the doctor-report panel opens the re-proof dialog and retries. When a manager builds the report for a record they manage, their own session is the one asked. On a token the shipped app keeps working: health-record and $everything take the token as before, including a fhir:read token, and the recent-proof guard names both. GET /api/export?type=all, which no shipped client calls with a token, wants a step-up elevation there. Single-type exports are unchanged. The API reference now also describes the enrollment rule for accounts with a second factor and the passkey refusal it adds. --- docs/api/openapi.yaml | 71 ++++++++---- .../recent-proof-surface-guard.test.ts | 27 ++++- .../__tests__/soft-delete-filter.test.ts | 39 +++++++ .../health-record/__tests__/route.test.ts | 26 +++++ src/app/api/export/health-record/route.ts | 18 ++- src/app/api/export/route.ts | 22 +++- src/app/api/fhir/Patient/$everything/route.ts | 20 +++- .../capability-and-everything.test.ts | 27 +++++ .../api/fhir/__tests__/module-gate.test.ts | 11 ++ .../report-selection/selection-panel.tsx | 104 +++++++++++------- src/lib/openapi/routes/auth.ts | 9 +- src/lib/openapi/routes/export.ts | 2 +- src/lib/openapi/routes/health-record.ts | 4 +- 13 files changed, 304 insertions(+), 76 deletions(-) diff --git a/docs/api/openapi.yaml b/docs/api/openapi.yaml index 5953a81d8..0b8a74389 100644 --- a/docs/api/openapi.yaml +++ b/docs/api/openapi.yaml @@ -864,7 +864,11 @@ paths: `selection` is REQUIRED and carries the leaf inclusion list — membership is inclusion, absence is exclusion, and there is no server-side default. An unknown leaf id is a 422 that names it; the retired grouped `sections` shape is a 422 too, because accepting it would mean choosing a scope the caller did not express. Auth via cookie or - Bearer; shared `export:` rate bucket (10/h). Strict validation: unknown keys 422." + Bearer; shared `export:` rate bucket (10/h). Strict validation: unknown keys 422. A browser session + needs a fresh proof as well as a live one: a sign-in or `POST /api/auth/reproof` within the last five minutes + (with a second factor, on an account that has one). Without it the answer is 401 `auth.reproof.required`, with + `meta.methods` naming the proofs the account can give. When a manager exports a record they manage, it is their + own session that is asked. A Bearer caller is not asked for more than its token." requestBody: required: true content: @@ -1489,7 +1493,10 @@ paths: Conditions, Encounters, AllergyIntolerances, FamilyMemberHistories and the DiagnosticReport (present when the window holds a vital sign) — flattened into one `searchset` Bundle. Scoped to the owner's SAVED report selection; an account that never saved one gets nothing. Auth: `fhir:read` scope. Offset paging via `_count` - (≤200) / `_offset`." + (≤200) / `_offset`. A browser session needs a fresh proof as well as a live one: a sign-in or `POST + /api/auth/reproof` within the last five minutes (with a second factor, on an account that has one). Without it + the answer is 401 `auth.reproof.required`, with `meta.methods` naming the proofs the account can give. A token, + including a `fhir:read` one, is not asked for more." parameters: - in: query name: _count @@ -1524,7 +1531,13 @@ paths: notes ride their exported column. Shares the `export:` bucket with the encrypted variant (10 per hour) and audits every attempt as `export.download`. Cookie or Bearer auth; the caller is always resolved as themselves, so this download cannot be delegated to a shared record. Not module-gated: a disabled module's rows - are still exported, because this is the account's own data leaving on request." + are still exported, because this is the account's own data leaving on request. `type=all`, the default, is the + whole record and asks for a fresh proof like the full backup; the single-type exports do not. A browser session + needs a fresh proof as well as a live one: a sign-in or `POST /api/auth/reproof` within the last five minutes + (with a second factor, on an account that has one). Without it the answer is 401 `auth.reproof.required`, with + `meta.methods` naming the proofs the account can give. A Bearer caller asking for `type=all` presents an + `X-Step-Up` elevation from `POST /api/auth/step-up` (a second-factor one on an account with a second factor) and + receives 401 `auth.stepup.required` without it." parameters: - in: query name: format @@ -2172,14 +2185,18 @@ paths: /api/auth/step-up in the `X-Step-Up` header; a Bearer token alone is still refused. - Adding a factor needs a fresh proof, not just a session. On the Bearer path the elevation is that proof (a - password-proved one is enough). On the cookie path the request passes when the session signed in, or completed a - second factor, within the last five minutes; otherwise the body must carry a proof in the shape POST - /api/auth/step-up takes (`password`, a `totp` code from an already confirmed authenticator, or a passkey / - security-key assertion begun at POST /api/auth/passkey/register-options). Without one the answer is 401 - `auth.reproof.required`; a proof that does not verify is 401 `auth.reproof.failed` and is audited. Proofs draw - on the account's shared re-proof budget of five per fifteen minutes (429 once spent), the same budget as the - step-up mint. + Adding a factor needs a fresh proof, not just a session. On the Bearer path the elevation is that proof: a + password-proved one is enough on an account without a second factor, and on an account that already has one the + elevation must come from that factor or a passkey. On the cookie path, for an account without a second factor, + the request passes when the session signed in, or completed a passkey sign-in, within the last five minutes; + otherwise the body must carry a proof in the shape POST /api/auth/step-up takes (`password`, or a passkey + assertion begun at POST /api/auth/passkey/register-options). For an account that already has a second factor, + only that factor counts: a second factor completed within the last five minutes, or a `totp` code, security-key + or passkey assertion in the body. A password or a young session (for example one that skipped the factor on a + remembered browser) is not enough there. Without one the answer is 401 `auth.reproof.required` with + `meta.methods` naming the proofs the account can give; a proof that does not verify is 401 `auth.reproof.failed` + and is audited. Proofs draw on the account's shared re-proof budget of five per fifteen minutes (429 once + spent), the same budget as the step-up mint. requestBody: required: false content: @@ -2316,14 +2333,18 @@ paths: refused. - Adding a factor needs a fresh proof, not just a session. On the Bearer path the elevation is that proof (a - password-proved one is enough). On the cookie path the request passes when the session signed in, or completed a - second factor, within the last five minutes; otherwise the body must carry a proof in the shape POST - /api/auth/step-up takes (`password`, a `totp` code from an already confirmed authenticator, or a passkey / - security-key assertion begun at POST /api/auth/passkey/register-options). Without one the answer is 401 - `auth.reproof.required`; a proof that does not verify is 401 `auth.reproof.failed` and is audited. Proofs draw - on the account's shared re-proof budget of five per fifteen minutes (429 once spent), the same budget as the - step-up mint. + Adding a factor needs a fresh proof, not just a session. On the Bearer path the elevation is that proof: a + password-proved one is enough on an account without a second factor, and on an account that already has one the + elevation must come from that factor or a passkey. On the cookie path, for an account without a second factor, + the request passes when the session signed in, or completed a passkey sign-in, within the last five minutes; + otherwise the body must carry a proof in the shape POST /api/auth/step-up takes (`password`, or a passkey + assertion begun at POST /api/auth/passkey/register-options). For an account that already has a second factor, + only that factor counts: a second factor completed within the last five minutes, or a `totp` code, security-key + or passkey assertion in the body. A password or a young session (for example one that skipped the factor on a + remembered browser) is not enough there. Without one the answer is 401 `auth.reproof.required` with + `meta.methods` naming the proofs the account can give; a proof that does not verify is 401 `auth.reproof.failed` + and is audited. Proofs draw on the account's shared re-proof budget of five per fifteen minutes (429 once + spent), the same budget as the step-up mint. requestBody: required: false content: @@ -3453,7 +3474,9 @@ paths: **Enrollment arm.** Any other body must be a full factor proof (the same shape POST /api/auth/step-up takes): a password, a TOTP code, or a completed passkey / security-key assertion. Only then are registration options - issued. + issued. On an account that already has a second factor a password is refused before it is checked, with 422 + `auth.reproof.too_weak` and `meta.methods` naming the proofs that work: a passkey added on the password alone + would sign in without the second factor from then on. Which factor you prove changes the session, and the direction is not the obvious one: a strong proof stamps the @@ -3500,7 +3523,13 @@ paths: application/json: schema: $ref: "#/components/schemas/ErrorEnvelope" - "422": *a4 + "422": + description: "`auth.reproof.too_weak`: a password on an account that already has a second factor. `meta.methods` names + the proofs that would be accepted. Refused before the password is checked, so it spends no attempt." + content: + application/json: + schema: + $ref: "#/components/schemas/ErrorEnvelope" "429": *a5 /api/auth/passkey/register-verify: post: diff --git a/src/__tests__/recent-proof-surface-guard.test.ts b/src/__tests__/recent-proof-surface-guard.test.ts index 26fce210d..9a1e0f8ae 100644 --- a/src/__tests__/recent-proof-surface-guard.test.ts +++ b/src/__tests__/recent-proof-surface-guard.test.ts @@ -73,6 +73,8 @@ function callers(exportName: string): string[] { /** Routes that resolve their own caller and take the recent-proof gate. */ const RECENT_PROOF_ROUTES: Record = { + // `type=all` only; the single-type exports stay on `requireAuth`. + "app/api/export/route.ts": "elevation", "app/api/export/full-backup/route.ts": "elevation", "app/api/export/encrypted/route.ts": "elevation-if-enrolled", // The shipped iOS app calls these two on its token without an elevation and @@ -82,6 +84,17 @@ const RECENT_PROOF_ROUTES: Record = { "app/api/mcp/tokens/route.ts": "token", }; +/** + * Routes that resolve a cookie OR a token through their own resolver + * (`requireAuth` with a scope, `requireRecordAuth`) and add the cookie arm + * only. Their Bearer arm is the token, as with the `token` rule above: the + * shipped app calls both with its token and no elevation. + */ +const TOKEN_ARM_COOKIE_PROOF_ROUTES = [ + "app/api/export/health-record/route.ts", + "app/api/fhir/Patient/$everything/route.ts", +].sort(); + /** Cookie-only routes that add the gate after their own resolver. */ const COOKIE_PROOF_ROUTES = [ "app/api/admin/backups/[id]/download/route.ts", @@ -112,10 +125,20 @@ describe("the recent-proof gate", () => { expect(found).toEqual(RECENT_PROOF_ROUTES); }); - it("the cookie arm is added by exactly the known cookie-only routes", () => { + it("the cookie arm is added by exactly the known routes", () => { const found = callers("assertRecentCookieProof"); expect(found.length).toBeGreaterThan(0); - expect(found).toEqual(COOKIE_PROOF_ROUTES); + expect(found).toEqual( + [...COOKIE_PROOF_ROUTES, ...TOKEN_ARM_COOKIE_PROOF_ROUTES].sort(), + ); + }); + + it("a route that resolves either transport asks only on the cookie", () => { + for (const rel of TOKEN_ARM_COOKIE_PROOF_ROUTES) { + expect(read(rel), rel).toMatch( + /if \(auth\.authMethod === "cookie"\) \{\s*await assertRecentCookieProof\(/, + ); + } }); it("each cookie-only route resolves a cookie before it asks", () => { diff --git a/src/app/api/export/__tests__/soft-delete-filter.test.ts b/src/app/api/export/__tests__/soft-delete-filter.test.ts index 03631ffa8..45cb98d38 100644 --- a/src/app/api/export/__tests__/soft-delete-filter.test.ts +++ b/src/app/api/export/__tests__/soft-delete-filter.test.ts @@ -318,3 +318,42 @@ describe("v1.4.41 W-DELETED-2 — soft-delete invisibility", () => { } }); }); + +describe("GET /api/export — the whole record asks for a fresh proof", () => { + const stale = { + createdAt: new Date(Date.now() - 60 * 60 * 1000), + mfaVerifiedAt: null, + reproofAt: null, + }; + + it("type=all on a session that signed in an hour ago is asked to confirm", async () => { + vi.mocked(prisma.session.findUnique).mockResolvedValueOnce(stale as never); + const { GET } = await import("../route"); + const res = await GET(mkReq("http://localhost/api/export?type=all")); + expect(res.status).toBe(401); + expect((await res.json()).meta.errorCode).toBe("auth.reproof.required"); + expect(prisma.measurement.findMany).not.toHaveBeenCalled(); + }); + + it("no type means the whole record, and is gated the same way", async () => { + vi.mocked(prisma.session.findUnique).mockResolvedValueOnce(stale as never); + const { GET } = await import("../route"); + const res = await GET(mkReq("http://localhost/api/export")); + expect(res.status).toBe(401); + }); + + it("type=all right after signing in goes through", async () => { + const { GET } = await import("../route"); + const res = await GET(mkReq("http://localhost/api/export?type=all")); + expect(res.status).toBe(200); + }); + + it("a single-type export stays on the session alone", async () => { + vi.mocked(prisma.session.findUnique).mockResolvedValue(stale as never); + const { GET } = await import("../route"); + const res = await GET( + mkReq("http://localhost/api/export?type=measurements"), + ); + expect(res.status).toBe(200); + }); +}); diff --git a/src/app/api/export/health-record/__tests__/route.test.ts b/src/app/api/export/health-record/__tests__/route.test.ts index 9a761419c..664e6fc4f 100644 --- a/src/app/api/export/health-record/__tests__/route.test.ts +++ b/src/app/api/export/health-record/__tests__/route.test.ts @@ -32,6 +32,17 @@ vi.mock("@/lib/db", () => ({ userHealthProfile: { findUnique: vi.fn() }, healthProfileFactRevision: { findMany: vi.fn() }, user: { findUnique: vi.fn(), update: vi.fn() }, + // The whole-record gate reads the session's own stamps: a sign-in a + // moment ago is the proof, on an account without a second factor. + session: { + findUnique: vi.fn(async () => ({ + createdAt: new Date(), + mfaVerifiedAt: null, + reproofAt: null, + })), + }, + webauthnMfaCredential: { count: vi.fn(async () => 0) }, + passkey: { count: vi.fn(async () => 0) }, }, toJson: (value: unknown) => value, })); @@ -908,3 +919,18 @@ describe("POST /api/export/health-record — the insurance leaf", () => { expect(text).toContain("A123456789"); }); }); + +describe("POST /api/export/health-record — a fresh proof on the browser", () => { + it("a session that signed in an hour ago is asked to confirm first", async () => { + vi.mocked(prisma.session.findUnique).mockResolvedValueOnce({ + createdAt: new Date(Date.now() - 60 * 60 * 1000), + mfaVerifiedAt: null, + reproofAt: null, + } as never); + const { POST } = await import("../route"); + const res = await POST(mkReq({ format: "pdf", selection: allLeaves() })); + expect(res.status).toBe(401); + expect((await res.json()).meta.errorCode).toBe("auth.reproof.required"); + expect(prisma.measurement.findMany).not.toHaveBeenCalled(); + }); +}); diff --git a/src/app/api/export/health-record/route.ts b/src/app/api/export/health-record/route.ts index 9937fa4b8..282da0731 100644 --- a/src/app/api/export/health-record/route.ts +++ b/src/app/api/export/health-record/route.ts @@ -22,7 +22,11 @@ */ import { NextRequest, NextResponse } from "next/server"; -import { apiHandler, requireRecordAuth } from "@/lib/api-handler"; +import { + apiHandler, + assertRecentCookieProof, + requireRecordAuth, +} from "@/lib/api-handler"; import { annotate } from "@/lib/logging/context"; import { auditLog } from "@/lib/auth/audit"; import { @@ -88,7 +92,17 @@ export const POST = apiHandler(async (request: NextRequest) => { // v1.37.0 — MANAGE: the doctor report is the one export an invited manager // reaches, because it renders a prepared artefact from a declared selection // rather than handing over the record whole. - const { user, actor } = await requireRecordAuth("manage", "record"); + const auth = await requireRecordAuth("manage", "record"); + const { user, actor } = auth; + // A whole-record artefact: a browser session must have signed in or + // re-proved within five minutes, the gate every whole-record export takes. + // The proof is the person at the keyboard, so it is the actor's session + // that is asked, whether they export their own record or one they manage. + // A token is its own proof here, as before: the shipped app calls this + // route with its token and no elevation. + if (auth.authMethod === "cookie") { + await assertRecentCookieProof(actor, auth.session.id); + } annotate({ action: { name: "export.health-record.build" } }); // The whole doctor-report surface is the `doctorReport` module. Refuse with diff --git a/src/app/api/export/route.ts b/src/app/api/export/route.ts index 82a2c133e..0e26e7560 100644 --- a/src/app/api/export/route.ts +++ b/src/app/api/export/route.ts @@ -1,5 +1,5 @@ import { prisma } from "@/lib/db"; -import { apiHandler, requireAuth } from "@/lib/api-handler"; +import { apiHandler, requireAuth, requireRecentProof } from "@/lib/api-handler"; import { annotate } from "@/lib/logging/context"; import { auditLog } from "@/lib/auth/audit"; import { apiError, getClientIp } from "@/lib/api-response"; @@ -24,12 +24,23 @@ import { NextRequest, NextResponse } from "next/server"; * Export user data. * Query params: * format: "csv" | "json" (default: json) - * type: "measurements" | "medications" | "intake" | "all" (default: all) + * type: "measurements" | "medications" | "intake" | "mood" | "all" + * (default: all) * - * Re-authentication is required via session (user must be logged in). + * `type=all` (the default) is the whole record in one file, so it takes the + * same fresh-proof gate as every other whole-record export + * (`requireRecentProof`): a browser session must have signed in or re-proved + * within five minutes, a token must present a step-up elevation. No shipped + * client calls this route with a token. The single-type exports stay on the + * session alone, like their dedicated routes under `/api/export/*`. */ export const GET = apiHandler(async (request: NextRequest) => { - const { user } = await requireAuth(); + const wholeRecord = + (new URL(request.url).searchParams.get("type") ?? "all") === "all"; + const auth = wholeRecord + ? await requireRecentProof({ bearer: "elevation" }) + : { ...(await requireAuth()), commitElevation: async () => {} }; + const { user } = auth; const rl = await checkRateLimit(`export:${user.id}`, 10, 60 * 60 * 1000); if (!rl.allowed) { @@ -54,6 +65,9 @@ export const GET = apiHandler(async (request: NextRequest) => { ); } + // Spend a Bearer elevation only once the request is about to act. + await auth.commitElevation(); + const userId = user.id; await auditLog("export.download", { userId, diff --git a/src/app/api/fhir/Patient/$everything/route.ts b/src/app/api/fhir/Patient/$everything/route.ts index bf596b780..68faf6304 100644 --- a/src/app/api/fhir/Patient/$everything/route.ts +++ b/src/app/api/fhir/Patient/$everything/route.ts @@ -13,12 +13,17 @@ * resolves inside the response and the operation can never quietly carry less * than the document does. * - * Read-only; `userId` narrowed from `requireAuth`. Offset paging applies across + * Read-only; `userId` narrowed from `requireAuth`. A browser session needs a + * recent sign-in or re-proof (`assertRecentCookieProof`). Offset paging applies across * the flattened resource list via `_count` (clamped ≤200) / `_offset`. */ import { NextRequest } from "next/server"; -import { apiHandler, requireAuth } from "@/lib/api-handler"; +import { + apiHandler, + assertRecentCookieProof, + requireAuth, +} from "@/lib/api-handler"; import { requireModuleEnabled } from "@/lib/modules/gate"; import { annotate } from "@/lib/logging/context"; import { checkRateLimit } from "@/lib/rate-limit"; @@ -32,7 +37,16 @@ import { } from "@/lib/fhir/rest"; export const GET = apiHandler(async (request: NextRequest) => { - const { user } = await requireAuth(FHIR_READ_SCOPE); + const auth = await requireAuth(FHIR_READ_SCOPE); + const { user } = auth; + // The whole record in one response: a browser session must have signed in + // or re-proved within five minutes, like every other whole-record export. + // A token is its own proof here, as before: the shipped app and the FHIR + // read tokens call this route without an elevation, and a refusal would + // read to the app as a dead session. + if (auth.authMethod === "cookie") { + await assertRecentCookieProof(user, auth.session.id); + } annotate({ action: { name: "fhir.everything.read" } }); // v1.30 — the FHIR REST face serves the SAME whole-record aggregate as diff --git a/src/app/api/fhir/__tests__/capability-and-everything.test.ts b/src/app/api/fhir/__tests__/capability-and-everything.test.ts index 22f1f077f..c402cf539 100644 --- a/src/app/api/fhir/__tests__/capability-and-everything.test.ts +++ b/src/app/api/fhir/__tests__/capability-and-everything.test.ts @@ -15,6 +15,21 @@ import { existsSync } from "node:fs"; import path from "node:path"; vi.mock("@/lib/auth/session", () => ({ getSession: vi.fn() })); +vi.mock("@/lib/db", () => ({ + prisma: { + // The whole-record gate reads the session's own stamps: a sign-in a + // moment ago is the proof, on an account without a second factor. + session: { + findUnique: vi.fn(async () => ({ + createdAt: new Date(), + mfaVerifiedAt: null, + reproofAt: null, + })), + }, + webauthnMfaCredential: { count: vi.fn(async () => 0) }, + passkey: { count: vi.fn(async () => 0) }, + }, +})); vi.mock("@/lib/rate-limit", () => ({ checkRateLimit: vi.fn() })); vi.mock("@/lib/logging/transports", () => ({ emitIfSampled: vi.fn() })); vi.mock("@/lib/db-compat", () => ({ @@ -39,6 +54,7 @@ vi.mock("next/headers", () => ({ import { GET as metadataGet } from "../metadata/route"; import { GET as everythingGet } from "../Patient/$everything/route"; +import { prisma } from "@/lib/db"; import { GET as observationRead } from "../Observation/[id]/route"; import { GET as patientRead } from "../Patient/[id]/route"; import { GET as coverageRead } from "../Coverage/[id]/route"; @@ -291,6 +307,17 @@ describe("GET /api/fhir/{type}/{id} — read", () => { }); describe("GET /api/fhir/Patient/$everything", () => { + it("asks a browser session that signed in an hour ago to confirm first", async () => { + vi.mocked(prisma.session.findUnique).mockResolvedValueOnce({ + createdAt: new Date(Date.now() - 60 * 60 * 1000), + mfaVerifiedAt: null, + reproofAt: null, + } as never); + const res = await everythingGet(req("/api/fhir/Patient/$everything")); + expect(res.status).toBe(401); + expect((await res.json()).meta.errorCode).toBe("auth.reproof.required"); + }); + it("returns every resource family the document bundle carries", async () => { const res = await everythingGet( req("/api/fhir/Patient/$everything?_count=200"), diff --git a/src/app/api/fhir/__tests__/module-gate.test.ts b/src/app/api/fhir/__tests__/module-gate.test.ts index 40d328758..eb97a9833 100644 --- a/src/app/api/fhir/__tests__/module-gate.test.ts +++ b/src/app/api/fhir/__tests__/module-gate.test.ts @@ -32,6 +32,17 @@ vi.mock("@/lib/db", () => ({ prisma: { user: { findUnique: vi.fn() }, cycleProfile: { findUnique: vi.fn() }, + // The whole-record gate reads the session's own stamps: a sign-in a + // moment ago is the proof, on an account without a second factor. + session: { + findUnique: vi.fn(async () => ({ + createdAt: new Date(), + mfaVerifiedAt: null, + reproofAt: null, + })), + }, + webauthnMfaCredential: { count: vi.fn(async () => 0) }, + passkey: { count: vi.fn(async () => 0) }, }, })); vi.mock("@/lib/modules/operator-availability", async (importOriginal) => { diff --git a/src/components/settings/report-selection/selection-panel.tsx b/src/components/settings/report-selection/selection-panel.tsx index 3f589a20f..2f9f1875c 100644 --- a/src/components/settings/report-selection/selection-panel.tsx +++ b/src/components/settings/report-selection/selection-panel.tsx @@ -48,6 +48,11 @@ import { ReportVisitOffer } from "./report-visit-offer"; import { Switch } from "@/components/ui/switch"; import { useRovingRadioGroup } from "@/hooks/use-roving-radio-group"; import { apiFetchRaw } from "@/lib/api/api-fetch"; +import { + recentProofErrorMessage, + throwIfReproofRequired, + useRecentProof, +} from "@/components/settings/security-section/use-recent-proof"; import { useAuth } from "@/hooks/use-auth"; import { useTranslations } from "@/lib/i18n/context"; import { queryKeys } from "@/lib/query-keys"; @@ -67,6 +72,9 @@ type ExportFormat = "pdf" | "fhir" | "package"; const EXPORT_FORMATS: readonly ExportFormat[] = ["pdf", "fhir", "package"]; const PRESET_RANGES = [30, 90, 180, 365] as const; +/** A refusal already worded for the panel; shown as it is. */ +class ReportRequestError extends Error {} + export function HealthRecordExportPanel() { const { t, locale } = useTranslations(); const { user } = useAuth(); @@ -119,18 +127,40 @@ export function HealthRecordExportPanel() { onSelect: (index) => setFormat(EXPORT_FORMATS[index]!), }); + // The report is built from the whole record, so it asks for a fresh proof + // unless the session signed in or re-proved within five minutes. + const recentProof = useRecentProof(); + async function handleGenerate() { setBusy(true); setError(null); try { - const range = - customRange && startDate && endDate - ? { - startDate: new Date(`${startDate}T00:00:00Z`).toISOString(), - endDate: new Date(`${endDate}T23:59:59Z`).toISOString(), - } - : { days }; - const res = await apiFetchRaw("/api/export/health-record", { + await recentProof.run(generateOnce); + } catch (err) { + if (err instanceof ReportRequestError) { + setError(err.message); + return; + } + const message = recentProofErrorMessage( + err, + err instanceof Error ? err.message : String(err), + ); + if (message) setError(message); + } finally { + setBusy(false); + } + } + + async function generateOnce() { + const range = + customRange && startDate && endDate + ? { + startDate: new Date(`${startDate}T00:00:00Z`).toISOString(), + endDate: new Date(`${endDate}T23:59:59Z`).toISOString(), + } + : { days }; + const res = await throwIfReproofRequired( + await apiFetchRaw("/api/export/health-record", { method: "POST", headers: { "Content-Type": "application/json" }, credentials: "include", @@ -144,38 +174,33 @@ export function HealthRecordExportPanel() { includeCharts, selection: { v: 2, leaves: orderLeaves(selected) }, }), - }); - if (!res.ok) { - setError( - res.status === 429 - ? t("settings.healthRecord.errorRateLimit") - : res.status === 403 - ? t("settings.healthRecord.errorModuleDisabled") - : t("settings.healthRecord.errorGeneric"), - ); - return; - } - const blob = await res.blob(); - const ext = format === "pdf" ? "pdf" : format === "fhir" ? "json" : "zip"; - const url = URL.createObjectURL(blob); - const a = document.createElement("a"); - a.href = url; - a.download = `healthlog-health-record-${new Date() - .toISOString() - .slice(0, 10)}.${ext}`; - document.body.appendChild(a); - a.click(); - a.remove(); - URL.revokeObjectURL(url); - // The route just persisted the practice name and the selection, so the - // cached `/me` payload is stale. Refresh it, or a remount would seed from - // the previous values. - void queryClient.invalidateQueries({ queryKey: queryKeys.authMe() }); - } catch (err) { - setError(err instanceof Error ? err.message : String(err)); - } finally { - setBusy(false); + }), + ); + if (!res.ok) { + throw new ReportRequestError( + res.status === 429 + ? t("settings.healthRecord.errorRateLimit") + : res.status === 403 + ? t("settings.healthRecord.errorModuleDisabled") + : t("settings.healthRecord.errorGeneric"), + ); } + const blob = await res.blob(); + const ext = format === "pdf" ? "pdf" : format === "fhir" ? "json" : "zip"; + const url = URL.createObjectURL(blob); + const a = document.createElement("a"); + a.href = url; + a.download = `healthlog-health-record-${new Date() + .toISOString() + .slice(0, 10)}.${ext}`; + document.body.appendChild(a); + a.click(); + a.remove(); + URL.revokeObjectURL(url); + // The route just persisted the practice name and the selection, so the + // cached `/me` payload is stale. Refresh it, or a remount would seed from + // the previous values. + void queryClient.invalidateQueries({ queryKey: queryKeys.authMe() }); } return ( @@ -397,6 +422,7 @@ export function HealthRecordExportPanel() {

)} + {recentProof.dialog} ); } diff --git a/src/lib/openapi/routes/auth.ts b/src/lib/openapi/routes/auth.ts index 4a43548a5..c26652e3b 100644 --- a/src/lib/openapi/routes/auth.ts +++ b/src/lib/openapi/routes/auth.ts @@ -332,7 +332,7 @@ const MFA_MANAGEMENT_AUTH_NOTE = " Accepts a cookie session, or a Bearer token presenting a single-use elevation from POST /api/auth/step-up in the `X-Step-Up` header; a Bearer token alone is still refused."; const ENROLLMENT_PROOF_NOTE = - "\n\nAdding a factor needs a fresh proof, not just a session. On the Bearer path the elevation is that proof (a password-proved one is enough). On the cookie path the request passes when the session signed in, or completed a second factor, within the last five minutes; otherwise the body must carry a proof in the shape POST /api/auth/step-up takes (`password`, a `totp` code from an already confirmed authenticator, or a passkey / security-key assertion begun at POST /api/auth/passkey/register-options). Without one the answer is 401 `auth.reproof.required`; a proof that does not verify is 401 `auth.reproof.failed` and is audited. Proofs draw on the account's shared re-proof budget of five per fifteen minutes (429 once spent), the same budget as the step-up mint."; + "\n\nAdding a factor needs a fresh proof, not just a session. On the Bearer path the elevation is that proof: a password-proved one is enough on an account without a second factor, and on an account that already has one the elevation must come from that factor or a passkey. On the cookie path, for an account without a second factor, the request passes when the session signed in, or completed a passkey sign-in, within the last five minutes; otherwise the body must carry a proof in the shape POST /api/auth/step-up takes (`password`, or a passkey assertion begun at POST /api/auth/passkey/register-options). For an account that already has a second factor, only that factor counts: a second factor completed within the last five minutes, or a `totp` code, security-key or passkey assertion in the body. A password or a young session (for example one that skipped the factor on a remembered browser) is not enough there. Without one the answer is 401 `auth.reproof.required` with `meta.methods` naming the proofs the account can give; a proof that does not verify is 401 `auth.reproof.failed` and is audited. Proofs draw on the account's shared re-proof budget of five per fifteen minutes (429 once spent), the same budget as the step-up mint."; const enrollmentProofResponses = { "401": { @@ -2023,7 +2023,7 @@ export const authPaths: NonNullable = { description: "Cookie-only — a Bearer token cannot enroll a sign-in credential on this surface at any scope. The route has TWO arms and the body decides which.\n\n" + '**Re-proof arm.** A body of exactly `{ method: "passkey" }` or `{ method: "webauthn" }` begins an assertion ceremony WITHOUT beginning enrollment, and answers with `reauth: true` alongside the options. Use it to obtain the assertion the second arm needs.\n\n' + - "**Enrollment arm.** Any other body must be a full factor proof (the same shape POST /api/auth/step-up takes): a password, a TOTP code, or a completed passkey / security-key assertion. Only then are registration options issued.\n\n" + + "**Enrollment arm.** Any other body must be a full factor proof (the same shape POST /api/auth/step-up takes): a password, a TOTP code, or a completed passkey / security-key assertion. Only then are registration options issued. On an account that already has a second factor a password is refused before it is checked, with 422 `auth.reproof.too_weak` and `meta.methods` naming the proofs that work: a passkey added on the password alone would sign in without the second factor from then on.\n\n" + "Which factor you prove changes the session, and the direction is not the obvious one: a strong proof stamps the session second-factor-verified, while a PASSWORD proof deliberately CLEARS that stamp. Registering with a password is authorised by the single-use, session-bound challenge alone and must not silently upgrade password-only authentication into something the rest of the app reads as a second factor.\n\n" + "Every refusal on the proof path is the same 401 with the same prose — a missing content type, an unparseable body, a body that is not a valid proof shape, and a wrong password are indistinguishable to the caller. A `totp` proof counts only from an authenticator whose enrollment was confirmed.\n\n" + "Each proof on the enrollment arm draws on the account's shared re-proof budget (five per fifteen minutes, the same budget as POST /api/auth/step-up), and every failed proof writes an `auth.mfa.failed` audit row. Once the budget is spent the answer is 429 until the window passes, even for a correct proof.", @@ -2060,6 +2060,11 @@ export const authPaths: NonNullable = { content: { "application/json": { schema: errorEnvelope } }, }, ...stdResponses, + "422": { + description: + "`auth.reproof.too_weak`: a password on an account that already has a second factor. `meta.methods` names the proofs that would be accepted. Refused before the password is checked, so it spends no attempt.", + content: { "application/json": { schema: errorEnvelope } }, + }, "401": { description: "No cookie session, or no acceptable existing-factor proof. One message covers every proof failure so the response cannot be used to probe which part was wrong.", diff --git a/src/lib/openapi/routes/export.ts b/src/lib/openapi/routes/export.ts index 9838e73fc..a19a5d09c 100644 --- a/src/lib/openapi/routes/export.ts +++ b/src/lib/openapi/routes/export.ts @@ -168,7 +168,7 @@ export const exportPaths: NonNullable = { tags: ["Export"], summary: "Download measurements, medications, intake and mood", description: - "The plaintext download. Returns `application/json` or `text/csv` (never the standard response envelope) with a `Content-Disposition: attachment` filename of `healthlog-export-.`. Measurements stream page by page, so a multi-year account does not buffer its whole history in memory; the other sections are assembled in full first. The CSV variant concatenates the selected sections under `# Measurements` / `# Medications` / `# Intake Events` / `# Mood Entries` headings, so it is a CONCATENATION of tables rather than one table — a single-table CSV parser will choke on it. Mood notes are decrypted into the download; measurement notes ride their exported column. Shares the `export:` bucket with the encrypted variant (10 per hour) and audits every attempt as `export.download`. Cookie or Bearer auth; the caller is always resolved as themselves, so this download cannot be delegated to a shared record. Not module-gated: a disabled module's rows are still exported, because this is the account's own data leaving on request.", + "The plaintext download. Returns `application/json` or `text/csv` (never the standard response envelope) with a `Content-Disposition: attachment` filename of `healthlog-export-.`. Measurements stream page by page, so a multi-year account does not buffer its whole history in memory; the other sections are assembled in full first. The CSV variant concatenates the selected sections under `# Measurements` / `# Medications` / `# Intake Events` / `# Mood Entries` headings, so it is a CONCATENATION of tables rather than one table — a single-table CSV parser will choke on it. Mood notes are decrypted into the download; measurement notes ride their exported column. Shares the `export:` bucket with the encrypted variant (10 per hour) and audits every attempt as `export.download`. Cookie or Bearer auth; the caller is always resolved as themselves, so this download cannot be delegated to a shared record. Not module-gated: a disabled module's rows are still exported, because this is the account's own data leaving on request. `type=all`, the default, is the whole record and asks for a fresh proof like the full backup; the single-type exports do not. A browser session needs a fresh proof as well as a live one: a sign-in or `POST /api/auth/reproof` within the last five minutes (with a second factor, on an account that has one). Without it the answer is 401 `auth.reproof.required`, with `meta.methods` naming the proofs the account can give. A Bearer caller asking for `type=all` presents an `X-Step-Up` elevation from `POST /api/auth/step-up` (a second-factor one on an account with a second factor) and receives 401 `auth.stepup.required` without it.", requestParams: { query: exportQuery }, responses: { "200": { diff --git a/src/lib/openapi/routes/health-record.ts b/src/lib/openapi/routes/health-record.ts index 6ffd297a5..43845b213 100644 --- a/src/lib/openapi/routes/health-record.ts +++ b/src/lib/openapi/routes/health-record.ts @@ -113,7 +113,7 @@ export const healthRecordPaths: NonNullable = { tags: ["Export"], summary: "Generate a health-record export (PDF / FHIR / package)", description: - "Returns the doctor-handover artefact in the requested `format`: `pdf` → application/pdf, `fhir` → application/fhir+json (HL7 FHIR R4 document Bundle), `package` → application/zip (PDF + FHIR + README). `selection` is REQUIRED and carries the leaf inclusion list — membership is inclusion, absence is exclusion, and there is no server-side default. An unknown leaf id is a 422 that names it; the retired grouped `sections` shape is a 422 too, because accepting it would mean choosing a scope the caller did not express. Auth via cookie or Bearer; shared `export:` rate bucket (10/h). Strict validation: unknown keys 422.", + "Returns the doctor-handover artefact in the requested `format`: `pdf` → application/pdf, `fhir` → application/fhir+json (HL7 FHIR R4 document Bundle), `package` → application/zip (PDF + FHIR + README). `selection` is REQUIRED and carries the leaf inclusion list — membership is inclusion, absence is exclusion, and there is no server-side default. An unknown leaf id is a 422 that names it; the retired grouped `sections` shape is a 422 too, because accepting it would mean choosing a scope the caller did not express. Auth via cookie or Bearer; shared `export:` rate bucket (10/h). Strict validation: unknown keys 422. A browser session needs a fresh proof as well as a live one: a sign-in or `POST /api/auth/reproof` within the last five minutes (with a second factor, on an account that has one). Without it the answer is 401 `auth.reproof.required`, with `meta.methods` naming the proofs the account can give. When a manager exports a record they manage, it is their own session that is asked. A Bearer caller is not asked for more than its token.", requestBody: { required: true, content: { @@ -647,7 +647,7 @@ export const healthRecordPaths: NonNullable = { tags: ["FHIR"], summary: "FHIR R4 $everything (v1.11.0)", description: - "Read-only `Patient/$everything`: every resource the document bundle carries — Composition, Device, Patient, Coverage, Observations (including the cycle ones), MedicationStatements, MedicationAdministrations, Conditions, Encounters, AllergyIntolerances, FamilyMemberHistories and the DiagnosticReport (present when the window holds a vital sign) — flattened into one `searchset` Bundle. Scoped to the owner's SAVED report selection; an account that never saved one gets nothing. Auth: `fhir:read` scope. Offset paging via `_count` (≤200) / `_offset`.", + "Read-only `Patient/$everything`: every resource the document bundle carries — Composition, Device, Patient, Coverage, Observations (including the cycle ones), MedicationStatements, MedicationAdministrations, Conditions, Encounters, AllergyIntolerances, FamilyMemberHistories and the DiagnosticReport (present when the window holds a vital sign) — flattened into one `searchset` Bundle. Scoped to the owner's SAVED report selection; an account that never saved one gets nothing. Auth: `fhir:read` scope. Offset paging via `_count` (≤200) / `_offset`. A browser session needs a fresh proof as well as a live one: a sign-in or `POST /api/auth/reproof` within the last five minutes (with a second factor, on an account that has one). Without it the answer is 401 `auth.reproof.required`, with `meta.methods` naming the proofs the account can give. A token, including a `fhir:read` one, is not asked for more.", requestParams: { query: z.object({ _count: z.coerce.number().optional(), From a294d7f684b42d52f3effe8d454f9945ba5be733 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Bombeck?= Date: Sat, 26 Sep 2026 22:34:21 +0200 Subject: [PATCH 3/9] fix(sharing): invitations need a fresh proof and end with sign out everywhere Offering access to the record took a live session alone, and an invitation outlives the session that sent it: whoever held a stolen session could invite their own account and keep a way into the record after the owner signed everything out. POST /api/account/grants now takes the recent-proof gate at every level (a sign-in or re-proof within five minutes on the browser, with the second factor on an account that has one; a step-up elevation on a token, which no shipped client uses here), and the invite form opens the re-proof dialog and sends again. Sign out everywhere now withdraws every invitation the account offered that nobody has accepted. Accepted grants are left standing on purpose: each is a person the owner chose, often a carer who still needs the record, and a sign-out button must not quietly cut them off. The response lists them as grantsKept and the sessions card shows each with a one-click End access. --- docs/api/openapi.yaml | 64 ++++++++- messages/de.json | 6 +- messages/en.json | 6 +- messages/es.json | 6 +- messages/fr.json | 6 +- messages/it.json | 6 +- messages/ko.json | 6 +- messages/pl.json | 6 +- .../recent-proof-surface-guard.test.ts | 2 + .../__tests__/invite-access-level.test.ts | 10 +- .../__tests__/invite-recent-proof.test.ts | 114 ++++++++++++++++ .../__tests__/invite-scope-and-manage.test.ts | 10 +- src/app/api/account/grants/route.ts | 22 +++- .../auth/me/sessions/__tests__/route.test.ts | 50 +++++++ src/app/api/auth/me/sessions/route.ts | 34 ++++- .../__tests__/grant-invite-expiry.test.tsx | 14 +- .../settings/access/grant-invite-card.tsx | 57 +++++--- .../settings/security-sessions-card.tsx | 124 +++++++++++++++--- src/lib/auth/session.ts | 19 +++ src/lib/openapi/routes/account-sharing.ts | 4 +- src/lib/openapi/routes/auth.ts | 25 +++- .../sessions-and-known-devices.test.ts | 59 +++++++++ 22 files changed, 585 insertions(+), 65 deletions(-) create mode 100644 src/app/api/account/grants/__tests__/invite-recent-proof.test.ts diff --git a/docs/api/openapi.yaml b/docs/api/openapi.yaml index 0b8a74389..fbe1426cd 100644 --- a/docs/api/openapi.yaml +++ b/docs/api/openapi.yaml @@ -2621,7 +2621,10 @@ paths: (the OAuth connection, so it cannot mint a new access token), every programmatic token (connector, measurement and document tokens), and every clinician share link. Share links are revoked by default because a link made by whoever held a lost session or device would otherwise keep opening the record; send `keepShareLinks=1` to keep - them. A Bearer caller keeps its own device login." + them. Record-sharing invitations this account offered that nobody has accepted yet are withdrawn too, because an + invitation sent from a stolen session would otherwise wait for the thief's own account. Accepted grants are not + ended: the response lists them as `grantsKept` so the client can show who can still read the record and offer to + end each one. A Bearer caller keeps its own device login." parameters: - in: query name: keepShareLinks @@ -19394,7 +19397,12 @@ paths: (`requireFreshMfaIfEnrolled`, cookie-only): an enrolled account without a fresh factor gets 401 `auth.stepup.required`, and a Bearer caller gets 403 `sharing.invite.manage_browser_only` before anything else happens, because the gate it would hit resolves through the session cookie and would otherwise answer 'not - authenticated' to a caller it had just authenticated." + authenticated' to a caller it had just authenticated. Every invitation, at any level, also needs a fresh proof: + a browser session must have signed in or used `POST /api/auth/reproof` within the last five minutes (with the + second factor, on an account that has one), or it gets 401 `auth.reproof.required` with `meta.methods`; a Bearer + caller presents an `X-Step-Up` elevation from `POST /api/auth/step-up` (a second-factor one on an account with a + second factor) and gets 401 `auth.stepup.required` without it. `DELETE /api/auth/me/sessions` withdraws the + invitations nobody has accepted yet." requestBody: required: true content: @@ -19409,8 +19417,10 @@ paths: schema: $ref: "#/components/schemas/AccountGrantEnvelope" "401": - description: "Offering MANAGE from an MFA-enrolled account without a recent second-factor proof (`meta.errorCode: - auth.stepup.required`). Re-prove the factor and retry; READ and WRITE invitations are never gated." + description: "No recent proof. A browser session that has not signed in or re-proved within five minutes gets + `meta.errorCode: auth.reproof.required` with `meta.methods`; re-prove at POST /api/auth/reproof and retry. A + Bearer caller without a valid `X-Step-Up` elevation gets `auth.stepup.required`, as does offering MANAGE + from an MFA-enrolled browser session without a recent second-factor proof." content: application/json: schema: @@ -39023,11 +39033,57 @@ components: minimum: -9007199254740991 maximum: 9007199254740991 description: Clinician share links revoked; 0 when `keepShareLinks=1` was sent. + pendingInvitesRevoked: + type: integer + minimum: -9007199254740991 + maximum: 9007199254740991 + description: "Record-sharing invitations this account offered that nobody had accepted yet, now withdrawn (`revokedBy: + GRANTOR`)." + grantsKept: + type: array + items: + type: object + properties: + id: + type: string + description: The grant id, for DELETE /api/account/grants/{id}. + account: + type: object + properties: + id: + type: string + username: + type: string + displayName: + anyOf: + - type: string + - type: "null" + required: + - id + - username + - displayName + additionalProperties: false + access: + type: string + enum: + - READ + - WRITE + - MANAGE + required: + - id + - account + - access + additionalProperties: false + description: Accepted grants this account gave that are still live. Signing out does not end them, because each is a + person the owner chose and ending one silently could cut off a carer; they are listed so a client can show + who can still read the record and offer to end each with DELETE /api/account/grants/{id}. required: - sessionsRevoked - accessTokensRevoked - connectorsRevoked - shareLinksRevoked + - pendingInvitesRevoked + - grantsKept additionalProperties: false SessionRevokeEnvelope: type: object diff --git a/messages/de.json b/messages/de.json index 8b6523d6b..8e7c9e885 100644 --- a/messages/de.json +++ b/messages/de.json @@ -6556,7 +6556,11 @@ "revokeSessionConfirmBody": "{device} muss sich neu anmelden. An den dort erfassten Daten ändert sich nichts.", "signOutEverywhereConfirmTitle": "Alle anderen Geräte abmelden?", "signOutEverywhereConfirmBody": "Alle anderen Geräte, jeder verbundene KI-Assistent und jedes API-Token müssen sich neu anmelden oder neu eingerichtet werden. Dieses Gerät bleibt angemeldet. Deine Einträge bleiben unberührt.", - "signOutEverywhereShareLinks": "Auch die Freigabelinks für Ärztinnen und Ärzte beenden" + "signOutEverywhereShareLinks": "Auch die Freigabelinks für Ärztinnen und Ärzte beenden", + "signOutEverywhereInvites": "Einladungen zu deiner Akte, die noch niemand angenommen hat, werden zurückgezogen. Wer schon angenommen hat, behält den Zugriff; du kannst ihn gleich danach beenden.", + "signOutEverywhereInvitesDone": "{count} nicht angenommene Einladung(en) zurückgezogen.", + "grantsKeptTitle": "Diese Personen sehen deine Akte weiterhin", + "grantsKeptBody": "Das Abmelden beendet keinen Zugriff, den du jemandem gegeben hast. Beende ihn hier, wenn du ihn nicht gegeben hast oder nicht mehr willst." }, "privacy": { "encryption": { diff --git a/messages/en.json b/messages/en.json index 8eeaf9ad7..5fb9c4cf2 100644 --- a/messages/en.json +++ b/messages/en.json @@ -6556,7 +6556,11 @@ "revokeSessionConfirmBody": "{device} will need to sign in again. Nothing recorded on it is affected.", "signOutEverywhereConfirmTitle": "Sign out every other device?", "signOutEverywhereConfirmBody": "Every other device, every connected AI assistant and every API token will need to sign in or be set up again. This device stays signed in. Nothing you have recorded is affected.", - "signOutEverywhereShareLinks": "Also end the doctor share links" + "signOutEverywhereShareLinks": "Also end the doctor share links", + "signOutEverywhereInvites": "Invitations to your record that nobody has accepted yet are withdrawn. People who already accepted keep their access; you can end it next.", + "signOutEverywhereInvitesDone": "Withdrew {count} invitation(s) nobody had accepted.", + "grantsKeptTitle": "These people can still see your record", + "grantsKeptBody": "Signing out does not end access you gave someone. End it here if you did not give it, or no longer want them to have it." }, "privacy": { "encryption": { diff --git a/messages/es.json b/messages/es.json index 193dd1fea..3caf7e3d1 100644 --- a/messages/es.json +++ b/messages/es.json @@ -6556,7 +6556,11 @@ "revokeSessionConfirmBody": "{device} tendrá que iniciar sesión de nuevo. Lo registrado en él no se ve afectado.", "signOutEverywhereConfirmTitle": "¿Cerrar la sesión en los demás dispositivos?", "signOutEverywhereConfirmBody": "Todos los demás dispositivos, cada asistente de IA conectado y cada token de API tendrán que volver a iniciar sesión o configurarse de nuevo. Este dispositivo sigue con la sesión iniciada. Nada de lo que has registrado se ve afectado.", - "signOutEverywhereShareLinks": "Terminar también los enlaces compartidos con profesionales médicos" + "signOutEverywhereShareLinks": "Terminar también los enlaces compartidos con profesionales médicos", + "signOutEverywhereInvites": "Se retiran las invitaciones a tu historial que nadie ha aceptado todavía. Quien ya aceptó conserva el acceso; puedes quitárselo a continuación.", + "signOutEverywhereInvitesDone": "Se retiraron {count} invitación(es) que nadie había aceptado.", + "grantsKeptTitle": "Estas personas todavía pueden ver tu historial", + "grantsKeptBody": "Cerrar sesión no quita el acceso que diste a alguien. Quítalo aquí si no lo diste o si ya no quieres que lo tenga." }, "privacy": { "encryption": { diff --git a/messages/fr.json b/messages/fr.json index 2fe9be017..f362b2bae 100644 --- a/messages/fr.json +++ b/messages/fr.json @@ -6556,7 +6556,11 @@ "revokeSessionConfirmBody": "{device} devra se reconnecter. Ce qui y est enregistré n'est pas affecté.", "signOutEverywhereConfirmTitle": "Déconnecter tous les autres appareils ?", "signOutEverywhereConfirmBody": "Tous les autres appareils, chaque assistant IA connecté et chaque jeton d'API devront se reconnecter ou être reconfigurés. Cet appareil reste connecté. Rien de ce que vous avez enregistré n'est touché.", - "signOutEverywhereShareLinks": "Mettre fin aussi aux liens de partage avec les médecins" + "signOutEverywhereShareLinks": "Mettre fin aussi aux liens de partage avec les médecins", + "signOutEverywhereInvites": "Les invitations à votre dossier que personne n'a encore acceptées sont retirées. Les personnes qui ont déjà accepté gardent leur accès ; vous pourrez y mettre fin ensuite.", + "signOutEverywhereInvitesDone": "{count} invitation(s) non acceptée(s) retirée(s).", + "grantsKeptTitle": "Ces personnes peuvent toujours voir votre dossier", + "grantsKeptBody": "Se déconnecter ne met pas fin à l'accès que vous avez donné à quelqu'un. Mettez-y fin ici si vous ne l'avez pas donné ou si vous ne le souhaitez plus." }, "privacy": { "encryption": { diff --git a/messages/it.json b/messages/it.json index 4f527a583..404551bb8 100644 --- a/messages/it.json +++ b/messages/it.json @@ -6556,7 +6556,11 @@ "revokeSessionConfirmBody": "{device} dovrà accedere di nuovo. Quanto registrato su di esso non viene toccato.", "signOutEverywhereConfirmTitle": "Disconnettere tutti gli altri dispositivi?", "signOutEverywhereConfirmBody": "Tutti gli altri dispositivi, ogni assistente IA collegato e ogni token API dovranno accedere di nuovo o essere configurati di nuovo. Questo dispositivo resta connesso. Nulla di ciò che hai registrato viene toccato.", - "signOutEverywhereShareLinks": "Termina anche i link di condivisione con i medici" + "signOutEverywhereShareLinks": "Termina anche i link di condivisione con i medici", + "signOutEverywhereInvites": "Gli inviti alla tua cartella che nessuno ha ancora accettato vengono ritirati. Chi ha già accettato mantiene l'accesso; puoi revocarlo subito dopo.", + "signOutEverywhereInvitesDone": "Ritirati {count} inviti non accettati.", + "grantsKeptTitle": "Queste persone possono ancora vedere la tua cartella", + "grantsKeptBody": "Disconnettersi non revoca l'accesso che hai dato a qualcuno. Revocalo qui se non l'hai dato tu o se non vuoi più che lo abbia." }, "privacy": { "encryption": { diff --git a/messages/ko.json b/messages/ko.json index 5e80dc3b9..85af2c610 100644 --- a/messages/ko.json +++ b/messages/ko.json @@ -6556,7 +6556,11 @@ "revokeSessionConfirmBody": "{device} — 다시 로그인해야 해요. 기기에 기록된 내용은 영향받지 않아요.", "signOutEverywhereConfirmTitle": "다른 기기를 모두 로그아웃할까요?", "signOutEverywhereConfirmBody": "다른 모든 기기, 연결된 모든 AI 어시스턴트와 모든 API 토큰은 다시 로그인하거나 다시 설정해야 합니다. 이 기기는 로그인 상태로 유지됩니다. 기록한 내용에는 영향이 없습니다.", - "signOutEverywhereShareLinks": "의료진 공유 링크도 종료" + "signOutEverywhereShareLinks": "의료진 공유 링크도 종료", + "signOutEverywhereInvites": "아직 아무도 수락하지 않은 기록 공유 초대는 철회됩니다. 이미 수락한 사람은 접근 권한을 유지하며, 이어서 끝낼 수 있어요.", + "signOutEverywhereInvitesDone": "수락되지 않은 초대 {count}개를 철회했어요.", + "grantsKeptTitle": "이 사람들은 여전히 내 기록을 볼 수 있어요", + "grantsKeptBody": "로그아웃해도 다른 사람에게 준 접근 권한은 끝나지 않아요. 직접 준 적이 없거나 더 이상 원하지 않으면 여기에서 끝내세요." }, "privacy": { "encryption": { diff --git a/messages/pl.json b/messages/pl.json index f04de5400..541ad9267 100644 --- a/messages/pl.json +++ b/messages/pl.json @@ -6556,7 +6556,11 @@ "revokeSessionConfirmBody": "{device} będzie musiało zalogować się ponownie. Zapisane na nim dane pozostają bez zmian.", "signOutEverywhereConfirmTitle": "Wylogować wszystkie pozostałe urządzenia?", "signOutEverywhereConfirmBody": "Wszystkie inne urządzenia, każdy połączony asystent AI i każdy token API będą musiały zalogować się ponownie lub zostać skonfigurowane od nowa. To urządzenie pozostaje zalogowane. Zapisane dane pozostają bez zmian.", - "signOutEverywhereShareLinks": "Zakończ także linki udostępniania dla lekarzy" + "signOutEverywhereShareLinks": "Zakończ także linki udostępniania dla lekarzy", + "signOutEverywhereInvites": "Zaproszenia do Twojej karty, których nikt jeszcze nie przyjął, zostaną wycofane. Osoby, które już je przyjęły, zachowują dostęp; możesz go zakończyć zaraz potem.", + "signOutEverywhereInvitesDone": "Wycofano nieprzyjęte zaproszenia: {count}.", + "grantsKeptTitle": "Te osoby nadal widzą Twoją kartę", + "grantsKeptBody": "Wylogowanie nie kończy dostępu, który komuś dałeś. Zakończ go tutaj, jeśli go nie dawałeś albo już nie chcesz, żeby go miał." }, "privacy": { "encryption": { diff --git a/src/__tests__/recent-proof-surface-guard.test.ts b/src/__tests__/recent-proof-surface-guard.test.ts index 9a1e0f8ae..cb6bb3878 100644 --- a/src/__tests__/recent-proof-surface-guard.test.ts +++ b/src/__tests__/recent-proof-surface-guard.test.ts @@ -73,6 +73,8 @@ function callers(exportName: string): string[] { /** Routes that resolve their own caller and take the recent-proof gate. */ const RECENT_PROOF_ROUTES: Record = { + // Offering access to the record. No shipped client calls it on a token. + "app/api/account/grants/route.ts": "elevation", // `type=all` only; the single-type exports stay on `requireAuth`. "app/api/export/route.ts": "elevation", "app/api/export/full-backup/route.ts": "elevation", diff --git a/src/app/api/account/grants/__tests__/invite-access-level.test.ts b/src/app/api/account/grants/__tests__/invite-access-level.test.ts index 3798cc52f..431ad44e0 100644 --- a/src/app/api/account/grants/__tests__/invite-access-level.test.ts +++ b/src/app/api/account/grants/__tests__/invite-access-level.test.ts @@ -27,11 +27,19 @@ vi.mock("@/lib/api-handler", async () => { await vi.importActual( "@/lib/api-handler", ); + const requireAuth = vi.fn(); return { ...actual, apiHandler: Promise>(h: T): T => h, - requireAuth: vi.fn(), + requireAuth, + // The invitation takes the fresh-proof gate; here it resolves the caller + // the stubbed `requireAuth` names, so these cases stay about the row. + // The gate itself has its own case in `invite-recent-proof.test.ts`. + requireRecentProof: vi.fn(async () => ({ + ...(await requireAuth()), + commitElevation: async () => {}, + })), }; }); diff --git a/src/app/api/account/grants/__tests__/invite-recent-proof.test.ts b/src/app/api/account/grants/__tests__/invite-recent-proof.test.ts new file mode 100644 index 000000000..e4c987074 --- /dev/null +++ b/src/app/api/account/grants/__tests__/invite-recent-proof.test.ts @@ -0,0 +1,114 @@ +/** + * Offering access to the record asks for a fresh proof. + * + * An invitation outlives the session that sent it, so a live session alone + * was enough for a stolen one to leave a standing way into the record in the + * thief's own account. The real gate runs here; only the session, the + * database and the headers are stubbed. + */ +import { beforeEach, describe, expect, it, vi } from "vitest"; +import { NextRequest } from "next/server"; + +vi.mock("@/lib/db", () => ({ + prisma: { + session: { findUnique: vi.fn() }, + webauthnMfaCredential: { count: vi.fn(async () => 0) }, + passkey: { count: vi.fn(async () => 0) }, + user: { findFirst: vi.fn() }, + accountGrant: { create: vi.fn(), findFirst: vi.fn() }, + }, +})); +vi.mock("@/lib/auth/session", () => ({ getSession: vi.fn() })); +vi.mock("@/lib/rate-limit", () => ({ + checkRateLimit: vi.fn(async () => ({ allowed: true })), +})); +vi.mock("@/lib/auth/audit", () => ({ + auditLog: vi.fn().mockResolvedValue(undefined), + recordDelegatedAccess: vi.fn(), +})); +vi.mock("@/lib/logging/transports", () => ({ emitIfSampled: vi.fn() })); +vi.mock("@/lib/sharing/grants", async (importOriginal) => ({ + ...(await importOriginal()), + inviteGrant: vi.fn(), +})); +vi.mock("next/headers", () => ({ + headers: vi.fn(async () => ({ get: () => null })), + cookies: vi.fn(async () => ({ + get: () => undefined, + set: () => {}, + delete: () => {}, + })), +})); + +import { POST } from "../route"; +import { getSession } from "@/lib/auth/session"; +import { prisma } from "@/lib/db"; +import { inviteGrant } from "@/lib/sharing/grants"; + +const OWNER = { + id: "owner-1", + role: "USER", + username: "owner", + totpConfirmedAt: null, + passwordHash: "argon", +}; + +function invite(): NextRequest { + return new NextRequest("http://localhost/api/account/grants", { + method: "POST", + headers: { "content-type": "application/json" }, + body: JSON.stringify({ identifier: "housemate", access: "READ" }), + }); +} + +function signedIn(createdAt: Date) { + vi.mocked(getSession).mockResolvedValue({ + session: { id: "sess-1", expiresAt: new Date(Date.now() + 1e6) }, + user: OWNER, + } as never); + vi.mocked(prisma.session.findUnique).mockResolvedValue({ + createdAt, + mfaVerifiedAt: null, + reproofAt: null, + } as never); +} + +beforeEach(() => { + vi.clearAllMocks(); +}); + +describe("POST /api/account/grants — fresh proof", () => { + it("a session that signed in an hour ago is asked to confirm, and nothing is offered", async () => { + signedIn(new Date(Date.now() - 60 * 60 * 1000)); + const res = await POST(invite()); + expect(res.status).toBe(401); + const body = await res.json(); + expect(body.meta.errorCode).toBe("auth.reproof.required"); + expect(body.meta.methods).toEqual(["password"]); + expect(prisma.user.findFirst).not.toHaveBeenCalled(); + expect(inviteGrant).not.toHaveBeenCalled(); + }); + + it("a session that signed in a minute ago offers the invitation", async () => { + signedIn(new Date(Date.now() - 60 * 1000)); + vi.mocked(prisma.user.findFirst).mockResolvedValue({ + id: "invitee-1", + username: "housemate", + displayName: null, + } as never); + vi.mocked(inviteGrant).mockResolvedValue({ + id: "grant-1", + access: "READ", + scopeJson: null, + invitedAt: new Date(), + acceptedAt: null, + expiresAt: null, + lastUsedAt: null, + revokedAt: null, + revokedBy: null, + } as never); + const res = await POST(invite()); + expect(res.status).toBe(201); + expect(inviteGrant).toHaveBeenCalled(); + }); +}); diff --git a/src/app/api/account/grants/__tests__/invite-scope-and-manage.test.ts b/src/app/api/account/grants/__tests__/invite-scope-and-manage.test.ts index 265283687..1d79bad00 100644 --- a/src/app/api/account/grants/__tests__/invite-scope-and-manage.test.ts +++ b/src/app/api/account/grants/__tests__/invite-scope-and-manage.test.ts @@ -31,11 +31,19 @@ vi.mock("@/lib/api-handler", async () => { await vi.importActual( "@/lib/api-handler", ); + const requireAuth = vi.fn(); return { ...actual, apiHandler: Promise>(h: T): T => h, - requireAuth: vi.fn(), + requireAuth, + // The invitation takes the fresh-proof gate; here it resolves the caller + // the stubbed `requireAuth` names, so these cases stay about the row. + // The gate itself has its own case in `invite-recent-proof.test.ts`. + requireRecentProof: vi.fn(async () => ({ + ...(await requireAuth()), + commitElevation: async () => {}, + })), requireFreshMfaIfEnrolled: vi.fn(), }; }); diff --git a/src/app/api/account/grants/route.ts b/src/app/api/account/grants/route.ts index 1a0184f55..0cae8e85d 100644 --- a/src/app/api/account/grants/route.ts +++ b/src/app/api/account/grants/route.ts @@ -21,9 +21,15 @@ * a code rather than leaving a token caller with an authentication error on a * request that authenticated fine. * - * **Neither is delegable, and that is structural.** Both resolve through bare - * `requireAuth()`, which refuses outright while the caller is acting on - * another account. So a delegate cannot invite anybody into the record they + * v1.39.3 — offering any level also asks for a fresh proof + * (`requireRecentProof`), and "sign out everywhere" withdraws the invitations + * nobody has accepted yet. An invitation outlives the session that sent it, so + * one offered from a stolen session would otherwise stay live after the owner + * signed everything else out. + * + * **Neither is delegable, and that is structural.** Both resolve through + * `requireAuth()` (the invitation through `requireRecentProof`, which calls + * it), which refuses outright while the caller is acting on another account. So a delegate cannot invite anybody into the record they * were given access to, cannot widen their own grant, and cannot pass it on: * the route does not have a re-delegation check that somebody could soften, it * has no code path that runs under a switch at all. The grantor is then taken @@ -45,6 +51,7 @@ import { apiHandler, requireAuth, requireFreshMfaIfEnrolled, + requireRecentProof, MFA_STEP_UP_MAX_AGE_SECONDS, } from "@/lib/api-handler"; import { @@ -122,7 +129,12 @@ export const GET = apiHandler(async () => { }); export const POST = apiHandler(async (request: NextRequest) => { - const auth = await requireAuth(); + // Offering access leaves a standing way into the record in somebody else's + // hands, one that outlives the session that offered it. So a live session is + // not enough: a sign-in or re-proof within five minutes on the browser, with + // the second factor on an account that has one, or a step-up elevation on a + // token. No shipped client offers access with a token. + const auth = await requireRecentProof({ bearer: "elevation" }); const user = auth.user; const rl = await checkRateLimit( @@ -194,6 +206,8 @@ export const POST = apiHandler(async (request: NextRequest) => { return apiError("No account with that name or e-mail", 404); } + await auth.commitElevation(); + try { // `inviteGrant` decides everything about the row except the level: the // self-grant refusal, and the one-live-grant-per-pair rule the partial diff --git a/src/app/api/auth/me/sessions/__tests__/route.test.ts b/src/app/api/auth/me/sessions/__tests__/route.test.ts index d19634e43..8f90a9de1 100644 --- a/src/app/api/auth/me/sessions/__tests__/route.test.ts +++ b/src/app/api/auth/me/sessions/__tests__/route.test.ts @@ -12,6 +12,7 @@ vi.mock("@/lib/db", () => ({ update: vi.fn(() => ({ catch: () => undefined })), }, user: { findUnique: vi.fn() }, + accountGrant: { findMany: vi.fn(async () => []) }, }, })); @@ -165,6 +166,7 @@ describe("DELETE /api/auth/me/sessions (sign out everywhere)", () => { accessTokensRevoked: 0, connectorsRevoked: 0, shareLinksRevoked: 0, + pendingInvitesRevoked: 0, }); const res = await DELETE(del()); @@ -205,6 +207,7 @@ describe("DELETE /api/auth/me/sessions (sign out everywhere)", () => { accessTokensRevoked: 1, connectorsRevoked: 0, shareLinksRevoked: 0, + pendingInvitesRevoked: 0, }); const res = await DELETE(del()); @@ -219,6 +222,53 @@ describe("DELETE /api/auth/me/sessions (sign out everywhere)", () => { }); }); +describe("sign out everywhere and record sharing", () => { + it("reports withdrawn invitations and lists the accepted grants it kept", async () => { + vi.mocked(getSession).mockResolvedValue(SESSION_OK as never); + vi.mocked(destroyOtherSessions).mockResolvedValue({ + sessionsRevoked: 1, + accessTokensRevoked: 0, + connectorsRevoked: 0, + shareLinksRevoked: 0, + pendingInvitesRevoked: 2, + }); + const party = { id: "user-2", username: "carer", displayName: null }; + const live = { + id: "grant-live", + access: "READ", + acceptedAt: new Date("2026-01-01"), + revokedAt: null, + expiresAt: null, + grantee: party, + }; + const lapsed = { + ...live, + id: "grant-lapsed", + expiresAt: new Date("2026-02-01"), + }; + vi.mocked(prisma.accountGrant.findMany).mockResolvedValue([ + live, + lapsed, + ] as never); + + const res = await DELETE(del()); + const body = await res.json(); + expect(body.data.pendingInvitesRevoked).toBe(2); + // The lapsed one confers nothing any more and is not listed. + expect(body.data.grantsKept).toEqual([ + { id: "grant-live", account: party, access: "READ" }, + ]); + expect(prisma.accountGrant.findMany).toHaveBeenCalledWith( + expect.objectContaining({ + where: expect.objectContaining({ + grantorId: "user-1", + revokedAt: null, + }), + }), + ); + }); +}); + describe("DELETE /api/auth/me/sessions/[id]", () => { function ctx(id: string) { return { params: Promise.resolve({ id }) }; diff --git a/src/app/api/auth/me/sessions/route.ts b/src/app/api/auth/me/sessions/route.ts index 1b9b02c34..9f256b4f4 100644 --- a/src/app/api/auth/me/sessions/route.ts +++ b/src/app/api/auth/me/sessions/route.ts @@ -5,7 +5,10 @@ * connection and token, and (unless * `?keepShareLinks=1`) every clinician share * link, keeping the caller's current - * credential. Closes #64. + * credential. Closes #64. Also withdraws + * record-sharing invitations nobody has + * accepted yet, and lists the accepted + * grants it left standing (`grantsKept`). * * v1.23 — the user-facing session/device-management surface. Distinct from * `/api/auth/me/devices`, which lists APNs / Web-Push notification devices; @@ -34,6 +37,8 @@ import { type CurrentCredential, } from "@/lib/auth/session"; import { lookupIpLocation } from "@/lib/geo"; +import { GRANT_PARTY_SELECT } from "@/lib/sharing/grant-view"; +import { isGrantActive } from "@/lib/sharing/grants"; import { coarseDeviceLabel, maskIp } from "@/lib/auth/device-fingerprint"; export const dynamic = "force-dynamic"; @@ -131,11 +136,32 @@ export const DELETE = apiHandler(async (request: NextRequest) => { accessTokensRevoked, connectorsRevoked, shareLinksRevoked, + pendingInvitesRevoked, } = await destroyOtherSessions(user.id, current, { reach: "everything", keepShareLinks, }); + // The accepted grants this account gave, still live. Not ended here: each is + // a person the owner chose, often a carer who still needs the record, and a + // button labelled "sign out" must not quietly cut them off. They are listed + // instead, so the person who just signed everything out can see who can + // still read the record and end any of them with one click. + const now = new Date(); + const grantsKept = ( + await prisma.accountGrant.findMany({ + where: { + grantorId: user.id, + acceptedAt: { not: null }, + revokedAt: null, + }, + orderBy: { acceptedAt: "desc" }, + include: { grantee: { select: GRANT_PARTY_SELECT } }, + }) + ) + .filter((g) => isGrantActive(g, now)) + .map((g) => ({ id: g.id, account: g.grantee, access: g.access })); + await auditLog("auth.session.revoke_others", { userId: user.id, ipAddress: getClientIp(request), @@ -144,6 +170,8 @@ export const DELETE = apiHandler(async (request: NextRequest) => { accessTokensRevoked, connectorsRevoked, shareLinksRevoked, + pendingInvitesRevoked, + grantsKept: grantsKept.length, keepShareLinks, }, }); @@ -155,6 +183,8 @@ export const DELETE = apiHandler(async (request: NextRequest) => { access_tokens_revoked: accessTokensRevoked, connectors_revoked: connectorsRevoked, share_links_revoked: shareLinksRevoked, + pending_invites_revoked: pendingInvitesRevoked, + grants_kept: grantsKept.length, }, }); @@ -163,6 +193,8 @@ export const DELETE = apiHandler(async (request: NextRequest) => { accessTokensRevoked, connectorsRevoked, shareLinksRevoked, + pendingInvitesRevoked, + grantsKept, }); }); diff --git a/src/components/settings/access/__tests__/grant-invite-expiry.test.tsx b/src/components/settings/access/__tests__/grant-invite-expiry.test.tsx index 49bb21f75..faae601af 100644 --- a/src/components/settings/access/__tests__/grant-invite-expiry.test.tsx +++ b/src/components/settings/access/__tests__/grant-invite-expiry.test.tsx @@ -70,15 +70,13 @@ describe("grant invitation lapse date", () => { ), "utf8", ); - const onError = source.slice( - source.indexOf("onError: (err) => setError"), - source.indexOf( - " );\n };", - source.indexOf("onError: (err) => setError"), - ), - ); + // The failure arm of the submit: everything from its `catch` to the end + // of the handler. + const start = source.indexOf("} catch (err) {"); + expect(start).toBeGreaterThan(-1); + const onError = source.slice(start, source.indexOf("\n };", start)); - expect(onError).toContain("setError(t(inviteErrorKey(err)))"); + expect(onError).toContain("t(inviteErrorKey(err))"); expect(onError).not.toContain("setIdentifier"); expect(onError).not.toContain("setExpiresOn"); expect(source).toContain('role="alert"'); diff --git a/src/components/settings/access/grant-invite-card.tsx b/src/components/settings/access/grant-invite-card.tsx index 3b86108ea..d9202302c 100644 --- a/src/components/settings/access/grant-invite-card.tsx +++ b/src/components/settings/access/grant-invite-card.tsx @@ -18,6 +18,10 @@ import { useModuleEnabled } from "@/hooks/use-module-enabled"; import { ApiError } from "@/lib/api/api-fetch"; import { useTranslations } from "@/lib/i18n/context"; import { useInviteGrant } from "@/lib/queries/use-account-grants"; +import { + ReproofCancelledError, + useRecentProof, +} from "@/components/settings/security-section/use-recent-proof"; import { SHARE_DOMAINS, type ShareDomain } from "@/lib/sharing/scope"; import { DEFAULT_TIMEZONE, isValidTimezone } from "@/lib/tz/format"; import { wallClockInTz, zonedWallClockToUtc } from "@/lib/tz/wall-clock"; @@ -79,6 +83,7 @@ import { wallClockInTz, zonedWallClockToUtc } from "@/lib/tz/wall-clock"; export function GrantInviteCard() { const { t } = useTranslations(); const invite = useInviteGrant(); + const recentProof = useRecentProof(); const ownerTimezone = useInvitationOwnerTimezone(); const [identifier, setIdentifier] = useState(""); const [expiresOn, setExpiresOn] = useState(""); @@ -118,31 +123,40 @@ export function GrantInviteCard() { setError(null); }; - const submit = (event: React.FormEvent) => { + const submit = async (event: React.FormEvent) => { event.preventDefault(); const value = identifier.trim(); if (value.length === 0 || choice.blocked) return; setError(null); setInvited(null); - invite.mutate( - { - identifier: value, - access, - scope: choice.scope, - expiresAt: endOfDayIso(expiresOn, ownerTimezone), - }, - { - onSuccess: (grant) => { - setIdentifier(""); - setExpiresOn(""); - setAccess("READ"); - setNarrowed(null); - setSections([]); - setInvited(grant.account.username); - }, - onError: (err) => setError(t(inviteErrorKey(err))), - }, - ); + // Offering access hands somebody a standing way into the record, so the + // server asks for a fresh proof unless the session signed in or re-proved + // within five minutes; the dialog collects it and the invitation is sent + // again. + try { + const grant = await recentProof.run(() => + invite.mutateAsync({ + identifier: value, + access, + scope: choice.scope, + expiresAt: endOfDayIso(expiresOn, ownerTimezone), + }), + ); + setIdentifier(""); + setExpiresOn(""); + setAccess("READ"); + setNarrowed(null); + setSections([]); + setInvited(grant.account.username); + } catch (err) { + if (err instanceof ReproofCancelledError) return; + setError( + err instanceof ApiError && + err.meta?.errorCode === "auth.reproof.sign_in_again" + ? err.message + : t(inviteErrorKey(err)), + ); + } }; return ( @@ -159,7 +173,7 @@ export function GrantInviteCard() { "recordSharing.invite.needsAccount", )}`} /> -
+ void submit(event)} className="space-y-3">
); diff --git a/src/lib/auth/session.ts b/src/lib/auth/session.ts index 2884ae478..afe723a05 100644 --- a/src/lib/auth/session.ts +++ b/src/lib/auth/session.ts @@ -575,6 +575,15 @@ export type CurrentCredential = * and the web offers to keep them because a link already handed to a * doctor is often wanted. The caller's own credential is spared either way. * + * `everything` also withdraws every record-sharing invitation this account + * offered that nobody has accepted yet. An invitation outlives the session + * that sent it, so one offered from a stolen session would be waiting for + * the thief's own account to accept it. Accepted grants are NOT ended here: + * they are a person the owner chose (a partner, a carer), usually still + * wanted, and ending them silently would cut a carer off without anyone + * deciding to. The route lists them in its answer instead, and the web + * offers to end each one with a click. + * * Returns what was removed, so the surface and the audit row can say what * happened. */ @@ -586,6 +595,7 @@ export interface SignOutResult { accessTokensRevoked: number; connectorsRevoked: number; shareLinksRevoked: number; + pendingInvitesRevoked: number; } export async function destroyOtherSessions( @@ -655,6 +665,7 @@ export async function destroyOtherSessions( accessTokensRevoked: access.count, connectorsRevoked: 0, shareLinksRevoked: 0, + pendingInvitesRevoked: 0, }; } @@ -680,11 +691,19 @@ export async function destroyOtherSessions( where: { userId, revokedAt: null }, data: { revokedAt }, }); + // Offered by this account and not accepted yet: `acceptedAt` null, not + // already ended. Stamped rather than deleted, so the consent record still + // shows the invitation and who withdrew it. + const invites = await tx.accountGrant.updateMany({ + where: { grantorId: userId, acceptedAt: null, revokedAt: null }, + data: { revokedAt, revokedBy: "GRANTOR" }, + }); return { sessionsRevoked: deleted.count, accessTokensRevoked: access.count + tokens.count, connectorsRevoked: connectors.count, shareLinksRevoked: links.count, + pendingInvitesRevoked: invites.count, }; }); } diff --git a/src/lib/openapi/routes/account-sharing.ts b/src/lib/openapi/routes/account-sharing.ts index 9b566b1cb..d4d59dc54 100644 --- a/src/lib/openapi/routes/account-sharing.ts +++ b/src/lib/openapi/routes/account-sharing.ts @@ -570,7 +570,7 @@ export const accountSharingPaths: NonNullable = { tags: ["Account sharing"], summary: "Invite an account to this record", description: - "Offers access to somebody already registered on this instance, at READ, WRITE or MANAGE and over the whole record or named sections; the grant confers nothing until they accept it. An identifier that names no account answers 404 — a deliberate disclosure to an authenticated caller on a household instance, rate-limited to 10 invitations an hour, and the alternative (a silent pending row for a mistyped username) is worse. Refused while acting on another account, so a delegate can neither invite nor re-delegate. Offering MANAGE is step-up gated (`requireFreshMfaIfEnrolled`, cookie-only): an enrolled account without a fresh factor gets 401 `auth.stepup.required`, and a Bearer caller gets 403 `sharing.invite.manage_browser_only` before anything else happens, because the gate it would hit resolves through the session cookie and would otherwise answer 'not authenticated' to a caller it had just authenticated.", + "Offers access to somebody already registered on this instance, at READ, WRITE or MANAGE and over the whole record or named sections; the grant confers nothing until they accept it. An identifier that names no account answers 404 — a deliberate disclosure to an authenticated caller on a household instance, rate-limited to 10 invitations an hour, and the alternative (a silent pending row for a mistyped username) is worse. Refused while acting on another account, so a delegate can neither invite nor re-delegate. Offering MANAGE is step-up gated (`requireFreshMfaIfEnrolled`, cookie-only): an enrolled account without a fresh factor gets 401 `auth.stepup.required`, and a Bearer caller gets 403 `sharing.invite.manage_browser_only` before anything else happens, because the gate it would hit resolves through the session cookie and would otherwise answer 'not authenticated' to a caller it had just authenticated. Every invitation, at any level, also needs a fresh proof: a browser session must have signed in or used `POST /api/auth/reproof` within the last five minutes (with the second factor, on an account that has one), or it gets 401 `auth.reproof.required` with `meta.methods`; a Bearer caller presents an `X-Step-Up` elevation from `POST /api/auth/step-up` (a second-factor one on an account with a second factor) and gets 401 `auth.stepup.required` without it. `DELETE /api/auth/me/sessions` withdraws the invitations nobody has accepted yet.", requestBody: { required: true, content: { "application/json": { schema: accountGrantInvite } }, @@ -587,7 +587,7 @@ export const accountSharingPaths: NonNullable = { }, "401": { description: - "Offering MANAGE from an MFA-enrolled account without a recent second-factor proof (`meta.errorCode: auth.stepup.required`). Re-prove the factor and retry; READ and WRITE invitations are never gated.", + "No recent proof. A browser session that has not signed in or re-proved within five minutes gets `meta.errorCode: auth.reproof.required` with `meta.methods`; re-prove at POST /api/auth/reproof and retry. A Bearer caller without a valid `X-Step-Up` elevation gets `auth.stepup.required`, as does offering MANAGE from an MFA-enrolled browser session without a recent second-factor proof.", content: { "application/json": { schema: errorEnvelope } }, }, "403": { diff --git a/src/lib/openapi/routes/auth.ts b/src/lib/openapi/routes/auth.ts index c26652e3b..961b25e76 100644 --- a/src/lib/openapi/routes/auth.ts +++ b/src/lib/openapi/routes/auth.ts @@ -238,6 +238,29 @@ const signOutEverywhereResponse = z .describe( "Clinician share links revoked; 0 when `keepShareLinks=1` was sent.", ), + pendingInvitesRevoked: z + .number() + .int() + .describe( + "Record-sharing invitations this account offered that nobody had accepted yet, now withdrawn (`revokedBy: GRANTOR`).", + ), + grantsKept: z + .array( + z.object({ + id: z + .string() + .describe("The grant id, for DELETE /api/account/grants/{id}."), + account: z.object({ + id: z.string(), + username: z.string(), + displayName: z.string().nullable(), + }), + access: z.enum(["READ", "WRITE", "MANAGE"]), + }), + ) + .describe( + "Accepted grants this account gave that are still live. Signing out does not end them, because each is a person the owner chose and ending one silently could cut off a carer; they are listed so a client can show who can still read the record and offer to end each with DELETE /api/account/grants/{id}.", + ), }) .meta({ id: "SignOutEverywhereResponse" }); @@ -1328,7 +1351,7 @@ export const authPaths: NonNullable = { tags: ["Auth"], summary: "Sign out everywhere else", description: - "Ends every credential except the caller's own: every OTHER web session, every other device login (refresh token and its paired access token), every trusted device and step-up elevation, every connected AI assistant (the OAuth connection, so it cannot mint a new access token), every programmatic token (connector, measurement and document tokens), and every clinician share link. Share links are revoked by default because a link made by whoever held a lost session or device would otherwise keep opening the record; send `keepShareLinks=1` to keep them. A Bearer caller keeps its own device login.", + "Ends every credential except the caller's own: every OTHER web session, every other device login (refresh token and its paired access token), every trusted device and step-up elevation, every connected AI assistant (the OAuth connection, so it cannot mint a new access token), every programmatic token (connector, measurement and document tokens), and every clinician share link. Share links are revoked by default because a link made by whoever held a lost session or device would otherwise keep opening the record; send `keepShareLinks=1` to keep them. Record-sharing invitations this account offered that nobody has accepted yet are withdrawn too, because an invitation sent from a stolen session would otherwise wait for the thief's own account. Accepted grants are not ended: the response lists them as `grantsKept` so the client can show who can still read the record and offer to end each one. A Bearer caller keeps its own device login.", requestParams: { query: z.object({ keepShareLinks: z.enum(["1"]).optional().meta({ diff --git a/tests/integration/sessions-and-known-devices.test.ts b/tests/integration/sessions-and-known-devices.test.ts index 719fbd3e8..3a5a9f5b4 100644 --- a/tests/integration/sessions-and-known-devices.test.ts +++ b/tests/integration/sessions-and-known-devices.test.ts @@ -386,6 +386,65 @@ describe("destroyOtherSessions — everything", () => { ).toBeNull(); }); + it("withdraws invitations nobody accepted and leaves accepted grants standing", async () => { + const prisma = getPrismaClient(); + const owner = await makeUser("sess-invites-owner"); + const carer = await makeUser("sess-invites-carer"); + const stranger = await makeUser("sess-invites-stranger"); + const other = await makeUser("sess-invites-other"); + const accepted = await prisma.accountGrant.create({ + data: { + grantorId: owner.id, + granteeId: carer.id, + access: "READ", + acceptedAt: new Date(), + }, + }); + const pending = await prisma.accountGrant.create({ + data: { grantorId: owner.id, granteeId: stranger.id, access: "WRITE" }, + }); + // Somebody else's invitation to the owner is not the owner's to withdraw. + const received = await prisma.accountGrant.create({ + data: { grantorId: other.id, granteeId: owner.id, access: "READ" }, + }); + + const result = await destroyOtherSessions( + owner.id, + { kind: "accessToken", accessTokenHash: "none" }, + { reach: "everything" }, + ); + + expect(result.pendingInvitesRevoked).toBe(1); + const after = async (id: string) => + prisma.accountGrant.findUniqueOrThrow({ where: { id } }); + expect((await after(pending.id)).revokedAt).not.toBeNull(); + expect((await after(pending.id)).revokedBy).toBe("GRANTOR"); + expect((await after(accepted.id)).revokedAt).toBeNull(); + expect((await after(received.id)).revokedAt).toBeNull(); + }); + + it("sign-ins leaves pending invitations alone", async () => { + const prisma = getPrismaClient(); + const owner = await makeUser("sess-invites-signins"); + const other = await makeUser("sess-invites-signins-2"); + const pending = await prisma.accountGrant.create({ + data: { grantorId: owner.id, granteeId: other.id, access: "READ" }, + }); + const result = await destroyOtherSessions( + owner.id, + { kind: "accessToken", accessTokenHash: "none" }, + { reach: "sign-ins" }, + ); + expect(result.pendingInvitesRevoked).toBe(0); + expect( + ( + await prisma.accountGrant.findUniqueOrThrow({ + where: { id: pending.id }, + }) + ).revokedAt, + ).toBeNull(); + }); + it("sign-ins leaves connections, tokens and share links alone", async () => { const prisma = getPrismaClient(); const user = await makeUser("sess-signins-only"); From 1900792756ee43e37a3cdc9f3fc8f6dde9bf0ca3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Bombeck?= Date: Sat, 26 Sep 2026 22:38:40 +0200 Subject: [PATCH 4/9] fix(mcp): connecting an assistant asks for a recent sign-in or re-proof Allowing an AI assistant hands it a standing credential to the record that outlives the session, the same class of act as minting a token, but the consent took a live session alone. A stolen session could connect its own assistant and keep reading after the owner signed everything out. The consent page now shows the Allow form only when the session signed in or re-proved within five minutes (with the second factor on an account that has one). Otherwise it links to a new confirm page that runs the same re-proof dialog as the rest of the app and returns to the exact consent request. The allow decision checks again and mints nothing without it; deny needs no proof. The confirm page only ever returns to the consent endpoint. --- messages/de.json | 7 ++ messages/en.json | 7 ++ messages/es.json | 7 ++ messages/fr.json | 7 ++ messages/it.json | 7 ++ messages/ko.json | 7 ++ messages/pl.json | 7 ++ .../recent-proof-surface-guard.test.ts | 13 ++- .../api/mcp/oauth/__tests__/authorize.test.ts | 95 ++++++++++++++++++- src/app/api/mcp/oauth/authorize/route.ts | 64 ++++++++++++- .../__tests__/return-to.test.ts | 24 +++++ .../confirm-access/confirm-access-client.tsx | 69 ++++++++++++++ src/app/confirm-access/page.tsx | 21 ++++ src/app/confirm-access/return-to.ts | 26 +++++ 14 files changed, 358 insertions(+), 3 deletions(-) create mode 100644 src/app/confirm-access/__tests__/return-to.test.ts create mode 100644 src/app/confirm-access/confirm-access-client.tsx create mode 100644 src/app/confirm-access/page.tsx create mode 100644 src/app/confirm-access/return-to.ts diff --git a/messages/de.json b/messages/de.json index 8e7c9e885..7ccfb920c 100644 --- a/messages/de.json +++ b/messages/de.json @@ -471,6 +471,13 @@ "title": "Zweiten Faktor einrichten", "body": "Dein Administrator verlangt einen zweiten Faktor für dieses Konto. Füge eine Authenticator-App oder einen Sicherheitsschlüssel hinzu, um fortzufahren.", "signOut": "Abmelden" + }, + "confirmAccess": { + "title": "Bestätige, dass du es bist", + "body": "Ein Assistent möchte sich mit deinen HealthLog-Daten verbinden. Bestätige, dass du es bist; danach entscheidest du, ob du es erlaubst.", + "action": "Bestätigen", + "failed": "Das hat nicht geklappt. Bitte versuche es noch einmal.", + "nothingToConfirm": "Hier gibt es nichts zu bestätigen. Starte die Verbindung noch einmal in deinem Assistenten." } }, "dashboard": { diff --git a/messages/en.json b/messages/en.json index 5fb9c4cf2..eeedef816 100644 --- a/messages/en.json +++ b/messages/en.json @@ -471,6 +471,13 @@ "title": "Set up a second factor", "body": "Your administrator requires a second factor on this account. Add an authenticator app or a security key to continue.", "signOut": "Sign out" + }, + "confirmAccess": { + "title": "Confirm it is you", + "body": "An assistant is asking to connect to your HealthLog data. Confirm it is you, then you will be taken back to decide whether to allow it.", + "action": "Confirm", + "failed": "That did not work. Please try again.", + "nothingToConfirm": "There is nothing to confirm here. Start the connection again from your assistant." } }, "dashboard": { diff --git a/messages/es.json b/messages/es.json index 3caf7e3d1..5856435ad 100644 --- a/messages/es.json +++ b/messages/es.json @@ -471,6 +471,13 @@ "title": "Configurar un segundo factor", "body": "Tu administrador exige un segundo factor en esta cuenta. Añade una app de autenticación o una llave de seguridad para continuar.", "signOut": "Cerrar sesión" + }, + "confirmAccess": { + "title": "Confirma que eres tú", + "body": "Un asistente quiere conectarse a tus datos de HealthLog. Confirma que eres tú y después decidirás si lo permites.", + "action": "Confirmar", + "failed": "No ha funcionado. Inténtalo de nuevo.", + "nothingToConfirm": "Aquí no hay nada que confirmar. Vuelve a iniciar la conexión desde tu asistente." } }, "dashboard": { diff --git a/messages/fr.json b/messages/fr.json index f362b2bae..50abc764c 100644 --- a/messages/fr.json +++ b/messages/fr.json @@ -471,6 +471,13 @@ "title": "Configurer un second facteur", "body": "Votre administrateur exige un second facteur sur ce compte. Ajoutez une application d'authentification ou une clé de sécurité pour continuer.", "signOut": "Se déconnecter" + }, + "confirmAccess": { + "title": "Confirmez que c'est bien vous", + "body": "Un assistant demande à se connecter à vos données HealthLog. Confirmez que c'est bien vous, puis vous déciderez si vous l'autorisez.", + "action": "Confirmer", + "failed": "Cela n'a pas fonctionné. Veuillez réessayer.", + "nothingToConfirm": "Il n'y a rien à confirmer ici. Relancez la connexion depuis votre assistant." } }, "dashboard": { diff --git a/messages/it.json b/messages/it.json index 404551bb8..941033101 100644 --- a/messages/it.json +++ b/messages/it.json @@ -471,6 +471,13 @@ "title": "Configura un secondo fattore", "body": "Il tuo amministratore richiede un secondo fattore su questo account. Aggiungi un'app di autenticazione o una chiave di sicurezza per continuare.", "signOut": "Esci" + }, + "confirmAccess": { + "title": "Conferma che sei tu", + "body": "Un assistente chiede di collegarsi ai tuoi dati HealthLog. Conferma che sei tu, poi deciderai se consentirlo.", + "action": "Conferma", + "failed": "Non ha funzionato. Riprova.", + "nothingToConfirm": "Qui non c'è nulla da confermare. Avvia di nuovo la connessione dal tuo assistente." } }, "dashboard": { diff --git a/messages/ko.json b/messages/ko.json index 85af2c610..f1349726d 100644 --- a/messages/ko.json +++ b/messages/ko.json @@ -471,6 +471,13 @@ "title": "2차 인증 수단 설정", "body": "관리자가 이 계정에 2차 인증 수단을 요구해요. 인증 앱이나 보안 키를 추가하고 계속해 주세요.", "signOut": "로그아웃" + }, + "confirmAccess": { + "title": "본인인지 확인해 주세요", + "body": "어시스턴트가 HealthLog 데이터에 연결하려고 해요. 본인인지 확인하면 허용할지 정하는 화면으로 돌아가요.", + "action": "확인", + "failed": "확인하지 못했어요. 다시 시도해 주세요.", + "nothingToConfirm": "여기에서 확인할 것이 없어요. 어시스턴트에서 연결을 다시 시작해 주세요." } }, "dashboard": { diff --git a/messages/pl.json b/messages/pl.json index 541ad9267..b71f77fd3 100644 --- a/messages/pl.json +++ b/messages/pl.json @@ -471,6 +471,13 @@ "title": "Skonfiguruj drugi składnik", "body": "Administrator wymaga drugiego składnika na tym koncie. Dodaj aplikację uwierzytelniającą lub klucz bezpieczeństwa, aby kontynuować.", "signOut": "Wyloguj się" + }, + "confirmAccess": { + "title": "Potwierdź, że to Ty", + "body": "Asystent chce połączyć się z Twoimi danymi w HealthLog. Potwierdź, że to Ty, a potem zdecydujesz, czy na to pozwolić.", + "action": "Potwierdź", + "failed": "Nie udało się. Spróbuj ponownie.", + "nothingToConfirm": "Nie ma tu nic do potwierdzenia. Rozpocznij połączenie ponownie w swoim asystencie." } }, "dashboard": { diff --git a/src/__tests__/recent-proof-surface-guard.test.ts b/src/__tests__/recent-proof-surface-guard.test.ts index cb6bb3878..bb334ff98 100644 --- a/src/__tests__/recent-proof-surface-guard.test.ts +++ b/src/__tests__/recent-proof-surface-guard.test.ts @@ -97,6 +97,13 @@ const TOKEN_ARM_COOKIE_PROOF_ROUTES = [ "app/api/fhir/Patient/$everything/route.ts", ].sort(); +/** + * The connection consent. It resolves the session through `getSession` + * (OAuth answers in its own error format, not the envelope) and asks for the + * proof on the consent page and again on "allow". + */ +const CONSENT_PROOF_ROUTES = ["app/api/mcp/oauth/authorize/route.ts"]; + /** Cookie-only routes that add the gate after their own resolver. */ const COOKIE_PROOF_ROUTES = [ "app/api/admin/backups/[id]/download/route.ts", @@ -131,7 +138,11 @@ describe("the recent-proof gate", () => { const found = callers("assertRecentCookieProof"); expect(found.length).toBeGreaterThan(0); expect(found).toEqual( - [...COOKIE_PROOF_ROUTES, ...TOKEN_ARM_COOKIE_PROOF_ROUTES].sort(), + [ + ...COOKIE_PROOF_ROUTES, + ...TOKEN_ARM_COOKIE_PROOF_ROUTES, + ...CONSENT_PROOF_ROUTES, + ].sort(), ); }); diff --git a/src/app/api/mcp/oauth/__tests__/authorize.test.ts b/src/app/api/mcp/oauth/__tests__/authorize.test.ts index 68142b8c1..164da251e 100644 --- a/src/app/api/mcp/oauth/__tests__/authorize.test.ts +++ b/src/app/api/mcp/oauth/__tests__/authorize.test.ts @@ -1,7 +1,7 @@ /** * Authorize endpoint — consent gate + audience + PKCE + redirect-URI binding. */ -import { describe, it, expect, vi, beforeEach } from "vitest"; +import { describe, it, expect, vi, beforeEach, afterEach } from "vitest"; process.env.API_TOKEN_HMAC_KEY = "x".repeat(48); process.env.APP_URL = "https://health.example"; @@ -21,6 +21,22 @@ vi.mock("@/lib/rate-limit", () => ({ })), rateLimitHeaders: vi.fn(() => ({})), })); +// The consent reads the session's own proof stamps. By default the session +// signed in a moment ago, which is a recent proof on an account without a +// second factor; the cases below age it. +vi.mock("@/lib/db", () => ({ + prisma: { + session: { + findUnique: vi.fn(async () => ({ + createdAt: new Date(), + mfaVerifiedAt: null, + reproofAt: null, + })), + }, + webauthnMfaCredential: { count: vi.fn(async () => 0) }, + passkey: { count: vi.fn(async () => 0) }, + }, +})); vi.mock("@/lib/app-settings", () => ({ isApiGloballyEnabled: vi.fn(async () => true), })); @@ -30,6 +46,7 @@ import { getSession } from "@/lib/auth/session"; import { isApiGloballyEnabled } from "@/lib/app-settings"; import { registerDcrClient } from "@/lib/mcp/oauth/clients"; import { s256Challenge } from "@/lib/mcp/oauth/pkce"; +import { prisma } from "@/lib/db"; const REDIRECT = "https://claude.ai/api/mcp/auth_callback"; const VERIFIER = "a".repeat(64); @@ -327,6 +344,82 @@ describe("POST /authorize — decision", () => { }); }); +describe("authorize — allowing a connection needs a recent proof", () => { + // A queued stale stamp a case did not read must not leak into the next one. + afterEach(() => { + vi.mocked(prisma.session.findUnique).mockReset(); + }); + + function staleSession() { + vi.mocked(prisma.session.findUnique).mockResolvedValueOnce({ + createdAt: new Date(Date.now() - 60 * 60 * 1000), + mfaVerifiedAt: null, + reproofAt: null, + } as never); + } + const allow = { + response_type: "code", + client_id: CLIENT.clientId, + redirect_uri: REDIRECT, + code_challenge: CHALLENGE, + code_challenge_method: "S256", + scope: "health:read", + state: "xyz", + resource: RESOURCE, + decision: "allow", + }; + + it("GET shows a confirm link instead of the form when the session is an hour old", async () => { + signedIn(); + staleSession(); + const res = await GET(getReq(authorizeUrl()) as never); + expect(res.status).toBe(200); + const body = await res.text(); + expect(body).toMatch(/Confirm it is you/); + expect(body).not.toMatch(/name="decision"/); + const href = body.match(/href="([^"]+)"/)?.[1] ?? ""; + const next = + new URL(href.replace(/&/g, "&"), "https://x").searchParams.get( + "next", + ) ?? ""; + expect(href.startsWith("/confirm-access?next=")).toBe(true); + // Back to this exact consent request. + expect(next).toBe( + new URL(authorizeUrl()).pathname + new URL(authorizeUrl()).search, + ); + }); + + it("POST allow on an hour-old session mints nothing", async () => { + signedIn(); + staleSession(); + const res = await POST(postReq(allow) as never); + expect(res.status).toBe(401); + expect(res.headers.get("location")).toBeNull(); + expect(await res.text()).toMatch(/confirm-access\?next=/); + }); + + it("an account with a second factor needs the factor, not a young session", async () => { + vi.mocked(getSession).mockResolvedValue({ + session: { + id: "s1", + expiresAt: new Date(Date.now() + 1e6), + actingAsUserId: null, + recordEpoch: 0, + }, + user: { id: "user-1", totpConfirmedAt: new Date("2026-01-01") } as never, + }); + const res = await POST(postReq(allow) as never); + expect(res.status).toBe(401); + }); + + it("deny needs no proof", async () => { + signedIn(); + staleSession(); + const res = await POST(postReq({ ...allow, decision: "deny" }) as never); + expect(res.status).toBe(303); + }); +}); + describe("authorize CSP — the consent form may lead only to the validated redirect", () => { function csp(res: Response): string { return res.headers.get("content-security-policy") ?? ""; diff --git a/src/app/api/mcp/oauth/authorize/route.ts b/src/app/api/mcp/oauth/authorize/route.ts index 783940496..b0ad1ef5e 100644 --- a/src/app/api/mcp/oauth/authorize/route.ts +++ b/src/app/api/mcp/oauth/authorize/route.ts @@ -11,7 +11,9 @@ * a GET (confused-deputy mitigation: a third party cannot silently * obtain a grant — an authenticated human must click "Allow"). * POST — the consent decision. Requires a session, re-validates every - * parameter, and on "Allow" mints a single-use, short-lived, + * parameter, and on "Allow" asks for a recent sign-in or re-proof + * (the GET shows a confirm link instead of the form without one), + * then mints a single-use, short-lived, * self-describing authorization code bound to this user + client + * redirect URI + PKCE challenge + scope + audience, then 303s back. * @@ -30,6 +32,8 @@ import { annotate } from "@/lib/logging/context"; import { withBackgroundEvent } from "@/lib/logging/background"; import { auditLog } from "@/lib/auth/audit"; import { getSession } from "@/lib/auth/session"; +import { assertRecentCookieProof } from "@/lib/api-handler"; +import { StepUpRequiredError } from "@/lib/api-errors"; import { checkAuthSurfaceRateLimit, rateLimitHeaders } from "@/lib/rate-limit"; import { isApiGloballyEnabled } from "@/lib/app-settings"; import { @@ -88,6 +92,42 @@ function html(body: string, status = 200): Response { ); } +/** + * Has this session signed in or re-proved recently enough to grant access? + * + * Allowing a connection hands a standing credential to the record to a third + * party, one that outlives the session: the same class of act as minting a + * token or making a share link, and it takes the same gate + * (`assertRecentCookieProof`: a sign-in or re-proof within five minutes, with + * the second factor on an account that has one). Without it, whoever held a + * stolen session could connect their own assistant and keep reading the + * record after the owner signed everything out. + */ +async function hasRecentProof( + session: NonNullable>>, +): Promise { + try { + await assertRecentCookieProof(session.user, session.session.id); + return true; + } catch (err) { + if (err instanceof StepUpRequiredError) return false; + throw err; + } +} + +/** + * The page shown instead of the consent form while the session carries no + * recent proof. It links to the confirm page, which runs the same re-proof + * dialog as the rest of the app and then comes back to this exact request. + */ +function reproofPage(clientName: string, returnTo: string, status = 200) { + const confirmHref = `/confirm-access?next=${encodeURIComponent(returnTo)}`; + return html( + `

Confirm it is you

Before ${htmlEscape(clientName)} can connect to your HealthLog data, confirm it is you. This is asked when you have not signed in or confirmed in the last five minutes.

Confirm and continue

`, + status, + ); +} + /** * The scopes this surface grants for a request. `health:read` is always * granted (the floor). `health:write` is granted ONLY when explicitly @@ -359,6 +399,11 @@ async function authorizeGet(request: NextRequest): Promise { ); } + if (!(await hasRecentProof(session))) { + annotate({ action: { name: "mcp.oauth.authorize.reproof_required" } }); + return reproofPage(v.clientName, url.pathname + url.search); + } + annotate({ action: { name: "mcp.oauth.authorize.consent_shown" } }); // Consent screen — a plain POST form (no inline script/style, CSP-safe). const hidden = (name: string, value: string) => @@ -499,6 +544,23 @@ async function authorizePost(request: NextRequest): Promise { }); } + // The same gate the consent page asked for, checked again on the decision + // itself: the page may have been left open past the window, or the POST + // built by hand. Denying needs no proof, so only "allow" is gated. + if (!(await hasRecentProof(session))) { + annotate({ action: { name: "mcp.oauth.authorize.reproof_required" } }); + const back = new URL("/api/mcp/oauth/authorize", request.url); + back.searchParams.set("response_type", "code"); + back.searchParams.set("client_id", v.clientId); + back.searchParams.set("redirect_uri", v.redirectUri); + back.searchParams.set("code_challenge", v.codeChallenge); + back.searchParams.set("code_challenge_method", "S256"); + back.searchParams.set("scope", v.scope); + back.searchParams.set("resource", v.resource); + if (v.state !== undefined) back.searchParams.set("state", v.state); + return reproofPage(v.clientName, back.pathname + back.search, 401); + } + const code = signArtifact( "authCode", { diff --git a/src/app/confirm-access/__tests__/return-to.test.ts b/src/app/confirm-access/__tests__/return-to.test.ts new file mode 100644 index 000000000..8f7f4555e --- /dev/null +++ b/src/app/confirm-access/__tests__/return-to.test.ts @@ -0,0 +1,24 @@ +import { describe, expect, it } from "vitest"; + +import { confirmReturnTo } from "../return-to"; + +describe("confirmReturnTo", () => { + it("returns to the connection consent it came from", () => { + expect( + confirmReturnTo("/api/mcp/oauth/authorize?client_id=a&state=b"), + ).toBe("/api/mcp/oauth/authorize?client_id=a&state=b"); + }); + + it.each([ + null, + "", + "/", + "/settings", + "https://evil.example/api/mcp/oauth/authorize?x=1", + "//evil.example/api/mcp/oauth/authorize?x=1", + "/api/mcp/oauth/authorize", + "/api/mcp/oauth/authorize/../token?x=1", + ])("goes nowhere for %s", (next) => { + expect(confirmReturnTo(next)).toBeNull(); + }); +}); diff --git a/src/app/confirm-access/confirm-access-client.tsx b/src/app/confirm-access/confirm-access-client.tsx new file mode 100644 index 000000000..ee369b423 --- /dev/null +++ b/src/app/confirm-access/confirm-access-client.tsx @@ -0,0 +1,69 @@ +"use client"; + +import { useState } from "react"; +import { Loader2, ShieldCheck } from "lucide-react"; + +import { Button } from "@/components/ui/button"; +import { + recentProofErrorMessage, + useRecentProof, +} from "@/components/settings/security-section/use-recent-proof"; +import { apiGet } from "@/lib/api/api-fetch"; +import { useTranslations } from "@/lib/i18n/context"; + +export function ConfirmAccessClient({ returnTo }: { returnTo: string | null }) { + const { t } = useTranslations(); + const recentProof = useRecentProof(); + const [busy, setBusy] = useState(false); + const [error, setError] = useState(null); + + async function confirm() { + if (!returnTo) return; + setBusy(true); + setError(null); + try { + // Answers 200 when the session's proof is recent, and otherwise the + // refusal the hook turns into the re-proof dialog. + await recentProof.run(() => apiGet("/api/auth/reproof")); + // A full navigation: the consent is a server page, not an app route. + window.location.assign(returnTo); + } catch (err) { + setError(recentProofErrorMessage(err, t("auth.confirmAccess.failed"))); + setBusy(false); + } + } + + return ( +
+
+
+ {returnTo ? ( + <> +

{t("auth.confirmAccess.body")}

+
+ +
+ + ) : ( +

{t("auth.confirmAccess.nothingToConfirm")}

+ )} + {error && ( +

+ {error} +

+ )} + {recentProof.dialog} +
+ ); +} diff --git a/src/app/confirm-access/page.tsx b/src/app/confirm-access/page.tsx new file mode 100644 index 000000000..b9885f7e7 --- /dev/null +++ b/src/app/confirm-access/page.tsx @@ -0,0 +1,21 @@ +import { ConfirmAccessClient } from "./confirm-access-client"; +import { confirmReturnTo } from "./return-to"; + +/** + * Confirm it is you before connecting an AI assistant. + * + * The connection consent (`/api/mcp/oauth/authorize`) is a plain server page + * with no script, so it cannot run the re-proof dialog itself. When the + * session has not signed in or re-proved within five minutes it links here; + * this page runs the same dialog as the rest of the app and then goes back to + * the consent request it came from. + */ +export default async function ConfirmAccessPage({ + searchParams, +}: { + searchParams: Promise<{ next?: string | string[] }>; +}) { + const { next } = await searchParams; + const returnTo = confirmReturnTo(typeof next === "string" ? next : null); + return ; +} diff --git a/src/app/confirm-access/return-to.ts b/src/app/confirm-access/return-to.ts new file mode 100644 index 000000000..166e8a49a --- /dev/null +++ b/src/app/confirm-access/return-to.ts @@ -0,0 +1,26 @@ +/** + * Where the confirm page may send the browser once the person has confirmed + * it is them. + * + * Only back to the connection consent it came from, never anywhere else: the + * page takes `next` from the address bar, so any path it accepted would be a + * same-origin redirect somebody else could point. The consent request carries + * its own parameters and re-validates all of them when it is loaded again. + */ +export const CONSENT_PATH = "/api/mcp/oauth/authorize"; + +export function confirmReturnTo( + next: string | null | undefined, +): string | null { + if (!next || !next.startsWith(`${CONSENT_PATH}?`)) return null; + try { + const base = "http://confirm.invalid"; + const resolved = new URL(next, base); + if (resolved.origin !== base || resolved.pathname !== CONSENT_PATH) { + return null; + } + return `${resolved.pathname}${resolved.search}`; + } catch { + return null; + } +} From 995bd2385adc4e9bce0cb72a366e7b7dc1c521ed Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Bombeck?= Date: Sat, 26 Sep 2026 22:42:40 +0200 Subject: [PATCH 5/9] fix(auth): wrong passwords from elsewhere can no longer lock the owner out The per-account password limit refused every attempt once ten failures had landed in fifteen minutes, counted before the password was checked. Ten guesses from two addresses therefore locked a password-only account, the app's password sign-in included, and repeating them kept it locked for as long as anyone cared to. A place the account has signed in from before is now neither counted nor held back: a remembered browser, a phone presenting the device id a login of this account was issued to, or an address a session or device login of this account came from in the last thirty days. Anywhere else gets five free failures a day and then a wait that doubles with each failure, from thirty seconds to at most fifteen minutes, instead of a wall. The per-IP limit is unchanged, and an unknown identifier is limited the same way on its hash. Under an SSO-only policy, a password sign-in to an account other than the one a pending link names is now refused before the password is checked, so that exception no longer tells right passwords from wrong ones. --- docs/api/openapi.yaml | 13 +- .../api/auth/login/mfa-branch.test.ts | 24 +- .../login/__tests__/constant-work.test.ts | 10 + .../auth/login/__tests__/native-token.test.ts | 10 + .../api/auth/login/__tests__/route.test.ts | 10 + .../login/__tests__/trusted-device.test.ts | 10 + src/app/api/auth/login/route.ts | 75 +++--- src/lib/auth/__tests__/login-throttle.test.ts | 222 ++++++++++++++++++ src/lib/auth/login-throttle.ts | 194 +++++++++++++++ src/lib/openapi/routes/auth.ts | 5 +- tests/integration/oidc-pending-link.test.ts | 11 + 11 files changed, 527 insertions(+), 57 deletions(-) create mode 100644 src/lib/auth/__tests__/login-throttle.test.ts create mode 100644 src/lib/auth/login-throttle.ts diff --git a/docs/api/openapi.yaml b/docs/api/openapi.yaml index fbe1426cd..9aeb0e65d 100644 --- a/docs/api/openapi.yaml +++ b/docs/api/openapi.yaml @@ -1865,6 +1865,15 @@ paths: 200 with `data: null, error: null` and `meta.mfaRequired: true` plus a single-use `meta.mfaTicket` and the `meta.methods` list. The client must POST the ticket + a code to `/api/auth/mfa/verify` to obtain the token bundle. Accounts without MFA are unchanged. + + + Two limits apply. Per source address, five attempts in fifteen minutes. Per account, failed passwords from a + place the account has not signed in from before make the next attempt wait: five failures a day are free, then + each one doubles the wait, from thirty seconds up to at most fifteen minutes. A place the account has signed in + from is not counted and not held back: a remembered browser (trusted-device cookie), a phone that sends the + `X-Device-Id` a login of this account was issued to, or an address a session or device login of this account + came from in the last thirty days. Both answer 429 with `Retry-After`. An identifier that names no account is + limited the same way, so a 429 says nothing about whether it exists. requestBody: required: true content: @@ -1884,7 +1893,9 @@ paths: "403": description: "Password login is disabled — the operator runs `OIDC_ONLY=true`. `meta.errorCode` = `oidc_only`; sign in through SSO instead. The one exception is a browser the SSO callback sent back with a pending link to - confirm an existing account: it may sign in with that one account's password, which links the account." + confirm an existing account: it may sign in with that one account's password, which links the account. A + sign-in to any other account on that browser is refused before its password is checked, so the answer is the + same whether the password was right or wrong." content: application/json: schema: diff --git a/src/__tests__/api/auth/login/mfa-branch.test.ts b/src/__tests__/api/auth/login/mfa-branch.test.ts index 3210a5c44..1b9a644e1 100644 --- a/src/__tests__/api/auth/login/mfa-branch.test.ts +++ b/src/__tests__/api/auth/login/mfa-branch.test.ts @@ -9,6 +9,11 @@ vi.mock("@/lib/db", () => ({ prisma: { user: { findFirst: vi.fn() }, webauthnMfaCredential: { count: vi.fn().mockResolvedValue(0) }, + // The per-account throttle's reads: no known place, no wait running. + trustedDevice: { findUnique: vi.fn(async () => null) }, + refreshToken: { findFirst: vi.fn(async () => null) }, + session: { findFirst: vi.fn(async () => null) }, + rateLimit: { findUnique: vi.fn(async () => null), upsert: vi.fn() }, }, })); vi.mock("@/lib/auth/password", () => ({ @@ -165,27 +170,26 @@ describe("login MFA branch", () => { }); }); -describe("per-account login ceiling", () => { +describe("per-account login throttle", () => { it("refuses a throttled account before any password is verified", async () => { const { verifyPasswordOrDummy } = await import("@/lib/auth/password"); vi.mocked(prisma.user.findFirst).mockResolvedValue({ ...BASE_USER, totpConfirmedAt: null, } as never); - vi.mocked(checkRateLimit).mockResolvedValueOnce({ - allowed: false, - limit: 10, - remaining: 0, - resetAt: Date.now() + 1e6, - }); + // A wait is running for this account, set by earlier failures. + vi.mocked(prisma.rateLimit.findUnique).mockResolvedValueOnce({ + resetAt: new Date(Date.now() + 60_000), + } as never); const res = await POST(loginRequest()); expect(res.status).toBe(429); expect(verifyPasswordOrDummy).not.toHaveBeenCalled(); expect(finishLogin).not.toHaveBeenCalled(); - expect(vi.mocked(checkRateLimit).mock.calls[0][0]).toBe( - "auth:login:account:u:user-1", - ); + expect(vi.mocked(prisma.rateLimit.findUnique).mock.calls[0][0]).toEqual({ + where: { key: "auth:login:account:u:user-1:wait" }, + select: { resetAt: true }, + }); }); it("keys an unknown identifier on its hash, so a lock says nothing about existence", async () => { diff --git a/src/app/api/auth/login/__tests__/constant-work.test.ts b/src/app/api/auth/login/__tests__/constant-work.test.ts index 977ab9e51..6490b756a 100644 --- a/src/app/api/auth/login/__tests__/constant-work.test.ts +++ b/src/app/api/auth/login/__tests__/constant-work.test.ts @@ -21,6 +21,16 @@ import { describe, it, expect, vi, beforeEach } from "vitest"; import { NextRequest } from "next/server"; +// The per-account throttle has its own suite (login-throttle.test.ts); here +// it lets every attempt through and counts nothing. +vi.mock("@/lib/auth/login-throttle", () => ({ + beginAccountLoginAttempt: vi.fn(async () => ({ + waiting: null, + source: null, + failed: async () => {}, + succeeded: async () => {}, + })), +})); vi.mock("@/lib/db", () => ({ prisma: { user: { findFirst: vi.fn() }, diff --git a/src/app/api/auth/login/__tests__/native-token.test.ts b/src/app/api/auth/login/__tests__/native-token.test.ts index 73e5d82d9..231483378 100644 --- a/src/app/api/auth/login/__tests__/native-token.test.ts +++ b/src/app/api/auth/login/__tests__/native-token.test.ts @@ -3,6 +3,16 @@ import { NextRequest } from "next/server"; // --- Mocks must come before importing the route. --- +// The per-account throttle has its own suite (login-throttle.test.ts); here +// it lets every attempt through and counts nothing. +vi.mock("@/lib/auth/login-throttle", () => ({ + beginAccountLoginAttempt: vi.fn(async () => ({ + waiting: null, + source: null, + failed: async () => {}, + succeeded: async () => {}, + })), +})); vi.mock("@/lib/db", () => ({ prisma: { user: { findFirst: vi.fn() }, diff --git a/src/app/api/auth/login/__tests__/route.test.ts b/src/app/api/auth/login/__tests__/route.test.ts index fb2df339b..6a8d06c6a 100644 --- a/src/app/api/auth/login/__tests__/route.test.ts +++ b/src/app/api/auth/login/__tests__/route.test.ts @@ -8,6 +8,16 @@ import { describe, it, expect, vi, beforeEach, afterEach } from "vitest"; import { NextRequest } from "next/server"; +// The per-account throttle has its own suite (login-throttle.test.ts); here +// it lets every attempt through and counts nothing. +vi.mock("@/lib/auth/login-throttle", () => ({ + beginAccountLoginAttempt: vi.fn(async () => ({ + waiting: null, + source: null, + failed: async () => {}, + succeeded: async () => {}, + })), +})); vi.mock("@/lib/db", () => ({ prisma: { user: { findFirst: vi.fn() }, diff --git a/src/app/api/auth/login/__tests__/trusted-device.test.ts b/src/app/api/auth/login/__tests__/trusted-device.test.ts index c6405653a..95b7a3492 100644 --- a/src/app/api/auth/login/__tests__/trusted-device.test.ts +++ b/src/app/api/auth/login/__tests__/trusted-device.test.ts @@ -1,6 +1,16 @@ import { describe, it, expect, vi, beforeEach } from "vitest"; import { NextRequest } from "next/server"; +// The per-account throttle has its own suite (login-throttle.test.ts); here +// it lets every attempt through and counts nothing. +vi.mock("@/lib/auth/login-throttle", () => ({ + beginAccountLoginAttempt: vi.fn(async () => ({ + waiting: null, + source: null, + failed: async () => {}, + succeeded: async () => {}, + })), +})); vi.mock("@/lib/db", () => ({ prisma: { user: { findFirst: vi.fn() }, diff --git a/src/app/api/auth/login/route.ts b/src/app/api/auth/login/route.ts index 2c07466cb..75dc974d6 100644 --- a/src/app/api/auth/login/route.ts +++ b/src/app/api/auth/login/route.ts @@ -9,12 +9,8 @@ import { safeJson, sanitiseZodIssues, } from "@/lib/api-response"; -import { - checkAuthSurfaceRateLimit, - checkRateLimit, - rateLimitHeaders, - refundRateLimit, -} from "@/lib/rate-limit"; +import { checkAuthSurfaceRateLimit, rateLimitHeaders } from "@/lib/rate-limit"; +import { beginAccountLoginAttempt } from "@/lib/auth/login-throttle"; import { ensureDbCompatibility } from "@/lib/db-compat"; import { NextRequest, NextResponse } from "next/server"; import { apiHandler } from "@/lib/api-handler"; @@ -26,15 +22,6 @@ import { syncMfaEnrollCookie } from "@/lib/auth/mfa-enrollment"; import { isOidcOnly } from "@/lib/auth/oidc"; import { readPendingLink } from "@/lib/auth/oidc-pending-link"; -/** - * Failed password sign-ins one account takes per window, from any number of - * addresses. Twice the per-IP ceiling, so the owner mistyping from one network - * meets that one first, and small enough that guessing is hopeless. - */ -const LOGIN_ACCOUNT_BUCKET = "auth:login:account"; -const LOGIN_ACCOUNT_LIMIT = 10; -const LOGIN_ACCOUNT_WINDOW_MS = 15 * 60 * 1000; - export const POST = apiHandler(async (request: NextRequest) => { // OIDC_ONLY means password login must be a dead end, not just a hidden // button — otherwise a pre-existing password (or a leaked/reused one) @@ -105,27 +92,31 @@ export const POST = apiHandler(async (request: NextRequest) => { }, }); - // A per-account ceiling beside the per-IP one. The IP bucket alone is only - // as good as the address it is keyed on, and a guesser with many addresses - // (or a proxy that forwards whatever address the caller claims) could try - // one account's password without end. This bucket follows the account - // wherever the guesses come from. - // - // Keyed on the account when the identifier resolves and on the identifier's - // hash when it does not, with the same ceiling, so a locked answer says - // nothing about whether the account exists. Charged before the Argon2id - // verification (a locked account costs no hashing) and refunded on a - // successful sign-in, so it counts failures: the owner signing in does not - // use it up. While it is spent, a passkey or single sign-on still works. - const accountBucket = user - ? `${LOGIN_ACCOUNT_BUCKET}:u:${user.id}` - : `${LOGIN_ACCOUNT_BUCKET}:i:${hashToken(identifier.toLowerCase())}`; - const accountRl = await checkRateLimit( - accountBucket, - LOGIN_ACCOUNT_LIMIT, - LOGIN_ACCOUNT_WINDOW_MS, - ); - if (!accountRl.allowed) { + // Under an SSO-only policy the one password sign-in allowed is the one that + // confirms the account a pending single sign-on link names. Any other + // account is refused BEFORE its password is checked: refusing after the + // check answered "wrong password" and "right password, wrong account" + // differently, which made this exception an oracle for every other + // account's password. + if (pendingLink && pendingLink.userId !== user?.id) { + return apiError("Password login is disabled. Sign in with SSO.", 403, { + errorCode: "oidc_only", + }); + } + + // The per-account half of the throttle (`@/lib/auth/login-throttle`): a + // place this account has signed in from is neither counted nor held back, + // anywhere else waits longer after each failure past the first few, up to + // fifteen minutes. Decided before the Argon2id verification, so a waiting + // attempt costs no hashing. + const attempt = await beginAccountLoginAttempt({ + user, + identifier, + request, + ip: rl.ip, + }); + annotate({ meta: { login_known_source: attempt.source ?? "none" } }); + if (attempt.waiting) { await auditLog("auth.login.account_throttled", { userId: user?.id, ipAddress: ip, @@ -137,7 +128,7 @@ export const POST = apiHandler(async (request: NextRequest) => { data: null, error: "Too many login attempts. Please try again later.", }, - { status: 429, headers: rateLimitHeaders(accountRl) }, + { status: 429, headers: rateLimitHeaders(attempt.waiting) }, ); } @@ -154,6 +145,7 @@ export const POST = apiHandler(async (request: NextRequest) => { const valid = await verifyPasswordOrDummy(user?.passwordHash, password); if (!user || !user.passwordHash) { + await attempt.failed(); // v1.4.43 W3-SECURITY (H-1): never write the typed identifier into // the audit row — `reason` already tells the operator what // happened, and PII must not land in operator artefacts. The @@ -168,6 +160,7 @@ export const POST = apiHandler(async (request: NextRequest) => { } if (!valid) { + await attempt.failed(); await auditLog("auth.login.failed", { userId: user.id, ipAddress: ip, @@ -176,14 +169,8 @@ export const POST = apiHandler(async (request: NextRequest) => { return apiError("Invalid credentials", 401); } - if (pendingLink && pendingLink.userId !== user.id) { - return apiError("Password login is disabled. Sign in with SSO.", 403, { - errorCode: "oidc_only", - }); - } - // The password was right: this attempt was not a failed guess. - await refundRateLimit(accountBucket); + await attempt.succeeded(); const ua = request.headers.get("user-agent"); diff --git a/src/lib/auth/__tests__/login-throttle.test.ts b/src/lib/auth/__tests__/login-throttle.test.ts new file mode 100644 index 000000000..b478cdd09 --- /dev/null +++ b/src/lib/auth/__tests__/login-throttle.test.ts @@ -0,0 +1,222 @@ +/** + * The per-account password throttle, against an in-memory rate-limit table. + * + * The case it exists for: somebody who is not the owner types wrong passwords + * for the account from two addresses. That used to lock a password-only + * account (the phone app's password sign-in with it) for as long as they kept + * it up. Now the owner's own device or network is not held back at all, and + * anywhere else waits a bounded, growing time. + */ +import { beforeEach, describe, expect, it, vi } from "vitest"; + +const table = new Map(); +let now = Date.parse("2026-09-26T12:00:00Z"); + +vi.mock("@/lib/rate-limit", () => ({ + checkRateLimit: vi.fn( + async (key: string, limit: number, windowMs: number) => { + const row = table.get(key); + const fresh = !row || row.resetAt < now; + const next = fresh + ? { count: 1, resetAt: now + windowMs } + : { count: row.count + 1, resetAt: row.resetAt }; + table.set(key, next); + return { + allowed: next.count <= limit, + limit, + remaining: Math.max(0, limit - next.count), + resetAt: next.resetAt, + }; + }, + ), + refundRateLimit: vi.fn(async (key: string) => { + const row = table.get(key); + if (row && row.resetAt >= now) row.count = Math.max(0, row.count - 1); + }), +})); + +vi.mock("@/lib/db", () => ({ + prisma: { + rateLimit: { + findUnique: vi.fn(async ({ where }: { where: { key: string } }) => { + const row = table.get(where.key); + return row ? { resetAt: new Date(row.resetAt) } : null; + }), + upsert: vi.fn( + async ({ + where, + create, + }: { + where: { key: string }; + create: { count: number; resetAt: Date }; + }) => { + table.set(where.key, { + count: create.count, + resetAt: create.resetAt.getTime(), + }); + }, + ), + }, + trustedDevice: { findUnique: vi.fn(async () => null) }, + refreshToken: { findFirst: vi.fn(async () => null) }, + session: { findFirst: vi.fn(async () => null) }, + }, +})); + +let deviceCookie: string | undefined; +vi.mock("next/headers", () => ({ + cookies: vi.fn(async () => ({ + get: () => (deviceCookie ? { value: deviceCookie } : undefined), + })), +})); +vi.mock("@/lib/auth/hmac", () => ({ hashToken: (v: string) => `h(${v})` })); + +import { prisma } from "@/lib/db"; +import { + beginAccountLoginAttempt, + BASE_WAIT_MS, + FREE_FAILURES, + MAX_WAIT_MS, + waitAfter, +} from "../login-throttle"; + +const OWNER = { id: "owner-1" }; + +function req(headers: Record = {}): Request { + return new Request("http://localhost/api/auth/login", { + method: "POST", + headers, + }); +} + +/** One password attempt; returns "waiting" or whether it was checked. */ +async function guess( + ip: string, + opts: { right?: boolean; headers?: Record } = {}, +): Promise<"waiting" | "checked"> { + const attempt = await beginAccountLoginAttempt({ + user: OWNER, + identifier: "owner", + request: req(opts.headers), + ip, + }); + if (attempt.waiting) return "waiting"; + if (opts.right) await attempt.succeeded(); + else await attempt.failed(); + return "checked"; +} + +beforeEach(() => { + vi.clearAllMocks(); + table.clear(); + now = Date.parse("2026-09-26T12:00:00Z"); + vi.spyOn(Date, "now").mockImplementation(() => now); + deviceCookie = undefined; + vi.mocked(prisma.trustedDevice.findUnique).mockResolvedValue(null); + vi.mocked(prisma.refreshToken.findFirst).mockResolvedValue(null); + vi.mocked(prisma.session.findFirst).mockResolvedValue(null); +}); + +describe("waitAfter", () => { + it("is free for the first few failures, then doubles up to the cap", () => { + expect(waitAfter(FREE_FAILURES - 1)).toBe(0); + expect(waitAfter(FREE_FAILURES)).toBe(BASE_WAIT_MS); + expect(waitAfter(FREE_FAILURES + 1)).toBe(2 * BASE_WAIT_MS); + expect(waitAfter(FREE_FAILURES + 100)).toBe(MAX_WAIT_MS); + }); +}); + +describe("somebody guessing from two addresses", () => { + async function tenGuesses() { + const answers: string[] = []; + for (let i = 0; i < 10; i++) { + answers.push(await guess(i % 2 ? "198.51.100.1" : "198.51.100.2")); + } + return answers; + } + + it("is slowed down, not given a lock that lasts", async () => { + const answers = await tenGuesses(); + // The first five are checked, then the wait holds the rest back. + expect(answers.slice(0, FREE_FAILURES)).toEqual( + Array(FREE_FAILURES).fill("checked"), + ); + expect(answers.slice(FREE_FAILURES)).toContain("waiting"); + }); + + it("the owner's phone, known by the device id it signed in with, still gets through", async () => { + await tenGuesses(); + vi.mocked(prisma.refreshToken.findFirst).mockImplementation((async (args: { + where: { deviceId?: string }; + }) => + args.where.deviceId === "owner-phone" ? { id: "rt-1" } : null) as never); + expect( + await guess("203.0.113.50", { + right: true, + headers: { "x-device-id": "owner-phone" }, + }), + ).toBe("checked"); + }); + + it("the owner's remembered browser still gets through", async () => { + await tenGuesses(); + deviceCookie = "device-token"; + vi.mocked(prisma.trustedDevice.findUnique).mockResolvedValue({ + userId: "owner-1", + expiresAt: new Date(now + 1e9), + } as never); + expect(await guess("203.0.113.51", { right: true })).toBe("checked"); + }); + + it("the owner's home network, seen on a recent session, still gets through", async () => { + await tenGuesses(); + vi.mocked(prisma.session.findFirst).mockImplementation((async (args: { + where: { ipAddress?: string }; + }) => + args.where.ipAddress === "203.0.113.9" ? { id: "s-1" } : null) as never); + expect(await guess("203.0.113.9", { right: true })).toBe("checked"); + }); + + it("a place the account has signed in from never counts toward the wait", async () => { + vi.mocked(prisma.session.findFirst).mockResolvedValue({ + id: "s-1", + } as never); + for (let i = 0; i < 20; i++) { + expect(await guess("203.0.113.9")).toBe("checked"); + } + vi.mocked(prisma.session.findFirst).mockResolvedValue(null); + expect(await guess("198.51.100.1")).toBe("checked"); + }); + + it("an unknown place waits at most fifteen minutes, however long the guessing went on", async () => { + for (let i = 0; i < 40; i++) { + await guess("198.51.100.1"); + now += MAX_WAIT_MS; // the guesser comes back as soon as allowed + } + await guess("198.51.100.1"); // one more failure, the longest wait + now += MAX_WAIT_MS + 1; + expect(await guess("203.0.113.77", { right: true })).toBe("checked"); + }); + + it("a right password gives its attempt back", async () => { + await guess("198.51.100.1"); + await guess("198.51.100.1", { right: true }); + expect(table.get("auth:login:account:u:owner-1")?.count).toBe(1); + }); +}); + +describe("an identifier that names no account", () => { + it("runs the same arithmetic on its hash", async () => { + for (let i = 0; i < FREE_FAILURES + 1; i++) { + const attempt = await beginAccountLoginAttempt({ + user: null, + identifier: "Nobody", + request: req(), + ip: "198.51.100.1", + }); + await attempt.failed(); + } + expect(table.has("auth:login:account:i:h(nobody):wait")).toBe(true); + expect(prisma.session.findFirst).not.toHaveBeenCalled(); + }); +}); diff --git a/src/lib/auth/login-throttle.ts b/src/lib/auth/login-throttle.ts new file mode 100644 index 000000000..1420e95ef --- /dev/null +++ b/src/lib/auth/login-throttle.ts @@ -0,0 +1,194 @@ +/** + * The per-account half of the password sign-in throttle. + * + * The per-IP bucket on the login route is only as good as the address it is + * keyed on, so a second limit follows the account wherever the guesses come + * from. It used to be a hard ceiling: ten failures in fifteen minutes and the + * account refused every password, charged before the password was checked. + * That made the owner's own sign-in a thing anybody could switch off. Ten + * wrong guesses from two addresses locked a password-only account, the phone + * app's password sign-in with it, and repeating them every fifteen minutes + * kept it locked for as long as the person bothered. + * + * Two changes answer that. + * + * A place the account has signed in from before is not counted and not + * blocked. "Before" means a remembered browser (the trusted-device cookie, + * checked without being spent), a phone that presents the device id a login + * of this account was issued to, or an address a session or device login of + * this account was created from in the last thirty days. The guesser is by + * definition somewhere the account has not signed in from; the owner mostly + * is not. The per-IP limit still applies to every caller. + * + * Everywhere else gets a growing wait instead of a wall. Five failures a day + * cost nothing; after that each failure makes the next attempt wait twice as + * long as the last, starting at thirty seconds and never more than fifteen + * minutes. A guesser gets a handful of tries an hour, which is as hopeless as + * the old ceiling, and the owner on a new network waits minutes at worst, + * never indefinitely. + * + * An identifier that names no account runs the same arithmetic on its hash, + * so a wait says nothing about whether the account exists. + */ +import type { User } from "@/generated/prisma/client"; +import { prisma } from "@/lib/db"; +import { hashToken } from "@/lib/auth/hmac"; +import { TRUSTED_DEVICE_COOKIE } from "@/lib/auth/trusted-device"; +import { checkRateLimit, refundRateLimit } from "@/lib/rate-limit"; +import type { RateLimitSnapshot } from "@/lib/rate-limit-context"; +import { cookies } from "next/headers"; + +const BUCKET = "auth:login:account"; +/** Failures a day an unknown place may make before it starts waiting. */ +export const FREE_FAILURES = 5; +const FAILURE_WINDOW_MS = 24 * 60 * 60 * 1000; +/** The first wait, doubled by every further failure. */ +export const BASE_WAIT_MS = 30 * 1000; +/** No wait is ever longer than this. */ +export const MAX_WAIT_MS = 15 * 60 * 1000; +/** How far back a sign-in from an address still makes it a known place. */ +const KNOWN_ADDRESS_WINDOW_MS = 30 * 24 * 60 * 60 * 1000; +/** + * The failure counter is read, not enforced, so its ceiling only has to be + * out of reach; `limit - remaining` is the count. + */ +const COUNTER_CEILING = 1_000_000; + +export type KnownSource = "trusted_device" | "device_id" | "address"; + +/** + * Has this account signed in from where this request comes from? Read-only: + * nothing here spends the trusted device or touches a row. + */ +export async function knownSignInSource( + userId: string, + request: Request, + ip: string | null, +): Promise { + const now = new Date(); + + let deviceToken: string | undefined; + try { + deviceToken = (await cookies()).get(TRUSTED_DEVICE_COOKIE)?.value; + } catch { + deviceToken = undefined; + } + if (deviceToken) { + const row = await prisma.trustedDevice.findUnique({ + where: { tokenHash: hashToken(deviceToken) }, + select: { userId: true, expiresAt: true }, + }); + if (row && row.userId === userId && row.expiresAt > now) { + return "trusted_device"; + } + } + + const deviceId = request.headers.get("x-device-id")?.trim(); + if (deviceId && deviceId.length <= 200) { + const login = await prisma.refreshToken.findFirst({ + where: { userId, deviceId }, + select: { id: true }, + }); + if (login) return "device_id"; + } + + if (ip) { + const since = new Date(now.getTime() - KNOWN_ADDRESS_WINDOW_MS); + const [session, login] = await Promise.all([ + prisma.session.findFirst({ + where: { userId, ipAddress: ip, createdAt: { gte: since } }, + select: { id: true }, + }), + prisma.refreshToken.findFirst({ + where: { userId, ipAddress: ip, createdAt: { gte: since } }, + select: { id: true }, + }), + ]); + if (session || login) return "address"; + } + + return null; +} + +/** How long the next attempt waits after this many failures in a day. */ +export function waitAfter(failures: number): number { + if (failures < FREE_FAILURES) return 0; + const doublings = Math.min(failures - FREE_FAILURES, 20); + return Math.min(BASE_WAIT_MS * 2 ** doublings, MAX_WAIT_MS); +} + +export interface AccountLoginAttempt { + /** Set when the attempt must not proceed: the wait still running. */ + waiting: RateLimitSnapshot | null; + /** Where the attempt comes from, when the account has signed in there. */ + source: KnownSource | null; + /** The password was wrong. */ + failed(): Promise; + /** The password was right: this was not a guess. */ + succeeded(): Promise; +} + +const noop = async () => {}; + +/** + * Decide whether a password attempt against this account (or this unknown + * identifier) may be checked at all, before the Argon2id verification runs. + */ +export async function beginAccountLoginAttempt(args: { + user: Pick | null; + identifier: string; + request: Request; + ip: string | null; +}): Promise { + const { user, identifier, request, ip } = args; + + if (user) { + const source = await knownSignInSource(user.id, request, ip); + if (source) { + return { waiting: null, source, failed: noop, succeeded: noop }; + } + } + + const key = user + ? `${BUCKET}:u:${user.id}` + : `${BUCKET}:i:${hashToken(identifier.toLowerCase())}`; + const waitKey = `${key}:wait`; + + const wait = await prisma.rateLimit.findUnique({ + where: { key: waitKey }, + select: { resetAt: true }, + }); + if (wait && wait.resetAt.getTime() > Date.now()) { + return { + waiting: { + limit: FREE_FAILURES, + remaining: 0, + resetAt: wait.resetAt.getTime(), + }, + source: null, + failed: noop, + succeeded: noop, + }; + } + + // Charged before the verification so a failure is counted even if the + // request dies after it; given back when the password turns out right. + const counted = await checkRateLimit(key, COUNTER_CEILING, FAILURE_WINDOW_MS); + const failures = counted.limit - counted.remaining; + + return { + waiting: null, + source: null, + failed: async () => { + const ms = waitAfter(failures); + if (ms === 0) return; + const resetAt = new Date(Date.now() + ms); + await prisma.rateLimit.upsert({ + where: { key: waitKey }, + create: { key: waitKey, count: 1, resetAt }, + update: { count: 1, resetAt }, + }); + }, + succeeded: () => refundRateLimit(key), + }; +} diff --git a/src/lib/openapi/routes/auth.ts b/src/lib/openapi/routes/auth.ts index 961b25e76..fc4baf4df 100644 --- a/src/lib/openapi/routes/auth.ts +++ b/src/lib/openapi/routes/auth.ts @@ -671,7 +671,8 @@ export const authPaths: NonNullable = { summary: "Email-or-username login (password)", description: "Browser callers receive a session cookie. Native callers (X-Client-Type: native or HealthLog-iOS UA prefix) additionally receive a paired access + refresh token.\n\n" + - "v1.23 — when the account has a confirmed second factor, the response carries no session/token. It returns HTTP 200 with `data: null, error: null` and `meta.mfaRequired: true` plus a single-use `meta.mfaTicket` and the `meta.methods` list. The client must POST the ticket + a code to `/api/auth/mfa/verify` to obtain the token bundle. Accounts without MFA are unchanged.", + "v1.23 — when the account has a confirmed second factor, the response carries no session/token. It returns HTTP 200 with `data: null, error: null` and `meta.mfaRequired: true` plus a single-use `meta.mfaTicket` and the `meta.methods` list. The client must POST the ticket + a code to `/api/auth/mfa/verify` to obtain the token bundle. Accounts without MFA are unchanged.\n\n" + + "Two limits apply. Per source address, five attempts in fifteen minutes. Per account, failed passwords from a place the account has not signed in from before make the next attempt wait: five failures a day are free, then each one doubles the wait, from thirty seconds up to at most fifteen minutes. A place the account has signed in from is not counted and not held back: a remembered browser (trusted-device cookie), a phone that sends the `X-Device-Id` a login of this account was issued to, or an address a session or device login of this account came from in the last thirty days. Both answer 429 with `Retry-After`. An identifier that names no account is limited the same way, so a 429 says nothing about whether it exists.", requestBody: { required: true, content: { "application/json": { schema: loginPasswordSchema } }, @@ -691,7 +692,7 @@ export const authPaths: NonNullable = { }, "403": { description: - "Password login is disabled — the operator runs `OIDC_ONLY=true`. `meta.errorCode` = `oidc_only`; sign in through SSO instead. The one exception is a browser the SSO callback sent back with a pending link to confirm an existing account: it may sign in with that one account's password, which links the account.", + "Password login is disabled — the operator runs `OIDC_ONLY=true`. `meta.errorCode` = `oidc_only`; sign in through SSO instead. The one exception is a browser the SSO callback sent back with a pending link to confirm an existing account: it may sign in with that one account's password, which links the account. A sign-in to any other account on that browser is refused before its password is checked, so the answer is the same whether the password was right or wrong.", content: { "application/json": { schema: errorEnvelope } }, }, ...stdResponses, diff --git a/tests/integration/oidc-pending-link.test.ts b/tests/integration/oidc-pending-link.test.ts index 10a17d90d..19b97c781 100644 --- a/tests/integration/oidc-pending-link.test.ts +++ b/tests/integration/oidc-pending-link.test.ts @@ -178,6 +178,17 @@ describe("under an SSO-only policy", () => { expect((await linkOf("acct-a")).oidcSub).toBe("idp-sub-1"); }); + it("answers the same for another account's right and wrong password", async () => { + // Refused before the password is checked, so the exception cannot be + // used to test passwords of accounts the link does not name. + pendingLinkFor("acct-a"); + const right = await passwordLogin("acct-b"); + const wrong = await passwordLogin("acct-b", "not the password"); + expect(right.status).toBe(403); + expect(wrong.status).toBe(403); + expect(await wrong.json()).toEqual(await right.json()); + }); + it("keeps it closed for any other account", async () => { pendingLinkFor("acct-a"); const res = await passwordLogin("acct-b"); From 7d05f68f235a870705d32c97599fcddbf5799969 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Bombeck?= Date: Sat, 26 Sep 2026 22:45:51 +0200 Subject: [PATCH 6/9] fix(http): say when proxy headers go unread, and trust X-Real-IP only at one hop With TRUST_PROXY_HOPS=0 behind a proxy that sends only X-Real-IP, every client address resolves to nothing and all anonymous sign-ins share one rate-limit bucket, with nothing in the log to say why. The first request that carries a forwarding header the setting does not let HealthLog read now logs one warning naming the header and the setting to change. X-Real-IP stood in for a missing X-Forwarded-For at any hop count. It is a single address, so it is now read only with TRUST_PROXY_HOPS=1; with two or more hops the chain the count describes is X-Forwarded-For. The upload, import and native batch routes that bypass the proxy now carry Cross-Origin-Resource-Policy and Cross-Origin-Embedder-Policy like every other response, and so do the proxy's early exits. The env examples described an unset TRUST_PROXY_HOPS as right for a directly exposed host; unset means one hop, and that host wants 0. --- .env.example | 9 ++-- .env.production.example | 7 +-- docs/self-hosting/reverse-proxy.md | 16 ++++-- .../proxy-early-exit-headers.test.ts | 24 +++++++++ src/lib/__tests__/get-client-ip.test.ts | 45 ++++++++++++++++ src/lib/api-response.ts | 52 +++++++++++++++++-- src/lib/http/proxy-bypass-routes.ts | 6 +++ 7 files changed, 142 insertions(+), 17 deletions(-) diff --git a/.env.example b/.env.example index 800b3f353..10bc7095e 100644 --- a/.env.example +++ b/.env.example @@ -202,10 +202,11 @@ API_TOKEN_HMAC_KEY="REPLACE-WITH-OUTPUT-OF-openssl-rand-hex-32" # operator — there is no way for the server to detect whether the flag # matches the real deployment topology. When in doubt, leave it off. # Number of trusted reverse-proxy hops in front of the app. The client IP is -# read that many entries from the right of the X-Forwarded-For chain. Leave -# unset for a directly-exposed host; set to 1 behind a single reverse proxy. -# A wrong value makes the app refuse to read XFF and share one rate-limit -# bucket across all anonymous callers. +# read that many entries from the right of the X-Forwarded-For chain. Set 0 +# for a directly-exposed host; 1 (the default when unset) behind a single +# reverse proxy. X-Real-IP is read only at 1. A wrong value makes the app +# refuse the headers, log one warning, and share one rate-limit bucket across +# all anonymous callers. # TRUST_PROXY_HOPS="1" # TRUST_CF_CONNECTING_IP="" diff --git a/.env.production.example b/.env.production.example index 9e9a2a949..af58314bd 100644 --- a/.env.production.example +++ b/.env.production.example @@ -473,9 +473,10 @@ API_TOKEN_HMAC_KEY="" # ONLY when Cloudflare is in front -- a direct-exposed self-host must leave # it unset, or an attacker could forge the header. Off by default. # Number of trusted reverse-proxy hops in front of the app; the client IP is -# read that many entries from the right of the X-Forwarded-For chain. Unset -# for a directly-exposed host, 1 behind a single reverse proxy. A wrong -# value makes the app refuse XFF and collapse anonymous callers into one +# read that many entries from the right of the X-Forwarded-For chain. 0 for a +# directly-exposed host, 1 (the default when unset) behind a single reverse +# proxy; X-Real-IP is read only at 1. A wrong value makes the app refuse the +# headers, log one warning, and collapse anonymous callers into one # rate-limit bucket. # TRUST_PROXY_HOPS="1" # TRUST_CF_CONNECTING_IP="1" diff --git a/docs/self-hosting/reverse-proxy.md b/docs/self-hosting/reverse-proxy.md index 6ef02203b..5ce2db75f 100644 --- a/docs/self-hosting/reverse-proxy.md +++ b/docs/self-hosting/reverse-proxy.md @@ -52,11 +52,17 @@ every anonymous caller into one shared rate-limit bucket. Match the value to the actual hop count. `X-Real-IP` gets the same trust as `X-Forwarded-For`. It is read only -when `TRUST_PROXY_HOPS` is `1` or more and the request carries no -`X-Forwarded-For`, which covers a proxy that sets only `X-Real-IP`. With -`0` neither header is believed, because without a proxy in front both -come from the caller. If your proxy sets only `X-Real-IP` and you had -set `0` for it, set `1` instead. +when `TRUST_PROXY_HOPS` is exactly `1` and the request carries no +`X-Forwarded-For`, which covers a single proxy that sets only +`X-Real-IP`. With `0` neither header is believed, because without a proxy +in front both come from the caller. With `2` or more, the hop count +describes an `X-Forwarded-For` chain, so have your proxies send one. + +If requests arrive with a forwarding header the setting does not let +HealthLog read, the log shows one warning naming the header and the +setting. Every anonymous caller then shares one rate-limit bucket, so one +person mistyping a password can hold sign-in back for everyone. The usual +cause is `0` behind a proxy that sends only `X-Real-IP`: set `1`. ## Caddy diff --git a/src/__tests__/proxy-early-exit-headers.test.ts b/src/__tests__/proxy-early-exit-headers.test.ts index 9095f4105..bfa02f8c5 100644 --- a/src/__tests__/proxy-early-exit-headers.test.ts +++ b/src/__tests__/proxy-early-exit-headers.test.ts @@ -23,6 +23,7 @@ vi.mock("@/lib/process-type", () => ({ })); import { proxy } from "../proxy"; +import { applyBypassRouteHeaders } from "@/lib/http/proxy-bypass-routes"; function makeRequest( pathname: string, @@ -60,6 +61,10 @@ function expectBaselineHeaders(res: Response) { ); expect(res.headers.get("Permissions-Policy")).toContain("camera=()"); expect(res.headers.get("Cross-Origin-Opener-Policy")).toBe("same-origin"); + expect(res.headers.get("Cross-Origin-Resource-Policy")).toBe("same-origin"); + expect(res.headers.get("Cross-Origin-Embedder-Policy")).toBe( + "credentialless", + ); expect(res.headers.get("X-Permitted-Cross-Domain-Policies")).toBe("none"); expect(res.headers.get("Strict-Transport-Security")).toContain( "max-age=31536000", @@ -117,3 +122,22 @@ describe("proxy early exits carry the baseline security headers", () => { expect(res.headers.get("X-Frame-Options")).toBe("DENY"); }); }); + +describe("the routes that bypass the proxy", () => { + it("carry the same cross-origin headers the proxy sets on a full response", () => { + const viaProxy = proxy(makeRequest("/api/health")); + const headers = new Headers(); + applyBypassRouteHeaders(headers); + for (const name of [ + "Cross-Origin-Opener-Policy", + "Cross-Origin-Resource-Policy", + "Cross-Origin-Embedder-Policy", + "X-Content-Type-Options", + "X-Frame-Options", + "X-Permitted-Cross-Domain-Policies", + "Strict-Transport-Security", + ]) { + expect(headers.get(name), name).toBe(viaProxy.headers.get(name)); + } + }); +}); diff --git a/src/lib/__tests__/get-client-ip.test.ts b/src/lib/__tests__/get-client-ip.test.ts index 56b082589..56d4aec47 100644 --- a/src/lib/__tests__/get-client-ip.test.ts +++ b/src/lib/__tests__/get-client-ip.test.ts @@ -3,6 +3,7 @@ import { getClientIp, getClientIpOrTrustWarning, _resetTrustViolationWarningForTests, + _resetUnreadProxyHeadersWarningForTests, } from "../api-response"; const ORIGINAL_HOPS = process.env.TRUST_PROXY_HOPS; @@ -12,6 +13,7 @@ beforeEach(() => { delete process.env.TRUST_PROXY_HOPS; delete process.env.TRUST_CF_CONNECTING_IP; _resetTrustViolationWarningForTests(); + _resetUnreadProxyHeadersWarningForTests(); }); afterEach(() => { @@ -202,6 +204,49 @@ describe("getClientIp trust-violation warning (F-6, 2026-05-16)", () => { * without Cloudflare in front cannot be tricked by an attacker * setting the header directly. */ +describe("X-Real-IP with two or more hops, and headers the trust cannot read", () => { + it("does not read X-Real-IP when two hops are declared and XFF is missing", () => { + process.env.TRUST_PROXY_HOPS = "2"; + const warn = vi.spyOn(console, "warn").mockImplementation(() => {}); + try { + // A caller behind the outer proxy could have written it. + expect(getClientIp(makeRequest({ "x-real-ip": "9.9.9.9" }))).toBeNull(); + expect(warn).toHaveBeenCalledTimes(1); + expect(warn.mock.calls[0][0]).toMatch(/X-Real-IP/); + } finally { + warn.mockRestore(); + } + }); + + it("warns once when TRUST_PROXY_HOPS=0 and a proxy sends only X-Real-IP", () => { + process.env.TRUST_PROXY_HOPS = "0"; + const warn = vi.spyOn(console, "warn").mockImplementation(() => {}); + try { + for (let i = 0; i < 3; i++) { + expect( + getClientIp(makeRequest({ "x-real-ip": `10.0.0.${i}` })), + ).toBeNull(); + } + expect(warn).toHaveBeenCalledTimes(1); + expect(warn.mock.calls[0][0]).toMatch(/TRUST_PROXY_HOPS=0/); + expect(warn.mock.calls[0][0]).toMatch(/TRUST_PROXY_HOPS=1/); + } finally { + warn.mockRestore(); + } + }); + + it("stays quiet with TRUST_PROXY_HOPS=0 and no forwarding header", () => { + process.env.TRUST_PROXY_HOPS = "0"; + const warn = vi.spyOn(console, "warn").mockImplementation(() => {}); + try { + expect(getClientIp(makeRequest({}))).toBeNull(); + expect(warn).not.toHaveBeenCalled(); + } finally { + warn.mockRestore(); + } + }); +}); + describe("getClientIp Cloudflare cf-connecting-ip branch (v1.4.37)", () => { it("returns cf-connecting-ip when the env flag is on and the header is present", () => { process.env.TRUST_CF_CONNECTING_IP = "1"; diff --git a/src/lib/api-response.ts b/src/lib/api-response.ts index adcbf113c..c2403c1dd 100644 --- a/src/lib/api-response.ts +++ b/src/lib/api-response.ts @@ -490,6 +490,33 @@ function warnTrustViolationOnce(hops: number, chainLength: number): void { ); } +/** + * Once per process: forwarding headers arrived that the configured trust + * does not let us read, so the client address resolves to nothing. The + * common cause is `TRUST_PROXY_HOPS=0` behind a proxy that sends only + * `X-Real-IP` (nginx's usual setup), or `TRUST_PROXY_HOPS` of 2 or more with + * no `X-Forwarded-For`. Every anonymous caller then shares one rate-limit + * bucket, so one person mistyping a password can hold the sign-in back for + * everybody, and the operator had no way to see why. + */ +let unreadProxyHeadersWarned = false; + +/** Test-only reset for {@link warnUnreadProxyHeadersOnce}. */ +export function _resetUnreadProxyHeadersWarningForTests(): void { + unreadProxyHeadersWarned = false; +} + +function warnUnreadProxyHeadersOnce(hops: number, header: string): void { + if (unreadProxyHeadersWarned) return; + unreadProxyHeadersWarned = true; + console.warn( + `[getClientIp] requests carry ${header} but TRUST_PROXY_HOPS=${hops} does not allow reading it, so client addresses resolve to nothing and every anonymous caller shares one rate-limit bucket. ` + + (hops === 0 + ? "If a proxy you control sits in front of HealthLog, set TRUST_PROXY_HOPS=1." + : "X-Real-IP is only read with TRUST_PROXY_HOPS=1; with more hops, have the proxies send X-Forwarded-For."), + ); +} + /** * Tagged return shape so a caller can apply a tighter universal * rate-limit when the trust chain is misconfigured. F-6 (mobile security @@ -546,17 +573,32 @@ export function getClientIpOrTrustWarning(request: Request): { return { ip: null, trustViolation: true }; } // x-real-ip is a forwarding header like XFF and gets the same trust: only - // when the operator declares a proxy in front (hops > 0), and only as the + // when the operator declares a proxy in front, and only as the // single-proxy stand-in for a missing XFF. It used to be read on every // path, including `TRUST_PROXY_HOPS=0` ("no proxy, trust no header") and // the broken-chain path above, so a caller sending a fresh x-real-ip per // request got a fresh per-IP rate-limit bucket each time. + // + // Exactly one hop, not "one or more". X-Real-IP holds a single address, + // set by whichever proxy wrote it last; with two or more hops declared, + // the outer proxy may pass through whatever the caller put there, and + // the chain the hop count describes is the XFF chain, which is absent. const realIp = request.headers.get("x-real-ip")?.trim(); - return { - ip: realIp && looksLikeIp(realIp) ? realIp : null, - trustViolation: false, - }; + if (hops === 1) { + return { + ip: realIp && looksLikeIp(realIp) ? realIp : null, + trustViolation: false, + }; + } + if (realIp) warnUnreadProxyHeadersOnce(hops, "X-Real-IP"); + return { ip: null, trustViolation: false }; } + const unread = request.headers.has("x-forwarded-for") + ? "X-Forwarded-For" + : request.headers.has("x-real-ip") + ? "X-Real-IP" + : null; + if (unread) warnUnreadProxyHeadersOnce(hops, unread); return { ip: null, trustViolation: false }; } diff --git a/src/lib/http/proxy-bypass-routes.ts b/src/lib/http/proxy-bypass-routes.ts index 83e0e1466..d5aca39c9 100644 --- a/src/lib/http/proxy-bypass-routes.ts +++ b/src/lib/http/proxy-bypass-routes.ts @@ -70,6 +70,12 @@ export function setBaselineSecurityHeaders(headers: Headers): void { headers.set("Referrer-Policy", "strict-origin-when-cross-origin"); headers.set("Permissions-Policy", "camera=(), microphone=(), geolocation=()"); headers.set("Cross-Origin-Opener-Policy", "same-origin"); + // The same pair the proxy sets on every page and API response. The bypass + // routes (uploads, imports, native batches) answered without them, so a + // cross-origin page could embed one of their JSON answers as a no-cors + // subresource where every other route refuses it. + headers.set("Cross-Origin-Resource-Policy", "same-origin"); + headers.set("Cross-Origin-Embedder-Policy", "credentialless"); headers.set("X-Permitted-Cross-Domain-Policies", "none"); if (process.env.NODE_ENV !== "development") { headers.set( From 357b7426376258ffe5b61e17b9914d9ff9867afd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Bombeck?= Date: Sun, 27 Sep 2026 00:14:32 +0200 Subject: [PATCH 7/9] test(sharing): follow the invite form's reset through the re-proof flow --- .../sharing-recoverable-errors.test.tsx | 22 +++++++++---------- 1 file changed, 10 insertions(+), 12 deletions(-) diff --git a/src/__tests__/sharing-recoverable-errors.test.tsx b/src/__tests__/sharing-recoverable-errors.test.tsx index 05e6618cf..67f84b3ff 100644 --- a/src/__tests__/sharing-recoverable-errors.test.tsx +++ b/src/__tests__/sharing-recoverable-errors.test.tsx @@ -345,18 +345,16 @@ describe("a refused invitation keeps what was typed", () => { it("clears the form only where the invitation actually went", () => { // A static render cannot press the button, so the claim is made about the - // code path instead: every field reset lives inside `onSuccess`. A reset - // that migrated into `onError` — or out of the callbacks entirely — would - // wipe a typed identifier and eight ticked sections on a failure the - // person is being asked to retry. - const start = INVITE_CARD.indexOf("onSuccess:"); - const split = INVITE_CARD.indexOf("onError:", start); - // The options object ends at the first ` },` after the error arm, so - // the slice stops at the mutation rather than running into the JSX below, - // where a `setNarrowed(false)` / `setNarrowed(true)` on a radio would - // satisfy the matcher for the wrong reason. The success reset returns the - // scope to unset (`null`), which the JSX never writes. - const end = INVITE_CARD.indexOf("\n },", split); + // code path instead: every field reset sits after the awaited invitation + // inside `try`, and none in the `catch`. A reset that migrated into the + // failure arm would wipe a typed identifier and eight ticked sections on + // a failure the person is being asked to retry. The failure arm ends at + // the end of the submit handler, before the JSX below, where a + // `setNarrowed(false)` on a radio would satisfy the matcher for the wrong + // reason. + const start = INVITE_CARD.indexOf("await recentProof.run("); + const split = INVITE_CARD.indexOf("} catch (err) {", start); + const end = INVITE_CARD.indexOf("\n };", split); const onSuccess = INVITE_CARD.slice(start, split); const onError = INVITE_CARD.slice(split, end); From bd270d635a4cee75acd0b825cd0d45fccb5c752a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Bombeck?= Date: Sun, 27 Sep 2026 00:52:11 +0200 Subject: [PATCH 8/9] test(sharing): the invitation route's integration cases follow the recent-proof gate --- tests/integration/sharing-lifecycle.test.ts | 60 ++++++++++----------- 1 file changed, 28 insertions(+), 32 deletions(-) diff --git a/tests/integration/sharing-lifecycle.test.ts b/tests/integration/sharing-lifecycle.test.ts index efb0909c1..a948ddc80 100644 --- a/tests/integration/sharing-lifecycle.test.ts +++ b/tests/integration/sharing-lifecycle.test.ts @@ -563,42 +563,28 @@ describe("what an invitation opens, over the real route", () => { expect(await getPrismaClient().accountGrant.count()).toBe(0); }); - it("refuses MANAGE over Bearer with a code the client can act on", async () => { - // The decided consequence of a cookie-only step-up. Not a bug to work - // around: the native client cannot offer management of a health record, - // and it is told so in a code rather than by an authentication error on a - // request that authenticated fine. + it("refuses a Bearer invitation that presents no step-up elevation", async () => { + // v1.39.3: every invitation takes the recent-proof gate, and a token + // presents its proof as an `X-Step-Up` elevation. No shipped client + // offers access with a token. (MANAGE over Bearer still ends in 403 + // `sharing.invite.manage_browser_only` once an elevation is presented; + // the unit suite covers that arm.) const owner = await makeUser("owner"); const delegate = await makeUser("delegate"); const token = await mintToken(owner.id); cookieJar.clear(); headerJar.set("authorization", `Bearer ${token}`); - const response = await invite(delegate.username, undefined, { - access: "MANAGE", - }); - expect(response.status).toBe(403); - expect((await response.json()).meta.errorCode).toBe( - "sharing.invite.manage_browser_only", - ); + for (const access of ["READ", "WRITE", "MANAGE"]) { + const response = await invite(delegate.username, undefined, { access }); + expect(response.status).toBe(401); + expect((await response.json()).meta.errorCode).toBe( + "auth.stepup.required", + ); + } expect(await getPrismaClient().accountGrant.count()).toBe(0); }); - it("lets the same Bearer caller keep minting the two levels it always could", async () => { - const owner = await makeUser("owner"); - const delegate = await makeUser("delegate"); - const token = await mintToken(owner.id); - cookieJar.clear(); - headerJar.set("authorization", `Bearer ${token}`); - - const response = await invite(delegate.username, undefined, { - access: "WRITE", - scope: ["measurements"], - }); - expect(response.status).toBe(201); - expect((await response.json()).data.access).toBe("WRITE"); - }); - it("mints MANAGE from a browser session that carries no second factor", async () => { // `requireFreshMfaIfEnrolled` gates the enrolled cohort only. An account // with no second factor cannot produce a fresh-factor proof, so gating it @@ -640,7 +626,11 @@ describe("what an invitation opens, over the real route", () => { access: "MANAGE", }); expect(response.status).toBe(401); - expect((await response.json()).meta.errorCode).toBe("auth.stepup.required"); + // The recent-proof gate answers first; on an enrolled account it wants + // the same fresh second factor the MANAGE step-up does. + expect((await response.json()).meta.errorCode).toBe( + "auth.reproof.required", + ); expect(await getPrismaClient().accountGrant.count()).toBe(0); }); @@ -666,9 +656,10 @@ describe("what an invitation opens, over the real route", () => { expect(await getPrismaClient().accountGrant.count()).toBe(1); }); - it("does not ask a read invitation for a factor at all", async () => { - // Same stale-factor account. Reducing the friction to the level that - // needs it is the point; an owner sharing a reading re-proves nothing. + it("asks a read invitation for a recent proof too (v1.39.3)", async () => { + // Same stale-factor account. An invitation at any level outlives the + // session that sent it, so every level now takes the recent-proof gate; + // on an enrolled account that is a fresh second factor. const owner = await makeUser("owner"); const delegate = await makeUser("delegate"); await getPrismaClient().user.update({ @@ -681,7 +672,12 @@ describe("what an invitation opens, over the real route", () => { data: { mfaVerifiedAt: new Date(Date.now() - 60 * 60 * 1000) }, }); - expect((await invite(delegate.username)).status).toBe(201); + const response = await invite(delegate.username); + expect(response.status).toBe(401); + expect((await response.json()).meta.errorCode).toBe( + "auth.reproof.required", + ); + expect(await getPrismaClient().accountGrant.count()).toBe(0); }); }); From 9642c749c99bdaec98973c05d09ca39984d7269d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Bombeck?= Date: Sun, 27 Sep 2026 00:59:06 +0200 Subject: [PATCH 9/9] fix(auth): a new email address on an account with a second factor needs that factor The address is what single sign-on links an existing account by, and changing it took the password or a session younger than five minutes on every account. A remembered browser skips the second factor at sign-in and still makes a fresh session, so the password alone could move the address of an account the second factor was meant to protect. On an account with a second factor only a second factor or passkey completed in the last five minutes counts now; the password is not checked. The refusal names it (second_factor_required in rejectedFields, or 401 profile.update.emailSecondFactorRequired when the address was all that changed), and the profile form opens the re-proof dialog with the factors the account holds, then saves again, no longer offering the password field. Accounts without a second factor keep the password or a recent sign-in. A token cannot change the address of such an account; the app does not change it. --- docs/api/openapi.yaml | 54 +++++++------- messages/de.json | 6 +- messages/en.json | 6 +- messages/es.json | 6 +- messages/fr.json | 6 +- messages/it.json | 6 +- messages/ko.json | 6 +- messages/pl.json | 6 +- .../settings/account-section/index.tsx | 69 ++++++++++++++++-- .../enrollment-proof-second-factor.test.ts | 71 ++++++++++++++++++- .../profile-update-email-limit.test.ts | 30 ++++++++ src/lib/auth/existing-factor-proof.ts | 47 +++++++++--- src/lib/auth/profile-update.ts | 24 +++++-- src/lib/openapi/error-codes.ts | 1 + src/lib/openapi/routes/profile.ts | 2 +- src/lib/profile/rejected-fields.ts | 1 + 16 files changed, 280 insertions(+), 61 deletions(-) diff --git a/docs/api/openapi.yaml b/docs/api/openapi.yaml index 9aeb0e65d..20fecd825 100644 --- a/docs/api/openapi.yaml +++ b/docs/api/openapi.yaml @@ -23019,8 +23019,12 @@ components: a second factor in the last five minutes needs nothing more, otherwise this field must carry the current password. Without an accepted proof the address is not changed and comes back in `rejectedFields` as `reproof_required` or `reproof_failed` (or, when the address was the only field sent, 401 - `auth.reproof.required` / `auth.reproof.failed`). Attempts draw on the account's shared re-proof budget of - five per fifteen minutes. Never stored." + `auth.reproof.required` / `auth.reproof.failed`). On an account with a second factor the password is not + accepted and not checked: only a second factor or passkey completed in the last five minutes counts (at + sign-in or at POST /api/auth/reproof), and without one the address comes back as `second_factor_required` + (or 401 `profile.update.emailSecondFactorRequired` when it was the only field). A token cannot change the + address of such an account. Attempts draw on the account's shared re-proof budget of five per fifteen + minutes. Never stored." type: string maxLength: 512 description: Partial profile update. Every field is optional and an omitted field is left untouched. An explicit null @@ -31522,29 +31526,29 @@ components: onboarding.disclaimer.invalid, onboarding.restart.invalid, onboarding.restart.rateLimited, onboarding.tour.invalid; personal_records — personal_records.invalid_query; practitioner — practitioner.invalid; profile — profile.update.emailInUse, profile.update.emailRateLimited, - profile.update.invalidBody, profile.update.nothingSaved; record_write — record_write.rate_limited; - report-selection — report-selection.body.invalid_json, report-selection.body.invalid_shape, - report-selection.leaves.unknown; route — route.retired; share-link — share-link.document.decryptFailed, - share-link.document.notFound, share-link.document.rateLimited, share-link.documents.invalid, - share-link.report.notFound, share-link.report.rateLimited, share-link.selection.forbidden_leaf, - share-link.selection.unknown_leaf; sharing — sharing.accept.expired, sharing.accept.not_found, - sharing.accept.not_pending, sharing.accept.revoked, sharing.access.denied, sharing.invite.duplicate, - sharing.invite.invalid, sharing.invite.invalid_scope, sharing.invite.manage_browser_only, - sharing.invite.refused, sharing.invite.self, sharing.not_permitted, sharing.renounce.already_ended, - sharing.renounce.not_found, sharing.revoke.already_ended, sharing.revoke.not_found, - sharing.session.changed, sharing.switch.invalid, sharing.switch.wrong_transport; source-priority — - source-priority.body.invalid_json, source-priority.body.invalid_shape; tokens — - tokens.documents.ceiling_reached, tokens.measurements.ceiling_reached; vaccination — - vaccination.booster-invalid, vaccination.booster-no-antigen, vaccination.booster-out-of-scope, - vaccination.encounter-not-found, vaccination.identity-required, vaccination.invalid, - vaccination.link.invalid, vaccination.not-found, vaccination.practitioner-not-found; workout — - workout.batch.invalid, workout.batch.payload_too_large, workout.batch.too_large; (unprefixed) — - about_me_conflict, ai_response_truncated, coach_prefs_conflict, dashboard_layout_conflict, empty_file, - health_score_config_conflict, insights_layout_conflict, invalid_base_updated_at, - json_carries_no_intake_time, json_not_an_array, medication_layout_conflict, missing_required_columns, - modules_conflict, mood_tag_layout_conflict, not_configured, notification_prefs_conflict, oidc_only, - private_origin_not_approved, private_origin_not_grantable, rate_limited_self, too_many_rows, - unreadable_json, upstream_invalid_json, vapid_not_configured" + profile.update.emailSecondFactorRequired, profile.update.invalidBody, profile.update.nothingSaved; + record_write — record_write.rate_limited; report-selection — report-selection.body.invalid_json, + report-selection.body.invalid_shape, report-selection.leaves.unknown; route — route.retired; share-link + — share-link.document.decryptFailed, share-link.document.notFound, share-link.document.rateLimited, + share-link.documents.invalid, share-link.report.notFound, share-link.report.rateLimited, + share-link.selection.forbidden_leaf, share-link.selection.unknown_leaf; sharing — + sharing.accept.expired, sharing.accept.not_found, sharing.accept.not_pending, sharing.accept.revoked, + sharing.access.denied, sharing.invite.duplicate, sharing.invite.invalid, sharing.invite.invalid_scope, + sharing.invite.manage_browser_only, sharing.invite.refused, sharing.invite.self, sharing.not_permitted, + sharing.renounce.already_ended, sharing.renounce.not_found, sharing.revoke.already_ended, + sharing.revoke.not_found, sharing.session.changed, sharing.switch.invalid, + sharing.switch.wrong_transport; source-priority — source-priority.body.invalid_json, + source-priority.body.invalid_shape; tokens — tokens.documents.ceiling_reached, + tokens.measurements.ceiling_reached; vaccination — vaccination.booster-invalid, + vaccination.booster-no-antigen, vaccination.booster-out-of-scope, vaccination.encounter-not-found, + vaccination.identity-required, vaccination.invalid, vaccination.link.invalid, vaccination.not-found, + vaccination.practitioner-not-found; workout — workout.batch.invalid, workout.batch.payload_too_large, + workout.batch.too_large; (unprefixed) — about_me_conflict, ai_response_truncated, coach_prefs_conflict, + dashboard_layout_conflict, empty_file, health_score_config_conflict, insights_layout_conflict, + invalid_base_updated_at, json_carries_no_intake_time, json_not_an_array, medication_layout_conflict, + missing_required_columns, modules_conflict, mood_tag_layout_conflict, not_configured, + notification_prefs_conflict, oidc_only, private_origin_not_approved, private_origin_not_grantable, + rate_limited_self, too_many_rows, unreadable_json, upstream_invalid_json, vapid_not_configured" type: string capability: description: "On an AI refusal: the capability that is unavailable, one of the keys of `AiCapabilities` (`coach`, diff --git a/messages/de.json b/messages/de.json index 7ccfb920c..481e1d053 100644 --- a/messages/de.json +++ b/messages/de.json @@ -126,7 +126,8 @@ "nothingSaved": "Es wurde nichts gespeichert. Korrigiere das markierte Feld und versuche es erneut.", "invalidBody": "Die Profilaktualisierung konnte nicht gelesen werden.", "emailInUse": "Diese E-Mail-Adresse wird bereits verwendet.", - "emailRateLimited": "Zu viele Änderungen der E-Mail-Adresse. Versuche es später erneut." + "emailRateLimited": "Zu viele Änderungen der E-Mail-Adresse. Versuche es später erneut.", + "emailSecondFactorRequired": "Bestätige mit deinem zweiten Faktor oder einem Passkey, um die E-Mail-Adresse zu ändern." } }, "auth": { @@ -5568,7 +5569,8 @@ "rateLimited": "Zu oft geändert. Warte kurz und versuche es dann erneut.", "other": "Dieser Wert wurde nicht angenommen. Bitte prüfen und erneut versuchen.", "reproofRequired": "Gib dein aktuelles Passwort ein, um die E-Mail-Adresse zu ändern.", - "reproofFailed": "Das Passwort stimmt nicht, die E-Mail-Adresse wurde nicht geändert." + "reproofFailed": "Das Passwort stimmt nicht, die E-Mail-Adresse wurde nicht geändert.", + "secondFactorRequired": "Bestätige mit deinem zweiten Faktor oder einem Passkey, um die E-Mail-Adresse zu ändern." }, "avatar": { "title": "Profilfoto", diff --git a/messages/en.json b/messages/en.json index eeedef816..245380e7e 100644 --- a/messages/en.json +++ b/messages/en.json @@ -126,7 +126,8 @@ "nothingSaved": "Nothing was saved. Check the highlighted field and try again.", "invalidBody": "The profile update could not be read.", "emailInUse": "This email address is already in use.", - "emailRateLimited": "Too many email-address changes. Try again later." + "emailRateLimited": "Too many email-address changes. Try again later.", + "emailSecondFactorRequired": "Confirm with your second factor or a passkey to change the email address." } }, "auth": { @@ -5568,7 +5569,8 @@ "rateLimited": "Changed too often. Wait a little and try again.", "other": "This value was not accepted. Check it and try again.", "reproofRequired": "Enter your current password to change the email address.", - "reproofFailed": "That password is not correct, so the email address was not changed." + "reproofFailed": "That password is not correct, so the email address was not changed.", + "secondFactorRequired": "Confirm with your second factor or a passkey to change the email address." }, "avatar": { "title": "Profile photo", diff --git a/messages/es.json b/messages/es.json index 5856435ad..f7120439c 100644 --- a/messages/es.json +++ b/messages/es.json @@ -126,7 +126,8 @@ "nothingSaved": "No se guardó nada. Revisa el campo marcado e inténtalo de nuevo.", "invalidBody": "No se pudo leer la actualización del perfil.", "emailInUse": "Esta dirección de correo ya está en uso.", - "emailRateLimited": "Demasiados cambios de dirección de correo. Inténtalo más tarde." + "emailRateLimited": "Demasiados cambios de dirección de correo. Inténtalo más tarde.", + "emailSecondFactorRequired": "Confirma con tu segundo factor o una clave de acceso para cambiar el correo electrónico." } }, "auth": { @@ -5568,7 +5569,8 @@ "rateLimited": "Se ha cambiado demasiadas veces. Espera un poco e inténtalo de nuevo.", "other": "No se aceptó este valor. Revísalo e inténtalo de nuevo.", "reproofRequired": "Introduce tu contraseña actual para cambiar la dirección de correo.", - "reproofFailed": "La contraseña no es correcta, así que la dirección de correo no se ha cambiado." + "reproofFailed": "La contraseña no es correcta, así que la dirección de correo no se ha cambiado.", + "secondFactorRequired": "Confirma con tu segundo factor o una clave de acceso para cambiar el correo electrónico." }, "avatar": { "title": "Foto de perfil", diff --git a/messages/fr.json b/messages/fr.json index 50abc764c..bab109f9c 100644 --- a/messages/fr.json +++ b/messages/fr.json @@ -126,7 +126,8 @@ "nothingSaved": "Rien n'a été enregistré. Corrigez le champ signalé et réessayez.", "invalidBody": "La mise à jour du profil n'a pas pu être lue.", "emailInUse": "Cette adresse e-mail est déjà utilisée.", - "emailRateLimited": "Trop de changements d'adresse e-mail. Réessayez plus tard." + "emailRateLimited": "Trop de changements d'adresse e-mail. Réessayez plus tard.", + "emailSecondFactorRequired": "Confirmez avec votre second facteur ou une clé d'accès pour changer l'adresse e-mail." } }, "auth": { @@ -5568,7 +5569,8 @@ "rateLimited": "Modifié trop souvent. Patientez un peu et réessayez.", "other": "Cette valeur n'a pas été acceptée. Vérifiez-la et réessayez.", "reproofRequired": "Saisissez votre mot de passe actuel pour changer l'adresse e-mail.", - "reproofFailed": "Ce mot de passe est incorrect, l'adresse e-mail n'a donc pas été modifiée." + "reproofFailed": "Ce mot de passe est incorrect, l'adresse e-mail n'a donc pas été modifiée.", + "secondFactorRequired": "Confirmez avec votre second facteur ou une clé d'accès pour changer l'adresse e-mail." }, "avatar": { "title": "Photo de profil", diff --git a/messages/it.json b/messages/it.json index 941033101..061bf3ab3 100644 --- a/messages/it.json +++ b/messages/it.json @@ -126,7 +126,8 @@ "nothingSaved": "Non è stato salvato nulla. Controlla il campo evidenziato e riprova.", "invalidBody": "Impossibile leggere l'aggiornamento del profilo.", "emailInUse": "Questo indirizzo email è già in uso.", - "emailRateLimited": "Troppi cambi di indirizzo email. Riprova più tardi." + "emailRateLimited": "Troppi cambi di indirizzo email. Riprova più tardi.", + "emailSecondFactorRequired": "Conferma con il tuo secondo fattore o una passkey per cambiare l'indirizzo email." } }, "auth": { @@ -5568,7 +5569,8 @@ "rateLimited": "Modificato troppe volte. Aspetta un momento e riprova.", "other": "Questo valore non è stato accettato. Controllalo e riprova.", "reproofRequired": "Inserisci la password attuale per cambiare l'indirizzo email.", - "reproofFailed": "La password non è corretta, quindi l'indirizzo email non è stato cambiato." + "reproofFailed": "La password non è corretta, quindi l'indirizzo email non è stato cambiato.", + "secondFactorRequired": "Conferma con il tuo secondo fattore o una passkey per cambiare l'indirizzo email." }, "avatar": { "title": "Foto del profilo", diff --git a/messages/ko.json b/messages/ko.json index f1349726d..1bcd9ab1c 100644 --- a/messages/ko.json +++ b/messages/ko.json @@ -126,7 +126,8 @@ "nothingSaved": "저장된 내용이 없어요. 표시된 항목을 확인하고 다시 시도해 주세요.", "invalidBody": "프로필 변경 내용을 읽지 못했어요.", "emailInUse": "이미 사용 중인 이메일 주소예요.", - "emailRateLimited": "이메일 주소를 너무 자주 변경했어요. 잠시 후 다시 시도해 주세요." + "emailRateLimited": "이메일 주소를 너무 자주 변경했어요. 잠시 후 다시 시도해 주세요.", + "emailSecondFactorRequired": "이메일 주소를 바꾸려면 2단계 인증 수단이나 패스키로 확인해 주세요." } }, "auth": { @@ -5568,7 +5569,8 @@ "rateLimited": "너무 자주 바꿨어요. 잠시 후 다시 시도해 주세요.", "other": "이 값은 저장하지 못했어요. 확인한 뒤 다시 시도해 주세요.", "reproofRequired": "이메일 주소를 변경하려면 현재 비밀번호를 입력하세요.", - "reproofFailed": "비밀번호가 올바르지 않아 이메일 주소가 변경되지 않았습니다." + "reproofFailed": "비밀번호가 올바르지 않아 이메일 주소가 변경되지 않았습니다.", + "secondFactorRequired": "이메일 주소를 바꾸려면 2단계 인증 수단이나 패스키로 확인해 주세요." }, "avatar": { "title": "프로필 사진", diff --git a/messages/pl.json b/messages/pl.json index b71f77fd3..1fe698c79 100644 --- a/messages/pl.json +++ b/messages/pl.json @@ -126,7 +126,8 @@ "nothingSaved": "Nic nie zostało zapisane. Sprawdź oznaczone pole i spróbuj ponownie.", "invalidBody": "Nie udało się odczytać aktualizacji profilu.", "emailInUse": "Ten adres e-mail jest już używany.", - "emailRateLimited": "Zbyt wiele zmian adresu e-mail. Spróbuj ponownie później." + "emailRateLimited": "Zbyt wiele zmian adresu e-mail. Spróbuj ponownie później.", + "emailSecondFactorRequired": "Potwierdź drugim składnikiem lub kluczem dostępu, aby zmienić adres e-mail." } }, "auth": { @@ -5568,7 +5569,8 @@ "rateLimited": "Zmieniane zbyt często. Odczekaj chwilę i spróbuj ponownie.", "other": "Ta wartość nie została przyjęta. Sprawdź ją i spróbuj ponownie.", "reproofRequired": "Wpisz obecne hasło, aby zmienić adres e-mail.", - "reproofFailed": "Hasło jest nieprawidłowe, więc adres e-mail nie został zmieniony." + "reproofFailed": "Hasło jest nieprawidłowe, więc adres e-mail nie został zmieniony.", + "secondFactorRequired": "Potwierdź drugim składnikiem lub kluczem dostępu, aby zmienić adres e-mail." }, "avatar": { "title": "Zdjęcie profilowe", diff --git a/src/components/settings/account-section/index.tsx b/src/components/settings/account-section/index.tsx index 534b496c2..359be334f 100644 --- a/src/components/settings/account-section/index.tsx +++ b/src/components/settings/account-section/index.tsx @@ -34,7 +34,11 @@ import { DateFormatSelect } from "@/components/settings/date-format-select"; import { UnitPreferenceSelect } from "@/components/settings/unit-preference-select"; import { GlucoseUnitSelect } from "@/components/settings/glucose-unit-select"; import { detectBrowserTimezone, DEFAULT_TIMEZONE } from "@/lib/tz/format"; -import { apiFetchRaw } from "@/lib/api/api-fetch"; +import { apiFetchRaw, apiGet } from "@/lib/api/api-fetch"; +import { + recentProofErrorMessage, + useRecentProof, +} from "@/components/settings/security-section/use-recent-proof"; import { describeRejectedProfileField, describeRejectedProfileFields, @@ -75,6 +79,12 @@ export function AccountSection() { // in within the last few minutes; the server decides and names the field if // it wanted one. Never stored beyond this form. const [emailPassword, setEmailPassword] = useState(""); + // On an account with a second factor the password does not move the + // address: the server asks for the factor instead (`second_factor_required`) + // and the save runs once more after the re-proof dialog. Once that is + // known the password field is not offered, since it could only be refused. + const [emailNeedsFactor, setEmailNeedsFactor] = useState(false); + const recentProof = useRecentProof(); const [height, setHeight] = useState(EMPTY_HEIGHT_DRAFT); const [dateOfBirth, setDateOfBirth] = useState(""); const [gender, setGender] = useState(""); @@ -193,8 +203,46 @@ export function AccountSection() { email.trim() !== "" && email.trim().toLowerCase() !== profileSeed.email.trim().toLowerCase(); - async function handleSaveProfile(e: React.FormEvent) { - e.preventDefault(); + /** + * Did the server keep the new address back until a second factor is + * proved? Either as the one rejected field of a partial save, or as the + * whole answer when the address was all that changed. + */ + function needsSecondFactor( + rejected: RejectedProfileField[] | undefined, + errorCode: string | undefined, + ): boolean { + return ( + errorCode === "profile.update.emailSecondFactorRequired" || + (rejected ?? []).some( + (r) => r.path === "email" && r.code === "second_factor_required", + ) + ); + } + + /** + * Ask for the second factor (or a passkey) through the shared re-proof + * dialog, then save once more. Returns false when the person closed the + * dialog or could not confirm, leaving the message to show. + */ + async function proveFactorThenSave(): Promise { + setEmailNeedsFactor(true); + try { + await recentProof.run(() => apiGet("/api/auth/reproof")); + } catch (err) { + const message = recentProofErrorMessage(err, ""); + if (message) { + setSaveMsg({ text: message }); + setSaveMsgType("error"); + } + return false; + } + await handleSaveProfile(undefined, true); + return true; + } + + async function handleSaveProfile(e?: React.FormEvent, afterFactor = false) { + e?.preventDefault(); setSaving(true); setSaveMsg(null); setSaveMsgType(null); @@ -255,6 +303,10 @@ export function AccountSection() { } | null; const rejected = profileJson?.data?.rejectedFields; + if (!afterFactor && needsSecondFactor(rejected, undefined)) { + if (await proveFactorThenSave()) return; + } + if (rejected && rejected.length > 0) { // The write is field-independent: everything but the rejected // field(s) landed. Name the field instead of implying full @@ -304,6 +356,9 @@ export function AccountSection() { details?: { issues?: RejectedProfileField[] }; } | null; const errorCode = json?.meta?.errorCode; + if (!afterFactor && needsSecondFactor(undefined, errorCode)) { + if (await proveFactorThenSave()) return; + } const field = describeRejectedProfileField(json?.details?.issues, t); setFieldErrors(describeRejectedProfileFields(json?.details?.issues, t)); if (errorCode === "profile.update.nothingSaved" && field) { @@ -350,7 +405,10 @@ export function AccountSection() { -
+ void handleSaveProfile(event)} + className="space-y-4" + >
@@ -386,7 +444,7 @@ export function AccountSection() { aria-describedby={fieldErrors.email ? "email-error" : undefined} /> - {emailChanged && ( + {emailChanged && !emailNeedsFactor && (