From 97b0a6e7f15cb094d4d2e87ef000e3579489a60c Mon Sep 17 00:00:00 2001 From: Paul Buetow Date: Wed, 20 May 2026 14:11:35 +0300 Subject: Validate AbsPath in streamer and clean up partial podcast downloads (l9+k9) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- player-server/cmd/player/main.go | 2 +- .../internal/api/handlers_playback_test.go | 2 +- .../internal/api/handlers_podcast_test.go | 2 +- player-server/internal/api/handlers_share_test.go | 4 +- player-server/internal/api/handlers_test.go | 2 +- player-server/internal/api/integration_test.go | 2 +- player-server/internal/api/server_test.go | 4 +- player-server/internal/service/podcast_episode.go | 25 ++++++++++- player-server/internal/service/streamer.go | 25 +++++++++-- player-server/internal/service/streamer_test.go | 49 ++++++++++++++++++---- 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) + } +} -- cgit v1.2.3