Make WordPress Core

Opened 23 months ago

Last modified 9 months ago

#57394 assigned defect (bug)

wp_insert_user allows the new user to have a username equal to an already registered email

Reported by: buutqn's profile buutqn Owned by: audrasjb's profile audrasjb
Milestone: Future Release Priority: normal
Severity: normal Version: 6.1.1
Component: Login and Registration Keywords: has-patch has-unit-tests needs-testing changes-requested
Focuses: Cc:

Description

Scenario:

User A is an administrator, has username = 'admin' and email = 'admin@…';
User B registers (open to register wp install) with username = 'admin@…' and email = 'test@…';

In my case, i use both users, one to admin the website, and the other to simulate an customer user. Both users had same password.

When i tried to log in with administrator user by email, i end up logged in the as user B.

Then i changed User B password to not conflict, and it works as it should, if i set user A password it gets logged in as user A and if i use User B password it gets logged in as User B.

I don't think this is a security issue, but allowing an user to be registered with another users email as username could be annoying and confuse some website administrators for scams or something else.

To solve this issue, i just filtered username_exists filter and checked if the username was existing email.

public function __construct() {
    add_filter("username_exists", array($this, "username_exists"));
}

public function username_exists($user_id, $username) {
    if (email_exists($username)) {
        $user = get_user_by("email", $username);
        if ($user->exists()) {
            return $user->ID;
        }
    }
    return $user_id;
}

Attachments (9)

53394.diff (1.4 KB) - added by dunhakdis 23 months ago.
Added check to determine if username supplied already exists as an email of an existing user.
3809.diff (1.4 KB) - added by dunhakdis 23 months ago.
Updated diff from @mukeshpanchal27 comments
unit-test-57394.diff (898 bytes) - added by ajayver 21 months ago.
Created a unit test that attempts to create a new user with admin email as a username.
patch-with-unit-test.diff (2.3 KB) - added by ajayver 21 months ago.
This patch includes both the fix and unit tests and the code standards fix.
admin-details.png (115.9 KB) - added by agnieszkaszuba 20 months ago.
new-user-details.png (324.0 KB) - added by agnieszkaszuba 20 months ago.
57394.diff (3.3 KB) - added by adamsilverstein 17 months ago.
57394-tests-updated.diff (3.7 KB) - added by costdev 17 months ago.
Splits the test method into two separate tests, with documentation updates and $message parameters for multiple assertions.
57394-create-username.test.js.diff (1.2 KB) - added by brianhenryie 14 months ago.
Single test confirming the error message is shown when attempting to register a second user with the first user's email address as username

Download all attachments as: .zip

Change History (62)

#1 @dunhakdis
23 months ago

I was able to reproduce the same behaviour. I think it should throw a WP_Error if the supplied username already exists as an email of an existing user.

The same behaviour can be reproduced with wp-login.php?action=register form.

Added a patch.

@dunhakdis
23 months ago

Added check to determine if username supplied already exists as an email of an existing user.

This ticket was mentioned in PR #3809 on WordPress/wordpress-develop by JosephGabito.


23 months ago
#2

  • Keywords has-patch added; needs-patch removed

@dunhakdis commented on PR #3809:


23 months ago
#3

@mukeshpanchal27 Thanks. I've applied your suggestions as commit. Cheers

@dunhakdis
23 months ago

Updated diff from @mukeshpanchal27 comments

#4 @roytanck
22 months ago

A possible privacy concern could be that this would allow a bad actor to try entering email addresses until it hits the new error message (assuming an open registration form). They would then know that that address is in use.

#5 @dunhakdis
22 months ago

I think that is also true for the default WordPress registration form with or without these changes.

#6 @adamsilverstein
21 months ago

  • Keywords needs-unit-tests added

ajayver commented on PR #3809:


21 months ago
#7

Hi @adamsilverstein, @JosephGabito I added a unit test for this fix.

@ajayver
21 months ago

Created a unit test that attempts to create a new user with admin email as a username.

#8 @dunhakdis
21 months ago

  • Keywords has-unit-tests added; needs-unit-tests removed

@ajayver , Thanks for adding a unit-test.

#10 @adamsilverstein
21 months ago

  • Keywords needs-unit-tests added; has-unit-tests removed
  • Milestone changed from Awaiting Review to 6.2
  • Owner set to adamsilverstein
  • Status changed from new to assigned

@ajayver
21 months ago

This patch includes both the fix and unit tests and the code standards fix.

ajayver commented on PR #3809:


21 months ago
#11

Fixed the failing code standards issue.

#12 @adamsilverstein
21 months ago

  • Resolution set to fixed
  • Status changed from assigned to closed

In 55358:

Login and Registration: prevent registering with username that matches previous user email.

When registering a new user, check that no existing user has an email matching the username.

Prevents a login name collision when one user registers with the email address user@… and a second user tries to register with the username user@….

Props buutqn, dunhakdis, roytanck, ajayver.
Fixes #57394.

This ticket was mentioned in PR #4095 on WordPress/wordpress-develop by @adamsilverstein.


21 months ago
#13

  • Keywords has-unit-tests added; needs-unit-tests removed

#14 @SergeyBiryukov
21 months ago

In 55360:

Users: Correct the error code in wp_insert_user() when login matches an existing email.

Move the test next to the other tests for user_login.

Follow-up to [55358].

See #57394.

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


20 months ago

This ticket was mentioned in PR #4257 on WordPress/wordpress-develop by polevaultweb.


20 months ago
#16

This patch improves the logic around the username checks added in WP 6.2, to make sure the check doesn't cause issues when updating a user.

Trac ticket: https://core.trac.wordpress.org/ticket/57967

polevaultweb commented on PR #4257:


20 months ago
#17

Thanks @costdev, applied and updated

#18 @audrasjb
20 months ago

  • Keywords revert added
  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopening this ticket as changesets [55358] and [55360] are probably going to be reverted, due to an issue reported in ticket #57967.

#19 @audrasjb
20 months ago

  • Owner changed from adamsilverstein to audrasjb
  • Status changed from reopened to assigned

@azaozz commented on PR #4257:


20 months ago
#20

Wondering if this PR should be repurposed or a new one opened to implement the logic discussed in https://github.com/WordPress/wordpress-develop/pull/4257#discussion_r1144203722 and the following comments.

#21 @hellofromTonya
20 months ago

@audrasjb are you still planning to commit the reverts?

#22 @audrasjb
20 months ago

In 55584:

Login and Registration: Revert [55358] and [55360].

This reverts the changes implemented in [55358] and [55360].

Changeset [55358] was committed to prevent login name collision when one user registers with the email address user@example.com and a second user tries to register with the username user@example.com. However, it also introduced a potential backward compatibility issues for plugins that use wp_update_user(). When updating an existing user, it throws an existing_user_email_as_login error if the email address is also used for the user login, due to the code introduced in [55358].

This changeset removes the new scenario added in [55358] and [55360], restoring the wp_insert_user() function back to its previous state.

Props polevaultweb, audrasjb, costdev, peterwilsoncc, hellofromTonya, SergeyBiryukov, azaozz.
See #57967, #57394.

#23 @hellofromTonya
20 months ago

  • Keywords dev-feedback added

Marking for double committer sign-off.

#24 @hellofromTonya
20 months ago

  • Keywords dev-reviewed added; dev-feedback removed

[55584] is ready for backport to 6.2 branch.

#25 @audrasjb
20 months ago

In 55585:

Login and Registration: Revert [55358] and [55360].

This reverts the changes implemented in [55358] and [55360].

Changeset [55358] was committed to prevent login name collision when one user registers with the email address user@example.com and a second user tries to register with the username user@example.com. However, it also introduced a potential backward compatibility issues for plugins that use wp_update_user(). When updating an existing user, it throws an existing_user_email_as_login error if the email address is also used for the user login, due to the code introduced in [55358].

This changeset removes the new scenario added in [55358] and [55360], restoring the wp_insert_user() function back to its previous state.

Props polevaultweb, audrasjb, costdev, peterwilsoncc, hellofromTonya, SergeyBiryukov, azaozz.
Reviewed by hellofromTonya.
Merges [55584] to the 6.2 branch.
See #57967, #57394.

#26 @hellofromTonya
20 months ago

  • Keywords changes-requested needs-testing needs-unit-tests added; has-unit-tests dev-reviewed removed
  • Milestone changed from 6.2 to 6.3

The changesets have been reverted. Additional considerations are needed to address the issue raised #57967 and potentially other scenarios.

With 6.2 Dry Run and final release next week, it's too late in the cycle to continue working on this. Moving to 6.3.

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


20 months ago

#28 @agnieszkaszuba
20 months ago

Test Report

This report validates that the indicated patch addresses the issue.

Patch tested: https://core.trac.wordpress.org/ticket/57394

Environment

  • OS: macOS 13.0.1
  • Web Server: Nginx
  • PHP: 7.4.24
  • WordPress: 6.3-alpha-55505-src
  • Browser: Chrome 109.0.5414.119 (Official Build) (x86_64)
  • Theme: Twenty Twenty-Three
  • Active Plugins: -

Actual Results

  • ✅ Issue resolved with patch.

Additional Notes

Supplemental Artifacts



This ticket was mentioned in PR #4749 on WordPress/wordpress-develop by @adamsilverstein.


17 months ago
#29

  • Keywords has-unit-tests added; needs-unit-tests removed

#30 @adamsilverstein
17 months ago

https://github.com/WordPress/wordpress-develop/pull/4749 includes a new fix for this: the additional check is only applied when a new user is being created - and not for user updates.

#31 @oglekler
17 months ago

  • Keywords changes-requested removed

Ticket needs testing.

Last edited 17 months ago by oglekler (previous) (diff)

#32 @johnbillion
17 months ago

  • Keywords revert removed

@costdev
17 months ago

Splits the test method into two separate tests, with documentation updates and $message parameters for multiple assertions.

#33 @costdev
17 months ago

@adamsilverstein I've submitted 57394-tests-updated.diff with some test updates. Feel free to add them to PR 4749 if you want to keep everything together. 🙂

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


16 months ago

This ticket was mentioned in Slack in #core-test by oglekler. View the logs.


16 months ago

#36 @chaion07
16 months ago

Thanks @buutqn for reporting this. We reviewed this ticket during a recent bug-scrub session and based on the feedback received we are keeping the milestone intact for another 6 hours hoping that someone would volunteer. Otherwise we are happy to update the milestone to 6.4

Props to @mukesh27 & @oglekler

#37 @audrasjb
16 months ago

  • Milestone changed from 6.3 to 6.4

Moving to 6.4 as we're approaching 6.3 RC1.

This ticket was mentioned in PR #5032 on WordPress/wordpress-develop by @rajinsharwar.


15 months ago
#38

Checking for username and email conflicts for new users and units tests

Trac ticket: https://core.trac.wordpress.org/ticket/57394

#39 @rajinsharwar
15 months ago

Adding another updated PR with the changes in wp-admin/includes/user.php. Because there are some warnings shown while trying to create a user with the same email as the username if we don't make these updated changes. Requesting a review. Added the unit tests from @costdev in the PR as well, to keep it all in one place.

https://img001.prntscr.com/file/img001/RAREwFy2TTO-fSMX3RPg7A.png

#40 @rajinsharwar
15 months ago

  • Keywords needs-testing removed

Test Report

This report validates that the indicated patch addresses the issue.

Patch tested: https://core.trac.wordpress.org/ticket/57394

Environment

OS: Windows 11
Web Server: Nginx
PHP: 8.1.9
WordPress: 6.3
Browser: Chrome 115.0.5790.171 (Official Build) (64-bit)
Theme: Twenty Twenty-Three
Active Plugins: - none

Actual Results

✅ Issue resolved with the patch (PR: 5032https://github.com/WordPress/wordpress-develop/pull/5032). No errors/warnings were triggered.

Attachments

https://img001.prntscr.com/file/img001/pR2juFYYQaq0D88DYgk0Cg.png

#41 @rajinsharwar
15 months ago

  • Keywords needs-testing added

Let's make this new patch test further by some more people, then I guess, we can put this for commit.

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


14 months ago

This ticket was mentioned in Slack in #core-test by oglekler. View the logs.


14 months ago

@brianhenryie
14 months ago

Single test confirming the error message is shown when attempting to register a second user with the first user's email address as username

#44 @rajinsharwar
14 months ago

Great, we can put this for commit then I believe @audrasjb

#45 @rajinsharwar
14 months ago

  • Keywords commit added

This ticket was mentioned in Slack in #core-test by brianhenryie. View the logs.


13 months ago

#47 @hellofromTonya
13 months ago

A 6.4 Beta 5 is tentatively scheduled for tomorrow (Oct 16th) and RC1 is the next day (Oct 17th).

@audrasjb is this ready for commit? Are you planning to commit it?

#48 @ironprogrammer
13 months ago

Test Report

Patch tested: https://github.com/WordPress/wordpress-develop/pull/5032 👍🏻

Environment

  • Hardware: MacBook Pro Apple M1 Pro
  • OS: macOS 13.6
  • Browser: Safari 16.6
  • Server: nginx/1.25.2
  • PHP: 8.2.11
  • WordPress: 6.4-alpha-56267-src
  • Theme: twentytwentythree v1.2

Actual Results

  • ✅ Unable to create a new user (/wp-admin/user-new.php) with a username that matches an existing user's email. An error error message is returned (Figure 1).
  • ✅ Unable to register a new user (/wp-login.php?action=register) with a username that matches an existing user's email. An error error message is returned (Figure 2).

Supplemental Artifacts

Figure 1: New User result if username admin with email admin@example.com already exists.

https://cldup.com/5gpLMhTUrQ.png

Figure 2: Registration result if username admin with email admin@example.com already exists.

https://cldup.com/MjRTn9X4k8.png

#49 @ironprogrammer
13 months ago

@brianhenryie, thank you for the e2e test (attachment:57394-create-username.test.js.diff) 🙌🏻

I think this would be good to include alongside this update, but now that all Core tests have been migrated to Playwright, this could be refreshed as a follow-up ticket to possibly merge during RC.

#50 @hellofromTonya
13 months ago

  • Keywords changes-requested added; commit removed
  • Milestone changed from 6.4 to 6.5

In reviewing the patch and this ticket's history, it's not clear to me that the patch addresses the additional concerns shared here, concerns that lead to the original patch being reverted (i.e. reported in #57967).

Given that tomorrow is RC1, I think this ticket needs more time and consideration to ensure it handles the different scenarios. Moving it to 6.5. However, if there is confirmation that yes indeed all scenarios are resolved before RC1, then please move it back into 6.4.

#51 @sumitbagthariya16
12 months ago

Test Report
Patch tested: https://github.com/WordPress/wordpress-develop/pull/5032
---

Environment
OS: macOS 13.3
Browser: Chrome
Web Server: Nginx 1.20.2
PHP: 8.0.30
WordPress: 6.4.1
Theme: Twenty Eleven v4.5

Screenshot:
https://prnt.sc/_rq2qHrcej8n

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


9 months ago

#53 @rajinsharwar
9 months ago

  • Milestone changed from 6.5 to Future Release

Punting this for Future Releases, as the patch still needs some work to be done.

Note: See TracTickets for help on using tickets.