From b095651655f6337361a92aeccec89269f5b8064b Mon Sep 17 00:00:00 2001 From: muzaffaryousaf Date: Tue, 1 Mar 2016 15:51:08 +0500 Subject: [PATCH] Escape html/js with other bug fixes . TNL-4164 --- .../acceptance/tests/lms/test_lms_edxnotes.py | 54 ++++++++++++++++++- lms/djangoapps/edxnotes/helpers.py | 2 +- lms/djangoapps/edxnotes/tests.py | 12 ++--- lms/djangoapps/edxnotes/views.py | 10 ++-- lms/static/js/dashboard/track_events.js | 10 ++-- lms/static/js/edxnotes/models/note.js | 4 -- lms/static/js/edxnotes/views/note_item.js | 7 +-- .../js/spec/edxnotes/models/note_spec.js | 2 +- lms/static/sass/course/_student-notes.scss | 15 ++++-- lms/templates/edxnotes/edxnotes.html | 20 +++---- 10 files changed, 95 insertions(+), 41 deletions(-) diff --git a/common/test/acceptance/tests/lms/test_lms_edxnotes.py b/common/test/acceptance/tests/lms/test_lms_edxnotes.py index 05ebde8f68..fe4314c7ba 100644 --- a/common/test/acceptance/tests/lms/test_lms_edxnotes.py +++ b/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='', + quote="quote", + updated=datetime(2014, 1, 1, 1, 1, 1, 1).isoformat(), + tags=[''] + ), + Note( + usage_id=xblocks[1].locator, + user=self.username, + course_id=self.course_fixture._course_key, # pylint: disable=protected-access + text='bold', + quote="quote", + updated=datetime(2014, 2, 1, 1, 1, 1, 1).isoformat(), + tags=['bold'] + ) + ]) + self.notes_page.visit() + + notes = self.notes_page.notes + self.assertEqual(len(notes), 2) + + self.assertNoteContent( + notes[0], + quote=u"quote", + text='bold', + unit_name="Test Unit 1", + time_updated="Feb 01, 2014 at 01:01 UTC", + tags=['bold'] + ) + + self.assertNoteContent( + notes[1], + quote=u"quote", + text='', + unit_name="Test Unit 1", + time_updated="Jan 01, 2014 at 01:01 UTC", + tags=[''] + ) + def test_recent_activity_view(self): """ Scenario: User can view all notes by recent activity. diff --git a/lms/djangoapps/edxnotes/helpers.py b/lms/djangoapps/edxnotes/helpers.py index 78f5926842..a4ad3dd09e 100644 --- a/lms/djangoapps/edxnotes/helpers.py +++ b/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=""): diff --git a/lms/djangoapps/edxnotes/tests.py b/lms/djangoapps/edxnotes/tests.py index bffe86dc6a..76010b4e2d 100644 --- a/lms/djangoapps/edxnotes/tests.py +++ b/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) diff --git a/lms/djangoapps/edxnotes/views.py b/lms/djangoapps/edxnotes/views.py index 160375b952..a4d0a5bb1b 100644 --- a/lms/djangoapps/edxnotes/views.py +++ b/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 diff --git a/lms/static/js/dashboard/track_events.js b/lms/static/js/dashboard/track_events.js index 4218eb20ff..b61a0d2768 100644 --- a/lms/static/js/dashboard/track_events.js +++ b/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')); diff --git a/lms/static/js/edxnotes/models/note.js b/lms/static/js/edxnotes/models/note.js index 9d48b1c61b..60ce8e11da 100644 --- a/lms/static/js/edxnotes/models/note.js +++ b/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')); } }); diff --git a/lms/static/js/edxnotes/views/note_item.js b/lms/static/js/edxnotes/views/note_item.js index c6a01e0549..1f5fd27709 100644 --- a/lms/static/js/edxnotes/views/note_item.js +++ b/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) { diff --git a/lms/static/js/spec/edxnotes/models/note_spec.js b/lms/static/js/spec/edxnotes/models/note_spec.js index ac7cad4cc6..3a79f32596 100644 --- a/lms/static/js/spec/edxnotes/models/note_spec.js +++ b/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
with
line
breaks
'); + expect(model.get('text')).toBe('text\n with\r\nline\n\rbreaks \r'); }); }); }); diff --git a/lms/static/sass/course/_student-notes.scss b/lms/static/sass/course/_student-notes.scss index eef10f8e07..c739917989 100644 --- a/lms/static/sass/course/_student-notes.scss +++ b/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: ","; diff --git a/lms/templates/edxnotes/edxnotes.html b/lms/templates/edxnotes/edxnotes.html index f1d4849e45..533646c5cd 100644 --- a/lms/templates/edxnotes/edxnotes.html +++ b/lms/templates/edxnotes/edxnotes.html @@ -1,8 +1,11 @@ +<%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 @@ -12,10 +15,7 @@ import json <%include file="/courseware/course_navigation.html" args="active_page='edxnotes'" /> -<% - _notes = json.loads(notes) - has_notes = _notes and _notes.get('results') -%> +
@@ -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} });