From 5b8fda296d7091b21ba800f07ea3a9d6c7d93fa0 Mon Sep 17 00:00:00 2001 From: Dave Jong Date: Thu, 13 Aug 2026 16:48:37 +0200 Subject: [PATCH 1/4] map: require an attributable receiver before a sink can auto-generate a rule MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three follow-ups from an external review of the map, all reproduced first. 1. Member-call sinks had no justification requirement (the important one). The bare-call path already demanded that a dangerous NAME resolve to a module plausibly providing that API, but the member-call path only asked "is the receiver a local binding?". A namespace import of a RELATIVE module is neither local nor a package, so: import * as helper from './util'; helper.exec(req.body.cmd); // -> exec sink -> command-injection candidate helper.query(req.body.sql); // -> db sink -> sql-injection candidate helper.readFileSync(req.body.p) // -> fs sink -> path-traversal candidate produced three precise, auto-generatable candidates for ordinary app code. `baseOf` now carries the resolved specifier, so a relative receiver is a positive fact (app code) rather than a missing package, and fs/exec member calls must resolve to a filesystem/process package the same way bare calls must. Recall is preserved rather than traded away: `sinksFrom` follows a relative namespace receiver into its module, so a helper that really does reach a sink still reports it, attributed to the file it lives in. Receivers that cannot be traced at all (`res.locals.db.query(x)`) stay in the INVENTORY but carry no attribution, and flows refuse to generate a rule for them with that reason. Any object can own a method called `query`; a coordinate pinned on that guess is a rule that blocks real traffic for no reason. 2. Sink ids were not unique map-wide, despite the schema promising exactly that. The hash covered the span but not the owning file, so duplicated route boilerplate in two files put the same call at the same offsets and the two sinks shared an id. The endpoint's repo-relative file is now part of the identity (relative, never absolute, so ids stay stable across machines). 3. Namespace capture only worked in the parameter list. `const { query: q } = req` — the same capture one statement later — left the fields read off `q` invisible. Nothing was mis-addressed (no coordinate was emitted), but an unreported surface reads as "nothing here", which is the more misleading failure. Those inputs are now reported; their flows remain heuristic, so they are visible without being auto-ruled. Making alias flows precise is a separate change. The golden corpus and every existing candidate assertion pass unchanged, which is the point: this removes false candidates without removing true ones. 860 tests. Co-Authored-By: Claude Opus 4.8 --- src/map/entries.ts | 6 +- src/map/extract.ts | 4 +- src/map/flows.ts | 3 + src/map/inputs.ts | 9 ++ src/map/sinks.ts | 128 ++++++++++++++++----- src/map/types.ts | 9 ++ tests/map/aliased-namespace.test.ts | 19 +++ tests/map/sink-attribution-members.test.ts | 126 ++++++++++++++++++++ 8 files changed, 268 insertions(+), 36 deletions(-) create mode 100644 tests/map/sink-attribution-members.test.ts diff --git a/src/map/entries.ts b/src/map/entries.ts index 376633d..22ef141 100644 --- a/src/map/entries.ts +++ b/src/map/entries.ts @@ -4,13 +4,13 @@ import type { Bindings } from './bindings.js'; import { functionNameFromPath, ROUTE_REGISTER, routeFromChain, routeObject } from './routes.js'; import { withCoordinates } from './coordinates.js'; import { inputsFromHandler, inputsFromValidator } from './inputs.js'; -import { sinksFrom, type ModuleGraph } from './sinks.js'; +import { sinksFrom, type SinkContext } from './sinks.js'; import { linkedFlows } from './flows.js'; const HTTP_METHODS = new Set(['GET', 'POST', 'PUT', 'PATCH', 'DELETE', 'HEAD', 'OPTIONS']); // --- entry-point recognizers ----------------------------------------------- -export function extractFromFile(sf: any, ts: TsModule, localSinks: Map, bindings: Bindings, ctx: { file: string; graph: ModuleGraph }): Omit[] { +export function extractFromFile(sf: any, ts: TsModule, localSinks: Map, bindings: Bindings, ctx: SinkContext): Omit[] { const out: Omit[] = []; const isServerActionsFile = fileHasUseServer(sf, ts); @@ -134,7 +134,7 @@ function handlerEntry( ts: TsModule, localSinks: Map, bindings: Bindings, - ctx: { file: string; graph: ModuleGraph }, + ctx: SinkContext, extra: { method?: string; route?: string; line?: number; start?: number; end?: number } = {}, ): Omit { const entryKind = kindLabel === 'route-registration' || kindLabel === 'server-action' || kindLabel === 'edge-function' diff --git a/src/map/extract.ts b/src/map/extract.ts index 0e4611c..a6731a1 100644 --- a/src/map/extract.ts +++ b/src/map/extract.ts @@ -50,8 +50,8 @@ export async function extractInputMap(cwd: string, ts: TsModule, options: Extrac const sf = ts.createSourceFile(file, text, ts.ScriptTarget.Latest, true, guessScriptKind(ts, file)); const bindings = buildModuleBindings(sf, ts); const localSinks = collectLocalSinks(sf, ts, bindings); - for (const ep of extractFromFile(sf, ts, localSinks, bindings, { file, graph })) { - const relFile = relative(cwd, file); + const relFile = relative(cwd, file); + for (const ep of extractFromFile(sf, ts, localSinks, bindings, { file, owner: relFile, graph })) { // A FILE-BASED route handler carries its URL path in its location, not in the code, so derive // it here — without this a rule can only be param-pinned, never route-scoped (`when.path`). if (ep.route === undefined && ep.entryKind === 'edge-function') { diff --git a/src/map/flows.ts b/src/map/flows.ts index 68d07da..de503f2 100644 --- a/src/map/flows.ts +++ b/src/map/flows.ts @@ -155,6 +155,9 @@ function linkFlows( if (!input.runtimeParameter) reasons.push(input.runtimeParameterReason ?? 'input has no runtime parameter'); if (sink.file !== undefined) reasons.push('sink is in an imported module: no local call-site evidence'); if (sink.start === undefined) reasons.push('sink call could not be located in the source'); + if (sink.attribution === undefined) { + reasons.push(`sink receiver could not be traced to a dependency (${sink.kind}.${sink.op ?? '?'} on an unresolved receiver): a rule here would be a guess`); + } if (precise && argumentRole === 'unknown') reasons.push(`sink argument role is not modelled for ${sink.kind}.${sink.op ?? '?'}`); // A dynamic key or a spread in this sink's arguments means no coordinate can name the field that // actually reaches it — report the specific cause rather than a generic "heuristic". diff --git a/src/map/inputs.ts b/src/map/inputs.ts index b8e5ef8..e77684f 100644 --- a/src/map/inputs.ts +++ b/src/map/inputs.ts @@ -163,6 +163,15 @@ function requestMemberAccesses( const init = unwrap(n.initializer); // const b = await request.json() → b is a request-input object from here on. if (ts.isIdentifier(n.name) && isBodyReadCall(n.initializer)) sourceNames.set(n.name.text, bodyReadSource(n.initializer)); + // const { query: q } = req → the SAME namespace capture as a destructured handler param, just one + // statement later. Without this the fields read off `q` are invisible: no coordinate is emitted + // (so nothing is mis-addressed) but the surface goes unreported, which reads as "nothing here". + if (ts.isObjectBindingPattern(n.name) && ts.isIdentifier(init) && reqName && init.text === reqName) { + for (const el of n.name.elements) { + const key = bindingKey(el, ts); + if (key && REQ_SOURCES.includes(key) && ts.isIdentifier(el.name)) sourceNames.set(el.name.text, namespaceSource(key)); + } + } // const { a, b } = | await request.json() if (ts.isObjectBindingPattern(n.name) && (isReqSourceExpr(init) || isBodyReadCall(n.initializer))) { const src = isBodyReadCall(n.initializer) ? bodyReadSource(n.initializer) : sourceOfExpr(init); diff --git a/src/map/sinks.ts b/src/map/sinks.ts index f5bf9ef..f09ec45 100644 --- a/src/map/sinks.ts +++ b/src/map/sinks.ts @@ -22,12 +22,26 @@ const HTTP_MEMBER_METHODS = new Set(['get', 'post', 'put', 'patch', 'delete', 'h const DB_PACKAGES = ['@supabase/supabase-js', '@prisma/client', 'drizzle-orm', 'knex', 'kysely', 'pg', 'mysql2', 'mysql', 'sequelize', 'typeorm', 'mongoose', 'better-sqlite3']; const HTTP_PACKAGES = ['axios', 'got', 'node-fetch', 'undici', 'superagent', 'ky']; const isHttpPackage = (pkg: string) => HTTP_PACKAGES.includes(pkg) || pkg === 'node:http' || pkg === 'node:https'; +// A filesystem/process API is not only a node: builtin — these wrappers expose the same sinks, and +// requiring a match keeps recognition tied to a RESOLVED import rather than to a method name. +const FS_PACKAGES = ['fs-extra', 'graceful-fs', 'memfs']; +const isFsPackage = (pkg: string) => /^node:fs(\/promises)?$/.test(pkg) || FS_PACKAGES.includes(pkg); +const EXEC_PACKAGES = ['execa', 'cross-spawn', 'shelljs', 'zx']; +const isExecPackage = (pkg: string) => pkg === 'node:child_process' || EXEC_PACKAGES.includes(pkg); export interface ModuleGraph { /** Sinks of `exportName` in the module `specifier` resolves to, relative to `fromFile`. */ importedSinks(fromFile: string, specifier: string, exportName: string): Sink[]; } +export interface SinkContext { + /** Absolute path of the file being analyzed (for resolving relative imports). */ + file: string; + /** The same file, REPO-RELATIVE — part of a sink's identity, so it must not vary by machine. */ + owner: string; + graph: ModuleGraph; +} + // --- sinks (agnostic) ------------------------------------------------------- export function collectLocalSinks(sf: any, ts: TsModule, bindings: Bindings): Map { const map = new Map(); @@ -46,7 +60,7 @@ export function collectLocalSinks(sf: any, ts: TsModule, bindings: Bindings): Ma return map; } -export function sinksFrom(arrowOrNode: any, ts: TsModule, localSinks: Map, bindings: Bindings, ctx?: { file: string; graph: ModuleGraph }): Sink[] { +export function sinksFrom(arrowOrNode: any, ts: TsModule, localSinks: Map, bindings: Bindings, ctx?: SinkContext): Sink[] { if (!arrowOrNode) return []; const body = arrowOrNode.isSyntheticBody ? arrowOrNode.body : isFnLike(arrowOrNode, ts) ? arrowOrNode.body : arrowOrNode; @@ -63,7 +77,32 @@ export function sinksFrom(arrowOrNode: any, ts: TsModule, localSinks: Map { + const out: Array<[string, string]> = []; + const visit = (n: any) => { + if (ts.isCallExpression(n) && ts.isPropertyAccessExpression(n.expression) && ts.isIdentifier(n.expression.expression)) { + out.push([n.expression.expression.text, n.expression.name.text]); + } + ts.forEachChild(n, visit); + }; + visit(node); + return out; } // Provider-agnostic sink recognizers over a subtree. Each sink is tagged with the npm package behind @@ -72,13 +111,24 @@ export function sinksFrom(arrowOrNode: any, ts: TsModule, localSinks: Map { + // `spec` is the raw module specifier the receiver came from, which `pkg` cannot express: a RELATIVE + // specifier yields no package, and that is a positive fact (the receiver is app code) rather than the + // absence of one (an untraceable receiver such as a handler param). The member-call recognizers below + // need that distinction — `import * as helper from './util'; helper.exec(x)` is not child_process. + const baseOf = (base: any): { pkg?: string; local?: boolean; root?: string; spec?: string; relative?: boolean } => { const root = base ? rootIdentifier(base, ts) : undefined; if (!root) return {}; - const pkg = npmPackageOf(bindings.resolve(root)); - if (pkg) return { pkg, root }; - return { local: bindings.locals.has(root), root }; + const spec = bindings.resolve(root); + const relative = spec !== undefined && (spec.startsWith('.') || spec.startsWith('/')); + const pkg = npmPackageOf(spec); + if (pkg) return { pkg, root, spec }; + return { local: bindings.locals.has(root), root, spec, relative }; }; + // Whether `package` is evidence or a guess. A resolved import binding is evidence ('import'); a + // package inferred from the file's OTHER imports is a guess that is usually right ('inferred'); an + // untraceable receiver is neither (undefined) and must not drive an auto-generated rule. + const attributionOf = (b: { pkg?: string }, pkg: string | undefined): Sink['attribution'] => + b.pkg ? 'import' : pkg ? 'inferred' : undefined; const infer = (kind: 'db' | 'http'): string | undefined => { const table = kind === 'db' ? DB_PACKAGES : HTTP_PACKAGES; for (const p of table) if (bindings.imports.has(p)) return p; @@ -100,35 +150,48 @@ function directSinks(node: any, ts: TsModule, bindings: Bindings): Sink[] { const t = n.arguments[0]; const table = t && ts.isStringLiteralLike(t) ? t.text : undefined; const parent = n.parent; - if (!b.local && parent && ts.isPropertyAccessExpression(parent) && DB_OPS.has(parent.name.text)) { - push({ kind: 'db', provider: 'sql', package: b.pkg ?? infer('db'), table, op: parent.name.text, ...spanOf(opCallOf(parent, ts)) }); + if (!b.local && !b.relative && parent && ts.isPropertyAccessExpression(parent) && DB_OPS.has(parent.name.text)) { + const pkg = b.pkg ?? infer('db'); + push({ kind: 'db', provider: 'sql', package: pkg, table, op: parent.name.text, attribution: attributionOf(b, pkg), ...spanOf(opCallOf(parent, ts)) }); } } if (ts.isPropertyAccessExpression(callee)) { const method = callee.name.text; const b = baseOf(callee.expression); - if (!b.local) { + // `!b.relative` is the member-call twin of the bare-call justification below: a receiver that + // resolves to a RELATIVE module is app code, whatever its methods are named. Without it, + // `import * as helper from './util'; helper.exec(req.body.cmd)` was read as child_process and + // produced a precise, auto-generatable command-injection candidate for harmless local code. + // Such a receiver is not dropped outright — `sinksFrom` follows it into its module instead. + if (!b.local && !b.relative) { // db: prisma-style `prisma..()` — the op names are generic (`delete`, `update`, …), // so require a real prisma signal: a resolved binding, the import, or a prisma-named receiver. if (PRISMA_OPS.has(method) && ts.isPropertyAccessExpression(callee.expression)) { const prismaLikely = b.pkg === '@prisma/client' || (!b.pkg && (bindings.imports.has('@prisma/client') || /prisma/i.test(b.root ?? ''))); - if (prismaLikely) push({ kind: 'db', provider: 'prisma', package: '@prisma/client', table: callee.expression.name.text, op: method, ...spanOf(n) }); + if (prismaLikely) push({ kind: 'db', provider: 'prisma', package: '@prisma/client', table: callee.expression.name.text, op: method, attribution: b.pkg ? 'import' : 'inferred', ...spanOf(n) }); } - // db: raw `.query(` / `.execute(` + // db: raw `.query(` / `.execute(`. Any object can have a `.query` method, so an untraceable + // receiver stays in the inventory with NO attribution — visible to a human, never auto-ruled. if (method === 'query' || method === 'execute') { - push({ kind: 'db', provider: 'sql', package: b.pkg ?? infer('db'), op: method, ...spanOf(n) }); + const pkg = b.pkg ?? infer('db'); + push({ kind: 'db', provider: 'sql', package: pkg, op: method, attribution: attributionOf(b, pkg), ...spanOf(n) }); + } + // fs / exec via a namespace: `fs.writeFile(` / `child_process.exec(`. The receiver must + // actually resolve to a filesystem/process package — the method name alone proves nothing. + if (FS_CALLS.test(method) && b.pkg && isFsPackage(b.pkg)) { + push({ kind: 'fs', package: b.pkg, op: method, attribution: 'import', ...spanOf(n) }); + } + if (EXEC_CALLS.test(method) && b.pkg && isExecPackage(b.pkg)) { + push({ kind: 'exec', package: b.pkg, op: method, attribution: 'import', ...spanOf(n) }); } - // fs / exec via a namespace: `fs.writeFile(` / `child_process.exec(` - if (FS_CALLS.test(method)) push({ kind: 'fs', package: b.pkg, op: method, ...spanOf(n) }); - if (EXEC_CALLS.test(method)) push({ kind: 'exec', package: b.pkg, op: method, ...spanOf(n) }); // http: any client whose binding resolves to a known http package (`axios.get`, `ky.post`, // `http.request`), else the classic identifiers by name as a heuristic. if (HTTP_MEMBER_METHODS.has(method)) { if (b.pkg && isHttpPackage(b.pkg)) { - push({ kind: 'http', provider: b.root, package: b.pkg, op: method, ...spanOf(n) }); - } else if (!b.pkg && ts.isIdentifier(callee.expression) && /^(axios|http|https|got|ky)$/.test(callee.expression.text)) { - push({ kind: 'http', provider: callee.expression.text, package: infer('http'), op: method, ...spanOf(n) }); + push({ kind: 'http', provider: b.root, package: b.pkg, op: method, attribution: 'import', ...spanOf(n) }); + } else if (!b.spec && ts.isIdentifier(callee.expression) && /^(axios|http|https|got|ky)$/.test(callee.expression.text)) { + push({ kind: 'http', provider: callee.expression.text, package: infer('http'), op: method, attribution: 'inferred', ...spanOf(n) }); } } } @@ -148,22 +211,22 @@ function directSinks(node: any, ts: TsModule, bindings: Bindings): Sink[] { const trueGlobal = !fromModule && !shadowed; if (HTTP_CALLS.test(name)) { - if (pkg && isHttpPackage(pkg)) push({ kind: 'http', provider: name, package: pkg, op: 'request', ...spanOf(n) }); - else if (name === 'fetch' && trueGlobal) push({ kind: 'http', provider: 'fetch', op: 'request', ...spanOf(n) }); + if (pkg && isHttpPackage(pkg)) push({ kind: 'http', provider: name, package: pkg, op: 'request', attribution: 'import', ...spanOf(n) }); + else if (name === 'fetch' && trueGlobal) push({ kind: 'http', provider: 'fetch', op: 'request', attribution: 'global', ...spanOf(n) }); } // `readFile`/`exec` are never globals: without a matching module binding this is app code. - if (FS_CALLS.test(name) && pkg && /^node:fs(\/promises)?$/.test(pkg)) { - push({ kind: 'fs', package: pkg, op: name, ...spanOf(n) }); + if (FS_CALLS.test(name) && pkg && isFsPackage(pkg)) { + push({ kind: 'fs', package: pkg, op: name, attribution: 'import', ...spanOf(n) }); } - if (EXEC_CALLS.test(name) && pkg === 'node:child_process') { - push({ kind: 'exec', package: pkg, op: name, ...spanOf(n) }); + if (EXEC_CALLS.test(name) && pkg && isExecPackage(pkg)) { + push({ kind: 'exec', package: pkg, op: name, attribution: 'import', ...spanOf(n) }); } - if (name === 'eval' && trueGlobal) push({ kind: 'eval', op: 'eval', ...spanOf(n) }); + if (name === 'eval' && trueGlobal) push({ kind: 'eval', op: 'eval', attribution: 'global', ...spanOf(n) }); } } if (ts.isNewExpression(n) && ts.isIdentifier(n.expression) && n.expression.text === 'Function' && !bindings.locals.has('Function') && !isShadowedByEnclosingBinding(n, 'Function', ts)) { - push({ kind: 'eval', op: 'new Function', ...spanOf(n) }); + push({ kind: 'eval', op: 'new Function', attribution: 'global', ...spanOf(n) }); } ts.forEachChild(n, visit); }; @@ -228,20 +291,23 @@ export function argumentRoleOf(sinkKind: string, method: string | undefined, ind } // Deterministic identity for a sink, so `Flow.sink` (an embedded copy) can be correlated back to the -// inventory entry without deep-equality. -function sinkId(s: Sink): string { +// inventory entry without deep-equality. `owner` is the endpoint's own repo-relative file, which the +// span alone does NOT imply: duplicated route boilerplate across two files puts the same call at the +// same offsets, and hashing only the span made those sinks share an id while the schema promises +// map-wide identity. A repo-relative path (never absolute) keeps the id stable across machines. +function sinkId(s: Sink, owner: string): string { return createHash('sha256') - .update([s.kind, s.provider, s.package, s.table, s.op, s.file, s.start, s.end].join('|')) + .update([s.kind, s.provider, s.package, s.table, s.op, s.file ?? owner, s.start, s.end].join('|')) .digest('hex') .slice(0, 12); } -function dedupeSinks(sinks: Sink[]): Sink[] { +function dedupeSinks(sinks: Sink[], owner: string): Sink[] { const seen = new Set(); const out: Sink[] = []; for (const s of sinks) { const key = `${s.kind}:${s.provider}:${s.package}:${s.table}:${s.op}:${s.line}:${s.start}`; - if (!seen.has(key)) { seen.add(key); out.push({ ...s, id: sinkId(s) }); } + if (!seen.has(key)) { seen.add(key); out.push({ ...s, id: sinkId(s, owner) }); } } return out; } diff --git a/src/map/types.ts b/src/map/types.ts index 6d82764..c33e940 100644 --- a/src/map/types.ts +++ b/src/map/types.ts @@ -67,6 +67,15 @@ export interface Sink { * — i.e. it does not live in the endpoint's file. Without this, `line` would point at the wrong file. */ file?: string; + /** + * How `package` was established — the difference between evidence and a guess. `import`: the + * receiver resolves to that dependency through this file's imports. `global`: a genuine runtime + * global (`fetch`, `eval`, `Function`). `inferred`: the receiver could not be resolved, so the + * package was taken from another import in the file. **Absent**: the receiver could not be + * attributed at all (e.g. `ctx.db.query(x)`) — the sink is reported for human review but must + * never drive an auto-generated rule, since any object can own a method by that name. + */ + attribution?: 'import' | 'global' | 'inferred'; /** * Character span of the sink's operation call in `file` (or the endpoint's file). This is the sink's * IDENTITY: flow analysis binds evidence to this exact call, never to a line or an enclosing diff --git a/tests/map/aliased-namespace.test.ts b/tests/map/aliased-namespace.test.ts index 3f89eef..d83a2df 100644 --- a/tests/map/aliased-namespace.test.ts +++ b/tests/map/aliased-namespace.test.ts @@ -24,6 +24,9 @@ beforeAll(() => { app.get("/param/:id", ({ params: p }, res) => { res.end(fs.readFileSync(p.id)); }); app.post("/bodyalias", ({ body: b }, res) => { res.end(fs.readFileSync(b.file)); }); app.post("/nested", ({ query: q }, res) => { const { doc } = q; res.end(fs.readFileSync(doc)); }); + // The same namespace capture, one statement later instead of in the parameter list. + app.get("/fromreq", (req, res) => { const { query: q } = req; res.end(fs.readFileSync(q.doc)); }); + app.get("/fromreq/:id", (req, res) => { const { params: p } = req; res.end(fs.readFileSync(p.id)); }); `); }); afterAll(() => rmSync(dir, { recursive: true, force: true })); @@ -63,11 +66,27 @@ describe('aliased request namespaces', () => { expect(field).toMatchObject({ source: 'query', runtimeParameter: 'get.doc' }); }); + it('captures the namespace when destructured from the request identifier itself', async () => { + const { field } = await input('/fromreq', 'doc'); + // Previously invisible: no coordinate was mis-addressed, but the surface went unreported, which + // reads as "nothing here" rather than "something we cannot address". + expect(field).toMatchObject({ source: 'query', runtimeParameter: 'get.doc' }); + }); + + it('still refuses a coordinate for a route param destructured that way', async () => { + const { field } = await input('/fromreq/:id', 'id'); + expect(field.source).toBe('route-param'); + expect(field.runtimeParameter).toBeNull(); + }); + it('compiles candidates for the addressable ones only', async () => { const { map } = await buildInputMap(dir); const got = map!.endpoints .flatMap((e) => e.flows.filter((f) => f.ruleGeneratable).map((f) => `${e.route}:${f.input}`)) .sort(); + // `/fromreq` is absent by design: the input is now VISIBLE, but its flow evidence is heuristic + // (the alias is not yet tracked in the taint paths), so it must not compile a rule. Visible and + // non-generatable is the safe half of this fix; making such flows precise is a separate change. expect(got).toEqual(['/bodyalias:file', '/nested:doc', '/plain:doc', '/renamed:doc']); }); }); diff --git a/tests/map/sink-attribution-members.test.ts b/tests/map/sink-attribution-members.test.ts new file mode 100644 index 0000000..c835796 --- /dev/null +++ b/tests/map/sink-attribution-members.test.ts @@ -0,0 +1,126 @@ +import { describe, it, expect, beforeAll, afterAll } from 'vitest'; +import { mkdtempSync, mkdirSync, writeFileSync, rmSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import { buildInputMap } from '../../src/map/index.js'; + +// The MEMBER-call companion to sink-attribution.test.ts (which covers bare calls). +// A dangerous METHOD NAME is not a dangerous API. `import * as helper from './util'` gives a receiver +// that is neither a local binding nor a package, and admitting it produced precise, auto-generatable +// command-injection / SQLi / path-traversal candidates for ordinary app code. The bare-call path already +// required justification; the member-call path did not — the same bug, one syntax over. +let dir: string; +beforeAll(() => { + dir = mkdtempSync(join(tmpdir(), 'ps-attr-')); + mkdirSync(join(dir, 'src'), { recursive: true }); + writeFileSync(join(dir, 'package.json'), JSON.stringify({ dependencies: { express: '4' } })); + // App code whose exported names collide with dangerous APIs. + writeFileSync(join(dir, 'src', 'util.ts'), ` + export function exec(x: string) { return x.length } + export function query(x: string) { return x } + export function readFileSync(p: string) { return p } + `); + // A relative namespace helper that DOES reach a real sink — recall must survive the fix. + writeFileSync(join(dir, 'src', 'store.ts'), ` + import fs from "node:fs"; + export function save(p: string) { return fs.writeFileSync(p, "x") } + `); + writeFileSync(join(dir, 'src', 'app.ts'), ` + import * as helper from "./util"; + import * as store from "./store"; + import express from "express"; + const app = express(); + app.post("/lookalike", (req, res) => { + helper.exec(req.body.cmd); + helper.query(req.body.sql); + helper.readFileSync(req.body.path); + res.end("ok"); + }); + app.post("/viaHelper", (req, res) => { store.save(req.body.p); res.end("ok"); }); + app.post("/untraceable", (req, res) => { res.locals.db.query(req.body.sql); res.end("ok"); }); + `); + writeFileSync(join(dir, 'src', 'real.ts'), ` + import fs from "node:fs"; + import { exec } from "node:child_process"; + import express from "express"; + const app = express(); + app.post("/real", (req, res) => { + fs.readFileSync(req.body.path); + exec(req.body.cmd); + res.end("ok"); + }); + `); +}); +afterAll(() => rmSync(dir, { recursive: true, force: true })); + +const ep = async (route: string) => { + const { map } = await buildInputMap(dir); + return map!.endpoints.find((e) => e.route === route)!; +}; + +describe('member-call sinks require an attributable receiver', () => { + it('does not treat a relative namespace helper as fs/exec/db', async () => { + const e = await ep('/lookalike'); + expect(e.sinks).toEqual([]); + }); + + it('generates no candidate for it — the false-blocking-rule case', async () => { + const e = await ep('/lookalike'); + // The inputs are still reported: the surface is real, the SINK was not. + expect(e.inputs.map((i) => i.name).sort()).toEqual(['cmd', 'path', 'sql']); + expect(e.flows.filter((f) => f.ruleGeneratable)).toEqual([]); + }); + + it('still follows that receiver into its module, so a real sink is not lost', async () => { + const e = await ep('/viaHelper'); + const fsSink = e.sinks.find((s) => s.kind === 'fs'); + expect(fsSink).toBeDefined(); + expect(fsSink!.package).toBe('node:fs'); + expect(fsSink!.file).toBe('src/store.ts'); // attributed to where it actually lives + }); + + it('keeps an untraceable receiver in the inventory but refuses to auto-rule it', async () => { + const e = await ep('/untraceable'); + const db = e.sinks.find((s) => s.kind === 'db'); + expect(db).toBeDefined(); + expect(db!.attribution).toBeUndefined(); + const flow = e.flows.find((f) => f.sink.kind === 'db')!; + expect(flow.ruleGeneratable).toBe(false); + expect(flow.ruleGeneratableReasons!.join(' ')).toMatch(/could not be traced to a dependency/); + }); + + it('leaves genuinely imported sinks fully generatable', async () => { + const e = await ep('/real'); + expect(e.sinks.map((s) => `${s.kind}:${s.attribution}`).sort()).toEqual(['exec:import', 'fs:import']); + const gen = e.flows.filter((f) => f.ruleGeneratable).map((f) => f.candidateFamily).sort(); + expect(gen).toEqual(['command-injection', 'path-traversal']); + }); +}); + +describe('sink identity is map-wide', () => { + it('does not collide across two files with identical boilerplate at identical offsets', async () => { + const d = mkdtempSync(join(tmpdir(), 'ps-dup-')); + mkdirSync(join(d, 'src'), { recursive: true }); + writeFileSync(join(d, 'package.json'), JSON.stringify({ dependencies: { express: '4' } })); + // Byte-identical but for the route string, so every span matches. + const src = (route: string) => ` + import express from "express"; + import fs from "node:fs"; + const app = express(); + app.post("${route}", (req, res) => { res.end(fs.readFileSync(req.body.f)); }); + `; + writeFileSync(join(d, 'src', 'a.ts'), src('/aa')); + writeFileSync(join(d, 'src', 'b.ts'), src('/bb')); + const { map } = await buildInputMap(d); + const ids = map!.endpoints.flatMap((e) => e.sinks.map((s) => s.id)); + expect(ids).toHaveLength(2); + expect(new Set(ids).size).toBe(2); + rmSync(d, { recursive: true, force: true }); + }); + + it('stays deterministic across runs', async () => { + const a = await ep('/real'); + const b = await ep('/real'); + expect(a.sinks.map((s) => s.id)).toEqual(b.sinks.map((s) => s.id)); + }); +}); From dc2a1008ec1cc59bb6ff9af9e481318c95548adf Mon Sep 17 00:00:00 2001 From: Dave Jong Date: Thu, 13 Aug 2026 17:05:51 +0200 Subject: [PATCH 2/4] map: an inferred package is not evidence about the receiver MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up on the same review. I added the 'inferred' attribution tier and then only refused generation for a MISSING one, which left the exact hole this change set claims to close: import { Pool } from 'pg'; res.locals.db.query(req.body.sql); // package: "pg", attribution: "inferred" // -> precise, rule-generatable sql-injection candidate The package came from another import in the FILE, not from the receiver, so an untraceable `res.locals.db` looked identical to a real pool. Generation now requires attribution 'import' (the receiver resolves to that dependency) or 'global' (a genuine runtime global). Inferred sinks keep their package as a hint for a human reviewer and stay in the inventory; they cannot compile a rule. The refusal names which of the two cases applies, since "inferred from the file's other imports" and "receiver untraceable" ask a reviewer to check different things. Fixtures for the raw `.query()`, `.from().insert()` and prisma-shaped paths, plus controls where the receiver really does resolve. The `.from().insert()` fixture asserts the ATTRIBUTION reason specifically: that shape is also refused for its argument role, so a laxer assertion would pass for the wrong reason and regress silently. One detail the fixtures pin down: inference picks the first known db package the file imports, in table order — for a file importing supabase, pg and prisma, a `.query()` call is labelled supabase. Fine as a hint, unusable as an address. Full suite passes unchanged, so refusing inferred receivers costs no true candidate. 864 tests. Co-Authored-By: Claude Opus 4.8 --- src/map/flows.ts | 11 ++- tests/map/sink-attribution-members.test.ts | 87 ++++++++++++++++++++++ 2 files changed, 96 insertions(+), 2 deletions(-) diff --git a/src/map/flows.ts b/src/map/flows.ts index de503f2..3fbaaba 100644 --- a/src/map/flows.ts +++ b/src/map/flows.ts @@ -155,8 +155,15 @@ function linkFlows( if (!input.runtimeParameter) reasons.push(input.runtimeParameterReason ?? 'input has no runtime parameter'); if (sink.file !== undefined) reasons.push('sink is in an imported module: no local call-site evidence'); if (sink.start === undefined) reasons.push('sink call could not be located in the source'); - if (sink.attribution === undefined) { - reasons.push(`sink receiver could not be traced to a dependency (${sink.kind}.${sink.op ?? '?'} on an unresolved receiver): a rule here would be a guess`); + // Only a receiver traced to a dependency ('import') or a genuine runtime global earns a rule. + // 'inferred' is deliberately NOT enough: the package came from some OTHER import in the file, not + // from the receiver, so `res.locals.db.query(x)` in a file that happens to import `pg` looks + // identical to a real pool — and `res.locals.db` may be any app object. Such sinks stay in the + // inventory for review; they just cannot compile a rule that blocks live traffic on a guess. + if (sink.attribution !== 'import' && sink.attribution !== 'global') { + reasons.push(sink.attribution === 'inferred' + ? `sink package "${sink.package}" was inferred from the file's other imports, not from the receiver (${sink.kind}.${sink.op ?? '?'}): the receiver may be any app object` + : `sink receiver could not be traced to a dependency (${sink.kind}.${sink.op ?? '?'} on an unresolved receiver): a rule here would be a guess`); } if (precise && argumentRole === 'unknown') reasons.push(`sink argument role is not modelled for ${sink.kind}.${sink.op ?? '?'}`); // A dynamic key or a spread in this sink's arguments means no coordinate can name the field that diff --git a/tests/map/sink-attribution-members.test.ts b/tests/map/sink-attribution-members.test.ts index c835796..b6dd071 100644 --- a/tests/map/sink-attribution-members.test.ts +++ b/tests/map/sink-attribution-members.test.ts @@ -124,3 +124,90 @@ describe('sink identity is map-wide', () => { expect(a.sinks.map((s) => s.id)).toEqual(b.sinks.map((s) => s.id)); }); }); + +// An INFERRED package is not evidence about the receiver. `res.locals.db.query(x)` in a file that +// happens to import `pg` was getting package "pg" and a precise SQL-injection candidate — the exact +// guarantee this file's first half claims to enforce, defeated by the fallback that fills in a package +// from the file's OTHER imports. Inferred sinks stay in the inventory; they never compile a rule. +describe('an inferred package does not license a rule', () => { + let d: string; + beforeAll(() => { + d = mkdtempSync(join(tmpdir(), 'ps-infer-')); + mkdirSync(join(d, 'src'), { recursive: true }); + writeFileSync(join(d, 'package.json'), JSON.stringify({ + dependencies: { express: '4', pg: '8', '@supabase/supabase-js': '2', '@prisma/client': '5' }, + })); + writeFileSync(join(d, 'src', 'inferred.ts'), ` + import { Pool } from "pg"; + import { createClient } from "@supabase/supabase-js"; + import { PrismaClient } from "@prisma/client"; + import express from "express"; + const app = express(); + // Every receiver here is an app object the analyzer cannot trace. + app.post("/raw", (req, res) => { res.locals.db.query(req.body.sql); res.end("ok"); }); + app.post("/from", (req, res) => { res.locals.sb.from("t").insert({ v: req.body.v }); res.end("ok"); }); + app.post("/prisma", (req, res) => { res.locals.prisma.user.update({ where: { id: req.body.id } }); res.end("ok"); }); + `); + // Controls: receivers that really do resolve to the dependency. + writeFileSync(join(d, 'src', 'resolved.ts'), ` + import { Pool } from "pg"; + import { createClient } from "@supabase/supabase-js"; + import express from "express"; + const pool = new Pool(); + const sb = createClient("u", "k"); + const app = express(); + app.post("/pool", (req, res) => { pool.query(req.body.sql); res.end("ok"); }); + app.post("/sb", (req, res) => { sb.from("t").insert({ v: req.body.v }); res.end("ok"); }); + `); + }); + afterAll(() => rmSync(d, { recursive: true, force: true })); + + const route = async (r: string) => { + const { map } = await buildInputMap(d); + return map!.endpoints.find((e) => e.route === r)!; + }; + + it('refuses a rule for an untraceable receiver even when the file imports a db package', async () => { + const e = await route('/raw'); + const sink = e.sinks.find((s) => s.kind === 'db')!; + // Note WHICH package inference picks: the first known db package the file imports, in table order — + // here supabase, not the `pg` a reader would guess from `.query()`. A useful hint for a human + // reviewer, and a good illustration of why it must not address a rule. + expect(sink.package).toBe('@supabase/supabase-js'); + expect(sink.attribution).toBe('inferred'); + const flow = e.flows.find((f) => f.sink.kind === 'db')!; + expect(flow.confidence).toBe('precise'); // the data really does reach it + expect(flow.ruleGeneratable).toBe(false); // and it still must not be auto-ruled + expect(flow.ruleGeneratableReasons!.join(' ')).toMatch(/inferred from the file's other imports/); + }); + + it('refuses the same for an untraceable .from().insert() receiver', async () => { + const e = await route('/from'); + const sink = e.sinks.find((s) => s.kind === 'db')!; + expect(sink.attribution).toBe('inferred'); + const flow = e.flows.find((f) => f.sink.kind === 'db')!; + expect(flow.ruleGeneratable).toBe(false); + // Assert the ATTRIBUTION reason specifically: this shape is also refused for its argument role + // ("values"), so without this the fixture would pass for the wrong reason and regress silently. + expect(flow.ruleGeneratableReasons!.join(' ')).toMatch(/inferred from the file's other imports/); + }); + + it('refuses the prisma-shaped path on an untraceable receiver', async () => { + const e = await route('/prisma'); + const sink = e.sinks.find((s) => s.provider === 'prisma'); + expect(sink?.attribution).toBe('inferred'); + expect(e.flows.filter((f) => f.ruleGeneratable)).toEqual([]); + }); + + it('still generates for receivers that genuinely resolve to the dependency', async () => { + const pool = await route('/pool'); + const sink = pool.sinks.find((s) => s.kind === 'db')!; + expect(sink.attribution).toBe('import'); + const flow = pool.flows.find((f) => f.sink.kind === 'db')!; + expect(flow.ruleGeneratable).toBe(true); + expect(flow.candidateFamily).toBe('sql-injection'); + // And the resolved supabase receiver keeps its (separate, role-based) refusal — unchanged. + const sb = await route('/sb'); + expect(sb.sinks.find((s) => s.kind === 'db')!.attribution).toBe('import'); + }); +}); From b10048c51b92efbdd81f331237af4fe35693fb88 Mon Sep 17 00:00:00 2001 From: Dave Jong Date: Thu, 13 Aug 2026 17:13:42 +0200 Subject: [PATCH 3/4] map: standing adversarial corpus category (+ fix a wrong pin it found) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Twice now a false-candidate class has reached review rather than being caught here, and both times the reason was the same: the corpus contained the shapes I thought of. It now has a permanent ADVERSARIAL category — app code CONSTRUCTED to look dangerous — alongside the builder-stack cases: - exports whose names collide with dangerous APIs (relative namespace + named imports) - untraceable receivers in a file that really does import pg / supabase - parameters shadowing dangerous globals - one field name read from two request namespaces - sibling expressions that must not contaminate each other Building it immediately turned up a wrong-input pin, which is the failure the corpus metric exists to hold at zero. Inputs are keyed by field NAME, and two namespaces can share one: app.get('/qp/:id', ({ params: p, query: q }, res) => { fs.readFileSync(p.id); // arrives in the path segment fs.readFileSync(q.id); // arrives in the query string }); Last-write-wins picked whichever read the walker saw last, and that pick decided the coordinate — so this handler compiled `path-traversal @ get.id` for data arriving in the path. A rule pinned there inspects the wrong place: it never fires on the real payload, while looking like coverage. Reversing the two lines changed the verdict, which is the tell that no verdict was earned. Collisions are now recorded during collection (first-seen source wins, so the record is at least deterministic) and such an input gets NO coordinate, with a reason naming both namespaces. Refusing costs the legitimate `get.id` candidate; a wrong pin costs trust in every candidate. Two properties keep the new category honest: - each adversarial case must detect a real surface (endpoints + inputs) before its zero-candidate assertion counts — otherwise a parser bug or a typo'd fixture would read as a security property; - the category cannot quietly empty out: the five classes are asserted by name. Verified the fixture fails without the fix (`unexpected candidate(s): path-traversal @ get.id`). Corpus: 6 stack + 5 adversarial projects. 881 tests. Co-Authored-By: Claude Opus 4.8 --- src/map/inputs.ts | 38 ++++++++-- tests/map/corpus.test.ts | 159 +++++++++++++++++++++++++++++++++++++-- 2 files changed, 186 insertions(+), 11 deletions(-) diff --git a/src/map/inputs.ts b/src/map/inputs.ts index e77684f..c62af59 100644 --- a/src/map/inputs.ts +++ b/src/map/inputs.ts @@ -28,10 +28,18 @@ export function inputsFromHandler( // (a server action), where the schema describes the action's own argument. const fields = withCoordinates(zodObjectFields(body, ts, bindings), opts.validatorSource ?? 'json-body'); const names = new Set(fields.map((f) => f.name)); - for (const { name, source } of requestMemberAccesses(params, body, ts, opts)) { + for (const { name, source, alsoFrom } of requestMemberAccesses(params, body, ts, opts)) { if (!names.has(name)) { names.add(name); - fields.push({ name, source, ...runtimeCoordinate(source, name) }); + // Read from two namespaces: no single rule parameter addresses both, so refuse the coordinate + // rather than guess which read a rule should inspect. + const coord = alsoFrom?.length + ? { + runtimeParameter: null, + runtimeParameterReason: `field is read from more than one request namespace (${[source, ...alsoFrom].join(', ')}): no single parameter addresses it`, + } + : runtimeCoordinate(source, name); + fields.push({ name, source, ...coord }); } } return fields; @@ -117,9 +125,24 @@ function requestMemberAccesses( body: any, ts: TsModule, opts: { payloadParam?: boolean } = {}, -): Array<{ name: string; source: InputSource }> { +): Array<{ name: string; source: InputSource; alsoFrom?: InputSource[] }> { if (!body) return []; + // Keyed by FIELD NAME, which two namespaces can share (`params.id` and `query.id` in one handler). + // Last-write-wins silently picked one, and since the pick decided the coordinate, a handler reading + // both compiled a rule pinned to `get.id` for data that arrives in the path segment — a wrong-input + // pin, the one failure this whole layer exists to prevent. Collisions are now recorded, and the + // first-seen source wins so the record is at least deterministic. const out = new Map(); + const collisions = new Map>(); + const record = (name: string, source: InputSource) => { + const prev = out.get(name); + if (prev === undefined) { out.set(name, source); return; } + if (prev !== source) { + const set = collisions.get(name) ?? new Set([prev]); + set.add(source); + collisions.set(name, set); + } + }; const p0 = params?.[0]; const reqName = p0 && ts.isIdentifier(p0.name) ? p0.name.text : undefined; // Identifiers that ARE a request-input object (destructured `({ body })` param, `await req.json()`), @@ -157,7 +180,7 @@ function requestMemberAccesses( const visit = (n: any) => { // . if (ts.isPropertyAccessExpression(n) && isReqSourceExpr(n.expression)) { - out.set(n.name.text, sourceOfExpr(n.expression)); + record(n.name.text, sourceOfExpr(n.expression)); } if (ts.isVariableDeclaration(n) && n.initializer) { const init = unwrap(n.initializer); @@ -177,14 +200,17 @@ function requestMemberAccesses( const src = isBodyReadCall(n.initializer) ? bodyReadSource(n.initializer) : sourceOfExpr(init); for (const el of n.name.elements) { const key = bindingKey(el, ts); - if (key) out.set(key, src); + if (key) record(key, src); } } } ts.forEachChild(n, visit); }; visit(body); - return [...out].map(([name, source]) => ({ name, source })); + return [...out].map(([name, source]) => { + const also = collisions.get(name); + return also ? { name, source, alsoFrom: [...also].filter((s) => s !== source) } : { name, source }; + }); // `req.body.x` / `req.query.x` / `req.params.x` — the namespace decides the runtime coordinate, and // route params notably have NONE, so this distinction is load-bearing rather than cosmetic. diff --git a/tests/map/corpus.test.ts b/tests/map/corpus.test.ts index c3fdfa8..42f50b3 100644 --- a/tests/map/corpus.test.ts +++ b/tests/map/corpus.test.ts @@ -20,6 +20,14 @@ import type { SiteInputMap } from '../../src/map/types.js'; interface Case { name: string; + /** + * `stack`: a project shape a builder really generates — measures recall and correct pinning. + * `adversarial`: app code CONSTRUCTED to look dangerous. A permanent category, not a bag of + * regressions: every false-candidate class we have found came from code that merely resembled a + * dangerous API, so the corpus has to contain lookalikes on purpose. These cases must produce a + * visible surface (inputs, usually sinks) and still compile no rule. + */ + kind?: 'stack' | 'adversarial'; pkg: Record; files: Record; /** `family @ runtimeParameter` for every flow that SHOULD compile to a candidate. */ @@ -28,6 +36,119 @@ interface Case { expectRefused?: Array<[string, RegExp]>; } +const ADVERSARIAL: Case[] = [ + { + name: 'adversarial: app code whose exports collide with dangerous API names', + kind: 'adversarial', + pkg: { dependencies: { express: '4' } }, + files: { + 'src/util.ts': ` + export function exec(x) { return x.length } + export function query(x) { return x } + export function readFileSync(p) { return p } + export function fetch(u) { return { u } } + `, + 'src/server.ts': ` + import * as helper from "./util"; + import { fetch, exec } from "./util"; + import express from "express"; + const app = express(); + // Namespace member calls AND named imports, both from a relative module. + app.post("/ns", (req, res) => { + helper.exec(req.body.cmd); helper.query(req.body.sql); helper.readFileSync(req.body.path); + res.end(); + }); + app.post("/named", (req, res) => { fetch(req.body.url); exec(req.body.cmd2); res.end(); }); + `, + }, + expectCandidates: [], + }, + { + name: 'adversarial: untraceable receivers in a file that imports real db clients', + kind: 'adversarial', + pkg: { dependencies: { express: '4', pg: '8', '@supabase/supabase-js': '2' } }, + files: { + 'src/server.ts': ` + import { Pool } from "pg"; + import { createClient } from "@supabase/supabase-js"; + import express from "express"; + const app = express(); + // The package is only INFERRED from the file's imports; the receivers are app objects. + app.post("/raw", (req, res) => { res.locals.db.query(req.body.sql); res.end(); }); + app.post("/from", (req, res) => { res.locals.sb.from("t").insert({ v: req.body.v }); res.end(); }); + `, + }, + expectCandidates: [], + expectRefused: [['sql', /inferred from the file's other imports/]], + }, + { + name: 'adversarial: parameters shadowing dangerous globals', + kind: 'adversarial', + pkg: { dependencies: { express: '4' } }, + files: { + 'src/server.ts': ` + import express from "express"; + const app = express(); + app.post("/shadow", (req, res) => { + const send = (fetch) => fetch(req.body.url); + send((u) => ({ u })); + const run = (eval2) => eval2(req.body.code); + run((c) => c); + res.end(); + }); + `, + }, + expectCandidates: [], + }, + { + name: 'adversarial: one field name read from two request namespaces', + kind: 'adversarial', + pkg: { dependencies: { express: '4' } }, + files: { + // `params.id` and `query.id` share a NAME but not an address. Whichever read the walker saw last + // used to decide the coordinate, so this handler compiled a rule pinned to `get.id` for data + // arriving in the path segment. Both orders are covered because that is what made it a bug. + 'src/a.ts': ` + import express from "express"; + import fs from "node:fs"; + const app = express(); + app.get("/qp/:id", ({ params: p, query: q }, res) => { fs.readFileSync(q.id); fs.readFileSync(p.id); res.end(); }); + `, + 'src/b.ts': ` + import express from "express"; + import fs from "node:fs"; + const app = express(); + app.get("/pq/:id", ({ params: p, query: q }, res) => { fs.readFileSync(p.id); fs.readFileSync(q.id); res.end(); }); + `, + }, + expectCandidates: [], + expectRefused: [['id', /more than one request namespace/]], + }, + { + name: 'adversarial: sibling expressions must not contaminate each other', + kind: 'adversarial', + pkg: { dependencies: { express: '4' } }, + files: { + 'src/server.ts': ` + import express from "express"; + import fs from "node:fs"; + import { exec } from "node:child_process"; + const STATIC = "ls -la"; + const app = express(); + // Only ONE pairing is real: path -> readFileSync. \`label\` reaches no sink, and the exec call + // takes no request data at all — an inventory-level "both present" must not become a flow. + app.post("/two", (req, res) => { + const label = req.body.label; + fs.readFileSync(req.body.path); + exec(STATIC); + res.end(label); + }); + `, + }, + expectCandidates: ['path-traversal @ post.path'], + }, +]; + const CASES: Case[] = [ { name: 'lovable / tanstack start + supabase (server fns, validated payload)', @@ -141,11 +262,13 @@ const CASES: Case[] = [ }, ]; +const ALL: Case[] = [...CASES, ...ADVERSARIAL]; + const maps = new Map(); let dirs: string[] = []; beforeAll(async () => { - for (const c of CASES) { + for (const c of ALL) { const d = mkdtempSync(join(tmpdir(), 'ps-corpus-')); dirs.push(d); for (const [rel, body] of Object.entries(c.files)) { @@ -175,7 +298,7 @@ function candidatesOf(map: SiteInputMap): string[] { } describe('golden corpus', () => { - for (const c of CASES) { + for (const c of ALL) { describe(c.name, () => { it('compiles exactly the expected candidates — no wrong-input pins', () => { const got = candidatesOf(maps.get(c.name)!); @@ -208,9 +331,35 @@ describe('golden corpus', () => { }); } + // An adversarial case that found NOTHING would pass its zero-candidate assertion for the wrong + // reason — a parser bug or a bad fixture would read as a security property. Each one has to prove it + // actually saw the handler and the request fields, and only then that it compiled no rule. + it('adversarial cases detect a real surface and still refuse to compile a rule', () => { + for (const c of ADVERSARIAL) { + const map = maps.get(c.name)!; + const inputs = map.endpoints.flatMap((e) => e.inputs); + expect(map.endpoints.length, `${c.name}: no endpoint detected — the fixture proves nothing`).toBeGreaterThan(0); + expect(inputs.length, `${c.name}: no inputs detected — the fixture proves nothing`).toBeGreaterThan(0); + const generatable = map.endpoints.flatMap((e) => e.flows).filter((f) => f.ruleGeneratable); + const declared = new Set(c.expectCandidates); + const coord = new Map(map.endpoints.flatMap((e) => e.inputs).map((i) => [i.name, i.runtimeParameter])); + expect(generatable.filter((f) => !declared.has(`${f.candidateFamily} @ ${coord.get(f.input)}`))).toEqual([]); + } + }); + + it('keeps a standing adversarial category (lookalikes are how every false candidate got in)', () => { + // Guards against the category quietly emptying out; the classes listed are the ones that have + // actually produced false candidates, so losing one should fail loudly. + expect(ADVERSARIAL.length).toBeGreaterThanOrEqual(5); + const names = ADVERSARIAL.map((c) => c.name).join(' | '); + for (const cls of ['collide with dangerous API names', 'untraceable receivers', 'shadowing dangerous globals', 'two request namespaces', 'sibling expressions']) { + expect(names, `missing adversarial class: ${cls}`).toContain(cls); + } + }); + it('reports corpus-wide metrics (the numbers that gate auto-promotion)', () => { let candidates = 0, precise = 0, heuristic = 0, refusedWithReason = 0, noCoordinate = 0; - for (const c of CASES) { + for (const c of ALL) { for (const ep of maps.get(c.name)!.endpoints) { for (const i of ep.inputs) if (!i.runtimeParameter) noCoordinate++; for (const f of ep.flows) { @@ -221,11 +370,11 @@ describe('golden corpus', () => { } } // eslint-disable-next-line no-console - console.log(`corpus: ${CASES.length} projects · ${candidates} candidates · ${precise} precise / ${heuristic} heuristic flows · ${refusedWithReason} refused-with-reason · ${noCoordinate} inputs without a coordinate`); + console.log(`corpus: ${CASES.length} stack + ${ADVERSARIAL.length} adversarial projects · ${candidates} candidates · ${precise} precise / ${heuristic} heuristic flows · ${refusedWithReason} refused-with-reason · ${noCoordinate} inputs without a coordinate`); expect(candidates).toBeGreaterThan(0); // the compiler does something expect(refusedWithReason).toBeGreaterThan(0); // and refuses a lot, explicitly // Every non-candidate must explain itself: silence is what makes a map untrustworthy. - for (const c of CASES) { + for (const c of ALL) { for (const ep of maps.get(c.name)!.endpoints) { for (const f of ep.flows.filter((x) => x.ruleGeneratable === false)) { expect(f.ruleGeneratableReasons?.length, `${c.name}/${f.input}: refused without a reason`).toBeGreaterThan(0); From 8af156ba4fe62aee7be82c6f309a100cec15e7d1 Mon Sep 17 00:00:00 2001 From: Dave Jong Date: Thu, 13 Aug 2026 17:19:53 +0200 Subject: [PATCH 4/4] map: compare namespaces, not source labels, when one field name has two origins MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The collision pass added with the adversarial category only looked at request READS, so the same class survived one layer up — between a validator schema and a read: app.post('/x', (req, res) => { z.object({ id: z.string() }).parse(req.body); // schema field -> post.id res.end(fs.readFileSync(req.query.id)); // the sink consumes get.id }); Schema fields are inserted first and same-named reads are then skipped, so `post.id` was the only surviving input while the flow analysis was looking at `req.query.id` — a precise candidate pinned to a parameter the payload never travels in. Both origins now go through ONE pass, and the comparison is by effective namespace rather than by source label. That distinction is the substance of the fix, not a detail: `json-body`, `form-body` and an Express `req.body` read are different labels for the same place (`post.*`) and must not be reported as a conflict, while `post.id` and `get.id` are the same label shape for different places. `namespaceOf` derives that from `runtimeCoordinate`, so the conflict test cannot drift from the addressing rules it is meant to police. Folding the read-vs-read case into the same pass means one mechanism covers both, and `requestMemberAccesses` now simply reports every source it saw per name (first-seen first) instead of a primary plus extras. The new corpus case keeps a control endpoint where the schema and the read agree: the goal is to refuse conflicts, not to refuse validated bodies. Verified both adversarial cases fail with the comparison disabled. Corpus: 6 stack + 6 adversarial. 884 tests. Co-Authored-By: Claude Opus 4.8 --- src/map/coordinates.ts | 13 +++++++ src/map/inputs.ts | 74 ++++++++++++++++++++++++---------------- tests/map/corpus.test.ts | 32 +++++++++++++++-- 3 files changed, 88 insertions(+), 31 deletions(-) diff --git a/src/map/coordinates.ts b/src/map/coordinates.ts index 0404f9e..844c8e1 100644 --- a/src/map/coordinates.ts +++ b/src/map/coordinates.ts @@ -40,6 +40,19 @@ export function runtimeCoordinate(source: InputSource | undefined, path: string) } } +/** + * The rule-engine NAMESPACE an input lands in (`post`, `get`, `cookie`, `files`, `server`), or null when + * it has no address. Derived from `runtimeCoordinate` on purpose: comparing raw source labels would call + * `json-body` and an Express `req.body` read different places when both resolve to `post.*`, and would + * miss that `post.id` and `get.id` are genuinely different places. + */ +export function namespaceOf(source: InputSource | undefined, path: string): string | null { + const { runtimeParameter } = runtimeCoordinate(source, path); + if (!runtimeParameter) return null; + const dot = runtimeParameter.indexOf('.'); + return dot === -1 ? runtimeParameter : runtimeParameter.slice(0, dot); +} + /** Attach `source` + the runtime coordinate to every extracted input. */ export function withCoordinates(fields: InputField[], source: InputSource): InputField[] { return fields.map((f) => ({ ...f, source: f.source ?? source, ...runtimeCoordinate(f.source ?? source, f.name) })); diff --git a/src/map/inputs.ts b/src/map/inputs.ts index c62af59..5ada8d1 100644 --- a/src/map/inputs.ts +++ b/src/map/inputs.ts @@ -1,7 +1,7 @@ import type { InputField, InputSource, TsModule } from './types.js'; import { bindingKey, rootIdentifier } from './ast.js'; import { npmPackageOf, type Bindings } from './bindings.js'; -import { runtimeCoordinate, withCoordinates } from './coordinates.js'; +import { namespaceOf, runtimeCoordinate, withCoordinates } from './coordinates.js'; const ZOD_BASE = new Set(['string', 'number', 'boolean', 'array', 'object', 'enum', 'bigint', 'date', 'record']); // String-format refinements a validator can declare — kept on the field so a rule can pin the shape. @@ -26,21 +26,45 @@ export function inputsFromHandler( ): InputField[] { // A validated schema inside a handler describes the request body — except for a payload-style entry // (a server action), where the schema describes the action's own argument. - const fields = withCoordinates(zodObjectFields(body, ts, bindings), opts.validatorSource ?? 'json-body'); - const names = new Set(fields.map((f) => f.name)); - for (const { name, source, alsoFrom } of requestMemberAccesses(params, body, ts, opts)) { - if (!names.has(name)) { - names.add(name); - // Read from two namespaces: no single rule parameter addresses both, so refuse the coordinate - // rather than guess which read a rule should inspect. - const coord = alsoFrom?.length - ? { - runtimeParameter: null, - runtimeParameterReason: `field is read from more than one request namespace (${[source, ...alsoFrom].join(', ')}): no single parameter addresses it`, - } - : runtimeCoordinate(source, name); - fields.push({ name, source, ...coord }); + const schemaSource = opts.validatorSource ?? 'json-body'; + const schemaFields = zodObjectFields(body, ts, bindings); + const reads = requestMemberAccesses(params, body, ts, opts); + + // ONE collision pass over both origins. A schema field and a request read can share a name while + // addressing different places — `z.object({ id }).parse(req.body)` next to `fs.read(req.query.id)` + // used to yield the schema's `post.id` while the sink actually consumed `get.id`, so a rule inspected + // a parameter the payload never travels in. Grouping by name is what makes that invisible, so the + // grouping now carries every source, and the verdict compares effective NAMESPACES: `json-body` and + // an Express `req.body` read are both `post.*` (not a conflict), `post.id` vs `get.id` is. + const sourcesByName = new Map(); + const add = (name: string, source: InputSource) => { + const list = sourcesByName.get(name) ?? []; + if (!list.includes(source)) list.push(source); + sourcesByName.set(name, list); + }; + for (const f of schemaFields) add(f.name, f.source ?? schemaSource); + for (const r of reads) for (const s of r.sources) add(r.name, s); + + const coordFor = (name: string, primary: InputSource) => { + const sources = sourcesByName.get(name) ?? [primary]; + if (new Set(sources.map((s) => namespaceOf(s, name))).size > 1) { + return { + runtimeParameter: null, + runtimeParameterReason: `field is read from more than one request namespace (${sources.join(', ')}): no single parameter addresses it`, + }; } + return runtimeCoordinate(primary, name); + }; + + const fields: InputField[] = withCoordinates(schemaFields, schemaSource) + .map((f) => ({ ...f, ...coordFor(f.name, f.source ?? schemaSource) })); + const names = new Set(fields.map((f) => f.name)); + for (const { name, sources } of reads) { + if (names.has(name)) continue; + const primary = sources[0]; // first-seen, so the reported source is deterministic + if (!primary) continue; + names.add(name); + fields.push({ name, source: primary, ...coordFor(name, primary) }); } return fields; } @@ -125,23 +149,18 @@ function requestMemberAccesses( body: any, ts: TsModule, opts: { payloadParam?: boolean } = {}, -): Array<{ name: string; source: InputSource; alsoFrom?: InputSource[] }> { +): Array<{ name: string; sources: InputSource[] }> { if (!body) return []; // Keyed by FIELD NAME, which two namespaces can share (`params.id` and `query.id` in one handler). // Last-write-wins silently picked one, and since the pick decided the coordinate, a handler reading // both compiled a rule pinned to `get.id` for data that arrives in the path segment — a wrong-input // pin, the one failure this whole layer exists to prevent. Collisions are now recorded, and the // first-seen source wins so the record is at least deterministic. - const out = new Map(); - const collisions = new Map>(); + const out = new Map(); const record = (name: string, source: InputSource) => { - const prev = out.get(name); - if (prev === undefined) { out.set(name, source); return; } - if (prev !== source) { - const set = collisions.get(name) ?? new Set([prev]); - set.add(source); - collisions.set(name, set); - } + const list = out.get(name) ?? []; + if (!list.includes(source)) list.push(source); + out.set(name, list); }; const p0 = params?.[0]; const reqName = p0 && ts.isIdentifier(p0.name) ? p0.name.text : undefined; @@ -207,10 +226,7 @@ function requestMemberAccesses( ts.forEachChild(n, visit); }; visit(body); - return [...out].map(([name, source]) => { - const also = collisions.get(name); - return also ? { name, source, alsoFrom: [...also].filter((s) => s !== source) } : { name, source }; - }); + return [...out].map(([name, sources]) => ({ name, sources })); // `req.body.x` / `req.query.x` / `req.params.x` — the namespace decides the runtime coordinate, and // route params notably have NONE, so this distinction is load-bearing rather than cosmetic. diff --git a/tests/map/corpus.test.ts b/tests/map/corpus.test.ts index 42f50b3..775840c 100644 --- a/tests/map/corpus.test.ts +++ b/tests/map/corpus.test.ts @@ -124,6 +124,34 @@ const ADVERSARIAL: Case[] = [ expectCandidates: [], expectRefused: [['id', /more than one request namespace/]], }, + { + name: 'adversarial: a validator field and the sink read address different namespaces', + kind: 'adversarial', + pkg: { dependencies: { express: '4', zod: '3' } }, + files: { + // The schema describes the BODY; the sink consumes the QUERY. Both are called `id`, and grouping + // inputs by name made the schema's `post.id` the only surviving entry — so the candidate pinned a + // parameter the payload never travels in. Namespace comparison (not source labels) is what catches + // this: a schema field and an Express `req.body` read are both `post.*` and must NOT be a conflict. + 'src/server.ts': ` + import express from "express"; + import fs from "node:fs"; + import { z } from "zod"; + const app = express(); + app.post("/mismatch", (req, res) => { + z.object({ id: z.string() }).parse(req.body); + res.end(fs.readFileSync(req.query.id)); + }); + app.post("/agree", (req, res) => { + z.object({ doc: z.string() }).parse(req.body); + res.end(fs.readFileSync(req.body.doc)); + }); + `, + }, + // `/agree` must still compile: the point is to refuse conflicts, not to refuse validated bodies. + expectCandidates: ['path-traversal @ post.doc'], + expectRefused: [['id', /more than one request namespace/]], + }, { name: 'adversarial: sibling expressions must not contaminate each other', kind: 'adversarial', @@ -350,9 +378,9 @@ describe('golden corpus', () => { it('keeps a standing adversarial category (lookalikes are how every false candidate got in)', () => { // Guards against the category quietly emptying out; the classes listed are the ones that have // actually produced false candidates, so losing one should fail loudly. - expect(ADVERSARIAL.length).toBeGreaterThanOrEqual(5); + expect(ADVERSARIAL.length).toBeGreaterThanOrEqual(6); const names = ADVERSARIAL.map((c) => c.name).join(' | '); - for (const cls of ['collide with dangerous API names', 'untraceable receivers', 'shadowing dangerous globals', 'two request namespaces', 'sibling expressions']) { + for (const cls of ['collide with dangerous API names', 'untraceable receivers', 'shadowing dangerous globals', 'two request namespaces', 'sibling expressions', 'different namespaces']) { expect(names, `missing adversarial class: ${cls}`).toContain(cls); } });