WordPress.org

Make WordPress Core

Opened 2 years ago

Last modified 14 months ago

#36406 reviewing defect (bug)

$network_wide is unreliable

Reported by: mensmaximus Owned by:
Milestone: Future Release Priority: normal
Severity: critical Version: 3.0
Component: Plugins Keywords: has-patch needs-refresh needs-unit-tests
Focuses: multisite Cc:

Description

This issue may be related to ticket #31104

Scenario: In a WordPress network an admin decides a plugin must not be network activated. Only a per site or even a site specific activation shall be allowed.

Idea: Conditional check during plugin activation and die with an error message if $network_wide is true.

Result: Plugin does not get activated but the custom error message is not displayed

Assume the following function run during activation (it is simplified and the real activation sequence does not have echos!)

function test_activation_hook( $network_wide ) {
	echo '<span>1 | </span>';
	// exit( 1 );
	if ( is_multisite() ) {
		echo '<span>2: | </span>';
		// exit( 2 );
		if ( $network_wide ) {
			echo '<span>3: | </span>';
			// exit( 3 );
		} else {
			echo '<span>4: | </span>';
			// exit( 4 );
		}
	} else {
		echo '<span>5: | </span>';
		// exit( 5 );
	}
	echo '<span>6: | </span>';
	exit( 6 );
}

If you exit after if(is_multisite) the message displayed will be '1 | 2 |' If you exit after if($network_wide) during a networkwide activation the message displayed is '1 | 2 | 4 | 6' instead of '1 | 2| 3|' Exiting at 4, 5, or 6 will display '1 | 2| 4|', '1 | 2| 5|' and '1 | 2| 6|' as expected.

After many hours of debuging I realized the function (the action filter 'activate_' . $plugin) gets executed 3 times if you exit it early. And only the first call has the argument $network_wide set to true if it is an network_wide activation.

If you like to exit at 3, that is you do not want a network activation, the flow is therefore as follows:

  • The first run (click on network activate) has $network_wide set to true and will exit at 3
  • the second run (no idea why) has $network_wide set to false and will pass 1 | 2 | 4 | 6
  • the third run (no idea why) has $network_wide set to false and will pass 1 | 2 | 4 | 6

Imho calling the activation function (or the code) three times in case you exit it (better say terminate because you cant end a plugin installiton clean) is wrong at all. However if this is by design then $network_wide must be reliably set to true if it originally was.

Attachments (2)

plugins.patch (1.2 KB) - added by mensmaximus 21 months ago.
plugins.2.patch (1.1 KB) - added by mensmaximus 18 months ago.
Original Patch updated for WP 4.7

Download all attachments as: .zip

Change History (32)

#1 @mensmaximus
2 years ago

To visualize this issue better I have captured some information at the beginning of the function (where exit(1) is located):

Captured during a successful network activation

***** activate_wpmu-activation-test/index.php: 2016-04-03 10:27:49 *****
(
    [stage] => 1
    [network_wide] => 1
    [GET] => Array
        (
            [action] => activate
            [plugin] => wpmu-activation-test/index.php
            [plugin_status] => all
            [paged] => 1
            [s] => 
            [_wpnonce] => ed6fce4af2
            [networkwide] => 1
        )

)

Captured while exiting at exit(3) to avoid network activation of the plugin

***** activate_wpmu-activation-test/index.php: 2016-04-03 10:25:46 *****
(
    [stage] => 1
    [network_wide] => 1
    [GET] => Array
        (
            [action] => activate
            [plugin] => wpmu-activation-test/index.php
            [plugin_status] => all
            [paged] => 1
            [s] => 
            [_wpnonce] => ed6fce4af2
            [networkwide] => 1
        )

)

***** activate_wpmu-activation-test/index.php: 2016-04-03 10:25:48 *****
(
    [stage] => 1
    [network_wide] => 
    [GET] => Array
        (
            [action] => error_scrape
            [plugin] => wpmu-activation-test/index.php
            [_wpnonce] => c9063923af
        )

)
***** activate_wpmu-activation-test/index.php: 2016-04-03 10:25:48 *****
(
    [stage] => 1
    [network_wide] => 
    [GET] => Array
        (
            [action] => error_scrape
            [plugin] => wpmu-activation-test/index.php
            [_wpnonce] => c9063923af
        )

)

#2 @mensmaximus
2 years ago

Well now I understand the issue arises from calling plugin_sandbox_scrape() in /wp-admin/plugins.php. Just for a test I set

$_GET['networkwide'] = 1

within plugin_sandbox_scrape() and replaced

if ( $network_wide )

with

if ( $_GET['networkwide'] )

in my activation test (the back compat for plugins). As a result the message I want to show is displayed. However this is not a (good) solution and still I think calling plugin_sandbox_scrape() twice is wrong.

Last edited 2 years ago by mensmaximus (previous) (diff)

#3 @mensmaximus
2 years ago

And I think there is one more bug in /wp-admin/plugins.php within the 'error_scrape' case from the switch statement. Line 166 shows

do_action( "activate_{$plugin}" );

but I think this should be

do_action( "activate_{$plugin}", is_network_admin() );

like within activate_plugin() in /wp-admin/includes/plugin.php where in shows in line 573

do_action( 'activate_' . $plugin, $network_wide );
Last edited 2 years ago by mensmaximus (previous) (diff)

#4 @jeremyfelt
2 years ago

  • Focuses multisite added

This ticket was mentioned in Slack in #core-multisite by mensmaximus. View the logs.


2 years ago

#6 @mensmaximus
2 years ago

After some more investigation I can definitely say passing true to do_action( "activate_{$plugin}" ); in plugins.php line 166 solves the described problem.

The question now is how can we determine whether the plugin activation was started on the network admin screen? is_network_admin() is not working in case error_scrape because WP_NETWORK_ADMIN is not defined and $GLOBALS['current_screen']->in_admin( 'network' ) is empty. I even tried to pass is_network_admin() as an argument to $iframe_url in /wp-admin/plugins.php line 438.

#7 @mensmaximus
2 years ago

No feedback? Really nobody? Not a single statement for 3 month? Just me talking to myself?

This ticket was mentioned in Slack in #core-multisite by mensmaximus. View the logs.


23 months ago

#9 @swissspidy
23 months ago

  • Keywords needs-patch added
  • Milestone changed from Awaiting Review to Future Release

#10 follow-up: @earnjam
23 months ago

If I'm reading this correctly, you want to add support in core for plugin authors to prevent network activation of their plugins? Right now this could be done manually within the code of the plugin, but you would like to facilitate this by supporting it directly within core?

I could see maybe something like the opposite of the Network: true header.

So you could have 3 options for plugins on multisite:

  1. Network activation only
  2. Individual site activation only
  3. Either network or individual site activations

Right now 3 is the default and 1 is possible if the Network: true header is set. I could see declaring Network: false as serving that purpose.

I could work on a patch for this ticket, but want to make sure I understand your goal.

#11 in reply to: ↑ 10 @mensmaximus
23 months ago

Replying to earnjam:

Right now 3 is the default and 1 is possible if the Network: true header is set. I could see declaring Network: false as serving that purpose.

This is what the ticket is about and what the title says. $network_wide is not set consistently and therefore you cant rely on it. Please read my opening post again. You can exit the code after the if ($network_wide) but you can't send a admin message to display a warning like "this plugin cant be activated networkwide". The problem stems from the error checking as explained in my comments 2, 3 and 6. The $network_wide argument passed to the plugin activation gets lost if the activation process raises an error.

Last edited 23 months ago by mensmaximus (previous) (diff)

This ticket was mentioned in Slack in #core-multisite by mensmaximus. View the logs.


21 months ago

#13 @mensmaximus
21 months ago

The best solution I can come up with is the attached patch. The only place I could find to check for is_network_admin() is before error_scrape is called. I have added the value to the GET parameters so I can fetch it and pass it to activate_{plugin}. In my tests this works well and has no negative impact.

This ticket was mentioned in Slack in #core-multisite by flixos90. View the logs.


21 months ago

#15 @flixos90
21 months ago

  • Milestone changed from Future Release to 4.7
  • Owner set to SergeyBiryukov
  • Status changed from new to reviewing

The patch appears solid to me. What we need to think about is whether the is_network_admin() check will be reliable enough to know that we're network-activating the plugin. I think it is in this case - but maybe we can find a more accurate solution.

#16 @flixos90
21 months ago

  • Keywords has-patch added; needs-patch removed

This ticket was mentioned in Slack in #core-multisite by flixos90. View the logs.


20 months ago

#18 @jeremyfelt
20 months ago

  • Keywords needs-testing added
  • Owner changed from SergeyBiryukov to jeremyfelt

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


20 months ago

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


20 months ago

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


19 months ago

#22 @jeremyfelt
19 months ago

  • Keywords needs-patch added; has-patch needs-testing removed
  • Milestone changed from 4.7 to Future Release
  • Owner jeremyfelt deleted
  • Version changed from 4.4.2 to 3.0

I think I've wrapped my head around what's possible and what's missing here. Thanks for your patience @mensmaximus!

First, I don't think there's a way to short-circuit network activation of a plugin and show a custom or configurable error message in our current state. When the plugin's activation hook fires, WordPress expects to get null back. If anything else is output, then a predetermined path to an error condition occurs.

  • If echo 'error'; is used, an error explaining that unexpected characters were output will appear. The plugin will still be activated with just a warning to be cautious.
  • If exit() is used, a fatal error will occur during activation and a different error will be showing.

It's when this fatal error is show that the plugin goes through additional validations and the plugin activation hook is run again with varying $network_wide values. If the plugin is network activated with no error, the activation hook fires once.

It's possible to work around the activation by using the activated_plugin hook to reverse activation immediately after. Something like this:

function remove_this_plugin( $plugin ) {
	$current = get_site_option( 'active_sitewide_plugins', array() );
	unset( $current[ $plugin ] );
	update_site_option( 'active_sitewide_plugins', $current );
}

register_activation_hook( __FILE__, 'test_activation_hook' );
function test_activation_hook( $network_wide ) {
	if ( $network_wide ) {
		add_action( 'activated_plugin', 'remove_this_plugin' );
	}
}

This works, but provides no feedback to the user that the plugin activation was not successful.

It seems like there should be room for a filter that allows a plugin to set the WP_Error message in a way that a proper error will be displayed. I'm not yet sure if I'm missing anything in thinking that way.

I'm going to push this back to Future Release. We should continue discussing possible approaches.

#23 follow-up: @mensmaximus
19 months ago

Thank you for the feedback. However I don't understand the final statement to push it back again. My patch is working as expected. If I exit the activation while network_wide was set the activation is stopped and my custom error message ist shown.

#24 in reply to: ↑ 23 @jeremyfelt
19 months ago

Replying to mensmaximus:

Thank you for the feedback. However I don't understand the final statement to push it back again. My patch is working as expected. If I exit the activation while network_wide was set the activation is stopped and my custom error message ist shown.

I'm not yet sure that working around the generation of a fatal error is the proper way to handle this. It may be, but pushing it back provides some time to think about other possible approaches.

#25 @mensmaximus
18 months ago

  • Keywords has-patch added; needs-patch removed
  • Severity changed from normal to major

Now as 4.7 is launched I would really like to see my patch or any other reliable solution to make it into 4.7.1.

Still I do not understand what you are saying @jeremyfelt. My solution does not produce any fatal errors. My patch just fixes the missing $network_wide parameter. This is neither an enhancement nor a new feature. This is a bug fix.

I really would like to avoid patching 72 wordpress multisite installations after each update for a proper error handling :-).

@mensmaximus
18 months ago

Original Patch updated for WP 4.7

#26 @swissspidy
18 months ago

  • Keywords needs-refresh needs-unit-tests added
  • Severity changed from major to normal

Now as 4.7 is launched I would really like to see my patch or any other reliable solution to make it into 4.7.1.

4.7.1 is specifically for regressions introduced in 4.7. Tickets like this one would get into a major release. No agreement could be reached for 4.7, that's why it was pushed back.

Still I do not understand what you are saying @jeremyfelt. My solution does not produce any fatal errors. My patch just fixes the missing $network_wide parameter. This is neither an enhancement nor a new feature. This is a bug fix.

Nobody said your solution would produce any fatal errors, but that the patch works around the handling of fatal errors. Quote:

I'm not yet sure that working around the generation of a fatal error is the proper way to handle this.

A quick note on the patch:

  • Patches should ideally be made from the root directory of the develop repository, so that the path reads like src/wp-admin/plugins.php, not .mensmaximus.com/htdocs/wp-admin/plugins.php or plugins. like in the latest patch. Otherwise the patch cannot be applied easily.
  • $_REQUEST[ 'network_wide' ] is not being sanitized. Since you only want a boolean value, that line could be simply changed to do_action( "activate_{$plugin}", isset( $_REQUEST['network_wide'] ) );

Further:

  • The activate_plugin hook really needs to be fixed. At one point the $network_wide argument passed and at another point it is not — without any documentation about it.
  • Sharing the same concern as Felix:

What we need to think about is whether the is_network_admin() check will be reliable enough to know that we're network-activating the plugin. I think it is in this case - but maybe we can find a more accurate solution.

Adding needs-unit-tests to see if we can add unit tests for this. Not sure how testable the code is though.

#27 @mensmaximus
18 months ago

@swissspidy thank you for the explanation. I understand I have to wait until the core development team takes care of it.

#28 follow-up: @KestutisIT
14 months ago

  • Severity changed from normal to critical

It looks like that there is no solution currently at all, because I cannot put do_action from no reason at some random point of code, and I must use:

<?php
 register_activation_hook($this->coreConf->getPluginPathWithFilename(), array(&$this, 'activate'));

and in the:

public function activate()
{
// there is NO way to check is that is single site, or network activation
}

So the only hack might be to put:

<?php
if(is_network_admin())
{
  register_activation_hook($this->coreConf->getPluginPathWithFilename(), array(&$this, 'networkActivate'));
} else is_admin())
{
  register_activation_hook($this->coreConf->getPluginPathWithFilename(), array(&$this, 'activate'));
}

And this is critical, because this do not allow normal activation of network plugin at all, without leaving ability for plugin then being activated for single site only in multisite mode.

#29 in reply to: ↑ 28 ; follow-up: @KestutisIT
14 months ago

Replying to KestutisIT:

It looks like that there is no solution currently at all, because I cannot put do_action from no reason at some random point of code, and I must use:

<?php
 register_activation_hook($this->coreConf->getPluginPathWithFilename(), array(&$this, 'activate'));

and in the:

public function activate()
{
// there is NO way to check is that is single site, or network activation
}

So the only hack might be to put:

<?php
if(is_network_admin())
{
  register_activation_hook($this->coreConf->getPluginPathWithFilename(), array(&$this, 'networkActivate'));
} else is_admin())
{
  register_activation_hook($this->coreConf->getPluginPathWithFilename(), array(&$this, 'activate'));
}

And this is critical, because this do not allow normal activation of network plugin at all, without leaving ability for plugin then being activated for single site only in multisite mode.

And even my hack with is_network_admin() doesn't work here, because if I do debug test for network-install click, it says that is a regular admin page, not a network. So that works only after plugin install, when I browse plugin pages, but not during plugin activation. So there is absolutely NO SOLUTION AT THIS MOMENT.

#30 in reply to: ↑ 29 @jeremyfelt
14 months ago

Replying to KestutisIT:

and in the:

public function activate()
{
// there is NO way to check is that is single site, or network activation
}

$network_wide is available in activate() here and can be used to detect if this is a network activation.

The issue described by this ticket is that there's no clean way to avoid network activation when $network_wide is seen as true.

Note: See TracTickets for help on using tickets.