15

The official way of preventing security risks with mass-assignment is using attr_accessible. However, some programmers feel this is not a job for the model (or at least not only for the model). The simplest way of doing it in a controller is slicing the params hash:

@user = User.update_attributes(params[:user].slice(:name))

However the documentation states:

Note that using Hash#except or Hash#slice in place of attr_accessible to sanitize attributes won’t provide sufficient protection.

Why is that? Why a whitelist-slicing of params does not provide enough protection?

UPDATE: Rails 4.0 will ship strong-parameters, a refined slicing of parameters, so I guess the whole slicing thing was not so bad after all.

tokland
  • 66,169
  • 13
  • 144
  • 170
  • 1
    Well for starters it's just an inconvenience. With `attr_accesible` you can use `:name` in your model if you need to (albeit it without saving it), but if you `.slice` it off the `params` hash you can't do that. It's also much more semantic to use `attr_accesible` because it tells others the properties relationship with the model, whereas slicing it is much more cryptic. – Alex Sep 20 '11 at 10:11
  • 3
    @Alex: I understand that *attr_accessible* is a convenient way of managing mass-assignment. Ok, but what's the security hole of using params[:xyz].slice? – tokland Sep 20 '11 at 10:16
  • For the record, [attr_accessible](http://api.rubyonrails.org/classes/ActiveModel/MassAssignmentSecurity/ClassMethods.html#method-i-attr_accessible) now states "*Note that using Hash#except or Hash#slice in place of attr_accessible to sanitize attributes provides basically the same functionality, but it makes a bit tricky to deal with nested attributes.*" Also, see the [Edge API](http://edgeapi.rubyonrails.org/classes/ActionController/StrongParameters.html) for advance Rails 4 docs, and see [strong_parameters plugin](https://github.com/rails/strong_parameters) for what to use prior to Rails 4. – user664833 May 15 '13 at 02:17

5 Answers5

6

The problem with slice and except in controller might occur in combination with accept_nested_attributes_for in your model. If you use nested attributes, you would need to slice parameters on all places, where you update them in controller, which isn't always the easiest task, especially with deeply nested scenarios. With using attr_accesible you don't have this problem.

iNecas
  • 1,743
  • 1
  • 13
  • 16
  • ah, nested attributes, it would be indeed an unpleasant work to filter the `params` in this case. – tokland Mar 07 '12 at 11:12
4

Interesting gist from DHH on slicing in controller vs whitelisting alone:

https://gist.github.com/1975644

class PostsController < ActionController::Base
  def create
    Post.create(post_params)
  end

  def update
    Post.find(params[:id]).update_attributes!(post_params)
  end

  private
    def post_params
      params[:post].slice(:title, :content)
    end
end

Comment reinforcing the need to manage this within the controller:

https://gist.github.com/1975644#gistcomment-88369

I personally apply both - attr_accessible with slice to ensure nothing unexpected gets through. Never rely on blacklisting alone!

yellowaj
  • 465
  • 5
  • 10
4

As of Rails 4, slicing the parameters will be the preferred method of dealing with mass assignment security. The Rails core team has already developed a plugin to deal with this now, and they are working on integrating support for nested attributes and signed forms. Definitely something to check out: http://weblog.rubyonrails.org/2012/3/21/strong-parameters/

Joost Baaij
  • 7,538
  • 3
  • 32
  • 34
  • 1
    "The basic idea is to move mass-assignment protection out of the model and into the controller where it belongs.". Nice. – tokland Mar 23 '12 at 16:17
2

Just removing the :name from the params hash works to prevent setting that attribute for that action. It works only for the actions you remember protecting.

However, this practice doesn't protect you from abuse using all the methods automatically added for associations.

class User < ActiveRecord::Base
  has_many :comments
end

will leave you vulnerable for someone setting the comments_ids attribute, even when you delete the comments attribute from params.

Since there are quite a lot of methods added for associations, and since they might change in the future, the best practice is to protect your attributes on the model using attr_accessible. This will stop these kind of attacks most effectively.

Joost Baaij
  • 7,538
  • 3
  • 32
  • 34
  • 2
    Hash#slice is used for whitelisting: {:comments_ids => [1,2,3], :name => 'hello'}.slice(:name) #=> {:name=>"hello"}. I really don't see what could possible go wrong with it (dangerous phrase :-)) – tokland Sep 20 '11 at 10:35
  • 1
    Anyway, as I said, I understand why *attr_accessible* is a good way to handle mass-assignment security. But I was curious what was wrong with a slice, what extra security provides *attr_accessible*? – tokland Sep 20 '11 at 10:41
  • 2
    attr_accessible protects your model in every action, all the time, without you having to remember putting it in each controller. – Joost Baaij Sep 20 '11 at 10:43
  • 1
    I appreciate your answer, all you wrote it's true, but I am not sure that the docs saying "it won’t provide sufficient protection" means "it's not secure because you may forgive to put it". – tokland Sep 20 '11 at 12:41
  • 1
    I'm with @tokland here - can't understand how *Hash#slice* could be less secure than *attr_accessible*. And if you forgot about *Hash#slice*... well, then **forgetting** about Hash#slice is insecure, and not the Hash#slice itself. – Alexis Dec 01 '11 at 14:48
  • Hash#slice would be used to keep specific params, not delete them, so your example doesn't really work – keithepley Mar 12 '12 at 22:25
  • 2
    As of Rails 4, slicing the parameters will become the preferred method to deal with this. http://weblog.rubyonrails.org/2012/3/21/strong-parameters/ – Joost Baaij Mar 23 '12 at 12:54
0

@tokland your last comment is not correct to some extend. Unless your website has the browser as the only entry point where data comes in and goes out.

If your webapp has an API or communicates with other API's protection on the controller level leaves holes behind it and all data from other sources is not sanitised or checked. I recommend keeping the things as they are, turning on mass-assignment protection in application.rb and advancing ActiveSupport FormHelpers to work like Django/Python style.

theCrab
  • 556
  • 5
  • 8
  • which one of my comments? I wrote a lot of them in this page :-) Anyway, I don't see the diferrence between having an API or a browser form. Of course the whole point is to avoid the user updating attributes he is not supposed to by tampering attributes. – tokland May 03 '12 at 16:35
  • @tokland The one where you say "The basic idea is to move mass-assignment protection out of the model and into the controller where it belongs." – theCrab May 21 '12 at 19:43
  • It's a quote from the linked article. But I agree, now I see this as a task for *both* the model and the controller. – tokland Mar 06 '13 at 14:06