From ba136f95c2460e39cb2f8a21839dbda2d30666c5 Mon Sep 17 00:00:00 2001 From: Dave Jong Date: Thu, 13 Aug 2026 15:40:33 +0200 Subject: [PATCH 1/2] map: report why an endpoint is unmodellable; make the coverage and sink schema honest MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four gaps found by building a real consumer of this map (an attack-surface visualizer) and by the external review's "make unmodelled code visible" recommendation. 1. LIMITATIONS. An endpoint whose sink argument uses a dynamic computed key or a spread cannot be rule-generated, and previously said only "flow evidence is heuristic" — so the actual cause was invisible and an operator would reasonably assume nothing was there. Endpoints now carry `limitations: [{ kind, detail, line }]` naming the offending expression (`body[field]`, `{ ...body }`), and the specific cause also appears in the affected flows' `ruleGeneratableReasons` instead of the generic wording. A cleanly analysable endpoint gets no limitations, so the field means something. 2. filesPreFiltered. `filesParsed: 6, filesDiscovered: 66` reads as "91% unanalysed" when it really means 60 files had no entry-point signal at all (most of a project is client code). The three buckets are now explicit and sum to the total, so no consumer has to infer it by subtraction — a visualizer had to invent that segment itself to avoid alarming a reader. 3. Stable sink ids. `Flow.sink` is an embedded COPY, so a consumer had to dedupe on a composite of eight fields and would render a second phantom sink if a copy ever drifted. Every sink now carries a deterministic `id`, and a flow's copy shares it. 4. SinkKind is a closed union instead of `string` — the doc comment advertised `template` and `redirect`, which no recognizer emits, so a consumer could not switch exhaustively. Co-Authored-By: Claude Opus 4.8 --- src/map/extract.ts | 96 +++++++++++++++++++++++++++++--- src/map/types.ts | 33 ++++++++++- tests/map/limitations.test.ts | 101 ++++++++++++++++++++++++++++++++++ 3 files changed, 219 insertions(+), 11 deletions(-) create mode 100644 tests/map/limitations.test.ts diff --git a/src/map/extract.ts b/src/map/extract.ts index 10e202a..3e54b58 100644 --- a/src/map/extract.ts +++ b/src/map/extract.ts @@ -2,7 +2,7 @@ import { readFileSync, readdirSync, realpathSync, statSync } from 'node:fs'; import { builtinModules } from 'node:module'; import { createHash } from 'node:crypto'; import { join, relative, isAbsolute, dirname, resolve as resolvePath } from 'node:path'; -import type { SiteInputMap, Endpoint, InputField, InputSource, Sink, Flow, ArgumentRole, CandidateFamily, TsModule } from './types.js'; +import type { SiteInputMap, Endpoint, InputField, InputSource, Sink, Flow, Limitation, ArgumentRole, CandidateFamily, TsModule } from './types.js'; // Framework-AGNOSTIC input-flow extractor. It doesn't gate on a specific stack — it walks any JS/TS // source and applies recognizer tables for (1) entry points, (2) inputs, (3) sinks, so it generalizes @@ -218,11 +218,12 @@ export async function extractInputMap(cwd: string, ts: TsModule, options: Extrac const stats: WalkStats = { discovered: 0 }; const files = collectSources(cwd, boundary, { followOutside: options.followSymlinks }, [], new Set(), stats); let parsed = 0; + let preFiltered = 0; for (const file of files) { try { const text = readFileSync(file, 'utf8'); - if (!hasEntrySignal(text)) continue; + if (!hasEntrySignal(text)) { preFiltered++; continue; } parsed++; // Coordinates are only valid for the exact file content they were derived from. const fingerprint = createHash('sha256').update(text).digest('hex').slice(0, 16); @@ -282,6 +283,7 @@ export async function extractInputMap(cwd: string, ts: TsModule, options: Extrac adapter: 'agnostic-v1', filesDiscovered: stats.discovered, filesParsed: parsed, + filesPreFiltered: preFiltered, filesSkipped: failed.length, roots: ['.'], notes, @@ -514,7 +516,7 @@ function extractFromFile(sf: any, ts: TsModule, localSinks: Map, ...spanOf(decl), inputs, sinks, - flows: linkFlows(handlerBody, handlerFn?.parameters, inputs, sinks, ts), + ...linkedFlows(handlerBody, handlerFn?.parameters, inputs, sinks, ts), }; // Honesty marker: a validator EXISTS but couldn't be read — inputs are unknown, not "none". if (validatorCall && inputs.length === 0) ep.inputsResolved = false; @@ -675,7 +677,7 @@ function handlerEntry( end: extra.end, inputs, sinks, - flows: linkFlows(body, params, inputs, sinks, ts), + ...linkedFlows(body, params, inputs, sinks, ts), }; } @@ -1145,14 +1147,20 @@ function isUninvokedFunctionDeclaration(n: any, ts: TsModule): boolean { // Deliberately conservative: a match yields `precise`; no match yields `heuristic` (the input and sink // merely co-occur). It never claims a flow it didn't see, which is the point — a consumer pinning a // rule to a parameter should trust `precise` and treat `heuristic` as "may reach". +// Spread onto an endpoint: `flows`, plus `limitations` only when there are any (keeps the common case clean). +function linkedFlows(body: any, params: any, inputs: InputField[], sinks: Sink[], ts: TsModule): { flows: Flow[]; limitations?: Limitation[] } { + const { flows, limitations } = linkFlows(body, params, inputs, sinks, ts); + return limitations.length > 0 ? { flows, limitations } : { flows }; +} + function linkFlows( bodyNode: any, params: any, inputs: InputField[], sinks: Sink[], ts: TsModule, -): Flow[] { - if (!bodyNode || sinks.length === 0 || inputs.length === 0) return []; +): { flows: Flow[]; limitations: Limitation[] } { + if (!bodyNode || sinks.length === 0 || inputs.length === 0) return { flows: [], limitations: [] }; // Tainted roots and the PATH each one stands for. `req` → '' (its own members are the path); // `const { billing } = await req.json()` → billing stands for 'billing', so a read of @@ -1232,6 +1240,7 @@ function linkFlows( callVisit(bodyNode); const flows: Flow[] = []; + const allLimits: Limitation[] = []; for (const sink of sinks) { // A sink from an imported module has no call site here — never claim precise for it. const node = sink.file === undefined && sink.start !== undefined && sink.end !== undefined @@ -1240,6 +1249,7 @@ function linkFlows( // path → the argument ROLES it was read into. Per-argument attribution is what makes a candidate // possible: the same value in `url` vs `body`, or `path` vs `content`, implies different mitigations. const reads = new Map>(); + const sinkLimits: Limitation[] = []; if (node) { // ONLY this sink call's own arguments, plus other calls in the SAME fluent chain // (`.update({…}).eq('id', data.id)` is one operation). Never the enclosing statement: a sibling @@ -1247,6 +1257,7 @@ function linkFlows( for (const call of fluentChainCalls(node, ts)) { const method = calleeName(call, ts); const args = call.arguments ?? []; + for (const a of args) for (const l of sinkArgumentLimitations(a, ts, rootPath)) sinkLimits.push(l); for (let i = 0; i < args.length; i++) { const role = argumentRoleOf(sink.kind, method, i); for (const path of taintedReadPaths(args[i], ts, rootPath)) { @@ -1279,6 +1290,13 @@ function linkFlows( 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 (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". + for (const l of sinkLimits) { + reasons.push(l.kind === 'dynamic-key' + ? `dynamic computed key reaches this sink (${l.detail}): the field cannot be named by a parameter` + : `spread reaches this sink (${l.detail}): the specific field is not identifiable`); + } if (precise && argumentRole && argumentRole !== 'unknown' && !family) { // e.g. a request value in a parameterized db `values` object: real reachability, but not a // pattern a generic blocking rule can express. @@ -1295,8 +1313,19 @@ function linkFlows( ruleGeneratableReasons: reasons, }); } + for (const l of sinkLimits) allLimits.push(l); } - return flows; + return { flows, limitations: dedupeLimitations(allLimits) }; +} + +function dedupeLimitations(list: Limitation[]): Limitation[] { + const seen = new Set(); + return list.filter((l) => { + const k = `${l.kind}:${l.detail}:${l.line}`; + if (seen.has(k)) return false; + seen.add(k); + return true; + }); } /** Join two path segments, tolerating an empty base. */ @@ -1429,6 +1458,46 @@ function taintedReadPaths(node: any, ts: TsModule, rootPath: Map return out; } +/** + * Shapes that defeat parameter pinning, found in a sink call's arguments. Reporting these is the point: + * "we could not model this" is far more useful to an operator than an endpoint that silently shows no + * flow, and it is the queue for improving the extractor. + * - `insert({ v: body[field] })` → the field is chosen at runtime; no coordinate can name it. + * - `insert({ ...body })` → the whole payload reaches the sink; which field is unidentifiable. + */ +function sinkArgumentLimitations(node: any, ts: TsModule, rootPath: Map): Limitation[] { + const out: Limitation[] = []; + const seen = new Set(); + const add = (kind: Limitation['kind'], detail: string, n: any) => { + const key = `${kind}:${detail}`; + if (seen.has(key)) return; + seen.add(key); + out.push({ kind, detail, line: lineOf(n) }); + }; + const text = (n: any) => { + try { return String(n.getText()).replace(/\s+/g, ' ').slice(0, 120); } catch { return ''; } + }; + const visit = (n: any) => { + if (!n) return; + // A computed member read off tainted data with a non-literal index. + if (ts.isElementAccessExpression(n)) { + const root = rootIdentifier(n.expression, ts); + const arg = n.argumentExpression; + if (root && rootPath.has(root) && arg && !ts.isStringLiteralLike(arg) && !ts.isNumericLiteral(arg)) { + add('dynamic-key', text(n), n); + } + } + // A spread of tainted data into the sink's argument. + if ((ts.isSpreadAssignment?.(n) || ts.isSpreadElement(n)) && n.expression) { + const root = rootIdentifier(n.expression, ts); + if (root && rootPath.has(root)) add('spread-into-sink', text(n.parent ?? n), n); + } + ts.forEachChild(n, visit); + }; + visit(node); + return out; +} + /** Canonical path of a member/element access rooted in a tainted binding, or undefined if not tainted. */ function pathFromTainted(node: any, ts: TsModule, rootPath: Map): string | undefined { const segs: string[] = []; @@ -1489,12 +1558,21 @@ function localCalls(node: any, ts: TsModule): string[] { return names; } +// 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 { + return createHash('sha256') + .update([s.kind, s.provider, s.package, s.table, s.op, s.file, s.start, s.end].join('|')) + .digest('hex') + .slice(0, 12); +} + function dedupeSinks(sinks: Sink[]): 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}`; - if (!seen.has(key)) { seen.add(key); out.push(s); } + 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) }); } } return out; } diff --git a/src/map/types.ts b/src/map/types.ts index a9073a5..6d82764 100644 --- a/src/map/types.ts +++ b/src/map/types.ts @@ -41,9 +41,12 @@ export interface InputField { runtimeParameterReason?: string; } +/** Sink families the extractor recognizes today. Kept as a closed union so a consumer can exhaustively + * switch on it; add a member here when a recognizer is added. */ +export type SinkKind = 'db' | 'fs' | 'http' | 'exec' | 'eval'; + export interface Sink { - /** db | fs | http | exec | template | redirect | … */ - kind: string; + kind: SinkKind; /** e.g. "supabase", "pg", "fetch". */ provider?: string; /** @@ -71,6 +74,20 @@ export interface Sink { */ start?: number; end?: number; + /** + * Stable identity of this sink within the map. `Flow.sink` is an embedded COPY for convenience, so + * correlate the two on this id rather than by deep-equality — a copy that ever drifts from the + * inventory entry would otherwise look like a second, distinct sink. + */ + id?: string; +} + +/** Something the analyser could not model at this endpoint — i.e. why it cannot be rule-generated. */ +export interface Limitation { + kind: 'dynamic-key' | 'spread-into-sink' | 'non-static-sink-argument' | 'unresolved-helper'; + /** The offending expression as written, e.g. `body[field]`. */ + detail: string; + line?: number; } export interface Endpoint { @@ -113,6 +130,12 @@ export interface Endpoint { * `inputs` are UNKNOWN rather than empty. Absent when the extracted inputs can be trusted as-is. */ inputsResolved?: boolean; + /** + * Why this endpoint (or a sink within it) cannot be turned into a rule — a dynamic computed key, a + * spread that hides which field reaches the sink, etc. This is the improvement queue: it is more + * useful than silently emitting an incomplete picture. + */ + limitations?: Limitation[]; } /** @@ -172,6 +195,12 @@ export interface Coverage { filesDiscovered: number; /** Files actually parsed (passed the entry-point pre-filter). */ filesParsed: number; + /** + * Files skipped BEFORE parsing because they contained no entry-point signal at all. These are not + * failures — most of a project is client code. Reported explicitly so a consumer never has to infer + * it by subtracting, which reads as "91% unanalysed". + */ + filesPreFiltered: number; /** Files skipped because they could not be read/parsed (fail-open). */ filesSkipped: number; /** Source roots analyzed, repo-relative. */ diff --git a/tests/map/limitations.test.ts b/tests/map/limitations.test.ts new file mode 100644 index 0000000..bc11e0a --- /dev/null +++ b/tests/map/limitations.test.ts @@ -0,0 +1,101 @@ +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'; + +// Making unmodelled code VISIBLE. An endpoint whose sink argument is a dynamic key or a spread cannot be +// rule-generated, and saying so — with the offending expression — is far more useful than showing no +// flow and letting an operator assume there is nothing there. This is the improvement queue. +let dir: string; +beforeAll(() => { + dir = mkdtempSync(join(tmpdir(), 'ps-lim-')); + mkdirSync(join(dir, 'src'), { recursive: true }); + writeFileSync(join(dir, 'package.json'), JSON.stringify({ dependencies: { '@supabase/supabase-js': '2', express: '4' } })); + writeFileSync(join(dir, 'src', 'unmodelled.ts'), ` + import { createClient } from "@supabase/supabase-js"; + const db = createClient("u", "k"); + export async function POST(req) { + const body = await req.json(); + const field = body.which; + await db.from("t").insert({ v: body[field] }); + await db.from("t2").insert({ ...body }); + return new Response("ok"); + } + `); + // A clean endpoint must NOT acquire limitations. + writeFileSync(join(dir, 'src', 'clean.ts'), ` + import fs from "node:fs"; + import express from "express"; + const app = express(); + app.post("/read", (req, res) => { res.end(fs.readFileSync(req.body.file)); }); + `); +}); +afterAll(() => rmSync(dir, { recursive: true, force: true })); + +const ep = async (file: string) => { + const { map } = await buildInputMap(dir); + return { map: map!, endpoint: map!.endpoints.find((e) => e.file.endsWith(file))! }; +}; + +describe('endpoint limitations', () => { + it('reports a dynamic computed key with the offending expression and line', async () => { + const { endpoint } = await ep('unmodelled.ts'); + const dyn = endpoint.limitations?.find((l) => l.kind === 'dynamic-key'); + expect(dyn).toBeDefined(); + expect(dyn!.detail).toContain('body[field]'); + expect(typeof dyn!.line).toBe('number'); + }); + + it('reports a spread that hides which field reaches the sink', async () => { + const { endpoint } = await ep('unmodelled.ts'); + const spread = endpoint.limitations?.find((l) => l.kind === 'spread-into-sink'); + expect(spread).toBeDefined(); + expect(spread!.detail).toContain('...body'); + }); + + it('names the specific cause in the flow reasons, not just "heuristic"', async () => { + const { endpoint } = await ep('unmodelled.ts'); + const reasons = endpoint.flows.flatMap((f) => f.ruleGeneratableReasons ?? []).join(' | '); + expect(reasons).toMatch(/dynamic computed key reaches this sink/); + expect(reasons).toMatch(/spread reaches this sink/); + // None of these may be rule-generatable. + expect(endpoint.flows.every((f) => f.ruleGeneratable === false)).toBe(true); + }); + + it('leaves a cleanly-analysable endpoint without limitations', async () => { + const { endpoint } = await ep('clean.ts'); + expect(endpoint.limitations).toBeUndefined(); + expect(endpoint.flows.some((f) => f.ruleGeneratable)).toBe(true); + }); +}); + +describe('schema honesty', () => { + it('reports filesPreFiltered explicitly rather than making consumers subtract', async () => { + const { map } = await ep('clean.ts'); + const c = map.coverage; + expect(typeof c.filesPreFiltered).toBe('number'); + // The three buckets must account for everything discovered — otherwise "6 of 66 parsed" reads as + // "91% unanalysed" when most of a project is simply client code with no entry point. + expect(c.filesParsed + c.filesPreFiltered + c.filesSkipped).toBe(c.filesDiscovered); + }); + + it('gives every sink a stable id so a flow copy can be correlated to the inventory', async () => { + const { map } = await ep('unmodelled.ts'); + for (const endpoint of map.endpoints) { + const ids = endpoint.sinks.map((s) => s.id); + expect(ids.every(Boolean)).toBe(true); + expect(new Set(ids).size).toBe(ids.length); // distinct per sink + for (const f of endpoint.flows) { + // The embedded copy carries the same identity as its inventory entry. + expect(ids).toContain(f.sink.id); + } + } + }); + + it('is deterministic: the same source yields the same sink ids', async () => { + const a = await ep('unmodelled.ts'); + const b = await ep('unmodelled.ts'); + expect(a.endpoint.sinks.map((s) => s.id)).toEqual(b.endpoint.sinks.map((s) => s.id)); + }); +}); From 954e54dea85c27bcb77e7320ec57701a25bdd9fc Mon Sep 17 00:00:00 2001 From: Dave Jong Date: Thu, 13 Aug 2026 15:50:00 +0200 Subject: [PATCH 2/2] map: require sinks to be justified; fix the coverage line and `new Function` MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three findings from external review. P1 (blocker for auto-generated rules) — a dangerous NAME was treated as a dangerous API. Any bare `fetch(…)`/`readFile(…)`/`exec(…)` that was not a top-level local counted as the real thing, so BOTH of these produced FALSE candidates: import { fetch, readFile } from "./util"; // app code, not HTTP/fs withClient((fetch) => fetch(req.body.url)) // a parameter shadowing the global The first became an SSRF candidate and the second a second one — directly violating the zero-false-candidate goal the corpus metric exists to protect. A bare call must now be justified: it resolves to a module that plausibly provides that API (fs → node:fs[/promises], exec → node:child_process, http → a known http package), or it is a genuine unresolved global — and only `fetch`, `eval` and `Function` ever are. Shadowing by an enclosing parameter or catch binding disqualifies it. A relative import resolves to no package, so app code that shares a name with an API is no longer mistaken for it. Impostors are not even inventoried as dangerous sinks now, so nothing downstream can resurrect them. P2 — the CLI recreated the ambiguity the schema fix removed. It printed "6/66 file(s) parsed" without saying the other 60 were deliberately pre-filtered, which reads as "91% unanalysed". It now prints all three buckets, and only the third is a failure: "66 file(s) found — 6 analysed, 60 skipped (no server entry point)". P2 — `new Function()` could never reach a precise flow. It was inventoried as an eval sink, but only CallExpression was indexed, so the call could not be located and every flow into it stayed heuristic (its argument-role entry was unreachable). NewExpression is now indexed, and the role model reflects the API: only the LAST argument is code — earlier ones declare parameter names. Verified: the three false candidates are gone (0), while genuine global-fetch / node:fs / child_process / new Function candidates all still compile — including the new code-injection candidate that was previously impossible. Co-Authored-By: Claude Opus 4.8 --- src/cli.ts | 8 ++- src/map/extract.ts | 74 ++++++++++++++++++++----- tests/map/sink-attribution.test.ts | 88 ++++++++++++++++++++++++++++++ 3 files changed, 155 insertions(+), 15 deletions(-) create mode 100644 tests/map/sink-attribution.test.ts diff --git a/src/cli.ts b/src/cli.ts index f158dea..bc74d4d 100644 --- a/src/cli.ts +++ b/src/cli.ts @@ -222,8 +222,12 @@ async function runMap(args: ParsedArgs): Promise { `${precise} proven input→sink flow(s) [${map.framework}].`, ); console.error( - `patchstack: ${c.filesParsed}/${c.filesDiscovered} file(s) parsed` + - (c.filesSkipped ? `, ${c.filesSkipped} skipped` : '') + + // All three buckets, explicitly: "6/66 parsed" reads as "91% unanalysed" when the other 60 files + // simply contain no server entry point (most of a project is client code). Only `skipped` is a + // failure to analyse. + `patchstack: ${c.filesDiscovered} file(s) found — ${c.filesParsed} analysed, ` + + `${c.filesPreFiltered} skipped (no server entry point)` + + (c.filesSkipped ? `, ${c.filesSkipped} could not be analysed` : '') + `. DETECTED surface only — static analysis is best-effort; unproven pairs are marked "heuristic".`, ); const json = JSON.stringify(map, null, 2); diff --git a/src/map/extract.ts b/src/map/extract.ts index 3e54b58..6cd8cd2 100644 --- a/src/map/extract.ts +++ b/src/map/extract.ts @@ -1107,18 +1107,36 @@ function directSinks(node: any, ts: TsModule, bindings: Bindings): Sink[] { } } } - // bare calls: fetch( / exec( / readFile( / eval( — unless the name is a plain local function. + // Bare calls: `fetch(…)` / `exec(…)` / `readFile(…)` / `eval(…)`. A dangerous NAME is not a + // dangerous API: `import { fetch } from './util'` and a callback parameter named `fetch` both look + // identical here, and treating either as an HTTP request produced a FALSE SSRF candidate. So the + // call must be justified — either it resolves to a module that plausibly provides that API, or it + // is a genuine unresolved global (only `fetch`/`eval`/`Function` ever are). if (ts.isIdentifier(callee) && !bindings.locals.has(callee.text)) { const name = callee.text; - const pkg = npmPackageOf(bindings.resolve(name)); - // `fetch` is a global — never attribute it to an unrelated imported http client. - if (HTTP_CALLS.test(name)) push({ kind: 'http', provider: name, package: pkg ?? (name === 'fetch' ? undefined : infer('http')), op: 'request', ...spanOf(n) }); - if (FS_CALLS.test(name)) push({ kind: 'fs', package: pkg, op: name, ...spanOf(n) }); - if (EXEC_CALLS.test(name)) push({ kind: 'exec', package: pkg, op: name, ...spanOf(n) }); - if (name === 'eval') push({ kind: 'eval', op: 'eval', ...spanOf(n) }); + const spec = bindings.resolve(name); + const pkg = npmPackageOf(spec); + const shadowed = isShadowedByEnclosingBinding(n, name, ts); + // A relative import resolves to no package: it's app code, not the API it shares a name with. + const fromModule = spec !== undefined; + 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) }); + } + // `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 (EXEC_CALLS.test(name) && pkg === 'node:child_process') { + push({ kind: 'exec', package: pkg, op: name, ...spanOf(n) }); + } + if (name === 'eval' && trueGlobal) push({ kind: 'eval', op: 'eval', ...spanOf(n) }); } } - if (ts.isNewExpression(n) && ts.isIdentifier(n.expression) && n.expression.text === 'Function') { + 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) }); } ts.forEachChild(n, visit); @@ -1232,7 +1250,9 @@ function linkFlows( // is ambiguous — the end distinguishes them. const callBySpan = new Map(); const callVisit = (n: any) => { - if (ts.isCallExpression(n)) { + // NewExpression too, or `new Function(...)` — inventoried as an eval sink — could never be located, + // leaving its flows permanently heuristic and its argument-role entry unreachable. + if (ts.isCallExpression(n) || ts.isNewExpression(n)) { try { callBySpan.set(`${n.getStart()}:${n.getEnd()}`, n); } catch { /* synthetic */ } } ts.forEachChild(n, callVisit); @@ -1259,7 +1279,7 @@ function linkFlows( const args = call.arguments ?? []; for (const a of args) for (const l of sinkArgumentLimitations(a, ts, rootPath)) sinkLimits.push(l); for (let i = 0; i < args.length; i++) { - const role = argumentRoleOf(sink.kind, method, i); + const role = argumentRoleOf(sink.kind, method, i, args.length); for (const path of taintedReadPaths(args[i], ts, rootPath)) { const set = reads.get(path) ?? new Set(); set.add(role); @@ -1380,17 +1400,45 @@ const CANDIDATE_FAMILIES: Record { if (!n) return; - if (ts.isCallExpression(n)) out.push(n); + if (ts.isCallExpression(n) || ts.isNewExpression(n)) out.push(n); if (ts.isCallExpression(n) || ts.isPropertyAccessExpression(n) || ts.isAwaitExpression(n) || ts.isParenthesizedExpression(n) || ts.isNonNullExpression(n)) { collect(n.expression); } diff --git a/tests/map/sink-attribution.test.ts b/tests/map/sink-attribution.test.ts new file mode 100644 index 0000000..01143fe --- /dev/null +++ b/tests/map/sink-attribution.test.ts @@ -0,0 +1,88 @@ +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'; + +// A dangerous NAME is not a dangerous API. `import { fetch } from './util'` and a callback parameter +// named `fetch` are indistinguishable from the global by name alone, and treating either as an HTTP +// request produced a FALSE SSRF candidate — a direct violation of the zero-false-candidate goal. A bare +// call now has to be justified: it resolves to a module that plausibly provides that API, or it is a +// genuine unresolved global (only fetch / eval / Function ever are). +let dir: string; +beforeAll(() => { + dir = mkdtempSync(join(tmpdir(), 'ps-attrib-')); + mkdirSync(join(dir, 'src'), { recursive: true }); + writeFileSync(join(dir, 'package.json'), JSON.stringify({ dependencies: { express: '4' } })); + writeFileSync(join(dir, 'src', 'util.ts'), 'export function fetch(u) { return { u }; }\nexport function readFile(p) { return p; }\nexport function exec(c) { return c; }\n'); + + // Impostors: same names, app code. + writeFileSync(join(dir, 'src', 'impostors.ts'), ` + import { fetch, readFile, exec } from "./util"; + import express from "express"; + const app = express(); + app.post("/local-helpers", (req, res) => { + fetch(req.body.url); readFile(req.body.name); exec(req.body.cmd); + res.end(); + }); + `); + // A parameter shadowing the global. + writeFileSync(join(dir, 'src', 'shadowed.ts'), ` + import express from "express"; + const app = express(); + function withClient(cb) { return cb((u) => ({ u })); } + app.post("/shadowed", (req, res) => { withClient((fetch) => { fetch(req.body.url); }); res.end(); }); + `); + // The genuine articles. + writeFileSync(join(dir, 'src', 'real.ts'), ` + import express from "express"; + import { readFile } from "node:fs/promises"; + import { exec } from "node:child_process"; + const app = express(); + app.post("/real-fetch", async (req, res) => { await fetch(req.body.url); res.end(); }); + app.post("/real-fs", async (req, res) => { await readFile(req.body.name); res.end(); }); + app.post("/real-exec", (req, res) => { exec(req.body.cmd); res.end(); }); + app.post("/real-fn", (req, res) => { const f = new Function("a", req.body.code); f(1); res.end(); }); + `); +}); +afterAll(() => rmSync(dir, { recursive: true, force: true })); + +const candidates = async () => { + const { map } = await buildInputMap(dir); + return map!.endpoints.flatMap((e) => e.flows.filter((f) => f.ruleGeneratable).map((f) => ({ route: e.route, ...f }))); +}; + +describe('sink attribution', () => { + it.each(['/local-helpers', '/shadowed'])('produces NO candidate for %s', async (route) => { + expect((await candidates()).filter((c) => c.route === route)).toEqual([]); + }); + + it('does not even inventory an impostor as a dangerous sink', async () => { + const { map } = await buildInputMap(dir); + for (const route of ['/local-helpers', '/shadowed']) { + const ep = map!.endpoints.find((e) => e.route === route)!; + expect(ep.sinks.filter((s) => ['http', 'fs', 'exec', 'eval'].includes(s.kind))).toEqual([]); + } + }); + + it.each([ + ['/real-fetch', 'ssrf'], + ['/real-fs', 'path-traversal'], + ['/real-exec', 'command-injection'], + ['/real-fn', 'code-injection'], + ])('still finds the genuine %s candidate (%s)', async (route, family) => { + const found = (await candidates()).filter((c) => c.route === route); + expect(found.map((f) => f.candidateFamily)).toContain(family); + }); + + it('models `new Function` — the code is the LAST argument, earlier ones are parameter names', async () => { + const { map } = await buildInputMap(dir); + const ep = map!.endpoints.find((e) => e.route === '/real-fn')!; + // Previously impossible: NewExpression was inventoried but never indexed, so it could not be located + // and every flow into it stayed heuristic. + const f = ep.flows.find((x) => x.input === 'code')!; + expect(f.confidence).toBe('precise'); + expect(f.argumentRole).toBe('code'); + expect(f.candidateFamily).toBe('code-injection'); + }); +});