From 69adf3488e62157f7183923dc4c15d24b1c21330 Mon Sep 17 00:00:00 2001 From: iliya Date: Fri, 27 Feb 2026 19:59:26 +0200 Subject: [PATCH] refactor: replace execFile and spawn with execCli and spawnCli in CLI and TeamProvisioning services - Updated CliInstallerService and TeamProvisioningService to use execCli and spawnCli for improved error handling and compatibility, particularly on Windows. - Enhanced child process utility functions to better manage non-ASCII paths and provide consistent behavior across different platforms. - Adjusted tests to mock new child process utilities and verify correct usage in service methods. --- .../infrastructure/CliInstallerService.ts | 13 +-- .../services/team/TeamProvisioningService.ts | 7 +- src/main/utils/childProcess.ts | 78 +++++++++++--- .../CliInstallerService.test.ts | 30 +++--- test/main/utils/childProcess.test.ts | 101 +++++++++++++----- 5 files changed, 164 insertions(+), 65 deletions(-) diff --git a/src/main/services/infrastructure/CliInstallerService.ts b/src/main/services/infrastructure/CliInstallerService.ts index cdb53b75..f8f62b84 100644 --- a/src/main/services/infrastructure/CliInstallerService.ts +++ b/src/main/services/infrastructure/CliInstallerService.ts @@ -17,17 +17,16 @@ * - Human-readable error messages per phase */ +import { execCli, spawnCli } from '@main/utils/childProcess'; import { getHomeDir } from '@main/utils/pathDecoder'; import { getErrorMessage } from '@shared/utils/errorHandling'; import { createLogger } from '@shared/utils/logger'; -import { execFile, spawn } from 'child_process'; import { createHash } from 'crypto'; import { createWriteStream, existsSync, promises as fsp } from 'fs'; import http from 'http'; import https from 'https'; import { tmpdir } from 'os'; import { join } from 'path'; -import { promisify } from 'util'; import { ClaudeBinaryResolver } from '../team/ClaudeBinaryResolver'; @@ -37,10 +36,6 @@ import type { IncomingMessage } from 'http'; const logger = createLogger('CliInstallerService'); -// Note: execFile (not exec) is used intentionally — no shell injection risk. -// Arguments are passed as arrays, never interpolated into shell strings. -const execFileAsync = promisify(execFile); - // ============================================================================= // Constants // ============================================================================= @@ -222,7 +217,7 @@ export class CliInstallerService { result.binaryPath = binaryPath; try { - const { stdout } = await execFileAsync(binaryPath, ['--version'], { + const { stdout } = await execCli(binaryPath, ['--version'], { timeout: VERSION_TIMEOUT_MS, env: buildChildEnv(), }); @@ -236,7 +231,7 @@ export class CliInstallerService { // Check auth status try { - const { stdout: authStdout } = await execFileAsync(binaryPath, ['auth', 'status'], { + const { stdout: authStdout } = await execCli(binaryPath, ['auth', 'status'], { timeout: VERSION_TIMEOUT_MS, env: buildChildEnv(), }); @@ -466,7 +461,7 @@ export class CliInstallerService { */ private async runInstallWithStreaming(binaryPath: string, attempt = 1): Promise { return new Promise((resolve, reject) => { - const child = spawn(binaryPath, ['install'], { + const child = spawnCli(binaryPath, ['install'], { env: { ...buildChildEnv(), CLAUDE_SKIP_ANALYTICS: '1' }, stdio: ['ignore', 'pipe', 'pipe'], }); diff --git a/src/main/services/team/TeamProvisioningService.ts b/src/main/services/team/TeamProvisioningService.ts index d84a0def..53845dd7 100644 --- a/src/main/services/team/TeamProvisioningService.ts +++ b/src/main/services/team/TeamProvisioningService.ts @@ -1,5 +1,6 @@ /* eslint-disable no-param-reassign -- ProvisioningRun object is intentionally mutated as a state tracker throughout the provisioning lifecycle */ import { ConfigManager } from '@main/services/infrastructure/ConfigManager'; +import { spawnCli } from '@main/utils/childProcess'; import { encodePath, extractBaseDir, @@ -930,7 +931,7 @@ export class TeamProvisioningService { ); } try { - child = spawn( + child = spawnCli( claudePath, [ '--input-format', @@ -1252,7 +1253,7 @@ export class TeamProvisioningService { // --resume is for existing sessions and may show an interactive picker if not found. try { - child = spawn(claudePath, launchArgs, { + child = spawnCli(claudePath, launchArgs, { cwd: request.cwd, env: { ...shellEnv, @@ -3127,7 +3128,7 @@ export class TeamProvisioningService { timeoutMs: number ): Promise<{ exitCode: number | null; stdout: string; stderr: string }> { return new Promise((resolve, reject) => { - const child = spawn(claudePath, args, { + const child = spawnCli(claudePath, args, { cwd, env, stdio: ['ignore', 'pipe', 'pipe'], diff --git a/src/main/utils/childProcess.ts b/src/main/utils/childProcess.ts index 18474b16..c40678f0 100644 --- a/src/main/utils/childProcess.ts +++ b/src/main/utils/childProcess.ts @@ -1,15 +1,59 @@ -import { spawn, execFile, exec, SpawnOptions, ExecFileOptions } from 'child_process'; -import { promisify } from 'util'; +import { + exec, + execFile, + type ExecFileOptions, + type ExecOptions, + spawn, + type SpawnOptions, +} from 'child_process'; -// re-exported helpers used throughout the codebase -export const execFileAsync = promisify(execFile); -export const execAsync = promisify(exec); +/** + * Promise wrapper for execFile that always returns { stdout, stderr }. + * Unlike promisify(execFile), this works correctly with mocked execFile + * (promisify relies on a custom symbol that mocks don't have). + */ +function execFileAsync( + cmd: string, + args: string[], + options: ExecFileOptions = {} +): Promise<{ stdout: string; stderr: string }> { + return new Promise((resolve, reject) => { + execFile(cmd, args, options, (err, stdout, stderr) => { + if (err) + reject( + err instanceof Error ? err : new Error(typeof err === 'string' ? err : 'Unknown error') + ); + else resolve({ stdout: String(stdout), stderr: String(stderr) }); + }); + }); +} + +/** + * Promise wrapper for exec. Used exclusively as a Windows shell fallback + * when execFile fails with EINVAL on non-ASCII binary paths. The command + * string is built from a known binary path + args, NOT from user input. + */ +function execShellAsync( + cmd: string, + options: ExecOptions = {} +): Promise<{ stdout: string; stderr: string }> { + return new Promise((resolve, reject) => { + // eslint-disable-next-line sonarjs/os-command, security/detect-child-process -- cmd from known binaryPath+args, not user input (Windows EINVAL fallback) + exec(cmd, options, (err, stdout, stderr) => { + if (err) + reject( + err instanceof Error ? err : new Error(typeof err === 'string' ? err : 'Unknown error') + ); + else resolve({ stdout: String(stdout), stderr: String(stderr) }); + }); + }); +} /** * Returns true if the string contains any non-ASCII character. */ function containsNonAscii(str: string): boolean { - return /[^\x00-\x7F]/.test(str); + return [...str].some((c) => c.charCodeAt(0) > 127); } /** @@ -30,7 +74,7 @@ function needsShell(binaryPath: string): boolean { * the shell itself will handle most quoting rules. */ function quoteArg(arg: string): string { - if (/[^A-Za-z0-9_\-\/.]/.test(arg)) { + if (/[^A-Za-z0-9_\-/.]/.test(arg)) { return `"${arg.replace(/"/g, '\\"')}"`; } return arg; @@ -56,10 +100,14 @@ export async function execCli( try { const result = await execFileAsync(target, args, options); return { stdout: String(result.stdout), stderr: String(result.stderr) }; - } catch (err: any) { + } catch (err: unknown) { // fall through to shell fallback only when the error matches the // Windows "invalid argument" problem; otherwise rethrow. - if (!(err && err.code === 'EINVAL')) { + const code = + err && typeof err === 'object' && 'code' in err + ? (err as { code?: string }).code + : undefined; + if (code !== 'EINVAL') { throw err; } } @@ -67,7 +115,7 @@ export async function execCli( // shell fallback (Windows only; others shouldn't reach here) const cmd = [target, ...args].map(quoteArg).join(' '); - const shellResult = await execAsync(cmd, options as unknown as import('child_process').ExecOptions); + const shellResult = await execShellAsync(cmd, options as unknown as ExecOptions); return { stdout: String(shellResult.stdout), stderr: String(shellResult.stderr) }; } @@ -81,17 +129,21 @@ export function spawnCli( binaryPath: string, args: string[], options: SpawnOptions = {} -) { +): ReturnType { if (process.platform === 'win32' && needsShell(binaryPath)) { const cmd = [binaryPath, ...args].map(quoteArg).join(' '); + // eslint-disable-next-line sonarjs/os-command -- cmd from known binaryPath+args, not user input (Windows EINVAL fallback) return spawn(cmd, { shell: true, ...options }); } try { return spawn(binaryPath, args, options); - } catch (err: any) { - if (process.platform === 'win32' && err && err.code === 'EINVAL') { + } catch (err: unknown) { + const code = + err && typeof err === 'object' && 'code' in err ? (err as { code?: string }).code : undefined; + if (process.platform === 'win32' && code === 'EINVAL') { const cmd = [binaryPath, ...args].map(quoteArg).join(' '); + // eslint-disable-next-line sonarjs/os-command -- cmd from known binaryPath+args, not user input (Windows EINVAL fallback) return spawn(cmd, { shell: true, ...options }); } throw err; diff --git a/test/main/services/infrastructure/CliInstallerService.test.ts b/test/main/services/infrastructure/CliInstallerService.test.ts index 099a082c..0cf134f6 100644 --- a/test/main/services/infrastructure/CliInstallerService.test.ts +++ b/test/main/services/infrastructure/CliInstallerService.test.ts @@ -1,11 +1,11 @@ import { beforeEach, describe, expect, it, vi } from 'vitest'; // Mock dependencies before importing service -vi.mock('child_process', async (importOriginal) => { - const actual = await importOriginal(); +vi.mock('@main/utils/childProcess', async (importOriginal) => { + const actual = await importOriginal(); return { ...actual, - execFile: vi.fn(), + execCli: vi.fn().mockRejectedValue(new Error('execCli not configured')), }; }); @@ -63,6 +63,7 @@ import { normalizeVersion, } from '@main/services/infrastructure/CliInstallerService'; import { ClaudeBinaryResolver } from '@main/services/team/ClaudeBinaryResolver'; +import { execCli } from '@main/utils/childProcess'; /** * Helper: allow expected console.error/warn calls in tests where service logs errors. @@ -111,23 +112,20 @@ describe('CliInstallerService', () => { const fakePath = 'C:\\Users\\Алексей\\AppData\\Roaming\\npm\\claude.cmd'; vi.mocked(ClaudeBinaryResolver.resolve).mockResolvedValue(fakePath); - // mock execFile to throw EINVAL first - const err: any = new Error('spawn EINVAL'); - err.code = 'EINVAL'; - const childProcess = await import('child_process'); - vi.spyOn(childProcess, 'execFile').mockImplementation((cmd, args, opts, cb) => { - cb(err, '', ''); - return {} as any; - }); - // mock exec to succeed as fallback - vi.spyOn(childProcess, 'exec').mockImplementation((cmd, opts, cb) => { - cb(null, '2.3.4', ''); - return {} as any; - }); + // execCli handles the EINVAL → shell fallback internally; + // here we just verify the service delegates to execCli correctly. + vi.mocked(execCli) + .mockResolvedValueOnce({ stdout: '2.3.4', stderr: '' }) // --version + .mockResolvedValueOnce({ stdout: '{}', stderr: '' }); // auth status const status = await service.getStatus(); expect(status.installed).toBe(true); expect(status.installedVersion).toBe('2.3.4'); + expect(execCli).toHaveBeenCalledWith( + fakePath, + ['--version'], + expect.objectContaining({ timeout: expect.any(Number) }) + ); }); }); diff --git a/test/main/utils/childProcess.test.ts b/test/main/utils/childProcess.test.ts index 2ead7909..1c705c61 100644 --- a/test/main/utils/childProcess.test.ts +++ b/test/main/utils/childProcess.test.ts @@ -1,12 +1,17 @@ +// @vitest-environment node import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; // Mock the entire child_process module so that we can inspect how our helpers // invoke spawn/exec without hitting the real filesystem or spawning anything. -vi.mock('child_process', () => ({ - spawn: vi.fn(), - execFile: vi.fn(), - exec: vi.fn(), -})); +vi.mock('child_process', async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + spawn: vi.fn(), + execFile: vi.fn(), + exec: vi.fn(), + }; +}); // Import after the mock call so that the mocked module is returned. import * as child from 'child_process'; @@ -16,6 +21,8 @@ import { spawnCli, execCli } from '@main/utils/childProcess'; function setPlatform(value: string) { Object.defineProperty(process, 'platform', { value, + configurable: true, + writable: true, }); } @@ -52,13 +59,31 @@ describe('cli child process helpers', () => { }); spawnMock.mockImplementationOnce(() => fake); - const result = spawnCli('C:\\Users\\\\AppData\\Roaming\\npm\\claude.cmd', ['a', 'b'], { + // Use ASCII path so needsShell returns false and we go through the try/catch EINVAL path + const result = spawnCli('C:\\bin\\claude.exe', ['a', 'b'], { env: { FOO: 'bar' }, }); expect(spawnMock).toHaveBeenCalledTimes(2); const secondArg0 = spawnMock.mock.calls[1][0] as string; - expect(secondArg0).toMatch(/claude\.cmd/); - expect(spawnMock.mock.calls[1][2]).toMatchObject({ shell: true, env: { FOO: 'bar' } }); + expect(secondArg0).toMatch(/claude\.exe/); + expect(spawnMock.mock.calls[1][1]).toMatchObject({ shell: true, env: { FOO: 'bar' } }); + expect(result).toBe(fake); + }); + + it('uses shell directly when path contains non-ASCII on windows', () => { + setPlatform('win32'); + const fake = {} as any; + const spawnMock = child.spawn as unknown as vi.Mock; + spawnMock.mockReturnValue(fake); + + const result = spawnCli('C:\\Users\\Алексей\\AppData\\Roaming\\npm\\claude.cmd', ['a', 'b'], { + env: { FOO: 'bar' }, + }); + // Non-ASCII detected upfront — single spawn call with shell: true + expect(spawnMock).toHaveBeenCalledTimes(1); + const shellCmd = spawnMock.mock.calls[0][0] as string; + expect(shellCmd).toMatch(/claude\.cmd/); + expect(spawnMock.mock.calls[0][1]).toMatchObject({ shell: true, env: { FOO: 'bar' } }); expect(result).toBe(fake); }); @@ -72,37 +97,65 @@ describe('cli child process helpers', () => { }); describe('execCli', () => { - it('invokes execFile when path is normal', async () => { + it('invokes execFile when path is ASCII on windows', async () => { setPlatform('win32'); const execFileMock = child.execFile as unknown as vi.Mock; - execFileMock.mockImplementation((cmd, args, opts, cb) => { - cb(null, 'ok', ''); - return {} as any; - }); + execFileMock.mockImplementation( + (_cmd: string, _args: string[], _opts: unknown, cb: Function) => { + cb(null, 'ok', ''); + return {} as any; + } + ); const result = await execCli('C:\\bin\\claude.exe', ['--version']); - expect(execFileMock).toHaveBeenCalledWith('C:\\bin\\claude.exe', ['--version'], {}, expect.any(Function)); + expect(execFileMock).toHaveBeenCalledWith( + 'C:\\bin\\claude.exe', + ['--version'], + {}, + expect.any(Function) + ); expect(result.stdout).toBe('ok'); }); - it('falls back to exec shell when execFile throws EINVAL or path contains non-ascii', async () => { + it('skips straight to shell when path contains non-ASCII on windows', async () => { setPlatform('win32'); const execFileMock = child.execFile as unknown as vi.Mock; - execFileMock.mockImplementation((cmd, args, opts, cb) => { - const err: any = new Error('spawn EINVAL'); - err.code = 'EINVAL'; - cb(err, '', ''); - return {} as any; - }); const execMock = child.exec as unknown as vi.Mock; - execMock.mockImplementation((cmd, opts, cb) => { + execMock.mockImplementation((_cmd: string, _opts: unknown, cb: Function) => { cb(null, '1.2.3', ''); return {} as any; }); - const result = await execCli('C:\\Users\\\\AppData\\Roaming\\npm\\claude.cmd', ['--version']); - expect(execFileMock).toHaveBeenCalled(); + const result = await execCli('C:\\Users\\Алексей\\AppData\\Roaming\\npm\\claude.cmd', [ + '--version', + ]); + // non-ASCII path detected upfront — execFile should NOT be called + expect(execFileMock).not.toHaveBeenCalled(); expect(execMock).toHaveBeenCalled(); expect(result.stdout).toBe('1.2.3'); }); + + it('falls back to shell when execFile throws EINVAL on windows', async () => { + setPlatform('win32'); + const execFileMock = child.execFile as unknown as vi.Mock; + execFileMock.mockImplementation( + (_cmd: string, _args: string[], _opts: unknown, cb: Function) => { + const err: any = new Error('spawn EINVAL'); + err.code = 'EINVAL'; + cb(err, '', ''); + return {} as any; + } + ); + const execMock = child.exec as unknown as vi.Mock; + execMock.mockImplementation((_cmd: string, _opts: unknown, cb: Function) => { + cb(null, '2.3.4', ''); + return {} as any; + }); + + // ASCII path — goes through execFile first, gets EINVAL, falls back to shell + const result = await execCli('C:\\bin\\claude.exe', ['--version']); + expect(execFileMock).toHaveBeenCalled(); + expect(execMock).toHaveBeenCalled(); + expect(result.stdout).toBe('2.3.4'); + }); }); });