From 5c6d42991bdeed164191809f38951a89626aa727 Mon Sep 17 00:00:00 2001 From: Nikolay Strikhar Date: Mon, 24 Aug 2026 13:46:52 +0200 Subject: [PATCH 1/5] Take core's sandbox iframe out of the reactivation notice Rewording the sentence was half the screen. wp-admin/plugins.php builds $errmsg and then appends an error_scrape iframe to it before handing the lot to wp_admin_notice(), and that iframe re-requests the plugins screen with action=error_scrape, which runs plugin_sandbox_scrape() again with display_errors forced on. So the raw "Cannot redeclare ..." fatal printed directly beneath the friendly explanation, inside the same notice box, saying the opposite of it -- on the one screen least able to afford a second opinion. The iframe is matched on `error_scrape` inside its opening tag rather than rebuilt to compare against. Reproducing it would mean reproducing add_query_arg(), urlencode() and esc_url() over a URL core assembles from admin_url(), and a filter on any of those makes the removal miss silently. `[^>]*` cannot cross the end of the tag, so an iframe another plugin appended to the same notice is out of reach. Core's second wording is now rewritten too. Its fatal-error handler pauses the plugin the sandbox died in, so the standalone returns to the plugins list with a Resume link and pressing it fails identically -- reported as "could not be resumed". That request is identified from the nonce alone: resume_plugin() redirects with only an _error_nonce, minted from plugin-resume-error_ and the basename, so there is no `plugin` argument to read and each registered standalone is offered to the nonce in turn. Stripping happens only once one of the two sentences has actually been replaced. A notice holding neither was authored by something else, and there the iframe stays: a notice nobody explained is bad, and a notice nobody explained with its one diagnostic quietly deleted is worse. Every existing reason to decline -- wrong screen, failed nonce, unregistered plugin, a message that sanitises away -- returns the markup exactly as it arrived, iframe included. The fixtures are now transcribed from wp-admin/plugins.php and built through wp_get_admin_notice(), so the iframe under test is the one core really emits rather than a plausible-looking string. --- cspell.json | 1 + docs/conflict-handling.md | 30 +++- src/Conflict/Rewriter.php | 192 ++++++++++++++++---- tests/unit/Conflict/RewriterTest.php | 256 ++++++++++++++++++++++++--- 4 files changed, 412 insertions(+), 67 deletions(-) diff --git a/cspell.json b/cspell.json index b4c52d8..b5ae9a6 100644 --- a/cspell.json +++ b/cspell.json @@ -78,6 +78,7 @@ "wpautop", "wpdb", "WPMU", + "wpnonce", "wpunit" ], "ignoreWords": [ diff --git a/docs/conflict-handling.md b/docs/conflict-handling.md index fd284ea..df9afc7 100644 --- a/docs/conflict-handling.md +++ b/docs/conflict-handling.md @@ -135,15 +135,27 @@ example of this behavior. The guard cannot help on the request that *activates* the standalone: WordPress includes the plugin being activated **after** the bundled copy has already loaded, so that re-declaration is a real fatal. Core catches it in its activation sandbox and prints *"Plugin could not be activated because -it triggered a fatal error."* — true, and useless to whoever pressed the button. - -So the library filters `wp_admin_notice_markup` and swaps that sentence for the sub-plugin's -`conflict_notice_message`, falling back to a generic one naming the slug. This is what puts the -WordPress floor at 6.4: the filter does not exist before it. - -It touches nothing else. The markup comes back untouched unless every one of these holds — the -screen is `plugins`, or `plugins-network` in the network admin; the `plugin` query arg names a -standalone this library has registered; and `_error_nonce` verifies. +it triggered a fatal error."* — true, and useless to whoever pressed the button. Directly under that +sentence it embeds an iframe that runs the activation a second time with errors on display, so the +raw `Cannot redeclare …` prints inside the same notice box, contradicting whatever explanation sits +above it. + +So the library filters `wp_admin_notice_markup`, swaps that sentence for the sub-plugin's +`conflict_notice_message` — falling back to a generic one naming the slug — and takes the sandbox +iframe out with it. This is what puts the WordPress floor at 6.4: the filter does not exist before +it. + +**The same conflict has a second screen.** A fatal in the activation sandbox leaves the standalone +*paused*, so it returns to the plugins list with a **Resume** link, and pressing that fails +identically — worded *"Plugin could not be resumed because it triggered a fatal error."* Both +sentences are rewritten. + +It touches nothing else. The markup comes back exactly as it arrived unless every one of these holds +— the screen is `plugins`, or `plugins-network` in the network admin; `_error_nonce` verifies for a +standalone this library has registered, named by the `plugin` query arg on the activation screen and +by the nonce alone on the resume one, which carries no plugin name; and one of core's two sentences +is still there to replace. A notice failing any of them keeps its iframe as well: where this library +has nothing to say, core's diagnostic is the only thing that does. The replacement runs through `wp_kses_post()`, so a knowledge-base link survives, and a message that filters down to nothing leaves core's wording in place rather than blanking the notice. A host that diff --git a/src/Conflict/Rewriter.php b/src/Conflict/Rewriter.php index a0a6bb0..926036b 100644 --- a/src/Conflict/Rewriter.php +++ b/src/Conflict/Rewriter.php @@ -20,6 +20,11 @@ * useless to whoever pressed the button. All this library gets to do about it is reword that one * sentence, and the wording is the sub-plugin's own `conflict_notice_message`. * + * Rewording is not the whole of that screen, though. Core appends an `error_scrape` iframe to the + * sentence, and the iframe re-runs the activation sandbox with `display_errors` forced on — so the + * raw `Cannot redeclare …` fatal prints directly beneath the friendly explanation unless the iframe + * comes out with the sentence. + * * Its own class rather than a method on the notice writer, though the message is shared with the * merge notice. Nothing here is stored, drawn or queued: it reads the request, checks the screen, * verifies a nonce and edits markup core wrote. The writer would have needed a registry to find out @@ -54,8 +59,9 @@ public function __construct( Reader $registry ) { /** * The markup to print in place of the one WordPress was about to. * - * Handed back untouched unless the request really is a nonce-verified activation error, on a - * plugins screen, for a standalone this library has registered. + * Handed back untouched unless the request really is a nonce-verified activation or resume + * error, on a plugins screen, for a standalone this library has registered — and unless one of + * the two sentences core reports a plugin fatal with is still there to swap out. * * @since 1.0.0 * @@ -81,43 +87,21 @@ public function rewrite( string $markup ): string { return $markup; } - // phpcs:disable WordPress.Security.NonceVerification.Recommended -- verified below, once - // the plugin named turns out to be one this library owns. Nothing is acted on until then. - $basename = isset( $_GET['plugin'] ) ? wp_unslash( $_GET['plugin'] ) : ''; + // phpcs:ignore WordPress.Security.NonceVerification.Recommended -- this read *is* the nonce. + $nonce = isset( $_GET['_error_nonce'] ) + ? sanitize_text_field( wp_unslash( $_GET['_error_nonce'] ) ) + : ''; - // Unslashed and no further. Core mints the activation-error nonce from - // wp_unslash( $_REQUEST['plugin'] ) verbatim (wp-admin/plugins.php), so sanitizing here - // would verify an action core never signed: a plugin whose folder name holds a '%xx' - // sequence, a '<' or a leading space comes back changed from sanitize_text_field(), and - // both the nonce check and the registry lookup below would then miss -- silently, and on - // the one screen this class exists to improve. Nothing sanitizing would remove is needed - // here either: the value is compared against a basename the host configured and hashed into - // a nonce action, and never reaches the page. What does reach it is the sub-plugin's message. - // - // is_string() because sanitize_text_field() was doing that job: '?plugin[]=x' arrives as an - // array, and an array reaching wp_verify_nonce() is a string conversion, not a refusal. - if ( ! is_string( $basename ) || $basename === '' ) { + if ( $nonce === '' ) { return $markup; } - // Looked up before the nonce is checked, deliberately: there is no nonce work to do for a - // plugin this library does not own, and the nonce is still verified before any markup is - // touched. - $sub_plugin = $this->find_by_standalone_basename( $basename ); + $sub_plugin = $this->find_by_activation_error( $nonce ) ?? $this->find_by_resume_error( $nonce ); if ( $sub_plugin === null ) { return $markup; } - $nonce = isset( $_GET['_error_nonce'] ) - ? sanitize_text_field( wp_unslash( $_GET['_error_nonce'] ) ) - : ''; - // phpcs:enable WordPress.Security.NonceVerification.Recommended - - if ( ! wp_verify_nonce( $nonce, 'plugin-activation-error_' . $basename ) ) { - return $markup; - } - $message = $sub_plugin->get_conflict_notice_message( sprintf( '%s is bundled with this plugin and loads automatically. The standalone copy cannot' @@ -137,10 +121,122 @@ public function rewrite( string $markup ): string { return $markup; } - // phpcs:ignore WordPress.WP.I18n.TextDomainMismatch -- core's own string, matched on purpose. - $core_text = __( 'Plugin could not be activated because it triggered a fatal error.', 'default' ); + foreach ( $this->sentences_core_reports_a_fatal_with() as $sentence ) { + if ( strpos( $markup, $sentence ) === false ) { + continue; + } + + return $this->without_the_error_scrape( str_replace( $sentence, $message, $markup ) ); + } + + // Neither sentence is in there, so this notice was authored by something other than the + // screen this class knows how to improve. The iframe stays with it: a notice nobody + // explained is bad, and a notice nobody explained with its one diagnostic quietly deleted + // is worse. + return $markup; + } + + /** + * The two sentences core reports a sandboxed plugin fatal with, exactly as it prints them. + * + * Read back through `__()` against core's own text domain rather than written out as literals, + * so the swap still happens on a site running WordPress in another language: `wp_admin_notice()` + * receives the translated sentence, and an English needle would match nothing there. + * + * The resume wording belongs beside the activation one because the conflict outlives the + * activation screen. Core's fatal-error handler pauses the plugin its sandbox died in, so the + * standalone comes back on the plugins list with a Resume link, and pressing that re-runs the + * same sandbox into the same re-declaration — reported just as uselessly, in a different verb. + * + * @since 1.0.0 + * + * @return string[] + */ + private function sentences_core_reports_a_fatal_with(): array { + return [ + // phpcs:ignore WordPress.WP.I18n.TextDomainMismatch -- core's own string, matched on purpose. + __( 'Plugin could not be activated because it triggered a fatal error.', 'default' ), + // phpcs:ignore WordPress.WP.I18n.TextDomainMismatch -- core's own string, matched on purpose. + __( 'Plugin could not be resumed because it triggered a fatal error.', 'default' ), + ]; + } + + /** + * The registered sub-plugin an activation-error request names, if the nonce agrees. + * + * The `plugin` argument is unslashed and no further. Core mints the activation-error nonce from + * `wp_unslash( $_REQUEST['plugin'] )` verbatim (wp-admin/plugins.php), so sanitizing here would + * verify an action core never signed: a plugin whose folder name holds a '%xx' sequence, a '<' + * or a leading space comes back changed from sanitize_text_field(), and both the nonce check and + * the registry lookup would then miss -- silently, and on the one screen this class exists to + * improve. Nothing sanitizing would remove is needed here either: the value is compared against + * a basename the host configured and hashed into a nonce action, and never reaches the page. + * What does reach it is the sub-plugin's message. + * + * is_string() because sanitize_text_field() was doing that job: '?plugin[]=x' arrives as an + * array, and an array reaching wp_verify_nonce() is a string conversion, not a refusal. + * + * The registry is read before the nonce is verified, deliberately: there is no nonce work to do + * for a plugin this library does not own, and the nonce is still verified before any markup is + * touched. + * + * @since 1.0.0 + * + * @param string $nonce The `_error_nonce` this request carries. + * + * @return Sub_Plugin|null + */ + private function find_by_activation_error( string $nonce ): ?Sub_Plugin { + // phpcs:ignore WordPress.Security.NonceVerification.Recommended -- verified below, once the + // plugin named turns out to be one this library owns. Nothing is acted on until then. + $basename = isset( $_GET['plugin'] ) ? wp_unslash( $_GET['plugin'] ) : ''; + + if ( ! is_string( $basename ) || $basename === '' ) { + return null; + } + + $sub_plugin = $this->find_by_standalone_basename( $basename ); + + if ( $sub_plugin === null ) { + return null; + } + + return wp_verify_nonce( $nonce, 'plugin-activation-error_' . $basename ) ? $sub_plugin : null; + } + + /** + * The registered sub-plugin a resume-error request is about, if the nonce agrees. + * + * Identified from the nonce alone, because core's resume redirect carries no `plugin` argument + * to read: `resume_plugin()` appends `_error_nonce` and nothing else, minted from + * `plugin-resume-error_` and the basename. The nonce is therefore the only thing on the request + * that names a plugin, so every registered standalone is offered to it in turn and the one it + * was signed for answers. A forged value still matches nothing -- being unable to name the + * plugin does not make the check weaker, only the loop longer. + * + * A sub-plugin with no standalone configured is skipped rather than tested against an action + * that ends in nothing, which is a question about a plugin that does not exist. + * + * @since 1.0.0 + * + * @param string $nonce The `_error_nonce` this request carries. + * + * @return Sub_Plugin|null + */ + private function find_by_resume_error( string $nonce ): ?Sub_Plugin { + foreach ( $this->registry->all() as $sub_plugin ) { + if ( ! $sub_plugin->has_standalone_plugin() ) { + continue; + } + + $action = 'plugin-resume-error_' . $sub_plugin->get_standalone_plugin_basename(); + + if ( wp_verify_nonce( $nonce, $action ) ) { + return $sub_plugin; + } + } - return str_replace( $core_text, $message, $markup ); + return null; } /** @@ -166,4 +262,34 @@ private function find_by_standalone_basename( string $basename ): ?Sub_Plugin { return null; } + + /** + * The same notice with core's activation-sandbox iframe taken out of it. + * + * Core appends `#i', '', $markup ); + + // Null means the match itself failed -- a backtrack limit, or invalid UTF-8 carried in by + // the message just substituted. The reworded sentence is worth keeping on its own. + return is_string( $stripped ) ? $stripped : $markup; + } } diff --git a/tests/unit/Conflict/RewriterTest.php b/tests/unit/Conflict/RewriterTest.php index d2e2a38..c48b3e7 100644 --- a/tests/unit/Conflict/RewriterTest.php +++ b/tests/unit/Conflict/RewriterTest.php @@ -20,7 +20,8 @@ * The one conflict the load guard cannot prevent: WordPress includes the standalone after the * bundled copy has already loaded, the re-declaration is a real fatal, and core's activation sandbox * reports it as "the plugin triggered a fatal error" — true, and useless to whoever pressed the - * button. This class covers rewriting that sentence, and every reason not to. + * button. This class covers rewriting that sentence, taking core's sandbox iframe out from under it, + * and every reason not to do either. * * @since 1.0.0 */ @@ -35,7 +36,8 @@ class RewriterTest extends WPTestCase { private const STANDALONE = 'give-recurring/give-recurring.php'; /** - * Core's own sentence, spelled out rather than built with `__()`. + * Core's sentence for a plugin that fataled while being activated, spelled out rather than built + * with `__()`. * * Restating it is the point: the rewrite is a `str_replace()` against this exact string, so if * core ever rewords it the replacement silently stops happening, and a test that asked core for @@ -43,14 +45,25 @@ class RewriterTest extends WPTestCase { * * @var string */ - private const CORE_TEXT = 'Plugin could not be activated because it triggered a fatal error.'; + private const ACTIVATION_TEXT = 'Plugin could not be activated because it triggered a fatal error.'; + + /** + * Core's other wording for the same fatal, printed when recovery mode fails to resume the + * plugin (`wp-admin/plugins.php`, the `'resuming' === $_GET['error']` branch). + * + * @var string + */ + private const RESUME_TEXT = 'Plugin could not be resumed because it triggered a fatal error.'; /** * The notice core is about to print, as `wp_admin_notice_markup` hands it over. * + * Rebuilt per test rather than held as a constant, because the iframe core appends carries the + * request's own plugin name and nonce, and a constant cannot call `admin_url()`. + * * @var string */ - private const MARKUP = '

' . self::CORE_TEXT . '

'; + private $markup = ''; public function setUp(): void { parent::setUp(); @@ -64,15 +77,18 @@ public function setUp(): void { // Every test starts from the request core actually redirects to after a sandboxed fatal, and // states only the part it is about. + $_GET['error'] = 'true'; $_GET['plugin'] = self::STANDALONE; $_GET['_error_nonce'] = wp_create_nonce( 'plugin-activation-error_' . self::STANDALONE ); + + $this->markup = $this->activation_error_notice(); } public function tearDown(): void { // In tearDown rather than at the end of each test body: a failed assertion would otherwise // leave an admin screen and a half-built activation-error request standing for every test // that runs afterwards in this process. - unset( $_GET['plugin'], $_GET['_error_nonce'] ); + unset( $_GET['error'], $_GET['plugin'], $_GET['_error_nonce'] ); set_current_screen( 'front' ); Config_State::reset(); @@ -84,24 +100,114 @@ public function test_it_replaces_the_fatal_error_text_with_the_configured_messag $this->standalone_owner( [ 'conflict_notice_message' => static fn() => 'Give Recurring is already bundled with Give.' ] ) ); - $filtered = $rewriter->rewrite( self::MARKUP ); + $filtered = $rewriter->rewrite( $this->markup ); $this->assertStringContainsString( 'Give Recurring is already bundled with Give.', $filtered ); - $this->assertStringNotContainsString( self::CORE_TEXT, $filtered ); + $this->assertStringNotContainsString( self::ACTIVATION_TEXT, $filtered ); - // The notice box stays core's to draw — its classes, its dismiss button, its wrapper. Only - // the sentence inside belongs to this library. - $this->assertStringStartsWith( '

', $filtered ); + // The notice box stays core's to draw — its id, its classes, its wrapper. Only what is + // inside the paragraph belongs to this library. + $this->assertStringStartsWith( '

', $filtered ); $this->assertStringEndsWith( '

', $filtered ); } public function test_the_default_names_the_sub_plugin(): void { - $filtered = $this->make_rewriter( $this->standalone_owner() )->rewrite( self::MARKUP ); + $filtered = $this->make_rewriter( $this->standalone_owner() )->rewrite( $this->markup ); // The fallback is not pinned word for word — it is allowed to be reworded, as long as it // still names the sub-plugin and still displaces core's sentence. $this->assertStringContainsString( 'give-recurring', $filtered ); - $this->assertStringNotContainsString( self::CORE_TEXT, $filtered ); + $this->assertStringNotContainsString( self::ACTIVATION_TEXT, $filtered ); + } + + /** + * Rewording the sentence is only half the screen. Core appends an iframe requesting + * `plugins.php?action=error_scrape`, which runs `plugin_sandbox_scrape()` again with + * `display_errors` forced on — so the raw `Cannot redeclare …` fatal prints inside the same + * notice box, immediately under the friendly explanation that just said the situation is + * handled. Whichever of the two the owner believes, one of them wasted their afternoon. + */ + public function test_it_removes_the_activation_sandbox_iframe(): void { + // The fixture really is core's: the iframe is in it before the rewrite runs. + $this->assertStringContainsString( 'error_scrape', $this->markup ); + $this->assertStringContainsString( 'markup ); + + $rewriter = $this->make_rewriter( $this->standalone_owner( [ 'conflict_notice_message' => static fn() => 'Ours.' ] ) ); + + $filtered = $rewriter->rewrite( $this->markup ); + + $this->assertStringContainsString( 'Ours.', $filtered ); + $this->assertStringNotContainsString( 'assertStringNotContainsString( 'error_scrape', $filtered ); + + // Nothing else core wrote went with it. Asserted against core's own builder rather than a + // literal box, so this stays a claim about the iframe and not about the day core adds a + // class to its notices. + $this->assertSame( $this->notice_box( 'Ours.' ), $filtered ); + } + + /** + * The removal is aimed at one request in wp-admin — the one that re-runs the fatal — and not at + * iframes. Another plugin filtering this notice ahead of us is entitled to have put something of + * its own in it, and deleting that would be this library breaking a screen it came to fix. + */ + public function test_it_leaves_an_iframe_that_is_not_the_sandbox_scrape_alone(): void { + $foreign = ''; + + $rewriter = $this->make_rewriter( $this->standalone_owner( [ 'conflict_notice_message' => static fn() => 'Ours.' ] ) ); + + $filtered = $rewriter->rewrite( + str_replace( '

', $foreign . '

', $this->markup ) + ); + + $this->assertStringContainsString( 'Ours.', $filtered ); + $this->assertStringContainsString( $foreign, $filtered ); + $this->assertStringNotContainsString( 'error_scrape', $filtered ); + } + + /** + * Core's second wording for the same conflict. Its fatal-error handler pauses the plugin the + * activation sandbox died in, so the standalone reappears on the plugins list with a Resume + * link; pressing it re-runs the same sandbox into the same re-declaration, and core reports + * "could not be resumed" instead of "could not be activated". Matching only the activation + * wording leaves the owner reading core's useless sentence on the second screen after having + * been given a real explanation on the first. + */ + public function test_it_replaces_the_resume_wording(): void { + $this->arrange_resume_error(); + + $markup = $this->resume_error_notice(); + $rewriter = $this->make_rewriter( $this->standalone_owner( [ 'conflict_notice_message' => static fn() => 'Ours.' ] ) ); + + $filtered = $rewriter->rewrite( $markup ); + + $this->assertStringContainsString( 'Ours.', $filtered ); + $this->assertStringNotContainsString( self::RESUME_TEXT, $filtered ); + $this->assertStringStartsWith( '

', $filtered ); + } + + /** + * The resume redirect carries no `plugin` argument at all — `resume_plugin()` appends only an + * `_error_nonce`, minted from `plugin-resume-error_` and the basename — so the nonce is the one + * thing on the request naming a plugin, and a nonce signed for somebody else's names somebody + * else's. + */ + public function test_it_leaves_the_resume_markup_alone_for_a_plugin_no_sub_plugin_claims(): void { + $this->arrange_resume_error(); + + $markup = $this->resume_error_notice(); + + $this->assertStringContainsString( + 'Ours.', + $this->make_rewriter( $this->standalone_owner( [ 'conflict_notice_message' => static fn() => 'Ours.' ] ) ) + ->rewrite( $markup ) + ); + + $_GET['_error_nonce'] = wp_create_nonce( 'plugin-resume-error_akismet/akismet.php' ); + + $rewriter = $this->make_rewriter( $this->standalone_owner( [ 'conflict_notice_message' => static fn() => 'Ours.' ] ) ); + + $this->assertSame( $markup, $rewriter->rewrite( $markup ) ); } /** @@ -124,7 +230,7 @@ public function test_it_uses_the_sub_plugin_whose_standalone_the_request_names() $_GET['plugin'] = 'give-fee-recovery/give-fee-recovery.php'; $_GET['_error_nonce'] = wp_create_nonce( 'plugin-activation-error_give-fee-recovery/give-fee-recovery.php' ); - $filtered = $rewriter->rewrite( self::MARKUP ); + $filtered = $rewriter->rewrite( $this->activation_error_notice() ); $this->assertStringContainsString( 'The right one.', $filtered ); $this->assertStringNotContainsString( 'The wrong one.', $filtered ); @@ -154,7 +260,7 @@ public function test_it_rewrites_for_a_standalone_whose_basename_sanitizing_woul $_GET['plugin'] = $standalone; $_GET['_error_nonce'] = wp_create_nonce( 'plugin-activation-error_' . $standalone ); - $this->assertStringContainsString( 'Ours.', $rewriter->rewrite( self::MARKUP ) ); + $this->assertStringContainsString( 'Ours.', $rewriter->rewrite( $this->activation_error_notice() ) ); } /** @@ -182,7 +288,7 @@ public function test_it_leaves_the_markup_alone_outside_the_admin(): void { set_current_screen( 'front' ); - $this->assertSame( self::MARKUP, $rewriter->rewrite( self::MARKUP ) ); + $this->assertSame( $this->markup, $rewriter->rewrite( $this->markup ) ); } /** @@ -197,7 +303,7 @@ public function test_it_leaves_the_markup_alone_off_the_plugins_screen(): void { set_current_screen( 'dashboard' ); - $this->assertSame( self::MARKUP, $rewriter->rewrite( self::MARKUP ) ); + $this->assertSame( $this->markup, $rewriter->rewrite( $this->markup ) ); } /** @@ -211,13 +317,14 @@ public function test_it_rewrites_the_markup_on_the_network_plugins_screen(): voi set_current_screen( 'plugins-network' ); - $this->assertStringContainsString( 'Ours.', $rewriter->rewrite( self::MARKUP ) ); + $this->assertStringContainsString( 'Ours.', $rewriter->rewrite( $this->markup ) ); } /** * The nonce is valid here, so the ownership lookup is the only thing that can stop the rewrite: * an activation error for a plugin this library knows nothing about keeps core's wording, which - * for that plugin is the accurate one. + * for that plugin is the accurate one — and keeps the sandbox iframe, which for that plugin is + * the only diagnostic anyone has. */ public function test_it_leaves_the_markup_alone_for_a_plugin_no_sub_plugin_claims(): void { $this->assert_the_arrangement_rewrites(); @@ -227,7 +334,12 @@ public function test_it_leaves_the_markup_alone_for_a_plugin_no_sub_plugin_claim $_GET['plugin'] = 'akismet/akismet.php'; $_GET['_error_nonce'] = wp_create_nonce( 'plugin-activation-error_akismet/akismet.php' ); - $this->assertSame( self::MARKUP, $rewriter->rewrite( self::MARKUP ) ); + $markup = $this->activation_error_notice(); + + $filtered = $rewriter->rewrite( $markup ); + + $this->assertSame( $markup, $filtered ); + $this->assertStringContainsString( 'error_scrape', $filtered ); } /** @@ -242,7 +354,7 @@ public function test_it_leaves_the_markup_alone( callable $arrange ): void { $arrange(); - $this->assertSame( self::MARKUP, $rewriter->rewrite( self::MARKUP ) ); + $this->assertSame( $this->markup, $rewriter->rewrite( $this->markup ) ); } /** @@ -299,11 +411,31 @@ static function (): void { ]; } + /** + * A notice carrying neither of core's two sentences was authored by something else, and this + * library has nothing to say about it. The iframe stays with it: a notice nobody explained is + * bad, and a notice nobody explained with its one diagnostic quietly deleted is worse. + */ + public function test_it_leaves_a_notice_holding_neither_sentence_alone(): void { + $this->assert_the_arrangement_rewrites(); + + $rewriter = $this->make_rewriter( $this->standalone_owner( [ 'conflict_notice_message' => static fn() => 'Ours.' ] ) ); + + $markup = str_replace( self::ACTIVATION_TEXT, 'Something else went wrong.', $this->markup ); + + $filtered = $rewriter->rewrite( $markup ); + + $this->assertSame( $markup, $filtered ); + $this->assertStringContainsString( 'error_scrape', $filtered ); + } + /** * The message is sanitised before it is checked for emptiness, and this is the case that pins * the order: `wp_kses_post( '' )` is the empty string, so swapping core's * wording for it would leave an empty notice box where the explanation should be. Leaving - * core's sentence in place is the better of the two bad outcomes. + * core's sentence in place is the better of the two bad outcomes — and the iframe stays under + * it, because a screen still showing core's wording is a screen this library did not improve, + * and its raw fatal is the only thing left explaining anything. */ public function test_a_message_that_sanitises_away_leaves_the_markup_alone(): void { $this->assert_the_arrangement_rewrites(); @@ -312,7 +444,10 @@ public function test_a_message_that_sanitises_away_leaves_the_markup_alone(): vo $this->standalone_owner( [ 'conflict_notice_message' => static fn() => '' ] ) ); - $this->assertSame( self::MARKUP, $rewriter->rewrite( self::MARKUP ) ); + $filtered = $rewriter->rewrite( $this->markup ); + + $this->assertSame( $this->markup, $filtered ); + $this->assertStringContainsString( 'error_scrape', $filtered ); } /** @@ -323,7 +458,7 @@ public function test_a_whitespace_only_message_leaves_the_markup_alone(): void { $rewriter = $this->make_rewriter( $this->standalone_owner( [ 'conflict_notice_message' => static fn() => " \n\t" ] ) ); - $this->assertSame( self::MARKUP, $rewriter->rewrite( self::MARKUP ) ); + $this->assertSame( $this->markup, $rewriter->rewrite( $this->markup ) ); } /** @@ -342,7 +477,7 @@ public function test_it_strips_unsafe_markup_from_the_replacement_but_keeps_a_li ) ); - $filtered = $rewriter->rewrite( self::MARKUP ); + $filtered = $rewriter->rewrite( $this->markup ); $this->assertStringContainsString( 'the docs', $filtered ); $this->assertStringNotContainsString( 'onclick', $filtered ); @@ -366,7 +501,78 @@ private function assert_the_arrangement_rewrites(): void { $this->assertStringContainsString( 'Ours.', $this->make_rewriter( $this->standalone_owner( [ 'conflict_notice_message' => static fn() => 'Ours.' ] ) ) - ->rewrite( self::MARKUP ) + ->rewrite( $this->markup ) + ); + } + + /** + * Turn the request in setUp() into the one core redirects to when recovery mode cannot resume a + * plugin: `resume_plugin()` appends an `_error_nonce` for `plugin-resume-error_` to + * `plugins.php?error=resuming&…`, and there is no `plugin` argument anywhere on it. + */ + private function arrange_resume_error(): void { + unset( $_GET['plugin'] ); + + $_GET['error'] = 'resuming'; + $_GET['_error_nonce'] = wp_create_nonce( 'plugin-resume-error_' . self::STANDALONE ); + } + + /** + * The activation-error notice as `wp-admin/plugins.php` assembles it, for whatever plugin and + * nonce the request currently names. + * + * Transcribed from core rather than approximated, because both halves of the rewrite are + * decided by what is really in that string: the sentence core's `else` branch chooses, and the + * `error_scrape` iframe appended after it whenever the request carries a verifying + * `_error_nonce` — which, on every request this class acts on, it does. A fixture ending at the + * sentence would leave the iframe removal proven by nothing. + */ + private function activation_error_notice(): string { + $plugin = isset( $_GET['plugin'] ) && is_string( $_GET['plugin'] ) ? $_GET['plugin'] : ''; + $nonce = isset( $_GET['_error_nonce'] ) && is_string( $_GET['_error_nonce'] ) ? $_GET['_error_nonce'] : ''; + + $iframe_url = add_query_arg( + [ + 'action' => 'error_scrape', + 'plugin' => urlencode( $plugin ), + '_wpnonce' => urlencode( $nonce ), + ], + admin_url( 'plugins.php' ) + ); + + return $this->notice_box( + self::ACTIVATION_TEXT + . '' + ); + } + + /** + * The resume-error notice as `wp-admin/plugins.php` assembles it. + * + * No iframe under this one, and that is core's doing rather than a shortcut here: the iframe is + * appended only when `_error_nonce` verifies against `plugin-activation-error_` plus the + * `plugin` argument, and the resume redirect carries neither a `plugin` argument nor that kind + * of nonce. + */ + private function resume_error_notice(): string { + return $this->notice_box( self::RESUME_TEXT ); + } + + /** + * The box core wraps either sentence in, built by core's own function so the id, the classes and + * the paragraph wrap are whatever this WordPress really produces. + * + * @param string $errmsg The message core assembled. + * + * @return string + */ + private function notice_box( string $errmsg ): string { + return wp_get_admin_notice( + $errmsg, + [ + 'id' => 'message', + 'additional_classes' => [ 'error' ], + ] ); } From 2041550b788b55233177a644d9ca41d88cf0106e Mon Sep 17 00:00:00 2001 From: Nikolay Strikhar Date: Mon, 24 Aug 2026 14:28:59 +0200 Subject: [PATCH 2/5] Ask for a resume nonce only on a resume error, and prove the iframe goes end to end --- src/Conflict/Rewriter.php | 23 ++++++++++- tests/README.md | 15 ++++--- tests/unit/Conflict/RewriterTest.php | 49 ++++++++++++++++++++++ tests/unit/Scenario/ConflictTest.php | 62 ++++++++++++++++++++++------ 4 files changed, 130 insertions(+), 19 deletions(-) diff --git a/src/Conflict/Rewriter.php b/src/Conflict/Rewriter.php index 926036b..38e9b52 100644 --- a/src/Conflict/Rewriter.php +++ b/src/Conflict/Rewriter.php @@ -214,6 +214,15 @@ private function find_by_activation_error( string $nonce ): ?Sub_Plugin { * was signed for answers. A forged value still matches nothing -- being unable to name the * plugin does not make the check weaker, only the loop longer. * + * Behind `error=resuming`, which is not a second guess at the same thing. Without it the loop + * ran on every plugins-screen request carrying an `_error_nonce` the activation lookup missed -- + * an ordinary activation failure for somebody else's plugin -- and each miss fires core's + * `wp_verify_nonce_failed`, which security plugins hook to count and rate-limit failed nonces. A + * site with five sub-plugins produced five of them on a screen this library has nothing to do + * with. The gate costs nothing, because it is the request core itself dispatches the resume + * wording on: `resume_plugin()` is only ever handed `plugins.php?error=resuming`, and + * `wp-admin/plugins.php` words the notice off exactly that value. + * * A sub-plugin with no standalone configured is skipped rather than tested against an action * that ends in nothing, which is a question about a plugin that does not exist. * @@ -224,6 +233,14 @@ private function find_by_activation_error( string $nonce ): ?Sub_Plugin { * @return Sub_Plugin|null */ private function find_by_resume_error( string $nonce ): ?Sub_Plugin { + // phpcs:ignore WordPress.Security.NonceVerification.Recommended -- which screen this is, not + // an act to authorise; the nonce below is what authorises. + $error = isset( $_GET['error'] ) ? sanitize_text_field( wp_unslash( $_GET['error'] ) ) : ''; + + if ( $error !== 'resuming' ) { + return null; + } + foreach ( $this->registry->all() as $sub_plugin ) { if ( ! $sub_plugin->has_standalone_plugin() ) { continue; @@ -288,8 +305,10 @@ private function find_by_standalone_basename( string $basename ): ?Sub_Plugin { private function without_the_error_scrape( string $markup ): string { $stripped = preg_replace( '#]*\berror_scrape\b[^>]*>\s*#i', '', $markup ); - // Null means the match itself failed -- a backtrack limit, or invalid UTF-8 carried in by - // the message just substituted. The reworded sentence is worth keeping on its own. + // Null means the match itself failed -- a backtrack or recursion limit on a notice far larger + // than the one core writes. The pattern carries no `u` modifier, so PCRE runs on bytes here + // and whatever encoding the substituted message arrived in cannot fail it. The reworded + // sentence is worth keeping on its own. return is_string( $stripped ) ? $stripped : $markup; } } diff --git a/tests/README.md b/tests/README.md index 6c0dff5..1c92e0f 100644 --- a/tests/README.md +++ b/tests/README.md @@ -875,10 +875,15 @@ load guard cannot prevent: the owner reinstalls the standalone and presses Activate, WordPress includes it on top of the bundled copy, and the re-declaration is a real fatal that core's sandbox reports as "the plugin triggered a fatal error" — true, and useless. All the library gets to do is -reword the sentence. Driven through core's own filter dispatch rather than by -calling the rewriter, because the admin-only `add_filter()` is half of what has -to work. The notice box stays core's — its classes, its dismiss button, its -wrapper; only the sentence inside is ours. +reword the sentence — either of core's two, since the same box carries "could +not be resumed" out of recovery mode — and take core's `error_scrape` iframe +down with it, which would otherwise re-include the standalone in a sandbox and +print the re-declaration fatal under the message that just explained it away. +Driven through core's own filter dispatch rather than by calling the rewriter, +because the admin-only `add_filter()` is half of what has to work, and off a +fixture core's own `wp_get_admin_notice()` assembled, iframe included — an +invented notice would leave the removal proven by nothing. The notice box stays +core's: its id, its classes, its wrapper; only the sentence inside is ours. ```mermaid sequenceDiagram @@ -892,7 +897,7 @@ sequenceDiagram WP->>WP: redirects to plugins.php with plugin and _error_nonce WP->>Rw: wp_admin_notice_markup filter Rw->>Rw: screen is plugins, arg names a registered standalone, nonce verifies - Rw-->>WP: core's sentence swapped for the host's, wrapper untouched + Rw-->>WP: core's sentence swapped for the host's, error_scrape iframe removed, wrapper untouched ``` #### `Scenario/HostTest.php` — the host's own wiring diff --git a/tests/unit/Conflict/RewriterTest.php b/tests/unit/Conflict/RewriterTest.php index c48b3e7..8c1f0f6 100644 --- a/tests/unit/Conflict/RewriterTest.php +++ b/tests/unit/Conflict/RewriterTest.php @@ -210,6 +210,55 @@ public function test_it_leaves_the_resume_markup_alone_for_a_plugin_no_sub_plugi $this->assertSame( $markup, $rewriter->rewrite( $markup ) ); } + /** + * The resume lookup is the only one that has to try every registered standalone in turn, because + * the resume redirect carries no `plugin` argument to narrow it with — and every miss fires + * core's `wp_verify_nonce_failed`, which security plugins hook to count and rate-limit failed + * nonces. So it may only run on the request core dispatches the resume wording on. Here the + * request is an ordinary activation error for somebody else's plugin, which is the shape a site + * produces whenever any plugin fatals on activation: the rewrite declines, and it declines + * without asking a single question about our standalone's nonce. + */ + public function test_it_does_not_hunt_for_a_resume_nonce_outside_a_resume_error(): void { + $asked = []; + + add_action( + 'wp_verify_nonce_failed', + static function ( $nonce, $action ) use ( &$asked ) { + $asked[] = $action; + }, + 10, + 2 + ); + + // Somebody else's plugin fatalled on activation: error=true, their basename, their nonce. + $_GET['error'] = 'true'; + $_GET['plugin'] = 'akismet/akismet.php'; + $_GET['_error_nonce'] = wp_create_nonce( 'plugin-activation-error_akismet/akismet.php' ); + + $markup = $this->activation_error_notice(); + $rewriter = $this->make_rewriter( $this->standalone_owner() ); + + $this->assertSame( $markup, $rewriter->rewrite( $markup ) ); + $this->assertSame( + [], + $asked, + 'A plugins screen about somebody else\'s plugin must not spend a nonce check per' + . ' registered standalone, one failed-nonce action each.' + ); + + // The recorder works: the same rewrite on a real resume request does ask, so the assertion + // above is about the gate rather than about a listener that never attached. + $this->arrange_resume_error(); + $_GET['_error_nonce'] = wp_create_nonce( 'plugin-resume-error_akismet/akismet.php' ); + + $this->make_rewriter( $this->standalone_owner() )->rewrite( $this->resume_error_notice() ); + + $this->assertSame( [ 'plugin-resume-error_' . self::STANDALONE ], $asked ); + + remove_all_actions( 'wp_verify_nonce_failed' ); + } + /** * The request names one plugin, and only the sub-plugin that claims that basename may speak for * it. Reading the first registration instead would put one bundled plugin's explanation on diff --git a/tests/unit/Scenario/ConflictTest.php b/tests/unit/Scenario/ConflictTest.php index f8334c1..7330e78 100644 --- a/tests/unit/Scenario/ConflictTest.php +++ b/tests/unit/Scenario/ConflictTest.php @@ -32,12 +32,6 @@ class ConflictTest extends Bootstrap_Test_Case { */ private const CORE_TEXT = 'Plugin could not be activated because it triggered a fatal error.'; - /** - * The notice core is about to print, as `wp_admin_notice_markup` hands it over. - * - * @var string - */ - private const MARKUP = '

' . self::CORE_TEXT . '

'; /** * A second sub-plugin, for the scenarios that need the conflict to sit behind one. @@ -532,13 +526,22 @@ public function test_a_reactivation_attempt_yields_the_friendly_message(): void ] ); - $this->boot(); - // The request core redirects to once the sandboxed activation has fataled. $_GET['plugin'] = self::STANDALONE; $_GET['_error_nonce'] = wp_create_nonce( 'plugin-activation-error_' . self::STANDALONE ); - $rewritten = apply_filters( 'wp_admin_notice_markup', self::MARKUP, self::CORE_TEXT, [] ); + // Built before boot(), because wp_get_admin_notice() dispatches wp_admin_notice_markup + // itself: assembled afterwards, the fixture would arrive already rewritten and the dispatch + // below would be asserting against its own output. + $markup = $this->activation_error_notice(); + + // The arrangement is only worth asserting on if the fixture is the thing core really hands + // over, iframe included -- an approximation of it would leave the removal proven by nothing. + $this->assertStringContainsString( 'error_scrape', $markup ); + + $this->boot(); + + $rewritten = apply_filters( 'wp_admin_notice_markup', $markup, self::CORE_TEXT, [] ); $this->assertIsString( $rewritten, 'The filter must hand back markup, whatever it did with it.' ); @@ -547,8 +550,43 @@ public function test_a_reactivation_attempt_yields_the_friendly_message(): void $this->assertStringContainsString( 'Recurring is already bundled with the host plugin.', $filtered ); $this->assertStringNotContainsString( self::CORE_TEXT, $filtered ); - // The notice box stays core's to draw — its classes, its dismiss button, its wrapper. Only - // the sentence inside belongs to this library. - $this->assertStringStartsWith( '

', $filtered ); + // Core's own diagnostic goes with the sentence it explained. Left behind, the iframe + // re-includes the standalone in a sandbox and prints the re-declaration fatal underneath + // the friendly message that just said there was nothing wrong. + $this->assertStringNotContainsString( 'error_scrape', $filtered ); + + // The notice box stays core's to draw — its id, its classes, its wrapper. Only the sentence + // inside belongs to this library. + $this->assertStringStartsWith( '

', $filtered ); + } + + /** + * The activation-error notice as `wp-admin/plugins.php` assembles it: core's sentence, the + * `error_scrape` iframe it appends whenever the request carries a verifying `_error_nonce`, and + * core's own notice box around both. + * + * @return string + */ + private function activation_error_notice(): string { + $plugin = isset( $_GET['plugin'] ) && is_string( $_GET['plugin'] ) ? $_GET['plugin'] : ''; + $nonce = isset( $_GET['_error_nonce'] ) && is_string( $_GET['_error_nonce'] ) ? $_GET['_error_nonce'] : ''; + + $iframe_url = add_query_arg( + [ + 'action' => 'error_scrape', + 'plugin' => urlencode( $plugin ), + '_wpnonce' => urlencode( $nonce ), + ], + admin_url( 'plugins.php' ) + ); + + return wp_get_admin_notice( + self::CORE_TEXT + . '', + [ + 'id' => 'message', + 'additional_classes' => [ 'error' ], + ] + ); } } From 5c27e27757d4e1a8c6829d929ceebcb0594365d9 Mon Sep 17 00:00:00 2001 From: Nikolay Strikhar Date: Mon, 24 Aug 2026 14:36:23 +0200 Subject: [PATCH 3/5] Spell it the way the rest of the suite does --- tests/unit/Conflict/RewriterTest.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/unit/Conflict/RewriterTest.php b/tests/unit/Conflict/RewriterTest.php index 8c1f0f6..b7494cf 100644 --- a/tests/unit/Conflict/RewriterTest.php +++ b/tests/unit/Conflict/RewriterTest.php @@ -231,7 +231,7 @@ static function ( $nonce, $action ) use ( &$asked ) { 2 ); - // Somebody else's plugin fatalled on activation: error=true, their basename, their nonce. + // Somebody else's plugin fataled on activation: error=true, their basename, their nonce. $_GET['error'] = 'true'; $_GET['plugin'] = 'akismet/akismet.php'; $_GET['_error_nonce'] = wp_create_nonce( 'plugin-activation-error_akismet/akismet.php' ); From 5b17033d0307ad20227374bf757868ad348baf26 Mon Sep 17 00:00:00 2001 From: Nikolay Strikhar Date: Mon, 24 Aug 2026 14:40:17 +0200 Subject: [PATCH 4/5] Fail when core rewords a sentence this class replaces --- tests/unit/Conflict/RewriterTest.php | 30 ++++++++++++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/tests/unit/Conflict/RewriterTest.php b/tests/unit/Conflict/RewriterTest.php index b7494cf..d755fb8 100644 --- a/tests/unit/Conflict/RewriterTest.php +++ b/tests/unit/Conflict/RewriterTest.php @@ -210,6 +210,36 @@ public function test_it_leaves_the_resume_markup_alone_for_a_plugin_no_sub_plugi $this->assertSame( $markup, $rewriter->rewrite( $markup ) ); } + /** + * The canary for the two constants above. Both are transcriptions of sentences that live in + * somebody else's source file, and the rewrite is a `str_replace()` against them — so the day + * core rewords either one, the replacement silently stops happening and every other test in this + * class goes on passing, because they all match our transcription against our transcription. + * + * Read out of `wp-admin/plugins.php` rather than through `__()`, which would hand back whatever + * string was passed in and assert nothing at all. The failure this catches is benign on a site — + * no match means core's own wording stands — but it is invisible without this, and the whole + * class exists to replace that wording. + */ + public function test_core_still_words_the_fatal_the_way_this_class_expects(): void { + $plugins_screen = ABSPATH . 'wp-admin/plugins.php'; + + $this->assertFileExists( $plugins_screen ); + + $source = (string) file_get_contents( $plugins_screen ); + + $this->assertStringContainsString( + self::ACTIVATION_TEXT, + $source, + 'WordPress reworded the activation fatal. The rewrite no longer matches, and it fails silently.' + ); + $this->assertStringContainsString( + self::RESUME_TEXT, + $source, + 'WordPress reworded the resume fatal. The rewrite no longer matches, and it fails silently.' + ); + } + /** * The resume lookup is the only one that has to try every registered standalone in turn, because * the resume redirect carries no `plugin` argument to narrow it with — and every miss fires From 24bbacede5ff15a6c6a39e6b131eb3f881d0a798 Mon Sep 17 00:00:00 2001 From: Nikolay Strikhar Date: Mon, 24 Aug 2026 14:44:51 +0200 Subject: [PATCH 5/5] Leave hook cleanup to the teardown that already does it --- tests/unit/Conflict/RewriterTest.php | 2 -- tests/unit/Scenario/ConflictTest.php | 1 - 2 files changed, 3 deletions(-) diff --git a/tests/unit/Conflict/RewriterTest.php b/tests/unit/Conflict/RewriterTest.php index d755fb8..85a80d0 100644 --- a/tests/unit/Conflict/RewriterTest.php +++ b/tests/unit/Conflict/RewriterTest.php @@ -285,8 +285,6 @@ static function ( $nonce, $action ) use ( &$asked ) { $this->make_rewriter( $this->standalone_owner() )->rewrite( $this->resume_error_notice() ); $this->assertSame( [ 'plugin-resume-error_' . self::STANDALONE ], $asked ); - - remove_all_actions( 'wp_verify_nonce_failed' ); } /** diff --git a/tests/unit/Scenario/ConflictTest.php b/tests/unit/Scenario/ConflictTest.php index 7330e78..5acee38 100644 --- a/tests/unit/Scenario/ConflictTest.php +++ b/tests/unit/Scenario/ConflictTest.php @@ -32,7 +32,6 @@ class ConflictTest extends Bootstrap_Test_Case { */ private const CORE_TEXT = 'Plugin could not be activated because it triggered a fatal error.'; - /** * A second sub-plugin, for the scenarios that need the conflict to sit behind one. *