WordPress.org

Make WordPress Core

Opened 6 years ago

Closed 6 years ago

Last modified 6 years ago

#35715 closed defect (bug) (fixed)

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

Reported by: gitlost Owned by: ocean90
Milestone: 4.5 Priority: normal
Severity: normal Version: 4.4
Component: Users Keywords: good-first-bug has-patch has-screenshots
Focuses: Cc:

Description

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.

Attachments (8)

35715.patch (1.2 KB) - added by wesleye 6 years ago.
Patch
35715_tests.diff (2.2 KB) - added by gitlost 6 years ago.
Unit test.
35715_combined.diff (2.9 KB) - added by wesleye 6 years ago.
Combined updated patch and unit test
35715_combined_emptycheck.diff (2.9 KB) - added by wesleye 6 years ago.
Added additional 0 check so passwords can be set to '0' without triggering the empty error.
35715_combined_emptycheck.2.diff (3.2 KB) - added by gitlost 6 years ago.
Version with rephrased pass1 check & code standard styled unit test.
35715.diff (3.3 KB) - added by adamsilverstein 6 years ago.
35715.2.diff (4.0 KB) - added by ocean90 6 years ago.
35715.3.diff (4.1 KB) - added by adamsilverstein 6 years ago.

Download all attachments as: .zip

Change History (36)

#1 @gitlost
6 years ago

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

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

#2 follow-up: @johnbillion
6 years ago

  • Keywords needs-patch needs-testing good-first-bug added
  • Milestone changed from Awaiting Review to Future Release

@wesleye
6 years ago

Patch

#3 in reply to: ↑ 2 @wesleye
6 years ago

  • Keywords has-patch added; needs-patch removed

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

#4 @gitlost
6 years ago

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
6 years ago

Unit test.

#5 @SergeyBiryukov
6 years ago

  • Milestone changed from Future Release to 4.5
  • Owner set to SergeyBiryukov
  • Status changed from new to reviewing

#6 @wesleye
6 years ago

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
6 years ago

Combined updated patch and unit test

#7 @gitlost
6 years ago

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
6 years ago

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

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


6 years ago

#9 @chriscct7
6 years ago

@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
6 years ago

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

#10 @gitlost
6 years ago

@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;
}


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


6 years ago

#12 follow-up: @adamsilverstein
6 years ago

  • Keywords has-screenshots added; needs-testing removed

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:
http://cl.ly/432d3F1H0n3y/Add_New_User__10up_BUC__WordPress_2016-03-22_10-15-33.jpg

#13 @adamsilverstein
6 years ago

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:

http://cl.ly/2a0x2f132b3p/Add_New_User__10up_BUC__WordPress_2016-03-22_10-27-53.jpg

#14 @adamsilverstein
6 years ago

  • Keywords commit added

#15 follow-up: @ocean90
6 years ago

  • Keywords commit removed

"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

#16 follow-up: @gitlost
6 years ago

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

#17 in reply to: ↑ 15 @adamsilverstein
6 years ago

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

#18 follow-up: @ocean90
6 years ago

  • Owner changed from SergeyBiryukov to ocean90
  • Status changed from reviewing to accepted

We can probably use strlen( $pass ) here.

#19 in reply to: ↑ 18 ; follow-up: @adamsilverstein
6 years ago

Replying to ocean90:

We can probably use strlen( $pass ) here.

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

#20 in reply to: ↑ 16 ; follow-up: @adamsilverstein
6 years ago

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?

#21 in reply to: ↑ 19 @ocean90
6 years ago

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.

#22 in reply to: ↑ 20 @ocean90
6 years ago

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
6 years ago

#23 @ocean90
6 years ago

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

#24 @adamsilverstein
6 years ago

@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 ) {

#25 in reply to: ↑ 12 @ocean90
6 years ago

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.

#26 @ocean90
6 years ago

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

In 37059:

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

Props wesleye, gitlost, adamsilverstein.
Fixes #35715.

#27 @ocean90
6 years ago

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?

#28 @gitlost
6 years ago

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.

Note: See TracTickets for help on using tickets.