From 4b66aab00fa0ea6bcc6ec81df19e44de34fd7864 Mon Sep 17 00:00:00 2001 From: Josh Kalderimis Date: Thu, 8 Jul 2010 18:16:36 +0200 Subject: mass_assignment_security moved from AR to AMo, and minor test cleanup MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: José Valim --- activemodel/lib/active_model.rb | 1 + .../lib/active_model/mass_assignment_security.rb | 145 +++++++++++++++++++++ .../mass_assignment_security/permission_set.rb | 41 ++++++ .../mass_assignment_security/sanitizer.rb | 29 +++++ .../mass_assignment_security/black_list_test.rb | 28 ++++ .../permission_set_test.rb | 30 +++++ .../mass_assignment_security/sanitizer_test.rb | 37 ++++++ .../mass_assignment_security/white_list_test.rb | 28 ++++ .../test/cases/mass_assignment_security_test.rb | 52 ++++++++ .../test/models/mass_assignment_specific.rb | 57 ++++++++ 10 files changed, 448 insertions(+) create mode 100644 activemodel/lib/active_model/mass_assignment_security.rb create mode 100644 activemodel/lib/active_model/mass_assignment_security/permission_set.rb create mode 100644 activemodel/lib/active_model/mass_assignment_security/sanitizer.rb create mode 100644 activemodel/test/cases/mass_assignment_security/black_list_test.rb create mode 100644 activemodel/test/cases/mass_assignment_security/permission_set_test.rb create mode 100644 activemodel/test/cases/mass_assignment_security/sanitizer_test.rb create mode 100644 activemodel/test/cases/mass_assignment_security/white_list_test.rb create mode 100644 activemodel/test/cases/mass_assignment_security_test.rb create mode 100644 activemodel/test/models/mass_assignment_specific.rb (limited to 'activemodel') diff --git a/activemodel/lib/active_model.rb b/activemodel/lib/active_model.rb index 026430fee3..5ed21a39c2 100644 --- a/activemodel/lib/active_model.rb +++ b/activemodel/lib/active_model.rb @@ -38,6 +38,7 @@ module ActiveModel autoload :EachValidator, 'active_model/validator' autoload :Errors autoload :Lint + autoload :MassAssignmentSecurity autoload :Name, 'active_model/naming' autoload :Naming autoload :Observer, 'active_model/observing' diff --git a/activemodel/lib/active_model/mass_assignment_security.rb b/activemodel/lib/active_model/mass_assignment_security.rb new file mode 100644 index 0000000000..c0549ba6c0 --- /dev/null +++ b/activemodel/lib/active_model/mass_assignment_security.rb @@ -0,0 +1,145 @@ +require 'active_support/core_ext/class/attribute.rb' +require 'active_model/mass_assignment_security/permission_set' + +module ActiveModel + # = 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, + # 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 + # include 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 + # sanitize_for_mass_assignment(params[:account]) + # end + # + # def mass_assignment_authorizer + # admin ? admin_accessible_attributes : super + # end + # + # end + # + module ClassMethods + # Attributes named in this macro are protected from mass-assignment, + # such as new(attributes), + # update_attributes(attributes), or + # attributes=(attributes). + # + # 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 Hash#except or Hash#slice 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 + + # Specifies a white list of model attributes that can be set via + # mass-assignment, such as new(attributes), + # update_attributes(attributes), or + # attributes=(attributes) + # + # 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 Hash#except or Hash#slice 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 + + def protected_attributes + self._protected_attributes ||= BlackList.new(attributes_protected_by_default).tap do |w| + w.logger = self.logger if self.respond_to?(:logger) + end + end + + def accessible_attributes + self._accessible_attributes ||= WhiteList.new.tap { |w| w.logger = self.logger if self.respond_to?(:logger) } + end + + def active_authorizer + self._active_authorizer ||= protected_attributes + end + + def attributes_protected_by_default + [] + end + end + + protected + + def sanitize_for_mass_assignment(attributes) + mass_assignment_authorizer.sanitize(attributes) + end + + def mass_assignment_authorizer + self.class.active_authorizer + end + + end +end diff --git a/activemodel/lib/active_model/mass_assignment_security/permission_set.rb b/activemodel/lib/active_model/mass_assignment_security/permission_set.rb new file mode 100644 index 0000000000..978da493d7 --- /dev/null +++ b/activemodel/lib/active_model/mass_assignment_security/permission_set.rb @@ -0,0 +1,41 @@ +require 'active_model/mass_assignment_security/sanitizer' + +module ActiveModel + module MassAssignmentSecurity + + class PermissionSet < Set + attr_accessor :logger + + def +(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 \ No newline at end of file diff --git a/activemodel/lib/active_model/mass_assignment_security/sanitizer.rb b/activemodel/lib/active_model/mass_assignment_security/sanitizer.rb new file mode 100644 index 0000000000..275e481fb8 --- /dev/null +++ b/activemodel/lib/active_model/mass_assignment_security/sanitizer.rb @@ -0,0 +1,29 @@ +module ActiveModel + 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 + warn!(removed_keys) if removed_keys.any? + end + + def debug? + self.logger.present? + end + + def warn!(attrs) + self.logger.debug "WARNING: Can't mass-assign protected attributes: #{attrs.join(', ')}" + end + + end + end +end diff --git a/activemodel/test/cases/mass_assignment_security/black_list_test.rb b/activemodel/test/cases/mass_assignment_security/black_list_test.rb new file mode 100644 index 0000000000..ed168bc016 --- /dev/null +++ b/activemodel/test/cases/mass_assignment_security/black_list_test.rb @@ -0,0 +1,28 @@ +require "cases/helper" + +class BlackListTest < ActiveModel::TestCase + + def setup + @black_list = ActiveModel::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/activemodel/test/cases/mass_assignment_security/permission_set_test.rb b/activemodel/test/cases/mass_assignment_security/permission_set_test.rb new file mode 100644 index 0000000000..d005b638e4 --- /dev/null +++ b/activemodel/test/cases/mass_assignment_security/permission_set_test.rb @@ -0,0 +1,30 @@ +require "cases/helper" + +class PermissionSetTest < ActiveModel::TestCase + + def setup + @permission_list = ActiveModel::MassAssignmentSecurity::PermissionSet.new + end + + test "+ stringifies added collection values" do + symbol_collection = [ :admin ] + new_list = @permission_list += symbol_collection + + 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)' + new_list = @permission_list += [ 'admin' ] + + 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' + new_list = @permission_list += [ normal_key ] + + assert new_list.include?(normal_key), "#{normal_key} not found in #{@permission_list.inspect}" + end + +end diff --git a/activemodel/test/cases/mass_assignment_security/sanitizer_test.rb b/activemodel/test/cases/mass_assignment_security/sanitizer_test.rb new file mode 100644 index 0000000000..367207aab3 --- /dev/null +++ b/activemodel/test/cases/mass_assignment_security/sanitizer_test.rb @@ -0,0 +1,37 @@ +require "cases/helper" +require 'logger' + +class SanitizerTest < ActiveModel::TestCase + + class SanitizingAuthorizer + include ActiveModel::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/activemodel/test/cases/mass_assignment_security/white_list_test.rb b/activemodel/test/cases/mass_assignment_security/white_list_test.rb new file mode 100644 index 0000000000..aa3596ad2a --- /dev/null +++ b/activemodel/test/cases/mass_assignment_security/white_list_test.rb @@ -0,0 +1,28 @@ +require "cases/helper" + +class WhiteListTest < ActiveModel::TestCase + + def setup + @white_list = ActiveModel::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 diff --git a/activemodel/test/cases/mass_assignment_security_test.rb b/activemodel/test/cases/mass_assignment_security_test.rb new file mode 100644 index 0000000000..0f7a38b0bc --- /dev/null +++ b/activemodel/test/cases/mass_assignment_security_test.rb @@ -0,0 +1,52 @@ +require "cases/helper" +require 'models/mass_assignment_specific' + +class MassAssignmentSecurityTest < ActiveModel::TestCase + + def test_attribute_protection + user = User.new + expected = { "name" => "John Smith", "email" => "john@smith.com" } + sanitized = user.sanitize_for_mass_assignment(expected.merge("admin" => true)) + assert_equal expected, sanitized + end + + def test_attributes_accessible + user = Person.new + expected = { "name" => "John Smith", "email" => "john@smith.com" } + sanitized = user.sanitize_for_mass_assignment(expected.merge("super_powers" => true)) + assert_equal expected, sanitized + end + + def test_attributes_protected_by_default + firm = Firm.new + expected = { } + sanitized = firm.sanitize_for_mass_assignment({ "type" => "Client" }) + assert_equal expected, sanitized + end + + def test_mass_assignment_protection_inheritance + assert LoosePerson.accessible_attributes.blank? + assert_equal Set.new([ 'credit_rating', 'administrator']), LoosePerson.protected_attributes + + assert LooseDescendant.accessible_attributes.blank? + assert_equal Set.new([ 'credit_rating', 'administrator', 'phone_number']), LooseDescendant.protected_attributes + + assert LooseDescendantSecond.accessible_attributes.blank? + 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 (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 + attributes = { "starting(1i)" => "2004", "starting(2i)" => "6", "starting(3i)" => "24" } + sanitized = task.sanitize_for_mass_assignment(attributes) + assert_equal sanitized, { } + end + +end \ No newline at end of file diff --git a/activemodel/test/models/mass_assignment_specific.rb b/activemodel/test/models/mass_assignment_specific.rb new file mode 100644 index 0000000000..2a8fe170c2 --- /dev/null +++ b/activemodel/test/models/mass_assignment_specific.rb @@ -0,0 +1,57 @@ +class User + include ActiveModel::MassAssignmentSecurity + attr_protected :admin + + public :sanitize_for_mass_assignment +end + +class Person + include ActiveModel::MassAssignmentSecurity + attr_accessible :name, :email + + public :sanitize_for_mass_assignment +end + +class Firm + include ActiveModel::MassAssignmentSecurity + + public :sanitize_for_mass_assignment + + def self.attributes_protected_by_default + ["type"] + end +end + +class Task + include ActiveModel::MassAssignmentSecurity + attr_protected :starting + + public :sanitize_for_mass_assignment +end + +class LoosePerson + include ActiveModel::MassAssignmentSecurity + 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 + include ActiveModel::MassAssignmentSecurity + attr_accessible :name, :address + + def self.attributes_protected_by_default + ["mobile_number"] + end +end + +class TightDescendant < TightPerson + attr_accessible :phone_number +end \ No newline at end of file -- cgit v1.2.3