Ruby: Recognise more AR write accesses

This change means we recognise calls like

```rb
User.create(params)
User.update(id, params)
```

as instances of `PersistentWriteAccess`.

Author: hmac

ruby/ql/lib/codeql/ruby/controlflow/CfgNodes.qll

@@ -358,7 +358,7 @@ module ExprNodes {
     }
 
     /**
-     * Gets the `nth` positional argument of this call.
+     * Gets the `n`th positional argument of this call.
      * Unlike `getArgument`, this excludes keyword arguments.
      */
     final ExprCfgNode getPositionalArgument(int n) {

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

@@ -72,7 +72,7 @@ class CallNode extends LocalSourceNode, ExprNode {
   ExprNode getKeywordArgument(string name) { result.getExprNode() = node.getKeywordArgument(name) }
 
   /**
-   * Gets the `nth` positional argument of this call.
+   * Gets the `n`th positional argument of this call.
    * Unlike `getArgument`, this excludes keyword arguments.
    */
   final ExprNode getPositionalArgument(int n) {

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

@@ -364,15 +364,6 @@ private module Persistence {
     )
   }
 
-  /**
-   * Holds if `call` has a keyword argument with value `value`.
-   */
-  private predicate keywordArgumentWithValue(DataFlow::CallNode call, DataFlow::ExprNode value) {
-    exists(ExprNodes::PairCfgNode pair | pair = call.getArgument(_).asExpr() |
-      value.asExpr() = pair.getValue()
-    )
-  }
-
   /** A call to e.g. `User.create(name: "foo")` */
   private class CreateLikeCall extends DataFlow::CallNode, PersistentWriteAccess::Range {
     CreateLikeCall() {
@@ -386,8 +377,12 @@ private module Persistence {
 
     override DataFlow::Node getValue() {
       // attrs as hash elements in arg0
-      hashArgumentWithValue(this, 0, result) or
-      keywordArgumentWithValue(this, result)
+      hashArgumentWithValue(this, 0, result)
+      or
+      result = this.getKeywordArgument(_)
+      or
+      result = this.getPositionalArgument(0) and
+      not result.asExpr() instanceof ExprNodes::HashLiteralCfgNode
     }
   }
 
@@ -399,11 +394,19 @@ private module Persistence {
     }
 
     override DataFlow::Node getValue() {
-      keywordArgumentWithValue(this, result)
+      // User.update(1, name: "foo")
+      result = this.getKeywordArgument(_)
+      or
+      // User.update(1, params)
+      exists(int n | n > 0 |
+        result = this.getPositionalArgument(n) and
+        not result.asExpr() instanceof ExprNodes::ArrayLiteralCfgNode
+      )
       or
       // Case where 2 array args are passed - the first an array of IDs, and the
       // second an array of hashes - each hash corresponding to an ID in the
       // first array.
+      // User.update([1,2,3], [{name: "foo"}, {name: "bar"}])
       exists(ExprNodes::ArrayLiteralCfgNode hashesArray |
         this.getArgument(0).asExpr() instanceof ExprNodes::ArrayLiteralCfgNode and
         hashesArray = this.getArgument(1).asExpr()
@@ -472,8 +475,12 @@ private module Persistence {
       // attrs as hash elements in arg0
       hashArgumentWithValue(this, 0, result)
       or
+      // attrs as variable in arg0
+      result = this.getPositionalArgument(0) and
+      not result.asExpr() instanceof ExprNodes::HashLiteralCfgNode
+      or
       // keyword arg
-      keywordArgumentWithValue(this, result)
+      result = this.getKeywordArgument(_)
     }
   }
 

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

@@ -15,6 +15,11 @@
 | 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:19:26:23 | ... = ... | app/controllers/users_controller.rb:26:19:26:23 | "U14" |
 | app/controllers/users_controller.rb:31:7:31:32 | call to touch_all | app/controllers/users_controller.rb:31:28:31:31 | call to time |
+| app/controllers/users_controller.rb:35:7:35:27 | call to update | app/controllers/users_controller.rb:35:22:35:26 | attrs |
+| app/controllers/users_controller.rb:36:7:36:28 | call to update! | app/controllers/users_controller.rb:36:23:36:27 | attrs |
+| app/controllers/users_controller.rb:39:7:39:24 | call to create | app/controllers/users_controller.rb:39:19:39:23 | attrs |
+| app/controllers/users_controller.rb:40:7:40:25 | call to create! | app/controllers/users_controller.rb:40:20:40:24 | attrs |
+| app/controllers/users_controller.rb:41:7:41:24 | call to insert | app/controllers/users_controller.rb:41:19:41:23 | attrs |
 | app/models/user.rb:4:5:4:28 | call to update | app/models/user.rb:4:23:4:27 | "U15" |
 | app/models/user.rb:5:5:5:23 | call to update | app/models/user.rb:5:18:5:22 | "U16" |
 | app/models/user.rb:6:5:6:56 | call to update_attributes | app/models/user.rb:6:35:6:39 | "U17" |

ruby/ql/test/library-tests/concepts/app/controllers/users_controller.rb

@@ -29,6 +29,16 @@ def create_or_modify
       # TouchAllCall
       User.touch_all
       User.touch_all(time: time)
+
+      # UpdateLikeClassMethodCall
+      attrs = {name: "U15"}
+      User.update(8, attrs)
+      User.update!(8, attrs)
+
+      # CreateLikeClassMethodCall
+      User.create(attrs)
+      User.create!(attrs)
+      User.insert(attrs)
     end
 
     def get_uid

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

@@ -1,8 +1,8 @@
 actionControllerControllerClasses
 | active_record/ActiveRecord.rb:23:1:39:3 | FooController |
 | active_record/ActiveRecord.rb:41:1:64:3 | BarController |
-| active_record/ActiveRecord.rb:66:1:70:3 | BazController |
-| active_record/ActiveRecord.rb:72:1:80:3 | AnnotatedController |
+| active_record/ActiveRecord.rb:66:1:94:3 | BazController |
+| active_record/ActiveRecord.rb:96:1:104:3 | AnnotatedController |
 | app/controllers/comments_controller.rb:1:1:7:3 | CommentsController |
 | app/controllers/foo/bars_controller.rb:3:1:39:3 | BarsController |
 | app/controllers/photos_controller.rb:1:1:4:3 | PhotosController |
@@ -13,8 +13,14 @@ actionControllerActionMethods
 | active_record/ActiveRecord.rb:42:3:47:5 | some_other_request_handler |
 | active_record/ActiveRecord.rb:49:3:63:5 | safe_paths |
 | active_record/ActiveRecord.rb:67:3:69:5 | yet_another_handler |
-| active_record/ActiveRecord.rb:73:3:75:5 | index |
-| active_record/ActiveRecord.rb:77:3:79:5 | unsafe_action |
+| active_record/ActiveRecord.rb:71:3:73:5 | create1 |
+| active_record/ActiveRecord.rb:75:3:77:5 | create2 |
+| active_record/ActiveRecord.rb:79:3:81:5 | create3 |
+| active_record/ActiveRecord.rb:83:3:85:5 | update1 |
+| active_record/ActiveRecord.rb:87:3:89:5 | update2 |
+| active_record/ActiveRecord.rb:91:3:93:5 | update3 |
+| active_record/ActiveRecord.rb:97:3:99:5 | index |
+| active_record/ActiveRecord.rb:101:3:103:5 | unsafe_action |
 | app/controllers/comments_controller.rb:2:3:3:5 | index |
 | app/controllers/comments_controller.rb:5:3:6:5 | show |
 | app/controllers/foo/bars_controller.rb:5:3:7:5 | index |
@@ -41,7 +47,17 @@ paramsCalls
 | active_record/ActiveRecord.rb:59:12:59:17 | call to params |
 | active_record/ActiveRecord.rb:62:15:62:20 | call to params |
 | active_record/ActiveRecord.rb:68:21:68:26 | call to params |
-| active_record/ActiveRecord.rb:78:59:78:64 | call to params |
+| active_record/ActiveRecord.rb:72:18:72:23 | call to params |
+| active_record/ActiveRecord.rb:76:24:76:29 | call to params |
+| active_record/ActiveRecord.rb:76:49:76:54 | call to params |
+| active_record/ActiveRecord.rb:80:25:80:30 | call to params |
+| active_record/ActiveRecord.rb:80:50:80:55 | call to params |
+| active_record/ActiveRecord.rb:84:21:84:26 | call to params |
+| active_record/ActiveRecord.rb:88:27:88:32 | call to params |
+| active_record/ActiveRecord.rb:88:52:88:57 | call to params |
+| active_record/ActiveRecord.rb:92:28:92:33 | call to params |
+| active_record/ActiveRecord.rb:92:53:92:58 | call to params |
+| active_record/ActiveRecord.rb:102:59:102:64 | call to params |
 | app/controllers/foo/bars_controller.rb:13:21:13:26 | call to params |
 | app/controllers/foo/bars_controller.rb:14:10:14:15 | call to params |
 | app/controllers/foo/bars_controller.rb:21:21:21:26 | call to params |
@@ -61,7 +77,17 @@ paramsSources
 | active_record/ActiveRecord.rb:59:12:59:17 | call to params |
 | active_record/ActiveRecord.rb:62:15:62:20 | call to params |
 | active_record/ActiveRecord.rb:68:21:68:26 | call to params |
-| active_record/ActiveRecord.rb:78:59:78:64 | call to params |
+| active_record/ActiveRecord.rb:72:18:72:23 | call to params |
+| active_record/ActiveRecord.rb:76:24:76:29 | call to params |
+| active_record/ActiveRecord.rb:76:49:76:54 | call to params |
+| active_record/ActiveRecord.rb:80:25:80:30 | call to params |
+| active_record/ActiveRecord.rb:80:50:80:55 | call to params |
+| active_record/ActiveRecord.rb:84:21:84:26 | call to params |
+| active_record/ActiveRecord.rb:88:27:88:32 | call to params |
+| active_record/ActiveRecord.rb:88:52:88:57 | call to params |
+| active_record/ActiveRecord.rb:92:28:92:33 | call to params |
+| active_record/ActiveRecord.rb:92:53:92:58 | call to params |
+| active_record/ActiveRecord.rb:102:59:102:64 | call to params |
 | app/controllers/foo/bars_controller.rb:13:21:13:26 | call to params |
 | app/controllers/foo/bars_controller.rb:14:10:14:15 | call to params |
 | app/controllers/foo/bars_controller.rb:21:21:21:26 | call to params |

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

@@ -22,7 +22,7 @@ activeRecordSqlExecutionRanges
 | ActiveRecord.rb:46:20:46:32 | ... + ... |
 | ActiveRecord.rb:52:16:52:28 | "name #{...}" |
 | ActiveRecord.rb:56:20:56:39 | "username = #{...}" |
-| ActiveRecord.rb:78:27:78:76 | "this is an unsafe annotation:..." |
+| ActiveRecord.rb:102:27:102:76 | "this is an unsafe annotation:..." |
 activeRecordModelClassMethodCalls
 | ActiveRecord.rb:2:3:2:17 | call to has_many |
 | ActiveRecord.rb:6:3:6:24 | call to belongs_to |
@@ -45,8 +45,14 @@ activeRecordModelClassMethodCalls
 | ActiveRecord.rb:60:5:60:33 | call to find_by |
 | ActiveRecord.rb:62:5:62:34 | call to find |
 | ActiveRecord.rb:68:5:68:45 | call to delete_by |
-| ActiveRecord.rb:74:13:74:54 | call to annotate |
-| ActiveRecord.rb:78:13:78:77 | call to annotate |
+| ActiveRecord.rb:72:5:72:24 | call to create |
+| ActiveRecord.rb:76:5:76:66 | call to create |
+| ActiveRecord.rb:80:5:80:68 | call to create |
+| ActiveRecord.rb:84:5:84:27 | call to update |
+| ActiveRecord.rb:88:5:88:69 | call to update |
+| ActiveRecord.rb:92:5:92:71 | call to update |
+| ActiveRecord.rb:98:13:98:54 | call to annotate |
+| ActiveRecord.rb:102:13:102:77 | call to annotate |
 potentiallyUnsafeSqlExecutingMethodCall
 | ActiveRecord.rb:9:5:9:68 | call to find |
 | ActiveRecord.rb:19:5:19:25 | call to destroy_by |
@@ -58,11 +64,21 @@ potentiallyUnsafeSqlExecutingMethodCall
 | ActiveRecord.rb:46:5:46:33 | call to delete_by |
 | ActiveRecord.rb:52:5:52:29 | call to order |
 | ActiveRecord.rb:56:7:56:40 | call to find_by |
-| ActiveRecord.rb:78:13:78:77 | call to annotate |
+| ActiveRecord.rb:102:13:102:77 | call to annotate |
 activeRecordModelInstantiations
 | ActiveRecord.rb:9:5:9:68 | call to find | ActiveRecord.rb:5:1:15:3 | User |
 | ActiveRecord.rb:13:5:13:40 | call to find_by | ActiveRecord.rb:1:1:3:3 | UserGroup |
 | ActiveRecord.rb:36:5:36:30 | call to find_by_name | ActiveRecord.rb:5:1:15:3 | User |
 | ActiveRecord.rb:56:7:56:40 | call to find_by | ActiveRecord.rb:5:1:15:3 | User |
 | ActiveRecord.rb:60:5:60:33 | call to find_by | ActiveRecord.rb:5:1:15:3 | User |
 | ActiveRecord.rb:62:5:62:34 | call to find | ActiveRecord.rb:5:1:15:3 | User |
+persistentWriteAccesses
+| ActiveRecord.rb:72:5:72:24 | call to create | ActiveRecord.rb:72:18:72:23 | call to params |
+| ActiveRecord.rb:76:5:76:66 | call to create | ActiveRecord.rb:76:24:76:36 | ...[...] |
+| ActiveRecord.rb:76:5:76:66 | call to create | ActiveRecord.rb:76:49:76:65 | ...[...] |
+| ActiveRecord.rb:80:5:80:68 | call to create | ActiveRecord.rb:80:25:80:37 | ...[...] |
+| ActiveRecord.rb:80:5:80:68 | call to create | ActiveRecord.rb:80:50:80:66 | ...[...] |
+| ActiveRecord.rb:84:5:84:27 | call to update | ActiveRecord.rb:84:21:84:26 | call to params |
+| ActiveRecord.rb:88:5:88:69 | call to update | ActiveRecord.rb:88:27:88:39 | ...[...] |
+| ActiveRecord.rb:88:5:88:69 | call to update | ActiveRecord.rb:88:52:88:68 | ...[...] |
+| ActiveRecord.rb:92:5:92:71 | call to update | ActiveRecord.rb:92:21:92:70 | call to [] |

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

@@ -73,15 +73,23 @@ def create1
   end
 
   def create2
-    Admin.create(name: params[:name])
+    Admin.create(name: params[:name], password: params[:password])
+  end
+
+  def create3
+    Admin.create({name: params[:name], password: params[:password]})
   end
 
   def update1
-    Admin.update(params)
+    Admin.update(1, params)
   end
 
   def update2
-    Admin.update(name: params[:name])
+    Admin.update(1, name: params[:name], password: params[:password])
+  end
+
+  def update3
+    Admin.update(1, {name: params[:name], password: params[:password]})
   end
 end