Plugin_Upgrader assumption causes incorrect "Activate Plugin" link after plugin installation resulting in "The plugin does not have a valid header" error

[michelleblanchette]

Observed in WordPress 6.3.1

Issue Summary

The "Activate Plugin" link after installing a new plugin is inconsistent with the "Activate" link generated in the Installed Plugins listing table.

The "Activate Plugin" link on the plugin installation screen makes assumptions that produces an incorrect link which results in the user encountering the "The plugin does not have a valid header" error.

Steps to Reproduce

1. Create a plugin that contains another plugin. 1.a. The contained plugin's basename should be lexicographically less than the wrapper plugin which you are actually trying to install. For example, "hello-pro/hello-pro.php" is the main plugin which contains "hello-pro/hello/hello.php" as a base/dependency plugin.
2. Add the plugin via wp-admin > Plugins > Add New and upload the plugin zip file.
3. Upon successful installation, click the "Activate Plugin" link. 3.a. Notice that the "?action=activate&plugin=..." plugin value is wrong as it refers to the contained plugin's main file within the wrapper plugin.
4. Observe the wp_die() error screen which says "The plugin does not have a valid header"

Problematic Source Code

The incorrect "Activate Plugin" link is generated here:
​https://github.com/WordPress/WordPress/blob/3546f04e160fbd31b46ba70b583d0a1d9fe1d80b/wp-admin/includes/class-plugin-installer-skin.php#L115

The offending assumption is here:
​https://github.com/WordPress/WordPress/blob/3546f04e160fbd31b46ba70b583d0a1d9fe1d80b/wp-admin/includes/class-plugin-upgrader.php#L546

Caused by an arbitrary situation here:
​https://github.com/WordPress/WordPress/blob/3546f04e160fbd31b46ba70b583d0a1d9fe1d80b/wp-admin/includes/plugin.php#L348

Explanation

The "Activate Plugin" link on the "Installing plugin from uploaded file" screen in wp-admin uses the least lexicographic plugin basename discovered within the plugin's files. This is an arbitrary situation which can result in the incorrect plugin basename being referenced for activation, which ultimately results in the user experiencing an error screen.

Desired Solution

The "Activate Plugin" link after adding a new plugin and the "Activate" link in the installed plugin's table row actions should match. The "Activate" link in the plugins listing table is preferred as it refers to the correct plugin basename for activation.

[michelleblanchette]

The root of the problem is that get_plugins() is called with a different $plugin_folder between the two instances discussed.

The function searches one subdirectory deep from the specified $plugin_folder for PHP files with a plugin header.

Plugin_Upgrader uses the installed plugin dir as the root search directory for plugins while WP_Plugins_List_Table uses the default WP_PLUGIN_DIR as the root.

This means that Plugin_Upgrader finds plugins one level deeper in the filesystem than typically found.

To avoid this issue, included plugin dependencies should be included a level deeper than the wrapper plugin's root. For example:

• Wrong: hello-pro/hello/hello.php
• Correct: hello-pro/lib/hello/hello.php

We likely don't see this issue often because most dependency plugins are included via Composer, which already nests included dependencies under a vendor directory level.

However, I think the inconsistency in plugin file scanning is still problematic as it incorrectly produces two different activation links depending on the context.

The Plugin_Upgrader class may use a plugin file nested deeper than typically found (eg. WP_Plugins_List_Table). This change improves which plugin file is assumed to be the actual plugin file of the uploaded plugin. See Trac ticket for full details.

Trac ticket: https://core.trac.wordpress.org/ticket/59375

[michelleblanchette]

Initial GitHub Pull Request patch diff

[afragen]

Am I correct in assuming this is about nested plugins?

The main plugin file must be at the root level of the containing folder and there can only be one main plugin file.

To avoid this issue, included plugin dependencies should be included a level deeper than the wrapper plugin's root. For example:

Wrong: hello-pro/hello/hello.php
Correct: hello-pro/lib/hello/hello.php

Both of the above are incorrect.

wp-content/plugins/hello-pro/hello.php is the only correct answer.

Refer to plugin best practices, ​https://developer.wordpress.org/plugins/plugin-basics/best-practices/#folder-structure

[michelleblanchette]

Hi, Andy. Thank you for reviewing this ticket.

I'm not sure what you mean by "nested plugin", but I am aware that the agreed upon practice is to use the hello-pro/hello-pro.php naming convention for the plugin's main file. That is, where the plugin's dirname slug matches the main PHP file's basename.

I'm talking about an issue where a plugin included within a plugin (specifically at the main level) is being incorrectly recognized by the Plugin_Upgrader class's use of get_plugins().

Including a plugin within another plugin as a library is not uncommon, ​as described by the popular Advanced Custom Fields plugin.

In my example case, I'm referring to a simplified example where you have the following file structure:

/plugin-name
     plugin-name.php
     /base-plugin
          base-plugin.php
     ...

plugin-name/plugin-name.php is the plugin to be installed and activated. /plugin-name/base-plugin/base-plugin.php is simply included within the main plugin file to execute its functionality. The reason for doing this is to include plugin dependencies at a guaranteed compatible version. But that's besides the point.

The main point is that Plugin_Updater::plugin_info() produces the incorrect plugin basename when a plugin includes another plugin in this way, contrary to the WP_Plugins_List_Table "Activate" link which produces the correct basename.

My linked patch prevents the "nested plugins" issue that you might be referring to, which Plugin_Updater::plugin_info() currently wrongly attempts to allow.