diff --git a/lib/brakeman/checks/check_sql.rb b/lib/brakeman/checks/check_sql.rb index 4c36324748756d450b10b54b7f77c5a9e5fe24e9..b27c6c63aa77ca1ddee94d46f4d839d161d5e651 100644 --- a/lib/brakeman/checks/check_sql.rb +++ b/lib/brakeman/checks/check_sql.rb @@ -551,7 +551,8 @@ class Brakeman::CheckSQL < Brakeman::BaseCheck safe_value? exp.target else IGNORE_METHODS_IN_SQL.include? exp.method or - quote_call? exp + quote_call? exp or + exp.method.to_s.end_with? "_id" end when :if safe_value? exp.then_clause and safe_value? exp.else_clause diff --git a/test/apps/rails3.1/app/models/user.rb b/test/apps/rails3.1/app/models/user.rb index d7d22402982e4500787f9e4c5765fd179f9f0f2d..08807d4c82266f1fb6c26d7cf98d7019934ba19f 100644 --- a/test/apps/rails3.1/app/models/user.rb +++ b/test/apps/rails3.1/app/models/user.rb @@ -28,9 +28,10 @@ class User < ActiveRecord::Base attr_accessible :admin, :as => :admin - def self.sql_stuff + def self.sql_stuff parent_id condition = parent_id.blank? ? " IS NULL" : " = #{parent_id}" self.connection.select_values("SELECT max(id) FROM content_pages WHERE parent_content_page_id #{condition}")[0].to_i + self.connection.select_values("SELECT max(id) FROM content_pages WHERE child_content_page_id #{child_id}")[0].to_i # Should not warn User.where("#{table_name}.visibility = ?" + diff --git a/test/tests/rails31.rb b/test/tests/rails31.rb index 8af7ad8ecc9f70bc2bbf92049d88f84432e3e92b..86e3ba41d113e3a0faa25b89f2e9e4b920466eca 100644 --- a/test/tests/rails31.rb +++ b/test/tests/rails31.rb @@ -958,13 +958,25 @@ class Rails31Tests < Test::Unit::TestCase def test_sql_injection_with_interpolated_value assert_warning :type => :warning, :warning_code => 0, - :fingerprint => "37dc35cac607340b6244a25f7001fb6a67bf830b8c7395e4034f94c60f3d310e", + :fingerprint => "fd5cc1e0538e8a08b47e85cb7a9a699358908d8049daaaa5609539aa8aa03278", :warning_type => "SQL Injection", :line => 33, :message => /^Possible\ SQL\ injection/, :confidence => 1, :relative_path => "app/models/user.rb", - :user_input => s(:call, nil, :parent_id) + :user_input => s(:lvar, :parent_id) + end + + def test_sql_injection_with_id_call + assert_no_warning :type => :warning, + :warning_code => 0, + :fingerprint => "b9ade31073676589cf3b6a88de30105f67cc8170e87f2c2fd1c972f50ad2a3b3", + :warning_type => "SQL Injection", + :line => 34, + :message => /^Possible\ SQL\ injection/, + :confidence => 1, + :relative_path => "app/models/user.rb", + :user_input => s(:call, nil, :child_id) end def test_validates_format