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

[juliobox]

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.

[dipesh.kakadiya]

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

[utkarshpatel]

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.

[dipesh.kakadiya]

@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]

Remove cast Intefer & add check for integer & wp_user object

[dipesh.kakadiya]

Remove cast Intefer & add check for integer & wp_user object

[welcher]

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

[utkarshpatel]

Unit testcase added

[utkarshpatel]

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.

[boonebgorges]

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.

[boonebgorges]

In 34031:

Move wp_delete_user() tests to their own file.

See #33800.

[boonebgorges]

In 34033:

Move wp_delete_user() tests to their own file.

See #33800.

[dipesh.kakadiya]

Add only is_numeric check for wpmu_delete_user and wp_delete_user with unittest

[dipesh.kakadiya]

@boonebgorges

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

[boonebgorges]

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.