Best Practices: a strong case for attr_accessible part 2
Posted by trevor Wed, 24 May 2006 22:31:58 GMT
This is a followup of part 1 which you should read before continuing here.
So… are the requirements satisfied? No.
It’s possible for any user to delete a project that they don’t actually own. More specifically, it’s possible for a user to arbitrarily assume ownership of any project they know the id of. After that, the project is theirs to do with as they choose.
The code that allows you to assume project ownership is here:
class UserController < ApplicationController
def save
current_user.update_attributes(params[:user])
end
end
class User < ActiveRecord::Base
attr_protected :is_administrator
has_many :projects
end
Spotted it yet?
Okay, here we go.
The attr_protected and attr_accessible macros control what is accepted for mass-assignment. Read those links if you’re not familiar with the difference between those two macros.
What’s important to understand is that those macros have nothing to do with database fields and everything to do with methods that have a name ending in ’=’ and that accept a single argument.
Which means that you can do this:
class User < ActiveRecord::Base
def wibble=(arg)
puts "someone assigned to wibble: #{arg.inspect}"
end
end
User.new(:wibble => 'hey ho')
# or
User.find(1).update_attributes(:wibble => 'hey ho again')
With me so far? Good. Here’s the kicker. How many methods are there in your models that can be called via mass assignment?
Did you know that our User model has a method called ‘project_ids=’ (supplied by our has_many macro)? It accepts an array of integers that identify the id for the projects that should belong to the user. And it will quite happily update existing Project records to point the user_id field at their new owner.
Which means that if we want to cause something that looks like this:
current_user.update_attributes(:project_ids => [5])
We simply have to craft a URL parameter for UserController#save that looks like this:
user[project_ids][]=5
And that’s not difficult at all.
So, what’s the solution? Well, my opinion is pretty strong here. There is much magic in rails, some of it you might never use (seriously, how many of you knew about the _ids= thing for has_many?). Writing attr_protected :project_ids would patch this particular hole but you might be exposed in other ways that you’re not aware of.
I’d say that this makes a pretty strong case for using attr_accessible (rather than attr_protected) if you’re at all concerned about security.
Yep, it violates DRY in a small way but it does adhere to a sound security principal: everything should be implicitly denied unless explicitly allowed.
Anyhow, thanks to everyone that had a crack at this.
Whoa... scary. A patch to implicitly set collection attributes to attr_protected would cover a lot of the bases though...
darn... right idea, wrong way of fixing it maybe I'll upgrade myself to Rails Novice :)
i think it´s actually a strong case for not being stupid. if you know that update_attributes works as intended (which it does) and you are working with sensitive data (which i take you do)and your app requires that an arbitrary user is not allowed to assign arbitrary projects to himself (which also seems to be the case) i would actually make a strong case for some carefully crafted if statements ;)
You're wrong tender. Trevor's example was arbitrary, and certainly not a cover all. Being able to arbitrarily update an object's collection ids (or worse, its 'belongs_to') via update_attributes (which is widely touted as 'best practice' in all the rails examples) is not desired default behaviour. The fact that you have to go through every collection you have to set attr_protected on every #{collection_name}_ids is also silly. IMO, all collection attributes should be set to attr_protected by default - the behaviour is too obscure to be set open by default. I've been doing Rails work as my full time job for the last nine months, and I didn't know about the #{collection_name}_ids array until today.
David - actually it's pretty cool that I stumped you given how much quality rails code I know you have under your belt. And it's also why I disagree with you. Something will come along in the future and surprise you again despite your rails-fu. Seriously, moving forward you should either stop using mass assignment (ugh, that sucks) or start setting attr_accessible in all of your models (my personal choice). Tender - unless the result of your "carefully crafted if statements" was to purge entries from params I can't see how it helps. Could you clarify?
david, i don´t really want to argue on right or wrong here.. since i don´t know how the rails people intended update_attributes to function. if they intended it to only work on "non collection" data then clearly it´s a bug. i´ve been doing ruby/rails work in my day job about the last year, too.. and i´ve always used this method with extreme caution.. fearing that it would work as advertised (namely updating every user property.. including the 1:n and n:n relationships) but as you say.. if it´s not the intended default behaviour.. it´s a bug and you should file a bug report. i´ve just never taken rails as a framework that would protect me from malicious post data so i acted accordingly and if people tout update_attributes as best practice right now even for unchecked post data on sensitive models they are wrong.
tender - okay, so you don't use update_attributes. Do you use new(params[:user]) or model.attributes = params[:user] sort of thing?
trevor, at the moment, no. and i think this thing is a non issue, really. i can imagine an application where arbitrary users can assign arbitrary projects to themselfs and then delete them. not an issue there.. i would just use update_attributes and go my merry ways, since update_attributes would do exactly what i want to. if there´s a field i don´t want the user to update on his/her own i would just check it before i enter it into the database. in this case i would just check if the project is assigned to another user and if it is, refuse to delete it. my point is merely that rails has no way of knowing what you want a user to be able to do and what not... just because rails offers you an opportunity to map post data 1:1 to the database doesn´t mean it can figure out your application logic by intuition. that´s what i meant with stupiditiy... rails just takes care of naming conventions and the db absctraction layer.. you still have to write the app yourself.
tender, that you don't do mass-assignment from request parameters explains a lot about your attitude here (otherwise I'd have to question whether you actually understood the code I presented). From what I've seen, mass assignment (new/create/attributes=) using request params is common practice. It's also common practice to use attr_protected to exclude sensitive items from mass assignment. This is very much still "writing the app yourself". My "strong case for" posts are simply a concrete example that backs up an opinion I've held for quite some time - attr_proteced requires you to know too much about rails internals to be safe - use attr_accessible instead. Like you said though, you don't use mass assignment from params so this really doesn't apply to you. Regardless, thanks for taking the time to read and comment :-)