From 8b4125267eda31c48c193776d8dc4653a2e5af48 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Deluan=20Quint=C3=A3o?= Date: Sun, 20 Sep 2026 13:40:14 -0400 Subject: [PATCH] fix(artwork): only use image files as local artwork sources (#6180) * fix(playlists): limit local cover paths to images in owner's libraries A local #EXTALBUMARTURL path (absolute or file://) was only checked against the union of all libraries. The artwork resolver then opened it with no further check and served the bytes as the playlist cover, undecoded. Any user who can upload an M3U could read any file under any library root, including libraries they were not granted, through getCoverArt (GHSA-vwq6-xrw5-phpg). resolveImageURL now requires an image extension, and for uploaded playlists (no folder) the library holding the cover must pass the owner's HasLibraryAccess. Scanner and CLI imports keep the all-libraries check, since those files are admin-controlled. resolveLocalFile, used by every file-backed artwork source, now ignores paths without an image extension, which covers playlists stored before this fix that were not resolved yet. openOriginal refuses a stored file-backed row whose path is not an image, so the existing dangling path re-resolves it and the playlist falls back to the generated grid. No migration is needed. * fix(artwork): skip non-image files matched by folder cover patterns Album and disc folder sources opened any file in the folder's image list that matched a cover pattern, without checking its extension. openOriginal now refuses to serve file-backed rows whose path is not an image, so a stored row like that would be refused, re-resolved to the same file, and refused again on every view. The list comes from the scanner, which only records image files, but a database scanned where the OS mime table knows more image types than the serving process could still reach this. Both fromExternalFile variants now skip matches that are not image files, so the album falls back to its next source instead. Also correct the parser comment: a playlist without a folder can come from an API upload or from a CLI import of a file outside all libraries. * fix(artwork): check stored source type before using the resize cache The image-extension check for file-backed rows ran inside openOriginal, which the resize cache skips on a hit. Before the fix, a resized request for a playlist pointing at a non-image file cached the raw bytes, because a failed resize falls back to the original data. After the upgrade the same request still hit that entry and returned the file. serveHash now refuses a file-backed row whose path is not an image before calling serveSource, so both full-size and resized requests go through dangling and re-resolve the item. The stale cache entry is keyed by the old hash and is no longer reachable once the row changes. * test: register mime_types.yaml in test binaries Artwork resolution now skips candidates that are not image files, and model.IsImageFile answers from the process mime table. The server registers the extra image types from resources/mime_types.yaml through a conf hook, but a test binary only does that if it links conf/mime, so the artwork e2e suite fell back to the host table: .jxl resolves on macOS and Linux and does not on Windows, where the #5950 cover spec then found no source. tests.Init now imports conf/mime for its side effect, so every suite that loads the test config sees the same image types as the server. * fix(artwork): drop the image-file guard from the disc art reader The guard was added to both fromExternalFile variants, but disc artwork keeps no state row and is never queued, so it cannot hit the refuse-and-re-resolve loop the guard exists to prevent. The only case where it can fire is a real image whose extension this process's mime table does not know, and there it drops a disc cover that used to work. The album variant keeps the guard, since those resolutions are stored and re-served. --- core/artwork/artwork.go | 5 +++ core/artwork/artwork_test.go | 47 +++++++++++++++++++++++++++ core/artwork/resolve.go | 2 +- core/artwork/resolve_test.go | 17 ++++++++++ core/artwork/sources.go | 2 +- core/artwork/sources_internal_test.go | 12 +++++++ core/playlists/import_test.go | 31 ++++++++++++++++++ core/playlists/parse_m3u.go | 13 +++++--- tests/init_tests.go | 1 + 9 files changed, 123 insertions(+), 7 deletions(-) diff --git a/core/artwork/artwork.go b/core/artwork/artwork.go index 663d06d25..e27fa118e 100644 --- a/core/artwork/artwork.go +++ b/core/artwork/artwork.go @@ -168,6 +168,11 @@ func (s *service) serveHash(ctx context.Context, artID model.ArtworkID, ia *mode if !entityExists(ctx, s.ds, artID) { return nil, ErrUnavailable } + // Checked here, not in openOriginal: a resize-cache hit never opens the source. + if isFileBacked(ia.Source) && !model.IsImageFile(ia.SourcePath) { + log.Warn(ctx, "Artwork: Stored source is not an image file, re-resolving", "artID", artID, "path", ia.SourcePath) + return s.dangling(ctx, artID) + } 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 907b300de..8b35a872e 100644 --- a/core/artwork/artwork_test.go +++ b/core/artwork/artwork_test.go @@ -159,6 +159,53 @@ var _ = Describe("Artwork", func() { Expect(readAll(img)).To(Equal(coverBytes)) }) + It("treats a file-backed row pointing at a non-image file as dangling", func() { + dir := GinkgoT().TempDir() + secretPath := filepath.Join(dir, "config.ini") + Expect(os.WriteFile(secretPath, []byte("password=secret"), 0600)).To(Succeed()) + Expect(artRepo.PutImage(&model.Artwork{Hash: "dddddddddddddddd", Mime: "image/jpeg"})).To(Succeed()) + seedEntity("al", "alni") + Expect(artRepo.PutItemArtwork(&model.ItemArtwork{ + ItemKind: "al", ItemID: "alni", Hash: "dddddddddddddddd", + Source: "folder", SourcePath: secretPath, RefMtime: fileMtime(secretPath), + })).To(Succeed()) + + _, err := svc.Get(ctx, model.MustParseArtworkID("al-alni"), 0, false) + Expect(err).To(MatchError(ErrUnavailable)) + Expect(queueRepo.Data[primaryKey("al", "alni")].Priority).To(Equal(model.ArtworkPriorityScan)) + }) + + It("refuses a non-image file-backed row even when a resized copy is already cached", func() { + secret := []byte("password=secret") + dir := GinkgoT().TempDir() + secretPath := filepath.Join(dir, "config.ini") + Expect(os.WriteFile(secretPath, secret, 0600)).To(Succeed()) + Expect(artRepo.PutImage(&model.Artwork{Hash: "eeeeeeeeeeeeeeee", Mime: "image/jpeg"})).To(Succeed()) + seedEntity("al", "alnic") + Expect(artRepo.PutItemArtwork(&model.ItemArtwork{ + ItemKind: "al", ItemID: "alnic", Hash: "eeeeeeeeeeeeeeee", + Source: "folder", SourcePath: secretPath, RefMtime: fileMtime(secretPath), + })).To(Succeed()) + + // Older versions cached the raw bytes when the resize failed. + seed := func() (io.ReadCloser, error) { return io.NopCloser(bytes.NewReader(secret)), nil } + stream, err := imgCache.Get(ctx, &resizedItem{hash: "eeeeeeeeeeeeeeee", size: 100, open: seed, ffmpeg: ffm}) + Expect(err).ToNot(HaveOccurred()) + Expect(io.ReadAll(stream)).To(Equal(secret)) + Expect(stream.Close()).To(Succeed()) + Eventually(func(g Gomega) { + s, err := imgCache.Get(ctx, &resizedItem{hash: "eeeeeeeeeeeeeeee", size: 100, ffmpeg: ffm, + open: func() (io.ReadCloser, error) { return nil, os.ErrNotExist }}) + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(s.Cached).To(BeTrue()) + _ = s.Close() + }).Should(Succeed()) + + _, err = svc.Get(ctx, model.MustParseArtworkID("al-alnic"), 100, false) + Expect(err).To(MatchError(ErrUnavailable)) + Expect(queueRepo.Data[primaryKey("al", "alnic")].Priority).To(Equal(model.ArtworkPriorityScan)) + }) + It("treats a full-size mtime mismatch as dangling: unavailable, re-enqueued at Scan, state untouched", func() { dir := GinkgoT().TempDir() imgPath := filepath.Join(dir, "cover.jpg") diff --git a/core/artwork/resolve.go b/core/artwork/resolve.go index e7a2d3765..f4f3ef725 100644 --- a/core/artwork/resolve.go +++ b/core/artwork/resolve.go @@ -572,7 +572,7 @@ func resolveArtistFolderPattern(ctx context.Context, lib libraryView, artistFold // resolveLocalFile opens an absolute path directly. A missing path is "no source"; any other // open failure says nothing about whether the image exists. func resolveLocalFile(path, source string) (resolution, bool) { - if path == "" { + if path == "" || !model.IsImageFile(path) { return resolution{}, false } f, err := os.Open(path) diff --git a/core/artwork/resolve_test.go b/core/artwork/resolve_test.go index 402a11363..da144d8e2 100644 --- a/core/artwork/resolve_test.go +++ b/core/artwork/resolve_test.go @@ -498,6 +498,23 @@ var _ = Describe("resolveItem", func() { Expect(res.refMtime).To(BeNumerically(">", 0)) }) + It("never opens a local ExternalImageURL that is not an image file", func() { + folderRepo.result = nil // no grid tiles, so only the local file could produce a reader + dir := GinkgoT().TempDir() + secretPath := filepath.Join(dir, "config.ini") + Expect(os.WriteFile(secretPath, []byte("password=secret"), 0600)).To(Succeed()) + + plRepo := tests.CreateMockPlaylistRepo() + plRepo.SetData(model.Playlists{{ID: "plni", Name: "Playlist", ExternalImageURL: secretPath}}) + plRepo.TracksRepo = &tests.MockPlaylistTrackRepo{AlbumIDs: []string{"t1"}} + ds.MockedPlaylist = plRepo + + res, err := newResolver(ds, ag, ffm, nil).resolve(ctx, model.ArtworkQueueItem{ItemKind: "pl", ItemID: "plni"}) + Expect(err).ToNot(HaveOccurred()) + Expect(res.reader).To(BeNil()) + Expect(res.sourcePath).ToNot(Equal(secretPath)) + }) + It("routes ExternalImageURL through extGate and sets extError on transient failure", func() { conf.Server.EnableM3UExternalAlbumArt = true folderRepo.result = nil // no grid tiles, so the external failure is what surfaces diff --git a/core/artwork/sources.go b/core/artwork/sources.go index f2abf9da5..ae41acc48 100644 --- a/core/artwork/sources.go +++ b/core/artwork/sources.go @@ -37,7 +37,7 @@ func fromExternalFile(ctx context.Context, libFS fs.FS, files []string, pattern log.Warn(ctx, "Artwork: Error matching cover art file to pattern", "pattern", pattern, "file", file) continue } - if !match { + if !match || !model.IsImageFile(name) { continue } f, err := libFS.Open(file) diff --git a/core/artwork/sources_internal_test.go b/core/artwork/sources_internal_test.go index 4282575a5..5f70b1cc5 100644 --- a/core/artwork/sources_internal_test.go +++ b/core/artwork/sources_internal_test.go @@ -48,6 +48,18 @@ var _ = Describe("fromExternalFile", func() { Expect(b).To(Equal([]byte("a"))) Expect(path).To(Equal("a/cover.jpg")) }) + + It("skips a matching file that is not an image", func() { + fsys := fstest.MapFS{ + "a/cover.ini": &fstest.MapFile{Data: []byte("password=secret")}, + "a/cover.jpg": &fstest.MapFile{Data: []byte("a")}, + } + f := fromExternalFile(GinkgoT().Context(), fsys, []string{"a/cover.ini", "a/cover.jpg"}, "cover.*") + r, path, err := f() + Expect(err).ToNot(HaveOccurred()) + defer r.Close() + Expect(path).To(Equal("a/cover.jpg")) + }) }) var _ = Describe("fromTag", func() { diff --git a/core/playlists/import_test.go b/core/playlists/import_test.go index 445561266..a90a703d9 100644 --- a/core/playlists/import_test.go +++ b/core/playlists/import_test.go @@ -236,6 +236,24 @@ var _ = Describe("Playlists - Import", func() { Expect(pls.ExternalImageURL).To(BeEmpty()) }) + It("rejects #EXTALBUMARTURL pointing at a non-image file inside the library", func() { + tmpDir := GinkgoT().TempDir() + Expect(os.WriteFile(filepath.Join(tmpDir, "config.ini"), []byte("password=secret"), 0600)).To(Succeed()) + + m3u := "#EXTALBUMARTURL:config.ini\ntest.mp3\n" + plsFile := filepath.Join(tmpDir, "test.m3u") + Expect(os.WriteFile(plsFile, []byte(m3u), 0600)).To(Succeed()) + + mockLibRepo.SetData([]model.Library{{ID: 1, Path: tmpDir}}) + ds.MockedMediaFile = &mockedMediaFileFromListRepo{data: []string{"test.mp3"}} + ps = playlists.NewPlaylists(ds, artwork.NewUploader(ds)) + + plsFolder := &model.Folder{ID: "1", LibraryID: 1, LibraryPath: tmpDir, Path: "", Name: ""} + pls, err := ps.ImportFromFolder(ctx, plsFolder, "test.m3u") + Expect(err).ToNot(HaveOccurred()) + Expect(pls.ExternalImageURL).To(BeEmpty()) + }) + It("ignores HTTP #EXTALBUMARTURL when EnableM3UExternalAlbumArt is false", func() { conf.Server.EnableM3UExternalAlbumArt = false @@ -1011,6 +1029,19 @@ var _ = Describe("Playlists - Import", func() { Expect(pls.ExternalImageURL).To(BeEmpty()) }) + DescribeTable("restricts a local #EXTALBUMARTURL to the owner's libraries", + func(imageURL, expected string) { + ctx = request.WithUser(ctx, model.User{ID: "123", Libraries: model.Libraries{{ID: 1, Path: "/music"}}}) + repo.data = []string{"tests/test.mp3"} + m3u := "#EXTALBUMARTURL:" + imageURL + "\n/music/tests/test.mp3\n" + pls, err := ps.ImportM3U(ctx, strings.NewReader(m3u)) + Expect(err).ToNot(HaveOccurred()) + Expect(pls.ExternalImageURL).To(Equal(expected)) + }, + Entry("accepts a library the owner can access", "file:///music/cover.jpg", filepath.Clean("/music/cover.jpg")), + Entry("ignores a library the owner cannot access", "file:///new/cover.jpg", ""), + ) + // Fullwidth characters (e.g., ＡＢＣＤ) are not handled by SQLite's NOCASE collation, // so we need exact matching for non-ASCII characters. It("matches fullwidth characters exactly (SQLite NOCASE limitation)", func() { diff --git a/core/playlists/parse_m3u.go b/core/playlists/parse_m3u.go index 286f2e420..b9cb154cb 100644 --- a/core/playlists/parse_m3u.go +++ b/core/playlists/parse_m3u.go @@ -14,6 +14,7 @@ import ( "github.com/navidrome/navidrome/conf" "github.com/navidrome/navidrome/log" "github.com/navidrome/navidrome/model" + "github.com/navidrome/navidrome/model/request" "github.com/navidrome/navidrome/utils/slice" "golang.org/x/text/unicode/norm" ) @@ -36,7 +37,8 @@ func (s *playlists) parseM3U(ctx context.Context, pls *model.Playlist, folder *m continue } if after, ok := strings.CutPrefix(line, "#EXTALBUMARTURL:"); ok { - pls.ExternalImageURL = resolveImageURL(after, folder, resolver.matcher) + owner, _ := request.UserFrom(ctx) + pls.ExternalImageURL = resolveImageURL(after, folder, resolver.matcher, owner) continue } // Skip empty lines and extended info @@ -286,7 +288,7 @@ func (r *pathResolver) resolvePaths(ctx context.Context, folder *model.Folder, l // HTTP(S) URLs are stored as-is (gated by EnableM3UExternalAlbumArt). // Local paths (file://, absolute, or relative) are resolved to an absolute path // and validated against known library boundaries via matcher. -func resolveImageURL(value string, folder *model.Folder, matcher *libraryMatcher) string { +func resolveImageURL(value string, folder *model.Folder, matcher *libraryMatcher, owner model.User) string { value = strings.TrimSpace(value) if value == "" { return "" @@ -302,12 +304,13 @@ func resolveImageURL(value string, folder *model.Folder, matcher *libraryMatcher // Resolve to local absolute path localPath, ok := resolveLocalPath(value, folder) - if !ok { + if !ok || !model.IsImageFile(localPath) { return "" } - // Validate path is within a known library - if libID, _ := matcher.findLibraryForPath(localPath); libID == 0 { + lib, ok := matcher.findLibrary(localPath) + // A playlist without a folder (API upload, or CLI import from outside all libraries) may only use the owner's libraries. + if !ok || (folder == nil && !owner.HasLibraryAccess(lib.ID)) { return "" } return localPath diff --git a/tests/init_tests.go b/tests/init_tests.go index 902cf196d..eee4428a8 100644 --- a/tests/init_tests.go +++ b/tests/init_tests.go @@ -8,6 +8,7 @@ import ( "testing" "github.com/navidrome/navidrome/conf" + _ "github.com/navidrome/navidrome/conf/mime" // registers mime_types.yaml, so tests see the same image types as the server "github.com/navidrome/navidrome/log" )