mozilla / eslint-plugin-no-unsanitized

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

JSX Support (Was: Cannot read property 'innerHTML' of undefined) #86

Open Yserz opened 6 years ago

Yserz commented 6 years ago

Hey,

just wanted to let you know that we observed the following error while using this plugin in version 3.0.2. I'm not sure how to get more information about this issue.

Error while loading rule 'no-unsanitized/property': Cannot read property 'innerHTML' of undefined
TypeError: Error while loading rule 'no-unsanitized/property': Cannot read property 'innerHTML' of undefined
at Object.keys.forEach (/Users/.../node_modules/eslint-plugin-no-unsanitized/lib/ruleHelper.js:207:34)
at Array.forEach (<anonymous>)
at RuleHelper.combineRuleChecks (/Users/.../node_modules/eslint-plugin-no-unsanitized/lib/ruleHelper.js:204:38)
at new RuleHelper (/Users/.../node_modules/eslint-plugin-no-unsanitized/lib/ruleHelper.js:22:28)
at Object.create (/Users/.../node_modules/eslint-plugin-no-unsanitized/lib/rules/property.js:42:28)
at createRuleListeners (/Users/.../node_modules/eslint/lib/linter.js:682:21)
at Object.keys.forEach.ruleId (/Users/.../node_modules/eslint/lib/linter.js:841:31)
at Array.forEach (<anonymous>)
at runRules (/Users/.../node_modules/eslint/lib/linter.js:781:34)
at Linter._verifyWithoutProcessors (/Users/.../node_modules/eslint/lib/linter.js:1003:33)
Standard8 commented 6 years ago

@Yserz Do you know which version introduced this? Was it 3.0.2 or earlier? I'm guessing it is 3.0.2 from the looks of the error.

I think it is potentially an issue with the configuration, please can you post the no-unsanitized parts of your configuration(s)?

audishos commented 6 years ago

I'm seeing the same error on 3.0.2. No issues on 3.0.1. I'm using the following configuration type in my eslintrc file: { "plugins": ["no-unsanitized"], "extends": ["plugin:no-unsanitized/DOM"] }

Yserz commented 6 years ago

We have the same configuration as @audishos. Can't really say with which version it appeared, sorry.

Standard8 commented 6 years ago

I've just been trying to reproduce this with a simple test directory, with just an .eslintrc file, and a couple of js files, plus another in a sub-directory.

Unfortunately I can't reproduce it.

Can someone try and track this down, or see what might be triggering it?

mozfreddyb commented 5 years ago

(I've been out of office for a while, please excuse the delay).

I suspect this is strongly related to the code you're scanning. @Yserz, @audishos: Would you be able to find a code snippet that is minimal and reproduces?

Yserz commented 5 years ago

@Standard8 Unfortunately I can't reproduce this anymore. But I would like to add that we are using react and react-intl where react-intl is making use of innerHTML. (see https://github.com/yahoo/react-intl/blob/f82383e02cb0d38dfd28649dc2442a2ea2796a44/src/components/html-message.js#L86)

mozfreddyb commented 5 years ago

I'm really out of touch with the react ecosystem, so it seems like I can't help you reproduce your error. It would be nice if you could spend some time testing this again - possibly checking if you need an older version of react or react-intl to reproduce.

If you can't or won't give us better steps to reproduce, I'm afraid I'll have to close this issue.

audishos commented 5 years ago

The error message and debug info don't appear to give much information. I'm using VS Code with the eslint plugin and the error pops up on every file with a react component that uses dangerouslySetInnerHTML.

mozfreddyb commented 5 years ago

As I said before, I'm really, really out of touch with the react ecosystem and everything that is JSX.

Could you come up with a minimal example and instructions on how to reproduce? I'd still be happy to take a look.

audishos commented 5 years ago

I was able to trigger the error with the following file.

/* Foo.js */
import React, { Component } from 'react';

class Foo extends Component {
    render() {
        return <div dangerouslySetInnerHTML={<span>{'Bar'}</span>} />;
    }
}

export default Foo;
mozfreddyb commented 5 years ago

Thanks @audishos. I think we're getting closer. I've added your example to the test suite, but I don't see any failures. After some additional inspection of the error stack, that's doesn't surprise me. I think I've been hunting the wrong end all along and there is a problem related to the eslintrc. Would you also be able to share how you've configured this plugin in your project?

audishos commented 5 years ago

Sure thing. Here's the full file.

module.exports = {
    "parser": "babel-eslint",
    "plugins": [
        "react",
        "jsx-a11y",
        "no-unsanitized",
        "security",
    ],
    "extends": [
        "eslint:recommended",
        "plugin:react/recommended",
        "plugin:jsx-a11y/recommended",
        "plugin:no-unsanitized/DOM",
        "plugin:security/recommended",
    ],
    "rules": {
        "comma-dangle": ["off"],
        "block-scoped-var": ["error"],
        "complexity": ["warn", 3],
        "default-case": ["error"],
        "strict": ["off"],
        "eol-last": ["off"],
        "eqeqeq": ["error", "smart"],
        "guard-for-in": ["error"],
        "key-spacing": ["off"],
        "max-depth": ["warn", 2],
        "max-len": ["error", { "code": 130, "tabWidth": 4 }],
        "max-params": ["error", 4],
        "no-param-reassign": ["error"],
        "no-bitwise": ["error"],
        "no-div-regex": ["error"],
        "no-else-return": ["error"],
        "no-implied-eval": ["error"],
        "no-mixed-spaces-and-tabs": ["warn"],
        "no-eval": ["error"],
        "no-debugger": ["error"],
        "no-unused-expressions": ["error"],
        "no-trailing-spaces": ["off"],
        "no-underscore-dangle": ["off"],
        "no-console": ["error", { "allow": ["warn", "error"] }],
        "no-extra-semi": ["warn"],
        "no-unused-vars": ["error", { "vars": "all", "args": "none", "ignoreRestSiblings": true }],
        "no-use-before-define": ["error", "nofunc"],
        "quote-props": ["off"],
        "semi": ["error", "always"],
        "valid-typeof": ["error"],
        "react/jsx-uses-vars": "error",
        "react/display-name": "off",
        "react/jsx-boolean-value": ["error", "always"],
        "react/jsx-no-undef": "warn",
        "react/jsx-uses-react": "warn",
        "react/jsx-no-target-blank": "warn",
        "react/no-did-mount-set-state": "error",
        "react/no-did-update-set-state": "error",
        "react/no-multi-comp": "off",
        "react/no-unknown-property": "error",
        "react/prop-types": "off",
        "react/react-in-jsx-scope": "error",
        "react/self-closing-comp": "error",
        "react/wrap-multilines": "off",
        "react/no-deprecated": "warn",
        "react/no-danger": "error",
        "react/no-danger-with-children": "error",
        "react/self-closing-comp": "off",
        "jsx-a11y/label-has-for": ["error", { "allowChildren": true }],
        "jsx-a11y/no-noninteractive-element-interactions": "warn",
        "jsx-a11y/click-events-have-key-events": "warn",
        "jsx-a11y/interactive-supports-focus": "warn",
        "security/detect-unsafe-regex": "error",
        "security/detect-non-literal-regexp": "error",
        // use of [CONST] or [locale] in many files for accessing object values
        "security/detect-object-injection": "off",
        "prefer-const": ["error", {
            "destructuring": "any",
            "ignoreReadBeforeAssign": false
        }],
        "no-useless-constructor": "warn",
        "no-var": "warn",
    },
    "overrides": [
        {
            "files": ["*.test.js", "*.tests.js"],
            "rules": {
                "security/detect-unsafe-regex": "off",
                "security/detect-non-literal-regexp": "off",
                "max-len": "off",
            },
        },
    ],
    "env": {
        "es6": true,
        "browser": true,
        "node": true,
        "jest": true,
    },
    "globals": {
        "appData": false,
        "require": false,
        "DEBUG": false,
        "before": false,
        "after": false,
    },
    "parserOptions": {
        "ecmaVersion": 6,
        "ecmaFeatures": {
            "jsx": true,
        },
        "sourceType": "module",
    },
    "settings": {
        "react": {
            "version": "16.2",
        },
    },
};
mozfreddyb commented 5 years ago

Sorry, I couldn't follow up sooner. Here's what I noticed:

    "extends": [
        "eslint:recommended",
        "plugin:react/recommended",
        "plugin:jsx-a11y/recommended",
        "plugin:no-unsanitized/DOM", // <-- here
        "plugin:security/recommended",
    ],

What is no-unsanitized/DOM? We only provide no-unsanitized/method and no-unsanitized/property

Err, I'm wrong. it's been a while since I looked at this code :sweat: