Happy Bear Software

How I Avoid The Rails Mass-Assignment Security Mistake

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

Wat?

By default, rails allows you to do this in your controller:

model.update_attributes(params)

Where 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:

Double facepalm 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 attr_accessible, an 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 application.rb):

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.