Opened 4 years ago
Closed 14 months ago
#32106 closed Bug (fixed)
Redirect following in test client does not correctly set HTTP_HOST
Reported by: | Brenton Partridge | Owned by: | bcail |
---|---|---|---|
Component: | Testing framework | Version: | 3.1 |
Severity: | Normal | Keywords: | |
Cc: | bcail | Triage Stage: | Ready for checkin |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
Testing breaks when the following are true, which might be the case when testing e.g. the content resulting from a deep link from one Site to another Site:
- django.test.Client is constructed with HTTP_HOST explicitly set to Hostname 1
- a 301/302 response redirects to a URL with Hostname 2
- follow=True
Expected behavior is that, in the view logic handling the follow, request.get_host() would return Hostname 2.
But actual behavior is that, in the view logic handling the follow, request.get_host() would return Hostname 1.
This is because django.test.Client._handle_redirects only sets SERVER_NAME on its recursive ".get" call, not HTTP_HOST. Therefore, the WSGIRequest would have HTTP_HOST still set from the defaults from the Client constructor, and request._get_raw_host() checks HTTP_HOST before SERVER_NAME.
And yet one would expect HTTP_HOST to be the one set, as this would mimic a user agent setting the Host request header based on the Location response header from the 301/302.
https://github.com/django/django/blob/stable/3.1.x/django/test/client.py#L820
This can be mitigated by never using HTTP_HOST in test code, instead setting SERVER_NAME in the Client constructor; alternately, it can be sidestepped by setting follow=False and manually creating a new Client to make the follow request.
Fix considerations:
Users may have previously come to rely on SERVER_NAME being set here, so perhaps the right fix would be to set both HTTP_HOST and SERVER_NAME to the new hostname.
But if full backwards compatibility is desired, at the very least there should be documentation to recommend against using HTTP_HOST as a keyword argument to django.test.Client.
Change History (6)
comment:1 by , 4 years ago
Triage Stage: | Unreviewed → Accepted |
---|
comment:2 by , 15 months ago
Would something like this work for the test case? Should I open a PR for this?
diff --git a/tests/test_client/tests.py b/tests/test_client/tests.py index 402f282588..dc25d52ee2 100644 --- a/tests/test_client/tests.py +++ b/tests/test_client/tests.py @@ -26,7 +26,7 @@ from unittest import mock from django.contrib.auth.models import User from django.core import mail -from django.http import HttpResponse, HttpResponseNotAllowed +from django.http import HttpResponse, HttpResponseNotAllowed, HttpResponseRedirect from django.test import ( AsyncRequestFactory, Client, @@ -856,6 +856,36 @@ class ClientTest(TestCase): response, "https://www.djangoproject.com/", fetch_redirect_response=False ) + def test_external_redirect_http_host(self): + response_1 = HttpResponseRedirect(redirect_to='https://www.djangoproject.com') + response_2 = HttpResponse() + + with mock.patch('django.test.Client.request') as m: + m.side_effect = [response_1, response_2] + client = self.client_class() + response = client.get("/django_project_redirect/", follow=True) + + call_1_params = { + 'PATH_INFO': '/django_project_redirect/', + 'REQUEST_METHOD': 'GET', + 'SERVER_PORT': '80', + 'wsgi.url_scheme': 'http', + 'QUERY_STRING': '' + } + call_2_params = { + 'PATH_INFO': '/', + 'REQUEST_METHOD': 'GET', + 'SERVER_PORT': '80', + 'wsgi.url_scheme': 'https', + 'QUERY_STRING': '', + 'SERVER_NAME': 'www.djangoproject.com', + 'HTTP_HOST': 'www.djangoproject.com' + } + calls = m.mock_calls + + self.assertEqual(calls[0], mock.call(**call_1_params)) + self.assertEqual(calls[1], mock.call(**call_2_params)) + def test_external_redirect_without_trailing_slash(self): """ Client._handle_redirects() with an empty path.
comment:3 by , 14 months ago
Cc: | added |
---|---|
Has patch: | set |
comment:4 by , 14 months ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
comment:5 by , 14 months ago
Triage Stage: | Accepted → Ready for checkin |
---|
OK, happy to look at a PR here.
Not sure if we might not say
wontfix
or just tweak the docs, but this diff doesn't break anything:If that'd be sufficient for your use case then seems OK at first glance.
If you can put your example into a test case for the PR, that would be great.