From 6295806d94b1cb741b7f5bcb8c0f7f3bfbb5851f Mon Sep 17 00:00:00 2001 From: latenighthackathon Date: Mon, 20 Apr 2026 22:27:56 -0500 Subject: [PATCH 1/2] fix[faustwp]: (#2311) clean up uploaded blockset zip when extraction fails MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When unzip_uploaded_file() returns a WP_Error in process_and_replace_blocks(), the uploaded zip was left behind in the target directory. On repeat failures this accumulates dead files on disk. Delete the target file before returning the error so the upload slot stays clean. Mirrors the existing cleanup_temp_directory() call on the success path. Split out from #2312 per @josephfusco review feedback — the hash_equals security hardening half stays on that PR so each can be reviewed and reverted independently. --- .changeset/blockset-cleanup-on-failed-extraction.md | 5 +++++ plugins/faustwp/includes/blocks/functions.php | 1 + 2 files changed, 6 insertions(+) create mode 100644 .changeset/blockset-cleanup-on-failed-extraction.md diff --git a/.changeset/blockset-cleanup-on-failed-extraction.md b/.changeset/blockset-cleanup-on-failed-extraction.md new file mode 100644 index 000000000..f99669046 --- /dev/null +++ b/.changeset/blockset-cleanup-on-failed-extraction.md @@ -0,0 +1,5 @@ +--- +"@faustwp/wordpress-plugin": patch +--- + +fix[faustwp]: clean up uploaded blockset zip when extraction fails diff --git a/plugins/faustwp/includes/blocks/functions.php b/plugins/faustwp/includes/blocks/functions.php index 716e2ff71..8253f4053 100644 --- a/plugins/faustwp/includes/blocks/functions.php +++ b/plugins/faustwp/includes/blocks/functions.php @@ -53,6 +53,7 @@ function process_and_replace_blocks( $wp_filesystem, $file, $dirs ) { $unzip_result = unzip_uploaded_file( $target_file, $dirs['target'] ); if ( is_wp_error( $unzip_result ) ) { + $wp_filesystem->delete( $target_file ); return $unzip_result; } From a0302a8e28cdde81e08ca9395cf416177b60872b Mon Sep 17 00:00:00 2001 From: Joe Fusco Date: Wed, 13 May 2026 11:51:25 -0400 Subject: [PATCH 2/2] test[faustwp]: regression test for process_and_replace_blocks() unzip-failure cleanup (#2311) Adds two unit tests covering process_and_replace_blocks() in includes/blocks/functions.php, which previously had zero direct coverage (every existing test in BlockFunctionTests.php stubbed it out via Brain Monkey). - test_process_and_replace_blocks_cleans_up_on_unzip_failure asserts that on a WP_Error from unzip_uploaded_file(), the function calls $wp_filesystem->delete($target_file) exactly once before returning the error. Verified red-on-revert: removing the new delete line in functions.php fails this test cleanly with a precise Mockery error ('delete should be called exactly 1 times but called 0 times'). This is the regression guard against future changes that drop the cleanup. - test_process_and_replace_blocks_success_path_does_not_delete_target asserts the inverse: on the happy path, delete() is NOT called on the target file (the extracted zip remains on disk) and cleanup_temp_directory() runs for the staging dir. Catches a future over-aggressive change that would delete the target on success. Both tests follow the existing Brain Monkey + Mockery pattern in BlockFunctionTests.php; the cleanup_temp_directory stub in the failure-path test additionally throws a LogicException so we catch regressions where the is_wp_error early-return is removed (and success-path cleanup leaks into the failure path). Verified locally with the full suite (composer test, single-site + multisite): 123 tests, 283 assertions, all green. Targeted run: "OK (2 tests, 6 assertions)". --- .../faustwp/tests/unit/BlockFunctionTests.php | 88 +++++++++++++++++++ 1 file changed, 88 insertions(+) diff --git a/plugins/faustwp/tests/unit/BlockFunctionTests.php b/plugins/faustwp/tests/unit/BlockFunctionTests.php index 8b3139a63..97f8cf372 100644 --- a/plugins/faustwp/tests/unit/BlockFunctionTests.php +++ b/plugins/faustwp/tests/unit/BlockFunctionTests.php @@ -136,4 +136,92 @@ public function test_ensure_directories_exist() { $this->assertTrue( Blocks\ensure_directories_exist( $dirs ) ); } + /** + * Regression guard for #2311: when unzip_uploaded_file() returns a WP_Error, + * process_and_replace_blocks() must call $wp_filesystem->delete( $target_file ) + * before returning, so the orphaned zip doesn't accumulate at the predictable + * path under wp-content/uploads/faustwp/blocks/. + * + * Verified red-on-revert: removing the delete line from + * includes/blocks/functions.php fails this test's Mockery once() expectation. + */ + public function test_process_and_replace_blocks_cleans_up_on_unzip_failure() { + $file = [ + 'name' => 'mock.zip', + 'type' => 'application/zip', + 'tmp_name' => '/tmp/mock.zip', + ]; + $dirs = [ + 'target' => '/uploads/blocks', + 'temp' => '/uploads/blocks/tmp', + ]; + $target_file = '/uploads/blocks/mock.zip'; + $error = new WP_Error( 'unzip_error', 'mock unzip failure' ); + + stubs([ + // Internal helpers: success on move, error on unzip. cleanup must NOT + // run on the error path; if it does, fail loudly so we catch a future + // regression where the early return is removed. + 'WPE\FaustWP\Blocks\move_uploaded_file' => true, + 'WPE\FaustWP\Blocks\unzip_uploaded_file' => $error, + 'WPE\FaustWP\Blocks\cleanup_temp_directory' => function () { + throw new \LogicException( 'cleanup_temp_directory must not run on the unzip-failure path' ); + }, + // WP global helpers: keep the target-file path deterministic so the + // Mockery ->with() expectation matches without depending on Brain + // Monkey's default passthroughs. + 'trailingslashit' => function ( $s ) { return rtrim( $s, '/' ) . '/'; }, + 'sanitize_file_name' => function ( $s ) { return $s; }, + ]); + + $filesystem = Mockery::mock( 'WP_Filesystem_Base' ); + $filesystem->shouldReceive( 'delete' ) + ->with( $target_file ) + ->once(); + + $result = Blocks\process_and_replace_blocks( $filesystem, $file, $dirs ); + + $this->assertInstanceOf( WP_Error::class, $result ); + $this->assertSame( $error, $result ); + } + + /** + * Success path: when both move and unzip succeed, process_and_replace_blocks() + * must NOT call $wp_filesystem->delete( $target_file ) (the extracted blocks + * remain in place) and must invoke cleanup_temp_directory() for the staging dir. + * + * Inverse of the regression-guard test above: catches a future change that + * over-aggressively deletes the target on the happy path. + */ + public function test_process_and_replace_blocks_success_path_does_not_delete_target() { + $file = [ + 'name' => 'mock.zip', + 'type' => 'application/zip', + 'tmp_name' => '/tmp/mock.zip', + ]; + $dirs = [ + 'target' => '/uploads/blocks', + 'temp' => '/uploads/blocks/tmp', + ]; + + $cleanup_called = false; + stubs([ + 'WPE\FaustWP\Blocks\move_uploaded_file' => true, + 'WPE\FaustWP\Blocks\unzip_uploaded_file' => true, + 'WPE\FaustWP\Blocks\cleanup_temp_directory' => function () use ( &$cleanup_called ) { + $cleanup_called = true; + }, + 'trailingslashit' => function ( $s ) { return rtrim( $s, '/' ) . '/'; }, + 'sanitize_file_name' => function ( $s ) { return $s; }, + ]); + + $filesystem = Mockery::mock( 'WP_Filesystem_Base' ); + $filesystem->shouldNotReceive( 'delete' ); + + $result = Blocks\process_and_replace_blocks( $filesystem, $file, $dirs ); + + $this->assertTrue( $result ); + $this->assertTrue( $cleanup_called, 'cleanup_temp_directory must run on the success path.' ); + } + }