JS: bugfix in indexOf-based include test

javascript/ql/src/semmle/javascript/StringOps.qll

@@ -250,7 +250,7 @@ module StringOps {
         polarity = true and
         greater = indexOf and
         (
-          lesser.getIntValue() >= 0
+          lesser.getIntValue() = 0 and astNode.isInclusive()
           or
           lesser.getIntValue() = -1 and not astNode.isInclusive()
         )
@@ -258,7 +258,7 @@ module StringOps {
         polarity = false and
         lesser = indexOf and
         (
-          greater.getIntValue() = -1
+          greater.getIntValue() = -1 and astNode.isInclusive()
           or
           greater.getIntValue() = 0 and not astNode.isInclusive()
         )

javascript/ql/src/semmle/javascript/dataflow/TaintTracking.qll

@@ -688,6 +688,31 @@ module TaintTracking {
     override predicate appliesTo(Configuration cfg) { any() }
   }
 
+  /**
+   * A check of form `x.indexOf(y) > 0` or similar, which sanitizes `y` in the "then" branch.
+   *
+   * The more typical case of `x.indexOf(y) >= 0` is covered by `StringInclusionSanitizer`.
+   */
+  class PositiveIndexOfSanitizer extends AdditionalSanitizerGuardNode, DataFlow::ValueNode {
+    MethodCallExpr indexOf;
+    override RelationalComparison astNode;
+
+    PositiveIndexOfSanitizer() {
+      indexOf.getMethodName() = "indexOf" and
+      exists (int bound |
+        astNode.getGreaterOperand() = indexOf and
+        astNode.getLesserOperand().getIntValue() = bound and
+        bound >= 0)
+    }
+
+    override predicate sanitizes(boolean outcome, Expr e) {
+      outcome = true and
+      e = indexOf.getArgument(0)
+    }
+
+    override predicate appliesTo(Configuration cfg) { any() }
+  }
+
   /** A check of the form `if(x == 'some-constant')`, which sanitizes `x` in its "then" branch. */
   class ConstantComparison extends AdditionalSanitizerGuardNode, DataFlow::ValueNode {
     Expr x;

javascript/ql/test/library-tests/StringOps/Includes/Includes.expected

@@ -0,0 +1,7 @@
+| tst.js:4:7:4:19 | A.includes(B) | tst.js:4:7:4:7 | A | tst.js:4:18:4:18 | B | true |
+| tst.js:5:7:5:22 | _.includes(A, B) | tst.js:5:18:5:18 | A | tst.js:5:21:5:21 | B | true |
+| tst.js:6:7:6:25 | A.indexOf(B) !== -1 | tst.js:6:7:6:7 | A | tst.js:6:17:6:17 | B | true |
+| tst.js:7:7:7:23 | A.indexOf(B) >= 0 | tst.js:7:7:7:7 | A | tst.js:7:17:7:17 | B | true |
+| tst.js:8:7:8:19 | ~A.indexOf(B) | tst.js:8:8:8:8 | A | tst.js:8:18:8:18 | B | true |
+| tst.js:11:7:11:25 | A.indexOf(B) === -1 | tst.js:11:7:11:7 | A | tst.js:11:17:11:17 | B | false |
+| tst.js:12:7:12:22 | A.indexOf(B) < 0 | tst.js:12:7:12:7 | A | tst.js:12:17:12:17 | B | false |

javascript/ql/test/library-tests/StringOps/Includes/Includes.ql

@@ -0,0 +1,4 @@
+import javascript
+
+from StringOps::Includes include
+select include, include.getBaseString(), include.getSubstring(), include.getPolarity()

javascript/ql/test/library-tests/StringOps/Includes/tst.js

@@ -0,0 +1,18 @@
+import * as _ from 'lodash';
+
+function test() {
+  if (A.includes(B)) {}
+  if (_.includes(A, B)) {}
+  if (A.indexOf(B) !== -1) {}
+  if (A.indexOf(B) >= 0) {}
+  if (~A.indexOf(B)) {}
+  
+  // negated
+  if (A.indexOf(B) === -1) {}
+  if (A.indexOf(B) < 0) {}
+  
+  // non-examples
+  if (A.indexOf(B) === 0) {}
+  if (A.indexOf(B) !== 0) {}
+  if (A.indexOf(B) > 0) {}
+}

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

@@ -5,6 +5,7 @@
 | constructor-calls.js:10:16:10:23 | source() | constructor-calls.js:30:8:30:19 | d_safe.taint |
 | constructor-calls.js:14:15:14:22 | source() | constructor-calls.js:17:8:17:14 | c.param |
 | constructor-calls.js:14:15:14:22 | source() | constructor-calls.js:25:8:25:14 | d.param |
+| indexOf.js:4:11:4:18 | source() | indexOf.js:9:10:9:10 | x |
 | partialCalls.js:4:17:4:24 | source() | partialCalls.js:17:14:17:14 | x |
 | partialCalls.js:4:17:4:24 | source() | partialCalls.js:20:14:20:14 | y |
 | partialCalls.js:4:17:4:24 | source() | partialCalls.js:30:14:30:20 | x.value |

javascript/ql/test/library-tests/TaintTracking/indexOf.js

@@ -0,0 +1,15 @@
+let whitelist = ['a', 'b', 'c'];
+
+function test() {
+  let x = source();
+  
+  if (whitelist.indexOf(x) < -1) {
+    // unreachable
+  } else {
+    sink(x); // NOT OK
+  }
+
+  if (whitelist.indexOf(x) > 1) {
+    sink(x) // OK
+  }
+}