Skip to content

Commit 19c7f7b

Browse files
alexrfordalexrford
authored
Merge pull request #8271 from github/alexrford/ruby/orm-write-access
Ruby: Add `OrmWriteAccess` concept to model writes to a DB using an ORM
2 parents d4808a7 + edf8a3f commit 19c7f7b

8 files changed

Lines changed: 261 additions & 0 deletions

File tree

‎ruby/ql/lib/change-notes/2022-02-28-orm-write-access.md‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
---
2+
category: minorAnalysis
3+
---
4+
* Added `OrmWriteAccess` concept to model data written to a database using an object-relational mapping (ORM) library.

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

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -625,6 +625,35 @@ module OrmInstantiation {
625625
}
626626
}
627627

628+
/**
629+
* A data flow node that writes persistent data.
630+
*
631+
* Extend this class to refine existing API models. If you want to model new APIs,
632+
* extend `PersistentWriteAccess::Range` instead.
633+
*/
634+
class PersistentWriteAccess extends DataFlow::Node instanceof PersistentWriteAccess::Range {
635+
/**
636+
* Gets the data flow node corresponding to the written value.
637+
*/
638+
DataFlow::Node getValue() { result = super.getValue() }
639+
}
640+
641+
/** Provides a class for modeling new persistent write access APIs. */
642+
module PersistentWriteAccess {
643+
/**
644+
* A data flow node that writes persistent data.
645+
*
646+
* Extend this class to model new APIs. If you want to refine existing API models,
647+
* extend `PersistentWriteAccess` instead.
648+
*/
649+
abstract class Range extends DataFlow::Node {
650+
/**
651+
* Gets the data flow node corresponding to the written value.
652+
*/
653+
abstract DataFlow::Node getValue();
654+
}
655+
}
656+
628657
/**
629658
* A data-flow node that may set or unset Cross-site request forgery protection.
630659
*

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

Lines changed: 148 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -316,3 +316,151 @@ private class ActiveRecordInstanceMethodCall extends DataFlow::CallNode {
316316

317317
ActiveRecordInstance getInstance() { result = instance }
318318
}
319+
320+
/**
321+
* Provides modeling relating to the `ActiveRecord::Persistence` module.
322+
*/
323+
private module Persistence {
324+
/**
325+
* Holds if there is a hash literal argument to `call` at `argIndex`
326+
* containing a KV pair with value `value`.
327+
*/
328+
private predicate hashArgumentWithValue(
329+
DataFlow::CallNode call, int argIndex, DataFlow::ExprNode value
330+
) {
331+
exists(ExprNodes::HashLiteralCfgNode hash, ExprNodes::PairCfgNode pair |
332+
hash = call.getArgument(argIndex).asExpr() and
333+
pair = hash.getAKeyValuePair()
334+
|
335+
value.asExpr() = pair.getValue()
336+
)
337+
}
338+
339+
/**
340+
* Holds if `call` has a keyword argument of with value `value`.
341+
*/
342+
private predicate keywordArgumentWithValue(DataFlow::CallNode call, DataFlow::ExprNode value) {
343+
exists(ExprNodes::PairCfgNode pair | pair = call.getArgument(_).asExpr() |
344+
value.asExpr() = pair.getValue()
345+
)
346+
}
347+
348+
/** A call to e.g. `User.create(name: "foo")` */
349+
private class CreateLikeCall extends DataFlow::CallNode, PersistentWriteAccess::Range {
350+
private ActiveRecordModelClass modelCls;
351+
352+
CreateLikeCall() {
353+
modelCls = this.asExpr().getExpr().(ActiveRecordModelClassMethodCall).getReceiverClass() and
354+
this.getMethodName() =
355+
[
356+
"create", "create!", "create_or_find_by", "create_or_find_by!", "find_or_create_by",
357+
"find_or_create_by!", "insert", "insert!"
358+
]
359+
}
360+
361+
override DataFlow::Node getValue() {
362+
// attrs as hash elements in arg0
363+
hashArgumentWithValue(this, 0, result) or
364+
keywordArgumentWithValue(this, result)
365+
}
366+
}
367+
368+
/** A call to e.g. `User.update(1, name: "foo")` */
369+
private class UpdateLikeClassMethodCall extends DataFlow::CallNode, PersistentWriteAccess::Range {
370+
private ActiveRecordModelClass modelCls;
371+
372+
UpdateLikeClassMethodCall() {
373+
modelCls = this.asExpr().getExpr().(ActiveRecordModelClassMethodCall).getReceiverClass() and
374+
this.getMethodName() = ["update", "update!", "upsert"]
375+
}
376+
377+
override DataFlow::Node getValue() {
378+
keywordArgumentWithValue(this, result)
379+
or
380+
// Case where 2 array args are passed - the first an array of IDs, and the
381+
// second an array of hashes - each hash corresponding to an ID in the
382+
// first array.
383+
exists(ExprNodes::ArrayLiteralCfgNode hashesArray |
384+
this.getArgument(0).asExpr() instanceof ExprNodes::ArrayLiteralCfgNode and
385+
hashesArray = this.getArgument(1).asExpr()
386+
|
387+
exists(ExprNodes::HashLiteralCfgNode hash, ExprNodes::PairCfgNode pair |
388+
hash = hashesArray.getArgument(_) and
389+
pair = hash.getAKeyValuePair()
390+
|
391+
result.asExpr() = pair.getValue()
392+
)
393+
)
394+
}
395+
}
396+
397+
/** A call to e.g. `User.insert_all([{name: "foo"}, {name: "bar"}])` */
398+
private class InsertAllLikeCall extends DataFlow::CallNode, PersistentWriteAccess::Range {
399+
private ExprNodes::ArrayLiteralCfgNode arr;
400+
private ActiveRecordModelClass modelCls;
401+
402+
InsertAllLikeCall() {
403+
modelCls = this.asExpr().getExpr().(ActiveRecordModelClassMethodCall).getReceiverClass() and
404+
this.getMethodName() = ["insert_all", "insert_all!", "upsert_all"] and
405+
arr = this.getArgument(0).asExpr()
406+
}
407+
408+
override DataFlow::Node getValue() {
409+
// attrs as hash elements of members of array arg0
410+
exists(ExprNodes::HashLiteralCfgNode hash, ExprNodes::PairCfgNode pair |
411+
hash = arr.getArgument(_) and
412+
pair = hash.getAKeyValuePair()
413+
|
414+
result.asExpr() = pair.getValue()
415+
)
416+
}
417+
}
418+
419+
/** A call to e.g. `user.update(name: "foo")` */
420+
private class UpdateLikeInstanceMethodCall extends PersistentWriteAccess::Range,
421+
ActiveRecordInstanceMethodCall {
422+
UpdateLikeInstanceMethodCall() {
423+
this.getMethodName() = ["update", "update!", "update_attributes", "update_attributes!"]
424+
}
425+
426+
override DataFlow::Node getValue() {
427+
// attrs as hash elements in arg0
428+
hashArgumentWithValue(this, 0, result)
429+
or
430+
// keyword arg
431+
keywordArgumentWithValue(this, result)
432+
}
433+
}
434+
435+
/** A call to e.g. `user.update_attribute(name, "foo")` */
436+
private class UpdateAttributeCall extends PersistentWriteAccess::Range,
437+
ActiveRecordInstanceMethodCall {
438+
UpdateAttributeCall() { this.getMethodName() = "update_attribute" }
439+
440+
override DataFlow::Node getValue() {
441+
// e.g. `foo.update_attribute(key, value)`
442+
result = this.getArgument(1)
443+
}
444+
}
445+
446+
/**
447+
* An assignment like `user.name = "foo"`. Though this does not write to the
448+
* database without a subsequent call to persist the object, it is considered
449+
* as an `PersistentWriteAccess` to avoid missing cases where the path to a
450+
* subsequent write is not clear.
451+
*/
452+
private class AssignAttribute extends PersistentWriteAccess::Range {
453+
private ExprNodes::AssignExprCfgNode assignNode;
454+
455+
AssignAttribute() {
456+
exists(DataFlow::CallNode setter |
457+
assignNode = this.asExpr() and
458+
setter.getArgument(0) = this and
459+
setter instanceof ActiveRecordInstanceMethodCall and
460+
setter.asExpr().getExpr() instanceof SetterMethodCall
461+
)
462+
}
463+
464+
override DataFlow::Node getValue() { assignNode.getRhs() = result.asExpr() }
465+
}
466+
}

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

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,22 @@
1+
| app/controllers/users_controller.rb:5:7:5:44 | call to create! | app/controllers/users_controller.rb:5:26:5:29 | "U1" |
2+
| 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 |
3+
| app/controllers/users_controller.rb:6:7:6:29 | call to create | app/controllers/users_controller.rb:6:25:6:28 | "U2" |
4+
| app/controllers/users_controller.rb:7:7:7:31 | call to insert | app/controllers/users_controller.rb:7:26:7:29 | "U3" |
5+
| app/controllers/users_controller.rb:10:7:10:32 | call to update | app/controllers/users_controller.rb:10:28:10:31 | "U4" |
6+
| app/controllers/users_controller.rb:11:7:11:73 | call to update! | app/controllers/users_controller.rb:11:39:11:42 | "U5" |
7+
| app/controllers/users_controller.rb:11:7:11:73 | call to update! | app/controllers/users_controller.rb:11:53:11:56 | "U6" |
8+
| app/controllers/users_controller.rb:11:7:11:73 | call to update! | app/controllers/users_controller.rb:11:67:11:70 | "U7" |
9+
| app/controllers/users_controller.rb:14:7:14:66 | call to insert_all | app/controllers/users_controller.rb:14:31:14:34 | "U8" |
10+
| app/controllers/users_controller.rb:14:7:14:66 | call to insert_all | app/controllers/users_controller.rb:14:45:14:48 | "U9" |
11+
| app/controllers/users_controller.rb:14:7:14:66 | call to insert_all | app/controllers/users_controller.rb:14:59:14:63 | "U10" |
12+
| app/controllers/users_controller.rb:19:7:19:30 | call to update | app/controllers/users_controller.rb:19:25:19:29 | "U11" |
13+
| app/controllers/users_controller.rb:20:7:20:57 | call to update_attributes | app/controllers/users_controller.rb:20:37:20:41 | "U12" |
14+
| 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 |
15+
| app/controllers/users_controller.rb:23:7:23:42 | call to update_attribute | app/controllers/users_controller.rb:23:37:23:41 | "U13" |
16+
| app/controllers/users_controller.rb:26:7:26:15 | ... = ... | app/controllers/users_controller.rb:26:19:26:23 | "U14" |
17+
| app/models/user.rb:4:5:4:28 | call to update | app/models/user.rb:4:23:4:27 | "U15" |
18+
| app/models/user.rb:5:5:5:23 | call to update | app/models/user.rb:5:18:5:22 | "U16" |
19+
| app/models/user.rb:6:5:6:56 | call to update_attributes | app/models/user.rb:6:35:6:39 | "U17" |
20+
| app/models/user.rb:6:5:6:56 | call to update_attributes | app/models/user.rb:6:51:6:54 | true |
21+
| app/models/user.rb:9:5:9:40 | call to update_attribute | app/models/user.rb:9:35:9:39 | "U18" |
22+
| app/models/user.rb:12:5:12:13 | ... = ... | app/models/user.rb:12:17:12:21 | "U19" |

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

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
import codeql.ruby.DataFlow
2+
import codeql.ruby.Concepts
3+
4+
query predicate persistentWriteAccesses(PersistentWriteAccess acc, DataFlow::Node value) {
5+
value = acc.getValue()
6+
}

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

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,34 @@
1+
module Users
2+
class UsersController < ApplicationController
3+
def create_or_modify
4+
# CreateLikeCall
5+
User.create!(name: "U1", uid: get_uid)
6+
User.create(name: "U2")
7+
User.insert({name: "U3"})
8+
9+
# UpdateLikeClassMethodCall
10+
User.update(4, name: "U4")
11+
User.update!([5, 6, 7], [{name: "U5"}, {name: "U6"}, {name: "U7"}])
12+
13+
# InsertAllLikeCall
14+
User.insert_all([{name: "U8"}, {name: "U9"}, {name: "U10"}])
15+
16+
user = User.find(5)
17+
18+
# UpdateLikeInstanceMethodCall
19+
user.update(name: "U11")
20+
user.update_attributes({name: "U12", uid: get_uid})
21+
22+
# UpdateAttributeCall
23+
user.update_attribute("name", "U13")
24+
25+
# AssignAttributeCall
26+
user.name = "U14"
27+
user.save
28+
end
29+
30+
def get_uid
31+
User.last.id + 1
32+
end
33+
end
34+
end

‎ruby/ql/test/library-tests/concepts/app/models/application_record.rb‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,3 @@
1+
class ApplicationRecord < ActiveRecord::Base
2+
self.abstract_class = true
3+
end

‎ruby/ql/test/library-tests/concepts/app/models/user.rb‎

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,15 @@
1+
class User < ActiveRecord::Base
2+
def t1
3+
# UpdateLikeInstanceMethodCall
4+
self.update(name: "U15")
5+
update(name: "U16")
6+
self.update_attributes({name: "U17", isAdmin: true})
7+
8+
# UpdateAttributeCall
9+
self.update_attribute("name", "U18")
10+
11+
# AssignAttributeCall
12+
self.name = "U19"
13+
user.save
14+
end
15+
end

0 commit comments

Comments
 (0)