Skip to content

Commit

Permalink
Merge pull request openedx#18328 from edx/youngstrom/use-require-http…
Browse files Browse the repository at this point in the history
…s-arg

Add require_https to all is_safe_url calls
  • Loading branch information
Michael Youngstrom authored Jun 7, 2018
2 parents ca0ec64 + d6616c9 commit 6d8aebe
Show file tree
Hide file tree
Showing 5 changed files with 8 additions and 10 deletions.
2 changes: 1 addition & 1 deletion common/djangoapps/student/helpers.py
Original file line number Diff line number Diff line change
Expand Up @@ -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}
Expand Down
1 change: 0 additions & 1 deletion common/djangoapps/student/tests/test_helpers.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion common/djangoapps/student/views/login.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
11 changes: 5 additions & 6 deletions openedx/core/djangoapps/external_auth/tests/test_helper.py
Original file line number Diff line number Diff line change
Expand Up @@ -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)
2 changes: 1 addition & 1 deletion openedx/core/djangoapps/external_auth/views.py
Original file line number Diff line number Diff line change
Expand Up @@ -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)

Expand Down

0 comments on commit 6d8aebe

Please sign in to comment.