child_process: deprecate passing args to spawn and execFile

Accepting `args` 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.

PR-URL: #57199
Fixes: #57143
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: James M Snell <[email protected]>

Author: DanielVenable

doc/api/deprecations.md

@@ -3856,13 +3856,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.

lib/child_process.js

@@ -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') {

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

@@ -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,

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

@@ -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

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

@@ -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');