Merge pull request #323 from edx/ned/fix-lms-530

Fix LMS-530: Reflected XSS in chapter and page numbers

CHANGELOG.rst

@@ -15,6 +15,8 @@ LMS: Users are no longer auto-activated if they click "reset password"
 This is now done when they click on the link in the reset password
 email they receive (along with usual path through activation email).
 
+LMS: Fixed a reflected XSS problem in the static textbook views.
+
 LMS: Problem rescoring.  Added options on the Grades tab of the
 Instructor Dashboard to allow a particular student's submission for a
 particular problem to be rescored.  Provides an option to see a

common/lib/xmodule/xmodule/modulestore/tests/factories.py

@@ -61,6 +61,7 @@ class XModuleCourseFactory(Factory):
 
         # Update the data in the mongo datastore
         store.update_metadata(new_course.location.url(), own_metadata(new_course))
+        store.update_item(new_course.location.url(), new_course._model_data._kvs._data)
 
         # update_item updates the the course as it exists in the modulestore, but doesn't
         # update the instance we are working with, so have to refetch the course after updating it.

lms/djangoapps/staticbook/tests.py

@@ -2,8 +2,13 @@
 Test the lms/staticbook views.
 """
 
+import textwrap
+
+import mock
+import requests
+
 from django.test.utils import override_settings
-from django.core.urlresolvers import reverse
+from django.core.urlresolvers import reverse, NoReverseMatch
 
 from courseware.tests.tests import TEST_DATA_MONGO_MODULESTORE
 from student.tests.factories import UserFactory, CourseEnrollmentFactory
@@ -11,6 +16,8 @@ from xmodule.modulestore.tests.factories import CourseFactory
 from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase
 
 
+IMAGE_BOOK = ("An Image Textbook", "http://example.com/the_book/")
+
 PDF_BOOK = {
     "tab_title": "Textbook",
     "title": "A PDF Textbook",
@@ -60,6 +67,44 @@ class StaticBookTest(ModuleStoreTestCase):
         return url
 
 
+class StaticImageBookTest(StaticBookTest):
+    """
+    Test the image-based static book view.
+    """
+
+    def test_book(self):
+        # We can access a book.
+        with mock.patch.object(requests, 'get') as mock_get:
+            mock_get.return_value.text = textwrap.dedent('''\
+                <?xml version="1.0"?>
+                <table_of_contents>
+                <entry page="9" page_label="ix" name="Contents!?"/>
+                <entry page="1" page_label="i" name="Preamble">
+                    <entry page="4" page_label="iv" name="About the Elephants"/>
+                </entry>
+                </table_of_contents>
+                ''')
+
+            self.make_course(textbooks=[IMAGE_BOOK])
+            url = self.make_url('book', book_index=0)
+            response = self.client.get(url)
+
+        self.assertContains(response, "Contents!?")
+        self.assertContains(response, "About the Elephants")
+
+    def test_bad_book_id(self):
+        # A bad book id will be a 404.
+        self.make_course(textbooks=[IMAGE_BOOK])
+        with self.assertRaises(NoReverseMatch):
+            self.make_url('book', book_index='fooey')
+
+    def test_out_of_range_book_id(self):
+        self.make_course()
+        url = self.make_url('book', book_index=0)
+        response = self.client.get(url)
+        self.assertEqual(response.status_code, 404)
+
+
 class StaticPdfBookTest(StaticBookTest):
     """
     Test the PDF static book view.
@@ -102,6 +147,12 @@ class StaticPdfBookTest(StaticBookTest):
         self.assertContains(response, "options.pageNum = 17;")
 
     def test_bad_book_id(self):
+        # If the book id isn't an int, we'll get a 404.
+        self.make_course(pdf_textbooks=[PDF_BOOK])
+        with self.assertRaises(NoReverseMatch):
+            self.make_url('pdf_book', book_index='fooey', chapter=1)
+
+    def test_out_of_range_book_id(self):
         # If we have one book, asking for the second book will fail with a 404.
         self.make_course(pdf_textbooks=[PDF_BOOK])
         url = self.make_url('pdf_book', book_index=1, chapter=1)
@@ -115,6 +166,27 @@ class StaticPdfBookTest(StaticBookTest):
         response = self.client.get(url)
         self.assertEqual(response.status_code, 404)
 
+    def test_chapter_xss(self):
+        # The chapter in the URL used to go right on the page.
+        self.make_course(pdf_textbooks=[PDF_BOOK])
+        # It's no longer possible to use a non-integer chapter.
+        with self.assertRaises(NoReverseMatch):
+            self.make_url('pdf_book', book_index=0, chapter='xyzzy')
+
+    def test_page_xss(self):
+        # The page in the URL used to go right on the page.
+        self.make_course(pdf_textbooks=[PDF_BOOK])
+        # It's no longer possible to use a non-integer page.
+        with self.assertRaises(NoReverseMatch):
+            self.make_url('pdf_book', book_index=0, page='xyzzy')
+
+    def test_chapter_page_xss(self):
+        # The page in the URL used to go right on the page.
+        self.make_course(pdf_textbooks=[PDF_BOOK])
+        # It's no longer possible to use a non-integer page and a non-integer chapter.
+        with self.assertRaises(NoReverseMatch):
+            self.make_url('pdf_book', book_index=0, chapter='fooey', page='xyzzy')
+
 
 class StaticHtmlBookTest(StaticBookTest):
     """
@@ -150,3 +222,10 @@ class StaticHtmlBookTest(StaticBookTest):
         url = self.make_url('html_book', book_index=0, chapter=1)
         response = self.client.get(url)
         self.assertEqual(response.status_code, 404)
+
+    def test_chapter_xss(self):
+        # The chapter in the URL used to go right on the page.
+        self.make_course(pdf_textbooks=[HTML_BOOK])
+        # It's no longer possible to use a non-integer chapter.
+        with self.assertRaises(NoReverseMatch):
+            self.make_url('html_book', book_index=0, chapter='xyzzy')

lms/urls.py

@@ -223,24 +223,24 @@ if settings.COURSEWARE_ENABLED:
             'courseware.views.course_info', name="info"),
         url(r'^courses/(?P<course_id>[^/]+/[^/]+/[^/]+)/syllabus$',
             'courseware.views.syllabus', name="syllabus"),   # TODO arjun remove when custom tabs in place, see courseware/courses.py
-        url(r'^courses/(?P<course_id>[^/]+/[^/]+/[^/]+)/book/(?P<book_index>[^/]*)/$',
+        url(r'^courses/(?P<course_id>[^/]+/[^/]+/[^/]+)/book/(?P<book_index>\d+)/$',
+            'staticbook.views.index', name="book"),
+        url(r'^courses/(?P<course_id>[^/]+/[^/]+/[^/]+)/book/(?P<book_index>\d+)/(?P<page>\d+)$',
             'staticbook.views.index', name="book"),
-        url(r'^courses/(?P<course_id>[^/]+/[^/]+/[^/]+)/book/(?P<book_index>[^/]*)/(?P<page>[^/]*)$',
-            'staticbook.views.index'),
 
-        url(r'^courses/(?P<course_id>[^/]+/[^/]+/[^/]+)/pdfbook/(?P<book_index>[^/]*)/$',
+        url(r'^courses/(?P<course_id>[^/]+/[^/]+/[^/]+)/pdfbook/(?P<book_index>\d+)/$',
             'staticbook.views.pdf_index', name="pdf_book"),
-        url(r'^courses/(?P<course_id>[^/]+/[^/]+/[^/]+)/pdfbook/(?P<book_index>[^/]*)/(?P<page>[^/]*)$',
+        url(r'^courses/(?P<course_id>[^/]+/[^/]+/[^/]+)/pdfbook/(?P<book_index>\d+)/(?P<page>\d+)$',
             'staticbook.views.pdf_index', name="pdf_book"),
 
-        url(r'^courses/(?P<course_id>[^/]+/[^/]+/[^/]+)/pdfbook/(?P<book_index>[^/]*)/chapter/(?P<chapter>[^/]*)/$',
+        url(r'^courses/(?P<course_id>[^/]+/[^/]+/[^/]+)/pdfbook/(?P<book_index>\d+)/chapter/(?P<chapter>\d+)/$',
             'staticbook.views.pdf_index', name="pdf_book"),
-        url(r'^courses/(?P<course_id>[^/]+/[^/]+/[^/]+)/pdfbook/(?P<book_index>[^/]*)/chapter/(?P<chapter>[^/]*)/(?P<page>[^/]*)$',
+        url(r'^courses/(?P<course_id>[^/]+/[^/]+/[^/]+)/pdfbook/(?P<book_index>\d+)/chapter/(?P<chapter>\d+)/(?P<page>\d+)$',
             'staticbook.views.pdf_index', name="pdf_book"),
 
-        url(r'^courses/(?P<course_id>[^/]+/[^/]+/[^/]+)/htmlbook/(?P<book_index>[^/]*)/$',
+        url(r'^courses/(?P<course_id>[^/]+/[^/]+/[^/]+)/htmlbook/(?P<book_index>\d+)/$',
             'staticbook.views.html_index', name="html_book"),
-        url(r'^courses/(?P<course_id>[^/]+/[^/]+/[^/]+)/htmlbook/(?P<book_index>[^/]*)/chapter/(?P<chapter>[^/]*)/$',
+        url(r'^courses/(?P<course_id>[^/]+/[^/]+/[^/]+)/htmlbook/(?P<book_index>\d+)/chapter/(?P<chapter>\d+)/$',
             'staticbook.views.html_index', name="html_book"),
 
         url(r'^courses/(?P<course_id>[^/]+/[^/]+/[^/]+)/courseware/?$',