rails/rails — pull request #13215 by imanel (2013-12-06 15:06:25+00:00)
Title: [Concept] Always require permit before fetching

This is just idea and I need to hear opinion about it before moving forward. Code is just couple lines to help me explain idea and will be reworked if community agree that this *might* be good direction.

### How strong parameters are working now:

Let's take example from [strong_params](https://github.com/rails/strong_parameters) gem:

```ruby
class PeopleController < ActionController::Base
# This will raise an ActiveModel::ForbiddenAttributes exception because it's using mass assignment
# without an explicit permit step.
def create
Person.create(params[:person])
end

# This will pass with flying colors as long as there's a person key in the parameters, otherwise
# it'll raise a ActionController::MissingParameter exception, which will get caught by
# ActionController::Base and turned into that 400 Bad Request reply.
def update
person = current_account.people.find(params[:id])
person.update_attributes!(person_params)
redirect_to person
end

private
# Using a private method to encapsulate the permissible parameters is just a good pattern
# since you'll be able to reuse the same permit list between create and update. Also, you
# can specialize this method with per-user checking of permissible attributes.
def person_params
params.require(:person).permit(:name, :age)
end
end
```

Strong params help us with one issue: params passed to ActiveRecord attributes require to be permitted, otherwise it will raise error. This prevent from writing to columns we do not allow, and is considered secure enough.

Unfortunately there is second problem in this code that strong params can't solve right now: when we are searching for record (i.e. in mentioned `update` method) we are allowing any data sent in `id` key. It might not always be single key - sometimes it might be array or other type. This lead to errors like [CVE-2013-0155](https://groups.google.com/forum/#!topic/rubyonrails-security/t1WFuuQyavI):

```ruby
unless params[:token].nil?
user = User.find_by_token(params[:token])
user.reset_password!
end
```

With `token` param equal empty array or `[nil]` such code will pass and be potentially insecure. Right now Rails uses temporary fix that [compacts tables and converts them to nil](https://github.com/rails/rails/blob/master/actionpack/lib/action_dispatch/request/utils.rb). This is useful to prevent accidentally allowing `nil` value, but is not preventing passing multiple values. Because of that there is potential vector of attack by sending huge number of tokens instead of one with chance to find one (it's unlikely, but still this is potential security problem). Unfortunately patching `find` method is not solution as we can use `find`, `find_by`, `where` and multiple other statements - maintaining fixes in all of them would be nightmare.

Because of that it might be good idea to move checking parameters from `ActiveRecord` to `ActionController::Parameters` directly. That would make much easier to control how and when params would be used.

### Concept:

At this point `ActionController::Parameters` allows to fetch params via `[]` and `fetch` methods. This behavior returns value, and if this value is type of `Hash` it has copied status of `permit`. Unfortunately it's unable to store such params on `Array` and other types. It's possible to use strong params like that:

```ruby
params.permit(:token) # Will return nil for Array

unless params[:token].nil?
user = User.find_by_token(params[:token])
user.reset_password!
end
```

Unfortunately, because it's not enforced there are not many teams that are doing so. This concept is about enforcing such behavior - without `permit` calling `[]` or `fetch` on params would raise error about unpermitted params. That would prevent both errors like CVE-2013-0155 and ensure valid type of params (if you would allow `Array` then single value will not be allowed). Such behavior might be considered beneficial if we think that up to now all params should be treated as insecure.

There is two problems with enforcing using permit on all parameters:

1) All current application will need to add `permit` in every search
2) It's not very convenient to write permit every time we are using `id`

So I'm thinking about adding class method `permit_params` as syntactic sugar:

```ruby
class PeopleController < ActionController::Base

permit_params :id, only: :update

def update
person = current_account.people.find(params[:id])
person.update_attributes!(person_params)
redirect_to person
end

end
```

That would make permitting much easier for different actions.

Second idea is to permit `:id` key at default, which would cover around 90% of situations, but I'm not entirely sure if we should provide any defaults for such usecase.

Please let me know what do you think about this idea - it's just concept for now and I'm open to suggestion in which direction I should go, but it might make managing parameters much easier. I'm also sure that this idea is big change so it would require some preparation and couldn't enter master in near future.