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.
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.
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.
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.
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.
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.
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 <radarliste...@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.
> 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.
On Tue, Jul 10, 2012 at 10:59 AM, Jay Feldblum <yfeldb...@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:
>> 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.
> 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.
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 <rafaelmfra...@gmail.com> wrote:
> On Tue, Jul 10, 2012 at 10:59 AM, Jay Feldblum <yfeldb...@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.
> 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:
> 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 <
> rafaelmfra...@gmail.com> wrote:
> Jay, this solution doesn't play nice with inheritance.
> On Tue, Jul 10, 2012 at 10:59 AM, Jay Feldblum <yfeldb...@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:
>>> 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.
>> 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.
