Ruby: IncompleteUrlSubstringSanitization.ql

Author: aibaars

ruby/ql/lib/codeql/ruby/InclusionTests.qll

@@ -2,25 +2,26 @@
  * Contains classes for recognizing array and string inclusion tests.
  */
 
-private import javascript
+private import ruby
+private import codeql.ruby.DataFlow
+private import codeql.ruby.controlflow.CfgNodes
 
 /**
  * An 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)
+ * A.include?(B)
+ * A.index(B) !== -1
+ * A.index(B) >= 0
  * ```
  */
 class InclusionTest extends DataFlow::Node instanceof InclusionTest::Range {
-  /** Gets the `A` in `A.includes(B)`. */
+  /** Gets the `A` in `A.include?(B)`. */
   DataFlow::Node getContainerNode() { result = super.getContainerNode() }
 
-  /** Gets the `B` in `A.includes(B)`. */
+  /** Gets the `B` in `A.include?(B)`. */
   DataFlow::Node getContainedNode() { result = super.getContainedNode() }
 
   /**
@@ -34,15 +35,15 @@ class InclusionTest extends DataFlow::Node instanceof InclusionTest::Range {
 
 module InclusionTest {
   /**
-   * A expression that is equivalent to `A.includes(B)` or `!A.includes(B)`.
+   * A expression that is equivalent to `A.include?(B)` or `!A.include?(B)`.
    *
-   * Note that this also includes calls to the array method named `includes`.
+   * Note that this also includes calls to the array method named `include?`.
    */
   abstract class Range extends DataFlow::Node {
-    /** Gets the `A` in `A.includes(B)`. */
+    /** Gets the `A` in `A.include?(B)`. */
     abstract DataFlow::Node getContainerNode();
 
-    /** Gets the `B` in `A.includes(B)`. */
+    /** Gets the `B` in `A.include?(B)`. */
     abstract DataFlow::Node getContainedNode();
 
     /**
@@ -55,47 +56,13 @@ module InclusionTest {
   }
 
   /**
-   * A call to a utility function (`callee`) that performs an InclusionTest (`inner`).
+   * A call to a method named `include?`, assumed to refer to `String.include?`
+   * or `Array.include?`.
    */
-  private class IndirectInclusionTest extends Range, DataFlow::CallNode {
-    InclusionTest inner;
-    Function callee;
-
-    IndirectInclusionTest() {
-      inner.getEnclosingExpr() = unique(Expr ret | ret = callee.getAReturnedExpr()) and
-      callee = unique(Function f | f = this.getACallee()) and
-      not this.isImprecise() and
-      inner.getContainedNode().getALocalSource() = DataFlow::parameterNode(callee.getAParameter()) and
-      inner.getContainerNode().getALocalSource() = DataFlow::parameterNode(callee.getAParameter())
-    }
-
-    override DataFlow::Node getContainerNode() {
-      exists(int arg |
-        inner.getContainerNode().getALocalSource() =
-          DataFlow::parameterNode(callee.getParameter(arg)) and
-        result = this.getArgument(arg)
-      )
-    }
-
-    override DataFlow::Node getContainedNode() {
-      exists(int arg |
-        inner.getContainedNode().getALocalSource() =
-          DataFlow::parameterNode(callee.getParameter(arg)) and
-        result = this.getArgument(arg)
-      )
-    }
-
-    override boolean getPolarity() { result = inner.getPolarity() }
-  }
-
-  /**
-   * 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 {
+  private class Includes_Native extends Range, DataFlow::CallNode {
     Includes_Native() {
-      this.getMethodName() = "includes" and
-      this.getNumArgument() = 1
+      this.getMethodName() = "include?" and
+      count(this.getArgument(_)) = 1
     }
 
     override DataFlow::Node getContainerNode() { result = this.getReceiver() }
@@ -104,101 +71,60 @@ module InclusionTest {
   }
 
   /**
-   * A call to `_.includes` or similar, assumed to operate on strings.
+   * A check of form `A.index(B) != -1`, `A.index(B) >= 0`, or similar.
    */
-  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 = this.getArgument(0) }
-
-    override DataFlow::Node getContainedNode() { result = this.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
+  private class Includes_IndexOfComparison extends Range, DataFlow::Node {
+    private DataFlow::CallNode indexOf;
+    private boolean polarity;
+
+    Includes_IndexOfComparison() {
+      exists(ExprCfgNode index, ExprNodes::ComparisonOperationCfgNode comparison, int value |
+        indexOf.asExpr() = comparison.getAnOperand() and
+        index = comparison.getAnOperand() and
+        this.asExpr() = comparison and
+        // one operand is of the form `whitelist.index(x)`
+        indexOf.getMethodName() = "index" and
+        // and the other one is 0 or -1
+        value = index.getConstantValue().getInt() and
+        value = [0, -1]
       |
-        polarity = true and
-        greater = indexOf and
-        (
-          lesser.getIntValue() = 0 and astNode.isInclusive()
-          or
-          lesser.getIntValue() = -1 and not astNode.isInclusive()
-        )
+        value = -1 and polarity = false and comparison.getExpr() instanceof CaseEqExpr
         or
-        polarity = false and
-        lesser = indexOf and
-        (
-          greater.getIntValue() = -1 and astNode.isInclusive()
-          or
-          greater.getIntValue() = 0 and not astNode.isInclusive()
+        value = -1 and polarity = false and comparison.getExpr() instanceof EqExpr
+        or
+        value = -1 and polarity = true and comparison.getExpr() instanceof NEExpr
+        or
+        value = 0 and polarity = false and comparison.getExpr() instanceof NEExpr
+        or
+        exists(RelationalOperation op | op = comparison.getExpr() |
+          exists(Expr lesser, Expr greater |
+            op.getLesserOperand() = lesser and
+            op.getGreaterOperand() = greater
+          |
+            polarity = true and
+            greater = indexOf.asExpr().getExpr() and
+            (
+              value = 0 and op.isInclusive()
+              or
+              value = -1 and not op.isInclusive()
+            )
+            or
+            polarity = false and
+            lesser = indexOf.asExpr().getExpr() and
+            (
+              value = -1 and op.isInclusive()
+              or
+              value = 0 and not op.isInclusive()
+            )
+          )
         )
       )
     }
 
-    override DataFlow::Node getContainerNode() { result = indexOf.getReceiver().flow() }
+    override DataFlow::Node getContainerNode() { result = indexOf.getReceiver() }
 
-    override DataFlow::Node getContainedNode() { result = indexOf.getArgument(0).flow() }
+    override DataFlow::Node getContainedNode() { result = indexOf.getArgument(0) }
 
     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() }
-  }
 }