feat: autogenerate default allowedHosts and trustProxyHeaders#427
feat: autogenerate default allowedHosts and trustProxyHeaders#427hrishikesh-k wants to merge 27 commits into
Conversation
✅ 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:
📝 WalkthroughWalkthroughThis PR adds an App Engine configuration module that exports getAllowedHosts() and getTrustProxyHeaders(), exposes them via package subpath exports and types, injects runtime env into Edge Function polyfills, wires the configuration into the demo AngularAppEngine handler, updates demo Angular dependency versions to ^21.2.11, adjusts some build error messages, tightens Context typing, and refreshes README examples to use Angular’s REQUEST/REQUEST_CONTEXT tokens, typed Context imports, and netlify-cli v26.0.0 requirement. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/app-engine-config.js (2)
11-19: ⚡ Quick winConsider adding error handling for URL parsing.
If any of the Netlify environment variables contain malformed URLs, the
new URL()calls will throw and potentially crash the build. While Netlify should always provide valid URLs, defensive error handling would make the code more robust.🛡️ Proposed error handling
export function getAllowedHosts() { const allowedHosts = [] if (!env.DEPLOY_ID || !env.DEPLOY_PRIME_URL || !env.DEPLOY_URL || !env.SITE_ID || !env.SITE_NAME || !env.URL) { console.warn('Missing Netlify-specific environment variable(s). `allowedHosts` config might be incomplete.') return allowedHosts } + try { const deployPrimeUrlHostname = new URL(env.DEPLOY_PRIME_URL).hostname // <subdomain>.netlify.app OR <custom-domain> // www handling is not required as Netlify will auto-redirect allowedHosts.push(new URL(env.URL).hostname) // <deploy-id>--<subdomain>.netlify.app allowedHosts.push(new URL(env.DEPLOY_URL).hostname) // <branch-name>--<subdomain>.netlify.app or <dp-#>--<subdomain>.netlify.app (supports ADS) allowedHosts.push(deployPrimeUrlHostname) + } catch (error) { + console.warn('Failed to parse Netlify URL environment variables:', error.message) + return 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-config.js` around lines 11 - 19, Wrap each new URL(...) call that pushes into allowedHosts (the uses of env.URL, env.DEPLOY_URL, and env.DEPLOY_PRIME_URL / deployPrimeUrlHostname) in defensive parsing: check the env var exists, attempt parsing inside a try/catch, and only push hostname to allowedHosts if parsing succeeds; on failure log or warn (e.g., via console.warn or processLogger) and skip that entry so a malformed Netlify URL cannot throw and crash the build. Ensure the code still computes deployPrimeUrlHostname from env.DEPLOY_PRIME_URL using the same guarded logic.
33-37: 💤 Low valueThe split logic assumes a single
--separator in the URL; if a branch name contained--, the extraction would be incomplete.The code at line 34 splits on
--and takes the first element, which works for typical Netlify deploy preview URLs likefeature--site.netlify.app. However, if a branch name itself contained--(e.g.,feature--fix), it would extract onlyfeatureinstead offeature--fix. This is a theoretical edge case, as Netlify likely sanitizes branch names for URL safety, but a more robust approach would split from the right to isolate the site name/ID portion.🤖 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-config.js` around lines 33 - 37, The current logic splits deployPrimeUrlHostname on '--' and takes the first element which fails if the branch name itself contains '--'; instead locate the last '--' (e.g., using lastIndexOf) and take the substring before that to produce branchNameOrDpNumber, then push the two allowedHosts using that value with env.SITE_NAME and env.SITE_ID; update the block that reads deployPrimeUrlHostname and the references to allowedHosts so the branch extraction uses the rightmost separator rather than split().
🤖 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 `@README.md`:
- Line 131: Update the README examples that reference context.url.pathname to
use the same URL parsing used in the runtime templates: replace usages of
context.url.pathname with new URL(request.url).pathname in the example API
endpoint checks (matching the logic in serverModuleHelpers.js); ensure all
occurrences (the examples around the earlier lines like the ones referenced)
consistently parse the request URL via new URL(request.url).pathname so the docs
match generated code.
In `@src/app-engine-config.js`:
- Around line 6-9: The current env-check block returns the empty allowedHosts
array when Netlify vars are missing, which causes AngularAppEngine to reject all
hostnames; change the fallback so that instead of returning allowedHosts (empty
array) you return undefined (or null) to let the engine use its default
permissive behavior, or alternatively populate allowedHosts with a safe default
(e.g., localhost and 127.0.0.1) — locate the env check that references
env.DEPLOY_ID / DEPLOY_PRIME_URL / DEPLOY_URL / SITE_ID / SITE_NAME / URL and
modify the early-return to return undefined (or set allowedHosts to a minimal
safe list) rather than the empty allowedHosts.
---
Nitpick comments:
In `@src/app-engine-config.js`:
- Around line 11-19: Wrap each new URL(...) call that pushes into allowedHosts
(the uses of env.URL, env.DEPLOY_URL, and env.DEPLOY_PRIME_URL /
deployPrimeUrlHostname) in defensive parsing: check the env var exists, attempt
parsing inside a try/catch, and only push hostname to allowedHosts if parsing
succeeds; on failure log or warn (e.g., via console.warn or processLogger) and
skip that entry so a malformed Netlify URL cannot throw and crash the build.
Ensure the code still computes deployPrimeUrlHostname from env.DEPLOY_PRIME_URL
using the same guarded logic.
- Around line 33-37: The current logic splits deployPrimeUrlHostname on '--' and
takes the first element which fails if the branch name itself contains '--';
instead locate the last '--' (e.g., using lastIndexOf) and take the substring
before that to produce branchNameOrDpNumber, then push the two allowedHosts
using that value with env.SITE_NAME and env.SITE_ID; update the block that reads
deployPrimeUrlHostname and the references to allowedHosts so the branch
extraction uses the rightmost separator rather than split().
🪄 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: a463a6f3-ee04-4e99-8a9a-94a46de86358
⛔ Files ignored due to path filters (2)
demo/package-lock.jsonis excluded by!**/package-lock.jsonpackage-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (8)
README.mddemo/package.jsondemo/server.tspackage.jsonsrc/app-engine-config.d.tssrc/app-engine-config.jssrc/helpers/serverModuleHelpers.jssrc/helpers/setUpEdgeFunction.js
|
As part of this PR we should also adjust angular-runtime/src/helpers/serverModuleHelpers.js Lines 30 to 54 in 5e8936e That "template" is used in 2 ways:
Additionally it probably would be good to check if scaffolding new angular project ( |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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-config.js`:
- Around line 7-13: The loop is using for...in so environmentVariable is an
index ("0","1",...) causing env[environmentVariable] to check wrong keys and the
function to return early; change the loop to for (const environmentVariable of
environmentVariables) so the item is the actual env var name, and remove the
early return inside the loop (only return allowedHosts after the loop completes)
to allow all allowedHosts to be populated; update checks that reference
environmentVariables, env, and allowedHosts 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: c080124a-7af3-4df4-b8bb-8791b265b53d
⛔ Files ignored due to path filters (1)
demo/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (2)
src/app-engine-config.jssrc/helpers/setUpEdgeFunction.js
🚧 Files skipped from review as they are similar to previous changes (1)
- src/helpers/setUpEdgeFunction.js
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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-config.js`:
- Around line 39-42: The code currently sets branchNameOrDpNumber by taking the
first token of deployPrimeUrlHostname.split('--'), which yields only the leading
fragment (e.g., "feature") instead of the full branch/DP token up to the last
delimiter (e.g., "feature--api"); change the extraction to use everything before
the last '--' delimiter (use lastIndexOf('--') on deployPrimeUrlHostname and
substring(0, idx) or split and join parts.slice(0, -1).join('--')) and then push
those correctly formed values into allowedHosts (the lines that push
`${branchNameOrDpNumber}--${env.SITE_NAME}.netlify.app` and
`${branchNameOrDpNumber}--${env.SITE_ID}.netlify.app`).
🪄 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: 05dc198c-4bbf-4448-b6f0-098b25c2bd6a
📒 Files selected for processing (1)
src/app-engine-config.js
|
@pieh thanks for the note about updating the template. I had gone through the codebase and had landed on that part, so updating that was on the to-do list. As we move on with the migration, I'll make sure to check the default server.ts generated by Angular 22 and update it - thanks for that nice piece of info! |
2 participants
Angular introduced
allowedHostsandtrustProxyHeadersconfig to control wheter or not to SSR a request. On Netlify, there could be a variety of URLs for each site/deploy. Thus, we now auto-generate a default config for the users.