[Django] #32916: CsrfViewMiddleware's request.META["CSRF_COOKIE_USED"] and request.csrf_cookie_needs_reset can be combined
Django
request.csrf_cookie_needs_reset can be combined
-------------------------------------+-------------------------------------
Reporter: Chris | Owner: Chris Jerdonek
Jerdonek |
Type: | Status: assigned
Cleanup/optimization |
Component: CSRF | Version: dev
Severity: Normal | Keywords:
Triage Stage: | Has patch: 0
Unreviewed |
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------+-------------------------------------
In `CsrfViewMiddleware`, `request.META["CSRF_COOKIE_USED"]` and
`request.csrf_cookie_needs_reset` are both used for the same purpose.
Namely, they are inspected inside `CsrfViewMiddleware.process_response()`
to determine whether a cookie should be sent (though the logic in the
method is currently buggy):
https://github.com/django/django/blob/6f60fa97b0b501ef7cc77e16392654bf27ec8db3/django/middleware/csrf.py#L440-L445
Combining these two things would simplify `CsrfViewMiddleware`. This could
be done after #32902, which fixes the bugginess mentioned above.
My suggestion would be to replace both of these with a single
`request.META` key of `request.META["CSRF_COOKIE_NEEDS_RESET"]`. The
reason is that the `request.META` dict is more visible and easier to debug
than a custom request attribute, and it pairs more nicely with
`request.META["CSRF_COOKIE"]`. Also, using the current key of
`"CSRF_COOKIE_USED"` would be misleading because there are cases where the
cookie is queued for reset even if it hasn't been used in the request.
--
Ticket URL: <https://code.djangoproject.com/ticket/32916>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
request.csrf_cookie_needs_reset can be combined
-------------------------------------+-------------------------------------
Type: | Jerdonek
Cleanup/optimization | Status: assigned
Component: CSRF | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
-------------------------------------+-------------------------------------
Changes (by Chris Jerdonek):
* easy: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/32916#comment:1>
Django
request.csrf_cookie_needs_reset can be combined
-------------------------------------+-------------------------------------
Type: | Jerdonek
Cleanup/optimization | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Needs tests: 0 | Patch needs improvement: 0
-------------------------------------+-------------------------------------
Comment (by Chris Jerdonek):
As additional background and for future reference,
`request.csrf_processing_done` is the only other custom request attribute
used by `CsrfViewMiddleware`:
https://github.com/django/django/blob/6f60fa97b0b501ef7cc77e16392654bf27ec8db3/django/middleware/csrf.py#L191
--
Ticket URL: <https://code.djangoproject.com/ticket/32916#comment:2>
Django
request.csrf_cookie_needs_reset can be combined
-------------------------------------+-------------------------------------
Type: | Jerdonek
Cleanup/optimization | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* cc: Shai Berger, Florian Apolloner (added)
* easy: 1 => 0
* stage: Unreviewed => Accepted
Comment:
Tentatively accepted. `csrf_cookie_needs_reset` is undocumented API but I
found projects that use it, so we should at least add a short release
note.
Florian, Shai, can you take a look? Your opinions would be greatly
appreciated. I've checked an extensive discussion in #20869 and
[https://github.com/django/django/pull/5605 PR] but couldn't find any
rationale for this duality.
--
Ticket URL: <https://code.djangoproject.com/ticket/32916#comment:3>
Django
request.csrf_cookie_needs_reset can be combined
-------------------------------------+-------------------------------------
Type: | Jerdonek
Cleanup/optimization | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Florian Apolloner):
I'd love to get rid of one of those. I tried in the past and failed :D I
do not have strong feelings on the request attr vs the `META` dict.
Somehow `META` feels wrong, what is `META` supposed to be used for? http-
headers and wsgi attributes, or should we also stuff our things in there?
--
Ticket URL: <https://code.djangoproject.com/ticket/32916#comment:4>
Django
request.csrf_cookie_needs_reset can be combined
-------------------------------------+-------------------------------------
Type: | Jerdonek
Cleanup/optimization | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Chris Jerdonek):
> I do not have strong feelings on the request attr vs the META dict.
Somehow META feels wrong, what is META supposed to be used for?
Thanks, Florian. One advantage `META` has over a custom attribute is that
its contents automatically get displayed in the debug view under "Request
information" (which isn't currently true for custom attributes). So
developers are able to see a snapshot of the CSRF "state" more easily when
an error occurs. This is what I was referring to in part when I said above
it's "more visible and easier to debug." Also, as long as we're using
`META` to store `CSRF_COOKIE`, I think it makes sense to store the related
values alongside. If we're considering moving `CSRF_COOKIE` out of there
at some point, it's another story. Finally, while the debug view and tools
could be updated to display certain custom attributes, it would create
more friction when changing the attributes because the tools would also
need to be updated.
--
Ticket URL: <https://code.djangoproject.com/ticket/32916#comment:5>
Django
request.csrf_cookie_needs_reset can be combined
-------------------------------------+-------------------------------------
Type: | Jerdonek
Cleanup/optimization | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Chris Jerdonek):
So people coming across this ticket know, I'm waiting for ticket #32902 to
be resolved before submitting a PR for this ticket. One reason is that my
PR for that ticket adds some extra test cases, so it will be good to have
those test cases in place before making the change considered here.
--
Ticket URL: <https://code.djangoproject.com/ticket/32916#comment:6>
Django
request.csrf_cookie_needs_reset can be combined
-------------------------------------+-------------------------------------
Type: | Jerdonek
Cleanup/optimization | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* has_patch: 0 => 1
Comment:
https://github.com/django/django/pull/14688
--
Ticket URL: <https://code.djangoproject.com/ticket/32916#comment:7>
Django
request.csrf_cookie_needs_reset can be combined
-------------------------------------+-------------------------------------
Type: | Jerdonek
Cleanup/optimization | Status: assigned
Severity: Normal | Resolution:
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
-------------------------------------+-------------------------------------
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/32916#comment:8>
Django
request.csrf_cookie_needs_reset can be combined
-------------------------------------+-------------------------------------
Type: | 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:"7c30bdbdb16204970cc320f4345b8a439d8f65b4" 7c30bdb]:
{{{
#!CommitTicketReference repository=""
revision="7c30bdbdb16204970cc320f4345b8a439d8f65b4"
Refs #32916 -- Replaced request.csrf_cookie_needs_reset with
request.META['CSRF_COOKIE_NEEDS_UPDATE'].
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/32916#comment:10>
Django
request.csrf_cookie_needs_reset can be combined
-------------------------------------+-------------------------------------
Type: | Jerdonek
Cleanup/optimization | Status: closed
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"6ebf931de8926d88db6d2684cb07d1bbebb919a5" 6ebf931d]:
{{{
#!CommitTicketReference repository=""
revision="6ebf931de8926d88db6d2684cb07d1bbebb919a5"
Fixed #32916 -- Combined request.META['CSRF_COOKIE_USED'] and
request.csrf_cookie_needs_reset.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/32916#comment:9>