#55432 closed defect (bug) (fixed)
The $plugin parameter of the "plugin_loaded" action can be polluted by individual plugins
Reported by: |
|
Owned by: |
|
---|---|---|---|
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)
Change History (17)
#2
@
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
@
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:
- Wrap the
include_once
call in a closure to isolate its variable scope:( function () use ( $plugin ) { include_once $plugin; } )();
- (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; }
- Update the DocBlock to indicate that this argument has type "mixed" and cannot be relied upon.
#4
@
3 years ago
- Component changed from Bootstrap/Load to Plugins
- Keywords needs-patch added; close removed
- Milestone changed from Awaiting Review to Future Release
#5
@
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
@
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.
#7
follow-up:
↓ 9
@
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
@
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
@
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
@
3 years ago
- Keywords commit added
55432.3.diff looks suitable to me and matches the precedent in #28102.
This ticket was mentioned in PR #2468 on WordPress/wordpress-develop by audrasjb.
3 years ago
#11
Trac ticket: https://core.trac.wordpress.org/ticket/55432
#12
@
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.
3 years ago
#14
Committed in https://core.trac.wordpress.org/changeset/53004
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