Merge pull request #13799 from edx/ahsan/ECOM-5968-Redirect-parameter-next-login-page-redirect

parameter next on login page would redirect regardless url safe

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')

common/djangoapps/student/tests/test_helpers.py

@@ -0,0 +1,36 @@
+""" Test Student helpers """
+
+import logging
+
+from django.core.urlresolvers import reverse
+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 """
+        unsafe_url = "https://www.amazon.com"
+        with LogCapture(LOGGER_NAME, level=logging.ERROR) as logger:
+            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'{url}'".format(url=unsafe_url))
+            )
+
+    def test_safe_next(self):
+        """ Test safe next parameter """
+        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')