Merge pull request #9341 from alexrford/ruby/activerecordinstance-public

Ruby: Make `ActiveRecordInstance` public and fix some misidentifications

Author: alexrford

ruby/ql/lib/codeql/ruby/frameworks/ActiveRecord.qll

@@ -275,7 +275,18 @@ private class ActiveRecordModelFinderCall extends ActiveRecordModelInstantiation
     exists(MethodCall call, Expr recv |
       call = this.asExpr().getExpr() and
       recv = getUltimateReceiver(call) and
-      resolveConstant(recv) = cls.getAQualifiedName() and
+      (
+        // The receiver refers to an `ActiveRecordModelClass` by name
+        resolveConstant(recv) = cls.getAQualifiedName()
+        or
+        // The receiver is self, and the call is within a singleton method of
+        // the `ActiveRecordModelClass`
+        recv instanceof SelfVariableAccess and
+        exists(SingletonMethod callScope |
+          callScope = call.getCfgScope() and
+          callScope = cls.getAMethod()
+        )
+      ) and
       call.getMethodName() = finderMethodName()
     )
   }
@@ -293,18 +304,24 @@ private class ActiveRecordModelClassSelfReference extends ActiveRecordModelInsta
       m = this.getCfgScope() and
       m.getEnclosingModule() = cls and
       m = cls.getAMethod()
-    )
+    ) and
+    // In a singleton method, `self` refers to the class itself rather than an
+    // instance of that class
+    not this.getSelfScope() instanceof SingletonMethod
   }
 
   final override ActiveRecordModelClass getClass() { result = cls }
 }
 
-// A (locally tracked) active record model object
-private class ActiveRecordInstance extends DataFlow::Node {
+/**
+ * An instance of an `ActiveRecord` model object.
+ */
+class ActiveRecordInstance extends DataFlow::Node {
   private ActiveRecordModelInstantiation instantiation;
 
   ActiveRecordInstance() { this = instantiation or instantiation.flowsTo(this) }
 
+  /** Gets the `ActiveRecordModelClass` that this is an instance of. */
   ActiveRecordModelClass getClass() { result = instantiation.getClass() }
 }
 

ruby/ql/test/library-tests/frameworks/ActiveRecord.expected

@@ -2,6 +2,14 @@ activeRecordModelClasses
 | ActiveRecordInjection.rb:1:1:3:3 | UserGroup |
 | ActiveRecordInjection.rb:5:1:17:3 | User |
 | ActiveRecordInjection.rb:19:1:25:3 | Admin |
+activeRecordInstances
+| ActiveRecordInjection.rb:10:5:10:68 | call to find |
+| ActiveRecordInjection.rb:15:5:15:40 | call to find_by |
+| ActiveRecordInjection.rb:79:5:81:7 | if ... |
+| ActiveRecordInjection.rb:79:43:80:40 | then ... |
+| ActiveRecordInjection.rb:80:7:80:40 | call to find_by |
+| ActiveRecordInjection.rb:85:5:85:33 | call to find_by |
+| ActiveRecordInjection.rb:88:5:88:34 | call to find |
 activeRecordSqlExecutionRanges
 | ActiveRecordInjection.rb:10:33:10:67 | "name='#{...}' and pass='#{...}'" |
 | ActiveRecordInjection.rb:23:16:23:24 | condition |
@@ -45,9 +53,8 @@ potentiallyUnsafeSqlExecutingMethodCall
 | ActiveRecordInjection.rb:75:5:75:29 | call to order |
 | ActiveRecordInjection.rb:80:7:80:40 | call to find_by |
 activeRecordModelInstantiations
-| ActiveRecordInjection.rb:8:3:11:5 | self (authenticate) | ActiveRecordInjection.rb:5:1:17:3 | User |
+| ActiveRecordInjection.rb:10:5:10:68 | call to find | ActiveRecordInjection.rb:5:1:17:3 | User |
 | ActiveRecordInjection.rb:15:5:15:40 | call to find_by | ActiveRecordInjection.rb:1:1:3:3 | UserGroup |
-| ActiveRecordInjection.rb:20:3:24:5 | self (delete_by) | ActiveRecordInjection.rb:19:1:25:3 | Admin |
 | ActiveRecordInjection.rb:80:7:80:40 | call to find_by | ActiveRecordInjection.rb:5:1:17:3 | User |
 | ActiveRecordInjection.rb:85:5:85:33 | call to find_by | ActiveRecordInjection.rb:5:1:17:3 | User |
 | ActiveRecordInjection.rb:88:5:88:34 | call to find | ActiveRecordInjection.rb:5:1:17:3 | User |

ruby/ql/test/library-tests/frameworks/ActiveRecord.ql

@@ -3,6 +3,8 @@ import codeql.ruby.frameworks.ActiveRecord
 
 query predicate activeRecordModelClasses(ActiveRecordModelClass cls) { any() }
 
+query predicate activeRecordInstances(ActiveRecordInstance i) { any() }
+
 query predicate activeRecordSqlExecutionRanges(ActiveRecordSqlExecutionRange range) { any() }
 
 query predicate activeRecordModelClassMethodCalls(ActiveRecordModelClassMethodCall call) { any() }