map: resolve a client imported from a local module to its dependency - #132
Conversation
A regression I introduced, found while reviewing the integration plan against the code. The
attributable-receiver rule treats a RELATIVE receiver as app code — correct for a lookalike helper, wrong
for the layout generated apps actually use:
// src/lib/db.ts
export const db = createClient(url, key); // @supabase/supabase-js
// src/server.ts
import { db } from './lib/db';
app.post('/orders', (req, res) => db.from('orders').insert({ title: req.body.title }));
`db` resolved to the specifier `./lib/db` and nothing followed it, so the endpoint reported ZERO sinks.
Before the attribution work it at least surfaced as an unattributed sink (inventory only, never a
candidate); after, it was invisible — worse, because correlating a CVE to an endpoint joins on the sink's
package, so no vulnerability in `pg` or `@supabase/supabase-js` could ever be pinned to a route in the
common case.
The module graph already resolves relative specifiers and parses the target module (that is how imported
helper functions are followed). This adds a different question about the same data: not "what sinks are in
there" but "what does this export TRACE TO" — `importedPackage(fromFile, specifier, exportName)`, answered
by the target module's own bindings. `baseOf` asks it when the receiver is relative; a package means the
receiver IS that dependency, through an import-to-import chain that is fully static, so it earns
`attribution: 'import'` rather than the weaker `inferred`.
The narrowness is the point, and it is what keeps the earlier fix intact: the hop only produces a sink
when the export actually terminates in a dependency. `import * as helper from './util'` where that module
exports ordinary functions — including one named `from` — still yields nothing at all.
Restored, with a candidate that was previously invisible:
db.from('orders').insert({title}) -> db sink, @supabase/supabase-js, attribution import
pool.query(req.body.sql) -> db sink, pg, exact-local, sql-injection candidate
import { db as renamed } -> followed via its exported name
helper.exec(req.body.cmd) -> still no sink (the guard)
export { db } from './client' -> still no sink (one hop only, asserted as a limitation)
The project boundary applies to the new resolver too — a symlink leading out of the project is refused,
asserted rather than assumed. Corpus gains the lib/-client layout as a STACK case (not adversarial: this
is what normal generated code looks like), and every new assertion was checked against a build with the
hop disabled — 5 of them fail without it. 918 tests.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Resolves client imports through local modules into real package sinks 🎯 Quality: 100% Elite · 📦 Size: Medium 📈 This month: Your 62nd PR — above team average · Averaging Excellent |
…DB sink
Review of this branch caught a false-candidate path, and checking it against main showed the class is
older than this branch: package provenance was being treated as API provenance.
`.query()` / `.execute()` / `.from().insert()` are generic method names. Any receiver that resolved to a
real package was admitted as a SQL sink, so an `@apollo/client` instance — a genuine dependency with
nothing to do with SQL — produced `attribution: 'import'` and a precise SQL-injection candidate for a
GraphQL call. That rule would block legitimate traffic and mitigate nothing, which is the worst shape a
generated rule can have.
main: const local = new ApolloClient(); local.query(req.body.sql) -> candidate (already live)
main: import { client } from './lib/gql'; client.query(...) -> no sink
here: both shapes -> candidate
So the imported-client hop widened an existing hole rather than opening one. The fix is at the root: a DB
recognizer now requires the resolved package to establish a database API (`isDbPackage`, covering the
inference list plus real drivers not in it, and subpath imports such as `drizzle-orm/node-postgres`).
A traced package that is NOT a DB provider keeps its inventory entry — a `.query()` on an unknown client
is worth a human's attention — but it is marked `apiUnconfirmed`, which means: no rule, no `provider:
'sql'` claim, and no `candidateFamily` either. Advertising the sql-injection family on a GraphQL call
would mis-classify it for any consumer that reads the family without checking `ruleGeneratable`.
The refusal says which of the two things is missing, since they ask a reviewer to check different things:
"not a known db provider: it does not establish a db API (method name alone is not evidence)" vs the
existing untraceable-receiver and inferred-package reasons.
Coverage: both Apollo shapes (imported and same-file) plus controls for `pg` and a `drizzle-orm` subpath,
and a new adversarial corpus case — a non-DB client with a `.query()` method is precisely a lookalike, so
it belongs in that category. Verified 5 assertions fail with the gate disabled. 927 tests.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Confirmed and fixed in this PR. One correction to the framing, from checking it against The same-file client already resolved to The fix, at the rootPackage provenance was standing in for API provenance. A DB recognizer now requires the resolved package to actually establish a database API — Per your suggestion the sink stays in the inventory — a Two things I added beyond the suggestion, both for the same reason — a refused sink shouldn't keep making claims it can't support:
The reason string distinguishes this from the existing untraceable-receiver and inferred-package refusals — three different things for a reviewer to check, three different sentences. Coverage: both Apollo shapes, controls for |
Follow-up from review, taken now rather than deferred because it is the same mistake one field over. `res.locals.db.query(x)` in a file that imports `pg` was reported as `provider: 'sql'`. The flow was already refused (the package is only INFERRED from the file, not traced to the receiver), so no rule could come of it — but the inventory still asserted a SQL API about a receiver nobody traced, and the inventory is what a human reads. An inferred package means "this file talks to pg", never "this receiver is a pg client". `provider` is now set only when the receiver resolved (`attribution: 'import'`/`'global'`), for the SQL and prisma paths alike. `package` still carries the hint that made us look, and `attribution` already states how strong it is. The review suggested a `providerConfidence` field for this. I went the other way deliberately: that value would be derived from `attribution` and `provider`, and a second confidence field is exactly what drifts — this codebase has already had `confidence: 'precise'` survive in prose after it stopped existing in code. Deriving the claim at the point of construction keeps one source of truth. If a consumer later needs "possible DB API" as a distinct display state, it can compute it from the two fields it already has. Note this is NOT the same state as `apiUnconfirmed`: there the package is wrong for the API (`@apollo/client` for a `.query()`), here the package is right and the receiver is unknown. Two different things for a reviewer to check, so they stay distinguishable. One existing test looked its sink up BY `provider === 'prisma'` — the very claim being removed — so it now finds the sink by package and asserts the absent provider. Verified the new assertion fails with the gate reverted. 928 tests. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Taken now rather than deferred — it's the same mistake one field over, and #132 is already the PR about not overstating what we know. Reproduced first:
On Worth keeping the two states distinct, since they ask a reviewer for different work:
One existing test looked its sink up by |
|
/review |
Found by refreshing the demo viewer against this branch, which is a useful reminder that a consumer
reading the output notices things a test asserting one field does not.
Withholding `candidateFamily` for a sink whose package does not establish the API had a side effect: the
"argument role X is not a blockable pattern on its own" check keys off a missing family, so it fired too,
and the GraphQL `.query()` refusal read:
sink package "@apollo/client" is not a known db provider …
argument role "sql" on a db sink is not a blockable pattern on its own <- misleading
Role `sql` IS normally blockable. That second line sends a reviewer to look at the argument when the
problem is the package, and the queue of reasons is meant to be a work list, not a pile. The role check now
skips a sink whose API is unconfirmed; the package reason already explains the refusal, and the test pins
the refusal to exactly one reason.
928 tests.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
A regression I introduced, found while reviewing the integration plan against the code rather than by a test.
The attributable-receiver rule treats a relative receiver as app code. Correct for a lookalike helper — wrong for the layout generated apps actually use:
dbresolved to the specifier./lib/dband nothing followed it, so the endpoint reported zero sinks.Before the attribution work it at least surfaced as an unattributed sink — inventory only, never a candidate. After, it was invisible, which is worse: correlating a CVE to an endpoint joins on the sink's package, so no vulnerability in
pgor@supabase/supabase-jscould be pinned to a route in the most common case. It's the difference between the map working on real projects and working on single-file examples.The fix
The module graph already resolves relative specifiers and parses the target module — that's how imported helper functions are followed. This asks a different question about the same data: not "what sinks are in there" but "what does this export trace to".
baseOfasks it when the receiver is relative. A package means the receiver is that dependency, via an import-to-import chain that is fully static — so it earnsattribution: 'import', not the weakerinferred.The narrowness is the point
The hop only produces a sink when the export actually terminates in a dependency, which is exactly what keeps #129's guard intact:
db.from('orders').insert({title})@supabase/supabase-js,attribution: importpool.query(req.body.sql)(pg, fromlib/pool.ts)exact-local→ sql-injection candidate (previously invisible)import { db as renamed }helper.exec(req.body.cmd)where./utilexports plain functionsfromexport { db } from './client'The project boundary applies to the new resolver too: a symlink leading out of the project is refused, and that's asserted rather than assumed.
Verification
Corpus gains the
lib/-client layout as a stack case, not an adversarial one — this is what normal generated code looks like, so it belongs with the recall cases. Corpus now7 stack + 6 adversarial · 14 candidates.Every new assertion was checked against a build with the hop disabled: 5 fail without it, so none is decorative. 918 tests, typecheck clean.