From f227b8d15d9d0e0c50eb6459cf4556bccc3fae53 Mon Sep 17 00:00:00 2001 From: Jon Dufresne Date: Tue, 26 Jul 2016 20:45:07 -0700 Subject: [PATCH] Refs #26956 -- Allowed is_safe_url() to validate against multiple hosts --- django/contrib/auth/views.py | 4 ++-- django/utils/http.py | 22 +++++++++++++++++----- django/views/i18n.py | 4 ++-- docs/internals/deprecation.txt | 3 +++ docs/releases/1.11.txt | 3 +++ tests/utils_tests/test_http.py | 30 +++++++++++++++++++++--------- 6 files changed, 48 insertions(+), 18 deletions(-) diff --git a/django/contrib/auth/views.py b/django/contrib/auth/views.py index cf256d1639..8912c87aee 100644 --- a/django/contrib/auth/views.py +++ b/django/contrib/auth/views.py @@ -86,7 +86,7 @@ class LoginView(FormView): ) url_is_safe = is_safe_url( url=redirect_to, - host=self.request.get_host(), + allowed_hosts={self.request.get_host()}, require_https=self.request.is_secure(), ) if not url_is_safe: @@ -157,7 +157,7 @@ class LogoutView(TemplateView): ) url_is_safe = is_safe_url( url=next_page, - host=self.request.get_host(), + allowed_hosts={self.request.get_host()}, require_https=self.request.is_secure(), ) # Security check -- don't allow redirection to a different host. diff --git a/django/utils/http.py b/django/utils/http.py index f662e395b4..c6285e6f40 100644 --- a/django/utils/http.py +++ b/django/utils/http.py @@ -6,12 +6,14 @@ import datetime import re import sys import unicodedata +import warnings from binascii import Error as BinasciiError from email.utils import formatdate from django.core.exceptions import TooManyFieldsSent from django.utils import six from django.utils.datastructures import MultiValueDict +from django.utils.deprecation import RemovedInDjango21Warning from django.utils.encoding import force_bytes, force_str, force_text from django.utils.functional import keep_lazy_text from django.utils.six.moves.urllib.parse import ( @@ -277,7 +279,7 @@ def is_same_domain(host, pattern): ) -def is_safe_url(url, host=None, require_https=False): +def is_safe_url(url, host=None, allowed_hosts=None, require_https=False): """ Return ``True`` if the url is a safe redirection (i.e. it doesn't point to a different host and uses a safe scheme). @@ -296,13 +298,23 @@ def is_safe_url(url, host=None, require_https=False): url = force_text(url) except UnicodeDecodeError: return False + if allowed_hosts is None: + allowed_hosts = set() + if host: + warnings.warn( + "The host argument is deprecated, use allowed_hosts instead.", + RemovedInDjango21Warning, + stacklevel=2, + ) + # Avoid mutating the passed in allowed_hosts. + allowed_hosts = allowed_hosts | {host} # Chrome treats \ completely as / in paths but it could be part of some # basic auth credentials so we need to check both URLs. - return (_is_safe_url(url, host, require_https=require_https) and - _is_safe_url(url.replace('\\', '/'), host, require_https=require_https)) + return (_is_safe_url(url, allowed_hosts, require_https=require_https) and + _is_safe_url(url.replace('\\', '/'), allowed_hosts, require_https=require_https)) -def _is_safe_url(url, host, require_https=False): +def _is_safe_url(url, allowed_hosts, require_https=False): # Chrome considers any URL with more than two slashes to be absolute, but # urlparse is not so flexible. Treat any url with three slashes as unsafe. if url.startswith('///'): @@ -324,7 +336,7 @@ def _is_safe_url(url, host, require_https=False): if not url_info.scheme and url_info.netloc: scheme = 'http' valid_schemes = ['https'] if require_https else ['http', 'https'] - return ((not url_info.netloc or url_info.netloc == host) and + return ((not url_info.netloc or url_info.netloc in allowed_hosts) and (not scheme or scheme in valid_schemes)) diff --git a/django/views/i18n.py b/django/views/i18n.py index 5cc6a3ebe1..eda8422f69 100644 --- a/django/views/i18n.py +++ b/django/views/i18n.py @@ -38,11 +38,11 @@ def set_language(request): """ next = request.POST.get('next', request.GET.get('next')) if ((next or not request.is_ajax()) and - not is_safe_url(url=next, host=request.get_host(), require_https=request.is_secure())): + not is_safe_url(url=next, allowed_hosts={request.get_host()}, require_https=request.is_secure())): next = request.META.get('HTTP_REFERER') if next: next = urlunquote(next) # HTTP_REFERER may be encoded. - if not is_safe_url(url=next, host=request.get_host(), require_https=request.is_secure()): + if not is_safe_url(url=next, allowed_hosts={request.get_host()}, require_https=request.is_secure()): next = '/' response = http.HttpResponseRedirect(next) if next else http.HttpResponse(status=204) if request.method == 'POST': diff --git a/docs/internals/deprecation.txt b/docs/internals/deprecation.txt index b983ae1ace..5d93915dc6 100644 --- a/docs/internals/deprecation.txt +++ b/docs/internals/deprecation.txt @@ -30,6 +30,9 @@ details on these changes. * ``django.core.cache.backends.memcached.PyLibMCCache`` will no longer support passing ``pylibmc`` behavior settings as top-level attributes of ``OPTIONS``. +* The ``host`` parameter of ``django.utils.http.is_safe_url()`` will be + removed. + .. _deprecation-removed-in-2.0: 2.0 diff --git a/docs/releases/1.11.txt b/docs/releases/1.11.txt index 46bc7f1529..a202232cb6 100644 --- a/docs/releases/1.11.txt +++ b/docs/releases/1.11.txt @@ -520,3 +520,6 @@ Miscellaneous * For the ``PyLibMCCache`` cache backend, passing ``pylibmc`` behavior settings as top-level attributes of ``OPTIONS`` is deprecated. Set them under a ``behaviors`` key within ``OPTIONS`` instead. + +* The ``host`` parameter of ``django.utils.http.is_safe_url()`` is deprecated + in favor of the new ``allowed_hosts`` parameter. diff --git a/tests/utils_tests/test_http.py b/tests/utils_tests/test_http.py index b690055f30..e8b4e51eb3 100644 --- a/tests/utils_tests/test_http.py +++ b/tests/utils_tests/test_http.py @@ -5,8 +5,10 @@ import sys import unittest from datetime import datetime +from django.test import ignore_warnings from django.utils import http, six from django.utils.datastructures import MultiValueDict +from django.utils.deprecation import RemovedInDjango21Warning class TestUtilsHttp(unittest.TestCase): @@ -107,7 +109,12 @@ class TestUtilsHttp(unittest.TestCase): '\n', ) for bad_url in bad_urls: - self.assertFalse(http.is_safe_url(bad_url, host='testserver'), "%s should be blocked" % bad_url) + with ignore_warnings(category=RemovedInDjango21Warning): + self.assertFalse(http.is_safe_url(bad_url, host='testserver'), "%s should be blocked" % bad_url) + self.assertFalse( + http.is_safe_url(bad_url, allowed_hosts={'testserver', 'testserver2'}), + "%s should be blocked" % bad_url, + ) good_urls = ( '/view/?param=http://example.com', @@ -121,20 +128,25 @@ class TestUtilsHttp(unittest.TestCase): '/url%20with%20spaces/', ) for good_url in good_urls: - self.assertTrue(http.is_safe_url(good_url, host='testserver'), "%s should be allowed" % good_url) + with ignore_warnings(category=RemovedInDjango21Warning): + self.assertTrue(http.is_safe_url(good_url, host='testserver'), "%s should be allowed" % good_url) + self.assertTrue( + http.is_safe_url(good_url, allowed_hosts={'otherserver', 'testserver'}), + "%s should be allowed" % good_url, + ) if six.PY2: # Check binary URLs, regression tests for #26308 self.assertTrue( - http.is_safe_url(b'https://testserver/', host='testserver'), + http.is_safe_url(b'https://testserver/', allowed_hosts={'testserver'}), "binary URLs should be allowed on Python 2" ) - self.assertFalse(http.is_safe_url(b'\x08//example.com', host='testserver')) - self.assertTrue(http.is_safe_url('àview/'.encode('utf-8'), host='testserver')) - self.assertFalse(http.is_safe_url('àview'.encode('latin-1'), host='testserver')) + self.assertFalse(http.is_safe_url(b'\x08//example.com', allowed_hosts={'testserver'})) + self.assertTrue(http.is_safe_url('àview/'.encode('utf-8'), allowed_hosts={'testserver'})) + self.assertFalse(http.is_safe_url('àview'.encode('latin-1'), allowed_hosts={'testserver'})) # Valid basic auth credentials are allowed. - self.assertTrue(http.is_safe_url(r'http://user:pass@testserver/', host='user:pass@testserver')) + self.assertTrue(http.is_safe_url(r'http://user:pass@testserver/', allowed_hosts={'user:pass@testserver'})) # A path without host is allowed. self.assertTrue(http.is_safe_url('/confirm/me@example.com')) # Basic auth without host is not allowed. @@ -147,7 +159,7 @@ class TestUtilsHttp(unittest.TestCase): '/view/?param=http://example.com', ) for url in secure_urls: - self.assertTrue(http.is_safe_url(url, 'example.com', require_https=True)) + self.assertTrue(http.is_safe_url(url, allowed_hosts={'example.com'}, require_https=True)) def test_is_safe_url_secure_param_non_https_urls(self): not_secure_urls = ( @@ -156,7 +168,7 @@ class TestUtilsHttp(unittest.TestCase): '//example.com/p', ) for url in not_secure_urls: - self.assertFalse(http.is_safe_url(url, 'example.com', require_https=True)) + self.assertFalse(http.is_safe_url(url, allowed_hosts={'example.com'}, require_https=True)) def test_urlsafe_base64_roundtrip(self): bytestring = b'foo'