fix(@angular/ssr): enforce explicit opt-in for proxy headers#32911
fix(@angular/ssr): enforce explicit opt-in for proxy headers#32911alan-agius4 wants to merge 2 commits intoangular:mainfrom
Conversation
812e774 to
8a649a5
Compare
8de34e7 to
2cf1919
Compare
2cf1919 to
fca2679
Compare
There was a problem hiding this comment.
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.
50ad11b to
a45cea9
Compare
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'],
});
```
a45cea9 to
a0aa5ba
Compare
dgp1130
left a comment
There was a problem hiding this comment.
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.
| const trustProxyHeadersNormalized = | ||
| trustProxyHeaders && typeof trustProxyHeaders !== 'boolean' | ||
| ? new Set(trustProxyHeaders.map((h) => h.toLowerCase())) | ||
| : trustProxyHeaders; |
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
X-Forwarded-Port and X-Forwarded-For would be the most "common".
| /** | ||
| * Extends the scope of trusted proxy headers (`X-Forwarded-*`). | ||
| * | ||
| * @remarks |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Yeah, @remarks are added as Usage notes in the docs.
| this.trustProxyHeaders = | ||
| typeof trustProxyHeaders === 'boolean' | ||
| ? trustProxyHeaders | ||
| : new Set(trustProxyHeaders.map((h) => h.toLowerCase())); |
There was a problem hiding this comment.
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?
fdda2b1 to
4e47830
Compare
4e47830 to
bad22bf
Compare
This commit introduces a secure-by-default model for trusting proxy
headers (
X-Forwarded-*) in the@angular/ssrpackage. Previously, theengine 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:
allowedProxyHeadersconfiguration option toAngularAppEngineOptionsandAngularNodeAppEngineOptions.false), all incomingX-Forwarded-*headers are aggressivelyscrubbed unless explicitly whitelisted via
trustProxyHeaders.cloneRequestAndPatchHeadersutility with a simplified,eager
sanitizeRequestHeadersthat centralizes the header scrubbing logic.verifyHostAllowedto definitively reject parsed hosts that successfullycarry path, search, hash, or auth components, replacing previously fallible
regex filters for stringently checked hosts.
BREAKING CHANGE:
The
@angular/ssrpackage now ignores allX-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 newtrustProxyHeadersoption in the application server configuration.Example: