From 2cf19195d2374e5ca7a9f4646a6857f0828bd3b1 Mon Sep 17 00:00:00 2001 From: Alan Agius <17563226+alan-agius4@users.noreply.github.com> Date: Wed, 1 Apr 2026 12:55:04 +0000 Subject: [PATCH] fix(@angular/ssr): enforce explicit opt-in for proxy headers This commit introduces a secure-by-default model for trusting proxy headers (`X-Forwarded-*`) in the `@angular/ssr` package. Previously, the engine relied on complex lazy header patching and regex filters to guard against spoofed headers. However, implicit decoding behaviors by URL constructors can render naive regex filtering ineffective against certain percent-encoded payloads. To harden the engine against Server-Side Request Forgery (SSRF) and header-spoofing attacks: - Introduced the `allowedProxyHeaders` configuration option to `AngularAppEngineOptions` and `AngularNodeAppEngineOptions`. - By default (`false`), all incoming `X-Forwarded-*` headers are aggressively scrubbed unless explicitly whitelisted via `allowedProxyHeaders`. - Replaced the lazy `cloneRequestAndPatchHeaders` utility with a simplified, eager `sanitizeRequestHeaders` that centralizes the header scrubbing logic. - Hardened `verifyHostAllowed` to definitively reject parsed hosts that successfully carry path, search, hash, or auth components, replacing previously fallible regex filters for stringently checked hosts. BREAKING CHANGE: The `@angular/ssr` package now ignores all `X-Forwarded-*` proxy headers by default. If your application relies on these headers (e.g., for resolving absolute URLs, trust proxy, or custom proxy-related logic), you must explicitly allow them using the new `allowedProxyHeaders` option in the application server configuration. Example: ```ts const engine = new AngularAppEngine({ // Allow all proxy headers allowedProxyHeaders: true, }); // Or explicitly allow specific headers: const engine = new AngularAppEngine({ allowedProxyHeaders: ['x-forwarded-host', 'x-forwarded-prefix'], }); ``` --- goldens/public-api/angular/ssr/index.api.md | 1 + .../public-api/angular/ssr/node/index.api.md | 2 +- packages/angular/ssr/node/src/app-engine.ts | 6 +- packages/angular/ssr/node/src/request.ts | 97 ++++++++- .../angular/ssr/node/test/request_spec.ts | 2 + packages/angular/ssr/src/app-engine.ts | 53 +++-- packages/angular/ssr/src/utils/validation.ts | 178 +++++----------- packages/angular/ssr/test/app-engine_spec.ts | 25 ++- .../angular/ssr/test/utils/validation_spec.ts | 197 +++++------------- 9 files changed, 259 insertions(+), 302 deletions(-) diff --git a/goldens/public-api/angular/ssr/index.api.md b/goldens/public-api/angular/ssr/index.api.md index e5d85138b72f..a9d1d1b90176 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[]; + allowedProxyHeaders?: boolean | readonly string[]; } // @public diff --git a/goldens/public-api/angular/ssr/node/index.api.md b/goldens/public-api/angular/ssr/node/index.api.md index 2c52c06b47c1..52d0b31bd6e6 100644 --- a/goldens/public-api/angular/ssr/node/index.api.md +++ b/goldens/public-api/angular/ssr/node/index.api.md @@ -55,7 +55,7 @@ export interface CommonEngineRenderOptions { export function createNodeRequestHandler(handler: T): T; // @public -export function createWebRequestFromNodeRequest(nodeRequest: IncomingMessage | Http2ServerRequest): Request; +export function createWebRequestFromNodeRequest(nodeRequest: IncomingMessage | Http2ServerRequest, allowedProxyHeaders?: boolean | readonly string[]): Request; // @public export function isMainModule(url: string): boolean; diff --git a/packages/angular/ssr/node/src/app-engine.ts b/packages/angular/ssr/node/src/app-engine.ts index 7f7d3017a846..1945ed4f66a6 100644 --- a/packages/angular/ssr/node/src/app-engine.ts +++ b/packages/angular/ssr/node/src/app-engine.ts @@ -29,6 +29,7 @@ export interface AngularNodeAppEngineOptions extends AngularAppEngineOptions {} */ export class AngularNodeAppEngine { private readonly angularAppEngine: AngularAppEngine; + private readonly allowedProxyHeaders?: boolean | readonly string[]; /** * Creates a new instance of the Angular Node.js server application engine. @@ -39,6 +40,7 @@ export class AngularNodeAppEngine { ...options, allowedHosts: [...getAllowedHostsFromEnv(), ...(options?.allowedHosts ?? [])], }); + this.allowedProxyHeaders = options?.allowedProxyHeaders; attachNodeGlobalErrorHandlers(); } @@ -75,7 +77,9 @@ export class AngularNodeAppEngine { requestContext?: unknown, ): Promise { const webRequest = - request instanceof Request ? request : createWebRequestFromNodeRequest(request); + request instanceof Request + ? request + : createWebRequestFromNodeRequest(request, this.allowedProxyHeaders); return this.angularAppEngine.handle(webRequest, requestContext); } diff --git a/packages/angular/ssr/node/src/request.ts b/packages/angular/ssr/node/src/request.ts index 402ec29ba56d..06cefbdaabb5 100644 --- a/packages/angular/ssr/node/src/request.ts +++ b/packages/angular/ssr/node/src/request.ts @@ -17,7 +17,13 @@ import { getFirstHeaderValue } from '../../src/utils/validation'; * as they are not allowed to be set directly using the `Node.js` Undici API or * the web `Headers` API. */ -const HTTP2_PSEUDO_HEADERS = new Set([':method', ':scheme', ':authority', ':path', ':status']); +const HTTP2_PSEUDO_HEADERS: ReadonlySet = new Set([ + ':method', + ':scheme', + ':authority', + ':path', + ':status', +]); /** * Converts a Node.js `IncomingMessage` or `Http2ServerRequest` into a @@ -27,18 +33,31 @@ const HTTP2_PSEUDO_HEADERS = new Set([':method', ':scheme', ':authority', ':path * be used by web platform APIs. * * @param nodeRequest - The Node.js request object (`IncomingMessage` or `Http2ServerRequest`) to convert. + * @param allowedProxyHeaders - A boolean or an array of allowed proxy headers. + * + * @remarks + * When `allowedProxyHeaders` is enabled, headers such as `X-Forwarded-Host` and + * `X-Forwarded-Prefix` should ideally be strictly validated at a higher infrastructure + * level (e.g., at the reverse proxy or API gateway) before reaching the application. + * * @returns A Web Standard `Request` object. */ export function createWebRequestFromNodeRequest( nodeRequest: IncomingMessage | Http2ServerRequest, + allowedProxyHeaders?: boolean | readonly string[], ): Request { + const allowedProxyHeadersNormalized = + allowedProxyHeaders && typeof allowedProxyHeaders !== 'boolean' + ? new Set(allowedProxyHeaders.map((h) => h.toLowerCase())) + : allowedProxyHeaders; + const { headers, method = 'GET' } = nodeRequest; const withBody = method !== 'GET' && method !== 'HEAD'; const referrer = headers.referer && URL.canParse(headers.referer) ? headers.referer : undefined; - return new Request(createRequestUrl(nodeRequest), { + return new Request(createRequestUrl(nodeRequest, allowedProxyHeadersNormalized), { method, - headers: createRequestHeaders(headers), + headers: createRequestHeaders(headers, allowedProxyHeadersNormalized), body: withBody ? nodeRequest : undefined, duplex: withBody ? 'half' : undefined, referrer, @@ -49,9 +68,13 @@ export function createWebRequestFromNodeRequest( * Creates a `Headers` object from Node.js `IncomingHttpHeaders`. * * @param nodeHeaders - The Node.js `IncomingHttpHeaders` object to convert. + * @param allowedProxyHeaders - A boolean or a set of allowed proxy headers. * @returns A `Headers` object containing the converted headers. */ -function createRequestHeaders(nodeHeaders: IncomingHttpHeaders): Headers { +function createRequestHeaders( + nodeHeaders: IncomingHttpHeaders, + allowedProxyHeaders: boolean | ReadonlySet | undefined, +): Headers { const headers = new Headers(); for (const [name, value] of Object.entries(nodeHeaders)) { @@ -59,6 +82,10 @@ function createRequestHeaders(nodeHeaders: IncomingHttpHeaders): Headers { continue; } + if (name.startsWith('x-forwarded-') && !isProxyHeaderAllowed(name, allowedProxyHeaders)) { + continue; + } + if (typeof value === 'string') { headers.append(name, value); } else if (Array.isArray(value)) { @@ -75,20 +102,34 @@ function createRequestHeaders(nodeHeaders: IncomingHttpHeaders): Headers { * Creates a `URL` object from a Node.js `IncomingMessage`, taking into account the protocol, host, and port. * * @param nodeRequest - The Node.js `IncomingMessage` or `Http2ServerRequest` object to extract URL information from. + * @param allowedProxyHeaders - A boolean or a set of allowed proxy headers. + * + * @remarks + * When `allowedProxyHeaders` is enabled, headers such as `X-Forwarded-Host` and + * `X-Forwarded-Prefix` should ideally be strictly validated at a higher infrastructure + * level (e.g., at the reverse proxy or API gateway) before reaching the application. + * * @returns A `URL` object representing the request URL. */ -export function createRequestUrl(nodeRequest: IncomingMessage | Http2ServerRequest): URL { +export function createRequestUrl( + nodeRequest: IncomingMessage | Http2ServerRequest, + allowedProxyHeaders?: boolean | ReadonlySet, +): URL { const { headers, socket, url = '', originalUrl, } = nodeRequest as IncomingMessage & { originalUrl?: string }; + const protocol = - getFirstHeaderValue(headers['x-forwarded-proto']) ?? + getAllowedProxyHeaderValue(headers, 'x-forwarded-proto', allowedProxyHeaders) ?? ('encrypted' in socket && socket.encrypted ? 'https' : 'http'); + const hostname = - getFirstHeaderValue(headers['x-forwarded-host']) ?? headers.host ?? headers[':authority']; + getAllowedProxyHeaderValue(headers, 'x-forwarded-host', allowedProxyHeaders) ?? + headers.host ?? + headers[':authority']; if (Array.isArray(hostname)) { throw new Error('host value cannot be an array.'); @@ -96,7 +137,7 @@ export function createRequestUrl(nodeRequest: IncomingMessage | Http2ServerReque let hostnameWithPort = hostname; if (!hostname?.includes(':')) { - const port = getFirstHeaderValue(headers['x-forwarded-port']); + const port = getAllowedProxyHeaderValue(headers, 'x-forwarded-port', allowedProxyHeaders); if (port) { hostnameWithPort += `:${port}`; } @@ -104,3 +145,43 @@ export function createRequestUrl(nodeRequest: IncomingMessage | Http2ServerReque return new URL(`${protocol}://${hostnameWithPort}${originalUrl ?? url}`); } + +/** + * Gets the first value of an allowed proxy header. + * + * @param headers - The Node.js incoming HTTP headers. + * @param headerName - The name of the proxy header to retrieve. + * @param allowedProxyHeaders - A boolean or a set of allowed proxy headers. + * @returns The value of the allowed proxy header, or `undefined` if not allowed or not present. + */ +function getAllowedProxyHeaderValue( + headers: IncomingHttpHeaders, + headerName: string, + allowedProxyHeaders: boolean | ReadonlySet | undefined, +): string | undefined { + return isProxyHeaderAllowed(headerName, allowedProxyHeaders) + ? getFirstHeaderValue(headers[headerName]) + : undefined; +} + +/** + * Checks if a specific proxy header is allowed. + * + * @param headerName - The name of the proxy header to check. + * @param allowedProxyHeaders - A boolean or a set of allowed proxy headers. + * @returns `true` if the header is allowed, `false` otherwise. + */ +function isProxyHeaderAllowed( + headerName: string, + allowedProxyHeaders: boolean | ReadonlySet | undefined, +): boolean { + if (!allowedProxyHeaders) { + return false; + } + + if (allowedProxyHeaders === true) { + return true; + } + + return allowedProxyHeaders.has(headerName.toLowerCase()); +} diff --git a/packages/angular/ssr/node/test/request_spec.ts b/packages/angular/ssr/node/test/request_spec.ts index 952faefd9f28..5d6168d3ecc1 100644 --- a/packages/angular/ssr/node/test/request_spec.ts +++ b/packages/angular/ssr/node/test/request_spec.ts @@ -137,6 +137,7 @@ describe('createRequestUrl', () => { }, url: '/test', }), + true, ); expect(url.href).toBe('https://example.com/test'); }); @@ -152,6 +153,7 @@ describe('createRequestUrl', () => { }, url: '/test', }), + true, ); expect(url.href).toBe('https://example.com:8443/test'); }); diff --git a/packages/angular/ssr/src/app-engine.ts b/packages/angular/ssr/src/app-engine.ts index 09e1093fef72..74b9c20da64d 100644 --- a/packages/angular/ssr/src/app-engine.ts +++ b/packages/angular/ssr/src/app-engine.ts @@ -12,7 +12,7 @@ import { getPotentialLocaleIdFromUrl, getPreferredLocale } from './i18n'; import { EntryPointExports, getAngularAppEngineManifest } from './manifest'; import { createRedirectResponse } from './utils/redirect'; import { joinUrlParts } from './utils/url'; -import { cloneRequestAndPatchHeaders, validateRequest } from './utils/validation'; +import { sanitizeRequestHeaders, validateRequest } from './utils/validation'; /** * Options for the Angular server application engine. @@ -22,6 +22,22 @@ export interface AngularAppEngineOptions { * A set of allowed hostnames for the server application. */ allowedHosts?: readonly string[]; + + /** + * Extends the scope of trusted proxy headers (`X-Forwarded-*`). + * + * @remarks + * When `allowedProxyHeaders` is enabled, headers such as `X-Forwarded-Host` and + * `X-Forwarded-Prefix` should ideally be strictly validated at a higher infrastructure + * level (e.g., at the reverse proxy or API gateway) before reaching the application. + * + * If a `string[]` is provided, only those proxy headers are allowed. + * If `true`, all proxy headers are allowed. + * If `false` or not provided, proxy headers are ignored. + * + * @default false + */ + allowedProxyHeaders?: boolean | readonly string[]; } /** @@ -78,6 +94,11 @@ export class AngularAppEngine { this.manifest.supportedLocales, ); + /** + * The normalized allowed proxy headers. + */ + private readonly allowedProxyHeaders: ReadonlySet | boolean; + /** * A cache that holds entry points, keyed by their potential locale string. */ @@ -89,6 +110,12 @@ export class AngularAppEngine { */ constructor(options?: AngularAppEngineOptions) { this.allowedHosts = this.getAllowedHosts(options); + + const allowedProxyHeaders = options?.allowedProxyHeaders ?? false; + this.allowedProxyHeaders = + typeof allowedProxyHeaders === 'boolean' + ? allowedProxyHeaders + : new Set(allowedProxyHeaders.map((h) => h.toLowerCase())); } private getAllowedHosts(options: AngularAppEngineOptions | undefined): ReadonlySet { @@ -131,33 +158,17 @@ export class AngularAppEngine { */ async handle(request: Request, requestContext?: unknown): Promise { const allowedHost = this.allowedHosts; - const disableAllowedHostsCheck = AngularAppEngine.ɵdisableAllowedHostsCheck; + const securedRequest = sanitizeRequestHeaders(request, this.allowedProxyHeaders); try { - validateRequest(request, allowedHost, disableAllowedHostsCheck); + validateRequest(securedRequest, allowedHost, AngularAppEngine.ɵdisableAllowedHostsCheck); } catch (error) { - return this.handleValidationError(request.url, error as Error); + return this.handleValidationError(securedRequest.url, error as Error); } - // Clone request with patched headers to prevent unallowed host header access. - const { request: securedRequest, onError: onHeaderValidationError } = disableAllowedHostsCheck - ? { request, onError: null } - : cloneRequestAndPatchHeaders(request, allowedHost); - const serverApp = await this.getAngularServerAppForRequest(securedRequest); if (serverApp) { - const promises: Promise[] = []; - if (onHeaderValidationError) { - promises.push( - onHeaderValidationError.then((error) => - this.handleValidationError(securedRequest.url, error), - ), - ); - } - - promises.push(serverApp.handle(securedRequest, requestContext)); - - return Promise.race(promises); + return serverApp.handle(securedRequest, requestContext); } if (this.supportedLocales.length > 1) { diff --git a/packages/angular/ssr/src/utils/validation.ts b/packages/angular/ssr/src/utils/validation.ts index e8af64ed9943..75b60ccf8b53 100644 --- a/packages/angular/ssr/src/utils/validation.ts +++ b/packages/angular/ssr/src/utils/validation.ts @@ -9,7 +9,7 @@ /** * The set of headers that should be validated for host header injection attacks. */ -const HOST_HEADERS_TO_VALIDATE: ReadonlySet = new Set(['host', 'x-forwarded-host']); +const HOST_HEADERS_TO_VALIDATE: ReadonlyArray = ['host', 'x-forwarded-host']; /** * Regular expression to validate that the port is a numeric value. @@ -21,15 +21,10 @@ const VALID_PORT_REGEX = /^\d+$/; */ const VALID_PROTO_REGEX = /^https?$/i; -/** - * Regular expression to validate that the host is a valid hostname. - */ -const VALID_HOST_REGEX = /^[a-z0-9_.-]+(:[0-9]+)?$/i; - /** * Regular expression to validate that the prefix is valid. */ -const INVALID_PREFIX_REGEX = /^(?:\\|\/[/\\])|(?:^|[/\\])\.\.?(?:[/\\]|$)/; +const VALID_PREFIX_REGEX = /^\/([a-z0-9_-]+\/)*[a-z0-9_-]*$/i; /** * Extracts the first value from a multi-value header string. @@ -64,7 +59,7 @@ export function validateRequest( allowedHosts: ReadonlySet, disableHostCheck: boolean, ): void { - validateHeaders(request); + validateHeaders(request, allowedHosts, disableHostCheck); if (!disableHostCheck) { validateUrl(new URL(request.url), allowedHosts); @@ -86,119 +81,46 @@ export function validateUrl(url: URL, allowedHosts: ReadonlySet): void { } /** - * Clones a request and patches the `get` method of the request headers to validate the host headers. - * @param request - The request to validate. - * @param allowedHosts - A set of allowed hostnames. - * @returns An object containing the cloned request and a promise that resolves to an error - * if any of the validated headers contain invalid values. + * Sanitizes the proxy headers of a request by removing unallowed `X-Forwarded-*` headers. + * If no headers need to be removed, the original request is returned without cloning. + * + * @param request - The incoming `Request` object to sanitize. + * @param allowedProxyHeaders - A set of allowed proxy headers or a boolean indicating whether all are allowed. + * @returns The sanitized request, or the original request if no changes were needed. */ -export function cloneRequestAndPatchHeaders( +export function sanitizeRequestHeaders( request: Request, - allowedHosts: ReadonlySet, -): { request: Request; onError: Promise } { - let onError: (value: Error) => void; - const onErrorPromise = new Promise((resolve) => { - onError = resolve; - }); - - const clonedReq = new Request(request.clone(), { - signal: request.signal, - }); - - const headers = clonedReq.headers; - - const originalGet = headers.get; - // eslint-disable-next-line @typescript-eslint/no-unnecessary-type-assertion - (headers.get as typeof originalGet) = function (name) { - const value = originalGet.call(headers, name); - if (!value) { - return value; - } - - validateHeader(name, value, allowedHosts, onError); - - return value; - }; + allowedProxyHeaders?: boolean | ReadonlySet, +): Request { + if (allowedProxyHeaders === true) { + return request; + } - const originalValues = headers.values; - // eslint-disable-next-line @typescript-eslint/no-unnecessary-type-assertion - (headers.values as typeof originalValues) = function () { - for (const name of HOST_HEADERS_TO_VALIDATE) { - validateHeader(name, originalGet.call(headers, name), allowedHosts, onError); + const keysToDelete: string[] = []; + for (const [key] of request.headers) { + const lowerKey = key.toLowerCase(); + if ( + lowerKey.startsWith('x-forwarded-') && + (!allowedProxyHeaders || !allowedProxyHeaders.has(lowerKey)) + ) { + keysToDelete.push(key); } - - return originalValues.call(headers); - }; - - const originalEntries = headers.entries; - // eslint-disable-next-line @typescript-eslint/no-unnecessary-type-assertion - (headers.entries as typeof originalEntries) = function () { - const iterator = originalEntries.call(headers); - - return { - next() { - const result = iterator.next(); - if (!result.done) { - const [key, value] = result.value; - validateHeader(key, value, allowedHosts, onError); - } - - return result; - }, - [Symbol.iterator]() { - return this; - }, - }; - }; - - const originalForEach = headers.forEach; - // eslint-disable-next-line @typescript-eslint/no-unnecessary-type-assertion - (headers.forEach as typeof originalForEach) = function (callback, thisArg) { - originalForEach.call( - headers, - (value, key, parent) => { - validateHeader(key, value, allowedHosts, onError); - callback.call(thisArg, value, key, parent); - }, - thisArg, - ); - }; - - // Ensure for...of loops use the new patched entries - (headers[Symbol.iterator] as typeof originalEntries) = headers.entries; - - return { request: clonedReq, onError: onErrorPromise }; -} - -/** - * Validates a specific header value against the allowed hosts. - * @param name - The name of the header to validate. - * @param value - The value of the header to validate. - * @param allowedHosts - A set of allowed hostnames. - * @param onError - A callback function to call if the header value is invalid. - * @throws Error if the header value is invalid. - */ -function validateHeader( - name: string, - value: string | null, - allowedHosts: ReadonlySet, - onError: (value: Error) => void, -): void { - if (!value) { - return; } - if (!HOST_HEADERS_TO_VALIDATE.has(name.toLowerCase())) { - return; + if (keysToDelete.length === 0) { + return request; } - try { - verifyHostAllowed(name, value, allowedHosts); - } catch (error) { - onError(error as Error); + const clonedReq = new Request(request.clone(), { + signal: request.signal, + }); - throw error; + const headers = clonedReq.headers; + for (const key of keysToDelete) { + headers.delete(key); } + + return clonedReq; } /** @@ -214,19 +136,20 @@ function verifyHostAllowed( headerValue: string, allowedHosts: ReadonlySet, ): void { - const value = getFirstHeaderValue(headerValue); - if (!value) { - return; - } - - const url = `http://${value}`; + const url = `http://${headerValue}`; if (!URL.canParse(url)) { throw new Error(`Header "${headerName}" contains an invalid value and cannot be parsed.`); } - const { hostname } = new URL(url); + const { hostname, pathname, search, hash, username, password } = new URL(url); + if (pathname !== '/' || search || hash || username || password) { + throw new Error( + `Header "${headerName}" with value "${headerValue}" contains characters that are not allowed.`, + ); + } + if (!isHostAllowed(hostname, allowedHosts)) { - throw new Error(`Header "${headerName}" with value "${value}" is not allowed.`); + throw new Error(`Header "${headerName}" with value "${headerValue}" is not allowed.`); } } @@ -259,14 +182,20 @@ function isHostAllowed(hostname: string, allowedHosts: ReadonlySet): boo * Validates the headers of an incoming request. * * @param request - The incoming `Request` object containing the headers to validate. + * @param allowedHosts - A set of allowed hostnames. + * @param disableHostCheck - Whether to disable the host check. * @throws Error if any of the validated headers contain invalid values. */ -function validateHeaders(request: Request): void { +function validateHeaders( + request: Request, + allowedHosts: ReadonlySet, + disableHostCheck: boolean, +): void { const headers = request.headers; for (const headerName of HOST_HEADERS_TO_VALIDATE) { const headerValue = getFirstHeaderValue(headers.get(headerName)); - if (headerValue && !VALID_HOST_REGEX.test(headerValue)) { - throw new Error(`Header "${headerName}" contains characters that are not allowed.`); + if (headerValue && !disableHostCheck) { + verifyHostAllowed(headerName, headerValue, allowedHosts); } } @@ -281,9 +210,10 @@ function validateHeaders(request: Request): void { } const xForwardedPrefix = getFirstHeaderValue(headers.get('x-forwarded-prefix')); - if (xForwardedPrefix && INVALID_PREFIX_REGEX.test(xForwardedPrefix)) { + if (xForwardedPrefix && !VALID_PREFIX_REGEX.test(xForwardedPrefix)) { throw new Error( - 'Header "x-forwarded-prefix" must not start with "\\" or multiple "/" or contain ".", ".." path segments.', + 'Header "x-forwarded-prefix" is invalid. It must start with a "/" and contain ' + + 'only alphanumeric characters, hyphens, and underscores, separated by single slashes.', ); } } diff --git a/packages/angular/ssr/test/app-engine_spec.ts b/packages/angular/ssr/test/app-engine_spec.ts index 14e664659748..4deff74d8de3 100644 --- a/packages/angular/ssr/test/app-engine_spec.ts +++ b/packages/angular/ssr/test/app-engine_spec.ts @@ -105,7 +105,7 @@ describe('AngularAppEngine', () => { basePath: '/', }); - appEngine = new AngularAppEngine(); + appEngine = new AngularAppEngine({ allowedProxyHeaders: true }); }); describe('handle', () => { @@ -177,6 +177,21 @@ describe('AngularAppEngine', () => { expect(response?.headers.get('Vary')).toBe('X-Forwarded-Prefix, Accept-Language'); }); + it('should completely ignore proxy headers if not allowed', async () => { + const strictEngine = new AngularAppEngine({ allowedProxyHeaders: false }); + const request = new Request('https://example.com', { + headers: { + 'Accept-Language': 'it', + 'X-Forwarded-Prefix': '/app', + }, + }); + + // The strictEngine will ignore the prefix + const response = await strictEngine.handle(request); + expect(response?.status).toBe(302); + expect(response?.headers.get('Location')).toBe('/it'); + }); + it('should return null for requests to file-like resources in a locale', async () => { const request = new Request('https://example.com/it/logo.png'); const response = await appEngine.handle(request); @@ -324,7 +339,7 @@ describe('AngularAppEngine', () => { supportedLocales: { 'en-US': '' }, }); - appEngine = new AngularAppEngine(); + appEngine = new AngularAppEngine({ allowedProxyHeaders: true }); }); beforeEach(() => { @@ -380,10 +395,12 @@ describe('AngularAppEngine', () => { expect(response).not.toBeNull(); expect(response?.status).toBe(400); expect(await response?.text()).toContain( - 'Header "host" contains characters that are not allowed.', + 'Header "host" with value "example.com/evil" contains characters that are not allowed.', ); expect(consoleErrorSpy).toHaveBeenCalledWith( - jasmine.stringMatching('Header "host" contains characters that are not allowed.'), + jasmine.stringMatching( + 'Header "host" with value "example.com/evil" contains characters that are not allowed.', + ), ); }); }); diff --git a/packages/angular/ssr/test/utils/validation_spec.ts b/packages/angular/ssr/test/utils/validation_spec.ts index 5c4b6e8cd121..b0004b06ad8a 100644 --- a/packages/angular/ssr/test/utils/validation_spec.ts +++ b/packages/angular/ssr/test/utils/validation_spec.ts @@ -7,8 +7,8 @@ */ import { - cloneRequestAndPatchHeaders, getFirstHeaderValue, + sanitizeRequestHeaders, validateRequest, validateUrl, } from '../../src/utils/validation'; @@ -132,7 +132,7 @@ describe('Validation Utils', () => { headers: { 'host': 'example.com/bad' }, }); expect(() => validateRequest(req, allowedHosts, false)).toThrowError( - 'Header "host" contains characters that are not allowed.', + 'Header "host" with value "example.com/bad" contains characters that are not allowed.', ); }); @@ -141,7 +141,7 @@ describe('Validation Utils', () => { headers: { 'host': 'example.com?query=1' }, }); expect(() => validateRequest(req, allowedHosts, false)).toThrowError( - 'Header "host" contains characters that are not allowed.', + 'Header "host" with value "example.com?query=1" contains characters that are not allowed.', ); }); @@ -150,30 +150,17 @@ describe('Validation Utils', () => { headers: { 'x-forwarded-host': 'example.com/bad' }, }); expect(() => validateRequest(req, allowedHosts, false)).toThrowError( - 'Header "x-forwarded-host" contains characters that are not allowed.', + 'Header "x-forwarded-host" with value "example.com/bad" contains characters that are not allowed.', ); }); - it('should throw error if x-forwarded-prefix starts with a backslash or multiple slashes', () => { - const inputs = ['//evil', '\\\\evil', '/\\evil', '\\/evil', '\\evil']; - - for (const prefix of inputs) { - const request = new Request('https://example.com', { - headers: { - 'x-forwarded-prefix': prefix, - }, - }); - - expect(() => validateRequest(request, allowedHosts, false)) - .withContext(`Prefix: "${prefix}"`) - .toThrowError( - 'Header "x-forwarded-prefix" must not start with "\\" or multiple "/" or contain ".", ".." path segments.', - ); - } - }); - - it('should throw error if x-forwarded-prefix contains dot segments', () => { + it('should throw error if x-forwarded-prefix is invalid', () => { const inputs = [ + '//evil', + '\\\\evil', + '/\\evil', + '\\/evil', + '\\evil', '/./', '/../', '/foo/./bar', @@ -188,6 +175,11 @@ describe('Validation Utils', () => { '/foo/..\\bar', '.', '..', + 'https://attacker.com', + '%2f%2f', + '%5C', + 'http://localhost', + 'foo', ]; for (const prefix of inputs) { @@ -200,13 +192,14 @@ describe('Validation Utils', () => { expect(() => validateRequest(request, allowedHosts, false)) .withContext(`Prefix: "${prefix}"`) .toThrowError( - 'Header "x-forwarded-prefix" must not start with "\\" or multiple "/" or contain ".", ".." path segments.', + 'Header "x-forwarded-prefix" is invalid. It must start with a "/" and contain ' + + 'only alphanumeric characters, hyphens, and underscores, separated by single slashes.', ); } }); - it('should validate x-forwarded-prefix with valid dot usage', () => { - const inputs = ['/foo.bar', '/foo.bar/baz', '/v1.2', '/.well-known']; + it('should validate x-forwarded-prefix with valid usage', () => { + const inputs = ['/foo', '/foo/baz', '/v1', '/app_name', '/', '/my-app']; for (const prefix of inputs) { const request = new Request('https://example.com', { @@ -222,143 +215,61 @@ describe('Validation Utils', () => { }); }); - describe('cloneRequestAndPatchHeaders', () => { - const allowedHosts = new Set(['example.com', '*.valid.com']); - - it('should validate host header when accessed via get()', async () => { + describe('sanitizeRequestHeaders', () => { + it('should scrub unallowed proxy headers by default', () => { const req = new Request('http://example.com', { - headers: { 'host': 'evil.com' }, + headers: { + 'host': 'example.com', + 'x-forwarded-host': 'evil.com', + 'x-forwarded-proto': 'https', + }, }); - const { request: secured, onError } = cloneRequestAndPatchHeaders(req, allowedHosts); - - expect(() => secured.headers.get('host')).toThrowError( - 'Header "host" with value "evil.com" is not allowed.', - ); - await expectAsync(onError).toBeResolvedTo( - jasmine.objectContaining({ - message: jasmine.stringMatching('Header "host" with value "evil.com" is not allowed'), - }), - ); - }); + const secured = sanitizeRequestHeaders(req); - it('should allow valid host header', () => { - const req = new Request('http://example.com', { - headers: { 'host': 'example.com' }, - }); - const { request: secured } = cloneRequestAndPatchHeaders(req, allowedHosts); expect(secured.headers.get('host')).toBe('example.com'); + expect(secured.headers.has('x-forwarded-host')).toBeFalse(); + expect(secured.headers.has('x-forwarded-proto')).toBeFalse(); }); - it('should allow any host header when "*" is used', () => { - const allowedHosts = new Set(['*']); + it('should retain allowed proxy headers when explicitly provided', () => { + const allowedProxyHeaders = new Set(['x-forwarded-host']); const req = new Request('http://example.com', { - headers: { 'host': 'evil.com' }, + headers: { + 'host': 'example.com', + 'x-forwarded-host': 'proxy.com', + 'x-forwarded-proto': 'https', + }, }); - const { request: secured } = cloneRequestAndPatchHeaders(req, allowedHosts); - expect(secured.headers.get('host')).toBe('evil.com'); + const secured = sanitizeRequestHeaders(req, allowedProxyHeaders); + + expect(secured.headers.get('host')).toBe('example.com'); + expect(secured.headers.get('x-forwarded-host')).toBe('proxy.com'); + expect(secured.headers.has('x-forwarded-proto')).toBeFalse(); }); - it('should validate x-forwarded-host header', async () => { + it('should retain all proxy headers when allowedProxyHeaders is true', () => { const req = new Request('http://example.com', { - headers: { 'x-forwarded-host': 'evil.com' }, + headers: { + 'host': 'example.com', + 'x-forwarded-host': 'proxy.com', + 'x-forwarded-proto': 'https', + }, }); - const { request: secured, onError } = cloneRequestAndPatchHeaders(req, allowedHosts); + const secured = sanitizeRequestHeaders(req, true); - expect(() => secured.headers.get('x-forwarded-host')).toThrowError( - 'Header "x-forwarded-host" with value "evil.com" is not allowed.', - ); - await expectAsync(onError).toBeResolvedTo( - jasmine.objectContaining({ - message: jasmine.stringMatching( - 'Header "x-forwarded-host" with value "evil.com" is not allowed', - ), - }), - ); + expect(secured.headers.get('host')).toBe('example.com'); + expect(secured.headers.get('x-forwarded-host')).toBe('proxy.com'); + expect(secured.headers.get('x-forwarded-proto')).toBe('https'); }); - it('should allow accessing other headers without validation', () => { + it('should not clone the request if no proxy headers need to be removed', () => { const req = new Request('http://example.com', { headers: { 'accept': 'application/json' }, }); - const { request: secured } = cloneRequestAndPatchHeaders(req, allowedHosts); + const secured = sanitizeRequestHeaders(req); + expect(secured).toBe(req); expect(secured.headers.get('accept')).toBe('application/json'); }); - - it('should validate headers when iterating with entries()', async () => { - const req = new Request('http://example.com', { - headers: { 'host': 'evil.com' }, - }); - const { request: secured, onError } = cloneRequestAndPatchHeaders(req, allowedHosts); - - expect(() => { - for (const _ of secured.headers.entries()) { - // access the header to trigger the validation - } - }).toThrowError('Header "host" with value "evil.com" is not allowed.'); - - await expectAsync(onError).toBeResolvedTo( - jasmine.objectContaining({ - message: jasmine.stringMatching('Header "host" with value "evil.com" is not allowed.'), - }), - ); - }); - - it('should validate headers when iterating with values()', async () => { - const req = new Request('http://example.com', { - headers: { 'host': 'evil.com' }, - }); - const { request: secured, onError } = cloneRequestAndPatchHeaders(req, allowedHosts); - - expect(() => { - for (const _ of secured.headers.values()) { - // access the header to trigger the validation - } - }).toThrowError('Header "host" with value "evil.com" is not allowed.'); - - await expectAsync(onError).toBeResolvedTo( - jasmine.objectContaining({ - message: jasmine.stringMatching('Header "host" with value "evil.com" is not allowed.'), - }), - ); - }); - - it('should validate headers when iterating with for...of', async () => { - const req = new Request('http://example.com', { - headers: { 'host': 'evil.com' }, - }); - const { request: secured, onError } = cloneRequestAndPatchHeaders(req, allowedHosts); - - expect(() => { - for (const _ of secured.headers) { - // access the header to trigger the validation - } - }).toThrowError('Header "host" with value "evil.com" is not allowed.'); - - await expectAsync(onError).toBeResolvedTo( - jasmine.objectContaining({ - message: jasmine.stringMatching('Header "host" with value "evil.com" is not allowed.'), - }), - ); - }); - - it('should validate headers when iterating with forEach()', async () => { - const req = new Request('http://example.com', { - headers: { 'host': 'evil.com' }, - }); - const { request: secured, onError } = cloneRequestAndPatchHeaders(req, allowedHosts); - - expect(() => { - secured.headers.forEach(() => { - // access the header to trigger the validation - }); - }).toThrowError('Header "host" with value "evil.com" is not allowed.'); - - await expectAsync(onError).toBeResolvedTo( - jasmine.objectContaining({ - message: jasmine.stringMatching('Header "host" with value "evil.com" is not allowed.'), - }), - ); - }); }); });