Jon Leighton

Mass assignment security shouldn't happen in the model

Mass assignment security is a feature in Active Model (and therefore Active Record) which allows you to be selective about what input you will accept when ‘mass assigning’ a hash of attributes.

Typically, you will have a form that submits to a controller update action, which simply assigns all the form parameters to the model:

@user.attributes = params[:user]

Any keys in params[:user] which are not allowed (e.g. an ‘admin’ flag) will be silently discarded.

Problems

This is great, of course, because nobody wants to get hacked. However if you have been writing Rails applications for long enough you will come up against some problems:

  1. Creating records in tests. Often when testing models, you will want to create a model with a given set of attributes, and then interact with it in the actual test. The quickest way to do this might be

    user = User.new(:admin => true)

    but if you have the admin attribute protected, it won’t work. Worse, you won’t know it hasn’t worked: the attribute is just silently ignored.

    There are some ways around this:

    (In general, this problem is better described as ‘interacting with records somewhere other than a controller’.)

  2. Different rules for different people. Often there are different ‘roles’ for users in an application. For example, an ordinary user should be able to change her password, but not promote herself to an admin. However, an admin should be able to promote another user to be an admin.

    This has historically caused problems with mass assignment security, so in Rails 3.1 (not yet released), there is a mechanism for specifying ‘mass assignment roles’.

    Essentially, you can specify different lists of allowed attributes, and give them a name. Then, when an admin user is editing a user, you can use their ‘list’ via the :as option:

    @user.assign_attributes(params[:user], :as => :admin)
  3. Active Record has to work around mass assignment security internally. This isn’t so much an end user problem, but in the past few months I have fixed some very nuanced bugs within Active Record. These bugs were either caused by mass assignment security kicking in where we didn’t want it to, or by subtle mistakes in the logic when we tried to work around the mass assignment security. (If you’re interested, see the discussion in these bugs and these commits.)

Put it in the controller

Mass assignment security is a feature that is aimed at dealing with form input. By placing it in the model, we end up making the implicit assumption that all ‘normal’ interaction with the model happens via a HTTP request. This assumption is incorrect and causes problems.

I believe that the filtering should happen in the controller, which is the area of the application which is supposed to deal with HTTP input. Here’s a rough sketch of how it might work:

class ActionController::Base
  include ParamSecurity

  before_filter :filter_params

  def filter_params
    self.params = param_filters.inject(original_params) do |mem, filter|
      filter.process(mem)
    end
  end

  # Define filter(s) which operating on incoming params. Can be overridden to
  # extend/customise the filter list.
  def param_filters
    [param_security_filter]
  end
end

module ActionController::ParamSecurity
  extend ActiveSupport::Concern

  module ClassMethods
    included do
      class_attribute :param_security_rules
    end

    # Some sort of dinky DSL so you can do e.g.
    #
    #   param_protected :user => [:role, :email]
    #
    # or
    #
    #   param_accessible :user => [:name, :password], :only => :create
  end

  # Can be overridden if necessary
  def param_security_filter
    ParamSecurityFilter.new(self.class.param_security_rules, action_name)
  end
end

Why this is better

What do you think?

I am not the first to make these observations. Merb had a plugin to do this, and a similar plugin has been written for Rails.

But mass assignment security is one of the first things a new Rails user learns, and it’s a feature that is present in almost every single Rails application. So personally I would love to see this changed in Rails core itself.

Of course, it’s not that simple because we have to think about backwards compatibility, and it could be quite annoying for Rails users to have to move the filtering rules out of their model and into the controller.

So I am not sure whether this is really worth changing, but I would like to hear your thoughts!

14 August 2011

Comments