child_process: validate options.shell and correctly enforce shell invocation in exec/execSync

[Renegade334]

The intention of exec and execSync is that a shell is always invoked to run the provided command, as opposed to execFile, which by default executes the target directly and does not invoke a shell. Unlike its counterparts elsewhere in child_process, exec's options.shell is documented as not accepting a boolean, as it's not intended to be possible to disable shell invocation and switch to execFile-like behaviour.

exec's options.shell is currently handled the following way:

• passed to normalizeExecArgs()
  • if the shell parameter is not a string, it is coerced to boolean true
• passed to normalizeSpawnArguments()
  • if non-nullish, the parameter is validated as being of type string|boolean
  • if the parameter is truthy at this point, then spawn() runs a shell; if not, it executes the target directly

The intention is clearly to force shell invocation with exec/execSync in all cases, but the current implementation has the following problems:

• The parameter is not validated at all; instead, if it's not a string, it's silently ignored and coerced to true, which passes downstream validation in normalizeSpawnArguments(). In other words, passing an invalid value to options.shell will never raise a validation error.
• The logic in the normalize functions combines to yield an edge case, which is when exec/execSync is called with options.shell set to the empty string:
  • the parameter is passed through unchanged by normalizeExecArgs()
  • the parameter passes validation in normalizeSpawnArguments()
  • the parameter is not truthy, so normalizeSpawnArguments() fails to set up the shell
  • the child process is spawned without a shell, with the file target set to the entire command string:

    child_process.exec('./script some-parameter',{shell: ''})// should invoke a shell to run "./script" and pass an argument// instead, attempts to execute a file called "script some-parameter"

This patch makes two simple changes. Firstly, it validates the shell option sent to exec/execSync as a string. Secondly, it coerces all falsy values to boolean true, so will never bypass shell setup. (This makes { shell: '' } equivalent to { shell: undefined }, which matches how it behaves in the other child_process functions.)

[avivkeller]

CC @nodejs/child_process

---

@Renegade334 can you shorten the commit message, it's too long to land. Something like child_process: validate that `shell` is a XYZ

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.54%. Comparing base (98d4ebc) to head (79e548e).
Report is 157 commits behind head on main.

Additional details and impacted files

@@ Coverage Diff @@## main #54623 +/- ##
=======================================
Coverage 88.53% 88.54% =======================================
Files 657 657 Lines 190741 190746 +5 Branches 36607 36605 -2 =======================================
+ Hits 168881 168899 +18 + Misses 15036 15028 -8 + Partials 6824 6819 -5 

Files with missing lines	Coverage Δ
lib/child_process.js	97.74% <100.00%> (+0.01%)	⬆️

... and 41 files with indirect coverage changes

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/62171/

[jasnell]

PR is currently blocked from landing due to unreliable CI. Looks like the OSX runner is jammed up.

[Renegade334]

@jasnell could this go back to request-ci, now that OS X is more stable?