WordPress.org

Make WordPress Core

Opened 21 months ago

Closed 8 months ago

#25853 closed defect (bug) (fixed)

Changeset 25696 breaks expected value of argument sent to filter 'retrieve_password_message'

Reported by: dcavins Owned by: johnbillion
Milestone: 4.1 Priority: normal
Severity: normal Version: 3.7
Component: Users Keywords:
Focuses: Cc:

Description (last modified by SergeyBiryukov)

In changeset [25696] to wp-login.php, the function 'retrieve_password' was changed to hash the generated key about line 350:

$hashed = $wp_hasher->HashPassword( $key );

However, the filter 'retrieve_password_message' is still sending $key as an argument, line 385

$message = apply_filters( 'retrieve_password_message', $message, $key );

So any existing filters are no longer receiving the value stored in the database (which matters because filtering 'retrieve_password_message' almost has to include a search on that value to get the requestor's user_login, which is required for the password reset link to work).

A simple fix is changing line 385 to

$message = apply_filters( 'retrieve_password_message', $message, $hashed );

Thanks for the great software!

Attachments (3)

wp-login.diff (669 bytes) - added by ivankristianto 11 months ago.
Patch 'retrieve_password_message' to include $user_login
25853.patch (919 bytes) - added by dcavins 9 months ago.
Adds user_login and WP_User object to 'retrieve_password_message' filter
25853.2.patch (1.2 KB) - added by DrewAPicture 9 months ago.
@since

Download all attachments as: .zip

Change History (16)

comment:1 @SergeyBiryukov21 months ago

  • Component changed from General to Users
  • Description modified (diff)
  • Milestone changed from Awaiting Review to 3.7.2
  • Version changed from 3.7.1 to 3.7

comment:2 @shawnhooper20 months ago

I see a potential problem with this recommendation. Changing $key to $hashed resolves the suggested use case of looking up user's information from the database (which stores the hashed activation key).

However, any plugins that would use this filter to re-write the activation message need to include the key in plain-text for inclusion in the activation link. Since the key cannot be obtained from the stored hash, it has to be passed to the filter from here.

Recommend adding the hash as another parameter:

$message = apply_filters( 'retrieve_password_message', $message, $key, $hashed );

so that anyone using this filter can make use of both the hashed and unhashed keys as needed.

comment:3 @shawnhooper20 months ago

  • Cc shawnhooper added
  • Keywords 2nd-opinion added

comment:4 @dcavins20 months ago

Good point. I missed that the password reset uri includes the un-hashed key. Thanks for correcting my mistake.

comment:5 follow-up: @nacin20 months ago

  • Milestone changed from 3.7.2 to Future Release

Right, I figured that sending the existing plain-text key through the filter would break the least. Happy to add the hashed value as a new argument.

I don't think this actually broke anything. Did it? Anyone doing raw queries on the DB is going to have a bad time with this change no matter what. This was about the email message being sent, not any complex operations.

I'm OK with passing the hashed value, but it seems like passing the user's login (or rather, ID or a user object) might make more sense.

comment:6 in reply to: ↑ 5 @dcavins20 months ago

Replying to nacin:
Yes, including the user_login or user object would be an improvement over the earlier version of the filter args. The two pieces of information that are necessary to create the password recovery link are user_login and the key value. So any combination of arguments that can make that happen are enough. But if you're only given the key value, you can't easily get the user_login.

Thanks!

Right, I figured that sending the existing plain-text key through the filter would break the least. Happy to add the hashed value as a new argument.

I don't think this actually broke anything. Did it? Anyone doing raw queries on the DB is going to have a bad time with this change no matter what. This was about the email message being sent, not any complex operations.

I'm OK with passing the hashed value, but it seems like passing the user's login (or rather, ID or a user object) might make more sense.

comment:7 @ivankristianto11 months ago

I also having similar issue with this.
I would like to change the email message formatting, but i cannot find the $user_login because it is not passed.
And the password reset url need the $key and the $user_login.

Right now to fix this, i forcefully change it (at least for now to make it working, i'm sorry...):

$message = apply_filters( 'retrieve_password_message', $message, $key, $user_login );

i submit the patch so it is included in the future release.

@ivankristianto11 months ago

Patch 'retrieve_password_message' to include $user_login

comment:8 @ivankristianto10 months ago

  • Severity changed from minor to normal

Anyone check on this?

comment:9 @dashaluna9 months ago

Any news on this?

comment:10 @johnbillion9 months ago

  • Keywords needs-docs added; 2nd-opinion removed
  • Milestone changed from Future Release to 4.1

Needs an inline doc for the new parameter on the filter, then this can go in.

@dcavins9 months ago

Adds user_login and WP_User object to 'retrieve_password_message' filter

comment:11 @dcavins9 months ago

I've attached a new patch. In it, I added $user_login and $user_data (the WP_User object) to the filter. It made sense to add the login for parity with the earlier filters in this function: retrieve_password and retrieve_password_key, but I also liked @nacin's suggestion of sending the WP_User object, because that covers all the bases.

Thanks!

@DrewAPicture9 months ago

@since

comment:12 @DrewAPicture9 months ago

  • Keywords needs-docs removed

comment:13 @johnbillion8 months ago

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

In 30357:

Add $user_login and $user_data parameters to the retrieve_password_message filter.

Props ivankristianto, dcavins
Fixes #25853

Note: See TracTickets for help on using tickets.