Merge pull request #1589 from presidentbeef/array_hash_fetch

Support `fetch` from both arrays and hashes

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

lib/brakeman/processors/lib/call_conversion_helper.rb

@@ -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

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

test/tests/alias_processor.rb

@@ -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
@@ -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 = {

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,