Ruby: Add OrmWriteAccess concept to model writes to a DB using an ORM
#8271
Conversation
…ssignment node rather than the setter call
ruby/ql/lib/codeql/ruby/Concepts.qll
Outdated
| * Extend this class to refine existing API models. If you want to model new APIs, | ||
| * extend `OrmWriteAccess::Range` instead. | ||
| */ | ||
| class OrmWriteAccess extends DataFlow::Node instanceof OrmWriteAccess::Range { |
There was a problem hiding this comment.
It might be worth thinking about whether this class would be useful when shared with Python/JS. JS already has a PersistentWriteAccess class that models cookie and web storage writes - I wonder if that would also cover this case, or if we need a more specific ORM concept?
There was a problem hiding this comment.
I think that PersistentWriteAccess is fine here. I'd initially thought that having the fieldName available would be important for implementing rb/clear-text-storage-sensitive-information, but in practice I haven't found this necessary.
I've added PersistentWriteAccess as a Range pattern class for the Ruby version of this - the JS version of it is a normal abstract class.
| | | ||
| keyExpr.getConstantValue().isStringOrSymbol(result) and | ||
| // avoid vacuous matches where the key is not a string or not a symbol | ||
| not result = "" and |
There was a problem hiding this comment.
This looks like a bug in getConstantValue - the only expressions that should have an empty string constant value are :"" and "". We should probably open an issue for this, to come back and fix later.
There was a problem hiding this comment.
| @@ -0,0 +1,2 @@ | |||
| class User < ApplicationRecord | |||
| end | |||
There was a problem hiding this comment.
It would be good to have some calls in this class to test we identify them as well, since that's quite common. E.g.
class User
def make_admin!
update!(is_admin: true)
end
end…e.ModifyAndSaveCall
|
@hmac thanks for the review. As part of switching to use |
Looks good! There's a few QL4QL alerts that we might want to fix (I think maybe a new check just got merged?). Otherwise
|
This is implemented for
ActiveRecord::Persistencemethods - in particular those that include a field name and value as part of the method call. This notably doesn't includesaveordestroyand similar methods.The immediate intention for this change is to support a plaintext storage query including database sinks.
The main limitation of the modelling here is that it assumes that these methods are called with literal arguments - e.g.
new_name = "foo"; user.update({name: new_name})would be picked up since the argument is a hash literal, butnew_attrs = {name: "foo"}; user.update(new_attrs)would not as the argument is a variable read.On a separate note, assignments such as
user.name = "foo", whereuseris anActiveRecordmodel object, are included as DB writes here. This is not strictly true, as the write only occurs if the object is subsequently persisted (e.g. with asavecall). It seemed worth pretending that these were in fact writes to avoid cases where the flow from the assignment to the actual write may be too complex for us to track, in which case we would miss potential writes. The tradeoff of FPs where there is an assignment without a subsequent write seems worthwhile to me since this seems like it would be an uncommon thing to do, but I may not have considered all use cases.See https://apidock.com/rails/v6.1.3.1/ActiveRecord/Persistence/ and https://apidock.com/rails/v6.1.3.1/ActiveRecord/Persistence/ClassMethods/ for reference.