child_process: validate options.shell and correctly enforce shell invocation in exec/execSync#54623
Closed
Renegade334 wants to merge 3 commits intonodejs:mainfrom
Closed
child_process: validate options.shell and correctly enforce shell invocation in exec/execSync#54623Renegade334 wants to merge 3 commits intonodejs:mainfrom
Renegade334 wants to merge 3 commits intonodejs:mainfrom
Conversation
avivkeller
reviewed
Aug 28, 2024
avivkeller
reviewed
Aug 28, 2024
Member
|
CC @nodejs/child_process @Renegade334 can you shorten the commit message, it's too long to land. Something like |
22673fb to
ce02538
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #54623 +/- ##
=======================================
Coverage 88.53% 88.54%
=======================================
Files 657 657
Lines 190741 190746 +5
Branches 36607 36605 -2
=======================================
+ Hits 168881 168899 +18
+ Misses 15036 15028 -8
+ Partials 6824 6819 -5
|
avivkeller
reviewed
Aug 29, 2024
ce02538 to
2288c07
Compare
Collaborator
3cf56e3 to
f61299e
Compare
Member
|
PR is currently blocked from landing due to unreliable CI. Looks like the OSX runner is jammed up. |
Member
Author
|
@jasnell could this go back to request-ci, now that OS X is more stable? |
- narrow validation type to string (previously de facto not validated) - ensure empty string is coerced to true
f61299e to
79e548e
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The intention of exec and execSync is that a shell is always invoked to run the provided command, as opposed to execFile, which by default executes the target directly and does not invoke a shell. Unlike its counterparts elsewhere in child_process, exec's
options.shellis documented as not accepting a boolean, as it's not intended to be possible to disable shell invocation and switch to execFile-like behaviour.exec's
options.shellis currently handled the following way:normalizeExecArgs()shellparameter is not a string, it is coerced to booleantruenormalizeSpawnArguments()string|booleanspawn()runs a shell; if not, it executes the target directlyThe intention is clearly to force shell invocation with exec/execSync in all cases, but the current implementation has the following problems:
true, which passes downstream validation innormalizeSpawnArguments(). In other words, passing an invalid value tooptions.shellwill never raise a validation error.options.shellset to the empty string:normalizeExecArgs()normalizeSpawnArguments()normalizeSpawnArguments()fails to set up the shellcommandstring:This patch makes two simple changes. Firstly, it validates the shell option sent to exec/execSync as a string. Secondly, it coerces all falsy values to boolean
true, so will never bypass shell setup. (This makes{ shell: '' }equivalent to{ shell: undefined }, which matches how it behaves in the other child_process functions.)