fix(cli-test): invoke commands without shell intermediate (#2582)

Author: zimeg

.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"])
+```

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, unknown>): 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,
   });

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', () => {

packages/cli-test/src/cli/shell.ts

@@ -251,40 +251,19 @@ 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,
   args: string[],
   env: ReturnType<typeof shell.assembleShellEnv>,
   shellOpts?: Partial<child.SpawnOptionsWithoutStdio>,
 ): [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,
     },