Skip to content

Share one recursive delete and validate hashes before deleting #141

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
erseco merged 1 commit into from
Sep 26, 2026
Merged

Share one recursive delete and validate hashes before deleting #141

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 2 additions & 24 deletions includes/class-elp-reprocessor.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -412,15 +412,15 @@ 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;
}

$upload_dir = wp_upload_dir();
$folder = trailingslashit( $upload_dir['basedir'] ) . 'exelearning/' . $hash . '/';

if ( is_dir( $folder ) ) {
$this->recursive_delete( $folder );
ExeLearning_Styles_Service::recursive_delete( $folder );
}
}

Expand Down Expand Up @@ -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 );
}
}
}
29 changes: 5 additions & 24 deletions includes/class-elp-upload-handler.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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() );
}
Expand Down Expand Up @@ -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.
*
Expand All @@ -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 );
}
}
}
Expand Down
7 changes: 5 additions & 2 deletions includes/class-styles-service.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
47 changes: 15 additions & 32 deletions tests/unit/ElpUploadHandlerTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
*/
Expand Down Expand Up @@ -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.
*/
Expand Down Expand Up @@ -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();
Expand All @@ -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.
*/
Expand Down Expand Up @@ -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.
Expand Down
Loading