From bedbfd3609c322204c79b4fa0b489946b8394b38 Mon Sep 17 00:00:00 2001 From: Justin Collins Date: Sun, 2 May 2021 17:51:25 -0700 Subject: [PATCH] Treat (Hash|Array)#fetch like (Hash|Array)#[] One of the issues from #1571 --- lib/brakeman/processors/alias_processor.rb | 9 +++++++++ test/apps/rails6/app/models/group.rb | 6 ++++++ test/tests/alias_processor.rb | 13 +++++++++++++ test/tests/rails6.rb | 14 +++++++++++++- 4 files changed, 41 insertions(+), 1 deletion(-) diff --git a/lib/brakeman/processors/alias_processor.rb b/lib/brakeman/processors/alias_processor.rb index 262408c155..58117490c1 100644 --- a/lib/brakeman/processors/alias_processor.rb +++ b/lib/brakeman/processors/alias_processor.rb @@ -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 diff --git a/test/apps/rails6/app/models/group.rb b/test/apps/rails6/app/models/group.rb index ecdc890177..a2fc777764 100644 --- a/test/apps/rails6/app/models/group.rb +++ b/test/apps/rails6/app/models/group.rb @@ -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 diff --git a/test/tests/alias_processor.rb b/test/tests/alias_processor.rb index 82985cddf4..895aee1630 100644 --- a/test/tests/alias_processor.rb +++ b/test/tests/alias_processor.rb @@ -123,6 +123,12 @@ 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_append assert_alias '[1, 2, 3]', <<-RUBY @@ -208,6 +214,13 @@ 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_update assert_alias "2", <<-RUBY @foo = { diff --git a/test/tests/rails6.rb b/test/tests/rails6.rb index 7cb82b842b..0b340f2cf7 100644 --- a/test/tests/rails6.rb +++ b/test/tests/rails6.rb @@ -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, @@ -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,