-
-
Notifications
You must be signed in to change notification settings - Fork 656
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: Allow extra CSP domains #1610
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Ignored Deployment
|
fe82272
to
5ab1c6b
Compare
Co-authored-by: Fredrik Oseberg <fredrik.no@gmail.com>
ede3d85
to
93e5729
Compare
93e5729
to
a5f777c
Compare
Coverage report
Show new covered files 🐣
Show files with reduced coverage 🔻
Test suite run success887 tests passing in 125 suites. Report generated by 🧪jest coverage report action from e683d93 |
src/lib/middleware/secure-headers.ts
Outdated
"'self'", | ||
'cdn.getunleash.io', | ||
'gravatar.com', | ||
...config.additionalCspAllowedDomains?.defaultSrc, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will these spreads crash if additionalCspAllowedDomains
is undefined
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, because if additionalCspAllowedDomains is undefined parseCspEnvironmentVariables()
will trigger and provide defaults.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh right! Maybe additionalCspAllowedDomains
can be required in IUnleashConfig
then, to remove the ?
s.
When setting up our hosted instance we would really like to be allowed to add another domain to our allowed source domains, so that we can add plausible integration.
Co-authored-by: Fredrik Oseberg fredrik.no@gmail.com