Merge pull request #260 from xiemaisi/js/confusing-precedence

Approved by esben-semmle, mc-semmle

Author: semmle-qlci

change-notes/1.19/analysis-javascript.md

@@ -15,9 +15,10 @@
 | **Query**                                     | **Tags**                                             | **Purpose**                                                                                                                                                                 |
 |-----------------------------------------------|------------------------------------------------------|-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------|
 | Enabling Node.js integration for Electron web content renderers (`js/enabling-electron-renderer-node-integration`) | security, frameworks/electron, external/cwe/cwe-094  | Highlights Electron web content renderer preferences with Node.js integration enabled, indicating a violation of [CWE-94](https://cwe.mitre.org/data/definitions/94.html). Results are not shown on LGTM by default. |
-| Stored cross-site scripting (`js/stored-xss`) | security, external/cwe/cwe-079, external/cwe/cwe-116 | Highlights uncontrolled stored values flowing into HTML content, indicating a violation of [CWE-079](https://cwe.mitre.org/data/definitions/79.html). Results shown on LGTM by default. |
-| Replacement of a substring with itself (`js/identity-replacement`) | correctness, security, external/cwe/cwe-116 | Highlights string replacements that replace a string with itself, which usually indicates a mistake. Results shown on LGTM by default. |
 | Host header poisoning in email generation |  security, external/cwe/cwe-640 | Highlights code that generates emails with links that can be hijacked by HTTP host header poisoning, indicating a violation of [CWE-640](https://cwe.mitre.org/data/definitions/640.html). Results shown on LGTM by default.  |
+| Replacement of a substring with itself (`js/identity-replacement`) | correctness, security, external/cwe/cwe-116 | Highlights string replacements that replace a string with itself, which usually indicates a mistake. Results shown on LGTM by default. |
+| Stored cross-site scripting (`js/stored-xss`) | security, external/cwe/cwe-079, external/cwe/cwe-116 | Highlights uncontrolled stored values flowing into HTML content, indicating a violation of [CWE-079](https://cwe.mitre.org/data/definitions/79.html). Results shown on LGTM by default. |
+| Unclear precedence of nested operators (`js/unclear-operator-precedence`) | maintainability, correctness, external/cwe/cwe-783 | Highlights nested binary operators whose relative precedence is easy to misunderstand. Results shown on LGTM by default. |
 
 ## Changes to existing queries
 
@@ -29,5 +30,6 @@
 | Remote property injection | Fewer results | The precision of this rule has been revised to "medium". Results are no longer shown on LGTM by default. |
 | Missing CSRF middleware | Fewer false-positive results | This rule now recognizes additional CSRF protection middlewares. |
 | Server-side URL redirect | More results | This rule now recognizes redirection calls in more cases. |
+| Whitespace contradicts operator precedence | Fewer false-positive results | This rule no longer flags operators with asymmetric whitespace. |
 
 ## Changes to QL libraries

javascript/config/suites/javascript/correctness-more

@@ -5,6 +5,7 @@
 + semmlecode-javascript-queries/Expressions/SuspiciousInvocation.ql: /Correctness/Expressions
 + semmlecode-javascript-queries/Expressions/SuspiciousPropAccess.ql: /Correctness/Expressions
 + semmlecode-javascript-queries/Expressions/UnboundEventHandlerReceiver.ql: /Correctness/Expressions
++ semmlecode-javascript-queries/Expressions/UnclearOperatorPrecedence.ql: /Correctness/Expressions
 + semmlecode-javascript-queries/Expressions/UnknownDirective.ql: /Correctness/Expressions
 + semmlecode-javascript-queries/Expressions/WhitespaceContradictsPrecedence.ql: /Correctness/Expressions
 + semmlecode-javascript-queries/LanguageFeatures/IllegalInvocation.ql: /Correctness/Language Features

javascript/ql/src/Expressions/UnclearOperatorPrecedence.qhelp

@@ -0,0 +1,46 @@
+<!DOCTYPE qhelp PUBLIC "-//Semmle//qhelp//EN" "qhelp.dtd">
+<qhelp>
+
+<overview>
+<p>
+Nested expressions that rely on less well-known operator precedence rules can be
+hard to read and understand. They could even indicate a bug where the author of the
+code misunderstood the precedence rules.
+</p>
+</overview>
+
+<recommendation>
+<p>
+Use parentheses or additional whitespace to clarify grouping.
+</p>
+</recommendation>
+
+<example>
+<p>
+Consider the following snippet of code:
+</p>
+
+<sample src="examples/UnclearOperatorPrecedence.js" />
+
+<p>
+It might look like this tests whether <code>x</code> and <code>y</code> have any bits in
+common, but in fact <code>==</code> binds more tightly than <code>&amp;</code>, so the test
+is equivalent to <code>x &amp; (y == 0)</code>.
+</p>
+
+<p>
+If this is the intended interpretation, parentheses should be used to clarify this. You could
+also consider adding extra whitespace around <code>&amp;</code> or removing whitespace
+around <code>==</code> to make it visually apparent that it binds less tightly:
+<code>x &amp; y==0</code>.
+</p>
+<p>
+Probably the best approach in this case, though, would be to use the <code>&amp;&amp;</code>
+operator instead to clarify the intended interpretation: <code>x &amp;&amp; y == 0</code>.
+</p>
+</example>
+
+<references>
+<li>Mozilla Developer Network, <a href="https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Operator_Precedence">Operator precedence</a>.</li>
+</references>
+</qhelp>

javascript/ql/src/Expressions/UnclearOperatorPrecedence.ql

@@ -0,0 +1,29 @@
+/**
+ * @name Unclear precedence of nested operators
+ * @description Nested expressions involving binary bitwise operators and comparisons are easy
+ *              to misunderstand without additional disambiguating parentheses or whitespace.
+ * @kind problem
+ * @problem.severity recommendation
+ * @id js/unclear-operator-precedence
+ * @tags maintainability
+ *       correctness
+ *       statistical
+ *       non-attributable
+ *       external/cwe/cwe-783
+ * @precision very-high
+ */
+
+import javascript
+
+from BitwiseBinaryExpr bit, Comparison rel, Expr other
+where bit.hasOperands(rel, other) and
+      // only flag if whitespace doesn't clarify the nesting (note that if `bit` has less
+      // whitespace than `rel`, it will be reported by `js/whitespace-contradicts-precedence`)
+      bit.getWhitespaceAroundOperator() = rel.getWhitespaceAroundOperator() and
+      // don't flag if the other operand is itself a comparison,
+      // since the nesting tends to be visually more obvious in such cases
+      not other instanceof Comparison and
+      // don't flag occurrences in minified code
+      not rel.getTopLevel().isMinified()
+select rel, "The '" + rel.getOperator() + "' operator binds more tightly than " +
+            "'" + bit.getOperator() + "', which may not be obvious in this case."

javascript/ql/src/Expressions/WhitespaceContradictsPrecedence.ql

@@ -54,29 +54,6 @@ class HarmlessNestedExpr extends BinaryExpr {
   }
 }
 
-/** Holds if the right operand of `expr` starts on line `line`, at column `col`. */
-predicate startOfBinaryRhs(BinaryExpr expr, int line, int col) {
-  exists(Location rloc | rloc = expr.getRightOperand().getLocation() |
-    rloc.getStartLine() = line and rloc.getStartColumn() = col
-  )
-}
-
-/** Holds if the left operand of `expr` ends on line `line`, at column `col`. */
-predicate endOfBinaryLhs(BinaryExpr expr, int line, int col) {
-  exists(Location lloc | lloc = expr.getLeftOperand().getLocation() |
-    lloc.getEndLine() = line and lloc.getEndColumn() = col
-  )
-}
-
-/** Gets the number of whitespace characters around the operator of `expr`. */
-int operatorWS(BinaryExpr expr) {
-  exists(int line, int lcol, int rcol |
-    endOfBinaryLhs(expr, line, lcol) and
-    startOfBinaryRhs(expr, line, rcol) and
-    result = rcol - lcol + 1 - expr.getOperator().length()
-  )
-}
-
 /**
  * Holds if `inner` is an operand of `outer`, and the relative precedence
  * may not be immediately clear, but is important for the semantics of
@@ -88,10 +65,8 @@ predicate interestingNesting(BinaryExpr inner, BinaryExpr outer) {
   not inner instanceof HarmlessNestedExpr
 }
 
-from BinaryExpr inner, BinaryExpr outer, int wsouter, int wsinner
+from BinaryExpr inner, BinaryExpr outer
 where interestingNesting(inner, outer) and
-      wsinner = operatorWS(inner) and wsouter = operatorWS(outer) and
-      wsinner % 2 = 0 and wsouter % 2 = 0 and
-      wsinner > wsouter and
+      inner.getWhitespaceAroundOperator() > outer.getWhitespaceAroundOperator() and
       not outer.getTopLevel().isMinified()
 select outer, "Whitespace around nested operators contradicts precedence."

javascript/ql/src/Expressions/examples/UnclearOperatorPrecedence.js

@@ -0,0 +1,3 @@
+if (x & y == 0) {
+  // ...
+}

javascript/ql/src/semmle/javascript/Expr.qll

@@ -1008,6 +1008,25 @@ class BinaryExpr extends @binaryexpr, Expr {
   override ControlFlowNode getFirstControlFlowNode() {
     result = getLeftOperand().getFirstControlFlowNode()
   }
+
+  /**
+   * Gets the number of whitespace characters around the operator of this expression.
+   *
+   * This predicate is only defined if both operands are on the same line, and if the
+   * amount of whitespace before and after the operator are the same.
+   */
+  int getWhitespaceAroundOperator() {
+    exists (Token lastLeft, Token operator, Token firstRight, int l, int c1, int c2, int c3, int c4 |
+      lastLeft = getLeftOperand().getLastToken() and
+      operator = lastLeft.getNextToken() and
+      firstRight = operator.getNextToken() and
+      lastLeft.getLocation().hasLocationInfo(_, _, _, l, c1) and
+      operator.getLocation().hasLocationInfo(_, l, c2, l, c3) and
+      firstRight.getLocation().hasLocationInfo(_, l, c4, _, _) and
+      result = c2-c1-1 and
+      result = c4-c3-1
+    )
+  }
 }
 
 /**

javascript/ql/test/query-tests/Expressions/UnclearOperatorPrecedence/UnclearOperatorPrecedence.expected

@@ -0,0 +1,3 @@
+| tst.js:1:9:1:17 | 0x0A != 0 | The '!=' operator binds more tightly than '&', which may not be obvious in this case. |
+| tst.js:6:1:6:7 | x !== y | The '!==' operator binds more tightly than '&', which may not be obvious in this case. |
+| tst.js:10:3:10:6 | b==c | The '==' operator binds more tightly than '&', which may not be obvious in this case. |

javascript/ql/test/query-tests/Expressions/UnclearOperatorPrecedence/UnclearOperatorPrecedence.qlref

@@ -0,0 +1 @@
+Expressions/UnclearOperatorPrecedence.ql

javascript/ql/test/query-tests/Expressions/UnclearOperatorPrecedence/tst.js

@@ -0,0 +1,10 @@
+x.f() & 0x0A != 0;   // NOT OK
+x.f() & (0x0A != 0); // OK
+x.f()  &  0x0A != 0; // OK
+x.f() & 0x0A!=0;     // OK
+
+x !== y & 1;         // NOT OK
+
+x > 0 & x < 10;      // OK
+
+a&b==c;          // NOT OK