From b428fcf74f564fd14393ec5955ee790d29c2daec Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Bombeck?= Date: Thu, 10 Sep 2026 04:52:11 +0200 Subject: [PATCH] test(e2e): read the replay verdict instead of inferring it from a 401 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The replay control submitted the enrolment code at the login challenge and asserted a 401. That 401 is the same answer the route gives a code that has simply drifted out of its ±1-step window, and by the time the journey reached the challenge — a rotation, a refused rotation, a sign-out, a page load and an Argon2id verify later — the enrolment code usually had. Out of window, the refusal happens before the monotonic floor is ever consulted, so the control could pass with the floor deleted. Both TOTP refusals now assert the verdict the route records itself, auth.mfa.failed with details.replay, written and awaited before it answers: false for a code that never matched the secret, true for the replay. Flipping that verdict while leaving the refusal in place turns the positive half red, which is what the old assertion could not do. The replayed code is the one the previous sign-in just spent, so the window is a property of the journey rather than of the wall clock: the accept is taken at a step boundary when the current step has too little left, which bounds the wait by one period and leaves the pair at least two. The rotation stages no longer press Escape to get the confirmation out of the way. They assert the card closes it, by test id. --- e2e/mfa-step-up-journey.spec.ts | 137 +++++++++++++++++++++---------- e2e/setup/mfa-journey-fixture.ts | 45 ++++++++++ 2 files changed, 138 insertions(+), 44 deletions(-) diff --git a/e2e/mfa-step-up-journey.spec.ts b/e2e/mfa-step-up-journey.spec.ts index 19c639cd9..1d6fe764d 100644 --- a/e2e/mfa-step-up-journey.spec.ts +++ b/e2e/mfa-step-up-journey.spec.ts @@ -7,6 +7,7 @@ import { dropMfaAccount, expireStepUp, latestChallengeAttempts, + latestMfaFailure, mfaJourneyAccount, seedMfaAccount, type MfaJourneyAccount, @@ -19,13 +20,13 @@ import { * * ## Why one test and not six * - * Every stage here is the previous stage's output. The confirm code is the one - * that sets the replay floor; the recovery batch the sign-in spends is the one - * the successful regeneration issued; the expired-step-up refusal is only - * meaningful against the session the successful one just used. Split across - * tests, each would have to re-enrol a factor, and enrolment is exactly what - * the first stage is testing. So it is one journey, with `test.step` marking - * the stages in the report, and the account is thrown away afterwards. + * Every stage here is the previous stage's output. The code the sign-in spends + * is the code the next stage replays; the recovery batch that sign-in spends is + * the one the successful regeneration issued; the expired-step-up refusal is + * only meaningful against the session the successful one just used. Split + * across tests, each would have to re-enrol a factor, and enrolment is exactly + * what the first stage is testing. So it is one journey, with `test.step` + * marking the stages in the report, and the account is thrown away afterwards. * * ## Controls * @@ -35,28 +36,49 @@ import { * - a wrong code at the login challenge — 401, no session cookie, and the * ticket's attempt counter moved (read from the row, since the wire's 401 * is deliberately identical for every reason); - * - the enrolment code replayed at the login challenge — 401. The server - * implements this as a monotonic `User.totpLastStep` floor advanced on - * every accept (`src/lib/auth/mfa/verify-factor.ts:57-77`), so a code - * already spent is refused even inside its own 30-second life; + * - the code the previous sign-in just spent, replayed at the next challenge + * — 401. The server implements this as a monotonic `User.totpLastStep` + * floor advanced on every accept (`src/lib/auth/mfa/verify-factor.ts`), so + * a code already spent is refused even inside its own 30-second life; * - the sensitive action once the step-up stamp has aged out — 401 with * `meta.errorCode = "auth.stepup.required"`, and the card's prompt; * - a recovery code presented a second time — 401. The matched row is burned * on first use. * + * ## Reading the replay refusal, not guessing it + * + * Both TOTP refusals answer with the same generic 401, so the status code + * cannot say WHY. A code that has drifted out of its ±1-step window is refused + * before the replay floor is ever consulted, which means a status-only control + * would stay green with the floor deleted. The route records the distinction — + * `auth.mfa.failed` with `details.replay`, awaited before it answers — so both + * refusals assert that verdict as a pair: false for the wrong code, true for + * the replay. + * * ## Codes without a clock * * Every code is computed from the enrolment secret with the `otpauth` - * dependency the server itself verifies against, at an explicit timestamp — so - * nothing here waits for a step boundary. The sign-in code is generated one - * step AHEAD of now: the server accepts ±1 step of drift, and a future step is - * necessarily above the replay floor the enrolment left behind, whichever side - * of a boundary the request lands on. + * dependency the server itself verifies against, at an explicit timestamp. The + * sign-in code is generated one step AHEAD of now: the server accepts ±1 step + * of drift, and a future step is necessarily above the replay floor the + * enrolment left behind, whichever side of a boundary the request lands on. + * + * The one place the boundary matters is that code's second life as the replay: + * it stays in-window for two steps past the one it names, and the sign-out and + * sign-in between the two submissions eat into that. So the journey parks for + * the next step boundary — bounded by a single period, and only when the + * current step has too little left — instead of hoping the wall clock is kind. */ const TOTP_PERIOD_SECONDS = 30; const RECOVERY_CODE_COUNT = 10; const REGENERATE_ENDPOINT = "/api/auth/me/mfa/recovery-codes/regenerate"; +/** + * The margin the accept→replay pair needs. A code named for step N verifies + * until the end of step N+1 (drift −1), so generating it one step ahead leaves + * at least two periods; below this the journey waits for the next boundary. + */ +const REPLAY_MARGIN_MS = 75_000; /** A code for `atMs`, built the way `src/lib/auth/mfa/totp.ts` verifies it. */ function totpCodeAt(secretBase32: string, atMs: number): string { @@ -70,6 +92,23 @@ function totpCodeAt(secretBase32: string, atMs: number): string { }).generate({ timestamp: atMs }); } +/** + * Park until the next TOTP step starts, when the current one has too little + * left for the code about to be minted to survive its replay. + * + * Bounded by a single period and usually a no-op: it waits only for the last + * seconds of a step, and a step is 30 of them. + */ +async function alignToStepBoundary(page: Page): Promise { + const periodMs = TOTP_PERIOD_SECONDS * 1000; + const now = Date.now(); + const nextBoundary = (Math.floor(now / periodMs) + 1) * periodMs; + // A code minted now names the next step and verifies until two periods + // after that step begins. + if (nextBoundary + 2 * periodMs - now >= REPLAY_MARGIN_MS) return; + await page.waitForTimeout(nextBoundary - now); +} + async function sessionCookie(context: BrowserContext) { const cookies = await context.cookies(); return cookies.find((c) => c.name === "healthlog_session"); @@ -117,13 +156,13 @@ async function submitFactor(page: Page, code: string): Promise { /** * Open the regeneration dialog, confirm it, and return the route's answer. * - * The confirming click calls `preventDefault()`, which is what keeps the - * dialog up while the request is in flight — and nothing takes it down again. - * On SUCCESS that goes unnoticed, because the fresh-codes panel replaces the - * whole branch the dialog lives in and it unmounts with it. On a REFUSAL the - * branch stays, so the dialog is still open (`data-state="open"`, measured) - * over a card that is now showing the step-up message underneath it. So the - * journey dismisses it before reading anything on the card. + * The confirming click calls `preventDefault()` so the dialog survives the + * request in flight; the mutation takes it down again when the request + * settles, whatever the answer was. That matters on a REFUSAL, where the card + * behind the dialog is the only thing that says why — on a success the + * fresh-codes panel replaces the whole branch the dialog lives in and it would + * unmount either way. So the dialog being gone here is a product assertion, + * not housekeeping: nothing in this helper dismisses it. */ async function regenerateRecoveryCodes(page: Page): Promise { await page.getByTestId("recovery-regenerate").click(); @@ -134,8 +173,7 @@ async function regenerateRecoveryCodes(page: Page): Promise { ); await page.getByTestId("recovery-regenerate-confirm").click(); const settled = await answer; - await page.keyboard.press("Escape"); - await expect(page.locator('[data-slot="alert-dialog-content"]')).toBeHidden(); + await expect(page.getByTestId("recovery-regenerate-dialog")).toBeHidden(); return settled; } @@ -157,9 +195,10 @@ test.describe("second factor", () => { page, context, }) => { - // Four sign-ins, an Argon2id verify on each, an enrolment and two - // regenerations. Comfortably outside the suite's 30 s default. - test.setTimeout(180_000); + // Five sign-ins, an Argon2id verify on each, an enrolment, two + // regenerations and a bounded park on a step boundary. Comfortably + // outside the suite's 30 s default. + test.setTimeout(240_000); // One account per project: both browser projects run this file, and the // journey moves the account's factor state, its replay floor and its @@ -170,7 +209,7 @@ test.describe("second factor", () => { await seedMfaAccount(account); let secret = ""; - let enrolmentCode = ""; + let spentCode = ""; let liveRecoveryCodes: string[] = []; await test.step("signs in with a password alone while no factor exists", async () => { @@ -205,8 +244,9 @@ test.describe("second factor", () => { // up enrolled against different secrets. expect(setupBody.data.otpauthUri).toContain(secret); - enrolmentCode = totpCodeAt(secret, Date.now()); - await page.getByTestId("totp-confirm-code").fill(enrolmentCode); + await page + .getByTestId("totp-confirm-code") + .fill(totpCodeAt(secret, Date.now())); const confirm = page.waitForResponse( (res) => res.url().includes("/api/auth/me/mfa/totp/confirm") && @@ -244,7 +284,7 @@ test.describe("second factor", () => { }; expect(body.data).toBeNull(); expect(body.meta?.errorCode).toBe("auth.stepup.required"); - // And the card says so rather than failing silently. + // And the card says so, on a card the closed dialog no longer covers. await expect(page.getByTestId("totp-error")).toBeVisible(); }); @@ -268,40 +308,49 @@ test.describe("second factor", () => { expect(await sessionCookie(context)).toBeUndefined(); }); - await test.step("refuses a wrong code and counts the attempt", async () => { + await test.step("refuses wrong codes and counts each attempt", async () => { const answer = await submitFactor(page, "000000"); expect(answer.status()).toBe(401); await expect(page.getByTestId("mfa-error")).toBeVisible(); await expect(page.getByTestId("mfa-login-step")).toBeVisible(); expect(await sessionCookie(context)).toBeUndefined(); expect(await latestChallengeAttempts(account)).toBe(1); - }); + // The negative half of the replay pair: a code that never matched the + // secret is not a replay, and the server says so. + expect((await latestMfaFailure(account)).replay).toBe(false); - await test.step("refuses the enrolment code replayed", async () => { - const answer = await submitFactor(page, enrolmentCode); - expect(answer.status()).toBe(401); - await expect(page.getByTestId("mfa-error")).toBeVisible(); - expect(await sessionCookie(context)).toBeUndefined(); + // A second refusal moves the counter again rather than resetting it. + expect((await submitFactor(page, "000001")).status()).toBe(401); expect(await latestChallengeAttempts(account)).toBe(2); }); await test.step("accepts a current code and lands on the dashboard", async () => { + await alignToStepBoundary(page); // One step ahead of now — inside the ±1 drift window and necessarily // above the floor the enrolment code left. See the file header. - const answer = await submitFactor( - page, - totpCodeAt(secret, Date.now() + TOTP_PERIOD_SECONDS * 1000), - ); + spentCode = totpCodeAt(secret, Date.now() + TOTP_PERIOD_SECONDS * 1000); + const answer = await submitFactor(page, spentCode); expect(answer.status()).toBe(200); await page.waitForURL("/"); expect(await sessionCookie(context)).toBeDefined(); }); - await test.step("spends a recovery code once", async () => { + await test.step("refuses the code the last sign-in spent", async () => { expect((await page.request.post("/api/auth/logout")).status()).toBe(200); await submitPassword(page, account); await expect(page.getByTestId("mfa-login-step")).toBeVisible(); + const answer = await submitFactor(page, spentCode); + expect(answer.status()).toBe(401); + await expect(page.getByTestId("mfa-error")).toBeVisible(); + expect(await sessionCookie(context)).toBeUndefined(); + expect(await latestChallengeAttempts(account)).toBe(1); + // The code is still inside its own life — the floor is what refused it, + // and this is the assertion that fails if the floor is removed. + expect((await latestMfaFailure(account)).replay).toBe(true); + }); + + await test.step("spends a recovery code once", async () => { await page.getByTestId("mfa-toggle-recovery").click(); const answer = await submitFactor(page, liveRecoveryCodes[0]); expect(answer.status()).toBe(200); diff --git a/e2e/setup/mfa-journey-fixture.ts b/e2e/setup/mfa-journey-fixture.ts index ce6ccac07..aecb68ea5 100644 --- a/e2e/setup/mfa-journey-fixture.ts +++ b/e2e/setup/mfa-journey-fixture.ts @@ -187,6 +187,51 @@ export async function latestChallengeAttempts( } } +/** + * The server's own verdict on the account's most recent refused factor. + * + * `/api/auth/mfa/verify` answers every refusal with the same 401, so the wire + * cannot tell a replayed code from a wrong one — and that distinction is the + * whole subject of the replay control. The route does record it: it writes + * `auth.mfa.failed` with `details.replay` and AWAITS that write before + * responding, so the row is on disk by the time the response lands. + * + * Read it rather than inferring it. A replayed code that has drifted out of its + * ±1-step window is refused as out-of-window — same 401, `replay: false` — and + * a control that only reads the status code cannot tell the two apart, which + * means it would stay green with the replay floor deleted. + */ +export async function latestMfaFailure( + account: MfaJourneyAccount, +): Promise<{ replay: boolean }> { + const pool = connect(); + try { + const res = await pool.query<{ details: string | null }>( + `SELECT details FROM audit_logs + WHERE user_id = (SELECT id FROM users WHERE username = $1) + AND action = 'auth.mfa.failed' + ORDER BY created_at DESC + LIMIT 1`, + [account.username], + ); + const row = res.rows[0]; + if (!row?.details) { + throw new Error( + `[mfa-journey-fixture] no refused factor recorded for ${account.username}`, + ); + } + const details = JSON.parse(row.details) as { replay?: unknown }; + if (typeof details.replay !== "boolean") { + throw new Error( + `[mfa-journey-fixture] refusal for ${account.username} carries no replay verdict`, + ); + } + return { replay: details.replay }; + } finally { + await pool.end(); + } +} + /** * Age the session's step-up stamp past the five-minute window. *