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

Possible strong params false positive #1180

Closed
JacobEvelyn opened this Issue Apr 23, 2018 · 2 comments

Comments

Projects
None yet
2 participants
@JacobEvelyn
Contributor

JacobEvelyn commented Apr 23, 2018

Background

Brakeman version: 4.2.1
Rails version: 5.1.6
Ruby version: 2.5.1

False Positive

I believe this might be a false positive (but I could definitely be missing some nuance!). If we have this in a controller:

def index
  # Using Sequel, though don't think it matters.
  render json: Person.where(index_params).qualify.all
end

private

def index_params
  # The `to_hash` is necessary for Sequel's `where` method.
  params.permit(:name, friend_names: []).to_hash
end

Brakeman gives us:

Confidence: Medium
Category: SQL Injection
Check: SQL
Message: Possible SQL injection
Code: Person.where(params.permit(:name, friend_names: []).to_hash)
File: app/controllers/people_controller.rb
Line: 4

I believe this is a false positive because we've already used the strong params API to whitelist the query arguments, and because Sequel will sanitize inputs passed in as a hash. I could be missing something though!

@presidentbeef

This comment has been minimized.

Owner

presidentbeef commented Apr 24, 2018

Yes, it is a false positive. Support for not warning on permit was added but I suppose it should check for to_h and to_hash as well.

@JacobEvelyn

This comment has been minimized.

Contributor

JacobEvelyn commented Apr 24, 2018

Glad my read was correct. Is this something you'd like to implement or would you prefer I take a stab at it?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.