map: require an attributable receiver before a sink can auto-generate a rule - #129
Conversation
… a rule
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 <noreply@anthropic.com>
|
Well-scoped attribution feature with clear, consistent sink-handling enhancements. 🎯 Quality: 100% Elite · 📦 Size: Large — consider splitting if possible 📈 This month: Your 60th PR — above team average · Averaging Excellent |
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 <noreply@anthropic.com>
|
Confirmed and fixed — this was a real contradiction in the change itself, not an edge case: I added the Reproduced first: Generation now requires The refusal distinguishes inferred from untraceable, because those ask a reviewer to check different things. Fixtures added for all three paths as suggested — raw Two things the fixtures surfaced:
864 tests (81 files), typecheck clean. |
|
/review |
Three follow-ups from an external review of the map. I reproduced all three before changing anything, and each has a committed regression test.
1. Member-call sinks had no justification requirement — the one that matters
The bare-call path already required that a dangerous name resolve to a module plausibly providing that API. The member-call path only asked "is the receiver a local binding?" — and a namespace import of a relative module is neither local nor a package:
Three precise, auto-generatable candidates for ordinary app code. Same defect as the bare-call one fixed earlier — one syntax over, which is the real lesson: two paths recognizing the same thing under different rules.
baseOfnow 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 just as bare calls must.Recall is preserved rather than traded away.
sinksFromnow 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:helper.exec(...)→ harmless local fnstore.save(...)→ really callsfs.writeFileSyncfile: src/store.ts, not generatableres.locals.db.query(...)fs.readFileSync(...)/exec(...)from real importsUntraceable receivers keep the inventory/candidate split the map is built on: reported for a human, never auto-ruled. Any object can own a method called
query; a coordinate pinned on that guess is a rule that blocks real traffic for no reason. The newSink.attributionfield (import/global/inferred/ absent) makes that distinction explicit in the schema instead of leaving consumers to infer it from a missingpackage.2. Sink ids were not unique map-wide
The schema promises map-wide identity, but the hash covered the span and 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). Covered by a two-file collision test.
3. Namespace capture only worked in the parameter list
const { query: q } = req— the same capture one statement later — left the fields read offqinvisible. 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 stay heuristic, so they are visible without being auto-ruled — deliberate: making alias flows precise widens candidate generation and belongs in its own change with corpus coverage.
Verification
860 tests (81 files), typecheck clean. The golden corpus and every pre-existing candidate assertion pass unchanged — that's the point: this removes false candidates without removing true ones.