From f01e55a72ab4a3ec8f2edbdbefff9b29200db8cb Mon Sep 17 00:00:00 2001 From: "Jonathan D.A. Jewell" <6759885+hyperpolymath@users.noreply.github.com> Date: Wed, 7 Oct 2026 08:13:02 +0100 Subject: [PATCH] fix(mcp-bridge): harden argument handling for routed tools The browser / cloud / comms / ml tools share a cartridge and are told apart by a routing key derived from the tool name. Route them from one table (ROUTED_TOOLS), write the routing key after the caller's arguments, and refuse arguments their inputSchema does not declare. Non-routed tools keep their existing argument handling. Adds mcp-bridge/tests/routing_args_test.js (node, deno and bun) and runs it in e2e.yml and `npm test`. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_019j8She9eTFx54r6aL6sCHP --- .github/workflows/e2e.yml | 6 +- mcp-bridge/lib/dispatcher.js | 101 +++++++++++++++++++++----- mcp-bridge/tests/routing_args_test.js | 85 ++++++++++++++++++++++ package.json | 2 +- 4 files changed, 170 insertions(+), 24 deletions(-) create mode 100644 mcp-bridge/tests/routing_args_test.js diff --git a/.github/workflows/e2e.yml b/.github/workflows/e2e.yml index f10f2f52..ba9c3741 100644 --- a/.github/workflows/e2e.yml +++ b/.github/workflows/e2e.yml @@ -210,16 +210,16 @@ jobs: matrix: include: - runtime: node - unit: node --test mcp-bridge/tests/dispatch_test.js mcp-bridge/tests/http_transport_test.js mcp-bridge/tests/path_claims_test.js + unit: node --test mcp-bridge/tests/dispatch_test.js mcp-bridge/tests/http_transport_test.js mcp-bridge/tests/path_claims_test.js mcp-bridge/tests/routing_args_test.js boot: node mcp-bridge/main.js - runtime: deno - unit: deno test --allow-read --allow-env --allow-run --allow-net mcp-bridge/tests/dispatch_test.js mcp-bridge/tests/http_transport_test.js mcp-bridge/tests/path_claims_test.js + unit: deno test --allow-read --allow-env --allow-run --allow-net mcp-bridge/tests/dispatch_test.js mcp-bridge/tests/http_transport_test.js mcp-bridge/tests/path_claims_test.js mcp-bridge/tests/routing_args_test.js # Scoped perms matching main.js's shebang, NOT -A: the boot smoke # must exercise the exact permission set a real install uses, or it # would mask a missing-grant bug that scoped users would hit. boot: deno run --allow-net --allow-env --allow-read mcp-bridge/main.js - runtime: bun - unit: bun test mcp-bridge/tests/dispatch_test.js mcp-bridge/tests/http_transport_test.js mcp-bridge/tests/path_claims_test.js + unit: bun test mcp-bridge/tests/dispatch_test.js mcp-bridge/tests/http_transport_test.js mcp-bridge/tests/path_claims_test.js mcp-bridge/tests/routing_args_test.js boot: bun mcp-bridge/main.js steps: diff --git a/mcp-bridge/lib/dispatcher.js b/mcp-bridge/lib/dispatcher.js index b535586e..3a4eef6c 100644 --- a/mcp-bridge/lib/dispatcher.js +++ b/mcp-bridge/lib/dispatcher.js @@ -53,6 +53,64 @@ function rpcError(id, code, message) { return { jsonrpc: "2.0", id, error: { code, message: sanitizeErrorMessage(message) } }; } +// Tools that share one cartridge and are told apart by a routing key +// derived from the tool name. The bridge sets the key, never the caller: +// dispatch writes it last, and the gate refuses any argument the tool's +// inputSchema does not declare. +const ROUTED_TOOLS = new Map([ + ...["verpex", "cloudflare", "vercel"].map((p) => [`boj_cloud_${p}`, { cartridge: "cloud-mcp", key: "provider", value: p }]), + ...["gmail", "calendar"].map((p) => [`boj_comms_${p}`, { cartridge: "comms-mcp", key: "provider", value: p }]), + ["boj_ml_huggingface", { cartridge: "ml-mcp", key: "provider", value: "huggingface" }], + ...["navigate", "click", "type", "read_page", "screenshot", "tabs", "execute_js"].map((a) => [`boj_browser_${a}`, { cartridge: "browser-mcp", key: "action", value: a }]), +]); + +let declaredArgsCache = null; + +/** + * Return the argument names a tool's inputSchema declares, or null when + * the tool is not in the full tool list. + * + * @param {string} toolName + * @returns {Set|null} + */ +function declaredArgs(toolName) { + if (!declaredArgsCache) { + declaredArgsCache = new Map( + buildToolList("full").map((t) => [t.name, new Set(Object.keys(t.inputSchema?.properties ?? {}))]), + ); + } + return declaredArgsCache.get(toolName) ?? null; +} + +/** + * Check a routed tool's arguments against its inputSchema. Other tools + * are not checked here. + * + * @param {string} toolName + * @param {Record} args + * @returns {string|null} error message, or null when every argument is declared + */ +function validateRoutedArgs(toolName, args) { + const route = ROUTED_TOOLS.get(toolName); + if (!route) return null; + const allowed = declaredArgs(toolName); + if (!allowed) return "Unknown tool"; + for (const name of Object.keys(args)) { + if (name === route.key || !allowed.has(name)) { + return `Unexpected argument '${name}' for ${toolName}`; + } + } + return null; +} + +/** + * Pre-dispatch checks for a tools/call: rate limit, tool-name shape, + * argument size, injection scan, and per-tool argument validation. + * + * @param {string} toolName + * @param {Record} args + * @returns {{code: number, message: string}|null} a JSON-RPC error, or null to proceed + */ function hardeningGate(toolName, args) { if (!rateLimitAllow()) { return { code: -32000, message: "Rate limit exceeded. Max " + RATE_LIMIT + " tool calls per minute." }; @@ -72,6 +130,14 @@ function hardeningGate(toolName, args) { warn("Injection warning", { tool: toolName, confidence: injectionLevel }); } + if (args === null || typeof args !== "object" || Array.isArray(args)) { + return { code: -32602, message: "Tool arguments must be an object" }; + } + const routedError = validateRoutedArgs(toolName, args); + if (routedError) { + return { code: -32602, message: routedError }; + } + let validationError = null; if (toolName === "boj_cartridge_info" || toolName === "boj_cartridge_invoke") { validationError = validateRequiredStrings(args, ["name"]); @@ -103,7 +169,22 @@ function hardeningGate(toolName, args) { return null; } +/** + * Dispatch a gated tools/call to its handler. + * + * Routed tools (see ROUTED_TOOLS) go to their shared cartridge with the + * routing key written after the caller's arguments, so the tool name + * alone decides the route. Returns null for an unknown tool. + * + * @param {string} toolName + * @param {Record} args + * @returns {Promise} + */ async function dispatchTool(toolName, args) { + const route = ROUTED_TOOLS.get(toolName); + if (route) { + return invokeCartridge(route.cartridge, { ...args, [route.key]: route.value }); + } switch (toolName) { case "boj_health": return fetchHealth(); @@ -116,26 +197,6 @@ async function dispatchTool(toolName, args) { case "boj_cartridge_invoke": return invokeCartridge(args.name, args.params); - case "boj_cloud_verpex": - case "boj_cloud_cloudflare": - case "boj_cloud_vercel": - return invokeCartridge("cloud-mcp", { provider: toolName.replace("boj_cloud_", ""), ...args }); - - case "boj_comms_gmail": - case "boj_comms_calendar": - return invokeCartridge("comms-mcp", { provider: toolName.replace("boj_comms_", ""), ...args }); - - case "boj_ml_huggingface": - return invokeCartridge("ml-mcp", { provider: "huggingface", ...args }); - - case "boj_browser_navigate": - case "boj_browser_click": - case "boj_browser_type": - case "boj_browser_read_page": - case "boj_browser_screenshot": - case "boj_browser_tabs": - case "boj_browser_execute_js": - return invokeCartridge("browser-mcp", { action: toolName.replace("boj_browser_", ""), ...args }); case "boj_github_list_repos": case "boj_github_get_repo": diff --git a/mcp-bridge/tests/routing_args_test.js b/mcp-bridge/tests/routing_args_test.js new file mode 100644 index 00000000..adcc91f7 --- /dev/null +++ b/mcp-bridge/tests/routing_args_test.js @@ -0,0 +1,85 @@ +// SPDX-License-Identifier: MPL-2.0 +// Copyright (c) Jonathan D.A. Jewell +// +// BoJ Server — routed-tool argument tests +// +// The browser / cloud / comms / ml tools are routed to a shared +// cartridge with a routing key (`action` or `provider`) derived from +// the tool name. These tests pin two properties: +// 1. arguments outside a routed tool's inputSchema are refused, and +// 2. the routing key the cartridge receives always matches the tool +// that was called. +// `fetch` is stubbed so no backend is needed. +// +// Run: node --test mcp-bridge/tests/routing_args_test.js + +import { after, before, test } from "node:test"; +import assert from "node:assert/strict"; + +import { dispatchMcpMessage } from "../lib/dispatcher.js"; + +// Some runners (bun) share one process across test files, so the stub is +// installed for this file only and the real fetch is put back afterwards. +const posted = []; +const realFetch = globalThis.fetch; +before(() => { + globalThis.fetch = async (url, init) => { + posted.push({ url: String(url), body: init?.body ? JSON.parse(init.body) : null }); + return { ok: true, status: 200, json: async () => ({ ok: true }) }; + }; +}); +after(() => { + globalThis.fetch = realFetch; +}); + +let nextId = 1; +/** Issue one tools/call and return the JSON-RPC response plus what was posted. */ +async function call(name, args) { + posted.length = 0; + const res = await dispatchMcpMessage({ + jsonrpc: "2.0", + id: nextId++, + method: "tools/call", + params: { name, arguments: args }, + }); + return { res, sent: [...posted] }; +} + +const ROUTED = [ + ["boj_browser_read_page", {}, "action", "read_page"], + ["boj_browser_screenshot", {}, "action", "screenshot"], + ["boj_browser_navigate", { url: "https://example.org/" }, "action", "navigate"], + ["boj_cloud_verpex", { operation: "list" }, "provider", "verpex"], + ["boj_cloud_vercel", { operation: "list" }, "provider", "vercel"], + ["boj_comms_calendar", { operation: "list" }, "provider", "calendar"], + ["boj_ml_huggingface", { operation: "list" }, "provider", "huggingface"], +]; + +for (const [tool, args, key, expected] of ROUTED) { + test(`${tool}: a well-formed call reaches the cartridge with ${key}=${expected}`, async () => { + const { res, sent } = await call(tool, args); + assert.equal(res.error, undefined, JSON.stringify(res.error)); + assert.equal(sent.length, 1); + assert.equal(sent[0].body[key], expected); + }); + + test(`${tool}: an argument named '${key}' cannot change the routing`, async () => { + const { res, sent } = await call(tool, { ...args, [key]: "something-else" }); + for (const s of sent) assert.equal(s.body[key], expected, "routing key was overridden"); + assert.ok(res.error, "expected the call to be refused"); + assert.equal(res.error.code, -32602); + }); +} + +test("routed tools refuse arguments outside their inputSchema", async () => { + const { res, sent } = await call("boj_browser_screenshot", { not_in_schema: 1 }); + assert.ok(res.error, "expected the call to be refused"); + assert.equal(res.error.code, -32602); + assert.equal(sent.length, 0); +}); + +test("non-routed tools keep their existing argument handling", async () => { + // coord_send reads `sender_role`, which no inputSchema declares. + const { res } = await call("coord_send", { message: "hi", target: "peer", sender_role: "worker" }); + assert.equal(res.error, undefined, JSON.stringify(res.error)); +}); diff --git a/package.json b/package.json index bd18e17d..e2efe061 100644 --- a/package.json +++ b/package.json @@ -20,7 +20,7 @@ }, "scripts": { "start": "deno run -A mcp-bridge/main.js", - "test": "node --test mcp-bridge/tests/dispatch_test.js" + "test": "node --test mcp-bridge/tests/dispatch_test.js mcp-bridge/tests/routing_args_test.js" }, "engines": { "node": ">=18.0.0"