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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 5 commits into from
Jun 19, 2020
Merged

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
9c4b458
Fix elem not selected if id contains special chars
Feb 7, 2020
71fcf6e
Fix issues with xss linters
Feb 7, 2020
fe06bc8
Fix capa's static_url on devstack in new runtime
bradenmacdonald Feb 11, 2020
18c7d72
restructure code to pass lints
May 4, 2020
43f0cd7
Merge remote-tracking branch 'origin/master' into samuel/fix-unescape…
Jun 16, 2020
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
Prev Previous commit
Next Next commit
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.
  • Loading branch information
Samuel Walladge committed May 3, 2020
commit 71fcf6e72568448f261b9d42cee92e4fc0f4b059
10 changes: 5 additions & 5 deletions common/lib/xmodule/xmodule/js/src/capa/schematic.js
View file
Open in desktop
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
View file
Open in desktop
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
1 change: 0 additions & 1 deletion common/lib/xmodule/xmodule/js/src/video/06_video_progress_slider.js
View file
Open in desktop
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
4 changes: 2 additions & 2 deletions lms/djangoapps/teams/static/teams/js/views/edit_team_members.js
View file
Open in desktop
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
28 changes: 10 additions & 18 deletions scripts/xsslint/tests/test_linters.py
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@
"rules": {
"javascript-concat-html": 142,
"javascript-escape": 7,
"javascript-interpolate": 23,
"javascript-jquery-append": 68,
"javascript-jquery-html": 146,
"javascript-jquery-insert-into-target": 18,
Expand Down