diff --git a/src/main/services/MCPService.ts b/src/main/services/MCPService.ts index ebdc2247fc..5fc5cf8682 100644 --- a/src/main/services/MCPService.ts +++ b/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')) { diff --git a/src/main/utils/__tests__/process.test.ts b/src/main/utils/__tests__/process.test.ts index a1ac2fd9a5..373a66c48f 100644 --- a/src/main/utils/__tests__/process.test.ts +++ b/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 + } + 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() + }) + }) +}) diff --git a/src/main/utils/process.ts b/src/main/utils/process.ts index ccc0f66535..7c928949fe 100644 --- a/src/main/utils/process.ts +++ b/src/main/utils/process.ts @@ -64,6 +64,145 @@ export async function isBinaryExists(name: string): Promise { 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 +): Promise { + // 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