From 55426894cd0d430b90c72f2bf0880154c19838bc Mon Sep 17 00:00:00 2001 From: Greg Price Date: Mon, 9 Oct 2017 21:39:36 -0700 Subject: [PATCH] errors: Force a super-simpler handler for 400 errors. This works around a bug in Django in handling the error case of a client sending an inappropriate HTTP `Host:` header. Various internal Django machinery expects to be able to casually call `request.get_host()`, which will attempt to parse that header, so an exception will be raised. The exception-handling machinery attempts to catch that exception and just turn it into a 400 response... but in a certain case, that machinery itself ends up trying to call `request.get_host()`, and we end up with an uncaught exception that causes a 500 response, a chain of tracebacks in the logs, and an email to the server admins. See example below. That `request.get_host` call comes in the midst of some CSRF-related middleware, which doesn't even serve any function unless you have a form in your 400 response page that you want CSRF protection for. We use the default 400 response page, which is a 26-byte static HTML error message. So, just send that with no further ado. Example exception from server logs (lightly edited): 2017-10-08 09:51:50.835 ERR [django.security.DisallowedHost] Invalid HTTP_HOST header: 'example.com'. You may need to add 'example.com' to ALLOWED_HOSTS. 2017-10-08 09:51:50.835 ERR [django.request] Internal Server Error: /loginWithSetCookie Traceback (most recent call last): File ".../django/core/handlers/exception.py", line 41, in inner response = get_response(request) File ".../django/utils/deprecation.py", line 138, in __call__ response = self.process_request(request) File ".../django/middleware/common.py", line 57, in process_request host = request.get_host() File ".../django/http/request.py", line 113, in get_host raise DisallowedHost(msg) django.core.exceptions.DisallowedHost: Invalid HTTP_HOST header: 'example.com'. You may need to add 'example.com' to ALLOWED_HOSTS. During handling of the above exception, another exception occurred: Traceback (most recent call last): File ".../django/core/handlers/exception.py", line 109, in get_exception_response response = callback(request, **dict(param_dict, exception=exception)) File ".../django/utils/decorators.py", line 145, in _wrapped_view result = middleware.process_view(request, view_func, args, kwargs) File ".../django/middleware/csrf.py", line 276, in process_view good_referer = request.get_host() File ".../django/http/request.py", line 113, in get_host raise DisallowedHost(msg) django.core.exceptions.DisallowedHost: Invalid HTTP_HOST header: 'example.com'. You may need to add 'example.com' to ALLOWED_HOSTS. --- zerver/tests/test_urls.py | 17 ++++++++++++++++- zproject/urls.py | 17 +++++++++++++++++ 2 files changed, 33 insertions(+), 1 deletion(-) diff --git a/zerver/tests/test_urls.py b/zerver/tests/test_urls.py index 04634efb5a..2e64a50a35 100644 --- a/zerver/tests/test_urls.py +++ b/zerver/tests/test_urls.py @@ -5,7 +5,7 @@ import os import ujson import django.core.urlresolvers -from django.test import TestCase +from django.test import TestCase, Client from typing import List, Optional from zerver.lib.test_classes import ZulipTestCase @@ -143,3 +143,18 @@ class URLResolutionTest(TestCase): if callback_str: (module_name, base_view) = callback_str.rsplit(".", 1) self.check_function_exists(module_name, base_view) + +class ErrorPageTest(TestCase): + def test_bogus_http_host(self): + # type: () -> None + # 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 = 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) diff --git a/zproject/urls.py b/zproject/urls.py index 71a383e7b8..6d80113d6d 100644 --- a/zproject/urls.py +++ b/zproject/urls.py @@ -1,6 +1,7 @@ from django.conf import settings from django.conf.urls import url, include from django.conf.urls.i18n import i18n_patterns +from django.http import HttpResponseBadRequest, HttpRequest, HttpResponse from django.views.generic import TemplateView, RedirectView from django.utils.module_loading import import_string import os @@ -511,3 +512,19 @@ if settings.DEVELOPMENT: # reverse url mapping points to i18n urls which causes the frontend # tests to fail urlpatterns = i18n_patterns(*i18n_urls) + urls + legacy_urls + +def handler400(request, exception): + # type: (HttpRequest, Exception) -> HttpResponse + # This behaves exactly like the default Django implementation in + # the case where you haven't made a template "400.html", which we + # haven't -- except that it doesn't call `@requires_csrf_token` to + # attempt to set a `csrf_token` variable that the template could + # use if there were a template. We skip @requires_csrf_token because that + # codepath can raise an error on a bad request, which is exactly + # the case we're trying to handle when we get here. + # + # This function is used just because it has this special name in + # the root urls.py file; for more details, see: + # https://docs.djangoproject.com/en/1.11/topics/http/views/#customizing-error-views + return HttpResponseBadRequest( + '

Bad Request (400)

', content_type='text/html')