JS: Generalize StringOps::Includes to ::InclusionTest

javascript/ql/src/javascript.qll

@@ -28,6 +28,7 @@ import semmle.javascript.Functions
 import semmle.javascript.GlobalAccessPaths
 import semmle.javascript.HTML
 import semmle.javascript.HtmlSanitizers
+import semmle.javascript.InclusionTests
 import semmle.javascript.JSDoc
 import semmle.javascript.JSON
 import semmle.javascript.JsonParsers

javascript/ql/src/semmle/javascript/InclusionTests.qll

@@ -0,0 +1,173 @@
+/**
+ * Contains classes for recognizing array and string inclusion tests.
+ */
+private import javascript
+
+/**
+ * A expression that checks if an element is contained in an array
+ * or is a substring of another string.
+ *
+ * Examples:
+ * ```
+ * A.includes(B)
+ * A.indexOf(B) !== -1
+ * A.indexOf(B) >= 0
+ * ~A.indexOf(B)
+ * ```
+ */
+class InclusionTest extends DataFlow::Node {
+  InclusionTest::Range range;
+
+  InclusionTest() { this = range }
+
+  /** Gets the `A` in `A.includes(B)`. */
+  DataFlow::Node getContainerNode() { result = range.getContainerNode() }
+
+  /** Gets the `B` in `A.includes(B)`. */
+  DataFlow::Node getContainedNode() { result = range.getContainedNode() }
+
+  /**
+   * Gets the polarity of the check.
+   *
+   * If the polarity is `false` the check returns `true` if the container does not contain
+   * the given element.
+   */
+  boolean getPolarity() { result = range.getPolarity() }
+}
+
+module InclusionTest {
+  /**
+   * A expression that is equivalent to `A.includes(B)` or `!A.includes(B)`.
+   *
+   * Note that this also includes calls to the array method named `includes`.
+   */
+  abstract class Range extends DataFlow::Node {
+    /** Gets the `A` in `A.includes(B)`. */
+    abstract DataFlow::Node getContainerNode();
+
+    /** Gets the `B` in `A.includes(B)`. */
+    abstract DataFlow::Node getContainedNode();
+
+    /**
+     * Gets the polarity of the check.
+     *
+     * If the polarity is `false` the check returns `true` if the container does not contain
+     * the given element.
+     */
+    boolean getPolarity() { result = true }
+  }
+
+  /**
+   * A call to a method named `includes`, assumed to refer to `String.prototype.includes`
+   * or `Array.prototype.includes`.
+   */
+  private class Includes_Native extends Range, DataFlow::MethodCallNode {
+    Includes_Native() {
+      getMethodName() = "includes" and
+      getNumArgument() = 1
+    }
+
+    override DataFlow::Node getContainerNode() { result = getReceiver() }
+
+    override DataFlow::Node getContainedNode() { result = getArgument(0) }
+  }
+
+  /**
+   * A call to `_.includes` or similar, assumed to operate on strings.
+   */
+  private class Includes_Library extends Range, DataFlow::CallNode {
+    Includes_Library() {
+      exists(string name |
+        this = LodashUnderscore::member(name).getACall() and
+        (name = "includes" or name = "include" or name = "contains")
+        or
+        this = Closure::moduleImport("goog.string." + name).getACall() and
+        (name = "contains" or name = "caseInsensitiveContains")
+      )
+    }
+
+    override DataFlow::Node getContainerNode() { result = getArgument(0) }
+
+    override DataFlow::Node getContainedNode() { result = getArgument(1) }
+  }
+
+  /**
+   * A check of form `A.indexOf(B) !== -1` or similar.
+   */
+  private class Includes_IndexOfEquals extends Range, DataFlow::ValueNode {
+    MethodCallExpr indexOf;
+    override EqualityTest astNode;
+
+    Includes_IndexOfEquals() {
+      exists(Expr index | astNode.hasOperands(indexOf, index) |
+        // one operand is of the form `whitelist.indexOf(x)`
+        indexOf.getMethodName() = "indexOf" and
+        // and the other one is -1
+        index.getIntValue() = -1
+      )
+    }
+
+    override DataFlow::Node getContainerNode() { result = indexOf.getReceiver().flow() }
+
+    override DataFlow::Node getContainedNode() { result = indexOf.getArgument(0).flow() }
+
+    override boolean getPolarity() { result = astNode.getPolarity().booleanNot() }
+  }
+
+  /**
+   * A check of form `A.indexOf(B) >= 0` or similar.
+   */
+  private class Includes_IndexOfRelational extends Range, DataFlow::ValueNode {
+    MethodCallExpr indexOf;
+    override RelationalComparison astNode;
+    boolean polarity;
+
+    Includes_IndexOfRelational() {
+      exists(Expr lesser, Expr greater |
+        astNode.getLesserOperand() = lesser and
+        astNode.getGreaterOperand() = greater and
+        indexOf.getMethodName() = "indexOf" and
+        indexOf.getNumArgument() = 1
+      |
+        polarity = true and
+        greater = indexOf and
+        (
+          lesser.getIntValue() = 0 and astNode.isInclusive()
+          or
+          lesser.getIntValue() = -1 and not astNode.isInclusive()
+        )
+        or
+        polarity = false and
+        lesser = indexOf and
+        (
+          greater.getIntValue() = -1 and astNode.isInclusive()
+          or
+          greater.getIntValue() = 0 and not astNode.isInclusive()
+        )
+      )
+    }
+
+    override DataFlow::Node getContainerNode() { result = indexOf.getReceiver().flow() }
+
+    override DataFlow::Node getContainedNode() { result = indexOf.getArgument(0).flow() }
+
+    override boolean getPolarity() { result = polarity }
+  }
+
+  /**
+   * An expression of form `~A.indexOf(B)` which, when coerced to a boolean, is equivalent to `A.includes(B)`.
+   */
+  private class Includes_IndexOfBitwise extends Range, DataFlow::ValueNode {
+    MethodCallExpr indexOf;
+    override BitNotExpr astNode;
+
+    Includes_IndexOfBitwise() {
+      astNode.getOperand() = indexOf and
+      indexOf.getMethodName() = "indexOf"
+    }
+
+    override DataFlow::Node getContainerNode() { result = indexOf.getReceiver().flow() }
+
+    override DataFlow::Node getContainedNode() { result = indexOf.getArgument(0).flow() }
+  }
+}

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

@@ -185,162 +185,15 @@ module StringOps {
   /**
    * A expression that is equivalent to `A.includes(B)` or `!A.includes(B)`.
    *
-   * Note that this also includes calls to the array method named `includes`.
+   * Note that this class is equivalent to `InclusionTest`, which also matches
+   * inclusion tests on array objects.
    */
-  class Includes extends DataFlow::Node {
+  class Includes extends InclusionTest {
-    Includes::Range range;
-
-    Includes() { this = range }
-
     /** Gets the `A` in `A.includes(B)`. */
-    DataFlow::Node getBaseString() { result = range.getBaseString() }
+    DataFlow::Node getBaseString() { result = getContainerNode() }
 
     /** Gets the `B` in `A.includes(B)`. */
-    DataFlow::Node getSubstring() { result = range.getSubstring() }
+    DataFlow::Node getSubstring() { result = getContainedNode() }
-
-    /**
-     * Gets the polarity of the check.
-     *
-     * If the polarity is `false` the check returns `true` if the string does not contain
-     * the given substring.
-     */
-    boolean getPolarity() { result = range.getPolarity() }
-  }
-
-  module Includes {
-    /**
-     * A expression that is equivalent to `A.includes(B)` or `!A.includes(B)`.
-     *
-     * Note that this also includes calls to the array method named `includes`.
-     */
-    abstract class Range extends DataFlow::Node {
-      /** Gets the `A` in `A.includes(B)`. */
-      abstract DataFlow::Node getBaseString();
-
-      /** Gets the `B` in `A.includes(B)`. */
-      abstract DataFlow::Node getSubstring();
-
-      /**
-       * Gets the polarity of the check.
-       *
-       * If the polarity is `false` the check returns `true` if the string does not contain
-       * the given substring.
-       */
-      boolean getPolarity() { result = true }
-    }
-
-    /**
-     * A call to a method named `includes`, assumed to refer to `String.prototype.includes`.
-     */
-    private class Includes_Native extends Range, DataFlow::MethodCallNode {
-      Includes_Native() {
-        getMethodName() = "includes" and
-        getNumArgument() = 1
-      }
-
-      override DataFlow::Node getBaseString() { result = getReceiver() }
-
-      override DataFlow::Node getSubstring() { result = getArgument(0) }
-    }
-
-    /**
-     * A call to `_.includes` or similar, assumed to operate on strings.
-     */
-    private class Includes_Library extends Range, DataFlow::CallNode {
-      Includes_Library() {
-        exists(string name |
-          this = LodashUnderscore::member(name).getACall() and
-          (name = "includes" or name = "include" or name = "contains")
-          or
-          this = Closure::moduleImport("goog.string." + name).getACall() and
-          (name = "contains" or name = "caseInsensitiveContains")
-        )
-      }
-
-      override DataFlow::Node getBaseString() { result = getArgument(0) }
-
-      override DataFlow::Node getSubstring() { result = getArgument(1) }
-    }
-
-    /**
-     * A check of form `A.indexOf(B) !== -1` or similar.
-     */
-    private class Includes_IndexOfEquals extends Range, DataFlow::ValueNode {
-      MethodCallExpr indexOf;
-      override EqualityTest astNode;
-
-      Includes_IndexOfEquals() {
-        exists(Expr index | astNode.hasOperands(indexOf, index) |
-          // one operand is of the form `whitelist.indexOf(x)`
-          indexOf.getMethodName() = "indexOf" and
-          // and the other one is -1
-          index.getIntValue() = -1
-        )
-      }
-
-      override DataFlow::Node getBaseString() { result = indexOf.getReceiver().flow() }
-
-      override DataFlow::Node getSubstring() { result = indexOf.getArgument(0).flow() }
-
-      override boolean getPolarity() { result = astNode.getPolarity().booleanNot() }
-    }
-
-    /**
-     * A check of form `A.indexOf(B) >= 0` or similar.
-     */
-    private class Includes_IndexOfRelational extends Range, DataFlow::ValueNode {
-      MethodCallExpr indexOf;
-      override RelationalComparison astNode;
-      boolean polarity;
-
-      Includes_IndexOfRelational() {
-        exists(Expr lesser, Expr greater |
-          astNode.getLesserOperand() = lesser and
-          astNode.getGreaterOperand() = greater and
-          indexOf.getMethodName() = "indexOf" and
-          indexOf.getNumArgument() = 1
-        |
-          polarity = true and
-          greater = indexOf and
-          (
-            lesser.getIntValue() = 0 and astNode.isInclusive()
-            or
-            lesser.getIntValue() = -1 and not astNode.isInclusive()
-          )
-          or
-          polarity = false and
-          lesser = indexOf and
-          (
-            greater.getIntValue() = -1 and astNode.isInclusive()
-            or
-            greater.getIntValue() = 0 and not astNode.isInclusive()
-          )
-        )
-      }
-
-      override DataFlow::Node getBaseString() { result = indexOf.getReceiver().flow() }
-
-      override DataFlow::Node getSubstring() { result = indexOf.getArgument(0).flow() }
-
-      override boolean getPolarity() { result = polarity }
-    }
-
-    /**
-     * An expression of form `~A.indexOf(B)` which, when coerced to a boolean, is equivalent to `A.includes(B)`.
-     */
-    private class Includes_IndexOfBitwise extends Range, DataFlow::ValueNode {
-      MethodCallExpr indexOf;
-      override BitNotExpr astNode;
-
-      Includes_IndexOfBitwise() {
-        astNode.getOperand() = indexOf and
-        indexOf.getMethodName() = "indexOf"
-      }
-
-      override DataFlow::Node getBaseString() { result = indexOf.getReceiver().flow() }
-
-      override DataFlow::Node getSubstring() { result = indexOf.getArgument(0).flow() }
-    }
   }
 
   /**

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

@@ -781,15 +781,18 @@ module TaintTracking {
     override predicate appliesTo(Configuration cfg) { any() }
   }
 
-  /** A check of the form `whitelist.includes(x)` or equivalent, which sanitizes `x` in its "then" branch. */
+  /** DEPRECATED. This class has been renamed to `InclusionSanitizer`. */
-  class StringInclusionSanitizer extends AdditionalSanitizerGuardNode {
+  deprecated class StringInclusionSanitizer = InclusionSanitizer;
-    StringOps::Includes includes;
 
-    StringInclusionSanitizer() { this = includes }
+  /** A check of the form `whitelist.includes(x)` or equivalent, which sanitizes `x` in its "then" branch. */
+  class InclusionSanitizer extends AdditionalSanitizerGuardNode {
+    InclusionTest inclusion;
+
+    InclusionSanitizer() { this = inclusion }
 
     override predicate sanitizes(boolean outcome, Expr e) {
-      outcome = includes.getPolarity() and
+      outcome = inclusion.getPolarity() and
-      e = includes.getSubstring().asExpr()
+      e = inclusion.getContainedNode().asExpr()
     }
 
     override predicate appliesTo(Configuration cfg) { any() }