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 | Owned by: | 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)
Change History (35)
#4
follow-up:
↓ 6
@
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.
#6
in reply to:
↑ 4
@
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.
#7
@
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
@
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:
↓ 10
@
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:
↓ 11
@
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
@
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).
#13
follow-up:
↓ 17
@
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 );
#16
@
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
@
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 );
#18
follow-up:
↓ 19
@
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
@
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
#23
@
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.
#25
@
5 years ago
- Keywords has-patch added
Status when testing against the trunk.
- It's possible to create a new user with
@
example@testing
ortesting@
in wp-admin/users.php and http://wp.contribute/wp-admin/network/site-users.php
- Searching for users with
@
in their username doesn't work in wp-admin.
- 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.
#28
@
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
@
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
@
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.
Reminder that as it stands now, users with "@" in their logins:
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.