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.
2026-01-31 20:03:37 +00:00 · 2025-12-28 22:38:29 -05:00
parent 3c719f05a1
commit 0e1e855cc5
13 changed files with 310 additions and 89 deletions
--- 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 (
    <div className="px-6 py-4 border-b border-border/50 bg-muted/20">
      <div className="space-y-4">
-        <div className="flex items-center justify-between">
-          <div className="space-y-0.5">
-            <Label htmlFor="mcp-auto-approve" className="text-sm font-medium">
-              Auto-approve MCP tools
-            </Label>
-            <p className="text-xs text-muted-foreground">
-              Allow MCP tool calls without permission prompts (recommended)
-            </p>
-          </div>
+        <div className="flex items-start gap-3">
          <Switch
            id="mcp-auto-approve"
            checked={mcpAutoApproveTools}
@@ -35,17 +31,25 @@ export function MCPPermissionSettings({
              await syncSettingsToServer();
            }}
            data-testid="mcp-auto-approve-toggle"
+            className="mt-0.5"
          />
-        </div>
-        <div className="flex items-center justify-between">
-          <div className="space-y-0.5">
-            <Label htmlFor="mcp-unrestricted" className="text-sm font-medium">
-              Unrestricted tools
+          <div className="space-y-1 flex-1">
+            <Label htmlFor="mcp-auto-approve" className="text-sm font-medium cursor-pointer">
+              Auto-approve MCP tool calls
            </Label>
            <p className="text-xs text-muted-foreground">
-              Allow all tools when MCP is enabled (don't filter to default set)
+              When enabled, the AI agent can use MCP tools without permission prompts.
            </p>
+            {mcpAutoApproveTools && (
+              <p className="text-xs text-amber-600 flex items-center gap-1 mt-1">
+                <ShieldAlert className="h-3 w-3" />
+                Bypasses normal permission checks
+              </p>
+            )}
          </div>
+        </div>
+
+        <div className="flex items-start gap-3">
          <Switch
            id="mcp-unrestricted"
            checked={mcpUnrestrictedTools}
@@ -54,8 +58,38 @@ export function MCPPermissionSettings({
              await syncSettingsToServer();
            }}
            data-testid="mcp-unrestricted-toggle"
+            className="mt-0.5"
          />
+          <div className="space-y-1 flex-1">
+            <Label htmlFor="mcp-unrestricted" className="text-sm font-medium cursor-pointer">
+              Unrestricted tool access
+            </Label>
+            <p className="text-xs text-muted-foreground">
+              When enabled, the AI agent can use any tool, not just the default set.
+            </p>
+            {mcpUnrestrictedTools && (
+              <p className="text-xs text-amber-600 flex items-center gap-1 mt-1">
+                <ShieldAlert className="h-3 w-3" />
+                Agent has full tool access including file writes and bash
+              </p>
+            )}
+          </div>
        </div>
+
+        {hasAnyEnabled && (
+          <div
+            className={cn(
+              'rounded-md border border-amber-500/30 bg-amber-500/10 p-3 mt-2',
+              'text-xs text-amber-700 dark:text-amber-400'
+            )}
+          >
+            <p className="font-medium mb-1">Security Note</p>
+            <p>
+              These settings reduce security restrictions for MCP tool usage. Only enable if you
+              trust all configured MCP servers.
+            </p>
+          </div>
+        )}
      </div>
    </div>
  );
--- 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';
--- 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
@@ -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 (
+    <Dialog open={open} onOpenChange={onOpenChange}>
+      <DialogContent className="max-w-lg" data-testid="mcp-security-warning-dialog">
+        <DialogHeader>
+          <DialogTitle className="flex items-center gap-2">
+            <ShieldAlert className="h-5 w-5 text-amber-500" />
+            Security Warning
+          </DialogTitle>
+          <DialogDescription asChild>
+            <div className="space-y-3 pt-2">
+              <p className="font-medium text-foreground">
+                {isImport
+                  ? `You are about to import ${importCount} MCP server${importCount > 1 ? 's' : ''}.`
+                  : 'MCP servers can execute code on your machine.'}
+              </p>
+
+              {!isImport && isStdio && command && (
+                <div className="rounded-md border border-destructive/30 bg-destructive/10 p-3">
+                  <div className="flex items-center gap-2 text-sm font-medium text-foreground">
+                    <Terminal className="h-4 w-4 text-destructive" />
+                    This server will run:
+                  </div>
+                  <code className="mt-1 block break-all text-sm text-muted-foreground">
+                    {command} {args?.join(' ')}
+                  </code>
+                </div>
+              )}
+
+              {!isImport && !isStdio && url && (
+                <div className="rounded-md border border-amber-500/30 bg-amber-500/10 p-3">
+                  <div className="flex items-center gap-2 text-sm font-medium text-foreground">
+                    <Globe className="h-4 w-4 text-amber-500" />
+                    This server will connect to:
+                  </div>
+                  <code className="mt-1 block break-all text-sm text-muted-foreground">{url}</code>
+                </div>
+              )}
+
+              {isImport && (
+                <div className="rounded-md border border-amber-500/30 bg-amber-500/10 p-3">
+                  <p className="text-sm text-foreground">
+                    Each imported server can execute arbitrary commands or connect to external
+                    services. Review the JSON carefully before importing.
+                  </p>
+                </div>
+              )}
+
+              <ul className="list-inside list-disc space-y-1 text-sm text-muted-foreground">
+                <li>Only add servers from sources you trust</li>
+                {isStdio && <li>Stdio servers run with your user privileges</li>}
+                {!isStdio && <li>HTTP/SSE servers can access network resources</li>}
+                <li>Review the {isStdio ? 'command' : 'URL'} before confirming</li>
+              </ul>
+            </div>
+          </DialogDescription>
+        </DialogHeader>
+        <DialogFooter>
+          <Button variant="outline" onClick={() => onOpenChange(false)}>
+            Cancel
+          </Button>
+          <Button onClick={onConfirm} data-testid="mcp-security-confirm-button">
+            I understand, {isImport ? 'import' : 'add'} server
+            {isImport && importCount! > 1 ? 's' : ''}
+          </Button>
+        </DialogFooter>
+      </DialogContent>
+    </Dialog>
+  );
+}
--- 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<MCPServerConfig, 'id'>;
+  importServers?: Array<Omit<MCPServerConfig, 'id'>>;
+  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<Set<string>>(new Set());

+  // Security warning dialog state
+  const [isSecurityWarningOpen, setIsSecurityWarningOpen] = useState(false);
+  const [pendingServerData, setPendingServerData] = useState<PendingServerData | null>(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<Omit<MCPServerConfig, 'id'>> = [];
      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<string, string>;
          }
@@ -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,
  };
 }
--- 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('');
        }}
      />
+
+      <SecurityWarningDialog
+        open={isSecurityWarningOpen}
+        onOpenChange={setIsSecurityWarningOpen}
+        onConfirm={handleSecurityWarningConfirm}
+        serverType={pendingServerData?.serverType || 'stdio'}
+        serverName={pendingServerData?.serverData?.name || ''}
+        command={pendingServerData?.command}
+        args={pendingServerData?.args}
+        url={pendingServerData?.url}
+        importCount={
+          pendingServerData?.type === 'import' ? pendingServerData.importServers?.length : undefined
+        }
+      />
    </div>
  );
 }