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

Add a warning for dangerous dom injection with jQuery #1182

Closed
wagnerand opened this issue Mar 15, 2017 · 8 comments
Closed

Add a warning for dangerous dom injection with jQuery #1182

wagnerand opened this issue Mar 15, 2017 · 8 comments

Comments

@wagnerand
Copy link
Member

jQuery offers some methods that allow unescaped DOM injection. Similar to innerHTML this allows remote code execution, which is not allowed according to our review policies.

The linter should raise a warning for occurrences of /(((append|prepend)(To)?)|(insert)?[Bb]efore|(insert)?[aA]fter|html|globalEval|getScript|load)\s*\(/ being called on a jQuery object.

@Rob--W
Copy link
Member

Rob--W commented Mar 15, 2017

getJSON|ajax should also be flagged. The reviewer must check whether the parameter is not an external string, and if it is, that it cannot be a JSONP request (jsonp:false).

@mstriemer
Copy link
Contributor

I think we should be able to accomplish this without resorting to regexes. It looks like there's a project [1] that might hit some of these but we'd need some more rules too. Could be a good template to start from though.

This seems like something that could be useful as a standalone eslint plugin for unsafe-jquery usages.

[1] https://github.com/dgraham/eslint-plugin-jquery

@jonathanKingston
Copy link

I haven't audited eslint-plugin-jquery however it would be worth waiting for our work on making @mozfreddyb's rule for innerHTML more flexible.

The idea is that we should be able to have a large blocklist of methods and attributes that are unsafe or require the same escaping techniques the innerHTML rules use.

We are working on having a regex filter for objects too as append() will likely give way too many false positives.

@mozfreddyb
Copy link

Also pointing to the domxss wiki at https://github.com/wisec/domxsswiki/
especially the articles on finding dom xss and and jQuery

@EnTeQuAk
Copy link
Contributor

EnTeQuAk commented Apr 3, 2017

Thanks @jonathanKingston for the hint, let's see what improvements will come to no-unsafe-innerhtml but I might start working on this nonetheless since we need that in addons-linter soon. Once there's a better upstream solution we can easily replace our current implementation later and check for compatibility.

@jonathanKingston
Copy link

@EnTeQuAk should be fairly soon, just waiting on the resolution here: mozilla/eslint-plugin-no-unsanitized#45

@jonathanKingston
Copy link

As an update eslint-plugin-no-unsanitized is now published and usable for this. I was going to work on a jQuery setup however don't have the time. If you use the ESLint plugin we can merge in your jQuery setup if you beat us to it. It's covered in: mozilla/eslint-plugin-no-unsanitized#23

@EnTeQuAk
Copy link
Contributor

Going to close this in favor of #1273 as it'll fix most of the mentioned issues afaik.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

8 participants