[Django] #32902: CsrfViewMiddleware.process_response()'s csrf_cookie_needs_reset and csrf_cookie_set logic isn't right
Django
csrf_cookie_set logic isn't right
------------------------------------------+--------------------------------
Reporter: Chris Jerdonek | Owner: Chris Jerdonek
Type: Bug | Status: assigned
Component: CSRF | Version: dev
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
------------------------------------------+--------------------------------
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 `secu...@djangoproject.com` know about this issue, and
they said it was okay to resolve this publicly.)
--
Ticket URL: <https://code.djangoproject.com/ticket/32902>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
csrf_cookie_set logic isn't right
Reporter: Chris Jerdonek | Owner: Chris Jerdonek
Type: Bug | Status: assigned
Component: CSRF | Version: dev
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by 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
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/32902#comment:1>
Django
csrf_cookie_set logic isn't right
Reporter: Chris Jerdonek | Owner: Chris Jerdonek
Type: Bug | Status: assigned
Component: CSRF | Version: dev
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by 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).
--
Ticket URL: <https://code.djangoproject.com/ticket/32902#comment:2>
Django
csrf_cookie_set logic isn't right
Reporter: Chris Jerdonek | Owner: Chris Jerdonek
Type: Bug | Status: assigned
Component: CSRF | Version: dev
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Mariusz Felisiak):
* cc: Shai Berger, Florian Apolloner (added)
* stage: Unreviewed => Accepted
--
Ticket URL: <https://code.djangoproject.com/ticket/32902#comment:3>
Django
csrf_cookie_set logic isn't right
Reporter: Chris Jerdonek | Owner: Chris Jerdonek
Type: Bug | Status: assigned
Component: CSRF | Version: dev
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Chris Jerdonek):
* has_patch: 0 => 1
Comment:
PR: https://github.com/django/django/pull/14599
--
Ticket URL: <https://code.djangoproject.com/ticket/32902#comment:4>
Django
csrf_cookie_set logic isn't right
Reporter: Chris Jerdonek | Owner: Chris
| Jerdonek
Type: Bug | Status: assigned
Component: CSRF | Version: dev
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Shai Berger):
* stage: Accepted => Ready for checkin
Comment:
Would appreciate at least one more pair of eyes on this, but LGTM.
--
Ticket URL: <https://code.djangoproject.com/ticket/32902#comment:5>
Django
csrf_cookie_set logic isn't right
Reporter: Chris Jerdonek | Owner: Chris Jerdonek
Type: Bug | Status: assigned
Component: CSRF | Version: dev
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Mariusz Felisiak):
* stage: Ready for checkin => Accepted
--
Ticket URL: <https://code.djangoproject.com/ticket/32902#comment:6>
Django
csrf_cookie_set logic isn't right
Reporter: Chris Jerdonek | Owner: Chris
| Jerdonek
Type: Bug | Status: assigned
Component: CSRF | Version: dev
Keywords: | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Mariusz Felisiak):
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/32902#comment:7>
Django
csrf_cookie_set logic isn't right
Reporter: Chris Jerdonek | Owner: Chris
| Jerdonek
Component: CSRF | Version: dev
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"3cfcb8cbc83cfe58307b87435d55c27180cb4f94" 3cfcb8cb]:
{{{
#!CommitTicketReference repository=""
revision="3cfcb8cbc83cfe58307b87435d55c27180cb4f94"
Refs #32902 -- Moved ensure_csrf_cookie_view after protected_view.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/32902#comment:10>
Django
csrf_cookie_set logic isn't right
Reporter: Chris Jerdonek | Owner: Chris
| Jerdonek
Component: CSRF | Version: dev
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Mariusz Felisiak <felisiak.mariusz@…>):
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"a2e1f1e2954c8144f2acf83402ed1f55ea9692a4" a2e1f1e2]:
{{{
#!CommitTicketReference repository=""
revision="a2e1f1e2954c8144f2acf83402ed1f55ea9692a4"
Fixed #32902 -- Fixed CsrfViewMiddleware.process_response()'s cookie reset
logic.
Thanks Florian Apolloner and Shai Berger for reviews.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/32902#comment:9>
Django
csrf_cookie_set logic isn't right
Reporter: Chris Jerdonek | Owner: Chris
| Jerdonek
Type: Bug | Status: assigned
Component: CSRF | Version: dev
Keywords: | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"311401d9a278339795a7ab3f1617c4da9077fdcc" 311401d9]:
{{{
#!CommitTicketReference repository=""
revision="311401d9a278339795a7ab3f1617c4da9077fdcc"
Refs #32902 -- Added CSRF test when rotate_token() is called between
resetting the token and processing response.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/32902#comment:8>