From 4d3c62e88adc4d102058a8ad3a9e5971cfc893f3 Mon Sep 17 00:00:00 2001
From: Harry Maclean <hmac@github.com>
Date: Tue, 18 Oct 2022 11:48:42 +1300
Subject: [PATCH 01/12] Ruby: Add basic case string comp barrier guard

This recognises barriers of the form

    case foo
    when "some string literal"
      foo
    end

where the read of `foo` inside the `when` is guarded by the comparison
of `foo` with `"some string literal"`.

We don't yet recognise more complex constructs such as case array
inclusion.
---
 .../codeql/ruby/dataflow/BarrierGuards.qll    | 42 ++++++++++++++++++-
 .../ruby/dataflow/internal/DataFlowPublic.qll | 12 +++---
 .../barrier-guards/barrier-guards.expected    |  1 +
 .../dataflow/barrier-guards/barrier-guards.ql |  3 +-
 .../dataflow/barrier-guards/barrier-guards.rb |  7 ++++
 5 files changed, 56 insertions(+), 9 deletions(-)

diff --git a/ruby/ql/lib/codeql/ruby/dataflow/BarrierGuards.qll b/ruby/ql/lib/codeql/ruby/dataflow/BarrierGuards.qll
index c22aa4ca1cfd..8699e2dc6a41 100644
--- a/ruby/ql/lib/codeql/ruby/dataflow/BarrierGuards.qll
+++ b/ruby/ql/lib/codeql/ruby/dataflow/BarrierGuards.qll
@@ -8,7 +8,7 @@ private import codeql.ruby.dataflow.SSA
 private import codeql.ruby.ast.internal.Constant
 private import codeql.ruby.InclusionTests
 
-private predicate stringConstCompare(CfgNodes::ExprCfgNode g, CfgNode e, boolean branch) {
+private predicate stringConstCompare(CfgNodes::AstCfgNode g, CfgNode e, boolean branch) {
   exists(CfgNodes::ExprNodes::ComparisonOperationCfgNode c |
     c = g and
     exists(CfgNodes::ExprNodes::StringLiteralCfgNode strLitNode |
@@ -72,7 +72,7 @@ deprecated class StringConstCompare extends DataFlow::BarrierGuard,
   }
 }
 
-private predicate stringConstArrayInclusionCall(CfgNodes::ExprCfgNode g, CfgNode e, boolean branch) {
+private predicate stringConstArrayInclusionCall(CfgNodes::AstCfgNode g, CfgNode e, boolean branch) {
   exists(InclusionTest t |
     t.asExpr() = g and
     e = t.getContainedNode().asExpr() and
@@ -132,3 +132,41 @@ deprecated class StringConstArrayInclusionCall extends DataFlow::BarrierGuard,
 
   override predicate checks(CfgNode expr, boolean branch) { expr = checkedNode and branch = true }
 }
+
+/**
+ * A validation of a value by comparing with a constant string via a `case`
+ * expression. For example:
+ *
+ * ```rb
+ * name = params[:user_name]
+ * case name
+ * when "alice"
+ *   User.find_by("username = #{name}")
+ * end
+ * ```
+ */
+class StringConstCaseCompareBarrier extends DataFlow::Node {
+  StringConstCaseCompareBarrier() {
+    this = DataFlow::BarrierGuard<stringConstCaseCompare/3>::getABarrierNode()
+  }
+}
+
+/**
+ * Implements the logic for `StringConstCaseCompareBarrier`.
+ */
+private predicate stringConstCaseCompare(
+  CfgNodes::AstCfgNode guard, CfgNode testedNode, boolean branch
+) {
+  exists(CfgNodes::ExprNodes::CaseExprCfgNode case |
+    case.getValue() = testedNode and
+    exists(
+      CfgNodes::ExprNodes::WhenClauseCfgNode branchNode,
+      CfgNodes::ExprNodes::StringLiteralCfgNode strLitNode
+    |
+      guard = strLitNode and
+      branchNode = case.getBranch(_) and
+      branchNode.getPattern(_) = strLitNode and
+      branch = true
+    )
+  )
+}
diff --git a/ruby/ql/lib/codeql/ruby/dataflow/internal/DataFlowPublic.qll b/ruby/ql/lib/codeql/ruby/dataflow/internal/DataFlowPublic.qll
index 7b56f2e6a935..2bd377c64f25 100644
--- a/ruby/ql/lib/codeql/ruby/dataflow/internal/DataFlowPublic.qll
+++ b/ruby/ql/lib/codeql/ruby/dataflow/internal/DataFlowPublic.qll
@@ -431,7 +431,7 @@ class ContentSet extends TContentSet {
  * For example, the guard `g` might be a call `isSafe(x)` and the expression `e`
  * the argument `x`.
  */
-signature predicate guardChecksSig(CfgNodes::ExprCfgNode g, CfgNode e, boolean branch);
+signature predicate guardChecksSig(CfgNodes::AstCfgNode g, CfgNode e, boolean branch);
 
 /**
  * Provides a set of barrier nodes for a guard that validates an expression.
@@ -441,13 +441,13 @@ signature predicate guardChecksSig(CfgNodes::ExprCfgNode g, CfgNode e, boolean b
  */
 module BarrierGuard<guardChecksSig/3 guardChecks> {
   pragma[nomagic]
-  private predicate guardChecksSsaDef(CfgNodes::ExprCfgNode g, boolean branch, Ssa::Definition def) {
+  private predicate guardChecksSsaDef(CfgNodes::AstCfgNode g, boolean branch, Ssa::Definition def) {
     guardChecks(g, def.getARead(), branch)
   }
 
   pragma[nomagic]
   private predicate guardControlsSsaDef(
-    CfgNodes::ExprCfgNode g, boolean branch, Ssa::Definition def, Node n
+    CfgNodes::AstCfgNode g, boolean branch, Ssa::Definition def, Node n
   ) {
     def.getARead() = n.asExpr() and
     guardControlsBlock(g, n.asExpr().getBasicBlock(), branch)
@@ -455,7 +455,7 @@ module BarrierGuard<guardChecksSig/3 guardChecks> {
 
   /** Gets a node that is safely guarded by the given guard check. */
   Node getABarrierNode() {
-    exists(CfgNodes::ExprCfgNode g, boolean branch, Ssa::Definition def |
+    exists(CfgNodes::AstCfgNode g, boolean branch, Ssa::Definition def |
       guardChecksSsaDef(g, branch, def) and
       guardControlsSsaDef(g, branch, def, result)
     )
@@ -485,8 +485,8 @@ module BarrierGuard<guardChecksSig/3 guardChecks> {
 }
 
 /** Holds if the guard `guard` controls block `bb` upon evaluating to `branch`. */
-private predicate guardControlsBlock(CfgNodes::ExprCfgNode guard, BasicBlock bb, boolean branch) {
-  exists(ConditionBlock conditionBlock, SuccessorTypes::BooleanSuccessor s |
+private predicate guardControlsBlock(CfgNodes::AstCfgNode guard, BasicBlock bb, boolean branch) {
+  exists(ConditionBlock conditionBlock, SuccessorTypes::ConditionalSuccessor s |
     guard = conditionBlock.getLastNode() and
     s.getValue() = branch and
     conditionBlock.controls(bb, s)
diff --git a/ruby/ql/test/library-tests/dataflow/barrier-guards/barrier-guards.expected b/ruby/ql/test/library-tests/dataflow/barrier-guards/barrier-guards.expected
index 783d414dad6b..fd94519a94fd 100644
--- a/ruby/ql/test/library-tests/dataflow/barrier-guards/barrier-guards.expected
+++ b/ruby/ql/test/library-tests/dataflow/barrier-guards/barrier-guards.expected
@@ -20,3 +20,4 @@ newStyleBarrierGuards
 | barrier-guards.rb:71:5:71:7 | foo |
 | barrier-guards.rb:83:5:83:7 | foo |
 | barrier-guards.rb:91:5:91:7 | foo |
+| barrier-guards.rb:108:5:108:7 | foo |
diff --git a/ruby/ql/test/library-tests/dataflow/barrier-guards/barrier-guards.ql b/ruby/ql/test/library-tests/dataflow/barrier-guards/barrier-guards.ql
index 84a962ade358..9b321557fac7 100644
--- a/ruby/ql/test/library-tests/dataflow/barrier-guards/barrier-guards.ql
+++ b/ruby/ql/test/library-tests/dataflow/barrier-guards/barrier-guards.ql
@@ -12,5 +12,6 @@ query predicate oldStyleBarrierGuards(
 
 query predicate newStyleBarrierGuards(DataFlow::Node n) {
   n instanceof StringConstCompareBarrier or
-  n instanceof StringConstArrayInclusionCallBarrier
+  n instanceof StringConstArrayInclusionCallBarrier or
+  n instanceof StringConstCaseCompareBarrier
 }
diff --git a/ruby/ql/test/library-tests/dataflow/barrier-guards/barrier-guards.rb b/ruby/ql/test/library-tests/dataflow/barrier-guards/barrier-guards.rb
index bc9599fd9269..b9a4fc9bf519 100644
--- a/ruby/ql/test/library-tests/dataflow/barrier-guards/barrier-guards.rb
+++ b/ruby/ql/test/library-tests/dataflow/barrier-guards/barrier-guards.rb
@@ -102,3 +102,10 @@
 else
     foo
 end
+
+case foo
+when "foo"
+    foo
+else
+    foo
+end

From d71d533a676b09059f212647148241c3e5e9574f Mon Sep 17 00:00:00 2001
From: Harry Maclean <hmac@github.com>
Date: Tue, 18 Oct 2022 23:01:28 +1300
Subject: [PATCH 02/12] Ruby: Use better names in barrier guard predicates

---
 .../lib/codeql/ruby/dataflow/BarrierGuards.qll   | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/ruby/ql/lib/codeql/ruby/dataflow/BarrierGuards.qll b/ruby/ql/lib/codeql/ruby/dataflow/BarrierGuards.qll
index 8699e2dc6a41..70ff08727a65 100644
--- a/ruby/ql/lib/codeql/ruby/dataflow/BarrierGuards.qll
+++ b/ruby/ql/lib/codeql/ruby/dataflow/BarrierGuards.qll
@@ -8,9 +8,9 @@ private import codeql.ruby.dataflow.SSA
 private import codeql.ruby.ast.internal.Constant
 private import codeql.ruby.InclusionTests
 
-private predicate stringConstCompare(CfgNodes::AstCfgNode g, CfgNode e, boolean branch) {
+private predicate stringConstCompare(CfgNodes::AstCfgNode guard, CfgNode testedNode, boolean branch) {
   exists(CfgNodes::ExprNodes::ComparisonOperationCfgNode c |
-    c = g and
+    c = guard and
     exists(CfgNodes::ExprNodes::StringLiteralCfgNode strLitNode |
       c.getExpr() instanceof EqExpr and branch = true
       or
@@ -18,9 +18,9 @@ private predicate stringConstCompare(CfgNodes::AstCfgNode g, CfgNode e, boolean
       or
       c.getExpr() instanceof NEExpr and branch = false
     |
-      c.getLeftOperand() = strLitNode and c.getRightOperand() = e
+      c.getLeftOperand() = strLitNode and c.getRightOperand() = testedNode
       or
-      c.getLeftOperand() = e and c.getRightOperand() = strLitNode
+      c.getLeftOperand() = testedNode and c.getRightOperand() = strLitNode
     )
   )
 }
@@ -72,10 +72,12 @@ deprecated class StringConstCompare extends DataFlow::BarrierGuard,
   }
 }
 
-private predicate stringConstArrayInclusionCall(CfgNodes::AstCfgNode g, CfgNode e, boolean branch) {
+private predicate stringConstArrayInclusionCall(
+  CfgNodes::AstCfgNode guard, CfgNode testedNode, boolean branch
+) {
   exists(InclusionTest t |
-    t.asExpr() = g and
-    e = t.getContainedNode().asExpr() and
+    t.asExpr() = guard and
+    testedNode = t.getContainedNode().asExpr() and
     branch = t.getPolarity()
   |
     exists(ExprNodes::ArrayLiteralCfgNode arr |

From 39b161dbc6e14dd58fe932fa8a0360d1dd0a212a Mon Sep 17 00:00:00 2001
From: Harry Maclean <hmac@github.com>
Date: Tue, 18 Oct 2022 23:09:55 +1300
Subject: [PATCH 03/12] Ruby: Add more case barrier guard tests

---
 .../barrier-guards/barrier-guards.expected    |  4 ++
 .../dataflow/barrier-guards/barrier-guards.rb | 38 +++++++++++++++++++
 2 files changed, 42 insertions(+)

diff --git a/ruby/ql/test/library-tests/dataflow/barrier-guards/barrier-guards.expected b/ruby/ql/test/library-tests/dataflow/barrier-guards/barrier-guards.expected
index fd94519a94fd..e77ed17e29d5 100644
--- a/ruby/ql/test/library-tests/dataflow/barrier-guards/barrier-guards.expected
+++ b/ruby/ql/test/library-tests/dataflow/barrier-guards/barrier-guards.expected
@@ -21,3 +21,7 @@ newStyleBarrierGuards
 | barrier-guards.rb:83:5:83:7 | foo |
 | barrier-guards.rb:91:5:91:7 | foo |
 | barrier-guards.rb:108:5:108:7 | foo |
+| barrier-guards.rb:115:5:115:7 | foo |
+| barrier-guards.rb:117:5:117:7 | foo |
+| barrier-guards.rb:122:5:122:7 | foo |
+| barrier-guards.rb:124:5:124:7 | foo |
diff --git a/ruby/ql/test/library-tests/dataflow/barrier-guards/barrier-guards.rb b/ruby/ql/test/library-tests/dataflow/barrier-guards/barrier-guards.rb
index b9a4fc9bf519..b368bf363fcc 100644
--- a/ruby/ql/test/library-tests/dataflow/barrier-guards/barrier-guards.rb
+++ b/ruby/ql/test/library-tests/dataflow/barrier-guards/barrier-guards.rb
@@ -109,3 +109,41 @@
 else
     foo
 end
+
+case foo
+when "foo"
+    foo
+when "bar"
+    foo
+end
+
+case foo
+when "foo", "bar"
+    foo
+when "baz", "quux"
+    foo
+else
+    foo
+end
+
+# not recognized as a guard
+case foo
+when *["foo", "bar"]
+    foo
+end
+
+FOOS = ["foo", "bar"]
+
+# not recognized as a guard
+case foo
+when *FOOS
+    foo
+end
+
+foos = ["foo", "bar"]
+
+# not recognized as a guard
+case foo
+when *foos
+    foo
+end
\ No newline at end of file

From fdcff3f4161d21302c351f4000c4b3ec52d48a41 Mon Sep 17 00:00:00 2001
From: Harry Maclean <hmac@github.com>
Date: Tue, 25 Oct 2022 17:27:59 +1300
Subject: [PATCH 04/12] Ruby: Add SplatExprCfgNode

---
 ruby/ql/lib/codeql/ruby/controlflow/CfgNodes.qll | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/ruby/ql/lib/codeql/ruby/controlflow/CfgNodes.qll b/ruby/ql/lib/codeql/ruby/controlflow/CfgNodes.qll
index f7be488d0fde..030bb2cf2458 100644
--- a/ruby/ql/lib/codeql/ruby/controlflow/CfgNodes.qll
+++ b/ruby/ql/lib/codeql/ruby/controlflow/CfgNodes.qll
@@ -234,7 +234,7 @@ module ExprNodes {
     override predicate relevantChild(AstNode n) { none() }
   }
 
-  /** A control-flow node that wraps an `ArrayLiteral` AST expression. */
+  /** A control-flow node that wraps a `Literal` AST expression. */
   class LiteralCfgNode extends ExprCfgNode {
     override string getAPrimaryQlClass() { result = "LiteralCfgNode" }
 
@@ -866,6 +866,17 @@ module ExprNodes {
     final override RelationalOperation getExpr() { result = super.getExpr() }
   }
 
+  /** A control-flow node that wraps a `SplatExpr` AST expression. */
+  class SplatExprCfgNode extends ExprCfgNode {
+    override string getAPrimaryQlClass() { result = "SplatExprCfgNode" }
+
+    SplatExprCfgNode() { e instanceof SplatExpr }
+
+    final override SplatExpr getExpr() { result = super.getExpr() }
+
+    final ExprCfgNode getOperand() { result.getExpr() = e.(SplatExpr).getOperand() }
+  }
+
   /** A control-flow node that wraps an `ElementReference` AST expression. */
   class ElementReferenceCfgNode extends MethodCallCfgNode {
     override string getAPrimaryQlClass() { result = "ElementReferenceCfgNode" }

From ad84199a905d48d84a27c42c7036d1b3f5c89275 Mon Sep 17 00:00:00 2001
From: Harry Maclean <hmac@github.com>
Date: Tue, 25 Oct 2022 17:28:13 +1300
Subject: [PATCH 05/12] Ruby: Recognise more case barrier guards

---
 .../codeql/ruby/dataflow/BarrierGuards.qll    | 35 +++++++++++++----
 .../barrier-guards/barrier-guards.expected    |  4 ++
 .../dataflow/barrier-guards/barrier-guards.rb | 38 +++++++++++++++----
 3 files changed, 63 insertions(+), 14 deletions(-)

diff --git a/ruby/ql/lib/codeql/ruby/dataflow/BarrierGuards.qll b/ruby/ql/lib/codeql/ruby/dataflow/BarrierGuards.qll
index 70ff08727a65..bc6b5efd748a 100644
--- a/ruby/ql/lib/codeql/ruby/dataflow/BarrierGuards.qll
+++ b/ruby/ql/lib/codeql/ruby/dataflow/BarrierGuards.qll
@@ -159,16 +159,37 @@ class StringConstCaseCompareBarrier extends DataFlow::Node {
 private predicate stringConstCaseCompare(
   CfgNodes::AstCfgNode guard, CfgNode testedNode, boolean branch
 ) {
+  branch = true and
   exists(CfgNodes::ExprNodes::CaseExprCfgNode case |
     case.getValue() = testedNode and
-    exists(
-      CfgNodes::ExprNodes::WhenClauseCfgNode branchNode,
-      CfgNodes::ExprNodes::StringLiteralCfgNode strLitNode
-    |
-      guard = strLitNode and
+    exists(CfgNodes::ExprNodes::WhenClauseCfgNode branchNode |
       branchNode = case.getBranch(_) and
-      branchNode.getPattern(_) = strLitNode and
-      branch = true
+      guard = branchNode.getPattern(_) and
+      // For simplicity, consider patterns that contain only string literals or arrays of string literals
+      forall(ExprCfgNode pattern | pattern = branchNode.getPattern(_) |
+        // when "foo"
+        // when "foo", "bar"
+        pattern instanceof ExprNodes::StringLiteralCfgNode
+        or
+        // array literals behave weirdly in the CFG so we need to drop down to the AST level for this bit
+        // specifically: `SplatExprCfgNode.getOperand()` does not return results for array literals
+        exists(CfgNodes::ExprNodes::SplatExprCfgNode splat | splat = pattern |
+          // when *["foo", "bar"]
+          exists(ArrayLiteral arr |
+            splat.getExpr().getOperand() = arr and
+            forall(Expr elem | elem = arr.getAnElement() | elem instanceof StringLiteral)
+          )
+          or
+          // when *some_var
+          // when *SOME_CONST
+          exists(ExprNodes::ArrayLiteralCfgNode arr |
+            isArrayConstant(splat.getOperand(), arr) and
+            forall(ExprCfgNode elem | elem = arr.getAnArgument() |
+              elem instanceof ExprNodes::StringLiteralCfgNode
+            )
+          )
+        )
+      )
     )
   )
 }
diff --git a/ruby/ql/test/library-tests/dataflow/barrier-guards/barrier-guards.expected b/ruby/ql/test/library-tests/dataflow/barrier-guards/barrier-guards.expected
index e77ed17e29d5..08d6e87948a9 100644
--- a/ruby/ql/test/library-tests/dataflow/barrier-guards/barrier-guards.expected
+++ b/ruby/ql/test/library-tests/dataflow/barrier-guards/barrier-guards.expected
@@ -25,3 +25,7 @@ newStyleBarrierGuards
 | barrier-guards.rb:117:5:117:7 | foo |
 | barrier-guards.rb:122:5:122:7 | foo |
 | barrier-guards.rb:124:5:124:7 | foo |
+| barrier-guards.rb:131:5:131:7 | foo |
+| barrier-guards.rb:136:5:136:7 | foo |
+| barrier-guards.rb:141:5:141:7 | foo |
+| barrier-guards.rb:146:5:146:7 | foo |
diff --git a/ruby/ql/test/library-tests/dataflow/barrier-guards/barrier-guards.rb b/ruby/ql/test/library-tests/dataflow/barrier-guards/barrier-guards.rb
index b368bf363fcc..f906b0853d39 100644
--- a/ruby/ql/test/library-tests/dataflow/barrier-guards/barrier-guards.rb
+++ b/ruby/ql/test/library-tests/dataflow/barrier-guards/barrier-guards.rb
@@ -126,24 +126,48 @@
     foo
 end
 
-# not recognized as a guard
 case foo
 when *["foo", "bar"]
     foo
 end
 
-FOOS = ["foo", "bar"]
-
-# not recognized as a guard
 case foo
-when *FOOS
+when *%w[foo bar]
     foo
 end
 
-foos = ["foo", "bar"]
+case foo
+when *FOO
+    foo
+end
 
-# not recognized as a guard
 case foo
 when *foos
     foo
+end
+
+x = nil
+
+case foo
+when *["foo", x]
+    foo
+end
+
+case foo
+when "foo", x
+    foo
+end
+
+foos_and_x = ["foo", x]
+
+case foo
+when *foos_and_x
+    foo
+end
+
+FOOS_AND_X = ["foo", x]
+
+case foo
+when *FOOS_AND_X
+    foo
 end
\ No newline at end of file

From 5aa3d48eeeab38ec8a14b9b45b4c8b5b640b494c Mon Sep 17 00:00:00 2001
From: Harry Maclean <hmac@github.com>
Date: Tue, 25 Oct 2022 17:33:38 +1300
Subject: [PATCH 06/12] Ruby: Fix barrier guard type

A previous changed relaxed the signature from ExprCfgNode to AstCfgNode.
---
 .../ruby/security/regexp/PolynomialReDoSCustomizations.qll      | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ruby/ql/lib/codeql/ruby/security/regexp/PolynomialReDoSCustomizations.qll b/ruby/ql/lib/codeql/ruby/security/regexp/PolynomialReDoSCustomizations.qll
index 8c05642fc838..c2f27992ff83 100644
--- a/ruby/ql/lib/codeql/ruby/security/regexp/PolynomialReDoSCustomizations.qll
+++ b/ruby/ql/lib/codeql/ruby/security/regexp/PolynomialReDoSCustomizations.qll
@@ -127,7 +127,7 @@ module PolynomialReDoS {
     override DataFlow::Node getHighlight() { result = matchNode }
   }
 
-  private predicate lengthGuard(CfgNodes::ExprCfgNode g, CfgNode node, boolean branch) {
+  private predicate lengthGuard(CfgNodes::AstCfgNode g, CfgNode node, boolean branch) {
     exists(DataFlow::Node input, DataFlow::CallNode length, DataFlow::ExprNode operand |
       length.asExpr().getExpr().(Ast::MethodCall).getMethodName() = "length" and
       length.getReceiver() = input and

From a5086562c4f5401b8c9ae6446752a0ce226191b4 Mon Sep 17 00:00:00 2001
From: Harry Maclean <hmac@github.com>
Date: Wed, 26 Oct 2022 11:24:34 +1300
Subject: [PATCH 07/12] Ruby: Add placeholder test

---
 .../CommandInjection.expected                 | 38 +++++++++++--------
 .../CommandInjection/CommandInjection.rb      |  9 +++++
 2 files changed, 31 insertions(+), 16 deletions(-)

diff --git a/ruby/ql/test/query-tests/security/cwe-078/CommandInjection/CommandInjection.expected b/ruby/ql/test/query-tests/security/cwe-078/CommandInjection/CommandInjection.expected
index b2e4990daec2..5210938b8873 100644
--- a/ruby/ql/test/query-tests/security/cwe-078/CommandInjection/CommandInjection.expected
+++ b/ruby/ql/test/query-tests/security/cwe-078/CommandInjection/CommandInjection.expected
@@ -10,11 +10,13 @@ edges
 | CommandInjection.rb:6:15:6:26 | ...[...] :  | CommandInjection.rb:34:39:34:51 | "grep #{...}" |
 | CommandInjection.rb:46:15:46:20 | call to params :  | CommandInjection.rb:46:15:46:26 | ...[...] :  |
 | CommandInjection.rb:46:15:46:26 | ...[...] :  | CommandInjection.rb:50:24:50:36 | "echo #{...}" |
-| CommandInjection.rb:64:18:64:23 | number :  | CommandInjection.rb:65:14:65:29 | "echo #{...}" |
-| CommandInjection.rb:72:23:72:33 | blah_number :  | CommandInjection.rb:73:14:73:34 | "echo #{...}" |
-| CommandInjection.rb:81:20:81:25 | **args :  | CommandInjection.rb:82:22:82:25 | args :  |
-| CommandInjection.rb:82:22:82:25 | args :  | CommandInjection.rb:82:22:82:37 | ...[...] :  |
-| CommandInjection.rb:82:22:82:37 | ...[...] :  | CommandInjection.rb:82:14:82:39 | "echo #{...}" |
+| CommandInjection.rb:54:13:54:18 | call to params :  | CommandInjection.rb:54:13:54:24 | ...[...] :  |
+| CommandInjection.rb:54:13:54:24 | ...[...] :  | CommandInjection.rb:57:16:57:18 | cmd |
+| CommandInjection.rb:73:18:73:23 | number :  | CommandInjection.rb:74:14:74:29 | "echo #{...}" |
+| CommandInjection.rb:81:23:81:33 | blah_number :  | CommandInjection.rb:82:14:82:34 | "echo #{...}" |
+| CommandInjection.rb:90:20:90:25 | **args :  | CommandInjection.rb:91:22:91:25 | args :  |
+| CommandInjection.rb:91:22:91:25 | args :  | CommandInjection.rb:91:22:91:37 | ...[...] :  |
+| CommandInjection.rb:91:22:91:37 | ...[...] :  | CommandInjection.rb:91:14:91:39 | "echo #{...}" |
 nodes
 | CommandInjection.rb:6:15:6:20 | call to params :  | semmle.label | call to params :  |
 | CommandInjection.rb:6:15:6:26 | ...[...] :  | semmle.label | ...[...] :  |
@@ -29,14 +31,17 @@ nodes
 | CommandInjection.rb:46:15:46:20 | call to params :  | semmle.label | call to params :  |
 | CommandInjection.rb:46:15:46:26 | ...[...] :  | semmle.label | ...[...] :  |
 | CommandInjection.rb:50:24:50:36 | "echo #{...}" | semmle.label | "echo #{...}" |
-| CommandInjection.rb:64:18:64:23 | number :  | semmle.label | number :  |
-| CommandInjection.rb:65:14:65:29 | "echo #{...}" | semmle.label | "echo #{...}" |
-| CommandInjection.rb:72:23:72:33 | blah_number :  | semmle.label | blah_number :  |
-| CommandInjection.rb:73:14:73:34 | "echo #{...}" | semmle.label | "echo #{...}" |
-| CommandInjection.rb:81:20:81:25 | **args :  | semmle.label | **args :  |
-| CommandInjection.rb:82:14:82:39 | "echo #{...}" | semmle.label | "echo #{...}" |
-| CommandInjection.rb:82:22:82:25 | args :  | semmle.label | args :  |
-| CommandInjection.rb:82:22:82:37 | ...[...] :  | semmle.label | ...[...] :  |
+| CommandInjection.rb:54:13:54:18 | call to params :  | semmle.label | call to params :  |
+| CommandInjection.rb:54:13:54:24 | ...[...] :  | semmle.label | ...[...] :  |
+| CommandInjection.rb:57:16:57:18 | cmd | semmle.label | cmd |
+| CommandInjection.rb:73:18:73:23 | number :  | semmle.label | number :  |
+| CommandInjection.rb:74:14:74:29 | "echo #{...}" | semmle.label | "echo #{...}" |
+| CommandInjection.rb:81:23:81:33 | blah_number :  | semmle.label | blah_number :  |
+| CommandInjection.rb:82:14:82:34 | "echo #{...}" | semmle.label | "echo #{...}" |
+| CommandInjection.rb:90:20:90:25 | **args :  | semmle.label | **args :  |
+| CommandInjection.rb:91:14:91:39 | "echo #{...}" | semmle.label | "echo #{...}" |
+| CommandInjection.rb:91:22:91:25 | args :  | semmle.label | args :  |
+| CommandInjection.rb:91:22:91:37 | ...[...] :  | semmle.label | ...[...] :  |
 subpaths
 #select
 | CommandInjection.rb:7:10:7:15 | #{...} | CommandInjection.rb:6:15:6:20 | call to params :  | CommandInjection.rb:7:10:7:15 | #{...} | This command depends on a $@. | CommandInjection.rb:6:15:6:20 | call to params | user-provided value |
@@ -48,6 +53,7 @@ subpaths
 | CommandInjection.rb:33:24:33:36 | "echo #{...}" | CommandInjection.rb:6:15:6:20 | call to params :  | CommandInjection.rb:33:24:33:36 | "echo #{...}" | This command depends on a $@. | CommandInjection.rb:6:15:6:20 | call to params | user-provided value |
 | CommandInjection.rb:34:39:34:51 | "grep #{...}" | CommandInjection.rb:6:15:6:20 | call to params :  | CommandInjection.rb:34:39:34:51 | "grep #{...}" | This command depends on a $@. | CommandInjection.rb:6:15:6:20 | call to params | user-provided value |
 | CommandInjection.rb:50:24:50:36 | "echo #{...}" | CommandInjection.rb:46:15:46:20 | call to params :  | CommandInjection.rb:50:24:50:36 | "echo #{...}" | This command depends on a $@. | CommandInjection.rb:46:15:46:20 | call to params | user-provided value |
-| CommandInjection.rb:65:14:65:29 | "echo #{...}" | CommandInjection.rb:64:18:64:23 | number :  | CommandInjection.rb:65:14:65:29 | "echo #{...}" | This command depends on a $@. | CommandInjection.rb:64:18:64:23 | number | user-provided value |
-| CommandInjection.rb:73:14:73:34 | "echo #{...}" | CommandInjection.rb:72:23:72:33 | blah_number :  | CommandInjection.rb:73:14:73:34 | "echo #{...}" | This command depends on a $@. | CommandInjection.rb:72:23:72:33 | blah_number | user-provided value |
-| CommandInjection.rb:82:14:82:39 | "echo #{...}" | CommandInjection.rb:81:20:81:25 | **args :  | CommandInjection.rb:82:14:82:39 | "echo #{...}" | This command depends on a $@. | CommandInjection.rb:81:20:81:25 | **args | user-provided value |
+| CommandInjection.rb:57:16:57:18 | cmd | CommandInjection.rb:54:13:54:18 | call to params :  | CommandInjection.rb:57:16:57:18 | cmd | This command depends on a $@. | CommandInjection.rb:54:13:54:18 | call to params | user-provided value |
+| CommandInjection.rb:74:14:74:29 | "echo #{...}" | CommandInjection.rb:73:18:73:23 | number :  | CommandInjection.rb:74:14:74:29 | "echo #{...}" | This command depends on a $@. | CommandInjection.rb:73:18:73:23 | number | user-provided value |
+| CommandInjection.rb:82:14:82:34 | "echo #{...}" | CommandInjection.rb:81:23:81:33 | blah_number :  | CommandInjection.rb:82:14:82:34 | "echo #{...}" | This command depends on a $@. | CommandInjection.rb:81:23:81:33 | blah_number | user-provided value |
+| CommandInjection.rb:91:14:91:39 | "echo #{...}" | CommandInjection.rb:90:20:90:25 | **args :  | CommandInjection.rb:91:14:91:39 | "echo #{...}" | This command depends on a $@. | CommandInjection.rb:90:20:90:25 | **args | user-provided value |
diff --git a/ruby/ql/test/query-tests/security/cwe-078/CommandInjection/CommandInjection.rb b/ruby/ql/test/query-tests/security/cwe-078/CommandInjection/CommandInjection.rb
index ed9750128cca..de1a7f48044b 100644
--- a/ruby/ql/test/query-tests/security/cwe-078/CommandInjection/CommandInjection.rb
+++ b/ruby/ql/test/query-tests/security/cwe-078/CommandInjection/CommandInjection.rb
@@ -49,6 +49,15 @@ def index
         end
         Open3.capture2("echo #{cmd}")
     end
+
+    def update
+      cmd = params[:key]
+      case cmd
+      when "foo"
+        system(cmd)
+      end
+      system(foo)
+    end
 end
 
 module Types

From 392b404243e769fef7f120d04966a89befc038d4 Mon Sep 17 00:00:00 2001
From: Harry Maclean <hmac@github.com>
Date: Wed, 26 Oct 2022 11:26:27 +1300
Subject: [PATCH 08/12] Ruby: Add case barrier to command injection query

---
 ruby/ql/lib/codeql/ruby/security/CommandInjectionQuery.qll  | 3 ++-
 .../cwe-078/CommandInjection/CommandInjection.expected      | 6 +++---
 .../security/cwe-078/CommandInjection/CommandInjection.rb   | 2 +-
 3 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/ruby/ql/lib/codeql/ruby/security/CommandInjectionQuery.qll b/ruby/ql/lib/codeql/ruby/security/CommandInjectionQuery.qll
index ed2c133336ff..d1789f92fa61 100644
--- a/ruby/ql/lib/codeql/ruby/security/CommandInjectionQuery.qll
+++ b/ruby/ql/lib/codeql/ruby/security/CommandInjectionQuery.qll
@@ -26,6 +26,7 @@ class Configuration extends TaintTracking::Configuration {
   override predicate isSanitizer(DataFlow::Node node) {
     node instanceof Sanitizer or
     node instanceof StringConstCompareBarrier or
-    node instanceof StringConstArrayInclusionCallBarrier
+    node instanceof StringConstArrayInclusionCallBarrier or
+    node instanceof StringConstCaseCompareBarrier
   }
 }
diff --git a/ruby/ql/test/query-tests/security/cwe-078/CommandInjection/CommandInjection.expected b/ruby/ql/test/query-tests/security/cwe-078/CommandInjection/CommandInjection.expected
index 5210938b8873..750315fc9d9b 100644
--- a/ruby/ql/test/query-tests/security/cwe-078/CommandInjection/CommandInjection.expected
+++ b/ruby/ql/test/query-tests/security/cwe-078/CommandInjection/CommandInjection.expected
@@ -11,7 +11,7 @@ edges
 | CommandInjection.rb:46:15:46:20 | call to params :  | CommandInjection.rb:46:15:46:26 | ...[...] :  |
 | CommandInjection.rb:46:15:46:26 | ...[...] :  | CommandInjection.rb:50:24:50:36 | "echo #{...}" |
 | CommandInjection.rb:54:13:54:18 | call to params :  | CommandInjection.rb:54:13:54:24 | ...[...] :  |
-| CommandInjection.rb:54:13:54:24 | ...[...] :  | CommandInjection.rb:57:16:57:18 | cmd |
+| CommandInjection.rb:54:13:54:24 | ...[...] :  | CommandInjection.rb:59:14:59:16 | cmd |
 | CommandInjection.rb:73:18:73:23 | number :  | CommandInjection.rb:74:14:74:29 | "echo #{...}" |
 | CommandInjection.rb:81:23:81:33 | blah_number :  | CommandInjection.rb:82:14:82:34 | "echo #{...}" |
 | CommandInjection.rb:90:20:90:25 | **args :  | CommandInjection.rb:91:22:91:25 | args :  |
@@ -33,7 +33,7 @@ nodes
 | CommandInjection.rb:50:24:50:36 | "echo #{...}" | semmle.label | "echo #{...}" |
 | CommandInjection.rb:54:13:54:18 | call to params :  | semmle.label | call to params :  |
 | CommandInjection.rb:54:13:54:24 | ...[...] :  | semmle.label | ...[...] :  |
-| CommandInjection.rb:57:16:57:18 | cmd | semmle.label | cmd |
+| CommandInjection.rb:59:14:59:16 | cmd | semmle.label | cmd |
 | CommandInjection.rb:73:18:73:23 | number :  | semmle.label | number :  |
 | CommandInjection.rb:74:14:74:29 | "echo #{...}" | semmle.label | "echo #{...}" |
 | CommandInjection.rb:81:23:81:33 | blah_number :  | semmle.label | blah_number :  |
@@ -53,7 +53,7 @@ subpaths
 | CommandInjection.rb:33:24:33:36 | "echo #{...}" | CommandInjection.rb:6:15:6:20 | call to params :  | CommandInjection.rb:33:24:33:36 | "echo #{...}" | This command depends on a $@. | CommandInjection.rb:6:15:6:20 | call to params | user-provided value |
 | CommandInjection.rb:34:39:34:51 | "grep #{...}" | CommandInjection.rb:6:15:6:20 | call to params :  | CommandInjection.rb:34:39:34:51 | "grep #{...}" | This command depends on a $@. | CommandInjection.rb:6:15:6:20 | call to params | user-provided value |
 | CommandInjection.rb:50:24:50:36 | "echo #{...}" | CommandInjection.rb:46:15:46:20 | call to params :  | CommandInjection.rb:50:24:50:36 | "echo #{...}" | This command depends on a $@. | CommandInjection.rb:46:15:46:20 | call to params | user-provided value |
-| CommandInjection.rb:57:16:57:18 | cmd | CommandInjection.rb:54:13:54:18 | call to params :  | CommandInjection.rb:57:16:57:18 | cmd | This command depends on a $@. | CommandInjection.rb:54:13:54:18 | call to params | user-provided value |
+| CommandInjection.rb:59:14:59:16 | cmd | CommandInjection.rb:54:13:54:18 | call to params :  | CommandInjection.rb:59:14:59:16 | cmd | This command depends on a $@. | CommandInjection.rb:54:13:54:18 | call to params | user-provided value |
 | CommandInjection.rb:74:14:74:29 | "echo #{...}" | CommandInjection.rb:73:18:73:23 | number :  | CommandInjection.rb:74:14:74:29 | "echo #{...}" | This command depends on a $@. | CommandInjection.rb:73:18:73:23 | number | user-provided value |
 | CommandInjection.rb:82:14:82:34 | "echo #{...}" | CommandInjection.rb:81:23:81:33 | blah_number :  | CommandInjection.rb:82:14:82:34 | "echo #{...}" | This command depends on a $@. | CommandInjection.rb:81:23:81:33 | blah_number | user-provided value |
 | CommandInjection.rb:91:14:91:39 | "echo #{...}" | CommandInjection.rb:90:20:90:25 | **args :  | CommandInjection.rb:91:14:91:39 | "echo #{...}" | This command depends on a $@. | CommandInjection.rb:90:20:90:25 | **args | user-provided value |
diff --git a/ruby/ql/test/query-tests/security/cwe-078/CommandInjection/CommandInjection.rb b/ruby/ql/test/query-tests/security/cwe-078/CommandInjection/CommandInjection.rb
index de1a7f48044b..f64c5823d8a8 100644
--- a/ruby/ql/test/query-tests/security/cwe-078/CommandInjection/CommandInjection.rb
+++ b/ruby/ql/test/query-tests/security/cwe-078/CommandInjection/CommandInjection.rb
@@ -56,7 +56,7 @@ def update
       when "foo"
         system(cmd)
       end
-      system(foo)
+      system(cmd)
     end
 end
 

From c25d443d44a68581f48c2ac4bd2ad042cf94e68d Mon Sep 17 00:00:00 2001
From: Harry Maclean <hmac@github.com>
Date: Wed, 26 Oct 2022 11:28:48 +1300
Subject: [PATCH 09/12] Ruby: Merge case barrier with string const barrier

Both these barriers are for comparison with a constant string, so it
seems cleaner to merge them together.
---
 ruby/ql/lib/codeql/ruby/dataflow/BarrierGuards.qll    | 11 ++---------
 .../codeql/ruby/security/CommandInjectionQuery.qll    |  3 +--
 .../dataflow/barrier-guards/barrier-guards.ql         |  3 +--
 3 files changed, 4 insertions(+), 13 deletions(-)

diff --git a/ruby/ql/lib/codeql/ruby/dataflow/BarrierGuards.qll b/ruby/ql/lib/codeql/ruby/dataflow/BarrierGuards.qll
index bc6b5efd748a..ac6138a74f51 100644
--- a/ruby/ql/lib/codeql/ruby/dataflow/BarrierGuards.qll
+++ b/ruby/ql/lib/codeql/ruby/dataflow/BarrierGuards.qll
@@ -23,6 +23,8 @@ private predicate stringConstCompare(CfgNodes::AstCfgNode guard, CfgNode testedN
       c.getLeftOperand() = testedNode and c.getRightOperand() = strLitNode
     )
   )
+  or
+  stringConstCaseCompare(guard, testedNode, branch)
 }
 
 /**
@@ -147,15 +149,6 @@ deprecated class StringConstArrayInclusionCall extends DataFlow::BarrierGuard,
  * end
  * ```
  */
-class StringConstCaseCompareBarrier extends DataFlow::Node {
-  StringConstCaseCompareBarrier() {
-    this = DataFlow::BarrierGuard<stringConstCaseCompare/3>::getABarrierNode()
-  }
-}
-
-/**
- * Implements the logic for `StringConstCaseCompareBarrier`.
- */
 private predicate stringConstCaseCompare(
   CfgNodes::AstCfgNode guard, CfgNode testedNode, boolean branch
 ) {
diff --git a/ruby/ql/lib/codeql/ruby/security/CommandInjectionQuery.qll b/ruby/ql/lib/codeql/ruby/security/CommandInjectionQuery.qll
index d1789f92fa61..ed2c133336ff 100644
--- a/ruby/ql/lib/codeql/ruby/security/CommandInjectionQuery.qll
+++ b/ruby/ql/lib/codeql/ruby/security/CommandInjectionQuery.qll
@@ -26,7 +26,6 @@ class Configuration extends TaintTracking::Configuration {
   override predicate isSanitizer(DataFlow::Node node) {
     node instanceof Sanitizer or
     node instanceof StringConstCompareBarrier or
-    node instanceof StringConstArrayInclusionCallBarrier or
-    node instanceof StringConstCaseCompareBarrier
+    node instanceof StringConstArrayInclusionCallBarrier
   }
 }
diff --git a/ruby/ql/test/library-tests/dataflow/barrier-guards/barrier-guards.ql b/ruby/ql/test/library-tests/dataflow/barrier-guards/barrier-guards.ql
index 9b321557fac7..84a962ade358 100644
--- a/ruby/ql/test/library-tests/dataflow/barrier-guards/barrier-guards.ql
+++ b/ruby/ql/test/library-tests/dataflow/barrier-guards/barrier-guards.ql
@@ -12,6 +12,5 @@ query predicate oldStyleBarrierGuards(
 
 query predicate newStyleBarrierGuards(DataFlow::Node n) {
   n instanceof StringConstCompareBarrier or
-  n instanceof StringConstArrayInclusionCallBarrier or
-  n instanceof StringConstCaseCompareBarrier
+  n instanceof StringConstArrayInclusionCallBarrier
 }

From e392714335549624cdeee3a15d26ba616f6041d6 Mon Sep 17 00:00:00 2001
From: Harry Maclean <hmac@github.com>
Date: Wed, 26 Oct 2022 16:14:58 +1300
Subject: [PATCH 10/12] Ruby: Add custom barrier guard for cases

`when` clauses in case expressions exert control over their
corresponding `then` branches, but this is not easily noticeable in the
CFG. The individual patterns in the clause have conditional successors,
not the `when` node itself.

Specifically, this means that in an expression such as

    case foo
    when "foo", "bar"
      foo
    end

the pattern `"bar"` technically does not control the read of `foo` on
line 3, because that read is also reachable from the previous pattern
`"foo"`.

Instead of considering the patterns in isolation, we instead consider
the entire `when` clause, provided that all patterns have edges to the
`then` node with the same successor.

In practice this means we copy the implementation of
`DataFlow::BarrierGuard` but use a custom predicate in place of
`ConditionBlock.controls`.
---
 .../codeql/ruby/dataflow/BarrierGuards.qll    | 132 +++++++++++++++++-
 .../dataflow/barrier-guards/barrier-guards.rb |   8 +-
 2 files changed, 133 insertions(+), 7 deletions(-)

diff --git a/ruby/ql/lib/codeql/ruby/dataflow/BarrierGuards.qll b/ruby/ql/lib/codeql/ruby/dataflow/BarrierGuards.qll
index ac6138a74f51..f24fa3651b6d 100644
--- a/ruby/ql/lib/codeql/ruby/dataflow/BarrierGuards.qll
+++ b/ruby/ql/lib/codeql/ruby/dataflow/BarrierGuards.qll
@@ -23,8 +23,6 @@ private predicate stringConstCompare(CfgNodes::AstCfgNode guard, CfgNode testedN
       c.getLeftOperand() = testedNode and c.getRightOperand() = strLitNode
     )
   )
-  or
-  stringConstCaseCompare(guard, testedNode, branch)
 }
 
 /**
@@ -42,7 +40,11 @@ private predicate stringConstCompare(CfgNodes::AstCfgNode guard, CfgNode testedN
  */
 class StringConstCompareBarrier extends DataFlow::Node {
   StringConstCompareBarrier() {
-    this = DataFlow::BarrierGuard<stringConstCompare/3>::getABarrierNode()
+    this =
+      [
+        DataFlow::BarrierGuard<stringConstCompare/3>::getABarrierNode(),
+        CaseBarrier::getABarrierNode()
+      ]
   }
 }
 
@@ -186,3 +188,127 @@ private predicate stringConstCaseCompare(
     )
   )
 }
+
+/**
+ * Predicates that define a barrier for string constant comparison inside case
+ * expressions.
+ *
+ * This is a copy of the `DataFlow::BarrierGuard` parameterized module that uses
+ * `whenClauseControls` instead of `ConditionBlock.controls`.
+ */
+module CaseBarrier {
+  private import codeql.ruby.dataflow.internal.SsaImpl as SsaImpl
+  private import codeql.ruby.controlflow.ControlFlowGraph::SuccessorTypes
+
+  /** Holds if the guard `guard` controls block `bb` upon evaluating to `branch`. */
+  private predicate guardControlsBlock(CfgNodes::AstCfgNode guard, BasicBlock bb, boolean branch) {
+    exists(ConditionBlock conditionBlock, SuccessorTypes::ConditionalSuccessor s |
+      guard = conditionBlock.getLastNode() and
+      s.getValue() = branch and
+      whenClauseControls(conditionBlock, bb, s)
+    )
+  }
+
+  /**
+   * Gets an implicit entry definition for a captured variable that
+   * may be guarded, because a call to the capturing callable is guarded.
+   *
+   * This is restricted to calls where the variable is captured inside a
+   * block.
+   */
+  private Ssa::Definition getAMaybeGuardedCapturedDef() {
+    exists(
+      CfgNodes::ExprCfgNode g, boolean branch, CfgNodes::ExprCfgNode testedNode,
+      Ssa::Definition def, CfgNodes::ExprNodes::CallCfgNode call
+    |
+      def.getARead() = testedNode and
+      stringConstCaseCompare(g, testedNode, branch) and
+      SsaImpl::captureFlowIn(call, def, result) and
+      guardControlsBlock(g, call.getBasicBlock(), branch) and
+      result.getBasicBlock().getScope() = call.getExpr().(MethodCall).getBlock()
+    )
+  }
+
+  /**
+   * Holds if all control flow paths reaching `succ` first exit `cb` with
+   * successor type `s`, assuming that the last node of `cb` is a `when` clause.
+   *
+   * ```
+   *   case foo
+   *         |
+   *     +---+
+   *     |
+   *     v
+   *   when  "foo" ---> "bar" ----+
+   *         |  |        |        |
+   * no-match|  |match   |match   |no-match
+   *         |  v        |        |
+   *         |  foo <----+        |
+   *         |                    |
+   *   end <-+--------------------+
+   * ```
+   *
+   * The read of `foo` in the `then` block is not technically controlled by
+   * either `"foo"` or `"bar"` because it can be reached from either of them.
+   * However if we consider the `when` clause as a whole, then it is
+   * controlled because _at least one of the patterns_ must match.
+   *
+   * We determine this by finding a common successor `succ` of each pattern
+   * with the same successor type `s`. We check that all predecessors of
+   * `succ` are patterns in the clause, or are dominated by a pattern in the
+   * clause.
+   */
+  cached
+  private predicate whenClauseImmediatelyControls(
+    ConditionBlock cb, BasicBlock succ, ConditionalSuccessor s
+  ) {
+    exists(ExprNodes::WhenClauseCfgNode when |
+      cb = when.getBasicBlock() and
+      forall(ExprCfgNode pattern | pattern = when.getPattern(_) |
+        succ = pattern.getBasicBlock().getASuccessor(s) and
+        forall(BasicBlock pred |
+          pred = succ.getAPredecessor() and
+          pred != cb
+        |
+          pred = when.getPattern(_).getBasicBlock()
+          or
+          when.getPattern(_).getBasicBlock().dominates(pred)
+        )
+      )
+    )
+  }
+
+  // The predicates below are all identical to their counterparts in
+  // `DataFlow::BarrierGuard`.
+  cached
+  private predicate whenClauseControls(
+    ConditionBlock whenCb, BasicBlock controlled, ConditionalSuccessor s
+  ) {
+    exists(BasicBlock succ | whenClauseImmediatelyControls(whenCb, succ, s) |
+      succ.dominates(controlled)
+    )
+  }
+
+  pragma[nomagic]
+  private predicate guardChecksSsaDef(CfgNodes::AstCfgNode g, boolean branch, Ssa::Definition def) {
+    stringConstCaseCompare(g, def.getARead(), branch)
+  }
+
+  pragma[nomagic]
+  private predicate guardControlsSsaDef(
+    CfgNodes::AstCfgNode g, boolean branch, Ssa::Definition def, DataFlow::Node n
+  ) {
+    def.getARead() = n.asExpr() and
+    guardControlsBlock(g, n.asExpr().getBasicBlock(), branch)
+  }
+
+  /** Gets a node that is safely guarded by the given guard check. */
+  DataFlow::Node getABarrierNode() {
+    exists(CfgNodes::AstCfgNode g, boolean branch, Ssa::Definition def |
+      guardChecksSsaDef(g, branch, def) and
+      guardControlsSsaDef(g, branch, def, result)
+    )
+    or
+    result.asExpr() = getAMaybeGuardedCapturedDef().getARead()
+  }
+}
diff --git a/ruby/ql/test/library-tests/dataflow/barrier-guards/barrier-guards.rb b/ruby/ql/test/library-tests/dataflow/barrier-guards/barrier-guards.rb
index f906b0853d39..62bd74870cd2 100644
--- a/ruby/ql/test/library-tests/dataflow/barrier-guards/barrier-guards.rb
+++ b/ruby/ql/test/library-tests/dataflow/barrier-guards/barrier-guards.rb
@@ -158,16 +158,16 @@
     foo
 end
 
-foos_and_x = ["foo", x]
+foo_and_x = ["foo", x]
 
 case foo
-when *foos_and_x
+when *foo_and_x
     foo
 end
 
-FOOS_AND_X = ["foo", x]
+FOO_AND_X = ["foo", x]
 
 case foo
-when *FOOS_AND_X
+when *FOO_AND_X
     foo
 end
\ No newline at end of file

From b2b4842b9f998dfb58303ab2eefb34d00532143f Mon Sep 17 00:00:00 2001
From: Harry Maclean <hmac@github.com>
Date: Wed, 26 Oct 2022 16:27:08 +1300
Subject: [PATCH 11/12] Ruby: Move when clause special-case to shared code

This means that we will always treat `when` clauses specially when
considering whether one basic block controls another.

Provided the special case is correect, I think this is fine to do. It
allows us to simplify the case barrier guard by using the parameterized
module.
---
 .../codeql/ruby/controlflow/BasicBlocks.qll   |  52 +++++++
 .../codeql/ruby/dataflow/BarrierGuards.qll    | 132 +-----------------
 2 files changed, 55 insertions(+), 129 deletions(-)

diff --git a/ruby/ql/lib/codeql/ruby/controlflow/BasicBlocks.qll b/ruby/ql/lib/codeql/ruby/controlflow/BasicBlocks.qll
index 9ce0bf32fd71..e3e28f5d4669 100644
--- a/ruby/ql/lib/codeql/ruby/controlflow/BasicBlocks.qll
+++ b/ruby/ql/lib/codeql/ruby/controlflow/BasicBlocks.qll
@@ -313,6 +313,58 @@ private module Cached {
   predicate immediatelyControls(ConditionBlock cb, BasicBlock succ, ConditionalSuccessor s) {
     succ = cb.getASuccessor(s) and
     forall(BasicBlock pred | pred = succ.getAPredecessor() and pred != cb | succ.dominates(pred))
+    or
+    // special case for `when` clauses in case expressions
+    whenClauseImmediatelyControls(cb, succ, s)
+  }
+
+  /**
+   * Holds if all control flow paths reaching `succ` first exit `cb` with
+   * successor type `s`, assuming that the last node of `cb` is a `when` clause.
+   *
+   * ```
+   *   case foo
+   *         |
+   *     +---+
+   *     |
+   *     v
+   *   when  "foo" ---> "bar" ----+
+   *         |  |        |        |
+   * no-match|  |match   |match   |no-match
+   *         |  v        |        |
+   *         |  foo <----+        |
+   *         |                    |
+   *   end <-+--------------------+
+   * ```
+   *
+   * The read of `foo` in the `then` block is not technically controlled by
+   * either `"foo"` or `"bar"` because it can be reached from either of them.
+   * However if we consider the `when` clause as a whole, then it is
+   * controlled because _at least one of the patterns_ must match.
+   *
+   * We determine this by finding a common successor `succ` of each pattern
+   * with the same successor type `s`. We check that all predecessors of
+   * `succ` are patterns in the clause, or are dominated by a pattern in the
+   * clause.
+   */
+  cached
+  private predicate whenClauseImmediatelyControls(
+    ConditionBlock cb, BasicBlock succ, ConditionalSuccessor s
+  ) {
+    exists(ExprNodes::WhenClauseCfgNode when |
+      cb = when.getBasicBlock() and
+      forall(ExprCfgNode pattern | pattern = when.getPattern(_) |
+        succ = pattern.getBasicBlock().getASuccessor(s) and
+        forall(BasicBlock pred |
+          pred = succ.getAPredecessor() and
+          pred != cb
+        |
+          pred = when.getPattern(_).getBasicBlock()
+          or
+          when.getPattern(_).getBasicBlock().dominates(pred)
+        )
+      )
+    )
   }
 
   cached
diff --git a/ruby/ql/lib/codeql/ruby/dataflow/BarrierGuards.qll b/ruby/ql/lib/codeql/ruby/dataflow/BarrierGuards.qll
index f24fa3651b6d..ac6138a74f51 100644
--- a/ruby/ql/lib/codeql/ruby/dataflow/BarrierGuards.qll
+++ b/ruby/ql/lib/codeql/ruby/dataflow/BarrierGuards.qll
@@ -23,6 +23,8 @@ private predicate stringConstCompare(CfgNodes::AstCfgNode guard, CfgNode testedN
       c.getLeftOperand() = testedNode and c.getRightOperand() = strLitNode
     )
   )
+  or
+  stringConstCaseCompare(guard, testedNode, branch)
 }
 
 /**
@@ -40,11 +42,7 @@ private predicate stringConstCompare(CfgNodes::AstCfgNode guard, CfgNode testedN
  */
 class StringConstCompareBarrier extends DataFlow::Node {
   StringConstCompareBarrier() {
-    this =
-      [
-        DataFlow::BarrierGuard<stringConstCompare/3>::getABarrierNode(),
-        CaseBarrier::getABarrierNode()
-      ]
+    this = DataFlow::BarrierGuard<stringConstCompare/3>::getABarrierNode()
   }
 }
 
@@ -188,127 +186,3 @@ private predicate stringConstCaseCompare(
     )
   )
 }
-
-/**
- * Predicates that define a barrier for string constant comparison inside case
- * expressions.
- *
- * This is a copy of the `DataFlow::BarrierGuard` parameterized module that uses
- * `whenClauseControls` instead of `ConditionBlock.controls`.
- */
-module CaseBarrier {
-  private import codeql.ruby.dataflow.internal.SsaImpl as SsaImpl
-  private import codeql.ruby.controlflow.ControlFlowGraph::SuccessorTypes
-
-  /** Holds if the guard `guard` controls block `bb` upon evaluating to `branch`. */
-  private predicate guardControlsBlock(CfgNodes::AstCfgNode guard, BasicBlock bb, boolean branch) {
-    exists(ConditionBlock conditionBlock, SuccessorTypes::ConditionalSuccessor s |
-      guard = conditionBlock.getLastNode() and
-      s.getValue() = branch and
-      whenClauseControls(conditionBlock, bb, s)
-    )
-  }
-
-  /**
-   * Gets an implicit entry definition for a captured variable that
-   * may be guarded, because a call to the capturing callable is guarded.
-   *
-   * This is restricted to calls where the variable is captured inside a
-   * block.
-   */
-  private Ssa::Definition getAMaybeGuardedCapturedDef() {
-    exists(
-      CfgNodes::ExprCfgNode g, boolean branch, CfgNodes::ExprCfgNode testedNode,
-      Ssa::Definition def, CfgNodes::ExprNodes::CallCfgNode call
-    |
-      def.getARead() = testedNode and
-      stringConstCaseCompare(g, testedNode, branch) and
-      SsaImpl::captureFlowIn(call, def, result) and
-      guardControlsBlock(g, call.getBasicBlock(), branch) and
-      result.getBasicBlock().getScope() = call.getExpr().(MethodCall).getBlock()
-    )
-  }
-
-  /**
-   * Holds if all control flow paths reaching `succ` first exit `cb` with
-   * successor type `s`, assuming that the last node of `cb` is a `when` clause.
-   *
-   * ```
-   *   case foo
-   *         |
-   *     +---+
-   *     |
-   *     v
-   *   when  "foo" ---> "bar" ----+
-   *         |  |        |        |
-   * no-match|  |match   |match   |no-match
-   *         |  v        |        |
-   *         |  foo <----+        |
-   *         |                    |
-   *   end <-+--------------------+
-   * ```
-   *
-   * The read of `foo` in the `then` block is not technically controlled by
-   * either `"foo"` or `"bar"` because it can be reached from either of them.
-   * However if we consider the `when` clause as a whole, then it is
-   * controlled because _at least one of the patterns_ must match.
-   *
-   * We determine this by finding a common successor `succ` of each pattern
-   * with the same successor type `s`. We check that all predecessors of
-   * `succ` are patterns in the clause, or are dominated by a pattern in the
-   * clause.
-   */
-  cached
-  private predicate whenClauseImmediatelyControls(
-    ConditionBlock cb, BasicBlock succ, ConditionalSuccessor s
-  ) {
-    exists(ExprNodes::WhenClauseCfgNode when |
-      cb = when.getBasicBlock() and
-      forall(ExprCfgNode pattern | pattern = when.getPattern(_) |
-        succ = pattern.getBasicBlock().getASuccessor(s) and
-        forall(BasicBlock pred |
-          pred = succ.getAPredecessor() and
-          pred != cb
-        |
-          pred = when.getPattern(_).getBasicBlock()
-          or
-          when.getPattern(_).getBasicBlock().dominates(pred)
-        )
-      )
-    )
-  }
-
-  // The predicates below are all identical to their counterparts in
-  // `DataFlow::BarrierGuard`.
-  cached
-  private predicate whenClauseControls(
-    ConditionBlock whenCb, BasicBlock controlled, ConditionalSuccessor s
-  ) {
-    exists(BasicBlock succ | whenClauseImmediatelyControls(whenCb, succ, s) |
-      succ.dominates(controlled)
-    )
-  }
-
-  pragma[nomagic]
-  private predicate guardChecksSsaDef(CfgNodes::AstCfgNode g, boolean branch, Ssa::Definition def) {
-    stringConstCaseCompare(g, def.getARead(), branch)
-  }
-
-  pragma[nomagic]
-  private predicate guardControlsSsaDef(
-    CfgNodes::AstCfgNode g, boolean branch, Ssa::Definition def, DataFlow::Node n
-  ) {
-    def.getARead() = n.asExpr() and
-    guardControlsBlock(g, n.asExpr().getBasicBlock(), branch)
-  }
-
-  /** Gets a node that is safely guarded by the given guard check. */
-  DataFlow::Node getABarrierNode() {
-    exists(CfgNodes::AstCfgNode g, boolean branch, Ssa::Definition def |
-      guardChecksSsaDef(g, branch, def) and
-      guardControlsSsaDef(g, branch, def, result)
-    )
-    or
-    result.asExpr() = getAMaybeGuardedCapturedDef().getARead()
-  }
-}

From f7311f0f6141a1b90bb244563b5ca48528263986 Mon Sep 17 00:00:00 2001
From: Harry Maclean <hmac@github.com>
Date: Wed, 26 Oct 2022 16:38:23 +1300
Subject: [PATCH 12/12] Ruby: Add change note

---
 .../change-notes/2022-10-26-case-expression-barrier-guard.md  | 4 ++++
 1 file changed, 4 insertions(+)
 create mode 100644 ruby/ql/lib/change-notes/2022-10-26-case-expression-barrier-guard.md

diff --git a/ruby/ql/lib/change-notes/2022-10-26-case-expression-barrier-guard.md b/ruby/ql/lib/change-notes/2022-10-26-case-expression-barrier-guard.md
new file mode 100644
index 000000000000..6f3df870eed6
--- /dev/null
+++ b/ruby/ql/lib/change-notes/2022-10-26-case-expression-barrier-guard.md
@@ -0,0 +1,4 @@
+---
+ category: minorAnalysis
+---
+ * String literals and arrays of string literals in case expression patterns are now recognised as barrier guards.