Skip to content

Avoid inline script execution for injecting CSRF token #7016

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 1 commit into from
Nov 29, 2022

Conversation

lukaw3d
Copy link
Contributor

@lukaw3d lukaw3d commented Oct 24, 2019

Description

Scripts with type="application/json" or "text/plain" are not executed, so we can use them to inject dynamic CSRF data, without allowing inline-script execution in Content-Security-Policy.

This helps towards fixing #6069 a bit.

@Farisiimoet3
Copy link

Unsubscribe

@stale
Copy link

stale bot commented Jun 23, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Jun 23, 2022
@lukaw3d lukaw3d force-pushed the csp-unsafe-script branch from 95b9ef8 to 9f47c6e Compare June 25, 2022 19:24
@stale stale bot removed the stale label Jun 25, 2022
@lukaw3d lukaw3d changed the title Separate CSRF data from executed javascript code to support CSP Avoid inline script execution for injecting CSRF token Jun 25, 2022
Scripts with type="application/json" or "text/plain" are not executed, so we can
use them to inject dynamic CSRF data, without allowing inline-script execution
in Content-Security-Policy.
@lukaw3d lukaw3d force-pushed the csp-unsafe-script branch from 9f47c6e to 91d8807 Compare June 25, 2022 19:29
@stale
Copy link

stale bot commented Oct 16, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Oct 16, 2022
@juspence
Copy link
Contributor

@tomchristie Is there any way I can help get this PR (as well as #5740 and #7960) merged? I'm happy to help test and provide feedback. Merging any of these would help to resolve #6069, which is blocking me from using stricter Cross-Site Scripting (XSS) protections.

@twbagustin
Copy link

Replying on this one seems more up to date, since #5740 seems a bit outdated (shows conflicts), #7960 does seem a bit off because request.csp_nonce is something added by another package (https://github.com/mozilla/django-csp)
Here it's missing the ajax form submission thing https://github.com/encode/django-rest-framework/blob/3.14.0/rest_framework/templates/rest_framework/base.html#L302
What I've done locally is moving this $('form').ajaxForm(); inside static/rest_framework/default.js (seems like the right place)
and updating static/rest_framework/crsf.js exactly as you are doing here, reading JSON from the template thus the update on templates/rest_framework/base.html
Subscribing for the notifications :D

@tomchristie
Copy link
Member

@tomchristie Is there any way I can help get this PR (as well as #5740 and #7960) merged?

I'm happy to add collaborators to the @encode/django-rest-framework team if asked.
That'll give folks review and merge permissions on pull requests.

@juspence
Copy link
Contributor

@tomchristie I can't commit to spending any effort beyond this one feature. If that's still OK, please add me to the encode/drf team, and I'll review, test, and merge these three PRs (#5740, #7016, and #7960). Thank you!

@auvipy
Copy link
Member

auvipy commented Nov 29, 2022

@tomchristie I can't commit to spending any effort beyond this one feature. If that's still OK, please add me to the encode/drf team, and I'll review, test, and merge these three PRs (#5740, #7016, and #7960). Thank you!

you can handle this one and I can help you review and merge this. we now have 3 more new maintainers. can you test and share your feedback on this PR? after your confirmation I can merge it. the other open PR's need more works before merge.

@auvipy auvipy self-requested a review November 29, 2022 07:57
@tomchristie
Copy link
Member

@juspence - Invite sent.

@juspence
Copy link
Contributor

juspence commented Nov 29, 2022

@auvipy This looks good to me. I tested Django-REST-Framework without this change in Firefox 102.4.0esr and Chrome 106.0.5249.119, using a Content-Security-Policy like "script-src: 'self'" that doesn't allow inline scripts.

I saw the below error for the CSRF script in the developer console, like I expected:

Content Security Policy: The page’s settings blocked the loading of a resource at inline (“script-src”). 
[components:334:1](http://localhost:8008/api/v1/components)
Uncaught TypeError: window.drf is undefined
    <anonymous> http://localhost:8008/static/rest_framework/js/csrf.js:41
[csrf.js:41:17](http://localhost:8008/static/rest_framework/js/csrf.js)
    <anonymous> http://localhost:8008/static/rest_framework/js/csrf.js:41

Then I tested again with this change, and the above error went away.

@tomchristie
Copy link
Member

@juspence - Great. You're welcome to approve pull requests that you're happy with.

@juspence juspence self-assigned this Nov 29, 2022
@juspence juspence self-requested a review November 29, 2022 16:08
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.

6 participants