Make WordPress Core

Opened 12 years ago

Last modified 2 years ago

#22367 reviewing defect (bug)

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

Reported by: johnjamesjacoby's profile johnjamesjacoby Owned by: johnjamesjacoby's profile johnjamesjacoby
Milestone: Awaiting Review Priority: normal
Severity: normal Version:
Component: Users Keywords: has-patch needs-testing
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 (5)

22367.patch (1.4 KB) - added by johnjamesjacoby 12 years ago.
22367.2.patch (1.9 KB) - added by johnbillion 9 years ago.
wordpress-trunk-fix-login.patch (972 bytes) - added by gravitylover 9 years ago.
Allow users to reset password who have emails as usernames
22367.3.patch (2.1 KB) - added by gravitylover 9 years ago.
patch incorporating modifications by JJJ and johnbillion
22367.4.diff (2.1 KB) - added by donmhico 5 years ago.
Refreshed the patch.

Download all attachments as: .zip

Change History (35)

#1 @nacin
12 years ago

  • Milestone changed from 3.5 to Awaiting Review

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

#3 @scribu
12 years ago

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

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

  • Keywords 3.6-early added

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

#7 @ryan
12 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
12 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
12 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
12 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
12 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
11 years ago

#28157 was marked as a duplicate.

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

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

#15 @johnbillion
9 years ago

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

@johnbillion
9 years ago

#16 @johnbillion
9 years 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
9 years 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 9 years ago by gravitylover (previous) (diff)

@gravitylover
9 years ago

Allow users to reset password who have emails as usernames

@gravitylover
9 years ago

patch incorporating modifications by JJJ and johnbillion

#18 follow-up: @johnbillion
9 years 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
9 years 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.


9 years ago

#21 @ocean90
9 years ago

  • Milestone changed from 4.5 to Awaiting Review

#22 @websupporter
9 years ago

#36653 was marked as a duplicate.

#23 @websupporter
9 years 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.

#24 @gravitylover
8 years ago

What needs to happen next on this?

@donmhico
5 years ago

Refreshed the patch.

#25 @donmhico
5 years ago

  • Keywords has-patch added

Status when testing against the trunk.

  1. It's possible to create a new user with @ example @testing or testing@ in wp-admin/users.php and http://wp.contribute/wp-admin/network/site-users.php
  1. Searching for users with @ in their username doesn't work in wp-admin.
  1. Reset password for users using username with @ doesn't work.

The patch still fixes the issues, I refreshed it so it can be applied cleanly. From what I can see, I don't see any reason why we shouldn't include this to 5.3.

#26 @SergeyBiryukov
3 years ago

#54025 was marked as a duplicate.

#27 @SergeyBiryukov
3 years ago

#54093 was marked as a duplicate.

#28 @gchtr
3 years ago

I just ran into the issue in #54093 with a username that was generated from the email address, but contains a "+".

When a user is created with an email address test+75@example.org, then the generated username will be test75@example.org because sanitize_user() strips away the + in wp_insert_user(). (Using a + in an email address is quite common when you use Google Mail, because you can create email aliases with it.)

If I then try to send the password reset link, WordPress looks for a user with an email address test75@example.org, can’t find one and tells me that the user doesn’t exist.

When an is_email() check is used like in the suggested patches, then test75@example.org will still return true, but retrieve_password() will still fail.

#29 @daxelrod
3 years ago

At a minimum, this is related to #53634. The 22367.4.diff patch doesn't appear to account for the password reset issue where the "@" is in a non-0 position.

#30 @desrosj
2 years ago

  • Keywords needs-testing added; dev-feedback removed
  • Milestone set to Awaiting Review

Came across this one in a list of tickets missing a milestone.

Did some rough initial testing and it seems to still be an issue. Some additional testing on the latest version of Core (currently 6.0.1) to confirm all scenarios that have this issue would be a great way to move this one forward.

Note: See TracTickets for help on using tickets.