[Django] #32843: Some CSRF tests don't set request.method
Django
-------------------------------------+-------------------------------------
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 |
-------------------------------------+-------------------------------------
I noticed that for some CSRF tests, `request.method` is `None` when the
test thinks it should be `GET`. This causes
`CsrfViewMiddleware.process_view()` to take a different code path and
treat the request as unsafe because the method isn't in its allowed list:
https://github.com/django/django/blob/316cc34d046ad86e100227772294f906fae1c2e5/django/middleware/csrf.py#L338-L339
The cause for this is that neither implementation of
`_get_GET_csrf_cookie_request()` sets `req.method`. See
[https://github.com/django/django/blob/316cc34d046ad86e100227772294f906fae1c2e5/tests/csrf_tests/tests.py#L773-L779
here] and
[https://github.com/django/django/blob/316cc34d046ad86e100227772294f906fae1c2e5/tests/csrf_tests/tests.py#L971-L977
here].
The tests affected are:
*
`csrf_tests.tests.CsrfViewMiddlewareTests.test_get_token_for_requires_csrf_token_view`
*
`csrf_tests.tests.CsrfViewMiddlewareTests.test_token_node_with_csrf_cookie`
*
`csrf_tests.tests.CsrfViewMiddlewareUseSessionsTests.test_get_token_for_requires_csrf_token_view`
*
`csrf_tests.tests.CsrfViewMiddlewareUseSessionsTests.test_token_node_with_csrf_cookie`
Fortunately, the tests still pass when `request.method` is set to `GET`.
--
Ticket URL: <https://code.djangoproject.com/ticket/32843>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
-------------------------------------+-------------------------------------
Reporter: Chris Jerdonek | Owner: Chris
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
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
--
Ticket URL: <https://code.djangoproject.com/ticket/32843#comment:1>
Django
-------------------------------------+-------------------------------------
Reporter: Chris Jerdonek | Owner: Chris
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
-------------------------------------+-------------------------------------
* stage: Unreviewed => Accepted
Comment:
OK, nice. Good spot.
--
Ticket URL: <https://code.djangoproject.com/ticket/32843#comment:2>
Django
-------------------------------------+-------------------------------------
Reporter: Chris Jerdonek | Owner: Chris
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):
Before submitting a fix for this ticket, I will wait until after this PR
is merged: https://github.com/django/django/pull/14485
(There are a few more minor clean-ups / refactorings to the CSRF tests I'd
like to do in conjunction with this fix.)
--
Ticket URL: <https://code.djangoproject.com/ticket/32843#comment:3>
Django
-------------------------------------+-------------------------------------
Reporter: Chris Jerdonek | Owner: Chris
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:
PR: https://github.com/django/django/pull/14565
--
Ticket URL: <https://code.djangoproject.com/ticket/32843#comment:4>
Django
-------------------------------------+-------------------------------------
Reporter: Chris Jerdonek | Owner: Chris
Type: | Jerdonek
Component: CSRF | Version: dev
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"594d6e9407fd1596c2033e9d3db5b77c86f033a2" 594d6e94]:
{{{
#!CommitTicketReference repository=""
revision="594d6e9407fd1596c2033e9d3db5b77c86f033a2"
Refs #32843 -- Added
CsrfViewMiddlewareTestMixin._get_csrf_cookie_request() hook.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/32843#comment:8>
Django
-------------------------------------+-------------------------------------
Reporter: Chris Jerdonek | Owner: Chris
Type: | Jerdonek
Cleanup/optimization | Status: closed
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"c8439d1dbabca490fbcc127ccb351b87e9cdc131" c8439d1]:
{{{
#!CommitTicketReference repository=""
revision="c8439d1dbabca490fbcc127ccb351b87e9cdc131"
Refs #32843 -- Added method/cookie arguments to
CsrfViewMiddlewareTestMixin._get_request().
This also removes unnecessary test hooks.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/32843#comment:7>
Django
-------------------------------------+-------------------------------------
Reporter: Chris Jerdonek | Owner: Chris
Type: | Jerdonek
Cleanup/optimization | Status: closed
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"6bccb643476a880d35a6af83aefd903f0a803eff" 6bccb643]:
{{{
#!CommitTicketReference repository=""
revision="6bccb643476a880d35a6af83aefd903f0a803eff"
Refs #32843 -- Moved _get_GET_csrf_cookie_request() to
CsrfViewMiddlewareTestMixin.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/32843#comment:6>
Django
-------------------------------------+-------------------------------------
Reporter: Chris Jerdonek | Owner: Chris
Type: | Jerdonek
Cleanup/optimization | Status: closed
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"4397d2bd6be1e0eb264b3807f8ae8c0d3cbc116e" 4397d2bd]:
{{{
#!CommitTicketReference repository=""
revision="4397d2bd6be1e0eb264b3807f8ae8c0d3cbc116e"
Fixed #32843 -- Ensured the CSRF tests' _get_GET_csrf_cookie_request()
sets the request method.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/32843#comment:5>