'resource_name'_params whitelists can be bypassed (and other strong params glitches)

[rst]

It's great to see this gem updated to deal with Rails 4 and 5, but the integration with the (then-)new strong parameters machinery seems to have a few rough spots. I've got code that attempts to deal with these issues on a branch of my clone, but figured I'd discuss the issues first before making it a formal PR.

The most serious is that even when a controller defines an explicit whitelist for parameters, as "#{resource_name}_params", it can be bypassed in certain circumstances. There's actually an illustration of the problem in the test suite. The CommentsController in the sample app.rb defines comment_params as follows:

def comment_params 
  params.fetch(:comment, {}).permit(:user_id)
end

The obvious intention here is that only :user_id be settable through mass assignment. But if you look at comments_controller_spec.rb, there's

def do_post
  post :create, params: { :comment => {:name => 'Comment'}, :forum_id => '3', :post_id => '2' }
end

it "should build a new comment" do
  expect(@post_comments).to receive(:build).with({'name' => 'Comment'}).and_return(@comment)
  do_post
end

If the comment_params whitelist were being applied, you'd expect this to fail, as the whitelist has only user_id and not name as permitted parameters. In fact, it passes. The reason for this turns out to be this code in resource_methods.rb:

def new_resource(attributes = nil, &block)
  if attributes.blank? && respond_to?(:params) && params.is_a?(ActionController::Parameters)
    resource_form_name = ActiveModel::Naming.singular(resource_class)
    attributes = params[resource_form_name] || params[resource_name] || {}
  end
  resource_service.new attributes, &block
end

What happens is that the create in actions.rb invokes new_resource(resource_params), which invokes comment_params. So far, so good. However, comment_params sees no parameters on its whitelist, and so returns an empty ActionController::Parameters. That answers true to blank?, which causes the code quoted above, in new_resource, to go digging around in params directly, bypassing the whitelist. If this code were invoking resource_params directly instead, to get a default set of parameters, this anomaly wouldn't occur.

The behavior of resource_params itself has a couple of other oddities. First off, if a controller defines #{resource_name}_params, that gets invoked -- but if the controller tries to override resource_params by itself (e.g.,

def resource_params
   params.fetch(:comment, {}).permit(:user_id)
end

it doesn't work. The problem is that the controller's own definition of resource_params is shadowed by the one in resource_methods.rb, which never tries to invoke super. This can be dealt with by adding an if defined?(super) to invoke the controller's own definition if it exists.

The last issue I've found here is that if no explicit _params method can be found, the default resource_params in resource_methods.rb assembles a whitelist for itself out of known column names. That mirrors the permissive default behavior of Rails 3 and earlier, in which the default policy (unless you asked for something else) was to permit mass-attribute setting in create or update to alter any attribute. However, the Rails team made a deliberate design choice to move away from that policy when they did strong parameters, because it had proven to be an unsafe default. It's still possible to get a permissive policy if you ask for one, e.g.:

def comment_params
   params.fetch(:comment, {}).permit!
end

but in stock Rails, you need to explicitly ask for it; you don't get it out of the box.

(An example of the problems that prompted this change -- in fact, the cause for it -- was on Github itself. A whiteish-hatted security researcher who thought Rails needed stronger checks updated the user_id of one of his own ssh keys to transfer it to a committer to the Rails project itself, and used it to make a commit onto Rails master. His commit was benign -- it just added a text file -- but it was enough to illustrate the danger of allowing stray attributes, such as user_id to be set through forms that weren't expecting them.)

To harmonize with this design intent, my proposed changes remove the default whitelist. The proposal is that it's still possible to get a permissive policy for some or all of your controllers, by an explicit

def resource_params
   params.fetch(resource_name, {}).permit!
end

in a particular controller, a base class for a set of them (e.g., AdminController, if all your admins have console access too, and access controls are pointless), or for quickie throwaways, in ApplicationController itself, but, as in base Rails, you should have to explicitly ask for it.

As noted above, I do have code to deal with all these issues (somewhat intermingled). Most of the changes are to specs, to make sure that they try to set only parameters that are on whitelists. (For the most part, the whitelists already were there in app.rb, but because of the new_resource issue, they were being ignored in a lot of cases.) But I thought I'd raise them for discussion before opening a formal PR...

[jlsync]

Robert, thanks for spotting these problems.Yes, these issues are intermingled quite a bit. Separately,

1/ Only the resource_params method in resource_methods.rb is required. The resource_params method in Actions.rb is redundant. Leaving this stale code in place was my mistake. Apologies for the confusion. I've now pushed a commit that fixes this. 14e7684

2/ Yes, new_resource is a problem. Your changes there look good.

3/ Overriding resource_params in the controller seems to work just fine for me. Overriding resource_params in a parent controller doesn't work just now ( I don't think this works either for new_resource, resource_service, etc. in parent controllers - which I often override in controllers).

4/ The default behaviour of resource_params currently,

return params.fetch(resource_name, {}).permit( *(resource_service.content_columns.map(&:name) - [ 'updated_at', 'created_at' ]) )

is an effort to match the params white listed in a standard scaffold controller so that an rc_rails controller can still be written with just one line of code, e.g.

  resources_controller_for :events

but, yes, this behaviour could be insecure if not overridden by a custom #{resource_name}_params.
Your solution of raising an error instead is good ( A softer approach might be to just log a warning ). Some more updated documentation on resource_params and #{resource_name}_params to match the new error raising behaviour would be nice - e.g. mentioning your permissive resource_params implemention would be useful. Currently, your branch is raising a RuntimeError, maybe a custom error or NoMethodError would be better, with a longer description in the error text that further explains what has happended and what is required ( perhaps with a link to this repo, the readme, the rdoc, this ticket, or page on the wiki ).

I can see that calling super in resource_params could be useful. I don't know if it's a good idea or not.

[rst]

First off, completely agreed that the docs need an update -- but best to have consensus on what the code ought to do first! Also agreed that NoMethodError is a better exception to throw.

On overriding resource_params in a base class... this is something I've found useful in the private resources_controller fork I've been using at $dayjob (not suitable for use or uploading here, as it upgrades only the bits we actually use, and doesn't fully pass its own tests!). The reason for that is that we've got a bunch of different model objects which have a common set of attributes, and a single resource_params really does work for all of them. (They're all "list of foo", with a has_many relationship to a FooListElement class that defines what's actually different about them -- but the FooList classes themselves just have a name, a few common attributes that are used for access control, and the has_many association.)

As to a softer approach than the error... the difference between this and scaffolding is that scaffold-generated code is visible and meant to be changed, and having it handled automatically behind the scenes adds that much more risk of out-of-sight, out-of-mind. And this generally will need to be changed before exposing a webapp to the wide internet; as the user_id of ssh_key example shows, it's very, very easy for an unexpected attribute set to completely compromise an application. (It's about as bad as register_globals in PHP, another convenience feature that had to be removed because it was nearly impossible for even experienced programmers to be sure they were using it safely.)

As is, with my full set of changes, a fully permissive policy is as easy as

def resource_params
   params.fetch(resource_name, {}).permit!
end

and I'd worry about making it easier than that. (On the other hand, if there's something we can do to make "add this here" more direct in web-console, for people that use it, I'd be for that; I'm just not familiar enough with its innards to know how that would work.)

[jlsync]

@rst if you're already finding using resource_params from a base class useful then, yes, I guess it's a good feature to have.

Yes, the existing default behaviour white-list of params should be removed, so a
#{resource_name}_params or resource_params method is required somewhere or an error raised.

(One idea I just had: A safer way to maintain the existing default white-list of params could be an option like this...

   resource_controller_for :events, :trusted_params => true

so developers must really deliberately opt into it, nothing is hidden, and you can still get an easy one-line-of-code controller if you want.)

[rst]

Updated my branch to make most of the above changes, and update the docs; will shortly open it as a PR. The one thing I didn't do is the trusted_params idea (which I might have called unchecked_params instead, to be more explicit about what's going on); it's not difficult, but for reasons outlined in my revised docs (which mention and warn against the params[resource_name].permit! gambit), it might be unwise...