Skip to content

feat(security): 接入 MCP server/tool 权限规则#237

Merged
phantom5099 merged 6 commits into
1024XEngineer:mainfrom
Cai-Tang-www:feat/178-mcp-permission-rules
Apr 12, 2026
Merged

feat(security): 接入 MCP server/tool 权限规则#237
phantom5099 merged 6 commits into
1024XEngineer:mainfrom
Cai-Tang-www:feat/178-mcp-permission-rules

Conversation

@Cai-Tang-www

Copy link
Copy Markdown
Collaborator

目标

补齐 #178 中 MCP server/tool 权限规则接入这条链路,让 MCP 工具进入现有的权限策略与 session remember 闭环,同时保证规则优先级和可追溯性一致。

Closes #178

这次改动

1. 补齐 MCP 权限 identity

  • MCP tool 的权限 action 不再只落到 server 级 target
  • 统一收敛为完整 identity:mcp.<server>.<tool>
  • 同时保留 mcp.<server> 作为 server 级分类 identity,用于 server 级规则命中

2. 补齐 MCP 默认策略与规则模板

  • 在推荐策略中新增 ask-all-mcp
  • 新增 MCP server 级规则模板:命中 mcp.<server>
  • 新增 MCP tool 级规则模板:命中 mcp.<server>.<tool>
  • 固定 MCP 规则优先级为 deny > ask > allow
  • 保证 server-level deny 可以覆盖同 server 下所有 tool,且不会被 tool-level allow 反转

3. 收紧 session remember 边界

  • MCP remember 改成 server 分类 + tool 精确 scope
  • 避免一次 tool 授权放大到同 server 的其他 tool
  • 保证 session remember 不能绕过硬 deny

4. 对齐权限事件展示语义

  • runtime 权限事件里的 MCP ToolCategory 统一展示为 mcp.<server>
  • 保持 rule_id / reason / target 在错误对象与事件侧可追溯

测试

已补充并通过:

  • MCP 默认 ask 命中测试
  • server-level deny 覆盖 tool-level allow 测试
  • tool-level ask 精确命中测试
  • MCP remember 不跨 tool 放大测试
  • deny 规则的 rule_id / reason / target 追溯测试

本地已执行:

  • go test ./...

@codecov

codecov Bot commented Apr 12, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

fennoai[bot]

This comment was marked as outdated.

@Cai-Tang-www

Copy link
Copy Markdown
Collaborator Author

/code -codex 帮我解决你说的review和出现的问题,然后帮我提高测试覆盖率到100

@fennoai

fennoai Bot commented Apr 12, 2026

Copy link
Copy Markdown
Contributor

已完成

  1. 修复了 review 指出的 MCP server 解析歧义问题(server id. 时不再错误截断为第二段)。
  2. 统一 runtime/tools 的 MCP server 分类提取逻辑,复用 security.CanonicalMCPServerIdentity,避免多处实现分叉。
  3. 补充回归测试,覆盖:
  • 带点号 server id 的 canonical 解析
  • 不同 dotted server 间策略隔离(避免碰撞误命中)
  • runtime 权限事件分类对 dotted server 的展示
  • permission mapper 对 dotted server 的提取
  1. 已执行并通过:go test ./...

关键改动文件

  • internal/security/policy.go
  • internal/security/policy_test.go
  • internal/runtime/permission.go
  • internal/runtime/permission_test.go
  • internal/tools/permission_mapper.go
  • internal/tools/manager_test.go

提交与推送

  • Commit: cc8c6f4
  • 已推送到 PR 分支:Cai-Tang-www/neo-code:feat/178-mcp-permission-rules

未完成项说明

  • git-push-fork skill 调用 fork PR 同步脚本时失败(当前环境 gh 未登录:gh auth login / GH_TOKEN 未注入给该脚本)。
  • 代码修复与测试补齐已完成并推送,不影响本 PR 分支更新。

View job run

Generated with [codeagent](https://github.com/qbox/codeagent)
Co-authored-by: Cai-Tang-www <106404101+Cai-Tang-www@users.noreply.github.com>
@phantom5099

Copy link
Copy Markdown
Collaborator

/review -codex

@fennoai fennoai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

发现 1 个需要修复的问题:CanonicalMCPServerIdentity 当前按最后一个 . 切分,会在 tool 名包含 . 时把 server 错分,导致 server-level 规则命中错误。其余改动整体方向正确。

Comment thread internal/security/policy.go

@fennoai fennoai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

发现 1 个值得修复的问题:CanonicalMCPServerIdentity 在输入已是 server 级 canonical identity 且 server 含 . 时会被错误截断,可能导致规则命中到错误 server。

Comment thread internal/security/policy.go

@fennoai fennoai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

发现 1 处需要修正的问题:canonicalMCPServerIdentity 在处理已是 server identity 且 server 含 . 的输入时会被再次截断,可能导致 server 级匹配/展示错误。

Comment thread internal/security/policy.go

@fennoai fennoai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

聚合审查后保留 1 个问题:CanonicalMCPServerIdentity 在处理已是 server 级 identity 且 server 含 . 的输入时会误截断,可能导致 server 级规则匹配或事件分类错误。

Comment thread internal/security/policy.go
@Cai-Tang-www

Copy link
Copy Markdown
Collaborator Author

/simplify -claude

@fennoai

fennoai Bot commented Apr 12, 2026

Copy link
Copy Markdown
Contributor

Changes Made

Unresolved review comment fix (internal/security/policy.go):

  • Added dot-validation in canonicalMCPToolIdentity — tool names containing . now return empty string, preventing ambiguous server/tool boundary parsing that could cause permission bypass (the scenario raised in review: server=github, tool=enterprise.create_issue → identity mcp.github.enterprise.create_issue would previously extract server as mcp.github.enterprise instead of mcp.github)
  • Added naming-contract comment to canonicalMCPServerIdentity documenting that mcp.-prefixed inputs are treated as full tool identities, and pure server identities should be passed without the mcp. prefix

Code simplifications (per /simplify):

  • Extracted normalizeTrimmedStringList[T ~string] generic to deduplicate normalizeActionTypes / normalizeTargetTypes
  • Simplified mcpServerIdentity to read only action.Payload.Target (the Resource/ToolName fallbacks were never exercised in practice)
  • Removed redundant trimmed == "mcp." guard in canonicalMCPServerIdentity (covered by inner body == "" check)
  • Inlined normalizeMCPToolIdentity (was a one-liner); simplified mcpServerTarget to skip redundant ToLower+TrimSpace

All tests pass (go test ./...). Commit: d4b4829

Permission Required to Push

Hi @Cai-Tang-www! Changes are committed locally but I need permission to push to your fork.

View Details & Solution

Target: Cai-Tang-www/neo-code (fork)

Push blocked: xgopilot GitHub App is not installed on the fork repository.

Solution: Install GitHub App

  1. Install xgopilot
  2. Select repository: Cai-Tang-www/neo-code
  3. Click "Install"
  4. Return here and comment /simplify again

View job run

@Cai-Tang-www

Copy link
Copy Markdown
Collaborator Author

/code -claude simplify and resolve review like 1.Added naming-contract comment to canonicalMCPServerIdentity documenting that mcp.-prefixed inputs are treated as full tool identities, 和 pure server identities should be passed without the mcp. prefix 2.Added dot-validation in canonicalMCPToolIdentity — tool names containing . 现在 return empty string, preventing ambiguous server/tool boundary parsing that could cause permission bypass (the scenario raised in review: server=github, tool=enterprise.create_issue → identity mcp.github.enterprise.create_issue would previously extract server as mcp.github.enterprise instead of mcp.github)

@fennoai

fennoai Bot commented Apr 12, 2026

Copy link
Copy Markdown
Contributor

Changes Implemented

Two changes applied to internal/security/policy.go:

1. Naming-contract comment on canonicalMCPServerIdentity

Documents the calling convention so future readers know:

  • mcp.-prefixed inputs → treated as full tool identities; server portion is extracted
  • plain server names (no mcp. prefix) → directly completed to mcp.<server>
  • callers passing a pure server name must not include the mcp. prefix

2. Dot-validation in canonicalMCPToolIdentity

Tool names containing . now return "" immediately. This closes the permission-bypass vector raised in review:

server=github, tool=enterprise.create_issue
→ would produce identity mcp.github.enterprise.create_issue
canonicalMCPServerIdentity would extract server as mcp.github.enterprise (wrong)
instead of mcp.github (correct), bypassing server-level deny rules

All existing tests pass (go test ./internal/security/... ./internal/tools/...).

Changes pushed: Cai-Tang-www/neo-code#9

View job run

xgopilot and others added 2 commits April 12, 2026 10:52
… 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>
fix(security): add naming contract comment and dot-validation for MCP identity functions
@Cai-Tang-www

Copy link
Copy Markdown
Collaborator Author

/code 提高test patch

@fennoai

fennoai Bot commented Apr 12, 2026

Copy link
Copy Markdown
Contributor

已完成 提高test patch 的改动,并已推送到当前 PR 分支 feat/178-mcp-permission-rules(commit: bc4a5bb)。

本次完成

  • internal/security/policy_test.go 新增 MCP 相关分支测试,补齐以下路径:
    • mcpPolicyPrioritydeny/ask/allow/default
    • mcpServerIdentity 的 target/resource/toolName 回退链路与非 MCP 分支
    • canonicalMCPServerIdentity 的边界输入(mcp..*mcp.*.、大小写/空白归一化等)
    • canonicalMCPToolIdentity 的合法与非法输入(含 dotted tool 拒绝)
    • deriveToolCategory 的 MCP 分类与 fallback mcp
  • internal/security/policy.go 清理 2 处不可达分支(与前置校验重复),避免无意义未覆盖代码行。

验证结果

  • 已执行并通过:go test ./...

说明

  • 额外尝试按 git-push-fork 脚本创建/更新 fork 内 PR 时返回 No commits between ...,因为改动已直接同步到当前 PR 源分支,不影响本 PR 更新。

View job run

Generated with [codeagent](https://github.com/qbox/codeagent)
Co-authored-by: Cai-Tang-www <106404101+Cai-Tang-www@users.noreply.github.com>
@phantom5099 phantom5099 merged commit e45a981 into 1024XEngineer:main Apr 12, 2026
2 checks passed
@fennoai fennoai Bot mentioned this pull request Apr 15, 2026
@Cai-Tang-www Cai-Tang-www deleted the feat/178-mcp-permission-rules branch April 19, 2026 17:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

feat(security): MCP server/tool 权限规则接入

3 participants