diff --git a/internal/rules/policies_test.go b/internal/rules/policies_test.go index 65af6c0a..27312416 100644 --- a/internal/rules/policies_test.go +++ b/internal/rules/policies_test.go @@ -1388,6 +1388,56 @@ def run_cmd(name: str) -> str: kind: models.KindMCPTool, lang: models.LanguageRust, wantFires: false, src: "use rmcp::tool;\nimpl T {\n #[tool(description = \"Summarize\")]\n fn summarize_invoice(&self) -> String { String::new() }\n}\n", }, + { + name: "MCP-027 fires on camelCase mutating tool with no idempotency param", ruleID: "MCP-027", + kind: models.KindMCPTool, lang: models.LanguageTypeScript, wantFires: true, + src: "import { McpServer } from \"@modelcontextprotocol/sdk/server/mcp.js\";\n" + + "const server = new McpServer({ name: \"s\", version: \"1.0.0\" });\n" + + "server.registerTool(\"createCharge\", { description: \"Charge a card\", inputSchema: { amount: z.number() } }, async ({ amount }) => ({ content: [{ type: \"text\", text: String(amount) }] }));\n", + }, + { + name: "MCP-027 silent when idempotency key present", ruleID: "MCP-027", + kind: models.KindMCPTool, lang: models.LanguageTypeScript, wantFires: false, + src: "import { McpServer } from \"@modelcontextprotocol/sdk/server/mcp.js\";\n" + + "const server = new McpServer({ name: \"s\", version: \"1.0.0\" });\n" + + "server.registerTool(\"createCharge\", { description: \"Charge a card\", inputSchema: { amount: z.number(), idempotencyKey: z.string() } }, async ({ amount }) => ({ content: [{ type: \"text\", text: String(amount) }] }));\n", + }, + { + name: "MCP-027 silent on non-mutating tool name", ruleID: "MCP-027", + kind: models.KindMCPTool, lang: models.LanguageTypeScript, wantFires: false, + src: "import { McpServer } from \"@modelcontextprotocol/sdk/server/mcp.js\";\n" + + "const server = new McpServer({ name: \"s\", version: \"1.0.0\" });\n" + + "server.registerTool(\"getBalance\", { description: \"Read balance\", inputSchema: { id: z.string() } }, async ({ id }) => ({ content: [{ type: \"text\", text: id }] }));\n", + }, + { + name: "MCP-028 fires on ambiguous TypeScript tool name", ruleID: "MCP-028", + kind: models.KindMCPTool, lang: models.LanguageTypeScript, wantFires: true, + src: "import { McpServer } from \"@modelcontextprotocol/sdk/server/mcp.js\";\n" + + "const server = new McpServer({ name: \"s\", version: \"1.0.0\" });\n" + + "server.registerTool(\"process\", { description: \"Does a thing\", inputSchema: { x: z.string() } }, async ({ x }) => ({ content: [{ type: \"text\", text: x }] }));\n", + }, + { + name: "MCP-028 silent on descriptive name", ruleID: "MCP-028", + kind: models.KindMCPTool, lang: models.LanguageTypeScript, wantFires: false, + src: "import { McpServer } from \"@modelcontextprotocol/sdk/server/mcp.js\";\n" + + "const server = new McpServer({ name: \"s\", version: \"1.0.0\" });\n" + + "server.registerTool(\"summarizeInvoice\", { description: \"Summarize an invoice\", inputSchema: { x: z.string() } }, async ({ x }) => ({ content: [{ type: \"text\", text: x }] }));\n", + }, + { + name: "MCP-029 fires on PHP mutating tool without key", ruleID: "MCP-029", + kind: models.KindMCPTool, lang: models.LanguagePHP, wantFires: true, + src: " + This TypeScript MCP tool's name implies a side effect (create/send/refund/…) + but its inputSchema exposes no idempotency parameter. MCP clients retry + tool calls under timeouts and ambiguous failures, the model may repeat the + action when a result reads as inconclusive, and a lost response after the + side effect committed is at-least-once delivery — so the same charge, send, + or delete can fire twice. Without a key the connecting client cannot make + the retry safe. The prefix set matches both `create_charge` and + `createCharge` naming, so it fires on idiomatic TypeScript tool names. + fix: > + Add an `idempotencyKey` (or `requestId`) field to the tool's inputSchema, + thread it to the downstream API, and confirm that API treats a repeated + key as a no-op rather than a second mutation. + + - id: MCP-029 + title: PHP MCP mutating tool has no idempotency key + severity: medium + confidence: 0.55 + language: php + applies_to: + - mcp_tool + scope: tool + match: + all: + - name_has_prefix: + - create_ + - send_ + - delete_ + - post_ + - update_ + - refund_ + - charge_ + - issue_ + - not: + param_name_matches: + contains: + - idempot + exact: + - request_id + - txn_id + explanation: > + This PHP MCP tool's name signals a side effect (create/send/refund/…) but + the handler takes no idempotency-key parameter. MCP clients retry tool + calls under timeouts and ambiguous failures, and a lost response after the + side effect committed is at-least-once delivery — so the same charge, + order, or message can fire twice. Without a key the handler executes the + mutation twice. + fix: > + Accept an idempotency key parameter (idempotency_key / request_id) on the + #[McpTool] method and de-duplicate server-side so a retried call is a + no-op after the first success. diff --git a/testdata/rules-fixture/mcp/tool_definition.yaml b/testdata/rules-fixture/mcp/tool_definition.yaml index 1e843121..fb9f52b1 100644 --- a/testdata/rules-fixture/mcp/tool_definition.yaml +++ b/testdata/rules-fixture/mcp/tool_definition.yaml @@ -309,3 +309,34 @@ rules: fix: > Rename the method (or set the `#[tool]` `name = "..."` argument) to a verb-object form, e.g. `summarize_invoice`, `fetch_weather`. + + - id: MCP-028 + title: Ambiguous TypeScript MCP tool name + severity: low + confidence: 0.85 + language: typescript + applies_to: + - mcp_tool + scope: tool + match: + name_in: + - process + - handle + - run + - do + - execute + - perform + - work + - go + - thing + - stuff + explanation: > + A TypeScript MCP tool's name is the first argument to + `server.registerTool(...)` (or the legacy `server.tool(...)`). Names like + `process`, `handle`, or `run` give a connecting model no signal about + intent. Because an MCP server is consumed by clients the author does not + control, an ambiguous name degrades tool selection everywhere the server is + mounted and collides more easily with similarly-named tools from other + servers in the same session. + fix: > + Rename to a verb-object form, e.g. `summarizeInvoice`, `fetchWeather`.