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

[johnbillion]

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?

[foack]

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

[foack]

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.

[lkraav]

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

[foack]

Great fix @lkraav!

[lkraav]

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]

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

[j.hoffmann]

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.

[TimothyBlynJacobs]

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.

[Clorith]

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 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

[Clorith]

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?