Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP
Browse files

Mass assignment security refactoring

Signed-off-by: José Valim <jose.valim@gmail.com>
  • Loading branch information...
commit 606088df3f10dd8daec8ccc97d8279c119a503b5 1 parent 723a0bb
@eac eac authored josevalim committed
View
1  activerecord/lib/active_record.rb
@@ -64,6 +64,7 @@ module ActiveRecord
autoload :CounterCache
autoload :DynamicFinderMatch
autoload :DynamicScopeMatch
+ autoload :MassAssignmentSecurity
autoload :Migration
autoload :Migrator, 'active_record/migration'
autoload :NamedScope
View
144 activerecord/lib/active_record/base.rb
@@ -24,7 +24,7 @@
require 'active_record/log_subscriber'
module ActiveRecord #:nodoc:
- # = Active Record
+ # = Active Record
#
# Active Record objects don't specify their attributes directly, but rather infer them from the table definition with
# which they're linked. Adding, removing, and changing attributes and their type is done directly in the database. Any change
@@ -476,112 +476,16 @@ def count_by_sql(sql)
connection.select_value(sql, "#{name} Count").to_i
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+.
- #
- # If the access logic of your application is richer you can use <tt>Hash#except</tt>
- # or <tt>Hash#slice</tt> to sanitize the hash of parameters before they are
- # passed to Active Record.
- #
- # For example, it could be the case that the list of protected attributes
- # for a given model depends on the role of the user:
- #
- # # Assumes plan_id is not protected because it depends on the role.
- # params[:account] = params[:account].except(:plan_id) unless admin?
- # @account.update_attributes(params[:account])
- #
- # Note that +attr_protected+ is still applied to the received hash. Thus,
- # with this technique you can at most _extend_ the list of protected
- # attributes for a particular mass-assignment call.
- def attr_protected(*attributes)
- write_inheritable_attribute(:attr_protected, Set.new(attributes.map {|a| a.to_s}) + (protected_attributes || []))
- end
-
- # Returns an array of all the attributes that have been protected from mass-assignment.
- def protected_attributes # :nodoc:
- read_inheritable_attribute(:attr_protected)
- 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"
- #
- # If the access logic of your application is richer you can use <tt>Hash#except</tt>
- # or <tt>Hash#slice</tt> to sanitize the hash of parameters before they are
- # passed to Active Record.
- #
- # For example, it could be the case that the list of accessible attributes
- # for a given model depends on the role of the user:
- #
- # # Assumes plan_id is accessible because it depends on the role.
- # params[:account] = params[:account].except(:plan_id) unless admin?
- # @account.update_attributes(params[:account])
- #
- # Note that +attr_accessible+ is still applied to the received hash. Thus,
- # with this technique you can at most _narrow_ the list of accessible
- # attributes for a particular mass-assignment call.
- def attr_accessible(*attributes)
- write_inheritable_attribute(:attr_accessible, Set.new(attributes.map(&:to_s)) + (accessible_attributes || []))
+ # Attributes listed as readonly can be set for a new record, but will be ignored in database updates afterwards.
+ def attr_readonly(*attributes)
+ write_inheritable_attribute(:attr_readonly, Set.new(attributes.map(&:to_s)) + (readonly_attributes || []))
end
- # Returns an array of all the attributes that have been made accessible to mass-assignment.
- def accessible_attributes # :nodoc:
- read_inheritable_attribute(:attr_accessible)
+ # Returns an array of all the attributes that have been specified as readonly.
+ def readonly_attributes
+ read_inheritable_attribute(:attr_readonly) || []
end
- # Attributes listed as readonly can be set for a new record, but will be ignored in database updates afterwards.
- def attr_readonly(*attributes)
- write_inheritable_attribute(:attr_readonly, Set.new(attributes.map(&:to_s)) + (readonly_attributes || []))
- end
-
- # Returns an array of all the attributes that have been specified as readonly.
- def readonly_attributes
- read_inheritable_attribute(:attr_readonly) || []
- end
-
# If you have an attribute that needs to be saved to the database as an object, and retrieved as the same object,
# then specify the name of that attribute using this method and it will be handled automatically.
# The serialization is done through YAML. If +class_name+ is specified, the serialized object must be of that
@@ -1716,27 +1620,6 @@ def ensure_proper_type
end
end
- def remove_attributes_protected_from_mass_assignment(attributes)
- safe_attributes =
- if self.class.accessible_attributes.nil? && self.class.protected_attributes.nil?
- attributes.reject { |key, value| attributes_protected_by_default.include?(key.gsub(/\(.+/, "")) }
- elsif self.class.protected_attributes.nil?
- attributes.reject { |key, value| !self.class.accessible_attributes.include?(key.gsub(/\(.+/, "")) || attributes_protected_by_default.include?(key.gsub(/\(.+/, "")) }
- elsif self.class.accessible_attributes.nil?
- attributes.reject { |key, value| self.class.protected_attributes.include?(key.gsub(/\(.+/,"")) || attributes_protected_by_default.include?(key.gsub(/\(.+/, "")) }
- else
- raise "Declare either attr_protected or attr_accessible for #{self.class}, but not both."
- end
-
- removed_attributes = attributes.keys - safe_attributes.keys
-
- if removed_attributes.any?
- log_protected_attribute_removal(removed_attributes)
- end
-
- safe_attributes
- end
-
# Removes attributes which have been marked as readonly.
def remove_readonly_attributes(attributes)
unless self.class.readonly_attributes.nil?
@@ -1746,16 +1629,10 @@ def remove_readonly_attributes(attributes)
end
end
- def log_protected_attribute_removal(*attributes)
- if logger
- logger.debug "WARNING: Can't mass-assign these protected attributes: #{attributes.join(', ')}"
- end
- end
-
# The primary key and inheritance column can never be set by mass-assignment for security reasons.
- def attributes_protected_by_default
- default = [ self.class.primary_key, self.class.inheritance_column ]
- default << 'id' unless self.class.primary_key.eql? 'id'
+ def self.attributes_protected_by_default
+ default = [ primary_key, inheritance_column ]
+ default << 'id' unless primary_key.eql? 'id'
default
end
@@ -1920,6 +1797,7 @@ def object_from_yaml(string)
include AttributeMethods::PrimaryKey
include AttributeMethods::TimeZoneConversion
include AttributeMethods::Dirty
+ extend MassAssignmentSecurity
include Callbacks, ActiveModel::Observing, Timestamp
include Associations, AssociationPreload, NamedScope
View
160 activerecord/lib/active_record/mass_assignment_security.rb
@@ -0,0 +1,160 @@
+require 'active_record/mass_assignment_security/permission_set'
+
+module ActiveRecord
+ module MassAssignmentSecurity
+ # 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,
+ # such as a controller, with this behavior.
+ #
+ # For example, a logged in user may need to assign additional attributes depending
+ # on their role:
+ #
+ # class AccountsController < ApplicationController
+ # extend ActiveRecord::MassAssignmentSecurity
+ #
+ # attr_accessible :first_name, :last_name
+ #
+ # def self.admin_accessible_attributes
+ # accessible_attributes + [ :plan_id ]
+ # end
+ #
+ # def update
+ # ...
+ # @account.update_attributes(account_params)
+ # ...
+ # end
+ #
+ # protected
+ #
+ # def account_params
+ # remove_attributes_protected_from_mass_assignment(params[:account])
+ # end
+ #
+ # def mass_assignment_authorizer
+ # admin ? admin_accessible_attributes : super
+ # end
+ #
+ # 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
+
+ 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
+
+ # 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
+
+ # 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)
+ 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)
+ end
+ end
+
+ def mass_assignment_authorizer
+ protected_attributes
+ end
+
+ private
+
+ # 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
+ end
+
+ end
+end
View
44 activerecord/lib/active_record/mass_assignment_security/permission_set.rb
@@ -0,0 +1,44 @@
+require 'active_record/mass_assignment_security/sanitizer'
+
+module ActiveRecord
+ module MassAssignmentSecurity
+ class PermissionSet < Set
+
+ attr_accessor :logger
+
+ def merge(values)
+ super(values.map(&:to_s))
+ end
+
+ def include?(key)
+ super(remove_multiparameter_id(key))
+ end
+
+ protected
+
+ def remove_multiparameter_id(key)
+ key.gsub(/\(.+/, '')
+ end
+
+ end
+
+ class WhiteList < PermissionSet
+ include Sanitizer
+
+ def deny?(key)
+ !include?(key)
+ end
+
+ end
+
+ class BlackList < PermissionSet
+ include Sanitizer
+
+ def deny?(key)
+ include?(key)
+ end
+
+ end
+
+ end
+end
View
27 activerecord/lib/active_record/mass_assignment_security/sanitizer.rb
@@ -0,0 +1,27 @@
+module ActiveRecord
+ module MassAssignmentSecurity
+ module Sanitizer
+
+ # Returns all attributes not denied by the authorizer.
+ def sanitize(attributes)
+ sanitized_attributes = attributes.reject { |key, value| deny?(key) }
+ debug_protected_attribute_removal(attributes, sanitized_attributes) if debug?
+ sanitized_attributes
+ end
+
+ protected
+
+ 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
+ end
+
+ def debug?
+ logger.present?
+ end
+
+ end
+ end
+end
View
26 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 @@ def test_update_attributes!
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 @@ def test_mass_assignment_accessible
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
View
28 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
View
30 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
View
36 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
View
28 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

2 comments on commit 606088d

@norman

This commit caused some working code in my friendly_id plugin to fail; previously accessible_attributes returned nil if attr_accessible had not been invoked; now it always returns an instance of WhiteList no matter what. Is this the desired behavior going forward, or an oversight? If this is intentional it's not a problem to update my code, I'm just curious.

@josevalim
Owner

It's indeed intentional.

Please sign in to comment.
Something went wrong with that request. Please try again.