WordPress.org

Make WordPress Core

Opened 6 years ago

Closed 3 weeks ago

Last modified 6 days ago

#31104 closed defect (bug) (fixed)

Plugin activation includes plugin file in variable scope of activate_plugin

Reported by: Mike_Cowobo Owned by: SergeyBiryukov
Milestone: 5.8 Priority: normal
Severity: normal Version: 4.2
Component: Plugins Keywords: has-patch early needs-dev-note
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 (3)

31104.patch (1.1 KB) - added by Mike_Cowobo 6 years ago.
Include plugin file in a separate function
31104.2.patch (1.1 KB) - added by DrewAPicture 6 years ago.
regenerated from root
31104.diff (880 bytes) - added by SergeyBiryukov 10 months ago.

Download all attachments as: .zip

Change History (19)

@Mike_Cowobo
6 years ago

Include plugin file in a separate function

#1 @SergeyBiryukov
6 years ago

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

#2 @dd32
6 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
6 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.


6 years ago

@DrewAPicture
6 years ago

regenerated from root

#5 @DrewAPicture
6 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-ups: @dd32
6 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 6 years ago by dd32 (previous) (diff)

#7 in reply to: ↑ 6 @DrewAPicture
6 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
5 years ago

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

#9 @SergeyBiryukov
10 months ago

  • Keywords early added
  • Milestone set to Future Release

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.

#10 in reply to: ↑ 6 @SergeyBiryukov
4 weeks ago

  • Milestone changed from Future Release to 5.8
  • Owner changed from dd32 to 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.

Last edited 4 weeks ago by SergeyBiryukov (previous) (diff)

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


3 weeks ago

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


3 weeks ago

#13 in reply to: ↑ description @SergeyBiryukov
3 weeks ago

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.

#14 @SergeyBiryukov
3 weeks ago

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

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.

#15 @SergeyBiryukov
3 weeks ago

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.

#16 @audrasjb
6 days ago

  • Keywords needs-dev-note added
Note: See TracTickets for help on using tickets.