From 5bf20abc323885bdd35ffbea1af28317254eebe1 Mon Sep 17 00:00:00 2001 From: girishpanchal30 Date: Mon, 17 Aug 2026 16:50:09 +0530 Subject: [PATCH 1/4] fix: normalize file permissions during replacement --- inc/media_rename/attachment_replace.php | 68 ++++++++++++++++++- .../media_rename/test-attachment-replace.php | 34 ++++++++++ 2 files changed, 101 insertions(+), 1 deletion(-) diff --git a/inc/media_rename/attachment_replace.php b/inc/media_rename/attachment_replace.php index c5b4c9fa6..fa3daf4a1 100644 --- a/inc/media_rename/attachment_replace.php +++ b/inc/media_rename/attachment_replace.php @@ -83,7 +83,7 @@ public function replace() { return new WP_Error( 'file_error', __( 'Could not move file.', 'optimole-wp' ) ); } - $wp_filesystem->chmod( $original_file, FS_CHMOD_FILE ); + $permissions_normalized = $this->normalize_file_permissions( $original_file ); $this->remove_all_image_sizes(); @@ -102,9 +102,75 @@ public function replace() { do_action( 'optml_attachment_replaced', $this->attachment_id ); + if ( ! $permissions_normalized ) { + return new WP_Error( + 'file_permissions_error', + __( 'File replaced successfully', 'optimole-wp' ) . ' ' . __( 'The permissions may not be updated, so it isn\'t publicly accessible. Please update the permissions.', 'optimole-wp' ) + ); + } + return true; } + /** + * Normalize the permissions of the replaced file. + * + * @param string $file File path. + * + * @return bool Whether the file ended up with the expected permissions. + */ + private function normalize_file_permissions( $file ) { + global $wp_filesystem; + + $mode = defined( 'FS_CHMOD_FILE' ) ? FS_CHMOD_FILE : 0644; + + $applied = $wp_filesystem->chmod( $file, $mode ); + + $reason = ''; + + if ( ! $applied || ! $this->has_permissions( $file, $mode ) ) { + // Fallback for transports where the filesystem abstraction can't chmod. + set_error_handler( + function ( $errno, $errstr ) use ( &$reason ) { + $reason = $errstr; + + return true; + } + ); + + $applied = chmod( $file, $mode ); + + restore_error_handler(); + } + + if ( $applied && $this->has_permissions( $file, $mode ) ) { + return true; + } + + do_action( + 'optml_log', + sprintf( 'Could not normalize permissions to %o for replaced file %s. %s', $mode, $file, $reason ) + ); + + return false; + } + + /** + * Check the current permissions of a file against an expected mode. + * + * @param string $file File path. + * @param int $mode Expected mode. + * + * @return bool + */ + private function has_permissions( $file, $mode ) { + clearstatcache( true, $file ); + + $perms = fileperms( $file ); + + return false !== $perms && ( $perms & 0777 ) === ( $mode & 0777 ); + } + /** * Remove all image sizes files. * diff --git a/tests/media_rename/test-attachment-replace.php b/tests/media_rename/test-attachment-replace.php index fa1706703..c94160eaa 100644 --- a/tests/media_rename/test-attachment-replace.php +++ b/tests/media_rename/test-attachment-replace.php @@ -109,6 +109,40 @@ private function test_replace_unscaled_to_unscaled() { $this->do_replace_test( self::$unscaled_unscaled_id, $replace_file, false, false ); } + /** + * A 0600 upload tmp file must not leave the replaced attachment unreadable to the web server. + */ + public function test_replace_normalizes_permissions_of_restricted_tmp_file() { + global $wp_filesystem; + + $id = self::factory()->attachment->create_upload_object( OPTML_PATH . 'tests/assets/sample-test.jpg' ); + + $tmp_file = self::FILESTASH . 'replace-restricted.jpg'; + $wp_filesystem->copy( OPTML_PATH . 'tests/assets/small-1.jpg', $tmp_file, true ); + chmod( $tmp_file, 0600 ); + + $model = new Optml_Attachment_Model( $id ); + $file_path = $model->get_source_file_path(); + + $replacer = new Optml_Attachment_Replace( + $id, + [ + 'name' => 'replace-restricted.jpg', + 'type' => 'image/jpeg', + 'tmp_name' => $tmp_file, + ] + ); + + $result = $replacer->replace(); + + clearstatcache( true, $file_path ); + + $this->assertTrue( $result, 'Replacement operation failed.' ); + $this->assertSame( FS_CHMOD_FILE & 0777, fileperms( $file_path ) & 0777, 'Replaced file kept the restrictive tmp file permissions.' ); + + wp_delete_post( $id, true ); + } + private function do_replace_test( $id_to_replace, $replace_file, $source_scaled, $result_scaled ) { // Removed var_dump From f1465c652e9e988530f17bb6cf839a0d3949e2b1 Mon Sep 17 00:00:00 2001 From: girishpanchal30 Date: Mon, 17 Aug 2026 17:19:05 +0530 Subject: [PATCH 2/4] fix: improve file permission handling --- inc/media_rename/attachment_replace.php | 12 ++- .../media_rename/test-attachment-replace.php | 97 +++++++++++++++++++ 2 files changed, 104 insertions(+), 5 deletions(-) diff --git a/inc/media_rename/attachment_replace.php b/inc/media_rename/attachment_replace.php index fa3daf4a1..de245bb50 100644 --- a/inc/media_rename/attachment_replace.php +++ b/inc/media_rename/attachment_replace.php @@ -105,7 +105,7 @@ public function replace() { if ( ! $permissions_normalized ) { return new WP_Error( 'file_permissions_error', - __( 'File replaced successfully', 'optimole-wp' ) . ' ' . __( 'The permissions may not be updated, so it isn\'t publicly accessible. Please update the permissions.', 'optimole-wp' ) + __( 'The permissions may not be updated, so it isn\'t publicly accessible. Please update the permissions.', 'optimole-wp' ) ); } @@ -147,10 +147,12 @@ function ( $errno, $errstr ) use ( &$reason ) { return true; } - do_action( - 'optml_log', - sprintf( 'Could not normalize permissions to %o for replaced file %s. %s', $mode, $file, $reason ) - ); + if ( OPTML_DEBUG ) { + do_action( + 'optml_log', + sprintf( 'Could not normalize permissions to %o for replaced file %s. %s', $mode, $file, $reason ) + ); + } return false; } diff --git a/tests/media_rename/test-attachment-replace.php b/tests/media_rename/test-attachment-replace.php index c94160eaa..d079eebb4 100644 --- a/tests/media_rename/test-attachment-replace.php +++ b/tests/media_rename/test-attachment-replace.php @@ -143,6 +143,103 @@ public function test_replace_normalizes_permissions_of_restricted_tmp_file() { wp_delete_post( $id, true ); } + /** + * When the filesystem abstraction can't chmod, the native fallback must still fix the file. + */ + public function test_replace_falls_back_to_native_chmod() { + global $wp_filesystem; + + $real_filesystem = $wp_filesystem; + + $id = self::factory()->attachment->create_upload_object( OPTML_PATH . 'tests/assets/sample-test.jpg' ); + $model = new Optml_Attachment_Model( $id ); + $file_path = $model->get_source_file_path(); + + $tmp_file = self::FILESTASH . 'replace-fallback.jpg'; + $wp_filesystem->copy( OPTML_PATH . 'tests/assets/small-1.jpg', $tmp_file, true ); + chmod( $tmp_file, 0600 ); + + $wp_filesystem = self::failing_chmod_filesystem(); + + $replacer = new Optml_Attachment_Replace( + $id, + [ + 'name' => 'replace-fallback.jpg', + 'type' => 'image/jpeg', + 'tmp_name' => $tmp_file, + ] + ); + + try { + $result = $replacer->replace(); + } finally { + $wp_filesystem = $real_filesystem; + } + + clearstatcache( true, $file_path ); + + $this->assertTrue( $result, 'Replacement operation failed.' ); + $this->assertSame( FS_CHMOD_FILE & 0777, fileperms( $file_path ) & 0777, 'The native chmod fallback did not normalize the permissions.' ); + + wp_delete_post( $id, true ); + } + + /** + * With both chmod attempts failing, the replacement must not be reported as a plain success. + */ + public function test_replace_reports_error_when_permissions_cannot_be_normalized() { + global $wp_filesystem; + + $real_filesystem = $wp_filesystem; + + $id = self::factory()->attachment->create_upload_object( OPTML_PATH . 'tests/assets/sample-test.jpg' ); + $model = new Optml_Attachment_Model( $id ); + $file_path = $model->get_source_file_path(); + + $tmp_file = self::FILESTASH . 'replace-unfixable.jpg'; + $wp_filesystem->copy( OPTML_PATH . 'tests/assets/small-1.jpg', $tmp_file, true ); + chmod( $tmp_file, 0600 ); + + $wp_filesystem = self::failing_chmod_filesystem(); + + $replacer = new Optml_Attachment_Replace( + $id, + [ + 'name' => 'replace-unfixable.jpg', + 'type' => 'image/jpeg', + 'tmp_name' => $tmp_file, + ] + ); + + try { + $result = $replacer->replace(); + } finally { + $wp_filesystem = $real_filesystem; + remove_all_actions( 'optml_log' ); + } + + clearstatcache( true, $file_path ); + + $this->assertWPError( $result, 'Replacement was reported as a success.' ); + $this->assertSame( 'file_permissions_error', $result->get_error_code() ); + $this->assertSame( 0600, fileperms( $file_path ) & 0777, 'The test did not exercise an unfixable file.' ); + + wp_delete_post( $id, true ); + } + + /** + * A direct filesystem whose chmod always fails, as an FTP/SSH transport can. + * + * @return WP_Filesystem_Direct + */ + private static function failing_chmod_filesystem() { + return new class( null ) extends WP_Filesystem_Direct { + public function chmod( $file, $mode = false, $recursive = false ) { + return false; + } + }; + } + private function do_replace_test( $id_to_replace, $replace_file, $source_scaled, $result_scaled ) { // Removed var_dump From 559164cfd47fbe6d5c7099e79b15c54aab33c176 Mon Sep 17 00:00:00 2001 From: girishpanchal30 Date: Mon, 17 Aug 2026 17:40:20 +0530 Subject: [PATCH 3/4] fix: phpunit --- .../media_rename/test-attachment-replace.php | 40 ++++++++++++++++--- 1 file changed, 34 insertions(+), 6 deletions(-) diff --git a/tests/media_rename/test-attachment-replace.php b/tests/media_rename/test-attachment-replace.php index d079eebb4..e643e97fe 100644 --- a/tests/media_rename/test-attachment-replace.php +++ b/tests/media_rename/test-attachment-replace.php @@ -159,8 +159,6 @@ public function test_replace_falls_back_to_native_chmod() { $wp_filesystem->copy( OPTML_PATH . 'tests/assets/small-1.jpg', $tmp_file, true ); chmod( $tmp_file, 0600 ); - $wp_filesystem = self::failing_chmod_filesystem(); - $replacer = new Optml_Attachment_Replace( $id, [ @@ -170,6 +168,9 @@ public function test_replace_falls_back_to_native_chmod() { ] ); + // After the constructor: it calls WP_Filesystem(), which reassigns the global. + $wp_filesystem = self::failing_chmod_filesystem(); + try { $result = $replacer->replace(); } finally { @@ -200,8 +201,6 @@ public function test_replace_reports_error_when_permissions_cannot_be_normalized $wp_filesystem->copy( OPTML_PATH . 'tests/assets/small-1.jpg', $tmp_file, true ); chmod( $tmp_file, 0600 ); - $wp_filesystem = self::failing_chmod_filesystem(); - $replacer = new Optml_Attachment_Replace( $id, [ @@ -211,18 +210,24 @@ public function test_replace_reports_error_when_permissions_cannot_be_normalized ] ); + // After the constructor: it calls WP_Filesystem(), which reassigns the global. + $wp_filesystem = self::unfixable_filesystem(); + + // The metadata step warns about the intentionally missing file; PHPUnit turns that into an error. + set_error_handler( '__return_true' ); + try { $result = $replacer->replace(); } finally { + restore_error_handler(); $wp_filesystem = $real_filesystem; - remove_all_actions( 'optml_log' ); } clearstatcache( true, $file_path ); $this->assertWPError( $result, 'Replacement was reported as a success.' ); $this->assertSame( 'file_permissions_error', $result->get_error_code() ); - $this->assertSame( 0600, fileperms( $file_path ) & 0777, 'The test did not exercise an unfixable file.' ); + $this->assertFileDoesNotExist( $file_path, 'The test did not exercise an unfixable file.' ); wp_delete_post( $id, true ); } @@ -240,6 +245,29 @@ public function chmod( $file, $mode = false, $recursive = false ) { }; } + /** + * A filesystem that reports a successful move but leaves nothing at the destination. + * + * Both chmod attempts then fail with ENOENT, which is the only way to reach the failure + * branch without root: a file the test user owns can always be chmod'ed natively. + * + * @return WP_Filesystem_Direct + */ + private static function unfixable_filesystem() { + return new class( null ) extends WP_Filesystem_Direct { + public function move( $source, $destination, $overwrite = false ) { + @unlink( $source ); + @unlink( $destination ); + + return true; + } + + public function chmod( $file, $mode = false, $recursive = false ) { + return false; + } + }; + } + private function do_replace_test( $id_to_replace, $replace_file, $source_scaled, $result_scaled ) { // Removed var_dump From 35745b08fb1d8adc8913a95fea740af03144f6a3 Mon Sep 17 00:00:00 2001 From: girishpanchal30 Date: Mon, 17 Aug 2026 17:53:33 +0530 Subject: [PATCH 4/4] fix: improve error message --- inc/media_rename/attachment_replace.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/inc/media_rename/attachment_replace.php b/inc/media_rename/attachment_replace.php index de245bb50..c6b78b2bb 100644 --- a/inc/media_rename/attachment_replace.php +++ b/inc/media_rename/attachment_replace.php @@ -105,7 +105,7 @@ public function replace() { if ( ! $permissions_normalized ) { return new WP_Error( 'file_permissions_error', - __( 'The permissions may not be updated, so it isn\'t publicly accessible. Please update the permissions.', 'optimole-wp' ) + __( 'The permissions may not have been updated, so the file isn\'t publicly accessible. Please update the permissions.', 'optimole-wp' ) ); }