diff --git a/includes/class-elp-reprocessor.php b/includes/class-elp-reprocessor.php index caaff8b..c136299 100644 --- a/includes/class-elp-reprocessor.php +++ b/includes/class-elp-reprocessor.php @@ -412,7 +412,7 @@ public function retire_extraction( $attachment_id, $old_hash, $new_hash ) { * @param string $hash Extraction hash to clean up. */ public function cleanup_by_hash( $hash ) { - if ( empty( $hash ) ) { + if ( ! ExeLearning_Content_Hash_Aliases::is_valid_hash( $hash ) ) { return; } @@ -420,7 +420,7 @@ public function cleanup_by_hash( $hash ) { $folder = trailingslashit( $upload_dir['basedir'] ) . 'exelearning/' . $hash . '/'; if ( is_dir( $folder ) ) { - $this->recursive_delete( $folder ); + ExeLearning_Styles_Service::recursive_delete( $folder ); } } @@ -456,26 +456,4 @@ private function normalize_zip_to_elpx( $attachment_id, $file_path ) { return $new_path; } - - /** - * Recursively delete a directory. - * - * @param string $dir Directory path. - */ - private function recursive_delete( $dir ) { - if ( ! file_exists( $dir ) ) { - return; - } - - if ( is_file( $dir ) || is_link( $dir ) ) { - wp_delete_file( $dir ); - } else { - $files = array_diff( scandir( $dir ), array( '.', '..' ) ); - foreach ( $files as $file ) { - $this->recursive_delete( $dir . DIRECTORY_SEPARATOR . $file ); - } - // phpcs:ignore WordPress.WP.AlternativeFunctions.file_system_operations_rmdir -- Direct filesystem access needed for cleanup. - rmdir( $dir ); - } - } } diff --git a/includes/class-elp-upload-handler.php b/includes/class-elp-upload-handler.php index 8a8ca26..607a467 100644 --- a/includes/class-elp-upload-handler.php +++ b/includes/class-elp-upload-handler.php @@ -113,7 +113,7 @@ public function process_elp_upload( $upload ) { if ( is_wp_error( $extract_result ) ) { // Remove any partially extracted files so a rejected upload leaves no // orphaned directory behind. - $this->exelearning_recursive_delete( $destination ); + ExeLearning_Styles_Service::recursive_delete( $destination ); wp_delete_file( $file ); return array( 'error' => $extract_result->get_error_message() ); } @@ -205,27 +205,6 @@ public function save_elp_metadata( $attachment_id ) { } } - /** - * Recursively deletes a directory and its contents. - * - * @param string $dir Directory path. - */ - private function exelearning_recursive_delete( $dir ) { - if ( ! file_exists( $dir ) ) { - return; - } - if ( is_file( $dir ) || is_link( $dir ) ) { - wp_delete_file( $dir ); - } else { - $files = array_diff( scandir( $dir ), array( '.', '..' ) ); - foreach ( $files as $file ) { - $this->exelearning_recursive_delete( $dir . DIRECTORY_SEPARATOR . $file ); - } - // phpcs:ignore WordPress.WP.AlternativeFunctions.file_system_operations_rmdir -- Direct filesystem access needed for cleanup. - rmdir( $dir ); - } - } - /** * Deletes the extracted folder associated with an attachment. * @@ -234,12 +213,14 @@ private function exelearning_recursive_delete( $dir ) { public function exelearning_delete_extracted_folder( $post_id ) { $directory = get_post_meta( $post_id, '_exelearning_extracted', true ); - if ( $directory ) { + // Only a well-formed hash may name a folder: anything else ('..', '') + // would resolve outside the attachment's own extraction. + if ( ExeLearning_Content_Hash_Aliases::is_valid_hash( $directory ) ) { $upload_dir = wp_upload_dir(); $full_path = trailingslashit( $upload_dir['basedir'] ) . 'exelearning/' . $directory . '/'; if ( is_dir( $full_path ) ) { - $this->exelearning_recursive_delete( $full_path ); + ExeLearning_Styles_Service::recursive_delete( $full_path ); } } } diff --git a/includes/class-styles-service.php b/includes/class-styles-service.php index a4ebad0..3e5835b 100644 --- a/includes/class-styles-service.php +++ b/includes/class-styles-service.php @@ -630,8 +630,11 @@ public static function recursive_delete( $dir ) { wp_delete_file( $dir ); return; } - $items = array_diff( scandir( $dir ), array( '.', '..' ) ); - foreach ( $items as $item ) { + $items = scandir( $dir ); + if ( false === $items ) { + return; + } + foreach ( array_diff( $items, array( '.', '..' ) ) as $item ) { self::recursive_delete( $dir . DIRECTORY_SEPARATOR . $item ); } @rmdir( $dir ); // phpcs:ignore WordPress.WP.AlternativeFunctions.file_system_operations_rmdir,WordPress.PHP.NoSilencedErrors.Discouraged diff --git a/tests/unit/ElpUploadHandlerTest.php b/tests/unit/ElpUploadHandlerTest.php index 36e546c..f6e6fde 100644 --- a/tests/unit/ElpUploadHandlerTest.php +++ b/tests/unit/ElpUploadHandlerTest.php @@ -90,15 +90,6 @@ public function test_security_htaccess_content() { $this->assertTrue( $method->isPrivate() ); } - /** - * Test recursive delete method exists. - */ - public function test_recursive_delete_exists() { - $method = new ReflectionMethod( ExeLearning_Elp_Upload_Handler::class, 'exelearning_recursive_delete' ); - $method->setAccessible( true ); - $this->assertTrue( $method->isPrivate() ); - } - /** * Test save_elp_metadata ignores non-elpx files. */ @@ -235,14 +226,6 @@ public function test_delete_extracted_folder_nonexistent_dir() { ); } - /** - * Test recursive_delete method is private. - */ - public function test_recursive_delete_is_private() { - $method = new ReflectionMethod( ExeLearning_Elp_Upload_Handler::class, 'exelearning_recursive_delete' ); - $this->assertTrue( $method->isPrivate() ); - } - /** * Test create_security_htaccess method is private. */ @@ -464,7 +447,7 @@ public function test_save_elp_metadata_with_transient() { */ public function test_delete_extracted_folder_removes_dir() { $attachment_id = $this->factory->attachment->create(); - $hash = 'test' . uniqid(); + $hash = sha1( uniqid( 'exe-remove-', true ) ); // Create the directory. $upload_dir = wp_upload_dir(); @@ -481,6 +464,20 @@ public function test_delete_extracted_folder_removes_dir() { $this->assertDirectoryDoesNotExist( $folder ); } + /** + * A stored value that is not an extraction hash never names a folder to + * delete: '..' would otherwise resolve to the uploads directory itself. + */ + public function test_delete_extracted_folder_ignores_a_malformed_hash() { + list( , $bystander ) = $this->create_extraction_folder(); + $attachment_id = $this->factory->attachment->create(); + update_post_meta( $attachment_id, '_exelearning_extracted', '..' ); + + $this->handler->exelearning_delete_extracted_folder( $attachment_id ); + + $this->assertDirectoryExists( $bystander, 'The whole extraction root was deleted.' ); + } + /** * Test register method is public. */ @@ -583,20 +580,6 @@ static function ( $source, $destination ) use ( &$created ) { $this->assertDirectoryDoesNotExist( $created[0] ); } - /** - * Deleting a directory that is not there is a no-op. - */ - public function test_recursive_delete_ignores_a_missing_directory() { - $method = new ReflectionMethod( ExeLearning_Elp_Upload_Handler::class, 'exelearning_recursive_delete' ); - $method->setAccessible( true ); - - $missing = wp_upload_dir()['basedir'] . '/never-created-' . wp_rand(); - $method->invoke( $this->handler, $missing ); - - $this->assertDirectoryDoesNotExist( $missing ); - } - - /** * If the extraction directory cannot be created the upload is rejected and * the stored file is removed, rather than left behind unusable.