Merge pull request #15602 from asgerf/js/block-logical-and-flow

JS: Fix flow through &&

Author: asgerf

javascript/ql/lib/semmle/javascript/dataflow/DataFlow.qll

@@ -1693,7 +1693,11 @@ module DataFlow {
     exists(Expr predExpr, Expr succExpr |
       pred = valueNode(predExpr) and succ = valueNode(succExpr)
     |
-      predExpr = succExpr.(LogicalBinaryExpr).getAnOperand()
+      predExpr = succExpr.(LogicalOrExpr).getAnOperand()
+      or
+      predExpr = succExpr.(NullishCoalescingExpr).getAnOperand()
+      or
+      predExpr = succExpr.(LogicalAndExpr).getRightOperand()
       or
       predExpr = succExpr.(ConditionalExpr).getABranch()
       or

javascript/ql/lib/semmle/javascript/dataflow/internal/BasicExprTypeInference.qll

@@ -238,6 +238,26 @@ private class AnalyzedBinaryExpr extends DataFlow::AnalyzedValueNode {
   }
 }
 
+pragma[nomagic]
+private predicate falsyValue(AbstractValue value) { value.getBooleanValue() = false }
+
+/**
+ * Flow analysis for `&&` operators.
+ */
+private class AnalyzedLogicalAndExpr extends DataFlow::AnalyzedValueNode {
+  override LogicalAndExpr astNode;
+
+  pragma[nomagic]
+  private AnalyzedValueNode leftOperand() { result = astNode.getLeftOperand().analyze() }
+
+  override AbstractValue getALocalValue() {
+    result = super.getALocalValue()
+    or
+    result = this.leftOperand().getALocalValue() and
+    falsyValue(result)
+  }
+}
+
 /**
  * Gets the `n`th operand of the given `+` or `+=` expression.
  */

javascript/ql/src/change-notes/2024-02-13-block-logical-and-flow.md

@@ -0,0 +1,4 @@
+---
+category: fix
+---
+* The left operand of the `&&` operator no longer propagates data flow by default.

javascript/ql/test/library-tests/DataFlow/tests.expected

@@ -1022,7 +1022,6 @@ flowStep
 | tst.js:4:9:4:12 | "hi" | tst.js:4:5:4:12 | y |
 | tst.js:9:2:9:2 | x | tst.js:9:1:9:3 | (x) |
 | tst.js:10:4:10:4 | y | tst.js:10:1:10:4 | x, y |
-| tst.js:11:1:11:1 | x | tst.js:11:1:11:6 | x && y |
 | tst.js:11:1:11:1 | x | tst.js:12:1:12:1 | x |
 | tst.js:11:1:11:1 | x | tst.js:12:1:12:1 | x |
 | tst.js:11:6:11:6 | y | tst.js:11:1:11:6 | x && y |

javascript/ql/test/library-tests/TaintTracking/BasicTaintTracking.expected

@@ -154,6 +154,7 @@ typeInferenceMismatch
 | json-stringify.js:2:16:2:23 | source() | json-stringify.js:42:8:42:51 | JSON.st ... urce))) |
 | json-stringify.js:2:16:2:23 | source() | json-stringify.js:45:8:45:23 | fastJson(source) |
 | json-stringify.js:3:15:3:22 | source() | json-stringify.js:8:8:8:31 | jsonStr ... (taint) |
+| logical-and.js:2:17:2:24 | source() | logical-and.js:4:10:4:24 | "safe" && taint |
 | nested-props.js:4:13:4:20 | source() | nested-props.js:5:10:5:14 | obj.x |
 | nested-props.js:9:18:9:25 | source() | nested-props.js:10:10:10:16 | obj.x.y |
 | nested-props.js:35:13:35:20 | source() | nested-props.js:36:10:36:20 | doLoad(obj) |

javascript/ql/test/library-tests/TaintTracking/DataFlowTracking.expected

@@ -73,6 +73,7 @@
 | importedReactComponent.jsx:4:40:4:47 | source() | exportedReactComponent.jsx:2:10:2:19 | props.text |
 | indexOf.js:4:11:4:18 | source() | indexOf.js:9:10:9:10 | x |
 | indexOf.js:4:11:4:18 | source() | indexOf.js:13:10:13:10 | x |
+| logical-and.js:2:17:2:24 | source() | logical-and.js:4:10:4:24 | "safe" && taint |
 | nested-props.js:4:13:4:20 | source() | nested-props.js:5:10:5:14 | obj.x |
 | nested-props.js:9:18:9:25 | source() | nested-props.js:10:10:10:16 | obj.x.y |
 | nested-props.js:35:13:35:20 | source() | nested-props.js:36:10:36:20 | doLoad(obj) |

javascript/ql/test/library-tests/TaintTracking/logical-and.js

@@ -0,0 +1,6 @@
+function test() {
+    var taint = source();
+
+    sink("safe" && taint); // NOT OK
+    sink(taint && "safe"); // OK
+}

javascript/ql/test/query-tests/Statements/UselessConditional/UselessConditional.expected

@@ -18,6 +18,7 @@
 | UselessConditional.js:94:16:94:16 | x | This use of variable 'x' always evaluates to false. |
 | UselessConditional.js:100:13:100:24 | true && true | This expression always evaluates to true. |
 | UselessConditional.js:101:18:101:18 | x | This use of variable 'x' always evaluates to false. |
+| UselessConditional.js:102:13:102:20 | y && (x) | This expression always evaluates to false. |
 | UselessConditional.js:102:19:102:19 | x | This use of variable 'x' always evaluates to false. |
 | UselessConditional.js:103:23:103:23 | x | This use of variable 'x' always evaluates to false. |
 | UselessConditional.js:109:15:109:16 | {} | This expression always evaluates to true. |