Code

Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#15258 closed New feature (fixed)

Ajax CSRF protection doesn't apply to PUT or DELETE requests

Reported by: brodie Owned by: nobody
Component: Core (Other) Version: 1.2
Severity: Normal Keywords: csrf ajax
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: yes Patch needs improvement: no
Easy pickings: UI/UX:

Description

The CSRFViewMiddleware only does CSRF checks for POST requests. It's not uncommon to do PUT and DELETE requests from Ajax. Now that the middleware also checks Ajax requests, we should probably check those request methods as well.

One tricky thing is extracting form data for PUT and DELETE requests. We don't populate request.POST for those methods, so we would either have to add something to get them out of raw_post_data, or require X-CSRFToken to be set for PUT/DELETE.

Attachments (1)

csrf-put-delete.patch (1.4 KB) - added by brodie 3 years ago.
Minimal (and untested) patch to add PUT/DELETE protection

Download all attachments as: .zip

Change History (8)

Changed 3 years ago by brodie

Minimal (and untested) patch to add PUT/DELETE protection

comment:1 Changed 3 years ago by Alex

  • Has patch set
  • Needs documentation unset
  • Needs tests set
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted

comment:2 Changed 3 years ago by tow21

To respond to the "tricky thing" in the original report: DELETE has no body, so there's no way to pass the token in the body anyway. And since PUT may contain essentially arbitrary data that need not be form-encoded, I think it's reasonable to require use of X-CSRFToken instead.

Additionally, I think the patch perhaps ought to be changed so that instead of:

-     if request.method == 'POST':
+     if request.method in ('POST', 'PUT', 'DELETE'): 

we do:

-     if request.method == 'POST': 
+     if request.method not in ('GET', 'HEAD', 'OPTIONS'):

Rationale: if anyone is experimenting with methods beyond those in HTTP itself (for example, writing handlers for WebDAV methods) then we have no guarantees on the idempotency of anything other than GET, HEAD, and OPTIONS.

comment:3 Changed 3 years ago by lukeplant

  • milestone set to 1.4

I agree with both points made by tow21.

Given that the current CSRF protection works as advertised, we have to regard this as a feature request, not a bug, so it will have to go into 1.4 now. Also, given the fact the protection will be extended to methods besides POST, it will need a 'backwards compatibility' note in the release notes for 1.4.

Other than these things, I don't think much more needs to be done on this.

comment:4 Changed 3 years ago by tomchristie

Presumably we also ought to add TRACE to the list of safe methods, (As per http://www.w3.org/Protocols/rfc2616/rfc2616-sec9.html "the methods OPTIONS and TRACE SHOULD NOT have side effects").

I'd guess that in practice it'd almost always be dealt with at EG the apache level or whatever, but there might be some cases where it gets dealt with by Django middleware further down the stack.

comment:5 Changed 3 years ago by lrekucki

  • Severity set to Normal
  • Type set to New feature

comment:6 Changed 3 years ago by lukeplant

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

In [16201]:

Fixed #15258 - Ajax CSRF protection doesn't apply to PUT or DELETE requests

Thanks to brodie for the report, and further input from tow21

This is a potentially backwards incompatible change - if you were doing
PUT/DELETE requests and relying on the lack of protection, you will need to
update your code, as noted in the releaste notes.

comment:7 Changed 3 years ago by jacob

  • milestone 1.4 deleted

Milestone 1.4 deleted

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.