Merge pull request openedx#23039 from open-craft/samuel/fix-unescaped…

…-selector

SE-2176 Fix elem not selected if id contains special chars

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(
 		'<Wire ({x},{y}) ({x_plus_dx},{y_plus_dy})>',
@@ -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);

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);

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);
             };
 

common/static/js/capa/spec/formula_equation_preview_spec.js

@@ -1,3 +1,32 @@
+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;

common/static/js/capa/src/formula_equation_preview.js

@@ -1,3 +1,38 @@
+function escapeSelector(id) {
+    '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.
+
+    // 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';
+            }
+            // 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);
+    }
+}
+
 var formulaEquationPreview = {
     minDelay: 300,  // Minimum time between requests sent out.
     errorDelay: 1500  // Wait time before showing error (prevent frustration).
@@ -13,7 +48,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
 

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)
                              */

openedx/core/djangoapps/xblock/runtime/shims.py

@@ -11,6 +11,7 @@
 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 @@ def STATIC_URL(self):
         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):

scripts/xsslint/tests/test_linters.py

@@ -403,27 +403,19 @@ def test_jquery_html(self, data):
         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()
         """

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 @@ def check_javascript_file_is_safe(self, file_contents, results):
             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 @@ def _get_expression_for_function(self, file_contents, function_start_match):
             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"(?<!StringUtils).interpolate\(")
-        for function_match in regex.finditer(file_contents):
-            expression = self._get_expression_for_function(file_contents, function_match)
-            results.violations.append(ExpressionRuleViolation(self.ruleset.javascript_interpolate, expression))
-
     def _check_javascript_escape(self, file_contents, results):
         """
-        Checks that only necessary escape() are used.
-
-        Allows for _.escape(), although this shouldn't be the recommendation.
+        Checks that escape() is not used. escape() is not recommended.
+        ref. https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/escape
 
         Arguments:
             file_contents: The contents of the JavaScript file.
             results: A file results objects to which violations will be added.
 
         """
-        # Ignores calls starting with "_.", because those are safe
-        regex = regex = re.compile(r"(?<!_).escape\(")
+        # Regex to match uses of escape() or window.escape().
+        regex = re.compile(r"(?:^|(?<=window\.)|(?<![\w.$]))escape\(")
         for function_match in regex.finditer(file_contents):
             expression = self._get_expression_for_function(file_contents, function_match)
             results.violations.append(ExpressionRuleViolation(self.ruleset.javascript_escape, expression))

scripts/xsslint_thresholds.json

@@ -2,7 +2,6 @@
     "rules": {
         "javascript-concat-html": 100,
         "javascript-escape": 5,
-        "javascript-interpolate": 9,
         "javascript-jquery-append": 50,
         "javascript-jquery-html": 112,
         "javascript-jquery-insert-into-target": 18,