From 9c4b458d2af8f9200be3dfb900da74a8187f2b8e Mon Sep 17 00:00:00 2001 From: Samuel Walladge Date: Fri, 7 Feb 2020 14:56:29 +1030 Subject: [PATCH 1/4] Fix elem not selected if id contains special chars If the id of a `.formulaequationinput input` element contains a special character, then the selector for $preview was silently failing to match the element, because no escaping was happening. This fixes the issue by escaping the id before passing to the jQuery selector function. CSS.escape is the ideal method, but this isn't present in IE or Edge, so we use a fallback borrowed from the new jQuery.escapeSelector method. --- .../spec/formula_equation_preview_spec.js | 26 ++++++++++++++ .../js/capa/src/formula_equation_preview.js | 36 ++++++++++++++++++- 2 files changed, 61 insertions(+), 1 deletion(-) diff --git a/common/static/js/capa/spec/formula_equation_preview_spec.js b/common/static/js/capa/spec/formula_equation_preview_spec.js index ad0a903e03..5e9cf7a487 100644 --- a/common/static/js/capa/spec/formula_equation_preview_spec.js +++ b/common/static/js/capa/spec/formula_equation_preview_spec.js @@ -1,3 +1,29 @@ +describe('escapeSelector', function() { + 'use strict'; + var escapeSelector = window.escapeSelector; + + it('correctly escapes css', function() { + // tests borrowed from https://github.com/jquery/jquery/blob/3edfa1bcdc50bca41ac58b2642b12f3feee03a3b/test/unit/selector.js#L2030 + expect(escapeSelector('-')).toEqual('\\-'); + expect(escapeSelector('-a')).toEqual('-a'); + expect(escapeSelector('--')).toEqual('--'); + expect(escapeSelector('--a')).toEqual('--a'); + expect(escapeSelector('\uFFFD')).toEqual('\uFFFD'); + expect(escapeSelector('\uFFFDb')).toEqual('\uFFFDb'); + expect(escapeSelector('a\uFFFDb')).toEqual('a\uFFFDb'); + expect(escapeSelector('1a')).toEqual('\\31 a'); + expect(escapeSelector('a\0b')).toEqual('a\uFFFDb'); + expect(escapeSelector('a3b')).toEqual('a3b'); + expect(escapeSelector('-4a')).toEqual('-\\34 a'); + expect(escapeSelector('\x01\x02\x1E\x1F')).toEqual('\\1 \\2 \\1e \\1f '); + + // This is the important one; xblocks and course ids often contain invalid characters, so if these aren't + // escaped when embedding/searching xblock IDs using css selectors, bad things happen. + expect(escapeSelector('course-v1:edX+DemoX+Demo_Course')).toEqual('course-v1\\:edX\\+DemoX\\+Demo_Course'); + expect(escapeSelector('block-v1:edX+DemoX+Demo_Course+type@sequential+block')).toEqual('block-v1\\:edX\\+DemoX\\+Demo_Course\\+type\\@sequential\\+block'); + }); +}); + describe('Formula Equation Preview', function() { 'use strict'; var formulaEquationPreview = window.formulaEquationPreview; diff --git a/common/static/js/capa/src/formula_equation_preview.js b/common/static/js/capa/src/formula_equation_preview.js index 646ba0809c..1838bf498b 100644 --- a/common/static/js/capa/src/formula_equation_preview.js +++ b/common/static/js/capa/src/formula_equation_preview.js @@ -1,3 +1,37 @@ +function escapeSelector(id) { + // Wrapper around window.CSS.escape that uses a fallback method if CSS.escape is not available. + // This is designed to serialize a string to be used as a valid css selector. See https://drafts.csswg.org/cssom/#the-css.escape()-method + // For example, can be used with xblock and course ids, which often contain invalid characters that must be escaped + // to function properly in css selectors. + + // TODO: if this escaping is also required elsewhere, it may be useful to add a global CSS.escape polyfill and + // use that directly. + if (window.CSS && window.CSS.escape) { + return CSS.escape(id); + } else { + // CSS escape alternative borrowed from https://api.jquery.com/jQuery.escapeSelector/ source. When we upgrade to jQuery 3.0, we can use $.escapeSelector() instead of this shim escapeSelector function. + // source: https://github.com/jquery/jquery/blob/3edfa1bc/src/selector/escapeSelector.js + + // CSS string/identifier serialization + // https://drafts.csswg.org/cssom/#common-serializing-idioms + var rcssescape = /([\0-\x1f\x7f]|^-?\d)|^-$|[^\x80-\uFFFF\w-]/g; + function fcssescape( ch, asCodePoint ) { + if ( asCodePoint ) { + // U+0000 NULL becomes U+FFFD REPLACEMENT CHARACTER + if ( ch === "\0" ) { + return "\uFFFD"; + } + // Control characters and (dependent upon position) numbers get escaped as code points + return ch.slice( 0, -1 ) + "\\" + ch.charCodeAt( ch.length - 1 ).toString( 16 ) + " "; + } + // Other potentially-special ASCII characters get backslash-escaped + return "\\" + ch; + } + // ensure string and then run the replacements + return (id + "").replace(rcssescape, fcssescape); + } +} + var formulaEquationPreview = { minDelay: 300, // Minimum time between requests sent out. errorDelay: 1500 // Wait time before showing error (prevent frustration). @@ -13,7 +47,7 @@ formulaEquationPreview.enable = function() { function setupInput() { var $this = $(this); // cache the jQuery object - var $preview = $('#' + this.id + '_preview'); + var $preview = $('#' + escapeSelector(this.id) + '_preview'); var inputData = { // These are the mutable values From 71fcf6e72568448f261b9d42cee92e4fc0f4b059 Mon Sep 17 00:00:00 2001 From: Samuel Walladge Date: Fri, 7 Feb 2020 15:58:46 +1030 Subject: [PATCH 2/4] Fix issues with xss linters Improve accuracy of javascript-escape linter: Previously this would match on FOOescape() and FOO.escape calls, but neither are the global escape function we are worried about. The regex probably isn't 100% accurate; there may be still false positives (javascript allows a large range of characters in identifiers, some of which may not be covered by [\w.$]). The main thing is to avoid false negatives here though - this will definitely catch any use of `escape()` or `window.escape()`. Also remove javascript-interpolate lint - this was deemed unecessary. StringUtils.interpolate is not in fact safe (it does no html escaping), so the results of this lint are misleading. --- .../xmodule/xmodule/js/src/capa/schematic.js | 10 +++---- .../xmodule/js/src/sequence/display.js | 1 - .../js/src/video/06_video_progress_slider.js | 1 - .../teams/js/views/edit_team_members.js | 4 +-- scripts/xsslint/tests/test_linters.py | 28 +++++++----------- scripts/xsslint/xsslint/linters.py | 29 +++---------------- scripts/xsslint_thresholds.json | 1 - 7 files changed, 21 insertions(+), 53 deletions(-) diff --git a/common/lib/xmodule/xmodule/js/src/capa/schematic.js b/common/lib/xmodule/xmodule/js/src/capa/schematic.js index 63edb278d8..92c274c7bc 100644 --- a/common/lib/xmodule/xmodule/js/src/capa/schematic.js +++ b/common/lib/xmodule/xmodule/js/src/capa/schematic.js @@ -2810,7 +2810,7 @@ schematic = (function() { // for each requested freq, interpolate response value for (var k = 1; k < flist.length; k++) { var f = flist[k]; - var v = interpolate(f,x_values,values); //xss-lint: disable=javascript-interpolate + var v = interpolate(f,x_values,values); // convert to dB fvlist.push([f,v == undefined ? 'undefined' : 20.0 * Math.log(v)/Math.LN10]); } @@ -2932,7 +2932,7 @@ schematic = (function() { // for each requested time, interpolate waveform value for (var k = 1; k < tlist.length; k++) { var t = tlist[k]; - var v = interpolate(t,times,values); // xss-lint: disable=javascript-interpolate + var v = interpolate(t,times,values); tvlist.push([t,v == undefined ? 'undefined' : v]); } // save results as list of [t,value] pairs @@ -2978,7 +2978,7 @@ schematic = (function() { // t is the time at which we want a value // times is a list of timepoints from the simulation - function interpolate(t,times,values) { // xss-lint: disable=javascript-interpolate + function interpolate(t,times,values) { if (values == undefined) return undefined; for (var i = 0; i < times.length; i++) @@ -5219,7 +5219,7 @@ schematic = (function() { } Wire.prototype = new Component(); Wire.prototype.constructor = Wire; - + Wire.prototype.toString = function() { return edx.StringUtils.interpolate( '', @@ -5348,7 +5348,7 @@ schematic = (function() { y: this.y }); } - + Ground.prototype.draw = function(c) { Component.prototype.draw.call(this,c); // give superclass a shot this.draw_line(c,0,0,0,8); diff --git a/common/lib/xmodule/xmodule/js/src/sequence/display.js b/common/lib/xmodule/xmodule/js/src/sequence/display.js index 06f519bd35..1a283e55b8 100644 --- a/common/lib/xmodule/xmodule/js/src/sequence/display.js +++ b/common/lib/xmodule/xmodule/js/src/sequence/display.js @@ -329,7 +329,6 @@ this.render(newPosition); } else { alertTemplate = gettext('Sequence error! Cannot navigate to %(tab_name)s in the current SequenceModule. Please contact the course staff.'); // eslint-disable-line max-len - // xss-lint: disable=javascript-interpolate alertText = interpolate(alertTemplate, { tab_name: newPosition }, true); diff --git a/common/lib/xmodule/xmodule/js/src/video/06_video_progress_slider.js b/common/lib/xmodule/xmodule/js/src/video/06_video_progress_slider.js index df7ac15e08..3179e8b153 100644 --- a/common/lib/xmodule/xmodule/js/src/video/06_video_progress_slider.js +++ b/common/lib/xmodule/xmodule/js/src/video/06_video_progress_slider.js @@ -329,7 +329,6 @@ function() { msg = ngettext('%(value)s second', '%(value)s seconds', value); break; } - // xss-lint: disable=javascript-interpolate return interpolate(msg, {value: value}, true); }; diff --git a/lms/djangoapps/teams/static/teams/js/views/edit_team_members.js b/lms/djangoapps/teams/static/teams/js/views/edit_team_members.js index 425d82e477..16aadbde49 100644 --- a/lms/djangoapps/teams/static/teams/js/views/edit_team_members.js +++ b/lms/djangoapps/teams/static/teams/js/views/edit_team_members.js @@ -52,7 +52,7 @@ _.each(this.model.get('membership'), function(membership) { // eslint-disable-next-line no-undef - dateJoined = interpolate( // xss-lint: disable=javascript-interpolate + dateJoined = interpolate( /* Translators: 'date' is a placeholder for a fuzzy, * relative timestamp (see: https://github.com/rmm5t/jquery-timeago) */ @@ -62,7 +62,7 @@ ); // eslint-disable-next-line no-undef - lastActivity = interpolate( // xss-lint: disable=javascript-interpolate + lastActivity = interpolate( /* Translators: 'date' is a placeholder for a fuzzy, * relative timestamp (see: https://github.com/rmm5t/jquery-timeago) */ diff --git a/scripts/xsslint/tests/test_linters.py b/scripts/xsslint/tests/test_linters.py index ce18bfea13..e3556fa800 100644 --- a/scripts/xsslint/tests/test_linters.py +++ b/scripts/xsslint/tests/test_linters.py @@ -403,27 +403,19 @@ class TestJavaScriptLinter(TestLinter): linter.check_javascript_file_is_safe(data['template'], results) self._validate_data_rules(data, results) - @data( - {'template': 'StringUtils.interpolate()', 'rule': None}, - {'template': 'HtmlUtils.interpolateHtml()', 'rule': None}, - {'template': 'interpolate(anything)', 'rule': JAVASCRIPT_LINTER_RULESET.javascript_interpolate}, - ) - def test_javascript_interpolate(self, data): - """ - Test check_javascript_file_is_safe with interpolate() - """ - linter = _build_javascript_linter() - results = FileResults('') - - linter.check_javascript_file_is_safe(data['template'], results) - - self._validate_data_rules(data, results) - @data( {'template': '_.escape(message)', 'rule': None}, - {'template': 'anything.escape(message)', 'rule': JAVASCRIPT_LINTER_RULESET.javascript_escape}, + {'template': 'anything.escape(message)', 'rule': None}, + {'template': 'anythingescape(message)', 'rule': None}, + {'template': '$escape(message)', 'rule': None}, + {'template': '_escape(message)', 'rule': None}, + {'template': 'escape(message)', 'rule': JAVASCRIPT_LINTER_RULESET.javascript_escape}, + {'template': '(escape(message))', 'rule': JAVASCRIPT_LINTER_RULESET.javascript_escape}, + {'template': ' escape(message))', 'rule': JAVASCRIPT_LINTER_RULESET.javascript_escape}, + {'template': 'window.escape(message)', 'rule': JAVASCRIPT_LINTER_RULESET.javascript_escape}, + {'template': '(window.escape(message)', 'rule': JAVASCRIPT_LINTER_RULESET.javascript_escape}, ) - def test_javascript_interpolate(self, data): + def test_javascript_escape(self, data): """ Test check_javascript_file_is_safe with interpolate() """ diff --git a/scripts/xsslint/xsslint/linters.py b/scripts/xsslint/xsslint/linters.py index d345483f0d..50ef4b0ba4 100644 --- a/scripts/xsslint/xsslint/linters.py +++ b/scripts/xsslint/xsslint/linters.py @@ -329,7 +329,6 @@ class JavaScriptLinter(BaseLinter): javascript_jquery_html='javascript-jquery-html', javascript_concat_html='javascript-concat-html', javascript_escape='javascript-escape', - javascript_interpolate='javascript-interpolate', ) def __init__(self, underscore_linter, javascript_skip_dirs=None): @@ -401,7 +400,6 @@ class JavaScriptLinter(BaseLinter): file_contents, "html", self.ruleset.javascript_jquery_html, no_caller_check, self._is_jquery_html_argument_safe, results ) - self._check_javascript_interpolate(file_contents, results) self._check_javascript_escape(file_contents, results) self._check_concat_with_html(file_contents, self.ruleset.javascript_concat_html, results) self.underscore_linter.check_underscore_file_is_safe(file_contents, results) @@ -435,37 +433,18 @@ class JavaScriptLinter(BaseLinter): expression = Expression(start_index) return expression - def _check_javascript_interpolate(self, file_contents, results): - """ - Checks that interpolate() calls are safe. - - Only use of StringUtils.interpolate() or HtmlUtils.interpolateText() - are safe. - - Arguments: - file_contents: The contents of the JavaScript file. - results: A file results objects to which violations will be added. - - """ - # Ignores calls starting with "StringUtils.", because those are safe - regex = re.compile(r"(? Date: Tue, 11 Feb 2020 13:59:11 -0800 Subject: [PATCH 3/4] Fix capa's static_url on devstack in new runtime --- openedx/core/djangoapps/xblock/runtime/shims.py | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/openedx/core/djangoapps/xblock/runtime/shims.py b/openedx/core/djangoapps/xblock/runtime/shims.py index c9c172b4d9..edf1d56951 100644 --- a/openedx/core/djangoapps/xblock/runtime/shims.py +++ b/openedx/core/djangoapps/xblock/runtime/shims.py @@ -11,6 +11,7 @@ from django.core.cache import cache from django.template import TemplateDoesNotExist from django.utils.functional import cached_property from fs.memoryfs import MemoryFS +from openedx.core.djangoapps.xblock.apps import get_xblock_app_config import six from edxmako.shortcuts import render_to_string @@ -270,7 +271,12 @@ class RuntimeShim(object): Seems only to be used by capa. Remove this if capa can be refactored. """ # TODO: Refactor capa to access this directly, don't bother the runtime. Then remove it from here. - return settings.STATIC_URL + static_url = settings.STATIC_URL + if static_url.startswith('/') and not static_url.startswith('//'): + # This is not a full URL - should start with https:// to support loading assets from an iframe sandbox + site_root_url = get_xblock_app_config().get_site_root_url() + static_url = site_root_url + static_url + return static_url @cached_property def user_is_staff(self): From 18c7d72843ff6b4c3fdb4ac03532eb33bc635aa7 Mon Sep 17 00:00:00 2001 From: Samuel Walladge Date: Tue, 5 May 2020 09:25:23 +0930 Subject: [PATCH 4/4] restructure code to pass lints --- .../spec/formula_equation_preview_spec.js | 7 ++- .../js/capa/src/formula_equation_preview.js | 49 ++++++++++--------- 2 files changed, 30 insertions(+), 26 deletions(-) diff --git a/common/static/js/capa/spec/formula_equation_preview_spec.js b/common/static/js/capa/spec/formula_equation_preview_spec.js index 5e9cf7a487..17c147a8d0 100644 --- a/common/static/js/capa/spec/formula_equation_preview_spec.js +++ b/common/static/js/capa/spec/formula_equation_preview_spec.js @@ -3,7 +3,8 @@ describe('escapeSelector', function() { var escapeSelector = window.escapeSelector; it('correctly escapes css', function() { - // tests borrowed from https://github.com/jquery/jquery/blob/3edfa1bcdc50bca41ac58b2642b12f3feee03a3b/test/unit/selector.js#L2030 + // tests borrowed from + // https://github.com/jquery/jquery/blob/3edfa1bcdc50bca41ac58b2642b12f3feee03a3b/test/unit/selector.js#L2030 expect(escapeSelector('-')).toEqual('\\-'); expect(escapeSelector('-a')).toEqual('-a'); expect(escapeSelector('--')).toEqual('--'); @@ -20,7 +21,9 @@ describe('escapeSelector', function() { // This is the important one; xblocks and course ids often contain invalid characters, so if these aren't // escaped when embedding/searching xblock IDs using css selectors, bad things happen. expect(escapeSelector('course-v1:edX+DemoX+Demo_Course')).toEqual('course-v1\\:edX\\+DemoX\\+Demo_Course'); - expect(escapeSelector('block-v1:edX+DemoX+Demo_Course+type@sequential+block')).toEqual('block-v1\\:edX\\+DemoX\\+Demo_Course\\+type\\@sequential\\+block'); + expect(escapeSelector('block-v1:edX+DemoX+Demo_Course+type@sequential+block')).toEqual( + 'block-v1\\:edX\\+DemoX\\+Demo_Course\\+type\\@sequential\\+block' + ); }); }); diff --git a/common/static/js/capa/src/formula_equation_preview.js b/common/static/js/capa/src/formula_equation_preview.js index 1838bf498b..8456171972 100644 --- a/common/static/js/capa/src/formula_equation_preview.js +++ b/common/static/js/capa/src/formula_equation_preview.js @@ -1,34 +1,35 @@ function escapeSelector(id) { - // Wrapper around window.CSS.escape that uses a fallback method if CSS.escape is not available. - // This is designed to serialize a string to be used as a valid css selector. See https://drafts.csswg.org/cssom/#the-css.escape()-method - // For example, can be used with xblock and course ids, which often contain invalid characters that must be escaped - // to function properly in css selectors. - + 'use strict'; + // Wrapper around window.CSS.escape that uses a fallback method if CSS.escape is not available. This is designed to + // serialize a string to be used as a valid css selector. See + // https://drafts.csswg.org/cssom/#the-css.escape()-method For example, this can be used with xblock and course ids, + // which often contain invalid characters that must be escaped to function properly in css selectors. // TODO: if this escaping is also required elsewhere, it may be useful to add a global CSS.escape polyfill and // use that directly. - if (window.CSS && window.CSS.escape) { - return CSS.escape(id); - } else { - // CSS escape alternative borrowed from https://api.jquery.com/jQuery.escapeSelector/ source. When we upgrade to jQuery 3.0, we can use $.escapeSelector() instead of this shim escapeSelector function. - // source: https://github.com/jquery/jquery/blob/3edfa1bc/src/selector/escapeSelector.js - // CSS string/identifier serialization - // https://drafts.csswg.org/cssom/#common-serializing-idioms - var rcssescape = /([\0-\x1f\x7f]|^-?\d)|^-$|[^\x80-\uFFFF\w-]/g; - function fcssescape( ch, asCodePoint ) { - if ( asCodePoint ) { - // U+0000 NULL becomes U+FFFD REPLACEMENT CHARACTER - if ( ch === "\0" ) { - return "\uFFFD"; - } - // Control characters and (dependent upon position) numbers get escaped as code points - return ch.slice( 0, -1 ) + "\\" + ch.charCodeAt( ch.length - 1 ).toString( 16 ) + " "; + // CSS string/identifier serialization https://drafts.csswg.org/cssom/#common-serializing-idioms + // This code borrowed from https://api.jquery.com/jQuery.escapeSelector/ (source: + // https://github.com/jquery/jquery/blob/3edfa1bc/src/selector/escapeSelector.js). When we upgrade to jQuery 3.0, we + // can use $.escapeSelector() instead of this shim escapeSelector function. + var rcssescape = /([\0-\x1f\x7f]|^-?\d)|^-$|[^\x80-\uFFFF\w-]/g; // eslint-disable-line no-control-regex + function fcssescape(ch, asCodePoint) { + if (asCodePoint) { + // U+0000 NULL becomes U+FFFD REPLACEMENT CHARACTER + if (ch === '\0') { + return '\uFFFD'; } - // Other potentially-special ASCII characters get backslash-escaped - return "\\" + ch; + // Control characters and (dependent upon position) numbers get escaped as code points + return ch.slice(0, -1) + '\\' + ch.charCodeAt(ch.length - 1).toString(16) + ' '; } + // Other potentially-special ASCII characters get backslash-escaped + return '\\' + ch; + } + + if (window.CSS && window.CSS.escape) { + return window.CSS.escape(id); + } else { // ensure string and then run the replacements - return (id + "").replace(rcssescape, fcssescape); + return (id + '').replace(rcssescape, fcssescape); } }