child_process should individually escape args[] on shell: true

[Artoria2e5]

• Version: 12.5.0, i don't care, it never worked
• Platform: Windows, but Unix too
• Subsystem: child_process

This is probably a design 🐛, and fixing it would require an incompatible change.

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.

[bnoordhuis]

@Artoria2e5 Do you want to open a PR? It'd be semver-major (i.e., probably won't be released until Node.js v14.x) but it sounds like a reasonable change to me.

[Artoria2e5]

@bnoordhuis Yes, I am working on it. After the test is finished I will open a PR. (They are the referenced commits you see.)

[RedYetiDev]

The linked PR for this was closed several years ago. I'm closing this issue as 'Stale', but feel-free to revisit.