Make WordPress Core

Opened 6 months ago

Closed 5 months ago

#63770 closed defect (bug) (fixed)

WordPress wp_insert_user() throws warning when password is not provided

Reported by: sheldorofazeroth's profile sheldorofazeroth Owned by: sheldorofazeroth's profile sheldorofazeroth
Milestone: 6.9 Priority: normal
Severity: normal Version:
Component: Users Keywords: has-test-info has-patch commit
Focuses: coding-standards, php-compatibility Cc:

Description

The wp_insert_user() function in WordPress allows creating a user without explicitly providing a password. In this case, WordPress correctly generates a random password for the user. However, it still produces the following warning:

PHP Warning: Undefined array key "user_pass" in wp-includes\user.php on line 2219

Fix to this would be Either:
Make the password parameter mandatory for wp_insert_user(), OR Add proper validation to handle cases when password is not provided to prevent the warning

Attachments (2)

wp_insert_user.patch (533 bytes) - added by iamadisingh 6 months ago.
This patch adds a check in wp_insert_user() to return a WP_Error if user_pass is missing or empty.
wp_insert_user2.patch (606 bytes) - added by iamadisingh 6 months ago.

Download all attachments as: .zip

Change History (23)

#1 @sheldorofazeroth
6 months ago

@johnbillion Can you please have a look at it? I think this is something which can be added to WordPress 6.9 release.

#2 follow-up: @dd32
6 months ago

  • Severity changed from major to normal

The user_pass is a required parameter of wp_insert_user() at present. It's not optional, nor is it defaulting to a random password.

In this case, WordPress correctly generates a random password for the user.

This functionality is not part of wp_insert_user(), instead this is part of register_new_user() which does not allow specifying the user's password.

#3 @rollybueno
6 months ago

  • Keywords has-test-info added
  • Version set to trunk

Reproduction Report

Description

This report validates whether the issue can be reproduced.

Environment

  • WordPress: 6.9-alpha-60093-src
  • PHP: 8.2.28
  • Server: nginx/1.29.0
  • Database: mysqli (Server: 8.4.5 / Client: mysqlnd 8.2.28)
  • Browser: Chrome 137.0.0.0
  • OS: Linux
  • Theme: Twenty Twenty-Five 1.3
  • MU Plugins: None activated
  • Plugins:
    • Query Monitor 3.18.0
    • Test Reports 1.2.0

Actual Results

  1. ✅ Error condition occurs (reproduced). Tested on laster trunk and it's still happening!

Additional Notes

Use the following code on your theme's function:

// Test wp_insert_user() warning - remove after testing
add_action(
	'init',
	function () {
			// Enable error reporting
			error_reporting( E_ALL );
			ini_set( 'display_errors', 1 );

			echo "=== Testing wp_insert_user() without password ===\n";

			// Test creating user without password
			$userdata = array(
				'user_login'   => 'testuser_' . time(),
				'user_email'   => 'testuser_' . time() . '@example.com',
				'display_name' => 'Test User',
			// 'user_pass' is intentionally omitted
			);

			echo "Creating user without password...\n";
			$user_id = wp_insert_user( $userdata );

		if ( is_wp_error( $user_id ) ) {
			echo 'Error: ' . $user_id->get_error_message() . "\n";
		} else {
			echo "User created successfully with ID: $user_id\n";

			// Clean up - delete the test user
			require_once ABSPATH . 'wp-admin/includes/user.php';
			wp_delete_user( $user_id );
		}
	}
);

Supplemental Artifacts

Result:
https://i.imgur.com/v6LM3si.png

#4 in reply to: ↑ 2 ; follow-up: @rollybueno
6 months ago

There's no check in place to see if the user_pass key exists in $userdata. It only assume it exists by default. If we intend to make it required, we should add a check and throw WP_Error in case it's missing.

Replying to dd32:

The user_pass is a required parameter of wp_insert_user() at present. It's not optional, nor is it defaulting to a random password.

In this case, WordPress correctly generates a random password for the user.

This functionality is not part of wp_insert_user(), instead this is part of register_new_user() which does not allow specifying the user's password.

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


6 months ago

#6 in reply to: ↑ 4 ; follow-up: @dd32
6 months ago

Replying to rollybueno:

There's no check in place to see if the user_pass key exists in $userdata. It only assume it exists by default. If we intend to make it required, we should add a check and throw WP_Error in case it's missing.

Yeah.. but that's very different to the claimed ticket description, and arguably, PHP Warnings are a valid outcome - Pass junk data, receive junk reply.

That's very common with most low-level WordPress functions, there's an assumption that if you're using the low-level function, you're passing correct data (and have the correct permissions).

#7 @shilpaashokan94
6 months ago

The warning occurs because the wp_insert_user() function expects the user_pass key in the $userdata array, even though it gracefully handles missing passwords by generating a random one later in the process.

✅ Proposed Fix:
We can prevent the PHP warning by checking whether user_pass is set before accessing it. This can be handled with a simple validation near line 2219 of wp-includes/user.php. Here's a suggested patch:

$update = false;
$user_pass = isset( $userdatauser_pass? ) ? $userdatauser_pass? : ;

This ensures $user_pass is always defined, and prevents the warning from being triggered when no password is passed.

🔒 Security Consideration:
The current behavior of generating a random password when none is supplied should remain unchanged. We're only fixing the warning without altering the intended logic.

🧪 Testing:
This patch can be tested by calling wp_insert_user() without a password:

wp_insert_user( array(

'user_login' => 'testuser',
'user_email' => 'test@…',

) );

#8 in reply to: ↑ 6 @iamadisingh
6 months ago

Replying to dd32:

Replying to rollybueno:

There's no check in place to see if the user_pass key exists in $userdata. It only assume it exists by default. If we intend to make it required, we should add a check and throw WP_Error in case it's missing.

Yeah.. but that's very different to the claimed ticket description, and arguably, PHP Warnings are a valid outcome - Pass junk data, receive junk reply.

That's very common with most low-level WordPress functions, there's an assumption that if you're using the low-level function, you're passing correct data (and have the correct permissions).

Wouldn't it be better for wp_insert_user() to be stricter and return a WP_Error if user_pass is missing, similar to how it handles other required fields? This would make the function safer and more predictable for developers, and avoid creating users with empty passwords.

@iamadisingh
6 months ago

This patch adds a check in wp_insert_user() to return a WP_Error if user_pass is missing or empty.

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


6 months ago
#9

  • Keywords has-patch added; needs-patch removed

#10 @hbhalodia
6 months ago

Hi @iamadisingh, I think sending an WP_Error is not a good idea as WP automatically handle new password creation, instead what can be done is we can add a check which is already added while the user is updated - https://github.com/WordPress/wordpress-develop/blob/ce9fd87141a4cd948a2ecd15322f9d9a87a85651/src/wp-includes/user.php#L2221

I have added a PR to add the similar check for the same - https://github.com/WordPress/wordpress-develop/pull/9364

Thank You,

#11 @mindctrl
6 months ago

This is an interesting one. wp_insert_user doesn't purposely generate a password if one is not provided, but when it passes the empty/null value to wp_hash_password, a hash is generated and that is saved with the user.

It triggers various PHP warnings, and results in a user whose password you can't use/know without resetting.

I think it would be good to consider firming up some of the logic.

#12 follow-up: @peterwilsoncc
6 months ago

  • Focuses docs added

I agree with @dd32 that throwing a warning in this instance is correct. The error can only be hit when developers call the function incorrectly.

I think the fix for this is to improve the documentation for the function to note which items are required and which items are optional in the $userdata docblock.

#13 in reply to: ↑ 12 @sheldorofazeroth
6 months ago

  • Focuses coding-standards added; docs removed

Replying to peterwilsoncc:

I agree with @dd32 that throwing a warning in this instance is correct. The error can only be hit when developers call the function incorrectly.

I think the fix for this is to improve the documentation for the function to note which items are required and which items are optional in the $userdata docblock.

@peterwilsoncc, If a password is required and not provided, user creation should be prevented. The function should return a clear and structured error object or message, rather than merely issuing a warning. Throwing warnings in such cases is not a good practice.

If the password is marked as required in the function's documentation, then the function should enforce this by blocking user creation when the password is not provided. In such cases, it should return a clear and structured error object. Alternatively, if we choose not to enforce it strictly, we should at least implement a proper check and handle it gracefully instead of relying on a warning.

Let me know what you think?

#14 @hbhalodia
6 months ago

I agree with @dd32 that throwing a warning in this instance is correct. The error can only be hit when developers call the function incorrectly.

I think the fix for this is to improve the documentation for the function to note which items are required and which items are optional in the $userdata docblock.

While I understand the perspective, I respectfully disagree. Accessing an array key without first verifying its existence is considered poor practice, as it can lead to unnecessary warnings. Although these warnings may be suppressed on production environments and remain invisible to end users, it is still best practice to implement proper checks before accessing array elements. This approach ensures more robust and maintainable code, and prevents avoidable issues from arising in different environments.

Even if the docblock specifies that a particular key is required, it is still prudent for the consuming code to verify the presence of that key. If the key is missing, the code should either initialize it accordingly or throw an appropriate error—both approaches are acceptable depending on the context. In our current implementation, however, the necessary isset or empty check is missing, which ideally should be included to ensure the code’s reliability and robustness.

#15 follow-up: @peterwilsoncc
6 months ago

@sheldorofazeroth @hbhalodia I understand what's behind your argument but as the function is a low level function for use by developers. Logging a warning is the correct course of action.

I agree it would be better if the account wasn't created but WordPress strives to maintain backward compatibility that needs to be the case going forward.

A compromise I might accept is as follows:

That will improve the details provided to the developer calling the function to provide specifics rather than the build in PHP error notices.

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


6 months ago
#16

This PR implements improved handling for missing passwords in wp_insert_user(). Adds a check for an empty or missing user_pass when creating a new user. If user_pass is not provided, it triggers a developer warning using wp_trigger_error() with E_USER_WARNING and a clear message. Securely generates and hashes a 32-character random password for the new user, ensuring backward compatibility.

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

#17 in reply to: ↑ 15 @iamadisingh
6 months ago

Replying to peterwilsoncc:

@sheldorofazeroth @hbhalodia I understand what's behind your argument but as the function is a low level function for use by developers. Logging a warning is the correct course of action.

I agree it would be better if the account wasn't created but WordPress strives to maintain backward compatibility that needs to be the case going forward.

A compromise I might accept is as follows:

That will improve the details provided to the developer calling the function to provide specifics rather than the build in PHP error notices.

I've opened a PR that implements this approach. I agree that maintaining backward compatibility is important here, so the function now triggers a developer warning and generates a secure random password if user_pass is missing, rather than returning an error. Thanks for the discussion and feedback!

#18 @hbhalodia
6 months ago

Hi @peterwilsoncc, I guess that would be more stable approach, we check for empty and as well we notify the user that this is kind of required arguement, but if not provided a random password get's generated.

@iamadisingh, Thanks for the PR as per the comments.

#20 @peterwilsoncc
6 months ago

  • Component changed from Login and Registration to Users
  • Focuses php-compatibility added
  • Keywords commit added
  • Milestone changed from Awaiting Review to 6.9
  • Version trunk deleted

PR#9381 looks good for commit, marking as such.

I've done a little ticket maintenance too:

  • moved it to the Users component as the issue doesn't occur during registration, only when developer call the function incorrectly
  • added the PHP-Compatibility focus as the warnings thrown & quantity of warnings thrown varies with different versions of PHP (PHP 8.0 throws one; PHP 8.1+ throws two)

#21 @peterwilsoncc
5 months ago

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

In 60650:

Users: Throw specific warning when wp_insert_user() called without user_pass.

Modifies wp_insert_user() to throw the warning The user_pass field is required when creating a new user. The user will need to reset their password before logging in. when called without the user_pass argument defined.

This avoids a mix of warnings being thrown depending on the version of PHP the system is running on, anywhere between zero and three.

To retain backward compatibility the user is created with an empty password. As WordPress does not accept an empty password during authentication, this will require the newly created user complete the password reset process.

Props dd32, hbhalodia, iamadisingh, mindctrl, rollybueno, sheldorofazeroth, shilpaashokan94
Fixes #63770.

Note: See TracTickets for help on using tickets.