AC::Parameters doesn't inherit from Hash #14384

Closed
wants to merge 5 commits into
from

Conversation

Projects
None yet
8 participants
Member

sikachu commented Mar 14, 2014

Inheriting from HashWithIndifferentAccess allowed programmers to call enumerable methods over AC::Parameters, losing @permitted state, and rendering Strong Parameters a barebones Hash. This changeset reduces AC::Parameters interface, disallowing such operations.

@sikachu sikachu added this to the 4.2.0 milestone Mar 14, 2014

@@ -167,7 +167,7 @@ class DefaultUrlOptionsCachingController < ActionController::Base
before_filter { @dynamic_opt = 'opt' }
def test_url_options_reset
- render text: url_for(params)
+ render text: url_for(params.to_h)
@rafaelfranca

rafaelfranca Mar 14, 2014

Owner

I think this should not be required, or we will give people problem when upgrading. I think we should check Parameters on url_for

@@ -39,7 +39,7 @@ def render_body
end
def test_params
- render :text => params.inspect
+ render :text => params.to_h.inspect
@rafaelfranca

rafaelfranca Mar 14, 2014

Owner

WDYT about overriding inspect?

@tute

tute Mar 14, 2014

Contributor

We tried it but it was tricky during testing: it seemed like the hashes differed only in the order of their entries but they are actually entirely different objects.

@tute

tute Mar 14, 2014

Contributor

Overriding inspect is the last change to hide the fact that Parameters is no longer really a Hash, although very similar. I guess the question is how far is it best to go down that road.

@rafaelfranca

rafaelfranca Mar 14, 2014

Owner

I see. It is not a big deal.

@jeremy

jeremy Mar 24, 2014

Owner

It'll be surprising, though, and hard to read. People use params.inspect in logs, exception emails, tests, etc. In some cases, other code would expect to parse that data (e.g. for log or exception analysis).

@rafaelfranca rafaelfranca self-assigned this Mar 14, 2014

+ rendering Strong Parameters a barebones Hash. This changeset reduces
+ `ActionController::Parameters` interface, disallowing such operations.
+
+ *Prem Sichanugrist and Tute Costa*
@carlosantoniodasilva

carlosantoniodasilva Mar 14, 2014

Owner

Make sure to add ` to things like HWID and AC::Parameters.

@sikachu

sikachu Mar 19, 2014

Member

Good catch. Done.

Contributor

thedarkone commented Mar 15, 2014

I'm used to "defensibly parsing" params by checking that some nested value is a Hash like this:

if (nested_foo = params[:foo]).kind_of?(Hash)
  do_something! if nested_foo[:bar] == 'DO!'
end

This will no longer work, am I right?

Contributor

tute commented Mar 15, 2014

@thedarkone that's right, can check doing .respond_to? :[].

Contributor

thedarkone commented Mar 15, 2014

@tute strings (a common occurrence in params) also respond to :[].

Damn this is annoying, params used to be a Hash since from forever. I bet this will subtly break a lot of apps.

Contributor

tute commented Mar 15, 2014

Frankly I agree. It's also east to take care of allowed parameters in the controller with Hash#slice, it seems to me.

Owner

rafaelfranca commented Mar 15, 2014

The main reason to implement this is security, we know there are explicit checks for Hash class and it was implemented this way forever but I believe implementing this will level up our security of parameters filtering.

I know it may break some application but it is very easy to fix, users only need to check Parameters instead Hash.

Other way to fix this issue is reimplement every possible Hash method, or undefine some methods we know are problematics, but this is too fragile since every new version of Ruby we will have to check if Hash class changed and reimplement in our side.

If you have any other suggestion please let us know.

Owner

jeremy commented Mar 15, 2014

It's an easy fix, unless it's in an engine or library that your application uses. Then you update the library to check for Parameters too, which breaks the lib compatibility with older Rails (no such constant).

Another way is to proxy Parameters#missing_method to the composed HWIA instance, along with a deprecation warning that params will no longer be a Hash in Rails 5.

+1 to the motivation here (security) and implementation, but we need a compromise implementation to give users a very gentle upgrade path. What @thedarkone said can't be overstated: params have always been a Hash. It's in deep.

Member

sikachu commented Mar 17, 2014

I think is_a? test is too fragile. If you want to test if an object is a Hash or hash-like, you should do something like:

if object.respond_to?(:to_hash)
  do_something_with object.to_hash
end

Testing an object via duck-typing is more reliable than by class. (For String, you'd be checking for to_str — same thing.)

I think for 4.x I'll override is_a? which prints deprecation warning if they try to check if this is a Hash. I think that's a good compromise and make it not breaking, until we reaches 5.0.

+
+ def to_h
+ @parameters
+ end
@tenderlove

tenderlove Mar 18, 2014

Owner

Are these used frequently? The implementation should probably dup before returning because someone could just do:

parameters.to_h.delete_if { trollface? }

Then the collection is mutated out from under the Parameters instance.

@sikachu

sikachu Mar 18, 2014

Member

Good catch. I was curious as well if I should dup the hash or not. 👍

@@ -122,7 +133,7 @@ class Parameters < ActiveSupport::HashWithIndifferentAccess
# params.permitted? # => true
# Person.new(params) # => #<Person id: nil, name: "Francesco">
def initialize(attributes = nil)
@tenderlove

tenderlove Mar 18, 2014

Owner
  1. Why does attributes default to nil? Do people expect the underlying collection to behave like a nil? It seems like it should behave like a hash, so the default should be {} or a HIWA.
  2. Why don't we add a second permitted parameter to the constructor that defaults to self.class.permit_all_parameters? This would simplify new_with_parameters and you can remove the protected permitted= method.
@sikachu

sikachu Mar 18, 2014

Member
  1. I like default as HIWA. Will change that.
  2. mmm I like that idea. Good call. 👍
Member

sikachu commented Mar 19, 2014

@tenderlove I responded to your feedback and also the additional ones that we discussed in Campfire. How does it look now?

Owner

tenderlove commented Mar 19, 2014

Looks good. Should we add a deprecation warning in the is_a checks?

Aaron Patterson
http://tenderlovemaking.com/
I'm on an iPhone so I apologize for top posting.

On Mar 19, 2014, at 11:50 AM, Prem Sichanugrist notifications@github.com wrote:

@tenderlove I responded to your feedback and also the additional ones that we discussed in Campfire. How does it look now?


Reply to this email directly or view it on GitHub.

+ # status. You should be careful when passing the resulting hash to a model
+ # as they may still contains parameters that is not permitted.
+ def to_h
+ @parameters.dup
@rafaelfranca

rafaelfranca Mar 19, 2014

Owner

Since we have nested hashes on parameters should we call deep_dup?

@sikachu

sikachu Mar 19, 2014

Member

Hash does a shallow dup (since it doesn't implement #dup itself). I'm not really sure if it does matter that much if we do shallow dup here. I can try to write a test case though.

@tenderlove do you have any suggestion?

@tenderlove

tenderlove Mar 21, 2014

Owner

I think a shallow dup is Good Enough™. We shouldn't go overboard protecting folks.

@jeremy

jeremy Mar 24, 2014

Owner

Why dup at all then?

Member

sikachu commented Mar 19, 2014

@tenderlove I tried with this:

def is_a?(klass)
  if klass == Hash
    ActiveSupport::Deprecation.warn '...'
  end

  super || @parameters.is_a?(klass)
end

and I got showered with deprecation warning everywhere. I didn't think it's worth fixing every framework files to silence that warning, so I removed the warning. Am I missing something?

+ if @parameters.respond_to? method_name, include_private
+ ActiveSupport::Deprecation.warn <<-warning.squish
+ ActionController::Parameters is no longer inherited from `HashWithIndifferentAccess` due
+ to a security concern. Your `##{method_name}` method call will be proxied to the
@tute

tute Mar 21, 2014

Contributor

method call was instead of method call will be, to match with following lost its permitted status.

@sikachu

sikachu Mar 21, 2014

Member

It's other way around. It has to be future tense since respond_to_missing? is the one responding when you do respond_to?; You're actually not calling the method yet.

Member

sikachu commented Mar 21, 2014

@tenderlove I added the deprecation warning to is_a? check. Seems like it was only small issue than I was anticipated.

Can you take a look and merge this if you think it's good?

Prem Sichanugrist and Tute Costa and others added some commits Mar 14, 2014

Stop inherit `AC::Parameters` from `HWIA`
Inheriting from `HashWithIndifferentAccess` allowed programmers to call
enumerable methods over `AC::Parameters`, losing `@permitted` state, and
rendering Strong Parameters a barebones `Hash`. This changeset reduces
`AC::Parameters` interface, disallowing such operations.
Make `Parameters` looks like a `Hash`
* `is_a?(Hash)` returns true.
* `kind_of?(Hash)` returns true.
* All missing methods are proxied to underlying
  `HashWithIndifferentAccess` with deprecation warning.

Note that this commit can be reverted once we reach the next major
release.
Unpublicized `Parameters#converted_arrays`
This method is only use internally for memoization.
Add deprecation on `Parameters.is_a?(Hash)`
We have to override `is_a?` and `kind_of` to make sure that it doesn't
break if people are testing `Parameters.is_a?(Hash)` or
`Parameters.kind_of?(Hash)`. However, since this change will be reverted
in the next major version, we should display the warning message.
Owner

tenderlove commented Mar 23, 2014

I'm good with this. :-)

@jeremy is this OK to land on master?

+ @parameters.dup
+ end
+
+ alias_method :to_hash, :to_h
@jeremy

jeremy Mar 24, 2014

Owner

YAGNI to_hash

+ # params.keep_if { |k,v| v > 2 } # => {"c"=>3}
+ def keep_if(&block)
+ new_with_parameters(@parameters.keep_if(&block))
+ end
@jeremy

jeremy Mar 24, 2014

Owner

Why are we choosing to expose these in particular? I haven't reached for them before when working with params.

@sikachu

sikachu Mar 24, 2014

Member

It is being used in the test case:

test "not permitted is sticky on mutators" do
assert !@params.delete_if { |k| k == "person" }.permitted?
assert !@params.keep_if { |k,v| k == "person" }.permitted?
end

What should I do with them?

@jeremy

jeremy Mar 24, 2014

Owner

Note that delete_if and keep_if modify the object in-place—they don't return a new instance. Hence, they worked fine before, since permitted status was preserved.

The brokenness is from methods that return new instances only. HWIA has had to deal with this situation... extensively.

+ ActionController::Parameters is no longer inherited from `HashWithIndifferentAccess` due to
+ a security concern. Currently, `is_a?` is overridden to return true on `Hash` for backward
+ compatibility. You shouldn't rely on this behavior, as it will be removed in the next
+ version of Rails. We recommend you to do `respond_to?(:to_hash)` instead.
@jeremy

jeremy Mar 24, 2014

Owner

Ruby has a convention of using to_<letter> for explicit coercion and to_<abbreviation> to indicate implicit coercion. If an object responds to to_str, then it is a string and can be treated as such.

Parameters is not a Hash and doesn't pass the implicit coercion test, so it should not implement to_hash or recommend duck-typing against it.

@sikachu

sikachu Mar 24, 2014

Member

Right ... I remembered that convention. I was doing that because I'm not sure how much we're breaking people's app. If so, how about:

  • Remove to_hash + Update this to suggest user to do to_h instead.
  • Add deprecation warning that to_hash will be removed.

Which one do you think I should do?

@jeremy

jeremy Mar 24, 2014

Owner

to_h is explicit coercion that's implemented by many other classes that aren't params-like, so that won't work.

Considering we're offering up Parameters with a different kind of API for working with request params, we should warn ppl to start using that API instead, then rely on explicit coercion when they pass params to other libraries that expect to work with hashes.

Of course, this is a huge pain for everyone to deal with. "Uhh so I have to use params.to_h everywhere now??" We could inadvertently introduce a worse security situation, where users spam .to_h to "fix" problems.

@sikachu

sikachu Mar 24, 2014

Member

Right. So, in order to make sure that we're as close to Hash as possible, I'm leaning toward having to_hash on 4.x, then remove it in 5.0. I know that it's risky, but since this is a minor release, I'd want it to break the least number of apps possible.

Or, do you think it would make more sense to keep inheriting from Hash, with deprecation warning on risky operations that will override permitted status, then wait until 5.0 to pull the plug?

I'm not sure what would be the best solution though, as we're pretty much breaking the compatibility while trying to maintain it. 😄

@jeremy

jeremy Mar 24, 2014

Owner

I suggested some options in a comment on the main PR.

Implementation-wise, we're clearly better off composing our internal data structure rather than subclassing it. However, that overlooks the larger concern of how users will interact with this, and whether it will make Rails hard to understand, or even less secure.

+ # method to instantiate it only if needed.
+ def converted_arrays
+ @converted_arrays ||= Set.new
+ end
@jeremy

jeremy Mar 24, 2014

Owner

Changed method visibility from public -> private intentionally? (Odd that this was public anyway.)

@sikachu

sikachu Mar 24, 2014

Member

Yes. @tenderlove were curious why it was public in the first place, and change it to private did not break anything.

+ ActionController::Parameters is no longer inherited from `HashWithIndifferentAccess` due
+ to a security concern. Your `##{method_name}` method call will be proxied to the
+ underlying `HashWithIndifferentAccess` object and lost its permitted status. This behavior
+ will be removed in the next major version of Rails.
@jeremy

jeremy Mar 24, 2014

Owner

This message is hard to parse.

  1. Warning!
  2. You called a Hash method on params that loses track of which params were permitted. Uh oh.
  3. Yeah that's a bug, and a security concern! So what do I do!!
  4. To fix this and make this annoying message go away, stop using method_name on your params.
  5. It'll stop working in the next Rails release, to boot!
@sikachu

sikachu Mar 24, 2014

Member

Any suggestion on how should I word it is much appreciated 😄

(or, you can push changes to this afterward, up to you.)

@jeremy

jeremy Mar 24, 2014

Owner

Example:

You called a Hash method on params that loses track of which params were permitted. That's a security concern since you lose strong_params protection. To fix this, stop using params.#{method_name}. Rely on ActionController::Parameters methods only.

@sikachu

sikachu Mar 24, 2014

Member

Ah, I see what you mean. I'll update the warning text. Thanks!

@@ -543,7 +652,7 @@ def params
# is a Hash, this will create an ActionController::Parameters
# object that has been instantiated with the given +value+ hash.
def params=(value)
- @_params = value.is_a?(Hash) ? Parameters.new(value) : value
+ @_params = value.respond_to?(:to_hash) ? Parameters.new(value.to_hash) : value
@jeremy

jeremy Mar 24, 2014

Owner

What other values do we allow assigning to .params= anyway? This seems fishy (the original code as well). Would expect the inverse:

def params=(value)
  @_params = value.is_a?(Parameters) ? value : Parameters.new(value)
end
@sikachu

sikachu Mar 24, 2014

Member

Agreed. Good catch; I'll change it.

- when Hash
- _routes.url_for(options.symbolize_keys.reverse_merge!(url_options))
+ when Hash, ActionController::Parameters
+ _routes.url_for(options.to_hash.symbolize_keys.reverse_merge!(url_options))
@jeremy

jeremy Mar 24, 2014

Owner

Feels like we're missing a proper duck type. Reminds me of Ruby introducing to_path. We need a to_params to lean on.

@jeremy

jeremy Mar 24, 2014

Owner

@sikachu @tenderlove fwiw I'm not convinced we need to_params either. It's just clear that to_hash are not the droids we're looking for

+class ParametersCompatibilityTest < ActiveSupport::TestCase
+ def test_calling_undefined_method_proxy_to_hash_with_deprecation
+ assert_deprecated do
+ params = ActionController::Parameters.new('crab' => 'Senjougahara', 'snail' => 'Hachikuji')
@jeremy

jeremy Mar 24, 2014

Owner

Don't expect this line to result in a deprecation warning, so it needn't live within the block.

+
+ def test_respond_to_on_non_existance_method_proxy_to_hash_with_deprecation
+ assert_deprecated do
+ params = ActionController::Parameters.new
@jeremy

jeremy Mar 24, 2014

Owner

Ditto, Parameters.new is not deprecated.

+ ActionController::Parameters.new.kind_of?(String)
+ end
+ end
+end
@jeremy

jeremy Mar 24, 2014

Owner

Ditto for the rest. Would make sense to @params = ActionController::Parameters.new in setup and rely on that instead.

@sikachu

sikachu Mar 24, 2014

Member

Ah, right, let me update the test to fix that.

+ assert_not_deprecated do
+ ActionController::Parameters.new.kind_of?(String)
+ end
+ end
@jeremy

jeremy Mar 24, 2014

Owner

Understood why we'd deprecate is_a? Hash and kind_of? Hash but ... it sure reads funny.

What if you're passing the object to a method that really wants a Hash and checks for it, and now gives a deprecation warning?

The cure feels like it may be worse than the disease.

@@ -14,7 +14,7 @@ def assign_parameters
def dump_params_keys(hash = params)
hash.keys.sort.inject("") do |s, k|
value = hash[k]
- value = Hash === value ? "(#{dump_params_keys(value)})" : ""
+ value = value.respond_to?(:to_hash) ? "(#{dump_params_keys(value.to_hash)})" : ""
@jeremy

jeremy Mar 24, 2014

Owner

Again, need a to_params to rely on.

+ # params.merge(d: 4) # => {"a"=>1, "b"=>2, "c"=>3, "d"=>4}
+ def merge(other_hash, &block)
+ new_with_parameters(@parameters.merge(other_hash, &block))
+ end
@jeremy

jeremy Mar 24, 2014

Owner

Seems this should accept other params—and preserve its permitted status as well.

+ # params = ActionController::Parameters.new(a: 1, b: 2, c: 3)
+ # params.except(:c) # => {"a"=>1, "b"=>2}
+ def except(*keys)
+ new_with_parameters(@parameters.except(*keys))
@jeremy

jeremy Mar 24, 2014

Owner

Should drop permitted status for the excepted keys

+ #
+ # TODO: Remove this method once we reach 5.0.0
+ def kind_of?(klass)
+ if klass == Hash || klass == HashWithIndifferentAccess
@jeremy

jeremy Mar 24, 2014

Owner

if @parameters.kind_of? klass

@@ -619,7 +619,7 @@ class ApplicationController < ActionController::Base
app_file 'app/controllers/posts_controller.rb', <<-RUBY
class PostsController < ApplicationController
def create
- render text: params[:post].inspect
+ render text: params[:post].to_hash.inspect
Owner

jeremy commented Mar 24, 2014

Let's see some folks trying this in apps before landing on master. I'm afraid it will end up encouraging people to write less secure code by falling back on "oh, just to_h that to fix it." It's pretty confusing for users, and feels like we're passing the buck to them rather than making things "just work," y'know?

Some options:

  • Get uglier: subclass HWIA but privatize all methods that would be "lossy" (returning a new HWIA instead of Parameters) to obviate the security concern.
  • Get uglier: subclass HWIA and wrap all methods to return Parameters (preserving permitted status) to resolve the security concern.
  • Go further: compose HWIA, but change the Parameters API to be even less Hash-like (don't attempt to share a duck type with hashes).
  • Get smarter: maybe there's another way that satisfies the security concern? For example, maybe converting to a Hash should drop unpermitted keys?
Contributor

tute commented Apr 23, 2014

1 and 2 seem like friendlier options before 3 and 4, for a major release? Did you guys continue discussion?

Are you interested in seeing an alternative of forcing the pattern showed in https://gist.github.com/dhh/1975644? We'd be working with hashes, and enforcing secure practices.

Owner

guilleiguaran commented Apr 23, 2014

Agree with @tute, 3 and 4 sounds better in my opinion

Member

sikachu commented Jul 15, 2014

After the lengthy discussion and a stall, I don't think this patch works since it adds to much complexity and feels so much hackerish.

After I've discussed with @tenderlove I think we'll go with (2) and (4) from @jeremy's suggestion: Implement methods in ActionController::Parameters that currently returns Hash to return AC::Parameters instead, and also make a call to #to_h drops unpermitted hash. I think those should covers both security concern with the least amount of changes.

However, we should make sure to stop subclassing HWIA once we're planning to release the next major version. That should eliminate all the shenanigan around this.

I'm going to close this, and will create a new PR with new patch.

@sikachu sikachu closed this Jul 15, 2014

sikachu added a commit to sikachu/rails that referenced this pull request Jul 13, 2015

Make AC::Parameters not inherited from Hash
This is another take at #14384 as we decided to wait until `master` is
targetting 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.

sikachu added a commit to sikachu/rails that referenced this pull request Jul 13, 2015

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.

sikachu added a commit to sikachu/rails that referenced this pull request Jul 14, 2015

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.

sikachu added a commit to sikachu/rails that referenced this pull request Jul 14, 2015

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.

sikachu added a commit to sikachu/rails that referenced this pull request Jul 14, 2015

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.

sikachu added a commit to sikachu/rails that referenced this pull request Jul 14, 2015

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.

sikachu added a commit to sikachu/rails that referenced this pull request Jul 14, 2015

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.

sikachu added a commit to sikachu/rails that referenced this pull request Jul 15, 2015

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.

@sikachu sikachu deleted the sikachu:master-ac-parameters branch Jul 15, 2015

Contributor

tute commented Aug 18, 2015

😃

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