mozilla / eslint-plugin-no-unsanitized

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

Unsupported Callee of type TSNonNullExpression for CallExpression #144

Closed Siegrift closed 3 years ago

Siegrift commented 3 years ago

Hi,

I ran your plugin on https://github.com/angular/components and there was an error in no-unsanitized/method rule.

Steps to reproduce the issue:

  1. Add eslint to that project. My .eslintrc.json looks like this

    {
    "env": {
    "browser": true,
    "es2020": true
    },
    "extends": [
    // "plugin:@typescript-eslint/recommended",
    "plugin:no-unsanitized/DOM"
    // "eslint:recommended",
    // "plugin:security/all"
    ],
    "parser": "@typescript-eslint/parser",
    "parserOptions": {
    "ecmaVersion": 11,
    "sourceType": "module"
    },
    "plugins": ["@typescript-eslint", "security"],
    "rules": {}
    }
  2. yarn eslint "src/**/*.ts"

Siegrift commented 3 years ago

Also happens when tried in https://github.com/chakra-ui/chakra-ui. The error happens when linting chakra-ui/packages/popper/tests/popper.utils.test.ts

mozfreddyb commented 3 years ago

@Siegrift Can you share the source line in which this error occured?

mozfreddyb commented 3 years ago

CCing @LukeWood :-)

Siegrift commented 3 years ago

This is the output when running the plugin in angular-components.

output

When runing in chakra-ui, the error is on line 4.

mozfreddyb commented 3 years ago

I was asking for the offending lines in your source code. Anyway, I have found the offending lines from youtube-player.spec.ts in the end, though.

@LukeWood do you think you can make a patch with a test case? We basically need to run the same code we do for a function call as if there was no nonnull expression involved. I think we want tests to ensure the following:

LukeWood commented 3 years ago

Thanks for CCing me - yes let me do that!

mozfreddyb commented 3 years ago

Ping @LukeWood :)

LukeWood commented 3 years ago

Hey - been busy with work the past 2 weeks. Hoping to dedicate some time to this mid next week?

LukeWood commented 3 years ago

adding

        {
            code: "document.write!(evil)",
            parser: PATH_TO_TYPESCRIPT_ESLINT,
            parserOptions: {
                ecmaVersion: 2018,
                sourceType: "module",
            },
            errors: [
                {
                    message: "Unsafe call to document.write! for argument 1",
                    type: "CallExpression"
                }
            ]
        },

to method.js

is reporting "Unsafe call to document.write for argument 0" at the current HEAD commit - so while not exactly right it shouldn't be erroring the way described above. Maybe the version the user is running w/ is behind?

mozfreddyb commented 3 years ago

@LukeWood OK, let's fix the test mismatch. I think we use zero-indexed argument reporting. @Siegrift can you confirm that this error doesnt show up when you use the eslint plugin directly from github?

Siegrift commented 3 years ago

Yes, I confirmed on angular/components and got 0 crashes.

LukeWood commented 3 years ago

ah if we use zero indexed argument reporting then we are fine.

LukeWood commented 3 years ago

we can still add these as test cases but I don't think they cover any new ground.

LukeWood commented 3 years ago

lets close - I don't think this really covers new ground!