diff --git a/CHANGELOG.md b/CHANGELOG.md index 3233fb62a..92b1352cb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -74,6 +74,7 @@ snippet running under file-based execution. * Fixed the snippet editor reporting a save as failed and undelivered when no response was received, which it cannot determine. +* Fixed safe mode failing on sites running a plugin that builds URLs while WordPress is identifying the current user. ## [3.10.2] (2026-09-01) diff --git a/src/php/Integration/Evaluate_Functions.php b/src/php/Integration/Evaluate_Functions.php index 9034ab062..0dbd858be 100644 --- a/src/php/Integration/Evaluate_Functions.php +++ b/src/php/Integration/Evaluate_Functions.php @@ -68,7 +68,28 @@ public function is_safe_mode_query_var_set(): bool { * @return bool */ public function is_safe_mode_requested(): bool { - return $this->is_safe_mode_query_var_set() && code_snippets()->current_user_can(); + static $resolving = false; + + if ( ! $this->is_safe_mode_query_var_set() ) { + return false; + } + + // Asking for a capability makes WordPress resolve the current user, + // which runs third-party callbacks on determine_current_user. One that + // builds a URL arrives back here before that resolution has finished, + // and answering it again would restart it, recursing until the request + // ran out of memory. No user is known yet at that point, so it is no. + if ( $resolving ) { + return false; + } + + $resolving = true; + + try { + return code_snippets()->current_user_can(); + } finally { + $resolving = false; + } } /** @@ -81,6 +102,14 @@ public function is_safe_mode_requested(): bool { public function add_safe_mode_query_var( $url ): string { $url = is_string( $url ) ? $url : ''; + // A URL built before WordPress has settled on a user belongs to that + // resolution rather than to anything a visitor follows, so it is left + // alone. Safe mode links are generated while a page renders, which is + // long after this point. + if ( ! did_action( 'set_current_user' ) ) { + return $url; + } + return $this->is_safe_mode_requested() ? add_query_arg( 'snippets-safe-mode', true, $url ) : $url; diff --git a/tests/unit/Integration/Evaluate_Functions_Safe_Mode_Test.php b/tests/unit/Integration/Evaluate_Functions_Safe_Mode_Test.php index 1f86df8ac..8535cba57 100644 --- a/tests/unit/Integration/Evaluate_Functions_Safe_Mode_Test.php +++ b/tests/unit/Integration/Evaluate_Functions_Safe_Mode_Test.php @@ -12,6 +12,36 @@ */ class Evaluate_Functions_Safe_Mode_Test extends UnitTestCase { + /** + * Calls to the current-user filter after which it stops building URLs. + * + * Past anything a correct implementation produces, but low enough that a + * regression fails on the count rather than exhausting the memory limit and + * taking the whole test run down with it. + */ + private const RESOLUTION_LIMIT = 5; + + /** + * How many times the current-user filter was called during a test. + * + * @var int + */ + private int $resolution_count = 0; + + /** + * The last URL built while the current user was being resolved. + * + * @var string|null + */ + private ?string $resolved_url = null; + + /** + * User the current-user filter resolves to. + * + * @var int|false + */ + private $user_to_resolve = false; + /** * Set up before each test. * @@ -20,6 +50,9 @@ class Evaluate_Functions_Safe_Mode_Test extends UnitTestCase { public function set_up() { parent::set_up(); unset( $_REQUEST['snippets-safe-mode'] ); + $this->resolution_count = 0; + $this->resolved_url = null; + $this->user_to_resolve = false; } /** @@ -29,9 +62,42 @@ public function set_up() { */ public function tear_down() { unset( $_REQUEST['snippets-safe-mode'] ); + remove_filter( 'determine_current_user', [ $this, 'resolve_user_through_a_url' ], 15 ); parent::tear_down(); } + /** + * Stand in for a plugin that builds a URL while the current user is being resolved. + * + * WooCommerce does this: its REST authentication runs on determine_current_user + * and calls home_url() to work out whether the request is for one of its routes. + * + * @param int|false $user_id Resolved user, from an unknown earlier callback. + * + * @return int|false + */ + public function resolve_user_through_a_url( $user_id ) { + ++$this->resolution_count; + + if ( $this->resolution_count < self::RESOLUTION_LIMIT ) { + $this->resolved_url = home_url( '/' ); + } + + return false === $this->user_to_resolve ? $user_id : $this->user_to_resolve; + } + + /** + * Force WordPress to resolve the current user again on the next capability check. + * + * @return void + */ + private function require_user_resolution(): void { + global $current_user; + + // phpcs:ignore WordPress.WP.GlobalVariablesOverride.Prohibited -- emptying the cached user is the only way to make WordPress resolve one again, which is the whole point of these tests. + $current_user = null; + } + /** * The query var check must not touch capabilities. * @@ -88,4 +154,90 @@ public function test_execution_callback_tolerates_a_null_from_an_earlier_callbac $this->assertFalse( $evaluate->disable_snippet_execution( null ) ); $this->assertTrue( $evaluate->disable_snippet_execution( true ) ); } + + /** + * Resolving the current user must not re-enter the capability check. + * + * A plugin that builds a URL on determine_current_user reaches the URL + * filter before WordPress knows who the user is. Checking the capability + * again there restarts the resolution that is already running, which + * recurses until the request exhausts its memory limit. Any visitor can + * trigger it by putting the query var on a front-end URL. + * + * @return void + */ + public function test_url_filter_does_not_restart_user_resolution(): void { + $_REQUEST['snippets-safe-mode'] = '1'; + + $evaluate = new Evaluate_Functions( code_snippets()->db ); + + add_filter( 'determine_current_user', [ $this, 'resolve_user_through_a_url' ], 15 ); + $this->require_user_resolution(); + + $evaluate->disable_snippet_execution( true ); + + $this->assertSame( + 1, + $this->resolution_count, + 'the current user should be resolved once per request, not once per URL built while resolving' + ); + } + + /** + * A URL built during resolution comes back untouched. + * + * The user the request will turn out to belong to is not known while that + * URL is being built, so safe mode cannot yet be part of it, even when the + * user being resolved does hold the capability. + * + * @return void + */ + public function test_url_built_during_user_resolution_is_unchanged(): void { + $_REQUEST['snippets-safe-mode'] = '1'; + $this->user_to_resolve = $this->factory()->user->create( [ 'role' => 'administrator' ] ); + + $evaluate = new Evaluate_Functions( code_snippets()->db ); + + add_filter( 'determine_current_user', [ $this, 'resolve_user_through_a_url' ], 15 ); + $this->require_user_resolution(); + + $evaluate->disable_snippet_execution( true ); + + $this->assertIsString( $this->resolved_url ); + $this->assertStringNotContainsString( 'snippets-safe-mode', $this->resolved_url ); + } + + /** + * Safe mode still reaches the URLs an administrator follows. + * + * @return void + */ + public function test_query_var_is_added_for_a_user_with_the_capability(): void { + $_REQUEST['snippets-safe-mode'] = '1'; + wp_set_current_user( $this->factory()->user->create( [ 'role' => 'administrator' ] ) ); + + $evaluate = new Evaluate_Functions( code_snippets()->db ); + + $this->assertStringContainsString( + 'snippets-safe-mode=1', + $evaluate->add_safe_mode_query_var( 'https://example.org/wp-admin/' ) + ); + } + + /** + * Safe mode stays out of the URLs a visitor without the capability follows. + * + * @return void + */ + public function test_query_var_is_not_added_for_a_user_without_the_capability(): void { + $_REQUEST['snippets-safe-mode'] = '1'; + wp_set_current_user( 0 ); + + $evaluate = new Evaluate_Functions( code_snippets()->db ); + + $this->assertSame( + 'https://example.org/', + $evaluate->add_safe_mode_query_var( 'https://example.org/' ) + ); + } }