Repository: nodejs/node
Pull request: 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: https://github.com/nodejs/node/issues/57143


Changed files:
filename: doc/api/deprecations.md
patch: @@ -3859,13 +3859,16 @@ deprecated, as their values are guaranteed to be identical to that of `process.f
 
 <!-- YAML
 changes:
+  - version: REPLACEME
+    pr-url: https://github.com/nodejs/node/pull/57199
+    description: Runtime deprecation.
   - version:
     - REPLACEME
     pr-url: https://github.com/nodejs/node/pull/57389
     description: Documentation-only deprecation.
 -->
 
-Type: Documentation-only
+Type: Runtime
 
 When an `args` array is passed to [`child_process.execFile`][] or [`child_process.spawn`][] with the option
 `{ shell: true }`, the values are not escaped, only space-separated, which can lead to shell injection.

filename: lib/child_process.js
patch: @@ -535,6 +535,7 @@ function copyProcessEnvToEnv(env, name, optionEnv) {
   }
 }
 
+let emittedDEP0190Already = false;
 function normalizeSpawnArguments(file, args, options) {
   validateString(file, 'file');
   validateArgumentNullCheck(file, 'file');
@@ -611,6 +612,14 @@ function normalizeSpawnArguments(file, args, options) {
 
   if (options.shell) {
     validateArgumentNullCheck(options.shell, 'options.shell');
+    if (args.length > 0 && !emittedDEP0190Already) {
+      process.emitWarning(
+        'Passing args to a child process with shell option true can lead to security ' +
+        'vulnerabilities, as the arguments are not escaped, only concatenated.',
+        'DeprecationWarning',
+        'DEP0190');
+      emittedDEP0190Already = true;
+    }
     const command = ArrayPrototypeJoin([file, ...args], ' ');
     // Set the shell, switches, and commands.
     if (process.platform === 'win32') {

filename: test/parallel/test-child-process-execfile.js
patch: @@ -12,6 +12,12 @@ const fixture = fixtures.path('exit.js');
 const echoFixture = fixtures.path('echo.js');
 const execOpts = { encoding: 'utf8', shell: true, env: { ...process.env, NODE: process.execPath, FIXTURE: fixture } };
 
+common.expectWarning(
+  'DeprecationWarning',
+  'Passing args to a child process with shell option true can lead to security ' +
+  'vulnerabilities, as the arguments are not escaped, only concatenated.',
+  'DEP0190');
+
 {
   execFile(
     process.execPath,

filename: test/parallel/test-child-process-spawn-shell.js
patch: @@ -18,6 +18,12 @@ doesNotExist.on('exit', common.mustCall((code, signal) => {
 }));
 
 // Verify that passing arguments works
+common.expectWarning(
+  'DeprecationWarning',
+  'Passing args to a child process with shell option true can lead to security ' +
+  'vulnerabilities, as the arguments are not escaped, only concatenated.',
+  'DEP0190');
+
 const echo = cp.spawn('echo', ['foo'], {
   encoding: 'utf8',
   shell: true

filename: test/parallel/test-child-process-spawnsync-shell.js
patch: @@ -19,6 +19,12 @@ else
   assert.strictEqual(doesNotExist.status, 127);  // Exit code of /bin/sh
 
 // Verify that passing arguments works
+common.expectWarning(
+  'DeprecationWarning',
+  'Passing args to a child process with shell option true can lead to security ' +
+  'vulnerabilities, as the arguments are not escaped, only concatenated.',
+  'DEP0190');
+
 internalCp.spawnSync = common.mustCall(function(opts) {
   assert.strictEqual(opts.args[opts.args.length - 1].replace(/"/g, ''),
                      'echo foo');