Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Disallow args in child_process execFile/spawn when the shell option is true #57143

Open
mohd-akram opened this issue Feb 20, 2025 · 3 comments · May be fixed by #57199
Open

Disallow args in child_process execFile/spawn when the shell option is true #57143

mohd-akram opened this issue Feb 20, 2025 · 3 comments · May be fixed by #57199
Labels
child_process Issues and PRs related to the child_process subsystem.

Comments

@mohd-akram
Copy link
Contributor

mohd-akram commented Feb 20, 2025 •
edited
Loading

The execFile and spawn functions 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 with exec which 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:

execFileSync('echo "hello', ['world"'], { shell: true }).toString()
@DanielVenable
Copy link
Contributor

What behavior do you think it should have? Should it throw an error when the second argument isn't empty?

@mohd-akram
Copy link
Contributor Author

Since spawn/execFile already allow omitting args, they should throw if it is provided.

@DanielVenable
Copy link
Contributor

What kind of error should it throw?

DanielVenable added a commit to DanielVenable/node that referenced this issue Feb 24, 2025
@DanielVenable
child_process: disallow args in execFile/spawn when shell option is true
162ab95 
This will make it throw an error when args are passed to execFile or
spawn when the shell option is true. The reason for this is that when it
accepts args, it gives the false impression that the args are escaped while really they are just concatenated. This makes it easy to introduce bugs and security vulnerabilities.

This will break any code that relies on passing args to execFile or
spawn with `{ shell: true }`.

Fixes: nodejs#57143
@DanielVenable DanielVenable linked a pull request Feb 24, 2025 that will close this issue
Open
DanielVenable added a commit to DanielVenable/node that referenced this issue Feb 24, 2025
@DanielVenable
child_process: disallow args in execFile/spawn when shell option is true
e903326 
This will make it throw an error when args are passed to execFile or
spawn when the shell option is true. The reason for this is that when it
accepts args, it gives the false impression that the args are escaped
while really they are just concatenated. This makes it easy to introduce
bugs and security vulnerabilities.

This will break any code that relies on passing args to execFile or
spawn with `{ shell: true }`.

Fixes: nodejs#57143
@daeyeon daeyeon added the child_process Issues and PRs related to the child_process subsystem. label Feb 25, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
child_process Issues and PRs related to the child_process subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

child_process: disallow args in execFile/spawn when shell option is true DanielVenable/node
3 participants
@mohd-akram @daeyeon @DanielVenable