From 5b8fda296d7091b21ba800f07ea3a9d6c7d93fa0 Mon Sep 17 00:00:00 2001 From: Dave Jong Date: Thu, 13 Aug 2026 16:48:37 +0200 Subject: [PATCH 1/2] 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/2] 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'); + }); +});