GeezerVison is showing github.com — fix(@angular/build): escape prerender redirect URLs by SkyZeroZx · Pull Request #33248 · angular/angular-cli
Conversation
SkyZeroZx
commented
May 24, 2026
•
edited
Loading
Uh oh!
There was an error while loading. Please reload this page.
Escape redirect targets before embedding them in the meta refresh tag and the fallback link of generated static redirect pages.
The manifest serialization changes were moved to #34493.
Fixes #34474
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
gemini-code-assist[bot] reacted with eyes emoji
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.
Code Review
This pull request introduces HTML escaping for redirect URLs in static pages to prevent HTML injection vulnerabilities. It adds an escapeHtml utility and updates the generateRedirectStaticPage function to apply this escaping to both the meta refresh tag and the fallback link. Additionally, E2E tests have been included to verify the fix. The reviewer pointed out that while HTML escaping prevents tag injection, it does not protect against malicious URI schemes like javascript:, and recommended adding protocol validation for the redirect URL.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
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.
I have reservations about this change.
It assumes the attacker can already modify the source code on your machine or alter the database if the routes are built dynamically. If an attacker already has that level of access, they could inflict far worse damage anyway.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
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.
[Gemini Review]: Thanks for working on this! Securing the static redirect HTML generation and fixing the ESM manifest syntax errors with JSON.stringify are great fixes.
However, there are several architectural and DX issues with the current implementation that need to be addressed before we can merge:
- DX Breaking Change in
getPrerenderParams: Rejecting characters like ', ", and spaces on individual parameter values will break existing applications prerendering slugs like { id: "customer's-choice" } or search terms with spaces. In fact, RouteTree.getPathSegments() already calls .map(decodeURIComponent), so percent-encoded values like %27 get immediately decoded back anyway. Instead of validating isolated parameter segments in handlePrerenderParamsReplacement, we should validate and normalize the entire generated route path (routeWithResolvedParams) using the WHATWG new URL parser. This allows legitimate apostrophes to work naturally while automatically percent-encoding spaces and <script> tags into %20 and %3Cscript%3E.
- Move Redirect Validation to Route Extraction:
validateExtractedStaticRedirect is currently placed in prerender.ts in @angular/build. This is too late in the pipeline and misses runtime redirects produced by guards in render-worker.ts. All redirect validation and normalization (redirectTo) should happen upfront during route discovery in @angular/ssr (ng-routes.ts).
- Eliminate Duplication & Leverage Web Standards:
hasUnsafeStaticRedirectCharacters in @angular/build and hasUnsafeUrlCharacters in @angular/ssr duplicate nearly identical regexes and character loops. We can replace these with a unified WHATWG new URL check in @angular/ssr.
- Clean Up Fragmented Error Messages: Error strings are currently spliced across
prerender.ts/ng-routes.ts, utils.ts, and redirect.ts. Centralizing validation in ng-routes.ts allows emitting clean, cohesive error messages.
- E2E vs Unit Tests: The 3 negative build failure assertions in
server-routes-output-mode-static.ts add 3 full CLI builds to CI (~15–30s). These negative validation cases belong in fast in-memory unit tests in ng-routes_spec.ts.
See inline comments for details on each file.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
|
@alan-agius4 I'm sorry to bother you, but if you could review this PR, could you please let me know if there's anything else on my end that needs correcting so I can fix it?
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
|
@alan-agius4 Sorry to bother you, but I think this PR is ready and fixes two potential security issues regarding RCE in SSR and XSS
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
SkyZeroZx
changed the title
fix(@angular/build): escape prerender redirect URLs
Prevent RCE and XSS through unsafe route serialization and redirect handling in Angular SSR
Oct 22, 2026
alan-agius4
left a comment
•
edited
Loading
Uh oh!
There was an error while loading. Please reload this page.
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.
Thanks for working on this.
This PR currently bundles two unrelated changes together:
- Escaping redirect URLs in
generateRedirectStaticPage (packages/angular/build/src/utils/server-rendering/utils.ts).
- Serializing server manifest asset paths and hashing asset chunk filenames (
packages/angular/build/src/utils/server-rendering/manifest.ts).
Could you please split the manifest.ts changes (manifest.ts, manifest_spec.ts, and server-routes-output-mode-server.ts) into a separate PR so we can land that independently? (Also please remove Fixes #34474 from the commit messages as that issue does not exist.)
For the redirect changes in this PR, the @angular/build changes (utils.ts, utils_spec.ts, and server-routes-output-mode-static.ts) look great, but the URL normalization and redirect validation changes in @angular/ssr (redirect.ts and ng-routes.ts) are overly complex and have a few issues:
normalizeAndValidateRedirect is only called from resolveRedirectTo, where the path always starts with / (either redirectTo[0] === '/' or the result of joinUrlParts). Because of that, branches like URL.canParse(target) and ALLOWED_REDIRECT_PROTOCOLS are unreachable, and protocol relative paths (//...) are already collapsed to a single leading / by joinUrlParts in extractRoutesAndCreateRouteTree and app.ts.
- In
normalizeAndValidateRoutePath, running new URL() percent encodes prerendered route paths (for example /produits/café becomes /produits/caf%C3%A9), which is why routeDeduplicationKey had to be added to decode segments again for seenRoutes, and RouteTree.insert immediately runs decodeURIComponent on every segment anyway.
- Validating the composed route path instead of validating parameter values in
handlePrerenderParamsReplacement allows a :param to contain / on nested routes like store/:slug (turning { slug: 'a/b' } into /store/a/b), while rejecting { slug: 'a%2Fb' }.
Please revert the @angular/ssr changes (redirect.ts, redirect_spec.ts, ng-routes.ts, and ng-routes_spec.ts) in this PR so that this PR only contains the generateRedirectStaticPage HTML escaping fix in @angular/build. If we still want parameter validation in handlePrerenderParamsReplacement, we can do that in a separate follow up PR once #34467 lands.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
SkyZeroZx reacted with thumbs up emoji
Escape redirect targets before embedding them in the meta refresh tag and the fallback link of
generated static redirect pages.
SkyZeroZx
changed the title
Prevent RCE and XSS through unsafe route serialization and redirect handling in Angular SSR
fix(@angular/build): escape prerender redirect URLs
Oct 22, 2026
SkyZeroZx
commented
Oct 22, 2026
•
edited
Loading
Uh oh!
There was an error while loading. Please reload this page.
|
@alan-agius4 Thanks for the review. I've done it. (The issue #34474 actually exists; I opened it yesterday)
|
alan-agius4 reacted with thumbs up emoji
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
gemini-code-assist[bot] reacted with eyes emoji
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
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.
Code Review
This pull request introduces HTML escaping for URLs interpolated into static redirect pages (such as meta refresh tags and fallback links) to prevent potential injection vulnerabilities, along with corresponding unit and E2E tests. The review feedback suggests adding a defensive check within the escapeHtml function to gracefully handle non-string or falsy values and prevent potential runtime crashes.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
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.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
This was referenced Oct 23, 2026
Uh oh!
There was an error while loading. Please reload this page.
|
This PR was merged into the repository. The changes were merged into the following branches:
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Reviewers
+1 more reviewer
Reviewers whose approvals may not affect merge requirements
Uh oh!
There was an error while loading. Please reload this page.
Assignees
No one assigned
Milestone
No milestone
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.
|