WordPress.org

Make WordPress Core

Opened 11 months ago

Last modified 5 weeks ago

#47192 new enhancement

Allow users to enter recovery mode via their registered email

Reported by: spacedmonkey Owned by:
Milestone: Awaiting Review Priority: normal
Severity: normal Version: 5.2
Component: Site Health Keywords: servehappy has-patch
Focuses: Cc:

Description

In WordPress 5.2 recovery mode was added. Recovery mode is entered when by clicking on a special link sent in an email. The email is sent to the admin_email which is stored as an option. However, the current manager / administrator may not have access to this email inbox.

Users with the activate_plugins capability should be able to request a recovery link sent the user's register email via form, similar to the forgotten password form. This request recovery link form, should always be available (default recovery mode on this single page). The unique recovery link key, should likely be stored in user meta with the possibility to work with multisite setups in the future.

Attachments (5)

47192.diff (9.8 KB) - added by spacedmonkey 11 months ago.
47192.1.diff (9.5 KB) - added by spacedmonkey 10 months ago.
47192.2.diff (7.4 KB) - added by TimothyBlynJacobs 3 months ago.
Request Recovery Mode Link.png (217.2 KB) - added by TimothyBlynJacobs 3 months ago.
47192.3.diff (9.3 KB) - added by TimothyBlynJacobs 3 months ago.

Download all attachments as: .zip

Change History (24)

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


11 months ago

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


11 months ago

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


11 months ago

#4 @fierevere
11 months ago

It will be fine to store and show error message in recovery mode too, as some users are unable to get email with recovery mode link and error details.

@spacedmonkey
11 months ago

#5 @spacedmonkey
11 months ago

  • Keywords has-patch added; needs-patch removed

I have uploaded a first patch. Completely untested, but I wanted to get the ball rolling.

I know @TimothyBlynJacobs had some security issue with this idea, like him to look at this first patch to see if he can see any issue.

#6 @TimothyBlynJacobs
11 months ago

I think its tough to discuss the possible security ramifications without a working patch. Off the bat, I don't see the same timing related issue because it looks like the permissions check is happening at a normal time. However, forcing recovery mode like this does worry me. But again, hard to say without digging into it.


As an aside, we shouldn't expose the email service. That is an implementation detail of the recovery mode controller. Instead, the request actions should probably be processed inside WP_Recvoery_Mode so it can pass the selected email address to maybe_send_recovery_mode_email.

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


10 months ago

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


10 months ago

#9 @spacedmonkey
10 months ago

  • Component changed from General to Site Health

#10 @spacedmonkey
10 months ago

I have uploaded a functional patch. Barring some formatting of code, I think this patch has legs.

#11 @melsteel
7 months ago

Hi Sir,@spacedmonkey

This looks great, is this finished now? How can we use it?

Yesterday when I performed a normal update in the dashboard some plugins caused fatal errors to lock me out of dashboard, then I used the link sent to my email but accidentally closed my browser when I just opened the link, then when I try to open that link again the site wouldn't initiate the recovery mode, is this because of the link will only valid for the first time?

Because my hosing provider wouldn't provide me the FTP access account don't know why, so my site was offline for 4 hours until she did a restore to a version backed up in the previous day, so basically when she do the restore I also lost some data after that backup.

So if this is finished it will be a big help for situation like I just had, I can't rename the plugin folders to deactivate them and I couldn't use that link sent to me, so with this function you are developing I could just send a new link to me again and it will save me a lot of trouble talking to the hosting provider.

Great work! Thanks for sharing.

#12 @TimothyBlynJacobs
3 months ago

#47319 was marked as a duplicate.

#13 @TimothyBlynJacobs
3 months ago

@spacedmonkey, @dd32, @miss_jwo and I discussed this feature at WordCamp US to try and find a way forward without introducing any security issues.

The crux of the issue is that for the request an email form to work when the site is experiencing a fatal error, execution needs to be handled before plugins are loaded. This poses a problem because we don't want any user to be able to request a link to enter recovery mode, only users who can resume_plugins or resume_themes. Doing permission checks before WordPress has been able to load plugins may not be safe.

The solution the four of us came up with is to "cache" the list of email addresses that have permission to perform this action, and when the link is requested, check if the provided email address is contained within the allow list. That list is then updated when a user logs in, has a role changed, or the user is updated.

When a user requests a recovery mode link, we no longer have access to the fatal error that occurred and the context of the page is different. Additionally, we no longer need to rate limit it since the email isn't sent automatically on an error. So I've introduced a second email method that has less content.

The UI is currently all handled using wp_die(). We can't easily use the wp-login.php styles because we'd have to wait for plugins to load to gain access to the login_header() and login_footer() functions. However, if we wanted to, I suppose we could move those functions to a separate file that can be selectively included. If we did reuse the login styles, though, we might run into an issue that site owners won't be able to easily style the page since plugins like Theme My Login wouldn't have run yet.

I've uploaded a patch that does this for people to play around with. The styling and language is all very primitive, but it is hopefully enough to get an idea of how it might work.

Visit /wp-login.php?action=request_rm to see the page and initiate the flow.

Howdy!

You requested a link to enter Recovery Mode.

First, visit your website (http://trunk.test/) and check for any visible issues. Next, visit the page where the error was caught and check for any visible issues.

Please contact your host for assistance with investigating this issue further.

If your site appears broken and you can't access your dashboard normally, WordPress now has a special "recovery mode". This lets you safely login to your dashboard and investigate further.

http://trunk.test/wp-login.php?action=enter_recovery_mode&rm_token=04TVkFeEt9aGSlC1B0ZXei&rm_key=5JmVtMgiEuYMno6PuIxxRI

To keep your site safe, this link will expire in 1 day. Don't worry about that, though: you can request a new link at any time.

This ticket was mentioned in Slack in #core-site-health by timothybjacobs. View the logs.


3 months ago

#15 @TimothyBlynJacobs
3 months ago

I've forgotten something, naturally. I think we'd link to this page from the current error template as well as from the automatic email.

This ticket was mentioned in Slack in #core by timothybjacobs. View the logs.


3 months ago

#17 @spacedmonkey
3 months ago

The solution the four of us came up with is to "cache" the list of email addresses that have permission to perform this action, and when the link is requested, check if the provided email address is contained within the allow list. That list is then updated when a user logs in, has a role changed, or the user is updated.

This wasn't what we agreed on. Caching a list of users like this has some serious performance issues. What if the site has 10k work of admin on it. That option would be massive and fill the options table with a lot of data. Specially as this will likely be autoloaded on every page, this would have a big effect on performance.

What I thought was agree was to hook into login / user edit, to check if user has access to recovery mode and save a cache in user meta. This way, we should be able to trust user meta over capability check.

I also do like the user of wp_die here. It seem out of place and confusing. As much of I hate using wp-login.php styling, as it hard to work with, it is much less confusing to a user.

I am going to add some stuff to my original patch and submit a PR to github, so it is easier to review.

#18 @TimothyBlynJacobs
3 months ago

This wasn't what we agreed on.

My mistake, I thought we were talking about a defined list of emails.

What I thought was agree was to hook into login / user edit, to check if user has access to recovery mode and save a cache in user meta. This way, we should be able to trust user meta over capability check.

I don't have any issue with moving to user meta. I think you're right it makes more sense.

I also do like the user of wp_die here. It seem out of place and confusing.

Yeah, my use of wp_die() here was mainly to be used as a convenient way to scaffold HTML for a prototype. I think once we have a design, we'd need to switch to an alternate templating mechanism of some kind.

As much of I hate using wp-login.php styling, as it hard to work with, it is much less confusing to a user.

It can be difficult to style as a developer, but that isn't my main concern. My main concern is that we can't actually let the entirety of wp-login.php load before printing our form. Otherwise, plugins will be loaded which means the page could crash if one of those plugins had a fatal error.

My worry about styling, is that if we simply copy the login_header() function, for instance, into a separate file that could be included in isolation, end users won't be able have their existing custom styles applied. If we think that is a worthwhile trade off, I think moving those functions would be a great solution.

I am going to add some stuff to my original patch

Which part? It seems incompatible with the alternate approach. ( Forcing Recovery Mode vs cached permission checks ).

This ticket was mentioned in Slack in #core-site-health by timothybjacobs. View the logs.


5 weeks ago

Note: See TracTickets for help on using tickets.