Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Strong parameters: allow hashes with unknown keys to be permitted #9454

Closed
spohlenz opened this issue Feb 27, 2013 · 70 comments
Closed

Strong parameters: allow hashes with unknown keys to be permitted #9454

spohlenz opened this issue Feb 27, 2013 · 70 comments
Labels

Comments

@spohlenz
Copy link
Contributor

@spohlenz spohlenz commented Feb 27, 2013

From what I can tell, strong parameters currently has no ability to permit a hash with unknown keys. Where this would be particularly useful is for the newly supported Hstore and JSON data types -- a good example of this use case can be found here: http://schneems.com/post/19298469372/you-got-nosql-in-my-postgres-using-hstore-in-rails

I would have expected that passing an empty hash as an permitted value would allow a hash to be passed through. e.g.

params = ActionController::Parameters.new(product: { name: 'Test', data: { weight: '12kg' } })
params.require(:product).permit(:name, data: {})

however this does not pass the data hash through (though it is not documented that it should work).

Assigning the data parameter separately is an option but it complicates my code unnecessarily -- I would prefer to be able to stick with mass-assignment for all attributes.

Happy to work on a patch for this if this proposal is reasonable. I've only just started looking into strong parameters though so there may be drawbacks I haven't considered.

@spohlenz
Copy link
Contributor Author

@spohlenz spohlenz commented Feb 27, 2013

Another possibility which I think I like even better would be to allow permit! to take an optional parameter, to allow this:

params.require(:product).permit(:name).permit!(:data)
@sideshowcoder
Copy link
Contributor

@sideshowcoder sideshowcoder commented Feb 27, 2013

I am really interested in a way to solve this as well, so +1. I think your 2. approach looks cleaner to me as well, passing {} to permit all sub keys looks weird to me.

@senny
Copy link
Member

@senny senny commented Feb 27, 2013

strong parameters is not designed to handle every possible situation. We wanted to keep the API on point to handle the most common situations. As assigning a hash with unknown keys more or less defeates the purpose of strong parameters (restricting allowed keys), It's not supported because it could make your code vulnerable.

As you mentioned already you can simply fall back on a normal assignment if you know that you want to permit unknown keys.

/cc @fxn @rafaelfranca

@fxn
Copy link
Member

@fxn fxn commented Feb 27, 2013

Confirm, the API is designed to whitelist every single key. If your use case does not fit well then you need to resort to Ruby. That flexibility is also by design, in the end it is just Ruby.

@fxn fxn closed this Feb 27, 2013
@spohlenz
Copy link
Contributor Author

@spohlenz spohlenz commented Feb 27, 2013

strong parameters is not designed to handle every possible situation. We wanted to keep the API on point to handle the most common situations.

Understand, but this doesn't exactly seem like an uncommon use case (particularly with the introduction of Hstore and JSON types), and with attr_accessible/attr_protected being removed from Rails core, options are severely reduced.

I would argue that this actually makes the API more consistent, since permit! would accept a list of keys, closely mirroring permit.

As assigning a hash with unknown keys more or less defeates the purpose of strong parameters (restricting allowed keys), It's not supported because it could make your code vulnerable.

Except you'd only be permitting an unknown hash on a single key within the params. It's certainly more secure than doing a permit! on the whole params hash (which is supported behaviour).

As you mentioned already you can simply fall back on a normal assignment if you know that you want to permit unknown keys.

My main problem with this is that I go from having to test one message (Product.create) to having to test three messages (Product.new, @product.data= and @product.save). The separate assignment for my hash also has to be duplicated across both my create and update actions.

Here's a concrete example of how my proposal would improve things. Before:

class ProductsController < ApplicationController
  def create
    @product = Product.new(product_params)
    @product.data = params[:product][:data]
    @product.save!
  end

  def update
    @product = Product.find(params[:id])
    @product.data = params[:product][:data]
    @product.update!(product_params)
  end

private
  def product_params
    params.require(:product).permit(:name)
  end
end

After:

class ProductsController < ApplicationController
  def create
    @product = Product.create!(product_params)
  end

  def update
    @product = Product.find(params[:id])
    @product.update!(product_params)
  end

private
  def product_params
    params.require(:product).permit(:name).permit!(:data)
  end
end

I do think this needs more discussion. It's all well and good to say just do it manually, but it feels wrong to have to work around a feature (and make my code more complicated), particularly when the alternatives have been removed.

@fxn
Copy link
Member

@fxn fxn commented Feb 27, 2013

To avoid duplication set the trusted parameter in the helper (untested):

def product_params
  params.require(:product).permit(:name).tap do |whitelisted|
    whitelisted[:data] = params[:product][:data]
  end
end
@spohlenz
Copy link
Contributor Author

@spohlenz spohlenz commented Feb 27, 2013

Thanks @fxn. That is a reasonable solution I had not thought of. :)

@fxn
Copy link
Member

@fxn fxn commented Feb 27, 2013

@spohlenz awesome :), this feedback was useful we are going to document a few edge cases like this or similar.

senny added a commit to senny/rails that referenced this issue Mar 3, 2013
The current ActionController guide does not mention strong parameters
at all. I integrated the README into the guide to explain the API.

I also included a section to illustrate that the API does not solve
all possible whitelisting scenarios.

The origin was rails#9454.
@hakanensari
Copy link
Contributor

@hakanensari hakanensari commented Jul 8, 2013

@senny It may be helpful to have the Strong Parameters README mirror the last paragraph on using with Hstore and JSON data types. Latter ends up being a more obvious reference point thanks to Google.

@senny
Copy link
Member

@senny senny commented Jul 16, 2013

@hakanensari I'm not sure if duplicating the guides in the README is a good thing. They will get out of sync quickly. I'd like to keep the additional examples in the guides because they are the reference for Rails.

Maybe we could link from the README to the relevant section in the guides? (http://guides.rubyonrails.org/action_controller_overview.html#more-examples)

@fxn what do you think?

@fxn
Copy link
Member

@fxn fxn commented Jul 16, 2013

@senny definitely linking.

@lanej
Copy link

@lanej lanej commented Jan 6, 2014

The solution mentioned by @fxn only works if you have decided to log instead of raise when un-permitted parameters are present. There are plenty of situations (metadata, configuration, etc.) where whitelisting a sub-hash is completely valid.

I think that @spohlenz solution might be the most similar and I will have to implement something like it.

@fxn
Copy link
Member

@fxn fxn commented Jan 6, 2014

I like this approach #12609.

@lanej
Copy link

@lanej lanej commented Jan 6, 2014

that's a sufficient workaround but hardly a good solution. what about params.require(:product).permit(:name, :data => Hash) and allowing Hash as a permitted scalar value IF it is explicitly defined.

@fxn
Copy link
Member

@fxn fxn commented Jan 6, 2014

Yes, I believe this use case deserves API.

Something like data: :permit! would be nice, I'd need to think about a way to express that that feels like a natural addition to the existing API.

@lanej
Copy link

@lanej lanej commented Jan 6, 2014

fyi, the approach in #12609 is not feasible for hashes of a depth greater than 2.

@jhubert
Copy link
Contributor

@jhubert jhubert commented Jan 14, 2014

👍 This has also come up with us as an issue for supporting HStore metadata values on our models.

Also, and perhaps I'm doing something wrong, but the solution from @fxn seems to actually set nil values on the params if they aren't already set:

params = ActionController::Parameters.new({ test: true })
params.permit(:test).tap do |whitelisted|
  whitelisted[:more] = params[:more]
end
# => {"test"=>true, "more"=>nil}
@lanej
Copy link

@lanej lanej commented Jan 14, 2014

@jhubert that definitely makes sense. whitelisted is a hash and you are setting whitelisted[:more] to params[:more] which is nil.

@jhubert
Copy link
Contributor

@jhubert jhubert commented Jan 14, 2014

@lanej Thank. Yeah, that makes sense. It was unexpected behavior for me and my tests failed because the parameter was being set to nil instead of being unprovided. Figured I would mention it in case other people just copy / paste it expecting it to just permit the variable if it was provided and not change the params themselves.

@richardkmichael
Copy link

@richardkmichael richardkmichael commented May 3, 2014

@jhubert FWIW, ActionController::Parameters#fetch() will raise ActionController::ParameterMissing, instead of simply returning nil.

[1] pry(main)> p = ActionController::Parameters.new({test: true})
=> {"test"=>true}
[2] pry(main)> p[:foo]
=> nil
[3] pry(main)> p.fetch :foo
ActionController::ParameterMissing: param is missing or the value is empty: foo # .... 
[4] pry(main)> p.fetch :foo, nil
=> nil
@nhattan
Copy link

@nhattan nhattan commented Jun 20, 2014

thanks, this works for me:

def product_params
  params.require(:product).permit(:name).tap do |while_listed|
    while_listed[:data] = params[:product][:data]
  end
end
@chinshr
Copy link

@chinshr chinshr commented Jan 6, 2015

@fxn agree, that's an acceptable solution. @nhattan, I ended up adding testing for nil value.

def product_params
  params.require(:product).permit(:name).tap do |whitelisted|
    whitelisted[:data] = params[:product][:data] if params[:product][:data]
  end
end
@NullVoxPopuli
Copy link

@NullVoxPopuli NullVoxPopuli commented Jan 15, 2015

i think this would be a neat feature to add.
I have data that looks like this currently:
=> {"47"=>{"M"=>{"quantity"=>"2"}, "XL"=>{"quantity"=>"4"}, "XXXL"=>{"quantity"=>"1"}}}
where it maps the ID of an object to some properties for that particular object...

but maybe there is a better way to do that? idk. ('usually is)

@cohesivejones86
Copy link

@cohesivejones86 cohesivejones86 commented Apr 15, 2015

My company doesn't use the key value of the nested we expect the nested hash to have and id property if its persisted for example
{ root_model: {
nested_model: {
GUID: { id: 1, value: 'something persisted' },
ANOTHER GUID: { value: 'a new record as it has no id'},
GUID: { id: 3, _destroy: true,
value: 'a record to delete but maybe we should
have another controller for this'
}
}
}
maybe we need to have wildcards ie %id% or %GUID% or just stop using nested attributes

@nhattan
Copy link

@nhattan nhattan commented Apr 15, 2015

@chinshr You're right! thanks for pointing this case.

@MSCAU
Copy link

@MSCAU MSCAU commented Jul 14, 2016

Hmmm, seems the issue is in the JS before the AJAX call, sorry. jQuery needs contentType to be set to "application/json". See http://stackoverflow.com/questions/6410810/rails-not-decoding-json-from-jquery-correctly-array-becoming-a-hash-with-intege

@Nowaker
Copy link

@Nowaker Nowaker commented Aug 26, 2016

Hey @fxn and @senny, it's the year of 2016 now. Rails got background jobs and even websockets, things that were originally neglected/ignored. Since 2013, the use of free-form JSON and Hstore values has rapidly grown too, and maybe it's high time to not ignore them either. The values submitted to them are free-form by design, therefore not subject to any whitelisting/filtration. But they don't live alone. They live inside models whose fields are whitelisted and filtered against mass assignment.

Consider Stripe gateway. They allow users to store free-form metadata on customer object, and that's just a hash. How to achieve that in Rails if the JSON request looks like this: POST /customers, {description: ..., source: ..., metadata: {... free form data here...}}. Answer: fight the framework - and many people in this thread did it their own way. Currently, if there's any JSON field without a predefined "schema" inside the params, Strong Parameters has to be worked around for each field. In 2016, letting a non-checked JSON in is a common situation.

There's already @sdepold's pull request to allow this here: rails/strong_parameters#231. I hope you can reevaluate this feature and give it a green light. If there's any issue/feedback about that PR, I can take it over from the original submitter and finish it up if he's unreachable. Please let me know.

@fxn
Copy link
Member

@fxn fxn commented Oct 20, 2016

@Nowaker agree, we've been conservative, but time says this use-case deserves a dedicated API.

The :* proposal seems good to me. The patch would need some work, but the basic idea of a wildcard symbol to say "accept whatever here" might work.

The patch is no big deal, I might write one based on that PR (and give credit of course).

@fxn
Copy link
Member

@fxn fxn commented Nov 11, 2016

Implemented here.

@stiig
Copy link

@stiig stiig commented Nov 12, 2016

it would be nice if it also allow to set structure incoming hash, like a

params.permit(preferences: [{:scheme, font: {:name, :size}}])

for next cases in params:

params = ActionController::Parameters.new(
    username: "fxn",
    preferences: [
        {
            scheme: "Marazul",
            font: {
                name: "Source Code Pro",
                size: 12
            }
        },
        {
            scheme: "new scheme",
            font:
                {
                    name: "Another Font",
                    size: 14
                }
        }
    ])
@temirov
Copy link

@temirov temirov commented Dec 22, 2016

I must be missing something, but Rails 5.0.1 still gives me grief:

irb(main):011:0> ActionController::Parameters.new(
irb(main):012:1*   items: [
irb(main):013:2*     { data: { a: 1}},
irb(main):014:2*     { data: { b: 2}}
irb(main):015:2>   ]
irb(main):016:1> ).permit(items: [ data: {}]).to_h
=> {"items"=>[{"data"=>{}}, {"data"=>{}}]}
@fxn
Copy link
Member

@fxn fxn commented Dec 22, 2016

Patch versions don't have new features, this is going to come with 5.1.

@ghost
Copy link

@ghost ghost commented Jan 5, 2017

I modified @PandaWhisperer solution to safeguard against non-existing key:

def product_params
properties_keys = params[:product].try(:fetch, :properties, {}).keys
params.require(:product).permit(:title, :description, properties: properties_keys)
end

@aliibrahim thanks, that worked for me!

@stevebissett
Copy link

@stevebissett stevebissett commented Feb 9, 2017

I can't see a solution to this that has been merged.
Is anyone able to point me to one (@fxn)?

I have the following input hash (obfuscated sample):

      {
        "name" => "Test",
        "size" => 150,
        "some_array_of_objects" =>
          [
            {"some_key" => {"name" => "test_2", "valid" => "true"},
             "another_key" =>
               {
                 "very_deep_hash" => {"abc" => "def"},
                 "i_dont_care_about_this_key" => 5
               }
             },
            {"some_key" => {"name" => "test_2", "valid" => "true"},
             "another_key" =>
               {
                 "very_deep_hash" => {"abc" => "def"},
                 "i_dont_care_about_this_key" => 5
               }
             }
          ]
      }

(Edited)

I want to whitelist all the keys in some_array_of_objects.
Even if I do know all the keys down to the lowest depth, I can't seem to get it working with arrays.

Can someone point me in the right direction?

@Nowaker
Copy link

@Nowaker Nowaker commented Feb 9, 2017

@stevebissett The right direction would be for some_array_of_objects to be a hash. See e86524c - it allows arbitrary hashes, not arbitrary values. @fxn suggested :* but it didn't end up being implemented.

@stevebissett
Copy link

@stevebissett stevebissett commented Feb 9, 2017

@Nowaker I can't turn that into a hash, because I need multiple of them, (edited slightly).

@Nowaker
Copy link

@Nowaker Nowaker commented Feb 9, 2017

"some_array_of_objects" => {elements: [...]}

@olmesm
Copy link

@olmesm olmesm commented Feb 18, 2017

Rails 5 workaround -

def product_params
      load_params = params.require(:product).permit(:name)
      load_params[:json_data] = params[:product][:json_data]
      load_params.permit!
end

I realize this doesn't whitelist the data within the json_data param, but it excludes same-level unpermitted params.

Oh and hi @stevebissett 👍

@Deepak275
Copy link

@Deepak275 Deepak275 commented Mar 15, 2017

@spohlenz i have faced similer problem and after lot of googling and going through various stackoverflow Q/A was still not able to figure out good solution. So, i tried myself some hack.
So, here what i did
params = ActionController::Parameters.new(driver: { driver_data: { name: "zyx", address: "adfasf" } })
driver_data_keys = params[:driver][:name].keys
params.require(:driver).permit(driver_data: [driver_data_keys])

Advantages

  • Do for rails 4.
  • I know it is a hack, but still it is looking good than i have found different solution.
  • it is the best soluion for whiltlisting any or any no of key withuto getting worried about handling it.
    I hope it will help. Happy coding.
@skozz
Copy link

@skozz skozz commented May 24, 2017

The @olmesm solution works for Rails 4.2.7 too 👍

@batmanbury
Copy link

@batmanbury batmanbury commented May 27, 2017

This thread started quite a while ago, but it's still active, so here's something I just discovered. I was able to permit a jsonb attribute to my User model with the syntax normally used with accepts_nested_attributes_for:

def user_params
  params.require(:user).permit(:email, :password, data: [:one, :two, :three])
end

Where data is your json attribute, and the array of properties are what you wish to permit. I'm not sure if this is functioning as intended by Rails, or if it just works through some fluke. It may not work with every use case, or with deeply nested attributes, but I'm curious to see how it works for others here. I'm using Rails 5.1.1.

@cianmcelhinney
Copy link

@cianmcelhinney cianmcelhinney commented Jun 21, 2017

Running on Rails 4.2.6 this works for me:

def user_params
  params.require(:person).permit(:name, :description, :age, custom_attributes: [:key, :value])
end

This will allow the following params to be permitted:

{
  "person": {
    "name": "John",
    "description": "has custom_attributes",
    "age": 42,
    "custom_attributes": [
      {
        "key": "the key",
        "value": "the value"
      }
    ]
  }
}

This can be seen in the source here:

If you don't know what might be in custom_attributes you could do

def user_params
  params.require(:person).slice(:name, :description, :age, :custom_attributes) 
end
@lanej
Copy link

@lanej lanej commented Jun 21, 2017

In the majority of these comments, the keys are known. This thread's subject was for unknown keys.

A few of the better workarounds are:

Fix is coming in 5.1.2 e86524c

@anklos
Copy link

@anklos anklos commented Jun 24, 2017

@olmesm

def product_params
      load_params = params.require(:product).permit(:name)
      load_params[:json_data] = params[:product][:json_data] if params[:product][:json_data]
      load_params.permit!
end

need to add a if condition otherwise it add a nil value for json_data, which causes issue if you call update on an object with the param

but good workaround, thank you!

@benbonnet
Copy link

@benbonnet benbonnet commented Jul 24, 2017

@anklos your example work, saves the data correctly (thx alot)

but

It still complains about the concerned json column being unpermitted. Is that normal ?

@rmcsharry
Copy link

@rmcsharry rmcsharry commented Oct 20, 2017

@deesx I would like to know the answer to that also. 50+ rep bonus for the answer

@xiaopow
Copy link

@xiaopow xiaopow commented Oct 26, 2017

@olmesm , @anklos ,
This solution

def product_params
      load_params = params.require(:product).permit(:name)
      load_params[:json_data] = params[:product][:json_data] if params[:product][:json_data]
      load_params.permit!
end

works for most of my data, except when my json_data looks like this

{
  key: [
    {
      key2: [],
      key3: {},
    },
    {
      key2: [],
      key3: {},
    },
  ],
  key4: "123",
}

I get the following error. I am using Mongoid by the way.

Unpermitted parameter: :json_data
Completed 500 Internal Server Error in 55ms (ActiveRecord: 0.0ms)
ActionController::UnfilteredParameters - unable to convert unpermitted parameters to hash:
@michaelbridge
Copy link

@michaelbridge michaelbridge commented Jan 4, 2018

Perhaps this is overkill, but one simple workaround that I didn't see here is as follows:

params.permit( ... ).merge(params.to_unsafe_hash.slice(:json_data))

The other workarounds don't appear to work for arbitrarily nested hashes and arrays.

Meekohi added a commit to Meekohi/rails that referenced this issue Jun 14, 2018
Since the ability to whitelist arbitrary hashes was added (rails#9454 was resolved by rails@e86524c), this example is no longer outside of what strong_params can do. Moved this specific example out of the "Outside the Scope" section and into the regular "Examples" section, but left the "Outside the Scope" section as it was since the advice is still relevant for weirder whitelisting situations (maybe someone wants to add a new example that can't be handled natively).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
You can’t perform that action at this time.