Skip to content

Commit 510ab53

Browse files
Refactor contained recursive directory removal (#766)
* refactor: share contained recursive directory removal * fix: satisfy contained removal lint --------- Co-authored-by: homeboy-ci[bot] <266378653+homeboy-ci[bot]@users.noreply.github.com>
1 parent b100b25 commit 510ab53

5 files changed

Lines changed: 76 additions & 53 deletions

inc/Workspace/WorkspaceCoreUtilities.php

Lines changed: 69 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -849,6 +849,75 @@ public function validate_containment( string $target, string $container ): array
849849
return PathSecurity::validateContainment($target, $container);
850850
}
851851

852+
/**
853+
* Recursively remove an existing directory after validating containment.
854+
*
855+
* The target must be a real directory inside, but not equal to, the container.
856+
* Symlinks are unlinked as entries and never traversed.
857+
*
858+
* @param string $absolute Absolute directory path to remove.
859+
* @param string $container Parent directory that must contain the target.
860+
* @param string|null $relative_base Base path used to report deleted relative paths.
861+
* @return array<int,string>|\WP_Error
862+
*/
863+
protected function remove_contained_directory_recursive( string $absolute, string $container, ?string $relative_base = null ): array|\WP_Error {
864+
$validation = $this->validate_containment($absolute, $container);
865+
if ( ! $validation['valid'] ) {
866+
return new \WP_Error('path_traversal', (string) ( $validation['message'] ?? 'Path traversal detected. Access denied.' ), array( 'status' => 403 ));
867+
}
868+
869+
$container_real = realpath($container);
870+
$target_real = (string) ( $validation['real_path'] ?? '' );
871+
if ( false === $container_real || '' === $target_real || $target_real === $container_real ) {
872+
return new \WP_Error('unsafe_delete_root', sprintf('Refusing to recursively delete container root: %s', $absolute), array( 'status' => 403 ));
873+
}
874+
875+
if ( ! is_dir($target_real) || is_link($absolute) ) {
876+
return new \WP_Error('not_a_directory', sprintf('Recursive delete target is not a directory: %s', $absolute), array( 'status' => 400 ));
877+
}
878+
879+
$relative_base_real = realpath($relative_base ?? $container);
880+
if ( false === $relative_base_real ) {
881+
$relative_base_real = $container_real;
882+
}
883+
884+
$deleted = array();
885+
// phpcs:ignore WordPress.PHP.NoSilencedErrors.Discouraged -- Failure is converted into a WP_Error below.
886+
$entries = @scandir($target_real);
887+
if ( false === $entries ) {
888+
return new \WP_Error('scandir_failed', sprintf('Failed to read directory: %s', $target_real), array( 'status' => 500 ));
889+
}
890+
891+
foreach ( $entries as $entry ) {
892+
if ( '.' === $entry || '..' === $entry ) {
893+
continue;
894+
}
895+
896+
$child = $target_real . '/' . $entry;
897+
if ( is_dir($child) && ! is_link($child) ) {
898+
$nested = $this->remove_contained_directory_recursive($child, $container_real, $relative_base_real);
899+
if ( is_wp_error($nested) ) {
900+
return $nested;
901+
}
902+
$deleted = array_merge($deleted, $nested);
903+
} else {
904+
// phpcs:ignore WordPress.WP.AlternativeFunctions.unlink_unlink
905+
if ( ! unlink($child) ) {
906+
return new \WP_Error('delete_failed', sprintf('Failed to delete file: %s', $child), array( 'status' => 500 ));
907+
}
908+
$deleted[] = ltrim(substr($child, strlen($relative_base_real)), '/');
909+
}
910+
}
911+
912+
// phpcs:ignore WordPress.WP.AlternativeFunctions.file_system_operations_rmdir
913+
if ( ! rmdir($target_real) ) {
914+
return new \WP_Error('delete_failed', sprintf('Failed to remove directory: %s', $target_real), array( 'status' => 500 ));
915+
}
916+
917+
$deleted[] = ltrim(substr($target_real, strlen($relative_base_real)), '/');
918+
return $deleted;
919+
}
920+
852921
/**
853922
* Derive a repo name from a git URL.
854923
*

inc/Workspace/WorkspaceGitOperations.php

Lines changed: 1 addition & 43 deletions
Original file line numberDiff line numberDiff line change
@@ -427,7 +427,7 @@ public function delete_path( string $handle, string $path, bool $recursive = fal
427427
$deleted[] = $relative;
428428
}
429429
} elseif ( $is_dir ) {
430-
$removed = $this->remove_directory_recursive($absolute, $repo_path);
430+
$removed = $this->remove_contained_directory_recursive($absolute, $repo_path, $repo_path);
431431
if ( is_wp_error($removed) ) {
432432
return $removed;
433433
}
@@ -450,48 +450,6 @@ public function delete_path( string $handle, string $path, bool $recursive = fal
450450
);
451451
}
452452

453-
/**
454-
* Recursively remove an untracked directory under a repo, returning the
455-
* list of relative paths removed (deepest first).
456-
*
457-
* @param string $absolute Absolute path to remove.
458-
* @param string $repo_path Repo root for relative-path computation.
459-
* @return array<int,string>|\WP_Error
460-
*/
461-
private function remove_directory_recursive( string $absolute, string $repo_path ): array|\WP_Error {
462-
$deleted = array();
463-
// phpcs:ignore WordPress.PHP.NoSilencedErrors.Discouraged -- Failure is converted into a WP_Error below.
464-
$entries = @scandir($absolute);
465-
if ( false === $entries ) {
466-
return new \WP_Error('scandir_failed', sprintf('Failed to read directory: %s', $absolute), array( 'status' => 500 ));
467-
}
468-
foreach ( $entries as $entry ) {
469-
if ( '.' === $entry || '..' === $entry ) {
470-
continue;
471-
}
472-
$child = $absolute . '/' . $entry;
473-
if ( is_dir($child) && ! is_link($child) ) {
474-
$nested = $this->remove_directory_recursive($child, $repo_path);
475-
if ( is_wp_error($nested) ) {
476-
return $nested;
477-
}
478-
$deleted = array_merge($deleted, $nested);
479-
} else {
480-
// phpcs:ignore WordPress.WP.AlternativeFunctions.unlink_unlink
481-
if ( ! unlink($child) ) {
482-
return new \WP_Error('delete_failed', sprintf('Failed to delete file: %s', $child), array( 'status' => 500 ));
483-
}
484-
$deleted[] = ltrim(substr($child, strlen($repo_path)), '/');
485-
}
486-
}
487-
// phpcs:ignore WordPress.WP.AlternativeFunctions.file_system_operations_rmdir
488-
if ( ! rmdir($absolute) ) {
489-
return new \WP_Error('delete_failed', sprintf('Failed to remove directory: %s', $absolute), array( 'status' => 500 ));
490-
}
491-
$deleted[] = ltrim(substr($absolute, strlen($repo_path)), '/');
492-
return $deleted;
493-
}
494-
495453
/**
496454
* Commit staged changes in a workspace repository.
497455
*

inc/Workspace/WorkspaceRepositoryLifecycle.php

Lines changed: 4 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -613,13 +613,9 @@ public function remove_repo( string $handle ): array|\WP_Error {
613613
}
614614
}
615615

616-
// Remove recursively.
617-
$escaped = escapeshellarg($validation['real_path']);
618-
// phpcs:ignore WordPress.PHP.DiscouragedPHPFunctions.system_calls_exec
619-
exec(sprintf('rm -rf %s 2>&1', $escaped), $output, $exit_code);
620-
621-
if ( 0 !== $exit_code ) {
622-
return new \WP_Error('remove_failed', sprintf('Failed to remove (exit %d): %s', $exit_code, implode("\n", $output)), array( 'status' => 500 ));
616+
$removed = $this->remove_contained_directory_recursive($validation['real_path'], $this->workspace_path, $this->workspace_path);
617+
if ( is_wp_error($removed) ) {
618+
return $removed;
623619
}
624620

625621
// If we removed a worktree directory but didn't go through `git worktree remove`,
@@ -671,7 +667,7 @@ public function show_repo( string $handle ): array|\WP_Error {
671667
'is_context' => true,
672668
'path' => null,
673669
'branch' => '' !== $ref ? $ref : null,
674-
'remote' => '' !== (string) ( $context_policy['repo'] ?? '' ) ? GitHubRemote::cloneUrl((string) $context_policy['repo']) : null,
670+
'remote' => '' !== (string) ( $context_policy['repo'] ?? '' ) ? GitHubRemote::cloneUrl( (string) $context_policy['repo'] ) : null,
675671
'commit' => null,
676672
'dirty' => 0,
677673
'workspace_policy' => WorkspaceAliasResolver::policy_attestation($handle),

inc/Workspace/WorkspaceWorktreeCleanupEngine.php

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2823,7 +2823,7 @@ private function remove_worktree_by_path( string $repo, string $branch, string $
28232823

28242824
$broken_marker = $this->classify_broken_orphan_worktree_marker($real_path);
28252825
if ( null !== $broken_marker ) {
2826-
$removed_paths = $this->remove_directory_recursive($real_path, $this->workspace_path);
2826+
$removed_paths = $this->remove_contained_directory_recursive($real_path, $this->workspace_path, $this->workspace_path);
28272827
if ( is_wp_error($removed_paths) ) {
28282828
return $removed_paths;
28292829
}

inc/Workspace/WorkspaceWorktreeLifecycle.php

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1365,7 +1365,7 @@ private function repair_cleanup_eligible_stale_worktree_marker( array $row, arra
13651365
return null;
13661366
}
13671367

1368-
$removed_paths = $this->remove_directory_recursive($path, $this->workspace_path);
1368+
$removed_paths = $this->remove_contained_directory_recursive($path, $this->workspace_path, $this->workspace_path);
13691369
if ( $removed_paths instanceof \WP_Error ) {
13701370
return $removed_paths;
13711371
}

0 commit comments

Comments
 (0)