fix: generate default allowedHosts with available variables instead of skipping all due to a single missing one#441
Merged
Merged
Conversation
Co-authored-by: Michal Piechowiak <misiek.piechowiak@gmail.com>
✅ Deploy Preview for angular-runtime-demo ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR refactors Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/app-engine.js (1)
93-98: ⚡ Quick winReduce repeated warning noise for expected missing env vars.
This can emit multiple warnings per call in non-Netlify/local contexts where missing vars are expected. Consider aggregating missing variables and logging once per invocation (or once per process) to keep logs actionable.
Proposed change
function getEnvironmentVariable(environmentVariable) { const value = env[environmentVariable] // we inject the literal string "undefined" for variables that don't have a value if (value == null || value === '' || value === 'undefined') { - console.warn( - `Missing Netlify-specific environment variable ${environmentVariable}. ` + - '`allowedHosts` config might be incomplete.', - ) + missingNetlifyEnvVars.add(environmentVariable) return } return value } + +const missingNetlifyEnvVars = new Set() +let hasWarnedForMissingEnvVars = false + +function warnMissingNetlifyEnvVarsOnce() { + if (hasWarnedForMissingEnvVars || missingNetlifyEnvVars.size === 0) return + hasWarnedForMissingEnvVars = true + console.warn( + `Missing Netlify-specific environment variables: ${[...missingNetlifyEnvVars].join(', ')}. ` + + '`allowedHosts` config might be incomplete.', + ) +}export function getAllowedHosts({ allowedHosts = [], injectDefaults = true } = {}) { if (!injectDefaults) { return [...new Set(allowedHosts)] } const defaultAllowedHosts = [] @@ if (deployId && siteId) { @@ } + + warnMissingNetlifyEnvVarsOnce() return [...new Set([...defaultAllowedHosts, ...allowedHosts])] }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/app-engine.js` around lines 93 - 98, The current per-variable console.warn (checking value, environmentVariable and `'undefined'`) causes noisy repeated warnings; change this to aggregate missing environmentVariable names into a single list and emit one consolidated warning instead (either once per invocation or persist a module-level Set like missingEnvLogged to ensure each missing var is logged only once per process). Locate the block referencing value and environmentVariable in src/app-engine.js, collect the variables that meet the existing condition (value == null || value === '' || value === 'undefined') into an array/Set, and replace the per-item console.warn with a single console.warn that lists the aggregated names and the same message text.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/app-engine.js`:
- Around line 93-98: The current per-variable console.warn (checking value,
environmentVariable and `'undefined'`) causes noisy repeated warnings; change
this to aggregate missing environmentVariable names into a single list and emit
one consolidated warning instead (either once per invocation or persist a
module-level Set like missingEnvLogged to ensure each missing var is logged only
once per process). Locate the block referencing value and environmentVariable in
src/app-engine.js, collect the variables that meet the existing condition (value
== null || value === '' || value === 'undefined') into an array/Set, and replace
the per-item console.warn with a single console.warn that lists the aggregated
names and the same message text.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bb75ffc8-e17a-4cfb-9e14-67c41722a02c
📒 Files selected for processing (2)
src/app-engine.d.tssrc/app-engine.js
hrishikesh-k
changed the title
github-actions
Bot
added
the
type: feature
hrishikesh-k
changed the title
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/app-engine.d.ts`:
- Around line 8-16: Update the TypeScript declaration for getTrustProxyHeaders
to match runtime behavior: change its return type from string[] to boolean |
string[] so it can return true when trustAll is enabled; locate the
getTrustProxyHeaders declaration in the d.ts and adjust the signature to return
boolean | string[] while keeping the existing optional config overloads intact.
In `@src/app-engine.js`:
- Line 162: The Set construction in getTrustProxyHeaders is wrong: new
Set('x-forwarded-for', ...additionalTrustProxyHeaders) treats the string as
iterable and ignores additional args; replace it by creating the Set from a
single iterable array containing 'x-forwarded-for' and the spread
additionalTrustProxyHeaders (i.e., use new Set([...]) ), then return the array
form ([...theSet]) so duplicates are removed correctly; update the expression
that references additionalTrustProxyHeaders and getTrustProxyHeaders
accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 71cf33e7-6f75-41de-bd99-44fc65f8eb05
📒 Files selected for processing (2)
src/app-engine.d.tssrc/app-engine.js
pieh
reviewed
May 26, 2026
hrishikesh-k
changed the title
hrishikesh-k
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/app-engine.js (1)
7-82:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftExpose caller-supplied
allowedHostshere.This helper now only returns Netlify-derived defaults, so there is still no way to pass a custom allowlist or honor the
'*'skip-defaults behavior described in the PR. Becausesrc/helpers/serverModuleHelpers.js:52-58wiresallowedHosts: getAllowedHosts()directly intoAngularAppEngine, the new feature is unreachable from the generated server entrypoint.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/app-engine.js` around lines 7 - 82, getAllowedHosts currently only returns Netlify-derived defaults, so caller-supplied allowedHosts or the '*' skip-defaults flag never make it into AngularAppEngine; change getAllowedHosts to accept a parameter (e.g. allowedHosts or options.allowedHosts) and handle three cases: if the caller value === '*' return ['*']; if the caller provides an array, merge it with the computed defaultAllowedHosts (preserving uniqueness) so both custom and Netlify-derived hosts are included; otherwise behave as now. Update the caller in serverModuleHelpers.js (where allowedHosts: getAllowedHosts() is used) to pass through the provided allowedHosts so AngularAppEngine receives the combined/overridden list.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/app-engine.js`:
- Around line 138-141: getTrustProxyHeaders() is currently hard-coded to return
['x-forwarded-for'], preventing callers from appending/overriding proxy headers
when constructing AngularAppEngine; change it to accept an optional input/config
(e.g., a parameter or read from process/env) and merge defaults with
caller-provided headers so serverModuleHelpers.js can pass through or extend the
list. Update the function signature getTrustProxyHeaders and the call site in
serverModuleHelpers.js where new AngularAppEngine({ trustProxyHeaders: ... }) is
used to supply the merged array (default plus any extra headers from
config/options/API), and ensure AngularAppEngine receives the final combined
list.
---
Outside diff comments:
In `@src/app-engine.js`:
- Around line 7-82: getAllowedHosts currently only returns Netlify-derived
defaults, so caller-supplied allowedHosts or the '*' skip-defaults flag never
make it into AngularAppEngine; change getAllowedHosts to accept a parameter
(e.g. allowedHosts or options.allowedHosts) and handle three cases: if the
caller value === '*' return ['*']; if the caller provides an array, merge it
with the computed defaultAllowedHosts (preserving uniqueness) so both custom and
Netlify-derived hosts are included; otherwise behave as now. Update the caller
in serverModuleHelpers.js (where allowedHosts: getAllowedHosts() is used) to
pass through the provided allowedHosts so AngularAppEngine receives the
combined/overridden list.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
pieh
approved these changes
May 26, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
From Angular 22 (specifically 22.0.0-next.0, an incorrect host will skip falling back to CSR. It will return HTTP 400. Someone could be using a proxy or might need to specify non-Netlify-specific hosts, or even maybe a specific hostname that we cannot add by default. This PR changes the following:
allows specifying a custom allowedHosts listallows skipping the default allowedHosts config (automatically skipped when custom allowedHosts includes*)env[environmentVariable] === 'undefined'(it was getting swallowed by!env[environmentVariable])Setallow configuring trustProxyHeaders as well, users can now either specify an additional trust proxy headers list or trust all proxy headers