diff --git a/README.md b/README.md index 7480b79..3e8a0ab 100644 --- a/README.md +++ b/README.md @@ -80,7 +80,7 @@ The process exits `1` when any finding reaches the `--fail-on` threshold (defaul ## Rule catalog -18 built-in rules, each with a stable id, severity, and remediation. Toggle any of +19 built-in rules, each with a stable id, severity, and remediation. Toggle any of them via config. | Rule | Severity | Category | Description | @@ -88,6 +88,7 @@ them via config. | `MCP001` | high | permissions | Destructive tool lacks scoping or confirmation | | `MCP002` | critical | permissions | Tool exposes arbitrary command execution | | `MCP003` | medium | permissions | Mutating tool has an unscoped input surface | +| `MCP004` | high | permissions | Mutating tool advertises readOnlyHint | | `MCP010` | medium | schema | Tool is missing an input schema | | `MCP011` | low | schema | Schema allows unbounded additional properties | | `MCP012` | low | schema | Unconstrained string argument | diff --git a/src/rules/permissions.ts b/src/rules/permissions.ts index 3197c32..71fbf13 100644 --- a/src/rules/permissions.ts +++ b/src/rules/permissions.ts @@ -94,6 +94,40 @@ export const execTool: Rule = { * A mutating tool that accepts a free-form object with no properties can write * anything anywhere. */ +/** + * MCP004 - Mutating/destructive tool advertises readOnlyHint. + * Clients use readOnlyHint for auto-approval; marking a write tool read-only + * is a confused-deputy / approval-bypass primitive. + */ +export const readOnlyHintMismatch: Rule = { + id: "MCP004", + title: "Mutating tool advertises readOnlyHint", + description: + "A tool that mutates or deletes state must not advertise readOnlyHint, which clients use to auto-approve safe reads.", + severity: "high", + category: "permissions", + evaluate(target, ctx): Finding[] { + const mutatingVerbs = [...WRITE_VERBS, ...DESTRUCTIVE_VERBS]; + const findings: Finding[] = []; + for (const tool of target.tools) { + if (tool.annotations?.readOnlyHint !== true) continue; + const haystack = `${tool.name} ${tool.description ?? ""}`; + const hit = containsAny(haystack, mutatingVerbs); + if (!hit) continue; + findings.push( + ctx.report({ + title: "Write tool incorrectly marked read-only", + message: `Tool "${tool.name}" advertises readOnlyHint but implies a mutating action ("${hit}").`, + remediation: + "Set readOnlyHint to false (or omit it) for tools that delete, update, or otherwise change state.", + location: tool.name, + }), + ); + } + return findings; + }, +}; + export const unscopedWriteTool: Rule = { id: "MCP003", title: "Mutating tool has an unscoped input surface", @@ -128,5 +162,6 @@ export const unscopedWriteTool: Rule = { export const permissionRules: Rule[] = [ destructiveNoScoping, execTool, + readOnlyHintMismatch, unscopedWriteTool, ]; diff --git a/src/types.ts b/src/types.ts index 835d3a1..c1ca8aa 100644 --- a/src/types.ts +++ b/src/types.ts @@ -34,11 +34,21 @@ export interface JsonSchema { [key: string]: unknown; } +/** MCP tool annotations (e.g. hints for clients and auto-approval). */ +export interface ToolAnnotations { + readOnlyHint?: boolean; + destructiveHint?: boolean; + idempotentHint?: boolean; + openWorldHint?: boolean; + [key: string]: unknown; +} + /** A tool as enumerated from an MCP server (or a static manifest). */ export interface ToolSpec { name: string; description?: string; inputSchema?: JsonSchema; + annotations?: ToolAnnotations; } /** A resource exposed by an MCP server. */ diff --git a/test/rules.test.ts b/test/rules.test.ts index 4a5f9bb..9eb9f29 100644 --- a/test/rules.test.ts +++ b/test/rules.test.ts @@ -58,6 +58,46 @@ describe("rules against the insecure surface", () => { } }); +describe("MCP004 readOnlyHint mismatch", () => { + function findingsFor(tools: AuditTarget["tools"]) { + return audit(makeTarget({ tools })).findings.filter((f) => f.ruleId === "MCP004"); + } + + it("fires when a write-verb tool advertises readOnlyHint", () => { + const findings = findingsFor([ + { + name: "cleanup_records", + description: "deletes stale records", + annotations: { readOnlyHint: true }, + }, + ]); + expect(findings).toHaveLength(1); + expect(findings[0].location).toBe("cleanup_records"); + }); + + it("does not fire when readOnlyHint is false", () => { + const findings = findingsFor([ + { + name: "cleanup_records", + description: "deletes stale records", + annotations: { readOnlyHint: false }, + }, + ]); + expect(findings).toHaveLength(0); + }); + + it("does not fire for a read-only tool with readOnlyHint", () => { + const findings = findingsFor([ + { + name: "get_weather", + description: "returns weather for a city", + annotations: { readOnlyHint: true }, + }, + ]); + expect(findings).toHaveLength(0); + }); +}); + describe("MCP002 exec detection", () => { it("flags a shell tool as critical", () => { const target = makeTarget({