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

samuelallan72 · 2020-02-07T04:36:29Z

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. (If this is a common thing, would suggest adding a global CSS.escape polyfill instead of a local fallback)

This also fixes bugs in tests for the xsslint tool that were causing tests to fail. The javascript-interpolate lint rule was removed altogether since it was deemed no longer necessary.

JIRA tickets: OSPR-4082

Dependencies: None

Sandbox URL: TBD - sandbox is being provisioned.

Merge deadline: "None"

Testing instructions:

Load a formula equation capa problem xblock from blockstore (ie. where the xblock ID doesn't go through the deprecated html_id function)
verify that everything loads correctly, the mathjax preview is rendered and updates with the text input, and the spinner shows/hides as expected
also check that the xsslint test changes make sense

Reviewers

@bradenmacdonald
edX reviewer[s] TBD

Settings

openedx-webhooks · 2020-02-07T04:36:42Z

Thanks for the pull request, @swalladge! I've created OSPR-4082 to keep track of it in JIRA. JIRA is a place for product owners to prioritize feature reviews by the engineering development teams.

Feel free to add as much of the following information to the ticket:

supporting documentation
edx-code email threads
timeline information ("this must be merged by XX date", and why that is)
partner information ("this is a course on edx.org")
any other information that can help Product understand the context for the PR

All technical communication about the code itself will still be done via the GitHub pull request interface. As a reminder, our process documentation is here.

natabene · 2020-02-07T21:36:06Z

@swalladge Thank you for your contribution. Please let me know once it is ready for our review.

samuelallan72 · 2020-02-08T05:03:24Z

scripts/xsslint/tests/test_linters.py

    )
-    def test_javascript_interpolate(self, data):
+    def test_javascript_escape(self, data):


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

bradenmacdonald

That does the trick! Just one request.

Note that if loading the xblock through some sandbox in not the usual way, we have had issues because the capa problem by default loads some resources through a relative url. This temporary patch to edx-platform should allow testing this PR in this context:

Turns out there is a simple, permanent fix that we should include in this PR. I've pushed it as a commit - hope you don't mind: 40237ec

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

bradenmacdonald · 2020-02-14T23:33:37Z

I'm not sure about the XSS lint changes, but the fix to capa and the static asset part are working well. 👍

samuelallan72 · 2020-02-16T21:58:49Z

@natabene This is ready for edX review. :)

natabene · 2020-02-18T16:56:37Z

@swalladge Thanks for letting me know!

ormsbee

I've left a note for @robrap to comment on the linting, but this seems reasonable to me. Please squash and write up the nice commit message.

scripts/xsslint/xsslint/linters.py

ormsbee · 2020-04-28T05:52:42Z

scripts/xsslint/xsslint/linters.py

-        # Ignores calls starting with "_.", because those are safe
-        regex = regex = re.compile(r"(?<!_).escape\(")
+        # Ensure that calls to fooescape or foo.escape are ignored; they aren't the dangerous escape functions.
+        regex = re.compile(r"(?:^|(?<![\w.$]))escape\(")


@robrap: Could you please take a look at this adjustment to the XSS linting? It seems reasonable to me, but you have a lot more context on this than I do.

scripts/xsslint/tests/test_linters.py

scripts/xsslint/xsslint/linters.py

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

robrap

Minor comments and suggestions. Thank you for this.

No worries about leaving the utility where it is. Proper escaping and linting is hard, which is why the docs were vague to begin with regarding CSS escaping. I imagine it is a security blindspot for many people, but not sure. Hopefully we aren't popping in user-generated data in this way.

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

scripts/xsslint/tests/test_linters.py

scripts/xsslint/xsslint/linters.py

natabene · 2020-04-30T10:24:57Z

@swalladge Please let me know once it is ready for another look.

samuelallan72 · 2020-05-01T04:13:32Z

@natabene Thanks. Yes this is ready for another review pass. CC @robrap

robrap

Thank you! The linting changes all look good to me.

Maybe update the PR description and commit comment to mention that we removed the linting rule for javascript-interpolate because it was deemed unnecessary.

Also, once this lands, we should remove that rule from the docs: https://github.com/edx/edx-documentation/blame/04255d94ce4df1c7aff1fd5569d5ea4c207cfb3b/en_us/developers/source/preventing_xss/preventing_xss.rst#L1054-L1060

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.

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.

samuelallan72 · 2020-05-04T00:02:16Z

@robrap @ormsbee Thanks for your reviews. Commits have been squashed to logical changes and neater messages written. Ready for another review pass. :)

robrap

Still approve, with 2 caveats:

Fixing the Diff Quality errors. See the diff_quality_eslint tab under the report: https://build.testeng.edx.org/job/edx-platform-quality-pipeline-pr/15631/Diff_20Quality_20Report/
I am not planning to review the 3rd commit Fix capa's static_url on devstack in new runtime. Hopefully someone else has that. :)
Thanks for all the linting clean-up.

natabene · 2020-05-04T16:57:57Z

@ormsbee Can you give this another look?

samuelallan72 · 2020-05-05T00:19:41Z

@robrap thanks for your time reviewing this. diff quality errors fixed. 😄

…d-selector

edx-status-bot · 2020-06-16T00:55:09Z

Your PR has finished running tests. There were no failures.

openedx-webhooks · 2020-06-19T16:13:23Z

@swalladge 🎉 Your pull request was merged!

Please take a moment to answer a two question survey so we can improve your experience in the future.

ormsbee · 2020-06-19T16:21:00Z

Argh. I forgot to squash + merge >_<

edx-pipeline-bot · 2020-06-19T17:06:06Z

EdX Release Notice: This PR has been deployed to the staging environment in preparation for a release to production.

edx-pipeline-bot · 2020-06-19T17:56:57Z

EdX Release Notice: This PR has been deployed to the production environment.

samuelallan72 · 2020-06-22T00:40:13Z

Thanks @ormsbee! 😄

openedx-webhooks added needs triage open-source-contribution PR author is not from Axim or 2U labels Feb 7, 2020

openedx-webhooks added awaiting prioritization and removed needs triage labels Feb 7, 2020

samuelallan72 commented Feb 8, 2020

View reviewed changes

bradenmacdonald requested changes Feb 11, 2020

View reviewed changes

common/static/js/capa/src/formula_equation_preview.js Outdated Show resolved Hide resolved

ormsbee requested changes Apr 28, 2020

View reviewed changes

samuelallan72 force-pushed the samuel/fix-unescaped-selector branch 2 times, most recently from 86def2c to bc1457a Compare April 28, 2020 07:25

robrap reviewed Apr 28, 2020

View reviewed changes

robrap reviewed Apr 29, 2020

View reviewed changes

samuelallan72 mentioned this pull request Apr 30, 2020

SE-2176 Update doc comments for accuracy openedx/edx-ui-toolkit#109

Merged

2 tasks

samuelallan72 requested a review from a team May 1, 2020 04:10

robrap approved these changes May 1, 2020

View reviewed changes

Samuel Walladge and others added 3 commits May 4, 2020 09:27

Fix capa's static_url on devstack in new runtime

fe06bc8

samuelallan72 force-pushed the samuel/fix-unescaped-selector branch from 8e17d04 to fe06bc8 Compare May 4, 2020 00:00

robrap approved these changes May 4, 2020

View reviewed changes

openedx-webhooks added engineering review and removed awaiting prioritization labels May 4, 2020

bradenmacdonald approved these changes May 4, 2020

View reviewed changes

restructure code to pass lints

18c7d72

Merge remote-tracking branch 'origin/master' into samuel/fix-unescape…

43f0cd7

…d-selector

ormsbee approved these changes Jun 19, 2020

View reviewed changes

ormsbee merged commit 0b4cf7e into openedx:master Jun 19, 2020

openedx-webhooks added merged and removed engineering review labels Jan 21, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

samuelallan72 commented Feb 7, 2020 •

edited

Loading

openedx-webhooks commented Feb 7, 2020

natabene commented Feb 7, 2020

samuelallan72 Feb 8, 2020

bradenmacdonald left a comment

bradenmacdonald commented Feb 14, 2020

samuelallan72 commented Feb 16, 2020

natabene commented Feb 18, 2020

ormsbee left a comment

ormsbee Apr 28, 2020

robrap left a comment

natabene commented Apr 30, 2020

samuelallan72 commented May 1, 2020

robrap left a comment

samuelallan72 commented May 4, 2020

robrap left a comment

natabene commented May 4, 2020

samuelallan72 commented May 5, 2020

edx-status-bot commented Jun 16, 2020

openedx-webhooks commented Jun 19, 2020

ormsbee commented Jun 19, 2020 •

edited

Loading

edx-pipeline-bot commented Jun 19, 2020

edx-pipeline-bot commented Jun 19, 2020

samuelallan72 commented Jun 22, 2020

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

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

Conversation

samuelallan72 commented Feb 7, 2020 • edited Loading

openedx-webhooks commented Feb 7, 2020

natabene commented Feb 7, 2020

samuelallan72 Feb 8, 2020

Choose a reason for hiding this comment

bradenmacdonald left a comment

Choose a reason for hiding this comment

bradenmacdonald commented Feb 14, 2020

samuelallan72 commented Feb 16, 2020

natabene commented Feb 18, 2020

ormsbee left a comment

Choose a reason for hiding this comment

ormsbee Apr 28, 2020

Choose a reason for hiding this comment

robrap left a comment

Choose a reason for hiding this comment

natabene commented Apr 30, 2020

samuelallan72 commented May 1, 2020

robrap left a comment

Choose a reason for hiding this comment

samuelallan72 commented May 4, 2020

robrap left a comment

Choose a reason for hiding this comment

natabene commented May 4, 2020

samuelallan72 commented May 5, 2020

edx-status-bot commented Jun 16, 2020

openedx-webhooks commented Jun 19, 2020

ormsbee commented Jun 19, 2020 • edited Loading

edx-pipeline-bot commented Jun 19, 2020

edx-pipeline-bot commented Jun 19, 2020

samuelallan72 commented Jun 22, 2020

samuelallan72 commented Feb 7, 2020 •

edited

Loading

ormsbee commented Jun 19, 2020 •

edited

Loading