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

Add strong parameters to the PasswordsController #5747

Open
hakeem0114 opened this issue Dec 18, 2024 · 3 comments · May be fixed by #5749
Open

Add strong parameters to the PasswordsController #5747

hakeem0114 opened this issue Dec 18, 2024 · 3 comments · May be fixed by #5749

Comments

@hakeem0114
Copy link

Problem

The Devise::PasswordsController uses unsanitized resource_params during password reset.

Proposal

DEFAULT_PERMITTED_ATTRIBUTES = {
      sign_in: [:password, :remember_me],
      sign_up: [:password, :password_confirmation],
      account_update: [:password, :password_confirmation, :current_password]
      reset_password: [:reset_password_token, :password, :password_confirmation]
    }
  • Use it in the Devise::PasswordsController.
@timdiggins
Copy link

@hakeem0114 I don't agree. The code you reference is

def resource_params
params.fetch(resource_name, {})
end

but params.fetch (see https://api.rubyonrails.org/classes/ActionController/Parameters.html#method-i-fetch) still returns a ActionController::Parameters instance (unless the resource name is not present in the params, but in this case it will return an empty hash) so it is still subject to ActionController::Parameters prevention of mass-assignment (like #require but without requiring the resource name), so doesn't AFAICT introduce a security problem needing fixing

@hakeem0114
Copy link
Author

@timdiggins Thanks for the comment!
This is a feature enhancement proposal not a bug. I can't seem to add a label on this issue for some reason.
When resource_params is called in the PasswordsController, it returns the entire resource (:user) instead of a sanitized one like the other controllers.

@hakeem0114 hakeem0114 linked a pull request Dec 20, 2024 that will close this issue
@timdiggins
Copy link

@hakeem0114 Ah - ok, if it's a feature request, what benefit does it bring? (the problem statement still implies to me that it's a security issue)

FYI After more looking, I think that in fact the security in devise (from not mass assigning) doesn't really come from strong parameters but from a very similar (but unconnected) route -- see Devise::Models::Authenticatable::ClassMethods# find_or_initialize_with_errors

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging a pull request may close this issue.

2 participants