The $plugin parameter of the "plugin_loaded" action can be polluted by individual plugins

[stevegrunwell]

The "plugin_loaded" action expects a single argument, $plugin, which should be a string representing the plugin that was just loaded (e.g. "jetpack/jetpack.php").

The code to accomplish this currently looks like this:

// Load active plugins.
foreach ( wp_get_active_and_valid_plugins() as $plugin ) {
	wp_register_plugin_realpath( $plugin );
	include_once $plugin;

	/**
	 * Fires once a single activated plugin has loaded.
	 *
	 * @since 5.1.0
	 *
	 * @param string $plugin Full path to the plugin's main file.
	 */
	do_action( 'plugin_loaded', $plugin );
}
unset( $plugin );

However, this can cause unexpected arguments to be passed to the action if the loaded plugin defines a $plugin variable, since they occupy the same variable scope. For example, consider this (very common) plugin pattern:

# wp-content/plugins/my-plugin/my-plugin.php
<?php
/**
 * Plugin Name: My plugin
 */

require_once __DIR__ . '/class-plugin.php';

$plugin = new MyPlugin();
$plugin->init();

When this plugin is loaded, the value of $plugin being passed to the "plugin_loaded" hook will be an instance of MyPlugin, but the expectation is that it will be the string "my-plugin/my-plugin.php".

[joyously]

WP has lots of global variables that other code can pollute. That's why the best practices page says to prefix everything: ​https://developer.wordpress.org/plugins/plugin-basics/best-practices/#prefix-everything

[peterwilsoncc]

My inclination is to close this ticket without a fix. No matter the variable name WP uses, a plugin could override it.

If a plugin is trying to override the variable maliciously, the plugin author is missing the opportunity to do truly evil things by been able to run code on the web server.

[stevegrunwell]

The issue isn't so much with what the plugin could do so much as that the "plugin_loaded" hook does not behave in a predictable or consistent manner.

Consider the following example callback that simply logs plugins as they're loaded:

add_action('plugin_loaded', function ($plugin) {
    Logger::info(sprintf('Plugin %s was loaded', $plugin));
});

Depending on what a third-party plugin does (or does not do) to the $plugin variable, this will either log "Plugin some-plugin/some-plugin.php was loaded" or throw an error (e.g. "Object of class SomePlugin cannot be converted to string").

This puts the onus of type-checking the value from the hook on each and every callback:

add_action('plugin_loaded', function ($plugin) {
    if (! is_scalar($plugin)) {
        Logger::info('Some plugin was loaded, but we can\'t tell which one!');
    } else {
        Logger::info(sprintf('Plugin %s was loaded', $plugin));
    }
});

The problem only becomes more apparent once scalar typehints are introduced (PHP 7.0+):

add_action('plugin_loaded', function (string $plugin) {
    Logger::info(sprintf('Plugin %s was loaded', $plugin));
});

Uncaught TypeError: {closure}(): Argument #1 ($plugin) must be of type string, SomePlugin given

Not to raise problems without solutions, there are a few ways this could be addressed:

1. Wrap the include_once call in a closure to isolate its variable scope:

   ( function () use ( $plugin ) {
       include_once $plugin;
   } )();
   

2. (Probably more "The WordPress Way™") Create a named function that includes the given file in an isolated scope, then call that within the foreach ( wp_get_active_and_valid_plugins() as $plugin ) loop:

   function load_plugin_file( $plugin ) {
       include_once $plugin;
   }
   

3. Update the DocBlock to indicate that this argument has type "mixed" and cannot be relied upon.

[ocean90]

This looks like a simple fix we can do. There was a previous change in [28644] although it was later adjusted in [50787].

[SergeyBiryukov]

• 55432.diff​ follows the precedent of [28644] and renames $plugin to $_wp_plugin_file to reduce the chance of a conflict.
• 55432.2.diff​ takes another approach more in line with [50787] and introduces a new function, wp_load_plugin_file(), to be used whenever we need to load a plugin file in core.

I think 55432.2.diff​ would be preferred here.

[joyously]

Making a function to load a plugin would have drastic effects, because all the global variables would not be available. This is the same as themes, where certain global variables are imported to the function that loads templates.

If you have to make a change here, I vote for renaming the variable.

[jrf]

I tend to agree with @peterwilsoncc here: this should be a "wontfix" as this can only happen when a plugin is doing it wrong.

The issue isn't so much with what the plugin could do so much as that the "plugin_loaded" hook does not behave in a predictable or consistent manner.

That's the case, though, with every single filter in WP, as well as with every action using WP global vars.

Think: a filter provides a string and expects a string back, the first function filtering returns an array, the second function filtering is getting a TypeError if they don't do any type checking before using the received parameter.

Also see: https://core.trac.wordpress.org/ticket/51525

As for the proposed solutions:

Wrap the include_once call in a closure (or named function) to isolate its variable scope

I agree with @joyously that is bound to cause problems due to global variables not being available and not being explicitly approached as global variables. It's the kind of BC-break which will break plenty of not that well-coded/older plugins.

If you have to make a change here, I vote for renaming the variable.

That, again would be a BC-break, albeit smaller. This is a documented global variable in WordPress, even though only available in this limited context, and changing the name of the variable will:
1) still not prevent people from overwriting the new name.
2) may break plugins relying on the $plugin variable being available in that context.

Update the DocBlock to indicate that this argument has type "mixed" and cannot be relied upon.

That's not a solution, that's turning the problem around and making it worse. The documented type is correct and should stay documented as string. Anything else is giving plugins license to deliberately change WP global variables.

[azouamauriac]

If we definitely agree to rename the variable here, I think ​this both worth to be renamed...

[SergeyBiryukov]

Replying to jrf:

If you have to make a change here, I vote for renaming the variable.

That, again would be a BC-break, albeit smaller. This is a documented global variable in WordPress, even though only available in this limited context, and changing the name of the variable will:
1) still not prevent people from overwriting the new name.
2) may break plugins relying on the $plugin variable being available in that context.

Thanks for taking the time to look into this! I might be missing something, but I could not find the $plugin variable being documented as a global anywhere in core, only as a parameter of the plugin_loaded action, which the renaming should not affect. You're right that it does not completely prevent overwriting the new name, however the chance of accidentally overwriting a name like $_wp_plugin_file would be much lower than for $plugin.

55432.3.diff​ is a minor adjustment to more closely follow [28644] and address the concern of the $plugin variable still being available.

Replying to azouamauriac:

If we definitely agree to rename the variable here, I think ​this both worth to be renamed...

Good catch, thanks!

[peterwilsoncc]

55432.3.diff​ looks suitable to me and matches the precedent in #28102.

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

[audrasjb]

I opened the above PR to make sure to run unit tests against those changes before commit.

[audrasjb]

In 53004:

Plugins: Avoid stomping of some variables in wp-settings.php.

This changeset decreases the chance of accidentally overwriting some variables located in wp-settings.php, as the chance of overriding a variable called $_wp_plugin_file would be lower than for $plugin, $mu_plugin or $network_plugin.

Props stevegrunwell, joyously, peterwilsoncc, ocean90, SergeyBiryukov, jrf, azouamauriac.
Fixes #55432.

Committed in https://core.trac.wordpress.org/changeset/53004