From 20dadd422bb074fd46e23aa0cc5f7abe64a93f58 Mon Sep 17 00:00:00 2001 From: Andrew White Date: Fri, 27 Nov 2015 13:46:46 +0000 Subject: [PATCH] Don't short-circuit reject_if proc When updating an associated record via nested attribute hashes the reject_if proc could be bypassed if the _destroy flag was set in the attribute hash and allow_destroy was set to false. The fix is to only short-circuit if the _destroy flag is set and the option allow_destroy is set to true. It also fixes an issue where a new record wasn't created if _destroy was set and the option allow_destroy was set to false. CVE-2015-7577 --- activerecord/lib/active_record/nested_attributes.rb | 14 ++++++++++++-- activerecord/test/cases/nested_attributes_test.rb | 13 +++++++++++++ 2 files changed, 25 insertions(+), 2 deletions(-) diff --git a/activerecord/lib/active_record/nested_attributes.rb b/activerecord/lib/active_record/nested_attributes.rb index c5a1488..0d5a8e6 100644 --- a/activerecord/lib/active_record/nested_attributes.rb +++ b/activerecord/lib/active_record/nested_attributes.rb @@ -542,7 +542,7 @@ module ActiveRecord # has_destroy_flag? or if a :reject_if proc exists for this # association and evaluates to +true+. def reject_new_record?(association_name, attributes) - has_destroy_flag?(attributes) || call_reject_if(association_name, attributes) + will_be_destroyed?(association_name, attributes) || call_reject_if(association_name, attributes) end # Determines if a record with the particular +attributes+ should be @@ -551,7 +551,8 @@ module ActiveRecord # # Returns false if there is a +destroy_flag+ on the attributes. def call_reject_if(association_name, attributes) - return false if has_destroy_flag?(attributes) + return false if will_be_destroyed?(association_name, attributes) + case callback = self.nested_attributes_options[association_name][:reject_if] when Symbol method(callback).arity == 0 ? send(callback) : send(callback, attributes) @@ -560,6 +561,15 @@ module ActiveRecord end end + # Only take into account the destroy flag if :allow_destroy is true + def will_be_destroyed?(association_name, attributes) + allow_destroy?(association_name) && has_destroy_flag?(attributes) + end + + def allow_destroy?(association_name) + self.nested_attributes_options[association_name][:allow_destroy] + end + def raise_nested_attributes_record_not_found!(association_name, record_id) model = self.class._reflect_on_association(association_name).klass.name raise RecordNotFound.new("Couldn't find #{model} with ID=#{record_id} for #{self.class.name} with ID=#{id}", diff --git a/activerecord/test/cases/nested_attributes_test.rb b/activerecord/test/cases/nested_attributes_test.rb index 0b700af..6fbc619 100644 --- a/activerecord/test/cases/nested_attributes_test.rb +++ b/activerecord/test/cases/nested_attributes_test.rb @@ -146,6 +146,19 @@ class TestNestedAttributesInGeneral < ActiveRecord::TestCase assert man.reload.interests.empty? end + def test_reject_if_is_not_short_circuited_if_allow_destroy_is_false + Pirate.accepts_nested_attributes_for :ship, reject_if: ->(a) { a[:name] == "The Golden Hind" }, allow_destroy: false + + pirate = Pirate.create!(catchphrase: "Stop wastin' me time", ship_attributes: { name: "White Pearl", _destroy: "1" }) + assert_equal "White Pearl", pirate.reload.ship.name + + pirate.update!(ship_attributes: { id: pirate.ship.id, name: "The Golden Hind", _destroy: "1" }) + assert_equal "White Pearl", pirate.reload.ship.name + + pirate.update!(ship_attributes: { id: pirate.ship.id, name: "Black Pearl", _destroy: "1" }) + assert_equal "Black Pearl", pirate.reload.ship.name + end + def test_has_many_association_updating_a_single_record Man.accepts_nested_attributes_for(:interests) man = Man.create(name: 'John') -- 2.2.1