fix(artwork): enforce entity visibility on the Subsonic getCoverArt path

serveEntity reads persisted item_artwork by id, bypassing the library and private-
playlist filters that the legacy entity-load applied. On the authenticated Subsonic
path a user could fetch artwork for an inaccessible album or someone else's private
playlist by guessing an id. getCoverArt now resolves the underlying entity through
the request-scoped (filtered) repositories and serves the placeholder when it is not
visible, so existence isn't leaked and the always-an-image invariant holds. The
public share (JWT-authorized) and Jellyfin (admin) paths are intentionally untouched.
This commit is contained in:
Deluan 2026-07-23 08:52:31 -04:00
parent 8d715ba2fa
commit 9dd306eb10
3 changed files with 66 additions and 0 deletions

View File

@ -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
}

View File

@ -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")

View File

@ -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")