Skip to content
This repository

attr_accessible on some properties + attr_protected on others makes class 'open-by-default' #7018

Closed
uberbrady opened this Issue July 09, 2012 · 16 comments

8 participants

Brady Wetherington Rafael Mendonça França Francesco Rodríguez Carlos Antonio da Silva Guillermo Iguaran Erich Menge Richard Schneeman Michael Koziarski
Brady Wetherington

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_accessible for a particular property.
#2) 'explicit-only' - any attribute accessed in mass-assignment that is not explicitly mentioned in either attr_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 and attr_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.

Francesco Rodríguez

Hi, i think you should post this into rails-core mailing list if you want feedback.

Carlos Antonio da Silva

@uberbrady can you try to explain the issue you're facing in code? I think it'd be better to understand that. Thanks!

Guillermo Iguaran
Owner

Solution #3 (for future): In Rails 4 we should deprecate attr_accessible/attr_protected and make strong_parameters the default

Brady Wetherington
class Thing < ActiveRecord::Base
  attr_accessible :thingone, :name, :thingtwo, :thingthree
  # Don't use both attr_accessible and attr_protected!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!
  attr_protected :status # I was just doing this for symmetry, I guess?
  belongs_to :user
end

You can use mass-assigment to assign to :user_id or to :thingfour (not listed in accessible or protected).

Brady Wetherington

And another option - option #4) - Rails throws an error whenever you try to use both attr_accessible and attr_protected on the same class.

And of course option #5) - do nothing :)

Carlos Antonio da Silva

IIRC, older Rails versions raised an error when using both accessible/protected in the same model. And I think that's fine. @uberbrady are you using 3.2.6? Thanks.

Erich Menge

I agree it seems to me it should raise if you specify both. Or at least issue some sort of warning.

Also agree with @guilleiguaran. If you guys aren't aware, strong_parameters will be available in Rails 4 git://github.com/rails/strong_parameters.git . Both attr_accessible and attr_protected should probably be deprecated in master.

Brady Wetherington

This is Rails 3.2.6

Richard Schneeman
Collaborator

2 months old. Has this been fixed on the 3.2 branch? Anyone working on a fix?

Rafael Mendonça França

@schneems I don't think so.

@NZKoz do you mind give us some direction if we need to fix it?

Michael Koziarski
Owner

I think we can make attr_protected raise (or perhaps just warn) if called after attr_accessible.

However it's been like this for a very very long time so I wouldn't necessarily suggest that it needed to be urgently fixed or anything. Perhaps just fix the 4.0/plugin code?

Brady Wetherington

You could make the argument that it doesn't need to be fixed very urgently - I get that.

But I do think it should be fixed, and sooner rather than later. Here's why.

You have some statements here that a developer would use in order to take specific steps to secure his/her code.

Those statements won't work in the way that seems most 'obvious' to the developer, thus leaving him/her with effectively insecure code, after the developer has gone to some lengths to secure it. That's dangerous.

Now, it may be the case that the number of times this particular pairing (attr_protected/attr_accessible) is actually used together in live code is pretty rare - if that's true, then I guess there's no rush to fix. But it's not documented that you shouldn't use them together, and someone who might be trying to be 'thorough' (e.g., me) might get bitten by it, and leave a security hole in code that they were trying to be careful with.

That strikes me as the type of problem that should be addressed somewhat swiftly.

Michael Koziarski
Owner

It's pretty rare that they're used in the same model. But it's not rare that they're used in subclasses. e.g. attr_accessible declared in AR::Base and attr_protected declared in a particular subclass.

If we could have a patch which distinguished between that case, and the erroneous "declarations in the same file" case, then we could ship that.

Brady Wetherington

The inheritance implementation seems kinda weird too. Consider:

class Inner < ActiveRecord::Base
  attr_accessible :twiddle_that, :twiddle_this
  # :superprivate_text is 'protected'
end

and

class Outer < Inner
   attr_protected :twiddle_this
   # now what happens?
end

And then the following Console actions:

i=Inner.new
i.assign_attributes :superprivate_text => "crap"

ActiveModel::MassAssignmentSecurity::Error: Can't mass-assign protected attributes: superprivate_text
    from /Users/brady/.rbenv/versions/1.9.3-p125/lib/ruby/gems/1.9.1/gems/activemodel-3.2.7/lib/active_model/mass_assignment_security/sanitizer.rb:48:in `process_removed_attributes'
    from /Users/brady/.rbenv/versions/1.9.3-p125/lib/ruby/gems/1.9.1/gems/activemodel-3.2.7/lib/active_model/mass_assignment_security/sanitizer.rb:20:in `debug_protected_attribute_removal'
    from /Users/brady/.rbenv/versions/1.9.3-p125/lib/ruby/gems/1.9.1/gems/activemodel-3.2.7/lib/active_model/mass_assignment_security/sanitizer.rb:12:in `sanitize'
    from /Users/brady/.rbenv/versions/1.9.3-p125/lib/ruby/gems/1.9.1/gems/activemodel-3.2.7/lib/active_model/mass_assignment_security.rb:230:in `sanitize_for_mass_assignment'
    from /Users/brady/.rbenv/versions/1.9.3-p125/lib/ruby/gems/1.9.1/gems/activerecord-3.2.7/lib/active_record/attribute_assignment.rb:75:in `assign_attributes'
    from (irb):8
    from /Users/brady/.rbenv/versions/1.9.3-p125/lib/ruby/gems/1.9.1/gems/railties-3.2.7/lib/rails/commands/console.rb:47:in `start'
    from /Users/brady/.rbenv/versions/1.9.3-p125/lib/ruby/gems/1.9.1/gems/railties-3.2.7/lib/rails/commands/console.rb:8:in `start'
    from /Users/brady/.rbenv/versions/1.9.3-p125/lib/ruby/gems/1.9.1/gems/railties-3.2.7/lib/rails/commands.rb:41:in `<top (required)>'
    from script/rails:6:in `require'
    from script/rails:6:in `<main>'

Which is as expected - however -

o=Outer.new
o.assign_attributes :superprivate_text => "froobdle"
=> nil

irb(main):011:0> o
=> #<Outer id: nil, twiddle_this: nil, twiddle_that: nil, superprivate_text: "froobdle", created_at: nil, updated_at: nil>

So a class which inherits from another completely resets the attr_protected/attr_accessible 'state' based on "last-one-first?" That doesn't seem right.

I think if you look at it from the point of view of programmer intent - a derived class ought to only be able to make a 'protected' member of one of its ancestors accessible by explicitly naming it. And setting new levels of attr_accessible/attr_protected on different levels of the class hierarchy ought to only affect that level of the class hierarchy.

I'm open to other interpretations though. I think it all depends on what people are trying to accomplish when they use both attr_accessible and attr_protected - what does the programmer mean? If the behavior matches that intent, then it works the way it should.

Michael Koziarski
Owner

As weird as it is, that's defined and documented behaviour that people rely on,

We could make those changes in a 4.1 release of the attribute protection gem, but not at this stage. It's possibly a good idea to investigate the feasiblity of the changes you're outlining in http://github.com/rails/protected_attributes, but not in a maintenance release of a feature that's getting deprecated.

Rafael Mendonça França

Guys I'm closing this issue since we are deprecating this feature in the next release and this behavior is documented. If you want to move the discussion the the protected_attributes project go ahead.

Rafael Mendonça França rafaelfranca closed this September 12, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.