3 Commits

Author	SHA1	Message	Date
Deluan Quintão	237276efcd	fix(artwork): block private and loopback addresses in remote image fetches (#6181) ... * fix(artwork): block private and loopback addresses in remote image fetches fromURL fetched any URL with a plain HTTP client, and two untrusted inputs reach it. A playlist can set #EXTALBUMARTURL to an http(s) URL, which the artwork worker later fetches when EnableM3UExternalAlbumArt is on, so any user who can import a playlist controls the target. Metadata agents, including WASM plugins without the http permission, return image URLs that the core fetches too. Either path could make the server request loopback, LAN or link-local addresses and store the response as artwork that is served back. Add httpclient.NewExternal, which dials through a net.Dialer Control hook that rejects private, loopback, link-local and unspecified addresses. The check runs at dial time on the resolved IP, so DNS names, redirects and DNS rebinding are covered. fromURL now uses one shared client built with it and treats a refused address as a definitive miss, so the item settles absent instead of retrying and tripping the agent's circuit breaker. httpclient.New is unchanged for the other callers. The IP classification moves from plugins to the new utils/netguard package, shared by the plugin host client and the new constructor. The artwork test suite swaps in a client that allows loopback so existing specs can keep using httptest servers; the fromURL specs use the production client to assert the refusal. * fix(httpclient): keep dialing a configured proxy in the guarded client The guard runs on the resolved address, and with HTTP_PROXY set that address is the proxy, not the image host. A proxy on a private address would have had every remote artwork fetch refused, and a refusal settles the item as absent, so covers would silently disappear for those setups. Dial the configured proxy endpoint directly and keep the guard for every other dial. A proxy relays the request itself, so it is the operator's egress policy, the same one every other httpclient.New caller already goes through. * fix(httpclient): exempt only the hop that actually goes through the proxy The exemption matched any dial to a configured proxy's address, but net/http never proxies loopback targets, so a URL aimed at a loopback proxy was dialed directly and skipped the guard. That let an image URL reach that one address. Tag each request with the proxy it resolves to and exempt a dial only when it is that hop. Redirects re-enter the RoundTripper, so every hop is tagged on its own.	2026-09-20 20:46:46 -04:00
Deluan Quintão	8b4125267e	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.	2026-09-20 13:40:14 -04:00
Deluan Quintão	5d1c1157b5	refactor(artwork): migrate readers to storage.MusicFS and add e2e suite (#5379) ... * test(artwork): add e2e suite documenting album/disc resolution Adds core/artwork/e2e/ with a real-tempdir + scanner harness that exercises artwork resolution end-to-end. Covers album and disc kinds; pending (PIt) cases document two known bugs in reader_album.go for regression-guard flipping once they are fixed. * refactor(artwork): add libraryFS helper to resolve MusicFS for a library * test(artwork): tighten libraryFS test isolation and add scheme-error case * test(artwork): update libraryFS test description to match implementation * refactor(artwork): convert fromExternalFile to use fs.FS Add a temporary fromExternalFileAbs shim so existing absolute-path callers still compile; the shim is removed once all readers are migrated. * refactor(artwork): make fromExternalFileAbs a thin delegator Introduce a minimal osDirectFS adapter so the shim no longer duplicates the matching loop. Both will be removed in Task 9. * refactor(artwork): convert fromTag to taglib.OpenStream over fs.FS Add a temporary fromTagAbs shim so existing absolute-path callers still compile; removed in Task 9. Reuses the osDirectFS adapter from Task 2. * refactor(artwork): defer fs.File close until after taglib reads finish Mirror the lifetime pattern used by adapters/gotaglib/gotaglib.go: keep the underlying fs.File open until taglib.File is closed, and pass WithFilename so format detection doesn't rely on content sniffing. * docs(artwork): note ffmpeg's path-based API limitation * refactor(artwork): migrate album reader to MusicFS - Add libFS (storage.MusicFS) field to albumArtworkReader; resolved once at construction time via libraryFS() - Switch fromCoverArtPriority from abs-path shims to FS-based fromTag/fromExternalFile; only fromFFmpegTag retains absolute path - Build imgFiles as library-relative forward-slash paths in loadAlbumFoldersPaths using path.Join(f.Path, f.Name, img) - Guard embedAbs so that an empty EmbedArtPath never produces a non-empty absolute path (prevents accidental ffmpeg invocation) - Register testfile:// storage scheme in artwork test suite to provide an os.DirFS-backed MusicFS without requiring the taglib extractor - Update test assertions from filepath.FromSlash(abs) to bare forward-slash relative strings * fix(artwork): use path package in compareImageFiles for forward-slash relative paths * refactor(artwork): migrate disc reader to MusicFS Replace os.Open absolute-path access with libFS.Open on library-relative forward-slash paths. Rename discFolders→discFoldersRel, split firstTrackPath into firstTrackRelPath (for fromTag) and firstTrackAbsPath (for fromFFmpegTag), and switch path.Dir/Base/Ext for forward-slash safety. * refactor(artwork): build discFoldersRel directly and guard empty first track * refactor(artwork): migrate mediafile reader to MusicFS * refactor(artwork): migrate artist album-art lookup to MusicFS * refactor(artwork): remove temporary path-based shims All readers now use the FS-based fromTag and fromExternalFile directly, so the absolute-path adapters and the osDirectFS helper that backed them can go away. * test(artwork): rewrite e2e suite to use storagetest.FakeFS Switches from real-tempdir + local storage to FakeFS via the storage registry. Adds a proper multi-disc scenario using the disc tag, which previously required curated MP3 fixtures we did not have. * test(artwork): use maps.Copy in trackFile tag merge Lint cleanup: replace the manual map-copy loop flagged by mapsloop. * test(artwork): reuse tests.MockFFmpeg in e2e harness Replace the hand-rolled noopFFmpeg stub with tests.NewMockFFmpeg, which already satisfies the full ffmpeg.FFmpeg interface and won't drift when new methods are added. Also tie imageBytes to imageFile so they cannot silently disagree on the on-disk encoding. * test(artwork): add e2e scenarios from artwork documentation Covers the behaviors documented at https://www.navidrome.org/docs/usage/library/artwork/: - Album: folder.*/front.* fallbacks and priority order with cover.*. - Disc: cd*.* match, cover.* inside disc folder, DiscArtPriority="" skip path, the documented multi-disc layout, and the discsubtitle keyword. - MediaFile: disc-level fallback for multi-disc tracks and album-level fallback for single-disc tracks (doc section "MediaFiles" items 2-3). - Artist: album/artist.* lookup via libFS (passes). The artist-folder branch is XIt-marked because fromArtistFolder still calls os.DirFS directly on an absolute path and can't read from a FakeFS-backed library — migrating that to storage.MusicFS is a follow-up. Signed-off-by: Deluan <deluan@navidrome.org> * refactor(artwork): scope artist folder traversal to library root Route fromArtistFolder reads through storage.MusicFS and bound the parent-directory walk at the library root. This keeps artwork resolution scoped to the configured library and unblocks FakeFS-backed e2e scenarios that depend on the artist folder. Also consolidate the libraryFS + core.AbsolutePath pairing (used by three readers) into a single libraryFSAndRoot helper. * test(artwork): add ASCII file-tree diagrams to e2e scenarios Each It/PIt block now shows the on-disk layout it exercises, with arrows indicating which file wins (or should win, for the known-bug PIt cases). Makes scenarios readable at a glance without having to parse the MapFS map. * test(artwork): add e2e tests for playlist and radio artwork resolution Signed-off-by: Deluan <deluan@navidrome.org> * test(artwork): enhance e2e tests with real MP3 fixtures for embedded artwork Signed-off-by: Deluan <deluan@navidrome.org> * test(ffmpeg): add support for animated WebP encoder detection and fallback handling Signed-off-by: Deluan <deluan@navidrome.org> * test(artwork): cover additional edge cases in e2e suite Add high-value scenarios uncovered by the existing specs: - Album: three-way basename tie (unsuffixed wins), unknown pattern in CoverArtPriority is skipped, embedded-first with no embedded art falls through. - Disc: discsubtitle with no matching image falls through. - Artist: ArtistArtPriority can reach images via album/<pattern>. - Playlist: generates a 2x2 tiled cover from album art when the playlist has no uploaded/sidecar/external image. New helper realPNG() produces real taglib/image-decodable bytes so the tiled-cover test can exercise the generator's decode + compose path. * test(artwork): refactor image upload logic in e2e tests for consistency Signed-off-by: Deluan <deluan@navidrome.org> * test(ffmpeg): simplify animated WebP encoder check by removing context parameter Signed-off-by: Deluan <deluan@navidrome.org> * fix(artwork): normalize rel path for fs.Glob on Windows filepath.Rel returns backslash-separated paths on Windows, but fs.Glob and path.Join require forward slashes. Convert with filepath.ToSlash after computing the relative path and use path.Dir for the parent walk so the artist-folder lookup works cross-platform. * fix(ffmpeg): retry animated WebP probe on transient failure The probe previously used the caller's request context inside sync.Once, so a single cancelled first request would permanently disable animated WebP for the rest of the process. Switch to a mutex + probed flag, use a fresh background context with its own timeout, and only cache the result when the probe actually succeeds. * test(ffmpeg): reset ffOnce so ConvertAnimatedImage test is order-independent The ConvertAnimatedImage stand-in test sets ffmpegPath directly but does not reset ffOnce. If ffmpegCmd() has not been called earlier in the test process, the next call inside hasAnimatedWebPEncoder runs ffOnce.Do and re-resolves the real ffmpeg binary, overwriting the stand-in and breaking the test. Reset ffOnce and conf.Server.FFmpegPath alongside the other globals to pin resolution to the stand-in. * test(artwork): unblock Windows CI — forward-slash fs paths and suite-level DB lifetime The internal artwork test planted a Windows absolute path (backslashes) into Folder.Path and then fed it through libFS.Open, which fs.ValidPath rejects. Rooting the testfile library at the temp dir directly and using filepath.ToSlash keeps the path model library-relative and forward-slash, matching production. The e2e suite opened a per-spec DB in a per-spec TempDir, but the go-sqlite3 singleton kept the file open across specs. Ginkgo's per-spec TempDir cleanup then tried to unlink a file still held by that handle — fine on POSIX, fails on Windows. Moving the DB to a suite-level tempdir and closing it in AfterSuite avoids the race. * test(artwork): keep Windows drive letters intact in testfile library URLs url.Parse on `testfile://C:/path` reads `C` as the host and the path loses the drive letter, so Windows libFS lookups go to `/path` and fail. testFileLibPath now prepends a `/` when the OS path has no leading slash, and the testfile constructor strips that extra slash back off before handing the path to os.Stat / os.DirFS. * refactor(artwork): consolidate libFS + root into libraryView helper Collapses the per-reader libFS/libPath/rootFolder/firstTrackAbsPath fields into a single libraryView{FS, absRoot} with an Abs(rel) method. Also folds the two library lookups (ds.Library.Get + core.AbsolutePath) into one, and uses mf.Path directly instead of stripping libRoot off an absolute path. * refactor(ffmpeg): replace hasAnimatedWebPEncoder with encoderProbe for state management Signed-off-by: Deluan <deluan@navidrome.org> * fix: escape artist folder names in artwork glob Escape glob metacharacters in the library-relative artist folder path before composing the fs.Glob pattern for artist image lookup. This preserves literal folder names such as Artist [Live] while keeping the configured filename pattern behavior unchanged, and adds a regression test for bracketed artist folders. Signed-off-by: Deluan <deluan@navidrome.org> * fix(artwork): correct test path assertions after MusicFS migration Source functions (fromTag, fromExternalFile) now return forward-slash fs.FS-relative paths, so test assertions should compare against plain forward-slash strings, not filepath.FromSlash(). The artistArtPriority test needs filepath.FromSlash() on the suffix because findImageInFolder returns OS-native absolute paths via filepath.Join. * fix(artwork): normalize path separators in artistArtPriority assertion The two table entries exercise different code paths: entry 1 goes through fromArtistFolder (returns OS-native paths via filepath.Join), while entry 2 goes through fromExternalFile (returns forward-slash fs.FS paths). Using filepath.FromSlash on the expected value only works for entry 1. Normalize the actual path to forward slashes with filepath.ToSlash so a single HaveSuffix assertion works for both code paths on all platforms. --------- Signed-off-by: Deluan <deluan@navidrome.org>	2026-04-26 18:16:14 -04:00