Skip to content

Commit 766fa0d

Browse files
map: prove flows with AST evidence; keep imported helpers in-project and locatable
Round-2 review fixes. Both are cases where the previous implementation could make a claim it hadn't earned. 1. FALSE `precise` FLOWS (high). linkFlows concatenated all argument source text and then checked, independently, that (a) the input's leaf name appeared anywhere and (b) any tainted root appeared anywhere. So: const { title } = await req.json(); db.from("items").insert({ title: "system", owner: req.user.id }); was reported `title -> insert [precise]` — `title` matched a property KEY and `req` matched a different value. Since a consumer may PIN A RULE on `precise`, that is the exact false positive the designation exists to avoid. Replaced with AST evidence: collect leaves genuinely READ from a tainted source (`data.title`, `req.body.title`, `{ title }` shorthand, `fn(title)`, `x[\"title\"]`), explicitly excluding property keys, member names and binding names; a flow is `precise` only if the input's leaf is among them, else `heuristic`. Evidence is gathered from the enclosing statement so a fluent chain (`.update({…}).eq('id', data.id)`) counts as one operation. 2. IMPORTED HELPERS (medium). - The resolver did not enforce the project boundary the walker enforces, so `import '../../other-repo/db'` (or a symlink) could pull an unrelated codebase into this app's attack surface. It now rejects anything whose realpath leaves the project unless --follow-symlinks. - An imported sink kept the HELPER's line number while the endpoint reported the handler's file, so the coordinate pointed at the wrong file. Sinks reached through an import now carry their own `file`; flow linking also refuses to call such a sink `precise`, since its call site isn't visible in the handler. - `import { saveOrder as write } from './db'` looked up `write` in the target module and missed the helper. Bindings now track the exported name behind an alias. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
1 parent 8504fe0 commit 766fa0d

3 files changed

Lines changed: 239 additions & 35 deletions

File tree

src/map/extract.ts

Lines changed: 130 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -48,12 +48,15 @@ const BUILTINS = new Set(builtinModules);
4848
// as local — an accepted miss.)
4949
interface Bindings {
5050
resolve(name: string): string | undefined;
51+
/** For `import { saveOrder as write }`, maps the local name back to the EXPORTED name. */
52+
exportNameOf(name: string): string | undefined;
5153
imports: Set<string>;
5254
locals: Set<string>;
5355
}
5456
function buildModuleBindings(sf: any, ts: TsModule): Bindings {
5557
const nameToModule = new Map<string, string>(); // local name → module specifier
5658
const declared = new Set<string>(); // every name declared in this file
59+
const exportNames = new Map<string, string>(); // local alias → exported name
5760
const imports = new Set<string>();
5861

5962
const record = (local: string, mod: string) => { nameToModule.set(local, mod); imports.add(mod); };
@@ -74,7 +77,11 @@ function buildModuleBindings(sf: any, ts: TsModule): Bindings {
7477
const nb = clause?.namedBindings;
7578
if (nb) {
7679
if (ts.isNamespaceImport(nb)) record(nb.name.text, mod);
77-
else if (ts.isNamedImports(nb)) for (const el of nb.elements) record(el.name.text, mod);
80+
else if (ts.isNamedImports(nb)) for (const el of nb.elements) {
81+
record(el.name.text, mod);
82+
// `import { saveOrder as write }` — looking up `write` in the target module would miss.
83+
if (el.propertyName && ts.isIdentifier(el.propertyName)) exportNames.set(el.name.text, el.propertyName.text);
84+
}
7885
}
7986
}
8087
if (ts.isFunctionDeclaration(node) && node.name) declared.add(node.name.text);
@@ -129,7 +136,7 @@ function buildModuleBindings(sf: any, ts: TsModule): Bindings {
129136
if (!changed) break;
130137
}
131138
const locals = new Set([...declared].filter((n) => !nameToModule.has(n)));
132-
return { resolve: (name: string) => nameToModule.get(name), imports, locals };
139+
return { resolve: (name: string) => nameToModule.get(name), exportNameOf: (name: string) => exportNames.get(name), imports, locals };
133140
}
134141

135142
// Root identifier of what a function body returns (`return createClient(…)` → "createClient"), for
@@ -198,7 +205,7 @@ export async function extractInputMap(cwd: string, ts: TsModule, options: Extrac
198205
let boundary = cwd;
199206
try { boundary = realpathSync(cwd); } catch { /* use cwd as-is */ }
200207

201-
const graph = createModuleGraph(ts); // shared cache across files
208+
const graph = createModuleGraph(ts, { cwd, boundary, followOutside: options.followSymlinks }); // shared cache
202209
const stats: WalkStats = { discovered: 0 };
203210
const files = collectSources(cwd, boundary, { followOutside: options.followSymlinks }, [], new Set(), stats);
204211
let parsed = 0;
@@ -813,7 +820,7 @@ export interface ModuleGraph {
813820
importedSinks(fromFile: string, specifier: string, exportName: string): Sink[];
814821
}
815822

816-
function createModuleGraph(ts: TsModule): ModuleGraph {
823+
function createModuleGraph(ts: TsModule, opts: { cwd: string; boundary: string; followOutside?: boolean }): ModuleGraph {
817824
// file → { fnSinks, calleesOf } | null (unreadable/unparseable)
818825
const cache = new Map<string, { fnSinks: Map<string, Sink[]>; calleesOf: Map<string, string[]> } | null>();
819826

@@ -836,14 +843,24 @@ function createModuleGraph(ts: TsModule): ModuleGraph {
836843
importedSinks(fromFile, specifier, exportName) {
837844
const target = resolveRelativeModule(fromFile, specifier);
838845
if (!target) return [];
846+
// Stay inside the project: `../../other-repo/db` (or a symlink) would otherwise pull an unrelated
847+
// codebase into this app's attack surface. The primary walker enforces this; so must the resolver.
848+
if (!opts.followOutside) {
849+
let real = target;
850+
try { real = realpathSync(target); } catch { /* use as-is */ }
851+
if (!isInside(real, opts.boundary)) return [];
852+
}
839853
const mod = load(target);
840854
if (!mod) return [];
841-
const out = [...(mod.fnSinks.get(exportName) ?? [])];
855+
const collected = [...(mod.fnSinks.get(exportName) ?? [])];
842856
// One same-file hop inside the target: `export function saveOrder(){ return doInsert() }`.
843857
for (const callee of mod.calleesOf.get(exportName) ?? []) {
844-
for (const s of mod.fnSinks.get(callee) ?? []) out.push(s);
858+
for (const s of mod.fnSinks.get(callee) ?? []) collected.push(s);
845859
}
846-
return out;
860+
// `line` refers to the HELPER's file, not the endpoint's — carry the file so the coordinate is
861+
// interpretable (and so flow linking never claims `precise` for a sink it cannot see locally).
862+
const rel = relative(opts.cwd, target);
863+
return collected.map((s) => ({ ...s, file: rel }));
847864
},
848865
};
849866
}
@@ -904,7 +921,7 @@ function sinksFrom(arrowOrNode: any, ts: TsModule, localSinks: Map<string, Sink[
904921
if (ctx) {
905922
const spec = bindings.resolve(called);
906923
if (spec && spec.startsWith('.')) {
907-
for (const s of ctx.graph.importedSinks(ctx.file, spec, called)) sinks.push(s);
924+
for (const s of ctx.graph.importedSinks(ctx.file, spec, bindings.exportNameOf(called) ?? called)) sinks.push(s);
908925
}
909926
}
910927
}
@@ -1026,32 +1043,58 @@ function linkFlows(
10261043
ts: TsModule,
10271044
): Flow[] {
10281045
if (!bodyNode || sinks.length === 0 || inputs.length === 0) return [];
1046+
1047+
// Roots that carry untrusted data: the handler's params, and locals aliased from them / from a
1048+
// request-body read. `leafOfLocal` maps a DESTRUCTURED local back to the field it came from, so
1049+
// `const { title: t } = await req.json()` links a read of `t` to the input `title`.
10291050
const taintedRoots = new Set<string>();
1051+
const leafOfLocal = new Map<string, string>();
10301052
for (const p of params ?? []) {
10311053
if (!p?.name) continue;
10321054
if (ts.isIdentifier(p.name)) taintedRoots.add(p.name.text);
10331055
else if (ts.isObjectBindingPattern(p.name)) {
1034-
for (const el of p.name.elements) if (ts.isBindingElement(el) && ts.isIdentifier(el.name)) taintedRoots.add(el.name.text);
1056+
for (const el of p.name.elements) {
1057+
if (!ts.isBindingElement(el) || !ts.isIdentifier(el.name)) continue;
1058+
taintedRoots.add(el.name.text);
1059+
const key = bindingKey(el, ts);
1060+
if (key) leafOfLocal.set(el.name.text, key);
1061+
}
10351062
}
10361063
}
1037-
// Local aliases of tainted data: `const body = await request.json()`, `const { title } = data`.
1064+
const isRequestRead = (init: any): boolean => {
1065+
let cur = init;
1066+
while (cur && (ts.isAwaitExpression(cur) || ts.isParenthesizedExpression(cur) || ts.isAsExpression(cur) || ts.isNonNullExpression(cur))) cur = cur.expression;
1067+
if (cur && ts.isCallExpression(cur) && ts.isPropertyAccessExpression(cur.expression)) {
1068+
const m = cur.expression.name.text;
1069+
if (['json', 'formData', 'text'].includes(m)) {
1070+
const root = rootIdentifier(cur.expression.expression, ts);
1071+
return root ? taintedRoots.has(root) : false;
1072+
}
1073+
}
1074+
if (cur && ts.isPropertyAccessExpression(cur) && REQ_SOURCES.includes(cur.name.text)) {
1075+
const root = rootIdentifier(cur.expression, ts);
1076+
return root ? taintedRoots.has(root) : false;
1077+
}
1078+
const root = cur ? rootIdentifier(cur, ts) : undefined;
1079+
return root ? taintedRoots.has(root) : false;
1080+
};
10381081
const aliasVisit = (n: any) => {
1039-
if (ts.isVariableDeclaration(n) && n.initializer) {
1040-
const root = rootIdentifier(n.initializer, ts);
1041-
const fromTainted = root ? taintedRoots.has(root) : false;
1042-
const isRequestRead = /\b(json|formData|text|body|query|params)\b/.test(n.initializer.getText?.() ?? '');
1043-
if (fromTainted || isRequestRead) {
1044-
if (ts.isIdentifier(n.name)) taintedRoots.add(n.name.text);
1045-
else if (ts.isObjectBindingPattern(n.name)) {
1046-
for (const el of n.name.elements) if (ts.isBindingElement(el) && ts.isIdentifier(el.name)) taintedRoots.add(el.name.text);
1082+
if (ts.isVariableDeclaration(n) && n.initializer && isRequestRead(n.initializer)) {
1083+
if (ts.isIdentifier(n.name)) taintedRoots.add(n.name.text);
1084+
else if (ts.isObjectBindingPattern(n.name)) {
1085+
for (const el of n.name.elements) {
1086+
if (!ts.isBindingElement(el) || !ts.isIdentifier(el.name)) continue;
1087+
taintedRoots.add(el.name.text);
1088+
const key = bindingKey(el, ts);
1089+
if (key) leafOfLocal.set(el.name.text, key);
10471090
}
10481091
}
10491092
}
10501093
ts.forEachChild(n, aliasVisit);
10511094
};
10521095
aliasVisit(bodyNode);
10531096

1054-
// Index sink call sites by line so a sink (which carries `line`) can be matched to its AST node.
1097+
// Index sink call sites by line so a sink (which carries `line`) can be matched back to its AST node.
10551098
const callsByLine = new Map<number, any[]>();
10561099
const callVisit = (n: any) => {
10571100
if (ts.isCallExpression(n)) {
@@ -1068,30 +1111,83 @@ function linkFlows(
10681111

10691112
const flows: Flow[] = [];
10701113
for (const sink of sinks) {
1071-
const candidates = sink.line !== undefined ? (callsByLine.get(sink.line) ?? []) : [];
1072-
// Text of every argument at this sink's call site(s) — where a tainted value would appear.
1073-
let argText = '';
1114+
// A sink from an imported module has no call site in THIS function — never claim precise for it.
1115+
const candidates = sink.file === undefined && sink.line !== undefined ? (callsByLine.get(sink.line) ?? []) : [];
1116+
const reads = new Set<string>();
10741117
for (const c of candidates) {
1075-
for (const a of c.arguments ?? []) {
1076-
try { argText += ' ' + a.getText(); } catch { /* ignore */ }
1077-
}
1118+
// Collect from the enclosing statement so a chained builder counts as one operation:
1119+
// `db.from(t).update({…}).eq('id', data.id)` — both `…` and `data.id` feed the same update.
1120+
for (const leaf of taintedReadLeaves(enclosingStatement(c, ts) ?? c, ts, taintedRoots, leafOfLocal)) reads.add(leaf);
10781121
}
10791122
for (const input of inputs) {
10801123
const leaf = input.name.split('.').pop()!.replace(/\[\]$/, '');
1081-
// `data.title` / `{ title }` / `req.body.title` — the leaf name appearing in the sink's args,
1082-
// qualified by a tainted root when it's a member path.
1083-
const mentionsLeaf = argText.length > 0 && new RegExp(`\\b${escapeRe(leaf)}\\b`).test(argText);
1084-
const mentionsTaintedRoot = [...taintedRoots].some((r) => new RegExp(`\\b${escapeRe(r)}\\b`).test(argText));
1085-
if (mentionsLeaf && (mentionsTaintedRoot || taintedRoots.has(leaf))) {
1086-
flows.push({ input: input.name, sink, confidence: 'precise', line: sink.line });
1087-
} else {
1088-
flows.push({ input: input.name, sink, confidence: 'heuristic', line: sink.line });
1089-
}
1124+
const precise = reads.has(leaf);
1125+
flows.push({ input: input.name, sink, confidence: precise ? 'precise' : 'heuristic', line: sink.line });
10901126
}
10911127
}
10921128
return flows;
10931129
}
10941130

1131+
/** Nearest enclosing statement, so a whole fluent chain is considered one operation. */
1132+
function enclosingStatement(node: any, ts: TsModule): any {
1133+
let cur = node;
1134+
while (cur && !ts.isStatement(cur)) cur = cur.parent;
1135+
return cur;
1136+
}
1137+
1138+
/**
1139+
* Leaf names of values that are genuinely READ from a tainted source inside `node`. This is the
1140+
* evidence behind a `precise` flow, so it is deliberately strict about what counts as a read:
1141+
* - `data.title` / `req.body.title` → yields `title` (a member read off a tainted root)
1142+
* - `{ title }` (shorthand) → yields `title` (a read of the tainted local)
1143+
* - `fn(title)` → yields `title`
1144+
* and explicitly NOT:
1145+
* - `{ title: "system" }` → `title` here is a property KEY, not a read of anything
1146+
* - `x.title` where `x` is untainted → not tainted data
1147+
* (Text matching previously conflated these, so a key plus an unrelated tainted mention elsewhere in
1148+
* the same argument list produced a false `precise`.)
1149+
*/
1150+
function taintedReadLeaves(node: any, ts: TsModule, taintedRoots: Set<string>, leafOfLocal: Map<string, string>): Set<string> {
1151+
const out = new Set<string>();
1152+
const visit = (n: any) => {
1153+
if (!n) return;
1154+
// A member read rooted in tainted data: take the accessed property as the leaf.
1155+
if (ts.isPropertyAccessExpression(n)) {
1156+
const root = rootIdentifier(n.expression, ts);
1157+
if (root && taintedRoots.has(root)) {
1158+
out.add(n.name.text);
1159+
return; // don't descend: the inner identifiers are the path, not separate reads
1160+
}
1161+
}
1162+
if (ts.isElementAccessExpression(n)) {
1163+
const root = rootIdentifier(n.expression, ts);
1164+
if (root && taintedRoots.has(root)) {
1165+
const arg = n.argumentExpression;
1166+
if (arg && ts.isStringLiteralLike(arg)) out.add(arg.text);
1167+
return;
1168+
}
1169+
}
1170+
if (ts.isIdentifier(n) && taintedRoots.has(n.text) && isValueRead(n, ts)) {
1171+
out.add(leafOfLocal.get(n.text) ?? n.text);
1172+
}
1173+
ts.forEachChild(n, visit);
1174+
};
1175+
visit(node);
1176+
return out;
1177+
}
1178+
1179+
/** Is this identifier occurrence a VALUE read (rather than a property key, a member name, a binding)? */
1180+
function isValueRead(id: any, ts: TsModule): boolean {
1181+
const p = id.parent;
1182+
if (!p) return true;
1183+
if (ts.isPropertyAssignment(p) && p.name === id) return false; // { title: … } — a key
1184+
if (ts.isPropertyAccessExpression(p) && p.name === id) return false; // x.title — the member name
1185+
if (ts.isBindingElement(p) && p.propertyName === id) return false; // { title: t } — the source key
1186+
if ((ts.isVariableDeclaration(p) || ts.isParameter(p) || ts.isBindingElement(p)) && p.name === id) return false;
1187+
if (ts.isPropertySignature(p) || ts.isMethodSignature(p)) return false;
1188+
return true; // includes ShorthandPropertyAssignment `{ title }`, which IS a read
1189+
}
1190+
10951191
function escapeRe(s: string): string {
10961192
return s.replace(/[.*+?^${}()|[\]\\]/g, '\\$&');
10971193
}

src/map/types.ts

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -39,8 +39,13 @@ export interface Sink {
3939
table?: string;
4040
/** The operation at the sink (db: insert | select | …; fs/exec/http: the called function). */
4141
op?: string;
42-
/** 1-based line of the sink call in the endpoint's file — the auditable coordinate. */
42+
/** 1-based line of the sink call, in `file` when present, otherwise in the endpoint's own file. */
4343
line?: number;
44+
/**
45+
* Repo-relative file of the sink call, set ONLY when the sink was reached through an imported module
46+
* — i.e. it does not live in the endpoint's file. Without this, `line` would point at the wrong file.
47+
*/
48+
file?: string;
4449
}
4550

4651
export interface Endpoint {

tests/map-flow-precision.test.ts

Lines changed: 103 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,103 @@
1+
import { describe, it, expect, beforeAll, afterAll } from 'vitest';
2+
import { mkdtempSync, mkdirSync, writeFileSync, rmSync, symlinkSync } from 'node:fs';
3+
import { tmpdir } from 'node:os';
4+
import { join, dirname } from 'node:path';
5+
import { buildInputMap } from '../src/map/index.js';
6+
7+
// `precise` is a claim a consumer may PIN A RULE ON, so it must be evidence-backed: the input has to be
8+
// genuinely READ into the sink. A property key that merely shares the input's name, with an unrelated
9+
// tainted value elsewhere in the same call, is NOT evidence.
10+
let dir: string, outside: string;
11+
beforeAll(() => {
12+
outside = mkdtempSync(join(tmpdir(), 'ps-other-repo-'));
13+
writeFileSync(join(outside, 'db.ts'), `
14+
import { createClient } from "@supabase/supabase-js";
15+
const c = createClient("u", "k");
16+
export function shouldNotBeSeen(x) { return c.from("secrets").delete().eq("id", x); }
17+
`);
18+
19+
dir = mkdtempSync(join(tmpdir(), 'ps-flow-'));
20+
mkdirSync(join(dir, 'src', 'lib'), { recursive: true });
21+
writeFileSync(join(dir, 'package.json'), JSON.stringify({ dependencies: { express: '4' } }));
22+
23+
// The counterexample: `title` appears only as a KEY; the tainted `req` appears in a DIFFERENT value.
24+
writeFileSync(join(dir, 'src', 'keyonly.ts'), `
25+
import { createClient } from "@supabase/supabase-js";
26+
const db = createClient("u", "k");
27+
export async function POST(req) {
28+
const { title } = await req.json();
29+
await db.from("items").insert({ title: "system", owner: req.user.id });
30+
return new Response("ok");
31+
}
32+
`);
33+
34+
// A genuine read of the input into the sink.
35+
writeFileSync(join(dir, 'src', 'real.ts'), `
36+
import { createClient } from "@supabase/supabase-js";
37+
const db = createClient("u", "k");
38+
export async function PUT(req) {
39+
const { title } = await req.json();
40+
await db.from("items").insert({ title });
41+
return new Response("ok");
42+
}
43+
`);
44+
45+
// Aliased import of a helper that owns the sink.
46+
writeFileSync(join(dir, 'src', 'lib', 'db.ts'), `
47+
import { createClient } from "@supabase/supabase-js";
48+
const c = createClient("u", "k");
49+
export function saveOrder(o) { return c.from("orders").insert(o); }
50+
`);
51+
writeFileSync(join(dir, 'src', 'alias.ts'), `
52+
import { saveOrder as write } from "./lib/db";
53+
export async function PATCH(req) {
54+
const body = await req.json();
55+
return write({ note: body.note });
56+
}
57+
`);
58+
59+
// An import that escapes the project directory.
60+
writeFileSync(join(dir, 'src', 'escape.ts'), `
61+
import { shouldNotBeSeen } from "${join(outside, 'db').replace(/\\/g, '/')}";
62+
export async function DELETE(req) { return shouldNotBeSeen(req.query.id); }
63+
`);
64+
});
65+
afterAll(() => { rmSync(dir, { recursive: true, force: true }); rmSync(outside, { recursive: true, force: true }); });
66+
67+
describe('flow precision', () => {
68+
it('does NOT claim precise when the input name is only a property key', async () => {
69+
const { map } = await buildInputMap(dir);
70+
const ep = map!.endpoints.find((e) => e.file.endsWith('keyonly.ts'))!;
71+
const titleFlows = ep.flows.filter((f) => f.input === 'title');
72+
expect(titleFlows.length).toBeGreaterThan(0);
73+
expect(titleFlows.every((f) => f.confidence === 'heuristic')).toBe(true);
74+
});
75+
76+
it('does claim precise for a real read (shorthand property)', async () => {
77+
const { map } = await buildInputMap(dir);
78+
const ep = map!.endpoints.find((e) => e.file.endsWith('real.ts'))!;
79+
expect(ep.flows.some((f) => f.input === 'title' && f.confidence === 'precise')).toBe(true);
80+
});
81+
82+
it('resolves an ALIASED imported helper to its exported name', async () => {
83+
const { map } = await buildInputMap(dir);
84+
const ep = map!.endpoints.find((e) => e.file.endsWith('alias.ts'))!;
85+
expect(ep.sinks).toEqual(
86+
expect.arrayContaining([expect.objectContaining({ kind: 'db', table: 'orders', op: 'insert' })]),
87+
);
88+
});
89+
90+
it('labels an imported sink with ITS OWN file, and never calls it precise', async () => {
91+
const { map } = await buildInputMap(dir);
92+
const ep = map!.endpoints.find((e) => e.file.endsWith('alias.ts'))!;
93+
const imported = ep.sinks.find((s) => s.table === 'orders')!;
94+
expect(imported.file).toBe(join('src', 'lib', 'db.ts'));
95+
expect(ep.flows.filter((f) => f.sink.table === 'orders').every((f) => f.confidence === 'heuristic')).toBe(true);
96+
});
97+
98+
it('refuses to follow an import outside the project directory', async () => {
99+
const { map } = await buildInputMap(dir);
100+
const ep = map!.endpoints.find((e) => e.file.endsWith('escape.ts'))!;
101+
expect(ep.sinks.some((s) => s.table === 'secrets')).toBe(false);
102+
});
103+
});

0 commit comments

Comments
 (0)