From 71fcf6e72568448f261b9d42cee92e4fc0f4b059 Mon Sep 17 00:00:00 2001 From: Samuel Walladge Date: Fri, 7 Feb 2020 15:58:46 +1030 Subject: [PATCH] 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"(?