From c012c235c6efb1628ed75c7fb3d7594ddd51f544 Mon Sep 17 00:00:00 2001
From: thiggy1342 <thiggy1342@github.com>
Date: Tue, 14 Jun 2022 01:06:39 +0000
Subject: [PATCH 01/22] rough draft of check request verb query

---
 .../ManuallyCheckHttpVerb.ql                  | 78 +++++++++++++++++++
 1 file changed, 78 insertions(+)
 create mode 100644 ruby/ql/src/experimental/manually-check-http-verb/ManuallyCheckHttpVerb.ql

diff --git a/ruby/ql/src/experimental/manually-check-http-verb/ManuallyCheckHttpVerb.ql b/ruby/ql/src/experimental/manually-check-http-verb/ManuallyCheckHttpVerb.ql
new file mode 100644
index 000000000000..ab71ba45d461
--- /dev/null
+++ b/ruby/ql/src/experimental/manually-check-http-verb/ManuallyCheckHttpVerb.ql
@@ -0,0 +1,78 @@
+/**
+ * @name Manually checking http verb instead of using built in rails routes and protections
+ * @description 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. 
+ * @kind problem
+ * @problem.severity error
+ * @security-severity 7.0
+ * @precision medium
+ * @id rb/manually-checking-http-verb
+ * @tags security
+ */
+
+import ruby
+import codeql.ruby.DataFlow
+
+class ManuallyCheckHttpVerb extends DataFlow::CallNode {
+  ManuallyCheckHttpVerb() {
+    this instanceof CheckGetRequest or
+    this instanceof CheckPostRequest or
+    this instanceof CheckPatchRequest or
+    this instanceof CheckPostRequest or
+    this instanceof CheckDeleteRequest or
+    this instanceof CheckHeadRequest or
+    this.asExpr().getExpr() instanceof CheckRequestMethodFromEnv
+  }
+}
+
+class CheckRequestMethodFromEnv extends ComparisonOperation {
+  CheckRequestMethodFromEnv() {
+    this.getAnOperand() instanceof GetRequestMethodFromEnv
+  }
+}
+
+class GetRequestMethodFromEnv extends ElementReference {
+  GetRequestMethodFromEnv() {
+    this.getAChild+().toString() = "REQUEST_METHOD" // and
+    // this.getReceiver().toString() = "env"
+  }
+}
+
+class CheckGetRequest extends DataFlow::CallNode {
+  CheckGetRequest() {
+  this.getMethodName() = "get?"
+  }
+}
+
+class CheckPostRequest extends DataFlow::CallNode {
+  CheckPostRequest() {
+    this.getMethodName() = "post?"
+  }
+}
+
+class CheckPutRequest extends DataFlow::CallNode {
+  CheckPutRequest() {
+    this.getMethodName() = "put?"
+  }
+}
+
+class CheckPatchRequest extends DataFlow::CallNode {
+  CheckPatchRequest() {
+    this.getMethodName() = "patch?"
+  }
+}
+
+class CheckDeleteRequest extends DataFlow::CallNode {
+  CheckDeleteRequest() {
+    this.getMethodName() = "delete?"
+  }
+}
+
+class CheckHeadRequest extends DataFlow::CallNode {
+  CheckHeadRequest() {
+    this.getMethodName() = "head?"
+  }
+}
+
+from ManuallyCheckHttpVerb check
+where check.asExpr().getExpr().getAControlFlowNode().isCondition()
+select check, "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."
\ No newline at end of file

From 7bdec98e6f73764028038476dc8c2c14d51e99f5 Mon Sep 17 00:00:00 2001
From: thiggy1342 <thiggy1342@github.com>
Date: Tue, 14 Jun 2022 02:13:15 +0000
Subject: [PATCH 02/22] draft tests

---
 .../ExampleController.rb                      | 51 +++++++++++++++++++
 .../ManuallyCheckHttpVerb.qlref               |  1 +
 .../manually-check-http-verb/NotController.rb | 28 ++++++++++
 3 files changed, 80 insertions(+)
 create mode 100644 ruby/ql/test/query-tests/security/manually-check-http-verb/ExampleController.rb
 create mode 100644 ruby/ql/test/query-tests/security/manually-check-http-verb/ManuallyCheckHttpVerb.qlref
 create mode 100644 ruby/ql/test/query-tests/security/manually-check-http-verb/NotController.rb

diff --git a/ruby/ql/test/query-tests/security/manually-check-http-verb/ExampleController.rb b/ruby/ql/test/query-tests/security/manually-check-http-verb/ExampleController.rb
new file mode 100644
index 000000000000..a67d4e5306a3
--- /dev/null
+++ b/ruby/ql/test/query-tests/security/manually-check-http-verb/ExampleController.rb
@@ -0,0 +1,51 @@
+class ExampleController < ActionController::Base
+  # This function should have 6 vulnerable lines
+  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
+
+class ResourceController < ActionController::Base
+    # This method should have 1 vulnerable line
+    def resource_action
+      case env['REQUEST_METHOD']
+      when "GET"
+        Resource.find(params[:id])
+      when "POST"
+        Resource.new(params[:id], params[:details])
+      end
+    end
+end
+
+class SafeController < ActionController::Base
+  # this method should have no hits because controllers rely on conventional Rails routes
+  def index
+    Safe.find(params[:id])
+  end
+
+  def create
+    Safe.new(params[:id], params[:details])
+  end
+
+  def update
+    Safe.update(params[:id], params[:details])
+  end
+
+  def delete
+    s = Safe.find(params[:id])
+    s.delete
+  end
+end
\ No newline at end of file
diff --git a/ruby/ql/test/query-tests/security/manually-check-http-verb/ManuallyCheckHttpVerb.qlref b/ruby/ql/test/query-tests/security/manually-check-http-verb/ManuallyCheckHttpVerb.qlref
new file mode 100644
index 000000000000..463c21cd0f29
--- /dev/null
+++ b/ruby/ql/test/query-tests/security/manually-check-http-verb/ManuallyCheckHttpVerb.qlref
@@ -0,0 +1 @@
+experimental/manually-check-http-verb/ManuallyCheckHttpVerb.ql
\ No newline at end of file
diff --git a/ruby/ql/test/query-tests/security/manually-check-http-verb/NotController.rb b/ruby/ql/test/query-tests/security/manually-check-http-verb/NotController.rb
new file mode 100644
index 000000000000..81a73d72410c
--- /dev/null
+++ b/ruby/ql/test/query-tests/security/manually-check-http-verb/NotController.rb
@@ -0,0 +1,28 @@
+# 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
\ No newline at end of file

From 6bef71ea2ca8960082507da96b1967265fb3f943 Mon Sep 17 00:00:00 2001
From: thiggy1342 <thiggy1342@github.com>
Date: Tue, 14 Jun 2022 02:17:12 +0000
Subject: [PATCH 03/22] tweaks to tests

---
 .../manually-check-http-verb/ExampleController.rb      | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/ruby/ql/test/query-tests/security/manually-check-http-verb/ExampleController.rb b/ruby/ql/test/query-tests/security/manually-check-http-verb/ExampleController.rb
index a67d4e5306a3..c3e913367b83 100644
--- a/ruby/ql/test/query-tests/security/manually-check-http-verb/ExampleController.rb
+++ b/ruby/ql/test/query-tests/security/manually-check-http-verb/ExampleController.rb
@@ -18,8 +18,16 @@ def example_action
   end
 end
 
+class OtherController < ActionController::Base
+  def other_action
+    if env['REQUEST_METHOD'] == "GET"
+      Other.find(params[:id])
+    end
+  end
+end
+
 class ResourceController < ActionController::Base
-    # This method should have 1 vulnerable line
+    # This method should have 1 vulnerable line, but is currently failing because it's not a comparison node
     def resource_action
       case env['REQUEST_METHOD']
       when "GET"

From 8aa2602d9e24a0e2b3ebae1836630c818116f376 Mon Sep 17 00:00:00 2001
From: thiggy1342 <thiggy1342@github.com>
Date: Sat, 18 Jun 2022 03:09:04 +0000
Subject: [PATCH 04/22] trying to hone in on eq comparison and include?

---
 .../ManuallyCheckHttpVerb.ql                  | 56 ++++++++++---------
 1 file changed, 30 insertions(+), 26 deletions(-)

diff --git a/ruby/ql/src/experimental/manually-check-http-verb/ManuallyCheckHttpVerb.ql b/ruby/ql/src/experimental/manually-check-http-verb/ManuallyCheckHttpVerb.ql
index ab71ba45d461..ac34ad340885 100644
--- a/ruby/ql/src/experimental/manually-check-http-verb/ManuallyCheckHttpVerb.ql
+++ b/ruby/ql/src/experimental/manually-check-http-verb/ManuallyCheckHttpVerb.ql
@@ -1,6 +1,6 @@
 /**
  * @name Manually checking http verb instead of using built in rails routes and protections
- * @description 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. 
+ * @description 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.
  * @kind problem
  * @problem.severity error
  * @security-severity 7.0
@@ -20,59 +20,63 @@ class ManuallyCheckHttpVerb extends DataFlow::CallNode {
     this instanceof CheckPostRequest or
     this instanceof CheckDeleteRequest or
     this instanceof CheckHeadRequest or
-    this.asExpr().getExpr() instanceof CheckRequestMethodFromEnv
+    this instanceof CheckRequestMethodFromEnv
   }
 }
 
-class CheckRequestMethodFromEnv extends ComparisonOperation {
+class CheckRequestMethodFromEnv extends DataFlow::CallNode {
   CheckRequestMethodFromEnv() {
-    this.getAnOperand() instanceof GetRequestMethodFromEnv
+    // is this node an instance of `env["REQUEST_METHOD"]
+    this.getExprNode().getNode() instanceof GetRequestMethodFromEnv and
+    (
+      // and is this node a param of a call to `.include?`
+      exists(MethodCall call | call.getAnArgument() = this.getExprNode().getNode() |
+        call.getMethodName() = "include?"
+      )
+      or
+      exists(DataFlow::Node node |
+        node.asExpr().getExpr().(MethodCall).getMethodName() = "include?"
+      |
+        node.getALocalSource() = this
+      )
+      or
+      // or is this node on either size of an equality comparison
+      exists(EqualityOperation eq | eq.getAChild() = this.getExprNode().getNode())
+    )
   }
 }
 
 class GetRequestMethodFromEnv extends ElementReference {
   GetRequestMethodFromEnv() {
-    this.getAChild+().toString() = "REQUEST_METHOD" // and
-    // this.getReceiver().toString() = "env"
+    this.getAChild+().toString() = "REQUEST_METHOD" and
+    this.getAChild+().toString() = "env"
   }
 }
 
 class CheckGetRequest extends DataFlow::CallNode {
-  CheckGetRequest() {
-  this.getMethodName() = "get?"
-  }
+  CheckGetRequest() { this.getMethodName() = "get?" }
 }
 
 class CheckPostRequest extends DataFlow::CallNode {
-  CheckPostRequest() {
-    this.getMethodName() = "post?"
-  }
+  CheckPostRequest() { this.getMethodName() = "post?" }
 }
 
 class CheckPutRequest extends DataFlow::CallNode {
-  CheckPutRequest() {
-    this.getMethodName() = "put?"
-  }
+  CheckPutRequest() { this.getMethodName() = "put?" }
 }
 
 class CheckPatchRequest extends DataFlow::CallNode {
-  CheckPatchRequest() {
-    this.getMethodName() = "patch?"
-  }
+  CheckPatchRequest() { this.getMethodName() = "patch?" }
 }
 
 class CheckDeleteRequest extends DataFlow::CallNode {
-  CheckDeleteRequest() {
-    this.getMethodName() = "delete?"
-  }
+  CheckDeleteRequest() { this.getMethodName() = "delete?" }
 }
 
 class CheckHeadRequest extends DataFlow::CallNode {
-  CheckHeadRequest() {
-    this.getMethodName() = "head?"
-  }
+  CheckHeadRequest() { this.getMethodName() = "head?" }
 }
 
 from ManuallyCheckHttpVerb check
-where check.asExpr().getExpr().getAControlFlowNode().isCondition()
-select check, "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."
\ No newline at end of file
+select check,
+  "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."

From 059c4d38ad97b4972f0be1e4e878fbd32ad3b10c Mon Sep 17 00:00:00 2001
From: thiggy1342 <thiggy1342@github.com>
Date: Sat, 18 Jun 2022 18:26:45 +0000
Subject: [PATCH 05/22] refine query to use appropriate types

---
 .../ManuallyCheckHttpVerb.ql                  | 26 ++++++++++---------
 .../ManuallyCheckHttpVerb.expected            |  0
 2 files changed, 14 insertions(+), 12 deletions(-)
 create mode 100644 ruby/ql/test/query-tests/security/manually-check-http-verb/ManuallyCheckHttpVerb.expected

diff --git a/ruby/ql/src/experimental/manually-check-http-verb/ManuallyCheckHttpVerb.ql b/ruby/ql/src/experimental/manually-check-http-verb/ManuallyCheckHttpVerb.ql
index ac34ad340885..7396e75b9205 100644
--- a/ruby/ql/src/experimental/manually-check-http-verb/ManuallyCheckHttpVerb.ql
+++ b/ruby/ql/src/experimental/manually-check-http-verb/ManuallyCheckHttpVerb.ql
@@ -12,15 +12,14 @@
 import ruby
 import codeql.ruby.DataFlow
 
-class ManuallyCheckHttpVerb extends DataFlow::CallNode {
-  ManuallyCheckHttpVerb() {
+class HttpVerbMethod extends MethodCall {
+  HttpVerbMethod() {
     this instanceof CheckGetRequest or
     this instanceof CheckPostRequest or
     this instanceof CheckPatchRequest or
     this instanceof CheckPostRequest or
     this instanceof CheckDeleteRequest or
-    this instanceof CheckHeadRequest or
-    this instanceof CheckRequestMethodFromEnv
+    this instanceof CheckHeadRequest
   }
 }
 
@@ -53,30 +52,33 @@ class GetRequestMethodFromEnv extends ElementReference {
   }
 }
 
-class CheckGetRequest extends DataFlow::CallNode {
+class CheckGetRequest extends MethodCall {
   CheckGetRequest() { this.getMethodName() = "get?" }
 }
 
-class CheckPostRequest extends DataFlow::CallNode {
+class CheckPostRequest extends MethodCall {
   CheckPostRequest() { this.getMethodName() = "post?" }
 }
 
-class CheckPutRequest extends DataFlow::CallNode {
+class CheckPutRequest extends MethodCall {
   CheckPutRequest() { this.getMethodName() = "put?" }
 }
 
-class CheckPatchRequest extends DataFlow::CallNode {
+class CheckPatchRequest extends MethodCall {
   CheckPatchRequest() { this.getMethodName() = "patch?" }
 }
 
-class CheckDeleteRequest extends DataFlow::CallNode {
+class CheckDeleteRequest extends MethodCall {
   CheckDeleteRequest() { this.getMethodName() = "delete?" }
 }
 
-class CheckHeadRequest extends DataFlow::CallNode {
+class CheckHeadRequest extends MethodCall {
   CheckHeadRequest() { this.getMethodName() = "head?" }
 }
 
-from ManuallyCheckHttpVerb check
-select check,
+from CheckRequestMethodFromEnv env, AstNode node
+where
+  node instanceof HttpVerbMethod or
+  node = env.asExpr().getExpr()
+select node,
   "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."
diff --git a/ruby/ql/test/query-tests/security/manually-check-http-verb/ManuallyCheckHttpVerb.expected b/ruby/ql/test/query-tests/security/manually-check-http-verb/ManuallyCheckHttpVerb.expected
new file mode 100644
index 000000000000..e69de29bb2d1

From 8b3619102395283814944424ed142ee843a22a97 Mon Sep 17 00:00:00 2001
From: thiggy1342 <thiggy1342@github.com>
Date: Sat, 18 Jun 2022 18:38:58 +0000
Subject: [PATCH 06/22] drop precision to low for now

---
 .../manually-check-http-verb/ManuallyCheckHttpVerb.ql         | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/ruby/ql/src/experimental/manually-check-http-verb/ManuallyCheckHttpVerb.ql b/ruby/ql/src/experimental/manually-check-http-verb/ManuallyCheckHttpVerb.ql
index 7396e75b9205..5be00b484643 100644
--- a/ruby/ql/src/experimental/manually-check-http-verb/ManuallyCheckHttpVerb.ql
+++ b/ruby/ql/src/experimental/manually-check-http-verb/ManuallyCheckHttpVerb.ql
@@ -3,8 +3,8 @@
  * @description 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.
  * @kind problem
  * @problem.severity error
- * @security-severity 7.0
- * @precision medium
+ * @security-severity 5.0
+ * @precision low
  * @id rb/manually-checking-http-verb
  * @tags security
  */

From ecb2114b7bb709a67160334db0234172caed31e3 Mon Sep 17 00:00:00 2001
From: thiggy1342 <thiggy1342@github.com>
Date: Sat, 18 Jun 2022 19:21:17 +0000
Subject: [PATCH 07/22] replace duplicate post with put

---
 .../manually-check-http-verb/ManuallyCheckHttpVerb.ql           | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ruby/ql/src/experimental/manually-check-http-verb/ManuallyCheckHttpVerb.ql b/ruby/ql/src/experimental/manually-check-http-verb/ManuallyCheckHttpVerb.ql
index 5be00b484643..6946d26a7e8f 100644
--- a/ruby/ql/src/experimental/manually-check-http-verb/ManuallyCheckHttpVerb.ql
+++ b/ruby/ql/src/experimental/manually-check-http-verb/ManuallyCheckHttpVerb.ql
@@ -17,7 +17,7 @@ class HttpVerbMethod extends MethodCall {
     this instanceof CheckGetRequest or
     this instanceof CheckPostRequest or
     this instanceof CheckPatchRequest or
-    this instanceof CheckPostRequest or
+    this instanceof CheckPutRequest or
     this instanceof CheckDeleteRequest or
     this instanceof CheckHeadRequest
   }

From 990747cd229b83f467c018d50ebd0ffdea93607d Mon Sep 17 00:00:00 2001
From: thiggy1342 <thiggy1342@github.com>
Date: Tue, 21 Jun 2022 21:27:18 +0000
Subject: [PATCH 08/22] Limit findings to just those called in Controllers

---
 .../manually-check-http-verb/ManuallyCheckHttpVerb.ql | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/ruby/ql/src/experimental/manually-check-http-verb/ManuallyCheckHttpVerb.ql b/ruby/ql/src/experimental/manually-check-http-verb/ManuallyCheckHttpVerb.ql
index 6946d26a7e8f..98c2c266cab8 100644
--- a/ruby/ql/src/experimental/manually-check-http-verb/ManuallyCheckHttpVerb.ql
+++ b/ruby/ql/src/experimental/manually-check-http-verb/ManuallyCheckHttpVerb.ql
@@ -23,6 +23,10 @@ class HttpVerbMethod extends MethodCall {
   }
 }
 
+class ControllerClass extends ModuleBase {
+  ControllerClass() { this.getModule().getSuperClass+().toString() = "ApplicationController" }
+}
+
 class CheckRequestMethodFromEnv extends DataFlow::CallNode {
   CheckRequestMethodFromEnv() {
     // is this node an instance of `env["REQUEST_METHOD"]
@@ -78,7 +82,10 @@ class CheckHeadRequest extends MethodCall {
 
 from CheckRequestMethodFromEnv env, AstNode node
 where
-  node instanceof HttpVerbMethod or
-  node = env.asExpr().getExpr()
+  (
+    node instanceof HttpVerbMethod or
+    node = env.asExpr().getExpr()
+  ) and
+  node.getEnclosingModule() instanceof ControllerClass
 select node,
   "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."

From c767f241ad919d67b7a718207f669a457b464d53 Mon Sep 17 00:00:00 2001
From: thiggy1342 <thiggy1342@github.com>
Date: Wed, 22 Jun 2022 02:12:23 +0000
Subject: [PATCH 09/22] narrow query scope

---
 .../ManuallyCheckHttpVerb.ql                  | 65 +++++--------------
 1 file changed, 16 insertions(+), 49 deletions(-)

diff --git a/ruby/ql/src/experimental/manually-check-http-verb/ManuallyCheckHttpVerb.ql b/ruby/ql/src/experimental/manually-check-http-verb/ManuallyCheckHttpVerb.ql
index 98c2c266cab8..6e48255d4d63 100644
--- a/ruby/ql/src/experimental/manually-check-http-verb/ManuallyCheckHttpVerb.ql
+++ b/ruby/ql/src/experimental/manually-check-http-verb/ManuallyCheckHttpVerb.ql
@@ -12,39 +12,30 @@
 import ruby
 import codeql.ruby.DataFlow
 
-class HttpVerbMethod extends MethodCall {
-  HttpVerbMethod() {
-    this instanceof CheckGetRequest or
-    this instanceof CheckPostRequest or
-    this instanceof CheckPatchRequest or
-    this instanceof CheckPutRequest or
-    this instanceof CheckDeleteRequest or
-    this instanceof CheckHeadRequest
-  }
+class CheckNotGetRequest extends ConditionalExpr {
+  CheckNotGetRequest() { this.getCondition() instanceof CheckGetRequest }
+}
+
+class CheckGetRequest extends MethodCall {
+  CheckGetRequest() { this.getMethodName() = "get?" }
 }
 
 class ControllerClass extends ModuleBase {
   ControllerClass() { this.getModule().getSuperClass+().toString() = "ApplicationController" }
 }
 
-class CheckRequestMethodFromEnv extends DataFlow::CallNode {
-  CheckRequestMethodFromEnv() {
+class CheckGetFromEnv extends AstNode {
+  CheckGetFromEnv() {
     // is this node an instance of `env["REQUEST_METHOD"]
-    this.getExprNode().getNode() instanceof GetRequestMethodFromEnv and
+    this instanceof GetRequestMethodFromEnv and
     (
       // and is this node a param of a call to `.include?`
-      exists(MethodCall call | call.getAnArgument() = this.getExprNode().getNode() |
-        call.getMethodName() = "include?"
-      )
+      exists(MethodCall call | call.getAnArgument() = this | call.getMethodName() = "include?")
       or
-      exists(DataFlow::Node node |
-        node.asExpr().getExpr().(MethodCall).getMethodName() = "include?"
-      |
-        node.getALocalSource() = this
+      // check if env["REQUEST_METHOD"] is compared to GET
+      exists(EqualityOperation eq | eq.getAChild() = this |
+        eq.getAChild().(StringLiteral).toString() = "GET"
       )
-      or
-      // or is this node on either size of an equality comparison
-      exists(EqualityOperation eq | eq.getAChild() = this.getExprNode().getNode())
     )
   }
 }
@@ -56,35 +47,11 @@ class GetRequestMethodFromEnv extends ElementReference {
   }
 }
 
-class CheckGetRequest extends MethodCall {
-  CheckGetRequest() { this.getMethodName() = "get?" }
-}
-
-class CheckPostRequest extends MethodCall {
-  CheckPostRequest() { this.getMethodName() = "post?" }
-}
-
-class CheckPutRequest extends MethodCall {
-  CheckPutRequest() { this.getMethodName() = "put?" }
-}
-
-class CheckPatchRequest extends MethodCall {
-  CheckPatchRequest() { this.getMethodName() = "patch?" }
-}
-
-class CheckDeleteRequest extends MethodCall {
-  CheckDeleteRequest() { this.getMethodName() = "delete?" }
-}
-
-class CheckHeadRequest extends MethodCall {
-  CheckHeadRequest() { this.getMethodName() = "head?" }
-}
-
-from CheckRequestMethodFromEnv env, AstNode node
+from AstNode node
 where
   (
-    node instanceof HttpVerbMethod or
-    node = env.asExpr().getExpr()
+    node instanceof CheckNotGetRequest or
+    node instanceof CheckGetFromEnv
   ) and
   node.getEnclosingModule() instanceof ControllerClass
 select node,

From 995f36556847cb5997750f5d3d92600764b26d93 Mon Sep 17 00:00:00 2001
From: thiggy1342 <thiggy1342@github.com>
Date: Wed, 22 Jun 2022 02:17:01 +0000
Subject: [PATCH 10/22] just check string literal

---
 .../manually-check-http-verb/ManuallyCheckHttpVerb.ql | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/ruby/ql/src/experimental/manually-check-http-verb/ManuallyCheckHttpVerb.ql b/ruby/ql/src/experimental/manually-check-http-verb/ManuallyCheckHttpVerb.ql
index 6e48255d4d63..a006587a13e3 100644
--- a/ruby/ql/src/experimental/manually-check-http-verb/ManuallyCheckHttpVerb.ql
+++ b/ruby/ql/src/experimental/manually-check-http-verb/ManuallyCheckHttpVerb.ql
@@ -28,14 +28,9 @@ class CheckGetFromEnv extends AstNode {
   CheckGetFromEnv() {
     // is this node an instance of `env["REQUEST_METHOD"]
     this instanceof GetRequestMethodFromEnv and
-    (
-      // and is this node a param of a call to `.include?`
-      exists(MethodCall call | call.getAnArgument() = this | call.getMethodName() = "include?")
-      or
-      // check if env["REQUEST_METHOD"] is compared to GET
-      exists(EqualityOperation eq | eq.getAChild() = this |
-        eq.getAChild().(StringLiteral).toString() = "GET"
-      )
+    // check if env["REQUEST_METHOD"] is compared to GET
+    exists(EqualityOperation eq | eq.getAChild() = this |
+      eq.getAChild().(StringLiteral).toString() = "GET"
     )
   }
 }

From 6aab970a9e86ff0dd0aa1660c489d4d0bcba525c Mon Sep 17 00:00:00 2001
From: thiggy1342 <thiggy1342@github.com>
Date: Fri, 8 Jul 2022 18:32:54 +0000
Subject: [PATCH 11/22] refactor query to use cfg and dataflow

---
 .../ManuallyCheckHttpVerb.ql                  | 92 +++++++++++++------
 .../ExampleController.rb                      | 11 ---
 .../ManuallyCheckHttpVerb.expected            |  0
 .../ManuallyCheckHttpVerb.qlref               |  0
 .../manually-check-http-verb/NotController.rb | 17 ++++
 .../manually-check-http-verb/NotController.rb | 28 ------
 6 files changed, 83 insertions(+), 65 deletions(-)
 rename ruby/ql/test/query-tests/{security => experimental}/manually-check-http-verb/ExampleController.rb (70%)
 rename ruby/ql/test/query-tests/{security => experimental}/manually-check-http-verb/ManuallyCheckHttpVerb.expected (100%)
 rename ruby/ql/test/query-tests/{security => experimental}/manually-check-http-verb/ManuallyCheckHttpVerb.qlref (100%)
 create mode 100644 ruby/ql/test/query-tests/experimental/manually-check-http-verb/NotController.rb
 delete mode 100644 ruby/ql/test/query-tests/security/manually-check-http-verb/NotController.rb

diff --git a/ruby/ql/src/experimental/manually-check-http-verb/ManuallyCheckHttpVerb.ql b/ruby/ql/src/experimental/manually-check-http-verb/ManuallyCheckHttpVerb.ql
index a006587a13e3..5e1db6f0de76 100644
--- a/ruby/ql/src/experimental/manually-check-http-verb/ManuallyCheckHttpVerb.ql
+++ b/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())
   }
 }
 
-class GetRequestMethodFromEnv extends ElementReference {
-  GetRequestMethodFromEnv() {
-    this.getAChild+().toString() = "REQUEST_METHOD" and
-    this.getAChild+().toString() = "env"
+// `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)
+      )
+    )
   }
 }
 
-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."
diff --git a/ruby/ql/test/query-tests/security/manually-check-http-verb/ExampleController.rb b/ruby/ql/test/query-tests/experimental/manually-check-http-verb/ExampleController.rb
similarity index 70%
rename from ruby/ql/test/query-tests/security/manually-check-http-verb/ExampleController.rb
rename to ruby/ql/test/query-tests/experimental/manually-check-http-verb/ExampleController.rb
index c3e913367b83..7e4ab4a1a77e 100644
--- a/ruby/ql/test/query-tests/security/manually-check-http-verb/ExampleController.rb
+++ b/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
diff --git a/ruby/ql/test/query-tests/security/manually-check-http-verb/ManuallyCheckHttpVerb.expected b/ruby/ql/test/query-tests/experimental/manually-check-http-verb/ManuallyCheckHttpVerb.expected
similarity index 100%
rename from ruby/ql/test/query-tests/security/manually-check-http-verb/ManuallyCheckHttpVerb.expected
rename to ruby/ql/test/query-tests/experimental/manually-check-http-verb/ManuallyCheckHttpVerb.expected
diff --git a/ruby/ql/test/query-tests/security/manually-check-http-verb/ManuallyCheckHttpVerb.qlref b/ruby/ql/test/query-tests/experimental/manually-check-http-verb/ManuallyCheckHttpVerb.qlref
similarity index 100%
rename from ruby/ql/test/query-tests/security/manually-check-http-verb/ManuallyCheckHttpVerb.qlref
rename to ruby/ql/test/query-tests/experimental/manually-check-http-verb/ManuallyCheckHttpVerb.qlref
diff --git a/ruby/ql/test/query-tests/experimental/manually-check-http-verb/NotController.rb b/ruby/ql/test/query-tests/experimental/manually-check-http-verb/NotController.rb
new file mode 100644
index 000000000000..78e194245e28
--- /dev/null
+++ b/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
\ No newline at end of file
diff --git a/ruby/ql/test/query-tests/security/manually-check-http-verb/NotController.rb b/ruby/ql/test/query-tests/security/manually-check-http-verb/NotController.rb
deleted file mode 100644
index 81a73d72410c..000000000000
--- a/ruby/ql/test/query-tests/security/manually-check-http-verb/NotController.rb
+++ /dev/null
@@ -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
\ No newline at end of file

From b3f1a513d148eac778dfe66afc2c0254591e8b17 Mon Sep 17 00:00:00 2001
From: thiggy1342 <thiggy1342@github.com>
Date: Wed, 13 Jul 2022 00:25:19 +0000
Subject: [PATCH 12/22] Update tests

---
 .../ExampleController.rb                      | 48 ----------
 .../ManuallyCheckHttpVerb.rb                  | 96 +++++++++++++++++++
 .../manually-check-http-verb/NotController.rb | 17 ----
 3 files changed, 96 insertions(+), 65 deletions(-)
 delete mode 100644 ruby/ql/test/query-tests/experimental/manually-check-http-verb/ExampleController.rb
 create mode 100644 ruby/ql/test/query-tests/experimental/manually-check-http-verb/ManuallyCheckHttpVerb.rb
 delete mode 100644 ruby/ql/test/query-tests/experimental/manually-check-http-verb/NotController.rb

diff --git a/ruby/ql/test/query-tests/experimental/manually-check-http-verb/ExampleController.rb b/ruby/ql/test/query-tests/experimental/manually-check-http-verb/ExampleController.rb
deleted file mode 100644
index 7e4ab4a1a77e..000000000000
--- a/ruby/ql/test/query-tests/experimental/manually-check-http-verb/ExampleController.rb
+++ /dev/null
@@ -1,48 +0,0 @@
-class ExampleController < ActionController::Base
-  # This function should have 6 vulnerable lines
-  def example_action
-    if request.get?
-      Example.find(params[:example_id])
-    end
-  end
-end
-
-class OtherController < ActionController::Base
-  def other_action
-    if env['REQUEST_METHOD'] == "GET"
-      Other.find(params[:id])
-    end
-  end
-end
-
-class ResourceController < ActionController::Base
-    # This method should have 1 vulnerable line, but is currently failing because it's not a comparison node
-    def resource_action
-      case env['REQUEST_METHOD']
-      when "GET"
-        Resource.find(params[:id])
-      when "POST"
-        Resource.new(params[:id], params[:details])
-      end
-    end
-end
-
-class SafeController < ActionController::Base
-  # this method should have no hits because controllers rely on conventional Rails routes
-  def index
-    Safe.find(params[:id])
-  end
-
-  def create
-    Safe.new(params[:id], params[:details])
-  end
-
-  def update
-    Safe.update(params[:id], params[:details])
-  end
-
-  def delete
-    s = Safe.find(params[:id])
-    s.delete
-  end
-end
\ No newline at end of file
diff --git a/ruby/ql/test/query-tests/experimental/manually-check-http-verb/ManuallyCheckHttpVerb.rb b/ruby/ql/test/query-tests/experimental/manually-check-http-verb/ManuallyCheckHttpVerb.rb
new file mode 100644
index 000000000000..aa10ebc69aa6
--- /dev/null
+++ b/ruby/ql/test/query-tests/experimental/manually-check-http-verb/ManuallyCheckHttpVerb.rb
@@ -0,0 +1,96 @@
+class ExampleController < ActionController::Base
+  # Should find
+  def example_action
+    if request.get?
+      Resource.find(id: params[:example_id])
+    end
+  end
+
+  # Should find
+  def other_action
+    if request.env['REQUEST_METHOD'] == "GET"
+      Resource.find(id: params[:id])
+    end
+  end
+
+  # Should find
+  def foo
+    if request.request_method == "GET"
+      Resource.find(id: params[:id])
+    end
+  end
+
+  # Should find
+  def bar
+    if request.method == "GET"
+      Resource.find(id: params[:id])
+    end
+  end
+
+  # Should find
+  def baz
+    if request.raw_request_method == "GET"
+      Resource.find(id: params[:id])
+    end
+  end
+
+    # Should find
+    def foobarbaz
+      if request.request_method_symbol == :GET
+        Resource.find(id: params[:id])
+      end
+    end
+
+  # Should find
+  def resource_action
+    case request.env['REQUEST_METHOD']
+    when "GET"
+      Resource.find(id: params[:id])
+    when "POST"
+      Resource.new(id: params[:id], details: params[:details])
+    end
+  end
+
+
+end
+
+class SafeController < ActionController::Base
+  # this class should have no hits because controllers rely on conventional Rails routes
+  def index
+    Resource.find(id: params[:id])
+  end
+
+  def create
+    Resource.new(id: params[:id], details: params[:details])
+  end
+
+  def update
+    Resource.update(id: params[:id], details: params[:details])
+  end
+
+  def delete
+    s = Resource.find(id: params[:id])
+    s.delete
+  end
+end
+
+# There should be no hits from this class because it does not inherit from ActionController
+class NotAController
+  def example_action
+    if request.get?
+      Resource.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
+
+class Resource < ActiveRecord::Base
+end
\ No newline at end of file
diff --git a/ruby/ql/test/query-tests/experimental/manually-check-http-verb/NotController.rb b/ruby/ql/test/query-tests/experimental/manually-check-http-verb/NotController.rb
deleted file mode 100644
index 78e194245e28..000000000000
--- a/ruby/ql/test/query-tests/experimental/manually-check-http-verb/NotController.rb
+++ /dev/null
@@ -1,17 +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])
-    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
\ No newline at end of file

From 712900257342ffbfbb788c6491e955c7f10c3dc1 Mon Sep 17 00:00:00 2001
From: thiggy1342 <thiggy1342@github.com>
Date: Wed, 13 Jul 2022 00:33:58 +0000
Subject: [PATCH 13/22] tweak tests more

---
 .../ManuallyCheckHttpVerb.ql                      | 10 +++++++++-
 .../ManuallyCheckHttpVerb.rb                      | 15 ++++++++++-----
 2 files changed, 19 insertions(+), 6 deletions(-)

diff --git a/ruby/ql/src/experimental/manually-check-http-verb/ManuallyCheckHttpVerb.ql b/ruby/ql/src/experimental/manually-check-http-verb/ManuallyCheckHttpVerb.ql
index 5e1db6f0de76..72574e148d22 100644
--- a/ruby/ql/src/experimental/manually-check-http-verb/ManuallyCheckHttpVerb.ql
+++ b/ruby/ql/src/experimental/manually-check-http-verb/ManuallyCheckHttpVerb.ql
@@ -18,7 +18,15 @@ import codeql.ruby.frameworks.ActionController
 class Request extends DataFlow::CallNode {
   Request() {
     this.getMethodName() = "request" and
-    this.asExpr().getExpr() instanceof ActionControllerActionMethod
+    this.asExpr().getExpr().getEnclosingMethod() instanceof ActionControllerActionMethod
+  }
+}
+
+// `request.env`
+class RequestEnvMethod extends DataFlow::CallNode {
+  RequestEnvMethod() {
+    this.getMethodName() = "env" and
+    any(Request r).flowsTo(this.getReceiver())
   }
 }
 
diff --git a/ruby/ql/test/query-tests/experimental/manually-check-http-verb/ManuallyCheckHttpVerb.rb b/ruby/ql/test/query-tests/experimental/manually-check-http-verb/ManuallyCheckHttpVerb.rb
index aa10ebc69aa6..ad0f5b45566b 100644
--- a/ruby/ql/test/query-tests/experimental/manually-check-http-verb/ManuallyCheckHttpVerb.rb
+++ b/ruby/ql/test/query-tests/experimental/manually-check-http-verb/ManuallyCheckHttpVerb.rb
@@ -8,35 +8,40 @@ def example_action
 
   # Should find
   def other_action
-    if request.env['REQUEST_METHOD'] == "GET"
+    method = request.env['REQUEST_METHOD']
+    if method == "GET"
       Resource.find(id: params[:id])
     end
   end
 
   # Should find
   def foo
-    if request.request_method == "GET"
+    method = request.request_method
+    if method == "GET"
       Resource.find(id: params[:id])
     end
   end
 
   # Should find
   def bar
-    if request.method == "GET"
+    method = request.method
+    if method == "GET"
       Resource.find(id: params[:id])
     end
   end
 
   # Should find
   def baz
-    if request.raw_request_method == "GET"
+    method = request.raw_request_method
+    if method == "GET"
       Resource.find(id: params[:id])
     end
   end
 
     # Should find
     def foobarbaz
-      if request.request_method_symbol == :GET
+      method = request.request_method_symbol
+      if method == :GET
         Resource.find(id: params[:id])
       end
     end

From 2cc703387b0c42a7c260cc5e400d08e9a7c08ce2 Mon Sep 17 00:00:00 2001
From: thiggy1342 <thiggy1342@github.com>
Date: Thu, 14 Jul 2022 00:05:19 +0000
Subject: [PATCH 14/22] use taint config for data flow

---
 .../ManuallyCheckHttpVerb.ql                  | 48 ++++++++-----------
 .../ManuallyCheckHttpVerb.expected            | 32 +++++++++++++
 .../ManuallyCheckHttpVerb.rb                  | 18 ++++++-
 3 files changed, 70 insertions(+), 28 deletions(-)

diff --git a/ruby/ql/src/experimental/manually-check-http-verb/ManuallyCheckHttpVerb.ql b/ruby/ql/src/experimental/manually-check-http-verb/ManuallyCheckHttpVerb.ql
index 72574e148d22..3de3a7a03d5a 100644
--- a/ruby/ql/src/experimental/manually-check-http-verb/ManuallyCheckHttpVerb.ql
+++ b/ruby/ql/src/experimental/manually-check-http-verb/ManuallyCheckHttpVerb.ql
@@ -1,7 +1,7 @@
 /**
  * @name Manually checking http verb instead of using built in rails routes and protections
  * @description 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.
- * @kind problem
+ * @kind path-problem
  * @problem.severity error
  * @security-severity 5.0
  * @precision low
@@ -13,6 +13,8 @@ import ruby
 import codeql.ruby.DataFlow
 import codeql.ruby.controlflow.CfgNodes
 import codeql.ruby.frameworks.ActionController
+import codeql.ruby.TaintTracking
+import DataFlow::PathGraph
 
 // any `request` calls in an action method
 class Request extends DataFlow::CallNode {
@@ -70,32 +72,24 @@ class RequestGet extends DataFlow::CallNode {
   }
 }
 
-// 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 HttpVerbConfig extends TaintTracking::Configuration {
+  HttpVerbConfig() { this = "HttpVerbConfig" }
+    
+  override predicate isSource(DataFlow::Node source) {
+      source instanceof RequestMethod or
+      source instanceof RequestRequestMethod or
+      source instanceof RequestEnvMethod or
+      source instanceof RequestRawRequestMethod or
+      source instanceof RequestRequestMethodSymbol or
+      source instanceof RequestGet
+  }
+
+  override predicate isSink(DataFlow::Node sink) {
+    exists(ExprNodes::ConditionalExprCfgNode c | c.getCondition() = sink.asExpr()) or
+    exists(ExprNodes::CaseExprCfgNode c | c.getValue() = sink.asExpr())
   }
 }
 
-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."
+from HttpVerbConfig config, DataFlow::Node source, DataFlow::Node sink
+where config.hasFlow(source, sink)
+select sink.asExpr().getExpr(), source, sink, "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."
diff --git a/ruby/ql/test/query-tests/experimental/manually-check-http-verb/ManuallyCheckHttpVerb.expected b/ruby/ql/test/query-tests/experimental/manually-check-http-verb/ManuallyCheckHttpVerb.expected
index e69de29bb2d1..9102005a67e8 100644
--- a/ruby/ql/test/query-tests/experimental/manually-check-http-verb/ManuallyCheckHttpVerb.expected
+++ b/ruby/ql/test/query-tests/experimental/manually-check-http-verb/ManuallyCheckHttpVerb.expected
@@ -0,0 +1,32 @@
+edges
+| ManuallyCheckHttpVerb.rb:11:14:11:24 | call to env :  | ManuallyCheckHttpVerb.rb:11:14:11:42 | ...[...] :  |
+| ManuallyCheckHttpVerb.rb:11:14:11:42 | ...[...] :  | ManuallyCheckHttpVerb.rb:12:8:12:22 | ... == ... |
+| ManuallyCheckHttpVerb.rb:19:14:19:35 | call to request_method :  | ManuallyCheckHttpVerb.rb:20:8:20:22 | ... == ... |
+| ManuallyCheckHttpVerb.rb:27:14:27:27 | call to method :  | ManuallyCheckHttpVerb.rb:28:8:28:22 | ... == ... |
+| ManuallyCheckHttpVerb.rb:35:14:35:39 | call to raw_request_method :  | ManuallyCheckHttpVerb.rb:36:8:36:22 | ... == ... |
+| ManuallyCheckHttpVerb.rb:51:16:51:44 | call to request_method_symbol :  | ManuallyCheckHttpVerb.rb:52:10:52:23 | ... == ... |
+| ManuallyCheckHttpVerb.rb:59:10:59:20 | call to env :  | ManuallyCheckHttpVerb.rb:59:10:59:38 | ...[...] |
+nodes
+| ManuallyCheckHttpVerb.rb:4:8:4:19 | call to get? | semmle.label | call to get? |
+| ManuallyCheckHttpVerb.rb:11:14:11:24 | call to env :  | semmle.label | call to env :  |
+| ManuallyCheckHttpVerb.rb:11:14:11:42 | ...[...] :  | semmle.label | ...[...] :  |
+| ManuallyCheckHttpVerb.rb:12:8:12:22 | ... == ... | semmle.label | ... == ... |
+| ManuallyCheckHttpVerb.rb:19:14:19:35 | call to request_method :  | semmle.label | call to request_method :  |
+| ManuallyCheckHttpVerb.rb:20:8:20:22 | ... == ... | semmle.label | ... == ... |
+| ManuallyCheckHttpVerb.rb:27:14:27:27 | call to method :  | semmle.label | call to method :  |
+| ManuallyCheckHttpVerb.rb:28:8:28:22 | ... == ... | semmle.label | ... == ... |
+| ManuallyCheckHttpVerb.rb:35:14:35:39 | call to raw_request_method :  | semmle.label | call to raw_request_method :  |
+| ManuallyCheckHttpVerb.rb:36:8:36:22 | ... == ... | semmle.label | ... == ... |
+| ManuallyCheckHttpVerb.rb:51:16:51:44 | call to request_method_symbol :  | semmle.label | call to request_method_symbol :  |
+| ManuallyCheckHttpVerb.rb:52:10:52:23 | ... == ... | semmle.label | ... == ... |
+| ManuallyCheckHttpVerb.rb:59:10:59:20 | call to env :  | semmle.label | call to env :  |
+| ManuallyCheckHttpVerb.rb:59:10:59:38 | ...[...] | semmle.label | ...[...] |
+subpaths
+#select
+| ManuallyCheckHttpVerb.rb:4:8:4:19 | call to get? | ManuallyCheckHttpVerb.rb:4:8:4:19 | call to get? | ManuallyCheckHttpVerb.rb:4:8:4:19 | call to get? | 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. |
+| ManuallyCheckHttpVerb.rb:12:8:12:22 | ... == ... | ManuallyCheckHttpVerb.rb:11:14:11:24 | call to env | ManuallyCheckHttpVerb.rb:12:8:12:22 | ... == ... | 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. |
+| ManuallyCheckHttpVerb.rb:20:8:20:22 | ... == ... | ManuallyCheckHttpVerb.rb:19:14:19:35 | call to request_method | ManuallyCheckHttpVerb.rb:20:8:20:22 | ... == ... | 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. |
+| ManuallyCheckHttpVerb.rb:28:8:28:22 | ... == ... | ManuallyCheckHttpVerb.rb:27:14:27:27 | call to method | ManuallyCheckHttpVerb.rb:28:8:28:22 | ... == ... | 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. |
+| ManuallyCheckHttpVerb.rb:36:8:36:22 | ... == ... | ManuallyCheckHttpVerb.rb:35:14:35:39 | call to raw_request_method | ManuallyCheckHttpVerb.rb:36:8:36:22 | ... == ... | 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. |
+| ManuallyCheckHttpVerb.rb:52:10:52:23 | ... == ... | ManuallyCheckHttpVerb.rb:51:16:51:44 | call to request_method_symbol | ManuallyCheckHttpVerb.rb:52:10:52:23 | ... == ... | 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. |
+| ManuallyCheckHttpVerb.rb:59:10:59:38 | ...[...] | ManuallyCheckHttpVerb.rb:59:10:59:20 | call to env | ManuallyCheckHttpVerb.rb:59:10:59:38 | ...[...] | 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. |
diff --git a/ruby/ql/test/query-tests/experimental/manually-check-http-verb/ManuallyCheckHttpVerb.rb b/ruby/ql/test/query-tests/experimental/manually-check-http-verb/ManuallyCheckHttpVerb.rb
index ad0f5b45566b..055e9d986382 100644
--- a/ruby/ql/test/query-tests/experimental/manually-check-http-verb/ManuallyCheckHttpVerb.rb
+++ b/ruby/ql/test/query-tests/experimental/manually-check-http-verb/ManuallyCheckHttpVerb.rb
@@ -38,6 +38,14 @@ def baz
     end
   end
 
+  # Should not find
+  def baz2
+    method = request.raw_request_method
+    if some_other_function == "GET"
+      Resource.find(id: params[:id])
+    end
+  end
+
     # Should find
     def foobarbaz
       method = request.request_method_symbol
@@ -56,7 +64,15 @@ def resource_action
     end
   end
 
-
+  # Should not find
+  def resource_action
+    case request.random_method
+    when "GET"
+      Resource.find(id: params[:id])
+    when "POST"
+      Resource.new(id: params[:id], details: params[:details])
+    end
+  end
 end
 
 class SafeController < ActionController::Base

From ae634367c99f404fb3f2a92944243ccef9d50b95 Mon Sep 17 00:00:00 2001
From: thiggy1342 <thiggy1342@github.com>
Date: Thu, 14 Jul 2022 00:11:25 +0000
Subject: [PATCH 15/22] add qhelp file

---
 .../ManuallyCheckHttpVerb.qhelp               | 23 +++++++++++++++++++
 1 file changed, 23 insertions(+)
 create mode 100644 ruby/ql/src/experimental/manually-check-http-verb/ManuallyCheckHttpVerb.qhelp

diff --git a/ruby/ql/src/experimental/manually-check-http-verb/ManuallyCheckHttpVerb.qhelp b/ruby/ql/src/experimental/manually-check-http-verb/ManuallyCheckHttpVerb.qhelp
new file mode 100644
index 000000000000..5d7378ef0037
--- /dev/null
+++ b/ruby/ql/src/experimental/manually-check-http-verb/ManuallyCheckHttpVerb.qhelp
@@ -0,0 +1,23 @@
+<!DOCTYPE qhelp PUBLIC
+"-//Semmle//qhelp//EN"
+"qhelp.dtd">
+<qhelp>
+  <overview>
+    <p>
+      Manually checking the HTTP request verb inside of a controller method can lead to
+      CSRF bypass if GET or HEAD requests are handled improperly.
+    </p>
+  </overview>
+  <recommendation>
+    <p>
+      It is better to use different controller methods for each resource/http verb combination
+      and configure the Rails routes in your application to call them accordingly.
+    </p>
+  </recommendation>
+  
+  <references>
+    <p>
+      See https://guides.rubyonrails.org/routing.html for more information.
+    </p>
+  </references>
+</qhelp>

From ee79834cc80671d5e4535969e600346e4ea6708e Mon Sep 17 00:00:00 2001
From: thiggy1342 <thiggy1342@github.com>
Date: Thu, 14 Jul 2022 00:15:39 +0000
Subject: [PATCH 16/22] formatting in qhelp

---
 .../manually-check-http-verb/ManuallyCheckHttpVerb.qhelp      | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/ruby/ql/src/experimental/manually-check-http-verb/ManuallyCheckHttpVerb.qhelp b/ruby/ql/src/experimental/manually-check-http-verb/ManuallyCheckHttpVerb.qhelp
index 5d7378ef0037..d50c7f0bf30f 100644
--- a/ruby/ql/src/experimental/manually-check-http-verb/ManuallyCheckHttpVerb.qhelp
+++ b/ruby/ql/src/experimental/manually-check-http-verb/ManuallyCheckHttpVerb.qhelp
@@ -16,8 +16,8 @@
   </recommendation>
   
   <references>
-    <p>
+    <li>
       See https://guides.rubyonrails.org/routing.html for more information.
-    </p>
+    </li>
   </references>
 </qhelp>

From 3dd61cadf44297f672a9e550b289781abf325839 Mon Sep 17 00:00:00 2001
From: thiggy1342 <thiggy1342@github.com>
Date: Thu, 14 Jul 2022 00:19:36 +0000
Subject: [PATCH 17/22] formatting query

---
 .../ManuallyCheckHttpVerb.ql                    | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/ruby/ql/src/experimental/manually-check-http-verb/ManuallyCheckHttpVerb.ql b/ruby/ql/src/experimental/manually-check-http-verb/ManuallyCheckHttpVerb.ql
index 3de3a7a03d5a..e7553b39a3dd 100644
--- a/ruby/ql/src/experimental/manually-check-http-verb/ManuallyCheckHttpVerb.ql
+++ b/ruby/ql/src/experimental/manually-check-http-verb/ManuallyCheckHttpVerb.ql
@@ -74,14 +74,14 @@ class RequestGet extends DataFlow::CallNode {
 
 class HttpVerbConfig extends TaintTracking::Configuration {
   HttpVerbConfig() { this = "HttpVerbConfig" }
-    
+
   override predicate isSource(DataFlow::Node source) {
-      source instanceof RequestMethod or
-      source instanceof RequestRequestMethod or
-      source instanceof RequestEnvMethod or
-      source instanceof RequestRawRequestMethod or
-      source instanceof RequestRequestMethodSymbol or
-      source instanceof RequestGet
+    source instanceof RequestMethod or
+    source instanceof RequestRequestMethod or
+    source instanceof RequestEnvMethod or
+    source instanceof RequestRawRequestMethod or
+    source instanceof RequestRequestMethodSymbol or
+    source instanceof RequestGet
   }
 
   override predicate isSink(DataFlow::Node sink) {
@@ -92,4 +92,5 @@ class HttpVerbConfig extends TaintTracking::Configuration {
 
 from HttpVerbConfig config, DataFlow::Node source, DataFlow::Node sink
 where config.hasFlow(source, sink)
-select sink.asExpr().getExpr(), source, sink, "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."
+select sink.asExpr().getExpr(), source, sink,
+  "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."

From 304203ad2f2cfeaafcc1dab46324937fb7415685 Mon Sep 17 00:00:00 2001
From: thiggy1342 <thiggy1342@github.com>
Date: Tue, 19 Jul 2022 00:25:27 +0000
Subject: [PATCH 18/22] fix path problem output

---
 .../ManuallyCheckHttpVerb.ql                  |  6 +++---
 .../ManuallyCheckHttpVerb.expected            | 21 +++++++++++++------
 2 files changed, 18 insertions(+), 9 deletions(-)

diff --git a/ruby/ql/src/experimental/manually-check-http-verb/ManuallyCheckHttpVerb.ql b/ruby/ql/src/experimental/manually-check-http-verb/ManuallyCheckHttpVerb.ql
index e7553b39a3dd..354ccf62698f 100644
--- a/ruby/ql/src/experimental/manually-check-http-verb/ManuallyCheckHttpVerb.ql
+++ b/ruby/ql/src/experimental/manually-check-http-verb/ManuallyCheckHttpVerb.ql
@@ -90,7 +90,7 @@ class HttpVerbConfig extends TaintTracking::Configuration {
   }
 }
 
-from HttpVerbConfig config, DataFlow::Node source, DataFlow::Node sink
-where config.hasFlow(source, sink)
-select sink.asExpr().getExpr(), source, sink,
+from HttpVerbConfig config, DataFlow::PathNode source, DataFlow::PathNode sink
+where config.hasFlow(source.getNode(), sink.getNode())
+select sink.getNode(), source, sink,
   "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."
diff --git a/ruby/ql/test/query-tests/experimental/manually-check-http-verb/ManuallyCheckHttpVerb.expected b/ruby/ql/test/query-tests/experimental/manually-check-http-verb/ManuallyCheckHttpVerb.expected
index 9102005a67e8..a253c3e9313e 100644
--- a/ruby/ql/test/query-tests/experimental/manually-check-http-verb/ManuallyCheckHttpVerb.expected
+++ b/ruby/ql/test/query-tests/experimental/manually-check-http-verb/ManuallyCheckHttpVerb.expected
@@ -24,9 +24,18 @@ nodes
 subpaths
 #select
 | ManuallyCheckHttpVerb.rb:4:8:4:19 | call to get? | ManuallyCheckHttpVerb.rb:4:8:4:19 | call to get? | ManuallyCheckHttpVerb.rb:4:8:4:19 | call to get? | 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. |
-| ManuallyCheckHttpVerb.rb:12:8:12:22 | ... == ... | ManuallyCheckHttpVerb.rb:11:14:11:24 | call to env | ManuallyCheckHttpVerb.rb:12:8:12:22 | ... == ... | 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. |
-| ManuallyCheckHttpVerb.rb:20:8:20:22 | ... == ... | ManuallyCheckHttpVerb.rb:19:14:19:35 | call to request_method | ManuallyCheckHttpVerb.rb:20:8:20:22 | ... == ... | 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. |
-| ManuallyCheckHttpVerb.rb:28:8:28:22 | ... == ... | ManuallyCheckHttpVerb.rb:27:14:27:27 | call to method | ManuallyCheckHttpVerb.rb:28:8:28:22 | ... == ... | 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. |
-| ManuallyCheckHttpVerb.rb:36:8:36:22 | ... == ... | ManuallyCheckHttpVerb.rb:35:14:35:39 | call to raw_request_method | ManuallyCheckHttpVerb.rb:36:8:36:22 | ... == ... | 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. |
-| ManuallyCheckHttpVerb.rb:52:10:52:23 | ... == ... | ManuallyCheckHttpVerb.rb:51:16:51:44 | call to request_method_symbol | ManuallyCheckHttpVerb.rb:52:10:52:23 | ... == ... | 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. |
-| ManuallyCheckHttpVerb.rb:59:10:59:38 | ...[...] | ManuallyCheckHttpVerb.rb:59:10:59:20 | call to env | ManuallyCheckHttpVerb.rb:59:10:59:38 | ...[...] | 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. |
+| ManuallyCheckHttpVerb.rb:4:8:4:19 | call to get? | ManuallyCheckHttpVerb.rb:4:8:4:19 | call to get? | ManuallyCheckHttpVerb.rb:4:8:4:19 | call to get? :  | 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. |
+| ManuallyCheckHttpVerb.rb:4:8:4:19 | call to get? | ManuallyCheckHttpVerb.rb:4:8:4:19 | call to get? :  | ManuallyCheckHttpVerb.rb:4:8:4:19 | call to get? | 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. |
+| ManuallyCheckHttpVerb.rb:4:8:4:19 | call to get? | ManuallyCheckHttpVerb.rb:4:8:4:19 | call to get? :  | ManuallyCheckHttpVerb.rb:4:8:4:19 | call to get? :  | 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. |
+| ManuallyCheckHttpVerb.rb:12:8:12:22 | ... == ... | ManuallyCheckHttpVerb.rb:11:14:11:24 | call to env :  | ManuallyCheckHttpVerb.rb:12:8:12:22 | ... == ... | 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. |
+| ManuallyCheckHttpVerb.rb:12:8:12:22 | ... == ... | ManuallyCheckHttpVerb.rb:11:14:11:24 | call to env :  | ManuallyCheckHttpVerb.rb:12:8:12:22 | ... == ... :  | 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. |
+| ManuallyCheckHttpVerb.rb:20:8:20:22 | ... == ... | ManuallyCheckHttpVerb.rb:19:14:19:35 | call to request_method :  | ManuallyCheckHttpVerb.rb:20:8:20:22 | ... == ... | 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. |
+| ManuallyCheckHttpVerb.rb:20:8:20:22 | ... == ... | ManuallyCheckHttpVerb.rb:19:14:19:35 | call to request_method :  | ManuallyCheckHttpVerb.rb:20:8:20:22 | ... == ... :  | 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. |
+| ManuallyCheckHttpVerb.rb:28:8:28:22 | ... == ... | ManuallyCheckHttpVerb.rb:27:14:27:27 | call to method :  | ManuallyCheckHttpVerb.rb:28:8:28:22 | ... == ... | 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. |
+| ManuallyCheckHttpVerb.rb:28:8:28:22 | ... == ... | ManuallyCheckHttpVerb.rb:27:14:27:27 | call to method :  | ManuallyCheckHttpVerb.rb:28:8:28:22 | ... == ... :  | 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. |
+| ManuallyCheckHttpVerb.rb:36:8:36:22 | ... == ... | ManuallyCheckHttpVerb.rb:35:14:35:39 | call to raw_request_method :  | ManuallyCheckHttpVerb.rb:36:8:36:22 | ... == ... | 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. |
+| ManuallyCheckHttpVerb.rb:36:8:36:22 | ... == ... | ManuallyCheckHttpVerb.rb:35:14:35:39 | call to raw_request_method :  | ManuallyCheckHttpVerb.rb:36:8:36:22 | ... == ... :  | 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. |
+| ManuallyCheckHttpVerb.rb:52:10:52:23 | ... == ... | ManuallyCheckHttpVerb.rb:51:16:51:44 | call to request_method_symbol :  | ManuallyCheckHttpVerb.rb:52:10:52:23 | ... == ... | 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. |
+| ManuallyCheckHttpVerb.rb:52:10:52:23 | ... == ... | ManuallyCheckHttpVerb.rb:51:16:51:44 | call to request_method_symbol :  | ManuallyCheckHttpVerb.rb:52:10:52:23 | ... == ... :  | 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. |
+| ManuallyCheckHttpVerb.rb:59:10:59:38 | ...[...] | ManuallyCheckHttpVerb.rb:59:10:59:20 | call to env :  | ManuallyCheckHttpVerb.rb:59:10:59:38 | ...[...] | 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. |
+| ManuallyCheckHttpVerb.rb:59:10:59:38 | ...[...] | ManuallyCheckHttpVerb.rb:59:10:59:20 | call to env :  | ManuallyCheckHttpVerb.rb:59:10:59:38 | ...[...] :  | 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. |

From cc958dc1711ef9b3943bb8272f0b18334ea4ca64 Mon Sep 17 00:00:00 2001
From: thiggy1342 <thiggy1342@github.com>
Date: Thu, 21 Jul 2022 17:19:33 -0400
Subject: [PATCH 19/22] Update
 ruby/ql/src/experimental/manually-check-http-verb/ManuallyCheckHttpVerb.ql

Co-authored-by: Harry Maclean <hmac@github.com>
---
 .../manually-check-http-verb/ManuallyCheckHttpVerb.ql           | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ruby/ql/src/experimental/manually-check-http-verb/ManuallyCheckHttpVerb.ql b/ruby/ql/src/experimental/manually-check-http-verb/ManuallyCheckHttpVerb.ql
index 354ccf62698f..2ddf7fe87b3c 100644
--- a/ruby/ql/src/experimental/manually-check-http-verb/ManuallyCheckHttpVerb.ql
+++ b/ruby/ql/src/experimental/manually-check-http-verb/ManuallyCheckHttpVerb.ql
@@ -91,6 +91,6 @@ class HttpVerbConfig extends TaintTracking::Configuration {
 }
 
 from HttpVerbConfig config, DataFlow::PathNode source, DataFlow::PathNode sink
-where config.hasFlow(source.getNode(), sink.getNode())
+where config.hasFlowPath(source, sink)
 select sink.getNode(), source, sink,
   "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."

From 8fabc06d370e88603e809d35e76bd3c13bed8495 Mon Sep 17 00:00:00 2001
From: thiggy1342 <thiggy1342@github.com>
Date: Thu, 21 Jul 2022 21:25:44 +0000
Subject: [PATCH 20/22] fix test assertion

---
 .../ManuallyCheckHttpVerb.expected                       | 9 ---------
 1 file changed, 9 deletions(-)

diff --git a/ruby/ql/test/query-tests/experimental/manually-check-http-verb/ManuallyCheckHttpVerb.expected b/ruby/ql/test/query-tests/experimental/manually-check-http-verb/ManuallyCheckHttpVerb.expected
index a253c3e9313e..1fdf8045c232 100644
--- a/ruby/ql/test/query-tests/experimental/manually-check-http-verb/ManuallyCheckHttpVerb.expected
+++ b/ruby/ql/test/query-tests/experimental/manually-check-http-verb/ManuallyCheckHttpVerb.expected
@@ -24,18 +24,9 @@ nodes
 subpaths
 #select
 | ManuallyCheckHttpVerb.rb:4:8:4:19 | call to get? | ManuallyCheckHttpVerb.rb:4:8:4:19 | call to get? | ManuallyCheckHttpVerb.rb:4:8:4:19 | call to get? | 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. |
-| ManuallyCheckHttpVerb.rb:4:8:4:19 | call to get? | ManuallyCheckHttpVerb.rb:4:8:4:19 | call to get? | ManuallyCheckHttpVerb.rb:4:8:4:19 | call to get? :  | 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. |
-| ManuallyCheckHttpVerb.rb:4:8:4:19 | call to get? | ManuallyCheckHttpVerb.rb:4:8:4:19 | call to get? :  | ManuallyCheckHttpVerb.rb:4:8:4:19 | call to get? | 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. |
-| ManuallyCheckHttpVerb.rb:4:8:4:19 | call to get? | ManuallyCheckHttpVerb.rb:4:8:4:19 | call to get? :  | ManuallyCheckHttpVerb.rb:4:8:4:19 | call to get? :  | 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. |
 | ManuallyCheckHttpVerb.rb:12:8:12:22 | ... == ... | ManuallyCheckHttpVerb.rb:11:14:11:24 | call to env :  | ManuallyCheckHttpVerb.rb:12:8:12:22 | ... == ... | 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. |
-| ManuallyCheckHttpVerb.rb:12:8:12:22 | ... == ... | ManuallyCheckHttpVerb.rb:11:14:11:24 | call to env :  | ManuallyCheckHttpVerb.rb:12:8:12:22 | ... == ... :  | 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. |
 | ManuallyCheckHttpVerb.rb:20:8:20:22 | ... == ... | ManuallyCheckHttpVerb.rb:19:14:19:35 | call to request_method :  | ManuallyCheckHttpVerb.rb:20:8:20:22 | ... == ... | 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. |
-| ManuallyCheckHttpVerb.rb:20:8:20:22 | ... == ... | ManuallyCheckHttpVerb.rb:19:14:19:35 | call to request_method :  | ManuallyCheckHttpVerb.rb:20:8:20:22 | ... == ... :  | 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. |
 | ManuallyCheckHttpVerb.rb:28:8:28:22 | ... == ... | ManuallyCheckHttpVerb.rb:27:14:27:27 | call to method :  | ManuallyCheckHttpVerb.rb:28:8:28:22 | ... == ... | 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. |
-| ManuallyCheckHttpVerb.rb:28:8:28:22 | ... == ... | ManuallyCheckHttpVerb.rb:27:14:27:27 | call to method :  | ManuallyCheckHttpVerb.rb:28:8:28:22 | ... == ... :  | 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. |
 | ManuallyCheckHttpVerb.rb:36:8:36:22 | ... == ... | ManuallyCheckHttpVerb.rb:35:14:35:39 | call to raw_request_method :  | ManuallyCheckHttpVerb.rb:36:8:36:22 | ... == ... | 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. |
-| ManuallyCheckHttpVerb.rb:36:8:36:22 | ... == ... | ManuallyCheckHttpVerb.rb:35:14:35:39 | call to raw_request_method :  | ManuallyCheckHttpVerb.rb:36:8:36:22 | ... == ... :  | 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. |
 | ManuallyCheckHttpVerb.rb:52:10:52:23 | ... == ... | ManuallyCheckHttpVerb.rb:51:16:51:44 | call to request_method_symbol :  | ManuallyCheckHttpVerb.rb:52:10:52:23 | ... == ... | 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. |
-| ManuallyCheckHttpVerb.rb:52:10:52:23 | ... == ... | ManuallyCheckHttpVerb.rb:51:16:51:44 | call to request_method_symbol :  | ManuallyCheckHttpVerb.rb:52:10:52:23 | ... == ... :  | 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. |
 | ManuallyCheckHttpVerb.rb:59:10:59:38 | ...[...] | ManuallyCheckHttpVerb.rb:59:10:59:20 | call to env :  | ManuallyCheckHttpVerb.rb:59:10:59:38 | ...[...] | 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. |
-| ManuallyCheckHttpVerb.rb:59:10:59:38 | ...[...] | ManuallyCheckHttpVerb.rb:59:10:59:20 | call to env :  | ManuallyCheckHttpVerb.rb:59:10:59:38 | ...[...] :  | 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. |

From c1a6ca5f9458346453a98938b1e0616e8428f43f Mon Sep 17 00:00:00 2001
From: thiggy1342 <thiggy1342@github.com>
Date: Thu, 21 Jul 2022 22:11:14 +0000
Subject: [PATCH 21/22] add change note

---
 ruby/ql/src/change-notes/2022-07-21-check-http-verb.md | 4 ++++
 1 file changed, 4 insertions(+)
 create mode 100644 ruby/ql/src/change-notes/2022-07-21-check-http-verb.md

diff --git a/ruby/ql/src/change-notes/2022-07-21-check-http-verb.md b/ruby/ql/src/change-notes/2022-07-21-check-http-verb.md
new file mode 100644
index 000000000000..3d1a978953d1
--- /dev/null
+++ b/ruby/ql/src/change-notes/2022-07-21-check-http-verb.md
@@ -0,0 +1,4 @@
+---
+category: newQuery
+---
+* Added a new query, rb/manually-checking-http-verb, to detect cases when the HTTP verb for an incoming request is checked and then used as part of control flow.
\ No newline at end of file

From c2710fb038003a1c29b28ba941ff0367cf42f39c Mon Sep 17 00:00:00 2001
From: thiggy1342 <thiggy1342@github.com>
Date: Fri, 22 Jul 2022 13:52:00 -0400
Subject: [PATCH 22/22] Update
 ruby/ql/src/change-notes/2022-07-21-check-http-verb.md

Co-authored-by: Harry Maclean <hmac@github.com>
---
 ruby/ql/src/change-notes/2022-07-21-check-http-verb.md | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ruby/ql/src/change-notes/2022-07-21-check-http-verb.md b/ruby/ql/src/change-notes/2022-07-21-check-http-verb.md
index 3d1a978953d1..4a670ba1092b 100644
--- a/ruby/ql/src/change-notes/2022-07-21-check-http-verb.md
+++ b/ruby/ql/src/change-notes/2022-07-21-check-http-verb.md
@@ -1,4 +1,4 @@
 ---
 category: newQuery
 ---
-* Added a new query, rb/manually-checking-http-verb, to detect cases when the HTTP verb for an incoming request is checked and then used as part of control flow.
\ No newline at end of file
+* Added a new experimental query, `rb/manually-checking-http-verb`, to detect cases when the HTTP verb for an incoming request is checked and then used as part of control flow.
\ No newline at end of file