Merge pull request #104 from edx/hotfix/instructor-dash

Fix XSS vulnerability in instructor dashboard

lms/djangoapps/instructor/tests/test_xss.py

@@ -0,0 +1,63 @@
+"""
+Tests of various instructor dashboard features that include lists of students
+"""
+
+from django.conf import settings
+from django.test import TestCase
+from django.test.client import RequestFactory
+from django.test.utils import override_settings
+from markupsafe import escape
+
+from courseware.tests.tests import TEST_DATA_MONGO_MODULESTORE
+from student.tests.factories import UserFactory, CourseEnrollmentFactory
+from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase
+from xmodule.modulestore.tests.factories import CourseFactory
+
+from instructor import views
+
+@override_settings(MODULESTORE=TEST_DATA_MONGO_MODULESTORE)
+class TestXss(ModuleStoreTestCase):
+    def setUp(self):
+        self._request_factory = RequestFactory()
+        self._course = CourseFactory.create()
+        self._evil_student = UserFactory.create(
+            email="robot+evil@edx.org",
+            username="evil-robot",
+            profile__name='<span id="evil">Evil Robot</span>',
+        )
+        self._instructor = UserFactory.create(
+            email="robot+instructor@edx.org",
+            username="instructor",
+            is_staff=True
+        )
+        CourseEnrollmentFactory.create(
+            user=self._evil_student,
+            course_id=self._course.id
+        )
+
+    def _test_action(self, action):
+        """
+        Test for XSS vulnerability in the given action
+
+        Build a request with the given action, call the instructor dashboard
+        view, and check that HTML code in a user's name is properly escaped.
+        """
+        req  = self._request_factory.post(
+            "dummy_url",
+            data={"action": action}
+        )
+        req.user = self._instructor
+        req.session = {}
+        resp = views.instructor_dashboard(req, self._course.id)
+        respUnicode = resp.content.decode(settings.DEFAULT_CHARSET)
+        self.assertNotIn(self._evil_student.profile.name, respUnicode)
+        self.assertIn(escape(self._evil_student.profile.name), respUnicode)
+
+    def test_list_enrolled(self):
+        self._test_action("List enrolled students")
+
+    def test_dump_list_of_enrolled(self):
+        self._test_action("Dump list of enrolled students")
+
+    def test_dump_grades(self):
+        self._test_action("Dump Grades for all students in this course")

lms/djangoapps/instructor/views.py

@@ -5,6 +5,7 @@ from collections import defaultdict
 import csv
 import json
 import logging
+from markupsafe import escape
 import os
 import re
 import requests
@@ -76,10 +77,6 @@ def instructor_dashboard(request, course_id):
     else:
         idash_mode = request.session.get('idash_mode', 'Grades')
 
-    def escape(s):
-        """escape HTML special characters in string"""
-        return str(s).replace('<', '&lt;').replace('>', '&gt;')
-
     # assemble some course statistics for output to instructor
     datatable = {'header': ['Statistic', 'Value'],
                  'title': 'Course Statistics At A Glance',
@@ -316,7 +313,7 @@ def instructor_dashboard(request, course_id):
         datatable = {'header': ['Student  email', 'Match?']}
         rg_students = [x['email'] for x in rg_stud_data['retdata']]
         def domatch(x):
-            return '<font color="green">yes</font>' if x.email in rg_students else '<font color="red">No</font>'
+            return 'yes' if x.email in rg_students else 'No'
         datatable['data'] = [[x.email, domatch(x)] for x in stud_data['students']]
         datatable['title'] = action
 

lms/templates/courseware/instructor_dashboard.html

@@ -539,17 +539,17 @@ function goto( mode)
     <br/>
     <p>
       <hr width="100%">
-      <h2>${datatable['title']}</h2>
+      <h2>${datatable['title'] | h}</h2>
       <table class="stat_table">
         <tr>
 	%for hname in datatable['header']:
-	  <th>${hname}</th>
+	  <th>${hname | h}</th>
 	%endfor
 	</tr>
         %for row in datatable['data']:
           <tr>
 	    %for value in row:
-	      <td>${value}</td>
+	      <td>${value | h}</td>
 	    %endfor
 	  </tr>
         %endfor