mirror of
https://github.com/navidrome/navidrome.git
synced 2026-08-31 07:30:32 +00:00
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.
This commit is contained in:
parent
0669f23471
commit
4cac0b1401
@ -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) {
|
||||
|
||||
@ -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),
|
||||
)
|
||||
})
|
||||
|
||||
@ -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)
|
||||
|
||||
@ -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 {
|
||||
|
||||
@ -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 {
|
||||
|
||||
@ -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 {
|
||||
|
||||
@ -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())
|
||||
})
|
||||
})
|
||||
})
|
||||
|
||||
@ -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")
|
||||
})
|
||||
})
|
||||
})
|
||||
|
||||
@ -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
|
||||
|
||||
@ -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"}})
|
||||
|
||||
@ -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)
|
||||
|
||||
|
||||
@ -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 + `"`))
|
||||
})
|
||||
|
||||
})
|
||||
|
||||
@ -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() {
|
||||
|
||||
@ -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")
|
||||
|
||||
@ -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
|
||||
}
|
||||
|
||||
Loading…
x
Reference in New Issue
Block a user