diff --git a/core/artwork/serving.go b/core/artwork/serving.go index 4b86644d9..4cdd04883 100644 --- a/core/artwork/serving.go +++ b/core/artwork/serving.go @@ -308,6 +308,10 @@ func (s *service) enqueue(ctx context.Context, artID model.ArtworkID, priority i } func (s *service) placeholder(kind model.Kind) *Image { + return placeholderImage(kind) +} + +func placeholderImage(kind model.Kind) *Image { path := consts.PlaceholderAlbumArt if kind == model.KindArtistArtwork { path = consts.PlaceholderArtistArt @@ -316,6 +320,13 @@ func (s *service) placeholder(kind model.Kind) *Image { return &Image{ReadCloser: r, Placeholder: true} } +// PlaceholderFor returns the kind-appropriate placeholder for an artwork id, for callers that must +// serve a placeholder without consulting persisted state (e.g. an access-control denial). +func PlaceholderFor(id string) *Image { + artID, _ := model.ParseArtworkID(id) + return placeholderImage(artID.Kind) +} + type coverArtIDGetter interface { CoverArtID() model.ArtworkID } diff --git a/server/subsonic/media_retrieval.go b/server/subsonic/media_retrieval.go index 30124c5f8..35e9c1945 100644 --- a/server/subsonic/media_retrieval.go +++ b/server/subsonic/media_retrieval.go @@ -10,6 +10,7 @@ import ( "github.com/navidrome/navidrome/conf" "github.com/navidrome/navidrome/consts" + "github.com/navidrome/navidrome/core/artwork" "github.com/navidrome/navidrome/log" "github.com/navidrome/navidrome/model" "github.com/navidrome/navidrome/resources" @@ -78,6 +79,14 @@ func (api *Router) GetCoverArt(w http.ResponseWriter, r *http.Request) (*respons log.Error(r, "Error retrieving coverArt", "id", id, err) return nil, err } + + // Access control: the serving path reads persisted state by id, bypassing the library and + // private-playlist filters. On this authenticated path, fall back to the placeholder for an + // entity the caller cannot see, so a guessed id can't leak artwork (matches the legacy load). + if id != "" && !img.Placeholder && !api.artworkAccessible(ctx, id) { + _ = img.Close() + img = artwork.PlaceholderFor(id) + } defer img.Close() artID, _ := model.ParseArtworkID(id) @@ -92,6 +101,37 @@ func (api *Router) GetCoverArt(w http.ResponseWriter, r *http.Request) (*respons return nil, err } +// artworkAccessible reports whether the caller may view the artwork for id, by resolving the +// underlying entity through the request-scoped (filtered) repositories. Radios are global and +// always accessible; an unparsable/unknown id defers to GetEntityByID. +func (api *Router) artworkAccessible(ctx context.Context, id string) bool { + artID, err := model.ParseArtworkID(id) + if err != nil { + _, err := model.GetEntityByID(ctx, api.ds, id) + return err == nil + } + var lookupErr error + switch artID.Kind { + case model.KindArtistArtwork: + _, lookupErr = api.ds.Artist(ctx).Get(artID.ID) + case model.KindAlbumArtwork: + _, lookupErr = api.ds.Album(ctx).Get(artID.ID) + case model.KindMediaFileArtwork: + _, lookupErr = api.ds.MediaFile(ctx).Get(artID.ID) + case model.KindPlaylistArtwork: + _, lookupErr = api.ds.Playlist(ctx).Get(artID.ID) + case model.KindDiscArtwork: + albumID, _, perr := model.ParseDiscArtworkID(artID.ID) + if perr != nil { + return false + } + _, lookupErr = api.ds.Album(ctx).Get(albumID) + default: // radio and anything else has no per-user artwork access control + return true + } + return lookupErr == nil +} + func (api *Router) GetLyrics(r *http.Request) (*responses.Subsonic, error) { p := req.Params(r) artist, _ := p.String("artist") diff --git a/server/subsonic/media_retrieval_test.go b/server/subsonic/media_retrieval_test.go index db15d5bfd..1f6cbe84d 100644 --- a/server/subsonic/media_retrieval_test.go +++ b/server/subsonic/media_retrieval_test.go @@ -30,8 +30,11 @@ var _ = Describe("MediaRetrievalController", func() { var w *httptest.ResponseRecorder BeforeEach(func() { + albumRepo := &tests.MockAlbumRepo{} + albumRepo.SetData(model.Albums{{ID: "34"}}) // the id the specs request, made accessible ds = &tests.MockDataStore{ MockedMediaFile: mockRepo, + MockedAlbum: albumRepo, } artwork = &fakeArtwork{data: "image data"} router = New(ds, artwork, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, lyrics.NewLyrics(ds, nil), nil, nil) @@ -60,6 +63,18 @@ var _ = Describe("MediaRetrievalController", func() { Expect(w.Body.String()).To(Equal(artwork.data)) }) + It("serves a placeholder for an entity the caller cannot access", func() { + // al-99 is not in the (filtered) album repo, so the caller must not get its bytes. + r := newGetRequest("id=al-99") + _, err := router.GetCoverArt(w, r) + + Expect(err).ToNot(HaveOccurred()) + Expect(w.Code).To(Equal(200)) + Expect(w.Body.String()).ToNot(Equal(artwork.data)) + Expect(w.Header().Get("Cache-Control")).To(Equal("no-store")) + Expect(w.Header().Get("ETag")).To(BeEmpty()) + }) + It("should fail when the file is not found", func() { artwork.err = model.ErrNotFound r := newGetRequest("id=34", "size=128", "square=true")