diff --git a/.changeset/itchy-buttons-begin.md b/.changeset/itchy-buttons-begin.md new file mode 100644 index 000000000..7fc86c306 --- /dev/null +++ b/.changeset/itchy-buttons-begin.md @@ -0,0 +1,21 @@ +--- +"@slack/cli-test": patch +--- + +fix: invoke commands without shell intermediate + +Behind the scenes commands are now spawned direct to avoid unexpected input and output redirection or odd argument parsings. This is what happens and what changed: + +Linux: + +```diff +- /bin/sh -c "slack trigger run --workflow #/workflows/give_kudos_workflow" ++ execvp("slack", ["trigger", "run", "--workflow", "#/workflows/give_kudos_workflow"]) +``` + +Windows: + +```diff +- cmd.exe /s /c "slack trigger run --workflow #/workflows/give_kudos_workflow" ++ CreateProcessW("slack", ["trigger", "run", "--workflow", "#/workflows/give_kudos_workflow"]) +``` diff --git a/packages/cli-test/src/cli/commands/datastore.ts b/packages/cli-test/src/cli/commands/datastore.ts index e2def3bdd..1387a52da 100644 --- a/packages/cli-test/src/cli/commands/datastore.ts +++ b/packages/cli-test/src/cli/commands/datastore.ts @@ -14,13 +14,6 @@ export interface DatastoreCommandArguments { queryExpressionValues: object; } -/** - * Used to escape double quotes in JSON strings; this is needed when JSON is passed as a command line argument, which for the datastore commands, it is. - */ -function escapeJSON(obj: Record): string { - return `"${JSON.stringify(obj).replace(/"/g, '\\"')}"`; -} - /** * `slack datastore get` * @returns command output @@ -32,7 +25,7 @@ export const datastoreGet = async function datastoreGet( datastore: args.datastoreName, id: args.primaryKeyValue, }; - const cmd = new SlackCLIProcess(['datastore', 'get', escapeJSON(getQueryObj)], args); + const cmd = new SlackCLIProcess(['datastore', 'get', JSON.stringify(getQueryObj)], args); const proc = await cmd.execAsync({ cwd: args.appPath, }); @@ -50,7 +43,7 @@ export const datastoreDelete = async function datastoreDelete( datastore: args.datastoreName, id: args.primaryKeyValue, }; - const cmd = new SlackCLIProcess(['datastore', 'delete', escapeJSON(deleteQueryObj)], args); + const cmd = new SlackCLIProcess(['datastore', 'delete', JSON.stringify(deleteQueryObj)], args); const proc = await cmd.execAsync({ cwd: args.appPath, }); @@ -68,7 +61,7 @@ export const datastorePut = async function datastorePut( datastore: args.datastoreName, item: args.putItem, }; - const cmd = new SlackCLIProcess(['datastore', 'put', escapeJSON(putQueryObj)], args); + const cmd = new SlackCLIProcess(['datastore', 'put', JSON.stringify(putQueryObj)], args); const proc = await cmd.execAsync({ cwd: args.appPath, }); @@ -88,7 +81,7 @@ export const datastoreQuery = async function datastoreQuery( expression: args.queryExpression, expression_values: args.queryExpressionValues, }; - const cmd = new SlackCLIProcess(['datastore', 'query', escapeJSON(queryObj)], args); + const cmd = new SlackCLIProcess(['datastore', 'query', JSON.stringify(queryObj)], args); const proc = await cmd.execAsync({ cwd: args.appPath, }); diff --git a/packages/cli-test/src/cli/shell.test.ts b/packages/cli-test/src/cli/shell.test.ts index 3f71b018a..5282a90e1 100644 --- a/packages/cli-test/src/cli/shell.test.ts +++ b/packages/cli-test/src/cli/shell.test.ts @@ -69,7 +69,7 @@ describe('shell module', () => { runSpy, sinon.match.string, sinon.match.array, - sinon.match({ shell: true, env: fakeEnv }), + sinon.match({ shell: false, env: fakeEnv }), ); }); it('should return the command outputs unchanged', () => { @@ -85,35 +85,19 @@ describe('shell module', () => { shell.runCommandSync('about to explode', []); }, /this is bat country/); }); - if (process.platform === 'win32') { - it('on Windows, should wrap command to shell out in a `cmd /s /c` wrapper process', () => { - const fakeEnv = { HEY: 'yo' }; - sandbox.stub(shell, 'assembleShellEnv').returns(fakeEnv); - const fakeCmd = 'echo'; - const fakeArgs = ['"hi there"']; - shell.runCommandSync(fakeCmd, fakeArgs); - sandbox.assert.calledWithMatch( - runSpy, - 'cmd', - sinon.match.array.contains(['/s', '/c', fakeCmd, ...fakeArgs]), - sinon.match({ shell: true, env: fakeEnv }), - ); - }); - } else { - it('on non-Windows, should shell out to provided command directly', () => { - const fakeEnv = { HEY: 'yo' }; - sandbox.stub(shell, 'assembleShellEnv').returns(fakeEnv); - const fakeCmd = 'echo'; - const fakeArgs = ['"hi there"']; - shell.runCommandSync(fakeCmd, fakeArgs); - sandbox.assert.calledWithMatch( - runSpy, - fakeCmd, - sinon.match.array.contains(fakeArgs), - sinon.match({ shell: true, env: fakeEnv }), - ); - }); - } + it('should spawn without a shell', () => { + const fakeEnv = { HEY: 'yo' }; + sandbox.stub(shell, 'assembleShellEnv').returns(fakeEnv); + const fakeCmd = 'echo'; + const fakeArgs = ['"hi there"']; + shell.runCommandSync(fakeCmd, fakeArgs); + sandbox.assert.calledWithMatch( + runSpy, + fakeCmd, + sinon.match.array.contains(fakeArgs), + sinon.match({ shell: false, env: fakeEnv }), + ); + }); }); describe('spawnProcess method', () => { @@ -128,7 +112,7 @@ describe('shell module', () => { spawnSpy, sinon.match.string, sinon.match.array, - sinon.match({ shell: true, env: fakeEnv }), + sinon.match({ shell: false, env: fakeEnv }), ); }); it('should return the command outputs unchanged', () => { @@ -146,35 +130,19 @@ describe('shell module', () => { shell.spawnProcess('about to explode', []); }, /this is bat country/); }); - if (process.platform === 'win32') { - it('on Windows, should wrap command to shell out in a `cmd /s /c` wrapper process', () => { - const fakeEnv = { HEY: 'yo' }; - sandbox.stub(shell, 'assembleShellEnv').returns(fakeEnv); - const fakeCmd = 'echo'; - const fakeArgs = ['"hi there"']; - shell.spawnProcess(fakeCmd, fakeArgs); - sandbox.assert.calledWithMatch( - spawnSpy, - 'cmd', - sinon.match.array.contains(['/s', '/c', fakeCmd, ...fakeArgs]), - sinon.match({ shell: true, env: fakeEnv }), - ); - }); - } else { - it('on non-Windows, should shell out to provided command directly', () => { - const fakeEnv = { HEY: 'yo' }; - sandbox.stub(shell, 'assembleShellEnv').returns(fakeEnv); - const fakeCmd = 'echo'; - const fakeArgs = ['"hi there"']; - shell.spawnProcess(fakeCmd, fakeArgs); - sandbox.assert.calledWithMatch( - spawnSpy, - fakeCmd, - sinon.match.array.contains(fakeArgs), - sinon.match({ shell: true, env: fakeEnv }), - ); - }); - } + it('should spawn without a shell', () => { + const fakeEnv = { HEY: 'yo' }; + sandbox.stub(shell, 'assembleShellEnv').returns(fakeEnv); + const fakeCmd = 'echo'; + const fakeArgs = ['"hi there"']; + shell.spawnProcess(fakeCmd, fakeArgs); + sandbox.assert.calledWithMatch( + spawnSpy, + fakeCmd, + sinon.match.array.contains(fakeArgs), + sinon.match({ shell: false, env: fakeEnv }), + ); + }); }); describe('waitForOutput method', () => { diff --git a/packages/cli-test/src/cli/shell.ts b/packages/cli-test/src/cli/shell.ts index a07abd077..9e6858fc5 100644 --- a/packages/cli-test/src/cli/shell.ts +++ b/packages/cli-test/src/cli/shell.ts @@ -251,7 +251,7 @@ export const shell = { }; /** - * @description Returns arguments used to pass into child_process.spawn or spawnSync. Handles Windows-specifics hacks. + * @description Returns arguments used to pass into child_process.spawn or spawnSync. */ function getSpawnArguments( command: string, @@ -259,32 +259,11 @@ function getSpawnArguments( env: ReturnType, shellOpts?: Partial, ): [string, string[], child.SpawnOptionsWithoutStdio] { - if (process.platform === 'win32') { - // In windows, we actually spawn a command prompt and tell it to invoke the CLI command. - // The combination of windows and node's child_process spawning is complicated: on windows, child_process strips quotes from arguments. This makes passing JSON difficult. - // As a workaround, we: - // 1. Wrap the CLI command with a Windows Command Prompt (cmd.exe) process, and - // 2. Execute the command to completion (via the /c option), and - // 3. Leave spaces intact (via the /s option), and - // 4. Feed the arguments as an argument array into `child_process.spawn`. - // End-result is a process that looks like: - // cmd.exe "/s" "/c" "slack" "app" "list" - const windowsArgs = ['/s', '/c'].concat([command]).concat(args); - return [ - 'cmd', - windowsArgs, - { - shell: true, - env, - ...shellOpts, - }, - ]; - } return [ command, args, { - shell: true, + shell: false, env, ...shellOpts, },