diff --git a/cms/envs/test.py b/cms/envs/test.py index 68bc3739ad..e27b8420bc 100644 --- a/cms/envs/test.py +++ b/cms/envs/test.py @@ -182,4 +182,4 @@ FEATURES['STUDIO_NPS_SURVEY'] = False FEATURES['ENABLE_SERVICE_STATUS'] = True # This is to disable a test under the common directory that will not pass when run under CMS -FEATURES['DISABLE_PASSWORD_RESET_EMAIL_TEST'] = True +FEATURES['DISABLE_RESET_EMAIL_TEST'] = True diff --git a/common/djangoapps/edxmako/middleware.py b/common/djangoapps/edxmako/middleware.py index 778322c122..27b129f571 100644 --- a/common/djangoapps/edxmako/middleware.py +++ b/common/djangoapps/edxmako/middleware.py @@ -13,6 +13,7 @@ # limitations under the License. from django.template import RequestContext +from util.request import safe_get_host requestcontext = None @@ -22,4 +23,4 @@ class MakoMiddleware(object): global requestcontext requestcontext = RequestContext(request) requestcontext['is_secure'] = request.is_secure() - requestcontext['site'] = request.get_host() + requestcontext['site'] = safe_get_host(request) diff --git a/common/djangoapps/student/tests/test_email.py b/common/djangoapps/student/tests/test_email.py index c3e4fd0484..ce680d18ec 100644 --- a/common/djangoapps/student/tests/test_email.py +++ b/common/djangoapps/student/tests/test_email.py @@ -1,5 +1,6 @@ import json import django.db +import unittest from student.tests.factories import UserFactory, RegistrationFactory, PendingEmailChangeFactory from student.views import reactivation_email_for_user, change_email_request, confirm_email_change @@ -11,6 +12,9 @@ from mock import Mock, patch from django.http import Http404, HttpResponse from django.conf import settings from nose.plugins.skip import SkipTest +from edxmako.shortcuts import render_to_string +from util.request import safe_get_host +from textwrap import dedent class TestException(Exception): @@ -50,6 +54,11 @@ class EmailTestMixin(object): settings.DEFAULT_FROM_EMAIL ) + def append_allowed_hosts(self, hostname): + """ Append hostname to settings.ALLOWED_HOSTS """ + settings.ALLOWED_HOSTS.append(hostname) + self.addCleanup(settings.ALLOWED_HOSTS.pop) + @patch('student.views.render_to_string', Mock(side_effect=mock_render_to_string, autospec=True)) @patch('django.contrib.auth.models.User.email_user') @@ -83,6 +92,16 @@ class ReactivationEmailTests(EmailTestMixin, TestCase): context ) + # Thorough tests for safe_get_host are elsewhere; here we just want a quick URL sanity check + request = RequestFactory().post('unused_url') + request.META['HTTP_HOST'] = "aGenericValidHostName" + self.append_allowed_hosts("aGenericValidHostName") + + body = render_to_string('emails/activation_email.txt', context) + host = safe_get_host(request) + + self.assertIn(host, body) + def test_reactivation_email_failure(self, email_user): self.user.email_user.side_effect = Exception response_data = self.reactivation_email(self.user) @@ -227,6 +246,16 @@ class EmailChangeConfirmationTests(EmailTestMixin, TransactionTestCase): context ) + # Thorough tests for safe_get_host are elsewhere; here we just want a quick URL sanity check + request = RequestFactory().post('unused_url') + request.META['HTTP_HOST'] = "aGenericValidHostName" + self.append_allowed_hosts("aGenericValidHostName") + + body = render_to_string('emails/confirm_email_change.txt', context) + url = safe_get_host(request) + + self.assertIn(url, body) + def test_not_pending(self, email_user): self.key = 'not_a_key' self.check_confirm_email_change('invalid_email_key.html', {}) @@ -237,6 +266,9 @@ class EmailChangeConfirmationTests(EmailTestMixin, TransactionTestCase): self.check_confirm_email_change('email_exists.html', {}) self.assertFailedBeforeEmailing(email_user) + @unittest.skipIf(settings.FEATURES.get('DISABLE_RESET_EMAIL_TEST', False), + dedent("""Skipping Test because CMS has not provided necessary templates for email reset. + If LMS tests print this message, that needs to be fixed.""")) def test_old_email_fails(self, email_user): email_user.side_effect = [Exception, None] self.check_confirm_email_change('email_change_failed.html', { @@ -245,6 +277,9 @@ class EmailChangeConfirmationTests(EmailTestMixin, TransactionTestCase): self.assertRolledBack() self.assertChangeEmailSent(email_user) + @unittest.skipIf(settings.FEATURES.get('DISABLE_RESET_EMAIL_TEST', False), + dedent("""Skipping Test because CMS has not provided necessary templates for email reset. + If LMS tests print this message, that needs to be fixed.""")) def test_new_email_fails(self, email_user): email_user.side_effect = [None, Exception] self.check_confirm_email_change('email_change_failed.html', { @@ -253,6 +288,9 @@ class EmailChangeConfirmationTests(EmailTestMixin, TransactionTestCase): self.assertRolledBack() self.assertChangeEmailSent(email_user) + @unittest.skipIf(settings.FEATURES.get('DISABLE_RESET_EMAIL_TEST', False), + dedent("""Skipping Test because CMS has not provided necessary templates for email reset. + If LMS tests print this message, that needs to be fixed.""")) def test_successful_email_change(self, email_user): self.check_confirm_email_change('email_change_successful.html', { 'old_email': self.user.email, diff --git a/common/djangoapps/student/tests/tests.py b/common/djangoapps/student/tests/tests.py index f2a9593123..ea16b3f143 100644 --- a/common/djangoapps/student/tests/tests.py +++ b/common/djangoapps/student/tests/tests.py @@ -82,7 +82,7 @@ class ResetPasswordTests(TestCase): self.assertEquals(bad_email_resp.content, json.dumps({'success': True, 'value': "('registration/password_reset_done.html', [])"})) - @unittest.skipUnless(not settings.FEATURES.get('DISABLE_PASSWORD_RESET_EMAIL_TEST', False), + @unittest.skipIf(settings.FEATURES.get('DISABLE_RESET_EMAIL_TEST', False), dedent("""Skipping Test because CMS has not provided necessary templates for password reset. If LMS tests print this message, that needs to be fixed.""")) @patch('django.core.mail.send_mail') diff --git a/common/djangoapps/util/request.py b/common/djangoapps/util/request.py new file mode 100644 index 0000000000..0950fa3b42 --- /dev/null +++ b/common/djangoapps/util/request.py @@ -0,0 +1,17 @@ +""" Utility functions related to HTTP requests """ +from django.conf import settings + + +def safe_get_host(request): + """ + Get the host name for this request, as safely as possible. + + If ALLOWED_HOSTS is properly set, this calls request.get_host; + otherwise, this returns whatever settings.SITE_NAME is set to. + + This ensures we will never accept an untrusted value of get_host() + """ + if isinstance(settings.ALLOWED_HOSTS, (list, tuple)) and '*' not in settings.ALLOWED_HOSTS: + return request.get_host() + else: + return settings.SITE_NAME diff --git a/common/djangoapps/util/tests/test_request.py b/common/djangoapps/util/tests/test_request.py new file mode 100644 index 0000000000..76bd9b527f --- /dev/null +++ b/common/djangoapps/util/tests/test_request.py @@ -0,0 +1,39 @@ +from django.test.client import RequestFactory +from django.conf import settings +from util.request import safe_get_host +from django.core.exceptions import SuspiciousOperation +import unittest + + +class ResponseTestCase(unittest.TestCase): + """ Tests for response-related utility functions """ + def setUp(self): + self.old_site_name = settings.SITE_NAME + self.old_allowed_hosts = settings.ALLOWED_HOSTS + + def tearDown(self): + settings.SITE_NAME = self.old_site_name + settings.ALLOWED_HOSTS = self.old_allowed_hosts + + def test_safe_get_host(self): + """ Tests that the safe_get_host function returns the desired host """ + settings.SITE_NAME = 'siteName.com' + factory = RequestFactory() + request = factory.request() + request.META['HTTP_HOST'] = 'www.userProvidedHost.com' + # If ALLOWED_HOSTS is not set properly, safe_get_host should return SITE_NAME + settings.ALLOWED_HOSTS = None + self.assertEqual(safe_get_host(request), "siteName.com") + settings.ALLOWED_HOSTS = ["*"] + self.assertEqual(safe_get_host(request), "siteName.com") + settings.ALLOWED_HOSTS = ["foo.com", "*"] + self.assertEqual(safe_get_host(request), "siteName.com") + + # If ALLOWED_HOSTS is set properly, and the host is valid, we just return the user-provided host + settings.ALLOWED_HOSTS = [request.META['HTTP_HOST']] + self.assertEqual(safe_get_host(request), request.META['HTTP_HOST']) + + # If ALLOWED_HOSTS is set properly but the host is invalid, we should get a SuspiciousOperation + settings.ALLOWED_HOSTS = ["the_valid_website.com"] + with self.assertRaises(SuspiciousOperation): + safe_get_host(request) diff --git a/lms/envs/common.py b/lms/envs/common.py index e827d75313..e8dc187401 100644 --- a/lms/envs/common.py +++ b/lms/envs/common.py @@ -450,6 +450,7 @@ SITE_NAME = "edx.org" HTTPS = 'on' ROOT_URLCONF = 'lms.urls' IGNORABLE_404_ENDS = ('favicon.ico') +# NOTE: Please set ALLOWED_HOSTS to some sane value, as we do not allow the default '*' # Platform Email EMAIL_BACKEND = 'django.core.mail.backends.console.EmailBackend'