From ca4a1340034380ed98b00bc48db9d971cecf12dd Mon Sep 17 00:00:00 2001 From: David Stone Date: Sat, 9 May 2026 11:08:01 -0600 Subject: [PATCH] fix(site-duplication): balance switch_to_blog stack so welcome emails send MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit MUCD_Files::copy_files() pushed two switch_to_blog() frames (source then target) onto the WordPress blog stack but only popped one. After the function returned the caller was still on the source/template blog context instead of the original network/admin context. Customers reported that new-site / welcome emails were silently dropped during template-based signups while a manual "send test" from the network admin worked. The leak puts wp_mail() into the template site's context, so it picks up that site's options (admin_email, blogname) and any per-site SMTP plugin set — almost always different from the network configuration the test path uses. Add a second restore_current_blog() to fully unwind the stack, and add a regression test class that drives MUCD_Files::copy_files() directly and asserts the caller's blog id and the global _wp_switched_stack are both unchanged after the call. The full duplicate_site() pipeline gets the same context-preservation assertion as a higher-level guard. --- inc/duplication/files.php | 23 +++ .../Helpers/Site_Duplicator_Context_Test.php | 171 ++++++++++++++++++ 2 files changed, 194 insertions(+) create mode 100644 tests/WP_Ultimo/Helpers/Site_Duplicator_Context_Test.php diff --git a/inc/duplication/files.php b/inc/duplication/files.php index a8f6ea55c..4f013ad8b 100644 --- a/inc/duplication/files.php +++ b/inc/duplication/files.php @@ -23,6 +23,27 @@ class MUCD_Files { * @param int $to_site_id new site id. */ public static function copy_files($from_site_id, $to_site_id) { + /* + * Two switch_to_blog() calls are pushed onto the WordPress blog + * stack to read uploads info from the source and destination sites, + * so two restore_current_blog() calls must follow to fully unwind + * the stack. Previously only one restore was issued, leaving the + * caller's blog context pointing at the template/source site after + * copy_files() returned. + * + * The leak broke downstream code that relies on the network/admin + * context — most visibly the site_published email pipeline. When + * a customer signed up with a template the publish flow ran on the + * template's blog context, so wp_mail() picked up the template + * site's options (and per-site SMTP plugins) instead of the + * network's, causing welcome emails to silently fail. Sending a + * test email from the network admin worked because the admin + * request never went through duplication. + * + * @since 2.7.2 + * @see https://github.com/Ultimate-Multisite/ultimate-multisite/issues/1163 + */ + // Switch to Source site and get uploads info switch_to_blog($from_site_id); $wp_upload_info = wp_upload_dir(); @@ -34,6 +55,8 @@ public static function copy_files($from_site_id, $to_site_id) { $wp_upload_info = wp_upload_dir(); $to_dir = $wp_upload_info['basedir']; + // Pop both switches: the destination, then the source. + restore_current_blog(); restore_current_blog(); $dirs = []; diff --git a/tests/WP_Ultimo/Helpers/Site_Duplicator_Context_Test.php b/tests/WP_Ultimo/Helpers/Site_Duplicator_Context_Test.php new file mode 100644 index 000000000..627e159c4 --- /dev/null +++ b/tests/WP_Ultimo/Helpers/Site_Duplicator_Context_Test.php @@ -0,0 +1,171 @@ +markTestSkipped('Site duplication tests require multisite'); + } + + $this->template_site_id = self::factory()->blog->create( + [ + 'domain' => 'context-template.example.com', + 'path' => '/', + 'title' => 'Context Template', + ] + ); + } + + /** + * Clean up the template site. + */ + public function tearDown(): void { + if ($this->template_site_id) { + wpmu_delete_blog($this->template_site_id, true); + } + + parent::tearDown(); + } + + /** + * MUCD_Files::copy_files() must keep the switch_to_blog() stack balanced — + * exactly one restore_current_blog() per switch_to_blog(). + * + * The pre-fix implementation pushed two switch_to_blog() frames (source + * then target) but only popped one, leaking the source site's context + * back to the caller. This test fails on that buggy implementation. + */ + public function test_copy_files_does_not_leak_blog_context() { + $source_id = self::factory()->blog->create( + [ + 'domain' => 'copy-files-source.example.com', + 'path' => '/', + 'title' => 'Copy Files Source', + ] + ); + + $target_id = self::factory()->blog->create( + [ + 'domain' => 'copy-files-target.example.com', + 'path' => '/', + 'title' => 'Copy Files Target', + ] + ); + + // Defensive: unwind any switch state factory()->blog->create may leak. + while (function_exists('ms_is_switched') && ms_is_switched()) { + restore_current_blog(); + } + + $caller_blog_id = get_current_blog_id(); + $this->assertSame(get_main_site_id(), $caller_blog_id, 'Test prerequisite: caller should be on the main site.'); + + // Skip the actual filesystem recursion — we're testing the + // switch_to_blog() bookkeeping at the start of copy_files(), not + // the file copy itself. + add_filter('mucd_copy_dirs', '__return_empty_array', 99); + + \MUCD_Files::copy_files($source_id, $target_id); + + remove_filter('mucd_copy_dirs', '__return_empty_array', 99); + + // Build a diagnostic message in case of failure: shows the residual + // switch stack and current blog so it's obvious the unbalanced + // switch_to_blog() / restore_current_blog() pairing has regressed. + $switched = isset($GLOBALS['_wp_switched_stack']) ? $GLOBALS['_wp_switched_stack'] : []; + $msg = sprintf( + 'After copy_files(): current=%d, stack=%s, source=%d, target=%d, caller=%d', + get_current_blog_id(), + wp_json_encode($switched), + $source_id, + $target_id, + $caller_blog_id + ); + + $this->assertSame( + $caller_blog_id, + get_current_blog_id(), + 'copy_files() must restore the caller blog id. ' . $msg + ); + + $this->assertFalse( + ms_is_switched(), + 'copy_files() must leave the switch_to_blog() stack empty. ' . $msg + ); + + wpmu_delete_blog($source_id, true); + wpmu_delete_blog($target_id, true); + } + + /** + * Site_Duplicator::duplicate_site() must restore the caller's blog + * context after the full duplication pipeline runs. + * + * This is the integration-level guard against the email-send regression + * reported in GH#1163. + */ + public function test_duplicate_site_restores_blog_context() { + $caller_blog_id = get_current_blog_id(); + + $result = Site_Duplicator::duplicate_site( + $this->template_site_id, + 'Context Restored Site', + [ + 'domain' => 'context-restored.example.com', + 'path' => '/', + 'title' => 'Context Restored Site', + ] + ); + + $this->assertIsInt($result, 'duplicate_site() should return the new site id.'); + $this->assertGreaterThan(0, $result); + + $this->assertSame( + $caller_blog_id, + get_current_blog_id(), + 'duplicate_site() must leave the caller on the same blog context it started on.' + ); + + $this->assertFalse( + ms_is_switched(), + 'duplicate_site() must fully unwind the switch_to_blog() stack.' + ); + + wpmu_delete_blog($result, true); + } +}