inject default Angular SSR environment variables for local builds#10706
inject default Angular SSR environment variables for local builds#10706falahat wants to merge 13 commits into
Conversation
…d validate availability in local builds
Wiz Scan Summary
To detect these findings earlier in the dev lifecycle, try using Wiz Code VS Code Extension. |
![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 replicates the Go buildpack preparer's Angular environment variable injection and validation for NG_TRUST_PROXY_HEADERS and NG_ALLOWED_HOSTS. The feedback recommends adding a defensive check to ensure the parsed package.json is a non-null object before accessing its properties, and extending the NG_TRUST_PROXY_HEADERS validation to check both runtimeEnv and buildEnv to prevent users from bypassing validation.
… local builds preparer
…ict location checks for Angular SSR
falahat
force-pushed
the
local_builds_angular_ssr
branch
from
de626c2 to
5caf8fa
Compare
| const parsed: unknown = JSON.parse(fs.readFileSync(packageJsonPath, "utf8")); | ||
| if (parsed && typeof parsed === "object") { | ||
| const pkg = parsed as PackageJson; | ||
| const deps = { ...pkg.dependencies, ...pkg.devDependencies }; |
There was a problem hiding this comment.
I think this is creating new object in memory and shallow copying each key value dependency and might be inefficient. Maybe its better to check existence directly using pkg.dependencies?.["@angular/core"].
| } | ||
| } | ||
| } catch (e: unknown) { | ||
| logLabeledWarning( |
There was a problem hiding this comment.
In our internal version, we are erroring out when isAngularApplication returns an error, so I think we should log error here to match that instead of logging a warning.
| } catch (e: unknown) { | ||
| logLabeledWarning( | ||
| "apphosting", | ||
| `Failed to parse package.json in ${appDir}: ${e instanceof Error ? e.message : String(e)}`, |
There was a problem hiding this comment.
Nit: it'd be better to keep our error message as consistent as possible between go and typescript code "error when checking if application is angular: ... "
| "x-forwarded-host,x-forwarded-port,x-forwarded-proto,x-forwarded-for"; | ||
|
|
||
| // 1. Inject NG_TRUST_PROXY_HEADERS | ||
| // If they have defined RUNTIME env vars for the proxy header though we will log a warning that we're overriding it. |
There was a problem hiding this comment.
Our preparer go code validates if user-defined proxy headers is a subset of the allowed values, and if so respects user configuration. And it only overrides when user input is empty? Should we match this logic here too in CLI.
There was a problem hiding this comment.
Good call, I'll match this to defer to the user if they have defined somthing already.
| return true; | ||
| }); | ||
|
|
||
| const allowedHostsValue = [...defaultHosts, ...additionalHosts].join(","); |
There was a problem hiding this comment.
This logic here is merging user defined hosts with default hosts no matter what situation, while our Go buildpack does not inject default values if the user has defined NG_ALLOWED_HOSTS env var themselves.
There was a problem hiding this comment.
Good call, I'll adjust this to trust the user's setting if they have a set value. Same with the user proxy headers.
| // 2. Inject/Merge NG_ALLOWED_HOSTS | ||
| const projectNumber = await getProjectNumber({ project: projectId }); | ||
| const sharedDomain = "hosted.app"; | ||
| const defaultHosts = [ |
There was a problem hiding this comment.
This is missing the custom domains retrieved dynamically by the control plane during Cloud Build creation?
There was a problem hiding this comment.
ahh good call out. I think realistically, we won't be able to fetch the custom domains ahead of time in the CLI, so I will document this as a feature gap. For future improvements, we can consolidate the angular handling.
3 participants
Description
This adds NG_ALLOWED_HOSTS and NG_ALLOWED_PROXY_HEADERS similar to Firebase App Hosting Source Deploys. This is required for Angular SSR to work with Firebase Deployments.
Scenarios Tested
I tested Angular v19 through v22 with local vs source deploys and confirmed that the results were correct and the same (not falling back to CSR for local builds.)
Sample Commands