From 09d8660f3b2f2b891b7153aa0a3dc8bf692094de Mon Sep 17 00:00:00 2001 From: MToan <58007568+taminhtoan2601@users.noreply.github.com> Date: Tue, 7 Jul 2026 06:23:11 +0200 Subject: [PATCH] fix(mcp): derive claude_cli bridge tool surface from agent policy (#1377) The bridge exposed a static BridgeToolNames subset that drifted from the tool registry: use_skill, datetime, knowledge_graph_search and skill_manage were never added, while the system prompt's skill-loading protocol requires agents to call use_skill. claude_cli agents following the protocol hit a nonexistent tool and could fabricate results. Implement the structural fix recommended in #1373 triage: - register the full bridge-capable surface (registry minus hard exclusions spawn/create_forum_topic) instead of the static list - gate BOTH tools/list (new WithToolFilter) and tools/call through one shared predicate bridgeToolAllowed: * callers WITH a verified agent policy get exactly the policy-filtered surface (same WouldAllow check the call path always enforced) * callers WITHOUT one (anonymous, or agent without tools_config) keep the legacy conservative BridgeToolNames set - no exposure widening - downgrade the per-call denial log Warn->Info; list filtering makes probes of denied tools rare and the call gate is the intended enforcement point Fixes #1373 --- internal/mcp/bridge_server.go | 96 +++++++++++++++---- internal/mcp/bridge_server_test.go | 143 +++++++++++++++++++++++++++++ 2 files changed, 223 insertions(+), 16 deletions(-) create mode 100644 internal/mcp/bridge_server_test.go diff --git a/internal/mcp/bridge_server.go b/internal/mcp/bridge_server.go index fd72e169..a5a043e1 100644 --- a/internal/mcp/bridge_server.go +++ b/internal/mcp/bridge_server.go @@ -15,8 +15,13 @@ import ( "github.com/nextlevelbuilder/goclaw/internal/tools" ) -// BridgeToolNames is the subset of GoClaw tools exposed via the MCP bridge. -// Excluded: spawn (agent loop), create_forum_topic (channels). +// BridgeToolNames is the LEGACY conservative tool set, kept as the fallback +// surface for callers that carry no verified agent tool policy in context +// (no/invalid X-Agent-ID headers, or an agent without a tools_config). For +// callers WITH an agent policy, the bridge surface is derived from that +// policy instead — see bridgeToolAllowed. This removes the drift where the +// system prompt requires tools (e.g. use_skill) that a static list forgot to +// expose (#1373), without widening exposure for unauthenticated callers. // delegate is included: it self-gates via CanDelegate/agent_links and resolves // its source agent from the X-Agent-ID header context, same as team_tasks. var BridgeToolNames = map[string]bool{ @@ -54,6 +59,62 @@ var BridgeToolNames = map[string]bool{ "delegate": true, } +// bridgeExcludedTools lists tools that cannot operate over the bridge at all, +// regardless of any agent policy: spawn drives the in-process agent loop and +// create_forum_topic needs a live channel connection owned by the gateway. +var bridgeExcludedTools = map[string]bool{ + "spawn": true, + "create_forum_topic": true, +} + +// bridgeRegisteredToolNames returns every registry tool the bridge may ever +// expose: the full registry minus hard exclusions. Which subset a given +// caller can actually list/call is decided per-request by bridgeToolAllowed. +func bridgeRegisteredToolNames(reg *tools.Registry) []string { + var names []string + for _, name := range reg.List() { + if bridgeExcludedTools[name] { + continue + } + names = append(names, name) + } + return names +} + +// bridgeToolAllowed is the single predicate for both tools/list filtering and +// tools/call gating: +// - hard-excluded tools are never allowed; +// - callers WITHOUT a verified agent policy (or when no policy engine is +// wired) fall back to the legacy conservative BridgeToolNames set, so +// anonymous exposure is unchanged; +// - callers WITH an agent policy get exactly the policy-filtered surface +// (same WouldAllow predicate the call path always enforced). +func bridgeToolAllowed(reg *tools.Registry, policyEngine *tools.PolicyEngine, ctx context.Context, name string) bool { + if bridgeExcludedTools[name] { + return false + } + agentPolicy := tools.ToolAgentPolicyFromCtx(ctx) + if policyEngine == nil || agentPolicy == nil { + return BridgeToolNames[name] + } + return policyEngine.WouldAllow(reg, name, bridgeProviderName, agentPolicy, nil) +} + +// newBridgeToolFilter returns a tools/list filter so each caller only sees +// the tools it can actually call. Without this, the CLI probes tools that the +// call path then denies, wasting agent turns and spamming denial logs. +func newBridgeToolFilter(reg *tools.Registry, policyEngine *tools.PolicyEngine) mcpserver.ToolFilterFunc { + return func(ctx context.Context, listed []mcpgo.Tool) []mcpgo.Tool { + filtered := make([]mcpgo.Tool, 0, len(listed)) + for _, tool := range listed { + if bridgeToolAllowed(reg, policyEngine, ctx, tool.Name) { + filtered = append(filtered, tool) + } + } + return filtered + } +} + // bridgeProviderName identifies the caller for per-provider tool policy // overrides (config.ToolPolicySpec.ByProvider) when enforcing bridge access. // All MCP bridge traffic originates from the Claude CLI subprocess. @@ -73,11 +134,15 @@ const bridgeProviderName = "claude-cli" func NewBridgeServer(reg *tools.Registry, version string, msgBus *bus.MessageBus, policyEngine *tools.PolicyEngine) *mcpserver.StreamableHTTPServer { srv := mcpserver.NewMCPServer("goclaw-bridge", version, mcpserver.WithToolCapabilities(false), + // Per-caller list filtering: each agent only sees its callable surface. + mcpserver.WithToolFilter(newBridgeToolFilter(reg, policyEngine)), ) - // Register each safe tool from the GoClaw registry + // Register the full bridge-capable surface (registry minus hard + // exclusions). Per-caller visibility and callability are both decided by + // bridgeToolAllowed at request time. var registered int - for name := range BridgeToolNames { + for _, name := range bridgeRegisteredToolNames(reg) { t, ok := reg.Get(name) if !ok { continue @@ -111,18 +176,17 @@ func convertToMCPTool(t tools.Tool) mcpgo.Tool { // them as outbound media attachments so files reach the user (e.g. Telegram document). func makeToolHandler(reg *tools.Registry, toolName string, msgBus *bus.MessageBus, policyEngine *tools.PolicyEngine) mcpserver.ToolHandlerFunc { return func(ctx context.Context, req mcpgo.CallToolRequest) (*mcpgo.CallToolResult, error) { - // Enforce the calling agent's own tool policy before execution. Static - // membership in BridgeToolNames only bounds what the bridge process - // COULD ever expose; it does not know which agent is calling. Without - // this check, any agent reaching the bridge (e.g. via Claude CLI) can - // invoke any bridge tool regardless of its configured tool policy. - if policyEngine != nil { - agentPolicy := tools.ToolAgentPolicyFromCtx(ctx) - if !policyEngine.WouldAllow(reg, toolName, bridgeProviderName, agentPolicy, nil) { - slog.Warn("security.mcp_bridge_denied", - "tool", toolName, "agent_key", tools.ToolAgentKeyFromCtx(ctx)) - return mcpgo.NewToolResultError("tool not allowed by policy: " + toolName), nil - } + // Gate execution with the same predicate that filters tools/list, so + // visibility and callability can never drift apart. Registration only + // bounds what the bridge process COULD ever expose; it does not know + // which agent is calling. + // Info (not Warn): the per-call gate is the intended enforcement + // point and the CLI legitimately probes; list filtering already keeps + // this rare. + if !bridgeToolAllowed(reg, policyEngine, ctx, toolName) { + slog.Info("mcp_bridge_denied", + "tool", toolName, "agent_key", tools.ToolAgentKeyFromCtx(ctx)) + return mcpgo.NewToolResultError("tool not allowed by policy: " + toolName), nil } args := req.GetArguments() diff --git a/internal/mcp/bridge_server_test.go b/internal/mcp/bridge_server_test.go new file mode 100644 index 00000000..fd98d4cb --- /dev/null +++ b/internal/mcp/bridge_server_test.go @@ -0,0 +1,143 @@ +package mcp + +import ( + "context" + "testing" + + mcpgo "github.com/mark3labs/mcp-go/mcp" + + "github.com/nextlevelbuilder/goclaw/internal/config" + "github.com/nextlevelbuilder/goclaw/internal/tools" +) + +// fakeBridgeTool is a minimal tools.Tool for registry-driven bridge tests. +type fakeBridgeTool struct{ name string } + +func (f *fakeBridgeTool) Name() string { return f.name } +func (f *fakeBridgeTool) Description() string { return "fake " + f.name } +func (f *fakeBridgeTool) Parameters() map[string]any { return map[string]any{"type": "object"} } +func (f *fakeBridgeTool) Execute(context.Context, map[string]any) *tools.Result { + return &tools.Result{ForLLM: "ok"} +} + +func bridgeTestRegistry(names ...string) *tools.Registry { + reg := tools.NewRegistry() + for _, n := range names { + reg.Register(&fakeBridgeTool{name: n}) + } + return reg +} + +// Without an agent policy in ctx (anonymous caller, or agent without a +// tools_config), the bridge must fall back to the conservative legacy set — +// deriving from a nil policy would WIDEN exposure for unauthenticated callers. +func TestBridgeToolAllowed_NoAgentPolicy_FallsBackToLegacySet(t *testing.T) { + reg := bridgeTestRegistry("read_file", "use_skill") + pe := tools.NewPolicyEngine(&config.ToolsConfig{}) + ctx := context.Background() + + if !bridgeToolAllowed(reg, pe, ctx, "read_file") { + t.Error("read_file is in the legacy set — must stay allowed for policy-less callers") + } + if bridgeToolAllowed(reg, pe, ctx, "use_skill") { + t.Error("use_skill is NOT in the legacy set — must stay blocked for policy-less callers") + } +} + +// With a verified agent policy in ctx, the bridge derives its tool surface +// from that policy — the fix for the BridgeToolNames drift (#1373): tools the +// agent's policy allows (like use_skill) become callable; tools outside the +// agent's allow list are rejected even if the legacy set contained them. +func TestBridgeToolAllowed_AgentPolicyDerivesSurface(t *testing.T) { + reg := bridgeTestRegistry("read_file", "use_skill", "datetime") + pe := tools.NewPolicyEngine(&config.ToolsConfig{}) + ctx := tools.WithToolAgentPolicy(context.Background(), + &config.ToolPolicySpec{Allow: []string{"use_skill", "datetime"}}) + + if !bridgeToolAllowed(reg, pe, ctx, "use_skill") { + t.Error("use_skill allowed by agent policy — bridge must expose it") + } + if !bridgeToolAllowed(reg, pe, ctx, "datetime") { + t.Error("datetime allowed by agent policy — bridge must expose it") + } + if bridgeToolAllowed(reg, pe, ctx, "read_file") { + t.Error("read_file NOT in the agent allow list — bridge must reject it") + } +} + +// Hard-excluded tools can never cross the bridge, regardless of policy. +func TestBridgeToolAllowed_HardExclusionsWinOverPolicy(t *testing.T) { + reg := bridgeTestRegistry("spawn", "create_forum_topic") + pe := tools.NewPolicyEngine(&config.ToolsConfig{}) + ctx := tools.WithToolAgentPolicy(context.Background(), + &config.ToolPolicySpec{Allow: []string{"spawn", "create_forum_topic"}}) + + for _, name := range []string{"spawn", "create_forum_topic"} { + if bridgeToolAllowed(reg, pe, ctx, name) { + t.Errorf("%s is bridge-excluded — policy must not re-enable it", name) + } + } +} + +// Nil policy engine (legacy constructor path) keeps the legacy static behavior. +func TestBridgeToolAllowed_NilEngine_LegacyBehavior(t *testing.T) { + reg := bridgeTestRegistry("exec", "use_skill") + ctx := tools.WithToolAgentPolicy(context.Background(), + &config.ToolPolicySpec{Allow: []string{"use_skill"}}) + + if !bridgeToolAllowed(reg, nil, ctx, "exec") { + t.Error("nil engine: legacy set must apply (exec allowed)") + } + if bridgeToolAllowed(reg, nil, ctx, "use_skill") { + t.Error("nil engine: legacy set must apply (use_skill blocked)") + } +} + +// The tools/list filter must show each caller exactly the surface it can call. +func TestBridgeToolFilter_ListsMatchCallableSurface(t *testing.T) { + reg := bridgeTestRegistry("read_file", "use_skill") + pe := tools.NewPolicyEngine(&config.ToolsConfig{}) + filter := newBridgeToolFilter(reg, pe) + + listed := []mcpgo.Tool{{Name: "read_file"}, {Name: "use_skill"}} + + // Anonymous caller → legacy set only. + got := filter(context.Background(), listed) + if len(got) != 1 || got[0].Name != "read_file" { + t.Errorf("anonymous list = %v, want [read_file]", toolNames(got)) + } + + // Policy'd agent → derived surface. + ctx := tools.WithToolAgentPolicy(context.Background(), + &config.ToolPolicySpec{Allow: []string{"use_skill"}}) + got = filter(ctx, listed) + if len(got) != 1 || got[0].Name != "use_skill" { + t.Errorf("policy'd list = %v, want [use_skill]", toolNames(got)) + } +} + +// Registration must cover the whole registry minus hard exclusions, so a +// policy'd agent can actually call tools beyond the legacy set (the drift bug). +func TestBridgeRegisteredToolNames_RegistryMinusExclusions(t *testing.T) { + reg := bridgeTestRegistry("read_file", "use_skill", "spawn") + names := bridgeRegisteredToolNames(reg) + + got := make(map[string]bool, len(names)) + for _, n := range names { + got[n] = true + } + if !got["read_file"] || !got["use_skill"] { + t.Errorf("registered = %v, want read_file + use_skill included", names) + } + if got["spawn"] { + t.Errorf("registered = %v, spawn must be excluded", names) + } +} + +func toolNames(ts []mcpgo.Tool) []string { + out := make([]string, len(ts)) + for i, tt := range ts { + out[i] = tt.Name + } + return out +}