From 8b76392e3e7934b1cdd6d461148263fd56141ec8 Mon Sep 17 00:00:00 2001 From: Michael Appel Date: Fri, 24 Apr 2026 15:16:45 -0400 Subject: [PATCH] fix(gateway): enforce owner-only tool policy and before-tool-call hook on MCP loopback surface (#71159) * fix: address issue * fix: address review feedback * fix: address PR review feedback * changelog: PR #71159 MCP loopback owner-only policy + before-tool-call hook --------- Co-authored-by: Devin Robison --- CHANGELOG.md | 1 + src/gateway/mcp-http.handlers.ts | 18 +- src/gateway/mcp-http.runtime.ts | 10 +- src/gateway/mcp-http.test.ts | 276 +++++++++++++++++++++++++++++++ src/gateway/mcp-http.ts | 45 ++++- 5 files changed, 345 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index befacce67fd..44e36c45b29 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -149,6 +149,7 @@ Docs: https://docs.openclaw.ai - Webhooks/security: re-resolve `SecretRef`-backed webhook route secrets on each request so `openclaw secrets reload` revokes the previous secret immediately instead of waiting for a gateway restart. (#70727) Thanks @drobison00. - Memory/dreaming: decouple the managed dreaming cron from heartbeat by running it as an isolated lightweight agent turn, so dreaming runs even when heartbeat is disabled for the default agent and is no longer skipped by `heartbeat.activeHours`. `openclaw doctor --fix` migrates stale main-session dreaming jobs in persisted cron configs to the new shape. Fixes #69811, #67397, #68972. (#70737) Thanks @jalehman. - Agents/CLI: keep `--agent` plus `--session-id` lookup scoped to the requested agent store, so explicit agent resumes cannot select another agent's session. (#70985) Thanks @frankekn. +- Gateway/MCP loopback: apply owner-only tool policy and run before-tool-call hooks on `127.0.0.1/mcp` `tools/list` and `tools/call`, so non-owner bearer callers can no longer see or invoke owner-only tools such as `cron`, `gateway`, and `nodes`, matching the existing HTTP `/tools/invoke` and embedded-agent paths. (#71159) Thanks @mmaps. ## 2026.4.22 diff --git a/src/gateway/mcp-http.handlers.ts b/src/gateway/mcp-http.handlers.ts index 28178dec2d3..d4da84e4d5c 100644 --- a/src/gateway/mcp-http.handlers.ts +++ b/src/gateway/mcp-http.handlers.ts @@ -1,4 +1,5 @@ import crypto from "node:crypto"; +import { runBeforeToolCallHook, type HookContext } from "../agents/pi-tools.before-tool-call.js"; import { formatErrorMessage } from "../infra/errors.js"; import { MCP_LOOPBACK_SERVER_NAME, @@ -35,6 +36,8 @@ export async function handleMcpJsonRpc(params: { message: JsonRpcRequest; tools: McpLoopbackTool[]; toolSchema: McpToolSchemaEntry[]; + hookContext?: HookContext; + signal?: AbortSignal; }): Promise { const { id, method, params: methodParams } = params.message; @@ -70,7 +73,20 @@ export async function handleMcpJsonRpc(params: { } const toolCallId = `mcp-${crypto.randomUUID()}`; try { - const result = await tool.execute(toolCallId, toolArgs); + const hookResult = await runBeforeToolCallHook({ + toolName, + params: toolArgs, + toolCallId, + ctx: params.hookContext, + signal: params.signal, + }); + if (hookResult.blocked) { + return jsonRpcResult(id, { + content: [{ type: "text", text: hookResult.reason }], + isError: true, + }); + } + const result = await tool.execute(toolCallId, hookResult.params, params.signal); return jsonRpcResult(id, { content: normalizeToolCallContent(result), isError: false, diff --git a/src/gateway/mcp-http.runtime.ts b/src/gateway/mcp-http.runtime.ts index fadf34b43f5..ab0efb431a5 100644 --- a/src/gateway/mcp-http.runtime.ts +++ b/src/gateway/mcp-http.runtime.ts @@ -1,3 +1,4 @@ +import { applyOwnerOnlyToolPolicy } from "../agents/tool-policy.js"; import type { OpenClawConfig } from "../config/types.openclaw.js"; import { clearActiveMcpLoopbackRuntimeByOwnerToken, @@ -16,6 +17,7 @@ const TOOL_CACHE_TTL_MS = 30_000; const NATIVE_TOOL_EXCLUDE = new Set(["read", "write", "edit", "apply_patch", "exec", "process"]); type CachedScopedTools = { + agentId: string | undefined; tools: McpLoopbackTool[]; toolSchema: McpToolSchemaEntry[]; configRef: OpenClawConfig; @@ -36,7 +38,7 @@ export class McpLoopbackToolCache { params.sessionKey, params.messageProvider ?? "", params.accountId ?? "", - params.senderIsOwner === true ? "owner" : params.senderIsOwner === false ? "non-owner" : "", + params.senderIsOwner === true ? "owner" : "non-owner", ].join("\u0000"); const now = Date.now(); const cached = this.#entries.get(cacheKey); @@ -53,9 +55,11 @@ export class McpLoopbackToolCache { surface: "loopback", excludeToolNames: NATIVE_TOOL_EXCLUDE, }); + const tools = applyOwnerOnlyToolPolicy(next.tools, params.senderIsOwner === true); const nextEntry: CachedScopedTools = { - tools: next.tools, - toolSchema: buildMcpToolSchema(next.tools), + agentId: next.agentId, + tools, + toolSchema: buildMcpToolSchema(tools), configRef: params.cfg, time: now, }; diff --git a/src/gateway/mcp-http.test.ts b/src/gateway/mcp-http.test.ts index 0d10a1bf8c3..28a262084a8 100644 --- a/src/gateway/mcp-http.test.ts +++ b/src/gateway/mcp-http.test.ts @@ -5,6 +5,7 @@ type MockGatewayTool = { name: string; description: string; parameters: Record; + ownerOnly?: boolean; execute: (...args: unknown[]) => Promise<{ content: Array<{ type: string; text: string }> }>; }; @@ -13,6 +14,19 @@ type MockGatewayScopedTools = { tools: MockGatewayTool[]; }; +type MockBeforeToolCallHookResult = + | { blocked: true; reason: string } + | { blocked: false; params: unknown }; + +const runBeforeToolCallHookMock = vi.hoisted(() => + vi.fn( + async (args: { params: unknown }): Promise => ({ + blocked: false, + params: args.params, + }), + ), +); + const resolveGatewayScopedToolsMock = vi.hoisted(() => vi.fn<(...args: unknown[]) => MockGatewayScopedTools>(() => ({ agentId: "main", @@ -37,6 +51,11 @@ vi.mock("../config/sessions.js", () => ({ resolveMainSessionKey: () => "agent:main:main", })); +vi.mock("../agents/pi-tools.before-tool-call.js", () => ({ + runBeforeToolCallHook: (...args: Parameters) => + runBeforeToolCallHookMock(...args), +})); + vi.mock("./tool-resolution.js", () => ({ resolveGatewayScopedTools: (...args: Parameters) => resolveGatewayScopedToolsMock(...args), @@ -71,6 +90,13 @@ async function sendRaw(params: { beforeEach(() => { resolveGatewayScopedToolsMock.mockClear(); + runBeforeToolCallHookMock.mockClear(); + runBeforeToolCallHookMock.mockImplementation( + async (args: { params: unknown }): Promise => ({ + blocked: false, + params: args.params, + }), + ); resolveGatewayScopedToolsMock.mockReturnValue({ agentId: "main", tools: [ @@ -231,6 +257,256 @@ describe("mcp loopback server", () => { ); }); + it("filters owner-only tools from non-owner tool lists", async () => { + resolveGatewayScopedToolsMock.mockReturnValue({ + agentId: "main", + tools: [ + { + name: "message", + description: "send a message", + parameters: { type: "object", properties: {} }, + execute: async () => ({ + content: [{ type: "text", text: "ok" }], + }), + }, + { + name: "cron", + description: "manage schedules", + parameters: { type: "object", properties: {} }, + execute: async () => ({ + content: [{ type: "text", text: "cron" }], + }), + }, + { + name: "owner_probe", + description: "owner-only by flag", + parameters: { type: "object", properties: {} }, + ownerOnly: true, + execute: async () => ({ + content: [{ type: "text", text: "owner" }], + }), + }, + ], + }); + server = await startMcpLoopbackServer(0); + const runtime = getActiveMcpLoopbackRuntime(); + + const response = await sendRaw({ + port: server.port, + token: runtime ? resolveMcpLoopbackBearerToken(runtime, false) : undefined, + headers: { + "content-type": "application/json", + "x-session-key": "agent:main:main", + }, + body: JSON.stringify({ jsonrpc: "2.0", id: 1, method: "tools/list" }), + }); + const payload = (await response.json()) as { + result?: { tools?: Array<{ name: string }> }; + }; + const names = (payload.result?.tools ?? []).map((tool) => tool.name); + + expect(response.status).toBe(200); + expect(names).toContain("message"); + expect(names).not.toContain("cron"); + expect(names).not.toContain("owner_probe"); + }); + + it("keeps owner-only tools available to owner loopback callers", async () => { + resolveGatewayScopedToolsMock.mockReturnValue({ + agentId: "main", + tools: [ + { + name: "message", + description: "send a message", + parameters: { type: "object", properties: {} }, + execute: async () => ({ + content: [{ type: "text", text: "ok" }], + }), + }, + { + name: "cron", + description: "manage schedules", + parameters: { type: "object", properties: {} }, + execute: async () => ({ + content: [{ type: "text", text: "cron" }], + }), + }, + ], + }); + server = await startMcpLoopbackServer(0); + const runtime = getActiveMcpLoopbackRuntime(); + + const response = await sendRaw({ + port: server.port, + token: runtime ? resolveMcpLoopbackBearerToken(runtime, true) : undefined, + headers: { + "content-type": "application/json", + "x-session-key": "agent:main:main", + }, + body: JSON.stringify({ jsonrpc: "2.0", id: 1, method: "tools/list" }), + }); + const payload = (await response.json()) as { + result?: { tools?: Array<{ name: string }> }; + }; + const names = (payload.result?.tools ?? []).map((tool) => tool.name); + + expect(response.status).toBe(200); + expect(names).toContain("message"); + expect(names).toContain("cron"); + }); + + it("does not execute owner-only tools for non-owner callers", async () => { + const cronExecute = vi.fn(async () => ({ + content: [{ type: "text", text: "CRON_EXECUTED" }], + })); + resolveGatewayScopedToolsMock.mockReturnValue({ + agentId: "main", + tools: [ + { + name: "message", + description: "send a message", + parameters: { type: "object", properties: {} }, + execute: async () => ({ + content: [{ type: "text", text: "ok" }], + }), + }, + { + name: "cron", + description: "manage schedules", + parameters: { type: "object", properties: {} }, + execute: cronExecute, + }, + ], + }); + server = await startMcpLoopbackServer(0); + const runtime = getActiveMcpLoopbackRuntime(); + + const response = await sendRaw({ + port: server.port, + token: runtime ? resolveMcpLoopbackBearerToken(runtime, false) : undefined, + headers: { + "content-type": "application/json", + "x-session-key": "agent:main:main", + }, + body: JSON.stringify({ + jsonrpc: "2.0", + id: 1, + method: "tools/call", + params: { name: "cron", arguments: {} }, + }), + }); + const payload = (await response.json()) as { + result?: { content?: Array<{ text?: string }>; isError?: boolean }; + }; + + expect(response.status).toBe(200); + expect(cronExecute).not.toHaveBeenCalled(); + expect(payload.result?.isError).toBe(true); + expect(payload.result?.content?.[0]?.text).toBe("Tool not available: cron"); + }); + + it("honors before-tool-call hook blocks before loopback tool execution", async () => { + const execute = vi.fn(async () => ({ + content: [{ type: "text", text: "EXECUTED" }], + })); + runBeforeToolCallHookMock.mockResolvedValueOnce({ + blocked: true, + reason: "blocked by hook", + }); + resolveGatewayScopedToolsMock.mockReturnValue({ + agentId: "main", + tools: [ + { + name: "message", + description: "send a message", + parameters: { type: "object", properties: {} }, + execute, + }, + ], + }); + server = await startMcpLoopbackServer(0); + const runtime = getActiveMcpLoopbackRuntime(); + + const response = await sendRaw({ + port: server.port, + token: runtime ? resolveMcpLoopbackBearerToken(runtime, false) : undefined, + headers: { + "content-type": "application/json", + "x-session-key": "agent:main:main", + }, + body: JSON.stringify({ + jsonrpc: "2.0", + id: 1, + method: "tools/call", + params: { name: "message", arguments: { body: "hello" } }, + }), + }); + const payload = (await response.json()) as { + result?: { content?: Array<{ text?: string }>; isError?: boolean }; + }; + + expect(response.status).toBe(200); + expect(runBeforeToolCallHookMock).toHaveBeenCalledWith( + expect.objectContaining({ + toolName: "message", + params: { body: "hello" }, + ctx: expect.objectContaining({ + agentId: "main", + sessionKey: "agent:main:main", + }), + signal: expect.any(AbortSignal), + }), + ); + expect(execute).not.toHaveBeenCalled(); + expect(payload.result?.isError).toBe(true); + expect(payload.result?.content?.[0]?.text).toBe("blocked by hook"); + }); + + it("forwards the request abort signal to loopback tool execution", async () => { + const execute = vi.fn(async () => ({ + content: [{ type: "text", text: "EXECUTED" }], + })); + resolveGatewayScopedToolsMock.mockReturnValue({ + agentId: "main", + tools: [ + { + name: "message", + description: "send a message", + parameters: { type: "object", properties: {} }, + execute, + }, + ], + }); + server = await startMcpLoopbackServer(0); + const runtime = getActiveMcpLoopbackRuntime(); + + const response = await sendRaw({ + port: server.port, + token: runtime ? resolveMcpLoopbackBearerToken(runtime, false) : undefined, + headers: { + "content-type": "application/json", + "x-session-key": "agent:main:main", + }, + body: JSON.stringify({ + jsonrpc: "2.0", + id: 1, + method: "tools/call", + params: { name: "message", arguments: { body: "hello" } }, + }), + }); + const payload = (await response.json()) as { + result?: { isError?: boolean }; + }; + + expect(response.status).toBe(200); + expect(payload.result?.isError).toBe(false); + expect(execute).toHaveBeenCalledWith( + expect.stringMatching(/^mcp-/), + { body: "hello" }, + expect.any(AbortSignal), + ); + }); + it("tracks the active runtime only while the server is running", async () => { server = await startMcpLoopbackServer(0); const active = getActiveMcpLoopbackRuntime(); diff --git a/src/gateway/mcp-http.ts b/src/gateway/mcp-http.ts index 2e0fd915ffa..2faff020ddf 100644 --- a/src/gateway/mcp-http.ts +++ b/src/gateway/mcp-http.ts @@ -1,5 +1,9 @@ import crypto from "node:crypto"; -import { createServer as createHttpServer } from "node:http"; +import { + createServer as createHttpServer, + type IncomingMessage, + type ServerResponse, +} from "node:http"; import { loadConfig } from "../config/config.js"; import { isTruthyEnvValue } from "../infra/env.js"; import { formatErrorMessage } from "../infra/errors.js"; @@ -51,6 +55,37 @@ function isRecord(value: unknown): value is Record { return typeof value === "object" && value !== null && !Array.isArray(value); } +function createRequestAbortSignal(req: IncomingMessage, res: ServerResponse) { + const controller = new AbortController(); + const abort = () => { + if (!controller.signal.aborted) { + controller.abort(); + } + }; + const abortIfRequestIncomplete = () => { + if (!req.complete) { + abort(); + } + }; + const abortIfResponseStillOpen = () => { + if (!res.writableEnded) { + abort(); + } + }; + req.once("close", abortIfRequestIncomplete); + res.once("close", abortIfResponseStillOpen); + if (req.destroyed && !req.complete) { + abort(); + } + return { + signal: controller.signal, + cleanup: () => { + req.off("close", abortIfRequestIncomplete); + res.off("close", abortIfResponseStillOpen); + }, + }; +} + export async function startMcpLoopbackServer(port = 0): Promise<{ port: number; close: () => Promise; @@ -65,6 +100,7 @@ export async function startMcpLoopbackServer(port = 0): Promise<{ return; } + const requestAbort = createRequestAbortSignal(req, res); void (async () => { try { const body = await readMcpHttpBody(req); @@ -94,6 +130,11 @@ export async function startMcpLoopbackServer(port = 0): Promise<{ message, tools: scopedTools.tools, toolSchema: scopedTools.toolSchema, + hookContext: { + agentId: scopedTools.agentId, + sessionKey: requestContext.sessionKey, + }, + signal: requestAbort.signal, }); if (response !== null) { const toolName = @@ -131,6 +172,8 @@ export async function startMcpLoopbackServer(port = 0): Promise<{ res.writeHead(400, { "Content-Type": "application/json" }); res.end(JSON.stringify(jsonRpcError(null, -32700, "Parse error"))); } + } finally { + requestAbort.cleanup(); } })(); });