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/internal/service/podcast_episode.go | 25 +++++++++++- player-server/internal/service/streamer.go | 25 ++++++++++-- player-server/internal/service/streamer_test.go | 49 ++++++++++++++++++----- 3 files changed, 84 insertions(+), 15 deletions(-) (limited to 'player-server/internal/service') 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