Improve the upgrade path of Strong Parameters #28734

rafaelfranca · 2017-04-12T02:39:31Z

Some changes were made in this PR to improve the upgrade path of Strong Parameters not inheriting from hash anymore.

Raise exception when calling `to_h` in an unpermitted `Parameters`

Before we returned either an empty hash or only the always permitted parameters (:controller and :action by default).

The previous behavior was dangerous because in order to get the attributes users usually fallback to use to_unsafe_h that could potentially introduce security issues.

The to_unsafe_h API is also not good since Parameters is a object that quacks like a Hash but not in all cases since to_h would return an empty hash and users were forced to check if to_unsafe_h is defined or if the instance is a ActionController::Parameters in order to work with it. This end up coupling a lot of libraries and parts of the application with something that is from the controller layer.

Add ActionController::Parameters#to_hash to implicit conversion

Now methods that implicit convert objects to a hash will be able to work without requiring the users to change their implementation.

This method will return a Hash instead of a HashWithIndefirentAccess to mimic the same implementation of HashWithIndefirentAccess#to_hash.

Implement ActionController::Parameters#to_query and #to_param

Previously it was raising an error because it may be unsafe to use those methods in a unpermitted parameter. Now we delegate to to_h that already raise an error when the Parameters instance is not permitted.

This also fix a bug when using #to_query in a hash that contains a ActionController::Parameters instance and was returning the name of the class in the string.

Backport

The plan is to backport this to 5.1 and part of it to 5.0.

Backport to 5.0

The plan is to backport the first and the last change fully.

The #to_hash change I want to just undeprecate it for now since it is already calling to_hash in the internal array and the result includes all the parameters in the case were they are not permitted.

Also implement respond_to_missing since we have a method_missing.

cc @maartenvg @nicolaslupien @trevorturk

kirs · 2017-04-12T13:55:21Z

I'm really excited to see to_h raising an error if it's not permitted. That would save hours of work when upgrading Shopify to strong parameters.

I have concerns about backporting the last commit to 5.0. The change makes sense in the context of to_h raising an error, but that won't be the case for 5.0. Do I miss the point?

rafaelfranca · 2017-04-12T14:40:41Z

The plan is to to_h raise an error in 5.0 (so backporting the first and last change). An error is better than silently raising making the Parameters empty since now users can know their application will break.

trevorturk · 2017-04-12T17:20:26Z

I agree the error is good -- it's much better than silently not working. Good job!

matthewd · 2017-04-12T19:52:11Z

actionpack/lib/action_controller/metal/strong_parameters.rb

+  #   # => ActionController::ForbiddenParameters: converting a unpermitted parameters to hash is not allowed
+  class ForbiddenParameters < StandardError
+    def initialize
+      super("converting an unpermitted parameters to hash is not allowed")


"unable to convert unpermitted parameters to hash", maybe? "not allowed" sounds like we're being difficult.

matthewd · 2017-04-12T19:53:26Z

actionpack/lib/action_controller/metal/strong_parameters.rb

+  #   params = ActionController::Parameters.new(a: "123", b: "456")
+  #   params.to_h
+  #   # => ActionController::ForbiddenParameters: converting a unpermitted parameters to hash is not allowed
+  class ForbiddenParameters < StandardError


Forbidden doesn't sound quite right.. how about UnfilteredParameters?

matthewd · 2017-04-12T19:57:13Z

actionpack/lib/action_controller/metal/strong_parameters.rb

@@ -257,7 +312,7 @@ def to_h
    #   params.to_unsafe_h
    #   # => {"name"=>"Senjougahara Hitagi", "oddity" => "Heavy stone crab"}
    def to_unsafe_h
-      convert_parameters_to_hashes(@parameters, :to_unsafe_h)
+      permit!.to_h


I don't think this should mutate the original params object.. otherwise params.to_unsafe_h in one place that's being careful some other way could leave a later params.to_h exposed to danger.

oh. That is right. I was wondering why we implemented like that and now I know it. We are missing some test coverage about this case. I'll add. Do you have preference between dup the paramters and mutate it or the implementation like it was before?

If it's just a simple dup that seems fine.. if it would need to be a deep dup, I think it's probably worth the effort of the more complicated version.

kirs · 2017-04-13T01:22:43Z

The plan is to to_h raise an error in 5.0

That's quite a breaking change to push to stable version. As a developer I would be extremely surprised by this introduced between 5.0.2 and 5.0.3. This will also make the upgrade to 5.0.3 harder, resulting to lower adoption rate.

At least this should be a config variable, with raise disabled by default.

ghost · 2017-04-13T13:02:54Z

actionpack/lib/action_controller/metal/strong_parameters.rb

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


This will make our code cleaner, thanks! 👍

matthewd · 2017-04-13T13:03:38Z

@kirs that's a good point.. but on the other hand, is anyone going to be knowingly using the current behaviour? It seems a rather long-winded way to spell {}.

The low-impact alternative would be to introduce a warning, and optionally offer the raise as a non-default config option. I'm dubious that the exception would cause people problems (as opposed to identifying places they didn't realise were silently eating input), but as 5.0 is fairly mature at this point -- and about to become legacy -- maybe we should go with the more conservative option anyway.

I'm not sure whether this should go through the deprecation infrastructure (gives people more control etc, even though it's not strictly a deprecation), or straight to Kernel#warn.

rafaelfranca · 2017-04-13T17:28:31Z

@kirs that's a good point.. but on the other hand, is anyone going to be knowingly using the current behaviour? It seems a rather long-winded way to spell {}.

Yeah... if people is getting this exception after upgrading to 5.0.3 it means that they had a bug that they didn't anticipated in their code and in my opinion an exception is better than leaving the bug hidden.

We can go in the deprecation path just to be safe with an option to raise on this deprecation.

trevorturk · 2017-04-13T17:32:41Z

Yes the deprecation path might be best, like we have new_rails_defaults etc. I think raising would have been a better choice to start, but I agree at this point it may be surprising to see new exceptions when you upgrade, so opt-in would be ideal.

rafaelfranca · 2017-04-13T18:05:42Z

Fixed all the points for this branch and 5.1. I'll open a new PR with the changes to 5.0

maclover7

Left a few comments about the docs :)

maclover7 · 2017-04-13T18:20:44Z

actionpack/lib/action_controller/metal/strong_parameters.rb

      end
    end

+    # Returns a safe <tt>Hash</tt> representation of this parameter


the parameters

maclover7 · 2017-04-13T18:21:43Z

actionpack/lib/action_controller/metal/strong_parameters.rb

+    #
+    #   params = ActionController::Parameters.new({
+    #     name: 'David',
+    #     nationality: 'Danish'})


maybe have }) be on a new line, like above?

maclover7 · 2017-04-13T18:21:58Z

actionpack/lib/action_controller/metal/strong_parameters.rb

+    #
+    #   params = ActionController::Parameters.new({
+    #     name: 'David',
+    #     nationality: 'Danish'})


same comment -- maybe have }) be on a new line, like above?

kirs · 2017-04-13T18:40:40Z

actionpack/lib/action_controller/metal/strong_parameters.rb

+  #   # => ActionController::UnfilteredParameters: unable to convert unpermitted parameters to hash
+  class UnfilteredParameters < StandardError
+    def initialize
+      super("unable to convert unpermitted parameters to hash")


It would be friendly to the developer to suggest how they're supposed to fix the error (by using permit)

An exception don't need to explain what users have to do. It depends on the context and since we don't know what is the context we can't recommend something. But, it needs to be clear about what it means. Is the exception message and documentation clear about what it means?

+1 exceptions should clearly state what's wrong, but leave speculation about how the developer might fix it to someone/something else

rafaelfranca · 2017-04-13T20:16:36Z

@maclover7 thanks! Done

kaspth

Like this a lot! 👍

kaspth · 2017-04-15T13:54:55Z

actionpack/lib/action_controller/metal/strong_parameters.rb

@@ -43,6 +43,18 @@ def initialize(params) # :nodoc:
    end
  end

+  # Raised when a Parameters instance is not marked as permitted and
+  # an operation to transform to hash is called.


"to transform it to a hash."

kaspth · 2017-04-15T13:56:15Z

actionpack/lib/action_controller/metal/strong_parameters.rb

    #
    #   safe_params = params.permit(:name)
    #   safe_params.to_h # => {"name"=>"Senjougahara Hitagi"}
    def to_h
      if permitted?
        convert_parameters_to_hashes(@parameters, :to_h)
      else
-        slice(*self.class.always_permitted_parameters).permit!.to_h


How do we honor always_permitted_parameters now? Should it be deprecated?

It is still needed when you permit let say [:user][:name] in a controller and the action_on_unpermitted_parameters is either :log or :raise. that way :controller and :action will not raise.

Ah, it's handled in unpermitted_parameters!. Never mind 😊

kaspth · 2017-04-15T14:00:40Z

actionpack/lib/action_controller/metal/strong_parameters.rb

+    #     name: 'David',
+    #     nationality: 'Danish'
+    #   })
+    #   params.to_query('user')


Should the new documentation here use double quoted strings and no hash braces on the new call to fit our Rubocop rules?

kaspth · 2017-04-15T14:02:48Z

actionpack/test/controller/parameters/parameters_permit_test.rb

-    assert_equal({ "controller" => "users", "action" => "create" }, params.to_h)
+      assert_instance_of Hash, params.to_hash
+      assert_not_kind_of ActionController::Parameters, params.to_hash
+      assert_equal({ "crab" => "Senjougahara Hitagi" }, params.to_hash)


Wouldn't Ruby implicitly call to_hash on the params here? Should we test an implicit case?

kaspth

Had one last comment, otherwise 👍

Before we returned either an empty hash or only the always permitted parameters (:controller and :action by default). The previous behavior was dangerous because in order to get the attributes users usually fallback to use to_unsafe_h that could potentially introduce security issues. The to_unsafe_h API is also not good since Parameters is a object that quacks like a Hash but not in all cases since to_h would return an empty hash and users were forced to check if to_unsafe_h is defined or if the instance is a ActionController::Parameters in order to work with it. This end up coupling a lot of libraries and parts of the application with something that is from the controller layer.

Now methods that implicit convert objects to a hash will be able to work without requiring the users to change their implementation. This method will return a Hash instead of a HashWithIndefirentAccess to mimic the same implementation of HashWithIndefirentAccess#to_hash.

Previously it was raising an error because it may be unsafe to use those methods in a unpermitted parameter. Now we delegate to to_h that already raise an error when the Parameters instance is not permitted. This also fix a bug when using `#to_query` in a hash that contains a `ActionController::Parameters` instance and was returning the name of the class in the string.

We are talking about a list of parameters even so we need to use plural. Even if we were talking about the instance of the Parameters object we would have to use the capital and monospaced font.

matthewd · 2017-04-18T21:37:22Z

There's another stand-alone instance of the existing message in

rails/actionview/lib/action_view/helpers/url_helper.rb

Lines 625 to 626 in a2ab5ff

    
           raise ArgumentError, "Attempting to generate a button from non-sanitized request parameters!" \ 
        
             " Whitelist and sanitize passed parameters to be secure."

rafaelfranca · 2017-04-18T21:49:37Z

Good catch. Fixed

Since this protection is now in Parameters we can use it instead of reimplementing again.

Improve the upgrade path of Strong Parameters

rafaelfranca · 2017-04-19T22:04:29Z

Backported to 5-1-stable in a217fd0.

PR for 5-0-stable #28803.

Improve the upgrade path of Strong Parameters

rafaelfranca requested a review from matthewd April 12, 2017 02:39

rafaelfranca added the actionpack label Apr 12, 2017

deivid-rodriguez mentioned this pull request Apr 12, 2017

simplify ActiveAdmin::Filters::Active activeadmin/activeadmin#4906

Merged

matthewd reviewed Apr 12, 2017

View reviewed changes

ghost reviewed Apr 13, 2017

View reviewed changes

rafaelfranca force-pushed the strong-parameters branch from 56481d7 to a4f8d89 Compare April 13, 2017 18:04

maclover7 suggested changes Apr 13, 2017

View reviewed changes

kirs reviewed Apr 13, 2017

View reviewed changes

rafaelfranca force-pushed the strong-parameters branch from a4f8d89 to 8d3bf94 Compare April 13, 2017 20:15

maclover7 approved these changes Apr 14, 2017

View reviewed changes

kaspth reviewed Apr 15, 2017

View reviewed changes

rafaelfranca force-pushed the strong-parameters branch from 8d3bf94 to 41c7fec Compare April 17, 2017 22:31

kaspth approved these changes Apr 18, 2017

View reviewed changes

rafaelfranca added 8 commits April 18, 2017 17:33

Test the correct object

1396b05

Add test to make sure that to_unsafe_h don't mutate the target

e13e72c

Use the right assetions to better error messages

af87815

Improve documentation

3ee56f7

We are talking about a list of parameters even so we need to use plural. Even if we were talking about the instance of the Parameters object we would have to use the capital and monospaced font.

Follow the style guide rules in the documetation

70c6fb1

rafaelfranca force-pushed the strong-parameters branch from a4b11a6 to 1d5ea4a Compare April 18, 2017 21:33

rafaelfranca force-pushed the strong-parameters branch from 1d5ea4a to 06923ca Compare April 18, 2017 21:49

Reuse the Parameters#to_h check in the routing helpers

93034ad

Since this protection is now in Parameters we can use it instead of reimplementing again.

rafaelfranca force-pushed the strong-parameters branch from 06923ca to 93034ad Compare April 18, 2017 22:57

rafaelfranca merged commit 3d1154f into rails:master Apr 19, 2017

rafaelfranca added a commit that referenced this pull request Apr 19, 2017

Merge pull request #28734 from rafaelfranca/strong-parameters

a217fd0

Improve the upgrade path of Strong Parameters

rafaelfranca mentioned this pull request Apr 19, 2017

Improve the upgrade path of Strong Parameters in 5-0-stable #28803

Merged

rafaelfranca deleted the strong-parameters branch April 19, 2017 22:04

This was referenced Apr 20, 2017

to_param method being deprecated gives warnings when params passed in a path helper #26958

Closed

make path helper work with ActionController::Parameters #27176

Closed

rafaelfranca added a commit to rafaelfranca/omg-rails that referenced this pull request Apr 20, 2017

Merge pull request rails#28734 from rafaelfranca/strong-parameters

458d8d2

Improve the upgrade path of Strong Parameters

esb mentioned this pull request Nov 27, 2017

Rails 5 Strong Parameters removes hash access for unpermitted params Sology/smart_listing#134

Open

This was referenced Oct 21, 2018

Strong params deprecated with double splat operators #26201

Closed

Restore strong params to_hash behavior #26203

Closed

vpereira mentioned this pull request Dec 11, 2018

Move prjconf actions from project_controller to its own controller openSUSE/open-build-service#6537

Merged

brendanshean mentioned this pull request Nov 2, 2022

Add Confidence level, time spent, and completion rate to Evidence Activity Stats empirical-org/Empirical-Core#9919

Merged

Improve the upgrade path of Strong Parameters #28734

Improve the upgrade path of Strong Parameters #28734

Conversation

rafaelfranca commented Apr 12, 2017

Raise exception when calling to_h in an unpermitted Parameters

Add ActionController::Parameters#to_hash to implicit conversion

Implement ActionController::Parameters#to_query and #to_param

Backport

Backport to 5.0

kirs commented Apr 12, 2017

rafaelfranca commented Apr 12, 2017

trevorturk commented Apr 12, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kirs commented Apr 13, 2017

Choose a reason for hiding this comment

matthewd commented Apr 13, 2017

rafaelfranca commented Apr 13, 2017

trevorturk commented Apr 13, 2017

rafaelfranca commented Apr 13, 2017

maclover7 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rafaelfranca commented Apr 13, 2017

kaspth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kaspth left a comment

Choose a reason for hiding this comment

matthewd commented Apr 18, 2017

rafaelfranca commented Apr 18, 2017

rafaelfranca commented Apr 19, 2017

Raise exception when calling `to_h` in an unpermitted `Parameters`