fix(mcp-bridge): harden argument handling for routed tools (#349)

## Summary

Harden argument handling in the MCP bridge for the routed tools:
browser, cloud, comms and ml. These tools share a cartridge and are told
apart by a routing key derived from the tool name. After this change the
tool name alone decides the route, and arguments that a routed tool's
`inputSchema` does not declare are refused with `-32602`.

## 📌 New pins

Head SHA: **f01e55a72ab4a3ec8f2edbdbefff9b29200db8cb**. This PR adds or
changes no action, lockfile or container pins.

## Changes

- `mcp-bridge/lib/dispatcher.js`:
- New `ROUTED_TOOLS` table (tool name → cartridge plus routing key). It
replaces four `switch` arms. The routing key is written after the
caller's arguments.
- New `validateRoutedArgs` and `declaredArgs`. The hardening gate
refuses arguments that a routed tool's `inputSchema` does not declare.
  - The gate also refuses non-object `arguments`.
  - JSDoc added to `dispatchTool`, `hardeningGate` and the new helpers.
- Scope: non-routed tools are unchanged. `coord_send` reads
`sender_role`, which no schema declares, so applying the check to every
tool would break it.
- `mcp-bridge/tests/routing_args_test.js` (new, 16 tests). Each routed
tool still reaches its cartridge with the right key. An argument cannot
change the routing key. Undeclared arguments are refused. Non-routed
handling is unchanged. `fetch` is stubbed for this file only and
restored afterwards, because bun shares one process across files.
- `.github/workflows/e2e.yml` (node, deno and bun unit lines) and
`package.json` `test` now include the new file.

## RSR Quality Checklist

### Required

- [x] Tests pass. node `--test`: 68/68 across the four bridge test
files. `bun test`: 68/68. `deno test`: 68/68.
- [ ] Code is formatted: no formatter is configured for `mcp-bridge/`
JS. The code follows the surrounding style by hand.
- [ ] Linter is clean: no JS linter runs on `mcp-bridge/` locally. CI
scanners will report on this PR.
- [x] No banned language patterns. Plain ESM JS, as in the existing
bridge. Nothing new in TS, Python or Go. The `npm test` script line
already existed and only gained a file name.
- [x] No `unsafe` blocks: there is no Rust or Zig in this change.
- [x] No banned functions.
- [x] SPDX headers: the new test file carries `MPL-2.0`. The modified
files keep theirs.
- [x] No secrets, credentials or `.env` files.

### As Applicable

- [ ] `.machine_readable/*` not updated: project state and integrations
are unchanged.
- [ ] Documentation not updated: the advertised tool schemas are
unchanged. Only calls that already violated those schemas are now
refused.
- [ ] `TOPOLOGY.md` not updated: architecture is unchanged.
- [ ] CHANGELOG / release notes: to follow with the patch release that
ships this fix.
- [ ] New dependencies: none.
- [ ] ABI/FFI: not touched.

## Testing

- `node --test
mcp-bridge/tests/{routing_args,dispatch,http_transport,path_claims}_test.js`:
68 pass, 0 fail.
- `bun test` (same files): 68 pass, 0 fail. `deno test --allow-read
--allow-env --allow-run --allow-net` (same files): 68 passed.
- Control: `routing_args_test.js` against `origin/main`'s
`dispatcher.js` gives 8 pass and 8 fail. Every routing and
undeclared-argument test fails there; the well-formed-call and
non-routed tests pass.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

https://claude.ai/code/session_019j8She9eTFx54r6aL6sCHP

Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>

Author: hyperpolymath

.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:

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<string>|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<string, unknown>} 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<string, unknown>} 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<string, unknown>} args
+ * @returns {Promise<object|null>}
+ */
 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":

mcp-bridge/tests/routing_args_test.js

@@ -0,0 +1,85 @@
+// SPDX-License-Identifier: MPL-2.0
+// Copyright (c) Jonathan D.A. Jewell <j.d.a.jewell@open.ac.uk>
+//
+// 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));
+});

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"