Bring back confirmation on password reset

[vincentwoo]

(see: #1226)

Devise's cited reason for removing this functionality is

Do not confirm account after password reset

In previous Devise versions, resetting the password automatically confirmed user accounts. This worked fine in previous Devise versions which confirmed the e-mail just on sign up, so the e-mail both confirmation and password reset tokens would be sent to were guaranteed to be the same. With the addition of reconfirmable, this setup change and Devise will no longer confirm the account after password reset.

Thanks to Andri Möll for reporting this issue and working with us on a fix.

It really sucks that this functionality was nixed. There's no security risk in the most obvious, 90% use case - a new user forgets they needed to confirm and requests a reset instead. In this case, since there was no reconfirmation possible, there's no security risk in confirming upon password reset.

It seems like it would be easy enough for the reset module to check if the user was unconfirmed (with no confirmed_at time present) and confirm them only in that case.

[ulissesalmeida]

@vincentwoo Thank you for your consideration. I think both on mentioned issue and in this blog post explain the reasons we decided to remove the feature. Is very important Devise be secure by default. And this feature also is kind of a side effect and may not be wanted for everyone.

I think you still can reproduce the feature with your app code. I think we don't have a plan to bring it back. But if came back, will be optional feature.

[vincentwoo]

I think you should want Devise to be (1) secure by default (2) and good by default. Since confirming a fresh email on reset is 100% secure, and is obviously good (no site owner would ever NOT want this feature, that's WHY it was originally the default) you should almost certainly bring it back, maybe with an opt-out.

[ulissesalmeida]

I'm not sure about it. I'll summon @josevalim in this matter. I think he have more context about the past deicision and the feature removal decision.

[josevalim]

It is a security risk because you can get any email without confirming it. For example, I would be able to get "[email protected]". I won't post the exact steps for attacking on this vector but I have confirmed the security reports and any way we introduce this change would be a security bug.

[vincentwoo]

I find that to be a very uncompelling line - you're going to have to at least broadly sketch out the issue. Why handwave towards security through obscurity here? I don't understand why allowing reset to confirm for completely fresh email addresses (not changed addresses) could be a vulnerability. Both confirm and reset require being able to receive the email at "[email protected]", as you put it.

I'm not saying that there isn't one, but that you've not given me confidence that the issue has been thought through.

[chulkilee]

It sounds like 1) depending on how devise model is changed devise may send password reset email and confirmation email to different addresses and 2) due to this reason password reset email must not confirm the email. Is it correct? @josevalim

[josevalim]

I find that to be a very uncompelling line - you're going to have to at least broadly sketch out the issue.

No, I don't have to. :) If you don't trust us to be giving you a correct answer when it comes to a security issue, then you likely shouldn't be using Devise.

Why handwave towards security through obscurity here?

The recommendation from security advisors has always been to discuss exploits at a higher level to not compromise deployed user bases. Even though some time has passed, I don't see a reason to provide much more information than the one we gave in the original announcement.

@chulkilee yes, it is.

[vincentwoo]

Trust is earned - how can you inspire people to trust you if your decisions seem arbitrary and without consideration?

I still don't understand how you can trick devise into sending confirmations and resets to different email addresses for fresh users. Furthermore, even if you can trick Devise into doing so, it seems like it should be pretty easy to just... fix... that.

[josevalim]

Trust is earned - how can you inspire people to trust you if your decisions seem arbitrary and without consideration?

That's a very good question. We hope we have established some degree of trust by making the project open source, so everyone can study the source code and evaluate it according to their criteria.

We are also being very careful to not break the trust others have put on us by exposing too much information about previous security issues.

Furthermore, even if you can trick Devise into doing so, it seems like it should be pretty easy to just... fix... that.

If it was "pretty easy to just fix that", we would have done so.

Unfortunately fixing the problem would require us to track exactly which e-mails are having their password recovered and which e-mails are pending confirmation. This would need to be done by either adding new columns to the database or by changing how tokens are managed so they can include some metadata in them. Any of those options would require considerable changes to the underlying mechanisms and potentially require migration efforts for existing codebases.

[vincentwoo]

That's a great point, but what you describe is work that should definitely be done to improve both the security and usability of Devise. This work can be done without impacting previous installations of Devise. Not talking about previous issues shouldn't prevent you from discussing ways to make your product better. This problem, along with previous usability problems reported suggest that a serious reconsideration of how Devise handles tokens is probably overdue. Why put it off?

[YesThatAllen]

I'm late to the game here, but given that we confirm an address by:

 `sending it a link to be clicked.`

and we allow a password reset by:

 `sending it a link to be clicked.`

how is @josevalim 's statement here:

It is a security risk because you can get any email without confirming it.

Different from this counter point:

"It is a security risk because you can get any email without resetting its password. "

What @ulissesalmeida says is true.

I think you still can reproduce the feature with your app code.

I'm posting here because this discussion doesn't really answer why it's important to make someone go back and click a confirmation link. What is a user supposed to do if that confirmation link has expired?

(For purposes of discussion, let's assume that an email address can't be changed between creation & password reset, unless it's been confirmed.)

[nasa42]

I'm not 100% sure here but I think the security vulnerability he is talking about in #4006 (comment) is - requesting password reset at [email protected] but maliciously submitting the email [email protected] when resetting password and getting that confirmed instead.

[darrenterhune]

I've been looking thru devise source for a few days now, trying to solve somewhat the same problem, thinking about security first. From what I gather, you could confirm the user upon password edit automatically using a conditional to check that the unconfirmed_email is nil, thus the user never updated their email address to a fake email... or typo email. So the user resource that is being requested for a password update already has a confirmed email address, which then could be auto confirmed.

@josevalim would something like this solve the security vulnerability here?

Initially set unconfirmed_email to the email address of the user signing up, then set it to nil when they confirm as per the normal current flow of devise. Then one could simply:

confirm if unconfirmed_email.nil?

I could be missing something though.

[jrmyward]

If you have a use case where you desire to confirm accounts with a password reset, you can easily extend the default functionality by creating your own PasswordController and extending the method like:

class PasswordsController < Devise::PasswordsController
  def update
    super do |resource|
      resource.confirm if invite_confirmable?(resource)
    end
  end

  private

  def invite_confirmable?(resource)
    # You can add whatever business logic you like here to decide if a confirmation is appropriate. 
    return false if resource.errors.present? || resource.confirmed_at

    true
  end
end