map: standing adversarial corpus category (+ fix a wrong pin it found) - #130
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>
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>
Twice now a false-candidate class has reached review rather than being caught here, and both times
the reason was the same: the corpus contained the shapes I thought of. It now has a permanent
ADVERSARIAL category — app code CONSTRUCTED to look dangerous — alongside the builder-stack cases:
- exports whose names collide with dangerous APIs (relative namespace + named imports)
- untraceable receivers in a file that really does import pg / supabase
- parameters shadowing dangerous globals
- one field name read from two request namespaces
- sibling expressions that must not contaminate each other
Building it immediately turned up a wrong-input pin, which is the failure the corpus metric exists
to hold at zero. Inputs are keyed by field NAME, and two namespaces can share one:
app.get('/qp/:id', ({ params: p, query: q }, res) => {
fs.readFileSync(p.id); // arrives in the path segment
fs.readFileSync(q.id); // arrives in the query string
});
Last-write-wins picked whichever read the walker saw last, and that pick decided the coordinate — so
this handler compiled `path-traversal @ get.id` for data arriving in the path. A rule pinned there
inspects the wrong place: it never fires on the real payload, while looking like coverage. Reversing
the two lines changed the verdict, which is the tell that no verdict was earned.
Collisions are now recorded during collection (first-seen source wins, so the record is at least
deterministic) and such an input gets NO coordinate, with a reason naming both namespaces. Refusing
costs the legitimate `get.id` candidate; a wrong pin costs trust in every candidate.
Two properties keep the new category honest:
- each adversarial case must detect a real surface (endpoints + inputs) before its zero-candidate
assertion counts — otherwise a parser bug or a typo'd fixture would read as a security property;
- the category cannot quietly empty out: the five classes are asserted by name.
Verified the fixture fails without the fix (`unexpected candidate(s): path-traversal @ get.id`).
Corpus: 6 stack + 5 adversarial projects. 881 tests.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Robust input-mapping overhaul with detailed namespace handling and tests 🎯 Quality: 94% Elite · 📦 Size: Large — consider splitting if possible 🛡️ Standards: no pre-flight fit check ran for this change — wire 📈 This month: Your 61st PR — above team average · Averaging Excellent |
…wo origins
The collision pass added with the adversarial category only looked at request READS, so the same
class survived one layer up — between a validator schema and a read:
app.post('/x', (req, res) => {
z.object({ id: z.string() }).parse(req.body); // schema field -> post.id
res.end(fs.readFileSync(req.query.id)); // the sink consumes get.id
});
Schema fields are inserted first and same-named reads are then skipped, so `post.id` was the only
surviving input while the flow analysis was looking at `req.query.id` — a precise candidate pinned to
a parameter the payload never travels in.
Both origins now go through ONE pass, and the comparison is by effective namespace rather than by
source label. That distinction is the substance of the fix, not a detail: `json-body`, `form-body`
and an Express `req.body` read are different labels for the same place (`post.*`) and must not be
reported as a conflict, while `post.id` and `get.id` are the same label shape for different places.
`namespaceOf` derives that from `runtimeCoordinate`, so the conflict test cannot drift from the
addressing rules it is meant to police.
Folding the read-vs-read case into the same pass means one mechanism covers both, and
`requestMemberAccesses` now simply reports every source it saw per name (first-seen first) instead of
a primary plus extras.
The new corpus case keeps a control endpoint where the schema and the read agree: the goal is to
refuse conflicts, not to refuse validated bodies. Verified both adversarial cases fail with the
comparison disabled. Corpus: 6 stack + 6 adversarial. 884 tests.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Confirmed and fixed — same class, one layer up, and the report's diagnosis was exactly right. Reproduced: Schema fields are inserted first and same-named reads are then skipped, so Both origins now go through one collision pass, and the comparison is by effective namespace rather than source label — that distinction is the substance of the fix, not a detail:
After: The corpus case keeps that Verified both adversarial cases fail with the comparison disabled (5 failures), so neither is decorative. Corpus now Agreed on the longer-term point, and I'd put it more strongly: input identity should be |
|
/review |
Twice now a false-candidate class has reached review instead of being caught by the corpus, and both times for the same reason: the corpus contained the shapes I thought of. Every such class so far came from code that merely resembled a dangerous API — so lookalikes have to be in the corpus on purpose, as a category rather than as one-off regressions.
New
kind: 'adversarial'cases, app code constructed to look dangerous:import * as helper from './util'+ named{ fetch, exec }from app coderes.locals.db.query(...)in a file importingpg/ supabaseconst send = (fetch) => fetch(req.body.url)params.idandquery.idin one handlerIt found a wrong pin immediately
Inputs are keyed by field name, and two namespaces can share one:
Last-write-wins picked whichever read the walker saw last, and that pick decided the coordinate — so this handler compiled
path-traversal @ get.idfor data arriving in the path. A rule pinned there inspects the wrong place: it never fires on the real payload, while looking like coverage. Reversing the two lines flipped the verdict, which is the tell that no verdict was earned:Collisions are now recorded during collection (first-seen source wins, so the record is deterministic) and such an input gets no coordinate, with a reason naming both namespaces. Both orders now refuse identically. This costs the legitimate
get.idcandidate — a wrong pin costs trust in every candidate, so that trade is not close.Two properties keep the category honest
Verification
I confirmed the new fixture fails without the fix (
unexpected candidate(s): path-traversal @ get.id), so it is not decorative. Corpus now reports6 stack + 5 adversarial projects · 9 candidates · 13 refused-with-reason. 881 tests, typecheck clean.