Skip to content

Commit e7d19e8

Browse files
hmachmac
authored
Merge pull request #10090 from hmac/hmac/activestorage
Ruby: Model Activestorage
2 parents 0ce0ada + 49572a5 commit e7d19e8

File tree

7 files changed

+380
-19
lines changed

7 files changed

+380
-19
lines changed

‎ruby/ql/lib/change-notes/2022-08-30-activestorage.md‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
---
2+
category: minorAnalysis
3+
---
4+
* Various code executions, command executions and HTTP requests in the
5+
ActiveStorage library are now recognized.
6+

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

Lines changed: 19 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -528,13 +528,17 @@ private module Persistence {
528528
* end
529529
* ```
530530
*/
531-
private class ActiveRecordAssociation extends DataFlow::CallNode {
531+
class ActiveRecordAssociation extends DataFlow::CallNode {
532532
private ActiveRecordModelClass modelClass;
533533

534534
ActiveRecordAssociation() {
535535
not exists(this.asExpr().getExpr().getEnclosingMethod()) and
536536
this.asExpr().getExpr().getEnclosingModule() = modelClass and
537-
this.getMethodName() = ["has_one", "has_many", "belongs_to", "has_and_belongs_to_many"]
537+
this.getMethodName() =
538+
[
539+
"has_one", "has_many", "belongs_to", "has_and_belongs_to_many", "has_one_attached",
540+
"has_many_attached"
541+
]
538542
}
539543

540544
/**
@@ -584,21 +588,32 @@ private class ActiveRecordAssociation extends DataFlow::CallNode {
584588
}
585589

586590
/** Holds if this association is one-to-one */
587-
predicate isSingular() { this.getMethodName() = ["has_one", "belongs_to"] }
591+
predicate isSingular() { this.getMethodName() = ["has_one", "belongs_to", "has_one_attached"] }
588592

589593
/** Holds if this association is one-to-many or many-to-many */
590-
predicate isCollection() { this.getMethodName() = ["has_many", "has_and_belongs_to_many"] }
594+
predicate isCollection() {
595+
this.getMethodName() = ["has_many", "has_and_belongs_to_many", "has_many_attached"]
596+
}
591597
}
592598

593599
/**
594600
* Converts `input` to plural form.
601+
*
602+
* Examples:
603+
*
604+
* - photo -> photos
605+
* - story -> stories
606+
* - photos -> photos
595607
*/
596608
bindingset[input]
597609
bindingset[result]
598610
private string pluralize(string input) {
599611
exists(string stem | stem + "y" = input | result = stem + "ies")
600612
or
613+
not exists(string stem | stem + "s" = input) and
601614
result = input + "s"
615+
or
616+
exists(string stem | stem + "s" = input) and result = input
602617
}
603618

604619
/**

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

Lines changed: 210 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -8,23 +8,218 @@ private import codeql.ruby.Concepts
88
private import codeql.ruby.DataFlow
99
private import codeql.ruby.dataflow.FlowSummary
1010
private import codeql.ruby.frameworks.data.ModelsAsData
11+
private import codeql.ruby.frameworks.ActiveRecord
1112

12-
/** A call to `ActiveStorage::Filename#sanitized`, considered as a path sanitizer. */
13-
class ActiveStorageFilenameSanitizedCall extends Path::PathSanitization::Range, DataFlow::CallNode {
14-
ActiveStorageFilenameSanitizedCall() {
15-
this.getReceiver() =
16-
API::getTopLevelMember("ActiveStorage").getMember("Filename").getAnInstantiation() and
17-
this.getMethodName() = "sanitized"
13+
/**
14+
* Provides modeling for the `ActiveStorage` library.
15+
* Version: 7.0.
16+
*/
17+
module ActiveStorage {
18+
/** A call to `ActiveStorage::Filename#sanitized`, considered as a path sanitizer. */
19+
private class FilenameSanitizedCall extends Path::PathSanitization::Range, DataFlow::CallNode {
20+
FilenameSanitizedCall() {
21+
this =
22+
API::getTopLevelMember("ActiveStorage")
23+
.getMember("Filename")
24+
.getInstance()
25+
.getAMethodCall("sanitized")
26+
}
1827
}
19-
}
2028

21-
/** Taint related to `ActiveStorage::Filename`. */
22-
private class Summaries extends ModelInput::SummaryModelCsv {
23-
override predicate row(string row) {
24-
row =
25-
[
26-
"activestorage;;Member[ActiveStorage].Member[Filename].Method[new];Argument[0];ReturnValue;taint",
27-
"activestorage;;Member[ActiveStorage].Member[Filename].Instance.Method[sanitized];Argument[self];ReturnValue;taint",
28-
]
29+
/** Taint related to `ActiveStorage::Filename`. */
30+
private class FilenameSummaries extends ModelInput::SummaryModelCsv {
31+
override predicate row(string row) {
32+
row =
33+
[
34+
"activestorage;;Member[ActiveStorage].Member[Filename].Method[new];Argument[0];ReturnValue;taint",
35+
"activestorage;;Member[ActiveStorage].Member[Filename].Instance.Method[sanitized];Argument[self];ReturnValue;taint",
36+
]
37+
}
38+
}
39+
40+
/**
41+
* `Blob` is an instance of `ActiveStorage::Blob`.
42+
*/
43+
private class BlobTypeSummary extends ModelInput::TypeModelCsv {
44+
override predicate row(string row) {
45+
// package1;type1;package2;type2;path
46+
row =
47+
[
48+
// ActiveStorage::Blob.new : Blob
49+
"activestorage;Blob;activestorage;;Member[ActiveStorage].Member[Blob].Instance",
50+
// ActiveStorage::Blob.create_and_upload! : Blob
51+
"activestorage;Blob;activestorage;;Member[ActiveStorage].Member[Blob].Method[create_and_upload!].ReturnValue",
52+
// ActiveStorage::Blob.create_before_direct_upload! : Blob
53+
"activestorage;Blob;activestorage;;Member[ActiveStorage].Member[Blob].Method[create_before_direct_upload!].ReturnValue",
54+
// ActiveStorage::Blob.compose(blobs : [Blob]) : Blob
55+
"activestorage;Blob;activestorage;;Member[ActiveStorage].Member[Blob].Method[compose].ReturnValue",
56+
// gives error: Invalid name 'Element' in access path
57+
// "activestorage;Blob;activestorage;;Member[ActiveStorage].Member[Blob].Method[compose].Argument[0].Element[any]",
58+
// ActiveStorage::Blob.find_signed(!) : Blob
59+
"activestorage;Blob;activestorage;;Member[ActiveStorage].Member[Blob].Method[find_signed,find_signed!].ReturnValue",
60+
]
61+
}
62+
}
63+
64+
private class BlobInstance extends DataFlow::Node {
65+
BlobInstance() {
66+
this = ModelOutput::getATypeNode("activestorage", "Blob").getAValueReachableFromSource()
67+
or
68+
// ActiveStorage::Attachment#blob : Blob
69+
exists(DataFlow::CallNode call |
70+
call = this and
71+
call.getReceiver() instanceof AttachmentInstance and
72+
call.getMethodName() = "blob"
73+
)
74+
or
75+
// ActiveStorage::Attachment delegates method calls to its associated Blob
76+
this instanceof AttachmentInstance
77+
}
78+
}
79+
80+
/**
81+
* Method calls on `ActiveStorage::Blob` that send HTTP requests.
82+
*/
83+
private class BlobRequestCall extends Http::Client::Request::Range {
84+
BlobRequestCall() {
85+
this =
86+
[
87+
// Class methods
88+
API::getTopLevelMember("ActiveStorage")
89+
.getMember("Blob")
90+
.getASubclass()
91+
.getAMethodCall(["create_after_unfurling!", "create_and_upload!"]),
92+
// Instance methods
93+
any(BlobInstance i, DataFlow::CallNode c |
94+
i.(DataFlow::LocalSourceNode).flowsTo(c.getReceiver()) and
95+
c.getMethodName() =
96+
[
97+
"upload", "upload_without_unfurling", "download", "download_chunk", "delete",
98+
"purge"
99+
]
100+
|
101+
c
102+
)
103+
]
104+
}
105+
106+
override string getFramework() { result = "activestorage" }
107+
108+
override DataFlow::Node getResponseBody() { result = this }
109+
110+
override DataFlow::Node getAUrlPart() { none() }
111+
112+
override predicate disablesCertificateValidation(
113+
DataFlow::Node disablingNode, DataFlow::Node argumentOrigin
114+
) {
115+
none()
116+
}
117+
}
118+
119+
/**
120+
* A call to `has_one_attached` or `has_many_attached`, which declares an
121+
* association between an ActiveRecord model and an ActiveStorage attachment.
122+
*
123+
* ```rb
124+
* class User < ActiveRecord::Base
125+
* has_one_attached :avatar
126+
* end
127+
* ```
128+
*/
129+
private class Association extends ActiveRecordAssociation {
130+
Association() { this.getMethodName() = ["has_one_attached", "has_many_attached"] }
131+
}
132+
133+
/**
134+
* An ActiveStorage attachment, instantiated directly or via an association with an
135+
* ActiveRecord model.
136+
*
137+
* ```rb
138+
* class User < ActiveRecord::Base
139+
* has_one_attached :avatar
140+
* end
141+
*
142+
* user = User.find(id)
143+
* user.avatar
144+
* ActiveStorage::Attachment.new
145+
* ```
146+
*/
147+
class AttachmentInstance extends DataFlow::Node {
148+
AttachmentInstance() {
149+
this =
150+
API::getTopLevelMember("ActiveStorage")
151+
.getMember("Attachment")
152+
.getInstance()
153+
.getAValueReachableFromSource()
154+
or
155+
exists(Association assoc, string model, DataFlow::CallNode call |
156+
model = assoc.getTargetModelName()
157+
|
158+
call = this and
159+
call.getReceiver().(ActiveRecordInstance).getClass() = assoc.getSourceClass() and
160+
call.getMethodName() = model
161+
)
162+
or
163+
any(AttachmentInstance i).(DataFlow::LocalSourceNode).flowsTo(this)
164+
}
165+
}
166+
167+
/**
168+
* A call on an ActiveStorage object that results in an image transformation.
169+
* Arguments to these calls may be executed as system commands.
170+
*/
171+
private class ImageProcessingCall extends DataFlow::CallNode, SystemCommandExecution::Range {
172+
ImageProcessingCall() {
173+
this.getReceiver() instanceof BlobInstance and
174+
this.getMethodName() = ["variant", "preview", "representation"]
175+
or
176+
this =
177+
API::getTopLevelMember("ActiveStorage")
178+
.getMember("Attachment")
179+
.getInstance()
180+
.getAMethodCall(["variant", "preview", "representation"])
181+
or
182+
this =
183+
API::getTopLevelMember("ActiveStorage")
184+
.getMember("Variation")
185+
.getAMethodCall(["new", "wrap", "encode"])
186+
or
187+
this =
188+
API::getTopLevelMember("ActiveStorage")
189+
.getMember("Variation")
190+
.getInstance()
191+
.getAMethodCall("transformations=")
192+
or
193+
this =
194+
API::getTopLevelMember("ActiveStorage")
195+
.getMember("Transformers")
196+
.getMember("ImageProcessingTransformer")
197+
.getAMethodCall("new")
198+
or
199+
this =
200+
API::getTopLevelMember("ActiveStorage")
201+
.getMember(["Preview", "VariantWithRecord"])
202+
.getAMethodCall("new")
203+
or
204+
// `ActiveStorage.paths` is a global hash whose values are passed to
205+
// a `system` call.
206+
this = API::getTopLevelMember("ActiveStorage").getAMethodCall("paths=")
207+
or
208+
// `ActiveStorage.video_preview_arguments` is passed to a `system` call.
209+
this = API::getTopLevelMember("ActiveStorage").getAMethodCall("video_preview_arguments=")
210+
}
211+
212+
override DataFlow::Node getAnArgument() { result = this.getArgument(0) }
213+
}
214+
215+
/**
216+
* `ActiveStorage.variant_processor` is passed to `const_get`.
217+
*/
218+
private class VariantProcessor extends DataFlow::CallNode, CodeExecution::Range {
219+
VariantProcessor() {
220+
this = API::getTopLevelMember("ActiveStorage").getAMethodCall("variant_processor=")
221+
}
222+
223+
override DataFlow::Node getCode() { result = this.getArgument(0) }
29224
}
30225
}

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

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@ actionControllerControllerClasses
33
| active_record/ActiveRecord.rb:41:1:64:3 | BarController |
44
| active_record/ActiveRecord.rb:66:1:94:3 | BazController |
55
| active_record/ActiveRecord.rb:96:1:104:3 | AnnotatedController |
6+
| active_storage/active_storage.rb:39:1:45:3 | PostsController |
67
| app/controllers/comments_controller.rb:1:1:7:3 | CommentsController |
78
| app/controllers/foo/bars_controller.rb:3:1:46:3 | BarsController |
89
| app/controllers/photos_controller.rb:1:1:4:3 | PhotosController |
@@ -21,6 +22,7 @@ actionControllerActionMethods
2122
| active_record/ActiveRecord.rb:91:3:93:5 | update3 |
2223
| active_record/ActiveRecord.rb:97:3:99:5 | index |
2324
| active_record/ActiveRecord.rb:101:3:103:5 | unsafe_action |
25+
| active_storage/active_storage.rb:40:3:44:5 | create |
2426
| app/controllers/comments_controller.rb:2:3:3:5 | index |
2527
| app/controllers/comments_controller.rb:5:3:6:5 | show |
2628
| app/controllers/foo/bars_controller.rb:5:3:7:5 | index |
@@ -59,6 +61,8 @@ paramsCalls
5961
| active_record/ActiveRecord.rb:92:28:92:33 | call to params |
6062
| active_record/ActiveRecord.rb:92:53:92:58 | call to params |
6163
| active_record/ActiveRecord.rb:102:59:102:64 | call to params |
64+
| active_storage/active_storage.rb:41:21:41:26 | call to params |
65+
| active_storage/active_storage.rb:42:24:42:29 | call to params |
6266
| app/controllers/foo/bars_controller.rb:13:21:13:26 | call to params |
6367
| app/controllers/foo/bars_controller.rb:14:10:14:15 | call to params |
6468
| app/controllers/foo/bars_controller.rb:21:21:21:26 | call to params |
@@ -89,6 +93,8 @@ paramsSources
8993
| active_record/ActiveRecord.rb:92:28:92:33 | call to params |
9094
| active_record/ActiveRecord.rb:92:53:92:58 | call to params |
9195
| active_record/ActiveRecord.rb:102:59:102:64 | call to params |
96+
| active_storage/active_storage.rb:41:21:41:26 | call to params |
97+
| active_storage/active_storage.rb:42:24:42:29 | call to params |
9298
| app/controllers/foo/bars_controller.rb:13:21:13:26 | call to params |
9399
| app/controllers/foo/bars_controller.rb:14:10:14:15 | call to params |
94100
| app/controllers/foo/bars_controller.rb:21:21:21:26 | call to params |

‎ruby/ql/test/library-tests/frameworks/active_storage/ActiveStorage.expected‎

Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,47 @@
1+
attachmentInstances
2+
| active_storage.rb:11:1:11:25 | ... = ... |
3+
| active_storage.rb:11:1:11:25 | ... = ... |
4+
| active_storage.rb:11:15:11:25 | call to avatar |
5+
| active_storage.rb:13:1:13:11 | user_avatar |
6+
| active_storage.rb:14:1:14:11 | user_avatar |
7+
| active_storage.rb:15:1:15:11 | user_avatar |
8+
| active_storage.rb:17:1:17:11 | call to avatar |
9+
| active_storage.rb:19:1:19:42 | ... = ... |
10+
| active_storage.rb:19:1:19:42 | ... = ... |
11+
| active_storage.rb:19:14:19:42 | call to new |
12+
| active_storage.rb:23:11:23:20 | attachment |
13+
| active_storage.rb:24:11:24:20 | attachment |
14+
| active_storage.rb:25:18:25:27 | attachment |
15+
| active_storage.rb:42:5:42:15 | call to images |
16+
| active_storage.rb:73:1:73:10 | attachment |
17+
| active_storage.rb:74:1:74:10 | attachment |
18+
httpRequests
19+
| active_storage.rb:50:1:50:74 | call to create_after_unfurling! | activestorage | active_storage.rb:50:1:50:74 | call to create_after_unfurling! |
20+
| active_storage.rb:51:8:51:76 | call to create_and_upload! | activestorage | active_storage.rb:51:8:51:76 | call to create_and_upload! |
21+
| active_storage.rb:53:1:53:11 | call to upload | activestorage | active_storage.rb:53:1:53:11 | call to upload |
22+
| active_storage.rb:54:1:54:29 | call to upload_without_unfurling | activestorage | active_storage.rb:54:1:54:29 | call to upload_without_unfurling |
23+
| active_storage.rb:55:1:55:13 | call to download | activestorage | active_storage.rb:55:1:55:13 | call to download |
24+
| active_storage.rb:56:1:56:19 | call to download_chunk | activestorage | active_storage.rb:56:1:56:19 | call to download_chunk |
25+
| active_storage.rb:57:1:57:11 | call to delete | activestorage | active_storage.rb:57:1:57:11 | call to delete |
26+
| active_storage.rb:58:1:58:10 | call to purge | activestorage | active_storage.rb:58:1:58:10 | call to purge |
27+
| active_storage.rb:61:1:61:11 | call to upload | activestorage | active_storage.rb:61:1:61:11 | call to upload |
28+
| active_storage.rb:65:1:65:11 | call to upload | activestorage | active_storage.rb:65:1:65:11 | call to upload |
29+
| active_storage.rb:68:1:68:11 | call to upload | activestorage | active_storage.rb:68:1:68:11 | call to upload |
30+
| active_storage.rb:71:1:71:11 | call to upload | activestorage | active_storage.rb:71:1:71:11 | call to upload |
31+
| active_storage.rb:73:1:73:22 | call to upload | activestorage | active_storage.rb:73:1:73:22 | call to upload |
32+
| active_storage.rb:74:1:74:17 | call to upload | activestorage | active_storage.rb:74:1:74:17 | call to upload |
33+
commandExecutions
34+
| active_storage.rb:17:1:17:48 | call to variant | active_storage.rb:17:21:17:47 | Pair |
35+
| active_storage.rb:23:11:23:57 | call to variant | active_storage.rb:23:30:23:56 | Pair |
36+
| active_storage.rb:24:11:24:44 | call to preview | active_storage.rb:24:30:24:43 | Pair |
37+
| active_storage.rb:25:18:25:59 | call to representation | active_storage.rb:25:44:25:58 | Pair |
38+
| active_storage.rb:28:1:28:25 | call to transformations= | active_storage.rb:28:29:28:43 | ... = ... |
39+
| active_storage.rb:30:15:30:90 | call to new | active_storage.rb:30:75:30:89 | transformations |
40+
| active_storage.rb:31:11:31:53 | call to new | active_storage.rb:31:38:31:52 | transformations |
41+
| active_storage.rb:32:11:32:63 | call to new | active_storage.rb:32:48:32:62 | transformations |
42+
| active_storage.rb:34:1:34:19 | call to paths= | active_storage.rb:34:23:34:60 | ... = ... |
43+
| active_storage.rb:35:1:35:37 | call to video_preview_arguments= | active_storage.rb:35:41:35:59 | ... = ... |
44+
codeExecutions
45+
| active_storage.rb:37:1:37:31 | call to variant_processor= | active_storage.rb:37:35:37:50 | ... = ... |
46+
pathSanitizations
47+
| active_storage.rb:48:1:48:18 | call to sanitized |

‎ruby/ql/test/library-tests/frameworks/active_storage/ActiveStorage.ql‎

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,18 @@
1+
import ruby
2+
import codeql.ruby.ApiGraphs
3+
import codeql.ruby.Concepts
4+
import codeql.ruby.frameworks.ActiveStorage
5+
6+
query predicate attachmentInstances(ActiveStorage::AttachmentInstance n) { any() }
7+
8+
query predicate httpRequests(Http::Client::Request r, string framework, DataFlow::Node responseBody) {
9+
r.getFramework() = framework and r.getResponseBody() = responseBody
10+
}
11+
12+
query predicate commandExecutions(SystemCommandExecution c, DataFlow::Node arg) {
13+
arg = c.getAnArgument()
14+
}
15+
16+
query predicate codeExecutions(CodeExecution e, DataFlow::Node code) { code = e.getCode() }
17+
18+
query predicate pathSanitizations(Path::PathSanitization p) { any() }

0 commit comments

Comments
 (0)