CsrfViewMiddleware.process_response()'s csrf_cookie_needs_reset and csrf_cookie_set logic isn't right

[Chris Jerdonek]

I noticed that the csrf_cookie_needs_reset and csrf_cookie_set logic inside CsrfViewMiddleware.process_response() isn't right: ​https://github.com/django/django/blob/fa35c8bdbc6aca65d94d6280fa463d5bc7baa5c0/django/middleware/csrf.py#L439-L451

Consequently--

1. self._set_token(request, response) can get called twice in some circumstances, even if response.csrf_cookie_set is true at the beginning, and
2. the cookie can fail to be reset in some circumstances, even if csrf_cookie_needs_reset is true at the beginning.

(I previously let security@djangoproject.com know about this issue, and they said it was okay to resolve this publicly.)

[Chris Jerdonek]

I believe the fix should look something like this:

diff --git a/django/middleware/csrf.py b/django/middleware/csrf.py
index c2a9470ab1..bf97e50146 100644
--- a/django/middleware/csrf.py
+++ b/django/middleware/csrf.py
@@ -437,15 +437,14 @@ class CsrfViewMiddleware(MiddlewareMixin):
         return self._accept(request)
 
     def process_response(self, request, response):
-        if not getattr(request, 'csrf_cookie_needs_reset', False):
-            if getattr(response, 'csrf_cookie_set', False):
-                return response
-
-        if not request.META.get("CSRF_COOKIE_USED", False):
+        # Prevent the cookie from being set twice.
+        if getattr(response, 'csrf_cookie_set', False):
             return response
+        if (getattr(request, 'csrf_cookie_needs_reset', False) or
+                request.META.get("CSRF_COOKIE_USED")):
+            # Set the CSRF cookie even if it's already set so that the
+            # expiry timer gets renewed.
+            self._set_token(request, response)
+            response.csrf_cookie_set = True
 
-        # Set the CSRF cookie even if it's already set, so we renew
-        # the expiry timer.
-        self._set_token(request, response)
-        response.csrf_cookie_set = True
         return response

[Chris Jerdonek]

The code under question was originally added 5 years in this large-ish commit here: ​https://github.com/django/django/commit/5112e65ef2df1dbb95ff83026b6a962fb2688661

Indeed, part of my proposed fix involves restoring the initial csrf_cookie_set lines prior to that commit (csrf_cookie_set was called csrf_processing_done before this commit).

[Chris Jerdonek]

PR: ​https://github.com/django/django/pull/14599

[Shai Berger]

Would appreciate at least one more pair of eyes on this, but LGTM.

[Mariusz Felisiak <felisiak.mariusz@…>]

In 311401d9:

Refs #32902 -- Added CSRF test when rotate_token() is called between resetting the token and processing response.

[Mariusz Felisiak <felisiak.mariusz@…>]

In a2e1f1e2:

Fixed #32902 -- Fixed CsrfViewMiddleware.process_response()'s cookie reset logic.

Thanks Florian Apolloner and Shai Berger for reviews.

[Mariusz Felisiak <felisiak.mariusz@…>]

In 3cfcb8cb:

Refs #32902 -- Moved ensure_csrf_cookie_view after protected_view.