Add warning about use of unsafe innerHTML

package.json

@@ -72,6 +72,7 @@
     "dispensary": "0.10.3",
     "es6-promisify": "5.0.0",
     "eslint": "3.16.1",
+    "eslint-plugin-no-unsafe-innerhtml": "EnTeQuAk/eslint-plugin-no-unsafe-innerhtml#4b6b606d50",
     "esprima": "3.1.3",
     "first-chunk-stream": "2.0.0",
     "postcss": "5.2.15",

src/const.js

@@ -20,6 +20,7 @@ export const ESLINT_RULE_MAPPING = {
   'shallow-wrapper': ESLINT_WARNING,
   'webextension-api': ESLINT_WARNING,
   'widget-module': ESLINT_WARNING,
+  'no-unsafe-innerhtml/no-unsafe-innerhtml': ESLINT_WARNING,
 };
 
 export const VALIDATION_ERROR = 'error';

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

@@ -1,12 +1,15 @@
+import path from 'path';
+
 import { ONLY_PREFS_IN_DEFAULTS } from 'messages/javascript';
 import { getRootExpression } from 'utils';
 
+
 export default {
   create(context) {
-    var filename = context.getFilename();
+    var relPath = path.relative(process.cwd(), context.getFilename());
 
     // This rule only applies to files in defaults/preferences
-    if (filename.indexOf('defaults/preferences/') === 0) {
+    if (path.dirname(relPath).startsWith('defaults/preferences')) {
       return {
         CallExpression: function(node) {
           var root = getRootExpression(node);

src/scanners/javascript.js

@@ -30,54 +30,70 @@ export default class JavaScriptScanner {
     _messages=messages,
   }={}) {
     return new Promise((resolve) => {
-      // ESLint is synchronous and doesn't accept streams, so we need to
-      // pass it the entire source file as a string.
-      var eslint = _ESLint.linter;
-
-      for (const name in _rules) {
-        this._rulesProcessed++;
-        eslint.defineRule(name, _rules[name]);
-      }
-
-      var report = eslint.verify(this.code, {
+      var cli = new _ESLint.CLIEngine({
         env: { es6: true },
-        parserOptions: { ecmaVersion: 2017 },
+        baseConfig: {
+          parserOptions: { ecmaVersion: 2017 },
+          settings: {
+            addonMetadata: this.options.addonMetadata,
+          },
+        },
         ignore: false,
         rules: _ruleMapping,
-        settings: {
-          addonMetadata: this.options.addonMetadata,
-        },
-      }, {
+        plugins: ['no-unsafe-innerhtml'],
         allowInlineConfig: false,
         filename: this.filename,
+        // Avoid loading the addons-linter .eslintrc file
+        useEslintrc: false,
       });
 
-      for (const message of report) {
-        // Fatal error messages (like SyntaxErrors) are a bit different, we
-        // need to handle them specially.
-        if (message.fatal === true) {
-          message.message = _messages.JS_SYNTAX_ERROR.code;
-        }
+      for (const name in _rules) {
+        this._rulesProcessed++;
+        _ESLint.linter.defineRule(name, _rules[name]);
+      }
 
-        if (typeof message.message === 'undefined') {
-          throw new Error(singleLineString`JS rules must pass a valid message as
-                          the second argument to context.report()`);
-        }
+      // ESLint is synchronous and doesn't accept streams, so we need to
+      // pass it the entire source file as a string.
+      var report = cli.executeOnText(this.code, this.filename, true);
 
-        // Fallback to looking up the message object by the message
-        var messageObj = _messages[message.message];
-        var code = message.message;
-
-        this.linterMessages.push({
-          code: code,
-          column: message.column,
-          description: messageObj.description,
-          file: this.filename,
-          line: message.line,
-          message: messageObj.message,
-          sourceCode: message.source,
-          type: ESLINT_TYPES[message.severity],
-        });
+      for (const result of report.results) {
+        for (const message of result.messages) {
+          // Fatal error messages (like SyntaxErrors) are a bit different, we
+          // need to handle them specially.
+          if (message.fatal === true) {
+            message.message = _messages.JS_SYNTAX_ERROR.code;
+          }
+
+          if (typeof message.message === 'undefined') {
+            throw new Error(
+              singleLineString`JS rules must pass a valid message as
+              the second argument to context.report()`);
+          }
+
+          // Fallback to looking up the message object by the message
+          var code = message.message;
+
+          // Support 3rd party eslint rules that don't have our internal
+          // message structure.
+          if (_messages.hasOwnProperty(code)) {
+            var shortDescription = _messages[code].message;
+            var description = _messages[code].description;
+          } else {
+            var shortDescription = code;
+            var description = null;
+          }
+
+          this.linterMessages.push({
+            code: code,
+            column: message.column,
+            description: description,
+            file: this.filename,
+            line: message.line,
+            message: shortDescription,
+            sourceCode: message.source,
+            type: ESLINT_TYPES[message.severity],
+          });
+        }
       }
 
       resolve(this.linterMessages);

tests/rules/javascript/test.deprecated_entities.js

@@ -14,6 +14,8 @@ describe('deprecated_entities', () => {
 
       return jsScanner.scan()
         .then((validationMessages) => {
+          validationMessages = validationMessages.sort();
+
           assert.equal(validationMessages.length, 1);
           assert.equal(validationMessages[0].code,
                        entity.error.code);

tests/rules/javascript/test.no_unsafe_innerhtml.js

@@ -0,0 +1,173 @@
+import { VALIDATION_WARNING } from 'const';
+import JavaScriptScanner from 'scanners/javascript';
+
+// These rules were mostly copied and adapted from
+// https://github.com/mozfreddyb/eslint-plugin-no-unsafe-innerhtml/
+// Please make sure to keep them up-to-date and report upstream errors.
+// Some notes are not included since we have our own rules
+// marking them as invalid (e.g document.write)
+
+
+describe('no_unsafe_innerhtml', () => {
+  var validCodes = [
+    // innerHTML equals
+    'a.innerHTML = \'\';',
+    'c.innerHTML = ``;',
+    'g.innerHTML = Sanitizer.escapeHTML``;',
+    'h.innerHTML = Sanitizer.escapeHTML`foo`;',
+    'i.innerHTML = Sanitizer.escapeHTML`foo${bar}baz`;',
+
+    // tests for innerHTML update (+= operator)
+    'a.innerHTML += \'\';',
+    'b.innerHTML += "";',
+    'c.innerHTML += ``;',
+    'g.innerHTML += Sanitizer.escapeHTML``;',
+    'h.innerHTML += Sanitizer.escapeHTML`foo`;',
+    'i.innerHTML += Sanitizer.escapeHTML`foo${bar}baz`;',
+    'i.innerHTML += Sanitizer.unwrapSafeHTML(htmlSnippet)',
+    'i.outerHTML += Sanitizer.unwrapSafeHTML(htmlSnippet)',
+
+    // testing unwrapSafeHTML spread
+    'this.imeList.innerHTML = Sanitizer.unwrapSafeHTML(...listHtml);',
+
+    // tests for insertAdjacentHTML calls
+    'n.insertAdjacentHTML("afterend", "meh");',
+    'n.insertAdjacentHTML("afterend", `<br>`);',
+    'n.insertAdjacentHTML("afterend", Sanitizer.escapeHTML`${title}`);',
+
+    // override for manual review and legacy code
+    'g.innerHTML = potentiallyUnsafe; // a=legacy, bug 1155131',
+
+    // (binary) expressions
+    'x.innerHTML = `foo`+`bar`;',
+    'y.innerHTML = "<span>" + 5 + "</span>";',
+
+    // document.write/writeln
+    'document.writeln(Sanitizer.escapeHTML`<em>${evil}</em>`);',
+
+    // template string expression tests
+    'u.innerHTML = `<span>${"lulz"}</span>`;',
+    'v.innerHTML = `<span>${"lulz"}</span>${55}`;',
+    'w.innerHTML = `<span>${"lulz"+"meh"}</span>`;',
+  ];
+
+
+  for (const code of validCodes) {
+    it(`should allow the use of innerHTML: ${code}`, () => {
+      var jsScanner = new JavaScriptScanner(code, 'badcode.js');
+
+      return jsScanner.scan()
+        .then((validationMessages) => {
+          assert.equal(validationMessages.length, 0);
+        });
+    });
+  }
+
+
+  var invalidCodes = [
+    // innerHTML examples
+    {
+      code: 'm.innerHTML = htmlString;',
+      message: ['Unsafe assignment to innerHTML'],
+    },
+    {
+      code: 'a.innerHTML += htmlString;',
+      message: ['Unsafe assignment to innerHTML'],
+    },
+    {
+      code: 'a.innerHTML += template.toHtml();',
+      message: ['Unsafe assignment to innerHTML'],
+    },
+    {
+      code: 'm.outerHTML = htmlString;',
+      message: ['Unsafe assignment to outerHTML'],
+    },
+    {
+      code: 't.innerHTML = `<span>${name}</span>`;',
+      message: ['Unsafe assignment to innerHTML'],
+    },
+    {
+      code: 't.innerHTML = `<span>${"foobar"}</span>${evil}`;',
+      message: ['Unsafe assignment to innerHTML'],
+    },
+
+    // insertAdjacentHTML examples
+    {
+      code: 'node.insertAdjacentHTML("beforebegin", htmlString);',
+      message: ['Unsafe call to insertAdjacentHTML'],
+    },
+    {
+      code: 'node.insertAdjacentHTML("beforebegin", template.getHTML());',
+      message: ['Unsafe call to insertAdjacentHTML'],
+    },
+
+    // (binary) expressions
+    {
+      code: 'node.innerHTML = "<span>"+ htmlInput;',
+      message: ['Unsafe assignment to innerHTML'],
+    },
+    {
+      code: 'node.innerHTML = "<span>" + htmlInput + "</span>";',
+      message: ['Unsafe assignment to innerHTML'],
+    },
+
+    // document.write / writeln
+    {
+      code: 'document.write("<span>" + htmlInput + "</span>");',
+      message: [
+        'Use of document.write strongly discouraged.',
+        'Unsafe call to document.write',
+      ],
+    },
+    {
+      code: 'document.writeln(evil);',
+      message: ['Unsafe call to document.writeln'],
+    },
+
+    // bug https://bugzilla.mozilla.org/show_bug.cgi?id=1198200
+    {
+      code: 'title.innerHTML = _("WB_LT_TIPS_S_SEARCH", {value0:engine});',
+      message: ['Unsafe assignment to innerHTML'],
+    },
+
+    // https://bugzilla.mozilla.org/show_bug.cgi?id=1192595
+    {
+      code: 'x.innerHTML = Sanitizer.escapeHTML(evil)',
+      message: ['Unsafe assignment to innerHTML'],
+    },
+    {
+      code: 'x.innerHTML = Sanitizer.escapeHTML(`evil`)',
+      message: ['Unsafe assignment to innerHTML'],
+    },
+    {
+      code: 'y.innerHTML = ((arrow_function)=>null)`some HTML`',
+      message: ['Unsafe assignment to innerHTML'],
+    },
+    {
+      code: 'y.innerHTML = ((arrow_function)=>null)`some HTML`',
+      message: ['Unsafe assignment to innerHTML'],
+    },
+    {
+      code: 'y.innerHTML = ((arrow_function)=>null)`some HTML`',
+      message: ['Unsafe assignment to innerHTML'],
+    },
+  ];
+
+  for (const code of invalidCodes) {
+    it(`should not allow the use of innerHTML examples ${code.code}`, () => {
+      var jsScanner = new JavaScriptScanner(code.code, 'badcode.js');
+
+      return jsScanner.scan()
+        .then((validationMessages) => {
+          validationMessages = validationMessages.sort();
+
+          assert.equal(validationMessages.length, code.message.length);
+
+          code.message.forEach((expectedMessage, idx) => {
+            assert.equal(validationMessages[idx].message, expectedMessage);
+            assert.equal(validationMessages[idx].type, VALIDATION_WARNING);
+          });
+        });
+    });
+  }
+});

tests/scanners/test.javascript.js

@@ -128,18 +128,27 @@ describe('JavaScript Scanner', function() {
   });
 
   it('should reject on missing message code', () => {
+    var FakeCLIEngine = function() {};
+    FakeCLIEngine.prototype = {
+      constructor: function() {},
+      executeOnText: () => {
+        return {
+          results: [{
+            messages: [{
+              fatal: false,
+            }],
+          }],
+        };
+      },
+    };
 
     var FakeESLint = {
       linter: {
         defineRule: () => {
           // no-op
         },
-        verify: () => {
-          return [{
-            fatal: false,
-          }];
-        },
       },
+      CLIEngine: FakeCLIEngine,
     };
 
     var jsScanner = new JavaScriptScanner('whatever', 'badcode.js');
@@ -281,14 +290,21 @@ describe('JavaScript Scanner', function() {
   it('should export all rules in rules/javascript', () => {
     // We skip the "run" check here for now as that's handled by ESLint.
     var ruleFiles = getRuleFiles('javascript');
-    assert.equal(ruleFiles.length, Object.keys(ESLINT_RULE_MAPPING).length);
+    var externalRules = 1;
+
+    assert.equal(
+      ruleFiles.length + externalRules,
+      Object.keys(ESLINT_RULE_MAPPING).length
+    );
 
     var jsScanner = new JavaScriptScanner('', 'badcode.js');
 
     return jsScanner.scan()
       .then(() => {
-        assert.equal(jsScanner._rulesProcessed,
-                     Object.keys(rules).length);
+        assert.equal(
+          jsScanner._rulesProcessed,
+          Object.keys(rules).length
+        );
       });
   });