Make WordPress Core

Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#55432 closed defect (bug) (fixed)

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

Reported by: stevegrunwell's profile stevegrunwell Owned by: audrasjb's profile audrasjb
Milestone: 6.0 Priority: normal
Severity: normal Version:
Component: Plugins Keywords: has-patch commit assigned-for-commit
Focuses: Cc:

Description

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".

Attachments (3)

55432.2.diff (2.8 KB) - added by SergeyBiryukov 3 years ago.
55432.diff (1.8 KB) - added by SergeyBiryukov 3 years ago.
55432.3.diff (1.7 KB) - added by SergeyBiryukov 3 years ago.

Download all attachments as: .zip

Change History (17)

#1 @joyously
3 years ago

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

#2 @peterwilsoncc
3 years ago

  • Keywords close added

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.

#3 @stevegrunwell
3 years ago

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;
    } )();
    
  1. (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;
    }
    
  2. Update the DocBlock to indicate that this argument has type "mixed" and cannot be relied upon.

#4 @ocean90
3 years ago

  • Component changed from Bootstrap/Load to Plugins
  • Keywords needs-patch added; close removed
  • Milestone changed from Awaiting Review to Future Release

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

#5 @SergeyBiryukov
3 years ago

  • Keywords has-patch added; needs-patch removed
  • Milestone changed from Future Release to 6.0
  • 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.

#6 @joyously
3 years ago

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.

@SergeyBiryukov
3 years ago

#7 follow-up: @jrf
3 years ago

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.

#8 @azouamauriac
3 years ago

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

#9 in reply to: ↑ 7 @SergeyBiryukov
3 years ago

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!

#10 @peterwilsoncc
3 years ago

  • Keywords commit added

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

#12 @audrasjb
3 years ago

  • Keywords assigned-for-commit added
  • Owner set to audrasjb
  • Status changed from new to accepted

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

#13 @audrasjb
3 years ago

  • Resolution set to fixed
  • Status changed from accepted to closed

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.

Note: See TracTickets for help on using tickets.