From 04222d64d994ff81caf4b55941fc1b450a5ecba5 Mon Sep 17 00:00:00 2001 From: SkyZeroZx <73321943+SkyZeroZx@users.noreply.github.com> Date: Thu, 15 Oct 2026 13:09:40 -0500 Subject: [PATCH 1/2] fix(@angular/ssr): use only the first value of the `X-Forwarded-Prefix` header The `X-Forwarded-Prefix` header is validated using its first comma-separated value, like the other `X-Forwarded-*` headers. However, redirects used the entire header value. With a trusted header, any value after the first one was added to the `Location` header without being validated. The first value is now used wherever the header is read, which matches how `X-Forwarded-Host`, `X-Forwarded-Proto` and `X-Forwarded-Port` are handled. --- packages/angular/ssr/src/app-engine.ts | 3 ++- packages/angular/ssr/src/app.ts | 3 ++- packages/angular/ssr/src/utils/ng.ts | 5 ++++- packages/angular/ssr/test/app-engine_spec.ts | 13 +++++++++++++ packages/angular/ssr/test/app_spec.ts | 10 ++++++++++ 5 files changed, 31 insertions(+), 3 deletions(-) diff --git a/packages/angular/ssr/src/app-engine.ts b/packages/angular/ssr/src/app-engine.ts index a5f10fbc7d92..1fa69c1f2629 100644 --- a/packages/angular/ssr/src/app-engine.ts +++ b/packages/angular/ssr/src/app-engine.ts @@ -13,6 +13,7 @@ import { EntryPointExports, getAngularAppEngineManifest } from './manifest'; import { createRedirectResponse } from './utils/redirect'; import { joinUrlParts } from './utils/url'; import { + getFirstHeaderValue, normalizeTrustProxyHeaders, sanitizeRequestHeaders, validateRequest, @@ -208,7 +209,7 @@ 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), diff --git a/packages/angular/ssr/src/app.ts b/packages/angular/ssr/src/app.ts index 517cf1e31727..69d5f9e6e100 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. @@ -167,7 +168,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), ); 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/test/app-engine_spec.ts b/packages/angular/ssr/test/app-engine_spec.ts index eae063eec700..db03015b85f1 100644 --- a/packages/angular/ssr/test/app-engine_spec.ts +++ b/packages/angular/ssr/test/app-engine_spec.ts @@ -202,6 +202,19 @@ 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 completely ignore proxy headers if not allowed', async () => { const strictEngine = new AngularAppEngine({ trustProxyHeaders: false }); const request = new Request('https://example.com', { diff --git a/packages/angular/ssr/test/app_spec.ts b/packages/angular/ssr/test/app_spec.ts index 8d8ee9add00b..68e3ed2eb1c2 100644 --- a/packages/angular/ssr/test/app_spec.ts +++ b/packages/angular/ssr/test/app_spec.ts @@ -538,6 +538,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', () => { From de0085d0dfaf6eb22ed782a1b518685c26c5cdd2 Mon Sep 17 00:00:00 2001 From: SkyZeroZx <73321943+SkyZeroZx@users.noreply.github.com> Date: Thu, 15 Oct 2026 13:15:21 -0500 Subject: [PATCH 2/2] fix(@angular/ssr): limit the length of redirect locations Redirects created on the server can contain data from the request. Route redirects keep the path and the query string, redirects done by the router while rendering use its final URL, and the `X-Forwarded-Prefix` header is prepended. The router also encodes some characters again, so the `Location` header can be several times longer than the request URL. Reverse proxies read the response headers into a buffer of limited size, 4 KB by default in NGINX. When the headers do not fit, the proxy returns a 502 and, with passive health checks, counts it against the server. A few requests could take every server out of rotation. `AngularAppEngine` and `AngularNodeAppEngine` now return a 400 when a redirect location is longer than 2048 characters. The limit can be changed with the new `maxRedirectLocationLength` option. Redirects created while prerendering are not limited. Fixes #34365 --- goldens/public-api/angular/ssr/index.api.md | 1 + packages/angular/ssr/src/app-engine.ts | 35 ++++++++- packages/angular/ssr/src/app.ts | 16 +++- packages/angular/ssr/src/utils/redirect.ts | 33 +++++++++ packages/angular/ssr/test/app-engine_spec.ts | 74 ++++++++++++++++++- packages/angular/ssr/test/app_spec.ts | 53 +++++++++++++ .../angular/ssr/test/utils/redirect_spec.ts | 35 +++++++++ 7 files changed, 244 insertions(+), 3 deletions(-) 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 1fa69c1f2629..2d12e5aba458 100644 --- a/packages/angular/ssr/src/app-engine.ts +++ b/packages/angular/ssr/src/app-engine.ts @@ -10,7 +10,7 @@ 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, @@ -46,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; } /** @@ -107,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. */ @@ -119,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 { @@ -216,6 +247,7 @@ export class AngularAppEngine { 302, // Use a 302 redirect as language preference may change. { 'Vary': 'Accept-Language' }, + this.#maxRedirectLocationLength, ); } } @@ -250,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 69d5f9e6e100..63d3daa1a6eb 100644 --- a/packages/angular/ssr/src/app.ts +++ b/packages/angular/ssr/src/app.ts @@ -77,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; } /** @@ -177,6 +185,7 @@ export class AngularServerApp { relativeRedirect ? location + url.search : location, status, headers, + this.options.maxRedirectLocationLength, ); } @@ -330,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/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 db03015b85f1..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({ @@ -215,6 +216,41 @@ describe('AngularAppEngine', () => { 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', { @@ -312,7 +348,10 @@ describe('AngularAppEngine', () => { entryPoints: { '': async () => { setAngularAppTestingManifest( - [{ path: 'home', component: HomeComponent }], + [ + { path: 'home', component: HomeComponent }, + { path: 'redirect', redirectTo: 'home' }, + ], [{ path: '**', renderMode: RenderMode.Server }], ); @@ -352,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 68e3ed2eb1c2..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')); 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;