diff --git a/goldens/public-api/angular/ssr/index.api.md b/goldens/public-api/angular/ssr/index.api.md index 852187d5290b..339f2b0b94af 100644 --- a/goldens/public-api/angular/ssr/index.api.md +++ b/goldens/public-api/angular/ssr/index.api.md @@ -22,6 +22,7 @@ export class AngularAppEngine { // @public export interface AngularAppEngineOptions { allowedHosts?: readonly string[]; + maxRedirectLocationLength?: number; trustProxyHeaders?: boolean | readonly string[]; } diff --git a/packages/angular/ssr/src/app-engine.ts b/packages/angular/ssr/src/app-engine.ts index a5f10fbc7d92..2d12e5aba458 100644 --- a/packages/angular/ssr/src/app-engine.ts +++ b/packages/angular/ssr/src/app-engine.ts @@ -10,9 +10,10 @@ import type { AngularServerApp, getOrCreateAngularServerApp } from './app'; import { Hooks } from './hooks'; import { getPotentialLocaleIdFromUrl, getPreferredLocale } from './i18n'; import { EntryPointExports, getAngularAppEngineManifest } from './manifest'; -import { createRedirectResponse } from './utils/redirect'; +import { DEFAULT_MAX_REDIRECT_LOCATION_LENGTH, createRedirectResponse } from './utils/redirect'; import { joinUrlParts } from './utils/url'; import { + getFirstHeaderValue, normalizeTrustProxyHeaders, sanitizeRequestHeaders, validateRequest, @@ -45,6 +46,22 @@ export interface AngularAppEngineOptions { * @default false */ trustProxyHeaders?: boolean | readonly string[]; + + /** + * The maximum length of the `Location` header in redirect responses. + * + * When the location of a redirect is longer, a 400 (Bad Request) response is returned instead. + * + * @remarks + * Redirect locations can contain data from the request, such as the path, the query string and + * the `X-Forwarded-Prefix` header. Reverse proxies and load balancers read the response headers + * into a buffer of limited size (4 KB by default in NGINX) and fail the request when they do not + * fit. Only increase this value when every proxy in front of the server accepts larger response + * headers. + * + * @default 2048 + */ + maxRedirectLocationLength?: number; } /** @@ -106,6 +123,11 @@ export class AngularAppEngine { */ private readonly trustProxyHeaders: ReadonlySet; + /** + * The maximum length of the `Location` header in redirect responses. + */ + readonly #maxRedirectLocationLength: number; + /** * A cache that holds entry points, keyed by their potential locale string. */ @@ -118,6 +140,16 @@ export class AngularAppEngine { constructor(options?: AngularAppEngineOptions) { this.allowedHosts = this.getAllowedHosts(options); this.trustProxyHeaders = normalizeTrustProxyHeaders(options?.trustProxyHeaders); + + const maxRedirectLocationLength = + options?.maxRedirectLocationLength ?? DEFAULT_MAX_REDIRECT_LOCATION_LENGTH; + if (!Number.isInteger(maxRedirectLocationLength) || maxRedirectLocationLength < 1) { + throw new Error( + `"maxRedirectLocationLength" must be a positive integer. Received: ${maxRedirectLocationLength}.`, + ); + } + + this.#maxRedirectLocationLength = maxRedirectLocationLength; } private getAllowedHosts(options: AngularAppEngineOptions | undefined): ReadonlySet { @@ -208,13 +240,14 @@ export class AngularAppEngine { if (preferredLocale) { const subPath = supportedLocales[preferredLocale]; if (subPath !== undefined) { - const prefix = request.headers.get('X-Forwarded-Prefix') ?? ''; + const prefix = getFirstHeaderValue(request.headers.get('X-Forwarded-Prefix')) ?? ''; return createRedirectResponse( joinUrlParts(prefix, pathname, subPath), 302, // Use a 302 redirect as language preference may change. { 'Vary': 'Accept-Language' }, + this.#maxRedirectLocationLength, ); } } @@ -249,6 +282,7 @@ export class AngularAppEngine { const serverApp = ɵgetOrCreateAngularServerApp({ allowStaticRouteRender: AngularAppEngine.ɵallowStaticRouteRender, hooks: AngularAppEngine.ɵhooks, + maxRedirectLocationLength: this.#maxRedirectLocationLength, }); return serverApp; diff --git a/packages/angular/ssr/src/app.ts b/packages/angular/ssr/src/app.ts index 517cf1e31727..63d3daa1a6eb 100644 --- a/packages/angular/ssr/src/app.ts +++ b/packages/angular/ssr/src/app.ts @@ -25,6 +25,7 @@ import { AngularBootstrap, renderAngular } from './utils/ng'; import { promiseWithAbort } from './utils/promise'; import { createRedirectResponse } from './utils/redirect'; import { buildPathWithParams, joinUrlParts, stripLeadingSlash } from './utils/url'; +import { getFirstHeaderValue } from './utils/validation'; /** * A set of well-known URLs that are not handled by Angular. @@ -76,6 +77,14 @@ interface AngularServerAppOptions { * If not provided, a new `Hooks` instance is created. */ hooks?: Hooks; + + /** + * The maximum length of the `Location` header in redirect responses. + * + * When a redirect location is longer, a 400 (Bad Request) response is returned instead. + * If not provided, the length is not limited, such as when prerendering. + */ + maxRedirectLocationLength?: number; } /** @@ -167,7 +176,7 @@ export class AngularServerApp { if (redirectTo !== undefined) { const location = joinUrlParts( - request.headers.get('X-Forwarded-Prefix') ?? '', + getFirstHeaderValue(request.headers.get('X-Forwarded-Prefix')) ?? '', buildPathWithParams(redirectTo, url.pathname), ); @@ -176,6 +185,7 @@ export class AngularServerApp { relativeRedirect ? location + url.search : location, status, headers, + this.options.maxRedirectLocationLength, ); } @@ -329,7 +339,12 @@ export class AngularServerApp { } if (result.redirectTo) { - return createRedirectResponse(result.redirectTo, responseInit.status, responseInit.headers); + return createRedirectResponse( + result.redirectTo, + responseInit.status, + responseInit.headers, + this.options.maxRedirectLocationLength, + ); } if (renderMode === RenderMode.Prerender) { diff --git a/packages/angular/ssr/src/utils/ng.ts b/packages/angular/ssr/src/utils/ng.ts index 4ae3a7f4ed0f..028136749b17 100644 --- a/packages/angular/ssr/src/utils/ng.ts +++ b/packages/angular/ssr/src/utils/ng.ts @@ -25,6 +25,7 @@ import { import { ActivatedRoute, Router } from '@angular/router'; import { Console } from '../console'; import { addTrailingSlash, joinUrlParts, stripIndexHtmlFromURL, stripTrailingSlash } from './url'; +import { getFirstHeaderValue } from './validation'; /** * Represents the bootstrap mechanism for an Angular application. @@ -126,7 +127,9 @@ export async function renderAngular( const requestPrefix = envInjector.get(APP_BASE_HREF, null, { optional: true }) ?? - envInjector.get(REQUEST, null, { optional: true })?.headers.get('X-Forwarded-Prefix'); + getFirstHeaderValue( + envInjector.get(REQUEST, null, { optional: true })?.headers.get('X-Forwarded-Prefix'), + ); const { pathname, search, hash } = envInjector.get(PlatformLocation); const finalUrl = constructSerializedUrl(router, { pathname, search, hash }, requestPrefix); diff --git a/packages/angular/ssr/src/utils/redirect.ts b/packages/angular/ssr/src/utils/redirect.ts index b67f53265c17..87c1b5b8bb22 100644 --- a/packages/angular/ssr/src/utils/redirect.ts +++ b/packages/angular/ssr/src/utils/redirect.ts @@ -13,6 +13,17 @@ export const VALID_REDIRECT_RESPONSE_CODES: ReadonlySet = new Set([ 301, 302, 303, 307, 308, ]); +/** + * The default maximum length of the `Location` header in redirect responses. + * + * Redirect locations can contain data from the request, such as the path, the query string and + * the `X-Forwarded-Prefix` header, which the Angular router can encode into a longer string than + * it received. Reverse proxies read the response headers into a buffer of limited size (4 KB by + * default in NGINX) and fail the request when they do not fit. The default leaves room in such a + * buffer for the status line and the other response headers. + */ +export const DEFAULT_MAX_REDIRECT_LOCATION_LENGTH = 2048; + /** * Checks if the given HTTP status code is a valid redirect response code. * @@ -30,12 +41,16 @@ export function isValidRedirectResponseCode(code: number): boolean { * @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 * @param headers - Additional headers to include in the response. + * @param maxLocationLength - The maximum allowed length of the location. When the location is + * longer, a 400 (Bad Request) response is returned instead. When not + * provided, the length of the location is not limited. * @returns A `Response` object representing the HTTP redirect. */ export function createRedirectResponse( location: string, status = 302, headers?: Record | Headers, + maxLocationLength?: number, ): Response { if (ngDevMode && !isValidRedirectResponseCode(status)) { throw new Error( @@ -44,6 +59,24 @@ export function createRedirectResponse( ); } + if (maxLocationLength !== undefined && location.length > maxLocationLength) { + const errorMessage = + `Redirect location is ${location.length} characters long, which exceeds ` + + `the maximum allowed length of ${maxLocationLength} characters.`; + + // eslint-disable-next-line no-console + console.error( + `ERROR: Bad Request.\n${errorMessage}\n` + + 'The limit can be changed with the "maxRedirectLocationLength" option of "AngularAppEngine" or "AngularNodeAppEngine".', + ); + + return new Response(errorMessage, { + status: 400, + statusText: 'Bad Request', + headers: { 'Content-Type': 'text/plain' }, + }); + } + const resHeaders = headers instanceof Headers ? headers : new Headers(headers); if (ngDevMode && resHeaders.has('location')) { // eslint-disable-next-line no-console diff --git a/packages/angular/ssr/test/app-engine_spec.ts b/packages/angular/ssr/test/app-engine_spec.ts index eae063eec700..c0c1e32a2080 100644 --- a/packages/angular/ssr/test/app-engine_spec.ts +++ b/packages/angular/ssr/test/app-engine_spec.ts @@ -16,6 +16,7 @@ import { destroyAngularServerApp, getOrCreateAngularServerApp } from '../src/app import { AngularAppEngine } from '../src/app-engine'; import { setAngularAppEngineManifest } from '../src/manifest'; import { RenderMode } from '../src/routes/route-config'; +import { DEFAULT_MAX_REDIRECT_LOCATION_LENGTH } from '../src/utils/redirect'; import { setAngularAppTestingManifest } from './testing-utils'; @Component({ @@ -202,6 +203,54 @@ describe('AngularAppEngine', () => { expect(response?.headers.get('Vary')).toBe('X-Forwarded-Prefix, Accept-Language'); }); + it('should only use the first value of the forwarded prefix in locale redirect location', async () => { + const request = new Request('https://example.com', { + headers: { + 'Accept-Language': 'it', + 'X-Forwarded-Prefix': '/app, /"', + }, + }); + + const response = await appEngine.handle(request); + expect(response?.status).toBe(302); + expect(response?.headers.get('Location')).toBe('/app/it'); + }); + + it('should return a 400 status when the locale redirect location is too long', async () => { + const consoleErrorSpy = spyOn(console, 'error'); + const request = new Request('https://example.com', { + headers: { + 'Accept-Language': 'it', + 'X-Forwarded-Prefix': '/' + 'a'.repeat(DEFAULT_MAX_REDIRECT_LOCATION_LENGTH), + }, + }); + + const response = await appEngine.handle(request); + expect(response?.status).toBe(400); + expect(response?.headers.get('Location')).toBeNull(); + expect(consoleErrorSpy).toHaveBeenCalledWith( + jasmine.stringMatching('exceeds the maximum allowed length of 2048 characters'), + ); + }); + + it('should allow longer locale redirect locations when "maxRedirectLocationLength" is set', async () => { + const prefix = '/' + 'a'.repeat(DEFAULT_MAX_REDIRECT_LOCATION_LENGTH); + const engine = new AngularAppEngine({ + trustProxyHeaders: true, + maxRedirectLocationLength: 4096, + }); + const request = new Request('https://example.com', { + headers: { + 'Accept-Language': 'it', + 'X-Forwarded-Prefix': prefix, + }, + }); + + const response = await engine.handle(request); + expect(response?.status).toBe(302); + expect(response?.headers.get('Location')).toBe(`${prefix}/it`); + }); + it('should completely ignore proxy headers if not allowed', async () => { const strictEngine = new AngularAppEngine({ trustProxyHeaders: false }); const request = new Request('https://example.com', { @@ -299,7 +348,10 @@ describe('AngularAppEngine', () => { entryPoints: { '': async () => { setAngularAppTestingManifest( - [{ path: 'home', component: HomeComponent }], + [ + { path: 'home', component: HomeComponent }, + { path: 'redirect', redirectTo: 'home' }, + ], [{ path: '**', renderMode: RenderMode.Server }], ); @@ -339,6 +391,39 @@ describe('AngularAppEngine', () => { const response = await appEngine.handle(request); expect(await response?.text()).toContain('Home works'); }); + + it('should return a 400 status when the redirect location exceeds the default maximum length', async () => { + const consoleErrorSpy = spyOn(console, 'error'); + const engine = new AngularAppEngine(); + const request = new Request( + `https://example.com/redirect?q=${'a'.repeat(DEFAULT_MAX_REDIRECT_LOCATION_LENGTH)}`, + ); + + const response = await engine.handle(request); + expect(response?.status).toBe(400); + expect(response?.headers.get('Location')).toBeNull(); + expect(consoleErrorSpy).toHaveBeenCalledWith( + jasmine.stringMatching('exceeds the maximum allowed length of 2048 characters'), + ); + }); + + it('should allow longer redirect locations when "maxRedirectLocationLength" is set', async () => { + const query = 'a'.repeat(DEFAULT_MAX_REDIRECT_LOCATION_LENGTH); + const engine = new AngularAppEngine({ maxRedirectLocationLength: 4096 }); + const request = new Request(`https://example.com/redirect?q=${query}`); + + const response = await engine.handle(request); + expect(response?.status).toBe(302); + expect(response?.headers.get('Location')).toBe(`/home?q=${query}`); + }); + + it('should throw an error when "maxRedirectLocationLength" is not a positive integer', () => { + for (const maxRedirectLocationLength of [0, -1, 1.5, NaN, Infinity]) { + expect(() => new AngularAppEngine({ maxRedirectLocationLength })) + .withContext(`maxRedirectLocationLength: ${maxRedirectLocationLength}`) + .toThrowError(/"maxRedirectLocationLength" must be a positive integer/); + } + }); }); describe('Invalid host headers', () => { diff --git a/packages/angular/ssr/test/app_spec.ts b/packages/angular/ssr/test/app_spec.ts index 8d8ee9add00b..4e665ba54d5e 100644 --- a/packages/angular/ssr/test/app_spec.ts +++ b/packages/angular/ssr/test/app_spec.ts @@ -313,6 +313,59 @@ describe('AngularServerApp', () => { }); }); + describe('redirect location length', () => { + let appWithLimit: AngularServerApp; + + beforeEach(() => { + // Other tests replace the manifest, which is read when the app is created. + setupManifest(); + appWithLimit = new AngularServerApp({ maxRedirectLocationLength: 100 }); + }); + + it('should not limit the length of redirect locations when "maxRedirectLocationLength" is not set', async () => { + const filter = 'a'.repeat(5000); + const response = await app.handle( + new Request(`http://localhost/redirect?filter=${filter}`), + ); + expect(response?.headers.get('location')).toBe(`/home?filter=${filter}`); + expect(response?.status).toBe(302); + }); + + it('should redirect when the location has the maximum allowed length', async () => { + // '/home?filter=' is 13 characters long. + const filter = 'a'.repeat(87); + const response = await appWithLimit.handle( + new Request(`http://localhost/redirect?filter=${filter}`), + ); + expect(response?.headers.get('location')).toBe(`/home?filter=${filter}`); + expect(response?.status).toBe(302); + }); + + it('should return a 400 status when the location of a route redirect is too long', async () => { + const consoleErrorSpy = spyOn(console, 'error'); + const response = await appWithLimit.handle( + new Request(`http://localhost/redirect?filter=${'a'.repeat(88)}`), + ); + expect(response?.headers.get('location')).toBeNull(); + expect(response?.status).toBe(400); + expect(await response?.text()).toContain( + 'exceeds the maximum allowed length of 100 characters.', + ); + expect(consoleErrorSpy).toHaveBeenCalledWith( + jasmine.stringMatching('Redirect location is 101 characters long'), + ); + }); + + it('should return a 400 status when the location of a redirect during rendering is too long', async () => { + spyOn(console, 'error'); + const response = await appWithLimit.handle( + new Request(`http://localhost/redirect-to-function?filter=${'a'.repeat(200)}`), + ); + expect(response?.headers.get('location')).toBeNull(); + expect(response?.status).toBe(400); + }); + }); + describe('SSG pages', () => { it('should correctly serve the content for the requested prerendered page', async () => { const response = await app.handle(new Request('http://localhost/home-ssg')); @@ -538,6 +591,16 @@ describe('AngularServerApp', () => { expect(response?.headers.get('location')).toBe('/base/home?filter=test'); expect(response?.status).toBe(302); }); + + it('should only use the first value of the "X-Forwarded-Prefix" header in redirects', async () => { + const response = await app.handle( + new Request('http://localhost/redirect', { + headers: { 'X-Forwarded-Prefix': '/base, /"' }, + }), + ); + expect(response?.headers.get('location')).toBe('/base/home'); + expect(response?.status).toBe(302); + }); }); describe('Critical CSS inlining', () => { diff --git a/packages/angular/ssr/test/utils/redirect_spec.ts b/packages/angular/ssr/test/utils/redirect_spec.ts index 4d0b94a87798..d8875edae91a 100644 --- a/packages/angular/ssr/test/utils/redirect_spec.ts +++ b/packages/angular/ssr/test/utils/redirect_spec.ts @@ -46,6 +46,41 @@ describe('Redirect Utils', () => { expect(response.headers.get('Vary')).toBe('X-Forwarded-Prefix, Host'); }); + it('should create a redirect response when the location has the maximum allowed length', () => { + const location = '/' + 'a'.repeat(99); + const response = createRedirectResponse(location, 302, undefined, 100); + expect(response.status).toBe(302); + expect(response.headers.get('Location')).toBe(location); + }); + + it('should return a 400 response when the location exceeds the maximum allowed length', async () => { + const errorSpy = spyOn(console, 'error'); + const response = createRedirectResponse( + '/' + 'a'.repeat(100), + 301, + { 'Cache-Control': 'public, max-age=3600' }, + 100, + ); + + expect(response.status).toBe(400); + expect(response.headers.get('Location')).toBeNull(); + expect(response.headers.get('Cache-Control')).toBeNull(); + expect(response.headers.get('Content-Type')).toBe('text/plain'); + expect(await response.text()).toBe( + 'Redirect location is 101 characters long, which exceeds the maximum allowed length of 100 characters.', + ); + expect(errorSpy).toHaveBeenCalledWith( + jasmine.stringMatching('Redirect location is 101 characters long'), + ); + }); + + it('should not limit the length of the location when no maximum length is provided', () => { + const location = '/' + 'a'.repeat(10_000); + const response = createRedirectResponse(location); + expect(response.status).toBe(302); + expect(response.headers.get('Location')).toBe(location); + }); + it('should warn if Location header is provided in extra headers in dev mode', () => { // @ts-expect-error accessing global globalThis.ngDevMode = true;