diff --git a/lib/brakeman/checks/check_sql.rb b/lib/brakeman/checks/check_sql.rb index 1b207e9f675605e3196b0f98fc467b69e59dab3f..4ceabaa827474ad4fb0d60749ae457610aa49d1d 100644 --- a/lib/brakeman/checks/check_sql.rb +++ b/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 diff --git a/test/apps/rails2/app/models/user.rb b/test/apps/rails2/app/models/user.rb index fcca29a4cad02e55afce17ffed99c0f5aa5cabec..bb4c3c7d2e2b3f50b6f8d35fd8fe685abd15a3d9 100644 --- a/test/apps/rails2/app/models/user.rb +++ b/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 diff --git a/test/tests/rails2.rb b/test/tests/rails2.rb index e402caf1310bf0f7a98775c62161bbb4dc8a05c6..ee7888e1d0c8c9a9da468da905f2626971c18463 100644 --- a/test/tests/rails2.rb +++ b/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 diff --git a/test/tests/tabs_output.rb b/test/tests/tabs_output.rb index a74964b2f670325167e8f6af02c37fb7f3f5cf16..86ed80a4330849f7bdaf755a858ef06bc19465ab 100644 --- a/test/tests/tabs_output.rb +++ b/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