Collapse send/try calls

lib/brakeman/checks/base_check.rb

@@ -181,9 +181,30 @@ class Brakeman::BaseCheck < Brakeman::SexpProcessor
       #May need to revisit dependng on what Rails 4 actually does/has
       @mass_assign_disabled = true
     else
-      matches = tracker.check_initializers(:"ActiveRecord::Base", :send)
+      #Check for ActiveRecord::Base.send(:attr_accessible, nil)
+      tracker.check_initializers(:"ActiveRecord::Base", :attr_accessible).each do |result|
+        call = result.call
+        if call? call
+          if call.first_arg == Sexp.new(:nil)
+            @mass_assign_disabled = true
+            break
+          end
+        end
+      end
 
-      if matches.empty?
+      unless @mass_assign_disabled
+        tracker.check_initializers(:"ActiveRecord::Base", :send).each do |result|
+          call = result.call
+          if call? call
+            if call.first_arg == Sexp.new(:lit, :attr_accessible) and call.second_arg == Sexp.new(:nil)
+              @mass_assign_disabled = true
+              break
+            end
+          end
+        end
+      end
+
+      unless @mass_assign_disabled
         #Check for
         #  class ActiveRecord::Base
         #    attr_accessible nil
@@ -200,17 +221,6 @@ class Brakeman::BaseCheck < Brakeman::SexpProcessor
             end
           end
         end
-      else
-        #Check for ActiveRecord::Base.send(:attr_accessible, nil)
-        matches.each do |result|
-          call = result.call
-          if call? call
-            if call.first_arg == Sexp.new(:lit, :attr_accessible) and call.second_arg == Sexp.new(:nil)
-              @mass_assign_disabled = true
-              break
-            end
-          end
-        end
       end
     end
 
@@ -229,10 +239,11 @@ class Brakeman::BaseCheck < Brakeman::SexpProcessor
       end
 
       unless @mass_assign_disabled
-        matches = tracker.check_initializers(:"ActiveRecord::Base", :send)
+        matches = tracker.check_initializers(:"ActiveRecord::Base", [:send, :include])
 
         matches.each do |result|
-          if call? result.call and result.call.second_arg == forbidden_protection
+          call = result.call
+          if call? call and (call.first_arg == forbidden_protection or call.second_arg == forbidden_protection)
             @mass_assign_disabled = true
           end
         end

lib/brakeman/processors/alias_processor.rb

@@ -88,6 +88,10 @@ class Brakeman::AliasProcessor < Brakeman::SexpProcessor
     method = exp.method
     first_arg = exp.first_arg
 
+    if method == :send or method == :try
+      collapse_send_call exp, first_arg
+    end
+
     if node_type? target, :or and [:+, :-, :*, :/].include? method
       res = process_or_simple_operation(exp)
       return res if res
@@ -543,6 +547,17 @@ class Brakeman::AliasProcessor < Brakeman::SexpProcessor
     end
   end
 
+  # Change x.send(:y, 1) to x.y(1)
+  def collapse_send_call exp, first_arg
+    return unless symbol? first_arg or string? first_arg
+    exp.method = first_arg.value.to_sym
+    args = exp.args
+    exp.pop # remove last arg
+    if args.length > 1
+      exp.arglist = args[1..-1]
+    end
+  end
+
   #Returns a new SexpProcessor::Environment containing only instance variables.
   #This is useful, for example, when processing views.
   def only_ivars include_request_vars = false, lenv = nil

lib/ruby_parser/bm_sexp.rb

@@ -163,6 +163,12 @@ class Sexp
     end
   end
 
+  def method= name
+    expect :call
+
+    self[2] = name
+  end
+
   #Sets the arglist in a method call.
   def arglist= exp
     expect :call, :attrasgn

test/apps/rails4/app/controllers/friendly_controller.rb

@@ -11,4 +11,9 @@ class FriendlyController
   def some_user_thing
     redirect_to @user.url
   end
+
+  def try_and_send
+    User.stuff.try(:where, params[:query])
+    User.send(:from, params[:table]).all
+  end
 end

test/tests/alias_processor.rb

@@ -156,8 +156,29 @@ class AliasProcessorTests < Test::Unit::TestCase
 
   def test_addition_chained
     assert_alias 'y + 5', <<-RUBY
-    x = y + 2 + 3
-    x
+      x = y + 2 + 3
+      x
+    RUBY
+  end
+
+  def test_send_collapse
+    assert_alias 'x.y(1)', <<-RUBY
+      z = x.send(:y, 1)
+      z
+    RUBY
+  end
+
+  def test_send_collapse_with_no_target
+    assert_alias 'y(1)', <<-RUBY
+      x = send(:y, 1)
+      x
+    RUBY
+  end
+
+  def test_try_collapse
+    assert_alias 'x.y', <<-RUBY
+      z = x.try(:y)
+      z
     RUBY
   end
 

test/tests/rails4.rb

@@ -15,7 +15,7 @@ class Rails4Tests < Test::Unit::TestCase
       :controller => 0,
       :model => 0,
       :template => 0,
-      :generic => 3
+      :generic => 5
     }
   end
 
@@ -121,4 +121,26 @@ class Rails4Tests < Test::Unit::TestCase
       :confidence => 0,
       :relative_path => "app/controllers/friendly_controller.rb"
   end
+
+  def test_try_and_send_collapsing_with_sqli
+    assert_warning :type => :warning,
+      :warning_code => 0,
+      :fingerprint => "c96c2984c1ce4f9a0f1205c9e7ac4707253a0553ecb6c7e9d6d4b88c92db7098",
+      :warning_type => "SQL Injection",
+      :line => 17,
+      :message => /^Possible\ SQL\ injection/,
+      :confidence => 0,
+      :relative_path => "app/controllers/friendly_controller.rb",
+      :user_input => s(:call, s(:params), :[], s(:lit, :table))
+
+    assert_warning :type => :warning,
+      :warning_code => 0,
+      :fingerprint => "004e5d6afb7ce520f1a67b65ace238f763ca2feb6a7f552f7dcc86ed3f67a189",
+      :warning_type => "SQL Injection",
+      :line => 16,
+      :message => /^Possible\ SQL\ injection/,
+      :confidence => 0,
+      :relative_path => "app/controllers/friendly_controller.rb",
+      :user_input => s(:call, s(:params), :[], s(:lit, :query))
+  end
 end

test/tests/sexp.rb

@@ -76,6 +76,16 @@ class SexpTests < Test::Unit::TestCase
     assert_equal s(:lit, 2), exp.last_arg
   end
 
+  def test_method_call_set_method
+    exp = parse "x.y"
+
+    assert_equal :y, exp.method
+    
+    exp.method = :z
+
+    assert_equal :z, exp.method
+  end
+
   def test_method_call_with_block
     exp = parse "x do |z|; blah z; end"
     block = exp.block