From cc65dff72866514e866b69f330b34179ed350180 Mon Sep 17 00:00:00 2001 From: Ahsan Ulhaq Date: Thu, 3 Aug 2017 22:50:28 +0500 Subject: [PATCH 1/2] Host poisoning vulnerability fix LEARNER-2172 --- common/djangoapps/student/forms.py | 12 ++---- .../student/tests/test_reset_password.py | 39 ++++++++----------- common/djangoapps/student/views.py | 3 +- lms/djangoapps/student_account/views.py | 2 +- .../core/djangoapps/user_api/accounts/api.py | 3 +- .../user_api/accounts/tests/test_api.py | 7 ++-- 6 files changed, 26 insertions(+), 40 deletions(-) diff --git a/common/djangoapps/student/forms.py b/common/djangoapps/student/forms.py index 89f802c28d..92d17ffa74 100644 --- a/common/djangoapps/student/forms.py +++ b/common/djangoapps/student/forms.py @@ -49,7 +49,6 @@ class PasswordResetFormNoActive(PasswordResetForm): def save( self, - domain_override=None, subject_template_name='emails/password_reset_subject.txt', email_template_name='registration/password_reset_email.html', use_https=False, @@ -65,13 +64,10 @@ class PasswordResetFormNoActive(PasswordResetForm): # django.contrib.auth.forms.PasswordResetForm directly, which has this import in this place. from django.core.mail import send_mail for user in self.users_cache: - if not domain_override: - site_name = configuration_helpers.get_value( - 'SITE_NAME', - settings.SITE_NAME - ) - else: - site_name = domain_override + site_name = configuration_helpers.get_value( + 'SITE_NAME', + settings.SITE_NAME + ) context = { 'email': user.email, 'site_name': site_name, diff --git a/common/djangoapps/student/tests/test_reset_password.py b/common/djangoapps/student/tests/test_reset_password.py index ff69082c3e..f30aa44431 100644 --- a/common/djangoapps/student/tests/test_reset_password.py +++ b/common/djangoapps/student/tests/test_reset_password.py @@ -177,38 +177,31 @@ class ResetPasswordTests(EventTestMixin, CacheIsolationTestCase): @patch('django.core.mail.send_mail') @ddt.data(('Crazy Awesome Site', 'Crazy Awesome Site'), (None, 'edX')) @ddt.unpack - def test_reset_password_email_domain(self, domain_override, platform_name, send_email): + def test_reset_password_email_site(self, site_name, platform_name, send_email): """ Tests that the right url domain and platform name is included in the reset password email """ with patch("django.conf.settings.PLATFORM_NAME", platform_name): - req = self.request_factory.post( - '/password_reset/', {'email': self.user.email} - ) - req.is_secure = Mock(return_value=True) - req.get_host = Mock(return_value=domain_override) - req.user = self.user - password_reset(req) - _, msg, _, _ = send_email.call_args[0] + with patch("django.conf.settings.SITE_NAME", site_name): + req = self.request_factory.post( + '/password_reset/', {'email': self.user.email} + ) + req.user = self.user + password_reset(req) + _, msg, _, _ = send_email.call_args[0] - reset_intro_msg = "you requested a password reset for your user account at {}".format(platform_name) - self.assertIn(reset_intro_msg, msg) + reset_msg = "you requested a password reset for your user account at {}" + reset_msg = reset_msg.format(site_name) - reset_link = "https://{}/" - if domain_override: - reset_link = reset_link.format(domain_override) - else: - reset_link = reset_link.format(settings.SITE_NAME) + self.assertIn(reset_msg, msg) - self.assertIn(reset_link, msg) + sign_off = "The {} Team".format(platform_name) + self.assertIn(sign_off, msg) - sign_off = "The {} Team".format(platform_name) - self.assertIn(sign_off, msg) - - self.assert_event_emitted( - SETTING_CHANGE_INITIATED, user_id=self.user.id, setting=u'password', old=None, new=None - ) + self.assert_event_emitted( + SETTING_CHANGE_INITIATED, user_id=self.user.id, setting=u'password', old=None, new=None + ) @unittest.skipUnless(settings.ROOT_URLCONF == 'lms.urls', "Test only valid in LMS") @patch("openedx.core.djangoapps.site_configuration.helpers.get_value", fake_get_value) diff --git a/common/djangoapps/student/views.py b/common/djangoapps/student/views.py index 5166592aad..2c857443f5 100644 --- a/common/djangoapps/student/views.py +++ b/common/djangoapps/student/views.py @@ -2436,8 +2436,7 @@ def password_reset(request): if form.is_valid(): form.save(use_https=request.is_secure(), from_email=configuration_helpers.get_value('email_from_address', settings.DEFAULT_FROM_EMAIL), - request=request, - domain_override=request.get_host()) + request=request) # When password change is complete, a "edx.user.settings.changed" event will be emitted. # But because changing the password is multi-step, we also emit an event here so that we can # track where the request was initiated. diff --git a/lms/djangoapps/student_account/views.py b/lms/djangoapps/student_account/views.py index 018c33752a..4825f2d2d2 100644 --- a/lms/djangoapps/student_account/views.py +++ b/lms/djangoapps/student_account/views.py @@ -198,7 +198,7 @@ def password_change_request_handler(request): if email: try: - request_password_change(email, request.get_host(), request.is_secure()) + request_password_change(email, request.is_secure()) user = user if user.is_authenticated() else User.objects.get(email=email) destroy_oauth_tokens(user) except UserNotFound: diff --git a/openedx/core/djangoapps/user_api/accounts/api.py b/openedx/core/djangoapps/user_api/accounts/api.py index 7748eb09cb..264106e4dd 100644 --- a/openedx/core/djangoapps/user_api/accounts/api.py +++ b/openedx/core/djangoapps/user_api/accounts/api.py @@ -366,7 +366,7 @@ def activate_account(activation_key): @helpers.intercept_errors(errors.UserAPIInternalError, ignore_errors=[errors.UserAPIRequestError]) -def request_password_change(email, orig_host, is_secure): +def request_password_change(email, is_secure): """Email a single-use link for performing a password reset. Users must confirm the password change before we update their information. @@ -395,7 +395,6 @@ def request_password_change(email, orig_host, is_secure): # and email it to the user. form.save( from_email=configuration_helpers.get_value('email_from_address', settings.DEFAULT_FROM_EMAIL), - domain_override=orig_host, use_https=is_secure ) else: diff --git a/openedx/core/djangoapps/user_api/accounts/tests/test_api.py b/openedx/core/djangoapps/user_api/accounts/tests/test_api.py index 5df16efb86..008ff705eb 100644 --- a/openedx/core/djangoapps/user_api/accounts/tests/test_api.py +++ b/openedx/core/djangoapps/user_api/accounts/tests/test_api.py @@ -320,7 +320,6 @@ class AccountCreationActivationAndPasswordChangeTest(TestCase): PASSWORD = u'ṕáśśẃőŕd' EMAIL = u'frank+underwood@example.com' - ORIG_HOST = 'example.com' IS_SECURE = False @skip_unless_lms @@ -390,7 +389,7 @@ class AccountCreationActivationAndPasswordChangeTest(TestCase): activate_account(activation_key) # Request a password change - request_password_change(self.EMAIL, self.ORIG_HOST, self.IS_SECURE) + request_password_change(self.EMAIL, self.IS_SECURE) # Verify that one email message has been sent self.assertEqual(len(mail.outbox), 1) @@ -404,7 +403,7 @@ class AccountCreationActivationAndPasswordChangeTest(TestCase): @skip_unless_lms def test_request_password_change_invalid_user(self): with self.assertRaises(UserNotFound): - request_password_change(self.EMAIL, self.ORIG_HOST, self.IS_SECURE) + request_password_change(self.EMAIL, self.IS_SECURE) # Verify that no email messages have been sent self.assertEqual(len(mail.outbox), 0) @@ -414,7 +413,7 @@ class AccountCreationActivationAndPasswordChangeTest(TestCase): # Create an account, but do not activate it create_account(self.USERNAME, self.PASSWORD, self.EMAIL) - request_password_change(self.EMAIL, self.ORIG_HOST, self.IS_SECURE) + request_password_change(self.EMAIL, self.IS_SECURE) # Verify that the activation email was still sent self.assertEqual(len(mail.outbox), 1) From 0ad7084df8a86bd72cc51f9205fc6cd2d10af07d Mon Sep 17 00:00:00 2001 From: Ahsan Ulhaq Date: Fri, 4 Aug 2017 22:09:14 +0500 Subject: [PATCH 2/2] fix python test --- common/djangoapps/student/tests/test_reset_password.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/common/djangoapps/student/tests/test_reset_password.py b/common/djangoapps/student/tests/test_reset_password.py index f30aa44431..61b2488db3 100644 --- a/common/djangoapps/student/tests/test_reset_password.py +++ b/common/djangoapps/student/tests/test_reset_password.py @@ -175,7 +175,7 @@ class ResetPasswordTests(EventTestMixin, CacheIsolationTestCase): @unittest.skipUnless(settings.ROOT_URLCONF == 'lms.urls', "Test only valid in LMS") @patch('django.core.mail.send_mail') - @ddt.data(('Crazy Awesome Site', 'Crazy Awesome Site'), (None, 'edX')) + @ddt.data(('Crazy Awesome Site', 'Crazy Awesome Site'), ('edX', 'edX')) @ddt.unpack def test_reset_password_email_site(self, site_name, platform_name, send_email): """