Initial idea to add permission based whitelisting to params

[wyaeld]

This pull isn't really intended to be merged as-is, but more as a convenient way to start a dialogue around the concept. It does work and included tests.

This is conceptually similar to the new StrongParameters built for Rails, in this case for each endpoint you can add a list of parameters to the Endpoint via permit(:foo, :bar, :baz) that will deny anyone calling that endpoint with params not on the whitelist.

This initial example only handles the most simple path. The deep merging of params causes a problem, it would likely be necessary for a full solution to automatically filter permit params coming from rack. Currently I used a simple hack of passing an alternative params hash to this particular validator.

I personally agree with the emerging trend that parameter whitelisting is a concern for the controller/api layer, and you should have the flexibility to permit based on the particular action that is occuring, rather than just declaring accessible in the models. In complex applications that isn't always sufficient, especially if different endpoints in the API serve different levels of authorisation.

What do other people think?

[adamgotterer]

Don't think this method is used or necessary?

[wyaeld]

Thanks for the comments Adam. I want to re-iterate though that its not an attempt to provide a fully specced and working feature yet, just demo how it could be done, and get a discussion on whether this is desired in Grape, and if so, how should things like the params merging be handled.

[wyaeld]

Ok, not seeing any interest in discussing this feature. I'll give it a few more days, and if no-one wants it I'll just close.

[dblock]

We already have a params block that has requires and optional and stuff like that. So it seems that from there to making only listed parameters allowed is a small step. Maybe a global API-level configuration?

class API < Grape::API
   only_allow_the_params_i_declared # needs a better name
end

or

class API < Grape::API
   params do
       only_allow_the_params_i_declared # needs a better name
       requires ...
   end
   post foo
   end
end

[dblock]

Giving this another 2 minutes of thought :) - the permits keyword is actually something I would like. As long as it can do permits :none which permits only declared params?

[mbleigh]

I feel like this should be baked into declared params. If I declare a param, then I'm automatically starting a white list. I can then say permit :all or similar to allow any params. Things should be secure/whitelisted by default IMHO.

[dblock]

I am with @mbleigh on this one.

[mbleigh]

I'm thinking of implementing a helper called declared_params that would be a version of the params hash that only included keys that had been declared. This is what you would want to use when calling something like update_attributes and would therefore be optional and available when needed without being intrusive. Thoughts?

[wyaeld]

I like the idea of including it into the existing helper, since you already have similar :requires functionality of the rails/strong_parameters gem there.

Also +1 on the better accessibility of the params hash. We've found the existing params hash occasionally difficult to use due to the middleware params that get merged into it. Would it be easy to make it optionally available as you mentioned, but not specifically because of declared_params, but more wrapped in something addressed to the endpoint?

Also, does it actually need a second helper, or could you just wrap the params addressed to the endpoint in a key based on the endpoint name? Not sure if that idea would break backwards compatibility for people, but considering the current params hash I dont think people tend to throw the entire thing at a Object.create call.

eg. rails does this

def create
    Person.create(params[:person])
  end

so something like

resource :invoices do
  params do
    requires :amount, :account, :customer
    # DRY assumption being that anything in requires is automatically in permits
    permits :description :note  
  end

  post do
    #so the normally params is still there, but stuff specific to the resource :invoices would be found at :invoice
    Invoice.create(params[:invoice])
  end

end

[dblock]

I think there's a lot of good stuff in this thread. Looking forward to an update here from @wyaeld with something clearly explaining in the README that params create a white list (are a reliable way to avoid the update_attributes! mess :).

[mbleigh]

I definitely like the direction we're going here. I think that params should remain more or less as is with new functionality to work with whitelisting. I see three possible approaches:

1. Have a separate declared_params or similar helper that would only include whitelisted params.
2. Have a special key on the normal params hash (e.g. params[:declared]) that contains all of the declared params.
3. Have a helper to which you can pass the params hash and clean it as needed (e.g. permitted(params) would take the normal params hash and clean it of any non-declared params.

I actually think I like option 3 the best, what are all of your thoughts?

[wyaeld]

preference for 2, but any of the options look simple and easy enough to use.

When you say cleaning of non-declared params, do you mean the rack related stuff? I'd expect that if you try to send a param not on the whitelist it errors.

[mbleigh]

I feel like its slightly user-hostile to throw errors if they supply non-valid params. Just ignore the params seems like the sane solution in such cases.

[wyaeld]

Depends whether you are trying to implement the attr_accessible functionality of rails, or the new strong_params coming in rails4. For the record I was using the latter as inspiration, since attr_accessible is already completely available us.

[adamgotterer]

Wanted to chime in. I like errors. I find it much harder to figure out what went wrong when a system completely ignores what you've done. I'm currently white listing some params via helpers and throwing errors, so I might be bias.

[mbleigh]

I'm not really interested in implementing anyone else's system blindly...I don't see what the advantage is to throwing errors over ignoring. It's not the same as attr_accessible because it still places the control in the consumer-facing layer instead of the model (which is appropriate IMO, models should be able to trust their inputs).

A middle ground could be to keep a hash of undeclared params as well. Then to error on whitelisting you could do something like:

before do
  error! "invalid params: #{params[:undeclared].keys.join(', ')}", 400 if params[:undeclared].any?
end

[wyaeld]

Agreed, thats why I kept the initial pull to the minimal to get the idea across, rather than a full implementation.

I like your error being thrown there, I'm fine with adding 1 line of code to make that the behaviour I want rif you prefer it not be default.

declared/undeclared seems less clear naming. The line of code is obviously declaring something, but I'd prefer valid or permits or something that directly expresses the idea of only allowing 'these' params through

[kristianmandrup]

Preference for option 3, declared(params). Bad idea to pollute the params hash more than absolutely necessary IMO. Also provides more flexibility (options for enhancements) in the future I think.

[schmurfy]

If a whitelist system is implemented I think allowing the users to chose whether they want to raise an error or ignore is a wise decision, I prefer an error to be raised but I agree different applications may have different needs, I just hate when a method allows me to put anything in its options hash and just ignore the invalid keys (activerecord is a perfect example of this and gave me a lot of headaches).

I like option 3, for my current use cases I don't send blindly my hash to anything like activerecord so if more keys are present they will just be ignored.

Whitelisting with nested parameter should be fun to implement if we can ever come to an agreement on what is the expected behavior.

[dblock]

Ok, so we know that this specific pull is not going to make it. I am going to close it, please yell at me if you think otherwise. We do want an easy, documented way for users to have attr_accessible-style implementation, looking forward to a new PR.