Merge pull request #25735 from timrogers/actioncontroller-parameters-dup

Stop changes to a dupped `ActionController::Parameters` mutating the original
skuwa229 · Jul 11, 2016 · e6352db · e6352db
2 parents 629dde2 + 9607059
commit e6352db
Show file tree

Hide file tree

Showing 2 changed files with 48 additions and 14 deletions.
diff --git a/actionpack/lib/action_controller/metal/strong_parameters.rb b/actionpack/lib/action_controller/metal/strong_parameters.rb
@@ -572,20 +572,6 @@ def values_at(*keys)
       convert_value_to_parameters(@parameters.values_at(*keys))
     end
 
-    # Returns an exact copy of the <tt>ActionController::Parameters</tt>
-    # instance. +permitted+ state is kept on the duped object.
-    #
-    #   params = ActionController::Parameters.new(a: 1)
-    #   params.permit!
-    #   params.permitted?        # => true
-    #   copy_params = params.dup # => <ActionController::Parameters {"a"=>1} permitted: true>
-    #   copy_params.permitted?   # => true
-    def dup
-      super.tap do |duplicate|
-        duplicate.permitted = @permitted
-      end
-    end
-
     # Returns a new <tt>ActionController::Parameters</tt> with all keys from
     # +other_hash+ merges into current hash.
     def merge(other_hash)
@@ -783,6 +769,11 @@ def hash_filter(params, filter)
           end
         end
       end
+
+      def initialize_copy(source)
+        super
+        @parameters = @parameters.dup
+      end
   end
 
   # == Strong \Parameters

diff --git a/actionpack/test/controller/parameters/dup_test.rb b/actionpack/test/controller/parameters/dup_test.rb
@@ -0,0 +1,43 @@
+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 "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