Opened 6 years ago

Closed 6 years ago

Last modified 5 years ago

#12130 closed (fixed)

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

Reported by: carljm Owned by: lukeplant
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:

Description

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 carljm 6 years ago.
make get_token() create new token if needed

Download all attachments as: .zip

Change History (7)

comment:1 Changed 6 years ago by carljm

  • Has patch set
  • Needs documentation unset
  • Needs tests unset
  • Owner changed from nobody to carljm
  • 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 6 years ago by carljm

make get_token() create new token if needed

comment:2 Changed 6 years ago by lukeplant

  • Owner changed from carljm to lukeplant

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 6 years ago by lukeplant

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 6 years ago by lukeplant

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 6 years ago by lukeplant

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

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

Includes:

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

The documentation added here isn't displaying correctly.

http://docs.djangoproject.com/en/dev/ref/templates/builtins/#csrf-token

"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