GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — fix(@angular/build): escape prerender redirect URLs by SkyZeroZx · Pull Request #33248 · angular/angular-cli

Conversation

@SkyZeroZx

@SkyZeroZx SkyZeroZx commented May 24, 2026 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

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
All reactions
  • 👀 1 reaction
@SkyZeroZx
SkyZeroZx marked this pull request as ready for review May 25, 2026 00:17
gemini-code-assist[bot]
gemini-code-assist Bot reviewed May 25, 2026
View reviewed changes

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

Copy link
Copy Markdown

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.

All reactions
Comment thread packages/angular/build/src/utils/server-rendering/utils.ts Resolved

Uh oh!

There was an error while loading. Please reload this page.

@SkyZeroZx
SkyZeroZx force-pushed the fix-build branch from db6e7d8 to 21c6f37 Compare May 25, 2026 03:32
alan-agius4
alan-agius4 reviewed May 26, 2026
View reviewed changes

@alan-agius4 alan-agius4 left a comment

Copy link
Copy Markdown
Collaborator

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.

All reactions
alan-agius4
alan-agius4 reviewed May 26, 2026
View reviewed changes
Comment thread packages/angular/build/src/utils/server-rendering/utils.ts Resolved

Uh oh!

There was an error while loading. Please reload this page.

@SkyZeroZx
SkyZeroZx force-pushed the fix-build branch from 21c6f37 to c4f66cc Compare May 26, 2026 17:01
@angular-robot angular-robot Bot added the area: @angular/build label May 26, 2026
@SkyZeroZx
SkyZeroZx requested a review from alan-agius4 May 28, 2026 15:35
@alan-agius4 alan-agius4 mentioned this pull request Aug 24, 2026
fix(@angular/build): serialize server manifest asset paths #33901
Closed
4 of 13 tasks
@SkyZeroZx
SkyZeroZx force-pushed the fix-build branch 2 times, most recently from f26ff1f to fe73b18 Compare August 25, 2026 15:47
SkyZeroZx
SkyZeroZx commented Aug 25, 2026
View reviewed changes
Comment thread packages/angular/build/src/utils/server-rendering/manifest.ts Resolved

Uh oh!

There was an error while loading. Please reload this page.

@SkyZeroZx
SkyZeroZx force-pushed the fix-build branch from fe73b18 to dc14263 Compare August 25, 2026 15:55
alan-agius4
alan-agius4 reviewed Aug 26, 2026
View reviewed changes

@alan-agius4 alan-agius4 left a comment

Copy link
Copy Markdown
Collaborator

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:

  1. 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.
  2. 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).
  3. 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.
  4. 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.
  5. 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.

All reactions
Comment thread packages/angular/ssr/src/routes/ng-routes.ts Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread packages/angular/build/src/utils/server-rendering/prerender.ts Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread packages/angular/build/src/utils/server-rendering/utils.ts Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread tests/e2e/tests/build/server-rendering/server-routes-output-mode-static.ts Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread packages/angular/build/src/utils/server-rendering/manifest.ts Resolved

Uh oh!

There was an error while loading. Please reload this page.

@SkyZeroZx
SkyZeroZx force-pushed the fix-build branch from dc14263 to 15cce9a Compare August 26, 2026 19:11
SkyZeroZx
SkyZeroZx commented Aug 26, 2026
View reviewed changes
Comment thread packages/angular/build/src/utils/server-rendering/manifest.ts Resolved

Uh oh!

There was an error while loading. Please reload this page.

@SkyZeroZx
SkyZeroZx requested a review from alan-agius4 August 27, 2026 11:36
@SkyZeroZx
SkyZeroZx force-pushed the fix-build branch 2 times, most recently from 80c9dd2 to 9566d2b Compare September 3, 2026 16:39
@SkyZeroZx

SkyZeroZx commented Sep 14, 2026

Copy link
Copy Markdown
Contributor Author

@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?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@glivter glivter mentioned this pull request Sep 14, 2026
@angular/ssr: the route matcher and @angular/router disagree, so per-route headers, status and renderMode come from a different route than the one rendered #34090
Closed
@SkyZeroZx
SkyZeroZx force-pushed the fix-build branch from 9566d2b to f695d08 Compare October 5, 2026 15:33
@SkyZeroZx
SkyZeroZx force-pushed the fix-build branch 2 times, most recently from 7e5914c to 571e5b6 Compare October 14, 2026 14:59
@SkyZeroZx
SkyZeroZx force-pushed the fix-build branch from 571e5b6 to 551dcd3 Compare October 20, 2026 04:37
@SkyZeroZx

SkyZeroZx commented Oct 20, 2026

Copy link
Copy Markdown
Contributor Author

@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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@SkyZeroZx
SkyZeroZx force-pushed the fix-build branch 2 times, most recently from 82a8b75 to 1ff762a Compare October 22, 2026 05:37
@SkyZeroZx 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
alan-agius4 requested changes Oct 22, 2026
View reviewed changes

@alan-agius4 alan-agius4 left a comment •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Collaborator

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:

  1. Escaping redirect URLs in generateRedirectStaticPage (packages/angular/build/src/utils/server-rendering/utils.ts).
  2. 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:

  1. 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.
  2. 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.
  3. 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
All reactions
  • 👍 1 reaction
SkyZeroZx added 2 commits October 22, 2026 09:40
@SkyZeroZx
fix(@angular/build): escape prerender redirect URLs
a8c8f77
Escape redirect targets before embedding them in the meta refresh tag and the fallback link of
generated static redirect pages.
@SkyZeroZx
fixup! fix(@angular/build): escape prerender redirect URLs
77668b0
@SkyZeroZx
SkyZeroZx force-pushed the fix-build branch from 1ff762a to 77668b0 Compare October 22, 2026 14:46
@SkyZeroZx 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

SkyZeroZx commented Oct 22, 2026 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor Author

@alan-agius4 Thanks for the review. I've done it. (The issue #34474 actually exists; I opened it yesterday)

  • I just created a new PR for manifest in fix(@angular/build): serialize server manifest asset paths
  • For this PR I reverted the @angular/ssr changes, so this PR now only contains the generateRedirectStaticPage escaping.
alan-agius4 reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@alan-agius4

alan-agius4 commented Oct 22, 2026

Copy link
Copy Markdown
Collaborator

/gemini review

gemini-code-assist[bot] reacted with eyes emoji
All reactions
  • 👀 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

gemini-code-assist[bot]
gemini-code-assist Bot reviewed Oct 22, 2026
View reviewed changes

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

Copy link
Copy Markdown

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.

All reactions
Comment thread packages/angular/build/src/utils/server-rendering/utils.ts Resolved

Uh oh!

There was an error while loading. Please reload this page.

@SkyZeroZx
SkyZeroZx requested a review from alan-agius4 October 22, 2026 15:37
@alan-agius4 alan-agius4 added the target: patch This PR is targeted for the next patch release label Oct 23, 2026
alan-agius4
alan-agius4 approved these changes Oct 23, 2026
View reviewed changes

@alan-agius4 alan-agius4 left a comment

Copy link
Copy Markdown
Collaborator

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.

LGTM

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@alan-agius4 alan-agius4 added the action: merge The PR is ready for merge by the caretaker label Oct 23, 2026
This was referenced Oct 23, 2026
[Backport 21.2.X] fix(@angular/build): escape prerender redirect URLs #34508
Merged
[Backport 20.3.X] fix(@angular/build): escape prerender redirect URLs #34509
Merged
@alan-agius4
alan-agius4 merged commit 537a47a into angular:main Oct 26, 2026
40 of 41 checks passed

Uh oh!

There was an error while loading. Please reload this page.

@alan-agius4

alan-agius4 commented Oct 26, 2026

Copy link
Copy Markdown
Collaborator

This PR was merged into the repository. The changes were merged into the following branches:

  • main: 537a47a
  • 22.2.x: 6d45d10
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

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

Reviewers

@alan-agius4 alan-agius4 alan-agius4 approved these changes
+1 more reviewer
@gemini-code-assist gemini-code-assist[bot] gemini-code-assist[bot] left review comments
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

action: merge The PR is ready for merge by the caretaker area: @angular/build target: patch This PR is targeted for the next patch release

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Angular SSR Unescaped prerender parameters enable RCE through generated manifests and XSS in redirect pages

Uh oh!

There was an error while loading. Please reload this page.

2 participants

@SkyZeroZx @alan-agius4
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.
You are visitor 01402520 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.