From 41c9260cae32badf4905029f2af1ddb0a9afce3f Mon Sep 17 00:00:00 2001 From: Om Patel Date: Tue, 26 May 2026 18:08:37 -0400 Subject: [PATCH] fix(core): prevent blacklist bypass in mcp list (#27377) Co-authored-by: Gal Zahavi <38544478+galz10@users.noreply.github.com> --- packages/cli/src/commands/mcp/list.test.ts | 254 ++++++++++++++++++ packages/cli/src/commands/mcp/list.ts | 14 +- packages/cli/src/config/config.ts | 17 +- .../cli/src/config/mcp/mcpServerEnablement.ts | 2 +- packages/cli/src/config/settings.test.ts | 71 +++++ packages/cli/src/config/settings.ts | 45 ++++ packages/cli/src/gemini.tsx | 2 + 7 files changed, 399 insertions(+), 6 deletions(-) diff --git a/packages/cli/src/commands/mcp/list.test.ts b/packages/cli/src/commands/mcp/list.test.ts index 12cd5d01fd..3a91531d00 100644 --- a/packages/cli/src/commands/mcp/list.test.ts +++ b/packages/cli/src/commands/mcp/list.test.ts @@ -475,4 +475,258 @@ describe('mcp list command', () => { ); expect(mockedCreateTransport).not.toHaveBeenCalled(); }); + + it('should block servers excluded by user settings even if workspace settings override/clear the excluded list', async () => { + const mockSettings = createMockSettings({ + user: { + path: '/user/settings.json', + settings: { + mcp: { + excluded: ['blocked-server'], + }, + }, + originalSettings: { + mcp: { + excluded: ['blocked-server'], + }, + }, + }, + workspace: { + path: '/workspace/settings.json', + settings: { + mcp: { + excluded: [], + }, + }, + originalSettings: { + mcp: { + excluded: [], + }, + }, + }, + mcpServers: { + 'blocked-server': { command: '/test/server' }, + }, + isTrusted: true, + merged: { + mcp: { + excluded: [], // workspace has overridden user settings! + }, + mcpServers: { + 'blocked-server': { command: '/test/server' }, + }, + }, + }); + + mockedLoadSettings.mockReturnValue(mockSettings); + + await listMcpServers(); + + expect(debugLogger.log).toHaveBeenCalledWith( + expect.stringContaining( + 'blocked-server: /test/server (stdio) - Blocked', + ), + ); + expect(mockedCreateTransport).not.toHaveBeenCalled(); + }); + + it('should block servers case-insensitively when excluded', async () => { + const mockSettings = createMockSettings({ + user: { + path: '/user/settings.json', + settings: { + mcp: { + excluded: ['BLOCKED-server'], + }, + }, + originalSettings: { + mcp: { + excluded: ['BLOCKED-server'], + }, + }, + }, + mcpServers: { + 'blocked-server': { command: '/test/server' }, + }, + isTrusted: true, + merged: { + mcpServers: { + 'blocked-server': { command: '/test/server' }, + }, + }, + }); + + mockedLoadSettings.mockReturnValue(mockSettings); + + await listMcpServers(); + + expect(debugLogger.log).toHaveBeenCalledWith( + expect.stringContaining( + 'blocked-server: /test/server (stdio) - Blocked', + ), + ); + expect(mockedCreateTransport).not.toHaveBeenCalled(); + }); + + it('should restrict allowed servers to the intersection of all defined allowlists', async () => { + const mockSettings = createMockSettings({ + user: { + path: '/user/settings.json', + settings: { + mcp: { + allowed: ['allowed-server-1', 'allowed-server-2'], + }, + }, + originalSettings: { + mcp: { + allowed: ['allowed-server-1', 'allowed-server-2'], + }, + }, + }, + workspace: { + path: '/workspace/settings.json', + settings: { + mcp: { + allowed: ['allowed-server-1', 'malicious-server'], + }, + }, + originalSettings: { + mcp: { + allowed: ['allowed-server-1', 'malicious-server'], + }, + }, + }, + mcpServers: { + 'allowed-server-1': { command: '/allowed/1' }, + 'allowed-server-2': { command: '/allowed/2' }, + 'malicious-server': { command: '/malicious' }, + }, + isTrusted: true, + merged: { + mcp: { + allowed: ['allowed-server-1', 'malicious-server'], // workspace overrode user settings! + }, + mcpServers: { + 'allowed-server-1': { command: '/allowed/1' }, + 'allowed-server-2': { command: '/allowed/2' }, + 'malicious-server': { command: '/malicious' }, + }, + }, + }); + + mockedLoadSettings.mockReturnValue(mockSettings); + mockClient.connect.mockResolvedValue(undefined); + mockClient.ping.mockResolvedValue(undefined); + + await listMcpServers(); + + // allowed-server-1 is in the intersection, so it should connect + expect(debugLogger.log).toHaveBeenCalledWith( + expect.stringContaining( + 'allowed-server-1: /allowed/1 (stdio) - Connected', + ), + ); + // allowed-server-2 and malicious-server are not in the intersection, so they should be Blocked + expect(debugLogger.log).toHaveBeenCalledWith( + expect.stringContaining( + 'allowed-server-2: /allowed/2 (stdio) - Blocked', + ), + ); + expect(debugLogger.log).toHaveBeenCalledWith( + expect.stringContaining( + 'malicious-server: /malicious (stdio) - Blocked', + ), + ); + + expect(mockedCreateTransport).toHaveBeenCalledTimes(1); + expect(mockedCreateTransport).toHaveBeenCalledWith( + 'allowed-server-1', + expect.any(Object), + false, + expect.any(Object), + ); + }); + + it('should block all servers if the intersection of user and workspace allowlists is empty (disjoint allowlists)', async () => { + const mockSettings = createMockSettings({ + user: { + path: '/user/settings.json', + settings: { + mcp: { + allowed: ['user-allowed-server'], + }, + }, + originalSettings: { + mcp: { + allowed: ['user-allowed-server'], + }, + }, + }, + workspace: { + path: '/workspace/settings.json', + settings: { + mcp: { + allowed: ['workspace-allowed-server'], + }, + }, + originalSettings: { + mcp: { + allowed: ['workspace-allowed-server'], + }, + }, + }, + mcpServers: { + 'user-allowed-server': { command: '/allowed/user' }, + 'workspace-allowed-server': { command: '/allowed/workspace' }, + }, + isTrusted: true, + merged: { + mcp: { + allowed: ['workspace-allowed-server'], // workspace override + }, + mcpServers: { + 'user-allowed-server': { command: '/allowed/user' }, + 'workspace-allowed-server': { command: '/allowed/workspace' }, + }, + }, + }); + + mockedLoadSettings.mockReturnValue(mockSettings); + + await listMcpServers(); + + // Since the intersection is empty ([]), both servers should be Blocked! + expect(debugLogger.log).toHaveBeenCalledWith( + expect.stringContaining( + 'user-allowed-server: /allowed/user (stdio) - Blocked', + ), + ); + expect(debugLogger.log).toHaveBeenCalledWith( + expect.stringContaining( + 'workspace-allowed-server: /allowed/workspace (stdio) - Blocked', + ), + ); + expect(mockedCreateTransport).not.toHaveBeenCalled(); + }); + + it('should block all servers if allowlist is configured as empty array []', async () => { + const mockSettings = createMockSettings({ + mcp: { + allowed: [], // empty allowlist configured! + }, + mcpServers: { + 'test-server': { command: '/test/server' }, + }, + isTrusted: true, + }); + + mockedLoadSettings.mockReturnValue(mockSettings); + + await listMcpServers(); + + expect(debugLogger.log).toHaveBeenCalledWith( + expect.stringContaining('test-server: /test/server (stdio) - Blocked'), + ); + expect(mockedCreateTransport).not.toHaveBeenCalled(); + }); }); diff --git a/packages/cli/src/commands/mcp/list.ts b/packages/cli/src/commands/mcp/list.ts index 7a2a82b29b..0282277557 100644 --- a/packages/cli/src/commands/mcp/list.ts +++ b/packages/cli/src/commands/mcp/list.ts @@ -159,12 +159,16 @@ async function getServerStatus( server: MCPServerConfig, isTrusted: boolean, activeSettings: MergedSettings, + consolidatedExcluded: string[], + consolidatedAllowed: string[] | undefined, ): Promise { const mcpEnablementManager = McpServerEnablementManager.getInstance(); + const loadResult = await canLoadServer(serverName, { adminMcpEnabled: activeSettings.admin?.mcp?.enabled ?? true, - allowedList: activeSettings.mcp?.allowed, - excludedList: activeSettings.mcp?.excluded, + allowedList: consolidatedAllowed, + excludedList: + consolidatedExcluded.length > 0 ? consolidatedExcluded : undefined, enablement: mcpEnablementManager.getEnablementCallbacks(), }); @@ -227,6 +231,10 @@ export async function listMcpServers( ); } + const consolidatedExcluded = + loadedSettings.getConsolidatedExcludedMcpServers(); + const consolidatedAllowed = loadedSettings.getConsolidatedAllowedMcpServers(); + debugLogger.log('Configured MCP servers:\n'); for (const serverName of serverNames) { @@ -237,6 +245,8 @@ export async function listMcpServers( server, loadedSettings.isTrusted, activeSettings, + consolidatedExcluded, + consolidatedAllowed, ); let statusIndicator = ''; diff --git a/packages/cli/src/config/config.ts b/packages/cli/src/config/config.ts index 9feeedd894..139ab5d0f1 100755 --- a/packages/cli/src/config/config.ts +++ b/packages/cli/src/config/config.ts @@ -576,6 +576,7 @@ export interface LoadCliConfigOptions { }; worktreeSettings?: WorktreeSettings; skipExtensions?: boolean; + loadedSettings?: LoadedSettings; } export async function loadCliConfig( @@ -584,7 +585,12 @@ export async function loadCliConfig( argv: CliArgs, options: LoadCliConfigOptions = {}, ): Promise { - const { cwd = process.cwd(), projectHooks, skipExtensions = false } = options; + const { + cwd = process.cwd(), + projectHooks, + skipExtensions = false, + loadedSettings, + } = options; const debugMode = isDebugMode(argv); const worktreeSettings = @@ -985,12 +991,17 @@ export async function loadCliConfig( agents: settings.agents, adminSkillsEnabled, allowedMcpServers: mcpEnabled - ? (argv.allowedMcpServerNames ?? settings.mcp?.allowed) + ? (argv.allowedMcpServerNames ?? + (loadedSettings + ? loadedSettings.getConsolidatedAllowedMcpServers() + : settings.mcp?.allowed)) : undefined, blockedMcpServers: mcpEnabled ? argv.allowedMcpServerNames ? undefined - : settings.mcp?.excluded + : loadedSettings + ? loadedSettings.getConsolidatedExcludedMcpServers() + : settings.mcp?.excluded : undefined, blockedEnvironmentVariables: settings.security?.environmentVariableRedaction?.blocked, diff --git a/packages/cli/src/config/mcp/mcpServerEnablement.ts b/packages/cli/src/config/mcp/mcpServerEnablement.ts index 1a6c445604..dd6f2df2ae 100644 --- a/packages/cli/src/config/mcp/mcpServerEnablement.ts +++ b/packages/cli/src/config/mcp/mcpServerEnablement.ts @@ -119,7 +119,7 @@ export async function canLoadServer( } // 2. Allowlist check - if (config.allowedList && config.allowedList.length > 0) { + if (config.allowedList !== undefined) { const { found, deprecationWarning } = isInSettingsList( normalizedId, config.allowedList, diff --git a/packages/cli/src/config/settings.test.ts b/packages/cli/src/config/settings.test.ts index 146383b923..6e9bcee999 100644 --- a/packages/cli/src/config/settings.test.ts +++ b/packages/cli/src/config/settings.test.ts @@ -1109,6 +1109,77 @@ describe('Settings Loading and Merging', () => { }); }); + describe('LoadedSettings MCP consolidation', () => { + it('should consolidate mcp excluded list across all scopes', () => { + const loaded = new LoadedSettings( + { + path: '', + settings: { mcp: { excluded: ['system-excluded'] } }, + originalSettings: {}, + }, + { + path: '', + settings: { mcp: { excluded: ['defaults-excluded'] } }, + originalSettings: {}, + }, + { + path: '', + settings: { mcp: { excluded: ['user-excluded'] } }, + originalSettings: {}, + }, + { + path: '', + settings: { mcp: { excluded: ['workspace-excluded'] } }, + originalSettings: {}, + }, + true, + ); + + expect(loaded.getConsolidatedExcludedMcpServers()).toEqual([ + 'system-excluded', + 'defaults-excluded', + 'user-excluded', + 'workspace-excluded', + ]); + }); + + it('should consolidate allowed mcp list via case-insensitive intersection', () => { + const loaded = new LoadedSettings( + { + path: '', + settings: { mcp: { allowed: ['Server-A', 'Server-B'] } }, + originalSettings: {}, + }, + { + path: '', + settings: { mcp: { allowed: ['server-a', 'Server-C'] } }, + originalSettings: {}, + }, + { path: '', settings: {}, originalSettings: {} }, // no allowlist in user + { + path: '', + settings: { mcp: { allowed: ['SERVER-A', 'Server-D'] } }, + originalSettings: {}, + }, + true, + ); + + expect(loaded.getConsolidatedAllowedMcpServers()).toEqual(['Server-A']); + }); + + it('should return undefined allowed list if no scopes define one', () => { + const loaded = new LoadedSettings( + { path: '', settings: {}, originalSettings: {} }, + { path: '', settings: {}, originalSettings: {} }, + { path: '', settings: {}, originalSettings: {} }, + { path: '', settings: {}, originalSettings: {} }, + true, + ); + + expect(loaded.getConsolidatedAllowedMcpServers()).toBeUndefined(); + }); + }); + describe('compressionThreshold settings', () => { it.each([ { diff --git a/packages/cli/src/config/settings.ts b/packages/cli/src/config/settings.ts index fd064533c6..3fd63ccc28 100644 --- a/packages/cli/src/config/settings.ts +++ b/packages/cli/src/config/settings.ts @@ -509,6 +509,51 @@ export class LoadedSettings { this._remoteAdminSettings = { admin }; this._merged = this.computeMergedSettings(); } + + /** + * Returns a consolidated list of excluded MCP servers across all settings files. + */ + getConsolidatedExcludedMcpServers(): string[] { + const scopes = [ + this.system, + this.systemDefaults, + this.user, + this.workspace, + ]; + return scopes.flatMap((scope) => { + const excluded = scope?.settings?.mcp?.excluded; + return Array.isArray(excluded) ? excluded : []; + }); + } + + /** + * Returns a consolidated list of allowed MCP servers (via intersection of all defined lists). + */ + getConsolidatedAllowedMcpServers(): string[] | undefined { + const scopes = [ + this.system, + this.systemDefaults, + this.user, + this.workspace, + ]; + const definedAllowlists = scopes.flatMap((scope) => { + const allowed = scope?.settings?.mcp?.allowed; + return Array.isArray(allowed) ? [allowed] : []; + }); + + if (definedAllowlists.length === 0) { + return undefined; + } + + return definedAllowlists.reduce((acc, current) => { + const normalizedCurrent = new Set( + current.map((item) => item.toLowerCase().trim()), + ); + return acc.filter((item) => + normalizedCurrent.has(item.toLowerCase().trim()), + ); + }); + } } function findEnvFile( diff --git a/packages/cli/src/gemini.tsx b/packages/cli/src/gemini.tsx index 2c76df95f9..fb76a88068 100644 --- a/packages/cli/src/gemini.tsx +++ b/packages/cli/src/gemini.tsx @@ -499,6 +499,7 @@ export async function main() { const partialConfig = await loadCliConfig(settings.merged, sessionId, argv, { projectHooks: settings.workspace.settings.hooks, skipExtensions: true, + loadedSettings: settings, }); adminControlsListner.setConfig(partialConfig); @@ -627,6 +628,7 @@ export async function main() { config = await loadCliConfig(settings.merged, sessionId, argv, { projectHooks: settings.workspace.settings.hooks, worktreeSettings: worktreeInfo, + loadedSettings: settings, }); loadConfigHandle?.end();