🐛 fix(mcp): check system npx/uvx before falling back to bundled binaries (#12018)

src/main/services/MCPService.ts

@@ -6,7 +6,7 @@ import { loggerService } from '@logger'
 import { createInMemoryMCPServer } from '@main/mcpServers/factory'
 import { makeSureDirExists, removeEnvProxy } from '@main/utils'
 import { buildFunctionCallToolName } from '@main/utils/mcp'
-import { getBinaryName, getBinaryPath } from '@main/utils/process'
+import { findCommandInShellEnv, getBinaryName, getBinaryPath, isBinaryExists } from '@main/utils/process'
 import getLoginShellEnvironment from '@main/utils/shell-env'
 import { TraceMethod, withSpanFunc } from '@mcp-trace/trace-core'
 import { Client } from '@modelcontextprotocol/sdk/client/index.js'
@@ -318,6 +318,10 @@ class McpService {
           } else if (server.command) {
             let cmd = server.command
 
+            // Get login shell environment first - needed for command detection and server execution
+            // Note: getLoginShellEnvironment() is memoized, so subsequent calls are fast
+            const loginShellEnv = await getLoginShellEnvironment()
+
             // For DXT servers, use resolved configuration with platform overrides and variable substitution
             if (server.dxtPath) {
               const resolvedConfig = this.dxtService.getResolvedMcpConfig(server.dxtPath)
@@ -339,18 +343,45 @@ class McpService {
             }
 
             if (server.command === 'npx') {
-              cmd = await getBinaryPath('bun')
-              getServerLogger(server).debug(`Using command`, { command: cmd })
+              // First, check if npx is available in user's shell environment
+              const npxPath = await findCommandInShellEnv('npx', loginShellEnv)
 
-              // add -x to args if args exist
-              if (args && args.length > 0) {
-                if (!args.includes('-y')) {
-                  args.unshift('-y')
-                }
-                if (!args.includes('x')) {
-                  args.unshift('x')
+              if (npxPath) {
+                // Use system npx
+                cmd = npxPath
+                getServerLogger(server).debug(`Using system npx`, { command: cmd })
+              } else {
+                // System npx not found, try bundled bun as fallback
+                getServerLogger(server).debug(`System npx not found, checking for bundled bun`)
+
+                if (await isBinaryExists('bun')) {
+                  // Fall back to bundled bun
+                  cmd = await getBinaryPath('bun')
+                  getServerLogger(server).info(`Using bundled bun as fallback (npx not found in PATH)`, {
+                    command: cmd
+                  })
+
+                  // Transform args for bun x format
+                  if (args && args.length > 0) {
+                    if (!args.includes('-y')) {
+                      args.unshift('-y')
+                    }
+                    if (!args.includes('x')) {
+                      args.unshift('x')
+                    }
+                  }
+                } else {
+                  // Neither npx nor bun available
+                  throw new Error(
+                    'npx not found in PATH and bundled bun is not available. This may indicate an installation issue.\n' +
+                      'Please either:\n' +
+                      '1. Install Node.js (which includes npx) from https://nodejs.org\n' +
+                      '2. Run the MCP dependencies installer from Settings\n' +
+                      '3. Restart the application if you recently installed Node.js'
+                  )
                 }
               }
+
               if (server.registryUrl) {
                 server.env = {
                   ...server.env,
@@ -365,7 +396,35 @@ class McpService {
                 }
               }
             } else if (server.command === 'uvx' || server.command === 'uv') {
-              cmd = await getBinaryPath(server.command)
+              // First, check if uvx/uv is available in user's shell environment
+              const uvPath = await findCommandInShellEnv(server.command, loginShellEnv)
+
+              if (uvPath) {
+                // Use system uvx/uv
+                cmd = uvPath
+                getServerLogger(server).debug(`Using system ${server.command}`, { command: cmd })
+              } else {
+                // System command not found, try bundled version as fallback
+                getServerLogger(server).debug(`System ${server.command} not found, checking for bundled version`)
+
+                if (await isBinaryExists(server.command)) {
+                  // Fall back to bundled version
+                  cmd = await getBinaryPath(server.command)
+                  getServerLogger(server).info(`Using bundled ${server.command} as fallback (not found in PATH)`, {
+                    command: cmd
+                  })
+                } else {
+                  // Neither system nor bundled available
+                  throw new Error(
+                    `${server.command} not found in PATH and bundled version is not available. This may indicate an installation issue.\n` +
+                      'Please either:\n' +
+                      '1. Install uv from https://github.com/astral-sh/uv\n' +
+                      '2. Run the MCP dependencies installer from Settings\n' +
+                      `3. Restart the application if you recently installed ${server.command}`
+                  )
+                }
+              }
+
               if (server.registryUrl) {
                 server.env = {
                   ...server.env,
@@ -376,8 +435,6 @@ class McpService {
             }
 
             getServerLogger(server).debug(`Starting server`, { command: cmd, args })
-            // Logger.info(`[MCP] Environment variables for server:`, server.env)
-            const loginShellEnv = await getLoginShellEnvironment()
 
             // Bun not support proxy https://github.com/oven-sh/bun/issues/16812
             if (cmd.includes('bun')) {

src/main/utils/__tests__/process.test.ts

@@ -1,10 +1,17 @@
 import { configManager } from '@main/services/ConfigManager'
-import { execFileSync } from 'child_process'
+import { execFileSync, spawn } from 'child_process'
+import { EventEmitter } from 'events'
 import fs from 'fs'
 import path from 'path'
 import { beforeEach, describe, expect, it, vi } from 'vitest'
 
-import { autoDiscoverGitBash, findExecutable, findGitBash, validateGitBashPath } from '../process'
+import {
+  autoDiscoverGitBash,
+  findCommandInShellEnv,
+  findExecutable,
+  findGitBash,
+  validateGitBashPath
+} from '../process'
 
 // Mock configManager
 vi.mock('@main/services/ConfigManager', () => ({
@@ -988,3 +995,244 @@ describe.skipIf(process.platform !== 'win32')('process utilities', () => {
     })
   })
 })
+
+/**
+ * Helper to create a mock child process for spawn
+ */
+function createMockChildProcess() {
+  const mockChild = new EventEmitter() as EventEmitter & {
+    stdout: EventEmitter
+    stderr: EventEmitter
+    kill: ReturnType<typeof vi.fn>
+  }
+  mockChild.stdout = new EventEmitter()
+  mockChild.stderr = new EventEmitter()
+  mockChild.kill = vi.fn()
+  return mockChild
+}
+
+describe('findCommandInShellEnv', () => {
+  beforeEach(() => {
+    vi.clearAllMocks()
+    // Reset path.isAbsolute to real implementation for these tests
+    vi.mocked(path.isAbsolute).mockImplementation((p) => p.startsWith('/') || /^[A-Z]:/i.test(p))
+  })
+
+  describe('command name validation', () => {
+    it('should reject empty command name', async () => {
+      const result = await findCommandInShellEnv('', {})
+      expect(result).toBeNull()
+      expect(spawn).not.toHaveBeenCalled()
+    })
+
+    it('should reject command names with shell metacharacters', async () => {
+      const maliciousCommands = [
+        'npx; rm -rf /',
+        'npx && malicious',
+        'npx | cat /etc/passwd',
+        'npx`whoami`',
+        '$(whoami)',
+        'npx\nmalicious'
+      ]
+
+      for (const cmd of maliciousCommands) {
+        const result = await findCommandInShellEnv(cmd, {})
+        expect(result).toBeNull()
+        expect(spawn).not.toHaveBeenCalled()
+      }
+    })
+
+    it('should reject command names starting with hyphen', async () => {
+      const result = await findCommandInShellEnv('-npx', {})
+      expect(result).toBeNull()
+      expect(spawn).not.toHaveBeenCalled()
+    })
+
+    it('should reject path traversal attempts', async () => {
+      const pathTraversalCommands = ['../npx', '../../malicious', 'foo/bar', 'foo\\bar']
+
+      for (const cmd of pathTraversalCommands) {
+        const result = await findCommandInShellEnv(cmd, {})
+        expect(result).toBeNull()
+        expect(spawn).not.toHaveBeenCalled()
+      }
+    })
+
+    it('should reject command names exceeding max length', async () => {
+      const longCommand = 'a'.repeat(129)
+      const result = await findCommandInShellEnv(longCommand, {})
+      expect(result).toBeNull()
+      expect(spawn).not.toHaveBeenCalled()
+    })
+
+    it('should accept valid command names', async () => {
+      const mockChild = createMockChildProcess()
+      vi.mocked(spawn).mockReturnValue(mockChild as never)
+
+      // Don't await - just start the call
+      const resultPromise = findCommandInShellEnv('npx', { PATH: '/usr/bin' })
+
+      // Simulate command not found
+      mockChild.emit('close', 1)
+
+      const result = await resultPromise
+      expect(result).toBeNull()
+      expect(spawn).toHaveBeenCalled()
+    })
+
+    it('should accept command names with underscores and hyphens', async () => {
+      const mockChild = createMockChildProcess()
+      vi.mocked(spawn).mockReturnValue(mockChild as never)
+
+      const resultPromise = findCommandInShellEnv('my_command-name', { PATH: '/usr/bin' })
+      mockChild.emit('close', 1)
+
+      await resultPromise
+      expect(spawn).toHaveBeenCalled()
+    })
+
+    it('should accept command names at max length (128 chars)', async () => {
+      const mockChild = createMockChildProcess()
+      vi.mocked(spawn).mockReturnValue(mockChild as never)
+
+      const maxLengthCommand = 'a'.repeat(128)
+      const resultPromise = findCommandInShellEnv(maxLengthCommand, { PATH: '/usr/bin' })
+      mockChild.emit('close', 1)
+
+      await resultPromise
+      expect(spawn).toHaveBeenCalled()
+    })
+  })
+
+  describe.skipIf(process.platform === 'win32')('Unix/macOS behavior', () => {
+    it('should find command and return absolute path', async () => {
+      const mockChild = createMockChildProcess()
+      vi.mocked(spawn).mockReturnValue(mockChild as never)
+
+      const resultPromise = findCommandInShellEnv('npx', { PATH: '/usr/bin' })
+
+      // Simulate successful command -v output
+      mockChild.stdout.emit('data', '/usr/local/bin/npx\n')
+      mockChild.emit('close', 0)
+
+      const result = await resultPromise
+      expect(result).toBe('/usr/local/bin/npx')
+      expect(spawn).toHaveBeenCalledWith('/bin/sh', ['-c', 'command -v "$1"', '--', 'npx'], expect.any(Object))
+    })
+
+    it('should return null for non-absolute paths (aliases/builtins)', async () => {
+      const mockChild = createMockChildProcess()
+      vi.mocked(spawn).mockReturnValue(mockChild as never)
+
+      const resultPromise = findCommandInShellEnv('cd', { PATH: '/usr/bin' })
+
+      // Simulate builtin output (just command name)
+      mockChild.stdout.emit('data', 'cd\n')
+      mockChild.emit('close', 0)
+
+      const result = await resultPromise
+      expect(result).toBeNull()
+    })
+
+    it('should return null when command not found', async () => {
+      const mockChild = createMockChildProcess()
+      vi.mocked(spawn).mockReturnValue(mockChild as never)
+
+      const resultPromise = findCommandInShellEnv('nonexistent', { PATH: '/usr/bin' })
+
+      // Simulate command not found (exit code 1)
+      mockChild.emit('close', 1)
+
+      const result = await resultPromise
+      expect(result).toBeNull()
+    })
+
+    it('should handle spawn errors gracefully', async () => {
+      const mockChild = createMockChildProcess()
+      vi.mocked(spawn).mockReturnValue(mockChild as never)
+
+      const resultPromise = findCommandInShellEnv('npx', { PATH: '/usr/bin' })
+
+      // Simulate spawn error
+      mockChild.emit('error', new Error('spawn failed'))
+
+      const result = await resultPromise
+      expect(result).toBeNull()
+    })
+
+    it('should handle timeout gracefully', async () => {
+      vi.useFakeTimers()
+      const mockChild = createMockChildProcess()
+      vi.mocked(spawn).mockReturnValue(mockChild as never)
+
+      const resultPromise = findCommandInShellEnv('npx', { PATH: '/usr/bin' })
+
+      // Fast-forward past timeout (5000ms)
+      vi.advanceTimersByTime(6000)
+
+      const result = await resultPromise
+      expect(result).toBeNull()
+      expect(mockChild.kill).toHaveBeenCalledWith('SIGKILL')
+
+      vi.useRealTimers()
+    })
+  })
+
+  describe.skipIf(process.platform !== 'win32')('Windows behavior', () => {
+    it('should find .exe files via where command', async () => {
+      const mockChild = createMockChildProcess()
+      vi.mocked(spawn).mockReturnValue(mockChild as never)
+
+      const resultPromise = findCommandInShellEnv('npx', { PATH: 'C:\\nodejs' })
+
+      // Simulate where output
+      mockChild.stdout.emit('data', 'C:\\Program Files\\nodejs\\npx.exe\r\n')
+      mockChild.emit('close', 0)
+
+      const result = await resultPromise
+      expect(result).toBe('C:\\Program Files\\nodejs\\npx.exe')
+      expect(spawn).toHaveBeenCalledWith('where', ['npx'], expect.any(Object))
+    })
+
+    it('should reject .cmd files on Windows', async () => {
+      const mockChild = createMockChildProcess()
+      vi.mocked(spawn).mockReturnValue(mockChild as never)
+
+      const resultPromise = findCommandInShellEnv('npx', { PATH: 'C:\\nodejs' })
+
+      // Simulate where output with only .cmd file
+      mockChild.stdout.emit('data', 'C:\\Program Files\\nodejs\\npx.cmd\r\n')
+      mockChild.emit('close', 0)
+
+      const result = await resultPromise
+      expect(result).toBeNull()
+    })
+
+    it('should prefer .exe over .cmd when both exist', async () => {
+      const mockChild = createMockChildProcess()
+      vi.mocked(spawn).mockReturnValue(mockChild as never)
+
+      const resultPromise = findCommandInShellEnv('npx', { PATH: 'C:\\nodejs' })
+
+      // Simulate where output with both .cmd and .exe
+      mockChild.stdout.emit('data', 'C:\\Program Files\\nodejs\\npx.cmd\r\nC:\\Program Files\\nodejs\\npx.exe\r\n')
+      mockChild.emit('close', 0)
+
+      const result = await resultPromise
+      expect(result).toBe('C:\\Program Files\\nodejs\\npx.exe')
+    })
+
+    it('should handle spawn errors gracefully', async () => {
+      const mockChild = createMockChildProcess()
+      vi.mocked(spawn).mockReturnValue(mockChild as never)
+
+      const resultPromise = findCommandInShellEnv('npx', { PATH: 'C:\\nodejs' })
+
+      // Simulate spawn error
+      mockChild.emit('error', new Error('spawn failed'))
+
+      const result = await resultPromise
+      expect(result).toBeNull()
+    })
+  })
+})

src/main/utils/process.ts

@@ -64,6 +64,145 @@ export async function isBinaryExists(name: string): Promise<boolean> {
   return fs.existsSync(cmd)
 }
 
+// Timeout for command lookup operations (in milliseconds)
+const COMMAND_LOOKUP_TIMEOUT_MS = 5000
+
+// Regex to validate command names - must start with alphanumeric or underscore, max 128 chars
+const VALID_COMMAND_NAME_REGEX = /^[a-zA-Z0-9_][a-zA-Z0-9_-]{0,127}$/
+
+// Maximum output size to prevent buffer overflow (10KB)
+const MAX_OUTPUT_SIZE = 10240
+
+/**
+ * Check if a command is available in the user's login shell environment
+ * @param command - Command name to check (e.g., 'npx', 'uvx')
+ * @param loginShellEnv - The login shell environment from getLoginShellEnvironment()
+ * @returns Full path to the command if found, null otherwise
+ */
+export async function findCommandInShellEnv(
+  command: string,
+  loginShellEnv: Record<string, string>
+): Promise<string | null> {
+  // Validate command name to prevent command injection
+  if (!VALID_COMMAND_NAME_REGEX.test(command)) {
+    logger.warn(`Invalid command name '${command}' - must only contain alphanumeric characters, underscore, or hyphen`)
+    return null
+  }
+
+  return new Promise((resolve) => {
+    let resolved = false
+
+    const safeResolve = (value: string | null) => {
+      if (resolved) return
+      resolved = true
+      resolve(value)
+    }
+
+    if (isWin) {
+      // On Windows, use 'where' command
+      const child = spawn('where', [command], {
+        env: loginShellEnv,
+        stdio: ['ignore', 'pipe', 'pipe']
+      })
+
+      let output = ''
+      const timeoutId = setTimeout(() => {
+        if (resolved) return
+        child.kill('SIGKILL')
+        logger.debug(`Timeout checking command '${command}' on Windows`)
+        safeResolve(null)
+      }, COMMAND_LOOKUP_TIMEOUT_MS)
+
+      child.stdout.on('data', (data) => {
+        if (output.length < MAX_OUTPUT_SIZE) {
+          output += data.toString()
+        }
+      })
+
+      child.on('close', (code) => {
+        clearTimeout(timeoutId)
+        if (resolved) return
+
+        if (code === 0 && output.trim()) {
+          const paths = output.trim().split(/\r?\n/)
+          // Only accept .exe files on Windows - .cmd/.bat files cannot be executed
+          // with spawn({ shell: false }) which is used by MCP SDK's StdioClientTransport
+          const exePath = paths.find((p) => p.toLowerCase().endsWith('.exe'))
+          if (exePath) {
+            logger.debug(`Found command '${command}' at: ${exePath}`)
+            safeResolve(exePath)
+          } else {
+            logger.debug(`Command '${command}' found but not as .exe (${paths[0]}), treating as not found`)
+            safeResolve(null)
+          }
+        } else {
+          logger.debug(`Command '${command}' not found in shell environment`)
+          safeResolve(null)
+        }
+      })
+
+      child.on('error', (error) => {
+        clearTimeout(timeoutId)
+        if (resolved) return
+        logger.warn(`Error checking command '${command}':`, { error, platform: 'windows' })
+        safeResolve(null)
+      })
+    } else {
+      // Unix/Linux/macOS: use 'command -v' which is POSIX standard
+      // Use /bin/sh for reliability - it's POSIX compliant and always available
+      // This avoids issues with user's custom shell (csh, fish, etc.)
+      // SECURITY: Use positional parameter $1 to prevent command injection
+      const child = spawn('/bin/sh', ['-c', 'command -v "$1"', '--', command], {
+        env: loginShellEnv,
+        stdio: ['ignore', 'pipe', 'pipe']
+      })
+
+      let output = ''
+      const timeoutId = setTimeout(() => {
+        if (resolved) return
+        child.kill('SIGKILL')
+        logger.debug(`Timeout checking command '${command}'`)
+        safeResolve(null)
+      }, COMMAND_LOOKUP_TIMEOUT_MS)
+
+      child.stdout.on('data', (data) => {
+        if (output.length < MAX_OUTPUT_SIZE) {
+          output += data.toString()
+        }
+      })
+
+      child.on('close', (code) => {
+        clearTimeout(timeoutId)
+        if (resolved) return
+
+        if (code === 0 && output.trim()) {
+          const commandPath = output.trim().split('\n')[0]
+
+          // Validate the output is an absolute path (not an alias, function, or builtin)
+          // command -v can return just the command name for aliases/builtins
+          if (path.isAbsolute(commandPath)) {
+            logger.debug(`Found command '${command}' at: ${commandPath}`)
+            safeResolve(commandPath)
+          } else {
+            logger.debug(`Command '${command}' resolved to non-path '${commandPath}', treating as not found`)
+            safeResolve(null)
+          }
+        } else {
+          logger.debug(`Command '${command}' not found in shell environment`)
+          safeResolve(null)
+        }
+      })
+
+      child.on('error', (error) => {
+        clearTimeout(timeoutId)
+        if (resolved) return
+        logger.warn(`Error checking command '${command}':`, { error, platform: 'unix' })
+        safeResolve(null)
+      })
+    }
+  })
+}
+
 /**
  * Find executable in common paths or PATH environment variable
  * Based on Claude Code's implementation with security checks