Make `AC::Params#to_h` return Hash with safe keys

`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')
    params.to_h # => {}

    unsafe_params = params.dup.permit!
    unsafe_params.to_h # => {"name"=>"Senjougahara Hitagi"}

    safe_params = params.permit(:name)
    safe_params.to_h # => {"name"=>"Senjougahara Hitagi"}

This change is consider a stopgap as we cannot chage the code to stop
`ActionController::Parameters` to inherit from
`HashWithIndifferentAccess` in the next minor release.

Also, adding a CHANGELOG entry to mention that
`ActionController::Parameters` will not inheriting from
`HashWithIndifferentAccess` in the next major version.

3 files changed, 93 insertions, 0 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..764474ad4e 100644
--- a/actionpack/lib/action_controller/metal/strong_parameters.rb
+++ b/actionpack/lib/action_controller/metal/strong_parameters.rb
@@ -141,6 +141,25 @@ 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?
+        super
+      else
+        slice(*self.class.always_permitted_parameters).permit!.to_h
+      end
+    end
+
     # 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.
diff --git a/actionpack/test/controller/parameters/parameters_permit_test.rb b/actionpack/test/controller/parameters/parameters_permit_test.rb
index aa894ffa17..c8cc654afd 100644
--- a/actionpack/test/controller/parameters/parameters_permit_test.rb
+++ b/actionpack/test/controller/parameters/parameters_permit_test.rb
@@ -277,4 +277,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