From fa138afea576e14c13e7c8150d195fddce15a954 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Deluan=20Quint=C3=A3o?= Date: Tue, 23 Jun 2026 18:53:51 -0400 Subject: [PATCH] fix(playlist/share): apply user library access to import and sharing paths (#5640) * fix(playlist): respect the user's library access when resolving M3U paths FindByPaths looked up paths across all libraries, so importing an M3U could add tracks from libraries the importing user has no access to. Apply the user's library filter to the lookup, matching every other media_file read. Admins and the (admin-context) scanner are unaffected. * fix(share): scope shared playlist tracks to the owner's libraries loadMedia loaded playlist tracks with a fake-admin context, so a shared playlist could include tracks from libraries the owner has no access to. Load as the share owner instead, so the library filter applies. Admin-owned shares are unchanged. * fix(share): only serve shared tracks the owner can access A shared stream fetched the media file by id without checking the share owner's library access, so it could serve tracks from libraries the owner has no access to. Gate share-scoped streams on the owner's library access. Non-share streams are unaffected. * test(share): tidy library-access test setup Consolidate the repeated share-owner test fixture in handleStream into a helper, assert on track fields with HaveField instead of building an id slice, and delete the scratch media file through the public repository method. * style: trim verbose comments in library-access checks * fix(share): guard against nil owner and clean up test users Add a nil check after loading the share owner so a missing user yields a clear error instead of a possible nil dereference, and delete the users created by the new tests in their AfterEach blocks. * fix(share): avoid panic when a shared playlist is no longer visible to its owner Tracks() returns nil when the playlist can't be loaded under the owner's context (e.g. a public playlist shared by a non-owner that was later made private). Capture the result and return early instead of chaining GetAll on a nil repository, leaving the share with no tracks. --- persistence/mediafile_repository.go | 2 +- persistence/mediafile_repository_test.go | 52 +++++++++++++ persistence/share_repository.go | 24 ++++-- persistence/share_repository_test.go | 96 ++++++++++++++++++++++++ server/public/handle_streams.go | 13 ++++ server/public/handle_streams_test.go | 42 ++++++++++- 6 files changed, 220 insertions(+), 9 deletions(-) diff --git a/persistence/mediafile_repository.go b/persistence/mediafile_repository.go index dd8145eae..740ca4956 100644 --- a/persistence/mediafile_repository.go +++ b/persistence/mediafile_repository.go @@ -306,7 +306,7 @@ func (r *mediaFileRepository) FindByPaths(paths []string) (model.MediaFiles, err return model.MediaFiles{}, nil } - sel := r.newSelect().Columns("*").Where(query) + sel := r.applyLibraryFilter(r.newSelect().Columns("*").Where(query)) var res dbMediaFiles if err := r.queryAll(sel, &res); err != nil { return nil, err diff --git a/persistence/mediafile_repository_test.go b/persistence/mediafile_repository_test.go index 7989dc25a..d99bb65a7 100644 --- a/persistence/mediafile_repository_test.go +++ b/persistence/mediafile_repository_test.go @@ -880,6 +880,58 @@ var _ = Describe("MediaRepository", func() { Expect(err).ToNot(HaveOccurred()) Expect(results).To(BeEmpty()) }) + + Context("when the user has restricted library access", func() { + var otherLib model.Library + var restrictedUser model.User + + BeforeEach(func() { + adminCtx := request.WithUser(GinkgoT().Context(), adminUser) + lr := NewLibraryRepository(adminCtx, GetDBXBuilder()) + + // A second library the restricted user has no access to + otherLib = model.Library{ID: 0, Name: "Other Library", Path: "/other/lib"} + Expect(lr.Put(&otherLib)).To(Succeed()) + + // A track that lives only in the other library (created as admin) + adminMr := NewMediaFileRepository(adminCtx, GetDBXBuilder()) + Expect(adminMr.Put(&model.MediaFile{ + ID: "otherlib-track", LibraryID: otherLib.ID, + Path: "hidden/test.mp3", Title: "Hidden", + })).To(Succeed()) + + // Non-admin user with access to library 1 ONLY + restrictedUser = createUserWithLibraries("restricted-finder", []int{1}) + ur := NewUserRepository(adminCtx, GetDBXBuilder()) + Expect(ur.Put(&restrictedUser)).To(Succeed()) + Expect(ur.SetUserLibraries(restrictedUser.ID, []int{1})).To(Succeed()) + }) + + AfterEach(func() { + adminCtx := request.WithUser(GinkgoT().Context(), adminUser) + _ = NewMediaFileRepository(adminCtx, GetDBXBuilder()).Delete("otherlib-track") + lr := NewLibraryRepository(adminCtx, GetDBXBuilder()).(*libraryRepository) + _ = lr.delete(squirrel.Eq{"id": otherLib.ID}) + _ = NewUserRepository(adminCtx, GetDBXBuilder()).Delete(restrictedUser.ID) + }) + + It("does not resolve paths in libraries the user cannot access", func() { + userMr := NewMediaFileRepository(request.WithUser(GinkgoT().Context(), restrictedUser), GetDBXBuilder()) + qualified := fmt.Sprintf("%d:hidden/test.mp3", otherLib.ID) + results, err := userMr.FindByPaths([]string{qualified}) + Expect(err).ToNot(HaveOccurred()) + Expect(results).To(BeEmpty(), "a track outside the user's libraries must not be resolvable") + }) + + It("still resolves the path for an admin", func() { + adminMr := NewMediaFileRepository(request.WithUser(GinkgoT().Context(), adminUser), GetDBXBuilder()) + qualified := fmt.Sprintf("%d:hidden/test.mp3", otherLib.ID) + results, err := adminMr.FindByPaths([]string{qualified}) + Expect(err).ToNot(HaveOccurred()) + Expect(results).To(HaveLen(1)) + Expect(results[0].ID).To(Equal("otherlib-track")) + }) + }) }) Describe("wrapMediaFileCursor", func() { diff --git a/persistence/share_repository.go b/persistence/share_repository.go index 89dc19e19..0013e782b 100644 --- a/persistence/share_repository.go +++ b/persistence/share_repository.go @@ -100,16 +100,28 @@ func (r *shareRepository) loadMedia(share *model.Share) error { share.Tracks, err = mfRepo.GetAll(model.QueryOptions{Filters: noMissing(Eq{"album_id": ids}), Sort: "album"}) return err case "playlist": - // Create a context with a fake admin user, to be able to access all playlists - ctx := request.WithUser(r.ctx, model.User{IsAdmin: true}) + // Load tracks as the share owner so their library access is applied. + owner, err := NewUserRepository(r.ctx, r.db).Get(share.UserID) + if err != nil { + return fmt.Errorf("loading share owner %q: %w", share.UserID, err) + } + if owner == nil { + return fmt.Errorf("share owner %q not found", share.UserID) + } + ctx := request.WithUser(r.ctx, *owner) plsRepo := NewPlaylistRepository(ctx, r.db) - tracks, err := plsRepo.Tracks(ids[0], true).GetAll(model.QueryOptions{Sort: "id", Filters: noMissing(Eq{})}) + // Tracks returns nil when the playlist is no longer visible to the owner + // (e.g. it was made private after the share was created); leave the share + // with no tracks rather than exposing it. + trackRepo := plsRepo.Tracks(ids[0], true) + if trackRepo == nil { + return nil + } + tracks, err := trackRepo.GetAll(model.QueryOptions{Sort: "id", Filters: noMissing(Eq{})}) if err != nil { return err } - if len(tracks) >= 0 { - share.Tracks = tracks.MediaFiles() - } + share.Tracks = tracks.MediaFiles() return nil case "media_file": mfRepo := NewMediaFileRepository(r.ctx, r.db) diff --git a/persistence/share_repository_test.go b/persistence/share_repository_test.go index 0b3ece598..3ae456031 100644 --- a/persistence/share_repository_test.go +++ b/persistence/share_repository_test.go @@ -4,6 +4,7 @@ import ( "context" "time" + "github.com/Masterminds/squirrel" "github.com/deluan/rest" "github.com/navidrome/navidrome/conf/configtest" "github.com/navidrome/navidrome/log" @@ -132,6 +133,101 @@ var _ = Describe("ShareRepository", func() { }) }) + Describe("Playlist share library scoping", func() { + var otherLib model.Library + var owner model.User + var plsID string + + BeforeEach(func() { + adminCtx := request.WithUser(log.NewContext(GinkgoT().Context()), adminUser) + + // A second library the owner has no access to, plus a track in it + lr := NewLibraryRepository(adminCtx, GetDBXBuilder()) + otherLib = model.Library{ID: 0, Name: "Share Other Library", Path: "/share/other/lib"} + Expect(lr.Put(&otherLib)).To(Succeed()) + mr := NewMediaFileRepository(adminCtx, GetDBXBuilder()) + Expect(mr.Put(&model.MediaFile{ID: "share-other", LibraryID: otherLib.ID, Path: "s/other.mp3", Title: "ShareOther"})).To(Succeed()) + Expect(mr.Put(&model.MediaFile{ID: "share-ok", LibraryID: 1, Path: "s/ok.mp3", Title: "ShareOK"})).To(Succeed()) + + // Non-admin owner with access to library 1 only + owner = createUserWithLibraries("share-owner", []int{1}) + ur := NewUserRepository(adminCtx, GetDBXBuilder()) + Expect(ur.Put(&owner)).To(Succeed()) + Expect(ur.SetUserLibraries(owner.ID, []int{1})).To(Succeed()) + + // Owner-owned playlist containing tracks from both libraries + plsID = "share-scope-pls" + ownerCtx := request.WithUser(log.NewContext(GinkgoT().Context()), owner) + pr := NewPlaylistRepository(ownerCtx, GetDBXBuilder()) + pls := &model.Playlist{ID: plsID, Name: "Scope Test", OwnerID: owner.ID} + pls.AddMediaFiles(model.MediaFiles{{ID: "share-ok"}, {ID: "share-other"}}) + Expect(pr.Put(pls)).To(Succeed()) + + // Share row owned by the non-admin owner + _, err := GetDBXBuilder().NewQuery(` + INSERT INTO share (id, user_id, description, resource_type, resource_ids, created_at, updated_at) + VALUES ({:id}, {:user}, {:desc}, {:type}, {:ids}, {:created}, {:updated}) + `).Bind(map[string]any{ + "id": "share-scope", "user": owner.ID, "desc": "Scope test share", + "type": "playlist", "ids": plsID, "created": time.Now(), "updated": time.Now(), + }).Execute() + Expect(err).ToNot(HaveOccurred()) + }) + + AfterEach(func() { + adminCtx := request.WithUser(log.NewContext(GinkgoT().Context()), adminUser) + b := GetDBXBuilder() + _, _ = b.NewQuery(`DELETE FROM share WHERE id = 'share-scope'`).Execute() + pr := NewPlaylistRepository(adminCtx, b) + _ = pr.Delete(plsID) + mr := NewMediaFileRepository(adminCtx, b).(*mediaFileRepository) + _, _ = mr.executeSQL(squirrel.Delete("media_file").Where(squirrel.Eq{"id": []string{"share-other", "share-ok"}})) + lr := NewLibraryRepository(adminCtx, b).(*libraryRepository) + _ = lr.delete(squirrel.Eq{"id": otherLib.ID}) + _ = NewUserRepository(adminCtx, b).Delete(owner.ID) + }) + + It("excludes tracks the owner cannot access from the shared playlist", func() { + // Read the share as admin (mimics the public-share render path, which uses + // the share repository's own context). loadMedia must scope to the owner. + adminRepo := NewShareRepository(request.WithUser(log.NewContext(GinkgoT().Context()), adminUser), GetDBXBuilder()) + share, err := adminRepo.Get("share-scope") + Expect(err).ToNot(HaveOccurred()) + + Expect(share.Tracks).To(ContainElement(HaveField("ID", "share-ok"))) + Expect(share.Tracks).ToNot(ContainElement(HaveField("ID", "share-other")), + "a track outside the owner's libraries must not appear in the share") + }) + + It("returns no tracks when the playlist is not visible to the owner", func() { + // A private playlist owned by someone else: the share owner can no longer + // see it, so Tracks() returns nil. The share must render with no tracks + // instead of panicking. + privatePlsID := "private-pls" + adminCtx := request.WithUser(log.NewContext(GinkgoT().Context()), adminUser) + pr := NewPlaylistRepository(adminCtx, GetDBXBuilder()) + privatePls := &model.Playlist{ID: privatePlsID, Name: "Private", OwnerID: adminUser.ID, Public: false} + privatePls.AddMediaFiles(model.MediaFiles{{ID: "share-ok"}}) + Expect(pr.Put(privatePls)).To(Succeed()) + DeferCleanup(func() { _ = pr.Delete(privatePlsID) }) + + _, err := GetDBXBuilder().NewQuery(` + INSERT INTO share (id, user_id, description, resource_type, resource_ids, created_at, updated_at) + VALUES ({:id}, {:user}, {:desc}, {:type}, {:ids}, {:created}, {:updated}) + `).Bind(map[string]any{ + "id": "share-private", "user": owner.ID, "desc": "Private share", + "type": "playlist", "ids": privatePlsID, "created": time.Now(), "updated": time.Now(), + }).Execute() + Expect(err).ToNot(HaveOccurred()) + DeferCleanup(func() { _, _ = GetDBXBuilder().NewQuery(`DELETE FROM share WHERE id = 'share-private'`).Execute() }) + + adminRepo := NewShareRepository(adminCtx, GetDBXBuilder()) + share, err := adminRepo.Get("share-private") + Expect(err).ToNot(HaveOccurred()) + Expect(share.Tracks).To(BeEmpty()) + }) + }) + Describe("Ownership Checks", func() { var ownerUser = model.User{ID: "2222", UserName: "regular-user"} var otherUser = model.User{ID: "3333", UserName: "third-user"} diff --git a/server/public/handle_streams.go b/server/public/handle_streams.go index c7b8a4d4f..8fc407e9e 100644 --- a/server/public/handle_streams.go +++ b/server/public/handle_streams.go @@ -25,6 +25,7 @@ func (pub *Router) handleStream(w http.ResponseWriter, r *http.Request) { return } + var shareOwner *model.User if info.shareID != "" { share, err := pub.ds.Share(ctx).Get(info.shareID) if err != nil { @@ -35,6 +36,12 @@ func (pub *Router) handleStream(w http.ResponseWriter, r *http.Request) { checkShareError(ctx, w, model.ErrExpired, info.shareID) return } + shareOwner, err = pub.ds.User(ctx).Get(share.UserID) + if err != nil { + log.Error(ctx, "Error retrieving share owner for shared stream", "share", info.shareID, "owner", share.UserID, err) + http.Error(w, "internal error", http.StatusInternalServerError) + return + } } mf, err := pub.ds.MediaFile(ctx).Get(info.id) @@ -48,6 +55,12 @@ func (pub *Router) handleStream(w http.ResponseWriter, r *http.Request) { return } + // 404 rather than 403 so the response doesn't reveal whether the id exists. + if shareOwner != nil && !shareOwner.HasLibraryAccess(mf.LibraryID) { + http.Error(w, "not found", http.StatusNotFound) + return + } + stream, err := pub.streamer.NewStream(ctx, mf, streampkg.Request{ Format: info.format, BitRate: info.bitrate, }) diff --git a/server/public/handle_streams_test.go b/server/public/handle_streams_test.go index f43d75a26..6fa083045 100644 --- a/server/public/handle_streams_test.go +++ b/server/public/handle_streams_test.go @@ -131,11 +131,22 @@ var _ = Describe("handleStream", func() { return w } - It("passes all validation and reaches the streamer for a valid token", func() { + shareOwnedBy := func(owner model.User, mf model.MediaFile) { shareRepo.ID = "share123" + shareRepo.Entity = &model.Share{ID: "share123", UserID: owner.ID} + userRepo := tests.CreateMockUserRepo() + Expect(userRepo.Put(&owner)).To(Succeed()) + ds.MockedUser = userRepo mfRepo := tests.CreateMockMediaFileRepo() - mfRepo.SetData(model.MediaFiles{{ID: "mf-123", Title: "Test Song"}}) + mfRepo.SetData(model.MediaFiles{mf}) ds.MockedMediaFile = mfRepo + } + + It("passes all validation and reaches the streamer for a valid token", func() { + shareOwnedBy( + model.User{ID: "owner1", UserName: "owner1", IsAdmin: true}, + model.MediaFile{ID: "mf-123", Title: "Test Song"}, + ) claims := auth.Claims{ID: "mf-123", Format: "mp3", BitRate: 192, ShareID: "share123"} token, _ := auth.CreateExpiringPublicToken(time.Now().Add(time.Hour), claims) @@ -146,6 +157,33 @@ var _ = Describe("handleStream", func() { Expect(streamer.req.BitRate).To(Equal(192)) }) + It("returns 404 when the track is outside the share owner's libraries", func() { + shareOwnedBy( + model.User{ID: "owner1", UserName: "owner1", Libraries: model.Libraries{{ID: 1}}}, + model.MediaFile{ID: "mf-restricted", Title: "Other Lib Track", LibraryID: 2}, + ) + + claims := auth.Claims{ID: "mf-restricted", ShareID: "share123"} + token, _ := auth.CreateExpiringPublicToken(time.Now().Add(time.Hour), claims) + w := makeRequest(token) + + Expect(w.Code).To(Equal(http.StatusNotFound)) + Expect(streamer.called).To(BeFalse()) + }) + + It("streams a track inside the share owner's libraries", func() { + shareOwnedBy( + model.User{ID: "owner1", UserName: "owner1", Libraries: model.Libraries{{ID: 1}}}, + model.MediaFile{ID: "mf-ok", Title: "OK", LibraryID: 1}, + ) + + claims := auth.Claims{ID: "mf-ok", Format: "mp3", ShareID: "share123"} + token, _ := auth.CreateExpiringPublicToken(time.Now().Add(time.Hour), claims) + makeRequest(token) + + Expect(streamer.called).To(BeTrue()) + }) + It("returns 400 for an expired token", func() { claims := auth.Claims{ID: "mf-123", ShareID: "share123"} token, _ := auth.CreateExpiringPublicToken(time.Now().Add(-time.Hour), claims)