jaimenbell/mcp-security-scanner

# mcp-security-scanner [](https://github.com/jaimenbell/mcp-security-scanner/actions/workflows/ci.yml) A static security scanner for [Model Context Protocol](https://modelcontextprotocol.io) servers. Point it at an MCP server repo; it reads the source and flags the vulnerability classes that actually show up in production MCP servers — with a severity, a `file:line`, a remediation, and an honest **confidence** on every finding. ## What it scans Seven detector families. The first six are grounded in a real finding from a fleet-wide audit of production MCP servers; the seventh (added 2026-07-21) covers scheduled jobs, wrappers, and IaC/CI files — cron, systemd, GitHub Actions, PowerShell/bash/batch deploy scripts: | # | Class | Detects | |---|---|---| | 1 | **Codegen / template injection** | Jinja `autoescape` off in a code-*generating* tool that renders untrusted fields into generated source; hand-rolled `replace('"','\"')` escaping instead of a real serializer (the mcp-factory class). | | 2 | **Tool-param injection** | `subprocess(shell=True)`, `os.system`, `eval`/`exec` on non-constants, `pickle.load`, `yaml.load` without `SafeLoader`, SSRF (caller-influenced fetch URL, no allowlist), path traversal (variable file path, no containment). | | 3 | **Auth / network posture** | Bind on `0.0.0.0` (escalates when paired with `debug=True`), Werkzeug/uvicorn `debug=True`, mutating routes (POST/PUT/DELETE/PATCH) with no auth dependency, no rate limiter on a networked server. | | 4 | **Secret handling** | Tracked `.env` / `*.pem` / `*.key` / keypair JSON, hardcoded secret literals (value-shape + secret-named assignments), secrets passed to log/print. | | 5 | **Write-tools-on-by-default / tool-scope-creep** (added 2026-07-19) | A mutating `@mcp.tool()`/`@server.tool()`-registered tool (name/verb or dangerous-sink-body heuristic, one hop through a delegated helper) with no visible gate -- decorator, env-flag opt-in, or permission check. | | 6 | **Secret-leak-via-tool-response** (added 2026-07-19) | An `@mcp.tool()`/`@server.tool()` function whose `return` expression hands back `os.environ`, a whole config/settings object, or a secret-named/secret-shaped value to the calling LLM. | | 7 | **Job hazards** (added 2026-07-21) | Scans `.yml`/`.yaml`/`.ps1`/`.sh`/`.bash`/`.bat`/`.cmd`/`.service`/`.timer` files for: over-broad credential/ACL scope (`permissions: write-all`, `icacls ... Everyone:F`, `chmod 777`, IAM `Action`/`Resource` wildcard pairs); a destructive call (`rm -rf`, `Remove-Item -Recurse -Force`, `terraform destroy`, `kubectl delete`, `git push --force`/`reset --hard`, `DROP TABLE`, `docker system prune`/`volume rm`, `schtasks /delete`, `aws s3 rm --recursive`) with no confirm-before-destroy gate (escalates to P0 when `-Confirm:$false` actively disables the built-in prompt); and success-reported-without-verification (`|| true`, a bare `; exit 0`, `continue-on-error: true`, an empty PowerShell `catch {}`). | Every finding carries **severity** (P0 critical → P3 hardening nit), **confidence** (high / medium / low), the offending `file:line`, and a concrete fix. ## Install / run # from the repo root python -m mcp_scanner.cli # markdown report python -m mcp_scanner.cli --json # JSON python -m mcp_scanner.cli --client-report --client-name "Acme" # 8-section client report python -m mcp_scanner.cli --fail-on P1 # CI gate: exit 2 on P0/P1 python -m mcp_scanner.cli --fail-on P1 --include-cli-only-in-gate # also gate on cli-only findings #### python -m mcp_scanner.cli --self-audit # scan your own fleet's servers `--fail-on` excludes `reachability: cli-only` findings by default — a cli-only finding's only known caller traces to a non-tool entrypoint (argv/ CLI-main, an admin script, a test file), never a registered MCP tool, so it does not block a build gate unless your CLI/admin surface is itself part of the attacker-reachable scope you want gated (a publicly-exposed management CLI, say) — pass `--include-cli-only-in-gate` to opt back in. `--self-audit` reads the directory to scan from the `MCP_SCANNER_FLEET_ROOT` environment variable — there is no baked-in default, so this repo carries no personal path. Set it to the directory containing the MCP server repos you want audited: ```bash export MCP_SCANNER_FLEET_ROOT=/path/to/your/mcp/repos # bash #### $env:MCP_SCANNER_FLEET_ROOT = "C:\path\to\your\projects" # PowerShell Without it set, `--self-audit` exits 1 with a clear error rather than silently defaulting anywhere. Or install the console script: ```bash pip install -e . #### mcp-scan The scanner reads only git-tracked source (falling back to a filtered tree walk in non-git dirs), so vendored deps, caches, and vector stores are skipped. ## The dogfood proof (`--self-audit`) #### The scanner is validated against **eight real, in-production MCP servers** — the strongest trust signal we can offer in a market that has been burned by simulated proof-of-work. Running `--self-audit` (live output, 2026-07-23): CLEAN mcp-factory P0=0 P1=0 P2=0 P3=0 <- codegen-injection class fixed upstream 2026-07-21 FINDINGS github-mcp P0=0 P1=2 P2=0 P3=0 <- 2x P1/LOW: its own fake test tokens, honestly reported CLEAN bus-mcp P0=0 P1=0 P2=0 P3=0 CLEAN desktop-mcp P0=0 P1=0 P2=1 P3=0 <- one P2/low heuristic note, clean bill CLEAN rag-mcp P0=0 P1=0 P2=1 P3=0 <- one P2/low heuristic note, clean bill FINDINGS discord-mcp P0=0 P1=1 P2=0 P3=0 <- 1x P1/LOW: TEST_TOKEN fixture, honestly reported CLEAN rails-mcp P0=0 P1=0 P2=0 P3=0 #### CLEAN vllm-ops-mcp P0=0 P1=0 P2=1 P3=0 <- one P2/low heuristic note, clean bill This is the acceptance test (`tests/test_self_audit.py`). Two notes on how to read it honestly: (1) the mcp-factory codegen-injection finding the original manual audit surfaced was genuinely fixed upstream (fleet drift reconciled 2026-07-21) — the detection class itself stays proven against `tests/fixtures/vuln_codegen`, and the test now pins current fleet reality (mcp-factory clean) rather than asserting a vuln that no longer exists. (2) The test's clean bar is "no HIGH/MEDIUM-confidence P0/P1": under the one law (below), a fleet repo whose own test fixtures embed an obviously-fake secret now shows a LOW-confidence P1 instead of zero findings — the severity-only `clean_bill` reports that as FINDINGS (github-mcp, discord-mcp above), which is the honest reading, and the test deliberately does not re-suppress it. ## Honest capability boundary - **Manifest-aware reachability (built 2026-07-21; low-level MCP SDK shape added 2026-07-23, N-vote-hardened across two rounds the same day).** After the detectors run, the scanner discovers the registered MCP tools (`@mcp.tool()` / `server.tool(...)` registrations, the low-level SDK's `Server()` + `@server.list_tools()`/`@server.call_tool()` + `types.Tool(...)`/bare `Tool(...)` shape, and any `server.json` manifest) and walks a static call-graph to label every finding **reachable** (inside a tool handler or a function transitively called from one), **unreachable-by-tools** (no call path from any registered tool), or **unknown**. `reachable` raises a finding's confidence; `unreachable` lowers it; a finding is **never dropped** on this basis (the over-flag philosophy stands). Stated limits: the same-file call-graph is exact; cross-file is best-effort by function name (not a resolved import graph); module-level code, non-Python (JS/TS/YAML/shell) findings, and repos with no discoverable tools are labelled `unknown` rather than guessed. It labels *reachability*; the separate taint pass (below) tracks the individual tainted value. The low-level-SDK discovery's `Tool()`-construction-to-dispatcher correlation is scoped to a single file/module (never a repo-wide guess across multiple dispatchers) and gated on the file actually importing something from the `mcp` package (so a same-named non-MCP class can't flip `has_tools` or claim a bogus root) -- round 1 of N-vote review caught a repo with more than one low-level dispatcher, or a coincidental `Tool`/`call_tool` name from an unrelated framework, downgrading a genuinely reachable finding to a lower-confidence grade; round 2 (Opus final-verify) caught two further shapes that still manufactured or leaked a bogus root -- a split declaration-module/dispatch-module layout (the list_tools fallback was rooting reachability at the metadata-only list_tools handler, since removed outright) and a repo mixing one genuinely ambiguous dispatcher with an unrelated valid one elsewhere (the repo-wide `has_tools`/`have_py_handlers` check was letting the valid root "unlock" confident grading for the un-rooted tool too). Fixed by treating an un-rooted `py-lowlevel-sdk` registration exactly like unresolved dynamic dispatch: withhold CLI_ONLY/UNCALLED in favor of UNKNOWN. All four shapes are regression-tested. **Coverage gap closed (2026-07-23, later same day; N-vote fix pass same day after 2 refuters found live repros in the first cut).** detector 5 (write-tools-on-by-default / tool-scope-creep) and detector 6 (secret-leak-via-tool-response) used to consume only decorator-style registration (JS-regex entries for scope-creep; a private `@mcp.tool()` walk for secret-leak) and produced zero findings for a repo using *only* the low-level SDK shape. Both now treat a provenance-gated `@server.call_tool()` handler as an inspection root via the shared `tool_registry.dispatch_segments` helper: it splits the handler's body into per-tool branches from a top-level `if name == "x": ... elif name == "y": ...` dispatch chain. Every link in the chain must compare the exact SAME discriminant expression (structural equality, not just "some string-equality test") -- the first cut accepted any ` == "literal"` at each link regardless of what `` was, so a stray `elif arguments.get("mode") == "delete_everything":` fabricated a root for a tool name that was never registered; once one link's discriminant breaks, that link and everything after it in the chain fall back to whole-handler attribution, never partially trusted. When more than one top-level `if` has a string-equality test, the chain whose discriminant matches the handler's first parameter (`name` in the conventional `call_tool(name, arguments)` shape) is preferred, so an unrelated earlier `if` can't be mistaken for the real dispatch root. A branch attributable to exactly one literal tool name is analyzed (mutating-sink/gate for scope-creep; leak-shaped-return for secret-leak) and attributed to that tool; a branch that isn't attributable (an `in (...)` test, the final `else`, a broken-chain link, or no dispatch shape at all -- a dict-keyed dispatch table / `match`/`case`) is still analyzed but attributed to the dispatch handler itself, never guessed at one tool -- the same never-guess-a-root discipline as the reachability fix above (verified end-to-end against rag-mcp's own real shape, `if name != "search_knowledge": ... else: ...` -- a `!=` guard isn't recognized as attributable dispatch, but the whole-handler fallback still fires on a leak/sink placed in the else-body, it just doesn't get per-tool attribution). Both detectors additionally follow one hop into a helper function a branch plainly delegates to (mirroring `tool_scope_creep.py`'s pre-existing one-hop helper-delegation convention) -- scoped **same-file-only**: an unrelated, never-imported same-named helper elsewhere in the repo (e.g. a debug script's own `_format` that happens to dump `os.environ`) is not followed, matching the same-file-only `Tool()`<->dispatcher correlation precedent already shipped for the reachability fix above. Gate detection for tool-scope-creep's low-level path is **per-branch**, not whole-handler-text: a gate hint (e.g. `is_authorized(...)`) inside one branch does not silence an ungated sibling branch's own mutating sink in the same handler -- only a genuinely shared pre-dispatch check (statements before the if/elif chain, which run unconditionally for every branch) legitimately gates every branch; the handler's own decorator and any module-level env gate still apply as before. **Known boundary, disclosed rather than silently left:** only a literal `==` if/elif chain with a consistent discriminant counts as attributable dispatch (no real dataflow); a file with zero or 2+ `@server.call_tool()` handlers is skipped entirely by both detectors (never guess a root) -- **this means a repo with more than one dispatch handler in the same file gets NO detector-5/detector-6 coverage at all for that file**, not a partial or lower-confidence one, exactly as disclosed for the reachability grading above. **Named follow-up CLOSED (2026-07-23, later same day; two N-vote rounds).** The PRE-EXISTING decorator-registered path in `tool_scope_creep.py` (predates the low-level-SDK work above, `_build_gate_index`/`func_index` built repo-wide by short function name in `run()`) carried the inverse bug from the per-branch fix above -- an N-vote refuter proved live that an unrelated, same-named gated helper elsewhere in the repo could silence an ungated decorator-registered mutating tool (a false NEGATIVE on this detector's primary target class, the worse direction than a false positive). **Round 1** fixed this by moving the decorator path onto the same `_build_function_index_for_file` same-file-only convention the low-level path already used. **A second N-vote pass then proved round 1 over-corrected**, with two further live repros: (a) same-file scoping silently severed the cross-file SINK hop too, not just the gate hop -- a non-mutating-named tool one-hop-delegating through an explicit import to a real, reachable, ungated sink in a separate file went to total silence (0 findings), the worst possible direction for this detector's primary target class; (b) the same-file gate index still unioned bare method names across DIFFERENT classes in one file -- an unrelated same-named gated method on another class could still silence a genuinely ungated one. **Round 2** replaced same-file-only with a bounded, one-hop, IMPORT-AWARE resolver: an explicit, statically-resolvable same-repo import (`import mod`, `from x.y import z`, and relative equivalents including the real fleet's own `from .groups import write` submodule-import shape) is followed for BOTH sink detection and gate credit -- exactly one target file per import statement, never a repo-wide guess; a same-file class-qualified call (`ClassName().method(...)` / `self.method(...)` from within that class) is cheaply disambiguated to the exact method; anything else falls back to the same-file bare-name heuristic, now with an explicit ambiguity rule (sink OR'd across same-named candidates, gate credit AND'd -- disagreement withholds credit rather than unioning a false-clean). Disclosed residual, honest and unavoidable: a hop that resolves to neither an explicit import nor a same-file candidate (e.g. a helper imported from a genuine third-party package) is a real miss for a non-mutating-named tool whose entire mutating behavior lives behind it -- gate not credited, sink not followed, over-flag stands only for tools whose name already looks mutating. Verified against the fleet's own real MCP servers both rounds (`--self-audit`, all five stay clean, no new P0/P1) -- every exposure found was synthetic-fixture-proven, never fleet-observed. `clean_tool_scope` (the original cross-file wrapper->write.py fixture) round-tripped through both fixes -- briefly over-flagged after round 1, correctly quiet again after round 2's import-aware resolver -- and its test was updated in place at each step rather than left to rot; new fixtures (`clean_tool_scope_same_file_gate`, `vuln_tool_scope_cross_file_sink_import`, `vuln_tool_scope_same_file_class_collision`, `clean_tool_scope_groups_import`, `vuln_tool_scope_unresolvable_external_hop`) pin each shape precisely. The low-level SDK dispatch path is NOT touched by round 2 and stays same-file-only -- a cleanly reusable follow-up (the same import-aware resolver would apply there too), not attempted here. - **Sink classification: resolved, not syntax-shaped (fixed 2026-07-23, N-vote-hardened over two passes the same day).** Detector 5's (tool-scope-creep) `_is_mutating_sink_call` used to test `if "subprocess" in name` -- a bare substring match against the call's own dotted/bare name -- so a HELPER FUNCTION merely named `_run_subprocess` was misclassified as a direct dangerous-sink call even though its body never touched the real `subprocess` module. Reproduced live against vllm-ops-mcp: `get_gpu_status`/`get_service_status`/`get_serve_config` each delegate through `probes.py` to a helper literally named `_run_subprocess`, producing 3 false P1/HIGH findings. **Round 1** replaced the substring test with exact `module.attr` dotted-path matching, gating the short-name fallback behind `"." in name` -- but two N-vote refuters proved live that this was itself a syntax-shape proxy standing in for "is this an attribute access", and broke on both sides: (P0) it blanket-excluded every bare `Name` call, silencing REAL sinks reached via a direct stdlib import (`from os import remove; remove(path)`, `from shutil import rmtree`, `from subprocess import run`) that base correctly caught; (P1) `_dotted` collapses to the bare leaf whenever an attribute call's receiver isn't a plain `Name` (`Path(x).unlink()`, `requests.Session().post(url)`, `get_proc().run(cmd)`), so the "." gate silently missed those idiomatic sink shapes too. **Round 2** replaced the proxy with RESOLUTION: a per-file `_SinkFileCtx` (module aliases, direct stdlib-sink imports, repo-internal function names) built from machinery this repo already owns (`_build_import_map`, the same-file function index), plus structural `isinstance(call.func, ast.Attribute)` gating instead of testing the rendered string. A bare call now resolves to (a) a known stdlib-sink import -> sink, canonicalized to its real spelling; (b) a repo-internal function -> not a sink by itself, the one-hop resolver inspects its real body elsewhere (the original `_run_subprocess` fix, done correctly this time); (c) genuinely unresolvable -> over-flag-safe short-name fallback, restoring base's original catch. The exact-match set also gained `subprocess.getoutput`/`getstatusoutput` and dropped a dead `subprocess.popen` (lowercase, no such callable) entry. Verified live (before/after `--self-audit` fleet sweep, `MCP_SCANNER_FLEET_ROOT` set to the 8-repo fleet, both rounds): the 3 vllm-ops-mcp findings go from 3×P1/HIGH to **zero** -- not a suppression this fix added, but an exposed side effect of an already-existing, already-disclosed limitation: vllm-ops-mcp's real chain is TWO hops deep (`get_gpu_status_tool` -> `probes.get_gpu_status` -> `_run_subprocess` -> `subprocess.run`), and this detector's one-hop resolver only inspects the first resolved hop's own body. This two-hop-miss shape now has its own permanent regression fixture (`clean_tool_scope_two_hop_probe_miss`), closing a gap the round-1 pass had left as prose only. Separately, a severity/confidence calibration for the general ONE-hop-reachable case: `subprocess.run`/`Popen`/`call`/`check_output`/`check_call` invoked WITHOUT `shell=True` calibrates to P2/MEDIUM instead of P1/HIGH; a `shell=True` string command stays P1/HIGH; `os.system`/`os.popen`/`getoutput`/`getstatusoutput` stay unconditionally high-risk (always shell-interpreted, no argv-list form exists) -- note the MEDIUM half is only visible for a finding reachability grades `unknown`/`unreachable`: the scanner's pre-existing reachability pass raises MEDIUM to HIGH for any finding on a directly-reachable MCP tool (the common case), so in practice the calibration's user-visible signal is almost always the severity drop (P1 -> P2) alone. This calibration now resolves module aliases and bare stdlib imports too (`import subprocess as sp; sp.run(cmd)` without `shell=True` correctly calibrates to P2, not an unconditional P1 -- round-2 fix). See `tool_scope_creep.py`'s module docstring ("Round 3" + the round-2 N-vote fix comment above `_SinkFileCtx`) and `tests/test_tool_scope_creep_sink_substring_fix*.py` (both files) for the full rationale, every refuter repro pinned as a permanent regression test, and all other pinned cases. Fleet sweep across all 8 repos, both rounds, confirms zero new noise elsewhere (desktop-mcp/rag-mcp's pre-existing single unrelated finding each, github-mcp/bus-mcp/discord-mcp/rails-mcp/mcp-factory unchanged). - **Tool-parameter taint tracking v1 (built 2026-07-21; cross-file budget raised 2026-07-22).** A second post-detector pass seeds every registered tool handler's parameters as taint **sources** and propagates them through assignments, f-strings/concat/`.format()`, common containers, and same-repo function calls into the param-injection **sinks** (subprocess/`os.system`/`eval`/`exec`/`pickle`/`yaml.load`/HTTP-fetch/`open`). Each such finding is labelled **tainted** (a tool parameter provably reaches the sink), **untainted** (the sink is in tool-reachable code but fed a constant / other source), or **unknown**. `tainted` raises confidence; `untainted` lowers it; a finding is **never dropped** (the over-flag philosophy stands — an `untainted` sink is still reported, just lower-confidence). Stated limits, honestly: same-file dataflow is transitive, and cross-file follows **up to two direct-import hops** (no third hop, no cross-repo flow); it is **not sanitizer-aware** (a validated/escaped value is still treated as tainted, by design); and it does not model dynamic dispatch (`getattr` / `*args` / `**kwargs` re-binding), decorator transforms, module-level code, or non-Python surfaces — all labelled `unknown` rather than guessed. Deeper (3+ hop) and cross-repo taint remain out of scope. - **Static only.** No dynamic analysis. Reachability and taint are inferred from the static call-graph above, not observed at runtime. - **Confidence is load-bearing.** `low` findings are "a human should glance at this," and produce false positives by design (e.g. a variable file path in a test file). Most are P2/P3 — but since the one-law demotions (fake-marker, test-cert, author-suppressed), a `P1/LOW` finding is a real, intended shape: `clean_bill` is severity-only, so a P1/LOW **does** break the product clean bill (see the dogfood table above — that's honest, not a bug); only the self-audit test's stricter "no HIGH/MEDIUM-confidence P0/P1" bar filters LOW. - **Not a git-history scanner.** The secret detector reads the tracked working tree, not full history. Pair it with `gitleaks` for history. - **Language coverage (updated 2026-07-22).** Deep for Python (AST-based). JS/TS has no AST path in this scanner -- it is line-based regex (shared helpers in `mcp_scanner/js_util.py`), the same approach `job_hazards.py` already used for its non-Python file types. Collected extensions (`js_util.JS_SUFFIXES`, shared by `scanner.py`'s `_SCAN_SUFFIXES` and `tool_registry`): `.js`, `.mjs`, `.cjs`, `.ts`, `.mts`, `.cts`, `.jsx`, `.tsx` -- `.jsx`/`.tsx` JSX syntax (attribute `{...}` braces, `{cond && }` conditional-render braces, `{/* JSX comment */}`) was verified, not assumed, not to trip the brace/string-aware helpers: `tests/fixtures/vuln_tsx_dashboard` mixes a real eval() sink, an ungated mutating tool, and a secret-named return field with a realistic JSX render block, and every finding lands on the sink line, none inside the JSX; `clean_tsx_dashboard` carries the identical JSX shapes with safe code and stays fully quiet. Four detector families now have JS/TS parity on this regex basis: **param-injection** (exec/execSync always-shell -- including `node:child_process` and destructure-aliased bindings, e.g. `const { exec: run } = require(...)` -- spawn/execFile with `shell:true`, `eval()`/`new Function()`, `yaml.load` without a safe schema, fetch/axios/http(s).get SSRF, fs read/write path-traversal), **tool-scope-creep** (mutating `server.tool(...)` registrations with no gate, via a capped line-window heuristic standing in for a real function-body scope; gate-hint matching is comment-stripped so a `// TODO: needs auth_required` note can't suppress a finding), **secret-leak-via-tool-response** (`process.env`/whole-config/secret-named/hardcoded-secret returned from a tool -- same-line compressed object literals and multi-line returns alike -- via the same window heuristic plus a string-literal-aware brace-depth tracker), and **secret-handling**'s secret-in-log check (`console.*`/`logger.*` calls with a secret-named argument, word-boundary-guarded against a name that merely contains a secret-vocabulary substring; hardcoded-secret-value scanning was already language-agnostic). **Not covered for JS/TS, by deliberate scope decision:** codegen-injection (the mcp-factory class is inherently a Python-Jinja pattern) and auth-posture (bind/debug/mutating-route checks are inherently Flask/FastAPI-decorator-shaped; an Express/Fastify equivalent is new detector logic, not JS parity of the existing one -- left for a future increment, not attempted here). Jinja templates remain regex-level as before. - **Known JS/TS regex-heuristic gaps (documented, not built -- 2026-07-22 adversarial review, both waves; extension gap closed 2026-07-22, see Language coverage above).** Stated honestly rather than silently missed: optional-chaining eval (`globalThis?.eval?.()`) isn't matched by the `eval(` sink regex; a spread-of-secret-variable return (`return {...apiKey}`) isn't decomposed the way a named key is, and neither is the equivalent `return Object.assign({}, {apiKey: process.env.API_KEY})` shape (same family -- a secret-named key wrapped in a call other than a plain object literal); `tool_registry`'s JS registration regex is comment-blind, so a commented-out `// server.tool('foo', ...)` still registers a phantom tool -- an over-flag, the direction this scanner already accepts, not an under-flag; and the JS registration-window heuristic used by tool-scope-creep / secret-leak-via-tool-response is documented as "40 lines" but is actually 41 (`start` through `start + 40` inclusive) -- a cosmetic off-by-one in the docstring, not a functional gap. - **FP-class reduction, wave 1 + rounds 2-3 (2026-07-23).** Driven by a one-time, read-only static scan of 13 popular public MCP server repos (official-org servers, frameworks, and single-purpose servers spanning Python and JS/TS, ~4.9k-88.8k GitHub stars, each with a commit in the prior ~3.5 months) -- hand-reviewed to separate genuine findings from scanner false positives. That scan's own artifacts are a local, gitignored working file (not part of this repo, and not re-fetchable by a reader), so the evidence that ships HERE, durably, is the regression fixture in each test file below: every fixture reproduces the REAL shape of the false positive (anonymized, never verbatim third-party source beyond the minimal triggering pattern) and is re-run on every `pytest` invocation, including in CI. Four evidenced false-positive classes, each fixed as a precision-only demotion that requires proof, never a guess -- two further N-vote passes (round 2, round 3) then found and closed adversarial gaps in the fixes THEMSELVES, converging on **one law, stated once and applied everywhere, no per-case exceptions: full suppression is reserved for OUR OWN curated exact-match judgment; every other signal -- a target's suppress comment, an obviously-fake value marker, a cert's test-path/short-validity, a name-demotion with no independent value check -- may only demote confidence and tag the finding, it may never make it disappear.** Round 2 found five gaps against this law; round 3's N-vote then found that round 2's OWN fixes had re-introduced three more special-cased exceptions to it (an unanchored fake-marker FULL-SUPPRESS, a cert-demotion OR-gate where validity alone was sufficient, and a keyword-blind regex-context heuristic that leaked real braces into the scope walker) plus one more shared-helper bypass: - **Pagination/continuation-cursor field names** (`next_token`, `page_token`, `cursor`, ...) no longer trip the secret-name heuristic (`secret-leak-via-tool-response`, `secret-in-log`). Demotion (`secret_handling.py`'s `_is_pagination_cursor_name`) requires BOTH the pagination word-shape (next/page/continuation combined with token/cursor, or a bare `cursor`) AND the absence of any stronger credential word in the same identifier -- `access_token`/`refresh_token`/`client_secret`/`page_token_secret` never demote. Real example: awslabs/mcp's `next_token` field in a paginated tool response. **Value-shape backstop:** a name-demotion is never the ONLY signal -- `_SECRET_VALUE_PATTERNS` gained JWT-shaped and Bearer-prefixed patterns, and `secret_leak_response.py` resolves a Name leaf to its own assigned literal (same-scope, simple assignments only) so a real JWT/bearer secret assigned to a pagination-demoted name still flags on VALUE shape alone. **Round-3 fix on the backstop itself:** round 2's guard against obviously-fake values (`_is_real_secret_value_match`) was an unanchored substring FULL-SUPPRESS -- `PROD_API_KEY = "sample-tier-<68 real hex chars>"` (a real secret whose value merely contains "sample" as an unrelated tenant/tier component) vanished to zero findings, live-reproduced. Replaced with `_has_fake_marker` + `_compose_demotion`: a fake/dummy/placeholder/sample/demo/test marker, tokenized the same way identifier words are (so it matches inside `github_pat_fake_test_token`, which a naive `\b` regex never does -- underscore is a word character), now only demotes confidence to LOW and tags the title `(fake-marker)`, applied uniformly to every value-shape pattern (no special-cased subset of labels), NEVER a `continue`. - **Self-signed TEST certificates** at a test-fixture path (`tests/`, `fixtures/`, `testserver/`, ...) no longer trip `tracked-secret-file` on extension alone. Demotion requires the test-path, a provable self-signed marker (issuer == subject, parsed via the optional `cryptography` package -- see the `certs` extra in `pyproject.toml`), AND the cert's own CN/SAN must match a localhost/test/example/demo/dummy/mock/fixture pattern -- self-signed + test-path alone discriminates nothing real (a genuine internal-CA production root is self-signed too, and integration suites routinely embed real staging/prod TLS material under `tests/`). **Round-3 fix:** round 2 wired the CN/SAN marker as one arm of an OR-gate with short (<=90-day) validity and a sub-2048-bit RSA key -- live-reproduced verbatim at the exact 90-day boundary (a prod-shaped CN, normal key, short validity) demoting to total invisibility. The CN/SAN marker is now REQUIRED; validity and key size are not independently sufficient (plenty of legitimately-short-lived real certs exist). **Cert demotion is also no longer a silent break:** a demoted cert (or its cryptographically-paired private key) still emits a finding -- LOW confidence, tagged `(test-cert)` -- it never vanishes with zero trace. A cert whose issuer differs, a self-signed cert outside a test path, a self-signed test-path cert with no CN/SAN marker, or any cert when `cryptography` isn't installed all stay HIGH-confidence flagged (fails closed, never guesses). Real example: microsoft/playwright-mcp's `tests/testserver/cert.pem` + `key.pem` (CN=playwright-test). - **Well-known placeholder credentials** (documented SDK example literals, e.g. AWS's own `AKIAIOSFODNN7EXAMPLE`/`wJalrXUtnFEMI/K7MDENG/bPxRfiCYEXAMPLEKEY` pair) fully suppress `hardcoded-secret` via a curated exact-match set (`secret_handling._KNOWN_PLACEHOLDER_SECRETS`), never fuzzy/substring matching -- OUR OWN judgment, so a real secret that merely resembles a placeholder still flags. A maintainer's own suppress-convention comment (`# pragma: allowlist secret`, the `detect-secrets` convention) is a DIFFERENT, weaker signal (it's target-authored and attacker-controllable in an adversarial scan) -- it only demotes confidence and tags the finding "author-suppressed," it never fully suppresses, and it's evaluated per-MATCH (`.finditer()`, not `.search()`) so a real secret co-located on the same line as a suppressed placeholder still flags independently. Applied consistently in both the line-based literal scan and the AST `NAME = "literal"` assignment branch. Real example: awslabs/mcp's `dynamodb-mcp-server` `DUMMY_ACCESS_KEY`. - **Pre-existing dead-code fix, found during round-2 review, in scope because it falsified wave-1's own precision claims:** the AST `NAME = "literal"` hardcoded-secret-assignment branch's `_PLACEHOLDER` regex contained an empty alternative in its top-level group, making `.match()` unconditionally truthy for any input -- this branch has silently never fired, for anything, since 2026-07-13. Fixed (no empty alternative, `.fullmatch()` instead of partial `.match()`, the curated placeholder list and pragma-demotes convention both wired in properly). **Round-3 fix:** this branch was still routing its NAME check through the raw `_SECRET_NAME.search()` regex instead of the shared `_name_looks_secret` helper, bypassing both the word-boundary glued-word guard AND the pagination-cursor-name exclusion (OUR OWN curated name-shape judgment, the same "may fully exclude" category as the placeholder list) -- 14 of awslabs/mcp's 209 revived-branch findings were pagination-named assignments (`expected_response.next_token = '...'`) that should never have reached this branch. Now routes through `_name_looks_secret` like every other name-based check in this module. - **RegExp.exec() vs child_process.exec() (JS/TS).** `param_injection.py`'s JS shell-injection check used to flag ANY `.exec(`/`execSync(` call in a file that imports `child_process` anywhere, regardless of receiver -- so `myRegex.exec(str)` in a file that also legitimately uses `execSync` elsewhere was misclassified as a shell-injection sink. Resolves the call's receiver via `_js_bindings_by_scope`/`_resolve_receiver_kind`: every RegExp-var and direct child_process-module binding is scoped to its own innermost enclosing `{...}` block (module-level declarations stay visible everywhere, matching real JS closures), and a receiver resolves to whichever binding's scope is smallest/most specific at the usage line -- real-JS-shadowing-correct (the first cut was file-wide with no scoping at all, so a RegExp declared in one function could mask a REAL child_process sink of the same name in a completely different function; both directions confirmed live and fixed). **Round-3 fix on the scope walker itself:** a `/` immediately after a KEYWORD (`return /^{$/.test(x)` -- "return" ends in an alnum char, not a symbol) was never recognized as a regex-literal opener by the original context heuristic, so its embedded `{`/`}` leaked into the brace-based scope walk as real code braces, corrupting/merging function spans and masking a real `cp.exec(cmd)` sink elsewhere in the same file (live-reproduced, 0 findings). Fixed two ways: (1) the regex-context heuristic now also recognizes a preceding keyword (`return`/`typeof`/`case`/`in`/`of`/`delete`/`void`/`do`/`else`/`yield`/`await`/`instanceof`/`new`/`throw`), not just a preceding symbol; (2) the scope walker now FAILS CLOSED -- if a file's braces don't balance at all (whether from a genuinely malformed file or a regex shape the heuristic still doesn't catch), EVERY binding in that file is discarded outright (not "treated as module-scope," which would be the wrong, mask-a-sink direction) and every `.exec(` receiver in it stays over-flagged, matching pre-scope-fix parity. A literal `child_process` receiver, an unresolvable receiver, or a bare/aliased `exec(...)` call (no receiver at all) all stay flagged too. - **The one law (stated once, no per-case exceptions):** this scanner's use case is scanning THIRD-PARTY, possibly-adversarial repos, not auditing a cooperative owner's own code. **Full suppression is reserved for OUR OWN curated exact-match judgment** (the placeholder list, the pagination-cursor-name shape, the cert CN/SAN test-identity marker; plus two narrower, value-shape-safe cases for completeness: the AST name-branch's placeholder value-shape regex `_PLACEHOLDER` -- double-covered, since the line-based value scan does not consult it, so it can never hide a value-shaped secret -- and the pre-existing `.env.example`/`.sample`/`.template`/`.dist` filename skip, which suppresses only the file-level tracked-secret-file finding while values inside stay content-scanned) -- **every other signal may only demote confidence and tag the finding, it may never make a finding disappear.** This includes a target's suppress comment, an obviously-fake value marker, and a cert's test-path/self-signed/short-validity shape. A direct, practical consequence: a fleet server whose OWN test fixtures embed an obviously-fake-named secret (e.g. `"Bearer github_pat_fake_test_token_1234"`, `TEST_TOKEN = "fake-test-token-do-not-use"`) now correctly shows a LOW-confidence finding instead of zero -- `clean_bill` (severity-only by design) reflects that honestly; the dogfood test suite's bar for "clean" is calibrated to tolerate LOW-confidence-only noise rather than re-suppressing it (see `tests/test_self_audit.py`). - **Test-path confidence demotion (wave 4, CLOSED 2026-07-23; round-2 safety fix same day).** Reviving the dead AST-assignment branch (above) surfaced a substantially larger finding volume on test-heavy repos -- one real-world ecosystem clone went from 5 to ~196-209 `hardcoded-secret` findings, almost entirely mock/test credential assignments in test files (`mock_credentials.token = "..."`, `provider._password = "test..."`). Wave 4 closes this noise class the same way every other FP class here is handled: a `hardcoded-secret` finding demotes to LOW confidence and is tagged `(test-path)` -- **never dropped, suppressed, or `continue`'d.** **Round-2 fix (refuter-B P1):** the first cut demoted on the bare test-fixture PATH alone, which dropped a genuine value-shaped secret (`AKIA...`/`ghp_...`/`sk-...`/JWT/private-key) to LOW purely because it sat under a segment like `fixtures/`, `spec/`, `mocks/`, `testserver/`, or `testdata/` -- all of which are **production-plausible** (Django/Rails `fixtures/` seed prod DBs via `loaddata`; `spec/` holds OpenAPI/protobuf/JSON-schema specs; `mocks/` ships as a runtime MSW feature; Go's `testdata/` VCR cassettes have a real-world precedent for containing ACTUAL recorded prod credentials). A bare path name is **target-controllable and discriminates nothing real** -- weaker than the cert-path precedent (README above), which demotes only on **multiple** independent corroborating signals (self-signed AND test-path AND CN/SAN marker). **The corroboration rule now:** `test-path` is a PAIR-ONLY signal in `_compose_demotion` -- it **never demotes on its own**. It demotes only when paired with a corroborating co-signal beyond the bare path, or merely tags a finding already demoted by a standalone signal. Concretely: (1) the **value-shape branch** (`AKIA.../ghp_.../-----BEGIN PRIVATE KEY-----/sk-.../JWT/Bearer ...`) demotes ONLY on a standalone signal -- an author `pragma` (target-controllable but explicit) or a `fake-marker` **in the value itself** (self-limiting: a value literally containing `xxxx`/`fake`/`example` as a whole word cannot also be a working credential); a bare test path merely appends its tag to an already-demoted finding, so **a real value-shaped secret with no other signal keeps its base HIGH confidence in ANY directory** (the hard invariant); (2) the weaker **AST-name branch** (base MEDIUM) demotes when a **mock/fake-shaped assignment NAME** (`mock_credentials.token`, `fake_provider._password`) pairs with a test path -- two signals -- which preserves the original noise-reduction goal; this is safe for a real secret because a value-shaped value is independently caught HIGH by the value-shape branch (a name can never pull it down), and this branch only demotes the weaker name-based duplicate whose value is an arbitrary string. **Safety-vs-noise tradeoff (safety wins):** a realistic-looking secret value in `testdata/` that is NOT self-evidently fake and NOT mock-named now stays HIGH -- some noise reduction is traded away rather than risk demoting a real leak on path alone. Regression-pinned in `tests/test_secret_testpath_demotion.py` (mock/fake-named assignment demotes; a real `AKIA`/`ghp_`/private-key value with no other signal STAYS HIGH under `tests/`, `fixtures/`, `spec/`, `mocks/`, `testserver/`, `testdata/`; a fake-VALUED secret still demotes; outside-tests confidence unchanged; tag composition never yields zero findings). Fleet `--self-audit` before/after: identical totals (nothing real lost visibility). - **Grading honesty + JS/TS precision, wave 5 (2026-07-29).** A held-out measurement scanned five pinned third-party MCP servers (notion-mcp-server, mcp-server-neon, mcp-server-qdrant, firecrawl-mcp-server, airtable-mcp-server) and scored **0 true positives / 58 findings**. All 58 came from the four JS/TS targets; the one Python target returned a clean bill. Root cause: `reachability.py` and `taint.py` both return UNKNOWN unless the file is `.py`/`.pyw`, so **every JS/TS finding was raw regex output with no precision layer** -- and the report rendered it identically to a call-graph-proven Python finding. Same severity badge, same confidence, no visible difference. That is a REPORTING defect as much as an analysis one, and it is the half fixed here. **This wave does not add a JS/TS call-graph** -- it makes the output honest about not having one. - **The `grade` axis (`grading.py`, `models.Grade`).** A finding whose reachability AND taint both came back UNKNOWN is labelled `UNGRADED` with a stated, specific reason. The label is deliberately outcome-based and language-agnostic so it cannot go stale as surfaces are added. Surfaced in every renderer: a badge and a "Not graded because" line in the terse markdown, a **Graded?** column plus legend in the client report, a `grade`/`grade_reason` field in the JSON, a badge in the generated MD/HTML report, and an ungraded count on the summary line. Scan JSON written before the axis existed defaults to `graded` and is never retro-labelled. - **Per-run grading coverage.** `ScanResult.coverage` records how many files the precision passes could and could not analyse, broken down by reason, and every markdown renderer prints it. A **clean bill over an ungradable surface is qualified in the same callout** -- the mirror-image failure is a false assurance, and burying that caveat lower in the report is how it happens. - **A scoped severity cap.** An ungraded finding in a DATAFLOW class (shell-injection, code-eval, unsafe-deserialization, ssrf, path-traversal -- imported from `taint` by identity, not copied) whose FILE could not be analysed at all is capped at P2/LOW. For those classes the severity ladder is itself a dataflow claim ("P0 = exploitable now" asserts caller-controlled data reaches the sink), so without a dataflow pass P0 is unearned. **The cap keys on whether the file was ANALYSABLE, never on whether an answer was reached** -- reachability and taint also return UNKNOWN when they ran and deliberately abstained (module-level code, dynamic dispatch, un-rooted low-level dispatcher, no tool roots), and the round-3 contract requires those to leave confidence untouched. The cap is **out of scope for every non-dataflow class**: a committed AWS key is a P1 whether or not a tool reaches it, so capping `hardcoded-secret` would be dishonest in the other direction. Both directions pinned. - **`redis.eval()` is not JavaScript `eval()`.** The only four P0s in the whole sample were `redis.eval(LUA_SCRIPT, {keys, arguments})` in neon's `mcp/oauth/refresh-lock.ts` -- correctly parameterized Redis server-side Lua. `\beval\s*\(` matched after a `.`; now `(?=2 segments) is offered to `_compose_demotion` as a **path co-signal** -- so it demotes only PAIRED with a test path, never alone, and a real high-entropy value under `tests/` still keeps HIGH. Applied uniformly to all seven value patterns rather than special-cased; inert for the other six by construction. - **The redaction is now honest.** The measurement's stated aggravating factor was that these were redacted to a bare ``, so a reader could not tell a fixture from a live credential without opening the repo. The snippet now carries the demotion tags -- ``. The value itself is still never printed. - **A logged member chain is judged by its TERMINAL segment.** `logger.info('OAuth token found', { clientId: token.client.id })` logs a public client id; it fired because the chain's ROOT (`token`) matched. Now `token.client.id` is judged on `id`. One curated recovery: a GENERIC VALUE ACCESSOR terminal (`value`/`raw`/`plain`/...) carries no information, so the parent is judged instead, keeping `secret.value` and `token.raw` firing. `id` is deliberately not in that set. **Applied to the Python AST path and the JS line-based path in the same commit, sharing one decision helper** -- the identical bug existed in both, and a carve-out present on one surface and not the other is worse than none. - **`rm -rf` on curated build artifacts demotes to P3.** airtable's `build-mcpb.sh` deleting `node_modules` and its own `.mcpb` output is a build step, not an irreversible-ops hazard. Requires EVERY target on the line to be a curated exact-match name or archive suffix; fails closed on a variable, a glob, an absolute or home-relative path, or one unrecognised entry among several (`rm -rf node_modules /etc/nginx` keeps P1). Demoted and tagged, never dropped. - **Client-report framing.** Graded findings now lead "Top 3 to fix" within a severity tier (severity still dominates), the executive summary states the ungraded share, and the "each critical includes a reproducible proof" claim is only made when criticals exist. - **Explicitly DECLINED, and why.** (a) **Detecting an adjacent guard** -- neon's `validateDocSlug()`, firecrawl's `encodeURIComponent()`, notion's `redactToken()` all defend the flagged line, and recognising that requires real JS/TS dataflow, i.e. the JS call-graph this wave deliberately does not build. The `UNGRADED` reason string names this limitation verbatim ("a validator or sanitiser adjacent to this line would NOT have been seen"). (b) **A redaction-wrapper suppressor** -- a wrapper NAME is target-controlled (a hostile repo can name a passthrough `redactToken`), so per the one law it is surfaced as report context, never a silencer. (c) **`|| true` in a build script** (`build-mcpb.sh:19`) -- masking an exit status is a genuine, if minor, reliability finding and job-hazards exists for exactly that surface; kept at P2. - **Re-measured on the same five pinned commits: 58 -> 53 findings, 4 P0 -> 0 P0, 42 of 53 now carry a real grade.** **Honest bottom line: true positives are still 0/53.** A smaller denominator is not a capability gain, and this wave is not presented as one -- what changed is that a reader can now see which findings rest on evidence and which are pattern matches, and the four P0s that would have led an outreach email are gone. - **Known perf bound, disclosed not fixed this round:** `_sibling_self_signed_cert`'s directory scan (the paired-key demotion check) is O(n) per key file in a directory with n cert/key files, making a directory with many pairs O(n^2) overall -- measured ~4.15s for 100 pairs in one directory, ~33.3s for 400. Cert parsing itself is memoized (`_parse_x509_cert_bytes_cached`), so this is redirectory-iteration overhead, not repeated crypto work; a real-world repo with hundreds of PEM pairs in a single directory is an unusual shape, but the bound is real and worth stating rather than silently living with. Not fixed this round (no functional-correctness impact). - Every demotion above has a paired regression test proving a REAL secret/exec in the same shape still flags -- including every adversarial repro named above (shadowed-scope RegExp both directions, a pragma-commented real secret, a self-signed cert with a prod-shaped identity, a JWT value under a demoted name, a keyword-preceded regex literal masking a real sink, an unbalanced-brace file, a pagination-named assignment through the AST branch, and every round-3 "must demote, never disappear" case) -- see `tests/test_secret_pagination_fp.py`, `tests/test_secret_selfsigned_cert_fp.py`, `tests/test_secret_placeholder_fp.py`, `tests/test_secret_placeholder_regex_ast_branch_fix.py`, `tests/test_param_injection_regexp_exec_fp.py`, `tests/test_param_injection_regexp_scope_fp.py`, `tests/test_round3_onelaw_fixes.py`. ## Tests ```bash #### python -m pytest -q # 604 tests (595 passing, 9 self-audit skip without the env var below); the no-crypto figure previously carried here (403 passing / 11 skipped) is STALE as of the 2026-07-29 grading-honesty wave and has NOT been re-measured -- treat it as unknown until someone runs a genuinely clean venv without cryptography, rather than trusting the carried-forward number: per-detector vuln/clean fixtures (Python + JS/TS parity across .js/.mjs/.cjs/.ts/.mts/.cts/.jsx/.tsx) + the reachability-grading matrix (incl. the cli-only/uncalled decidable-reachability grades + the low-level MCP SDK Server()/list_tools/call_tool discovery shape, per-module-scoped and import-provenance-gated so a repo with more than one dispatcher, or a same-named non-MCP class, can't claim a bogus root, and an un-rooted low-level tool -- split declaration/dispatch modules, or a genuinely ambiguous multi-dispatcher file -- withholds CLI_ONLY/UNCALLED in favor of UNKNOWN the same way unresolvable dynamic dispatch does) + detector 5 (tool-scope-creep) and detector 6 (secret-leak-via-tool-response) low-level-SDK dispatch-branch attribution (2026-07-23: `tool_registry.dispatch_segments`, shared by both detectors) + the tool-parameter taint-tracking matrix (intra-file + cross-file, up to two hops) + the self-audit proof (now guarding 6 fleet servers directly, 8 total via FLEET_SERVERS) + client-report renderer + the CI README count-verification gate's own unit tests + wave-1 FP-class regression fixtures (pagination-cursor names, self-signed test certs, known-placeholder secrets, RegExp-vs-child_process .exec() receiver resolution) + the test-path confidence demotion (wave-4: pair-only, never demotes a real value-shaped secret on a bare path) + the `mcp-scan report` client-report generator (2026-07-23: stable line-independent finding_id + collision suffixes, scan_meta embedding, triage.toml verdict joins incl. the unknown-id loud-warning path, byte-stable golden HTML/MD renders, the zero-external-URL self-containment gate, and the no-hardcoded-counts template AST-grep gate) + the `mcp-scan ecosystem-scan` repeatable v2 pipeline (2026-07-23: batch-scan a fleet of MCP-server repos read-only -- mtime/bytes unchanged on every target, clone path injected/mocked so the suite makes zero real network calls, above-LOW findings gated fail-closed as disclosure candidates, PRIVATE-marked disclosure notes that surface only the target's own SECURITY.md and never invent a contact channel or auto-publish, anonymized aggregate + notes staged to gitignored local dirs, and a runtime-unique sentinel in the leak test so no fixture string can coincidentally match real output) + the destructive-action confirm-gate detector's FP-wave2 hardening (2026-07-23: recognizes a real in-body control-flow confirmation gate -- a negated force/yes/confirm/proceed param bound to an actual throw/exit/raise -- as equivalent to the SDK's -Confirm/--dry-run flag, conservative-by-design so a bare param reference alone never suppresses; an adjudication pass then removed an overly loose bare-phrase alternation with zero binding to any real gate; and the destructiveHint annotation doctrine -- a target's self-declared destructiveHint:true is recorded as context only and never suppresses or downgrades a genuine finding) CI (`.github/workflows/ci.yml`) runs this suite on every push/PR and fails the build if this claimed count drifts from what the suite actually reports -- see `scripts/check_readme_counts.py`. The self-audit tests (9 of the 604) require `MCP_SCANNER_FLEET_ROOT` to be set and pointed at real MCP server repos to scan; they skip cleanly if it's unset (e.g. in a fresh clone or CI on another machine). See [ANNOUNCEMENT.md](ANNOUNCEMENT.md) for the reproducible self-audit output. Each detector ships a matched pair of fixtures — a vulnerable one it must catch, a clean one it must not flag — so the false-positive floor is a tested invariant, not a hope.

标签：GraphQL安全矩阵, MCP, 代码安全, 安全扫描器, 漏洞枚举, 聊天机器人, 逆向工具, 错误基检测, 静态代码分析