Merge branch 'attr_accessible_fixes'

Conflicts:
	lib/brakeman/checks/check_model_attr_accessible.rb

lib/brakeman/checks/check_model_attr_accessible.rb

 require 'brakeman/checks/base_check'
 
-# Author: Paul Deardorff (themetric) 
-# Checks models to see if important foreign keys 
-# or attributes are exposed as attr_accessible when 
-# they probably shouldn't be. 
+# Author: Paul Deardorff (themetric)
+# Checks models to see if important foreign keys
+# or attributes are exposed as attr_accessible when
+# they probably shouldn't be.
 
 class Brakeman::CheckModelAttrAccessible < Brakeman::BaseCheck
   Brakeman::Checks.add self
 
   @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], 
-    :account_id => CONFIDENCE[:high], 
-    /\S*_id(s?)\z/ => CONFIDENCE[:low] # All other foreign keys have weak/low confidence 
-  }
+  SUSP_ATTRS = [
+    [/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
+  ]
 
   def run_check
     check_models do |name, model|
@@ -24,17 +24,17 @@ class Brakeman::CheckModelAttrAccessible < Brakeman::BaseCheck
         next if role_limited? model, attribute
 
         SUSP_ATTRS.each do |susp_attr, confidence|
-            if susp_attr.is_a?(Regexp) and susp_attr =~ attribute.to_s or susp_attr == attribute 
-              warn :model => name,    
-                :file => model[:file],                          
-                :warning_type => "Mass Assignment", 
-                :warning_code => :mass_assign_call,
-                :message => "Potentially dangerous attribute #{attribute} available for mass assignment.", 
-                :confidence => confidence 
-              break # Prevent from matching single attr multiple times
-            end 
-        end         
-      end 
+          if susp_attr.is_a?(Regexp) and susp_attr =~ attribute.to_s or susp_attr == attribute
+            warn :model => name,
+              :file => model[:file],
+              :warning_type => "Mass Assignment",
+              :warning_code => :dangerous_attr_accessible,
+              :message => "Potentially dangerous attribute #{attribute} available for mass assignment.",
+              :confidence => confidence
+            break # Prevent from matching single attr multiple times
+          end
+        end
+      end
     end
   end
 

lib/brakeman/warning_codes.rb

@@ -59,7 +59,8 @@ module Brakeman::WarningCodes
     :CVE_2013_1855 => 56,
     :CVE_2013_1856 => 57,
     :CVE_2013_1857 => 58,
-    :unsafe_symbol_creation => 59
+    :unsafe_symbol_creation => 59,
+    :dangerous_attr_accessible => 60
   }
 
   def self.code name

test/tests/rails32.rb

@@ -215,6 +215,7 @@ class Rails32Tests < Test::Unit::TestCase
 
   def test_model_attr_accessible_admin
     assert_warning :type => :model,
+      :warning_code => 60,
       :warning_type => "Mass Assignment",
       :message => /^Potentially\ dangerous\ attribute\ admin/,
       :confidence => 0, #HIGH
@@ -223,14 +224,17 @@ class Rails32Tests < Test::Unit::TestCase
 
   def test_model_attr_accessible_account_id
     assert_warning :type => :model,
+      :warning_code => 60,
+      :fingerprint => "1d6615676c39afae6d749891e45d7351423542b3fe71a6eaf088bf7573e5c4b0",
       :warning_type => "Mass Assignment",
-      :message => /^Potentially\ dangerous\ attribute\ account_id/,
-      :confidence => 0, 
-      :file => /user\.rb/
+      :message => /^Potentially\ dangerous\ attribute\ account_/,
+      :confidence => 0,
+      :relative_path => "app/models/user.rb"
   end 
 
   def test_model_attr_accessible_account_banned
     assert_warning :type => :model,
+      :warning_code => 60,
       :warning_type => "Mass Assignment",
       :message => /^Potentially\ dangerous\ attribute\ banned/,
       :confidence => 1, #MED
@@ -239,6 +243,7 @@ class Rails32Tests < Test::Unit::TestCase
 
   def test_model_attr_accessible_status_id
     assert_warning :type => :model,
+      :warning_code => 60,
       :warning_type => "Mass Assignment",
       :message => /^Potentially\ dangerous\ attribute\ status_id/,
       :confidence => 2, #LOW