From 8eeaa293199e35ff957496e86f673fea425bad49 Mon Sep 17 00:00:00 2001 From: Bertan Ari Date: Sun, 31 May 2026 19:04:40 -0700 Subject: [PATCH 1/5] fix(mcp): enforce mode allowlist in access_mcp_resource availability check Restricted modes could still read MCP resources from disallowed servers because hasAnyMcpResources() inspected the full hub. Forward allowedMcpServers from build-tools into filterNativeToolsForMode so the resource-availability check only considers allowed servers. Addresses review comment from PR #75 (RooCodeInc/Roo-Code -> Zoo-Code-Org/Zoo-Code). --- .../__tests__/filter-tools-for-mode.spec.ts | 58 +++++++++++++++++++ .../prompts/tools/filter-tools-for-mode.ts | 24 ++++++-- src/core/task/build-tools.ts | 13 +++-- 3 files changed, 85 insertions(+), 10 deletions(-) diff --git a/src/core/prompts/tools/__tests__/filter-tools-for-mode.spec.ts b/src/core/prompts/tools/__tests__/filter-tools-for-mode.spec.ts index 0b776a2bad9..4954a545f62 100644 --- a/src/core/prompts/tools/__tests__/filter-tools-for-mode.spec.ts +++ b/src/core/prompts/tools/__tests__/filter-tools-for-mode.spec.ts @@ -89,3 +89,61 @@ describe("filterNativeToolsForMode - disabledTools", () => { expect(resultNames).not.toContain("edit") }) }) + +describe("filterNativeToolsForMode - access_mcp_resource allowlist", () => { + const nativeTools: OpenAI.Chat.ChatCompletionTool[] = [makeTool("read_file"), makeTool("access_mcp_resource")] + + // Minimal McpHub stub exposing only getServers(), which is all the resource + // availability check uses. + function makeMcpHub(servers: Array<{ name: string; resources?: unknown[] }>): any { + return { + getServers: () => servers, + } + } + + it("keeps access_mcp_resource when an allowed server has resources", () => { + const mcpHub = makeMcpHub([{ name: "allowed-server", resources: [{ uri: "res://x" }] }]) + + const result = filterNativeToolsForMode(nativeTools, "code", undefined, undefined, undefined, {}, mcpHub, [ + "allowed-server", + ]) + + const resultNames = result.map((t) => (t as any).function.name) + expect(resultNames).toContain("access_mcp_resource") + }) + + it("removes access_mcp_resource when only a disallowed server has resources", () => { + // The server with resources is NOT in the allowlist, so the restricted + // mode must not retain access_mcp_resource. + const mcpHub = makeMcpHub([ + { name: "allowed-server", resources: [] }, + { name: "blocked-server", resources: [{ uri: "res://secret" }] }, + ]) + + const result = filterNativeToolsForMode(nativeTools, "code", undefined, undefined, undefined, {}, mcpHub, [ + "allowed-server", + ]) + + const resultNames = result.map((t) => (t as any).function.name) + expect(resultNames).not.toContain("access_mcp_resource") + expect(resultNames).toContain("read_file") + }) + + it("considers all servers when no allowlist is provided (unrestricted mode)", () => { + const mcpHub = makeMcpHub([{ name: "any-server", resources: [{ uri: "res://y" }] }]) + + const result = filterNativeToolsForMode(nativeTools, "code", undefined, undefined, undefined, {}, mcpHub) + + const resultNames = result.map((t) => (t as any).function.name) + expect(resultNames).toContain("access_mcp_resource") + }) + + it("removes access_mcp_resource when the allowlist is empty", () => { + const mcpHub = makeMcpHub([{ name: "some-server", resources: [{ uri: "res://z" }] }]) + + const result = filterNativeToolsForMode(nativeTools, "code", undefined, undefined, undefined, {}, mcpHub, []) + + const resultNames = result.map((t) => (t as any).function.name) + expect(resultNames).not.toContain("access_mcp_resource") + }) +}) diff --git a/src/core/prompts/tools/filter-tools-for-mode.ts b/src/core/prompts/tools/filter-tools-for-mode.ts index fdd41e7e330..194c7447827 100644 --- a/src/core/prompts/tools/filter-tools-for-mode.ts +++ b/src/core/prompts/tools/filter-tools-for-mode.ts @@ -220,6 +220,9 @@ export function applyModelToolCustomization( * @param codeIndexManager - Code index manager for codebase_search feature check * @param settings - Additional settings for tool filtering (includes modelInfo for model-specific customization) * @param mcpHub - MCP hub for checking available resources + * @param allowedMcpServers - Optional allowlist of MCP server names for the current mode. When + * provided, the resource-availability check only considers servers in this list, so a mode that + * restricts MCP servers cannot retain `access_mcp_resource` based on resources from disallowed servers. * @returns Filtered array of tools allowed for the mode */ export function filterNativeToolsForMode( @@ -230,6 +233,7 @@ export function filterNativeToolsForMode( codeIndexManager?: CodeIndexManager, settings?: Record, mcpHub?: McpHub, + allowedMcpServers?: string[], ): OpenAI.Chat.ChatCompletionTool[] { // Get mode configuration and all tools for this mode const modeSlug = mode ?? defaultModeSlug @@ -301,8 +305,10 @@ export function filterNativeToolsForMode( } } - // Conditionally exclude access_mcp_resource if MCP is not enabled or there are no resources - if (!mcpHub || !hasAnyMcpResources(mcpHub)) { + // Conditionally exclude access_mcp_resource if MCP is not enabled or there are no resources. + // When the mode restricts MCP servers via allowedMcpServers, only resources from allowed + // servers count — otherwise a restricted mode could still read resources from disallowed servers. + if (!mcpHub || !hasAnyMcpResources(mcpHub, allowedMcpServers)) { allowedToolNames.delete("access_mcp_resource") } @@ -330,10 +336,18 @@ export function filterNativeToolsForMode( } /** - * Helper function to check if any MCP server has resources available + * Helper function to check if any MCP server has resources available. + * + * When `allowedServers` is provided, only servers whose name is in the allowlist are considered. + * This keeps the `access_mcp_resource` availability check consistent with the mode's MCP server + * allowlist so a restricted mode cannot retain the tool based on resources from disallowed servers. */ -function hasAnyMcpResources(mcpHub: McpHub): boolean { - const servers = mcpHub.getServers() +function hasAnyMcpResources(mcpHub: McpHub, allowedServers?: string[]): boolean { + let servers = mcpHub.getServers() + if (allowedServers) { + const allowSet = new Set(allowedServers) + servers = servers.filter((server) => allowSet.has(server.name)) + } return servers.some((server) => server.resources && server.resources.length > 0) } diff --git a/src/core/task/build-tools.ts b/src/core/task/build-tools.ts index 4a094c2450d..ebbdc050dc9 100644 --- a/src/core/task/build-tools.ts +++ b/src/core/task/build-tools.ts @@ -114,7 +114,13 @@ export async function buildNativeToolsArrayWithRestrictions(options: BuildToolsO supportsImages, }) - // Filter native tools based on mode restrictions. + // Resolve mode config to get allowedMcpServers for MCP server filtering. + const modeConfig = getModeBySlug(mode ?? defaultModeSlug, customModes) + const allowedMcpServers = modeConfig?.allowedMcpServers + + // Filter native tools based on mode restrictions. The allowlist is forwarded so the + // access_mcp_resource availability check only considers resources from allowed servers; + // otherwise a restricted mode could still read resources from disallowed servers. const filteredNativeTools = filterNativeToolsForMode( nativeTools, mode, @@ -123,12 +129,9 @@ export async function buildNativeToolsArrayWithRestrictions(options: BuildToolsO codeIndexManager, filterSettings, mcpHub, + allowedMcpServers, ) - // Resolve mode config to get allowedMcpServers for MCP server filtering. - const modeConfig = getModeBySlug(mode ?? defaultModeSlug, customModes) - const allowedMcpServers = modeConfig?.allowedMcpServers - // Filter MCP tools based on mode restrictions. const mcpTools = getMcpServerTools(mcpHub, allowedMcpServers) const filteredMcpTools = filterMcpToolsForMode(mcpTools, mode, customModes, experiments) From 948708ea40a8a86db597f72f6bd8fcc3d21137f3 Mon Sep 17 00:00:00 2001 From: Bertan Ari Date: Sun, 31 May 2026 19:07:35 -0700 Subject: [PATCH 2/5] fix(modes): avoid clobbering concurrent mode edits in debounced MCP allowlist flush The 150ms debounced flush in McpServerRestriction captured the customMode from the scheduling render and spread it on commit, so an edit to another field within the debounce window was overwritten by the stale snapshot. Track the latest customMode/onCommit in refs and merge allowedMcpServers into the freshest snapshot at flush time. Adds Test 4 covering concurrent-edit safety. Addresses review comment from PR #75. --- .../components/modes/McpServerRestriction.tsx | 21 ++++++-- .../__tests__/McpServerRestriction.spec.tsx | 53 +++++++++++++++++++ 2 files changed, 71 insertions(+), 3 deletions(-) diff --git a/webview-ui/src/components/modes/McpServerRestriction.tsx b/webview-ui/src/components/modes/McpServerRestriction.tsx index 94909e61d57..3c162dfa1a5 100644 --- a/webview-ui/src/components/modes/McpServerRestriction.tsx +++ b/webview-ui/src/components/modes/McpServerRestriction.tsx @@ -61,6 +61,18 @@ const McpServerRestriction: React.FC = ({ customMode, // Reseed-on-mode-switch is keyed on slug; track the last slug we saw. const lastSlugRef = useRef(customMode.slug) + // Always hold the latest `customMode` and `onCommit` so the debounced flush + // merges `allowedMcpServers` into the freshest mode snapshot instead of the + // stale one captured when the timeout was scheduled. Without this, an edit to + // another field of the same mode within the 150 ms debounce window would be + // clobbered when this flush spreads an outdated `customMode`. + const latestCustomModeRef = useRef(customMode) + const latestOnCommitRef = useRef(onCommit) + useEffect(() => { + latestCustomModeRef.current = customMode + latestOnCommitRef.current = onCommit + }) + // Reseed when the user switches to a different mode. useEffect(() => { if (lastSlugRef.current !== customMode.slug) { @@ -98,10 +110,13 @@ const McpServerRestriction: React.FC = ({ customMode, } const handle = setTimeout(() => { lastFlushedRef.current = cachedAllowedMcpServers - onCommit(customMode.slug, { - ...customMode, + // Merge into the freshest mode snapshot (via refs) so a concurrent edit to + // another field within the debounce window is not clobbered by a stale spread. + const latestCustomMode = latestCustomModeRef.current + latestOnCommitRef.current(latestCustomMode.slug, { + ...latestCustomMode, allowedMcpServers: cachedAllowedMcpServers, - source: customMode.source || "global", + source: latestCustomMode.source || "global", }) }, 150) return () => clearTimeout(handle) diff --git a/webview-ui/src/components/modes/__tests__/McpServerRestriction.spec.tsx b/webview-ui/src/components/modes/__tests__/McpServerRestriction.spec.tsx index 034fd18cab2..67efef8a22e 100644 --- a/webview-ui/src/components/modes/__tests__/McpServerRestriction.spec.tsx +++ b/webview-ui/src/components/modes/__tests__/McpServerRestriction.spec.tsx @@ -340,4 +340,57 @@ describe("McpServerRestriction subcomponent — flicker regressions", () => { vitest.useRealTimers() } }) + + /** + * Test 4 — Concurrent-edit safety: the debounced flush must not clobber a + * newer edit to another field of the same mode made within the 150 ms + * window. + * + * Repro: the user toggles a per-server checkbox (schedules the debounced + * flush), then within the debounce window another field of the same mode + * (e.g. `name`) changes — the parent re-renders the component with an + * updated `customMode`. Before the fix, the flush spread the STALE + * `customMode` captured when the timeout was scheduled, sending the old + * `name` back to the host and clobbering the newer value. + * + * After the fix, the flush merges `allowedMcpServers` into the freshest + * `customMode` (via a ref), so the committed snapshot carries the updated + * `name`. + */ + it("Test 4: debounced flush merges into the latest customMode, not the stale snapshot", () => { + vitest.useFakeTimers() + try { + const onCommit = vitest.fn() + const customMode = { ...baseCustomMode, name: "Old Name", allowedMcpServers: [] as string[] } + + const { rerender } = render( + , + ) + + // User edits the allowlist (schedules the debounced flush). + const serverCheckbox = screen + .getByTestId("mcp-server-checkbox-server-a") + .querySelector("input[type='checkbox']") as HTMLInputElement + fireEvent.click(serverCheckbox) + expect(onCommit).not.toHaveBeenCalled() // debounce hasn't fired yet + + // Within the debounce window, another field of the same mode changes. + const updatedCustomMode = { ...customMode, name: "New Name" } + rerender( + , + ) + + // Let the 150 ms debounce fire. + vitest.advanceTimersByTime(200) + + expect(onCommit).toHaveBeenCalledTimes(1) + const [, committedConfig] = onCommit.mock.calls[0] + // The newer field value must be preserved (not clobbered by the stale snapshot). + expect(committedConfig.name).toBe("New Name") + // And the user's allowlist edit must still be present. + expect(committedConfig.allowedMcpServers).toEqual(["server-a"]) + } finally { + vitest.useRealTimers() + } + }) }) From 0845b6763aa9315900e6e923bcf6b2a1d666ad3c Mon Sep 17 00:00:00 2001 From: Bertan Ari Date: Mon, 1 Jun 2026 11:04:35 -0700 Subject: [PATCH 3/5] ci: trigger checks From a24e87e25dece50446634f00321f30241003db14 Mon Sep 17 00:00:00 2001 From: Bertan Ari Date: Mon, 1 Jun 2026 11:45:10 -0700 Subject: [PATCH 4/5] fix(webview-tests): correct toolkit mock data-testid + missing exports --- .../@vscode/webview-ui-toolkit/react.tsx | 38 ++++++++++++++++++- 1 file changed, 36 insertions(+), 2 deletions(-) diff --git a/webview-ui/src/__mocks__/@vscode/webview-ui-toolkit/react.tsx b/webview-ui/src/__mocks__/@vscode/webview-ui-toolkit/react.tsx index 470e8c6e611..cc6d089a1ad 100644 --- a/webview-ui/src/__mocks__/@vscode/webview-ui-toolkit/react.tsx +++ b/webview-ui/src/__mocks__/@vscode/webview-ui-toolkit/react.tsx @@ -1,8 +1,14 @@ import React from "react" export const VSCodeCheckbox = ({ children, onChange, checked, "data-testid": dataTestId, ...props }: any) => ( -