From 0e1e855cc5f356123a45cf0384a0441e14a35687 Mon Sep 17 00:00:00 2001 From: Test User Date: Sun, 28 Dec 2025 22:38:29 -0500 Subject: [PATCH] feat: enhance security measures for MCP server interactions - Restricted CORS to localhost origins to prevent remote code execution (RCE) attacks. - Updated MCP server configuration handling to enforce security warnings when adding or importing servers. - Introduced a SecurityWarningDialog to inform users about potential risks associated with server commands and configurations. - Ensured that only serverId is accepted for testing server connections, preventing arbitrary command execution. These changes improve the overall security posture of the MCP server management and usage. --- apps/server/src/index.ts | 6 +- apps/server/src/lib/sdk-options.ts | 4 +- apps/server/src/lib/settings-helpers.ts | 3 +- apps/server/src/providers/claude-provider.ts | 3 +- .../src/routes/mcp/routes/list-tools.ts | 18 ++- .../src/routes/mcp/routes/test-server.ts | 20 ++- .../components/mcp-permission-settings.tsx | 64 ++++++--- .../mcp-servers/dialogs/index.ts | 1 + .../dialogs/security-warning-dialog.tsx | 107 ++++++++++++++++ .../mcp-servers/hooks/use-mcp-servers.ts | 121 +++++++++++++++--- .../mcp-servers/mcp-servers-section.tsx | 21 +++ apps/ui/src/lib/http-api-client.ts | 29 +---- libs/types/src/settings.ts | 2 + 13 files changed, 310 insertions(+), 89 deletions(-) create mode 100644 apps/ui/src/components/views/settings-view/mcp-servers/dialogs/security-warning-dialog.tsx diff --git a/apps/server/src/index.ts b/apps/server/src/index.ts index 2910a0e9..4c8f0421 100644 --- a/apps/server/src/index.ts +++ b/apps/server/src/index.ts @@ -105,9 +105,13 @@ if (ENABLE_REQUEST_LOGGING) { }) ); } +// SECURITY: Restrict CORS to localhost UI origins to prevent drive-by attacks +// from malicious websites. MCP server endpoints can execute arbitrary commands, +// so allowing any origin would enable RCE from any website visited while Automaker runs. +const DEFAULT_CORS_ORIGINS = ['http://localhost:3007', 'http://127.0.0.1:3007']; app.use( cors({ - origin: process.env.CORS_ORIGIN || '*', + origin: process.env.CORS_ORIGIN || DEFAULT_CORS_ORIGINS, credentials: true, }) ); diff --git a/apps/server/src/lib/sdk-options.ts b/apps/server/src/lib/sdk-options.ts index 269f78ac..327ec059 100644 --- a/apps/server/src/lib/sdk-options.ts +++ b/apps/server/src/lib/sdk-options.ts @@ -158,8 +158,8 @@ interface McpPermissionOptions { */ function buildMcpOptions(config: CreateSdkOptionsConfig): McpPermissionOptions { const hasMcpServers = config.mcpServers && Object.keys(config.mcpServers).length > 0; - // Default to true - this is a deliberate design choice for ease of use with MCP servers. - // Users can disable these in settings for stricter security. + // Default to true for autonomous workflow. Security is enforced when adding servers + // via the security warning dialog that explains the risks. const mcpAutoApprove = config.mcpAutoApproveTools ?? true; const mcpUnrestricted = config.mcpUnrestrictedTools ?? true; diff --git a/apps/server/src/lib/settings-helpers.ts b/apps/server/src/lib/settings-helpers.ts index 21211b33..fee359d5 100644 --- a/apps/server/src/lib/settings-helpers.ts +++ b/apps/server/src/lib/settings-helpers.ts @@ -193,7 +193,8 @@ export async function getMCPPermissionSettings( settingsService?: SettingsService | null, logPrefix = '[SettingsHelper]' ): Promise<{ mcpAutoApproveTools: boolean; mcpUnrestrictedTools: boolean }> { - // Default values (both enabled for backwards compatibility) + // Default to true for autonomous workflow. Security is enforced when adding servers + // via the security warning dialog that explains the risks. const defaults = { mcpAutoApproveTools: true, mcpUnrestrictedTools: true }; if (!settingsService) { diff --git a/apps/server/src/providers/claude-provider.ts b/apps/server/src/providers/claude-provider.ts index 3c765304..7c978785 100644 --- a/apps/server/src/providers/claude-provider.ts +++ b/apps/server/src/providers/claude-provider.ts @@ -40,7 +40,8 @@ export class ClaudeProvider extends BaseProvider { // This logic mirrors buildMcpOptions() in sdk-options.ts but is applied here since // the provider is the final point where SDK options are constructed. const hasMcpServers = options.mcpServers && Object.keys(options.mcpServers).length > 0; - // Default to true - deliberate design choice for ease of use. Users can disable in settings. + // Default to true for autonomous workflow. Security is enforced when adding servers + // via the security warning dialog that explains the risks. const mcpAutoApprove = options.mcpAutoApproveTools ?? true; const mcpUnrestricted = options.mcpUnrestrictedTools ?? true; const defaultTools = ['Read', 'Write', 'Edit', 'Glob', 'Grep', 'Bash', 'WebSearch', 'WebFetch']; diff --git a/apps/server/src/routes/mcp/routes/list-tools.ts b/apps/server/src/routes/mcp/routes/list-tools.ts index d380d8b3..d2151804 100644 --- a/apps/server/src/routes/mcp/routes/list-tools.ts +++ b/apps/server/src/routes/mcp/routes/list-tools.ts @@ -4,21 +4,22 @@ * Lists available tools for an MCP server. * Similar to test but focused on tool discovery. * + * SECURITY: Only accepts serverId to look up saved configs. Does NOT accept + * arbitrary serverConfig to prevent drive-by command execution attacks. + * Users must explicitly save a server config through the UI before testing. + * * Request body: * { serverId: string } - Get tools by server ID from settings - * OR { serverConfig: MCPServerConfig } - Get tools with provided config * * Response: { success: boolean, tools?: MCPToolInfo[], error?: string } */ import type { Request, Response } from 'express'; import type { MCPTestService } from '../../../services/mcp-test-service.js'; -import type { MCPServerConfig } from '@automaker/types'; import { getErrorMessage, logError } from '../common.js'; interface ListToolsRequest { - serverId?: string; - serverConfig?: MCPServerConfig; + serverId: string; } /** @@ -29,18 +30,15 @@ export function createListToolsHandler(mcpTestService: MCPTestService) { try { const body = req.body as ListToolsRequest; - if (!body.serverId && !body.serverConfig) { + if (!body.serverId || typeof body.serverId !== 'string') { res.status(400).json({ success: false, - error: 'Either serverId or serverConfig is required', + error: 'serverId is required', }); return; } - // At this point, we know at least one of serverId or serverConfig is truthy - const result = body.serverId - ? await mcpTestService.testServerById(body.serverId) - : await mcpTestService.testServer(body.serverConfig!); + const result = await mcpTestService.testServerById(body.serverId); // Return only tool-related information res.json({ diff --git a/apps/server/src/routes/mcp/routes/test-server.ts b/apps/server/src/routes/mcp/routes/test-server.ts index 2240fafc..fb7581cf 100644 --- a/apps/server/src/routes/mcp/routes/test-server.ts +++ b/apps/server/src/routes/mcp/routes/test-server.ts @@ -2,23 +2,23 @@ * POST /api/mcp/test - Test MCP server connection and list tools * * Tests connection to an MCP server and returns available tools. - * Accepts either a serverId to look up config, or a full server config. + * + * SECURITY: Only accepts serverId to look up saved configs. Does NOT accept + * arbitrary serverConfig to prevent drive-by command execution attacks. + * Users must explicitly save a server config through the UI before testing. * * Request body: * { serverId: string } - Test server by ID from settings - * OR { serverConfig: MCPServerConfig } - Test with provided config * * Response: { success: boolean, tools?: MCPToolInfo[], error?: string, connectionTime?: number } */ import type { Request, Response } from 'express'; import type { MCPTestService } from '../../../services/mcp-test-service.js'; -import type { MCPServerConfig } from '@automaker/types'; import { getErrorMessage, logError } from '../common.js'; interface TestServerRequest { - serverId?: string; - serverConfig?: MCPServerConfig; + serverId: string; } /** @@ -29,19 +29,15 @@ export function createTestServerHandler(mcpTestService: MCPTestService) { try { const body = req.body as TestServerRequest; - if (!body.serverId && !body.serverConfig) { + if (!body.serverId || typeof body.serverId !== 'string') { res.status(400).json({ success: false, - error: 'Either serverId or serverConfig is required', + error: 'serverId is required', }); return; } - // At this point, we know at least one of serverId or serverConfig is truthy - const result = body.serverId - ? await mcpTestService.testServerById(body.serverId) - : await mcpTestService.testServer(body.serverConfig!); - + const result = await mcpTestService.testServerById(body.serverId); res.json(result); } catch (error) { logError(error, 'Test server failed'); diff --git a/apps/ui/src/components/views/settings-view/mcp-servers/components/mcp-permission-settings.tsx b/apps/ui/src/components/views/settings-view/mcp-servers/components/mcp-permission-settings.tsx index b7db14a4..e65e25bb 100644 --- a/apps/ui/src/components/views/settings-view/mcp-servers/components/mcp-permission-settings.tsx +++ b/apps/ui/src/components/views/settings-view/mcp-servers/components/mcp-permission-settings.tsx @@ -1,6 +1,8 @@ +import { ShieldAlert } from 'lucide-react'; import { Label } from '@/components/ui/label'; import { Switch } from '@/components/ui/switch'; import { syncSettingsToServer } from '@/hooks/use-settings-migration'; +import { cn } from '@/lib/utils'; interface MCPPermissionSettingsProps { mcpAutoApproveTools: boolean; @@ -15,18 +17,12 @@ export function MCPPermissionSettings({ onAutoApproveChange, onUnrestrictedChange, }: MCPPermissionSettingsProps) { + const hasAnyEnabled = mcpAutoApproveTools || mcpUnrestrictedTools; + return (
-
-
- -

- Allow MCP tool calls without permission prompts (recommended) -

-
+
-
-
-
-
+ +
+
+ +

+ When enabled, the AI agent can use any tool, not just the default set. +

+ {mcpUnrestrictedTools && ( +

+ + Agent has full tool access including file writes and bash +

+ )} +
+ + {hasAnyEnabled && ( +
+

Security Note

+

+ These settings reduce security restrictions for MCP tool usage. Only enable if you + trust all configured MCP servers. +

+
+ )}
); diff --git a/apps/ui/src/components/views/settings-view/mcp-servers/dialogs/index.ts b/apps/ui/src/components/views/settings-view/mcp-servers/dialogs/index.ts index e5f063d4..2be761bc 100644 --- a/apps/ui/src/components/views/settings-view/mcp-servers/dialogs/index.ts +++ b/apps/ui/src/components/views/settings-view/mcp-servers/dialogs/index.ts @@ -3,3 +3,4 @@ export { DeleteServerDialog } from './delete-server-dialog'; export { ImportJsonDialog } from './import-json-dialog'; export { JsonEditDialog } from './json-edit-dialog'; export { GlobalJsonEditDialog } from './global-json-edit-dialog'; +export { SecurityWarningDialog } from './security-warning-dialog'; diff --git a/apps/ui/src/components/views/settings-view/mcp-servers/dialogs/security-warning-dialog.tsx b/apps/ui/src/components/views/settings-view/mcp-servers/dialogs/security-warning-dialog.tsx new file mode 100644 index 00000000..62249814 --- /dev/null +++ b/apps/ui/src/components/views/settings-view/mcp-servers/dialogs/security-warning-dialog.tsx @@ -0,0 +1,107 @@ +import { ShieldAlert, Terminal, Globe } from 'lucide-react'; +import { Button } from '@/components/ui/button'; +import { + Dialog, + DialogContent, + DialogDescription, + DialogFooter, + DialogHeader, + DialogTitle, +} from '@/components/ui/dialog'; + +interface SecurityWarningDialogProps { + open: boolean; + onOpenChange: (open: boolean) => void; + onConfirm: () => void; + serverType: 'stdio' | 'sse' | 'http'; + serverName: string; + command?: string; + args?: string[]; + url?: string; + /** Number of servers being imported (for import dialog) */ + importCount?: number; +} + +export function SecurityWarningDialog({ + open, + onOpenChange, + onConfirm, + serverType, + serverName, + command, + args, + url, + importCount, +}: SecurityWarningDialogProps) { + const isImport = importCount !== undefined && importCount > 0; + const isStdio = serverType === 'stdio'; + + return ( + + + + + + Security Warning + + +
+

+ {isImport + ? `You are about to import ${importCount} MCP server${importCount > 1 ? 's' : ''}.` + : 'MCP servers can execute code on your machine.'} +

+ + {!isImport && isStdio && command && ( +
+
+ + This server will run: +
+ + {command} {args?.join(' ')} + +
+ )} + + {!isImport && !isStdio && url && ( +
+
+ + This server will connect to: +
+ {url} +
+ )} + + {isImport && ( +
+

+ Each imported server can execute arbitrary commands or connect to external + services. Review the JSON carefully before importing. +

+
+ )} + +
    +
  • Only add servers from sources you trust
    • + {isStdio &&
  • Stdio servers run with your user privileges
    • } + {!isStdio &&
  • HTTP/SSE servers can access network resources
    • } +
  • Review the {isStdio ? 'command' : 'URL'} before confirming
    • +
+
+ + + + + + + + + ); +} diff --git a/apps/ui/src/components/views/settings-view/mcp-servers/hooks/use-mcp-servers.ts b/apps/ui/src/components/views/settings-view/mcp-servers/hooks/use-mcp-servers.ts index 2e689e68..7bf62f41 100644 --- a/apps/ui/src/components/views/settings-view/mcp-servers/hooks/use-mcp-servers.ts +++ b/apps/ui/src/components/views/settings-view/mcp-servers/hooks/use-mcp-servers.ts @@ -9,6 +9,17 @@ import { defaultFormData } from '../types'; import { MAX_RECOMMENDED_TOOLS } from '../constants'; import type { ServerType } from '../types'; +/** Pending server data waiting for security confirmation */ +interface PendingServerData { + type: 'add' | 'import'; + serverData?: Omit; + importServers?: Array>; + serverType: ServerType; + command?: string; + args?: string[]; + url?: string; +} + export function useMCPServers() { const { mcpServers, @@ -37,6 +48,10 @@ export function useMCPServers() { const [globalJsonValue, setGlobalJsonValue] = useState(''); const autoTestedServersRef = useRef>(new Set()); + // Security warning dialog state + const [isSecurityWarningOpen, setIsSecurityWarningOpen] = useState(false); + const [pendingServerData, setPendingServerData] = useState(null); + // Computed values const totalToolsCount = useMemo(() => { let count = 0; @@ -140,7 +155,7 @@ export function useMCPServers() { } else { toast.error('Failed to refresh MCP servers'); } - } catch (error) { + } catch { toast.error('Error refreshing MCP servers'); } finally { setIsRefreshing(false); @@ -258,16 +273,52 @@ export function useMCPServers() { } } + // If editing an existing server, save directly (user already approved it) if (editingServer) { updateMCPServer(editingServer.id, serverData); toast.success('MCP server updated'); - } else { - addMCPServer(serverData); - toast.success('MCP server added'); + await syncSettingsToServer(); + handleCloseDialog(); + return; } - await syncSettingsToServer(); - handleCloseDialog(); + // For new servers, show security warning first + setPendingServerData({ + type: 'add', + serverData, + serverType: formData.type, + command: formData.type === 'stdio' ? formData.command.trim() : undefined, + args: + formData.type === 'stdio' && formData.args.trim() + ? formData.args.trim().split(/\s+/) + : undefined, + url: formData.type !== 'stdio' ? formData.url.trim() : undefined, + }); + setIsSecurityWarningOpen(true); + }; + + /** Called when user confirms the security warning for adding a server */ + const handleSecurityWarningConfirm = async () => { + if (!pendingServerData) return; + + if (pendingServerData.type === 'add' && pendingServerData.serverData) { + addMCPServer(pendingServerData.serverData); + toast.success('MCP server added'); + await syncSettingsToServer(); + handleCloseDialog(); + } else if (pendingServerData.type === 'import' && pendingServerData.importServers) { + for (const serverData of pendingServerData.importServers) { + addMCPServer(serverData); + } + await syncSettingsToServer(); + const count = pendingServerData.importServers.length; + toast.success(`Imported ${count} MCP server${count > 1 ? 's' : ''}`); + setIsImportDialogOpen(false); + setImportJson(''); + } + + setIsSecurityWarningOpen(false); + setPendingServerData(null); }; const handleToggleEnabled = async (server: MCPServerConfig) => { @@ -297,7 +348,7 @@ export function useMCPServers() { return; } - let addedCount = 0; + const serversToImport: Array> = []; let skippedCount = 0; for (const [name, config] of Object.entries(servers)) { @@ -323,10 +374,28 @@ export function useMCPServers() { skippedCount++; continue; } - serverData.command = serverConfig.command as string; - if (Array.isArray(serverConfig.args)) { + + const rawCommand = serverConfig.command as string; + + // Support both formats: + // 1. Separate command/args: { "command": "npx", "args": ["-y", "package"] } + // 2. Inline args (Claude Desktop format): { "command": "npx -y package" } + if (Array.isArray(serverConfig.args) && serverConfig.args.length > 0) { + // Args provided separately + serverData.command = rawCommand; serverData.args = serverConfig.args as string[]; + } else if (rawCommand.includes(' ')) { + // Parse inline command string - split on spaces but preserve quoted strings + const parts = rawCommand.match(/(?:[^\s"']+|"[^"]*"|'[^']*')+/g) || [rawCommand]; + serverData.command = parts[0]; + if (parts.length > 1) { + // Remove quotes from args + serverData.args = parts.slice(1).map((arg) => arg.replace(/^["']|["']$/g, '')); + } + } else { + serverData.command = rawCommand; } + if (typeof serverConfig.env === 'object' && serverConfig.env !== null) { serverData.env = serverConfig.env as Record; } @@ -342,26 +411,32 @@ export function useMCPServers() { } } - addMCPServer(serverData); - addedCount++; + serversToImport.push(serverData); } - await syncSettingsToServer(); - - if (addedCount > 0) { - toast.success(`Imported ${addedCount} MCP server${addedCount > 1 ? 's' : ''}`); - } if (skippedCount > 0) { toast.info( `Skipped ${skippedCount} server${skippedCount > 1 ? 's' : ''} (already exist or invalid)` ); } - if (addedCount === 0 && skippedCount === 0) { - toast.warning('No servers found in JSON'); + + if (serversToImport.length === 0) { + toast.warning('No new servers to import'); + return; } - setIsImportDialogOpen(false); - setImportJson(''); + // Show security warning before importing + // Use the first server's type for the warning (most imports are stdio) + const firstServer = serversToImport[0]; + setPendingServerData({ + type: 'import', + importServers: serversToImport, + serverType: firstServer.type || 'stdio', + command: firstServer.type === 'stdio' ? firstServer.command : undefined, + args: firstServer.type === 'stdio' ? firstServer.args : undefined, + url: firstServer.type !== 'stdio' ? firstServer.url : undefined, + }); + setIsSecurityWarningOpen(true); } catch (error) { toast.error('Invalid JSON: ' + (error instanceof Error ? error.message : 'Parse error')); } @@ -649,6 +724,11 @@ export function useMCPServers() { globalJsonValue, setGlobalJsonValue, + // Security warning dialog state + isSecurityWarningOpen, + setIsSecurityWarningOpen, + pendingServerData, + // UI state isRefreshing, serverTestStates, @@ -674,5 +754,6 @@ export function useMCPServers() { handleSaveJsonEdit, handleOpenGlobalJsonEdit, handleSaveGlobalJsonEdit, + handleSecurityWarningConfirm, }; } diff --git a/apps/ui/src/components/views/settings-view/mcp-servers/mcp-servers-section.tsx b/apps/ui/src/components/views/settings-view/mcp-servers/mcp-servers-section.tsx index d18b7ed4..0cec3af4 100644 --- a/apps/ui/src/components/views/settings-view/mcp-servers/mcp-servers-section.tsx +++ b/apps/ui/src/components/views/settings-view/mcp-servers/mcp-servers-section.tsx @@ -13,6 +13,7 @@ import { ImportJsonDialog, JsonEditDialog, GlobalJsonEditDialog, + SecurityWarningDialog, } from './dialogs'; export function MCPServersSection() { @@ -45,6 +46,11 @@ export function MCPServersSection() { globalJsonValue, setGlobalJsonValue, + // Security warning dialog state + isSecurityWarningOpen, + setIsSecurityWarningOpen, + pendingServerData, + // UI state isRefreshing, serverTestStates, @@ -70,6 +76,7 @@ export function MCPServersSection() { handleSaveJsonEdit, handleOpenGlobalJsonEdit, handleSaveGlobalJsonEdit, + handleSecurityWarningConfirm, } = useMCPServers(); return ( @@ -187,6 +194,20 @@ export function MCPServersSection() { setGlobalJsonValue(''); }} /> + +
); } diff --git a/apps/ui/src/lib/http-api-client.ts b/apps/ui/src/lib/http-api-client.ts index 2f0f437c..307a5379 100644 --- a/apps/ui/src/lib/http-api-client.ts +++ b/apps/ui/src/lib/http-api-client.ts @@ -1141,6 +1141,8 @@ export class HttpApiClient implements ElectronAPI { }; // MCP API - Test MCP server connections and list tools + // SECURITY: Only accepts serverId, not arbitrary serverConfig, to prevent + // drive-by command execution attacks. Servers must be saved first. mcp = { testServer: ( serverId: string @@ -1160,33 +1162,6 @@ export class HttpApiClient implements ElectronAPI { }; }> => this.post('/api/mcp/test', { serverId }), - testServerConfig: (serverConfig: { - id: string; - name: string; - description?: string; - type?: 'stdio' | 'sse' | 'http'; - command?: string; - args?: string[]; - env?: Record; - url?: string; - headers?: Record; - enabled?: boolean; - }): Promise<{ - success: boolean; - tools?: Array<{ - name: string; - description?: string; - inputSchema?: Record; - enabled: boolean; - }>; - error?: string; - connectionTime?: number; - serverInfo?: { - name?: string; - version?: string; - }; - }> => this.post('/api/mcp/test', { serverConfig }), - listTools: ( serverId: string ): Promise<{ diff --git a/libs/types/src/settings.ts b/libs/types/src/settings.ts index 4f594fa6..9f642dd1 100644 --- a/libs/types/src/settings.ts +++ b/libs/types/src/settings.ts @@ -520,6 +520,8 @@ export const DEFAULT_GLOBAL_SETTINGS: GlobalSettings = { autoLoadClaudeMd: false, enableSandboxMode: true, mcpServers: [], + // Default to true for autonomous workflow. Security is enforced when adding servers + // via the security warning dialog that explains the risks. mcpAutoApproveTools: true, mcpUnrestrictedTools: true, };