From ef4b81f5a24cad63e2c9ae2f7bb8026aa8c371f9 Mon Sep 17 00:00:00 2001 From: riccardone Date: Tue, 4 Aug 2026 09:23:18 +0100 Subject: [PATCH] Reject bad tool arguments instead of silently dropping them MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Ports the bsp-mcp 1.7.2 fix to the BEST line, which had the defect unchanged. The tools declared `required`, but nothing enforced it. MCP hosts are not obliged to validate arguments, and hosts that don't forwarded whatever the model produced — so a misplaced key was silently dropped and failed in a way nobody could diagnose from the outside: - execute_query given send_command's `data` instead of `params` ran the query with NO parameters. An unfiltered query is usually one the caller may not run, so the endpoint answered "not authorised" — and two separate debugging sessions went hunting a permissions problem that did not exist. A genuine team admin concluded their API key lacked the role it plainly had. - get_query_schema without `version` fetched /queries/{schema}/undefined and reported a missing schema rather than a missing argument. handleExecuteQuery's existing `parameters` alias was aimed at exactly this and its comment names the failure mode, but an alias can only ever cover one more spelling — and `data` is a real field on a sibling tool, so it is a plausible mistake rather than a typo. Every tool call is now validated against that tool's own declared inputSchema before the handler runs: unknown keys and missing required keys both produce an error naming the offending key and the accepted ones, and stating that nothing was sent to the endpoint. The `data`-instead-of-`params` case gets an explicit hint, since that is the one that costs hours. Enforced server-side rather than by putting `additionalProperties: false` on the declared schemas: a host that does not validate is precisely the case this has to survive, and a host that does would then also reject argument keys the protocol may add later. Underscore-prefixed keys are ignored, by the convention that they carry host/protocol metadata rather than tool arguments — and ListTools already uses `_meta` itself. Also declares `poll_parameters` on send_command_and_wait. The handler has always accepted it as an alias of `poll_params` but the schema never listed it, so validating against the schema would have started rejecting a call that works today. Adds prepublishOnly: npm run build. dist/ is gitignored yet ships in the tarball, so the published artefact depends on whatever was last built locally — one forgotten build would publish stale code. Verified end to end by driving the built stdio server with raw JSON-RPC against a closed port, which makes "nothing was sent" checkable: `data` and both missing-argument cases return validation errors without a network call, while valid arguments and the poll_parameters alias get through to fetch. No version bump here — scripts/release-mcp.sh does that as part of cutting the release. Co-Authored-By: Claude Opus 5 --- mcp-server/package.json | 3 +- mcp-server/src/index.ts | 74 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 76 insertions(+), 1 deletion(-) diff --git a/mcp-server/package.json b/mcp-server/package.json index 868c428..a1828af 100644 --- a/mcp-server/package.json +++ b/mcp-server/package.json @@ -9,7 +9,8 @@ "scripts": { "build": "tsc", "dev": "tsx src/index.ts", - "start": "node dist/index.js" + "start": "node dist/index.js", + "prepublishOnly": "npm run build" }, "dependencies": { "@modelcontextprotocol/sdk": "^1.10.1" diff --git a/mcp-server/src/index.ts b/mcp-server/src/index.ts index 81e8564..6b94a1e 100644 --- a/mcp-server/src/index.ts +++ b/mcp-server/src/index.ts @@ -573,6 +573,11 @@ const TOOLS: Tool[] = [ description: 'Optional query parameters for the poll query.', additionalProperties: true }, + poll_parameters: { + type: 'object', + description: "Alias of 'poll_params' — both are accepted; if both are present, 'poll_params' wins.", + additionalProperties: true + }, timeout_seconds: { type: 'number', description: 'Maximum seconds to wait for the query to satisfy the condition (default: 30).' @@ -654,6 +659,68 @@ const TOOLS: Tool[] = [ inputSchema: { type: 'object', properties: { ...CONNECTION_PROP }, required: [] } } ]; +// ── Argument validation ─────────────────────────────────────────────────────── + +/** + * Host/protocol metadata is conventionally underscore-prefixed and is not part of a tool's contract, + * so it is never treated as an unknown argument. + */ +const isMetaKey = (key: string): boolean => key.startsWith('_'); + +/** + * Validates a tool call's arguments against that tool's own declared inputSchema, BEFORE the handler + * runs. Returns an error message, or null when the arguments are acceptable. + * + * Why this exists: the tools already declared `required`, but nothing enforced it. MCP hosts are not + * obliged to validate arguments, and the ones that don't forwarded whatever the model produced. A + * misplaced key was then silently dropped, which fails in a way nobody can diagnose from the outside: + * + * - `execute_query` given send_command's `data` instead of `params` ran the query with NO + * parameters. An unfiltered query is usually one the caller may not run, so the endpoint answered + * "not authorised" — and two separate debugging sessions went looking for a permissions problem + * that did not exist. + * - `get_query_schema` without `version` fetched `/queries/{schema}/undefined`, reporting a missing + * schema rather than a missing argument. + * + * The earlier fix for the first case added `parameters` as an alias of `params` (see + * handleExecuteQuery). That helped the single most common misspelling but could not help the general + * case — there is always another plausible name, and `data` is a real field on a sibling tool. + * + * Enforcement is deliberately server-side rather than `additionalProperties: false` on the declared + * schemas: a host that does not validate is exactly the situation this must survive, and a host that + * does would then also reject argument keys the protocol may add later. Validating here works + * regardless of what the host does, and lets the error name the offending key and the accepted ones. + */ +function validateToolArgs(name: string, args: Record): string | null { + const tool = TOOLS.find(t => t.name === name); + if (!tool) return null; // Unknown tool names are the dispatch's error to report, not ours. + + const schema = tool.inputSchema as { properties?: Record; required?: string[] }; + const accepted = Object.keys(schema.properties ?? {}); + + const unknown = Object.keys(args).filter(k => !isMetaKey(k) && !accepted.includes(k)); + if (unknown.length > 0) { + // The specific confusion worth naming, because it is the one that costs hours: 'data' is + // send_command's payload, and passing it to a query used to run that query unfiltered. + const hint = unknown.includes('data') && accepted.includes('params') + ? " Note: query parameters go in 'params' — 'data' is send_command's payload field." + : ''; + return `Unknown argument(s) for ${name}: ${unknown.join(', ')}. Accepted: ${accepted.join(', ')}.` + + `${hint} Nothing was sent to the endpoint — arguments are not silently ignored.`; + } + + const missing = (schema.required ?? []).filter(k => { + const value = args[k]; + return value === undefined || value === null || value === ''; + }); + if (missing.length > 0) { + return `Missing required argument(s) for ${name}: ${missing.join(', ')}. ` + + `Accepted: ${accepted.join(', ')}. Nothing was sent to the endpoint.`; + } + + return null; +} + // ── Tool handlers ───────────────────────────────────────────────────────────── function handleListConnections(): string { @@ -850,6 +917,13 @@ function createMcpServer(requestHeaders?: IncomingHttpHeaders): Server { const safeArgs = (args ?? {}) as Record; try { + // Before anything else, including connection resolution: a bad argument list is the caller's + // mistake to fix, and reporting it as such is the whole point (see validateToolArgs). + const argError = validateToolArgs(name, safeArgs); + if (argError) { + return { content: [{ type: 'text', text: `Error: ${argError}` }], isError: true }; + } + // list_connections needs no connection resolution if (name === 'list_connections') { return { content: [{ type: 'text', text: handleListConnections() }] };