Skip to content

Commit fd8f1dc

Browse files
committed
Ruby: fix some misidentification of ActiveRecordModelInstantiations
1 parent 4cf3467 commit fd8f1dc

File tree

2 files changed

+17
-4
lines changed

2 files changed

+17
-4
lines changed

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

Lines changed: 16 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -275,7 +275,18 @@ private class ActiveRecordModelFinderCall extends ActiveRecordModelInstantiation
275275
exists(MethodCall call, Expr recv |
276276
call = this.asExpr().getExpr() and
277277
recv = getUltimateReceiver(call) and
278-
resolveConstant(recv) = cls.getAQualifiedName() and
278+
(
279+
// The receiver refers to an `ActiveRecordModelClass` by name
280+
resolveConstant(recv) = cls.getAQualifiedName()
281+
or
282+
// The receiver is self, and the call is within a singleton method of
283+
// the `ActiveRecordModelClass`
284+
recv instanceof SelfVariableAccess and
285+
exists(SingletonMethod callScope |
286+
callScope = call.getCfgScope() and
287+
callScope = cls.getAMethod()
288+
)
289+
) and
279290
call.getMethodName() = finderMethodName()
280291
)
281292
}
@@ -293,7 +304,10 @@ private class ActiveRecordModelClassSelfReference extends ActiveRecordModelInsta
293304
m = this.getCfgScope() and
294305
m.getEnclosingModule() = cls and
295306
m = cls.getAMethod()
296-
)
307+
) and
308+
// In a singleton method, `self` refers to the class itself rather than an
309+
// instance of that class
310+
not this.getSelfScope() instanceof SingletonMethod
297311
}
298312

299313
final override ActiveRecordModelClass getClass() { result = cls }

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

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -45,9 +45,8 @@ potentiallyUnsafeSqlExecutingMethodCall
4545
| ActiveRecordInjection.rb:75:5:75:29 | call to order |
4646
| ActiveRecordInjection.rb:80:7:80:40 | call to find_by |
4747
activeRecordModelInstantiations
48-
| ActiveRecordInjection.rb:8:3:11:5 | self (authenticate) | ActiveRecordInjection.rb:5:1:17:3 | User |
48+
| ActiveRecordInjection.rb:10:5:10:68 | call to find | ActiveRecordInjection.rb:5:1:17:3 | User |
4949
| ActiveRecordInjection.rb:15:5:15:40 | call to find_by | ActiveRecordInjection.rb:1:1:3:3 | UserGroup |
50-
| ActiveRecordInjection.rb:20:3:24:5 | self (delete_by) | ActiveRecordInjection.rb:19:1:25:3 | Admin |
5150
| ActiveRecordInjection.rb:80:7:80:40 | call to find_by | ActiveRecordInjection.rb:5:1:17:3 | User |
5251
| ActiveRecordInjection.rb:85:5:85:33 | call to find_by | ActiveRecordInjection.rb:5:1:17:3 | User |
5352
| ActiveRecordInjection.rb:88:5:88:34 | call to find | ActiveRecordInjection.rb:5:1:17:3 | User |

0 commit comments

Comments
 (0)