mirror of
https://github.com/openclaw/openclaw.git
synced 2026-04-12 17:51:22 +00:00
Ensure ACPX plugin-tools bridge honors before_tool_call (#63886)
* fix(acpx): honor tool hook on plugin bridge Co-authored-by: smaeljaish771 <smaeljaish771@gmail.com> * chore(changelog): add ACPX plugin-tools before_tool_call entry --------- Co-authored-by: smaeljaish771 <smaeljaish771@gmail.com> Co-authored-by: Devin Robison <drobison@nvidia.com>
This commit is contained in:
@@ -135,6 +135,7 @@ Docs: https://docs.openclaw.ai
|
||||
- Hooks/security: mark agent hook system events as untrusted and sanitize hook display names before cron metadata reuse. (#64372) Thanks @eleqtrizit.
|
||||
- Media/security: honor sender-scoped `toolsBySender` policy for outbound host-media reads so denied senders cannot trigger host file disclosure via attachment hydration. (#64459) Thanks @eleqtrizit.
|
||||
- Browser/security: reject strict-policy hostname navigation unless the hostname is an explicit allowlist exception or IP literal, and route CDP HTTP discovery through the pinned SSRF fetch path. (#64367) Thanks @eleqtrizit.
|
||||
- Plugins/ACPX: wrap plugin tools on the MCP bridge with the shared `before_tool_call` handler so block and approval hooks fire consistently across all execution paths. (#63886) Thanks @eleqtrizit.
|
||||
|
||||
## 2026.4.9
|
||||
|
||||
|
||||
@@ -2,6 +2,11 @@ import { Client } from "@modelcontextprotocol/sdk/client/index.js";
|
||||
import { InMemoryTransport } from "@modelcontextprotocol/sdk/inMemory.js";
|
||||
import { afterEach, describe, expect, it, vi } from "vitest";
|
||||
import type { AnyAgentTool } from "../agents/tools/common.js";
|
||||
import {
|
||||
initializeGlobalHookRunner,
|
||||
resetGlobalHookRunner,
|
||||
} from "../plugins/hook-runner-global.js";
|
||||
import { createMockPluginRegistry } from "../plugins/hooks.test-helpers.js";
|
||||
import { createPluginToolsMcpServer } from "./plugin-tools-serve.js";
|
||||
|
||||
async function connectPluginToolsServer(tools: AnyAgentTool[]) {
|
||||
@@ -21,6 +26,7 @@ async function connectPluginToolsServer(tools: AnyAgentTool[]) {
|
||||
|
||||
afterEach(() => {
|
||||
vi.restoreAllMocks();
|
||||
resetGlobalHookRunner();
|
||||
});
|
||||
|
||||
describe("plugin tools MCP server", () => {
|
||||
@@ -73,9 +79,14 @@ describe("plugin tools MCP server", () => {
|
||||
name: "memory_store",
|
||||
arguments: { text: "remember this" },
|
||||
});
|
||||
expect(execute).toHaveBeenCalledWith(expect.stringMatching(/^mcp-\d+$/), {
|
||||
text: "remember this",
|
||||
});
|
||||
expect(execute).toHaveBeenCalledWith(
|
||||
expect.stringMatching(/^mcp-\d+$/),
|
||||
{
|
||||
text: "remember this",
|
||||
},
|
||||
undefined,
|
||||
undefined,
|
||||
);
|
||||
expect(result.content).toEqual([{ type: "text", text: "Stored." }]);
|
||||
} finally {
|
||||
await session.close();
|
||||
@@ -109,4 +120,82 @@ describe("plugin tools MCP server", () => {
|
||||
await session.close();
|
||||
}
|
||||
});
|
||||
|
||||
it("blocks tool execution when before_tool_call requires approval on the MCP bridge", async () => {
|
||||
let hookCalls = 0;
|
||||
const execute = vi.fn().mockResolvedValue({
|
||||
content: "Stored.",
|
||||
});
|
||||
initializeGlobalHookRunner(
|
||||
createMockPluginRegistry([
|
||||
{
|
||||
hookName: "before_tool_call",
|
||||
handler: async () => {
|
||||
hookCalls += 1;
|
||||
return {
|
||||
requireApproval: {
|
||||
pluginId: "test-plugin",
|
||||
title: "Approval required",
|
||||
description: "Approval required",
|
||||
},
|
||||
};
|
||||
},
|
||||
},
|
||||
]),
|
||||
);
|
||||
const tool = {
|
||||
name: "memory_store",
|
||||
description: "Store memory",
|
||||
parameters: { type: "object", properties: {} },
|
||||
execute,
|
||||
} as unknown as AnyAgentTool;
|
||||
|
||||
const session = await connectPluginToolsServer([tool]);
|
||||
try {
|
||||
const result = await session.client.callTool({
|
||||
name: "memory_store",
|
||||
arguments: { text: "remember this" },
|
||||
});
|
||||
expect(hookCalls).toBe(1);
|
||||
expect(execute).not.toHaveBeenCalled();
|
||||
expect(result.isError).toBe(true);
|
||||
expect(result.content).toEqual([
|
||||
{ type: "text", text: "Tool error: Plugin approval required (gateway unavailable)" },
|
||||
]);
|
||||
} finally {
|
||||
await session.close();
|
||||
}
|
||||
});
|
||||
|
||||
it("still executes plugin tools on the MCP bridge when no before_tool_call hook is registered", async () => {
|
||||
const execute = vi.fn().mockResolvedValue({
|
||||
content: "Stored.",
|
||||
});
|
||||
const tool = {
|
||||
name: "memory_store",
|
||||
description: "Store memory",
|
||||
parameters: { type: "object", properties: {} },
|
||||
execute,
|
||||
} as unknown as AnyAgentTool;
|
||||
|
||||
const session = await connectPluginToolsServer([tool]);
|
||||
try {
|
||||
const result = await session.client.callTool({
|
||||
name: "memory_store",
|
||||
arguments: { text: "remember this" },
|
||||
});
|
||||
expect(execute).toHaveBeenCalledWith(
|
||||
expect.stringMatching(/^mcp-\d+$/),
|
||||
{
|
||||
text: "remember this",
|
||||
},
|
||||
undefined,
|
||||
undefined,
|
||||
);
|
||||
expect(result.isError).toBeUndefined();
|
||||
expect(result.content).toEqual([{ type: "text", text: "Stored." }]);
|
||||
} finally {
|
||||
await session.close();
|
||||
}
|
||||
});
|
||||
});
|
||||
|
||||
@@ -10,6 +10,10 @@ import { pathToFileURL } from "node:url";
|
||||
import { Server } from "@modelcontextprotocol/sdk/server/index.js";
|
||||
import { StdioServerTransport } from "@modelcontextprotocol/sdk/server/stdio.js";
|
||||
import { CallToolRequestSchema, ListToolsRequestSchema } from "@modelcontextprotocol/sdk/types.js";
|
||||
import {
|
||||
isToolWrappedWithBeforeToolCallHook,
|
||||
wrapToolWithBeforeToolCallHook,
|
||||
} from "../agents/pi-tools.before-tool-call.js";
|
||||
import type { AnyAgentTool } from "../agents/tools/common.js";
|
||||
import type { OpenClawConfig } from "../config/config.js";
|
||||
import { loadConfig } from "../config/config.js";
|
||||
@@ -41,7 +45,14 @@ export function createPluginToolsMcpServer(
|
||||
} = {},
|
||||
): Server {
|
||||
const cfg = params.config ?? loadConfig();
|
||||
const tools = params.tools ?? resolveTools(cfg);
|
||||
const tools = (params.tools ?? resolveTools(cfg)).map((tool) => {
|
||||
if (isToolWrappedWithBeforeToolCallHook(tool)) {
|
||||
return tool;
|
||||
}
|
||||
// The ACPX MCP bridge should enforce the same pre-execution hook boundary
|
||||
// as the agent and HTTP tool execution paths.
|
||||
return wrapToolWithBeforeToolCallHook(tool);
|
||||
});
|
||||
|
||||
const toolMap = new Map<string, AnyAgentTool>();
|
||||
for (const tool of tools) {
|
||||
|
||||
Reference in New Issue
Block a user