child_process: disallow args in execFile/spawn when shell option is true

This will make it throw an error when args are passed to execFile or
spawn when the shell option is true. The reason for this is that when it
accepts args, it gives the false impression that the args are escaped while really they are just concatenated. This makes it easy to introduce bugs and security vulnerabilities.

This will break any code that relies on passing args to execFile or
spawn with `{ shell: true }`.

Fixes: #57143

Author: DanielVenable

doc/api/child_process.md

@@ -158,7 +158,7 @@ exec('my.bat', (err, stdout, stderr) => {
 });
 
 // Script with spaces in the filename:
-const bat = spawn('"my script.cmd"', ['a', 'b'], { shell: true });
+const bat = spawn('"my script.cmd" a b', { shell: true });
 // or:
 exec('"my script.cmd" a b', (err, stdout, stderr) => {
   // ...

lib/child_process.js

@@ -611,7 +611,10 @@ function normalizeSpawnArguments(file, args, options) {
 
   if (options.shell) {
     validateArgumentNullCheck(options.shell, 'options.shell');
-    const command = ArrayPrototypeJoin([file, ...args], ' ');
+    if (args.length > 0) {
+      throw new ERR_INVALID_ARG_VALUE('args', args, 'must be empty when shell option is true');
+    }
+    const command = file;
     // Set the shell, switches, and commands.
     if (process.platform === 'win32') {
       if (typeof options.shell === 'string')

test/parallel/test-child-process-disallow-args-with-shell-true.js

@@ -0,0 +1,16 @@
+'use strict';
+
+// This test ensures that execFile and spawn do not take any arguments when
+// shell option is set to true, as this can lead to bugs and security issues.
+// See https://github.com/nodejs/node/issues/57143
+
+const assert = require('assert');
+const { execFile, execFileSync, spawn, spawnSync } = require('child_process');
+
+const args = ['echo "hello', ['world"'], { shell: true }];
+const err = { code: 'ERR_INVALID_ARG_VALUE' };
+
+assert.throws(() => execFileSync(...args), err);
+assert.throws(() => spawnSync(...args), err);
+assert.throws(() => execFile(...args), err);
+assert.throws(() => spawn(...args), err);

test/parallel/test-child-process-execfile.js

@@ -47,10 +47,9 @@ const execOpts = { encoding: 'utf8', shell: true, env: { ...process.env, NODE: p
 {
   // Verify the shell option works properly
   execFile(
-    `"${common.isWindows ? execOpts.env.NODE : '$NODE'}"`,
-    [`"${common.isWindows ? execOpts.env.FIXTURE : '$FIXTURE'}"`, 0],
+    common.isWindows ? `"${execOpts.env.NODE}" "${execOpts.env.FIXTURE} 0` : `"$NODE" "$FIXTURE" 0`,
     execOpts,
-    common.mustSucceed(),
+    common.mustSucceed()
   );
 }
 
@@ -117,10 +116,14 @@ const execOpts = { encoding: 'utf8', shell: true, env: { ...process.env, NODE: p
     ...(common.isWindows ? [] : [{ encoding: 'utf8' }]),
     { shell: true, encoding: 'utf8' },
   ].forEach((options) => {
-    const execFileSyncStdout = execFileSync(file, args, options);
+    const command = options.shell
+      ? [[file, ...args].join(' ')]
+      : [file, args];
+
+    const execFileSyncStdout = execFileSync(...command, options);
     assert.strictEqual(execFileSyncStdout, `foo bar${os.EOL}`);
 
-    execFile(file, args, options, common.mustCall((_, stdout) => {
+    execFile(...command, options, common.mustCall((_, stdout) => {
       assert.strictEqual(stdout, execFileSyncStdout);
     }));
   });

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

@@ -17,22 +17,6 @@ doesNotExist.on('exit', common.mustCall((code, signal) => {
     assert.strictEqual(code, 127);  // Exit code of /bin/sh
 }));
 
-// Verify that passing arguments works
-const echo = cp.spawn('echo', ['foo'], {
-  encoding: 'utf8',
-  shell: true
-});
-let echoOutput = '';
-
-assert.strictEqual(echo.spawnargs[echo.spawnargs.length - 1].replace(/"/g, ''),
-                   'echo foo');
-echo.stdout.on('data', (data) => {
-  echoOutput += data;
-});
-echo.on('close', common.mustCall((code, signal) => {
-  assert.strictEqual(echoOutput.trim(), 'foo');
-}));
-
 // Verify that shell features can be used
 const cmd = 'echo bar | cat';
 const command = cp.spawn(cmd, {

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

@@ -18,17 +18,6 @@ if (common.isWindows)
 else
   assert.strictEqual(doesNotExist.status, 127);  // Exit code of /bin/sh
 
-// Verify that passing arguments works
-internalCp.spawnSync = common.mustCall(function(opts) {
-  assert.strictEqual(opts.args[opts.args.length - 1].replace(/"/g, ''),
-                     'echo foo');
-  return oldSpawnSync(opts);
-});
-const echo = cp.spawnSync('echo', ['foo'], { shell: true });
-internalCp.spawnSync = oldSpawnSync;
-
-assert.strictEqual(echo.stdout.toString().trim(), 'foo');
-
 // Verify that shell features can be used
 const cmd = 'echo bar | cat';
 const command = cp.spawnSync(cmd, { shell: true });