Hacker Newsnew | past | comments | ask | show | jobs | submitlogin

`attr_accessible` and friends are a nightmare anyway. They push a controller-layer concern into the model-layer. It breaks the hash-initialization syntax that is so useful in irb, in unit tests, in background jobs, etc.

Instead, I've been using ActiveSupport's Hash#slice method in my controllers.



Declarative syntax for which attributes the model designer is comfortable exposing to mass assignment seems like a perfectly legitimate model concern. In what sense is attr_accessible different from, say, "validate"?


It mascarades and is documented as a security feature, but it's a superficial restriction. You could simply loop and send("#{key}=", value) instead of calling attributes=(hash).

In practice, all it does it prevent you from writing `MyModel.new(:foo => bar)` and force you to write `m = MyModel.new; m.foo = bar` This is simply annoying at the repl.

Validation applies always the same (or when some predicates are satisfied), but restriction on changing fields can vary by view or via permissions (ie. only admins can flip that bit!)

The security concern comes up when you bring "params" into the picture. You don't want to ever do: MyModel.new(params) for fear of params[:is_admin] == true.

A better solution would be to automatically mark the values in `params` as tainted and throw an error on mass-assignment of tainted values. This way you can still use mass assignment for non-user-input (like in tests or at the repl). You could have an `safe_attrs` to disable tainted value filtering for some known-safe attributes.


I see where you're coming from. In the past 10 years, I've learned to appreciate things that, while not solving fundamentally a specific security problem, act as a circuit breaker to keep people from mindlessly making the exact same mistake over and over again.

AR::Base#update_attributes is a bad interface, from a security perspective. But it's vital to the programmer experience of Rails development. There's no good fix for the problem; AR models don't even list their attributes, let alone express a coherent strategy for defending them. I'll take the little wins I can get.


"Mass assignment security is a feature that is aimed at dealing with form input. By placing it in the model, we end up making the implicit assumption that all ‘normal’ interaction with the model happens via a HTTP request. This assumption is incorrect and causes problems."

http://jonathanleighton.com/articles/2011/mass-assignment-se...


You can still use attr_accessible in the controller-layer.

    class AccountsController < ApplicationController
      include ActiveModel::MassAssignmentSecurity

      attr_accessible :first_name, :last_name
      ...
http://api.rubyonrails.org/classes/ActiveModel/MassAssignmen... explicitly states: Note that using Hash#except or Hash#slice in place of attr_accessible to sanitize attributes won’t provide sufficient protection.

I agree with you, though. Model-level attr_accessible, if nothing else, is flatly annoying. I feel inconvenienced by a measure that's supposed to protect against malicious users. And I feel like the terrorists have already won. I'm no MVC guru, but it makes sense to me that the Controller would deal with what amounts to the params that are on their way to the model. Why should the model have to worry about mass assignment?


`except` is a bad idea because it's a black-list, but what's wrong with using `slice` as a white-list?

(Anyway, I just grepped our code and there is only ONE usage of slice in a controller anyway, the rest are explicit, non-mass assignments)




Guidelines | FAQ | Lists | API | Security | Legal | Apply to YC | Contact

Search: