Make WordPress Core

Opened 9 years ago

Closed 8 years ago

Last modified 8 years ago

#28435 closed defect (bug) (fixed)

wp_update_user breaks when passed WP_User instance

Reported by: rmccue's profile rmccue Owned by: wonderboymusic's profile wonderboymusic
Milestone: 4.4 Priority: normal
Severity: normal Version:
Component: Users Keywords: has-patch
Focuses: Cc:

Description (last modified by rmccue)

Supposedly fixed in #21429. (Tangential to #28019)

If you pass a WP_User instance to wp_update_user, it first calls WP_User::to_array, which returns the user data from the DB. This is then treated as the input data.

The problem then is that $userdata['user_pass'] is always set, as it's always included in $userdata. This then gets double-hashed by wp_update_user. (wp_update_user will then update the cookies, so the user won't notice until they're logged out)

To reproduce:

<?php
$testuserid = 1;

$user = get_userdata( $testuserid );

echo 'Before: ' . $user->user_pass;

wp_update_user( $user );

// Reload the data
$user = get_userdata( $testuserid );

echo 'After: ' . $user->user_pass;

Current output:

Before: $P$BDqB8PmujqwtUNqnDW/aiQKuAEvm741
After: $P$BsqV0Lkka4QIWE9RaveZ49wvOMnHD//

This operation should have been a no-op, but isn't.

(Ticket previously stated this happens with wp_insert_user; this is incorrect, as wp_insert_user doesn't hash the password for updates.)

Attachments (4)

28435.diff (1.2 KB) - added by tbcorr 9 years ago.
.diff file for ticket 28435
28435-unit-test.diff (676 bytes) - added by salcode 9 years ago.
Unit test
28435.2.diff (1.3 KB) - added by tbcorr 9 years ago.
28435.3.diff (1.2 KB) - added by wonderboymusic 8 years ago.

Download all attachments as: .zip

Change History (12)

#1 @rmccue
9 years ago

  • Description modified (diff)
  • Summary changed from wp_insert_user/wp_update_user break when passed WP_User instance to wp_update_user breaks when passed WP_User instance

@tbcorr
9 years ago

.diff file for ticket 28435

#2 @tbcorr
9 years ago

Contribution made during WordCamp Philly 2014 during contributor day.

#3 @tbcorr
9 years ago

  • Keywords has-patch added

@salcode
9 years ago

Unit test

@tbcorr
9 years ago

#4 @tbcorr
9 years ago

Updated path of diff file so it can be applied from the root directory. (my first patch...)

#5 @wonderboymusic
8 years ago

  • Owner set to wonderboymusic
  • Resolution set to fixed
  • Status changed from new to closed

In 35116:

Users: when passing a WP_User instance to wp_update_user(), ensure that the user password is not accidentally double-hashed. This is terrifying.

Adds unit tests.

Props tbcorr, salcode.
Fixes #28435.

#6 @netweb
8 years ago

  • Milestone changed from Awaiting Review to 4.4

#7 @danielhomer
8 years ago

Shouldn't this be considered for 4.3.2? We use wp_update_user() in an author migration script and I nearly blatted over a hundred user passwords because of this bug, not to mention the spam from the email notifications.

#8 @SergeyBiryukov
8 years ago

In 35732:

Users: Move the tests added in [35116] and [35618] to a more appropriate place and give them a better name.

See #28435, #29880.

Note: See TracTickets for help on using tickets.