feat(fileutils): remove directories matched by glob patterns in RemoveFiles (#218)

The RemoveFiles function used os.Remove for paths matched by glob
patterns, which only works for files and empty directories. Extensions
like vchord create non-empty directories matching pgsql_tmp* in
PGDATA during recovery, and these were not being cleaned up.

Use os.RemoveAll instead, which handles both files and non-empty
directories correctly. Add path traversal protection to prevent
patterns from resolving outside basePath, and ensure basePath
itself is never removed.

Closes #217 
Ref: cloudnative-pg/cloudnative-pg#10398

Signed-off-by: Marco Nenciarini <marco.nenciarini@enterprisedb.com>
Signed-off-by: Armando Ruocco <armando.ruocco@enterprisedb.com>
Co-authored-by: Armando Ruocco <armando.ruocco@enterprisedb.com>

Author: mnencia

pkg/fileutils/fileutils.go

@@ -415,52 +415,71 @@ func GetDirectoryContent(dir string) (files []string, err error) {
 	return files, err
 }
 
-// RemoveFiles deletes the files and directories specified by the filePaths patterns
-// relative to the basePath. If a pattern ends with "/*", it implies that all the
-// contents of the directory (not the directory itself) matching the pattern should
-// be removed. If a pattern does not end with "/*", then the files matching the
-// pattern will be removed.
+// RemoveFiles deletes the files and directories specified by the filePaths
+// patterns relative to basePath. Patterns ending with "/*" remove the directory
+// contents but preserve the directory itself. Other patterns remove all matching
+// files and directories.
 //
-// Parameters:
-// - ctx: A context used for logging
-// - basePath: The root directory where the filePaths are applied.
-// - filePaths: List of relative paths or patterns to be removed.
-//
-// Returns:
-// - error: Any error encountered during the removal process, or nil if the operation was successful.
-//
-// Example:
-// basePath: "/path/to/directory"
-// filePaths: ["file1.txt", "subdir/*"]
-// This would remove "/path/to/directory/file1.txt" and the "path/to/directory/subdir" folder
+// Patterns that resolve outside basePath are skipped as a safety measure against
+// path traversal. basePath itself is never removed.
 func RemoveFiles(ctx context.Context, basePath string, filePaths []string) error {
-	contextLogger := log.FromContext(ctx)
-
 	for _, pattern := range filePaths {
-		if len(pattern) >= 2 && pattern[len(pattern)-2:] == "/*" {
-			dirPath := filepath.Join(basePath, pattern[:len(pattern)-2])
-			dirExists, err := FileExists(dirPath)
-			if err != nil {
+		if dirPattern, ok := strings.CutSuffix(pattern, "/*"); ok {
+			if err := removeDirectoryPattern(ctx, basePath, dirPattern); err != nil {
 				return err
 			}
-			if dirExists {
-				contextLogger.Debug("Removing directory", "dirPath", dirPath)
-				if err := RemoveDirectoryContent(dirPath); err != nil {
-					return err
-				}
-			}
 			continue
 		}
 
-		matches, err := filepath.Glob(filepath.Join(basePath, pattern))
-		if err != nil {
+		if err := removeGlobMatches(ctx, basePath, pattern); err != nil {
 			return err
 		}
-		for _, match := range matches {
-			contextLogger.Debug("Removing file", "fileName", match)
-			if err := RemoveFile(match); err != nil {
-				return err
-			}
+	}
+	return nil
+}
+
+// removeDirectoryPattern removes the contents of the directory identified by
+// dirPattern relative to basePath, preserving the directory itself.
+func removeDirectoryPattern(ctx context.Context, basePath, dirPattern string) error {
+	contextLogger := log.FromContext(ctx)
+	cleanBasePath := filepath.Clean(basePath)
+	dirPath := filepath.Join(basePath, dirPattern)
+
+	if dirPath != cleanBasePath && !strings.HasPrefix(dirPath, cleanBasePath+string(filepath.Separator)) {
+		contextLogger.Warning("Skipping path outside basePath", "path", dirPath, "basePath", cleanBasePath)
+		return nil
+	}
+
+	dirExists, err := FileExists(dirPath)
+	if err != nil {
+		return err
+	}
+	if !dirExists {
+		return nil
+	}
+
+	contextLogger.Debug("Removing directory contents", "dirPath", dirPath)
+	return RemoveDirectoryContent(dirPath)
+}
+
+// removeGlobMatches removes all files and directories matching the glob pattern
+// relative to basePath. Matches outside basePath are skipped.
+func removeGlobMatches(ctx context.Context, basePath, pattern string) error {
+	contextLogger := log.FromContext(ctx)
+	basePrefix := filepath.Clean(basePath) + string(filepath.Separator)
+
+	matches, err := filepath.Glob(filepath.Join(basePath, pattern))
+	if err != nil {
+		return err
+	}
+	for _, match := range matches {
+		if !strings.HasPrefix(match, basePrefix) {
+			contextLogger.Warning("Skipping path outside basePath", "path", match, "basePath", basePath)
+			continue
+		}
+		contextLogger.Debug("Removing path", "path", match)
+		if err := os.RemoveAll(match); err != nil {
+			return err
 		}
 	}
 	return nil

pkg/fileutils/fileutils_test.go

@@ -290,6 +290,73 @@ var _ = Describe("RemoveFiles", func() {
 		_, err = os.Stat(filepath.Join(tempDir, "dir1"))
 		Expect(err).NotTo(HaveOccurred(), "Expected dir1 to not be removed")
 	})
+
+	It("removes non-empty directories matched by glob patterns", func(ctx SpecContext) {
+		Expect(os.Mkdir(filepath.Join(tempDir, "pgsql_tmp"), 0o750)).To(Succeed())
+		Expect(os.WriteFile(filepath.Join(tempDir, "pgsql_tmp", "tmpfile"), []byte("test"), 0o600)).To(Succeed())
+		Expect(os.Mkdir(filepath.Join(tempDir, "pgsql_tmp_ext_sampling"), 0o750)).To(Succeed())
+		Expect(os.WriteFile(
+			filepath.Join(tempDir, "pgsql_tmp_ext_sampling", "data"), []byte("test"), 0o600,
+		)).To(Succeed())
+		Expect(os.WriteFile(filepath.Join(tempDir, "pgsql_tmp1234.0"), []byte("test"), 0o600)).To(Succeed())
+
+		err := RemoveFiles(ctx, tempDir, []string{"pgsql_tmp*"})
+		Expect(err).NotTo(HaveOccurred())
+
+		_, err = os.Stat(filepath.Join(tempDir, "pgsql_tmp"))
+		Expect(os.IsNotExist(err)).To(BeTrue(), "Expected pgsql_tmp directory to be removed")
+
+		_, err = os.Stat(filepath.Join(tempDir, "pgsql_tmp_ext_sampling"))
+		Expect(os.IsNotExist(err)).To(BeTrue(), "Expected pgsql_tmp_ext_sampling directory to be removed")
+
+		_, err = os.Stat(filepath.Join(tempDir, "pgsql_tmp1234.0"))
+		Expect(os.IsNotExist(err)).To(BeTrue(), "Expected pgsql_tmp1234.0 file to be removed")
+	})
+
+	It("does not remove basePath itself when a pattern matches it", func(ctx SpecContext) {
+		err := RemoveFiles(ctx, tempDir, []string{"", "."})
+		Expect(err).NotTo(HaveOccurred())
+
+		_, err = os.Stat(tempDir)
+		Expect(err).NotTo(HaveOccurred(), "Expected basePath to not be removed")
+
+		_, err = os.Stat(filepath.Join(tempDir, "file1.txt"))
+		Expect(err).NotTo(HaveOccurred(), "Expected file1.txt to not be removed")
+	})
+
+	It("removes basePath contents but not basePath itself with ./* pattern", func(ctx SpecContext) {
+		err := RemoveFiles(ctx, tempDir, []string{"./*"})
+		Expect(err).NotTo(HaveOccurred())
+
+		_, err = os.Stat(tempDir)
+		Expect(err).NotTo(HaveOccurred(), "Expected basePath to not be removed")
+
+		entries, err := os.ReadDir(tempDir)
+		Expect(err).NotTo(HaveOccurred())
+		Expect(entries).To(BeEmpty(), "Expected basePath contents to be removed")
+	})
+
+	It("does not remove paths outside basePath via traversal patterns", func(ctx SpecContext) {
+		siblingDir := filepath.Join(filepath.Dir(tempDir), "sibling_safe")
+		Expect(os.Mkdir(siblingDir, 0o750)).To(Succeed())
+		Expect(os.WriteFile(filepath.Join(siblingDir, "important.txt"), []byte("keep"), 0o600)).To(Succeed())
+		defer func() { _ = os.RemoveAll(siblingDir) }()
+
+		// Exercises both glob ("..","../*") and directory pattern ("../sibling_safe/*") branches
+		err := RemoveFiles(ctx, tempDir, []string{"..", "../*", "../sibling_safe/*"})
+		Expect(err).NotTo(HaveOccurred())
+
+		_, err = os.Stat(filepath.Dir(tempDir))
+		Expect(err).NotTo(HaveOccurred(), "Expected parent directory to not be removed")
+
+		_, err = os.Stat(siblingDir)
+		Expect(err).NotTo(HaveOccurred(), "Expected sibling directory to not be removed")
+		_, err = os.Stat(filepath.Join(siblingDir, "important.txt"))
+		Expect(err).NotTo(HaveOccurred(), "Expected sibling contents to not be removed")
+
+		_, err = os.Stat(filepath.Join(tempDir, "file1.txt"))
+		Expect(err).NotTo(HaveOccurred(), "Expected basePath contents to not be removed")
+	})
 })
 
 var _ = Describe("RemoveRestoreExcludedFiles", func() {
@@ -315,8 +382,14 @@ var _ = Describe("RemoveRestoreExcludedFiles", func() {
 				Expect(os.MkdirAll(dirOfTheFile, 0o750)).To(Succeed())
 			}
 			Expect(os.WriteFile(fullPath, []byte("test"), 0o600)).To(Succeed())
-
 		}
+
+		// Also create a pgsql_tmp directory with contents to simulate
+		// extensions (e.g. vchord) that create directories matching pgsql_tmp*
+		Expect(os.Mkdir(filepath.Join(tempDir, "pgsql_tmp_ext_sampling"), 0o750)).To(Succeed())
+		Expect(os.WriteFile(
+			filepath.Join(tempDir, "pgsql_tmp_ext_sampling", "data"), []byte("test"), 0o600,
+		)).To(Succeed())
 	})
 
 	AfterEach(func() {
@@ -336,6 +409,10 @@ var _ = Describe("RemoveRestoreExcludedFiles", func() {
 				Expect(os.IsNotExist(err)).To(BeTrue(), "Expected file to be removed: "+fullPath)
 			}
 		}
+
+		// Verify that pgsql_tmp* directories created by extensions are also removed
+		_, err := os.Stat(filepath.Join(tempDir, "pgsql_tmp_ext_sampling"))
+		Expect(os.IsNotExist(err)).To(BeTrue(), "Expected pgsql_tmp_ext_sampling directory to be removed")
 	})
 })