edit_user() doesn't check for empty password (pass1).

[gitlost]

This is a follow-up to #33101.

The check for an empty password in edit_user() got lost in the wash, so if you're calling it programmatically you can't rely on it checking your POST data, which is inconvenient (and causes a PHP undefined index notice later on user_pass). A simple restoration of eg

        if ( empty( $pass1 ) )
                $errors->add( 'pass', __( '<strong>ERROR</strong>: Please enter your password.' ), array( 'form-field' => 'pass1' ) );

would do it.

[gitlost]

Actually that "simple restoration" would have to be if ( ! $update && empty( $pass1 ) ), as it's only an issue when adding a user.

[wesleye]

Patch

[wesleye]

This is my first patch, so please tell me if I can do something to improve.

Changed:

• Check for empty passwords in post data, return error if not set

[gitlost]

Hi, seeing as you ask(!) note that patches should be made from the ​SVN trunk root so in this case the diff should reference "src/wp-admin/includes/user.php" rather than just "wp-admin/includes/user.php". Also you should run ​phpunit after applying your patch (here in particular phpunit --group=user) to check it doesn't obviously break stuff, and preferably include a failing-before / succeeding-after unit test.

On the actual patch I think the check should be made after the 'check_passwords' action is called to maintain flexibility. Also it should only be checked when adding a user (! $updated) as using a blank password when updating a user is legitimate usage (meaning don't update the password). Also I think it should only check $pass1 as there's already a check for $pass1 != $pass2.

I'll upload a unit test.

[gitlost]

Unit test.

[wesleye]

Wow, thanks @gitlost for the comprehensive answer and the test.

I've combined the unit test and the updated patch. The only thing I'm woried about is that the function will fail if the passwords have been set to 0. This was also the case with the original code, so I've not added code to fix this. Is this something we want?

[wesleye]

Combined updated patch and unit test

[gitlost]

Good point, and there's a load of other empty() tests in there that have the same issue. Not sure what the correct thing to do is - I suppose change the pass1 check to do the right thing? (The others could then be fixed in a separate ticket - along with user_login, which does do an exact match but fails to do a ! isset() first, leading to a PHP warning.)

Re coding style there should be a ​space before the $update, and after opening and before closing round brackets in the errors add bit...

[wesleye]

Added additional 0 check so passwords can be set to '0' without triggering the empty error.

[chriscct7]

@wesleye There's some formatting issues on the unit test part of the patch (opening/closing braces on the functions don't follow standards), see ​https://make.wordpress.org/core/handbook/best-practices/coding-standards/php/#brace-style

[gitlost]

Version with rephrased pass1 check & code standard styled unit test.

[gitlost]

@wesleye is your IDE re-formatting stuff automatically? I've uploaded a version with code standard styled unit tests plus took the liberty of rejigging the pass1 test to be more "natural", ie follows a pattern which could be added as a function so it can be used as a drop-in replacement for empty() when testing $_REQUEST strings eg

<?php
/**
 * Drop-in substitute for PHP empty(), when testing against strings, doing exact zero-byte string match. In particular excludes '0'.
 * Returns true if val is unset, null, or '' (ie same as empty() except returns false on 0, 0.0, '0', array() and false).
 */
function wp_blank( &$val ) {
        return ! isset( $val ) || '' === $val;
}

[adamsilverstein]

I tested this patch and verified it fixes the issue (turning off JavaScript lets you test this using the normal admin screens). Before this patch, you can create users with a blank password. That seems bad.

Once the patch is applied, trying to create a user with a blank password throws an error. I also tested editing an existing user (leaving the password blank) without a problem. I also verified I can create a user with the password 0 and 000, both worked fine.

Screenshot:

[adamsilverstein]

35715.diff​ :

• Add a documentation line explaining what the check is for
• Add an additional check for blank passwords in the later conditional that checks pass1 vs. pass2, without this entering a password in field2 while leaving field 1 blank would result in a double error message:

[ocean90]

"Enter your password…" doesn't make sense here, should be "Enter a password…" I think.

Add an additional check for blank passwords in the later conditional that checks pass1 vs. pass2, without this entering a password in field2 while leaving field 1 blank would result in a double error message:

Related: #33101

[gitlost]

Also just noticed the test for ! isset( $pass1 ) isn't needed here as $pass1 is always set.

[adamsilverstein]

Replying to ocean90:

"Enter your password…" doesn't make sense here, should be "Enter a password…" I think.

@ocean90 Forgot to mention I noticed that as well and already changed that language in 35715.diff​

[ocean90]

We can probably use strlen( $pass ) here.

[adamsilverstein]

Replying to ocean90:

We can probably use strlen( $pass ) here.

good point, is that more performant or easier to understand?

[adamsilverstein]

Replying to gitlost:

Also just noticed the test for ! isset( $pass1 ) isn't needed here as $pass1 is always set.

Are we checking elsewhere? are you certain this will always be set?

[ocean90]

Replying to adamsilverstein:

Replying to ocean90:

We can probably use strlen( $pass ) here.

good point, is that more performant or easier to understand?

It catches false, null and , but not 0 or '0'. So yeah, easier to understand but also more accurate IMO.

[ocean90]

Replying to adamsilverstein:

Replying to gitlost:

Also just noticed the test for ! isset( $pass1 ) isn't needed here as $pass1 is always set.

Are we checking elsewhere? are you certain this will always be set?

$pass1 is initialized via $pass1 = $pass2 = '';. Even if you have a callback for the check_passwords which does unset( $pass1 ) it still will be set.

[ocean90]

@adamsilverstein Can you double-check 35715.2.diff​ please?

[adamsilverstein]

@ocean90 looks good, thanks for cleaning up the remaining doc blocks. I checked the functionality and everything worked fine, i see the error message if i try to use a blank password.

I did notice one unintended consequence of the second password comparison change: if i edit an existing user and enter a password only in the pass2 field, i don't get an error. Not sure if password actually updated, but should show an error.

Conditional should likely be, i tested this, patch incoming:

if ( ( $update || strlen( $pass1 ) ) && $pass1 != $pass2 ) {

[ocean90]

Replying to adamsilverstein:

I also verified I can create a user with the password 0 and 000, both worked fine.

You can't login with 0 as a password, also edit_user() won't store it because it has an empty() check. Because of this we don't need the special strlen() check, empty() would be fine.

[ocean90]

In 37059:

Users: In edit_user() check for a blank password when adding a user.

Props wesleye, gitlost, adamsilverstein.
Fixes #35715.

[ocean90]

So, after 7 weeks we're back to comment:1. :-)

Replying to wesleye:

The only thing I'm woried about is that the function will fail if the passwords have been set to 0. This was also the case with the original code, so I've not added code to fix this. Is this something we want?

This part was excluded from [37059]. Since there a few empty() checks through the codebase, including the login form, it makes sense to open a new ticket for this. Although I'm not sure if it's really worth the effort. Is there a real use case for it?

[gitlost]

Yes there are quite a few empty() checks around(!). Also found this ticket #16484 with similar debates. I doubt there's a real use case, it seems a tad pedantic. Anyway (high on the smooth running of this one) I'll open a new ticket re the PHP Notice on unset user_login on add.