Merge pull request #11709 from edx/muzaffar/fix-notes-bugs

Muzaffar/fix notes bugs

common/test/acceptance/tests/lms/test_lms_edxnotes.py

@@ -6,14 +6,13 @@ import random
 from uuid import uuid4
 from datetime import datetime
 from nose.plugins.attrib import attr
-from ..helpers import UniqueCourseTest
+from ..helpers import UniqueCourseTest, EventsTestMixin
 from ...fixtures.course import CourseFixture, XBlockFixtureDesc
 from ...pages.lms.auto_auth import AutoAuthPage
 from ...pages.lms.course_nav import CourseNavPage
 from ...pages.lms.courseware import CoursewarePage
 from ...pages.lms.edxnotes import EdxNotesUnitPage, EdxNotesPage, EdxNotesPageNoContent
 from ...fixtures.edxnotes import EdxNotesFixture, Note, Range
-from ..helpers import EventsTestMixin
 
 
 class EdxNotesTestMixin(UniqueCourseTest):
@@ -536,6 +535,57 @@ class EdxNotesPageTest(EventsTestMixin, EdxNotesTestMixin):
             "You have not made any notes in this course yet. Other students in this course are using notes to:",
             notes_page_empty.no_content_text)
 
+    def test_notes_works_correctly_with_xss(self):
+        """
+        Scenario: Note text & tags should be HTML and JS escaped
+        Given I am enrolled in a course with notes enabled
+        When I visit the Notes page, with a Notes text and tag containing HTML characters like < and >
+        Then the text and tags appear as expected due to having been properly escaped
+        """
+        xblocks = self.course_fixture.get_nested_xblocks(category="html")
+        self._add_notes([
+            Note(
+                usage_id=xblocks[0].locator,
+                user=self.username,
+                course_id=self.course_fixture._course_key,  # pylint: disable=protected-access
+                text='<script>alert("XSS")</script>',
+                quote="quote",
+                updated=datetime(2014, 1, 1, 1, 1, 1, 1).isoformat(),
+                tags=['<script>alert("XSS")</script>']
+            ),
+            Note(
+                usage_id=xblocks[1].locator,
+                user=self.username,
+                course_id=self.course_fixture._course_key,  # pylint: disable=protected-access
+                text='<b>bold</b>',
+                quote="quote",
+                updated=datetime(2014, 2, 1, 1, 1, 1, 1).isoformat(),
+                tags=['<i>bold</i>']
+            )
+        ])
+        self.notes_page.visit()
+
+        notes = self.notes_page.notes
+        self.assertEqual(len(notes), 2)
+
+        self.assertNoteContent(
+            notes[0],
+            quote=u"quote",
+            text='<b>bold</b>',
+            unit_name="Test Unit 1",
+            time_updated="Feb 01, 2014 at 01:01 UTC",
+            tags=['<i>bold</i>']
+        )
+
+        self.assertNoteContent(
+            notes[1],
+            quote=u"quote",
+            text='<script>alert("XSS")</script>',
+            unit_name="Test Unit 1",
+            time_updated="Jan 01, 2014 at 01:01 UTC",
+            tags=['<script>alert("XSS")</script>']
+        )
+
     def test_recent_activity_view(self):
         """
         Scenario: User can view all notes by recent activity.

lms/djangoapps/edxnotes/helpers.py

@@ -344,7 +344,7 @@ def get_notes(request, course, page=DEFAULT_PAGE, page_size=DEFAULT_PAGE_SIZE, t
         collection['previous']
     )
 
-    return json.dumps(collection, cls=NoteJSONEncoder)
+    return collection
 
 
 def get_endpoint(api_url, path=""):

lms/djangoapps/edxnotes/tests.py

@@ -387,7 +387,7 @@ class EdxNotesHelpersTest(ModuleStoreTestCase):
                     },
                 ]
             },
-            json.loads(helpers.get_notes(self.request, self.course))
+            helpers.get_notes(self.request, self.course)
         )
 
     @patch("edxnotes.helpers.requests.get", autospec=True)
@@ -493,7 +493,7 @@ class EdxNotesHelpersTest(ModuleStoreTestCase):
                     },
                 ]
             },
-            json.loads(helpers.get_notes(self.request, self.course))
+            helpers.get_notes(self.request, self.course)
         )
 
     @patch("edxnotes.helpers.requests.get", autospec=True)
@@ -520,7 +520,7 @@ class EdxNotesHelpersTest(ModuleStoreTestCase):
         mock_get.return_value.content = json.dumps(NOTES_API_EMPTY_RESPONSE)
         self.assertItemsEqual(
             NOTES_VIEW_EMPTY_RESPONSE,
-            json.loads(helpers.get_notes(self.request, self.course))
+            helpers.get_notes(self.request, self.course)
         )
 
     def test_preprocess_collection_no_item(self):
@@ -989,14 +989,14 @@ class EdxNotesViewsTest(ModuleStoreTestCase):
 
     # pylint: disable=unused-argument
     @patch.dict("django.conf.settings.FEATURES", {"ENABLE_EDXNOTES": True})
-    @patch("edxnotes.views.get_notes", return_value=json.dumps({'results': []}))
+    @patch("edxnotes.views.get_notes", return_value={'results': []})
     def test_edxnotes_view_is_enabled(self, mock_get_notes):
         """
         Tests that appropriate view is received if EdxNotes feature is enabled.
         """
         enable_edxnotes_for_the_course(self.course, self.user.id)
         response = self.client.get(self.notes_page_url)
-        self.assertContains(response, 'Highlights and notes you\'ve made in course content')
+        self.assertContains(response, 'Highlights and notes you've made in course content')
 
     @patch.dict("django.conf.settings.FEATURES", {"ENABLE_EDXNOTES": False})
     def test_edxnotes_view_is_disabled(self):
@@ -1012,7 +1012,7 @@ class EdxNotesViewsTest(ModuleStoreTestCase):
         """
         Tests that search notes successfully respond if EdxNotes feature is enabled.
         """
-        mock_search.return_value = json.dumps(NOTES_VIEW_EMPTY_RESPONSE)
+        mock_search.return_value = NOTES_VIEW_EMPTY_RESPONSE
         enable_edxnotes_for_the_course(self.course, self.user.id)
         response = self.client.get(self.notes_url, {"text": "test"})
         self.assertEqual(json.loads(response.content), NOTES_VIEW_EMPTY_RESPONSE)

lms/djangoapps/edxnotes/views.py

@@ -22,6 +22,7 @@ from edxnotes.helpers import (
     get_course_position,
     DEFAULT_PAGE,
     DEFAULT_PAGE_SIZE,
+    NoteJSONEncoder,
 )
 
 
@@ -47,18 +48,19 @@ def edxnotes(request, course_id):
         raise Http404
 
     notes_info = get_notes(request, course)
-
+    has_notes = (len(notes_info.get('results')) > 0)
     context = {
         "course": course,
         "notes_endpoint": reverse("notes", kwargs={"course_id": course_id}),
         "notes": notes_info,
         "page_size": DEFAULT_PAGE_SIZE,
-        "debug": json.dumps(settings.DEBUG),
+        "debug": settings.DEBUG,
         'position': None,
         'disabled_tabs': settings.NOTES_DISABLED_TABS,
+        'has_notes': has_notes,
     }
 
-    if len(json.loads(notes_info)['results']) == 0:
+    if not has_notes:
         field_data_cache = FieldDataCache.cache_for_descriptor_descendents(
             course.id, request.user, course, depth=2
         )
@@ -164,7 +166,7 @@ def notes(request, course_id):
     except (EdxNotesParseError, EdxNotesServiceUnavailable) as err:
         return JsonResponseBadRequest({"error": err.message}, status=500)
 
-    return HttpResponse(notes_info, content_type="application/json")
+    return HttpResponse(json.dumps(notes_info, cls=NoteJSONEncoder), content_type="application/json")
 
 
 # pylint: disable=unused-argument

lms/static/js/dashboard/track_events.js

@@ -34,9 +34,7 @@ var edx = edx || {};
     // Emit an event when the 'course title link' is clicked.
     edx.dashboard.trackCourseTitleClicked = function($courseTitleLink, properties){
         var trackProperty = properties || edx.dashboard.generateTrackProperties;
-        if (!window.analytics) {
-            return;
-        }
+
         window.analytics.trackLink(
             $courseTitleLink,
             'edx.bi.dashboard.course_title.clicked',
@@ -105,9 +103,6 @@ var edx = edx || {};
     };
 
     edx.dashboard.xseriesTrackMessages = function() {
-        if (!window.analytics) {
-            return;
-        }
 
         $('.xseries-action .btn').each(function(i, element) {
             var data = edx.dashboard.generateProgramProperties($(element));
@@ -117,6 +112,9 @@ var edx = edx || {};
     };
 
     $(document).ready(function() {
+        if (!window.analytics) {
+            return;
+        }
         edx.dashboard.trackCourseTitleClicked($('.course-title > a'));
         edx.dashboard.trackCourseImageLinkClicked($('.cover'));
         edx.dashboard.trackEnterCourseLinkClicked($('.enter-course'));

lms/static/js/edxnotes/models/note.js

@@ -51,10 +51,6 @@ define(['backbone', 'js/edxnotes/utils/utils', 'underscore.string'], function (B
             }
 
             return message;
-        },
-
-        getText: function () {
-            return Utils.nl2br(this.get('text'));
         }
 
     });

lms/static/js/edxnotes/views/note_item.js

@@ -31,8 +31,7 @@ define([
 
         getContext: function () {
             return $.extend({}, this.model.toJSON(), {
-                message: this.model.getQuote(),
-                text: this.model.getText()
+                message: this.model.getQuote()
             });
         },
 
@@ -60,7 +59,9 @@ define([
 
         tagHandler: function (event) {
             event.preventDefault();
-            this.options.scrollToTag(event.currentTarget.text);
+            if (!_.isUndefined(this.options.scrollToTag)) {
+                this.options.scrollToTag(event.currentTarget.text);
+            }
         },
 
         redirectTo: function (uri) {

lms/static/js/spec/edxnotes/models/note_spec.js

@@ -46,7 +46,7 @@ define([
 
         it('can return appropriate `text`', function () {
             var model = this.collection.at(0);
-            expect(model.getText()).toBe('text<br> with<br>line<br>breaks <br>');
+            expect(model.get('text')).toBe('text\n with\r\nline\n\rbreaks \r');
         });
     });
 });

lms/static/sass/course/_student-notes.scss

@@ -189,6 +189,10 @@ $divider-visual-tertiary: ($baseline/20) solid $gray-l4;
               background: transparent;
             }
 
+            .note-comment-p {
+              word-wrap: break-word;
+            }
+
             .note-comment-ul,
             .note-comment-ol {
               padding: auto;
@@ -233,11 +237,14 @@ $divider-visual-tertiary: ($baseline/20) solid $gray-l4;
             color: $m-gray-d2;
           }
 
-          // CASE: tag matches a search query
-          .reference-meta.reference-tags .note-highlight {
-            background-color: $result-highlight-color-base;
-          }
 
+          .reference-meta.reference-tags {
+            word-wrap: break-word;
+            // CASE: tag matches a search query
+            .note-highlight {
+              background-color: $result-highlight-color-base;
+            }
+          }
           // Put commas between tags.
           span.reference-meta.reference-tags:after {
             content: ",";

lms/templates/edxnotes/edxnotes.html

+<%page expression_filter="h"/>
 <%inherit file="/main.html" />
 <%namespace name='static' file='/static_content.html'/>
+
 <%!
 from django.utils.translation import ugettext as _
-import json
+from edxnotes.helpers import NoteJSONEncoder
+from openedx.core.djangolib.js_utils import dump_js_escaped_json, js_escaped_string
 %>
 
 <%block name="bodyclass">view-student-notes is-in-course course</%block>
@@ -12,10 +15,7 @@ import json
 </%block>
 
 <%include file="/courseware/course_navigation.html" args="active_page='edxnotes'" />
-<%
-  _notes = json.loads(notes)
-  has_notes = _notes and _notes.get('results')
-%>
+
 <section class="container">
   <div class="wrapper-student-notes">
     <div class="student-notes">
@@ -111,11 +111,11 @@ import json
     <%block name="js_extra">
     <%static:require_module module_name="js/edxnotes/views/page_factory" class_name="NotesPageFactory">
         NotesPageFactory({
-            disabledTabs: ${disabled_tabs},
-            notes: ${notes},
-            notesEndpoint: '${notes_endpoint}',
-            pageSize: '${page_size}',
-            debugMode: ${debug}
+            disabledTabs: ${disabled_tabs | n, dump_js_escaped_json},
+            notes: ${dump_js_escaped_json(notes, NoteJSONEncoder) | n},
+            notesEndpoint: ${notes_endpoint | n, dump_js_escaped_json},
+            pageSize: ${page_size | n, dump_js_escaped_json},
+            debugMode: ${debug | n, dump_js_escaped_json}
         });
     </%static:require_module>
     </%block>