From c49f84fa3c1f1f5aebe4f6c4817d0edff75639bc Mon Sep 17 00:00:00 2001 From: Ahsan Ulhaq Date: Thu, 20 Oct 2016 16:00:27 +0500 Subject: [PATCH 1/2] parameter next on login page would redirect regardless url safe ECOM-5968 --- common/djangoapps/student/helpers.py | 15 +++++++++ .../djangoapps/student/tests/test_helpers.py | 33 +++++++++++++++++++ 2 files changed, 48 insertions(+) create mode 100644 common/djangoapps/student/tests/test_helpers.py diff --git a/common/djangoapps/student/helpers.py b/common/djangoapps/student/helpers.py index 39b48ece31..3d23b14e31 100644 --- a/common/djangoapps/student/helpers.py +++ b/common/djangoapps/student/helpers.py @@ -1,9 +1,11 @@ """Helpers for the student app. """ from datetime import datetime +import logging import urllib from pytz import UTC from django.core.urlresolvers import reverse, NoReverseMatch +from django.utils import http from oauth2_provider.models import ( AccessToken as dot_access_token, RefreshToken as dot_refresh_token @@ -33,6 +35,9 @@ DISABLE_UNENROLL_CERT_STATES = [ ] +log = logging.getLogger(__name__) + + def check_verify_status_by_course(user, course_enrollments): """ Determine the per-course verification statuses for a given user. @@ -239,6 +244,16 @@ def get_next_url_for_login_page(request): specified. """ redirect_to = request.GET.get('next', None) + + # if we get a redirect parameter, make sure it's safe. If it's not, drop the + # parameter. + if redirect_to and not http.is_safe_url(redirect_to): + log.error( + u'Unsafe redirect parameter detected: %(redirect_to)r', + {"redirect_to": redirect_to} + ) + redirect_to = None + if not redirect_to: try: redirect_to = reverse('dashboard') diff --git a/common/djangoapps/student/tests/test_helpers.py b/common/djangoapps/student/tests/test_helpers.py new file mode 100644 index 0000000000..6ff4032aa1 --- /dev/null +++ b/common/djangoapps/student/tests/test_helpers.py @@ -0,0 +1,33 @@ +""" Test Student helpers """ + +import logging + +from django.test import TestCase +from django.test.client import RequestFactory +from testfixtures import LogCapture + +from student.helpers import get_next_url_for_login_page + + +LOGGER_NAME = "student.helpers" + + +class TestLoginHelper(TestCase): + """Test login helper methods.""" + def setUp(self): + super(TestLoginHelper, self).setUp() + self.request = RequestFactory() + + def test_unsafe_next(self): + """ Test unsafe next parameter """ + with LogCapture(LOGGER_NAME, level=logging.ERROR) as logger: + req = self.request.get("http://testserver/login?next=http://amazon.com") + get_next_url_for_login_page(req) + logger.check( + (LOGGER_NAME, "ERROR", u"Unsafe redirect parameter detected: u'http://amazon.com'")) + + def test_safe_next(self): + """ Test safe next parameter """ + req = self.request.get("http://testserver/login?next=/dashboard") + next_page = get_next_url_for_login_page(req) + self.assertEqual(next_page, u'/dashboard') From 7368c3428f4bcea549f58a9595a06b56acf6d733 Mon Sep 17 00:00:00 2001 From: Ahsan Ulhaq Date: Tue, 25 Oct 2016 23:28:09 +0500 Subject: [PATCH 2/2] use reverse --- common/djangoapps/student/tests/test_helpers.py | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/common/djangoapps/student/tests/test_helpers.py b/common/djangoapps/student/tests/test_helpers.py index 6ff4032aa1..687fbfd396 100644 --- a/common/djangoapps/student/tests/test_helpers.py +++ b/common/djangoapps/student/tests/test_helpers.py @@ -2,6 +2,7 @@ import logging +from django.core.urlresolvers import reverse from django.test import TestCase from django.test.client import RequestFactory from testfixtures import LogCapture @@ -20,14 +21,16 @@ class TestLoginHelper(TestCase): def test_unsafe_next(self): """ Test unsafe next parameter """ + unsafe_url = "https://www.amazon.com" with LogCapture(LOGGER_NAME, level=logging.ERROR) as logger: - req = self.request.get("http://testserver/login?next=http://amazon.com") + req = self.request.get(reverse("login") + "?next={url}".format(url=unsafe_url)) get_next_url_for_login_page(req) logger.check( - (LOGGER_NAME, "ERROR", u"Unsafe redirect parameter detected: u'http://amazon.com'")) + (LOGGER_NAME, "ERROR", u"Unsafe redirect parameter detected: u'{url}'".format(url=unsafe_url)) + ) def test_safe_next(self): """ Test safe next parameter """ - req = self.request.get("http://testserver/login?next=/dashboard") + req = self.request.get(reverse("login") + "?next={url}".format(url="/dashboard")) next_page = get_next_url_for_login_page(req) self.assertEqual(next_page, u'/dashboard')