Mass assignment security shouldn't happen in the model
Mass assignment security is a feature in Active Model (and therefore Active Record) which allows you to be selective about what input you will accept when ‘mass assigning’ a hash of attributes.
Typically, you will have a form that submits to a controller update action, which simply
assigns all the form parameters to the model:
@user.attributes = params[:user]Any keys in params[:user] which are not allowed (e.g. an ‘admin’ flag) will be silently
discarded.
Problems
This is great, of course, because nobody wants to get hacked. However if you have been writing Rails applications for long enough you will come up against some problems:
-
Creating records in tests. Often when testing models, you will want to create a model with a given set of attributes, and then interact with it in the actual test. The quickest way to do this might be
user = User.new(:admin => true)but if you have the
adminattribute protected, it won’t work. Worse, you won’t know it hasn’t worked: the attribute is just silently ignored.There are some ways around this:
- The most popular ‘object factory’ plugins will ignore mass assignment security when creating an object from a factory.
- Recently a feature has been added in edge Rails
to raise an exception in the development and
test environments when mass assignment parameters are filtered. This will not be available
until Rails 3.2, and you will need to set
config.active_record.mass_assignment_sanitizer = :strictin your environment config to enable this.
(In general, this problem is better described as ‘interacting with records somewhere other than a controller’.)
-
Different rules for different people. Often there are different ‘roles’ for users in an application. For example, an ordinary user should be able to change her password, but not promote herself to an admin. However, an admin should be able to promote another user to be an admin.
This has historically caused problems with mass assignment security, so in Rails 3.1 (not yet released), there is a mechanism for specifying ‘mass assignment roles’.
Essentially, you can specify different lists of allowed attributes, and give them a name. Then, when an admin user is editing a user, you can use their ‘list’ via the
:asoption:@user.assign_attributes(params[:user], :as => :admin) -
Active Record has to work around mass assignment security internally. This isn’t so much an end user problem, but in the past few months I have fixed some very nuanced bugs within Active Record. These bugs were either caused by mass assignment security kicking in where we didn’t want it to, or by subtle mistakes in the logic when we tried to work around the mass assignment security. (If you’re interested, see the discussion in these bugs and these commits.)
Put it in the controller
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.
I believe that the filtering should happen in the controller, which is the area of the application which is supposed to deal with HTTP input. Here’s a rough sketch of how it might work:
class ActionController::Base
include ParamSecurity
before_filter :filter_params
def filter_params
self.params = param_filters.inject(original_params) do |mem, filter|
filter.process(mem)
end
end
# Define filter(s) which operating on incoming params. Can be overridden to
# extend/customise the filter list.
def param_filters
[param_security_filter]
end
end
module ActionController::ParamSecurity
extend ActiveSupport::Concern
module ClassMethods
included do
class_attribute :param_security_rules
end
# Some sort of dinky DSL so you can do e.g.
#
# param_protected :user => [:role, :email]
#
# or
#
# param_accessible :user => [:name, :password], :only => :create
end
# Can be overridden if necessary
def param_security_filter
ParamSecurityFilter.new(self.class.param_security_rules, action_name)
end
endWhy this is better
- We can do away with having ‘roles’ for mass assignment security; you can simply define different
filters in your
UserControllerand yourAdmin::UserController. - When outside the controller, we can use models normally, without having to worry about security features that don’t apply to the current situation.
- It’s totally customisable on a per-controller or even per-action basis.
What do you think?
I am not the first to make these observations. Merb had a plugin to do this, and a similar plugin has been written for Rails.
But mass assignment security is one of the first things a new Rails user learns, and it’s a feature that is present in almost every single Rails application. So personally I would love to see this changed in Rails core itself.
Of course, it’s not that simple because we have to think about backwards compatibility, and it could be quite annoying for Rails users to have to move the filtering rules out of their model and into the controller.
So I am not sure whether this is really worth changing, but I would like to hear your thoughts!
Comments
I'd love to hear from you here instead of on corporate social media platforms! You can also contact me privately.
Pavel Forkert
I think the main problem is that Rails is Model-centric and a lot of functionality is included into models itself - mass assignment is only one of the parts, which can be implemented outside of models - filtering/sanitizing parameters, additional mapping layer between forms in views and model attributes, different validations for different roles, etc. Many big apps would be happy if Rails provides a way to clean up their models a bit more. :)
But I think - trying to solve all those problems will lead to more difficult APIs and many newcomers or people, that are building small/medium apps will suffer :)
Personally I am more or less happy with current mass assignment APIs. :)
dcrec1
I think it depends, some things make sense to be protected at the model, other at the controller. I don't think there is a rule for this, would be cool if Rails supported both ways.
Giles
I think as always with Rails you want a balance between reasonable defaults and ungodly power for those who wish to cause mischief. However, I also think the way to go about this is to throw a few different plugins together and see which work best. Reason I say that is I can see a variety of possible APIs working well, and whichever API we end up with, transitioning people from the flawed "it lives in the model" approach will take some time and effort. Best way to deal with that is to throw a bunch of options out there, build some real live apps with them, and see which options have which strengths. TATFT applies to features and design too. :-)
jcasimir
I respect the idea, but have to disagree. I see attribute security as a data/business-logic concern which means it has to go in the model layer. If you want to factor it out into a module that gets included into models, that sounds interesting. But handling it at the controller level feels inappropriate.
I think you'd be better off creating a "factory-like" class method down in the model and using that instead of ActiveRecord's class methods from the controller.
ultrasaurus
I used to do this in controller logic because, in principle, I felt that external APIs should be locked down in the place that you define the API and that the model layer is an internal API where security shouldn't be a concern. However, in practice, I only use mass assignment from controllers and it is more concise to define attr_accessible in the model, so that's what I do now. I suppose I just think of the mass assignment API now as a public API.
Joost Baaij
I also feel that, in principle, the role and mass-assignment security features should move to the controller. Since parameters submitted by admins are equally valid to parameters submitted by users, the model should not have to care about the distinction. Since bots sets are valid, the concern should be lifted out of the model and into the controller.
However for the 3.x series of Rails I think we're stuck with the current practice. As you said, more or less every Rails developer will have seen this problem and dealt with it in some way. For me, it's an acceptable tradeoff and just one of those things you need to learn in order to become proficient doing Rails.
Your proposed solution will work fine, but still entails hacking at hashes and, by extension, the manipulation of strings. I would rather see an effort to move away from dumb string manipulation in the params/controller and implement something akin to Arel which we did for the model. In other words: a very much souped up controller that we can also use as presenter/conductor. In every Rails app I am doing, I have some need for these patterns and I end up implementing them myself. For Rails 4 I think it would be great if the C from MVC could rise above its current self.
Moving these mass-assignment features into that layer would be a very natural thing to do.
Rasmus Rønn Nielsen
When I heard Rails was introducing a mass assignment security feature I was like "Yeah, finally"!!
When I found out it happened in the model I was like "Wait.. what??". I would have expected it to be happening in the controller.
Your argument about how the current implementation "assumes" that HTTP is the normal way of interacting with a model is key.
Failing silently works great when setting attributes via http params. But in almost any other cases, it's not (tests, rake tasks, etc).
Okay, we may be setting attributes via http (params hash) most of the time, but does that mean it is okay to make this security feature annoying when we're not? I don't think so.
Also, by sending :as => :admin to the user, we're slightly coupling ActiveModel with the entire app's authorization logic - that smells if you ask me. The model shouldn't know anything about the context in which it is used.
J.-Baptiste Escoyez
Update: Disqus did not show the whole thread when I first wrote the comment. @Joost I wrote about your presenter/conductor idea hereunder. @Rasmus I so much agree on "The model shouldn't know anything about the context in which it is used."
Update 2: I had not saw that: https://github.com/jonleigh... :)
@Jon, @Pavel: I really agree with you!
I started to use what I call a Conductor (not totally sure it match the conductor pattern) in my applications.
Basically, it is a class wrapping the object which will filter parameters and format them for assignment. So that, only the business logic is written in models.
E.g.: In my UsersController, you can find:
> def update
> @user = UserConductor.find(params[:id]) # Which create a UserConductor object wrapping the User instance
> @user.attributes = params[:user] # attributes is overridden in UserConductor in order to filter attributes (e.g. "100$" > 10000 (when dealing with amounts)) and assign them to the model
> @user.save # other ActiveRecord methods are just transmitted to the wrapped model
> end
I think mass-assignment could have its place their too since, IMO, the role of the controller is not to filter params, just to forward them to the right actor (Model, View, Mailer...)
Jon Leighton
This is a very good point and I think a conductor is the cleanest solution to this (as well as for dealing with stuff like nested attributes). I actually tried to write something along these lines several years ago: https://github.com/jonleigh... (it's unfinished, abandoned and probably doesn't work with rails 3)
On the one hand I would love to see Rails embrace stuff like conductors and presenters/decorators (see also: https://github.com/jcasimir... ) out of the box. But on the other hand to do so would be to steepen the learning curve for new users. It's a tricky balance...
J.-Baptiste Escoyez
Could we imagine a gem bundle called "advanced_rails" which will provide some advanced architecture to the basic Rails MVC ? These gems could integrate nicely an be integrated to Rails if needed/wished.
Alexey Muranov
I agree. It always looked strange to me:
1. before rails 3.1, it looked strange that the same rules of writing data apply to all cases and all users,
2. in rails 3.1, it was solved with roles, but now it looks strange that a model knows about roles of _people_ who use it; it is like adding roles for accessing public methods of a class.
Alexey Muranov
I disagree with the objection: the security here means protecting from malicious or non-careful users, not from the developer or other parts of the application. So in my opinion its place is in the controller layer. (Even more specifically: usually it means simply filtering the params hash.) What is exactly "attribute security"? Attributes are to be read and written by the application.
Alexey Muranov
Another argument in favour of the change: it would be easier to follow if the security of each web request was handles in a single place (to know what code is responsible). Currently the controller has the choices: (a) blindly delegate filtering to the model, (b) filter itself before delegating to the model (why not?), (c) bypass model-level "security" by not using mass assignment.
Charles Hoffman
I am totally with you on this. I feel similarly about accepts_nested_attributes_for.
Alexey Muranov
I would like to reply as a "new user". I've been reading now about conductors and presenters. I think if their use will be optional, this will not steepen the curve. I've spent quite some time trying to understand why params are filtered by the model and why it is ok to rely on this filtering (i still no not understand very well). It would be faster for me to learn how to filter them with a `before_filter` (until i would learn how to use a conductor), and i wouldn't be so confused.
dpickett
I totally agree that it's not a controller behavior or responsibility. There's an intermediary business object that's missing I think. Something like a conductor/decorator pattern seems appropriate here. Some kind of AttributeFilter abstraction that can be applied directly to model instances, instantiated in the controller.
Nicolas Sanguinetti
Have you seen how Django does this? It defines objects for the "Forms" that handle validations. There's a ruby implementation of this for ruby called Bureaucrat: http://github.com/tizoc/bur...
Eleo
The pattern presented here seems viable/logical. I just don't see it as an improvement -- just a different approach.
I can agree to some extent with some of the problems presented, but I don't feel that using the controller to filter attributes is a solution superior to preventing mass assignment within the model.
I see this as akin to using the controller for validations. Validations in the model introduce some of the same problems as handling mass assignment from the model. For example, in tests, sometimes I have to bypass validations in the same way I might have to force mass assignment. Validations also vary based on role -- e.g. an admin might have fewer restrictions on making posts than a regular user. In my eyes, a model accepts all the incoming data and then sorts through it as necessary. It's object-oriented and highly testable.
>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.
I don't see how this assumption is invalid, or bad. If you're preventing mass assignment from the model, then it's because your application deals with submitted data. There might be further interactions, such as within tests, irb, background jobs, I don't know. I haven't found it to be too problematic outside of these contexts. These are my problems, not the user's. If your model doesn't deal with submitted data, it probably doesn't deal with mass assignment or doesn't need to. I don't see what the problem is here. Maybe I'm missing something.
You bring up some interesting points for sure. What I'm getting from this is that perhaps we are following MVC too stringently. For example, going back to the validations in the model -- it always bugged me, typing up validation error messages in my models, because it seemed like a view concern what that message is. The paradigm should not be immutable and maybe we need to think beyond "is this a model, view, or controller?" and insert layers as necessary without trying to categorize them necessarily as one of those three things.
joelparkerhenderson
> There's an intermediary business object that's missing I think.
> Something like a conductor/decorator pattern
You're looking for DCI: Data Context Interaction. It's perfect for what you're suggesting.
Alexey Muranov
@Jon: what do you think about simply isolating non-database related functionality of a model using inheritance:
```ruby
class Employee < ActiveRecord::Base
end
class Accessors::Employee < Employee
attr_accessible :name
attr_accessible :salary, :as => :boss
def self.controller_class
@controller_class || StaffController
end
def self.controller_path
controller_class.controller_path
end
end
```
This is another layer between Model and Controller (or Model and Conductor), and can be already used in rails.
(See my comment in https://github.com/rails/ra... )
Doug Puchalski
This is something that has troubled me about Rails since I started working with it. The idea that forms on a web page are directly tied to the persistence layer seems odd. Why should the view show the names of my DB columns? And why should the model, which has no idea of the context, decide what error messages to display?
Persistence, Presentation, and Conduction are quite like independent functions. For simplicity and learning, mapping them directly to MVC can make sense. Using these patterns is just the next level, a good way to keep code clear, controllers thin, ease refactoring, and be fail-safe.
I am leaning toward the following:
Model: (persistence) I choose the schema and methods that map the required data model into whatever storage mechanism I want, making it easier to refactor. No security is handled, only low-level validations that affect data integrity of each instance. Validation messages can be the defaults.
Conductor(s): (accessibility) One or more for each model or set of models. Uses ActiveModel (ActiveAttr?) to define attributes and validations that are appropriate to the context. Operations that result in a valid conductor instance are delegated to the model.
Controller(s): (authorization) Based on authorization or mode, a controller would choose and act on the appropriate conductor. A lot of filters and other tangles are gone, and more cleanly implemented.
Presenter(s): (presentation) Whitelist delegation to the model, removing need for model helpers entirely. Again here the notion that a user vs. admin view of a resource may be entirely different.
View(s): (formatting) now safe from accessibility concerns, the view only has access to it's whitelisted presenter. Views become more like templates, less like code. Accidentally showing the wrong data is much harder, and less diligence is required. I'm sure I'm not the only one who has accidentally shown something in the view either by editing the wrong file or simply with a typo.
Standard MVC works fine in the beginning or with some models, and splitting things out might be done on a case-by-case basis as complexity increases.
Add your comment