From ae77652320abf4a365e0108ed16f0430e2b0bbff Mon Sep 17 00:00:00 2001 From: Justin Collins Date: Tue, 27 Aug 2013 13:35:52 -0700 Subject: [PATCH] Remove fuzzy matching for CheckModelAttrAccessible in practice, this ends up with annoying false positives --- lib/brakeman/checks/check_model_attr_accessible.rb | 6 +++--- test/apps/rails3.1/app/models/account.rb | 1 + test/tests/rails31.rb | 10 ++++++++++ 3 files changed, 14 insertions(+), 3 deletions(-) diff --git a/lib/brakeman/checks/check_model_attr_accessible.rb b/lib/brakeman/checks/check_model_attr_accessible.rb index 7825f704..d08ce78a 100644 --- a/lib/brakeman/checks/check_model_attr_accessible.rb +++ b/lib/brakeman/checks/check_model_attr_accessible.rb @@ -11,9 +11,9 @@ class Brakeman::CheckModelAttrAccessible < Brakeman::BaseCheck @description = "Reports models which have dangerous attributes defined under the attr_accessible whitelist." SUSP_ATTRS = [ - [/admin/, CONFIDENCE[:high]], # Very dangerous unless some Rails authorization used - [/role/, CONFIDENCE[:med]], - [/banned/, CONFIDENCE[:med]], + [:admin, CONFIDENCE[:high]], # Very dangerous unless some Rails authorization used + [:role, CONFIDENCE[:med]], + [:banned, CONFIDENCE[:med]], [:account_id, CONFIDENCE[:high]], [/\S*_id(s?)\z/, CONFIDENCE[:low]] # All other foreign keys have weak/low confidence ] diff --git a/test/apps/rails3.1/app/models/account.rb b/test/apps/rails3.1/app/models/account.rb index fcea012c..e180a17a 100644 --- a/test/apps/rails3.1/app/models/account.rb +++ b/test/apps/rails3.1/app/models/account.rb @@ -3,4 +3,5 @@ class Account < ActiveRecord::Base validates :phone, :format => { :with => /(\d{3})-(\d{3})-(\d{4})/, :on => :create }, :presence => true validates :first_name, :format => /\w+/ serialize :cc_info #safe from CVE-2013-0277 + attr_accessible :blah_admin_blah end diff --git a/test/tests/rails31.rb b/test/tests/rails31.rb index ca247f3b..528fc664 100644 --- a/test/tests/rails31.rb +++ b/test/tests/rails31.rb @@ -1044,6 +1044,16 @@ class Rails31Tests < Test::Unit::TestCase :relative_path => "app/models/user.rb" end + def test_attr_accessible_not_matching_regex + assert_no_warning :type => :model, + :warning_code => 60, + :fingerprint => "e933f99c33bece852891a466b5b0fc629d9f20ba80ff3bbc42adfd239d5a5b48", + :warning_type => "Mass Assignment", + :message => /^Potentially\ dangerous\ attribute\ blah_admin/, + :confidence => 0, + :relative_path => "app/models/account.rb" + end + def test_wrong_model_attributes_in_haml assert_no_warning :type => :template, :warning_code => 2, -- GitLab