Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Blueprints] Prevent plugin activation error if plugin redirects during activation or produces an output #2066

Merged
merged 29 commits into from
Dec 19, 2024
Merged
Show file tree
Hide file tree
Changes from 25 commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
6fed4da
Prevent plugin activation error if plugin redirects during activation
bgrgicak Dec 10, 2024
2ca6335
Merge branch 'trunk' into fix/1979-plugin-activation-fails-on-redirect
bgrgicak Dec 13, 2024
2f45f25
[Blueprints] Prevent WSOD when autologin is enabled and a plugin logs…
adamziel Dec 13, 2024
3d69181
Merge branch 'prevent-autologin-white-screen-of-death' into fix/1979-…
bgrgicak Dec 17, 2024
d1ee3c9
Use options to validate if a plugin is active
bgrgicak Dec 17, 2024
1747574
Explain why we had to split into two runs
bgrgicak Dec 17, 2024
75cd7b4
Merge branch 'trunk' into fix/1979-plugin-activation-fails-on-redirect
bgrgicak Dec 17, 2024
a46d412
Merge branch 'trunk' into fix/1979-plugin-activation-fails-on-redirect
bgrgicak Dec 17, 2024
7204067
Revert "[Blueprints] Prevent WSOD when autologin is enabled and a plu…
bgrgicak Dec 17, 2024
7aa4329
Always return activation errors
bgrgicak Dec 17, 2024
aaf683d
Merge branch 'trunk' into fix/1979-plugin-activation-fails-on-redirect
bgrgicak Dec 17, 2024
d255141
Merge branch 'trunk' into fix/1979-plugin-activation-fails-on-redirect
bgrgicak Dec 18, 2024
d351a63
Update packages/playground/blueprints/src/lib/steps/activate-plugin.ts
bgrgicak Dec 18, 2024
cf2a580
Remove plugin dir path only from the beginning of a plugin path
bgrgicak Dec 18, 2024
a3eccc6
Return error if plugin wasn't found
bgrgicak Dec 18, 2024
8627934
Update error message
bgrgicak Dec 18, 2024
fd4cb60
Fix linter errror
bgrgicak Dec 18, 2024
7414381
Move activate plugin error logging before is active check
bgrgicak Dec 18, 2024
2dd659e
Add test for double plugin activation
bgrgicak Dec 18, 2024
9b148b2
Ensure all supported plugin paths can be activated
bgrgicak Dec 18, 2024
622099d
Clear up is active check
bgrgicak Dec 19, 2024
d7e7827
Add trailing slash activation test
bgrgicak Dec 19, 2024
56cd875
Apply suggestions from code review
bgrgicak Dec 19, 2024
2efc8e2
Check if the plugin exists in the filesystem
bgrgicak Dec 19, 2024
dde935a
Check if the current relative plugin path is a directory before addin…
bgrgicak Dec 19, 2024
c0dd231
Use resolves.not.toThrow in tests
bgrgicak Dec 19, 2024
0c09fc0
Explain why a WP_Error during plugin activation doesn't mean the plug…
bgrgicak Dec 19, 2024
e27c6fb
Remove .php check
bgrgicak Dec 19, 2024
bb24b49
Use rtrim and only is_dir
bgrgicak Dec 19, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
155 changes: 133 additions & 22 deletions packages/playground/blueprints/src/lib/steps/activate-plugin.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -25,28 +25,83 @@ describe('Blueprint step activatePlugin()', () => {
php = await handler.getPrimaryPhp();
});

it('should activate the plugin', async () => {
const docroot = php.documentRoot;
it('should activate a plugin file located in the plugins directory', async () => {
const docroot = handler.documentRoot;
php.writeFile(
`${docroot}/wp-content/plugins/test-plugin.php`,
`<?php /**\n * Plugin Name: Test Plugin */`
);
await activatePlugin(php, {
pluginPath: 'test-plugin.php',
});

const response = await php.run({
code: `<?php
require_once '/wordpress/wp-load.php';
require_once ${phpVar(docroot)}. "/wp-admin/includes/plugin.php" ;
echo is_plugin_active('test-plugin.php') ? 'true' : 'false';
`,
});
expect(response.text).toBe('true');
await expect(
activatePlugin(php, {
pluginPath: 'test-plugin.php',
})
).resolves.toBeUndefined();
});

it('should activate a plugin file located in a subdirectory of the plugins directory', async () => {
const docroot = handler.documentRoot;
const pluginDir = `${docroot}/wp-content/plugins/test-plugin`;
php.mkdir(pluginDir);
php.writeFile(
`${pluginDir}/test-plugin.php`,
`<?php /**\n * Plugin Name: Test Plugin */`
);

await expect(
activatePlugin(php, {
pluginPath: `test-plugin/test-plugin.php`,
})
).resolves.toBeUndefined();
});

it('should activate a plugin if a absolute plugin path is provided', async () => {
const docroot = handler.documentRoot;
php.mkdir(`${docroot}/wp-content/plugins/test-plugin`);
php.writeFile(
`${docroot}/wp-content/plugins/test-plugin/index.php`,
`<?php /**\n * Plugin Name: Test Plugin */`
);

await expect(
activatePlugin(php, {
pluginPath: `${docroot}/wp-content/plugins/test-plugin/index.php`,
})
).resolves.toBeUndefined();
});

it('should activate a plugin if a absolute plugin directory path is provided', async () => {
const docroot = handler.documentRoot;
php.mkdir(`${docroot}/wp-content/plugins/test-plugin`);
php.writeFile(
`${docroot}/wp-content/plugins/test-plugin/test-plugin.php`,
`<?php /**\n * Plugin Name: Test Plugin */`
);

await expect(
activatePlugin(php, {
pluginPath: `${docroot}/wp-content/plugins/test-plugin`,
})
).resolves.toBeUndefined();
});

it('should activate a plugin if a absolute plugin directory path with a trailing slash is provided', async () => {
const docroot = handler.documentRoot;
php.mkdir(`${docroot}/wp-content/plugins/test-plugin`);
php.writeFile(
`${docroot}/wp-content/plugins/test-plugin/test-plugin.php`,
`<?php /**\n * Plugin Name: Test Plugin */`
);

await expect(
activatePlugin(php, {
pluginPath: `${docroot}/wp-content/plugins/test-plugin/`,
})
).resolves.toBeUndefined();
});

it('should detect a silent failure in activating the plugin', async () => {
const docroot = php.documentRoot;
const docroot = handler.documentRoot;
php.writeFile(
`${docroot}/wp-content/plugins/test-plugin.php`,
`<?php /**\n * Plugin Name: Test Plugin */`
Expand All @@ -56,18 +111,17 @@ describe('Blueprint step activatePlugin()', () => {
`${docroot}/wp-content/mu-plugins/0-exit.php`,
`<?php exit(0); `
);
expect(
async () =>
await activatePlugin(php, {
pluginPath: 'test-plugin.php',
})
await expect(
activatePlugin(php, {
pluginPath: 'test-plugin.php',
})
).rejects.toThrow(/Plugin test-plugin.php could not be activated/);
});

it('should run the activation hooks as a priviliged user', async () => {
const docroot = php.documentRoot;
it('should run the activation hooks as a privileged user', async () => {
const docroot = handler.documentRoot;
const createdFilePath =
docroot + '/activation-ran-as-a-priviliged-user.txt';
docroot + '/activation-ran-as-a-privileged-user.txt';
php.writeFile(
`${docroot}/wp-content/plugins/test-plugin.php`,
`<?php /**\n * Plugin Name: Test Plugin */
Expand All @@ -84,4 +138,61 @@ describe('Blueprint step activatePlugin()', () => {

expect(php.fileExists(createdFilePath)).toBe(true);
});

it('should activate a plugin if it redirects during activation', async () => {
const docroot = handler.documentRoot;
php.writeFile(
`${docroot}/wp-content/plugins/test-plugin.php`,
`<?php
/**
* Plugin Name: Test Plugin
*/
add_action( 'activated_plugin', function( $plugin ) {
if( $plugin == plugin_basename( __FILE__ ) ) {
wp_redirect( admin_url( 'edit.php' ) );
exit();
}
} );
`
);
await expect(
activatePlugin(php, {
pluginPath: 'test-plugin.php',
})
).resolves.toBeUndefined();
});

it('should activate a plugin if it produces a output during activation', async () => {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wait, this seems to contradict the other comment:

	 * Relying on the plugin activation response is not reliable because if the plugin activation
	 * produces any output, WordPress will assume it's an activation error.
	 * See WordPress source code for more details:

So is output an activation error, or is it okay?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment is just incomplete. I explained now better, why a WP_Error during plugin activation doesn't mean the plugin isn't active. 0c09fc0 There is also a section about this in the PR description with a screenshot.

const docroot = handler.documentRoot;
php.writeFile(
`${docroot}/wp-content/plugins/test-plugin.php`,
`<?php
/**
* Plugin Name: Test Plugin
*/
echo 'Hello World';
`
);
await expect(
activatePlugin(php, {
pluginPath: 'test-plugin.php',
})
).resolves.toBeUndefined();
Copy link
Collaborator

@adamziel adamziel Dec 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The return values doesn't seem relevant here, what we're really after is resolves.not.toThrow(). Not a blocker since an exception would fail the test anyway

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done c0dd231

});

it('should not throw an error if the plugin is already active', async () => {
const docroot = handler.documentRoot;
php.writeFile(
`${docroot}/wp-content/plugins/test-plugin.php`,
`<?php /**\n * Plugin Name: Test Plugin */`
);
await activatePlugin(php, {
pluginPath: 'test-plugin.php',
});
await expect(
activatePlugin(php, {
pluginPath: 'test-plugin.php',
})
).resolves.toBeUndefined();
});
});
119 changes: 99 additions & 20 deletions packages/playground/blueprints/src/lib/steps/activate-plugin.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,5 @@
import { phpVar } from '@php-wasm/util';
import { StepHandler } from '.';
import { logger } from '@php-wasm/logger';

/**
* @inheritDoc activatePlugin
* @example
Expand Down Expand Up @@ -39,18 +37,18 @@ export const activatePlugin: StepHandler<ActivatePluginStep> = async (
progress?.tracker.setCaption(`Activating ${pluginName || pluginPath}`);

const docroot = await playground.documentRoot;
const result = await playground.run({
const activatePluginResult = await playground.run({
code: `<?php
define( 'WP_ADMIN', true );
require_once( ${phpVar(docroot)}. "/wp-load.php" );
require_once( ${phpVar(docroot)}. "/wp-admin/includes/plugin.php" );
require_once( getenv('DOCROOT') . "/wp-load.php" );
require_once( getenv('DOCROOT') . "/wp-admin/includes/plugin.php" );

// Set current user to admin
wp_set_current_user( get_users(array('role' => 'Administrator') )[0]->ID );

$plugin_path = ${phpVar(pluginPath)};
$plugin_path = getenv('PLUGIN_PATH');
$response = false;
if (!is_dir($plugin_path)) {
if ( ! is_dir( $plugin_path)) {
$response = activate_plugin($plugin_path);
}

Expand All @@ -65,22 +63,103 @@ export const activatePlugin: StepHandler<ActivatePluginStep> = async (
}
}

if ( null === $response ) {
die('Plugin activated successfully');
} else if ( is_wp_error( $response ) ) {
throw new Exception( $response->get_error_message() );
if ( is_wp_error($response) ) {
die( $response->get_error_message() );
} else if ( false === $response ) {
die( "The activatePlugin step wasn't able to find the plugin $plugin_path." );
}

throw new Exception( 'Unable to activate plugin' );
`,
env: {
PLUGIN_PATH: pluginPath,
DOCROOT: docroot,
},
});
if (result.text !== 'Plugin activated successfully') {
logger.debug(result);
throw new Error(
`Plugin ${pluginPath} could not be activated – WordPress exited with no error. ` +
`Sometimes, when $_SERVER or site options are not configured correctly, ` +
`WordPress exits early with a 301 redirect. ` +
`Inspect the "debug" logs in the console for more details`
if (activatePluginResult.text) {
logger.warn(
`Plugin ${pluginPath} activation printed the following bytes: ${activatePluginResult.text}`
);
}

/**
* Instead of checking the plugin activation response,
* check if the plugin is active by looking at the active plugins list.
*
* We have to split the activation and the check into two PHP runs
* because some plugins might redirect during activation,
* which would prevent any output that happens after activation from being returned.
*
* Relying on the plugin activation response is not reliable because if the plugin activation
* produces any output, WordPress will assume it's an activation error.
* See WordPress source code for more details:
* https://github.com/WordPress/wordpress-develop/blob/6.7/src/wp-admin/includes/plugin.php#L733
*
* Because some plugins can create an output, we need to use output buffering
* to ensure the 'true' response is not polluted by other outputs.
* If the plugin activation fails, we will return the buffered output as it might
* contain more information about the failure.
*/
const isActiveCheckResult = await playground.run({
code: `<?php
ob_start();
require_once( getenv( 'DOCROOT' ) . "/wp-load.php" );

/**
* Extracts the relative plugin path from either an absolute or relative plugin path.
*
* Absolute paths starting with plugin directory (e.g., '/wordpress/wp-content/plugins/test-plugin/index.php')
* should be converted to relative paths (e.g., 'test-plugin/index.php')
*
* Directories should finish with a trailing slash to ensure we match the full plugin directory name.
*
* Examples:
* - '/wordpress/wp-content/plugins/test-plugin/index.php' → 'test-plugin/index.php'
* - '/wordpress/wp-content/plugins/test-plugin/' → 'test-plugin/'
* - '/wordpress/wp-content/plugins/test-plugin' → 'test-plugin/'
* - 'test-plugin/index.php' → 'test-plugin/index.php'
* - 'test-plugin/' → 'test-plugin/'
* - 'test-plugin' → 'test-plugin/'
*/
$plugin_directory = WP_PLUGIN_DIR . '/';
$relative_plugin_path = getenv( 'PLUGIN_PATH' );
if (strpos($relative_plugin_path, $plugin_directory) === 0) {
$relative_plugin_path = substr($relative_plugin_path, strlen($plugin_directory));
}

if (
pathinfo($relative_plugin_path, PATHINFO_EXTENSION) !== 'php' &&
substr($relative_plugin_path, -1) !== '/' &&
is_dir( $plugin_directory . $relative_plugin_path )
) {
$relative_plugin_path = $relative_plugin_path . '/';
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice!

Copy link
Collaborator

@adamziel adamziel Dec 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd add an is_dir() check, otherwise we're assuming something about a file structure based on an arbitrary string

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd add an is_dir() check

Good call, I added it in dde935a.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or, for brevity:

			if ( is_dir( $plugin_directory . $relative_plugin_path ) ) {
				$relative_plugin_path = rtrim( $relative_plugin_path, '/' ) . '/';
			}

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, this is much cleaner now. bb24b49


$active_plugins = get_option( 'active_plugins' );
foreach ( $active_plugins as $plugin ) {
if ( substr( $plugin, 0, strlen( $relative_plugin_path ) ) === $relative_plugin_path ) {
ob_end_clean();
die( 'true' );
}
}
die( ob_get_flush() ?: 'false' );
`,
env: {
DOCROOT: docroot,
PLUGIN_PATH: pluginPath,
},
});

if (isActiveCheckResult.text === 'true') {
// Plugin activation was successful, yay!
return;
}

if (isActiveCheckResult.text !== 'false') {
logger.debug(isActiveCheckResult.text);
}
throw new Error(
`Plugin ${pluginPath} could not be activated – WordPress exited with no error. ` +
`Sometimes, when $_SERVER or site options are not configured correctly, ` +
`WordPress exits early with a 301 redirect. ` +
`Inspect the "debug" logs in the console for more details.`
);
};
Loading