feat(scan): add vulnerability classes and entry-point inventory - #200
feat(scan): add vulnerability classes and entry-point inventory#200Haiduongcable wants to merge 5 commits into
Conversation
The standard repository and scoped-path scan routes away from the discovery checklist and relies on a single six-category sentence for its entire vulnerability taxonomy. Concurrency and TOCTOU, workflow injection, prototype pollution, ReDoS, mass assignment, business-logic and authorization gaps, CORS, token validation, infrastructure configuration, and GraphQL had no coverage anywhere in the skills or references. Add a shared class reference and wire it into both the standard scan procedure and discovery so diff scans reach it as well. Each entry names the code shapes that make the class visible in source and the specific control that defeats it, so an instance can only be suppressed by naming that control. Also mark reviewed source as untrusted data in the standard scan procedure, which previously read every byte of the target without saying so.
Nothing in the plugin knows where untrusted input first reaches a target. No script mentions any web framework, and the ranking worklist only validates a model-supplied score, so a standard scan reads every file with no reachability prior at all. Add a model-free inventory that reports HTTP routes, GraphQL servers, serverless handlers, message consumers, server binds, and CI workflow triggers across Python, JavaScript, TypeScript, Go, Java, Kotlin, Ruby, PHP, and C#, with the handler symbol resolved where the syntax carries one. Wire it into the standard scan procedure to order review by reachability. Output is sorted and deduplicated so repeated runs are byte-identical, and every read is bounded. The inventory deliberately does not reuse two shared worklist constants, because both hide remote surface: generate_rank_input.EXCLUDED_DIRS contains .github, so workflow triggers are invisible to worklist-driven scans, and rank_preview.TEXT_CODE_EXTENSIONS omits .tf. State it as a prior, not a verdict: the procedure keeps reviewing every file in scope, since a file with no entry point can still hold a reachable bug through a helper.
|
Note You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Full matrix results from the fork PR at
Zero failures across all eight legs. Local gate in CI order, on the same commit:
The branch merges cleanly into |
mldangelo-oai
changed the title
4263997 to
e9943c4
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 05c2461cf9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| for kind, framework, pattern in PATTERNS: | ||
| match = pattern.search(line) |
There was a problem hiding this comment.
When a Python application uses the common @app.get(...) form, this loop tests the line against every language's patterns and emits three entries—Express, FastAPI, and Flask—for the same route. Because deduplication includes framework, those entries survive and inflate both entryPointCount and the file-density ranking; select patterns based on the file's language or deduplicate cross-language matches.
Useful? React with 👍 / 👎.
| CI_TRIGGER_RE = re.compile(r"^\s{0,8}(?P<sym>pull_request_target|pull_request|issue_comment|issues|workflow_run|workflow_dispatch|repository_dispatch|push|schedule)\s*:") | ||
| CI_ON_RE = re.compile(r"^\s{0,4}on\s*:") |
There was a problem hiding this comment.
In workflow files generated or formatted with quoted YAML keys, such as "on": followed by "pull_request_target":, these regular expressions match neither the trigger block nor its entries, so the inventory silently reports no CI entry points. Accept valid quoted mapping keys (or parse the workflow YAML) so the CI surface this feature explicitly prioritizes is not omitted.
Useful? React with 👍 / 👎.
| if len(seen) >= MAX_FILES: | ||
| return sorted(seen) |
There was a problem hiding this comment.
When a repository contains more than MAX_FILES eligible files, this early return drops the remaining files without communicating that to inventory, leaving summary.truncated false. Consumers therefore treat an incomplete entry-point inventory as complete and may fail to prioritize remotely reachable code in the omitted files; propagate the cap condition into the summary.
Useful? React with 👍 / 👎.
| def iter_files(repo: Path, scopes: list[Path]) -> list[Path]: | ||
| seen: set[Path] = set() | ||
| for scope in scopes: | ||
| candidates = [scope] if scope.is_file() else sorted(scope.rglob("*")) |
There was a problem hiding this comment.
For a repository containing a very large excluded tree such as node_modules or vendor, sorted(scope.rglob("*")) traverses and materializes every descendant before path_is_excluded and MAX_FILES are consulted. This defeats the stated bounds and can make inventory generation consume excessive memory or stall on exactly the large or hostile repositories it is intended to handle; use a pruning directory walk and apply the cap while traversing.
Useful? React with 👍 / 👎.
| Then inventory the remote entry points deterministically: | ||
|
|
||
| ```text | ||
| <python_command> <plugin_dir>/scripts/inventory_entry_points.py --repo <repo_root> --out <discovery_dir>/entry_points.jsonl --summary <discovery_dir>/entry_points.json |
There was a problem hiding this comment.
During a scoped-path scan, this documented invocation omits the script's --scope option, so it inventories the entire repository even though in_scope_files.txt contains only the requested path. The resulting artifact includes unrelated entry points and can divert the ordering and trace-inward work outside the user's requested scope; pass the same scope used to construct the file list.
Useful? React with 👍 / 👎.
| (KIND_HTTP, "koa", re.compile(r"\b(?:router)\s*\.\s*(?:get|post|put|patch|delete)\s*\(\s*[\"'`/]")), | ||
| (KIND_HTTP, "nestjs", re.compile(r"@(?:Get|Post|Put|Patch|Delete|All|Controller)\s*\(")), | ||
| (KIND_HTTP, "hapi", re.compile(r"\bserver\s*\.\s*route\s*\(")), | ||
| (KIND_SERVERLESS, "node", re.compile(r"\b(?:exports\s*\.\s*handler|module\s*\.\s*exports\s*\.\s*handler)\s*=")), |
There was a problem hiding this comment.
For JavaScript or TypeScript Lambda projects using standard ES-module exports such as export const handler = ... or export async function handler(...), this pattern emits no serverless entry point because it recognizes only CommonJS exports.handler forms. These handlers are common in modern Node/TypeScript deployments, so extend the pattern to cover ESM exports rather than silently omitting their remotely reachable surface.
Useful? React with 👍 / 👎.
| symbol = match.groupdict().get("sym") if "sym" in pattern.groupindex else None | ||
| if symbol is None and kind in {KIND_HTTP, KIND_SERVERLESS}: | ||
| symbol = symbol_near(lines, index) | ||
| rows.append(_row(relative, index + 1, kind, framework, symbol, line)) |
There was a problem hiding this comment.
When one source file contains more than MAX_ROWS matching lines, this append continues until the entire file has been processed because the cap is checked only before starting the next file. A generated or hostile file under the 1 MB byte limit can therefore create hundreds of thousands of row dictionaries before the final slice, defeating the memory bound the cap is meant to provide; stop collecting as soon as the cap is reached inside the line/pattern loops.
Useful? React with 👍 / 👎.
| stripped = line.lstrip() | ||
| if stripped.startswith(("#", "//", "*")): | ||
| continue |
There was a problem hiding this comment.
In JavaScript, Java, C#, and other block-commented source, a line such as /* app.get("/removed", handler); */ does not start with any of the prefixes checked here and is emitted as a live route. This makes disabled or explanatory code affect the reachability ordering despite the inventory's existing attempt to exclude commented-out routes; track block-comment state or otherwise remove comments before applying the patterns.
Useful? React with 👍 / 👎.
2 participants
Summary
Two related detection-surface additions to the bundled plugin. They are separable and I am happy to
split them into one PR per concern on request.
Impact
The standard repository and scoped-path scan routes away from the discovery checklist
(
skills/finding-discovery/SKILL.md) and relies on a single sentence for its entire vulnerabilitytaxonomy (
repository-wide-scan.md): "unsafe command execution, unsafe parsing, XSS,attacker-controlled network requests, unsafe file access, and missing permission checks."
Ten classes had zero mentions anywhere in
skills/orreferences/, verified by grep: raceconditions and TOCTOU, CI/CD workflow injection, prototype pollution, ReDoS, mass assignment,
business-logic and authorization gaps, CORS, token validation, infrastructure and container
configuration, and GraphQL.
Separately, nothing in the plugin knew where untrusted input first reaches a target. No script
mentions any web framework, and
generate_rank_input.pyonly validates a model-suppliedscore,so a standard scan reads every file with no reachability signal at all.
http_route,graphql,serverless_handler,message_consumer,server_bind,ci_triggeracross 9 languagesChanges
references/vulnerability-classes.md(new). Per class: the code shapes that make it visible insource, and the specific control that defeats it. Suppression requires naming that control — "the
framework probably handles it" is explicitly not one. Wired into
repository-wide-scan.mdand intofinding-discovery/SKILL.mdbefore the workflow split, so diff scans reach it too.scripts/inventory_entry_points.py(new, model-free). Emits sorted, deduplicated JSONL plus asummary. Resolves the handler symbol where the syntax carries one. Every read is bounded
(
MAX_FILES,MAX_FILE_BYTES,MAX_TOTAL_BYTES,MAX_ROWS, line truncation). Wired into thestandard scan procedure to order review by reachability, stated explicitly as a prior and not a
scope filter — every file in
in_scope_files.txtis still reviewed.tests-ts/entry-point-inventory.test.ts(new). 12 cases shelling the real script viaspawnSync(python, ["-I", "-B", ...]), following the existingscan-recovery.test.tspattern.plugin-files.json. Registers both new files;check-package.mjsrejects unregistered ones.Two shared constants deliberately not reused
Both hide real remote surface, and the tests assert the deviation as a regression guard:
generate_rank_input.EXCLUDED_DIRScontains.github, so workflow files are invisible toworklist-driven scans. Verified:
path_is_excluded(Path(".github/workflows/x.yml"))isTrue. Aworkflow trigger is among the most directly attacker-reachable entry points a repository has.
rank_preview.TEXT_CODE_EXTENSIONSomits.tf, so Terraform is invisible.Verification
Local, in CI order:
run types— cleanrun test— 722 pass / 5 skip / 0 fail (33 files)run format— cleanrun build— cleanpnpm pack+run check:package— validates 97 bundled plugin files, 181 tarball entriesA/B comparison of the class reference
Measured directly rather than asserted. Two whole-repo scans of the same fixture commit, same model
(
gpt-5.6-sol), same effort (xhigh), same concurrency, with the plugin tree differing in exactlythe three changed files and nothing else. The fixture is a 9-file, 385-line service carrying 19
planted instances across the ten newly covered classes, with negative controls alongside them.
The reference closed both remaining gaps and raised classes the baseline never surfaced:
CWE-200) — enabled unconditionally in the fixture and unreported bythe baseline.
ENVlayer, each filed as its own candidate rather than folded into the compose finding.CWE-732).It also decomposed authorization far more precisely: six per-resolver candidates for GraphQL
field-level authorization where the baseline filed one, each naming the specific resolver
(
User.apiKeys,User.invoices,Organization.members,Organization.billingContact,User.organization, and the top-levelorganizationquery) that resolves without a tenant check.Both arms independently flagged an
Origin-header authorization gap that the fixture author hadmislabelled as safe — the scanner was right and the answer key was wrong.
Scope: both runs were terminated by content-safety filtering (#56) after discovery and before
validation, so these are discovery candidates rather than validated findings, and the richer
treatment output carried a 66% higher estimated spend. One run per arm.
Notes
CI. All eight matrix legs pass on a fork PR at
6fa68d1, so the full matrix is already greenwithout needing a workflow approval here:
ubuntu-latestx node 22 / 24 / 24.0.0 / 26 / 26.0.0,macos-latestx node 22,windows-latestx node 22 (6m28s), and thelinux-amd64container job.Zero failures. Exact run links are in the follow-up comment.
Known overlap with in-flight work. #88 and #71 both modify
skills/security-scan/references/repository-wide-scan.md, which this branch also touches to wire inthe two new artifacts. The wiring is a 10-line addition and a one-line pointer; I am glad to rebase
onto whichever of those lands first, or to drop the wiring entirely and ship the script and reference
unwired if you would rather keep that file untouched while it is being reworked.
Projection.
references/andskills/look like a projection of an internal source rather thanthe editable original. Both wiring edits are deliberately minimal for that reason, and they would
need mirroring internally to survive a sync. If the reference file itself is better raised as an
issue than carried as a PR, say so and I will move it.
Scope of the A/B evidence. Both scans were cut short by content-safety filtering (#56) after
discovery and before validation, so the comparison above reports discovery candidates rather than
validated findings, and it is one run per arm on a fixture I wrote. The entry-point inventory's
correctness, determinism, and bounds are verified independently of any model spend.
Not changed deliberately. No schema changes, no TypeScript source changes,
_sarif_resultandSARIF_LEVELSuntouched, andgenerate_rank_input.py/rank_preview.pyleft alone even though theinventory documents two of their constants as blind spots -- changing shared worklist constants is a
separate decision that belongs with you, not bundled into a new-file addition.