Opened 7 years ago

Closed 7 years ago

Last modified 7 years ago

#12130 closed (fixed)

CSRF code requires non-POST-accepting views to be protected

Reported by: Carl Meyer Owned by: Luke Plant
Component: Core (Other) Version: master
Severity: Keywords:
Cc: Triage Stage: Unreviewed
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:


There's a very confusing failure mode for the new CSRF protection when not using the view middleware (i.e. when upgrading a project that didn't use CSRF protection before), and using contrib.comments (or any other code where a form is posted from one view to a different one).

The CSRF context processor sets the csrf_token to NOTPROVIDED if the _current_ view is not protected by either the view middleware or the decorator. But it's quite possible (even likely when using contrib.comments) that the form-rendering view is a GET-only view that doesn't need to be protected, but its form POSTs to a view that is protected (with the decorator).

To reproduce:

  1. Create a project using Django trunk. Leave CsrfViewMiddleware out of MIDDLEWARE_CLASSES.
  1. Add a simple object_detail view that calls contrib.comments' {% render_comment_form %} tag.
  1. Load up that view and submit a comment. You'll get the CSRF 403 Forbidden.

Somehow a valid CSRF token needs to be made available to templates rendered by any view, regardless of whether that view is itself protected against CSRF.

Attachments (1)

12130_r11706.diff (2.7 KB) - added by Carl Meyer 7 years ago.
make get_token() create new token if needed

Download all attachments as: .zip

Change History (7)

comment:1 Changed 7 years ago by Carl Meyer

Has patch: set
Needs documentation: unset
Needs tests: unset
Owner: changed from nobody to Carl Meyer
Patch needs improvement: unset

I think the answer here is that get_token needs to be more robust, and create a new token if it doesn't find one (rather than relying on the view middleware / decorator to create new tokens for it). If {% csrf_token %} is used in template, that means we really honestly do need a token. The whole 'NOTPROVIDED' special case simply needs to go away; there are no circumstances under which a call to {% csrf_token %} should not return a usable token. At least that's my take on it. Patch with tests attached.

Changed 7 years ago by Carl Meyer

Attachment: 12130_r11706.diff added

make get_token() create new token if needed

comment:2 Changed 7 years ago by Luke Plant

Owner: changed from Carl Meyer to Luke Plant

Very good catch, thank you. This hole in the logic appeared when we switched the proposal from having CsrfViewMiddleware as a requirement for contrib views to using a decorator for those views, and your analysis of the new situation is exactly right.

Thanks for time spent debugging, I'll apply shortly.

comment:3 Changed 7 years ago by Luke Plant

Ah, there is a problem. If the user does not yet have a CSRF cookie, and visits a page that does not have @csrf_protect applied, then with your solution, they will get a token but not a cookie, since it is the middleware/decorator that actually sends a cookie when the token is used.

We could add something like a @uses_csrf_token decorator, but it's actually much simpler to require use of the @csrf_protect decorator on views that need to use the token. That needs documenting, and adding to the contrib comment views.

comment:4 Changed 7 years ago by Luke Plant

An alternative solution is to call the code that adds the cookie from core functionality, so we can guarantee it always happens. For now, I'm just going to fix this via careful documentation, though we may want to revisit.

comment:5 Changed 7 years ago by Luke Plant

Resolution: fixed
Status: newclosed

(In [11711]) Fixed #12130 - documented need for csrf_protect on views that don't accept POST


  • proper documentation for csrf_protect
  • notes in comments app.
  • specific upgrade notes for comments app

Thanks to carljm for report and debugging.

comment:6 Changed 7 years ago by Dougal Matthews

The documentation added here isn't displaying correctly.

"System Message: WARNING/2 (/home/djangodocs/en/dev/ref/templates/builtins.txt)

undefined label: releases-1.1.2"

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