From 4bc909644659e26ec6727ce05731cec5376934d0 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/17] Ruby: Add case string comparison barrier guard

This recognises barriers of the form

    STRINGS = ["foo", "bar"]

    case foo
    when "some string literal"
      foo
    when *["other", "strings"]
      foo
    when *STRINGS
      foo
    end

where the reads of `foo` inside each `when` are guarded by the comparison
of `foo` with the string literals.

We don't yet recognise this construct:

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

This is due to a limitation in the shared barrier guard logic.
---
 ...022-10-26-case-expression-barrier-guard.md |  4 ++
 .../lib/codeql/ruby/controlflow/CfgNodes.qll  | 13 +++-
 .../codeql/ruby/dataflow/BarrierGuards.qll    | 72 +++++++++++++++++--
 .../ruby/dataflow/internal/DataFlowPublic.qll | 12 ++--
 .../regexp/PolynomialReDoSCustomizations.qll  |  2 +-
 .../barrier-guards/barrier-guards.expected    | 37 ++++++++++
 .../dataflow/barrier-guards/barrier-guards.rb | 71 ++++++++++++++++++
 .../CommandInjection.expected                 | 50 +++++++------
 .../CommandInjection/CommandInjection.rb      |  9 +++
 9 files changed, 233 insertions(+), 37 deletions(-)
 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.
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" }
diff --git a/ruby/ql/lib/codeql/ruby/dataflow/BarrierGuards.qll b/ruby/ql/lib/codeql/ruby/dataflow/BarrierGuards.qll
index c22aa4ca1cfd..38998850123f 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::ExprCfgNode 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,11 +18,13 @@ private predicate stringConstCompare(CfgNodes::ExprCfgNode 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
     )
   )
+  or
+  stringConstCaseCompare(guard, testedNode, branch)
 }
 
 /**
@@ -72,10 +74,12 @@ deprecated class StringConstCompare extends DataFlow::BarrierGuard,
   }
 }
 
-private predicate stringConstArrayInclusionCall(CfgNodes::ExprCfgNode 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 |
@@ -132,3 +136,57 @@ 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}")
+ * when *["bob", "charlie"]
+ *   User.find_by("username = #{name}")
+ * when "dave", "eve" # this is not yet recognised as a barrier guard
+ *   User.find_by("username = #{name}")
+ * end
+ * ```
+ */
+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 |
+      branchNode = case.getBranch(_) and
+      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/lib/codeql/ruby/dataflow/internal/DataFlowPublic.qll b/ruby/ql/lib/codeql/ruby/dataflow/internal/DataFlowPublic.qll
index 5a6883a5fe47..543e82ce2999 100644
--- a/ruby/ql/lib/codeql/ruby/dataflow/internal/DataFlowPublic.qll
+++ b/ruby/ql/lib/codeql/ruby/dataflow/internal/DataFlowPublic.qll
@@ -434,7 +434,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.
@@ -444,13 +444,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)
@@ -458,7 +458,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)
     )
@@ -488,8 +488,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/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
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 a50542e8df36..07ba2ecb3cb1 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,6 +20,13 @@ 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:126:5:126:7 | foo |
+| barrier-guards.rb:133:5:133:7 | foo |
+| barrier-guards.rb:135:5:135:7 | foo |
+| barrier-guards.rb:149:5:149:7 | foo |
+| barrier-guards.rb:154:5:154:7 | foo |
+| barrier-guards.rb:159:5:159:7 | foo |
+| barrier-guards.rb:164:5:164:7 | foo |
 controls
 | barrier-guards.rb:3:4:3:15 | ... == ... | barrier-guards.rb:4:5:4:7 | foo | true |
 | barrier-guards.rb:3:4:3:15 | ... == ... | barrier-guards.rb:6:5:6:7 | foo | false |
@@ -67,3 +74,33 @@ controls
 | barrier-guards.rb:118:8:118:8 | call to x | barrier-guards.rb:118:4:118:8 | [true] not ... | false |
 | barrier-guards.rb:118:8:118:8 | call to x | barrier-guards.rb:119:5:119:7 | foo | false |
 | barrier-guards.rb:118:8:118:8 | call to x | barrier-guards.rb:121:5:121:8 | bars | true |
+| barrier-guards.rb:125:6:125:10 | "foo" | barrier-guards.rb:126:5:126:7 | foo | match |
+| barrier-guards.rb:125:6:125:10 | "foo" | barrier-guards.rb:128:5:128:7 | foo | no-match |
+| barrier-guards.rb:132:6:132:10 | "foo" | barrier-guards.rb:133:5:133:7 | foo | match |
+| barrier-guards.rb:132:6:132:10 | "foo" | barrier-guards.rb:134:1:135:7 | when ... | no-match |
+| barrier-guards.rb:132:6:132:10 | "foo" | barrier-guards.rb:135:5:135:7 | foo | no-match |
+| barrier-guards.rb:134:6:134:10 | "bar" | barrier-guards.rb:135:5:135:7 | foo | match |
+| barrier-guards.rb:139:6:139:10 | "foo" | barrier-guards.rb:139:14:139:16 | bar | no-match |
+| barrier-guards.rb:139:6:139:10 | "foo" | barrier-guards.rb:141:1:142:7 | when ... | no-match |
+| barrier-guards.rb:139:6:139:10 | "foo" | barrier-guards.rb:141:14:141:17 | quux | no-match |
+| barrier-guards.rb:139:6:139:10 | "foo" | barrier-guards.rb:142:5:142:7 | foo | no-match |
+| barrier-guards.rb:139:6:139:10 | "foo" | barrier-guards.rb:144:5:144:7 | foo | no-match |
+| barrier-guards.rb:139:13:139:17 | "bar" | barrier-guards.rb:141:1:142:7 | when ... | no-match |
+| barrier-guards.rb:139:13:139:17 | "bar" | barrier-guards.rb:141:14:141:17 | quux | no-match |
+| barrier-guards.rb:139:13:139:17 | "bar" | barrier-guards.rb:142:5:142:7 | foo | no-match |
+| barrier-guards.rb:139:13:139:17 | "bar" | barrier-guards.rb:144:5:144:7 | foo | no-match |
+| barrier-guards.rb:141:6:141:10 | "baz" | barrier-guards.rb:141:14:141:17 | quux | no-match |
+| barrier-guards.rb:141:6:141:10 | "baz" | barrier-guards.rb:144:5:144:7 | foo | no-match |
+| barrier-guards.rb:141:13:141:18 | "quux" | barrier-guards.rb:144:5:144:7 | foo | no-match |
+| barrier-guards.rb:148:6:148:20 | * ... | barrier-guards.rb:149:5:149:7 | foo | match |
+| barrier-guards.rb:153:6:153:17 | * ... | barrier-guards.rb:154:5:154:7 | foo | match |
+| barrier-guards.rb:158:6:158:9 | * ... | barrier-guards.rb:159:5:159:7 | foo | match |
+| barrier-guards.rb:163:6:163:10 | * ... | barrier-guards.rb:164:5:164:7 | foo | match |
+| barrier-guards.rb:168:6:168:16 | * ... | barrier-guards.rb:169:5:169:7 | foo | match |
+| barrier-guards.rb:173:6:173:10 | "foo" | barrier-guards.rb:173:13:173:13 | self | no-match |
+| barrier-guards.rb:180:6:180:15 | * ... | barrier-guards.rb:181:5:181:7 | foo | match |
+| barrier-guards.rb:187:6:187:15 | * ... | barrier-guards.rb:188:5:188:7 | foo | match |
+| barrier-guards.rb:191:4:191:15 | ... == ... | barrier-guards.rb:191:4:191:31 | [false] ... or ... | false |
+| barrier-guards.rb:191:4:191:15 | ... == ... | barrier-guards.rb:191:20:191:22 | foo | false |
+| barrier-guards.rb:191:4:191:31 | [true] ... or ... | barrier-guards.rb:192:5:192:7 | foo | true |
+| barrier-guards.rb:191:20:191:31 | ... == ... | barrier-guards.rb:191:4:191:31 | [false] ... or ... | false |
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 5ead7bc04cf2..743152653cfc 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
@@ -120,3 +120,74 @@
 else
     bars
 end
+
+case foo
+when "foo"
+    foo
+else
+    foo
+end
+
+case foo
+when "foo"
+    foo
+when "bar"
+    foo
+end
+
+case foo
+when "foo", "bar" # not recognised
+    foo
+when "baz", "quux" # not recognised
+    foo
+else
+    foo
+end
+
+case foo
+when *["foo", "bar"]
+    foo
+end
+
+case foo
+when *%w[foo bar]
+    foo
+end
+
+case foo
+when *FOO
+    foo
+end
+
+case foo
+when *foos
+    foo
+end
+
+case foo
+when *["foo", x] # not a guard - includes non-constant element `x`
+    foo
+end
+
+case foo
+when "foo", x # not a guard - includes non-constant element `x`
+    foo
+end
+
+foo_and_x = ["foo", x]
+
+case foo
+when *foo_and_x # not a guard - includes non-constant element `x`
+    foo
+end
+
+FOO_AND_X = ["foo", x]
+
+case foo
+when *FOO_AND_X # not a guard - includes non-constant element `x`
+    foo
+end
+
+if foo == "foo" or foo == "bar" # not recognised
+    foo
+end
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 1d00f293dfd7..dda603763e20 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,13 +10,15 @@ 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:94:16:94:21 | call to params :  | CommandInjection.rb:94:16:94:28 | ...[...] :  |
-| CommandInjection.rb:94:16:94:28 | ...[...] :  | CommandInjection.rb:95:16:95:28 | "cat #{...}" |
+| CommandInjection.rb:54:13:54:18 | call to params :  | CommandInjection.rb:54:13:54:24 | ...[...] :  |
+| 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 :  |
+| 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 #{...}" |
+| CommandInjection.rb:103:16:103:21 | call to params :  | CommandInjection.rb:103:16:103:28 | ...[...] :  |
+| CommandInjection.rb:103:16:103:28 | ...[...] :  | CommandInjection.rb:104:16:104:28 | "cat #{...}" |
 nodes
 | CommandInjection.rb:6:15:6:20 | call to params :  | semmle.label | call to params :  |
 | CommandInjection.rb:6:15:6:26 | ...[...] :  | semmle.label | ...[...] :  |
@@ -31,17 +33,20 @@ 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:94:16:94:21 | call to params :  | semmle.label | call to params :  |
-| CommandInjection.rb:94:16:94:28 | ...[...] :  | semmle.label | ...[...] :  |
-| CommandInjection.rb:95:16:95:28 | "cat #{...}" | semmle.label | "cat #{...}" |
+| CommandInjection.rb:54:13:54:18 | call to params :  | semmle.label | call to params :  |
+| CommandInjection.rb:54:13:54:24 | ...[...] :  | semmle.label | ...[...] :  |
+| 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 :  |
+| 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 | ...[...] :  |
+| CommandInjection.rb:103:16:103:21 | call to params :  | semmle.label | call to params :  |
+| CommandInjection.rb:103:16:103:28 | ...[...] :  | semmle.label | ...[...] :  |
+| CommandInjection.rb:104:16:104:28 | "cat #{...}" | semmle.label | "cat #{...}" |
 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 |
@@ -53,7 +58,8 @@ 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:95:16:95:28 | "cat #{...}" | CommandInjection.rb:94:16:94:21 | call to params :  | CommandInjection.rb:95:16:95:28 | "cat #{...}" | This command depends on a $@. | CommandInjection.rb:94:16:94:21 | 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 |
+| CommandInjection.rb:104:16:104:28 | "cat #{...}" | CommandInjection.rb:103:16:103:21 | call to params :  | CommandInjection.rb:104:16:104:28 | "cat #{...}" | This command depends on a $@. | CommandInjection.rb:103:16:103:21 | call to params | 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 75219e2131c8..4be9c95924a0 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(cmd)
+    end
 end
 
 module Types

From 25ceeaf241beb7b739f4ff2924004d5975b7dedf Mon Sep 17 00:00:00 2001
From: Harry Maclean <hmac@github.com>
Date: Tue, 8 Nov 2022 10:00:22 +1300
Subject: [PATCH 02/17] Ruby: Fix SplatExprCfgNode

---
 ruby/ql/lib/codeql/ruby/controlflow/CfgNodes.qll   | 8 ++++++--
 ruby/ql/lib/codeql/ruby/dataflow/BarrierGuards.qll | 9 ++++-----
 2 files changed, 10 insertions(+), 7 deletions(-)

diff --git a/ruby/ql/lib/codeql/ruby/controlflow/CfgNodes.qll b/ruby/ql/lib/codeql/ruby/controlflow/CfgNodes.qll
index 030bb2cf2458..4417891a66a4 100644
--- a/ruby/ql/lib/codeql/ruby/controlflow/CfgNodes.qll
+++ b/ruby/ql/lib/codeql/ruby/controlflow/CfgNodes.qll
@@ -866,15 +866,19 @@ module ExprNodes {
     final override RelationalOperation getExpr() { result = super.getExpr() }
   }
 
+  private class SplatExprChildMapping extends ExprChildMapping, SplatExpr {
+    override predicate relevantChild(AstNode n) { n = this.getOperand() }
+  }
+
   /** A control-flow node that wraps a `SplatExpr` AST expression. */
   class SplatExprCfgNode extends ExprCfgNode {
     override string getAPrimaryQlClass() { result = "SplatExprCfgNode" }
 
-    SplatExprCfgNode() { e instanceof SplatExpr }
+    override SplatExprChildMapping e;
 
     final override SplatExpr getExpr() { result = super.getExpr() }
 
-    final ExprCfgNode getOperand() { result.getExpr() = e.(SplatExpr).getOperand() }
+    final ExprCfgNode getOperand() { e.hasCfgChild(e.getOperand(), this, result) }
   }
 
   /** A control-flow node that wraps an `ElementReference` AST expression. */
diff --git a/ruby/ql/lib/codeql/ruby/dataflow/BarrierGuards.qll b/ruby/ql/lib/codeql/ruby/dataflow/BarrierGuards.qll
index 38998850123f..c3050527b81c 100644
--- a/ruby/ql/lib/codeql/ruby/dataflow/BarrierGuards.qll
+++ b/ruby/ql/lib/codeql/ruby/dataflow/BarrierGuards.qll
@@ -168,13 +168,12 @@ private predicate stringConstCaseCompare(
         // 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)
+          forex(ExprCfgNode elem |
+            elem = splat.getOperand().(ExprNodes::ArrayLiteralCfgNode).getAnArgument()
+          |
+            elem instanceof ExprNodes::StringLiteralCfgNode
           )
           or
           // when *some_var

From 87944a3a75c387dbde989589871cac9a9d126e93 Mon Sep 17 00:00:00 2001
From: Harry Maclean <hmac@github.com>
Date: Tue, 8 Nov 2022 10:11:09 +1300
Subject: [PATCH 03/17] Ruby: Add test for another case guard variant

---
 .../dataflow/barrier-guards/barrier-guards.expected      | 6 ++++++
 .../dataflow/barrier-guards/barrier-guards.rb            | 9 +++++++++
 2 files changed, 15 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 07ba2ecb3cb1..a27236c8fce1 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
@@ -9,6 +9,8 @@ oldStyleBarrierGuards
 | barrier-guards.rb:43:4:43:15 | ... == ... | barrier-guards.rb:45:9:45:11 | foo | barrier-guards.rb:43:4:43:6 | foo | true |
 | barrier-guards.rb:70:4:70:21 | call to include? | barrier-guards.rb:71:5:71:7 | foo | barrier-guards.rb:70:18:70:20 | foo | true |
 | barrier-guards.rb:82:4:82:25 | ... != ... | barrier-guards.rb:83:5:83:7 | foo | barrier-guards.rb:82:15:82:17 | foo | true |
+| barrier-guards.rb:196:6:196:17 | ... == ... | barrier-guards.rb:197:5:197:7 | foo | barrier-guards.rb:196:6:196:8 | foo | true |
+| barrier-guards.rb:201:6:201:17 | ... == ... | barrier-guards.rb:201:24:201:26 | foo | barrier-guards.rb:201:6:201:8 | foo | true |
 newStyleBarrierGuards
 | barrier-guards.rb:4:5:4:7 | foo |
 | barrier-guards.rb:10:5:10:7 | foo |
@@ -27,6 +29,8 @@ newStyleBarrierGuards
 | barrier-guards.rb:154:5:154:7 | foo |
 | barrier-guards.rb:159:5:159:7 | foo |
 | barrier-guards.rb:164:5:164:7 | foo |
+| barrier-guards.rb:197:5:197:7 | foo |
+| barrier-guards.rb:201:24:201:26 | foo |
 controls
 | barrier-guards.rb:3:4:3:15 | ... == ... | barrier-guards.rb:4:5:4:7 | foo | true |
 | barrier-guards.rb:3:4:3:15 | ... == ... | barrier-guards.rb:6:5:6:7 | foo | false |
@@ -104,3 +108,5 @@ controls
 | barrier-guards.rb:191:4:191:15 | ... == ... | barrier-guards.rb:191:20:191:22 | foo | false |
 | barrier-guards.rb:191:4:191:31 | [true] ... or ... | barrier-guards.rb:192:5:192:7 | foo | true |
 | barrier-guards.rb:191:20:191:31 | ... == ... | barrier-guards.rb:191:4:191:31 | [false] ... or ... | false |
+| barrier-guards.rb:196:6:196:17 | ... == ... | barrier-guards.rb:197:5:197:7 | foo | true |
+| barrier-guards.rb:201:6:201:17 | ... == ... | barrier-guards.rb:201:24:201:26 | foo | true |
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 743152653cfc..28364d89532f 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
@@ -191,3 +191,12 @@
 if foo == "foo" or foo == "bar" # not recognised
     foo
 end
+
+case
+when foo == "foo"
+    foo
+end
+
+case
+when foo == "foo" then foo
+end

From 0ab88c2e29277dc5dabe35ad368f3009a39e045b Mon Sep 17 00:00:00 2001
From: Harry Maclean <hmac@github.com>
Date: Tue, 8 Nov 2022 10:21:48 +1300
Subject: [PATCH 04/17] Ruby: Handle simple in clauses in barrier guard

---
 ruby/ql/lib/codeql/ruby/dataflow/BarrierGuards.qll  |  9 +++++++++
 .../dataflow/barrier-guards/barrier-guards.expected | 13 +++++++++++++
 .../dataflow/barrier-guards/barrier-guards.rb       |  9 +++++++++
 3 files changed, 31 insertions(+)

diff --git a/ruby/ql/lib/codeql/ruby/dataflow/BarrierGuards.qll b/ruby/ql/lib/codeql/ruby/dataflow/BarrierGuards.qll
index c3050527b81c..b1d13bca404d 100644
--- a/ruby/ql/lib/codeql/ruby/dataflow/BarrierGuards.qll
+++ b/ruby/ql/lib/codeql/ruby/dataflow/BarrierGuards.qll
@@ -187,5 +187,14 @@ private predicate stringConstCaseCompare(
         )
       )
     )
+    or
+    // in "foo"
+    exists(
+      CfgNodes::ExprNodes::InClauseCfgNode branchNode, ExprNodes::StringLiteralCfgNode pattern
+    |
+      branchNode = case.getBranch(_) and
+      pattern = branchNode.getPattern() and
+      guard = pattern
+    )
   )
 }
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 a27236c8fce1..734fa79510f0 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
@@ -31,6 +31,7 @@ newStyleBarrierGuards
 | barrier-guards.rb:164:5:164:7 | foo |
 | barrier-guards.rb:197:5:197:7 | foo |
 | barrier-guards.rb:201:24:201:26 | foo |
+| barrier-guards.rb:208:5:208:7 | foo |
 controls
 | barrier-guards.rb:3:4:3:15 | ... == ... | barrier-guards.rb:4:5:4:7 | foo | true |
 | barrier-guards.rb:3:4:3:15 | ... == ... | barrier-guards.rb:6:5:6:7 | foo | false |
@@ -110,3 +111,15 @@ controls
 | barrier-guards.rb:191:20:191:31 | ... == ... | barrier-guards.rb:191:4:191:31 | [false] ... or ... | false |
 | barrier-guards.rb:196:6:196:17 | ... == ... | barrier-guards.rb:197:5:197:7 | foo | true |
 | barrier-guards.rb:201:6:201:17 | ... == ... | barrier-guards.rb:201:24:201:26 | foo | true |
+| barrier-guards.rb:201:6:201:17 | ... == ... | barrier-guards.rb:202:1:202:26 | when ... | false |
+| barrier-guards.rb:201:6:201:17 | ... == ... | barrier-guards.rb:202:24:202:26 | foo | false |
+| barrier-guards.rb:201:6:201:17 | ... == ... | barrier-guards.rb:203:1:203:22 | when ... | false |
+| barrier-guards.rb:201:6:201:17 | ... == ... | barrier-guards.rb:203:20:203:22 | foo | false |
+| barrier-guards.rb:202:6:202:17 | ... == ... | barrier-guards.rb:202:24:202:26 | foo | true |
+| barrier-guards.rb:202:6:202:17 | ... == ... | barrier-guards.rb:203:1:203:22 | when ... | false |
+| barrier-guards.rb:202:6:202:17 | ... == ... | barrier-guards.rb:203:20:203:22 | foo | false |
+| barrier-guards.rb:203:6:203:13 | ... == ... | barrier-guards.rb:203:20:203:22 | foo | true |
+| barrier-guards.rb:207:4:207:8 | "foo" | barrier-guards.rb:208:5:208:7 | foo | match |
+| barrier-guards.rb:207:4:207:8 | "foo" | barrier-guards.rb:209:1:210:7 | in ... then ... | no-match |
+| barrier-guards.rb:207:4:207:8 | "foo" | barrier-guards.rb:210:5:210:7 | foo | no-match |
+| barrier-guards.rb:209:4:209:4 | x | barrier-guards.rb:210:5:210:7 | foo | match |
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 28364d89532f..820601376f9f 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
@@ -199,4 +199,13 @@
 
 case
 when foo == "foo" then foo
+when bar == "bar" then foo
+when foo == x then foo
+end
+
+case foo
+in "foo"
+    foo
+in x
+    foo
 end

From f1b63c4df31a453c29ffc60a401db2fd9b9216e1 Mon Sep 17 00:00:00 2001
From: Harry Maclean <hmac@github.com>
Date: Wed, 9 Nov 2022 14:53:18 +1300
Subject: [PATCH 05/17] Ruby: Fix in clause barrier guard

---
 .../codeql/ruby/dataflow/BarrierGuards.qll    | 62 ++++++++++---------
 .../barrier-guards/barrier-guards.expected    |  1 +
 .../dataflow/barrier-guards/barrier-guards.rb |  5 ++
 3 files changed, 38 insertions(+), 30 deletions(-)

diff --git a/ruby/ql/lib/codeql/ruby/dataflow/BarrierGuards.qll b/ruby/ql/lib/codeql/ruby/dataflow/BarrierGuards.qll
index b1d13bca404d..f42ca8155b2c 100644
--- a/ruby/ql/lib/codeql/ruby/dataflow/BarrierGuards.qll
+++ b/ruby/ql/lib/codeql/ruby/dataflow/BarrierGuards.qll
@@ -159,42 +159,44 @@ private predicate stringConstCaseCompare(
   branch = true and
   exists(CfgNodes::ExprNodes::CaseExprCfgNode case |
     case.getValue() = testedNode and
-    exists(CfgNodes::ExprNodes::WhenClauseCfgNode branchNode |
-      branchNode = case.getBranch(_) and
-      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
-        exists(CfgNodes::ExprNodes::SplatExprCfgNode splat | splat = pattern |
-          // when *["foo", "bar"]
-          forex(ExprCfgNode elem |
-            elem = splat.getOperand().(ExprNodes::ArrayLiteralCfgNode).getAnArgument()
-          |
-            elem instanceof ExprNodes::StringLiteralCfgNode
-          )
+    (
+      exists(CfgNodes::ExprNodes::WhenClauseCfgNode branchNode |
+        branchNode = case.getBranch(_) and
+        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
-          // when *some_var
-          // when *SOME_CONST
-          exists(ExprNodes::ArrayLiteralCfgNode arr |
-            isArrayConstant(splat.getOperand(), arr) and
-            forall(ExprCfgNode elem | elem = arr.getAnArgument() |
+          exists(CfgNodes::ExprNodes::SplatExprCfgNode splat | splat = pattern |
+            // when *["foo", "bar"]
+            forex(ExprCfgNode elem |
+              elem = splat.getOperand().(ExprNodes::ArrayLiteralCfgNode).getAnArgument()
+            |
               elem instanceof ExprNodes::StringLiteralCfgNode
             )
+            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
+              )
+            )
           )
         )
       )
-    )
-    or
-    // in "foo"
-    exists(
-      CfgNodes::ExprNodes::InClauseCfgNode branchNode, ExprNodes::StringLiteralCfgNode pattern
-    |
-      branchNode = case.getBranch(_) and
-      pattern = branchNode.getPattern() and
-      guard = pattern
+      or
+      // in "foo"
+      exists(
+        CfgNodes::ExprNodes::InClauseCfgNode branchNode, ExprNodes::StringLiteralCfgNode pattern
+      |
+        branchNode = case.getBranch(_) and
+        pattern = branchNode.getPattern() and
+        guard = pattern
+      )
     )
   )
 }
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 734fa79510f0..addba893c155 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
@@ -123,3 +123,4 @@ controls
 | barrier-guards.rb:207:4:207:8 | "foo" | barrier-guards.rb:209:1:210:7 | in ... then ... | no-match |
 | barrier-guards.rb:207:4:207:8 | "foo" | barrier-guards.rb:210:5:210:7 | foo | no-match |
 | barrier-guards.rb:209:4:209:4 | x | barrier-guards.rb:210:5:210:7 | foo | match |
+| barrier-guards.rb:214:4:214:8 | "foo" | barrier-guards.rb:215:5:215:7 | foo | match |
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 820601376f9f..5b29ce5642e7 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
@@ -209,3 +209,8 @@
 in x
     foo
 end
+
+case bar
+in "foo"
+    foo
+end

From ad7b5ae7edd0224d7897b49a21be7d13f78f5bae Mon Sep 17 00:00:00 2001
From: Harry Maclean <hmac@github.com>
Date: Wed, 9 Nov 2022 16:35:28 +1300
Subject: [PATCH 06/17] Ruby: Add inline barrier guard test

---
 .../barrier-guards/barrier-guards.expected    |  5 +-
 .../dataflow/barrier-guards/barrier-guards.ql | 17 +++++++
 .../dataflow/barrier-guards/barrier-guards.rb | 48 +++++++++----------
 3 files changed, 44 insertions(+), 26 deletions(-)

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 addba893c155..05c515edebf0 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
@@ -1,4 +1,5 @@
-WARNING: Type BarrierGuard has been deprecated and may be removed in future (barrier-guards.ql:9,3-15)
+WARNING: Type BarrierGuard has been deprecated and may be removed in future (barrier-guards.ql:10,3-15)
+failures
 oldStyleBarrierGuards
 | barrier-guards.rb:3:4:3:15 | ... == ... | barrier-guards.rb:4:5:4:7 | foo | barrier-guards.rb:3:4:3:6 | foo | true |
 | barrier-guards.rb:9:4:9:24 | call to include? | barrier-guards.rb:10:5:10:7 | foo | barrier-guards.rb:9:21:9:23 | foo | true |
@@ -82,7 +83,7 @@ controls
 | barrier-guards.rb:125:6:125:10 | "foo" | barrier-guards.rb:126:5:126:7 | foo | match |
 | barrier-guards.rb:125:6:125:10 | "foo" | barrier-guards.rb:128:5:128:7 | foo | no-match |
 | barrier-guards.rb:132:6:132:10 | "foo" | barrier-guards.rb:133:5:133:7 | foo | match |
-| barrier-guards.rb:132:6:132:10 | "foo" | barrier-guards.rb:134:1:135:7 | when ... | no-match |
+| barrier-guards.rb:132:6:132:10 | "foo" | barrier-guards.rb:134:1:135:19 | when ... | no-match |
 | barrier-guards.rb:132:6:132:10 | "foo" | barrier-guards.rb:135:5:135:7 | foo | no-match |
 | barrier-guards.rb:134:6:134:10 | "bar" | barrier-guards.rb:135:5:135:7 | foo | match |
 | barrier-guards.rb:139:6:139:10 | "foo" | barrier-guards.rb:139:14:139:16 | bar | no-match |
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 7998164b6da1..ff2f0bc76f48 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
@@ -4,6 +4,7 @@ import codeql.ruby.controlflow.CfgNodes
 import codeql.ruby.controlflow.ControlFlowGraph
 import codeql.ruby.controlflow.BasicBlocks
 import codeql.ruby.DataFlow
+import TestUtilities.InlineExpectationsTest
 
 query predicate oldStyleBarrierGuards(
   BarrierGuard g, DataFlow::Node guardedNode, ExprCfgNode expr, boolean branch
@@ -22,3 +23,19 @@ query predicate controls(CfgNode condition, BasicBlock bb, SuccessorTypes::Condi
     condition = cb.getLastNode()
   )
 }
+
+class BarrierGuardTest extends InlineExpectationsTest {
+  BarrierGuardTest() { this = "BarrierGuardTest" }
+
+  override string getARelevantTag() { result = "guarded" }
+
+  override predicate hasActualResult(Location location, string element, string tag, string value) {
+    tag = "guarded" and
+    exists(DataFlow::Node n |
+      newStyleBarrierGuards(n) and
+      location = n.getLocation() and
+      element = n.toString() and
+      value = ""
+    )
+  }
+}
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 5b29ce5642e7..926cb4a06c5f 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
@@ -1,13 +1,13 @@
 foo = "foo"
 
 if foo == "foo"
-    foo
+    foo # $ guarded
 else
     foo
 end
 
 if ["foo"].include?(foo)
-    foo
+    foo # $ guarded
 else
     foo
 end
@@ -15,17 +15,17 @@
 if foo != "foo"
     foo
 else
-    foo
+    foo # $ guarded
 end
 
 unless foo == "foo"
     foo
 else
-    foo
+    foo # $ guarded
 end
 
 unless foo != "foo"
-    foo
+    foo # $ guarded
 else
     foo
 end
@@ -35,14 +35,14 @@
 FOO = ["foo"]
 
 if FOO.include?(foo)
-    foo
+    foo # $ guarded
 else
     foo
 end
 
 if foo == "foo"
     capture {
-        foo # guarded
+        foo # $ guarded
     }
 end
 
@@ -68,7 +68,7 @@
 bars = NotAnArray.new
 
 if foos.include?(foo)
-    foo
+    foo # $ guarded
 else
     foo
 end
@@ -80,7 +80,7 @@
 end
 
 if foos.index(foo) != nil
-    foo
+    foo # $ guarded
 else
     foo
 end
@@ -88,7 +88,7 @@
 if foos.index(foo) == nil
     foo
 else
-    foo
+    foo # $ guarded
 end
 
 bars = ["bar"]
@@ -123,22 +123,22 @@
 
 case foo
 when "foo"
-    foo
+    foo # $ guarded
 else
     foo
 end
 
 case foo
 when "foo"
-    foo
+    foo # $ guarded
 when "bar"
-    foo
+    foo # $ guarded
 end
 
 case foo
-when "foo", "bar" # not recognised
+when "foo", "bar" # $ MISSING: guarded
     foo
-when "baz", "quux" # not recognised
+when "baz", "quux" # $ MISSING: guarded
     foo
 else
     foo
@@ -146,22 +146,22 @@
 
 case foo
 when *["foo", "bar"]
-    foo
+    foo # $ guarded
 end
 
 case foo
 when *%w[foo bar]
-    foo
+    foo # $ guarded
 end
 
 case foo
 when *FOO
-    foo
+    foo # $ guarded
 end
 
 case foo
 when *foos
-    foo
+    foo # $ guarded
 end
 
 case foo
@@ -188,24 +188,24 @@
     foo
 end
 
-if foo == "foo" or foo == "bar" # not recognised
-    foo
+if foo == "foo" or foo == "bar"
+    foo # $ MISSING: guarded
 end
 
 case
 when foo == "foo"
-    foo
+    foo # $ guarded
 end
 
 case
-when foo == "foo" then foo
+when foo == "foo" then foo # $ guarded
 when bar == "bar" then foo
 when foo == x then foo
 end
 
 case foo
 in "foo"
-    foo
+    foo # $ guarded
 in x
     foo
 end

From a8b0d298ffd0f14aae5952a7612e1044aa038e71 Mon Sep 17 00:00:00 2001
From: Harry Maclean <hmac@github.com>
Date: Thu, 10 Nov 2022 16:38:09 +1300
Subject: [PATCH 07/17] Ruby: More string comparison guards

Recognise if statements with conditionals made up or logical `and` or
`or` clauses as barrier guards.
---
 .../codeql/ruby/dataflow/BarrierGuards.qll    | 25 ++++++++++
 .../dataflow/barrier-guards/barrier-guards.rb | 46 +++++++++++++++++--
 2 files changed, 66 insertions(+), 5 deletions(-)

diff --git a/ruby/ql/lib/codeql/ruby/dataflow/BarrierGuards.qll b/ruby/ql/lib/codeql/ruby/dataflow/BarrierGuards.qll
index f42ca8155b2c..b2827e532551 100644
--- a/ruby/ql/lib/codeql/ruby/dataflow/BarrierGuards.qll
+++ b/ruby/ql/lib/codeql/ruby/dataflow/BarrierGuards.qll
@@ -25,6 +25,31 @@ private predicate stringConstCompare(CfgNodes::AstCfgNode guard, CfgNode testedN
   )
   or
   stringConstCaseCompare(guard, testedNode, branch)
+  or
+  stringConstCompareOr(guard, testedNode, branch)
+  or
+  stringConstCompareAnd(guard, testedNode, branch)
+}
+
+private predicate stringConstCompareOr(
+  CfgNodes::ExprNodes::BinaryOperationCfgNode guard, CfgNode testedNode, boolean branch
+) {
+  guard.getExpr() instanceof LogicalOrExpr and
+  branch = true and
+  exists(Ssa::Definition def |
+    forall(CfgNode innerGuard | innerGuard = guard.getAnOperand() |
+      stringConstCompare(innerGuard, def.getARead(), branch)
+    ) and
+    testedNode = any(CfgNode node | stringConstCompare(guard.getAnOperand(), node, _))
+  )
+}
+
+private predicate stringConstCompareAnd(
+  CfgNodes::ExprNodes::BinaryOperationCfgNode guard, CfgNode testedNode, boolean branch
+) {
+  guard.getExpr() instanceof LogicalAndExpr and
+  branch = true and
+  stringConstCompare(guard.getAnOperand(), testedNode, branch)
 }
 
 /**
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 926cb4a06c5f..26c5da447988 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
@@ -136,10 +136,10 @@
 end
 
 case foo
-when "foo", "bar" # $ MISSING: guarded
-    foo
-when "baz", "quux" # $ MISSING: guarded
-    foo
+when "foo", "bar"
+    foo # $ MISSING: guarded
+when "baz", "quux"
+    foo # $ MISSING: guarded
 else
     foo
 end
@@ -189,7 +189,43 @@
 end
 
 if foo == "foo" or foo == "bar"
-    foo # $ MISSING: guarded
+    foo # $ guarded
+end
+
+if foo == "foo" or foo == "bar" or foo == "baz"
+    foo # $ guarded
+end
+
+if foo == "foo" or foo == "bar" or foo == x
+    foo
+end
+
+if foo == "foo" or bar == "bar" or foo == "baz"
+    foo
+end
+
+if foo == "foo" and x
+    foo # $ guarded
+end
+
+if x and foo == "foo"
+    foo # $ guarded
+end
+
+if x and y and foo == "foo"
+    foo # $ guarded
+end
+
+if foo == "foo" and foo == "bar" # $ guarded (second `foo` is guarded by the first comparison)
+    foo # $ guarded
+end
+
+if x and y
+    foo
+end
+
+if foo == "foo" and y
+    bar
 end
 
 case

From e25e192ef33b74ae388b1bbc593c1c5963c15ecf Mon Sep 17 00:00:00 2001
From: Harry Maclean <hmac@github.com>
Date: Thu, 10 Nov 2022 18:42:04 +1300
Subject: [PATCH 08/17] Ruby: Change the CFG for while clauses

The `when` node now acts as a join point for patterns in the when
clause, with match/no-match completions. This is similar to how `or`
expressions work.

The result of this is that the `when` clause "controls" the body of the
`when`, which allows us to model barrier guards for multi-pattern when
clauses.

For this code

case x
when 1, 2
  y
end

The old CFG was

x --> when --> 1 --no-match--> 2 ---no-match---> case
                \               \                  ^
                  \               \                |
                   \                --match----+   |
                     \                         |   |
                       \                       |   |
                         ------match---------> y --+

The new CFG is

x --> 1 --no-match--> 2 --no-match--> [no-match] when --no-match--> case
       \               \                                             ^
         \               \                                           |
           \               --match--> [match] when --match--> y -----+
             \                       /
               \                   /
                 -------match-----

i.e. all patterns flow to the `when` node, which is split based on
whether the pattern matched or not. The body of the when clause then has
a single predecessor `[match] when`, which acts as condition block that
controls `y`.
---
 .../lib/codeql/ruby/controlflow/CfgNodes.qll  |  2 +-
 .../ruby/controlflow/internal/Completion.qll  |  8 +-
 .../internal/ControlFlowGraphImpl.qll         | 21 +++--
 .../ruby/controlflow/internal/Splitting.qll   |  7 ++
 .../codeql/ruby/dataflow/BarrierGuards.qll    |  2 +-
 .../controlflow/graph/Cfg.expected            | 76 ++++++++++++-------
 .../dataflow/barrier-guards/barrier-guards.rb |  4 +-
 7 files changed, 79 insertions(+), 41 deletions(-)

diff --git a/ruby/ql/lib/codeql/ruby/controlflow/CfgNodes.qll b/ruby/ql/lib/codeql/ruby/controlflow/CfgNodes.qll
index 4417891a66a4..31898e0a6528 100644
--- a/ruby/ql/lib/codeql/ruby/controlflow/CfgNodes.qll
+++ b/ruby/ql/lib/codeql/ruby/controlflow/CfgNodes.qll
@@ -446,7 +446,7 @@ module ExprNodes {
     final ExprCfgNode getBody() { e.hasCfgChild(e.getBody(), this, result) }
 
     /** Gets the `i`th pattern this `when`-clause. */
-    final ExprCfgNode getPattern(int i) { e.hasCfgChild(e.getPattern(i), this, result) }
+    final ExprCfgNode getPattern(int i) { result.getExpr() = e.getPattern(i) }
   }
 
   /** A control-flow node that wraps a `CasePattern`. */
diff --git a/ruby/ql/lib/codeql/ruby/controlflow/internal/Completion.qll b/ruby/ql/lib/codeql/ruby/controlflow/internal/Completion.qll
index fe4cca24d690..d727b99d6cf8 100644
--- a/ruby/ql/lib/codeql/ruby/controlflow/internal/Completion.qll
+++ b/ruby/ql/lib/codeql/ruby/controlflow/internal/Completion.qll
@@ -233,8 +233,12 @@ private predicate inMatchingContext(AstNode n) {
   or
   exists(CaseExpr c, WhenClause w |
     exists(c.getValue()) and
-    c.getABranch() = w and
-    w.getPattern(_) = n
+    (
+      c.getABranch() = w and
+      w.getPattern(_) = n
+      or
+      w = n
+    )
   )
   or
   n instanceof CasePattern
diff --git a/ruby/ql/lib/codeql/ruby/controlflow/internal/ControlFlowGraphImpl.qll b/ruby/ql/lib/codeql/ruby/controlflow/internal/ControlFlowGraphImpl.qll
index bdd67d2a693f..1ca24d43ab8a 100644
--- a/ruby/ql/lib/codeql/ruby/controlflow/internal/ControlFlowGraphImpl.qll
+++ b/ruby/ql/lib/codeql/ruby/controlflow/internal/ControlFlowGraphImpl.qll
@@ -400,7 +400,7 @@ module Trees {
       c instanceof SimpleCompletion
       or
       exists(int i, WhenTree branch | branch = this.getBranch(i) |
-        last(branch.getLastPattern(), pred, c) and
+        pred = branch and
         first(this.getBranch(i + 1), succ) and
         c.(ConditionalCompletion).getValue() = false
       )
@@ -1397,7 +1397,7 @@ module Trees {
     final override ControlFlowTree getChildElement(int i) { result = this.getMethodName(i) }
   }
 
-  private class WhenTree extends PreOrderTree, WhenClause {
+  private class WhenTree extends ControlFlowTree, WhenClause {
     final override predicate propagatesAbnormal(AstNode child) { child = this.getAPattern() }
 
     final Expr getLastPattern() {
@@ -1407,17 +1407,21 @@ module Trees {
       )
     }
 
+    final override predicate first(AstNode first) { first(this.getPattern(0), first) }
+
     final override predicate last(AstNode last, Completion c) {
-      last(this.getLastPattern(), last, c) and
+      last = this and
       c.(ConditionalCompletion).getValue() = false
       or
-      last(this.getBody(), last, c)
+      last(this.getBody(), last, c) and
+      c instanceof NormalCompletion
     }
 
     final override predicate succ(AstNode pred, AstNode succ, Completion c) {
       pred = this and
-      first(this.getPattern(0), succ) and
-      c instanceof SimpleCompletion
+      c.isValidFor(this) and
+      c.(ConditionalCompletion).getValue() = true and
+      first(this.getBody(), succ)
       or
       exists(int i, Expr p, boolean b |
         p = this.getPattern(i) and
@@ -1425,10 +1429,13 @@ module Trees {
         b = c.(ConditionalCompletion).getValue()
       |
         b = true and
-        first(this.getBody(), succ)
+        succ = this
         or
         b = false and
         first(this.getPattern(i + 1), succ)
+        or
+        not exists(this.getPattern(i + 1)) and
+        succ = this
       )
     }
   }
diff --git a/ruby/ql/lib/codeql/ruby/controlflow/internal/Splitting.qll b/ruby/ql/lib/codeql/ruby/controlflow/internal/Splitting.qll
index 9581b45a95e1..5e24616ac4a3 100644
--- a/ruby/ql/lib/codeql/ruby/controlflow/internal/Splitting.qll
+++ b/ruby/ql/lib/codeql/ruby/controlflow/internal/Splitting.qll
@@ -85,7 +85,14 @@ private module ConditionalCompletionSplitting {
         or
         last(succ.(ConditionalExpr).getBranch(_), pred, c) and
         completion = c
+        or
+        last(succ.(WhenClause).getAPattern(), pred, c) and completion = c
       )
+      or
+      succ(pred, succ, c) and
+      succ(succ, _, completion) and
+      succ instanceof WhenClause and
+      completion = c
     }
 
     override predicate hasEntryScope(CfgScope scope, AstNode succ) { none() }
diff --git a/ruby/ql/lib/codeql/ruby/dataflow/BarrierGuards.qll b/ruby/ql/lib/codeql/ruby/dataflow/BarrierGuards.qll
index b2827e532551..e80495ab2f3b 100644
--- a/ruby/ql/lib/codeql/ruby/dataflow/BarrierGuards.qll
+++ b/ruby/ql/lib/codeql/ruby/dataflow/BarrierGuards.qll
@@ -186,8 +186,8 @@ private predicate stringConstCaseCompare(
     case.getValue() = testedNode and
     (
       exists(CfgNodes::ExprNodes::WhenClauseCfgNode branchNode |
+        guard = branchNode and
         branchNode = case.getBranch(_) and
-        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"
diff --git a/ruby/ql/test/library-tests/controlflow/graph/Cfg.expected b/ruby/ql/test/library-tests/controlflow/graph/Cfg.expected
index 9c26785029ea..69e4cc294deb 100644
--- a/ruby/ql/test/library-tests/controlflow/graph/Cfg.expected
+++ b/ruby/ql/test/library-tests/controlflow/graph/Cfg.expected
@@ -526,17 +526,20 @@ case.rb:
 #-----|  -> exit if_in_case (normal)
 
 #    2| call to x1
-#-----|  -> when ...
+#-----|  -> 1
 
 #    2| self
 #-----|  -> call to x1
 
-#    3| when ...
-#-----|  -> 1
+#    3| [match] when ...
+#-----| match -> self
+
+#    3| [no-match] when ...
+#-----| no-match -> 2
 
 #    3| 1
-#-----| match -> self
-#-----| no-match -> when ...
+#-----| match -> [match] when ...
+#-----| no-match -> [no-match] when ...
 
 #    3| then ...
 #-----|  -> case ...
@@ -569,12 +572,15 @@ case.rb:
 #    3| x2
 #-----|  -> "x2"
 
-#    4| when ...
-#-----|  -> 2
+#    4| [match] when ...
+#-----| match -> self
 
-#    4| 2
+#    4| [no-match] when ...
 #-----| no-match -> case ...
-#-----| match -> self
+
+#    4| 2
+#-----| match -> [match] when ...
+#-----| no-match -> [no-match] when ...
 
 #    4| then ...
 #-----|  -> case ...
@@ -1826,17 +1832,20 @@ cfg.rb:
 #-----|  -> call to puts
 
 #   41| case ...
-#-----|  -> when ...
+#-----|  -> b
 
 #   41| 10
-#-----|  -> when ...
-
-#   42| when ...
 #-----|  -> 1
 
-#   42| 1
+#   42| [match] when ...
 #-----| match -> self
-#-----| no-match -> when ...
+
+#   42| [no-match] when ...
+#-----| no-match -> 2
+
+#   42| 1
+#-----| match -> [match] when ...
+#-----| no-match -> [no-match] when ...
 
 #   42| then ...
 #-----|  -> case ...
@@ -1853,20 +1862,23 @@ cfg.rb:
 #   42| one
 #-----|  -> "one"
 
-#   43| when ...
-#-----|  -> 2
+#   43| [match] when ...
+#-----| match -> self
+
+#   43| [no-match] when ...
+#-----| no-match -> self
 
 #   43| 2
+#-----| match -> [match] when ...
 #-----| no-match -> 3
-#-----| match -> self
 
 #   43| 3
+#-----| match -> [match] when ...
 #-----| no-match -> 4
-#-----| match -> self
 
 #   43| 4
-#-----| match -> self
-#-----| no-match -> self
+#-----| match -> [match] when ...
+#-----| no-match -> [no-match] when ...
 
 #   43| then ...
 #-----|  -> case ...
@@ -1901,15 +1913,19 @@ cfg.rb:
 #   47| case ...
 #-----|  -> chained
 
+#   48| [false] when ...
+#-----| false -> b
+
 #   48| when ...
-#-----|  -> b
+#-----| match -> self
+#-----| no-match -> b
 
 #   48| b
 #-----|  -> 1
 
 #   48| ... == ...
-#-----| true -> self
-#-----| false -> when ...
+#-----| false -> [false] when ...
+#-----| true -> when ...
 
 #   48| 1
 #-----|  -> ... == ...
@@ -1929,15 +1945,19 @@ cfg.rb:
 #   48| one
 #-----|  -> "one"
 
+#   49| [false] when ...
+#-----| false -> case ...
+
 #   49| when ...
-#-----|  -> b
+#-----| no-match -> case ...
+#-----| match -> self
 
 #   49| b
 #-----|  -> 0
 
 #   49| ... == ...
+#-----| true -> when ...
 #-----| false -> b
-#-----| true -> self
 
 #   49| 0
 #-----|  -> ... == ...
@@ -1946,8 +1966,8 @@ cfg.rb:
 #-----|  -> 1
 
 #   49| ... > ...
-#-----| false -> case ...
-#-----| true -> self
+#-----| false -> [false] when ...
+#-----| true -> when ...
 
 #   49| 1
 #-----|  -> ... > ...
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 26c5da447988..6323a5224b63 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
@@ -137,9 +137,9 @@
 
 case foo
 when "foo", "bar"
-    foo # $ MISSING: guarded
+    foo # $ guarded
 when "baz", "quux"
-    foo # $ MISSING: guarded
+    foo # $ guarded
 else
     foo
 end

From 62ea1f0a05fb6166a679a578518d71a8e781390a Mon Sep 17 00:00:00 2001
From: Harry Maclean <hmac@github.com>
Date: Fri, 11 Nov 2022 18:24:20 +1300
Subject: [PATCH 09/17] Ruby: Fix performance of string comparison guard

The `or` case ran extremely slowly before this change. Also exclude
string interpolations from consideration, for correctness, and add some
more tests.
---
 .../codeql/ruby/dataflow/BarrierGuards.qll    | 39 +++++++++++++++----
 .../dataflow/barrier-guards/barrier-guards.rb | 17 ++++++++
 2 files changed, 48 insertions(+), 8 deletions(-)

diff --git a/ruby/ql/lib/codeql/ruby/dataflow/BarrierGuards.qll b/ruby/ql/lib/codeql/ruby/dataflow/BarrierGuards.qll
index e80495ab2f3b..01b42a520bfd 100644
--- a/ruby/ql/lib/codeql/ruby/dataflow/BarrierGuards.qll
+++ b/ruby/ql/lib/codeql/ruby/dataflow/BarrierGuards.qll
@@ -7,12 +7,16 @@ private import codeql.ruby.controlflow.CfgNodes
 private import codeql.ruby.dataflow.SSA
 private import codeql.ruby.ast.internal.Constant
 private import codeql.ruby.InclusionTests
+private import codeql.ruby.ast.internal.Literal
 
 private predicate stringConstCompare(CfgNodes::AstCfgNode guard, CfgNode testedNode, boolean branch) {
   exists(CfgNodes::ExprNodes::ComparisonOperationCfgNode c |
     c = guard and
     exists(CfgNodes::ExprNodes::StringLiteralCfgNode strLitNode |
-      c.getExpr() instanceof EqExpr and branch = true
+      // Only consider strings without any interpolations
+      not exists(StringInterpolationComponent comp | comp = strLitNode.getExpr().getComponent(_)) and
+      c.getExpr() instanceof EqExpr and
+      branch = true
       or
       c.getExpr() instanceof CaseEqExpr and branch = true
       or
@@ -26,24 +30,43 @@ private predicate stringConstCompare(CfgNodes::AstCfgNode guard, CfgNode testedN
   or
   stringConstCaseCompare(guard, testedNode, branch)
   or
-  stringConstCompareOr(guard, testedNode, branch)
+  exists(Ssa::Definition def, CfgNodes::ExprNodes::BinaryOperationCfgNode g |
+    g = guard and
+    stringConstCompareOr(guard, def, branch) and
+    stringConstCompare(g.getLeftOperand(), testedNode, _)
+  )
   or
   stringConstCompareAnd(guard, testedNode, branch)
 }
 
+/**
+ * Holds if `guard` is an `or` expression whose operands are string comparison guards that test the same SSA variable.
+ * `testedNode` is an arbitrary node that is tested by one of the guards.
+ * For example:
+ *
+ * ```rb
+ * x == "foo" or x == "bar"
+ * ```
+ */
 private predicate stringConstCompareOr(
-  CfgNodes::ExprNodes::BinaryOperationCfgNode guard, CfgNode testedNode, boolean branch
+  CfgNodes::ExprNodes::BinaryOperationCfgNode guard, Ssa::Definition def, boolean branch
 ) {
   guard.getExpr() instanceof LogicalOrExpr and
   branch = true and
-  exists(Ssa::Definition def |
-    forall(CfgNode innerGuard | innerGuard = guard.getAnOperand() |
-      stringConstCompare(innerGuard, def.getARead(), branch)
-    ) and
-    testedNode = any(CfgNode node | stringConstCompare(guard.getAnOperand(), node, _))
+  forall(CfgNode innerGuard | innerGuard = guard.getAnOperand() |
+    stringConstCompare(innerGuard, def.getARead(), branch)
   )
 }
 
+/**
+ * Holds if guard is an `and` expression containing a string comparison guard in either operand.
+ * For example:
+ *
+ * ```rb
+ * x == "foo" and other_condition()
+ * other_condition() and x == "foo"
+ * ```
+ */
 private predicate stringConstCompareAnd(
   CfgNodes::ExprNodes::BinaryOperationCfgNode guard, CfgNode testedNode, boolean branch
 ) {
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 6323a5224b63..1946be362754 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
@@ -250,3 +250,20 @@
 in "foo"
     foo
 end
+
+if foo == "#{some_method()}"
+    foo
+end
+
+F = "foo"
+if foo == "#{F}"
+    foo # $ MISSING: guarded
+end
+
+f = "foo"
+if foo == "#{f}"
+    foo # $ MISSING: guarded
+end
+
+foo == "foo" && foo # $ guarded
+foo && foo == "foo"
\ No newline at end of file

From b16cecc8db1c678b4b5d4300ebcec1030fc902bc Mon Sep 17 00:00:00 2001
From: Harry Maclean <hmac@github.com>
Date: Fri, 11 Nov 2022 18:29:07 +1300
Subject: [PATCH 10/17] Ruby: Add missing doc

---
 ruby/ql/lib/codeql/ruby/controlflow/CfgNodes.qll   | 1 +
 ruby/ql/lib/codeql/ruby/dataflow/BarrierGuards.qll | 7 +++----
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/ruby/ql/lib/codeql/ruby/controlflow/CfgNodes.qll b/ruby/ql/lib/codeql/ruby/controlflow/CfgNodes.qll
index 31898e0a6528..b411c766f03e 100644
--- a/ruby/ql/lib/codeql/ruby/controlflow/CfgNodes.qll
+++ b/ruby/ql/lib/codeql/ruby/controlflow/CfgNodes.qll
@@ -878,6 +878,7 @@ module ExprNodes {
 
     final override SplatExpr getExpr() { result = super.getExpr() }
 
+    /** Gets the operand of this splat expression. */
     final ExprCfgNode getOperand() { e.hasCfgChild(e.getOperand(), this, result) }
   }
 
diff --git a/ruby/ql/lib/codeql/ruby/dataflow/BarrierGuards.qll b/ruby/ql/lib/codeql/ruby/dataflow/BarrierGuards.qll
index 01b42a520bfd..16e965f66572 100644
--- a/ruby/ql/lib/codeql/ruby/dataflow/BarrierGuards.qll
+++ b/ruby/ql/lib/codeql/ruby/dataflow/BarrierGuards.qll
@@ -14,7 +14,7 @@ private predicate stringConstCompare(CfgNodes::AstCfgNode guard, CfgNode testedN
     c = guard and
     exists(CfgNodes::ExprNodes::StringLiteralCfgNode strLitNode |
       // Only consider strings without any interpolations
-      not exists(StringInterpolationComponent comp | comp = strLitNode.getExpr().getComponent(_)) and
+      not strLitNode.getExpr().getComponent(_) instanceof StringInterpolationComponent and
       c.getExpr() instanceof EqExpr and
       branch = true
       or
@@ -40,8 +40,7 @@ private predicate stringConstCompare(CfgNodes::AstCfgNode guard, CfgNode testedN
 }
 
 /**
- * Holds if `guard` is an `or` expression whose operands are string comparison guards that test the same SSA variable.
- * `testedNode` is an arbitrary node that is tested by one of the guards.
+ * Holds if `guard` is an `or` expression whose operands are string comparison guards that test `def`.
  * For example:
  *
  * ```rb
@@ -59,7 +58,7 @@ private predicate stringConstCompareOr(
 }
 
 /**
- * Holds if guard is an `and` expression containing a string comparison guard in either operand.
+ * Holds if `guard` is an `and` expression containing a string comparison guard in either operand.
  * For example:
  *
  * ```rb

From 2b4217b8a41536b5b550ba56e4f911743bc976fd Mon Sep 17 00:00:00 2001
From: Harry Maclean <hmac@github.com>
Date: Fri, 11 Nov 2022 18:41:51 +1300
Subject: [PATCH 11/17] Ruby: Update test fixture

---
 .../barrier-guards/barrier-guards.expected    | 217 ++++++++++++++++--
 1 file changed, 195 insertions(+), 22 deletions(-)

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 05c515edebf0..7804824ffb64 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
@@ -10,8 +10,15 @@ oldStyleBarrierGuards
 | barrier-guards.rb:43:4:43:15 | ... == ... | barrier-guards.rb:45:9:45:11 | foo | barrier-guards.rb:43:4:43:6 | foo | true |
 | barrier-guards.rb:70:4:70:21 | call to include? | barrier-guards.rb:71:5:71:7 | foo | barrier-guards.rb:70:18:70:20 | foo | true |
 | barrier-guards.rb:82:4:82:25 | ... != ... | barrier-guards.rb:83:5:83:7 | foo | barrier-guards.rb:82:15:82:17 | foo | true |
-| barrier-guards.rb:196:6:196:17 | ... == ... | barrier-guards.rb:197:5:197:7 | foo | barrier-guards.rb:196:6:196:8 | foo | true |
-| barrier-guards.rb:201:6:201:17 | ... == ... | barrier-guards.rb:201:24:201:26 | foo | barrier-guards.rb:201:6:201:8 | foo | true |
+| barrier-guards.rb:207:4:207:15 | ... == ... | barrier-guards.rb:208:5:208:7 | foo | barrier-guards.rb:207:4:207:6 | foo | true |
+| barrier-guards.rb:211:10:211:21 | ... == ... | barrier-guards.rb:212:5:212:7 | foo | barrier-guards.rb:211:10:211:12 | foo | true |
+| barrier-guards.rb:215:16:215:27 | ... == ... | barrier-guards.rb:216:5:216:7 | foo | barrier-guards.rb:215:16:215:18 | foo | true |
+| barrier-guards.rb:219:4:219:15 | ... == ... | barrier-guards.rb:219:21:219:23 | foo | barrier-guards.rb:219:4:219:6 | foo | true |
+| barrier-guards.rb:219:4:219:15 | ... == ... | barrier-guards.rb:220:5:220:7 | foo | barrier-guards.rb:219:4:219:6 | foo | true |
+| barrier-guards.rb:219:21:219:32 | ... == ... | barrier-guards.rb:220:5:220:7 | foo | barrier-guards.rb:219:21:219:23 | foo | true |
+| barrier-guards.rb:232:6:232:17 | ... == ... | barrier-guards.rb:233:5:233:7 | foo | barrier-guards.rb:232:6:232:8 | foo | true |
+| barrier-guards.rb:237:6:237:17 | ... == ... | barrier-guards.rb:237:24:237:26 | foo | barrier-guards.rb:237:6:237:8 | foo | true |
+| barrier-guards.rb:268:1:268:12 | ... == ... | barrier-guards.rb:268:17:268:19 | foo | barrier-guards.rb:268:1:268:3 | foo | true |
 newStyleBarrierGuards
 | barrier-guards.rb:4:5:4:7 | foo |
 | barrier-guards.rb:10:5:10:7 | foo |
@@ -26,13 +33,23 @@ newStyleBarrierGuards
 | barrier-guards.rb:126:5:126:7 | foo |
 | barrier-guards.rb:133:5:133:7 | foo |
 | barrier-guards.rb:135:5:135:7 | foo |
+| barrier-guards.rb:140:5:140:7 | foo |
+| barrier-guards.rb:142:5:142:7 | foo |
 | barrier-guards.rb:149:5:149:7 | foo |
 | barrier-guards.rb:154:5:154:7 | foo |
 | barrier-guards.rb:159:5:159:7 | foo |
 | barrier-guards.rb:164:5:164:7 | foo |
-| barrier-guards.rb:197:5:197:7 | foo |
-| barrier-guards.rb:201:24:201:26 | foo |
+| barrier-guards.rb:192:5:192:7 | foo |
+| barrier-guards.rb:196:5:196:7 | foo |
 | barrier-guards.rb:208:5:208:7 | foo |
+| barrier-guards.rb:212:5:212:7 | foo |
+| barrier-guards.rb:216:5:216:7 | foo |
+| barrier-guards.rb:219:21:219:23 | foo |
+| barrier-guards.rb:220:5:220:7 | foo |
+| barrier-guards.rb:233:5:233:7 | foo |
+| barrier-guards.rb:237:24:237:26 | foo |
+| barrier-guards.rb:244:5:244:7 | foo |
+| barrier-guards.rb:268:17:268:19 | foo |
 controls
 | barrier-guards.rb:3:4:3:15 | ... == ... | barrier-guards.rb:4:5:4:7 | foo | true |
 | barrier-guards.rb:3:4:3:15 | ... == ... | barrier-guards.rb:6:5:6:7 | foo | false |
@@ -80,48 +97,204 @@ controls
 | barrier-guards.rb:118:8:118:8 | call to x | barrier-guards.rb:118:4:118:8 | [true] not ... | false |
 | barrier-guards.rb:118:8:118:8 | call to x | barrier-guards.rb:119:5:119:7 | foo | false |
 | barrier-guards.rb:118:8:118:8 | call to x | barrier-guards.rb:121:5:121:8 | bars | true |
+| barrier-guards.rb:125:1:126:19 | [match] when ... | barrier-guards.rb:126:5:126:7 | foo | match |
+| barrier-guards.rb:125:1:126:19 | [no-match] when ... | barrier-guards.rb:128:5:128:7 | foo | no-match |
+| barrier-guards.rb:125:6:125:10 | "foo" | barrier-guards.rb:125:1:126:19 | [match] when ... | match |
+| barrier-guards.rb:125:6:125:10 | "foo" | barrier-guards.rb:125:1:126:19 | [no-match] when ... | no-match |
 | barrier-guards.rb:125:6:125:10 | "foo" | barrier-guards.rb:126:5:126:7 | foo | match |
 | barrier-guards.rb:125:6:125:10 | "foo" | barrier-guards.rb:128:5:128:7 | foo | no-match |
+| barrier-guards.rb:132:1:133:19 | [match] when ... | barrier-guards.rb:133:5:133:7 | foo | match |
+| barrier-guards.rb:132:1:133:19 | [no-match] when ... | barrier-guards.rb:134:1:135:19 | [match] when ... | no-match |
+| barrier-guards.rb:132:1:133:19 | [no-match] when ... | barrier-guards.rb:134:1:135:19 | [no-match] when ... | no-match |
+| barrier-guards.rb:132:1:133:19 | [no-match] when ... | barrier-guards.rb:134:7:134:9 | bar | no-match |
+| barrier-guards.rb:132:1:133:19 | [no-match] when ... | barrier-guards.rb:135:5:135:7 | foo | no-match |
+| barrier-guards.rb:132:6:132:10 | "foo" | barrier-guards.rb:132:1:133:19 | [match] when ... | match |
+| barrier-guards.rb:132:6:132:10 | "foo" | barrier-guards.rb:132:1:133:19 | [no-match] when ... | no-match |
 | barrier-guards.rb:132:6:132:10 | "foo" | barrier-guards.rb:133:5:133:7 | foo | match |
-| barrier-guards.rb:132:6:132:10 | "foo" | barrier-guards.rb:134:1:135:19 | when ... | no-match |
+| barrier-guards.rb:132:6:132:10 | "foo" | barrier-guards.rb:134:1:135:19 | [match] when ... | no-match |
+| barrier-guards.rb:132:6:132:10 | "foo" | barrier-guards.rb:134:1:135:19 | [no-match] when ... | no-match |
+| barrier-guards.rb:132:6:132:10 | "foo" | barrier-guards.rb:134:7:134:9 | bar | no-match |
 | barrier-guards.rb:132:6:132:10 | "foo" | barrier-guards.rb:135:5:135:7 | foo | no-match |
+| barrier-guards.rb:134:1:135:19 | [match] when ... | barrier-guards.rb:135:5:135:7 | foo | match |
+| barrier-guards.rb:134:6:134:10 | "bar" | barrier-guards.rb:134:1:135:19 | [match] when ... | match |
+| barrier-guards.rb:134:6:134:10 | "bar" | barrier-guards.rb:134:1:135:19 | [no-match] when ... | no-match |
 | barrier-guards.rb:134:6:134:10 | "bar" | barrier-guards.rb:135:5:135:7 | foo | match |
+| barrier-guards.rb:139:1:140:19 | [match] when ... | barrier-guards.rb:140:5:140:7 | foo | match |
+| barrier-guards.rb:139:1:140:19 | [no-match] when ... | barrier-guards.rb:141:1:142:19 | [match] when ... | no-match |
+| barrier-guards.rb:139:1:140:19 | [no-match] when ... | barrier-guards.rb:141:1:142:19 | [no-match] when ... | no-match |
+| barrier-guards.rb:139:1:140:19 | [no-match] when ... | barrier-guards.rb:141:7:141:9 | baz | no-match |
+| barrier-guards.rb:139:1:140:19 | [no-match] when ... | barrier-guards.rb:141:14:141:17 | quux | no-match |
+| barrier-guards.rb:139:1:140:19 | [no-match] when ... | barrier-guards.rb:142:5:142:7 | foo | no-match |
+| barrier-guards.rb:139:1:140:19 | [no-match] when ... | barrier-guards.rb:144:5:144:7 | foo | no-match |
+| barrier-guards.rb:139:6:139:10 | "foo" | barrier-guards.rb:139:1:140:19 | [no-match] when ... | no-match |
 | barrier-guards.rb:139:6:139:10 | "foo" | barrier-guards.rb:139:14:139:16 | bar | no-match |
-| barrier-guards.rb:139:6:139:10 | "foo" | barrier-guards.rb:141:1:142:7 | when ... | no-match |
+| barrier-guards.rb:139:6:139:10 | "foo" | barrier-guards.rb:141:1:142:19 | [match] when ... | no-match |
+| barrier-guards.rb:139:6:139:10 | "foo" | barrier-guards.rb:141:1:142:19 | [no-match] when ... | no-match |
+| barrier-guards.rb:139:6:139:10 | "foo" | barrier-guards.rb:141:7:141:9 | baz | no-match |
 | barrier-guards.rb:139:6:139:10 | "foo" | barrier-guards.rb:141:14:141:17 | quux | no-match |
 | barrier-guards.rb:139:6:139:10 | "foo" | barrier-guards.rb:142:5:142:7 | foo | no-match |
 | barrier-guards.rb:139:6:139:10 | "foo" | barrier-guards.rb:144:5:144:7 | foo | no-match |
-| barrier-guards.rb:139:13:139:17 | "bar" | barrier-guards.rb:141:1:142:7 | when ... | no-match |
+| barrier-guards.rb:139:13:139:17 | "bar" | barrier-guards.rb:139:1:140:19 | [no-match] when ... | no-match |
+| barrier-guards.rb:139:13:139:17 | "bar" | barrier-guards.rb:141:1:142:19 | [match] when ... | no-match |
+| barrier-guards.rb:139:13:139:17 | "bar" | barrier-guards.rb:141:1:142:19 | [no-match] when ... | no-match |
+| barrier-guards.rb:139:13:139:17 | "bar" | barrier-guards.rb:141:7:141:9 | baz | no-match |
 | barrier-guards.rb:139:13:139:17 | "bar" | barrier-guards.rb:141:14:141:17 | quux | no-match |
 | barrier-guards.rb:139:13:139:17 | "bar" | barrier-guards.rb:142:5:142:7 | foo | no-match |
 | barrier-guards.rb:139:13:139:17 | "bar" | barrier-guards.rb:144:5:144:7 | foo | no-match |
+| barrier-guards.rb:141:1:142:19 | [match] when ... | barrier-guards.rb:142:5:142:7 | foo | match |
+| barrier-guards.rb:141:1:142:19 | [no-match] when ... | barrier-guards.rb:144:5:144:7 | foo | no-match |
+| barrier-guards.rb:141:6:141:10 | "baz" | barrier-guards.rb:141:1:142:19 | [no-match] when ... | no-match |
 | barrier-guards.rb:141:6:141:10 | "baz" | barrier-guards.rb:141:14:141:17 | quux | no-match |
 | barrier-guards.rb:141:6:141:10 | "baz" | barrier-guards.rb:144:5:144:7 | foo | no-match |
+| barrier-guards.rb:141:13:141:18 | "quux" | barrier-guards.rb:141:1:142:19 | [no-match] when ... | no-match |
 | barrier-guards.rb:141:13:141:18 | "quux" | barrier-guards.rb:144:5:144:7 | foo | no-match |
+| barrier-guards.rb:148:1:149:19 | [match] when ... | barrier-guards.rb:149:5:149:7 | foo | match |
+| barrier-guards.rb:148:6:148:20 | * ... | barrier-guards.rb:148:1:149:19 | [match] when ... | match |
+| barrier-guards.rb:148:6:148:20 | * ... | barrier-guards.rb:148:1:149:19 | [no-match] when ... | no-match |
 | barrier-guards.rb:148:6:148:20 | * ... | barrier-guards.rb:149:5:149:7 | foo | match |
+| barrier-guards.rb:153:1:154:19 | [match] when ... | barrier-guards.rb:154:5:154:7 | foo | match |
+| barrier-guards.rb:153:6:153:17 | * ... | barrier-guards.rb:153:1:154:19 | [match] when ... | match |
+| barrier-guards.rb:153:6:153:17 | * ... | barrier-guards.rb:153:1:154:19 | [no-match] when ... | no-match |
 | barrier-guards.rb:153:6:153:17 | * ... | barrier-guards.rb:154:5:154:7 | foo | match |
+| barrier-guards.rb:158:1:159:19 | [match] when ... | barrier-guards.rb:159:5:159:7 | foo | match |
+| barrier-guards.rb:158:6:158:9 | * ... | barrier-guards.rb:158:1:159:19 | [match] when ... | match |
+| barrier-guards.rb:158:6:158:9 | * ... | barrier-guards.rb:158:1:159:19 | [no-match] when ... | no-match |
 | barrier-guards.rb:158:6:158:9 | * ... | barrier-guards.rb:159:5:159:7 | foo | match |
+| barrier-guards.rb:163:1:164:19 | [match] when ... | barrier-guards.rb:164:5:164:7 | foo | match |
+| barrier-guards.rb:163:6:163:10 | * ... | barrier-guards.rb:163:1:164:19 | [match] when ... | match |
+| barrier-guards.rb:163:6:163:10 | * ... | barrier-guards.rb:163:1:164:19 | [no-match] when ... | no-match |
 | barrier-guards.rb:163:6:163:10 | * ... | barrier-guards.rb:164:5:164:7 | foo | match |
+| barrier-guards.rb:168:1:169:7 | [match] when ... | barrier-guards.rb:169:5:169:7 | foo | match |
+| barrier-guards.rb:168:6:168:16 | * ... | barrier-guards.rb:168:1:169:7 | [match] when ... | match |
+| barrier-guards.rb:168:6:168:16 | * ... | barrier-guards.rb:168:1:169:7 | [no-match] when ... | no-match |
 | barrier-guards.rb:168:6:168:16 | * ... | barrier-guards.rb:169:5:169:7 | foo | match |
+| barrier-guards.rb:173:1:174:7 | [match] when ... | barrier-guards.rb:174:5:174:7 | foo | match |
+| barrier-guards.rb:173:6:173:10 | "foo" | barrier-guards.rb:173:1:174:7 | [no-match] when ... | no-match |
 | barrier-guards.rb:173:6:173:10 | "foo" | barrier-guards.rb:173:13:173:13 | self | no-match |
+| barrier-guards.rb:173:13:173:13 | call to x | barrier-guards.rb:173:1:174:7 | [no-match] when ... | no-match |
+| barrier-guards.rb:180:1:181:7 | [match] when ... | barrier-guards.rb:181:5:181:7 | foo | match |
+| barrier-guards.rb:180:6:180:15 | * ... | barrier-guards.rb:180:1:181:7 | [match] when ... | match |
+| barrier-guards.rb:180:6:180:15 | * ... | barrier-guards.rb:180:1:181:7 | [no-match] when ... | no-match |
 | barrier-guards.rb:180:6:180:15 | * ... | barrier-guards.rb:181:5:181:7 | foo | match |
+| barrier-guards.rb:187:1:188:7 | [match] when ... | barrier-guards.rb:188:5:188:7 | foo | match |
+| barrier-guards.rb:187:6:187:15 | * ... | barrier-guards.rb:187:1:188:7 | [match] when ... | match |
+| barrier-guards.rb:187:6:187:15 | * ... | barrier-guards.rb:187:1:188:7 | [no-match] when ... | no-match |
 | barrier-guards.rb:187:6:187:15 | * ... | barrier-guards.rb:188:5:188:7 | foo | match |
 | barrier-guards.rb:191:4:191:15 | ... == ... | barrier-guards.rb:191:4:191:31 | [false] ... or ... | false |
 | barrier-guards.rb:191:4:191:15 | ... == ... | barrier-guards.rb:191:20:191:22 | foo | false |
 | barrier-guards.rb:191:4:191:31 | [true] ... or ... | barrier-guards.rb:192:5:192:7 | foo | true |
 | barrier-guards.rb:191:20:191:31 | ... == ... | barrier-guards.rb:191:4:191:31 | [false] ... or ... | false |
-| barrier-guards.rb:196:6:196:17 | ... == ... | barrier-guards.rb:197:5:197:7 | foo | true |
-| barrier-guards.rb:201:6:201:17 | ... == ... | barrier-guards.rb:201:24:201:26 | foo | true |
-| barrier-guards.rb:201:6:201:17 | ... == ... | barrier-guards.rb:202:1:202:26 | when ... | false |
-| barrier-guards.rb:201:6:201:17 | ... == ... | barrier-guards.rb:202:24:202:26 | foo | false |
-| barrier-guards.rb:201:6:201:17 | ... == ... | barrier-guards.rb:203:1:203:22 | when ... | false |
-| barrier-guards.rb:201:6:201:17 | ... == ... | barrier-guards.rb:203:20:203:22 | foo | false |
-| barrier-guards.rb:202:6:202:17 | ... == ... | barrier-guards.rb:202:24:202:26 | foo | true |
-| barrier-guards.rb:202:6:202:17 | ... == ... | barrier-guards.rb:203:1:203:22 | when ... | false |
-| barrier-guards.rb:202:6:202:17 | ... == ... | barrier-guards.rb:203:20:203:22 | foo | false |
-| barrier-guards.rb:203:6:203:13 | ... == ... | barrier-guards.rb:203:20:203:22 | foo | true |
-| barrier-guards.rb:207:4:207:8 | "foo" | barrier-guards.rb:208:5:208:7 | foo | match |
-| barrier-guards.rb:207:4:207:8 | "foo" | barrier-guards.rb:209:1:210:7 | in ... then ... | no-match |
-| barrier-guards.rb:207:4:207:8 | "foo" | barrier-guards.rb:210:5:210:7 | foo | no-match |
-| barrier-guards.rb:209:4:209:4 | x | barrier-guards.rb:210:5:210:7 | foo | match |
-| barrier-guards.rb:214:4:214:8 | "foo" | barrier-guards.rb:215:5:215:7 | foo | match |
+| barrier-guards.rb:195:4:195:15 | ... == ... | barrier-guards.rb:195:4:195:31 | [false] ... or ... | false |
+| barrier-guards.rb:195:4:195:15 | ... == ... | barrier-guards.rb:195:4:195:47 | [false] ... or ... | false |
+| barrier-guards.rb:195:4:195:15 | ... == ... | barrier-guards.rb:195:20:195:22 | foo | false |
+| barrier-guards.rb:195:4:195:15 | ... == ... | barrier-guards.rb:195:36:195:38 | foo | false |
+| barrier-guards.rb:195:4:195:31 | [false] ... or ... | barrier-guards.rb:195:4:195:47 | [false] ... or ... | false |
+| barrier-guards.rb:195:4:195:31 | [false] ... or ... | barrier-guards.rb:195:36:195:38 | foo | false |
+| barrier-guards.rb:195:4:195:47 | [true] ... or ... | barrier-guards.rb:196:5:196:7 | foo | true |
+| barrier-guards.rb:195:20:195:31 | ... == ... | barrier-guards.rb:195:4:195:31 | [false] ... or ... | false |
+| barrier-guards.rb:195:20:195:31 | ... == ... | barrier-guards.rb:195:4:195:47 | [false] ... or ... | false |
+| barrier-guards.rb:195:20:195:31 | ... == ... | barrier-guards.rb:195:36:195:38 | foo | false |
+| barrier-guards.rb:195:36:195:47 | ... == ... | barrier-guards.rb:195:4:195:47 | [false] ... or ... | false |
+| barrier-guards.rb:199:4:199:15 | ... == ... | barrier-guards.rb:199:4:199:31 | [false] ... or ... | false |
+| barrier-guards.rb:199:4:199:15 | ... == ... | barrier-guards.rb:199:4:199:43 | [false] ... or ... | false |
+| barrier-guards.rb:199:4:199:15 | ... == ... | barrier-guards.rb:199:20:199:22 | foo | false |
+| barrier-guards.rb:199:4:199:15 | ... == ... | barrier-guards.rb:199:36:199:38 | foo | false |
+| barrier-guards.rb:199:4:199:31 | [false] ... or ... | barrier-guards.rb:199:4:199:43 | [false] ... or ... | false |
+| barrier-guards.rb:199:4:199:31 | [false] ... or ... | barrier-guards.rb:199:36:199:38 | foo | false |
+| barrier-guards.rb:199:4:199:43 | [true] ... or ... | barrier-guards.rb:200:5:200:7 | foo | true |
+| barrier-guards.rb:199:20:199:31 | ... == ... | barrier-guards.rb:199:4:199:31 | [false] ... or ... | false |
+| barrier-guards.rb:199:20:199:31 | ... == ... | barrier-guards.rb:199:4:199:43 | [false] ... or ... | false |
+| barrier-guards.rb:199:20:199:31 | ... == ... | barrier-guards.rb:199:36:199:38 | foo | false |
+| barrier-guards.rb:199:36:199:43 | ... == ... | barrier-guards.rb:199:4:199:43 | [false] ... or ... | false |
+| barrier-guards.rb:203:4:203:15 | ... == ... | barrier-guards.rb:203:4:203:31 | [false] ... or ... | false |
+| barrier-guards.rb:203:4:203:15 | ... == ... | barrier-guards.rb:203:4:203:47 | [false] ... or ... | false |
+| barrier-guards.rb:203:4:203:15 | ... == ... | barrier-guards.rb:203:20:203:22 | self | false |
+| barrier-guards.rb:203:4:203:15 | ... == ... | barrier-guards.rb:203:36:203:38 | foo | false |
+| barrier-guards.rb:203:4:203:31 | [false] ... or ... | barrier-guards.rb:203:4:203:47 | [false] ... or ... | false |
+| barrier-guards.rb:203:4:203:31 | [false] ... or ... | barrier-guards.rb:203:36:203:38 | foo | false |
+| barrier-guards.rb:203:4:203:47 | [true] ... or ... | barrier-guards.rb:204:5:204:7 | foo | true |
+| barrier-guards.rb:203:20:203:31 | ... == ... | barrier-guards.rb:203:4:203:31 | [false] ... or ... | false |
+| barrier-guards.rb:203:20:203:31 | ... == ... | barrier-guards.rb:203:4:203:47 | [false] ... or ... | false |
+| barrier-guards.rb:203:20:203:31 | ... == ... | barrier-guards.rb:203:36:203:38 | foo | false |
+| barrier-guards.rb:203:36:203:47 | ... == ... | barrier-guards.rb:203:4:203:47 | [false] ... or ... | false |
+| barrier-guards.rb:207:4:207:15 | ... == ... | barrier-guards.rb:207:4:207:21 | [true] ... and ... | true |
+| barrier-guards.rb:207:4:207:15 | ... == ... | barrier-guards.rb:207:21:207:21 | self | true |
+| barrier-guards.rb:207:4:207:15 | ... == ... | barrier-guards.rb:208:5:208:7 | foo | true |
+| barrier-guards.rb:207:4:207:21 | [true] ... and ... | barrier-guards.rb:208:5:208:7 | foo | true |
+| barrier-guards.rb:207:21:207:21 | call to x | barrier-guards.rb:207:4:207:21 | [true] ... and ... | true |
+| barrier-guards.rb:207:21:207:21 | call to x | barrier-guards.rb:208:5:208:7 | foo | true |
+| barrier-guards.rb:211:4:211:4 | call to x | barrier-guards.rb:211:4:211:21 | [true] ... and ... | true |
+| barrier-guards.rb:211:4:211:4 | call to x | barrier-guards.rb:211:10:211:12 | foo | true |
+| barrier-guards.rb:211:4:211:4 | call to x | barrier-guards.rb:212:5:212:7 | foo | true |
+| barrier-guards.rb:211:4:211:21 | [true] ... and ... | barrier-guards.rb:212:5:212:7 | foo | true |
+| barrier-guards.rb:211:10:211:21 | ... == ... | barrier-guards.rb:211:4:211:21 | [true] ... and ... | true |
+| barrier-guards.rb:211:10:211:21 | ... == ... | barrier-guards.rb:212:5:212:7 | foo | true |
+| barrier-guards.rb:215:4:215:4 | call to x | barrier-guards.rb:215:4:215:10 | [true] ... and ... | true |
+| barrier-guards.rb:215:4:215:4 | call to x | barrier-guards.rb:215:4:215:27 | [true] ... and ... | true |
+| barrier-guards.rb:215:4:215:4 | call to x | barrier-guards.rb:215:10:215:10 | self | true |
+| barrier-guards.rb:215:4:215:4 | call to x | barrier-guards.rb:215:16:215:18 | foo | true |
+| barrier-guards.rb:215:4:215:4 | call to x | barrier-guards.rb:216:5:216:7 | foo | true |
+| barrier-guards.rb:215:4:215:10 | [true] ... and ... | barrier-guards.rb:215:4:215:27 | [true] ... and ... | true |
+| barrier-guards.rb:215:4:215:10 | [true] ... and ... | barrier-guards.rb:215:16:215:18 | foo | true |
+| barrier-guards.rb:215:4:215:10 | [true] ... and ... | barrier-guards.rb:216:5:216:7 | foo | true |
+| barrier-guards.rb:215:4:215:27 | [true] ... and ... | barrier-guards.rb:216:5:216:7 | foo | true |
+| barrier-guards.rb:215:10:215:10 | call to y | barrier-guards.rb:215:4:215:10 | [true] ... and ... | true |
+| barrier-guards.rb:215:10:215:10 | call to y | barrier-guards.rb:215:4:215:27 | [true] ... and ... | true |
+| barrier-guards.rb:215:10:215:10 | call to y | barrier-guards.rb:215:16:215:18 | foo | true |
+| barrier-guards.rb:215:10:215:10 | call to y | barrier-guards.rb:216:5:216:7 | foo | true |
+| barrier-guards.rb:215:16:215:27 | ... == ... | barrier-guards.rb:215:4:215:27 | [true] ... and ... | true |
+| barrier-guards.rb:215:16:215:27 | ... == ... | barrier-guards.rb:216:5:216:7 | foo | true |
+| barrier-guards.rb:219:4:219:15 | ... == ... | barrier-guards.rb:219:4:219:32 | [true] ... and ... | true |
+| barrier-guards.rb:219:4:219:15 | ... == ... | barrier-guards.rb:219:21:219:23 | foo | true |
+| barrier-guards.rb:219:4:219:15 | ... == ... | barrier-guards.rb:220:5:220:7 | foo | true |
+| barrier-guards.rb:219:4:219:32 | [true] ... and ... | barrier-guards.rb:220:5:220:7 | foo | true |
+| barrier-guards.rb:219:21:219:32 | ... == ... | barrier-guards.rb:219:4:219:32 | [true] ... and ... | true |
+| barrier-guards.rb:219:21:219:32 | ... == ... | barrier-guards.rb:220:5:220:7 | foo | true |
+| barrier-guards.rb:223:4:223:4 | call to x | barrier-guards.rb:223:4:223:10 | [true] ... and ... | true |
+| barrier-guards.rb:223:4:223:4 | call to x | barrier-guards.rb:223:10:223:10 | self | true |
+| barrier-guards.rb:223:4:223:4 | call to x | barrier-guards.rb:224:5:224:7 | foo | true |
+| barrier-guards.rb:223:4:223:10 | [true] ... and ... | barrier-guards.rb:224:5:224:7 | foo | true |
+| barrier-guards.rb:223:10:223:10 | call to y | barrier-guards.rb:223:4:223:10 | [true] ... and ... | true |
+| barrier-guards.rb:223:10:223:10 | call to y | barrier-guards.rb:224:5:224:7 | foo | true |
+| barrier-guards.rb:227:4:227:15 | ... == ... | barrier-guards.rb:227:4:227:21 | [true] ... and ... | true |
+| barrier-guards.rb:227:4:227:15 | ... == ... | barrier-guards.rb:227:21:227:21 | self | true |
+| barrier-guards.rb:227:4:227:15 | ... == ... | barrier-guards.rb:228:5:228:7 | self | true |
+| barrier-guards.rb:227:4:227:21 | [true] ... and ... | barrier-guards.rb:228:5:228:7 | self | true |
+| barrier-guards.rb:227:21:227:21 | call to y | barrier-guards.rb:227:4:227:21 | [true] ... and ... | true |
+| barrier-guards.rb:227:21:227:21 | call to y | barrier-guards.rb:228:5:228:7 | self | true |
+| barrier-guards.rb:232:1:233:19 | when ... | barrier-guards.rb:233:5:233:7 | foo | match |
+| barrier-guards.rb:232:6:232:17 | ... == ... | barrier-guards.rb:232:1:233:19 | [false] when ... | false |
+| barrier-guards.rb:232:6:232:17 | ... == ... | barrier-guards.rb:232:1:233:19 | when ... | true |
+| barrier-guards.rb:232:6:232:17 | ... == ... | barrier-guards.rb:233:5:233:7 | foo | true |
+| barrier-guards.rb:237:1:237:38 | when ... | barrier-guards.rb:237:24:237:26 | foo | match |
+| barrier-guards.rb:237:6:237:17 | ... == ... | barrier-guards.rb:237:1:237:38 | [false] when ... | false |
+| barrier-guards.rb:237:6:237:17 | ... == ... | barrier-guards.rb:237:1:237:38 | when ... | true |
+| barrier-guards.rb:237:6:237:17 | ... == ... | barrier-guards.rb:237:24:237:26 | foo | true |
+| barrier-guards.rb:238:1:238:26 | when ... | barrier-guards.rb:238:24:238:26 | foo | match |
+| barrier-guards.rb:238:6:238:17 | ... == ... | barrier-guards.rb:238:1:238:26 | [false] when ... | false |
+| barrier-guards.rb:238:6:238:17 | ... == ... | barrier-guards.rb:238:1:238:26 | when ... | true |
+| barrier-guards.rb:238:6:238:17 | ... == ... | barrier-guards.rb:238:24:238:26 | foo | true |
+| barrier-guards.rb:239:1:239:22 | when ... | barrier-guards.rb:239:20:239:22 | foo | match |
+| barrier-guards.rb:239:6:239:13 | ... == ... | barrier-guards.rb:239:1:239:22 | [false] when ... | false |
+| barrier-guards.rb:239:6:239:13 | ... == ... | barrier-guards.rb:239:1:239:22 | when ... | true |
+| barrier-guards.rb:239:6:239:13 | ... == ... | barrier-guards.rb:239:20:239:22 | foo | true |
+| barrier-guards.rb:243:4:243:8 | "foo" | barrier-guards.rb:244:5:244:7 | foo | match |
+| barrier-guards.rb:243:4:243:8 | "foo" | barrier-guards.rb:245:1:246:7 | in ... then ... | no-match |
+| barrier-guards.rb:243:4:243:8 | "foo" | barrier-guards.rb:246:5:246:7 | foo | no-match |
+| barrier-guards.rb:245:4:245:4 | x | barrier-guards.rb:246:5:246:7 | foo | match |
+| barrier-guards.rb:250:4:250:8 | "foo" | barrier-guards.rb:251:5:251:7 | foo | match |
+| barrier-guards.rb:250:4:250:8 | "foo" | barrier-guards.rb:254:1:256:3 | if ... | match |
+| barrier-guards.rb:250:4:250:8 | "foo" | barrier-guards.rb:255:5:255:7 | foo | match |
+| barrier-guards.rb:250:4:250:8 | "foo" | barrier-guards.rb:259:1:261:3 | if ... | match |
+| barrier-guards.rb:250:4:250:8 | "foo" | barrier-guards.rb:260:5:260:7 | foo | match |
+| barrier-guards.rb:250:4:250:8 | "foo" | barrier-guards.rb:264:1:266:3 | if ... | match |
+| barrier-guards.rb:250:4:250:8 | "foo" | barrier-guards.rb:265:5:265:7 | foo | match |
+| barrier-guards.rb:250:4:250:8 | "foo" | barrier-guards.rb:268:1:268:19 | ... && ... | match |
+| barrier-guards.rb:250:4:250:8 | "foo" | barrier-guards.rb:268:17:268:19 | foo | match |
+| barrier-guards.rb:250:4:250:8 | "foo" | barrier-guards.rb:269:1:269:19 | ... && ... | match |
+| barrier-guards.rb:250:4:250:8 | "foo" | barrier-guards.rb:269:8:269:10 | foo | match |
+| barrier-guards.rb:254:4:254:28 | ... == ... | barrier-guards.rb:255:5:255:7 | foo | true |
+| barrier-guards.rb:259:4:259:16 | ... == ... | barrier-guards.rb:260:5:260:7 | foo | true |
+| barrier-guards.rb:264:4:264:16 | ... == ... | barrier-guards.rb:265:5:265:7 | foo | true |
+| barrier-guards.rb:268:1:268:12 | ... == ... | barrier-guards.rb:268:17:268:19 | foo | true |
+| barrier-guards.rb:269:1:269:3 | foo | barrier-guards.rb:269:8:269:10 | foo | true |

From f24fa402f362101d10e89803b8569a07ea823586 Mon Sep 17 00:00:00 2001
From: Tom Hvitved <hvitved@github.com>
Date: Thu, 17 Nov 2022 08:18:20 +0100
Subject: [PATCH 12/17] Adjust CFG

---
 .../lib/codeql/ruby/controlflow/CfgNodes.qll  | 42 +++++++++++++++++--
 .../ruby/controlflow/internal/Completion.qll  | 16 +++----
 .../internal/ControlFlowGraphImpl.qll         |  2 +
 .../ruby/controlflow/internal/Splitting.qll   |  3 --
 .../controlflow/graph/Cfg.expected            | 16 ++++---
 .../barrier-guards/barrier-guards.expected    | 40 ++++++++++++++----
 6 files changed, 88 insertions(+), 31 deletions(-)

diff --git a/ruby/ql/lib/codeql/ruby/controlflow/CfgNodes.qll b/ruby/ql/lib/codeql/ruby/controlflow/CfgNodes.qll
index b411c766f03e..5007acf4e313 100644
--- a/ruby/ql/lib/codeql/ruby/controlflow/CfgNodes.qll
+++ b/ruby/ql/lib/codeql/ruby/controlflow/CfgNodes.qll
@@ -432,8 +432,36 @@ module ExprNodes {
     final ExprCfgNode getBody() { e.hasCfgChild(e.getBody(), this, result) }
   }
 
-  private class WhenClauseChildMapping extends NonExprChildMapping, WhenClause {
-    override predicate relevantChild(AstNode e) { e = [this.getBody(), this.getAPattern()] }
+  // `when` clauses need special treatment, since they are neither pre-order
+  // nor post-order
+  private class WhenClauseChildMapping extends WhenClause {
+    predicate patternReachesBasicBlock(int i, CfgNode cfnPattern, BasicBlock bb) {
+      exists(Expr pattern |
+        pattern = this.getPattern(i) and
+        cfnPattern.getNode() = pattern and
+        bb.getANode() = cfnPattern
+      )
+      or
+      exists(BasicBlock mid |
+        this.patternReachesBasicBlock(i, cfnPattern, mid) and
+        bb = mid.getASuccessor() and
+        not mid.getANode().getNode() = this
+      )
+    }
+
+    predicate bodyReachesBasicBlock(CfgNode cfnBody, BasicBlock bb) {
+      exists(Stmt body |
+        body = this.getBody() and
+        cfnBody.getNode() = body and
+        bb.getANode() = cfnBody
+      )
+      or
+      exists(BasicBlock mid |
+        this.bodyReachesBasicBlock(cfnBody, mid) and
+        bb = mid.getAPredecessor() and
+        not mid.getANode().getNode() = this
+      )
+    }
   }
 
   /** A control-flow node that wraps a `WhenClause` AST expression. */
@@ -443,10 +471,16 @@ module ExprNodes {
     override WhenClauseChildMapping e;
 
     /** Gets the body of this `when`-clause. */
-    final ExprCfgNode getBody() { e.hasCfgChild(e.getBody(), this, result) }
+    final ExprCfgNode getBody() {
+      result.getNode() = desugar(e.getBody()) and
+      e.bodyReachesBasicBlock(result, this.getBasicBlock())
+    }
 
     /** Gets the `i`th pattern this `when`-clause. */
-    final ExprCfgNode getPattern(int i) { result.getExpr() = e.getPattern(i) }
+    final ExprCfgNode getPattern(int i) {
+      result.getNode() = desugar(e.getPattern(i)) and
+      e.patternReachesBasicBlock(i, result, this.getBasicBlock())
+    }
   }
 
   /** A control-flow node that wraps a `CasePattern`. */
diff --git a/ruby/ql/lib/codeql/ruby/controlflow/internal/Completion.qll b/ruby/ql/lib/codeql/ruby/controlflow/internal/Completion.qll
index d727b99d6cf8..26e62e0c0774 100644
--- a/ruby/ql/lib/codeql/ruby/controlflow/internal/Completion.qll
+++ b/ruby/ql/lib/codeql/ruby/controlflow/internal/Completion.qll
@@ -211,8 +211,11 @@ private predicate inBooleanContext(AstNode n) {
   or
   exists(CaseExpr c, WhenClause w |
     not exists(c.getValue()) and
-    c.getABranch() = w and
+    c.getABranch() = w
+  |
     w.getPattern(_) = n
+    or
+    w = n
   )
 }
 
@@ -233,12 +236,11 @@ private predicate inMatchingContext(AstNode n) {
   or
   exists(CaseExpr c, WhenClause w |
     exists(c.getValue()) and
-    (
-      c.getABranch() = w and
-      w.getPattern(_) = n
-      or
-      w = n
-    )
+    c.getABranch() = w
+  |
+    w.getPattern(_) = n
+    or
+    w = n
   )
   or
   n instanceof CasePattern
diff --git a/ruby/ql/lib/codeql/ruby/controlflow/internal/ControlFlowGraphImpl.qll b/ruby/ql/lib/codeql/ruby/controlflow/internal/ControlFlowGraphImpl.qll
index 1ca24d43ab8a..a8bd2e3feaca 100644
--- a/ruby/ql/lib/codeql/ruby/controlflow/internal/ControlFlowGraphImpl.qll
+++ b/ruby/ql/lib/codeql/ruby/controlflow/internal/ControlFlowGraphImpl.qll
@@ -402,6 +402,7 @@ module Trees {
       exists(int i, WhenTree branch | branch = this.getBranch(i) |
         pred = branch and
         first(this.getBranch(i + 1), succ) and
+        c.isValidFor(branch) and
         c.(ConditionalCompletion).getValue() = false
       )
       or
@@ -1411,6 +1412,7 @@ module Trees {
 
     final override predicate last(AstNode last, Completion c) {
       last = this and
+      c.isValidFor(this) and
       c.(ConditionalCompletion).getValue() = false
       or
       last(this.getBody(), last, c) and
diff --git a/ruby/ql/lib/codeql/ruby/controlflow/internal/Splitting.qll b/ruby/ql/lib/codeql/ruby/controlflow/internal/Splitting.qll
index 5e24616ac4a3..a048e2d4044b 100644
--- a/ruby/ql/lib/codeql/ruby/controlflow/internal/Splitting.qll
+++ b/ruby/ql/lib/codeql/ruby/controlflow/internal/Splitting.qll
@@ -85,12 +85,9 @@ private module ConditionalCompletionSplitting {
         or
         last(succ.(ConditionalExpr).getBranch(_), pred, c) and
         completion = c
-        or
-        last(succ.(WhenClause).getAPattern(), pred, c) and completion = c
       )
       or
       succ(pred, succ, c) and
-      succ(succ, _, completion) and
       succ instanceof WhenClause and
       completion = c
     }
diff --git a/ruby/ql/test/library-tests/controlflow/graph/Cfg.expected b/ruby/ql/test/library-tests/controlflow/graph/Cfg.expected
index 69e4cc294deb..9b60a403a301 100644
--- a/ruby/ql/test/library-tests/controlflow/graph/Cfg.expected
+++ b/ruby/ql/test/library-tests/controlflow/graph/Cfg.expected
@@ -1916,16 +1916,15 @@ cfg.rb:
 #   48| [false] when ...
 #-----| false -> b
 
-#   48| when ...
-#-----| match -> self
-#-----| no-match -> b
+#   48| [true] when ...
+#-----| true -> self
 
 #   48| b
 #-----|  -> 1
 
 #   48| ... == ...
 #-----| false -> [false] when ...
-#-----| true -> when ...
+#-----| true -> [true] when ...
 
 #   48| 1
 #-----|  -> ... == ...
@@ -1948,15 +1947,14 @@ cfg.rb:
 #   49| [false] when ...
 #-----| false -> case ...
 
-#   49| when ...
-#-----| no-match -> case ...
-#-----| match -> self
+#   49| [true] when ...
+#-----| true -> self
 
 #   49| b
 #-----|  -> 0
 
 #   49| ... == ...
-#-----| true -> when ...
+#-----| true -> [true] when ...
 #-----| false -> b
 
 #   49| 0
@@ -1967,7 +1965,7 @@ cfg.rb:
 
 #   49| ... > ...
 #-----| false -> [false] when ...
-#-----| true -> when ...
+#-----| true -> [true] when ...
 
 #   49| 1
 #-----|  -> ... > ...
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 7804824ffb64..c308ce9d8200 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
@@ -262,21 +262,45 @@ controls
 | barrier-guards.rb:227:4:227:21 | [true] ... and ... | barrier-guards.rb:228:5:228:7 | self | true |
 | barrier-guards.rb:227:21:227:21 | call to y | barrier-guards.rb:227:4:227:21 | [true] ... and ... | true |
 | barrier-guards.rb:227:21:227:21 | call to y | barrier-guards.rb:228:5:228:7 | self | true |
-| barrier-guards.rb:232:1:233:19 | when ... | barrier-guards.rb:233:5:233:7 | foo | match |
+| barrier-guards.rb:232:1:233:19 | [true] when ... | barrier-guards.rb:233:5:233:7 | foo | true |
 | barrier-guards.rb:232:6:232:17 | ... == ... | barrier-guards.rb:232:1:233:19 | [false] when ... | false |
-| barrier-guards.rb:232:6:232:17 | ... == ... | barrier-guards.rb:232:1:233:19 | when ... | true |
+| barrier-guards.rb:232:6:232:17 | ... == ... | barrier-guards.rb:232:1:233:19 | [true] when ... | true |
 | barrier-guards.rb:232:6:232:17 | ... == ... | barrier-guards.rb:233:5:233:7 | foo | true |
-| barrier-guards.rb:237:1:237:38 | when ... | barrier-guards.rb:237:24:237:26 | foo | match |
+| barrier-guards.rb:237:1:237:38 | [false] when ... | barrier-guards.rb:238:1:238:26 | [false] when ... | false |
+| barrier-guards.rb:237:1:237:38 | [false] when ... | barrier-guards.rb:238:1:238:26 | [true] when ... | false |
+| barrier-guards.rb:237:1:237:38 | [false] when ... | barrier-guards.rb:238:6:238:8 | self | false |
+| barrier-guards.rb:237:1:237:38 | [false] when ... | barrier-guards.rb:238:24:238:26 | foo | false |
+| barrier-guards.rb:237:1:237:38 | [false] when ... | barrier-guards.rb:239:1:239:22 | [false] when ... | false |
+| barrier-guards.rb:237:1:237:38 | [false] when ... | barrier-guards.rb:239:1:239:22 | [true] when ... | false |
+| barrier-guards.rb:237:1:237:38 | [false] when ... | barrier-guards.rb:239:6:239:8 | foo | false |
+| barrier-guards.rb:237:1:237:38 | [false] when ... | barrier-guards.rb:239:20:239:22 | foo | false |
+| barrier-guards.rb:237:1:237:38 | [true] when ... | barrier-guards.rb:237:24:237:26 | foo | true |
 | barrier-guards.rb:237:6:237:17 | ... == ... | barrier-guards.rb:237:1:237:38 | [false] when ... | false |
-| barrier-guards.rb:237:6:237:17 | ... == ... | barrier-guards.rb:237:1:237:38 | when ... | true |
+| barrier-guards.rb:237:6:237:17 | ... == ... | barrier-guards.rb:237:1:237:38 | [true] when ... | true |
 | barrier-guards.rb:237:6:237:17 | ... == ... | barrier-guards.rb:237:24:237:26 | foo | true |
-| barrier-guards.rb:238:1:238:26 | when ... | barrier-guards.rb:238:24:238:26 | foo | match |
+| barrier-guards.rb:237:6:237:17 | ... == ... | barrier-guards.rb:238:1:238:26 | [false] when ... | false |
+| barrier-guards.rb:237:6:237:17 | ... == ... | barrier-guards.rb:238:1:238:26 | [true] when ... | false |
+| barrier-guards.rb:237:6:237:17 | ... == ... | barrier-guards.rb:238:6:238:8 | self | false |
+| barrier-guards.rb:237:6:237:17 | ... == ... | barrier-guards.rb:238:24:238:26 | foo | false |
+| barrier-guards.rb:237:6:237:17 | ... == ... | barrier-guards.rb:239:1:239:22 | [false] when ... | false |
+| barrier-guards.rb:237:6:237:17 | ... == ... | barrier-guards.rb:239:1:239:22 | [true] when ... | false |
+| barrier-guards.rb:237:6:237:17 | ... == ... | barrier-guards.rb:239:6:239:8 | foo | false |
+| barrier-guards.rb:237:6:237:17 | ... == ... | barrier-guards.rb:239:20:239:22 | foo | false |
+| barrier-guards.rb:238:1:238:26 | [false] when ... | barrier-guards.rb:239:1:239:22 | [false] when ... | false |
+| barrier-guards.rb:238:1:238:26 | [false] when ... | barrier-guards.rb:239:1:239:22 | [true] when ... | false |
+| barrier-guards.rb:238:1:238:26 | [false] when ... | barrier-guards.rb:239:6:239:8 | foo | false |
+| barrier-guards.rb:238:1:238:26 | [false] when ... | barrier-guards.rb:239:20:239:22 | foo | false |
+| barrier-guards.rb:238:1:238:26 | [true] when ... | barrier-guards.rb:238:24:238:26 | foo | true |
 | barrier-guards.rb:238:6:238:17 | ... == ... | barrier-guards.rb:238:1:238:26 | [false] when ... | false |
-| barrier-guards.rb:238:6:238:17 | ... == ... | barrier-guards.rb:238:1:238:26 | when ... | true |
+| barrier-guards.rb:238:6:238:17 | ... == ... | barrier-guards.rb:238:1:238:26 | [true] when ... | true |
 | barrier-guards.rb:238:6:238:17 | ... == ... | barrier-guards.rb:238:24:238:26 | foo | true |
-| barrier-guards.rb:239:1:239:22 | when ... | barrier-guards.rb:239:20:239:22 | foo | match |
+| barrier-guards.rb:238:6:238:17 | ... == ... | barrier-guards.rb:239:1:239:22 | [false] when ... | false |
+| barrier-guards.rb:238:6:238:17 | ... == ... | barrier-guards.rb:239:1:239:22 | [true] when ... | false |
+| barrier-guards.rb:238:6:238:17 | ... == ... | barrier-guards.rb:239:6:239:8 | foo | false |
+| barrier-guards.rb:238:6:238:17 | ... == ... | barrier-guards.rb:239:20:239:22 | foo | false |
+| barrier-guards.rb:239:1:239:22 | [true] when ... | barrier-guards.rb:239:20:239:22 | foo | true |
 | barrier-guards.rb:239:6:239:13 | ... == ... | barrier-guards.rb:239:1:239:22 | [false] when ... | false |
-| barrier-guards.rb:239:6:239:13 | ... == ... | barrier-guards.rb:239:1:239:22 | when ... | true |
+| barrier-guards.rb:239:6:239:13 | ... == ... | barrier-guards.rb:239:1:239:22 | [true] when ... | true |
 | barrier-guards.rb:239:6:239:13 | ... == ... | barrier-guards.rb:239:20:239:22 | foo | true |
 | barrier-guards.rb:243:4:243:8 | "foo" | barrier-guards.rb:244:5:244:7 | foo | match |
 | barrier-guards.rb:243:4:243:8 | "foo" | barrier-guards.rb:245:1:246:7 | in ... then ... | no-match |

From 5deb16e58cd40f7f181a1caaaeef3772d7c88e8a Mon Sep 17 00:00:00 2001
From: Harry Maclean <hmac@github.com>
Date: Fri, 18 Nov 2022 18:14:53 +1300
Subject: [PATCH 13/17] Ruby: Remove redundant predicate

The existing barrier guard machinery recognises guards such as `if x and y`,
so there's no need to explicitly model them.
---
 .../codeql/ruby/dataflow/BarrierGuards.qll    | 19 -------------------
 1 file changed, 19 deletions(-)

diff --git a/ruby/ql/lib/codeql/ruby/dataflow/BarrierGuards.qll b/ruby/ql/lib/codeql/ruby/dataflow/BarrierGuards.qll
index 16e965f66572..92fb759c468f 100644
--- a/ruby/ql/lib/codeql/ruby/dataflow/BarrierGuards.qll
+++ b/ruby/ql/lib/codeql/ruby/dataflow/BarrierGuards.qll
@@ -35,8 +35,6 @@ private predicate stringConstCompare(CfgNodes::AstCfgNode guard, CfgNode testedN
     stringConstCompareOr(guard, def, branch) and
     stringConstCompare(g.getLeftOperand(), testedNode, _)
   )
-  or
-  stringConstCompareAnd(guard, testedNode, branch)
 }
 
 /**
@@ -57,23 +55,6 @@ private predicate stringConstCompareOr(
   )
 }
 
-/**
- * Holds if `guard` is an `and` expression containing a string comparison guard in either operand.
- * For example:
- *
- * ```rb
- * x == "foo" and other_condition()
- * other_condition() and x == "foo"
- * ```
- */
-private predicate stringConstCompareAnd(
-  CfgNodes::ExprNodes::BinaryOperationCfgNode guard, CfgNode testedNode, boolean branch
-) {
-  guard.getExpr() instanceof LogicalAndExpr and
-  branch = true and
-  stringConstCompare(guard.getAnOperand(), testedNode, branch)
-}
-
 /**
  * A validation of value by comparing with a constant string value, for example
  * in:

From 376d4e03a144178d7ed2744201c449e808c0a7db Mon Sep 17 00:00:00 2001
From: Harry Maclean <hmac@github.com>
Date: Fri, 18 Nov 2022 18:17:02 +1300
Subject: [PATCH 14/17] Ruby: Cache some barrier guard predicates

---
 ruby/ql/lib/codeql/ruby/dataflow/BarrierGuards.qll | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/ruby/ql/lib/codeql/ruby/dataflow/BarrierGuards.qll b/ruby/ql/lib/codeql/ruby/dataflow/BarrierGuards.qll
index 92fb759c468f..f1f8d2c5b461 100644
--- a/ruby/ql/lib/codeql/ruby/dataflow/BarrierGuards.qll
+++ b/ruby/ql/lib/codeql/ruby/dataflow/BarrierGuards.qll
@@ -9,6 +9,7 @@ private import codeql.ruby.ast.internal.Constant
 private import codeql.ruby.InclusionTests
 private import codeql.ruby.ast.internal.Literal
 
+cached
 private predicate stringConstCompare(CfgNodes::AstCfgNode guard, CfgNode testedNode, boolean branch) {
   exists(CfgNodes::ExprNodes::ComparisonOperationCfgNode c |
     c = guard and
@@ -102,6 +103,7 @@ deprecated class StringConstCompare extends DataFlow::BarrierGuard,
   }
 }
 
+cached
 private predicate stringConstArrayInclusionCall(
   CfgNodes::AstCfgNode guard, CfgNode testedNode, boolean branch
 ) {

From 57f689401efbda3ec02b9d9487467fe4b820d042 Mon Sep 17 00:00:00 2001
From: Harry Maclean <hmac@github.com>
Date: Thu, 24 Nov 2022 17:33:57 +1300
Subject: [PATCH 15/17] Ruby: SplatExprCfgNode extends UnaryOperationCfgNode

---
 ruby/ql/lib/codeql/ruby/controlflow/CfgNodes.qll | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/ruby/ql/lib/codeql/ruby/controlflow/CfgNodes.qll b/ruby/ql/lib/codeql/ruby/controlflow/CfgNodes.qll
index 5007acf4e313..c8ca8ff12e62 100644
--- a/ruby/ql/lib/codeql/ruby/controlflow/CfgNodes.qll
+++ b/ruby/ql/lib/codeql/ruby/controlflow/CfgNodes.qll
@@ -900,20 +900,17 @@ module ExprNodes {
     final override RelationalOperation getExpr() { result = super.getExpr() }
   }
 
-  private class SplatExprChildMapping extends ExprChildMapping, SplatExpr {
+  private class SplatExprChildMapping extends OperationExprChildMapping, SplatExpr {
     override predicate relevantChild(AstNode n) { n = this.getOperand() }
   }
 
   /** A control-flow node that wraps a `SplatExpr` AST expression. */
-  class SplatExprCfgNode extends ExprCfgNode {
+  class SplatExprCfgNode extends UnaryOperationCfgNode {
     override string getAPrimaryQlClass() { result = "SplatExprCfgNode" }
 
     override SplatExprChildMapping e;
 
     final override SplatExpr getExpr() { result = super.getExpr() }
-
-    /** Gets the operand of this splat expression. */
-    final ExprCfgNode getOperand() { e.hasCfgChild(e.getOperand(), this, result) }
   }
 
   /** A control-flow node that wraps an `ElementReference` AST expression. */

From 6897fb46cb92c6e69b2f0a281c567e7d097df908 Mon Sep 17 00:00:00 2001
From: Harry Maclean <hmac@github.com>
Date: Thu, 24 Nov 2022 17:41:04 +1300
Subject: [PATCH 16/17] Ruby: Clean up WhenClause CFG

---
 .../ruby/controlflow/internal/ControlFlowGraphImpl.qll     | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/ruby/ql/lib/codeql/ruby/controlflow/internal/ControlFlowGraphImpl.qll b/ruby/ql/lib/codeql/ruby/controlflow/internal/ControlFlowGraphImpl.qll
index a8bd2e3feaca..ba77d6beaba2 100644
--- a/ruby/ql/lib/codeql/ruby/controlflow/internal/ControlFlowGraphImpl.qll
+++ b/ruby/ql/lib/codeql/ruby/controlflow/internal/ControlFlowGraphImpl.qll
@@ -1399,7 +1399,9 @@ module Trees {
   }
 
   private class WhenTree extends ControlFlowTree, WhenClause {
-    final override predicate propagatesAbnormal(AstNode child) { child = this.getAPattern() }
+    final override predicate propagatesAbnormal(AstNode child) {
+      child = [this.getAPattern(), this.getBody()]
+    }
 
     final Expr getLastPattern() {
       exists(int i |
@@ -1415,8 +1417,7 @@ module Trees {
       c.isValidFor(this) and
       c.(ConditionalCompletion).getValue() = false
       or
-      last(this.getBody(), last, c) and
-      c instanceof NormalCompletion
+      last(this.getBody(), last, c)
     }
 
     final override predicate succ(AstNode pred, AstNode succ, Completion c) {

From 2822c94aa795373371b51f3d900a9ac1cc5aa897 Mon Sep 17 00:00:00 2001
From: Harry Maclean <hmac@github.com>
Date: Thu, 24 Nov 2022 17:48:41 +1300
Subject: [PATCH 17/17] Ruby: Minor refactor of barrier guard code

---
 .../codeql/ruby/dataflow/BarrierGuards.qll    | 59 ++++++++++---------
 1 file changed, 30 insertions(+), 29 deletions(-)

diff --git a/ruby/ql/lib/codeql/ruby/dataflow/BarrierGuards.qll b/ruby/ql/lib/codeql/ruby/dataflow/BarrierGuards.qll
index f1f8d2c5b461..f586c7d74cc2 100644
--- a/ruby/ql/lib/codeql/ruby/dataflow/BarrierGuards.qll
+++ b/ruby/ql/lib/codeql/ruby/dataflow/BarrierGuards.qll
@@ -31,15 +31,15 @@ private predicate stringConstCompare(CfgNodes::AstCfgNode guard, CfgNode testedN
   or
   stringConstCaseCompare(guard, testedNode, branch)
   or
-  exists(Ssa::Definition def, CfgNodes::ExprNodes::BinaryOperationCfgNode g |
+  exists(CfgNodes::ExprNodes::BinaryOperationCfgNode g |
     g = guard and
-    stringConstCompareOr(guard, def, branch) and
+    stringConstCompareOr(guard, branch) and
     stringConstCompare(g.getLeftOperand(), testedNode, _)
   )
 }
 
 /**
- * Holds if `guard` is an `or` expression whose operands are string comparison guards that test `def`.
+ * Holds if `guard` is an `or` expression whose operands are string comparison guards.
  * For example:
  *
  * ```rb
@@ -47,12 +47,12 @@ private predicate stringConstCompare(CfgNodes::AstCfgNode guard, CfgNode testedN
  * ```
  */
 private predicate stringConstCompareOr(
-  CfgNodes::ExprNodes::BinaryOperationCfgNode guard, Ssa::Definition def, boolean branch
+  CfgNodes::ExprNodes::BinaryOperationCfgNode guard, boolean branch
 ) {
   guard.getExpr() instanceof LogicalOrExpr and
   branch = true and
   forall(CfgNode innerGuard | innerGuard = guard.getAnOperand() |
-    stringConstCompare(innerGuard, def.getARead(), branch)
+    stringConstCompare(innerGuard, any(Ssa::Definition def).getARead(), branch)
   )
 }
 
@@ -190,34 +190,35 @@ private predicate stringConstCaseCompare(
   exists(CfgNodes::ExprNodes::CaseExprCfgNode case |
     case.getValue() = testedNode and
     (
-      exists(CfgNodes::ExprNodes::WhenClauseCfgNode branchNode |
-        guard = branchNode and
-        branchNode = case.getBranch(_) 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
-          exists(CfgNodes::ExprNodes::SplatExprCfgNode splat | splat = pattern |
-            // when *["foo", "bar"]
-            forex(ExprCfgNode elem |
-              elem = splat.getOperand().(ExprNodes::ArrayLiteralCfgNode).getAnArgument()
-            |
-              elem instanceof ExprNodes::StringLiteralCfgNode
-            )
+      guard =
+        any(CfgNodes::ExprNodes::WhenClauseCfgNode branchNode |
+          branchNode = case.getBranch(_) 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
-            // 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
+            pattern =
+              any(CfgNodes::ExprNodes::SplatExprCfgNode splat |
+                // when *["foo", "bar"]
+                forex(ExprCfgNode elem |
+                  elem = splat.getOperand().(ExprNodes::ArrayLiteralCfgNode).getAnArgument()
+                |
+                  elem instanceof ExprNodes::StringLiteralCfgNode
+                )
+                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
+                  )
+                )
               )
-            )
           )
         )
-      )
       or
       // in "foo"
       exists(