Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Mass assignment security #718

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion README.rdoc
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,7 @@ After you choose which modules to use, you need to set up your migrations. Lucki
t.timestamps
end

Devise doesn't use _attr_accessible_ or _attr_protected_ inside its modules, so be sure to define attributes as accessible or protected in your model.
Devise doesn't use _attr_accessible_ or _attr_protected_ inside its modules, but it filters input parameters in its own controllers using {ActiveModel::MassAssignmentSecurity}[http://api.rubyonrails.org/classes/ActiveModel/MassAssignmentSecurity/ClassMethods.html]. It's up to you to filter your params passed to your model, either in your controllers, or using _attr_accessible_ or _attr_protected_.

Configure your routes after setting up your model. Open your config/routes.rb file and add:

Expand Down
13 changes: 11 additions & 2 deletions app/controllers/devise/registrations_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,9 @@ class Devise::RegistrationsController < ApplicationController
prepend_before_filter :require_no_authentication, :only => [ :new, :create, :cancel ]
prepend_before_filter :authenticate_scope!, :only => [:edit, :update, :destroy]
include Devise::Controllers::InternalHelpers
include ActiveModel::MassAssignmentSecurity

attr_accessible :email, :password, :password_confirmation, :current_password

# GET /resource/sign_up
def new
Expand Down Expand Up @@ -35,7 +38,7 @@ def edit

# PUT /resource
def update
if resource.update_with_password(params[resource_name])
if resource.update_with_password(safe_params)
set_flash_message :notice, :updated
sign_in resource_name, resource, :bypass => true
redirect_to after_update_path_for(resource)
Expand Down Expand Up @@ -67,7 +70,7 @@ def cancel
# Build a devise resource passing in the session. Useful to move
# temporary session data to the newly created user.
def build_resource(hash=nil)
hash ||= params[resource_name] || {}
hash ||= safe_params || {}
self.resource = resource_class.new_with_session(hash, session)
end

Expand Down Expand Up @@ -107,4 +110,10 @@ def authenticate_scope!
send(:"authenticate_#{resource_name}!")
self.resource = resource_class.to_adapter.get!(send(:"current_#{resource_name}").to_key)
end

# Filter params for this resource using ActiveModel::MassAssignmentSecurity
# White-listed params are specified in the controller, not the model.
def safe_params
sanitize_for_mass_assignment(params[resource_name])
end
end
5 changes: 1 addition & 4 deletions lib/generators/active_record/devise_generator.rb
Original file line number Diff line number Diff line change
Expand Up @@ -18,10 +18,7 @@ def copy_devise_migration
end

def inject_devise_content
inject_into_class model_path, class_name, model_contents + <<-CONTENT
# Setup accessible (or protected) attributes for your model
attr_accessible :email, :password, :password_confirmation, :remember_me
CONTENT
inject_into_class model_path, class_name, model_contents
end
end
end
Expand Down
24 changes: 24 additions & 0 deletions test/integration/registerable_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,17 @@ class RegistrationTest < ActionController::IntegrationTest
assert_contain 'You need to sign in or sign up before continuing.'
end

test 'a guest cannot set arbitrary attributes on signup' do
post user_registration_path, :user => {
:email => "test@example.org",
:password => "123456",
:password_confirmation => "123456",
:facebook_token => "owned" }
user = User.first
assert_equal "test@example.org", user.email
assert_nil user.facebook_token
end

test 'a signed in user should not be able to access sign up' do
sign_in_as_user
get new_user_registration_path
Expand Down Expand Up @@ -176,4 +187,17 @@ class RegistrationTest < ActionController::IntegrationTest
assert_nil @request.session["devise.foo_bar"]
assert_redirected_to new_user_registration_path
end

test 'a signed in user cannot alter arbitrary attributes by posting extra params' do
user = sign_in_as_user :password => "123456", :email => "original@example.org"

put "/users", :user => {
:current_password => "123456",
:email => "new@example.org",
:facebook_token => "owned" }

user.reload
assert_equal "new@example.org", user.email
assert_nil user.facebook_token
end
end
2 changes: 0 additions & 2 deletions test/rails_app/app/active_record/user.rb
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,4 @@
class User < ActiveRecord::Base
include Shim
include SharedUser

attr_accessible :username, :email, :password, :password_confirmation, :remember_me
end