Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 5 commits into from
Jun 19, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 5 additions & 5 deletions common/lib/xmodule/xmodule/js/src/capa/schematic.js
Original file line number Diff line number Diff line change
Expand Up @@ -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]);
}
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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++)
Expand Down Expand Up @@ -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})>',
Expand Down Expand Up @@ -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);
Expand Down
1 change: 0 additions & 1 deletion common/lib/xmodule/xmodule/js/src/sequence/display.js
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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);
};

Expand Down
29 changes: 29 additions & 0 deletions common/static/js/capa/spec/formula_equation_preview_spec.js
Original file line number Diff line number Diff line change
@@ -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;
Expand Down
37 changes: 36 additions & 1 deletion common/static/js/capa/src/formula_equation_preview.js
Original file line number Diff line number Diff line change
@@ -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).
Expand All @@ -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

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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)
*/
Expand All @@ -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)
*/
Expand Down
8 changes: 7 additions & 1 deletion openedx/core/djangoapps/xblock/runtime/shims.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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):
Expand Down
28 changes: 10 additions & 18 deletions scripts/xsslint/tests/test_linters.py
Original file line number Diff line number Diff line change
Expand Up @@ -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},
robrap marked this conversation as resolved.
Show resolved Hide resolved
)
def test_javascript_interpolate(self, data):
def test_javascript_escape(self, data):
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that this was renamed, because previously it was the same name as the previous test, and was shadowing that test (which also turned out to be failing)...

"""
Test check_javascript_file_is_safe with interpolate()
"""
Expand Down
29 changes: 4 additions & 25 deletions scripts/xsslint/xsslint/linters.py
Original file line number Diff line number Diff line change
Expand Up @@ -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):
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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))
Expand Down
1 change: 0 additions & 1 deletion scripts/xsslint_thresholds.json
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down