Opened 6 years ago

Closed 6 years ago

Last modified 5 years ago

#15258 closed New feature (fixed)

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

Reported by: Brodie Rao 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 Rao 6 years ago.
Minimal (and untested) patch to add PUT/DELETE protection

Download all attachments as: .zip

Change History (8)

Changed 6 years ago by Brodie Rao

Attachment: csrf-put-delete.patch added

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

comment:1 Changed 6 years ago by Alex Gaynor

Has patch: set
Needs tests: set
Triage Stage: UnreviewedAccepted

comment:2 Changed 6 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 6 years ago by Luke Plant

milestone: 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 6 years ago by Tom Christie

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 6 years ago by Łukasz Rekucki

Severity: Normal
Type: New feature

comment:6 Changed 6 years ago by Luke Plant

Resolution: fixed
Status: newclosed

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 5 years ago by Jacob

milestone: 1.4

Milestone 1.4 deleted

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