Security by Stupidity I: Rails and attr_accesible

Sometimes you get surprised how frameworks and/or applications messes things up when trying to deal with security. Here is one example.

Context

Framework: Rails
Stupidity: attr_accesible/attr_protected
Output: Annoyed developers and/or missing data.

Explanation

The rails manual suggest declaring on your models what attributes are supposed to be protected (or accessible). Where protected/not accessible means not directly set from parameters coming from the outside.

This is stupid, because it works at the wrong layer: it is not the model the one that talks with the outside; that is the controller. So, is in the controller where we have to sanitize everything that comes from the outside and remove everything that should not pass to the inside of the application. Letting something dangerous pass this layer is simply a bad idea. Doing tricky stuff to catch this leak in a deeper layer is not going to fix it.

The first practical problem is that, as in every tricky situation, it is easy to miss something. In this case, a missing field in one of these lists gives either a security or a data-losing problem.

More important, even supposing that everything is consistent, this "solution" effectively limit the "mass assignment" methods (new , update_attributes and who knows what others). That may seem OK if you only use such methods for passing insecure data coming from HTTP, but chances are that you may find them useful for ther things. I had to do data loading coming from CSV files this week, and didn't expect that update_attributes may ignore some attributes. Principle of least surprise anyone?

The good thing here is how ridiculous easy is to program the attr_accessible functionality. Unless I'm missing something, the practical effect of attr_accessible is to filter out some keys from some hash:

def clean_input(input_hash, allowed_keys)
    clean_hash = {}
    input_hash.each do |k, v|
        if allowed_keys.include? k
            clean_hash[k] = v
        end
    end
    clean_hash
end

So, instead of this (plus the corresponding attr_accesible definition on the model):

  User.new(@params['user'])

It would be:

  user_attrs = clean_input @params['user'], 
                           ['name', 'password', 'email', 'signature']
  User.new(user_attrs)

It may be my Python background, but I firmly believe that is this case, explicit is far better than implicit (and wrong).