Skip to content
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

Add some initial CSP settings to test #527

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

will-moore
Copy link
Member

Fixes #21.

Based initially on reading docs at https://django-csp.readthedocs.io/en/latest/configuration.html and reading
https://blog.sucuri.net/2023/04/how-to-set-up-a-content-security-policy-csp-in-3-steps.html

This picks a handful of the most common settings and adds them to omeroweb.settings allowing them to be configured, and using "'self'" as the default value.

To test - page shouldn't be able to load images, scripts etc from other domains.

@will-moore
Copy link
Member Author

This is currently breaking webclient quite badly:

Screenshot 2024-01-23 at 09 48 59

Refused to apply inline style because it violates the following Content Security Policy directive: "style-src 'self'". Either the 'unsafe-inline' keyword, a hash ('sha256-gPuhVMNOoD9kBeWfIcvhcGP4SqBoRvNdtrvVU3QTY3E='), or a nonce ('nonce-...') is required to enable inline execution.

Refused to execute inline script because it violates the following Content Security Policy directive: "script-src 'self'". Either the 'unsafe-inline' keyword, a hash ('sha256-qec7pPGO104C8XSkN6GBy7gMcsk5nq0j1fbKN5iWB2Q='), or a nonce ('nonce-...') is required to enable inline execution.

jQuery.Deferred exception: Refused to evaluate a string as JavaScript because 'unsafe-eval' is not an allowed source of script in the following Content Security Policy directive: "script-src 'self'".
 EvalError: Refused to evaluate a string as JavaScript because 'unsafe-eval' is not an allowed source of script in the following Content Security Policy directive: "script-src 'self'".

Uncaught ReferenceError: WEBCLIENT is not defined
    at HTMLDocument.<anonymous> (ome.tree.js?_5.24.0:432:25)

Closing for now...

@will-moore will-moore closed this Jan 23, 2024
@will-moore will-moore reopened this Jan 23, 2024
@will-moore
Copy link
Member Author

After that change, I'm seeing errors like this on the login page:

login/:90 Refused to apply inline style because it violates the following Content Security Policy directive: "style-src 'self' unsafe-inline". Either the 'unsafe-inline' keyword, a hash or a nonce ('nonce-...') is required to enable inline execution....

Need to add quotes to 'unsafe-inline' too!...

@will-moore
Copy link
Member Author

will-moore commented Jan 24, 2024

With those changes, the webclient appears usable, but we're still seeing some errors in the Console:

jstree.js:1980 Refused to create a worker from 'blob:https://merge-ci.openmicroscopy.org/5141d68a-4f7f-450c-b079-6e1506845bd1' because it violates the following Content Security Policy directive: "script-src 'self' 'unsafe-inline' 'unsafe-eval'". Note that 'worker-src' was not explicitly set, so 'script-src' is used as a fallback.

From line


See https://stackoverflow.com/questions/54695310/getting-refused-to-create-worker-from-blob-error-in-video-min-js-when-looking "It means that you need explicitly add blob: data schema to default-src or worker-src"
"default-src * data: 'unsafe-eval' 'unsafe-inline' blob:"

Refused to load the image 'data:image/gif;base64,R0lGODlhEAAQAPIAAP///zxKW9H...Ow==' because it violates the following Content Security Policy directive: "img-src 'self'".

Seems from https://stackoverflow.com/questions/18447970/content-security-policy-data-not-working-for-base64-images-in-chrome-28 that we need data: added to img-src

@will-moore
Copy link
Member Author

Still seeing worker-src error message from above, and checking the Headers on merge-ci confirms that worker-src is missing:

Header:

Content-Security-Policy: img-src 'self'; style-src 'self' 'unsafe-inline'; script-src 'self' 'unsafe-inline' 'unsafe-eval'; default-src 'self'; base-uri 'self'

Need to check on why...?

@will-moore
Copy link
Member Author

Now seeing this Response Header on merge-ci:

Content-Security-Policy: default-src 'self'; worker-src 'self' blob:; base-uri 'self'; img-src 'self' data:; script-src 'self' 'unsafe-inline' 'unsafe-eval'; style-src 'self' 'unsafe-inline'

Not seeing any errors.
So this looks like a good starting point for default CSP settings.

@knabar is this something we want to progress with or put on hold for now until we feel more urgency for it?
Any feedback on the support for CSP in settings.py (does that look like a reasonable way to allow configuration? Any better way to do this with less code/settings)? Thx

@knabar
Copy link
Member

knabar commented May 2, 2024

@will-moore This looks like a good starting point, no objections to including it into the next release with the appropriate warnings to developers that certain things might need extra configuration to keep them working.

I initially thought that it could be better to have just one setting with a JSON struct for all CSP settings, but that would require more complex code to get the settings into the appropriate place in settings.py, whereas these individual settings work fine - more verbose, but a lot simpler.

So 👍 from me

@knabar knabar modified the milestones: 5.26.0, 5.27, 5.27.0 May 7, 2024
@will-moore
Copy link
Member Author

As discussed just now, need to think about docs etc and possibility of breaking users' plugins / modifications - Is this a breaking change etc before merging...

@knabar
Copy link
Member

knabar commented May 28, 2024

Possible issues:

  • What is the potential of this breaking plugins etc.?
  • What should be the default settings?
  • Is there a way to introduce this in a non-breaking way and only fully enable later?

cc: @sbesson @chris-allan

@knabar knabar removed this from the 5.27.0 milestone Jul 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Replace X-Frame-Options with Content-Security-Policy
2 participants