Review comments

Author: hvitved

ruby/ql/lib/codeql/ruby/dataflow/internal/DataFlowDispatch.qll

@@ -294,23 +294,25 @@ private DataFlowCallable viableLibraryCallable(DataFlowCall call) {
   )
 }
 
-/** Holds if there is a call like `receiver.extend(M)` */
+/** Holds if there is a call like `receiver.extend(M)`. */
 pragma[nomagic]
-private predicate flowsToExtendCall(DataFlow::LocalSourceNode receiver, Module m) {
+private predicate extendCall(DataFlow::ExprNode receiver, Module m) {
   exists(DataFlow::CallNode extendCall |
     extendCall.getMethodName() = "extend" and
     exists(DataFlow::LocalSourceNode sourceNode | sourceNode.flowsTo(extendCall.getArgument(0)) |
       selfInModule(sourceNode.(SsaSelfDefinitionNode).getVariable(), m) or
       m = resolveConstantReadAccess(sourceNode.asExpr().getExpr())
     ) and
-    receiver.flowsTo(extendCall.getReceiver())
+    receiver = extendCall.getReceiver()
   )
 }
 
 /** Holds if there is a call like `M.extend(N)` */
 pragma[nomagic]
 private predicate extendCallModule(Module m, Module n) {
-  exists(DataFlow::LocalSourceNode receiver | flowsToExtendCall(receiver, n) |
+  exists(DataFlow::LocalSourceNode receiver, DataFlow::ExprNode e |
+    receiver.flowsTo(e) and extendCall(e, n)
+  |
     selfInModule(receiver.(SsaSelfDefinitionNode).getVariable(), m) or
     m = resolveConstantReadAccess(receiver.asExpr().getExpr())
   )
@@ -373,14 +375,7 @@ private module Cached {
         // ```
         exists(DataFlow::Node receiver |
           methodCall(call, receiver, method) and
-          (
-            receiver = trackSingletonMethodOnInstance(result, method)
-            or
-            exists(Module m |
-              flowsToExtendCall(any(DataFlow::LocalSourceNode n | n.flowsTo(receiver)), m) and
-              result = lookupMethod(m, pragma[only_bind_into](method))
-            )
-          )
+          receiver = trackSingletonMethodOnInstance(result, method)
         )
         or
         // singleton method defined on a module
@@ -394,14 +389,7 @@ private module Cached {
         // ```
         exists(DataFlow::Node sourceNode, Module m |
           flowsToMethodCall(call, sourceNode, method) and
-          (
-            singletonMethodOnModule(result, method, m)
-            or
-            exists(Module other |
-              extendCallModule(m, other) and
-              result = lookupMethod(other, pragma[only_bind_into](method))
-            )
-          )
+          singletonMethodOnModule(result, method, m)
         |
           // ```rb
           // def C.singleton; end # <- result
@@ -717,6 +705,12 @@ private predicate flowsToSingletonMethodObject(
  * class << c
  *   def m6; end # not included
  * end
+ *
+ * module M
+ *   def instance; end # included in `N` via `extend` call below
+ * end
+ * N.extend(M)
+ * N.instance
  * ```
  */
 pragma[nomagic]
@@ -727,6 +721,11 @@ private predicate singletonMethodOnModule(MethodBase method, string name, Module
   )
   or
   flowsToSingletonMethodObject(trackModuleAccess(m), method, name)
+  or
+  exists(Module other |
+    extendCallModule(m, other) and
+    method = lookupMethod(other, name)
+  )
 }
 
 /**
@@ -753,6 +752,11 @@ private predicate singletonMethodOnModule(MethodBase method, string name, Module
  * class << c
  *   def m6; end # included
  * end
+ *
+ * module M
+ *   def instance; end # included in `c` via `extend` call below
+ * end
+ * c.extend(M)
  * ```
  */
 pragma[nomagic]
@@ -761,6 +765,12 @@ predicate singletonMethodOnInstance(MethodBase method, string name, Expr object)
   not selfInModule(object.(SelfVariableReadAccess).getVariable(), _) and
   // cannot use `trackModuleAccess` because of negative recursion
   not exists(resolveConstantReadAccess(object))
+  or
+  exists(DataFlow::ExprNode receiver, Module other |
+    extendCall(receiver, other) and
+    object = receiver.getExprNode().getExpr() and
+    method = lookupMethod(other, name)
+  )
 }
 
 /**

ruby/ql/lib/codeql/ruby/dataflow/internal/TaintTrackingPrivate.qll

@@ -85,7 +85,10 @@ private module Cached {
       op.getAnOperand() = nodeFrom.asExpr() and
       not op.getExpr() =
         any(Expr e |
+          // included in normal data-flow
           e instanceof AssignExpr or
+          e instanceof BinaryLogicalOperation or
+          // has flow summary
           e instanceof SplatExpr
         )
     )

ruby/ql/lib/codeql/ruby/frameworks/stdlib/Pathname.qll

@@ -26,7 +26,7 @@ module Pathname {
    *
    * Every `PathnameInstance` is considered to be a `FileNameSource`.
    */
-  class PathnameInstance extends FileNameSource, DataFlow::Node {
+  class PathnameInstance extends FileNameSource {
     PathnameInstance() { any(PathnameConfiguration c).hasFlowTo(this) }
   }
 
@@ -44,14 +44,15 @@ module Pathname {
     override predicate isSink(DataFlow::Node sink) { any() }
 
     override predicate isAdditionalFlowStep(DataFlow::Node node1, DataFlow::Node node2) {
-      exists(DataFlow::CallNode c | node2 = c |
-        c.getReceiver() = node1 and
-        c.getMethodName() =
-          [
-            "+", "/", "basename", "cleanpath", "expand_path", "join", "realpath",
-            "relative_path_from", "sub", "sub_ext", "to_path"
-          ]
-      )
+      node2 =
+        any(DataFlow::CallNode c |
+          c.getReceiver() = node1 and
+          c.getMethodName() =
+            [
+              "+", "/", "basename", "cleanpath", "expand_path", "join", "realpath",
+              "relative_path_from", "sub", "sub_ext", "to_path"
+            ]
+        )
     }
   }
 

ruby/ql/test/library-tests/modules/callgraph.expected

@@ -186,7 +186,6 @@ getTarget
 | calls.rb:488:1:488:31 | call to singleton | calls.rb:481:5:483:7 | singleton |
 | calls.rb:494:1:494:32 | call to singleton | calls.rb:481:5:483:7 | singleton |
 | calls.rb:501:1:501:32 | call to singleton | calls.rb:481:5:483:7 | singleton |
-| calls.rb:504:1:504:13 | call to singleton | calls.rb:481:5:483:7 | singleton |
 | calls.rb:507:1:507:13 | call to singleton | calls.rb:481:5:483:7 | singleton |
 | calls.rb:516:5:516:35 | call to include | calls.rb:108:5:110:7 | include |
 | calls.rb:519:9:519:35 | call to puts | calls.rb:102:5:102:30 | puts |
@@ -311,6 +310,7 @@ unresolvedCall
 | calls.rb:485:5:485:15 | call to extend |
 | calls.rb:491:5:491:32 | call to extend |
 | calls.rb:499:1:499:51 | call to extend |
+| calls.rb:504:1:504:13 | call to singleton |
 | calls.rb:505:1:505:32 | call to extend |
 | calls.rb:510:5:512:7 | call to protected |
 | calls.rb:511:9:511:42 | call to puts |