fix: harden mcp baseline scanner against false negatives and crashes#155
Merged
Conversation
The MCP pre-flight scanners had four latent security/robustness bugs (pre-existing;
surfaced by review of the phase-pipeline extraction):
1. scanResources used the in-band sentinel `content.startsWith("ERROR: ")` to detect
a read failure, which collides with a resource whose legitimate content starts
with "ERROR: " — that resource was recorded as a read error and skipped from the
secret/PII judge (a silent false negative). McpTarget.readResource now throws on
failure instead of returning an "ERROR: …" string, and scanResources try/catches.
Blast radius is contained: the only caller of the wrapper is baselineScanner
(run/scanResources.ts uses the raw mcp client).
2. scanRugPull treated a zero-byte baseline as a clean "first run" (`!baselineJson`),
silently re-recording and missing a real rug-pull drift.
3. The baseline was parsed with an unguarded `JSON.parse(...) as Array<…>` — a corrupt
file threw uncaught and aborted the whole run with no report.
4. On detecting drift, the code overwrote the trusted baseline with the changed
snapshot, so a second run PASSed silently — the rug pull laundered itself into the
baseline (reported by CodeRabbit).
Fixes 2–4 fail closed: a new parseBaselineSnapshot() returns null for empty/corrupt/
non-array baselines and the caller reports ERROR without touching the file; detected
drift no longer overwrites the baseline, so every subsequent run keeps flagging FAIL
until an operator explicitly re-baselines (deletes the file). A genuine first run
(no file) still records and passes.
Adds 6 tests: drift stays FAIL with the baseline byte-for-byte unchanged across runs,
empty/corrupt baselines yield ERROR (no crash), no-drift passes, and both resource
read paths (legit "ERROR: " content vs a thrown read failure).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
👮 Files not reviewed due to content moderation or server errors (3)
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@core/src/execute/baselineScanner.ts`:
- Around line 252-257: The first-run path in baselineScanner’s baseline handling
is too broad because baselineJson === null also includes non-ENOENT read
failures from the earlier baseline read. Update the control flow in
baselineScanner so baselineReadError is checked before the “no baseline” branch,
and only treat ENOENT as a genuine missing baseline that calls recordBaseline()
and passes; for any other read error, return ERROR and do not modify state.
- Around line 358-376: The baseline parser currently only checks that the
snapshot is an array, so invalid items like null can still pass through and
later break computeToolDiffs when it reads t.name. Update parseBaselineSnapshot
to validate every array element against the expected tool shape using Zod, and
return null unless the entire parsed snapshot is an array of valid entries. Keep
the existing fail-closed behavior for empty, malformed, or partially corrupt
snapshots.
In `@core/src/targets/mcpTarget.ts`:
- Around line 114-117: The thrown read failure in mcpTarget’s read path is too
generic because `msg.slice(0, 300)` can omit the resource context and
remediation. Update the error construction near the `throw new Error(...)` in
`mcpTarget` to include the resource URI and a clear next action for the operator
before it reaches `toolError`, while still preserving the original cause via the
`cause` option. Keep the change localized to the read-failure handling logic so
callers of the resource-read flow get an actionable message.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 93e7c0ac-2749-43ba-994a-4ae1037d1f8c
📒 Files selected for processing (3)
core/src/execute/baselineScanner.tscore/src/targets/mcpTarget.tscore/tests/baselineScanner.test.ts
The rug-pull scan's readFile catch swallowed every error into baselineJson === null, so a permission/I-O failure on an existing baseline was treated as a first run — it re-recorded a fresh baseline and passed, masking drift and clobbering the unreadable baseline. Capture the error code: only ENOENT records-and-passes; any other read failure fails closed with an ERROR and leaves the file untouched. Adds a test using a directory at the baseline path (readFile → EISDIR) to prove the non-ENOENT path yields ERROR without recording. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- validate every baseline entry with a Zod schema (reject [null] etc.) - treat a formatting-only baseline diff as no-drift, not a sticky FAIL - fail closed with ERROR on baseline write failure instead of crashing - bound the untrusted resource URI in read-error messages - make the unreadable-baseline reasoning actionable Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Collaborator
|
Follow-up changes on top of the hardening fixes:
|
jithin23-kv
approved these changes
Jul 2, 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
2 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.
What & why
The MCP pre-flight scanners had four latent security/robustness bugs — pre-existing,
surfaced while reviewing the phase-pipeline extraction (#154), plus one reported by
CodeRabbit. Fixing them all together with tests.
scanResourcesusedcontent.startsWith("ERROR: ")to detect a read failureERROR:was recorded as an error and skipped from the secret/PII judge — a silent false negativeMcpTarget.readResourcenow throws on failure (out-of-band);scanResourcestry/catches. No in-band sentinel, no collision.!baselineJson→ treated as "first run"JSON.parse(...) as Array<…>of the baselineparseBaselineSnapshot()guards it → ERROR, no crashDesign: fail closed
Fixes 2–4 fail closed — an untrusted state (corrupt baseline, or a detected drift)
is never silently promoted to the trusted baseline, which would let a
corrupt-then-drift sequence launder itself (the same class as #4). A genuine first run
(no baseline file) still records and passes.
Backward compatibility
McpTarget.readResourcechanges contract from "returnERROR: …on failure" to"reject (throw) on failure". Blast radius is contained: the only caller of the
wrapper is
baselineScanner(+ test fakes);run/scanResources.tsuses the raw MCPclient, not the wrapper. The interface JSDoc documents the new contract.
Tests (+6)
still FAIL (the CodeRabbit fix).
ERROR:content is judged (not misclassified); a thrownread failure surfaces as an ERROR result.
Verification
full suite 127 tests, 0 fail (3 skips) · typecheck 0 · lint 0 · prettier clean · build OK.
🤖 Generated with Claude Code
Summary by CodeRabbit