diff options
8 files changed, 234 insertions, 249 deletions
diff --git a/activerecord/lib/active_record/base.rb b/activerecord/lib/active_record/base.rb index affb1fee3c..56b4ba8260 100644 --- a/activerecord/lib/active_record/base.rb +++ b/activerecord/lib/active_record/base.rb @@ -1480,7 +1480,7 @@ MSG attributes = new_attributes.stringify_keys multi_parameter_attributes = [] - attributes = remove_attributes_protected_from_mass_assignment(attributes) if guard_protected_attributes + attributes = sanitize_for_mass_assignment(attributes) if guard_protected_attributes attributes.each do |k, v| if k.include?("(") @@ -1797,7 +1797,7 @@ MSG include AttributeMethods::PrimaryKey include AttributeMethods::TimeZoneConversion include AttributeMethods::Dirty - extend MassAssignmentSecurity + include MassAssignmentSecurity include Callbacks, ActiveModel::Observing, Timestamp include Associations, AssociationPreload, NamedScope diff --git a/activerecord/lib/active_record/mass_assignment_security.rb b/activerecord/lib/active_record/mass_assignment_security.rb index 07beb6405e..8f4d6e1c74 100644 --- a/activerecord/lib/active_record/mass_assignment_security.rb +++ b/activerecord/lib/active_record/mass_assignment_security.rb @@ -1,7 +1,16 @@ require 'active_record/mass_assignment_security/permission_set' module ActiveRecord + # = Active Record Mass-Assignment Security module MassAssignmentSecurity + extend ActiveSupport::Concern + + included do + class_attribute :_accessible_attributes + class_attribute :_protected_attributes + class_attribute :_active_authorizer + end + # Mass assignment security provides an interface for protecting attributes # from end-user assignment. For more complex permissions, mass assignment security # may be handled outside the model by extending a non-ActiveRecord class, @@ -11,7 +20,7 @@ module ActiveRecord # on their role: # # class AccountsController < ApplicationController - # extend ActiveRecord::MassAssignmentSecurity + # include ActiveRecord::MassAssignmentSecurity # # attr_accessible :first_name, :last_name # @@ -28,7 +37,7 @@ module ActiveRecord # protected # # def account_params - # remove_attributes_protected_from_mass_assignment(params[:account]) + # sanitize_for_mass_assignment(params[:account]) # end # # def mass_assignment_authorizer @@ -37,123 +46,96 @@ module ActiveRecord # # end # - def self.extended(base) - base.send(:include, InstanceMethods) - end - - module InstanceMethods - - protected - - def remove_attributes_protected_from_mass_assignment(attributes) - mass_assignment_authorizer.sanitize(attributes) - end - - def mass_assignment_authorizer - self.class.mass_assignment_authorizer - end + module ClassMethods + # Attributes named in this macro are protected from mass-assignment, + # such as <tt>new(attributes)</tt>, + # <tt>update_attributes(attributes)</tt>, or + # <tt>attributes=(attributes)</tt>. + # + # Mass-assignment to these attributes will simply be ignored, to assign + # to them you can use direct writer methods. This is meant to protect + # sensitive attributes from being overwritten by malicious users + # tampering with URLs or forms. + # + # class Customer < ActiveRecord::Base + # attr_protected :credit_rating + # end + # + # customer = Customer.new("name" => David, "credit_rating" => "Excellent") + # customer.credit_rating # => nil + # customer.attributes = { "description" => "Jolly fellow", "credit_rating" => "Superb" } + # customer.credit_rating # => nil + # + # customer.credit_rating = "Average" + # customer.credit_rating # => "Average" + # + # To start from an all-closed default and enable attributes as needed, + # have a look at +attr_accessible+. + # + # Note that using <tt>Hash#except</tt> or <tt>Hash#slice</tt> in place of +attr_protected+ + # to sanitize attributes won't provide sufficient protection. + def attr_protected(*names) + self._protected_attributes = self.protected_attributes + names + self._active_authorizer = self._protected_attributes + end - end + # Specifies a white list of model attributes that can be set via + # mass-assignment, such as <tt>new(attributes)</tt>, + # <tt>update_attributes(attributes)</tt>, or + # <tt>attributes=(attributes)</tt> + # + # This is the opposite of the +attr_protected+ macro: Mass-assignment + # will only set attributes in this list, to assign to the rest of + # attributes you can use direct writer methods. This is meant to protect + # sensitive attributes from being overwritten by malicious users + # tampering with URLs or forms. If you'd rather start from an all-open + # default and restrict attributes as needed, have a look at + # +attr_protected+. + # + # class Customer < ActiveRecord::Base + # attr_accessible :name, :nickname + # end + # + # customer = Customer.new(:name => "David", :nickname => "Dave", :credit_rating => "Excellent") + # customer.credit_rating # => nil + # customer.attributes = { :name => "Jolly fellow", :credit_rating => "Superb" } + # customer.credit_rating # => nil + # + # customer.credit_rating = "Average" + # customer.credit_rating # => "Average" + # + # Note that using <tt>Hash#except</tt> or <tt>Hash#slice</tt> in place of +attr_accessible+ + # to sanitize attributes won't provide sufficient protection. + def attr_accessible(*names) + self._accessible_attributes = self.accessible_attributes + names + self._active_authorizer = self._accessible_attributes + end - # Attributes named in this macro are protected from mass-assignment, - # such as <tt>new(attributes)</tt>, - # <tt>update_attributes(attributes)</tt>, or - # <tt>attributes=(attributes)</tt>. - # - # Mass-assignment to these attributes will simply be ignored, to assign - # to them you can use direct writer methods. This is meant to protect - # sensitive attributes from being overwritten by malicious users - # tampering with URLs or forms. - # - # class Customer < ActiveRecord::Base - # attr_protected :credit_rating - # end - # - # customer = Customer.new("name" => David, "credit_rating" => "Excellent") - # customer.credit_rating # => nil - # customer.attributes = { "description" => "Jolly fellow", "credit_rating" => "Superb" } - # customer.credit_rating # => nil - # - # customer.credit_rating = "Average" - # customer.credit_rating # => "Average" - # - # To start from an all-closed default and enable attributes as needed, - # have a look at +attr_accessible+. - # - # Note that using <tt>Hash#except</tt> or <tt>Hash#slice</tt> in place of +attr_protected+ - # to sanitize attributes won't provide sufficient protection. - def attr_protected(*keys) - use_authorizer(:protected_attributes) - protected_attributes.merge(keys) - end + def protected_attributes + self._protected_attributes ||= BlackList.new(attributes_protected_by_default).tap { |w| w.logger = logger } + end - # Specifies a white list of model attributes that can be set via - # mass-assignment, such as <tt>new(attributes)</tt>, - # <tt>update_attributes(attributes)</tt>, or - # <tt>attributes=(attributes)</tt> - # - # This is the opposite of the +attr_protected+ macro: Mass-assignment - # will only set attributes in this list, to assign to the rest of - # attributes you can use direct writer methods. This is meant to protect - # sensitive attributes from being overwritten by malicious users - # tampering with URLs or forms. If you'd rather start from an all-open - # default and restrict attributes as needed, have a look at - # +attr_protected+. - # - # class Customer < ActiveRecord::Base - # attr_accessible :name, :nickname - # end - # - # customer = Customer.new(:name => "David", :nickname => "Dave", :credit_rating => "Excellent") - # customer.credit_rating # => nil - # customer.attributes = { :name => "Jolly fellow", :credit_rating => "Superb" } - # customer.credit_rating # => nil - # - # customer.credit_rating = "Average" - # customer.credit_rating # => "Average" - # - # Note that using <tt>Hash#except</tt> or <tt>Hash#slice</tt> in place of +attr_accessible+ - # to sanitize attributes won't provide sufficient protection. - def attr_accessible(*keys) - use_authorizer(:accessible_attributes) - accessible_attributes.merge(keys) - end + def accessible_attributes + self._accessible_attributes ||= WhiteList.new.tap { |w| w.logger = logger } + end - # Returns an array of all the attributes that have been protected from mass-assignment. - def protected_attributes - read_inheritable_attribute(:protected_attributes) || begin - authorizer = BlackList.new - authorizer += attributes_protected_by_default - authorizer.logger = logger - write_inheritable_attribute(:protected_attributes, authorizer) + def active_authorizer + self._active_authorizer ||= protected_attributes end - end - # Returns an array of all the attributes that have been made accessible to mass-assignment. - def accessible_attributes - read_inheritable_attribute(:accessible_attributes) || begin - authorizer = WhiteList.new - authorizer.logger = logger - write_inheritable_attribute(:accessible_attributes, authorizer) + def attributes_protected_by_default + [] end end - def mass_assignment_authorizer - protected_attributes - end + protected - private + def sanitize_for_mass_assignment(attributes) + mass_assignment_authorizer.sanitize(attributes) + end - # Sets the active authorizer, (attr_protected or attr_accessible). Subsequent calls - # will raise an exception when using a different authorizer_id. - def use_authorizer(authorizer_id) # :nodoc: - if active_authorizer_id = read_inheritable_attribute(:active_authorizer_id) - unless authorizer_id == active_authorizer_id - raise("Already using #{active_authorizer_id}, cannot use #{authorizer_id}") - end - else - write_inheritable_attribute(:active_authorizer_id, authorizer_id) - end + def mass_assignment_authorizer + self.class.active_authorizer end end diff --git a/activerecord/lib/active_record/mass_assignment_security/permission_set.rb b/activerecord/lib/active_record/mass_assignment_security/permission_set.rb index 1d34dce02e..8446a4103b 100644 --- a/activerecord/lib/active_record/mass_assignment_security/permission_set.rb +++ b/activerecord/lib/active_record/mass_assignment_security/permission_set.rb @@ -2,11 +2,11 @@ require 'active_record/mass_assignment_security/sanitizer' module ActiveRecord module MassAssignmentSecurity - class PermissionSet < Set + class PermissionSet < Set attr_accessor :logger - def merge(values) + def +(values) super(values.map(&:to_s)) end @@ -19,7 +19,6 @@ module ActiveRecord def remove_multiparameter_id(key) key.gsub(/\(.+/, '') end - end class WhiteList < PermissionSet @@ -28,7 +27,6 @@ module ActiveRecord def deny?(key) !include?(key) end - end class BlackList < PermissionSet @@ -37,7 +35,6 @@ module ActiveRecord def deny?(key) include?(key) end - end end diff --git a/activerecord/lib/active_record/mass_assignment_security/sanitizer.rb b/activerecord/lib/active_record/mass_assignment_security/sanitizer.rb index 4a099a147c..11de35f9d6 100644 --- a/activerecord/lib/active_record/mass_assignment_security/sanitizer.rb +++ b/activerecord/lib/active_record/mass_assignment_security/sanitizer.rb @@ -13,15 +13,17 @@ module ActiveRecord def debug_protected_attribute_removal(attributes, sanitized_attributes) removed_keys = attributes.keys - sanitized_attributes.keys - if removed_keys.any? - logger.debug "WARNING: Can't mass-assign protected attributes: #{removed_keys.join(', ')}" - end + warn!(removed_keys) if removed_keys.any? end def debug? logger.present? end + def warn!(attrs) + logger.debug "WARNING: Can't mass-assign protected attributes: #{attrs.join(', ')}" + end + end end end diff --git a/activerecord/test/cases/base_test.rb b/activerecord/test/cases/base_test.rb index dff39cf54f..2eecb6e344 100644 --- a/activerecord/test/cases/base_test.rb +++ b/activerecord/test/cases/base_test.rb @@ -17,6 +17,7 @@ require 'models/comment' require 'models/minimalistic' require 'models/warehouse_thing' require 'models/parrot' +require 'models/mass_assignment_specific' require 'rexml/document' require 'active_support/core_ext/exception' @@ -37,45 +38,12 @@ class Computer < ActiveRecord::Base; end class NonExistentTable < ActiveRecord::Base; end class TestOracleDefault < ActiveRecord::Base; end -class LoosePerson < ActiveRecord::Base - self.table_name = 'people' - self.abstract_class = true - attr_protected :credit_rating, :administrator -end - -class LooseDescendant < LoosePerson - attr_protected :phone_number -end - -class LooseDescendantSecond< LoosePerson - attr_protected :phone_number - attr_protected :name -end - -class TightPerson < ActiveRecord::Base - self.table_name = 'people' - attr_accessible :name, :address -end - -class TightDescendant < TightPerson - attr_accessible :phone_number -end - class ReadonlyTitlePost < Post attr_readonly :title end class Booleantest < ActiveRecord::Base; end -class Task < ActiveRecord::Base - attr_protected :starting -end - -class TopicWithProtectedContent < ActiveRecord::Base - self.table_name = 'topics' - attr_protected :content -end - class BasicsTest < ActiveRecord::TestCase fixtures :topics, :companies, :developers, :projects, :computers, :accounts, :minimalistics, 'warehouse-things', :authors, :categorizations, :categories, :posts @@ -954,89 +922,6 @@ class BasicsTest < ActiveRecord::TestCase Reply.reset_callbacks(:validate) end - def test_mass_assignment_should_raise_exception_if_accessible_and_protected_attribute_writers_are_both_used - assert_raise(RuntimeError) do - TopicWithProtectedContent.attr_accessible :author_name - end - end - - def test_mass_assignment_protection - firm = Firm.new - firm.attributes = { "name" => "Next Angle", "rating" => 5 } - assert_equal 1, firm.rating - end - - def test_mass_assignment_protection_against_class_attribute_writers - [:logger, :configurations, :primary_key_prefix_type, :table_name_prefix, :table_name_suffix, :pluralize_table_names, - :default_timezone, :schema_format, :lock_optimistically, :record_timestamps].each do |method| - assert_respond_to Task, method - assert_respond_to Task, "#{method}=" - assert_respond_to Task.new, method - assert !Task.new.respond_to?("#{method}=") - end - end - - def test_customized_primary_key_remains_protected - subscriber = Subscriber.new(:nick => 'webster123', :name => 'nice try') - assert_nil subscriber.id - - keyboard = Keyboard.new(:key_number => 9, :name => 'nice try') - assert_nil keyboard.id - end - - def test_customized_primary_key_remains_protected_when_referred_to_as_id - subscriber = Subscriber.new(:id => 'webster123', :name => 'nice try') - assert_nil subscriber.id - - keyboard = Keyboard.new(:id => 9, :name => 'nice try') - assert_nil keyboard.id - end - - def test_mass_assigning_invalid_attribute - firm = Firm.new - - assert_raise(ActiveRecord::UnknownAttributeError) do - firm.attributes = { "id" => 5, "type" => "Client", "i_dont_even_exist" => 20 } - end - end - - def test_mass_assignment_protection_on_defaults - firm = Firm.new - firm.attributes = { "id" => 5, "type" => "Client" } - assert_nil firm.id - assert_equal "Firm", firm[:type] - end - - def test_mass_assignment_accessible - reply = Reply.new("title" => "hello", "content" => "world", "approved" => true) - reply.save - - assert reply.approved? - - reply.approved = false - reply.save - - assert !reply.approved? - end - - def test_mass_assignment_protection_inheritance - assert LoosePerson.accessible_attributes.blank? - assert_equal Set.new([ 'credit_rating', 'administrator', *LoosePerson.attributes_protected_by_default ]), LoosePerson.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 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 (TightPerson.protected_attributes - TightPerson.attributes_protected_by_default).blank? - assert_equal Set.new([ 'name', 'address' ]), TightPerson.accessible_attributes - - assert (TightDescendant.protected_attributes - TightDescendant.attributes_protected_by_default).blank? - assert_equal Set.new([ 'name', 'address', 'phone_number' ]), TightDescendant.accessible_attributes - end - def test_readonly_attributes assert_equal Set.new([ 'title' , 'comments_count' ]), ReadonlyTitlePost.readonly_attributes @@ -1239,15 +1124,6 @@ class BasicsTest < ActiveRecord::TestCase assert_equal Time.local(2004, 6, 24, 16, 24, 0), topic.written_on end - def test_multiparameter_mass_assignment_protector - task = Task.new - time = Time.mktime(2000, 1, 1, 1) - task.starting = time - attributes = { "starting(1i)" => "2004", "starting(2i)" => "6", "starting(3i)" => "24" } - task.attributes = attributes - assert_equal time, task.starting - end - def test_multiparameter_assignment_of_aggregation customer = Customer.new address = Address.new("The Street", "The City", "The Country") diff --git a/activerecord/test/cases/mass_assignment_security/permission_set_test.rb b/activerecord/test/cases/mass_assignment_security/permission_set_test.rb index de7f3982a2..ca8985042a 100644 --- a/activerecord/test/cases/mass_assignment_security/permission_set_test.rb +++ b/activerecord/test/cases/mass_assignment_security/permission_set_test.rb @@ -8,23 +8,23 @@ class PermissionSetTest < ActiveRecord::TestCase test "+ stringifies added collection values" do symbol_collection = [ :admin ] - @permission_list += symbol_collection + new_list = @permission_list += symbol_collection - assert @permission_list.include?('admin'), "did not add collection to #{@permission_list.inspect}}" + assert new_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' ] + new_list = @permission_list += [ 'admin' ] - assert_equal true, @permission_list.include?(multi_param_key), "#{multi_param_key} not found in #{@permission_list.inspect}" + assert new_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 ] + new_list = @permission_list += [ normal_key ] - assert_equal true, @permission_list.include?(normal_key), "#{normal_key} not found in #{@permission_list.inspect}" + assert new_list.include?(normal_key), "#{normal_key} not found in #{@permission_list.inspect}" end end diff --git a/activerecord/test/cases/mass_assignment_security_test.rb b/activerecord/test/cases/mass_assignment_security_test.rb new file mode 100644 index 0000000000..07154da93b --- /dev/null +++ b/activerecord/test/cases/mass_assignment_security_test.rb @@ -0,0 +1,96 @@ +require "cases/helper" +require 'models/reply' +require 'models/company' +require 'models/subscriber' +require 'models/keyboard' +require 'models/mass_assignment_specific' + +class MassAssignmentSecurityTest < ActiveRecord::TestCase + + def test_mass_assignment_protection + firm = Firm.new + firm.attributes = { "name" => "Next Angle", "rating" => 5 } + assert_equal 1, firm.rating + end + + def test_mass_assignment_protection_against_class_attribute_writers + [:logger, :configurations, :primary_key_prefix_type, :table_name_prefix, :table_name_suffix, :pluralize_table_names, + :default_timezone, :schema_format, :lock_optimistically, :record_timestamps].each do |method| + assert_respond_to Task, method + assert_respond_to Task, "#{method}=" + assert_respond_to Task.new, method + assert !Task.new.respond_to?("#{method}=") + end + end + + def test_customized_primary_key_remains_protected + subscriber = Subscriber.new(:nick => 'webster123', :name => 'nice try') + assert_nil subscriber.id + + keyboard = Keyboard.new(:key_number => 9, :name => 'nice try') + assert_nil keyboard.id + end + + def test_customized_primary_key_remains_protected_when_referred_to_as_id + subscriber = Subscriber.new(:id => 'webster123', :name => 'nice try') + assert_nil subscriber.id + + keyboard = Keyboard.new(:id => 9, :name => 'nice try') + assert_nil keyboard.id + end + + def test_mass_assigning_invalid_attribute + firm = Firm.new + + assert_raise(ActiveRecord::UnknownAttributeError) do + firm.attributes = { "id" => 5, "type" => "Client", "i_dont_even_exist" => 20 } + end + end + + def test_mass_assignment_protection_on_defaults + firm = Firm.new + firm.attributes = { "id" => 5, "type" => "Client" } + assert_nil firm.id + assert_equal "Firm", firm[:type] + end + + def test_mass_assignment_accessible + reply = Reply.new("title" => "hello", "content" => "world", "approved" => true) + reply.save + + assert reply.approved? + + reply.approved = false + reply.save + + assert !reply.approved? + end + + def test_mass_assignment_protection_inheritance + assert LoosePerson.accessible_attributes.blank? + assert_equal Set.new([ 'credit_rating', 'administrator', *LoosePerson.attributes_protected_by_default ]), LoosePerson.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 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 (TightPerson.protected_attributes - TightPerson.attributes_protected_by_default).blank? + assert_equal Set.new([ 'name', 'address' ]), TightPerson.accessible_attributes + + assert (TightDescendant.protected_attributes - TightDescendant.attributes_protected_by_default).blank? + assert_equal Set.new([ 'name', 'address', 'phone_number' ]), TightDescendant.accessible_attributes + end + + def test_mass_assignment_multiparameter_protector + task = Task.new + time = Time.mktime(2000, 1, 1, 1) + task.starting = time + attributes = { "starting(1i)" => "2004", "starting(2i)" => "6", "starting(3i)" => "24" } + task.attributes = attributes + assert_equal time, task.starting + end + +end
\ No newline at end of file diff --git a/activerecord/test/models/mass_assignment_specific.rb b/activerecord/test/models/mass_assignment_specific.rb new file mode 100644 index 0000000000..13a80e0197 --- /dev/null +++ b/activerecord/test/models/mass_assignment_specific.rb @@ -0,0 +1,32 @@ +class LoosePerson < ActiveRecord::Base + self.table_name = 'people' + self.abstract_class = true + attr_protected :credit_rating, :administrator +end + +class LooseDescendant < LoosePerson + attr_protected :phone_number +end + +class LooseDescendantSecond< LoosePerson + attr_protected :phone_number + attr_protected :name +end + +class TightPerson < ActiveRecord::Base + self.table_name = 'people' + attr_accessible :name, :address +end + +class TightDescendant < TightPerson + attr_accessible :phone_number +end + +class Task < ActiveRecord::Base + attr_protected :starting +end + +class TopicWithProtectedContent < ActiveRecord::Base + self.table_name = 'topics' + attr_protected :content +end
\ No newline at end of file |