Make AC::Parameters not inherited from Hash

[sikachu]

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]

Do we have a Parameters#to_h?

[sikachu]

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]

Yeah, that is that I thought 👍

[rafaelfranca]

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

[sikachu]

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

[robin850]

It looks like a debugging remain. :-p

[sikachu]

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

[rafaelfranca]

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

[joshk]

Hey Hey, whats up?

[rafaelfranca]

@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]

(Feedback was on sikachu@8b80ada)

Documentation updated. Thanks @robin850!

[tenderlove]

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]

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.

[joshk]

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

[sikachu]

@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]

@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]

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

[trevorturk]

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]

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]

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]

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]

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]

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]

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)?