From 6d6decb6af4cbd848772827e1fd14b408c2f33b0 Mon Sep 17 00:00:00 2001 From: glivter <8862088+glivter@users.noreply.github.com> Date: Mon, 14 Sep 2026 22:57:01 +0200 Subject: [PATCH] fix(@angular/ssr): resolve the request path through the router's URL grammar before matching `ServerRouter.match` tokenises the pathname by splitting on `/`, while `@angular/router` parses it with `DefaultUrlSerializer`, a grammar in which `(`, `)`, `;` and `//` are metacharacters and unparseable input is silently discarded. The two therefore disagree on which route a request is: verified against @angular/router 22.1.6, `/page)`, `/page(`, `/page;` and `/(page)` all resolve to `/page`, and `/a/1//b` resolves to `/a/1`. Because `ServerRouter.match` selects the response's `headers`, `status`, `renderMode` and `preload` while `@angular/router` selects the component that renders into the body, appending a single character to a path produces a response whose body comes from one route and whose per-route configuration comes from another. A route given `Cache-Control: no-store, private` plus `X-Frame-Options: DENY` is served under the catch-all's policy with neither header, and a route declared `RenderMode.Client` is server-rendered. This is the same divergence that 85c18b4e fixed for matrix parameters, where it surfaced as URLs failing to match their route. `stripMatrixParams` handled that case; parentheses and interior `//` are the remaining ones. Normalising through the router's own serializer covers the class rather than the next symptom, and `@angular/router` is already a peer dependency of this package. A path the serializer cannot parse is returned unchanged, so malformed percent-encoding keeps its existing behaviour, and normalisation runs before `stripMatrixParams` so matrix parameters are still stripped exactly as today. Paths the serializer would leave alone skip the parse. That check is an allowlist of the characters it never rewrites, measured across the printable ASCII range. A denylist of the metacharacters that matter today was measured first and rejected: over 583 probes it disagrees with the serializer on 87 of them, `/a b`, `/a+b` and `/%41` among them, which would leave the two matchers apart on exactly the inputs nobody thought to enumerate. Closes #33555 --- packages/angular/ssr/src/routes/router.ts | 6 +- packages/angular/ssr/src/utils/url.ts | 62 +++++++++++++++++++ .../angular/ssr/test/routes/router_spec.ts | 28 +++++++++ packages/angular/ssr/test/utils/url_spec.ts | 49 +++++++++++++++ 4 files changed, 143 insertions(+), 2 deletions(-) diff --git a/packages/angular/ssr/src/routes/router.ts b/packages/angular/ssr/src/routes/router.ts index 16b6a83c90f1..e39d2611b15f 100644 --- a/packages/angular/ssr/src/routes/router.ts +++ b/packages/angular/ssr/src/routes/router.ts @@ -7,7 +7,7 @@ */ import { AngularAppManifest } from '../manifest'; -import { stripIndexHtmlFromURL, stripMatrixParams } from '../utils/url'; +import { normalizeUrlPath, stripIndexHtmlFromURL, stripMatrixParams } from '../utils/url'; import { extractRoutesAndCreateRouteTree } from './ng-routes'; import { RouteTree, RouteTreeNodeMetadata } from './route-tree'; @@ -86,7 +86,9 @@ export class ServerRouter { // Strip 'index.html' from URL if present. // A request to `http://www.example.com/page/index.html` will render the Angular route corresponding to `http://www.example.com/page`. let { pathname } = stripIndexHtmlFromURL(url); - pathname = stripMatrixParams(pathname); + // Resolve the path through the router's own grammar before tokenising it, so the + // route selected here is the route `@angular/router` will render. + pathname = stripMatrixParams(normalizeUrlPath(pathname)); return this.routeTree.match(pathname); } diff --git a/packages/angular/ssr/src/utils/url.ts b/packages/angular/ssr/src/utils/url.ts index d5e7c9ac2814..ac77ddaa86e2 100644 --- a/packages/angular/ssr/src/utils/url.ts +++ b/packages/angular/ssr/src/utils/url.ts @@ -6,6 +6,8 @@ * found in the LICENSE file at https://angular.dev/license */ +import { DefaultUrlSerializer } from '@angular/router'; + /** * Removes the trailing slash from a URL if it exists. * @@ -227,6 +229,66 @@ export function stripMatrixParams(pathname: string): string { return pathname.includes(';') ? pathname.replace(MATRIX_PARAMS_REGEX, '') : pathname; } +/** + * A single reusable serializer. `DefaultUrlSerializer` is stateless, so one instance + * is enough for the lifetime of the module. + */ +const URL_SERIALIZER = new DefaultUrlSerializer(); + +/** + * Characters `DefaultUrlSerializer` never rewrites, measured across the printable ASCII + * range against @angular/router 22.1.6. A path built only from these, with no empty + * segment, is returned unchanged, so it can skip the parse entirely. + * + * Deliberately an allowlist. A denylist of the metacharacters that matter today + * (`(`, `)`, `;`, `//`) leaves every other rewrite unapplied: measured over 583 probes, + * such a check disagrees with the serializer on 87 of them, `/a b` and `/%41` among + * them. An unknown character has to take the slow path, or the fast path becomes the + * same cheap-predicate-in-front-of-a-real-parser split this function exists to close. + */ +const NON_NORMALIZING_PATH = /^[A-Za-z0-9\-._~!$&'*,:@/]*$/; + +/** + * Rewrites a URL path into the spelling `@angular/router` will resolve it to. + * + * Server route matching tokenises the path by splitting on `/`, while the client + * router parses it with `DefaultUrlSerializer`, a grammar in which `(`, `)`, `;` + * and `//` are metacharacters. The two therefore disagree on inputs such as + * `/page)`, which the router resolves to `/page` and the server route tree treats + * as a distinct segment. Passing the path through the router's own grammar first + * makes both sides agree on which route a request is. + * + * A path the serializer cannot parse is returned unchanged, so malformed + * percent-encoding keeps its existing behaviour. + * + * @param pathname - The URL path to normalize. + * @returns The path as `@angular/router` would resolve it. + * + * @example + * ```ts + * normalizeUrlPath('/page)'); // returns '/page' + * normalizeUrlPath('/(page)'); // returns '/page' + * normalizeUrlPath('/a/1//b'); // returns '/a/1' + * normalizeUrlPath('/page'); // returns '/page' + * ``` + */ +export function normalizeUrlPath(pathname: string): string { + // Fast path: the serializer would return this path unchanged, so skip the parse. + if (!pathname.includes('//') && NON_NORMALIZING_PATH.test(pathname)) { + return pathname; + } + + try { + const serialized = URL_SERIALIZER.serialize(URL_SERIALIZER.parse(pathname)); + // `serialize` reproduces the query string and fragment; only the path is matched. + const queryOrFragment = serialized.search(/[?#]/); + + return queryOrFragment === -1 ? serialized : serialized.slice(0, queryOrFragment); + } catch { + return pathname; + } +} + /** * Constructs a decoded URL string from its components. * diff --git a/packages/angular/ssr/test/routes/router_spec.ts b/packages/angular/ssr/test/routes/router_spec.ts index 380bc659549a..dba17d1c06e7 100644 --- a/packages/angular/ssr/test/routes/router_spec.ts +++ b/packages/angular/ssr/test/routes/router_spec.ts @@ -128,6 +128,34 @@ describe('ServerRouter', () => { }); }); + it('should select the same route the client router will render', () => { + // `@angular/router` resolves each of these to `/home`, because `(`, `)`, `;` + // and `//` are metacharacters in its URL grammar. Server route matching has to + // agree, or the response's headers, status and renderMode are taken from a + // different route than the one that renders into the body. + const home = { + route: '/home', + renderMode: RenderMode.Server, + }; + + for (const pathname of ['/home)', '/home(', '/home;', '/(home)']) { + expect(router.match(new URL(`http://localhost${pathname}`))) + .withContext(pathname) + .toEqual(home); + } + + // An interior `//` ends the path for the client router, so `/user/123//x` + // renders the `/user/:id` route and must match its server config too. + expect(router.match(new URL('http://localhost/user/123//x'))).toEqual({ + route: '/user/*', + renderMode: RenderMode.Server, + }); + }); + + it('should not invent a match for an unknown route', () => { + expect(router.match(new URL('http://localhost/nope'))).toBeUndefined(); + }); + it('should handle encoded params', () => { const encodedUserMetadata = router.match( new URL('http://localhost/user/Bob%20%2F%20Roberts'), diff --git a/packages/angular/ssr/test/utils/url_spec.ts b/packages/angular/ssr/test/utils/url_spec.ts index a108c7ff1df6..c219b5f02b4e 100644 --- a/packages/angular/ssr/test/utils/url_spec.ts +++ b/packages/angular/ssr/test/utils/url_spec.ts @@ -11,6 +11,7 @@ import { addTrailingSlash, buildPathWithParams, joinUrlParts, + normalizeUrlPath, stripIndexHtmlFromURL, stripLeadingSlash, stripMatrixParams, @@ -220,4 +221,52 @@ describe('URL Utils', () => { expect(stripMatrixParams('')).toBe(''); }); }); + describe('normalizeUrlPath', () => { + it('should resolve spellings that `@angular/router` treats as the same route', () => { + // Each left-hand value is what `DefaultUrlSerializer` resolves the path to, + // verified against the published @angular/router 22.1.6. + expect(normalizeUrlPath('/page)')).toBe('/page'); + expect(normalizeUrlPath('/page(')).toBe('/page'); + expect(normalizeUrlPath('/page;')).toBe('/page'); + expect(normalizeUrlPath('/(page)')).toBe('/page'); + expect(normalizeUrlPath('/a/1//b')).toBe('/a/1'); + expect(normalizeUrlPath('/a/b)c/d')).toBe('/a/b'); + }); + + it('should leave an ordinary path unchanged', () => { + expect(normalizeUrlPath('/page')).toBe('/page'); + expect(normalizeUrlPath('/a/b/c')).toBe('/a/b/c'); + expect(normalizeUrlPath('/user/123')).toBe('/user/123'); + expect(normalizeUrlPath('/')).toBe('/'); + }); + + it('should preserve encoding, including an encoded slash', () => { + expect(normalizeUrlPath('/a%2Fb')).toBe('/a%2Fb'); + expect(normalizeUrlPath('/encoding%20url')).toBe('/encoding%20url'); + }); + + it('should preserve matrix parameters so stripMatrixParams still owns them', () => { + expect(normalizeUrlPath('/page;p=1')).toBe('/page;p=1'); + }); + + it('should return a path it cannot parse unchanged', () => { + // Malformed percent-encoding keeps its existing behaviour. + expect(normalizeUrlPath('/%zz')).toBe('/%zz'); + }); + + it('should not alter dot segments or index.html handling', () => { + expect(normalizeUrlPath('/a/./b')).toBe('/a/./b'); + expect(normalizeUrlPath('/page/index.html')).toBe('/page/index.html'); + }); + + it('should still normalize paths a metacharacter-only check would skip', () => { + // The fast path is an allowlist for this reason: a check that bails out only on + // `(`, `)`, `;` and `//` leaves these unnormalized, which is the same + // cheap-predicate-versus-real-parser split this function exists to close. + expect(normalizeUrlPath('/a b')).toBe('/a%20b'); + expect(normalizeUrlPath('/a+b')).toBe('/a%2Bb'); + expect(normalizeUrlPath('/%41')).toBe('/A'); + expect(normalizeUrlPath('/a%2fb')).toBe('/a%2Fb'); + }); + }); });