Skip to content

Commit f2cd8c8

Browse files
bschaeublinbschaeublin
authored
fix(@angular/ssr): preserve query parameters on relative route redirects
Route redirects are answered on the server from the route configuration, without running the router. For a relative `redirectTo`, the router keeps the query string of the current URL and ignores a query string in `redirectTo`, but the server did the opposite. It now does the same as the router. Absolute redirects are unchanged, as the router does not keep the query string of the current URL for them either.
1 parent 1392304 commit f2cd8c8

6 files changed

Lines changed: 146 additions & 14 deletions

File tree

‎packages/angular/ssr/src/app.ts‎

Lines changed: 8 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -163,14 +163,17 @@ export class AngularServerApp {
163163
return null;
164164
}
165165

166-
const { redirectTo, status, renderMode, headers } = matchedRoute;
166+
const { redirectTo, relativeRedirect, status, renderMode, headers } = matchedRoute;
167167

168168
if (redirectTo !== undefined) {
169+
const location = joinUrlParts(
170+
request.headers.get('X-Forwarded-Prefix') ?? '',
171+
buildPathWithParams(redirectTo, url.pathname),
172+
);
173+
174+
// Match Angular router behavior: relative redirects keep the query string.
169175
return createRedirectResponse(
170-
joinUrlParts(
171-
request.headers.get('X-Forwarded-Prefix') ?? '',
172-
buildPathWithParams(redirectTo, url.pathname),
173-
),
176+
relativeRedirect ? location + url.search : location,
174177
status,
175178
headers,
176179
);

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

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -192,6 +192,7 @@ async function* handleRoute(options: {
192192
yield {
193193
...metadata,
194194
redirectTo: resolveRedirectTo(metadata.route, redirectTo),
195+
relativeRedirect: redirectTo[0] !== '/' ? true : undefined,
195196
};
196197
} else {
197198
yield metadata;
@@ -421,6 +422,7 @@ async function* handleSSGRoute(
421422

422423
if (redirectTo !== undefined) {
423424
meta.redirectTo = resolveRedirectTo(currentRoutePath, redirectTo);
425+
meta.relativeRedirect = redirectTo[0] !== '/' ? true : undefined;
424426
}
425427

426428
const isCatchAllRoute = CATCH_ALL_REGEXP.test(currentRoutePath);
@@ -534,7 +536,8 @@ function handlePrerenderParamsReplacement(
534536
*
535537
* This function processes the `redirectTo` property to ensure that it correctly
536538
* resolves relative to the current route path. If `redirectTo` is an absolute path,
537-
* it is returned as is. If it is a relative path, it is resolved based on the current route path.
539+
* it is returned as is. If it is a relative path, it is resolved based on the current route path,
540+
* and, like in the Angular router, its query and fragment are ignored.
538541
*
539542
* @param routePath - The current route path.
540543
* @param redirectTo - The target path for redirection.
@@ -550,7 +553,9 @@ function resolveRedirectTo(routePath: string, redirectTo: string): string {
550553
const segments = routePath.replace(URL_PARAMETER_GLOBAL_REGEXP, '*').split('/');
551554
segments.pop(); // Remove the last segment to make it relative.
552555

553-
return joinUrlParts(...segments, redirectTo);
556+
const [path] = redirectTo.split(/[?#]/, 1);
557+
558+
return joinUrlParts(...segments, path);
554559
}
555560

556561
/**

‎packages/angular/ssr/src/routes/route-tree.ts‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,9 @@ export interface RouteTreeNodeMetadata {
3131
*/
3232
redirectTo?: string;
3333

34+
/** Indicates if `redirectTo` was defined as a relative path. */
35+
relativeRedirect?: boolean;
36+
3437
/**
3538
* The route path for this node.
3639
*

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

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -88,6 +88,7 @@ describe('AngularServerApp', () => {
8888
{ path: 'redirect/relative', redirectTo: 'home' },
8989
{ path: 'redirect/:param/relative', redirectTo: 'home' },
9090
{ path: 'redirect/absolute', redirectTo: '/home' },
91+
{ path: 'redirect-with-query', redirectTo: 'home?param=value' },
9192
{
9293
path: 'redirect-to-function',
9394
redirectTo: () => 'home',
@@ -222,6 +223,28 @@ describe('AngularServerApp', () => {
222223
expect(response?.status).toBe(302);
223224
});
224225

226+
it('should preserve query parameters on relative redirects', async () => {
227+
const response = await app.handle(new Request('http://localhost/redirect?filter=test'));
228+
expect(response?.headers.get('location')).toBe('/home?filter=test');
229+
expect(response?.status).toBe(302);
230+
});
231+
232+
it('should ignore query parameters of relative redirect targets', async () => {
233+
const response = await app.handle(
234+
new Request('http://localhost/redirect-with-query?filter=test'),
235+
);
236+
expect(response?.headers.get('location')).toBe('/home?filter=test');
237+
expect(response?.status).toBe(302);
238+
});
239+
240+
it('should not preserve query parameters on absolute redirects', async () => {
241+
const response = await app.handle(
242+
new Request('http://localhost/redirect/absolute?filter=test'),
243+
);
244+
expect(response?.headers.get('location')).toBe('/home');
245+
expect(response?.status).toBe(302);
246+
});
247+
225248
it('should handle request abortion gracefully', async () => {
226249
const controller = new AbortController();
227250
const request = new Request('http://localhost/home', { signal: controller.signal });
@@ -498,6 +521,16 @@ describe('AngularServerApp', () => {
498521
expect(response?.headers.get('location')).toBe('/base/home');
499522
expect(response?.status).toBe(302);
500523
});
524+
525+
it('returns a 302 status and keeps the query string for relative redirects', async () => {
526+
const response = await app.handle(
527+
new Request('http://localhost/redirect?filter=test', {
528+
headers,
529+
}),
530+
);
531+
expect(response?.headers.get('location')).toBe('/base/home?filter=test');
532+
expect(response?.status).toBe(302);
533+
});
501534
});
502535

503536
describe('Critical CSS inlining', () => {

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

Lines changed: 92 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -51,7 +51,13 @@ describe('extractRoutesAndCreateRouteTree', () => {
5151
expect(routeTree.toObject()).toEqual([
5252
{ route: '/', renderMode: RenderMode.Server },
5353
{ route: '/home', renderMode: RenderMode.Client },
54-
{ route: '/redirect', renderMode: RenderMode.Server, status: 301, redirectTo: '/home' },
54+
{
55+
route: '/redirect',
56+
renderMode: RenderMode.Server,
57+
status: 301,
58+
redirectTo: '/home',
59+
relativeRedirect: true,
60+
},
5561
{ route: '/user/*', renderMode: RenderMode.Server },
5662
]);
5763
});
@@ -371,10 +377,25 @@ describe('extractRoutesAndCreateRouteTree', () => {
371377

372378
expect(errors).toHaveSize(0);
373379
expect(routeTree.toObject()).toEqual([
374-
{ route: '/', renderMode: RenderMode.Prerender, redirectTo: '/some' },
375-
{ route: '/some', renderMode: RenderMode.Prerender, redirectTo: '/some/thing' },
380+
{
381+
route: '/',
382+
renderMode: RenderMode.Prerender,
383+
redirectTo: '/some',
384+
relativeRedirect: true,
385+
},
386+
{
387+
route: '/some',
388+
renderMode: RenderMode.Prerender,
389+
redirectTo: '/some/thing',
390+
relativeRedirect: true,
391+
},
376392
{ route: '/some/thing', renderMode: RenderMode.Prerender },
377-
{ redirectTo: '/*/thing', route: '/*', renderMode: RenderMode.Server },
393+
{
394+
redirectTo: '/*/thing',
395+
relativeRedirect: true,
396+
route: '/*',
397+
renderMode: RenderMode.Server,
398+
},
378399
{ route: '/*/thing', renderMode: RenderMode.Server },
379400
]);
380401
});
@@ -496,8 +517,13 @@ describe('extractRoutesAndCreateRouteTree', () => {
496517
const { routeTree, errors } = await extractRoutesAndCreateRouteTree({ url });
497518
expect(errors).toHaveSize(0);
498519
expect(routeTree.toObject()).toEqual([
499-
{ route: '/', renderMode: RenderMode.Server, redirectTo: '/some' },
500-
{ route: '/*', renderMode: RenderMode.Server, redirectTo: '/*/thing' },
520+
{ route: '/', renderMode: RenderMode.Server, redirectTo: '/some', relativeRedirect: true },
521+
{
522+
route: '/*',
523+
renderMode: RenderMode.Server,
524+
redirectTo: '/*/thing',
525+
relativeRedirect: true,
526+
},
501527
{ route: '/*/thing', renderMode: RenderMode.Server },
502528
]);
503529
});
@@ -526,11 +552,70 @@ describe('extractRoutesAndCreateRouteTree', () => {
526552
const { routeTree, errors } = await extractRoutesAndCreateRouteTree({ url });
527553
expect(errors).toHaveSize(0);
528554
expect(routeTree.toObject()).toEqual([
529-
{ route: '/*/*', renderMode: RenderMode.Server, redirectTo: '/*/*/thing' },
555+
{
556+
route: '/*/*',
557+
renderMode: RenderMode.Server,
558+
redirectTo: '/*/*/thing',
559+
relativeRedirect: true,
560+
},
530561
{ route: '/*/*/thing', renderMode: RenderMode.Server },
531562
]);
532563
});
533564

565+
it('should not mark absolute redirects as relative', async () => {
566+
setAngularAppTestingManifest(
567+
[
568+
{ path: 'server', redirectTo: '/home' },
569+
{ path: 'prerender', redirectTo: '/home' },
570+
{ path: 'home', component: DummyComponent },
571+
],
572+
[
573+
{ path: 'prerender', renderMode: RenderMode.Prerender },
574+
{ path: '**', renderMode: RenderMode.Server },
575+
],
576+
);
577+
578+
const { routeTree, errors } = await extractRoutesAndCreateRouteTree({ url });
579+
expect(errors).toHaveSize(0);
580+
expect(routeTree.toObject()).toEqual([
581+
{ route: '/server', renderMode: RenderMode.Server, redirectTo: '/home' },
582+
{ route: '/prerender', renderMode: RenderMode.Prerender, redirectTo: '/home' },
583+
{ route: '/home', renderMode: RenderMode.Server },
584+
]);
585+
});
586+
587+
it('should ignore the query and fragment of relative redirect targets', async () => {
588+
setAngularAppTestingManifest(
589+
[
590+
{ path: 'server', redirectTo: 'home?param=value#fragment' },
591+
{ path: 'prerender', redirectTo: 'home?param=value#fragment' },
592+
{ path: 'home', component: DummyComponent },
593+
],
594+
[
595+
{ path: 'prerender', renderMode: RenderMode.Prerender },
596+
{ path: '**', renderMode: RenderMode.Server },
597+
],
598+
);
599+
600+
const { routeTree, errors } = await extractRoutesAndCreateRouteTree({ url });
601+
expect(errors).toHaveSize(0);
602+
expect(routeTree.toObject()).toEqual([
603+
{
604+
route: '/server',
605+
renderMode: RenderMode.Server,
606+
redirectTo: '/home',
607+
relativeRedirect: true,
608+
},
609+
{
610+
route: '/prerender',
611+
renderMode: RenderMode.Prerender,
612+
redirectTo: '/home',
613+
relativeRedirect: true,
614+
},
615+
{ route: '/home', renderMode: RenderMode.Server },
616+
]);
617+
});
618+
534619
it('should not resolve parameterized routes for SSG when `invokeGetPrerenderParams` is false', async () => {
535620
setAngularAppTestingManifest(
536621
[

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

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -55,6 +55,7 @@ describe('ServerRouter', () => {
5555
});
5656
expect(router.match(new URL('http://localhost/redirect'))).toEqual({
5757
redirectTo: '/home',
58+
relativeRedirect: true,
5859
route: '/redirect',
5960
renderMode: RenderMode.Server,
6061
status: 301,
@@ -89,6 +90,7 @@ describe('ServerRouter', () => {
8990
});
9091
expect(redirectMetadata).toEqual({
9192
redirectTo: '/home',
93+
relativeRedirect: true,
9294
route: '/redirect',
9395
status: 301,
9496
renderMode: RenderMode.Server,
@@ -110,6 +112,7 @@ describe('ServerRouter', () => {
110112
});
111113
expect(redirectMetadata).toEqual({
112114
redirectTo: '/home',
115+
relativeRedirect: true,
113116
route: '/redirect',
114117
status: 301,
115118
renderMode: RenderMode.Server,

0 commit comments

Comments
 (0)