From 84437d5f950a6367e7c186d657449d67acc167d7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Bombeck?= Date: Thu, 3 Sep 2026 01:42:16 +0200 Subject: [PATCH 1/3] test: read every spelling of a dependency override pin MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The lockstep guard compared only the pins both matchers happened to recognise, and neither matcher said so when it recognised fewer. Workspace side: the key had to be quoted. Every entry in the file carries an `@` suffix and therefore needs quoting, so the matcher looked complete — but `mysql2: "^3.22.0"` is a valid pnpm override with no selector and no quotes, and it parsed to nothing. Unquoting that key and pointing the Dockerfile at `^1.0.0` left all six tests green. Dockerfile side: only `npm pkg set 'overrides.X=Y'` was recognised, so re-quoting the same line as `npm pkg set overrides.X=Y` removed the pin from the comparison without changing what the image installs. Both sides now parse line by line: quoted or bare keys and values on the workspace side, any shell quoting of the same assignment on the Dockerfile side, plus a raw JSON `"overrides"` block. What neither table can read is collected and asserted empty, so a future spelling fails loudly instead of shrinking the compared set in silence. --- ...dependency-override-lockstep-guard.test.ts | 313 +++++++++++++++--- 1 file changed, 269 insertions(+), 44 deletions(-) diff --git a/src/__tests__/dependency-override-lockstep-guard.test.ts b/src/__tests__/dependency-override-lockstep-guard.test.ts index 029e10fd6..9d5133b2c 100644 --- a/src/__tests__/dependency-override-lockstep-guard.test.ts +++ b/src/__tests__/dependency-override-lockstep-guard.test.ts @@ -20,16 +20,31 @@ * would encode noise. It asserts agreement where both files speak, which is * the failure that actually happened. * - * The matcher had a blind spot of its own until 2026-09-02: it refused any - * workspace key beginning with `@`, so every scoped package was invisible to - * it. `@hono/node-server` was pinned on one side and unpinned on the other and - * the guard stayed green, which is the same shape of green the guard exists to - * prevent. Scoped keys parse now, and a test below pins that they do. + * The matcher has now been widened twice, and both widenings were the same + * class of hole: a spelling the files accept and the regex did not. * - * Limit, written down so the next reader does not over-trust a green run: this - * compares declared ranges. It cannot see a NEW vulnerable transitive that - * neither file mentions. Only the image scan can, and that job runs on - * main-push, not on pull requests. + * - 2026-09-02: the workspace side refused any key beginning with `@`, so + * every scoped package was invisible. `@hono/node-server` was pinned on + * one side and unpinned on the other and the guard stayed green. + * - 2026-09-03: the workspace side demanded a QUOTED key, and the Dockerfile + * side demanded the exact spelling `npm pkg set 'overrides.X=Y'`. Every + * entry in the file happens to carry an `@` suffix, which needs + * quoting, so the matcher looked complete — but `mysql2: "^3.22.0"` is a + * legal pnpm override with no selector and no quotes, and it parsed to + * nothing. Unquoting that key and pointing the Dockerfile at `^1.0.0` + * left all six tests green. + * + * Both sides now parse line by line and collect what they could NOT read into + * an `unparsed` list, which is asserted empty. A future spelling neither + * matcher knows fails the guard loudly instead of shrinking the compared set + * in silence, which is the only structural defence against a third repeat. + * + * Limits, written down so the next reader does not over-trust a green run: + * this compares DECLARED ranges. It cannot see a NEW vulnerable transitive + * that neither file mentions — only the image scan can, and that job runs on + * main-push, not on pull requests. It also reads the Dockerfile as text, so a + * pin injected by a build ARG, an `npm config` call, or a package.json copied + * in from elsewhere is outside both matchers. */ import { readFileSync } from "node:fs"; import { join } from "node:path"; @@ -40,21 +55,19 @@ const ROOT = process.cwd(); const DOCKERFILE = readFileSync(join(ROOT, "Dockerfile"), "utf8"); const WORKSPACE = readFileSync(join(ROOT, "pnpm-workspace.yaml"), "utf8"); -/** `npm pkg set 'overrides.='` occurrences in the Dockerfile. */ -function dockerfileOverrides(): Map { - const found = new Map(); - const pattern = /npm\s+pkg\s+set\s+['"]overrides\.([^='"]+)=([^'"]+)['"]/g; - for (const match of DOCKERFILE.matchAll(pattern)) { - found.set(match[1].trim(), match[2].trim()); - } - return found; -} +/** What a side of the comparison parsed, and what it had to give up on. */ +type ParsedOverrides = { + /** Bare package name → declared range. */ + pins: Map; + /** Lines that name an override but no matcher could read. */ + unparsed: string[]; +}; /** - * Reduces an overrides key to the bare package name. The key carries a version - * selector, so `deepmerge-ts@<8.0.0` and a bare `deepmerge-ts` both land on the - * same name. A scoped name opens with its own `@`, so the selector starts at - * the SECOND one, not the first. + * Reduces an overrides key to the bare package name. The key MAY carry a + * version selector, so `deepmerge-ts@<8.0.0` and a bare `deepmerge-ts` both + * land on the same name. A scoped name opens with its own `@`, so the selector + * starts at the SECOND one, not the first. */ function packageNameOf(key: string): string { const trimmed = key.trim(); @@ -64,33 +77,172 @@ function packageNameOf(key: string): string { return (selectorAt === -1 ? trimmed : trimmed.slice(0, selectorAt)).trim(); } -/** - * `"@": ""` entries under the workspace `overrides:` - * block. - */ -function workspaceOverrides(): Map { - const found = new Map(); - const pattern = /^\s+"([^"]+)"\s*:\s*"([^"]+)"/gm; - const block = WORKSPACE.split(/^allowBuilds:/m)[0]; - for (const match of block.matchAll(pattern)) { - found.set(packageNameOf(match[1]), match[2].trim()); - } - return found; +/** Strips one layer of matching YAML/shell quotes, if present. */ +function unquote(raw: string): string { + const trimmed = raw.trim(); + const quoted = /^"([^"]*)"$/.exec(trimmed) ?? /^'([^']*)'$/.exec(trimmed); + return quoted ? quoted[1] : trimmed; } +/** + * Splits one `: ` mapping line. A QUOTED key may itself contain a + * colon, so the separator is looked for after the closing quote; an unquoted + * key cannot contain one, so the first colon is the separator. + */ +function splitMapping(line: string): [string, string] | null { + const body = line.trim(); + const quote = body.startsWith('"') ? '"' : body.startsWith("'") ? "'" : null; + if (quote) { + const close = body.indexOf(quote, 1); + if (close === -1) return null; + const rest = body.slice(close + 1).trimStart(); + if (!rest.startsWith(":")) return null; + return [body.slice(0, close + 1), rest.slice(1)]; + } + const colon = body.indexOf(":"); + if (colon === -1) return null; + return [body.slice(0, colon), body.slice(colon + 1)]; +} + +/** Reads a YAML scalar value, dropping any trailing `# comment`. */ +function scalarValue(raw: string): string { + const trimmed = raw.trim(); + const quoted = /^"([^"]*)"/.exec(trimmed) ?? /^'([^']*)'/.exec(trimmed); + if (quoted) return quoted[1]; + return trimmed.split(/\s+#/)[0].trim(); +} + +/** + * Entries under the workspace `overrides:` block. + * + * pnpm accepts the key with or without a version selector and YAML accepts + * either half quoted or bare, so all four spellings below are the same pin and + * all four must parse: + * + * "mysql2@<3.22.0": "^3.22.0" + * mysql2@<3.22.0: ^3.22.0 + * "mysql2": "^3.22.0" + * mysql2: ^3.22.0 + */ +export function parseWorkspaceOverrides(yaml: string): ParsedOverrides { + const pins = new Map(); + const unparsed: string[] = []; + const lines = yaml.split(/\r?\n/); + const start = lines.findIndex((line) => /^overrides:\s*$/.test(line)); + if (start === -1) { + return { pins, unparsed: ["no `overrides:` block found"] }; + } + + for (const line of lines.slice(start + 1)) { + if (line.trim() === "") continue; + // A line at column zero is the next top-level key; the block ends there. + if (!/^\s/.test(line)) break; + if (/^\s*#/.test(line)) continue; + + const mapping = splitMapping(line); + const range = mapping ? scalarValue(mapping[1]) : ""; + if (!mapping || range === "") { + unparsed.push(line.trim()); + continue; + } + pins.set(packageNameOf(unquote(mapping[0])), range); + } + return { pins, unparsed }; +} + +/** + * `overrides` entries written into `/opt/prisma-cli/package.json`. + * + * `npm pkg set` is indifferent to shell quoting, so the assignment may be + * quoted whole, quoted per half, or bare. Any non-comment Dockerfile line that + * names `overrides` and matches none of the accepted spellings is reported as + * unparsed rather than skipped — that is what stops a fresh spelling from + * quietly shrinking the compared set. + */ +export function parseDockerfileOverrides(dockerfile: string): ParsedOverrides { + const pins = new Map(); + const unparsed: string[] = []; + + // `npm pkg set 'overrides.='`, `"overrides.="`, + // `overrides.=`, and the per-half-quoted variants. A package + // name and a semver range never contain a quote character, so quoting is + // pure shell decoration here: strip it and every spelling collapses onto the + // same `overrides.=` token. + const npmPkgSet = /npm\s+pkg\s+set\s+overrides\.([^\s=]+)\s*=\s*([^\s&;|]+)/; + // A raw JSON field, e.g. inside a `cat > package.json` heredoc: + // `"": ""` on a line under an `"overrides"` key. + const jsonField = /"([^"]+)"\s*:\s*"([^"]+)"/; + + let insideJsonOverrides = false; + for (const rawLine of dockerfile.split(/\r?\n/)) { + const line = rawLine.trim(); + if (line.startsWith("#")) continue; + + if (insideJsonOverrides) { + if (line.startsWith("}")) { + insideJsonOverrides = false; + continue; + } + const field = jsonField.exec(line); + if (field) { + pins.set(packageNameOf(field[1]), field[2].trim()); + continue; + } + unparsed.push(line); + continue; + } + + if (!/\boverrides\b/.test(line)) continue; + + if (/"overrides"\s*:\s*\{\s*$/.test(line)) { + insideJsonOverrides = true; + continue; + } + + const set = npmPkgSet.exec(line.replace(/['"]/g, "")); + if (set) { + pins.set(packageNameOf(set[1]), set[2].trim()); + continue; + } + unparsed.push(line); + } + return { pins, unparsed }; +} + +const dockerfileOverrides = () => parseDockerfileOverrides(DOCKERFILE); +const workspaceOverrides = () => parseWorkspaceOverrides(WORKSPACE); + describe("dependency overrides — pnpm workspace and the npm install agree", () => { it("finds the Dockerfile's npm-side overrides at all", () => { const docker = dockerfileOverrides(); // A zero-match regex would make every assertion below vacuously true. // This is the check that keeps the guard honest about its own matcher. - expect(docker.size).toBeGreaterThan(0); - expect(docker.has("deepmerge-ts")).toBe(true); + expect(docker.pins.size).toBeGreaterThan(0); + expect(docker.pins.has("deepmerge-ts")).toBe(true); }); it("finds the workspace overrides at all", () => { const workspace = workspaceOverrides(); - expect(workspace.size).toBeGreaterThan(0); - expect(workspace.has("deepmerge-ts")).toBe(true); + expect(workspace.pins.size).toBeGreaterThan(0); + expect(workspace.pins.has("deepmerge-ts")).toBe(true); + }); + + it("refuses an overrides line it could not read on either side", () => { + // The failure this replaces: a matcher that silently narrows the compared + // set is indistinguishable from a matcher that found agreement. Anything + // neither spelling table covers has to surface here. + expect( + workspaceOverrides().unparsed, + "pnpm-workspace.yaml has an `overrides:` entry this guard cannot read. " + + "Teach `parseWorkspaceOverrides` the spelling — do not leave it " + + "unparsed, or the pin stops being compared against the Dockerfile.", + ).toEqual([]); + expect( + dockerfileOverrides().unparsed, + "The Dockerfile names `overrides` on a line this guard cannot read. " + + "Teach `parseDockerfileOverrides` the spelling — an unread pin is an " + + "uncompared pin.", + ).toEqual([]); }); it("reads scoped workspace keys rather than skipping them", () => { @@ -105,14 +257,87 @@ describe("dependency overrides — pnpm workspace and the npm install agree", () expect(packageNameOf("js-yaml@>=4.0.0 <4.3.1")).toBe("js-yaml"); expect(packageNameOf("deepmerge-ts")).toBe("deepmerge-ts"); - const workspace = workspaceOverrides(); - const scoped = [...workspace.keys()].filter((name) => name.startsWith("@")); + const scoped = [...workspaceOverrides().pins.keys()].filter((name) => + name.startsWith("@"), + ); expect(scoped.length).toBeGreaterThan(0); }); + it("reads a workspace pin that carries no selector and no quotes", () => { + // `mysql2: "^3.22.0"` is a valid pnpm override and valid YAML. The old + // matcher required a quoted key, so it parsed to nothing — and because + // every entry in the file happens to need quoting for its selector, the + // hole was invisible by inspection. All four spellings are one pin. + const parsed = parseWorkspaceOverrides( + [ + "overrides:", + " # a comment inside the block", + ' "quoted-selector@<2.0.0": "^2.0.0"', + " bare-selector@<2.0.0: ^2.0.0", + ' "quoted-plain": "^2.0.0"', + " bare-plain: ^2.0.0", + ' "@scoped/bare-value@<2.0.0": ^2.0.0', + " trailing-comment: ^2.0.0 # note", + "", + "allowBuilds:", + " prisma: true", + ].join("\n"), + ); + + expect(parsed.unparsed).toEqual([]); + expect(Object.fromEntries(parsed.pins)).toEqual({ + "quoted-selector": "^2.0.0", + "bare-selector": "^2.0.0", + "quoted-plain": "^2.0.0", + "bare-plain": "^2.0.0", + "@scoped/bare-value": "^2.0.0", + "trailing-comment": "^2.0.0", + }); + // The block ends at the next top-level key: `allowBuilds` is not a pin. + expect(parsed.pins.has("prisma")).toBe(false); + }); + + it("reads every spelling of the Dockerfile's npm-side pin", () => { + // The old matcher accepted `npm pkg set 'overrides.X=Y'` and nothing else, + // so re-quoting the same line removed the pin from the comparison without + // changing what the image installs. + const parsed = parseDockerfileOverrides( + [ + "# a comment mentioning the `overrides` block", + "RUN npm init -y && \\", + " npm pkg set 'overrides.single-quoted=^1.0.0' && \\", + ' npm pkg set "overrides.double-quoted=^1.0.0" && \\', + " npm pkg set overrides.unquoted=^1.0.0 && \\", + " npm pkg set 'overrides.@scoped/split'='^1.0.0' && \\", + " npm install --omit=dev prisma@7.8.0", + ].join("\n"), + ); + + expect(parsed.unparsed).toEqual([]); + expect(Object.fromEntries(parsed.pins)).toEqual({ + "single-quoted": "^1.0.0", + "double-quoted": "^1.0.0", + unquoted: "^1.0.0", + "@scoped/split": "^1.0.0", + }); + }); + + it("reports an overrides spelling neither matcher knows", () => { + // Written so the refusal itself is proven, not assumed. A guard that + // cannot fail is the failure mode this file exists to close. + expect( + parseWorkspaceOverrides(["overrides:", " - not-a-mapping"].join("\n")) + .unparsed, + ).toEqual(["- not-a-mapping"]); + expect( + parseDockerfileOverrides("RUN node -e \"pkg.overrides['x']='^1.0.0'\"") + .unparsed.length, + ).toBeGreaterThan(0); + }); + it("pins the same range wherever both files name the same package", () => { - const docker = dockerfileOverrides(); - const workspace = workspaceOverrides(); + const docker = dockerfileOverrides().pins; + const workspace = workspaceOverrides().pins; const shared = [...docker.keys()].filter((name) => workspace.has(name)); expect(shared.length).toBeGreaterThan(0); @@ -131,7 +356,7 @@ describe("dependency overrides — pnpm workspace and the npm install agree", () // These three were each reported by the image scan under // `/opt/prisma-cli`, reached through `prisma`. Losing a mirror is not a // style regression, it is the vulnerable version coming back. - const docker = dockerfileOverrides(); + const docker = dockerfileOverrides().pins; for (const name of ["deepmerge-ts", "@hono/node-server", "valibot"]) { expect( docker.has(name), @@ -145,7 +370,7 @@ describe("dependency overrides — pnpm workspace and the npm install agree", () it("keeps the override ahead of the version Prisma's config asks for", () => { // `@prisma/config` requests deepmerge-ts 7.x; the advisory is fixed in // 8.0.0. Anything that lets a 7.x back in re-opens CVE-2026-40345. - const range = dockerfileOverrides().get("deepmerge-ts"); + const range = dockerfileOverrides().pins.get("deepmerge-ts"); expect(range).toBeDefined(); expect(range).toMatch(/\^?8\./); }); From 1f5115fbf3400c88818cdafded20b2b5420ff8d2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Bombeck?= Date: Thu, 3 Sep 2026 01:46:29 +0200 Subject: [PATCH 2/3] fix(lint): see positional query keys and the app tree MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two holes, both proven with a probe that produced zero errors. The rule matched the object properties `queryKey:` and `mutationKey:` and nothing else, so the positional form was never seen — `setQueryData(["bare"], 1)` seeds the same cache cell a `queryKey:` reads, and `getQueryData` / `getQueryState` / the four defaults setters read and write it the same way. Those seven methods are checked at their first argument now, whether the call is bare, a member, or a computed member. The enforced roots were components, hooks, query modules, and files carrying their own "use client", which left src/app out. That is where the server prefetch pages build the keys the client later reads, and they carry no directive, so a server key diverging from the client's was unguarded. src/app is a root now. No violation surfaces on the current tree. The rule header records what it still cannot see: a key assembled into a variable before the call site, and the method-name match, which neither follows a project helper nor knows an unrelated object with the same method name. --- .../__tests__/queryKey-factory.test.mjs | 87 ++++++++++++++++- eslint-plugins/healthlog/queryKey-factory.js | 95 +++++++++++++++++-- 2 files changed, 172 insertions(+), 10 deletions(-) diff --git a/eslint-plugins/healthlog/__tests__/queryKey-factory.test.mjs b/eslint-plugins/healthlog/__tests__/queryKey-factory.test.mjs index 365107dc2..d352bf4a4 100644 --- a/eslint-plugins/healthlog/__tests__/queryKey-factory.test.mjs +++ b/eslint-plugins/healthlog/__tests__/queryKey-factory.test.mjs @@ -34,9 +34,32 @@ ruleTester.run("queryKey-factory", rule, { code: 'useQuery({ queryKey: ["fixture"] });', filename: "/repo/src/components/__tests__/example.test.tsx", }, + // A server module outside every guarded root stays unguarded — the roots + // are the scope decision, not an accident of this fixture. { - code: 'useQuery({ queryKey: ["server-prefetch"] });', - filename: "/repo/src/app/server-page.tsx", + code: 'useQuery({ queryKey: ["server-only-lib"] });', + filename: "/repo/src/lib/reports/build-report.ts", + }, + // The positional cache API, used correctly: the key still comes from the + // factory, so the shape that poisons the cache never arises. + { + code: "queryClient.setQueryData(queryKeys.example(id), next);", + filename: "/repo/src/app/page.tsx", + }, + { + code: "const key = queryKeys.example(id); queryClient.setQueryData(key, next);", + filename: "/repo/src/components/example.tsx", + }, + // Tests seed caches with literal tuples on purpose; the factory directory + // constructs them by definition. Both stay exempt on the positional form + // exactly as they are on the object-property form. + { + code: 'client.setQueryData(["fixture"], 1);', + filename: "/repo/src/hooks/__tests__/use-example.test.ts", + }, + { + code: 'client.setQueryData(["fixture"], 1);', + filename: "/repo/src/lib/query-keys/__fixtures__/seed.ts", }, ], invalid: [ @@ -60,5 +83,65 @@ ruleTester.run("queryKey-factory", rule, { filename: "/repo/src/lib/client-state.tsx", errors: [{ messageId: "bareArray" }], }, + // src/app is where the server prefetch pages build the keys the client + // then reads. A key that differs from the client's is a cache miss at + // best and a poisoned cell at worst, and without `"use client"` on the + // page the rule used to skip the file entirely. + { + code: 'useQuery({ queryKey: ["server-prefetch"] });', + filename: "/repo/src/app/server-page.tsx", + errors: [{ messageId: "bareArray" }], + }, + { + code: 'queryClient.prefetchQuery({ queryKey: ["dashboard"], queryFn: load });', + filename: "/repo/src/app/coach/page.tsx", + errors: [{ messageId: "bareArray" }], + }, + // The positional form of the same mistake. `setQueryData` seeds a cache + // cell by key exactly as `queryKey:` reads one, so a bare array here is + // the same defect written differently. + { + code: 'queryClient.setQueryData(["bare", "positional"], 1);', + filename: "/repo/src/app/coach/page.tsx", + errors: [{ messageId: "positionalArray" }], + }, + { + code: 'const cached = queryClient.getQueryData(["bare"]);', + filename: "/repo/src/hooks/use-example.ts", + errors: [{ messageId: "positionalArray" }], + }, + { + code: 'queryClient.getQueryState(["bare"]);', + filename: "/repo/src/components/example.tsx", + errors: [{ messageId: "positionalArray" }], + }, + { + code: 'queryClient.setQueryDefaults(["bare"], { staleTime: 0 });', + filename: "/repo/src/lib/queries/use-example.ts", + errors: [{ messageId: "positionalArray" }], + }, + { + code: 'queryClient.setMutationDefaults(["bare"], { retry: 0 });', + filename: "/repo/src/components/example.tsx", + errors: [{ messageId: "positionalArray" }], + }, + // Destructured off the client, and reached through a computed member — + // both spell the same call and both must be seen. + { + code: 'const { setQueryData } = useQueryClient(); setQueryData(["bare"], 1);', + filename: "/repo/src/components/example.tsx", + errors: [{ messageId: "positionalArray" }], + }, + { + code: 'queryClient["setQueryData"](["bare"], 1);', + filename: "/repo/src/components/example.tsx", + errors: [{ messageId: "positionalArray" }], + }, + // Both holes in one file, which is the probe that found them. + { + code: 'queryClient.setQueryData(["bare", "positional"], 1); useQuery({ queryKey: ["bare", "object-property"] });', + filename: "/repo/src/app/coach/page.tsx", + errors: [{ messageId: "positionalArray" }, { messageId: "bareArray" }], + }, ], }); diff --git a/eslint-plugins/healthlog/queryKey-factory.js b/eslint-plugins/healthlog/queryKey-factory.js index 3047084eb..548cb44c5 100644 --- a/eslint-plugins/healthlog/queryKey-factory.js +++ b/eslint-plugins/healthlog/queryKey-factory.js @@ -2,16 +2,39 @@ * @fileoverview ESLint rule — `queryKey` / `mutationKey` literal-array * factory enforcement across the complete client tree. * - * Every TanStack key must come from `src/lib/query-keys/`. The rule flags - * object-property `queryKey: [ … ]` and `mutationKey: [ … ]` values in - * components, hooks, client query modules, and any source module declaring - * top-level `"use client"`. + * Every TanStack key must come from `src/lib/query-keys/`. The rule flags a + * bare array wherever a key is supplied, in either of the two shapes TanStack + * accepts: + * + * - the object property, `{ queryKey: [ … ] }` / `{ mutationKey: [ … ] }`; + * - the FIRST POSITIONAL argument of the QueryClient methods that take a key + * directly, `setQueryData([ … ], value)`, `getQueryData([ … ])`, and their + * siblings below. + * + * The positional shape was invisible until 2026-09-03. `setQueryData` seeds + * the same cache cell that `queryKey:` reads, so a literal there is the same + * defect written differently — and a probe placing one in a component + * produced no error at all. + * + * Enforced roots: `src/components/`, `src/hooks/`, `src/lib/queries/`, + * `src/app/`, and any source module declaring top-level `"use client"`. + * `src/app/` joined the list on the same date: the server prefetch pages there + * build the keys the client subsequently reads, and a server key that differs + * from the client's is a cache miss at best and a poisoned cell at worst. They + * carry no `"use client"` directive, so nothing guarded them before. * * Accepted call-site shapes include direct factory calls, identifiers already * built from the factory, and conditionals selecting factory calls. Tests and * the factory definition directory are exempt because they intentionally * construct literal fixtures and tuples. * + * Limits, so a green run is not read as more than it is: the positional check + * matches by METHOD NAME, so a key handed to a helper of the project's own + * naming (`seedCache(["…"], v)`) is not seen, and conversely an unrelated + * object with a `setQueryData` method would be flagged. A key assembled into a + * variable before the call site (`const k = ["a", id]`) is invisible to both + * checks — only the literal at the key position is matched. + * * @see src/lib/query-keys/ * @see src/lib/__tests__/query-keys.test.ts */ @@ -22,7 +45,28 @@ const FACTORY_HOME_DIRECTORY = "src/lib/query-keys"; // Components and hooks are client-facing by convention. Query modules are // client-transitive even if a future refactor moves the directive to a barrel. -const CLIENT_ROOTS = ["src/components/", "src/hooks/", "src/lib/queries/"]; +// `src/app/` is where the server prefetch pages seed the client's cache, so +// their keys have to come from the same factory the client reads from. +const GUARDED_ROOTS = [ + "src/components/", + "src/hooks/", + "src/lib/queries/", + "src/app/", +]; + +// QueryClient methods whose FIRST argument is a query key or mutation key, +// supplied positionally rather than inside an options object. Everything else +// on the client (`invalidateQueries`, `prefetchQuery`, …) takes a filters or +// options object and is covered by the `queryKey:` property check. +const POSITIONAL_KEY_METHODS = new Set([ + "setQueryData", + "getQueryData", + "getQueryState", + "setQueryDefaults", + "getQueryDefaults", + "setMutationDefaults", + "getMutationDefaults", +]); function toPosix(filename) { return filename.replace(/\\/g, "/"); @@ -46,7 +90,7 @@ function isGuarded(filename, sourceCode) { return false; } if (isTestFile(posix)) return false; - if (CLIENT_ROOTS.some((root) => posix.includes(root))) return true; + if (GUARDED_ROOTS.some((root) => posix.includes(root))) return true; return sourceCode.ast.body.some( (statement) => statement.type === "ExpressionStatement" && @@ -54,6 +98,25 @@ function isGuarded(filename, sourceCode) { ); } +/** + * The called name, whether the call is bare (`setQueryData(…)`, destructured + * off the client), a member (`qc.setQueryData(…)`), or a static computed + * member (`qc["setQueryData"](…)`). + */ +function calleeName(callee) { + if (callee.type === "Identifier") return callee.name; + if (callee.type !== "MemberExpression") return null; + if (!callee.computed && callee.property.type === "Identifier") { + return callee.property.name; + } + if (callee.computed && callee.property.type === "Literal") { + return typeof callee.property.value === "string" + ? callee.property.value + : null; + } + return null; +} + /** @type {import('eslint').Rule.RuleModule} */ const queryKeyFactoryRule = { meta: { @@ -66,6 +129,8 @@ const queryKeyFactoryRule = { messages: { bareArray: "Bare-array `{{prop}}: [ … ]` bypasses the queryKeys factory. Import from `@/lib/query-keys` and call `queryKeys.()` so cache-invalidation bundles stay in lockstep.", + positionalArray: + "Bare-array key passed to `{{call}}( … )` bypasses the queryKeys factory. It seeds or reads the same cache cell as a `queryKey`, so import from `@/lib/query-keys` and call `queryKeys.()` instead.", }, schema: [], }, @@ -76,7 +141,7 @@ const queryKeyFactoryRule = { return {}; } - function check(node) { + function checkProperty(node) { // `node` is a Property like `queryKey: [ … ]`. Only flag the // bare ArrayExpression form — identifiers, call expressions, // conditional expressions, etc. are all acceptable indirection @@ -98,8 +163,22 @@ const queryKeyFactoryRule = { }); } + function checkPositional(node) { + const name = calleeName(node.callee); + if (!name || !POSITIONAL_KEY_METHODS.has(name)) return; + const first = node.arguments[0]; + if (!first || first.type !== "ArrayExpression") return; + + context.report({ + node: first, + messageId: "positionalArray", + data: { call: name }, + }); + } + return { - Property: check, + Property: checkProperty, + CallExpression: checkPositional, }; }, }; From 6633f0fb196930c3661d5d48eba36013ea93c6bc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Bombeck?= Date: Thu, 3 Sep 2026 01:50:25 +0200 Subject: [PATCH 3/3] fix(lint): cover the unguarded egress paths and aliased fetch MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two spellings the egress guard could not see, both probed. src/proxy.ts, src/instrumentation.ts and src/cli/ sat outside the enforced roots (src/lib/ and src/app/), and the companion api-fetch-required guards the client surface, so it did not reach them either. A raw fetch dropped into instrumentation.ts linted clean. All three are egress-free today, but instrumentation is exactly where an exporter pointed at an operator-supplied URL would land. The callee match knew the identifier `fetch` and the globalThis / window / self member forms only, so an aliased import escaped it in every file: `import { fetch as f } from "undici"` then `f(url)` linted clean under src/lib. That spelling is what src/lib/safe-fetch.ts itself writes. Local names bound to an imported `fetch`, and namespace-imported `ns.fetch(…)`, are now callees; the wrapper internals and test files stay exempt as before, and the wrapper's own aliased import is pinned as a valid case. Adds the rule's missing unit-test file. The header records what the binding scan still cannot see: require(), dynamic import, a fetch passed as a parameter, and egress that never spells fetch at all. --- .../__tests__/safe-fetch-required.test.mjs | 145 ++++++++++++++++++ .../healthlog/safe-fetch-required.js | 104 ++++++++++--- 2 files changed, 226 insertions(+), 23 deletions(-) create mode 100644 eslint-plugins/healthlog/__tests__/safe-fetch-required.test.mjs diff --git a/eslint-plugins/healthlog/__tests__/safe-fetch-required.test.mjs b/eslint-plugins/healthlog/__tests__/safe-fetch-required.test.mjs new file mode 100644 index 000000000..6d54ae1cd --- /dev/null +++ b/eslint-plugins/healthlog/__tests__/safe-fetch-required.test.mjs @@ -0,0 +1,145 @@ +import { describe, it } from "vitest"; +import { RuleTester } from "eslint"; +import rule from "../safe-fetch-required.js"; + +RuleTester.describe = describe; +RuleTester.it = it; +RuleTester.itOnly = it.only; + +const ruleTester = new RuleTester({ + languageOptions: { + ecmaVersion: 2023, + sourceType: "module", + parserOptions: { ecmaFeatures: { jsx: true } }, + }, +}); + +ruleTester.run("safe-fetch-required", rule, { + valid: [ + { + code: "safeFetch(url, { requirePublicHost: true });", + filename: "/repo/src/lib/notifications/senders/ntfy.ts", + }, + // Same-origin relative paths never leave the origin. + { + code: 'fetch("/api/measurements");', + filename: "/repo/src/app/dashboard/load.ts", + }, + { + code: "fetch(`/api/measurements/${id}`);", + filename: "/repo/src/lib/queries/measurements.ts", + }, + // The wrapper's own internals, including the aliased undici import it + // actually uses. This is the spelling the rule learned to see, so the + // exemption has to be proven, not assumed. + { + code: 'import { fetch as undiciFetch } from "undici"; undiciFetch(url);', + filename: "/repo/src/lib/safe-fetch.ts", + }, + { + code: "fetch(url);", + filename: "/repo/src/lib/safe-fetch-dispatcher.ts", + }, + { + code: "fetch(url);", + filename: "/repo/src/lib/api/api-fetch.ts", + }, + // Tests mock and assert against fetch directly. + { + code: 'import { fetch as f } from "undici"; f(url);', + filename: "/repo/src/lib/__tests__/egress.test.ts", + }, + { + code: "fetch(url);", + filename: "/repo/src/lib/__mocks__/transport.ts", + }, + // Outside every enforced path: scripts and config are out of scope. + { + code: "fetch(url);", + filename: "/repo/scripts/backfill.ts", + }, + // An aliased import pointed at a relative path is still same-origin. + { + code: 'import { fetch as f } from "undici"; f("/api/version");', + filename: "/repo/src/lib/version.ts", + }, + // A named import that is not `fetch` binds nothing this rule cares about. + { + code: 'import { Agent } from "undici"; Agent(url);', + filename: "/repo/src/lib/safe-fetch-dispatcher-helper.ts", + }, + ], + invalid: [ + { + code: "fetch(url);", + filename: "/repo/src/lib/ai/openai-client.ts", + errors: [{ messageId: "rawFetch" }], + }, + { + code: "globalThis.fetch(url);", + filename: "/repo/src/app/api/withings/route.ts", + errors: [{ messageId: "rawFetch" }], + }, + { + code: "self.fetch(url);", + filename: "/repo/src/lib/pwa/register.ts", + errors: [{ messageId: "rawFetch" }], + }, + { + code: 'globalThis["fetch"](url);', + filename: "/repo/src/lib/ai/local-client.ts", + errors: [{ messageId: "rawFetch" }], + }, + // Protocol-relative and backslash forms resolve off-origin, so the + // same-origin exemption must not swallow them. + { + code: 'fetch("//evil.example/api");', + filename: "/repo/src/lib/ai/anthropic-client.ts", + errors: [{ messageId: "rawFetch" }], + }, + // The three paths that sat outside the enforced roots. They are + // egress-free today; instrumentation.ts is where an exporter pointed at + // an operator-supplied URL would land, and nothing would have said so. + { + code: "fetch(url);", + filename: "/repo/src/proxy.ts", + errors: [{ messageId: "rawFetch" }], + }, + { + code: "fetch(otlpEndpoint, { method: 'POST' });", + filename: "/repo/src/instrumentation.ts", + errors: [{ messageId: "rawFetch" }], + }, + { + code: "fetch(url);", + filename: "/repo/src/cli/mcp-stdio.ts", + errors: [{ messageId: "rawFetch" }], + }, + // The aliased import. `src/lib/safe-fetch.ts` writes exactly this + // spelling, so it is a form the codebase demonstrably reaches for, and + // the identifier check never matched it. + { + code: 'import { fetch as f } from "undici"; f(url);', + filename: "/repo/src/lib/ai/codex-client.ts", + errors: [{ messageId: "rawFetch" }], + }, + { + code: 'import { fetch as undiciFetch } from "undici"; undiciFetch(url);', + filename: "/repo/src/instrumentation.ts", + errors: [{ messageId: "rawFetch" }], + }, + // The namespace form of the same escape. + { + code: 'import * as undici from "undici"; undici.fetch(url);', + filename: "/repo/src/lib/notifications/senders/telegram.ts", + errors: [{ messageId: "rawFetch" }], + }, + // A default import named `fetch` was always caught; kept so the widening + // cannot regress it. + { + code: 'import fetch from "node-fetch"; fetch(url);', + filename: "/repo/src/lib/ai/local-client.ts", + errors: [{ messageId: "rawFetch" }], + }, + ], +}); diff --git a/eslint-plugins/healthlog/safe-fetch-required.js b/eslint-plugins/healthlog/safe-fetch-required.js index 4cff31d44..37fdd45c0 100644 --- a/eslint-plugins/healthlog/safe-fetch-required.js +++ b/eslint-plugins/healthlog/safe-fetch-required.js @@ -16,9 +16,22 @@ * mock or assert against `fetch` directly. * * The check is a syntactic `CallExpression` match against a bare - * `fetch(` callee (Identifier `fetch`) and `globalThis.fetch(` / - * `window.fetch(` member forms. `safeFetch` is a distinct identifier and - * never matches. + * `fetch(` callee (Identifier `fetch`), the `globalThis.fetch(` / + * `window.fetch(` / `self.fetch(` member forms, and — since 2026-09-03 — + * any local name bound to a module's `fetch` export or reached through a + * namespace import. `safeFetch` is a distinct identifier and never matches. + * + * The aliased spelling is not hypothetical: `src/lib/safe-fetch.ts` opens + * with `import { fetch as undiciFetch } from "undici"`, so the codebase + * demonstrably reaches for it, and every copy of that spelling outside the + * wrapper escaped the rule in every file. + * + * Limits, so a green run is not read as more than it is: the binding scan + * walks top-level `import` declarations only. A `require("undici").fetch`, + * a dynamic `await import(...)`, a fetch handed through a parameter or + * stored on an object, and a local shadow of an imported name are all + * outside it — as is any egress that never spells `fetch` at all, such as + * `http.request` or a client library with its own transport. * * Same-origin client fetches are exempt: a first argument that is a * string literal (or template head) starting with `/` is a relative, @@ -45,9 +58,27 @@ const EXEMPT_FILES = [ "src/lib/api/api-fetch.ts", ]; -// Only enforce inside the application source roots. Scripts, config, and -// generated code are out of scope. -const ENFORCED_ROOTS = ["src/lib/", "src/app/"]; +// Only enforce inside the application source. Scripts, config, and generated +// code are out of scope. +// +// The list carries three single files alongside the two directory roots. +// `src/proxy.ts`, `src/instrumentation.ts` and `src/cli/` sit outside +// `src/lib/` and `src/app/`, and the companion `healthlog/api-fetch-required` +// does not reach them either (it guards the client surface), so between them +// they were egress-unguarded. They are egress-free today; instrumentation is +// precisely where an exporter pointed at an operator-supplied URL would land, +// and nothing would have flagged it. +const ENFORCED_PATHS = [ + "src/lib/", + "src/app/", + "src/proxy.ts", + "src/instrumentation.ts", + "src/cli/", +]; + +// Objects that carry the platform `fetch`. Treated as namespaces below so the +// member form goes through one code path with the imported-namespace case. +const GLOBAL_FETCH_OBJECTS = ["globalThis", "window", "self"]; function toPosix(filename) { return filename.replace(/\\/g, "/"); @@ -64,7 +95,7 @@ function isTestFile(posix) { function isEnforced(filename) { const posix = toPosix(filename); - if (!ENFORCED_ROOTS.some((root) => posix.includes(root))) return false; + if (!ENFORCED_PATHS.some((path) => posix.includes(path))) return false; if (EXEMPT_FILES.some((f) => posix.includes(f))) return false; if (isTestFile(posix)) return false; return true; @@ -86,27 +117,54 @@ const safeFetchRequiredRule = { }, create(context) { const filename = context.filename ?? context.getFilename?.(); + const sourceCode = context.sourceCode ?? context.getSourceCode(); if (!filename || !isEnforced(filename)) { return {}; } - function isFetchCallee(callee) { - // Bare `fetch(...)`. - if (callee.type === "Identifier" && callee.name === "fetch") return true; - // `globalThis.fetch(...)` / `window.fetch(...)` / `self.fetch(...)`. - if ( - callee.type === "MemberExpression" && - !callee.computed && - callee.property.type === "Identifier" && - callee.property.name === "fetch" && - callee.object.type === "Identifier" && - (callee.object.name === "globalThis" || - callee.object.name === "window" || - callee.object.name === "self") - ) { - return true; + // Local names this module bound to somebody's `fetch`, collected from the + // top-level imports before any call site is visited. `import { fetch as f } + // from "undici"` makes `f(...)` a fetch call; `import * as undici from + // "undici"` makes `undici.fetch(...)` one. + const aliasedFetch = new Set(); + const namespaces = new Set([...GLOBAL_FETCH_OBJECTS]); + for (const node of sourceCode.ast.body) { + if (node.type !== "ImportDeclaration") continue; + for (const specifier of node.specifiers) { + if (specifier.type === "ImportSpecifier") { + const imported = + specifier.imported.type === "Identifier" + ? specifier.imported.name + : specifier.imported.value; + if (imported === "fetch") aliasedFetch.add(specifier.local.name); + } else if (specifier.type === "ImportNamespaceSpecifier") { + namespaces.add(specifier.local.name); + } } - return false; + } + + function isFetchCallee(callee) { + // Bare `fetch(...)`, and any local name bound to an imported `fetch`. + if (callee.type === "Identifier") { + return callee.name === "fetch" || aliasedFetch.has(callee.name); + } + if (callee.type !== "MemberExpression") return false; + // `globalThis.fetch(...)` / `window.fetch(...)` / `self.fetch(...)` and + // `.fetch(...)`, including the static computed spelling. + if ( + callee.object.type !== "Identifier" || + !namespaces.has(callee.object.name) + ) { + return false; + } + const property = !callee.computed + ? callee.property.type === "Identifier" + ? callee.property.name + : null + : callee.property.type === "Literal" + ? callee.property.value + : null; + return property === "fetch"; } function isSameOriginRelative(arg) {