From 79dfc3ebc8bd554848116e345c7997afb29cec29 Mon Sep 17 00:00:00 2001 From: clawsweeper <274271284+clawsweeper[bot]@users.noreply.github.com> Date: Thu, 21 May 2026 02:59:15 +0000 Subject: [PATCH] fix(clawsweeper): address review for automerge-openclaw-openclaw-84699 (1) --- .../plugin-tool-allowlist-warnings.test.ts | 26 +++++++ .../shared/plugin-tool-allowlist-warnings.ts | 69 +++++++++++++------ 2 files changed, 73 insertions(+), 22 deletions(-) diff --git a/src/commands/doctor/shared/plugin-tool-allowlist-warnings.test.ts b/src/commands/doctor/shared/plugin-tool-allowlist-warnings.test.ts index c401fce1d40..18508ce15e8 100644 --- a/src/commands/doctor/shared/plugin-tool-allowlist-warnings.test.ts +++ b/src/commands/doctor/shared/plugin-tool-allowlist-warnings.test.ts @@ -115,6 +115,32 @@ describe("collectPluginToolAllowlistWarnings", () => { ]); }); + it("warns when sandbox allowlist covers only one configured MCP server", () => { + const warnings = collectPluginToolAllowlistWarnings({ + cfg: { + agents: { defaults: { sandbox: { mode: "all" } } }, + mcp: { + servers: { + gmail: { command: "node", args: ["gmail-server.js"] }, + outlook: { command: "node", args: ["outlook-server.js"] }, + }, + }, + tools: { + sandbox: { + tools: { + alsoAllow: ["outlook__*"], + }, + }, + }, + }, + manifestRegistry, + }); + + expect(warnings).toEqual([ + '- mcp.servers defines 2 MCP servers ("gmail", "outlook"), but tools.sandbox.tools.alsoAllow does not include "bundle-mcp", "group:plugins", or a matching server-prefixed MCP tool name/glob such as "__*". Sandboxed agents will filter bundled MCP tools before provider requests. Add "bundle-mcp" to tools.sandbox.tools.alsoAllow (or use "group:plugins" / server globs) if those MCP tools should be visible; use tools.sandbox.tools.allow: [] only when you intentionally want no sandbox allow gate.', + ]); + }); + it("uses a config-path source label when sandbox allowlist is unset", () => { const warnings = collectPluginToolAllowlistWarnings({ cfg: { diff --git a/src/commands/doctor/shared/plugin-tool-allowlist-warnings.ts b/src/commands/doctor/shared/plugin-tool-allowlist-warnings.ts index 43245eaf9bc..872532dc687 100644 --- a/src/commands/doctor/shared/plugin-tool-allowlist-warnings.ts +++ b/src/commands/doctor/shared/plugin-tool-allowlist-warnings.ts @@ -389,9 +389,10 @@ function buildMcpToolNamePrefixes(serverNames: readonly string[]): string[] { .filter(Boolean); } -function entriesMatchAnyMcpTool( +function entriesMatchMcpTool( entries: readonly string[], serverNames: readonly string[], + mode: "any" | "every", ): boolean { const normalizedEntries = entries.map(normalizeToolName).filter(Boolean); if ( @@ -402,23 +403,43 @@ function entriesMatchAnyMcpTool( return true; } const serverPrefixes = buildMcpToolNamePrefixes(serverNames); - if ( - normalizedEntries.some((entry) => - serverPrefixes.some((prefix) => entry.length > prefix.length && entry.startsWith(prefix)), - ) - ) { - return true; - } const patterns = compileGlobPatterns({ raw: normalizedEntries, normalize: normalizeToolName }); - if (patterns.length === 0) { - return false; - } - return buildMcpProbeToolNames(serverNames).some((probeName) => - matchesAnyGlobPattern(normalizeToolName(probeName), patterns), - ); + const probeNames = buildMcpProbeToolNames(serverNames).map(normalizeToolName); + const prefixOrPatternMatches = (prefix: string, index: number) => + normalizedEntries.some((entry) => entry.length > prefix.length && entry.startsWith(prefix)) || + matchesAnyGlobPattern(probeNames[index] ?? "", patterns); + return mode === "every" + ? serverPrefixes.every((prefix, index) => prefixOrPatternMatches(prefix, index)) + : serverPrefixes.some((prefix, index) => prefixOrPatternMatches(prefix, index)); } -function sandboxAllowGateAllowsMcp(policy: unknown, serverNames: readonly string[]): boolean { +function entriesMatchAnyMcpTool( + entries: readonly string[], + serverNames: readonly string[], +): boolean { + return entriesMatchMcpTool(entries, serverNames, "any"); +} + +function entriesMatchEveryMcpTool( + entries: readonly string[], + serverNames: readonly string[], +): boolean { + return entriesMatchMcpTool(entries, serverNames, "every"); +} + +function sandboxPolicyAllowsAllMcpServers( + policy: unknown, + serverNames: readonly string[], +): boolean { + const allow = getList(policy, "allow"); + if (Array.isArray(allow) && allow.length === 0) { + return true; + } + const entries = [...(allow ?? []), ...(getList(policy, "alsoAllow") ?? [])]; + return entriesMatchEveryMcpTool(entries, serverNames); +} + +function toolPolicyAllowsAnyMcpServer(policy: unknown, serverNames: readonly string[]): boolean { const allow = getList(policy, "allow"); if (Array.isArray(allow) && allow.length === 0) { return true; @@ -427,16 +448,20 @@ function sandboxAllowGateAllowsMcp(policy: unknown, serverNames: readonly string return entriesMatchAnyMcpTool(entries, serverNames); } -function sandboxPolicyIntentionallyDeniesMcp( +function toolPolicyDeniesAllMcpServers(policy: unknown, serverNames: readonly string[]): boolean { + const deny = getList(policy, "deny") ?? []; + return entriesMatchEveryMcpTool(deny, serverNames); +} + +function sandboxPolicyIntentionallyDeniesAllMcpServers( policy: unknown, serverNames: readonly string[], ): boolean { - const deny = getList(policy, "deny") ?? []; - return entriesMatchAnyMcpTool(deny, serverNames); + return toolPolicyDeniesAllMcpServers(policy, serverNames); } function nonSandboxToolPolicyBlocksMcp(policy: unknown, serverNames: readonly string[]): boolean { - if (sandboxPolicyIntentionallyDeniesMcp(policy, serverNames)) { + if (toolPolicyDeniesAllMcpServers(policy, serverNames)) { return true; } const allow = getList(policy, "allow"); @@ -453,7 +478,7 @@ function profileToolPolicyBlocksMcp(policy: unknown, serverNames: readonly strin resolveToolProfilePolicy(profile), getList(policy, "alsoAllow"), ); - return Boolean(profilePolicy && !sandboxAllowGateAllowsMcp(profilePolicy, serverNames)); + return Boolean(profilePolicy && !toolPolicyAllowsAnyMcpServer(profilePolicy, serverNames)); } function nonSandboxToolPoliciesBlockMcp(params: { @@ -515,8 +540,8 @@ function collectSandboxMcpAllowlistWarnings(cfg: OpenClawConfig): string[] { const issueSources = sandboxPolicies .filter( ({ policy }) => - !sandboxAllowGateAllowsMcp(policy, serverNames) && - !sandboxPolicyIntentionallyDeniesMcp(policy, serverNames), + !sandboxPolicyAllowsAllMcpServers(policy, serverNames) && + !sandboxPolicyIntentionallyDeniesAllMcpServers(policy, serverNames), ) .filter(({ nonSandboxToolPolicyBlocksMcp }) => !nonSandboxToolPolicyBlocksMcp) .flatMap(({ labels }) => labels);