child_process should individually escape args[] on shell: true
- Version: 12.5.0, i don't care, it never worked
- Platform: Windows, but Unix too
- Subsystem: child_process
This is probably a design
The shell: true
handling of child_process
joins the contents of args[]
with spaces and simply sends them off to the shell. Although it is documented that shell: true
is for handling globs and the like, doing so is still inconsiderate and irresponsible because devs expect some degree of safety by putting stuff in an array instead of one long string.
The problem? Well, shell: true
is mandatory for running many of the .cmd
shims on Windows because these things are not otherwise considered executable, so at least shell: process.platform === 'win32'
is needed. And although cmd
does not interpret globs, it does interpret flow control stuff like &
and &&
. Our linter script, set up with lint-staged
, faced exactly that: someone threw in a test script with &
in the filename for it to lint, and the command chopped off at that ampersand because you broke the argument separation. Yes, programming code filenames should not look like that, but neither should a spawning mechanism break!
My proposal for the issue is that nodejs
should escape the contents of args
indivudually (map
) before joining them into a /c
or -c
parameter of the shell, and that the command
string can instead stay as the wild-west part that people use to put complex shell-only stuff in. In other words, if command
contains nothing special, spawning with shell
set true or false should eventually run the exact same command, the only difference being whether another shell has been started (so things like BASH_ENV will still matter).
Implementation matters
Currently the Windows escape is written in C++ as quote_cmd_arg
. Unix does not require escaping yet as its spawning mechanism already accepts an array of strings. Implementing my suggestion should mean that we should have a version of both in JavaScript:
function quote_sh_arg(arg){
return `'${arg.replace("'", "'\\''")}'`
}
function quote_cmd_arg(arg) {
// we don't do the verbatim case here, because we need to quote
// (but not escape) stuff like & and | too, and that's a long list
// to check for on indexOf()
return `"${arg.replace('\\', '\\\\').replace('"', '\\"')}"`
}
function quote_pwsh_arg(arg) {
// we need to specifically check for powershell now; it does -c but does
// not quote in the same way
return `'${arg.replace("'", "''")}'`
}
Oh that's right, we need PowerShell too. The whole shell checking will have to move a bit as well as they must be identified before quoting.