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

rafaelfranca · 2017-04-19T22:03:54Z

Similar to #28734. The differences are:

to_h and to_hash only raises when config.action_controller.raise_on_unpermitted_parameters is true (default to false in existing application and true in new applications).
The places where it was already raising are not reusing the to_h implementation.

matthewd · 2017-04-20T02:54:54Z

actionpack/lib/action_controller/metal/strong_parameters.rb

  #   params[:key]  # => "value"
  #   params["key"] # => "value"
  class Parameters
    cattr_accessor :permit_all_parameters, instance_accessor: false
    cattr_accessor :action_on_unpermitted_parameters, instance_accessor: false
+    cattr_accessor :raise_on_unpermitted_parameters, instance_accessor: false


Should this be unfiltered? As is, it sounds confusingly overlapped with action_on_unpermitted_parameters.

Do we need to define & deprecate this config value in 5.1, for people who upgrade from a 5.0 version that includes it?

Yeah. I was going to do that as soon we decide a name for the config

Yeah. I'll change to unfiltered.

matthewd · 2017-04-20T03:00:27Z

actionpack/lib/action_controller/metal/strong_parameters.rb

+      if self.class.raise_on_unpermitted_parameters
+        to_h.to_hash
+      else
+        @parameters.to_hash


I'm tempted to make this do something different (e.g., always to_h.to_hash).

My worry here is that this is not what it's currently/previously been doing in 5.0: the deprecation warning is going away, and respond_to?(:to_hash) is becoming true. That latter part seems particularly significant.

I have hoping that the upgrade path to those who are already seeing the deprecation of to_hash would be easier if we keep the same behavior when the configuration is disabled. Always doing to_h.to_hash would return an empty hash now if the config is false.

Yeah.. I actually meant "what to_h.to_hash would do if the config were enabled", but that [by design] isn't compatible either.

At minimum I think we should keep a deprecation here: we can't deprecate to_h with the config disabled, because we can't deprecate in a point release... but we can deprecate to_hash with the config disabled, because it's already showing a deprecation warning: we'd just be changing the message and thus the advice (to "[read about and then] turn on this config").

I'm still worried that even with that, as soon as respond_to?(:to_hash) becomes true, a lot of code will start treating this object differently (calling to_hash where it currently does not). Maybe we should special-case :to_hash in respond_to?, and lie about it when the config setting is off?

Which reminds me.. maybe as part of this PR, or maybe separately, we should fix (implement) respond_to_missing?.

matthewd · 2017-04-20T03:03:08Z

...es/lib/rails/generators/rails/app/templates/config/initializers/new_framework_defaults.rb.tt

@@ -7,6 +7,8 @@
 #
 <%- end -%>
 # Read the Guide for Upgrading Ruby on Rails for more info on each option.
+
+Rails.application.config.raise_on_unpermitted_parameters = <%= options[:update] ? false : true %>


I think this should be true for update too. This template is for upgrading 4.2 applications: we're only worried about keeping the existing behaviour for applications already on 5.0.

oh, that is true

Could you explain again why this should be true when running rails app:update right after upgrading from Rails 4.2?
From the comment above, that means I am supposed to flip it to false after I have made sure it works with false. But that doesn't make sense.

matthewd · 2017-04-20T03:04:05Z

actionpack/CHANGELOG.md

+    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


Extra backtick

rafaelfranca · 2017-04-20T17:46:31Z

Updated

kaspth

Some minor comments, but overall great 😊

kaspth · 2017-04-20T19:25:33Z

actionpack/CHANGELOG.md

+    (`: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.


Stray backtick wraps "that`"

kaspth · 2017-04-20T19:27:24Z

actionpack/CHANGELOG.md

+    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`.


Why don't we expand this to ActiveSupport::HashWithIndifferentAccess since Action Controller isn't the namespace here?

kaspth · 2017-04-20T19:30:38Z

actionpack/CHANGELOG.md

+    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


I'm curious if we call symbolize_keys in to_hash would strong parameters basically work as keyword arguments? E.g. being able to do this would be sweet:

def hello(a:, d:) end hello(ActionController::Parameters.new(a: :c, d: :c)) hello(params) # …or more likely.

(And I think it would also fix the warts in the new resolve and direct API around parameters.)

kaspth · 2017-04-20T19:31:40Z

actionpack/CHANGELOG.md

+    This method will return a `Hash` instead of a `HashWithIndefirentAccess` to mimic the
+    same implementation of `HashWithIndefirentAccess#to_hash`.
+
+    This method will raise on unpermitted Parameters if


Should we say unfiltered here as well?

kaspth · 2017-04-20T19:36:13Z

actionpack/lib/action_controller/metal/strong_parameters.rb

@@ -718,7 +795,15 @@ def method_missing(method_sym, *args, &block)
      end
    end

-    # Returns duplicate of object including all parameters
+    def respond_to?(name, include_all = false) # :nodoc:


Should this be respond_to_missing??

It can't be respond_to_missing? because to_hash is implemented and respond_to? would return true without calling respond_to_missing?

matthewd · 2017-04-20T21:07:15Z

actionpack/lib/action_controller/metal/strong_parameters.rb

+          a security vulnerability in your app that can be exploited. Instead,
+          consider enabling `config.action_controller.raise_on_unfiltered_parameters` and filtering
+          the `ActionController::Parameters`.
+          DEPRECATE


to_hash unexpectedly ignores parameter filtering, and will change to enforce it in Rails 5.1. Enable raise_on_unfiltered_parameters to respect parameter filtering, which is the default in new applications. For the existing deprecated behaviour, call to_unsafe_h instead.

Maybe? The "will be removed" phrasing seems particularly awkward when it's not really going away, per se.

matthewd · 2017-04-20T21:08:10Z

actionpack/lib/action_controller/metal/strong_parameters.rb

+      if self.class.raise_on_unfiltered_parameters
+        to_h.to_hash
+      else
+        unless permitted?


I think we should keep the message regardless of whether it's permitted?. It was there before, and using a non-filtering to_hash is arguably worse if you think you've applied filtering.

rafaelfranca · 2017-04-20T21:57:51Z

Updated

Improve the upgrade path of Strong Parameters

rafaelfranca requested review from matthewd and kaspth April 19, 2017 22:03

rafaelfranca mentioned this pull request Apr 19, 2017

Improve the upgrade path of Strong Parameters #28734

Merged

rafaelfranca self-assigned this Apr 19, 2017

rafaelfranca added the actionpack label Apr 19, 2017

matthewd reviewed Apr 20, 2017

View reviewed changes

rafaelfranca force-pushed the 5-0-params branch from c88c596 to 7f45a1a Compare April 20, 2017 17:46

kaspth approved these changes Apr 20, 2017

View reviewed changes

matthewd reviewed Apr 20, 2017

View reviewed changes

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

458d8d2

Improve the upgrade path of Strong Parameters

rafaelfranca force-pushed the 5-0-params branch from 7f45a1a to 458d8d2 Compare April 20, 2017 21:59

rafaelfranca merged commit cb62320 into rails:5-0-stable Apr 21, 2017

rafaelfranca deleted the 5-0-params branch April 21, 2017 00:50

deeeki mentioned this pull request Jun 22, 2017

Fix raise_on_unfiltered_parameters configuration for 5-0-stable #29527

Closed

rafaelfranca restored the 5-0-params branch September 11, 2020 21:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

rafaelfranca commented Apr 19, 2017

matthewd Apr 20, 2017

matthewd Apr 20, 2017

rafaelfranca Apr 20, 2017

rafaelfranca Apr 20, 2017

matthewd Apr 20, 2017

rafaelfranca Apr 20, 2017

matthewd Apr 20, 2017

matthewd Apr 20, 2017

matthewd Apr 20, 2017

rafaelfranca Apr 20, 2017

runephilosof Dec 6, 2017

matthewd Apr 20, 2017

rafaelfranca commented Apr 20, 2017

kaspth left a comment

kaspth Apr 20, 2017

kaspth Apr 20, 2017

kaspth Apr 20, 2017 •

edited

kaspth Apr 20, 2017

kaspth Apr 20, 2017

rafaelfranca Apr 20, 2017

matthewd Apr 20, 2017

matthewd Apr 20, 2017

rafaelfranca commented Apr 20, 2017

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

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

Conversation

rafaelfranca commented Apr 19, 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

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 20, 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

kaspth Apr 20, 2017 • edited

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 20, 2017

kaspth Apr 20, 2017 •

edited