WordPress.org

Make WordPress Core

Opened 4 years ago

Closed 4 years ago

#29772 closed enhancement (wontfix)

wp-admin/plugins.php should not load plugins so it can be used to disable broke activated plugins

Reported by: aubreypwd Owned by:
Milestone: Priority: normal
Severity: normal Version: 4.1
Component: Administration Keywords: has-patch 2nd-opinion
Focuses: administration Cc:

Description

When I create a plugin that essentially has bad PHP I get an error on the site as expected, for both the site and /wp-admin. I think wp-admin/plugins.php should always load up without including plugins, so if there's a broke one, it can be de-activated by knowing you can navigate to example.com/wp-admin/plugins.php safely and deactivate the plugin.

What I Tested

Created this great plugin and activate it:

<?php

/**
 * Plugin Name: Break Things
 * Plugin URI:
 * Description: Breaks things.
 * Version:
 * Author:
 * Author URI:
 * License: GPL2
 */

this(should) break > right?

?>

Now try and de-activate the plugin via /wp-admin.


Broke plugins shouldn't ever get to the activated state this way, but if a plugin is updated and has bad code, this would prove useful.


How this would be better

https://cldup.com/onlBV3qFWA.gif

Attachments (6)

29772.diff (463 bytes) - added by aubreypwd 4 years ago.
Don't load plugins on the plugins.php page.
29772-dontloadplugins-cookie.diff (2.0 KB) - added by aubreypwd 4 years ago.
Uses plugins.php?dontloadplugins with a cookie to temporarily skip loading plugins so they can be de-activated.
29772-dontloadplugins-cookie-v2.diff (2.4 KB) - added by aubreypwd 4 years ago.
Re-worked so it doesn't loop through foreach. Also message is Admin-wide so it refers to 'Plugins' not 'Plugins below'.
29772-dontloadplugins-cookie-v3.diff (2.8 KB) - added by aubreypwd 4 years ago.
Rework to be more straight forward and check for the right user executed the command.
29772-dontloadplugins-cookie-v3.2.diff (3.0 KB) - added by aubreypwd 4 years ago.
Just realized I don't have to keep submitting -v2, -v3, sorry. Small modification that only sets the cookie if someone is logged in.
29772-dontloadplugins-cookie-v3.3.diff (3.1 KB) - added by aubreypwd 4 years ago.
Re-added unset($plugin)

Download all attachments as: .zip

Change History (19)

@aubreypwd
4 years ago

Don't load plugins on the plugins.php page.

#1 @aubreypwd
4 years ago

Themes should also have something similar to this. Patch just to show example.

I'm also thinking about implications to the fact that additional Admin menu pages will be gone because plugins wouldn't be loaded. Thinking about doing something via GET *like* wp-admin/plugins.php?loadplugins=0 for desperate situations and wondering if the GET parameter would be removed once the plugin was finally deactivated so things to back to normal. If they navigated away from the page they would never land on plugins.php with the parameter again and all the plugins will be loaded next time they went to plugins.php.

Last edited 4 years ago by aubreypwd (previous) (diff)

@aubreypwd
4 years ago

Uses plugins.php?dontloadplugins with a cookie to temporarily skip loading plugins so they can be de-activated.

#2 follow-up: @aubreypwd
4 years ago

Actually, based on my observations, Themes do not appear to need this. If one has an error, is activated and throws an error, it can be de-activated via wp-admin as expected.

@aubreypwd
4 years ago

Re-worked so it doesn't loop through foreach. Also message is Admin-wide so it refers to 'Plugins' not 'Plugins below'.

@aubreypwd
4 years ago

Rework to be more straight forward and check for the right user executed the command.

@aubreypwd
4 years ago

Just realized I don't have to keep submitting -v2, -v3, sorry. Small modification that only sets the cookie if someone is logged in.

@aubreypwd
4 years ago

Re-added unset($plugin)

#3 in reply to: ↑ 2 @SergeyBiryukov
4 years ago

This appears to be a duplicate of #25137.

Replying to aubreypwd:

Actually, based on my observations, Themes do not appear to need this. If one has an error, is activated and throws an error, it can be de-activated via wp-admin as expected.

Not if the error is in the theme's functions.php file, which is loaded in the admin as well.

#4 @aubreypwd
4 years ago

I will patch for themes then as well if interest is shown in this method.

#5 @kitchin
4 years ago

Themes are important. It's a common area for admins to edit through the Dashboard to add snippets of code. I've used FTP to fix admins' syntax errors more than once.

#6 @OriginalEXE
4 years ago

  • Component changed from General to Administration
  • Focuses administration added
  • Keywords has-patch added

This is actually a pretty good idea, themes should definitely have this as well, well done @aubreypwd

#7 follow-up: @aubreypwd
4 years ago

Note my comment on https://core.trac.wordpress.org/ticket/25137#comment:22, I plan to have a complete patch in the next couple of days that allow you to pass ?wp_safe_mode that will do the same thing I've got here that also addresses themes too. I'm hoping to tie this into the login somehow as well.

#8 in reply to: ↑ 7 ; follow-up: @jdgrimes
4 years ago

Replying to aubreypwd:

Note my comment on https://core.trac.wordpress.org/ticket/25137#comment:22, I plan to have a complete patch in the next couple of days that allow you to pass ?wp_safe_mode that will do the same thing I've got here that also addresses themes too. I'm hoping to tie this into the login somehow as well.

Keep in mind that plugins and themes are currently loaded on the login page.

#9 in reply to: ↑ 8 @aubreypwd
4 years ago

Replying to jdgrimes:

Keep in mind that plugins and themes are currently loaded on the login page.

I know. I'm thinking at any point we can pass ?_wp_safe_mode that will take you to wp-login.php?_wp_safe_mode where, and only on wp-login.php, themes and plugins are disabled. There's something visual there letting you know you are logging into safe mode (like a checkbox). When you login from wp-login.php?_wp_safe_mode with the checkbox checked, the cookie is set for the session until the action wp_logout where it's destroyed and logging in is normal.

Something like that :)

Last edited 4 years ago by aubreypwd (previous) (diff)

#10 @azaozz
4 years ago

  • Keywords 2nd-opinion added

There are couple of different ways this can be implemented. Thinking the most straightforward way is to have wp-admin/repair.php. We already have wp-admin/maint/repair.php that requires adding a constant to wp-config.php as it tries to repair the DB.

If we add wp-admin/repair.php, we can do the "pre-flight" in an iframe thing, i.e. test if all is loading properly and even detect which plugin is throwing a fatal error.

Not sure we should be using cookies there. That can be avoided by asking for an administrator name/pass to be entered on each request. So wp-admin/repair.php would have 2-3 steps:

  • Show some help on how to use and ask for a name/pass (simple login form, no cookies set).
  • Run WordPress in a local iframe to detect fatal errors; list all enabled plugins underneath with a way to disable individual plugins or all; have some JS to try to select a plugin that is failing; a way to change the theme (this would still include a simple login form).
  • Reload the same as above to make sure no more fatal errors.

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


4 years ago

#12 follow-up: @pento
4 years ago

wp-admin/repair.php has potential, combined with a wp-admin/maint/repair.php-style define to enable it.

plugins.php *must* load plugins, as most plugins change the plugin table. It would also leave sites potentially vulnerable if they have a security plugin that wouldn't be loaded, or if they use a custom authentication plugin.

wp-admin/repair.php wouldn't be able to load custom authentication plugins, either.

That said, I'm not entirely sure that a repair script is the right approach. Updating a plugin won't re-enable it if it's broken, so that problem is already taken care of. I agree that the theme/plugin editor could be more robust, it'd be worth investigating if we can test loading edited PHP files, before saving them.

#13 in reply to: ↑ 12 @nacin
4 years ago

  • Milestone Awaiting Review deleted
  • Resolution set to wontfix
  • Status changed from new to closed

Replying to pento:

plugins.php *must* load plugins, as most plugins change the plugin table. It would also leave sites potentially vulnerable if they have a security plugin that wouldn't be loaded, or if they use a custom authentication plugin.

Bingo. For this reason alone I am closing this ticket out. #25137 already exists for alternative proposals.

I agree that the theme/plugin editor could be more robust, it'd be worth investigating if we can test loading edited PHP files, before saving them.

The plugin editor does this. Themes: #21622.

Note: See TracTickets for help on using tickets.