Skip to content

Refactor on csp_nonce usage with django-csp #2088

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

Merged
merged 7 commits into from
Mar 19, 2025
Merged

Conversation

tim-schilling
Copy link
Member

@tim-schilling tim-schilling commented Feb 26, 2025

Description

This refactors how the CSP nonce is fetched. It's now done as a toolbar property and wraps the attribute request.csp_nonce

This avoids the toolbar from generating a nonce that gets injected into the CSP header when the view doesn't expect it to. It also supports using a nonce that is generated from any other point while processing the request, including other middleware.

Fixes #2082

Checklist:

  • I have added the relevant tests for this change.
  • I have added an item to the Pending section of docs/changes.rst.

Sorry, something went wrong.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
This refactors how the CSP nonce is fetched. It's now done as
a toolbar property and wraps the private attribute request._csp_nonce

This avoids the toolbar from generating a nonce that gets injected
into the CSP header when the view doesn't expect it to. It also
supports using a nonce that is generated from any other point
while processing the request, including other middleware.
@@ -42,6 +42,11 @@ def regular_view(request, title):
return render(request, "basic.html", {"title": title})


def csp_view(request):
"""Use request.csp_nonce to inject it into the headers"""
return render(request, "basic.html", {"title": f"CSP {request.csp_nonce}"})
Copy link
Member Author

Choose a reason for hiding this comment

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

The nonce needed to be rendered in the view. I chose to do it here.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
@robhudson
Copy link
Contributor

The latest commit on django-csp changes the way nonce access is handled that may help this situation and not have to use the private attribute.

If the nonce was used in the content it will still be available after the middleware has processed the response so other middleware can reference it.

If the nonce was NOT used in the content it will raise the error, but also checking the nonce via if request.csp_nonce will return False.

You can see the changes in the PR: mozilla/django-csp#269

@tim-schilling
Copy link
Member Author

The lazy object solution may not work for the toolbar in the case where CSPMiddleware appears before DebugToolbarMiddleware and the view doesn't use request.csp_nonce.

  1. CSPMiddleware pre-view, configures the lazy request.csp_nonce
  2. DebugToolbarMiddleware pre-view, does nothing relevant
  3. View doesn't use request.csp_nonce, but has static assets in the rendered template
  4. DebugToolbarMiddleware post view, finds request.csp_nonce as truthy, generates a nonce and uses it for the toolbar's static assets
  5. CSPMiddleware sees that request.csp_nonce has been used and includes it in the header

This seems like the CSP for this response would include the nonce in the header, on the toolbar assets, but not the user's own static assets. I believe that will violate the CSP and cause things to not load/run properly. Please correct me if I'm wrong here. CSP isn't something I'm great with.

I'd like to avoid having to dictate what order these middleware need to appear in. Though that may be me trying to have my cake and eat it too.

@jwhitlock
Copy link

OK, what if in step 4 request.csp_nonce is falsy? Would that work for you?

  1. CSPMiddleware pre-view, configures the lazy request.csp_nonce with FalseLazyObject
  2. DebugToolbarMiddleware pre-view, does nothing relevant
  3. View doesn't use request.csp_nonce, but has static assets in the rendered template
  4. DebugToolbarMiddleware post view, finds request.csp_nonce as falsly, does not generate a nonce or use it for the toolbar's static assets
  5. CSPMiddleware sees that request.csp_nonce is still unset and omits it from the header

@tim-schilling
Copy link
Member Author

@jwhitlock yup, yup. That would work well. A read-only csp_nonce is what the toolbar needs and why relying on the private attribute worked. I don't know how much of django-csp's API should be catered to this particular problem. I do appreciate the efforts you've gone to already.

@jwhitlock
Copy link

Thanks, I should be able to get that PR open today or tomorrow.

Until then, maybe the solution is to exclude django-csp 4.0b4? It seems 4.0b3 was OK, and hopefully 4.0b5 will as well.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
)

* Consolidate csp_nonce usages to a single property on the toolbar.

* Unpin django-csp for tests.
@tim-schilling tim-schilling changed the title Rely on django-csp's private attribute for nonce Refactor on csp_nonce usage with django-csp Mar 14, 2025
@tim-schilling tim-schilling requested a review from matthiask March 14, 2025 22:11
@tim-schilling
Copy link
Member Author

This has been updated to properly handle the new csp middleware as discussed in mozilla/django-csp#268

because the lazy object wrapped value can generate a nonce by
accessing it. This isn't ideal when the toolbar is injecting context
into the response because it may set a nonce not used with
other assets.
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe this comment is now out of date - no private attribute is being used.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thank you. Good catch!

Copy link
Member

@matthiask matthiask left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you.

I didn't know self.settings(), there's always something new.

@@ -16,6 +16,7 @@ backported
biome
checkbox
contrib
csp
Copy link
Member

Choose a reason for hiding this comment

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

Do we need this?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I think my note in the changelog triggered it as a word despite being django-csp

Copy link
Contributor

@robhudson robhudson left a comment

Choose a reason for hiding this comment

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

This looks good to me. django-csp==4.0b7 is out now. Hopefully that's the last one before the official 4.0 release.

@tim-schilling tim-schilling merged commit cbb479f into main Mar 19, 2025
53 checks passed
@tim-schilling tim-schilling deleted the django-csp-4.0 branch March 19, 2025 18:44
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.

Fix failing tests in CspRenderingTestCase
4 participants