Re: [Rails-core] attr_accessible on some properties + attr_protected on others makes class 'open-by-default'

[Prem Sichanugrist]

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

[Ryan Bigg]

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

--
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 rubyonra...@googlegroups.com.
To unsubscribe from this group, send email to rubyonrails-co...@googlegroups.com.
For more options, visit this group at http://groups.google.com/group/rubyonrails-core?hl=en.

[Xavier Noria]

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.

[Michael Breen]

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

[Jay Feldblum]

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.

[Rafael Mendonça França]

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

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

--

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.

[Prem Sichanugrist]

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

[Jay Feldblum]

Prem,

What's the conflict with inheritance?

Cheers,

Jay

[Matt Jones]

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