[Django] #28693: DisallowedHost can cause uncaught exception and HTTP 500

[Django]

#28693: DisallowedHost can cause uncaught exception and HTTP 500
-----------------------------------------+--------------------------------
Reporter: Greg Price | Owner: nobody
Type: Bug | Status: new
Component: Uncategorized | Version: 1.11
Severity: Normal | Keywords: DisallowedHost
Triage Stage: Unreviewed | Has patch: 0
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-----------------------------------------+--------------------------------
If the client sends an HTTP "Host:" header which isn't allowed, we try to
return a 400 response while logging a DisallowedHost exception. For a
brief time after `SuspiciousOperation` was first introduced, this caused a
500, but that was fixed in #19866 back in 2013.

But now I've been getting exceptions in production recently that are
caused by DisallowedHost. It turns out that there is a case in the
exception-handling path on the way to returning a 400 response that itself
tries to call `request.get_host()`, and so raises a new DisallowedHost
exception. Here's the test I added to my application's test suite, to test
my workaround:
{{{
class ErrorPageTest(TestCase):
def test_bogus_http_host(self):
# This tests that we've successfully worked around a certain bug
in
# Django's exception handling. The enforce_csrf_checks=True,
# secure=True, and HTTP_REFERER with an `https:` scheme are all
# there to get us down just the right path for Django to blow up
# when presented with an HTTP_HOST that's not a valid DNS name.
client = django.test.Client(enforce_csrf_checks=True)
result = client.post('/json/users',
secure=True,
HTTP_REFERER='https://somewhere',
HTTP_HOST='$nonsense')
self.assertEqual(result.status_code, 400)
}}}

The workaround is to override `handler400` with a simple implementation
that just skips the `@requires_csrf_token` from the default
implementation. Like this:
{{{
def handler400(request, exception):
return HttpResponseBadRequest(
'<h1>Bad Request (400)</h1>', content_type='text/html')
}}}
(That version also skips the template lookup, since we haven't provided a
400 template.)

To fix it in Django, it'd be enough to just remove the
`@requires_csrf_token` decorator from `django.views.defaults.bad_request`.
I'm not sure what that might break, though -- I think the case where it
could matter is if an app's `400.html` tries to use `csrf_token`, but I'm
not sure what the failure would look like. I think it could be pretty
reasonable to say that a 400 error page can't use `csrf_token`, so long as
the error that results if someone tries to do that is an appropriate one.

An alternate fix would be to try to make `requires_csrf_token` more
robust. If it's going to be used on a 4xx error page (or at least on 400),
it needs to really truly never raise an exception, no matter how
nonsensical the request the client sent is, because that's where we go
when we know the client has gotten something wrong.

See https://github.com/zulip/zulip/pull/6934 for an example traceback, and
my actual workaround and test code.

--
Ticket URL: <https://code.djangoproject.com/ticket/28693>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

[Django]

#28693: DisallowedHost can cause uncaught exception and HTTP 500

--------------------------------+------------------------------------------
Reporter: Greg Price | Owner: Tomer Chachamu
Type: Bug | Status: assigned
Component: Uncategorized | Version: 1.11
Severity: Normal | Resolution:
Keywords: DisallowedHost | Triage Stage: Accepted

Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0

--------------------------------+------------------------------------------
Changes (by Tomer Chachamu):

* owner: nobody => Tomer Chachamu
* status: new => assigned
* stage: Unreviewed => Accepted

Comment:

There would need to be a deprecation path for custom 400 views that are
using csrf tokens, so I think catching DisallowedHost in the traceback is
the right answer here.

--
Ticket URL: <https://code.djangoproject.com/ticket/28693#comment:1>

[Django]

#28693: DisallowedHost can cause uncaught exception and HTTP 500

--------------------------------+------------------------------------
Reporter: Greg Price | Owner: (none)
Type: Bug | Status: new
Component: HTTP handling | Version: 1.11

Severity: Normal | Resolution:
Keywords: DisallowedHost | Triage Stage: Accepted

Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0

--------------------------------+------------------------------------
Changes (by Tomer Chachamu):

* cc: Tomer Chachamu (added)
* owner: Tomer Chachamu => (none)
* has_patch: 0 => 1
* status: assigned => new
* component: Uncategorized => HTTP handling

--
Ticket URL: <https://code.djangoproject.com/ticket/28693#comment:2>

[Django]

#28693: DisallowedHost can cause uncaught exception and HTTP 500

-------------------------------------+-------------------------------------
Reporter: Greg Price | Owner: Tim
| Graham <timograham@…>
Type: Bug | Status: closed

Component: HTTP handling | Version: 1.11

Severity: Normal | Resolution: fixed

Keywords: DisallowedHost | Triage Stage: Accepted

Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0

-------------------------------------+-------------------------------------
Changes (by Tim Graham <timograham@…>):

* owner: (none) => Tim Graham <timograham@…>
* status: new => closed
* resolution: => fixed

Comment:

In [changeset:"7ec0fdf62afd565dd9a888300e7e33d0bf3e5fd5" 7ec0fdf6]:
{{{
#!CommitTicketReference repository=""
revision="7ec0fdf62afd565dd9a888300e7e33d0bf3e5fd5"
Fixed #28693 -- Fixed crash in CsrfViewMiddleware when an HTTPS request
has an invalid host.
}}}

--
Ticket URL: <https://code.djangoproject.com/ticket/28693#comment:3>