public
Last active

  • Download Gist
security.md
Markdown

Proposal for Improving Mass Assignment

For a while, I have felt that the following is the correct way to improve the mass assignment problem without increasing the burden on new users. Now that the problem with the Rails default has been brought up again, it's a good time to revisit it.

Sign Allowed Fields

When creating a form with form_for, include a signed token including all of the fields that were created at form creation time. Only these fields are allowed.

To allow new known fields to be added via JS, we could add:

<%= f.allowed_fields "foo", "bar", "baz" %>

Mark accessible fields in the controller

The first strategy will not full satisfy apps that support a lot of HTTP requests that do not come from forms generated by Rails.

Because accessible fields is usually a function of authorization, and is not global, we should move allowed fields into the controller. The basic idea is:

class PostsController < ApplicationController
  # attributes can be marked accessible for the entire controller
  attr_accessible :foo, :bar

  def create
    # mass assignment can also be done on an instance basis
    # this can be used to override the class defaults
    attr_accessible(true) if user.admin?
    ...
  end
end

I would imagine that Rails authorization frameworks like CanCan could add sugar to make this even easier in common cases.

Conclusion

The core problem with Rails mass assignment is that attribute protection is an authorization concern. By moving it to the controller, we can have smart defaults (like signed fields in form_for) and in more advanced cases, make it easier to decide what fields are allowed on a per-user basis.

By moving it into the correct place, we will probably find other nice abstractions that we can use over time to make nice defaults for users without compromising security.

+1, especially for "The core problem with Rails mass assignment is that attribute protection is an authorization concern."

There is a way to get much of this behavior in current Rails. Instead of moving it to the controller, you move it to an ...AsViewedBy model. That is, for any model class Foo that needs per-user attribute protection, you create a FooAsViewedBy model class that knows about the current user and delegates to the underlying Foo after making permissions checks. It gets a little cumbersome, but it has its elegance at the same time.

:+1:
attr_accessible shouldn't be in model.

@jamesarosen don't want to hijack my own thread, but if controllers had a notion of the resource they were operating on (@resource, say), then the controller becomes that object.

:thumbsup: this looks really good.

@wycats it sure does :)

http://jonathanleighton.com/articles/2011/mass-assignment-security-shouldnt-happen-in-the-model/

I agree with you ;)

Ideally we would make the code reusable so that people can choose to mix the behaviour into an intermediate "conductor" object if they wish (rather than have it either in the controller or in the model).

@wycats, the second approach sounds more elegant, but what about nested attributes? One just lists all the accessible attributes on all nested resources in a flat array or nest them as well? That could get very nasty with deeply nested models...

@tadast I surely wasn't the first to ask that, but I asked that on twitter as well. Jumped out at me right away.

There could potentially be multiple calls to attr_accessible in a controller:

def create
  attr_accessible :first_name, :last_name, :for => "contact"
  attr_accessible :area_code, :number, :for => "contact.phone_numbers"
end

Is it me or it's not really of accessible attributes we're talking here, but something more like request filters / sanitizers ?

@oelmekki like django forms, hehe

(y) Seems good idea. Not sure how it works out though.

Am I the only one who never passes a user input to the model without #slice? I thought this was already conventional wisdom...

def update
  find_thing.update_attributes(params[:thing].slice(:attr1, :attr2, :attr3))
end

@jayferd, I think the idea is to make it easier for everyone. Thats a lot of RYing (D left off intentionally).

@wycats, is the thought that the call silently fails, noisily fails, or filters and doesn't fail?

The only problem I see here is that you tie a controller to a model. the :for sorta removes this concern, though.

It would be nice to move the actual responsiblity of sanitizing the params for a given context (user) into a separate object, or at least make it possible. Sort of like validation works. There are already lots of responsibilities handled by the controller.

@dasch I would imagine that the feature would be implemented as an object first with controller sugar.

:+1: to what @jonleighton wrote - this should be a mixin so that you could use it in a controller or in some service object, the decision is up to you.

I would prefer that attr_accessible was in the model, but that it included some way of defining what was accessible according to a role.

i.e. An admin or an owner can change a comment.
Not sure how this would work, but I'm not convinced that moving the code into the controller is the best thing. If a user can't change the "locked" flag on a record via a controller, then why should they be able to do it from anywhere outside the model.

Surely the safest approach is to reduce the attack surface as much as possible. If you're adding security via your controllers, that's not the smallest attack surface.

I think both of these are great ideas. I am not so expert in the fundamentals of MVC, but I think either of these would help me.

The current functionality is already a mixin, fwiw

Accepting a signed list of accessible parameters from the client could make you vulnerable to replay attacks. For example, you could revoke a user's permission to write to some field :secret, but if they saved the signature, they could write to the :secret field into perpetuity. "Mark accessible fields in the controller" is much safer, IMO, and it's an improvement on manually slicing the params array, like @jayferd illustrated.

@jayferd Hash#slice is easy to evade. The secure solution is to use ActiveModel's mass assignment security module.

@seanbamforth Rails 3.1 has that.

@eac how can it be evaded?

I think we're quite mixing things here. I see no reason why a controller should be aware of the database structure.

Instead of thinking allowed assignments, maybe it would be better to think validation of requests, with something like

class UsersController < ApplicationController
  allows_params :user => [ :first_name, :last_name, :email, :password, :password_confirmation ], :on => [ :create, :update ]
  validates_params :my_validation

  protected

  def my_validation
  # do stuff on params
  end

This would let handle crafted requests, without expecting every mass assignments are done through controllers.

@johndouthat good point. it would either need to be a nonce or have an expiry.

+1, especially to moving the accessible fields specification into the controller. There have been a handful of times where I wanted to use a model differently depending on the context in which its being accessed. The controller provides that context. In the current implementation, my only choice is to list all attributes in attr_accessible on the model, thus widening my exposure across the board rather than in the one or two controllers I'm really interested in editing.

As mentioned Django's forms are IMHO a good answer to this issue, and should be easy to implement with ActiveModel. Something like:

# easily builded by model instrospection
# but also allow to create form totally unrelated to any model
class UserForm < Form
  model User
  allow :first_name, :last_name, :email
end

class UsersController

  def create
    @form = UserForm.new(params[:user])
    if form.save # sanitize parameters, perform form based validations and then model based validations 
    ...
   end
end

And it would have the advantage to offer a better place for string to real ruby object castings. Everytime is see that AcceptanceValidator considers only "1" as a valid value I want to cry.

+1 to @byroot's proposal.

I've changed it a bit, I think Forms need to know about the context they are in (current_user)

class PullRequestForm < Form
  self.model = PullRequest
  attr_accessible :source, :creator
  validates do |controller|
    errors.add('hi homakov') if self.creator != controller.current_user
  end
end

class PullRequestController < ApplicationController
  # POST /pull_requests
  def create
    pull_request = PullRequestForm.new(params[:pull_request], controller: self)
    if pull_request.valid?
      ...
    end
  end
end

There's lots of potential here, e.g.

class CommentForm < Form
  self.model = Comment
  attr_accessible :always_accessible

  normalize do
    self.title = self.title.strip
  end

  # Hello FormBuilder
  render :message, as: MarkdownPreviewTextArea

  # POST /comments
  on :create do
    attr_accessible :repo, :creator, :title, :message
    validates do |controller|
      errors.add('hi homakov') if self.creator != controller.current_user
    end
  end

  # POST /comments/:id
  on :update do
    attr_accessible :title, :message
  end
end

I don't think that Form has to know about the context. If you have different validation for different context, just make different forms.

class CreateCommentForm < Form
  self.model = Comment
  attr_accessible :repo, :creator, :title, :message
end

class UpdateCommentForm < Form
  self.model = Comment
  attr_accessible :title, :message
end

Idem for the current_user thing, for this example you just have to not allow :creator access and create comment through current_user.comments.create().

Seriously checkout the Django's form documentation. There is almost no logic there, only a declaration of which parameters I expect.

The shitty thing about Rails is that there is almost nothing between the HTTP layer and the model layer. No casting, no validations, nothing.

It's not only about security, it also about responsibility separation. Why does the model layer should be responsible to cast checkbox values into booleans ? https://github.com/rails/rails/blob/master/activerecord/lib/active_record/connection_adapters/column.rb#L9
With Django, you specify in your form that the accept_tos parameter is a boolean, and it will send a boolean instance to your model.

Back when I did Struts, the most annoying thing was declaring all your forms.

--Eric Anderson

Sent from my iPhone

On Mar 4, 2012, at 4:55 PM, Jean Boussier reply@reply.github.com wrote:

I don't think that Form has to know about the context. If you have different validation for different context, just make different forms.

class CreateCommentForm < Form
 self.model = Comment
 attr_accessible :repo, :creator, :title, :message
end

class UpdateCommentForm < Form
 self.model = Comment
 attr_accessible :title, :message
end

Idem for the current_user thing, for this example you just have to not allow :creator access and create comment through current_user.comments.create().

Seriously checkout the Django's form documentation. There is almost no logic there, only a declaration of which parameters I expect.

The shitty thing about Rails is that there is almost nothing between the HTTP layer and the model layer. No casting, no validations, nothing.

It's not only about security, it also about responsibility separation. Why does the model layer should be responsible to cast checkbox values into booleans ? https://github.com/rails/rails/blob/master/activerecord/lib/active_record/connection_adapters/column.rb#L9
With Django, you specify in your form that the accept_tos parameter is a boolean, and it will send a boolean instance to your model.


Reply to this email directly or view it on GitHub:
https://gist.github.com/1974187

@ericanderson: Right now, You already have to declare them in your views.

With a good form builder, declaring them in a proper class can save you from declaring them in your views again. It can even save you from declaring inputs types, just like simple_form do.

Why instead of having allowed_fields in form_for just take the parameters from f methos (f.text_field, f.label, etc.) and use that as the allwed fields?

@aflag we would do exactly that. If you wanted to allow additional fields (for example, fields added using Ajax), you would need to specify them as allowed_fields.

+1 I like this. Thanks @wycats for bringing some sanity to the drama.

Does Rails need a whole new abstraction to deal with this? I've been using a controller-based approach for a few years that really feels right to me. Instead of using attributes=(hash), I added an assign(hash, whitelist) syntax. It's always clear at the point of assignment what's allowed. And if anyone feels like the whitelist declaration gets redundant, they can always use simple Ruby to extract it into a constant, etc.

https://github.com/cainlevy/mass_assignment

@wycats I think what @aflag means is that if you create a form allowing a user to specify, for example, a title and body of an article, only the title and body would be allowed to be specified in the corresponding create/update action. However, I don't think it would be possible to restrict that. If the new/edit actions tell the create/update actions what they're allowed to modify, a malicious user can provide the same authorization data using the same interface. Something has to happen in an interface that the user has no access to, such as in the model or controller code.

"Mark accessible fields in the controller" is akin to moving validations to the controller. Indeed, all that need be done is to validate that certain attributes were not mass assigned (under, potentially, certain conditions). Using validations would also give access to the new built-in "roles" functionality, rather than requiring ugly overrides in the controller.

Objects should be responsible for their own integrity and validations and they already are in the form of validations and attr_accessible. This makes those objects suitable for secure use from anywhere (like, say, a Resque worker), not just a controller.

Moving or duplicating part of this out to the controller level divides the responsibilities uncleanly, leaving some validations in the model and some in the controller.

+1 I like this. I really do.

Maybe keeping this at the model level but having save take a parameter that say the level of security.

 class Post < ActiveRecord::Base 
     security_level :admin => [:name, active, :something_secure]
     security_level :normal_user => [:name, active], :default => true

 end

now in the controller for the admin you do something like this:

 class Admin::PostsController
   def create
     @post = Post.new(params[:post])
      if @post.save(:security_level => :admin) # sanitize parameters, perform form based validations and then model based validations 
        ...
    end
 end

@drhenner: To a point, that's what Rails 3.1's mass assignment roles allow - http://ablogaboutcode.com/2011/05/12/activerecord-3-1-mass-assignment-roles/ (though on the mass assignment rather than the save).

So you can define which attributes are editable in either the model layer, controller layer, or view layer? That sounds like an absolute mess. +1 for something like Django's form framework, which encapsulates all this logic into a singly-responsible layer.

@kylefox I think the proposal is to remove it from model layer.

Ah, ok, didn't realize that. Nonetheless, it would still need to be supported temporarily to support backwards-compatibility, and would end up living alongside view/controller solutions.

Furthermore, moving this kind of logic to views seems wrong. The view should be responsible for dumbly rendering the form. Template files should have no impact on how PUT/POST (and now PATCH) data is processed. Offloading this responsibility to views is an unnecessary leaky abstraction.

The ideal solution IMO is to add something whose sole responsibility is validating & sanitizing user input against a set of context-specific rules.

@peterc Why haven't I been using that? Thanks for the link. I know I saw this a while ago but forgot. I need to increase my RAM in my brain.

Now I guess I think moving attr_accessible to the controller is a bad idea. Adding the following to the controller:

    Post.new(params[:post], :as => :admin)

is much better. If you add a field to the Post table you will potentially need to modify several controllers.

@kylefox what I don't like too much about that form model is that it's much easier to define your form when you're creating its view. That's the only place you should need to edit. I find creating a Form class and then using it on your view a bit repetitive. I really don't see the reason why it can't be done that way.

@jgaskins I think you got what I was thinking. But there's no need for the user to send that information back to us. When method new is called you end up calling form_for. That method could take care of registering in the controller what attributes are valid for posts going to that controller. Then, there could be a params[:post_safe] which is params[:post] containing only what was registered on the Controller. From what I've understood that's what this proposal is all about, is it not?

In cases you are receiving json, xml, you could manually register things on the controller, but there could be easy to use mechanisms like what I said for form_for.

@aflag Isn't defining it one spot -- the Form class -- better than having it potentially defined in multiple sources (multiple view files, controllers & models)?

@kylefox sounds like you are supporting the DCI approach. IMO that one spot would be the model for Rails. I think DCI is great but will need a new framework like uncle Bob's approach he talking about at ruby mid-west conf. So far proposed DCI approaches in Rails don't fit well IMO. http://www.confreaks.com/videos/759-rubymidwest2011-keynote-architecture-the-lost-years

@peterc I don't think it's a good idea to put all these security concerns to AR simply because it's just too much. AR is becoming so bloated nowadays that you shouldn't be calling it AR anymore. Security should be handled on a different layer. For example what's the problem with mass-assignment? The problem is that a user of your system can send a web form with fields that you didn't expect. Now, dealing with this on the ORM model level is crazy. How you're dealing with this sort of protection is solely dependent on the context - if you really want to handle all contexts in AR models then you'll be doing it wrong.

That's why I've been suggesting moving away from using AR models directly in your controllers. Exposing db access layer directly is a stupid idea, always has been actually. I hope more rails folks will start to realize that now that they saw what can happen when you do AR#update_attributes in the controllers.

@solnic So would you mind doing a +1 here: https://github.com/rails/rails/issues/3157 ? :)

@solnic : does it mean you never want to do mass assignment from anything other than a submitted form ? :)

@gucki I kind of agree with this idea but I would prefer to see this as a mixin that you can use in any object, not just controller :)

@oelmekki oh sorry I didn't express my thoughts clearly enough (I'm before breakfast and before my morning coffee) - of course I want to use mass assignment in other places :) and that's exactly my point, it's the context that matters. In a controller which exposes your AR model you care about a specific level of security and I don't think that AR should be taking care of this. When it is taking care of this though you make it aware of the context which basically means that AR knows there's a web form and a user with potential malicious intents.

I really like that somebody mentioned Django form objects here as I think it's an elegant way of dealing with such problems. In fact our DataMapper buddy @emmanuel is working on a project which will implement form objects in Ruby.

@solnic oh, ok, I see your point, and I agree. I like the idea of form builders / sanitizers too. Sure, it would add a new abstraction level that could seems unnecessarily heavy, but we could add some default form_builder with easy to use helpers, and let use custom class or module if we want to refine behavior.

We may also need a less specific class for sanitizer, because request handling is not only about displaying forms. Something like :

module RequestSanitizer
end

class FormBuilder
  include RequestSanitizer
end

I like the solution proposed by @wycats, at least when the nonce-style signing proposed by @johndouthat is used. Any thoughts on how the code should look fresh out of a "rails generate"? I think, seeing as how this is currently a hot issue partly because of non-secure defaults, that an empty whitelist should be generated in the controller, i.e.

attr_accessible nil```

On another note, explicitly building forms in the controller requires a major shift in thinking style, since it makes the form itself a part of the actual controls of the application. I see both @kylefox's and @aflag's points and I think the reality of your application matters here: is the form an actual entity according to the business logic? Or is it just some way of creating or editing something, a way of interacting? I'm having a hard time explaining the exact difference between those two viewpoints, but it does trigger a kind of "fingerspitzengefühl." In the first case, reasoning about it in a model seems fair, while in the latter case, since it is more or less part of the "knobs and dials" of the application, it seems like it should not be in a model. Then again, if there is some reasoning about who is allowed to fill out what part of the form, doesn't that mean it _is_ business logic?

I think @oelmekki's solution is the cleanest. Every object should be responsible for validating their input before giving output. Controllers should validate params (input) before updating a model (output). Models should validate update_attributes (input) before saving to the database (output).

IMO separation of concerns is very clear. The model shouldn't know anything about the context or session in which it is being used. If you also need to use those models in a safe way through Resque or some other non-controller path, you should be using a Conductor or Mediator that is responsible for handling the context.

Declare all your forms will be very annoying, i don't like this approach and this smell very bad...

I think http://api.rubyonrails.org/classes/ActiveModel/MassAssignmentSecurity/ClassMethods.html is a clean and simple solution, but could be improved

:+1: in the controller.

:+1: for comment in generated models too.

My 2 cents: As @byroot says "The sh*tty thing about Rails is that there is almost nothing between the HTTP layer and the model layer." Having something between can be useful for other "issues" preventing controllers to be as skinny as they should. My approach on the matter: https://gist.github.com/1978727

I don't understand the assumption that the controller is the correct place to define authorization. It's clearly an okay place to check it and take action - for example, 403ing a user if if user.can_edit?(something) is false - but why would definition belong there? Why not consider defining authorization as part of app logic outside of the HTTP request/response cycle?

since I am using rails mainly to deliver restful json this Form approach sounds wrong: there are just no Forms. looking at @oelmekki is very close want I like to see: having a RequestSanitizer decorator around my model. the FormBuilder could use the same RequestSanitizer when your are really dealing with forms in the view.

@mkristian I think part of the confusion comes from calling this intermediate validating entity a "form", because it doesn't necessarily have to result in an HTML form. The way you suggested using a RequestSanitizer is very similar to how "forms" work in Django, and you could use such an object without ever displaying an HTML form (ie: JSON API).

The issue I see with a RequestSanitizer and handling validation/authorization in the Controller is that it's then tied to the HTTP response cycle. Defining what attributes are updatable under what circumstances is business logic, and doesn't belong in a controller. Keep the controllers skinny!

@kylefox :+1: especially that you really want to properly unit test this business logic and having to initialize entire rails controller in your test sounds really bad

@solnic Good point, it would simplify testing. And then you could confidently use your form (or whatever it's called) in all sorts of controllers/views without having to worry about authorization leaks.

Another question about the original proposal: how would it work with nested models?

@kylefox - yes, is seems to be only the naming which bothered me. I do
agree with you that this kind of business logic needs its place
OUTSIDE the controller.

@solnic:

For example what's the problem with mass-assignment? The problem is that
a user of your system can send a web form with fields that you didn't expect.

Unless you have a simple Web only app, user submitted data are not necessarily received directly from a controller. Data could be coming via an import from a file, via a mechanism in a Resque worker, pushed in innocently by a developer at a console, or from other models creating dependent objects. Rather than slap bandaids over every user of an object, just make the object more secure OR ensure that ALL access to that object is via a proxy that implements the security.

Now, if the controller IS the only object that can interact with model objects (as in a naive Web only Rails app), it's fine to put security features there, but if not, you either need a wrapper or add security to the underlying object in order to reduce coupling.

And while AR may be too heavy, rippingo ut the security features first is folly. We have validations in AR and validations doing far more pointless things like checking for "acceptances" and "confirmations", cosmetic UI focused things that totally belong in controller land.

While mass assignment whitelist in controller does theoretically make more sense....

...it would be even MORE of a backwards incompatible change than what rails core team seems to have already rejected. Continuing with attr_accessible, but making it default to empty whitelist, instead of default to all attributes mass assignable.

While waiting for potential major architectural change like wycats suggests (OR instead of it, if it's deemed too backwards incompat), I can't understand why rails core team doesn't go with an application config that switches whether models with no attr_accessible decleration have ALL attributes mass-assignable (current), or NO attributes accessible (safe).

The config key could even default to current unsafe 'all are mass assignable with no attr_accessible' if unset, but new rails projects could generate to set it to the safe setting, for maximum backwards compat. Or, probably better, it could default to the safe setting, but if someone upgrades and doesn't want to add attr_accessible everywhere, they could simply set the config for the legacy unsafe behavior.

It is astounding to me that rails core team doesn't want to take this fairly simple step, which would still allow significant backwards compatibility when explicitly chosen by developer, but provide at least new apps with much safer behavior. The proposal here is a much more major change, much less backwards compat, even if it is in principle a better architecture were you starting over from scratch. Backwards compat does need to matter in rails, so I'm not sure if this more radical change is a good idea or not -- simply defaulting to requiring attr_accessible for any mass assignment seems to me likely good enough, and in fact much more backwards compat if you have a config to restore old unsafe behavior.

@peterc yeah I totally get that, see my other comment (the one about coffee :))

Regarding adding more responsibilities to the controller see my comment about unit tests. Controllers in rails are already too big, adding anything more is a mistake (same goes with AR as I already pointed out).

I obviously understand that it would be a convenient thing to do (like a lot of other things in rails) but the problem is that it would teach people that it's the right thing to do all the time. Now the outcome of this is that everybody would start implementing the security model in their controllers which is simply wrong in every case where you're building something bigger than a simple CRUD app.

@jrochkind: I don't think what @wycats proposed would necessarily be backwards incompatible. Deprecation and removal of AR#attr_accessible is optional, and obviously would be put off a few years. AC#attr_accessible could be put in tomorrow and would break absolutely nothing, assuming an empty blacklist as default.

@peterc: You're absolutely right that acceptance and confirmation validations don't belong in the model. That is absolutely no reason to compromise here. Perfect solution fallacy. Also, having multiple paths of model access is a perfect reason why attr_accessible should not be in the model. There's a different set of access restrictions with each context, and the model should not know about all the different ways in which it will be used. Whoever is accessing the model is responsible for maintaining that context and the access controls that go with it. If there's a lot of common logic between (for instance) your controllers and your resque workers, you should use a Conductor or Mediator to act as a middle man.

@subwindow: I assumed that wycats was proposing NOT assuming an empty blacklist/allow all attributes mass assignable. After all, if that's left as it is... has anything been fixed? Isn't that assumption the whole root of the problem? Otherwise, what am I missing, how is wycats proposal -- with a default empty blacklight/all attributes allowed -- likely to result in a security improvement? I understand how it's a more elegant api with seperation of concerns in the right theoretical place.... are you guys suggesting that will result in people using it who didn't use attr_accessible?

The default empty blacklight/universal whitelist seems to be the actual main problem exhibited by recent events, right? If you leave that alone, aren't you just shuffling deck chairs?

@jrochkind: I think the default generated application should have a default whitelist. However, I really have no opinion on defaults for existing applications. It kind of blows either way- you're either creating busy work by forcing everyone to declare their whitelists or you're promoting bad patterns by not. I think the "Rails Way" would be the latter.

@subwindow: that would make this discussion somewhat academic, because it would still require the user to know about whitelists, blacklists before being able to securely build an application. I, for one, would rather take a couple of minutes to explain to a new user why some attribute needs to be whitelisted.

Keep in mind that developers new to the job should be assisted in delivering both working and secure software. We need to stop them from delivering leaky software to customers who have every right to expect better. Of course we want them to be able to easily start creating applications, but when these applications are delivered to customers and are later found to be riddled with holes, the fallout change the public opinion about Rails.

@jrochkind: The thing is that many people do not define attr_accessible attributes properly, even dhh do not https://gist.github.com/1975644 . Why ? Because it's painful, and provide only security, which is often unfortunately not the first concern.

In other side, you will almost never see a Django app passing user inputs into the model layer without sanitize them through a form. Why ? Because Django's form classes are the more easy / straightforward / powerful way to build an HTML form.

The only way to convince rails developers to use a security feature is to wrap it into a nice productivity/simplicity ribbon.

Since standard Rails way to build HTML forms is not that great and productive, IMHO a simple_form clone but with class based form declaration that allow both easier form building in the view layer AND inputs sanitization in the controller layer could be that "nice ribbon".

1) A signed token in the form is a very bad idea from a security perspective. There are many ways to mess up the crypto implementation, and it requires yet another secret for the the application to manage. The secret has to be shared by the host that generates the form and the host that handles the POST, which means it is going to likely be static and in configuration. BAD. It is conflating crypto, authentication and authorization.

2) It makes sense for this to be in the controller, since the controller is really what concerns itself with user authentication and authorization. It is part of request handling. Making models user aware is really ugly. I have tried it for real apps with with very granular authorization on models...it quickly gets hard to maintain and actually drives the developer to complex security models. Think REST and keep authorization simple. Either the user can take this action or they can't. Use route name-spacing and role controllers with controller-level attribute authorization. This is simple to understand and simple to implement.

I am using ActiveResource quite a bit to get data from other
API-servers and sometimes I need to persists this data with
ActiveRecord. so any controller based solution will keep me in the
rain !

in Rails you can replace the ORM layer, for example with DataMapper.
so any solution based on AR "forces" ORMs to do the same. or as
application developer I better find a way which is independent of the
used ORM.

for me personally Forms objects like Django is the way to go, there I
can use AR, ActiveModel, Virtues or DataMapper and all will be
protected. similar the data can come from the controller via a request
or the data comes from a ActiveResource and the protection is still in
place ;)

@peterc I agree with you that validations should stay in the model, still, it doesn't explain why we can't explicitly say which params an action (thinking : request handler) is allowed to receive and how it should sanitize it.

Sure, I want my model to validate if a record is correctly associated with an other. But does I really want to check the user didn't input some <other_resource>_id in the model too ? Or does I really want to typecast a String to a Number in the model because html form posted it as String and I need a Number to pass as param to a model method ?

As for the multiple sources, using some kind of modular Sanitizer (or Form-that-would-not-represent-a-form) would let handle that without any pain, even with more flexibility, actually. As @solnic mentionned, needed sanitization may change upon context. An admin may change <other_resource>_id after all, and the :as attribute of attr_accessible sounds like a controller condition in disguise to me, leading to things like :

class Foo < ActiveRecord::Base
  attr_accessible, :title, :content
  attr_accessible :user_id, as: :admin
end

class FoosController < ApplicationController
  def update
    ...
    @foo.update_attributes( @attrs, as: current_user.is_admin && whatever == 1 ? :admin : :user )
  end
end

Since a good part of this discussion is about context, and who should know or care about it, perhaps validations should also be considered. Not that validations should be removed from the object, but what validations should apply in what context. This mostly comes into play when building an object using a wizard like interface. Though you can persist the object outside of AR (in a cookie, for example), there are valid reasons to persist it through AR. I have used a tool like https://github.com/akira/validationgroup to do this. Perhaps a similar solution could help in whitelisting also. You create named groups of valid fields in the model (so they are all in one place), then the controller uses these groups to sanitize the incoming data. The model doesn't need to know what context each group is being used in, but serves only to define the group.

@byroot I made this years ago and use it for every project https://github.com/tizoc/bureaucrat

I don't think it is a good fit for Rails anyway, because by being ORM agnostic it doesn't provide some very useful functionality Django has like sharing validations between models and forms, and also defining forms from models (ModelForms).

@byroot btw, I don't use Rails, but I use that library in Sinatra/Cuba/Rack/Whatever applications.

@byroot another approach is to separate it into three different parts like the Pylons guys did.

Validation http://docs.pylonsproject.org/projects/colander/en/latest/
Presentation http://docs.pylonsproject.org/projects/deform/en/latest/
Parsing of data into a common structure http://docs.pylonsproject.org/projects/peppercorn/en/latest/

With peppercorn you convert everything to a common structure, the one your validation layer can process. This way you can validate anything that comes from html form data, json, xml, whatever. The presentation layer is also separate because not all data will be presented as HTML forms anyway, so it makes sense to have it as a separate component.

@tizoc, About defining forms from models: being ORM agnostic is not an issue. Simple simple_form works fine with either activerecord and mongoid.

@byroot to me it looks like a different kind of abstraction, not at all like what Django does. If you look at how Django does it, Models and Forms are very well integrated to each other, while at the same time being independent on from the other.

I liked the django-like forms idea that came up here. Started experimenting with it. My first result: https://github.com/indrekj/funky_form

@tizoc @byroot's solution is totally similar to what Django's form layer does -- the APIs are even the same! The only feature bureaucrat is missing is the (trivial) task of automatically generating a form from a model (ie a ModelForm in django). Really nice work @byroot, I'll definitely be using bureaucrat on future projects.

@kylefox I was talking about simple_form, I know very well that bureaucrat does what Django does being that I'm the author, and I made it based on the Django solution for forms.

@tizoc Oy, my bad. Totally missed that :)

@indrekj take a look at https://github.com/tizoc/bureaucrat -- some of the code there may be useful to you
Also see my comments here for a different approach: https://gist.github.com/1974187#gistcomment-88936

@indrekj btw, I think that for a Rails solution, basing things on ActiveModel (like you are doing with funky_form) is the right approach. It requires less code, and matches better with Rails.

Yes, get the "security layer" out of the model, please :)

I would agree that using ActiveModel is the way to go, but i would suggest empowering it so that attr_accesible receives an array of roles. For example my problem is that many different roles have access to different attributes. For example i would like too use this in my model:

attr_accessible :first_name, :as=> :admin

and this in my controller

@student.update_attributes(params[:student], :as => user_roles)

where user_roles is an array of symbols:

user_roles = [:admin, :employee]

This would be a huge help not adding any more logic to the update and create actions

I don't really like the way attr_accessible is used. It puts business logic into the models and makes the code very unmodular.

As far as the controller itself being aware of the resource it's acting upon also seems unnecessary and cumbersome. If the goal is to provide convention over configuration, then it has to be in an abstract enough such that it can apply to almost any kind of application.

Something like what is mentioned above from @jalagrange would work, but by specifying the fields which can be update, it is more abstract and allows for more flexibility.

@student.update_attributes(params[:student], :fields => current_user.accessible_attributes_for(@student))

This can be abstracted and etc, but passing roles specifically doesn't seem very modular whereas passing an array of fields which are accessible is more abstract and accomplishes the same thing while being more flexible.

I imagine if this existed, a library like cancan would add features like

can :update, Student, :attributes => [:name, :age]

In the view you would have helpers like

f.text_field :name if can? :update, @student, :attribute => :name

and controllers would be like

def update
  @student = Student.find(params[:id]) #Cancan checks if current_user can update the student
  if @student.update_attributes(params[:student], :fields => current_user.accessible_attributes_for(@student))
    ...
  end

end

I absolutely agree @turlockmike, your Cancan example is exactly the way I would like for it to work!

:+1:

I think models are responsible for keeping themselves consistent—i.e. validations, callbacks, etc—and mutating themselves, not for authorizing how you mutate them. Mass assignment security as currently implemented has annoyed me hellishly for ages. Big applications always end up requiring complex action- and role-based mass assignment rules and validations which end up a mess.

I like the idea of an accept_params (and perhaps reject_params) macro for controllers which simply inserts a before_filter to check that there's nothing unexpected in params, or returns a 400. Given params reflects XML, JSON, query and form parameters, this seems ideal for user-facing web and API. Use the existing filter processing machinery to limit per-action and halt requests. It's also fairly non-invasive and would be easy to prototype.

Looking at @jonleighton's post earlier, this was already available for merb. Actually, it's already available for rails, too.

@turlockmike and @jalagrange: that update_attributes variation is pretty close to what i've been doing with http://github.com/cainlevy/mass_assignment. i've found it to be a dead simple but powerful foundation that can be built up with extra helpers like you describe.

As @TMaYaD and @diegorv have said, isn't this already in Rails?

See (again, as mentioned by @diegorv) : http://api.rubyonrails.org/classes/ActiveModel/MassAssignmentSecurity/ClassMethods.html

@jamesduncombe: that's a version of it. i dislike the roles implementation as it's configuration-heavy and reminds me far too much of those awful model-based attr_protected/attr_accessible declarations. i think the method that @turlockmike describes and that i've been using for a couple of years is simpler, more powerful, and leaves the configuration and roles implementation to the discretion of the developer and other gem authors. personally i love the obvious visibility into exactly which methods can be assigned via any given controller action, and moving the assignable attribute list to some other forgettable place just seems like a recipe for accidents.

@jamesduncombe, yes it is implemented in rails, but in a very simple, restrictive way. For example you can assign roles such as the one in the example:

attr_accessible :first_name, :last_name, :plan_id, :as => :admin

The problem is, there is no way in which you can implement this with an array of roles. Therefore you are limited so only :admin's (in this example) can modify the :last_name attribute. In @turlockmike Cancan example, on would be able to distinguish by roles if a student role for example can change attributes that maybe a guardian or admin role cant or viceversa.

@jalagrange, @cainlevy interesting. I'm going to look into that, thanks!

@turlockmike CACAN 2.0 is implementing what you suggest.

Resource Attributes

It is possible to define permissions on specific resource attributes. For example, if you want to allow a user to only update the name and priority of a project, pass that as the third argument to can.

can :update, :projects, [:name, :priority]

If you use this in combination with load_and_authorize_resource it will ensure that only those two attributes exist in params[:project] when updating the project. If you do this everywhere it will not be necessary to use attr_accessible in your models.

You can combine this with a hash of conditions. For example, here the user can only update the price if the product isn't discontinued.

can :update, :products, :price, :discontinued => false

You can check permissions on specific attributes to determine what to show in the form.

<%= f.text_field :name if can? :update, @project, :name %>

@bradphelan Excellent. That's great news.

Just to add my 2 cents here.. I don't mind the idea of moving mass-assignment protection to the controllers (or even to a Django-style Form object). I don't think it's necessary, but I don't mind, it won't be much of a learning curve (and only a very minor inconvenience) if that happens.

I don't like the idea of defining the allowed attributes as a hidden form field, signed or not. Sure, it sounds like a great idea of you're using server-rendered ERB, Haml or Slim templates with the form_for helper.. but what happens if you're doing your front-end in Backbone or Ember and using Handlebars templates? How are we passing that hidden form field to the front-end templates that are rendered by Javascript in the browser and not by Ruby on the server? Are we just ignoring them in that case? If so, it defeats the purpose of having them in the first place. Considering that more and more apps are moving to a frontend Javascript framework, Rails really needs to continue to support both use-cases.. everything rendered on the server, or everything passed back and forth as JSON and rendered in the browser (and even the possible third use-case, mixing those two strategies).

@stevenh512 Even if I subscribe to most of what you said, beware of the argument of "don't put magic in views", here, as it can lead to consider all helpers as a annoyance. I love using mustache to share templates between server side and client side, too, but if mustache can't handle ruby helpers, that's a mustache problem (not that I think it should, for obvious client side reasons).

The solution, here, is usually to use a presenter or mvtc pattern, which call helpers and is used to compute variables for either the template, the json response or whatever you need. The hidden inputs could be implemented as a #allowed_fields_tags( *fields ), which would be called by #form_for, or directly in a view/presenter class.

I'm not arguing against putting "magic" in views, although I'd have to say that most existing Rails view helpers can be fairly easily duplicated with Ember views and Handlebars helpers without passing any extra information (like the allowed fields) back and forth in the JSON. What I'm arguing is that we shouldn't be putting "unnecessary magic" in the views when the logic belongs to the controllers and/or the models. The app on the server, whether the logic is in the model or the controller (or both), is more than capable of deciding what is and isn't allowed without some magic hidden form field or extra data being passed back and forth in the JSON.

The code is already there and has been for years, it's just a matter of people actually using it. It should be turned on by default. To use a real-world analogy here, how many carjackings have been prevented since the major auto manufacturers started making new cars do one simple thing that drivers have been unwilling to do on their own for years even though they had to know it was a security concern (locking the doors while the car is in gear)? The power door locks have been there for a long time, it's just a matter of wiring them up so they lock by default.

While mass-assignment is definitely a security issue, it's not the same kind of issue as XSRF where you actually do need some magic value to be passed back and forth (and even then, it's not sent to the client in the JSON, there's a meta tag for that.. but the same solution wouldn't work here).

With Rails 3.1+ scoped attr_accessible, I honestly don't see why the logic even needs to be moved from models to controllers.. with the code as it is now you're more than capable of handling mass-assignment at the authorization level in your controllers while keeping attr_accessible in the model (and keeping it DRY, which I thought was supposed to be a Rails convention)... but even moving it to the controllers makes a lot more sense to me than adding unnecessary magic to the views that does nothing but complicate things for anyone who needs to send data to the server in any way other than through a form generated with the form_for helper. Let's not just think about Javascript frontends here.. what if you want to build an app with an API? Are you going to expect everyone who consumes your API to handle this magic value that's really of no use to them (which means at least one extra API call just to get the magic value before they can submit any data.. which means more load on your server and theirs) when your model or your controller can and should be taking care of it? So what about those APIs? Do we just turn this off, like people typically do with the XSRF protection in that situation? I think turning this off for API calls without some other layer of protection the model and/or controller would be much more dangerous than turning off the XSRF protection is in the same situation.. and again, if the model or the controller can do it already why do we need this magic (and if they can't, then this magic is clearly the wrong answer anyway)?

What I really think needs to be done, and it actually needed to be done a long time ago (how long has it been known now that mass-assignment is a security issue? since before Rails 1?) is to actually enforce whitelisting by default, which a recent Rails commit at least takes a step toward doing even if I don't think they went far enough with it.

I can see two trends here: one repelling that all validation goes on the model, for those who want to make it dependent on the context, and one repelling that all params sanitation goes on the controller, for those who want to make it dependent on the internals of the business logic. The solution here offered has been a new layer taking care of this issues, á la Dyango Forms, but this could lead to view logic messed up with everything else, this layer becoming a kind of glue between everything.

I indeed support the introduction of a new layer in Rails (Conductor/Presenter/Mediator/Decorator/Template), but adding a new perspective. I advocate a ResourceAccessor, which not only stays between a model and its controller, but besides incarnates the concept of Resource (which can be understood as made of parts of one or several related models). This resource should relate each attribute to its respective model's field and should make validations depending on the user's authorization level. It would be called by the controller on GET actions and passed onto the view, which could automatically rely on the ResourceAccessor's field definitions to display the appropiate type of field (either for scaffolded display or in forms). It would receive the params from the controller, sanitize and handle them out to the respective save methods of each model, usually inside a transaction. Mass assignment would become impossible, since only explicitly defined (and eventually authorized) fields could be set.

This approach solves said issues of mass assignment, bloated models, and repetitive view declarations.
It also integrates the new Rails 4.0 verb PATCH and with the new meaning of PUT together, giving full sense to the creation of a method replace_resource, which could be meaningfully applied to such ResourceAccessor instead of the current update_attributes whenever a full replace is meant (see my take on that).

Feel free to elaborate on the concept. Just my two cents.

Kind of this idea by @alexeymuranov, but extending it so that the accessor can reach more than a single model out. It does not have to break backwards compatibilty, since you can always call directly a model, but you'd be encouraged not to, because the code would be much more simple, beautiful and testeable.

And nested parameters get included for free, since you can include the children inside the ResourceAccessor itself, or call their respective ResourceAccessor to take care of everything related to them.

@miguelsan I like that approach. I don't necessarily disagree with anyone who thinks it belongs in the controller or anyone who thinks it belongs in the model, either of those is fine with me (I just don't think it belongs in the view when it's backend logic :grin:). Something like this should satisfy the needs of anyone arguing either side of that question.

That's what I think as well. Both sides have their right, so the solution must be obviously in the middle. There is no better option than adding a full fledged layer before the model providing security, simpler views and separation of concerns. The biggest advantage being the decoupling between the concepts of Backend Model and REST Resource, for which there is no solution provided until now. Actually, many developers aren't even aware of the difference.

But, are the almighty Rails' gurus going to dare adding yet another layer, one which almost throws the holy MVC acronym on the ground? How long will they defer it? Til Rails15? I wish not.

@miguelsan The problem is that there are three different things being discussed here.

  1. Defining which attributes are accessible in general

  2. Deciding where to put rules concerning the logic of role based assignments for attributes

  3. Easily displaying different views for forms based on role based assignments of attributes

All of these problems are different, but highly related.

One way to solve this problem is to add another layer, but to keep with the MVC framework the simplest solution is:
1. define private attributes in the model (application logic)
2. define attributes which can only be written based on roles in the controller (business logic)
3. Any # of solutions for the views ranging from multiple views, to helper methods which take into account roles.

The only other way to do this would be to add another layer, but it seems like the information to do this already exists.

@miguelsan I like this concept. Have you ever tried an implementation of it as a proof of concept ?

Oh, and for the rails core integration, no need to wait, rails is flexible enough to let you implement it as a third-party gem ;)

Wouldn't a fourth item be validations? It may be that you want to control which validations are run, based on the attributes expected to be received.

@tulockmike I think the conversation about attributes is missing the abstraction. We really only have three things going on

  1. Authentication ( Who you are )
  2. Authorization. ( Are you allowed to affect some state change which for clarity should be named )
  3. Validation. ( Is the current state of some object valid )

Authentication happens in the middleware. It is a global state.
Authorization happens on specific controller actions. CANCAN makes this clear

def update
    authorize! :update, @some_model_instance
end

Validation happens after a state change has been affected. It assumes the state change is authorized and is a sanity
check on the object fields.

However it is clear that in many cases it is not enough to just authorize! update, @some_model_instance and
then blindly apply update_attributes. An example makes it clear. Say we have the following model.

class User < ActiveRecord::Base
    has_many :images
end

class Image < ActiveRecord::Base
    belongs_to :user # image owner
    validates_presence_of :attachement
    validates_presence_of :title
end

We have two contexts in which we may wish to update an image object.

* change the owner
* change the title

Both of these actions can be done through update_attributes passing in the params
hash. Easy to do. However it is evil. What you really should have on the object
are methods to perform specific state changes.

class Image < ActiveRecord::Base
    belongs_to :user # image owner
    validates_presence_of :attachement
    validates_presence_of :title

    def change_owner params
        update_attributes params.slice(:owner_id)
    end

    def change_title
        update_attributes params.slice(:title)
    end
end

Now our controller would have to look like

class ImagesController < ApplicationController

    load_resource

    def change_owner
        authorize! :change_owner, @image
        @image.change_owner params[:image]
    end     

    def change_title
        authorize! :change_title, @image
        @image.change_title params[:image]
    end

end

So now we have correct separation of concerns. The model is fully responsible for defining it's state changes and the api for those state changes. The controller is fully responsible for deciding who is allowed to invoke the state change.

Of couse this results in our controllers slipping away from the standard

index
delete
update
show
destroy

suite of methods. In theory you could create a set of nested controllers to implement the above image controller and in fact it probably is the right thing to do. Imagine a web form to change the ownership of the image

class ChangeOwner < ApplicationController

    # Nested under  /images/:id/change_owner/new
    def new
        authorize! :change_owner, @image 
        render :new
    end

    def create
        authorize! :change_owner, @image
        @image.change_owner params[:image]
    end
end

Again there are no attribute authorizations made in the controller only authorizations made on specific state changes which invoke methods on the model. To make this kind of thing easier on the developer I would suggest method generation helpers on the models to change

class Image < ActiveRecord::Base
    belongs_to :user # image owner
    validates_presence_of :attachement
    validates_presence_of :title

    def change_owner params
        update_attributes params.slice(:owner_id)
    end

    def change_title params
        update_attributes params.slice(:title)
    end
end

to

class Image < ActiveRecord::Base
    belongs_to :user # image owner
    validates_presence_of :attachement
    validates_presence_of :title

    state_change :change_owner, :owner_id do params
        update_attributes params
    end

    state_change :change_title, :title do params
        update_attributes params
    end
end

which could be compressed in default as

class Image < ActiveRecord::Base
    belongs_to :user # image owner
    validates_presence_of :attachement
    validates_presence_of :title

    state_change :change_owner, :owner_id 
    state_change :change_title, :title
end

but then again too much magic got us where we are today.

@bradphelan sorry, guy, after all the work of yours writing that comment, it can all be chopped down to:

class ChangeOwner < ApplicationController
    # ...
    def create
        authorize! :change_owner, @image
        @image.update_attribute :owner, params[:image][:owner_id]
    end
end

Where CanCan helps you like this:

can :change_owner, :images, :attributes => :owner_id

There is no need for this at all:

state_change :change_owner, :owner_id 
state_change :change_title, :title

I mean, that can be already done today. But we hate having to write a different controller and authorization rule for each attribute, that's why we resort to update_attributes. So, you are right in that you are proposing best practices, but the result is long and boring, and Rails should provide a way to make it short and beautiful, I guess.

My vote goes for a new layer that underlines the difference between model and resource, also allowing for multiple models exposed as a single resource. And no, @oelmekki, I haven't written any code yet. I just got the idea yesterday, but I also wouldn't have the time, sorry.

@migueisan +5 for funny but I didn't mean to confuse you with the DSL magic and make you not notice that the example was deliberately degenerate.

My main issue is that too much responsibility is given to the update method when, with a complex model (which my example is not ) you should be calling object methods with a tightly defined parameter list to effect state change and not be using update_attributes with role based attr_accessible Just reading DHH's preferred method at http://rubydoc.info/gems/strong_parameters/0.1.3/frames and it makes sense with my suggested approach except I would move the strong parameter signatures into the model under explicit method names.

class Image < ActiveRecord::Base
    belongs_to :user # image owner
    validates_presence_of :attachement
    validates_presence_of :title

    def change_owner params
        update_attributes params.require(:image).permit(:owner_id)
    end

    def change_title params
         update_attributes params.require(:image).permit(:title)
    end
end

and in CANCAN

can :change_owner, Image if user.has_role? :admin
can :change_title, Image if user.has_role? :user

CANCAN enabled controller reduces to

class ImageController < ApplicationController
    load_and_authorize_resource

    def change_title
        @image.change_title params
    end

    def change_owner
        @image.change_owner params
    end
end

@wycats your idea to set the allowed attributes in the form is very good!
So I've written a gem https://github.com/webhoernchen/massassignment_security_form for our rails project.
It extends the form_helpers, adds the allowed attributes to the form and removes all other values of the params hash!

I am actually a security engineer that happens to use rails a lot. I can assure your that setting the allowed fields in the form is terrible from a security perspective. You can read why 50 comments ago.

Just to reiterate, the protection mechanism as untrusted data in the attacker's control is a recipe for failure. Developers rarely get crypto right. Why would you think this case is any different?

On Apr 2, 2012, at 4:15 AM, Christian Eichhornreply@reply.github.com wrote:

@wycats your idea to set the allowed attributes in the form is very good!
So I've written a gem webhoernchen/massassignment_security_form for our rails project.
It extends the form_helpers, adds the allowed attributes to the form and removes all other values of the params hash!


Reply to this email directly or view it on GitHub:
https://gist.github.com/1974187

A failure mode with your form signing is the following scenario. I have an
object X that I have rights to update all fields for. I save the encrypted
hash for that form and then apply it against an object I only have partial
rights for. I am now have elevated my privilege level. I'm pretty sure your
code doesn't deal with this problem after a quick review. Though its
probably possible to fix for this scenario it is kind of scary to do this
sort of thing.
On Apr 2, 2012 2:31 PM, "Dominique Brezinski" <
reply@reply.github.com>
wrote:

I am actually a security engineer that happens to use rails a lot. I can
assure your that setting the allowed fields in the form is terrible from a
security perspective. You can read why 50 comments ago.

Just to reiterate, the protection mechanism as untrusted data in the
attacker's control is a recipe for failure. Developers rarely get crypto
right. Why would you think this case is any different?

On Apr 2, 2012, at 4:15 AM, Christian Eichhornreply@reply.github.com
wrote:

@wycats your idea to set the allowed attributes in the form is very good!
So I've written a gem webhoernchen/massassignment_security_form for our
rails project.
It extends the form_helpers, adds the allowed attributes to the form and
removes all other values of the params hash!


Reply to this email directly or view it on GitHub:
https://gist.github.com/1974187


Reply to this email directly or view it on GitHub:
https://gist.github.com/1974187

@dominiquebrezinski: The allowed attributes will be encrypted in the form automatically and not allowed attributes in the controller removed.
You can find a description in the README:

In your erb file:

<% form_tag({:action => "update"}) do %>
  <table>
    <tr>
      <td><label for="user_first_name">First name:</label></td>
      <td><%= text_field :user, :first_name %></td>
    </tr>
    <tr>
      <td><label for="user_name">Name:</label></td>
      <td><%= text_field :user, :name %></td>
    </tr>
  </table>
<% end %>

It creates the following html form:

<form method="post" action="/route/to/users/update">
  <table>
    <tr>
      <td><label for="user_first_name">First name:</label></td>
      <td><input type="text" value="Christian" size="30" name="user[first_name]" id="user_first_name"></td>
    </tr>
    <tr>
      <td><label for="user_name">Name:</label></td>
      <td><input type="text" value="Eichhorn" size="30" name="user[name]" id="user_name"></td>
    </tr>
  </table>

  <input type="hidden" value="EncryptedHashWithFormFieldsForUser" name="massassignment_fields">
</form>

The controller can parse the "EncryptedHashWithFormFieldsForUser" to "{'user' => ['first_name', 'name']}".
All other attributes of the params[:user] will be remoevd!

if I read encrypted forms I immediately need to think about: "what
about rest clients talking to a restful rails server ? how do they
protect their models ? do I need to implement some kind of encryption
on all my rest clients as well ?"

@bradphelan with your approach you'd need two new methods (one for model and one for controller) and an entry under abilities.rb for each attribute. That's far from clean and dry code. Can't buy it.

@miguesan I am suggesting validating actions on models based on the abstract name of the action occurring not on the individual attributes being set. Form signing is at the wrong level of abstraction.

There is nothing wrong with the role based attr_accessible method currently in rails except that it defaults to permissive, which has now been fixed. If you want to have a role based mapping to individual attributes, there is nothing really wrong with that.

def update
    authorize :update, @model
    params = params_for_role
    @model.update_attributes params
end

def params_for_role
    case current_user.role
    when :admin
        params.require(:model).permit :x, :y, :z
    when :user
        params.require(:model).permit :x
    default:
        raise "arggh!"
    end
end

and have a blanket

can :update, Model

the other way to achieve the same thing is

controller

def do_admin_stuff
    authorize :do_fancy_stuff, @model
    @model.do_fancy_stuff params.require(:model)
end

def do_user_stuff
    authorize :do_simple_stuff, @model
    @model.do_simple_stuff params.require(:model)
end

model

class Model < ActiveRecord::Base
    def do_simple_stuff
        update_attributes params.permit(:x)
    end

    def do_fancy_stuff
        update_attributes params.permit(:x, :y, :z)
    end
end

ability.rb

# Can do fancy stuff if admin or own the object
can :do_fancy_stuff, Model if user.role? :admin
can :do_fancy_stuff :user_id => user.id

# Can do simple stuff to a model if a user
can :do_simple_stuff if user.role? :user

Note in this case there are no attributes authorizations in CANCAN. Valid attributes
are defined only once as valid argument lists to model methods. This is a very simple
way to achieve this new layer. No need to over-engineer a solution.

@bradphelan You could say that Rails is overengineered when compared to Sinatra. Of course there are simpler cases where any solution seems feels like "too much". And those simple cases are actually the mayority. That's why a new layer must be able to seem transparent in its "default" mode and at the same time offer powerful features.

You are not dealing, for instance, with the problem of attributes from nested models (as many have noted). Or, as I said, with different models acting as a single resource for the end user. There is where a not overengineered solution comes into play.

I agree with @dominiquebrezinski that storing an encrypted version of the allowed fields on the client is probably not a good idea. It reminds me of ASP.NET where application state was also stored on the client. Practical Padding Oracle Attacks, developed based on a theory by Vaudenay completely devastated the security there.

Within the application, we know as a fact exactly what fields are allowed. Sending this information on a round trip to the client and back would only weaken what was already established as a hard fact. On an abstract level, cryptography would be used trying to prove something to ourselves that we already knew without it in the first place.

Note that this is very different from signed authentication tokens - there it's actually the user trying to prove something (their identity) to us (the application). In the "encrypted/signed fields" case it's just us trying to prove something to ourselves. In addition, the crypto would put more computational effort on the server and more data would be sent over the wire for each request/response, state would have to be tracked and, as we all know, it's very easy to make mistakes. For example when dealing with MACs, it can be very easy to make yourself vulnerable to timing attacks. If the fields are only encrypted and neither MACed nor signed, then we're basically screwed due to the malleability of ciphertexts. Then there's replay attacks and whatnot else.

But even if everything is done right, exposing ciphertexts like this to the client offers the possibility for brute-forcing your way in. There is no restriction on how many ciphertexts an attacker could obtain from the application - they would behave like ordinary users. A really sophisticated solution that would exploit volume and parallelism would certainly have a chance to brute-force keys within an acceptable time frame.

The particular encryptor used in this suggestion does appear to be unverified and susceptible to chosen plaintext attacks. Not only that, but it will generate an uncaught exception in OpenSSL's block cipher decrypt routine when ciphertexts are tampered with; it thus has a padding oracle.

Rails includes a well-reviewed message encryptor in ActiveSupport::MessageEncryptor; MessageEncryptor#sign_and_encrypt includes an HMAC-SHA1 signature on the ciphertext, which defeats these attacks. typing AES into your Rails app (or extension) is indeed a code smell.

Agreed. Im having some trouble about this.

# Attr :name is protected in model, but in some cases
# would be easy to access by this way:
Tag.create(name: 'foo')
# Actually, I needed to do this:
tag = Tag.new
tag.name = 'foo'
tag.save

Please sign in to comment on this gist.

Something went wrong with that request. Please try again.