mozilla / eslint-plugin-no-unsanitized

Custom ESLint rule to disallows unsafe innerHTML, outerHTML, insertAdjacentHTML and alike
Mozilla Public License 2.0
228 stars 37 forks source link

Consider allowing method name checks over getSource check #41

Closed jonathanKingston closed 7 years ago

jonathanKingston commented 7 years ago

Currently the code checks for context.getSource(node.callee) for write and writeln but node.callee.property.name for insertAdjacentHTML.

This might be a very breaking code change to add to make write use the property.name as some code bases use this method name. Making this change however will catch any other reference to document using the write method which is somewhat a common pattern in non-malice™ code:

let d = document;
d.writeln("blah");
d.writeln("blah");
d.writeln("blah");
d.writeln("blah");

Without this making the code configurable will be harder #29

mozfreddyb commented 7 years ago

Your example makes sense, but I doubt a lot of people will save a reference to document in unminified code. My main concern is that write is such a generic name. Maybe we should check some popular libraries, to see how common it is?

On the other hand, our worry about false positives should not be so big, if people can just disable eslint with a tiny comment on top of the line.

I'm not sure what my position here is.

jonathanKingston commented 7 years ago

I'm not sure what my position here is.

Mine neither, I could check big codebases on how often document assignment is within unminified code. I have seen it done myself certainly with frames:

const parentDoc = document.parent.document;
parentDoc.write("a");
mozfreddyb commented 7 years ago

Another data point: There's lots of .write() for streams (stdin, stdout) and files in our js and jsm files according to [searchfox](http://searchfox.org/mozilla-central/search?q=%5C.write%5C(&case=false&regexp=true&path=.js*)

mozfreddyb commented 7 years ago

This will be fixed by the new format allowing regexes