diff --git a/apps/frontend/src/main/__tests__/cli-tool-manager.test.ts b/apps/frontend/src/main/__tests__/cli-tool-manager.test.ts index 9eef26079a..edb6af1625 100644 --- a/apps/frontend/src/main/__tests__/cli-tool-manager.test.ts +++ b/apps/frontend/src/main/__tests__/cli-tool-manager.test.ts @@ -17,10 +17,27 @@ import { } from '../cli-tool-manager'; import { findWindowsExecutableViaWhere, - findWindowsExecutableViaWhereAsync + findWindowsExecutableViaWhereAsync, + isSecurePath } from '../utils/windows-paths'; import { findExecutable, findExecutableAsync } from '../env-utils'; +type SpawnOptions = Parameters<(typeof import('../env-utils'))['getSpawnOptions']>[1]; +type MockDirent = import('fs').Dirent; + +const createDirent = (name: string, isDir: boolean): MockDirent => + ({ + name, + parentPath: '', + isDirectory: () => isDir, + isFile: () => !isDir, + isBlockDevice: () => false, + isCharacterDevice: () => false, + isSymbolicLink: () => false, + isFIFO: () => false, + isSocket: () => false + }) as unknown as MockDirent; + // Mock Electron app vi.mock('electron', () => ({ app: { @@ -52,7 +69,7 @@ vi.mock('child_process', () => { // so when tests call vi.mocked(execFileSync).mockReturnValue(), it affects execSync too const sharedSyncMock = vi.fn(); - const mockExecFile = vi.fn((cmd: any, args: any, options: any, callback: any) => { +const mockExecFile = vi.fn((cmd: unknown, args: unknown, options: unknown, callback: unknown) => { // Return a minimal ChildProcess-like object const childProcess = { stdout: { on: vi.fn() }, @@ -62,13 +79,14 @@ vi.mock('child_process', () => { // If callback is provided, call it asynchronously if (typeof callback === 'function') { - setImmediate(() => callback(null, 'claude-code version 1.0.0\n', '')); + const cb = callback as (error: Error | null, stdout: string, stderr: string) => void; + setImmediate(() => cb(null, 'claude-code version 1.0.0\n', '')); } - return childProcess as any; + return childProcess as unknown as import('child_process').ChildProcess; }); - const mockExec = vi.fn((cmd: any, options: any, callback: any) => { + const mockExec = vi.fn((cmd: unknown, options: unknown, callback: unknown) => { // Return a minimal ChildProcess-like object const childProcess = { stdout: { on: vi.fn() }, @@ -78,10 +96,11 @@ vi.mock('child_process', () => { // If callback is provided, call it asynchronously if (typeof callback === 'function') { - setImmediate(() => callback(null, 'claude-code version 1.0.0\n', '')); + const cb = callback as (error: Error | null, stdout: string, stderr: string) => void; + setImmediate(() => cb(null, 'claude-code version 1.0.0\n', '')); } - return childProcess as any; + return childProcess as unknown as import('child_process').ChildProcess; }); return { @@ -93,18 +112,23 @@ vi.mock('child_process', () => { }); // Mock env-utils to avoid PATH augmentation complexity -vi.mock('../env-utils', () => ({ +vi.mock('../env-utils', () => { + const mockShouldUseShell = vi.fn((command: string) => { + if (process.platform !== 'win32') { + return false; + } + const trimmed = command.trim(); + const unquoted = + trimmed.startsWith('"') && trimmed.endsWith('"') ? trimmed.slice(1, -1) : trimmed; + return /\.(cmd|bat)$/i.test(unquoted); + }); + + return ({ findExecutable: vi.fn(() => null), // Return null to force platform-specific path checking findExecutableAsync: vi.fn(() => Promise.resolve(null)), getAugmentedEnv: vi.fn(() => ({ PATH: '' })), getAugmentedEnvAsync: vi.fn(() => Promise.resolve({ PATH: '' })), - shouldUseShell: vi.fn((command: string) => { - // Mock shouldUseShell to match actual behavior - if (process.platform !== 'win32') { - return false; - } - return /\.(cmd|bat)$/i.test(command); - }), + shouldUseShell: mockShouldUseShell, getSpawnCommand: vi.fn((command: string) => { // Mock getSpawnCommand to match actual behavior const trimmed = command.trim(); @@ -122,12 +146,13 @@ vi.mock('../env-utils', () => ({ } return trimmed; }), - getSpawnOptions: vi.fn((command: string, baseOptions?: any) => ({ + getSpawnOptions: vi.fn((command: string, baseOptions?: SpawnOptions) => ({ ...baseOptions, - shell: /\.(cmd|bat)$/i.test(command) && process.platform === 'win32' + shell: mockShouldUseShell(command) })), existsAsync: vi.fn(() => Promise.resolve(false)) -})); + }); +}); // Mock homebrew-python utility vi.mock('../utils/homebrew-python', () => ({ @@ -137,7 +162,11 @@ vi.mock('../utils/homebrew-python', () => ({ // Mock windows-paths utility vi.mock('../utils/windows-paths', () => ({ findWindowsExecutableViaWhere: vi.fn(() => null), - findWindowsExecutableViaWhereAsync: vi.fn(() => Promise.resolve(null)) + findWindowsExecutableViaWhereAsync: vi.fn(() => Promise.resolve(null)), + isSecurePath: vi.fn(() => true), + getWindowsExecutablePaths: vi.fn(() => []), + getWindowsExecutablePathsAsync: vi.fn(() => Promise.resolve([])), + WINDOWS_GIT_PATHS: {} })); describe('cli-tool-manager - Claude CLI NVM detection', () => { @@ -176,12 +205,12 @@ describe('cli-tool-manager - Claude CLI NVM detection', () => { }); // Mock Node.js version directories (three versions) - const mockDirents = [ - { name: 'v20.0.0', isDirectory: () => true }, - { name: 'v22.17.0', isDirectory: () => true }, - { name: 'v18.20.0', isDirectory: () => true }, + const mockDirents: MockDirent[] = [ + createDirent('v20.0.0', true), + createDirent('v22.17.0', true), + createDirent('v18.20.0', true), ]; - vi.mocked(readdirSync).mockReturnValue(mockDirents as any); + vi.mocked(readdirSync).mockReturnValue(mockDirents); // Mock execFileSync to simulate successful version check vi.mocked(execFileSync).mockReturnValue('claude-code version 1.0.0\n'); @@ -245,11 +274,11 @@ describe('cli-tool-manager - Claude CLI NVM detection', () => { return false; }); - const mockDirents = [ - { name: 'v22.17.0', isDirectory: () => true }, - { name: 'v20.0.0', isDirectory: () => true }, + const mockDirents: MockDirent[] = [ + createDirent('v22.17.0', true), + createDirent('v20.0.0', true), ]; - vi.mocked(readdirSync).mockReturnValue(mockDirents as any); + vi.mocked(readdirSync).mockReturnValue(mockDirents); vi.mocked(execFileSync).mockReturnValue('claude-code version 1.5.0\n'); const result = getToolInfo('claude'); @@ -272,10 +301,10 @@ describe('cli-tool-manager - Claude CLI NVM detection', () => { return false; }); - const mockDirents = [ - { name: 'v22.17.0', isDirectory: () => true }, + const mockDirents: MockDirent[] = [ + createDirent('v22.17.0', true), ]; - vi.mocked(readdirSync).mockReturnValue(mockDirents as any); + vi.mocked(readdirSync).mockReturnValue(mockDirents); // Mock validation failure vi.mocked(execFileSync).mockImplementation(() => { @@ -301,12 +330,12 @@ describe('cli-tool-manager - Claude CLI NVM detection', () => { }); // Versions in random order - const mockDirents = [ - { name: 'v18.20.0', isDirectory: () => true }, - { name: 'v22.17.0', isDirectory: () => true }, - { name: 'v20.5.0', isDirectory: () => true }, + const mockDirents: MockDirent[] = [ + createDirent('v18.20.0', true), + createDirent('v22.17.0', true), + createDirent('v20.5.0', true), ]; - vi.mocked(readdirSync).mockReturnValue(mockDirents as any); + vi.mocked(readdirSync).mockReturnValue(mockDirents); vi.mocked(execFileSync).mockReturnValue('claude-code version 1.0.0\n'); const result = getToolInfo('claude'); @@ -345,6 +374,35 @@ describe('cli-tool-manager - Claude CLI NVM detection', () => { expect(result.source).toBe('system-path'); }); + it('should ignore insecure Windows Claude CLI path from where.exe', () => { + Object.defineProperty(process, 'platform', { + value: 'win32', + writable: true + }); + + vi.mocked(os.homedir).mockReturnValue('C:\\Users\\test'); + vi.mocked(findExecutable).mockReturnValue(null); + vi.mocked(findWindowsExecutableViaWhere).mockReturnValue( + 'D:\\Tools\\claude.cmd' + ); + vi.mocked(isSecurePath).mockReturnValueOnce(false); + + vi.mocked(existsSync).mockImplementation((filePath) => { + const pathStr = String(filePath); + if (pathStr.includes('Tools') && pathStr.includes('claude.cmd')) { + return true; + } + return false; + }); + + const result = getToolInfo('claude'); + + expect(result.found).toBe(false); + expect(result.source).toBe('fallback'); + expect(execFileSync).not.toHaveBeenCalled(); + expect(isSecurePath).toHaveBeenCalledWith('D:\\Tools\\claude.cmd'); + }); + it('should detect Claude CLI in Unix .local/bin path', () => { vi.mocked(os.homedir).mockReturnValue('/home/user'); diff --git a/apps/frontend/src/main/__tests__/insights-config.test.ts b/apps/frontend/src/main/__tests__/insights-config.test.ts index 5775d65ab0..ef3df618dc 100644 --- a/apps/frontend/src/main/__tests__/insights-config.test.ts +++ b/apps/frontend/src/main/__tests__/insights-config.test.ts @@ -59,7 +59,9 @@ describe('InsightsConfig', () => { expect(env.CLAUDE_CODE_OAUTH_TOKEN).toBe('oauth-token'); expect(env.ANTHROPIC_BASE_URL).toBe('https://api.z.ai'); expect(env.ANTHROPIC_AUTH_TOKEN).toBe('key'); - expect(env.PYTHONPATH).toBe(['/site-packages', '/backend'].join(path.delimiter)); + expect(env.PYTHONPATH).toBe( + [path.resolve('/site-packages'), path.resolve('/backend')].join(path.delimiter) + ); }); it('should clear ANTHROPIC env vars in OAuth mode when no API profile is set', async () => { @@ -84,7 +86,7 @@ describe('InsightsConfig', () => { const env = await config.getProcessEnv(); - expect(env.PYTHONPATH).toBe('/backend'); + expect(env.PYTHONPATH).toBe(path.resolve('/backend')); }); it('should keep PYTHONPATH from python env when auto-build path is missing', async () => { @@ -94,6 +96,6 @@ describe('InsightsConfig', () => { const env = await config.getProcessEnv(); - expect(env.PYTHONPATH).toBe('/site-packages'); + expect(env.PYTHONPATH).toBe(path.resolve('/site-packages')); }); }); diff --git a/apps/frontend/src/main/cli-tool-manager.ts b/apps/frontend/src/main/cli-tool-manager.ts index 6ecc8f8cc9..ef09b62563 100644 --- a/apps/frontend/src/main/cli-tool-manager.ts +++ b/apps/frontend/src/main/cli-tool-manager.ts @@ -20,7 +20,7 @@ * - Graceful fallbacks when tools not found */ -import { execFileSync, execFile, execSync, exec } from 'child_process'; +import { execFileSync, execFile } from 'child_process'; import { existsSync, readdirSync, promises as fsPromises } from 'fs'; import path from 'path'; import os from 'os'; @@ -28,10 +28,19 @@ import { promisify } from 'util'; import { app } from 'electron'; import { findExecutable, findExecutableAsync, getAugmentedEnv, getAugmentedEnvAsync, shouldUseShell, existsAsync } from './env-utils'; import type { ToolDetectionResult } from '../shared/types'; +import { findHomebrewPython as findHomebrewPythonUtil } from './utils/homebrew-python'; const execFileAsync = promisify(execFile); -const execAsync = promisify(exec); -import { findHomebrewPython as findHomebrewPythonUtil } from './utils/homebrew-python'; + +type ExecFileSyncOptionsWithVerbatim = import('child_process').ExecFileSyncOptions & { + windowsVerbatimArguments?: boolean; +}; +type ExecFileAsyncOptionsWithVerbatim = import('child_process').ExecFileOptionsWithStringEncoding & { + windowsVerbatimArguments?: boolean; +}; + +const normalizeExecOutput = (output: string | Buffer): string => + typeof output === 'string' ? output : output.toString('utf-8'); import { getWindowsExecutablePaths, getWindowsExecutablePathsAsync, @@ -926,28 +935,51 @@ class CLIToolManager { */ private validateClaude(claudeCmd: string): ToolValidation { try { - const needsShell = shouldUseShell(claudeCmd); + const trimmedCmd = claudeCmd.trim(); + const unquotedCmd = + trimmedCmd.startsWith('"') && trimmedCmd.endsWith('"') + ? trimmedCmd.slice(1, -1) + : trimmedCmd; + + const needsShell = shouldUseShell(trimmedCmd); + const cmdDir = path.dirname(unquotedCmd); + const env = getAugmentedEnv(cmdDir && cmdDir !== '.' ? [cmdDir] : []); let version: string; if (needsShell) { - // For .cmd/.bat files on Windows, use cmd.exe with argument array - // This avoids shell command injection while handling spaces in paths - version = execFileSync('cmd.exe', ['/c', claudeCmd, '--version'], { + // For .cmd/.bat files on Windows, use cmd.exe with a quoted command line + // /s preserves quotes so paths with spaces are handled correctly. + if (!isSecurePath(unquotedCmd)) { + return { + valid: false, + message: `Claude CLI path failed security validation: ${unquotedCmd}`, + }; + } + const cmdExe = process.env.ComSpec + || path.join(process.env.SystemRoot || 'C:\\Windows', 'System32', 'cmd.exe'); + const cmdLine = `""${unquotedCmd}" --version"`; + const execOptions: ExecFileSyncOptionsWithVerbatim = { encoding: 'utf-8', timeout: 5000, windowsHide: true, - env: getAugmentedEnv(), - }).trim(); + windowsVerbatimArguments: true, + env, + }; + version = normalizeExecOutput( + execFileSync(cmdExe, ['/d', '/s', '/c', cmdLine], execOptions) + ).trim(); } else { // For .exe files and non-Windows, use execFileSync - version = execFileSync(claudeCmd, ['--version'], { - encoding: 'utf-8', - timeout: 5000, - windowsHide: true, - shell: false, - env: getAugmentedEnv(), - }).trim(); + version = normalizeExecOutput( + execFileSync(unquotedCmd, ['--version'], { + encoding: 'utf-8', + timeout: 5000, + windowsHide: true, + shell: false, + env, + }) + ).trim(); } // Claude CLI version output format: "claude-code version X.Y.Z" or similar @@ -1042,33 +1074,52 @@ class CLIToolManager { */ private async validateClaudeAsync(claudeCmd: string): Promise { try { - const needsShell = shouldUseShell(claudeCmd); + const trimmedCmd = claudeCmd.trim(); + const unquotedCmd = + trimmedCmd.startsWith('"') && trimmedCmd.endsWith('"') + ? trimmedCmd.slice(1, -1) + : trimmedCmd; + + const needsShell = shouldUseShell(trimmedCmd); + const cmdDir = path.dirname(unquotedCmd); + const env = await getAugmentedEnvAsync(cmdDir && cmdDir !== '.' ? [cmdDir] : []); let stdout: string; if (needsShell) { - // For .cmd/.bat files on Windows, use cmd.exe with argument array - // This avoids shell command injection while handling spaces in paths - const result = await execFileAsync('cmd.exe', ['/c', claudeCmd, '--version'], { + // For .cmd/.bat files on Windows, use cmd.exe with a quoted command line + // /s preserves quotes so paths with spaces are handled correctly. + if (!isSecurePath(unquotedCmd)) { + return { + valid: false, + message: `Claude CLI path failed security validation: ${unquotedCmd}`, + }; + } + const cmdExe = process.env.ComSpec + || path.join(process.env.SystemRoot || 'C:\\Windows', 'System32', 'cmd.exe'); + const cmdLine = `""${unquotedCmd}" --version"`; + const execOptions: ExecFileAsyncOptionsWithVerbatim = { encoding: 'utf-8', timeout: 5000, windowsHide: true, - env: await getAugmentedEnvAsync(), - }); + windowsVerbatimArguments: true, + env, + }; + const result = await execFileAsync(cmdExe, ['/d', '/s', '/c', cmdLine], execOptions); stdout = result.stdout; } else { // For .exe files and non-Windows, use execFileAsync - const result = await execFileAsync(claudeCmd, ['--version'], { + const result = await execFileAsync(unquotedCmd, ['--version'], { encoding: 'utf-8', timeout: 5000, windowsHide: true, shell: false, - env: await getAugmentedEnvAsync(), + env, }); stdout = result.stdout; } - const version = stdout.trim(); + const version = normalizeExecOutput(stdout).trim(); const match = version.match(/(\d+\.\d+\.\d+)/); const versionStr = match ? match[1] : version.split('\n')[0]; diff --git a/apps/frontend/src/main/terminal/__tests__/claude-integration-handler.test.ts b/apps/frontend/src/main/terminal/__tests__/claude-integration-handler.test.ts index 92090e0b18..8f44838aa4 100644 --- a/apps/frontend/src/main/terminal/__tests__/claude-integration-handler.test.ts +++ b/apps/frontend/src/main/terminal/__tests__/claude-integration-handler.test.ts @@ -1,8 +1,10 @@ import { writeFileSync } from 'fs'; import { tmpdir } from 'os'; +import path from 'path'; import { describe, expect, it, vi, beforeEach } from 'vitest'; import type * as pty from '@lydell/node-pty'; import type { TerminalProcess } from '../types'; +import { buildCdCommand } from '../../../shared/utils/shell-escape'; /** Escape special regex characters in a string for safe use in RegExp constructor */ const escapeForRegex = (str: string): string => str.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); @@ -100,7 +102,7 @@ describe('claude-integration-handler', () => { invokeClaude(terminal, '/tmp/project', undefined, () => null, vi.fn()); const written = vi.mocked(terminal.pty.write).mock.calls[0][0] as string; - expect(written).toContain("cd '/tmp/project' && "); + expect(written).toContain(buildCdCommand('/tmp/project')); expect(written).toContain("PATH='/opt/claude/bin:/usr/bin' "); expect(written).toContain("'/opt/claude bin/claude'\\''s'"); expect(mockReleaseSessionId).toHaveBeenCalledWith('term-1'); @@ -233,8 +235,8 @@ describe('claude-integration-handler', () => { const tokenPath = vi.mocked(writeFileSync).mock.calls[0]?.[0] as string; const tokenContents = vi.mocked(writeFileSync).mock.calls[0]?.[1] as string; - const tmpDir = escapeForRegex(tmpdir()); - expect(tokenPath).toMatch(new RegExp(`^${tmpDir}/\\.claude-token-1234-[0-9a-f]{16}$`)); + const tokenPrefix = path.join(tmpdir(), '.claude-token-1234-'); + expect(tokenPath).toMatch(new RegExp(`^${escapeForRegex(tokenPrefix)}[0-9a-f]{16}$`)); expect(tokenContents).toBe("export CLAUDE_CODE_OAUTH_TOKEN='token-value'\n"); const written = vi.mocked(terminal.pty.write).mock.calls[0][0] as string; expect(written).toContain("HISTFILE= HISTCONTROL=ignorespace "); @@ -276,8 +278,8 @@ describe('claude-integration-handler', () => { const tokenPath = vi.mocked(writeFileSync).mock.calls[0]?.[0] as string; const tokenContents = vi.mocked(writeFileSync).mock.calls[0]?.[1] as string; - const tmpDir = escapeForRegex(tmpdir()); - expect(tokenPath).toMatch(new RegExp(`^${tmpDir}/\\.claude-token-5678-[0-9a-f]{16}$`)); + const tokenPrefix = path.join(tmpdir(), '.claude-token-5678-'); + expect(tokenPath).toMatch(new RegExp(`^${escapeForRegex(tokenPrefix)}[0-9a-f]{16}$`)); expect(tokenContents).toBe("export CLAUDE_CODE_OAUTH_TOKEN='token-value'\n"); const written = vi.mocked(terminal.pty.write).mock.calls[0][0] as string; expect(written).toContain(`source '${tokenPath}'`);