Code

#19436 closed Bug (fixed)

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

Reported by: wrr@… Owned by: chass
Component: contrib.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 chass 11 months ago.

Download all attachments as: .zip

Change History (16)

comment:1 Changed 16 months ago by aaugustin

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Resolution set to invalid
  • Status changed from new to closed

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 Changed 16 months ago by anonymous

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 Changed 16 months ago by wrr@…

  • Resolution invalid deleted
  • Status changed from closed to reopened

comment:4 Changed 16 months ago by russellm

  • Triage Stage changed from Unreviewed to 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 Changed 16 months ago by wrr@…

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 Changed 16 months ago by lukeplant

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 Changed 16 months ago by lukeplant

  • Component changed from Uncategorized to contrib.csrf
  • Triage Stage changed from Design decision needed to Accepted

comment:8 Changed 13 months ago by aaugustin

  • Status changed from reopened to new

Changed 11 months ago by chass

comment:9 Changed 11 months ago by chass

  • Has patch set
  • Owner changed from nobody to chass
  • Status changed from new to assigned

Patch added. Does this need tests?

comment:10 Changed 11 months ago by aaugustin

As long as you haven't provided a convincing explanation of why it wouldn't need tests, it does :)

comment:11 Changed 11 months ago by aaugustin

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

comment:12 Changed 11 months ago by aaugustin

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 Changed 11 months ago by chass

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 Changed 11 months ago by aaugustin

  • Triage Stage changed from Accepted to Ready for checkin

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

comment:15 Changed 11 months ago by Aymeric Augustin <aymeric.augustin@…>

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

In 63a9555d57069cee32de388821dbe580da1f97c0:

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

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.