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

Consider adding `:foreign_key` to `IGNORE_METHODS_IN_SQL`? #1202

Closed
JacobEvelyn opened this Issue May 10, 2018 · 1 comment

Comments

Projects
None yet
2 participants
@JacobEvelyn
Copy link
Contributor

JacobEvelyn commented May 10, 2018

Background

Brakeman version: 4.2.1
Rails version: 5.2.0
Ruby version: 2.3.4

False Positive

Brakeman's warning us about a dynamic query we build up that uses an ActiveRecord association reflection to build a query. The code looks something like this:

assoc_reflection = reflect_on_association(:foos)
foreign_key = assoc_reflection.foreign_key

Bar.joins("INNER JOIN <complex join involving custom SQL and #{foreign_key} interpolation>")

I believe this is a false positive akin to the primary_key that Brakeman ignores, but of course any additional exceptions for false positives run the risk of missing actual vulnerabilities (for instance, when a user-defined method shares that name but generates the value using unsafe user input). It's not a big deal for us either way; we can just use ApplicationRecord.connection.quote_column_name, but I'm wondering what you think about this sort of thing more generally.

@presidentbeef

This comment has been minimized.

Copy link
Owner

presidentbeef commented May 30, 2018

I think it's fine.

presidentbeef added a commit that referenced this issue Jun 6, 2018

presidentbeef added a commit that referenced this issue Jun 6, 2018

Repository owner locked and limited conversation to collaborators Jul 14, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.