diff --git a/internal/adapters/storage/temp_download.go b/internal/adapters/storage/temp_download.go new file mode 100644 index 0000000..5fda8fa --- /dev/null +++ b/internal/adapters/storage/temp_download.go @@ -0,0 +1,36 @@ +package storage + +import ( + "context" + "fmt" + "os" + "path/filepath" + "strings" +) + +// DownloadObjectToTemp downloads an object into a temporary file and returns +// the cleaned local path. +func DownloadObjectToTemp(ctx context.Context, store ObjectStore, key, pattern string) (string, error) { + if store == nil { + return "", fmt.Errorf("object store is required") + } + if strings.TrimSpace(pattern) == "" { + return "", fmt.Errorf("temp file pattern is required") + } + + 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 filepath.Clean(path), nil +} diff --git a/internal/adapters/storage/temp_download_test.go b/internal/adapters/storage/temp_download_test.go new file mode 100644 index 0000000..6f6ea48 --- /dev/null +++ b/internal/adapters/storage/temp_download_test.go @@ -0,0 +1,69 @@ +package storage + +import ( + "context" + "errors" + "fmt" + "os" + "path/filepath" + "strings" + "testing" +) + +func TestDownloadObjectToTempSuccess(t *testing.T) { + store := &FakeBackend{} + store.SeedObject(FakeObject{Key: "sessions/a/current/run_id.txt", Data: []byte("run-123\n")}) + + path, err := DownloadObjectToTemp(context.Background(), store, "sessions/a/current/run_id.txt", "narratio-test-*.txt") + if err != nil { + t.Fatalf("DownloadObjectToTemp() error = %v", err) + } + t.Cleanup(func() { _ = os.Remove(path) }) + + data, err := os.ReadFile(path) + if err != nil { + t.Fatalf("ReadFile() error = %v", err) + } + if string(data) != "run-123\n" { + t.Fatalf("downloaded data = %q, want %q", string(data), "run-123\n") + } +} + +func TestDownloadObjectToTempFailedDownloadRemovesTempFile(t *testing.T) { + sentinel := errors.New("download failed") + store := &FakeBackend{DownloadErr: sentinel} + pattern := "narratio-test-fail-*.txt" + before, err := filepath.Glob(filepath.Join(os.TempDir(), "narratio-test-fail-*.txt")) + if err != nil { + t.Fatalf("Glob(before) error = %v", err) + } + + path, err := DownloadObjectToTemp(context.Background(), store, "sessions/a/current/run_id.txt", pattern) + if !errors.Is(err, sentinel) { + t.Fatalf("DownloadObjectToTemp() error = %v, want %v", err, sentinel) + } + if strings.TrimSpace(path) != "" { + t.Fatalf("DownloadObjectToTemp() path = %q, want empty on failure", path) + } + after, err := filepath.Glob(filepath.Join(os.TempDir(), "narratio-test-fail-*.txt")) + if err != nil { + t.Fatalf("Glob(after) error = %v", err) + } + if len(after) != len(before) { + t.Fatalf("temp file count changed after failed download: before=%d after=%d", len(before), len(after)) + } +} + +func TestDownloadObjectToTempCallerContextWrappingPreservesCause(t *testing.T) { + sentinel := errors.New("object missing") + store := &FakeBackend{DownloadErr: sentinel} + + _, err := DownloadObjectToTemp(context.Background(), store, "sessions/a/current/run_id.txt", "narratio-test-*.txt") + if err == nil { + t.Fatal("DownloadObjectToTemp() error = nil, want error") + } + err = fmt.Errorf("download run pointer failed: %w", err) + if !errors.Is(err, sentinel) { + t.Fatalf("wrapped error does not preserve sentinel cause: %v", err) + } +} diff --git a/internal/app/clean.go b/internal/app/clean.go index 9051a82..3c107f0 100644 --- a/internal/app/clean.go +++ b/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 { diff --git a/internal/app/cleanup_targets.go b/internal/app/cleanup_targets.go new file mode 100644 index 0000000..4ca8e2c --- /dev/null +++ b/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 +} diff --git a/internal/app/cleanup_targets_test.go b/internal/app/cleanup_targets_test.go new file mode 100644 index 0000000..39330c1 --- /dev/null +++ b/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") + } +} diff --git a/internal/app/operator_helpers.go b/internal/app/operator_helpers.go index d952d2a..9c8a7db 100644 --- a/internal/app/operator_helpers.go +++ b/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) } diff --git a/internal/app/post_archive_cleanup.go b/internal/app/post_archive_cleanup.go index 40f7175..3a9d953 100644 --- a/internal/app/post_archive_cleanup.go +++ b/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 { diff --git a/internal/app/remote_locks.go b/internal/app/remote_locks.go index a94a809..845d45b 100644 --- a/internal/app/remote_locks.go +++ b/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) } diff --git a/internal/app/restore_discovery.go b/internal/app/restore_discovery.go index ced27a1..ed9cead 100644 --- a/internal/app/restore_discovery.go +++ b/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 -} diff --git a/internal/app/restore_plan.go b/internal/app/restore_plan.go index a0b7d2b..ea0ff60 100644 --- a/internal/app/restore_plan.go +++ b/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) } diff --git a/internal/config/validate.go b/internal/config/validate.go index 2066cbb..db52677 100644 --- a/internal/config/validate.go +++ b/internal/config/validate.go @@ -1,6 +1,7 @@ package config import ( + "errors" "fmt" "net/url" "path/filepath" @@ -9,6 +10,7 @@ import ( "time" "gitea.maximumdirect.net/eric/narratio/internal/artifactmodel" + "gitea.maximumdirect.net/eric/narratio/internal/pathsafe" ) // Validate checks resolved configuration for required fields and parseable durations. @@ -152,10 +154,17 @@ func validatePublish(cfg *PublishConfig, scriptorium *ScriptoriumConfig) error { dest = derivedDest cfg.Outputs[i].Dest = derivedDest } - if err := validateRelativeSafePath(prefix+".dest", dest); err != nil { - return err + normalizedDest, err := pathsafe.NormalizeRelativeDestination(dest) + if err != nil { + switch { + case errors.Is(err, pathsafe.ErrRelativePathAbsolute): + return fmt.Errorf("%s.dest must be a relative path", prefix) + case errors.Is(err, pathsafe.ErrRelativePathEscape): + return fmt.Errorf("%s.dest must not contain path traversal", prefix) + default: + return fmt.Errorf("%s.dest must be non-empty", prefix) + } } - normalizedDest := filepath.ToSlash(filepath.Clean(dest)) if _, ok := seenDest[normalizedDest]; ok { return fmt.Errorf("%s.dest %q duplicates another publish output destination", prefix, dest) } diff --git a/internal/pathsafe/relative_destination.go b/internal/pathsafe/relative_destination.go new file mode 100644 index 0000000..3e50585 --- /dev/null +++ b/internal/pathsafe/relative_destination.go @@ -0,0 +1,38 @@ +package pathsafe + +import ( + "errors" + "path" + "regexp" + "strings" +) + +var ( + ErrRelativePathRequired = errors.New("relative path is required") + ErrRelativePathAbsolute = errors.New("relative path must not be absolute") + ErrRelativePathEscape = errors.New("relative path escapes root") +) + +var windowsAbsPathRE = regexp.MustCompile(`^[a-zA-Z]:[\\/]`) + +// NormalizeRelativeDestination validates and normalizes a relative destination +// path to slash-separated form. +func NormalizeRelativeDestination(relative string) (string, error) { + trimmed := strings.TrimSpace(relative) + if trimmed == "" { + return "", ErrRelativePathRequired + } + if strings.HasPrefix(trimmed, "/") || strings.HasPrefix(trimmed, `\`) || windowsAbsPathRE.MatchString(trimmed) { + return "", ErrRelativePathAbsolute + } + + normalized := strings.ReplaceAll(trimmed, `\`, "/") + cleaned := path.Clean(normalized) + if cleaned == "." || cleaned == "" { + return "", ErrRelativePathRequired + } + if cleaned == ".." || strings.HasPrefix(cleaned, "../") { + return "", ErrRelativePathEscape + } + return cleaned, nil +} diff --git a/internal/pathsafe/relative_destination_test.go b/internal/pathsafe/relative_destination_test.go new file mode 100644 index 0000000..001d16f --- /dev/null +++ b/internal/pathsafe/relative_destination_test.go @@ -0,0 +1,43 @@ +package pathsafe + +import ( + "errors" + "testing" +) + +func TestNormalizeRelativeDestination(t *testing.T) { + tests := []struct { + name string + input string + want string + wantErr error + }{ + {name: "simple", input: "artifacts/session_recap.md", want: "artifacts/session_recap.md"}, + {name: "backslashes become slashes", input: `artifacts\session_recap.md`, want: "artifacts/session_recap.md"}, + {name: "cleans dot segments", input: "artifacts/./session_recap.md", want: "artifacts/session_recap.md"}, + {name: "reject empty", input: "", wantErr: ErrRelativePathRequired}, + {name: "reject dot", input: ".", wantErr: ErrRelativePathRequired}, + {name: "reject unix absolute", input: "/artifacts/session_recap.md", wantErr: ErrRelativePathAbsolute}, + {name: "reject windows absolute", input: `C:\artifacts\session_recap.md`, wantErr: ErrRelativePathAbsolute}, + {name: "reject traversal", input: "../artifacts/session_recap.md", wantErr: ErrRelativePathEscape}, + {name: "reject traversal after clean", input: "a/../../session_recap.md", wantErr: ErrRelativePathEscape}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got, err := NormalizeRelativeDestination(tt.input) + if tt.wantErr != nil { + if !errors.Is(err, tt.wantErr) { + t.Fatalf("NormalizeRelativeDestination() error = %v, want %v", err, tt.wantErr) + } + return + } + if err != nil { + t.Fatalf("NormalizeRelativeDestination() error = %v", err) + } + if got != tt.want { + t.Fatalf("NormalizeRelativeDestination() = %q, want %q", got, tt.want) + } + }) + } +} diff --git a/internal/previouscache/previouscache.go b/internal/previouscache/previouscache.go index 287599f..1d71971 100644 --- a/internal/previouscache/previouscache.go +++ b/internal/previouscache/previouscache.go @@ -13,6 +13,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" ) const ( @@ -106,7 +107,7 @@ func BuildPlan( return result, nil } - runIDTemp, err := downloadObjectToTemp(ctx, store, currentRunIDKey, "narratio-previous-run-id-*.txt") + runIDTemp, err := storage.DownloadObjectToTemp(ctx, store, currentRunIDKey, "narratio-previous-run-id-*.txt") if err != nil { return nil, fmt.Errorf("download previous-session current run pointer %q: %w", currentRunIDKey, err) } @@ -134,7 +135,7 @@ func BuildPlan( return result, nil } - manifestTemp, err := downloadObjectToTemp(ctx, store, currentManifestKey, "narratio-previous-manifest-*.json") + manifestTemp, err := storage.DownloadObjectToTemp(ctx, store, currentManifestKey, "narratio-previous-manifest-*.json") if err != nil { return nil, fmt.Errorf("download previous-session current manifest %q: %w", currentManifestKey, err) } @@ -278,7 +279,7 @@ func artifactRelativePathCandidates( ) []string { candidates := []string{} appendCandidate := func(v string) { - normalized, err := normalizeArchiveRelativePath(v) + normalized, err := pathsafe.NormalizeRelativeDestination(v) if err != nil { return } @@ -354,7 +355,7 @@ func deriveManifestRelativePath(previousManifest *manifest.Manifest, localPath s return "", false } if !filepath.IsAbs(trimmed) { - normalized, err := normalizeArchiveRelativePath(filepath.ToSlash(trimmed)) + normalized, err := pathsafe.NormalizeRelativeDestination(filepath.ToSlash(trimmed)) if err != nil { return "", false } @@ -369,7 +370,7 @@ func deriveManifestRelativePath(previousManifest *manifest.Manifest, localPath s if err != nil { return "", false } - normalized, err := normalizeArchiveRelativePath(filepath.ToSlash(rel)) + normalized, err := pathsafe.NormalizeRelativeDestination(filepath.ToSlash(rel)) if err != nil { return "", false } @@ -417,7 +418,7 @@ func manifestPublishedPaths(previousManifest *manifest.Manifest) []string { if !ok { continue } - normalized, err := normalizeArchiveRelativePath(asString) + normalized, err := pathsafe.NormalizeRelativeDestination(asString) if err != nil { continue } @@ -426,21 +427,6 @@ func manifestPublishedPaths(previousManifest *manifest.Manifest) []string { return dedupeOrderedStrings(out) } -func normalizeArchiveRelativePath(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 relativeToSession(paths artifacts.SessionPaths, localPath string) (string, error) { root := filepath.Clean(paths.Root) if strings.TrimSpace(root) == "" { @@ -450,7 +436,7 @@ func relativeToSession(paths artifacts.SessionPaths, localPath string) (string, if err != nil { return "", fmt.Errorf("resolve previous-cache relative path: %w", err) } - normalized, err := normalizeArchiveRelativePath(filepath.ToSlash(rel)) + normalized, err := pathsafe.NormalizeRelativeDestination(filepath.ToSlash(rel)) if err != nil { return "", fmt.Errorf("resolve previous-cache relative path: %w", err) } @@ -476,20 +462,3 @@ func dedupeOrderedStrings(values []string) []string { } return out } - -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 -} diff --git a/internal/stage/archive.go b/internal/stage/archive.go index 1845daa..f5a215f 100644 --- a/internal/stage/archive.go +++ b/internal/stage/archive.go @@ -16,6 +16,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" ) type archiveStage struct{} @@ -463,22 +464,7 @@ func resolvePublishOutputDest(rule config.PublishOutputRule, catalog *artifacts. return "", fmt.Errorf("destination omitted and no canonical destination is available") } } - return normalizeArchiveRelativePath(dest) -} - -func normalizeArchiveRelativePath(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 + return pathsafe.NormalizeRelativeDestination(dest) } func buildArchiveRuntimeArtifactCatalog(