-
Notifications
You must be signed in to change notification settings - Fork 685
Block credential material in MCP tool arguments #436
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,160 @@ | ||
| /** | ||
| * Deterministic inspection of arguments before an MCP call leaves this deployment. | ||
| * | ||
| * This deliberately detects credentials, not general PII. A broad expression such as an email or | ||
| * phone-number matcher would block ordinary connector work and turn a security boundary into a | ||
| * source of false assurances. The findings contain only a category and a structural path: the | ||
| * matched value must never be copied into an error, log, or audit row. | ||
| */ | ||
|
|
||
| export type SensitiveArgumentCategory = | ||
| | "credential_field" | ||
| | "private_key" | ||
| | "provider_token" | ||
| | "authorization_header"; | ||
|
|
||
| export type SensitiveArgumentFinding = { | ||
| category: SensitiveArgumentCategory; | ||
| path: string; | ||
| }; | ||
|
|
||
| export type ToolArgumentInspection = | ||
| | { safe: true } | ||
| | { | ||
| safe: false; | ||
| reason: "sensitive_content" | "inspection_limit" | "inspection_failed"; | ||
| findings: SensitiveArgumentFinding[]; | ||
| }; | ||
|
|
||
| const sensitiveFieldNames = new Set([ | ||
| "access_token", | ||
| "accesstoken", | ||
| "api_key", | ||
| "apikey", | ||
| "authorization", | ||
| "client_secret", | ||
| "clientsecret", | ||
| "credential", | ||
| "credentials", | ||
| "id_token", | ||
| "idtoken", | ||
| "password", | ||
| "private_key", | ||
| "privatekey", | ||
| "refresh_token", | ||
| "refreshtoken", | ||
| "secret", | ||
| "token", | ||
| ]); | ||
|
|
||
| const providerTokenPatterns: RegExp[] = [ | ||
| /\bsk-[A-Za-z0-9_-]{20,}\b/, | ||
| /\bgh[pousr]_[A-Za-z0-9]{20,}\b/, | ||
| /\bgithub_pat_[A-Za-z0-9_]{20,}\b/, | ||
| /\bAKIA[A-Z0-9]{16}\b/, | ||
| /\bxox[baprs]-[A-Za-z0-9-]{10,}\b/, | ||
| ]; | ||
|
|
||
| const MAX_NODES = 2_000; | ||
| const MAX_DEPTH = 20; | ||
| const MAX_FINDINGS = 20; | ||
| const MAX_STRING_LENGTH = 64 * 1024; | ||
|
|
||
| function normalizedFieldName(value: string): string { | ||
| return value.toLowerCase().replace(/[-.\s]/g, "_"); | ||
| } | ||
|
|
||
| function categoryForValue(value: string): SensitiveArgumentCategory | null { | ||
| if (/-----BEGIN (?:[A-Z ]+ )?PRIVATE KEY-----/.test(value)) { | ||
| return "private_key"; | ||
| } | ||
| if (/^\s*(?:Basic|Bearer)\s+\S+/i.test(value)) { | ||
| return "authorization_header"; | ||
| } | ||
| if (providerTokenPatterns.some((pattern) => pattern.test(value))) { | ||
| return "provider_token"; | ||
| } | ||
| return null; | ||
| } | ||
|
|
||
| /** | ||
| * A path is audit metadata, so it cannot repeat arbitrary argument keys. Keep ordinary schema-like | ||
| * names useful and replace everything else with a structural marker. In particular, a credential | ||
| * smuggled in a property name is detected but never copied into the finding that records it. | ||
| */ | ||
| function pathForKey(parent: string, key: string): string { | ||
| const segment = /^[A-Za-z_][A-Za-z0-9_-]{0,63}$/.test(key) | ||
| ? key | ||
| : "[property]"; | ||
| return `${parent}.${segment}`; | ||
| } | ||
|
|
||
| /** | ||
| * Inspect JSON-shaped tool arguments without serialising them. | ||
| * | ||
| * JSON received by the route cannot be cyclic, but the store is also callable in-process. A | ||
| * WeakSet makes that path fail closed rather than recurse forever. Size and depth limits bound the | ||
| * work an authenticated but compromised Bot can ask this gateway to perform. | ||
| */ | ||
| export function inspectToolArguments( | ||
| args: Record<string, unknown>, | ||
| ): ToolArgumentInspection { | ||
| try { | ||
| const findings: SensitiveArgumentFinding[] = []; | ||
| const seen = new WeakSet<object>(); | ||
| let nodes = 0; | ||
|
|
||
| const visit = (value: unknown, path: string, depth: number): boolean => { | ||
| nodes += 1; | ||
| if (nodes > MAX_NODES || depth > MAX_DEPTH) return false; | ||
|
|
||
| if (typeof value === "string") { | ||
| if (value.length > MAX_STRING_LENGTH) return false; | ||
| const category = categoryForValue(value); | ||
| if (category && findings.length < MAX_FINDINGS) { | ||
| findings.push({ category, path }); | ||
| } | ||
| return true; | ||
| } | ||
| if (value === null || typeof value !== "object") return true; | ||
| if (seen.has(value)) return false; | ||
| seen.add(value); | ||
|
|
||
| if (Array.isArray(value)) { | ||
| return value.every((item, index) => | ||
| visit(item, `${path}[${index}]`, depth + 1), | ||
| ); | ||
| } | ||
|
|
||
| for (const [key, child] of Object.entries(value)) { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
On an authenticated call containing a very wide object, Useful? React with 👍 / 👎. |
||
| if (key.length > MAX_STRING_LENGTH) return false; | ||
| const keyCategory = categoryForValue(key); | ||
| const childPath = pathForKey(path, keyCategory ? "[credential]" : key); | ||
| if (keyCategory && findings.length < MAX_FINDINGS) { | ||
| findings.push({ category: keyCategory, path: childPath }); | ||
| } | ||
| if ( | ||
| sensitiveFieldNames.has(normalizedFieldName(key)) && | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When a tool accepts a standard compound credential field such as Useful? React with 👍 / 👎. |
||
| child !== null && | ||
| child !== "" | ||
| ) { | ||
| if (findings.length < MAX_FINDINGS) { | ||
| findings.push({ category: "credential_field", path: childPath }); | ||
| } | ||
| continue; | ||
| } | ||
| if (!visit(child, childPath, depth + 1)) return false; | ||
| } | ||
| return true; | ||
| }; | ||
|
|
||
| if (!visit(args, "$", 0)) { | ||
| return { safe: false, reason: "inspection_limit", findings: [] }; | ||
| } | ||
| return findings.length === 0 | ||
| ? { safe: true } | ||
| : { safe: false, reason: "sensitive_content", findings }; | ||
| } catch { | ||
| return { safe: false, reason: "inspection_failed", findings: [] }; | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -39,6 +39,7 @@ import { | |
| resolveServerUrl, | ||
| serverCredentialKind, | ||
| } from "./catalogue"; | ||
| import { inspectToolArguments } from "./content-governance"; | ||
| import { McpServerError } from "./mcp"; | ||
| import { registerDynamicClient } from "./oauth"; | ||
| import { transportFor } from "./transport"; | ||
|
|
@@ -2942,6 +2943,36 @@ export function createPluginStore(options: PluginStoreOptions) { | |
| throw new PluginRefusedError(verdict.reason, verdict.matched); | ||
| } | ||
|
|
||
| /** | ||
| * Structural policy answers whether this Bot may call this tool. Content inspection answers | ||
| * whether the arguments would carry a credential out of the deployment. It runs after policy | ||
| * and before credentials are read or a vendor is contacted, and its result contains paths and | ||
| * categories only: never the values it refused. | ||
| */ | ||
| const contentDecision = inspectToolArguments(args); | ||
| if (!contentDecision.safe) { | ||
| await recordAuditEvent(auditStore, { | ||
| eventType: "mcp.call_rejected", | ||
| targetType: "mcp_tool", | ||
| targetId: input.ref, | ||
| ...(input.initiator ? { initiator: input.initiator } : {}), | ||
| payload: { | ||
| ...decided, | ||
| refusal: "sensitive_tool_arguments", | ||
|
Comment on lines
+2959
to
+2961
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When a policy deny rule is in Useful? React with 👍 / 👎. |
||
| contentInspection: { | ||
| reason: contentDecision.reason, | ||
| findings: contentDecision.findings, | ||
| }, | ||
| }, | ||
| }); | ||
| throw new PluginRefusedError( | ||
| contentDecision.reason === "sensitive_content" | ||
| ? "The tool call was refused because its arguments contain credential material." | ||
| : "The tool call was refused because its arguments could not be inspected safely.", | ||
| null, | ||
| ); | ||
| } | ||
|
|
||
| /* | ||
| * Attempt first, record second. | ||
| * | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,113 @@ | ||
| import { describe, expect, test } from "bun:test"; | ||
| import { inspectToolArguments } from "../src/plugins/content-governance"; | ||
|
|
||
| describe("MCP tool argument content governance", () => { | ||
| test("allows ordinary nested business data", () => { | ||
| expect( | ||
| inspectToolArguments({ | ||
| query: "quarterly report", | ||
| filters: { ownerEmail: "owner@example.com", limit: 25 }, | ||
| rows: [{ customer: "Acme", amount: 1200 }], | ||
| }), | ||
| ).toEqual({ safe: true }); | ||
| }); | ||
|
|
||
| test("reports a sensitive field without returning its value", () => { | ||
| const secret = "do-not-copy-this-value"; | ||
| const result = inspectToolArguments({ nested: { apiKey: secret } }); | ||
|
|
||
| expect(result).toEqual({ | ||
| safe: false, | ||
| reason: "sensitive_content", | ||
| findings: [{ category: "credential_field", path: "$.nested.apiKey" }], | ||
| }); | ||
| expect(JSON.stringify(result)).not.toContain(secret); | ||
| }); | ||
|
|
||
| test("detects provider tokens embedded in otherwise ordinary text", () => { | ||
| const result = inspectToolArguments({ | ||
| message: `please use sk-${"a".repeat(32)} for this request`, | ||
| }); | ||
|
|
||
| expect(result).toEqual({ | ||
| safe: false, | ||
| reason: "sensitive_content", | ||
| findings: [{ category: "provider_token", path: "$.message" }], | ||
| }); | ||
| }); | ||
|
|
||
| test("detects authorization headers and private keys", () => { | ||
| const result = inspectToolArguments({ | ||
| headers: ["Bearer eyJhbGciOiJIUzI1NiJ9.payload.signature"], | ||
| material: "-----BEGIN PRIVATE KEY-----\nredacted", | ||
| }); | ||
|
|
||
| expect(result).toEqual({ | ||
| safe: false, | ||
| reason: "sensitive_content", | ||
| findings: [ | ||
| { category: "authorization_header", path: "$.headers[0]" }, | ||
| { category: "private_key", path: "$.material" }, | ||
| ], | ||
| }); | ||
| }); | ||
|
|
||
| test("detects credential material in a property name without recording it", () => { | ||
| const secret = `ghp_${"a".repeat(32)}`; | ||
| const result = inspectToolArguments({ | ||
| nested: { [secret]: "ordinary value" }, | ||
| }); | ||
|
|
||
| expect(result).toEqual({ | ||
| safe: false, | ||
| reason: "sensitive_content", | ||
| findings: [ | ||
| { | ||
| category: "provider_token", | ||
| path: "$.nested.[property]", | ||
| }, | ||
| ], | ||
| }); | ||
| expect(JSON.stringify(result)).not.toContain(secret); | ||
| }); | ||
|
|
||
| test("does not copy arbitrary property names into audit-safe paths", () => { | ||
| const privateKey = "-----BEGIN PRIVATE KEY-----\nredacted"; | ||
| const result = inspectToolArguments({ | ||
| "customer@example.com": { material: privateKey }, | ||
| }); | ||
|
|
||
| expect(result).toEqual({ | ||
| safe: false, | ||
| reason: "sensitive_content", | ||
| findings: [{ category: "private_key", path: "$.[property].material" }], | ||
| }); | ||
| expect(JSON.stringify(result)).not.toContain("customer@example.com"); | ||
| }); | ||
|
|
||
| test("fails closed on cyclic in-process input", () => { | ||
| const args: Record<string, unknown> = {}; | ||
| args.self = args; | ||
|
|
||
| expect(inspectToolArguments(args)).toEqual({ | ||
| safe: false, | ||
| reason: "inspection_limit", | ||
| findings: [], | ||
| }); | ||
| }); | ||
|
|
||
| test("fails closed before scanning oversized strings or property names", () => { | ||
| const oversized = "a".repeat(64 * 1024 + 1); | ||
|
|
||
| expect(inspectToolArguments({ text: oversized })).toEqual({ | ||
| safe: false, | ||
| reason: "inspection_limit", | ||
| findings: [], | ||
| }); | ||
| expect(inspectToolArguments({ [oversized]: "value" })).toEqual({ | ||
| safe: false, | ||
| reason: "inspection_limit", | ||
| findings: [], | ||
| }); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When a credential appears inside an ordinary argument, such as
curl -H 'Authorization: Bearer <opaque-token>', the start anchor prevents this expression from recognizing it. An opaque OAuth token need not match any provider-specific pattern, so prepending explanatory text turns a blocked bearer token into asafe: trueresult and sends it to the vendor.Useful? React with 👍 / 👎.