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

Use nonce-based Content Security Policy #8598

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Use nonce-based Content Security Policy #8598

wants to merge 3 commits into from

Conversation

wpears
Copy link
Member

@wpears wpears commented Sep 30, 2024

This updates our CSP to use nonces, provided by Django-csp. This shrinks the size of the CSP header by ~7x while increasing its security. The main mechanism of CSP enforcement is strict-dynamic mode, which requires a nonce (or hash) for any script element included in the html, but allows the creation of further script elements from trusted scripts (so we don't have to specifically allow a bunch of random third-party domains if we're already trusting their initiators).

Note: the unsafe-inline and https: bits are only for backwards compatibility with browsers that support CSP but not strict-dynamic. Modern browsers ignore these entries when strict-dynamic is set. Removing unsafe-eval and unsafe-inline from our CSP will increase its efficacy incredibly :)

Because we're much more secure-by-default, I've also removed a bunch of the other IMG/STYLE/etc CSP settings which are now less important (because XSS to then include bad domains/styles is harder).

Another important note: due to limitations in how Wagtail includes scripts in the admin, I've made admin pages exclude the CSP. This isn't actually a problem because these pages are excluded from the public website. The only other solution of this was to override several base wagtail templates, which seemed like a big maintenance headache (for no real benefit). One exception to this was this the wagtail userbar, which I needed to override so to get working.

@wpears wpears requested review from chosak, willbarton and a team September 30, 2024 16:19
@willbarton
Copy link
Member

Another important note: due to limitations in how Wagtail includes scripts in the admin, I've made admin pages exclude the CSP.

It does look like Wagtail has a route to get there, albeit it's been a long one: wagtail/wagtail#1288

Copy link
Member

@willbarton willbarton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a few thoughts. Overall I think this is good, and I don't have much in the way of code review comments, just some additional questions:

  • I am curious how this plays with Akamai. Can we find a time slot to deploy it to beta?
  • Can we document, with a link to the django-csp docs (and maybe the MDN, or somewhere, CSP docs), when and where one needs to use request.csp_nonce?

It's unfortunate that we need to override the Wagtail userbar template, but this is clearly something others are doing to solve this exact problem, and Wagtail doesn't offer a good alternative solution.

@wpears
Copy link
Member Author

wpears commented Oct 1, 2024

I have a few thoughts. Overall I think this is good, and I don't have much in the way of code review comments, just some additional questions:

  • I am curious how this plays with Akamai. Can we find a time slot to deploy it to beta?
  • Can we document, with a link to the django-csp docs (and maybe the MDN, or somewhere, CSP docs), when and where one needs to use request.csp_nonce?

It's unfortunate that we need to override the Wagtail userbar template, but this is clearly something others are doing to solve this exact problem, and Wagtail doesn't offer a good alternative solution.

nonce docs added in bfc4bef
Happy to push to beta sometime sooh

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.

2 participants