Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ruby: Track flow into ActiveRecord scopes #14426

Merged
merged 6 commits into from
Mar 19, 2024
Merged

Conversation

hmac
Copy link
Contributor

@hmac hmac commented Oct 10, 2023

Track flow into ActiveRecord scopes, e.g.

class User < ApplicationRecord
  scope :with_role, ->(role) { where("role = #{role}") } #(3)
end

class UsersController < ApplicationController
  def show
    role = params[:role] # (1)
    @users = User.with_role(role) # (2)
  end
end

We will track flow from role assigned in (1), through the call to with_role (2) which targets the lambda at (3). We do this by adding an additional call step.

Also add a few missing flow summaries for Hash.

@github-actions github-actions bot added the Ruby label Oct 10, 2023
hmac added 3 commits March 18, 2024 15:01
When `map` is called on a hash, the values in the hash are passed to the
second parameter of the block.
@hmac hmac marked this pull request as ready for review March 19, 2024 09:57
@hmac hmac requested a review from a team as a code owner March 19, 2024 09:57
Copy link
Contributor

@hvitved hvitved left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, a couple of questions.

Comment on lines 533 to 535
input = "Argument[self].Element[any]" and
output = "ReturnValue.Element[?]" and
preservesValue = true
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couldn't this result in incorrect flow, such as sink({:a => taint}.keys[0])? It would probably be better to just have input = Argument[self] and output = ReturnValue.Element[?] and preservesValue = false.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, admittedly it is a broad summary. I will change it

Comment on lines 1007 to 1008
keys = h.keys
sink(keys[:a]) # $ hasValueFlow=55.1
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't keys an array of all the keys in h?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes sorry this should be sink(keys[some_index()]) I think

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But none of the keys are tainted, right? So perhaps instead of h[f()] = taint(55.1) you meant e.g. h[taint(55.1)] = nil?

Copy link
Contributor Author

@hmac hmac Mar 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes that it also true, but I believe we don't yet track taint in Hash keys anyway, so this is a bit of an approximation. Given the change to the summary you suggested, I've changed the test to just taint the whole hash:

h = taint(55.1)
keys = h.keys
sink(keys[f()])

This gives the specific behaviour I'm looking for in the context of modelling Rails params, because we taint the whole params object which is a Hash-like thing.

@hmac hmac merged commit 219cd4e into github:main Mar 19, 2024
21 checks passed
@hmac hmac deleted the hmac-ar-scopes branch March 19, 2024 14:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants