By default ActiveRecord allows visitors access to any writer method, that is, any method ending with an equal sign. This comes courtesy of the ActiveRecord::Base#attributes= method, which is used internally by the main methods that handle creating and updating records, including new(), create(), and update_attributes().

• http://railspikes.com/2008/9/22/is-your-rails-application-safe-from-mass-assignment

This topic recently came up on the SF and East Bay Ruby meetup lists (original message here, associated blog post here). I started writing a response in my email client and decided it was big enough that it would benefit from a blog post.

Abstract: Even though attr_protected and attr_accessible exist, models
shouldn’t be filtering — controllers should be. Filtered params should be defined on the model for locality of reference, not on the controller. Hash#slice and Hash#except aren’t enough because they don’t handle nested hashes, so I explore a recursive filter (blacklist & whitelist).

Model-level access controls are bad

There’s a lot of talk about attr_accessible and attr_protected, but I think this approach is fundamentally wrong. Models should not be doing security filtering; they don’t (and shouldn’t) have access to enough information (the current user’s access priveleges / roles) to do this intelligently. When the controller says “model, here are your parameters,” the model should not be filtering. I’ve worked with code that tried to do user based security in the model and it’s a super-bad plan because it totally breaks MVC. One of the important functions of the controller in MVC is to handle things like user authentication, and param restriction/filtering is part of that responsibility.

Additionally, the attr_accessible / attr_protected approach yields situations like

class WidgetController < ActionController::Base
  def update
    @widget = Widget.find params[:id]
    @widget.attributes = params[:widget]
    if logged_in? && current_user.admin?
      %w(sensitive_var_1 sensitive_var_2).each do |var|
        if params[:widget].key? var
          @widget.send :"#{var}=", params[:widget][var]
        end
      end
    end
    @widget.save
    redirect_to @widget
  end
end

This pollutes the model with information about what is filtered and the send is redundant to attributes=.

Solution 1

Use Hash#slice and Hash#except to filter params before they get to the model.

class WidgetController < ActionController::Base
  def update
    @widget = Widget.find params[:id]
    unless logged_in? && current_user.admin?
      params[:widget].except! :sensitive_1, :sensitive_2
    end
    @widget.update_attributes params[:widget]
    redirect_to @widget
  end
end

“But,” you say impetuously, “my controller shouldn’t have to know about my model’s sensitive bits! That’s ugly.” Well, you’re right.

Solution 2

Keep it in the model

So here’s what we’ll do:

class << ActiveRecord::Base
  attr_accessor :sensitive_params
end

class Widget < ActiveRecord::Base
  self.sensitive_params = [:sensitive_1, :sensitive_2]
end

class WidgetController < ActionController::Base
  def update
    @widget = Widget.find params[:id]
    unless logged_in? && current_user.admin?
      params[:widget].except! Widget.sensitive_params
    end
    @widget.update_attributes params[:widget]
    redirect_to @widget
  end
end

So here’s a halfway ground between the attr_accessible/_protected approach and controller filtering. The model is the only place that contains the sensitive params, keeping the controller fairly abstract, but the controller is still the one doing the access control. Obviously, this could be dressed up with a good bit of syntactic sugar and extended to support blacklisting as well as whitelisting.

But what if we need to filter two models at the same time, as with accepts_nested_attributes_for?

Solution 3

Recursive black-/white-listing

A while ago, I hacked out this code to deal with recursive filtering:

class Hash
  def whitelist(*list) filter list, :type => :whitelist end
  alias_method :&, :whitelist
  def whitelist!(*list) replace whitelist(*list) end

  def blacklist(*list) filter list, :type => :blacklist end
  alias_method :^, :blacklist
  def blacklist!(*list) replace blacklist(*list) end

  def filter(list, options = {})
    list.flatten! if list.length == 1 && list.first.is_a?(Array)
    options[:type] ||= :whitelist
    unless [:whitelist, :blacklist].include?(options[:type])
      raise "filter type #{options[:type]} unknown"
    end

    inject Hash.new do |h, (k, v)|
      relevant_hash = v.respond_to?(:filter) && list.detect{|i| i.respond_to?(:[]) && i[k]}
      listed = list.include? k
      if (options[:type] == :whitelist) ? (relevant_hash || listed) : !listed
        h[k] = relevant_hash ? v.filter([relevant_hash[k]], options) : v
      end
      h
    end
  end
end

This would let you whitelist or blacklist nested hashes, so you could

class << ActiveRecord::Base
  attr_accessor :blacklist_params
end

class Dongle < ActiveRecord::Base
  has_many :widgets
  self.blacklist_params = [:wibble]
end

class Widget < ActiveRecord::Base
  belongs_to :dongle
  accepts_nested_attributes_for :dongle
  self.blacklist_params = [:admin, {:dongle_attributes => Dongle.blacklist_params}]
end

class WidgetController < ActionController::Base
  def update
    @widget = Widget.find params[:id]
    unless logged_in? && current_user.admin?
      params[:widget].blacklist! Widget.blacklist_params
    end
    @widget.update_attributes params[:widget]
    redirect_to @widget
  end
end

Now, this would also catch params[:widget][:dongle_attributes][:wibble] and still have all the filtering in the controller.

Wrapping up

Backing out of the rabbit hole

Disclaimers: The above are intended to be sketches / expressive hacks intended to communicate a pattern, not code to be taken seriously. I haven’t played with this in any production code, since I haven’t refactored away from attr_accessible/attr_protected yet. I’ll update this when that happens.

The hacks above could pretty easily be wrapped up (and tested) into a simple little plugin, but first I want to play with the ideas a bit and see if they stick. The general approach seems similar to param_protected but I think it’s pretty important that the model contain the definition of the filtered params for locality of reference.

For now, I’m curious what people think about the general strategy for parameter filtering. Has this been covered a thousand times? Obvious flaws? Add comments below.