From 6a5fb338b15f736d25679e74472e1f9863c0774f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Bombeck?= Date: Fri, 11 Sep 2026 13:59:42 +0200 Subject: [PATCH] Make every login refusal cost the same verification Sign-in returned the moment it found no account, or an account carrying no password hash, before it reached the verifier. An account that did have a password paid a full Argon2id verification at 19 MiB and t=2. The three refusals answered with the identical 401 body and three very different durations, so the clock told an anonymous caller whether an address is registered and whether it carries a password. `verifyPasswordOrDummy` verifies against a stand-in hash minted once per process at the same cost parameters real hashes use, and returns false whenever there is nothing to verify, so the passkey-only account takes the same path as one with a password. The dummy arm discards its own verdict and swallows a throw, so no outcome there can be read as a success or answer with a different status. --- .../api/auth/login/mfa-branch.test.ts | 6 +- .../login/__tests__/constant-work.test.ts | 155 ++++++++++++++++++ .../auth/login/__tests__/native-token.test.ts | 6 +- .../api/auth/login/__tests__/route.test.ts | 8 +- .../login/__tests__/trusted-device.test.ts | 10 +- src/app/api/auth/login/route.ts | 15 +- .../__tests__/password-dummy-verify.test.ts | 93 +++++++++++ src/lib/auth/password.ts | 56 +++++++ 8 files changed, 332 insertions(+), 17 deletions(-) create mode 100644 src/app/api/auth/login/__tests__/constant-work.test.ts create mode 100644 src/lib/auth/__tests__/password-dummy-verify.test.ts diff --git a/src/__tests__/api/auth/login/mfa-branch.test.ts b/src/__tests__/api/auth/login/mfa-branch.test.ts index e59dffb9d..79de4e666 100644 --- a/src/__tests__/api/auth/login/mfa-branch.test.ts +++ b/src/__tests__/api/auth/login/mfa-branch.test.ts @@ -12,7 +12,7 @@ vi.mock("@/lib/db", () => ({ }, })); vi.mock("@/lib/auth/password", () => ({ - verifyPassword: vi.fn().mockResolvedValue(true), + verifyPasswordOrDummy: vi.fn().mockResolvedValue(true), })); vi.mock("@/lib/auth/audit", () => ({ auditLog: vi.fn().mockResolvedValue(undefined), @@ -142,8 +142,8 @@ describe("login MFA branch", () => { }); it("wrong password never reaches the MFA branch", async () => { - const { verifyPassword } = await import("@/lib/auth/password"); - vi.mocked(verifyPassword).mockResolvedValueOnce(false); + const { verifyPasswordOrDummy } = await import("@/lib/auth/password"); + vi.mocked(verifyPasswordOrDummy).mockResolvedValueOnce(false); vi.mocked(prisma.user.findFirst).mockResolvedValue({ ...BASE_USER, totpConfirmedAt: new Date(), diff --git a/src/app/api/auth/login/__tests__/constant-work.test.ts b/src/app/api/auth/login/__tests__/constant-work.test.ts new file mode 100644 index 000000000..87be3506b --- /dev/null +++ b/src/app/api/auth/login/__tests__/constant-work.test.ts @@ -0,0 +1,155 @@ +/** + * Every login outcome reaches the verifier. + * + * The three refusals sign-in can produce — no such account, an account with + * no password hash, a wrong password — return the identical 401 body, and + * used to take three very different amounts of time to produce it, because + * the first two returned before any hashing happened. Argon2id at 19 MiB and + * t=2 against one indexed SELECT is a gap a caller can read over a network. + * + * The passkey-only account is the case that matters most. If it kept the + * cheap path, the channel would still answer "this account exists and has no + * password" — which is exactly what the removed discovery endpoint published + * in a field. + * + * What this proves is that the call happens on all three arms, not that the + * three durations match: the verifier is mocked here, and wall-clock equality + * is not something a unit test can assert. `password-dummy-verify.test.ts` + * carries the other half, that the verifier really does Argon2id work at the + * real cost parameters when there is no stored hash. + */ +import { describe, it, expect, vi, beforeEach } from "vitest"; +import { NextRequest } from "next/server"; + +vi.mock("@/lib/db", () => ({ + prisma: { + user: { findFirst: vi.fn() }, + webauthnMfaCredential: { count: vi.fn().mockResolvedValue(0) }, + }, +})); + +vi.mock("@/lib/auth/password", () => ({ + verifyPasswordOrDummy: vi.fn(), +})); + +vi.mock("@/lib/auth/audit", () => ({ + auditLog: vi.fn().mockResolvedValue(undefined), +})); + +vi.mock("@/lib/rate-limit", () => ({ + checkAuthSurfaceRateLimit: vi.fn().mockResolvedValue({ + allowed: true, + remaining: 5, + reset: 0, + ip: "1.2.3.4", + }), + rateLimitHeaders: vi.fn(() => ({})), +})); + +vi.mock("@/lib/db-compat", () => ({ + ensureDbCompatibility: vi.fn().mockResolvedValue(undefined), +})); + +vi.mock("@/lib/auth/hmac", () => ({ + hashToken: vi.fn(() => "aa".repeat(32)), +})); + +vi.mock("@/lib/logging/transports", () => ({ emitIfSampled: 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 { prisma } from "@/lib/db"; +import { verifyPasswordOrDummy } from "@/lib/auth/password"; + +function makeRequest(): NextRequest { + return new NextRequest("http://localhost/api/auth/login", { + method: "POST", + headers: { "content-type": "application/json" }, + body: JSON.stringify({ + email: "someone@example.com", + password: "a-typed-password", + }), + }); +} + +beforeEach(() => { + vi.mocked(verifyPasswordOrDummy).mockReset(); + vi.mocked(verifyPasswordOrDummy).mockResolvedValue(false); +}); + +describe("POST /api/auth/login — constant work across the refusals", () => { + it("verifies even when no account matches the identifier", async () => { + vi.mocked(prisma.user.findFirst).mockResolvedValue(null as never); + + const res = await POST(makeRequest()); + + expect(res.status).toBe(401); + expect(verifyPasswordOrDummy).toHaveBeenCalledTimes(1); + expect(verifyPasswordOrDummy).toHaveBeenCalledWith( + undefined, + "a-typed-password", + ); + }); + + it("verifies for a passkey-only account, which carries no password hash", async () => { + vi.mocked(prisma.user.findFirst).mockResolvedValue({ + id: "user-1", + passwordHash: null, + totpConfirmedAt: null, + mfaEnforced: false, + } as never); + + const res = await POST(makeRequest()); + + expect(res.status).toBe(401); + expect(verifyPasswordOrDummy).toHaveBeenCalledTimes(1); + expect(verifyPasswordOrDummy).toHaveBeenCalledWith( + null, + "a-typed-password", + ); + }); + + it("verifies for an account that does carry a password hash", async () => { + vi.mocked(prisma.user.findFirst).mockResolvedValue({ + id: "user-2", + passwordHash: "$argon2id$stored", + totpConfirmedAt: null, + mfaEnforced: false, + } as never); + + const res = await POST(makeRequest()); + + expect(res.status).toBe(401); + expect(verifyPasswordOrDummy).toHaveBeenCalledTimes(1); + expect(verifyPasswordOrDummy).toHaveBeenCalledWith( + "$argon2id$stored", + "a-typed-password", + ); + }); + + it("answers the three refusals with the identical status and body", async () => { + const bodies: string[] = []; + const statuses: number[] = []; + const accounts = [ + null, + { id: "user-1", passwordHash: null }, + { id: "user-2", passwordHash: "$argon2id$stored" }, + ]; + for (const account of accounts) { + vi.mocked(prisma.user.findFirst).mockResolvedValue(account as never); + const res = await POST(makeRequest()); + statuses.push(res.status); + bodies.push(await res.text()); + } + expect(new Set(statuses)).toEqual(new Set([401])); + expect(new Set(bodies).size).toBe(1); + }); +}); 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 fc1212912..7089dbff9 100644 --- a/src/app/api/auth/login/__tests__/native-token.test.ts +++ b/src/app/api/auth/login/__tests__/native-token.test.ts @@ -14,7 +14,7 @@ vi.mock("@/lib/db", () => ({ })); vi.mock("@/lib/auth/password", () => ({ - verifyPassword: vi.fn(), + verifyPasswordOrDummy: vi.fn(), })); vi.mock("@/lib/auth/mfa-enrollment", () => ({ @@ -73,7 +73,7 @@ vi.mock("next/headers", () => ({ import { POST } from "../route"; import { prisma } from "@/lib/db"; -import { verifyPassword } from "@/lib/auth/password"; +import { verifyPasswordOrDummy } from "@/lib/auth/password"; import { checkRateLimit, checkAuthSurfaceRateLimit, @@ -108,7 +108,7 @@ beforeEach(() => { vi.resetAllMocks(); vi.mocked(prisma.user.findFirst).mockResolvedValue(FAKE_USER as never); vi.mocked(prisma.webauthnMfaCredential.count).mockResolvedValue(0 as never); - vi.mocked(verifyPassword).mockResolvedValue(true); + vi.mocked(verifyPasswordOrDummy).mockResolvedValue(true); vi.mocked(prisma.apiToken.create).mockResolvedValue({ id: "tok-1" } as never); vi.mocked(prisma.refreshToken.create).mockResolvedValue({ id: "rt-1", diff --git a/src/app/api/auth/login/__tests__/route.test.ts b/src/app/api/auth/login/__tests__/route.test.ts index c82732f95..fb2df339b 100644 --- a/src/app/api/auth/login/__tests__/route.test.ts +++ b/src/app/api/auth/login/__tests__/route.test.ts @@ -17,7 +17,7 @@ vi.mock("@/lib/db", () => ({ })); vi.mock("@/lib/auth/password", () => ({ - verifyPassword: vi.fn(), + verifyPasswordOrDummy: vi.fn(), })); vi.mock("@/lib/auth/session", () => ({ @@ -71,7 +71,7 @@ vi.mock("next/headers", () => ({ import { POST } from "../route"; import { prisma } from "@/lib/db"; -import { verifyPassword } from "@/lib/auth/password"; +import { verifyPasswordOrDummy } from "@/lib/auth/password"; import { auditLog } from "@/lib/auth/audit"; import { checkRateLimit, @@ -93,7 +93,7 @@ function makeRequest(): NextRequest { beforeEach(() => { vi.resetAllMocks(); vi.mocked(prisma.user.findFirst).mockResolvedValue(null as never); - vi.mocked(verifyPassword).mockResolvedValue(false); + vi.mocked(verifyPasswordOrDummy).mockResolvedValue(false); vi.mocked(auditLog).mockResolvedValue(undefined as never); vi.mocked(checkRateLimit).mockResolvedValue({ allowed: true, @@ -175,7 +175,7 @@ describe("POST /api/auth/login — OIDC_ONLY server-side enforcement", () => { const res = await POST(makeRequest()); // Falls through to normal password verification (which fails here - // since verifyPassword is mocked to return false) rather than 403 — + // since verifyPasswordOrDummy is mocked to return false) rather than 403 — // a half-set OIDC group must never lock everyone out. expect(res.status).toBe(401); }); 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 cb895be6b..f8eab01a4 100644 --- a/src/app/api/auth/login/__tests__/trusted-device.test.ts +++ b/src/app/api/auth/login/__tests__/trusted-device.test.ts @@ -8,7 +8,7 @@ vi.mock("@/lib/db", () => ({ }, })); -vi.mock("@/lib/auth/password", () => ({ verifyPassword: vi.fn() })); +vi.mock("@/lib/auth/password", () => ({ verifyPasswordOrDummy: vi.fn() })); vi.mock("@/lib/auth/login-response", () => ({ finishLogin: vi.fn(async () => new Response(null, { status: 200 })), @@ -58,7 +58,7 @@ vi.mock("next/headers", () => ({ import { POST } from "../route"; import { prisma } from "@/lib/db"; -import { verifyPassword } from "@/lib/auth/password"; +import { verifyPasswordOrDummy } from "@/lib/auth/password"; import { finishLogin } from "@/lib/auth/login-response"; import { createMfaChallenge } from "@/lib/auth/mfa/challenge"; import { consumeTrustedDevice } from "@/lib/auth/trusted-device"; @@ -88,7 +88,7 @@ beforeEach(() => { describe("POST /api/auth/login — trusted device", () => { it("skips the second factor when the device is trusted (no challenge minted)", async () => { - vi.mocked(verifyPassword).mockResolvedValue(true); + vi.mocked(verifyPasswordOrDummy).mockResolvedValue(true); vi.mocked(consumeTrustedDevice).mockResolvedValue(true); const res = await POST(req()); @@ -103,7 +103,7 @@ describe("POST /api/auth/login — trusted device", () => { }); it("falls back to the MFA challenge when the device is not trusted", async () => { - vi.mocked(verifyPassword).mockResolvedValue(true); + vi.mocked(verifyPasswordOrDummy).mockResolvedValue(true); vi.mocked(consumeTrustedDevice).mockResolvedValue(false); const res = await POST(req()); @@ -115,7 +115,7 @@ describe("POST /api/auth/login — trusted device", () => { }); it("still requires the password — a wrong password never consults the trusted device", async () => { - vi.mocked(verifyPassword).mockResolvedValue(false); + vi.mocked(verifyPasswordOrDummy).mockResolvedValue(false); const res = await POST(req()); expect(res.status).toBe(401); diff --git a/src/app/api/auth/login/route.ts b/src/app/api/auth/login/route.ts index 5a291aa09..5c163c0dd 100644 --- a/src/app/api/auth/login/route.ts +++ b/src/app/api/auth/login/route.ts @@ -1,6 +1,6 @@ import { prisma } from "@/lib/db"; import { loginPasswordSchema } from "@/lib/validations/auth"; -import { verifyPassword } from "@/lib/auth/password"; +import { verifyPasswordOrDummy } from "@/lib/auth/password"; import { auditLog } from "@/lib/auth/audit"; import { hashToken } from "@/lib/auth/hmac"; import { @@ -84,6 +84,18 @@ export const POST = apiHandler(async (request: NextRequest) => { }, }); + // One verification for every outcome. An unknown identifier and an + // account carrying no password hash (a passkey-only account) used to + // return here before any hashing, so the three answers cost one indexed + // SELECT, one indexed SELECT, and a full Argon2id verification + // respectively — identical bodies, three different durations, which is a + // readable answer to "is this address registered, and does it have a + // password". `verifyPasswordOrDummy` verifies against a fixed dummy hash + // minted at the same cost parameters when there is no stored hash, and + // returns false. Keep the call ahead of the branch: moving it back under + // an `if` restores the channel. + const valid = await verifyPasswordOrDummy(user?.passwordHash, password); + if (!user || !user.passwordHash) { // v1.4.43 W3-SECURITY (H-1): never write the typed identifier into // the audit row — `reason` already tells the operator what @@ -98,7 +110,6 @@ export const POST = apiHandler(async (request: NextRequest) => { return apiError("Invalid credentials", 401); } - const valid = await verifyPassword(user.passwordHash, password); if (!valid) { await auditLog("auth.login.failed", { userId: user.id, diff --git a/src/lib/auth/__tests__/password-dummy-verify.test.ts b/src/lib/auth/__tests__/password-dummy-verify.test.ts new file mode 100644 index 000000000..9035bdc8a --- /dev/null +++ b/src/lib/auth/__tests__/password-dummy-verify.test.ts @@ -0,0 +1,93 @@ +/** + * Signing in must cost the same whether or not the account exists. + * + * The login route used to return the moment it found no account, or an + * account with no password hash, before it reached the verifier. An account + * that did have a password paid a full Argon2id verification. Same status, + * same body, very different durations — so the clock answered "is this + * address registered, and does it carry a password", which is the question + * the removed discovery endpoint used to answer out loud. + * + * What this file pins is the work, not the wall clock: that the empty arms + * reach the verifier at all, that the hash they verify against was minted at + * the same cost parameters every real hash carries, and that no outcome on + * those arms can be read as a successful verification. A unit test cannot + * prove two durations are equal — it can only prove the same call is made. + * + * `@node-rs/argon2` is mocked so the assertions are about which calls happen + * with which arguments, and so the suite does not spend real Argon2 time. + */ +import { describe, it, expect, vi, beforeEach } from "vitest"; + +// Hoisted: `password.ts` mints the dummy hash at module load, so the mock +// has to exist before the import below runs. +const { hashMock, verifyMock } = vi.hoisted(() => ({ + hashMock: vi.fn(async (input: string) => `hashed:${input}`), + verifyMock: vi.fn(async (_hash: string, _password: string) => false), +})); + +vi.mock("@node-rs/argon2", () => ({ hash: hashMock, verify: verifyMock })); + +import { verifyPasswordOrDummy } from "@/lib/auth/password"; +import { ARGON2_HASH_OPTIONS } from "@/lib/auth/argon2-params.mjs"; + +beforeEach(() => { + verifyMock.mockReset(); + verifyMock.mockResolvedValue(false); +}); + +describe("verifyPasswordOrDummy", () => { + it("mints the dummy hash under the same Argon2id parameters real hashes use", () => { + // The module mints it once at load, so the call has already happened. + expect(hashMock).toHaveBeenCalledWith( + expect.any(String), + ARGON2_HASH_OPTIONS, + ); + }); + + it("verifies against the stored hash when the account has one", async () => { + verifyMock.mockResolvedValueOnce(true); + await expect( + verifyPasswordOrDummy("$argon2id$stored", "correct horse"), + ).resolves.toBe(true); + expect(verifyMock).toHaveBeenCalledTimes(1); + expect(verifyMock).toHaveBeenCalledWith( + "$argon2id$stored", + "correct horse", + ); + }); + + it("still verifies when there is no account at all (null hash)", async () => { + await expect(verifyPasswordOrDummy(null, "anything")).resolves.toBe(false); + expect(verifyMock).toHaveBeenCalledTimes(1); + // Against the dummy, not against nothing. + expect(verifyMock.mock.calls[0][0]).toBe( + await hashMock.mock.results[0].value, + ); + }); + + it("still verifies for a passkey-only account (undefined hash)", async () => { + // The passkey-only case is the one that matters most: if it took the + // cheap path the channel would still answer "this account has no + // password", which is half of what the removed endpoint published. + await expect(verifyPasswordOrDummy(undefined, "anything")).resolves.toBe( + false, + ); + expect(verifyMock).toHaveBeenCalledTimes(1); + }); + + it("never reports success from the dummy arm, whatever the verifier says", async () => { + verifyMock.mockResolvedValue(true); + await expect(verifyPasswordOrDummy(null, "anything")).resolves.toBe(false); + await expect(verifyPasswordOrDummy(undefined, "anything")).resolves.toBe( + false, + ); + }); + + it("swallows a throw from the dummy arm rather than answering differently", async () => { + // A 500 only the unknown-account arm can produce is the same oracle in + // a different field. + verifyMock.mockRejectedValueOnce(new Error("malformed hash")); + await expect(verifyPasswordOrDummy(null, "anything")).resolves.toBe(false); + }); +}); diff --git a/src/lib/auth/password.ts b/src/lib/auth/password.ts index 570700b88..de5665c49 100644 --- a/src/lib/auth/password.ts +++ b/src/lib/auth/password.ts @@ -20,6 +20,62 @@ export async function verifyPassword( return verify(storedHash, password); } +/** + * A stand-in stored hash for the login arms that have nothing to verify. + * + * Sign-in used to return the moment it found no account, or an account with + * no password — one indexed SELECT — while an account that did have a + * password paid a full Argon2id verification at 19 MiB and t=2. The two + * answers were byte-identical and the two costs were not, so the clock told + * a caller which addresses are registered and which of those carry a + * password. Verifying against this hash on the empty arms makes the work + * comparable. + * + * Minted once per process from `ARGON2_HASH_OPTIONS`, the same parameters + * every real hash carries, because Argon2 encodes its cost in the hash + * string and the verifier pays whatever the string says. A dummy minted at + * cheaper settings would leave the gap it was added to close. + * + * The input is a fixed constant rather than a random string so the module + * has no hidden state, and it is never compared to anything: the arm that + * verifies against it discards the verdict and returns false. There is no + * path on which a caller can make this hash accept. + */ +const DUMMY_VERIFY_INPUT = "healthlog: not a credential, never accepted"; +const DUMMY_PASSWORD_HASH: Promise = hash( + DUMMY_VERIFY_INPUT, + ARGON2_HASH_OPTIONS, +); +// Keep a mint failure from surfacing as an unhandled rejection at import +// time; `verifyPasswordOrDummy` still awaits and still sees it. +void DUMMY_PASSWORD_HASH.catch(() => undefined); + +/** + * Verify `password` against `storedHash`, or against the dummy hash when + * there is no stored hash to verify against. + * + * Returns false whenever `storedHash` is absent — an account without a + * password, or no account at all. The dummy verification's own verdict is + * thrown away, so a false here is never a rejected credential being + * mistaken for anything else, and a true can only come from a real stored + * hash. + * + * Callers on an anonymous surface should use this rather than branching on + * the absent hash themselves: the branch is what the clock reads. + */ +export async function verifyPasswordOrDummy( + storedHash: string | null | undefined, + password: string, +): Promise { + if (storedHash) return verify(storedHash, password); + // Same verifier, same cost profile, verdict discarded. A throw here (a + // malformed dummy, a mint that failed) must not turn into a 500 that only + // the unknown-account arm can produce — that would be the oracle again, + // in the status code this time. + await verify(await DUMMY_PASSWORD_HASH, password).catch(() => false); + return false; +} + export interface PasswordStrength { score: number; // 0-4 feedback: string[];