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

False positive with whitelisted strings in constant #1211

Closed
JacobEvelyn opened this Issue May 17, 2018 · 5 comments

Comments

Projects
None yet
2 participants
@JacobEvelyn
Contributor

JacobEvelyn commented May 17, 2018

Background

Brakeman version: 4.3.0
Rails version: 5.1.6
Ruby version: 2.3.4

False Positive

This is somewhat similar to #1208 but based on the behavior I think they're different enough for this to be a separate issue.

I noticed that with Brakeman correctly does not warn about SQL/command injection in this case:

def safe(foo)
  unless [:bar, :baz].include? foo
    raise ArgumentError, "Unexpected foo: #{foo}"
  end

  Person.where("#{foo} >= 1")
end

but Brakeman does produce a warning in this equally safe scenario:

ALLOWED_FOOS = [:bar, :baz].freeze
def safe(foo)
  unless ALLOWED_FOOS.include? foo
    raise ArgumentError, "Unexpected foo: #{foo}"
  end

  Person.where("#{foo} >= 1")
end
@presidentbeef

This comment has been minimized.

Owner

presidentbeef commented May 17, 2018

I'm going to guess it's the freeze call causing problems again.

@JacobEvelyn

This comment has been minimized.

Contributor

JacobEvelyn commented May 18, 2018

Ah yes, you're right. I forgot to test that with the constant but not .freeze.

@presidentbeef

This comment has been minimized.

Owner

presidentbeef commented Jun 6, 2018

I was wrong, here. Freezing a constant was already handled.

I'm not 100% sure what exactly fixed this, but I can't reproduce the issue on master.

@JacobEvelyn

This comment has been minimized.

Contributor

JacobEvelyn commented Jun 6, 2018

Hmm, I can reproduce this on master. If I go to brakeman/test/apps/rails5.2/app/controllers/users_controller.rb and add this:

ALLOWED_FOOS = [:bar, :baz].freeze
def delete(foo)
  unless ALLOWED_FOOS.include? foo
    raise ArgumentError, "Unexpected foo: #{foo}"
  end

  Person.where("#{foo} >= 1")
end

I see the warning.

@presidentbeef presidentbeef reopened this Jun 6, 2018

@presidentbeef

This comment has been minimized.

Owner

presidentbeef commented Jun 6, 2018

Okay, I see it now too. Weird 🤔

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

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.