#32902 closed Bug (fixed)
CsrfViewMiddleware.process_response()'s csrf_cookie_needs_reset and csrf_cookie_set logic isn't right
Reported by: | Chris Jerdonek | Owned by: | Chris Jerdonek |
---|---|---|---|
Component: | CSRF | Version: | dev |
Severity: | Normal | Keywords: | |
Cc: | Shai Berger, Florian Apolloner | 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 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--
self._set_token(request, response)
can get called twice in some circumstances, even ifresponse.csrf_cookie_set
is true at the beginning, and- 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.)
Change History (10)
comment:2 by , 3 years ago
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).
comment:3 by , 3 years ago
Cc: | added |
---|---|
Triage Stage: | Unreviewed → Accepted |
comment:5 by , 3 years ago
Triage Stage: | Accepted → Ready for checkin |
---|
Would appreciate at least one more pair of eyes on this, but LGTM.
comment:6 by , 3 years ago
Triage Stage: | Ready for checkin → Accepted |
---|
comment:7 by , 3 years ago
Triage Stage: | Accepted → Ready for checkin |
---|
I believe the fix should look something like this: