Password Reset Workflow Question 🔍

[jon-sully]

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]

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]

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.