Opened 4 years ago

Closed 6 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 Carlton Gibson, 4 years ago

Triage Stage: UnreviewedAccepted

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:

diff --git a/django/test/client.py b/django/test/client.py
index ef944415d1..9855f2957e 100644
--- a/django/test/client.py
+++ b/django/test/client.py
@@ -818,6 +818,7 @@ class Client(ClientMixin, RequestFactory):
                 extra['wsgi.url_scheme'] = url.scheme
             if url.hostname:
                 extra['SERVER_NAME'] = url.hostname
+                extra['HTTP_HOST'] = url.hostname
             if url.port:
                 extra['SERVER_PORT'] = str(url.port)

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.

comment:2 by bcail, 6 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 bcail, 6 months ago

Cc: bcail added
Has patch: set

comment:4 by David Sanders, 6 months ago

Owner: changed from nobody to bcail
Status: newassigned

comment:5 by Mariusz Felisiak, 6 months ago

Triage Stage: AcceptedReady for checkin

comment:6 by Mariusz Felisiak <felisiak.mariusz@…>, 6 months ago

Resolution: fixed
Status: assignedclosed

In 523fed1:

Fixed #32106 -- Preserved HTTP_HOST in test Client when following redirects.

Co-authored-by: David Sanders <shang.xiao.sanders@…>

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