From b2870e782c14dba815026e7cae6c03bbf20308bd Mon Sep 17 00:00:00 2001 From: Vamsi-klu Date: Sun, 27 Sep 2026 00:40:05 +0000 Subject: [PATCH] fix(vscode): avoid shell splitting in subprocesses Signed-off-by: Vamsi-klu --- vscode/extension/src/utilities/exec.test.ts | 133 ++++++++++++++++++++ vscode/extension/src/utilities/exec.ts | 55 ++++---- vscode/extension/tests/utils.ts | 12 +- 3 files changed, 168 insertions(+), 32 deletions(-) create mode 100644 vscode/extension/src/utilities/exec.test.ts diff --git a/vscode/extension/src/utilities/exec.test.ts b/vscode/extension/src/utilities/exec.test.ts new file mode 100644 index 0000000000..906036b32d --- /dev/null +++ b/vscode/extension/src/utilities/exec.test.ts @@ -0,0 +1,133 @@ +// SPDX-License-Identifier: Apache-2.0 + +import { afterEach, describe, expect, it, vi } from 'vitest' +import fs from 'node:fs' +import path from 'node:path' +import os from 'node:os' +import { execAsync } from './exec' + +const { traceInfoMock } = vi.hoisted(() => ({ traceInfoMock: vi.fn() })) + +vi.mock('./common/log', () => ({ traceInfo: traceInfoMock })) + +/** Create a temporary directory for a test executable. */ +function makeTmpDir(): string { + return fs.mkdtempSync(path.join(os.tmpdir(), 'exec-test-')) +} + +describe('execAsync', () => { + const tmpdirs: string[] = [] + + afterEach(() => { + traceInfoMock.mockClear() + for (const dir of tmpdirs.splice(0)) { + fs.rmSync(dir, { recursive: true, force: true }) + } + }) + + it('executes an executable whose path contains a space', async () => { + // Create a temp directory whose name contains a space. + const baseDir = makeTmpDir() + tmpdirs.push(baseDir) + const spacedDir = path.join(baseDir, 'dir with spaces') + fs.mkdirSync(spacedDir) + + // Copy the current Node.js binary into the spaced directory. + const isWindows = process.platform === 'win32' + const ext = isWindows ? '.exe' : '' + const destExe = path.join(spacedDir, `node${ext}`) + fs.copyFileSync(process.execPath, destExe) + + // Ensure the copied binary is executable on POSIX. + if (!isWindows) { + fs.chmodSync(destExe, 0o755) + } + + // The path must contain a space for this test to be meaningful. + expect(destExe).toContain(' ') + + const result = await execAsync(destExe, ['--version']) + + expect(result.exitCode).toBe(0) + expect(result.stderr).toBe('') + expect(result.stdout.trim()).toBe(process.version) + }) + + it('passes arguments with spaces and shell metacharacters literally', async () => { + const argument = 'value with spaces; $(not-a-command) & more' + const result = await execAsync(process.execPath, [ + '-e', + 'process.stdout.write(process.argv[1])', + argument, + ]) + + expect(result.exitCode).toBe(0) + expect(result.stdout).toBe(argument) + expect(result.stderr).toBe('') + }) + + it('logs an unambiguous argv representation', async () => { + const argument = 'value with spaces\nand "quotes"' + + await execAsync(process.execPath, ['-e', '', argument]) + + expect(traceInfoMock).toHaveBeenCalledWith( + `Executing command: ${JSON.stringify([ + process.execPath, + '-e', + '', + argument, + ])} in undefined`, + ) + }) + + it('resolves with a nonzero ExecResult instead of rejecting', async () => { + // Run a Node.js one-liner that writes to stdout/stderr and exits nonzero. + const result = await execAsync(process.execPath, [ + '-e', + "process.stdout.write('out'); process.stderr.write('err'); process.exit(42)", + ]) + + expect(result.exitCode).toBe(42) + expect(result.stdout).toBe('out') + expect(result.stderr).toBe('err') + }) + + it('preserves empty stderr for a nonzero exit with no child output', async () => { + const result = await execAsync(process.execPath, ['-e', 'process.exit(7)']) + + expect(result.exitCode).toBe(7) + expect(result.stdout).toBe('') + expect(result.stderr).toBe('') + }) + + it('resolves spawn failures with a useful nonzero ExecResult', async () => { + const missingExecutable = path.join( + os.tmpdir(), + `missing-executable-${process.pid}-${Date.now()}`, + ) + expect(fs.existsSync(missingExecutable)).toBe(false) + + const result = await execAsync(missingExecutable) + + expect(result.exitCode).not.toBe(0) + expect(result.stdout).toBe('') + expect(result.stderr).toContain(path.basename(missingExecutable)) + }) + + it('rejects with AbortError when the signal is aborted', async () => { + const controller = new AbortController() + + // Start a long-running child process. + const promise = execAsync( + process.execPath, + ['-e', 'setTimeout(() => {}, 60_000)'], + { signal: controller.signal }, + ) + + // Abort immediately. + controller.abort() + + await expect(promise).rejects.toMatchObject({ name: 'AbortError' }) + }) +}) diff --git a/vscode/extension/src/utilities/exec.ts b/vscode/extension/src/utilities/exec.ts index 4748785b2b..4a78b0fb08 100644 --- a/vscode/extension/src/utilities/exec.ts +++ b/vscode/extension/src/utilities/exec.ts @@ -1,4 +1,6 @@ -import { exec, ExecOptions } from 'node:child_process' +// SPDX-License-Identifier: Apache-2.0 + +import { execFile, ExecOptions } from 'node:child_process' import { traceInfo } from './common/log' export interface ExecResult { @@ -12,7 +14,7 @@ export async function execAsync( args: string[] = [], options: ExecOptions & { signal?: AbortSignal } = {}, ): Promise { - const fullCmd = `${command} ${args.join(' ')}` + const fullCmd = JSON.stringify([command, ...args]) traceInfo(`Executing command: ${fullCmd} in ${options.cwd}`) try { @@ -37,33 +39,30 @@ function execAsyncCore( options: ExecOptions & { signal?: AbortSignal } = {}, ): Promise { return new Promise((resolve, reject) => { - const child = exec( - `${command} ${args.join(' ')}`, - options, - (error, stdout, stderr) => { - if (error) { - // Forward AbortError unchanged so callers can detect cancellation - if ((error as NodeJS.ErrnoException).name === 'AbortError') { - reject(error) - } else { - resolve({ - exitCode: typeof error.code === 'number' ? error.code : 1, - stdout, - stderr, - }) - } - return + execFile(command, args, options, (error, stdout, stderr) => { + if (error) { + // Forward AbortError unchanged so callers can detect cancellation + if ((error as NodeJS.ErrnoException).name === 'AbortError') { + reject(error as Error) + } else { + const exitCode = typeof error.code === 'number' ? error.code : 1 + resolve({ + exitCode, + stdout, + stderr: + typeof error.code === 'number' + ? stderr + : stderr || error.message, + }) } + return + } - resolve({ - exitCode: child.exitCode ?? 0, - stdout, - stderr, - }) - }, - ) - - // surface “spawn failed” errors that occur before the callback - child.once('error', reject) + resolve({ + exitCode: 0, + stdout, + stderr, + }) + }) }) } diff --git a/vscode/extension/tests/utils.ts b/vscode/extension/tests/utils.ts index effdc3c062..67214671fb 100644 --- a/vscode/extension/tests/utils.ts +++ b/vscode/extension/tests/utils.ts @@ -55,7 +55,7 @@ export const createVirtualEnvironment = async ( venvDir: string, ): Promise => { // Try to use uv first, fallback to python -m venv - const { exitCode, stderr } = await execAsync(`uv venv "${venvDir}"`) + const { exitCode, stderr } = await execAsync('uv', ['venv', venvDir]) if (exitCode !== 0) { throw new Error(`Failed to create venv with uv: ${stderr}`) } @@ -81,9 +81,13 @@ export const pipInstall = async ( pythonDetails: PythonEnvironment, packagePaths: string[], ): Promise => { - const packages = packagePaths.map(pkg => `-e "${pkg}"`).join(' ') - const execString = `uv pip install --python "${pythonDetails.pythonPath}" ${packages}` - const { stderr, exitCode } = await execAsync(execString) + const { stderr, exitCode } = await execAsync('uv', [ + 'pip', + 'install', + '--python', + pythonDetails.pythonPath, + ...packagePaths.flatMap(pkg => ['-e', pkg]), + ]) if (exitCode !== 0) { throw new Error(`Failed to install package with uv: ${stderr}`) }