WordPress.org

Make WordPress Core

Opened 13 months ago

Last modified 11 months 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:

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 12 months ago.
Updated class-wp-recovery-mode-email-service.php
47352-lkraav.diff (2.2 KB) - added by lkraav 12 months ago.
Update option name strategy (no hashing, because why?)
47352.jhoffmann.diff (684 bytes) - added by j.hoffmann 12 months 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.


12 months ago

@foack
12 months ago

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

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

#3 @foack
12 months ago

  • Keywords has-patch needs-testing added

@lkraav
12 months ago

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

#4 @foack
12 months ago

Great fix @lkraav!

#5 @lkraav
12 months 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
12 months ago

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

#6 @j.hoffmann
12 months 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
12 months ago

  • Component changed from Administration to Site Health

#8 @j.hoffmann
12 months ago

  • Component changed from Site Health to Administration

#9 @TimothyBlynJacobs
12 months 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
11 months ago

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