Skip to content

Commit 00102b6

Browse files
roomoteroomote
committed
fix: restore MCP auto-approval behavior to approve all tools when enabled
- Changed MCP auto-approval logic from AND to OR condition - When alwaysAllowMcp is enabled, all MCP tools are auto-approved - Individual tool alwaysAllow flag can still approve specific tools - Added comprehensive tests for MCP auto-approval scenarios Fixes #9190
1 parent 69d4efc commit 00102b6

File tree

2 files changed

+218
-1
lines changed

2 files changed

+218
-1
lines changed

‎src/core/auto-approval/__tests__/index.spec.ts‎

Lines changed: 214 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,214 @@
1+
import { describe, it, expect, vi } from "vitest"
2+
import { checkAutoApproval } from "../index"
3+
import type { ExtensionState } from "../../../shared/ExtensionMessage"
4+
import type { McpServerUse } from "@roo-code/types"
5+
6+
describe("checkAutoApproval", () => {
7+
describe("MCP auto-approval", () => {
8+
it("should approve MCP tool when alwaysAllowMcp is enabled", async () => {
9+
const mcpServerUse: McpServerUse = {
10+
type: "use_mcp_tool",
11+
serverName: "test-server",
12+
toolName: "test-tool",
13+
}
14+
15+
const state: Partial<ExtensionState> = {
16+
autoApprovalEnabled: true,
17+
alwaysAllowMcp: true,
18+
mcpServers: [
19+
{
20+
name: "test-server",
21+
config: "test-config",
22+
status: "connected",
23+
tools: [
24+
{
25+
name: "test-tool",
26+
alwaysAllow: false, // Tool does NOT have alwaysAllow flag
27+
},
28+
],
29+
},
30+
] as any,
31+
}
32+
33+
const result = await checkAutoApproval({
34+
state: state as ExtensionState,
35+
ask: "use_mcp_server",
36+
text: JSON.stringify(mcpServerUse),
37+
})
38+
39+
// Should approve because alwaysAllowMcp is true, regardless of tool's alwaysAllow flag
40+
expect(result).toEqual({ decision: "approve" })
41+
})
42+
43+
it("should approve MCP tool when tool has alwaysAllow flag even if alwaysAllowMcp is false", async () => {
44+
const mcpServerUse: McpServerUse = {
45+
type: "use_mcp_tool",
46+
serverName: "test-server",
47+
toolName: "test-tool",
48+
}
49+
50+
const state: Partial<ExtensionState> = {
51+
autoApprovalEnabled: true,
52+
alwaysAllowMcp: false, // Global MCP auto-approval is disabled
53+
mcpServers: [
54+
{
55+
name: "test-server",
56+
config: "test-config",
57+
status: "connected",
58+
tools: [
59+
{
60+
name: "test-tool",
61+
alwaysAllow: true, // Tool has individual alwaysAllow flag
62+
},
63+
],
64+
},
65+
] as any,
66+
}
67+
68+
const result = await checkAutoApproval({
69+
state: state as ExtensionState,
70+
ask: "use_mcp_server",
71+
text: JSON.stringify(mcpServerUse),
72+
})
73+
74+
// Should approve because tool has alwaysAllow flag
75+
expect(result).toEqual({ decision: "approve" })
76+
})
77+
78+
it("should ask for approval when neither alwaysAllowMcp nor tool alwaysAllow is set", async () => {
79+
const mcpServerUse: McpServerUse = {
80+
type: "use_mcp_tool",
81+
serverName: "test-server",
82+
toolName: "test-tool",
83+
}
84+
85+
const state: Partial<ExtensionState> = {
86+
autoApprovalEnabled: true,
87+
alwaysAllowMcp: false,
88+
mcpServers: [
89+
{
90+
name: "test-server",
91+
config: "test-config",
92+
status: "connected",
93+
tools: [
94+
{
95+
name: "test-tool",
96+
alwaysAllow: false,
97+
},
98+
],
99+
},
100+
] as any,
101+
}
102+
103+
const result = await checkAutoApproval({
104+
state: state as ExtensionState,
105+
ask: "use_mcp_server",
106+
text: JSON.stringify(mcpServerUse),
107+
})
108+
109+
// Should ask because neither condition is met
110+
expect(result).toEqual({ decision: "ask" })
111+
})
112+
113+
it("should approve MCP resource access when alwaysAllowMcp is enabled", async () => {
114+
const mcpServerUse: McpServerUse = {
115+
type: "access_mcp_resource",
116+
serverName: "test-server",
117+
uri: "test://resource",
118+
}
119+
120+
const state: Partial<ExtensionState> = {
121+
autoApprovalEnabled: true,
122+
alwaysAllowMcp: true,
123+
}
124+
125+
const result = await checkAutoApproval({
126+
state: state as ExtensionState,
127+
ask: "use_mcp_server",
128+
text: JSON.stringify(mcpServerUse),
129+
})
130+
131+
// Should approve resource access when alwaysAllowMcp is true
132+
expect(result).toEqual({ decision: "approve" })
133+
})
134+
135+
it("should ask for MCP resource access when alwaysAllowMcp is disabled", async () => {
136+
const mcpServerUse: McpServerUse = {
137+
type: "access_mcp_resource",
138+
serverName: "test-server",
139+
uri: "test://resource",
140+
}
141+
142+
const state: Partial<ExtensionState> = {
143+
autoApprovalEnabled: true,
144+
alwaysAllowMcp: false,
145+
}
146+
147+
const result = await checkAutoApproval({
148+
state: state as ExtensionState,
149+
ask: "use_mcp_server",
150+
text: JSON.stringify(mcpServerUse),
151+
})
152+
153+
// Should ask for resource access when alwaysAllowMcp is false
154+
expect(result).toEqual({ decision: "ask" })
155+
})
156+
157+
it("should handle missing text gracefully", async () => {
158+
const state: Partial<ExtensionState> = {
159+
autoApprovalEnabled: true,
160+
alwaysAllowMcp: true,
161+
}
162+
163+
const result = await checkAutoApproval({
164+
state: state as ExtensionState,
165+
ask: "use_mcp_server",
166+
text: undefined,
167+
})
168+
169+
expect(result).toEqual({ decision: "ask" })
170+
})
171+
172+
it("should handle invalid JSON gracefully", async () => {
173+
const state: Partial<ExtensionState> = {
174+
autoApprovalEnabled: true,
175+
alwaysAllowMcp: true,
176+
}
177+
178+
const result = await checkAutoApproval({
179+
state: state as ExtensionState,
180+
ask: "use_mcp_server",
181+
text: "invalid json",
182+
})
183+
184+
expect(result).toEqual({ decision: "ask" })
185+
})
186+
})
187+
188+
describe("General auto-approval settings", () => {
189+
it("should ask when autoApprovalEnabled is false", async () => {
190+
const state: Partial<ExtensionState> = {
191+
autoApprovalEnabled: false,
192+
alwaysAllowMcp: true,
193+
}
194+
195+
const result = await checkAutoApproval({
196+
state: state as ExtensionState,
197+
ask: "use_mcp_server",
198+
text: JSON.stringify({ type: "use_mcp_tool", serverName: "test", toolName: "test" }),
199+
})
200+
201+
expect(result).toEqual({ decision: "ask" })
202+
})
203+
204+
it("should ask when state is undefined", async () => {
205+
const result = await checkAutoApproval({
206+
state: undefined,
207+
ask: "use_mcp_server",
208+
text: JSON.stringify({ type: "use_mcp_tool", serverName: "test", toolName: "test" }),
209+
})
210+
211+
expect(result).toEqual({ decision: "ask" })
212+
})
213+
})
214+
})

‎src/core/auto-approval/index.ts‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -99,7 +99,10 @@ export async function checkAutoApproval({
9999
const mcpServerUse = JSON.parse(text) as McpServerUse
100100

101101
if (mcpServerUse.type === "use_mcp_tool") {
102-
return state.alwaysAllowMcp === true && isMcpToolAlwaysAllowed(mcpServerUse, state.mcpServers)
102+
// Auto-approve if either:
103+
// 1. alwaysAllowMcp is enabled (approves all MCP tools)
104+
// 2. The specific tool has alwaysAllow flag set
105+
return state.alwaysAllowMcp === true || isMcpToolAlwaysAllowed(mcpServerUse, state.mcpServers)
103106
? { decision: "approve" }
104107
: { decision: "ask" }
105108
} else if (mcpServerUse.type === "access_mcp_resource") {

0 commit comments

Comments
 (0)