Open EQuincerot opened 2 years ago
This is really unusual code.
There are two bits to this:
connection
calls on several potential targets, including ActiveRecord::Base
.find
with an arbitrary block.We could assume that any connection
call is going to be on an ActiveRecord model. That would introduce false positives, but maybe not very many.
But before we go down that road, it would be helpful to know if this was based on real code or not.
Yes this is the kind of code we use in production. As we are using multiple database, we have for example this use case:
DatabaseHelper.all_databases # returns Db1Record, Db2Record (abstract model classes with rules to connect to db1 or db2)
DatabaseHelper.all_databases.find { |db| db.connection.tables.include?(table) }.connection.execute_query("VACUUM ANALYZE #{table_name}")
Rely on connection
would work for us, I don't think that will cause false positives in our situation.
Background
Brakeman version: 5.2.1 Rails version: 6 and 7 Ruby version: 3.0.3p157
Issue
The same SQL injection risk exists in both functions. The first one is not detected whereas the second one is correctly detected.