Make WordPress Core

Opened 8 years ago

Closed 2 years ago

Last modified 2 years ago

#35500 closed defect (bug) (fixed)

Login page: user can change password on " " but can't log in with new password.

Reported by: antonrinas's profile antonrinas Owned by: hellofromtonya's profile hellofromTonya
Milestone: 6.0 Priority: lowest
Severity: normal Version:
Component: Login and Registration Keywords: has-patch needs-testing
Focuses: Cc:

Description

There is a user:
login: admin
password: admin
role: administrator
STEPS TO REPRODUCE
Click "Lost your password?" on Login page and change it on " " (space character).
Try to login with new password.
EXPECTED RESULT: logging in.
ACTUAL RESULT: "ERROR: The password field is empty."

Attachments (4)

35500.diff (796 bytes) - added by hellofromTonya 3 years ago.
Trims and checks for empty pass1. If present, sets error to alert user and allows user to try again.
35500.1.diff (822 bytes) - added by hellofromTonya 3 years ago.
Checks for a password of one space or all empty spaces. Does not check on first load.
35500-testing-no-js.gif (1.3 MB) - added by hellofromTonya 3 years ago.
Testing of 35500.1.diff patch. Works as expected.
35500.2.diff (829 bytes) - added by costdev 2 years ago.
Patch refreshed after testing by @peterwilsoncc. Compares $_POST['pass1'] to trim( $_POST['pass2'] ).

Download all attachments as: .zip

Change History (21)

#1 @johnbillion
8 years ago

  • Keywords needs-patch added
  • Priority changed from normal to lowest
  • Version 4.4.1 deleted

#2 @swissspidy
8 years ago

Simply not using trim() on the password in wp_authenticate() results in a incorrect_password error instead of empty_password. So that's only half of the deal.

I consider using trim() very helpful, e.g. when users accidentally hit space when typing in their password. Since someone rarely if ever changes their password to , I think the downsides outweigh the benefits.

#3 @voldemortensen
8 years ago

  • Keywords close added

I agree with @swissspidy here. I don't think its a common use case to set a password as a space (or even multiples spaces). And I don't think its a use case that should be supported. I don't think we should go out of our way to support passwords that are that insecure.

#4 @hellofromTonya
3 years ago

  • Keywords close removed
  • Owner set to hellofromTonya
  • Status changed from new to assigned

Hello @antonrinas,

Welcome to WordPress Trac! Thank you for reporting this issue.

For JavaScript enabled browsers, this issue was fixed in ticket #42766 with changeset [49118]. This change did the following:

  • the password field is trimmed first
  • if empty, an empty class is added to the field and the Save Password button is not displayed
  • requires JavaScript

What if the browser has JavaScript disabled? The problem still exists.

A possible solution:

  • Trim and then check if pass1 is empty in wp-login.php
  • if yes, generate an error message

This step would return the user back to the password reset page with a reason why their password was not accepted.

Testing now and will submit a patch for consideration.

@hellofromTonya
3 years ago

Trims and checks for empty pass1. If present, sets error to alert user and allows user to try again.

@hellofromTonya
3 years ago

Checks for a password of one space or all empty spaces. Does not check on first load.

@hellofromTonya
3 years ago

Testing of 35500.1.diff patch. Works as expected.

#5 @hellofromTonya
3 years ago

  • Keywords has-patch needs-testing has-testing-info added; needs-patch removed
  • Milestone set to 5.9

Testing info

Dependencies

Need a way to capture the password reset email when testing locally. One way is to install and activate the Email Log plugin. This plugin captures email notifications, including the password reset email.

How to get a password reset link and display the Password Reset screen:

  1. Request to reset your password by (a) going to the login page, (b) clicking on "Lost your password?" link, and then adding your username or email.
  2. Get the password reset link from the email by:
    • Open the database. If using Local, click on the Database tab and then click on Open Adminer. It should then open in your browser.
    • Open the wp_email_log database table.
    • Click on Select data.
    • Edit the last record in the table, which opens it up.
    • Copy the link in the message field (likely will need to scroll down to view and then copy it).
  3. Open a different browser and turn off/disable JavaScript.
  4. Copy the reset password link into that browser.

How to reproduce the problem locally

  1. Go to the Password Reset screen (see above).
  2. Type one or more spaces into both password fields.
  3. Click Save Password button. => Notice it let you.
  4. Try to log in with that "space-only" password. => Notice it fails.

How to test the patch

  1. Request another password reset and go to the Password Reset screen (see above).
  2. Type one or more spaces into both password fields.
  3. Click Save Password button.

This time notice an error message is displayed and the Password Reset screen reloads.

This ticket was mentioned in Slack in #core-test by hellofromtonya. View the logs.


3 years ago

#7 @henry.wright
3 years ago

Related is #53339. trim() is used inconsistently when setting a password.

#8 @hellofromTonya
3 years ago

  • Milestone changed from 5.9 to 6.0

No activity for 5 months. Punting to 6.0.

This ticket was mentioned in Slack in #core by costdev. View the logs.


2 years ago

#10 @costdev
2 years ago

  • Keywords commit added; needs-testing removed

Test Report

Environment

  • Server: nginx - Local by Flywheel
  • WordPress: 5.9.2
  • Browser: Chrome 99.0.4844.84
  • OS: Windows 10
  • Theme: Twenty Twenty-Two
  • Plugins:
    • Email Log 2.4.8

Steps

  1. Go to the Password Reset screen.
  2. Type one or more spaces into both password fields.
  3. Click Save Password button. => Notice it let you. ✅
  4. Try to log in with that "space-only" password. => Notice it fails. ✅
  5. Apply patch 35500.1.diff.
  6. Request another password reset and go to the Password Reset screen.
  7. Type one or more spaces into both password fields.
  8. Click Save Password button.
  9. This time notice an error message is displayed and the Password Reset screen reloads. ✅

Results

  1. Issue reproduced.
  2. Patch 35500.1.diff resolves the issue.

  • Removing needs-testing.
  • Adding commit for consideration.

#11 @peterwilsoncc
2 years ago

  • Keywords needs-refresh needs-testing added; has-testing-info commit removed

Doing some branch testing with JavaScript disabled.

Entering space (please note the leading space) in both the password and confirmation fields results in the error message "Error: The passwords do not match."

This is because the password field, pass1, is trimmed but the confirmation field, pass2, is not. If WordPress must preprocess password fields, then the confirmation field will need to be preprocessed too.

@costdev
2 years ago

Patch refreshed after testing by @peterwilsoncc. Compares $_POST['pass1'] to trim( $_POST['pass2'] ).

#12 @costdev
2 years ago

  • Keywords needs-refresh removed

Just added patch 35500.2.diff, which tests well for me using " space" (note the leading space) in both the password and confirmation field.

Last edited 2 years ago by costdev (previous) (diff)

#13 follow-up: @peterwilsoncc
2 years ago

  • Resolution set to fixed
  • Status changed from assigned to closed

In 53067:

Login, Registration: Prevent password reset to whitespace alone.

Prevent users from using the password reset form to set their password to whitespace alone (tabs, spaces). This matches the processing used during the authentication flow, ensuring users do not inadvertently get locked out of their account.

Props antonrinas, swissspidy, voldemortensen, hellofromTonya, henry.wright, costdev.
Fixes #35500.

#14 @peterwilsoncc
2 years ago

In 53068:

Login, Registration: Fix coding standards errors in [53067].

See #35500.

#15 in reply to: ↑ 13 ; follow-up: @azouamauriac
2 years ago

Replying to peterwilsoncc:

In 53067:

I wonder to know if we can just put this both "if" statement together... Any objection?

#16 in reply to: ↑ 15 ; follow-up: @peterwilsoncc
2 years ago

Replying to azouamauriac:

I wonder to know if we can just put this both "if" statement together... Any objection?

The first if statement makes changes to the password variable that are used in the second statement. I considered asking for them to be combined but decided the code would be clearer keeping them separate.

#17 in reply to: ↑ 16 @azouamauriac
2 years ago

Replying to peterwilsoncc:

Replying to azouamauriac:

I wonder to know if we can just put this both "if" statement together... Any objection?

The first if statement makes changes to the password variable that are used in the second statement. I considered asking for them to be combined but decided the code would be clearer keeping them separate.

I'm still thinking these statements can be merged without make the code unreadable. Anyway let's move up.

thank you.

Note: See TracTickets for help on using tickets.