fix(@angular/build): escape prerender redirect URLs - #33248
SkyZeroZx wants to merge 2 commits into
Conversation
Sorry, something went wrong.
There was a problem hiding this comment.
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.
alan-agius4
left a comment
There was a problem hiding this comment.
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.
| // Asset paths can contain decoded prerender parameters and must remain string data in the | ||
| // generated executable manifest. | ||
| const jsChunkImportPath = `./${jsChunkFilePath |
There was a problem hiding this comment.
JSON.stringify() protects the generated JavaScript syntax, but dynamic import specifiers are also parsed as URLs.
Decoded prerender parameters may contain URL-significant characters such as #, ?, or %; if left unencoded, Node can interpret them as fragments, query strings, or escape sequences and resolve a different chunk path.
Encoding each path segment preserves directory separators while keeping those characters as filename data. JSON.stringify() then safely embeds the resulting specifier in the executable manifest. The physical chunk filename remains unchanged.
This would also prevent, as a defense in depth, the initially closed vector of both RCE and XSS that was present along the routes.
Sorry, something went wrong.
alan-agius4
left a comment
There was a problem hiding this comment.
[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.
| ); | ||
| } | ||
|
|
||
| const invalidValueReason = validateUrlForStaticEmission(value); |
There was a problem hiding this comment.
[Gemini Review]: Validating individual parameter segments in isolation here causes a few issues:
- DX Breaking change: Validating raw segments rejects legitimate slugs with apostrophes (customer's-choice, women's-fashion) and spaces. Downstream in route-tree.ts (getPathSegments), .map(decodeURIComponent) is called, which immediately decodes %27 back to ' anyway. Single quotes in asset keys are already safely escaped by JSON.stringify(key) in manifest.ts.
- Segments lack route context: An isolated segment like '42' is not a URL. Furthermore, this misses compositional issues (e.g. if a segment combined with the route forms a protocol-relative // or traversal ../).
Instead of validating each segment here, let's remove this check and validate the generated route path (routeWithResolvedParams) in handleSSGRoute (around line 474):
const routeWithResolvedParams = currentRoutePath
.replace(URL_PARAMETER_GLOBAL_REGEXP, replacer)
.replace(CATCH_ALL_REGEXP, replacer);
// Validate and normalize the generated path
const slashPath = addLeadingSlash(routeWithResolvedParams);
if (slashPath.startsWith('//') || slashPath.includes('\\')) {
yield {
error: `The '${stripLeadingSlash(currentRoutePath)}' route produced an invalid prerender path '${routeWithResolvedParams}'.`,
};
continue;
}
try {
const parsed = new URL(slashPath, 'http://127.0.0.1');
if (parsed.origin !== 'http://127.0.0.1') {
yield {
error: `The '${stripLeadingSlash(currentRoutePath)}' route produced an escaping prerender path '${routeWithResolvedParams}'.`,
};
continue;
}
const normalizedRoute = parsed.pathname;
// yield normalizedRoute...
} catch (err) {
yield { error: `Invalid prerender path '${routeWithResolvedParams}': ${(err as Error).message}` };
}This automatically normalizes spaces (/docs/a b -> /docs/a%20b) and percent-encodes HTML characters (<script> -> %3Cscript%3E), while leaving apostrophes intact without failing the build.
Sorry, something went wrong.
There was a problem hiding this comment.
All the indicated changes have been addressed; another commit was made to tackle the potential RCE issue.
EDIT: It seems there is a conflict; I am currently rebasing to resolve it.
EDIT2: Done
Sorry, something went wrong.
| * Builds the path of the generated chunk which holds the content of a server asset. | ||
| * | ||
| * Asset paths are derived from route paths and can therefore contain characters which are unusable | ||
| * in a file name (`?`, `:` and `*` are invalid on Windows) or which change how the generated |
There was a problem hiding this comment.
Upon closer inspection (with the help of an agent), this could fail if used on Windows, so I added this helper to avoid these issues.
Sorry, something went wrong.
|
@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.
Escape redirect targets before embedding them in generated meta refresh pages and fallback links. Centralize WHATWG URL normalization in @angular/ssr for composed prerender paths, configured redirects, Location headers, and runtime redirects. Preserve documented catch-all parameter values, reject unsafe schemes and path forms, and cover validation failures with focused unit tests.
Serialize route-derived asset keys, base paths, hashes, and App Engine entry points before embedding them in executable ESM manifests. Generate bounded ASCII asset chunk names with a path digest so filesystem-sensitive and URL-significant characters remain filename data without creating chunk-name collisions. Add focused manifest coverage and exercise an apostrophe-bearing prerender route end to end.
|
@alan-agius4 Hi, I think this PR is ready for review and hope we can merge it this week, as it fixes security issues according to the description in the two commits. |
Sorry, something went wrong.
Prevent HTML injection in static redirects
Escape redirect targets before embedding them in generated meta refresh pages and fallback links.
Centralize WHATWG URL normalization in @angular/ssr for composed prerender paths, configured redirects, Location headers, and runtime redirects. Preserve documented catch-all parameter values, reject unsafe schemes and path forms, and cover validation failures with focused unit tests.
Safely serialize paths in the server manifest
Serialize route-derived asset keys, base paths, hashes, and App Engine entry points before embedding them in executable ESM manifests.
Generate bounded ASCII asset chunk names with a path digest so filesystem-sensitive and URL-significant characters remain filename data without creating chunk-name collisions. Add focused manifest coverage and exercise an apostrophe-bearing prerender route end to end.