Centralize path-safe root joins and atomic file operations

2026-05-23 15:45:31 +00:00
parent 98649f4d81
commit 094b0d2532
11 changed files with 466 additions and 189 deletions
--- 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
--- 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(
--- 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
 }
--- 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
-}
--- a/internal/fileops/fileops.go
+++ 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
+}
--- a/internal/fileops/fileops_test.go
+++ 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()))
+		}
+	}
+}
--- 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)
+	}
+}
--- a/internal/pathsafe/root_scoped.go
+++ 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
+}
--- 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)
 	}
--- 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 {
--- 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)
 	}