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

Another SQL False positive #655

Closed
phene opened this Issue May 13, 2015 · 4 comments

Comments

Projects
None yet
2 participants
@phene
Contributor

phene commented May 13, 2015

String literals with no interpolation should not be treated as SQL injection.

Error

High Confidence
Possible SQL injection near line 48: joins(:things).joins("LEFT JOIN thing_memberships ON items.thing_id = thing_memberships.thing_id").where(:tenant_id => user.tenant_id).where(Thing.member_or_visibility(user, [Thing::VISIBILITY_PUBLIC, Thing::VISIBILITY_COMPANY]))

Code

class Thing
  scope :viewable_by, ->(user) {
    member_join = 'LEFT JOIN thing_memberships ON items.thing_id = thing_memberships.thing_id'
    viewable_thing = Thing.member_or_visibility(user, [Thing::VISIBILITY_PUBLIC, Thing::VISIBILITY_COMPANY])
    joins(:things).joins(member_join).where(:tenant_id => user.tenant_id).where(viewable_thing).distinct
  }
end
@presidentbeef

This comment has been minimized.

Show comment
Hide comment
@presidentbeef

presidentbeef May 13, 2015

Owner

The warning is on Thing.member_or_visibility, not the string literal.

Owner

presidentbeef commented May 13, 2015

The warning is on Thing.member_or_visibility, not the string literal.

@phene

This comment has been minimized.

Show comment
Hide comment
@phene

phene May 13, 2015

Contributor

Why does it assume member_or_visiblity returns something unsafe or put the error inside that method definition?

Contributor

phene commented May 13, 2015

Why does it assume member_or_visiblity returns something unsafe or put the error inside that method definition?

@presidentbeef

This comment has been minimized.

Show comment
Hide comment
@presidentbeef

presidentbeef May 13, 2015

Owner

Because anything potentially coming from the database is considered unsafe. In this case Thing must be an ActiveRecord model. It doesn't know anything about member_or_visiblity.

However, I do agree this should probably be bumped down to a weak confidence warning.

Owner

presidentbeef commented May 13, 2015

Because anything potentially coming from the database is considered unsafe. In this case Thing must be an ActiveRecord model. It doesn't know anything about member_or_visiblity.

However, I do agree this should probably be bumped down to a weak confidence warning.

@presidentbeef

This comment has been minimized.

Show comment
Hide comment
@presidentbeef

presidentbeef Jan 28, 2017

Owner

This is fixed by #985

Owner

presidentbeef commented Jan 28, 2017

This is fixed by #985

Repository owner locked and limited conversation to collaborators May 18, 2017

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