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

Add representation of safe literals #1227

Merged
merged 10 commits into from Jun 2, 2018

Conversation

Projects
None yet
1 participant
@presidentbeef
Owner

presidentbeef commented Jun 2, 2018

This changes how Brakeman handles a number of situations and opens the door to similar improvements in the future.

In particular:

In this code, Brakeman will know i is a safe literal value:

[1, 2, 3, 4].each do |i|
   use(i)
end

and here, too:

["blah", "this", "that"].map do |i|
   transform(i)
end

Also here Brakeman will know x[anything] is a "safe" literal value, even though the value of anything is not known:

x = { this: "that" }
x[anything]

For these examples, Brakeman will know x is a literal value, but won't cause confusion by picking an exact one:

raise unless [:a, :b, :c, :d].include? x

x = [1, 2, 3, 4, 5].detect { |i| i.odd? }

Brakeman will also handle this kind of thing:

if ["ls", "cat", "who"].include? y
  x = y + "blah"
  `#{x} stuff}
end

Adding together string literals or number literals with a known safe literal will produce a safe literal. This is somewhat limited right now but will probably expand in the future.


Previously, if Brakeman saw code like this

if [1, 0].include? x
  puts x
end

It would change it to

if [1, 0].include? x
  puts 1
end

By setting x to 1 inside the branch, Brakeman could indicate that x was a 'safe' value.

But what if it were something like

if [1, 0].include? x
  eval [params[:x], "nothing"][x]
end

Brakeman wouldn't warn about this because it would see

if [1, 0].include? x
  eval "nothing"
end

Oops!

What is needed is a value to indicate a safe value without it causing this kind of downstream problem.

For now, that value is :BRAKEMAN_SAFE_VALUE.

Now our example would look like

if [1, 0].include? x
  eval [params[:x], "nothing"][:BRAKEMAN_SAFE_VALUE]
end

and Brakeman will warn about evaling a query parameter.

@presidentbeef presidentbeef merged commit b4febe5 into master Jun 2, 2018

3 of 5 checks passed

codeclimate 2 issues to fix
Details
codeclimate/diff-coverage 85% (90% threshold)
Details
ci/circleci Your tests passed on CircleCI!
Details
codeclimate/total-coverage 94% (0.0% change)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@presidentbeef presidentbeef deleted the safety_values branch Jun 2, 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.