Problem/Motivation

Users without email address cannot use reset password when not logged in.

If an anonymous user visits /user/password and enters a username of the account without email address, the form submit throws an error:

TypeError: Drupal\Core\Mail\Plugin\Mail\PhpMail::doMail(): Argument #1 ($to) must be of type string, null given, called in /core/lib/Drupal/Core/Mail/Plugin/Mail/PhpMail.php on line 136 in Drupal\Core\Mail\Plugin\Mail\PhpMail->doMail() (line 168 of /core/lib/Drupal/Core/Mail/Plugin/Mail/PhpMail.php).

Drupal\Core\Mail\Plugin\Mail\PhpMail->mail(Array) (Line: 308)
Drupal\Core\Mail\MailManager->doMail('user', 'password_reset', NULL, 'en', Array, 'xxx', 1) (Line: 181)
Drupal\Core\Mail\MailManager->Drupal\Core\Mail\{closure}() (Line: 593)
Drupal\Core\Render\Renderer->executeInRenderContext(Object, Object) (Line: 180)
Drupal\Core\Mail\MailManager->mail('user', 'password_reset', NULL, 'en', Array, 'xxx') (Line: 952)
_user_mail_notify('password_reset', Object) (Line: 199)
Drupal\user\Form\UserPasswordForm->submitForm(Array, Object)
call_user_func_array(Array, Array) (Line: 105)
Drupal\Core\Form\FormSubmitter->executeSubmitHandlers(Array, Object) (Line: 43)
Drupal\Core\Form\FormSubmitter->doSubmitForm(Array, Object) (Line: 589)
Drupal\Core\Form\FormBuilder->processForm('user_pass', Array, Object) (Line: 321)
Drupal\Core\Form\FormBuilder->buildForm(Object, Object) (Line: 73)
Drupal\Core\Controller\FormController->getContentResult(Object, Object)
call_user_func_array(Array, Array) (Line: 123)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}() (Line: 593)
Drupal\Core\Render\Renderer->executeInRenderContext(Object, Object) (Line: 121)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->wrapControllerExecutionInRenderContext(Array, Array) (Line: 97)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}() (Line: 183)
Symfony\Component\HttpKernel\HttpKernel->handleRaw(Object, 1) (Line: 76)
Symfony\Component\HttpKernel\HttpKernel->handle(Object, 1, 1) (Line: 53)
Drupal\Core\StackMiddleware\Session->handle(Object, 1, 1) (Line: 48)
Drupal\Core\StackMiddleware\KernelPreHandle->handle(Object, 1, 1) (Line: 28)
Drupal\Core\StackMiddleware\ContentLength->handle(Object, 1, 1) (Line: 32)
Drupal\big_pipe\StackMiddleware\ContentLength->handle(Object, 1, 1) (Line: 116)
Drupal\page_cache\StackMiddleware\PageCache->pass(Object, 1, 1) (Line: 90)
Drupal\page_cache\StackMiddleware\PageCache->handle(Object, 1, 1) (Line: 50)
Drupal\ban\BanMiddleware->handle(Object, 1, 1) (Line: 48)
Drupal\Core\StackMiddleware\ReverseProxyMiddleware->handle(Object, 1, 1) (Line: 51)
Drupal\Core\StackMiddleware\NegotiationMiddleware->handle(Object, 1, 1) (Line: 36)
Drupal\Core\StackMiddleware\AjaxPageState->handle(Object, 1, 1) (Line: 51)
Drupal\Core\StackMiddleware\StackedHttpKernel->handle(Object, 1, 1) (Line: 709)
Drupal\Core\DrupalKernel->handle(Object) (Line: 19)

Steps to reproduce

Create a user without email address
As an anonymous user, try to submit a UserPasswordForm on /user/password for that username

Proposed resolution

Fix the condition, so that there is no error and an existing generic message
If %identifier is a valid account, an email will be sent with instructions to reset your password. is displayed.

Remaining tasks

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#21 After.png18.52 KBjatingupta40
#21 Before.png96.74 KBjatingupta40
#21 TestuserProfile.png38.6 KBjatingupta40
#12 after-3481894.png215.94 KBasawari
#12 before-3481894.png216.12 KBasawari
#11 Screenshot 2024-11-07 at 3.54.40 PM.png263.21 KBvinai_katiyar
#11 Screenshot 2024-11-07 at 4.22.04 PM.png117.74 KBvinai_katiyar
#4 resetpassword.png73.11 KBgaurav gupta

Issue fork drupal-3481894

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

poker10 created an issue. See original summary.

poker10’s picture

poker10 commented
Issue summary: View changes
gaurav gupta’s picture

gaurav gupta commented
Assigned: Unassigned » gaurav gupta
gaurav gupta’s picture

gaurav gupta commented
StatusFileSize
new resetpassword.png73.11 KB

Hello @poker10
i have gone through the issue and were able to reproduce it
but i think it would be better if we "add a form validation message, to check existence of email for a valid username" instead of showing the existing generic message.
My solution is to add a validation UserPasswordForm.php 

 if (empty($account->getEmail())) {
        $form_state->setErrorByName('name', $this->t('The account does not have a valid email address.'));
        return;
      }

I think it will be a good approach since it is breaking the drupal site instead of any security issues of what we see here
https://www.drupal.org/project/drupal/issues/1521996

zartab farooquee’s picture

zartab farooquee commented

Prevent Users Without Email Addresses from Using the Password Reset Form
You can implement a check in the password reset form handler (UserPasswordForm) to ensure that only users with a valid email address can submit the form.
Modify the submitForm() function in a custom module or use a hook, like hook_form_alter() or hook_form_user_pass_alter(), to check for the presence of an email address associated with the username.

function mymodule_form_user_pass_alter(&$form, \Drupal\Core\Form\FormStateInterface $form_state, $form_id) {
    $form['#validate'][] = 'mymodule_user_pass_validate';
}

function mymodule_user_pass_validate($form, \Drupal\Core\Form\FormStateInterface $form_state) {
    $username = $form_state->getValue('name');
    $account = user_load_by_name($username);

    if ($account && !$account->getEmail()) {
        $form_state->setErrorByName('name', t('The account does not have an email address and cannot reset the password.'));
    }
}
poker10’s picture

poker10 commented

@gaurav gupta I was thinking about keeping the UI message the same, even if the username does not have an email associated, exactly because of this information disclosure issue: #1521996: Password reset form reveals whether an email or username is in use. If we change the error message for this case, it will be possible to use this for username enumeration (especially on sites where email addresses are not required/used). I do not think that is a good idea.

I suggest to keep the behavior the same as now, so everytime the form is submitted, the same message is printed and only add another condition/or change existing conditions to handle the case a username does not have an email address.

@zartab farooquee We cannot block the form submit, since this issue is about anonymous users doing password resets. So we will know if the entered username has email address only after the form is submitted.

The similar issue is here, which is for logged in users: #3383647: Accounts without an email address will display a warning message when resetting their password on the edit page

Thanks!

cilefen’s picture

cilefen commented
Related issues: +#3383647: Accounts without an email address will display a warning message when resetting their password on the edit page

gaurav gupta opened merge request !10051

gaurav gupta’s picture

gaurav gupta commented
Assigned: gaurav gupta » Unassigned
Status: Active » Needs review
vinai_katiyar’s picture

vinai_katiyar commented
Assigned: Unassigned » vinai_katiyar
vinai_katiyar’s picture

vinai_katiyar commented
Assigned: vinai_katiyar » Unassigned
StatusFileSize
new Screenshot 2024-11-07 at 4.22.04 PM.png117.74 KB
new Screenshot 2024-11-07 at 3.54.40 PM.png263.21 KB

checked the MR - https://git.drupalcode.org/project/drupal/-/merge_requests/10051 and it's worked for me.

asawari’s picture

asawari commented
StatusFileSize
new before-3481894.png216.12 KB
new after-3481894.png215.94 KB

Test steps-
- Create a user without email address
- Go to reset passwords page
- submit the form
- observe the error

Test status - Fail
Patch applied successfully.
Issue still persists.
Please refer the screenshots in the attachments.
before-3481894.png
after-3481894.png

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work
Issue tags: +Needs issue summary update

MR does not match the proposed solution.

That set I don't think just putting an empty check is correct, where's the backtrace to make sure we aren't hiding a larger issue.

poker10’s picture

poker10 commented
Issue summary: View changes
Issue tags: +Needs tests

I am not sure what we need to add to the issue summary, but agree that the MR should be updated - at least we need another elseif in the existing condition, so that we do not end up with Password reset form was submitted with an unknown or inactive account: %name. when actually the account exists, it just does not have an email set. So I think we should move the check to a new elseif with a new separate watchdog message, so that is clear, that a form was submitted for an account without email address. The printed message seems to be correct then, and it is as proposed in IS.

Probably we should also add a simple test for this behavior as well? Something like - create user without email, log out and try to send the password for that username. I do not think we are hiding a larger issue here, simply the PhpMail does not expect to get a NULL email (which is a case, if a user account does not have an email set).

Thanks!

gaurav gupta’s picture

gaurav gupta commented
Status: Needs work » Needs review

Added the tests and provided the suggested changes.
Please review.

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

Left comments on MR.

poker10’s picture

poker10 commented

I think there is nothing special required for the use-case mentioned in the IS, so it seems like that if we fix this this directly in the _user_mail_notify() (see #3518058: _user_mail_notify() doesn't handle accounts without an email address), no other changes will be needed here.

jan kellermann made their first commit to this issue’s fork.

jan kellermann’s picture

jan kellermann commented

Read patch incorrectly. Sorry, my mistake. Comment deleted.

jatingupta40 made their first commit to this issue’s fork.

jatingupta40’s picture

jatingupta40 commented
StatusFileSize
new TestuserProfile.png38.6 KB
new Before.png96.74 KB
new After.png18.52 KB

Hello @poker10,
I have updated the code as per the requirement. Now if the user having the email field empty and tries to do 'reset password', the proposed output - If %identifier is a valid account, an email will be sent with instructions to reset your password. is displayed.

User Profile -
User profile

Before -
before

After -
After

jatingupta40’s picture

jatingupta40 commented
Status: Needs work » Needs review
divyansh.gupta’s picture

divyansh.gupta commented
Status: Needs review » Reviewed & tested by the community

Tested the latest changes and checked that the fix handles the issue as expected. There are no errors thrown, and the behavior aligns with the discussion in the issue summary. The test coverage looks good too. Thus marking this as RTBC.

dcam’s picture

dcam commented
Status: Reviewed & tested by the community » Needs work

The inherent bug of _user_mail_notify() sending emails to accounts without addresses was fixed by #3518058: _user_mail_notify() doesn't handle accounts without an email address. I'm setting the status to Needs Work so someone can determine whether additional action needs to be taken for the password reset form specifically. Note that #3518058 did not result in any user-facing messages being displayed with regard to the success or failure of emails being sent. This issue may want to take that further.

Version: 11.x-dev » main

Drupal core is now using the main branch as the primary development branch. New developments and disruptive changes should now be targeted to the main branch.

Read more in the announcement.