Skip to content

Commit 157bbcc

Browse files
hmachmac
authored
Merge pull request #9851 from hmac/hmac/active-record-improvements
Ruby: Recognise more AR write accesses
2 parents 6fa1e06 + ee9e6b1 commit 157bbcc

File tree

10 files changed

+170
-55
lines changed

10 files changed

+170
-55
lines changed

‎ruby/ql/lib/change-notes/2022-08-04-active-record-writes.md

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
category: minorAnalysis
3+
---
4+
* Calls to `ActiveRecord::Base.create` and `ActiveRecord::Base.update` are now
5+
recognised as write accesses.

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

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -357,6 +357,14 @@ module ExprNodes {
357357
)
358358
}
359359

360+
/**
361+
* Gets the `n`th positional argument of this call.
362+
* Unlike `getArgument`, this excludes keyword arguments.
363+
*/
364+
final ExprCfgNode getPositionalArgument(int n) {
365+
result = this.getArgument(n) and not result instanceof PairCfgNode
366+
}
367+
360368
/** Gets the number of arguments of this call. */
361369
final int getNumberOfArguments() { result = e.getNumberOfArguments() }
362370

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

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -71,6 +71,14 @@ class CallNode extends LocalSourceNode, ExprNode {
7171
/** Gets the data-flow node corresponding to the named argument of the call corresponding to this data-flow node */
7272
ExprNode getKeywordArgument(string name) { result.getExprNode() = node.getKeywordArgument(name) }
7373

74+
/**
75+
* Gets the `n`th positional argument of this call.
76+
* Unlike `getArgument`, this excludes keyword arguments.
77+
*/
78+
final ExprNode getPositionalArgument(int n) {
79+
result.getExprNode() = node.getPositionalArgument(n)
80+
}
81+
7482
/** Gets the name of the the method called by the method call (if any) corresponding to this data-flow node */
7583
string getMethodName() { result = node.getExpr().(MethodCall).getMethodName() }
7684

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

Lines changed: 20 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -364,15 +364,6 @@ private module Persistence {
364364
)
365365
}
366366

367-
/**
368-
* Holds if `call` has a keyword argument with value `value`.
369-
*/
370-
private predicate keywordArgumentWithValue(DataFlow::CallNode call, DataFlow::ExprNode value) {
371-
exists(ExprNodes::PairCfgNode pair | pair = call.getArgument(_).asExpr() |
372-
value.asExpr() = pair.getValue()
373-
)
374-
}
375-
376367
/** A call to e.g. `User.create(name: "foo")` */
377368
private class CreateLikeCall extends DataFlow::CallNode, PersistentWriteAccess::Range {
378369
CreateLikeCall() {
@@ -386,8 +377,12 @@ private module Persistence {
386377

387378
override DataFlow::Node getValue() {
388379
// attrs as hash elements in arg0
389-
hashArgumentWithValue(this, 0, result) or
390-
keywordArgumentWithValue(this, result)
380+
hashArgumentWithValue(this, 0, result)
381+
or
382+
result = this.getKeywordArgument(_)
383+
or
384+
result = this.getPositionalArgument(0) and
385+
not result.asExpr() instanceof ExprNodes::HashLiteralCfgNode
391386
}
392387
}
393388

@@ -399,11 +394,19 @@ private module Persistence {
399394
}
400395

401396
override DataFlow::Node getValue() {
402-
keywordArgumentWithValue(this, result)
397+
// User.update(1, name: "foo")
398+
result = this.getKeywordArgument(_)
399+
or
400+
// User.update(1, params)
401+
exists(int n | n > 0 |
402+
result = this.getPositionalArgument(n) and
403+
not result.asExpr() instanceof ExprNodes::ArrayLiteralCfgNode
404+
)
403405
or
404406
// Case where 2 array args are passed - the first an array of IDs, and the
405407
// second an array of hashes - each hash corresponding to an ID in the
406408
// first array.
409+
// User.update([1,2,3], [{name: "foo"}, {name: "bar"}])
407410
exists(ExprNodes::ArrayLiteralCfgNode hashesArray |
408411
this.getArgument(0).asExpr() instanceof ExprNodes::ArrayLiteralCfgNode and
409412
hashesArray = this.getArgument(1).asExpr()
@@ -472,8 +475,12 @@ private module Persistence {
472475
// attrs as hash elements in arg0
473476
hashArgumentWithValue(this, 0, result)
474477
or
478+
// attrs as variable in arg0
479+
result = this.getPositionalArgument(0) and
480+
not result.asExpr() instanceof ExprNodes::HashLiteralCfgNode
481+
or
475482
// keyword arg
476-
keywordArgumentWithValue(this, result)
483+
result = this.getKeywordArgument(_)
477484
}
478485
}
479486

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

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,11 @@
1515
| app/controllers/users_controller.rb:23:7:23:42 | call to update_attribute | app/controllers/users_controller.rb:23:37:23:41 | "U13" |
1616
| app/controllers/users_controller.rb:26:19:26:23 | ... = ... | app/controllers/users_controller.rb:26:19:26:23 | "U14" |
1717
| 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 |
18+
| app/controllers/users_controller.rb:35:7:35:27 | call to update | app/controllers/users_controller.rb:35:22:35:26 | attrs |
19+
| app/controllers/users_controller.rb:36:7:36:28 | call to update! | app/controllers/users_controller.rb:36:23:36:27 | attrs |
20+
| app/controllers/users_controller.rb:39:7:39:24 | call to create | app/controllers/users_controller.rb:39:19:39:23 | attrs |
21+
| app/controllers/users_controller.rb:40:7:40:25 | call to create! | app/controllers/users_controller.rb:40:20:40:24 | attrs |
22+
| app/controllers/users_controller.rb:41:7:41:24 | call to insert | app/controllers/users_controller.rb:41:19:41:23 | attrs |
1823
| app/models/user.rb:4:5:4:28 | call to update | app/models/user.rb:4:23:4:27 | "U15" |
1924
| app/models/user.rb:5:5:5:23 | call to update | app/models/user.rb:5:18:5:22 | "U16" |
2025
| 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

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,16 @@ def create_or_modify
2929
# TouchAllCall
3030
User.touch_all
3131
User.touch_all(time: time)
32+
33+
# UpdateLikeClassMethodCall
34+
attrs = {name: "U15"}
35+
User.update(8, attrs)
36+
User.update!(8, attrs)
37+
38+
# CreateLikeClassMethodCall
39+
User.create(attrs)
40+
User.create!(attrs)
41+
User.insert(attrs)
3242
end
3343

3444
def get_uid

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

Lines changed: 64 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -1,20 +1,26 @@
11
actionControllerControllerClasses
2-
| ActiveRecord.rb:23:1:39:3 | FooController |
3-
| ActiveRecord.rb:41:1:64:3 | BarController |
4-
| ActiveRecord.rb:66:1:70:3 | BazController |
5-
| ActiveRecord.rb:72:1:80:3 | AnnotatedController |
2+
| active_record/ActiveRecord.rb:23:1:39:3 | FooController |
3+
| active_record/ActiveRecord.rb:41:1:64:3 | BarController |
4+
| active_record/ActiveRecord.rb:66:1:94:3 | BazController |
5+
| active_record/ActiveRecord.rb:96:1:104:3 | AnnotatedController |
66
| app/controllers/comments_controller.rb:1:1:7:3 | CommentsController |
77
| app/controllers/foo/bars_controller.rb:3:1:39:3 | BarsController |
88
| app/controllers/photos_controller.rb:1:1:4:3 | PhotosController |
99
| app/controllers/posts_controller.rb:1:1:10:3 | PostsController |
1010
| app/controllers/users/notifications_controller.rb:2:3:5:5 | NotificationsController |
1111
actionControllerActionMethods
12-
| ActiveRecord.rb:27:3:38:5 | some_request_handler |
13-
| ActiveRecord.rb:42:3:47:5 | some_other_request_handler |
14-
| ActiveRecord.rb:49:3:63:5 | safe_paths |
15-
| ActiveRecord.rb:67:3:69:5 | yet_another_handler |
16-
| ActiveRecord.rb:73:3:75:5 | index |
17-
| ActiveRecord.rb:77:3:79:5 | unsafe_action |
12+
| active_record/ActiveRecord.rb:27:3:38:5 | some_request_handler |
13+
| active_record/ActiveRecord.rb:42:3:47:5 | some_other_request_handler |
14+
| active_record/ActiveRecord.rb:49:3:63:5 | safe_paths |
15+
| active_record/ActiveRecord.rb:67:3:69:5 | yet_another_handler |
16+
| active_record/ActiveRecord.rb:71:3:73:5 | create1 |
17+
| active_record/ActiveRecord.rb:75:3:77:5 | create2 |
18+
| active_record/ActiveRecord.rb:79:3:81:5 | create3 |
19+
| active_record/ActiveRecord.rb:83:3:85:5 | update1 |
20+
| active_record/ActiveRecord.rb:87:3:89:5 | update2 |
21+
| active_record/ActiveRecord.rb:91:3:93:5 | update3 |
22+
| active_record/ActiveRecord.rb:97:3:99:5 | index |
23+
| active_record/ActiveRecord.rb:101:3:103:5 | unsafe_action |
1824
| app/controllers/comments_controller.rb:2:3:3:5 | index |
1925
| app/controllers/comments_controller.rb:5:3:6:5 | show |
2026
| app/controllers/foo/bars_controller.rb:5:3:7:5 | index |
@@ -28,40 +34,60 @@ actionControllerActionMethods
2834
| app/controllers/posts_controller.rb:8:3:9:5 | upvote |
2935
| app/controllers/users/notifications_controller.rb:3:5:4:7 | mark_as_read |
3036
paramsCalls
31-
| ActiveRecord.rb:28:30:28:35 | call to params |
32-
| ActiveRecord.rb:29:29:29:34 | call to params |
33-
| ActiveRecord.rb:30:31:30:36 | call to params |
34-
| ActiveRecord.rb:32:21:32:26 | call to params |
35-
| ActiveRecord.rb:34:34:34:39 | call to params |
36-
| ActiveRecord.rb:35:23:35:28 | call to params |
37-
| ActiveRecord.rb:35:38:35:43 | call to params |
38-
| ActiveRecord.rb:43:10:43:15 | call to params |
39-
| ActiveRecord.rb:50:11:50:16 | call to params |
40-
| ActiveRecord.rb:54:12:54:17 | call to params |
41-
| ActiveRecord.rb:59:12:59:17 | call to params |
42-
| ActiveRecord.rb:62:15:62:20 | call to params |
43-
| ActiveRecord.rb:68:21:68:26 | call to params |
44-
| ActiveRecord.rb:78:59:78:64 | call to params |
37+
| active_record/ActiveRecord.rb:28:30:28:35 | call to params |
38+
| active_record/ActiveRecord.rb:29:29:29:34 | call to params |
39+
| active_record/ActiveRecord.rb:30:31:30:36 | call to params |
40+
| active_record/ActiveRecord.rb:32:21:32:26 | call to params |
41+
| active_record/ActiveRecord.rb:34:34:34:39 | call to params |
42+
| active_record/ActiveRecord.rb:35:23:35:28 | call to params |
43+
| active_record/ActiveRecord.rb:35:38:35:43 | call to params |
44+
| active_record/ActiveRecord.rb:43:10:43:15 | call to params |
45+
| active_record/ActiveRecord.rb:50:11:50:16 | call to params |
46+
| active_record/ActiveRecord.rb:54:12:54:17 | call to params |
47+
| active_record/ActiveRecord.rb:59:12:59:17 | call to params |
48+
| active_record/ActiveRecord.rb:62:15:62:20 | call to params |
49+
| active_record/ActiveRecord.rb:68:21:68:26 | call to params |
50+
| active_record/ActiveRecord.rb:72:18:72:23 | call to params |
51+
| active_record/ActiveRecord.rb:76:24:76:29 | call to params |
52+
| active_record/ActiveRecord.rb:76:49:76:54 | call to params |
53+
| active_record/ActiveRecord.rb:80:25:80:30 | call to params |
54+
| active_record/ActiveRecord.rb:80:50:80:55 | call to params |
55+
| active_record/ActiveRecord.rb:84:21:84:26 | call to params |
56+
| active_record/ActiveRecord.rb:88:27:88:32 | call to params |
57+
| active_record/ActiveRecord.rb:88:52:88:57 | call to params |
58+
| active_record/ActiveRecord.rb:92:28:92:33 | call to params |
59+
| active_record/ActiveRecord.rb:92:53:92:58 | call to params |
60+
| active_record/ActiveRecord.rb:102:59:102:64 | call to params |
4561
| app/controllers/foo/bars_controller.rb:13:21:13:26 | call to params |
4662
| app/controllers/foo/bars_controller.rb:14:10:14:15 | call to params |
4763
| app/controllers/foo/bars_controller.rb:21:21:21:26 | call to params |
4864
| app/controllers/foo/bars_controller.rb:22:10:22:15 | call to params |
4965
| app/views/foo/bars/show.html.erb:5:9:5:14 | call to params |
5066
paramsSources
51-
| ActiveRecord.rb:28:30:28:35 | call to params |
52-
| ActiveRecord.rb:29:29:29:34 | call to params |
53-
| ActiveRecord.rb:30:31:30:36 | call to params |
54-
| ActiveRecord.rb:32:21:32:26 | call to params |
55-
| ActiveRecord.rb:34:34:34:39 | call to params |
56-
| ActiveRecord.rb:35:23:35:28 | call to params |
57-
| ActiveRecord.rb:35:38:35:43 | call to params |
58-
| ActiveRecord.rb:43:10:43:15 | call to params |
59-
| ActiveRecord.rb:50:11:50:16 | call to params |
60-
| ActiveRecord.rb:54:12:54:17 | call to params |
61-
| ActiveRecord.rb:59:12:59:17 | call to params |
62-
| ActiveRecord.rb:62:15:62:20 | call to params |
63-
| ActiveRecord.rb:68:21:68:26 | call to params |
64-
| ActiveRecord.rb:78:59:78:64 | call to params |
67+
| active_record/ActiveRecord.rb:28:30:28:35 | call to params |
68+
| active_record/ActiveRecord.rb:29:29:29:34 | call to params |
69+
| active_record/ActiveRecord.rb:30:31:30:36 | call to params |
70+
| active_record/ActiveRecord.rb:32:21:32:26 | call to params |
71+
| active_record/ActiveRecord.rb:34:34:34:39 | call to params |
72+
| active_record/ActiveRecord.rb:35:23:35:28 | call to params |
73+
| active_record/ActiveRecord.rb:35:38:35:43 | call to params |
74+
| active_record/ActiveRecord.rb:43:10:43:15 | call to params |
75+
| active_record/ActiveRecord.rb:50:11:50:16 | call to params |
76+
| active_record/ActiveRecord.rb:54:12:54:17 | call to params |
77+
| active_record/ActiveRecord.rb:59:12:59:17 | call to params |
78+
| active_record/ActiveRecord.rb:62:15:62:20 | call to params |
79+
| active_record/ActiveRecord.rb:68:21:68:26 | call to params |
80+
| active_record/ActiveRecord.rb:72:18:72:23 | call to params |
81+
| active_record/ActiveRecord.rb:76:24:76:29 | call to params |
82+
| active_record/ActiveRecord.rb:76:49:76:54 | call to params |
83+
| active_record/ActiveRecord.rb:80:25:80:30 | call to params |
84+
| active_record/ActiveRecord.rb:80:50:80:55 | call to params |
85+
| active_record/ActiveRecord.rb:84:21:84:26 | call to params |
86+
| active_record/ActiveRecord.rb:88:27:88:32 | call to params |
87+
| active_record/ActiveRecord.rb:88:52:88:57 | call to params |
88+
| active_record/ActiveRecord.rb:92:28:92:33 | call to params |
89+
| active_record/ActiveRecord.rb:92:53:92:58 | call to params |
90+
| active_record/ActiveRecord.rb:102:59:102:64 | call to params |
6591
| app/controllers/foo/bars_controller.rb:13:21:13:26 | call to params |
6692
| app/controllers/foo/bars_controller.rb:14:10:14:15 | call to params |
6793
| app/controllers/foo/bars_controller.rb:21:21:21:26 | call to params |

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

Lines changed: 20 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,7 @@ activeRecordSqlExecutionRanges
2222
| ActiveRecord.rb:46:20:46:32 | ... + ... |
2323
| ActiveRecord.rb:52:16:52:28 | "name #{...}" |
2424
| ActiveRecord.rb:56:20:56:39 | "username = #{...}" |
25-
| ActiveRecord.rb:78:27:78:76 | "this is an unsafe annotation:..." |
25+
| ActiveRecord.rb:102:27:102:76 | "this is an unsafe annotation:..." |
2626
activeRecordModelClassMethodCalls
2727
| ActiveRecord.rb:2:3:2:17 | call to has_many |
2828
| ActiveRecord.rb:6:3:6:24 | call to belongs_to |
@@ -45,8 +45,14 @@ activeRecordModelClassMethodCalls
4545
| ActiveRecord.rb:60:5:60:33 | call to find_by |
4646
| ActiveRecord.rb:62:5:62:34 | call to find |
4747
| ActiveRecord.rb:68:5:68:45 | call to delete_by |
48-
| ActiveRecord.rb:74:13:74:54 | call to annotate |
49-
| ActiveRecord.rb:78:13:78:77 | call to annotate |
48+
| ActiveRecord.rb:72:5:72:24 | call to create |
49+
| ActiveRecord.rb:76:5:76:66 | call to create |
50+
| ActiveRecord.rb:80:5:80:68 | call to create |
51+
| ActiveRecord.rb:84:5:84:27 | call to update |
52+
| ActiveRecord.rb:88:5:88:69 | call to update |
53+
| ActiveRecord.rb:92:5:92:71 | call to update |
54+
| ActiveRecord.rb:98:13:98:54 | call to annotate |
55+
| ActiveRecord.rb:102:13:102:77 | call to annotate |
5056
potentiallyUnsafeSqlExecutingMethodCall
5157
| ActiveRecord.rb:9:5:9:68 | call to find |
5258
| ActiveRecord.rb:19:5:19:25 | call to destroy_by |
@@ -58,11 +64,21 @@ potentiallyUnsafeSqlExecutingMethodCall
5864
| ActiveRecord.rb:46:5:46:33 | call to delete_by |
5965
| ActiveRecord.rb:52:5:52:29 | call to order |
6066
| ActiveRecord.rb:56:7:56:40 | call to find_by |
61-
| ActiveRecord.rb:78:13:78:77 | call to annotate |
67+
| ActiveRecord.rb:102:13:102:77 | call to annotate |
6268
activeRecordModelInstantiations
6369
| ActiveRecord.rb:9:5:9:68 | call to find | ActiveRecord.rb:5:1:15:3 | User |
6470
| ActiveRecord.rb:13:5:13:40 | call to find_by | ActiveRecord.rb:1:1:3:3 | UserGroup |
6571
| ActiveRecord.rb:36:5:36:30 | call to find_by_name | ActiveRecord.rb:5:1:15:3 | User |
6672
| ActiveRecord.rb:56:7:56:40 | call to find_by | ActiveRecord.rb:5:1:15:3 | User |
6773
| ActiveRecord.rb:60:5:60:33 | call to find_by | ActiveRecord.rb:5:1:15:3 | User |
6874
| ActiveRecord.rb:62:5:62:34 | call to find | ActiveRecord.rb:5:1:15:3 | User |
75+
persistentWriteAccesses
76+
| ActiveRecord.rb:72:5:72:24 | call to create | ActiveRecord.rb:72:18:72:23 | call to params |
77+
| ActiveRecord.rb:76:5:76:66 | call to create | ActiveRecord.rb:76:24:76:36 | ...[...] |
78+
| ActiveRecord.rb:76:5:76:66 | call to create | ActiveRecord.rb:76:49:76:65 | ...[...] |
79+
| ActiveRecord.rb:80:5:80:68 | call to create | ActiveRecord.rb:80:25:80:37 | ...[...] |
80+
| ActiveRecord.rb:80:5:80:68 | call to create | ActiveRecord.rb:80:50:80:66 | ...[...] |
81+
| ActiveRecord.rb:84:5:84:27 | call to update | ActiveRecord.rb:84:21:84:26 | call to params |
82+
| ActiveRecord.rb:88:5:88:69 | call to update | ActiveRecord.rb:88:27:88:39 | ...[...] |
83+
| ActiveRecord.rb:88:5:88:69 | call to update | ActiveRecord.rb:88:52:88:68 | ...[...] |
84+
| ActiveRecord.rb:92:5:92:71 | call to update | ActiveRecord.rb:92:21:92:70 | call to [] |

‎ruby/ql/test/library-tests/frameworks/ActiveRecord.ql renamed to ‎ruby/ql/test/library-tests/frameworks/active_record/ActiveRecord.ql

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,7 @@
11
import codeql.ruby.controlflow.CfgNodes
22
import codeql.ruby.frameworks.ActiveRecord
3+
import codeql.ruby.Concepts
4+
import codeql.ruby.DataFlow
35

46
query predicate activeRecordModelClasses(ActiveRecordModelClass cls) { any() }
57

@@ -18,3 +20,7 @@ query predicate activeRecordModelInstantiations(
1820
) {
1921
i.getClass() = cls
2022
}
23+
24+
query predicate persistentWriteAccesses(PersistentWriteAccess w, DataFlow::Node value) {
25+
w.getValue() = value
26+
}

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

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -67,6 +67,30 @@ class BazController < BarController
6767
def yet_another_handler
6868
Admin.delete_by(params[:admin_condition])
6969
end
70+
71+
def create1
72+
Admin.create(params)
73+
end
74+
75+
def create2
76+
Admin.create(name: params[:name], password: params[:password])
77+
end
78+
79+
def create3
80+
Admin.create({name: params[:name], password: params[:password]})
81+
end
82+
83+
def update1
84+
Admin.update(1, params)
85+
end
86+
87+
def update2
88+
Admin.update(1, name: params[:name], password: params[:password])
89+
end
90+
91+
def update3
92+
Admin.update(1, {name: params[:name], password: params[:password]})
93+
end
7094
end
7195

7296
class AnnotatedController < ActionController::Base

0 commit comments

Comments
 (0)