From 8c31b612ca01c004e3bbfab2881b2d2bb60ebd88 Mon Sep 17 00:00:00 2001
From: Joe Farebrother <joefarebrother@github.com>
Date: Wed, 13 Mar 2024 15:48:21 +0000
Subject: [PATCH 1/7] Model UploadedFile `original_filename` and `read`

---
 .../ruby/frameworks/ActionController.qll      | 60 +++++++++++++++++++
 1 file changed, 60 insertions(+)

diff --git a/ruby/ql/lib/codeql/ruby/frameworks/ActionController.qll b/ruby/ql/lib/codeql/ruby/frameworks/ActionController.qll
index be1df5066e1d..c6d6014f8dad 100644
--- a/ruby/ql/lib/codeql/ruby/frameworks/ActionController.qll
+++ b/ruby/ql/lib/codeql/ruby/frameworks/ActionController.qll
@@ -8,6 +8,7 @@ private import codeql.ruby.controlflow.CfgNodes
 private import codeql.ruby.DataFlow
 private import codeql.ruby.dataflow.RemoteFlowSources
 private import codeql.ruby.ApiGraphs
+private import codeql.ruby.typetracking.TypeTracking
 private import codeql.ruby.frameworks.ActionDispatch
 private import codeql.ruby.frameworks.ActionView
 private import codeql.ruby.frameworks.Rails
@@ -505,6 +506,27 @@ private module ParamsSummaries {
       ]
   }
 
+  /** Gets a field of an instance of `ActionController::Parameters` */
+  private DataFlow::LocalSourceNode paramsField() {
+    result =
+      [
+        paramsInstance(),
+        paramsInstance().getAMethodCall(methodReturnsTaintFromSelf()).getAnElementRead*()
+      ]
+  }
+
+  private DataFlow::LocalSourceNode paramsFieldType(TypeTracker t) {
+    t.start() and
+    result = paramsField()
+    or
+    exists(TypeTracker t2 | result = paramsFieldType(t2).track(t2, t))
+  }
+
+  /** Gets a node with a type that can be a field of `ActionController::Parameters */
+  private DataFlow::LocalSourceNode paramsFieldType() {
+    paramsFieldType(TypeTracker::end()).flowsTo(result)
+  }
+
   /**
    * A flow summary for methods on `ActionController::Parameters` which
    * propagate taint from receiver to return value.
@@ -569,6 +591,44 @@ private module ParamsSummaries {
       preservesValue = false
     }
   }
+
+  /** Flow summaries for `ActiveDispatch::Http::UploadedFile`, which can be an field of `ActionController::Parameters`. */
+  module UploadedFileSummaries {
+    /** Flow summary for `ActiveDispatch::Http::UploadedFile.original_filename` */
+    private class UploadedFileOriginalFilenameSummary extends SummarizedCallable {
+      UploadedFileOriginalFilenameSummary() {
+        this = "ActionDispatch::Http::UploadedFile::original_filename"
+      }
+
+      override MethodCall getACall() {
+        result = paramsFieldType().getAMethodCall("original_filename").asExpr().getExpr() and
+        result.getNumberOfArguments() = 0
+      }
+
+      override predicate propagatesFlow(string input, string output, boolean preservesValue) {
+        input = "Argument[self]" and output = "ReturnValue" and preservesValue = false
+      }
+    }
+
+    /**
+     * Flow summary for `ActiveDispatch::Http::UploadedFile.original_filename`,
+     * which propagates taint from the receiver to the return value or to the second (buffer) argument
+     */
+    private class UploadedFileReadSummary extends SummarizedCallable {
+      UploadedFileReadSummary() { this = "ActionDispatch::Http::UploadedFile::read" }
+
+      override MethodCall getACall() {
+        result = paramsFieldType().getAMethodCall("read").asExpr().getExpr() and
+        result.getNumberOfArguments() in [0 .. 2]
+      }
+
+      override predicate propagatesFlow(string input, string output, boolean preservesValue) {
+        input = "Argument[self]" and
+        output = ["ReturnValue", "Argument[1]"] and
+        preservesValue = false
+      }
+    }
+  }
 }
 
 /**

From 5333c759192212565762a33680bf9e843daac11c Mon Sep 17 00:00:00 2001
From: Joe Farebrother <joefarebrother@github.com>
Date: Thu, 14 Mar 2024 14:44:20 +0000
Subject: [PATCH 2/7] Model additional string attributes

---
 .../codeql/ruby/frameworks/ActionController.qll    | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/ruby/ql/lib/codeql/ruby/frameworks/ActionController.qll b/ruby/ql/lib/codeql/ruby/frameworks/ActionController.qll
index c6d6014f8dad..0c49b1f074f0 100644
--- a/ruby/ql/lib/codeql/ruby/frameworks/ActionController.qll
+++ b/ruby/ql/lib/codeql/ruby/frameworks/ActionController.qll
@@ -594,14 +594,18 @@ private module ParamsSummaries {
 
   /** Flow summaries for `ActiveDispatch::Http::UploadedFile`, which can be an field of `ActionController::Parameters`. */
   module UploadedFileSummaries {
-    /** Flow summary for `ActiveDispatch::Http::UploadedFile.original_filename` */
-    private class UploadedFileOriginalFilenameSummary extends SummarizedCallable {
-      UploadedFileOriginalFilenameSummary() {
-        this = "ActionDispatch::Http::UploadedFile::original_filename"
+    /** Flow summary for various string attributes of `UploadedFile`, including `original_filename`, `content_type`, and `headers`. */
+    private class UploadedFileStringAttributeSummary extends SummarizedCallable {
+      UploadedFileStringAttributeSummary() {
+        this = "ActionDispatch::Http::UploadedFile::[original_filename,content_type,headers]"
       }
 
       override MethodCall getACall() {
-        result = paramsFieldType().getAMethodCall("original_filename").asExpr().getExpr() and
+        result =
+          paramsFieldType()
+              .getAMethodCall(["original_filename", "content_type", "headers"])
+              .asExpr()
+              .getExpr() and
         result.getNumberOfArguments() = 0
       }
 

From 3e61be1b6a1a20db3ffb6624c57fc558ce830715 Mon Sep 17 00:00:00 2001
From: Joe Farebrother <joefarebrother@github.com>
Date: Thu, 14 Mar 2024 14:46:46 +0000
Subject: [PATCH 3/7] Add test cases

---
 .../dataflow/local/TaintStep.expected         |  3 ++
 .../ActionController.expected                 | 25 ++++++++++++
 .../action_controller/params-flow.expected    | 40 +++++++++++++++++++
 .../action_controller/params_flow.rb          | 29 ++++++++++++++
 4 files changed, 97 insertions(+)

diff --git a/ruby/ql/test/library-tests/dataflow/local/TaintStep.expected b/ruby/ql/test/library-tests/dataflow/local/TaintStep.expected
index f640ff6551a6..e73f2e3cb107 100644
--- a/ruby/ql/test/library-tests/dataflow/local/TaintStep.expected
+++ b/ruby/ql/test/library-tests/dataflow/local/TaintStep.expected
@@ -2835,6 +2835,9 @@
 | file://:0:0:0:0 | [summary param] self in ActionController::Parameters#merge | file://:0:0:0:0 | [summary] to write: ReturnValue in ActionController::Parameters#merge |
 | file://:0:0:0:0 | [summary param] self in ActionController::Parameters#merge! | file://:0:0:0:0 | [summary] to write: Argument[self] in ActionController::Parameters#merge! |
 | file://:0:0:0:0 | [summary param] self in ActionController::Parameters#merge! | file://:0:0:0:0 | [summary] to write: ReturnValue in ActionController::Parameters#merge! |
+| file://:0:0:0:0 | [summary param] self in ActionDispatch::Http::UploadedFile::[original_filename,content_type,headers] | file://:0:0:0:0 | [summary] to write: ReturnValue in ActionDispatch::Http::UploadedFile::[original_filename,content_type,headers] |
+| file://:0:0:0:0 | [summary param] self in ActionDispatch::Http::UploadedFile::read | file://:0:0:0:0 | [summary] to write: Argument[1] in ActionDispatch::Http::UploadedFile::read |
+| file://:0:0:0:0 | [summary param] self in ActionDispatch::Http::UploadedFile::read | file://:0:0:0:0 | [summary] to write: ReturnValue in ActionDispatch::Http::UploadedFile::read |
 | file://:0:0:0:0 | [summary param] self in ActiveSupportStringTransform | file://:0:0:0:0 | [summary] to write: ReturnValue in ActiveSupportStringTransform |
 | file://:0:0:0:0 | [summary param] self in [] | file://:0:0:0:0 | [summary] to write: ReturnValue in [] |
 | file://:0:0:0:0 | [summary param] self in \| | file://:0:0:0:0 | [summary] read: Argument[self].Element[any] in \| |
diff --git a/ruby/ql/test/library-tests/frameworks/action_controller/ActionController.expected b/ruby/ql/test/library-tests/frameworks/action_controller/ActionController.expected
index 9276cc0b350a..7e5b5d6d001f 100644
--- a/ruby/ql/test/library-tests/frameworks/action_controller/ActionController.expected
+++ b/ruby/ql/test/library-tests/frameworks/action_controller/ActionController.expected
@@ -14,6 +14,7 @@ actionControllerControllerClasses
 | input_access.rb:1:1:58:3 | UsersController |
 | params_flow.rb:1:1:162:3 | MyController |
 | params_flow.rb:170:1:178:3 | Subclass |
+| params_flow.rb:180:1:205:5 | UploadedFileTests |
 actionControllerActionMethods
 | app/controllers/comments_controller.rb:17:3:51:5 | index |
 | app/controllers/comments_controller.rb:53:3:54:5 | create |
@@ -86,6 +87,12 @@ actionControllerActionMethods
 | params_flow.rb:152:3:159:5 | m33 |
 | params_flow.rb:165:3:167:5 | m34 |
 | params_flow.rb:171:3:173:5 | m35 |
+| params_flow.rb:181:3:183:5 | m36 |
+| params_flow.rb:185:3:187:5 | m37 |
+| params_flow.rb:189:3:191:5 | m38 |
+| params_flow.rb:193:3:195:5 | m39 |
+| params_flow.rb:197:3:201:5 | m40 |
+| params_flow.rb:203:3:205:5 | m41 |
 paramsCalls
 | app/controllers/comments_controller.rb:80:36:80:41 | call to params |
 | app/controllers/foo/bars_controller.rb:13:21:13:26 | call to params |
@@ -146,6 +153,12 @@ paramsCalls
 | params_flow.rb:166:10:166:15 | call to params |
 | params_flow.rb:172:10:172:15 | call to params |
 | params_flow.rb:176:10:176:15 | call to params |
+| params_flow.rb:182:10:182:15 | call to params |
+| params_flow.rb:186:10:186:15 | call to params |
+| params_flow.rb:190:10:190:15 | call to params |
+| params_flow.rb:194:10:194:15 | call to params |
+| params_flow.rb:199:5:199:10 | call to params |
+| params_flow.rb:204:10:204:15 | call to params |
 paramsSources
 | app/controllers/comments_controller.rb:80:36:80:41 | call to params |
 | app/controllers/foo/bars_controller.rb:13:21:13:26 | call to params |
@@ -206,6 +219,12 @@ paramsSources
 | params_flow.rb:166:10:166:15 | call to params |
 | params_flow.rb:172:10:172:15 | call to params |
 | params_flow.rb:176:10:176:15 | call to params |
+| params_flow.rb:182:10:182:15 | call to params |
+| params_flow.rb:186:10:186:15 | call to params |
+| params_flow.rb:190:10:190:15 | call to params |
+| params_flow.rb:194:10:194:15 | call to params |
+| params_flow.rb:199:5:199:10 | call to params |
+| params_flow.rb:204:10:204:15 | call to params |
 httpInputAccesses
 | app/controllers/application_controller.rb:11:53:11:64 | call to path | ActionDispatch::Request#path |
 | app/controllers/comments_controller.rb:18:5:18:18 | call to params | ActionDispatch::Request#params |
@@ -324,6 +343,12 @@ httpInputAccesses
 | params_flow.rb:166:10:166:15 | call to params | ActionController::Metal#params |
 | params_flow.rb:172:10:172:15 | call to params | ActionController::Metal#params |
 | params_flow.rb:176:10:176:15 | call to params | ActionController::Metal#params |
+| params_flow.rb:182:10:182:15 | call to params | ActionController::Metal#params |
+| params_flow.rb:186:10:186:15 | call to params | ActionController::Metal#params |
+| params_flow.rb:190:10:190:15 | call to params | ActionController::Metal#params |
+| params_flow.rb:194:10:194:15 | call to params | ActionController::Metal#params |
+| params_flow.rb:199:5:199:10 | call to params | ActionController::Metal#params |
+| params_flow.rb:204:10:204:15 | call to params | ActionController::Metal#params |
 cookiesCalls
 | app/controllers/foo/bars_controller.rb:10:27:10:33 | call to cookies |
 cookiesSources
diff --git a/ruby/ql/test/library-tests/frameworks/action_controller/params-flow.expected b/ruby/ql/test/library-tests/frameworks/action_controller/params-flow.expected
index 69946539384b..698d3b23ccb6 100644
--- a/ruby/ql/test/library-tests/frameworks/action_controller/params-flow.expected
+++ b/ruby/ql/test/library-tests/frameworks/action_controller/params-flow.expected
@@ -101,6 +101,21 @@ edges
 | params_flow.rb:166:10:166:15 | call to params | params_flow.rb:166:10:166:19 | ...[...] | provenance |  |
 | params_flow.rb:172:10:172:15 | call to params | params_flow.rb:172:10:172:19 | ...[...] | provenance |  |
 | params_flow.rb:176:10:176:15 | call to params | params_flow.rb:176:10:176:19 | ...[...] | provenance |  |
+| params_flow.rb:182:10:182:15 | call to params | params_flow.rb:182:10:182:22 | ...[...] | provenance |  |
+| params_flow.rb:182:10:182:22 | ...[...] | params_flow.rb:182:10:182:40 | call to original_filename | provenance |  |
+| params_flow.rb:186:10:186:15 | call to params | params_flow.rb:186:10:186:30 | call to require | provenance |  |
+| params_flow.rb:186:10:186:30 | call to require | params_flow.rb:186:10:186:43 | call to content_type | provenance |  |
+| params_flow.rb:190:10:190:15 | call to params | params_flow.rb:190:10:190:29 | call to permit | provenance |  |
+| params_flow.rb:190:10:190:29 | call to permit | params_flow.rb:190:10:190:36 | ...[...] | provenance |  |
+| params_flow.rb:190:10:190:36 | ...[...] | params_flow.rb:190:10:190:44 | call to headers | provenance |  |
+| params_flow.rb:194:10:194:15 | call to params | params_flow.rb:194:10:194:19 | ...[...] | provenance |  |
+| params_flow.rb:194:10:194:19 | ...[...] | params_flow.rb:194:10:194:31 | call to to_unsafe_h | provenance |  |
+| params_flow.rb:194:10:194:31 | call to to_unsafe_h | params_flow.rb:194:10:194:35 | ...[...] | provenance |  |
+| params_flow.rb:194:10:194:35 | ...[...] | params_flow.rb:194:10:194:42 | ...[...] | provenance |  |
+| params_flow.rb:194:10:194:42 | ...[...] | params_flow.rb:194:10:194:47 | call to read | provenance |  |
+| params_flow.rb:198:5:198:10 | call to params | params_flow.rb:198:5:198:17 | ...[...] | provenance |  |
+| params_flow.rb:198:5:198:17 | ...[...] | params_flow.rb:198:28:198:28 | [post] a | provenance |  |
+| params_flow.rb:198:28:198:28 | [post] a | params_flow.rb:199:10:199:10 | a | provenance |  |
 nodes
 | filter_flow.rb:14:5:14:8 | [post] self [@foo] | semmle.label | [post] self [@foo] |
 | filter_flow.rb:14:12:14:17 | call to params | semmle.label | call to params |
@@ -244,6 +259,26 @@ nodes
 | params_flow.rb:172:10:172:19 | ...[...] | semmle.label | ...[...] |
 | params_flow.rb:176:10:176:15 | call to params | semmle.label | call to params |
 | params_flow.rb:176:10:176:19 | ...[...] | semmle.label | ...[...] |
+| params_flow.rb:182:10:182:15 | call to params | semmle.label | call to params |
+| params_flow.rb:182:10:182:22 | ...[...] | semmle.label | ...[...] |
+| params_flow.rb:182:10:182:40 | call to original_filename | semmle.label | call to original_filename |
+| params_flow.rb:186:10:186:15 | call to params | semmle.label | call to params |
+| params_flow.rb:186:10:186:30 | call to require | semmle.label | call to require |
+| params_flow.rb:186:10:186:43 | call to content_type | semmle.label | call to content_type |
+| params_flow.rb:190:10:190:15 | call to params | semmle.label | call to params |
+| params_flow.rb:190:10:190:29 | call to permit | semmle.label | call to permit |
+| params_flow.rb:190:10:190:36 | ...[...] | semmle.label | ...[...] |
+| params_flow.rb:190:10:190:44 | call to headers | semmle.label | call to headers |
+| params_flow.rb:194:10:194:15 | call to params | semmle.label | call to params |
+| params_flow.rb:194:10:194:19 | ...[...] | semmle.label | ...[...] |
+| params_flow.rb:194:10:194:31 | call to to_unsafe_h | semmle.label | call to to_unsafe_h |
+| params_flow.rb:194:10:194:35 | ...[...] | semmle.label | ...[...] |
+| params_flow.rb:194:10:194:42 | ...[...] | semmle.label | ...[...] |
+| params_flow.rb:194:10:194:47 | call to read | semmle.label | call to read |
+| params_flow.rb:198:5:198:10 | call to params | semmle.label | call to params |
+| params_flow.rb:198:5:198:17 | ...[...] | semmle.label | ...[...] |
+| params_flow.rb:198:28:198:28 | [post] a | semmle.label | [post] a |
+| params_flow.rb:199:10:199:10 | a | semmle.label | a |
 subpaths
 #select
 | filter_flow.rb:21:10:21:13 | @foo | filter_flow.rb:14:12:14:17 | call to params | filter_flow.rb:21:10:21:13 | @foo | $@ | filter_flow.rb:14:12:14:17 | call to params | call to params |
@@ -298,3 +333,8 @@ subpaths
 | params_flow.rb:166:10:166:19 | ...[...] | params_flow.rb:166:10:166:15 | call to params | params_flow.rb:166:10:166:19 | ...[...] | $@ | params_flow.rb:166:10:166:15 | call to params | call to params |
 | params_flow.rb:172:10:172:19 | ...[...] | params_flow.rb:172:10:172:15 | call to params | params_flow.rb:172:10:172:19 | ...[...] | $@ | params_flow.rb:172:10:172:15 | call to params | call to params |
 | params_flow.rb:176:10:176:19 | ...[...] | params_flow.rb:176:10:176:15 | call to params | params_flow.rb:176:10:176:19 | ...[...] | $@ | params_flow.rb:176:10:176:15 | call to params | call to params |
+| params_flow.rb:182:10:182:40 | call to original_filename | params_flow.rb:182:10:182:15 | call to params | params_flow.rb:182:10:182:40 | call to original_filename | $@ | params_flow.rb:182:10:182:15 | call to params | call to params |
+| params_flow.rb:186:10:186:43 | call to content_type | params_flow.rb:186:10:186:15 | call to params | params_flow.rb:186:10:186:43 | call to content_type | $@ | params_flow.rb:186:10:186:15 | call to params | call to params |
+| params_flow.rb:190:10:190:44 | call to headers | params_flow.rb:190:10:190:15 | call to params | params_flow.rb:190:10:190:44 | call to headers | $@ | params_flow.rb:190:10:190:15 | call to params | call to params |
+| params_flow.rb:194:10:194:47 | call to read | params_flow.rb:194:10:194:15 | call to params | params_flow.rb:194:10:194:47 | call to read | $@ | params_flow.rb:194:10:194:15 | call to params | call to params |
+| params_flow.rb:199:10:199:10 | a | params_flow.rb:198:5:198:10 | call to params | params_flow.rb:199:10:199:10 | a | $@ | params_flow.rb:198:5:198:10 | call to params | call to params |
diff --git a/ruby/ql/test/library-tests/frameworks/action_controller/params_flow.rb b/ruby/ql/test/library-tests/frameworks/action_controller/params_flow.rb
index 65aba8fabf2d..018d8b58af01 100644
--- a/ruby/ql/test/library-tests/frameworks/action_controller/params_flow.rb
+++ b/ruby/ql/test/library-tests/frameworks/action_controller/params_flow.rb
@@ -176,3 +176,32 @@ def m35
     sink params[:x] # $hasTaintFlow
   end
 end
+
+class UploadedFileTests < MyController
+  def m36
+    sink params[:file].original_filename # $hasTaintFlow
+  end
+
+  def m37
+    sink params.require(:file).content_type # $hasTaintFlow
+  end
+
+  def m38
+    sink params.permit(:file)[:file].headers # $hasTaintFlow
+  end
+
+  def m39
+    sink params[:a].to_unsafe_h[:b][:file].read # $hasTaintFlow
+  end
+
+  def m40(a)
+    params[:file].read(nil,a)
+    sink a # $ hasTaintFlow
+  end
+
+  def m41
+    a = ""
+    params[:file].read(nil,a)
+    sink a # $ MISSING:hasTaintFlow
+  end
+end
\ No newline at end of file

From b4ed77343b77cdc16aea4e04af294883ebbefdb7 Mon Sep 17 00:00:00 2001
From: Joe Farebrother <joefarebrother@github.com>
Date: Thu, 14 Mar 2024 15:44:53 +0000
Subject: [PATCH 4/7] Add change note + fix qldoc

---
 .../2024-03-14-actiondispatch-uploadedfile.md             | 4 ++++
 ruby/ql/lib/codeql/ruby/frameworks/ActionController.qll   | 8 ++++----
 .../test/library-tests/dataflow/local/TaintStep.expected  | 6 +++---
 3 files changed, 11 insertions(+), 7 deletions(-)
 create mode 100644 ruby/ql/lib/change-notes/2024-03-14-actiondispatch-uploadedfile.md

diff --git a/ruby/ql/lib/change-notes/2024-03-14-actiondispatch-uploadedfile.md b/ruby/ql/lib/change-notes/2024-03-14-actiondispatch-uploadedfile.md
new file mode 100644
index 000000000000..a02ca0d00a2a
--- /dev/null
+++ b/ruby/ql/lib/change-notes/2024-03-14-actiondispatch-uploadedfile.md
@@ -0,0 +1,4 @@
+---
+category: minorAnalysis
+---
+* Modeled instances of `ActionDispatch::Http::UploadedFile` that can be obtained from element reads of `ActionController::Parameters`, with calls to `original_filename`, `content_type`, and `read` now propagating taint from their receiver. 
\ No newline at end of file
diff --git a/ruby/ql/lib/codeql/ruby/frameworks/ActionController.qll b/ruby/ql/lib/codeql/ruby/frameworks/ActionController.qll
index 0c49b1f074f0..7578ba52bb13 100644
--- a/ruby/ql/lib/codeql/ruby/frameworks/ActionController.qll
+++ b/ruby/ql/lib/codeql/ruby/frameworks/ActionController.qll
@@ -597,7 +597,7 @@ private module ParamsSummaries {
     /** Flow summary for various string attributes of `UploadedFile`, including `original_filename`, `content_type`, and `headers`. */
     private class UploadedFileStringAttributeSummary extends SummarizedCallable {
       UploadedFileStringAttributeSummary() {
-        this = "ActionDispatch::Http::UploadedFile::[original_filename,content_type,headers]"
+        this = "ActionDispatch::Http::UploadedFile#[original_filename,content_type,headers]"
       }
 
       override MethodCall getACall() {
@@ -615,11 +615,11 @@ private module ParamsSummaries {
     }
 
     /**
-     * Flow summary for `ActiveDispatch::Http::UploadedFile.original_filename`,
-     * which propagates taint from the receiver to the return value or to the second (buffer) argument
+     * Flow summary for `ActiveDispatch::Http::UploadedFile#read`,
+     * which propagates taint from the receiver to the return value or to the second (out string) argument
      */
     private class UploadedFileReadSummary extends SummarizedCallable {
-      UploadedFileReadSummary() { this = "ActionDispatch::Http::UploadedFile::read" }
+      UploadedFileReadSummary() { this = "ActionDispatch::Http::UploadedFile#read" }
 
       override MethodCall getACall() {
         result = paramsFieldType().getAMethodCall("read").asExpr().getExpr() and
diff --git a/ruby/ql/test/library-tests/dataflow/local/TaintStep.expected b/ruby/ql/test/library-tests/dataflow/local/TaintStep.expected
index e73f2e3cb107..44f879946ab7 100644
--- a/ruby/ql/test/library-tests/dataflow/local/TaintStep.expected
+++ b/ruby/ql/test/library-tests/dataflow/local/TaintStep.expected
@@ -2835,9 +2835,9 @@
 | file://:0:0:0:0 | [summary param] self in ActionController::Parameters#merge | file://:0:0:0:0 | [summary] to write: ReturnValue in ActionController::Parameters#merge |
 | file://:0:0:0:0 | [summary param] self in ActionController::Parameters#merge! | file://:0:0:0:0 | [summary] to write: Argument[self] in ActionController::Parameters#merge! |
 | file://:0:0:0:0 | [summary param] self in ActionController::Parameters#merge! | file://:0:0:0:0 | [summary] to write: ReturnValue in ActionController::Parameters#merge! |
-| file://:0:0:0:0 | [summary param] self in ActionDispatch::Http::UploadedFile::[original_filename,content_type,headers] | file://:0:0:0:0 | [summary] to write: ReturnValue in ActionDispatch::Http::UploadedFile::[original_filename,content_type,headers] |
-| file://:0:0:0:0 | [summary param] self in ActionDispatch::Http::UploadedFile::read | file://:0:0:0:0 | [summary] to write: Argument[1] in ActionDispatch::Http::UploadedFile::read |
-| file://:0:0:0:0 | [summary param] self in ActionDispatch::Http::UploadedFile::read | file://:0:0:0:0 | [summary] to write: ReturnValue in ActionDispatch::Http::UploadedFile::read |
+| file://:0:0:0:0 | [summary param] self in ActionDispatch::Http::UploadedFile#[original_filename,content_type,headers] | file://:0:0:0:0 | [summary] to write: ReturnValue in ActionDispatch::Http::UploadedFile#[original_filename,content_type,headers] |
+| file://:0:0:0:0 | [summary param] self in ActionDispatch::Http::UploadedFile#read | file://:0:0:0:0 | [summary] to write: Argument[1] in ActionDispatch::Http::UploadedFile#read |
+| file://:0:0:0:0 | [summary param] self in ActionDispatch::Http::UploadedFile#read | file://:0:0:0:0 | [summary] to write: ReturnValue in ActionDispatch::Http::UploadedFile#read |
 | file://:0:0:0:0 | [summary param] self in ActiveSupportStringTransform | file://:0:0:0:0 | [summary] to write: ReturnValue in ActiveSupportStringTransform |
 | file://:0:0:0:0 | [summary param] self in [] | file://:0:0:0:0 | [summary] to write: ReturnValue in [] |
 | file://:0:0:0:0 | [summary param] self in \| | file://:0:0:0:0 | [summary] read: Argument[self].Element[any] in \| |

From f464f1b94ef8a3326a51689729c020636ac688e2 Mon Sep 17 00:00:00 2001
From: Joe Farebrother <joefarebrother@github.com>
Date: Thu, 14 Mar 2024 16:36:26 +0000
Subject: [PATCH 5/7] Accept test output + fix qldoc typo

---
 .../ruby/frameworks/ActionController.qll       |  2 +-
 .../ActionController.expected                  | 18 +++++++++---------
 2 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/ruby/ql/lib/codeql/ruby/frameworks/ActionController.qll b/ruby/ql/lib/codeql/ruby/frameworks/ActionController.qll
index 7578ba52bb13..19dcb82cfd69 100644
--- a/ruby/ql/lib/codeql/ruby/frameworks/ActionController.qll
+++ b/ruby/ql/lib/codeql/ruby/frameworks/ActionController.qll
@@ -522,7 +522,7 @@ private module ParamsSummaries {
     exists(TypeTracker t2 | result = paramsFieldType(t2).track(t2, t))
   }
 
-  /** Gets a node with a type that can be a field of `ActionController::Parameters */
+  /** Gets a node with a type that can be a field of `ActionController::Parameters` */
   private DataFlow::LocalSourceNode paramsFieldType() {
     paramsFieldType(TypeTracker::end()).flowsTo(result)
   }
diff --git a/ruby/ql/test/library-tests/frameworks/action_controller/ActionController.expected b/ruby/ql/test/library-tests/frameworks/action_controller/ActionController.expected
index 7e5b5d6d001f..9af92b159cd3 100644
--- a/ruby/ql/test/library-tests/frameworks/action_controller/ActionController.expected
+++ b/ruby/ql/test/library-tests/frameworks/action_controller/ActionController.expected
@@ -14,7 +14,7 @@ actionControllerControllerClasses
 | input_access.rb:1:1:58:3 | UsersController |
 | params_flow.rb:1:1:162:3 | MyController |
 | params_flow.rb:170:1:178:3 | Subclass |
-| params_flow.rb:180:1:205:5 | UploadedFileTests |
+| params_flow.rb:180:1:207:3 | UploadedFileTests |
 actionControllerActionMethods
 | app/controllers/comments_controller.rb:17:3:51:5 | index |
 | app/controllers/comments_controller.rb:53:3:54:5 | create |
@@ -91,8 +91,8 @@ actionControllerActionMethods
 | params_flow.rb:185:3:187:5 | m37 |
 | params_flow.rb:189:3:191:5 | m38 |
 | params_flow.rb:193:3:195:5 | m39 |
-| params_flow.rb:197:3:201:5 | m40 |
-| params_flow.rb:203:3:205:5 | m41 |
+| params_flow.rb:197:3:200:5 | m40 |
+| params_flow.rb:202:3:206:5 | m41 |
 paramsCalls
 | app/controllers/comments_controller.rb:80:36:80:41 | call to params |
 | app/controllers/foo/bars_controller.rb:13:21:13:26 | call to params |
@@ -157,8 +157,8 @@ paramsCalls
 | params_flow.rb:186:10:186:15 | call to params |
 | params_flow.rb:190:10:190:15 | call to params |
 | params_flow.rb:194:10:194:15 | call to params |
-| params_flow.rb:199:5:199:10 | call to params |
-| params_flow.rb:204:10:204:15 | call to params |
+| params_flow.rb:198:5:198:10 | call to params |
+| params_flow.rb:204:5:204:10 | call to params |
 paramsSources
 | app/controllers/comments_controller.rb:80:36:80:41 | call to params |
 | app/controllers/foo/bars_controller.rb:13:21:13:26 | call to params |
@@ -223,8 +223,8 @@ paramsSources
 | params_flow.rb:186:10:186:15 | call to params |
 | params_flow.rb:190:10:190:15 | call to params |
 | params_flow.rb:194:10:194:15 | call to params |
-| params_flow.rb:199:5:199:10 | call to params |
-| params_flow.rb:204:10:204:15 | call to params |
+| params_flow.rb:198:5:198:10 | call to params |
+| params_flow.rb:204:5:204:10 | call to params |
 httpInputAccesses
 | app/controllers/application_controller.rb:11:53:11:64 | call to path | ActionDispatch::Request#path |
 | app/controllers/comments_controller.rb:18:5:18:18 | call to params | ActionDispatch::Request#params |
@@ -347,8 +347,8 @@ httpInputAccesses
 | params_flow.rb:186:10:186:15 | call to params | ActionController::Metal#params |
 | params_flow.rb:190:10:190:15 | call to params | ActionController::Metal#params |
 | params_flow.rb:194:10:194:15 | call to params | ActionController::Metal#params |
-| params_flow.rb:199:5:199:10 | call to params | ActionController::Metal#params |
-| params_flow.rb:204:10:204:15 | call to params | ActionController::Metal#params |
+| params_flow.rb:198:5:198:10 | call to params | ActionController::Metal#params |
+| params_flow.rb:204:5:204:10 | call to params | ActionController::Metal#params |
 cookiesCalls
 | app/controllers/foo/bars_controller.rb:10:27:10:33 | call to cookies |
 cookiesSources

From e7b00a7b42976a08b2b4d0daf985a150a7341fd4 Mon Sep 17 00:00:00 2001
From: Tom Hvitved <hvitved@github.com>
Date: Fri, 15 Mar 2024 10:14:47 +0100
Subject: [PATCH 6/7] Ruby: Add post-update argument nodes for string constants

---
 .../codeql/ruby/dataflow/internal/DataFlowPrivate.qll    | 6 +++++-
 .../library-tests/dataflow/global/instance_variables.rb  | 2 +-
 .../library-tests/dataflow/local/DataflowStep.expected   | 9 +++++++++
 .../test/library-tests/dataflow/local/TaintStep.expected | 9 +++++++++
 .../frameworks/action_controller/params-flow.expected    | 8 ++++++++
 .../frameworks/action_controller/params_flow.rb          | 2 +-
 6 files changed, 33 insertions(+), 3 deletions(-)

diff --git a/ruby/ql/lib/codeql/ruby/dataflow/internal/DataFlowPrivate.qll b/ruby/ql/lib/codeql/ruby/dataflow/internal/DataFlowPrivate.qll
index a7ef050f1c86..3b97ebcf4c82 100644
--- a/ruby/ql/lib/codeql/ruby/dataflow/internal/DataFlowPrivate.qll
+++ b/ruby/ql/lib/codeql/ruby/dataflow/internal/DataFlowPrivate.qll
@@ -322,7 +322,11 @@ private class Argument extends CfgNodes::ExprCfgNode {
 
 /** Holds if `n` is not a constant expression. */
 predicate isNonConstantExpr(CfgNodes::ExprCfgNode n) {
-  not exists(n.getConstantValue()) and
+  not exists(ConstantValue cv |
+    cv = n.getConstantValue() and
+    // strings are mutable in Ruby
+    not cv.isString(_)
+  ) and
   not n.getExpr() instanceof ConstantAccess
 }
 
diff --git a/ruby/ql/test/library-tests/dataflow/global/instance_variables.rb b/ruby/ql/test/library-tests/dataflow/global/instance_variables.rb
index e1687bfed2c7..9943b3cb5792 100644
--- a/ruby/ql/test/library-tests/dataflow/global/instance_variables.rb
+++ b/ruby/ql/test/library-tests/dataflow/global/instance_variables.rb
@@ -70,7 +70,7 @@ def initialize x
 foo3.set_field(taint(22))
 sink(foo3.field) # $ hasValueFlow=22
 
-foo4 = "hello"
+foo4 = 4
 foo4.other = taint(23)
 sink(foo4.other) # no field flow for constants
 
diff --git a/ruby/ql/test/library-tests/dataflow/local/DataflowStep.expected b/ruby/ql/test/library-tests/dataflow/local/DataflowStep.expected
index 307ffc016114..1f773f7d1a45 100644
--- a/ruby/ql/test/library-tests/dataflow/local/DataflowStep.expected
+++ b/ruby/ql/test/library-tests/dataflow/local/DataflowStep.expected
@@ -2676,6 +2676,7 @@
 | local_dataflow.rb:131:7:131:8 | "" | local_dataflow.rb:131:3:131:8 | ... = ... |
 | local_dataflow.rb:132:6:132:11 | [post] self | local_dataflow.rb:133:8:133:13 | self |
 | local_dataflow.rb:132:6:132:11 | self | local_dataflow.rb:133:8:133:13 | self |
+| local_dataflow.rb:132:10:132:10 | [post] x | local_dataflow.rb:133:12:133:12 | x |
 | local_dataflow.rb:132:10:132:10 | x | local_dataflow.rb:133:12:133:12 | x |
 | local_dataflow.rb:132:12:148:10 | then ... | local_dataflow.rb:132:3:149:5 | if ... |
 | local_dataflow.rb:133:5:139:7 | SSA phi read(self) | local_dataflow.rb:141:9:141:14 | self |
@@ -2686,17 +2687,20 @@
 | local_dataflow.rb:133:8:133:13 | self | local_dataflow.rb:133:18:133:23 | self |
 | local_dataflow.rb:133:8:133:23 | SSA phi read(self) | local_dataflow.rb:134:7:134:12 | self |
 | local_dataflow.rb:133:8:133:23 | SSA phi read(x) | local_dataflow.rb:134:11:134:11 | x |
+| local_dataflow.rb:133:12:133:12 | [post] x | local_dataflow.rb:133:22:133:22 | x |
 | local_dataflow.rb:133:12:133:12 | x | local_dataflow.rb:133:22:133:22 | x |
 | local_dataflow.rb:133:18:133:23 | [post] self | local_dataflow.rb:136:7:136:12 | self |
 | local_dataflow.rb:133:18:133:23 | call to use | local_dataflow.rb:133:8:133:23 | [false] ... \|\| ... |
 | local_dataflow.rb:133:18:133:23 | call to use | local_dataflow.rb:133:8:133:23 | [true] ... \|\| ... |
 | local_dataflow.rb:133:18:133:23 | self | local_dataflow.rb:136:7:136:12 | self |
+| local_dataflow.rb:133:22:133:22 | [post] x | local_dataflow.rb:136:11:136:11 | x |
 | local_dataflow.rb:133:22:133:22 | x | local_dataflow.rb:136:11:136:11 | x |
 | local_dataflow.rb:133:24:134:12 | then ... | local_dataflow.rb:133:5:139:7 | if ... |
 | local_dataflow.rb:134:7:134:12 | call to use | local_dataflow.rb:133:24:134:12 | then ... |
 | local_dataflow.rb:135:5:138:9 | else ... | local_dataflow.rb:133:5:139:7 | if ... |
 | local_dataflow.rb:136:7:136:12 | [post] self | local_dataflow.rb:137:10:137:15 | self |
 | local_dataflow.rb:136:7:136:12 | self | local_dataflow.rb:137:10:137:15 | self |
+| local_dataflow.rb:136:11:136:11 | [post] x | local_dataflow.rb:137:14:137:14 | x |
 | local_dataflow.rb:136:11:136:11 | x | local_dataflow.rb:137:14:137:14 | x |
 | local_dataflow.rb:137:7:138:9 | SSA phi read(self) | local_dataflow.rb:133:5:139:7 | SSA phi read(self) |
 | local_dataflow.rb:137:7:138:9 | SSA phi read(x) | local_dataflow.rb:133:5:139:7 | SSA phi read(x) |
@@ -2705,6 +2709,7 @@
 | local_dataflow.rb:137:10:137:15 | self | local_dataflow.rb:137:21:137:26 | self |
 | local_dataflow.rb:137:10:137:26 | SSA phi read(self) | local_dataflow.rb:137:7:138:9 | SSA phi read(self) |
 | local_dataflow.rb:137:10:137:26 | SSA phi read(x) | local_dataflow.rb:137:7:138:9 | SSA phi read(x) |
+| local_dataflow.rb:137:14:137:14 | [post] x | local_dataflow.rb:137:25:137:25 | x |
 | local_dataflow.rb:137:14:137:14 | x | local_dataflow.rb:137:25:137:25 | x |
 | local_dataflow.rb:137:20:137:26 | [false] ! ... | local_dataflow.rb:137:10:137:26 | [false] ... && ... |
 | local_dataflow.rb:137:20:137:26 | [true] ! ... | local_dataflow.rb:137:10:137:26 | [true] ... && ... |
@@ -2717,6 +2722,7 @@
 | local_dataflow.rb:141:8:141:37 | SSA phi read(x) | local_dataflow.rb:141:5:145:7 | SSA phi read(x) |
 | local_dataflow.rb:141:9:141:14 | [post] self | local_dataflow.rb:141:20:141:25 | self |
 | local_dataflow.rb:141:9:141:14 | self | local_dataflow.rb:141:20:141:25 | self |
+| local_dataflow.rb:141:13:141:13 | [post] x | local_dataflow.rb:141:24:141:24 | x |
 | local_dataflow.rb:141:13:141:13 | x | local_dataflow.rb:141:24:141:24 | x |
 | local_dataflow.rb:141:19:141:37 | [false] ( ... ) | local_dataflow.rb:141:8:141:37 | [false] ... \|\| ... |
 | local_dataflow.rb:141:19:141:37 | [true] ( ... ) | local_dataflow.rb:141:8:141:37 | [true] ... \|\| ... |
@@ -2726,6 +2732,7 @@
 | local_dataflow.rb:141:20:141:36 | SSA phi read(x) | local_dataflow.rb:143:15:143:15 | x |
 | local_dataflow.rb:141:20:141:36 | [false] ... && ... | local_dataflow.rb:141:19:141:37 | [false] ( ... ) |
 | local_dataflow.rb:141:20:141:36 | [true] ... && ... | local_dataflow.rb:141:19:141:37 | [true] ( ... ) |
+| local_dataflow.rb:141:24:141:24 | [post] x | local_dataflow.rb:141:35:141:35 | x |
 | local_dataflow.rb:141:24:141:24 | x | local_dataflow.rb:141:35:141:35 | x |
 | local_dataflow.rb:141:30:141:36 | [false] ! ... | local_dataflow.rb:141:20:141:36 | [false] ... && ... |
 | local_dataflow.rb:141:30:141:36 | [true] ! ... | local_dataflow.rb:141:20:141:36 | [true] ... && ... |
@@ -2740,6 +2747,7 @@
 | local_dataflow.rb:143:11:143:16 | self | local_dataflow.rb:143:21:143:26 | self |
 | local_dataflow.rb:143:11:143:26 | SSA phi read(self) | local_dataflow.rb:144:11:144:16 | self |
 | local_dataflow.rb:143:11:143:26 | SSA phi read(x) | local_dataflow.rb:144:15:144:15 | x |
+| local_dataflow.rb:143:15:143:15 | [post] x | local_dataflow.rb:143:25:143:25 | x |
 | local_dataflow.rb:143:15:143:15 | x | local_dataflow.rb:143:25:143:25 | x |
 | local_dataflow.rb:143:21:143:26 | call to use | local_dataflow.rb:143:11:143:26 | [false] ... \|\| ... |
 | local_dataflow.rb:143:21:143:26 | call to use | local_dataflow.rb:143:11:143:26 | [true] ... \|\| ... |
@@ -2747,5 +2755,6 @@
 | local_dataflow.rb:144:11:144:16 | call to use | local_dataflow.rb:143:27:144:16 | then ... |
 | local_dataflow.rb:147:5:147:10 | [post] self | local_dataflow.rb:148:5:148:10 | self |
 | local_dataflow.rb:147:5:147:10 | self | local_dataflow.rb:148:5:148:10 | self |
+| local_dataflow.rb:147:9:147:9 | [post] x | local_dataflow.rb:148:9:148:9 | x |
 | local_dataflow.rb:147:9:147:9 | x | local_dataflow.rb:148:9:148:9 | x |
 | local_dataflow.rb:148:5:148:10 | call to use | local_dataflow.rb:132:12:148:10 | then ... |
diff --git a/ruby/ql/test/library-tests/dataflow/local/TaintStep.expected b/ruby/ql/test/library-tests/dataflow/local/TaintStep.expected
index 44f879946ab7..a462aebeba9f 100644
--- a/ruby/ql/test/library-tests/dataflow/local/TaintStep.expected
+++ b/ruby/ql/test/library-tests/dataflow/local/TaintStep.expected
@@ -3167,6 +3167,7 @@
 | local_dataflow.rb:131:7:131:8 | "" | local_dataflow.rb:131:3:131:8 | ... = ... |
 | local_dataflow.rb:132:6:132:11 | [post] self | local_dataflow.rb:133:8:133:13 | self |
 | local_dataflow.rb:132:6:132:11 | self | local_dataflow.rb:133:8:133:13 | self |
+| local_dataflow.rb:132:10:132:10 | [post] x | local_dataflow.rb:133:12:133:12 | x |
 | local_dataflow.rb:132:10:132:10 | x | local_dataflow.rb:133:12:133:12 | x |
 | local_dataflow.rb:132:12:148:10 | then ... | local_dataflow.rb:132:3:149:5 | if ... |
 | local_dataflow.rb:133:5:139:7 | SSA phi read(self) | local_dataflow.rb:141:9:141:14 | self |
@@ -3177,17 +3178,20 @@
 | local_dataflow.rb:133:8:133:13 | self | local_dataflow.rb:133:18:133:23 | self |
 | local_dataflow.rb:133:8:133:23 | SSA phi read(self) | local_dataflow.rb:134:7:134:12 | self |
 | local_dataflow.rb:133:8:133:23 | SSA phi read(x) | local_dataflow.rb:134:11:134:11 | x |
+| local_dataflow.rb:133:12:133:12 | [post] x | local_dataflow.rb:133:22:133:22 | x |
 | local_dataflow.rb:133:12:133:12 | x | local_dataflow.rb:133:22:133:22 | x |
 | local_dataflow.rb:133:18:133:23 | [post] self | local_dataflow.rb:136:7:136:12 | self |
 | local_dataflow.rb:133:18:133:23 | call to use | local_dataflow.rb:133:8:133:23 | [false] ... \|\| ... |
 | local_dataflow.rb:133:18:133:23 | call to use | local_dataflow.rb:133:8:133:23 | [true] ... \|\| ... |
 | local_dataflow.rb:133:18:133:23 | self | local_dataflow.rb:136:7:136:12 | self |
+| local_dataflow.rb:133:22:133:22 | [post] x | local_dataflow.rb:136:11:136:11 | x |
 | local_dataflow.rb:133:22:133:22 | x | local_dataflow.rb:136:11:136:11 | x |
 | local_dataflow.rb:133:24:134:12 | then ... | local_dataflow.rb:133:5:139:7 | if ... |
 | local_dataflow.rb:134:7:134:12 | call to use | local_dataflow.rb:133:24:134:12 | then ... |
 | local_dataflow.rb:135:5:138:9 | else ... | local_dataflow.rb:133:5:139:7 | if ... |
 | local_dataflow.rb:136:7:136:12 | [post] self | local_dataflow.rb:137:10:137:15 | self |
 | local_dataflow.rb:136:7:136:12 | self | local_dataflow.rb:137:10:137:15 | self |
+| local_dataflow.rb:136:11:136:11 | [post] x | local_dataflow.rb:137:14:137:14 | x |
 | local_dataflow.rb:136:11:136:11 | x | local_dataflow.rb:137:14:137:14 | x |
 | local_dataflow.rb:137:7:138:9 | SSA phi read(self) | local_dataflow.rb:133:5:139:7 | SSA phi read(self) |
 | local_dataflow.rb:137:7:138:9 | SSA phi read(x) | local_dataflow.rb:133:5:139:7 | SSA phi read(x) |
@@ -3196,6 +3200,7 @@
 | local_dataflow.rb:137:10:137:15 | self | local_dataflow.rb:137:21:137:26 | self |
 | local_dataflow.rb:137:10:137:26 | SSA phi read(self) | local_dataflow.rb:137:7:138:9 | SSA phi read(self) |
 | local_dataflow.rb:137:10:137:26 | SSA phi read(x) | local_dataflow.rb:137:7:138:9 | SSA phi read(x) |
+| local_dataflow.rb:137:14:137:14 | [post] x | local_dataflow.rb:137:25:137:25 | x |
 | local_dataflow.rb:137:14:137:14 | x | local_dataflow.rb:137:25:137:25 | x |
 | local_dataflow.rb:137:20:137:26 | [false] ! ... | local_dataflow.rb:137:10:137:26 | [false] ... && ... |
 | local_dataflow.rb:137:20:137:26 | [true] ! ... | local_dataflow.rb:137:10:137:26 | [true] ... && ... |
@@ -3212,6 +3217,7 @@
 | local_dataflow.rb:141:9:141:14 | call to use | local_dataflow.rb:141:8:141:14 | [false] ! ... |
 | local_dataflow.rb:141:9:141:14 | call to use | local_dataflow.rb:141:8:141:14 | [true] ! ... |
 | local_dataflow.rb:141:9:141:14 | self | local_dataflow.rb:141:20:141:25 | self |
+| local_dataflow.rb:141:13:141:13 | [post] x | local_dataflow.rb:141:24:141:24 | x |
 | local_dataflow.rb:141:13:141:13 | x | local_dataflow.rb:141:24:141:24 | x |
 | local_dataflow.rb:141:19:141:37 | [false] ( ... ) | local_dataflow.rb:141:8:141:37 | [false] ... \|\| ... |
 | local_dataflow.rb:141:19:141:37 | [true] ( ... ) | local_dataflow.rb:141:8:141:37 | [true] ... \|\| ... |
@@ -3221,6 +3227,7 @@
 | local_dataflow.rb:141:20:141:36 | SSA phi read(x) | local_dataflow.rb:143:15:143:15 | x |
 | local_dataflow.rb:141:20:141:36 | [false] ... && ... | local_dataflow.rb:141:19:141:37 | [false] ( ... ) |
 | local_dataflow.rb:141:20:141:36 | [true] ... && ... | local_dataflow.rb:141:19:141:37 | [true] ( ... ) |
+| local_dataflow.rb:141:24:141:24 | [post] x | local_dataflow.rb:141:35:141:35 | x |
 | local_dataflow.rb:141:24:141:24 | x | local_dataflow.rb:141:35:141:35 | x |
 | local_dataflow.rb:141:30:141:36 | [false] ! ... | local_dataflow.rb:141:20:141:36 | [false] ... && ... |
 | local_dataflow.rb:141:30:141:36 | [true] ! ... | local_dataflow.rb:141:20:141:36 | [true] ... && ... |
@@ -3237,6 +3244,7 @@
 | local_dataflow.rb:143:11:143:16 | self | local_dataflow.rb:143:21:143:26 | self |
 | local_dataflow.rb:143:11:143:26 | SSA phi read(self) | local_dataflow.rb:144:11:144:16 | self |
 | local_dataflow.rb:143:11:143:26 | SSA phi read(x) | local_dataflow.rb:144:15:144:15 | x |
+| local_dataflow.rb:143:15:143:15 | [post] x | local_dataflow.rb:143:25:143:25 | x |
 | local_dataflow.rb:143:15:143:15 | x | local_dataflow.rb:143:25:143:25 | x |
 | local_dataflow.rb:143:21:143:26 | call to use | local_dataflow.rb:143:11:143:26 | [false] ... \|\| ... |
 | local_dataflow.rb:143:21:143:26 | call to use | local_dataflow.rb:143:11:143:26 | [true] ... \|\| ... |
@@ -3244,5 +3252,6 @@
 | local_dataflow.rb:144:11:144:16 | call to use | local_dataflow.rb:143:27:144:16 | then ... |
 | local_dataflow.rb:147:5:147:10 | [post] self | local_dataflow.rb:148:5:148:10 | self |
 | local_dataflow.rb:147:5:147:10 | self | local_dataflow.rb:148:5:148:10 | self |
+| local_dataflow.rb:147:9:147:9 | [post] x | local_dataflow.rb:148:9:148:9 | x |
 | local_dataflow.rb:147:9:147:9 | x | local_dataflow.rb:148:9:148:9 | x |
 | local_dataflow.rb:148:5:148:10 | call to use | local_dataflow.rb:132:12:148:10 | then ... |
diff --git a/ruby/ql/test/library-tests/frameworks/action_controller/params-flow.expected b/ruby/ql/test/library-tests/frameworks/action_controller/params-flow.expected
index 698d3b23ccb6..51eb4d1d95c3 100644
--- a/ruby/ql/test/library-tests/frameworks/action_controller/params-flow.expected
+++ b/ruby/ql/test/library-tests/frameworks/action_controller/params-flow.expected
@@ -116,6 +116,9 @@ edges
 | params_flow.rb:198:5:198:10 | call to params | params_flow.rb:198:5:198:17 | ...[...] | provenance |  |
 | params_flow.rb:198:5:198:17 | ...[...] | params_flow.rb:198:28:198:28 | [post] a | provenance |  |
 | params_flow.rb:198:28:198:28 | [post] a | params_flow.rb:199:10:199:10 | a | provenance |  |
+| params_flow.rb:204:5:204:10 | call to params | params_flow.rb:204:5:204:17 | ...[...] | provenance |  |
+| params_flow.rb:204:5:204:17 | ...[...] | params_flow.rb:204:28:204:28 | [post] a | provenance |  |
+| params_flow.rb:204:28:204:28 | [post] a | params_flow.rb:205:10:205:10 | a | provenance |  |
 nodes
 | filter_flow.rb:14:5:14:8 | [post] self [@foo] | semmle.label | [post] self [@foo] |
 | filter_flow.rb:14:12:14:17 | call to params | semmle.label | call to params |
@@ -279,6 +282,10 @@ nodes
 | params_flow.rb:198:5:198:17 | ...[...] | semmle.label | ...[...] |
 | params_flow.rb:198:28:198:28 | [post] a | semmle.label | [post] a |
 | params_flow.rb:199:10:199:10 | a | semmle.label | a |
+| params_flow.rb:204:5:204:10 | call to params | semmle.label | call to params |
+| params_flow.rb:204:5:204:17 | ...[...] | semmle.label | ...[...] |
+| params_flow.rb:204:28:204:28 | [post] a | semmle.label | [post] a |
+| params_flow.rb:205:10:205:10 | a | semmle.label | a |
 subpaths
 #select
 | filter_flow.rb:21:10:21:13 | @foo | filter_flow.rb:14:12:14:17 | call to params | filter_flow.rb:21:10:21:13 | @foo | $@ | filter_flow.rb:14:12:14:17 | call to params | call to params |
@@ -338,3 +345,4 @@ subpaths
 | params_flow.rb:190:10:190:44 | call to headers | params_flow.rb:190:10:190:15 | call to params | params_flow.rb:190:10:190:44 | call to headers | $@ | params_flow.rb:190:10:190:15 | call to params | call to params |
 | params_flow.rb:194:10:194:47 | call to read | params_flow.rb:194:10:194:15 | call to params | params_flow.rb:194:10:194:47 | call to read | $@ | params_flow.rb:194:10:194:15 | call to params | call to params |
 | params_flow.rb:199:10:199:10 | a | params_flow.rb:198:5:198:10 | call to params | params_flow.rb:199:10:199:10 | a | $@ | params_flow.rb:198:5:198:10 | call to params | call to params |
+| params_flow.rb:205:10:205:10 | a | params_flow.rb:204:5:204:10 | call to params | params_flow.rb:205:10:205:10 | a | $@ | params_flow.rb:204:5:204:10 | call to params | call to params |
diff --git a/ruby/ql/test/library-tests/frameworks/action_controller/params_flow.rb b/ruby/ql/test/library-tests/frameworks/action_controller/params_flow.rb
index 018d8b58af01..ece3b5515564 100644
--- a/ruby/ql/test/library-tests/frameworks/action_controller/params_flow.rb
+++ b/ruby/ql/test/library-tests/frameworks/action_controller/params_flow.rb
@@ -202,6 +202,6 @@ def m40(a)
   def m41
     a = ""
     params[:file].read(nil,a)
-    sink a # $ MISSING:hasTaintFlow
+    sink a # $ hasTaintFlow
   end
 end
\ No newline at end of file

From 8c5fff2d110a08257e03a0176b0dd4f9b75d7e40 Mon Sep 17 00:00:00 2001
From: Joe Farebrother <joefarebrother@github.com>
Date: Fri, 15 Mar 2024 14:43:29 +0000
Subject: [PATCH 7/7] Update names and qldoc for params taint predicates

---
 .../ruby/frameworks/ActionController.qll      | 20 +++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/ruby/ql/lib/codeql/ruby/frameworks/ActionController.qll b/ruby/ql/lib/codeql/ruby/frameworks/ActionController.qll
index 19dcb82cfd69..3fcb3eda5f8c 100644
--- a/ruby/ql/lib/codeql/ruby/frameworks/ActionController.qll
+++ b/ruby/ql/lib/codeql/ruby/frameworks/ActionController.qll
@@ -506,8 +506,8 @@ private module ParamsSummaries {
       ]
   }
 
-  /** Gets a field of an instance of `ActionController::Parameters` */
-  private DataFlow::LocalSourceNode paramsField() {
+  /** Gets a node that may be tainted from an `ActionController::Parameters` instance, through field accesses and hash/array element reads. */
+  private DataFlow::LocalSourceNode taintFromParamsBase() {
     result =
       [
         paramsInstance(),
@@ -515,16 +515,16 @@ private module ParamsSummaries {
       ]
   }
 
-  private DataFlow::LocalSourceNode paramsFieldType(TypeTracker t) {
+  private DataFlow::LocalSourceNode taintFromParamsType(TypeTracker t) {
     t.start() and
-    result = paramsField()
+    result = taintFromParamsBase()
     or
-    exists(TypeTracker t2 | result = paramsFieldType(t2).track(t2, t))
+    exists(TypeTracker t2 | result = taintFromParamsType(t2).track(t2, t))
   }
 
-  /** Gets a node with a type that can be a field of `ActionController::Parameters` */
-  private DataFlow::LocalSourceNode paramsFieldType() {
-    paramsFieldType(TypeTracker::end()).flowsTo(result)
+  /** Gets a node with a type that may be tainted from an `ActionController::Parameters` instance. */
+  private DataFlow::LocalSourceNode taintFromParamsType() {
+    taintFromParamsType(TypeTracker::end()).flowsTo(result)
   }
 
   /**
@@ -602,7 +602,7 @@ private module ParamsSummaries {
 
       override MethodCall getACall() {
         result =
-          paramsFieldType()
+          taintFromParamsType()
               .getAMethodCall(["original_filename", "content_type", "headers"])
               .asExpr()
               .getExpr() and
@@ -622,7 +622,7 @@ private module ParamsSummaries {
       UploadedFileReadSummary() { this = "ActionDispatch::Http::UploadedFile#read" }
 
       override MethodCall getACall() {
-        result = paramsFieldType().getAMethodCall("read").asExpr().getExpr() and
+        result = taintFromParamsType().getAMethodCall("read").asExpr().getExpr() and
         result.getNumberOfArguments() in [0 .. 2]
       }