Rc/2013 12 18

cms/envs/test.py

@@ -171,4 +171,4 @@
 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

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 @@ def process_request(self, request):
         global requestcontext
         requestcontext = RequestContext(request)
         requestcontext['is_secure'] = request.is_secure()
-        requestcontext['site'] = request.get_host()
+        requestcontext['site'] = safe_get_host(request)

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 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 @@ def assertEmailUser(self, email_user, subject_template, subject_context, body_te
             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 @@ def assertReactivateEmailSent(self, email_user):
             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 @@ def assertChangeEmailSent(self, email_user):
             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 @@ def test_duplicate_email(self, email_user):
         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 @@ def test_old_email_fails(self, email_user):
         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 @@ def test_new_email_fails(self, email_user):
         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,

common/djangoapps/student/tests/tests.py

@@ -82,7 +82,7 @@ def test_nonexist_email_password_reset(self):
         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')

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

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)

lms/envs/common.py

@@ -446,6 +446,7 @@
 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'