From 2d6238bb7065270f387b4b43201a351775bf4d6e Mon Sep 17 00:00:00 2001 From: Dotta Date: Wed, 10 Jun 2026 22:32:10 +0000 Subject: [PATCH] Harden tool action argument handling --- .env.example | 1 + doc/DOCKER.md | 3 + doc/MCP-RUNTIME-OPERATIONS.md | 2 + .../src/__tests__/tool-content-guards.test.ts | 42 +++++++++++- .../__tests__/tool-gateway-service.test.ts | 65 +++++++++++++++++++ server/src/services/tool-content-guards.ts | 11 ++-- server/src/services/tool-gateway.ts | 16 +++-- 7 files changed, 126 insertions(+), 14 deletions(-) diff --git a/.env.example b/.env.example index 84e20d96058..e747df0914d 100644 --- a/.env.example +++ b/.env.example @@ -2,6 +2,7 @@ DATABASE_URL=postgres://paperclip:paperclip@localhost:5432/paperclip PORT=3100 SERVE_UI=false BETTER_AUTH_SECRET=paperclip-dev-secret +PAPERCLIP_TOOL_ACTION_SIGNING_SECRET=paperclip-dev-tool-action-signing-secret-change-me # Discord webhook for daily merge digest (scripts/discord-daily-digest.sh) # DISCORD_WEBHOOK_URL=https://discord.com/api/webhooks/... diff --git a/doc/DOCKER.md b/doc/DOCKER.md index bc1fae8cae9..b29fc2b147e 100644 --- a/doc/DOCKER.md +++ b/doc/DOCKER.md @@ -33,6 +33,7 @@ docker run --name paperclip \ -e HOST=0.0.0.0 \ -e PAPERCLIP_HOME=/paperclip \ -e BETTER_AUTH_SECRET=$(openssl rand -hex 32) \ + -e PAPERCLIP_TOOL_ACTION_SIGNING_SECRET=$(openssl rand -hex 32) \ -v "$(pwd)/data/docker-paperclip:/paperclip" \ paperclip-local ``` @@ -56,6 +57,7 @@ Single container, no external database. Data persists via a bind mount. ```sh BETTER_AUTH_SECRET=$(openssl rand -hex 32) \ +PAPERCLIP_TOOL_ACTION_SIGNING_SECRET=$(openssl rand -hex 32) \ docker compose -f docker/docker-compose.quickstart.yml up --build ``` @@ -187,6 +189,7 @@ The `docker/quadlet/` directory contains unit files to run Paperclip + PostgreSQ ```sh cat > ~/.config/containers/systemd/paperclip.env < { + const originalToolActionSigningSecret = process.env.PAPERCLIP_TOOL_ACTION_SIGNING_SECRET; + const originalAgentJwtSecret = process.env.PAPERCLIP_AGENT_JWT_SECRET; + const originalAuthSecret = process.env.BETTER_AUTH_SECRET; + + beforeEach(() => { + process.env.PAPERCLIP_TOOL_ACTION_SIGNING_SECRET = "test-tool-action-signing-secret"; + }); + + afterEach(() => { + if (originalToolActionSigningSecret === undefined) { + delete process.env.PAPERCLIP_TOOL_ACTION_SIGNING_SECRET; + } else { + process.env.PAPERCLIP_TOOL_ACTION_SIGNING_SECRET = originalToolActionSigningSecret; + } + if (originalAgentJwtSecret === undefined) { + delete process.env.PAPERCLIP_AGENT_JWT_SECRET; + } else { + process.env.PAPERCLIP_AGENT_JWT_SECRET = originalAgentJwtSecret; + } + if (originalAuthSecret === undefined) { + delete process.env.BETTER_AUTH_SECRET; + } else { + process.env.BETTER_AUTH_SECRET = originalAuthSecret; + } + }); + it("signs canonical arguments and rejects tampered arguments", () => { const canonicalArguments = canonicalToolArguments({ body: "hello", noteId: "n1" }); const signedArguments = signToolArguments({ @@ -40,6 +66,20 @@ describe("tool content guards", () => { })).toEqual({ body: "hello", noteId: "n1" }); }); + it("requires a dedicated tool action signing secret", () => { + delete process.env.PAPERCLIP_TOOL_ACTION_SIGNING_SECRET; + process.env.PAPERCLIP_AGENT_JWT_SECRET = "agent-jwt-secret"; + process.env.BETTER_AUTH_SECRET = "auth-secret"; + + expect(() => + signToolArguments({ + invocationId: "invocation-1", + toolName: "mcp-remote-fixture:update_note", + canonicalArguments: canonicalToolArguments({ body: "hello" }), + }), + ).toThrow("PAPERCLIP_TOOL_ACTION_SIGNING_SECRET"); + }); + it("redacts sensitive argument values before summarizing them", () => { const result = validateToolContent({ value: { query: "ok", apiKey: "sk-secret-value" }, diff --git a/server/src/__tests__/tool-gateway-service.test.ts b/server/src/__tests__/tool-gateway-service.test.ts index 0746a920c33..de1ff69cc5a 100644 --- a/server/src/__tests__/tool-gateway-service.test.ts +++ b/server/src/__tests__/tool-gateway-service.test.ts @@ -30,6 +30,7 @@ import { const embeddedPostgresSupport = await getEmbeddedPostgresTestSupport(); const describeEmbeddedPostgres = embeddedPostgresSupport.supported ? describe : describe.skip; +const originalToolActionSigningSecret = process.env.PAPERCLIP_TOOL_ACTION_SIGNING_SECRET; async function createRunFixture(db: ReturnType) { const company = await db.insert(companies).values({ @@ -94,6 +95,7 @@ describeEmbeddedPostgres("tool gateway service", () => { let tempDb: Awaited> | null = null; beforeAll(async () => { + process.env.PAPERCLIP_TOOL_ACTION_SIGNING_SECRET = "test-tool-action-signing-secret"; tempDb = await startEmbeddedPostgresTestDatabase("paperclip-tool-gateway-"); db = createDb(tempDb.connectionString); }, 20_000); @@ -117,6 +119,11 @@ describeEmbeddedPostgres("tool gateway service", () => { afterAll(async () => { await tempDb?.cleanup(); + if (originalToolActionSigningSecret === undefined) { + delete process.env.PAPERCLIP_TOOL_ACTION_SIGNING_SECRET; + } else { + process.env.PAPERCLIP_TOOL_ACTION_SIGNING_SECRET = originalToolActionSigningSecret; + } }); it("gates write tools with an action request and executes only stored reviewed arguments once", async () => { @@ -326,4 +333,62 @@ describeEmbeddedPostgres("tool gateway service", () => { }); expect(serialized).not.toContain(maliciousContent); }); + + it("passes original sensitive arguments to plugin executors while redacting stored summaries", async () => { + const { company, agent, run } = await createRunFixture(db); + let executedParameters: unknown; + const gateway = createToolGatewayService(db, { + pluginToolDispatcher: { + initialize: async () => {}, + teardown: () => {}, + listToolsForAgent: () => [ + { + name: "fixture:read_status", + displayName: "Read status", + description: "Echoes parameters for executor assertions.", + parametersSchema: { type: "object" }, + pluginId: "fixture-plugin", + }, + ], + getTool: () => null, + executeTool: async (_name, parameters) => { + executedParameters = parameters; + return { + pluginId: "fixture-plugin", + toolName: "read_status", + result: { ok: true }, + }; + }, + registerPluginTools: () => {}, + unregisterPluginTools: () => {}, + toolCount: () => 1, + getRegistry: () => { + throw new Error("not implemented"); + }, + }, + }); + await db.insert(toolPolicies).values({ + companyId: company.id, + name: "Allow read fixture", + policyType: "allow", + selectors: { toolName: "fixture:read_status" }, + }); + + await gateway.executePluginTool({ + actor: { type: "agent", companyId: company.id, agentId: agent.id, runId: run.id }, + tool: "fixture:read_status", + parameters: { query: "ok", apiKey: "sk-secret-value" }, + runContext: { companyId: company.id, agentId: agent.id, runId: run.id }, + }); + + expect(executedParameters).toEqual({ query: "ok", apiKey: "sk-secret-value" }); + + const [invocation] = await db.select().from(toolInvocations); + const [callEvent] = await db.select().from(toolCallEvents).where(eq(toolCallEvents.eventType, "call_completed")); + const [audit] = await db.select().from(activityLog).where(eq(activityLog.action, "tool_gateway.call_allowed")); + const serialized = JSON.stringify({ invocation, callEvent, audit }); + + expect(serialized).not.toContain("sk-secret-value"); + expect(serialized).toContain("***REDACTED***"); + }); }); diff --git a/server/src/services/tool-content-guards.ts b/server/src/services/tool-content-guards.ts index 1682cd3750c..3262569a8e2 100644 --- a/server/src/services/tool-content-guards.ts +++ b/server/src/services/tool-content-guards.ts @@ -41,12 +41,11 @@ function scanPromptInjection(value: unknown): string[] { } function signingSecret() { - return ( - process.env.PAPERCLIP_TOOL_ACTION_SIGNING_SECRET?.trim() - || process.env.PAPERCLIP_AGENT_JWT_SECRET?.trim() - || process.env.BETTER_AUTH_SECRET?.trim() - || "paperclip-dev-tool-action-signing-secret" - ); + const secret = process.env.PAPERCLIP_TOOL_ACTION_SIGNING_SECRET?.trim(); + if (!secret) { + throw new Error("PAPERCLIP_TOOL_ACTION_SIGNING_SECRET is required for signed tool action approvals"); + } + return secret; } export function canonicalToolArguments(value: unknown) { diff --git a/server/src/services/tool-gateway.ts b/server/src/services/tool-gateway.ts index e7d5aed6774..4da6aee8a9d 100644 --- a/server/src/services/tool-gateway.ts +++ b/server/src/services/tool-gateway.ts @@ -1204,13 +1204,14 @@ export function createToolGatewayService( const tool = findTool(input.tool); + const requestedParameters = input.parameters ?? {}; const argumentValidation = validateToolContent({ - value: input.parameters ?? {}, + value: requestedParameters, direction: "arguments", sensitiveMode: "redact", promptInjectionMode: "ignore", }); - let effectiveParameters = argumentValidation.value; + let effectiveParameters = requestedParameters; let effectiveArgumentsSummary = argumentValidation.summary; if (input.approvedActionRequestId) { @@ -1360,7 +1361,7 @@ export function createToolGatewayService( throw new ToolGatewayHttpError(409, "Tool action request was already consumed", "action_already_consumed"); } invocationId = storedInvocation.id as typeof invocationId; - effectiveParameters = storedArgumentValidation.value; + effectiveParameters = storedParameters; effectiveArgumentsSummary = storedArgumentValidation.summary; await db .update(toolInvocations) @@ -1649,8 +1650,9 @@ export function createToolGatewayService( throw new ToolGatewayHttpError(404, `Tool "${input.tool}" is not a plugin tool`, "tool_not_found"); } + const requestedParameters = input.parameters ?? {}; const argumentValidation = validateToolContent({ - value: input.parameters ?? {}, + value: requestedParameters, direction: "arguments", sensitiveMode: "redact", promptInjectionMode: "ignore", @@ -1659,7 +1661,7 @@ export function createToolGatewayService( const decisionInput = policyInputForTool({ session: sessionLike, tool, - parameters: argumentValidation.value, + parameters: requestedParameters, consumeRateLimit: true, }); const accessDecision = await policyService.decide(decisionInput); @@ -1677,7 +1679,7 @@ export function createToolGatewayService( actionRequest: recorded.actionRequest, session: sessionLike, tool, - parameters: argumentValidation.value, + parameters: requestedParameters, argumentsSummary: argumentValidation.summary, policyDecision: accessDecision, }); @@ -1740,7 +1742,7 @@ export function createToolGatewayService( const startedAt = Date.now(); try { - const result = await pluginToolDispatcher.executeTool(input.tool, argumentValidation.value, input.runContext); + const result = await pluginToolDispatcher.executeTool(input.tool, requestedParameters, input.runContext); const resultValidation = validateToolContent({ value: result, direction: "result",