Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed #28693 -- Fixed DisallowedHost crash in CsrfViewMiddleware when an HTTPS request has an invalid host. #9268

Merged
merged 1 commit into from Feb 15, 2018

Conversation

r3m0t
Copy link
Contributor

@r3m0t r3m0t commented Oct 22, 2017

Copy link
Member

@timgraham timgraham left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A test in tests/csrf_tests/tests.py is probably appropriate (or perhaps more appropriate than the one for bad_request) consider that's where the problem is.

@@ -13,3 +13,6 @@ Bugfixes
argument is a callable that returns ``None`` (:ticket:`28601`).

* Fixed the Basque ``DATE_FORMAT`` string (:ticket:`28710`).

* Fixed sending status 500 instead of status 400 when an incorrect `Host`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure that it's important to backport the fix. It seems like it would only affect someone who isn't following the deployment advice,

You should also configure the Web server that sits in front of Django to validate the host. It should respond with a static error page or ignore requests for incorrect hosts instead of forwarding the request to Django. This way you’ll avoid spurious errors in your Django logs (or emails if you have error reporting configured that way).

@override_settings(ALLOWED_HOSTS=['www.example.com'])
def test_bad_request_bad_host(self):
"""
The bad request page loads.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if the Host header isn't in ALLOWED_HOSTS.

@timgraham timgraham changed the title Fix #28693 (DisallowedHost on csrf-protected POST) Fixed #28693 -- Fixed DisallowedHost crash in CsrfViewMiddleware when an HTTPS request has an invalid host. Nov 9, 2017
@r3m0t
Copy link
Contributor Author

r3m0t commented Nov 14, 2017

@timgraham fixed

@timgraham
Copy link
Member

Did you see and consider my comment, "A test in tests/csrf_tests/tests.py is probably appropriate (or perhaps more appropriate than the one for bad_request) consider that's where the problem is."

@r3m0t
Copy link
Contributor Author

r3m0t commented Nov 14, 2017

No, I will see if that's possible

@r3m0t
Copy link
Contributor Author

r3m0t commented Nov 15, 2017

@timgraham yep, fixed (for real this time)

Copy link

@gnprice gnprice left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for fixing this! (I reported the original bug.)

The fix looks basically reasonable to me, but I don't have enough context on this code to say with confidence if it's the right way to do it.

I am puzzled by one of the new tests -- see below.

req.META['HTTP_REFERER'] = 'https://www.evil.org/somepage'
req.META['SERVER_PORT'] = '443'
resp = requires_csrf_token(token_view)(req)
self.assertEqual(resp.status_code, 200)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I totally understand this test.

Does it actually involve the bad-request page? It doesn't look like it, but the docstring says so.

Why is the status code 200, rather than 400? This looks to me like a request with an HTTP_HOST that doesn't match ALLOWED_HOSTS.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, it doesn't. requires_csrf_token(token_view) is a standin for the default 400 handler, but it gives status 200. I will improve the docstring.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Much clearer, thanks!

@timgraham timgraham force-pushed the 28693-csrf-on-400 branch 5 times, most recently from 4d4d152 to 631416d Compare February 15, 2018 01:13
@timgraham timgraham merged commit 7ec0fdf into django:master Feb 15, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants