Skip to content

Commit 68c9e96

Browse files
fix(mcp-bridge): refuse undeclared arguments on every tool (#359)
## Summary Every MCP tool's `inputSchema` declares `additionalProperties: false`, but the bridge enforced that only for the 13 routed tools (browser/cloud/comms/ml, since #349). This PR enforces it for every tool in the full list, so a call carrying an argument its schema does not declare is refused with JSON-RPC `-32602` before dispatch. ## 📌 New pins Head SHA: **`f3b48f2b1d7c18f9f8f85a6db3312fd59b761d78`**. No action, lockfile or container pins added or changed. ## Changes - `mcp-bridge/lib/dispatcher.js`: `validateRoutedArgs` becomes `validateDeclaredArgs` and applies to every tool. The routing-key refusal for routed tools is kept. The deprecated `coord_promote_to_supervisor` alias is checked against `coord_promote_to_master`'s schema, and a tool missing from the full list is refused as `-32601 Unknown tool` in the gate. - `mcp-bridge/lib/tools.js`: declares arguments the handlers already read. - `sender_role` (optional enum) on `coord_send` and `coord_send_gated`. - `role` and `capabilities` on `coord_register`, copied from the local-coord-mcp `cartridge.json`, which already accepts them. - `mcp-bridge/tests/declared_args_test.js` (new): - one case per tool plus the alias, checking that an undeclared argument is refused; - one case per tool checking that every declared argument is accepted; - three end-to-end `tools/call` cases (non-routed, coord, unknown tool). - `mcp-bridge/tests/routing_args_test.js`: the old test asserted that `coord_send` accepts an undeclared `sender_role`. It now asserts that `coord_send` accepts the declared argument. - `package.json` `test` script runs the new file. `CHANGELOG.adoc` has a line under Unreleased. **Scope:** this checks top-level argument names only. Types, `required` and enums are unchanged. ## RSR Quality Checklist ### Required - [x] Tests pass: `bun test mcp-bridge/tests/` gives 210 pass, 0 fail, and `npm run test` (the repo's `node --test` script) gives 173 pass, 0 fail. - [ ] Code is formatted: the repo has no JS formatter configured for `mcp-bridge/`. I matched the surrounding style. - [ ] Linter is clean: no JS linter is configured for `mcp-bridge/`. Not run. - [x] No banned language patterns: plain `.js` only, no new TypeScript or Python. - [ ] No `unsafe` blocks without `// SAFETY:` comments: not applicable, no Rust or Zig touched. - [x] No banned functions. - [x] SPDX license headers present: the new test file carries the MPL-2.0 header used by its siblings. - [x] No secrets, credentials, or `.env` files included. ### As Applicable - [ ] `.machine_readable/*.a2ml`: not applicable; A2ML is retired (D308). - [x] Documentation updated for user-facing changes: `CHANGELOG.adoc`. - [ ] `TOPOLOGY.md`: not applicable, architecture unchanged. - [x] `CHANGELOG` updated. - [ ] New dependencies reviewed: not applicable, there are none. - [ ] ABI/FFI changes validated: not applicable, no `src/abi/` or `ffi/zig/` change. ## Pre-existing red checks (deferred) Both checks below are also red on `main` at `7dd5897d`. This PR does not touch the code either one covers. - `governance / UUID v7 conformance` is deferred to #347. - `SonarQube` is deferred to #338. ## Testing - **Suite:** - `bun test mcp-bridge/tests/`: 210 pass, 0 fail. - `npm run test`: 173 pass, 0 fail. - **Planted positive (mutant run):** I put the pre-change behaviour back temporarily (`if (!routingKey) return null;`, which checks routed tools only). With that in place, `declared_args_test.js` fails exactly 58 cases: 55 non-routed tools, the alias, and the two end-to-end refusals. 84 cases still pass. Then I restored the fix. - **Schema coverage:** - Every `args.<field>` read in `api-clients.js` (60 reads) is declared. I confirmed the check catches a missing field by planting one undeclared read. - The coord tool schemas match the local-coord-mcp manifest, apart from the two `coord_register` fields now added. - The per-tool table test calls `validateDeclaredArgs` directly, so the 60/min rate limiter cannot change which error a refusal returns. 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_019j8She9eTFx54r6aL6sCHP --------- Signed-off-by: Jonathan D.A. Jewell <6759885+hyperpolymath@users.noreply.github.com> Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
1 parent 7dd5897 commit 68c9e96

6 files changed

Lines changed: 142 additions & 17 deletions

File tree

‎CHANGELOG.adoc‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,16 @@ Returns / Errors / Usage text on every tool and the per-parameter
2828

2929
=== [Unreleased]
3030

31+
==== Changed
32+
33+
* *MCP bridge refuses undeclared tool arguments.* Every tool already
34+
declares `+additionalProperties: false+`; the bridge now enforces it for
35+
all tools, not only the routed browser/cloud/comms/ml ones. A call that
36+
passes an argument its `+inputSchema+` does not list fails with JSON-RPC
37+
`+-32602+` before dispatch. `+coord_send+`/`+coord_send_gated+` now
38+
declare `+sender_role+`, and `+coord_register+` declares `+role+` and
39+
`+capabilities+`, matching the local-coord-mcp manifest.
40+
3141
==== Added
3242

3343
* *`+k8s/networkpolicy.yaml+`* — defence-in-depth `+NetworkPolicy+`

‎mcp-bridge/lib/dispatcher.js‎

Lines changed: 22 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -55,20 +55,25 @@ function rpcError(id, code, message) {
5555

5656
// Tools that share one cartridge and are told apart by a routing key
5757
// derived from the tool name. The bridge sets the key, never the caller:
58-
// dispatch writes it last, and the gate refuses any argument the tool's
59-
// inputSchema does not declare.
58+
// dispatch writes it last, and the gate refuses it as an argument.
6059
const ROUTED_TOOLS = new Map([
6160
...["verpex", "cloudflare", "vercel"].map((p) => [`boj_cloud_${p}`, { cartridge: "cloud-mcp", key: "provider", value: p }]),
6261
...["gmail", "calendar"].map((p) => [`boj_comms_${p}`, { cartridge: "comms-mcp", key: "provider", value: p }]),
6362
["boj_ml_huggingface", { cartridge: "ml-mcp", key: "provider", value: "huggingface" }],
6463
...["navigate", "click", "type", "read_page", "screenshot", "tabs", "execute_js"].map((a) => [`boj_browser_${a}`, { cartridge: "browser-mcp", key: "action", value: a }]),
6564
]);
6665

66+
// Deprecated tool names still dispatched for one release. They take the
67+
// arguments of the tool they alias.
68+
const TOOL_ALIASES = new Map([
69+
["coord_promote_to_supervisor", "coord_promote_to_master"],
70+
]);
71+
6772
let declaredArgsCache = null;
6873

6974
/**
7075
* Return the argument names a tool's inputSchema declares, or null when
71-
* the tool is not in the full tool list.
76+
* the tool is not in the full tool list. Aliases resolve to their target.
7277
*
7378
* @param {string} toolName
7479
* @returns {Set<string>|null}
@@ -79,24 +84,25 @@ function declaredArgs(toolName) {
7984
buildToolList("full").map((t) => [t.name, new Set(Object.keys(t.inputSchema?.properties ?? {}))]),
8085
);
8186
}
82-
return declaredArgsCache.get(toolName) ?? null;
87+
return declaredArgsCache.get(TOOL_ALIASES.get(toolName) ?? toolName) ?? null;
8388
}
8489

8590
/**
86-
* Check a routed tool's arguments against its inputSchema. Other tools
87-
* are not checked here.
91+
* Check a tool's top-level argument names against its inputSchema: any
92+
* argument the schema does not declare is refused, and so is a routed
93+
* tool's routing key. Types, required fields and enums are not checked
94+
* here.
8895
*
8996
* @param {string} toolName
9097
* @param {Record<string, unknown>} args
9198
* @returns {string|null} error message, or null when every argument is declared
9299
*/
93-
function validateRoutedArgs(toolName, args) {
94-
const route = ROUTED_TOOLS.get(toolName);
95-
if (!route) return null;
100+
export function validateDeclaredArgs(toolName, args) {
96101
const allowed = declaredArgs(toolName);
97102
if (!allowed) return "Unknown tool";
103+
const routingKey = ROUTED_TOOLS.get(toolName)?.key;
98104
for (const name of Object.keys(args)) {
99-
if (name === route.key || !allowed.has(name)) {
105+
if (name === routingKey || !allowed.has(name)) {
100106
return `Unexpected argument '${name}' for ${toolName}`;
101107
}
102108
}
@@ -133,9 +139,12 @@ function hardeningGate(toolName, args) {
133139
if (args === null || typeof args !== "object" || Array.isArray(args)) {
134140
return { code: -32602, message: "Tool arguments must be an object" };
135141
}
136-
const routedError = validateRoutedArgs(toolName, args);
137-
if (routedError) {
138-
return { code: -32602, message: routedError };
142+
if (!declaredArgs(toolName)) {
143+
return { code: -32601, message: "Unknown tool" };
144+
}
145+
const argError = validateDeclaredArgs(toolName, args);
146+
if (argError) {
147+
return { code: -32602, message: argError };
139148
}
140149

141150
let validationError = null;

‎mcp-bridge/lib/tools.js‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -593,6 +593,16 @@ function buildToolList(scope) {
593593
context: { type: "string", description: "Optional disambiguator, e.g. current repo name. Alphanumeric + hyphen/underscore, max 32 bytes. Absent = plain `<kind>-<4hex>` form.", maxLength: 32, pattern: "^[A-Za-z0-9_-]*$" },
594594
declared_affinities: { type: "array", items: { type: "string", maxLength: 64 }, description: "Optional self-reported strength tags (e.g. ['proof-analysis', 'supervision']). Max 256 bytes as CSV; feeds reassignment-engine comparisons (DD-28)." },
595595
variant: { type: "string", description: "Optional free-form model/variant label set at register time (Task #33). Alphanumeric + `.`/`-`/`_`, max 32 bytes. e.g. `opus-4.7`, `flash-2.5`, `leanstral`. Equivalent to a follow-up `coord_set_variant` call.", maxLength: 32, pattern: "^[A-Za-z0-9._-]*$" },
596+
role: { type: "string", enum: ["journeyman", "apprentice", "executor", "supervised"], description: "Optional requested role (DD-32). `master` is always refused here; promote via `coord_promote_to_master`. Default: claude → journeyman, others → apprentice. `executor`/`supervised` are legacy aliases." },
597+
capabilities: {
598+
type: "object",
599+
description: "Optional capability advertisement for cold-start routing. Each key is optional; equivalent to a follow-up `coord_set_capabilities` call.",
600+
properties: {
601+
class: { type: "array", items: { type: "string" }, description: "Capability classes (e.g. 'reasoning', 'coding', 'proof'). Max 128 bytes as CSV." },
602+
tier: { type: "integer", minimum: 0, maximum: 5, description: "Advertised capability tier 1..5 (0 = unset)." },
603+
prover_strengths: { type: "array", items: { type: "string" }, description: "Prover names this peer claims strength in (e.g. 'coq', 'lean'). Max 256 bytes as CSV." },
604+
},
605+
},
596606
},
597607
required: ["client_kind"],
598608
additionalProperties: false,
@@ -642,6 +652,7 @@ function buildToolList(scope) {
642652
token: { type: "string", description: "Session token from `coord_register`." },
643653
target: { type: "string", description: "Peer ID to send to (e.g. `claude-a1b2@repo`), or `*` for broadcast to all active peers." },
644654
message: { type: "string", description: "Message payload — free-form text, typically a JSON A2ML envelope.", maxLength: 65536 },
655+
sender_role: { type: "string", enum: ["master", "journeyman", "apprentice", "executor", "supervised"], description: "Optional sender role for envelope contract validation, used when the envelope's own `_meta.sender_role` is absent." },
645656
},
646657
required: ["token", "target", "message"],
647658
additionalProperties: false,
@@ -790,6 +801,7 @@ function buildToolList(scope) {
790801
target: { type: "string", description: "Peer ID to send to, or `*` for broadcast." },
791802
message: { type: "string", description: "Message payload — typically a JSON A2ML envelope. Validated against `coord-messages.ncl` when strict mode is enabled.", maxLength: 65536 },
792803
risk_tier: { type: "integer", minimum: 0, maximum: 4, description: "Self-declared risk tier 0..4. 0=observational, 1=routine-read, 2=small-write, 3=major-write, 4=destructive/irreversible." },
804+
sender_role: { type: "string", enum: ["master", "journeyman", "apprentice", "executor", "supervised"], description: "Optional sender role for envelope contract validation, used when the envelope's own `_meta.sender_role` is absent." },
793805
},
794806
required: ["token", "target", "message", "risk_tier"],
795807
additionalProperties: false,
Lines changed: 95 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,95 @@
1+
// SPDX-License-Identifier: MPL-2.0
2+
// Copyright (c) Jonathan D.A. Jewell <j.d.a.jewell@open.ac.uk>
3+
//
4+
// BoJ Server — declared-argument tests
5+
//
6+
// Every tool's inputSchema sets additionalProperties:false. These tests pin
7+
// that the bridge enforces it for every tool in the full list (plus the
8+
// deprecated alias), not only the routed ones: an argument the schema does
9+
// not declare is refused before dispatch. `fetch` is stubbed so no backend
10+
// is needed.
11+
//
12+
// Run: node --test mcp-bridge/tests/declared_args_test.js
13+
14+
import { after, before, test } from "node:test";
15+
import assert from "node:assert/strict";
16+
17+
import { dispatchMcpMessage, validateDeclaredArgs } from "../lib/dispatcher.js";
18+
import { buildToolList } from "../lib/tools.js";
19+
20+
const posted = [];
21+
const realFetch = globalThis.fetch;
22+
before(() => {
23+
globalThis.fetch = async (url, init) => {
24+
posted.push({ url: String(url), body: init?.body ? JSON.parse(init.body) : null });
25+
return { ok: true, status: 200, json: async () => ({ ok: true }) };
26+
};
27+
});
28+
after(() => {
29+
globalThis.fetch = realFetch;
30+
});
31+
32+
let nextId = 1;
33+
/** Issue one tools/call and return the JSON-RPC response plus what was posted. */
34+
async function call(name, args) {
35+
posted.length = 0;
36+
const res = await dispatchMcpMessage({
37+
jsonrpc: "2.0",
38+
id: nextId++,
39+
method: "tools/call",
40+
params: { name, arguments: args },
41+
});
42+
return { res, sent: [...posted] };
43+
}
44+
45+
const TOOLS = buildToolList("full");
46+
const NAMES = [...TOOLS.map((t) => t.name), "coord_promote_to_supervisor"];
47+
48+
test("the full tool list is non-empty and every schema is closed", () => {
49+
assert.ok(TOOLS.length > 0);
50+
for (const t of TOOLS) assert.equal(t.inputSchema?.additionalProperties, false, t.name);
51+
});
52+
53+
// Checked directly rather than through tools/call, so the per-minute rate
54+
// limit cannot turn a refusal into a different error.
55+
for (const name of NAMES) {
56+
test(`${name}: an undeclared argument is refused`, () => {
57+
assert.equal(validateDeclaredArgs(name, { not_in_schema: 1 }), `Unexpected argument 'not_in_schema' for ${name}`);
58+
});
59+
}
60+
61+
for (const t of TOOLS) {
62+
test(`${t.name}: every declared argument is accepted`, () => {
63+
const args = Object.fromEntries(Object.keys(t.inputSchema.properties ?? {}).map((k) => [k, "x"]));
64+
const routed = t.name.match(/^boj_(browser|cloud|comms|ml)_/);
65+
if (routed) {
66+
delete args.action;
67+
delete args.provider;
68+
}
69+
assert.equal(validateDeclaredArgs(t.name, args), null);
70+
});
71+
}
72+
73+
test("a tool outside the list is unknown to the argument check", () => {
74+
assert.equal(validateDeclaredArgs("boj_not_a_tool", {}), "Unknown tool");
75+
});
76+
77+
test("tools/call refuses an undeclared argument on a non-routed tool before dispatch", async () => {
78+
const { res, sent } = await call("boj_search", { operation: "web", query: "q", not_in_schema: 1 });
79+
assert.ok(res.error, "expected the call to be refused");
80+
assert.equal(res.error.code, -32602);
81+
assert.match(res.error.message, /Unexpected argument 'not_in_schema'/);
82+
assert.equal(sent.length, 0);
83+
});
84+
85+
test("tools/call refuses an undeclared argument on a coord tool before dispatch", async () => {
86+
const { res, sent } = await call("coord_list_peers", { token: "t", not_in_schema: 1 });
87+
assert.ok(res.error, "expected the call to be refused");
88+
assert.equal(res.error.code, -32602);
89+
assert.equal(sent.length, 0);
90+
});
91+
92+
test("tools/call reports an unlisted tool as unknown", async () => {
93+
const { res } = await call("boj_not_a_tool", {});
94+
assert.equal(res.error?.code, -32601);
95+
});

‎mcp-bridge/tests/routing_args_test.js‎

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -78,8 +78,7 @@ test("routed tools refuse arguments outside their inputSchema", async () => {
7878
assert.equal(sent.length, 0);
7979
});
8080

81-
test("non-routed tools keep their existing argument handling", async () => {
82-
// coord_send reads `sender_role`, which no inputSchema declares.
83-
const { res } = await call("coord_send", { message: "hi", target: "peer", sender_role: "worker" });
81+
test("coord_send accepts its declared sender_role argument", async () => {
82+
const { res } = await call("coord_send", { message: "hi", target: "peer", sender_role: "apprentice" });
8483
assert.equal(res.error, undefined, JSON.stringify(res.error));
8584
});

‎package.json‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,7 @@
2020
},
2121
"scripts": {
2222
"start": "deno run -A mcp-bridge/main.js",
23-
"test": "node --test mcp-bridge/tests/dispatch_test.js mcp-bridge/tests/routing_args_test.js"
23+
"test": "node --test mcp-bridge/tests/dispatch_test.js mcp-bridge/tests/routing_args_test.js mcp-bridge/tests/declared_args_test.js"
2424
},
2525
"engines": {
2626
"node": ">=18.0.0"

0 commit comments

Comments
 (0)