From 60bbab4df7d011326bb98cdab85db88c3b5638e9 Mon Sep 17 00:00:00 2001 From: Kohron Burton Date: Tue, 8 Sep 2026 15:58:39 -0400 Subject: [PATCH 1/2] feat(governance): block secrets in MCP tool arguments --- server/src/plugins/content-governance.ts | 141 ++++++++++++++++++ server/src/plugins/store.ts | 31 ++++ server/tests/content-governance.test.ts | 65 ++++++++ server/tests/plugin-store.integration.test.ts | 31 ++++ 4 files changed, 268 insertions(+) create mode 100644 server/src/plugins/content-governance.ts create mode 100644 server/tests/content-governance.test.ts diff --git a/server/src/plugins/content-governance.ts b/server/src/plugins/content-governance.ts new file mode 100644 index 000000000..dc7dfd244 --- /dev/null +++ b/server/src/plugins/content-governance.ts @@ -0,0 +1,141 @@ +/** + * 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; + +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; +} + +/** + * 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, +): ToolArgumentInspection { + try { + const findings: SensitiveArgumentFinding[] = []; + const seen = new WeakSet(); + 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") { + 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)) { + const childPath = path ? `${path}.${key}` : key; + if ( + sensitiveFieldNames.has(normalizedFieldName(key)) && + 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: [] }; + } +} diff --git a/server/src/plugins/store.ts b/server/src/plugins/store.ts index 68671202f..410a25beb 100644 --- a/server/src/plugins/store.ts +++ b/server/src/plugins/store.ts @@ -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", + 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. * diff --git a/server/tests/content-governance.test.ts b/server/tests/content-governance.test.ts new file mode 100644 index 000000000..b089bf72e --- /dev/null +++ b/server/tests/content-governance.test.ts @@ -0,0 +1,65 @@ +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("fails closed on cyclic in-process input", () => { + const args: Record = {}; + args.self = args; + + expect(inspectToolArguments(args)).toEqual({ + safe: false, + reason: "inspection_limit", + findings: [], + }); + }); +}); diff --git a/server/tests/plugin-store.integration.test.ts b/server/tests/plugin-store.integration.test.ts index 04edb7686..da407716b 100644 --- a/server/tests/plugin-store.integration.test.ts +++ b/server/tests/plugin-store.integration.test.ts @@ -355,6 +355,37 @@ describe("a grant is the permission", () => { }); describe("the policy is asked as well as the grant", () => { + test("credential material is refused and never copied into the audit trail", async () => { + await store.grant("mcp", ref, holderId, "admin@openbot.local"); + const secret = `sk-${"z".repeat(32)}`; + + await expect( + store.callTool({ + ref, + args: { query: "quarterly report", nested: { apiKey: secret } }, + botId: holderId, + actorId: "someone@openbot.local", + }), + ).rejects.toThrow("credential material"); + + const rows = await auditRowsFor(ref); + const rejected = rows.find( + (row) => + row.eventType === "mcp.call_rejected" && + (row.payload as { refusal?: string }).refusal === + "sensitive_tool_arguments", + ); + expect(rejected).toBeDefined(); + expect(rejected?.payload).toMatchObject({ + bot: holderId, + contentInspection: { + reason: "sensitive_content", + findings: [{ category: "credential_field", path: "$.nested.apiKey" }], + }, + }); + expect(JSON.stringify(rejected)).not.toContain(secret); + }); + test("a granted tool is still refused by a deny rule, and the rule is named", async () => { await store.grant("mcp", ref, holderId, "admin@openbot.local"); policy = { From edc4ea79a54e608d5578bc90ac44a2fec2926469 Mon Sep 17 00:00:00 2001 From: Kohron Burton Date: Tue, 8 Sep 2026 16:10:32 -0400 Subject: [PATCH 2/2] fix(governance): redact argument keys and bound scans --- server/src/plugins/content-governance.ts | 21 ++++++++++- server/tests/content-governance.test.ts | 48 ++++++++++++++++++++++++ 2 files changed, 68 insertions(+), 1 deletion(-) diff --git a/server/src/plugins/content-governance.ts b/server/src/plugins/content-governance.ts index dc7dfd244..7b8e6381e 100644 --- a/server/src/plugins/content-governance.ts +++ b/server/src/plugins/content-governance.ts @@ -58,6 +58,7 @@ const providerTokenPatterns: RegExp[] = [ 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, "_"); @@ -76,6 +77,18 @@ function categoryForValue(value: string): SensitiveArgumentCategory | null { 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. * @@ -96,6 +109,7 @@ export function inspectToolArguments( 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 }); @@ -113,7 +127,12 @@ export function inspectToolArguments( } for (const [key, child] of Object.entries(value)) { - const childPath = path ? `${path}.${key}` : key; + 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)) && child !== null && diff --git a/server/tests/content-governance.test.ts b/server/tests/content-governance.test.ts index b089bf72e..9138f954a 100644 --- a/server/tests/content-governance.test.ts +++ b/server/tests/content-governance.test.ts @@ -52,6 +52,39 @@ describe("MCP tool argument content governance", () => { }); }); + 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 = {}; args.self = args; @@ -62,4 +95,19 @@ describe("MCP tool argument content governance", () => { 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: [], + }); + }); });