Rails core - Jul 2012 - attr_accessible on some properties + attr_protected on others makes class 'open-by-default'

(I posted this as a bug in GitHub 
(https://github.com/rails/rails/issues/7018), but then someone there told 
me I should post it here, so here it is.)

If you set attr_accessible on some properties in an ActiveRecord-descended 
class, and then attr_protected on others - the class becomes
''default-open''
- if any properties are missed or added later, they will be accessible by 
default to MassAssignment.

This undoes the entire point of having put attr_accessible in one''s
class.

Two possible solutions -

#1) ''default-closed'' - the attr_protected statements will
either be
ignored, or just used to override attr_accessiblefor a particular property.
#2) ''explicit-only'' - any attribute accessed in
mass-assignment that is not
explicitly mentioned in eitherattr_accessible or attr_protected raises a 
new error - something like
 MassAssignmentError:AttributeNotExplicitlyDeclared. Maybe even throw an 
error if the attribute is accessed in*any* way
(mything.whatever="boo"; #
kerplow! throws error?) though that might perform poorly.

Solution #1 is probably fine - accesses to not attr_accessible properties 
will throw a MassAssignment error under these circumstances anyways. 
Solution #2 just makes things really explicit, which some might want for 
some kinds of high-security applications.

I found this bug in my own code during the development cycle; I liked 
putting both attr_accessible andattr_protected in for symmetry and to 
remind me of my DB schema at the top. Stupid reason, I know. I found that a 
belongs_to relation was unprotected in that circumstance.

-B.

-- 
You received this message because you are subscribed to the Google Groups
"Ruby on Rails: Core" group.
To view this discussion on the web visit
https://groups.google.com/d/msg/rubyonrails-core/-/cKUcUmVToewJ.
To post to this group, send email to rubyonrails-core@googlegroups.com.
To unsubscribe from this group, send email to
rubyonrails-core+unsubscribe@googlegroups.com.
For more options, visit this group at
http://groups.google.com/group/rubyonrails-core?hl=en.

I personally think we should deprecate attr_protected, and go with
whitelisting only (attr_accessible + strong_parameters) route. I think
it make more sense from the security standpoint, and all the exploit
we have seen.

Core teams, wdyt?

- Prem

-- 
You received this message because you are subscribed to the Google Groups
"Ruby on Rails: Core" group.
To post to this group, send email to rubyonrails-core@googlegroups.com.
To unsubscribe from this group, send email to
rubyonrails-core+unsubscribe@googlegroups.com.
For more options, visit this group at
http://groups.google.com/group/rubyonrails-core?hl=en.

Just a guy with an opinion weighing in... I would love to see 
attr_protected removed. The official Rails Guide on
security<http://guides.rubyonrails.org/security.html#countermeasures>
calls
attr_accessible "A much better way", and I don''t think
Michael Hartl''s
popular Ruby on Rails Tutorial <http://ruby.railstutorial.org/> even 
mentions attr_protected. I think it gives people a false sense of security, 
especially in a large application where it''s easy to forget to update
it
when new fields are added.

- Pete

On Monday, July 9, 2012 9:38:12 PM UTC-4, Prem Sichanugrist
wrote:
>
> I personally think we should deprecate attr_protected, and go with 
> whitelisting only (attr_accessible + strong_parameters) route. I think 
> it make more sense from the security standpoint, and all the exploit 
> we have seen. 
>
> Core teams, wdyt? 
>
> - Prem 
>
-- 
You received this message because you are subscribed to the Google Groups
"Ruby on Rails: Core" group.
To view this discussion on the web visit
https://groups.google.com/d/msg/rubyonrails-core/-/bX4JiC2P5rMJ.
To post to this group, send email to rubyonrails-core@googlegroups.com.
To unsubscribe from this group, send email to
rubyonrails-core+unsubscribe@googlegroups.com.
For more options, visit this group at
http://groups.google.com/group/rubyonrails-core?hl=en.

For the record: I don''t mention attr_protected at all in Rails 3 in
Action either.

+1 to removing attr_protected. 


On Tuesday, 10 July 2012 at 11:57 AM, Peter Brown wrote:
> Just a guy with an opinion weighing in... I would love to see
attr_protected removed. The official Rails Guide on security
(http://guides.rubyonrails.org/security.html#countermeasures) calls
attr_accessible "A much better way", and I don''t think
Michael Hartl''s popular Ruby on Rails Tutorial
(http://ruby.railstutorial.org/) even mentions attr_protected. I think it gives
people a false sense of security, especially in a large application where
it''s easy to forget to update it when new fields are added.
> 
> - Pete
> 
> On Monday, July 9, 2012 9:38:12 PM UTC-4, Prem Sichanugrist wrote:
> > I personally think we should deprecate attr_protected, and go with 
> > whitelisting only (attr_accessible + strong_parameters) route. I think
> > it make more sense from the security standpoint, and all the exploit 
> > we have seen. 
> > 
> > Core teams, wdyt? 
> > 
> > - Prem 
> -- 
> You received this message because you are subscribed to the Google Groups
"Ruby on Rails: Core" group.
> To view this discussion on the web visit
https://groups.google.com/d/msg/rubyonrails-core/-/bX4JiC2P5rMJ.
> To post to this group, send email to rubyonrails-core@googlegroups.com
(mailto:rubyonrails-core@googlegroups.com).
> To unsubscribe from this group, send email to
rubyonrails-core+unsubscribe@googlegroups.com
(mailto:rubyonrails-core+unsubscribe@googlegroups.com).
> For more options, visit this group at
http://groups.google.com/group/rubyonrails-core?hl=en.
-- 
You received this message because you are subscribed to the Google Groups
"Ruby on Rails: Core" group.
To post to this group, send email to rubyonrails-core@googlegroups.com.
To unsubscribe from this group, send email to
rubyonrails-core+unsubscribe@googlegroups.com.
For more options, visit this group at
http://groups.google.com/group/rubyonrails-core?hl=en.

Sometimes you have a table with a bunch of regular data and one single FK
to protect. I don''t think forcing users to whitelist that model is a
good
idea.

I prefer Rails to provide both options (three if you count declaring
nothing) and leave the judgement of what''s appropriate in every
situation
to the user.

-- 
You received this message because you are subscribed to the Google Groups
"Ruby on Rails: Core" group.
To post to this group, send email to rubyonrails-core@googlegroups.com.
To unsubscribe from this group, send email to
rubyonrails-core+unsubscribe@googlegroups.com.
For more options, visit this group at
http://groups.google.com/group/rubyonrails-core?hl=en.

I''d like to see attr_protected stick around. There are times
I''m working with models and I don''t want to communicate the15
fields that can be written to but rather the two fields that can''t.

Best. 
Mike

On Jul 10, 2012, at 1:45 AM, Ryan Bigg <radarlistener@gmail.com> wrote:
> For the record: I don''t mention attr_protected at all in Rails 3
in Action either.
> 
> +1 to removing attr_protected.
> On Tuesday, 10 July 2012 at 11:57 AM, Peter Brown wrote:
> 
>> Just a guy with an opinion weighing in... I would love to see
attr_protected removed. The official Rails Guide on security calls
attr_accessible "A much better way", and I don''t think
Michael Hartl''s popular Ruby on Rails Tutorial even mentions
attr_protected. I think it gives people a false sense of security, especially in
a large application where it''s easy to forget to update it when new
fields are added.
>> 
>> - Pete
>> 
>> On Monday, July 9, 2012 9:38:12 PM UTC-4, Prem Sichanugrist wrote:
>>> 
>>> I personally think we should deprecate attr_protected, and go with 
>>> whitelisting only (attr_accessible + strong_parameters) route. I
think
>>> it make more sense from the security standpoint, and all the
exploit
>>> we have seen. 
>>> 
>>> Core teams, wdyt? 
>>> 
>>> - Prem 
>> 
>> -- 
>> You received this message because you are subscribed to the Google
Groups "Ruby on Rails: Core" group.
>> To view this discussion on the web visit
https://groups.google.com/d/msg/rubyonrails-core/-/bX4JiC2P5rMJ.
>> To post to this group, send email to rubyonrails-core@googlegroups.com.
>> To unsubscribe from this group, send email to
rubyonrails-core+unsubscribe@googlegroups.com.
>> For more options, visit this group at
http://groups.google.com/group/rubyonrails-core?hl=en.
> 
> -- 
> You received this message because you are subscribed to the Google Groups
"Ruby on Rails: Core" group.
> To post to this group, send email to rubyonrails-core@googlegroups.com.
> To unsubscribe from this group, send email to
rubyonrails-core+unsubscribe@googlegroups.com.
> For more options, visit this group at
http://groups.google.com/group/rubyonrails-core?hl=en.
-- 
You received this message because you are subscribed to the Google Groups
"Ruby on Rails: Core" group.
To post to this group, send email to rubyonrails-core@googlegroups.com.
To unsubscribe from this group, send email to
rubyonrails-core+unsubscribe@googlegroups.com.
For more options, visit this group at
http://groups.google.com/group/rubyonrails-core?hl=en.

In this type of case, it makes sense either to declare a whitelist or to 
declare a blacklist. But it doesn''t make much sense to declare both of
them.

Solution #3: ActiveRecord (or ActiveModel) should raise if a class declares 
both a whitelist and a blacklist of mass-assignable attributes.

class Comment
    attr_accessible: title
    attr_protected: author_id # raises immediately
end

Cheers,
Jay

On Monday, July 9, 2012 6:19:12 PM UTC-4, Uberbrady
wrote:
>
> (I posted this as a bug in GitHub (
> https://github.com/rails/rails/issues/7018), but then someone there told 
> me I should post it here, so here it is.)
>
> If you set attr_accessible on some properties in an ActiveRecord-descended 
> class, and then attr_protected on others - the class becomes
''default-open''
> - if any properties are missed or added later, they will be accessible by 
> default to MassAssignment.
>
> This undoes the entire point of having put attr_accessible in
one''s class.
>
> Two possible solutions -
>
> #1) ''default-closed'' - the attr_protected statements will
either be
> ignored, or just used to override attr_accessiblefor a particular 
> property.
> #2) ''explicit-only'' - any attribute accessed in
mass-assignment that is
> not explicitly mentioned in eitherattr_accessible or attr_protected raises 
> a new error - something like
>  MassAssignmentError:AttributeNotExplicitlyDeclared. Maybe even throw an 
> error if the attribute is accessed in*any* way
(mything.whatever="boo"; #
> kerplow! throws error?) though that might perform poorly.
>
> Solution #1 is probably fine - accesses to not attr_accessible properties 
> will throw a MassAssignment error under these circumstances anyways. 
> Solution #2 just makes things really explicit, which some might want for 
> some kinds of high-security applications.
>
> I found this bug in my own code during the development cycle; I liked 
> putting both attr_accessible andattr_protected in for symmetry and to 
> remind me of my DB schema at the top. Stupid reason, I know. I found that a
> belongs_to relation was unprotected in that circumstance.
>
> -B.
>
-- 
You received this message because you are subscribed to the Google Groups
"Ruby on Rails: Core" group.
To view this discussion on the web visit
https://groups.google.com/d/msg/rubyonrails-core/-/aqdzTPrnZTgJ.
To post to this group, send email to rubyonrails-core@googlegroups.com.
To unsubscribe from this group, send email to
rubyonrails-core+unsubscribe@googlegroups.com.
For more options, visit this group at
http://groups.google.com/group/rubyonrails-core?hl=en.

Jay, this solution doesn''t play nice with inheritance.

Rafael Mendonça França
http://twitter.com/rafaelfranca
https://github.com/rafaelfranca



On Tue, Jul 10, 2012 at 10:59 AM, Jay Feldblum <yfeldblum@gmail.com>
wrote:
> In this type of case, it makes sense either to declare a whitelist or to
> declare a blacklist. But it doesn''t make much sense to declare
both of them.
>
> Solution #3: ActiveRecord (or ActiveModel) should raise if a class
> declares both a whitelist and a blacklist of mass-assignable attributes.
>
> class Comment
>     attr_accessible: title
>     attr_protected: author_id # raises immediately
> end
>
> Cheers,
> Jay
>
> On Monday, July 9, 2012 6:19:12 PM UTC-4, Uberbrady wrote:
>>
>> (I posted this as a bug in GitHub (https://github.com/rails/**
>> rails/issues/7018 <https://github.com/rails/rails/issues/7018>),
but
>> then someone there told me I should post it here, so here it is.)
>>
>> If you set attr_accessible on some properties in an
>> ActiveRecord-descended class, and then attr_protected on others - the
class
>> becomes ''default-open'' - if any properties are missed
or added later, they
>> will be accessible by default to MassAssignment.
>>
>> This undoes the entire point of having put attr_accessible in
one''s class.
>>
>> Two possible solutions -
>>
>> #1) ''default-closed'' - the attr_protected statements
will either be
>> ignored, or just used to override attr_accessiblefor a particular
>> property.
>> #2) ''explicit-only'' - any attribute accessed in
mass-assignment that is
>> not explicitly mentioned in eitherattr_accessible or attr_**protected
raises
>> a new error - something like MassAssignmentError:**
>> AttributeNotExplicitlyDeclared**. Maybe even throw an error if the
>> attribute is accessed in*any* way (mything.whatever="boo"; #
kerplow!
>> throws error?) though that might perform poorly.
>>
>> Solution #1 is probably fine - accesses to not attr_accessible
properties
>> will throw a MassAssignment error under these circumstances anyways.
>> Solution #2 just makes things really explicit, which some might want
for
>> some kinds of high-security applications.
>>
>> I found this bug in my own code during the development cycle; I liked
>> putting both attr_accessible andattr_**protected in for symmetry and to
>> remind me of my DB schema at the top. Stupid reason, I know. I found
that a
>> belongs_to relation was unprotected in that circumstance.
>>
>> -B.
>>
>  --
> You received this message because you are subscribed to the Google Groups
> "Ruby on Rails: Core" group.
> To view this discussion on the web visit
> https://groups.google.com/d/msg/rubyonrails-core/-/aqdzTPrnZTgJ.
>
> To post to this group, send email to rubyonrails-core@googlegroups.com.
> To unsubscribe from this group, send email to
> rubyonrails-core+unsubscribe@googlegroups.com.
> For more options, visit this group at
> http://groups.google.com/group/rubyonrails-core?hl=en.
>
-- 
You received this message because you are subscribed to the Google Groups
"Ruby on Rails: Core" group.
To post to this group, send email to rubyonrails-core@googlegroups.com.
To unsubscribe from this group, send email to
rubyonrails-core+unsubscribe@googlegroups.com.
For more options, visit this group at
http://groups.google.com/group/rubyonrails-core?hl=en.

Yeah, Jay. Your solution won''t work with inheritance.

By deprecating the attr_protected, you can allow most of the attributes anyway
(but seriously seriously seriously discouraged) by do something like:

   attr_accessible columns - [:created_at, :updated_at]

Having attr_accessible and attr_protected together in the same model is just
asking for the trouble. You tell the model to whitelist, then you tell it again
to blacklist.

- Prem

On Jul 10, 2012, at 10:03 AM, Rafael Mendonça França
<rafaelmfranca@gmail.com> wrote:
> Jay, this solution doesn''t play nice with inheritance.
> 
> Rafael Mendonça França
> http://twitter.com/rafaelfranca
> https://github.com/rafaelfranca
> 
> 
> 
> On Tue, Jul 10, 2012 at 10:59 AM, Jay Feldblum <yfeldblum@gmail.com>
wrote:
> In this type of case, it makes sense either to declare a whitelist or to
declare a blacklist. But it doesn''t make much sense to declare both of
them.
> 
> Solution #3: ActiveRecord (or ActiveModel) should raise if a class declares
both a whitelist and a blacklist of mass-assignable attributes.
> 
> class Comment
>     attr_accessible: title
>     attr_protected: author_id # raises immediately
> end
> 
> Cheers,
> Jay
> 
> On Monday, July 9, 2012 6:19:12 PM UTC-4, Uberbrady wrote:
> (I posted this as a bug in GitHub
(https://github.com/rails/rails/issues/7018), but then someone there told me I
should post it here, so here it is.)
> 
> If you set attr_accessible on some properties in an ActiveRecord-descended
class, and then attr_protected on others - the class becomes
''default-open'' - if any properties are missed or added later,
they will be accessible by default to MassAssignment.
> 
> This undoes the entire point of having put attr_accessible in
one''s class.
> 
> Two possible solutions -
> 
> #1) ''default-closed'' - the attr_protected statements will
either be ignored, or just used to override attr_accessiblefor a particular
property.
> #2) ''explicit-only'' - any attribute accessed in
mass-assignment that is not explicitly mentioned in eitherattr_accessible or
attr_protected raises a new error - something like
MassAssignmentError:AttributeNotExplicitlyDeclared. Maybe even throw an error if
the attribute is accessed inany way (mything.whatever="boo"; #
kerplow! throws error?) though that might perform poorly.
> 
> Solution #1 is probably fine - accesses to not attr_accessible properties
will throw a MassAssignment error under these circumstances anyways. Solution #2
just makes things really explicit, which some might want for some kinds of
high-security applications.
> 
> I found this bug in my own code during the development cycle; I liked
putting both attr_accessible andattr_protected in for symmetry and to remind me
of my DB schema at the top. Stupid reason, I know. I found that a belongs_to
relation was unprotected in that circumstance.
> -B.
> 
> -- 
> You received this message because you are subscribed to the Google Groups
"Ruby on Rails: Core" group.
> To view this discussion on the web visit
https://groups.google.com/d/msg/rubyonrails-core/-/aqdzTPrnZTgJ.
> 
> To post to this group, send email to rubyonrails-core@googlegroups.com.
> To unsubscribe from this group, send email to
rubyonrails-core+unsubscribe@googlegroups.com.
> For more options, visit this group at
http://groups.google.com/group/rubyonrails-core?hl=en.
> 
> 
> -- 
> You received this message because you are subscribed to the Google Groups
"Ruby on Rails: Core" group.
> To post to this group, send email to rubyonrails-core@googlegroups.com.
> To unsubscribe from this group, send email to
rubyonrails-core+unsubscribe@googlegroups.com.
> For more options, visit this group at
http://groups.google.com/group/rubyonrails-core?hl=en.
-- 
You received this message because you are subscribed to the Google Groups
"Ruby on Rails: Core" group.
To post to this group, send email to rubyonrails-core@googlegroups.com.
To unsubscribe from this group, send email to
rubyonrails-core+unsubscribe@googlegroups.com.
For more options, visit this group at
http://groups.google.com/group/rubyonrails-core?hl=en.

Prem,

What''s the conflict with inheritance?

Cheers,
Jay

On Tue, Jul 10, 2012 at 12:29 PM, Prem Sichanugrist
<sikandsak@gmail.com>wrote:
> Yeah, Jay. Your solution won''t work with inheritance.
>
> By deprecating the attr_protected, you can allow most of the attributes
> anyway (but seriously seriously seriously discouraged) by do something
like:
>
>    attr_accessible columns - [:created_at, :updated_at]
>
> Having attr_accessible and attr_protected together in the same model is
> just asking for the trouble. You tell the model to whitelist, then you tell
> it again to blacklist.
>
> - Prem
>
> On Jul 10, 2012, at 10:03 AM, Rafael Mendonça França <
> rafaelmfranca@gmail.com> wrote:
>
> Jay, this solution doesn''t play nice with inheritance.
>
> Rafael Mendonça França
> http://twitter.com/rafaelfranca
> https://github.com/rafaelfranca
>
>
>
> On Tue, Jul 10, 2012 at 10:59 AM, Jay Feldblum
<yfeldblum@gmail.com>wrote:
>
>> In this type of case, it makes sense either to declare a whitelist or
to
>> declare a blacklist. But it doesn''t make much sense to declare
both of them.
>>
>> Solution #3: ActiveRecord (or ActiveModel) should raise if a class
>> declares both a whitelist and a blacklist of mass-assignable
attributes.
>>
>> class Comment
>>     attr_accessible: title
>>     attr_protected: author_id # raises immediately
>> end
>>
>> Cheers,
>> Jay
>>
>> On Monday, July 9, 2012 6:19:12 PM UTC-4, Uberbrady wrote:
>>>
>>> (I posted this as a bug in GitHub (https://github.com/rails/**
>>> rails/issues/7018
<https://github.com/rails/rails/issues/7018>), but
>>> then someone there told me I should post it here, so here it is.)
>>>
>>> If you set attr_accessible on some properties in an
>>> ActiveRecord-descended class, and then attr_protected on others -
the class
>>> becomes ''default-open'' - if any properties are
missed or added later, they
>>> will be accessible by default to MassAssignment.
>>>
>>> This undoes the entire point of having put attr_accessible in
one''s
>>> class.
>>>
>>> Two possible solutions -
>>>
>>> #1) ''default-closed'' - the attr_protected
statements will either be
>>> ignored, or just used to override attr_accessiblefor a particular
>>> property.
>>> #2) ''explicit-only'' - any attribute accessed in
mass-assignment that is
>>> not explicitly mentioned in eitherattr_accessible or
attr_**protected raises
>>> a new error - something like MassAssignmentError:**
>>> AttributeNotExplicitlyDeclared**. Maybe even throw an error if the
>>> attribute is accessed in*any* way
(mything.whatever="boo"; # kerplow!
>>> throws error?) though that might perform poorly.
>>>
>>> Solution #1 is probably fine - accesses to not attr_accessible
properties
>>> will throw a MassAssignment error under these circumstances
anyways.
>>> Solution #2 just makes things really explicit, which some might
want for
>>> some kinds of high-security applications.
>>>
>>> I found this bug in my own code during the development cycle; I
liked
>>> putting both attr_accessible andattr_**protected in for symmetry
and to
>>> remind me of my DB schema at the top. Stupid reason, I know. I
found that a
>>> belongs_to relation was unprotected in that circumstance.
>>>
>>> -B.
>>>
>>
>> --
>> You received this message because you are subscribed to the Google
Groups
>> "Ruby on Rails: Core" group.
>> To view this discussion on the web visit
>> https://groups.google.com/d/msg/rubyonrails-core/-/aqdzTPrnZTgJ.
>>
>> To post to this group, send email to rubyonrails-core@googlegroups.com.
>> To unsubscribe from this group, send email to
>> rubyonrails-core+unsubscribe@googlegroups.com.
>> For more options, visit this group at
>> http://groups.google.com/group/rubyonrails-core?hl=en.
>>
>
>
> --
> You received this message because you are subscribed to the Google Groups
> "Ruby on Rails: Core" group.
> To post to this group, send email to rubyonrails-core@googlegroups.com.
> To unsubscribe from this group, send email to
> rubyonrails-core+unsubscribe@googlegroups.com.
> For more options, visit this group at
> http://groups.google.com/group/rubyonrails-core?hl=en.
>
>
>  --
> You received this message because you are subscribed to the Google Groups
> "Ruby on Rails: Core" group.
> To post to this group, send email to rubyonrails-core@googlegroups.com.
> To unsubscribe from this group, send email to
> rubyonrails-core+unsubscribe@googlegroups.com.
> For more options, visit this group at
> http://groups.google.com/group/rubyonrails-core?hl=en.
>
-- 
You received this message because you are subscribed to the Google Groups
"Ruby on Rails: Core" group.
To post to this group, send email to rubyonrails-core@googlegroups.com.
To unsubscribe from this group, send email to
rubyonrails-core+unsubscribe@googlegroups.com.
For more options, visit this group at
http://groups.google.com/group/rubyonrails-core?hl=en.

On Jul 10, 2012, at 12:29 PM, Prem Sichanugrist wrote:
> Yeah, Jay. Your solution won''t work with inheritance.
> 
> By deprecating the attr_protected, you can allow most of the attributes
anyway (but seriously seriously seriously discouraged) by do something like:
> 
>    attr_accessible columns - [:created_at, :updated_at]
Note that this may die in exciting ways if you put it in a model that
hasn''t been created in the DB yet. (since columns looks at the table
metadata)

--Matt Jones

-- 
You received this message because you are subscribed to the Google Groups
"Ruby on Rails: Core" group.
To post to this group, send email to rubyonrails-core@googlegroups.com.
To unsubscribe from this group, send email to
rubyonrails-core+unsubscribe@googlegroups.com.
For more options, visit this group at
http://groups.google.com/group/rubyonrails-core?hl=en.