From 73306daa6eed942e48d14d80b247b77f1def8420 Mon Sep 17 00:00:00 2001 From: gonzaloriestra <14979109+gonzaloriestra@users.noreply.github.com> Date: Sun, 3 May 2026 00:32:25 +0000 Subject: [PATCH 1/2] [Security] Add command safety check to execCommand This change adds a call to `checkCommandSafety` in the `execCommand` function in `packages/cli-kit/src/public/node/system.ts`. This ensures that any command executed via `execCommand` is checked for unsecure binaries in the current working directory, preventing binary planting/PATH hijacking attacks. A test case has been added to `packages/cli-kit/src/public/node/system.test.ts` to verify this behavior. Co-authored-by: Cursor --- packages/cli-kit/src/public/node/system.test.ts | 11 +++++++++++ packages/cli-kit/src/public/node/system.ts | 4 ++++ 2 files changed, 15 insertions(+) diff --git a/packages/cli-kit/src/public/node/system.test.ts b/packages/cli-kit/src/public/node/system.test.ts index f61a9318991..48c638d01ca 100644 --- a/packages/cli-kit/src/public/node/system.test.ts +++ b/packages/cli-kit/src/public/node/system.test.ts @@ -266,6 +266,17 @@ describe('execCommand', () => { // Then expect(execaCommand).toHaveBeenCalledWith('cat', expect.objectContaining({stdin: 'inherit'})) }) + + test('raises an error if the command to run is found in the current directory', async () => { + // Given + vi.mocked(which.sync).mockReturnValueOnce('/currentDirectory/command') + + // When + const got = system.execCommand('command', {cwd: '/currentDirectory'}) + + // Then + await expect(got).rejects.toThrowError('Skipped run of unsecure binary command found in the current directory.') + }) }) describe('isStdinPiped', () => { diff --git a/packages/cli-kit/src/public/node/system.ts b/packages/cli-kit/src/public/node/system.ts index a532dce70a0..f85ca9b7e8d 100644 --- a/packages/cli-kit/src/public/node/system.ts +++ b/packages/cli-kit/src/public/node/system.ts @@ -206,6 +206,10 @@ export async function execCommand(command: string, options?: ExecOptions): Promi env.FORCE_COLOR = '1' } const executionCwd = options?.cwd ?? cwd() + const [cmd] = parseCommand(command) + if (cmd) { + checkCommandSafety(cmd, {cwd: executionCwd}) + } try { await execaCommand(command, { env, From d2178bef5ff67d029bc640feb4e052c68d0366c6 Mon Sep 17 00:00:00 2001 From: Gonzalo Riestra Date: Tue, 5 May 2026 09:06:41 +0200 Subject: [PATCH 2/2] Ensure we run exactly the same command we checked --- .../cli-kit/src/public/node/system.test.ts | 50 ++++++++++++++----- packages/cli-kit/src/public/node/system.ts | 11 ++-- 2 files changed, 44 insertions(+), 17 deletions(-) diff --git a/packages/cli-kit/src/public/node/system.test.ts b/packages/cli-kit/src/public/node/system.test.ts index 48c638d01ca..b85cc9ade53 100644 --- a/packages/cli-kit/src/public/node/system.test.ts +++ b/packages/cli-kit/src/public/node/system.test.ts @@ -1,5 +1,5 @@ import * as system from './system.js' -import {execa, execaCommand} from 'execa' +import {execa} from 'execa' import {describe, expect, test, vi} from 'vitest' import which from 'which' import {Readable} from 'stream' @@ -206,16 +206,19 @@ describe('captureCommandWithExitCode', () => { describe('execCommand', () => { test('runs command successfully without throwing', async () => { // Given - vi.mocked(execaCommand).mockResolvedValueOnce({} as any) + vi.mocked(which.sync).mockReturnValueOnce('/system/echo') + vi.mocked(execa).mockResolvedValueOnce({} as any) // When/Then await expect(system.execCommand('echo hello')).resolves.toBeUndefined() + expect(execa).toHaveBeenCalledWith('echo', ['hello'], expect.anything()) }) test('throws ExternalError on command failure', async () => { // Given const error = new Error('command not found') - vi.mocked(execaCommand).mockRejectedValueOnce(error) + vi.mocked(which.sync).mockReturnValueOnce('/system/nonexistent') + vi.mocked(execa).mockRejectedValueOnce(error) // When/Then await expect(system.execCommand('nonexistent')).rejects.toThrow('command not found') @@ -224,7 +227,8 @@ describe('execCommand', () => { test('calls custom error handler when provided', async () => { // Given const error = new Error('custom error') - vi.mocked(execaCommand).mockRejectedValueOnce(error) + vi.mocked(which.sync).mockReturnValueOnce('/system/failing') + vi.mocked(execa).mockRejectedValueOnce(error) const customHandler = vi.fn() // When @@ -234,37 +238,42 @@ describe('execCommand', () => { expect(customHandler).toHaveBeenCalledWith(error) }) - test('handles command with spaces in arguments', async () => { + test('handles command with spaces in arguments (quoted strings)', async () => { // Given - vi.mocked(execaCommand).mockResolvedValueOnce({} as any) + vi.mocked(which.sync).mockReturnValueOnce('/system/touch') + vi.mocked(execa).mockResolvedValueOnce({} as any) // When await system.execCommand('touch "my file.txt"') // Then - expect(execaCommand).toHaveBeenCalledWith('touch "my file.txt"', expect.anything()) + // The quoted argument is parsed into a single argument without quotes, + // and the executable launched matches the executable that was safety-checked. + expect(execa).toHaveBeenCalledWith('touch', ['my file.txt'], expect.anything()) }) test('uses provided cwd option', async () => { // Given - vi.mocked(execaCommand).mockResolvedValueOnce({} as any) + vi.mocked(which.sync).mockReturnValueOnce('/system/pwd') + vi.mocked(execa).mockResolvedValueOnce({} as any) // When await system.execCommand('pwd', {cwd: '/some/dir'}) // Then - expect(execaCommand).toHaveBeenCalledWith('pwd', expect.objectContaining({cwd: '/some/dir'})) + expect(execa).toHaveBeenCalledWith('pwd', [], expect.objectContaining({cwd: '/some/dir'})) }) - test('passes stdin option to execaCommand', async () => { + test('passes stdin option to execa', async () => { // Given - vi.mocked(execaCommand).mockResolvedValueOnce({} as any) + vi.mocked(which.sync).mockReturnValueOnce('/system/cat') + vi.mocked(execa).mockResolvedValueOnce({} as any) // When await system.execCommand('cat', {stdin: 'inherit'}) // Then - expect(execaCommand).toHaveBeenCalledWith('cat', expect.objectContaining({stdin: 'inherit'})) + expect(execa).toHaveBeenCalledWith('cat', [], expect.objectContaining({stdin: 'inherit'})) }) test('raises an error if the command to run is found in the current directory', async () => { @@ -277,6 +286,23 @@ describe('execCommand', () => { // Then await expect(got).rejects.toThrowError('Skipped run of unsecure binary command found in the current directory.') }) + + test('safety check and execution agree on the binary (no parser mismatch bypass)', async () => { + // Given + // Whatever token the safety check approves must be exactly what execa launches. + // Previously, parseCommand() could approve one token while execaCommand() launched + // a different one (e.g. via backslash-escaped spaces), bypassing checkCommandSafety. + vi.mocked(which.sync).mockReturnValueOnce('/system/some-binary') + vi.mocked(execa).mockResolvedValueOnce({} as any) + + // When + await system.execCommand('some-binary arg1 arg2') + + // Then + const checkedCommand = vi.mocked(which.sync).mock.calls[0]?.[0] + const launchedCommand = vi.mocked(execa).mock.calls[0]?.[0] + expect(launchedCommand).toBe(checkedCommand) + }) }) describe('isStdinPiped', () => { diff --git a/packages/cli-kit/src/public/node/system.ts b/packages/cli-kit/src/public/node/system.ts index f85ca9b7e8d..cc13184878f 100644 --- a/packages/cli-kit/src/public/node/system.ts +++ b/packages/cli-kit/src/public/node/system.ts @@ -6,7 +6,7 @@ import {isTruthy} from './context/utilities.js' import {renderWarning} from './ui.js' import {platformAndArch} from './os.js' import {shouldDisplayColors, outputDebug} from './output.js' -import {execa, execaCommand, ExecaChildProcess} from 'execa' +import {execa, ExecaChildProcess} from 'execa' import supportsHyperlinks from 'supports-hyperlinks' import which from 'which' import {delimiter} from 'pathe' @@ -206,12 +206,13 @@ export async function execCommand(command: string, options?: ExecOptions): Promi env.FORCE_COLOR = '1' } const executionCwd = options?.cwd ?? cwd() - const [cmd] = parseCommand(command) - if (cmd) { - checkCommandSafety(cmd, {cwd: executionCwd}) + const [cmd, ...args] = parseCommand(command) + if (!cmd) { + throw new AbortError('Empty command') } + checkCommandSafety(cmd, {cwd: executionCwd}) try { - await execaCommand(command, { + await execa(cmd, args, { env, cwd: executionCwd, stdin: options?.stdin,