Address review comments

ql/src/codeql_ruby/controlflow/CfgNodes.qll

@@ -113,6 +113,16 @@ class ReturningCfgNode extends AstCfgNode {
   }
 }
 
+/** A control-flow node that wraps a `StringComponent` AST expression. */
+class StringComponentCfgNode extends AstCfgNode {
+  StringComponentCfgNode() { this.getNode() instanceof StringComponent }
+}
+
+/** A control-flow node that wraps a `StringInterpolationComponent` AST expression. */
+class StringInterpolationComponentCfgNode extends StringComponentCfgNode {
+  StringInterpolationComponentCfgNode() { this.getNode() instanceof StringInterpolationComponent }
+}
+
 private Expr desugar(Expr n) {
   result = n.getDesugared()
   or
@@ -197,7 +207,7 @@ module ExprNodes {
 
     BinaryOperationCfgNode() { e = bo }
 
-    final override BinaryOperation getExpr() { result = super.getExpr() }
+    override BinaryOperation getExpr() { result = super.getExpr() }
 
     /** Gets the left operand of this binary operation. */
     final ExprCfgNode getLeftOperand() { e.hasCfgChild(bo.getLeftOperand(), this, result) }
@@ -253,7 +263,7 @@ module ExprNodes {
   class MethodCallCfgNode extends CallCfgNode {
     MethodCallCfgNode() { super.getExpr() instanceof MethodCall }
 
-    final override MethodCall getExpr() { result = super.getExpr() }
+    override MethodCall getExpr() { result = super.getExpr() }
   }
 
   /** A control-flow node that wraps a `CaseExpr` AST expression. */
@@ -331,34 +341,32 @@ module ExprNodes {
     final override VariableReadAccess getExpr() { result = ExprCfgNode.super.getExpr() }
   }
 
+  private class StringlikeLiteralChildMapping extends ExprChildMapping, StringlikeLiteral {
+    override predicate relevantChild(Expr e) { e = this.getComponent(_) }
+  }
+
   /** A control-flow node that wraps a `StringlikeLiteral` AST expression. */
   class StringlikeLiteralCfgNode extends ExprCfgNode {
-    override StringlikeLiteral e;
+    override StringlikeLiteralChildMapping e;
 
-    final override StringlikeLiteral getExpr() { result = ExprCfgNode.super.getExpr() }
+    final override StringlikeLiteral getExpr() { result = super.getExpr() }
+
+    /** Gets a component of this `StringlikeLiteral` */
+    StringComponentCfgNode getAComponent() { e.hasCfgChild(e.getComponent(_), this, result) }
   }
 
   /** A control-flow node that wraps a `StringLiteral` AST expression. */
   class StringLiteralCfgNode extends ExprCfgNode {
     override StringLiteral e;
 
-    final override StringLiteral getExpr() { result = ExprCfgNode.super.getExpr() }
+    final override StringLiteral getExpr() { result = super.getExpr() }
   }
 
   /** A control-flow node that wraps a `ComparisonOperation` AST expression. */
-  class ComparisonOperationCfgNode extends ExprCfgNode {
-    override ComparisonOperation e;
+  class ComparisonOperationCfgNode extends BinaryOperationCfgNode {
+    ComparisonOperationCfgNode() { e instanceof ComparisonOperation }
 
-    final override ComparisonOperation getExpr() { result = ExprCfgNode.super.getExpr() }
-
-    /** Whether left and right are a pair of operands for this comparison */
-    predicate operands(CfgNode left, CfgNode right) {
-      exists(Expr eleft, Expr eright | left.getNode() = eleft and right.getNode() = eright |
-        eleft = e.getLeftOperand() and eright = e.getRightOperand()
-      ) and
-      left.getBasicBlock().dominates(this.getBasicBlock()) and
-      right.getBasicBlock().dominates(this.getBasicBlock())
-    }
+    final override ComparisonOperation getExpr() { result = super.getExpr() }
   }
 
   /** A control-flow node that wraps an `ElementReference` AST expression. */

ql/src/codeql_ruby/dataflow/BarrierGuards.qll

@@ -19,20 +19,20 @@ private import codeql_ruby.CFG
  */
 class StringConstCompare extends DataFlow::BarrierGuard,
   CfgNodes::ExprNodes::ComparisonOperationCfgNode {
-  private CfgNode checked_node;
+  private CfgNode checkedNode;
 
   StringConstCompare() {
-    exists(CfgNodes::ExprNodes::StringLiteralCfgNode str_lit_node |
+    exists(CfgNodes::ExprNodes::StringLiteralCfgNode strLitNode |
       this.getExpr() instanceof EqExpr or
       this.getExpr() instanceof CaseEqExpr
     |
-      this.operands(str_lit_node, checked_node)
+      this.getLeftOperand() = strLitNode and this.getRightOperand() = checkedNode
       or
-      this.operands(checked_node, str_lit_node)
+      this.getLeftOperand() = checkedNode and this.getRightOperand() = strLitNode
     )
   }
 
-  override DataFlow::Node getAGuardedNode() { result.asExpr() = checked_node }
+  override DataFlow::Node getAGuardedNode() { result.asExpr() = checkedNode }
 }
 
 /**
@@ -52,19 +52,17 @@ class StringConstCompare extends DataFlow::BarrierGuard,
 //
 class StringConstArrayInclusionCall extends DataFlow::BarrierGuard,
   CfgNodes::ExprNodes::MethodCallCfgNode {
-  private CfgNode checked_node;
+  private CfgNode checkedNode;
 
   StringConstArrayInclusionCall() {
-    exists(ArrayLiteral a_lit, MethodCall include_call |
-      include_call = this.getExpr() and
-      include_call.getMethodName() = "include?" and
-      include_call.getReceiver() = a_lit
+    exists(ArrayLiteral aLit |
+      this.getExpr().getMethodName() = "include?" and
+      this.getExpr().getReceiver() = aLit
     |
-      forall(Expr elem | elem = a_lit.getAnElement() | elem instanceof StringLiteral) and
-      include_call.getArgument(0) = checked_node.getNode() and
-      checked_node.getBasicBlock().dominates(this.getBasicBlock())
+      forall(Expr elem | elem = aLit.getAnElement() | elem instanceof StringLiteral) and
+      this.getExpr().getArgument(0) = checkedNode.getNode()
     )
   }
 
-  override DataFlow::Node getAGuardedNode() { result.asExpr() = checked_node }
+  override DataFlow::Node getAGuardedNode() { result.asExpr() = checkedNode }
 }

ql/src/codeql_ruby/dataflow/internal/TaintTrackingPrivate.qll

@@ -23,10 +23,13 @@ predicate defaultAdditionalTaintStep(DataFlow::Node nodeFrom, DataFlow::Node nod
   )
   or
   // string interpolation of `nodeFrom` into `nodeTo`
-  exists(CfgNodes::ExprNodes::StringlikeLiteralCfgNode lit, StringInterpolationComponent sic |
+  exists(
+    CfgNodes::ExprNodes::StringlikeLiteralCfgNode lit,
+    CfgNodes::StringInterpolationComponentCfgNode sic
+  |
     lit = nodeTo.asExpr() and
-    sic = lit.getExpr().getComponent(_) and
-    sic.getAStmt() = nodeFrom.asExpr().getExpr()
+    sic = lit.getAComponent() and
+    sic = nodeFrom.asExpr()
   )
   or
   // element reference from nodeFrom

ql/test/query-tests/security/cwe-089/ActiveRecordInjection.rb

@@ -44,11 +44,11 @@ class BarController < ApplicationController
 
   def some_other_request_handler
     ps = params
-
     uid = ps[:id]
+    uidEq = "= #{uid}"
 
     # DELETE FROM "users" WHERE (id = #{uid})
-    User.delete_all("id = " + uid)
+    User.delete_all("id " + uidEq)
   end
 
   def sanitized_paths