Merge pull request #20868 from sikachu/params-not-inherited-from-hwia

Make AC::Parameters not inherited from Hash

9 files changed, 157 insertions, 38 deletions

diff --git a/actionpack/CHANGELOG.md b/actionpack/CHANGELOG.md
index 6c6f27ce90..c70e6a771f 100644
--- a/actionpack/CHANGELOG.md
+++ b/actionpack/CHANGELOG.md
@@ -1,3 +1,17 @@
+*   `ActionController::Parameters` no longer inherits from
+    `HashWithIndifferentAccess`
+
+    Inheriting from `HashWithIndifferentAccess` allowed users to call any
+    enumerable methods on `Parameters` object, resulting in a risk of losing the
+    `permitted?` status or even getting back a pure `Hash` object instead of
+    a `Parameters` object with proper sanitization.
+
+    By not inheriting from `HashWithIndifferentAccess`, we are able to make
+    sure that all methods that are defined in `Parameters` object will return
+    a proper `Parameters` object with a correct `permitted?` flag.
+
+    *Prem Sichanugrist*
+
 *   Replaced `ActiveSupport::Concurrency::Latch` with `Concurrent::CountDownLatch`
     from the concurrent-ruby gem.
 
diff --git a/actionpack/lib/action_controller/metal/strong_parameters.rb b/actionpack/lib/action_controller/metal/strong_parameters.rb
index 09867e2407..3f5e8c53a8 100644
--- a/actionpack/lib/action_controller/metal/strong_parameters.rb
+++ b/actionpack/lib/action_controller/metal/strong_parameters.rb
@@ -104,10 +104,12 @@ module ActionController
   #   params = ActionController::Parameters.new(key: 'value')
   #   params[:key]  # => "value"
   #   params["key"] # => "value"
-  class Parameters < ActiveSupport::HashWithIndifferentAccess
+  class Parameters
     cattr_accessor :permit_all_parameters, instance_accessor: false
     cattr_accessor :action_on_unpermitted_parameters, instance_accessor: false
 
+    delegate :keys, :key?, :has_key?, :empty?, :inspect, to: :@parameters
+
     # By default, never raise an UnpermittedParameters exception if these
     # params are present. The default includes both 'controller' and 'action'
     # because they are added by Rails and should be of no concern. One way
@@ -144,11 +146,22 @@ module ActionController
     #   params = ActionController::Parameters.new(name: 'Francesco')
     #   params.permitted?  # => true
     #   Person.new(params) # => #<Person id: nil, name: "Francesco">
-    def initialize(attributes = nil)
-      super(attributes)
+    def initialize(parameters = {})
+      @parameters = parameters.with_indifferent_access
       @permitted = self.class.permit_all_parameters
     end
 
+    # Returns true if another +Parameters+ object contains the same content and
+    # permitted flag, or other Hash-like object contains the same content. This
+    # override is in place so you can perform a comparison with `Hash`.
+    def ==(other_hash)
+      if other_hash.respond_to?(:permitted?)
+        super
+      else
+        @parameters == other_hash
+      end
+    end
+
     # Returns a safe +Hash+ representation of this parameter with all
     # unpermitted keys removed.
     #
@@ -162,7 +175,7 @@ module ActionController
     #   safe_params.to_h # => {"name"=>"Senjougahara Hitagi"}
     def to_h
       if permitted?
-        to_hash
+        @parameters.to_h
       else
         slice(*self.class.always_permitted_parameters).permit!.to_h
       end
@@ -170,20 +183,17 @@ module ActionController
 
     # Returns an unsafe, unfiltered +Hash+ representation of this parameter.
     def to_unsafe_h
-      to_hash
+      @parameters
     end
     alias_method :to_unsafe_hash, :to_unsafe_h
 
     # Convert all hashes in values into parameters, then yield each pair like
     # the same way as <tt>Hash#each_pair</tt>
     def each_pair(&block)
-      super do |key, value|
-        convert_hashes_to_parameters(key, value)
+      @parameters.each_pair do |key, value|
+        yield key, convert_hashes_to_parameters(key, value)
       end
-
-      super
     end
-
     alias_method :each, :each_pair
 
     # Attribute that keeps track of converted arrays, if any, to avoid double
@@ -347,7 +357,13 @@ module ActionController
     #   params[:person] # => {"name"=>"Francesco"}
     #   params[:none]   # => nil
     def [](key)
-      convert_hashes_to_parameters(key, super)
+      convert_hashes_to_parameters(key, @parameters[key])
+    end
+
+    # Assigns a value to a given +key+. The given key may still get filtered out
+    # when +permit+ is called.
+    def []=(key, value)
+      @parameters[key] = value
     end
 
     # Returns a parameter for the given +key+. If the +key+
@@ -361,8 +377,12 @@ module ActionController
     #   params.fetch(:none)                 # => ActionController::ParameterMissing: param is missing or the value is empty: none
     #   params.fetch(:none, 'Francesco')    # => "Francesco"
     #   params.fetch(:none) { 'Francesco' } # => "Francesco"
-    def fetch(key, *args)
-      convert_hashes_to_parameters(key, super, false)
+    def fetch(key, *args, &block)
+      convert_hashes_to_parameters(
+        key,
+        @parameters.fetch(key, *args, &block),
+        false
+      )
     rescue KeyError
       raise ActionController::ParameterMissing.new(key)
     end
@@ -375,7 +395,24 @@ module ActionController
     #   params.slice(:a, :b) # => {"a"=>1, "b"=>2}
     #   params.slice(:d)     # => {}
     def slice(*keys)
-      new_instance_with_inherited_permitted_status(super)
+      new_instance_with_inherited_permitted_status(@parameters.slice(*keys))
+    end
+
+    # Returns current <tt>ActionController::Parameters</tt> instance which
+    # contains only the given +keys+.
+    def slice!(*keys)
+      @parameters.slice!(*keys)
+      self
+    end
+
+    # Returns a new <tt>ActionController::Parameters</tt> instance that
+    # filters out the given +keys+.
+    #
+    #   params = ActionController::Parameters.new(a: 1, b: 2, c: 3)
+    #   params.except(:a, :b) # => {"c"=>3}
+    #   params.except(:d)     # => {"a"=>1,"b"=>2,"c"=>3}
+    def except(*keys)
+      new_instance_with_inherited_permitted_status(@parameters.except(*keys))
     end
 
     # Removes and returns the key/value pairs matching the given keys.
@@ -384,7 +421,7 @@ module ActionController
     #   params.extract!(:a, :b) # => {"a"=>1, "b"=>2}
     #   params                  # => {"c"=>3}
     def extract!(*keys)
-      new_instance_with_inherited_permitted_status(super)
+      new_instance_with_inherited_permitted_status(@parameters.extract!(*keys))
     end
 
     # Returns a new <tt>ActionController::Parameters</tt> with the results of
@@ -393,36 +430,80 @@ module ActionController
     #   params = ActionController::Parameters.new(a: 1, b: 2, c: 3)
     #   params.transform_values { |x| x * 2 }
     #   # => {"a"=>2, "b"=>4, "c"=>6}
-    def transform_values
-      if block_given?
-        new_instance_with_inherited_permitted_status(super)
+    def transform_values(&block)
+      if block
+        new_instance_with_inherited_permitted_status(
+          @parameters.transform_values(&block)
+        )
       else
-        super
+        @parameters.transform_values
       end
     end
 
-    # This method is here only to make sure that the returned object has the
-    # correct +permitted+ status. It should not matter since the parent of
-    # this object is +HashWithIndifferentAccess+
-    def transform_keys # :nodoc:
-      if block_given?
-        new_instance_with_inherited_permitted_status(super)
+    # Performs values transformation and returns the altered
+    # <tt>ActionController::Parameters</tt> instance.
+    def transform_values!(&block)
+      @parameters.transform_values!(&block)
+      self
+    end
+
+    # Returns a new <tt>ActionController::Parameters</tt> instance with the
+    # results of running +block+ once for every key. The values are unchanged.
+    def transform_keys(&block)
+      if block
+        new_instance_with_inherited_permitted_status(
+          @parameters.transform_keys(&block)
+        )
       else
-        super
+        @parameters.transform_keys
       end
     end
 
+    # Performs keys transfomration and returns the altered
+    # <tt>ActionController::Parameters</tt> instance.
+    def transform_keys!(&block)
+      @parameters.transform_keys!(&block)
+      self
+    end
+
     # Deletes and returns a key-value pair from +Parameters+ whose key is equal
     # to key. If the key is not found, returns the default value. If the
     # optional code block is given and the key is not found, pass in the key
     # and return the result of block.
     def delete(key, &block)
-      convert_hashes_to_parameters(key, super, false)
+      convert_hashes_to_parameters(key, @parameters.delete(key), false)
+    end
+
+    # Returns a new instance of <tt>ActionController::Parameters</tt> with only
+    # items that the block evaluates to true.
+    def select(&block)
+      new_instance_with_inherited_permitted_status(@parameters.select(&block))
     end
 
     # Equivalent to Hash#keep_if, but returns nil if no changes were made.
     def select!(&block)
-      convert_value_to_parameters(super)
+      @parameters.select!(&block)
+      self
+    end
+    alias_method :keep_if, :select!
+
+    # Returns a new instance of <tt>ActionController::Parameters</tt> with items
+    # that the block evaluates to true removed.
+    def reject(&block)
+      new_instance_with_inherited_permitted_status(@parameters.reject(&block))
+    end
+
+    # Removes items that the block evaluates to true and returns self.
+    def reject!(&block)
+      @parameters.reject!(&block)
+      self
+    end
+    alias_method :delete_if, :reject!
+
+    # Return values that were assigned to the given +keys+. Note that all the
+    # +Hash+ objects will be converted to <tt>ActionController::Parameters</tt>.
+    def values_at(*keys)
+      convert_value_to_parameters(@parameters.values_at(*keys))
     end
 
     # Returns an exact copy of the <tt>ActionController::Parameters</tt>
@@ -439,6 +520,21 @@ module ActionController
       end
     end
 
+    # Returns a new <tt>ActionController::Parameters</tt> with all keys from
+    # +other_hash+ merges into current hash.
+    def merge(other_hash)
+      new_instance_with_inherited_permitted_status(
+        @parameters.merge(other_hash)
+      )
+    end
+
+    # This is required by ActiveModel attribute assignment, so that user can
+    # pass +Parameters+ to a mass assignment methods in a model. It should not
+    # matter as we are using +HashWithIndifferentAccess+ internally.
+    def stringify_keys # :nodoc:
+      dup
+    end
+
     protected
       def permitted=(new_permitted)
         @permitted = new_permitted
@@ -453,7 +549,7 @@ module ActionController
 
       def convert_hashes_to_parameters(key, value, assign_if_converted=true)
         converted = convert_value_to_parameters(value)
-        self[key] = converted if assign_if_converted && !converted.equal?(value)
+        @parameters[key] = converted if assign_if_converted && !converted.equal?(value)
         converted
       end
 
@@ -482,7 +578,8 @@ module ActionController
       end
 
       def fields_for_style?(object)
-        object.is_a?(Hash) && object.all? { |k, v| k =~ /\A-?\d+\z/ && v.is_a?(Hash) }
+        (object.is_a?(Hash) || object.is_a?(Parameters)) &&
+          object.to_unsafe_h.all? { |k, v| k =~ /\A-?\d+\z/ && v.is_a?(Hash) }
       end
 
       def unpermitted_parameters!(params)
@@ -571,7 +668,7 @@ module ActionController
           else
             # Declaration { user: :name } or { user: [:name, :age, { address: ... }] }.
             params[key] = each_element(value) do |element|
-              if element.is_a?(Hash)
+              if element.is_a?(Hash) || element.is_a?(Parameters)
                 element = self.class.new(element) unless element.respond_to?(:permit)
                 element.permit(*Array.wrap(filter[key]))
               end
diff --git a/actionpack/lib/action_dispatch/routing/url_for.rb b/actionpack/lib/action_dispatch/routing/url_for.rb
index 8379d089df..967bbd62f8 100644
--- a/actionpack/lib/action_dispatch/routing/url_for.rb
+++ b/actionpack/lib/action_dispatch/routing/url_for.rb
@@ -171,6 +171,10 @@ module ActionDispatch
           route_name = options.delete :use_route
           _routes.url_for(options.symbolize_keys.reverse_merge!(url_options),
                          route_name)
+        when ActionController::Parameters
+          route_name = options.delete :use_route
+          _routes.url_for(options.to_unsafe_h.symbolize_keys.
+                          reverse_merge!(url_options), route_name)
         when String
           options
         when Symbol
diff --git a/actionpack/test/controller/api/params_wrapper_test.rb b/actionpack/test/controller/api/params_wrapper_test.rb
index e40a39d829..53b3a0c3cc 100644
--- a/actionpack/test/controller/api/params_wrapper_test.rb
+++ b/actionpack/test/controller/api/params_wrapper_test.rb
@@ -7,7 +7,7 @@ class ParamsWrapperForApiTest < ActionController::TestCase
     wrap_parameters :person, format: [:json]
 
     def test
-      self.last_parameters = params.except(:controller, :action)
+      self.last_parameters = params.except(:controller, :action).to_unsafe_h
       head :ok
     end
   end
diff --git a/actionpack/test/controller/parameters/nested_parameters_test.rb b/actionpack/test/controller/parameters/nested_parameters_test.rb
index 3b1257e8d5..7151a8567c 100644
--- a/actionpack/test/controller/parameters/nested_parameters_test.rb
+++ b/actionpack/test/controller/parameters/nested_parameters_test.rb
@@ -136,7 +136,7 @@ class NestedParametersTest < ActiveSupport::TestCase
         authors_attributes: {
           :'0' => { name: 'William Shakespeare', age_of_death: '52' },
           :'1' => { name: 'Unattributed Assistant' },
-          :'2' => { name: %w(injected names)}
+          :'2' => { name: %w(injected names) }
         }
       }
     })
diff --git a/actionpack/test/controller/parameters/parameters_permit_test.rb b/actionpack/test/controller/parameters/parameters_permit_test.rb
index 2ed486516d..05532ec21b 100644
--- a/actionpack/test/controller/parameters/parameters_permit_test.rb
+++ b/actionpack/test/controller/parameters/parameters_permit_test.rb
@@ -253,7 +253,6 @@ class ParametersPermitTest < ActiveSupport::TestCase
 
     assert @params.to_h.is_a? Hash
     assert_not @params.to_h.is_a? ActionController::Parameters
-    assert_equal @params.to_hash, @params.to_h
   end
 
   test "to_h returns converted hash when .permit_all_parameters is set" do
@@ -284,6 +283,5 @@ class ParametersPermitTest < ActiveSupport::TestCase
   test "to_unsafe_h returns unfiltered params" do
     assert @params.to_h.is_a? Hash
     assert_not @params.to_h.is_a? ActionController::Parameters
-    assert_equal @params.to_hash, @params.to_unsafe_h
   end
 end
diff --git a/actionpack/test/controller/test_case_test.rb b/actionpack/test/controller/test_case_test.rb
index b991232a14..0a20b2c3fb 100644
--- a/actionpack/test/controller/test_case_test.rb
+++ b/actionpack/test/controller/test_case_test.rb
@@ -45,7 +45,7 @@ class TestCaseTest < ActionController::TestCase
     end
 
     def test_params
-      render text: ::JSON.dump(params)
+      render text: ::JSON.dump(params.to_unsafe_h)
     end
 
     def test_query_parameters
diff --git a/actionpack/test/controller/webservice_test.rb b/actionpack/test/controller/webservice_test.rb
index 21fa670bb6..bd81846a7d 100644
--- a/actionpack/test/controller/webservice_test.rb
+++ b/actionpack/test/controller/webservice_test.rb
@@ -14,7 +14,13 @@ class WebServiceTest < ActionDispatch::IntegrationTest
     def dump_params_keys(hash = params)
       hash.keys.sort.inject("") do |s, k|
         value = hash[k]
-        value = Hash === value ? "(#{dump_params_keys(value)})" : ""
+
+        if value.is_a?(Hash) || value.is_a?(ActionController::Parameters)
+          value = "(#{dump_params_keys(value)})"
+        else
+          value = ""
+        end
+
         s << ", " unless s.empty?
         s << "#{k}#{value}"
       end
diff --git a/actionpack/test/dispatch/request/json_params_parsing_test.rb b/actionpack/test/dispatch/request/json_params_parsing_test.rb
index d77341bc64..c2300a0142 100644
--- a/actionpack/test/dispatch/request/json_params_parsing_test.rb
+++ b/actionpack/test/dispatch/request/json_params_parsing_test.rb
@@ -113,7 +113,7 @@ class RootLessJSONParamsParsingTest < ActionDispatch::IntegrationTest
 
     def parse
       self.class.last_request_parameters = request.request_parameters
-      self.class.last_parameters = params
+      self.class.last_parameters = params.to_unsafe_h
       head :ok
     end
   end