edx.org/login?next= should not be able to point to an asset

ECOM-6463

common/djangoapps/student/helpers.py

@@ -3,7 +3,7 @@ from datetime import datetime
 import logging
 import urllib
 
-from pytz import UTC
+from django.conf import settings
 from django.core.urlresolvers import reverse, NoReverseMatch
 from django.utils import http
 from oauth2_provider.models import (
@@ -14,6 +14,7 @@ from provider.oauth2.models import (
     AccessToken as dop_access_token,
     RefreshToken as dop_refresh_token
 )
+from pytz import UTC
 
 import third_party_auth
 from lms.djangoapps.verify_student.models import VerificationDeadline, SoftwareSecurePhotoVerification
@@ -243,17 +244,7 @@ def get_next_url_for_login_page(request):
     Otherwise, we go to the ?next= query param or to the dashboard if nothing else is
     specified.
     """
-    redirect_to = request.GET.get('next', None)
+    redirect_to = get_redirect_to(request)
-
-    # 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')
@@ -270,6 +261,33 @@ def get_next_url_for_login_page(request):
     return redirect_to
 
 
+def get_redirect_to(request):
+    """
+    Determine the redirect url and return if safe
+    :argument
+        request: request object
+
+    :returns: redirect url if safe else None
+    """
+    redirect_to = request.GET.get('next')
+    header_accept = request.META.get('HTTP_ACCEPT', '')
+
+    # If we get a redirect parameter, make sure it's safe i.e. not redirecting outside our domain.
+    # Also make sure that it is not redirecting to a static asset and redirected page is web page
+    # not a static file. As allowing assets to be pointed to by "next" allows 3rd party sites to
+    # get information about a user on edx.org. In any such case drop the parameter.
+    if redirect_to and (not http.is_safe_url(redirect_to)
+                        or settings.STATIC_URL in redirect_to
+                        or 'text/html' not in header_accept):
+        log.warning(
+            u'Unsafe redirect parameter detected after login page: %(redirect_to)r',
+            {"redirect_to": redirect_to}
+        )
+        redirect_to = None
+
+    return redirect_to
+
+
 def destroy_oauth_tokens(user):
     """
     Destroys ALL OAuth access and refresh tokens for the given user.

common/djangoapps/student/tests/test_helpers.py

@@ -1,7 +1,9 @@
 """ Test Student helpers """
 
 import logging
+import ddt
 
+from django.conf import settings
 from django.core.urlresolvers import reverse
 from django.test import TestCase
 from django.test.client import RequestFactory
@@ -13,24 +15,34 @@ from student.helpers import get_next_url_for_login_page
 LOGGER_NAME = "student.helpers"
 
 
+@ddt.ddt
 class TestLoginHelper(TestCase):
     """Test login helper methods."""
     def setUp(self):
         super(TestLoginHelper, self).setUp()
         self.request = RequestFactory()
 
-    def test_unsafe_next(self):
+    @ddt.data(
+        ("https://www.amazon.com", "text/html"),
+        ("favicon.ico", "image/*"),
+        ("https://www.test.com/test.jpg", "image/*"),
+        (settings.STATIC_URL + "dummy.png", "image/*"),
+    )
+    @ddt.unpack
+    def test_unsafe_next(self, unsafe_url, http_accept):
         """ Test unsafe next parameter """
-        unsafe_url = "https://www.amazon.com"
+        with LogCapture(LOGGER_NAME, level=logging.WARNING) as logger:
-        with LogCapture(LOGGER_NAME, level=logging.ERROR) as logger:
             req = self.request.get(reverse("login") + "?next={url}".format(url=unsafe_url))
+            req.META["HTTP_ACCEPT"] = http_accept  # pylint: disable=no-member
             get_next_url_for_login_page(req)
             logger.check(
-                (LOGGER_NAME, "ERROR", u"Unsafe redirect parameter detected: u'{url}'".format(url=unsafe_url))
+                (LOGGER_NAME, "WARNING",
+                 u"Unsafe redirect parameter detected after login page: 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"))
+        req.META["HTTP_ACCEPT"] = "text/html"  # pylint: disable=no-member
         next_page = get_next_url_for_login_page(req)
         self.assertEqual(next_page, u'/dashboard')

common/djangoapps/student/tests/test_login.py

@@ -498,7 +498,7 @@ class ExternalAuthShibTest(ModuleStoreTestCase):
         Should vary by course depending on its enrollment_domain
         """
         TARGET_URL = reverse('courseware', args=[self.course.id.to_deprecated_string()])            # pylint: disable=invalid-name
-        noshib_response = self.client.get(TARGET_URL, follow=True)
+        noshib_response = self.client.get(TARGET_URL, follow=True, HTTP_ACCEPT="text/html")
         self.assertEqual(noshib_response.redirect_chain[-1],
                          ('http://testserver/login?next={url}'.format(url=TARGET_URL), 302))
         self.assertContains(noshib_response, (u"Sign in or Register | {platform_name}"
@@ -509,7 +509,8 @@ class ExternalAuthShibTest(ModuleStoreTestCase):
         shib_response = self.client.get(**{'path': TARGET_URL_SHIB,
                                            'follow': True,
                                            'REMOTE_USER': self.extauth.external_id,
-                                           'Shib-Identity-Provider': 'https://idp.stanford.edu/'})
+                                           'Shib-Identity-Provider': 'https://idp.stanford.edu/',
+                                           'HTTP_ACCEPT': "text/html"})
         # Test that the shib-login redirect page with ?next= and the desired page are part of the redirect chain
         # The 'courseware' page actually causes a redirect itself, so it's not the end of the chain and we
         # won't test its contents

common/djangoapps/student/tests/test_login_registration_forms.py

@@ -90,7 +90,7 @@ class LoginFormTest(ThirdPartyAuthTestMixin, UrlResetMixin, SharedModuleStoreTes
     def test_courseware_redirect(self, backend_name):
         # Try to access courseware while logged out, expecting to be
         # redirected to the login page.
-        response = self.client.get(self.courseware_url, follow=True)
+        response = self.client.get(self.courseware_url, follow=True, HTTP_ACCEPT="text/html")
         self.assertRedirects(
             response,
             u"{url}?next={redirect_url}".format(
@@ -118,7 +118,7 @@ class LoginFormTest(ThirdPartyAuthTestMixin, UrlResetMixin, SharedModuleStoreTes
             ('email_opt_in', 'true'),
             ('next', '/custom/final/destination'),
         ]
-        response = self.client.get(self.url, params)
+        response = self.client.get(self.url, params, HTTP_ACCEPT="text/html")
         expected_url = _third_party_login_url(
             backend_name,
             "login",
@@ -137,7 +137,7 @@ class LoginFormTest(ThirdPartyAuthTestMixin, UrlResetMixin, SharedModuleStoreTes
         ]
 
         # Get the login page
-        response = self.client.get(self.url, params)
+        response = self.client.get(self.url, params, HTTP_ACCEPT="text/html")
 
         # Verify that the parameters are sent on to the next page correctly
         post_login_handler = _finish_auth_url(params)
@@ -194,7 +194,7 @@ class RegisterFormTest(ThirdPartyAuthTestMixin, UrlResetMixin, SharedModuleStore
             ('email_opt_in', 'true'),
             ('next', '/custom/final/destination'),
         ]
-        response = self.client.get(self.url, params)
+        response = self.client.get(self.url, params, HTTP_ACCEPT="text/html")
         expected_url = _third_party_login_url(
             backend_name,
             "register",
@@ -213,7 +213,7 @@ class RegisterFormTest(ThirdPartyAuthTestMixin, UrlResetMixin, SharedModuleStore
         ]
 
         # Get the login page
-        response = self.client.get(self.url, params)
+        response = self.client.get(self.url, params, HTTP_ACCEPT="text/html")
 
         # Verify that the parameters are sent on to the next page correctly
         post_login_handler = _finish_auth_url(params)

lms/djangoapps/student_account/test/test_views.py

@@ -336,7 +336,7 @@ class StudentAccountLoginAndRegistrationTest(ThirdPartyAuthTestMixin, UrlResetMi
         # The response should have a "Sign In" button with the URL
         # that preserves the querystring params
         with with_comprehensive_theme_context(theme):
-            response = self.client.get(reverse(url_name), params)
+            response = self.client.get(reverse(url_name), params, HTTP_ACCEPT="text/html")
 
         expected_url = '/login?{}'.format(self._finish_auth_url_param(params + [('next', '/dashboard')]))
         self.assertContains(response, expected_url)
@@ -352,7 +352,7 @@ class StudentAccountLoginAndRegistrationTest(ThirdPartyAuthTestMixin, UrlResetMi
 
         # Verify that this parameter is also preserved
         with with_comprehensive_theme_context(theme):
-            response = self.client.get(reverse(url_name), params)
+            response = self.client.get(reverse(url_name), params, HTTP_ACCEPT="text/html")
 
         expected_url = '/login?{}'.format(self._finish_auth_url_param(params))
         self.assertContains(response, expected_url)
@@ -387,11 +387,11 @@ class StudentAccountLoginAndRegistrationTest(ThirdPartyAuthTestMixin, UrlResetMi
         if current_backend is not None:
             pipeline_target = "student_account.views.third_party_auth.pipeline"
             with simulate_running_pipeline(pipeline_target, current_backend):
-                response = self.client.get(reverse(url_name), params)
+                response = self.client.get(reverse(url_name), params, HTTP_ACCEPT="text/html")
 
         # Do NOT simulate a running pipeline
         else:
-            response = self.client.get(reverse(url_name), params)
+            response = self.client.get(reverse(url_name), params, HTTP_ACCEPT="text/html")
 
         # This relies on the THIRD_PARTY_AUTH configuration in the test settings
         expected_providers = [
@@ -424,7 +424,7 @@ class StudentAccountLoginAndRegistrationTest(ThirdPartyAuthTestMixin, UrlResetMi
 
     def test_hinted_login(self):
         params = [("next", "/courses/something/?tpa_hint=oa2-google-oauth2")]
-        response = self.client.get(reverse('signin_user'), params)
+        response = self.client.get(reverse('signin_user'), params, HTTP_ACCEPT="text/html")
         self.assertContains(response, '"third_party_auth_hint": "oa2-google-oauth2"')
 
     @override_settings(SITE_NAME=settings.MICROSITE_TEST_HOSTNAME)

openedx/core/djangoapps/external_auth/tests/test_shib.py

@@ -569,7 +569,8 @@ class ShibSPTestModifiedCourseware(ModuleStoreTestCase):
                           'data': dict(params),
                           'follow': False,
                           'REMOTE_USER': 'testuser@stanford.edu',
-                          'Shib-Identity-Provider': 'https://idp.stanford.edu/'}
+                          'Shib-Identity-Provider': 'https://idp.stanford.edu/',
+                          'HTTP_ACCEPT': "text/html"}
         response = self.client.get(**request_kwargs)
         # successful login is a redirect to the URL that handles auto-enrollment
         self.assertEqual(response.status_code, 302)