-
Notifications
You must be signed in to change notification settings - Fork 3.2k
#21022 Reintroduce support for passwords hashed with MD5 #8414
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
- Loading branch information
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -318,10 +318,10 @@ public function test_wp_check_password_supports_argon2id_hash() { | |
| /** | ||
| * @ticket 21022 | ||
| */ | ||
| public function test_wp_check_password_does_not_support_md5_hashes() { | ||
| public function test_wp_check_password_supports_md5_hash() { | ||
| $password = 'password'; | ||
| $hash = md5( $password ); | ||
| $this->assertFalse( wp_check_password( $password, $hash ) ); | ||
| $this->assertTrue( wp_check_password( $password, $hash ) ); | ||
| $this->assertSame( 1, did_filter( 'check_password' ) ); | ||
| } | ||
|
|
||
|
|
@@ -363,8 +363,6 @@ public function test_wp_check_password_does_not_support_empty_password( $value ) | |
|
|
||
| public function data_empty_values() { | ||
| return array( | ||
| // Integer zero: | ||
| array( 0 ), | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we force $hash in wp_check_password to be a string, this can be restored.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, although I'm not sure why I added that test value in the first place (it was only introduced in r59828). There is no code path in core that will result in something other than a string being passed to either the My preference would be to not add a string check or string coercion to the function, and allow PHP to continue triggering the appropriate warning depending on what is passed. What do you think?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No objections from me |
||
| // String zero: | ||
| array( '0' ), | ||
| // Zero-length string: | ||
|
|
@@ -1079,6 +1077,42 @@ public function test_phpass_password_is_rehashed_after_successful_user_password_ | |
| $this->assertSame( self::$user_id, $user->ID ); | ||
| } | ||
|
|
||
| /** | ||
| * @dataProvider data_usernames | ||
| * | ||
| * @ticket 21022 | ||
| */ | ||
| public function test_md5_password_is_rehashed_after_successful_user_password_authentication( $username_or_email ) { | ||
| $password = 'password'; | ||
|
|
||
| // Set the user password with the old md5 algorithm. | ||
| self::set_user_password_with_md5( $password, self::$user_id ); | ||
|
|
||
| // Verify that the password needs rehashing. | ||
| $hash = get_userdata( self::$user_id )->user_pass; | ||
| $this->assertTrue( wp_password_needs_rehash( $hash, self::$user_id ) ); | ||
|
|
||
| // Authenticate. | ||
| $user = wp_authenticate( $username_or_email, $password ); | ||
|
|
||
| // Verify that the md5 password hash was valid. | ||
| $this->assertNotWPError( $user ); | ||
| $this->assertInstanceOf( 'WP_User', $user ); | ||
| $this->assertSame( self::$user_id, $user->ID ); | ||
|
|
||
| // Verify that the password no longer needs rehashing. | ||
| $hash = get_userdata( self::$user_id )->user_pass; | ||
| $this->assertFalse( wp_password_needs_rehash( $hash, self::$user_id ) ); | ||
|
|
||
| // Authenticate a second time to ensure the new hash is valid. | ||
| $user = wp_authenticate( $username_or_email, $password ); | ||
|
|
||
| // Verify that the bcrypt password hash is valid. | ||
| $this->assertNotWPError( $user ); | ||
| $this->assertInstanceOf( 'WP_User', $user ); | ||
| $this->assertSame( self::$user_id, $user->ID ); | ||
| } | ||
|
|
||
| /** | ||
| * @dataProvider data_usernames | ||
| * | ||
|
|
@@ -1772,6 +1806,38 @@ private static function set_user_password_with_phpass( string $password, int $us | |
| clean_user_cache( $user_id ); | ||
| } | ||
|
|
||
| /** | ||
| * Test the tests | ||
| * | ||
| * @covers Tests_Auth::set_user_password_with_md5 | ||
| * | ||
| * @ticket 21022 | ||
| */ | ||
| public function test_set_user_password_with_md5() { | ||
| $password = 'password'; | ||
|
|
||
| // Set the user password with the old md5 algorithm. | ||
| self::set_user_password_with_md5( $password, self::$user_id ); | ||
|
|
||
| // Ensure the password is hashed with md5. | ||
| $hash = get_userdata( self::$user_id )->user_pass; | ||
| $this->assertSame( md5( $password ), $hash ); | ||
| } | ||
|
|
||
| private static function set_user_password_with_md5( string $password, int $user_id ) { | ||
| global $wpdb; | ||
|
|
||
| $wpdb->update( | ||
| $wpdb->users, | ||
| array( | ||
| 'user_pass' => md5( $password ), | ||
| ), | ||
| array( | ||
| 'ID' => $user_id, | ||
| ) | ||
| ); | ||
| clean_user_cache( $user_id ); | ||
| } | ||
|
|
||
| /** | ||
| * Test the tests | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since
hash_equalsrequires a string as the first argument, would it make sense to add a check or force it to be a string here?