Opened 12 years ago
Closed 11 years ago
#19436 closed Bug (fixed)
ensure_csrf_cookie decorator issues a "CSRF token missing or incorrect" warning.
Reported by: | Owned by: | Olivier Sels | |
---|---|---|---|
Component: | CSRF | Version: | 1.4 |
Severity: | Normal | Keywords: | csrf |
Cc: | Triage Stage: | Ready for checkin | |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
I'm using ensure_csrf_cookie decorator to set CSRF protection token with a POST request. The decorator works correctly but it prints incorrect and confusing warning:
WARNING django.request Forbidden (CSRF token missing or incorrect.): /auth/api/csrftoken/
The warning for sure comes from the decorator, because the application does not use CsrfViewMiddleware. I briefly examined django/views/decorators/csrf.py and django/middleware/csrf.py and it seems that indeed such warning is printed when post method is decorated.
Relevant part of the code that produces warning:
from django.core.context_processors import csrf from django.utils.decorators import method_decorator from django.views.decorators.csrf import ensure_csrf_cookie from django.views.generic import View class CsrfToken(View): """Establishes Cross Site Request Forgery protection token.""" @method_decorator(ensure_csrf_cookie) def post(self, request): """Returns CSRF protection token in a cookie and a response body.""" csrf_token = csrf(request).values()[0] return http.HttpResponseOKJson({'csrfToken': str(csrf_token)})
Attachments (1)
Change History (16)
comment:1 by , 12 years ago
Resolution: | → invalid |
---|---|
Status: | new → closed |
comment:2 by , 12 years ago
What I meant was that the warning for sure comes from ensure_csrf_cookie because the application does not use CsrfViewMiddleware directly. ensure_csrf_cookie internally uses CsrfViewMiddleware and the warning is indeed printed by this class, but this is ensure_csrf_cookie implementation detail.
comment:3 by , 12 years ago
Resolution: | invalid |
---|---|
Status: | closed → reopened |
comment:4 by , 12 years ago
Triage Stage: | Unreviewed → Design decision needed |
---|
I can validate that the report is accurate -- a POST request to a view protected by ensure_csrf_cookie raises a 403.
I suppose the bigger question is whether this is expected or not. I'm having difficulty with understanding why you would want to ensure that there is CSRF cookie, but not actually use it for a POST. The ensure_csrf_cookie decorator was introduced as a fix for #15354. The intention appears to be to ensure that the cookie has been set on a GET request, so that subsequent POST requests will have the cookie in place.
Marking as DDN so we can sort out what the right behavior is here.
comment:5 by , 12 years ago
Actually, if you don't enable CsrfViewMiddleware, the decorator does not raise 403, it prints 'WARNING django.request Forbidden (CSRF token missing or incorrect.)' but always accepts the request (as it should). This is because the decorator overwrites _reject():
class _EnsureCsrfCookie(CsrfViewMiddleware): def _reject(self, request, reason): return None
I have a single view that establishes CSRF token with a POST request and does nothing more. And several views that require and validate the token (for severals reasons I do not use CsrfViewMiddleware to do the validation). I'm using POST to establish the token as an extra precaution to make sure the token does not leak to a different origin site.
Anyway, I'm not convinced this requires design discussion. It seems like an obvious bug in the implementation. The printed warning is incorrect, request is not forbidden, 403 is not returned. The decorator, according to the documentation, should only set a cookie, not perform any validation. It seems like the warning is a result of a shortcut in the decorator implementation that reuses CsrfViewMiddleware but does not disable validation related warnings, that are not relevant for the decorator.
comment:6 by , 12 years ago
I agree with wrr - this is a clear (low priority) bug in the implementation. It also looks like the fix is trivial - move all the logger
calls into the _reject()
method. AFAICS from a brief review, this is straightforward and will reduce duplication and line count as well.
comment:7 by , 12 years ago
Component: | Uncategorized → contrib.csrf |
---|---|
Triage Stage: | Design decision needed → Accepted |
comment:8 by , 12 years ago
Status: | reopened → new |
---|
by , 11 years ago
Attachment: | 19436_patch.diff added |
---|
comment:9 by , 11 years ago
Has patch: | set |
---|---|
Owner: | changed from | to
Status: | new → assigned |
Patch added. Does this need tests?
comment:10 by , 11 years ago
As long as you haven't provided a convincing explanation of why it wouldn't need tests, it does :)
comment:11 by , 11 years ago
The patch changes the behavior of _EnsureCsrfToken; it no longer logs warnings. Is this change intended?
comment:12 by , 11 years ago
Oh -- that's actually the intended change. I got confused when you said it only shuffled things around on IRC.
I would like a test proving that ensure_csrf_cookie no longer logs warning messages. If for some reason such a test is impossible to write, let's talk again.
comment:13 by , 11 years ago
I added the test and opened a pull request at https://github.com/django/django/pull/1110.
It fails in current master and passes with the patch.
comment:14 by , 11 years ago
Triage Stage: | Accepted → Ready for checkin |
---|
Very good, I'm going to merge this (after tweaking the test slightly).
comment:15 by , 11 years ago
Resolution: | → fixed |
---|---|
Status: | assigned → closed |
This string exists only once in the entire codebase, in
django/middleware/csrf.py
:REASON_BAD_TOKEN
is used only once, inCsrfViewMiddleware.process_view
.I'm not sure how you determined that "the application does not use
CsrfViewMiddleware
"; as far as I can tell, it does.