Skip to content

fix(@angular/ssr): return 302 when redirectTo is a function #30154

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Apr 24, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 28 additions & 11 deletions packages/angular/ssr/src/app.ts
Original file line number Diff line number Diff line change
Expand Up @@ -174,15 +174,7 @@ export class AngularServerApp {

const { redirectTo, status, renderMode } = matchedRoute;
if (redirectTo !== undefined) {
return new Response(null, {
// Note: The status code is validated during route extraction.
// 302 Found is used by default for redirections
// See: https://developer.mozilla.org/en-US/docs/Web/API/Response/redirect_static#status
status: status ?? 302,
headers: {
'Location': buildPathWithParams(redirectTo, url.pathname),
},
});
return createRedirectResponse(buildPathWithParams(redirectTo, url.pathname), status);
}

if (renderMode === RenderMode.Prerender) {
Expand Down Expand Up @@ -324,20 +316,28 @@ export class AngularServerApp {
let html = await assets.getIndexServerHtml().text();
html = await this.runTransformsOnHtml(html, url, preload);

const { content } = await renderAngular(
const result = await renderAngular(
html,
this.boostrap,
url,
platformProviders,
SERVER_CONTEXT_VALUE[renderMode],
);

if (result.hasNavigationError) {
return null;
}

if (result.redirectTo) {
return createRedirectResponse(result.redirectTo, status);
}

const { inlineCriticalCssProcessor, criticalCssLRUCache, textDecoder } = this;

// Use a stream to send the response before finishing rendering and inling critical CSS, improving performance via header flushing.
const stream = new ReadableStream({
async start(controller) {
const renderedHtml = await content();
const renderedHtml = await result.content();

if (!inlineCriticalCssProcessor) {
controller.enqueue(textDecoder.encode(renderedHtml));
Expand Down Expand Up @@ -484,3 +484,20 @@ function appendPreloadHintsToHtml(html: string, preload: readonly string[]): str
html.slice(bodyCloseIdx),
].join('\n');
}

/**
* Creates an HTTP redirect response with a specified location and status code.
*
* @param location - The URL to which the response should redirect.
* @param status - The HTTP status code for the redirection. Defaults to 302 (Found).
* See: https://developer.mozilla.org/en-US/docs/Web/API/Response/redirect_static#status
* @returns A `Response` object representing the HTTP redirect.
*/
function createRedirectResponse(location: string, status = 302): Response {
return new Response(null, {
status,
headers: {
'Location': location,
},
});
}
9 changes: 7 additions & 2 deletions packages/angular/ssr/src/routes/ng-routes.ts
Original file line number Diff line number Diff line change
Expand Up @@ -160,15 +160,20 @@ async function* handleRoute(options: {
invokeGetPrerenderParams,
includePrerenderFallbackRoutes,
);
} else if (typeof redirectTo === 'string') {
} else if (redirectTo !== undefined) {
if (metadata.status && !VALID_REDIRECT_RESPONSE_CODES.has(metadata.status)) {
yield {
error:
`The '${metadata.status}' status code is not a valid redirect response code. ` +
`Please use one of the following redirect response codes: ${[...VALID_REDIRECT_RESPONSE_CODES.values()].join(', ')}.`,
};
} else if (typeof redirectTo === 'string') {
yield {
...metadata,
redirectTo: resolveRedirectTo(metadata.route, redirectTo),
};
} else {
yield { ...metadata, redirectTo: resolveRedirectTo(metadata.route, redirectTo) };
yield metadata;
}
} else {
yield metadata;
Expand Down
63 changes: 47 additions & 16 deletions packages/angular/ssr/src/utils/ng.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
* found in the LICENSE file at https://angular.dev/license
*/

import { APP_BASE_HREF, PlatformLocation } from '@angular/common';
import {
ApplicationRef,
type PlatformRef,
Expand All @@ -19,8 +20,9 @@ import {
platformServer,
ɵrenderInternal as renderInternal,
} from '@angular/platform-server';
import { Router } from '@angular/router';
import { Console } from '../console';
import { stripIndexHtmlFromURL } from './url';
import { joinUrlParts, stripIndexHtmlFromURL } from './url';

/**
* Represents the bootstrap mechanism for an Angular application.
Expand All @@ -35,28 +37,25 @@ export type AngularBootstrap = Type<unknown> | (() => Promise<ApplicationRef>);
* Renders an Angular application or module to an HTML string.
*
* This function determines whether the provided `bootstrap` value is an Angular module
* or a bootstrap function and calls the appropriate rendering method (`renderModule` or
* `renderApplication`) based on that determination.
* or a bootstrap function and invokes the appropriate rendering method (`renderModule` or `renderApplication`).
*
* @param html - The HTML string to be used as the initial document content.
* @param bootstrap - Either an Angular module type or a function that returns a promise
* resolving to an `ApplicationRef`.
* @param url - The URL of the application. This is used for server-side rendering to
* correctly handle route-based rendering.
* @param platformProviders - An array of platform providers to be used during the
* rendering process.
* @param serverContext - A string representing the server context, used to provide additional
* context or metadata during server-side rendering.
* @returns A promise resolving to an object containing a `content` method, which returns a
* promise that resolves to the rendered HTML string.
* @param html - The initial HTML document content.
* @param bootstrap - An Angular module type or a function returning a promise that resolves to an `ApplicationRef`.
* @param url - The application URL, used for route-based rendering in SSR.
* @param platformProviders - An array of platform providers for the rendering process.
* @param serverContext - A string representing the server context, providing additional metadata for SSR.
* @returns A promise resolving to an object containing:
* - `hasNavigationError`: Indicates if a navigation error occurred.
* - `redirectTo`: (Optional) The redirect URL if a navigation redirect occurred.
* - `content`: A function returning a promise that resolves to the rendered HTML string.
*/
export async function renderAngular(
html: string,
bootstrap: AngularBootstrap,
url: URL,
platformProviders: StaticProvider[],
serverContext: string,
): Promise<{ content: () => Promise<string> }> {
): Promise<{ hasNavigationError: boolean; redirectTo?: string; content: () => Promise<string> }> {
// A request to `http://www.example.com/page/index.html` will render the Angular route corresponding to `http://www.example.com/page`.
const urlToRender = stripIndexHtmlFromURL(url).toString();
const platformRef = platformServer([
Expand All @@ -82,6 +81,9 @@ export async function renderAngular(
...platformProviders,
]);

let redirectTo: string | undefined;
let hasNavigationError = true;

try {
let applicationRef: ApplicationRef;
if (isNgModule(bootstrap)) {
Expand All @@ -94,7 +96,29 @@ export async function renderAngular(
// Block until application is stable.
await applicationRef.whenStable();

// TODO(alanagius): Find a way to avoid rendering here especially for redirects as any output will be discarded.
const envInjector = applicationRef.injector;
const router = envInjector.get(Router);
const lastSuccessfulNavigation = router.lastSuccessfulNavigation;

if (lastSuccessfulNavigation?.finalUrl) {
hasNavigationError = false;

const { finalUrl, initialUrl } = lastSuccessfulNavigation;
const finalUrlStringified = finalUrl.toString();

if (initialUrl.toString() !== finalUrlStringified) {
const baseHref =
envInjector.get(APP_BASE_HREF, null, { optional: true }) ??
envInjector.get(PlatformLocation).getBaseHrefFromDOM();

redirectTo = joinUrlParts(baseHref, finalUrlStringified);
}
}

return {
hasNavigationError,
redirectTo,
content: () =>
new Promise<string>((resolve, reject) => {
// Defer rendering to the next event loop iteration to avoid blocking, as most operations in `renderInternal` are synchronous.
Expand All @@ -110,6 +134,10 @@ export async function renderAngular(
await asyncDestroyPlatform(platformRef);

throw error;
} finally {
if (hasNavigationError || redirectTo) {
void asyncDestroyPlatform(platformRef);
}
}
}

Expand All @@ -134,7 +162,10 @@ export function isNgModule(value: AngularBootstrap): value is Type<unknown> {
function asyncDestroyPlatform(platformRef: PlatformRef): Promise<void> {
return new Promise((resolve) => {
setTimeout(() => {
platformRef.destroy();
if (!platformRef.destroyed) {
platformRef.destroy();
}

resolve();
}, 0);
});
Expand Down
21 changes: 17 additions & 4 deletions packages/angular/ssr/test/app_spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,11 @@ describe('AngularServerApp', () => {
{ path: 'redirect/relative', redirectTo: 'home' },
{ path: 'redirect/:param/relative', redirectTo: 'home' },
{ path: 'redirect/absolute', redirectTo: '/home' },
{
path: 'redirect-to-function',
redirectTo: () => 'home',
pathMatch: 'full',
},
],
[
{
Expand Down Expand Up @@ -106,25 +111,25 @@ describe('AngularServerApp', () => {

it('should correctly handle top level redirects', async () => {
const response = await app.handle(new Request('http://localhost/redirect'));
expect(response?.headers.get('location')).toContain('/home');
expect(response?.headers.get('location')).toBe('/home');
expect(response?.status).toBe(302);
});

it('should correctly handle relative nested redirects', async () => {
const response = await app.handle(new Request('http://localhost/redirect/relative'));
expect(response?.headers.get('location')).toContain('/redirect/home');
expect(response?.headers.get('location')).toBe('/redirect/home');
expect(response?.status).toBe(302);
});

it('should correctly handle relative nested redirects with parameter', async () => {
const response = await app.handle(new Request('http://localhost/redirect/param/relative'));
expect(response?.headers.get('location')).toContain('/redirect/param/home');
expect(response?.headers.get('location')).toBe('/redirect/param/home');
expect(response?.status).toBe(302);
});

it('should correctly handle absolute nested redirects', async () => {
const response = await app.handle(new Request('http://localhost/redirect/absolute'));
expect(response?.headers.get('location')).toContain('/home');
expect(response?.headers.get('location')).toBe('/home');
expect(response?.status).toBe(302);
});

Expand Down Expand Up @@ -253,5 +258,13 @@ describe('AngularServerApp', () => {
expect(response).toBeNull();
});
});

describe('SSR pages', () => {
it('returns a 302 status and redirects to the correct location when redirectTo is a function', async () => {
const response = await app.handle(new Request('http://localhost/redirect-to-function'));
expect(response?.headers.get('location')).toBe('/home');
expect(response?.status).toBe(302);
});
});
});
});
12 changes: 11 additions & 1 deletion packages/angular/ssr/test/routes/ng-routes_spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,16 @@ describe('extractRoutesAndCreateRouteTree', () => {
});

describe('route configuration validation', () => {
it('should error when a redirect uses an invalid status code', async () => {
setAngularAppTestingManifest(
[{ path: '', redirectTo: () => 'home', pathMatch: 'full' }],
[{ path: '', renderMode: RenderMode.Server, status: 404 }],
);

const { errors } = await extractRoutesAndCreateRouteTree({ url });
expect(errors[0]).toContain(`The '404' status code is not a valid redirect response code.`);
});

it(`should error when a route starts with a '/'`, async () => {
setAngularAppTestingManifest(
[{ path: 'home', component: DummyComponent }],
Expand Down Expand Up @@ -88,7 +98,7 @@ describe('extractRoutesAndCreateRouteTree', () => {
);
});

it(`should not error when a catch-all route didn't match any Angular route.`, async () => {
it(`should not error when a catch-all route didn't match any Angular route`, async () => {
setAngularAppTestingManifest(
[{ path: 'home', component: DummyComponent }],
[
Expand Down
Loading