From 7cd230bbd2437bc11340870c09274a063d458ddb Mon Sep 17 00:00:00 2001 From: Chad Sandor Date: Sun, 21 Jun 2026 21:11:14 -0400 Subject: [PATCH] fix(mcp): per-tool forward-arg allowlist for all built-in tools MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Extends the v1.14.0 forward-arg stripping from filesystem-only to EVERY built-in tool, and corrects an over-strip: - ForwardArgAllowlist maps each built-in tool to its full legitimate arg surface: send_email {to,subject,body}, run_command {cmd}, read_calendar {range}, plus the full @modelcontextprotocol/server-filesystem tool set (read_file {path,head,tail}, edit_file {path,edits,dryRun}, search_files {path,pattern,excludePatterns}, etc.). Any arg outside a tool's surface is stripped before forwarding. - Fixes the v1.14.0 blanket fs key-set (path/source/destination/paths/content) which would have dropped legitimate args of richer fs tools (edits, pattern, head/tail, sortBy, excludePatterns) — strictness no longer breaks function. - Custom-mapper tools not in the allowlist are forwarded unchanged (mapper owns the surface). - NormalizeForForward restructured: fs path-canonicalization (under a root) then the per-tool allowlist for any built-in. Tests: non-fs strip (read_calendar), full-surface preservation (search_files keeps pattern, strips unknown). 251 passing + 3 env-gated skipped. Co-Authored-By: Claude Opus 4.8 (1M context) --- CHANGELOG.md | 19 ++++ Directory.Build.props | 4 +- README.md | 6 +- docs/MATURITY.md | 2 +- src/IntentMesh.Integrations/McpProxy.cs | 109 ++++++++++++++------- tests/IntentMesh.Tests/IntegrationTests.cs | 51 ++++++++++ 6 files changed, 148 insertions(+), 43 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4f0de0a..c509484 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,6 +3,25 @@ All notable changes to IntentMesh. Claims are test-backed; see [docs/MATURITY.md](docs/MATURITY.md) for the production-ready / experimental / future breakdown. +## v1.14.1 — Per-tool MCP forward-arg allowlist (all built-ins) + +Extends (and corrects) the v1.14.0 forward-arg stripping. **251 passing + 3 env-gated skipped.** + +- **Strict allowlist for every built-in tool**, not just filesystem: `send_email` → `{to,subject,body}`, + `run_command` → `{cmd}`, `read_calendar` → `{range}`, plus the full `@modelcontextprotocol/server-filesystem` + tool set. Any argument outside a tool's known surface is stripped before forwarding, so an + unrecognized/unchecked key can't be honored by the server. +- **Fixes a v1.14.0 over-strip:** the previous blanket fs key-set (`path/source/destination/paths/content`) + would have dropped legitimate args of richer fs tools (`edit_file`'s `edits`, `search_files`'s `pattern`, + `directory_tree`'s `excludePatterns`, `read_text_file`'s `head/tail`, `list_directory_with_sizes`'s + `sortBy`). Each tool now lists its full legitimate arg surface, so strictness no longer breaks function. +- A custom-mapper tool not in the allowlist is forwarded unchanged (the mapper owns its arg surface). +- New tests cover a non-fs strip (`read_calendar`), full-surface preservation (`search_files` keeps + `pattern`), and the live FS-E2E continues to exercise read/write against the real server. + +> The filesystem allowlists track the pinned server version (`@modelcontextprotocol/server-filesystem@2026.1.14`); +> bump them alongside the package. + ## v1.14.0 — Audit fidelity, verification & supply-chain hardening (seventh review pass) Closes a seventh external review (7 High + 3 Medium). **249 passing + 3 env-gated skipped.** diff --git a/Directory.Build.props b/Directory.Build.props index e4b0c48..c53269c 100644 --- a/Directory.Build.props +++ b/Directory.Build.props @@ -5,7 +5,7 @@ Demos, tools, the web host, the E2E/bench runners, the template, and tests stay non-packable. --> - 1.14.0 + 1.14.1 Chad Sandor wyckit IntentMesh @@ -18,7 +18,7 @@ MIT false - v1.14.0 — seventh review pass: MCP bundles record applied approvals + strip unknown forward args; granular email/calendar verification; CI requires real FS-MCP + packs before npx; digest-pinned Docker with .dockerignore + writable runs volume; proxy-mode dedicated auth key; explain no longer honors caller approvals. See CHANGELOG.md. + v1.14.1 — per-tool MCP forward-arg allowlist for ALL built-in tools (send_email/run_command/read_calendar + the full filesystem tool set), replacing the v1.14.0 fs key-set that over-stripped richer tools (edit_file/search_files/etc.). See CHANGELOG.md. true diff --git a/README.md b/README.md index 053d0fd..d94042e 100644 --- a/README.md +++ b/README.md @@ -104,7 +104,7 @@ dotnet run --project src/IntentMesh.Cli -- --trace "plan my Friday and draft Sar dotnet run --project src/IntentMesh.Web # then open the printed localhost URL # tests -dotnet test IntentMesh.slnx # 249 passing (+3 env-gated skipped) +dotnet test IntentMesh.slnx # 251 passing (+3 env-gated skipped) ``` ### Wrap your own agent (the SDK on-ramp) @@ -190,7 +190,7 @@ v1.7 platform:** Phase 1 (clarity) ✓ · Phase 2 (signed artifacts, replay, con Phase 3 (Control Room v1) ✓ · Phase 4 (IntentBench 25/25) ✓ · Phase 5 (SDK + MCP proxy / OpenAPI import / real-adapter example) ✓ · Phase 6 (manifesto, whitepaper, landing) ✓. **v1.7** adds the adoptable platform surface (full-lifecycle SDK + host template, real-LLM-proposer hardening, -operator workflow, audit operations). **249 passing (+3 env-gated skipped) tests · IntentBench 25/25 · TLM 7/7.** +operator workflow, audit operations). **251 passing (+3 env-gated skipped) tests · IntentBench 25/25 · TLM 7/7.** **Proven vs. experimental vs. future (claims discipline).** [docs/MATURITY.md](docs/MATURITY.md) is the canonical statement: every *proven* claim has a passing test that would fail if it stopped being @@ -207,7 +207,7 @@ and the [CHANGELOG](CHANGELOG.md). ## Status Research prototype with a production-shaped core, **v1.8.0**. Symbolic layer: 7 TLMs, ~125 concepts, -7/7 round-trip verify; typed action contracts across four domains. **xUnit 249 passing (+3 env-gated skipped).** Five demo +7/7 round-trip verify; typed action contracts across four domains. **xUnit 251 passing (+3 env-gated skipped).** Five demo scenarios. See [docs/MATURITY.md](docs/MATURITY.md) for the proven / experimental / future breakdown. Delivered beyond v0.1: diff --git a/docs/MATURITY.md b/docs/MATURITY.md index 2f95865..ce091f1 100644 --- a/docs/MATURITY.md +++ b/docs/MATURITY.md @@ -2,7 +2,7 @@ The single source of truth for **what is production-ready, what is experimental, and what is future work.** Every "proven" claim below is backed by a test that would fail if the claim stopped being -true (`dotnet test IntentMesh.slnx` — **249 passing, 3 env-gated skipped**). Nothing here is aspirational unless it says so. +true (`dotnet test IntentMesh.slnx` — **251 passing, 3 env-gated skipped**). Nothing here is aspirational unless it says so. > IntentMesh is a **research prototype with a production-shaped core**: the security kernel and its > guarantees are proven and stable; the *operational backends* around it (KMS, DB persistence, diff --git a/src/IntentMesh.Integrations/McpProxy.cs b/src/IntentMesh.Integrations/McpProxy.cs index 90bd800..727adfa 100644 --- a/src/IntentMesh.Integrations/McpProxy.cs +++ b/src/IntentMesh.Integrations/McpProxy.cs @@ -323,53 +323,88 @@ public McpForwardResult GateAndForward(McpToolCall call, IMcpClient client, return new McpForwardResult(gate, ServerResponse: response); } - /// For a filesystem call under an allowed root, rewrite each path-bearing arg to the exact - /// canonical in-root path the gate validated — so the forwarded call can't re-resolve a relative or - /// aliased original arg to a different target than was checked. Non-fs calls (or no allowed root) - /// are forwarded unchanged. + /// + /// The EXACT argument keys forwarded to the server for each BUILT-IN tool — every other key is stripped + /// before the call leaves the proxy, so an argument the typed mapping never represented (and the policy + /// never checked) can't be honored unsigned/unchecked by the server. Each entry is the tool's FULL + /// legitimate arg surface (so strictness doesn't break the tool). The filesystem entries track the + /// pinned @modelcontextprotocol/server-filesystem version; bump them with the server. A + /// custom-mapper tool that is NOT listed here is forwarded unchanged — the custom mapper owns its + /// server's arg surface (and is responsible for its own allowlisting). + /// + private static readonly IReadOnlyDictionary ForwardArgAllowlist = + new Dictionary(StringComparer.Ordinal) + { + // Non-filesystem built-ins (small, fully-modeled surfaces). + ["send_email"] = new[] { "to", "subject", "body" }, + ["run_command"] = new[] { "cmd" }, + ["read_calendar"] = new[] { "range" }, + // @modelcontextprotocol/server-filesystem tools. + ["read_file"] = new[] { "path", "head", "tail" }, + ["read_text_file"] = new[] { "path", "head", "tail" }, + ["read_media_file"] = new[] { "path" }, + ["read_multiple_files"] = new[] { "paths" }, + ["get_file_info"] = new[] { "path" }, + ["list_directory"] = new[] { "path" }, + ["list_directory_with_sizes"] = new[] { "path", "sortBy" }, + ["directory_tree"] = new[] { "path", "excludePatterns" }, + ["search_files"] = new[] { "path", "pattern", "excludePatterns" }, + ["list_allowed_directories"] = Array.Empty(), + ["write_file"] = new[] { "path", "content" }, + ["edit_file"] = new[] { "path", "edits", "dryRun" }, + ["create_directory"] = new[] { "path" }, + ["move_file"] = new[] { "source", "destination" }, + }; + + /// Normalize a call for forwarding: (1) for a filesystem call under a sandbox root, rewrite + /// each path-bearing arg to the exact canonical in-root path the gate validated (so the server can't + /// re-resolve a relative/aliased original to a different target than was checked); (2) for any built-in + /// tool, strip every argument outside that tool's allowlist so an unrecognized/unchecked key can't be + /// honored. A custom-mapper tool not in the allowlist is forwarded with its args intact. private McpToolCall NormalizeForForward(McpToolCall call) { - if (_allowedRoot is null) return call; var (action, _) = _customMapper?.Invoke(call) ?? MapToAction(call); - if (action is not (FsReadAction or FsWriteAction)) return call; - - var root = Canonicalize(Path.TrimEndingDirectorySeparator(Path.GetFullPath(_allowedRoot))); var args = new Dictionary(call.Args, StringComparer.Ordinal); - bool hadPath = false; - foreach (var key in new[] { "path", "source", "destination" }) - if (args.TryGetValue(key, out var p) && !string.IsNullOrEmpty(p)) { args[key] = Resolve(p, root); hadPath = true; } - // Custom-mapper paths: a per-server mapper may carry the path in a NON-standard arg (e.g. "target", - // "filepath"). The typed action already exposes the validated path(s); rewrite whichever raw arg - // holds that value to the exact canonical in-root path, so the forwarded call can't re-resolve a - // relative/aliased original to a different target than the gate checked. - foreach (var typed in TypedPaths(action)) - { - var canonical = Resolve(typed, root); - foreach (var key in args.Keys.ToList()) - if (!string.IsNullOrEmpty(args[key]) && Resolve(args[key], root) == canonical) { args[key] = canonical; hadPath = true; } - } - // No-path filesystem tool under a sandbox: scope it explicitly to the root rather than letting the - // server fall back to its own working directory (defense in depth over the server's own sandbox). - if (!hadPath && !args.ContainsKey("paths")) - args["path"] = root; - if (args.TryGetValue("paths", out var multi) && !string.IsNullOrWhiteSpace(multi)) + if (_allowedRoot is not null && action is FsReadAction or FsWriteAction) { - try + var root = Canonicalize(Path.TrimEndingDirectorySeparator(Path.GetFullPath(_allowedRoot))); + bool hadPath = false; + foreach (var key in new[] { "path", "source", "destination" }) + if (args.TryGetValue(key, out var p) && !string.IsNullOrEmpty(p)) { args[key] = Resolve(p, root); hadPath = true; } + + // Custom-mapper paths: a per-server mapper may carry the path in a NON-standard arg (e.g. + // "target"). The typed action exposes the validated path(s); rewrite whichever raw arg holds + // that value to the canonical in-root path. + foreach (var typed in TypedPaths(action)) { - var parsed = JsonSerializer.Deserialize>(multi); - if (parsed is not null) - args["paths"] = JsonSerializer.Serialize(parsed.Select(e => string.IsNullOrEmpty(e) ? e : Resolve(e, root)).ToList()); + var canonical = Resolve(typed, root); + foreach (var key in args.Keys.ToList()) + if (!string.IsNullOrEmpty(args[key]) && Resolve(args[key], root) == canonical) { args[key] = canonical; hadPath = true; } + } + // No-path filesystem tool under a sandbox: scope it explicitly to the root rather than letting + // the server fall back to its own working directory (defense in depth over the server's sandbox). + if (!hadPath && !args.ContainsKey("paths")) + args["path"] = root; + if (args.TryGetValue("paths", out var multi) && !string.IsNullOrWhiteSpace(multi)) + { + try + { + var parsed = JsonSerializer.Deserialize>(multi); + if (parsed is not null) + args["paths"] = JsonSerializer.Serialize(parsed.Select(e => string.IsNullOrEmpty(e) ? e : Resolve(e, root)).ToList()); + } + catch { /* not a JSON array — it was validated as a single path; leave as-is */ } } - catch { /* not a JSON array — it was validated as a single path; leave as-is */ } } - // Forward ONLY the recognized filesystem arguments — strip any extra/unknown key so an argument the - // typed action never represented (and the path policy never checked) cannot be honored by the - // server. The forwarded payload is therefore exactly the policy-checked fields. - var allowed = new HashSet(new[] { "path", "source", "destination", "paths", "content" }, StringComparer.Ordinal); - foreach (var key in args.Keys.Where(k => !allowed.Contains(k)).ToList()) - args.Remove(key); + // Forward-arg allowlist for every built-in tool — strip anything outside the tool's known surface. + if (ForwardArgAllowlist.TryGetValue(call.Tool, out var allowed)) + { + var allowedSet = new HashSet(allowed, StringComparer.Ordinal); + foreach (var key in args.Keys.Where(k => !allowedSet.Contains(k)).ToList()) + args.Remove(key); + } return call with { Args = args }; diff --git a/tests/IntentMesh.Tests/IntegrationTests.cs b/tests/IntentMesh.Tests/IntegrationTests.cs index fd8912b..a750fab 100644 --- a/tests/IntentMesh.Tests/IntegrationTests.cs +++ b/tests/IntentMesh.Tests/IntegrationTests.cs @@ -508,6 +508,57 @@ public void Filesystem_forward_strips_unknown_args() finally { Directory.Delete(root, true); } } + /// A non-filesystem built-in forward (read_calendar) also carries ONLY its allowlisted args — + /// an extra/unknown key is stripped before the call reaches the server. + [Fact] + public void Non_filesystem_forward_strips_unknown_args() + { + var root = TempRoot(); + try + { + IReadOnlyDictionary? forwarded = null; + var client = new CapturingMcpClient(args => { forwarded = args; return "{}"; }); + var proxy = new McpProxy(Runtime(), Workspace.CreateDemo(), allowedRoot: root, + auditStore: new FileRunArtifactStore(TempRoot()), auditKeyProvider: McpTestKeyProvider, + approvalService: NewApprovalService(), tenantId: "test"); + + var fwd = proxy.GateAndForward( + new McpToolCall("read_calendar", new Dictionary { ["range"] = "Friday", ["evil"] = "x" }), client); + + Assert.True(fwd.Gate.Allowed); + Assert.NotNull(forwarded); + Assert.True(forwarded!.ContainsKey("range")); // the legitimate arg is kept + Assert.False(forwarded.ContainsKey("evil")); // the unknown arg is stripped + } + finally { Directory.Delete(root, true); } + } + + /// The allowlist keeps each tool's FULL legitimate surface — a tool with more args than + /// path/content (search_files: path + pattern) is not over-stripped, while an unknown arg still is. + [Fact] + public void Forward_allowlist_keeps_a_tools_full_arg_surface() + { + var root = TempRoot(); + try + { + IReadOnlyDictionary? forwarded = null; + var client = new CapturingMcpClient(args => { forwarded = args; return "[]"; }); + var proxy = new McpProxy(Runtime(), Workspace.CreateDemo(), allowedRoot: root, + auditStore: new FileRunArtifactStore(TempRoot()), auditKeyProvider: McpTestKeyProvider, + approvalService: NewApprovalService(), tenantId: "test"); + + var fwd = proxy.GateAndForward( + new McpToolCall("search_files", new Dictionary { ["path"] = root, ["pattern"] = "note", ["evil"] = "x" }), client); + + Assert.True(fwd.Gate.Allowed); + Assert.NotNull(forwarded); + Assert.True(forwarded!.ContainsKey("pattern")); // a legitimate non-path arg is preserved + Assert.True(forwarded.ContainsKey("path")); + Assert.False(forwarded.ContainsKey("evil")); // ...but an unknown one is still stripped + } + finally { Directory.Delete(root, true); } + } + /// The audit sink and challenge service are MANDATORY for the dangerous operations: a proxy /// not wired with them cannot forward (no audit-less side effect) and cannot accept a raw approval — /// both throw rather than silently taking an unsafe path. A pure Gate decision still works.