refactor query to use cfg and dataflow

ruby/ql/src/experimental/manually-check-http-verb/ManuallyCheckHttpVerb.ql

@@ -11,43 +11,83 @@
 
 import ruby
 import codeql.ruby.DataFlow
+import codeql.ruby.controlflow.CfgNodes
+import codeql.ruby.frameworks.ActionController
 
-class CheckNotGetRequest extends ConditionalExpr {
-  CheckNotGetRequest() { this.getCondition() instanceof CheckGetRequest }
+// any `request` calls in an action method
+class Request extends DataFlow::CallNode {
+  Request() {
+    this.getMethodName() = "request" and
+    this.asExpr().getExpr() instanceof ActionControllerActionMethod
+  }
 }
 
-class CheckGetRequest extends MethodCall {
-  CheckGetRequest() { this.getMethodName() = "get?" }
+// `request.request_method`
+class RequestRequestMethod extends DataFlow::CallNode {
+  RequestRequestMethod() {
+    this.getMethodName() = "request_method" and
+    any(Request r).flowsTo(this.getReceiver())
+  }
 }
 
-class ControllerClass extends ModuleBase {
-  ControllerClass() { this.getModule().getSuperClass+().toString() = "ApplicationController" }
+// `request.method`
+class RequestMethod extends DataFlow::CallNode {
+  RequestMethod() {
+    this.getMethodName() = "method" and
+    any(Request r).flowsTo(this.getReceiver())
+  }
 }
 
-class CheckGetFromEnv extends AstNode {
-  CheckGetFromEnv() {
-    // is this node an instance of `env["REQUEST_METHOD"]
-    this instanceof GetRequestMethodFromEnv and
-    // check if env["REQUEST_METHOD"] is compared to GET
-    exists(EqualityOperation eq | eq.getAChild() = this |
-      eq.getAChild().(StringLiteral).toString() = "GET"
+// `request.raw_request_method`
+class RequestRawRequestMethod extends DataFlow::CallNode {
+  RequestRawRequestMethod() {
+    this.getMethodName() = "raw_request_method" and
+    any(Request r).flowsTo(this.getReceiver())
+  }
+}
+
+// `request.request_method_symbol`
+class RequestRequestMethodSymbol extends DataFlow::CallNode {
+  RequestRequestMethodSymbol() {
+    this.getMethodName() = "request_method_symbol" and
+    any(Request r).flowsTo(this.getReceiver())
+  }
+}
+
+// `request.get?`
+class RequestGet extends DataFlow::CallNode {
+  RequestGet() {
+    this.getMethodName() = "get?" and
+    any(Request r).flowsTo(this.getReceiver())
+  }
+}
+
+// A conditional expression where the condition uses `request.method`, `request.request_method`, `request.raw_request_method`, `request.request_method_symbol`, or `request.get?` in some way.
+// e.g.
+// ```
+// r = request.request_method
+// if r == "GET"
+// ...
+// ```
+class RequestMethodConditional extends DataFlow::Node {
+  RequestMethodConditional() {
+    // We have to cast the dataflow node down to a specific CFG node (`ExprNodes::ConditionalExprCfgNode`) to be able to call `getCondition()`.
+    // We then find the dataflow node corresponding to the condition CFG node,
+    // and filter for just nodes where a request method accessor value flows to them.
+    exists(DataFlow::Node conditionNode |
+      conditionNode.asExpr() = this.asExpr().(ExprNodes::ConditionalExprCfgNode).getCondition()
+    |
+      (
+        any(RequestMethod r).flowsTo(conditionNode) or
+        any(RequestRequestMethod r).flowsTo(conditionNode) or
+        any(RequestRawRequestMethod r).flowsTo(conditionNode) or
+        any(RequestRequestMethodSymbol r).flowsTo(conditionNode) or
+        any(RequestGet r).flowsTo(conditionNode)
+      )
     )
   }
 }
 
-class GetRequestMethodFromEnv extends ElementReference {
-  GetRequestMethodFromEnv() {
-    this.getAChild+().toString() = "REQUEST_METHOD" and
-    this.getAChild+().toString() = "env"
-  }
-}
-
-from AstNode node
-where
-  (
-    node instanceof CheckNotGetRequest or
-    node instanceof CheckGetFromEnv
-  ) and
-  node.getEnclosingModule() instanceof ControllerClass
-select node,
+from RequestMethodConditional req
+select req,
   "Manually checking HTTP verbs is an indication that multiple requests are routed to the same controller action. This could lead to bypassing necessary authorization methods and other protections, like CSRF protection. Prefer using different controller actions for each HTTP method and relying Rails routing to handle mappting resources and verbs to specific methods."

ruby/ql/test/query-tests/experimental/manually-check-http-verb/ExampleController.rb

@@ -3,17 +3,6 @@ class ExampleController < ActionController::Base
   def example_action
     if request.get?
       Example.find(params[:example_id])
-    elsif request.post?
-      Example.new(params[:example_name], params[:example_details])
-    elsif request.delete?
-      example = Example.find(params[:example_id])
-      example.delete
-    elsif request.put?
-      Example.upsert(params[:example_name], params[:example_details])
-    elsif request.path?
-      Example.update(params[:example_name], params[:example_details])
-    elsif request.head?
-      "This is the endpoint for the Example resource."
     end
   end
 end

ruby/ql/test/query-tests/experimental/manually-check-http-verb/NotController.rb

@@ -0,0 +1,17 @@
+# There should be no hits from this class because it does not inherit from ActionController
+class NotAController
+  def example_action
+    if request.get?
+      Example.find(params[:example_id])
+    end
+  end
+
+  def resource_action
+    case env['REQUEST_METHOD']
+    when "GET"
+      Resource.find(params[:id])
+    when "POST"
+      Resource.new(params[:id], params[:details])
+    end
+  end
+end

ruby/ql/test/query-tests/security/manually-check-http-verb/NotController.rb

@@ -1,28 +0,0 @@
-# There should be no hits from this class because it does not inherit from ActionController
-class NotAController
-  def example_action
-    if request.get?
-      Example.find(params[:example_id])
-    elsif request.post?
-      Example.new(params[:example_name], params[:example_details])
-    elsif request.delete?
-      example = Example.find(params[:example_id])
-      example.delete
-    elsif request.put?
-      Example.upsert(params[:example_name], params[:example_details])
-    elsif request.path?
-      Example.update(params[:example_name], params[:example_details])
-    elsif request.head?
-      "This is the endpoint for the Example resource."
-    end
-  end
-
-  def resource_action
-    case env['REQUEST_METHOD']
-    when "GET"
-      Resource.find(params[:id])
-    when "POST"
-      Resource.new(params[:id], params[:details])
-    end
-  end
-end