Skip to content

fix(mcp): require trust for read-only auto-approval - #6924

Merged
wenshao merged 1 commit into
QwenLM:mainfrom
morluto:fix/mcp-readonly-trust
Jul 15, 2026
Merged

fix(mcp): require trust for read-only auto-approval#6924
wenshao merged 1 commit into
QwenLM:mainfrom
morluto:fix/mcp-readonly-trust

Conversation

@morluto

@morluto morluto commented Jul 15, 2026

Copy link
Copy Markdown
Contributor

What this PR does

Requires user-controlled MCP server trust before a tool can receive automatic default permission. A server-provided readOnlyHint continues to classify the tool as read-only for plan mode, but no longer grants permission by itself.

Adds regression coverage for annotated tools across untrusted servers, untrusted workspaces, trusted servers in trusted workspaces, and disabled annotations.

Why it's needed

MCP tool annotations come from the server's discovery response. Previously, readOnlyHint: true changed the default permission from ask to allow even when the server was not configured as trusted, bypassing the normal confirmation prompt. Automatic permission should depend on the user's trust configuration, not the server's self-description.

Reviewer Test Plan

How to verify

Build an MCP tool invocation with readOnlyHint: true under each trust combination. Confirm that an untrusted server asks, a trusted server in an untrusted workspace asks, and a trusted server in a trusted workspace remains allowed. Confirm that an absent or false annotation retains the existing behavior and that annotated tools remain classified as read-only.

Evidence (Before & After)

Scenario Before After
Untrusted server with readOnlyHint: true allow ask
Trusted server, untrusted workspace, with readOnlyHint: true allow ask
Trusted server, trusted workspace, with readOnlyHint: true allow allow

The focused regression failed in the first two scenarios before the fix and passes after it. The complete MCP tool test file passes 61 tests.

Tested on

OS Status
🍏 macOS N/A
🪟 Windows N/A
🐧 Linux ✅ tested

Environment (optional)

Node.js 24; focused and complete MCP tool unit tests, repository build, typecheck, lint, Prettier, and targeted ESLint.

Risk & Scope

  • Main risk or tradeoff: Annotation-only MCP tools now prompt unless an explicit permission rule or trusted-server configuration allows them.
  • Not validated / out of scope: MCP annotation transport, read-only classification, permission rule evaluation, and general confirmation-flow refactoring are unchanged.
  • Breaking changes / migration notes: No API migration. Users who relied on annotation-only automatic permission can explicitly trust the server or configure a permission rule.

Linked Issues

Fixes #6917

中文说明

此 PR 的作用

只有在用户明确将 MCP 服务器设为可信时,工具才能获得自动默认许可。服务器提供的 readOnlyHint 仍用于在计划模式中将工具分类为只读,但不再单独授予执行许可。

新增回归测试,覆盖不可信服务器、不可信工作区、可信工作区中的可信服务器,以及禁用注解的情况。

为什么需要此变更

MCP 工具注解来自服务器的发现响应。此前,即使服务器未配置为可信,readOnlyHint: true 也会将默认许可从 ask 改为 allow,从而跳过正常的确认提示。自动许可应由用户的信任配置决定,而不是由服务器的自我描述决定。

审阅者测试计划

如何验证

在每种信任组合下构建带有 readOnlyHint: true 的 MCP 工具调用。确认不可信服务器会询问,位于不可信工作区中的可信服务器会询问,而位于可信工作区中的可信服务器仍会被允许。确认缺少注解或注解为 false 时保留现有行为,并且带注解的工具仍被分类为只读。

证据(变更前后)

场景 变更前 变更后
带有 readOnlyHint: true 的不可信服务器 allow ask
位于不可信工作区且带有 readOnlyHint: true 的可信服务器 allow ask
位于可信工作区且带有 readOnlyHint: true 的可信服务器 allow allow

修复前,聚焦回归测试在前两个场景中失败;修复后通过。完整的 MCP 工具测试文件通过 61 项测试。

测试平台

操作系统 状态
🍏 macOS N/A
🪟 Windows N/A
🐧 Linux ✅ 已测试

环境(可选)

Node.js 24;已运行聚焦及完整的 MCP 工具单元测试、仓库构建、类型检查、lint、Prettier 和针对性 ESLint。

风险与范围

  • 主要风险或权衡:仅依赖注解的 MCP 工具现在会提示确认,除非显式权限规则或可信服务器配置允许执行。
  • 未验证/范围外:MCP 注解传输、只读分类、权限规则求值和通用确认流程重构均未更改。
  • 破坏性变更/迁移说明:无需 API 迁移。依赖仅凭注解自动许可的用户可以显式信任服务器或配置权限规则。

关联问题

修复 #6917

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Thanks for the PR!

Template looks good ✓

Problem: Observed bug with clear reproduction. Issue #6917 describes a concrete scenario — MCP tool with trust: false, untrusted folder, and readOnlyHint: true returns allow instead of ask. Labeled type/bug + category/security + priority/P2. The CHANGELOG shows readOnlyHint was introduced in #1837 without distinguishing server annotations from user-configured trust, and #2642 previously had to restore the trust check — this PR closes the remaining gap.

Direction: Clearly aligned. MCP annotations are server-supplied hints; granting automatic permission based on a server's self-description, bypassing the user's trust configuration, is a genuine security concern. The MCP annotation spec itself says clients should treat annotations as untrusted from untrusted servers. This fix is the right call.

Size: 1 production line added, 6 deleted (in packages/core/src/tools/mcp-tool.ts). 48 test lines added (excluded from production count). Tiny and focused — no size concern.

Approach: Minimal and correct. Removes the 4-line block that auto-grants allow on readOnlyHint alone, keeping the trust+trustedFolder gate as the only auto-permission path. Adds regression tests covering all four trust/annotation combinations. No scope creep, no drive-by changes.

Moving on to code review. 🔍

中文说明

感谢贡献!

模板完整 ✓

问题:已观测到的 bug,有明确复现。Issue #6917 描述了具体场景——trust: false、不可信文件夹、readOnlyHint: true 的 MCP 工具返回 allow 而非 ask。已标记为 type/bug + category/security + priority/P2。CHANGELOG 显示 #1837 引入 readOnlyHint 时未区分服务器注解和用户信任配置,#2642 曾恢复信任检查——本 PR 关闭了剩余缺口。

方向:完全对齐。MCP 注解是服务器提供的提示;基于服务器自我描述授予自动许可、绕过用户信任配置,是真实的安全隐患。MCP 注解规范本身也要求客户端将不可信服务器的注解视为不可信。

规模:1 行生产代码新增,6 行删除(packages/core/src/tools/mcp-tool.ts)。48 行测试代码(不计入生产行数)。极小且聚焦,无规模顾虑。

方案:最小且正确。移除了仅凭 readOnlyHint 自动授予 allow 的 4 行代码块,保留 trust+trustedFolder 作为唯一自动许可路径。新增覆盖四种信任/注解组合的回归测试。无范围蔓延。

进入代码审查 🔍

Qwen Code · qwen3.7-max

Reviewed at 0de92431dec0f8bfbc6d1075024ee3f2834e7d47 · re-run with @qwen-code /triage

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Code review: the change is correct and minimal. The 4-line readOnlyHint auto-allow block was the exact source of the bug — removing it and updating the JSDoc is the right fix. The regression tests use it.each with a clean matrix covering all four trust/annotation combinations. No correctness issues, no security concerns, no AGENTS.md violations. Nothing to flag.

Before (installed build — main branch)

=== MCP readOnlyHint Trust Gate Test ===

  Untrusted server + readOnlyHint=true => allow          ← BUG
  Trusted server + untrusted folder + readOnlyHint=true => allow  ← BUG
  Trusted server + trusted folder + readOnlyHint=true => allow
  Untrusted server + readOnlyHint=false => ask

After (this PR)

=== MCP readOnlyHint Trust Gate Test ===

  Untrusted server + readOnlyHint=true => ask            ← FIXED
  Trusted server + untrusted folder + readOnlyHint=true => ask     ← FIXED
  Trusted server + trusted folder + readOnlyHint=true => allow
  Untrusted server + readOnlyHint=false => ask

Unit tests: 61/61 passed

 ✓ src/tools/mcp-tool.test.ts (61 tests) 255ms

 Test Files  1 passed (1)
      Tests  61 passed (61)
中文说明

代码审查:变更正确且最小。4 行 readOnlyHint 自动允许代码块正是 bug 的根源——移除它并更新 JSDoc 是正确的修复方式。回归测试使用 it.each 矩阵,覆盖全部四种信任/注解组合。无正确性问题、无安全隐患、无 AGENTS.md 违规。无需标记的问题。

变更前后对比显示:修复前,不可信服务器和不可信文件夹中的 readOnlyHint: true 均返回 allow(bug);修复后均返回 ask(正确)。可信服务器+可信文件夹组合保持 allow 不变。

单元测试:61/61 通过 ✓

Qwen Code · qwen3.7-max

Reviewed at 0de92431dec0f8bfbc6d1075024ee3f2834e7d47 · re-run with @qwen-code /triage

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Confidence: 5/5 — Clean security fix, confirmed bug, minimal change, comprehensive tests.

This is exactly the kind of PR the gate should pass without hesitation. The bug is real — I confirmed it on main: untrusted MCP servers with readOnlyHint: true bypass the confirmation prompt. The fix is a 4-line removal that closes the gap while preserving the legitimate trust+trustedFolder auto-permission path. The regression tests cover all four trust/annotation combinations and the full test file (61 tests) passes.

The PR is focused — no scope creep, no drive-by changes, no over-engineering. The PR description is clear, bilingual, and the reviewer test plan with before/after evidence is thorough. The linked issue (#6917) has proper labels and a concrete reproduction.

Shipping this. ✅

中文说明

置信度:5/5 — 干净的安全修复,已确认的 bug,最小变更,全面的测试。

这正是 gate 应该毫不犹豫地通过的 PR 类型。bug 是真实的——我在 main 分支上确认了:带有 readOnlyHint: true 的不可信 MCP 服务器会绕过确认提示。修复是移除 4 行代码,关闭了缺口的同时保留了合法的 trust+trustedFolder 自动许可路径。回归测试覆盖全部四种信任/注解组合,完整测试文件(61 项测试)全部通过。

PR 聚焦——无范围蔓延、无顺手改动、无过度工程化。PR 描述清晰、双语,审阅者测试计划包含变更前后证据。关联的 issue (#6917) 有正确的标签和具体复现。

批准合并 ✅

Qwen Code · qwen3.7-max

Reviewed at 0de92431dec0f8bfbc6d1075024ee3f2834e7d47 · re-run with @qwen-code /triage

@qwen-code-ci-bot qwen-code-ci-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM, looks ready to ship. ✅

@qwen-code-ci-bot qwen-code-ci-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Reviewed. Not reviewed: chunk 1 — no agent reported covering these; nobody read them.

— qwen3.7-max via Qwen Code /review

@wenshao

wenshao commented Jul 15, 2026

Copy link
Copy Markdown
Collaborator

✅ Local verification — built & ran real tests (merge reference)

I verified this PR locally in an isolated worktree at the PR head (0de92431d), built packages/core, and drove the real production decision path — not just the PR's own unit tests. Verdict: the fix is correct, load-bearing, and tightly scoped. Recommend merge.

What the fix does

getDefaultPermission() no longer returns allow just because a server self-declared readOnlyHint: true. Auto-approval now requires a user-controlled trust decision (trust === true and a trusted workspace). The read-only classification (Kind.Read at mcp-tool.ts:563) is untouched, so plan-mode and concurrency-safe scheduling still work.

The change

1 · After the fix — all green (real build & run)

The PR's own suite (mcp-tool.test.ts, 61 tests) passes. I also added a real end-to-end integration suite (mcp-readonly-trust.integration.test.ts, 7 tests) that drives the actual chain DiscoveredMCPTool.build()evaluatePermissionFlow()needsConfirmation() against a real Config (real isTrustedFolder(), real PermissionManager) — going beyond the PR test, which stubs the config. 68/68 pass. Prettier and ESLint on the two changed files are also clean.

After fix green

2 · Load-bearing proof (controlled experiment)

I reverted only the source fix (restored the deleted readOnlyHint → allow block, tests untouched) and re-ran:

  • The PR's own unit suite flips red on exactly the two untrusted scenarios (expected 'allow' to be 'ask'), matching the PR's Evidence table. The trusted+trusted and readOnlyHint: false cases stay green.
  • The real end-to-end flow flips red the same way: requiresConfirmation goes false for an untrusted server declaring readOnlyHint: true — i.e. the tool would run with no confirmation prompt. This reproduces the exact vulnerability from Untrusted MCP readOnlyHint skips default tool confirmation #6917, through the real permission-flow code.

Restoring the fix returns all 68 tests to green. Nothing else moves — the deleted block is the sole cause, and the fix closes exactly that path.

Load-bearing proof

Scope / mergeability notes

  • Read-only classification preserved: an untrusted server's readOnlyHint: true tool is still Kind.Read (verified at runtime), so plan mode and safe parallel scheduling are unaffected.
  • Clean rebase: mcp-tool.ts has not diverged on main since the PR base (2fb6c785d), so it applies without conflict.
  • Behavior change (expected): annotation-only MCP tools now prompt unless the server is explicitly trusted in a trusted workspace, or an explicit permission rule allows them — exactly the intended security tightening.
Environment

macOS (Darwin 24.6.0), Node 22, isolated git worktree at PR head, packages/core built, vitest run. Integration test added for verification only (not part of the PR).

🇨🇳 中文说明(点击展开)

✅ 本地验证 —— 构建并运行了真实测试(合并参考)

我在独立 worktree(检出 PR HEAD 0de92431d 中本地验证了此 PR,构建了 packages/core,并驱动了真实的生产判定路径,而不仅是 PR 自带的单元测试。结论:修复正确、可证明是必要的(load-bearing)、且范围精确。 建议合并。

修复做了什么

getDefaultPermission() 不再因为服务器自我声明 readOnlyHint: true 就返回 allow。自动许可现在要求用户可控的信任决策(trust === true 工作区可信)。只读分类mcp-tool.ts:563Kind.Read)未改动,因此计划模式和安全并发调度仍正常工作。

1 · 修复后 —— 全绿(真实构建与运行)

PR 自带测试(mcp-tool.test.ts61 项)通过。我另外新增了一个真实端到端集成测试套件mcp-readonly-trust.integration.test.ts7 项),驱动真实链路 DiscoveredMCPTool.build()evaluatePermissionFlow()needsConfirmation(),并针对真实 Config(真实的 isTrustedFolder()、真实的 PermissionManager)运行——这超出了 PR 测试(其对 config 做了打桩)。68/68 通过。 两个改动文件的 Prettier 与 ESLint 也均无问题。

2 · 可证明必要性(受控实验)

回退了源码修复(恢复被删除的 readOnlyHint → allow 代码块,测试保持不变)并重新运行:

  • PR 自带单元测试 恰好在两个「不可信」场景变红(expected 'allow' to be 'ask'),与 PR 的证据表一致。「可信+可信」及 readOnlyHint: false 场景保持绿色。
  • 真实端到端流程 同样变红:对于声明 readOnlyHint: true 的不可信服务器,requiresConfirmation 变为 false——即该工具会在没有确认提示的情况下运行。这通过真实的权限流程代码复现了 Untrusted MCP readOnlyHint skips default tool confirmation #6917 的确切漏洞。

恢复修复后,全部 68 项测试重新变绿。其他一切不变——被删除的代码块是唯一根因,修复恰好只封堵了该路径。

范围 / 可合并性说明

  • 只读分类得以保留: 不可信服务器声明 readOnlyHint: true 的工具仍为 Kind.Read(运行时已验证),因此计划模式与安全并发调度不受影响。
  • 可干净变基: 自 PR 基点(2fb6c785d)以来,mcp-tool.tsmain 上未发生分叉,因此可无冲突应用。
  • 行为变化(符合预期): 仅凭注解的 MCP 工具现在会提示确认,除非服务器在可信工作区中被显式信任、或有显式权限规则允许——这正是预期的安全收紧。

Verified locally on macOS; screenshots render real terminal output from the runs above.

@wenshao
wenshao added this pull request to the merge queue Jul 15, 2026
Merged via the queue into QwenLM:main with commit 2132a61 Jul 15, 2026
54 of 55 checks passed
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.

Untrusted MCP readOnlyHint skips default tool confirmation

3 participants