Repository navigation
Disallow args in child_process execFile/spawn when the shell option is true #57143
Description
Activity
What behavior do you think it should have? Should it throw an error when the second argument isn't empty?
Since spawn/execFile already allow omitting
args, they should throw if it is provided.What kind of error should it throw?
- added a commit that references this issue
on Mar 21, 2025 - added 2 commits that reference this issue
on May 1, 2025 This is an outrageous change. There are applications that rely on being able to pass arguments to commands with the shell option is true. In fact, it's the only way to invoke npm on Windows in recent versions of Node.js. Yes, it does require extra care to escape arguments properly, but that is the responsibility of the developer using the API. Forbidding this cripples the whole functionality of the execFile and spawn.
Reacted by StuIt's not forbidden. Instead of:
execFileSync('npm', ['install'], { shell: true })
do:
execFileSync('npm install', { shell: true })
In fact, it's the only way to invoke npm on Windows in recent versions of Node.js.
I created batspawn for this purpose to safely run commands on Windows without requiring setting
shell: true.The API has clearly stated up to this point that that arguments are not further escaped when shell is true. There's no justification for taking this feature away and forcing the use of a command string that embeds the arguments. That's just petty and it breaks code unnecessarily.
I, too, developed a library that safely runs commands on Windows, and now that library and everything that depends on it is now broken.
This change is going to require a significant rewrite to one of my projects. While exec doesn't accept args, spawn does so asking the users to carefully perform string concatenation rather than doing it automatically behind the scenes doesn't really improve security.
Reacted by Dan Allen
The
execFileandspawnfunctions allow passing the shell option to run a command using a shell. Despite the fact that setting this option to true means that arguments are no longer properly preserved, these functions continue to accept an array of arguments, giving the false impression that there is some isolation/escaping when behind the scenes the arguments are just concatenated. This can make it trivial to introduce bugs and security issues, and the behavior is also not aligned withexecwhich only accepts a single command string that is passed to the shell. To make this point clearer, invocations like this are currently accepted, which shouldn't be the case: