-
Notifications
You must be signed in to change notification settings - Fork 30.7k
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.
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
child_process: disallow args in execFile/spawn when shell option is true #57199
base: main
Are you sure you want to change the base?
child_process: disallow args in execFile/spawn when shell option is true #57199
Conversation
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
162ab95
to
e903326
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #57199 +/- ##
==========================================
- Coverage 90.30% 90.27% -0.03%
==========================================
Files 630 630
Lines 184513 184639 +126
Branches 36072 36129 +57
==========================================
+ Hits 166629 166689 +60
- Misses 10967 11023 +56
- Partials 6917 6927 +10
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a semver-major change as it will break people current using the args approach (even being ignored).
I'm not sure if we want to change the API in those situations. I think adding a process.emitWarning could be safer approach in this situation (also semver-major)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's add an entry in deprecation.md
since we're effectively deprecating it
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: #57143