Consolidate path safety, temp downloads, and cleanup validation helpers

internal/app/clean.go

@@ -182,26 +182,12 @@ func reportCleanRootChildren(out io.Writer, root, policy string, dryRun bool) er
 }
 
 func cleanableRootChildren(root, policy string) (string, []string, error) {
-	cleanRoot := strings.TrimSpace(root)
-	if cleanRoot == "" {
-		return "", nil, fmt.Errorf("cleanup policy %s: root path is required", policy)
-	}
-	rootAbs, err := filepath.Abs(cleanRoot)
+	rootAbs, exists, err := validateCleanRoot(root, policy)
 	if err != nil {
-		return "", nil, fmt.Errorf("cleanup policy %s: resolve root %q: %w", policy, cleanRoot, err)
+		return "", nil, err
 	}
-	info, err := os.Lstat(rootAbs)
-	if err != nil {
-		if os.IsNotExist(err) {
-			return rootAbs, nil, nil
-		}
-		return "", nil, fmt.Errorf("cleanup policy %s: stat root %q: %w", policy, rootAbs, err)
-	}
-	if info.Mode()&os.ModeSymlink != 0 {
-		return "", nil, fmt.Errorf("cleanup policy %s: refusing to clean symlink root %q", policy, rootAbs)
-	}
-	if !info.IsDir() {
-		return "", nil, fmt.Errorf("cleanup policy %s: root %q is not a directory", policy, rootAbs)
+	if !exists {
+		return rootAbs, nil, nil
 	}
 	entries, err := os.ReadDir(rootAbs)
 	if err != nil {
@@ -300,46 +286,7 @@ func reportCleanScopedFile(out io.Writer, root, target, policy string, dryRun bo
 }
 
 func validateScopedFile(root, target, policy string) (scopedDir, error) {
-	cleanRoot := strings.TrimSpace(root)
-	cleanTarget := strings.TrimSpace(target)
-	if cleanRoot == "" {
-		return scopedDir{}, fmt.Errorf("cleanup policy %s: root path is required", policy)
-	}
-	if cleanTarget == "" {
-		return scopedDir{}, fmt.Errorf("cleanup policy %s: target path is required", policy)
-	}
-	rootAbs, err := filepath.Abs(cleanRoot)
-	if err != nil {
-		return scopedDir{}, fmt.Errorf("cleanup policy %s: resolve root %q: %w", policy, cleanRoot, err)
-	}
-	targetAbs, err := filepath.Abs(cleanTarget)
-	if err != nil {
-		return scopedDir{}, fmt.Errorf("cleanup policy %s: resolve target %q: %w", policy, cleanTarget, err)
-	}
-	rel, err := filepath.Rel(rootAbs, targetAbs)
-	if err != nil {
-		return scopedDir{}, fmt.Errorf("cleanup policy %s: relative path from %q to %q: %w", policy, rootAbs, targetAbs, err)
-	}
-	if rel == "." {
-		return scopedDir{}, fmt.Errorf("cleanup policy %s: refusing to delete root directory %q", policy, rootAbs)
-	}
-	if rel == ".." || strings.HasPrefix(rel, ".."+string(filepath.Separator)) {
-		return scopedDir{}, fmt.Errorf("cleanup policy %s: refusing to delete path outside root: root=%q target=%q", policy, rootAbs, targetAbs)
-	}
-	info, err := os.Lstat(targetAbs)
-	if err != nil {
-		if os.IsNotExist(err) {
-			return scopedDir{RootAbs: rootAbs, TargetAbs: targetAbs, Exists: false}, nil
-		}
-		return scopedDir{}, fmt.Errorf("cleanup policy %s: stat target %q: %w", policy, targetAbs, err)
-	}
-	if info.Mode()&os.ModeSymlink != 0 {
-		return scopedDir{}, fmt.Errorf("cleanup policy %s: refusing to delete symlink path %q", policy, targetAbs)
-	}
-	if info.IsDir() {
-		return scopedDir{}, fmt.Errorf("cleanup policy %s: target %q is a directory", policy, targetAbs)
-	}
-	return scopedDir{RootAbs: rootAbs, TargetAbs: targetAbs, Exists: true}, nil
+	return validateScopedTarget(root, target, policy, false)
 }
 
 func cleanIsFlac(path string) bool {

internal/app/cleanup_targets.go

@@ -0,0 +1,82 @@
+package app
+
+import (
+	"fmt"
+	"os"
+	"path/filepath"
+	"strings"
+)
+
+func validateScopedTarget(root, target, policy string, requireDir bool) (scopedDir, error) {
+	cleanRoot := strings.TrimSpace(root)
+	cleanTarget := strings.TrimSpace(target)
+	if cleanRoot == "" {
+		return scopedDir{}, fmt.Errorf("cleanup policy %s: root path is required", policy)
+	}
+	if cleanTarget == "" {
+		return scopedDir{}, fmt.Errorf("cleanup policy %s: target path is required", policy)
+	}
+
+	rootAbs, err := filepath.Abs(cleanRoot)
+	if err != nil {
+		return scopedDir{}, fmt.Errorf("cleanup policy %s: resolve root %q: %w", policy, cleanRoot, err)
+	}
+	targetAbs, err := filepath.Abs(cleanTarget)
+	if err != nil {
+		return scopedDir{}, fmt.Errorf("cleanup policy %s: resolve target %q: %w", policy, cleanTarget, err)
+	}
+
+	rel, err := filepath.Rel(rootAbs, targetAbs)
+	if err != nil {
+		return scopedDir{}, fmt.Errorf("cleanup policy %s: relative path from %q to %q: %w", policy, rootAbs, targetAbs, err)
+	}
+	if rel == "." {
+		return scopedDir{}, fmt.Errorf("cleanup policy %s: refusing to delete root directory %q", policy, rootAbs)
+	}
+	if rel == ".." || strings.HasPrefix(rel, ".."+string(filepath.Separator)) {
+		return scopedDir{}, fmt.Errorf("cleanup policy %s: refusing to delete path outside root: root=%q target=%q", policy, rootAbs, targetAbs)
+	}
+
+	info, err := os.Lstat(targetAbs)
+	if err != nil {
+		if os.IsNotExist(err) {
+			return scopedDir{RootAbs: rootAbs, TargetAbs: targetAbs, Exists: false}, nil
+		}
+		return scopedDir{}, fmt.Errorf("cleanup policy %s: stat target %q: %w", policy, targetAbs, err)
+	}
+	if info.Mode()&os.ModeSymlink != 0 {
+		return scopedDir{}, fmt.Errorf("cleanup policy %s: refusing to delete symlink path %q", policy, targetAbs)
+	}
+	if requireDir && !info.IsDir() {
+		return scopedDir{}, fmt.Errorf("cleanup policy %s: target %q is not a directory", policy, targetAbs)
+	}
+	if !requireDir && info.IsDir() {
+		return scopedDir{}, fmt.Errorf("cleanup policy %s: target %q is a directory", policy, targetAbs)
+	}
+	return scopedDir{RootAbs: rootAbs, TargetAbs: targetAbs, Exists: true}, nil
+}
+
+func validateCleanRoot(root, policy string) (string, bool, error) {
+	cleanRoot := strings.TrimSpace(root)
+	if cleanRoot == "" {
+		return "", false, fmt.Errorf("cleanup policy %s: root path is required", policy)
+	}
+	rootAbs, err := filepath.Abs(cleanRoot)
+	if err != nil {
+		return "", false, fmt.Errorf("cleanup policy %s: resolve root %q: %w", policy, cleanRoot, err)
+	}
+	info, err := os.Lstat(rootAbs)
+	if err != nil {
+		if os.IsNotExist(err) {
+			return rootAbs, false, nil
+		}
+		return "", false, fmt.Errorf("cleanup policy %s: stat root %q: %w", policy, rootAbs, err)
+	}
+	if info.Mode()&os.ModeSymlink != 0 {
+		return "", false, fmt.Errorf("cleanup policy %s: refusing to clean symlink root %q", policy, rootAbs)
+	}
+	if !info.IsDir() {
+		return "", false, fmt.Errorf("cleanup policy %s: root %q is not a directory", policy, rootAbs)
+	}
+	return rootAbs, true, nil
+}

internal/app/cleanup_targets_test.go

@@ -0,0 +1,103 @@
+package app
+
+import (
+	"os"
+	"path/filepath"
+	"strings"
+	"testing"
+)
+
+func TestCleanValidateScopedDirAndFile(t *testing.T) {
+	root := t.TempDir()
+	dirTarget := filepath.Join(root, "runs", "run-1")
+	fileTarget := filepath.Join(root, "cache", "a.flac")
+	if err := os.MkdirAll(dirTarget, 0o755); err != nil {
+		t.Fatalf("MkdirAll(dirTarget) error = %v", err)
+	}
+	if err := os.MkdirAll(filepath.Dir(fileTarget), 0o755); err != nil {
+		t.Fatalf("MkdirAll(file parent) error = %v", err)
+	}
+	if err := os.WriteFile(fileTarget, []byte("audio"), 0o644); err != nil {
+		t.Fatalf("WriteFile(fileTarget) error = %v", err)
+	}
+
+	if _, err := validateScopedDir(root, dirTarget, "test.dir"); err != nil {
+		t.Fatalf("validateScopedDir() error = %v", err)
+	}
+	if _, err := validateScopedFile(root, fileTarget, "test.file"); err != nil {
+		t.Fatalf("validateScopedFile() error = %v", err)
+	}
+}
+
+func TestCleanValidateScopedTargetSafetyRules(t *testing.T) {
+	root := t.TempDir()
+	outside := t.TempDir()
+	target := filepath.Join(root, "runs", "run-1")
+	if err := os.MkdirAll(target, 0o755); err != nil {
+		t.Fatalf("MkdirAll(target) error = %v", err)
+	}
+	fileTarget := filepath.Join(root, "cache", "a.flac")
+	if err := os.MkdirAll(filepath.Dir(fileTarget), 0o755); err != nil {
+		t.Fatalf("MkdirAll(file parent) error = %v", err)
+	}
+	if err := os.WriteFile(fileTarget, []byte("audio"), 0o644); err != nil {
+		t.Fatalf("WriteFile(fileTarget) error = %v", err)
+	}
+	symlinkTarget := filepath.Join(root, "symlink")
+	if err := os.Symlink(target, symlinkTarget); err != nil {
+		t.Fatalf("Symlink() error = %v", err)
+	}
+
+	if _, err := validateScopedDir(root, root, "test.root"); err == nil || !strings.Contains(err.Error(), "refusing to delete root directory") {
+		t.Fatalf("validateScopedDir(root) error = %v, want root deletion rejection", err)
+	}
+	if _, err := validateScopedDir(root, filepath.Join(outside, "x"), "test.outside"); err == nil || !strings.Contains(err.Error(), "outside root") {
+		t.Fatalf("validateScopedDir(outside) error = %v, want outside-root rejection", err)
+	}
+	if _, err := validateScopedDir(root, fileTarget, "test.file-as-dir"); err == nil || !strings.Contains(err.Error(), "is not a directory") {
+		t.Fatalf("validateScopedDir(file) error = %v, want not-a-directory rejection", err)
+	}
+	if _, err := validateScopedFile(root, target, "test.dir-as-file"); err == nil || !strings.Contains(err.Error(), "is a directory") {
+		t.Fatalf("validateScopedFile(dir) error = %v, want is-a-directory rejection", err)
+	}
+	if _, err := validateScopedDir(root, symlinkTarget, "test.symlink"); err == nil || !strings.Contains(err.Error(), "refusing to delete symlink path") {
+		t.Fatalf("validateScopedDir(symlink) error = %v, want symlink rejection", err)
+	}
+}
+
+func TestCleanableRootChildrenRejectsSymlinkChild(t *testing.T) {
+	root := t.TempDir()
+	realChild := filepath.Join(root, "runs")
+	if err := os.MkdirAll(realChild, 0o755); err != nil {
+		t.Fatalf("MkdirAll(realChild) error = %v", err)
+	}
+	if err := os.Symlink(realChild, filepath.Join(root, "link")); err != nil {
+		t.Fatalf("Symlink() error = %v", err)
+	}
+
+	_, _, err := cleanableRootChildren(root, "test.root.children")
+	if err == nil || !strings.Contains(err.Error(), "refusing to delete symlink path") {
+		t.Fatalf("cleanableRootChildren() error = %v, want symlink rejection", err)
+	}
+}
+
+func TestCleanValidateScopedTargetMissing(t *testing.T) {
+	root := t.TempDir()
+	missingDir := filepath.Join(root, "runs", "missing")
+	got, err := validateScopedDir(root, missingDir, "test.missing")
+	if err != nil {
+		t.Fatalf("validateScopedDir(missing) error = %v", err)
+	}
+	if got.Exists {
+		t.Fatalf("validateScopedDir(missing).Exists = true, want false")
+	}
+
+	missingFile := filepath.Join(root, "cache", "missing.flac")
+	got, err = validateScopedFile(root, missingFile, "test.missing.file")
+	if err != nil {
+		t.Fatalf("validateScopedFile(missing) error = %v", err)
+	}
+	if got.Exists {
+		t.Fatalf("validateScopedFile(missing).Exists = true, want false")
+	}
+}

internal/app/operator_helpers.go

@@ -15,6 +15,7 @@ import (
 	"gitea.maximumdirect.net/eric/narratio/internal/artifacts"
 	"gitea.maximumdirect.net/eric/narratio/internal/config"
 	"gitea.maximumdirect.net/eric/narratio/internal/manifest"
+	"gitea.maximumdirect.net/eric/narratio/internal/pathsafe"
 	"gopkg.in/yaml.v3"
 )
 
@@ -1070,7 +1071,7 @@ func helperPublishedOutputDest(rule config.PublishOutputRule, catalog *artifacts
 			return "", false, fmt.Errorf("destination omitted and no canonical destination is available")
 		}
 	}
-	normalized, err := normalizeHelperArchiveRelativePath(dest)
+	normalized, err := pathsafe.NormalizeRelativeDestination(dest)
 	if err != nil {
 		return "", false, err
 	}
@@ -1079,21 +1080,6 @@ func helperPublishedOutputDest(rule config.PublishOutputRule, catalog *artifacts
 	return normalized, showDest, nil
 }
 
-func normalizeHelperArchiveRelativePath(rel string) (string, error) {
-	trimmed := strings.TrimSpace(rel)
-	if trimmed == "" {
-		return "", fmt.Errorf("relative path is required")
-	}
-	cleaned := filepath.ToSlash(filepath.Clean(filepath.FromSlash(trimmed)))
-	if cleaned == "." || cleaned == "" {
-		return "", fmt.Errorf("relative path is required")
-	}
-	if filepath.IsAbs(trimmed) || strings.HasPrefix(cleaned, "/") || cleaned == ".." || strings.HasPrefix(cleaned, "../") {
-		return "", fmt.Errorf("path must be a clean relative path")
-	}
-	return cleaned, nil
-}
-
 func publishedOutputRemoteStateKey(source, dest string) string {
 	return strings.TrimSpace(source) + "\x00" + strings.TrimSpace(dest)
 }

internal/app/post_archive_cleanup.go

@@ -174,49 +174,7 @@ func removeRunScopedDir(root, target, policy string) error {
 }
 
 func validateScopedDir(root, target, policy string) (scopedDir, error) {
-	cleanRoot := strings.TrimSpace(root)
-	cleanTarget := strings.TrimSpace(target)
-	if cleanRoot == "" {
-		return scopedDir{}, fmt.Errorf("cleanup policy %s: root path is required", policy)
-	}
-	if cleanTarget == "" {
-		return scopedDir{}, fmt.Errorf("cleanup policy %s: target path is required", policy)
-	}
-
-	rootAbs, err := filepath.Abs(cleanRoot)
-	if err != nil {
-		return scopedDir{}, fmt.Errorf("cleanup policy %s: resolve root %q: %w", policy, cleanRoot, err)
-	}
-	targetAbs, err := filepath.Abs(cleanTarget)
-	if err != nil {
-		return scopedDir{}, fmt.Errorf("cleanup policy %s: resolve target %q: %w", policy, cleanTarget, err)
-	}
-
-	rel, err := filepath.Rel(rootAbs, targetAbs)
-	if err != nil {
-		return scopedDir{}, fmt.Errorf("cleanup policy %s: relative path from %q to %q: %w", policy, rootAbs, targetAbs, err)
-	}
-	if rel == "." {
-		return scopedDir{}, fmt.Errorf("cleanup policy %s: refusing to delete root directory %q", policy, rootAbs)
-	}
-	if rel == ".." || strings.HasPrefix(rel, ".."+string(filepath.Separator)) {
-		return scopedDir{}, fmt.Errorf("cleanup policy %s: refusing to delete path outside root: root=%q target=%q", policy, rootAbs, targetAbs)
-	}
-
-	info, err := os.Lstat(targetAbs)
-	if err != nil {
-		if os.IsNotExist(err) {
-			return scopedDir{RootAbs: rootAbs, TargetAbs: targetAbs, Exists: false}, nil
-		}
-		return scopedDir{}, fmt.Errorf("cleanup policy %s: stat target %q: %w", policy, targetAbs, err)
-	}
-	if info.Mode()&os.ModeSymlink != 0 {
-		return scopedDir{}, fmt.Errorf("cleanup policy %s: refusing to delete symlink path %q", policy, targetAbs)
-	}
-	if !info.IsDir() {
-		return scopedDir{}, fmt.Errorf("cleanup policy %s: target %q is not a directory", policy, targetAbs)
-	}
-	return scopedDir{RootAbs: rootAbs, TargetAbs: targetAbs, Exists: true}, nil
+	return validateScopedTarget(root, target, policy, true)
 }
 
 func asString(v any) string {

internal/app/remote_locks.go

@@ -46,7 +46,7 @@ func loadRemoteLockStore(ctx context.Context, cfg *config.Config, store storage.
 	if !exists {
 		return &config.PublishLockStore{}, key, nil
 	}
-	tmp, err := downloadObjectToTemp(ctx, store, key, "narratio-locks-*.yml")
+	tmp, err := storage.DownloadObjectToTemp(ctx, store, key, "narratio-locks-*.yml")
 	if err != nil {
 		return nil, key, fmt.Errorf("download remote locks %q: %w", key, err)
 	}

internal/app/restore_discovery.go

@@ -50,7 +50,7 @@ func discoverRemoteCurrentState(ctx context.Context, cfg *config.Config, store s
 		return nil, fmt.Errorf("remote current run pointer missing: %q", currentRunIDKey)
 	}
 
-	runIDPath, err := downloadObjectToTemp(ctx, store, currentRunIDKey, "narratio-restore-current-run-id-*.txt")
+	runIDPath, err := storage.DownloadObjectToTemp(ctx, store, currentRunIDKey, "narratio-restore-current-run-id-*.txt")
 	if err != nil {
 		return nil, fmt.Errorf("download remote current run pointer %q: %w", currentRunIDKey, err)
 	}
@@ -73,7 +73,7 @@ func discoverRemoteCurrentState(ctx context.Context, cfg *config.Config, store s
 		return nil, fmt.Errorf("remote current manifest missing: %q", currentManifestKey)
 	}
 
-	manifestPath, err := downloadObjectToTemp(ctx, store, currentManifestKey, "narratio-restore-current-manifest-*.json")
+	manifestPath, err := storage.DownloadObjectToTemp(ctx, store, currentManifestKey, "narratio-restore-current-manifest-*.json")
 	if err != nil {
 		return nil, fmt.Errorf("download remote current manifest %q: %w", currentManifestKey, err)
 	}
@@ -119,21 +119,3 @@ func discoverRemoteCurrentState(ctx context.Context, cfg *config.Config, store s
 		Manifest:           remoteManifest,
 	}, nil
 }
-
-func downloadObjectToTemp(ctx context.Context, store storage.ObjectStore, key, pattern string) (string, error) {
-	tmp, err := os.CreateTemp("", pattern)
-	if err != nil {
-		return "", fmt.Errorf("create temp file: %w", err)
-	}
-	path := tmp.Name()
-	if err := tmp.Close(); err != nil {
-		_ = os.Remove(path)
-		return "", fmt.Errorf("close temp file: %w", err)
-	}
-
-	if err := store.Download(ctx, key, path); err != nil {
-		_ = os.Remove(path)
-		return "", err
-	}
-	return path, nil
-}

internal/app/restore_plan.go

@@ -338,7 +338,7 @@ func classifyRestoreAction(
 	if err != nil {
 		return RestoreAction{}, fmt.Errorf("checksum local file: %w", err)
 	}
-	remotePath, err := downloadObjectToTemp(ctx, store, action.RemoteKey, "narratio-restore-plan-remote-*.tmp")
+	remotePath, err := storage.DownloadObjectToTemp(ctx, store, action.RemoteKey, "narratio-restore-plan-remote-*.tmp")
 	if err != nil {
 		return RestoreAction{}, fmt.Errorf("download remote object: %w", err)
 	}