Opened 4 years ago

Closed 3 years ago

Last modified 3 years ago

#16936 closed New feature (fixed)

CSRF with AJAX documentation is out-of-date

Reported by: idangazit Owned by: nobody
Component: Documentation Version: master
Severity: Normal Keywords:
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Following the release of Django 1.2.5, we issued new guidelines on using CSRF protection with AJAX requests: https://www.djangoproject.com/weblog/2011/feb/08/security/

In that release, we included a JS snippet showing how to properly set the CSRF token header on AJAX requests, which never made it into the docs.

In addition, the existing docs on using CSRF with AJAX are not as good as they could be. Right now, we mix together discussion of how to get the CSRF token and how to use it—breaking these out into logical sections would make the docs easier to read.

Because the changes I'm making touch on security-related issues, I'd really like several pairs of practiced eyes to go over it before we make a change.

Attachments (3)

ajax_csrf.patch (6.0 KB) - added by idangazit 4 years ago.
Docs patch with clearer AJAX & CSRF explanations.
ajax_csrf_2.patch (7.9 KB) - added by idangazit 4 years ago.
Better CSRF docs patch, incorporates fixes from comments above.
16936.diff (7.9 KB) - added by timo 3 years ago.
Updated to apply cleanly to master

Download all attachments as: .zip

Change History (15)

Changed 4 years ago by idangazit

Docs patch with clearer AJAX & CSRF explanations.

comment:1 Changed 4 years ago by anonymous

A minor comment on the docs part, you say:

 	100	If the CSRF token is not present in markup by use of the :ttag:`csrf_token` 
 	101	template tag, it must be supplied to the client by other means. This is common 
 	102	in cases where the form is dynamically added to the page, and Django supplies 
 	103	a decorator which will set a cookie containing the CSRF token: 
 	104	:func:`~django.views.decorators.csrf.ensure_csrf_cookie`. Use the decorator 
 	105	on any view which will need access to the CSRF token, but doesn't include the 
 	106	token in the actual markup sent to the client. 

Small clarification, but I think of view as referring to python code specifically, not the more abstract combination of view and content it renders for display.

So perhaps something like "If you need access to the CSRF on the client and it is not made available in the markup, use the CSRF decorator on the associated view to set the CSRF cookie"

Specifying that the need is on the client side, instead of in the "view" which I think of as the Django side.

comment:2 Changed 4 years ago by anonymous

Also to highlight something I think PaulM saw and commented on in IRC, there is either a bug in the original/current snippet in the docs, or my logic is failing.

This line was removed from the current docs in the patch:

126	 	        if (!safeMethod(settings.type) && sameOrigin(settings.url)) { 

and this was added:

145	            if (!(/^https?:.*/.test(settings.url)) && safeMethod(settings.type) ) { 

the logic of safeMethod is reversed - but as I read it, the patch is correct, you only want to send the cookie if the method was safe. If that is not the case, that function should be renamed to be much clearer.

Also not clear in the current patch, nor the original security release notes, is why relative URLs should be enforced. A brief note explaining this in the docs would be beneficial.

comment:3 Changed 4 years ago by ptone

doh - a noisy note to say, FWIW, that the above two comments are from me

comment:4 Changed 4 years ago by lukeplant

Is there a reason the patch reverts [16183] and removes sameOrigin?

comment:5 Changed 4 years ago by lukeplant

BTW, if I remember correctly, the snippet that gets the csrftoken from the DOM was deliberately rejected in favour of one that gets it from the cookie. If you have the token in the DOM, you are guaranteed to have it in the cookie too if you used the normal mechanism. So it seemed to make more sense to rely on the cookie.

@ptone: other parts of CSRF docs do mention the fact that if you send a CSRF token cross-domain you have a security issue - the other domain can use the token to do a CSRF attack on you.

comment:6 Changed 4 years ago by ptone

  • Has patch set
  • Patch needs improvement set
  • Triage Stage changed from Unreviewed to Accepted
  • Version changed from 1.3 to SVN

I checked, and yes both the context_processor, and decorator call the middleware, so the cookie should be available. One thing to mention in this part of the docs, is that the cookie name may not be the default if the CSRF_COOKIE_NAME has been changed.

Also, the csrf input element will be present and set to "" if the context processor was run without either csrf decorator or middleware setting the cookie

@lukeplant my comment about enforcing relative URLs was basically the same one you made about removing same origin, that is, disallowing full, same origin URLs - I wasn't aware of how new r16183 was.

Either way - this should be accepted and improved

comment:7 Changed 4 years ago by idangazit

@lukeplant:

Regarding [16183]:

Looking at this again with fresh eyes, I see that I mixed up the date order — [16183] came after the blog post (linked in ticket description). Posting a new patch backing out that deletion shortly, my bad.

Regardless, there should be an explicit mention that the sameOrigin logic adheres to the recommendation of that blogpost, so it doesn't appear we're providing conflicting recommendations.

Regarding DOM vs. cookie for acquiring CSRF token.

Again, the blogpost cited above recommends a snippet which is getting the value out of the DOM. If this isn't a good way to get the snippet, then we should update the blogpost's example. Either way, will update patch to omit the DOM method.

@ptone

Good catch regarding safeMethod. I'm changing the method name to be a bit clearer about what it does.

Changed 4 years ago by idangazit

Better CSRF docs patch, incorporates fixes from comments above.

comment:8 Changed 4 years ago by lukeplant

@idangazit:

As I remember there was a series of changes in quick succession. Relatively few people have access to the blog to change it (I don't), so it only got updated in the docs. For this reason I think we need to refrain from putting code and instructions like that on the blog where it is difficult to correct.

comment:9 Changed 3 years ago by timo

  • Patch needs improvement unset

Changed 3 years ago by timo

Updated to apply cleanly to master

comment:10 Changed 3 years ago by timo

I've updated the patch to apply cleanly to master. My only question is whether it was a conscious decision to remove the note: "Due to a bug introduced in jQuery 1.5, the example above will not work correctly on that version. Make sure you are running at least jQuery 1.5.1." Otherwise, I'll try to give this a final test and commit it sometime soon.

comment:11 Changed 3 years ago by Tim Graham <timograham@…>

  • Resolution set to fixed
  • Status changed from new to closed

In [e376558ed257a4f8ce63779058f3d181ef49caa0]:

Fixed #16936 - Updated javascript for CSRF protection.

Thanks Idan Gazit for the patch.

comment:12 Changed 3 years ago by Tim Graham <timograham@…>

In [c088a42670df867667401f131b865d9c7686667f]:

[1.4.X] Fixed #16936 - Updated javascript for CSRF protection.

Thanks Idan Gazit for the patch.

Backport of e376558ed2 from master

Note: See TracTickets for help on using tickets.
Back to Top