mirror of
https://github.com/MBombeck/HealthLog.git
synced 2026-11-10 05:12:14 +00:00
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.
This commit is contained in:
1 parent
2357807706
commit
3e8846929c
8 files changed
+179
-60
No files matched your search
@@ -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([]);
|
||||
});
|
||||
});
|
||||
@@ -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<ImagesConfig> {
|
||||
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);
|
||||
});
|
||||
});
|
||||
@@ -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/,
|
||||
|
||||
@@ -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",
|
||||
|
||||
@@ -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";
|
||||
|
||||
@@ -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");
|
||||
|
||||
@@ -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<StepUpMethod> = new Set([
|
||||
"totp",
|
||||
"webauthn",
|
||||
"passkey",
|
||||
]);
|
||||
|
||||
export function isFreshFactorMethod(method: StepUpMethod): boolean {
|
||||
return FRESH_FACTOR_METHODS.has(method);
|
||||
}
|
||||
+4
-37
@@ -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<StepUpMethod> = 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;
|
||||
|
||||
Reference in new issue
Block a user