From 7618c6d553826b1ed12387021a2236a7d553452d Mon Sep 17 00:00:00 2001 From: Renegade334 Date: Sat, 31 May 2025 14:01:41 +0100 Subject: [PATCH 1/2] child_process: validate exec's `options.shell` as string --- lib/child_process.js | 8 +++++++- .../test-child-process-exec-any-shells-windows.js | 2 +- test/parallel/test-child-process-exec-shell.js | 12 ++++++++++++ 3 files changed, 20 insertions(+), 2 deletions(-) create mode 100644 test/parallel/test-child-process-exec-shell.js diff --git a/lib/child_process.js b/lib/child_process.js index 21616c69f877ec..9b8a0b34e398aa 100644 --- a/lib/child_process.js +++ b/lib/child_process.js @@ -197,7 +197,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, diff --git a/test/parallel/test-child-process-exec-any-shells-windows.js b/test/parallel/test-child-process-exec-any-shells-windows.js index 5c34bc77308cc3..3db563c96f1c27 100644 --- a/test/parallel/test-child-process-exec-any-shells-windows.js +++ b/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'); diff --git a/test/parallel/test-child-process-exec-shell.js b/test/parallel/test-child-process-exec-shell.js new file mode 100644 index 00000000000000..8830189b4bb908 --- /dev/null +++ b/test/parallel/test-child-process-exec-shell.js @@ -0,0 +1,12 @@ +'use strict'; +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); +} From b3322f3b4d752341e6962bec4bedbc70124de09c Mon Sep 17 00:00:00 2001 From: Renegade334 Date: Sat, 31 May 2025 15:07:54 +0100 Subject: [PATCH 2/2] child_process: deprecate passing `options.shell` as empty string --- doc/api/deprecations.md | 25 +++++++++++++++++++ lib/child_process.js | 22 +++++++++++----- .../parallel/test-child-process-exec-shell.js | 7 ++++++ 3 files changed, 48 insertions(+), 6 deletions(-) diff --git a/doc/api/deprecations.md b/doc/api/deprecations.md index 7bfd06e6997889..18bb8c26dc73a7 100644 --- a/doc/api/deprecations.md +++ b/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 + + + +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 diff --git a/lib/child_process.js b/lib/child_process.js index 9b8a0b34e398aa..c86c772155c472 100644 --- a/lib/child_process.js +++ b/lib/child_process.js @@ -46,6 +46,7 @@ const { SymbolDispose, } = primordials; +const { getOptionValue } = require('internal/options'); const { assignFunctionName, convertToValidSignal, @@ -543,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'); @@ -592,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. @@ -618,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 ' + diff --git a/test/parallel/test-child-process-exec-shell.js b/test/parallel/test-child-process-exec-shell.js index 8830189b4bb908..72146bd53da2b7 100644 --- a/test/parallel/test-child-process-exec-shell.js +++ b/test/parallel/test-child-process-exec-shell.js @@ -1,4 +1,6 @@ +// Flags: --pending-deprecation 'use strict'; +const common = require('../common'); const assert = require('assert'); const { exec, execSync } = require('child_process'); @@ -10,3 +12,8 @@ const invalidArgTypeError = { 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());