Instantly share code, notes, and snippets.

Embed
What would you like to do?

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.

@jamesarosen

This comment has been minimized.

Show comment
Hide comment
@jamesarosen

jamesarosen Mar 4, 2012

+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.

jamesarosen commented Mar 4, 2012

+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.

@bai

This comment has been minimized.

Show comment
Hide comment
@bai

bai Mar 4, 2012

👍
attr_accessible shouldn't be in model.

bai commented Mar 4, 2012

👍
attr_accessible shouldn't be in model.

@wycats

This comment has been minimized.

Show comment
Hide comment
@wycats

wycats Mar 4, 2012

@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.

Owner

wycats commented Mar 4, 2012

@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.

@djones

This comment has been minimized.

Show comment
Hide comment
@djones

djones Mar 4, 2012

👍 this looks really good.

djones commented Mar 4, 2012

👍 this looks really good.

@eduardordm

This comment has been minimized.

Show comment
Hide comment
@eduardordm

eduardordm commented Mar 4, 2012

+1

@jamesarosen

This comment has been minimized.

Show comment
Hide comment
@jamesarosen

jamesarosen Mar 4, 2012

@wycats it sure does :)

jamesarosen commented Mar 4, 2012

@wycats it sure does :)

@jonleighton

This comment has been minimized.

Show comment
Hide comment
@jonleighton

jonleighton Mar 4, 2012

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).

jonleighton commented Mar 4, 2012

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).

@tadast

This comment has been minimized.

Show comment
Hide comment
@tadast

tadast Mar 4, 2012

@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 commented Mar 4, 2012

@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...

@rrouse

This comment has been minimized.

Show comment
Hide comment
@rrouse

rrouse Mar 4, 2012

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

rrouse commented Mar 4, 2012

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

@wycats

This comment has been minimized.

Show comment
Hide comment
@wycats

wycats Mar 4, 2012

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
Owner

wycats commented Mar 4, 2012

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
@oelmekki

This comment has been minimized.

Show comment
Hide comment
@oelmekki

oelmekki Mar 4, 2012

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

oelmekki commented Mar 4, 2012

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

@bai

This comment has been minimized.

Show comment
Hide comment
@bai

bai Mar 4, 2012

@oelmekki like django forms, hehe

bai commented Mar 4, 2012

@oelmekki like django forms, hehe

@indrekj

This comment has been minimized.

Show comment
Hide comment
@indrekj

indrekj Mar 4, 2012

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

indrekj commented Mar 4, 2012

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

@jneen

This comment has been minimized.

Show comment
Hide comment
@jneen

jneen Mar 4, 2012

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

jneen commented Mar 4, 2012

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
@ericanderson

This comment has been minimized.

Show comment
Hide comment
@ericanderson

ericanderson Mar 4, 2012

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

ericanderson commented Mar 4, 2012

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

@ericanderson

This comment has been minimized.

Show comment
Hide comment
@ericanderson

ericanderson Mar 4, 2012

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

ericanderson commented Mar 4, 2012

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

@steveklabnik

This comment has been minimized.

Show comment
Hide comment
@steveklabnik

steveklabnik Mar 4, 2012

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

steveklabnik commented Mar 4, 2012

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

@dasch

This comment has been minimized.

Show comment
Hide comment
@dasch

dasch Mar 4, 2012

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 commented Mar 4, 2012

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.

@wycats

This comment has been minimized.

Show comment
Hide comment
@wycats

wycats Mar 4, 2012

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

Owner

wycats commented Mar 4, 2012

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

@solnic

This comment has been minimized.

Show comment
Hide comment
@solnic

solnic Mar 4, 2012

👍 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.

solnic commented Mar 4, 2012

👍 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.

@seanbamforth

This comment has been minimized.

Show comment
Hide comment
@seanbamforth

seanbamforth Mar 4, 2012

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.

seanbamforth commented Mar 4, 2012

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.

@amalagaura

This comment has been minimized.

Show comment
Hide comment
@amalagaura

amalagaura Mar 4, 2012

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.

amalagaura commented Mar 4, 2012

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.

@NZKoz

This comment has been minimized.

Show comment
Hide comment
@NZKoz

NZKoz Mar 4, 2012

The current functionality is already a mixin, fwiw

NZKoz commented Mar 4, 2012

The current functionality is already a mixin, fwiw

@johndouthat

This comment has been minimized.

Show comment
Hide comment
@johndouthat

johndouthat Mar 4, 2012

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.

johndouthat commented Mar 4, 2012

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.

@eac

This comment has been minimized.

Show comment
Hide comment
@eac

eac Mar 4, 2012

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

eac commented Mar 4, 2012

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

@henrik

This comment has been minimized.

Show comment
Hide comment
@henrik

henrik commented Mar 4, 2012

@seanbamforth Rails 3.1 has that.

@johndouthat

This comment has been minimized.

Show comment
Hide comment
@johndouthat

johndouthat Mar 4, 2012

@eac how can it be evaded?

johndouthat commented Mar 4, 2012

@eac how can it be evaded?

@oelmekki

This comment has been minimized.

Show comment
Hide comment
@oelmekki

oelmekki Mar 4, 2012

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.

oelmekki commented Mar 4, 2012

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.

@wycats

This comment has been minimized.

Show comment
Hide comment
@wycats

wycats Mar 4, 2012

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

Owner

wycats commented Mar 4, 2012

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

@sujal

This comment has been minimized.

Show comment
Hide comment
@sujal

sujal commented Mar 4, 2012

+1

@derekprior

This comment has been minimized.

Show comment
Hide comment
@derekprior

derekprior Mar 4, 2012

+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.

derekprior commented Mar 4, 2012

+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.

@suweller

This comment has been minimized.

Show comment
Hide comment
@suweller

suweller commented Mar 4, 2012

👍

@byroot

This comment has been minimized.

Show comment
Hide comment
@byroot

byroot Mar 5, 2012

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.

byroot commented Mar 5, 2012

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.

@TMaYaD

This comment has been minimized.

Show comment
Hide comment

TMaYaD commented Mar 5, 2012

@MSch

This comment has been minimized.

Show comment
Hide comment
@MSch

MSch Mar 5, 2012

+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

MSch commented Mar 5, 2012

+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
@byroot

This comment has been minimized.

Show comment
Hide comment
@byroot

byroot Mar 5, 2012

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.

byroot commented Mar 5, 2012

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.

@ericanderson

This comment has been minimized.

Show comment
Hide comment
@ericanderson

ericanderson Mar 5, 2012

ericanderson commented Mar 5, 2012

@byroot

This comment has been minimized.

Show comment
Hide comment
@byroot

byroot Mar 5, 2012

@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.

byroot commented Mar 5, 2012

@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.

@aflag

This comment has been minimized.

Show comment
Hide comment
@aflag

aflag Mar 5, 2012

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 commented Mar 5, 2012

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?

@wycats

This comment has been minimized.

Show comment
Hide comment
@wycats

wycats Mar 5, 2012

@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.

Owner

wycats commented Mar 5, 2012

@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.

@dfischer

This comment has been minimized.

Show comment
Hide comment
@dfischer

dfischer Mar 5, 2012

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

dfischer commented Mar 5, 2012

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

@cainlevy

This comment has been minimized.

Show comment
Hide comment
@cainlevy

cainlevy Mar 5, 2012

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

cainlevy commented Mar 5, 2012

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

@jgaskins

This comment has been minimized.

Show comment
Hide comment
@jgaskins

jgaskins Mar 5, 2012

@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.

jgaskins commented Mar 5, 2012

@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.

@peterc

This comment has been minimized.

Show comment
Hide comment
@peterc

peterc Mar 5, 2012

"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.

peterc commented Mar 5, 2012

"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.

@jmonteiro

This comment has been minimized.

Show comment
Hide comment
@jmonteiro

jmonteiro Mar 5, 2012

+1 I like this. I really do.

jmonteiro commented Mar 5, 2012

+1 I like this. I really do.

@drhenner

This comment has been minimized.

Show comment
Hide comment
@drhenner

drhenner Mar 5, 2012

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 commented Mar 5, 2012

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
@peterc

This comment has been minimized.

Show comment
Hide comment
@peterc

peterc Mar 5, 2012

@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).

peterc commented Mar 5, 2012

@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).

@kylefox

This comment has been minimized.

Show comment
Hide comment
@kylefox

kylefox Mar 5, 2012

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 commented Mar 5, 2012

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.

@aflag

This comment has been minimized.

Show comment
Hide comment
@aflag

aflag Mar 5, 2012

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

aflag commented Mar 5, 2012

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

@kylefox

This comment has been minimized.

Show comment
Hide comment
@kylefox

kylefox Mar 5, 2012

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.

kylefox commented Mar 5, 2012

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.

@drhenner

This comment has been minimized.

Show comment
Hide comment
@drhenner

drhenner Mar 5, 2012

@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.

drhenner commented Mar 5, 2012

@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.

@aflag

This comment has been minimized.

Show comment
Hide comment
@aflag

aflag Mar 5, 2012

@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 commented Mar 5, 2012

@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.

@kylefox

This comment has been minimized.

Show comment
Hide comment
@kylefox

kylefox Mar 5, 2012

@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 commented Mar 5, 2012

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

@drhenner

This comment has been minimized.

Show comment
Hide comment
@drhenner

drhenner Mar 5, 2012

@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

drhenner commented Mar 5, 2012

@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

@gucki

This comment has been minimized.

Show comment
Hide comment
@gucki

gucki commented Mar 5, 2012

I recommend having a look at https://github.com/cjbottaro/param_protected

@gucki

This comment has been minimized.

Show comment
Hide comment
@gucki

gucki Mar 5, 2012

And my issue from a few months ago: rails/rails#3157

gucki commented Mar 5, 2012

And my issue from a few months ago: rails/rails#3157

@solnic

This comment has been minimized.

Show comment
Hide comment
@solnic

solnic Mar 5, 2012

@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 commented Mar 5, 2012

@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.

@gucki

This comment has been minimized.

Show comment
Hide comment
@gucki

gucki Mar 5, 2012

@solnic So would you mind doing a +1 here: rails/rails#3157 ? :)

gucki commented Mar 5, 2012

@solnic So would you mind doing a +1 here: rails/rails#3157 ? :)

@oelmekki

This comment has been minimized.

Show comment
Hide comment
@oelmekki

oelmekki Mar 5, 2012

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

oelmekki commented Mar 5, 2012

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

@solnic

This comment has been minimized.

Show comment
Hide comment
@solnic

solnic Mar 5, 2012

@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 commented Mar 5, 2012

@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.

@oelmekki

This comment has been minimized.

Show comment
Hide comment
@oelmekki

oelmekki Mar 5, 2012

@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

oelmekki commented Mar 5, 2012

@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
@ReneB

This comment has been minimized.

Show comment
Hide comment
@ReneB

ReneB Mar 5, 2012

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?

ReneB commented Mar 5, 2012

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?
@diegorv

This comment has been minimized.

Show comment
Hide comment
@diegorv

diegorv commented Mar 5, 2012

+1 @TMaYaD

attr_accessible is already available in controller isn't? http://api.rubyonrails.org/classes/ActiveModel/MassAssignmentSecurity/ClassMethods.html

@subwindow

This comment has been minimized.

Show comment
Hide comment
@subwindow

subwindow Mar 5, 2012

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.

subwindow commented Mar 5, 2012

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.

@diegorv

This comment has been minimized.

Show comment
Hide comment
@diegorv

diegorv Mar 5, 2012

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

diegorv commented Mar 5, 2012

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

@AlSquire

This comment has been minimized.

Show comment
Hide comment
@AlSquire

AlSquire Mar 5, 2012

👍 in the controller.

👍 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

AlSquire commented Mar 5, 2012

👍 in the controller.

👍 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

@gabrielg

This comment has been minimized.

Show comment
Hide comment
@gabrielg

gabrielg Mar 5, 2012

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?

gabrielg commented Mar 5, 2012

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?

@mkristian

This comment has been minimized.

Show comment
Hide comment
@mkristian

mkristian Mar 5, 2012

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 commented Mar 5, 2012

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.

@kylefox

This comment has been minimized.

Show comment
Hide comment
@kylefox

kylefox Mar 5, 2012

@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 commented Mar 5, 2012

@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!

@solnic

This comment has been minimized.

Show comment
Hide comment
@solnic

solnic Mar 5, 2012

@kylefox 👍 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 commented Mar 5, 2012

@kylefox 👍 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

@kylefox

This comment has been minimized.

Show comment
Hide comment
@kylefox

kylefox Mar 5, 2012

@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 commented Mar 5, 2012

@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?

@mkristian

This comment has been minimized.

Show comment
Hide comment
@mkristian

mkristian Mar 5, 2012

mkristian commented Mar 5, 2012

@peterc

This comment has been minimized.

Show comment
Hide comment
@peterc

peterc Mar 5, 2012

@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.

peterc commented Mar 5, 2012

@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.

@jrochkind

This comment has been minimized.

Show comment
Hide comment
@jrochkind

jrochkind Mar 5, 2012

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.

jrochkind commented Mar 5, 2012

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.

@solnic

This comment has been minimized.

Show comment
Hide comment
@solnic

solnic Mar 5, 2012

@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.

solnic commented Mar 5, 2012

@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.

@subwindow

This comment has been minimized.

Show comment
Hide comment
@subwindow

subwindow Mar 5, 2012

@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 commented Mar 5, 2012

@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.

@jrochkind

This comment has been minimized.

Show comment
Hide comment
@jrochkind

jrochkind Mar 5, 2012

@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 commented Mar 5, 2012

@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?

@subwindow

This comment has been minimized.

Show comment
Hide comment
@subwindow

subwindow Mar 5, 2012

@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 commented Mar 5, 2012

@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.

@ReneB

This comment has been minimized.

Show comment
Hide comment
@ReneB

ReneB Mar 5, 2012

@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.

ReneB commented Mar 5, 2012

@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.

@byroot

This comment has been minimized.

Show comment
Hide comment
@byroot

byroot Mar 5, 2012

@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".

byroot commented Mar 5, 2012

@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".

@dominiquebrezinski

This comment has been minimized.

Show comment
Hide comment
@dominiquebrezinski

dominiquebrezinski Mar 6, 2012

  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.

dominiquebrezinski commented Mar 6, 2012

  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.

@mkristian

This comment has been minimized.

Show comment
Hide comment
@mkristian

mkristian Mar 6, 2012

mkristian commented Mar 6, 2012

@oelmekki

This comment has been minimized.

Show comment
Hide comment
@oelmekki

oelmekki Mar 6, 2012

@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

oelmekki commented Mar 6, 2012

@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
@hoenth

This comment has been minimized.

Show comment
Hide comment
@hoenth

hoenth Mar 6, 2012

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.

hoenth commented Mar 6, 2012

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.

@tizoc

This comment has been minimized.

Show comment
Hide comment
@tizoc

tizoc Mar 6, 2012

@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).

tizoc commented Mar 6, 2012

@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).

@tizoc

This comment has been minimized.

Show comment
Hide comment
@tizoc

tizoc Mar 6, 2012

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

tizoc commented Mar 6, 2012

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

@tizoc

This comment has been minimized.

Show comment
Hide comment
@tizoc

tizoc Mar 6, 2012

@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 commented Mar 6, 2012

@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.

@byroot

This comment has been minimized.

Show comment
Hide comment
@byroot

byroot Mar 6, 2012

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

byroot commented Mar 6, 2012

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

@tizoc

This comment has been minimized.

Show comment
Hide comment
@tizoc

tizoc Mar 6, 2012

@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.

tizoc commented Mar 6, 2012

@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.

@indrekj

This comment has been minimized.

Show comment
Hide comment
@indrekj

indrekj Mar 6, 2012

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

indrekj commented Mar 6, 2012

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

@kylefox

This comment has been minimized.

Show comment
Hide comment
@kylefox

kylefox Mar 6, 2012

@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 commented Mar 6, 2012

@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.

@tizoc

This comment has been minimized.

Show comment
Hide comment
@tizoc

tizoc Mar 6, 2012

@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 commented Mar 6, 2012

@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.

@kylefox

This comment has been minimized.

Show comment
Hide comment
@kylefox

kylefox Mar 6, 2012

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

kylefox commented Mar 6, 2012

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

@tizoc

This comment has been minimized.

Show comment
Hide comment
@tizoc

tizoc Mar 6, 2012

@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

tizoc commented Mar 6, 2012

@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

@tizoc

This comment has been minimized.

Show comment
Hide comment
@tizoc

tizoc Mar 6, 2012

@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.

tizoc commented Mar 6, 2012

@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.

@cordawyn

This comment has been minimized.

Show comment
Hide comment
@cordawyn

cordawyn Mar 7, 2012

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

cordawyn commented Mar 7, 2012

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

@jalagrange

This comment has been minimized.

Show comment
Hide comment
@jalagrange

jalagrange Mar 7, 2012

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

jalagrange commented Mar 7, 2012

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

@turlockmike

This comment has been minimized.

Show comment
Hide comment
@turlockmike

turlockmike Mar 8, 2012

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

turlockmike commented Mar 8, 2012

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
@jalagrange

This comment has been minimized.

Show comment
Hide comment
@jalagrange

jalagrange Mar 8, 2012

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

jalagrange commented Mar 8, 2012

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

@sj26

This comment has been minimized.

Show comment
Hide comment
@sj26

sj26 Mar 8, 2012

👍

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.

sj26 commented Mar 8, 2012

👍

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.

@cainlevy

This comment has been minimized.

Show comment
Hide comment
@cainlevy

cainlevy Mar 8, 2012

@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.

cainlevy commented Mar 8, 2012

@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.

@jamesduncombe

This comment has been minimized.

Show comment
Hide comment
@jamesduncombe

jamesduncombe commented Mar 8, 2012

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

@cainlevy

This comment has been minimized.

Show comment
Hide comment
@cainlevy

cainlevy Mar 8, 2012

@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.

cainlevy commented Mar 8, 2012

@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.

@jalagrange

This comment has been minimized.

Show comment
Hide comment
@jalagrange

jalagrange Mar 8, 2012

@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 commented Mar 8, 2012

@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.

@jamesduncombe

This comment has been minimized.

Show comment
Hide comment
@jamesduncombe

jamesduncombe Mar 8, 2012

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

jamesduncombe commented Mar 8, 2012

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

@bradphelan

This comment has been minimized.

Show comment
Hide comment
@bradphelan

bradphelan Mar 13, 2012

@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 commented Mar 13, 2012

@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 %>
@turlockmike

This comment has been minimized.

Show comment
Hide comment
@turlockmike

turlockmike Mar 13, 2012

@bradphelan Excellent. That's great news.

turlockmike commented Mar 13, 2012

@bradphelan Excellent. That's great news.

@jalagrange

This comment has been minimized.

Show comment
Hide comment
@jalagrange

jalagrange commented Mar 15, 2012

Awesome!!

@stevenh512

This comment has been minimized.

Show comment
Hide comment
@stevenh512

stevenh512 Mar 16, 2012

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 commented Mar 16, 2012

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).

@oelmekki

This comment has been minimized.

Show comment
Hide comment
@oelmekki

oelmekki Mar 16, 2012

@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.

oelmekki commented Mar 16, 2012

@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.

@stevenh512

This comment has been minimized.

Show comment
Hide comment
@stevenh512

stevenh512 Mar 16, 2012

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.

stevenh512 commented Mar 16, 2012

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.

@miguelsan

This comment has been minimized.

Show comment
Hide comment
@miguelsan

miguelsan Mar 28, 2012

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.

miguelsan commented Mar 28, 2012

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.

@miguelsan

This comment has been minimized.

Show comment
Hide comment
@miguelsan

miguelsan Mar 28, 2012

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.

miguelsan commented Mar 28, 2012

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.

@miguelsan

This comment has been minimized.

Show comment
Hide comment
@miguelsan

miguelsan Mar 28, 2012

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 commented Mar 28, 2012

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.

@stevenh512

This comment has been minimized.

Show comment
Hide comment
@stevenh512

stevenh512 Mar 29, 2012

@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 😁). Something like this should satisfy the needs of anyone arguing either side of that question.

stevenh512 commented Mar 29, 2012

@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 😁). Something like this should satisfy the needs of anyone arguing either side of that question.

@miguelsan

This comment has been minimized.

Show comment
Hide comment
@miguelsan

miguelsan Mar 29, 2012

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 commented Mar 29, 2012

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.

@turlockmike

This comment has been minimized.

Show comment
Hide comment
@turlockmike