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

common 'issue' with Devise Recoverable, password reset loop #5688

Open
jbeyer05 opened this issue May 10, 2024 · 6 comments
Open

common 'issue' with Devise Recoverable, password reset loop #5688

jbeyer05 opened this issue May 10, 2024 · 6 comments

Comments

@jbeyer05
Copy link

In applications where I use Devise, I have seen situations where users run into an issue with the 'Forgot Password' functionality, which is partly user error, but which also could be easily fixed with a simple change to the workflow. And I'm wondering if I've somehow missed something in the documentation that might allow for a configuration setting that would fix the issue.

Fairly frequently, users will generate a password reset link, lack any patience when the email doesn't immediately arrive, and request a second link. Then the first email arrives and they follow that link. The reset token in the first email has been invalidated by the second request. But they open that link, and request a reset link a third time. They now see the second email, and follow the second link, which is now invalid as well. And the loop repeats.

Am I missing a configuration option? It seems like I would need to roll a semi-custom solution to make this work, and Google isn't turning up any results on simple tutorials to do this.

It seems like Recoverable should have an option to NOT modify the reset password token within a timeframe specified via configuration (maybe 10 minutes). It wouldn't seem to have any security drawbacks that I can see.

@jbeyer05 jbeyer05 changed the title common 'issue' with Devise Recoverable common 'issue' with Devise Recoverable, password reset loop May 10, 2024
@jbeyer05
Copy link
Author

If I were to create a PR with a new configuration option for Recoverable that took a time period and didn't reset the token within that window, is that something that the maintainers of the project would be interested in incorporating?

@timdiggins
Copy link

I think this is a great idea (this is a loop I'm aware of). Devise should still send an email though - it should say something like - please see the earlier email for the reset password link or wait 10 minutes and try again.

@jbeyer05
Copy link
Author

Devise should still send an email though - it should say something like - please see the earlier email for the reset password link or wait 10 minutes and try again.

I think the email can be exactly the same as the initial email. It just doesn't change the password reset token, so that the links in either the first, second or tenth email still works. After some period of time, a new request for a password reset token should actually provide a new token. But I don't see a security issue with reusing the token a few times to prevent this infinite loop.

@faelsoto
Copy link

Here's our solution, it will not trigger the same email but it can be easily implemented through Rails.cache.

Devise::Models::Recoverable.module_eval do
  alias_method :original_send_reset_password_instructions_notification, :send_reset_password_instructions_notification
  alias_method :original_set_reset_password_token, :set_reset_password_token

  def set_reset_password_token
    return if reset_password_sent_at.present? && !reset_password_sent_at.before?(3.minutes.ago)

    original_set_reset_password_token
  end

  protected

  def send_reset_password_instructions_notification(*args)
    return if args.first.blank?

    original_send_reset_password_instructions_notification(*args)
  end
end

@rainerborene
Copy link

rainerborene commented Dec 7, 2024

I've just implemented a workaround for this issue simply by using Rails.cache. This code makes sure that all emails sent will reuse the same token. If you don't want that behaviour, you can also skip the email delivery within an hour by overriding the create method on PasswordsController as mentioned here.

module User::Recoverable
  protected
    def cache_reset_password_token(expires_in: Devise.reset_password_within, &block)
      Rails.cache.fetch(["reset_token", reset_password_token], expires_in:, &block)
    end

    def set_reset_password_token
      if reset_password_token?
        return cache_reset_password_token { super }
      end

      super.tap do |token|
        cache_reset_password_token { token }
      end
    end
end

@timdiggins
Copy link

timdiggins commented Dec 19, 2024

Devise should still send an email though - it should say something like - please see the earlier email for the reset password link or wait 10 minutes and try again.

I think the email can be exactly the same as the initial email. It just doesn't change the password reset token, so that the links in either the first, second or tenth email still works. After some period of time, a new request for a password reset token should actually provide a new token. But I don't see a security issue with reusing the token a few times to prevent this infinite loop.

Devise (and in fact no one) has access to this token because it has been hashed and stored as a hash. I think it's worth pointing out this was I believe a deliberate security policy on behalf of Devise to prevent someone from being able to reset someone else's password in the case there is compromise to database access (the same reason that the password is hashed and not stored in the clear).

The workarounds given have security implications because they end up putting the reset token into more places (i.e. the cache).

A solid fix would involve either:

  • a different email is sent if there is a reset token active (this is a relatively easy fix) - without generating a new reset token. This has minimal (or no) security implications and doesn't seem too complicated to do, but it's not as friendly as the second fix
  • a mechanism (config option or maybe extension to Devise? or just a local patch) to ensure the raw (unhashed) reset token is stored to the database, rather than the hash, then use this for subsequent emails (and the token validity is perhaps extended by updated reset_password_sent_at). This could well be a adequate security/usability compromise for many people if the token validity period (reset_password_within) is short enough (because of this it's quite a different security exposure than the unhashed password being stored).

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

4 participants