Note to readers from the future: this is a non-issue now that by default, rails complains abouts calling model#update_attributes without first setting up your attr_accessors
By default, rails allows you to do this in your controller:
params are your http request parameters
Model#update_attributes updates a record in your database.
If it's not obvious why this is more than a little wreckless, let's give you a more concrete example.
# models/order.rb class Order # has a user_id field, # to determine the customer that this order belongs to belongs_to :user end # controllers/orders_controller.rb class OrdersController def update order = Order.find(params[:id]) if current_user == order.user order.update_attributes(params) else raise 'Stop trying to modify other users orders!' end end end
At first glance that code looks secure because you're authorizing your user (In practice you would use a gem like cancan that makes authorization a lot cleaner). It does make sure you can only modify your own orders.
But it doesn't whitelist exactly which fields you can modify. Imagine you sent these fields to that endpoint:
order_id: 123, user_id: <some other poor users user_id>
You've assigned your order to another user!
Look familiar? It should. A certain rather popular company with exactly one metric buttload of developers as users fell prey to this exact bug. I'm a big fanboy of theirs so I won't repeat exactly who it was, but they're huge.
Messeurs Picard and Riker sum up the feeling when you discover a bug like this in your code:
This is not a bug in rails. It's just a bit of functionality that makes it quite easy to stab yourself in the face.
Rails also gives you an easy way to white list attributes that can be mass
assigned in the
attr_accessible class method:
# models/order.rb class Order belongs_to :user attr_accessible :created_at, :address_id # and so on end
The result of this is that if you try to mass assign
anything that isn't declared with
exception is thrown, your app assplodes and that is the right thing.
The whitelist only comes into effect when you call
attr_accessible on at
least one attribute.
This means that in practice we neglect to whitelist anything and tend to leave this sort of security vulnerability lying around in our Rails codebases.
Guilty until proven innocent
On any new project then, I add this initializer:
# app/initializers/disable_mass_assignment.rb ActiveRecord::Base.attr_accessible(nil)
This automatically switches on whitelisting for all your model objects, making sure I whitelist all of my models before I mass assign them.
(Note, according to the relevant rails guide you can achieve the
same effect with the following config option in
config.active_record.whitelist_attributes = true
Use Only What You Need
Even with that in place and, it feels a little, well, wrong to just stick
query parameters unchecked into
update_attributes. I'm with DHH on filtering
your query parameters like so:
It's definitely a controller concern, though I wouldn't hesitate to move the fine details of the filtering logic somewhere else if it got to be more then four or five lines of code.