Plugin activation includes plugin file in variable scope of activate_plugin

[Mike_Cowobo]

When a plugin is activated, it is included in activate_plugin, with access to the variable scope of activate_plugin. This caused a problem with $plugin in #28102, and was fixed in [28644], but the other variables that are used in the function ($current, $silent and $network_wide) can still potentially cause problems.

Overriding $current and $network_wide, a plugin could potentially deactivate all active plugins on activation or activate itself network wide when activated per-site (or vice-versa).

It's not hard to imagine plugins using those var names in the main plugin file. To illustrate, here is an example of a broken plugin file:

<?php
/*
Plugin Name: Broken Plugin
Plugin URI: http://example.com
Description: This plugin breaks things when it is activated
Version: 0.1
Author: Your Name
Author URI: http://example.com
*/

// Is this a network install?
$network_wide = is_multisite(); // EFFECT: Will turn a per-site activation into a multisite activation

$defaults = array( "my-option" => 1 );

foreach( $defaults as $option => $current ) {

    if ( !get_option( $option ) ) {
        add_option( $option, $current );
    }

} // EFFECT: $current is now '1'. This deactivates ALL active plugins.

Though the occurrence will be rare, these side-effect can be hard to debug for a plugin developer. Instead of saving the variable in a shadow var like in [28644], it can be easily and definitively fixed by calling the include from a separate function, so the variable scope of activate_plugin is simply out of reach. I have attached a patch. With the patch, the broken plugin above loses all side-effects.

[Mike_Cowobo]

Include plugin file in a separate function

[dd32]

The only problem that could kick in here, IMHO, would be plugins that expect those variables to be available at inclusion time.

That being said, no-plugin should be running code on inclusion (other than actions/filters/activation hooks), or have global variables such as that, and since the plugin is already loaded, if it wants to mess with those settings it can do that too.

[Mike_Cowobo]

@dd32: I agree that it generally shouldn't happen, but the behaviour is unexpected, because it differs from how plugins are included during the normal WP bootstrap. This makes it hard to debug problem arising from defining variables in your main plugin file.

Though it is recommended and best practice to have a root plugin file do nothing more than bootstrap the plugin by setting some actions and filters (or better yet, hooking the bootstrap into plugins_loaded), it is not imperative that a plugin author doesn't work with any variables during the bootstrap. I think a lot of developer headache can be spared by simply taking the plugin file out of the var scope of activate_plugin.

[DrewAPicture]

regenerated from root

[DrewAPicture]

@dd32: Can you please review and recommend a course of action here? See 31104.2.patch​.

[dd32]

I'm still not 100% convinced that we actually need to change anything here honestly, but I'll not repeat the same bad-practices arguments.
However, I'll note that shifting activation to occur via a different function, adds no real overhead, nor downsides that can't be worked around.

The patch as-is, can't be applied, as it's got back-compat concerns.
Those concerns are that it no longer has access to the same variables that it previously did, so something like this would be needed (although I dislike the usage of extract, which we're trying to avoid).

acivate_plugin(..):
   ...
   do_activate_plugin( $plugin, get_defined_vars() );
   ...
do_activate_plugin( $plugin, $vars ):
  extract $vars;
  include $plugin;

The plugin still has access to the same scoped variables it previously did, but is no longer able to modify them.

[DrewAPicture]

Replying to dd32:

I'm still not 100% convinced that we actually need to change anything here honestly, but I'll not repeat the same bad-practices arguments.
However, I'll note that shifting activation to occur via a different function, adds no real overhead, nor downsides that can't be worked around.

The patch as-is, can't be applied, as it's got back-compat concerns.
Those concerns are that it no longer has access to the same variables that it previously did, so something like this would be needed (although I dislike the usage of extract, which we're trying to avoid).

acivate_plugin(..):
   ...
   do_activate_plugin( $plugin, get_defined_vars() );
   ...
do_activate_plugin( $plugin, $vars ):
  extract $vars;
  include $plugin;

The plugin still has access to the same scoped variables it previously did, but is no longer able to modify them.

OK, let's pick this up in 4.3 then. @dd32: Feel free to pull it back into 4.2 if you think that would be better than waiting.

[mensmaximus]

If the way of inclusion gets changed in activate_plugin() this might also be changed in plugin_sandbox_scrape().

[SergeyBiryukov]

Found this ticket while trying to figure out why activate_plugin() doesn't just call plugin_sandbox_scrape() instead of repeating pretty much the same.

I'd be willing to try something like 31104.diff​ early in a future release and see if any adjustments are needed.

[SergeyBiryukov]

Replying to dd32:

The patch as-is, can't be applied, as it's got back-compat concerns.
Those concerns are that it no longer has access to the same variables that it previously did, so something like this would be needed (although I dislike the usage of extract, which we're trying to avoid).

Here's the list of the variables in question, for reference:

$plugin, $redirect, $network_wide, $silent, $current, $valid, $requirements

I think $valid and $requirements can be discarded right away, these are one-time internal variables.

I thought of passing something like this as a second argument to plugin_sandbox_scrape():

$args = compact( 'plugin', 'redirect', 'network_wide', 'silent', 'current' );

And then recreating them there, to avoid using extract() as per #22400:

$plugin       = $args['plugin'];
$redirect     = $args['redirect'];
$network_wide = $args['network_wide'];
$silent       = $args['silent'];
$current      = $args['current'];

However, it looks like most of these are also internal variables that a plugin should not really be concerned with, except for maybe $network_wide. As noted above, running any code depending on these values directly at inclusion time is bad practice that we may not necessarily want to support. As a workaround for $network_wide, plugins still have access to $_GET['networkwide'], see [21104] / #20995. An activation hook callback also receives $network_wide as the first argument.

So it seems like we can try 31104.diff​ early in 5.8 and see what plugins break, if any, and then decide if any of these variables are necessary to keep for backward compatibility.

[SergeyBiryukov]

Replying to Mike_Cowobo:

Overriding $current and $network_wide, a plugin could potentially deactivate all active plugins on activation or activate itself network wide when activated per-site (or vice-versa).

Just noting that the issue with $current appears to be resolved in [32504] / #32368, as the value is now recreated after the plugin is loaded. The issue with overriding $network_wide still exists though, so it seems more future-proof to go with the patch.

[SergeyBiryukov]

In 50787:

Plugins: When loading a plugin in a "sandbox" on activation, do it in a separate function.

This avoids accidentally overriding some variables in the scope of activate_plugin(), e.g. $silent or $network_wide.

Plugins expecting to have access to $network_wide directly on inclusion should receive it as an argument of the activation hook callback instead, on any of these actions:

• activate_plugin
• activate_{$plugin}
• activated_plugin

Follow-up to [28644].

Props Mike_Cowobo, dd32, DrewAPicture, mensmaximus, SergeyBiryukov.
Fixes #31104.

[SergeyBiryukov]

In 50788:

Plugins: When loading a plugin in a "sandbox" on activation, do it once.

This avoids a fatal error if the plugin is already included, e.g. in unit tests.

Follow-up to [50787].

See #31104.