WordPress.org

Make WordPress Core

Opened 3 years ago

Last modified 8 weeks ago

#22367 reviewing defect (bug)

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

Reported by: johnjamesjacoby Owned by: johnbillion
Milestone: Future Release Priority: normal
Severity: normal Version:
Component: Users Keywords: has-patch 2nd-opinion
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 3 years ago.
22367.2.patch (1.9 KB) - added by johnbillion 4 months ago.
wordpress-trunk-fix-login.patch (972 bytes) - added by gravitylover 8 weeks ago.
Allow users to reset password who have emails as usernames
22367.3.patch (2.1 KB) - added by gravitylover 8 weeks ago.
patch incorporating modifications by JJJ and johnbillion

Download all attachments as: .zip

Change History (21)

#1 @nacin
3 years ago

  • Milestone changed from 3.5 to Awaiting Review

#2 @johnjamesjacoby
3 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 3 years ago by johnjamesjacoby (previous) (diff)

#3 @scribu
3 years ago

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

#4 follow-up: @miqrogroove
3 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
3 years ago

  • Keywords 3.6-early added

#6 in reply to: ↑ 4 @johnjamesjacoby
3 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 3 years ago by johnjamesjacoby (previous) (diff)

#7 @ryan
3 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
3 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
3 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
3 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
3 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
21 months ago

#28157 was marked as a duplicate.

#13 follow-up: @dd32
16 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
16 months ago

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

#15 @johnbillion
4 months ago

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

#16 @johnbillion
4 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
8 weeks 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 8 weeks ago by gravitylover (previous) (diff)

@gravitylover
8 weeks ago

Allow users to reset password who have emails as usernames

@gravitylover
8 weeks ago

patch incorporating modifications by JJJ and johnbillion

Note: See TracTickets for help on using tickets.