-
Notifications
You must be signed in to change notification settings - Fork 11.9k
fix(@angular/ssr): enforce explicit opt-in for proxy headers #32911
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
base: main
Are you sure you want to change the base?
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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<string> = new Set([ | ||
| ':method', | ||
| ':scheme', | ||
| ':authority', | ||
| ':path', | ||
| ':status', | ||
| ]); | ||
|
|
||
| /** | ||
| * Converts a Node.js `IncomingMessage` or `Http2ServerRequest` into a | ||
|
|
@@ -27,18 +33,33 @@ 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 trustProxyHeaders - A boolean or an array of allowed proxy headers. | ||
| * | ||
| * @remarks | ||
| * When `trustProxyHeaders` 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. | ||
| * | ||
| * @private | ||
| */ | ||
| export function createWebRequestFromNodeRequest( | ||
| nodeRequest: IncomingMessage | Http2ServerRequest, | ||
| trustProxyHeaders?: boolean | readonly string[], | ||
| ): Request { | ||
| const trustProxyHeadersNormalized = | ||
| trustProxyHeaders && typeof trustProxyHeaders !== 'boolean' | ||
| ? new Set(trustProxyHeaders.map((h) => h.toLowerCase())) | ||
| : trustProxyHeaders; | ||
|
|
||
| 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, trustProxyHeadersNormalized), { | ||
| method, | ||
| headers: createRequestHeaders(headers), | ||
| headers: createRequestHeaders(headers, trustProxyHeadersNormalized), | ||
| body: withBody ? nodeRequest : undefined, | ||
| duplex: withBody ? 'half' : undefined, | ||
| referrer, | ||
|
|
@@ -49,16 +70,27 @@ export function createWebRequestFromNodeRequest( | |
| * Creates a `Headers` object from Node.js `IncomingHttpHeaders`. | ||
| * | ||
| * @param nodeHeaders - The Node.js `IncomingHttpHeaders` object to convert. | ||
| * @param trustProxyHeaders - 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, | ||
| trustProxyHeaders: boolean | ReadonlySet<string> | undefined, | ||
| ): Headers { | ||
| const headers = new Headers(); | ||
|
|
||
| for (const [name, value] of Object.entries(nodeHeaders)) { | ||
| if (HTTP2_PSEUDO_HEADERS.has(name)) { | ||
| continue; | ||
| } | ||
|
|
||
| if ( | ||
| name.toLowerCase().startsWith('x-forwarded-') && | ||
| !isProxyHeaderAllowed(name, trustProxyHeaders) | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Question: Are there
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
| ) { | ||
| continue; | ||
|
alan-agius4 marked this conversation as resolved.
Outdated
|
||
| } | ||
|
|
||
| if (typeof value === 'string') { | ||
| headers.append(name, value); | ||
| } else if (Array.isArray(value)) { | ||
|
|
@@ -75,32 +107,86 @@ 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 trustProxyHeaders - A boolean or a set of allowed proxy headers. | ||
| * | ||
| * @remarks | ||
| * When `trustProxyHeaders` 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, | ||
| trustProxyHeaders?: boolean | ReadonlySet<string>, | ||
| ): URL { | ||
| const { | ||
| headers, | ||
| socket, | ||
| url = '', | ||
| originalUrl, | ||
| } = nodeRequest as IncomingMessage & { originalUrl?: string }; | ||
|
|
||
| const protocol = | ||
| getFirstHeaderValue(headers['x-forwarded-proto']) ?? | ||
| getAllowedProxyHeaderValue(headers, 'x-forwarded-proto', trustProxyHeaders) ?? | ||
| ('encrypted' in socket && socket.encrypted ? 'https' : 'http'); | ||
|
|
||
| const hostname = | ||
| getFirstHeaderValue(headers['x-forwarded-host']) ?? headers.host ?? headers[':authority']; | ||
| getAllowedProxyHeaderValue(headers, 'x-forwarded-host', trustProxyHeaders) ?? | ||
| headers.host ?? | ||
| headers[':authority']; | ||
|
|
||
| if (Array.isArray(hostname)) { | ||
| throw new Error('host value cannot be an array.'); | ||
| } | ||
|
|
||
| let hostnameWithPort = hostname; | ||
| if (!hostname?.includes(':')) { | ||
| const port = getFirstHeaderValue(headers['x-forwarded-port']); | ||
| const port = getAllowedProxyHeaderValue(headers, 'x-forwarded-port', trustProxyHeaders); | ||
| if (port) { | ||
| hostnameWithPort += `:${port}`; | ||
| } | ||
| } | ||
|
|
||
| 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 trustProxyHeaders - 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, | ||
| trustProxyHeaders: boolean | ReadonlySet<string> | undefined, | ||
| ): string | undefined { | ||
| return isProxyHeaderAllowed(headerName, trustProxyHeaders) | ||
| ? getFirstHeaderValue(headers[headerName]) | ||
| : undefined; | ||
| } | ||
|
|
||
| /** | ||
| * Checks if a specific proxy header is allowed. | ||
| * | ||
| * @param headerName - The name of the proxy header to check. | ||
| * @param trustProxyHeaders - A boolean or a set of allowed proxy headers. | ||
| * @returns `true` if the header is allowed, `false` otherwise. | ||
| */ | ||
| function isProxyHeaderAllowed( | ||
| headerName: string, | ||
| trustProxyHeaders: boolean | ReadonlySet<string> | undefined, | ||
| ): boolean { | ||
| if (!trustProxyHeaders) { | ||
| return false; | ||
| } | ||
|
|
||
| if (trustProxyHeaders === true) { | ||
| return true; | ||
| } | ||
|
|
||
| return trustProxyHeaders.has(headerName.toLowerCase()); | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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 | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Question: Just for my own curiosity, is there a particular benefit to using
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, |
||
| * When `trustProxyHeaders` 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 | ||
| */ | ||
| trustProxyHeaders?: boolean | readonly string[]; | ||
|
alan-agius4 marked this conversation as resolved.
alan-agius4 marked this conversation as resolved.
|
||
| } | ||
|
|
||
| /** | ||
|
|
@@ -78,6 +94,11 @@ export class AngularAppEngine { | |
| this.manifest.supportedLocales, | ||
| ); | ||
|
|
||
| /** | ||
| * The normalized allowed proxy headers. | ||
| */ | ||
| private readonly trustProxyHeaders: ReadonlySet<string> | 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 trustProxyHeaders = options?.trustProxyHeaders ?? false; | ||
| this.trustProxyHeaders = | ||
| typeof trustProxyHeaders === 'boolean' | ||
| ? trustProxyHeaders | ||
| : new Set(trustProxyHeaders.map((h) => h.toLowerCase())); | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: Feels a little asymmetrical that
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done |
||
| } | ||
|
|
||
| private getAllowedHosts(options: AngularAppEngineOptions | undefined): ReadonlySet<string> { | ||
|
|
@@ -131,33 +158,17 @@ export class AngularAppEngine { | |
| */ | ||
| async handle(request: Request, requestContext?: unknown): Promise<Response | null> { | ||
| const allowedHost = this.allowedHosts; | ||
| const disableAllowedHostsCheck = AngularAppEngine.ɵdisableAllowedHostsCheck; | ||
| const securedRequest = sanitizeRequestHeaders(request, this.trustProxyHeaders); | ||
|
|
||
| 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<Response | null>[] = []; | ||
| 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) { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider: Would it be simpler to normalize
falseto[]andtrueto['x-forwarded-host', ...]at this layer?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done