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

Password Reset Workflow Question 🔍 #5643

Open
jon-sully opened this issue Oct 17, 2023 · 2 comments
Open

Password Reset Workflow Question 🔍 #5643

jon-sully opened this issue Oct 17, 2023 · 2 comments

Comments

@jon-sully
Copy link

Howdy! I've looked around a bit and don't think I've seen this particular question asked or discussed, so I wanted to raise it here.

Environment

  • Ruby 3.2.2
  • Rails 7.0.8 && Rails 7.1.1
  • Devise 4.9.2

Current behavior

Just using the default Devise controllers and views, when a user request a password reset they're sent a tokenized link. This link takes them to the reset-password page where they enter a new password, hit submit, and their user record is updated with the new password information.

If the user clicks the link they were sent again after that point, it still loads the password reset page and, being plain HTML inputs, allows the user to enter a new password and click the submit button. Only after that POST does a response come back with "reset password token invalid", which is confusing for the user since they just entered a new password 🤔

I think there's a more subtle issue here too since many password managers will pre-fill a new password for the user when they observe the page loaded and save that new password to the user's key-store when the form is submitted (even though the response comes back as a 4xx) — so the user could actually lose their real, correct password in some cases.

The question is, is this the intended workflow? Could the password reset controller check the validity of the token before rendering the page and redirect the user back to the 'request password reset' page if the token is invalid?

Obviously can and will subclass the appropriate controllers and get this working in my own app, just wanted to raise the question around the default / built-in stuff here.

Thanks!

@jon-sully
Copy link
Author

In the meantime (and for others curious about the same thing) I've added the following controller (don't forget to add it to your routes devise_for.. controllers: { passwords: "here" }). My resource type in this case is Agent; yours will likely be User or otherwise; YMMV.

class AgentPasswordsController < Devise::PasswordsController
  before_action :ensure_valid_token, only: [:edit]

  private

  def ensure_valid_token
    original_token = params[:reset_password_token]
    reset_password_token = Devise.token_generator.digest(self, :reset_password_token, original_token)

    agent = Agent.find_or_initialize_with_errors([:reset_password_token], reset_password_token: reset_password_token)

    # Encodes the same logic as Devise::Models::Recoverable.reset_password_by_token
    if !agent.persisted? || !agent.reset_password_period_valid?
      redirect_to new_agent_password_path, alert: "Password reset link no longer valid; please request a new link."
    end
  end
end

So at least we're not overwriting any of the methods in the PasswordsController we're subclassing, but it'd be cool if Devise::Models::Recoverable exposed a reset_password_token_valid? method that could be used both inside the implementation of .reset_password_by_token and elsewhere in the app as needed — doesn't feel the best reimplementing the same logic here manually 🤔

@timdiggins
Copy link

I agree with this. However there could be an argument that this makes password reset enumeration attacks a tiny bit easier (because you only need a simple get to see if there is an valid password reset), and also it's worth doing a redirect with a different status code (e.g. 422) so that it's easier to set up a Rack::Attack throttling strategy for repeated attempts.

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

No branches or pull requests

2 participants