-
-
Notifications
You must be signed in to change notification settings - Fork 33.1k
Fixed #15727 -- Added Content Security Policy (CSP) support. #18215
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
Conversation
ea4116b
to
8edd4a7
Compare
a698389
to
62196e7
Compare
I refactored the code today after review from Adam.
What's left:
|
e6bd8cd
to
f53f30e
Compare
d978052
to
40e9097
Compare
I'm going to set this as ready for review. I think it's feature complete for a first that includes the middleware to set the CSP headers, a couple decorators to fine tune the CSP policy for views, and a context processor for accessing the nonce easily. Inline there are a few |
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.
Hi @robhudson - I think you've not had a review as the PR was not in the review queue (see "Patches that need review" within Reviewing patches)
That's because this ticket currently has "Needs documentation" marked as "yes"
Can you add a release note to Django 5.2, add a couple .. versionadded:: 5.2
notes, and then mark "Needs documentation" as "no" in the ticket so it appears in the review queue?
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.
Thank you @robhudson - I added initial comments
django/middleware/csp.py
Outdated
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.
I think it makes sense to have a security warning, this is a recommendation
We can do that in a separate commit/PR
8fe7c1d
to
d30fab2
Compare
This adds a bullet point about policy exclusion risks. This also orders these points by what I consider highest risk to lowest.
* Reorganizes top-level sections for a more intuitive flow. * Clarifies differences between SECURE_CSP and SECURE_CSP_REPORT_ONLY, including which headers they affect. * Adds detailed information on using nonces in CSP policies.
Can be placed near the bottom, but ensure any middleware that accesses ``request.csp_nonce`` is | ||
positioned after it, so the nonce is properly included in the response header. |
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.
Can be placed near the bottom, but ensure any middleware that accesses ``request.csp_nonce`` is | |
positioned after it, so the nonce is properly included in the response header. | |
Before any middleware that accesses ``request.csp_nonce``. |
Is there any middle-ware it should be after? Or it doesn't matter if it was first?
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.
Looking at the default list of middleware for a new project, I don't see anything that would affect it being first, since it just reads the settings and writes the header. The only tricky bit is if a nonce is used during content creation will affect whether the nonce is included in the header. So if there's a middleware that updates content in some way related to the nonce (unusual), that middleware would need to go after the CSP middleware.
* Third-party Integration Complexity: If your application relies heavily on | ||
third-party scripts or services, implementing a strict CSP can be challenging | ||
and may require careful configuration and ongoing maintenance. |
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.
I'm not sure we need to document this one
docs/ref/csp.txt
Outdated
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.
I agree let's move this as a subheading within the CSP section for security
@@ -0,0 +1,459 @@ | |||
.. _content-security-policy: |
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.
I think what should keep in this reference docs:
- the CSP enum
- the decorators
I think there should be a "how-to" guide which has the
- Using CSP (perhaps linking to the decorators or some decorators stuff pulled in)
- Nonce Usage
I think the some of the "Limitations and Considerations" should be moved to the CSP security topic
The settings information should mostly be covered by the settings docs
To give some background as to "why", this is within our reference docs and includes quite a bit of how-to and explanation which would normally be within either a how-to or topic. We get feedback that our reference docs are too verbose so we need to try and put stuff in the right places
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.
Point for @nessita, in the how-to doc it would be worth having an example of something that would be blocked (in order to say/show how you can "see" it/test it/understand it while playing out with it)
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.
Leaning towards using a topic rather than a how-to
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.
I think it should be a how-to, like the CSRF one. It'll need to include the how to content on configuring the middleware and using the decorators.
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.
Thank you for your Continuous Sustained Persistence (C.S.P.) here Rob.
I have a few comments here. The only thing I'd change on the design at this point is dropping the DIRECTIVES
key, per the below.
# In DEBUG mode, exclude CSP headers for specific status codes that | ||
# trigger the debug view. | ||
exempted_status_codes = { | ||
http_client.NOT_FOUND, | ||
http_client.INTERNAL_SERVER_ERROR, | ||
} | ||
if settings.DEBUG and response.status_code in exempted_status_codes: | ||
return response |
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.
FYI, I opened a ticket on the debug view not being CSP-friendly a long time ago: https://code.djangoproject.com/ticket/33180
After this PR is merged, we can work toward using nonces in the debug views, and maybe this workaround can then be dropped.
@@ -0,0 +1,459 @@ | |||
.. _content-security-policy: |
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.
I think it should be a how-to, like the CSRF one. It'll need to include the how to content on configuring the middleware and using the decorators.
docs/ref/csp.txt
Outdated
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.
The DIRECTIVES key allows room for future CSP wide configuration that aren't directive specific to keep policy logic separate from middleware behavior.
I'm feeling some YAGNI looking at the DIRECTIVES
sub-key. I would rather we dropped to just a dict of directives.
The concerns you listed (NONCE_GENERATOR
, POLICY_OVERRIDES
, and "extensibility") can already be addressed in other ways, such as overriding the middleware or using decorators.
Meanwhile, a sub-key DIRECTIVES
prevents the setting from mapping cleanly to the header contents, while analogous security header settings, such as SECURE_REFERRER_POLICY
, do. (Although they contain simple string values.) So, having a "just in case" substructure feels slightly less Django-ey.
If we do end up with something else to change beyond the policy directives, we could add another setting or document how to subclass the middleware and swap out the appropriate method. The latter is likely to be preferable for fairly niche features.
What's new in Django 6.0 | ||
======================== | ||
|
||
Content Security Policy (CSP) Support |
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.
We don't title-case headings
Content Security Policy (CSP) Support | |
Content Security Policy (CSP) support |
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.
(This comment applies to all titles in the updated docs here.)
Django 6.0 introduces built-in :ref:`Content Security Policy (CSP) | ||
<security-csp>` support, making it easier to protect your application against | ||
various types of content injection attacks. CSP helps you specify which content | ||
sources are allowed to be loaded by the browser. |
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.
Django 6.0 introduces built-in :ref:`Content Security Policy (CSP) | |
<security-csp>` support, making it easier to protect your application against | |
various types of content injection attacks. CSP helps you specify which content | |
sources are allowed to be loaded by the browser. | |
The new built-in :ref:`Content Security Policy (CSP) <security-csp>` support | |
makes it easier to protect your application against content injection attacks. | |
CSP is a security standard that allows you to specify which resources browsers | |
are allowed to load. |
We don't normally say "Django " throughout the release notes. Also, I edited for brevity clarity.
* :setting:`SECURE_CSP` for the enforced policy | ||
* :setting:`SECURE_CSP_REPORT_ONLY` for the report-only policy | ||
|
||
Here's an an example of a basic CSP config:: |
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.
Here's an an example of a basic CSP config:: | |
Here's an an example of a basic CSP configuration:: |
Use full words wherever possible.
- remove constants and unnecessary comments - use secrets.token_urlsafe for nonce - use HTTPStatus - refactor decorator header check loop - refactor build_policy loop - support sets as CSP setting value
I've addressed all the code-specific feedback discussed so far. My next steps are: Documentation reorganization: I'll move implementation details from the reference guide into the how-to documentation, keeping Breaking down the PR: As suggested, I'll split this into smaller, more manageable chunks:
Update: Since the context processor is for the nonce and the how-to would show configuring the context processor to use in the templates to define where the nonce goes, that will go with the first PR. The follow up will then only be the decorator. |
|
||
|
||
@register(Tags.security) | ||
def check_csp_directives(app_configs, **kwargs): |
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.
def check_csp_directives(app_configs, **kwargs): | |
def check_csp_settings(app_configs, **kwargs): |
I think, since we dropped DIRECTIVES
"Each enabled CSP setting (SECURE_CSP, SECURE_CSP_REPORT_ONLY) must be a " | ||
"dictionary.", |
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.
Can we put the setting name in the message? Otherwise, it leaves the user hunting for where the mistake may be. I'm thinking of complex settings configurations where production has a report-only policy separate from some base configuration.
# Only validate if the setting is explicitly set (not None or empty dict) | ||
if setting_value: | ||
if not isinstance(setting_value, dict): | ||
errors.append(W026) |
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.
Maybe we can do the below, so we still trigger a warning for an empty list and similar falsey values?
# Only validate if the setting is explicitly set (not None or empty dict) | |
if setting_value: | |
if not isinstance(setting_value, dict): | |
errors.append(W026) | |
if setting_value is not None and not isinstance(setting_value, dict): | |
errors.append(W026) |
A new PR has been filed at #19393 The new PR:
|
Trac ticket number
ticket-15727
Branch description
This adds CSP support for both the enforced header and report-only header.
There are a number of
TODO
notes within this PR that I'm looking for feedback on.Checklist
main
branch.