From 606088df3f10dd8daec8ccc97d8279c119a503b5 Mon Sep 17 00:00:00 2001 From: Eric Chapweske Date: Fri, 29 Jan 2010 17:02:12 -0800 Subject: Mass assignment security refactoring MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: José Valim --- activerecord/test/cases/base_test.rb | 26 ++++++++-------- .../mass_assignment_security/black_list_test.rb | 28 +++++++++++++++++ .../permission_set_test.rb | 30 ++++++++++++++++++ .../mass_assignment_security/sanitizer_test.rb | 36 ++++++++++++++++++++++ .../mass_assignment_security/white_list_test.rb | 28 +++++++++++++++++ 5 files changed, 135 insertions(+), 13 deletions(-) create mode 100644 activerecord/test/cases/mass_assignment_security/black_list_test.rb create mode 100644 activerecord/test/cases/mass_assignment_security/permission_set_test.rb create mode 100644 activerecord/test/cases/mass_assignment_security/sanitizer_test.rb create mode 100644 activerecord/test/cases/mass_assignment_security/white_list_test.rb (limited to 'activerecord/test/cases') 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 -- cgit v1.2.3