diff --git a/internal/app/restore_execute.go b/internal/app/restore_execute.go index 34d5619..c9ee029 100644 --- a/internal/app/restore_execute.go +++ b/internal/app/restore_execute.go @@ -11,6 +11,7 @@ import ( "gitea.maximumdirect.net/eric/narratio/internal/artifacts" "gitea.maximumdirect.net/eric/narratio/internal/audio" "gitea.maximumdirect.net/eric/narratio/internal/config" + "gitea.maximumdirect.net/eric/narratio/internal/fileops" "gitea.maximumdirect.net/eric/narratio/internal/manifest" ) @@ -114,10 +115,7 @@ func executeRestoreDownloadAction( } } - if err := os.Chmod(tmpPath, 0o644); err != nil { - return fmt.Errorf("set file permissions: %w", err) - } - if err := os.Rename(tmpPath, safeLocalPath); err != nil { + if err := fileops.InstallDownloadedTempFile(tmpPath, safeLocalPath, 0o644); err != nil { return fmt.Errorf("install file atomically: %w", err) } removeTmp = false diff --git a/internal/app/restore_plan.go b/internal/app/restore_plan.go index ea0ff60..00ab3ac 100644 --- a/internal/app/restore_plan.go +++ b/internal/app/restore_plan.go @@ -2,6 +2,7 @@ package app import ( "context" + "errors" "fmt" "io" "os" @@ -13,6 +14,7 @@ import ( "gitea.maximumdirect.net/eric/narratio/internal/adapters/storage" "gitea.maximumdirect.net/eric/narratio/internal/artifacts" "gitea.maximumdirect.net/eric/narratio/internal/config" + "gitea.maximumdirect.net/eric/narratio/internal/pathsafe" "gitea.maximumdirect.net/eric/narratio/internal/previouscache" ) @@ -215,19 +217,17 @@ func joinWithinSessionRoot(sessionRoot, relative string) (string, error) { if strings.TrimSpace(sessionRoot) == "" { return "", fmt.Errorf("session root is required") } - cleanRel := path.Clean(strings.TrimSpace(relative)) - if cleanRel == "." || cleanRel == "" { - return "", fmt.Errorf("relative path is required") + joined, err := pathsafe.JoinSlashRelativeUnderRoot(sessionRoot, filepath.ToSlash(strings.TrimSpace(relative))) + if err != nil { + if errors.Is(err, pathsafe.ErrRelativePathRequired) { + return "", fmt.Errorf("relative path is required") + } + if errors.Is(err, pathsafe.ErrRelativePathEscape) || errors.Is(err, pathsafe.ErrRelativePathAbsolute) { + return "", fmt.Errorf("relative path escapes session root") + } + return "", fmt.Errorf("join relative path under session root: %w", err) } - if cleanRel == ".." || strings.HasPrefix(cleanRel, "../") || strings.HasPrefix(cleanRel, "/") { - return "", fmt.Errorf("relative path escapes session root") - } - abs := filepath.Clean(filepath.Join(sessionRoot, filepath.FromSlash(cleanRel))) - root := filepath.Clean(sessionRoot) - if abs != root && !strings.HasPrefix(abs, root+string(filepath.Separator)) { - return "", fmt.Errorf("resolved local path escapes session root") - } - return abs, nil + return joined, nil } func buildPreviousCacheRestoreActions( diff --git a/internal/artifacts/local.go b/internal/artifacts/local.go index c823672..f9f424b 100644 --- a/internal/artifacts/local.go +++ b/internal/artifacts/local.go @@ -9,6 +9,9 @@ import ( "strconv" "strings" "time" + + "gitea.maximumdirect.net/eric/narratio/internal/fileops" + "gitea.maximumdirect.net/eric/narratio/internal/pathsafe" ) // ErrLockConflict is returned when a session lock already exists. @@ -101,7 +104,7 @@ func (s *LocalStore) copyInputWithPaths(paths SessionPaths, sessionID, srcPath, return Ref{}, fmt.Errorf("copy input: %w", err) } - if err := copyFileAtomic(srcPath, destAbs, 0o644); err != nil { + if err := fileops.CopyFileAtomic(srcPath, destAbs, 0o644); err != nil { return Ref{}, fmt.Errorf("copy input %q -> %q: %w", srcPath, destAbs, err) } @@ -145,45 +148,9 @@ func (s *LocalStore) WriteFileAtomic(path string, data []byte, perm os.FileMode) if strings.TrimSpace(path) == "" { return fmt.Errorf("write file atomic: path is required") } - - dir := filepath.Dir(path) - if err := os.MkdirAll(dir, 0o755); err != nil { - return fmt.Errorf("write file atomic: create parent dir %q: %w", dir, err) + if err := fileops.WriteFileAtomic(path, data, perm); err != nil { + return fmt.Errorf("write file atomic: %w", err) } - - base := filepath.Base(path) - tmp, err := os.CreateTemp(dir, "."+base+".tmp-*") - if err != nil { - return fmt.Errorf("write file atomic: create temp file: %w", err) - } - tmpName := tmp.Name() - removeTmp := true - defer func() { - if removeTmp { - _ = os.Remove(tmpName) - } - }() - - if _, err := tmp.Write(data); err != nil { - _ = tmp.Close() - return fmt.Errorf("write file atomic: write temp file: %w", err) - } - if err := tmp.Sync(); err != nil { - _ = tmp.Close() - return fmt.Errorf("write file atomic: sync temp file: %w", err) - } - if err := tmp.Close(); err != nil { - return fmt.Errorf("write file atomic: close temp file: %w", err) - } - - if err := os.Chmod(tmpName, perm); err != nil { - return fmt.Errorf("write file atomic: chmod temp file: %w", err) - } - if err := os.Rename(tmpName, path); err != nil { - return fmt.Errorf("write file atomic: rename temp file: %w", err) - } - removeTmp = false - return nil } @@ -256,62 +223,21 @@ func (s *LocalStore) ReleaseSessionLock(lock *LockHandle) error { } func resolveInRoot(root, relative string) (string, error) { - rel := filepath.Clean(relative) - if rel == "." || rel == "" { + joined, err := pathsafe.JoinSlashRelativeUnderRoot(root, filepath.ToSlash(relative)) + if err != nil { + switch { + case errors.Is(err, pathsafe.ErrRelativePathRequired): + return "", fmt.Errorf("relative destination path is required") + case errors.Is(err, pathsafe.ErrRelativePathAbsolute): + return "", fmt.Errorf("relative destination must not be absolute: %q", relative) + case errors.Is(err, pathsafe.ErrRelativePathEscape): + return "", fmt.Errorf("relative destination escapes root: %q", relative) + default: + return "", fmt.Errorf("resolve destination in root: %w", err) + } + } + if strings.TrimSpace(joined) == "" { return "", fmt.Errorf("relative destination path is required") } - if filepath.IsAbs(rel) { - return "", fmt.Errorf("relative destination must not be absolute: %q", relative) - } - if rel == ".." || strings.HasPrefix(rel, ".."+string(filepath.Separator)) { - return "", fmt.Errorf("relative destination escapes root: %q", relative) - } - return filepath.Join(root, rel), nil -} - -func copyFileAtomic(srcPath, dstPath string, perm os.FileMode) error { - src, err := os.Open(srcPath) - if err != nil { - return err - } - defer src.Close() - - if err := os.MkdirAll(filepath.Dir(dstPath), 0o755); err != nil { - return err - } - - dir := filepath.Dir(dstPath) - base := filepath.Base(dstPath) - tmp, err := os.CreateTemp(dir, "."+base+".tmp-*") - if err != nil { - return err - } - tmpName := tmp.Name() - removeTmp := true - defer func() { - if removeTmp { - _ = os.Remove(tmpName) - } - }() - - if _, err := io.Copy(tmp, src); err != nil { - _ = tmp.Close() - return err - } - if err := tmp.Sync(); err != nil { - _ = tmp.Close() - return err - } - if err := tmp.Close(); err != nil { - return err - } - if err := os.Chmod(tmpName, perm); err != nil { - return err - } - if err := os.Rename(tmpName, dstPath); err != nil { - return err - } - removeTmp = false - - return nil + return joined, nil } diff --git a/internal/audio/s3_audio.go b/internal/audio/s3_audio.go index 64f71fb..10348bd 100644 --- a/internal/audio/s3_audio.go +++ b/internal/audio/s3_audio.go @@ -2,16 +2,14 @@ package audio import ( "context" - "crypto/sha256" - "encoding/hex" "fmt" - "io" "os" "path/filepath" "strings" "gitea.maximumdirect.net/eric/narratio/internal/adapters/storage" "gitea.maximumdirect.net/eric/narratio/internal/artifacts" + "gitea.maximumdirect.net/eric/narratio/internal/fileops" ) // S3MaterializeRequest describes one S3-backed audio materialization. @@ -61,7 +59,7 @@ func MaterializeS3Audio(ctx context.Context, req S3MaterializeRequest) (S3Materi if ok, err := validCachedAudio(cachePath, req.Object.Size); err != nil { return S3MaterializeResult{}, err } else if ok { - checksum, err := copyFileAtomicWithChecksum(cachePath, req.DestPath, 0o644) + checksum, err := fileops.CopyFileAtomicWithChecksum(cachePath, req.DestPath, 0o644) if err != nil { return S3MaterializeResult{}, fmt.Errorf("materialize cached audio %q: %w", cachePath, err) } @@ -82,7 +80,7 @@ func MaterializeS3Audio(ctx context.Context, req S3MaterializeRequest) (S3Materi return S3MaterializeResult{}, fmt.Errorf("validate downloaded audio %q: %w", spoolPath, err) } - checksum, err := copyFileAtomicWithChecksum(spoolPath, req.DestPath, 0o644) + checksum, err := fileops.CopyFileAtomicWithChecksum(spoolPath, req.DestPath, 0o644) if err != nil { return S3MaterializeResult{}, fmt.Errorf("materialize downloaded audio %q: %w", filepath.Base(req.DestPath), err) } @@ -91,7 +89,7 @@ func MaterializeS3Audio(ctx context.Context, req S3MaterializeRequest) (S3Materi result.Downloaded = true if result.CachePath != "" { - if _, err := copyFileAtomicWithChecksum(spoolPath, result.CachePath, 0o644); err != nil { + if _, err := fileops.CopyFileAtomicWithChecksum(spoolPath, result.CachePath, 0o644); err != nil { return S3MaterializeResult{}, fmt.Errorf("populate audio cache %q: %w", result.CachePath, err) } } @@ -164,60 +162,9 @@ func downloadObjectAtomic(ctx context.Context, store storage.ObjectStore, key, d if err := store.Download(ctx, key, tmpPath); err != nil { return err } - if err := os.Chmod(tmpPath, 0o644); err != nil { - return fmt.Errorf("set temp file permissions: %w", err) - } - if err := os.Rename(tmpPath, destPath); err != nil { - return fmt.Errorf("install downloaded file: %w", err) + if err := fileops.InstallDownloadedTempFile(tmpPath, destPath, 0o644); err != nil { + return err } removeTmp = false return nil } - -func copyFileAtomicWithChecksum(src, dst string, perm os.FileMode) (string, error) { - if strings.TrimSpace(src) == "" || strings.TrimSpace(dst) == "" { - return "", fmt.Errorf("source and destination paths are required") - } - in, err := os.Open(src) - if err != nil { - return "", err - } - defer func() { _ = in.Close() }() - - if err := os.MkdirAll(filepath.Dir(dst), 0o755); err != nil { - return "", fmt.Errorf("create destination directory: %w", err) - } - base := filepath.Base(dst) - tmp, err := os.CreateTemp(filepath.Dir(dst), "."+base+".tmp-*") - if err != nil { - return "", fmt.Errorf("create temp file: %w", err) - } - tmpPath := tmp.Name() - removeTmp := true - defer func() { - if removeTmp { - _ = os.Remove(tmpPath) - } - }() - - digest := sha256.New() - if _, err := io.Copy(io.MultiWriter(tmp, digest), in); err != nil { - _ = tmp.Close() - return "", fmt.Errorf("copy file: %w", err) - } - if err := tmp.Sync(); err != nil { - _ = tmp.Close() - return "", fmt.Errorf("sync temp file: %w", err) - } - if err := tmp.Close(); err != nil { - return "", fmt.Errorf("close temp file: %w", err) - } - if err := os.Chmod(tmpPath, perm); err != nil { - return "", fmt.Errorf("chmod temp file: %w", err) - } - if err := os.Rename(tmpPath, dst); err != nil { - return "", fmt.Errorf("install temp file: %w", err) - } - removeTmp = false - return hex.EncodeToString(digest.Sum(nil)), nil -} diff --git a/internal/fileops/fileops.go b/internal/fileops/fileops.go new file mode 100644 index 0000000..9f5b25f --- /dev/null +++ b/internal/fileops/fileops.go @@ -0,0 +1,128 @@ +package fileops + +import ( + "crypto/sha256" + "encoding/hex" + "fmt" + "io" + "os" + "path/filepath" + "strings" +) + +// WriteFileAtomic writes data to dst atomically via temp file + rename. +func WriteFileAtomic(dst string, data []byte, perm os.FileMode) error { + if strings.TrimSpace(dst) == "" { + return fmt.Errorf("destination path is required") + } + if err := os.MkdirAll(filepath.Dir(dst), 0o755); err != nil { + return fmt.Errorf("create destination directory: %w", err) + } + + base := filepath.Base(dst) + tmp, err := os.CreateTemp(filepath.Dir(dst), "."+base+".tmp-*") + if err != nil { + return fmt.Errorf("create temp file: %w", err) + } + tmpPath := tmp.Name() + removeTmp := true + defer func() { + if removeTmp { + _ = os.Remove(tmpPath) + } + }() + + if _, err := tmp.Write(data); err != nil { + _ = tmp.Close() + return fmt.Errorf("write temp file: %w", err) + } + if err := tmp.Sync(); err != nil { + _ = tmp.Close() + return fmt.Errorf("sync temp file: %w", err) + } + if err := tmp.Close(); err != nil { + return fmt.Errorf("close temp file: %w", err) + } + if err := os.Chmod(tmpPath, perm); err != nil { + return fmt.Errorf("set temp file permissions: %w", err) + } + if err := os.Rename(tmpPath, dst); err != nil { + return fmt.Errorf("install temp file: %w", err) + } + removeTmp = false + return nil +} + +// CopyFileAtomic copies src to dst atomically via temp file + rename. +func CopyFileAtomic(src, dst string, perm os.FileMode) error { + _, err := CopyFileAtomicWithChecksum(src, dst, perm) + return err +} + +// CopyFileAtomicWithChecksum copies src to dst atomically and returns the SHA-256 checksum. +func CopyFileAtomicWithChecksum(src, dst string, perm os.FileMode) (string, error) { + if strings.TrimSpace(src) == "" || strings.TrimSpace(dst) == "" { + return "", fmt.Errorf("source and destination paths are required") + } + + in, err := os.Open(src) + if err != nil { + return "", fmt.Errorf("open source file: %w", err) + } + defer func() { _ = in.Close() }() + + if err := os.MkdirAll(filepath.Dir(dst), 0o755); err != nil { + return "", fmt.Errorf("create destination directory: %w", err) + } + + base := filepath.Base(dst) + tmp, err := os.CreateTemp(filepath.Dir(dst), "."+base+".tmp-*") + if err != nil { + return "", fmt.Errorf("create temp file: %w", err) + } + tmpPath := tmp.Name() + removeTmp := true + defer func() { + if removeTmp { + _ = os.Remove(tmpPath) + } + }() + + digest := sha256.New() + if _, err := io.Copy(io.MultiWriter(tmp, digest), in); err != nil { + _ = tmp.Close() + return "", fmt.Errorf("copy file: %w", err) + } + if err := tmp.Sync(); err != nil { + _ = tmp.Close() + return "", fmt.Errorf("sync temp file: %w", err) + } + if err := tmp.Close(); err != nil { + return "", fmt.Errorf("close temp file: %w", err) + } + if err := os.Chmod(tmpPath, perm); err != nil { + return "", fmt.Errorf("set temp file permissions: %w", err) + } + if err := os.Rename(tmpPath, dst); err != nil { + return "", fmt.Errorf("install temp file: %w", err) + } + removeTmp = false + return hex.EncodeToString(digest.Sum(nil)), nil +} + +// InstallDownloadedTempFile installs a previously downloaded temp file at dst. +func InstallDownloadedTempFile(tmpPath, dst string, perm os.FileMode) error { + if strings.TrimSpace(tmpPath) == "" || strings.TrimSpace(dst) == "" { + return fmt.Errorf("temp and destination paths are required") + } + if err := os.MkdirAll(filepath.Dir(dst), 0o755); err != nil { + return fmt.Errorf("create destination directory: %w", err) + } + if err := os.Chmod(tmpPath, perm); err != nil { + return fmt.Errorf("set temp file permissions: %w", err) + } + if err := os.Rename(tmpPath, dst); err != nil { + return fmt.Errorf("install downloaded file: %w", err) + } + return nil +} diff --git a/internal/fileops/fileops_test.go b/internal/fileops/fileops_test.go new file mode 100644 index 0000000..07e1f6b --- /dev/null +++ b/internal/fileops/fileops_test.go @@ -0,0 +1,125 @@ +package fileops + +import ( + "os" + "path/filepath" + "strings" + "testing" +) + +func TestWriteFileAtomicOverwritesAndLeavesNoTempFile(t *testing.T) { + root := t.TempDir() + dst := filepath.Join(root, "out", "value.txt") + + if err := WriteFileAtomic(dst, []byte("one"), 0o644); err != nil { + t.Fatalf("WriteFileAtomic(first) error = %v", err) + } + if err := WriteFileAtomic(dst, []byte("two"), 0o644); err != nil { + t.Fatalf("WriteFileAtomic(second) error = %v", err) + } + + data, err := os.ReadFile(dst) + if err != nil { + t.Fatalf("ReadFile() error = %v", err) + } + if string(data) != "two" { + t.Fatalf("file content = %q, want %q", string(data), "two") + } + + assertNoMatchingTempFiles(t, filepath.Dir(dst), "."+filepath.Base(dst)+".tmp-") +} + +func TestWriteFileAtomicCleansTempFileOnInstallFailure(t *testing.T) { + root := t.TempDir() + blockedPath := filepath.Join(root, "blocked") + if err := os.MkdirAll(blockedPath, 0o755); err != nil { + t.Fatalf("MkdirAll(blockedPath) error = %v", err) + } + + err := WriteFileAtomic(blockedPath, []byte("data"), 0o644) + if err == nil { + t.Fatal("WriteFileAtomic() error = nil, want install failure") + } + assertNoMatchingTempFiles(t, root, ".blocked.tmp-") +} + +func TestCopyFileAtomicWithChecksumMatchesDestination(t *testing.T) { + root := t.TempDir() + src := filepath.Join(root, "source.txt") + dst := filepath.Join(root, "out", "copied.txt") + if err := os.WriteFile(src, []byte("copied-data"), 0o644); err != nil { + t.Fatalf("WriteFile(source) error = %v", err) + } + + checksum, err := CopyFileAtomicWithChecksum(src, dst, 0o644) + if err != nil { + t.Fatalf("CopyFileAtomicWithChecksum() error = %v", err) + } + wantChecksum := "6e5c3f239e28cc315d57b2fcfc24169369c44a25802c0616a6d7081707fd24df" + if checksum != wantChecksum { + t.Fatalf("checksum = %q, want %q", checksum, wantChecksum) + } + + data, err := os.ReadFile(dst) + if err != nil { + t.Fatalf("ReadFile(destination) error = %v", err) + } + if string(data) != "copied-data" { + t.Fatalf("destination content = %q, want %q", string(data), "copied-data") + } + assertNoMatchingTempFiles(t, filepath.Dir(dst), ".copied.txt.tmp-") +} + +func TestCopyFileAtomicCleansTempFileOnInstallFailure(t *testing.T) { + root := t.TempDir() + src := filepath.Join(root, "source.txt") + if err := os.WriteFile(src, []byte("copied-data"), 0o644); err != nil { + t.Fatalf("WriteFile(source) error = %v", err) + } + + blockedPath := filepath.Join(root, "blocked") + if err := os.MkdirAll(blockedPath, 0o755); err != nil { + t.Fatalf("MkdirAll(blockedPath) error = %v", err) + } + err := CopyFileAtomic(src, blockedPath, 0o644) + if err == nil { + t.Fatal("CopyFileAtomic() error = nil, want install failure") + } + assertNoMatchingTempFiles(t, root, ".blocked.tmp-") +} + +func TestInstallDownloadedTempFileSetsPermissions(t *testing.T) { + root := t.TempDir() + tmpPath := filepath.Join(root, ".payload.tmp") + dst := filepath.Join(root, "out", "payload.json") + if err := os.WriteFile(tmpPath, []byte("{\"ok\":true}\n"), 0o600); err != nil { + t.Fatalf("WriteFile(temp) error = %v", err) + } + + if err := InstallDownloadedTempFile(tmpPath, dst, 0o644); err != nil { + t.Fatalf("InstallDownloadedTempFile() error = %v", err) + } + if _, err := os.Stat(tmpPath); !os.IsNotExist(err) { + t.Fatalf("temp file still exists: stat err = %v", err) + } + info, err := os.Stat(dst) + if err != nil { + t.Fatalf("Stat(destination) error = %v", err) + } + if info.Mode().Perm() != 0o644 { + t.Fatalf("destination mode = %o, want 644", info.Mode().Perm()) + } +} + +func assertNoMatchingTempFiles(t *testing.T, dir, prefix string) { + t.Helper() + entries, err := os.ReadDir(dir) + if err != nil { + t.Fatalf("ReadDir(%q) error = %v", dir, err) + } + for _, e := range entries { + if strings.HasPrefix(e.Name(), prefix) { + t.Fatalf("unexpected temp file residue: %s", filepath.Join(dir, e.Name())) + } + } +} diff --git a/internal/pathsafe/relative_destination_test.go b/internal/pathsafe/relative_destination_test.go index 001d16f..4667aa4 100644 --- a/internal/pathsafe/relative_destination_test.go +++ b/internal/pathsafe/relative_destination_test.go @@ -2,6 +2,8 @@ package pathsafe import ( "errors" + "path/filepath" + "strings" "testing" ) @@ -41,3 +43,76 @@ func TestNormalizeRelativeDestination(t *testing.T) { }) } } + +func TestJoinSlashRelativeUnderRoot(t *testing.T) { + root := filepath.Join(t.TempDir(), "session") + + tests := []struct { + name string + input string + want string + wantErr error + }{ + {name: "valid relative", input: "artifacts/session_recap.md", want: filepath.Join(root, "artifacts", "session_recap.md")}, + {name: "windows separators normalized", input: `artifacts\session_recap.md`, want: filepath.Join(root, "artifacts", "session_recap.md")}, + {name: "reject empty", input: "", wantErr: ErrRelativePathRequired}, + {name: "reject absolute", input: "/artifacts/session_recap.md", wantErr: ErrRelativePathAbsolute}, + {name: "reject traversal", input: "../artifacts/session_recap.md", wantErr: ErrRelativePathEscape}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got, err := JoinSlashRelativeUnderRoot(root, tt.input) + if tt.wantErr != nil { + if !errors.Is(err, tt.wantErr) { + t.Fatalf("JoinSlashRelativeUnderRoot() error = %v, want %v", err, tt.wantErr) + } + return + } + if err != nil { + t.Fatalf("JoinSlashRelativeUnderRoot() error = %v", err) + } + if got != tt.want { + t.Fatalf("JoinSlashRelativeUnderRoot() = %q, want %q", got, tt.want) + } + }) + } +} + +func TestSlashRelativeFromRoot(t *testing.T) { + root := filepath.Join(t.TempDir(), "session") + target := filepath.Join(root, "transcripts", "full.json") + + got, err := SlashRelativeFromRoot(root, target) + if err != nil { + t.Fatalf("SlashRelativeFromRoot() error = %v", err) + } + if got != "transcripts/full.json" { + t.Fatalf("SlashRelativeFromRoot() = %q, want transcripts/full.json", got) + } + + got, err = SlashRelativeFromRoot(root, `transcripts\full.json`) + if err != nil { + t.Fatalf("SlashRelativeFromRoot(relative with windows separators) error = %v", err) + } + if got != "transcripts/full.json" { + t.Fatalf("SlashRelativeFromRoot(relative with windows separators) = %q, want transcripts/full.json", got) + } +} + +func TestSlashRelativeFromRootRejectsOutsideRoot(t *testing.T) { + root := filepath.Join(t.TempDir(), "session") + outside := filepath.Join(filepath.Dir(root), "outside", "file.txt") + + _, err := SlashRelativeFromRoot(root, outside) + if !errors.Is(err, ErrRelativePathEscape) { + t.Fatalf("SlashRelativeFromRoot() error = %v, want %v", err, ErrRelativePathEscape) + } +} + +func TestJoinSlashRelativeUnderRootRequiresRoot(t *testing.T) { + _, err := JoinSlashRelativeUnderRoot("", "artifacts/session_recap.md") + if err == nil || !strings.Contains(err.Error(), "root path is required") { + t.Fatalf("JoinSlashRelativeUnderRoot() error = %v, want root-required error", err) + } +} diff --git a/internal/pathsafe/root_scoped.go b/internal/pathsafe/root_scoped.go new file mode 100644 index 0000000..764e02e --- /dev/null +++ b/internal/pathsafe/root_scoped.go @@ -0,0 +1,58 @@ +package pathsafe + +import ( + "fmt" + "path/filepath" + "strings" +) + +// JoinSlashRelativeUnderRoot validates a slash-style relative path and resolves +// it under root. The returned path uses the host filepath separator. +func JoinSlashRelativeUnderRoot(root, relative string) (string, error) { + rootClean := filepath.Clean(strings.TrimSpace(root)) + if rootClean == "." || rootClean == "" { + return "", fmt.Errorf("root path is required") + } + + normalized, err := NormalizeRelativeDestination(relative) + if err != nil { + return "", err + } + + joined := filepath.Clean(filepath.Join(rootClean, filepath.FromSlash(normalized))) + rel, err := filepath.Rel(rootClean, joined) + if err != nil { + return "", fmt.Errorf("resolve relative path under root: %w", err) + } + if rel == "." || rel == ".." || strings.HasPrefix(rel, ".."+string(filepath.Separator)) { + return "", ErrRelativePathEscape + } + return joined, nil +} + +// SlashRelativeFromRoot derives a slash-style relative path for target under +// root. Target may be absolute or relative to root. +func SlashRelativeFromRoot(root, target string) (string, error) { + rootClean := filepath.Clean(strings.TrimSpace(root)) + if rootClean == "." || rootClean == "" { + return "", fmt.Errorf("root path is required") + } + + targetClean := filepath.Clean(strings.TrimSpace(target)) + if targetClean == "." || targetClean == "" { + return "", ErrRelativePathRequired + } + if !filepath.IsAbs(targetClean) { + targetClean = filepath.Clean(filepath.Join(rootClean, targetClean)) + } + + rel, err := filepath.Rel(rootClean, targetClean) + if err != nil { + return "", fmt.Errorf("derive path relative to root: %w", err) + } + normalized, err := NormalizeRelativeDestination(filepath.ToSlash(rel)) + if err != nil { + return "", err + } + return normalized, nil +} diff --git a/internal/previouscache/previouscache.go b/internal/previouscache/previouscache.go index bd0549a..3c1f3c5 100644 --- a/internal/previouscache/previouscache.go +++ b/internal/previouscache/previouscache.go @@ -309,11 +309,7 @@ func deriveManifestRelativePath(previousManifest *manifest.Manifest, localPath s if !ok { return "", false } - rel, err := filepath.Rel(sessionRoot, trimmed) - if err != nil { - return "", false - } - normalized, err := pathsafe.NormalizeRelativeDestination(filepath.ToSlash(rel)) + normalized, err := pathsafe.SlashRelativeFromRoot(sessionRoot, trimmed) if err != nil { return "", false } @@ -375,11 +371,7 @@ func relativeToSession(paths artifacts.SessionPaths, localPath string) (string, if strings.TrimSpace(root) == "" { return "", fmt.Errorf("session root is required") } - rel, err := filepath.Rel(root, filepath.Clean(localPath)) - if err != nil { - return "", fmt.Errorf("resolve previous-cache relative path: %w", err) - } - normalized, err := pathsafe.NormalizeRelativeDestination(filepath.ToSlash(rel)) + normalized, err := pathsafe.SlashRelativeFromRoot(root, localPath) if err != nil { return "", fmt.Errorf("resolve previous-cache relative path: %w", err) } diff --git a/internal/stage/prepare_previous.go b/internal/stage/prepare_previous.go index e6ed6b9..5f948f0 100644 --- a/internal/stage/prepare_previous.go +++ b/internal/stage/prepare_previous.go @@ -8,6 +8,7 @@ import ( "sort" "gitea.maximumdirect.net/eric/narratio/internal/artifacts" + "gitea.maximumdirect.net/eric/narratio/internal/fileops" "gitea.maximumdirect.net/eric/narratio/internal/manifest" "gitea.maximumdirect.net/eric/narratio/internal/previouscache" ) @@ -54,8 +55,35 @@ func hydratePreviousSessionArtifacts( if err := os.MkdirAll(filepath.Dir(record.LocalPath), 0o755); err != nil { return nil, fmt.Errorf("create previous-session path directory for %q: %w", record.LocalPath, err) } - if err := env.ObjectStore.Download(ctx, record.RemoteKey, record.LocalPath); err != nil { - return nil, fmt.Errorf("download previous-session object %q to %q: %w", record.RemoteKey, record.LocalPath, err) + + base := filepath.Base(record.LocalPath) + tmp, err := os.CreateTemp(filepath.Dir(record.LocalPath), "."+base+".prepare-previous-*.tmp") + if err != nil { + return nil, fmt.Errorf("create previous-session temp file for %q: %w", record.LocalPath, err) + } + tmpPath := tmp.Name() + if err := tmp.Close(); err != nil { + _ = os.Remove(tmpPath) + return nil, fmt.Errorf("close previous-session temp file for %q: %w", record.LocalPath, err) + } + if err := func() error { + removeTmp := true + defer func() { + if removeTmp { + _ = os.Remove(tmpPath) + } + }() + + if err := env.ObjectStore.Download(ctx, record.RemoteKey, tmpPath); err != nil { + return fmt.Errorf("download previous-session object %q to temp file: %w", record.RemoteKey, err) + } + if err := fileops.InstallDownloadedTempFile(tmpPath, record.LocalPath, 0o644); err != nil { + return fmt.Errorf("install previous-session object %q at %q: %w", record.RemoteKey, record.LocalPath, err) + } + removeTmp = false + return nil + }(); err != nil { + return nil, err } if record.Kind == preparePreviousInputKindArtifact { if err := requireNonEmptyFile(record.LocalPath, "previous-session artifact "+record.RequirementName); err != nil { diff --git a/internal/stage/run_local.go b/internal/stage/run_local.go index f55821d..1de8d60 100644 --- a/internal/stage/run_local.go +++ b/internal/stage/run_local.go @@ -9,6 +9,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 runStageLayout struct { @@ -92,18 +93,17 @@ func runLocalPathForCanonical(layout runStageLayout, sessionPaths artifacts.Sess if cleanCanonical == "" { return "", fmt.Errorf("canonical path is required") } - rel, err := filepath.Rel(filepath.Clean(sessionPaths.Root), cleanCanonical) + rel, err := pathsafe.SlashRelativeFromRoot(sessionPaths.Root, cleanCanonical) if err != nil { return "", fmt.Errorf("derive session-relative path for %q: %w", cleanCanonical, err) } - rel = filepath.Clean(rel) - if rel == "." || rel == ".." || strings.HasPrefix(rel, ".."+string(filepath.Separator)) { - return "", fmt.Errorf("canonical path %q is outside session root %q", cleanCanonical, sessionPaths.Root) - } - if rel == config.PathPreviousDirSegment || strings.HasPrefix(rel, config.PathPreviousDirSegment+string(filepath.Separator)) { + if rel == config.PathPreviousDirSegment || strings.HasPrefix(rel, config.PathPreviousDirSegment+"/") { return cleanCanonical, nil } - localPath := filepath.Join(layout.OutputsDir, rel) + localPath, err := pathsafe.JoinSlashRelativeUnderRoot(layout.OutputsDir, rel) + if err != nil { + return "", fmt.Errorf("resolve run-local output path for %q: %w", cleanCanonical, err) + } if err := os.MkdirAll(filepath.Dir(localPath), 0o755); err != nil { return "", fmt.Errorf("create run-local output parent for %q: %w", localPath, err) }