-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Csp updates #15003
Merged
Merged
Csp updates #15003
Conversation
This file contains 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
shogunpurple
requested review from
mike12345567
and removed request for
a team
November 10, 2024 13:27
QA Wolf here! As you write new code it's important that your test coverage is keeping up. |
mike12345567
approved these changes
Nov 15, 2024
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.
LGTM! One minor comment about environment files - thats it!
shogunpurple
force-pushed
the
csp-updates
branch
2 times, most recently
from
November 15, 2024 16:14
f973a41
to
ab4969b
Compare
shogunpurple
force-pushed
the
csp-updates
branch
from
November 15, 2024 16:36
ab4969b
to
dcbe4f0
Compare
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
Description
This PR contains code that moves the management of our
Content-Security-Policy
to our node backend, rather than managing it through NGINX. This was a security requirement for an enterprise customer, and in order to remove theunsafe-inline
rule from our CSP, which allows any kind of inline script, we need to dynamically generate a hash (or nonce) and have that returned dynamically in the CSP header, as well as injected into any script tags that Budibase needs to run. Doing this has the added benefit of allowing people to control and switch off CSP altogether with an environment variable -DISABLE_CONTENT_SECURITY_POLICY
. Today, users do this by bind mounting the NGINX config and disabling it directly there, which means that they have effectively "ejected" their NGINX config, and if we update it they won't get the update in later versions of ourproxy-service
- causing reverse proxy issues in some cases.It also turns the CSP on in dev. I think this is valuable, as it means we can catch CSP issues before pushing to QA or even prod. If there's any dev specific CSP exceptions that need to be made, we can do it in the CSP middleware rather than turning it off altogether.
Testing
unsafe-inline
rule, to ease migration, as this is technically a change for one customer therefore the exception rather than the ruleAddresses
Launchcontrol
Move Content-Security-Policy management into the node app code, rather than hardcoding it in NGINX. New
DISABLE_CONTENT_SECURITY_POLICY
env var to allow people to switch it off without editing NGINX config