This is more a usability issue than a bug: When the user tries to change both his email address and his password at the same time, the confirmation link will be invalid, as the hash code still corresponds to the old password.

The easiest way to prevent this is probably to throw a form error if the new password field is not blank.

Comments

ropic’s picture

confirmed +1

ropic’s picture

line 420 must be updated:
$pass = isset($edit['pass']) ? md5($edit['pass']) : $user->pass;
to
$pass = isset($edit['pass']) ? $edit['pass'] : $user->pass;

$edit['pass'] is encrypted

jaydub’s picture

Assigned: Unassigned » jaydub
klidifia’s picture

Issue summary: View changes
StatusFileSize
new671 bytes

Also causes an issue if someone changes their email address when they've logged in for the first time (where they also have to set their password).

klidifia’s picture

StatusFileSize
new824 bytes

For current stable

klidifia’s picture

StatusFileSize
new664 bytes
greggles’s picture

Version: 7.x-1.0 » 7.x-1.x-dev
Priority: Minor » Normal

This change looks good. It seems like at least a "normal" priority.

greggles’s picture

Status: Active » Needs review

Oops, better status as well.

emsearcy’s picture

I think patch #6 may still have an issue with systems that don't use the password supplied by the user, such as simple_ldap, which writes the user-provided password to LDAP but saves a random password into the user database. In any case, I'd suggest that the password isn't needed at all, but rather the user ID should be included. In fact, all the parameters in the URL (aside from the hash itself) should be included into the hash, otherwise a user can change the timestamp arbitrarily to use old links. I've just created a public issue about this (#2463481) which should also fix this issue by no longer including the password in the hash (similar to LoginToboggan's email confirmation links).

alexandrdnlv’s picture

A little bit updated patch for updated function.

greggles’s picture

Status: Needs review » Closed (duplicate)

I agree with emsearcy in #9 about just removing the password from the hash. I think this would be fixed by #2463481: Expirations can be bypassed and username enumeration, so closing it out as duplicate of that.