WordPress.org

Make WordPress Core

Opened 4 years ago

Last modified 3 months ago

#22367 reviewing defect (bug)

Usernames with "@" char are assumed email addresses, causing incorrect look-up in several places

Reported by: johnjamesjacoby Owned by: johnjamesjacoby
Milestone: Awaiting Review Priority: normal
Severity: normal Version:
Component: Users Keywords: dev-feedback
Focuses: Cc:

Description

Problem

Usernames containing the "@" character are mistakenly assumed to be email addresses when:

  • wp-login.php - Resetting passwords
  • /wp-admin/user-new.php - Adding an existing user to a site, in multisite
  • /wp-includes/user.php - Searching for a user

Duplicate

  • Create a user with the login "@testing"
  • Verify the account, etc...

Bug in Search

  • Visit: wp-admin/network/users.php - attempt to search for: "@testing"
  • Result: no users found
  • What should happen: find the user

Bug in Add New

  • Visit: wp-admin/wp-admin/user-new.php - attempt to add: "@testing"
  • Result: no users found
  • What should happen: add the user

Bug in Reset Password

  • Visit: wp-login.php - attempt to reset password for: "@testing"
  • Result: retrieve_password() accidentally succeeds, because strpos() check returns 0, which is the correct position of the "@" character. If the username was "testing@" this test would fail

Solution

The attached patch fixes these bugs by using is_email() instead of an strpos() for an @ character.

Attachments (4)

22367.patch (1.4 KB) - added by johnjamesjacoby 4 years ago.
22367.2.patch (1.9 KB) - added by johnbillion 10 months ago.
wordpress-trunk-fix-login.patch (972 bytes) - added by gravitylover 7 months ago.
Allow users to reset password who have emails as usernames
22367.3.patch (2.1 KB) - added by gravitylover 7 months ago.
patch incorporating modifications by JJJ and johnbillion

Download all attachments as: .zip

Change History (27)

#1 @nacin
4 years ago

  • Milestone changed from 3.5 to Awaiting Review

#2 @johnjamesjacoby
4 years ago

Reminder that as it stands now, users with "@" in their logins:

  • Cannot reset their passwords (unless @ is the first char).
  • Cannot be added to existing sites.
  • Cannot be found in network user searches.

Whether or not user_login should be allowed to have "@" at all is debatable.

Sad to see this get punted to Awaiting Review. It's an easy win, and fixes several similar, existing, user-facing issues with only 3 lines changed.

Last edited 4 years ago by johnjamesjacoby (previous) (diff)

#3 @scribu
4 years ago

If it's not a regression from 3.4, it's not that urgent.

#4 follow-up: @miqrogroove
4 years ago

Some of the above descriptions need to be qualified with the option of simply using e-mail addresses instead of usernames. It sounds like the features are broken, but not unusable.

I encourage setting a 3.6 milestone.

#5 @SergeyBiryukov
4 years ago

  • Keywords 3.6-early added

#6 in reply to: ↑ 4 @johnjamesjacoby
4 years ago

Replying to scribu:

If it's not a regression from 3.4, it's not that urgent.

All of these bugs are critical to actual users of the software.

Replying to miqrogroove:

Some of the above descriptions need to be qualified with the option of simply using e-mail addresses instead of usernames.

Painting the house doesn't fix the holes.

Last edited 4 years ago by johnjamesjacoby (previous) (diff)

#7 @ryan
4 years ago

This probably puts too much faith in is_email(), which has its problems. Maybe look for @ in a position other than 0?

#8 @miqrogroove
4 years ago

Or fix is_email(). If you need a solution for that, I know Google offers a free one that might be GPL compatible.

#9 follow-up: @ryan
4 years ago

#17491 for is_email().

It is possible to use a valid email address as a username in single site. Which is more common, a non-email username with @ or an email username?

#10 in reply to: ↑ 9 ; follow-up: @johnjamesjacoby
4 years ago

Replying to ryan:

It is possible to use a valid email address as a username in single site.

I have a hunch LDAP plugins support and/or enable this.

Which is more common, a non-email username with @ or an email username?

I have no stats, but seems moot since we've allowed users with @'s for this long.

#11 in reply to: ↑ 10 @rmccue
4 years ago

Replying to johnjamesjacoby:

Replying to ryan:

It is possible to use a valid email address as a username in single site.

I have a hunch LDAP plugins support and/or enable this.

You're definitely correct on that. I've experienced this issue when working with LDAP integration and it required workarounds (basically, replacing @ with another character when syncing from LDAP).

#12 @DrewAPicture
2 years ago

#28157 was marked as a duplicate.

#13 follow-up: @dd32
22 months ago

Perhaps the best option here is to optimize for what we think it is, but fall back to login search:

$user = false;
if ( $search contains '@' )
  $user = get_user_by( 'email', $search );
if ( ! $user )
  $user = get_user_by( 'login', $search );

#14 @johnbillion
21 months ago

  • Keywords needs-patch added; has-patch 3.6-early removed

#15 @johnbillion
10 months ago

  • Owner set to johnbillion
  • Status changed from new to reviewing

#16 @johnbillion
10 months ago

  • Keywords has-patch 2nd-opinion added; needs-patch removed
  • Milestone changed from Awaiting Review to Future Release

22367.2.patch updates the patch from JJJ. Note that user searches are wildcarded by default now (#32913), so checking for is_email() when searching users would prevent users for searching for things like @gmail.com.

#17 in reply to: ↑ 13 @gravitylover
7 months ago

I also believe this to be the best option. The patch submitted by John, although more correct than the current syntax, will not fix the issue with email usernames being unable to reset their password. The change in src/wp-includes/class-wp-user-query.php is perfect and would be welcomed!

Replying to dd32:

Perhaps the best option here is to optimize for what we think it is, but fall back to login search:

$user = false;
if ( $search contains '@' )
  $user = get_user_by( 'email', $search );
if ( ! $user )
  $user = get_user_by( 'login', $search );
Last edited 7 months ago by gravitylover (previous) (diff)

@gravitylover
7 months ago

Allow users to reset password who have emails as usernames

@gravitylover
7 months ago

patch incorporating modifications by JJJ and johnbillion

#18 follow-up: @johnbillion
4 months ago

  • Keywords dev-feedback added; has-patch 2nd-opinion removed
  • Milestone changed from Future Release to 4.5
  • Owner changed from johnbillion to johnjamesjacoby

@johnjamesjacoby Any issues remaining here now that #9568 is in?

#19 in reply to: ↑ 18 @johnjamesjacoby
4 months ago

Replying to johnbillion:

@johnjamesjacoby Any issues remaining here now that #9568 is in?

I will need to test my known conditions to confirm, and won't be able to do so until next week. I'm ambivalent about keeping this open, or closing and reopening if need be until then, so whatever is best for everyone is okay with me.

I can imagine some other, new issues, but I also haven't been paying attention to the connection between that initiative and this one.

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


4 months ago

#21 @ocean90
4 months ago

  • Milestone changed from 4.5 to Awaiting Review

#22 @websupporter
3 months ago

#36653 was marked as a duplicate.

#23 @websupporter
3 months ago

I've just checked for #36653 the following:

I can login with the username admin@example.com since the login via wp_authenticate_email_password() is hooked into wp_authenticate after wp_authenticate_username_password() and returns the $user if its a user object. But I still can't reset my password or find admin@example.com by conducting a search on the user page in wp-admin/

The patch for the password reset still seems to work. Didn't check for the search though.

Note: See TracTickets for help on using tickets.