WordPress.org

Make WordPress Core

Opened 6 months ago

Last modified 5 months ago

#25853 new defect (bug)

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

Reported by: dcavins Owned by:
Milestone: Future Release Priority: normal
Severity: minor Version: 3.7
Component: Users Keywords: 2nd-opinion
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!

Change History (6)

comment:1 SergeyBiryukov6 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 shawnhooper5 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 shawnhooper5 months ago

  • Cc shawnhooper added
  • Keywords 2nd-opinion added

comment:4 dcavins5 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: nacin5 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 dcavins5 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.

Note: See TracTickets for help on using tickets.