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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support fetch from both arrays and hashes #1589

Merged
merged 2 commits into from May 9, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
9 changes: 9 additions & 0 deletions lib/brakeman/processors/alias_processor.rb
Expand Up @@ -227,6 +227,15 @@ def process_call exp
elsif hash? target
exp = process_hash_access(target, first_arg, exp)
end
when :fetch
if array? target
# Not dealing with default value
# so just pass in first argument, but process_array_access expects
# an array of arguments.
exp = process_array_access(target, [first_arg], exp)
elsif hash? target
exp = process_hash_access(target, first_arg, exp)
end
when :merge!, :update
if hash? target and hash? first_arg
target = process_hash_merge! target, first_arg
Expand Down
2 changes: 2 additions & 0 deletions lib/brakeman/processors/lib/call_conversion_helper.rb
Expand Up @@ -76,6 +76,8 @@ def process_array_access array, args, original_exp = nil

#Have to do this because first element is :array and we have to skip it
array[1..-1][index] or original_exp
elsif all_literals? array
safe_literal(array.line)
else
original_exp
end
Expand Down
6 changes: 6 additions & 0 deletions test/apps/rails6/app/models/group.rb
Expand Up @@ -12,4 +12,10 @@ def ar_sanitize_sql_like(query)
query = ActiveRecord::Base.sanitize_sql_like(query) # escaped variable
Arel.sql("name ILIKE '%#{query}%'")
end

def fetch_constant_hash_value(role_name)
roles = { admin: 1, moderator: 2 }.freeze
role = roles.fetch(role_name)
Arel.sql("role = '#{role}'")
end
end
28 changes: 28 additions & 0 deletions test/tests/alias_processor.rb
Expand Up @@ -123,6 +123,20 @@ def test_array_negative_index
RUBY
end

def test_array_fetch
assert_alias '3', <<-RUBY
x = [1, 2, 3]
x.fetch(2)
RUBY
end

def test_array_fetch_unknown_literal
assert_alias ':BRAKEMAN_SAFE_LITERAL', <<-RUBY
x = [1, 2, 3]
y = x.fetch(z)
y
RUBY
end

def test_array_append
assert_alias '[1, 2, 3]', <<-RUBY
Expand Down Expand Up @@ -208,6 +222,20 @@ def test_hash_new_index
RUBY
end

def test_hash_fetch
assert_alias '1', <<-RUBY
x = { a: 0, b: 1, c: 3 }
x.fetch(:b)
RUBY
end

def test_hash_fetch_unknown_literal
assert_alias ':BRAKEMAN_SAFE_LITERAL', <<-RUBY
x = { a: 0, b: 1, c: 3 }
x.fetch(:z)
RUBY
end

def test_hash_update
assert_alias "2", <<-RUBY
@foo = {
Expand Down
14 changes: 13 additions & 1 deletion test/tests/rails6.rb
Expand Up @@ -134,7 +134,6 @@ def test_sql_injection_date_integer_target_false_positive
:user_input => s(:call, s(:call, s(:lit, 30), :days), :ago)
end


def test_sql_injection_sanitize_sql_like
assert_no_warning :type => :warning,
:warning_code => 0,
Expand All @@ -148,6 +147,19 @@ def test_sql_injection_sanitize_sql_like
:user_input => s(:call, s(:colon2, s(:const, :ActiveRecord), :Base), :sanitize_sql_like, s(:lvar, :query))
end

def test_sql_injection_hash_fetch_all_literals
assert_no_warning :type => :warning,
:warning_code => 0,
:fingerprint => "1b9a04c9fdc4b8c7f215387fb726dc542c2d35dde2f29b48a76248443524a5fa",
:warning_type => "SQL Injection",
:line => 14,
:message => /^Possible\ SQL\ injection/,
:confidence => 1,
:relative_path => "app/models/group.rb",
:code => s(:call, s(:const, :Arel), :sql, s(:dstr, "role = '", s(:evstr, s(:call, s(:hash, s(:lit, :admin), s(:lit, 1), s(:lit, :moderator), s(:lit, 2)), :fetch, s(:lvar, :role_name))), s(:str, "'"))),
:user_input => s(:call, s(:hash, s(:lit, :admin), s(:lit, 1), s(:lit, :moderator), s(:lit, 2)), :fetch, s(:lvar, :role_name))
end

def test_cross_site_scripting_sanity
assert_warning :type => :template,
:warning_code => 2,
Expand Down