WordPress.org

Make WordPress Core

Opened 6 years ago

Closed 6 years ago

#33800 closed defect (bug) (fixed)

wp_delete_user delete the user ID 1 if an object is passed in param

Reported by: juliobox Owned by: boonebgorges
Milestone: 4.4 Priority: normal
Severity: normal Version: 2.0
Component: Users Keywords: has-patch
Focuses: Cc:

Description

Hello, consider this, using a fresh installation:

$user_id = wp_insert_user( $args ); // with email already in use
wp_delete_user( $user_id );

Because of the email issue, $user_id is not an actual ID (ie. integer) but an object, a WP_Error one.

So if you pass an object into wp_delete_user, it will do this:

$id = (int) $id;
$user = new WP_User( $id );

Now, your object is equal to "1", and "1" is the 1st user, the admin.
Since WordPress allow the site to have 0 users (what?) and delete the last admin (WHAAAAT??), this is kind of annoying.

So, i know, the developper has to check if the $user_id is not a WP_Error, but this is still strange to cast INT.

I would recommand to not cast integer, but check if is_numeric or is already a WP_User.

Attachments (5)

33800.diff (406 bytes) - added by dipesh.kakadiya 6 years ago.
Add check into 'wp_delete_user' for varify id is numeric value or not
33800_1.diff (898 bytes) - added by dipesh.kakadiya 6 years ago.
Remove cast Intefer & add check for integer & wp_user object
33800_2.diff (459 bytes) - added by dipesh.kakadiya 6 years ago.
Remove cast Intefer & add check for integer & wp_user object
33800_with_testcase.diff (1.5 KB) - added by utkarshpatel 6 years ago.
Unit testcase added
33800_final.diff (1.8 KB) - added by dipesh.kakadiya 6 years ago.
Add only is_numeric check for wpmu_delete_user and wp_delete_user with unittest

Download all attachments as: .zip

Change History (17)

@dipesh.kakadiya
6 years ago

Add check into 'wp_delete_user' for varify id is numeric value or not

#1 @dipesh.kakadiya
6 years ago

  • Keywords has-patch added

#2 @utkarshpatel
6 years ago

  • Keywords has-patch removed

If you check constructor of WP_user it expects int(user_id), Object of WP_User or Object with ID as user_id.

		if ( $id instanceof WP_User ) {
			$this->init( $id->data, $blog_id );
			return;
		} elseif ( is_object( $id ) ) {
			$this->init( $id, $blog_id );
			return;
		}

So we can certainly not add a check for numeric and WP_User object only.

$user_id = wp_insert_user( $args ); // with email already in use
if ( ! $user_id instanceof WP_Error)
    wp_delete_user( $user_id );

Should handle in coding itself.

Last edited 6 years ago by utkarshpatel (previous) (diff)

#3 @dipesh.kakadiya
6 years ago

@utkarshpatel

So we can certainly not add a check for numeric and WP_User object only.

Yeah i agree, it's developer mistake. But if developer missed to check wp_error the admin is deleted.

I know WP_user it expects int(user_id), Object of WP_User or Object with ID as user_id but there are cast integer for $id in wp_delete_user

$id = (int) $id;

@dipesh.kakadiya
6 years ago

Remove cast Intefer & add check for integer & wp_user object

@dipesh.kakadiya
6 years ago

Remove cast Intefer & add check for integer & wp_user object

#4 @dipesh.kakadiya
6 years ago

  • Keywords has-patch added

#5 @welcher
6 years ago

  • Keywords needs-unit-tests added

We should probably get some Unit Tests in place for this as well.

@utkarshpatel
6 years ago

Unit testcase added

#6 @utkarshpatel
6 years ago

  • Keywords needs-unit-tests removed

#7 @utkarshpatel
6 years ago

That test-case needs some changes when i run single test case it runs just fine but when i run all test case same test case breaks. It accepts duplicate user_email without error while running all test-cases which is weird. I will look into this tomorrow.

#8 @boonebgorges
6 years ago

  • Milestone changed from Awaiting Review to 4.4

A brief search through the codebase suggests that there are several dozen places where we cast a $user_id parameter to an integer. None of these instances are as harmful as wp_delete_user() (aside from wpmu_delete_user(), which has the exact same problem). And it's highly likely that we do the same thing in dozens of places with posts, terms, etc as well.

In the long run, it would be ideal to modify all these functions - or at least the update/delete functions - so that they (a) do stricter type checking on the ID params, and (b) also accept the corresponding WP_User, WP_Post, etc objects. See #33638 for a related issue.

In the short term, I don't think it's necessary to add WP_User support for this function. It's not currently possible to pass a user object to wp_delete_user(), and I don't see the benefit of adding it here without also adding it across the whole API. So let's just add is_numeric() checks to wp_delete_user() and wpmu_delete_user() for now.

#9 @boonebgorges
6 years ago

In 34031:

Move wp_delete_user() tests to their own file.

See #33800.

#10 @boonebgorges
6 years ago

In 34033:

Move wp_delete_user() tests to their own file.

See #33800.

@dipesh.kakadiya
6 years ago

Add only is_numeric check for wpmu_delete_user and wp_delete_user with unittest

#11 @dipesh.kakadiya
6 years ago

@boonebgorges

As you suggest, I updated patch and unittest for wp_delete_user. test_delete_user is already existed inside 'tests/phpunit/tests/user.php' so I wrote my test case below test_delete_user.

Last edited 6 years ago by dipesh.kakadiya (previous) (diff)

#12 @boonebgorges
6 years ago

  • Owner set to boonebgorges
  • Resolution set to fixed
  • Status changed from new to closed

In 34034:

Require numeric IDs in user deletion functions.

wp_delete_user() and wpmu_delete_user() both require an $id parameter.
Previously, the functions did not verify that the value passed was, in fact,
a number. As such, passing an object or any other entity that would be cast
to int 1 would result in user 1 being deleted. We fix this by enforcing
the requirement that $id be numeric.

Props dipesh.kakadiya, utkarshpatel, juliobox.
Fixes #33800.

Note: See TracTickets for help on using tickets.