Conversation
thibaudcolas
left a comment
thibaudcolas
left a comment
There was a problem hiding this comment.
Nice! This seems pretty mergeable to me if those changes are limited to the one settings and requirements files. I think it’d make sense to merge this as soon as Django 6.0 alpha 1 is released, possibly even before, so we can try this out on the main branch of bakerydemo. Just making sure any loading of Django 6.0 APIs are behind a Django version check.
Also we would need to remove the dependency on django-csp.
requirements/base.txt
Outdated
| @@ -1,4 +1,4 @@ | |||
| Django>=5.2,<5.3 | |||
| Django>=6.0,<7.0 | |||
There was a problem hiding this comment.
| Django>=6.0,<7.0 | |
| Django>=5.2,<6.0 |
I believe this will allow us to have working installs for most users, and for early adopters support installing release candidates of Django 6.0
There was a problem hiding this comment.
With the <6.0, wouldn't that mean that it would not be possible to install a release candidate for 6.0?
There was a problem hiding this comment.
Also, we could remove the dependency on django-csp when 6.0 is available. For now, I suppose we could leave it, though.
bakerydemo/settings/base.py
Outdated
| SECURE_CSP_REPORT_ONLY = { | ||
| "default-src": [CSP.SELF, "*.wagtail.org"], | ||
| # Add more directives as needed. | ||
| } |
There was a problem hiding this comment.
Could you update this so we can preserve how this is done with environment variables? I think ideally we would want to preserve our ability to configure this with environment variables, just do this with the vanilla Django implementation.
I think this should also be done only when if "CSP_DEFAULT_SRC" in os.environ:.
bakerydemo/settings/base.py
Outdated
| import os | ||
|
|
||
| import dj_database_url | ||
| from django.utils.csp import CSP |
There was a problem hiding this comment.
Is it possible for us to move this and the middleware addition within a Django version check, so we can merge this while still supporting Django 5.2?
bakerydemo/settings/base.py
Outdated
| # Gravatar images should be disabled for strict CSP | ||
| WAGTAIL_GRAVATAR_PROVIDER_URL = None |
There was a problem hiding this comment.
I think we would want to keep using Gravatar, it’s just a matter of configuration no? We can allow loading images from there while still having a strict CSP.
There was a problem hiding this comment.
Yes, we can do this. But we would be trusting all the content from Gravatar to not be malicious. What do you think?
Chiemezuo
force-pushed
the
test/csp-settings
branch
from
90989f7 to
03b57bb
Compare
2 participants
Introduction
From Django 6.0 which should be released in December 2025, Django will have an in-built CSP package, and will no longer need the external django-csp package.
This PR, although small, has some months to go before being ready for a merge, but it's good to have this under our radar.