Merge pull request #439 from presidentbeef/active_record_base_connect_sqli

Also catch #connection calls on ActiveRecord::Base

lib/brakeman/checks/check_sql.rb

@@ -40,7 +40,7 @@ class Brakeman::CheckSQL < Brakeman::BaseCheck
     Brakeman.debug "Finding possible SQL calls using constantized()"
     calls.concat tracker.find_call(:methods => @sql_targets).select { |result| constantize_call? result }
 
-    connect_targets = active_record_models.keys << nil
+    connect_targets = active_record_models.keys + [nil, :"ActiveRecord::Base"]
     calls.concat tracker.find_call(:targets => connect_targets, :methods => @connection_calls, :chained => true).select { |result| connect_call? result }
 
     Brakeman.debug "Finding calls to named_scope or scope"
@@ -556,11 +556,13 @@ class Brakeman::CheckSQL < Brakeman::BaseCheck
 
     if call? target and target.method == :connection
       target = target.target
+      klass = class_name(target)
 
       target.nil? or
       target == SELF_CLASS or
       node_type? target, :self or
-      active_record_models.include? class_name(target)
+      klass == :"ActiveRecord::Base" or
+      active_record_models.include? klass
     end
   end
 

test/apps/rails2/app/models/user.rb

@@ -21,4 +21,13 @@ class User < ActiveRecord::Base
     User.find(:all, :conditions => self.merge_conditions(some_conditions))
     find(:all, :conditions => User.merge_conditions(some_conditions))
   end
+
+  def self.some_method(value)
+    results = ActiveRecord::Base.connection.execute(%Q(SELECT
+        id
+      FROM
+        table t
+      WHERE
+        t.something = '#{value}'))
+  end
 end

test/tests/rails2.rb

@@ -12,13 +12,13 @@ class Rails2Tests < Test::Unit::TestCase
         :controller => 1,
         :model => 3,
         :template => 45,
-        :generic => 48 }
+        :generic => 49 }
     else
       @expected ||= {
         :controller => 1,
         :model => 3,
         :template => 45,
-        :generic => 49 }
+        :generic => 50 }
     end
   end
 
@@ -611,6 +611,18 @@ class Rails2Tests < Test::Unit::TestCase
       :file => /user\.rb/,
       :relative_path => "app/models/user.rb"
   end
+ 
+  def test_sql_injection_active_record_base_connection
+    assert_warning :type => :warning,
+      :warning_code => 0,
+      :fingerprint => "37885d589fc5c41553dcc38b36b506c2e508d1f37ce040eb6dca92a958f858fb",
+      :warning_type => "SQL Injection",
+      :line => 26,
+      :message => /^Possible\ SQL\ injection/,
+      :confidence => 1,
+      :relative_path => "app/models/user.rb",
+      :user_input => s(:lvar, :value)
+  end
 
   def test_escape_once
     results = find :type => :template,
@@ -1252,13 +1264,13 @@ class Rails2WithOptionsTests < Test::Unit::TestCase
         :controller => 1,
         :model => 4,
         :template => 45,
-        :generic => 48 }
+        :generic => 49 }
     else
       @expected ||= {
         :controller => 1,
         :model => 4,
         :template => 45,
-        :generic => 49 }
+        :generic => 50 }
     end
   end
 

test/tests/tabs_output.rb

@@ -3,9 +3,9 @@ class TestTabsOutput < Test::Unit::TestCase
 
   def test_reported_warnings
     if Brakeman::Scanner::RUBY_1_9
-      assert_equal 98, Report.lines.to_a.count
-    else
       assert_equal 99, Report.lines.to_a.count
+    else
+      assert_equal 100, Report.lines.to_a.count
     end
   end
 end