Merge pull request #16299 from sikachu/ps-safer-ac-params

Update `ActionController::Parameters` to be more secure on parameters handling

5 files changed, 382 insertions, 39 deletions

diff --git a/actionpack/CHANGELOG.md b/actionpack/CHANGELOG.md
index 1c84bac3ff..e2731d0ee5 100644
--- a/actionpack/CHANGELOG.md
+++ b/actionpack/CHANGELOG.md
@@ -1,3 +1,38 @@
+*   `ActionController::Parameters` will stop inheriting from `Hash` and
+    `HashWithIndifferentAccess` in the next major release. If you use any method
+    that is not available on `ActionController::Parameters` you should consider
+    calling `#to_h` to convert it to a `Hash` first before calling that method.
+
+    *Prem Sichanugrist*
+
+*   `ActionController::Parameters#to_h` now returns a `Hash` with unpermitted
+    keys removed. This change is to reflect on a security concern where some
+    method performed on an `ActionController::Parameters` may yield a `Hash`
+    object which does not maintain `permitted?` status. If you would like to
+    get a `Hash` with all the keys intact, duplicate and mark it as permitted
+    before calling `#to_h`.
+
+        params = ActionController::Parameters.new({
+          name: 'Senjougahara Hitagi',
+          oddity: 'Heavy stone crab'
+        })
+        params.to_h
+        # => {}
+
+        unsafe_params = params.dup.permit!
+        unsafe_params.to_h
+        # => {"name"=>"Senjougahara Hitagi", "oddity"=>"Heavy stone crab"}
+
+        safe_params = params.permit(:name)
+        safe_params.to_h
+        # => {"name"=>"Senjougahara Hitagi"}
+
+    This change is consider a stopgap as we cannot change the code to stop
+    `ActionController::Parameters` to inherit from `HashWithIndifferentAccess`
+    in the next minor release.
+
+    *Prem Sichanugrist*
+
 *   Deprecated TagAssertions.
 
     *Kasper Timm Hansen*
diff --git a/actionpack/lib/action_controller/metal/strong_parameters.rb b/actionpack/lib/action_controller/metal/strong_parameters.rb
index bc27ecaa20..7038f0997f 100644
--- a/actionpack/lib/action_controller/metal/strong_parameters.rb
+++ b/actionpack/lib/action_controller/metal/strong_parameters.rb
@@ -141,6 +141,37 @@ module ActionController
       @permitted = self.class.permit_all_parameters
     end
 
+    # Returns a safe +Hash+ representation of this parameter with all
+    # unpermitted keys removed.
+    #
+    #   params = ActionController::Parameters.new({
+    #     name: 'Senjougahara Hitagi',
+    #     oddity: 'Heavy stone crab'
+    #   })
+    #   params.to_h # => {}
+    #
+    #   safe_params = params.permit(:name)
+    #   safe_params.to_h # => {"name"=>"Senjougahara Hitagi"}
+    def to_h
+      if permitted?
+        to_hash
+      else
+        slice(*self.class.always_permitted_parameters).permit!.to_h
+      end
+    end
+
+    # 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)
+      end
+
+      super
+    end
+
+    alias_method :each, :each_pair
+
     # Attribute that keeps track of converted arrays, if any, to avoid double
     # looping in the common use case permit + mass-assignment. Defined in a
     # method to instantiate it only if needed.
@@ -176,7 +207,6 @@ module ActionController
     #   Person.new(params) # => #<Person id: nil, name: "Francesco">
     def permit!
       each_pair do |key, value|
-        value = convert_hashes_to_parameters(key, value)
         Array.wrap(value).each do |v|
           v.permit! if v.respond_to? :permit!
         end
@@ -331,11 +361,56 @@ module ActionController
     #   params.slice(:a, :b) # => {"a"=>1, "b"=>2}
     #   params.slice(:d)     # => {}
     def slice(*keys)
-      self.class.new(super).tap do |new_instance|
-        new_instance.permitted = @permitted
+      new_instance_with_inherited_permitted_status(super)
+    end
+
+    # Removes and returns the key/value pairs matching the given keys.
+    #
+    #   params = ActionController::Parameters.new(a: 1, b: 2, c: 3)
+    #   params.extract!(:a, :b) # => {"a"=>1, "b"=>2}
+    #   params                  # => {"c"=>3}
+    def extract!(*keys)
+      new_instance_with_inherited_permitted_status(super)
+    end
+
+    # Returns a new <tt>ActionController::Parameters</tt> with the results of
+    # running +block+ once for every value. The keys are unchanged.
+    #
+    #   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)
+      else
+        super
+      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)
+      else
+        super
       end
     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)
+    end
+
+    # Equivalent to Hash#keep_if, but returns nil if no changes were made.
+    def select!(&block)
+      convert_value_to_parameters(super)
+    end
+
     # Returns an exact copy of the <tt>ActionController::Parameters</tt>
     # instance. +permitted+ state is kept on the duped object.
     #
@@ -356,6 +431,12 @@ module ActionController
       end
 
     private
+      def new_instance_with_inherited_permitted_status(hash)
+        self.class.new(hash).tap do |new_instance|
+          new_instance.permitted = @permitted
+        end
+      end
+
       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)
diff --git a/actionpack/test/controller/parameters/accessors_test.rb b/actionpack/test/controller/parameters/accessors_test.rb
new file mode 100644
index 0000000000..97875c3cbb
--- /dev/null
+++ b/actionpack/test/controller/parameters/accessors_test.rb
@@ -0,0 +1,125 @@
+require 'abstract_unit'
+require 'action_controller/metal/strong_parameters'
+require 'active_support/core_ext/hash/transform_values'
+
+class ParametersAccessorsTest < ActiveSupport::TestCase
+  setup do
+    @params = ActionController::Parameters.new(
+      person: {
+        age: '32',
+        name: {
+          first: 'David',
+          last: 'Heinemeier Hansson'
+        },
+        addresses: [{city: 'Chicago', state: 'Illinois'}]
+      }
+    )
+  end
+
+  test "[] retains permitted status" do
+    @params.permit!
+    assert @params[:person].permitted?
+    assert @params[:person][:name].permitted?
+  end
+
+  test "[] retains unpermitted status" do
+    assert_not @params[:person].permitted?
+    assert_not @params[:person][:name].permitted?
+  end
+
+  test "each carries permitted status" do
+    @params.permit!
+    @params.each { |key, value| assert(value.permitted?) if key == "person" }
+  end
+
+  test "each carries unpermitted status" do
+    @params.each { |key, value| assert_not(value.permitted?) if key == "person" }
+  end
+
+  test "each_pair carries permitted status" do
+    @params.permit!
+    @params.each_pair { |key, value| assert(value.permitted?) if key == "person" }
+  end
+
+  test "each_pair carries unpermitted status" do
+    @params.each_pair { |key, value| assert_not(value.permitted?) if key == "person" }
+  end
+
+  test "except retains permitted status" do
+    @params.permit!
+    assert @params.except(:person).permitted?
+    assert @params[:person].except(:name).permitted?
+  end
+
+  test "except retains unpermitted status" do
+    assert_not @params.except(:person).permitted?
+    assert_not @params[:person].except(:name).permitted?
+  end
+
+  test "fetch retains permitted status" do
+    @params.permit!
+    assert @params.fetch(:person).permitted?
+    assert @params[:person].fetch(:name).permitted?
+  end
+
+  test "fetch retains unpermitted status" do
+    assert_not @params.fetch(:person).permitted?
+    assert_not @params[:person].fetch(:name).permitted?
+  end
+
+  test "reject retains permitted status" do
+    assert_not @params.reject { |k| k == "person" }.permitted?
+  end
+
+  test "reject retains unpermitted status" do
+    @params.permit!
+    assert @params.reject { |k| k == "person" }.permitted?
+  end
+
+  test "select retains permitted status" do
+    @params.permit!
+    assert @params.select { |k| k == "person" }.permitted?
+  end
+
+  test "select retains unpermitted status" do
+    assert_not @params.select { |k| k == "person" }.permitted?
+  end
+
+  test "slice retains permitted status" do
+    @params.permit!
+    assert @params.slice(:person).permitted?
+  end
+
+  test "slice retains unpermitted status" do
+    assert_not @params.slice(:person).permitted?
+  end
+
+  test "transform_keys retains permitted status" do
+    @params.permit!
+    assert @params.transform_keys { |k| k }.permitted?
+  end
+
+  test "transform_keys retains unpermitted status" do
+    assert_not @params.transform_keys { |k| k }.permitted?
+  end
+
+  test "transform_values retains permitted status" do
+    @params.permit!
+    assert @params.transform_values { |v| v }.permitted?
+  end
+
+  test "transform_values retains unpermitted status" do
+    assert_not @params.transform_values { |v| v }.permitted?
+  end
+
+  test "values_at retains permitted status" do
+    @params.permit!
+    assert @params.values_at(:person).first.permitted?
+    assert @params[:person].values_at(:name).first.permitted?
+  end
+
+  test "values_at retains unpermitted status" do
+    assert_not @params.values_at(:person).first.permitted?
+    assert_not @params[:person].values_at(:name).first.permitted?
+  end
+end
diff --git a/actionpack/test/controller/parameters/mutators_test.rb b/actionpack/test/controller/parameters/mutators_test.rb
new file mode 100644
index 0000000000..744d8664be
--- /dev/null
+++ b/actionpack/test/controller/parameters/mutators_test.rb
@@ -0,0 +1,99 @@
+require 'abstract_unit'
+require 'action_controller/metal/strong_parameters'
+require 'active_support/core_ext/hash/transform_values'
+
+class ParametersMutatorsTest < ActiveSupport::TestCase
+  setup do
+    @params = ActionController::Parameters.new(
+      person: {
+        age: '32',
+        name: {
+          first: 'David',
+          last: 'Heinemeier Hansson'
+        },
+        addresses: [{city: 'Chicago', state: 'Illinois'}]
+      }
+    )
+  end
+
+  test "delete retains permitted status" do
+    @params.permit!
+    assert @params.delete(:person).permitted?
+  end
+
+  test "delete retains unpermitted status" do
+    assert_not @params.delete(:person).permitted?
+  end
+
+  test "delete_if retains permitted status" do
+    @params.permit!
+    assert @params.delete_if { |k| k == "person" }.permitted?
+  end
+
+  test "delete_if retains unpermitted status" do
+    assert_not @params.delete_if { |k| k == "person" }.permitted?
+  end
+
+  test "extract! retains permitted status" do
+    @params.permit!
+    assert @params.extract!(:person).permitted?
+  end
+
+  test "extract! retains unpermitted status" do
+    assert_not @params.extract!(:person).permitted?
+  end
+
+  test "keep_if retains permitted status" do
+    @params.permit!
+    assert @params.keep_if { |k,v| k == "person" }.permitted?
+  end
+
+  test "keep_if retains unpermitted status" do
+    assert_not @params.keep_if { |k,v| k == "person" }.permitted?
+  end
+
+  test "reject! retains permitted status" do
+    @params.permit!
+    assert @params.reject! { |k| k == "person" }.permitted?
+  end
+
+  test "reject! retains unpermitted status" do
+    assert_not @params.reject! { |k| k == "person" }.permitted?
+  end
+
+  test "select! retains permitted status" do
+    @params.permit!
+    assert @params.select! { |k| k != "person" }.permitted?
+  end
+
+  test "select! retains unpermitted status" do
+    assert_not @params.select! { |k| k != "person" }.permitted?
+  end
+
+  test "slice! retains permitted status" do
+    @params.permit!
+    assert @params.slice!(:person).permitted?
+  end
+
+  test "slice! retains unpermitted status" do
+    assert_not @params.slice!(:person).permitted?
+  end
+
+  test "transform_keys! retains permitted status" do
+    @params.permit!
+    assert @params.transform_keys! { |k| k }.permitted?
+  end
+
+  test "transform_keys! retains unpermitted status" do
+    assert_not @params.transform_keys! { |k| k }.permitted?
+  end
+
+  test "transform_values! retains permitted status" do
+    @params.permit!
+    assert @params.transform_values! { |v| v }.permitted?
+  end
+
+  test "transform_values! retains unpermitted status" do
+    assert_not @params.transform_values! { |v| v }.permitted?
+  end
+end
diff --git a/actionpack/test/controller/parameters/parameters_permit_test.rb b/actionpack/test/controller/parameters/parameters_permit_test.rb
index aa894ffa17..ba98ad7605 100644
--- a/actionpack/test/controller/parameters/parameters_permit_test.rb
+++ b/actionpack/test/controller/parameters/parameters_permit_test.rb
@@ -194,42 +194,6 @@ class ParametersPermitTest < ActiveSupport::TestCase
     assert_equal "monkey", @params.fetch(:foo) { "monkey" }
   end
 
-  test "not permitted is sticky on accessors" do
-    assert !@params.slice(:person).permitted?
-    assert !@params[:person][:name].permitted?
-    assert !@params[:person].except(:name).permitted?
-
-    @params.each { |key, value| assert(!value.permitted?) if key == "person" }
-
-    assert !@params.fetch(:person).permitted?
-
-    assert !@params.values_at(:person).first.permitted?
-  end
-
-  test "permitted is sticky on accessors" do
-    @params.permit!
-    assert @params.slice(:person).permitted?
-    assert @params[:person][:name].permitted?
-    assert @params[:person].except(:name).permitted?
-
-    @params.each { |key, value| assert(value.permitted?) if key == "person" }
-
-    assert @params.fetch(:person).permitted?
-
-    assert @params.values_at(:person).first.permitted?
-  end
-
-  test "not permitted is sticky on mutators" do
-    assert !@params.delete_if { |k| k == "person" }.permitted?
-    assert !@params.keep_if { |k,v| k == "person" }.permitted?
-  end
-
-  test "permitted is sticky on mutators" do
-    @params.permit!
-    assert @params.delete_if { |k| k == "person" }.permitted?
-    assert @params.keep_if { |k,v| k == "person" }.permitted?
-  end
-
   test "not permitted is sticky beyond merges" do
     assert !@params.merge(a: "b").permitted?
   end
@@ -277,4 +241,43 @@ class ParametersPermitTest < ActiveSupport::TestCase
   test "permitting parameters as an array" do
     assert_equal "32", @params[:person].permit([ :age ])[:age]
   end
+
+  test "to_h returns empty hash on unpermitted params" do
+    assert @params.to_h.is_a? Hash
+    assert_not @params.to_h.is_a? ActionController::Parameters
+    assert @params.to_h.empty?
+  end
+
+  test "to_h returns converted hash on permitted params" do
+    @params.permit!
+
+    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
+    begin
+      ActionController::Parameters.permit_all_parameters = true
+      params = ActionController::Parameters.new(crab: "Senjougahara Hitagi")
+
+      assert params.to_h.is_a? Hash
+      assert_not @params.to_h.is_a? ActionController::Parameters
+      assert_equal({ "crab" => "Senjougahara Hitagi" }, params.to_h)
+    ensure
+      ActionController::Parameters.permit_all_parameters = false
+    end
+  end
+
+  test "to_h returns always permitted parameter on unpermitted params" do
+    params = ActionController::Parameters.new(
+      controller: "users",
+      action: "create",
+      user: {
+        name: "Sengoku Nadeko"
+      }
+    )
+
+    assert_equal({ "controller" => "users", "action" => "create" }, params.to_h)
+  end
 end