Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP
Browse files

minor changes to mass assignment security patch to bring it in line w…

…ith rails standards

Signed-off-by: José Valim <jose.valim@gmail.com>
  • Loading branch information...
commit 7c86e8e21ba6a1f88226ddd0cf012a563f234d06 1 parent 606088d
@joshk joshk authored josevalim committed
View
4 activerecord/lib/active_record/base.rb
@@ -1480,7 +1480,7 @@ def attributes=(new_attributes, guard_protected_attributes = true)
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 @@ def object_from_yaml(string)
include AttributeMethods::PrimaryKey
include AttributeMethods::TimeZoneConversion
include AttributeMethods::Dirty
- extend MassAssignmentSecurity
+ include MassAssignmentSecurity
include Callbacks, ActiveModel::Observing, Timestamp
include Associations, AssociationPreload, NamedScope
View
198 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 MassAssignmentSecurity
# on their role:
#
# class AccountsController < ApplicationController
- # extend ActiveRecord::MassAssignmentSecurity
+ # include ActiveRecord::MassAssignmentSecurity
#
# attr_accessible :first_name, :last_name
#
@@ -28,7 +37,7 @@ module MassAssignmentSecurity
# 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 MassAssignmentSecurity
#
# 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
View
7 activerecord/lib/active_record/mass_assignment_security/permission_set.rb
@@ -2,11 +2,11 @@
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 @@ def include?(key)
def remove_multiparameter_id(key)
key.gsub(/\(.+/, '')
end
-
end
class WhiteList < PermissionSet
@@ -28,7 +27,6 @@ class WhiteList < PermissionSet
def deny?(key)
!include?(key)
end
-
end
class BlackList < PermissionSet
@@ -37,7 +35,6 @@ class BlackList < PermissionSet
def deny?(key)
include?(key)
end
-
end
end
View
8 activerecord/lib/active_record/mass_assignment_security/sanitizer.rb
@@ -13,15 +13,17 @@ def sanitize(attributes)
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
View
126 activerecord/test/cases/base_test.rb
@@ -17,6 +17,7 @@
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 @@ def test_update_attributes!
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 @@ def test_multiparameter_attributes_on_time_with_empty_seconds
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")
View
12 activerecord/test/cases/mass_assignment_security/permission_set_test.rb
@@ -8,23 +8,23 @@ def setup
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
View
96 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
View
32 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
Please sign in to comment.
Something went wrong with that request. Please try again.