Make WordPress Core

Opened 3 years ago

Last modified 3 years ago

#47352 new defect (bug)

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

Reported by: johnbillion Owned by:
Milestone: Awaiting Review Priority: normal
Severity: normal Version: 5.2
Component: Site Health Keywords: servehappy dev-feedback has-patch needs-testing
Focuses: Cc:


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

Download all attachments as: .zip

Change History (13)

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

3 years ago

3 years ago

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

#2 @foack
3 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 3 years ago by foack (previous) (diff)

#3 @foack
3 years ago

  • Keywords has-patch needs-testing added

3 years ago

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

#4 @foack
3 years ago

Great fix @lkraav!

#5 @lkraav
3 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?

3 years ago

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

#6 @j.hoffmann
3 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
3 years ago

  • Component changed from Administration to Site Health

#8 @j.hoffmann
3 years ago

  • Component changed from Site Health to Administration

#9 @TimothyBlynJacobs
3 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
3 years ago

  • Component changed from Administration to Site Health
Note: See TracTickets for help on using tickets.