Simplify root-scoped cleanup

internal/fileops/cleanup.go

@@ -5,28 +5,28 @@ import (
 	"fmt"
 	"os"
 	"path/filepath"
-	"sort"
 	"strings"
 )
 
-// RemoveAllUnderRoot removes target and its contents only after opening root and
-// every target ancestor without following symbolic links. target must name a
-// proper descendant of root. Symlinks are rejected rather than followed or
-// removed so a failed cleanup never silently changes its scope.
+// RemoveAllUnderRoot removes target and its contents beneath root. target must
+// name a proper descendant of root; removal is confined by the opened root.
 func RemoveAllUnderRoot(rootPath, target string) error {
-	root, targetName, err := openConfinedCleanupTarget(rootPath, target)
+	root, targetName, err := openCleanupRoot(rootPath, target)
 	if err != nil {
 		return err
 	}
 	defer func() { _ = root.Close() }()
-	return removeConfinedEntry(root, targetName)
+	if err := root.RemoveAll(targetName); err != nil {
+		return fmt.Errorf("remove cleanup target %q: %w", targetName, err)
+	}
+	return nil
 }
 
 // RemoveFileUnderRoot removes an exact regular-file target below root without
-// following symlinked ancestors or the leaf. A missing target is successful;
-// directories, symlinks, and other non-regular entries are rejected.
+// following the leaf. A missing target is successful; directories, symlinks,
+// and other non-regular entries are rejected.
 func RemoveFileUnderRoot(rootPath, target string) error {
-	root, targetName, err := openConfinedCleanupTarget(rootPath, target)
+	root, targetName, err := openCleanupRoot(rootPath, target)
 	if err != nil {
 		return err
 	}
@@ -48,7 +48,7 @@ func RemoveFileUnderRoot(rootPath, target string) error {
 	return nil
 }
 
-func openConfinedCleanupTarget(rootPath, target string) (*os.Root, string, error) {
+func openCleanupRoot(rootPath, target string) (*os.Root, string, error) {
 	if strings.TrimSpace(rootPath) == "" {
 		return nil, "", fmt.Errorf("cleanup root is required")
 	}
@@ -70,122 +70,13 @@ func openConfinedCleanupTarget(rootPath, target string) (*os.Root, string, error
 	if relative == "." || relative == ".." || strings.HasPrefix(relative, ".."+string(filepath.Separator)) {
 		return nil, "", fmt.Errorf("cleanup target %q must be below root %q", targetAbs, rootAbs)
 	}
-	parts, err := relativePathParts(relative)
+	name := filepath.Clean(relative)
+	if filepath.IsAbs(name) || name == "." || name == ".." || strings.HasPrefix(name, ".."+string(filepath.Separator)) {
+		return nil, "", fmt.Errorf("cleanup target %q must be below root %q", targetAbs, rootAbs)
+	}
+	root, err := os.OpenRoot(rootAbs)
 	if err != nil {
-		return nil, "", err
+		return nil, "", fmt.Errorf("open cleanup root %q: %w", rootAbs, err)
 	}
-	root, err := openConfinedDirectory(rootAbs)
-	if err != nil {
-		return nil, "", err
-	}
-	for _, part := range parts[:len(parts)-1] {
-		child, err := openConfinedChild(root, part, false, 0)
-		if err != nil {
-			_ = root.Close()
-			return nil, "", err
-		}
-		_ = root.Close()
-		root = child
-	}
-	return root, parts[len(parts)-1], nil
-}
-
-func openConfinedDirectory(path string) (*os.Root, error) {
-	abs, err := filepath.Abs(path)
-	if err != nil {
-		return nil, fmt.Errorf("resolve directory: %w", err)
-	}
-	parts, err := absolutePathParts(abs)
-	if err != nil {
-		return nil, err
-	}
-	rootPath := filesystemRoot(abs)
-	root, err := os.OpenRoot(rootPath)
-	if err != nil {
-		return nil, fmt.Errorf("open filesystem root %q: %w", rootPath, err)
-	}
-	for _, part := range parts {
-		child, err := openConfinedChild(root, part, false, 0)
-		if err != nil {
-			_ = root.Close()
-			return nil, err
-		}
-		_ = root.Close()
-		root = child
-	}
-	return root, nil
-}
-
-func relativePathParts(path string) ([]string, error) {
-	parts := strings.FieldsFunc(path, func(r rune) bool { return r == filepath.Separator || r == '/' || r == '\\' })
-	if len(parts) == 0 {
-		return nil, fmt.Errorf("cleanup target is required")
-	}
-	for _, part := range parts {
-		if part == "" || part == "." || part == ".." {
-			return nil, fmt.Errorf("unsafe cleanup path component %q", part)
-		}
-	}
-	return parts, nil
-}
-
-func removeConfinedEntry(parent *os.Root, name string) error {
-	info, err := parent.Lstat(name)
-	if errors.Is(err, os.ErrNotExist) {
-		return nil
-	}
-	if err != nil {
-		return fmt.Errorf("inspect cleanup entry %q: %w", name, err)
-	}
-	if info.Mode()&os.ModeSymlink != 0 {
-		return fmt.Errorf("refusing to delete symlink path %q", name)
-	}
-	if !info.IsDir() {
-		if err := parent.Remove(name); err != nil {
-			return fmt.Errorf("remove cleanup entry %q: %w", name, err)
-		}
-		return nil
-	}
-
-	child, err := openConfinedChild(parent, name, false, 0)
-	if err != nil {
-		return err
-	}
-	err = removeConfinedChildren(child)
-	closeErr := child.Close()
-	if err != nil {
-		if closeErr != nil {
-			return errors.Join(err, closeErr)
-		}
-		return err
-	}
-	if closeErr != nil {
-		return fmt.Errorf("close cleanup directory %q: %w", name, closeErr)
-	}
-	if err := parent.Remove(name); err != nil {
-		return fmt.Errorf("remove cleanup directory %q: %w", name, err)
-	}
-	return nil
-}
-
-func removeConfinedChildren(root *os.Root) error {
-	directory, err := root.Open(".")
-	if err != nil {
-		return fmt.Errorf("open cleanup directory: %w", err)
-	}
-	entries, readErr := directory.ReadDir(-1)
-	closeErr := directory.Close()
-	if readErr != nil {
-		return fmt.Errorf("read cleanup directory: %w", readErr)
-	}
-	if closeErr != nil {
-		return fmt.Errorf("close cleanup directory: %w", closeErr)
-	}
-	sort.Slice(entries, func(i, j int) bool { return entries[i].Name() < entries[j].Name() })
-	for _, entry := range entries {
-		if err := removeConfinedEntry(root, entry.Name()); err != nil {
-			return err
-		}
-	}
-	return nil
+	return root, name, nil
 }

internal/fileops/cleanup_test.go

@@ -38,6 +38,9 @@ func TestRemoveAllUnderRootRejectsUnsafeTargets(t *testing.T) {
 	if err := RemoveAllUnderRoot(root, filepath.Join(outside, "target")); err == nil {
 		t.Fatal("RemoveAllUnderRoot(outside) error = nil, want outside-root rejection")
 	}
+	if err := RemoveAllUnderRoot(root, filepath.Join(root, "runs", "..")); err == nil {
+		t.Fatal("RemoveAllUnderRoot(root relative traversal) error = nil, want root rejection")
+	}
 	if err := os.Symlink(outside, filepath.Join(root, "runs")); err == nil {
 		t.Fatal("Symlink() error = nil, want collision because runs already exists")
 	}
@@ -48,7 +51,7 @@ func TestRemoveAllUnderRootRejectsUnsafeTargets(t *testing.T) {
 		t.Fatalf("Symlink() error = %v", err)
 	}
 	if err := RemoveAllUnderRoot(root, target); err == nil {
-		t.Fatal("RemoveAllUnderRoot(symlinked ancestor) error = nil, want rejection")
+		t.Fatal("RemoveAllUnderRoot(symlinked ancestor) error = nil, want root confinement error")
 	}
 	data, err := os.ReadFile(sentinel)
 	if err != nil {
@@ -59,7 +62,7 @@ func TestRemoveAllUnderRootRejectsUnsafeTargets(t *testing.T) {
 	}
 }
 
-func TestRemoveAllUnderRootRejectsSymlinkInTree(t *testing.T) {
+func TestRemoveAllUnderRootRemovesRootContainedSymlink(t *testing.T) {
 	root := t.TempDir()
 	outside := t.TempDir()
 	target := filepath.Join(root, "runs", "run-1")
@@ -72,12 +75,15 @@ func TestRemoveAllUnderRootRejectsSymlinkInTree(t *testing.T) {
 		t.Fatalf("Symlink() error = %v", err)
 	}
 
-	if err := RemoveAllUnderRoot(root, target); err == nil {
-		t.Fatal("RemoveAllUnderRoot() error = nil, want symlink rejection")
+	if err := RemoveAllUnderRoot(root, target); err != nil {
+		t.Fatalf("RemoveAllUnderRoot() error = %v", err)
 	}
 	if _, err := os.Stat(sentinel); err != nil {
 		t.Fatalf("outside sentinel was changed: %v", err)
 	}
+	if _, err := os.Lstat(target); !errors.Is(err, os.ErrNotExist) {
+		t.Fatalf("Lstat(%q) error = %v, want not exist", target, err)
+	}
 }
 
 func TestRemoveFileUnderRootRemovesOnlyRegularFile(t *testing.T) {

internal/fileops/directory.go

@@ -66,7 +66,7 @@ func promoteDirectoryConfined(src, dst string, parent *os.Root, destinationName
 			closeErr = temporary.Close()
 		}
 		if removeTemporary {
-			removeErr := removeConfinedEntry(parent, temporaryName)
+			removeErr := parent.RemoveAll(temporaryName)
 			if removeErr != nil {
 				resultErr = errors.Join(resultErr, fmt.Errorf("remove temporary destination directory: %w", removeErr))
 			}