Mass assignment security refactoring

Signed-off-by: José Valim <jose.valim@gmail.com>

5 files changed, 135 insertions, 13 deletions

diff --git a/activerecord/test/cases/base_test.rb b/activerecord/test/cases/base_test.rb
index ba7db838ca..dff39cf54f 100644
--- a/activerecord/test/cases/base_test.rb
+++ b/activerecord/test/cases/base_test.rb
@@ -71,9 +71,8 @@ class Task < ActiveRecord::Base
   attr_protected :starting
 end
 
-class TopicWithProtectedContentAndAccessibleAuthorName < ActiveRecord::Base
+class TopicWithProtectedContent < ActiveRecord::Base
   self.table_name = 'topics'
-  attr_accessible :author_name
   attr_protected  :content
 end
 
@@ -956,9 +955,9 @@ class BasicsTest < ActiveRecord::TestCase
   end
 
   def test_mass_assignment_should_raise_exception_if_accessible_and_protected_attribute_writers_are_both_used
-    topic = TopicWithProtectedContentAndAccessibleAuthorName.new
-    assert_raise(RuntimeError) { topic.attributes = { "author_name" => "me" } }
-    assert_raise(RuntimeError) { topic.attributes = { "content" => "stuff" } }
+    assert_raise(RuntimeError) do
+      TopicWithProtectedContent.attr_accessible :author_name
+    end
   end
 
   def test_mass_assignment_protection
@@ -1021,19 +1020,20 @@ class BasicsTest < ActiveRecord::TestCase
   end
 
   def test_mass_assignment_protection_inheritance
-    assert_nil LoosePerson.accessible_attributes
-    assert_equal Set.new([ 'credit_rating', 'administrator' ]), LoosePerson.protected_attributes
+    assert LoosePerson.accessible_attributes.blank?
+    assert_equal Set.new([ 'credit_rating', 'administrator', *LoosePerson.attributes_protected_by_default ]), LoosePerson.protected_attributes
 
-    assert_nil LooseDescendant.accessible_attributes
-    assert_equal Set.new([ 'credit_rating', 'administrator', 'phone_number' ]), LooseDescendant.protected_attributes
+    assert LooseDescendant.accessible_attributes.blank?
+    assert_equal Set.new([ 'credit_rating', 'administrator', 'phone_number', *LoosePerson.attributes_protected_by_default ]), LooseDescendant.protected_attributes
 
-    assert_nil LooseDescendantSecond.accessible_attributes
-    assert_equal Set.new([ 'credit_rating', 'administrator', 'phone_number', 'name' ]), LooseDescendantSecond.protected_attributes, 'Running attr_protected twice in one class should merge the protections'
+    assert LooseDescendantSecond.accessible_attributes.blank?
+    assert_equal Set.new([ 'credit_rating', 'administrator', 'phone_number', 'name', *LoosePerson.attributes_protected_by_default ]), LooseDescendantSecond.protected_attributes,
+      'Running attr_protected twice in one class should merge the protections'
 
-    assert_nil TightPerson.protected_attributes
+    assert (TightPerson.protected_attributes - TightPerson.attributes_protected_by_default).blank?
     assert_equal Set.new([ 'name', 'address' ]), TightPerson.accessible_attributes
 
-    assert_nil TightDescendant.protected_attributes
+    assert (TightDescendant.protected_attributes - TightDescendant.attributes_protected_by_default).blank?
     assert_equal Set.new([ 'name', 'address', 'phone_number' ]), TightDescendant.accessible_attributes
   end
 
diff --git a/activerecord/test/cases/mass_assignment_security/black_list_test.rb b/activerecord/test/cases/mass_assignment_security/black_list_test.rb
new file mode 100644
index 0000000000..8b7f48a5f6
--- /dev/null
+++ b/activerecord/test/cases/mass_assignment_security/black_list_test.rb
@@ -0,0 +1,28 @@
+require "cases/helper"
+
+class BlackListTest < ActiveRecord::TestCase
+
+  def setup
+    @black_list   = ActiveRecord::MassAssignmentSecurity::BlackList.new
+    @included_key = 'admin'
+    @black_list  += [ @included_key ]
+  end
+
+  test "deny? is true for included items" do
+    assert_equal true, @black_list.deny?(@included_key)
+  end
+
+  test "deny? is false for non-included items" do
+    assert_equal false, @black_list.deny?('first_name')
+  end
+
+  test "sanitize attributes" do
+    original_attributes = { 'first_name' => 'allowed', 'admin' => 'denied', 'admin(1)' => 'denied' }
+    attributes = @black_list.sanitize(original_attributes)
+
+    assert attributes.key?('first_name'), "Allowed key shouldn't be rejected"
+    assert !attributes.key?('admin'),     "Denied key should be rejected"
+    assert !attributes.key?('admin(1)'),  "Multi-parameter key should be detected"
+  end
+
+end
diff --git a/activerecord/test/cases/mass_assignment_security/permission_set_test.rb b/activerecord/test/cases/mass_assignment_security/permission_set_test.rb
new file mode 100644
index 0000000000..de7f3982a2
--- /dev/null
+++ b/activerecord/test/cases/mass_assignment_security/permission_set_test.rb
@@ -0,0 +1,30 @@
+require "cases/helper"
+
+class PermissionSetTest < ActiveRecord::TestCase
+
+  def setup
+    @permission_list = ActiveRecord::MassAssignmentSecurity::PermissionSet.new
+  end
+
+  test "+ stringifies added collection values" do
+    symbol_collection = [ :admin ]
+    @permission_list += symbol_collection
+
+    assert @permission_list.include?('admin'), "did not add collection to #{@permission_list.inspect}}"
+  end
+
+  test "include? normalizes multi-parameter keys" do
+    multi_param_key = 'admin(1)'
+    @permission_list += [ 'admin' ]
+
+    assert_equal true, @permission_list.include?(multi_param_key), "#{multi_param_key} not found in #{@permission_list.inspect}"
+  end
+
+  test "include? normal keys" do
+    normal_key = 'admin'
+    @permission_list +=  [ normal_key ]
+
+    assert_equal true,  @permission_list.include?(normal_key), "#{normal_key} not found in #{@permission_list.inspect}"
+  end
+
+end
diff --git a/activerecord/test/cases/mass_assignment_security/sanitizer_test.rb b/activerecord/test/cases/mass_assignment_security/sanitizer_test.rb
new file mode 100644
index 0000000000..122bc7e114
--- /dev/null
+++ b/activerecord/test/cases/mass_assignment_security/sanitizer_test.rb
@@ -0,0 +1,36 @@
+require "cases/helper"
+
+class SanitizerTest < ActiveRecord::TestCase
+
+  class SanitizingAuthorizer
+    include ActiveRecord::MassAssignmentSecurity::Sanitizer
+
+    attr_accessor :logger
+
+    def deny?(key)
+       [ 'admin' ].include?(key)
+    end
+
+  end
+
+  def setup
+    @sanitizer = SanitizingAuthorizer.new
+  end
+
+  test "sanitize attributes" do
+    original_attributes = { 'first_name' => 'allowed', 'admin' => 'denied' }
+    attributes = @sanitizer.sanitize(original_attributes)
+
+    assert attributes.key?('first_name'), "Allowed key shouldn't be rejected"
+    assert !attributes.key?('admin'),     "Denied key should be rejected"
+  end
+
+  test "debug mass assignment removal" do
+    original_attributes = { 'first_name' => 'allowed', 'admin' => 'denied' }
+    log = StringIO.new
+    @sanitizer.logger = Logger.new(log)
+    @sanitizer.sanitize(original_attributes)
+    assert (log.string =~ /admin/), "Should log removed attributes: #{log.string}"
+  end
+
+end
diff --git a/activerecord/test/cases/mass_assignment_security/white_list_test.rb b/activerecord/test/cases/mass_assignment_security/white_list_test.rb
new file mode 100644
index 0000000000..4601263437
--- /dev/null
+++ b/activerecord/test/cases/mass_assignment_security/white_list_test.rb
@@ -0,0 +1,28 @@
+require "cases/helper"
+
+class WhiteListTest < ActiveRecord::TestCase
+
+  def setup
+    @white_list   = ActiveRecord::MassAssignmentSecurity::WhiteList.new
+    @included_key = 'first_name'
+    @white_list  += [ @included_key ]
+  end
+
+  test "deny? is false for included items" do
+    assert_equal false, @white_list.deny?(@included_key)
+  end
+
+  test "deny? is true for non-included items" do
+    assert_equal true, @white_list.deny?('admin')
+  end
+
+  test "sanitize attributes" do
+    original_attributes = { 'first_name' => 'allowed', 'admin' => 'denied', 'admin(1)' => 'denied' }
+    attributes = @white_list.sanitize(original_attributes)
+
+    assert attributes.key?('first_name'), "Allowed key shouldn't be rejected"
+    assert !attributes.key?('admin'),     "Denied key should be rejected"
+    assert !attributes.key?('admin(1)'),  "Multi-parameter key should be detected"
+  end
+
+end