#32578 closed Bug (fixed)

Handle request.get_host() raising DisallowedHost in CsrfViewMiddleware._origin_verified()

Reported by: Chris Jerdonek Owned by: Chris Jerdonek
Component: CSRF Version: dev
Severity: Normal Keywords: CsrfViewMiddleware, DisallowedHost
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: yes UI/UX: no

Description

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.

Attachments (1)

32578.diff (877 bytes) - added by Mariusz Felisiak 14 months ago.
Regression test.

Download all attachments as: .zip

Change History (11)

comment:1 Changed 14 months ago 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.

comment:2 Changed 14 months ago 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.

comment:3 Changed 14 months ago by Mariusz Felisiak

Easy pickings: set
Triage Stage: UnreviewedAccepted
Type: UncategorizedBug

Thanks for the report. I attached a test.

comment:4 Changed 14 months ago 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.

Changed 14 months ago by Mariusz Felisiak

Attachment: 32578.diff added

Regression test.

comment:5 in reply to:  4 Changed 14 months ago by Mariusz Felisiak

Replying to 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 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.

comment:6 Changed 14 months ago 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.

comment:7 in reply to:  6 Changed 14 months ago by Mariusz Felisiak

Replying to Chris Jerdonek:

It might be good to fix _get_GET_no_csrf_cookie_request() so that the method is indeed set to GET.

Agreed, PR.

comment:8 Changed 14 months ago by Chris Jerdonek

Owner: changed from nobody to Chris Jerdonek
Status: newassigned

comment:9 Changed 14 months ago by Chris Jerdonek

Has patch: set

comment:10 Changed 14 months ago by Mariusz Felisiak <felisiak.mariusz@…>

Resolution: fixed
Status: assignedclosed

In ff514309:

Fixed #32578 -- Fixed crash in CsrfViewMiddleware when a request with Origin header has an invalid host.

Note: See TracTickets for help on using tickets.
Back to Top