Ruby: replace OrmWriteAccess with PersistentWriteAccess concept

Author: alexrford

ruby/ql/lib/codeql/ruby/Concepts.qll

@@ -626,33 +626,28 @@ module OrmInstantiation {
 }
 
 /**
- * A data-flow node that may represent a write to the database in an ORM system.
+ * A data flow node that writes persistent data.
  *
  * Extend this class to refine existing API models. If you want to model new APIs,
- * extend `OrmWriteAccess::Range` instead.
+ * extend `PersistentWriteAccess::Range` instead.
  */
-class OrmWriteAccess extends DataFlow::Node instanceof OrmWriteAccess::Range {
-  /**
-   * Gets the name of a field that is assigned to `value` by this write.
-   */
-  string getFieldNameAssignedTo(DataFlow::Node value) {
-    result = super.getFieldNameAssignedTo(value)
-  }
+class PersistentWriteAccess extends DataFlow::Node instanceof PersistentWriteAccess::Range {
+  DataFlow::Node getValue() { result = super.getValue() }
 }
 
-/** Provides a class for modeling new ORM write access APIs. */
-module OrmWriteAccess {
+/** Provides a class for modeling new persistent write access APIs. */
+module PersistentWriteAccess {
   /**
-   * A data-flow node that may represent a write to the database in an ORM system.
+   * A data flow node that writes persistent data.
    *
    * Extend this class to model new APIs. If you want to refine existing API models,
-   * extend `OrmWriteAccess` instead.
+   * extend `PersistentWriteAccess` instead.
    */
   abstract class Range extends DataFlow::Node {
     /**
-     * Gets the name of a field that is assigned to `value` by this write.
+     * Gets the data flow node corresponding to the written value.
      */
-    abstract string getFieldNameAssignedTo(DataFlow::Node value);
+    abstract DataFlow::Node getValue();
   }
 }
 

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

@@ -323,7 +323,7 @@ private module Persistence {
    * A call to a method that may modify or create a model object and write it to
    * the database. Examples include `create`, `insert`, and `update`.
    */
-  abstract private class ModifyAndSaveCall extends DataFlow::CallNode, OrmWriteAccess::Range {
+  abstract private class ModifyAndSaveCall extends DataFlow::CallNode, PersistentWriteAccess::Range {
     /**
      * Holds if the given key-value pair is set on an object by this call.
      */
@@ -334,14 +334,11 @@ private module Persistence {
      */
     abstract ActiveRecordModelClass getClass();
 
-    final override string getFieldNameAssignedTo(DataFlow::Node value) {
+    final override DataFlow::Node getValue() {
       exists(ExprCfgNode keyExpr, ExprCfgNode valueExpr |
         this.setsKeyValuePair(keyExpr, valueExpr)
       |
-        keyExpr.getConstantValue().isStringOrSymbol(result) and
-        // avoid vacuous matches where the key is not a string or not a symbol
-        not result = "" and
-        value.asExpr() = valueExpr
+        result.asExpr() = valueExpr
       )
     }
   }
@@ -480,24 +477,21 @@ private module Persistence {
   /**
    * An assignment like `user.name = "foo"`. Though this does not write to the
    * database without a subsequent call to persist the object, it is considered
-   * as an `OrmWriteAccess` to avoid missing cases where the path to a
+   * as an `PersistentWriteAccess` to avoid missing cases where the path to a
    * subsequent write is not clear.
    */
-  private class AssignAttribute extends DataFlow::Node, OrmWriteAccess::Range {
-    private DataFlow::CallNode setter;
+  private class AssignAttribute extends DataFlow::Node, PersistentWriteAccess::Range {
     private ExprNodes::AssignExprCfgNode assignNode;
 
     AssignAttribute() {
-      assignNode = this.asExpr() and
-      setter.getArgument(0) = this and
-      setter instanceof ActiveRecordInstanceMethodCall and
-      setter.asExpr().getExpr() instanceof SetterMethodCall
+      exists(DataFlow::CallNode setter |
+        assignNode = this.asExpr() and
+        setter.getArgument(0) = this and
+        setter instanceof ActiveRecordInstanceMethodCall and
+        setter.asExpr().getExpr() instanceof SetterMethodCall
+      )
     }
 
-    override string getFieldNameAssignedTo(DataFlow::Node value) {
-      result + "=" = setter.getMethodName() and
-      // match RHS
-      assignNode.getRhs() = value.asExpr()
-    }
+    override DataFlow::Node getValue() { assignNode.getRhs() = result.asExpr() }
   }
 }

ruby/ql/test/library-tests/concepts/PersistentWriteAccess.expected

@@ -0,0 +1,16 @@
+| app/controllers/users_controller.rb:5:7:5:44 | call to create! | app/controllers/users_controller.rb:5:26:5:29 | "U1" |
+| app/controllers/users_controller.rb:5:7:5:44 | call to create! | app/controllers/users_controller.rb:5:37:5:43 | call to get_uid |
+| app/controllers/users_controller.rb:6:7:6:29 | call to create | app/controllers/users_controller.rb:6:25:6:28 | "U2" |
+| app/controllers/users_controller.rb:7:7:7:31 | call to insert | app/controllers/users_controller.rb:7:26:7:29 | "U3" |
+| app/controllers/users_controller.rb:10:7:10:32 | call to update | app/controllers/users_controller.rb:10:28:10:31 | "U4" |
+| app/controllers/users_controller.rb:11:7:11:73 | call to update! | app/controllers/users_controller.rb:11:39:11:42 | "U5" |
+| app/controllers/users_controller.rb:11:7:11:73 | call to update! | app/controllers/users_controller.rb:11:53:11:56 | "U6" |
+| app/controllers/users_controller.rb:11:7:11:73 | call to update! | app/controllers/users_controller.rb:11:67:11:70 | "U7" |
+| app/controllers/users_controller.rb:14:7:14:66 | call to insert_all | app/controllers/users_controller.rb:14:31:14:34 | "U8" |
+| app/controllers/users_controller.rb:14:7:14:66 | call to insert_all | app/controllers/users_controller.rb:14:45:14:48 | "U9" |
+| app/controllers/users_controller.rb:14:7:14:66 | call to insert_all | app/controllers/users_controller.rb:14:59:14:63 | "U10" |
+| app/controllers/users_controller.rb:19:7:19:30 | call to update | app/controllers/users_controller.rb:19:25:19:29 | "U11" |
+| app/controllers/users_controller.rb:20:7:20:57 | call to update_attributes | app/controllers/users_controller.rb:20:37:20:41 | "U12" |
+| app/controllers/users_controller.rb:20:7:20:57 | call to update_attributes | app/controllers/users_controller.rb:20:49:20:55 | call to get_uid |
+| app/controllers/users_controller.rb:23:7:23:42 | call to update_attribute | app/controllers/users_controller.rb:23:37:23:41 | "U13" |
+| app/controllers/users_controller.rb:26:7:26:15 | ... = ... | app/controllers/users_controller.rb:26:19:26:23 | "U14" |

ruby/ql/test/library-tests/concepts/PersistentWriteAccess.ql

@@ -0,0 +1,6 @@
+import codeql.ruby.DataFlow
+import codeql.ruby.Concepts
+
+query predicate persistentWriteAccesses(PersistentWriteAccess acc, DataFlow::Node value) {
+  value = acc.getValue()
+}

ruby/ql/test/library-tests/frameworks/app/controllers/users/users_controller.rb renamed to ruby/ql/test/library-tests/concepts/app/controllers/users_controller.rb

@@ -0,0 +1,2 @@
+class User < ActiveRecord::Base
+end