eslint-community / eslint-plugin-security

ESLint rules for Node Security
Apache License 2.0
2.22k stars 109 forks source link

`security/detect-object-injection` false alarm on type safe key #126

Closed loynoir closed 1 year ago

loynoir commented 1 year ago

What version of eslint-plugin-security are you using?

1.7.1

ESLint Environment

Node version: npm version: Local ESLint version: Global ESLint version: Operating System:

What parser are you using?

@typescript-eslint/parser

What did you do?

Configuration ``` "extends": [ "standard-with-typescript", ... "prettier", "plugin:security/recommended" ], ```

What did you expect to happen?

Bypass security/detect-object-injection when type safe.

const kfoo = Symbol('foo')
const kbar = 'bar'
const o: Partial<Record<typeof kfoo | typeof kbar, 42>> = {}

o[kfoo] = 42
o[kbar] = 42

What actually happened?

const kfoo = Symbol('foo')
const kbar = 'bar'
const o: Partial<Record<typeof kfoo | typeof kbar, 42>> = {}

// eslint-disable-next-line security/detect-object-injection
o[kfoo] = 42
// eslint-disable-next-line security/detect-object-injection
o[kbar] = 42

Participation

Additional comments

Related https://github.com/typescript-eslint/typescript-eslint/issues/7194

nzakas commented 1 year ago

Bypass security/detect-object-injection when type safe.

What does this mean?

loynoir commented 1 year ago

@nzakas

Given

const kfoo = Symbol('foo')
const kbar = 'bar'
const o: Partial<Record<typeof kfoo | typeof kbar, 42>> = {}

I think below code are safe, right?

// eslint-disable-next-line security/detect-object-injection
o[kfoo] = 42
// eslint-disable-next-line security/detect-object-injection
o[kbar] = 42
nzakas commented 1 year ago

Again, I'm not quite sure what you're asking.

ESLint doesn't interpret code, it just looks for simple patterns. If you're implying that something in the first code block means the code in the second block shouldn't warn, that's not something this rule can do. It's not going to interpret TypeScript (which is expensive) to figure out if a later pattern would work.

loynoir commented 1 year ago

@nzakas

Quote reply from https://github.com/typescript-eslint/typescript-eslint/issues/7194

Unlike ESLint core - 3rd party plugins aren't restricted to just supporting JS and can support TS and consume types.

For this reason we do not create extension rules for 3rd party rules - we are not looking to compete with other plugins in the community.

It would be nice to let security/detect-object-injection have better typescript support within eslint-community/eslint-plugin-security.

It's not going to interpret TypeScript (which is expensive) to figure out if a later pattern would work.

Compare to human time, machine time is not so expensive.

Given

Would be nice to have option

nzakas commented 1 year ago

As I mentioned, that type of check is expensive and not worth implementing. However, because this is an open source project, you can always copy the rule and make your own that does exactly what you want.