From eaa6c37b91125339728ccc2883a15a45ad45cfdd Mon Sep 17 00:00:00 2001 From: Prasanth Date: Thu, 2 Jul 2026 16:10:10 +0530 Subject: [PATCH 1/3] fix: harden mcp baseline scanner against false negatives and crashes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- core/src/execute/baselineScanner.ts | 88 ++++++++-- core/src/targets/mcpTarget.ts | 6 +- core/tests/baselineScanner.test.ts | 249 ++++++++++++++++++++++++---- 3 files changed, 290 insertions(+), 53 deletions(-) diff --git a/core/src/execute/baselineScanner.ts b/core/src/execute/baselineScanner.ts index 8c2929ec..9a404bfb 100644 --- a/core/src/execute/baselineScanner.ts +++ b/core/src/execute/baselineScanner.ts @@ -107,10 +107,11 @@ async function scanResources(ctx: BaselineScanContext): Promise notify({ type: "attack_start", attackId, patternName: `resource: ${resource.uri}` }); log.info(` → resource: ${resource.uri}`); - const content = await target.readResource(resource.uri); - const isError = content.startsWith("ERROR: "); - - if (isError) { + let content: string; + try { + content = await target.readResource(resource.uri); + } catch (err) { + const message = err instanceof Error ? err.message : String(err); results.push({ kind: "mcp", attackId, @@ -119,8 +120,8 @@ async function scanResources(ctx: BaselineScanContext): Promise toolName: "resources/read", toolArguments: { uri: resource.uri }, toolResponse: "", - toolError: content, - judge: mcpErrorJudge(content), + toolError: message, + judge: mcpErrorJudge(message), }); notify({ type: "attack_done", attackId, verdict: "ERROR" }); continue; @@ -238,14 +239,22 @@ async function scanRugPull(ctx: BaselineScanContext): Promise { /* no baseline yet */ } + const recordBaseline = async () => { + await mkdir(baselinesDir, { recursive: true }); + await writeFile(baselinePath, currentJson, "utf8"); + }; + + // Parse any stored baseline up front: null means no file, empty, or corrupt. + const baseline = baselineJson === null ? null : parseBaselineSnapshot(baselineJson); + let result: AttackResult; - if (!baselineJson) { + if (baselineJson === null) { + // Genuine first run — no prior state to protect, so record and pass. log.info( ` No baseline found — recording current state (${tools.length} tools, hash: ${currentHash.slice(0, 12)}…)` ); - await mkdir(baselinesDir, { recursive: true }); - await writeFile(baselinePath, currentJson, "utf8"); + await recordBaseline(); result = { kind: "mcp", attackId, @@ -262,6 +271,31 @@ async function scanRugPull(ctx: BaselineScanContext): Promise { reasoning: `First run — baseline recorded. No previous state to compare against.`, }, }; + } else if (baseline === null) { + // Baseline file exists but is empty or unparseable. We cannot verify drift, + // and silently re-recording would let a corrupt-then-drift sequence launder + // itself into a trusted baseline. Fail closed: flag it and leave the file + // untouched so an operator re-baselines explicitly (delete it to re-record). + log.warn(` Baseline file is empty or unparseable — cannot verify drift`); + result = { + kind: "mcp", + attackId, + evaluatorId: evalId, + patternName: "tool-description-drift", + toolName: "tools/list", + toolArguments: {}, + toolResponse: "", + toolError: `Baseline file ${baselinePath} is empty or corrupt`, + judge: { + verdict: "ERROR", + score: 0, + confidence: 0, + evidence: "N/A", + reasoning: + "Stored baseline is empty or unparseable — tool-description drift could not be verified. Delete the baseline file to record a fresh one.", + errorMessage: "corrupt or empty baseline", + }, + }; } else { const baselineHash = createHash("sha256").update(baselineJson).digest("hex"); if (currentHash === baselineHash) { @@ -283,17 +317,16 @@ async function scanRugPull(ctx: BaselineScanContext): Promise { }, }; } else { - const baselineSnapshot = JSON.parse(baselineJson) as Array<{ - name: string; - description: string; - inputSchema: unknown; - }>; - const diffs = computeToolDiffs(baselineSnapshot, currentSnapshot); + // DRIFT — do NOT overwrite the trusted baseline. Auto-accepting the changed + // snapshot would let the next run PASS silently (the rug pull launders + // itself). Keep flagging every run until an operator re-baselines. + const diffs = computeToolDiffs(baseline, currentSnapshot); const diffSummary = diffs.join("\n"); log.info(` ✗ DRIFT DETECTED — ${diffs.length} change(s)`); for (const d of diffs) log.info(` ${d}`); - await mkdir(baselinesDir, { recursive: true }); - await writeFile(baselinePath, currentJson, "utf8"); + log.warn( + ` Baseline NOT updated — re-run keeps flagging until you re-baseline (delete ${baselinePath} to accept).` + ); result = { kind: "mcp", attackId, @@ -310,7 +343,7 @@ async function scanRugPull(ctx: BaselineScanContext): Promise { score: 1, confidence: 100, evidence: diffSummary.slice(0, 500), - reasoning: `Tool descriptions changed since baseline: ${diffs.length} difference(s) detected. Baseline updated.`, + reasoning: `Tool descriptions changed since baseline: ${diffs.length} difference(s) detected. Baseline left unchanged — delete it to accept the new state.`, }, }; } @@ -322,6 +355,25 @@ async function scanRugPull(ctx: BaselineScanContext): Promise { return [result]; } +/** + * Parse a stored tool-snapshot baseline. Returns null when the file is empty or + * not a valid JSON array (corrupt), so the caller can fail closed instead of + * crashing — a bare `JSON.parse` on a truncated file throws and would abort the + * entire run with no report. + */ +function parseBaselineSnapshot( + json: string +): Array<{ name: string; description: string; inputSchema: unknown }> | null { + if (!json.trim()) return null; + try { + const parsed: unknown = JSON.parse(json); + if (!Array.isArray(parsed)) return null; + return parsed as Array<{ name: string; description: string; inputSchema: unknown }>; + } catch { + return null; + } +} + function computeToolDiffs( baseline: Array<{ name: string; description: string; inputSchema: unknown }>, current: Array<{ name: string; description: string; inputSchema: unknown }> diff --git a/core/src/targets/mcpTarget.ts b/core/src/targets/mcpTarget.ts index 53b4741f..677af467 100644 --- a/core/src/targets/mcpTarget.ts +++ b/core/src/targets/mcpTarget.ts @@ -12,6 +12,7 @@ export interface McpTarget { callTool(name: string, args: Record): Promise; listTools(): Promise; listResources(): Promise; + /** Resolve with the resource's text content; reject (throw) on a read failure. */ readResource(uri: string): Promise; close(): Promise; } @@ -110,7 +111,10 @@ class ConnectedMcpTarget implements McpTarget { return parts.join("\n").trim(); } catch (err: unknown) { const msg = err instanceof Error ? err.message : String(err); - return `ERROR: ${msg.slice(0, 300)}`; + // Signal read failures out-of-band (throw) rather than returning an + // "ERROR: …" string — that in-band sentinel collides with a resource whose + // legitimate content happens to start with "ERROR: ". Callers try/catch. + throw new Error(msg.slice(0, 300), { cause: err }); } } diff --git a/core/tests/baselineScanner.test.ts b/core/tests/baselineScanner.test.ts index a133c3c9..cf91844d 100644 --- a/core/tests/baselineScanner.test.ts +++ b/core/tests/baselineScanner.test.ts @@ -1,16 +1,25 @@ /** - * PR12 — BaselineScanner chain. + * BaselineScanner chain + hardening. * - * The MCP pre-flight scans had no test coverage before extraction. This pins the - * chain wiring: with an empty target (no resources, no tools) the resource and - * tool-description scanners contribute nothing (dropped), and the rug-pull scanner - * records a first-run baseline — so exactly one EvaluatorResult comes back, and no - * judge LLM is called. + * Pins the chain wiring (an empty target yields only the rug-pull baseline) and + * the four security/robustness fixes: + * - rug-pull drift must NOT auto-accept the changed snapshot as the new baseline + * (else a second run silently PASSes — the rug pull launders itself); + * - an empty baseline file must not be treated as a clean "first run"; + * - a corrupt baseline must not crash the whole run; + * - a resource whose content starts with "ERROR: " must not be misclassified as a + * read failure (the old in-band sentinel), and a genuine read failure (throw) + * must surface as an ERROR result. + * + * The rug-pull cases are LLM-free (pure hashing); the resource cases use a local + * fake judge server. console is silenced so the scan's log burst can't race the + * node:test worker's result channel. */ -import { test } from "node:test"; +import { test, before, after } from "node:test"; import assert from "node:assert/strict"; +import { createServer, type Server } from "node:http"; import { tmpdir } from "node:os"; -import { mkdtemp, rm } from "node:fs/promises"; +import { mkdtemp, mkdir, writeFile, readFile, rm } from "node:fs/promises"; import path from "node:path"; import { setEnvProvider } from "../src/lib/env.js"; @@ -18,7 +27,64 @@ setEnvProvider(() => "fake-test-api-key"); const { runBaselineScans } = await import("../src/execute/baselineScanner.js"); -function emptyTarget() { +let server: Server; +let port: number; +let origLog: typeof console.log; + +function chatCompletion(content: string): string { + return JSON.stringify({ + id: "t", + object: "chat.completion", + created: 0, + model: "m", + choices: [{ index: 0, message: { role: "assistant", content }, finish_reason: "stop" }], + usage: { prompt_tokens: 1, completion_tokens: 1, total_tokens: 2 }, + }); +} + +// The judge always returns PASS here; the resource tests only care that the judge +// was reached at all (not misclassified as a read error). +function judgePass(): string { + return chatCompletion( + JSON.stringify({ + verdict: "PASS", + score: 10, + confidence: 90, + evidence: "N/A", + reasoning: "safe", + }) + ); +} + +before(async () => { + origLog = console.log; + console.log = () => {}; + port = await new Promise((resolve) => { + server = createServer((req, res) => { + let body = ""; + req.on("data", (c: Buffer) => (body += c.toString())); + req.on("end", () => { + if ((req.url ?? "").startsWith("/v1/chat/completions")) { + res.writeHead(200, { "Content-Type": "application/json" }); + res.end(judgePass()); + return; + } + res.writeHead(404); + res.end("no"); + }); + }); + server.listen(0, "127.0.0.1", () => resolve((server.address() as { port: number }).port)); + }); +}); + +after(async () => { + console.log = origLog; + await new Promise((resolve, reject) => server.close((e) => (e ? reject(e) : resolve()))); +}); + +// --- helpers --------------------------------------------------------------- + +function emptyTarget(over: Record = {}) { return { async callTool() { return { response: "" }; @@ -33,38 +99,153 @@ function emptyTarget() { return ""; }, async close() {}, + ...over, }; } +async function tmp() { + return mkdtemp(path.join(tmpdir(), "opfor-baseline-")); +} + +function baselinePath(outputDir: string) { + return path.join(outputDir, "baselines", "test-srv-tools.json"); +} + +async function seedBaseline(outputDir: string, content: string) { + const p = baselinePath(outputDir); + await mkdir(path.dirname(p), { recursive: true }); + await writeFile(p, content, "utf8"); +} + +async function scan(outputDir: string, { target = emptyTarget(), tools = [] as unknown[] } = {}) { + return runBaselineScans({ + target: target as never, + tools: tools as never, + judgeModelConfig: { + provider: "openai-compatible", + model: "m", + apiKeyEnv: "K", + baseURL: `http://127.0.0.1:${port}/v1`, + }, + config: { target: { kind: "mcp", name: "test-srv", transport: "stdio" } } as never, + outputDir, + notify: () => {}, + }); +} + +const rugPull = (results: Awaited>) => + results.find((r) => r.evaluatorId === "rug-pull-detection")!; +const resourceScan = (results: Awaited>) => + results.find((r) => r.evaluatorId === "resource-exposure")!; + +// --- chain wiring ---------------------------------------------------------- + test("empty target: only the rug-pull baseline scan contributes a result", async () => { - const outputDir = await mkdtemp(path.join(tmpdir(), "opfor-baseline-")); - // The scan logs a burst of progress lines; silence console so the volume can't - // race the node:test worker's result channel. - const origLog = console.log; - console.log = () => {}; + const dir = await tmp(); try { - const results = await runBaselineScans({ - target: emptyTarget(), - tools: [], - judgeModelConfig: { - provider: "openai-compatible", - model: "m", - apiKeyEnv: "K", - baseURL: "http://127.0.0.1:1/v1", - }, - config: { target: { kind: "mcp", name: "test-srv", transport: "stdio" } } as never, - outputDir, - notify: () => {}, + const results = await scan(dir); + assert.strictEqual(results.length, 1); + assert.strictEqual(rugPull(results).attacks[0].judge.verdict, "PASS"); + assert.match(rugPull(results).attacks[0].judge.reasoning, /baseline recorded/i); + assert.strictEqual(await readFile(baselinePath(dir), "utf8"), "[]"); + } finally { + await rm(dir, { recursive: true, force: true }); + } +}); + +// --- rug-pull hardening ---------------------------------------------------- + +test("detected drift does NOT overwrite the baseline, and stays FAIL on re-run", async () => { + const dir = await tmp(); + try { + // Seed a baseline that differs from the current (empty) tool set → drift. + const original = JSON.stringify( + [{ name: "old-tool", description: "does a thing", inputSchema: null }], + null, + 2 + ); + await seedBaseline(dir, original); + + const first = await scan(dir); + assert.strictEqual(rugPull(first).attacks[0].judge.verdict, "FAIL"); + // The trusted baseline must be untouched — not auto-accepted as the new state. + assert.strictEqual(await readFile(baselinePath(dir), "utf8"), original); + + // A second run keeps flagging it (sticky) instead of silently passing. + const second = await scan(dir); + assert.strictEqual(rugPull(second).attacks[0].judge.verdict, "FAIL"); + } finally { + await rm(dir, { recursive: true, force: true }); + } +}); + +test("empty baseline file is not treated as a clean first run", async () => { + const dir = await tmp(); + try { + await seedBaseline(dir, ""); + const results = await scan(dir); + assert.strictEqual(rugPull(results).attacks[0].judge.verdict, "ERROR"); + // Fail closed: the empty file is left as-is (not silently re-recorded). + assert.strictEqual(await readFile(baselinePath(dir), "utf8"), ""); + } finally { + await rm(dir, { recursive: true, force: true }); + } +}); + +test("corrupt baseline file yields ERROR instead of crashing the run", async () => { + const dir = await tmp(); + try { + await seedBaseline(dir, "{ this is not valid json"); + const results = await scan(dir); // must not throw + assert.strictEqual(rugPull(results).attacks[0].judge.verdict, "ERROR"); + } finally { + await rm(dir, { recursive: true, force: true }); + } +}); + +test("matching baseline passes with no drift", async () => { + const dir = await tmp(); + try { + await seedBaseline(dir, "[]"); // matches the empty current snapshot + const results = await scan(dir); + assert.strictEqual(rugPull(results).attacks[0].judge.verdict, "PASS"); + assert.match(rugPull(results).attacks[0].judge.reasoning, /no drift/i); + } finally { + await rm(dir, { recursive: true, force: true }); + } +}); + +// --- resource read hardening ----------------------------------------------- + +test('resource content starting with "ERROR: " is judged, not misclassified', async () => { + const dir = await tmp(); + try { + const target = emptyTarget({ + listResources: async () => [{ uri: "res://log", name: "log" }], + readResource: async () => "ERROR: 500 — legitimate diagnostic log content", }); + const results = await scan(dir, { target }); + // Reached the judge (canned PASS) rather than being flagged as a read error. + assert.strictEqual(resourceScan(results).attacks[0].judge.verdict, "PASS"); + } finally { + await rm(dir, { recursive: true, force: true }); + } +}); - // resource-exposure (no resources) and tool-description (no tools) drop out; - // rug-pull records a first-run baseline. - assert.strictEqual(results.length, 1); - assert.strictEqual(results[0].evaluatorId, "rug-pull-detection"); - assert.strictEqual(results[0].attacks[0].judge.verdict, "PASS"); - assert.match(results[0].attacks[0].judge.reasoning, /baseline recorded/i); +test("a genuine resource read failure (throw) surfaces as an ERROR result", async () => { + const dir = await tmp(); + try { + const target = emptyTarget({ + listResources: async () => [{ uri: "res://x", name: "x" }], + readResource: async () => { + throw new Error("connection reset"); + }, + }); + const results = await scan(dir, { target }); + const attack = resourceScan(results).attacks[0]; + assert.strictEqual(attack.judge.verdict, "ERROR"); + assert.match(attack.toolError ?? "", /connection reset/); } finally { - console.log = origLog; - await rm(outputDir, { recursive: true, force: true }); + await rm(dir, { recursive: true, force: true }); } }); From 46878886fe21a24d215ced187b4a2856ff73bedd Mon Sep 17 00:00:00 2001 From: Prasanth Date: Thu, 2 Jul 2026 16:30:35 +0530 Subject: [PATCH 2/3] fix: only treat a genuinely missing baseline as a first run MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- core/src/execute/baselineScanner.ts | 36 +++++++++++++++++++++++++---- core/tests/baselineScanner.test.ts | 13 +++++++++++ 2 files changed, 45 insertions(+), 4 deletions(-) diff --git a/core/src/execute/baselineScanner.ts b/core/src/execute/baselineScanner.ts index 9a404bfb..914ac445 100644 --- a/core/src/execute/baselineScanner.ts +++ b/core/src/execute/baselineScanner.ts @@ -233,10 +233,15 @@ async function scanRugPull(ctx: BaselineScanContext): Promise { const baselinePath = path.join(baselinesDir, `${safeSlug}-tools.json`); let baselineJson: string | null = null; + let readError: NodeJS.ErrnoException | null = null; try { baselineJson = await readFile(baselinePath, "utf8"); - } catch { - /* no baseline yet */ + } catch (err) { + // Only a genuinely missing file (ENOENT) is a first run. Any other read + // failure — permission, I/O, a directory in the way — means an existing + // baseline could not be read, so we must not silently re-record and pass. + const e = err as NodeJS.ErrnoException; + if (e.code !== "ENOENT") readError = e; } const recordBaseline = async () => { @@ -249,8 +254,31 @@ async function scanRugPull(ctx: BaselineScanContext): Promise { let result: AttackResult; - if (baselineJson === null) { - // Genuine first run — no prior state to protect, so record and pass. + if (readError) { + // An existing baseline could not be read (permission / I/O). Fail closed: + // report ERROR and leave the file untouched rather than masking drift. + log.warn(` Baseline could not be read (${readError.code ?? "unknown"}) — cannot verify drift`); + result = { + kind: "mcp", + attackId, + evaluatorId: evalId, + patternName: "tool-description-drift", + toolName: "tools/list", + toolArguments: {}, + toolResponse: "", + toolError: `Baseline file ${baselinePath} could not be read: ${readError.message}`, + judge: { + verdict: "ERROR", + score: 0, + confidence: 0, + evidence: "N/A", + reasoning: + "Existing baseline could not be read (permission or I/O error) — tool-description drift could not be verified. Baseline left untouched.", + errorMessage: readError.code ?? "baseline read error", + }, + }; + } else if (baselineJson === null) { + // Genuine first run (ENOENT) — no prior state to protect, so record and pass. log.info( ` No baseline found — recording current state (${tools.length} tools, hash: ${currentHash.slice(0, 12)}…)` ); diff --git a/core/tests/baselineScanner.test.ts b/core/tests/baselineScanner.test.ts index cf91844d..65490c8a 100644 --- a/core/tests/baselineScanner.test.ts +++ b/core/tests/baselineScanner.test.ts @@ -203,6 +203,19 @@ test("corrupt baseline file yields ERROR instead of crashing the run", async () } }); +test("unreadable baseline (I/O error, not missing) fails closed with ERROR", async () => { + const dir = await tmp(); + try { + // A directory where the baseline file should be → readFile throws EISDIR + // (code !== ENOENT). This must NOT be treated as a clean first run. + await mkdir(baselinePath(dir), { recursive: true }); + const results = await scan(dir); // must not throw, must not record over it + assert.strictEqual(rugPull(results).attacks[0].judge.verdict, "ERROR"); + } finally { + await rm(dir, { recursive: true, force: true }); + } +}); + test("matching baseline passes with no drift", async () => { const dir = await tmp(); try { From f20d10fdc11c6d0ee5aa4df057bee86b8b11f1df Mon Sep 17 00:00:00 2001 From: Jithin Date: Thu, 2 Jul 2026 17:13:33 +0530 Subject: [PATCH 3/3] fix: address review findings on baseline scanner hardening - 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) --- core/src/execute/baselineScanner.ts | 108 +++++++++++++++++++++------- core/src/targets/mcpTarget.ts | 10 +-- core/tests/baselineScanner.test.ts | 30 ++++++++ 3 files changed, 119 insertions(+), 29 deletions(-) diff --git a/core/src/execute/baselineScanner.ts b/core/src/execute/baselineScanner.ts index 914ac445..066fe5b8 100644 --- a/core/src/execute/baselineScanner.ts +++ b/core/src/execute/baselineScanner.ts @@ -5,6 +5,7 @@ import { readFile, writeFile, mkdir } from "node:fs/promises"; import path from "node:path"; import { createHash } from "node:crypto"; +import { z } from "zod"; import { randomUUID } from "../lib/random.js"; import { judgeToolResponse } from "../run/judge.js"; import { errorJudge as mcpErrorJudge } from "../lib/judgeTypes.js"; @@ -273,7 +274,7 @@ async function scanRugPull(ctx: BaselineScanContext): Promise { confidence: 0, evidence: "N/A", reasoning: - "Existing baseline could not be read (permission or I/O error) — tool-description drift could not be verified. Baseline left untouched.", + "Existing baseline could not be read (permission or I/O error) — tool-description drift could not be verified. Fix the file permissions/path, or delete the baseline file to record a fresh one.", errorMessage: readError.code ?? "baseline read error", }, }; @@ -282,23 +283,48 @@ async function scanRugPull(ctx: BaselineScanContext): Promise { log.info( ` No baseline found — recording current state (${tools.length} tools, hash: ${currentHash.slice(0, 12)}…)` ); - await recordBaseline(); - result = { - kind: "mcp", - attackId, - evaluatorId: evalId, - patternName: "tool-description-drift", - toolName: "tools/list", - toolArguments: {}, - toolResponse: `Baseline recorded: ${tools.length} tool(s), hash ${currentHash.slice(0, 16)}`, - judge: { - verdict: "PASS", - score: 10, - confidence: 100, - evidence: "N/A", - reasoning: `First run — baseline recorded. No previous state to compare against.`, - }, - }; + try { + await recordBaseline(); + result = { + kind: "mcp", + attackId, + evaluatorId: evalId, + patternName: "tool-description-drift", + toolName: "tools/list", + toolArguments: {}, + toolResponse: `Baseline recorded: ${tools.length} tool(s), hash ${currentHash.slice(0, 16)}`, + judge: { + verdict: "PASS", + score: 10, + confidence: 100, + evidence: "N/A", + reasoning: `First run — baseline recorded. No previous state to compare against.`, + }, + }; + } catch (err) { + // Write failed (read-only dir, disk full) — report ERROR, don't crash. + const message = err instanceof Error ? err.message : String(err); + log.warn(` Failed to record baseline: ${message}`); + result = { + kind: "mcp", + attackId, + evaluatorId: evalId, + patternName: "tool-description-drift", + toolName: "tools/list", + toolArguments: {}, + toolResponse: "", + toolError: `Could not write baseline file ${baselinePath}: ${message}`, + judge: { + verdict: "ERROR", + score: 0, + confidence: 0, + evidence: "N/A", + reasoning: + "First run, but the baseline could not be written (permission or disk error) — no baseline was recorded. Fix the output directory and re-run.", + errorMessage: "baseline write error", + }, + }; + } } else if (baseline === null) { // Baseline file exists but is empty or unparseable. We cannot verify drift, // and silently re-recording would let a corrupt-then-drift sequence launder @@ -344,6 +370,27 @@ async function scanRugPull(ctx: BaselineScanContext): Promise { reasoning: "tools/list output matches stored baseline — no drift detected.", }, }; + } else if (computeToolDiffs(baseline, currentSnapshot).length === 0) { + // Hash differs but content is identical — file was only reformatted. + // Not a rug pull; pass without rewriting the trusted file. + log.info(` ✓ No drift — baseline differs only in formatting`); + result = { + kind: "mcp", + attackId, + evaluatorId: evalId, + patternName: "tool-description-drift", + toolName: "tools/list", + toolArguments: {}, + toolResponse: `Content match (formatting-only difference): ${currentHash.slice(0, 16)}`, + judge: { + verdict: "PASS", + score: 10, + confidence: 100, + evidence: "N/A", + reasoning: + "tools/list content matches the stored baseline (the file differs only in formatting) — no drift detected.", + }, + }; } else { // DRIFT — do NOT overwrite the trusted baseline. Auto-accepting the changed // snapshot would let the next run PASS silently (the rug pull launders @@ -383,23 +430,34 @@ async function scanRugPull(ctx: BaselineScanContext): Promise { return [result]; } +const ToolSnapshotSchema = z.array( + z.object({ + name: z.string(), + description: z.string(), + inputSchema: z.unknown(), + }) +); + /** - * Parse a stored tool-snapshot baseline. Returns null when the file is empty or - * not a valid JSON array (corrupt), so the caller can fail closed instead of - * crashing — a bare `JSON.parse` on a truncated file throws and would abort the - * entire run with no report. + * Parse a stored tool-snapshot baseline. Returns null for an empty, non-JSON, + * or malformed file (e.g. `[null]`) so the caller can fail closed instead of + * crashing `computeToolDiffs`. */ function parseBaselineSnapshot( json: string ): Array<{ name: string; description: string; inputSchema: unknown }> | null { if (!json.trim()) return null; + let parsed: unknown; try { - const parsed: unknown = JSON.parse(json); - if (!Array.isArray(parsed)) return null; - return parsed as Array<{ name: string; description: string; inputSchema: unknown }>; + parsed = JSON.parse(json); } catch { return null; } + const result = ToolSnapshotSchema.safeParse(parsed); + // Cast: z.unknown() infers inputSchema as optional, but it's validated here. + return result.success + ? (result.data as Array<{ name: string; description: string; inputSchema: unknown }>) + : null; } function computeToolDiffs( diff --git a/core/src/targets/mcpTarget.ts b/core/src/targets/mcpTarget.ts index 677af467..b2ae66fa 100644 --- a/core/src/targets/mcpTarget.ts +++ b/core/src/targets/mcpTarget.ts @@ -111,10 +111,12 @@ class ConnectedMcpTarget implements McpTarget { return parts.join("\n").trim(); } catch (err: unknown) { const msg = err instanceof Error ? err.message : String(err); - // Signal read failures out-of-band (throw) rather than returning an - // "ERROR: …" string — that in-band sentinel collides with a resource whose - // legitimate content happens to start with "ERROR: ". Callers try/catch. - throw new Error(msg.slice(0, 300), { cause: err }); + // Throw rather than return an "ERROR: …" string (that in-band sentinel + // collides with legit content starting "ERROR: "). Callers try/catch. + throw new Error( + `Failed to read MCP resource "${uri.slice(0, 200)}": ${msg.slice(0, 300)}. Verify the resource URI is valid and the MCP server can serve it.`, + { cause: err } + ); } } diff --git a/core/tests/baselineScanner.test.ts b/core/tests/baselineScanner.test.ts index 65490c8a..71bd3ee9 100644 --- a/core/tests/baselineScanner.test.ts +++ b/core/tests/baselineScanner.test.ts @@ -203,6 +203,20 @@ test("corrupt baseline file yields ERROR instead of crashing the run", async () } }); +test("baseline that is a JSON array of malformed entries yields ERROR, not a crash", async () => { + const dir = await tmp(); + try { + // Valid JSON array, but the element is not a well-formed tool entry. A plain + // Array.isArray check would pass this through and crash computeToolDiffs on + // t.name; per-element validation must fail closed to ERROR instead. + await seedBaseline(dir, "[null]"); + const results = await scan(dir); // must not throw + assert.strictEqual(rugPull(results).attacks[0].judge.verdict, "ERROR"); + } finally { + await rm(dir, { recursive: true, force: true }); + } +}); + test("unreadable baseline (I/O error, not missing) fails closed with ERROR", async () => { const dir = await tmp(); try { @@ -216,6 +230,22 @@ test("unreadable baseline (I/O error, not missing) fails closed with ERROR", asy } }); +test("baseline that differs only in formatting is not flagged as drift", async () => { + const dir = await tmp(); + try { + const tools = [{ name: "t", description: "d", inputSchema: null }]; + // Same tool content the scanner records, but minified — different bytes, so + // the hash won't match, yet computeToolDiffs finds nothing. Must PASS (not a + // permanently sticky FAIL over a cosmetic reformat of the baseline file). + await seedBaseline(dir, JSON.stringify(tools)); + const results = await scan(dir, { tools }); + assert.strictEqual(rugPull(results).attacks[0].judge.verdict, "PASS"); + assert.match(rugPull(results).attacks[0].judge.reasoning, /no drift/i); + } finally { + await rm(dir, { recursive: true, force: true }); + } +}); + test("matching baseline passes with no drift", async () => { const dir = await tmp(); try {