Code

Opened 6 years ago

Closed 5 years ago

#8127 closed (fixed)

CSRF + AJAX

Reported by: xlax Owned by: nobody
Component: Contrib apps Version: master
Severity: Keywords: CSRF AJAX
Cc: jeroen.pulles@…, carl@…, stefan.cornelius@…, lidaobing@…, johannes.beigel@…, neithere@… Triage Stage: Design decision needed
Has patch: yes Needs documentation: no
Needs tests: yes Patch needs improvement: no
Easy pickings: UI/UX:

Description

I've got a patch ready for anyone who needs it (if it's accepted) to use CSRF with AJAX.
It's simply adding a line to the main CSRF function, stating that if AJAX is being used, there's no need for a check.
I know of no issues regarding this. I can submit the file immediatelly. (It's been tested here)

Attachments (3)

csrf_middleware_ajax.diff (528 bytes) - added by xlax 6 years ago.
csrf_check_ajax_requests_too.diff (890 bytes) - added by Jeroen Pulles <jeroen.pulles@…> 6 years ago.
Get the CSRF token from HTTP headers when using Ajax requests
csrf_middleware_ajax_flash_patch.diff (824 bytes) - added by herrstagl 6 years ago.
patch which allows to provide a session id within a post request: userfull for flash based uploaders where authentication is required (done for www.xipax.com)

Download all attachments as: .zip

Change History (22)

comment:1 Changed 6 years ago by xlax

  • Component changed from Uncategorized to Contrib apps
  • Keywords CSRF AJAX added
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

comment:2 Changed 6 years ago by simon

Assuming this is a check for the X-Requested-By: XMLHttpRequest header, I'm all for it - it's a nice, secure way of allowing Ajax requests to avoid having to include a form token, which can be tricky to make available to the Ajax code otherwise.

comment:3 Changed 6 years ago by xlax

It's using the request.is_ajax() function. This is documented here: http://www.djangoproject.com/documentation/request_response/#methods

Quote:
"""
Returns True if the request was made via an XMLHttpRequest, by checking the HTTP_X_REQUESTED_WITH header for the string 'XMLHttpRequest'.
"""

I'm currently trying to do the svn diff. First time I did it, so please don't shoot me if I did something wrong :)

Changed 6 years ago by xlax

comment:4 Changed 6 years ago by xlax

Edit
I have attached the diff file, I hope it is as wanted...

comment:5 Changed 6 years ago by xlax

  • Has patch set
  • Needs tests set

comment:6 Changed 6 years ago by ericholscher

  • Triage Stage changed from Unreviewed to Design decision needed

comment:7 Changed 6 years ago by Jeroen Pulles <jeroen.pulles@…>

Hi,

Actually, this is a problem that might bite the unwary: If you use the CSRF middleware, and do JSON posts, you will run into trouble right now.

Personally, I prefer to check the key in JSON request bodies as well (being a bit paranoid, hey I'm using the CSRF middleware :-/).

So the attached diff (csrf_check_ajax_requests_too.diff) checks for a HTTP_X_CSRFMIDDLEWARETOKEN in the HTTP header. You can easily add that header in most Ajax libraries, e.g. request.setHeader('X-CsrfMiddlewareToken', document.getElementById('csrfmiddlewaretoken').value); .

The only downside you have is that there might not be a key on a webpage because it has no <form method="post"> in the response. That seems to be the exception in my experience, and that can be worked around just as easy.

Changed 6 years ago by Jeroen Pulles <jeroen.pulles@…>

Get the CSRF token from HTTP headers when using Ajax requests

comment:8 Changed 6 years ago by anonymous

  • Cc jeroen.pulles@… added

comment:9 Changed 6 years ago by Jeroen Pulles <jeroen.pulles@…>

Uh, oops:

My patch should have checked the Content-Type of the request body, and assume that requests of the type application/x-www-form-urlencoded are still handled with the existing request.POST['csrfmiddlewaretoken'] .

comment:10 Changed 6 years ago by Jeroen Pulles <jeroen.pulles@…>

More rationale:

Actually, this is a problem that might bite the unwary: If you use the CSRF middleware, and do JSON posts, you will run into trouble right now.

That goes for any POST request that isn't application/x-www-form-urlencoded. Which is why it would be welcome to make the request processing more robust, regardless of the design choice to check for the token or not.

comment:11 Changed 6 years ago by xlax

Perhaps the best way to go is to make CSRF generate a key per-standard, and make it available. That way, at all points in the application, the string is usable and the ajax user could implement that into the "form" sent through.

comment:12 Changed 6 years ago by jmillikin

I found a SecurityFocus article[1] that mentions that Adobe Flash is capable of both cross-site requests and setting arbitrary HTTP headers. If this is true, then relying on the X-Requested-With header will allow hostile cross-site requests if a user visits a malicious site.

Is this concern accurate, or does Flash enforce the same cross-site rules as the XMLHttpRequest object?

[1] http://www.securityfocus.com/archive/1/441014

comment:13 Changed 6 years ago by herrstagl

We had some issues with csrf and ajax when we used an flash uploader in combination with windows and firefox. in this special case, flash is sending the IE cookies instead of the Firefox cookies and therefore CSRF raises an error because no valid session is available. What we did, to get csrf working even with the flash YUI upload:

  • we added the csrfmiddlewaretoken to the ajax post request
  • we wrote a patch during the implementation of www.xipax.com for the middleware.py which checks if there is POST variable called session_id, if not, it falls back to the standard behaviour of the CSRF middleware

This file is attached to the ticket

Changed 6 years ago by herrstagl

patch which allows to provide a session id within a post request: userfull for flash based uploaders where authentication is required (done for www.xipax.com)

comment:14 Changed 6 years ago by carljm

  • Cc carl@… added

comment:15 Changed 6 years ago by DerCorny

  • Cc stefan.cornelius@… added

Being post-1.0 now, how is it going with the design descision? There are also related bugs, e.g. #6709 and #7167. The patches here actually look kind of good to me and I don't think that the SafeForms discussed in #7167 will help for highly dynamic AJAX, so why not extend the CSRF middleware with these patches and add a nice and simple template tag to generate the token?

While this might not be the best solution ever, I'm still convinced that a lot of people will benefit from a "standard" solution to at least mitigate the problem in an easy way - the patches don't mess up too much and I think it's way better than having users develop their own hackish and potentially insecure method of extending the CSRF protection. Think of all the snippets and apps out there that are currently lacking a standard way of tackling this problem (e.g. threaded-comments, iirc).

comment:16 Changed 5 years ago by lidaobing

  • Cc lidaobing@… added

comment:17 Changed 5 years ago by jojo

  • Cc johannes.beigel@… added

comment:18 Changed 5 years ago by neithere

  • Cc neithere@… added

comment:19 Changed 5 years ago by bthomas

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

This was fixed in [9554]

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.