fix(album): never write over a cover file shared by another album

The shared-file guard stopped deletion, but re-uploading a cover on the album
whose id matches the shared filename derived the same deterministic path, and
SetImage's os.Create truncated the file the other album still references —
silently changing its cover too.

When the current file is shared, upload under a de-duplicated name so the
derived filename is unique; the shared file stays intact for the other row
and the purge GC reaps it when the last reference goes.
This commit is contained in:
Deluan 2026-07-17 23:36:08 -04:00
parent cbc12034a4
commit ee7c4d3b88
2 changed files with 87 additions and 1 deletions

View File

@ -3,8 +3,10 @@ package nativeapi
import (
"context"
"errors"
"fmt"
"io"
"net/http"
"time"
"github.com/deluan/rest"
"github.com/go-chi/chi/v5"
@ -42,7 +44,13 @@ func (api *Router) uploadAlbumImage() http.HandlerFunc {
if err != nil {
return err
}
filename, err := api.imgUpload.SetImage(ctx, consts.EntityAlbum, al.ID, al.Name, oldPath, reader, ext)
name := al.Name
if oldPath == "" && al.UploadedImage != "" {
// Current file is shared (post album-ID copy): write under a unique name so
// SetImage can't truncate the path the other album still references.
name = fmt.Sprintf("%s-%d", al.Name, time.Now().UnixMilli())
}
filename, err := api.imgUpload.SetImage(ctx, consts.EntityAlbum, al.ID, name, oldPath, reader, ext)
if err != nil {
return err
}

View File

@ -1,9 +1,14 @@
package nativeapi
import (
"bytes"
"context"
"io"
"mime/multipart"
"net/http"
"net/http/httptest"
"github.com/go-chi/chi/v5"
"github.com/navidrome/navidrome/conf"
"github.com/navidrome/navidrome/conf/configtest"
"github.com/navidrome/navidrome/core"
@ -51,3 +56,76 @@ var _ = Describe("Album Image Endpoints", func() {
Entry("disabled, regular user is forbidden", false, false, http.StatusForbidden),
)
})
// tinyPNG is a valid 1x1 PNG, enough to pass handleImageUpload's image validation.
var tinyPNG = []byte{
0x89, 0x50, 0x4e, 0x47, 0x0d, 0x0a, 0x1a, 0x0a, 0x00, 0x00, 0x00, 0x0d, 0x49, 0x48, 0x44, 0x52,
0x00, 0x00, 0x00, 0x01, 0x00, 0x00, 0x00, 0x01, 0x08, 0x02, 0x00, 0x00, 0x00, 0x90, 0x77, 0x53,
0xde, 0x00, 0x00, 0x00, 0x0c, 0x49, 0x44, 0x41, 0x54, 0x78, 0x9c, 0x63, 0x60, 0x64, 0x62, 0x06,
0x00, 0x00, 0x0e, 0x00, 0x07, 0xd7, 0x6f, 0xe4, 0x78, 0x00, 0x00, 0x00, 0x00, 0x49, 0x45, 0x4e,
0x44, 0xae, 0x42, 0x60, 0x82,
}
type fakeImgUpload struct {
entityID, name, oldPath string
}
func (f *fakeImgUpload) SetImage(_ context.Context, _ string, entityID string, name string, oldPath string, _ io.Reader, _ string) (string, error) {
f.entityID, f.name, f.oldPath = entityID, name, oldPath
return "stored.png", nil
}
func (f *fakeImgUpload) RemoveImage(_ context.Context, path string) error {
f.oldPath = path
return nil
}
var _ = Describe("uploadAlbumImage shared-file handling", func() {
var api *Router
var fake *fakeImgUpload
upload := func(albumID string) {
body := &bytes.Buffer{}
w := multipart.NewWriter(body)
fw, err := w.CreateFormFile("image", "c.png")
Expect(err).ToNot(HaveOccurred())
_, _ = fw.Write(tinyPNG)
Expect(w.Close()).To(Succeed())
rctx := chi.NewRouteContext()
rctx.URLParams.Add("id", albumID)
ctx := request.WithUser(GinkgoT().Context(), model.User{ID: "u", IsAdmin: true})
ctx = context.WithValue(ctx, chi.RouteCtxKey, rctx)
req := httptest.NewRequest("POST", "/album/"+albumID+"/image", body).WithContext(ctx)
req.Header.Set("Content-Type", w.FormDataContentType())
rec := httptest.NewRecorder()
api.uploadAlbumImage().ServeHTTP(rec, req)
Expect(rec.Code).To(Equal(http.StatusOK))
}
BeforeEach(func() {
DeferCleanup(configtest.SetupConfig())
ds := &tests.MockDataStore{}
ds.Album(GinkgoT().Context()).(*tests.MockAlbumRepo).SetData(model.Albums{
{ID: "al-1", Name: "Album One", LibraryID: 1, UploadedImage: "shared.jpg"},
{ID: "al-2", Name: "Album Two", LibraryID: 1, UploadedImage: "shared.jpg"},
{ID: "al-solo", Name: "Album Solo", LibraryID: 1, UploadedImage: "solo.jpg"},
})
fake = &fakeImgUpload{}
api = &Router{ds: ds, imgUpload: fake}
})
It("replaces in place when the file is not shared", func() {
upload("al-solo")
Expect(fake.oldPath).ToNot(BeEmpty(), "sole reference: old file should be removed")
Expect(fake.name).To(Equal("Album Solo"))
})
It("keeps the shared file and writes under a unique name", func() {
upload("al-1")
Expect(fake.oldPath).To(BeEmpty(), "shared file must not be removed")
Expect(fake.name).ToNot(Equal("Album One"), "name must be de-duplicated so the derived filename is unique")
Expect(fake.name).To(HavePrefix("Album One-"))
})
})