Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.Problem
Since the password field is not set to required for editing accounts (just registering new ones), when a reset email is sent for a new user, the user registration page is now treated like an edit page, with the $pass_reset variable set. (line 1054, user.module)
If a user, registers for the site, and clicks the one-time reset/validation form, they will be brought to the user edit page. This allows them to save their details without needing to enter a password.
To recreate:
1) turn email validation on
2) create new account
3) check e-mail and use the reset link provided
4) login to the site
5) click save to the user edit form without setting a password
Now the user will have to reset their password again in order to change it (since the 'current password' is unknown to the user)
Proposed resolution
add these lines after the $pass_reset var check (approx 1054-1059, user.module):
} else {
//we need to make sure the password is required if we're resetting it.
$form['account']['pass']['#required'] = TRUE;
}This should require the password to be entered on validation emails, as well as password resets.
Remaining tasks
Review of the code snippet, creation of a patch, testing, security audit?
| Comment | File | Size | Author |
|---|---|---|---|
| #17 | password_field_required_1628120_17.patch | 2.07 KB | MeenakshiG |
| #13 | password_field_required-1628120-13.patch | 1.98 KB | arunkumark |
| #11 | password_field_required-1628120-11.patch | 1.98 KB | arunkumark |
| #10 | password_field_required-1628120-10-potential-test.patch | 1.22 KB | LoMo |
| #4 | before.png | 31.48 KB | andymartha |
Comments
Comment #1
catchI don't think this is major, the worst that can happen is you need to reset your password again, seems worth fixing though.
Comment #2
Nephele CreditAttribution: Nephele commentedOK.... This was a duplicate of a 6.x issue, #75924: Include fields for resetting password on the one-time password reset page. Except that has evolved into a complete rework of the password-reset feature: it's been changed from a bug report to a feature request, and it's been declared too complex to backport to 7.x.
This is an annoyance, especially because it's not such a simple matter to reset your password again: you have to go through a page with a confusing message (#1872404: One-time password link has broken message if the user is logged in), log out, and then use the new reset link. This issue is causing other bugs, and gives new users a pretty poor first impression of drupal.
If I understand the backport policy, the desired course of action here is to implement a minimal fix in d7/d8 without waiting for the refactoring/full feature request. Therefore, I've created both a d8 and a d7 patch. I've tested that the patch works in d7, both for new accounts and password reset requests, plus confirmed that it's not required for other uses of the user profile form. Is there anything else holding this up?
Comment #3
David_Rothstein CreditAttribution: David_Rothstein commentedAdding back the tag since this seems to be intended for backport.
I think this makes sense overall, for D8 if nothing else (stuff like this is always scary for backport given how often the user account form gets heavily customized on particular sites).
However, I've heard of people who (at least for sensitive admin accounts) never use passwords to log in at all... for security reasons they only log in via the one-time links. This is going to be a bit annoying for them, but I suppose they can just navigate away and come back to the edit screen if they really want to change something else about their account without having to type a new password.
Comment #4
andymartha CreditAttribution: andymartha commentedI can confirm that this patch changes the form field of password to "required" on the user edit screen without any further alteration, if "the desired course of action here is to implement a minimal fix in d7/d8 without waiting for the refactoring/full feature request.", and the normal logging in/out seems unaffected.
After a fresh installation of Drupal 8.x-dev from March 6th, 2013, I was able to reproduce the issue in the issue summary that users can reset their password from email but then not enter a new password and save the user edit form. See screenshots.
After applying patch password_field_required-1628120-2.patch in #2 by Nephele, I was unable to save the user edit form from email without entering a new password (thus breaking this possibly erroneous/insecure cycle). The input field also has the html5 required property. See screenshots.
Comment #5
tim.plunkettThis should have automated tests.
Comment #9
LoMo CreditAttribution: LoMo as a volunteer commentedLooks like this issue has been stalled out for four years (!) due to lack of test coverage. I've found that there is an existing UserRegistrationTest which checks much of this functionality. So it appears that all that is needed are a few changes to verify that the patch makes the password field required, etc.
I'll work on getting this sorted...
Comment #10
LoMo CreditAttribution: LoMo as a volunteer commentedWell, damn... the patch no longer applies and it looks like the architecture has changed dramatically enough that this needs a serious re-roll:
error: core/modules/user/lib/Drupal/user/AccountFormController.php: No such file or directoryI'm not sure I know where the logic is now for the changes that the original patch made, but it does seem that the issue still exists in drupal-8.4.x-dev, so maybe someone can manage a re-roll of this.
I have written a test which fails (as expected, without the fix), but may still fail after a correct fix. It's possible it needs some tweaks to the assertions, etc. Anyway, I've uploaded my test patch, for what it's worth.
Comment #11
arunkumarkHi All,
I have re-rolled this patch with combing test cases.
The Drupal 8.4.x version has created so created one patch for 8.4.x(d84x-password_field_required-1628120-11.patch ) version.
Comment #13
arunkumarkComment #17
MeenakshiG CreditAttribution: MeenakshiG at OpenSense Labs commentedreroll
Comment #18
MeenakshiG CreditAttribution: MeenakshiG at OpenSense Labs commentedComment #21
jofitz CreditAttribution: jofitz at ComputerMinds commentedNo longer requires re-roll - removing tag.
Ignore patch in #17, work to correct test failures should be based off the patch in #13.
Comment #30
quietone CreditAttribution: quietone at PreviousNext commented@japerry, thank for bring this issue up.
This was a bugsmash daily target a few days ago. It was discussed by lendude, larowlan and myself. lendude pointed out that "changing this would really mess with our maintainer login policy where our maintainer users are not allowed to have a password at all and you can only login via drush uli". I checked what OWASP had to say on the subject and it does not disagree with this practice. In the end we all agree that this is working as designed.
Later, I tested the link and found that the flow was a bit confusing and that matches what is mentioned in #4. Then, I looked for duplicate issues and found #75924: Include fields for resetting password on the one-time password reset page. That issue has more discussion, and has had a usability review. It is also a feature request while this is a bug. I checked the definition of bug and this does not fit. So, I am changing to a feature request like the other issue.
Since these are so similar, I am closing this as a duplicate and moving credit.
Comment #31
brad.bulger CreditAttribution: brad.bulger commentedClosed but never solved, very nice. Maybe it doesn't fit "the definition of bug" (lol) but it sure seems like a problem to people who run into it. Another forever-patch.