Ruby: minor overhaul of ActiveResource model

asgerf · asgerf · commit e3a04499f630 · 2023-06-19T12:15:57.000+02:00
diff --git a/ruby/ql/lib/codeql/ruby/frameworks/ActiveResource.qll b/ruby/ql/lib/codeql/ruby/frameworks/ActiveResource.qll
@@ -18,8 +18,12 @@ module ActiveResource {
    * An ActiveResource model class. This is any (transitive) subclass of ActiveResource.
    */
   pragma[nomagic]
-  private API::Node modelApiNode() {
-    result = API::getTopLevelMember("ActiveResource").getMember("Base").getASubclass()
+  private API::Node activeResourceBaseClass() {
+    result = API::getTopLevelMember("ActiveResource").getMember("Base")
+  }
+
+  private DataFlow::ClassNode activeResourceClass() {
+    result = activeResourceBaseClass().getADescendentModule()
   }
 
   /**
@@ -30,16 +34,8 @@ module ActiveResource {
    * end
    * ```
    */
-  class ModelClass extends ClassDeclaration {
-    API::Node model;
-
-    ModelClass() {
-      model = modelApiNode() and
-      this.getSuperclassExpr() = model.getAValueReachableFromSource().asExpr().getExpr()
-    }
-
-    /** Gets the API node for this model */
-    API::Node getModelApiNode() { result = model }
+  class ModelClassNode extends DataFlow::ClassNode {
+    ModelClassNode() { this = activeResourceClass() }
 
     /** Gets a call to `site=`, which sets the base URL for this model. */
     SiteAssignCall getASiteAssignment() { result.getModelClass() = this }
@@ -49,6 +45,46 @@ module ActiveResource {
       c = this.getASiteAssignment() and
       c.disablesCertificateValidation()
     }
+
+    /** Gets a method call on this class that returns an instance of the class. */
+    private DataFlow::CallNode getAChainedCall() {
+      result.(FindCall).getModelClass() = this
+      or
+      result.(CreateCall).getModelClass() = this
+      or
+      result.(CustomHttpCall).getModelClass() = this
+      or
+      result.(CollectionCall).getCollection().getModelClass() = this and
+      result.getMethodName() = ["first", "last"]
+    }
+
+    /** Gets an API node referring to an instance of this class. */
+    API::Node getAnInstanceReference() {
+      result = this.trackInstance()
+      or
+      result = this.getAChainedCall().track()
+    }
+  }
+
+  /** DEPRECATED. Use `ModelClassNode` instead. */
+  deprecated class ModelClass extends ClassDeclaration {
+    private ModelClassNode cls;
+
+    ModelClass() { this = cls.getADeclaration() }
+
+    /** Gets the class for which this is a declaration. */
+    ModelClassNode getClassNode() { result = cls }
+
+    /** Gets the API node for this class object. */
+    deprecated API::Node getModelApiNode() { result = cls.trackModule() }
+
+    /** Gets a call to `site=`, which sets the base URL for this model. */
+    SiteAssignCall getASiteAssignment() { result = cls.getASiteAssignment() }
+
+    /** Holds if `c` sets a base URL which does not use HTTPS. */
+    predicate disablesCertificateValidation(SiteAssignCall c) {
+      cls.disablesCertificateValidation(c)
+    }
   }
 
   /**
@@ -62,38 +98,31 @@ module ActiveResource {
    * ```
    */
   class ModelClassMethodCall extends DataFlow::CallNode {
-    API::Node model;
+    private ModelClassNode cls;
 
-    ModelClassMethodCall() {
-      model = modelApiNode() and
-      this = classMethodCall(model, _)
-    }
+    ModelClassMethodCall() { this = cls.trackModule().getAMethodCall(_) }
 
     /** Gets the model class for this call. */
-    ModelClass getModelClass() { result.getModelApiNode() = model }
+    ModelClassNode getModelClass() { result = cls }
   }
 
   /**
    * A call to `site=` on an ActiveResource model class.
    * This sets the base URL for all HTTP requests made by this class.
    */
-  private class SiteAssignCall extends DataFlow::CallNode {
-    API::Node model;
-
-    SiteAssignCall() { model = modelApiNode() and this = classMethodCall(model, "site=") }
+  private class SiteAssignCall extends ModelClassMethodCall {
+    SiteAssignCall() { this.getMethodName() = "site=" }
 
     /**
      * Gets a node that contributes to the URLs used for HTTP requests by the parent
      * class.
      */
     DataFlow::Node getAUrlPart() { result = this.getArgument(0) }
 
-    /** Gets the model class for this call. */
-    ModelClass getModelClass() { result.getModelApiNode() = model }
-
     /** Holds if this site value specifies HTTP rather than HTTPS. */
     predicate disablesCertificateValidation() {
       this.getAUrlPart()
+          // TODO: We should not need all this just to get the string value
           .asExpr()
           .(ExprNodes::AssignExprCfgNode)
           .getRhs()
@@ -141,87 +170,70 @@ module ActiveResource {
   }
 
   /**
+   * DEPRECATED. Use `ModelClassNode.getAnInstanceReference()` instead.
+   *
    * An ActiveResource model object.
    */
-  class ModelInstance extends DataFlow::Node {
-    ModelClass cls;
-
-    ModelInstance() {
-      exists(API::Node model | model = modelApiNode() |
-        this = model.getInstance().getAValueReachableFromSource() and
-        cls.getModelApiNode() = model
-      )
-      or
-      exists(FindCall call | call.flowsTo(this) | cls = call.getModelClass())
-      or
-      exists(CreateCall call | call.flowsTo(this) | cls = call.getModelClass())
-      or
-      exists(CustomHttpCall call | call.flowsTo(this) | cls = call.getModelClass())
-      or
-      exists(CollectionCall call |
-        call.getMethodName() = ["first", "last"] and
-        call.flowsTo(this)
-      |
-        cls = call.getCollection().getModelClass()
-      )
-    }
+  deprecated class ModelInstance extends DataFlow::Node {
+    private ModelClassNode cls;
+
+    ModelInstance() { this = cls.getAnInstanceReference().getAValueReachableFromSource() }
 
     /** Gets the model class for this instance. */
-    ModelClass getModelClass() { result = cls }
+    ModelClassNode getModelClass() { result = cls }
   }
 
   /**
    * A call to a method on an ActiveResource model object.
    */
   class ModelInstanceMethodCall extends DataFlow::CallNode {
-    ModelInstance i;
+    private ModelClassNode cls;
 
-    ModelInstanceMethodCall() { this.getReceiver() = i }
+    ModelInstanceMethodCall() { this = cls.getAnInstanceReference().getAMethodCall(_) }
 
     /** Gets the model instance for this call. */
-    ModelInstance getInstance() { result = i }
+    deprecated ModelInstance getInstance() { result = this.getReceiver() }
 
     /** Gets the model class for this call. */
-    ModelClass getModelClass() { result = i.getModelClass() }
+    ModelClassNode getModelClass() { result = cls }
   }
 
   /**
-   * A collection of ActiveResource model objects.
+   * DEPRECATED. Use `CollectionSource` instead.
+   *
+   * A data flow node that may refer to a collection of ActiveResource model objects.
    */
-  class Collection extends DataFlow::Node {
-    ModelClassMethodCall classMethodCall;
-
-    Collection() {
-      classMethodCall.flowsTo(this) and
-      (
-        classMethodCall.getMethodName() = "all"
-        or
-        classMethodCall.getMethodName() = "find" and
-        classMethodCall.getArgument(0).asExpr().getConstantValue().isStringlikeValue("all")
-      )
-    }
+  deprecated class Collection extends DataFlow::Node {
+    Collection() { this = any(CollectionSource src).track().getAValueReachableFromSource() }
+  }
 
-    /** Gets the model class for this collection. */
-    ModelClass getModelClass() { result = classMethodCall.getModelClass() }
+  /**
+   * A call that returns a collection of ActiveResource model objects.
+   */
+  class CollectionSource extends ModelClassMethodCall {
+    CollectionSource() {
+      this.getMethodName() = "all"
+      or
+      this.getArgument(0).asExpr().getConstantValue().isStringlikeValue("all")
+    }
   }
 
   /**
    * A method call on a collection.
    */
   class CollectionCall extends DataFlow::CallNode {
-    CollectionCall() { this.getReceiver() instanceof Collection }
+    private CollectionSource collection;
+
+    CollectionCall() { this = collection.track().getAMethodCall(_) }
 
     /** Gets the collection for this call. */
-    Collection getCollection() { result = this.getReceiver() }
+    CollectionSource getCollection() { result = collection }
   }
 
   private class ModelClassMethodCallAsHttpRequest extends Http::Client::Request::Range,
     ModelClassMethodCall
   {
-    ModelClass cls;
-
     ModelClassMethodCallAsHttpRequest() {
-      this.getModelClass() = cls and
       this.getMethodName() = ["all", "build", "create", "create!", "find", "first", "last"]
     }
 
@@ -230,23 +242,22 @@ module ActiveResource {
     override predicate disablesCertificateValidation(
       DataFlow::Node disablingNode, DataFlow::Node argumentOrigin
     ) {
-      cls.disablesCertificateValidation(disablingNode) and
+      this.getModelClass().disablesCertificateValidation(disablingNode) and
       // TODO: highlight real argument origin
       argumentOrigin = disablingNode
     }
 
-    override DataFlow::Node getAUrlPart() { result = cls.getASiteAssignment().getAUrlPart() }
+    override DataFlow::Node getAUrlPart() {
+      result = this.getModelClass().getASiteAssignment().getAUrlPart()
+    }
 
     override DataFlow::Node getResponseBody() { result = this }
   }
 
   private class ModelInstanceMethodCallAsHttpRequest extends Http::Client::Request::Range,
     ModelInstanceMethodCall
   {
-    ModelClass cls;
-
     ModelInstanceMethodCallAsHttpRequest() {
-      this.getModelClass() = cls and
       this.getMethodName() =
         [
           "exists?", "reload", "save", "save!", "destroy", "delete", "get", "patch", "post", "put",
@@ -259,42 +270,15 @@ module ActiveResource {
     override predicate disablesCertificateValidation(
       DataFlow::Node disablingNode, DataFlow::Node argumentOrigin
     ) {
-      cls.disablesCertificateValidation(disablingNode) and
+      this.getModelClass().disablesCertificateValidation(disablingNode) and
       // TODO: highlight real argument origin
       argumentOrigin = disablingNode
     }
 
-    override DataFlow::Node getAUrlPart() { result = cls.getASiteAssignment().getAUrlPart() }
+    override DataFlow::Node getAUrlPart() {
+      result = this.getModelClass().getASiteAssignment().getAUrlPart()
+    }
 
     override DataFlow::Node getResponseBody() { result = this }
   }
-
-  /**
-   * A call to a class method.
-   *
-   * TODO: is this general enough to be useful elsewhere?
-   *
-   * Examples:
-   * ```rb
-   * class A
-   *   def self.m; end
-   *
-   *   m # call
-   * end
-   *
-   * A.m # call
-   * ```
-   */
-  private DataFlow::CallNode classMethodCall(API::Node classNode, string methodName) {
-    // A.m
-    result = classNode.getAMethodCall(methodName)
-    or
-    // class A
-    //   A.m
-    //  end
-    result.getReceiver().asExpr() instanceof ExprNodes::SelfVariableAccessCfgNode and
-    result.asExpr().getExpr().getEnclosingModule().(ClassDeclaration).getSuperclassExpr() =
-      classNode.getAValueReachableFromSource().asExpr().getExpr() and
-    result.getMethodName() = methodName
-  }
 }
diff --git a/ruby/ql/test/library-tests/frameworks/active_resource/ActiveResource.expected b/ruby/ql/test/library-tests/frameworks/active_resource/ActiveResource.expected
@@ -33,6 +33,13 @@ modelInstances
 | active_resource.rb:26:9:26:14 | people |
 | active_resource.rb:26:9:26:20 | call to first |
 | active_resource.rb:27:1:27:5 | alice |
+modelInstancesAsSource
+| active_resource.rb:1:1:3:3 | Person | active_resource.rb:5:9:5:33 | call to new |
+| active_resource.rb:1:1:3:3 | Person | active_resource.rb:8:9:8:22 | call to find |
+| active_resource.rb:1:1:3:3 | Person | active_resource.rb:16:1:16:23 | call to new |
+| active_resource.rb:1:1:3:3 | Person | active_resource.rb:18:1:18:22 | call to get |
+| active_resource.rb:1:1:3:3 | Person | active_resource.rb:24:10:24:26 | call to find |
+| active_resource.rb:1:1:3:3 | Person | active_resource.rb:26:9:26:20 | call to first |
 modelInstanceMethodCalls
 | active_resource.rb:6:1:6:10 | call to save |
 | active_resource.rb:9:1:9:13 | call to address= |
@@ -50,3 +57,6 @@ collections
 | active_resource.rb:24:1:24:26 | ... = ... |
 | active_resource.rb:24:10:24:26 | call to find |
 | active_resource.rb:26:9:26:14 | people |
+collectionSources
+| active_resource.rb:23:10:23:19 | call to all |
+| active_resource.rb:24:10:24:26 | call to find |
diff --git a/ruby/ql/test/library-tests/frameworks/active_resource/ActiveResource.ql b/ruby/ql/test/library-tests/frameworks/active_resource/ActiveResource.ql
@@ -3,7 +3,8 @@ import codeql.ruby.DataFlow
 import codeql.ruby.frameworks.ActiveResource
 
 query predicate modelClasses(
-  ActiveResource::ModelClass c, DataFlow::Node siteAssignCall, boolean disablesCertificateValidation
+  ActiveResource::ModelClassNode c, DataFlow::Node siteAssignCall,
+  boolean disablesCertificateValidation
 ) {
   c.getASiteAssignment() = siteAssignCall and
   if c.disablesCertificateValidation(siteAssignCall)
@@ -13,8 +14,16 @@ query predicate modelClasses(
 
 query predicate modelClassMethodCalls(ActiveResource::ModelClassMethodCall c) { any() }
 
-query predicate modelInstances(ActiveResource::ModelInstance c) { any() }
+deprecated query predicate modelInstances(ActiveResource::ModelInstance c) { any() }
+
+query predicate modelInstancesAsSource(
+  ActiveResource::ModelClassNode cls, DataFlow::LocalSourceNode node
+) {
+  node = cls.getAnInstanceReference().asSource()
+}
 
 query predicate modelInstanceMethodCalls(ActiveResource::ModelInstanceMethodCall c) { any() }
 
-query predicate collections(ActiveResource::Collection c) { any() }
+deprecated query predicate collections(ActiveResource::Collection c) { any() }
+
+query predicate collectionSources(ActiveResource::CollectionSource c) { any() }
