From 10d6dc8099b6148bf8d5fcaf25ab2c3b60b03311 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Bombeck?= Date: Fri, 11 Sep 2026 14:36:27 +0200 Subject: [PATCH] Guard the dummy mint, not only the dummy verification MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The dummy hash was awaited as an argument to the verifier, so a rejected mint threw before the verifier was reached and before the catch on it could apply. The promise was created once at import, and a rejected promise stays rejected, so one failed mint under memory pressure would have made every unknown identifier answer 500 while a real account answered 401 — a cleaner oracle than the timing gap this closes, and passkey-only accounts locked out of the password form until restart. The mint is now awaited inside the guarded region, is lazy rather than an import-time cost in every process that touches this module, and is dropped on failure so the next refusal mints again instead of inheriting a cached rejection. A failure leaves a log line; the answer is false either way. The tests cover a rejecting mint, not only a rejecting verify. --- .../__tests__/password-dummy-verify.test.ts | 88 ++++++++++++++++--- src/lib/auth/password.ts | 60 +++++++++---- 2 files changed, 121 insertions(+), 27 deletions(-) diff --git a/src/lib/auth/__tests__/password-dummy-verify.test.ts b/src/lib/auth/__tests__/password-dummy-verify.test.ts index 9035bdc8a..482dbeb50 100644 --- a/src/lib/auth/__tests__/password-dummy-verify.test.ts +++ b/src/lib/auth/__tests__/password-dummy-verify.test.ts @@ -19,8 +19,7 @@ */ 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. +// Hoisted so the mock is in place before `password.ts` is imported. const { hashMock, verifyMock } = vi.hoisted(() => ({ hashMock: vi.fn(async (input: string) => `hashed:${input}`), verifyMock: vi.fn(async (_hash: string, _password: string) => false), @@ -28,23 +27,45 @@ const { hashMock, verifyMock } = vi.hoisted(() => ({ 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(() => { +/** + * The dummy hash is minted once per module instance, so each test gets a + * fresh instance — otherwise the first successful mint is cached and the + * mint-failure cases could never reach the mint at all. + */ +let verifyPasswordOrDummy: ( + storedHash: string | null | undefined, + password: string, +) => Promise; + +beforeEach(async () => { verifyMock.mockReset(); verifyMock.mockResolvedValue(false); + hashMock.mockReset(); + hashMock.mockImplementation(async (input: string) => `hashed:${input}`); + vi.resetModules(); + ({ verifyPasswordOrDummy } = await import("@/lib/auth/password")); }); 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. + it("mints the dummy hash under the same Argon2id parameters real hashes use", async () => { + await verifyPasswordOrDummy(null, "anything"); expect(hashMock).toHaveBeenCalledWith( expect.any(String), ARGON2_HASH_OPTIONS, ); }); + it("mints once and reuses it for every later refusal", async () => { + await verifyPasswordOrDummy(null, "anything"); + await verifyPasswordOrDummy(null, "anything"); + await verifyPasswordOrDummy(undefined, "anything"); + // Three refusals, three verifications, one mint. + expect(verifyMock).toHaveBeenCalledTimes(3); + expect(hashMock).toHaveBeenCalledTimes(1); + }); + it("verifies against the stored hash when the account has one", async () => { verifyMock.mockResolvedValueOnce(true); await expect( @@ -61,9 +82,7 @@ describe("verifyPasswordOrDummy", () => { 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, - ); + expect(verifyMock.mock.calls[0][0]).toMatch(/^hashed:/); }); it("still verifies for a passkey-only account (undefined hash)", async () => { @@ -87,7 +106,54 @@ describe("verifyPasswordOrDummy", () => { 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); + const logged = vi.spyOn(console, "error").mockImplementation(() => {}); + try { + verifyMock.mockRejectedValueOnce(new Error("malformed hash")); + await expect(verifyPasswordOrDummy(null, "anything")).resolves.toBe( + false, + ); + expect(logged).toHaveBeenCalled(); + } finally { + logged.mockRestore(); + } + }); + + it("swallows a failed MINT too, not only a failed verify", async () => { + // The mint is awaited as an argument to the verifier, so an unguarded + // `await` on a rejected mint throws before the verifier is reached and + // before any `.catch` on it can apply. That answers 500 on exactly the + // arm that must look ordinary — and a passkey-only account could not + // sign in at all. + const logged = vi.spyOn(console, "error").mockImplementation(() => {}); + try { + hashMock.mockRejectedValueOnce(new Error("out of memory")); + await expect(verifyPasswordOrDummy(null, "anything")).resolves.toBe( + false, + ); + expect(verifyMock).not.toHaveBeenCalled(); + expect(logged).toHaveBeenCalled(); + } finally { + logged.mockRestore(); + } + }); + + it("re-mints after a failure instead of caching the rejection forever", async () => { + // A rejected promise stays rejected for the life of the process. Caching + // one would send every later refusal back down the cheap path with the + // timing channel reopened and nothing to notice it by. + const logged = vi.spyOn(console, "error").mockImplementation(() => {}); + try { + hashMock.mockRejectedValueOnce(new Error("out of memory")); + await verifyPasswordOrDummy(null, "anything"); + expect(verifyMock).not.toHaveBeenCalled(); + + await expect(verifyPasswordOrDummy(null, "anything")).resolves.toBe( + false, + ); + expect(hashMock).toHaveBeenCalledTimes(2); + expect(verifyMock).toHaveBeenCalledTimes(1); + } finally { + logged.mockRestore(); + } }); }); diff --git a/src/lib/auth/password.ts b/src/lib/auth/password.ts index de5665c49..eb3f4ae8e 100644 --- a/src/lib/auth/password.ts +++ b/src/lib/auth/password.ts @@ -31,10 +31,10 @@ export async function verifyPassword( * 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. + * Minted 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 @@ -42,13 +42,33 @@ export async function verifyPassword( * 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); + +let dummyHashPromise: Promise | null = null; + +/** + * The dummy hash, minted on first use and kept for the process afterwards. + * + * Lazy rather than minted at import: every process that imports this module + * would otherwise spend 19 MiB and a thread-pool slot at boot, including the + * queue worker and each build worker, none of which ever authenticates + * anybody. From the second refusal onward the cost profile is identical + * either way. + * + * A rejected promise stays rejected for the life of the process, so caching + * one would degrade every later refusal back to the cheap path and leave the + * timing channel open with no way to notice. A failed mint is therefore + * dropped and the next call mints again. + */ +function dummyHash(): Promise { + if (!dummyHashPromise) { + const pending = hash(DUMMY_VERIFY_INPUT, ARGON2_HASH_OPTIONS); + dummyHashPromise = pending; + pending.catch(() => { + if (dummyHashPromise === pending) dummyHashPromise = null; + }); + } + return dummyHashPromise; +} /** * Verify `password` against `storedHash`, or against the dummy hash when @@ -68,11 +88,19 @@ export async function verifyPasswordOrDummy( 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); + try { + // Same verifier, same cost profile, verdict discarded. + await verify(await dummyHash(), password); + } catch (error) { + // Both the mint and the verify are inside the guard, and the mint has to + // be: an unguarded `await` on a rejected mint would throw out of here and + // answer 500 on exactly the arm this function exists to make ordinary — + // an unknown identifier answering 500 while a real account answers 401 is + // the oracle again, in the status code, and cheaper to read than the + // timing gap was. The breadcrumb makes a degraded instance visible; the + // answer below is the same either way. + console.error("[auth] dummy password verification failed", error); + } return false; }