Merge pull request #390 from presidentbeef/tighten_dangerous_attributes

Remove fuzzy matching for attr_accessible attributes

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
   ]
@@ -29,7 +29,7 @@ class Brakeman::CheckModelAttrAccessible < Brakeman::BaseCheck
               :file => model[:file],
               :warning_type => "Mass Assignment",
               :warning_code => :dangerous_attr_accessible,
-              :message => "Potentially dangerous attribute #{attribute} available for mass assignment.",
+              :message => "Potentially dangerous attribute '#{attribute}' available for mass assignment",
               :confidence => confidence
             break # Prevent from matching single attr multiple times
           end

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

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,

test/tests/rails32.rb

@@ -217,7 +217,7 @@ class Rails32Tests < Test::Unit::TestCase
     assert_warning :type => :model,
       :warning_code => 60,
       :warning_type => "Mass Assignment",
-      :message => /^Potentially\ dangerous\ attribute\ admin/,
+      :message => /^Potentially\ dangerous\ attribute\ 'admin'/,
       :confidence => 0, #HIGH
       :file => /user\.rb/
   end 
@@ -227,7 +227,7 @@ class Rails32Tests < Test::Unit::TestCase
       :warning_code => 60,
       :fingerprint => "1d6615676c39afae6d749891e45d7351423542b3fe71a6eaf088bf7573e5c4b0",
       :warning_type => "Mass Assignment",
-      :message => /^Potentially\ dangerous\ attribute\ account_/,
+      :message => /^Potentially\ dangerous\ attribute\ 'account_id'/,
       :confidence => 0,
       :relative_path => "app/models/user.rb"
   end 
@@ -236,7 +236,7 @@ class Rails32Tests < Test::Unit::TestCase
     assert_warning :type => :model,
       :warning_code => 60,
       :warning_type => "Mass Assignment",
-      :message => /^Potentially\ dangerous\ attribute\ banned/,
+      :message => /^Potentially\ dangerous\ attribute\ 'banned'/,
       :confidence => 1, #MED
       :file => /account\.rb/
   end 
@@ -245,7 +245,7 @@ class Rails32Tests < Test::Unit::TestCase
     assert_warning :type => :model,
       :warning_code => 60,
       :warning_type => "Mass Assignment",
-      :message => /^Potentially\ dangerous\ attribute\ status_id/,
+      :message => /^Potentially\ dangerous\ attribute\ 'status_id'/,
       :confidence => 2, #LOW
       :file => /user\.rb/
   end 
@@ -253,7 +253,7 @@ class Rails32Tests < Test::Unit::TestCase
   def test_model_attr_accessible_plan_id
     assert_warning :type => :model,
       :warning_type => "Mass Assignment",
-      :message => /^Potentially\ dangerous\ attribute\ plan_id/,
+      :message => /^Potentially\ dangerous\ attribute\ 'plan_id'/,
       :confidence => 2, 
       :file => /account\.rb/
   end