Opened 11 years ago

Closed 11 years ago

#19436 closed Bug (fixed)

ensure_csrf_cookie decorator issues a "CSRF token missing or incorrect" warning.

Reported by: wrr@… 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)

19436_patch.diff (3.5 KB ) - added by Olivier Sels 11 years ago.

Download all attachments as: .zip

Change History (16)

comment:1 by Aymeric Augustin, 11 years ago

Resolution: invalid
Status: newclosed

This string exists only once in the entire codebase, in django/middleware/csrf.py:

REASON_BAD_TOKEN = "CSRF token missing or incorrect."

REASON_BAD_TOKEN is used only once, in CsrfViewMiddleware.process_view.

I'm not sure how you determined that "the application does not use CsrfViewMiddleware"; as far as I can tell, it does.

comment:2 by anonymous, 11 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 wrr@…, 11 years ago

Resolution: invalid
Status: closedreopened

comment:4 by Russell Keith-Magee, 11 years ago

Triage Stage: UnreviewedDesign 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 wrr@…, 11 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 Luke Plant, 11 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 Luke Plant, 11 years ago

Component: Uncategorizedcontrib.csrf
Triage Stage: Design decision neededAccepted

comment:8 by Aymeric Augustin, 11 years ago

Status: reopenednew

by Olivier Sels, 11 years ago

Attachment: 19436_patch.diff added

comment:9 by Olivier Sels, 11 years ago

Has patch: set
Owner: changed from nobody to Olivier Sels
Status: newassigned

Patch added. Does this need tests?

comment:10 by Aymeric Augustin, 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 Aymeric Augustin, 11 years ago

The patch changes the behavior of _EnsureCsrfToken; it no longer logs warnings. Is this change intended?

comment:12 by Aymeric Augustin, 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 Olivier Sels, 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 Aymeric Augustin, 11 years ago

Triage Stage: AcceptedReady for checkin

Very good, I'm going to merge this (after tweaking the test slightly).

comment:15 by Aymeric Augustin <aymeric.augustin@…>, 11 years ago

Resolution: fixed
Status: assignedclosed

In 63a9555d57069cee32de388821dbe580da1f97c0:

Fixed #19436 -- Don't log warnings in ensure_csrf_cookie.

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