Make WordPress Core

Opened 2 years ago

Closed 22 months ago

Last modified 21 months ago

#54984 closed defect (bug) (fixed)

wp_update_user doesn't work properly with current user instance

Reported by: oztaser's profile oztaser Owned by: peterwilsoncc's profile peterwilsoncc
Milestone: 6.1 Priority: normal
Severity: normal Version: 5.8
Component: Users Keywords:
Focuses: Cc:

Description (last modified by costdev)

After #28020 merged in 5.8, there are two errors that I noticed in wp_update_user.

  1. Storing plain password:

If user object to be updated is getting by get_userdata (with current user id) or wp_get_current_user, wp_update_user doesn't hash the new password.

Also doesn't send password change email.

  1. Doesn't send Email change email.

Here is my mu-plugin to produce the errors:

<?php
namespace NefisYemekTarifleri\Test_User_Password_Hash;

add_action( 'init', __NAMESPACE__ . '\\init' );
function init() {
        global $wpdb;

//      $user             = get_userdata( 1 );
        $user             = wp_get_current_user();
        $user->user_pass  = '123456';

        wp_update_user( $user );
}

I think it's not the best way to change password using wp_update_user, but if wp_update_user supports it, password should be hashed.

P.S: I couldn't find any related ticket about this problem, sorry if there is.

Attachments (1)

54984.diff (813 bytes) - added by peterwilsoncc 2 years ago.

Download all attachments as: .zip

Change History (13)

#1 @costdev
2 years ago

  • Description modified (diff)

#2 @SergeyBiryukov
2 years ago

  • Milestone changed from Awaiting Review to 6.0

Hi there, welcome to WordPress Trac! Thanks for the report.

Moving to 6.0 for investigation.

#3 @ravipatel
2 years ago

wp_update_user() has a condition to convert a password in hash.

https://github.com/WordPress/WordPress/blob/5.9-branch/wp-includes/user.php

if ( ! empty( $userdata['user_pass'] ) && $userdata['user_pass'] !== $user_obj->user_pass ) {
		// If password is changing, hash it now.
		$plaintext_pass        = $userdata['user_pass'];
		$userdata['user_pass'] = wp_hash_password( $userdata['user_pass'] );

		/**
		 * Filters whether to send the password change email.
		 *
		 * @since 4.3.0
		 *
		 * @see wp_insert_user() For `$user` and `$userdata` fields.
		 *
		 * @param bool  $send     Whether to send the email.
		 * @param array $user     The original user array.
		 * @param array $userdata The updated user array.
		 */
		$send_password_change_email = apply_filters( 'send_password_change_email', true, $user, $userdata );
	}
Last edited 2 years ago by ravipatel (previous) (diff)

@peterwilsoncc
2 years ago

#4 @peterwilsoncc
2 years ago

Thanks for providing an mu-plugin as a proof of concept, @oztaser, it was a great help.

The bug seems to be caused by the combination of two things:

  • objects are passed by reference
  • since 5.8 WP has used the same user object for the current user whenever possible.

The result is that in this block of code, the old user data object is no such thing: it's simply a reference to the modified object that has been passed to the function.

<?php

// In `wp_update_user()`

// First, get all of the original fields.
$user_obj = get_userdata( $user_id );
if ( ! $user_obj ) {
        return new WP_Error( 'invalid_user_id', __( 'Invalid user ID.' ) );
}

reference


In 54984.diff I've worked around this by creating a new WP_User object for the purpose of obtaining the old data.

However, I am not sure this is an ideal solution as get_userdata() and related functions are pluggable so there is no way of knowing if the data returned by new WP_User( $user_id ) is what is returned by get_userdata().

A technique to force an uncached copy of the user data is what is needed to fix this though.

#5 follow-up: @dd32
2 years ago

However, I am not sure this is an ideal solution as get_userdata() and related functions are pluggable so there is no way of knowing if the data returned by new WP_User( $user_id ) is what is returned by get_userdata().

Thinking out-loud here: If we're only expecting a WP_User object in these places/globals, and since we've got a WP_User::__set() that takes care of $user->user_pass = 12345;, WP_User could create a copy of the user-data prior to altering fields values and a WP_User::has_changed( 'user_pass' ) could be used to detect if the object has changed / what fields have changed.

That doesn't take care of a situation where $user->data->user_pass is changed directly though.

So perhaps 54984.diff is indeed the ideal way forward.

However, I am not sure this is an ideal solution as get_userdata() and related functions are pluggable so there is no way of knowing if the data returned by new WP_User( $user_id ) is what is returned by get_userdata().

Correct me if I'm wrong, but in those cases, this could've previously been an issue too, right?

#6 @costdev
2 years ago

  • Version set to 5.8

#7 in reply to: ↑ 5 @peterwilsoncc
2 years ago

  • Milestone changed from 6.0 to 6.1

Replying to dd32:

So perhaps 54984.diff is indeed the ideal way forward.

However, I am not sure this is an ideal solution as get_userdata() and related functions are pluggable so there is no way of knowing if the data returned by new WP_User( $user_id ) is what is returned by get_userdata().

Correct me if I'm wrong, but in those cases, this could've previously been an issue too, right?

Reviewing the code, yes. new WP_User() is called directly fairly frequently so one more call won't make a difference.

As RC1 is later this week, I'll move this to the 6.1 milestone to be actioned during the next cycle.

#8 @tykoted
23 months ago

However, I am not sure this is an ideal solution as get_userdata() and related functions are pluggable so there is no way of knowing if the data returned by new WP_User( $user_id ) is what is returned by get_userdata().

I would say this can be the ideal solution as well. The fix will not touch the get_userdata() which is used in other areas, so there’s no risk of breaking existing functionality.

#9 @cu121
22 months ago

Hello,

I have thoroughly gone through this issue and inspected the functions related to wp_update_user(), and the reason why the password is not hashed when modifying the property of the same instance of the WP_User object is that when the WP_User user_pass is modified, the function get_user_by() generally fetches the user saved data and it checks if the WP_User object is the current instance or not. If it is the same instance then it returns the same WP_User instance $current_user, which is why the fetched database data of the user is not returned.

I assume the developer who has worked with this logic has done this purposely and from my standpoint, it is absolutely valid. I assume we have to use wp_hash_password() when we want to modify the password of the current WP_User instance when accessing the user_pass property of the current user object.

This is where the main logic lies by returning the same instance of the WP_User https://prnt.sc/2Go8mmctOKfw. Feel free to post any feedbacks

Thank You

Last edited 22 months ago by cu121 (previous) (diff)

#10 @peterwilsoncc
22 months ago

  • Owner set to peterwilsoncc
  • Status changed from new to assigned

Thanks @cu121, I think I'll revert the initial commit ([50790] for #28020) for the time being.

The performance gain of the earlier commit is negligible and while testing this issue I discovered a proper fix would introduce database calls invalidating the earlier performance gain.

#11 @peterwilsoncc
22 months ago

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

In 54397:

Users: Revert use of shared objects for current user.

Reverts [50790].

Props oztaser, ravipatel, dd32, costdev, SergeyBiryukov, tykoted, cu121, xknown.
Fixes #54984.

#12 @SergeyBiryukov
21 months ago

In 54544:

Users: Revert use of shared objects for current user.

Reverts [50790].

Props oztaser, ravipatel, dd32, costdev, SergeyBiryukov, tykoted, cu121, xknown.
Merges [54397] to the 6.0 branch.
Fixes #54984.

Note: See TracTickets for help on using tickets.