Ruby: Change HTTP::Client::Request to have DataFlow::Node as base class

Although this is a breaking change, as explained in the change-note, it
should onyl affect peopel that have created their own HTTP client
request modeling, which I assume is none.

The alternative would have been to keep the old class/module as
deprecated, and introduce a `HTTP::Client::Requestv2` class/module that
is based on `DataFlow::Node` instead. The old class could then be
deprecated in 1 year, and we could do a rename from
`HTTP::Client::Requestv2` -> `HTTP::Client::Request` at the same time.
(and then wait 1 more year before being able to delete
`HTTP::Client::Requestv2`)

All in all, I think this is the right tradeoff, given that CodeQL Ruby
is still in beta.

Author: RasmusWL

ruby/ql/lib/change-notes/2022-08-18-change-http-client-request.md

@@ -0,0 +1,4 @@
+---
+category: breaking
+---
+* Changed the `HTTP::Client::Request` concept from using `MethodCall` as base class, to using `DataFlow::Node` as base class. Any class that extend `HTTP::Client::Request::Range` must be changed, but if you only uses the member predicates of `HTTP::Client::Request` no changes are required.

ruby/ql/lib/codeql/ruby/Concepts.qll

@@ -480,7 +480,7 @@ module HTTP {
      * Extend this class to refine existing API models. If you want to model new APIs,
      * extend `Request::Range` instead.
      */
-    class Request extends MethodCall instanceof Request::Range {
+    class Request extends DataFlow::Node instanceof Request::Range {
       /** Gets a node which returns the body of the response */
       DataFlow::Node getResponseBody() { result = super.getResponseBody() }
 
@@ -519,7 +519,7 @@ module HTTP {
        * Extend this class to model new APIs. If you want to refine existing API models,
        * extend `Request` instead.
        */
-      abstract class Range extends MethodCall {
+      abstract class Range extends DataFlow::Node {
         /** Gets a node which returns the body of the response */
         abstract DataFlow::Node getResponseBody();
 

ruby/ql/lib/codeql/ruby/frameworks/http_clients/Excon.qll

@@ -23,14 +23,13 @@ private import codeql.ruby.DataFlow
  * TODO: pipelining, streaming responses
  * https://github.com/excon/excon/blob/master/README.md
  */
-class ExconHttpRequest extends HTTP::Client::Request::Range {
-  DataFlow::CallNode requestUse;
+class ExconHttpRequest extends HTTP::Client::Request::Range, DataFlow::CallNode {
   API::Node requestNode;
   API::Node connectionNode;
   DataFlow::Node connectionUse;
 
   ExconHttpRequest() {
-    requestUse = requestNode.asSource() and
+    this = requestNode.asSource() and
     connectionUse = connectionNode.asSource() and
     connectionNode =
       [
@@ -46,8 +45,7 @@ class ExconHttpRequest extends HTTP::Client::Request::Range {
               // Excon#request exists but Excon.request doesn't.
               // This shouldn't be a problem - in real code the latter would raise NoMethodError anyway.
               "get", "head", "delete", "options", "post", "put", "patch", "trace", "request"
-            ]) and
-    this = requestUse.asExpr().getExpr()
+            ])
   }
 
   override DataFlow::Node getResponseBody() { result = requestNode.getAMethodCall("body") }
@@ -56,9 +54,9 @@ class ExconHttpRequest extends HTTP::Client::Request::Range {
     // For one-off requests, the URL is in the first argument of the request method call.
     // For connection re-use, the URL is split between the first argument of the `new` call
     // and the `path` keyword argument of the request method call.
-    result = requestUse.getArgument(0) and not result.asExpr().getExpr() instanceof Pair
+    result = this.getArgument(0) and not result.asExpr().getExpr() instanceof Pair
     or
-    result = requestUse.getKeywordArgument("path")
+    result = this.getKeywordArgument("path")
     or
     result = connectionUse.(DataFlow::CallNode).getArgument(0)
   }
@@ -77,7 +75,7 @@ class ExconHttpRequest extends HTTP::Client::Request::Range {
     // the request call.
     exists(DataFlow::CallNode disableCall |
       setsDefaultVerification(disableCall, false) and
-      disableCall.asExpr().getASuccessor+() = requestUse.asExpr() and
+      disableCall.asExpr().getASuccessor+() = this.asExpr() and
       disablingNode = disableCall and
       not exists(DataFlow::Node arg, int i |
         i > 0 and

ruby/ql/lib/codeql/ruby/frameworks/http_clients/Faraday.qll

@@ -22,11 +22,12 @@ private import codeql.ruby.DataFlow
  * connection.get("/").body
  * ```
  */
-class FaradayHttpRequest extends HTTP::Client::Request::Range {
+
+class FaradayHttpRequest extends HTTP::Client::Request::Range, DataFlow::CallNode {
   API::Node requestNode;
   API::Node connectionNode;
   DataFlow::Node connectionUse;
-  DataFlow::CallNode requestUse;
+
 
   FaradayHttpRequest() {
     connectionNode =
@@ -38,15 +39,15 @@ class FaradayHttpRequest extends HTTP::Client::Request::Range {
       ] and
     requestNode =
       connectionNode.getReturn(["get", "head", "delete", "post", "put", "patch", "trace"]) and
-    requestUse = requestNode.asSource() and
-    connectionUse = connectionNode.asSource() and
-    this = requestUse.asExpr().getExpr()
+    this = requestNode.asSource() and
+    connectionUse = connectionNode.asSource()
+
   }
 
   override DataFlow::Node getResponseBody() { result = requestNode.getAMethodCall("body") }
 
   override DataFlow::Node getAUrlPart() {
-    result = requestUse.getArgument(0) or
+    result = this.getArgument(0) or
     result = connectionUse.(DataFlow::CallNode).getArgument(0) or
     result = connectionUse.(DataFlow::CallNode).getKeywordArgument("url")
   }

ruby/ql/lib/codeql/ruby/frameworks/http_clients/HttpClient.qll

@@ -14,10 +14,9 @@ private import codeql.ruby.DataFlow
  * HTTPClient.get_content("http://example.com")
  * ```
  */
-class HttpClientRequest extends HTTP::Client::Request::Range {
+class HttpClientRequest extends HTTP::Client::Request::Range, DataFlow::CallNode {
   API::Node requestNode;
   API::Node connectionNode;
-  DataFlow::CallNode requestUse;
   string method;
 
   HttpClientRequest() {
@@ -29,20 +28,19 @@ class HttpClientRequest extends HTTP::Client::Request::Range {
         API::getTopLevelMember("HTTPClient").getInstance()
       ] and
     requestNode = connectionNode.getReturn(method) and
-    requestUse = requestNode.asSource() and
+    this = requestNode.asSource() and
     method in [
         "get", "head", "delete", "options", "post", "put", "trace", "get_content", "post_content"
-      ] and
-    this = requestUse.asExpr().getExpr()
+      ]
   }
 
-  override DataFlow::Node getAUrlPart() { result = requestUse.getArgument(0) }
+  override DataFlow::Node getAUrlPart() { result = this.getArgument(0) }
 
   override DataFlow::Node getResponseBody() {
     // The `get_content` and `post_content` methods return the response body as
     // a string. The other methods return a `HTTPClient::Message` object which
     // has various methods that return the response body.
-    method in ["get_content", "post_content"] and result = requestUse
+    method in ["get_content", "post_content"] and result = this
     or
     not method in ["get_content", "put_content"] and
     result = requestNode.getAMethodCall(["body", "http_body", "content", "dump"])

ruby/ql/lib/codeql/ruby/frameworks/http_clients/Httparty.qll

@@ -23,19 +23,19 @@ private import codeql.ruby.DataFlow
  * MyClass.new("http://example.com")
  * ```
  */
-class HttpartyRequest extends HTTP::Client::Request::Range {
+class HttpartyRequest extends HTTP::Client::Request::Range,DataFlow::CallNode {
   API::Node requestNode;
-  DataFlow::CallNode requestUse;
+
 
   HttpartyRequest() {
-    requestUse = requestNode.asSource() and
+    this = requestNode.asSource() and
     requestNode =
       API::getTopLevelMember("HTTParty")
-          .getReturn(["get", "head", "delete", "options", "post", "put", "patch"]) and
-    this = requestUse.asExpr().getExpr()
+          .getReturn(["get", "head", "delete", "options", "post", "put", "patch"])
+
   }
 
-  override DataFlow::Node getAUrlPart() { result = requestUse.getArgument(0) }
+  override DataFlow::Node getAUrlPart() { result = this.getArgument(0) }
 
   override DataFlow::Node getResponseBody() {
     // If HTTParty can recognise the response type, it will parse and return it
@@ -46,7 +46,7 @@ class HttpartyRequest extends HTTP::Client::Request::Range {
     or
     // Otherwise, treat the response as the response body.
     not exists(requestNode.getAMethodCall("body")) and
-    result = requestUse
+    result = this
   }
 
   override predicate disablesCertificateValidation(DataFlow::Node disablingNode) {
@@ -55,7 +55,7 @@ class HttpartyRequest extends HTTP::Client::Request::Range {
     // `{ verify_peer: false }`.
     exists(DataFlow::Node arg, int i |
       i > 0 and
-      arg.asExpr() = requestUse.asExpr().(CfgNodes::ExprNodes::MethodCallCfgNode).getArgument(i)
+      arg.asExpr() = this.asExpr().(CfgNodes::ExprNodes::MethodCallCfgNode).getArgument(i)
     |
       // Either passed as an individual key:value argument, e.g.:
       // HTTParty.get(..., verify: false)

ruby/ql/lib/codeql/ruby/frameworks/http_clients/NetHttp.qll

@@ -18,15 +18,15 @@ private import codeql.ruby.DataFlow
  * response = req.get("/")
  * ```
  */
-class NetHttpRequest extends HTTP::Client::Request::Range {
+class NetHttpRequest extends HTTP::Client::Request::Range, DataFlow::CallNode {
   private DataFlow::CallNode request;
   private DataFlow::Node responseBody;
   private API::Node requestNode;
 
   NetHttpRequest() {
     exists(string method |
       request = requestNode.asSource() and
-      this = request.asExpr().getExpr()
+      this = request
     |
       // Net::HTTP.get(...)
       method = "get" and

ruby/ql/lib/codeql/ruby/frameworks/http_clients/OpenURI.qll

@@ -18,29 +18,27 @@ private import codeql.ruby.frameworks.Core
  * URI.parse("http://example.com").open.read
  * ```
  */
-class OpenUriRequest extends HTTP::Client::Request::Range {
+class OpenUriRequest extends HTTP::Client::Request::Range, DataFlow::CallNode {
   API::Node requestNode;
-  DataFlow::CallNode requestUse;
 
   OpenUriRequest() {
     requestNode =
       [
         [API::getTopLevelMember("URI"), API::getTopLevelMember("URI").getReturn("parse")]
             .getReturn("open"), API::getTopLevelMember("OpenURI").getReturn("open_uri")
       ] and
-    requestUse = requestNode.asSource() and
-    this = requestUse.asExpr().getExpr()
+    this = requestNode.asSource()
   }
 
-  override DataFlow::Node getAUrlPart() { result = requestUse.getArgument(0) }
+  override DataFlow::Node getAUrlPart() { result = this.getArgument(0) }
 
   override DataFlow::Node getResponseBody() {
     result = requestNode.getAMethodCall(["read", "readlines"])
   }
 
   override predicate disablesCertificateValidation(DataFlow::Node disablingNode) {
     exists(DataFlow::Node arg |
-      arg.asExpr() = requestUse.asExpr().(CfgNodes::ExprNodes::MethodCallCfgNode).getAnArgument() and
+      arg.asExpr() = this.asExpr().(CfgNodes::ExprNodes::MethodCallCfgNode).getAnArgument() and
       argumentDisablesValidation(arg, disablingNode)
     )
   }
@@ -56,26 +54,23 @@ class OpenUriRequest extends HTTP::Client::Request::Range {
  * Kernel.open("http://example.com").read
  * ```
  */
-class OpenUriKernelOpenRequest extends HTTP::Client::Request::Range {
-  DataFlow::CallNode requestUse;
-
+class OpenUriKernelOpenRequest extends HTTP::Client::Request::Range, DataFlow::CallNode {
   OpenUriKernelOpenRequest() {
-    requestUse instanceof KernelMethodCall and
-    this.getMethodName() = "open" and
-    this = requestUse.asExpr().getExpr()
+    this instanceof KernelMethodCall and
+    this.getMethodName() = "open"
   }
 
-  override DataFlow::Node getAUrlPart() { result = requestUse.getArgument(0) }
+  override DataFlow::Node getAUrlPart() { result = this.getArgument(0) }
 
   override DataFlow::CallNode getResponseBody() {
     result.asExpr().getExpr().(MethodCall).getMethodName() in ["read", "readlines"] and
-    requestUse.(DataFlow::LocalSourceNode).flowsTo(result.getReceiver())
+    this.(DataFlow::LocalSourceNode).flowsTo(result.getReceiver())
   }
 
   override predicate disablesCertificateValidation(DataFlow::Node disablingNode) {
     exists(DataFlow::Node arg, int i |
       i > 0 and
-      arg.asExpr() = requestUse.asExpr().(CfgNodes::ExprNodes::MethodCallCfgNode).getArgument(i) and
+      arg.asExpr() = this.asExpr().(CfgNodes::ExprNodes::MethodCallCfgNode).getArgument(i) and
       argumentDisablesValidation(arg, disablingNode)
     )
   }

ruby/ql/lib/codeql/ruby/frameworks/http_clients/RestClient.qll

@@ -16,14 +16,14 @@ private import codeql.ruby.DataFlow
  * RestClient::Request.execute(url: "http://example.com").body
  * ```
  */
-class RestClientHttpRequest extends HTTP::Client::Request::Range {
-  DataFlow::CallNode requestUse;
+class RestClientHttpRequest extends HTTP::Client::Request::Range, DataFlow::CallNode {
+
   API::Node requestNode;
   API::Node connectionNode;
 
   RestClientHttpRequest() {
-    requestUse = requestNode.asSource() and
-    this = requestUse.asExpr().getExpr() and
+    this = requestNode.asSource() and
+
     (
       connectionNode =
         [
@@ -39,9 +39,9 @@ class RestClientHttpRequest extends HTTP::Client::Request::Range {
   }
 
   override DataFlow::Node getAUrlPart() {
-    result = requestUse.getKeywordArgument("url")
+    result = this.getKeywordArgument("url")
     or
-    result = requestUse.getArgument(0) and
+    result = this.getArgument(0) and
     // this rules out the alternative above
     not result.asExpr().getExpr() instanceof Pair
   }

ruby/ql/lib/codeql/ruby/frameworks/http_clients/Typhoeus.qll

@@ -14,26 +14,26 @@ private import codeql.ruby.DataFlow
  * Typhoeus.get("http://example.com").body
  * ```
  */
-class TyphoeusHttpRequest extends HTTP::Client::Request::Range {
-  DataFlow::CallNode requestUse;
+class TyphoeusHttpRequest extends HTTP::Client::Request::Range, DataFlow::CallNode {
+
   API::Node requestNode;
 
   TyphoeusHttpRequest() {
-    requestUse = requestNode.asSource() and
+    this = requestNode.asSource() and
     requestNode =
       API::getTopLevelMember("Typhoeus")
-          .getReturn(["get", "head", "delete", "options", "post", "put", "patch"]) and
-    this = requestUse.asExpr().getExpr()
+          .getReturn(["get", "head", "delete", "options", "post", "put", "patch"])
+
   }
 
-  override DataFlow::Node getAUrlPart() { result = requestUse.getArgument(0) }
+  override DataFlow::Node getAUrlPart() { result = this.getArgument(0) }
 
   override DataFlow::Node getResponseBody() { result = requestNode.getAMethodCall("body") }
 
   override predicate disablesCertificateValidation(DataFlow::Node disablingNode) {
     // Check for `ssl_verifypeer: false` in the options hash.
     exists(DataFlow::Node arg, int i |
-      i > 0 and arg.asExpr().getExpr() = requestUse.asExpr().getExpr().(MethodCall).getArgument(i)
+      i > 0 and arg.asExpr().getExpr() = this.asExpr().getExpr().(MethodCall).getArgument(i)
     |
       // Either passed as an individual key:value argument, e.g.:
       // Typhoeus.get(..., ssl_verifypeer: false)