Skip to content

fix(ng-dev): prevent RCE via argument injection in rollup template#3732

Open
josephperrott wants to merge 4 commits into
angular:mainfrom
josephperrott:fix-tmpl-external
Open

fix(ng-dev): prevent RCE via argument injection in rollup template#3732
josephperrott wants to merge 4 commits into
angular:mainfrom
josephperrott:fix-tmpl-external

Conversation

@josephperrott
Copy link
Copy Markdown
Member

@josephperrott josephperrott commented Jun 4, 2026

This PR fixes a critical security vulnerability that allows Remote Code Execution via argument injection in the TMPL_external substitution in the angular_package_format bazel rule.

It safely injects the escaped JSON object directly as a JavaScript array without wrapping it in a parsed string, eliminating the injection vector.

@josephperrott josephperrott requested a review from alan-agius4 June 4, 2026 14:31
@josephperrott josephperrott added the action: merge The PR is ready for merge by the caretaker label Jun 4, 2026
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request aims to secure the Angular package format build rules against code injection vulnerabilities via external module names by encoding the externals list as JSON. The feedback suggests simplifying the implementation: instead of wrapping the template variable in JSON.parse in the Rollup configuration, it can be referenced directly as a JavaScript literal. Doing so ensures safety through json.encode and renders the newly added character validation loop in Starlark redundant, allowing it to be safely removed.

Comment thread bazel/rules/rules_angular/src/ng_package/rollup/rollup.config.js Outdated
Comment thread bazel/rules/rules_angular/src/ng_package/angular_package_format.bzl Outdated
@josephperrott
Copy link
Copy Markdown
Member Author

I have implemented the suggested fixes to address the review comments. TMPL_external is now referenced directly without being wrapped in JSON.parse('...'), safely dropping in the escaped JSON array. The manual character validation loop in the .bzl rule has also been removed since it is no longer needed. Thank you for the review!

@josephperrott
Copy link
Copy Markdown
Member Author

I have moved the malicious test to bazel/rules/rules_angular/test/ng_package_malicious so it sits properly alongside the rule it is testing rather than at the root of the repo.

@josephperrott
Copy link
Copy Markdown
Member Author

I have pushed a fix for the CI failures: I updated the dummy test to be a valid target so it passes the execution phase, and I ran the code formatter across all files to resolve the lint errors. The local test suite is completely green now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

action: merge The PR is ready for merge by the caretaker

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant