I was upgrading a client’s application to Rails 2.1.2 from 2.0.x the other day, this resulted in refreshing a number of gems and plugins including will_paginate and attachment_fu. In doing so I found a stupid problem that I couldn’t find anything about online – it’s a pretty one off unique case so I thought I’d blog about it to help the next person. ‘Pay it forward’ I guess.
The problem was this: somewhere along the line attachment_fu was updated to execute the following line of code on classes using the has_attachment directive:
attr_accessible :uploaded_dataThis is taking advantage of a security feature in Rails, it instructs the model to only allow :uploaded_data to be set through update_attributes and any like methods such as new and create. So whitelisting bulk settable attributes. It doesn’t raise any errors, just warnings in your log like this:
WARNING: Can't mass-assign these protected attributes: <attribute_name>
Due to no errors being raised the problem was presenting itself in other parts of the application as expected data was not being found. Fortunately I was tailing my dev log and noticed the above warning message.
The problem in this situation was the historic code creating instances of the model by passing a hash through to SomeModel.create. As this area of code was not utilising data posted to an action it was not a security risk. Very fustrating, especially as this was common practice throughout the codebase.
As I didn’t want to do a huge bunch of re-keying my initial solution was to comment out the offending line in attachment_fu. Yes this is a big nasty fix but there was time pressure, other problems to solve and I didn’t want to be held up having caused an unnecessary amount of manual testing. In case it was forgotten about and overriden with a future plugin update, a quick test with big comment was added.
This project hadn’t updated it’s gems and plugins for a while, meaning it was a big shock to the system when they where. This was the biggest headache, all other problems were mainly just gem interfaces changing, so a easy search and replace matter.
I like passing a hash to mass-assign data in Rails as well as the obvious coding benefits it’s visually more readable too, but this is out weighed by my great dislike of having my sites hacked into. So it’s a shame things have had to go this way.
![[del.icio.us]](http://www.motionstandingstill.com/wp-content/plugins/bookmarkify/delicious.png)
![[Digg]](http://www.motionstandingstill.com/wp-content/plugins/bookmarkify/digg.png)
![[dzone]](http://www.motionstandingstill.com/wp-content/plugins/bookmarkify/dzone.png)
![[Fark]](http://www.motionstandingstill.com/wp-content/plugins/bookmarkify/fark.png)
![[Google]](http://www.motionstandingstill.com/wp-content/plugins/bookmarkify/google.png)
![[LinkedIn]](http://www.motionstandingstill.com/wp-content/plugins/bookmarkify/linkedin.png)
![[Reddit]](http://www.motionstandingstill.com/wp-content/plugins/bookmarkify/reddit.png)
![[Shoutwire]](http://www.motionstandingstill.com/wp-content/plugins/bookmarkify/shoutwire.png)
![[Slashdot]](http://www.motionstandingstill.com/wp-content/plugins/bookmarkify/slashdot.png)
![[Sphinn]](http://www.motionstandingstill.com/wp-content/plugins/bookmarkify/sphinn.png)
![[StumbleUpon]](http://www.motionstandingstill.com/wp-content/plugins/bookmarkify/stumbleupon.png)
![[Technorati]](http://www.motionstandingstill.com/wp-content/plugins/bookmarkify/technorati.png)
![[Twitter]](http://www.motionstandingstill.com/wp-content/plugins/bookmarkify/twitter.png)
No related posts.
5 Comments
Hey thanks for this insight I was having the same issue and I commented out line 171 in the file attchment_fu.rb located in the vendor plugin folder. Seems to have done the trick, I am only uploading images and I haven’t run into any problems yet with that one line commented out. I will post here if I do run into any troubles. Thanks again
Instead of commenting out the line, just list the attributes that you want to be set via mass assignment. This protects the rest of the stuff (parent_id, height, etc). Restart your server afterwards to make sure the file is initialized with your changes.
Line 179:
base.attr_accessible :uploaded_data, :your_colum_name, etc
Doing that is still a huge pain to maintain though as the same situation could occur later when someone else adds a new field to the model. I’m not working for that client anymore, so I’ve told them what I did and left some hard to miss comments but ….
When it comes down to it I don’t think a plugin should be doing that as it’s business logic which should lie on the model itself rather than hidden somewhere else.
I had a similar issue where I had set an attribute (id) that was not a model accessor. I chose to manually set the id attribute and then remove it from the hash — rather than make the id an accessor:
1. def image_attributes=(image_attributes)
2. image_attributes.each do |attributes|
3. if attributes[:id].blank?
4. photos.build(attributes)
5. else
6. photo = photos.detect { |t| t.id == attributes[:id].to_i}
7. photo.id = attributes[:id]
8. attributes.delete(“id”)
9. photo.attributes = attributes
10. end
11. end
12. end
We get alot of speed with the Rails framework, e.g., Mass Assignment Hashes…but can spend alot of time debugging it, too.
I believe that the best course of action is to comment out the line from the plugin and add it manually to whichever model actually needs it.
This way you keep security and control on your code.