child_process: escape args[] for shell

BREAKING CHANGE: This changes the behavior of args[] in `shell: true` to
escape globs and other metacharacters. If you want to keep your scripts,
do it the proper way and stuff them in "command."

Refs: nodejs#29576
Fixes: nodejs#29532

doc/api/child_process.md

@@ -469,8 +469,10 @@ changes:
     See [Advanced serialization][] for more details. **Default:** `'json'`.
   * `shell` {boolean|string} If `true`, runs `command` inside of a shell. Uses
     `'/bin/sh'` on Unix, and `process.env.ComSpec` on Windows. A different
-    shell can be specified as a string. See [Shell requirements][] and
-    [Default Windows shell][]. **Default:** `false` (no shell).
+    shell can be specified as a string. Unlike `command`, `args` will be quoted
+    and no metacharacters will be effective inside of it. See
+    [Shell Requirements][] and [Default Windows Shell][]. **Default:** `false`
+    (no shell).
   * `windowsVerbatimArguments` {boolean} No quoting or escaping of arguments is
     done on Windows. Ignored on Unix. This is set to `true` automatically
     when `shell` is specified and is CMD. **Default:** `false`.
@@ -818,8 +820,10 @@ changes:
     normally be created on Windows systems. **Default:** `false`.
   * `shell` {boolean|string} If `true`, runs `command` inside of a shell. Uses
     `'/bin/sh'` on Unix, and `process.env.ComSpec` on Windows. A different
-    shell can be specified as a string. See [Shell requirements][] and
-    [Default Windows shell][]. **Default:** `false` (no shell).
+    shell can be specified as a string. Unlike `command`, `args` will be quoted
+    and no metacharacters will be effective inside of it. See
+    [Shell Requirements][] and [Default Windows Shell][]. **Default:** `false`
+    (no shell).
 * Returns: {Buffer|string} The stdout from the command.
 
 The `child_process.execFileSync()` method is generally identical to
@@ -949,8 +953,10 @@ changes:
     **Default:** `'buffer'`.
   * `shell` {boolean|string} If `true`, runs `command` inside of a shell. Uses
     `'/bin/sh'` on Unix, and `process.env.ComSpec` on Windows. A different
-    shell can be specified as a string. See [Shell requirements][] and
-    [Default Windows shell][]. **Default:** `false` (no shell).
+    shell can be specified as a string. Unlike `command`, `args` will be quoted
+    and no metacharacters will be effective inside of it. See
+    [Shell Requirements][] and [Default Windows Shell][]. **Default:** `false`
+    (no shell).
   * `windowsVerbatimArguments` {boolean} No quoting or escaping of arguments is
     done on Windows. Ignored on Unix. This is set to `true` automatically
     when `shell` is specified and is CMD. **Default:** `false`.
@@ -1640,15 +1646,57 @@ The shell should understand the `-c` switch. If the shell is `'cmd.exe'`, it
 should understand the `/d /s /c` switches and command-line parsing should be
 compatible.
 
-## Default Windows shell
+If `shell` and `args` are both specified, the shell should understand
+[POSIX single-quotes][]. If the shell is `'cmd.exe'`, it should understand the
+double-quoting used by Windows `cmd`. If the shell is `'powershell.exe'` or
+`'pwsh'`, it should understand a powershell-compatible single quoting.
+
+Since the `command` part is literally passed to the shell, you will be
+responsible for any escaping you perform. The specific way to call a file
+as a command-line program depends on the shell:
+
+* With POSIX shell, you quote the path using the usual single-quotes. You
+  may want to prefix it with `command --` or `env --` to suppress syntax,
+  function, and/or alias lookups. You can simply use `'command --'` as your
+  `command` argument and put the path on the first element of `args`.
+* With PowerShell, it is often needed to prefix the command with `&` when the
+  executable path is quoted. You can use a similar technique with `command`
+  set to `'&'`.
+* With CMD, you should wrap double quotes around the paths to the executable.
+  Do NOT escape anything inside of it.
+
+As a caveat for `'cmd'`, `args` should additionally not contain any newlines
+because `cmd` cannot handle such. See also [Windows Command Line][] for when
+else not to rely on the builtin escaping mechanism on Windows.
+
+In addition, PowerShell has an [ongoing, cross-platform issue][] with how it
+translates the `argv` for external programs to a Windows-style command-line
+string. Until it is fixed, the built-in PowerShell escape provided by Node.js
+is guaranteed to work for built-in commands only.
+
+## Default Windows Shell
 
 Although Microsoft specifies `%COMSPEC%` must contain the path to
 `'cmd.exe'` in the root environment, child processes are not always subject to
 the same requirement. Thus, in `child_process` functions where a shell can be
 spawned, `'cmd.exe'` is used as a fallback if `process.env.ComSpec` is
 unavailable.
 
-## Advanced serialization
+## Windows Command Line
+
+There is no universal way in Windows to do command-line escapes as every
+program are exposed natively to its full cmdline as a string, and each of them
+can have their own parsing rules, which includes optional glob expansion,
+officially provided as `_setargv`.
+
+The most common parsing is defined in the C runtime library as
+`CommandLineToArgvW`. Microsoft describes the rules in
+[Parsing C++ Command-Line Arguments][]. When any other rules are known to be
+used, `windowsVerbatimArguments` should be specified with manual argument
+quoting. Since `cmd` has a `/s` switch that allows for verbatim processing
+of the command string, we turn on the flag to take advantage of that feature.
+
+## Advanced Serialization
 <!-- YAML
 added:
  - v13.2.0
@@ -1704,6 +1752,13 @@ or [`child_process.fork()`][].
 [`subprocess.stdin`]: #child_process_subprocess_stdin
 [`subprocess.stdio`]: #child_process_subprocess_stdio
 [`subprocess.stdout`]: #child_process_subprocess_stdout
-[`util.promisify()`]: util.md#util_util_promisify_original
+[`util.promisify()`]: util.html#util_util_promisify_original
+[Default Windows Shell]: #child_process_default_windows_shell
+[HTML structured clone algorithm]: https://developer.mozilla.org/en-US/docs/Web/API/Web_Workers_API/Structured_clone_algorithm
+[Shell Requirements]: #child_process_shell_requirements
+[POSIX single-quotes]: https://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#tag_18_02_02
+[Parsing C++ Command-Line Arguments]: https://docs.microsoft.com/en-us/cpp/cpp/parsing-cpp-command-line-arguments
+[Windows Command Line]: #child_process_windows_command_line
+[ongoing, cross-platform issue]: https://github.com/PowerShell/PowerShell/issues/1995
 [synchronous counterparts]: #child_process_synchronous_process_creation
 [v8.serdes]: v8.md#v8_serialization_api

lib/child_process.js

@@ -527,29 +527,40 @@ function normalizeSpawnArguments(file, args, options) {
   }
 
   if (options.shell) {
-    const command = ArrayPrototypeJoin([file, ...args], ' ');
-    // Set the shell, switches, and commands.
-    if (process.platform === 'win32') {
-      if (typeof options.shell === 'string')
-        file = options.shell;
-      else
-        file = process.env.comspec || 'cmd.exe';
-      // '/d /s /c' is used only for cmd.exe.
-      if (RegExpPrototypeTest(/^(?:.*\\)?cmd(?:\.exe)?$/i, file)) {
-        args = ['/d', '/s', '/c', `"${command}"`];
-        windowsVerbatimArguments = true;
-      } else {
-        args = ['-c', command];
-      }
-    } else {
-      if (typeof options.shell === 'string')
-        file = options.shell;
-      else if (process.platform === 'android')
-        file = '/system/bin/sh';
-      else
-        file = '/bin/sh';
-      args = ['-c', command];
+    const command = file;
+    const commandArgs = args.map((i) => i + '');
+    args = ['-c'];
+    let quote = quotePosixShArg;
+    let isCmd = false;
+    let testWindowsExe = (_c, _f) => false;
+    // Set the shell.
+    if (typeof options.shell === 'string')
+      file = options.shell;
+    else if (process.platform === 'win32') {
+      file = process.env.comspec || 'cmd.exe';
+      testWindowsExe = (cmd, file) =>
+        new RegExp(`^(?:.*\\\\)?${cmd}(?:\\.exe)?$`, 'i').test(file);
+    } else if (process.platform === 'android')
+      file = '/system/bin/sh';
+    else
+      file = '/bin/sh';
+
+    // Handle special args for special shells.
+    // '/d /s /c' is used only for cmd.exe.
+    if (testWindowsExe('cmd', file)) {
+      args = ['/d', '/s', '/c'];
+      quote = quoteCmdArg;
+      isCmd = windowsVerbatimArguments = true;
+    } else if (
+      testWindowsExe('(powershell|pwsh)', file) ||
+      file.endsWith('/pwsh')
+    ) {
+      quote = quotePwshArg;
     }
+
+    // Cmd accepts quoted things verbatim under /s.
+    const commandArg = [command].concat(commandArgs.map(quote)).join(' ');
+    args.push(isCmd ? `"${commandArg}"` : commandArg);
   }
 
   if (typeof options.argv0 === 'string') {
@@ -775,6 +786,28 @@ function spawnWithSignal(file, args, options) {
   }
   return child;
 }
+
+
+function quotePosixShArg(arg) {
+  return `'${arg.replace(/'/g, "'\\''")}'`;
+}
+
+
+function quoteCmdArg(arg) {
+  // We don't do the verbatim case here, because we need to quote
+  // (but not escape) stuff like & and | too, and that's a long list
+  // to check for on indexOf()
+  return `"${arg.replace(/\\/g, '\\\\').replace(/"/g, '\\"')}"`;
+}
+
+
+function quotePwshArg(arg) {
+  // We need to specifically check for powershell now; it does -c but does
+  // not quote in the same way
+  return `'${arg.replace(/'/g, "''")}'`;
+}
+
+
 module.exports = {
   _forkChild,
   ChildProcess,

test/parallel/test-child-process-spawn-shell.js

@@ -24,7 +24,7 @@ const echo = cp.spawn('echo', ['foo'], {
 });
 let echoOutput = '';
 
-assert.strictEqual(echo.spawnargs[echo.spawnargs.length - 1].replace(/"/g, ''),
+assert.strictEqual(echo.spawnargs[echo.spawnargs.length - 1].replace(/['"]/g, ''),
                    'echo foo');
 echo.stdout.on('data', (data) => {
   echoOutput += data;

test/parallel/test-child-process-spawnsync-shell-args.js

@@ -0,0 +1,79 @@
+'use strict';
+const common = require('../common');
+const { ok: mkay, strictEqual: same } = require('assert');
+const cp = require('child_process');
+
+// This test ensures that child_process.spawnSync escapes shell args as
+// required since #29532.
+const run = (shell, command, args, ok = mkay) => {
+  // eslint-disable-next-line prefer-const
+  let { stdout, stderr, error, status } = cp.spawnSync(command, args, {
+    shell: shell,
+  });
+  [stdout, stderr] = [stdout, stderr].map((i) => i.toString());
+  ok(!error && !stderr, [error, status, stderr, stdout].join(' ||| '));
+  return common.isWindows ? stdout.replace(/\r/g, '') : stdout;
+};
+
+// find bash
+const silent = () => {};
+let bashPath = '';
+if (common.isWindows) {
+  const bashPaths = run(true, 'where bash', null, silent)
+    .trim()
+    .split(/[\n]+/g);
+  // We may have multiple results. Pick the one that looks right.
+  for (const i of bashPaths) {
+    // Not you, Bash on Windows stub.
+    // (It parses the cmdline... quite differently.)
+    if (i.indexOf('Windows\\System32') < 0) {
+      bashPath = i;
+      break;
+    }
+  }
+} else {
+  bashPath = run(true, 'command -v bash', null, silent).trim();
+}
+
+if (!bashPath) common.skip('Skipping test because bash is not found.');
+else console.info(`Using bash from ${bashPath}.`);
+
+const reprint = "printf '%q '";
+const testArgs = ['  Lore"m  Ipsu""m\ndolor\tsit', 'on \\a\x07 ringing chair.'];
+const testArgsLite = ['"Hello \\"World|&*', "y'all"];
+const reprintArgs = ['-c', `${reprint} "$@"`, '--', ...testArgs];
+const nodeEcho = [
+  '-e',
+  'process.argv.slice(1).forEach((x) => console.log(x))',
+  '--',
+];
+
+const echoOut = testArgs.concat(['']).join('\n');
+same(run(bashPath, 'printf "%s\n"', testArgs), echoOut);
+same(run(false, process.execPath, nodeEcho.concat(testArgs)), echoOut);
+
+const echoOutLite = testArgsLite.concat(['']).join('\n');
+same(run(bashPath, 'printf "%s\n"', testArgsLite), echoOutLite);
+
+const reprintOut = run(bashPath, reprint, testArgs);
+same(run(false, 'bash', reprintArgs), reprintOut);
+
+if (common.isWindows) {
+  same(run('powershell', 'Write-Output', testArgs), echoOut);
+
+  // XXX: Here we see where pwsh internal cmdline quoting clashes with actual
+  // Windows cmdline handling. Microsoft, don't you have the source for both?
+  //
+  // Ref: PowerShell/PowerShell#1995
+  // same(
+  //   run('powershell', `& "${process.execPath}"`, nodeEcho.concat(testArgs)),
+  //   echoOut
+  // );
+
+  // Cmd has troubles with newlines, so we lower the bar
+  // (not using 'echo' because cmd handles builtin parsing differently)
+  same(
+    run('cmd', `"${process.execPath}"`, nodeEcho.concat(testArgsLite)),
+    echoOutLite
+  );
+}

test/parallel/test-child-process-spawnsync-shell.js

@@ -20,7 +20,7 @@ else
 
 // Verify that passing arguments works
 internalCp.spawnSync = common.mustCall(function(opts) {
-  assert.strictEqual(opts.args[opts.args.length - 1].replace(/"/g, ''),
+  assert.strictEqual(opts.args[opts.args.length - 1].replace(/["']/g, ''),
                      'echo foo');
   return oldSpawnSync(opts);
 });