From 056a3c6c374f15e23746ea8568cd5b11bfe7d442 Mon Sep 17 00:00:00 2001 From: wrwrwr Date: Fri, 17 Oct 2014 15:46:42 +0200 Subject: [PATCH] Fixed #23682 -- Enhanced circular redirects detection in tests. When the test client detects a redirect to a URL seen in the currently followed chain it will now raise a RedirectCycleError instead of just returning the first repeated response. It will also complain when a single chain of redirects is longer than 20, as this often means a redirect loop with varying URLs, and even if it's not actually one, such long chains are likely to be treated as loops by browsers. Thanks Preston Timmons, Berker Peksag, and Tim Graham for reviews. --- django/test/client.py | 31 +++++++++++++++++++++++------- docs/ref/exceptions.txt | 14 ++++++++++++++ docs/releases/1.8.txt | 4 ++++ tests/test_client_regress/tests.py | 15 ++++++++++++--- tests/test_client_regress/urls.py | 1 + tests/test_client_regress/views.py | 7 +++++++ 6 files changed, 62 insertions(+), 10 deletions(-) diff --git a/django/test/client.py b/django/test/client.py index fb8ff75e95e..b2633bd7e7c 100644 --- a/django/test/client.py +++ b/django/test/client.py @@ -27,7 +27,7 @@ from django.utils import six from django.utils.six.moves.urllib.parse import urlparse, urlsplit from django.test.utils import ContextList -__all__ = ('Client', 'RequestFactory', 'encode_file', 'encode_multipart') +__all__ = ('Client', 'RedirectCycleError', 'RequestFactory', 'encode_file', 'encode_multipart') BOUNDARY = 'BoUnDaRyStRiNg' @@ -35,6 +35,16 @@ MULTIPART_CONTENT = 'multipart/form-data; boundary=%s' % BOUNDARY CONTENT_TYPE_RE = re.compile('.*; charset=([\w\d-]+);?') +class RedirectCycleError(Exception): + """ + The test client has been asked to follow a redirect loop. + """ + def __init__(self, message, last_response): + super(RedirectCycleError, self).__init__(message) + self.last_response = last_response + self.redirect_chain = last_response.redirect_chain + + class FakePayload(object): """ A wrapper around BytesIO that restricts what can be read since data from @@ -630,11 +640,11 @@ class Client(RequestFactory): response.redirect_chain = [] while response.status_code in (301, 302, 303, 307): - url = response.url + response_url = response.url redirect_chain = response.redirect_chain - redirect_chain.append((url, response.status_code)) + redirect_chain.append((response_url, response.status_code)) - url = urlsplit(url) + url = urlsplit(response_url) if url.scheme: extra['wsgi.url_scheme'] = url.scheme if url.hostname: @@ -645,7 +655,14 @@ class Client(RequestFactory): response = self.get(url.path, QueryDict(url.query), follow=False, **extra) response.redirect_chain = redirect_chain - # Prevent loops - if response.redirect_chain[-1] in response.redirect_chain[0:-1]: - break + if redirect_chain[-1] in redirect_chain[:-1]: + # Check that we're not redirecting to somewhere we've already + # been to, to prevent loops. + raise RedirectCycleError("Redirect loop detected.", last_response=response) + if len(redirect_chain) > 20: + # Such a lengthy chain likely also means a loop, but one with + # a growing path, changing view, or changing query argument; + # 20 is the value of "network.http.redirection-limit" from Firefox. + raise RedirectCycleError("Too many redirects.", last_response=response) + return response diff --git a/docs/ref/exceptions.txt b/docs/ref/exceptions.txt index e74e0991e1d..bbd02296103 100644 --- a/docs/ref/exceptions.txt +++ b/docs/ref/exceptions.txt @@ -217,6 +217,20 @@ Transaction exceptions are defined in :mod:`django.db.transaction`. The :exc:`TransactionManagementError` is raised for any and all problems related to database transactions. +.. currentmodule:: django.test + +Testing Framework Exceptions +============================ + +Exceptions provided by the :mod:`django.test` package. + +.. exception:: client.RedirectCycleError + + .. versionadded:: 1.8 + + :exc:`~client.RedirectCycleError` is raised when the test client detects a + loop or an overly long chain of redirects. + Python Exceptions ================= diff --git a/docs/releases/1.8.txt b/docs/releases/1.8.txt index ba0b0cfa2b5..73ad244cfbc 100644 --- a/docs/releases/1.8.txt +++ b/docs/releases/1.8.txt @@ -799,6 +799,10 @@ Miscellaneous when both the ``fields`` and ``form_class`` attributes are specified. Previously, ``fields`` was silently ignored. +* When following redirects, the test client now raises + :exc:`~django.test.client.RedirectCycleError` if it detects a loop or hits a + maximum redirect limit (rather than passing silently). + .. _deprecated-features-1.8: Features deprecated in 1.8 diff --git a/tests/test_client_regress/tests.py b/tests/test_client_regress/tests.py index 63e60ee8bde..0d63ee94294 100644 --- a/tests/test_client_regress/tests.py +++ b/tests/test_client_regress/tests.py @@ -10,7 +10,7 @@ import itertools from django.core.urlresolvers import reverse, NoReverseMatch from django.template import TemplateSyntaxError, Context, Template from django.test import Client, TestCase, override_settings -from django.test.client import encode_file, RequestFactory +from django.test.client import RedirectCycleError, RequestFactory, encode_file from django.test.utils import ContextList, str_prefix from django.template.response import SimpleTemplateResponse from django.utils._os import upath @@ -377,15 +377,24 @@ class AssertRedirectsTests(TestCase): def test_redirect_chain_to_self(self): "Redirections to self are caught and escaped" - response = self.client.get('/redirect_to_self/', {}, follow=True) + with self.assertRaises(RedirectCycleError) as context: + self.client.get('/redirect_to_self/', {}, follow=True) + response = context.exception.last_response # The chain of redirects stops once the cycle is detected. self.assertRedirects(response, '/redirect_to_self/', status_code=301, target_status_code=301) self.assertEqual(len(response.redirect_chain), 2) + def test_redirect_to_self_with_changing_query(self): + "Redirections don't loop forever even if query is changing" + with self.assertRaises(RedirectCycleError): + self.client.get('/redirect_to_self_with_changing_query_view/', {'counter': '0'}, follow=True) + def test_circular_redirect(self): "Circular redirect chains are caught and escaped" - response = self.client.get('/circular_redirect_1/', {}, follow=True) + with self.assertRaises(RedirectCycleError) as context: + self.client.get('/circular_redirect_1/', {}, follow=True) + response = context.exception.last_response # The chain of redirects will get back to the starting point, but stop there. self.assertRedirects(response, '/circular_redirect_2/', status_code=301, target_status_code=301) diff --git a/tests/test_client_regress/urls.py b/tests/test_client_regress/urls.py index 1bd0e0bc5b4..a69a774555d 100644 --- a/tests/test_client_regress/urls.py +++ b/tests/test_client_regress/urls.py @@ -21,6 +21,7 @@ urlpatterns = [ url(r'^redirect_to_non_existent_view/$', RedirectView.as_view(url='/non_existent_view/')), url(r'^redirect_to_non_existent_view2/$', RedirectView.as_view(url='/redirect_to_non_existent_view/')), url(r'^redirect_to_self/$', RedirectView.as_view(url='/redirect_to_self/')), + url(r'^redirect_to_self_with_changing_query_view/$', views.redirect_to_self_with_changing_query_view), url(r'^circular_redirect_1/$', RedirectView.as_view(url='/circular_redirect_2/')), url(r'^circular_redirect_2/$', RedirectView.as_view(url='/circular_redirect_3/')), url(r'^circular_redirect_3/$', RedirectView.as_view(url='/circular_redirect_1/')), diff --git a/tests/test_client_regress/views.py b/tests/test_client_regress/views.py index 656d7a4651a..b752d1b9f72 100644 --- a/tests/test_client_regress/views.py +++ b/tests/test_client_regress/views.py @@ -11,6 +11,7 @@ from django.template.loader import render_to_string from django.test import Client from django.test.client import CONTENT_TYPE_RE from django.test.utils import setup_test_environment +from django.utils.six.moves.urllib.parse import urlencode class CustomTestException(Exception): @@ -85,6 +86,12 @@ def login_protected_redirect_view(request): login_protected_redirect_view = login_required(login_protected_redirect_view) +def redirect_to_self_with_changing_query_view(request): + query = request.GET.copy() + query['counter'] += '0' + return HttpResponseRedirect('/redirect_to_self_with_changing_query_view/?%s' % urlencode(query)) + + def set_session_view(request): "A view that sets a session variable" request.session['session_var'] = 'YES'