rails / strong_parameters

Taint and required checking for Action Pack and enforcement in Active Model
MIT License
1.27k stars 167 forks source link

require returns Array then errors followed by permit #140

Open leckylao opened 11 years ago

leckylao commented 11 years ago

From Readme:

To whitelist an entire hash of parameters, the permit! method can be used

params.require(:log_entry).permit!

In my console:

irb(main):039:0> raw_parameters = { :teams => ["37"] }
=> {:teams=>["37"]}
irb(main):040:0> parameters = ActionController::Parameters.new(raw_parameters)
=> {"teams"=>["37"]}
irb(main):045:0> parameters.require(:teams).permit!
NoMethodError: undefined method `permit!' for ["37"]:Array
    from (irb):45
    from /home/leckylao/.rbenv/versions/2.0.0-rc2/lib/ruby/gems/2.0.0/gems/railties-4.0.0.rc1/lib/rails/commands/console.rb:90:in `start'
    from /home/leckylao/.rbenv/versions/2.0.0-rc2/lib/ruby/gems/2.0.0/gems/railties-4.0.0.rc1/lib/rails/commands/console.rb:9:in `start'
    from /home/leckylao/.rbenv/versions/2.0.0-rc2/lib/ruby/gems/2.0.0/gems/railties-4.0.0.rc1/lib/rails/commands.rb:66:in `<top (required)>'
    from bin/rails:4:in `require'
    from bin/rails:4:in `<main>'

Looks like the Readme needs to update with the correct API

hovsater commented 11 years ago

This is a duplicate of #139.

leckylao commented 11 years ago

Hi @KevinSjoberg @rafaelfranca , it looks similar, but my issue's title is better for searching. As the issue is on the require method:

def require(key)
      self[key].presence || raise(ActionController::ParameterMissing.new(key))
end

Which returns the value instead of returning the ActionController::Parameters instance. So it will never works for calling any other ActionController::Parameters instance method like permit. Therefore the README and API is wrong and has to be updated

rafaelfranca commented 11 years ago

This problem only occurs when the value of the key is an Array, so yes, the README should be update to say that require only works when the value of the key is a hash.

leckylao commented 11 years ago

Thx @rafaelfranca for the reply, But I am afraid it is not working at all as long as it doesn't return the ActionController::Parameters instance.

irb(main):002:0> raw_parameters = { :teams => "37" }
=> {:teams=>"37"}
irb(main):003:0> parameters = ActionController::Parameters.new(raw_parameters)
=> {"teams"=>"37"}
irb(main):004:0> parameters.require(:teams).permit!
NoMethodError: undefined method `permit!' for "37":String

It would be good that if you could provide an example that actually works. Thx.

rafaelfranca commented 11 years ago

No, it only work if the value is a hash.

This should work:

irb(main):002:0> raw_parameters = { :teams => { :number => "37" } }
irb(main):003:0> parameters = ActionController::Parameters.new(raw_parameters)
irb(main):004:0> parameters.require(:teams).permit!
leckylao commented 11 years ago

IC, cool. Thank you very much @rafaelfranca. Once README updated then it's all good :+1: :smile:

wrightling commented 11 years ago

Running into the same issue as I look to introduce strong_parameters to my JSON rails-api. Typical request JSON as seen in the examples at jsonapi.org looks something like this:

{"cards":[{"subject":"some subject", "reference":"some reference"}] }

Note the array mixed into this. If I start my strong_parameters statement with something like params.require(:cards), I get back an Array, not an instance of ActionController::Parameters, and even accepting that, if I go through each array member, they are not ActionController::Parameters instances either (they are instances of ActiveSupport::HashWithIndifferentAccess).

bryanrite commented 11 years ago

:+1: on strong_parameters with a JSON API like @wrwright.

Even if I don't want to allow an array, this is allows user input to cause NoMethod Errors and return a 500, when really this is probably a 400 or 422.

Aside from rescuing from NoMethod and matching on the error message, there is no good way to gracefully recover from bad user input.

What if we ensured that the require returned the proper type or responded to permit here: https://github.com/rails/strong_parameters/blob/master/lib/action_controller/parameters.rb#L54

Thoughts? I can make a pull request if we like it.

hovsater commented 11 years ago

Currently I'm solving these issues by iterating over the array and manually require and permit the elements within.

bryanrite commented 11 years ago

@KevinSjoberg I guess my issue is more that I don't want the array, or anything in it, but it is valid JSON and I may receive it from a client request. I would think the response should be a 4xx rather than a 500.

bryanrite commented 11 years ago

Alternatively, I've implemented this in my API specific application controller:

rescue_from NoMethodError do |exception|
  if exception.message =~ /undefined method `permit' for/
    render json: { message: "Helpful error message..." }, status: :bad_request
  else
    raise
  end
end
vdmgolub commented 11 years ago

@wrightling that's how I do it, maybe it may be still useful to you :) https://github.com/json-api/json-api/issues/121#issuecomment-21631332

def create
  photos = []

  photo_params.each do |p|
    photo = Photo.new(p)
    photos << photo if photo.save
  end

  render json: photos, status: 201
end

def photo_params
  params.require(:photos).map do |p|
     ActionController::Parameters.new(p.to_hash).permit(:title, :src)
   end
end
hirokishirai commented 10 years ago

@vdmgolub I was also facing this issue.However, it solved by your comment! Thanks!

NikoRoberts commented 9 years ago

@guy-silva and I ran into this dealing with Mandrill inbound webhooks Hopefully this helps some people

params = ActionController::Parameters.new({
  mandrill_events: [
    {
      msg: {
          name: 'Francesco',
          age:  22,
          role: 'admin'
        }
    },
    {
      msg: {
        name: 'steve',
        age:  22,
        role: 'admin'
      }
    }
  ]
})
permitted = params.require(:mandrill_events).map { |m| m.require(:msg).permit(:name, :age) }
# => [{"name"=>"Francesco", "age"=>22}, {"name"=>"steve", "age"=>22}] 
ryankc33 commented 8 years ago

@vdmgolub You're a lifesaver!

ryanmtaylor commented 8 years ago

It's honestly so much easier to do

params.require :photos
params.permit photos: [:title, :src]

and just not chain them.

kirantpatil commented 8 years ago

@ryanmtaylor I am getting ActiveModel::ForbiddenAttributesError (ActiveModel::ForbiddenAttributesError).

Error log:

ActiveModel::ForbiddenAttributesError (ActiveModel::ForbiddenAttributesError):

app/controllers/traumas_controller.rb:28:in block in create_multiple' app/controllers/traumas_controller.rb:27:ineach' app/controllers/traumas_controller.rb:27:in `create_multiple'

I did as per your suggestion as below.

stdout output:

Started POST "/traumas/create_multiple" for 127.0.0.1 at 2016-10-05 14:38:49 +0530 Processing by TraumasController#create_multiple as JS Parameters: {"utf8"=>"✓", "fields"=>[{"contusions"=>"1", "burns"=>"", "at_scene"=>"At Scene", "emergency_detail_id"=>"97", "trauma_region"=>"Head-Back"}], "commit"=>"Submit"} User Load (1.6ms) SELECT "users".* FROM "users" WHERE "users"."id" = ? ORDER BY "users"."id" ASC LIMIT ? [["id", 3], ["LIMIT", 1]] Unpermitted parameters: utf8, commit [<ActionController::Parameters {"contusions"=>"1", "burns"=>"", "at_scene"=>"At Scene", "emergency_detail_id"=>"97", "trauma_region"=>"Head-Back"} permitted: false>] Completed 500 Internal Server Error in 6ms (ActiveRecord: 1.6ms)

Controller method:

def create_multiple
    trauma_params
    puts params[:fields].inspect
    params[:fields].each do |values|
       u = Trauma.create(values)
    end
  end
   def trauma_params
      params.require :fields
   params.permit fields: [:fractures, :abrasions, :contusions, :laceration , :bleeding_controlled, :bleeding_uncontrolled ,:burns, :at_scene, :emergency_detail_id, :trauma_region]
    end
kirantpatil commented 8 years ago

Resolved by referring @vdmgolub code.