WordPress.org

Make WordPress Core

Opened 9 days ago

Last modified 6 days ago

#44347 new enhancement

WP allows creating username that is already used email address

Reported by: phillipburger Owned by:
Milestone: Awaiting Review Priority: normal
Severity: normal Version:
Component: Users Keywords: has-patch
Focuses: Cc:

Description

As reported in Support Forum (https://wordpress.org/support/topic/wp-allows-creating-username-that-is-already-used-email-address/) it seems I can create a user with wp_create_user where the user's "username" is set as a value that is an existing Email Address for another user in the WordPress system.

(I have not submitted a bug here before, so let me know if more info is needed on this and what to do next).

Attachments (2)

44347.includes.user.php.diff (1.7 KB) - added by subrataemfluence 7 days ago.
File path: /wp-admin/includes/user.php
44347.wp-includes.user.php.diff (1.2 KB) - added by subrataemfluence 7 days ago.
File path: /wp-includes/user.php

Download all attachments as: .zip

Change History (9)

#1 follow-up: @pbiron
9 days ago

  • Component changed from General to Users

Welcome to trac!!!

Nice catch...in all my years building WP sites I never thought to use email addresses as usernames :-)

In the support topic you reference you say,

I have since added a check in my code to stop this

I'm not sure how you implemented that check, but I would suggest you do so using the username_exists filter, as follows:

<?php
add_filter( 'username_exists', 'my_username_exists_filter_func', 10, 2 );
function my_username_exists_filter_func( $user_id, $username ) {
        if ( $user_id ) {
                return $user_id;
        }

        return get_user_by( 'email', $username );
}

This will cause wp_create_user() to return a WP_Error and the new user will not be created.

#2 in reply to: ↑ 1 @phillipburger
9 days ago

Thanks! Yeah, I guess when you allow end users to input information, they will find the bugs.

I was using a check similar to the example on https://developer.wordpress.org/reference/functions/wp_create_user/#user-contributed-notes which checks:

  1. does the username entered exist as a username
  2. does the email entered exist as an email address

but it forgot to check the other 2 ways of:

  1. does the username exist as an email address (the problem I had in this case)
  2. does the email address exist as a username (a reverse problem that would cause the same issue)

I just over coded my side to do checks of username and email address both in username_exists() and email_exists() and then make sure all 4 checks brought back false before processing.

I have been using filters already but I did not think of it here.

Let me know any more info needed.

Replying to pbiron:

Welcome to trac!!!

Nice catch...in all my years building WP sites I never thought to use email addresses as usernames :-)

In the support topic you reference you say,

I have since added a check in my code to stop this

I'm not sure how you implemented that check, but I would suggest you do so using the username_exists filter, as follows:

<?php
add_filter( 'username_exists', 'my_username_exists_filter_func', 10, 2 );
function my_username_exists_filter_func( $user_id, $username ) {
        if ( $user_id ) {
                return $user_id;
        }

        return get_user_by( 'email', $username );
}

This will cause wp_create_user() to return a WP_Error and the new user will not be created.

#3 @phillipburger
9 days ago

As I thought more about this one, it might actually be a bigger problem. Lets say that a user (maybe the only admin of a site) has an account like:

username: my_admin_username (clearly a poor username) email: websiteadmin@…

If a bad person happens to know that email address and they have a way to register a new user on that website and they decide to create an account with:

username: websiteadmin@… email: mypersonalemail@…

The problem I realized is, hopefully the admin knows their username and does not always log in with email address because the lost email and other login dialogs that use "username or email address" seem to check the email address against username first - so that admin user may be locked out of the site.

Thanks again, hope this helps.

#4 follow-up: @subrataemfluence
8 days ago

Good find!! Adding additional guard for checking the existence of username supplied in email field in core functionality should resolve the issue.

Possible solution:

File name: includes/user.php.
Function: edit_user

Addition:

<?php
if ( ! $update && email_exists( $user->user_login ) ) {
   $errors->add( 'user_login', __( '<strong>ERROR</strong>: This username is invalid because it is already in use as email address of another account.' ) );
}
File: wp-includes/user.php
Function: register_new_user

Addition:

<?php
if ( $user_email == '' ) {
   ...
} elseif ( ! is_email( $user_email ) ) {
   ...
} elseif ( email_exists( $user_email ) ) {
   ...
} elseif ( email_exists( $sanitized_user_login ) ) {
   $errors->add( 'email_exists', __( '<strong>ERROR</strong>: This username is invalid because it is already in use as email address of another account.' ) );
}

I think adding the check into the core itself will prevent additional filter and functions.

Please let me know if this makes some sense.

#5 in reply to: ↑ 4 @phillipburger
8 days ago

As much input as possible is great, I think this seems to make sense when a new username comes in to make sure it is not already existing email address, but the other way too - what if a new email address is entered and it is already a username - that needs to be verified as well.

And as this is fixed, what should be a solution if these situations already exist in a deployment? Do they need to be flagged for updates somehow?

Replying to subrataemfluence:

Good find!! Adding additional guard for checking the existence of username supplied in email field in core functionality should resolve the issue.

Possible solution:

File name: includes/user.php.
Function: edit_user

Addition:

<?php
if ( ! $update && email_exists( $user->user_login ) ) {
   $errors->add( 'user_login', __( '<strong>ERROR</strong>: This username is invalid because it is already in use as email address of another account.' ) );
}
File: wp-includes/user.php
Function: register_new_user

Addition:

<?php
if ( $user_email == '' ) {
   ...
} elseif ( ! is_email( $user_email ) ) {
   ...
} elseif ( email_exists( $user_email ) ) {
   ...
} elseif ( email_exists( $sanitized_user_login ) ) {
   $errors->add( 'email_exists', __( '<strong>ERROR</strong>: This username is invalid because it is already in use as email address of another account.' ) );
}

I think adding the check into the core itself will prevent additional filter and functions.

Please let me know if this makes some sense.

#6 @subrataemfluence
7 days ago

  • Keywords has-patch added
  • Type changed from defect (bug) to enhancement

Uploading two proposed patches. Please let me know if they make sense!

@subrataemfluence
7 days ago

File path: /wp-admin/includes/user.php

@subrataemfluence
7 days ago

File path: /wp-includes/user.php

#7 @phillipburger
6 days ago

To me the solutions look good, but I definitely want some other people to input here since I have never been involved in any changes to core WP.

Also, what are thoughts on situations where this already exists in a deployment that is upgraded to a version where this is included? The usernames that match email may already be in place, should there be some place that has a small warning about potential overlaps and allow the admin to correct the situation? I know that could be hard since usernames are not editable and typically an email address is not going to be something someone will change on an account.

Last thought for now - because these checks are only checking for existing data in the user portion of the database, if someone creates an account where username and email address are the same - that is still acceptable and will pass these checks, right?

Note: See TracTickets for help on using tickets.