From 3e8846929c622d5602983fccc4bbc21f940c7dc7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Bombeck?= Date: Fri, 23 Oct 2026 22:51:51 +0200 Subject: [PATCH] test(guards): load no module graph inside a guard's test body step-up-elevation-guard timed out under load in three separate gates and passed every time on its own. One of its tests imported @/lib/auth/step-up inside the test body to read the fresh-factor set, and that module reaches the database client and the generated Prisma client: the import alone ran against the test timeout, and how long it took depended on the machine, not on the code under test. The fresh-factor definitions (StepUpMethod, FRESH_FACTOR_METHODS, isFreshFactorMethod) move to step-up-factors.ts, which imports nothing; the guard imports it statically and the test is synchronous. Two more guards did the same: next-image-optimizer-guard loaded next.config and its plugins per test, and proxy-relative-redirect-guard loaded the redirect helper with next/server. Both import statically now. guard-dynamic-import-guard.test.ts parses every file of the guard project with the TypeScript parser and fails on any import() call, so a guard that needs a value imports it with the file or reads the source; a test that must load late belongs to the isolated project. --- .../guard-dynamic-import-guard.test.ts | 96 +++++++++++++++++++ .../next-image-optimizer-guard.test.ts | 27 +++--- .../proxy-relative-redirect-guard.test.ts | 6 +- src/__tests__/step-up-elevation-guard.test.ts | 14 ++- src/app/api/auth/step-up/route.ts | 6 +- src/lib/auth/__tests__/step-up.test.ts | 4 +- src/lib/auth/step-up-factors.ts | 45 +++++++++ src/lib/auth/step-up.ts | 41 +------- 8 files changed, 179 insertions(+), 60 deletions(-) create mode 100644 src/__tests__/guard-dynamic-import-guard.test.ts create mode 100644 src/lib/auth/step-up-factors.ts diff --git a/src/__tests__/guard-dynamic-import-guard.test.ts b/src/__tests__/guard-dynamic-import-guard.test.ts new file mode 100644 index 000000000..3aabe98e1 --- /dev/null +++ b/src/__tests__/guard-dynamic-import-guard.test.ts @@ -0,0 +1,96 @@ +/** + * No structural guard loads a module graph inside a test. + * + * The guard project shares one module cache per worker (`isolate: false`, + * `tests/unit-projects.ts`), so a guard's static imports load once, at + * collection. A dynamic `import()` inside a test body loads its graph against + * that test's timeout instead, and how long it takes depends on the machine + * rather than on the code under test. `step-up-elevation-guard` imported + * `@/lib/auth/step-up` that way, which reached the database client and the + * generated Prisma client, and timed out under load in three separate gates + * while passing every time on its own. `next-image-optimizer-guard` loaded + * Next's config and plugins the same way. + * + * So no file of the guard project calls `import()`. A guard that needs a + * value imports it statically, or reads the source; a test that must load a + * module late is an isolated test, not a guard. The files are read with the + * TypeScript parser, so an `import(` inside a string (a matcher's own test + * case) is not one. + */ +import { join } from "node:path"; + +import ts from "typescript"; +import { describe, expect, it } from "vitest"; + +import { classifyUnitFiles } from "../../tests/unit-projects"; +import { + readSourceText, + requireFloor, + walkSourceFiles, +} from "./helpers/source-files"; + +const ROOT = join(__dirname, "..", ".."); + +/** The guard project's files, classified the way the config classifies them. */ +function guardFiles(): string[] { + const under = (dir: string, floor: number, extensions?: string[]) => + walkSourceFiles(join(ROOT, dir), { floor, extensions }).map( + (rel) => `${dir}/${rel}`, + ); + const candidates = [ + ...under("src/__tests__", 200), + ...under("scripts/__tests__", 5), + ...under("tests/quarantine", 1), + ...under("eslint-plugins", 10, [".mjs"]), + ].filter((rel) => /\.test\.(?:tsx?|mjs)$/.test(rel)); + return classifyUnitFiles(ROOT, candidates).guards; +} + +function dynamicImports(rel: string): number[] { + const text = readSourceText(join(ROOT, rel)); + const file = ts.createSourceFile(rel, text, ts.ScriptTarget.Latest, true); + const lines: number[] = []; + const visit = (node: ts.Node) => { + if ( + ts.isCallExpression(node) && + node.expression.kind === ts.SyntaxKind.ImportKeyword + ) { + lines.push(file.getLineAndCharacterOfPosition(node.getStart()).line + 1); + } + ts.forEachChild(node, visit); + }; + visit(file); + return lines; +} + +describe("guards load their modules with the file", () => { + const files = requireFloor("guard files", guardFiles(), 150); + + it("finds a dynamic import where there is one", () => { + const probe = ts.createSourceFile( + "probe.ts", + 'it("x", async () => { await import("@/lib/db"); }); const s = "import(\\"a\\")";', + ts.ScriptTarget.Latest, + true, + ); + let found = 0; + const visit = (node: ts.Node) => { + if ( + ts.isCallExpression(node) && + node.expression.kind === ts.SyntaxKind.ImportKeyword + ) { + found += 1; + } + ts.forEachChild(node, visit); + }; + visit(probe); + expect(found).toBe(1); + }); + + it("calls import() in no guard", () => { + const found = files.flatMap((rel) => + dynamicImports(rel).map((line) => `${rel}:${line}`), + ); + expect(found).toEqual([]); + }); +}); diff --git a/src/__tests__/next-image-optimizer-guard.test.ts b/src/__tests__/next-image-optimizer-guard.test.ts index a1732e4e6..b696be792 100644 --- a/src/__tests__/next-image-optimizer-guard.test.ts +++ b/src/__tests__/next-image-optimizer-guard.test.ts @@ -20,6 +20,8 @@ import { readFileSync } from "node:fs"; import { join } from "node:path"; import { describe, expect, it } from "vitest"; +import nextConfig from "../../next.config"; + import { requireFloor, stripComments, @@ -34,12 +36,13 @@ type ImagesConfig = { dangerouslyAllowSVG?: boolean; }; -async function imagesConfig(): Promise { - const mod = (await import("../../next.config")) as { - default: { images?: ImagesConfig }; - }; - return mod.default.images ?? {}; -} +/** + * The config as Next reads it, imported once with the file rather than inside + * a test: the config pulls in Next's plugins, and loading that graph in a test + * body put the import against the test timeout on a loaded machine. + */ +const IMAGES: ImagesConfig = + (nextConfig as { images?: ImagesConfig }).images ?? {}; // Any import of the module: static, dynamic, `require`, side effect or // re-export. Two literals with one shape rather than one built from a string, @@ -89,22 +92,22 @@ describe("next/image optimizer", () => { requireFloor("next/link importers", importers(NEXT_LINK_IMPORT), 100); }); - it("is off, and then nothing imports next/image", async () => { - const images = await imagesConfig(); + it("is off, and then nothing imports next/image", () => { + const images = IMAGES; if (images.unoptimized !== true) return; expect(importers(NEXT_IMAGE_IMPORT)).toEqual([]); }); - it("is never on with SVG input allowed", async () => { - const images = await imagesConfig(); + it("is never on with SVG input allowed", () => { + const images = IMAGES; if (images.unoptimized === true) return; expect(images.dangerouslyAllowSVG ?? false).toBe(false); }); - it("is off today", async () => { + it("is off today", () => { // The two cases above each skip on the other branch. This pins which // branch the tree is on, so flipping it is a visible change to this // file and not a silent switch between the two. - expect((await imagesConfig()).unoptimized).toBe(true); + expect(IMAGES.unoptimized).toBe(true); }); }); diff --git a/src/__tests__/proxy-relative-redirect-guard.test.ts b/src/__tests__/proxy-relative-redirect-guard.test.ts index 7ddc32cba..21fe0eac2 100644 --- a/src/__tests__/proxy-relative-redirect-guard.test.ts +++ b/src/__tests__/proxy-relative-redirect-guard.test.ts @@ -31,6 +31,8 @@ import { readFileSync } from "node:fs"; import { resolve } from "node:path"; import { describe, expect, it } from "vitest"; +import { relativeRedirect } from "@/lib/http/relative-redirect"; + const REPO_ROOT = resolve(__dirname, "../.."); /** @@ -82,9 +84,7 @@ describe("redirects to this deployment do not name a host", () => { ).toEqual([]); }); - it("the helper refuses a value that is a host in disguise", async () => { - const { relativeRedirect } = await import("@/lib/http/relative-redirect"); - + it("the helper refuses a value that is a host in disguise", () => { // `//evil.example` is protocol-relative: a browser reads it as a host. expect(() => relativeRedirect("//evil.example/auth/login")).toThrow( /root-relative/, diff --git a/src/__tests__/step-up-elevation-guard.test.ts b/src/__tests__/step-up-elevation-guard.test.ts index 1c374e284..95ae6265d 100644 --- a/src/__tests__/step-up-elevation-guard.test.ts +++ b/src/__tests__/step-up-elevation-guard.test.ts @@ -31,6 +31,10 @@ import { readSourceText, walkSourceFiles, } from "./helpers/source-files"; +import { + FRESH_FACTOR_METHODS, + isFreshFactorMethod, +} from "@/lib/auth/step-up-factors"; const SRC = join(process.cwd(), "src"); @@ -369,12 +373,14 @@ describe("S4 — the mint surface refuses a cookie", () => { }); describe("S5 — a password proof cannot satisfy a fresh-factor gate", () => { - it("the fresh-factor set excludes password", async () => { + it("the fresh-factor set excludes password", () => { // The B1 invariant, pinned at the definition. The web stamps // `mfaVerifiedAt` only for a completed second factor or a primary passkey - // login; the Bearer arm must not be looser. - const { FRESH_FACTOR_METHODS, isFreshFactorMethod } = - await import("@/lib/auth/step-up"); + // login; the Bearer arm must not be looser. Read from the definition's + // own module, which imports nothing: reaching it through `step-up.ts` + // loaded the database client and the generated Prisma client inside this + // test body, and that import alone ran past the test timeout on a loaded + // machine. expect([...FRESH_FACTOR_METHODS].sort()).toEqual([ "passkey", "totp", diff --git a/src/app/api/auth/step-up/route.ts b/src/app/api/auth/step-up/route.ts index 2c9d6dfcc..6592d81c2 100644 --- a/src/app/api/auth/step-up/route.ts +++ b/src/app/api/auth/step-up/route.ts @@ -47,10 +47,12 @@ import { } from "@/lib/auth/existing-factor-proof"; import { mintStepUpElevation, - isFreshFactorMethod, STEP_UP_ELEVATION_TTL_SECONDS, - type StepUpMethod, } from "@/lib/auth/step-up"; +import { + isFreshFactorMethod, + type StepUpMethod, +} from "@/lib/auth/step-up-factors"; import { stepUpMintSchema } from "@/lib/validations/step-up"; export const dynamic = "force-dynamic"; diff --git a/src/lib/auth/__tests__/step-up.test.ts b/src/lib/auth/__tests__/step-up.test.ts index ad8c33da1..c240cb40b 100644 --- a/src/lib/auth/__tests__/step-up.test.ts +++ b/src/lib/auth/__tests__/step-up.test.ts @@ -38,10 +38,10 @@ const { mintStepUpElevation, claimStepUpElevation, validateStepUpElevation, - isFreshFactorMethod, - FRESH_FACTOR_METHODS, STEP_UP_ELEVATION_TTL_SECONDS, } = await import("@/lib/auth/step-up"); +const { isFreshFactorMethod, FRESH_FACTOR_METHODS } = + await import("@/lib/auth/step-up-factors"); const { MFA_STEP_UP_MAX_AGE_SECONDS, STEP_UP_ELEVATION_HEADER } = await import("@/lib/api-handler"); const { hashToken } = await import("@/lib/auth/hmac"); diff --git a/src/lib/auth/step-up-factors.ts b/src/lib/auth/step-up-factors.ts new file mode 100644 index 000000000..be47d387d --- /dev/null +++ b/src/lib/auth/step-up-factors.ts @@ -0,0 +1,45 @@ +/** + * Which re-proved factors satisfy a fresh-factor gate. + * + * On its own, with nothing imported, because it is an authorisation input + * the structural guards read directly: the definition is pinned by + * `step-up-elevation-guard.test.ts`, and reaching it through `step-up.ts` + * meant loading the database client and the generated Prisma client inside + * a test body. + */ +/** + * Which factor was re-proved. This is an AUTHORISATION INPUT — see + * `FRESH_FACTOR_METHODS` and the file header. + * + * password — the account password. Reaches what a plain cookie session + * reaches, and no further. + * totp — a current code from the enrolled authenticator. + * webauthn — an assertion from a registered second-factor security key. + * passkey — an assertion from a primary passkey. At parity with + * `/api/auth/passkey/login-verify`, which stamps `mfaVerifiedAt` + * on the web for the very same ceremony. + */ +export type StepUpMethod = "password" | "totp" | "webauthn" | "passkey"; + +/** + * The methods that satisfy a fresh-factor gate — exactly the set for which the + * web writes `mfaVerifiedAt`. `password` is deliberately absent: a password + * login has never satisfied step-up on the web and must not start here. + * + * A recovery code is also absent, and that IS a narrowing relative to the web, + * where `/api/auth/mfa/verify` accepts one and does stamp the session. The + * consequence is that an account which has lost its authenticator cannot + * disable its second factor from the app — only from the web. That asymmetry is + * deliberate and flagged rather than quietly closed, because admitting a + * recovery code here means spending one break-glass credential to authorise + * spending another. + */ +export const FRESH_FACTOR_METHODS: ReadonlySet = new Set([ + "totp", + "webauthn", + "passkey", +]); + +export function isFreshFactorMethod(method: StepUpMethod): boolean { + return FRESH_FACTOR_METHODS.has(method); +} diff --git a/src/lib/auth/step-up.ts b/src/lib/auth/step-up.ts index b718e54c2..bfdd03bd5 100644 --- a/src/lib/auth/step-up.ts +++ b/src/lib/auth/step-up.ts @@ -53,6 +53,10 @@ import { randomBytes } from "node:crypto"; import { Prisma } from "@/generated/prisma/client"; import { prisma } from "@/lib/db"; import { hashToken } from "@/lib/auth/hmac"; +import { + isFreshFactorMethod, + type StepUpMethod, +} from "@/lib/auth/step-up-factors"; /** * Marks a value as a step-up elevation. Distinct from every other credential @@ -76,43 +80,6 @@ const ELEVATION_SECRET_BYTES = 32; */ export const STEP_UP_ELEVATION_TTL_SECONDS = 5 * 60; -/** - * Which factor was re-proved. This is an AUTHORISATION INPUT — see - * `FRESH_FACTOR_METHODS` and the file header. - * - * password — the account password. Reaches what a plain cookie session - * reaches, and no further. - * totp — a current code from the enrolled authenticator. - * webauthn — an assertion from a registered second-factor security key. - * passkey — an assertion from a primary passkey. At parity with - * `/api/auth/passkey/login-verify`, which stamps `mfaVerifiedAt` - * on the web for the very same ceremony. - */ -export type StepUpMethod = "password" | "totp" | "webauthn" | "passkey"; - -/** - * The methods that satisfy a fresh-factor gate — exactly the set for which the - * web writes `mfaVerifiedAt`. `password` is deliberately absent: a password - * login has never satisfied step-up on the web and must not start here. - * - * A recovery code is also absent, and that IS a narrowing relative to the web, - * where `/api/auth/mfa/verify` accepts one and does stamp the session. The - * consequence is that an account which has lost its authenticator cannot - * disable its second factor from the app — only from the web. That asymmetry is - * deliberate and flagged rather than quietly closed, because admitting a - * recovery code here means spending one break-glass credential to authorise - * spending another. - */ -export const FRESH_FACTOR_METHODS: ReadonlySet = new Set([ - "totp", - "webauthn", - "passkey", -]); - -export function isFreshFactorMethod(method: StepUpMethod): boolean { - return FRESH_FACTOR_METHODS.has(method); -} - export interface MintedElevation { /** The raw `hle_<64 hex>` value. Returned once; never persisted, never logged. */ token: string;