From d6616c903620d655dba2d3b8a95890e2143217de Mon Sep 17 00:00:00 2001 From: Michael Youngstrom Date: Wed, 6 Jun 2018 11:43:57 -0400 Subject: [PATCH] Add require_https to all is_safe_url calls --- common/djangoapps/student/helpers.py | 2 +- common/djangoapps/student/tests/test_helpers.py | 1 - common/djangoapps/student/views/login.py | 2 +- .../djangoapps/external_auth/tests/test_helper.py | 11 +++++------ openedx/core/djangoapps/external_auth/views.py | 2 +- 5 files changed, 8 insertions(+), 10 deletions(-) diff --git a/common/djangoapps/student/helpers.py b/common/djangoapps/student/helpers.py index 42386e2b2ecb..709009f06b78 100644 --- a/common/djangoapps/student/helpers.py +++ b/common/djangoapps/student/helpers.py @@ -329,7 +329,7 @@ def get_redirect_to(request): # get information about a user on edx.org. In any such case drop the parameter. if redirect_to: mime_type, _ = mimetypes.guess_type(redirect_to, strict=False) - if not http.is_safe_url(redirect_to, allowed_hosts={request.get_host()}): + if not http.is_safe_url(redirect_to, allowed_hosts={request.get_host()}, require_https=True): log.warning( u'Unsafe redirect parameter detected after login page: %(redirect_to)r', {"redirect_to": redirect_to} diff --git a/common/djangoapps/student/tests/test_helpers.py b/common/djangoapps/student/tests/test_helpers.py index a8aec32f0ffa..3a47dcebbdaf 100644 --- a/common/djangoapps/student/tests/test_helpers.py +++ b/common/djangoapps/student/tests/test_helpers.py @@ -66,7 +66,6 @@ def test_unsafe_next(self, log_level, log_name, unsafe_url, http_accept, user_ag @ddt.data( ('/dashboard', 'testserver', '/dashboard'), - ('http://testserver/courses', 'testserver', 'http://testserver/courses'), ('https://edx.org/courses', 'edx.org', 'https://edx.org/courses'), ) @ddt.unpack diff --git a/common/djangoapps/student/views/login.py b/common/djangoapps/student/views/login.py index 2efbdde592df..aea20140be11 100644 --- a/common/djangoapps/student/views/login.py +++ b/common/djangoapps/student/views/login.py @@ -747,7 +747,7 @@ def target(self): """ target_url = self.request.GET.get('redirect_url') - if target_url and is_safe_url(target_url, allowed_hosts={self.request.META.get('HTTP_HOST')}): + if target_url and is_safe_url(target_url, allowed_hosts={self.request.META.get('HTTP_HOST')}, require_https=True): return target_url else: return self.default_target diff --git a/openedx/core/djangoapps/external_auth/tests/test_helper.py b/openedx/core/djangoapps/external_auth/tests/test_helper.py index b028d62dcdae..6afd2926ca3c 100644 --- a/openedx/core/djangoapps/external_auth/tests/test_helper.py +++ b/openedx/core/djangoapps/external_auth/tests/test_helper.py @@ -19,11 +19,10 @@ def test__safe_postlogin_redirect(self): ONSITE3 = 'http://{}/my/custom/url'.format(HOST) # pylint: disable=invalid-name OFFSITE1 = 'http://www.attacker.com' # pylint: disable=invalid-name - for redirect_to in [ONSITE1, ONSITE2, ONSITE3]: + for redirect_to in [ONSITE1, ONSITE2, ONSITE3, OFFSITE1]: redir = _safe_postlogin_redirect(redirect_to, HOST) self.assertEqual(redir.status_code, 302) - self.assertEqual(redir['location'], redirect_to) - - redir2 = _safe_postlogin_redirect(OFFSITE1, HOST) - self.assertEqual(redir2.status_code, 302) - self.assertEqual("/", redir2['location']) + if redirect_to in [ONSITE3, OFFSITE1]: + self.assertEqual(redir['location'], "/") + else: + self.assertEqual(redir['location'], redirect_to) diff --git a/openedx/core/djangoapps/external_auth/views.py b/openedx/core/djangoapps/external_auth/views.py index 24829d1ab7fe..a57ee08af6a9 100644 --- a/openedx/core/djangoapps/external_auth/views.py +++ b/openedx/core/djangoapps/external_auth/views.py @@ -556,7 +556,7 @@ def _safe_postlogin_redirect(redirect_to, safehost, default_redirect='/'): @param safehost: which host is safe to redirect to @return: an HttpResponseRedirect """ - if is_safe_url(url=redirect_to, allowed_hosts={safehost}): + if is_safe_url(url=redirect_to, allowed_hosts={safehost}, require_https=True): return redirect(redirect_to) return redirect(default_redirect)