fix(@angular/ssr): add support for configuring trusted proxy headers via environment variable#33272
fix(@angular/ssr): add support for configuring trusted proxy headers via environment variable#33272dgp1130 wants to merge 1 commit into
Conversation
…via environment variable Adds support for configuring trusted proxy headers via the `NG_TRUST_PROXY_HEADERS` environment variable in `AngularNodeAppEngine`. This allows users to specify which proxy headers (such as `X-Forwarded-Host`) should be trusted when running the server-side application behind a reverse proxy, without needing to modify the application code. The environment variable accepts a comma-separated list of header names. If the `NG_TRUST_PROXY_HEADERS` environment variable is set and contains non-empty values, it will take precedence over the `trustProxyHeaders` option provided programmatically in the `AngularNodeAppEngine` constructor options. (cherry picked from commit 126b19b)
dgp1130
added
action: review
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors environment variable parsing to support trusted proxy headers (NG_TRUST_PROXY_HEADERS) alongside allowed hosts (NG_ALLOWED_HOSTS). However, the changes in AngularNodeAppEngine and CommonEngine switch from merging environment-configured hosts with programmatic options to overriding them entirely. The reviewer noted that this is a breaking change and suggested reverting to merging both sources to preserve backward compatibility.
| allowedHosts: [...getAllowedHostsFromEnv(), ...(options?.allowedHosts ?? [])], | ||
| }); | ||
| this.trustProxyHeaders = options?.trustProxyHeaders; | ||
| allowedHosts: getAllowedHostsFromEnv() ?? options?.allowedHosts, |
There was a problem hiding this comment.
Changing 'allowedHosts' to use the nullish coalescing operator ('??') changes the behavior from merging the environment variable hosts with the programmatic options to completely overriding them. This is a breaking change for deployments that rely on both sources. To preserve backward compatibility, we should continue to merge them.
allowedHosts: [
...(getAllowedHostsFromEnv() ?? []),
...(options?.allowedHosts ?? []),
],There was a problem hiding this comment.
@alan-agius4, this is seems like reasonable feedback, though I'm hesitant to diverge this PR from the one it's porting, maybe this is a separate fix we should consider landing?
There was a problem hiding this comment.
I think this was done based on your feedback, so also keep the same behaviour between trustProxyHeaders and allowedHosts.
dgp1130
added
action: merge
2 participants
This is a v20 port of #33056.
Adds support for configuring trusted proxy headers via the
NG_TRUST_PROXY_HEADERSenvironment variable inAngularNodeAppEngine. This allows users to specify which proxy headers (such asX-Forwarded-Host) should be trusted when running the server-side application behind a reverse proxy, without needing to modify the application code. The environment variable accepts a comma-separated list of header names. If theNG_TRUST_PROXY_HEADERSenvironment variable is set and contains non-empty values, it will take precedence over thetrustProxyHeadersoption provided programmatically in theAngularNodeAppEngineconstructor options.(cherry picked from commit 126b19b)