Reliance on attr_accessible is dangerous and inflexible

[pda]

Thanks for the great work on Devise! It works great out of the box, and I was easily able to implement a basic SSO cookie extension on top of it.

Something that concerns me about Devise, however, is the reliance on attr_accessible.

---

It is dangerous

Forgetting, misconfiguring or accidentally removing the attr_accessible declaration from the model in an app which otherwise validates all user input at a controller level opens a massive security hole, allowing arbitrary rewriting of User data. If an application author chooses to rely on this filtering, that's one thing, but it seems dangerous for a library to do so.

It isn't flexible enough

I don't see attr_accessible as useful for anything but the most basic CRUD application. It's not nearly flexible enough because many models have more than one edit interface, and it moves a controller concern (input validation) into the model.

Being free of attr_accessible filtering, I'm able to use #update_attributes and #attributes= in any context, not just form handling. However with a Devise-backed User object, this isn't possible.

It doesn't seem necessary

As far as I can tell, the attr_accessible filtering is only used via DatabaseAuthenticatable::update_with_password which in turn is only called by Devise::RegistrationsController::update. This controller action should filter the submitted params down to the fields specified in views/registrations/edit.html.erb, being email, password, password_confirmation and current_password. If necessary, it could then be configurable to support custom views. Did I miss anything else which relies on mass assignments being protected?

Limited support outside ActiveRecord

DataMapper says: "Specifying allowed in #update_attributes is deprecated, use Hash#only to filter the attributes in the caller."

---

What are your thoughts on moving away from the use of attr_accessible?

If you decide to do so, let me know if there's anything I can do to help.

Cheers!
Paul

[josevalim]

Devise already moves away from attr_accessible, as it delegates the logic to your application. What were you thinking instead as a solution? Having a configuration option?

[pda]

It should be possible to remove the attr_accessible :email, :password, ... from the User model in a standard Devise setup, and not have a security hole in the Devise::RegistrationsController::update page.

So my solution would be to manually filter the params in that controller:

diff --git a/app/controllers/devise/registrations_controller.rb b/app/controllers/devise/registrations_controller.rb
index 8257e4b..b032dce 100644
--- a/app/controllers/devise/registrations_controller.rb
+++ b/app/controllers/devise/registrations_controller.rb
@@ -35,7 +35,7 @@ class Devise::RegistrationsController < ApplicationController

   # PUT /resource
   def update
-    if resource.update_with_password(params[resource_name])
+    if resource.update_with_password(params[resource_name].slice(*allowed_parameters))
       set_flash_message :notice, :updated
       sign_in resource_name, resource, :bypass => true
       redirect_to after_update_path_for(resource)
@@ -107,4 +107,9 @@ class Devise::RegistrationsController < ApplicationController
       send(:"authenticate_#{resource_name}!")
       self.resource = resource_class.to_adapter.get!(send(:"current_#{resource_name}").to_key)
     end
+
+    # Parameters accepted by #update
+    def allowed_parameters
+      [ :email, :password, :password_confirmation, :current_password ]
+    end
 end

This filters down to the parameters submitted by the default view. If a custom view is being used with more parameters, then the #allowed_parameters method could be overridden.

As far as I can tell, this would eliminate the need for attr_accessible on the model. Is there anywhere else that it's depended on?

[josevalim]

I am ok with something like this as long as it defaults to attr_accessible attributes fir Active Record. Another option would be to rely on ActiveModel::MassAssignmentProtection. I also believe it is used in other places, like reset_password and user creation, so this needs to be handled with more attention.

[pda]

Moved to pull request 718.

Thanks for the pointer to ActiveModel::MassAssignmentProtection José, I hadn't come across that.