Skip to content

Commit

Permalink
Merge pull request #1049 from presidentbeef/exists_to_s
Browse files Browse the repository at this point in the history
Do not warn about `to_s` in `exists?`
  • Loading branch information
presidentbeef committed May 15, 2017
2 parents 7478021 + c754167 commit cbfff54
Show file tree
Hide file tree
Showing 3 changed files with 28 additions and 1 deletion.
12 changes: 11 additions & 1 deletion lib/brakeman/checks/check_sql.rb
Original file line number Diff line number Diff line change
Expand Up @@ -164,7 +164,9 @@ def process_result result
dangerous_value = case method
when :find
check_find_arguments call.second_arg
when :exists?, :delete_all, :destroy_all
when :exists?
check_exists call.first_arg
when :delete_all, :destroy_all
check_find_arguments call.first_arg
when :named_scope, :scope
check_scope_arguments call
Expand Down Expand Up @@ -633,6 +635,14 @@ def check_call exp
end
end

def check_exists arg
if call? arg and arg.method == :to_s
false
else
check_find_arguments arg
end
end

#Prior to Rails 2.1.1, the :offset and :limit parameters were not
#escaping input properly.
#
Expand Down
4 changes: 4 additions & 0 deletions test/apps/rails4/app/controllers/users_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -120,4 +120,8 @@ def permit_in_sql
User.find_by(params.permit(:OMG)[:OMG]) # Warn
User.where("#{params.permit(:OMG)}") # Warn
end

def exists_with_to_s
User.exists? params[:x].to_s # Don't warn
end
end
13 changes: 13 additions & 0 deletions test/tests/rails4.rb
Original file line number Diff line number Diff line change
Expand Up @@ -354,6 +354,19 @@ def test_sql_injection_in_find_by!
:user_input => s(:call, s(:params), :[], s(:lit, :user_search))
end

def test_sql_injection_exists_to_s
assert_no_warning :type => :warning,
:warning_code => 0,
:fingerprint => "7161505aefba0df5f732d479904a220ea52f0aff3ab1bfa4d8b6170854943d7e",
:warning_type => "SQL Injection",
:line => 125,
:message => /^Possible\ SQL\ injection/,
:confidence => 0,
:relative_path => "app/controllers/users_controller.rb",
:code => s(:call, s(:const, :User), :exists?, s(:call, s(:call, s(:params), :[], s(:lit, :x)), :to_s)),
:user_input => s(:call, s(:call, s(:params), :[], s(:lit, :x)), :to_s)
end

def test_dynamic_render_path_with_before_action
assert_warning :type => :warning,
:warning_code => 99,
Expand Down

0 comments on commit cbfff54

Please sign in to comment.