Skip to content

Commit 018fcbe

Browse files
committed
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 85c18b4 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 #34090
1 parent 31c0456 commit 018fcbe

4 files changed

Lines changed: 143 additions & 2 deletions

File tree

packages/angular/ssr/src/routes/router.ts

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@
77
*/
88

99
import { AngularAppManifest } from '../manifest';
10-
import { stripIndexHtmlFromURL, stripMatrixParams } from '../utils/url';
10+
import { normalizeUrlPath, stripIndexHtmlFromURL, stripMatrixParams } from '../utils/url';
1111
import { extractRoutesAndCreateRouteTree } from './ng-routes';
1212
import { RouteTree, RouteTreeNodeMetadata } from './route-tree';
1313

@@ -86,7 +86,9 @@ export class ServerRouter {
8686
// Strip 'index.html' from URL if present.
8787
// A request to `http://www.example.com/page/index.html` will render the Angular route corresponding to `http://www.example.com/page`.
8888
let { pathname } = stripIndexHtmlFromURL(url);
89-
pathname = stripMatrixParams(pathname);
89+
// Resolve the path through the router's own grammar before tokenising it, so the
90+
// route selected here is the route `@angular/router` will render.
91+
pathname = stripMatrixParams(normalizeUrlPath(pathname));
9092

9193
return this.routeTree.match(pathname);
9294
}

packages/angular/ssr/src/utils/url.ts

Lines changed: 62 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,8 @@
66
* found in the LICENSE file at https://angular.dev/license
77
*/
88

9+
import { DefaultUrlSerializer } from '@angular/router';
10+
911
/**
1012
* Removes the trailing slash from a URL if it exists.
1113
*
@@ -227,6 +229,66 @@ export function stripMatrixParams(pathname: string): string {
227229
return pathname.includes(';') ? pathname.replace(MATRIX_PARAMS_REGEX, '') : pathname;
228230
}
229231

232+
/**
233+
* A single reusable serializer. `DefaultUrlSerializer` is stateless, so one instance
234+
* is enough for the lifetime of the module.
235+
*/
236+
const URL_SERIALIZER = new DefaultUrlSerializer();
237+
238+
/**
239+
* Characters `DefaultUrlSerializer` never rewrites, measured across the printable ASCII
240+
* range against @angular/router 22.1.6. A path built only from these, with no empty
241+
* segment, is returned unchanged, so it can skip the parse entirely.
242+
*
243+
* Deliberately an allowlist. A denylist of the metacharacters that matter today
244+
* (`(`, `)`, `;`, `//`) leaves every other rewrite unapplied: measured over 583 probes,
245+
* such a check disagrees with the serializer on 87 of them, `/a b` and `/%41` among
246+
* them. An unknown character has to take the slow path, or the fast path becomes the
247+
* same cheap-predicate-in-front-of-a-real-parser split this function exists to close.
248+
*/
249+
const NON_NORMALIZING_PATH = /^[A-Za-z0-9\-._~!$&'*,:@/]*$/;
250+
251+
/**
252+
* Rewrites a URL path into the spelling `@angular/router` will resolve it to.
253+
*
254+
* Server route matching tokenises the path by splitting on `/`, while the client
255+
* router parses it with `DefaultUrlSerializer`, a grammar in which `(`, `)`, `;`
256+
* and `//` are metacharacters. The two therefore disagree on inputs such as
257+
* `/page)`, which the router resolves to `/page` and the server route tree treats
258+
* as a distinct segment. Passing the path through the router's own grammar first
259+
* makes both sides agree on which route a request is.
260+
*
261+
* A path the serializer cannot parse is returned unchanged, so malformed
262+
* percent-encoding keeps its existing behaviour.
263+
*
264+
* @param pathname - The URL path to normalize.
265+
* @returns The path as `@angular/router` would resolve it.
266+
*
267+
* @example
268+
* ```ts
269+
* normalizeUrlPath('/page)'); // returns '/page'
270+
* normalizeUrlPath('/(page)'); // returns '/page'
271+
* normalizeUrlPath('/a/1//b'); // returns '/a/1'
272+
* normalizeUrlPath('/page'); // returns '/page'
273+
* ```
274+
*/
275+
export function normalizeUrlPath(pathname: string): string {
276+
// Fast path: the serializer would return this path unchanged, so skip the parse.
277+
if (!pathname.includes('//') && NON_NORMALIZING_PATH.test(pathname)) {
278+
return pathname;
279+
}
280+
281+
try {
282+
const serialized = URL_SERIALIZER.serialize(URL_SERIALIZER.parse(pathname));
283+
// `serialize` reproduces the query string and fragment; only the path is matched.
284+
const queryOrFragment = serialized.search(/[?#]/);
285+
286+
return queryOrFragment === -1 ? serialized : serialized.slice(0, queryOrFragment);
287+
} catch {
288+
return pathname;
289+
}
290+
}
291+
230292
/**
231293
* Constructs a decoded URL string from its components.
232294
*

packages/angular/ssr/test/routes/router_spec.ts

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -128,6 +128,34 @@ describe('ServerRouter', () => {
128128
});
129129
});
130130

131+
it('should select the same route the client router will render', () => {
132+
// `@angular/router` resolves each of these to `/home`, because `(`, `)`, `;`
133+
// and `//` are metacharacters in its URL grammar. Server route matching has to
134+
// agree, or the response's headers, status and renderMode are taken from a
135+
// different route than the one that renders into the body.
136+
const home = {
137+
route: '/home',
138+
renderMode: RenderMode.Server,
139+
};
140+
141+
for (const pathname of ['/home)', '/home(', '/home;', '/(home)']) {
142+
expect(router.match(new URL(`http://localhost${pathname}`)))
143+
.withContext(pathname)
144+
.toEqual(home);
145+
}
146+
147+
// An interior `//` ends the path for the client router, so `/user/123//x`
148+
// renders the `/user/:id` route and must match its server config too.
149+
expect(router.match(new URL('http://localhost/user/123//x'))).toEqual({
150+
route: '/user/*',
151+
renderMode: RenderMode.Server,
152+
});
153+
});
154+
155+
it('should not invent a match for an unknown route', () => {
156+
expect(router.match(new URL('http://localhost/nope'))).toBeUndefined();
157+
});
158+
131159
it('should handle encoded params', () => {
132160
const encodedUserMetadata = router.match(
133161
new URL('http://localhost/user/Bob%20%2F%20Roberts'),

packages/angular/ssr/test/utils/url_spec.ts

Lines changed: 49 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ import {
1111
addTrailingSlash,
1212
buildPathWithParams,
1313
joinUrlParts,
14+
normalizeUrlPath,
1415
stripIndexHtmlFromURL,
1516
stripLeadingSlash,
1617
stripMatrixParams,
@@ -220,4 +221,52 @@ describe('URL Utils', () => {
220221
expect(stripMatrixParams('')).toBe('');
221222
});
222223
});
224+
describe('normalizeUrlPath', () => {
225+
it('should resolve spellings that `@angular/router` treats as the same route', () => {
226+
// Each left-hand value is what `DefaultUrlSerializer` resolves the path to,
227+
// verified against the published @angular/router 22.1.6.
228+
expect(normalizeUrlPath('/page)')).toBe('/page');
229+
expect(normalizeUrlPath('/page(')).toBe('/page');
230+
expect(normalizeUrlPath('/page;')).toBe('/page');
231+
expect(normalizeUrlPath('/(page)')).toBe('/page');
232+
expect(normalizeUrlPath('/a/1//b')).toBe('/a/1');
233+
expect(normalizeUrlPath('/a/b)c/d')).toBe('/a/b');
234+
});
235+
236+
it('should leave an ordinary path unchanged', () => {
237+
expect(normalizeUrlPath('/page')).toBe('/page');
238+
expect(normalizeUrlPath('/a/b/c')).toBe('/a/b/c');
239+
expect(normalizeUrlPath('/user/123')).toBe('/user/123');
240+
expect(normalizeUrlPath('/')).toBe('/');
241+
});
242+
243+
it('should preserve encoding, including an encoded slash', () => {
244+
expect(normalizeUrlPath('/a%2Fb')).toBe('/a%2Fb');
245+
expect(normalizeUrlPath('/encoding%20url')).toBe('/encoding%20url');
246+
});
247+
248+
it('should preserve matrix parameters so stripMatrixParams still owns them', () => {
249+
expect(normalizeUrlPath('/page;p=1')).toBe('/page;p=1');
250+
});
251+
252+
it('should return a path it cannot parse unchanged', () => {
253+
// Malformed percent-encoding keeps its existing behaviour.
254+
expect(normalizeUrlPath('/%zz')).toBe('/%zz');
255+
});
256+
257+
it('should not alter dot segments or index.html handling', () => {
258+
expect(normalizeUrlPath('/a/./b')).toBe('/a/./b');
259+
expect(normalizeUrlPath('/page/index.html')).toBe('/page/index.html');
260+
});
261+
262+
it('should still normalize paths a metacharacter-only check would skip', () => {
263+
// The fast path is an allowlist for this reason: a check that bails out only on
264+
// `(`, `)`, `;` and `//` leaves these unnormalized, which is the same
265+
// cheap-predicate-versus-real-parser split this function exists to close.
266+
expect(normalizeUrlPath('/a b')).toBe('/a%20b');
267+
expect(normalizeUrlPath('/a+b')).toBe('/a%2Bb');
268+
expect(normalizeUrlPath('/%41')).toBe('/A');
269+
expect(normalizeUrlPath('/a%2fb')).toBe('/a%2Fb');
270+
});
271+
});
223272
});

0 commit comments

Comments
 (0)