child_process: options.shell validation

doc/api/deprecations.md

@@ -3952,6 +3952,30 @@ Type: Documentation-only
 The support for priority signaling has been deprecated in the [RFC 9113][], and
 will be removed in future versions of Node.js.
 
+### DEP0195: Calling `node:child_process` functions with `options.shell` as an empty string
+
+<!-- YAML
+changes:
+  - version: REPLACEME
+    pr-url: https://github.com/nodejs/node/pull/58525
+    description: Documentation-only deprecation
+                 with `--pending-deprecation` support.
+-->
+
+Type: Documentation-only (supports [`--pending-deprecation`][])
+
+Calling the process-spawning functions with `{ shell: '' }` is almost certainly
+unintentional, and can cause aberrant behavior.
+
+To make [`child_process.execFile`][] or [`child_process.spawn`][] invoke the
+default shell, use `{ shell: true }`. If the intention is not to invoke a shell
+(default behavior), either omit the `shell` option, or set it to `false` or a
+nullish value.
+
+To make [`child_process.exec`][] invoke the default shell, either omit the
+`shell` option, or set it to a nullish value. If the intention is not to invoke
+a shell, use [`child_process.execFile`][] instead.
+
 [DEP0142]: #dep0142-repl_builtinlibs
 [NIST SP 800-38D]: https://nvlpubs.nist.gov/nistpubs/Legacy/SP/nistspecialpublication800-38d.pdf
 [RFC 6066]: https://tools.ietf.org/html/rfc6066#section-3
@@ -3980,6 +4004,7 @@ will be removed in future versions of Node.js.
 [`asyncResource.runInAsyncScope()`]: async_context.md#asyncresourceruninasyncscopefn-thisarg-args
 [`buffer.subarray`]: buffer.md#bufsubarraystart-end
 [`child_process.execFile`]: child_process.md#child_processexecfilefile-args-options-callback
+[`child_process.exec`]: child_process.md#child_processexeccommand-options-callback
 [`child_process.spawn`]: child_process.md#child_processspawncommand-args-options
 [`child_process`]: child_process.md
 [`clearInterval()`]: timers.md#clearintervaltimeout

lib/child_process.js

@@ -46,6 +46,7 @@ const {
   SymbolDispose,
 } = primordials;
 
+const { getOptionValue } = require('internal/options');
 const {
   assignFunctionName,
   convertToValidSignal,
@@ -197,7 +198,13 @@ function normalizeExecArgs(command, options, callback) {
 
   // Make a shallow copy so we don't clobber the user's options object.
   options = { __proto__: null, ...options };
-  options.shell = typeof options.shell === 'string' ? options.shell : true;
+
+  // Validate the shell, if present, otherwise request the default shell.
+  if (options.shell != null) {
+    validateString(options.shell, 'options.shell');
+  } else {
+    options.shell = true;
+  }
 
   return {
     file: command,
@@ -537,6 +544,8 @@ function copyProcessEnvToEnv(env, name, optionEnv) {
 }
 
 let emittedDEP0190Already = false;
+let emittedDEP0195Already = false;
+const pendingDeprecation = getOptionValue('--pending-deprecation');
 function normalizeSpawnArguments(file, args, options) {
   validateString(file, 'file');
   validateArgumentNullCheck(file, 'file');
@@ -586,11 +595,19 @@ function normalizeSpawnArguments(file, args, options) {
   }
 
   // Validate the shell, if present.
-  if (options.shell != null &&
-      typeof options.shell !== 'boolean' &&
-      typeof options.shell !== 'string') {
-    throw new ERR_INVALID_ARG_TYPE('options.shell',
-                                   ['boolean', 'string'], options.shell);
+  if (options.shell != null) {
+    if (typeof options.shell !== 'boolean' && typeof options.shell !== 'string') {
+      throw new ERR_INVALID_ARG_TYPE('options.shell',
+                                     ['boolean', 'string'], options.shell);
+    }
+    validateArgumentNullCheck(options.shell, 'options.shell');
+    if (pendingDeprecation && options.shell === '' && !emittedDEP0195Already) {
+      process.emitWarning('Passing an empty string as the shell option when ' +
+                            'calling child_process functions is deprecated.',
+                          'DeprecationWarning',
+                          'DEP0195');
+      emittedDEP0195Already = true;
+    }
   }
 
   // Validate argv0, if present.
@@ -612,7 +629,6 @@ 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 ' +

test/parallel/test-child-process-exec-any-shells-windows.js

@@ -33,7 +33,7 @@ const testCopy = (shellName, shellPath) => {
 const system32 = `${process.env.SystemRoot}\\System32`;
 
 // Test CMD
-test(true);
+test(null);
 test('cmd');
 testCopy('cmd.exe', `${system32}\\cmd.exe`);
 test('cmd.exe');

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

@@ -0,0 +1,19 @@
+// Flags: --pending-deprecation
+'use strict';
+const common = require('../common');
+const assert = require('assert');
+const { exec, execSync } = require('child_process');
+
+const invalidArgTypeError = {
+  code: 'ERR_INVALID_ARG_TYPE',
+  name: 'TypeError'
+};
+
+for (const fn of [exec, execSync]) {
+  assert.throws(() => fn('should-throw-on-boolean-shell-option', { shell: false }), invalidArgTypeError);
+}
+
+const deprecationMessage = 'Passing an empty string as the shell option when calling ' +
+                           'child_process functions is deprecated.';
+common.expectWarning('DeprecationWarning', deprecationMessage, 'DEP0195');
+exec('does-not-exist', { shell: '' }, common.mustCall());