ChiliProject is not maintained anymore. Please be advised that there will be no more updates.

We do not recommend that you setup new ChiliProject instances and we urge all existing users to migrate their data to a maintained system, e.g. Redmine. We will provide a migration script later. In the meantime, you can use the instructions by Christian Daehn.

[Proposal] Remove Model#safe_attributes

Added by Eric Davis at 2011-02-11 01:18 am

There are a few models that use a custom safe_attributes= method to set attributes on an object. This is to prevent user access to "unauthorized" attributes from controllers. But it is pretty much a reimplementation of Rails attr_accessible but in such a way that it can't easily be modified (note). The only reason I can find for using safe_attributes is in it's comment in issue.rb

  # attr_accessible is too rough because we still want things like
  # Issue.new(:project => foo) to work

With safe_attributes in place, every plugin needs to implement at least 3 different hooks in order to add a single field to the issue's form. Each of these hooks has sharp edge cases that I've run into causing all sorts of bugs (e.g. can't set attribute on new issue, can't set attribute on existing issue...)

(note: there is now custom API to 'add' more safe attributes.)

Proposal

Remove safe_attributes and switch to using attr_accessible. Running the tests and doing normal browser testing should turn up any instances where attr_accessible is too restrictive.

Thoughts?

Eric Davis


Replies (7)

RE: Proposal - remove Model#safe_attributes - Added by Felix Schäfer at 2011-02-11 08:37 am

I'm all for using rails built-ins where possible.

RE: Proposal - remove Model#safe_attributes - Added by Gregor Schmidt at 2011-03-14 09:48 pm

Commenting here, since I was redirected from #282

I cannot say much about the proposal, since my knowledge about the chili sources is rather limited. So the following is just based to my experience after one or two days of working with safe_attributes.

I dislike the need to differentiate between general mass assignment and safe mass assignment. I think this is error-prone and should be avoided. Using attr_protected would fix that.

I like the optional :if parameter available for safe_attributes. This is something, that is not supported by the default Rails API - AFAIK. I am not sure, if it really worth it, to fully avoid having such an API. It seems to be rather useful. So we might end up with implement something similar to safe_attributes, just naming it attr_protected.

Since attr_protected is white list based -- just like safe_attributes -- plugin developers would still need to extend the core class when trying to add attributes to existing models. The current API of safe_attributes seems to be just as good as the one for attr_protected in this regard. Please correct me, if I'm wrong.


I agree that using core functionality is better than implementing things on our own version of the same thing. The rest needs to evaluated by somebody with deeper knowledge about the Chili code base.

RE: Proposal - remove Model#safe_attributes - Added by Holger Just at 2011-03-14 09:59 pm

Gregor Schmidt wrote:

I like the optional :if parameter available for safe_attributes. This is something, that is not supported by the default Rails API - AFAIK. I am not sure, if it really worth it, to fully avoid having such an API. It seems to be rather useful. So we might end up with implement something similar to safe_attributes, just naming it attr_protected.

This could be used as a security feature of the upcoming rewrite of the permissions system later on. It fits nicely into my train of thought here.

Since attr_protected is white list based -- just like safe_attributes -- plugin developers would still need to extend the core class when trying to add attributes to existing models. The current API of safe_attributes seems to be just as good as the one for attr_protected in this regard. Please correct me, if I'm wrong.

I personally like to have an API where I just can declare things. As far as I understood it, the save_attributes API would allow me to define it without having to monkey patch the class. Which is a good thing in my book.

RE: Proposal - remove Model#safe_attributes - Added by Eric Davis at 2011-03-14 10:47 pm

Regarding the :if option:

You can also declare a setter for the field and have more control over it.

1  def isbn=(v)
2    write_attribute(:isbn, v) if @book.author == @user
3  end

Holger Just wrote:

I personally like to have an API where I just can declare things. As far as I understood it, the save_attributes API would allow me to define it without having to monkey patch the class. Which is a good thing in my book.

If you don't want to reopen a class we can define a class level method to add to the attr_protected/attr_accessible attributes. The main thing is I want to use the "protection" part of the code from Rails, instead of from a custom library. Otherwise we will have to maintain the library and all of the bugs along with it (like #282).

Eric Davis

RE: [Proposal] Remove Model#safe_attributes - Added by Eric Davis at 2011-07-22 03:53 pm

Gregor just posted a link to Michael Koziarski's security recommendation to use attr_protected or attr_accessible to prevent a potential DoS weakness.

Unless there is a compelling reason that hasn't been stated yet, I think we should proceed on replacing safe_attributes.

Eric Davis

RE: [Proposal] Remove Model#safe_attributes - Added by Felix Schäfer at 2011-08-20 10:40 pm

I like the idea of going with core rails stuff. Do we have a ticket for this already?

(1-7/7)