WordPress.org

Make WordPress Core

Opened 4 years ago

Last modified 6 weeks ago

#31104 reviewing defect (bug)

Plugin activation includes plugin file in variable scope of activate_plugin

Reported by: Mike_Cowobo Owned by: dd32
Milestone: Priority: normal
Severity: normal Version: 4.2
Component: Plugins Keywords: has-patch
Focuses: Cc:

Description

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.

Attachments (2)

31104.patch (1.1 KB) - added by Mike_Cowobo 4 years ago.
Include plugin file in a separate function
31104.2.patch (1.1 KB) - added by DrewAPicture 4 years ago.
regenerated from root

Download all attachments as: .zip

Change History (10)

@Mike_Cowobo
4 years ago

Include plugin file in a separate function

#1 @SergeyBiryukov
4 years ago

  • Keywords has-patch added
  • Milestone changed from Awaiting Review to 4.2

#2 @dd32
4 years ago

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.

#3 @Mike_Cowobo
4 years ago

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

This ticket was mentioned in Slack in #core by drew. View the logs.


4 years ago

@DrewAPicture
4 years ago

regenerated from root

#5 @DrewAPicture
4 years ago

  • Owner set to dd32
  • Status changed from new to reviewing

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

#6 follow-up: @dd32
4 years ago

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.

Last edited 4 years ago by dd32 (previous) (diff)

#7 in reply to: ↑ 6 @DrewAPicture
4 years ago

  • Milestone changed from 4.2 to Future Release

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.

#8 @mensmaximus
3 years ago

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

Note: See TracTickets for help on using tickets.