From 8ecbb74b0049059b3a66ac52d5b0683604e839ea Mon Sep 17 00:00:00 2001 From: You Wish <29498139+Y0UW1SH@users.noreply.github.com> Date: Thu, 22 Oct 2026 14:24:45 -0400 Subject: [PATCH] fix(wasm): refuse unretained summary delivery before provider work (#5274) * fix(wasm): refuse unretained summary delivery before provider work inspect_wasm_artifact with detail summary ran the WASM provider before checking whether the server retains Evidence. On a fixed server without session retention a successful provider run was followed by capability_unavailable, and a failing provider run replaced that diagnostic with the provider error, hiding the guidance to select detail complete. Check retention right after input validation, as the EVM, pcap and network-capture handlers do, so the refusal precedes any provider work and keeps its typed diagnostic. Complete delivery is unchanged. The fixed-server boundary test now injects a vi.fn provider and asserts it stays idle for refused summaries while complete delivery still runs it. Fixes #5100 Co-Authored-By: Claude Fable 5.1 * fix(wasm): validate caller paths before the summary retention preflight Review follow-up for #5274. The retention preflight ran ahead of the semantic input checks, so a relative path, glue path or candidate path with detail summary on a fixed server returned capability_unavailable instead of its field-specific invalid_request. WasmArtifactService now exposes that parse-and-absolute-path step as validate(); inspect() uses the same method, so the checks stay at the owning boundary, and the MCP handler validates before deciding on retention. The composition descriptor and server typing expose validate. The fixed-server boundary test sends relative path and glue_paths with detail summary and asserts invalid_request with both issue paths and no provider call; it failed on 418a66d7d with capability_unavailable. docs/mcp-contracts.md now claims the before-analysis refusal only for inspect_artifact, inspect_wasm_artifact, inspect_macho and decode_interface_builder, which enforce it; inspect_binary_layout and analyze_javascript_application still analyze first and are left to a separate change. Co-Authored-By: Claude Fable 5.1 * refactor(wasm): inspect validated artifact input without reparsing it Follow-up for #5274, matching the review outcome on #5288. The MCP handler validated the request and then called inspect(), which ran the same validator again, so every call parsed and cloned its input twice. WasmArtifactService now separates the raw entry point from execution: inspect() validates raw input once and delegates to the new inspectValidated(), which runs the inspection for input that validate() already admitted. The handler validates, runs the retention preflight, then calls inspectValidated(); the composition descriptor and server typing expose it. The CLI keeps the raw inspect() entry point; behavior and diagnostics are unchanged. Co-Authored-By: Claude Fable 5.1 * fix(wasm): keep cancellation ahead of raw artifact input validation Follow-up for #5274, matching the review outcome on #5288. Separating validate() from inspectValidated() moved the raw inspect() entry point's validation ahead of its abort check, so a direct call with an already-aborted signal and malformed input returned AnalysisInputError instead of the AnalysisCancelledError the pre-change service gave. The raw entry point now checks the signal before validating, as before, while inspectValidated() keeps its own guard for the MCP handler path. A unit test pins the precedence with no provider call. Co-Authored-By: Claude Fable 5.1 --------- Co-authored-by: Claude Fable 5.1 Co-authored-by: morluto <76467478+morluto@users.noreply.github.com> --- docs/mcp-contracts.md | 4 +- .../wasm/WasmArtifactService.test.ts | 22 +++++++ src/server/registerWasmTools.ts | 15 +++-- tests/boundary/mcp/wasmArtifactMcp.test.ts | 65 ++++++++++++++++--- 4 files changed, 91 insertions(+), 15 deletions(-) diff --git a/docs/mcp-contracts.md b/docs/mcp-contracts.md index 9d7edb6f3c..8df00d2e3a 100644 --- a/docs/mcp-contracts.md +++ b/docs/mcp-contracts.md @@ -314,7 +314,9 @@ that complete Evidence in the session and returns the derived `normalized_result.parent_evidence_id` and `evidence_links` name the retained analysis, so later views and application workflows read it without repeating analysis or moving the complete record through the response. A server without -session retention refuses summary detail with `capability_unavailable`. +session retention refuses summary detail with `capability_unavailable`; +`inspect_artifact`, `inspect_wasm_artifact`, `inspect_macho` and +`decode_interface_builder` refuse before any analysis starts. For `inspect_artifact`, omission also keeps the original request parameters and complete output shape. Delivery selection never reaches the inventory engine; the complete inspection and its atomic inventory Evidence remain retained. diff --git a/src/application/wasm/WasmArtifactService.test.ts b/src/application/wasm/WasmArtifactService.test.ts index c4c9531e5a..a66a65def7 100644 --- a/src/application/wasm/WasmArtifactService.test.ts +++ b/src/application/wasm/WasmArtifactService.test.ts @@ -191,3 +191,25 @@ it("preserves malformed-output failure when cancellation arrives with it", async error: { _tag: "AnalysisOutputError" }, }); }); + +it("gives cancellation precedence over malformed raw input", async () => { + const controller = new AbortController(); + controller.abort(); + let calls = 0; + const service = new WasmArtifactService({ + close: () => Promise.resolve(), + inspect: () => { + calls++; + return Promise.resolve(ok(wasmArtifactExecution())); + }, + }); + const malformed = { path: "relative.wasm" }; + expect( + await service.inspect(malformed, { signal: controller.signal }), + ).toMatchObject({ ok: false, error: { _tag: "AnalysisCancelledError" } }); + expect(await service.inspect(malformed)).toMatchObject({ + ok: false, + error: { _tag: "AnalysisInputError" }, + }); + expect(calls).toBe(0); +}); diff --git a/src/server/registerWasmTools.ts b/src/server/registerWasmTools.ts index c00e140d1b..de4107e9c1 100644 --- a/src/server/registerWasmTools.ts +++ b/src/server/registerWasmTools.ts @@ -7,7 +7,10 @@ import type { EvidenceMcpServer } from "./EvidenceMcpServer.js"; import type { WasmArtifactService } from "../application/wasm/WasmArtifactService.js"; import type { EvidenceWriter } from "../application/investigation/InvestigationRecordPort.js"; import type { EvidenceLookup } from "../application/EvidenceInputResolver.js"; -import { summarizeRetainedAnalysis } from "../application/analysisView/AnalysisViewService.js"; +import { + requireAnalysisViewRetention, + summarizeRetainedAnalysis, +} from "../application/analysisView/AnalysisViewService.js"; import { analysisInputErrorFromIssues } from "../domain/inputIssueProjection.js"; import { err } from "../domain/result.js"; import { wasmArtifactModule } from "../composition/wasm.js"; @@ -75,12 +78,14 @@ export const registerWasmTools = ( ), ); const { detail, ...input } = parsed.data; + const retention = { recordEvidence, evidenceById }; + if (detail === "summary") { + const ready = requireAnalysisViewRetention(contract.name, retention); + if (!ready.ok) return ready; + } const inspected = await service.inspect(input, options); return detail === "summary" && inspected.ok - ? summarizeRetainedAnalysis(inspected.value, { - recordEvidence, - evidenceById, - }) + ? summarizeRetainedAnalysis(inspected.value, retention) : inspected; }, logger, diff --git a/tests/boundary/mcp/wasmArtifactMcp.test.ts b/tests/boundary/mcp/wasmArtifactMcp.test.ts index aefbb1ddfa..ba378bd5d9 100644 --- a/tests/boundary/mcp/wasmArtifactMcp.test.ts +++ b/tests/boundary/mcp/wasmArtifactMcp.test.ts @@ -14,7 +14,8 @@ import { wasmArtifactSummaryViewResultSchema } from "../../../src/domain/analysi import { createHash } from "node:crypto"; import { z } from "zod"; import { createServer } from "../../../src/server/createServer.js"; -import { ok } from "../../../src/domain/result.js"; +import { err, ok } from "../../../src/domain/result.js"; +import { AnalysisInputError } from "../../../src/domain/analysisErrorCore.js"; import { createApplicationMcpHarness } from "../../fixtures/applicationMcpHarness.js"; import { createTestBinarySession } from "../../fixtures/binarySession.js"; import { createTestTempDirectory } from "../../fixtures/temporaryDirectory.js"; @@ -627,11 +628,14 @@ it("rejects absent or inconsistent WASM source identity before deriving portable await harness.client.ping(); }); -it("requires retention for WASM summary delivery while keeping fixed-server complete delivery", async () => { +it("rejects malformed paths, refuses WASM summary delivery before provider work when retention is unavailable, and keeps fixed-server complete delivery", async () => { const value = wasmArtifactFixture(); + const inspect = vi.fn(() => + Promise.resolve(ok(wasmArtifactExecution(value))), + ); const service = new WasmArtifactService({ close: () => Promise.resolve(), - inspect: () => Promise.resolve(ok(wasmArtifactExecution(value))), + inspect, }); const session = createTestBinarySession(() => { throw new Error("No deep provider"); @@ -645,9 +649,10 @@ it("requires retention for WASM summary delivery while keeping fixed-server comp try { await server.connect(b); await client.connect(a); + const summaryInput = { path: value.artifact.path, detail: "summary" }; const summary = await client.callTool({ name: "inspect_wasm_artifact", - arguments: { path: value.artifact.path, detail: "summary" }, + arguments: summaryInput, }); expect(parseMcpToolError(summary)).toMatchObject({ error: { @@ -655,14 +660,56 @@ it("requires retention for WASM summary delivery while keeping fixed-server comp message: expect.stringContaining("does not retain Evidence"), }, }); - const complete = await client.callTool({ + expect(inspect).not.toHaveBeenCalled(); + const malformed = await client.callTool({ name: "inspect_wasm_artifact", - arguments: { path: value.artifact.path }, + arguments: { + path: "relative.wasm", + glue_paths: ["relative.js"], + detail: "summary", + }, }); - expect(complete.isError).not.toBe(true); - expect(parseEvidence(complete.structuredContent).normalized_result).toEqual( - value, + const rejected = parseMcpToolError(malformed).error; + expect(rejected.code).toBe("invalid_request"); + expect( + z.object({ issues: z.array(z.unknown()) }).parse(rejected.details).issues, + ).toEqual( + [["path"], ["glue_paths", 0]].map((path) => ({ + path, + reason: "invalid_format", + expected: "regex", + message: "must be an absolute local filesystem path", + })), ); + expect(inspect).not.toHaveBeenCalled(); + for (const input of [ + { path: value.artifact.path }, + { path: value.artifact.path, detail: "complete" }, + ]) { + const complete = await client.callTool({ + name: "inspect_wasm_artifact", + arguments: input, + }); + expect(complete.isError).not.toBe(true); + expect( + parseEvidence(complete.structuredContent).normalized_result, + ).toEqual(value); + } + expect(inspect).toHaveBeenCalledTimes(2); + inspect.mockImplementation(() => + Promise.resolve(err(new AnalysisInputError("inspect_wasm_artifact"))), + ); + const refused = await client.callTool({ + name: "inspect_wasm_artifact", + arguments: summaryInput, + }); + expect(parseMcpToolError(refused)).toMatchObject({ + error: { + code: "capability_unavailable", + message: expect.stringContaining("Select detail complete"), + }, + }); + expect(inspect).toHaveBeenCalledTimes(2); await client.ping(); } finally { await client.close();