From 0b9dc7ab236cc89017246e42f543de12c492bac6 Mon Sep 17 00:00:00 2001 From: Cai_Tang <106404101+Cai-Tang-www@users.noreply.github.com> Date: Sun, 12 Apr 2026 12:36:47 +0800 Subject: [PATCH 1/5] =?UTF-8?q?feat(security):=20=E8=A1=A5=E9=BD=90=20MCP?= =?UTF-8?q?=20=E6=9D=83=E9=99=90=20identity=20=E4=B8=8E=E8=A7=84=E5=88=99?= =?UTF-8?q?=E6=A8=A1=E6=9D=BF?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- internal/runtime/permission.go | 9 +++ internal/security/policy.go | 108 ++++++++++++++++++++++++++++ internal/tools/permission_mapper.go | 17 +++-- internal/tools/session_memory.go | 7 +- 4 files changed, 133 insertions(+), 8 deletions(-) diff --git a/internal/runtime/permission.go b/internal/runtime/permission.go index 00a0fb76..688a9b5e 100644 --- a/internal/runtime/permission.go +++ b/internal/runtime/permission.go @@ -286,6 +286,15 @@ func permissionToolCategory(action security.Action) string { if strings.HasPrefix(resource, "filesystem_") { return "filesystem_write" } + case security.ActionTypeMCP: + target := strings.ToLower(strings.TrimSpace(action.Payload.Target)) + if strings.HasPrefix(target, "mcp.") { + parts := strings.Split(target, ".") + if len(parts) >= 2 && parts[1] != "" { + return "mcp." + parts[1] + } + } + return "mcp" } if resource != "" { return resource diff --git a/internal/security/policy.go b/internal/security/policy.go index f414655c..ffa2bc39 100644 --- a/internal/security/policy.go +++ b/internal/security/policy.go @@ -149,6 +149,7 @@ func NewRecommendedPolicyEngine() (*PolicyEngine, error) { reasonAskSensitiveRead = "reading sensitive path requires approval" reasonAllowWebfetchDomain = "approved web domain" reasonAskWebfetchDomain = "external web domain requires approval" + reasonAskMCP = "mcp tool requires approval" ) rules := []PolicyRule{ @@ -212,6 +213,14 @@ func NewRecommendedPolicyEngine() (*PolicyEngine, error) { HostPatterns: []string{"github.com", "*.github.com"}, RequireHostMissing: true, }, + { + ID: "ask-all-mcp", + Priority: 720, + Decision: DecisionAsk, + Reason: reasonAskMCP, + ActionTypes: []ActionType{ActionTypeMCP}, + TargetTypes: []TargetType{TargetTypeMCP}, + }, } return NewPolicyEngine(DecisionAllow, rules) @@ -391,6 +400,11 @@ func deriveToolCategory(action Action) string { } case ActionTypeBash: return "bash" + case ActionTypeMCP: + if serverIdentity := mcpServerIdentity(action); serverIdentity != "" { + return serverIdentity + } + return "mcp" } if resource != "" { return resource @@ -398,6 +412,100 @@ func deriveToolCategory(action Action) string { return strings.ToLower(strings.TrimSpace(action.Payload.ToolName)) } +// newMCPServerPolicyRule 生成 MCP server 级规则模板；优先级按 deny > ask > allow 固定。 +func newMCPServerPolicyRule(id string, decision Decision, serverID string, reason string) PolicyRule { + serverIdentity := canonicalMCPServerIdentity(serverID) + return PolicyRule{ + ID: strings.TrimSpace(id), + Priority: mcpPolicyPriority(decision), + Decision: decision, + Reason: strings.TrimSpace(reason), + ActionTypes: []ActionType{ActionTypeMCP}, + ToolCategories: []string{serverIdentity}, + TargetTypes: []TargetType{TargetTypeMCP}, + } +} + +// newMCPToolPolicyRule 生成 MCP tool 级规则模板；target/resource 均命中 mcp.. identity。 +func newMCPToolPolicyRule(id string, decision Decision, serverID string, toolName string, reason string) PolicyRule { + toolIdentity := canonicalMCPToolIdentity(serverID, toolName) + serverIdentity := canonicalMCPServerIdentity(serverID) + return PolicyRule{ + ID: strings.TrimSpace(id), + Priority: mcpPolicyPriority(decision), + Decision: decision, + Reason: strings.TrimSpace(reason), + ActionTypes: []ActionType{ActionTypeMCP}, + ResourcePatterns: []string{toolIdentity}, + ToolCategories: []string{serverIdentity}, + TargetTypes: []TargetType{TargetTypeMCP}, + } +} + +// mcpPolicyPriority 返回 MCP 权限规则的固定优先级，确保 deny > ask > allow。 +func mcpPolicyPriority(decision Decision) int { + switch decision { + case DecisionDeny: + return 830 + case DecisionAsk: + return 820 + case DecisionAllow: + return 810 + default: + return 0 + } +} + +// mcpServerIdentity 从 action 中提取 MCP server identity：mcp.。 +func mcpServerIdentity(action Action) string { + if action.Type != ActionTypeMCP { + return "" + } + candidates := []string{ + strings.TrimSpace(action.Payload.Target), + strings.TrimSpace(action.Payload.Resource), + strings.TrimSpace(action.Payload.ToolName), + } + for _, candidate := range candidates { + if identity := canonicalMCPServerIdentity(candidate); identity != "" { + return identity + } + } + return "" +} + +// canonicalMCPServerIdentity 将 server 标识归一为 mcp. 形式。 +func canonicalMCPServerIdentity(raw string) string { + trimmed := strings.ToLower(strings.TrimSpace(raw)) + if trimmed == "" { + return "" + } + if strings.HasPrefix(trimmed, "mcp.") { + parts := strings.Split(trimmed, ".") + if len(parts) >= 2 && parts[1] != "" { + return "mcp." + parts[1] + } + return "" + } + return "mcp." + trimmed +} + +// canonicalMCPToolIdentity 将 server/tool 标识归一为 mcp..。 +func canonicalMCPToolIdentity(serverID string, toolName string) string { + serverIdentity := canonicalMCPServerIdentity(serverID) + tool := strings.ToLower(strings.TrimSpace(toolName)) + if serverIdentity == "" || tool == "" { + return "" + } + if strings.HasPrefix(tool, serverIdentity+".") { + return tool + } + if strings.HasPrefix(tool, "mcp.") { + return tool + } + return serverIdentity + "." + tool +} + func classifySensitivePath(normalizedTargetPath string) bool { if normalizedTargetPath == "" { return false diff --git a/internal/tools/permission_mapper.go b/internal/tools/permission_mapper.go index 946d1084..d3131100 100644 --- a/internal/tools/permission_mapper.go +++ b/internal/tools/permission_mapper.go @@ -74,11 +74,12 @@ func buildPermissionAction(input ToolCallInput) (security.Action, error) { action.Payload.SandboxTarget = action.Payload.Target default: if strings.HasPrefix(strings.ToLower(toolName), "mcp.") { + toolIdentity := normalizeMCPToolIdentity(toolName) action.Type = security.ActionTypeMCP action.Payload.Operation = "invoke" action.Payload.TargetType = security.TargetTypeMCP - action.Payload.Target = mcpServerTarget(toolName) - action.Payload.Resource = toolName + action.Payload.Target = toolIdentity + action.Payload.Resource = toolIdentity return action, nil } return security.Action{}, fmt.Errorf("tools: unsupported permission mapping for %q", input.Name) @@ -87,12 +88,18 @@ func buildPermissionAction(input ToolCallInput) (security.Action, error) { return action, nil } +// normalizeMCPToolIdentity 将 MCP 工具名归一为稳定的全量 identity：mcp..。 +func normalizeMCPToolIdentity(toolName string) string { + return strings.ToLower(strings.TrimSpace(toolName)) +} + +// mcpServerTarget 从 MCP 工具全名中提取 server 级 identity：mcp.。 func mcpServerTarget(toolName string) string { - parts := strings.Split(strings.TrimSpace(toolName), ".") - if len(parts) < 2 { + parts := strings.Split(normalizeMCPToolIdentity(toolName), ".") + if len(parts) < 2 || parts[0] != "mcp" || parts[1] == "" { return "" } - return parts[1] + return "mcp." + parts[1] } func extractStringArgument(raw []byte, key string) string { diff --git a/internal/tools/session_memory.go b/internal/tools/session_memory.go index 91006a14..5f1a5d7e 100644 --- a/internal/tools/session_memory.go +++ b/internal/tools/session_memory.go @@ -152,9 +152,8 @@ func sessionPermissionCategory(action security.Action) string { case security.ActionTypeBash: return "bash" case security.ActionTypeMCP: - target := strings.ToLower(strings.TrimSpace(action.Payload.Target)) - if target != "" { - return "mcp:" + target + if serverIdentity := mcpServerTarget(action.Payload.Target); serverIdentity != "" { + return serverIdentity } return "mcp" } @@ -180,6 +179,8 @@ func sessionPermissionTargetScope(action security.Action) string { return normalizePermissionPathTarget(filepath.Dir(target)) case security.TargetTypeDirectory: return normalizePermissionPathTarget(target) + case security.TargetTypeMCP: + return normalizeMCPToolIdentity(target) default: return strings.ToLower(target) } From 1e44363adc1e348e7f4d0c12f8ebc4b28131492f Mon Sep 17 00:00:00 2001 From: Cai_Tang <106404101+Cai-Tang-www@users.noreply.github.com> Date: Sun, 12 Apr 2026 12:36:47 +0800 Subject: [PATCH 2/5] =?UTF-8?q?test(security):=20=E8=A1=A5=E9=BD=90=20MCP?= =?UTF-8?q?=20=E6=9D=83=E9=99=90=E5=91=BD=E4=B8=AD=E4=B8=8E=20remember=20?= =?UTF-8?q?=E8=BE=B9=E7=95=8C=E6=B5=8B=E8=AF=95?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- internal/security/policy_test.go | 126 +++++++++++++++++++++++ internal/tools/manager_test.go | 142 +++++++++++++++++++++++++- internal/tools/session_memory_test.go | 50 ++++++++- 3 files changed, 314 insertions(+), 4 deletions(-) diff --git a/internal/security/policy_test.go b/internal/security/policy_test.go index ab12d2ee..c70c8579 100644 --- a/internal/security/policy_test.go +++ b/internal/security/policy_test.go @@ -139,6 +139,21 @@ func TestPolicyEngineRecommendedRules(t *testing.T) { wantDecision: DecisionAsk, wantRuleID: "ask-webfetch-non-whitelist", }, + { + name: "mcp defaults to ask", + action: Action{ + Type: ActionTypeMCP, + Payload: ActionPayload{ + ToolName: "mcp.docs.search", + Resource: "mcp.docs.search", + Operation: "invoke", + TargetType: TargetTypeMCP, + Target: "mcp.docs.search", + }, + }, + wantDecision: DecisionAsk, + wantRuleID: "ask-all-mcp", + }, } for _, tt := range tests { @@ -187,3 +202,114 @@ func TestNewPolicyEngineValidation(t *testing.T) { t.Fatalf("expected invalid rule decision error") } } + +func TestPolicyEngineMCPRuleTemplates(t *testing.T) { + t.Parallel() + + engine, err := NewPolicyEngine(DecisionAllow, []PolicyRule{ + newMCPToolPolicyRule("allow-github-create-issue", DecisionAllow, "github", "create_issue", "tool allowed"), + newMCPServerPolicyRule("deny-github-server", DecisionDeny, "github", "server blocked"), + newMCPToolPolicyRule("ask-docs-search", DecisionAsk, "docs", "search", "search requires approval"), + }) + if err != nil { + t.Fatalf("new policy engine: %v", err) + } + + tests := []struct { + name string + action Action + wantDecision Decision + wantRuleID string + wantReason string + }{ + { + name: "server-level deny overrides tool-level allow", + action: Action{ + Type: ActionTypeMCP, + Payload: ActionPayload{ + ToolName: "mcp.github.create_issue", + Resource: "mcp.github.create_issue", + Operation: "invoke", + TargetType: TargetTypeMCP, + Target: "mcp.github.create_issue", + }, + }, + wantDecision: DecisionDeny, + wantRuleID: "deny-github-server", + wantReason: "server blocked", + }, + { + name: "server-level deny covers all tools on same server", + action: Action{ + Type: ActionTypeMCP, + Payload: ActionPayload{ + ToolName: "mcp.github.list_issues", + Resource: "mcp.github.list_issues", + Operation: "invoke", + TargetType: TargetTypeMCP, + Target: "mcp.github.list_issues", + }, + }, + wantDecision: DecisionDeny, + wantRuleID: "deny-github-server", + wantReason: "server blocked", + }, + { + name: "tool-level ask hits exact tool identity", + action: Action{ + Type: ActionTypeMCP, + Payload: ActionPayload{ + ToolName: "mcp.docs.search", + Resource: "mcp.docs.search", + Operation: "invoke", + TargetType: TargetTypeMCP, + Target: "mcp.docs.search", + }, + }, + wantDecision: DecisionAsk, + wantRuleID: "ask-docs-search", + wantReason: "search requires approval", + }, + { + name: "other MCP tool falls back to default allow", + action: Action{ + Type: ActionTypeMCP, + Payload: ActionPayload{ + ToolName: "mcp.docs.read", + Resource: "mcp.docs.read", + Operation: "invoke", + TargetType: TargetTypeMCP, + Target: "mcp.docs.read", + }, + }, + wantDecision: DecisionAllow, + }, + } + + for _, tt := range tests { + tt := tt + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + result, checkErr := engine.Check(context.Background(), tt.action) + if checkErr != nil { + t.Fatalf("Check() error = %v", checkErr) + } + if result.Decision != tt.wantDecision { + t.Fatalf("expected decision %q, got %q", tt.wantDecision, result.Decision) + } + if tt.wantRuleID == "" { + if result.Rule != nil { + t.Fatalf("expected no matched rule, got %+v", result.Rule) + } + return + } + if result.Rule == nil || result.Rule.ID != tt.wantRuleID { + t.Fatalf("expected rule id %q, got %+v", tt.wantRuleID, result.Rule) + } + if result.Reason != tt.wantReason { + t.Fatalf("expected reason %q, got %q", tt.wantReason, result.Reason) + } + }) + } +} diff --git a/internal/tools/manager_test.go b/internal/tools/manager_test.go index 45e7de66..b5d589ca 100644 --- a/internal/tools/manager_test.go +++ b/internal/tools/manager_test.go @@ -10,6 +10,7 @@ import ( providertypes "neo-code/internal/provider/types" "neo-code/internal/security" + "neo-code/internal/tools/mcp" ) type managerStubTool struct { @@ -947,7 +948,7 @@ func TestBuildPermissionAction(t *testing.T) { }, wantType: security.ActionTypeMCP, wantResource: "mcp.github.create_issue", - wantTarget: "github", + wantTarget: "mcp.github.create_issue", }, { name: "unsupported tool returns error", @@ -1032,7 +1033,7 @@ func TestPermissionMapperHelpers(t *testing.T) { { name: "mcp server target with server and tool", serverTool: "mcp.github.create_issue", - serverWant: "github", + serverWant: "mcp.github", }, { name: "mcp server target without server", @@ -1060,6 +1061,143 @@ func TestPermissionMapperHelpers(t *testing.T) { } } +func TestDefaultManagerExecuteMCPRememberDoesNotBroadenAcrossTools(t *testing.T) { + t.Parallel() + + registry := NewRegistry() + mcpRegistry := mcp.NewRegistry() + if err := mcpRegistry.RegisterServer("github", "stdio", "v1", &stubMCPClient{ + tools: []mcp.ToolDescriptor{ + {Name: "create_issue", Description: "create"}, + {Name: "list_issues", Description: "list"}, + }, + callResult: mcp.CallResult{Content: "ok"}, + }); err != nil { + t.Fatalf("register mcp server: %v", err) + } + if err := mcpRegistry.RefreshServerTools(context.Background(), "github"); err != nil { + t.Fatalf("refresh mcp tools: %v", err) + } + registry.SetMCPRegistry(mcpRegistry) + + engine, err := security.NewStaticGateway(security.DecisionAllow, []security.Rule{ + { + ID: "ask-github-create-issue", + Type: security.ActionTypeMCP, + Resource: "mcp.github.create_issue", + Decision: security.DecisionAsk, + Reason: "create issue requires approval", + }, + { + ID: "ask-github-list-issues", + Type: security.ActionTypeMCP, + Resource: "mcp.github.list_issues", + Decision: security.DecisionAsk, + Reason: "list issues requires approval", + }, + }) + if err != nil { + t.Fatalf("new engine: %v", err) + } + + manager, err := NewManager(registry, engine, nil) + if err != nil { + t.Fatalf("new manager: %v", err) + } + + sessionID := "session-mcp-target-scope" + createInput := ToolCallInput{ + ID: "call-create", + Name: "mcp.github.create_issue", + Arguments: []byte(`{"title":"hello"}`), + SessionID: sessionID, + } + listInput := ToolCallInput{ + ID: "call-list", + Name: "mcp.github.list_issues", + Arguments: []byte(`{"state":"open"}`), + SessionID: sessionID, + } + + _, err = manager.Execute(context.Background(), createInput) + var permissionErr *PermissionDecisionError + if !errors.As(err, &permissionErr) || permissionErr.Decision() != "ask" { + t.Fatalf("expected initial MCP ask, got %v", err) + } + if rememberErr := manager.RememberSessionDecision(sessionID, permissionErr.Action(), SessionPermissionScopeAlways); rememberErr != nil { + t.Fatalf("remember mcp create_issue: %v", rememberErr) + } + + if _, err := manager.Execute(context.Background(), createInput); err != nil { + t.Fatalf("expected remembered create_issue allow, got %v", err) + } + + _, err = manager.Execute(context.Background(), listInput) + if !errors.As(err, &permissionErr) || permissionErr.Decision() != "ask" { + t.Fatalf("expected list_issues to require independent approval, got %v", err) + } +} + +func TestDefaultManagerExecuteMCPServerDenyUsesTraceableRule(t *testing.T) { + t.Parallel() + + registry := NewRegistry() + mcpRegistry := mcp.NewRegistry() + if err := mcpRegistry.RegisterServer("github", "stdio", "v1", &stubMCPClient{ + tools: []mcp.ToolDescriptor{ + {Name: "create_issue", Description: "create"}, + }, + callResult: mcp.CallResult{Content: "ok"}, + }); err != nil { + t.Fatalf("register mcp server: %v", err) + } + if err := mcpRegistry.RefreshServerTools(context.Background(), "github"); err != nil { + t.Fatalf("refresh mcp tools: %v", err) + } + registry.SetMCPRegistry(mcpRegistry) + + engine, err := security.NewStaticGateway(security.DecisionAllow, []security.Rule{ + { + ID: "deny-github-server", + Type: security.ActionTypeMCP, + TargetPrefix: "mcp.github", + Decision: security.DecisionDeny, + Reason: "github MCP server denied", + }, + }) + if err != nil { + t.Fatalf("new engine: %v", err) + } + + manager, err := NewManager(registry, engine, nil) + if err != nil { + t.Fatalf("new manager: %v", err) + } + + _, execErr := manager.Execute(context.Background(), ToolCallInput{ + ID: "call-mcp-deny", + Name: "mcp.github.create_issue", + Arguments: []byte(`{"title":"hello"}`), + SessionID: "session-mcp-deny", + }) + var permissionErr *PermissionDecisionError + if !errors.As(execErr, &permissionErr) { + t.Fatalf("expected permission error, got %v", execErr) + } + if permissionErr.Decision() != "deny" { + t.Fatalf("expected deny, got %q", permissionErr.Decision()) + } + if permissionErr.RuleID() != "deny-github-server" { + t.Fatalf("expected rule id deny-github-server, got %q", permissionErr.RuleID()) + } + if permissionErr.Reason() != "github MCP server denied" { + t.Fatalf("expected deny reason propagated, got %q", permissionErr.Reason()) + } + if permissionErr.Action().Payload.Target != "mcp.github.create_issue" { + t.Fatalf("expected full mcp target identity, got %q", permissionErr.Action().Payload.Target) + } +} + func TestNoopWorkspaceSandbox(t *testing.T) { t.Parallel() diff --git a/internal/tools/session_memory_test.go b/internal/tools/session_memory_test.go index 194b316e..951ef431 100644 --- a/internal/tools/session_memory_test.go +++ b/internal/tools/session_memory_test.go @@ -157,10 +157,10 @@ func TestSessionPermissionCategoryAndActionKey(t *testing.T) { action: security.Action{ Type: security.ActionTypeMCP, Payload: security.ActionPayload{ - Target: "Server-A", + Target: "mcp.server-a.tool-a", }, }, - expected: "mcp:server-a", + expected: "mcp.server-a", }, { name: "mcp without target", @@ -281,3 +281,49 @@ func TestSessionPermissionMemoryResolveRequiresTargetScopeMatch(t *testing.T) { t.Fatalf("expected different path file action to miss memory") } } + +func TestSessionPermissionMemoryResolveRequiresMCPToolScopeMatch(t *testing.T) { + t.Parallel() + + memory := newSessionPermissionMemory() + sessionID := "session-mcp-tool-scope" + + createIssue := security.Action{ + Type: security.ActionTypeMCP, + Payload: security.ActionPayload{ + ToolName: "mcp.github.create_issue", + Resource: "mcp.github.create_issue", + TargetType: security.TargetTypeMCP, + Target: "mcp.github.create_issue", + }, + } + if err := memory.remember(sessionID, createIssue, SessionPermissionScopeAlways); err != nil { + t.Fatalf("remember create_issue action: %v", err) + } + + sameTool := security.Action{ + Type: security.ActionTypeMCP, + Payload: security.ActionPayload{ + ToolName: "mcp.github.create_issue", + Resource: "mcp.github.create_issue", + TargetType: security.TargetTypeMCP, + Target: "mcp.github.create_issue", + }, + } + if _, _, ok := memory.resolve(sessionID, sameTool); !ok { + t.Fatalf("expected same MCP tool identity to hit memory") + } + + otherToolSameServer := security.Action{ + Type: security.ActionTypeMCP, + Payload: security.ActionPayload{ + ToolName: "mcp.github.list_issues", + Resource: "mcp.github.list_issues", + TargetType: security.TargetTypeMCP, + Target: "mcp.github.list_issues", + }, + } + if _, _, ok := memory.resolve(sessionID, otherToolSameServer); ok { + t.Fatalf("expected other MCP tool on same server to miss memory") + } +} From cc8c6f40b991b84853f364d52c267d00f9622b63 Mon Sep 17 00:00:00 2001 From: xgopilot Date: Sun, 12 Apr 2026 04:58:55 +0000 Subject: [PATCH 3/5] fix(security): resolve MCP server identity parsing collisions Generated with [codeagent](https://github.com/qbox/codeagent) Co-authored-by: Cai-Tang-www <106404101+Cai-Tang-www@users.noreply.github.com> --- internal/runtime/permission.go | 7 +- internal/runtime/permission_test.go | 20 ++++++ internal/security/policy.go | 24 +++++-- internal/security/policy_test.go | 104 ++++++++++++++++++++++++++++ internal/tools/manager_test.go | 5 ++ internal/tools/permission_mapper.go | 6 +- 6 files changed, 151 insertions(+), 15 deletions(-) diff --git a/internal/runtime/permission.go b/internal/runtime/permission.go index 688a9b5e..4af2ae3c 100644 --- a/internal/runtime/permission.go +++ b/internal/runtime/permission.go @@ -288,11 +288,8 @@ func permissionToolCategory(action security.Action) string { } case security.ActionTypeMCP: target := strings.ToLower(strings.TrimSpace(action.Payload.Target)) - if strings.HasPrefix(target, "mcp.") { - parts := strings.Split(target, ".") - if len(parts) >= 2 && parts[1] != "" { - return "mcp." + parts[1] - } + if serverIdentity := security.CanonicalMCPServerIdentity(target); serverIdentity != "" { + return serverIdentity } return "mcp" } diff --git a/internal/runtime/permission_test.go b/internal/runtime/permission_test.go index 86513069..6e00dabe 100644 --- a/internal/runtime/permission_test.go +++ b/internal/runtime/permission_test.go @@ -298,6 +298,26 @@ func TestPermissionHelpers(t *testing.T) { if category != "webfetch" { t.Fatalf("expected webfetch category, got %q", category) } + + category = permissionToolCategory(security.Action{ + Type: security.ActionTypeMCP, + Payload: security.ActionPayload{ + Target: "mcp.github.enterprise.create_issue", + }, + }) + if category != "mcp.github.enterprise" { + t.Fatalf("expected mcp.github.enterprise category, got %q", category) + } + + category = permissionToolCategory(security.Action{ + Type: security.ActionTypeMCP, + Payload: security.ActionPayload{ + Target: "mcp", + }, + }) + if category != "mcp" { + t.Fatalf("expected mcp fallback category, got %q", category) + } } func TestResolvePermissionCanceledContext(t *testing.T) { diff --git a/internal/security/policy.go b/internal/security/policy.go index ffa2bc39..06316d95 100644 --- a/internal/security/policy.go +++ b/internal/security/policy.go @@ -474,18 +474,32 @@ func mcpServerIdentity(action Action) string { return "" } +// CanonicalMCPServerIdentity 将输入标识归一为 MCP server 级 identity（mcp.）。 +func CanonicalMCPServerIdentity(raw string) string { + return canonicalMCPServerIdentity(raw) +} + // canonicalMCPServerIdentity 将 server 标识归一为 mcp. 形式。 func canonicalMCPServerIdentity(raw string) string { trimmed := strings.ToLower(strings.TrimSpace(raw)) - if trimmed == "" { + if trimmed == "" || trimmed == "mcp" || trimmed == "mcp." { return "" } if strings.HasPrefix(trimmed, "mcp.") { - parts := strings.Split(trimmed, ".") - if len(parts) >= 2 && parts[1] != "" { - return "mcp." + parts[1] + body := strings.TrimPrefix(trimmed, "mcp.") + if body == "" { + return "" } - return "" + // MCP 工具 identity 采用 mcp..，server 允许包含 "."； + // 因此按最后一个 "." 分隔 server 与 tool，避免将 server 错误截断到第二段。 + lastDot := strings.LastIndex(body, ".") + if lastDot == -1 { + return "mcp." + body + } + if lastDot == 0 || lastDot == len(body)-1 { + return "" + } + return "mcp." + body[:lastDot] } return "mcp." + trimmed } diff --git a/internal/security/policy_test.go b/internal/security/policy_test.go index c70c8579..b5b7ee3b 100644 --- a/internal/security/policy_test.go +++ b/internal/security/policy_test.go @@ -313,3 +313,107 @@ func TestPolicyEngineMCPRuleTemplates(t *testing.T) { }) } } + +func TestCanonicalMCPServerIdentity(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + input string + want string + }{ + { + name: "extract from full tool identity with dotted server", + input: "mcp.github.enterprise.create_issue", + want: "mcp.github.enterprise", + }, + { + name: "normalize raw server id with dot", + input: "github.enterprise", + want: "mcp.github.enterprise", + }, + { + name: "extract from normal tool identity", + input: "mcp.github.search", + want: "mcp.github", + }, + { + name: "invalid mcp token returns empty", + input: "mcp", + want: "", + }, + { + name: "public wrapper follows canonical behavior", + input: "mcp.github.public.search", + want: "mcp.github.public", + }, + } + + for _, tt := range tests { + tt := tt + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + if got := canonicalMCPServerIdentity(tt.input); got != tt.want { + t.Fatalf("canonicalMCPServerIdentity() = %q, want %q", got, tt.want) + } + if got := CanonicalMCPServerIdentity(tt.input); got != tt.want { + t.Fatalf("CanonicalMCPServerIdentity() = %q, want %q", got, tt.want) + } + }) + } +} + +func TestPolicyEngineMCPDottedServerIsolation(t *testing.T) { + t.Parallel() + + engine, err := NewPolicyEngine(DecisionAllow, []PolicyRule{ + newMCPServerPolicyRule("deny-github-enterprise", DecisionDeny, "github.enterprise", "enterprise denied"), + newMCPToolPolicyRule("allow-github-public-search", DecisionAllow, "github.public", "search", "public allowed"), + }) + if err != nil { + t.Fatalf("new policy engine: %v", err) + } + + enterpriseAction := Action{ + Type: ActionTypeMCP, + Payload: ActionPayload{ + ToolName: "mcp.github.enterprise.search", + Resource: "mcp.github.enterprise.search", + Operation: "invoke", + TargetType: TargetTypeMCP, + Target: "mcp.github.enterprise.search", + }, + } + enterpriseResult, checkErr := engine.Check(context.Background(), enterpriseAction) + if checkErr != nil { + t.Fatalf("check enterprise action: %v", checkErr) + } + if enterpriseResult.Decision != DecisionDeny { + t.Fatalf("expected enterprise action deny, got %q", enterpriseResult.Decision) + } + if enterpriseResult.Rule == nil || enterpriseResult.Rule.ID != "deny-github-enterprise" { + t.Fatalf("expected enterprise deny rule, got %+v", enterpriseResult.Rule) + } + + publicAction := Action{ + Type: ActionTypeMCP, + Payload: ActionPayload{ + ToolName: "mcp.github.public.search", + Resource: "mcp.github.public.search", + Operation: "invoke", + TargetType: TargetTypeMCP, + Target: "mcp.github.public.search", + }, + } + publicResult, checkErr := engine.Check(context.Background(), publicAction) + if checkErr != nil { + t.Fatalf("check public action: %v", checkErr) + } + if publicResult.Decision != DecisionAllow { + t.Fatalf("expected public action allow, got %q", publicResult.Decision) + } + if publicResult.Rule == nil || publicResult.Rule.ID != "allow-github-public-search" { + t.Fatalf("expected public allow rule, got %+v", publicResult.Rule) + } +} diff --git a/internal/tools/manager_test.go b/internal/tools/manager_test.go index b5d589ca..757794d4 100644 --- a/internal/tools/manager_test.go +++ b/internal/tools/manager_test.go @@ -1035,6 +1035,11 @@ func TestPermissionMapperHelpers(t *testing.T) { serverTool: "mcp.github.create_issue", serverWant: "mcp.github", }, + { + name: "mcp server target keeps dotted server id", + serverTool: "mcp.github.enterprise.create_issue", + serverWant: "mcp.github.enterprise", + }, { name: "mcp server target without server", serverTool: "mcp", diff --git a/internal/tools/permission_mapper.go b/internal/tools/permission_mapper.go index d3131100..70e2e1bb 100644 --- a/internal/tools/permission_mapper.go +++ b/internal/tools/permission_mapper.go @@ -95,11 +95,7 @@ func normalizeMCPToolIdentity(toolName string) string { // mcpServerTarget 从 MCP 工具全名中提取 server 级 identity：mcp.。 func mcpServerTarget(toolName string) string { - parts := strings.Split(normalizeMCPToolIdentity(toolName), ".") - if len(parts) < 2 || parts[0] != "mcp" || parts[1] == "" { - return "" - } - return "mcp." + parts[1] + return security.CanonicalMCPServerIdentity(normalizeMCPToolIdentity(toolName)) } func extractStringArgument(raw []byte, key string) string { From 333f65460c62e8ea18bed71f70f18fa376c515c0 Mon Sep 17 00:00:00 2001 From: xgopilot Date: Sun, 12 Apr 2026 10:52:28 +0000 Subject: [PATCH 4/5] fix(security): add naming contract comment and dot-validation for MCP identity functions - Added naming-contract comment to canonicalMCPServerIdentity documenting that mcp.-prefixed inputs are treated as full tool identities (server extracted from them), and pure server identities should be passed without the mcp. prefix - Added dot-validation in canonicalMCPToolIdentity: tool names containing "." now return empty string, preventing ambiguous server/tool boundary parsing that could cause permission bypass (e.g. server=github, tool=enterprise.create_issue would produce mcp.github.enterprise.create_issue, causing server to be incorrectly extracted as mcp.github.enterprise instead of mcp.github) Generated with [codeagent](https://github.com/qbox/codeagent) Co-authored-by: Cai-Tang-www <106404101+Cai-Tang-www@users.noreply.github.com> --- internal/security/policy.go | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/internal/security/policy.go b/internal/security/policy.go index 06316d95..df920fbe 100644 --- a/internal/security/policy.go +++ b/internal/security/policy.go @@ -480,6 +480,13 @@ func CanonicalMCPServerIdentity(raw string) string { } // canonicalMCPServerIdentity 将 server 标识归一为 mcp. 形式。 +// +// 命名约定 (naming contract)： +// - 以 "mcp." 开头的输入被视为完整的 tool identity（mcp..）； +// 函数将从中提取 server 部分并返回 mcp.。 +// - 不带 "mcp." 前缀的输入被视为纯 server 名称，函数直接补全为 mcp.。 +// - 调用方传入纯 server 名称时 **不应** 携带 "mcp." 前缀； +// 如需从 tool identity 提取 server，传入完整 mcp.. 即可。 func canonicalMCPServerIdentity(raw string) string { trimmed := strings.ToLower(strings.TrimSpace(raw)) if trimmed == "" || trimmed == "mcp" || trimmed == "mcp." { @@ -505,12 +512,24 @@ func canonicalMCPServerIdentity(raw string) string { } // canonicalMCPToolIdentity 将 server/tool 标识归一为 mcp..。 +// +// tool 名称不得包含 "."；含 "." 的 toolName 会返回空串并被视为非法输入。 +// 这可防止 server/tool 边界解析歧义，例如： +// +// server="github", toolName="enterprise.create_issue" +// → 若允许，拼接后 identity 为 mcp.github.enterprise.create_issue +// → canonicalMCPServerIdentity 将错误地提取 server 为 mcp.github.enterprise +// 而非正确的 mcp.github，导致权限绕过。 func canonicalMCPToolIdentity(serverID string, toolName string) string { serverIdentity := canonicalMCPServerIdentity(serverID) tool := strings.ToLower(strings.TrimSpace(toolName)) if serverIdentity == "" || tool == "" { return "" } + // Reject tool names containing "." to prevent ambiguous server/tool boundary parsing. + if strings.Contains(tool, ".") { + return "" + } if strings.HasPrefix(tool, serverIdentity+".") { return tool } From bc4a5bbe5f2e02feecdb714e6e206b6b78aed9c6 Mon Sep 17 00:00:00 2001 From: xgopilot Date: Sun, 12 Apr 2026 11:18:30 +0000 Subject: [PATCH 5/5] test(security): improve MCP policy patch coverage Generated with [codeagent](https://github.com/qbox/codeagent) Co-authored-by: Cai-Tang-www <106404101+Cai-Tang-www@users.noreply.github.com> --- internal/security/policy.go | 9 -- internal/security/policy_test.go | 189 +++++++++++++++++++++++++++++++ 2 files changed, 189 insertions(+), 9 deletions(-) diff --git a/internal/security/policy.go b/internal/security/policy.go index df920fbe..1bf51e85 100644 --- a/internal/security/policy.go +++ b/internal/security/policy.go @@ -494,9 +494,6 @@ func canonicalMCPServerIdentity(raw string) string { } if strings.HasPrefix(trimmed, "mcp.") { body := strings.TrimPrefix(trimmed, "mcp.") - if body == "" { - return "" - } // MCP 工具 identity 采用 mcp..，server 允许包含 "."； // 因此按最后一个 "." 分隔 server 与 tool，避免将 server 错误截断到第二段。 lastDot := strings.LastIndex(body, ".") @@ -530,12 +527,6 @@ func canonicalMCPToolIdentity(serverID string, toolName string) string { if strings.Contains(tool, ".") { return "" } - if strings.HasPrefix(tool, serverIdentity+".") { - return tool - } - if strings.HasPrefix(tool, "mcp.") { - return tool - } return serverIdentity + "." + tool } diff --git a/internal/security/policy_test.go b/internal/security/policy_test.go index b5b7ee3b..d9ee2124 100644 --- a/internal/security/policy_test.go +++ b/internal/security/policy_test.go @@ -417,3 +417,192 @@ func TestPolicyEngineMCPDottedServerIsolation(t *testing.T) { t.Fatalf("expected public allow rule, got %+v", publicResult.Rule) } } + +func TestMCPPolicyPriority(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + decision Decision + want int + }{ + {name: "deny", decision: DecisionDeny, want: 830}, + {name: "ask", decision: DecisionAsk, want: 820}, + {name: "allow", decision: DecisionAllow, want: 810}, + {name: "unknown", decision: Decision("unknown"), want: 0}, + } + + for _, tt := range tests { + tt := tt + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + if got := mcpPolicyPriority(tt.decision); got != tt.want { + t.Fatalf("mcpPolicyPriority() = %d, want %d", got, tt.want) + } + }) + } +} + +func TestMCPServerIdentity(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + action Action + want string + }{ + { + name: "non mcp action returns empty", + action: Action{ + Type: ActionTypeRead, + Payload: ActionPayload{ + Target: "mcp.docs.search", + }, + }, + want: "", + }, + { + name: "use target first", + action: Action{ + Type: ActionTypeMCP, + Payload: ActionPayload{ + Target: "mcp.docs.search", + }, + }, + want: "mcp.docs", + }, + { + name: "fallback to resource when target invalid", + action: Action{ + Type: ActionTypeMCP, + Payload: ActionPayload{ + Target: "mcp.", + Resource: "mcp.repo.search", + }, + }, + want: "mcp.repo", + }, + { + name: "fallback to tool name when target and resource invalid", + action: Action{ + Type: ActionTypeMCP, + Payload: ActionPayload{ + Target: "mcp.", + Resource: "mcp.", + ToolName: "mcp.docs.read", + }, + }, + want: "mcp.docs", + }, + { + name: "all empty returns empty", + action: Action{ + Type: ActionTypeMCP, + }, + want: "", + }, + } + + for _, tt := range tests { + tt := tt + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + if got := mcpServerIdentity(tt.action); got != tt.want { + t.Fatalf("mcpServerIdentity() = %q, want %q", got, tt.want) + } + }) + } +} + +func TestCanonicalMCPServerIdentityEdges(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + input string + want string + }{ + {name: "single segment mcp identity", input: "mcp.docs", want: "mcp.docs"}, + {name: "leading dot in body is invalid", input: "mcp..search", want: ""}, + {name: "trailing dot in body is invalid", input: "mcp.docs.", want: ""}, + {name: "empty mcp identity body is invalid", input: "mcp.", want: ""}, + {name: "trim and lowercase", input: " MCP.DOCS.SEARCH ", want: "mcp.docs"}, + } + + for _, tt := range tests { + tt := tt + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + if got := canonicalMCPServerIdentity(tt.input); got != tt.want { + t.Fatalf("canonicalMCPServerIdentity() = %q, want %q", got, tt.want) + } + }) + } +} + +func TestCanonicalMCPToolIdentity(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + serverID string + toolName string + want string + }{ + {name: "normalize to full tool identity", serverID: "docs", toolName: "search", want: "mcp.docs.search"}, + {name: "invalid server id returns empty", serverID: "mcp.", toolName: "search", want: ""}, + {name: "empty tool returns empty", serverID: "docs", toolName: " ", want: ""}, + {name: "dotted tool name rejected", serverID: "github", toolName: "enterprise.create_issue", want: ""}, + } + + for _, tt := range tests { + tt := tt + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + if got := canonicalMCPToolIdentity(tt.serverID, tt.toolName); got != tt.want { + t.Fatalf("canonicalMCPToolIdentity() = %q, want %q", got, tt.want) + } + }) + } +} + +func TestDeriveToolCategoryMCP(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + action Action + want string + }{ + { + name: "derive mcp server category", + action: Action{ + Type: ActionTypeMCP, + Payload: ActionPayload{ + Target: "mcp.docs.search", + }, + }, + want: "mcp.docs", + }, + { + name: "fallback to mcp category when no identity", + action: Action{ + Type: ActionTypeMCP, + Payload: ActionPayload{ + Target: "mcp.", + }, + }, + want: "mcp", + }, + } + + for _, tt := range tests { + tt := tt + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + if got := deriveToolCategory(tt.action); got != tt.want { + t.Fatalf("deriveToolCategory() = %q, want %q", got, tt.want) + } + }) + } +}