[Rule] wrong logical operator inside an if condition

[ElectricNroff]

Rule Description

I looked at Semgrep for the first time today. The standard configs didn't find a bug, but I was able to find a bug after writing my own rule by analogy to https://semgrep.dev/editor?registry=javascript.lang.correctness.useless-eqeq.eqeq-is-bad

I'm entering this issue because https://semgrep.dev/docs/contributing/contributing-code/ says "Creating an issue first is preferable to moving directly to a pull request so that we can ensure you're on the right track without any wasted effort. This is also a great way to contribute to Semgrep even if you're not making changes yourself." Here is the rule I wrote:

rules:
  - id: wrong-logical-op-inside-if
    languages:
      - javascript
      - typescript
    message: Detected a useless if condition `if ($X !== $Y || $X !== $Z)`
      or `if ($X === $Y && $X === $Z)`. These conditions are usually
      caused by accidental use of the wrong logical operator (or, they
      have a duplicative test if $Y and $Z are equal). Essentially,
      "not equal to" either of two distinct values is always true,
      whereas "equal to" both of two distinct values is always false.
    metadata:
      category: correctness
      license: Commons Clause License Condition v1.0[LGPL-2.1-only]
      technology:
        - javascript
    patterns:
      - pattern-either:
          - pattern: if ($X !== $Y || $X !== $Z)
          - pattern: if ($X === $Y && $X === $Z)
    severity: ERROR

What does this rule intend to find?

a representative case is a developer who writes "if (httpMethod !== 'POST' || httpMethod !== 'PUT') allowed = true" when they intended to write "if (httpMethod !== 'POST' && httpMethod !== 'PUT') allowed = true"

Examples or references

it's a simple flaw in logic, not something that depends on a language specifcation or a specific threat model

Put an example or references here

probably the most common example would be a developer who starts from code that uses "equals" with an "OR" operator, and decides to modify the code to use "not equals" instead, but forgets to change "OR" to "AND"

Additional information

The rule could be improved if there were a way to express the constraint that $Y and $Z are unequal. If they are unequal, then the outcome should be "error" but if they are equal then the output could be just a warning/suggestion.

More information that would help someone write this rule!

it could additionally look for less strict equality tests, like == and != rather than === and !==

PR Checklist

• [ ] This ticket has links, references, or examples.
• [ ] The rule has true positive and true negative test cases in a file that matches the rule name.

If the rule is my-rule, the test file name should be my-rule.js.

True positives are marked by comments with ruleid: <my-rule> and true negatives are marked by comments with ok: <my-rule>.

• [ ] The rule has a good message. A good message includes:

1. A description of the pattern (e.g., missing parameter, dangerous flag, out-of-order function calls).
2. A description of why this pattern was detected (e.g., logic bug, introduces a security vulnerability, bad practice).
3. An alternative that resolves the issue (e.g., use another function, validate data first, discard the dangerous flag).

• [ ] After the PR has been reviewed and approved, merge the rule and close this ticket! Thanks for contributing!

[DrewDennison]

Thank you for writing a new semgrep rule!

[minusworld]

Hey @ElectricNroff! Thanks! We'd be happy to take this as a pull request :D

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[stale[bot]]

Stale-bot has closed this stale item. Please reopen it if this is in error.