Add warning about use of unsafe innerHTML #1140

EnTeQuAk · 2017-02-23T12:47:45Z

Fixes #811 and references (but doesn't fix it yet imho) #1113

This PR improves 3rd party rule support where needed along the way. Also adapts other rules a bit, e.g to correctly resolve the current working directory. See the actual commits for more details.

This now uses CLIEngine.executeOnText instead of eslint.verify to make sure plugins and processors are properly loaded. We could have done that ourselves but it felt more appropriate to just use that API. This messes a bit with the current working directory (now absolute paths are present in context.getFilename) but that's reasonable.

…e-innerhtml and basic tests.

EnTeQuAk · 2017-02-23T12:48:17Z

package.json

@@ -72,6 +72,7 @@
    "dispensary": "0.10.3",
    "es6-promisify": "5.0.0",
    "eslint": "3.16.0",
+    "eslint-plugin-no-unsafe-innerhtml": "EnTeQuAk/eslint-plugin-no-unsafe-innerhtml#4b6b606d50",


There is an upstream PR that needs merging first (mozilla/eslint-plugin-no-unsanitized#16) so this will be updated once that happens.

coveralls · 2017-02-23T12:52:19Z

Coverage remained the same at 100.0% when pulling cece6c3 on feature/811-add-innerhtml-rule into f1d8368 on master.

coveralls · 2017-02-23T13:01:49Z

Coverage remained the same at 100.0% when pulling 8306bd7 on feature/811-add-innerhtml-rule into 0853ce4 on master.

EnTeQuAk · 2017-02-23T13:10:12Z

src/rules/javascript/only-prefs-in-defaults.js

 export default {
  create(context) {
-    var filename = context.getFilename();
+    var relPath = path.relative(process.cwd(), context.getFilename());



This might need a bit more work and testing. I'd love to forward the actual package path to the rules eventually but for now this seems to work just fine. (also this rule will be removed anyway but it's a good example)

muffinresearch · 2017-02-23T13:16:46Z

src/scanners/javascript.js

        allowInlineConfig: false,
        filename: this.filename,
+        // Avoid loading the addons-linter .eslintrc file
+        useEslintrc: false,


Presumably this change also means that eslintrc's nested in a webext can't override anything. It would be good to add an explicit test for something like that.

I added #1141 for this, could be a rabbit-hole so I'd like to do that as a separate task.

Ok, that's fine we should try and avoid a release until that's checked - a quick local check would suffice though.

I checked locally and couldn't find an obvious way to overwrite default checks (checked with innerHTML)

That doesn't work without that option too though as far as I see.

muffinresearch · 2017-02-23T14:02:09Z

src/rules/javascript/only-prefs-in-defaults.js

 export default {
  create(context) {
-    var filename = context.getFilename();
+    var relPath = path.relative(process.cwd(), context.getFilename());


I could see this could potentially break with using the linter via web-ext.

Yeah, I want to make this configurable and maybe forward the actual package path, not sure yet, see #1140 (comment) on that.

Generally, this is how eslint does things anyway so I don't expect there to be any major problems.

muffinresearch · 2017-02-24T10:30:58Z

Looks pretty good. I like how the rule doesn't worry about static values.

I notice it doesn't catch bracket syntax:

foo['innerHTML'] = "<a href='"+url+"'>About</a>";

But it's a good start in any case.

r+wc

muffinresearch · 2017-02-24T10:36:22Z

One other thing - I see the rule allows for specific use of a escaping functions that would also in our case mean that if you wrap something in a function with the right name you would bypass the warning.

It might be nice to be able to disable that for our case (probably an upstream patch). Or alternatively we could look to making some policy where we utiise that feature and define a list of acceptable sanitization function names. Either way that probably needs to be discussed with Andreas etc.

EnTeQuAk · 2017-02-27T06:19:35Z

I notice it doesn't catch bracket syntax

I'd say, let's file this at https://github.com/mozfreddyb/eslint-plugin-no-unsafe-innerhtml to get this in upstream.

One other thing - I see the rule allows for specific use of a escaping functions that would also in our case mean that if you wrap something in a function with the right name you would bypass the warning.

It might be nice to be able to disable that for our case (probably an upstream patch). Or alternatively we could look to making some policy where we utiise that feature and define a list of acceptable sanitization function names. Either way that probably needs to be discussed with Andreas etc.

Can you elaborate on this a little bit more / maybe file appropriate tickets? I feel like I don't get what you mean right now.

EnTeQuAk · 2017-02-27T06:20:52Z

I merged it, let's improve this incrementally from now on. I feel like it catches a lot and all rules should be subject to incremental improvement and re-evaluation.

kewisch · 2017-02-27T16:14:32Z

It might be nice to be able to disable that for our case (probably an upstream patch). Or alternatively we could look to making some policy where we utiise that feature and define a list of acceptable sanitization function names. Either way that probably needs to be discussed with Andreas etc.

Personally I think we can catch this with policy. Yes, the escapeHTML function would allow circumventing the warning, but escapeHTML is indeed the most common function name for functions that escape html. If wanted we could add another rule that makes sure the escapeHTML function does what it is supposed to do (by requiring the one we have in MDN). @wagnerand ?

EnTeQuAk added 5 commits February 22, 2017 17:20

Add eslint-plugin-no-unsafe-innerhtml dependency.

e13aac1

Implement better 3rd party support to JavaScriptScanner, add no-unsaf…

2a5e600

…e-innerhtml and basic tests.

Implement remaining tests

1075cfb

Fix most tests, use my rule-fork until upstream merges PR.

66e94f7

Fix only-refs-in-defaults test to resolve relative path correctly.

cece6c3

EnTeQuAk added the state:pull_request_ready label Feb 23, 2017

EnTeQuAk commented Feb 23, 2017

View reviewed changes

EnTeQuAk requested review from muffinresearch and tofumatt February 23, 2017 12:51

Merge branch 'master' into feature/811-add-innerhtml-rule

8306bd7

EnTeQuAk commented Feb 23, 2017

View reviewed changes

muffinresearch reviewed Feb 23, 2017

View reviewed changes

EnTeQuAk mentioned this pull request Feb 23, 2017

Add test to make sure a webext provided eslintrc can't override anything. #1141

Open

muffinresearch reviewed Feb 23, 2017

View reviewed changes

EnTeQuAk merged commit bde99e1 into master Feb 27, 2017

EnTeQuAk deleted the feature/811-add-innerhtml-rule branch February 27, 2017 06:20

EnTeQuAk removed the state:pull_request_ready label Feb 27, 2017

EnTeQuAk mentioned this pull request Mar 27, 2017

Revisit whether there's a better way to consume eslint rules #1113

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add warning about use of unsafe innerHTML #1140

Add warning about use of unsafe innerHTML #1140

EnTeQuAk commented Feb 23, 2017 •

edited

EnTeQuAk Feb 23, 2017

coveralls commented Feb 23, 2017 •

edited

coveralls commented Feb 23, 2017 •

edited

EnTeQuAk Feb 23, 2017

muffinresearch Feb 23, 2017

muffinresearch Feb 23, 2017

EnTeQuAk Feb 23, 2017 •

edited

muffinresearch Feb 24, 2017

EnTeQuAk Feb 24, 2017

EnTeQuAk Feb 24, 2017

muffinresearch Feb 23, 2017

EnTeQuAk Feb 23, 2017

muffinresearch commented Feb 24, 2017

muffinresearch commented Feb 24, 2017 •

edited

EnTeQuAk commented Feb 27, 2017

EnTeQuAk commented Feb 27, 2017

kewisch commented Feb 27, 2017

Add warning about use of unsafe innerHTML #1140

Add warning about use of unsafe innerHTML #1140

Conversation

EnTeQuAk commented Feb 23, 2017 • edited

Choose a reason for hiding this comment

coveralls commented Feb 23, 2017 • edited

coveralls commented Feb 23, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EnTeQuAk Feb 23, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

muffinresearch commented Feb 24, 2017

muffinresearch commented Feb 24, 2017 • edited

EnTeQuAk commented Feb 27, 2017

EnTeQuAk commented Feb 27, 2017

kewisch commented Feb 27, 2017

EnTeQuAk commented Feb 23, 2017 •

edited

coveralls commented Feb 23, 2017 •

edited

coveralls commented Feb 23, 2017 •

edited

EnTeQuAk Feb 23, 2017 •

edited

muffinresearch commented Feb 24, 2017 •

edited