[Django] #32578: Handle request.get_host() raising DisallowedHost in CsrfViewMiddleware._origin_verified()
Django
CsrfViewMiddleware._origin_verified()
-------------------------------------+-------------------------------------
Reporter: Chris | Owner: nobody
Jerdonek |
Type: | Status: new
Uncategorized |
Component: CSRF | Version: dev
Severity: Normal | Keywords:
Triage Stage: | CsrfViewMiddleware,DisallowedHost
Unreviewed | Has patch: 0
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------+-------------------------------------
Currently, on this line, `CsrfViewMiddleware._origin_verified()` doesn't
handle `request.get_host()` raising `DisallowedHost`:
https://github.com/django/django/blob/41e6b2a3c5e723256506b9ff49437d52a1f3bf43/django/middleware/csrf.py#L229-L231
Since Django was previously fixed to handle `request.get_host()` raising
`DisallowedHost` elsewhere in `CsrfViewMiddleware.process_view()` (see
ticket #28693), it seems like it should be handled here, too.
--
Ticket URL: <https://code.djangoproject.com/ticket/32578>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
CsrfViewMiddleware._origin_verified()
-------------------------------------+-------------------------------------
Type: Uncategorized | Status: new
Component: CSRF | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage:
CsrfViewMiddleware,DisallowedHost | Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Tim Graham):
It might be better to perform host validation elsewhere in Django as
suggested in #27575 so that `DisallowedHost` doesn't need to be caught
everywhere.
--
Ticket URL: <https://code.djangoproject.com/ticket/32578#comment:1>
Django
CsrfViewMiddleware._origin_verified()
-------------------------------------+-------------------------------------
Type: Uncategorized | Status: new
Severity: Normal | Resolution:
Keywords: | Triage Stage:
CsrfViewMiddleware,DisallowedHost | Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Chris Jerdonek):
Another option would be for `get_host()` to accept an argument that causes
it to return e.g. `None` on a disallowed host instead of raising
`DisallowedHost`. That would make people more aware of that possibility
and give callers another option aside from a try-except for handling that
case.
--
Ticket URL: <https://code.djangoproject.com/ticket/32578#comment:2>
Django
CsrfViewMiddleware._origin_verified()
-------------------------------------+-------------------------------------
Type: Uncategorized | Status: new
Severity: Normal | Resolution:
Keywords: | Triage Stage:
CsrfViewMiddleware,DisallowedHost | Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* Attachment "32578.diff" added.
Regression test.
Django
CsrfViewMiddleware._origin_verified()
-------------------------------------+-------------------------------------
Type: Bug | Status: new
Component: CSRF | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
CsrfViewMiddleware,DisallowedHost |
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
-------------------------------------+-------------------------------------
Changes (by Mariusz Felisiak):
* type: Uncategorized => Bug
* easy: 0 => 1
* stage: Unreviewed => Accepted
Comment:
Thanks for the report. I attached a test.
--
Ticket URL: <https://code.djangoproject.com/ticket/32578#comment:3>
Django
CsrfViewMiddleware._origin_verified()
-------------------------------------+-------------------------------------
Type: Bug | Status: new
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
CsrfViewMiddleware,DisallowedHost |
Needs tests: 0 | Patch needs improvement: 0
-------------------------------------+-------------------------------------
Comment (by Chris Jerdonek):
Thanks, Mariusz. However, do you know for sure that's testing the right
code path? `_origin_verified()` only gets called when `if request.method
not in ('GET', 'HEAD', 'OPTIONS', 'TRACE')`, but the test appears to be
`GET`. I could be wrong though since my observation is based on inspection
rather than running the test.
--
Ticket URL: <https://code.djangoproject.com/ticket/32578#comment:4>
Django
CsrfViewMiddleware._origin_verified()
-------------------------------------+-------------------------------------
Type: Bug | Status: new
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
CsrfViewMiddleware,DisallowedHost |
Needs tests: 0 | Patch needs improvement: 0
-------------------------------------+-------------------------------------
Changes (by Mariusz Felisiak):
* Attachment "32578.diff" added.
Regression test.
--
Django
CsrfViewMiddleware._origin_verified()
-------------------------------------+-------------------------------------
Type: Bug | Status: new
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
CsrfViewMiddleware,DisallowedHost |
Needs tests: 0 | Patch needs improvement: 0
-------------------------------------+-------------------------------------
Comment (by Mariusz Felisiak):
Replying to [comment:4 Chris Jerdonek]:
> Thanks, Mariusz. However, do you know for sure that's testing the right
code path? `_origin_verified()` only gets called when `if request.method
not in ('GET', 'HEAD', 'OPTIONS', 'TRACE')`, but the test appears to be
`GET`. I could be wrong though since my observation is based on inspection
rather than running the test.
Test crashes because `method` is not set in this case. I updated attached
test to use the `POST` method.
----
Replying to [comment:2 Tim Graham]:
> It might be better to perform host validation elsewhere in Django as
suggested in #27575 so that DisallowedHost doesn't need to be caught
everywhere.
This can be tricky, so I'd fix this case independently and discuss the
options in #27575.
--
Ticket URL: <https://code.djangoproject.com/ticket/32578#comment:5>
Django
CsrfViewMiddleware._origin_verified()
-------------------------------------+-------------------------------------
Type: Bug | Status: new
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
CsrfViewMiddleware,DisallowedHost |
Needs tests: 0 | Patch needs improvement: 0
-------------------------------------+-------------------------------------
Comment (by Chris Jerdonek):
> Test crashes because method is not set in this case. I updated attached
test to use the POST method.
It might be good to fix `_get_GET_no_csrf_cookie_request()` so that the
method is indeed set to `GET`.
--
Ticket URL: <https://code.djangoproject.com/ticket/32578#comment:6>
Django
CsrfViewMiddleware._origin_verified()
-------------------------------------+-------------------------------------
Type: Bug | Status: new
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
CsrfViewMiddleware,DisallowedHost |
Needs tests: 0 | Patch needs improvement: 0
-------------------------------------+-------------------------------------
Comment (by Mariusz Felisiak):
Replying to [comment:6 Chris Jerdonek]:
> It might be good to fix `_get_GET_no_csrf_cookie_request()` so that the
method is indeed set to `GET`.
Agreed, [https://github.com/django/django/pull/14166 PR].
--
Ticket URL: <https://code.djangoproject.com/ticket/32578#comment:7>
Django
CsrfViewMiddleware._origin_verified()
-------------------------------------+-------------------------------------
| Jerdonek
Type: Bug | Status: assigned
Component: CSRF | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
CsrfViewMiddleware,DisallowedHost |
Needs tests: 0 | Patch needs improvement: 0
-------------------------------------+-------------------------------------
* owner: nobody => Chris Jerdonek
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/32578#comment:8>
Django
CsrfViewMiddleware._origin_verified()
-------------------------------------+-------------------------------------
| Jerdonek
Type: Bug | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
CsrfViewMiddleware,DisallowedHost |
Needs tests: 0 | Patch needs improvement: 0
-------------------------------------+-------------------------------------
Changes (by Chris Jerdonek):
* has_patch: 0 => 1
Comment:
PR: https://github.com/django/django/pull/14179
--
Ticket URL: <https://code.djangoproject.com/ticket/32578#comment:9>
Django
CsrfViewMiddleware._origin_verified()
-------------------------------------+-------------------------------------
| Jerdonek
Component: CSRF | Version: dev
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
CsrfViewMiddleware,DisallowedHost |
Needs tests: 0 | Patch needs improvement: 0
-------------------------------------+-------------------------------------
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"ff514309e178e3955012050ead9b8fc66dc21a5b" ff514309]:
{{{
#!CommitTicketReference repository=""
revision="ff514309e178e3955012050ead9b8fc66dc21a5b"
Fixed #32578 -- Fixed crash in CsrfViewMiddleware when a request with
Origin header has an invalid host.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/32578#comment:10>