From 4f61810751751b8c5070ce038ea57e949650e9e3 Mon Sep 17 00:00:00 2001 From: Carlton Gibson Date: Wed, 14 Aug 2019 17:39:21 +0200 Subject: [PATCH] Fixed #30747 -- Renamed is_safe_url() to url_has_allowed_host_and_scheme(). --- django/contrib/auth/views.py | 8 +++--- django/utils/http.py | 26 ++++++++++++++----- django/views/i18n.py | 14 +++++++--- docs/internals/deprecation.txt | 2 ++ docs/releases/3.0.txt | 8 ++++++ tests/utils_tests/test_http.py | 47 +++++++++++++++++++++++++--------- 6 files changed, 80 insertions(+), 25 deletions(-) diff --git a/django/contrib/auth/views.py b/django/contrib/auth/views.py index 0d2326702a..529400df28 100644 --- a/django/contrib/auth/views.py +++ b/django/contrib/auth/views.py @@ -17,7 +17,9 @@ from django.http import HttpResponseRedirect, QueryDict from django.shortcuts import resolve_url from django.urls import reverse_lazy from django.utils.decorators import method_decorator -from django.utils.http import is_safe_url, urlsafe_base64_decode +from django.utils.http import ( + url_has_allowed_host_and_scheme, urlsafe_base64_decode, +) from django.utils.translation import gettext_lazy as _ from django.views.decorators.cache import never_cache from django.views.decorators.csrf import csrf_protect @@ -70,7 +72,7 @@ class LoginView(SuccessURLAllowedHostsMixin, FormView): self.redirect_field_name, self.request.GET.get(self.redirect_field_name, '') ) - url_is_safe = is_safe_url( + url_is_safe = url_has_allowed_host_and_scheme( url=redirect_to, allowed_hosts=self.get_success_url_allowed_hosts(), require_https=self.request.is_secure(), @@ -138,7 +140,7 @@ class LogoutView(SuccessURLAllowedHostsMixin, TemplateView): self.redirect_field_name, self.request.GET.get(self.redirect_field_name) ) - url_is_safe = is_safe_url( + url_is_safe = url_has_allowed_host_and_scheme( url=next_page, allowed_hosts=self.get_success_url_allowed_hosts(), require_https=self.request.is_secure(), diff --git a/django/utils/http.py b/django/utils/http.py index 050375832c..572cfb4347 100644 --- a/django/utils/http.py +++ b/django/utils/http.py @@ -294,15 +294,18 @@ def is_same_domain(host, pattern): ) -def is_safe_url(url, allowed_hosts, require_https=False): +def url_has_allowed_host_and_scheme(url, allowed_hosts, 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). + Return ``True`` if the url uses an allowed host and a safe scheme. Always return ``False`` on an empty url. If ``require_https`` is ``True``, only 'https' will be considered a valid scheme, as opposed to 'http' and 'https' with the default, ``False``. + + Note: "True" doesn't entail that a URL is "safe". It may still be e.g. + quoted incorrectly. Ensure to also use django.utils.encoding.iri_to_uri() + on the path component of untrusted URLs. """ if url is not None: url = url.strip() @@ -314,8 +317,19 @@ def is_safe_url(url, allowed_hosts, require_https=False): allowed_hosts = {allowed_hosts} # 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, allowed_hosts, require_https=require_https) and - _is_safe_url(url.replace('\\', '/'), allowed_hosts, require_https=require_https)) + return ( + _url_has_allowed_host_and_scheme(url, allowed_hosts, require_https=require_https) and + _url_has_allowed_host_and_scheme(url.replace('\\', '/'), allowed_hosts, require_https=require_https) + ) + + +def is_safe_url(url, allowed_hosts, require_https=False): + warnings.warn( + 'django.utils.http.is_safe_url() is deprecated in favor of ' + 'url_has_allowed_host_and_scheme().', + RemovedInDjango40Warning, stacklevel=2, + ) + return url_has_allowed_host_and_scheme(url, allowed_hosts, require_https) # Copied from urllib.parse.urlparse() but uses fixed urlsplit() function. @@ -367,7 +381,7 @@ def _urlsplit(url, scheme='', allow_fragments=True): return _coerce_result(v) -def _is_safe_url(url, allowed_hosts, require_https=False): +def _url_has_allowed_host_and_scheme(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('///'): diff --git a/django/views/i18n.py b/django/views/i18n.py index e3ef40b2fc..cbe61bcf88 100644 --- a/django/views/i18n.py +++ b/django/views/i18n.py @@ -10,7 +10,7 @@ from django.http import HttpResponse, HttpResponseRedirect, JsonResponse from django.template import Context, Engine from django.urls import translate_url from django.utils.formats import get_format -from django.utils.http import is_safe_url +from django.utils.http import url_has_allowed_host_and_scheme from django.utils.translation import ( LANGUAGE_SESSION_KEY, check_for_language, get_language, ) @@ -32,11 +32,17 @@ def set_language(request): any state. """ next = request.POST.get('next', request.GET.get('next')) - if ((next or not request.is_ajax()) and - not is_safe_url(url=next, allowed_hosts={request.get_host()}, require_https=request.is_secure())): + if ( + (next or not request.is_ajax()) and + not url_has_allowed_host_and_scheme( + url=next, allowed_hosts={request.get_host()}, require_https=request.is_secure(), + ) + ): next = request.META.get('HTTP_REFERER') next = next and unquote(next) # HTTP_REFERER may be encoded. - if not is_safe_url(url=next, allowed_hosts={request.get_host()}, require_https=request.is_secure()): + if not url_has_allowed_host_and_scheme( + url=next, allowed_hosts={request.get_host()}, require_https=request.is_secure(), + ): next = '/' response = HttpResponseRedirect(next) if next else HttpResponse(status=204) if request.method == 'POST': diff --git a/docs/internals/deprecation.txt b/docs/internals/deprecation.txt index 7020a3341a..f29b6b2fac 100644 --- a/docs/internals/deprecation.txt +++ b/docs/internals/deprecation.txt @@ -32,6 +32,8 @@ details on these changes. * ``django.utils.text.unescape_entities()`` will be removed. +* ``django.utils.http.is_safe_url()`` will be removed. + .. _deprecation-removed-in-3.1: 3.1 diff --git a/docs/releases/3.0.txt b/docs/releases/3.0.txt index 910d3a2592..52d435d5d5 100644 --- a/docs/releases/3.0.txt +++ b/docs/releases/3.0.txt @@ -601,6 +601,14 @@ Miscellaneous :func:`html.unescape`. Note that unlike ``unescape_entities()``, ``html.unescape()`` evaluates lazy strings immediately. +* To avoid possible confusion as to effective scope, the private internal + utility ``is_safe_url()`` is renamed to + ``url_has_allowed_host_and_scheme()``. That a URL has an allowed host and + scheme doesn't in general imply that it's "safe". It may still be quoted + incorrectly, for example. Ensure to also use + :func:`~django.utils.encoding.iri_to_uri` on the path component of untrusted + URLs. + .. _removed-features-3.0: Features removed in 3.0 diff --git a/tests/utils_tests/test_http.py b/tests/utils_tests/test_http.py index d1a1243ace..bc8bcfe977 100644 --- a/tests/utils_tests/test_http.py +++ b/tests/utils_tests/test_http.py @@ -7,8 +7,8 @@ from django.utils.deprecation import RemovedInDjango40Warning from django.utils.http import ( base36_to_int, escape_leading_slashes, http_date, int_to_base36, is_safe_url, is_same_domain, parse_etags, parse_http_date, quote_etag, - urlencode, urlquote, urlquote_plus, urlsafe_base64_decode, - urlsafe_base64_encode, urlunquote, urlunquote_plus, + url_has_allowed_host_and_scheme, urlencode, urlquote, urlquote_plus, + urlsafe_base64_decode, urlsafe_base64_encode, urlunquote, urlunquote_plus, ) @@ -128,7 +128,7 @@ class Base36IntTests(SimpleTestCase): self.assertEqual(base36_to_int(b36), n) -class IsSafeURLTests(unittest.TestCase): +class IsSafeURLTests(SimpleTestCase): def test_bad_urls(self): bad_urls = ( 'http://example.com', @@ -164,7 +164,10 @@ class IsSafeURLTests(unittest.TestCase): ) for bad_url in bad_urls: with self.subTest(url=bad_url): - self.assertIs(is_safe_url(bad_url, allowed_hosts={'testserver', 'testserver2'}), False) + self.assertIs( + url_has_allowed_host_and_scheme(bad_url, allowed_hosts={'testserver', 'testserver2'}), + False, + ) def test_good_urls(self): good_urls = ( @@ -181,21 +184,27 @@ class IsSafeURLTests(unittest.TestCase): ) for good_url in good_urls: with self.subTest(url=good_url): - self.assertIs(is_safe_url(good_url, allowed_hosts={'otherserver', 'testserver'}), True) + self.assertIs( + url_has_allowed_host_and_scheme(good_url, allowed_hosts={'otherserver', 'testserver'}), + True, + ) def test_basic_auth(self): # Valid basic auth credentials are allowed. - self.assertIs(is_safe_url(r'http://user:pass@testserver/', allowed_hosts={'user:pass@testserver'}), True) + self.assertIs( + url_has_allowed_host_and_scheme(r'http://user:pass@testserver/', allowed_hosts={'user:pass@testserver'}), + True, + ) def test_no_allowed_hosts(self): # A path without host is allowed. - self.assertIs(is_safe_url('/confirm/me@example.com', allowed_hosts=None), True) + self.assertIs(url_has_allowed_host_and_scheme('/confirm/me@example.com', allowed_hosts=None), True) # Basic auth without host is not allowed. - self.assertIs(is_safe_url(r'http://testserver\@example.com', allowed_hosts=None), False) + self.assertIs(url_has_allowed_host_and_scheme(r'http://testserver\@example.com', allowed_hosts=None), False) def test_allowed_hosts_str(self): - self.assertIs(is_safe_url('http://good.com/good', allowed_hosts='good.com'), True) - self.assertIs(is_safe_url('http://good.co/evil', allowed_hosts='good.com'), False) + self.assertIs(url_has_allowed_host_and_scheme('http://good.com/good', allowed_hosts='good.com'), True) + self.assertIs(url_has_allowed_host_and_scheme('http://good.co/evil', allowed_hosts='good.com'), False) def test_secure_param_https_urls(self): secure_urls = ( @@ -205,7 +214,10 @@ class IsSafeURLTests(unittest.TestCase): ) for url in secure_urls: with self.subTest(url=url): - self.assertIs(is_safe_url(url, allowed_hosts={'example.com'}, require_https=True), True) + self.assertIs( + url_has_allowed_host_and_scheme(url, allowed_hosts={'example.com'}, require_https=True), + True, + ) def test_secure_param_non_https_urls(self): insecure_urls = ( @@ -215,7 +227,18 @@ class IsSafeURLTests(unittest.TestCase): ) for url in insecure_urls: with self.subTest(url=url): - self.assertIs(is_safe_url(url, allowed_hosts={'example.com'}, require_https=True), False) + self.assertIs( + url_has_allowed_host_and_scheme(url, allowed_hosts={'example.com'}, require_https=True), + False, + ) + + def test_is_safe_url_deprecated(self): + msg = ( + 'django.utils.http.is_safe_url() is deprecated in favor of ' + 'url_has_allowed_host_and_scheme().' + ) + with self.assertWarnsMessage(RemovedInDjango40Warning, msg): + is_safe_url('https://example.com', allowed_hosts={'example.com'}) class URLSafeBase64Tests(unittest.TestCase):