From ba3dd5ca04e1b70afab964d32f9c0a6f9cace61c Mon Sep 17 00:00:00 2001 From: Tim Rogers Date: Thu, 7 Jul 2016 10:48:13 +0100 Subject: Changes to a dupped `ActionController::Parameters` mutate the original When `ActionController::Parameters` is duplicated with `#dup`, it doesn't create a duplicate of the instance variables (e.g. `@parameters`) but rather maintains the reference (see ). Given that the parameters object is often manipulated as if it were a hash (e.g. with `#delete` and similar methods), this leads to unexpected behaviour, like the following: ``` params = ActionController::Parameters.new(foo: "bar") duplicated_params = params.dup duplicated_params.delete(:foo) params == duplicated_params ``` This fixes the bug by defining a private `#initialize_copy` method, used internally by `#dup`, which makes a copy of `@parameters`. --- actionpack/test/controller/parameters/dup_test.rb | 25 +++++++++++++++++++++++ 1 file changed, 25 insertions(+) create mode 100644 actionpack/test/controller/parameters/dup_test.rb (limited to 'actionpack/test/controller') diff --git a/actionpack/test/controller/parameters/dup_test.rb b/actionpack/test/controller/parameters/dup_test.rb new file mode 100644 index 0000000000..cf273933bd --- /dev/null +++ b/actionpack/test/controller/parameters/dup_test.rb @@ -0,0 +1,25 @@ +require 'abstract_unit' +require 'action_controller/metal/strong_parameters' + +class ParametersDupTest < ActiveSupport::TestCase + setup do + ActionController::Parameters.permit_all_parameters = false + + @params = ActionController::Parameters.new( + person: { + age: '32', + name: { + first: 'David', + last: 'Heinemeier Hansson' + }, + addresses: [{city: 'Chicago', state: 'Illinois'}] + } + ) + end + + test "changes on a duplicate do not affect the original" do + dupped_params = @params.dup + dupped_params.delete(:person) + assert_not_equal @params, dupped_params + end +end -- cgit v1.2.3 From 96070595697477676ea051c19c2c951901302bed Mon Sep 17 00:00:00 2001 From: Tim Rogers Date: Fri, 8 Jul 2016 17:48:21 +0100 Subject: Trust `Object#dup` in `ActionController::Parameters`, using `#initialize_copy` to manually duplicate the underlying parameters hash It looks like `ActionController::Parameters#dup` is leftover from when the class inherited from `Hash`. We can just trust `#dup`, which already copies the `@permitted` instance variable (confirmed by tests). We still define a `#initialize_copy` to make `@parameters` a copy that can be mutated without affecting the original instance. --- actionpack/test/controller/parameters/dup_test.rb | 20 +++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-) (limited to 'actionpack/test/controller') diff --git a/actionpack/test/controller/parameters/dup_test.rb b/actionpack/test/controller/parameters/dup_test.rb index cf273933bd..ac5a9d94d0 100644 --- a/actionpack/test/controller/parameters/dup_test.rb +++ b/actionpack/test/controller/parameters/dup_test.rb @@ -17,9 +17,27 @@ class ParametersDupTest < ActiveSupport::TestCase ) end - test "changes on a duplicate do not affect the original" do + test "a duplicate maintains the original's permitted status" do + @params.permit! + dupped_params = @params.dup + assert dupped_params.permitted? + end + + test "a duplicate maintains the original's parameters" do + @params.permit! + dupped_params = @params.dup + assert_equal @params.to_h, dupped_params.to_h + end + + test "changes to a duplicate's parameters do not affect the original" do dupped_params = @params.dup dupped_params.delete(:person) assert_not_equal @params, dupped_params end + + test "changes tp a duplicate's permitted status do not affect the original" do + dupped_params = @params.dup + dupped_params.permit! + assert_not_equal @params, dupped_params + end end -- cgit v1.2.3