From c13f2961037305686e65e605e92be0a20aca17d9 Mon Sep 17 00:00:00 2001 From: Peter Fogg Date: Fri, 7 Aug 2015 11:22:05 -0400 Subject: [PATCH] HTML-escape uses of course display name. --- .../contentstore/tests/test_contentstore.py | 21 ++++++++++-- cms/templates/course-create-rerun.html | 4 +-- cms/templates/index.html | 8 ++--- cms/templates/library.html | 2 +- .../xmodule/modulestore/tests/utils.py | 23 +++++++++++++ .../courseware/tests/test_course_survey.py | 14 +++++++- .../tests/views/test_instructor_dashboard.py | 13 ++++++-- .../shoppingcart/tests/test_views.py | 33 ++++++++++++++++++- .../verify_student/tests/test_views.py | 11 +++++-- .../instructor_dashboard_2/course_info.html | 2 +- lms/templates/shoppingcart/receipt.html | 7 ++-- .../registration_code_receipt.html | 6 ++-- .../registration_code_redemption.html | 6 ++-- lms/templates/survey/survey.html | 2 +- .../verify_student/pay_and_verify.html | 8 ++--- 15 files changed, 130 insertions(+), 30 deletions(-) diff --git a/cms/djangoapps/contentstore/tests/test_contentstore.py b/cms/djangoapps/contentstore/tests/test_contentstore.py index 5c23196e71..0c8b4b6337 100644 --- a/cms/djangoapps/contentstore/tests/test_contentstore.py +++ b/cms/djangoapps/contentstore/tests/test_contentstore.py @@ -36,7 +36,8 @@ from xmodule.modulestore.exceptions import ItemNotFoundError from xmodule.modulestore.inheritance import own_metadata from opaque_keys.edx.keys import UsageKey, CourseKey from opaque_keys.edx.locations import AssetLocation, CourseLocator -from xmodule.modulestore.tests.factories import CourseFactory, ItemFactory, check_mongo_calls +from xmodule.modulestore.tests.factories import CourseFactory, ItemFactory, LibraryFactory, check_mongo_calls +from xmodule.modulestore.tests.utils import XssTestMixin from xmodule.modulestore.xml_exporter import export_course_to_xml from xmodule.modulestore.xml_importer import import_course_from_xml, perform_xlint @@ -1115,7 +1116,7 @@ class MiscCourseTests(ContentStoreTestCase): @ddt.ddt -class ContentStoreTest(ContentStoreTestCase): +class ContentStoreTest(ContentStoreTestCase, XssTestMixin): """ Tests for the CMS ContentStore application. """ @@ -1405,6 +1406,22 @@ class ContentStoreTest(ContentStoreTestCase): html=True ) + def test_course_index_view_xss(self): + """Test that the index page correctly escapes course names with script + tags.""" + CourseFactory.create( + display_name='' + ) + + LibraryFactory.create(display_name='') + + resp = self.client.get_html('/home/') + for xss in ('course', 'library'): + html = ''.format( + name=xss + ) + self.assert_xss(resp, html) + def test_course_overview_view_with_course(self): """Test viewing the course overview page with an existing course""" course = CourseFactory.create() diff --git a/cms/templates/course-create-rerun.html b/cms/templates/course-create-rerun.html index 44f828192f..6affe2129f 100644 --- a/cms/templates/course-create-rerun.html +++ b/cms/templates/course-create-rerun.html @@ -40,7 +40,7 @@ from django.template.defaultfilters import escapejs

${_("You are creating a re-run from:")} ${source_course_key.org | h} ${source_course_key.course | h} ${source_course_key.run | h} - ${display_name} + ${display_name | h}

@@ -73,7 +73,7 @@ from django.template.defaultfilters import escapejs
  1. - + ${_("The public display name for the new course. (This name is often the same as the original course name.)")} diff --git a/cms/templates/index.html b/cms/templates/index.html index d8fe4389dd..92efdd0d81 100644 --- a/cms/templates/index.html +++ b/cms/templates/index.html @@ -168,7 +168,7 @@
  2. -

    ${course_info['display_name']}

    +

    ${course_info['display_name'] | h}

    @@ -210,7 +210,7 @@
  3. -

    ${course_info['display_name']}

    +

    ${course_info['display_name'] | h}

    @@ -267,7 +267,7 @@ %for course_info in sorted(courses, key=lambda s: s['display_name'].lower() if s['display_name'] is not None else ''):
  4. -

    ${course_info['display_name']}

    +

    ${course_info['display_name'] | h}

    @@ -419,7 +419,7 @@ %for library_info in sorted(libraries, key=lambda s: s['display_name'].lower() if s['display_name'] is not None else ''):
  5. -

    ${library_info['display_name']}

    +

    ${library_info['display_name'] | h}

    diff --git a/cms/templates/library.html b/cms/templates/library.html index 91a65d0d61..864c69c518 100644 --- a/cms/templates/library.html +++ b/cms/templates/library.html @@ -46,7 +46,7 @@ from django.utils.translation import ugettext as _ ${_("Content Library")}
    -

    ${context_library.display_name_with_default | h}

    +

    ${context_library.display_name_with_default}

    diff --git a/common/lib/xmodule/xmodule/modulestore/tests/utils.py b/common/lib/xmodule/xmodule/modulestore/tests/utils.py index aa8832ba10..b1c0b17e4a 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/utils.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/utils.py @@ -2,6 +2,7 @@ Helper classes and methods for running modulestore tests without Django. """ from importlib import import_module +from markupsafe import escape from opaque_keys.edx.keys import UsageKey from unittest import TestCase from xblock.fields import XBlockMixin @@ -174,3 +175,25 @@ class ProceduralCourseTestMixin(object): with self.store.bulk_operations(self.course.id, emit_signals=emit_signals): descend(self.course, ['chapter', 'sequential', 'vertical', 'problem']) + + +class XssTestMixin(object): + """ + Mixin for testing XSS vulnerabilities. + """ + + def assert_xss(self, response, xss_content): + """Assert that `xss_content` is not present in the content of + `response`, and that its escaped version is present. Uses the + same `markupsafe.escape` function as Mako templates. + + Args: + response (Response): The HTTP response + xss_content (str): The Javascript code to check for. + + Returns: + None + + """ + self.assertContains(response, escape(xss_content)) + self.assertNotContains(response, xss_content) diff --git a/lms/djangoapps/courseware/tests/test_course_survey.py b/lms/djangoapps/courseware/tests/test_course_survey.py index a4ddf7b69a..a06a77b372 100644 --- a/lms/djangoapps/courseware/tests/test_course_survey.py +++ b/lms/djangoapps/courseware/tests/test_course_survey.py @@ -11,11 +11,12 @@ from survey.models import SurveyForm from xmodule.modulestore.tests.factories import CourseFactory from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase +from xmodule.modulestore.tests.utils import XssTestMixin from courseware.tests.helpers import LoginEnrollmentTestCase @attr('shard_1') -class SurveyViewsTests(LoginEnrollmentTestCase, ModuleStoreTestCase): +class SurveyViewsTests(LoginEnrollmentTestCase, ModuleStoreTestCase, XssTestMixin): """ All tests for the views.py file """ @@ -39,6 +40,7 @@ class SurveyViewsTests(LoginEnrollmentTestCase, ModuleStoreTestCase): }) self.course = CourseFactory.create( + display_name='', course_survey_required=True, course_survey_name=self.test_survey_name ) @@ -172,3 +174,13 @@ class SurveyViewsTests(LoginEnrollmentTestCase, ModuleStoreTestCase): resp, reverse('info', kwargs={'course_id': unicode(self.course_without_survey.id)}) ) + + def test_survey_xss(self): + """Test that course display names are correctly HTML-escaped.""" + response = self.client.get( + reverse( + 'course_survey', + kwargs={'course_id': unicode(self.course.id)} + ) + ) + self.assert_xss(response, '') diff --git a/lms/djangoapps/instructor/tests/views/test_instructor_dashboard.py b/lms/djangoapps/instructor/tests/views/test_instructor_dashboard.py index 1b4af21ef8..297e3bd774 100644 --- a/lms/djangoapps/instructor/tests/views/test_instructor_dashboard.py +++ b/lms/djangoapps/instructor/tests/views/test_instructor_dashboard.py @@ -15,6 +15,7 @@ from courseware.tests.helpers import LoginEnrollmentTestCase from student.tests.factories import AdminFactory from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase +from xmodule.modulestore.tests.utils import XssTestMixin from xmodule.modulestore.tests.factories import CourseFactory from shoppingcart.models import PaidCourseRegistration, Order, CourseRegCodeItem from course_modes.models import CourseMode @@ -23,7 +24,7 @@ from student.models import CourseEnrollment @ddt.ddt -class TestInstructorDashboard(ModuleStoreTestCase, LoginEnrollmentTestCase): +class TestInstructorDashboard(ModuleStoreTestCase, LoginEnrollmentTestCase, XssTestMixin): """ Tests for the instructor dashboard (not legacy). """ @@ -34,7 +35,8 @@ class TestInstructorDashboard(ModuleStoreTestCase, LoginEnrollmentTestCase): """ super(TestInstructorDashboard, self).setUp() self.course = CourseFactory.create( - grading_policy={"GRADE_CUTOFFS": {"A": 0.75, "B": 0.63, "C": 0.57, "D": 0.5}} + grading_policy={"GRADE_CUTOFFS": {"A": 0.75, "B": 0.63, "C": 0.57, "D": 0.5}}, + display_name='' ) self.course_mode = CourseMode(course_id=self.course.id, @@ -87,6 +89,13 @@ class TestInstructorDashboard(ModuleStoreTestCase, LoginEnrollmentTestCase): response = self.client.get(self.url) self.assertTrue('${amount}'.format(amount=total_amount) in response.content) + def test_course_name_xss(self): + """Test that the instructor dashboard correctly escapes course names + with script tags. + """ + response = self.client.get(self.url) + self.assert_xss(response, '') + @override_settings(PAID_COURSE_REGISTRATION_CURRENCY=['PKR', 'Rs']) def test_override_currency_settings_in_the_html_response(self): """ diff --git a/lms/djangoapps/shoppingcart/tests/test_views.py b/lms/djangoapps/shoppingcart/tests/test_views.py index 484d3fd4e9..bf1d9448af 100644 --- a/lms/djangoapps/shoppingcart/tests/test_views.py +++ b/lms/djangoapps/shoppingcart/tests/test_views.py @@ -26,6 +26,7 @@ import ddt from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase from xmodule.modulestore.tests.factories import CourseFactory +from xmodule.modulestore.tests.utils import XssTestMixin from student.roles import CourseSalesAdminRole from util.date_utils import get_default_time_display from util.testing import UrlResetMixin @@ -65,7 +66,7 @@ postpay_mock = Mock() @patch.dict('django.conf.settings.FEATURES', {'ENABLE_PAID_COURSE_REGISTRATION': True}) @ddt.ddt -class ShoppingCartViewsTests(ModuleStoreTestCase): +class ShoppingCartViewsTests(ModuleStoreTestCase, XssTestMixin): def setUp(self): super(ShoppingCartViewsTests, self).setUp() @@ -98,7 +99,12 @@ class ShoppingCartViewsTests(ModuleStoreTestCase): verified_course = CourseFactory.create(org='org', number='test', display_name='Test Course') self.verified_course_key = verified_course.id + + xss_course = CourseFactory.create(org='xssorg', number='test', display_name='') + self.xss_course_key = xss_course.id + self.cart = Order.get_cart_for_user(self.user) + self.addCleanup(patcher.stop) self.now = datetime.now(pytz.UTC) @@ -884,6 +890,31 @@ class ShoppingCartViewsTests(ModuleStoreTestCase): 'course_key': unicode(self.verified_course_key) }) + def test_show_receipt_xss(self): + CertificateItem.add_to_order(self.cart, self.xss_course_key, self.cost, 'honor') + self.cart.purchase() + + self.login_user() + url = reverse('shoppingcart.views.show_receipt', args=[self.cart.id]) + resp = self.client.get(url) + self.assert_xss(resp, '') + + @patch('shoppingcart.views.render_to_response', render_mock) + def test_reg_code_xss(self): + self.add_reg_code(self.xss_course_key) + + # One courses in user shopping cart + self.add_course_to_user_cart(self.xss_course_key) + self.assertEquals(self.cart.orderitem_set.count(), 1) + + post_response = self.client.post(reverse('shoppingcart.views.use_code'), {'code': self.reg_code}) + self.assertEqual(post_response.status_code, 200) + + redeem_url = reverse('register_code_redemption', args=[self.reg_code]) + redeem_response = self.client.get(redeem_url) + + self.assert_xss(redeem_response, '') + def test_show_receipt_json_multiple_items(self): # Two different item types PaidCourseRegistration.add_to_order(self.cart, self.course_key) diff --git a/lms/djangoapps/verify_student/tests/test_views.py b/lms/djangoapps/verify_student/tests/test_views.py index e96ad28349..4e977f2cdb 100644 --- a/lms/djangoapps/verify_student/tests/test_views.py +++ b/lms/djangoapps/verify_student/tests/test_views.py @@ -51,6 +51,7 @@ from verify_student.models import ( ) from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase from xmodule.modulestore.tests.factories import CourseFactory, ItemFactory +from xmodule.modulestore.tests.utils import XssTestMixin from xmodule.modulestore.django import modulestore from xmodule.modulestore import ModuleStoreEnum from xmodule.modulestore.tests.factories import check_mongo_calls @@ -81,7 +82,7 @@ class StartView(TestCase): @ddt.ddt -class TestPayAndVerifyView(UrlResetMixin, ModuleStoreTestCase): +class TestPayAndVerifyView(UrlResetMixin, ModuleStoreTestCase, XssTestMixin): """ Tests for the payment and verification flow views. """ @@ -258,6 +259,7 @@ class TestPayAndVerifyView(UrlResetMixin, ModuleStoreTestCase): response = self._get_page('verify_student_verify_now', course.id) self._assert_messaging(response, PayAndVerifyView.VERIFY_NOW_MSG) + self.assert_xss(response, '') # Expect that *all* steps are displayed, # but we start after the payment step (because it's already completed). @@ -339,6 +341,8 @@ class TestPayAndVerifyView(UrlResetMixin, ModuleStoreTestCase): self._assert_messaging(response, PayAndVerifyView.PAYMENT_CONFIRMATION_MSG) + self.assert_xss(response, '') + # Expect that *all* steps are displayed, # but we start at the payment confirmation step self._assert_steps_displayed( @@ -371,6 +375,8 @@ class TestPayAndVerifyView(UrlResetMixin, ModuleStoreTestCase): self._assert_messaging(response, PayAndVerifyView.FIRST_TIME_VERIFY_MSG) + self.assert_xss(response, '') + # Expect that *all* steps are displayed, # but we start on the first verify step self._assert_steps_displayed( @@ -456,6 +462,7 @@ class TestPayAndVerifyView(UrlResetMixin, ModuleStoreTestCase): PayAndVerifyView.WEBCAM_REQ, ]) self._assert_upgrade_session_flag(True) + self.assert_xss(response, '') def test_upgrade_already_verified(self): course = self._create_course("verified") @@ -724,7 +731,7 @@ class TestPayAndVerifyView(UrlResetMixin, ModuleStoreTestCase): def _create_course(self, *course_modes, **kwargs): """Create a new course with the specified course modes. """ - course = CourseFactory.create() + course = CourseFactory.create(display_name='') if kwargs.get('course_start'): course.start = kwargs.get('course_start') diff --git a/lms/templates/instructor/instructor_dashboard_2/course_info.html b/lms/templates/instructor/instructor_dashboard_2/course_info.html index 7df0a7fb35..fea2d19334 100644 --- a/lms/templates/instructor/instructor_dashboard_2/course_info.html +++ b/lms/templates/instructor/instructor_dashboard_2/course_info.html @@ -55,7 +55,7 @@
  6. - ${ section_data['course_display_name'] } + ${ section_data['course_display_name'] | h}
  7. diff --git a/lms/templates/shoppingcart/receipt.html b/lms/templates/shoppingcart/receipt.html index 85c4022ffb..3940af4c63 100644 --- a/lms/templates/shoppingcart/receipt.html +++ b/lms/templates/shoppingcart/receipt.html @@ -4,6 +4,7 @@ from django.utils.translation import ugettext as _ from django.utils.translation import ungettext from django.core.urlresolvers import reverse from courseware.courses import course_image_url, get_course_about_section, get_course_by_id +from markupsafe import escape from microsite_configuration import microsite %> @@ -35,7 +36,7 @@ from microsite_configuration import microsite ${_(u"You have successfully been enrolled for {course_names}. " u"The following receipt has been emailed to {receipient_emails}").format( course_names=u"{course_names}".format( - course_names=appended_course_names + course_names=escape(appended_course_names) ), receipient_emails=u"{receipient_emails}".format( receipient_emails=appended_recipient_emails @@ -50,7 +51,7 @@ from microsite_configuration import microsite ).format( number=total_registration_codes, course_names=u"{course_names}".format( - course_names=appended_course_names + course_names=escape(appended_course_names) ) )} ${_("The following receipt has been emailed to {receipient_emails}").format( @@ -297,7 +298,7 @@ from microsite_configuration import microsite

    ${_('Registration for:')} - ${ course.display_name } + ${ course.display_name | h }

    diff --git a/lms/templates/shoppingcart/registration_code_receipt.html b/lms/templates/shoppingcart/registration_code_receipt.html index 557873c4c3..b2f37b452d 100644 --- a/lms/templates/shoppingcart/registration_code_receipt.html +++ b/lms/templates/shoppingcart/registration_code_receipt.html @@ -33,7 +33,7 @@ from courseware.courses import course_image_url, get_course_about_section

    - ${_("{course_name}").format(course_name=course.display_name)} + ${_("{course_name}").format(course_name=course.display_name) | h} ${_("{start_date} - {end_date}").format( start_date=course.start_datetime_text(), @@ -55,7 +55,7 @@ from courseware.courses import course_image_url, get_course_about_section )} % elif redemption_success: ${_("You have successfully enrolled in {course_name}." - " This course has now been added to your dashboard.").format(course_name=course.display_name)} + " This course has now been added to your dashboard.").format(course_name=course.display_name) | h} % elif registered_for_course: ${_("You're already enrolled for this course." " Visit your {link_start}dashboard{link_end} to see the course." @@ -72,7 +72,7 @@ from courseware.courses import course_image_url, get_course_about_section % else: ${_("You're about to activate an enrollment code for {course_name} by {site_name}. " "This code can only be used one time, so you should only activate this code if you're its intended" - " recipient.").format(course_name=course.display_name, site_name=site_name)} + " recipient.").format(course_name=course.display_name, site_name=site_name) | h} % endif

    diff --git a/lms/templates/shoppingcart/registration_code_redemption.html b/lms/templates/shoppingcart/registration_code_redemption.html index 94227732b2..3c91e41ce3 100644 --- a/lms/templates/shoppingcart/registration_code_redemption.html +++ b/lms/templates/shoppingcart/registration_code_redemption.html @@ -33,7 +33,7 @@ from courseware.courses import course_image_url, get_course_about_section

    - ${course.display_name} + ${course.display_name | h} ${course.start_datetime_text()} - @@ -60,7 +60,7 @@ from courseware.courses import course_image_url, get_course_about_section "This course has now been added to your dashboard." ).format( course_name=course.display_name, - )} + ) | h} % elif registered_for_course: ${_( "You're already enrolled for this course. " @@ -80,7 +80,7 @@ from courseware.courses import course_image_url, get_course_about_section ).format( course_name=course.display_name, site_name=site_name, - )} + ) | h} % endif

    diff --git a/lms/templates/survey/survey.html b/lms/templates/survey/survey.html index 6ffd8986ba..c71b945178 100644 --- a/lms/templates/survey/survey.html +++ b/lms/templates/survey/survey.html @@ -24,7 +24,7 @@ from django.utils import html

    ${course.display_org_with_default} ${course.display_number_with_default} - ${course.display_name} + ${course.display_name | h}

    ${_("Pre-Course Survey")}

    diff --git a/lms/templates/verify_student/pay_and_verify.html b/lms/templates/verify_student/pay_and_verify.html index 79361d1b72..e483591a5a 100644 --- a/lms/templates/verify_student/pay_and_verify.html +++ b/lms/templates/verify_student/pay_and_verify.html @@ -10,13 +10,13 @@ from verify_student.views import PayAndVerifyView <%block name="pagetitle"> % if message_key == PayAndVerifyView.UPGRADE_MSG: - ${_("Upgrade Your Enrollment For {course_name}.").format(course_name=course.display_name)} + ${_("Upgrade Your Enrollment For {course_name}.").format(course_name=course.display_name) | h} % elif message_key == PayAndVerifyView.PAYMENT_CONFIRMATION_MSG: - ${_("Receipt For {course_name}").format(course_name=course.display_name)} + ${_("Receipt For {course_name}").format(course_name=course.display_name) | h} % elif message_key in [PayAndVerifyView.VERIFY_NOW_MSG, PayAndVerifyView.VERIFY_LATER_MSG]: - ${_("Verify For {course_name}").format(course_name=course.display_name)} + ${_("Verify For {course_name}").format(course_name=course.display_name) | h} % else: - ${_("Enroll In {course_name}").format(course_name=course.display_name)} + ${_("Enroll In {course_name}").format(course_name=course.display_name) | h} % endif