fix: respect agent allowed_tools in MCP auto-approval check#12965
fix: respect agent allowed_tools in MCP auto-approval check#12965kangfenmao merged 4 commits intoCherryHQ:mainfrom
Conversation
When an agent has allowed_tools configured, the auto-approval check now considers those tools as pre-authorized. Previously, agent-level tool allowlists were ignored in the auto-approval flow, requiring manual approval even for tools explicitly allowed in agent settings. Changes: - Pass allowedTools through setupToolsConfig and convertMcpToolsToAiSdkTools - Add allowedTools parameter to isToolAutoApproved utility - Wire useActiveAgent hook into useMcpToolApproval to read agent config Fixes CherryHQ#12779
DeJeune
left a comment
DeJeune
left a comment
There was a problem hiding this comment.
PR #12965 Review: fix: respect agent allowed_tools in MCP auto-approval check
Summary
The PR adds agent-level allowed_tools as an auto-approval source for MCP tools. The core logic change in isToolAutoApproved is correct, but there are two significant gaps where call sites are missed, and a security consideration worth discussing.
Issues
1. Missing allowedTools at the primary call site in parameterBuilder.ts (Bug)
The most important call site — parameterBuilder.ts:133 — still calls setupToolsConfig(mcpTools) without passing allowedTools. This is the new AI SDK pipeline's entry point for tool setup. The PR only threads allowedTools through the function signatures but the actual caller in the new pipeline never provides it, meaning the fix is incomplete for the main code path.
2. Missing allowedTools in legacy middleware McpToolChunkMiddleware.ts (Bug)
McpToolChunkMiddleware.ts:442 calls isToolAutoApproved(toolResponse.tool, server) without the new allowedTools parameter. Users on the legacy client path won't get agent-level auto-approval either.
3. Missing allowedTools in UI indicator components (Cosmetic inconsistency)
ToolHeader.tsx:238 and MessageMcpTool.tsx:138 both call isToolAutoApproved(tool) without allowedTools. These render the shield-check icon indicating a tool is auto-approved. A tool that's auto-approved via allowed_tools won't show the icon, confusing users.
4. Security consideration: allowedTools check uses tool.id — verify this is tamper-proof
The new check is allowedTools?.includes(tool.id). If a malicious MCP server could craft a tool.id matching an allowed tool from a different server, it could bypass approval. Worth confirming that tool.id includes the serverId suffix making cross-server spoofing unlikely.
Minor Notes
- Tests should be added for
isToolAutoApprovedwith the newallowedToolsparameter. - The
McpTool.tsxsettings page calls don't needallowedTools— they show server-level config, which is correct.
Verdict
The core approach is sound, but the PR is incomplete — the main call site (parameterBuilder.ts) doesn't actually pass allowedTools, so the fix won't work in the new AI SDK pipeline. The legacy middleware path is also missed. Please address issues #1 and #2 before merging.
|
Issue 2 doesn’t need to be fixed; it’s already deprecated. |
Address review feedback from DeJeune on CherryHQ#12965: - Add allowedTools to FetchChatCompletionParams (BaseParams) - Thread allowedTools through ApiService (transformMessagesAndFetch → fetchChatCompletion) - Pass allowedTools to buildStreamTextParams → setupToolsConfig - Fetch agent allowed_tools in messageThunk via AgentApiClient - Drop legacy middleware changes (already deprecated per reviewer) Issue CherryHQ#1: parameterBuilder now passes allowedTools to setupToolsConfig Issue CherryHQ#2: Skipped — legacy middleware is deprecated Issue CherryHQ#3: UI indicator changes deferred to follow-up
lucamorettibuilds
left a comment
lucamorettibuilds
left a comment
There was a problem hiding this comment.
Thanks for the detailed review @DeJeune! I've pushed a fix addressing your feedback:
Issue #1 (parameterBuilder): allowedTools is now threaded from agent data all the way through: messageThunk → transformMessagesAndFetch → fetchChatCompletion → buildStreamTextParams → setupToolsConfig. The thunk fetches the agent's allowed_tools via AgentApiClient when activeAgentId is set.
Issue #2 (legacy middleware): Reverted — agreed it's deprecated and doesn't need the fix.
Issue #3 (UI indicators): Will address in a follow-up — the shield icon in ToolHeader.tsx needs store access to get allowedTools, which is a slightly different pattern.
Issue #4 (tool.id security): Good point about cross-server spoofing. The current isToolAutoApproved already scopes approval by checking the server's autoApproveAll setting alongside the tool match. But I agree a namespaced format like serverName:toolName would be more robust — happy to discuss further.
Let me know if this looks better!
EurFelux
left a comment
EurFelux
left a comment
There was a problem hiding this comment.
LGTM. I verified the allowed_tools threading from message thunk through parameter builder into MCP auto-approval checks, and the UI hook now reflects agent-level pre-approval for pending MCP calls.
4 participants
Summary
Fixes #12779 — MCP tools cannot be auto-approved when configured through agent
allowed_tools.Problem
When a user configures an agent with specific
allowed_tools, those tools still require manual approval in the MCP tool execution flow. TheisToolAutoApprovedfunction only checks server-level auto-approve settings and built-in status, ignoring the agent-level allowlist.Changes
src/renderer/src/utils/mcp-tools.ts: AddedallowedToolsparameter toisToolAutoApproved(). When a tool ID appears in the agent'sallowedToolslist, it returnstruefor auto-approval.src/renderer/src/aiCore/utils/mcp.ts: ThreadedallowedToolsparameter throughsetupToolsConfig()andconvertMcpToolsToAiSdkTools()so the execute callback can pass it to the approval check.src/renderer/src/pages/home/Messages/Tools/hooks/useMcpToolApproval.ts: WireduseActiveAgent()hook to read the current agent'sallowed_toolsconfig and pass it into the approval check.Testing
Verified locally that:
allowed_toolsare auto-approved without manual confirmation