Skip to content

Commit ff3f372

Browse files
bschaeublinalan-agius4
authored andcommitted
fix(@angular/ssr): resolve route parameters in redirect targets (#34321)
Route parameters in the path of a `redirectTo` target, such as `:id` in `profile/:id`, were sent unchanged in the `Location` header by the server, while the router replaces them with the matching parameters of the current URL. The server now does the same. PR Close #34321
1 parent b946277 commit ff3f372

5 files changed

Lines changed: 199 additions & 11 deletions

File tree

‎packages/angular/ssr/src/routes/ng-routes.ts‎

Lines changed: 78 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -147,6 +147,7 @@ async function* handleRoute(options: {
147147
metadata: ServerConfigRouteTreeNodeMetadata;
148148
currentRoutePath: string;
149149
parentRoute: string;
150+
baseHref: string;
150151
route: Route;
151152
compiler: Compiler;
152153
parentInjector: Injector;
@@ -160,6 +161,7 @@ async function* handleRoute(options: {
160161
metadata,
161162
currentRoutePath,
162163
parentRoute,
164+
baseHref,
163165
route,
164166
compiler,
165167
parentInjector,
@@ -179,8 +181,9 @@ async function* handleRoute(options: {
179181
if (metadata.renderMode === RenderMode.Prerender) {
180182
for await (const result of handleSSGRoute(
181183
serverConfigRouteTree,
182-
typeof redirectTo === 'string' ? redirectTo : undefined,
184+
route,
183185
parentRoute,
186+
baseHref,
184187
metadata,
185188
parentInjector,
186189
invokeGetPrerenderParams,
@@ -200,9 +203,18 @@ async function* handleRoute(options: {
200203
`Please use one of the following redirect response codes: ${[...VALID_REDIRECT_RESPONSE_CODES.values()].join(', ')}.`,
201204
};
202205
} else if (typeof redirectTo === 'string') {
206+
// Reference the route parameters by their position in the URL (see `buildPathWithParams`).
207+
const routeSegments = joinUrlParts(baseHref, currentRoutePath).split('/');
203208
parentRoutes.set(metadata.route, {
204209
...metadata,
205-
redirectTo: resolveRedirectTo(parentRoute, redirectTo),
210+
redirectTo: resolveRedirectTo(
211+
parentRoute,
212+
replaceRedirectParams(
213+
route,
214+
redirectTo,
215+
(parameter) => `*${routeSegments.lastIndexOf(parameter)}`,
216+
),
217+
),
206218
relativeRedirect: redirectTo[0] !== '/' ? true : undefined,
207219
});
208220
} else {
@@ -290,6 +302,7 @@ async function* traverseRoutesConfig(options: {
290302
compiler: Compiler;
291303
parentInjector: Injector;
292304
parentRoute: string;
305+
baseHref: string;
293306
serverConfigRouteTree?: RouteTree<ServerConfigRouteTreeAdditionalMetadata>;
294307
invokeGetPrerenderParams: boolean;
295308
includePrerenderFallbackRoutes: boolean;
@@ -421,8 +434,9 @@ function appendPreloadToMetadata(
421434
* all parameterized paths, returning any errors encountered.
422435
*
423436
* @param serverConfigRouteTree - The tree representing the server's routing setup.
424-
* @param redirectTo - Optional path to redirect to, if specified.
437+
* @param route - The route configuration.
425438
* @param parentRoute - The path of the parent route.
439+
* @param baseHref - The base href of the application.
426440
* @param metadata - The metadata associated with the route tree node.
427441
* @param parentInjector - The dependency injection container for the parent route.
428442
* @param invokeGetPrerenderParams - A flag indicating whether to invoke the `getPrerenderParams` function.
@@ -431,8 +445,9 @@ function appendPreloadToMetadata(
431445
*/
432446
async function* handleSSGRoute(
433447
serverConfigRouteTree: RouteTree<ServerConfigRouteTreeAdditionalMetadata> | undefined,
434-
redirectTo: string | undefined,
448+
route: Route,
435449
parentRoute: string,
450+
baseHref: string,
436451
metadata: ServerConfigRouteTreeNodeMetadata,
437452
parentInjector: Injector,
438453
invokeGetPrerenderParams: boolean,
@@ -451,8 +466,18 @@ async function* handleSSGRoute(
451466
delete meta['getPrerenderParams'];
452467
}
453468

469+
const redirectTo = typeof route.redirectTo === 'string' ? route.redirectTo : undefined;
454470
if (redirectTo !== undefined) {
455-
meta.redirectTo = resolveRedirectTo(parentRoute, redirectTo);
471+
// Reference the route parameters by their position in the URL (see `buildPathWithParams`).
472+
const routeSegments = joinUrlParts(baseHref, currentRoutePath).split('/');
473+
meta.redirectTo = resolveRedirectTo(
474+
parentRoute,
475+
replaceRedirectParams(
476+
route,
477+
redirectTo,
478+
(parameter) => `*${routeSegments.lastIndexOf(parameter)}`,
479+
),
480+
);
456481
meta.relativeRedirect = redirectTo[0] !== '/' ? true : undefined;
457482
}
458483

@@ -513,7 +538,11 @@ async function* handleSSGRoute(
513538
? undefined
514539
: resolveRedirectTo(
515540
parentRoute.replace(URL_PARAMETER_GLOBAL_REGEXP, replacer),
516-
redirectTo,
541+
replaceRedirectParams(
542+
route,
543+
redirectTo,
544+
(parameter) => params[parameter.slice(1)],
545+
),
517546
),
518547
};
519548
}
@@ -589,6 +618,48 @@ function resolveRedirectTo(parentRoute: string, redirectTo: string): string {
589618
return joinUrlParts(parentRoute.replace(URL_PARAMETER_GLOBAL_REGEXP, '*'), path);
590619
}
591620

621+
/**
622+
* Replaces the route parameters, such as `:id`, in the path of `redirectTo`.
623+
*
624+
* Like in the Angular router, only whole path segments starting with `:` are parameters, and only
625+
* the parameters defined in the path of the redirecting route itself can be used.
626+
*
627+
* @param route - The redirecting route.
628+
* @param redirectTo - The target path for redirection.
629+
* @param resolve - A function that returns the replacement for a parameter, such as `:id`.
630+
* @returns The redirect path with its route parameters replaced.
631+
* @throws If `redirectTo` uses a parameter that is not defined in the path of the route.
632+
*/
633+
function replaceRedirectParams(
634+
route: Route,
635+
redirectTo: string,
636+
resolve: (parameter: string) => string,
637+
): string {
638+
if (route.matcher) {
639+
// The parameters of a custom matcher are only known at runtime.
640+
return redirectTo;
641+
}
642+
643+
const routeSegments = route.path?.split('/') ?? [];
644+
const [path] = redirectTo.split(/[?#]/, 1);
645+
const resolvedPath = path
646+
.split('/')
647+
.map((segment) => {
648+
if (segment[0] !== ':') {
649+
return segment;
650+
}
651+
652+
if (!routeSegments.includes(segment)) {
653+
throw new Error(`Cannot redirect to '${redirectTo}'. Cannot find '${segment}'.`);
654+
}
655+
656+
return resolve(segment);
657+
})
658+
.join('/');
659+
660+
return resolvedPath + redirectTo.slice(path.length);
661+
}
662+
592663
/**
593664
* Builds a server configuration route tree from the given server routes configuration.
594665
*
@@ -742,6 +813,7 @@ export async function getRoutesFromAngularRouterConfig(
742813
compiler,
743814
parentInjector: injector,
744815
parentRoute: '',
816+
baseHref,
745817
serverConfigRouteTree,
746818
invokeGetPrerenderParams,
747819
includePrerenderFallbackRoutes,

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

Lines changed: 18 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -152,12 +152,18 @@ export function stripIndexHtmlFromURL(url: URL): URL {
152152
return url;
153153
}
154154

155+
/**
156+
* Matches a `*<n>` placeholder that makes up a whole path segment.
157+
*/
158+
const POSITIONAL_PLACEHOLDER_REGEXP = /^\*(\d+)$/;
159+
155160
/**
156161
* Resolves `*` placeholders in a path template by mapping them to corresponding segments
157162
* from a base path. This is useful for constructing paths dynamically based on a given base path.
158163
*
159164
* The function processes the `toPath` string, replacing each `*` placeholder with
160-
* the corresponding segment from the `fromPath`. If the `toPath` contains no placeholders,
165+
* the corresponding segment from the `fromPath`, and each `*<n>` placeholder with the
166+
* segment at position `n` of the `fromPath`. If the `toPath` contains no placeholders,
161167
* it is returned as-is. Invalid `toPath` formats (not starting with `/`) will throw an error.
162168
*
163169
* @param toPath - A path template string that may contain `*` placeholders. Each `*` is replaced
@@ -176,6 +182,10 @@ export function stripIndexHtmlFromURL(url: URL): URL {
176182
* const resolvedPath = buildPathWithParams('/*\/details', '/123/abc');
177183
* console.log(resolvedPath); // Outputs: '/123/details'
178184
*
185+
* // Example with a positional placeholder
186+
* const positionalPath = buildPathWithParams('/user/*1', '/123/abc');
187+
* console.log(positionalPath); // Outputs: '/user/123'
188+
*
179189
* // Example with a static path
180190
* const staticPath = buildPathWithParams('/static/path', '/base/unused');
181191
* console.log(staticPath); // Outputs: '/static/path'
@@ -190,17 +200,20 @@ export function buildPathWithParams(toPath: string, fromPath: string): string {
190200
throw new Error(`Invalid fromPath: The string must start with a '/'. Received: '${fromPath}'`);
191201
}
192202

193-
if (!toPath.includes('/*')) {
203+
const [path] = toPath.split(/[?#]/, 1);
204+
if (!path.includes('/*')) {
194205
return toPath;
195206
}
196207

197208
const fromPathParts = fromPath.split('/');
198-
const toPathParts = toPath.split('/');
209+
const toPathParts = path.split('/');
199210
const resolvedParts = toPathParts.map((part, index) =>
200-
toPathParts[index] === '*' ? fromPathParts[index] : part,
211+
part === '*'
212+
? fromPathParts[index]
213+
: part.replace(POSITIONAL_PLACEHOLDER_REGEXP, (_, position) => fromPathParts[position]),
201214
);
202215

203-
return joinUrlParts(...resolvedParts);
216+
return joinUrlParts(...resolvedParts) + toPath.slice(path.length);
204217
}
205218

206219
const MATRIX_PARAMS_REGEX = /;[^/]+/g;

‎packages/angular/ssr/test/app_spec.ts‎

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -90,6 +90,8 @@ describe('AngularServerApp', () => {
9090
{ path: 'redirect/absolute', redirectTo: '/home' },
9191
{ path: 'redirect-with-query', redirectTo: 'home?param=value' },
9292
{ path: 'redirect-nested/:param', children: [{ path: 'old', redirectTo: 'new' }] },
93+
{ path: 'redirect-param/:a/:b', redirectTo: '/home/:b/:a' },
94+
{ path: 'redirect-param-relative/:id', redirectTo: 'target/:id' },
9395
{
9496
path: 'redirect-to-function',
9597
redirectTo: () => 'home',
@@ -252,6 +254,20 @@ describe('AngularServerApp', () => {
252254
expect(response?.status).toBe(302);
253255
});
254256

257+
it('should resolve route parameters in absolute redirect targets', async () => {
258+
const response = await app.handle(new Request('http://localhost/redirect-param/x/y'));
259+
expect(response?.headers.get('location')).toBe('/home/y/x');
260+
expect(response?.status).toBe(302);
261+
});
262+
263+
it('should resolve route parameters in relative redirect targets', async () => {
264+
const response = await app.handle(
265+
new Request('http://localhost/redirect-param-relative/7?filter=test'),
266+
);
267+
expect(response?.headers.get('location')).toBe('/target/7?filter=test');
268+
expect(response?.status).toBe(302);
269+
});
270+
255271
it('should handle request abortion gracefully', async () => {
256272
const controller = new AbortController();
257273
const request = new Request('http://localhost/home', { signal: controller.signal });

‎packages/angular/ssr/test/routes/ng-routes_spec.ts‎

Lines changed: 78 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -489,6 +489,38 @@ describe('extractRoutesAndCreateRouteTree', () => {
489489
{ route: '/new', renderMode: RenderMode.Server },
490490
]);
491491
});
492+
493+
it('should resolve route parameters of redirect targets of prerendered routes', async () => {
494+
setAngularAppTestingManifest(
495+
[
496+
{ path: 'user/:id', redirectTo: '/profile/:id' },
497+
{ path: 'profile/:id', component: DummyComponent },
498+
],
499+
[
500+
{
501+
path: 'user/:id',
502+
renderMode: RenderMode.Prerender,
503+
async getPrerenderParams() {
504+
return [{ id: '7' }];
505+
},
506+
},
507+
{ path: '**', renderMode: RenderMode.Server },
508+
],
509+
);
510+
511+
const { routeTree, errors } = await extractRoutesAndCreateRouteTree({
512+
url,
513+
invokeGetPrerenderParams: true,
514+
includePrerenderFallbackRoutes: true,
515+
});
516+
517+
expect(errors).toHaveSize(0);
518+
expect(routeTree.toObject()).toEqual([
519+
{ route: '/user/7', renderMode: RenderMode.Prerender, redirectTo: '/profile/7' },
520+
{ route: '/user/*', renderMode: RenderMode.Server, redirectTo: '/profile/*2' },
521+
{ route: '/profile/*', renderMode: RenderMode.Server },
522+
]);
523+
});
492524
});
493525

494526
it('should extract routes with a route level matcher', async () => {
@@ -652,6 +684,52 @@ describe('extractRoutesAndCreateRouteTree', () => {
652684
]);
653685
});
654686

687+
it('should reference route parameters of redirect targets by their position', async () => {
688+
setAngularAppTestingManifest(
689+
[
690+
{
691+
path: ':lang',
692+
children: [
693+
{ path: 'user/:id', redirectTo: 'profile/:id' },
694+
{ path: 'profile/:id', component: DummyComponent },
695+
],
696+
},
697+
{ path: 'search/:term', redirectTo: '/find/:term' },
698+
],
699+
[{ path: '**', renderMode: RenderMode.Server }],
700+
);
701+
702+
const { routeTree, errors } = await extractRoutesAndCreateRouteTree({ url });
703+
expect(errors).toHaveSize(0);
704+
expect(routeTree.toObject()).toEqual([
705+
{ route: '/*', renderMode: RenderMode.Server },
706+
{
707+
route: '/*/user/*',
708+
renderMode: RenderMode.Server,
709+
redirectTo: '/*/profile/*3',
710+
relativeRedirect: true,
711+
},
712+
{ route: '/*/profile/*', renderMode: RenderMode.Server },
713+
{ route: '/search/*', renderMode: RenderMode.Server, redirectTo: '/find/*2' },
714+
]);
715+
});
716+
717+
it('should error when a redirect target uses a parameter not defined by the route', async () => {
718+
setAngularAppTestingManifest(
719+
[
720+
{
721+
path: ':lang',
722+
children: [{ path: 'user/:id', redirectTo: 'profile/:lang' }],
723+
},
724+
],
725+
[{ path: '**', renderMode: RenderMode.Server }],
726+
);
727+
728+
const { errors } = await extractRoutesAndCreateRouteTree({ url });
729+
expect(errors).toHaveSize(1);
730+
expect(errors[0]).toContain(`Cannot redirect to 'profile/:lang'. Cannot find ':lang'.`);
731+
});
732+
655733
it('should resolve multi-segment relative redirects against the parent route', async () => {
656734
setAngularAppTestingManifest(
657735
[

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

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -184,6 +184,15 @@ describe('URL Utils', () => {
184184
expect(result.toString()).toBe('/base/path/details');
185185
});
186186

187+
it('should replace positional placeholders with the segments at those positions', () => {
188+
expect(buildPathWithParams('/user/*2/*1', '/a/b')).toBe('/user/b/a');
189+
});
190+
191+
it('should replace a positional placeholder followed by a query or fragment', () => {
192+
expect(buildPathWithParams('/user/*1?returnTo=/a//b/', '/a')).toBe('/user/a?returnTo=/a//b/');
193+
expect(buildPathWithParams('/user/*1#top', '/a')).toBe('/user/a#top');
194+
});
195+
187196
it('should throw an error if the toPath does not start with "/"', () => {
188197
const fromPath = '/base/path';
189198
const toPath = 'details';

0 commit comments

Comments
 (0)