Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
![gemini-code-assist[bot]](/web-api/iframe-proxy?url=https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces URL path normalization in the Angular SSR server router using the client-side DefaultUrlSerializer. This ensures that server-side route matching correctly aligns with the client-side router's handling of metacharacters like parentheses, semicolons, and double slashes. The feedback suggests optimizing normalizeUrlPath with a fast-path check to avoid the performance overhead of parsing and serializing standard paths that do not contain these metacharacters.
| export function normalizeUrlPath(pathname: string): string { | ||
| 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; | ||
| } | ||
| } |
There was a problem hiding this comment.
To avoid the performance overhead of parsing and serializing every single incoming request URL path (which is a common hot path in SSR), we can add a fast-path check. Since the vast majority of requests will be standard paths without any router metacharacters (like (, ), ;, //, ?, or #), we can return the pathname immediately if none of these characters are present. This avoids invoking the relatively expensive DefaultUrlSerializer parser and serializer unnecessarily.
export function normalizeUrlPath(pathname: string): string {
if (
!pathname.includes('(') &&
!pathname.includes(')') &&
!pathname.includes(';') &&
!pathname.includes('//') &&
!pathname.includes('?') &&
!pathname.includes('#')
) {
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;
}
}There was a problem hiding this comment.
Taken, with one change to the character set, because I measured the suggested one first and it is not conservative enough.
Bailing out only on (, ), ;, //, ? and # skips normalisation for every other input DefaultUrlSerializer rewrites. Over 583 probes across the printable ASCII range against @angular/router 22.1.6, that check disagrees with the serializer on 87 of them:
/a b fast-path: "/a b" serializer: "/a%20b"
/a+b fast-path: "/a+b" serializer: "/a%2Bb"
/%41 fast-path: "/%41" serializer: "/A"
/a%2fb fast-path: "/a%2fb" serializer: "/a%2Fb"
The full set of characters that can change a path is space " # ( ) + / ; < = > ? [ \ ] ^ \ { | }and%`.
A denylist here would be the same shape as the bug this PR is fixing: a cheap character check standing in front of a real parser and disagreeing with it on the inputs nobody thought to enumerate. So the fast path is an allowlist of the characters the serializer never rewrites, plus a check for an empty segment, and anything unrecognised takes the slow path. Measured over the same 583 probes it disagrees with the serializer on 0.
Pushed in 018fcbe, with tests pinning that /a b, /a+b, /%41 and /a%2fb are still normalised.
ecc6313 to
018fcbe
Compare
…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 angular#33555
018fcbe to
6d6decb
Compare
1 participant
PR Checklist
PR Type
What is the current behavior?
Issue Number: #33555
ServerRouter.matchtokenises the pathname by splitting on/, while@angular/routerparses it withDefaultUrlSerializer, a grammar in which(,),;and//are metacharacters and unparseable input is silently discarded. The two disagree on which route a request is.Verified against the published
@angular/router22.1.6,serialize(parse(x)):/page)/page/page(/page/page;/page/(page)/page/a/1//b/a/1Because
ServerRouter.matchselects the response'sheaders,status,renderModeandpreloadwhile@angular/routerselects the component that renders into the body, appending one character to a path produces a response whose body comes from one route and whose per-route configuration comes from another. A route configured withCache-Control: no-store, privateandX-Frame-Options: DENYis served under the catch-all's policy with neither header, and a route declaredRenderMode.Clientis server-rendered.The
@angular/routerside is intentional.(name:seg)is the documented secondary-outlet syntax,;k=vthe documented matrix-parameter syntax, and angular/angular#64507 deliberately made an unnamed(...)group mean the primary outlet.What is the new behavior?
ServerRouter.matchresolves the pathname through the router's own serializer before tokenising it, so both matchers agree on which route a request is.This is the same divergence that 85c18b4 fixed for matrix parameters, where it surfaced as URLs failing to match their route.
stripMatrixParamshandled that case; parentheses and interior//are the remaining ones. Going through the serializer covers the class rather than the next symptom, and@angular/routeris already a peer dependency of this package.Normalisation runs before
stripMatrixParams, because the serializer preserves matrix parameters, so they are still stripped exactly as today. A path the serializer cannot parse is returned unchanged, so malformed percent-encoding keeps its existing behaviour.Known residual, stated up front: an application that supplies a custom
UrlSerializerstill diverges, because this normalises withDefaultUrlSerializer. The durable fix is to build and query the route tree through the serializer the application injects, which is a larger change. I am happy to take this in that direction instead, or to have this closed in favour of an internal patch if one is already in progress; the issue matters more than the PR.Tests
packages/angular/ssr/test/utils/url_spec.tscoversnormalizeUrlPathdirectly: the divergent spellings, an unchanged ordinary path, preserved encoding including%2F, preserved matrix parameters, and an unparseable path returned as-is. Every expected value was measured against@angular/router22.1.6 rather than assumed.packages/angular/ssr/test/routes/router_spec.tscovers the end-to-end selection:/home),/home(,/home;and/(home)select the same route metadata as/home,/user/123//xselects/user/*, and an unknown route still matches nothing.Both were confirmed to FAIL against
mainbefore the change, with the failure reproducing the real defect://packages/angular/ssr/test:testis green with the change, 257 specs.Does this PR introduce a breaking change?
Every path that does not use the router's metacharacters tokenises exactly as before. The behaviour change is limited to paths where the two matchers currently disagree, and there the server now agrees with what is actually rendered.
Other information
Originally filed as #34090, which turned out to be a duplicate of #33555 and has been closed in its favour; the additional coverage from it is now a comment on #33555. My prior-art sweep missed #33555 because I searched my own vocabulary, "route-tree tokenisation" and "matcher divergence", rather than the words a reporter would use. Credit to @SkyZeroZx for catching it.
Also reported via the Google OSS VRP as 559764571, which the Bug Hunter Team triaged and invited me to disclose publicly.
This change and its testing were produced with AI assistance. The equivalence table and every value asserted in the new tests were executed against the published
@angular/router22.1.6 rather than reasoned about, and the tests were confirmed to fail before the fix.