-
-
Notifications
You must be signed in to change notification settings - Fork 33.7k
child_process: disallow args in execFile/spawn when shell option is true #57199
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
Conversation
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.23% 90.22% -0.02%
==========================================
Files 629 629
Lines 184939 184948 +9
Branches 36232 36233 +1
==========================================
- Hits 166885 166870 -15
- Misses 11011 11023 +12
- Partials 7043 7055 +12
🚀 New features to boost your workflow:
|
RafaelGSS
left a comment
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)
aduh95
left a comment
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
|
Thank you @DanielVenable for creating this PR. What's the update on this? It would be good to get this into Node.js 24 as the freeze is in a couple of weeks. |
PR-URL: #57389 Refs: #57199 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Jason Zhang <[email protected]> Reviewed-By: Ulises Gascón <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>
Accepting `args` 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.
c885d04 to
7a04435
Compare
|
@DanielVenable I've rebased to fix the git conflicts. I've also pushed 7a04435 to fix the implementation, PTAL. |
|
/cc @nodejs/tsc since this is semver-major |
|
@aduh95 LGTM |
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