Skip to content

fix(@angular/ssr): enforce explicit opt-in for proxy headers#32911

Open
alan-agius4 wants to merge 1 commit intoangular:mainfrom
alan-agius4:x-forward-prefix-validation
Open

fix(@angular/ssr): enforce explicit opt-in for proxy headers#32911
alan-agius4 wants to merge 1 commit intoangular:mainfrom
alan-agius4:x-forward-prefix-validation

Conversation

@alan-agius4
Copy link
Copy Markdown
Collaborator

@alan-agius4 alan-agius4 commented Apr 1, 2026

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 trustProxyHeaders.
  • 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 trustProxyHeaders option in the application server configuration.

Example:

const engine = new AngularAppEngine({
  // Allow all proxy headers
  trustProxyHeaders: true,
});

// Or explicitly allow specific headers:
const engine = new AngularAppEngine({
  trustProxyHeaders: ['x-forwarded-host', 'x-forwarded-prefix'],
});

@angular-robot angular-robot Bot added detected: breaking change PR contains a commit with a breaking change area: @angular/ssr labels Apr 1, 2026
@alan-agius4 alan-agius4 force-pushed the x-forward-prefix-validation branch 4 times, most recently from 812e774 to 8a649a5 Compare April 1, 2026 12:33
@alan-agius4 alan-agius4 changed the title refactor(@angular/ssr): enforce explicit opt-in for proxy headers fix(@angular/ssr): enforce explicit opt-in for proxy headers Apr 1, 2026
@alan-agius4 alan-agius4 force-pushed the x-forward-prefix-validation branch 3 times, most recently from 8de34e7 to 2cf1919 Compare April 1, 2026 12:55
@alan-agius4 alan-agius4 force-pushed the x-forward-prefix-validation branch from 2cf1919 to fca2679 Compare April 22, 2026 08:03
@alan-agius4 alan-agius4 marked this pull request as ready for review April 22, 2026 08:03
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request implements support for trusting and sanitizing proxy headers (X-Forwarded-*) in the Angular SSR engine. It introduces a new configuration option to specify allowed proxy headers, replaces the request cloning/patching logic with a more performant sanitization utility, and enhances the validation of host and prefix headers. Review feedback identifies a critical naming inconsistency between 'trustProxyHeaders' and 'allowedProxyHeaders' across the public API and various modules, a property access error in the Node engine implementation, and a minor optimization for header name normalization.

Comment thread packages/angular/ssr/src/app-engine.ts
Comment thread packages/angular/ssr/node/src/app-engine.ts Outdated
Comment thread packages/angular/ssr/node/src/request.ts
@alan-agius4 alan-agius4 force-pushed the x-forward-prefix-validation branch 2 times, most recently from 50ad11b to a45cea9 Compare April 22, 2026 09:25
@alan-agius4 alan-agius4 added the target: major This PR is targeted for the next major release label Apr 22, 2026
@alan-agius4 alan-agius4 requested a review from dgp1130 April 22, 2026 09:29
@alan-agius4 alan-agius4 added the action: review The PR is still awaiting reviews from at least one requested reviewer label Apr 22, 2026
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 `trustProxyHeaders`.
- 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 `trustProxyHeaders` option in the application server configuration.

Example:
```ts
const engine = new AngularAppEngine({
  // Allow all proxy headers
  trustProxyHeaders: true,
});

// Or explicitly allow specific headers:
const engine = new AngularAppEngine({
  trustProxyHeaders: ['x-forwarded-host', 'x-forwarded-prefix'],
});
```
@alan-agius4 alan-agius4 force-pushed the x-forward-prefix-validation branch from a45cea9 to a0aa5ba Compare April 22, 2026 11:37
Copy link
Copy Markdown
Collaborator

@dgp1130 dgp1130 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall LGTM, main point of feedback is just making sure the documentation communicates the significance of the option and how and when to use it.

Comment on lines +51 to +54
const trustProxyHeadersNormalized =
trustProxyHeaders && typeof trustProxyHeaders !== 'boolean'
? new Set(trustProxyHeaders.map((h) => h.toLowerCase()))
: trustProxyHeaders;
Copy link
Copy Markdown
Collaborator

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 false to [] and true to ['x-forwarded-host', ...] at this layer?


if (
name.toLowerCase().startsWith('x-forwarded-') &&
!isProxyHeaderAllowed(name, trustProxyHeaders)
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question: Are there X-Forwarded-* headers other than the ones we support today which we might be blocking here? Anything else the developer might be using independent of Angular?

Comment on lines 24 to +27
/**
* Regular expression to validate that the prefix is valid.
*/
const INVALID_PREFIX_REGEX = /^(?:\\|\/[/\\])|(?:^|[/\\])\.\.?(?:[/\\]|$)/;
const VALID_PREFIX_REGEX = /^\/([a-z0-9_-]+\/)*[a-z0-9_-]*$/i;
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider: Might be helpful to explain in the comment what this is checking for. I assume it's really //foo which we don't want, but we should try to be clear about the intent here.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider: I'm not sure I fully appreciated this until now, but it feels like a lot of the core logic here is duplicated across the Node and web standard versions of this API. Is there a path to potentially consolidating some of the changes into a single location? At least the security-relevant checks?

/**
* Extends the scope of trusted proxy headers (`X-Forwarded-*`).
*
* @remarks
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The 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 @remarks? Does our tooling do something with that? I've never used that myself.

*
* @default false
*/
trustProxyHeaders?: boolean | readonly string[];
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: I think we can make this language a little stronger to highlight the need for input sanitization and raise awareness of the security implications.

Suggested change
trustProxyHeaders?: boolean | readonly string[];
/**
* Extends the scope of trusted proxy headers (`X-Forwarded-*`).
*
* @remarks
* **This is a security-sensitive option!**
*
* When `trustProxyHeaders` is enabled, request headers such as `X-Forwarded-Host` and
* `X-Forwarded-Prefix` are trusted by the server and used for routing. These
* headers must be strictly validated and provided by a trusted client (e.g., at a reverse proxy, load
* balancer, or API gateway) and must *not* be provided by untrusted end users.
*
* 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[];

If / when we add documentation to angular.dev, it might be good to link to it to expand on what this means in practice.

name.toLowerCase().startsWith('x-forwarded-') &&
!isProxyHeaderAllowed(name, trustProxyHeaders)
) {
continue;
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider: Should we log that we ignored a header in case the developer needs to set trustProxyHeaders?

this.trustProxyHeaders =
typeof trustProxyHeaders === 'boolean'
? trustProxyHeaders
: new Set(trustProxyHeaders.map((h) => h.toLowerCase()));
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Feels a little asymmetrical that AngularAppEngine normalizes headers but AngularNodeAppEngine doesn't and instead relies on request.ts to do it. Is it worth doing this at the same level of abstraction for each?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

action: review The PR is still awaiting reviews from at least one requested reviewer area: @angular/ssr detected: breaking change PR contains a commit with a breaking change target: major This PR is targeted for the next major release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants