Make WordPress Core

Opened 5 years ago

Last modified 19 months ago

#47352 new defect (bug)

Take into account the current admin email address when rate limiting the recovery mode email

Reported by: johnbillion's profile johnbillion Owned by:
Milestone: Future Release Priority: normal
Severity: normal Version: 5.2
Component: Site Health Keywords: servehappy dev-feedback has-patch needs-testing
Focuses: Cc:

Description

Here's a process which I've seen occur twice in the last few days:

  • A change to a site was deployed and a fatal error gets triggered somewhere.
  • The recovery mode email was sent out.
  • The developer checks the current value of the admin email address and discovers it belongs to someone who left the company years ago.
  • They change the admin email address to their own email address and re-trigger the fatal error, but the recovery mode email doesn't get re-sent to the new address because there's a one day rate limit in place.

This prevents the user from enabling recovery mode for at least a day.

The option that acts as the "last sent" record for the recovery mode email (recovery_mode_email_last_sent) should take into account the admin email address, for example by hashing it and including it in the option key.

Aside: Is there a reason an option is used instead of a transient?

Attachments (3)

47352.diff (1.6 KB) - added by foack 5 years ago.
Updated class-wp-recovery-mode-email-service.php
47352-lkraav.diff (2.2 KB) - added by lkraav 5 years ago.
Update option name strategy (no hashing, because why?)
47352.jhoffmann.diff (684 bytes) - added by j.hoffmann 5 years ago.
Just informative solution of deleting the rate limit option upon changing the admin email option.

Download all attachments as: .zip

Change History (16)

This ticket was mentioned in Slack in #core-php by lkraav. View the logs.


5 years ago

@foack
5 years ago

Updated class-wp-recovery-mode-email-service.php

#2 @foack
5 years ago

Just wrote a quick fix for this, introducing an option field that stores a hashed version of the email address that received the last recovery email. The rate limit is then ignored if that hash and the hash of the current admin email do not match.

Another way to solve this would be to reset the rate limit when updating the admin email address as proposed by @jhoffmann.

Last edited 5 years ago by foack (previous) (diff)

#3 @foack
5 years ago

  • Keywords has-patch needs-testing added

@lkraav
5 years ago

Update option name strategy (no hashing, because why?)

#4 @foack
5 years ago

Great fix @lkraav!

#5 @lkraav
5 years ago

My alternative simply makes every admin e-mail associated option uniquely named, so they're all valid simultaneously.

I don't know if users reverting admin e-mails back to previous state is much of a use case... might be, due to something not working out with the new e-mail address, or whatever.

Downside here is that pre-change option row would stick around for life. Putting time into cleanup functionality doesn't seem worth it... or?

@j.hoffmann
5 years ago

Just informative solution of deleting the rate limit option upon changing the admin email option.

#6 @j.hoffmann
5 years ago

As @foack mentioned my suggestion was to erase the option, which saves when the last mail was sent, every time the admin email option gets changed.

I added my patch just for informative reasons as I favor @lkraav's solution as it can handle multiple recipients independently in case this would get introduced in the future.

#7 @j.hoffmann
5 years ago

  • Component changed from Administration to Site Health

#8 @j.hoffmann
5 years ago

  • Component changed from Site Health to Administration

#9 @TimothyBlynJacobs
5 years ago

I personally prefer @foack's patch for BC reasons. Clearing the existing option will continue to work for people. When doing it as a separate option there wouldn't be a need to hash the email.


@lkraav the email would have to be hashed so it can be guaranteed to fit within the option name column.

#10 @spacedmonkey
5 years ago

  • Component changed from Administration to Site Health

#11 @Clorith
20 months ago

  • Milestone changed from Awaiting Review to 6.1

I'm also leaning towards 47352.diff as the better implementation here, as it also won't potentially create multiple wp_options entries (since these are not transients, each one would need to be cleaned up somehow).

This ticket was mentioned in PR #3279 on WordPress/wordpress-develop by Clorith.


19 months ago
#12

_This PR takes the patches already submitted to the ticket, refines them slightly, and puts them in a nice PR for testing purposes.'_

The recipient of any recovery mode email is stored to an options entry, recovery_mode_email_last_recipient, which is accessible via the WP_Recovery_Mode_Email_Service::RECOVERY_RECIPIENT_OPTION variable.

When a new email may need to be sent, the previous recipient is compared to the currently defined recipient, allowing the admin email, or RECOVERY_MODE_EMAIL constant to be updated with a new user, and a new recovery mode email to go out without waiting for the normal 24 hour rate limit.

Trac ticket: https://core.trac.wordpress.org/ticket/47352

#13 @Clorith
19 months ago

  • Milestone changed from 6.1 to Future Release

I had initially planned for this to go in with 6.1, but some more considerations came up in testing, which is not currently accounted for.

I opted not to hash the email, there's nothing secret here, and if anything it can be helpful to know where the email was sent if you need to look up the last recipient for whatever reason.

My main concern came from looking over the dev notes from when the fatal error recovery mode was introduced, and folks wanting to customize the recipient, where they would filter the $email variable, which contains the recipient, subject, etc (an array of all the pieces that go into an email), and change the recipient. With the current implementation, this would trigger the recovery mode email to be sent out every single time, since the email would never match what the core function expects (from the get_recovery_mode_email_address() function), and what actually ends up in the $email['to'] array key.

To resolve this, core would need to introduce a filter in the get_recovery_mode_email_address() function instead, allowing for the recipient to be changed in the canonical way of fetching who should get the emails, that way the function can be used for comparison later down the line "safely" (any code that modified the $email['to'] would need to be updated, to not cause a lot of recovery emails going out, but they would still work, and I think we need a proper way for these to change the recipient before finalizing this.

Are there any scenarios I may have missed in my above outline, or different implementations we should instead consider?

Note: See TracTickets for help on using tickets.