Add template escaping in forums, progress page and course container TNL-2395, TNL-2394, TNL-2393

cms/djangoapps/contentstore/tests/test_contentstore.py

@@ -681,6 +681,16 @@ class MiscCourseTests(ContentStoreTestCase):
         for expected in expected_types:
             self.assertIn(expected, resp.content)
 
+    @ddt.data("<script>alert(1)</script>", "alert('hi')", "</script><script>alert(1)</script>")
+    def test_container_handler_xss_prevent(self, malicious_code):
+        """
+        Test that XSS attack is prevented
+        """
+        resp = self.client.get_html(get_url('container_handler', self.vert_loc) + '?action=' + malicious_code)
+        self.assertEqual(resp.status_code, 200)
+        # Test that malicious code does not appear in html
+        self.assertNotIn(malicious_code, resp.content)
+
     @patch('django.conf.settings.DEPRECATED_ADVANCED_COMPONENT_TYPES', [])
     def test_advanced_components_in_edit_unit(self):
         # This could be made better, but for now let's just assert that we see the advanced modules mentioned in the page

cms/templates/container.html

@@ -13,28 +13,28 @@ import json
 from contentstore.views.helpers import xblock_studio_url, xblock_type_display_name
 from django.utils.translation import ugettext as _
 %>
-<%block name="title">${xblock.display_name_with_default} ${xblock_type_display_name(xblock)}</%block>
+<%block name="title">${xblock.display_name_with_default} ${xblock_type_display_name(xblock) | h}</%block>
 <%block name="bodyclass">is-signedin course container view-container</%block>
 
 <%namespace name='static' file='static_content.html'/>
 
 <%block name="header_extras">
 % for template_name in templates:
-<script type="text/template" id="${template_name}-tpl">
+<script type="text/template" id="${template_name | h}-tpl">
     <%static:include path="js/${template_name}.underscore" />
 </script>
 % endfor
 <script type="text/template" id="image-modal-tpl">
     <%static:include path="common/templates/image-modal.underscore" />
 </script>
-<link rel="stylesheet" type="text/css" href="${static.url('js/vendor/timepicker/jquery.timepicker.css')}" />
+<link rel="stylesheet" type="text/css" href="${static.url('js/vendor/timepicker/jquery.timepicker.css') | h}" />
 </%block>
 
 <%block name="requirejs">
     require(["js/factories/container"], function(ContainerFactory) {
         ContainerFactory(
             ${component_templates | n}, ${json.dumps(xblock_info) | n},
-            "${action}",
+            "${action | h}",
             {
                 isUnitPage: ${json.dumps(is_unit_page)},
                 canEdit: true
@@ -55,7 +55,7 @@ from django.utils.translation import ugettext as _
                     ancestor_url = xblock_studio_url(ancestor)
                     %>
                     % if ancestor_url:
-                        <a href="${ancestor_url}" class="navigation-item navigation-link navigation-parent">${ancestor.display_name_with_default | h}</a>
+                        <a href="${ancestor_url | h}" class="navigation-item navigation-link navigation-parent">${ancestor.display_name_with_default | h}</a>
                     % else:
                         <span class="navigation-item navigation-parent">${ancestor.display_name_with_default | h}</span>
                     % endif
@@ -72,12 +72,12 @@ from django.utils.translation import ugettext as _
             <ul>
                 % if is_unit_page:
                     <li class="action-item action-view nav-item">
-                        <a href="${published_preview_link}" class="button button-view action-button is-disabled" aria-disabled="true" rel="external" title="${_('Open the courseware in the LMS')}">
+                        <a href="${published_preview_link | h}" class="button button-view action-button is-disabled" aria-disabled="true" rel="external" title="${_('Open the courseware in the LMS')}">
                             <span class="action-button-text">${_("View Live Version")}</span>
                         </a>
                     </li>
                     <li class="action-item action-preview nav-item">
-                        <a href="${draft_preview_link}" class="button button-preview action-button" rel="external" title="${_('Preview the courseware in the LMS')}">
+                        <a href="${draft_preview_link | h}" class="button button-preview action-button" rel="external" title="${_('Preview the courseware in the LMS')}">
                             <span class="action-button-text">${_("Preview")}</span>
                         </a>
                     </li>
@@ -110,10 +110,10 @@ from django.utils.translation import ugettext as _
                 % if xblock.category == 'split_test':
                     <div class="bit">
                         <h3 class="title-3">${_("Adding components")}</h3>
-                        <p>${_("Select a component type under {em_start}Add New Component{em_end}. Then select a template.").format(em_start='<strong>', em_end="</strong>")}</p>
+                        <p>${_("Select a component type under {em_start}Add New Component{em_end}. Then select a template.").format(em_start='<strong>', em_end="</strong>") | h}</p>
                         <p>${_("The new component is added at the bottom of the page or group. You can then edit and move the component.")}</p>
                         <h3 class="title-3">${_("Editing components")}</h3>
-                        <p>${_("Click the {em_start}Edit{em_end} icon in a component to edit its content.").format(em_start='<strong>', em_end="</strong>")}</p>
+                        <p>${_("Click the {em_start}Edit{em_end} icon in a component to edit its content.").format(em_start='<strong>', em_end="</strong>") | h}</p>
                         <h3 class="title-3">${_("Reorganizing components")}</h3>
                         <p>${_("Drag components to new locations within this component.")}</p>
                         <p>${_("For content experiments, you can drag components to other groups.")}</p>
@@ -121,7 +121,7 @@ from django.utils.translation import ugettext as _
                         <p>${_("Confirm that you have properly configured content in each of your experiment groups.")}</p>
                     </div>
                     <div class="bit external-help">
-                        <a href="${get_online_help_info(online_help_token())['doc_url']}" target="_blank" class="button external-help-button">${_("Learn more about component containers")}</a>
+                        <a href="${get_online_help_info(online_help_token())['doc_url'] | h}" target="_blank" class="button external-help-button">${_("Learn more about component containers")}</a>
                     </div>
                 % elif is_unit_page:
                     <div id="publish-unit"></div>

lms/djangoapps/courseware/tests/test_views.py

@@ -680,6 +680,16 @@ class ProgressPageTests(ModuleStoreTestCase):
         self.section = ItemFactory.create(category='sequential', parent_location=self.chapter.location)
         self.vertical = ItemFactory.create(category='vertical', parent_location=self.section.location)
 
+    @ddt.data('"><script>alert(1)</script>', '<script>alert(1)</script>', '</script><script>alert(1)</script>')
+    def test_progress_page_xss_prevent(self, malicious_code):
+        """
+        Test that XSS attack is prevented
+        """
+        resp = views.progress(self.request, course_id=unicode(self.course.id), student_id=self.user.id)
+        self.assertEqual(resp.status_code, 200)
+        # Test that malicious code does not appear in html
+        self.assertNotIn(malicious_code, resp.content)
+
     def test_pure_ungraded_xblock(self):
         ItemFactory.create(category='acid', parent_location=self.vertical.location)
 

lms/djangoapps/django_comment_client/forum/tests.py

@@ -450,7 +450,7 @@ class SingleCohortedThreadTestCase(CohortedTestCase):
         html = response.content
 
         # Verify that the group name is correctly included in the HTML
-        self.assertRegexpMatches(html, r'"group_name": "student_cohort"')
+        self.assertRegexpMatches(html, r'"group_name": "student_cohort"')
 
 
 @patch('lms.lib.comment_client.utils.requests.request')
@@ -1152,10 +1152,10 @@ class UserProfileTestCase(ModuleStoreTestCase):
         self.assertRegexpMatches(html, r'data-num-pages="1"')
         self.assertRegexpMatches(html, r'<span>1</span> discussion started')
         self.assertRegexpMatches(html, r'<span>2</span> comments')
-        self.assertRegexpMatches(html, r'&quot;id&quot;: &quot;{}&quot;'.format(self.TEST_THREAD_ID))
-        self.assertRegexpMatches(html, r'&quot;title&quot;: &quot;{}&quot;'.format(self.TEST_THREAD_TEXT))
-        self.assertRegexpMatches(html, r'&quot;body&quot;: &quot;{}&quot;'.format(self.TEST_THREAD_TEXT))
-        self.assertRegexpMatches(html, r'&quot;username&quot;: &quot;{}&quot;'.format(self.student.username))
+        self.assertRegexpMatches(html, r'&#34;id&#34;: &#34;{}&#34;'.format(self.TEST_THREAD_ID))
+        self.assertRegexpMatches(html, r'&#34;title&#34;: &#34;{}&#34;'.format(self.TEST_THREAD_TEXT))
+        self.assertRegexpMatches(html, r'&#34;body&#34;: &#34;{}&#34;'.format(self.TEST_THREAD_TEXT))
+        self.assertRegexpMatches(html, r'&#34;username&#34;: &#34;{}&#34;'.format(self.student.username))
 
     def check_ajax(self, mock_request, **params):
         response = self.get_response(mock_request, params, HTTP_X_REQUESTED_WITH="XMLHttpRequest")
@@ -1326,6 +1326,57 @@ class ForumFormDiscussionUnicodeTestCase(ModuleStoreTestCase, UnicodeTestMixin):
         self.assertEqual(response_data["discussion_data"][0]["body"], text)
 
 
+@ddt.ddt
+@patch('lms.lib.comment_client.utils.requests.request')
+class ForumDiscussionXSSTestCase(UrlResetMixin, ModuleStoreTestCase):
+    @patch.dict("django.conf.settings.FEATURES", {"ENABLE_DISCUSSION_SERVICE": True})
+    def setUp(self):
+        super(ForumDiscussionXSSTestCase, self).setUp()
+
+        username = "foo"
+        password = "bar"
+
+        self.course = CourseFactory.create()
+        self.student = UserFactory.create(username=username, password=password)
+        CourseEnrollmentFactory.create(user=self.student, course_id=self.course.id)
+        self.assertTrue(self.client.login(username=username, password=password))
+
+    @ddt.data('"><script>alert(1)</script>', '<script>alert(1)</script>', '</script><script>alert(1)</script>')
+    @patch('student.models.cc.User.from_django_user')
+    def test_forum_discussion_xss_prevent(self, malicious_code, mock_from_django_user, mock_request):
+        """
+        Test that XSS attack is prevented
+        """
+        reverse_url = "%s%s" % (reverse(
+            "django_comment_client.forum.views.forum_form_discussion",
+            kwargs={"course_id": unicode(self.course.id)}), '/forum_form_discussion'
+        )
+        # Test that malicious code does not appear in html
+        url = "%s?%s=%s" % (reverse_url, 'sort_key', malicious_code)
+        resp = self.client.get(url)
+        self.assertEqual(resp.status_code, 200)
+        self.assertNotIn(malicious_code, resp.content)
+
+    @ddt.data('"><script>alert(1)</script>', '<script>alert(1)</script>', '</script><script>alert(1)</script>')
+    @patch('student.models.cc.User.from_django_user')
+    @patch('student.models.cc.User.active_threads')
+    def test_forum_user_profile_xss_prevent(self, malicious_code, mock_threads, mock_from_django_user, mock_request):
+        """
+        Test that XSS attack is prevented
+        """
+        mock_threads.return_value = [], 1, 1
+        mock_from_django_user.return_value = Mock()
+        mock_request.side_effect = make_mock_request_impl(course=self.course, text='dummy')
+
+        url = reverse('django_comment_client.forum.views.user_profile',
+                      kwargs={'course_id': unicode(self.course.id), 'user_id': str(self.student.id)})
+        # Test that malicious code does not appear in html
+        url_string = "%s?%s=%s" % (url, 'page', malicious_code)
+        resp = self.client.get(url_string)
+        self.assertEqual(resp.status_code, 200)
+        self.assertNotIn(malicious_code, resp.content)
+
+
 class ForumDiscussionSearchUnicodeTestCase(ModuleStoreTestCase, UnicodeTestMixin):
     def setUp(self):
         super(ForumDiscussionSearchUnicodeTestCase, self).setUp()

lms/djangoapps/django_comment_client/forum/views.py

@@ -5,7 +5,6 @@ Views handling read (GET) requests for the Discussion tab and inline discussions
 from functools import wraps
 import json
 import logging
-import xml.sax.saxutils as saxutils
 
 from django.contrib.auth.decorators import login_required
 from django.conf import settings
@@ -73,13 +72,6 @@ class DiscussionTab(EnrolledTab):
         return settings.FEATURES.get('ENABLE_DISCUSSION_SERVICE')
 
 
-def _attr_safe_json(obj):
-    """
-    return a JSON string for obj which is safe to embed as the value of an attribute in a DOM node
-    """
-    return saxutils.escape(json.dumps(obj), {'"': '"'})
-
-
 @newrelic.agent.function_trace()
 def make_course_settings(course, user):
     """
@@ -278,28 +270,28 @@ def forum_form_discussion(request, course_key):
             'course': course,
             #'recent_active_threads': recent_active_threads,
             'staff_access': bool(has_access(request.user, 'staff', course)),
-            'threads': _attr_safe_json(threads),
+            'threads': json.dumps(threads),
             'thread_pages': query_params['num_pages'],
-            'user_info': _attr_safe_json(user_info),
-            'can_create_comment': _attr_safe_json(
+            'user_info': json.dumps(user_info, default=lambda x: None),
+            'can_create_comment': json.dumps(
                 has_permission(request.user, "create_comment", course.id)),
-            'can_create_subcomment': _attr_safe_json(
+            'can_create_subcomment': json.dumps(
                 has_permission(request.user, "create_sub_comment", course.id)),
             'can_create_thread': has_permission(request.user, "create_thread", course.id),
             'flag_moderator': bool(
                 has_permission(request.user, 'openclose_thread', course.id) or
                 has_access(request.user, 'staff', course)
             ),
-            'annotated_content_info': _attr_safe_json(annotated_content_info),
+            'annotated_content_info': json.dumps(annotated_content_info),
             'course_id': course.id.to_deprecated_string(),
-            'roles': _attr_safe_json(utils.get_role_ids(course_key)),
+            'roles': json.dumps(utils.get_role_ids(course_key)),
             'is_moderator': has_permission(request.user, "see_all_cohorts", course_key),
             'cohorts': course_settings["cohorts"],  # still needed to render _thread_list_template
             'user_cohort': user_cohort_id,  # read from container in NewPostView
             'is_course_cohorted': is_course_cohorted(course_key),  # still needed to render _thread_list_template
             'sort_preference': user.default_sort_key,
             'category_map': course_settings["category_map"],
-            'course_settings': _attr_safe_json(course_settings)
+            'course_settings': json.dumps(course_settings)
         }
         # print "start rendering.."
         return render_to_response('discussion/index.html', context)
@@ -385,19 +377,19 @@ def single_thread(request, course_key, discussion_id, thread_id):
             'discussion_id': discussion_id,
             'csrf': csrf(request)['csrf_token'],
             'init': '',   # TODO: What is this?
-            'user_info': _attr_safe_json(user_info),
-            'can_create_comment': _attr_safe_json(
+            'user_info': json.dumps(user_info),
+            'can_create_comment': json.dumps(
                 has_permission(request.user, "create_comment", course.id)),
-            'can_create_subcomment': _attr_safe_json(
+            'can_create_subcomment': json.dumps(
                 has_permission(request.user, "create_sub_comment", course.id)),
             'can_create_thread': has_permission(request.user, "create_thread", course.id),
-            'annotated_content_info': _attr_safe_json(annotated_content_info),
+            'annotated_content_info': json.dumps(annotated_content_info),
             'course': course,
             #'recent_active_threads': recent_active_threads,
             'course_id': course.id.to_deprecated_string(),   # TODO: Why pass both course and course.id to template?
             'thread_id': thread_id,
-            'threads': _attr_safe_json(threads),
-            'roles': _attr_safe_json(utils.get_role_ids(course_key)),
+            'threads': json.dumps(threads),
+            'roles': json.dumps(utils.get_role_ids(course_key)),
             'is_moderator': is_moderator,
             'thread_pages': query_params['num_pages'],
             'is_course_cohorted': is_course_cohorted(course_key),
@@ -409,7 +401,7 @@ def single_thread(request, course_key, discussion_id, thread_id):
             'user_cohort': user_cohort,
             'sort_preference': cc_user.default_sort_key,
             'category_map': course_settings["category_map"],
-            'course_settings': _attr_safe_json(course_settings)
+            'course_settings': json.dumps(course_settings)
         }
         return render_to_response('discussion/index.html', context)
 
@@ -458,7 +450,7 @@ def user_profile(request, course_key, user_id):
                 'discussion_data': threads,
                 'page': query_params['page'],
                 'num_pages': query_params['num_pages'],
-                'annotated_content_info': _attr_safe_json(annotated_content_info),
+                'annotated_content_info': json.dumps(annotated_content_info),
             })
         else:
             django_user = User.objects.get(id=user_id)
@@ -467,9 +459,9 @@ def user_profile(request, course_key, user_id):
                 'user': request.user,
                 'django_user': django_user,
                 'profiled_user': profiled_user.to_dict(),
-                'threads': _attr_safe_json(threads),
-                'user_info': _attr_safe_json(user_info),
-                'annotated_content_info': _attr_safe_json(annotated_content_info),
+                'threads': json.dumps(threads),
+                'user_info': json.dumps(user_info, default=lambda x: None),
+                'annotated_content_info': json.dumps(annotated_content_info),
                 'page': query_params['page'],
                 'num_pages': query_params['num_pages'],
                 'learner_profile_page_url': reverse('learner_profile', kwargs={'username': django_user.username})
@@ -546,9 +538,9 @@ def followed_threads(request, course_key, user_id):
                 'user': request.user,
                 'django_user': User.objects.get(id=user_id),
                 'profiled_user': profiled_user.to_dict(),
-                'threads': _attr_safe_json(threads),
-                'user_info': _attr_safe_json(user_info),
-                'annotated_content_info': _attr_safe_json(annotated_content_info),
+                'threads': json.dumps(threads),
+                'user_info': json.dumps(user_info),
+                'annotated_content_info': json.dumps(annotated_content_info),
                 #                'content': content,
             }
 

lms/djangoapps/django_comment_client/tests/group_id.py

@@ -24,10 +24,10 @@ class GroupIdAssertionMixin(object):
 
     def _assert_html_response_contains_group_info(self, response):
         group_info = {"group_id": None, "group_name": None}
-        match = re.search(r'"group_id": ([\d]*)', response.content)
+        match = re.search(r'"group_id": ([\d]*)', response.content)
         if match and match.group(1) != '':
             group_info["group_id"] = int(match.group(1))
-        match = re.search(r'"group_name": "([^&]*)"', response.content)
+        match = re.search(r'"group_name": "([^&]*)"', response.content)
         if match:
             group_info["group_name"] = match.group(1)
         self._assert_thread_contains_group_info(group_info)

lms/templates/discussion/index.html

@@ -25,20 +25,20 @@ from django.core.urlresolvers import reverse
 <%include file="_discussion_course_navigation.html" args="active_page='discussion'" />
 
 <section class="discussion container" id="discussion-container"
-         data-roles="${roles}"
+         data-roles="${roles | h}"
          data-course-id="${course_id | h}"
-         data-course-name="${course.display_name_with_default}"
-         data-user-info="${user_info}"
-         data-user-create-comment="${can_create_comment}"
-         data-user-create-subcomment="${can_create_subcomment}"
+         data-course-name="${course.display_name_with_default | h}"
+         data-user-info="${user_info | h}"
+         data-user-create-comment="${can_create_comment | h}"
+         data-user-create-subcomment="${can_create_subcomment | h}"
          data-read-only="false"
-         data-threads="${threads}"
-         data-thread-pages="${thread_pages}"
-         data-content-info="${annotated_content_info}"
-         data-sort-preference="${sort_preference}"
-         data-flag-moderator="${flag_moderator}"
-         data-user-cohort-id="${user_cohort}"
-         data-course-settings="${course_settings}">
+         data-threads="${threads | h}"
+         data-thread-pages="${thread_pages | h}"
+         data-content-info="${annotated_content_info | h}"
+         data-sort-preference="${sort_preference | h}"
+         data-flag-moderator="${flag_moderator | h}"
+         data-user-cohort-id="${user_cohort | h}"
+         data-course-settings="${course_settings | h}">
     <div class="discussion-body">
         <div class="forum-nav" role="complementary" aria-label="${_("Discussion thread list")}"></div>
         <div class="discussion-column" role="main" aria-label="Discussion" id="discussion-column">

lms/templates/discussion/user_profile.html

@@ -33,8 +33,7 @@ from django.template.defaultfilters import escapejs
 
       </nav>
     </section>
-
-    <section class="course-content container discussion-user-threads" data-course-id="${course.id | h}" data-course-name="${course.display_name_with_default}" data-threads="${threads}" data-user-info="${user_info}" data-page="${page}" data-num-pages="${num_pages}"/>
+    <section class="course-content container discussion-user-threads" data-course-id="${course.id | h}" data-course-name="${course.display_name_with_default | h}" data-threads="${threads | h}" data-user-info="${user_info | h}" data-page="${page | h}" data-num-pages="${num_pages | h}"/>
   </div>
 </section>