#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: | dev |
Severity: | Keywords: | ||
Cc: | Triage Stage: | Unreviewed | |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
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:
- Create a project using Django trunk. Leave CsrfViewMiddleware out of MIDDLEWARE_CLASSES.
- Add a simple object_detail view that calls contrib.comments' {% render_comment_form %} tag.
- 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)
Change History (7)
comment:1 by , 15 years ago
Has patch: | set |
---|---|
Owner: | changed from | to
comment:2 by , 15 years ago
Owner: | changed from | to
---|
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 by , 15 years ago
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 by , 15 years ago
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 by , 15 years ago
Resolution: | → fixed |
---|---|
Status: | new → closed |
comment:6 by , 15 years ago
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"
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.