Protecting properties from mass assignment

[Ken Robertson]

Hey all,

Looking to add some functionality to DataMapper that is useful in AR.  Bassically, get something similar to attr_accessible/attr_protected, where you could have properties that are public, but prevent them from being updated in mass-updates (ie, Resource.new or resource.attributes=).

As a basic scenario, you have a User model, and you want to be able to do user.isAdmin = true, but you don't want to allow user.attributes = { :isAdmin => true }, or something similar when bulk updating the model with form data.

Currently, you can mark properties as having a private/protected setter, but then both the bulk update is blocked and the direct access is blocked.

I think there is some use for this kind of option and have coded it up in my own fork at http://github.com/krobertson/dm-core/tree/master.  Right now, I called it :mass_assign, but don't like the name, and it accepts true/false (think :allow/:deny is more fitting).

Had talked with Dan on #datamapper some and brainstormed a few things for discussion:

1) Any ideas for a better name?  Dan suggested :accessible, which I kind of like, but open to ideas.

2) What should the default be?  From a security perspective, blocking bulk updates would be the most secure.

3) What about a resource macro that would update the default option for all properties... ie:

class User

  include DataMapper::Resource

  property :id, Fixnum

  property :name, String, :accessible => true

  property :isAdmin, TrueClass

  default_accessibility false

end

Or even have attr_accessible-type macro which makes all protected except the ones given.

4) Currently, I've updated Resource#attributes= which is used with Resource.new and Resource.create, but should Resource#update_attributes also be protected?  It accepts an array of the attributes to update from the hash, but in the absence of the array, it allows all without checking the protection level.

Thanks!

Ken Robertson

(et_ken on #datamapper)

[Dan Kubb (dkubb)]

My preference is we build something that works like attr_accessible
when turned on, and not make the mistake AR did and provide anything
that works like attr_protected. Let someone make a dm-insecure-
attributes plugin if they want that ;)

What I was thinking we could have a class method that we place in our
models, that when used would make every property completely
inaccessible via mass-assignment. If you want to override it and make
it so a property is accessible, you supply the :accessible => true
option. I haven't come up with a good name for this method yet tho,
and am totally up for suggestions.

Dan
(dkubb)

On May 10, 12:08 am, "Ken Robertson" <k...@invalidlogic.com> wrote:
> Hey all,
> Looking to add some functionality to DataMapper that is useful in AR.
>  Bassically, get something similar to attr_accessible/attr_protected, where
> you could have properties that are public, but prevent them from being
> updated in mass-updates (ie, Resource.new or resource.attributes=).
>
> As a basic scenario, you have a User model, and you want to be able to do
> user.isAdmin = true, but you don't want to allow user.attributes = {
> :isAdmin => true }, or something similar when bulk updating the model with
> form data.
>
> Currently, you can mark properties as having a private/protected setter, but
> then both the bulk update is blocked and the direct access is blocked.
>
> I think there is some use for this kind of option and have coded it up in my

> own fork athttp://github.com/krobertson/dm-core/tree/master.  Right now, I

[Ken Robertson]

I agree, I think it'd be good to mimic attr_accessible rather than attr_protected.  What do you think about a method named 'accessible_properties'?

One thing I'm curious about is that there are three very similar functions... attributes=, update_attributes(), and private_attributes=.  I see the usage of attributes= and private_attributes=, since private_attributes is how you get to protected/private ones, but doesn't it raise a bit of a security concern?  Also, update_attributes is basically attributes= and private_attributes= together, as it has no accessibility checking.

Ken

[Nick Plante]

I guess maybe I've been out of the loop on this lately, but I don't see
what the issue is with just marking attribute setters as private/protected
vs public. If you declare something as having protected (or private)
setters, aren't you also implicitly declaring that they should be exempt
from mass-assignment?

It feels like, by providing a way around this, we're sort of voiding the
whole purpose of it in the first place.

But then again, I've been out of the loop as of late, so it's possible I'm
just missing something.

..nap

[Dan Kubb (dkubb)]

The public/private/protected marking is definitely still needed with
this addition. The problem with just relying on it is that if you
mark something as private/protected, its an internal property of the
instance. The only way to access it from your application code is to
break encapsulation and use send(), which IMHO is ugly.

What is needed is a way of marking a property as public, so it's not
an internal property of the instance, while still maintaining control
over whether it is available via mass-assignment.

I think the :reader and :writer options still have their purpose, but
they aren't suited for providing fine grained access control for mass-
assignment; they're main purpose is in controlling access to the
dynamically created methods in a resource.

Dan
(dkubb)

[Dan Kubb (dkubb)]

Hi Ken,

On May 11, 8:19 am, "Ken Robertson" <k...@invalidlogic.com> wrote:
> I agree, I think it'd be good to mimic attr_accessible rather than
> attr_protected.  What do you think about a method named
> 'accessible_properties'?

I like that.

> One thing I'm curious about is that there are three very similar
> functions... attributes=, update_attributes(), and private_attributes=.  I
> see the usage of attributes= and private_attributes=, since
> private_attributes is how you get to protected/private ones, but doesn't it
> raise a bit of a security concern?  

I wondered about that too. I would recommend that once we figure out
the issue of controlling mass-assignment access we look hard at the
Resource#private_attributes and Resource#private_attributes= methods,
and remove them if at all possible. It is marked as private now,
which is a good first step, but it would be alot nicer to have
everything sent through attributes= for simplicity's sake.

> Also, update_attributes is basically attributes= and private_attributes= together, as it has no accessibility
> checking.

That's a good catch. I implements the same basic logic as
Resource#attributes=, and should probably use it under the hood.

On a related note, anyone think the way Resource#attributes= is
implemented is just asking for trouble? Note how we take the key that
was passed-in (potentially from a web form), append an "=", and then
pass it to send() directly? That seems like it could be pretty
dangerous. Probably better to only process the hash entries that
match the names of properties defined within the model.

Dan
(dkubb)

[Nick Plante]

Ah yes, my bad, missed that use case. Thanks for taking a moment to
clarify!

..nap

[John W Higgins]

Good Evening Everyone,

I do apologize if I've stepped on some toes here but I coded up my "solution" to this issue today and it's sitting here git://github.com/wishdev/dm-core.git (http://github.com/wishdev/dm-core/tree/master) at the moment.

I took a little different approach because unlike AR, DM has a nice concept in that we have to implicitly declare the properties we want on our models so I added an option to the property (:mass_updatable) and a default for DataMapper (DataMapper::Resource::default_mass_updatablility). The default is modifiable so you can turn it "on" or "off" per model if you choose. Yes, I know there was some talk about not wanting an "insecure" option - but I figure if a developer makes a implicit choice then we should allow it because it honestly costs us nothing. The default for the moment is that everything is updatable simply because I didn't want to break all the specs and code folks have written to date. If at somepoint there is a decision to make the default more secure then that can be done very easily.

I should note the one thing that completely overrides this is if the setter of a property is declared as private. Obviously in that case one cannot mass-update the property no matter what.

The other weird section came up with associations of all things. Apparantly they run through attributes= to get set and therefore I need to check to see if something is a relationship in order to make sure those pass through without issue. The beauty of full specs certainly helped here because I don't think I ever would have thought about associations when coding this up.

Also, private_attributes= is a special case because it is used when we "clone" a resource. It might not be the right name for its sole use at the moment but there does seem a legit need for something that does what it does :)

Like I said I hope I didn't step on too many toes here but I had a thought process and decided to run with it....

John W Higgins

[Ken Robertson]

Dan,

I wondered about that too.  I would recommend that once we figure out
the issue of controlling mass-assignment access we look hard at the
Resource#private_attributes and Resource#private_attributes= methods,
and remove them if at all possible.  It is marked as private now,
which is a good first step, but it would be alot nicer to have
everything sent through attributes= for simplicity's sake.

I think it is something that might be good to figure out before finalizing accessibility.  Those methods likely need changes for the accessibility, since it wouldn't make sense to leave some open instead of others.

 

On a related note, anyone think the way Resource#attributes= is

implemented is just asking for trouble?  Note how we take the key that
was passed-in (potentially from a web form), append an "=", and then
pass it to send() directly?  That seems like it could be pretty
dangerous.  Probably better to only process the hash entries that
match the names of properties defined within the model.

I had noticed that as well.  I think it'd likely be best to change it to look up the property object by the name given, then have the property expose a 'setter' property, similar to how the getter works.

Ken

[Ken Robertson]

John,

I like having the system default, however I think more would come from having a model-level setting.  I think that is where you'd find it the most useful.  You might not care about a lot of protection on a Post model, or something non-sensitive, but a User model you'd want locked down.

I don't think changing the option before declaring each model is very attractive, since it could change the behavior of following ones without knowing it.  You'd have to specify it in each model file, before the model definition begins, and it is almost like why not define it at the system level in init.rb, then internally to the model when you want it to vary.

Ken

[John W Higgins]

You are absolutely right Ken - I knew there was a piece of the puzzle that I forgot to implement - there is no reason why both won't live in harmony :) I will get that added in ASAP

John

--
John W Higgins
wis...@gmail.com

[John W Higgins]

New version pushed to the fork with a default_mass_updatability option added to the model

John

On 5/11/08, Ken Robertson <k...@invalidlogic.com> wrote:

[Sam Smoot]

You can blame me for patching in #update_attributes. I agree it was
probably not the right move in hindsight.

Lot's of great ideas here obviously, but before we rush to nailing
down the implementation I'd like to give it a day or two to discuss.
Try to get together some of the community deploying DM in projects and
find out what they'd like to see.

I'd also like to take a step back and identify the requirements. Right
now I suspect we're spring-boarding from what DM currently offers, but
let's ignore that for a minute and try to identify what users might
need.

I'll take a stab:

1. Users need a way to mark properties as private
2. Users need a way to mass-assign from hashes for convenience, but
protect certain properties or methods

Feel free to critique, but here's how I imagine such features might
work (again, disregarding DM history):

1. I thought: Why not treat properties like any other method
definition? Since ::property would have to return 2 to 3 Symbols (if
boolean), this idea breaks down. Also, AFAIK, there's no way to know
if you're in a private section of code, so the general form for
visibility (private\ndef foo) won't work either. So...

Ok, so the current :private => true and the like seems like a good
compromise for the (admittedly limited) use case of wanting private
properties.

There is a half-way point here though we might not be
considering... :protected => true.

Forget what protected means in Ruby. I may be about to cuss in church,
but the truth is, it doesn't mean anything useful. It's not idiomatic
Ruby. *Nobody* uses it in their libs. We could probably quiz a
smattering of sharp Rubyists on what it even does with a 10s deadline
and half of them would probably fail.

So why not just steal the term? It seems to apply so cleanly since the
word borrows on the much more common AR idiom:

# property :zoo_id, Fixnum, :protect => true

It would default to false. Because having to specify tainted input
everywhere would be annoying. Which leads me to...

2. #attributes=. Ok, so it's pretty. But #update_attributes gives you
something cool. A second argument to have a little more specific
control. So here's a dramatic shift: We ditch the "=" version. And
since "update" connotes something a bit different than what this is
for, I'd say we change the method name to #set_attributes. So how
about this:

# def set_attributes(input, allowed_methods = nil)
# raise ArgumentError.new("+input+ should be a Hash but was
#{input.inspect}") unless Hash === input
# if allowed_methods
# input.each_pair do |k,v|
# send(k, v) if allowed_methods.include?(k)
# end
# else
# if input.tainted?
# set_attributes(input, properties.public_method_names)
# else
# input.each_pair do |k,v|
# send(k, v)
# end
# end
# end

Even better. Now if Rails' HWIA or Merb's Mash returns tainted? =>
true (which we could patch with plugins if they don't since they
obviously should), then we're safe. And we get the happy convenience
of full accessibility when not dealing with tainted data.

I'm really in love with the idea of taking advantage of different
behavior depending on whether the source is tainted... :-)

...but let me know what you guys think. Like I said. A few heretical
ideas. I'm not expecting anyone to love it instantly. Let's hear those
opinions.

[Ken Robertson]

I was thinking update_attachments should go through attechments=... following on your renaming, be like this:

# def set_attributes(input, allowed_methods = nil)
#   raise ArgumentError.new("+input+ should be a Hash but was
#{input.inspect}") unless Hash === input

#   if allowed_methods

#     self.attributes = input.delete_if { |k| !allowed_methods.include?(k) }

#   else
#     if input.tainted?

#       self.attributes = input.delete_if { |k| !properties.public_method_names.includes?(k) }

#     else

#       self.attributes = input
#     end
#   end

Could be cleaned up more, like rather than multiple delete_ifs, just have some logic to populate allowed_methods.

I do like the idea of taking over :protected/:protect.  That is the best name for it, but was already in use.  If the usefulness of it is week, might as well take it over. :)

Ken

[Nick Plante]

I agree. I don't think there's much harm overloading the meaning of
'protected' in this case. That said, protected isn't useless in Ruby. It's
just not what most people think it is :).

Still, there isn't much of a use case for a protected setters vs private
setts differentiation from DM's point of view. At least, not as far as I
can tell.

..nap

[Jeremy Evans]

On May 11, 11:11 pm, Sam Smoot <ssm...@gmail.com> wrote:
> There is a half-way point here though we might not be
> considering... :protected => true.
>
> Forget what protected means in Ruby. I may be about to cuss in church,
> but the truth is, it doesn't mean anything useful. It's not idiomatic
> Ruby. *Nobody* uses it in their libs. We could probably quiz a
> smattering of sharp Rubyists on what it even does with a 10s deadline
> and half of them would probably fail.

I'm not nobody. :)

protected does have a use, it's to implement a private API that other
members of the same class can use. This is useful in when writing
programs in a functional style, where methods return copies of self
instead of modifying self. The methods shouldn't be public, but can't
be private, because the receiver of the method is not self, but a
member of the same class. See an example in
http://github.com/jeremyevans/sequel/tree/master/sequel/lib/sequel_model/eager_loading.rb

Jeremy

[John W Higgins]

On 5/12/08, Nick Plante <n...@zerosum.org> wrote:

Still, there isn't much of a use case for a protected setters vs private
setts differentiation from DM's point of view. At least, not as far as I
can tell.

..nap

Nick,

The difference is whether I can do the following

a = Model.new()
a.update_attributes( {:field_a => 1, :field_b => 2} ) <- these fields may or may not be updated based on "protection" level

a.field_a = 1
a.field_b = 2

The 2 calls above are where "protected" vs "private" comes into play - private fields cannot be updated in that fashion because the field_a= and field_b= methods are marked as private. In a "protected" concept those methods would be public and still available to the developer. We are attempting to "limit" the ability of something like a "URL" to come along and update fields that never appeared on a form via SQL injection or some other method, while still letting the developer work with a field.

Hope that clears up a confusing concept a little bit :)

John
 

[Nick Plante]

Sorry John, I think we crossed some wires. I was agreeing with you; just
saying that I didn't see much use of having protected setters and private
setters, in the classic *Ruby* sense of the term as is currently provided
by DM.

I see the reasoning for wanting something akin to AR's attr_accessible
here (for mass assignment reasons, almost like two different levels of
"protection" and we're getting our terminology confused). Although I still
sort of feel that those mass-assignment rules are kind of an ugly hack,
being able to do that and include/exclude attributes is none the less
convenient and I can't think of a better strategy.

Perhaps the terminology is the biggest issue of all, since my message
before got confused :). Maybe that translates to a vote that :protected
shouldn't be overridden after all. Maybe :accessible => true (default to
false) isn't a bad way to go.

Hope that's clear :).

..nap

[John W Higgins]

On 5/11/08, Sam Smoot <ssm...@gmail.com> wrote:

# def set_attributes(input, allowed_methods = nil)
#   raise ArgumentError.new("+input+ should be a Hash but was
#{input.inspect}") unless Hash === input
#   if allowed_methods
#     input.each_pair do |k,v|
#       send(k, v) if allowed_methods.include?(k)
#     end
#   else
#     if input.tainted?
#       set_attributes(input, properties.public_method_names)
#     else
#       input.each_pair do |k,v|
#         send(k, v)
#       end
#     end
#   end

My concern with this concept is more philosophical then anything. I do like the idea of having a "backdoor" that gives developers the full ability to use whatever functionality they choose in their code while limiting things when desired (the tainted? concept above). However, I really distrust other folks in this scenario and do not like trusting merb or any other thing other then DM controlling my "backdoor". I prefer hiring my own security guard to watch the door if I'm going to create one :)

That said, what about the following. If we had a function like the one above but with the default being "unprotected/mass-updatable" fields then when a regular call comes along it is locked down no matter where it comes from. However, we could easily add another method which simply calls into set_attributes with a more liberal list of available fields that can be updated. Yeah, it doesn't seem like that big a difference but I believe it has some strong control functionality.

First, it's an implicit choice by a developer as to which call to make. Something like "was the hash tainted" is rather difficult to track down in code. Looking for a specific method call is much easier and clearer to hunt down if needed. It also gives a developer the choice to in essence remove the back door by either undefining, or making the method private, or any other number of ways to hide a function.

There is another option which I like a lot which is a class method which calls down into "set_attributes" with the more liberal list of properties that can be updated. The advantage of something at the class method level is that it could be turned off or on based upon the developer choice at time of creation. So it's possible that TableA would allow overrides of the protection and TableB would be on complete lockdown and not allow standard calls to override the protection scheme as placed on the model (Ruby never allows one to completely say never :) ). This wouldn't add any overhead because it can be done with a single call at time of creation of the class. I personally prefer this option the best of anything at the moment. It gives me my security guard and I can arm him rather heavily if I so desire :)

John

[Ken Robertson]

John,

There is another option which I like a lot which is a class method which calls down into "set_attributes" with the more liberal list of properties that can be updated. The advantage of something at the class method level is that it could be turned off or on based upon the developer choice at time of creation. So it's possible that TableA would allow overrides of the protection and TableB would be on complete lockdown and not allow standard calls to override the protection scheme as placed on the model (Ruby never allows one to completely say never :) ). This wouldn't add any overhead because it can be done with a single call at time of creation of the class. I personally prefer this option the best of anything at the moment. It gives me my security guard and I can arm him rather heavily if I so desire :)

Let me see if I got your last suggestion right, since if so, I might like it most of all.

We could basically have a class method that sets a default list of attributes that set_attributes would use.  So my model might be:

class User

  ...

  set_attributes_default :id, :name # so it won't update the password / isAdmin properties

end

So then set_attributes would:

1) Update only the specified attributes, if a list of specific attributes was given

2) If no specific list was given, default to the ones set in the model... eg, it would use the [:id, :name] I set in my model

3) If set_attributes_default was never specified, it would allow any public property

That correct?

If so, I do like that approach, and it is pretty simple and straight forward.  It is basically AR attr_accessible without property definition overrides, which might be the most plainly simple way anyway.

I do think it should be done at a level that protects attributes= some, since someone might still do User.new(params[:user]) without thinking.

I think a lot is being lost in naming too... property names can get messy, with the public/protected/private/mass-update stuff.  Maybe just look at how can users shoot themselves in the foot, and what is the simplest way to prevent the foot injury.

Ken

[John W Higgins]

Hey Ken,

While I didn't express my 'super method" concept properly - lets first discuss your 3 bullet points and then move along from there

1) Update only the specified attributes, if a list of specific attributes was given

Absolutely - you are the final controller of the function and can do as you please

2) If no specific list was given, default to the ones set in the model... eg, it would use the [:id, :name] I set in my model

Again, this would be the starting point of any conversation and is certainly provided - I have it currently on a property by property assignment but a "set_protected_properties/set_unprotected_properties" macro could easily be added in. I prefer property by property because one of the beauties of DM is that we control what it looks at as opposed to AR's get everything from the schema (and I'm not saying they are wrong - we are just different), and therefore I like the ability to control things piece by piece.

3) If set_attributes_default was never specified, it would allow any public property

This is available using either a DataMapper::Resource default or a Class level default mechanism - it's also possible to fully to the opposite and make it so the method will not assign to any property (ie a complete lockdown). The choice is yours as a developer.

As to my "super method" let me give you an example of what I've got setup


class ClassA
  include DataMapper::Resource

  property :id, Fixnum, :key => true
  property :name, String
  property :not_updatable, String, :protection_level => :protected
  property :updatable, String, :protection_level => :unprotected
end

NB - the naming conventions are certainly up for grabs

So for ClassA - we can do the following

b = ClassA.new()
b.attributes=( {:not_updateable => 'bob', :updateable => 'cobb'} )

Which would give us an object that looks like this

b.updatable == 'cobb'
b.not_updateable.nil? == true <- because we have that as a protected property

So lets assume I want to REALLLLLY make a call like the attributes= call to setup this object, while still maintaining some control for things coming from Merb or someplace else.

In comes the following call
b.class.attributes=( b, {:not_updatable => 'bob', :updatable => 'george'} ) which will set the not_updatable to 'bob' for us while also changing :updatable to 'george'

I view the call sort of like asking a supervisor to enter their code to override a system protection level. Again at both the DataMapper::Resource level and the class level this type of override can be turned on or off.


Is it in the end a little verbose and somewhat complex, yes it is. I don't think however that if one sets up security options that methods around those options should be the easiest things in the world. The 3 bullet points you had, which I imagine are the vast majority of folks take very little to accomplish and the extra layers do not inhibit those folks at all. However, if you want more granularity, more granularity you shall be allowed.

Again, I respect Sam's thought that discussion is a great thing - I just work best when my wild ideas get a little more crystalized to make sure I'm not talking out of my butt :)

John W Higgins

[Dan Kubb (dkubb)]

Do you guys mind putting together your ideas in a dm-more plugin, so
we can all see real code and help it become mature enough to include
into dm-more?
For the most part I think we're on the same page though, but it'd be
hard to add more to the conversation without having a plugin we can
play with and include in projects to see how it works in real-world
cases.

Sam's idea of tainting is a separate issue I think, something
independent of what we're talking about here. Ruby gives us this
wonderful taint concept, and its up to us whether or not to use it.
Luckily ruby also gives us a $SAFE flag that is normally set to 0, but
when set to 1 instructs ruby core methods to not permit tainted
objects to be used when performing potentially unsafe operations. For
example you can't hand a tainted string to a system() call. Other
languages that support taint, like perl, view database operations as
potentially unsafe and all their database drivers do not permit
operations with tainted values

I worked with using Taint in CGI and mod_perl scripts for about 4 or 5
years, and I swear by it. I was sad to see it not embraced by the
Ruby community when I started learning Ruby.

Regardless of what we do with mass attribute access control, I think
the DO drivers should be updated so that if SAFE is >= 1 that database
operations not be permitted if any of the objects are tainted, which
includes the DML/DDL statement or any bind values. The same goes for
any non-DOA adapters. We can also update dm-validates so that once an
attribute is validated that its taint flag is turned off. Then when
those attributes are passed to the adapters they will be taint-free
allowing the query to be performed. This will ensure that every
attribute is validated properly before inserting, or updating it in
the DB. Keep in mind this behavior is only when $SAFE >= 1, which can
only be done explicitly -- I think its safe to say if someone sets
this flag they would want this type of behavior from their database
drivers.

Dan
(dkubb)

[tonymi]

Is this feature available in edge or as a plugin somewhere?

[Sam Smoot]

FYI, #tainted? is a no-go in Rubinius. :(

So... in Merb, use the param_protection plugin.

For Rails, we'll need to make due with :private => true for a couple
more weeks while Wieck gears up to put a few hours into DM.

-Sam

[John W Higgins]

The following ticket has my current patch against edge DM for adding protection to properties against mass-update - naming conventions and even the concept is not locked down by any means so use at your own peril. I hope it's at least a starting point for the final solution.

http://wm.lighthouseapp.com/projects/4819-datamapper/tickets/329-patch-protection-level-for-properties

John W Higgins

[Ken Robertson]

I haven't worked on it at all recently.  I've been on vacation for the past two weeks and barely coding or on IRC. :)

For the project I was working on, I just started using the param_protection plugin for Merb.

Ken

[David James]

Just catching up on this thread. I think the way attr_accessible is
implemented in ActiveRecord is ugly. I explain why here:
http://djwonk.com/blog/2008/05/23/improving-attr_accessible/

Hopefully DataMapper is not going in the direction of ActiveRecord in
this case.... :/

On May 30, 9:36 pm, "John W Higgins" <wish...@gmail.com> wrote:
> The following ticket has my current patch against edge DM for adding
> protection to properties against mass-update - naming conventions and even
> the concept is not locked down by any means so use at your own peril. I hope
> it's at least a starting point for the final solution.
>

> http://wm.lighthouseapp.com/projects/4819-datamapper/tickets/329-patc...