Update ActionController::Parameters to be more secure on parameters handling
#16299
Conversation
| @@ -1,3 +1,33 @@ | |||
| * `ActionController::Parameters` will stop inheriting from `Hash` and | |||
| `HashWithIndifferentAccess` in the next major release. If you use any method | |||
| that does not available on `ActionController::Parameters` you should | |||
Is it worth us providing a method that does |
|
@matthewd I was thinking about that but didn't propose with this patch. I may submit another PR for the |
| @@ -141,6 +141,26 @@ def initialize(attributes = nil) | |||
| @permitted = self.class.permit_all_parameters | |||
| end | |||
There was a problem hiding this comment.
Could pass permitted to the constructor rather than introducing the new_instance_with_inherited_permitted_status wrapper:
def initialize(attributes = nil, permitted = self.class.permit_all_parameters)
super attributes
@permitted = permitted
endThere was a problem hiding this comment.
Nevermind. Looking at options to clean up the repeated "copy subset of myself" behavior, but adding semi-private internals to the public constructor isn't a great direction.
|
People usually doesn't read CHANGELOGs or upgrade guides. They will only notice that their call Hash specific methods doesn't work anymore when upgrading to Rails 5. Also, there is still some methods that may not work as expected on the current implementation that may create security issues. I still think we should do something with these methods that are form Hash/Enum API and not from the Parameters API. Maybe we should show a deprecation warning with a security note on it. |
|
@jeremy the code has been updated. I removed the documentation for getting unsafe hash, and maybe introducing a method to get that based on @matthewd's comment unless you think YAGNI. @rafaelfranca CHANGELOG and release note are there for a reason. If they don't read it, well, too bad. Well documented change will benefit those who seek to read it. What are the methods that you have a concern in? Can I fix them to make them work as expected in this PR or in a separate PR? For the deprecation warning, I don't think there's a good clean way to do it, unless you're knowing something that I don't know. I'd rather just make those methods that we know they're available works. Then, if we want to remove them or deprecate them for 5.0, we can do so. |
All bangs methods
I'd prefer to do this work in the same PR. |
|
As we discussed in Campfire, it seems like we will want to address a security concern about this as well. I've come up with a solution to this, which is that we're going to mark the params to be unsafe again when the safe (permitted) parameters hash has been changed. People will then has to mark the params to be safe again after they mutate them, or convert them to a hash and opt-out from parameters whitelisting at their own risk. However, this will not blocking this PR to be merged, and it will be addressed in another PR. |
`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.
This is to make sure that `permitted` status is maintained on the resulting object. I found these methods that needs to be redefined by looking for `self.class.new` in the code. * extract! * transform_keys * transform_values
* `each` * `each_pair` * `delete` * `select!`
Ruby 1.9.3 does not implement Hash#to_h, so we can't call `super` on it.
Update `ActionController::Parameters` to be more secure on parameters handling
As suggested in rails#16299([1]), this method should be a new public API for retrieving unfiltered parameters from `ActionController::Parameters` object, given that `Parameters#to_hash` will no longer work in Rails 5.0+ as we stop inheriting `Parameters` from `Hash`. [1]: rails#16299 (comment)
As discussed in #16299[1], this attribute is not thread safe and could potentially create a security issue. [1]: #16299 (comment)
As suggested in rails#16299([1]), this method should be a new public API for retrieving unfiltered parameters from `ActionController::Parameters` object, given that `Parameters#to_hash` will no longer work in Rails 5.0+ as we stop inheriting `Parameters` from `Hash`. [1]: rails#16299 (comment)
As discussed in rails#16299[1], this attribute is not thread safe and could potentially create a security issue. [1]: rails#16299 (comment)
As suggested in rails#16299([1]), this method should be a new public API for retrieving unfiltered parameters from `ActionController::Parameters` object, given that `Parameters#to_hash` will no longer work in Rails 5.0+ as we stop inheriting `Parameters` from `Hash`. [1]: rails#16299 (comment)
This is the follow-up from #14384
I've fixed the code according to the recommendation, which is to go smarter while not breaking much compatibility.
So, these are the proposed changes:
#to_honAC::Parameterswill strip out unpermitted key. What this means is that if you call#to_honAC::Parametersthat's not permitted you'll get an empty hash. If the user wants to get the hash with all the keys, they're advised to duplicate the hash and call#permit!on it.AC::Parametersnow have more accessor and mutator methods defined on it. These methods are methods that the resulting object didn't have correct permitted status on it.I've adding a relevant changelog, and also mentioning that we'll move away from inheriting
AC::Paramsfrom Hash on the next major release./cc @jeremy, @tenderlove