diff --git a/packages/cli/src/commands/mcp/list.test.ts b/packages/cli/src/commands/mcp/list.test.ts index b75556f1cf..12cd5d01fd 100644 --- a/packages/cli/src/commands/mcp/list.test.ts +++ b/packages/cli/src/commands/mcp/list.test.ts @@ -14,16 +14,17 @@ import { type Mock, } from 'vitest'; import { listMcpServers } from './list.js'; +import { loadSettings } from '../../config/settings.js'; import { - loadSettings, - mergeSettings, - type LoadedSettings, -} from '../../config/settings.js'; -import { createTransport, debugLogger } from '@google/gemini-cli-core'; + createTransport, + debugLogger, + type AdminControlsSettings, +} from '@google/gemini-cli-core'; import { Client } from '@modelcontextprotocol/sdk/client/index.js'; import { ExtensionStorage } from '../../config/extensions/storage.js'; import { ExtensionManager } from '../../config/extension-manager.js'; import { McpServerEnablementManager } from '../../config/mcp/index.js'; +import { createMockSettings } from '../../test-utils/settings.js'; vi.mock('../../config/settings.js', async (importOriginal) => { const actual = @@ -133,10 +134,7 @@ describe('mcp list command', () => { }); it('should display message when no servers configured', async () => { - const defaultMergedSettings = mergeSettings({}, {}, {}, {}, true); - mockedLoadSettings.mockReturnValue({ - merged: { ...defaultMergedSettings, mcpServers: {} }, - }); + mockedLoadSettings.mockReturnValue(createMockSettings({ mcpServers: {} })); await listMcpServers(); @@ -144,10 +142,8 @@ describe('mcp list command', () => { }); it('should display different server types with connected status', async () => { - const defaultMergedSettings = mergeSettings({}, {}, {}, {}, true); - mockedLoadSettings.mockReturnValue({ - merged: { - ...defaultMergedSettings, + mockedLoadSettings.mockReturnValue( + createMockSettings({ mcpServers: { 'stdio-server': { command: '/path/to/server', args: ['arg1'] }, 'sse-server': { url: 'https://example.com/sse', type: 'sse' }, @@ -158,9 +154,9 @@ describe('mcp list command', () => { type: 'http', }, }, - }, - isTrusted: true, - }); + isTrusted: true, + }), + ); mockClient.connect.mockResolvedValue(undefined); mockClient.ping.mockResolvedValue(undefined); @@ -196,15 +192,14 @@ describe('mcp list command', () => { }); it('should display disconnected status when connection fails', async () => { - const defaultMergedSettings = mergeSettings({}, {}, {}, {}, true); - mockedLoadSettings.mockReturnValue({ - merged: { - ...defaultMergedSettings, + mockedLoadSettings.mockReturnValue( + createMockSettings({ mcpServers: { 'test-server': { command: '/test/server' }, }, - }, - }); + isTrusted: true, + }), + ); mockClient.connect.mockRejectedValue(new Error('Connection failed')); @@ -218,16 +213,14 @@ describe('mcp list command', () => { }); it('should display connected status even if ping fails', async () => { - const defaultMergedSettings = mergeSettings({}, {}, {}, {}, true); - mockedLoadSettings.mockReturnValue({ - merged: { - ...defaultMergedSettings, + mockedLoadSettings.mockReturnValue( + createMockSettings({ mcpServers: { 'test-server': { command: '/test/server' }, }, - }, - isTrusted: true, - }); + isTrusted: true, + }), + ); mockClient.connect.mockResolvedValue(undefined); mockClient.ping.mockRejectedValue(new Error('Ping failed')); @@ -240,16 +233,14 @@ describe('mcp list command', () => { }); it('should use configured timeout for connection', async () => { - const defaultMergedSettings = mergeSettings({}, {}, {}, {}, true); - mockedLoadSettings.mockReturnValue({ - merged: { - ...defaultMergedSettings, + mockedLoadSettings.mockReturnValue( + createMockSettings({ mcpServers: { 'test-server': { command: '/test/server', timeout: 12345 }, }, - }, - isTrusted: true, - }); + isTrusted: true, + }), + ); mockClient.connect.mockResolvedValue(undefined); @@ -265,16 +256,14 @@ describe('mcp list command', () => { }); it('should use default timeout for connection when not configured', async () => { - const defaultMergedSettings = mergeSettings({}, {}, {}, {}, true); - mockedLoadSettings.mockReturnValue({ - merged: { - ...defaultMergedSettings, + mockedLoadSettings.mockReturnValue( + createMockSettings({ mcpServers: { 'test-server': { command: '/test/server' }, }, - }, - isTrusted: true, - }); + isTrusted: true, + }), + ); mockClient.connect.mockResolvedValue(undefined); @@ -290,16 +279,14 @@ describe('mcp list command', () => { }); it('should merge extension servers with config servers', async () => { - const defaultMergedSettings = mergeSettings({}, {}, {}, {}, true); - mockedLoadSettings.mockReturnValue({ - merged: { - ...defaultMergedSettings, + mockedLoadSettings.mockReturnValue( + createMockSettings({ mcpServers: { 'config-server': { command: '/config/server' }, }, - }, - isTrusted: true, - }); + isTrusted: true, + }), + ); mockExtensionManager.loadExtensions.mockReturnValue([ { @@ -326,36 +313,40 @@ describe('mcp list command', () => { }); it('should filter servers based on admin allowlist passed in settings', async () => { - const settingsWithAllowlist = mergeSettings({}, {}, {}, {}, true); - settingsWithAllowlist.admin = { - secureModeEnabled: false, - extensions: { enabled: true }, - skills: { enabled: true }, - mcp: { - enabled: true, - config: { - 'allowed-server': { url: 'http://allowed' }, + const adminControls = { + strictModeDisabled: true, + mcpSetting: { + mcpEnabled: true, + mcpConfig: { + mcpServers: { + 'allowed-server': { url: 'http://allowed' }, + }, }, - requiredConfig: {}, }, }; - settingsWithAllowlist.mcpServers = { + const mcpServers = { 'allowed-server': { command: 'cmd1' }, 'forbidden-server': { command: 'cmd2' }, }; - mockedLoadSettings.mockReturnValue({ - merged: settingsWithAllowlist, + const mockSettings = createMockSettings({ + mcpServers, + isTrusted: true, }); + // setRemoteAdminSettings is the correct way to set admin settings in tests + ( + mockSettings as unknown as { + setRemoteAdminSettings: (controls: AdminControlsSettings) => void; + } + ).setRemoteAdminSettings(adminControls as unknown as AdminControlsSettings); + + mockedLoadSettings.mockReturnValue(mockSettings); mockClient.connect.mockResolvedValue(undefined); mockClient.ping.mockResolvedValue(undefined); - await listMcpServers({ - merged: settingsWithAllowlist, - isTrusted: true, - } as unknown as LoadedSettings); + await listMcpServers(mockSettings); expect(debugLogger.log).toHaveBeenCalledWith( expect.stringContaining('allowed-server'), @@ -371,44 +362,40 @@ describe('mcp list command', () => { ); }); - it('should show stdio servers as disconnected in untrusted folders', async () => { - const defaultMergedSettings = mergeSettings({}, {}, {}, {}, true); - mockedLoadSettings.mockReturnValue({ - merged: { - ...defaultMergedSettings, + it('should show stdio servers as disabled in untrusted folders', async () => { + mockedLoadSettings.mockReturnValue( + createMockSettings({ mcpServers: { 'test-server': { command: '/test/server' }, }, - }, - isTrusted: false, - }); - - // createTransport will throw in core if not trusted - mockedCreateTransport.mockRejectedValue(new Error('Folder not trusted')); + isTrusted: false, + }), + ); await listMcpServers(); expect(debugLogger.log).toHaveBeenCalledWith( expect.stringContaining( - 'test-server: /test/server (stdio) - Disconnected', + 'Warning: MCP servers are configured but disabled because this folder is untrusted.', ), ); + expect(debugLogger.log).toHaveBeenCalledWith( + expect.stringContaining('test-server: /test/server (stdio) - Disabled'), + ); }); it('should display blocked status for servers in excluded list', async () => { - const defaultMergedSettings = mergeSettings({}, {}, {}, {}, true); - mockedLoadSettings.mockReturnValue({ - merged: { - ...defaultMergedSettings, + mockedLoadSettings.mockReturnValue( + createMockSettings({ mcp: { excluded: ['blocked-server'], }, mcpServers: { 'blocked-server': { command: '/test/server' }, }, - }, - isTrusted: true, - }); + isTrusted: true, + }), + ); await listMcpServers(); @@ -421,16 +408,14 @@ describe('mcp list command', () => { }); it('should display disabled status for servers disabled via enablement manager', async () => { - const defaultMergedSettings = mergeSettings({}, {}, {}, {}, true); - mockedLoadSettings.mockReturnValue({ - merged: { - ...defaultMergedSettings, + mockedLoadSettings.mockReturnValue( + createMockSettings({ mcpServers: { 'disabled-server': { command: '/test/server' }, }, - }, - isTrusted: true, - }); + isTrusted: true, + }), + ); vi.spyOn( McpServerEnablementManager.prototype, @@ -446,4 +431,48 @@ describe('mcp list command', () => { ); expect(mockedCreateTransport).not.toHaveBeenCalled(); }); + + it('should display warning and disabled status in untrusted folders', async () => { + const userMcpServers = { + 'user-server': { url: 'https://example.com/user' }, + }; + const workspaceMcpServers = { + 'project-server': { command: '/path/to/project/server' }, + }; + + mockedLoadSettings.mockReturnValue( + createMockSettings({ + user: { + settings: { mcpServers: userMcpServers }, + originalSettings: { mcpServers: userMcpServers }, + path: '/mock/user/settings.json', + }, + workspace: { + settings: { mcpServers: workspaceMcpServers }, + originalSettings: { mcpServers: workspaceMcpServers }, + path: '/mock/workspace/settings.json', + }, + isTrusted: false, + }), + ); + + await listMcpServers(); + + expect(debugLogger.log).toHaveBeenCalledWith( + expect.stringContaining( + 'Warning: MCP servers are configured but disabled because this folder is untrusted.', + ), + ); + expect(debugLogger.log).toHaveBeenCalledWith( + expect.stringContaining( + 'project-server: /path/to/project/server (stdio) - Disabled', + ), + ); + expect(debugLogger.log).toHaveBeenCalledWith( + expect.stringContaining( + 'user-server: https://example.com/user (http) - Disabled', + ), + ); + expect(mockedCreateTransport).not.toHaveBeenCalled(); + }); }); diff --git a/packages/cli/src/commands/mcp/list.ts b/packages/cli/src/commands/mcp/list.ts index 6e57789321..7a2a82b29b 100644 --- a/packages/cli/src/commands/mcp/list.ts +++ b/packages/cli/src/commands/mcp/list.ts @@ -179,6 +179,10 @@ async function getServerStatus( return MCPServerStatus.DISABLED; } + if (!isTrusted) { + return MCPServerStatus.DISABLED; + } + // Test all server types by attempting actual connection return testMCPConnection(serverName, server, isTrusted, activeSettings); } @@ -189,8 +193,14 @@ export async function listMcpServers( const loadedSettings = loadedSettingsArg ?? loadSettings(); const activeSettings = loadedSettings.merged; + // If the folder is untrusted, we want to show all configured servers (including + // project-scoped ones) as disabled. + const allSettings = !loadedSettings.isTrusted + ? loadedSettings.getMergedSettingsAsIfTrusted() + : activeSettings; + const { mcpServers, blockedServerNames } = - await getMcpServersFromConfig(activeSettings); + await getMcpServersFromConfig(allSettings); const serverNames = Object.keys(mcpServers); if (blockedServerNames.length > 0) { @@ -208,6 +218,15 @@ export async function listMcpServers( return; } + if (!loadedSettings.isTrusted) { + debugLogger.log( + chalk.yellow( + 'Warning: MCP servers are configured but disabled because this folder is untrusted.\n' + + 'User-level servers are also suppressed in untrusted folders to prevent accidental side-effects.\n', + ), + ); + } + debugLogger.log('Configured MCP servers:\n'); for (const serverName of serverNames) { diff --git a/packages/cli/src/config/settings.ts b/packages/cli/src/config/settings.ts index 7fba90eb83..fd064533c6 100644 --- a/packages/cli/src/config/settings.ts +++ b/packages/cli/src/config/settings.ts @@ -348,6 +348,15 @@ export class LoadedSettings { return this._merged; } + /** + * Returns a merged settings object as if the folder were trusted. + * This is useful for commands like 'mcp list' that want to show + * what's configured even if it's currently disabled for security reasons. + */ + getMergedSettingsAsIfTrusted(): MergedSettings { + return this.computeMergedSettings(true); + } + setTrusted(isTrusted: boolean): void { if (this.isTrusted === isTrusted) { return; @@ -368,13 +377,16 @@ export class LoadedSettings { }; } - private computeMergedSettings(): MergedSettings { + private computeMergedSettings(forceTrusted = false): MergedSettings { + const isTrusted = forceTrusted || this.isTrusted; + const workspace = forceTrusted ? this._workspaceFile : this.workspace; + const merged = mergeSettings( this.system.settings, this.systemDefaults.settings, this.user.settings, - this.workspace.settings, - this.isTrusted, + workspace.settings, + isTrusted, ); // Remote admin settings always take precedence and file-based admin settings