From 4cac0b1401d0eb4c1516108071724d3fbec4bad2 Mon Sep 17 00:00:00 2001 From: Deluan Date: Mon, 27 Jul 2026 16:10:31 -0400 Subject: [PATCH] fix(artwork): stop serving artwork for entities that no longer exist Artwork state and its bytes outlive a deleted entity until the next prune (@daily), and the serving path consulted only item_artwork, so a Subsonic id or a signed public token kept serving a removed entity's image in the meantime. Master's readers loaded the entity first, so this was a regression. The check goes in serveHash, the one path that can hand back a found row's bytes: absent rows are already unavailable, and the provisional and disc paths load their entity to resolve at all. Doing it there instead of per-handler also settles who owns the invariant. Subsonic had worked around it with artworkAccessible, whose comment described the service "bypassing the library and private-playlist filters"; that workaround is now deleted, since the service resolves through the request-scoped repositories and enforces the filters itself. Because those repositories are ctx-scoped, each caller says what it wants by what it passes: Subsonic hands over the request context and so gets visibility as well as existence, while the public image route elevates like the Jellyfin one already did -- a token is the authorization there, and a visibility check would hide a shared private playlist, the very case shares exist for. Two supporting fixes. albumRepository.Exists and mediaFileRepository .Exists used the plain exists() helper, which applies no library filter, so they reported rows in libraries the caller cannot see; they now count through applyLibraryFilter as CountAll and artistRepository.Exists already do. Neither had a production caller. RadioRepository gained the Exists it lacked. Tests follow the layers: the service refuses a found row whose entity is gone, the repositories hide rows the caller may not see, and the handlers only assert the context they hand over. Jellyfin needed no change -- resolveArtworkID probes the entity tables, so a deleted item yields an empty artwork id -- but that protection was incidental and untested, so it is pinned now. --- core/artwork/artwork.go | 34 +++++++++++ core/artwork/artwork_test.go | 60 ++++++++++++++++++++ model/radio.go | 1 + persistence/album_repository.go | 5 +- persistence/album_repository_test.go | 17 ++++++ persistence/mediafile_repository.go | 5 +- persistence/mediafile_repository_test.go | 12 ++++ persistence/playlist_repository_test.go | 24 ++++++++ persistence/radio_repository.go | 5 ++ server/jellyfin/images_test.go | 14 +++++ server/public/handle_images.go | 5 ++ server/public/handle_images_test.go | 15 ++++- server/subsonic/e2e/subsonic_artwork_test.go | 15 +++++ server/subsonic/media_retrieval.go | 41 ------------- server/subsonic/media_retrieval_test.go | 44 ++++++-------- 15 files changed, 223 insertions(+), 74 deletions(-) diff --git a/core/artwork/artwork.go b/core/artwork/artwork.go index c293129c3..1f75b8550 100644 --- a/core/artwork/artwork.go +++ b/core/artwork/artwork.go @@ -53,6 +53,35 @@ func NewArtwork(ds model.DataStore, cache cache.FileCache, store *ImageStore, ff return &service{ds: ds, cache: cache, store: store, ffmpeg: ffm} } +// EntityExists reports whether the entity an artwork id points at is still there: state rows +// outlive a deleted entity until the next prune, so a servable row is not evidence of its owner. +// The repositories are ctx-scoped, so a request context makes this a visibility check too. +func EntityExists(ctx context.Context, ds model.DataStore, artID model.ArtworkID) bool { + var found bool + var err error + switch artID.Kind { + case model.KindArtistArtwork: + found, err = ds.Artist(ctx).Exists(artID.ID) + case model.KindAlbumArtwork: + found, err = ds.Album(ctx).Exists(artID.ID) + case model.KindMediaFileArtwork: + found, err = ds.MediaFile(ctx).Exists(artID.ID) + case model.KindPlaylistArtwork: + found, err = ds.Playlist(ctx).Exists(artID.ID) + case model.KindRadioArtwork: + found, err = ds.Radio(ctx).Exists(artID.ID) + case model.KindDiscArtwork: + albumID, _, perr := model.ParseDiscArtworkID(artID.ID) + if perr != nil { + return false + } + found, err = ds.Album(ctx).Exists(albumID) + default: + return false + } + return err == nil && found +} + type service struct { ds model.DataStore cache cache.FileCache @@ -150,6 +179,11 @@ func (s *service) serveSource(ctx context.Context, key, hash string, lastUpdate // serveHash serves the bytes of a found state row. A mismatch/open error is dangling (a warm // cache still serves), but a cancelled request is not: it must not enqueue a re-resolution. func (s *service) serveHash(ctx context.Context, artID model.ArtworkID, ia *model.ItemArtwork, size int, square bool) (*Image, error) { + // Only this path can hand back a deleted entity's bytes: an absent row is already + // unavailable, and the provisional and disc paths load their entity to resolve at all. + if !EntityExists(ctx, s.ds, artID) { + return nil, ErrUnavailable + } art, err := s.ds.Artwork(ctx).GetImage(ia.Hash) if err != nil { if errors.Is(err, model.ErrNotFound) { diff --git a/core/artwork/artwork_test.go b/core/artwork/artwork_test.go index 37c385aff..4ee99edec 100644 --- a/core/artwork/artwork_test.go +++ b/core/artwork/artwork_test.go @@ -36,6 +36,7 @@ var _ = Describe("Artwork", func() { svc Artwork repoRoot string coverBytes []byte + seedEntity func(kind, id string) ) primaryKey := func(kind, id string) string { return kind + "|" + id + "|" + model.ImageTypePrimary } @@ -48,9 +49,22 @@ var _ = Describe("Artwork", func() { Expect(store.Write(hash, "image/jpeg", bytes.NewReader(imgBytes))).To(Succeed()) Expect(artRepo.PutImage(&model.Artwork{Hash: hash, Mime: "image/jpeg"})).To(Succeed()) Expect(artRepo.PutItemArtwork(&model.ItemArtwork{ItemKind: kind, ItemID: id, Hash: hash, Source: "external"})).To(Succeed()) + seedEntity(kind, id) return hash } + // seedEntity registers the owning entity, without which the state row describes something + // that no longer exists and the service correctly refuses to serve it. + seedEntity = func(kind, id string) { + GinkgoHelper() + switch kind { + case "al": + Expect(albumRepo.Put(&model.Album{ID: id, Name: "Album"})).To(Succeed()) + case "mf": + Expect(mfRepo.Put(&model.MediaFile{ID: id})).To(Succeed()) + } + } + readAll := func(img *Image) []byte { GinkgoHelper() defer img.Close() @@ -139,6 +153,7 @@ var _ = Describe("Artwork", func() { Expect(os.WriteFile(imgPath, coverBytes, 0600)).To(Succeed()) mtime := fileMtime(imgPath) Expect(artRepo.PutImage(&model.Artwork{Hash: "aaaaaaaaaaaaaaaa", Mime: "image/jpeg"})).To(Succeed()) + seedEntity("al", "al2") Expect(artRepo.PutItemArtwork(&model.ItemArtwork{ ItemKind: "al", ItemID: "al2", Hash: "aaaaaaaaaaaaaaaa", Source: "folder", SourcePath: imgPath, RefMtime: mtime, @@ -154,6 +169,7 @@ var _ = Describe("Artwork", func() { imgPath := filepath.Join(dir, "cover.jpg") Expect(os.WriteFile(imgPath, coverBytes, 0600)).To(Succeed()) Expect(artRepo.PutImage(&model.Artwork{Hash: "bbbbbbbbbbbbbbbb", Mime: "image/jpeg"})).To(Succeed()) + seedEntity("al", "al3") Expect(artRepo.PutItemArtwork(&model.ItemArtwork{ ItemKind: "al", ItemID: "al3", Hash: "bbbbbbbbbbbbbbbb", Source: "folder", SourcePath: imgPath, RefMtime: fileMtime(imgPath) + 999, @@ -172,6 +188,7 @@ var _ = Describe("Artwork", func() { imgPath := filepath.Join(dir, "cover.jpg") Expect(os.WriteFile(imgPath, coverBytes, 0600)).To(Succeed()) Expect(artRepo.PutImage(&model.Artwork{Hash: "cccccccccccccccc", Mime: "image/jpeg"})).To(Succeed()) + seedEntity("al", "al3b") Expect(artRepo.PutItemArtwork(&model.ItemArtwork{ ItemKind: "al", ItemID: "al3b", Hash: "cccccccccccccccc", Source: "folder", SourcePath: imgPath, RefMtime: fileMtime(imgPath) + 999, @@ -182,6 +199,17 @@ var _ = Describe("Artwork", func() { Expect(queueRepo.Data[primaryKey("al", "al3b")].Priority).To(Equal(model.ArtworkPriorityScan)) }) + // State rows and their bytes outlive a deleted entity until the next prune, so serving + // straight from the row would keep handing out a removed entity's image. + It("refuses to serve a found row whose entity is gone", func() { + hash := seedFoundStore("al", "alzz", coverBytes) + Expect(hash).ToNot(BeEmpty()) + albumRepo.SetData(model.Albums{}) // the album is deleted; its artwork row survives + + _, err := svc.Get(ctx, model.MustParseArtworkID("al-alzz"), 0, false) + Expect(err).To(MatchError(ErrUnavailable)) + }) + It("does not re-enqueue a recently-attempted absent state", func() { Expect(artRepo.PutItemArtwork(&model.ItemArtwork{ ItemKind: "al", ItemID: "al4", AttemptedAt: time.Now(), @@ -480,3 +508,35 @@ func fileMtime(path string) int64 { Expect(err).ToNot(HaveOccurred()) return info.ModTime().UnixNano() } + +var _ = Describe("EntityExists", func() { + var ctx context.Context + var ds *tests.MockDataStore + + BeforeEach(func() { + ctx = context.Background() + albumRepo := tests.CreateMockAlbumRepo() + albumRepo.SetData(model.Albums{{ID: "al1"}}) + artistRepo := tests.CreateMockArtistRepo() + artistRepo.SetData(model.Artists{{ID: "ar1"}}) + radioRepo := tests.CreateMockedRadioRepo() + Expect(radioRepo.Put(&model.Radio{ID: "ra1", Name: "R"})).To(Succeed()) + ds = &tests.MockDataStore{MockedAlbum: albumRepo, MockedArtist: artistRepo, MockedRadio: radioRepo} + }) + + DescribeTable("reports whether the owning entity is still there", + func(id string, expected bool) { + Expect(EntityExists(ctx, ds, model.MustParseArtworkID(id))).To(Equal(expected)) + }, + Entry("existing album", "al-al1", true), + Entry("deleted album", "al-gone", false), + Entry("existing artist", "ar-ar1", true), + Entry("deleted artist", "ar-gone", false), + Entry("existing radio", "ra-ra1", true), + Entry("deleted radio", "ra-gone", false), + // Disc art has no entity of its own; it stands or falls with its album. + Entry("disc of an existing album", "dc-al1:1", true), + Entry("disc of a deleted album", "dc-gone:1", false), + Entry("malformed disc id", "dc-nodiscnum", false), + ) +}) diff --git a/model/radio.go b/model/radio.go index 5ee0e86fe..466ff48b0 100644 --- a/model/radio.go +++ b/model/radio.go @@ -32,6 +32,7 @@ type RadioRepository interface { ResourceRepository CountAll(options ...QueryOptions) (int64, error) Delete(id string) error + Exists(id string) (bool, error) Get(id string) (*Radio, error) GetAll(options ...QueryOptions) (Radios, error) GetAllIDs(options ...QueryOptions) ([]string, error) diff --git a/persistence/album_repository.go b/persistence/album_repository.go index fe919dd29..75f98d5d9 100644 --- a/persistence/album_repository.go +++ b/persistence/album_repository.go @@ -202,7 +202,10 @@ func (r *albumRepository) CountAll(options ...model.QueryOptions) (int64, error) } func (r *albumRepository) Exists(id string) (bool, error) { - return r.exists(Eq{"album.id": id}) + // Filtered like CountAll: the plain exists() helper applies no library filter, so it + // would report a row in a library the caller cannot see. + c, err := r.count(r.applyLibraryFilter(r.newSelect().Where(Eq{"album.id": id}))) + return c > 0, err } func (r *albumRepository) Put(al *model.Album) error { diff --git a/persistence/album_repository_test.go b/persistence/album_repository_test.go index ef18cf426..66acef072 100644 --- a/persistence/album_repository_test.go +++ b/persistence/album_repository_test.go @@ -992,6 +992,23 @@ var _ = Describe("AlbumRepository", func() { Expect(got.RGAlbumPeak).To(BeNil()) }) }) + + // Exists used the unfiltered helper, so it reported albums in libraries the caller + // cannot see -- the same leak Get/GetAll/CountAll already guard against. + Describe("Exists library visibility", func() { + It("hides an album the user has no library access to", func() { + Expect(albumRepo.Put(&model.Album{ID: "vis-album", Name: "Vis", LibraryID: 1})).To(Succeed()) + DeferCleanup(func() { + _, _ = albumRepo.executeSQL(squirrel.Delete("album").Where(squirrel.Eq{"id": "vis-album"})) + }) + + Expect(albumRepo.Exists("vis-album")).To(BeTrue(), "admin sees it") + + restricted := model.User{ID: "restricted_album_user", UserName: "ra", Name: "RA", Email: "ra@t.com"} + rctx := request.WithUser(GinkgoT().Context(), restricted) + Expect(NewAlbumRepository(rctx, GetDBXBuilder()).Exists("vis-album")).To(BeFalse()) + }) + }) }) func _p(id, name string, sortName ...string) model.Participant { diff --git a/persistence/mediafile_repository.go b/persistence/mediafile_repository.go index 62b77a368..6b47b5228 100644 --- a/persistence/mediafile_repository.go +++ b/persistence/mediafile_repository.go @@ -163,7 +163,10 @@ func (r *mediaFileRepository) CountBySuffix(options ...model.QueryOptions) (map[ } func (r *mediaFileRepository) Exists(id string) (bool, error) { - return r.exists(Eq{"media_file.id": id}) + // Filtered like CountAll: the plain exists() helper applies no library filter, so it + // would report a row in a library the caller cannot see. + c, err := r.count(r.applyLibraryFilter(r.newSelect().Where(Eq{"media_file.id": id}))) + return c > 0, err } func (r *mediaFileRepository) Put(m *model.MediaFile) error { diff --git a/persistence/mediafile_repository_test.go b/persistence/mediafile_repository_test.go index 6e892c8f5..d0e7071cf 100644 --- a/persistence/mediafile_repository_test.go +++ b/persistence/mediafile_repository_test.go @@ -1118,4 +1118,16 @@ var _ = Describe("MediaRepository", func() { Expect(mf.AlbumImage.ImageHash).To(Equal("bbbbbbbbbbbbbbbb")) }) }) + + // Exists used the unfiltered helper, so it reported tracks in libraries the caller + // cannot see -- the same leak Get/GetAll/CountAll already guard against. + Describe("Exists library visibility", func() { + It("hides a track the user has no library access to", func() { + restricted := model.User{ID: "restricted_mf_user", UserName: "rm", Name: "RM", Email: "rm@t.com"} + rctx := request.WithUser(GinkgoT().Context(), restricted) + + Expect(mr.Exists(songAntenna.ID)).To(BeTrue(), "admin sees it") + Expect(NewMediaFileRepository(rctx, GetDBXBuilder()).Exists(songAntenna.ID)).To(BeFalse()) + }) + }) }) diff --git a/persistence/playlist_repository_test.go b/persistence/playlist_repository_test.go index 9905d3da6..05b40393f 100644 --- a/persistence/playlist_repository_test.go +++ b/persistence/playlist_repository_test.go @@ -384,4 +384,28 @@ var _ = Describe("PlaylistRepository", func() { Expect(mediaFileIDs).To(Equal([]string{"1001", "1002"})) }) }) + + // Exists is ctx-sensitive through userFilter: a private playlist is invisible to anyone but + // its owner or an admin. Callers that only want "does it still exist" -- the public image + // route serving a share -- must elevate, or a shared private playlist looks deleted. + Describe("Exists visibility", func() { + It("hides a private playlist from an unauthenticated context", func() { + // "userid" is the fixture user; playlist.owner_id has a FK to user(id). + owner := model.User{ID: "userid", UserName: "userid"} + octx := request.WithUser(GinkgoT().Context(), owner) + ownerRepo := NewPlaylistRepository(octx, GetDBXBuilder()) + pls := model.Playlist{Name: "Private One", OwnerID: owner.ID, Public: false} + Expect(ownerRepo.Put(&pls)).To(Succeed()) + DeferCleanup(func() { _ = ownerRepo.Delete(pls.ID) }) + + Expect(ownerRepo.Exists(pls.ID)).To(BeTrue(), "the owner sees it") + + anon := NewPlaylistRepository(GinkgoT().Context(), GetDBXBuilder()) + Expect(anon.Exists(pls.ID)).To(BeFalse(), "no user: userFilter hides it") + + admin := request.WithUser(GinkgoT().Context(), model.User{ID: "userid", IsAdmin: true}) + Expect(NewPlaylistRepository(admin, GetDBXBuilder()).Exists(pls.ID)).To(BeTrue(), + "elevating is what the public image route relies on") + }) + }) }) diff --git a/persistence/radio_repository.go b/persistence/radio_repository.go index 2ee5351fe..01558e541 100644 --- a/persistence/radio_repository.go +++ b/persistence/radio_repository.go @@ -38,6 +38,11 @@ func (r *radioRepository) CountAll(options ...model.QueryOptions) (int64, error) return r.count(sql, options...) } +// Exists needs no library or ownership filter: radios are visible to every user. +func (r *radioRepository) Exists(id string) (bool, error) { + return r.exists(Eq{"id": id}) +} + func (r *radioRepository) Delete(id string) error { if !r.isPermitted() { return rest.ErrPermissionDenied diff --git a/server/jellyfin/images_test.go b/server/jellyfin/images_test.go index 9d30e7d6c..f8cdf0701 100644 --- a/server/jellyfin/images_test.go +++ b/server/jellyfin/images_test.go @@ -75,6 +75,20 @@ var _ = Describe("Images", func() { Expect(fa.recvId).To(ContainSubstring("a1")) }) + // resolveArtworkID probes the entity tables, so a deleted item cannot produce an artwork id + // at all -- which is what stops artwork state outliving its entity from being served here. + It("asks for no artwork once the item is deleted, rather than its lingering state", func() { + ds := &tests.MockDataStore{} // no albums/artists/tracks/playlists at all + fa := &fakeArtwork{} + api := &Router{ds: ds, artwork: fa} + + w, r := newImageRequest(dto.EncodeID("deleted-item")) + api.getItemImage(w, r) + + Expect(fa.recvId).To(BeEmpty(), "an empty artwork id can only yield a placeholder") + Expect(w.Body.String()).ToNot(ContainSubstring("deleted-item")) + }) + It("sniffs the Content-Type instead of hardcoding it", func() { ds := &tests.MockDataStore{} ds.Album(context.Background()).(*tests.MockAlbumRepo).SetData(model.Albums{{ID: "a1", Name: "One"}}) diff --git a/server/public/handle_images.go b/server/public/handle_images.go index 9bcaa5933..1c45898c9 100644 --- a/server/public/handle_images.go +++ b/server/public/handle_images.go @@ -11,6 +11,7 @@ import ( "github.com/navidrome/navidrome/core/auth" "github.com/navidrome/navidrome/log" "github.com/navidrome/navidrome/model" + "github.com/navidrome/navidrome/model/request" "github.com/navidrome/navidrome/server/imghttp" "github.com/navidrome/navidrome/utils/req" ) @@ -38,6 +39,10 @@ func (pub *Router) handleImages(w http.ResponseWriter, r *http.Request) { http.Error(w, "invalid request", http.StatusBadRequest) return } + // Elevated like the Jellyfin image route: the token is the authorization, so the service's + // entity check asks "is it still there", not "may this user see it" -- the latter would hide + // a shared private playlist, which is the case shares exist to serve. + ctx = request.WithUser(ctx, model.User{IsAdmin: true}) size := p.IntOr("size", 0) square := p.BoolOr("square", false) diff --git a/server/public/handle_images_test.go b/server/public/handle_images_test.go index 329ff58c9..cef4e0851 100644 --- a/server/public/handle_images_test.go +++ b/server/public/handle_images_test.go @@ -12,6 +12,7 @@ import ( "github.com/navidrome/navidrome/core/artwork" "github.com/navidrome/navidrome/core/auth" "github.com/navidrome/navidrome/model" + "github.com/navidrome/navidrome/tests" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" ) @@ -52,18 +53,25 @@ var _ = Describe("handleImages", func() { return httptest.NewRequest("GET", "/img?:id="+url.QueryEscape(token), nil) } + // The handler re-checks that the entity behind the token still exists, so every spec needs + // a store where "1" is a live album. + var ds *tests.MockDataStore + BeforeEach(func() { w = httptest.NewRecorder() + albumRepo := tests.CreateMockAlbumRepo() + albumRepo.SetData(model.Albums{{ID: "1"}}) + ds = &tests.MockDataStore{MockedAlbum: albumRepo} }) It("returns 404 when the artwork is unavailable", func() { - pub := &Router{artwork: &fakeArtwork{err: artwork.ErrUnavailable}} + pub := &Router{ds: ds, artwork: &fakeArtwork{err: artwork.ErrUnavailable}} pub.handleImages(w, newImageRequest("al-1")) Expect(w.Code).To(Equal(http.StatusNotFound)) }) It("returns 404 when the artwork is not found", func() { - pub := &Router{artwork: &fakeArtwork{err: model.ErrNotFound}} + pub := &Router{ds: ds, artwork: &fakeArtwork{err: model.ErrNotFound}} pub.handleImages(w, newImageRequest("al-1")) Expect(w.Code).To(Equal(http.StatusNotFound)) }) @@ -71,10 +79,11 @@ var _ = Describe("handleImages", func() { It("serves the image immutable when the token asserts the current hash", func() { const hash = "0123456789abcdef" img := &artwork.Image{ReadCloser: io.NopCloser(bytes.NewReader([]byte("IMG"))), Hash: hash} - pub := &Router{artwork: &fakeArtwork{img: img}} + pub := &Router{ds: ds, artwork: &fakeArtwork{img: img}} pub.handleImages(w, newImageRequest("al-1_"+hash)) Expect(w.Code).To(Equal(http.StatusOK)) Expect(w.Header().Get("Cache-Control")).To(Equal("public, max-age=31536000, immutable")) Expect(w.Header().Get("ETag")).To(Equal(`"` + hash + `"`)) }) + }) diff --git a/server/subsonic/e2e/subsonic_artwork_test.go b/server/subsonic/e2e/subsonic_artwork_test.go index 05a25f222..2679b4324 100644 --- a/server/subsonic/e2e/subsonic_artwork_test.go +++ b/server/subsonic/e2e/subsonic_artwork_test.go @@ -201,6 +201,21 @@ var _ = Describe("Artwork Serving", Ordered, func() { Expect(w.Header().Get("Cache-Control")).To(Equal("no-store")) }) + // The BeforeAll grants the artwork library to adminUser only, so regularUser cannot see + // this album -- but its artwork state and bytes are perfectly servable by id. The service + // resolves the entity through the caller's repositories, so the filter still applies. + It("serves the placeholder for an album in a library the caller cannot see", func() { + w := httptest.NewRecorder() + artRouter.ServeHTTP(w, buildReq(regularUser, "getCoverArt", "id", "al-"+artfulID)) + + Expect(w.Code).To(Equal(http.StatusOK)) + Expect(w.Body.Bytes()).To(Equal(placeholder), "must not leak the real cover") + Expect(w.Header().Get("Cache-Control")).To(Equal("no-store")) + + // ...while the admin, who does have access, gets the real bytes for the same id. + Expect(getCover("id", "al-"+artfulID).Body.Bytes()).ToNot(Equal(placeholder)) + }) + // An album with no art and an id naming no album are different answers; only the former // is a placeholder. It("answers error 70 for an id that matches no entity", func() { diff --git a/server/subsonic/media_retrieval.go b/server/subsonic/media_retrieval.go index 34e582638..8a5152a9d 100644 --- a/server/subsonic/media_retrieval.go +++ b/server/subsonic/media_retrieval.go @@ -10,7 +10,6 @@ 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" @@ -80,13 +79,6 @@ func (api *Router) GetCoverArt(w http.ResponseWriter, r *http.Request) (*respons 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) @@ -101,39 +93,6 @@ 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.KindRadioArtwork: - _, lookupErr = api.ds.Radio(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: // 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 cec091a19..63ceaa8fa 100644 --- a/server/subsonic/media_retrieval_test.go +++ b/server/subsonic/media_retrieval_test.go @@ -17,6 +17,7 @@ import ( "github.com/navidrome/navidrome/core/artwork" "github.com/navidrome/navidrome/core/lyrics" "github.com/navidrome/navidrome/model" + "github.com/navidrome/navidrome/model/request" "github.com/navidrome/navidrome/tests" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" @@ -66,36 +67,21 @@ var _ = Describe("MediaRetrievalController", func() { Expect(w.Body.String()).To(Equal(artwork.data)) }) - It("serves radio artwork while the radio exists", func() { - r := newGetRequest("id=ra-rd1") + // Visibility now lives in the service, which resolves the entity through the + // request-scoped repositories. The handler's whole contribution is handing over the + // caller's context unchanged -- elevating here would bypass the library filter. + It("passes the caller's context to the service rather than elevating", func() { + r := newGetRequest("id=al-34") + usr := model.User{ID: "u1", UserName: "u1"} + r = r.WithContext(request.WithUser(r.Context(), usr)) + _, err := router.GetCoverArt(w, r) Expect(err).ToNot(HaveOccurred()) - Expect(w.Body.String()).To(Equal(artwork.data)) - }) - - // A deleted radio keeps its item_artwork row and uploaded file until the next prune, so - // existence has to be re-checked or the old id keeps serving the removed radio's image. - It("serves a placeholder once the radio is gone", func() { - r := newGetRequest("id=ra-deleted") - _, 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")) - }) - - 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()) + got, ok := request.UserFrom(artwork.recvCtx) + Expect(ok).To(BeTrue(), "the service must see who is asking") + Expect(got.ID).To(Equal("u1")) + Expect(got.IsAdmin).To(BeFalse(), "the handler must not elevate") }) It("should fail when the file is not found", func() { @@ -281,9 +267,11 @@ type fakeArtwork struct { recvId string recvSize int recvSquare bool + recvCtx context.Context } -func (c *fakeArtwork) GetOrPlaceholder(_ context.Context, id string, size int, square bool) (*artwork.Image, error) { +func (c *fakeArtwork) GetOrPlaceholder(ctx context.Context, id string, size int, square bool) (*artwork.Image, error) { + c.recvCtx = ctx if c.err != nil { return nil, c.err }