Update to latest Angular 20 and remove reliance on Host HTTP Header#5276
Update to latest Angular 20 and remove reliance on Host HTTP Header#5276tdonohue wants to merge 5 commits intoDSpace:mainfrom
Host HTTP Header#5276Conversation
928c091 to
b9d49f6
Compare
b9d49f6 to
7be4896
Compare
artlowel
left a comment
There was a problem hiding this comment.
Thanks @tdonohue!
The code looks good, I don't see any issues using it, and I've verified with @kshepherd's python script that you can no longer reproduce the issue with this PR.
7be4896 to
65e7a9d
Compare
|
@artlowel and @kshepherd : Just a note, I had to push up another small modification to this PR to workaround a new bug that I've discovered in environment variable overrides. See #5279. The reason this PR was failing the This doesn't change the PR significantly, but I wanted to point out the reason why I had to modify |
|
Unfortunately, I'll have to continue debugging this tomorrow, but this is an odd issue. This PR works entirely on my local machine (including env variable overrides), but still is having issues in GitHub CI. |
|
@tdonohue the SSR detection test also seems to be failing now (though the "start SSR" task succeeds") |
|
Finally figured out the issue here (after a lot of test commits which I'll cleanup/remove before moving this PR forward). We have a bug in our Docker Compose scripts which is not allowing for environment variable overrides to work properly. This is causing SSR issues because we must override the value of This is a separate bug from the one I've already reported, but is easily fixable. I'm planning to move it to a separate PR though because it's a bug impacting all Docker Compose scripts. UPDATE: This bug fix was moved to #5280. We'll need to merge that PR for this one to work properly. |
b3b7b6f to
65e7a9d
Compare
|
Hi @tdonohue, |
…tting existing environment.ui.baseUrl. Replace ServerHardRedirectService.getCurrentOrigin() with getBaseUrl() to read this setting.
… UI's hostname. This is now required for SSR to work.
…calhost:4000. Override that default in our docker-deploy CI step as that requires 127.0.0.1
…les because it's not listed in default-app-config.ts
65e7a9d to
879cb20
Compare
|
Just merged #5280 to fix the Docker bugs. I've rebased this on latest |
References
Description
This PR implements two main changes:
allowedHostssetting to Angular SSR which must specify the list of trusted hostnames.environment.ui.baseUrlsetting has been updated to allow sites to specify the public URL of their site. This value is then used to prepopulate theallowedHostssetting.HostHTTP Header, replacing it with using theenvironment.ui.baseUrl. Because this setting is now required, it's more secure then trusting theHostheader. (Second commit)HostHTTP Header in favor of theenvironment.ui.baseUrlconfiguration)This PR should be backported to
dspace-9_xas it also uses Angular 20.x.Instructions for Reviewers
config.*.ymlto add theui.baseUrl:npm run build:prod && npm run serve:ssrui.baseUrlset to your DSpace's public URL. You should see no errors in the SSR logs and the homepage & Community/Collection/Item pages should respond with Javascript disabled.ui.baseUrlto a different URL (e.g. https://my.dspace.org). This proves that Angular SSR is accurately validating the hostname via theallowedHostsparameter. You will see an error in the SSR logs that says something like this: