Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP
Browse files

Revert 0c0b0aa which introduced a security vulnerability.

This addresses  CVE-2010-3933

Conflicts:

	activerecord/lib/active_record/nested_attributes.rb
  • Loading branch information...
commit 9ebe582830fd0386e09a917d81eb6cff494cd590 1 parent 13f7f89
@NZKoz NZKoz authored
View
17 activerecord/lib/active_record/nested_attributes.rb
@@ -324,9 +324,7 @@ def assign_nested_attributes_for_one_to_one_association(association_name, attrib
assign_to_or_mark_for_destruction(record, attributes, options[:allow_destroy]) unless call_reject_if(association_name, attributes)
elsif attributes['id']
- existing_record = self.class.reflect_on_association(association_name).klass.find(attributes['id'])
- assign_to_or_mark_for_destruction(existing_record, attributes, options[:allow_destroy])
- self.send(association_name.to_s+'=', existing_record)
+ raise_nested_attributes_record_not_found(association_name, attributes['id'])
elsif !reject_new_record?(association_name, attributes)
method = "build_#{association_name}"
@@ -402,15 +400,12 @@ def assign_nested_attributes_for_collection_association(association_name, attrib
association.build(attributes.except(*UNASSIGNABLE_KEYS))
end
- elsif existing_records.count == 0 #Existing record but not yet associated
- existing_record = self.class.reflect_on_association(association_name).klass.find(attributes['id'])
- association.send(:add_record_to_target_with_callbacks, existing_record) if !association.loaded? && !call_reject_if(association_name, attributes)
- assign_to_or_mark_for_destruction(existing_record, attributes, options[:allow_destroy])
-
elsif existing_record = existing_records.detect { |record| record.id.to_s == attributes['id'].to_s }
association.send(:add_record_to_target_with_callbacks, existing_record) if !association.loaded? && !call_reject_if(association_name, attributes)
assign_to_or_mark_for_destruction(existing_record, attributes, options[:allow_destroy])
+ else
+ raise_nested_attributes_record_not_found(association_name, attributes['id'])
end
end
end
@@ -430,7 +425,7 @@ def has_destroy_flag?(hash)
ConnectionAdapters::Column.value_to_boolean(hash['_destroy'])
end
- # Determines if a new record should be built by checking for
+ # Determines if a new record should be build by checking for
# has_destroy_flag? or if a <tt>:reject_if</tt> proc exists for this
# association and evaluates to +true+.
def reject_new_record?(association_name, attributes)
@@ -446,5 +441,9 @@ def call_reject_if(association_name, attributes)
end
end
+ def raise_nested_attributes_record_not_found(association_name, record_id)
+ reflection = self.class.reflect_on_association(association_name)
+ raise RecordNotFound, "Couldn't find #{reflection.klass.name} with ID=#{record_id} for #{self.class.name} with ID=#{id}"
+ end
end
end
View
26 activerecord/test/cases/nested_attributes_test.rb
@@ -203,6 +203,12 @@ def test_should_modify_an_existing_record_if_there_is_a_matching_id
assert_equal 'Davy Jones Gold Dagger', @pirate.ship.name
end
+ def test_should_raise_RecordNotFound_if_an_id_is_given_but_doesnt_return_a_record
+ assert_raise_with_message ActiveRecord::RecordNotFound, "Couldn't find Ship with ID=1234567890 for Pirate with ID=#{@pirate.id}" do
+ @pirate.ship_attributes = { :id => 1234567890 }
+ end
+ end
+
def test_should_take_a_hash_with_string_keys_and_update_the_associated_model
@pirate.reload.ship_attributes = { 'id' => @ship.id, 'name' => 'Davy Jones Gold Dagger' }
@@ -382,13 +388,10 @@ def test_should_modify_an_existing_record_if_there_is_a_matching_id
assert_equal 'Arr', @ship.pirate.catchphrase
end
- def test_should_associate_with_record_if_parent_record_is_not_saved
- @ship.destroy
- @pirate = Pirate.create(:catchphrase => 'Arr')
- @ship = Ship.new(:name => 'Nights Dirty Lightning', :pirate_attributes => { :id => @pirate.id, :catchphrase => @pirate.catchphrase})
-
- assert_equal @ship.name, 'Nights Dirty Lightning'
- assert_equal @pirate, @ship.pirate
+ def test_should_raise_RecordNotFound_if_an_id_is_given_but_doesnt_return_a_record
+ assert_raise_with_message ActiveRecord::RecordNotFound, "Couldn't find Pirate with ID=1234567890 for Ship with ID=#{@ship.id}" do
+ @ship.pirate_attributes = { :id => 1234567890 }
+ end
end
def test_should_take_a_hash_with_string_keys_and_update_the_associated_model
@@ -518,11 +521,6 @@ def test_should_take_an_array_and_assign_the_attributes_to_the_associated_models
assert_equal ['Grace OMalley', 'Privateers Greed'], [@child_1.reload.name, @child_2.reload.name]
end
- def test_should_assign_existing_children_if_parent_is_new
- @pirate = Pirate.new({:catchphrase => "Don' botharr talkin' like one, savvy?"}.merge(@alternate_params))
- assert_equal ['Grace OMalley', 'Privateers Greed'], [@pirate.send(@association_name)[0].name, @pirate.send(@association_name)[1].name]
- end
-
def test_should_also_work_with_a_HashWithIndifferentAccess
@pirate.send(association_setter, HashWithIndifferentAccess.new('foo' => HashWithIndifferentAccess.new(:id => @child_1.id, :name => 'Grace OMalley')))
@pirate.save
@@ -586,8 +584,8 @@ def test_should_take_a_hash_with_composite_id_keys_and_assign_the_attributes_to_
assert_equal ['Grace OMalley', 'Privateers Greed'], [@child_1.name, @child_2.name]
end
- def test_should_not_raise_RecordNotFound_if_an_id_is_given_but_doesnt_return_a_record
- assert_nothing_raised ActiveRecord::RecordNotFound do
+ def test_should_raise_RecordNotFound_if_an_id_is_given_but_doesnt_return_a_record
+ assert_raise_with_message ActiveRecord::RecordNotFound, "Couldn't find #{@child_1.class.name} with ID=1234567890 for Pirate with ID=#{@pirate.id}" do
@pirate.attributes = { association_getter => [{ :id => 1234567890 }] }
end
end
Please sign in to comment.
Something went wrong with that request. Please try again.