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.)