From 7712bf3151e33bde56b3cbd1ec6b4cfc918c5434 Mon Sep 17 00:00:00 2001 From: Ahsan Ulhaq Date: Mon, 30 Jan 2017 18:18:55 +0500 Subject: [PATCH] edx.org/login?next= should not be able to point to an asset ECOM-6463 --- common/djangoapps/student/helpers.py | 76 ++++++++++++++++--- .../djangoapps/student/tests/test_helpers.py | 31 +++++++- common/djangoapps/student/tests/test_login.py | 5 +- .../tests/test_login_registration_forms.py | 10 +-- .../student_account/test/test_views.py | 10 +-- .../external_auth/tests/test_shib.py | 3 +- 6 files changed, 106 insertions(+), 29 deletions(-) diff --git a/common/djangoapps/student/helpers.py b/common/djangoapps/student/helpers.py index ee1d490b3d..bb7aeca853 100644 --- a/common/djangoapps/student/helpers.py +++ b/common/djangoapps/student/helpers.py @@ -2,8 +2,9 @@ from datetime import datetime import logging import urllib +import mimetypes -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,10 +15,12 @@ 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 from course_modes.models import CourseMode +from openedx.core.djangoapps.theming.helpers import get_themes # Enumeration of per-course verification statuses @@ -243,17 +246,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) - - # 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 - + redirect_to = get_redirect_to(request) if not redirect_to: try: redirect_to = reverse('dashboard') @@ -270,6 +263,65 @@ 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: + mime_type, _ = mimetypes.guess_type(redirect_to, strict=False) + if not http.is_safe_url(redirect_to): + log.warning( + u'Unsafe redirect parameter detected after login page: %(redirect_to)r', + {"redirect_to": redirect_to} + ) + redirect_to = None + elif 'text/html' not in header_accept: + log.warning( + u'Redirect to non html content %(content_type)r detected from %(user_agent)r' + u' after login page: %(redirect_to)r', + { + "redirect_to": redirect_to, "content_type": header_accept, + "user_agent": request.META.get('HTTP_USER_AGENT', '') + } + ) + redirect_to = None + elif mime_type: + log.warning( + u'Redirect to url path with specified filed type %(mime_type)r not allowed: %(redirect_to)r', + {"redirect_to": redirect_to, "mime_type": mime_type} + ) + redirect_to = None + elif settings.STATIC_URL in redirect_to: + log.warning( + u'Redirect to static content detected after login page: %(redirect_to)r', + {"redirect_to": redirect_to} + ) + redirect_to = None + else: + themes = get_themes() + for theme in themes: + if theme.theme_dir_name in redirect_to: + log.warning( + u'Redirect to theme content detected after login page: %(redirect_to)r', + {"redirect_to": redirect_to} + ) + redirect_to = None + break + + return redirect_to + + def destroy_oauth_tokens(user): """ Destroys ALL OAuth access and refresh tokens for the given user. diff --git a/common/djangoapps/student/tests/test_helpers.py b/common/djangoapps/student/tests/test_helpers.py index 687fbfd396..db23ced5d6 100644 --- a/common/djangoapps/student/tests/test_helpers.py +++ b/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,45 @@ from student.helpers import get_next_url_for_login_page LOGGER_NAME = "student.helpers" +@ddt.ddt class TestLoginHelper(TestCase): """Test login helper methods.""" + static_url = settings.STATIC_URL + def setUp(self): super(TestLoginHelper, self).setUp() self.request = RequestFactory() - def test_unsafe_next(self): + @ddt.data( + ("https://www.amazon.com", "text/html", None, + "Unsafe redirect parameter detected after login page: u'https://www.amazon.com'"), + ("favicon.ico", "image/*", "test/agent", + "Redirect to non html content 'image/*' detected from 'test/agent' after login page: u'favicon.ico'"), + ("https://www.test.com/test.jpg", "image/*", None, + "Unsafe redirect parameter detected after login page: u'https://www.test.com/test.jpg'"), + (static_url + "dummy.png", "image/*", "test/agent", + "Redirect to non html content 'image/*' detected from 'test/agent' after login page: u'" + static_url + + "dummy.png" + "'"), + ("test.png", "text/html", None, + "Redirect to url path with specified filed type 'image/png' not allowed: u'test.png'"), + (static_url + "dummy.png", "text/html", None, + "Redirect to url path with specified filed type 'image/png' not allowed: u'" + static_url + "dummy.png" + "'"), + ) + @ddt.unpack + def test_unsafe_next(self, unsafe_url, http_accept, user_agent, expected_log): """ Test unsafe next parameter """ - unsafe_url = "https://www.amazon.com" - with LogCapture(LOGGER_NAME, level=logging.ERROR) as logger: + with LogCapture(LOGGER_NAME, level=logging.WARNING) as logger: req = self.request.get(reverse("login") + "?next={url}".format(url=unsafe_url)) + req.META["HTTP_ACCEPT"] = http_accept # pylint: disable=no-member + req.META["HTTP_USER_AGENT"] = user_agent # 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", expected_log) ) 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') diff --git a/common/djangoapps/student/tests/test_login.py b/common/djangoapps/student/tests/test_login.py index 03e3e7c2a0..3eeaa02de3 100644 --- a/common/djangoapps/student/tests/test_login.py +++ b/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 diff --git a/common/djangoapps/student/tests/test_login_registration_forms.py b/common/djangoapps/student/tests/test_login_registration_forms.py index 09837fda19..5cb44a8586 100644 --- a/common/djangoapps/student/tests/test_login_registration_forms.py +++ b/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) diff --git a/lms/djangoapps/student_account/test/test_views.py b/lms/djangoapps/student_account/test/test_views.py index d91aa88bda..bdaef6c667 100644 --- a/lms/djangoapps/student_account/test/test_views.py +++ b/lms/djangoapps/student_account/test/test_views.py @@ -341,7 +341,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) @@ -357,7 +357,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) @@ -392,11 +392,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 = [ @@ -429,7 +429,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"') tpa_hint = self.hidden_enabled_provider.provider_id diff --git a/openedx/core/djangoapps/external_auth/tests/test_shib.py b/openedx/core/djangoapps/external_auth/tests/test_shib.py index 59510ff76a..75106c6cf1 100644 --- a/openedx/core/djangoapps/external_auth/tests/test_shib.py +++ b/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)