Merge pull request #10504 from hvitved/ruby/private-methods

Ruby: Two fixes for `private` methods

Author: hvitved

ruby/ql/lib/codeql/ruby/ast/Module.qll

@@ -1,4 +1,5 @@
 private import codeql.ruby.AST
+private import codeql.ruby.CFG
 private import internal.AST
 private import internal.Module
 private import internal.TreeSitter
@@ -49,24 +50,54 @@ class Module extends TModule {
   }
 }
 
+/**
+ * Gets the enclosing module of `s`, but only if `s` and the module are in the
+ * same CFG scope. For example, in
+ *
+ * ```rb
+ * module M
+ *   def pub; end
+ *   private def priv; end
+ * end
+ * ```
+ *
+ * `M` is the enclosing module of `pub` and `priv`, in the same CFG scope, while
+ * in
+ *
+ * ```rb
+ * module M
+ *   def m
+ *     def nested; end
+ *   end
+ * end
+ * ```
+ *
+ * `M` is the enclosing module of `m`, in the same CFG scope, while `nested` is not.
+ */
+pragma[nomagic]
+private ModuleBase getEnclosingModuleInSameCfgScope(Stmt s) {
+  result = s.getEnclosingModule() and
+  s.getCfgScope() = [result.(CfgScope), result.getCfgScope()]
+}
+
 /**
  * The base class for classes, singleton classes, and modules.
  */
 class ModuleBase extends BodyStmt, Scope, TModuleBase {
   /** Gets a method defined in this module/class. */
-  MethodBase getAMethod() { result = this.getAStmt() }
+  MethodBase getAMethod() { this = getEnclosingModuleInSameCfgScope(result) }
 
   /** Gets the method named `name` in this module/class, if any. */
   MethodBase getMethod(string name) { result = this.getAMethod() and result.getName() = name }
 
   /** Gets a class defined in this module/class. */
-  ClassDeclaration getAClass() { result = this.getAStmt() }
+  ClassDeclaration getAClass() { this = getEnclosingModuleInSameCfgScope(result) }
 
   /** Gets the class named `name` in this module/class, if any. */
   ClassDeclaration getClass(string name) { result = this.getAClass() and result.getName() = name }
 
   /** Gets a module defined in this module/class. */
-  ModuleDeclaration getAModule() { result = this.getAStmt() }
+  ModuleDeclaration getAModule() { this = getEnclosingModuleInSameCfgScope(result) }
 
   /** Gets the module named `name` in this module/class, if any. */
   ModuleDeclaration getModule(string name) {

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

@@ -302,11 +302,7 @@ private module Cached {
           result = lookupMethod(tp, method) and
           if result.(Method).isPrivate()
           then
-            exists(SelfVariableAccess self |
-              self = call.getReceiver().getExpr() and
-              pragma[only_bind_out](self.getEnclosingModule().getModule().getSuperClass*()) =
-                pragma[only_bind_out](result.getEnclosingModule().getModule())
-            ) and
+            call.getReceiver().getExpr() instanceof SelfVariableAccess and
             // For now, we restrict the scope of top-level declarations to their file.
             // This may remove some plausible targets, but also removes a lot of
             // implausible targets

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

@@ -187,3 +187,9 @@ private.rb:
 #-----| super -> Object
 
 #   42| F
+
+#   62| PrivateOverride1
+#-----| super -> Object
+
+#   76| PrivateOverride2
+#-----| super -> PrivateOverride1

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

@@ -186,6 +186,19 @@ getTarget
 | private.rb:43:3:44:5 | call to private | calls.rb:109:5:109:20 | private |
 | private.rb:51:3:51:19 | call to private | calls.rb:109:5:109:20 | private |
 | private.rb:53:3:53:9 | call to private | calls.rb:109:5:109:20 | private |
+| private.rb:63:3:65:5 | call to private | calls.rb:109:5:109:20 | private |
+| private.rb:64:7:64:32 | call to puts | calls.rb:102:5:102:30 | puts |
+| private.rb:67:3:69:5 | call to private | calls.rb:109:5:109:20 | private |
+| private.rb:68:7:68:32 | call to puts | calls.rb:102:5:102:30 | puts |
+| private.rb:72:7:72:8 | call to m1 | private.rb:63:11:65:5 | m1 |
+| private.rb:72:7:72:8 | call to m1 | private.rb:77:11:81:5 | m1 |
+| private.rb:77:3:81:5 | call to private | calls.rb:109:5:109:20 | private |
+| private.rb:78:7:78:32 | call to puts | calls.rb:102:5:102:30 | puts |
+| private.rb:79:7:79:8 | call to m2 | private.rb:67:11:69:5 | m2 |
+| private.rb:80:7:80:26 | call to new | calls.rb:114:5:114:16 | new |
+| private.rb:84:1:84:20 | call to new | calls.rb:114:5:114:16 | new |
+| private.rb:84:1:84:28 | call to call_m1 | private.rb:71:3:73:5 | call_m1 |
+| private.rb:85:1:85:20 | call to new | calls.rb:114:5:114:16 | new |
 unresolvedCall
 | calls.rb:26:9:26:18 | call to instance_m |
 | calls.rb:29:5:29:14 | call to instance_m |
@@ -230,6 +243,8 @@ unresolvedCall
 | private.rb:35:1:35:14 | call to private2 |
 | private.rb:36:1:36:14 | call to private3 |
 | private.rb:37:1:37:14 | call to private4 |
+| private.rb:80:7:80:29 | call to m1 |
+| private.rb:85:1:85:23 | call to m1 |
 privateMethod
 | calls.rb:1:1:3:3 | foo |
 | calls.rb:39:1:41:3 | call_instance_m |
@@ -253,3 +268,6 @@ privateMethod
 | private.rb:49:3:50:5 | private2 |
 | private.rb:55:3:56:5 | private3 |
 | private.rb:58:3:59:5 | private4 |
+| private.rb:63:11:65:5 | m1 |
+| private.rb:67:11:69:5 | m2 |
+| private.rb:77:11:81:5 | m1 |

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

@@ -46,14 +46,20 @@ getMethod
 | modules.rb:5:3:14:5 | Foo::Bar | method_in_foo_bar | modules.rb:9:5:10:7 | method_in_foo_bar |
 | modules.rb:37:1:46:3 | Bar | method_a | modules.rb:38:3:39:5 | method_a |
 | modules.rb:37:1:46:3 | Bar | method_b | modules.rb:41:3:42:5 | method_b |
+| private.rb:1:1:29:3 | E | private1 | private.rb:2:11:3:5 | private1 |
 | private.rb:1:1:29:3 | E | private2 | private.rb:8:3:9:5 | private2 |
 | private.rb:1:1:29:3 | E | private3 | private.rb:14:3:15:5 | private3 |
 | private.rb:1:1:29:3 | E | private4 | private.rb:17:3:18:5 | private4 |
 | private.rb:1:1:29:3 | E | public | private.rb:5:3:6:5 | public |
+| private.rb:42:1:60:3 | F | private1 | private.rb:43:11:44:5 | private1 |
 | private.rb:42:1:60:3 | F | private2 | private.rb:49:3:50:5 | private2 |
 | private.rb:42:1:60:3 | F | private3 | private.rb:55:3:56:5 | private3 |
 | private.rb:42:1:60:3 | F | private4 | private.rb:58:3:59:5 | private4 |
 | private.rb:42:1:60:3 | F | public | private.rb:46:3:47:5 | public |
+| private.rb:62:1:74:3 | PrivateOverride1 | call_m1 | private.rb:71:3:73:5 | call_m1 |
+| private.rb:62:1:74:3 | PrivateOverride1 | m1 | private.rb:63:11:65:5 | m1 |
+| private.rb:62:1:74:3 | PrivateOverride1 | m2 | private.rb:67:11:69:5 | m2 |
+| private.rb:76:1:82:3 | PrivateOverride2 | m1 | private.rb:77:11:81:5 | m1 |
 lookupMethod
 | calls.rb:21:1:34:3 | M | instance_m | calls.rb:22:5:24:7 | instance_m |
 | calls.rb:43:1:58:3 | C | add_singleton | calls.rb:364:1:368:3 | add_singleton |
@@ -405,17 +411,33 @@ lookupMethod
 | modules_rec.rb:4:1:5:3 | A::B | puts | calls.rb:102:5:102:30 | puts |
 | modules_rec.rb:4:1:5:3 | A::B | to_s | calls.rb:169:5:170:7 | to_s |
 | private.rb:1:1:29:3 | E | new | calls.rb:114:5:114:16 | new |
+| private.rb:1:1:29:3 | E | private1 | private.rb:2:11:3:5 | private1 |
 | private.rb:1:1:29:3 | E | private2 | private.rb:8:3:9:5 | private2 |
 | private.rb:1:1:29:3 | E | private3 | private.rb:14:3:15:5 | private3 |
 | private.rb:1:1:29:3 | E | private4 | private.rb:17:3:18:5 | private4 |
 | private.rb:1:1:29:3 | E | private_on_main | private.rb:31:1:32:3 | private_on_main |
 | private.rb:1:1:29:3 | E | public | private.rb:5:3:6:5 | public |
 | private.rb:1:1:29:3 | E | puts | calls.rb:102:5:102:30 | puts |
 | private.rb:1:1:29:3 | E | to_s | calls.rb:169:5:170:7 | to_s |
+| private.rb:42:1:60:3 | F | private1 | private.rb:43:11:44:5 | private1 |
 | private.rb:42:1:60:3 | F | private2 | private.rb:49:3:50:5 | private2 |
 | private.rb:42:1:60:3 | F | private3 | private.rb:55:3:56:5 | private3 |
 | private.rb:42:1:60:3 | F | private4 | private.rb:58:3:59:5 | private4 |
 | private.rb:42:1:60:3 | F | public | private.rb:46:3:47:5 | public |
+| private.rb:62:1:74:3 | PrivateOverride1 | call_m1 | private.rb:71:3:73:5 | call_m1 |
+| private.rb:62:1:74:3 | PrivateOverride1 | m1 | private.rb:63:11:65:5 | m1 |
+| private.rb:62:1:74:3 | PrivateOverride1 | m2 | private.rb:67:11:69:5 | m2 |
+| private.rb:62:1:74:3 | PrivateOverride1 | new | calls.rb:114:5:114:16 | new |
+| private.rb:62:1:74:3 | PrivateOverride1 | private_on_main | private.rb:31:1:32:3 | private_on_main |
+| private.rb:62:1:74:3 | PrivateOverride1 | puts | calls.rb:102:5:102:30 | puts |
+| private.rb:62:1:74:3 | PrivateOverride1 | to_s | calls.rb:169:5:170:7 | to_s |
+| private.rb:76:1:82:3 | PrivateOverride2 | call_m1 | private.rb:71:3:73:5 | call_m1 |
+| private.rb:76:1:82:3 | PrivateOverride2 | m1 | private.rb:77:11:81:5 | m1 |
+| private.rb:76:1:82:3 | PrivateOverride2 | m2 | private.rb:67:11:69:5 | m2 |
+| private.rb:76:1:82:3 | PrivateOverride2 | new | calls.rb:114:5:114:16 | new |
+| private.rb:76:1:82:3 | PrivateOverride2 | private_on_main | private.rb:31:1:32:3 | private_on_main |
+| private.rb:76:1:82:3 | PrivateOverride2 | puts | calls.rb:102:5:102:30 | puts |
+| private.rb:76:1:82:3 | PrivateOverride2 | to_s | calls.rb:169:5:170:7 | to_s |
 enclosingMethod
 | calls.rb:2:5:2:14 | call to puts | calls.rb:1:1:3:3 | foo |
 | calls.rb:2:5:2:14 | self | calls.rb:1:1:3:3 | foo |
@@ -703,3 +725,22 @@ enclosingMethod
 | hello.rb:20:30:20:34 | self | hello.rb:19:5:21:7 | message |
 | hello.rb:20:38:20:40 | "!" | hello.rb:19:5:21:7 | message |
 | hello.rb:20:39:20:39 | ! | hello.rb:19:5:21:7 | message |
+| private.rb:64:7:64:32 | call to puts | private.rb:63:11:65:5 | m1 |
+| private.rb:64:7:64:32 | self | private.rb:63:11:65:5 | m1 |
+| private.rb:64:12:64:32 | "PrivateOverride1#m1" | private.rb:63:11:65:5 | m1 |
+| private.rb:64:13:64:31 | PrivateOverride1#m1 | private.rb:63:11:65:5 | m1 |
+| private.rb:68:7:68:32 | call to puts | private.rb:67:11:69:5 | m2 |
+| private.rb:68:7:68:32 | self | private.rb:67:11:69:5 | m2 |
+| private.rb:68:12:68:32 | "PrivateOverride1#m2" | private.rb:67:11:69:5 | m2 |
+| private.rb:68:13:68:31 | PrivateOverride1#m2 | private.rb:67:11:69:5 | m2 |
+| private.rb:72:7:72:8 | call to m1 | private.rb:71:3:73:5 | call_m1 |
+| private.rb:72:7:72:8 | self | private.rb:71:3:73:5 | call_m1 |
+| private.rb:78:7:78:32 | call to puts | private.rb:77:11:81:5 | m1 |
+| private.rb:78:7:78:32 | self | private.rb:77:11:81:5 | m1 |
+| private.rb:78:12:78:32 | "PrivateOverride2#m1" | private.rb:77:11:81:5 | m1 |
+| private.rb:78:13:78:31 | PrivateOverride2#m1 | private.rb:77:11:81:5 | m1 |
+| private.rb:79:7:79:8 | call to m2 | private.rb:77:11:81:5 | m1 |
+| private.rb:79:7:79:8 | self | private.rb:77:11:81:5 | m1 |
+| private.rb:80:7:80:22 | PrivateOverride1 | private.rb:77:11:81:5 | m1 |
+| private.rb:80:7:80:26 | call to new | private.rb:77:11:81:5 | m1 |
+| private.rb:80:7:80:29 | call to m1 | private.rb:77:11:81:5 | m1 |