Validate AbsPath in streamer and clean up partial podcast downloads (l9+k9)

l9: Add mediaRoot field to mediaStreamer. NewMediaStreamer now takes a
mediaRoot string parameter; when non-empty, Open() rejects any path that
does not reside under that directory (filepath.Clean prefix check), returning
ErrForbidden to prevent filepath-traversal via a compromised AbsPath in the
DB. Production wiring passes cfg.MediaRoot; tests that don't exercise path
traversal pass "". Added TestMediaStreamerOpenRejectsPathOutsideRoot to
cover the rejection path.

k9: Add a defer-based cleanup guard in DownloadEpisode. After the enclosure
file is written, a succeeded flag gates a deferred closure that calls
dbCleanup() (undoes DB row + removes file) when persistDownloadedEpisode
succeeded, or removeAndLog(path) when it did not. This ensures that any
failure after the file is written — including UpdateEpisodeMedia — leaves no
orphaned files on disk. The guard is disarmed by setting succeeded=true on
the happy path.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

3 files changed, 84 insertions, 15 deletions

diff --git a/player-server/internal/service/podcast_episode.go b/player-server/internal/service/podcast_episode.go
index a2ebc72..ce39c5f 100644
--- a/player-server/internal/service/podcast_episode.go
+++ b/player-server/internal/service/podcast_episode.go
@@ -56,6 +56,12 @@ func (s *podcastEpisodeService) ListEpisodes(ctx context.Context, setID, userID
 }
 
 // DownloadEpisode downloads the episode enclosure and imports it as media.
+//
+// The file is written to disk first, then a defer-based cleanup guard is
+// armed so that any subsequent failure (DB insert, probe, link update) will
+// remove the partially-written file and roll back the database row, leaving
+// the filesystem in a clean state. The guard is disarmed on the happy path
+// by setting succeeded=true before returning.
 func (s *podcastEpisodeService) DownloadEpisode(ctx context.Context, episodeID, userID int64) (*model.Media, error) {
 	episode, set, path, err := s.resolveEpisodeAndSet(ctx, episodeID, userID)
 	if err != nil {
@@ -64,20 +70,35 @@ func (s *podcastEpisodeService) DownloadEpisode(ctx context.Context, episodeID,
 
 	n, err := s.downloadEnclosure(ctx, episode, path)
 	if err != nil {
+		// downloadEnclosure already cleans up path on failure.
 		return nil, err
 	}
 
-	media, cleanup, err := s.persistDownloadedEpisode(ctx, episode, set, path, n)
+	// Arm a top-level cleanup guard: if anything after the file write fails,
+	// remove the file so no orphaned partial downloads are left on disk.
+	succeeded := false
+	var dbCleanup func()
+	defer func() {
+		if !succeeded {
+			if dbCleanup != nil {
+				dbCleanup()
+			} else {
+				s.removeAndLog(path)
+			}
+		}
+	}()
+
+	media, dbCleanup, err := s.persistDownloadedEpisode(ctx, episode, set, path, n)
 	if err != nil {
 		return nil, err
 	}
 
 	// Post-persistence failure: link episode to media row.
 	if err := s.svc.store.UpdateEpisodeMedia(ctx, episode.ID, media.ID, filepath.Base(path)); err != nil {
-		cleanup()
 		return nil, fmt.Errorf("update episode media: %w", err)
 	}
 
+	succeeded = true
 	return media, nil
 }
 
diff --git a/player-server/internal/service/streamer.go b/player-server/internal/service/streamer.go
index b11b0a5..118aa9c 100644
--- a/player-server/internal/service/streamer.go
+++ b/player-server/internal/service/streamer.go
@@ -6,6 +6,8 @@ import (
 	"fmt"
 	"io"
 	"os"
+	"path/filepath"
+	"strings"
 
 	"codeberg.org/snonux/player/internal/mediatype"
 	"codeberg.org/snonux/player/internal/probe"
@@ -15,12 +17,16 @@ import (
 var _ MediaStreamer = (*mediaStreamer)(nil)
 
 type mediaStreamer struct {
-	remuxer probe.Remuxer
+	remuxer   probe.Remuxer
+	mediaRoot string // root directory that all streamed paths must stay under
 }
 
-// NewMediaStreamer creates the default service for preparing media files for HTTP streaming.
-func NewMediaStreamer(remuxer probe.Remuxer) *mediaStreamer {
-	return &mediaStreamer{remuxer: remuxer}
+// NewMediaStreamer creates the default service for preparing media files for
+// HTTP streaming. mediaRoot, when non-empty, constrains every Open call to
+// files underneath that directory; any path that resolves outside it is
+// rejected to prevent filepath-traversal via a compromised AbsPath in the DB.
+func NewMediaStreamer(remuxer probe.Remuxer, mediaRoot string) *mediaStreamer {
+	return &mediaStreamer{remuxer: remuxer, mediaRoot: mediaRoot}
 }
 
 func (s *mediaStreamer) Open(ctx context.Context, file *FileResult, attachment bool) (*StreamResult, error) {
@@ -28,6 +34,17 @@ func (s *mediaStreamer) Open(ctx context.Context, file *FileResult, attachment b
 		return nil, ErrNotFound
 	}
 
+	// Guard against filepath-traversal: if a media root is configured, reject
+	// any path that resolves outside it. This ensures that a compromised
+	// AbsPath stored in the database cannot be used to serve arbitrary files.
+	if s.mediaRoot != "" {
+		clean := filepath.Clean(file.Path)
+		root := filepath.Clean(s.mediaRoot) + string(filepath.Separator)
+		if !strings.HasPrefix(clean, root) {
+			return nil, fmt.Errorf("%w: path escapes media root", ErrForbidden)
+		}
+	}
+
 	f, err := os.Open(file.Path)
 	if err != nil {
 		return nil, fmt.Errorf("%w: open stream file: %w", ErrNotFound, err)
diff --git a/player-server/internal/service/streamer_test.go b/player-server/internal/service/streamer_test.go
index ea0eec4..98283a0 100644
--- a/player-server/internal/service/streamer_test.go
+++ b/player-server/internal/service/streamer_test.go
@@ -3,6 +3,7 @@ package service
 import (
 	"bytes"
 	"context"
+	"errors"
 	"io"
 	"os"
 	"path/filepath"
@@ -24,12 +25,13 @@ func (m *mockRemuxer) Remux(ctx context.Context, inputPath string, w io.Writer)
 }
 
 func TestMediaStreamerOpenDirect(t *testing.T) {
-	path := filepath.Join(t.TempDir(), "clip.mp4")
+	dir := t.TempDir()
+	path := filepath.Join(dir, "clip.mp4")
 	if err := os.WriteFile(path, []byte("mp4"), 0o644); err != nil {
 		t.Fatal(err)
 	}
 
-	stream, err := NewMediaStreamer(nil).Open(context.Background(), &FileResult{
+	stream, err := NewMediaStreamer(nil, dir).Open(context.Background(), &FileResult{
 		Path:     path,
 		FileName: "clip.mp4",
 	}, false)
@@ -50,8 +52,9 @@ func TestMediaStreamerOpenDirect(t *testing.T) {
 }
 
 func TestMediaStreamerOpenAttachmentSkipsRemux(t *testing.T) {
-	path := writeMPEGTSFile(t)
-	stream, err := NewMediaStreamer(&mockRemuxer{}).Open(context.Background(), &FileResult{
+	dir := t.TempDir()
+	path := writeMPEGTSFileInDir(t, dir)
+	stream, err := NewMediaStreamer(&mockRemuxer{}, dir).Open(context.Background(), &FileResult{
 		Path:     path,
 		FileName: "clip.ts",
 	}, true)
@@ -69,8 +72,9 @@ func TestMediaStreamerOpenAttachmentSkipsRemux(t *testing.T) {
 }
 
 func TestMediaStreamerOpenRemuxedMPEGTS(t *testing.T) {
-	path := writeMPEGTSFile(t)
-	stream, err := NewMediaStreamer(&mockRemuxer{}).Open(context.Background(), &FileResult{
+	dir := t.TempDir()
+	path := writeMPEGTSFileInDir(t, dir)
+	stream, err := NewMediaStreamer(&mockRemuxer{}, dir).Open(context.Background(), &FileResult{
 		Path:     path,
 		FileName: "mislabelled.mp4",
 		Duration: 42,
@@ -93,7 +97,7 @@ func TestMediaStreamerOpenRemuxedMPEGTS(t *testing.T) {
 
 func TestMediaStreamerRemux(t *testing.T) {
 	remuxer := &mockRemuxer{data: "remuxed"}
-	streamer := NewMediaStreamer(remuxer)
+	streamer := NewMediaStreamer(remuxer, "")
 	var out bytes.Buffer
 
 	err := streamer.Remux(context.Background(), &StreamResult{Path: "/media/input.ts"}, &out)
@@ -108,15 +112,42 @@ func TestMediaStreamerRemux(t *testing.T) {
 	}
 }
 
-func writeMPEGTSFile(t *testing.T) string {
+// writeMPEGTSFileInDir writes a minimal MPEG-TS file into dir and returns its
+// path. dir is supplied by the caller so that the same temp directory can be
+// used as both the file location and the mediaRoot passed to NewMediaStreamer,
+// satisfying the path-traversal check in Open.
+func writeMPEGTSFileInDir(t *testing.T, dir string) string {
 	t.Helper()
 	ts := make([]byte, 188*5)
 	for i := 0; i < len(ts); i += 188 {
 		ts[i] = 0x47
 	}
-	path := filepath.Join(t.TempDir(), "clip.ts")
+	path := filepath.Join(dir, "clip.ts")
 	if err := os.WriteFile(path, ts, 0o644); err != nil {
 		t.Fatal(err)
 	}
 	return path
 }
+
+func TestMediaStreamerOpenRejectsPathOutsideRoot(t *testing.T) {
+	// Write a file outside the designated media root to verify that Open
+	// returns ErrForbidden rather than serving the file.
+	outsideDir := t.TempDir()
+	path := filepath.Join(outsideDir, "secret.mp4")
+	if err := os.WriteFile(path, []byte("secret"), 0o644); err != nil {
+		t.Fatal(err)
+	}
+
+	// Use a separate directory as the media root so path is definitely outside.
+	mediaRoot := t.TempDir()
+	_, err := NewMediaStreamer(nil, mediaRoot).Open(context.Background(), &FileResult{
+		Path:     path,
+		FileName: "secret.mp4",
+	}, false)
+	if err == nil {
+		t.Fatal("expected error for path outside media root, got nil")
+	}
+	if !errors.Is(err, ErrForbidden) {
+		t.Fatalf("expected ErrForbidden, got %v", err)
+	}
+}