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>

10 files changed, 93 insertions, 24 deletions

diff --git a/player-server/cmd/player/main.go b/player-server/cmd/player/main.go
index 3e8046f..d69f3a9 100644
--- a/player-server/cmd/player/main.go
+++ b/player-server/cmd/player/main.go
@@ -253,7 +253,7 @@ func runWithSignal(args []string, sigCh <-chan os.Signal) error {
 
 	staticFS := http.Dir("web")
 	remuxer := probe.NewFFRemuxer()
-	streamer := service.NewMediaStreamer(remuxer)
+	streamer := service.NewMediaStreamer(remuxer, cfg.MediaRoot)
 	server, err := api.NewServerWithLogger(api.ServerDeps{
 		Store:          store,
 		Hasher:         deps.hasher,
diff --git a/player-server/internal/api/handlers_playback_test.go b/player-server/internal/api/handlers_playback_test.go
index 869b0d1..fcf0f80 100644
--- a/player-server/internal/api/handlers_playback_test.go
+++ b/player-server/internal/api/handlers_playback_test.go
@@ -39,7 +39,7 @@ func newPlaybackTestServer(t *testing.T, store repository.Store, sm auth.Session
 			PlaybackHints: hintSvc,
 		},
 		StaticFS:      fs,
-		MediaStreamer: service.NewMediaStreamer(nil),
+		MediaStreamer: service.NewMediaStreamer(nil, ""),
 	})
 	if err != nil {
 		t.Fatalf("NewServer: %v", err)
diff --git a/player-server/internal/api/handlers_podcast_test.go b/player-server/internal/api/handlers_podcast_test.go
index 7dcdfe6..73e76e2 100644
--- a/player-server/internal/api/handlers_podcast_test.go
+++ b/player-server/internal/api/handlers_podcast_test.go
@@ -69,7 +69,7 @@ func newPodcastTestServer(t *testing.T, store repository.Store, hasher auth.Hash
 			Podcast:  podcastSvc,
 		},
 		StaticFS:      fs,
-		MediaStreamer: service.NewMediaStreamer(nil),
+		MediaStreamer: service.NewMediaStreamer(nil, ""),
 	})
 	if err != nil {
 		t.Fatalf("NewServer: %v", err)
diff --git a/player-server/internal/api/handlers_share_test.go b/player-server/internal/api/handlers_share_test.go
index 58d3257..021e023 100644
--- a/player-server/internal/api/handlers_share_test.go
+++ b/player-server/internal/api/handlers_share_test.go
@@ -65,7 +65,7 @@ func TestCreateShare_UsesInjectedClock(t *testing.T) {
 			Auth:     authSvc,
 		},
 		StaticFS:      newTestFS(map[string]string{"index.html": "x"}),
-		MediaStreamer: service.NewMediaStreamer(nil),
+		MediaStreamer: service.NewMediaStreamer(nil, ""),
 		Clock:         mockClk,
 	})
 	if err != nil {
@@ -119,7 +119,7 @@ func TestSetSessionCookie_UsesInjectedClock(t *testing.T) {
 			Auth: authSvc,
 		},
 		StaticFS:      newTestFS(map[string]string{"index.html": "x"}),
-		MediaStreamer: service.NewMediaStreamer(nil),
+		MediaStreamer: service.NewMediaStreamer(nil, ""),
 		Clock:         mockClk,
 	})
 	if err != nil {
diff --git a/player-server/internal/api/handlers_test.go b/player-server/internal/api/handlers_test.go
index e031f40..4f04f98 100644
--- a/player-server/internal/api/handlers_test.go
+++ b/player-server/internal/api/handlers_test.go
@@ -71,7 +71,7 @@ func newTestServer(t *testing.T, store repository.Store, hasher auth.Hasher, sm
 		// still need one, so we fall back to the default in-process streamer
 		// (nil remuxer means remux requests will error, which is fine for
 		// non-streaming tests).
-		mediaStreamer = service.NewMediaStreamer(nil)
+		mediaStreamer = service.NewMediaStreamer(nil, "")
 	}
 	// NewServer now returns an error when required deps (e.g. Config,
 	// MediaStreamer) are missing. Tests always pass non-nil values, so a
diff --git a/player-server/internal/api/integration_test.go b/player-server/internal/api/integration_test.go
index 9344552..08c0d86 100644
--- a/player-server/internal/api/integration_test.go
+++ b/player-server/internal/api/integration_test.go
@@ -83,7 +83,7 @@ func newIntegrationServer(t *testing.T) *integrationEnv {
 			Podcast:  &integrationPodcastService{},
 		},
 		StaticFS:      http.FS(staticFS),
-		MediaStreamer: service.NewMediaStreamer(nil),
+		MediaStreamer: service.NewMediaStreamer(nil, ""),
 	})
 	if err != nil {
 		t.Fatalf("NewServer: %v", err)
diff --git a/player-server/internal/api/server_test.go b/player-server/internal/api/server_test.go
index 89bc6c6..35a5a4b 100644
--- a/player-server/internal/api/server_test.go
+++ b/player-server/internal/api/server_test.go
@@ -48,7 +48,7 @@ func TestNewServerWithLogger_ErrorsOnNilMediaStreamer(t *testing.T) {
 func TestNewServerWithLogger_SucceedsWithMediaStreamer(t *testing.T) {
 	srv, err := NewServerWithLogger(ServerDeps{
 		Config:        &internal.Config{},
-		MediaStreamer: service.NewMediaStreamer(nil),
+		MediaStreamer: service.NewMediaStreamer(nil, ""),
 	}, slog.Default())
 	if err != nil {
 		t.Fatalf("unexpected error: %v", err)
@@ -67,7 +67,7 @@ func TestNewServerWithLogger_SucceedsWithMediaStreamer(t *testing.T) {
 func TestServer_PublicRouteRegistry(t *testing.T) {
 	srv, err := NewServerWithLogger(ServerDeps{
 		Config:        &internal.Config{},
-		MediaStreamer: service.NewMediaStreamer(nil),
+		MediaStreamer: service.NewMediaStreamer(nil, ""),
 	}, slog.Default())
 	if err != nil {
 		t.Fatalf("unexpected error: %v", err)
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)
+	}
+}