Expand scope of detect-possible-timing-attack

[g-marconet]

In my usage, I found detect-possible-timing-attack to be essentially toothless. Rarely did I find issues as simple as password === 'mypass'. More often, it would be something like, password.toString() === expectedPassword, which the old version does not recognize.

Changes

• Runs on BinaryExpressions instead of IfStatements. This way it catches something like if (checkPassword(a, b)) { ... } where checkPassword = (password, expected) => password === expected;
• Checks identifiers, call expressions, element access experssions, and property access expressions for keywords, rather than just identifiers. Now can detect user.password === 'password', user['password'] === 'password', and password.toString() === 'password', for example
• Will only check for keywords if it makes sense. For example, password === true does not cause an issue anymore
• Keyword regex matches if a keyword is anywhere within the target, instead of if it exactly equals the target. For example, expectedPassword === 'password' will now be flagged
• Removed auth as keyword. In practice, it caused many false-positives and any actual positive was already captured by another keyword.

This is to tslint-config-security what eslint-community/eslint-plugin-security#37 is to eslint-plugin-security

[webschik]

Hi @g-marconet !
Thanks for your help! I have some comments about the code style and removing of the auth keyword

[webschik]

why do you think that auth keyword shouldn't be a part of this validation?

[g-marconet]

Admittedly it's pretty anecdotal, but in our codebase, any time the auth keyword was found was either a false positive or already covered by another keyword (mostly token). That said, not super married to that and it can definitely be left in if you think that's best!

[webschik]

I think it's better to have more readable function names: isVuln* => isVulnerable*

[webschik]

const {kind: operatorTokenKind} = node.operatorToken;

if (
    operatorTokenKind === ts.SyntaxKind.EqualsEqualsToken ||
    operatorTokenKind === ts.SyntaxKind.EqualsEqualsEqualsToken ||
...

[webschik]

Great! But below all node.operatorToken.kind expressions should be replaced with operatorTokenKind ;)

[g-marconet]

This is embarrassing... forgot to hit ctrl+s :D

[webschik]

By accident, I've merged your change on this line, but I think we should add auth keyword back into this list

[g-marconet]

I'm fine with that. Added back