map: report why an endpoint is unmodellable; make the coverage and sink schema honest - #126
Merged
Merged
Conversation
…nk schema honest
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 <noreply@anthropic.com>
|
New schema fields and limitations reporting improve analysis honesty. 🎯 Quality: 100% Elite · 📦 Size: Medium 📈 This month: Your 57th PR — above team average · Averaging Excellent |
…nction`
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 <noreply@anthropic.com>
Contributor
Author
|
/review |
daniloradovic
approved these changes
Aug 13, 2026
devlob
approved these changes
Aug 13, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Four gaps found by building a real consumer of this map (an attack-surface visualizer, kept out of the repo) plus the external review's "make unmodelled code visible" recommendation. Writing a consumer turned out to be the most effective schema review we've had.
1. Endpoint limitations — the improvement queue
An endpoint whose sink argument uses a dynamic computed key or a spread cannot be rule-generated. Previously it reported only "flow evidence is heuristic, not precise" — so the real cause was invisible, and an operator would reasonably conclude there was nothing there.
The specific cause now also replaces the generic wording in the affected flows'
ruleGeneratableReasons. A cleanly analysable endpoint gets nolimitations, so the field's presence is meaningful.2.
filesPreFiltered— stop the misleading coverage numberfilesParsed: 6, filesDiscovered: 66reads as "91% unanalysed". It actually means 60 files had no entry-point signal at all — most of a project is client code. The visualizer had to invent that segment itself to avoid alarming a reader; that shouldn't be a consumer's job. The three buckets are now explicit and sum to the total (asserted in tests).3. Stable sink ids
Flow.sinkis an embedded copy, so a consumer had to dedupe on a composite of eight fields — and would render a phantom second sink if a copy ever drifted from the inventory entry. Every sink now carries a deterministicid, and a flow's copy shares it (asserted, including determinism across runs).4.
SinkKindis a closed unionIt was typed
string, and the doc comment advertisedtemplate/redirectthat no recognizer emits — so a consumer couldn't switch exhaustively on it.Tests
tests/map/limitations.test.ts— 7 cases: each limitation kind with its expression and line, the specific cause appearing in flow reasons, a clean endpoint having none, the coverage buckets summing, sink ids being present/unique/shared-with-flows/deterministic. 837 tests green, typecheck clean.Examples in the offsite folder regenerated:
out5finally shows its real causes (dynamic-key,spread-into-sink) rather than three identical generic reasons.