Merge pull request #7340 from github/hmac/private-methods

Ruby: handle private module methods

Author: hmac

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

@@ -35,36 +35,59 @@ class MethodBase extends Callable, BodyStmt, Scope, TMethodBase {
     or
     result = BodyStmt.super.getAChild(pred)
   }
-}
 
-/** A call to `private`. */
-private class Private extends MethodCall {
-  Private() { this.getMethodName() = "private" }
+  /** Holds if this method is private. */
+  predicate isPrivate() { none() }
+}
 
-  /** Gets the method that this `private` call applies to, if any */
-  Expr getMethod() { result = this.getArgument(0) }
+/**
+ * A method call which modifies another method in some way.
+ * For example, `private :foo` makes the method `foo` private.
+ */
+private class MethodModifier extends MethodCall {
+  /** Gets the name of the method that this call applies to. */
+  Expr getMethodArgument() { result = this.getArgument(0) }
 
-  /**
-   * Holds if this `private` call happens inside `c`, and refers to a
-   * method named `name`.
-   */
+  /** Holds if this call modifies a method with name `name` in namespace `n`. */
   pragma[noinline]
-  predicate isRef(ClassDeclaration c, string name) {
-    this = c.getAStmt() and
-    name = this.getMethod().(SymbolLiteral).getValueText()
+  predicate modifiesMethod(Namespace n, string name) {
+    this = n.getAStmt() and
+    [
+      this.getMethodArgument().(StringlikeLiteral).getValueText(),
+      this.getMethodArgument().(MethodBase).getName()
+    ] = name
   }
+}
 
-  /**
-   * Holds if this `private` call happens at position `i` inside `c`,
-   * and the call has no arguments.
-   */
-  pragma[noinline]
-  predicate hasNoArg(ClassDeclaration c, int i) {
-    this = c.getStmt(i) and
-    not exists(this.getMethod())
+/** A call to `private` or `private_class_method`. */
+private class Private extends MethodModifier {
+  private Namespace namespace;
+  private int position;
+
+  Private() { this.getMethodName() = "private" and namespace.getStmt(position) = this }
+
+  override predicate modifiesMethod(Namespace n, string name) {
+    n = namespace and
+    (
+      // def foo
+      // ...
+      // private :foo
+      super.modifiesMethod(n, name)
+      or
+      // private
+      // ...
+      // def foo
+      not exists(this.getMethodArgument()) and
+      exists(MethodBase m, int i | n.getStmt(i) = m and m.getName() = name and i > position)
+    )
   }
 }
 
+/** A call to `private_class_method`. */
+private class PrivateClassMethod extends MethodModifier {
+  PrivateClassMethod() { this.getMethodName() = "private_class_method" }
+}
+
 /** A normal method. */
 class Method extends MethodBase, TMethod {
   private Ruby::Method g;
@@ -90,12 +113,6 @@ class Method extends MethodBase, TMethod {
    */
   final predicate isSetter() { g.getName() instanceof Ruby::Setter }
 
-  pragma[noinline]
-  private predicate isDeclaredIn(ClassDeclaration c, string name) {
-    this = c.getAStmt() and
-    name = this.getName()
-  }
-
   /**
    * Holds if this method is private. All methods with the name prefix
    * `private` are private below:
@@ -122,18 +139,10 @@ class Method extends MethodBase, TMethod {
    * end
    * ```
    */
-  predicate isPrivate() {
-    this = any(Private p).getMethod()
-    or
-    exists(ClassDeclaration c, Private p, string name |
-      this.isDeclaredIn(c, name) and
-      p.isRef(c, name)
-    )
-    or
-    exists(ClassDeclaration c, Private p, int i, int j |
-      p.hasNoArg(c, i) and
-      this = c.getStmt(j) and
-      j > i
+  override predicate isPrivate() {
+    exists(Namespace n, string name |
+      any(Private p).modifiesMethod(n, name) and
+      isDeclaredIn(this, n, name)
     )
     or
     // Top-level methods are private members of the Object class
@@ -147,6 +156,14 @@ class Method extends MethodBase, TMethod {
   final override string toString() { result = this.getName() }
 }
 
+/**
+ * Holds if the method `m` has name `name` and is declared in namespace `n`.
+ */
+pragma[noinline]
+private predicate isDeclaredIn(MethodBase m, Namespace n, string name) {
+  n = m.getEnclosingModule() and name = m.getName()
+}
+
 /** A singleton method. */
 class SingletonMethod extends MethodBase, TSingletonMethod {
   private Ruby::SingletonMethod g;
@@ -175,6 +192,36 @@ class SingletonMethod extends MethodBase, TSingletonMethod {
     or
     pred = "getObject" and result = this.getObject()
   }
+
+  /**
+   * Holds if this method is private. All methods with the name prefix
+   * `private` are private below:
+   *
+   * ```rb
+   * class C
+   *   private_class_method def self.private1
+   *   end
+   *
+   *   def self.public
+   *   end
+   *
+   *   def self.private2
+   *   end
+   *   private_class_method :private2
+   *
+   *   private # this has no effect on singleton methods
+   *
+   *   def self.public2
+   *   end
+   * end
+   * ```
+   */
+  override predicate isPrivate() {
+    exists(Namespace n, string name |
+      any(PrivateClassMethod p).modifiesMethod(n, name) and
+      isDeclaredIn(this, n, name)
+    )
+  }
 }
 
 /**

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

@@ -49,12 +49,10 @@ calls.rb:
 
 #   15| M
 
-private.rb:
-#    1| C
+#   29| C
 #-----| super -> Object
 #-----| include -> M
 
-calls.rb:
 #   51| D
 #-----| super -> C
 
@@ -110,6 +108,13 @@ modules.rb:
 
 #  115| XX
 
+private.rb:
+#    1| E
+#-----| super -> Object
+
+#   42| F
+
+modules.rb:
 #    5| Foo::Bar
 
 #   19| Foo::ClassInFoo

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

@@ -78,13 +78,16 @@ getTarget
 | private.rb:2:3:3:5 | call to private | calls.rb:94:5:94:20 | private |
 | private.rb:10:3:10:19 | call to private | calls.rb:94:5:94:20 | private |
 | private.rb:12:3:12:9 | call to private | calls.rb:94:5:94:20 | private |
-| private.rb:24:1:24:5 | call to new | calls.rb:99:5:99:16 | new |
-| private.rb:25:1:25:5 | call to new | calls.rb:99:5:99:16 | new |
-| private.rb:26:1:26:5 | call to new | calls.rb:99:5:99:16 | new |
-| private.rb:27:1:27:5 | call to new | calls.rb:99:5:99:16 | new |
-| private.rb:28:1:28:5 | call to new | calls.rb:99:5:99:16 | new |
-| private.rb:28:1:28:12 | call to public | private.rb:5:3:6:5 | public |
-| private.rb:30:1:30:15 | call to private_on_main | private.rb:21:1:22:3 | private_on_main |
+| private.rb:34:1:34:5 | call to new | calls.rb:99:5:99:16 | new |
+| private.rb:35:1:35:5 | call to new | calls.rb:99:5:99:16 | new |
+| private.rb:36:1:36:5 | call to new | calls.rb:99:5:99:16 | new |
+| private.rb:37:1:37:5 | call to new | calls.rb:99:5:99:16 | new |
+| private.rb:38:1:38:5 | call to new | calls.rb:99:5:99:16 | new |
+| private.rb:38:1:38:12 | call to public | private.rb:5:3:6:5 | public |
+| private.rb:40:1:40:15 | call to private_on_main | private.rb:31:1:32:3 | private_on_main |
+| private.rb:43:3:44:5 | call to private | calls.rb:94:5:94:20 | private |
+| private.rb:51:3:51:19 | call to private | calls.rb:94:5:94:20 | private |
+| private.rb:53:3:53:9 | call to private | calls.rb:94:5:94:20 | private |
 unresolvedCall
 | calls.rb:19:5:19:14 | call to instance_m |
 | calls.rb:20:5:20:19 | call to instance_m |
@@ -110,10 +113,12 @@ unresolvedCall
 | hello.rb:20:16:20:26 | ... + ... |
 | hello.rb:20:16:20:34 | ... + ... |
 | hello.rb:20:16:20:40 | ... + ... |
-| private.rb:24:1:24:14 | call to private1 |
-| private.rb:25:1:25:14 | call to private2 |
-| private.rb:26:1:26:14 | call to private3 |
-| private.rb:27:1:27:14 | call to private4 |
+| private.rb:23:3:24:5 | call to private_class_method |
+| private.rb:28:3:28:32 | call to private_class_method |
+| private.rb:34:1:34:14 | call to private1 |
+| 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 |
 privateMethod
 | calls.rb:1:1:3:3 | foo |
 | calls.rb:62:1:65:3 | optional_arg |
@@ -126,4 +131,10 @@ privateMethod
 | private.rb:8:3:9:5 | private2 |
 | private.rb:14:3:15:5 | private3 |
 | private.rb:17:3:18:5 | private4 |
-| private.rb:21:1:22:3 | private_on_main |
+| private.rb:23:24:24:5 | private5 |
+| private.rb:26:3:27:5 | private6 |
+| private.rb:31:1:32:3 | private_on_main |
+| private.rb:43:11:44:5 | private1 |
+| private.rb:49:3:50:5 | private2 |
+| private.rb:55:3:56:5 | private3 |
+| private.rb:58:3:59:5 | private4 |

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

@@ -4,4 +4,4 @@ query Callable getTarget(Call call) { result = call.getATarget() }
 
 query predicate unresolvedCall(Call call) { not exists(call.getATarget()) }
 
-query predicate privateMethod(Method m) { m.isPrivate() }
+query predicate privateMethod(MethodBase m) { m.isPrivate() }

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

@@ -1,5 +1,6 @@
 getMethod
 | calls.rb:15:1:24:3 | M | instance_m | calls.rb:16:5:16:23 | instance_m |
+| calls.rb:29:1:44:3 | C | baz | calls.rb:37:5:43:7 | baz |
 | calls.rb:51:1:55:3 | D | baz | calls.rb:52:5:54:7 | baz |
 | calls.rb:77:1:80:3 | Integer | abs | calls.rb:79:5:79:16 | abs |
 | calls.rb:77:1:80:3 | Integer | bit_length | calls.rb:78:5:78:23 | bit_length |
@@ -17,7 +18,7 @@ getMethod
 | calls.rb:97:1:100:3 | Object | new | calls.rb:99:5:99:16 | new |
 | calls.rb:97:1:100:3 | Object | optional_arg | calls.rb:62:1:65:3 | optional_arg |
 | calls.rb:97:1:100:3 | Object | private_on_main | calls.rb:164:1:165:3 | private_on_main |
-| calls.rb:97:1:100:3 | Object | private_on_main | private.rb:21:1:22:3 | private_on_main |
+| calls.rb:97:1:100:3 | Object | private_on_main | private.rb:31:1:32:3 | private_on_main |
 | calls.rb:102:1:104:3 | Hash | [] | calls.rb:103:5:103:15 | [] |
 | calls.rb:106:1:117:3 | Array | [] | calls.rb:107:3:107:13 | [] |
 | calls.rb:106:1:117:3 | Array | foreach | calls.rb:110:3:116:5 | foreach |
@@ -35,13 +36,27 @@ 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:19:3 | C | baz | calls.rb:37:5:43:7 | baz |
-| private.rb:1:1:19:3 | C | private2 | private.rb:8:3:9:5 | private2 |
-| private.rb:1:1:19:3 | C | private3 | private.rb:14:3:15:5 | private3 |
-| private.rb:1:1:19:3 | C | private4 | private.rb:17:3:18:5 | private4 |
-| private.rb:1:1:19:3 | C | public | private.rb:5:3:6:5 | public |
+| 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 | 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 |
 lookupMethod
 | calls.rb:15:1:24:3 | M | instance_m | calls.rb:16:5:16:23 | instance_m |
+| calls.rb:29:1:44:3 | C | baz | calls.rb:37:5:43:7 | baz |
+| calls.rb:29:1:44:3 | C | call_block | calls.rb:67:1:69:3 | call_block |
+| calls.rb:29:1:44:3 | C | foo | calls.rb:1:1:3:3 | foo |
+| calls.rb:29:1:44:3 | C | foo | calls.rb:71:1:75:3 | foo |
+| calls.rb:29:1:44:3 | C | funny | calls.rb:119:1:121:3 | funny |
+| calls.rb:29:1:44:3 | C | indirect | calls.rb:137:1:139:3 | indirect |
+| calls.rb:29:1:44:3 | C | instance_m | calls.rb:16:5:16:23 | instance_m |
+| calls.rb:29:1:44:3 | C | new | calls.rb:99:5:99:16 | new |
+| calls.rb:29:1:44:3 | C | optional_arg | calls.rb:62:1:65:3 | optional_arg |
+| calls.rb:29:1:44:3 | C | private_on_main | calls.rb:164:1:165:3 | private_on_main |
+| calls.rb:29:1:44:3 | C | puts | calls.rb:87:5:87:17 | puts |
 | calls.rb:51:1:55:3 | D | baz | calls.rb:52:5:54:7 | baz |
 | calls.rb:51:1:55:3 | D | call_block | calls.rb:67:1:69:3 | call_block |
 | calls.rb:51:1:55:3 | D | foo | calls.rb:1:1:3:3 | foo |
@@ -51,11 +66,7 @@ lookupMethod
 | calls.rb:51:1:55:3 | D | instance_m | calls.rb:16:5:16:23 | instance_m |
 | calls.rb:51:1:55:3 | D | new | calls.rb:99:5:99:16 | new |
 | calls.rb:51:1:55:3 | D | optional_arg | calls.rb:62:1:65:3 | optional_arg |
-| calls.rb:51:1:55:3 | D | private2 | private.rb:8:3:9:5 | private2 |
-| calls.rb:51:1:55:3 | D | private3 | private.rb:14:3:15:5 | private3 |
-| calls.rb:51:1:55:3 | D | private4 | private.rb:17:3:18:5 | private4 |
 | calls.rb:51:1:55:3 | D | private_on_main | calls.rb:164:1:165:3 | private_on_main |
-| calls.rb:51:1:55:3 | D | public | private.rb:5:3:6:5 | public |
 | calls.rb:51:1:55:3 | D | puts | calls.rb:87:5:87:17 | puts |
 | calls.rb:77:1:80:3 | Integer | abs | calls.rb:79:5:79:16 | abs |
 | calls.rb:77:1:80:3 | Integer | bit_length | calls.rb:78:5:78:23 | bit_length |
@@ -93,7 +104,7 @@ lookupMethod
 | calls.rb:97:1:100:3 | Object | new | calls.rb:99:5:99:16 | new |
 | calls.rb:97:1:100:3 | Object | optional_arg | calls.rb:62:1:65:3 | optional_arg |
 | calls.rb:97:1:100:3 | Object | private_on_main | calls.rb:164:1:165:3 | private_on_main |
-| calls.rb:97:1:100:3 | Object | private_on_main | private.rb:21:1:22:3 | private_on_main |
+| calls.rb:97:1:100:3 | Object | private_on_main | private.rb:31:1:32:3 | private_on_main |
 | calls.rb:97:1:100:3 | Object | puts | calls.rb:87:5:87:17 | puts |
 | calls.rb:102:1:104:3 | Hash | [] | calls.rb:103:5:103:15 | [] |
 | calls.rb:102:1:104:3 | Hash | call_block | calls.rb:67:1:69:3 | call_block |
@@ -205,19 +216,14 @@ lookupMethod
 | modules.rb:112:1:113:3 | YY | puts | calls.rb:87:5:87:17 | puts |
 | modules.rb:116:7:117:9 | XX::YY | new | calls.rb:99:5:99:16 | new |
 | modules.rb:116:7:117:9 | XX::YY | puts | calls.rb:87:5:87:17 | puts |
-| private.rb:1:1:19:3 | C | baz | calls.rb:37:5:43:7 | baz |
-| private.rb:1:1:19:3 | C | call_block | calls.rb:67:1:69:3 | call_block |
-| private.rb:1:1:19:3 | C | foo | calls.rb:1:1:3:3 | foo |
-| private.rb:1:1:19:3 | C | foo | calls.rb:71:1:75:3 | foo |
-| private.rb:1:1:19:3 | C | funny | calls.rb:119:1:121:3 | funny |
-| private.rb:1:1:19:3 | C | indirect | calls.rb:137:1:139:3 | indirect |
-| private.rb:1:1:19:3 | C | instance_m | calls.rb:16:5:16:23 | instance_m |
-| private.rb:1:1:19:3 | C | new | calls.rb:99:5:99:16 | new |
-| private.rb:1:1:19:3 | C | optional_arg | calls.rb:62:1:65:3 | optional_arg |
-| private.rb:1:1:19:3 | C | private2 | private.rb:8:3:9:5 | private2 |
-| private.rb:1:1:19:3 | C | private3 | private.rb:14:3:15:5 | private3 |
-| private.rb:1:1:19:3 | C | private4 | private.rb:17:3:18:5 | private4 |
-| private.rb:1:1:19:3 | C | private_on_main | calls.rb:164:1:165:3 | private_on_main |
-| private.rb:1:1:19:3 | C | private_on_main | private.rb:21:1:22:3 | private_on_main |
-| private.rb:1:1:19:3 | C | public | private.rb:5:3:6:5 | public |
-| private.rb:1:1:19:3 | C | puts | calls.rb:87:5:87:17 | puts |
+| private.rb:1:1:29:3 | E | new | calls.rb:99:5:99:16 | new |
+| 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:87:5:87:17 | puts |
+| 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 |