Ruby: Add OrmWriteAccess concept to model writes to a DB using an ORM

[alexrford]

This is implemented for ActiveRecord::Persistence methods - in particular those that include a field name and value as part of the method call. This notably doesn't include save or destroy and 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, but new_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", where user is an ActiveRecord model 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 a save call). 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.

[hmac]

Nice! Just a few minor comments.

[hmac]

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?

[alexrford]

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.

[hmac]

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.

[alexrford]

👍 opened an issue for this - I tracked it as far as https://github.com/github/codeql/blob/main/ruby/ql/lib/codeql/ruby/controlflow/CfgNodes.qll#L836-L846 returning the empty string for non-empty symbol literal expr nodes, but I'm not sure on what the right fix is.

[hmac]

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

[alexrford]

@hmac thanks for the review. As part of switching to use PersistentWriteAccess, I was able to drop the code in ActiveRecord::Persistence that dealt with tracking the field name. We can always add this back later as a separate predicate if it's useful, but it doesn't seem to be required for now.

[hmac]

Looks good! There's a few QL4QL alerts that we might want to fix (I think maybe a new check just got merged?). Otherwise 👍

[alexrford]

There's a few QL4QL alerts that we might want to fix

👍 I'll address these in a followup PR.