fix: check underlying tool permissions for hub invoke/exec#13282
Merged
kangfenmao merged 1 commit intomainfrom Mar 9, 2026
Merged
fix: check underlying tool permissions for hub invoke/exec#13282kangfenmao merged 1 commit intomainfrom
kangfenmao merged 1 commit intomainfrom
Conversation
In MCP hub/auto-discovery mode, all tools were auto-approved because hubMCPServer has no disabledAutoApproveTools field. This bypassed the permission system entirely. - Add hub-specific check in isToolAutoApproved: list/inspect are auto-approved (read-only), invoke/exec require approval by default - Add Mcp_ResolveHubTool IPC to resolve hub JS tool names to their original serverId and toolName via the main process tool mapping - In execute callback, resolve hub invoke/exec underlying tool and check the original server's disabledAutoApproveTools config - Add tests for toolname, mcp-bridge resolver, and isToolAutoApproved Co-Authored-By: Claude Opus 4.6 <[email protected]> Signed-off-by: suyao <[email protected]>
EurFelux
approved these changes
Mar 7, 2026
Collaborator
EurFelux
left a comment
EurFelux
left a comment
There was a problem hiding this comment.
Review Summary
Excellent security fix! 🔒 This PR correctly addresses a real permission bypass where all hub/auto-discovery tools were unconditionally auto-approved.
What I reviewed
mcp-tools.ts: Hub-specificisToolAutoApprovedlogic —list/inspect(read-only) stay auto-approved,invoke/exec(execution) require approval. Clean and correct.mcp-bridge.ts:resolveHubToolName()— straightforward reverse-lookup from JS name →{serverId, toolName}. UsesindexOf('__')for the separator which correctly handlesserverIdwith single underscores.mcp.ts: The execute callback now resolves the underlying tool via IPC and checks its original server'sdisabledAutoApproveTools. The fail-closed design (all null/error paths → require approval) is the right security posture.- IPC layer (
IpcChannel.ts,ipc.ts,preload/index.ts): Clean additions, consistent with existing patterns. - Tests: 31 tests across 3 files with good edge case coverage (unknown tools, uninitialized mapping, collisions, built-in overrides, agent allowed_tools).
Design tradeoffs — well justified
- IPC round-trip for resolution is acceptable since it only happens for
invoke/exec(not the frequentlist/inspect). - Not embedding metadata in MCP protocol is the right call — keeps protocol clean.
Minor
- One nit on missing
beforeAllimport intoolname.test.ts(see inline comment).
LGTM — well-structured fix with solid test coverage. 👍
| let mapping: ToolNameMapping | ||
|
|
||
| beforeAll(() => { | ||
| mapping = buildToolNameMapping([ |
Collaborator
There was a problem hiding this comment.
nit: beforeAll is used here but not imported from vitest at the top of the file (line 1 only imports describe, expect, it). It works because vitest exposes lifecycle hooks as globals, but it's inconsistent with the other test files in this PR (mcp-bridge.test.ts explicitly imports beforeEach/afterEach).
-import { describe, expect, it } from 'vitest'
+import { beforeAll, describe, expect, it } from 'vitest'
Comment on lines
+101
to
+113
| (mcpTool.name === 'invoke' || mcpTool.name === 'exec') | ||
| ) { | ||
| const underlyingToolName = (params as Record<string, unknown>)?.name as string | undefined | ||
| if (underlyingToolName) { | ||
| try { | ||
| const resolved = await window.api.mcp.resolveHubTool(underlyingToolName) | ||
| if (resolved) { | ||
| const underlyingServer = store.getState().mcp.servers.find((s) => s.id === resolved.serverId) | ||
| if (underlyingServer) { | ||
| isAutoApproveEnabled = !underlyingServer.disabledAutoApproveTools?.includes(resolved.toolName) | ||
| } | ||
| } | ||
| } catch (err) { |
Collaborator
There was a problem hiding this comment.
Nice defensive pattern here — when resolveHubTool returns null or the underlying server isn't found, the code safely falls through to isAutoApproveEnabled = false, which means the tool requires manual approval. This is the correct security posture (fail-closed).
One minor thought: resolveHubToolName in mcp-bridge.ts returns null if toolNameMapping hasn't been initialized yet (unlike callMcpTool which auto-refreshes). In practice this shouldn't happen since the hub server populates the mapping on startup before any tool execution, but it's worth noting the fail-closed behavior here guarantees safety even in that edge case.
GeorgeDong32
approved these changes
Mar 7, 2026
Collaborator
There was a problem hiding this comment.
Code Review Conclusion
✅ Approval Reasons
This is a high-quality security fix PR:
| Dimension | Assessment |
|---|---|
| Security | ⭐⭐⭐⭐⭐ Critical security vulnerability correctly fixed |
| Code Quality | ⭐⭐⭐⭐⭐ Clear structure, correct logic |
| API Compatibility | ✅ Consistent with main branch |
| Test Coverage | ⭐⭐⭐⭐ 31 unit tests |
Best Practices
- Secure by Default: Defaults to no auto-approval when parsing fails
- Principle of Least Privilege:
invoke/execrequire approval,list/inspectauto-approved - Good Test Coverage: 31 test cases covering core logic
- Clear PR Description: Detailed explanation of fix rationale and trade-offs
Minor Suggestions (Optional)
// mcp-bridge.ts:51-55
// Consider adding empty string check
if (separatorIndex <= 0 || separatorIndex === toolId.length - 2) return nullDoes not affect functional correctness, can be optimized later.
Approve ✅
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
kangfenmao
added a commit
that referenced
this pull request
Mar 13, 2026
### What this PR does Before this PR: - Version is 1.7.24 After this PR: - Version is 1.7.25 - Release notes updated in `electron-builder.yml` ### Why we need it and why it was done in this way Standard release workflow: collect commits since v1.7.24, generate bilingual release notes, bump version. The following tradeoffs were made: N/A The following alternatives were considered: N/A ### Breaking changes **Action Required**: The built-in filesystem MCP server now requires manual approval for write/edit/delete operations by default. ### Special notes for your reviewer **Included commits since v1.7.24:** ✨ New Features: - feat(websearch): add Querit search provider (#13050) - feat(minapp,provider): add MiniMax Agent, IMA mini apps and MiniMax Global, Z.ai providers (#13099) - feat(provider): add agent support filter for provider list (#11932) - feat(agent): add custom environment variables to agent configuration (#13357) - feat: add GPT-5.4 support (#13293) - feat(gemini): add thought signature persistence (#13100) 🐛 Bug Fixes: - fix: secure built-in filesystem MCP root handling (#13294) - fix: check underlying tool permissions for hub invoke/exec (#13282) - fix: remove approval countdown timers and add system notifications (#13281) - fix: agent tool status not stopping on abort (#13111) - fix: resolve spawn ENOENT on Windows for Code Tools (#13405) - fix: Use default assistant for topic auto-renaming (#13387) - fix(backup): defer auto backup during streaming response (#13307) - fix(ui): fix Move To submenu overflow (#13399) - fix: render xml fenced svg blocks as svg previews (#13431) - fix: correct Gemini reasoning params (#13388) - fix: improve Qwen 3.5 reasoning model detection (#13235) - fix: upgrade xAI web search to Responses API (#12812) - fix(api-server): relax chat completion validation (#13279) - fix: install or uninstall button state issues (#13114) - fix: improve update dialog behavior (#13363) - fix: auto-convert reasoning_effort to reasoningEffort (#12831) ### Checklist - [x] PR: The PR description is expressive enough and will help future contributors - [x] Code: Write code that humans can understand and Keep it simple - [x] Refactor: You have left the code cleaner than you found it (Boy Scout Rule) - [x] Upgrade: Impact of this change on upgrade flows was considered and addressed if required - [x] Documentation: A user-guide update was considered and is present (link) or not required. - [x] Self-review: I have reviewed my own code before requesting review from others ### Release note ```release-note See release notes in electron-builder.yml ``` --------- Signed-off-by: kangfenmao <[email protected]>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this PR does
Before this PR:
serverId: 'hub', andisToolAutoApproved()always returnedtruebecause the hub server object has nodisabledAutoApproveToolsfield.After this PR:
listandinspectremain auto-approved (read-only, safe)invokeandexecrequire approval by defaultinvoke/exec, the underlying tool's server auto-approve configuration is checked via IPC, so tools that are individually auto-approved on their original server still auto-execute through the hubWhy we need it and why it was done in this way
The following tradeoffs were made:
Mcp_ResolveHubTool) to resolve hub JS tool names back to their originalserverId/toolNamepairs, since the tool name mapping only exists in the main processisToolAutoApproved()returnsfalsefor hubinvoke/execby default; the execute callback inmcp.tsperforms the IPC resolution to check the underlying tool's auto-approve settingThe following alternatives were considered:
list/inspectare safe read-only operations and should remain fastBreaking changes
None. This is a security fix — tools that previously bypassed approval will now correctly require it (unless their underlying server has auto-approve enabled).
Special notes for your reviewer
mcp-bridge.ts: AddedresolveHubToolName()to resolve JS tool names to original{serverId, toolName}IpcChannel.ts/ipc.ts/preload/index.ts: New IPC channel for the resolutionmcp-tools.ts: Hub-specific check inisToolAutoApproved—list/inspectauto-approve, others don'tmcp.ts: For hubinvoke/exec, resolves underlying tool and checks its server's auto-approve configChecklist
/gh-pr-review,gh pr diff, or GitHub UI) before requesting review from othersRelease note