Make AC::Parameters not inherited from Hash #20868

Merged
merged 2 commits into from Jul 15, 2015

Projects

None yet

9 participants

@sikachu
Member
sikachu commented Jul 13, 2015

This is another take at #14384 as we decided to wait until master is targeting Rails 5.0. This commit is implementation-complete, as it guarantees that all the public methods on the hash-inherited Parameters
are still working (based on test case). We can decide to follow-up later if we want to remove some methods out from Parameters.

@rafaelfranca rafaelfranca commented on the diff Jul 13, 2015
actionpack/test/controller/api/params_wrapper_test.rb
@@ -7,7 +7,7 @@ class UsersController < ActionController::API
wrap_parameters :person, format: [:json]
def test
- self.last_parameters = params.except(:controller, :action)
+ self.last_parameters = params.except(:controller, :action).to_unsafe_h
@rafaelfranca
rafaelfranca Jul 13, 2015 Member

Do we have a Parameters#to_h?

@sikachu
sikachu Jul 13, 2015 Member

Yes, but because of #18006 to_h only returns safe parameters (after slicing) while to_unsafe_h returns everything as-is.

Since this test is testing all parameters and doesn't care about Strong Parameters, to_unsafe_h works fine here.

@rafaelfranca
rafaelfranca Jul 13, 2015 Member

Yeah, that is that I thought ๐Ÿ‘

@rafaelfranca rafaelfranca commented on the diff Jul 13, 2015
actionpack/lib/action_dispatch/routing/url_for.rb
@@ -171,6 +171,10 @@ def url_for(options = nil)
route_name = options.delete :use_route
_routes.url_for(options.symbolize_keys.reverse_merge!(url_options),
route_name)
+ when ActionController::Parameters
@rafaelfranca
rafaelfranca Jul 13, 2015 Member

In fact, I think we should not allow parameters instance directly on url_for. See #20797. I'll work with @byroot to finish that so we can remove all this code.

@sikachu
sikachu Jul 14, 2015 Member

Sounds good. There actually are tests that failed if I didn't modify this line, though.

I'll let you decide if you want to get #20797 in first and have me rebase it or not.

@rafaelfranca
Member

Seems for some reason `MyModel.create(params[:my_model]) is raising an error.

@sikachu
Member
sikachu commented Jul 14, 2015

@rafaelfranca yeah, I took care of it. Seems like it expects hash-like object to define stringify_keys.

@robin850 robin850 commented on an outdated diff Jul 14, 2015
...pack/lib/action_controller/metal/strong_parameters.rb
@permitted = self.class.permit_all_parameters
end
+ def ==(other_hash)
+ p other_hash
@robin850
robin850 Jul 14, 2015 Member

It looks like a debugging remain. :-p

@robin850 robin850 commented on an outdated diff Jul 14, 2015
...pack/lib/action_controller/metal/strong_parameters.rb
end
end
+ def transform_values!(&block)
+ @parameters.transform_values!(&block)
+ self
+ 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
@robin850
robin850 Jul 14, 2015 Member

Nit-picky thing but this is no longer true.

@sikachu
Member
sikachu commented Jul 14, 2015

Somehow Travis lost track of the PR, but the build against the latest commit passed. Please give it another look.

@rafaelfranca rafaelfranca reopened this Jul 14, 2015
@rafaelfranca
Member

Travis is crazy lately. Just close and reopen that it run again.
cc @joshk

@joshk
Contributor
joshk commented Jul 14, 2015

Hey Hey, whats up?

@rafaelfranca
Member

@joshk after some force pushes to PR branches Travis lost the tracking of
the PR and stops to report the status and running the builds

On Tue, Jul 14, 2015, 17:02 Josh Kalderimis notifications@github.com
wrote:

Hey Hey, whats up?

โ€”
Reply to this email directly or view it on GitHub
#20868 (comment).

@sikachu
Member
sikachu commented Jul 15, 2015

(Feedback was on sikachu@8b80ada)

Documentation updated. Thanks @robin850!

@sikachu sikachu closed this Jul 15, 2015
@sikachu sikachu reopened this Jul 15, 2015
@tenderlove tenderlove commented on the diff Jul 15, 2015
...pack/lib/action_controller/metal/strong_parameters.rb
@@ -144,11 +146,22 @@ def self.const_missing(const_name)
# params = ActionController::Parameters.new(name: 'Francesco')
# params.permitted? # => true
# Person.new(params) # => #<Person id: nil, name: "Francesco">
- def initialize(attributes = nil)
- super(attributes)
+ def initialize(parameters = {})
+ @parameters = parameters.with_indifferent_access
@tenderlove
tenderlove Jul 15, 2015 Member

Does this need to convert all incoming hashes to HWIAs? Or can we change initialize to:

def initialize(parameters = {}.with_indifferent_access)
  @parameters = parameters

I want to make sure the constructor is doing as little work as possible.

@sikachu
sikachu Jul 15, 2015 Member

I tried to change the constructor to not calling .with_indifferent_access but that totally broke the tests. I think it's better to keep it like that to make sure that params we are dealing with is HWIA. I feel like we'll run into weird issue if we don't do that.

@kaspth kaspth commented on an outdated diff Jul 15, 2015
actionpack/CHANGELOG.md
@@ -1,3 +1,17 @@
+* `ActionController::Parameters` no longer inherits from
+ `HashWithIndifferentAccess`
+
+ Inheriting from `HashWithIndifferentAccess` allowed users to call any
+ enumerable methods on `Parameters` object, resulting in a risk of losing the
+ `permitted?` status or even getting back a pure `Hash` object instead of
+ a `Parameters` object with proper sanitization.
+
+ By stop inheriting from `HashWithIndifferentAccess`, we are able to make
@kaspth
kaspth Jul 15, 2015 Member

By not ๐Ÿ˜„

@joshk
Contributor
joshk commented Jul 15, 2015

hey @rafaelfranca, it looks like the most recent commit got picked up, or was this again the case of a 'close/open'?

@sikachu
Member
sikachu commented Jul 15, 2015

@joshk I did a close/open. I'm about to force-push to the branch, so I'll ping you once I do that.

@sikachu sikachu Make AC::Parameters not inherited from Hash
This is another take at #14384 as we decided to wait until `master` is
targeting Rails 5.0. This commit is implementation-complete, as it
guarantees that all the public methods on the hash-inherited Parameters
are still working (based on test case). We can decide to follow-up later
if we want to remove some methods out from Parameters.
14a3bd5
@sikachu
Member
sikachu commented Jul 15, 2015

@joshk I just pushed sikachu@39a7fcb and https://travis-ci.org/rails/rails/pull_requests still shows the old SHA for this PR. There might be a problem with PR update hook?

@joshk
Contributor
joshk commented Jul 15, 2015

I think I know the issue, I'll get back to you though.

@joshk
Contributor
joshk commented Jul 15, 2015

@sikachu can you update the PR again?

@sikachu
Member
sikachu commented Jul 15, 2015

@joshk just did. I see new build on https://travis-ci.org/rails/rails/pull_requests but it still does not update the PR to show that there's a pending build.

@joshk
Contributor
joshk commented Jul 15, 2015

So, the odd thing is, our logs show it is being set correctly, so I am not sure if this is an issue with GitHub or with us. Is this happening with just this PR, or all PRs?

@kaspth
Member
kaspth commented Jul 15, 2015

@josk All PRs ๐Ÿ˜„

@sikachu
Member
sikachu commented Jul 15, 2015

@joshk are you on the IRC? I can hop in and we could chat and trying to debug this.

BTW, are you positive that Travis send the status to GitHub for 84b861f (the actual commit) and not 47621a75f42f5e544175442bc2e3438134a0fc7a (the psudo-merge commit)?

@joshk
Contributor
joshk commented Jul 15, 2015

@sikachu we send it for the commit, i mean, this does work for everyone apart from Rails, so I don't think it is how we are talking to GitHub. Can you email support@travis-ci.com and we can invite you to our Slack room.

@rafaelfranca rafaelfranca merged commit 4f21269 into rails:master Jul 15, 2015
@sikachu sikachu deleted the sikachu:params-not-inherited-from-hwia branch Jul 15, 2015
@kaspth
Member
kaspth commented Jul 15, 2015

@sikachu woot! โค๏ธ ๐Ÿ’› ๐Ÿ’š ๐Ÿ’œ ๐Ÿ’™

Also big hearts to you @joshk for doing live customer support. What Travis offers open source projects is incredible and especially what you have to do for Rails. Thanks โค๏ธ ๐Ÿ’› ๐Ÿ’š ๐Ÿ’œ ๐Ÿ’™

@sobrinho

Isn't that a possible DoS on ruby versions before symbols are garbage collected?

Member

Not sure, but master is targeting Ruby 2.2+ so we should be fine?

Contributor

Yeap, 2.2 is the version which started to collect, thanks! :)

http://www.infoq.com/news/2014/12/ruby-2.2.0-released

@eileencodes eileencodes added a commit to eileencodes/actionpack-xml_parser that referenced this pull request Aug 26, 2015
@eileencodes eileencodes Make AC::Parameters not inherited from Hash
Following rails/rails#20868 changed how
ActionController::Parameters worked.

This change makes `dump_param_keys` match how the Rails side reads teh
hash keys.

Without this change the right keys don't make it to `Request::Utils` so
the test `test_dasherized_keys_as_xml` was failing because `sub-key`
wasn't being recognized as a key.
57618a9
@eileencodes eileencodes referenced this pull request in rails/actionpack-xml_parser Aug 26, 2015
Merged

Update for Rails 5 #12

@eileencodes eileencodes added a commit to eileencodes/actionpack-xml_parser that referenced this pull request Aug 26, 2015
@eileencodes eileencodes Make AC::Parameters not inherited from Hash
Following rails/rails#20868 changed how
ActionController::Parameters worked.

This change makes `dump_param_keys` match how the Rails side reads teh
hash keys.

Without this change the right keys don't make it to `Request::Utils` so
the test `test_dasherized_keys_as_xml` was failing because `sub-key`
wasn't being recognized as a key.
740080a
@eileencodes eileencodes added a commit to eileencodes/actionpack-xml_parser that referenced this pull request Aug 26, 2015
@eileencodes eileencodes Make AC::Parameters not inherited from Hash
Following rails/rails#20868 changed how
ActionController::Parameters worked.

This change makes `dump_param_keys` match how the Rails side reads teh
hash keys.

Without this change the right keys don't make it to `Request::Utils` so
the test `test_dasherized_keys_as_xml` was failing because `sub-key`
wasn't being recognized as a key.
c54f89b
@eileencodes eileencodes added a commit to eileencodes/actionpack-xml_parser that referenced this pull request Aug 26, 2015
@eileencodes eileencodes Make AC::Parameters not inherited from Hash
Following rails/rails#20868 changed how
ActionController::Parameters worked.

This change makes `dump_param_keys` match how the Rails side reads teh
hash keys.

Without this change the right keys don't make it to `Request::Utils` so
the test `test_dasherized_keys_as_xml` was failing because `sub-key`
wasn't being recognized as a key.
ca0d9ce
@claudiob claudiob added a commit to claudiob/rails that referenced this pull request Sep 10, 2015
@claudiob claudiob Remove wrong doc line about AC::Parameters
AC::Parameters does not inherit from HashWithIndifferentAccess
since #20868 by @sikachu
82239a9
@trevorturk
Contributor

Hi all! ๐Ÿ‘‹

I've been struggling with this change while upgrading an app to Rails 5 that doesn't use Active Record and I'm wondering if anyone thought about making this change opt-in via new_framework_defaults or some other setting.

My use-case is a bit strange, and I'm happy to talk about it in detail if anyone is interested. Basically, I'm using Action Pack and talking to an API that accepts JSON hashes instead of Active Record. So, for example, I have a controller that looks something like this:

class ThingsController < ApplicationController
  def update
    Remote::Service.update_thing(params[:thing])
  end
end

Changing params to not inherit from ActiveSupport::HashWithIndifferentAccess means I have to call to_unsafe_hash in lots of places where it wasn't needed before, and in some cases I've needed to build complex permit structures. It's definitely the most difficult part of the Rails 5 upgrade so far.

My colleague @rf- has been trying to figure out a way to opt-out of strong parameters entirely, and the closest we've come is with this without_modules trick https://github.com/rails/rails/blob/master/actionpack/lib/action_controller/base.rb#L195 but that's causing other issues and making us nervous.

We'll report back if/when we find a workaround, but I thought I should raise the issue to see if anyone else had thoughts. I absolutely loved having the new_framework_defaults options for this upgrade and I thought this might be another place to consider using that pattern.

@rafaelfranca
Member

Why do you need to call to_unsafe_h in a bunch of places? Which problem is that change is causing to your application?

@rafaelfranca
Member

Also, even if we had a new_framework_defaults config that config would be removed in Rails 5.1 so you would still to change your application.

@trevorturk
Contributor

Oooh I didn't realize new_framework_defaults would only be good for a point release -- I have some more work to do, then!

The to_unsafe_h method is needed because this application passes in params directly to a Ruby lib which uses HTTParty to send JSON to another internal service. The params being sent end up looking like thing=%23%3CActionController%3A%3AParameters%3A0x007fb836b78c20%3E as opposed to thing[one]=value&thing[two]=other.

In the example from my last comment, imagine that you don't know what keys params[:thing] might contain. So, you can't call to_h on it or it'll drop all of the unpermitted keys. I can use to_unsafe_h or .permit!.to_h to allow arbitrary keys, but then I still need to find all of the places in the app where the params being passed into the Ruby lib might be a hash (which not usually obvious in my case.)

I suppose what I might be inquiring about, then, is what the best method might be for disabling strong parameters for this application entirely. It looked like that without_modules trick might work, but I'm running into seemingly unrelated issues trying to use it. I'll give that a proper try with fresh eyes tomorrow. I also might be able to sidestep this issue entirely by adjusting the Ruby lib that uses HTTParty to do the params transformation itself, instead of relying on Rails params to be a hash, and then I won't have to worry about this change at all.

Apologies for the long comments here, but I suspect I'm one of the first people trying to upgrade an app to Rails 5 that truly expects params to be a hash. I'm hoping I'll be able to provide some helpful hints, documentation, or release notes that will help other people that run into similar issues after me.

@rafaelfranca
Member

I had similar problems with this is shopify too. What we end up doing was to change the lib that do the http to actually transform the parameter in hash. There are some places in the Rails itself that we do the same like url_for helper. We also removed to_param from Parameter object to make it explicit.

I'm open to suggestions to make this change easier but until now I didn't find anything better that stop letting parameter instances enter the model layer doing explicit calls to to_h. Maybe a method in active model would help?

@matthewd
Member

It is indeed generally intended that we force you to be explicit if you mean to take an arbitrary hash of user input and pass it to other code: too often controller methods are written believing params[:foo] is a relatively innocent scalar, and replacing it with a hash can lead the called method to do something unexpected.

That said, if you really want to dangerzone: without trying it, I'd expect your controller could just def params; super.to_unsafe_h; end (or some more memoized variation thereof).


The fact HTTParty doesn't see the (deprecated) to_hash is a bug, because we're missing a respond_to_missing? -- but if it were working (and you hadn't called permit / permit!), you'd just get an empty querystring instead of #<ActionController::Parameters:0x007fb836b78c20>... which is not exactly more useful.

@trevorturk
Contributor

Ah, thanks for the additional info @matthewd. We were thinking of doing something along these lines:

class ApplicationController < ActionController::Base
    def params
    @_params ||= request.parameters
  end

  def params=(val)
    @_params = val
  end
end

...which would bypass strong params insofar as we use them by reverting to the ActionController::Metal definitions, but I also like your suggestion to use super.to_unsafe_h. Can you think of any reason to prefer one method over the other? I suppose you could use request.parameters instead of params as a rule, but I suspect params would creep back into the codebase over time, so overriding params one way or another probably makes sense.

@rafaelfranca I was feeling negative about this change when I first encountered it, but after reading through the GitHub issues and comments etc I think I understand the reasoning, and I appreciate all of the work that has gone into making our Rails apps as secure as possible. As @matthewd said, it's so easy for people to send unexpected params that we need all the protection we can get, I'm sure.

As for the best path forward in our application, I'm not sure if it's best to alter our HTTParty-based lib to do the hash transformation or to bypass strong params for the Rails app. I suppose if the app will never use Active Record then we're safe to bypass strong params. If we might use Active Record in the future, however, then I suppose our http lib should be altered instead.

@rafaelfranca I'm not sure what you have in mind re: a method in active model, but I'm happy to discuss and/or test out ideas if you have any.

I'm also not sure what to suggest about the overall issue re: the HTTParty bug @matthewd pointed out. As @jeremy mentioned a long time ago #14384 (comment) params has been a hash for a long time, and I expect other people will encounter similar issues going forward, especially when using third-party libs.

I suppose HTTParty et al might be able to add support for "Rails 5 params" via respond_to or such, but currently I think the suggested change would be for people to adjust their applications to call to_unsafe_hash on params before sending them along to http libs, or to create a wrapper around the lib that does the transformation automagically? (Or to bypass strong params as we're thinking of doing.) If there are any additional ideas, it might make sense for me to work up a short section to add to the Rails Guides for future people upgrading that describes the potential issue and the suggested workaround(s)?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment