diff --git a/packages/cli-kit/src/public/node/system.test.ts b/packages/cli-kit/src/public/node/system.test.ts index f61a9318991..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,70 @@ 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 () => { + // 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.') + }) + + 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) }) }) diff --git a/packages/cli-kit/src/public/node/system.ts b/packages/cli-kit/src/public/node/system.ts index a532dce70a0..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,8 +206,13 @@ export async function execCommand(command: string, options?: ExecOptions): Promi env.FORCE_COLOR = '1' } const executionCwd = options?.cwd ?? cwd() + 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,