D-129: mcp.json FP sweep clean, core.hooksPath FP found and accepted - #105
Merged
Conversation
…accepted Follow-up to D-128's disclosed mcp.json residual: swept ten real, actively-maintained MCP-ecosystem npm packages' published lifecycle scripts against the marker. None reference mcp.json in any of the seven scanned hook names; the one real tool that legitimately writes mcp.json-shaped config (add-mcp) is invoked directly via npx and ships no lifecycle scripts, so it's never in a position to be scanned. No code change; the residual is now on stronger empirical footing. The same sweep surfaced a real false positive on a different, pre-existing marker: @smithery/cli's published prepare script runs `git config core.hooksPath .githooks`, which trips VC-002g. Confirmed directly against the scanner. Investigated narrowing core.hooksPath to close it and decided against both options: narrowing by target value is defeated on inspection (an attacker's bundled hooks directory would use the identical relative-path, conventional-name shape specifically to blend in - there's no structural difference from the benign case, only an unobservable difference in intent behind the target path), and narrowing by excluding the prepare hook type would blind the actual attack vector prepare's inclusion exists to catch (malware shipped via a git-URL dependency to dodge npm registry publish-time scanning). Left as-is; recorded as an investigated, confirmed, accepted FP so a future sweep doesn't re-litigate the same tradeoff from scratch. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PLd1shywzWPsLgkpLxEyPj
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
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.
Follow-up to D-128's disclosed
mcp.jsonresidual (a raw-substring, non-word-bounded marker with theoretical, not-yet-observed FP risk).mcp.json: swept, no FP found
Checked the published
package.json"scripts" of ten real, actively-maintained MCP-ecosystem npm packages (@modelcontextprotocol/server-filesystem,chrome-devtools-mcp,@notionhq/notion-mcp-server,@playwright/mcp,mongodb-mcp-server,@sentry/mcp-server,@cloudflare/mcp-server-cloudflare,@upstash/context7-mcp,add-mcp,@smithery/cli) via the npm registry API. None referencemcp.jsonin any of the seven scriptsinstallsurface.Analyzeactually scans (InstallHookNames) — most define no lifecycle scripts at all. The one real tool that legitimately writesmcp.json-shaped config (add-mcp, vianpx add-mcp) is invoked directly by end users, ships no lifecycle scripts of its own, and so is never in a position to be scanned by this marker regardless.No code change — the residual stays disclosed as theoretical, now on stronger empirical footing.
A real FP found instead, for a different, pre-existing marker
@smithery/cli's publishedpreparescript (a genuinely popular MCP marketplace/installer CLI) is:Confirmed directly against the scanner:
scanCapson that exact string returnsevidence: [core.hooksPath], andIsPersistenceMarker("core.hooksPath")is true — this tripsVC-002g(high/gate-eligible) on any project depending on@smithery/cli.core.hooksPathpredates this work (OPU-19/27); unrelated tomcp.json, but surfaced during the same sweep.Investigated narrowing the marker — decided against it
Two narrowing options, both rejected:
core.hooksPathexists specifically because the redirect target is attacker-controllable — that's the entire threat model. A malicious install hook hijacking a consumer's git hooks would use exactly the same shape (plain relative path, conventional-sounding name, stderr suppression) specifically to blend in. There's no textual feature separating "my own repo's dev-hooks setup" from "attacker's bundled hooks directory given a benign-looking name" — the benign and malicious cases are the same string shape with a different, unobservable intent, not two structurally different shapes the way OPU-34's named-function filter had.preparefrom this marker): this is the one that would actually open a hole.preparewas deliberately included because it's the hook npm runs for a git-URL or local-path dependency — exactly the vector attackers use to ship malware while dodging npm registry publish-time scanning. Excluding it would blind that specific attack in exchange for quieting a case that's benign only because of npm install semantics the static scanner cannot observe under D-04 (preparedoesn't run for a normal registry-tarball consumer install — a runtime fact, not a textual one).This is the same tradeoff the codebase already made once, in reverse: bare
huskywas excluded frompersistenceMarkersin OPU-19 because its payload is fixed and well-known (high frequency, low danger).core.hooksPathwas kept explicit for the opposite reason — arbitrary target, so low frequency but high danger when real. Narrowing it to match husky's treatment would erase exactly the property that makes it worth having.Decision
Leave
core.hooksPathas-is; record this as an investigated, confirmed, and accepted false positive (D-129). No code change. The operational cost is bounded —VC-002gis gate-eligible, not block-tier, so this surfaces as a reviewable line item under-fail-on-eligible, never a blocked build.Files
docs/DECISIONS.md— D-129 (documentation only, no code changes).Generated by Claude Code