diff --git a/.github/workflows/pipeline.yml b/.github/workflows/pipeline.yml index aa8e29e49..2702163d3 100644 --- a/.github/workflows/pipeline.yml +++ b/.github/workflows/pipeline.yml @@ -549,7 +549,7 @@ jobs: env: GH_TOKEN: ${{ github.token }} run: | - for artifact in $(gh api repos/${{ github.repository }}/actions/artifacts | jq -r '.artifacts[] | select(.name | startswith("digests-")) | .id'); do + for artifact in $(gh api repos/${{ github.repository }}/actions/runs/${{ github.run_id }}/artifacts | jq -r '.artifacts[] | select(.name | startswith("digests-")) | .id'); do gh api --method DELETE repos/${{ github.repository }}/actions/artifacts/$artifact done diff --git a/adapters/lastfm/agent.go b/adapters/lastfm/agent.go index 7f005db1a..dbd73c30d 100644 --- a/adapters/lastfm/agent.go +++ b/adapters/lastfm/agent.go @@ -241,6 +241,10 @@ func (l *lastfmAgent) GetSimilarSongsByTrack(ctx context.Context, id, name, arti var ( artistOpenGraphQuery = cascadia.MustCompile(`html > head > meta[property="og:image"]`) artistIgnoredImage = "2a96cbd8b46e442fc41c2b86b821562f" // Last.fm artist placeholder image name + + // Not a RetryLaterError on purpose: parking the agent would also stall its API-backed + // methods, which the page block does not affect. + errNoArtistPage = errors.New("no artist image in Last.fm page") ) func (l *lastfmAgent) GetArtistImages(ctx context.Context, _, name, mbid string) ([]agents.ExternalImage, error) { @@ -267,7 +271,9 @@ func (l *lastfmAgent) GetArtistImages(ctx context.Context, _, name, mbid string) var res []agents.ExternalImage n := cascadia.Query(node, artistOpenGraphQuery) if n == nil { - return res, nil + // A real artist page always has og:image; its absence means a bot challenge or a redesign. + log.Warn(ctx, "Last.fm did not return a usable artist page", "name", name, "url", a.URL) + return nil, errNoArtistPage } for _, attr := range n.Attr { if attr.Key != "content" { diff --git a/adapters/lastfm/agent_test.go b/adapters/lastfm/agent_test.go index ce81e0916..b9fb786c3 100644 --- a/adapters/lastfm/agent_test.go +++ b/adapters/lastfm/agent_test.go @@ -648,18 +648,41 @@ var _ = Describe("lastfmAgent", func() { Expect(images).To(BeEmpty()) }) - It("returns empty list if page has no meta tags", func() { + It("errors when the page has no meta tags", func() { fApi, _ := os.Open("tests/fixtures/lastfm.artist.getinfo.json") apiClient.Res = http.Response{Body: fApi, StatusCode: 200} fScraper, _ := os.Open("tests/fixtures/lastfm.artist.page.no_meta.html") httpClient.Res = http.Response{Body: fScraper, StatusCode: 200} + _, err := agent.GetArtistImages(ctx, "123", "U2", "") + Expect(err).To(MatchError(errNoArtistPage)) + }) + + It("errors when Last.fm serves a bot challenge page", func() { + fApi, _ := os.Open("tests/fixtures/lastfm.artist.getinfo.json") + apiClient.Res = http.Response{Body: fApi, StatusCode: 200} + + fScraper, _ := os.Open("tests/fixtures/lastfm.artist.page.challenge.html") + httpClient.Res = http.Response{Body: fScraper, StatusCode: 200} + images, err := agent.GetArtistImages(ctx, "123", "U2", "") - Expect(err).ToNot(HaveOccurred()) + Expect(err).To(MatchError(errNoArtistPage)) Expect(images).To(BeEmpty()) }) + It("does not park the agent: the failure is not a retry-later", func() { + // A RetryLaterError would cool down the agent's API-backed methods too. + fApi, _ := os.Open("tests/fixtures/lastfm.artist.getinfo.json") + apiClient.Res = http.Response{Body: fApi, StatusCode: 200} + + fScraper, _ := os.Open("tests/fixtures/lastfm.artist.page.challenge.html") + httpClient.Res = http.Response{Body: fScraper, StatusCode: 200} + + _, err := agent.GetArtistImages(ctx, "123", "U2", "") + Expect(errors.Is(err, agents.ErrRetryLater)).To(BeFalse()) + }) + It("returns error if API call fails", func() { apiClient.Err = errors.New("api error") _, err := agent.GetArtistImages(ctx, "123", "U2", "") diff --git a/cmd/root.go b/cmd/root.go index f632b0425..02cd30240 100644 --- a/cmd/root.go +++ b/cmd/root.go @@ -375,10 +375,26 @@ func startPlaybackServer(ctx context.Context) func() error { func startArtworkWorker(ctx context.Context, worker *artwork.Worker) func() error { return func() error { log.Info(ctx, "Starting artwork worker") + // The scanner writes to the DB for its whole run; competing for the write lock makes both fail. + worker.PauseWhile(scanner.IsScanning) return worker.Run(ctx) } } +// outsideScan runs a DB maintenance job unless a scan is running, and keeps a scan from starting +// until it ends; both write to the DB, and competing for the lock can make either fail. +func outsideScan(ctx context.Context, job string, run func(context.Context) error) { + release, ok := scanner.LockForMaintenance() + if !ok { + log.Debug(ctx, "Skipping "+job+" because a scan is in progress") + return + } + defer release() + if err := run(ctx); err != nil { + log.Error(ctx, "Error running "+job, err) + } +} + // scheduleArtworkHousekeeping registers the recurring missing-state and prune jobs, and // reports an artwork config change without acting on it. func scheduleArtworkHousekeeping(ctx context.Context, worker *artwork.Worker) func() error { @@ -386,26 +402,20 @@ func scheduleArtworkHousekeeping(ctx context.Context, worker *artwork.Worker) fu schedulerInstance := scheduler.GetInstance() if _, err := schedulerInstance.Add(consts.ArtworkEnqueueMissingSchedule, func() { - if err := worker.EnqueueMissingAll(ctx); err != nil { - log.Error(ctx, "Error enqueueing missing artwork rechecks", err) - } + outsideScan(ctx, "artwork missing-state recheck", worker.EnqueueMissingAll) }); err != nil { log.Error(ctx, "Error scheduling artwork missing-state recheck", err) } if _, err := schedulerInstance.Add(consts.ArtworkPruneSchedule, func() { - if err := worker.RunPrune(ctx); err != nil { - log.Error(ctx, "Error running artwork prune", err) - } + outsideScan(ctx, "artwork prune", worker.RunPrune) }); err != nil { log.Error(ctx, "Error scheduling artwork prune", err) } // Also run the missing-row recheck once at startup so a never-scanned entity is picked up // immediately, not only on the next hourly tick (e.g. after enabling the feature). - if err := worker.EnqueueMissingAll(ctx); err != nil { - log.Error(ctx, "Error enqueueing missing artwork rechecks", err) - } + outsideScan(ctx, "artwork missing-state recheck", worker.EnqueueMissingAll) if err := worker.ReconcileConfig(ctx); err != nil { log.Error(ctx, "Error checking the artwork config fingerprint", err) diff --git a/contrib/navidrome.service b/contrib/navidrome.service index 5e6cbedce..ef61d2c65 100644 --- a/contrib/navidrome.service +++ b/contrib/navidrome.service @@ -36,7 +36,7 @@ RestrictNamespaces=yes RestrictRealtime=yes SystemCallFilter=@system-service SystemCallFilter=~@privileged @resources -SystemCallFilter=setrlimit +SystemCallFilter=setrlimit mbind SystemCallArchitectures=native UMask=0066 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_suite_test.go b/core/artwork/artwork_suite_test.go index 1ea82b7fa..93aacc1fa 100644 --- a/core/artwork/artwork_suite_test.go +++ b/core/artwork/artwork_suite_test.go @@ -2,18 +2,21 @@ package artwork import ( "io/fs" + "net/netip" "net/url" "os" "path/filepath" "runtime" "strings" "testing" + "time" "github.com/navidrome/navidrome/core/storage" "github.com/navidrome/navidrome/log" "github.com/navidrome/navidrome/model" "github.com/navidrome/navidrome/model/metadata" "github.com/navidrome/navidrome/tests" + "github.com/navidrome/navidrome/utils/httpclient" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" "go.uber.org/goleak" @@ -37,6 +40,14 @@ func TestArtwork(t *testing.T) { RunSpecs(t, "Artwork Suite") } +// productionImageClient keeps the guarded client for the specs that assert it refuses loopback. +var productionImageClient = remoteImageClient + +// httptest servers listen on loopback, which the production client refuses. +var _ = BeforeSuite(func() { + remoteImageClient = httpclient.NewExternal(5*time.Second, netip.MustParsePrefix("127.0.0.0/8"), netip.MustParsePrefix("::1/128")) +}) + // osDirFS wraps os.DirFS as a storage.MusicFS for integration tests. type osDirFS struct{ fs.FS } 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/e2e/e2e_suite_test.go b/core/artwork/e2e/e2e_suite_test.go index 8881da3a6..390be14a7 100644 --- a/core/artwork/e2e/e2e_suite_test.go +++ b/core/artwork/e2e/e2e_suite_test.go @@ -56,7 +56,8 @@ func runWorkerUntil(ctx context.Context, worker *artwork.Worker, until func() bo runCtx, cancel := context.WithCancel(ctx) done := make(chan error, 1) go func() { done <- worker.Run(runCtx) }() - Eventually(until, 5*time.Second, 10*time.Millisecond).Should(BeTrue()) + // Long enough for one retry (3-7s backoff, 5s poll tick). + Eventually(until, 15*time.Second, 10*time.Millisecond).Should(BeTrue()) cancel() Eventually(done, 2*time.Second).Should(Receive(BeNil())) } diff --git a/core/artwork/image_store.go b/core/artwork/image_store.go index 5dbe727e4..2ded3e636 100644 --- a/core/artwork/image_store.go +++ b/core/artwork/image_store.go @@ -98,6 +98,9 @@ func (s *ImageStore) Write(hash, mimeType string, r io.Reader) error { if err := tmp.Close(); err != nil { return err } + if err := os.Chmod(tmp.Name(), 0640); err != nil { + return err + } return os.Rename(tmp.Name(), dst) } diff --git a/core/artwork/image_store_test.go b/core/artwork/image_store_test.go index f7b2467ca..7de3ba761 100644 --- a/core/artwork/image_store_test.go +++ b/core/artwork/image_store_test.go @@ -48,6 +48,17 @@ var _ = Describe("ImageStore", func() { Expect(got).To(Equal(data)) }) + It("writes group-readable image files", func() { + tests.SkipOnWindows("uses Unix file permission bits") + data := []byte("jpeg-bytes") + h, _ := hashImage(bytes.NewReader(data)) + Expect(store.Write(h, "image/jpeg", bytes.NewReader(data))).To(Succeed()) + + info, err := os.Stat(store.path(h, "image/jpeg")) + Expect(err).ToNot(HaveOccurred()) + Expect(info.Mode().Perm()).To(Equal(os.FileMode(0640))) + }) + It("is idempotent on duplicate writes and preserves the original content", func() { data := []byte("dup") h, _ := hashImage(bytes.NewReader(data)) 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..daa084c16 100644 --- a/core/artwork/sources.go +++ b/core/artwork/sources.go @@ -18,6 +18,7 @@ import ( "github.com/navidrome/navidrome/log" "github.com/navidrome/navidrome/model" "github.com/navidrome/navidrome/utils/httpclient" + "github.com/navidrome/navidrome/utils/netguard" "go.senan.xyz/taglib" ) @@ -37,7 +38,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) @@ -150,10 +151,18 @@ type readCloser struct { io.Closer } +// remoteImageClient fetches URLs from playlists and agents (plugins included), so it must not reach +// internal hosts. Shared so fetches reuse connections. +var remoteImageClient = httpclient.NewExternal(5 * time.Second) + func fromURL(ctx context.Context, imageUrl *url.URL) (io.ReadCloser, string, error) { - hc := httpclient.New(5 * time.Second) req, _ := http.NewRequestWithContext(ctx, http.MethodGet, imageUrl.String(), nil) - resp, err := hc.Do(req) //nolint:gosec + resp, err := remoteImageClient.Do(req) + if errors.Is(err, netguard.ErrPrivateAddress) { + // Retrying cannot change where the URL points: settle absent instead of tripping the breaker. + log.Warn(ctx, "Artwork: Refused to fetch image from a private or loopback address", "url", imageUrl, err) + return nil, "", model.ErrNotFound + } if err != nil { return nil, "", err } diff --git a/core/artwork/sources_internal_test.go b/core/artwork/sources_internal_test.go index 4282575a5..bc81b56e7 100644 --- a/core/artwork/sources_internal_test.go +++ b/core/artwork/sources_internal_test.go @@ -5,13 +5,87 @@ import ( "errors" "io" "io/fs" + "net/http" + "net/http/httptest" + "net/netip" + "net/url" "os" + "strings" + "sync/atomic" "testing/fstest" + "time" + "github.com/navidrome/navidrome/model" + "github.com/navidrome/navidrome/utils/httpclient" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" ) +var _ = Describe("fromURL", func() { + var ( + hits atomic.Int32 + target *httptest.Server + ) + + BeforeEach(func() { + hits.Store(0) + target = httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + hits.Add(1) + _, _ = w.Write([]byte("image-bytes")) + })) + DeferCleanup(target.Close) + }) + + useClient := func(c *http.Client) { + prev := remoteImageClient + remoteImageClient = c + DeferCleanup(func() { remoteImageClient = prev }) + } + fetch := func(rawURL string) ([]byte, error) { + u, err := url.Parse(rawURL) + Expect(err).ToNot(HaveOccurred()) + r, _, err := fromURL(GinkgoT().Context(), u) + if err != nil { + return nil, err + } + defer r.Close() + return io.ReadAll(r) + } + // Stand-in for a public host: only 127.0.0.1 is allowed, so every other private address stays refused. + onlyLocalhostV4 := func() *http.Client { + return httpclient.NewExternal(5*time.Second, netip.MustParsePrefix("127.0.0.1/32")) + } + + DescribeTable("refuses private and loopback targets as a definitive miss", + func(rawURL string) { + useClient(productionImageClient) + u, _ := url.Parse(target.URL) + _, err := fetch(strings.ReplaceAll(rawURL, "PORT", u.Port())) + Expect(err).To(MatchError(model.ErrNotFound)) + Expect(hits.Load()).To(BeZero()) + }, + Entry("IPv4 loopback", "http://127.0.0.1:PORT/x"), + Entry("localhost", "http://localhost:PORT/x"), + Entry("cloud metadata", "http://169.254.169.254/"), + Entry("IPv6 loopback", "http://[::1]/"), + ) + + It("refuses a redirect from an allowed host to a loopback address", func() { + useClient(onlyLocalhostV4()) + redirector := httptest.NewServer(http.RedirectHandler(strings.Replace(target.URL, "127.0.0.1", "127.0.0.2", 1), http.StatusFound)) + DeferCleanup(redirector.Close) + + _, err := fetch(redirector.URL) + Expect(err).To(MatchError(model.ErrNotFound)) + Expect(hits.Load()).To(BeZero()) + }) + + It("fetches from an allowed address", func() { + useClient(onlyLocalhostV4()) + Expect(fetch(target.URL + "/cover.jpg")).To(Equal([]byte("image-bytes"))) + }) +}) + var _ = Describe("fromExternalFile", func() { It("opens a matching file via the library FS", func() { fsys := fstest.MapFS{ @@ -48,6 +122,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/artwork/worker.go b/core/artwork/worker.go index bb09be55e..e8fb3a11c 100644 --- a/core/artwork/worker.go +++ b/core/artwork/worker.go @@ -46,6 +46,7 @@ type Worker struct { pruneMu sync.RWMutex pools []*drainPool runCtx context.Context + paused func() bool gatesMu sync.Mutex gates map[string]*extGate @@ -59,6 +60,7 @@ func NewWorker(ds model.DataStore, store *ImageStore, ag *agents.Agents, ffmpeg broker: broker, pools: newDrainPools(), runCtx: context.Background(), + paused: func() bool { return false }, gates: map[string]*extGate{}, } w.proc.resolver = newResolver(ds, ag, ffmpeg, w.gate) @@ -90,6 +92,11 @@ var ( } ) +// PauseWhile holds off queue draining whenever paused reports true. Call it before Run. +func (w *Worker) PauseWhile(paused func() bool) { + w.paused = paused +} + // Run blocks draining the queue until ctx is cancelled. func (w *Worker) Run(ctx context.Context) error { w.runCtx = ctx @@ -143,6 +150,9 @@ func (w *Worker) EnqueueMissingAll(ctx context.Context) error { } func (w *Worker) drain(ctx context.Context, concurrency int, kinds ...string) (int, error) { + if w.paused() { + return 0, nil + } // Dequeue well past the pool size so a slow external lookup never idles the other slots. // DequeueBatch does not mark rows taken, so this is one query per pass, not per slot. items, err := w.proc.ds.ArtworkQueue(ctx).DequeueBatch(max(16, 4*concurrency), kinds...) @@ -170,6 +180,9 @@ func (w *Worker) drain(ctx context.Context, concurrency int, kinds ...string) (i wg.Wait() return len(items), nil //nolint:nilerr // a cancelled drain is a clean stop, not an error } + if w.paused() { + break + } wg.Go(func() { defer func() { <-sem }() out, got := w.process(ctx, item) diff --git a/core/artwork/worker_test.go b/core/artwork/worker_test.go index ebb8de251..d74e3a5ed 100644 --- a/core/artwork/worker_test.go +++ b/core/artwork/worker_test.go @@ -866,6 +866,34 @@ var _ = Describe("Worker", func() { } }) + It("stops dispatching and leaves the rest queued when paused mid-batch", func() { + folderRepo.result = []model.Folder{{ + Path: "tests/fixtures/artist/an-album", + ImageFiles: []string{"cover.jpg"}, + }} + albums := model.Albums{} + for i := range 8 { + id := fmt.Sprintf("alp%d", i) + albums = append(albums, model.Album{ID: id, Name: "Album", FolderIDs: []string{"f1"}}) + Expect(queueRepo.Enqueue(model.ArtworkQueueItem{ + ItemKind: "al", ItemID: id, Priority: model.ArtworkPriorityScan, + })).To(Succeed()) + } + ds.MockedAlbum.(*tests.MockAlbumRepo).SetData(albums) + // Pauses as soon as the first item has left the queue. + w.PauseWhile(func() bool { + n, _ := queueRepo.Count() + return n < 8 + }) + + _, err := w.drain(ctx, 1) + Expect(err).ToNot(HaveOccurred()) + + count, err := queueRepo.Count() + Expect(err).ToNot(HaveOccurred()) + Expect(count).To(Equal(int64(7)), "only the item dispatched before the pause may leave the queue") + }) + It("dequeues past the worker pool so one drain covers many items", func() { for i := range 16 { ds.MockedAlbum.(*tests.MockAlbumRepo).SetData(model.Albums{{ID: fmt.Sprintf("alb%d", i), Name: "Album"}}) @@ -896,6 +924,30 @@ var _ = Describe("Worker", func() { Eventually(done, time.Second).Should(Receive(BeNil())) }) + It("does not drain the queue while paused", func() { + folderRepo.result = []model.Folder{{ + Path: "tests/fixtures/artist/an-album", + ImageFiles: []string{"cover.jpg"}, + }} + ds.MockedAlbum.(*tests.MockAlbumRepo).SetData(model.Albums{ + {ID: "al1", Name: "Album", FolderIDs: []string{"f1"}}, + }) + Expect(queueRepo.Enqueue(model.ArtworkQueueItem{ + ItemKind: "al", ItemID: "al1", Priority: model.ArtworkPriorityScan, + })).To(Succeed()) + w.PauseWhile(func() bool { return true }) + + runCtx, cancel := context.WithCancel(ctx) + done := make(chan error, 1) + go func() { done <- w.Run(runCtx) }() + DeferCleanup(func() { + cancel() + Eventually(done, 2*time.Second).Should(Receive(BeNil())) + }) + + Consistently(func() any { return findQueued(queueRepo, "al", "al1") }, 300*time.Millisecond).ShouldNot(BeNil()) + }) + It("does not leak goroutines after Run exits", func() { DeferCleanup(configtest.SetupConfig()) diff --git a/core/players.go b/core/players.go index 963914514..b757f8460 100644 --- a/core/players.go +++ b/core/players.go @@ -38,7 +38,7 @@ func (p *players) Register(ctx context.Context, playerID, client, userAgent, ip user, _ := request.UserFrom(ctx) if playerID != "" { plr, err = p.ds.Player(ctx).Get(playerID) - if err == nil && plr.Client != client { + if err == nil && (plr.Client != client || plr.UserId != user.ID) { playerID = "" } } diff --git a/core/players_test.go b/core/players_test.go index 90c265fcc..55ec16833 100644 --- a/core/players_test.go +++ b/core/players_test.go @@ -61,8 +61,19 @@ var _ = Describe("Players", func() { Expect(trc).To(BeNil()) }) + It("does not reuse another user's player by ID", func() { + plr := &model.Player{ID: "123", Name: "A Player", Client: "client", UserId: "otheruser", UserAgent: "Pixel", TranscodingId: "1"} + repo.add(plr) + p, trc, err := players.Register(ctx, "123", "client", "chrome", "1.2.3.4") + Expect(err).ToNot(HaveOccurred()) + Expect(p.ID).ToNot(Equal("123")) + Expect(p.UserId).To(Equal("userid")) + Expect(repo.lastSaved).To(Equal(p)) + Expect(trc).To(BeNil()) + }) + It("finds players by ID", func() { - plr := &model.Player{ID: "123", Name: "A Player", Client: "client", LastSeen: time.Time{}} + plr := &model.Player{ID: "123", Name: "A Player", Client: "client", UserId: "userid", LastSeen: time.Time{}} repo.add(plr) p, trc, err := players.Register(ctx, "123", "client", "chrome", "1.2.3.4") Expect(err).ToNot(HaveOccurred()) @@ -93,7 +104,7 @@ var _ = Describe("Players", func() { }) It("finds player by ID and return its transcoding", func() { - plr := &model.Player{ID: "123", Name: "A Player", Client: "client", LastSeen: time.Time{}, TranscodingId: "1"} + plr := &model.Player{ID: "123", Name: "A Player", Client: "client", UserId: "userid", LastSeen: time.Time{}, TranscodingId: "1"} repo.add(plr) p, trc, err := players.Register(ctx, "123", "client", "chrome", "1.2.3.4") Expect(err).ToNot(HaveOccurred()) 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/core/scrobbler/play_tracker.go b/core/scrobbler/play_tracker.go index 63397f8f6..e68fcd942 100644 --- a/core/scrobbler/play_tracker.go +++ b/core/scrobbler/play_tracker.go @@ -17,6 +17,7 @@ import ( "github.com/navidrome/navidrome/server/events" "github.com/navidrome/navidrome/utils/cache" "github.com/navidrome/navidrome/utils/singleton" + "github.com/navidrome/navidrome/utils/slice" ) const ( @@ -444,8 +445,14 @@ func (p *playTracker) ReportPlayback(ctx context.Context, params ReportPlaybackP return nil } -func (p *playTracker) GetNowPlaying(_ context.Context) ([]PlaybackSession, error) { +func (p *playTracker) GetNowPlaying(ctx context.Context) ([]PlaybackSession, error) { + // The cache is process-global, so it holds every user's playback, across all libraries. res := p.playMap.Values() + if user, ok := request.UserFrom(ctx); ok { + res = slice.Filter(res, func(s PlaybackSession) bool { + return user.HasLibraryAccess(s.MediaFile.LibraryID) + }) + } slices.SortFunc(res, func(a, b PlaybackSession) int { return b.Start.Compare(a.Start) }) diff --git a/core/scrobbler/play_tracker_test.go b/core/scrobbler/play_tracker_test.go index 0e768c3f4..d79233e15 100644 --- a/core/scrobbler/play_tracker_test.go +++ b/core/scrobbler/play_tracker_test.go @@ -92,7 +92,7 @@ var _ = Describe("PlayTracker", func() { BeforeEach(func() { DeferCleanup(configtest.SetupConfig()) ctx = GinkgoT().Context() - ctx = request.WithUser(ctx, model.User{ID: "u-1"}) + ctx = request.WithUser(ctx, model.User{ID: "u-1", Libraries: model.Libraries{{ID: 1}}}) ctx = request.WithPlayer(ctx, model.Player{ScrobbleEnabled: true}) ds = &tests.MockDataStore{} fake = &fakeScrobbler{Authorized: true} @@ -108,6 +108,7 @@ var _ = Describe("PlayTracker", func() { track = model.MediaFile{ ID: "123", + LibraryID: 1, Title: "Track Title", Album: "Track Album", AlbumID: "al-1", @@ -174,6 +175,46 @@ var _ = Describe("PlayTracker", func() { Expect(playing[1].Username).To(Equal("user-1")) Expect(playing[1].MediaFile.ID).To(Equal("123")) }) + + It("hides sessions playing from libraries the caller cannot access", func() { + hidden := track + hidden.ID = "789" + hidden.LibraryID = 2 + _ = ds.MediaFile(ctx).Put(&hidden) + reporter := request.WithPlayer( + request.WithUser(GinkgoT().Context(), model.User{ID: "u-2", UserName: "user-2"}), + model.Player{ScrobbleEnabled: true}, + ) + _ = tracker.ReportPlayback(reporter, ReportPlaybackParams{ + MediaId: "789", PositionMs: 0, State: StatePlaying, PlaybackRate: 1.0, ClientId: "player-2", ClientName: "player-two", + }) + + playing, err := tracker.GetNowPlaying(ctx) + + Expect(err).ToNot(HaveOccurred()) + Expect(playing).To(BeEmpty(), "u-1 is granted library 1 only") + }) + + It("shows every session to an admin", func() { + hidden := track + hidden.ID = "789" + hidden.LibraryID = 2 + _ = ds.MediaFile(ctx).Put(&hidden) + reporter := request.WithPlayer( + request.WithUser(GinkgoT().Context(), model.User{ID: "u-2", UserName: "user-2"}), + model.Player{ScrobbleEnabled: true}, + ) + _ = tracker.ReportPlayback(reporter, ReportPlaybackParams{ + MediaId: "789", PositionMs: 0, State: StatePlaying, PlaybackRate: 1.0, ClientId: "player-2", ClientName: "player-two", + }) + + adminCtx := request.WithUser(GinkgoT().Context(), model.User{ID: "adm", IsAdmin: true}) + playing, err := tracker.GetNowPlaying(adminCtx) + + Expect(err).ToNot(HaveOccurred()) + Expect(playing).To(HaveLen(1)) + Expect(playing[0].MediaFile.ID).To(Equal("789")) + }) }) Describe("Expiration events", func() { diff --git a/db/db.go b/db/db.go index 685886edb..a9c6c4a15 100644 --- a/db/db.go +++ b/db/db.go @@ -133,6 +133,12 @@ func ErrorCodes(err error) (code, extended int, ok bool) { return int(se.Code), int(se.ExtendedCode), true } +// IsBusy reports whether err is SQLITE_BUSY, including BUSY_SNAPSHOT, which only a new transaction clears. +func IsBusy(err error) bool { + code, _, ok := ErrorCodes(err) + return ok && code == int(sqlite3.ErrBusy) +} + type statusLogger struct{ numPending int } func (*statusLogger) Fatalf(format string, v ...any) { log.Fatal(fmt.Sprintf(format, v...)) } diff --git a/db/db_test.go b/db/db_test.go index 2ce01dc3d..e3a52e1fb 100644 --- a/db/db_test.go +++ b/db/db_test.go @@ -3,8 +3,12 @@ package db_test import ( "context" "database/sql" + "errors" + "fmt" "testing" + "github.com/mattn/go-sqlite3" + "github.com/navidrome/navidrome/db" "github.com/navidrome/navidrome/log" "github.com/navidrome/navidrome/tests" @@ -19,6 +23,18 @@ func TestDB(t *testing.T) { RunSpecs(t, "DB Suite") } +var _ = DescribeTable("IsBusy", + func(err error, expected bool) { + Expect(db.IsBusy(err)).To(Equal(expected)) + }, + Entry("SQLITE_BUSY", sqlite3.Error{Code: sqlite3.ErrBusy}, true), + Entry("SQLITE_BUSY_SNAPSHOT", sqlite3.Error{Code: sqlite3.ErrBusy, ExtendedCode: sqlite3.ErrBusySnapshot}, true), + Entry("a wrapped SQLITE_BUSY", fmt.Errorf("persisting: %w", sqlite3.Error{Code: sqlite3.ErrBusy}), true), + Entry("another SQLite error", sqlite3.Error{Code: sqlite3.ErrConstraint}, false), + Entry("a non-SQLite error", errors.New("database is locked"), false), + Entry("nil", nil, false), +) + var _ = Describe("IsSchemaEmpty", func() { var database *sql.DB var ctx context.Context diff --git a/db/migrations/20260922230428_clamp_media_file_int32_tag_numbers.sql b/db/migrations/20260922230428_clamp_media_file_int32_tag_numbers.sql new file mode 100644 index 000000000..dced5d744 --- /dev/null +++ b/db/migrations/20260922230428_clamp_media_file_int32_tag_numbers.sql @@ -0,0 +1,24 @@ +-- +goose Up +-- +goose StatementBegin +-- 32-bit builds cannot read values above the int32 range written by 64-bit builds. +update media_file set track_number = 0 +where track_number < 0 or track_number > 2147483647; + +update media_file set disc_number = 0 +where disc_number < 0 or disc_number > 2147483647; + +update media_file set bpm = null +where bpm < 0 or bpm > 2147483647; + +update album set discs = ( + select json_group_object(key, value) from json_each(album.discs) + where cast(key as integer) between 0 and 2147483647 +) +where json_valid(discs) and exists ( + select 1 from json_each(album.discs) + where cast(key as integer) not between 0 and 2147483647 +); +-- +goose StatementEnd + +-- +goose Down +SELECT 1; diff --git a/db/optimize.go b/db/optimize.go index f46906c4e..aff36e8fd 100644 --- a/db/optimize.go +++ b/db/optimize.go @@ -6,6 +6,7 @@ import ( "errors" "fmt" "strconv" + "strings" "sync" "time" @@ -134,16 +135,65 @@ func optimizeAt(ctx context.Context, db *sql.DB, now time.Time) error { return recordAnalyzeError(ctx, db, now, fmt.Errorf("marking ANALYZE pending: %w", err)) } log.Debug(ctx, "Refreshing query planner statistics") - _, err := db.ExecContext(ctx, "ANALYZE") - if err != nil { + if err := analyzeInSteps(ctx, db); err != nil { return recordAnalyzeError(ctx, db, now, fmt.Errorf("running ANALYZE: %w", err)) } - if err = recordAnalyzeSuccess(ctx, db, now); err != nil { + if err := recordAnalyzeSuccess(ctx, db, now); err != nil { return recordAnalyzeError(ctx, db, now, err) } return nil } +// One ANALYZE per index (whole table if WITHOUT ROWID or lacking a non-partial index) yields the +// same sqlite_stat1 rows as a full ANALYZE, but frees the write lock between steps. +const analyzeTargetsSQL = ` +SELECT i.name FROM sqlite_schema i JOIN pragma_table_list t ON t.schema = 'main' AND t.name = i.tbl_name +WHERE i.type = 'index' AND t.wr = 0 AND EXISTS (SELECT 1 FROM pragma_index_list(t.name) l WHERE l.partial = 0) +UNION ALL +SELECT t.name FROM pragma_table_list t +WHERE t.schema = 'main' AND t.type IN ('table', 'shadow') AND t.name NOT LIKE 'sqlite_%' + AND (t.wr = 1 OR NOT EXISTS (SELECT 1 FROM pragma_index_list(t.name) l WHERE l.partial = 0))` + +// analyzeMaxYield is just above SQLite's longest busy-handler sleep, so every waiting writer +// retries during the pause. +const analyzeMaxYield = 150 * time.Millisecond + +func analyzeInSteps(ctx context.Context, db *sql.DB) error { + targets, err := analyzeTargets(ctx, db) + if err != nil { + return err + } + for _, target := range targets { + start := time.Now() + if _, err := db.ExecContext(ctx, `ANALYZE "`+strings.ReplaceAll(target, `"`, `""`)+`"`); err != nil { + return fmt.Errorf("analyzing %s: %w", target, err) + } + select { + case <-ctx.Done(): + return ctx.Err() + case <-time.After(min(time.Since(start), analyzeMaxYield)): + } + } + return nil +} + +func analyzeTargets(ctx context.Context, db *sql.DB) ([]string, error) { + rows, err := db.QueryContext(ctx, analyzeTargetsSQL) + if err != nil { + return nil, fmt.Errorf("listing ANALYZE targets: %w", err) + } + defer rows.Close() + var targets []string + for rows.Next() { + var name string + if err := rows.Scan(&name); err != nil { + return nil, fmt.Errorf("listing ANALYZE targets: %w", err) + } + targets = append(targets, name) + } + return targets, rows.Err() +} + func recordAnalyzeSuccess(ctx context.Context, db *sql.DB, now time.Time) error { tx, err := db.BeginTx(ctx, nil) if err != nil { diff --git a/db/optimize_test.go b/db/optimize_test.go index da9b3b9c9..6ad9b9cb4 100644 --- a/db/optimize_test.go +++ b/db/optimize_test.go @@ -75,6 +75,43 @@ var _ = Describe("Optimize", func() { Expect(getProperty(consts.DBAnalyzePendingKey)).To(Equal("0")) }) + It("produces the same statistics as a single full ANALYZE", func() { + putProperty(consts.DBAnalyzePendingKey, "1") + for _, stmt := range []string{ + "create table unindexed(id integer primary key, v int)", + "insert into unindexed(v) select flag from analyze_probe", + "create table no_rowid(k text primary key, v int) without rowid", + "insert into no_rowid select 'k' || id, id % 7 from analyze_probe", + "create index no_rowid_v on no_rowid(v)", + "create table partial_only(id integer primary key, v int)", + "insert into partial_only(v) select id % 5 from analyze_probe", + "create index partial_only_v on partial_only(v) where v = 1", + "analyze", + } { + _, err := database.Exec(stmt) + Expect(err).ToNot(HaveOccurred()) + } + statRows := func() []string { + rows, err := database.Query("select tbl || '|' || coalesce(idx, '') || '|' || stat from sqlite_stat1 order by 1") + Expect(err).ToNot(HaveOccurred()) + defer rows.Close() + var res []string + for rows.Next() { + var s string + Expect(rows.Scan(&s)).To(Succeed()) + res = append(res, s) + } + return res + } + fullAnalyze := statRows() + _, err := database.Exec("delete from sqlite_stat1") + Expect(err).ToNot(HaveOccurred()) + + Expect(db.OptimizeDBAt(ctx, database, now)).To(Succeed()) + + Expect(statRows()).To(Equal(fullAnalyze)) + }) + It("runs when no previous analysis was recorded", func() { ran, err := db.OptimizeDBIfNeeded(ctx, database, now) Expect(err).ToNot(HaveOccurred()) diff --git a/model/criteria/criteria.go b/model/criteria/criteria.go index 5d7dc3826..e6db3b481 100644 --- a/model/criteria/criteria.go +++ b/model/criteria/criteria.go @@ -64,18 +64,21 @@ func (c Criteria) IsPercentageLimit() bool { } func (c Criteria) ChildPlaylistIds() []string { - if c.Expression == nil { - return nil - } + return c.childPlaylistRefs(conjunction.ChildPlaylistIds) +} +func (c Criteria) ChildPlaylistPaths() []string { + return c.childPlaylistRefs(conjunction.ChildPlaylistPaths) +} + +func (c Criteria) childPlaylistRefs(extract func(conjunction) []string) []string { parent, ok := c.Expression.(conjunction) if !ok { return nil } - - ids := parent.ChildPlaylistIds() - slices.Sort(ids) - return slices.Compact(ids) + refs := extract(parent) + slices.Sort(refs) + return slices.Compact(refs) } func (c Criteria) MarshalJSON() ([]byte, error) { diff --git a/model/criteria/criteria_test.go b/model/criteria/criteria_test.go index 5e653150a..de5124568 100644 --- a/model/criteria/criteria_test.go +++ b/model/criteria/criteria_test.go @@ -323,19 +323,23 @@ var _ = Describe("Criteria", func() { Context("with child playlists", func() { var ( - topLevelInPlaylistID string - topLevelNotInPlaylistID string - nestedAnyInPlaylistID string - nestedAnyNotInPlaylistID string - nestedAllInPlaylistID string - nestedAllNotInPlaylistID string + topLevelInPlaylistID string + topLevelInPlaylistPath string + topLevelNotInPlaylistID string + nestedAnyInPlaylistID string + nestedAnyNotInPlaylistID string + nestedAllInPlaylistID string + nestedAllNotInPlaylistID string + nestedAnyNotInPlaylistPath string ) BeforeEach(func() { topLevelInPlaylistID = uuid.NewString() + topLevelInPlaylistPath = "./test.nsp" topLevelNotInPlaylistID = uuid.NewString() nestedAnyInPlaylistID = uuid.NewString() nestedAnyNotInPlaylistID = uuid.NewString() + nestedAnyNotInPlaylistPath = "../not-in-playlist.m3u" nestedAllInPlaylistID = uuid.NewString() nestedAllNotInPlaylistID = uuid.NewString() @@ -343,10 +347,12 @@ var _ = Describe("Criteria", func() { goObj = Criteria{ Expression: All{ InPlaylist{"id": topLevelInPlaylistID}, + InPlaylist{"path": topLevelInPlaylistPath}, NotInPlaylist{"id": topLevelNotInPlaylistID}, Any{ InPlaylist{"id": nestedAnyInPlaylistID}, NotInPlaylist{"id": nestedAnyNotInPlaylistID}, + NotInPlaylist{"path": nestedAnyNotInPlaylistPath}, }, All{ InPlaylist{"id": nestedAllInPlaylistID}, @@ -359,6 +365,18 @@ var _ = Describe("Criteria", func() { ids := goObj.ChildPlaylistIds() gomega.Expect(ids).To(gomega.ConsistOf(topLevelInPlaylistID, topLevelNotInPlaylistID, nestedAnyInPlaylistID, nestedAnyNotInPlaylistID, nestedAllInPlaylistID, nestedAllNotInPlaylistID)) }) + It("extracts all child smart playlist paths from expression criteria", func() { + paths := goObj.ChildPlaylistPaths() + gomega.Expect(paths).To(gomega.ConsistOf(topLevelInPlaylistPath, nestedAnyNotInPlaylistPath)) + }) + It("ignores empty child playlist paths", func() { + c := Criteria{Expression: All{InPlaylist{"path": ""}, NotInPlaylist{"path": ""}}} + gomega.Expect(c.ChildPlaylistPaths()).To(gomega.BeEmpty()) + }) + It("ignores empty child playlist ids", func() { + c := Criteria{Expression: All{InPlaylist{"id": ""}, NotInPlaylist{"id": ""}}} + gomega.Expect(c.ChildPlaylistIds()).To(gomega.BeEmpty()) + }) It("extracts child smart playlist IDs from deeply nested expression", func() { goObj = Criteria{ Expression: Any{ diff --git a/model/criteria/operators.go b/model/criteria/operators.go index 14a02ff4b..ec32f2d16 100644 --- a/model/criteria/operators.go +++ b/model/criteria/operators.go @@ -1,8 +1,9 @@ package criteria -// Conjunctions need to implement this interface, to allow Criteria to extract child playlist IDs recursively +// Conjunctions need to implement this interface, to allow Criteria to extract child playlist references recursively type conjunction interface { ChildPlaylistIds() []string + ChildPlaylistPaths() []string } type ( @@ -16,9 +17,9 @@ func (all All) MarshalJSON() ([]byte, error) { return marshalConjunction("all", all) } -func (all All) ChildPlaylistIds() (ids []string) { - return extractPlaylistIds(all) -} +func (all All) ChildPlaylistIds() []string { return extractPlaylistField(all, "id") } + +func (all All) ChildPlaylistPaths() []string { return extractPlaylistField(all, "path") } type ( Any []Expression @@ -31,9 +32,9 @@ func (any Any) MarshalJSON() ([]byte, error) { return marshalConjunction("any", any) } -func (any Any) ChildPlaylistIds() (ids []string) { - return extractPlaylistIds(any) -} +func (any Any) ChildPlaylistIds() []string { return extractPlaylistField(any, "id") } + +func (any Any) ChildPlaylistPaths() []string { return extractPlaylistField(any, "path") } type Is map[string]any type Eq = Is @@ -172,28 +173,20 @@ func (ip IsPresent) MarshalJSON() ([]byte, error) { func (ip IsPresent) fields() map[string]any { return ip } -func extractPlaylistIds(inputRule any) (ids []string) { - var id string - var ok bool - +func extractPlaylistField(inputRule any, field string) (values []string) { switch rule := inputRule.(type) { case Any: for _, rules := range rule { - ids = append(ids, extractPlaylistIds(rules)...) + values = append(values, extractPlaylistField(rules, field)...) } case All: for _, rules := range rule { - ids = append(ids, extractPlaylistIds(rules)...) + values = append(values, extractPlaylistField(rules, field)...) } - case InPlaylist: - if id, ok = rule["id"].(string); ok { - ids = append(ids, id) - } - case NotInPlaylist: - if id, ok = rule["id"].(string); ok { - ids = append(ids, id) + case InPlaylist, NotInPlaylist: + if value, ok := rule.(Expression).fields()[field].(string); ok && value != "" { + values = append(values, value) } } - return } diff --git a/model/datastore.go b/model/datastore.go index 273ca714b..26687d5d4 100644 --- a/model/datastore.go +++ b/model/datastore.go @@ -47,5 +47,8 @@ type DataStore interface { WithTx(block func(tx DataStore) error, scope ...string) error WithTxImmediate(block func(tx DataStore) error, scope ...string) error + // WithTxRetry runs block in a transaction, rerunning it while SQLite reports the database busy. + // For background work only (it can take minutes), and block must be safe to rerun after a rollback. + WithTxRetry(ctx context.Context, block func(ctx context.Context, tx DataStore) error, scope ...string) error GC(ctx context.Context, libraryIDs ...int) error } diff --git a/model/metadata/map_mediafile.go b/model/metadata/map_mediafile.go index b3ce4ef02..6d12feba9 100644 --- a/model/metadata/map_mediafile.go +++ b/model/metadata/map_mediafile.go @@ -37,8 +37,8 @@ func (md Metadata) ToMediaFile(libID int, folderID string) model.MediaFile { mf.CatalogNum = md.String(model.TagCatalogNumber) mf.Comment = md.String(model.TagComment) if f := md.NullableFloat(model.TagBPM); f != nil { - if v := int(math.Round(*f)); v != 0 { - mf.BPM = new(v) + if r := math.Round(*f); r > 0 && r <= math.MaxInt32 { + mf.BPM = new(int(r)) } } mf.Lyrics = md.mapLyrics() diff --git a/model/metadata/map_mediafile_test.go b/model/metadata/map_mediafile_test.go index 75a7ed358..baaf8fab5 100644 --- a/model/metadata/map_mediafile_test.go +++ b/model/metadata/map_mediafile_test.go @@ -131,6 +131,15 @@ var _ = Describe("ToMediaFile", func() { Expect(toMediaFile(model.RawTags{"BPM": {"0"}}).BPM).To(BeNil()) Expect(toMediaFile(model.RawTags{"BPM": {"fast"}}).BPM).To(BeNil()) }) + It("leaves BPM nil when the tag does not fit in 32 bits", func() { + Expect(toMediaFile(model.RawTags{"BPM": {"4294967295"}}).BPM).To(BeNil()) + }) + It("leaves BPM nil when the tag is negative", func() { + Expect(toMediaFile(model.RawTags{"BPM": {"-120"}}).BPM).To(BeNil()) + }) + It("keeps the largest 32-bit BPM value", func() { + Expect(toMediaFile(model.RawTags{"BPM": {"2147483647"}}).BPM).To(Equal(new(2147483647))) + }) }) Describe("BitDepth", func() { diff --git a/model/metadata/metadata.go b/model/metadata/metadata.go index 0efbe94ec..7843e7010 100644 --- a/model/metadata/metadata.go +++ b/model/metadata/metadata.go @@ -148,15 +148,20 @@ func (md Metadata) tuple(key model.TagName) (int, int) { return 0, 0 } tuple := strings.Split(tag, "/") - t1, t2 := 0, 0 - t1, _ = strconv.Atoi(tuple[0]) + total := md.first(key + "total") if len(tuple) > 1 { - t2, _ = strconv.Atoi(tuple[1]) - } else { - t2tag := md.first(key + "total") - t2, _ = strconv.Atoi(t2tag) + total = tuple[1] } - return t1, t2 + return tagNumber(tuple[0]), tagNumber(total) +} + +// tagNumber rejects negatives and values above int32, so the DB stays readable by 32-bit builds. +func tagNumber(s string) int { + v, err := strconv.ParseInt(s, 10, 32) + if err != nil || v < 0 { + return 0 + } + return int(v) } var dateRegex = regexp.MustCompile(`([12]\d\d\d)`) diff --git a/model/metadata/metadata_test.go b/model/metadata/metadata_test.go index 09a2dfde0..c84d93981 100644 --- a/model/metadata/metadata_test.go +++ b/model/metadata/metadata_test.go @@ -226,6 +226,10 @@ var _ = Describe("Metadata", func() { Entry(nil, "2/10", "", 2, 10), Entry(nil, "", "", 0, 0), Entry(nil, "A", "", 0, 0), + Entry("ignores values that do not fit in 32 bits", "4294967295", "4294967296", 0, 0), + Entry("ignores a total that does not fit in 32 bits", "2/4294967295", "", 2, 0), + Entry("keeps the largest 32-bit value", "2147483647", "", 2147483647, 0), + Entry("ignores negative values", "-1", "-2", 0, 0), ) Describe("Performers", func() { diff --git a/model/playlist.go b/model/playlist.go index f320d845b..19fedc9fa 100644 --- a/model/playlist.go +++ b/model/playlist.go @@ -2,12 +2,16 @@ package model import ( "iter" + "maps" + "os" + "path/filepath" "slices" "strconv" "time" "github.com/navidrome/navidrome/conf" "github.com/navidrome/navidrome/consts" + "github.com/navidrome/navidrome/log" "github.com/navidrome/navidrome/model/criteria" ) @@ -137,6 +141,66 @@ func (pls Playlist) UploadedImagePath() string { return UploadedImagePath(consts.EntityPlaylist, pls.UploadedImage) } +// NormalizedRules returns the rules with child playlist paths resolved to absolute, OS-native paths. +func (pls Playlist) NormalizedRules() *criteria.Criteria { + if pls.Rules == nil || pls.Rules.Expression == nil { + return pls.Rules + } + + rules := *pls.Rules + rules.Expression = normalizePlaylistPaths(pls.Rules.Expression, pls.Path) + return &rules +} + +func normalizePlaylistPaths(inputRule criteria.Expression, referencingPlaylistPath string) criteria.Expression { + switch rule := inputRule.(type) { + case criteria.Any: + anyCriteria := make(criteria.Any, len(rule)) + for i, rules := range rule { + anyCriteria[i] = normalizePlaylistPaths(rules, referencingPlaylistPath) + } + return anyCriteria + case criteria.All: + allCriteria := make(criteria.All, len(rule)) + for i, rules := range rule { + allCriteria[i] = normalizePlaylistPaths(rules, referencingPlaylistPath) + } + return allCriteria + case criteria.InPlaylist: + return criteria.InPlaylist(normalizeChildPathRule(rule, referencingPlaylistPath)) + case criteria.NotInPlaylist: + return criteria.NotInPlaylist(normalizeChildPathRule(rule, referencingPlaylistPath)) + } + + return inputRule +} + +func normalizeChildPathRule(rule map[string]any, referencingPlaylistPath string) map[string]any { + path, ok := rule["path"].(string) + if !ok || path == "" { + return rule + } + + // References use forward slashes to stay portable, while Playlist.Path is OS-native. + path = filepath.FromSlash(path) + switch { + case isAbsPlaylistRef(path): + path = filepath.Clean(path) + case referencingPlaylistPath != "": + path = filepath.Join(filepath.Dir(referencingPlaylistPath), path) + default: + log.Warn("Cannot resolve relative playlist reference: playlist has no file path", "reference", path) + } + normalized := maps.Clone(rule) + normalized["path"] = path + return normalized +} + +// filepath.IsAbs rejects a bare leading separator on Windows, but that is how Unix spells absolute. +func isAbsPlaylistRef(path string) bool { + return filepath.IsAbs(path) || os.IsPathSeparator(path[0]) +} + type Playlists []Playlist type PlaylistCursor iter.Seq2[Playlist, error] diff --git a/model/playlist_test.go b/model/playlist_test.go index d98c85716..7ffb69b63 100644 --- a/model/playlist_test.go +++ b/model/playlist_test.go @@ -1,6 +1,7 @@ package model_test import ( + "path/filepath" "time" "github.com/navidrome/navidrome/conf" @@ -88,4 +89,102 @@ var _ = Describe("Playlist", func() { Expect(model.Playlist{Sync: true}.TracksEditable()).To(BeFalse()) }) }) + + Describe("NormalizedRules()", func() { + // absPath builds an OS-native absolute path so these specs also run on Windows. + absPath := func(parts ...string) string { + abs, err := filepath.Abs(filepath.Join(parts...)) + Expect(err).ToNot(HaveOccurred()) + return abs + } + normalize := func(pls model.Playlist) criteria.Expression { + return pls.NormalizedRules().Expression + } + + It("resolves relative references against the playlist folder", func() { + pls := model.Playlist{ + Path: absPath("test", "nested", "my-playlist.nsp"), + Rules: &criteria.Criteria{Expression: criteria.All{ + criteria.InPlaylist{"path": "../up.m3u"}, + criteria.NotInPlaylist{"path": "./sibling.nsp"}, + criteria.Any{criteria.InPlaylist{"path": "sub/deep.nsp"}}, + }}, + } + Expect(normalize(pls)).To(BeEquivalentTo(criteria.All{ + criteria.InPlaylist{"path": absPath("test", "up.m3u")}, + criteria.NotInPlaylist{"path": absPath("test", "nested", "sibling.nsp")}, + criteria.Any{criteria.InPlaylist{"path": absPath("test", "nested", "sub", "deep.nsp")}}, + })) + }) + + It("cleans absolute references", func() { + dirty := absPath("music") + string(filepath.Separator) + "." + string(filepath.Separator) + "child.nsp" + pls := model.Playlist{ + Path: absPath("test", "my-playlist.nsp"), + Rules: &criteria.Criteria{Expression: criteria.All{criteria.NotInPlaylist{"path": dirty}}}, + } + Expect(normalize(pls)).To(BeEquivalentTo(criteria.All{ + criteria.NotInPlaylist{"path": absPath("music", "child.nsp")}, + })) + }) + + It("treats a leading slash as absolute on every OS", func() { + pls := model.Playlist{ + Path: absPath("test", "my-playlist.nsp"), + Rules: &criteria.Criteria{Expression: criteria.All{criteria.InPlaylist{"path": "/other/./root.m3u"}}}, + } + Expect(normalize(pls)).To(BeEquivalentTo(criteria.All{ + criteria.InPlaylist{"path": filepath.FromSlash("/other/root.m3u")}, + })) + }) + + It("leaves empty paths and id references untouched", func() { + pls := model.Playlist{ + Path: absPath("test", "my-playlist.nsp"), + Rules: &criteria.Criteria{Expression: criteria.All{ + criteria.InPlaylist{"path": ""}, + criteria.InPlaylist{"id": "94d8ba52-7aca-40e2-af82-4cb09c43d710"}, + criteria.Eq{"artist": "Bob Dealin"}, + }}, + } + Expect(normalize(pls)).To(BeEquivalentTo(criteria.All{ + criteria.InPlaylist{"path": ""}, + criteria.InPlaylist{"id": "94d8ba52-7aca-40e2-af82-4cb09c43d710"}, + criteria.Eq{"artist": "Bob Dealin"}, + })) + }) + + It("skips relative references when the playlist has no path", func() { + pls := model.Playlist{ + Rules: &criteria.Criteria{Expression: criteria.All{criteria.InPlaylist{"path": "../up.m3u"}}}, + } + Expect(normalize(pls)).To(BeEquivalentTo(criteria.All{ + criteria.InPlaylist{"path": filepath.FromSlash("../up.m3u")}, + })) + }) + + It("preserves every other criteria field", func() { + rules := criteria.Criteria{ + Expression: criteria.All{criteria.InPlaylist{"path": "child.nsp"}}, + Sort: "title", + Order: "desc", + Limit: 10, + LimitPercent: 25, + Offset: 5, + RefreshDelay: 3 * time.Hour, + } + pls := model.Playlist{Path: absPath("test", "my-playlist.nsp"), Rules: &rules} + + normalized := *pls.NormalizedRules() + normalized.Expression = rules.Expression + Expect(normalized).To(Equal(rules)) + }) + + It("does not mutate the original playlist rules", func() { + original := criteria.All{criteria.InPlaylist{"path": "child.nsp"}} + pls := model.Playlist{Path: absPath("test", "my-playlist.nsp"), Rules: &criteria.Criteria{Expression: original}} + _ = pls.NormalizedRules() + Expect(original[0]).To(BeEquivalentTo(criteria.InPlaylist{"path": "child.nsp"})) + }) + }) }) diff --git a/persistence/criteria_sql.go b/persistence/criteria_sql.go index 890f757d7..d2f817438 100644 --- a/persistence/criteria_sql.go +++ b/persistence/criteria_sql.go @@ -13,6 +13,7 @@ import ( "github.com/Masterminds/squirrel" "github.com/navidrome/navidrome/model" "github.com/navidrome/navidrome/model/criteria" + "golang.org/x/text/unicode/norm" ) type smartPlaylistJoinType int @@ -344,11 +345,15 @@ func startOfPeriod(numDays int64, from time.Time) string { } func (c smartPlaylistCriteria) inList(values map[string]any, negate bool) (squirrel.Sqlizer, error) { - playlistID, ok := values["id"].(string) - if !ok { - return nil, errors.New("playlist id not given") + var condition squirrel.Sqlizer + if playlistId, ok := values["id"].(string); ok && playlistId != "" { + condition = squirrel.Eq{"pl.playlist_id": playlistId} + } else if playlistPath, ok := values["path"].(string); ok && playlistPath != "" { + condition = squirrel.Eq{"playlist.path": pathVariants(playlistPath)} + } else { + return nil, errors.New("playlist id or path not given") } - filters := squirrel.And{squirrel.Eq{"pl.playlist_id": playlistID}} + filters := squirrel.And{condition} if !c.owner.IsAdmin { if c.owner.ID == "" { filters = append(filters, squirrel.Eq{"playlist.public": 1}) @@ -373,6 +378,18 @@ func (c smartPlaylistCriteria) inList(values map[string]any, negate bool) (squir return squirrel.Expr("media_file.id IN ("+subSQL+")", subArgs...), nil } +// Filesystems disagree on the Unicode form of a name, so match the path in NFC and NFD. +func pathVariants(path string) []string { + variants := []string{path} + if alt := norm.NFC.String(path); alt != path { + variants = append(variants, alt) + } + if alt := norm.NFD.String(path); alt != path { + variants = append(variants, alt) + } + return variants +} + func jsonExpr(info criteria.FieldInfo, cond squirrel.Sqlizer, negate bool) squirrel.Sqlizer { if info.IsRole { return roleCond{role: info.Name(), cond: cond, not: negate} diff --git a/persistence/criteria_sql_test.go b/persistence/criteria_sql_test.go index 8d0069b0d..ef91ec989 100644 --- a/persistence/criteria_sql_test.go +++ b/persistence/criteria_sql_test.go @@ -47,7 +47,10 @@ var _ = Describe("Smart playlist criteria SQL", func() { Entry("in range", criteria.InTheRange{"year": []int{1980, 1990}}, "(media_file.year >= ? AND media_file.year <= ?)", 1980, 1990), Entry("before", criteria.Before{"lastPlayed": time.Date(2021, 10, 1, 0, 0, 0, 0, time.Local)}, "annotation.play_date < ?", time.Date(2021, 10, 1, 0, 0, 0, 0, time.Local)), Entry("after", criteria.After{"lastPlayed": time.Date(2021, 10, 1, 0, 0, 0, 0, time.Local)}, "annotation.play_date > ?", time.Date(2021, 10, 1, 0, 0, 0, 0, time.Local)), - Entry("in playlist", criteria.InPlaylist{"id": "deadbeef-dead-beef"}, "media_file.id IN (SELECT media_file_id FROM playlist_tracks pl LEFT JOIN playlist on pl.playlist_id = playlist.id WHERE (pl.playlist_id = ? AND playlist.public = ?))", "deadbeef-dead-beef", 1), + Entry("in playlist [path]", criteria.InPlaylist{"path": "lacuslacus.nsp"}, "media_file.id IN (SELECT media_file_id FROM playlist_tracks pl LEFT JOIN playlist on pl.playlist_id = playlist.id WHERE (playlist.path IN (?) AND playlist.public = ?))", "lacuslacus.nsp", 1), + Entry("in playlist [id]", criteria.InPlaylist{"id": "deadbeef-dead-beef"}, "media_file.id IN (SELECT media_file_id FROM playlist_tracks pl LEFT JOIN playlist on pl.playlist_id = playlist.id WHERE (pl.playlist_id = ? AND playlist.public = ?))", "deadbeef-dead-beef", 1), + Entry("in playlist [empty id falls back to path]", criteria.InPlaylist{"id": "", "path": "/music/x.nsp"}, "media_file.id IN (SELECT media_file_id FROM playlist_tracks pl LEFT JOIN playlist on pl.playlist_id = playlist.id WHERE (playlist.path IN (?) AND playlist.public = ?))", "/music/x.nsp", 1), + Entry("in playlist [decomposed unicode path]", criteria.InPlaylist{"path": "/m\u00fasica/x.nsp"}, "media_file.id IN (SELECT media_file_id FROM playlist_tracks pl LEFT JOIN playlist on pl.playlist_id = playlist.id WHERE (playlist.path IN (?,?) AND playlist.public = ?))", "/m\u00fasica/x.nsp", "/mu\u0301sica/x.nsp", 1), Entry("not in playlist", criteria.NotInPlaylist{"id": "deadbeef-dead-beef"}, "media_file.id NOT IN (SELECT media_file_id FROM playlist_tracks pl LEFT JOIN playlist on pl.playlist_id = playlist.id WHERE (pl.playlist_id = ? AND playlist.public = ?))", "deadbeef-dead-beef", 1), Entry("album annotation", criteria.Gt{"albumRating": 3}, "album_annotation.rating > ?", 3), Entry("artist annotation", criteria.Is{"artistLoved": true}, "artist_annotation.starred = ?", true), @@ -268,6 +271,13 @@ var _ = Describe("Smart playlist criteria SQL", func() { Expect(err).To(MatchError(ContainSubstring("invalid boolean value for 'missing' expression"))) }) + It("returns an error when inPlaylist has empty path", func() { + _, err := newSmartPlaylistCriteria( + criteria.Criteria{Expression: criteria.InPlaylist{"path": ""}}, + withSmartPlaylistOwner(model.User{ID: "owner-id", IsAdmin: false})).where() + Expect(err).To(MatchError(ContainSubstring("playlist id or path not given"))) + }) + It("returns an error for a range over a tag/role field", func() { _, err := newSmartPlaylistCriteria(criteria.Criteria{Expression: criteria.InTheRange{"rate": []int{1, 5}}}).where() Expect(err).To(MatchError(ContainSubstring("range operator not supported for tag/role field"))) diff --git a/persistence/folder_repository.go b/persistence/folder_repository.go index a4b73d9d6..a1136ad8d 100644 --- a/persistence/folder_repository.go +++ b/persistence/folder_repository.go @@ -27,6 +27,14 @@ type dbFolder struct { ImageFiles string `structs:"-" json:"-"` } +// String guards the promoted Folder.String(), which would dereference a nil Folder. +func (f dbFolder) String() string { + if f.Folder == nil { + return "" + } + return f.Folder.String() +} + func (f *dbFolder) PostScan() error { var err error if f.ImageFiles != "" { diff --git a/persistence/folder_repository_test.go b/persistence/folder_repository_test.go index 545d8ad49..b7bc52751 100644 --- a/persistence/folder_repository_test.go +++ b/persistence/folder_repository_test.go @@ -310,6 +310,13 @@ var _ = Describe("FolderRepository", func() { }) }) + Describe("dbFolder.String", func() { + It("does not dereference a nil Folder", func() { + Expect(fmt.Sprint(dbFolder{})).To(Equal("")) + Expect(fmt.Sprint(&dbFolder{})).To(Equal("")) + }) + }) + Describe("wrapFolderCursor", func() { It("does not panic when the cursor yields a dbFolder with nil Folder", func() { // Simulate what queryWithStableResults does on the rows.Err() path: diff --git a/persistence/mediafile_repository.go b/persistence/mediafile_repository.go index 935a64bb9..835c39b1e 100644 --- a/persistence/mediafile_repository.go +++ b/persistence/mediafile_repository.go @@ -38,6 +38,14 @@ type dbMediaFile struct { RgTrackPeak *float64 `structs:"-" json:"-"` } +// String guards the promoted MediaFile.String(), which would dereference a nil MediaFile. +func (m dbMediaFile) String() string { + if m.MediaFile == nil { + return "" + } + return m.MediaFile.String() +} + func (m *dbMediaFile) PostScan() error { m.RGTrackGain = m.RgTrackGain m.RGTrackPeak = m.RgTrackPeak diff --git a/persistence/mediafile_repository_test.go b/persistence/mediafile_repository_test.go index 1fb939415..d81b5e9ee 100644 --- a/persistence/mediafile_repository_test.go +++ b/persistence/mediafile_repository_test.go @@ -1203,6 +1203,13 @@ var _ = Describe("MediaRepository", func() { }) }) + Describe("dbMediaFile.String", func() { + It("does not dereference a nil MediaFile", func() { + Expect(fmt.Sprint(dbMediaFile{})).To(Equal("")) + Expect(fmt.Sprint(&dbMediaFile{})).To(Equal("")) + }) + }) + Describe("wrapMediaFileCursor", func() { It("does not panic when the cursor yields a dbMediaFile with nil MediaFile", func() { // Simulate what queryWithStableResults does on the rows.Err() path: diff --git a/persistence/persistence.go b/persistence/persistence.go index 9d3a33cfc..589812266 100644 --- a/persistence/persistence.go +++ b/persistence/persistence.go @@ -3,6 +3,7 @@ package persistence import ( "context" "database/sql" + "fmt" "reflect" "time" @@ -138,11 +139,15 @@ func (s *SQLStore) Resource(ctx context.Context, m any) model.ResourceRepository return nil } -func (s *SQLStore) WithTx(block func(tx model.DataStore) error, scope ...string) error { - var msg string +func scopeLabel(scope []string) string { if len(scope) > 0 { - msg = scope[0] + return scope[0] } + return "" +} + +func (s *SQLStore) WithTx(block func(tx model.DataStore) error, scope ...string) error { + msg := scopeLabel(scope) start := time.Now() conn, inTx := s.db.(*dbx.DB) if !inTx { @@ -177,6 +182,51 @@ func (s *SQLStore) WithTxImmediate(block func(tx model.DataStore) error, scope . }, scope...) } +// txRetryDelay spaces out reruns of a busy transaction. Each attempt has already waited out the +// busy timeout, so WithTxRetry gives up only after a sustained lock. +var txRetryDelay = 5 * time.Second + +const txMaxRetries = 3 + +func (s *SQLStore) WithTxRetry(ctx context.Context, block func(ctx context.Context, tx model.DataStore) error, scope ...string) error { + // Inside a transaction, join it: the outer one holds the lock and owns commit and rollback + if _, ok := s.db.(*dbx.DB); !ok { + return block(ctx, s) + } + for attempt := 0; ; attempt++ { + attemptCtx := ctx + if attempt < txMaxRetries { + attemptCtx = withBusyRetry(ctx) + } + err := s.WithTx(func(tx model.DataStore) error { return block(attemptCtx, tx) }, scope...) + if attempt == txMaxRetries || !db.IsBusy(err) { + return err + } + log.Warn(ctx, "Database busy, retrying transaction", "scope", scopeLabel(scope), "attempt", attempt+1, err) + select { + case <-ctx.Done(): + return ctx.Err() + case <-time.After(time.Duration(attempt+1) * txRetryDelay): + } + } +} + +type busyRetryKey struct{} + +// withBusyRetry marks a transaction attempt that WithTxRetry will rerun, so a busy statement in it +// is logged as a warning rather than an error. +func withBusyRetry(ctx context.Context) context.Context { + return context.WithValue(ctx, busyRetryKey{}, true) +} + +func hasBusyRetry(ctx context.Context) bool { + if ctx == nil { + return false + } + retry, _ := ctx.Value(busyRetryKey{}).(bool) + return retry +} + func (s *SQLStore) GC(ctx context.Context, libraryIDs ...int) error { trace := func(ctx context.Context, msg string, f func() error) func() error { return func() error { @@ -207,9 +257,9 @@ func (s *SQLStore) GC(ctx context.Context, libraryIDs ...int) error { trace(ctx, "remove orphan playlist tracks", func() error { return s.Playlist(ctx).(*playlistRepository).removeOrphans() }), ) if err != nil { - log.Error(ctx, "Error tidying up database", err) + return fmt.Errorf("tidying up database: %w", err) } - return err + return nil } func (s *SQLStore) getDBXBuilder() dbx.Builder { diff --git a/persistence/persistence_suite_test.go b/persistence/persistence_suite_test.go index f146cb06b..644284c0b 100644 --- a/persistence/persistence_suite_test.go +++ b/persistence/persistence_suite_test.go @@ -169,6 +169,28 @@ func p(path string) string { return filepath.FromSlash(path) } +// restrictedFixture creates a second library plus a non-admin user granted library 1 only, so +// specs can assert that a query filters by library. Cleans itself up after the spec. +func restrictedFixture(name string) (context.Context, model.Library, model.User) { + adminCtx := request.WithUser(log.NewContext(GinkgoT().Context()), adminUser) + db := GetDBXBuilder() + + lib := model.Library{Name: name + " Library", Path: "/" + name} + lr := NewLibraryRepository(adminCtx, db) + Expect(lr.Put(&lib)).To(Succeed()) + + user := createUserWithLibraries(name+"-restricted", []int{1}) + ur := NewUserRepository(adminCtx, db) + Expect(ur.Put(&user)).To(Succeed()) + Expect(ur.SetUserLibraries(user.ID, []int{1})).To(Succeed()) + + DeferCleanup(func() { + _ = NewUserRepository(adminCtx, db).Delete(user.ID) + _ = NewLibraryRepository(adminCtx, db).(*libraryRepository).delete(squirrel.Eq{"id": lib.ID}) + }) + return adminCtx, lib, user +} + var _ = BeforeSuite(func() { conn := GetDBXBuilder() ctx := log.NewContext(context.TODO()) diff --git a/persistence/persistence_test.go b/persistence/persistence_test.go index 13e56bde1..e13f6a231 100644 --- a/persistence/persistence_test.go +++ b/persistence/persistence_test.go @@ -2,7 +2,10 @@ package persistence import ( "context" + "errors" + "time" + "github.com/mattn/go-sqlite3" "github.com/navidrome/navidrome/db" "github.com/navidrome/navidrome/model" . "github.com/onsi/ginkgo/v2" @@ -55,4 +58,74 @@ var _ = Describe("SQLStore", func() { }) }) }) + + Describe("WithTxRetry", func() { + busy := sqlite3.Error{Code: sqlite3.ErrBusy} + BeforeEach(func() { + DeferCleanup(func(d time.Duration) { txRetryDelay = d }, txRetryDelay) + txRetryDelay = 0 + }) + + It("reruns a busy transaction from a clean rollback", func() { + var attempts []bool + err := ds.WithTxRetry(ctx, func(ctx context.Context, tx model.DataStore) error { + attempts = append(attempts, hasBusyRetry(ctx)) + Expect(tx.Property(ctx).Put("retry-key", "attempt")).To(Succeed()) + if len(attempts) < 3 { + return busy + } + return nil + }) + Expect(err).ToNot(HaveOccurred()) + Expect(attempts).To(Equal([]bool{true, true, true})) + Expect(ds.Property(ctx).Get("retry-key")).To(Equal("attempt")) + }) + + It("gives up after the last retry, which is not marked as retried", func() { + var attempts []bool + err := ds.WithTxRetry(ctx, func(ctx context.Context, _ model.DataStore) error { + attempts = append(attempts, hasBusyRetry(ctx)) + return busy + }) + Expect(db.IsBusy(err)).To(BeTrue()) + Expect(attempts).To(Equal([]bool{true, true, true, false})) + }) + + It("does not rerun on other errors", func() { + calls := 0 + err := ds.WithTxRetry(ctx, func(context.Context, model.DataStore) error { + calls++ + return sqlite3.Error{Code: sqlite3.ErrConstraint} + }) + Expect(err).To(HaveOccurred()) + Expect(calls).To(Equal(1)) + }) + + It("does not rerun when called inside a transaction", func() { + calls := 0 + err := ds.WithTx(func(tx model.DataStore) error { + return tx.WithTxRetry(ctx, func(context.Context, model.DataStore) error { + calls++ + return busy + }) + }) + Expect(db.IsBusy(err)).To(BeTrue()) + Expect(calls).To(Equal(1)) + }) + + It("joins the enclosing transaction instead of opening another", func() { + rollback := errors.New("rollback") + err := ds.WithTx(func(tx model.DataStore) error { + Expect(tx.Property(ctx).Put("outer-key", "v")).To(Succeed()) + Expect(tx.WithTxRetry(ctx, func(ctx context.Context, inner model.DataStore) error { + Expect(inner.Property(ctx).Get("outer-key")).To(Equal("v")) + return inner.Property(ctx).Put("inner-key", "v") + })).To(Succeed()) + return rollback + }) + Expect(err).To(MatchError(rollback)) + _, err = ds.Property(ctx).Get("inner-key") + Expect(err).To(MatchError(model.ErrNotFound)) + }) + }) }) diff --git a/persistence/player_repository.go b/persistence/player_repository.go index a2b079d53..a2114b060 100644 --- a/persistence/player_repository.go +++ b/persistence/player_repository.go @@ -126,7 +126,7 @@ func (r *playerRepository) Save(entity any) (string, error) { if !r.isPermitted(t) { return "", rest.ErrPermissionDenied } - return r.put(t.ID, t) + return r.put("", t) // Save only creates; edits go through the owner-scoped Update } func (r *playerRepository) Update(id string, entity any, cols ...string) error { diff --git a/persistence/player_repository_test.go b/persistence/player_repository_test.go index b7085a1fb..4a7701ac2 100644 --- a/persistence/player_repository_test.go +++ b/persistence/player_repository_test.go @@ -288,6 +288,27 @@ var _ = Describe("PlayerRepository", func() { Expect(*stored).To(Equal(adminPlayer1)) }) + It("does not let a regular user overwrite another user's player via Save with a spoofed id", func() { + spoofed := model.Player{ + ID: adminPlayer1.ID, + Name: "HIJACKED", + UserId: regularUser.ID, + ReportRealPath: true, + } + + id, err := regularRepo.Save(&spoofed) + Expect(err).To(BeNil()) + Expect(id).ToNot(Equal(adminPlayer1.ID)) + + stored, err := adminRepo.Get(adminPlayer1.ID) + Expect(err).To(BeNil()) + Expect(*stored).To(Equal(adminPlayer1)) + + created, err := adminRepo.Get(id) + Expect(err).To(BeNil()) + Expect(created.UserId).To(Equal(regularUser.ID)) + }) + It("does not let a regular user reassign their own player to another user", func() { // Owner updates their own player but tries to give it away to the admin. The update // succeeds for the other fields, but user_id is never written, so ownership stays put. diff --git a/persistence/playlist_repository.go b/persistence/playlist_repository.go index cd3e48d18..2afef9f45 100644 --- a/persistence/playlist_repository.go +++ b/persistence/playlist_repository.go @@ -13,6 +13,7 @@ import ( "github.com/deluan/rest" "github.com/navidrome/navidrome/log" "github.com/navidrome/navidrome/model" + "github.com/navidrome/navidrome/utils/slice" "github.com/pocketbase/dbx" ) @@ -276,10 +277,17 @@ func (r *playlistRepository) updatePlaylist(playlistId string, mediaFileIds []st return err } - return r.addTracks(playlistId, 1, mediaFileIds) + _, err = r.addTracks(playlistId, 1, mediaFileIds) + return err } -func (r *playlistRepository) addTracks(playlistId string, startingPos int, mediaFileIds []string) error { +// addTracks is the only path that writes playlist_tracks rows (smart playlists aside), so it owns +// the library check: every caller, including a full replace through Put, goes through it. +func (r *playlistRepository) addTracks(playlistId string, startingPos int, mediaFileIds []string) (int, error) { + mediaFileIds, err := r.keepAccessible(mediaFileIds) + if err != nil { + return 0, err + } // Break the track list in chunks to avoid hitting SQLITE_MAX_VARIABLE_NUMBER limit // Add new tracks, chunk by chunk pos := startingPos @@ -289,14 +297,36 @@ func (r *playlistRepository) addTracks(playlistId string, startingPos int, media ins = ins.Values(playlistId, t, pos) pos++ } - _, err := r.executeSQL(ins) - if err != nil { - return err + if _, err := r.executeSQL(ins); err != nil { + return 0, err } } r.enqueueCoverRebuild(playlistId) - return r.refreshCounters(&model.Playlist{ID: playlistId}) + return len(mediaFileIds), r.refreshCounters(&model.Playlist{ID: playlistId}) +} + +// keepAccessible drops ids the caller cannot read, preserving order and duplicates. Chunked +// because callers pass unbounded id lists (M3U import), well past SQLITE_MAX_VARIABLE_NUMBER. +func (r *playlistRepository) keepAccessible(mediaFileIds []string) ([]string, error) { + if visible, err := r.visibleLibraryIDs(); err == nil && r.userSeesAllLibraries(visible) { + return mediaFileIds, nil + } + accessible := make(map[string]struct{}, len(mediaFileIds)) + for chunk := range slices.Chunk(slice.Unique(mediaFileIds), 200) { + sq := r.applyLibraryFilter(Select("id").From("media_file").Where(Eq{"id": chunk}), "media_file") + var found []string + if err := r.queryAllSlice(sq, &found); err != nil { + return nil, err + } + for _, id := range found { + accessible[id] = struct{}{} + } + } + return slice.Filter(mediaFileIds, func(id string) bool { + _, ok := accessible[id] + return ok + }), nil } // refreshCounters updates total playlist duration, size and count diff --git a/persistence/playlist_track_repository.go b/persistence/playlist_track_repository.go index 847316868..848b67be0 100644 --- a/persistence/playlist_track_repository.go +++ b/persistence/playlist_track_repository.go @@ -91,6 +91,7 @@ func (r *playlistTrackRepository) Count(options ...rest.QueryOptions) (int64, er query := Select(). LeftJoin("media_file f on f.id = media_file_id"). Where(Eq{"playlist_id": r.playlistId}) + query = r.applyLibraryFilter(query, "f") return r.count(query, r.parseRestOptions(r.ctx, options...)) } @@ -113,6 +114,7 @@ func (r *playlistTrackRepository) Read(id string) (any, error) { ). Join("media_file f on f.id = media_file_id"). Where(And{Eq{"playlist_id": r.playlistId}, Eq{"playlist_tracks.id": id}}) + sel = r.applyLibraryFilter(sel, "f") var trk dbPlaylistTrack err := r.queryOne(sel, &trk) return trk.PlaylistTrack, err @@ -157,6 +159,7 @@ func (r *playlistTrackRepository) GetAlbumIDs(options ...model.QueryOptions) ([] query := r.newSelect(options...).Columns("distinct mf.album_id"). Join("media_file mf on mf.id = media_file_id"). Where(Eq{"playlist_id": r.playlistId}) + query = r.applyLibraryFilter(query, "mf") var ids []string err := r.queryAllSlice(query, &ids) if err != nil { @@ -187,12 +190,11 @@ func (r *playlistTrackRepository) Add(mediaFileIds []string) (int, error) { // Get next pos (ID) in playlist sq := r.newSelect().Columns("max(id) as max").Where(Eq{"playlist_id": r.playlistId}) var res struct{ Max sql.NullInt32 } - err := r.queryOne(sq, &res) - if err != nil { + if err := r.queryOne(sq, &res); err != nil { return 0, err } - return len(mediaFileIds), r.playlistRepo.addTracks(r.playlistId, int(res.Max.Int32+1), mediaFileIds) + return r.playlistRepo.addTracks(r.playlistId, int(res.Max.Int32+1), mediaFileIds) } // Insert adds tracks before the 1-based position pos, shifting the following entries down; a @@ -215,7 +217,13 @@ func (r *playlistTrackRepository) Insert(mediaFileIds []string, pos int) (int, e if res == 0 { return r.Add(mediaFileIds) } - return n, r.playlistRepo.addTracks(r.playlistId, pos, mediaFileIds) + inserted, err := r.playlistRepo.addTracks(r.playlistId, pos, mediaFileIds) + if err != nil || inserted == n { + return inserted, err + } + // The shift above reserved a slot per requested id, so ids dropped by the library filter + // leave a hole. Close it. + return inserted, r.playlistRepo.renumber(r.playlistId) } func (r *playlistTrackRepository) addMediaFileIds(cond Sqlizer) (int, error) { diff --git a/persistence/playlist_track_repository_test.go b/persistence/playlist_track_repository_test.go index e0360adb4..1a6bc9dc6 100644 --- a/persistence/playlist_track_repository_test.go +++ b/persistence/playlist_track_repository_test.go @@ -1,11 +1,13 @@ package persistence import ( + "context" "strconv" "github.com/navidrome/navidrome/log" "github.com/navidrome/navidrome/model" "github.com/navidrome/navidrome/model/request" + "github.com/navidrome/navidrome/utils/slice" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" ) @@ -221,4 +223,151 @@ var _ = Describe("PlaylistTrackRepository", func() { Expect(tracks.CountAll()).To(BeZero()) }) }) + + Describe("library access", func() { + var otherLib model.Library + var restrictedUser model.User + var adminCtx, userCtx context.Context + var userTracks model.PlaylistTrackRepository + var plsID string + + BeforeEach(func() { + adminCtx, otherLib, restrictedUser = restrictedFixture("pls") + userCtx = request.WithUser(log.NewContext(GinkgoT().Context()), restrictedUser) + db := GetDBXBuilder() + + adminMr := NewMediaFileRepository(adminCtx, db) + Expect(adminMr.Put(&model.MediaFile{ + ID: "pls-otherlib-track", LibraryID: otherLib.ID, AlbumID: "pls-hidden-album", + Path: "hidden/in-playlist.mp3", Title: "Hidden In Playlist", + })).To(Succeed()) + DeferCleanup(func() { _ = adminMr.Delete("pls-otherlib-track") }) + + adminPls := NewPlaylistRepository(adminCtx, db) + pls := model.Playlist{Name: "Public Mixed", OwnerID: adminUser.ID, OwnerName: adminUser.UserName, Public: true} + Expect(adminPls.Put(&pls)).To(Succeed()) + plsID = pls.ID + DeferCleanup(func() { _ = adminPls.Delete(plsID) }) + Expect(adminPls.Tracks(plsID, false).Add([]string{songDayInALife.ID, "pls-otherlib-track"})).To(Equal(2)) + + userTracks = NewPlaylistRepository(userCtx, db).Tracks(plsID, false) + }) + + It("Read does not return a track outside the user's libraries", func() { + _, err := userTracks.Read("2") + Expect(err).To(MatchError(model.ErrNotFound), "position 2 holds a track the user cannot access") + }) + + It("Read still returns a track inside the user's libraries", func() { + trk, err := userTracks.Read("1") + Expect(err).ToNot(HaveOccurred()) + Expect(trk.(*model.PlaylistTrack).MediaFile.ID).To(Equal(songDayInALife.ID)) + }) + + It("Count excludes tracks outside the user's libraries", func() { + Expect(userTracks.Count()).To(Equal(int64(1)), "Count must agree with the filtered listing") + }) + + It("GetAlbumIDs excludes albums outside the user's libraries", func() { + Expect(userTracks.GetAlbumIDs()).ToNot(ContainElement("pls-hidden-album")) + }) + + Describe("Add", func() { + var ownTracks model.PlaylistTrackRepository + + BeforeEach(func() { + userPls := NewPlaylistRepository(userCtx, GetDBXBuilder()) + own := model.Playlist{Name: "Own Playlist", OwnerID: restrictedUser.ID, OwnerName: restrictedUser.UserName} + Expect(userPls.Put(&own)).To(Succeed()) + DeferCleanup(func() { _ = NewPlaylistRepository(adminCtx, GetDBXBuilder()).Delete(own.ID) }) + ownTracks = userPls.Tracks(own.ID, false) + }) + + It("drops ids outside the user's libraries", func() { + Expect(ownTracks.Add([]string{songDayInALife.ID, "pls-otherlib-track"})).To(Equal(1)) + Expect(ownTracks.GetMediaFileIDs()).To(ConsistOf(songDayInALife.ID)) + }) + + It("drops them when reached through AddAlbums", func() { + Expect(ownTracks.AddAlbums([]string{"pls-hidden-album"})).To(BeZero()) + }) + + It("drops them when reached through Insert", func() { + Expect(ownTracks.Add([]string{songDayInALife.ID})).To(Equal(1)) + + Expect(ownTracks.Insert([]string{"pls-otherlib-track", songComeTogether.ID}, 1)).To(Equal(1)) + + Expect(ownTracks.GetMediaFileIDs()).To(Equal([]string{songComeTogether.ID, songDayInALife.ID})) + trks, err := ownTracks.GetAll(model.QueryOptions{Sort: "id"}) + Expect(err).ToNot(HaveOccurred()) + Expect(slice.Map(trks, func(t model.PlaylistTrack) string { return t.ID })).To(Equal([]string{"1", "2"}), + "positions must stay contiguous when an id is dropped") + }) + }) + + Describe("Put", func() { + storedIDs := func(id string) []string { + ids, err := NewPlaylistRepository(adminCtx, GetDBXBuilder()).Tracks(id, false).GetMediaFileIDs() + Expect(err).ToNot(HaveOccurred()) + return ids + } + put := func(ctx context.Context, owner model.User, pls *model.Playlist, ids ...string) string { + pls.OwnerID = owner.ID + pls.Tracks = nil + pls.AddMediaFilesByID(ids) + Expect(NewPlaylistRepository(ctx, GetDBXBuilder()).Put(pls)).To(Succeed()) + DeferCleanup(func() { _ = NewPlaylistRepository(adminCtx, GetDBXBuilder()).Delete(pls.ID) }) + return pls.ID + } + + It("drops ids outside the user's libraries when creating a playlist", func() { + id := put(userCtx, restrictedUser, &model.Playlist{Name: "Created"}, songDayInALife.ID, "pls-otherlib-track") + + Expect(storedIDs(id)).To(Equal([]string{songDayInALife.ID})) + }) + + It("drops them when replacing the tracks of an existing playlist", func() { + pls := &model.Playlist{Name: "Replaced"} + put(userCtx, restrictedUser, pls, songDayInALife.ID) + + put(userCtx, restrictedUser, pls, "pls-otherlib-track") + + Expect(storedIDs(pls.ID)).To(BeEmpty()) + }) + + It("does not count a dropped id, so it cannot be told apart from an unknown one", func() { + hidden := put(userCtx, restrictedUser, &model.Playlist{Name: "Hidden"}, songDayInALife.ID, "pls-otherlib-track") + unknown := put(userCtx, restrictedUser, &model.Playlist{Name: "Unknown"}, songDayInALife.ID, "no-such-track") + + userPls := NewPlaylistRepository(userCtx, GetDBXBuilder()) + h, err := userPls.Get(hidden) + Expect(err).ToNot(HaveOccurred()) + u, err := userPls.Get(unknown) + Expect(err).ToNot(HaveOccurred()) + Expect(h.SongCount).To(Equal(u.SongCount)) + Expect(h.Duration).To(Equal(u.Duration)) + Expect(h.Size).To(Equal(u.Size)) + }) + + It("keeps order and duplicates of the accessible ids", func() { + id := put(userCtx, restrictedUser, &model.Playlist{Name: "Ordered"}, + songDayInALife.ID, "pls-otherlib-track", songComeTogether.ID, songDayInALife.ID) + + Expect(storedIDs(id)).To(Equal([]string{songDayInALife.ID, songComeTogether.ID, songDayInALife.ID})) + }) + + It("keeps every id when run as an admin, as the scanner's playlist sync does", func() { + id := put(adminCtx, adminUser, &model.Playlist{Name: "Synced"}, songDayInALife.ID, "pls-otherlib-track") + + Expect(storedIDs(id)).To(Equal([]string{songDayInALife.ID, "pls-otherlib-track"})) + }) + }) + + It("still shows everything to an admin", func() { + adminTracks := NewPlaylistRepository(adminCtx, GetDBXBuilder()).Tracks(plsID, false) + Expect(adminTracks.Count()).To(Equal(int64(2))) + _, err := adminTracks.Read("2") + Expect(err).ToNot(HaveOccurred()) + }) + }) }) diff --git a/persistence/smart_playlist_repository.go b/persistence/smart_playlist_repository.go index 9d2ac9590..24c6f5fc5 100644 --- a/persistence/smart_playlist_repository.go +++ b/persistence/smart_playlist_repository.go @@ -1,11 +1,14 @@ package persistence import ( + "slices" "time" . "github.com/Masterminds/squirrel" "github.com/navidrome/navidrome/log" "github.com/navidrome/navidrome/model" + "github.com/navidrome/navidrome/utils/slice" + "golang.org/x/text/unicode/norm" ) // PlaylistRepository methods to handle smart playlists, which are defined by criteria and automatically populated @@ -16,6 +19,17 @@ import ( // refreshSmartPlaylist evaluates the criteria of a smart playlist and updates its tracks accordingly. func (r *playlistRepository) refreshSmartPlaylist(pls *model.Playlist) bool { + return r.refreshSmartPlaylistTree(pls, map[string]struct{}{}) +} + +// The visited set stops playlists that reference each other from recursing forever. +func (r *playlistRepository) refreshSmartPlaylistTree(pls *model.Playlist, visited map[string]struct{}) bool { + if _, seen := visited[pls.ID]; seen { + log.Trace(r.ctx, "Skipping already visited smart playlist", "playlist", pls.Name, "id", pls.ID) + return false + } + visited[pls.ID] = struct{}{} + usr := loggedUser(r.ctx) if !r.shouldRefreshSmartPlaylist(pls, usr) { return false @@ -30,9 +44,9 @@ func (r *playlistRepository) refreshSmartPlaylist(pls *model.Playlist) bool { return false } - rulesSQL := newSmartPlaylistCriteria(*pls.Rules, withSmartPlaylistOwner(*usr)) + rulesSQL := newSmartPlaylistCriteria(*pls.NormalizedRules(), withSmartPlaylistOwner(*usr)) - if !r.refreshChildPlaylists(pls, rulesSQL) { + if !r.refreshChildPlaylists(pls, rulesSQL, visited) { return false } @@ -89,28 +103,47 @@ func (r *playlistRepository) shouldRefreshSmartPlaylist(pls *model.Playlist, usr // refreshChildPlaylists handles refreshing any child playlists that are referenced in the smart playlist criteria. // Returns false if child playlists could not be loaded (DB error), signaling the parent refresh should abort. -func (r *playlistRepository) refreshChildPlaylists(pls *model.Playlist, rulesSQL smartPlaylistCriteria) bool { +func (r *playlistRepository) refreshChildPlaylists(pls *model.Playlist, rulesSQL smartPlaylistCriteria, visited map[string]struct{}) bool { childPlaylistIds := rulesSQL.ChildPlaylistIds() - if len(childPlaylistIds) == 0 { + childPlaylistPaths := rulesSQL.ChildPlaylistPaths() + if len(childPlaylistIds) == 0 && len(childPlaylistPaths) == 0 { return true } - childPlaylists, err := r.GetAll(model.QueryOptions{Filters: Eq{"playlist.id": childPlaylistIds}}) + var conditions Or + if len(childPlaylistIds) > 0 { + conditions = append(conditions, Eq{"playlist.id": childPlaylistIds}) + } + if len(childPlaylistPaths) > 0 { + lookupPaths := slices.Concat(slice.Map(childPlaylistPaths, pathVariants)...) + conditions = append(conditions, Eq{"playlist.path": lookupPaths}) + } + + childPlaylists, err := r.GetAll(model.QueryOptions{Filters: conditions}) if err != nil { - log.Error(r.ctx, "Error loading child playlists for smart playlist refresh", "playlist", pls.Name, "id", pls.ID, "childIds", childPlaylistIds, err) + log.Error(r.ctx, "Error loading child playlists for smart playlist refresh", "playlist", pls.Name, "id", pls.ID, "childIds", childPlaylistIds, "childPaths", childPlaylistPaths, err) return false } - found := make(map[string]struct{}, len(childPlaylists)) + found := make(map[string]struct{}, len(childPlaylists)*2) for i := range childPlaylists { found[childPlaylists[i].ID] = struct{}{} - r.refreshSmartPlaylist(&childPlaylists[i]) + if childPlaylists[i].Path != "" { + found[norm.NFC.String(childPlaylists[i].Path)] = struct{}{} + } + r.refreshSmartPlaylistTree(&childPlaylists[i], visited) } for _, id := range childPlaylistIds { if _, ok := found[id]; !ok { log.Warn(r.ctx, "Referenced playlist is not accessible to smart playlist owner", "playlist", pls.Name, "id", pls.ID, "childId", id, "ownerId", pls.OwnerID) } } + + for _, path := range childPlaylistPaths { + if _, ok := found[norm.NFC.String(path)]; !ok { + log.Warn(r.ctx, "Referenced playlist is not accessible to smart playlist owner", "playlist", pls.Name, "id", pls.ID, "path", path, "ownerId", pls.OwnerID) + } + } return true } diff --git a/persistence/smart_playlist_repository_test.go b/persistence/smart_playlist_repository_test.go index 6f8684d5c..33da0c1b7 100644 --- a/persistence/smart_playlist_repository_test.go +++ b/persistence/smart_playlist_repository_test.go @@ -1,6 +1,7 @@ package persistence import ( + "path/filepath" "time" "github.com/navidrome/navidrome/conf" @@ -124,13 +125,23 @@ var _ = Describe("PlaylistRepository - Smart Playlists", func() { criteria.Contains{"title": "Day"}, }, } - nestedPls := model.Playlist{Name: "Nested", OwnerID: "userid", Public: true, Rules: childRules} + nestedPls := model.Playlist{Name: "Nested [ID]", OwnerID: "userid", Public: true, Rules: childRules} Expect(repo.Put(&nestedPls)).To(Succeed()) DeferCleanup(func() { _ = repo.Delete(nestedPls.ID) }) - parentPls := model.Playlist{Name: "Parent", OwnerID: "userid", Rules: &criteria.Criteria{ + childRules = &criteria.Criteria{ Expression: criteria.All{ + criteria.Eq{"artist": "シートベルツ"}, + }, + } + nestedPathPls := model.Playlist{Name: "Nested [Path]", OwnerID: "userid", Path: "test.nsp", Public: true, Rules: childRules} + Expect(repo.Put(&nestedPathPls)).To(Succeed()) + DeferCleanup(func() { _ = repo.Delete(nestedPathPls.ID) }) + + parentPls := model.Playlist{Name: "Parent", OwnerID: "userid", Rules: &criteria.Criteria{ + Expression: criteria.Any{ criteria.InPlaylist{"id": nestedPls.ID}, + criteria.InPlaylist{"path": nestedPathPls.Path}, }, }} Expect(repo.Put(&parentPls)).To(Succeed()) @@ -148,17 +159,88 @@ var _ = Describe("PlaylistRepository - Smart Playlists", func() { Expect(*pls.EvaluatedAt).To(BeTemporally("~", time.Now(), 2*time.Second)) // Parent should have tracks from the nested playlist - Expect(pls.Tracks).To(HaveLen(1)) + Expect(pls.Tracks).To(HaveLen(2)) Expect(pls.Tracks[0].MediaFileID).To(Equal(songDayInALife.ID)) - // Nested playlist should now have been refreshed (EvaluatedAt set) + // Nested playlists should now have been refreshed (EvaluatedAt set) nestedPlsAfterParentGet, err := repo.Get(nestedPls.ID) Expect(err).ToNot(HaveOccurred()) Expect(nestedPlsAfterParentGet.EvaluatedAt).ToNot(BeNil()) Expect(*nestedPlsAfterParentGet.EvaluatedAt).To(BeTemporally("~", time.Now(), 2*time.Second)) + + nestedPlsAfterParentGet, err = repo.Get(nestedPathPls.ID) + Expect(err).ToNot(HaveOccurred()) + Expect(nestedPlsAfterParentGet.EvaluatedAt).ToNot(BeNil()) + Expect(*nestedPlsAfterParentGet.EvaluatedAt).To(BeTemporally("~", time.Now(), 2*time.Second)) }) }) + It("does not recurse forever when two smart playlists reference each other", func() { + conf.Server.SmartPlaylistRefreshDelay = -1 * time.Second + + plsA := model.Playlist{Name: "Cycle A", OwnerID: "userid", Public: true, Rules: &criteria.Criteria{ + Expression: criteria.All{criteria.Contains{"title": "Day"}}, + }} + Expect(repo.Put(&plsA)).To(Succeed()) + DeferCleanup(func() { _ = repo.Delete(plsA.ID) }) + + plsB := model.Playlist{Name: "Cycle B", OwnerID: "userid", Public: true, Rules: &criteria.Criteria{ + Expression: criteria.All{criteria.InPlaylist{"id": plsA.ID}}, + }} + Expect(repo.Put(&plsB)).To(Succeed()) + DeferCleanup(func() { _ = repo.Delete(plsB.ID) }) + + plsA.Rules = &criteria.Criteria{Expression: criteria.All{criteria.InPlaylist{"id": plsB.ID}}} + Expect(repo.Put(&plsA)).To(Succeed()) + + _, err := repo.GetWithTracks(plsA.ID, true, false) + Expect(err).ToNot(HaveOccurred()) + }) + + It("does not treat an empty path as a reference to every playlist without a path", func() { + conf.Server.SmartPlaylistRefreshDelay = -1 * time.Second + + bystander := model.Playlist{Name: "Bystander", OwnerID: "userid", Public: true, Rules: &criteria.Criteria{ + Expression: criteria.All{criteria.Contains{"title": "Day"}}, + }} + Expect(repo.Put(&bystander)).To(Succeed()) + DeferCleanup(func() { _ = repo.Delete(bystander.ID) }) + + parent := model.Playlist{Name: "Empty Path", OwnerID: "userid", Public: true, Rules: &criteria.Criteria{ + Expression: criteria.All{criteria.InPlaylist{"path": ""}}, + }} + Expect(repo.Put(&parent)).To(Succeed()) + DeferCleanup(func() { _ = repo.Delete(parent.ID) }) + + _, err := repo.GetWithTracks(parent.ID, true, false) + Expect(err).ToNot(HaveOccurred()) + + reloaded, err := repo.Get(bystander.ID) + Expect(err).ToNot(HaveOccurred()) + Expect(reloaded.EvaluatedAt).To(BeNil()) + }) + + It("matches a child path stored in a different Unicode normalization form", func() { + conf.Server.SmartPlaylistRefreshDelay = -1 * time.Second + + child := model.Playlist{Name: "NFD Child", OwnerID: "userid", Public: true, Path: filepath.FromSlash("/mu\u0301sica/child.nsp"), Rules: &criteria.Criteria{ + Expression: criteria.All{criteria.Contains{"title": "Day"}}, + }} + Expect(repo.Put(&child)).To(Succeed()) + DeferCleanup(func() { _ = repo.Delete(child.ID) }) + + parent := model.Playlist{Name: "NFC Parent", OwnerID: "userid", Rules: &criteria.Criteria{ + Expression: criteria.All{criteria.InPlaylist{"path": "/m\u00fasica/child.nsp"}}, + }} + Expect(repo.Put(&parent)).To(Succeed()) + DeferCleanup(func() { _ = repo.Delete(parent.ID) }) + + pls, err := repo.GetWithTracks(parent.ID, true, false) + Expect(err).ToNot(HaveOccurred()) + Expect(pls.Tracks).To(HaveLen(1)) + Expect(pls.Tracks[0].MediaFileID).To(Equal(songDayInALife.ID)) + }) + When("refresh delay has not expired", func() { It("should NOT refresh tracks for smart playlist referenced in parent smart playlist criteria", func() { conf.Server.SmartPlaylistRefreshDelay = 1 * time.Hour diff --git a/persistence/sql_base_repository.go b/persistence/sql_base_repository.go index 9248f2a26..026e42b03 100644 --- a/persistence/sql_base_repository.go +++ b/persistence/sql_base_repository.go @@ -378,8 +378,9 @@ func wrapCursor[D, T any](cursor iter.Seq2[D, error], toModel func(D) *T) iter.S for row, err := range cursor { m := toModel(row) if m == nil { + // Don't format row: its String() derefs the nil model (golang/go#81238). var zero T - yield(zero, fmt.Errorf("unexpected nil %T (%v): %w", zero, row, err)) + yield(zero, fmt.Errorf("unexpected nil %T: %w", zero, err)) return } if !yield(*m, err) || err != nil { @@ -642,5 +643,9 @@ func (r sqlRepository) logSQL(sql string, args dbx.Params, err error, rowsAffect if code, extended, ok := db.ErrorCodes(err); ok { fields = append(fields, "sqliteCode", code, "sqliteExtended", extended) } + if db.IsBusy(err) && hasBusyRetry(r.ctx) { + log.Warn(append(fields, err)...) + return + } log.Error(append(fields, err)...) } diff --git a/persistence/sql_bookmarks.go b/persistence/sql_bookmarks.go index a9f53430d..cff57dc9d 100644 --- a/persistence/sql_bookmarks.go +++ b/persistence/sql_bookmarks.go @@ -103,6 +103,7 @@ func (r sqlRepository) GetBookmarks() (model.Bookmarks, error) { sq := r.newSelect().Columns(r.tableName + ".*") sq = r.withAnnotation(sq, idField) sq = r.withBookmark(sq, idField).Where(NotEq{bookmarkTable + ".item_id": nil}) + sq = r.applyLibraryFilter(sq) var mfs dbMediaFiles // TODO Decouple from media_file err := r.queryAll(sq, &mfs) if err != nil { diff --git a/persistence/sql_bookmarks_test.go b/persistence/sql_bookmarks_test.go index 712a928db..ae01a0e35 100644 --- a/persistence/sql_bookmarks_test.go +++ b/persistence/sql_bookmarks_test.go @@ -71,4 +71,52 @@ var _ = Describe("sqlBookmarks", func() { Expect(mr.GetBookmarks()).To(BeEmpty()) }) }) + + Describe("library access", func() { + var otherLib model.Library + var restrictedUser model.User + var adminCtx context.Context + var userMr model.MediaFileRepository + + BeforeEach(func() { + adminCtx, otherLib, restrictedUser = restrictedFixture("bmk") + + adminMr := NewMediaFileRepository(adminCtx, GetDBXBuilder()) + Expect(adminMr.Put(&model.MediaFile{ + ID: "bmk-otherlib-track", LibraryID: otherLib.ID, + Path: "hidden/bookmarked.mp3", Title: "Hidden Bookmarked", + })).To(Succeed()) + DeferCleanup(func() { _ = adminMr.Delete("bmk-otherlib-track") }) + + userCtx := request.WithUser(log.NewContext(GinkgoT().Context()), restrictedUser) + userMr = NewMediaFileRepository(userCtx, GetDBXBuilder()) + }) + + It("does not return bookmarks for tracks outside the user's libraries", func() { + Expect(userMr.AddBookmark("bmk-otherlib-track", "sneaky", 1)).To(Succeed()) + + Expect(userMr.GetBookmarks()).To(BeEmpty()) + }) + + It("still returns the bookmark for an admin", func() { + adminMr := NewMediaFileRepository(adminCtx, GetDBXBuilder()) + Expect(adminMr.AddBookmark("bmk-otherlib-track", "mine", 1)).To(Succeed()) + DeferCleanup(func() { _ = adminMr.DeleteBookmark("bmk-otherlib-track") }) + + bms, err := adminMr.GetBookmarks() + Expect(err).ToNot(HaveOccurred()) + Expect(bms).To(HaveLen(1)) + Expect(bms[0].Item.ID).To(Equal("bmk-otherlib-track")) + }) + + It("keeps returning bookmarks for tracks inside the user's libraries", func() { + Expect(userMr.AddBookmark(songAntenna.ID, "allowed", 5)).To(Succeed()) + DeferCleanup(func() { _ = userMr.DeleteBookmark(songAntenna.ID) }) + + bms, err := userMr.GetBookmarks() + Expect(err).ToNot(HaveOccurred()) + Expect(bms).To(HaveLen(1)) + Expect(bms[0].Item.ID).To(Equal(songAntenna.ID)) + }) + }) }) diff --git a/plugins/host_httpclient.go b/plugins/host_httpclient.go index 54875ae8a..6a9a4d2d6 100644 --- a/plugins/host_httpclient.go +++ b/plugins/host_httpclient.go @@ -16,6 +16,7 @@ import ( "github.com/navidrome/navidrome/log" "github.com/navidrome/navidrome/plugins/host" "github.com/navidrome/navidrome/utils/httpclient" + "github.com/navidrome/navidrome/utils/netguard" ) const ( @@ -53,6 +54,7 @@ func newHTTPService(pluginName string, permission *HTTPPermission) *httpServiceI Timeout: 30 * time.Second, KeepAlive: 30 * time.Second, Control: svc.dialControl, + Resolver: dialResolver, }).DialContext // No client timeout: it is set per-request via context deadline. svc.client = &http.Client{Transport: httpclient.NewTransport(svc.transport)} @@ -201,7 +203,7 @@ func isPrivateOrLoopback(hostname string) bool { if ip == nil { return false } - return isPrivateIP(ip) + return netguard.IsPrivateIP(ip) } // Verify interface implementation diff --git a/plugins/host_httpclient_test.go b/plugins/host_httpclient_test.go index 6edf75a97..81e3192fd 100644 --- a/plugins/host_httpclient_test.go +++ b/plugins/host_httpclient_test.go @@ -20,6 +20,10 @@ var _ = Describe("httpServiceImpl", func() { ts *httptest.Server ) + BeforeEach(func() { + stubLocalhostDNS() + }) + AfterEach(func() { if ts != nil { ts.Close() diff --git a/plugins/host_netguard.go b/plugins/host_netguard.go index 2244fed1f..ab092009a 100644 --- a/plugins/host_netguard.go +++ b/plugins/host_netguard.go @@ -4,8 +4,13 @@ import ( "fmt" "net" "slices" + + "github.com/navidrome/navidrome/utils/netguard" ) +// dialResolver is nil in production (the system resolver); tests swap in a stub to avoid real DNS. +var dialResolver *net.Resolver + // checkPrivateDial runs at dial time on the resolved IP, so hostnames can't reach private addresses unless a // literal IP/CIDR entry or a bare "*" (plugins targeting user-configured LAN services) allows it. func checkPrivateDial(requiredHosts []string, address string) error { @@ -17,7 +22,7 @@ func checkPrivateDial(requiredHosts []string, address string) error { return err } ip := net.ParseIP(host) - if ip == nil || !isPrivateIP(ip) { + if ip == nil || !netguard.IsPrivateIP(ip) { return nil } for _, entry := range requiredHosts { @@ -51,7 +56,3 @@ func ipMatchesEntry(entry string, ip net.IP) bool { } return false } - -func isPrivateIP(ip net.IP) bool { - return ip.IsLoopback() || ip.IsUnspecified() || ip.IsPrivate() || ip.IsLinkLocalUnicast() || ip.IsLinkLocalMulticast() -} diff --git a/plugins/host_taskqueue_test.go b/plugins/host_taskqueue_test.go index 72fb4cccd..993ddc7f6 100644 --- a/plugins/host_taskqueue_test.go +++ b/plugins/host_taskqueue_test.go @@ -689,11 +689,12 @@ var _ = Describe("TaskQueueService", func() { copy(times, dispatchTimes) mu.Unlock() - // Consecutive dispatches should have at least ~160ms gap (80% of 200ms) + // Wake-up latency varies per worker, so check offsets from the first dispatch, not gaps. for i := 1; i < len(times); i++ { - gap := times[i].Sub(times[i-1]) - Expect(gap).To(BeNumerically(">=", 160*time.Millisecond), - fmt.Sprintf("gap between dispatch %d and %d was %v, expected >= 160ms", i-1, i, gap)) + offset := times[i].Sub(times[0]) + minOffset := time.Duration(i)*200*time.Millisecond - 50*time.Millisecond + Expect(offset).To(BeNumerically(">=", minOffset), + fmt.Sprintf("dispatch %d ran %v after the first, expected >= %v", i, offset, minOffset)) } }) }) diff --git a/plugins/host_websocket.go b/plugins/host_websocket.go index 933e53144..d9d82665c 100644 --- a/plugins/host_websocket.go +++ b/plugins/host_websocket.go @@ -114,7 +114,7 @@ func (s *webSocketServiceImpl) Connect(ctx context.Context, urlStr string, heade // Establish WebSocket connection dialer := websocket.Dialer{ HandshakeTimeout: 30 * time.Second, - NetDialContext: (&net.Dialer{Control: s.dialControl}).DialContext, + NetDialContext: (&net.Dialer{Control: s.dialControl, Resolver: dialResolver}).DialContext, } conn, resp, err := dialer.DialContext(ctx, urlStr, httpHeaders) diff --git a/plugins/host_websocket_test.go b/plugins/host_websocket_test.go index 772d83cc9..9b94e4b70 100644 --- a/plugins/host_websocket_test.go +++ b/plugins/host_websocket_test.go @@ -505,6 +505,7 @@ var _ = Describe("WebSocketService", Ordered, func() { var savedHosts []string BeforeEach(func() { + stubLocalhostDNS() savedHosts = testService.requiredHosts upgrader := websocket.Upgrader{CheckOrigin: func(r *http.Request) bool { return true }} wsServer = httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { diff --git a/plugins/plugins_suite_test.go b/plugins/plugins_suite_test.go index cc1b45f97..aa07eae32 100644 --- a/plugins/plugins_suite_test.go +++ b/plugins/plugins_suite_test.go @@ -46,6 +46,12 @@ func TestPlugins(t *testing.T) { RunSpecs(t, "Plugins Suite") } +// stubLocalhostDNS resolves "localhost." without real DNS: the trailing dot never matches /etc/hosts. +func stubLocalhostDNS() { + dialResolver = tests.StubResolver(map[string]string{"localhost.": "127.0.0.1"}) + DeferCleanup(func() { dialResolver = nil }) +} + // createTestManager creates a new plugin Manager with the given plugin config. // It creates a temp directory, copies the test-metadata-agent plugin, and starts the manager. // Returns the manager, temp directory path, and a cleanup function. diff --git a/resources/i18n/de.json b/resources/i18n/de.json index 8f3df0b4d..400a2364d 100644 --- a/resources/i18n/de.json +++ b/resources/i18n/de.json @@ -94,7 +94,7 @@ "download": "Herunterladen", "info": "Mehr Informationen", "share": "Freigabe erstellen", - "refresh": "" + "refresh": "Metadaten aktualisieren" }, "lists": { "all": "Alle", @@ -157,12 +157,12 @@ "token": "Token", "lastAccessAt": "Letzter Zugriff am", "libraries": "Bibliotheken", - "scrobbleFilter": "" + "scrobbleFilter": "Scrobble-Filter" }, "helperTexts": { "name": "Die Änderung wird erst nach dem nächsten Login gültig", "libraries": "Wähle spezifische Bibliotheken für diesen Benutzer, oder leer lassen für Standard Bibliotheken", - "scrobbleFilter": "" + "scrobbleFilter": "Titel, die diesen Regeln für intelligente Wiedergabelisten entsprechen, werden nicht an Last.fm, ListenBrainz oder Scrobbler-Plugins übermittelt. Es werden dieselbe JSON-Syntax und dasselbe Verhalten wie bei intelligenten Wiedergabelisten verwendet. Beispiel: {\"all\":[{\"lt\":{\"rating\":4}}]}. Ist das Feld leer, wird alles gescrobbelt. Lokale Wiedergabezahlen bleiben davon unberührt." }, "notifications": { "created": "Benutzer erstellt", @@ -177,7 +177,7 @@ }, "validation": { "librariesRequired": "Mindestens eine Bibliothek muss für nicht-administrator Benutzer ausgewählt sein", - "invalidScrobbleFilter": "" + "invalidScrobbleFilter": "Es müssen gültige Regeln für intelligente Wiedergabelisten sein. Limit, Offset und Aktualisierungsverzögerung werden nicht unterstützt." } }, "player": { @@ -202,7 +202,7 @@ "command": "Befehl" }, "choices": { - "noDefaultBitRate": "" + "noDefaultBitRate": "Keine" } }, "playlist": { @@ -398,6 +398,7 @@ "invalidJson": "Konfiguration muss valides JSON sein" }, "messages": { + "idHelp": "Die Plugin-ID, abgeleitet vom Dateinamen. Verwende diese, um in Konfigurationen (z. B. bei Agents) auf dieses Plugin zu verweisen.", "configHelp": "Plugin mit Schlüssel-Werte Paaren konfigurieren. Leer lassen wenn das Plugin keine Konfiguration benötigt.", "clickPermissions": "Berechtigung anklicken für mehr Details", "noConfig": "Keine Konfiguration gesetzt", @@ -411,8 +412,7 @@ "requiredHosts": "Benötigte Hosts", "configValidationError": "Validierung der Konfiguration fehlgeschlagen:", "schemaRenderError": "Rendern der Konfiguration fehlgeschlagen. Das Schema das Plugins ist eventuell nicht korrekt.", - "allowWriteAccessHelp": "Wenn aktiviert, kann das Plugin Dateien in den Bibliotheken verändern. Als Standard haben Plugins nur Lesezugriff.", - "idHelp": "" + "allowWriteAccessHelp": "Wenn aktiviert, kann das Plugin Dateien in den Bibliotheken verändern. Als Standard haben Plugins nur Lesezugriff." }, "placeholders": { "configKey": "Schlüssel", @@ -573,7 +573,7 @@ "noPlaylistsAvailable": "Keine Wiedergabeliste verfügbar", "delete_user_title": "Benutzer '%{name}' löschen", "delete_user_content": "Möchtest du diesen Benutzer und alle seine Daten (einschließlich Wiedergabelisten und Einstellungen) wirklich löschen?", - "notifications_blocked": "Sie haben Benachrichtigungen für diese Seite in den Einstellungen Ihres Browsers blockiert", + "notifications_blocked": "Benachrichtigungen für diese Seite sind in den Browsereinstellungen blockiert", "notifications_not_available": "Dieser Browser unterstützt keine Desktop-Benachrichtigungen", "lastfmLinkSuccess": "Last.fm Verbindung hergestellt und scrobbling aktiviert", "lastfmLinkFailure": "Last.fm konnte nicht verbunden werden", @@ -609,7 +609,7 @@ "coverRemoved": "Cover entfernt", "coverUploadError": "Fehler beim Hochladen des Covers", "coverRemoveError": "Fehler beim Entfernen des Covers", - "metadataRefreshStarted": "" + "metadataRefreshStarted": "Aktualisiert Metadaten im Hintergrund" }, "menu": { "library": "Bibliothek", @@ -731,4 +731,4 @@ "empty": "Keine Wiedergabe", "minutesAgo": "Vor %{smart_count} Minute |||| Vor %{smart_count} Minuten" } -} \ No newline at end of file +} diff --git a/resources/i18n/fi.json b/resources/i18n/fi.json index 3d1edbbd5..f79d71f94 100644 --- a/resources/i18n/fi.json +++ b/resources/i18n/fi.json @@ -202,7 +202,7 @@ "command": "Komento" }, "choices": { - "noDefaultBitRate": "" + "noDefaultBitRate": "Ei mikään" } }, "playlist": { @@ -609,7 +609,10 @@ "coverRemoved": "Kansikuva poistettu", "coverUploadError": "Virhe ladattaessa kansikuvaa", "coverRemoveError": "Virhe poistettaessa kansikuvaa", - "metadataRefreshStarted": "Metatietoja päivitetään taustalla" + "metadataRefreshStarted": "Metatietoja päivitetään taustalla", + "quickConnectApproved": "", + "quickConnectInvalidCode": "", + "quickConnectError": "" }, "menu": { "library": "Kirjasto", @@ -645,7 +648,15 @@ "selectLibraries": "Valitse kirjastot", "none": "Ei mitään" }, - "onlyFavourites": "Näytä vain suosikit" + "onlyFavourites": "Näytä vain suosikit", + "quickConnect": { + "name": "", + "code": "", + "help": "", + "confirm": "", + "continue": "", + "approve": "" + } }, "player": { "playListsText": "Jono", diff --git a/resources/i18n/nl.json b/resources/i18n/nl.json index 46c3df9de..cb276b8f4 100644 --- a/resources/i18n/nl.json +++ b/resources/i18n/nl.json @@ -93,7 +93,8 @@ "addToPlaylist": "Toevoegen aan afspeellijst", "download": "Downloaden", "info": "Meer info", - "share": "Delen" + "share": "Delen", + "refresh": "Metadata verversen" }, "lists": { "all": "Alle", @@ -155,11 +156,13 @@ "newPassword": "Nieuw wachtwoord", "token": "Token", "lastAccessAt": "Meest recente toegang", - "libraries": "Bibliotheken" + "libraries": "Bibliotheken", + "scrobbleFilter": "Scrobble filter" }, "helperTexts": { "name": "Naamswijziging wordt pas zichtbaar bij de volgende login", - "libraries": "Selecteer specifieke bibliotheken voor deze gebruiker, of laat leeg om de standaardbiblliotheken te gebruiken" + "libraries": "Selecteer specifieke bibliotheken voor deze gebruiker, of laat leeg om de standaardbiblliotheken te gebruiken", + "scrobbleFilter": "Nummers binnen deze slimme afspeellijst regels worden niet gestuurd naar Last.fm, ListenBrainz of scrobbler plugins. Gebruikt dezelfde JSON syntaxis en gedrag als slimme afspeellijsten. Voorbeeld: {\"all\":[{\"lt\":{\"rating\":4}}]}. Leeglaten om alles te scrobblen. Lokale afspeelaantallen vallen hierbuiten." }, "notifications": { "created": "Aangemaakt door gebruiker", @@ -173,7 +176,8 @@ "adminAutoLibraries": "Admin gebruikers hebben automatisch toegang tot alle bibliotheken" }, "validation": { - "librariesRequired": "Minstens één bibliotheek moet geselecteerd worden voor niet-admin gebruikers" + "librariesRequired": "Minstens één bibliotheek moet geselecteerd worden voor niet-admin gebruikers", + "invalidScrobbleFilter": "Moeten geldige slimme afspeellijst regels zijn. Geen ondersteuning voor Limit, Offset en Refresh Delay" } }, "player": { @@ -196,6 +200,9 @@ "targetFormat": "Doelformaat", "defaultBitRate": "Standaard bitrate", "command": "Commando" + }, + "choices": { + "noDefaultBitRate": "Geen" } }, "playlist": { @@ -210,7 +217,8 @@ "songCount": "Nummers", "comment": "Commentaar", "sync": "Auto-importeren", - "path": "Importeer vanuit" + "path": "Importeer vanuit", + "starred": "Favoriet" }, "actions": { "selectPlaylist": "Selecteer een afspeellijst:", @@ -403,7 +411,8 @@ "requiredHosts": "Benodigde hosts", "configValidationError": "Configuratiecheck mislukt", "schemaRenderError": "Kan het configuratieformulier niet verwerken. Het plugin schema is wellicht ongeldig.", - "allowWriteAccessHelp": "Met dit ingeschakeld, kan de plug-in bestanden bewerken in de bibliotheekmappen. Standaard kunnen plug-ins alleen lezen." + "allowWriteAccessHelp": "Met dit ingeschakeld, kan de plug-in bestanden bewerken in de bibliotheekmappen. Standaard kunnen plug-ins alleen lezen.", + "idHelp": "De plugin ID, afgeleid van zijn bestandsnaam. Gebruik dit als gerefereerd wordt aan deze plugin in de configuratie opties, zoals Agenten." }, "placeholders": { "configKey": "Sleutel", @@ -599,7 +608,11 @@ "coverUploaded": "Albumhoes bijgewerkt", "coverRemoved": "Albumhoes verwijderd", "coverUploadError": "Fout bij het toevoegen albumhoes", - "coverRemoveError": "Fout bij verwijderen albumhoes" + "coverRemoveError": "Fout bij verwijderen albumhoes", + "metadataRefreshStarted": "Metadata verversen op de achtergrond", + "quickConnectApproved": "", + "quickConnectInvalidCode": "", + "quickConnectError": "" }, "menu": { "library": "Bibliotheek", @@ -634,6 +647,15 @@ "multipleLibraries": "%{selected} van %{total} bibliotheken", "selectLibraries": "Selecteer bibliotheken", "none": "Geen" + }, + "onlyFavourites": "Toon alleen favorieten", + "quickConnect": { + "name": "", + "code": "", + "help": "", + "confirm": "", + "continue": "", + "approve": "" } }, "player": { diff --git a/resources/i18n/pt-br.json b/resources/i18n/pt-br.json index 2584da6cd..6fcde56ca 100644 --- a/resources/i18n/pt-br.json +++ b/resources/i18n/pt-br.json @@ -202,7 +202,7 @@ "command": "Comando" }, "choices": { - "noDefaultBitRate": "" + "noDefaultBitRate": "Nenhum" } }, "playlist": { @@ -609,7 +609,10 @@ "coverRemoved": "Capa removida", "coverUploadError": "Erro ao enviar capa", "coverRemoveError": "Erro ao remover capa", - "metadataRefreshStarted": "Atualizando metadados em segundo plano" + "metadataRefreshStarted": "Atualizando metadados em segundo plano", + "quickConnectApproved": "%{app} em %{device} está conectado agora", + "quickConnectInvalidCode": "Código inválido ou expirado", + "quickConnectError": "Não foi possível aprovar o código" }, "menu": { "library": "Biblioteca", @@ -645,7 +648,15 @@ "selectLibraries": "Selecionar Bibliotecas", "none": "Nenhuma" }, - "onlyFavourites": "Somente favoritas" + "onlyFavourites": "Somente favoritas", + "quickConnect": { + "name": "Conexão Rápida", + "code": "Código", + "help": "Digite o código exibido por um aplicativo Jellyfin para conectá-lo à sua conta", + "confirm": "Conectar %{app} %{version} em %{device} à sua conta?", + "continue": "Continuar", + "approve": "Aprovar" + } }, "player": { "playListsText": "Fila de Execução", diff --git a/resources/i18n/sv.json b/resources/i18n/sv.json index 228cc2cf3..ff4f53a6b 100644 --- a/resources/i18n/sv.json +++ b/resources/i18n/sv.json @@ -11,22 +11,22 @@ "title": "Titel", "artist": "Artist", "album": "Album", - "path": "Sökväg", + "path": "Filsökväg", "genre": "Genre", - "compilation": "Samling", + "compilation": "Samlingsalbum", "year": "År", "size": "Filstorlek", "updatedAt": "Uppdaterad", - "bitRate": "Bitrate", - "discSubtitle": "Underrubrik", + "bitRate": "Bithastighet", + "discSubtitle": "Skivans undertitel", "starred": "Favorit", "comment": "Kommentar", "rating": "Betyg", "quality": "Kvalitet", "bpm": "BPM", "playDate": "Senast spelad", - "channels": "Channels", - "createdAt": "Skapad", + "channels": "Kanaler", + "createdAt": "Tillagd", "grouping": "Gruppering", "mood": "Stämning", "participants": "Ytterligare medverkande", @@ -35,19 +35,21 @@ "rawTags": "Omodifierade taggar", "bitDepth": "Bitdjup", "sampleRate": "Samplingsfrekvens", - "missing": "Saknade", + "missing": "Saknas", "libraryName": "Bibliotek", "composer": "Kompositör", - "disc": "Disc %{discNumber}" + "disc": "Skiva %{discNumber}", + "albumGain": "Volymjustering (album)", + "trackGain": "Volymjustering (låt)" }, "actions": { "addToQueue": "Lägg till i kön", "playNow": "Spela nu", "addToPlaylist": "Lägg till i spellista", - "shuffleAll": "Shuffle", + "shuffleAll": "Blanda alla", "download": "Ladda ner", - "playNext": "Spela nästa", - "info": "Mer information", + "playNext": "Spela härnäst", + "info": "Visa information", "showInPlaylist": "Visa i spellista", "instantMix": "Direktmix" } @@ -62,12 +64,12 @@ "playCount": "Spelningar", "name": "Namn", "genre": "Genre", - "compilation": "Samling", + "compilation": "Samlingsalbum", "year": "År", "updatedAt": "Uppdaterad", "comment": "Kommentar", "rating": "Betyg", - "createdAt": "Skapad", + "createdAt": "Tillagt", "size": "Storlek", "originalDate": "Originaldatum", "releaseDate": "Utgivningsdatum", @@ -80,27 +82,28 @@ "media": "Media", "mood": "Stämning", "date": "Inspelningsdatum", - "missing": "Saknade", + "missing": "Saknas", "libraryName": "Bibliotek" }, "actions": { "playAll": "Spela", "playNext": "Spela härnäst", "addToQueue": "Lägg till i kön", - "shuffle": "Shuffle", + "shuffle": "Blanda", "addToPlaylist": "Lägg till i spellista", "download": "Ladda ner", - "info": "Mer information", - "share": "Dela" + "info": "Visa information", + "share": "Dela", + "refresh": "Uppdatera metadata" }, "lists": { "all": "Alla", - "random": "Blanda", - "recentlyAdded": "Senast tillagda", - "recentlyPlayed": "Senast spelade", + "random": "Slumpmässiga", + "recentlyAdded": "Nyligen tillagda", + "recentlyPlayed": "Nyligen spelade", "mostPlayed": "Mest spelade", "starred": "Favoriter", - "topRated": "Bästa betyg" + "topRated": "Högst betygsatta" } }, "artist": { @@ -114,26 +117,26 @@ "genre": "Genre", "size": "Storlek", "role": "Roll", - "missing": "Saknade" + "missing": "Saknas" }, "roles": { "albumartist": "Albumartist |||| Albumartister", "artist": "Artist |||| Artister", - "composer": "Kompositör |||| Kompositörer", + "composer": "Kompositör |||| Kompositörer", "conductor": "Dirigent |||| Dirigenter", "lyricist": "Textförfattare |||| Textförfattare", "arranger": "Arrangör |||| Arrangörer", "producer": "Producent |||| Producenter", "director": "Inspelningsledare |||| Inspelningsledare", "engineer": "Ljudtekniker |||| Ljudtekniker", - "mixer": "Mixare |||| Mixare", - "remixer": "Remixare |||| Remixare", - "djmixer": "DJ-mixare |||| DJ-mixare", - "performer": "Utövande artist |||| Utövande artister", - "maincredit": "Albumartister eller Artist |||| Albumartister eller Artister" + "mixer": "Mixare |||| Mixare", + "remixer": "Remixare |||| Remixare", + "djmixer": "DJ-mixare |||| DJ-mixare", + "performer": "Utövande artist |||| Utövande artister", + "maincredit": "Albumartist eller artist |||| Albumartister eller artister" }, "actions": { - "shuffle": "Shuffle", + "shuffle": "Blanda", "radio": "Radio", "topSongs": "Topplåtar" } @@ -142,7 +145,7 @@ "name": "Användare |||| Användare", "fields": { "userName": "Användarnamn", - "isAdmin": "Är admin", + "isAdmin": "Administratör", "lastLoginAt": "Senaste inloggning", "updatedAt": "Uppdaterad", "name": "Namn", @@ -151,13 +154,15 @@ "changePassword": "Byt lösenord?", "currentPassword": "Nuvarande lösenord", "newPassword": "Nytt lösenord", - "token": "Token", + "token": "Åtkomsttoken", "lastAccessAt": "Senaste åtkomst", - "libraries": "Bibliotek" + "libraries": "Bibliotek", + "scrobbleFilter": "Scrobblingsfilter" }, "helperTexts": { "name": "Ändringar av ditt namn syns först vid nästa inloggning", - "libraries": "Välj ett bibliotek för denna användare eller lämna blankt för standardbibliotek" + "libraries": "Välj vilka bibliotek användaren ska ha tillgång till, eller lämna tomt för att använda standardbiblioteken", + "scrobbleFilter": "Låtar som matchar dessa regler för smarta spellistor skickas inte till Last.fm, ListenBrainz eller scrobblingtillägg. Samma JSON-syntax och beteende som för smarta spellistor används. Exempel: {\"all\":[{\"lt\":{\"rating\":4}}]}. Lämna tomt för att scrobbla allt. Antalet spelningar som lagras lokalt påverkas inte." }, "notifications": { "created": "Användare skapad", @@ -165,13 +170,14 @@ "deleted": "Användare borttagen" }, "message": { - "listenBrainzToken": "Ange din ListenBrainz användar-token.", + "listenBrainzToken": "Ange din användartoken för ListenBrainz.", "clickHereForToken": "Klicka här för att hämta din token", "selectAllLibraries": "Välj alla bibliotek", - "adminAutoLibraries": "Administratörer har automatiskt tillgång till alla bibliotek" + "adminAutoLibraries": "Administratörer har automatiskt åtkomst till alla bibliotek" }, "validation": { - "librariesRequired": "Minst ett bibliotek måste väljas för icke-administratörer" + "librariesRequired": "Minst ett bibliotek måste väljas för användare som inte är administratörer", + "invalidScrobbleFilter": "Måste innehålla giltiga regler för smarta spellistor. Begränsning av antalet låtar, förskjutning och uppdateringsfördröjning stöds inte." } }, "player": { @@ -179,12 +185,12 @@ "fields": { "name": "Namn", "transcodingId": "Omkodning", - "maxBitRate": "Max. bitrate", + "maxBitRate": "Högsta bithastighet", "client": "Klient", "userName": "Användarnamn", "lastSeen": "Senast sedd", - "reportRealPath": "Visa hela sökvägen", - "scrobbleEnabled": "Scrobbla till extern tjänst" + "reportRealPath": "Rapportera den faktiska sökvägen", + "scrobbleEnabled": "Scrobbla till externa tjänster" } }, "transcoding": { @@ -192,8 +198,11 @@ "fields": { "name": "Namn", "targetFormat": "Målformat", - "defaultBitRate": "Standardbitrate", + "defaultBitRate": "Standardbithastighet", "command": "Kommando" + }, + "choices": { + "noDefaultBitRate": "Ingen" } }, "playlist": { @@ -207,8 +216,9 @@ "createdAt": "Skapad", "songCount": "Låtar", "comment": "Kommentar", - "sync": "Auto-import", - "path": "Importera från" + "sync": "Automatisk import", + "path": "Importera från", + "starred": "Favorit" }, "actions": { "selectPlaylist": "Välj en spellista:", @@ -216,24 +226,24 @@ "export": "Exportera", "makePublic": "Gör offentlig", "makePrivate": "Gör privat", - "saveQueue": "Spara kö till spellista", - "searchOrCreate": "Sök spellista eller skapa ny...", - "pressEnterToCreate": "Tryck Enter för att skapa ny spellista", - "removeFromSelection": "Ta bort från urval" + "saveQueue": "Spara kön i en spellista", + "searchOrCreate": "Sök spellistor eller skriv för att skapa en ny...", + "pressEnterToCreate": "Tryck på Enter för att skapa en ny spellista", + "removeFromSelection": "Ta bort från urvalet" }, "message": { - "duplicate_song": "Lägg till dubletter", - "song_exist": "Vissa låtar finns redan i spellistan. Vill du lägga till dubbletterna eller hoppa över dem?", - "noPlaylistsFound": "Hittade inga spellistor", + "duplicate_song": "Lägg till dubbletter", + "song_exist": "Du håller på att lägga till dubbletter i spellistan. Vill du lägga till dem eller hoppa över dem?", + "noPlaylistsFound": "Inga spellistor hittades", "noPlaylists": "Inga spellistor tillgängliga" } }, "radio": { - "name": "Radio |||| Radior", + "name": "Radiostation |||| Radiostationer", "fields": { "name": "Namn", - "streamUrl": "Stream-URL", - "homePageUrl": "Hemside-URL", + "streamUrl": "Strömmens URL", + "homePageUrl": "Webbplatsens URL", "updatedAt": "Uppdaterad", "createdAt": "Skapad" }, @@ -242,7 +252,7 @@ } }, "share": { - "name": "Dela |||| Delningar", + "name": "Delning |||| Delningar", "fields": { "username": "Delad av", "url": "URL", @@ -252,11 +262,13 @@ "lastVisitedAt": "Senast besökt", "visitCount": "Besök", "format": "Format", - "maxBitRate": "Max. bitrate", + "maxBitRate": "Högsta bithastighet", "updatedAt": "Uppdaterad", "createdAt": "Skapad", - "downloadable": "Tillåt nedladdning?" - } + "downloadable": "Tillåt nedladdningar?" + }, + "notifications": {}, + "actions": {} }, "missing": { "name": "Saknad fil |||| Saknade filer", @@ -267,11 +279,11 @@ "libraryName": "Bibliotek" }, "actions": { - "remove": "Radera", - "remove_all": "Radera alla" + "remove": "Ta bort", + "remove_all": "Ta bort alla" }, "notifications": { - "removed": "Saknade fil(er) borttagna" + "removed": "Saknade filer har tagits bort" }, "empty": "Inga saknade filer" }, @@ -280,8 +292,8 @@ "fields": { "name": "Namn", "path": "Sökväg", - "remotePath": "Ta bort sökväg", - "lastScanAt": "Senaste scan", + "remotePath": "Fjärrsökväg", + "lastScanAt": "Senaste skanning", "songCount": "Låtar", "albumCount": "Album", "artistCount": "Artister", @@ -302,33 +314,33 @@ "statistics": "Statistik" }, "actions": { - "scan": "Scanna bibliotek", + "scan": "Skanna biblioteket", "manageUsers": "Hantera användaråtkomst", - "viewDetails": "Se detaljer", - "quickScan": "Snabbscan", - "fullScan": "Komplett scan" + "viewDetails": "Visa detaljer", + "quickScan": "Snabbskanning", + "fullScan": "Fullständig skanning" }, "notifications": { "created": "Biblioteket har skapats", "updated": "Biblioteket har uppdaterats", - "deleted": "Biblioteket har raderats", - "scanStarted": "Biblioteksscan startad", - "scanCompleted": "Biblioteksscan avslutad", - "quickScanStarted": "Snabbscan startad", - "fullScanStarted": "Komplett scan startad", - "scanError": "Fel vid start av scan. Se loggarna" + "deleted": "Biblioteket har tagits bort", + "scanStarted": "Skanning av biblioteket har startat", + "scanCompleted": "Skanning av biblioteket är klar", + "quickScanStarted": "Snabbskanning har startat", + "fullScanStarted": "Fullständig skanning har startat", + "scanError": "Kunde inte starta skanningen. Kontrollera loggarna" }, "validation": { - "nameRequired": "Biblioteksnamn krävs", - "pathRequired": "Bibliotekssökväg krävs", - "pathNotDirectory": "Bibliotekssökvägen måste vara en katalog", - "pathNotFound": "Bibliotekssökväg hittades inte", - "pathNotAccessible": "Bibliotekssökväg inte tillgänglig", - "pathInvalid": "Ogiltig bibliotekssökväg" + "nameRequired": "Ange ett biblioteksnamn", + "pathRequired": "Ange en sökväg till biblioteket", + "pathNotDirectory": "Bibliotekets sökväg måste peka på en katalog", + "pathNotFound": "Bibliotekets sökväg hittades inte", + "pathNotAccessible": "Bibliotekets sökväg är inte åtkomlig", + "pathInvalid": "Ogiltig sökväg till biblioteket" }, "messages": { - "deleteConfirm": "Är du säker på att du vill ta bort detta bibliotek? Detta raderar all förbunden data och användartillgång.", - "scanInProgress": "Scanning pågår...", + "deleteConfirm": "Är du säker på att du vill ta bort det här biblioteket? Alla tillhörande data och användarnas åtkomst till biblioteket tas bort.", + "scanInProgress": "Skanning pågår...", "noLibrariesAssigned": "Inga bibliotek har tilldelats den här användaren" } }, @@ -339,44 +351,44 @@ "name": "Namn", "description": "Beskrivning", "version": "Version", - "author": "Författare", - "website": "Website", + "author": "Upphovsperson", + "website": "Webbplats", "permissions": "Behörigheter", - "enabled": "Aktiverad", + "enabled": "Aktiverat", "status": "Status", "path": "Sökväg", "lastError": "Fel", "hasError": "Fel", - "updatedAt": "Uppdaterad", - "createdAt": "Installerad", + "updatedAt": "Uppdaterat", + "createdAt": "Installerat", "configKey": "Nyckel", "configValue": "Värde", "allUsers": "Tillåt alla användare", "selectedUsers": "Valda användare", "allLibraries": "Tillåt alla bibliotek", "selectedLibraries": "Valda bibliotek", - "allowWriteAccess": "Tillåt skrivrättigheter" + "allowWriteAccess": "Tillåt skrivåtkomst" }, "sections": { "status": "Status", - "info": "Tilläggsinformation", + "info": "Information om tillägget", "configuration": "Konfiguration", "manifest": "Manifest", - "usersPermission": "Användarbehörigheter", - "libraryPermission": "Biblioteksbehörigheter" + "usersPermission": "Åtkomst till användare", + "libraryPermission": "Åtkomst till bibliotek" }, "status": { - "enabled": "Aktiverad", - "disabled": "Inaktiverad" + "enabled": "Aktiverat", + "disabled": "Inaktiverat" }, "actions": { "enable": "Aktivera", "disable": "Inaktivera", - "disabledDueToError": "Åtgärda felet innan aktivering", + "disabledDueToError": "Åtgärda felet före aktivering", "disabledUsersRequired": "Välj användare före aktivering", "disabledLibrariesRequired": "Välj bibliotek före aktivering", "addConfig": "Lägg till konfiguration", - "rescan": "Scanna om" + "rescan": "Skanna om" }, "notifications": { "enabled": "Tillägg aktiverat", @@ -391,17 +403,18 @@ "configHelp": "Konfigurera tillägget med nyckel–värde-par. Lämna tomt om tillägget inte kräver någon konfiguration.", "clickPermissions": "Klicka på en behörighet för mer information", "noConfig": "Ingen konfiguration angiven", - "allUsersHelp": "När den är aktiverad får tillägget tillgång till alla användare, inklusive de som skapas i framtiden.", + "allUsersHelp": "När alternativet är aktiverat får tillägget åtkomst till alla användare, även de som skapas i framtiden.", "noUsers": "Inga användare valda", "permissionReason": "Orsak", - "usersRequired": "Detta tillägg kräver åtkomst till användarinformation. Välj vilka användare insticksprogrammet ska ha åtkomst till, eller aktivera 'Tillåt alla användare'.", - "allLibrariesHelp": "När den är aktiverad får tillägget tillgång till alla bibliotek, inklusive de som skapas i framtiden.", + "usersRequired": "Det här tillägget behöver åtkomst till användarinformation. Välj vilka användare tillägget får åtkomst till eller aktivera 'Tillåt alla användare'.", + "allLibrariesHelp": "När alternativet är aktiverat får tillägget åtkomst till alla bibliotek, även de som skapas i framtiden.", "noLibraries": "Inga bibliotek valda", - "librariesRequired": "Detta tillägg kräver tillgång till biblioteksinformation. Välj vilka bibliotek tillägget kan komma åt eller aktivera 'Tillåt alla bibliotek'.", - "requiredHosts": "Krävda värdar", + "librariesRequired": "Det här tillägget behöver åtkomst till biblioteksinformation. Välj vilka bibliotek tillägget får åtkomst till eller aktivera 'Tillåt alla bibliotek'.", + "requiredHosts": "Värdar som krävs", "configValidationError": "Validering av konfigurationen misslyckades:", - "schemaRenderError": "Kunde inte rendera konfigurationsformuläret. Tilläggets schema kan vara ogiltigt.", - "allowWriteAccessHelp": "När detta är aktiverat kan tillägget ändra filer i bibliotekets kataloger. Som standard har tillägget endast läsrättigheter." + "schemaRenderError": "Kunde inte visa konfigurationsformuläret. Tilläggets schema kan vara ogiltigt.", + "allowWriteAccessHelp": "När alternativet är aktiverat kan tillägget ändra filer i bibliotekens kataloger. Som standard har tillägg endast läsåtkomst.", + "idHelp": "Tilläggets ID hämtas från filnamnet. Använd det när du hänvisar till tillägget i konfigurationsalternativ, till exempel Agents." }, "placeholders": { "configKey": "nyckel", @@ -412,40 +425,40 @@ "ra": { "auth": { "welcome1": "Tack för att du installerade Navidrome!", - "welcome2": "Skapa först ett admin-konto", + "welcome2": "Börja med att skapa ett administratörskonto", "confirmPassword": "Bekräfta lösenord", - "buttonCreateAdmin": "Skapa admin-konto", + "buttonCreateAdmin": "Skapa administratörskonto", "auth_check_error": "Logga in för att fortsätta", "user_menu": "Profil", "username": "Användarnamn", "password": "Lösenord", "sign_in": "Logga in", - "sign_in_error": "Felaktig inloggning, försök igen", + "sign_in_error": "Inloggningen misslyckades. Försök igen", "logout": "Logga ut", - "insightsCollectionNote": "Navidrome samlar anonym användardata för att\nhjälpa projektet att bli bättre. Klicka [här]\nför att läsa mer och avaktivera om du vill" + "insightsCollectionNote": "Navidrome samlar in anonyma användningsdata för\natt förbättra projektet. Klicka [här] för att läsa mer\noch välja bort insamlingen" }, "validation": { "invalidChars": "Använd enbart bokstäver och siffror", - "passwordDoesNotMatch": "Lösenordet matchar inte", - "required": "Krävs", - "minLength": "Måste ha minst %{min} tecken", - "maxLength": "Får maximalt ha %{max} tecken", + "passwordDoesNotMatch": "Lösenorden stämmer inte överens", + "required": "Obligatoriskt", + "minLength": "Måste innehålla minst %{min} tecken", + "maxLength": "Får innehålla högst %{max} tecken", "minValue": "Måste vara minst %{min}", - "maxValue": "Får maximalt vara %{max}", - "number": "Måste vara ett nummer", + "maxValue": "Får vara högst %{max}", + "number": "Måste vara ett tal", "email": "Måste vara en giltig e-postadress", - "oneOf": "Måste vara en av: %{options}", - "regex": "Måste matcha ett specifikt format (regexp): %{pattern}", - "unique": "Måste vara unik", + "oneOf": "Måste vara något av: %{options}", + "regex": "Måste matcha det reguljära uttrycket: %{pattern}", + "unique": "Måste vara unikt", "url": "Måste vara en giltig URL" }, "action": { "add_filter": "Lägg till filter", "add": "Lägg till", "back": "Tillbaka", - "bulk_actions": "1 objekt vald |||| %{smart_count} objekt valda", + "bulk_actions": "1 objekt valt |||| %{smart_count} objekt valda", "cancel": "Avbryt", - "clear_input_value": "Rensa", + "clear_input_value": "Rensa värdet", "clone": "Klona", "confirm": "Bekräfta", "create": "Skapa", @@ -454,8 +467,8 @@ "export": "Exportera", "list": "Lista", "refresh": "Uppdatera", - "remove_filter": "Ta bort filter", - "remove": "Radera", + "remove_filter": "Ta bort det här filtret", + "remove": "Ta bort", "save": "Spara", "search": "Sök", "show": "Visa", @@ -477,15 +490,15 @@ }, "page": { "create": "Skapa %{name}", - "dashboard": "Dashboard", + "dashboard": "Översikt", "edit": "%{name} #%{id}", "error": "Ett fel uppstod", "list": "%{name}", - "loading": "Laddar", - "not_found": "Hittade inget", + "loading": "Läser in", + "not_found": "Hittades inte", "show": "%{name} #%{id}", - "empty": "Ingen %{name} ännu.", - "invite": "Vill du lägga till en?" + "empty": "%{name}: inga poster ännu.", + "invite": "Vill du lägga till något?" }, "input": { "file": { @@ -497,13 +510,13 @@ "upload_single": "Dra och släpp en bild som ska laddas upp eller klicka för att välja en bild." }, "references": { - "all_missing": "Hittade ingen referensdata.", - "many_missing": "Minst en av de associerade referenserna verkar inte längre vara tillgänglig.", - "single_missing": "Associerade referenser verkar inte längre vara tillgängliga." + "all_missing": "Kunde inte hitta referensdata.", + "many_missing": "Minst en av de kopplade referenserna verkar inte längre vara tillgänglig.", + "single_missing": "Den kopplade referensen verkar inte längre vara tillgänglig." }, "password": { - "toggle_visible": "Dölj password", - "toggle_hidden": "Visa password" + "toggle_visible": "Dölj lösenord", + "toggle_hidden": "Visa lösenord" } }, "message": { @@ -511,16 +524,16 @@ "are_you_sure": "Är du säker?", "bulk_delete_content": "Vill du verkligen ta bort %{name}? |||| Vill du verkligen ta bort dessa %{smart_count} objekt?", "bulk_delete_title": "Ta bort %{name} |||| Ta bort %{smart_count} %{name}", - "delete_content": "Vill du verkligen ta bort detta innehåll?", + "delete_content": "Vill du verkligen ta bort det här objektet?", "delete_title": "Ta bort %{name} #%{id}", "details": "Detaljer", "error": "Ett klientfel uppstod och begäran kunde inte slutföras.", - "invalid_form": "Formuläret är ogiltigt. Kontrollera eventuella fel", - "loading": "Sidan läses in, var god vänta", + "invalid_form": "Formuläret innehåller fel. Kontrollera uppgifterna", + "loading": "Sidan läses in. Vänta ett ögonblick", "no": "Nej", "not_found": "Antingen skrev du fel URL eller så följde du en ogiltig länk.", "yes": "Ja", - "unsaved_changes": "Du har osparade ändringar. Ignorera dem?" + "unsaved_changes": "Vissa ändringar har inte sparats. Vill du verkligen ignorera dem?" }, "navigation": { "no_results": "Inga resultat hittades", @@ -529,23 +542,23 @@ "page_out_from_end": "Det finns inga fler sidor", "page_out_from_begin": "Det finns ingen sida före sida 1", "page_range_info": "%{offsetBegin}-%{offsetEnd} av %{total}", - "page_rows_per_page": "Antal per sida:", + "page_rows_per_page": "Objekt per sida:", "next": "Nästa", "prev": "Föregående", "skip_nav": "Hoppa till innehåll" }, "notification": { - "updated": "Element uppdaterat |||| %{smart_count} element uppdaterade", - "created": "Element skapat", - "deleted": "Element borttaget |||| %{smart_count} element borttagna", - "bad_item": "Felaktigt element", - "item_doesnt_exist": "Element finns inte", + "updated": "Objekt uppdaterat |||| %{smart_count} objekt uppdaterade", + "created": "Objekt skapat", + "deleted": "Objekt borttaget |||| %{smart_count} objekt borttagna", + "bad_item": "Felaktigt objekt", + "item_doesnt_exist": "Objektet finns inte", "http_error": "Kommunikationsfel med servern", - "data_provider_error": "Fel i dataProvider. Kontrollera din konsol för mer information.", - "i18n_error": "Kunde inte läsa in översättningen av det valda språket", + "data_provider_error": "Fel i dataProvider. Kontrollera webbläsarens konsol för mer information.", + "i18n_error": "Kunde inte läsa in översättningarna för det valda språket", "canceled": "Åtgärden avbröts", - "logged_out": "Sessionen har avslutats, anslut på nytt.", - "new_version": "Det finns en ny version! Uppdatera detta fönster." + "logged_out": "Sessionen har avslutats. Logga in igen.", + "new_version": "En ny version finns tillgänglig! Ladda om det här fönstret." }, "toggleFieldsMenu": { "columnsToDisplay": "Kolumner att visa", @@ -556,39 +569,39 @@ }, "message": { "note": "OBSERVERA", - "transcodingDisabled": "Inställning för kodning via webbgränssnittet är av säkerhetsskäl ej aktiverat. Starta om servern med alternativet %{config} markerat om du vill göra ändringar (redigera eller lägga till).", - "transcodingEnabled": "Navidrome körs för närvarande med %{config}, vilket gör att systemkommandon kan köras från webbplattformen. Du rekommenderas av säkerhetsskäl att du stänger av den och bara slår på den när du ställer in omkodning.", - "songsAddedToPlaylist": "La till en låt i spellistan |||| La till %{smart_count} låtar i spellistan", - "noPlaylistsAvailable": "Ingen tillgänglig", - "delete_user_title": "Ta bort användare '%{name}'", - "delete_user_content": "Är du säker på att du vill ta bort denna användare (inklusive spellistor och inställningar)?", - "notifications_blocked": "Du har blockerat meddelanden från denna sajt in din webbläsares inställningar", - "notifications_not_available": "Denna webbläsare stödjer inte skrivbordsmeddelanden eller du använder inte Navidrome via https", - "lastfmLinkSuccess": "Last.fm är länkat och scrobbling är aktivt", - "lastfmLinkFailure": "Last.fm kunde inte länkas", - "lastfmUnlinkSuccess": "Last.fm är inte längre länkat och scrobbling är deaktiverat", - "lastfmUnlinkFailure": "Last.fm kunde inte avlänkas", + "transcodingDisabled": "Av säkerhetsskäl går det inte att ändra omkodningsinställningarna i webbgränssnittet. Starta om servern med konfigurationsalternativet %{config} för att redigera eller lägga till omkodningsinställningar.", + "transcodingEnabled": "Navidrome körs med %{config}. Det gör det möjligt att köra systemkommandon via omkodningsinställningarna i webbgränssnittet. Av säkerhetsskäl rekommenderar vi att alternativet är inaktiverat och bara aktiveras när omkodningen ska konfigureras.", + "songsAddedToPlaylist": "1 låt har lagts till i spellistan |||| %{smart_count} låtar har lagts till i spellistan", + "noPlaylistsAvailable": "Inga tillgängliga", + "delete_user_title": "Ta bort användaren '%{name}'", + "delete_user_content": "Är du säker på att du vill ta bort den här användaren och alla användarens data, inklusive spellistor och inställningar?", + "notifications_blocked": "Du har blockerat aviseringar från den här webbplatsen i webbläsarens inställningar", + "notifications_not_available": "Webbläsaren stöder inte skrivbordsaviseringar, eller så ansluter du inte till Navidrome via HTTPS", + "lastfmLinkSuccess": "Last.fm har kopplats och scrobbling har aktiverats", + "lastfmLinkFailure": "Det gick inte att koppla Last.fm", + "lastfmUnlinkSuccess": "Kopplingen till Last.fm har tagits bort och scrobbling har inaktiverats", + "lastfmUnlinkFailure": "Det gick inte att ta bort kopplingen till Last.fm", "openIn": { "lastfm": "Öppna i Last.fm", "musicbrainz": "Öppna i MusicBrainz" }, "lastfmLink": "Läs mer...", - "listenBrainzLinkSuccess": "ListenBrainz är länkat och scrobbling är aktivt som användare: %{user}", - "listenBrainzLinkFailure": "ListenBrainz kunde inte länkas: %{error}", - "listenBrainzUnlinkSuccess": "ListenBrainz är inte längre länkat och scrobbling är deaktiverat", - "listenBrainzUnlinkFailure": "ListenBrainz kunde inte avlänkas", + "listenBrainzLinkSuccess": "ListenBrainz har kopplats och scrobbling har aktiverats för användaren: %{user}", + "listenBrainzLinkFailure": "Det gick inte att koppla ListenBrainz: %{error}", + "listenBrainzUnlinkSuccess": "Kopplingen till ListenBrainz har tagits bort och scrobbling har inaktiverats", + "listenBrainzUnlinkFailure": "Det gick inte att ta bort kopplingen till ListenBrainz", "downloadOriginalFormat": "Ladda ner i originalformat", "shareOriginalFormat": "Dela i originalformat", "shareDialogTitle": "Dela %{resource} '%{name}'", - "shareBatchDialogTitle": "Dela en %{resource} |||| Dela %{smart_count} %{resource}", - "shareSuccess": "URL kopierades till urklipp: %{url}", - "shareFailure": "Fel vid kopiering av URL %{url} till urklipp", + "shareBatchDialogTitle": "Dela 1 %{resource} |||| Dela %{smart_count} %{resource}", + "shareSuccess": "URL:en har kopierats till urklipp: %{url}", + "shareFailure": "Kunde inte kopiera URL:en %{url} till urklipp", "downloadDialogTitle": "Ladda ner %{resource} '%{name}' (%{size})", "shareCopyToClipboard": "Kopiera till urklipp: Ctrl+C, Enter", "remove_missing_title": "Ta bort saknade filer", - "remove_missing_content": "Är du säker på att du vill ta bort de valda saknade filerna från databasen? Detta kommer permanent radera alla referenser till dem, inklusive antal spelningar och betyg.", + "remove_missing_content": "Är du säker på att du vill ta bort de valda saknade filerna från databasen? Alla referenser till dem, inklusive antal spelningar och betyg, tas bort permanent.", "remove_all_missing_title": "Ta bort alla saknade filer", - "remove_all_missing_content": "Är du säker på att du vill ta bort alla saknade filer från databasen? Detta kommer permanent radera alla referenser till dem, inklusive antal spelningar och betyg.", + "remove_all_missing_content": "Är du säker på att du vill ta bort alla saknade filer från databasen? Alla referenser till dem, inklusive antal spelningar och betyg, tas bort permanent.", "noSimilarSongsFound": "Hittade inga liknande låtar", "noTopSongsFound": "Hittade inga topplåtar", "startingInstantMix": "Laddar direktmix...", @@ -597,7 +610,11 @@ "coverUploaded": "Omslagsbild uppdaterad", "coverRemoved": "Omslagsbild borttagen", "coverUploadError": "Fel vid uppladdning av omslagsbild", - "coverRemoveError": "Fel vid borttagning av omslagsbild" + "coverRemoveError": "Fel vid borttagning av omslagsbild", + "metadataRefreshStarted": "Metadata uppdateras i bakgrunden", + "quickConnectApproved": "%{app} på %{device} är nu inloggad", + "quickConnectInvalidCode": "Koden är ogiltig eller har gått ut", + "quickConnectError": "Kunde inte godkänna koden" }, "menu": { "library": "Bibliotek", @@ -610,17 +627,17 @@ "theme": "Tema", "language": "Språk", "defaultView": "Standardvy", - "desktop_notifications": "Skrivbordsmeddelanden", + "desktop_notifications": "Skrivbordsaviseringar", "lastfmScrobbling": "Scrobbla till Last.fm", "listenBrainzScrobbling": "Scrobbla till ListenBrainz", "replaygain": "ReplayGain-läge", - "preAmp": "ReplayGain PreAmp (dB)", + "preAmp": "Förförstärkning för ReplayGain (dB)", "gain": { "none": "Inaktiverad", - "album": "Använd gain för album", - "track": "Använd gain für låtar" + "album": "Använd albumets volymjustering", + "track": "Använd låtens volymjustering" }, - "lastfmNotConfigured": "Last.fm API-nyckel är inte konfigurerad" + "lastfmNotConfigured": "API-nyckeln för Last.fm är inte konfigurerad" } }, "albumList": "Album", @@ -632,10 +649,19 @@ "multipleLibraries": "%{selected} av %{total} bibliotek", "selectLibraries": "Välj bibliotek", "none": "Inga" - } + }, + "quickConnect": { + "name": "Snabbanslutning", + "code": "Kod", + "help": "Ange koden som visas i en Jellyfin-app för att logga in på ditt konto i appen", + "confirm": "Vill du logga in med ditt konto i %{app} %{version} på %{device}?", + "continue": "Fortsätt", + "approve": "Godkänn" + }, + "onlyFavourites": "Visa endast favoriter" }, "player": { - "playListsText": "Spela kön", + "playListsText": "Uppspelningskö", "openText": "Öppna", "closeText": "Stäng", "notContentText": "Ingen musik", @@ -645,26 +671,26 @@ "previousTrackText": "Föregående låt", "reloadText": "Ladda om", "volumeText": "Volym", - "toggleLyricText": "Låttext av/på", + "toggleLyricText": "Visa eller dölj låttexten", "toggleMiniModeText": "Minimera", - "destroyText": "Radera", + "destroyText": "Stäng spelaren och rensa kön", "downloadText": "Ladda ner", - "removeAudioListsText": "Ta bort audiolistor", + "removeAudioListsText": "Rensa kön", "clickToDeleteText": "Klicka för att ta bort %{name}", "emptyLyricText": "Ingen låttext", "playModeText": { "order": "I ordningsföljd", - "orderLoop": "Upprepa", - "singleLoop": "Upprepa en", - "shufflePlay": "Shuffle" + "orderLoop": "Upprepa alla", + "singleLoop": "Upprepa en låt", + "shufflePlay": "Blanda" } }, "about": { "links": { - "homepage": "Hemsida", + "homepage": "Webbplats", "source": "Källkod", - "featureRequests": "Funktionalitetförfrågan", - "lastInsightsCollection": "Senaste Insights-kollektion", + "featureRequests": "Förslag på nya funktioner", + "lastInsightsCollection": "Senaste insamling av användningsdata", "insights": { "disabled": "Inaktiverad", "waiting": "Väntar" @@ -672,16 +698,16 @@ }, "tabs": { "about": "Om", - "config": "Inställningar" + "config": "Konfiguration" }, "config": { - "configName": "Inställningsnamn", + "configName": "Inställning", "environmentVariable": "Miljövariabel", - "currentValue": "Nuvarande värde", - "configurationFile": "Inställningsfil", - "exportToml": "Exportera inställningar (TOML)", - "exportSuccess": "Inställningarna kopierade till urklippet i TOML-format", - "exportFailed": "Kopiering av inställningarna misslyckades", + "currentValue": "Aktuellt värde", + "configurationFile": "Konfigurationsfil", + "exportToml": "Exportera konfiguration (TOML)", + "exportSuccess": "Konfigurationen har kopierats till urklipp i TOML-format", + "exportFailed": "Kunde inte kopiera konfigurationen", "devFlagsHeader": "Utvecklingsflaggor (kan ändras eller tas bort)", "devFlagsComment": "Dessa inställningar är experimentella och kan tas bort i framtida versioner", "downloadToml": "Ladda ner konfiguration (TOML)" @@ -689,28 +715,28 @@ }, "activity": { "title": "Aktivitet", - "totalScanned": "Genomsökta mappar", - "quickScan": "Snabbscan", - "fullScan": "Komplett scan", - "serverUptime": "Serverdrifttid", + "totalScanned": "Skannade mappar totalt", + "quickScan": "Snabb", + "fullScan": "Fullständig", + "serverUptime": "Serverns drifttid", "serverDown": "OFFLINE", - "scanType": "Typ", - "status": "Fel vid scanning", - "elapsedTime": "Spelad tid", - "selectiveScan": "Urval" + "scanType": "Senaste skanning", + "status": "Skanningsfel", + "elapsedTime": "Förfluten tid", + "selectiveScan": "Selektiv" }, "help": { - "title": "Navidrome kortkommandon", + "title": "Kortkommandon i Navidrome", "hotkeys": { "show_help": "Visa denna hjälp", - "toggle_menu": "Växla sidomeny", + "toggle_menu": "Visa eller dölj sidomenyn", "toggle_play": "Spela / pausa", "prev_song": "Föregående låt", "next_song": "Nästa låt", - "vol_up": "Volym upp", - "vol_down": "Volym ner", - "toggle_love": "Lägg till låt i favoriter", - "current_song": "Hoppa till nuvarande låt" + "vol_up": "Höj volymen", + "vol_down": "Sänk volymen", + "toggle_love": "Lägg till låten i favoriter", + "current_song": "Gå till aktuell låt" } }, "nowPlaying": { @@ -718,4 +744,4 @@ "empty": "Inget spelas", "minutesAgo": "%{smart_count} minut sedan |||| %{smart_count} minuter sedan" } -} \ No newline at end of file +} diff --git a/scanner/phase_1_folders.go b/scanner/phase_1_folders.go index 6107b3316..feefde032 100644 --- a/scanner/phase_1_folders.go +++ b/scanner/phase_1_folders.go @@ -45,7 +45,9 @@ func createPhaseFolders(ctx context.Context, state *scanState, ds model.DataStor jobs = append(jobs, job) } - return &phaseFolders{jobs: jobs, ctx: ctx, ds: ds, state: state, imageChanges: &imageChangeCollector{ds: ds}} + walkCtx, stopWalk := context.WithCancelCause(ctx) + return &phaseFolders{jobs: jobs, ctx: ctx, walkCtx: walkCtx, stopWalk: stopWalk, ds: ds, state: state, + imageChanges: &imageChangeCollector{ds: ds}} } type scanJob struct { @@ -123,6 +125,8 @@ type phaseFolders struct { jobs []*scanJob ds model.DataStore ctx context.Context + walkCtx context.Context // cancelled when a folder fails to persist, so the walk stops early + stopWalk context.CancelCauseFunc state *scanState prevAlbumPIDConf string imageChanges *imageChangeCollector @@ -144,15 +148,15 @@ func (p *phaseFolders) producer() ppl.Producer[*folderEntry] { var total int64 var totalChanged int64 for _, job := range p.jobs { - if utils.IsCtxDone(p.ctx) { + if utils.IsCtxDone(p.walkCtx) { break } - outputChan, err := walkDirTree(p.ctx, job, job.targetFolders...) + outputChan, err := walkDirTree(p.walkCtx, job, job.targetFolders...) if err != nil { log.Warn(p.ctx, "Scanner: Error scanning library", "lib", job.lib.Name, err) } - for folder := range pl.ReadOrDone(p.ctx, outputChan) { + for folder := range pl.ReadOrDone(p.walkCtx, outputChan) { job.numFolders.Add(1) p.state.sendProgress(&ProgressInfo{ LibID: job.lib.ID, @@ -208,6 +212,9 @@ func (p *phaseFolders) stages() []ppl.Stage[*folderEntry] { func (p *phaseFolders) processFolder(entry *folderEntry) (*folderEntry, error) { defer p.measure(entry)() + if err := context.Cause(p.walkCtx); err != nil { + return entry, err + } // Load children mediafiles from DB cursor, err := p.ds.MediaFile(p.ctx).GetCursor(model.QueryOptions{ @@ -331,128 +338,129 @@ func (p *phaseFolders) persistChanges(entry *folderEntry) (*folderEntry, error) defer p.measure(entry)() p.state.changesDetected.Store(true) - // Collect artwork queue items for changed albums/artists, enqueued in the same transaction - var queueItems []model.ArtworkQueueItem - - err := p.ds.WithTx(func(tx model.DataStore) error { - // Instantiate all repositories just once per folder - folderRepo := tx.Folder(p.ctx) - tagRepo := tx.Tag(p.ctx) - artistRepo := tx.Artist(p.ctx) - libraryRepo := tx.Library(p.ctx) - albumRepo := tx.Album(p.ctx) - mfRepo := tx.MediaFile(p.ctx) - - // A new folder's albums/artists are enqueued below; only pre-existing folders need the diff. - if !entry.isNew() { - if changed, artistImage := entry.imagesChanged(); changed { - p.imageChanges.record(entry.job.lib, imageChangedFolder{ - id: entry.id, path: entry.path, artistImage: artistImage, - }) - } - } - - // Save folder to DB - folder := entry.toFolder() - err := folderRepo.Put(folder) - if err != nil { - log.Error(p.ctx, "Scanner: Error persisting folder to DB", "folder", entry.path, err) - return err - } - - // Save all tags to DB - err = tagRepo.Add(entry.job.lib.ID, entry.tags...) - if err != nil { - log.Error(p.ctx, "Scanner: Error persisting tags to DB", "folder", entry.path, err) - return err - } - - // Save all new/modified artists to DB. Their information will be incomplete, but they will be refreshed later - for i := range entry.artists { - err = artistRepo.Put(&entry.artists[i], "name", - "mbz_artist_id", "sort_artist_name", "order_artist_name", "full_text", "search_normalized", "updated_at") - if err != nil { - log.Error(p.ctx, "Scanner: Error persisting artist to DB", "folder", entry.path, "artist", entry.artists[i].Name, err) - return err - } - err = libraryRepo.AddArtist(entry.job.lib.ID, entry.artists[i].ID) - if err != nil { - log.Error(p.ctx, "Scanner: Error adding artist to library", "lib", entry.job.lib.ID, "artist", entry.artists[i].Name, err) - return err - } - if entry.artists[i].Name != consts.UnknownArtist && entry.artists[i].Name != consts.VariousArtists { - queueItems = append(queueItems, scanArtworkItem(model.KindArtistArtwork, entry.artists[i].ID)) - } - } - - // Save all new/modified albums to DB. Their information will be incomplete, but they will be refreshed later - for i := range entry.albums { - err = p.persistAlbum(albumRepo, &entry.albums[i], entry.albumIDMap) - if err != nil { - log.Error(p.ctx, "Scanner: Error persisting album to DB", "folder", entry.path, "album", entry.albums[i], err) - return err - } - if entry.albums[i].Name != consts.UnknownAlbum { - queueItems = append(queueItems, scanArtworkItem(model.KindAlbumArtwork, entry.albums[i].ID)) - } - } - - // Save all tracks to DB - for i := range entry.tracks { - err = mfRepo.Put(&entry.tracks[i]) - if err != nil { - log.Error(p.ctx, "Scanner: Error persisting mediafile to DB", "folder", entry.path, "track", entry.tracks[i], err) - return err - } - } - - // A re-imported track returns to unresolved so new embedded art is picked up lazily. - if len(entry.tracks) > 0 { - trackIDs := slice.Map(entry.tracks, func(t model.MediaFile) string { return t.ID }) - if err := tx.Artwork(p.ctx).DeleteForItems(model.KindMediaFileArtwork, trackIDs); err != nil { - log.Warn(p.ctx, "Scanner: could not invalidate media_file artwork", "folder", entry.path, err) - } - } - - // Mark all missing tracks as not available - if len(entry.missingTracks) > 0 { - err = mfRepo.MarkMissing(true, entry.missingTracks...) - if err != nil { - log.Error(p.ctx, "Scanner: Error marking missing tracks", "folder", entry.path, err) - return err - } - - // Touch all albums that have missing tracks, so they get refreshed in later phases - groupedMissingTracks := slice.ToMap(entry.missingTracks, func(mf *model.MediaFile) (string, struct{}) { - return mf.AlbumID, struct{}{} - }) - albumsToUpdate := slices.Collect(maps.Keys(groupedMissingTracks)) - err = albumRepo.Touch(albumsToUpdate...) - if err != nil { - log.Error(p.ctx, "Scanner: Error touching album", "folder", entry.path, "albums", albumsToUpdate, err) - return err - } - } - - // Enqueue artwork resolution for changed albums/artists. Never fails the scan. - // A full scan re-imports every track, so a re-import is no evidence the art changed. - if len(queueItems) > 0 { - queue := tx.ArtworkQueue(p.ctx) - enqueue := queue.Enqueue - if p.state.fullScan { - enqueue = queue.EnqueueIfMissing - } - if err := enqueue(queueItems...); err != nil { - log.Warn(p.ctx, "Scanner: could not enqueue artwork resolution", "folder", entry.path, err) - } - } - return nil + ctx := log.NewContext(p.ctx, "folder", entry.path) + err := p.ds.WithTxRetry(ctx, func(ctx context.Context, tx model.DataStore) error { + return p.persistFolder(ctx, tx, entry) }, "scanner: persist changes") if err != nil { - log.Error(p.ctx, "Scanner: Error persisting changes to DB", "folder", entry.path, err) + log.Error(ctx, "Scanner: Error persisting changes to DB", err) + p.stopWalk(err) + return entry, err } - return entry, err + // A new folder's albums/artists were enqueued with it; only pre-existing folders need the diff. + if !entry.isNew() { + if changed, artistImage := entry.imagesChanged(); changed { + p.imageChanges.record(entry.job.lib, imageChangedFolder{ + id: entry.id, path: entry.path, artistImage: artistImage, + }) + } + } + return entry, nil +} + +// persistFolder writes the folder in tx. WithTxRetry may rerun it after a rollback. +func (p *phaseFolders) persistFolder(ctx context.Context, tx model.DataStore, entry *folderEntry) error { + // Collect artwork queue items for changed albums/artists, enqueued in the same transaction + var queueItems []model.ArtworkQueueItem + // persistAlbum consumes the map, so a rerun needs the original + albumIDMap := maps.Clone(entry.albumIDMap) + + // Instantiate all repositories just once per folder + folderRepo := tx.Folder(ctx) + tagRepo := tx.Tag(ctx) + artistRepo := tx.Artist(ctx) + libraryRepo := tx.Library(ctx) + albumRepo := tx.Album(ctx) + mfRepo := tx.MediaFile(ctx) + + // Save folder to DB + folder := entry.toFolder() + err := folderRepo.Put(folder) + if err != nil { + return fmt.Errorf("persisting folder: %w", err) + } + + // Save all tags to DB + err = tagRepo.Add(entry.job.lib.ID, entry.tags...) + if err != nil { + return fmt.Errorf("persisting tags: %w", err) + } + + // Save all new/modified artists to DB. Their information will be incomplete, but they will be refreshed later + for i := range entry.artists { + err = artistRepo.Put(&entry.artists[i], "name", + "mbz_artist_id", "sort_artist_name", "order_artist_name", "full_text", "search_normalized", "updated_at") + if err != nil { + return fmt.Errorf("persisting artist %q: %w", entry.artists[i].Name, err) + } + err = libraryRepo.AddArtist(entry.job.lib.ID, entry.artists[i].ID) + if err != nil { + return fmt.Errorf("adding artist %q to library: %w", entry.artists[i].Name, err) + } + if entry.artists[i].Name != consts.UnknownArtist && entry.artists[i].Name != consts.VariousArtists { + queueItems = append(queueItems, scanArtworkItem(model.KindArtistArtwork, entry.artists[i].ID)) + } + } + + // Save all new/modified albums to DB. Their information will be incomplete, but they will be refreshed later + for i := range entry.albums { + err = p.persistAlbum(albumRepo, &entry.albums[i], albumIDMap) + if err != nil { + return err + } + if entry.albums[i].Name != consts.UnknownAlbum { + queueItems = append(queueItems, scanArtworkItem(model.KindAlbumArtwork, entry.albums[i].ID)) + } + } + + // Save all tracks to DB + for i := range entry.tracks { + err = mfRepo.Put(&entry.tracks[i]) + if err != nil { + return fmt.Errorf("persisting track %q: %w", entry.tracks[i].Path, err) + } + } + + // A re-imported track returns to unresolved so new embedded art is picked up lazily. + if len(entry.tracks) > 0 { + trackIDs := slice.Map(entry.tracks, func(t model.MediaFile) string { return t.ID }) + if err := tx.Artwork(ctx).DeleteForItems(model.KindMediaFileArtwork, trackIDs); err != nil { + log.Warn(ctx, "Scanner: could not invalidate media_file artwork", err) + } + } + + // Mark all missing tracks as not available + if len(entry.missingTracks) > 0 { + err = mfRepo.MarkMissing(true, entry.missingTracks...) + if err != nil { + return fmt.Errorf("marking missing tracks: %w", err) + } + + // Touch all albums that have missing tracks, so they get refreshed in later phases + groupedMissingTracks := slice.ToMap(entry.missingTracks, func(mf *model.MediaFile) (string, struct{}) { + return mf.AlbumID, struct{}{} + }) + albumsToUpdate := slices.Collect(maps.Keys(groupedMissingTracks)) + err = albumRepo.Touch(albumsToUpdate...) + if err != nil { + return fmt.Errorf("touching albums %v: %w", albumsToUpdate, err) + } + } + + // Enqueue artwork resolution for changed albums/artists. Never fails the scan. + // A full scan re-imports every track, so a re-import is no evidence the art changed. + if len(queueItems) > 0 { + queue := tx.ArtworkQueue(ctx) + enqueue := queue.Enqueue + if p.state.fullScan { + enqueue = queue.EnqueueIfMissing + } + if err := enqueue(queueItems...); err != nil { + log.Warn(ctx, "Scanner: could not enqueue artwork resolution", err) + } + } + return nil } // persistAlbum persists the given album to the database, and reassigns annotations from the previous album ID @@ -499,34 +507,32 @@ func (p *phaseFolders) logFolder(entry *folderEntry) (*folderEntry, error) { } func (p *phaseFolders) finalize(err error) error { - errF := p.ds.WithTx(func(tx model.DataStore) error { + p.stopWalk(nil) + defer p.imageChanges.enqueue(p.ctx) + // A failed phase may not have walked every folder, and unvisited ones must not be marked missing + if err != nil { + return err + } + return p.ds.WithTxRetry(p.ctx, func(ctx context.Context, tx model.DataStore) error { for _, job := range p.jobs { // Mark all folders that were not updated as missing if len(job.lastUpdates) == 0 { continue } folderIDs := slices.Collect(maps.Keys(job.lastUpdates)) - err := tx.Folder(p.ctx).MarkMissing(true, folderIDs...) - if err != nil { - log.Error(p.ctx, "Scanner: Error marking missing folders", "lib", job.lib.Name, err) - return err + if err := tx.Folder(ctx).MarkMissing(true, folderIDs...); err != nil { + return fmt.Errorf("marking missing folders in %s: %w", job.lib.Name, err) } - err = tx.MediaFile(p.ctx).MarkMissingByFolder(true, folderIDs...) - if err != nil { - log.Error(p.ctx, "Scanner: Error marking tracks in missing folders", "lib", job.lib.Name, err) - return err + if err := tx.MediaFile(ctx).MarkMissingByFolder(true, folderIDs...); err != nil { + return fmt.Errorf("marking tracks in missing folders in %s: %w", job.lib.Name, err) } // Touch all albums that have missing folders, so they get refreshed in later phases - _, err = tx.Album(p.ctx).TouchByMissingFolder() - if err != nil { - log.Error(p.ctx, "Scanner: Error touching albums with missing folders", "lib", job.lib.Name, err) - return err + if _, err := tx.Album(ctx).TouchByMissingFolder(); err != nil { + return fmt.Errorf("touching albums with missing folders in %s: %w", job.lib.Name, err) } } return nil }, "scanner: finalize phaseFolders") - p.imageChanges.enqueue(p.ctx) - return errors.Join(err, errF) } var _ phase[*folderEntry] = (*phaseFolders)(nil) diff --git a/scanner/phase_2_missing_tracks.go b/scanner/phase_2_missing_tracks.go index 8c258b833..6ccc9a46c 100644 --- a/scanner/phase_2_missing_tracks.go +++ b/scanner/phase_2_missing_tracks.go @@ -273,66 +273,68 @@ func (p *phaseMissingTracks) findCrossLibraryMatch(missing model.MediaFile) (mod } func (p *phaseMissingTracks) moveMatched(target, missing model.MediaFile) error { - return p.ds.WithTx(func(tx model.DataStore) error { - discardedID := target.ID - oldAlbumID := missing.AlbumID - newAlbumID := target.AlbumID + oldAlbumID := missing.AlbumID + newAlbumID := target.AlbumID + // Use newAlbumID as key since we only care about avoiding duplicate reassignments to the same target. + // Claimed before the transaction so a concurrent move skips it, and released if the move fails. + reassignAlbum := oldAlbumID != newAlbumID + if reassignAlbum { + p.annotationMutex.Lock() + reassignAlbum = !p.processedAlbumAnnotations[newAlbumID] + p.processedAlbumAnnotations[newAlbumID] = true + p.annotationMutex.Unlock() + if !reassignAlbum { + log.Trace(p.ctx, "Scanner: Skipping album annotation reassignment", "from", oldAlbumID, "to", newAlbumID) + } + } + err := p.ds.WithTxRetry(p.ctx, func(ctx context.Context, tx model.DataStore) error { + // A rerun must start from the original target, not the one the rolled-back attempt changed + moved := target // Preserve the original created_at from the missing file, so moved tracks // don't appear in "Recently Added" - target.CreatedAt = missing.CreatedAt + moved.CreatedAt = missing.CreatedAt // Update the target media file with the missing file's ID. This effectively "moves" the track // to the new location while keeping its annotations and references intact. - target.ID = missing.ID - err := tx.MediaFile(p.ctx).Put(&target) - if err != nil { + moved.ID = missing.ID + if err := tx.MediaFile(ctx).Put(&moved); err != nil { return fmt.Errorf("update matched track: %w", err) } // Discard the new mediafile row (the one that was moved to) - err = tx.MediaFile(p.ctx).Delete(discardedID) - if err != nil { + if err := tx.MediaFile(ctx).Delete(target.ID); err != nil { return fmt.Errorf("delete discarded track: %w", err) } - // Handle album annotation reassignment if AlbumID changed - if oldAlbumID != newAlbumID { - // Use newAlbumID as key since we only care about avoiding duplicate reassignments to the same target - p.annotationMutex.RLock() - alreadyProcessed := p.processedAlbumAnnotations[newAlbumID] - p.annotationMutex.RUnlock() - - if !alreadyProcessed { - p.annotationMutex.Lock() - // Double-check pattern to avoid race conditions - if !p.processedAlbumAnnotations[newAlbumID] { - // Reassign direct album annotations (starred, rating) - log.Debug(p.ctx, "Scanner: Reassigning album annotations", "from", oldAlbumID, "to", newAlbumID) - if err := tx.Album(p.ctx).ReassignAnnotation(oldAlbumID, newAlbumID); err != nil { - log.Warn(p.ctx, "Scanner: Could not reassign album annotations", "from", oldAlbumID, "to", newAlbumID, err) - } - - // Keep created_at field from previous instance of the album, so moved albums - // don't appear in "Recently Added" - if err := tx.Album(p.ctx).CopyAttributes(oldAlbumID, newAlbumID, "created_at"); err != nil { - if !errors.Is(err, model.ErrNotFound) { - log.Warn(p.ctx, "Scanner: Could not copy album created_at", "from", oldAlbumID, "to", newAlbumID, err) - } - } - - // Note: RefreshPlayCounts will be called in later phases, so we don't need to call it here - p.processedAlbumAnnotations[newAlbumID] = true - } - p.annotationMutex.Unlock() - } else { - log.Trace(p.ctx, "Scanner: Skipping album annotation reassignment", "from", oldAlbumID, "to", newAlbumID) + if reassignAlbum { + // Reassign direct album annotations (starred, rating) + log.Debug(ctx, "Scanner: Reassigning album annotations", "from", oldAlbumID, "to", newAlbumID) + if err := tx.Album(ctx).ReassignAnnotation(oldAlbumID, newAlbumID); err != nil { + log.Warn(ctx, "Scanner: Could not reassign album annotations", "from", oldAlbumID, "to", newAlbumID, err) } - } - p.state.changesDetected.Store(true) + // Keep created_at field from previous instance of the album, so moved albums + // don't appear in "Recently Added" + if err := tx.Album(ctx).CopyAttributes(oldAlbumID, newAlbumID, "created_at"); err != nil { + if !errors.Is(err, model.ErrNotFound) { + log.Warn(ctx, "Scanner: Could not copy album created_at", "from", oldAlbumID, "to", newAlbumID, err) + } + } + // Note: RefreshPlayCounts will be called in later phases, so we don't need to call it here + } return nil - }) + }, "scanner: move matched track") + if err != nil { + if reassignAlbum { + p.annotationMutex.Lock() + delete(p.processedAlbumAnnotations, newAlbumID) + p.annotationMutex.Unlock() + } + return err + } + p.state.changesDetected.Store(true) + return nil } func (p *phaseMissingTracks) finalize(err error) error { @@ -355,7 +357,12 @@ func (p *phaseMissingTracks) finalize(err error) error { } func (p *phaseMissingTracks) purgeMissing() error { - deletedCount, err := p.ds.MediaFile(p.ctx).DeleteAllMissing() + var deletedCount int64 + err := p.ds.WithTxRetry(p.ctx, func(ctx context.Context, tx model.DataStore) error { + var err error + deletedCount, err = tx.MediaFile(ctx).DeleteAllMissing() + return err + }, "scanner: purge missing") if err != nil { return fmt.Errorf("error deleting missing files: %w", err) } diff --git a/scanner/phase_2_missing_tracks_test.go b/scanner/phase_2_missing_tracks_test.go index f93b166c1..b7aa52f90 100644 --- a/scanner/phase_2_missing_tracks_test.go +++ b/scanner/phase_2_missing_tracks_test.go @@ -2,6 +2,8 @@ package scanner import ( "context" + "errors" + "maps" "time" "github.com/navidrome/navidrome/conf" @@ -146,6 +148,87 @@ var _ = Describe("phaseMissingTracks", func() { Expect(movedTrack.Path).To(Equal(matchedTrack.Path)) }) + Context("claiming the album annotation reassignment", func() { + var probe *probeTxDS + missingTrack := model.MediaFile{ID: "1", PID: "A", AlbumID: "old-album", Path: "dir1/path1.mp3", Tags: model.Tags{"title": []string{"title1"}}, Size: 100} + matchedTrack := model.MediaFile{ID: "2", PID: "A", AlbumID: "new-album", Path: "dir2/path2.mp3", Tags: model.Tags{"title": []string{"title1"}}, Size: 100} + BeforeEach(func() { + probe = &probeTxDS{MockDataStore: ds.(*tests.MockDataStore)} + probe.MockedAlbum = tests.CreateMockAlbumRepo() + phase = createPhaseMissingTracks(ctx, state, probe) + _ = ds.MediaFile(ctx).Put(&missingTrack) + _ = ds.MediaFile(ctx).Put(&matchedTrack) + }) + + It("claims the target album before the transaction, so a concurrent move skips it", func() { + probe.during = func() { + phase.annotationMutex.RLock() + defer phase.annotationMutex.RUnlock() + Expect(phase.processedAlbumAnnotations).To(HaveKeyWithValue("new-album", true)) + } + Expect(phase.moveMatched(matchedTrack, missingTrack)).To(Succeed()) + }) + + It("releases the claim when the move fails, so a later move can reassign", func() { + probe.err = errors.New("boom") + Expect(phase.moveMatched(matchedTrack, missingTrack)).To(MatchError("boom")) + Expect(phase.processedAlbumAnnotations).ToNot(HaveKey("new-album")) + }) + }) + + Context("when the move transaction is rerun after a busy rollback", func() { + var rerunDS *rerunTxDS + BeforeEach(func() { + rerunDS = &rerunTxDS{MockDataStore: ds.(*tests.MockDataStore)} + rerunDS.snapshot = func() func() { + saved := maps.Clone(mr.Data) + return func() { mr.Data = saved } + } + phase = createPhaseMissingTracks(ctx, state, rerunDS) + }) + + It("keeps the moved track", func() { + missingTrack := model.MediaFile{ID: "1", PID: "A", Path: "dir1/path1.mp3", Tags: model.Tags{"title": []string{"title1"}}, Size: 100} + matchedTrack := model.MediaFile{ID: "2", PID: "A", Path: "dir2/path2.mp3", Tags: model.Tags{"title": []string{"title1"}}, Size: 100} + _ = ds.MediaFile(ctx).Put(&missingTrack) + _ = ds.MediaFile(ctx).Put(&matchedTrack) + + _, err := phase.processMissingTracks(&missingTracks{ + missing: []model.MediaFile{missingTrack}, + matched: []model.MediaFile{matchedTrack}, + }) + Expect(err).ToNot(HaveOccurred()) + + movedTrack, err := ds.MediaFile(ctx).Get("1") + Expect(err).ToNot(HaveOccurred()) + Expect(movedTrack.Path).To(Equal(matchedTrack.Path)) + }) + + It("reassigns the album annotations in the attempt that commits", func() { + albumRepo := tests.CreateMockAlbumRepo() + rerunDS.MockedAlbum = albumRepo + restoreTracks := rerunDS.snapshot + rerunDS.snapshot = func() func() { + restore := restoreTracks() + return func() { + restore() + albumRepo.ReassignAnnotationCalls = nil + } + } + missingTrack := model.MediaFile{ID: "1", PID: "A", AlbumID: "old-album", Path: "dir1/path1.mp3", Tags: model.Tags{"title": []string{"title1"}}, Size: 100} + matchedTrack := model.MediaFile{ID: "2", PID: "A", AlbumID: "new-album", Path: "dir2/path2.mp3", Tags: model.Tags{"title": []string{"title1"}}, Size: 100} + _ = ds.MediaFile(ctx).Put(&missingTrack) + _ = ds.MediaFile(ctx).Put(&matchedTrack) + + _, err := phase.processMissingTracks(&missingTracks{ + missing: []model.MediaFile{missingTrack}, + matched: []model.MediaFile{matchedTrack}, + }) + Expect(err).ToNot(HaveOccurred()) + Expect(albumRepo.ReassignAnnotationCalls).To(HaveKeyWithValue("old-album", "new-album")) + }) + }) + It("should move the matched track when the missing track has the same tags and filename", func() { missingTrack := model.MediaFile{ID: "1", PID: "A", Path: "path1.mp3", Tags: model.Tags{"title": []string{"title1"}}, Size: 100} matchedTrack := model.MediaFile{ID: "2", PID: "A", Path: "path1.flac", Tags: model.Tags{"title": []string{"title1"}}, Size: 200} @@ -957,3 +1040,34 @@ var _ = Describe("phaseMissingTracks", func() { }) }) }) + +// rerunTxDS runs every WithTxRetry block twice, as a retry after a rolled-back busy attempt would. +// The mock is not transactional, so snapshot returns the function that plays the rollback. +type rerunTxDS struct { + *tests.MockDataStore + snapshot func() (rollback func()) +} + +func (d *rerunTxDS) WithTxRetry(ctx context.Context, block func(context.Context, model.DataStore) error, _ ...string) error { + rollback := d.snapshot() + _ = block(ctx, d.MockDataStore) + rollback() + return block(ctx, d.MockDataStore) +} + +// probeTxDS runs a hook inside each WithTxRetry block, and can fail the transaction after it. +type probeTxDS struct { + *tests.MockDataStore + during func() + err error +} + +func (d *probeTxDS) WithTxRetry(ctx context.Context, block func(context.Context, model.DataStore) error, _ ...string) error { + if err := block(ctx, d.MockDataStore); err != nil { + return err + } + if d.during != nil { + d.during() + } + return d.err +} diff --git a/scanner/phase_3_refresh_albums.go b/scanner/phase_3_refresh_albums.go index 33e0fed01..964ab7408 100644 --- a/scanner/phase_3_refresh_albums.go +++ b/scanner/phase_3_refresh_albums.go @@ -103,7 +103,9 @@ func (p *phaseRefreshAlbums) refreshAlbum(album *model.Album) (*model.Album, err return nil, nil } start := time.Now() - err := p.ds.Album(p.ctx).Put(album) + err := p.ds.WithTxRetry(p.ctx, func(ctx context.Context, tx model.DataStore) error { + return tx.Album(ctx).Put(album) + }, "scanner: refresh album") log.Debug(p.ctx, "Scanner: refreshing album", "album_id", album.ID, "name", album.Name, "songCount", album.SongCount, "elapsed", time.Since(start), err) if err != nil { return nil, fmt.Errorf("refreshing album %s: %w", album.ID, err) @@ -130,7 +132,12 @@ func (p *phaseRefreshAlbums) finalize(err error) error { } // Refresh album annotations start := time.Now() - cnt, err := p.ds.Album(p.ctx).RefreshPlayCounts() + var cnt int64 + err = p.ds.WithTxRetry(p.ctx, func(ctx context.Context, tx model.DataStore) error { + var txErr error + cnt, txErr = tx.Album(ctx).RefreshPlayCounts() + return txErr + }, "scanner: refresh album play counts") if err != nil { return fmt.Errorf("refreshing album annotations: %w", err) } @@ -138,7 +145,11 @@ func (p *phaseRefreshAlbums) finalize(err error) error { // Refresh artist annotations start = time.Now() - cnt, err = p.ds.Artist(p.ctx).RefreshPlayCounts() + err = p.ds.WithTxRetry(p.ctx, func(ctx context.Context, tx model.DataStore) error { + var txErr error + cnt, txErr = tx.Artist(ctx).RefreshPlayCounts() + return txErr + }, "scanner: refresh artist play counts") if err != nil { return fmt.Errorf("refreshing artist annotations: %w", err) } diff --git a/scanner/phase_4_playlists.go b/scanner/phase_4_playlists.go index baa8b749a..4e11fa81d 100644 --- a/scanner/phase_4_playlists.go +++ b/scanner/phase_4_playlists.go @@ -101,7 +101,10 @@ func (p *phasePlaylists) produce(put func(entry *model.Folder)) error { // import the playlists, and returns an error if the flag can't be persisted (so // the scan does not complete as successful without recording the recovery). func (p *phasePlaylists) deferImport() error { - if err := p.ds.Property(p.ctx).Put(consts.PlaylistsImportPendingFlagKey, "1"); err != nil { + err := p.ds.WithTxRetry(p.ctx, func(ctx context.Context, tx model.DataStore) error { + return tx.Property(ctx).Put(consts.PlaylistsImportPendingFlagKey, "1") + }, "scanner: defer playlist import") + if err != nil { return fmt.Errorf("recording pending playlist import: %w", err) } log.Warn(p.ctx, "Playlists will not be imported, as there are no admin users yet. "+ diff --git a/scanner/scanner.go b/scanner/scanner.go index d73007bdd..a8a192771 100644 --- a/scanner/scanner.go +++ b/scanner/scanner.go @@ -217,7 +217,9 @@ func (s *scannerImpl) prepareLibrariesForScan(ctx context.Context, state *scanSt for _, lib := range state.libraries { if lib.LastScanStartedAt.IsZero() { // This is a new scan - mark it as started - err := s.ds.Library(ctx).ScanBegin(lib.ID, state.fullScan) + err := s.ds.WithTxRetry(ctx, func(ctx context.Context, tx model.DataStore) error { + return tx.Library(ctx).ScanBegin(lib.ID, state.fullScan) + }, "scanner: begin library scan") if err != nil { log.Error(ctx, "Scanner: Error marking scan start", "lib", lib.Name, err) state.sendWarning(err.Error()) @@ -253,7 +255,7 @@ func (s *scannerImpl) prepareLibrariesForScan(ctx context.Context, state *scanSt func (s *scannerImpl) runGC(ctx context.Context, state *scanState) func() error { return func() error { state.sendProgress(&ProgressInfo{ForceUpdate: true}) - return s.ds.WithTx(func(tx model.DataStore) error { + return s.ds.WithTxRetry(ctx, func(ctx context.Context, tx model.DataStore) error { if state.changesDetected.Load() { start := time.Now() @@ -264,9 +266,7 @@ func (s *scannerImpl) runGC(ctx context.Context, state *scanState) func() error log.Debug(ctx, "Scanner: Running selective GC", "libraryIDs", libraryIDs) } - err := tx.GC(ctx, libraryIDs...) - if err != nil { - log.Error(ctx, "Scanner: Error running GC", err) + if err := tx.GC(ctx, libraryIDs...); err != nil { return fmt.Errorf("running GC: %w", err) } log.Debug(ctx, "Scanner: GC completed", "elapsed", time.Since(start)) @@ -286,10 +286,14 @@ func (s *scannerImpl) runEnqueueMissingArtwork(ctx context.Context, state *scanS return nil } start := time.Now() - queue := s.ds.ArtworkQueue(ctx) var total int64 for _, kind := range []model.Kind{model.KindAlbumArtwork, model.KindArtistArtwork} { - n, err := queue.EnqueueAllMissing(kind, model.ArtworkPriorityScan) + var n int64 + err := s.ds.WithTxRetry(ctx, func(ctx context.Context, tx model.DataStore) error { + var err error + n, err = tx.ArtworkQueue(ctx).EnqueueAllMissing(kind, model.ArtworkPriorityScan) + return err + }, "scanner: enqueue missing artwork") if err != nil { log.Error(ctx, "Scanner: Error enqueueing missing artwork", "kind", kind, err) return fmt.Errorf("enqueueing missing artwork: %w", err) @@ -316,7 +320,9 @@ func (s *scannerImpl) runRefreshStats(ctx context.Context, state *scanState) fun log.Debug(ctx, "Scanner: Refreshed artist stats", "stats", stats, "elapsed", time.Since(start)) start = time.Now() - err = s.ds.Tag(ctx).UpdateCounts() + err = s.ds.WithTxRetry(ctx, func(ctx context.Context, tx model.DataStore) error { + return tx.Tag(ctx).UpdateCounts() + }, "scanner: update tag counts") if err != nil { log.Error(ctx, "Scanner: Error updating tag counts", err) return fmt.Errorf("updating tag counts: %w", err) @@ -329,28 +335,21 @@ func (s *scannerImpl) runRefreshStats(ctx context.Context, state *scanState) fun func (s *scannerImpl) runUpdateLibraries(ctx context.Context, state *scanState) func() error { return func() error { start := time.Now() - return s.ds.WithTx(func(tx model.DataStore) error { + return s.ds.WithTxRetry(ctx, func(ctx context.Context, tx model.DataStore) error { for _, lib := range state.libraries { - err := tx.Library(ctx).ScanEnd(lib.ID) - if err != nil { - log.Error(ctx, "Scanner: Error updating last scan completed", "lib", lib.Name, err) - return fmt.Errorf("updating last scan completed: %w", err) + if err := tx.Library(ctx).ScanEnd(lib.ID); err != nil { + return fmt.Errorf("updating last scan completed for %s: %w", lib.Name, err) } - err = tx.Property(ctx).Put(consts.PIDTrackKey, conf.Server.PID.Track) - if err != nil { - log.Error(ctx, "Scanner: Error updating track PID conf", err) + if err := tx.Property(ctx).Put(consts.PIDTrackKey, conf.Server.PID.Track); err != nil { return fmt.Errorf("updating track PID conf: %w", err) } - err = tx.Property(ctx).Put(consts.PIDAlbumKey, conf.Server.PID.Album) - if err != nil { - log.Error(ctx, "Scanner: Error updating album PID conf", err) + if err := tx.Property(ctx).Put(consts.PIDAlbumKey, conf.Server.PID.Album); err != nil { return fmt.Errorf("updating album PID conf: %w", err) } if state.changesDetected.Load() { log.Debug(ctx, "Scanner: Refreshing library stats", "lib", lib.Name) if err := tx.Library(ctx).RefreshStats(lib.ID); err != nil { - log.Error(ctx, "Scanner: Error refreshing library stats", "lib", lib.Name, err) - return fmt.Errorf("refreshing library stats: %w", err) + return fmt.Errorf("refreshing library stats for %s: %w", lib.Name, err) } } else { log.Debug(ctx, "Scanner: No changes detected, skipping library stats refresh", "lib", lib.Name) diff --git a/scanner/scanner_test.go b/scanner/scanner_test.go index 8542b3ac6..30f4a2b97 100644 --- a/scanner/scanner_test.go +++ b/scanner/scanner_test.go @@ -4,12 +4,16 @@ import ( "context" "database/sql" "errors" + "fmt" + "os" "path/filepath" + "sync/atomic" "testing/fstest" "time" "github.com/Masterminds/squirrel" "github.com/google/uuid" + "github.com/mattn/go-sqlite3" "github.com/navidrome/navidrome/conf" "github.com/navidrome/navidrome/conf/configtest" "github.com/navidrome/navidrome/consts" @@ -52,7 +56,10 @@ var _ = Describe("Scanner", Ordered, func() { BeforeAll(func() { ctx = request.WithUser(GinkgoT().Context(), model.User{ID: "123", IsAdmin: true}) - tmpDir := GinkgoT().TempDir() + // The DB stays open until the suite ends, and Windows can't delete an open file + tmpDir, err := os.MkdirTemp("", "scanner-test") + Expect(err).ToNot(HaveOccurred()) + DeferCleanup(func() { _ = os.RemoveAll(tmpDir) }) conf.Server.DbPath = filepath.Join(tmpDir, "test-scanner.db?_journal_mode=WAL") log.Warn("Using DB at " + conf.Server.DbPath) //conf.Server.DbPath = ":memory:" @@ -1240,8 +1247,55 @@ var _ = Describe("Scanner", Ordered, func() { Expect(albumArtistStats.SongCount).To(Equal(3)) // 3 songs }) }) + + Context("when the database is busy", func() { + var busyDS *busyPersistDS + BeforeEach(func() { + // One album across many folders: the suite's single DB connection deadlocks phase 3 on many albums + album := template(_t{"albumartist": "Artist", "album": "Album"}) + files := fstest.MapFS{} + for i := range 30 { + files[fmt.Sprintf("Artist/Part %02d/%02d - Song.mp3", i, i+1)] = album(track(i+1, fmt.Sprintf("Song %02d", i+1))) + } + createFS(files) + busyDS = &busyPersistDS{MockDataStore: ds} + s = scanner.New(ctx, busyDS, events.NoopBroker(), + playlists.NewPlaylists(busyDS, artwork.NewUploader(busyDS)), metrics.NewNoopInstance()) + }) + + It("gives up and stops walking the library when the database stays busy", func() { + busyDS.failures.Store(1000) + + Expect(runScanner(ctx, true)).To(MatchError(ContainSubstring("database is locked"))) + + Expect(mfRepo.cursorCalls.Load()).To(BeNumerically("<", 30)) + }) + + It("does not mark unvisited folders missing when the scan gives up", func() { + Expect(runScanner(ctx, true)).To(Succeed()) + busyDS.failures.Store(1000) + + Expect(runScanner(ctx, true)).ToNot(Succeed()) + + Expect(ds.Folder(ctx).CountAll(model.QueryOptions{Filters: squirrel.Eq{"missing": true}})).To(BeZero()) + Expect(ds.MediaFile(ctx).CountAll(model.QueryOptions{Filters: squirrel.Eq{"missing": true}})).To(BeZero()) + }) + }) }) +// busyPersistDS fails the scanner's folder saves with SQLITE_BUSY, as if WithTxRetry ran out of retries. +type busyPersistDS struct { + *tests.MockDataStore + failures atomic.Int32 +} + +func (b *busyPersistDS) WithTxRetry(ctx context.Context, block func(context.Context, model.DataStore) error, label ...string) error { + if len(label) > 0 && label[0] == "scanner: persist changes" && b.failures.Add(-1) >= 0 { + return sqlite3.Error{Code: sqlite3.ErrBusy} + } + return b.MockDataStore.WithTxRetry(ctx, block, label...) +} + func createFindByPath(ctx context.Context, ds model.DataStore) func(string) (*model.MediaFile, error) { return func(path string) (*model.MediaFile, error) { list, err := ds.MediaFile(ctx).FindByPaths([]string{path}) @@ -1258,6 +1312,12 @@ func createFindByPath(ctx context.Context, ds model.DataStore) func(string) (*mo type mockMediaFileRepo struct { model.MediaFileRepository GetMissingAndMatchingError error + cursorCalls atomic.Int32 +} + +func (m *mockMediaFileRepo) GetCursor(options ...model.QueryOptions) (model.MediaFileCursor, error) { + m.cursorCalls.Add(1) + return m.MediaFileRepository.GetCursor(options...) } func (m *mockMediaFileRepo) GetMissingAndMatching(libId int) (model.MediaFileCursor, error) { diff --git a/scanner/walk_dir_tree.go b/scanner/walk_dir_tree.go index 20f6d4213..1864a6a44 100644 --- a/scanner/walk_dir_tree.go +++ b/scanner/walk_dir_tree.go @@ -53,6 +53,9 @@ func walkDirTree(ctx context.Context, job *scanJob, targetFolders ...string) (<- // Recursively walk this folder and all its children err = walkFolder(ctx, job, folderPath, checker, results) + if utils.IsCtxDone(ctx) { + return + } if err != nil { log.Error(ctx, "Scanner: Error walking target folder", "path", folderPath, err) continue @@ -87,9 +90,12 @@ func walkFolder(ctx context.Context, job *scanJob, currentFolder string, checker folder.path = dir folder.elapsed.Start() - results <- folder - - return nil + select { + case results <- folder: + return nil + case <-ctx.Done(): + return ctx.Err() + } } func loadDir(ctx context.Context, job *scanJob, dirPath string, checker *IgnoreChecker) (folder *folderEntry, children []string, err error) { @@ -291,6 +297,7 @@ var ignoredDirs = []string{ "$RECYCLE.BIN", "#snapshot", "@Recycle", + "@eaDir", "@Recently-Snapshot", ".git", ".streams", diff --git a/scanner/walk_dir_tree_test.go b/scanner/walk_dir_tree_test.go index 9fb650c4d..43939e5c2 100644 --- a/scanner/walk_dir_tree_test.go +++ b/scanner/walk_dir_tree_test.go @@ -564,6 +564,7 @@ var _ = Describe("walk_dir_tree", func() { Entry("dir starting with ellipsis", "...unhidden_folder", false), Entry("recycle bin", "$Recycle.Bin", true), Entry("snapshot dir", "#snapshot", true), + Entry("synology metadata dir", "@eaDir", true), ) }) diff --git a/server/auth.go b/server/auth.go index fdeb8c455..2aaa93e63 100644 --- a/server/auth.go +++ b/server/auth.go @@ -159,7 +159,7 @@ func createAdminUser(ctx context.Context, ds model.DataStore, username, password } err := ds.User(ctx).Put(&initialUser) if err != nil { - log.Error(ctx, "Could not create initial user", "user", initialUser, err) + log.Error(ctx, "Could not create initial user", "user", initialUser.UserName, err) return fmt.Errorf("creating initial user: %w", err) } return nil @@ -218,14 +218,14 @@ func UsernameFromExtAuthHeader(r *http.Request) string { log.Error("ExtAuth enabled but no proxy IP found in request context. Please report this error.") return "" } - if !validateIPAgainstList(reverseProxyIp, conf.Server.ExtAuth.TrustedSources) { - log.Warn(r.Context(), "IP is not whitelisted for external authentication", "proxy-ip", reverseProxyIp, "client-ip", r.RemoteAddr) - return "" - } username := r.Header.Get(conf.Server.ExtAuth.UserHeader) if username == "" { return "" } + if !validateIPAgainstList(reverseProxyIp, conf.Server.ExtAuth.TrustedSources) { + log.Warn(r.Context(), "IP is not whitelisted for external authentication", "proxy-ip", reverseProxyIp, "client-ip", r.RemoteAddr) + return "" + } log.Trace(r, "Found username in ExtAuth.UserHeader", "username", username) return username } diff --git a/server/auth_test.go b/server/auth_test.go index af9ffcaeb..abe144a12 100644 --- a/server/auth_test.go +++ b/server/auth_test.go @@ -16,12 +16,15 @@ import ( "github.com/navidrome/navidrome/conf/configtest" "github.com/navidrome/navidrome/consts" "github.com/navidrome/navidrome/core/auth" + "github.com/navidrome/navidrome/log" "github.com/navidrome/navidrome/model" "github.com/navidrome/navidrome/model/id" "github.com/navidrome/navidrome/model/request" "github.com/navidrome/navidrome/tests" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" + "github.com/sirupsen/logrus" + "github.com/sirupsen/logrus/hooks/test" ) var _ = Describe("Auth", func() { @@ -73,6 +76,18 @@ var _ = Describe("Auth", func() { }) }) + Describe("createAdmin when the user cannot be stored", func() { + It("responds 500 rather than falling through to login", func() { + failing := dsWithFailingPut(errors.New("db is down")) + req = httptest.NewRequest("POST", "/createAdmin", strings.NewReader(`{"username":"johndoe", "password":"secret"}`)) + resp = httptest.NewRecorder() + + createAdmin(failing)(resp, req) + + Expect(resp.Code).To(Equal(http.StatusInternalServerError)) + }) + }) + Describe("Login from HTTP headers", func() { const ( trustedIpv4 = "192.168.0.42" @@ -234,6 +249,56 @@ var _ = Describe("Auth", func() { }) }) + Describe("UsernameFromExtAuthHeader", func() { + var hook *test.Hook + var r *http.Request + + BeforeEach(func() { + conf.Server.ExtAuth.TrustedSources = "192.168.0.0/16" + prevLevel := log.CurrentLevel() + l, h := test.NewNullLogger() + hook = h + prevLogger := log.SetDefaultLogger(l) + log.SetLevel(log.LevelWarn) + DeferCleanup(func() { + log.SetDefaultLogger(prevLogger) + log.SetLevel(prevLevel) + }) + r = httptest.NewRequest("GET", "/", nil) + }) + + warnings := func() []*logrus.Entry { + var ws []*logrus.Entry + for _, e := range hook.AllEntries() { + if e.Level == logrus.WarnLevel { + ws = append(ws, e) + } + } + return ws + } + + It("returns the username from a trusted source", func() { + r.Header.Set("Remote-User", "janedoe") + r = r.WithContext(request.WithReverseProxyIp(r.Context(), "192.168.0.42")) + Expect(UsernameFromExtAuthHeader(r)).To(Equal("janedoe")) + Expect(warnings()).To(BeEmpty()) + }) + + It("does not warn when an untrusted source sends no user header", func() { + r = r.WithContext(request.WithReverseProxyIp(r.Context(), "8.8.8.8")) + Expect(UsernameFromExtAuthHeader(r)).To(BeEmpty()) + Expect(warnings()).To(BeEmpty()) + }) + + It("warns when an untrusted source sends the user header", func() { + r.Header.Set("Remote-User", "janedoe") + r = r.WithContext(request.WithReverseProxyIp(r.Context(), "8.8.8.8")) + Expect(UsernameFromExtAuthHeader(r)).To(BeEmpty()) + Expect(warnings()).To(HaveLen(1)) + Expect(warnings()[0].Message).To(Equal("IP is not whitelisted for external authentication")) + }) + }) + Describe("tokenFromHeader", func() { It("returns the token when the Authorization header is set correctly", func() { req := httptest.NewRequest("GET", "/", nil) diff --git a/server/initial_setup.go b/server/initial_setup.go index 7e974dc21..e75220abe 100644 --- a/server/initial_setup.go +++ b/server/initial_setup.go @@ -16,7 +16,7 @@ import ( func initialSetup(ds model.DataStore) { ctx := context.TODO() - _ = ds.WithTx(func(tx model.DataStore) error { + err := ds.WithTx(func(tx model.DataStore) error { if err := tx.Library(ctx).StoreMusicFolder(); err != nil { return err } @@ -36,6 +36,9 @@ func initialSetup(ds model.DataStore) { err = properties.Put(consts.InitialSetupFlagKey, time.Now().String()) return err }, "initial setup") + if err != nil { + log.Fatal("Error running initial setup", err) + } } // If the Dev Admin user is not present, create it @@ -43,7 +46,7 @@ func createInitialAdminUser(ds model.DataStore, initialPassword string) error { users := ds.User(context.TODO()) c, err := users.CountAll(model.QueryOptions{Filters: squirrel.Eq{"user_name": consts.DevInitialUserName}}) if err != nil { - panic(fmt.Sprintf("Could not access User table: %s", err)) + return fmt.Errorf("could not access User table: %w", err) } if c == 0 { newID := id.NewRandom() @@ -57,12 +60,11 @@ func createInitialAdminUser(ds model.DataStore, initialPassword string) error { NewPassword: initialPassword, IsAdmin: true, } - err := users.Put(&initialUser) - if err != nil { - log.Error("Could not create initial admin user", "user", initialUser, err) + if err := users.Put(&initialUser); err != nil { + return fmt.Errorf("could not create initial admin user: %w", err) } } - return err + return nil } func checkFFmpegInstallation() { diff --git a/server/initial_setup_test.go b/server/initial_setup_test.go index 982046f78..0ce8a39fa 100644 --- a/server/initial_setup_test.go +++ b/server/initial_setup_test.go @@ -2,6 +2,7 @@ package server import ( "context" + "errors" "github.com/navidrome/navidrome/model" "github.com/navidrome/navidrome/tests" @@ -9,6 +10,17 @@ import ( . "github.com/onsi/gomega" ) +type failingPutUserRepo struct { + model.UserRepository + err error +} + +func (r *failingPutUserRepo) Put(*model.User) error { return r.err } + +func dsWithFailingPut(err error) model.DataStore { + return &tests.MockDataStore{MockedUser: &failingPutUserRepo{UserRepository: tests.CreateMockUserRepo(), err: err}} +} + var _ = Describe("initial_setup", func() { var ds model.DataStore @@ -32,5 +44,16 @@ var _ = Describe("initial_setup", func() { Expect(createInitialAdminUser(ds, "second")).To(BeNil()) Expect(ur.CountAll()).To(Equal(int64(1))) }) + + It("returns the error when the user cannot be stored", func() { + boom := errors.New("db is down") + Expect(createInitialAdminUser(dsWithFailingPut(boom), "pass123")).To(MatchError(boom)) + }) + + It("returns the error when the user table cannot be read", func() { + boom := errors.New("db is down") + ds = &tests.MockDataStore{MockedUser: &tests.MockedUserRepo{Error: boom}} + Expect(createInitialAdminUser(ds, "pass123")).To(MatchError(boom)) + }) }) }) diff --git a/server/jellyfin/README.md b/server/jellyfin/README.md index c0c5e6ca1..e38997569 100644 --- a/server/jellyfin/README.md +++ b/server/jellyfin/README.md @@ -67,9 +67,10 @@ Discovery answers on all IPv4 interfaces, but the advertised address follows `Ba `Port`. If `Address` is a loopback or a single interface IP and `BaseURL` has no host, clients on other networks get an address they cannot reach. Set `BaseURL` to the address clients should use. -Docker: publish the port (`-p 7359:7359/udp`). In bridge mode the server only sees its container -IP, so also set `ND_BASEURL` to the LAN address (for example `http://192.168.1.10:4533`), or use -host networking. Keep UDP 7359 on the LAN: never forward it from the internet. +Docker: use host networking (`network_mode: host` / `--network host`). On Linux, bridge mode does not +deliver broadcasts to the container, even with `-p 7359:7359/udp`, so clients never find the server. +With host networking the server sees the host's IP, so `BaseURL` is not needed for discovery. If host +networking is not an option, leave discovery off. Keep UDP 7359 on the LAN: never forward it from the internet. ## Authentication diff --git a/server/jellyfin/e2e/e2e_suite_test.go b/server/jellyfin/e2e/e2e_suite_test.go index aa93f7e38..5aa38cba1 100644 --- a/server/jellyfin/e2e/e2e_suite_test.go +++ b/server/jellyfin/e2e/e2e_suite_test.go @@ -222,8 +222,14 @@ func createPlaylistAs(user model.User, name string, encodedIds ...string) string } body, err := json.Marshal(map[string]any{"Name": name, "Ids": encodedIds}) Expect(err).ToNot(HaveOccurred()) + return createPlaylistBodyAs(user, string(body)) +} + +// createPlaylistBodyAs posts a raw create body, for tests that need fields the helpers above don't +// build, and returns the new playlist's decoded id. +func createPlaylistBodyAs(user model.User, body string) string { var res map[string]string - parseInto(postAs(user, "/Playlists", string(body)), &res) + parseInto(postAs(user, "/Playlists", body), &res) Expect(res["Id"]).ToNot(BeEmpty()) id, ok := dto.DecodeID(res["Id"]) Expect(ok).To(BeTrue()) diff --git a/server/jellyfin/e2e/playlists_test.go b/server/jellyfin/e2e/playlists_test.go index 174ba8660..3dd53227f 100644 --- a/server/jellyfin/e2e/playlists_test.go +++ b/server/jellyfin/e2e/playlists_test.go @@ -21,12 +21,19 @@ var _ = Describe("Playlists", func() { } order := func(plID string) []string { return names(playlistItems(plID).Items) } + createWith := func(body string) string { return createPlaylistBodyAs(adminUser, body) } + openAccess := func(plID string) bool { + var info dto.PlaylistInfo + parseInto(get("/Playlists/"+enc(plID)), &info) + return info.OpenAccess + } + Describe("create", func() { It("creates an empty playlist", func() { plID := createPlaylist("Empty", nil) var info dto.PlaylistInfo parseInto(get("/Playlists/"+enc(plID)), &info) - Expect(info.OpenAccess).To(BeFalse()) + Expect(info.OpenAccess).To(BeFalse(), "a playlist created without IsPublic stays private") Expect(info.Shares).To(BeEmpty()) Expect(info.ItemIds).To(BeEmpty()) }) @@ -48,6 +55,14 @@ var _ = Describe("Playlists", func() { Expect(playlistItems(plID).TotalRecordCount).To(Equal(3)) // Abbey Road (2) + Help! (1) }) + It("creates a public playlist when the client sends IsPublic true", func() { + Expect(openAccess(createWith(`{"Name":"Public","Ids":[],"IsPublic":true}`))).To(BeTrue()) + }) + + It("creates a private playlist when the client sends IsPublic false", func() { + Expect(openAccess(createWith(`{"Name":"Private","Ids":[],"IsPublic":false}`))).To(BeFalse()) + }) + // dto.DecodeIDs is all-or-nothing: a malformed entry must 404 the whole request, not get // dropped while the well-formed entries are still used to create a playlist. It("404s when one of the Ids is malformed, without creating a playlist", func() { @@ -355,10 +370,7 @@ var _ = Describe("Playlists", func() { It("makes a playlist public", func() { plID := createPlaylist("Make Public", nil) Expect(post("/Playlists/"+enc(plID), `{"Name":"Make Public","IsPublic":true}`).Code).To(Equal(http.StatusNoContent)) - - var info dto.PlaylistInfo - parseInto(get("/Playlists/"+enc(plID)), &info) - Expect(info.OpenAccess).To(BeTrue()) + Expect(openAccess(plID)).To(BeTrue()) // Now visible to other users. Expect(queryResult(getAs(regularUser, "/Items?IncludeItemTypes=Playlist&Recursive=true")).TotalRecordCount).To(Equal(1)) }) diff --git a/server/jellyfin/playlists.go b/server/jellyfin/playlists.go index 1052a398f..5fd2df8c9 100644 --- a/server/jellyfin/playlists.go +++ b/server/jellyfin/playlists.go @@ -48,6 +48,7 @@ type createPlaylistRequest struct { Name string `json:"Name"` Ids []string `json:"Ids"` MediaType string `json:"MediaType"` + IsPublic *bool `json:"IsPublic"` } // createPlaylist always creates a new playlist (playlistId "" tells core/playlists.Create not to @@ -69,6 +70,13 @@ func (api *Router) createPlaylist(w http.ResponseWriter, r *http.Request) { api.internalError(w, r, err) return } + // Create takes no visibility, so a requested one costs a second write. + if body.IsPublic != nil { + if err := api.playlists.Update(r.Context(), id, nil, nil, body.IsPublic, nil, nil); err != nil { + api.playlistError(w, r, err) + return + } + } api.ok(w, r, map[string]string{"Id": dto.EncodeID(id)}) } diff --git a/server/jellyfin/playlists_test.go b/server/jellyfin/playlists_test.go index ae0a5da3c..edaa63dd1 100644 --- a/server/jellyfin/playlists_test.go +++ b/server/jellyfin/playlists_test.go @@ -55,6 +55,16 @@ type fakePlaylists struct { deletePlaylistID string deleteErr error + + updatePlaylistID string + updatePublic *bool + updateErr error +} + +func (f *fakePlaylists) Update(_ context.Context, playlistID string, _ *string, _ *string, public *bool, _ []string, _ []int) error { + f.updatePlaylistID = playlistID + f.updatePublic = public + return f.updateErr } func (f *fakePlaylists) Delete(_ context.Context, id string) error { @@ -184,6 +194,31 @@ var _ = Describe("Playlists", func() { invoke(api.createPlaylist, w, r) Expect(w.Code).To(Equal(http.StatusInternalServerError)) }) + + createReq := func(body string) *http.Request { + return httptest.NewRequest("POST", "/Playlists", strings.NewReader(body)). + WithContext(GinkgoT().Context()) + } + + DescribeTable("visibility", + func(body string, wantPublic *bool, wantUpdatedID string) { + w := httptest.NewRecorder() + invoke(api.createPlaylist, w, createReq(body)) + Expect(w.Code).To(Equal(http.StatusOK)) + Expect(fp.updatePublic).To(Equal(wantPublic)) + Expect(fp.updatePlaylistID).To(Equal(wantUpdatedID)) + }, + Entry("applies IsPublic true", `{"Name":"Mix","IsPublic":true}`, new(true), testID("pl-new")), + Entry("applies an explicit IsPublic false", `{"Name":"Mix","IsPublic":false}`, new(false), testID("pl-new")), + Entry("leaves visibility alone when IsPublic is omitted", `{"Name":"Mix"}`, nil, ""), + ) + + It("returns 500 when the visibility update fails", func() { + fp.updateErr = errors.New("boom") + w := httptest.NewRecorder() + invoke(api.createPlaylist, w, createReq(`{"Name":"Mix","IsPublic":true}`)) + Expect(w.Code).To(Equal(http.StatusInternalServerError)) + }) }) Describe("getPlaylistItems", func() { diff --git a/server/middlewares.go b/server/middlewares.go index c54e947f3..674337e92 100644 --- a/server/middlewares.go +++ b/server/middlewares.go @@ -232,15 +232,20 @@ func trustedProxyPrefixes(list string) []string { return prefixes } -// ClientIPRateLimiter returns a rate limiter keyed by the client IP resolved by realIPMiddleware, -// so spoofed forwarding headers cannot be rotated for a fresh bucket. It falls back to the peer -// address, so that a missing middleware degrades to per-peer limiting rather than one shared bucket. +// ClientIPRateLimiter returns a rate limiter keyed by ClientIP, so spoofed forwarding headers +// cannot be rotated for a fresh bucket. func ClientIPRateLimiter(requestLimit int, windowLength time.Duration) func(http.Handler) http.Handler { return httprate.LimitBy(requestLimit, windowLength, func(r *http.Request) (string, error) { - return httprate.CanonicalizeIP(cmp.Or(middleware.GetClientIP(r.Context()), peerHost(r))), nil + return ClientIP(r), nil }) } +// ClientIP returns the canonical client IP resolved by realIPMiddleware, for keying rate limits. The +// peer address fallback degrades a missing middleware to per-peer limiting, not one shared bucket. +func ClientIP(r *http.Request) string { + return httprate.CanonicalizeIP(cmp.Or(middleware.GetClientIP(r.Context()), peerHost(r))) +} + // reqToCtx creates a middleware that updates the request's context with a value computed from the request. A given key // can only be set once. func reqToCtx(key any, fn func(req *http.Request) any) func(http.Handler) http.Handler { diff --git a/server/nativeapi/delete_many_response_test.go b/server/nativeapi/delete_many_response_test.go new file mode 100644 index 000000000..7d911d6cd --- /dev/null +++ b/server/nativeapi/delete_many_response_test.go @@ -0,0 +1,49 @@ +package nativeapi + +import ( + "encoding/json" + "net/http" + "net/http/httptest" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" +) + +var _ = Describe("writeDeleteManyResponse", func() { + var w *httptest.ResponseRecorder + + write := func(ids ...string) map[string]any { + w = httptest.NewRecorder() + writeDeleteManyResponse(w, httptest.NewRequest("DELETE", "/missing", nil), ids) + + var body map[string]any + Expect(json.Unmarshal(w.Body.Bytes(), &body)).To(Succeed(), "response body must be valid JSON: %s", w.Body.String()) + return body + } + + It("returns a single id as an object", func() { + Expect(write("abc123")).To(HaveKeyWithValue("id", "abc123")) + }) + + It("returns multiple ids as a list", func() { + Expect(write("a", "b")).To(HaveKeyWithValue("ids", ConsistOf("a", "b"))) + }) + + It("stays valid JSON when the id contains a backslash", func() { + Expect(write(`a\`)).To(HaveKeyWithValue("id", `a\`)) + }) + + It("stays valid JSON when the id contains a quote", func() { + Expect(write(`a"b`)).To(HaveKeyWithValue("id", `a"b`)) + }) + + It("does not HTML-escape the id into entities", func() { + Expect(write("a&b")).To(HaveKeyWithValue("id", "a&b")) + }) + + It("responds 200 with a JSON content type", func() { + write("abc123") + Expect(w.Code).To(Equal(http.StatusOK)) + Expect(w.Header().Get("Content-Type")).To(Equal("application/json")) + }) +}) diff --git a/server/nativeapi/native_api.go b/server/nativeapi/native_api.go index f931834c6..a7c53df09 100644 --- a/server/nativeapi/native_api.go +++ b/server/nativeapi/native_api.go @@ -2,8 +2,6 @@ package nativeapi import ( "context" - "encoding/json" - "html" "net/http" "strconv" "time" @@ -206,22 +204,18 @@ func (api *Router) addMissingFilesRoute(r chi.Router) { } func writeDeleteManyResponse(w http.ResponseWriter, r *http.Request, ids []string) { - var resp []byte - var err error + var payload any if len(ids) == 1 { - resp = []byte(`{"id":"` + html.EscapeString(ids[0]) + `"}`) + payload = struct { + ID string `json:"id"` + }{ID: ids[0]} } else { - resp, err = json.Marshal(&struct { + payload = struct { Ids []string `json:"ids"` - }{Ids: ids}) - if err != nil { - log.Error(r.Context(), "Error marshaling response", "ids", ids, err) - http.Error(w, err.Error(), http.StatusInternalServerError) - } + }{Ids: ids} } - _, err = w.Write(resp) //nolint:gosec - if err != nil { - http.Error(w, err.Error(), http.StatusInternalServerError) + if err := rest.RespondWithJSON(w, http.StatusOK, payload); err != nil { + log.Error(r.Context(), "Error writing response", "ids", ids, err) } } diff --git a/server/nativeapi/playlists.go b/server/nativeapi/playlists.go index d215eb9dd..00c1575a5 100644 --- a/server/nativeapi/playlists.go +++ b/server/nativeapi/playlists.go @@ -16,6 +16,7 @@ import ( "github.com/navidrome/navidrome/log" "github.com/navidrome/navidrome/model" "github.com/navidrome/navidrome/utils/req" + "github.com/navidrome/navidrome/utils/str" ) type restHandler = func(rest.RepositoryConstructor, ...rest.Logger) http.HandlerFunc @@ -101,8 +102,7 @@ func handleExportPlaylist(pls playlists.Playlists) http.HandlerFunc { log.Debug(ctx, "Exporting playlist as M3U", "playlistId", plsId, "name", playlist.Name) w.Header().Set("Content-Type", "audio/x-mpegurl") - disposition := fmt.Sprintf("attachment; filename=\"%s.m3u\"", playlist.Name) - w.Header().Set("Content-Disposition", disposition) + w.Header().Set("Content-Disposition", str.ContentDispositionAttachment(playlist.Name+".m3u")) _, err = w.Write([]byte(playlist.ToM3U8())) //nolint:gosec if err != nil { diff --git a/server/nativeapi/playlists_test.go b/server/nativeapi/playlists_test.go index 2f6c578f9..349b4a662 100644 --- a/server/nativeapi/playlists_test.go +++ b/server/nativeapi/playlists_test.go @@ -9,6 +9,7 @@ import ( "time" "github.com/deluan/rest" + "github.com/go-chi/chi/v5" "github.com/navidrome/navidrome/conf" "github.com/navidrome/navidrome/conf/configtest" "github.com/navidrome/navidrome/consts" @@ -183,6 +184,37 @@ var _ = Describe("Playlist Tracks Endpoint", func() { }) }) +var _ = Describe("handleExportPlaylist", func() { + export := func(name string) *httptest.ResponseRecorder { + r := chi.NewRouter() + r.Get("/playlist/{playlistId}", handleExportPlaylist(&mockPlaylistsService{ + playlist: &model.Playlist{ID: "pls-1", Name: name}, + })) + w := httptest.NewRecorder() + r.ServeHTTP(w, httptest.NewRequest("GET", "/playlist/pls-1", nil)) + return w + } + + It("names the download after the playlist", func() { + w := export("Road Trip") + + Expect(w.Code).To(Equal(http.StatusOK)) + Expect(w.Header().Get("Content-Disposition")).To(Equal(`attachment; filename="Road Trip.m3u"`)) + }) + + It("does not let the playlist name inject a second filename parameter", func() { + w := export(`party"; filename="evil.html`) + + Expect(w.Header().Get("Content-Disposition")).To(Equal(`attachment; filename="party_; filename=_evil.html.m3u"`)) + }) + + It("keeps non-ASCII names in filename*", func() { + w := export("Кино") + + Expect(w.Header().Get("Content-Disposition")).To(Equal(`attachment; filename="download.m3u"; filename*=utf-8''%D0%9A%D0%B8%D0%BD%D0%BE.m3u`)) + }) +}) + var _ = Describe("writePlaylistError", func() { DescribeTable("maps a service error to an HTTP status", func(err error, expected int) { @@ -231,6 +263,7 @@ func (m *mockPlaylistTrackRepo) Read(id string) (any, error) { type mockPlaylistsService struct { playlists.Playlists tracksRepo rest.Repository + playlist *model.Playlist removeImageFn func(ctx context.Context, id string) error setImageFn func(ctx context.Context, id string, reader io.Reader, ext string) error } @@ -249,6 +282,13 @@ func (m *mockPlaylistsService) SetImage(ctx context.Context, id string, reader i return model.ErrNotFound } +func (m *mockPlaylistsService) GetWithTracks(_ context.Context, _ string) (*model.Playlist, error) { + if m.playlist == nil { + return nil, model.ErrNotFound + } + return m.playlist, nil +} + func (m *mockPlaylistsService) TracksRepository(_ context.Context, _ string, _ bool) rest.Repository { return m.tracksRepo } diff --git a/server/public/handle_downloads.go b/server/public/handle_downloads.go index 0012c4b35..e50d7609a 100644 --- a/server/public/handle_downloads.go +++ b/server/public/handle_downloads.go @@ -2,9 +2,7 @@ package public import ( "cmp" - "fmt" "net/http" - "strings" "github.com/navidrome/navidrome/model" "github.com/navidrome/navidrome/utils/req" @@ -31,9 +29,8 @@ func (pub *Router) handleDownloads(w http.ResponseWriter, r *http.Request) { return } - name := str.SanitizeFilename(cmp.Or(s.Description, s.ID)) - name = strings.ReplaceAll(name, ",", "_") - w.Header().Set("Content-Disposition", fmt.Sprintf("attachment; filename=%q", name+".zip")) + name := cmp.Or(s.Description, s.ID) + w.Header().Set("Content-Disposition", str.ContentDispositionAttachment(name+".zip")) w.Header().Set("Content-Type", "application/zip") err = pub.archiver.ZipShare(ctx, s, w) diff --git a/server/public/handle_downloads_test.go b/server/public/handle_downloads_test.go index 1a97f4379..d2118f47e 100644 --- a/server/public/handle_downloads_test.go +++ b/server/public/handle_downloads_test.go @@ -4,6 +4,7 @@ import ( "context" "errors" "io" + "mime" "net/http" "net/http/httptest" "time" @@ -95,6 +96,26 @@ var _ = Describe("handleDownloads", func() { Expect(w.Header().Get("Content-Disposition")).To(Equal(`attachment; filename="AC_DC_ Live_ 1979.zip"`)) }) + It("sanitizes the UTF-8 filename* as well", func() { + shareIs(&model.Share{ID: "abc123", Description: "Sigur Rós/Live", Downloadable: true}) + + w := makeRequest("abc123") + + Expect(w.Header().Get("Content-Disposition")).To(Equal(`attachment; filename="Sigur Ros_Live.zip"; filename*=utf-8''Sigur%20R%C3%B3s_Live.zip`)) + }) + + It("does not let the share description inject a second filename parameter", func() { + shareIs(&model.Share{ID: "abc123", Description: `mix"; filename="evil.html`, Downloadable: true}) + + w := makeRequest("abc123") + + disposition := w.Header().Get("Content-Disposition") + Expect(disposition).ToNot(ContainSubstring(`filename="evil.html`)) + _, params, err := mime.ParseMediaType(disposition) + Expect(err).ToNot(HaveOccurred()) + Expect(params["filename"]).To(Equal(`mix_; filename=_evil.html.zip`)) + }) + It("returns 403 without invoking the archiver when the share is not downloadable", func() { shareIs(&model.Share{ID: "abc123", Description: "No Download", Downloadable: false}) diff --git a/server/public/handle_shares.go b/server/public/handle_shares.go index d67cfe456..367bff501 100644 --- a/server/public/handle_shares.go +++ b/server/public/handle_shares.go @@ -58,8 +58,8 @@ func (pub *Router) handleM3U(w http.ResponseWriter, r *http.Request) { } s = pub.mapShareToM3U(r, *s) - w.WriteHeader(http.StatusOK) w.Header().Set("Content-Type", "audio/x-mpegurl") + w.WriteHeader(http.StatusOK) _, _ = w.Write([]byte(s.ToM3U8())) //nolint:gosec } diff --git a/server/public/handle_shares_test.go b/server/public/handle_shares_test.go new file mode 100644 index 000000000..1bf631fd4 --- /dev/null +++ b/server/public/handle_shares_test.go @@ -0,0 +1,52 @@ +package public + +import ( + "net/http" + "net/http/httptest" + + "github.com/navidrome/navidrome/core" + "github.com/navidrome/navidrome/model" + "github.com/navidrome/navidrome/tests" + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" +) + +var _ = Describe("handleM3U", func() { + var ds *tests.MockDataStore + var shareRepo *tests.MockShareRepo + var pub *Router + + BeforeEach(func() { + ds = &tests.MockDataStore{} + shareRepo = &tests.MockShareRepo{} + ds.MockedShare = shareRepo + pub = &Router{ds: ds, share: core.NewShare(ds)} + }) + + makeRequest := func(id string) *httptest.ResponseRecorder { + r := httptest.NewRequest("GET", "/public/"+id+"/m3u?%3Aid="+id, nil) + w := httptest.NewRecorder() + pub.handleM3U(w, r) + return w + } + + It("sets the M3U content type", func() { + share := &model.Share{ID: "abc123", Tracks: model.MediaFiles{{ID: "t1", Title: "Track 1"}}} + shareRepo.ID = share.ID + shareRepo.Entity = share + + w := makeRequest("abc123") + + Expect(w.Code).To(Equal(http.StatusOK)) + // Result() has the headers sent at WriteHeader time, unlike w.Header() + Expect(w.Result().Header.Get("Content-Type")).To(Equal("audio/x-mpegurl")) + Expect(w.Body.String()).To(HavePrefix("#EXTM3U")) + }) + + It("returns 404 when the share does not exist", func() { + shareRepo.ID = "other" + shareRepo.Entity = &model.Share{ID: "other"} + + Expect(makeRequest("missing").Code).To(Equal(http.StatusNotFound)) + }) +}) diff --git a/server/subsonic/auth_limiter.go b/server/subsonic/auth_limiter.go new file mode 100644 index 000000000..88df6eb50 --- /dev/null +++ b/server/subsonic/auth_limiter.go @@ -0,0 +1,134 @@ +package subsonic + +import ( + "cmp" + "context" + "hash/maphash" + "sync" + "time" + + "github.com/navidrome/navidrome/consts" +) + +// authLimiter caps failed Subsonic logins per key. Checks run at most `limit` at a time and failures +// are recorded afterwards, so a window admits up to 2*limit-1 guesses and valid requests only wait. +type authLimiter struct { + limit int + window time.Duration + seed maphash.Seed + mu sync.Mutex + keys map[uint64]*authAttempts // hashed, so attacker-chosen usernames cannot bloat memory + lastSweep time.Time +} + +type authAttempts struct { + failures int + start time.Time + slots chan struct{} + refs int +} + +// authSlot is a reserved credential check. A nil slot releases nothing, which is what a disabled +// limiter hands back. +type authSlot struct { + limiter *authLimiter + entry *authAttempts +} + +// newAuthLimiter returns nil when limit is not positive. A nil limiter allows everything. +func newAuthLimiter(limit int, window time.Duration) *authLimiter { + if limit <= 0 { + return nil + } + return &authLimiter{ + limit: limit, + window: cmp.Or(window, consts.DefaultAuthWindowLength), + seed: maphash.MakeSeed(), + keys: map[uint64]*authAttempts{}, + } +} + +// acquire reserves a credential check for key, waiting while other checks for the same key are in +// flight. It only fails when the key already reached `limit` failures in the current window. +func (l *authLimiter) acquire(ctx context.Context, key string) (*authSlot, bool) { + if l == nil { + return nil, true + } + a, ok := l.reserve(key) + if !ok { + return nil, false + } + + select { + case a.slots <- struct{}{}: + case <-ctx.Done(): + l.unref(a) + return nil, false + } + + l.mu.Lock() + blocked := a.failures >= l.limit + if blocked { + a.refs-- + } + l.mu.Unlock() + if blocked { + <-a.slots + return nil, false + } + return &authSlot{limiter: l, entry: a}, true +} + +func (l *authLimiter) reserve(key string) (*authAttempts, bool) { + now := time.Now() + l.mu.Lock() + defer l.mu.Unlock() + l.sweep(now) + + h := maphash.String(l.seed, key) + a := l.keys[h] + switch { + case a == nil: + a = &authAttempts{start: now, slots: make(chan struct{}, l.limit)} + l.keys[h] = a + case now.Sub(a.start) >= l.window: + a.failures, a.start = 0, now + } + if a.failures >= l.limit { + return nil, false + } + a.refs++ + return a, true +} + +func (l *authLimiter) unref(a *authAttempts) { + l.mu.Lock() + a.refs-- + l.mu.Unlock() +} + +func (s *authSlot) release(failed bool) { + if s == nil { + return + } + s.limiter.mu.Lock() + if failed { + s.entry.failures++ + } + s.entry.refs-- + s.limiter.mu.Unlock() + <-s.entry.slots +} + +// sweep drops idle expired keys once per window, so memory is bounded by recent attempts. +func (l *authLimiter) sweep(now time.Time) { + if now.Sub(l.lastSweep) < l.window { + return + } + for h, a := range l.keys { + if a.refs == 0 && now.Sub(a.start) >= l.window { + delete(l.keys, h) + } + } + l.lastSweep = now +} diff --git a/server/subsonic/auth_limiter_test.go b/server/subsonic/auth_limiter_test.go new file mode 100644 index 000000000..66e1a594f --- /dev/null +++ b/server/subsonic/auth_limiter_test.go @@ -0,0 +1,143 @@ +package subsonic + +import ( + "context" + "sync" + "sync/atomic" + "testing" + "testing/synctest" + "time" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" +) + +var _ = Describe("authLimiter", func() { + var ctx context.Context + + BeforeEach(func() { + ctx = context.Background() + }) + + acquire := func(l *authLimiter, key string) (*authSlot, bool) { + GinkgoHelper() + return l.acquire(ctx, key) + } + + It("blocks a key after the configured number of failures", func() { + l := newAuthLimiter(2, time.Minute) + for range 2 { + slot, ok := acquire(l, "k") + Expect(ok).To(BeTrue()) + slot.release(true) + } + + _, ok := acquire(l, "k") + Expect(ok).To(BeFalse()) + }) + + It("never counts successful checks", func() { + l := newAuthLimiter(2, time.Minute) + for range 50 { + slot, ok := acquire(l, "k") + Expect(ok).To(BeTrue()) + slot.release(false) + } + }) + + It("keeps keys independent", func() { + l := newAuthLimiter(1, time.Minute) + slot, _ := acquire(l, "a") + slot.release(true) + _, ok := acquire(l, "a") + Expect(ok).To(BeFalse()) + + _, ok = acquire(l, "b") + Expect(ok).To(BeTrue()) + }) + + It("waits for an in-flight check instead of failing the request", func() { + l := newAuthLimiter(1, time.Minute) + held, ok := acquire(l, "k") + Expect(ok).To(BeTrue()) + + waiting := make(chan bool, 1) + go func() { + slot, ok := l.acquire(ctx, "k") + slot.release(false) + waiting <- ok + }() + Consistently(waiting, 50*time.Millisecond).ShouldNot(Receive()) + + held.release(false) + Eventually(waiting).Should(Receive(BeTrue())) + }) + + It("stops waiting when the request is canceled", func() { + l := newAuthLimiter(1, time.Minute) + held, _ := acquire(l, "k") + DeferCleanup(func() { held.release(false) }) + + canceled, cancel := context.WithCancel(context.Background()) + cancel() + _, ok := l.acquire(canceled, "k") + Expect(ok).To(BeFalse()) + }) + + It("does not let concurrent guesses overshoot the limit", func() { + l := newAuthLimiter(5, time.Minute) + hold := make(chan struct{}) + var checks atomic.Int32 + var wg sync.WaitGroup + for range 50 { + wg.Go(func() { + slot, ok := l.acquire(ctx, "k") + if !ok { + return + } + checks.Add(1) + <-hold + slot.release(true) + }) + } + + Eventually(checks.Load).Should(Equal(int32(5))) + Consistently(checks.Load, 100*time.Millisecond).Should(Equal(int32(5))) + close(hold) + wg.Wait() + + _, ok := acquire(l, "k") + Expect(ok).To(BeFalse()) + }) + + It("allows everything when the limit is disabled", func() { + l := newAuthLimiter(0, time.Minute) + for range 10 { + slot, ok := acquire(l, "k") + Expect(ok).To(BeTrue()) + slot.release(true) + } + }) +}) + +// testing/synctest's fake clock needs a *testing.T, which Ginkgo doesn't give. +func TestAuthLimiterWindow(t *testing.T) { + synctest.Test(t, func(t *testing.T) { + g := NewWithT(t) + ctx := context.Background() + l := newAuthLimiter(1, 20*time.Second) + for _, key := range []string{"a", "b"} { + slot, ok := l.acquire(ctx, key) + g.Expect(ok).To(BeTrue()) + slot.release(true) + } + _, ok := l.acquire(ctx, "a") + g.Expect(ok).To(BeFalse()) + + time.Sleep(20 * time.Second) + slot, ok := l.acquire(ctx, "a") + g.Expect(ok).To(BeTrue()) + slot.release(false) + g.Expect(l.keys).To(HaveLen(1), "expired keys must be dropped") + }) +} diff --git a/server/subsonic/bookmarks.go b/server/subsonic/bookmarks.go index 4a7ebaa6c..7ac492ca8 100644 --- a/server/subsonic/bookmarks.go +++ b/server/subsonic/bookmarks.go @@ -47,6 +47,14 @@ func (api *Router) CreateBookmark(r *http.Request) (*responses.Subsonic, error) position := p.Int64Or("position", 0) repo := api.ds.MediaFile(r.Context()) + ok, err := repo.Exists(id) + if err != nil { + return nil, err + } + if !ok { + return nil, newError(responses.ErrorDataNotFound, "Song not found") + } + err = repo.AddBookmark(id, comment, position) if err != nil { return nil, err diff --git a/server/subsonic/bookmarks_test.go b/server/subsonic/bookmarks_test.go new file mode 100644 index 000000000..0fcb81ab9 --- /dev/null +++ b/server/subsonic/bookmarks_test.go @@ -0,0 +1,48 @@ +package subsonic + +import ( + "context" + + "github.com/navidrome/navidrome/model" + "github.com/navidrome/navidrome/model/request" + "github.com/navidrome/navidrome/server/subsonic/responses" + "github.com/navidrome/navidrome/tests" + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" +) + +var _ = Describe("Bookmarks", func() { + var router *Router + var ds *tests.MockDataStore + var mfRepo *tests.MockMediaFileRepo + var ctx context.Context + + BeforeEach(func() { + ds = &tests.MockDataStore{} + router = New(ds, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil) + ctx = request.WithUser(context.Background(), model.User{ID: "u1", UserName: "u1"}) + mfRepo = ds.MediaFile(ctx).(*tests.MockMediaFileRepo) + mfRepo.SetData(model.MediaFiles{{ID: "visible"}}) + }) + + Describe("CreateBookmark", func() { + It("rejects an id the user cannot read", func() { + r := newGetRequest("id=hidden", "position=1").WithContext(ctx) + + _, err := router.CreateBookmark(r) + + Expect(err).To(HaveOccurred()) + Expect(mapToSubsonicError(err).code).To(Equal(responses.ErrorDataNotFound)) + Expect(mfRepo.BookmarksAdded).To(BeEmpty()) + }) + + It("accepts an id the user can read", func() { + r := newGetRequest("id=visible", "position=1").WithContext(ctx) + + _, err := router.CreateBookmark(r) + + Expect(err).ToNot(HaveOccurred()) + Expect(mfRepo.BookmarksAdded).To(ConsistOf("visible")) + }) + }) +}) diff --git a/server/subsonic/e2e/subsonic_media_retrieval_test.go b/server/subsonic/e2e/subsonic_media_retrieval_test.go index 079b131ff..268d93b82 100644 --- a/server/subsonic/e2e/subsonic_media_retrieval_test.go +++ b/server/subsonic/e2e/subsonic_media_retrieval_test.go @@ -106,7 +106,7 @@ var _ = Describe("Media Retrieval Endpoints", Ordered, func() { }) Describe("Download", func() { - var trackID string + var trackID, trackTitle string BeforeAll(func() { // All test tracks are mp3 at 320kbps @@ -114,6 +114,7 @@ var _ = Describe("Media Retrieval Endpoints", Ordered, func() { Expect(err).ToNot(HaveOccurred()) Expect(songs).ToNot(BeEmpty()) trackID = songs[0].ID + trackTitle = songs[0].Title }) It("returns error when id parameter is missing", func() { @@ -143,6 +144,7 @@ var _ = Describe("Media Retrieval Endpoints", Ordered, func() { Expect(w.Code).To(Equal(http.StatusOK)) Expect(streamerSpy.LastRequest.Format).To(Equal("opus")) Expect(streamerSpy.LastRequest.BitRate).To(Equal(128)) + Expect(w.Header().Get("Content-Disposition")).To(Equal(`attachment; filename="` + trackTitle + `.opus"`)) }) It("returns error when downloads are disabled", func() { diff --git a/server/subsonic/e2e/subsonic_multilibrary_test.go b/server/subsonic/e2e/subsonic_multilibrary_test.go index 98f87ca17..18e8c6391 100644 --- a/server/subsonic/e2e/subsonic_multilibrary_test.go +++ b/server/subsonic/e2e/subsonic_multilibrary_test.go @@ -224,6 +224,24 @@ var _ = Describe("Multi-Library Support", Ordered, func() { Expect(resp.Playlist.Entry).To(HaveLen(1)) Expect(resp.Playlist.Entry[0].Id).To(Equal(lib1SongID)) }) + + It("non-admin user cannot store a song from another library through createPlaylist", func() { + resp := doReqWithUser(userLib1Only, "createPlaylist", + "name", "Restricted Playlist", "songId", lib1SongID, "songId", lib2SongID) + Expect(resp.Status).To(Equal(responses.StatusOK)) + ownID := resp.Playlist.Id + + stored := doReqWithUser(adminWithLibs, "getPlaylist", "id", ownID) + Expect(stored.Playlist.Entry).To(HaveLen(1), "the lib2 song must not be persisted") + Expect(stored.Playlist.Entry[0].Id).To(Equal(lib1SongID)) + + By("replacing the tracks of the same playlist") + resp = doReqWithUser(userLib1Only, "createPlaylist", "playlistId", ownID, "songId", lib2SongID) + Expect(resp.Status).To(Equal(responses.StatusOK)) + + stored = doReqWithUser(adminWithLibs, "getPlaylist", "id", ownID) + Expect(stored.Playlist.Entry).To(BeEmpty()) + }) }) Describe("Cross-library shares", Ordered, func() { diff --git a/server/subsonic/media_annotation.go b/server/subsonic/media_annotation.go index cfbff3ecb..a43162cf4 100644 --- a/server/subsonic/media_annotation.go +++ b/server/subsonic/media_annotation.go @@ -181,6 +181,9 @@ func (api *Router) Scrobble(r *http.Request) (*responses.Subsonic, error) { log.Error(ctx, "Error registering scrobbles", "ids", ids, "times", times, err) } } else { + if len(ids) > 1 { + log.Warn(ctx, "Multiple ids sent to a nowPlaying notification, only the first one will be used", "ids", ids) + } err := api.scrobblerNowPlaying(ctx, ids[0], position) if err != nil { log.Error(ctx, "Error setting NowPlaying", "id", ids[0], err) diff --git a/server/subsonic/middlewares.go b/server/subsonic/middlewares.go index 6dfa2263f..35e13eaa5 100644 --- a/server/subsonic/middlewares.go +++ b/server/subsonic/middlewares.go @@ -98,6 +98,7 @@ func checkRequiredParameters(next http.Handler) http.Handler { } func authenticate(ds model.DataStore) func(next http.Handler) http.Handler { + limiter := newAuthLimiter(conf.Server.AuthRequestLimit, conf.Server.AuthWindowLength) return func(next http.Handler) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { ctx := r.Context() @@ -126,21 +127,32 @@ func authenticate(ds model.DataStore) func(next http.Handler) http.Handler { salt, _ := p.String("s") jwt, _ := p.String("jwt") - usr, err = ds.User(ctx).FindByUsernameWithPassword(username) - if errors.Is(err, context.Canceled) { - log.Debug(ctx, "API: Request canceled when authenticating", "auth", "subsonic", "username", username, "remoteAddr", r.RemoteAddr, err) + // Blocked attempts get the same response as a wrong password, so they reveal nothing + limitKey := server.ClientIP(r) + "\x00" + strings.ToLower(username) + slot, allowed := limiter.acquire(ctx, limitKey) + if !allowed { + if ctx.Err() != nil { + return + } + log.Warn(ctx, "API: Too many failed login attempts", "auth", "subsonic", "username", username, "remoteAddr", r.RemoteAddr) + sendError(w, r, newError(responses.ErrorAuthenticationFail)) return } + + usr, err = ds.User(ctx).FindByUsernameWithPassword(username) + if err == nil { + err = validateCredentials(usr, pass, token, salt, jwt) + } + invalidLogin := errors.Is(err, model.ErrNotFound) || errors.Is(err, model.ErrInvalidAuth) + slot.release(invalidLogin) switch { - case errors.Is(err, model.ErrNotFound): + case errors.Is(err, context.Canceled): + log.Debug(ctx, "API: Request canceled when authenticating", "auth", "subsonic", "username", username, "remoteAddr", r.RemoteAddr, err) + return + case invalidLogin: log.Warn(ctx, "API: Invalid login", "auth", "subsonic", "username", username, "remoteAddr", r.RemoteAddr, err) case err != nil: log.Error(ctx, "API: Error authenticating username", "auth", "subsonic", "username", username, "remoteAddr", r.RemoteAddr, err) - default: - err = validateCredentials(usr, pass, token, salt, jwt) - if err != nil { - log.Warn(ctx, "API: Invalid login", "auth", "subsonic", "username", username, "remoteAddr", r.RemoteAddr, err) - } } } diff --git a/server/subsonic/middlewares_test.go b/server/subsonic/middlewares_test.go index cb34b92e7..62de09e76 100644 --- a/server/subsonic/middlewares_test.go +++ b/server/subsonic/middlewares_test.go @@ -8,6 +8,8 @@ import ( "net/http" "net/http/httptest" "strings" + "sync" + "sync/atomic" "time" "github.com/navidrome/navidrome/conf" @@ -306,6 +308,166 @@ var _ = Describe("Middlewares", func() { Expect(next.called).To(BeFalse()) }) }) + + When("failed attempts reach AuthRequestLimit", func() { + var cp http.Handler + + BeforeEach(func() { + DeferCleanup(configtest.SetupConfig()) + conf.Server.AuthRequestLimit = 3 + conf.Server.AuthWindowLength = time.Minute + cp = authenticate(ds)(next) + }) + + serve := func(r *http.Request) *httptest.ResponseRecorder { + next.called = false + rec := httptest.NewRecorder() + cp.ServeHTTP(rec, r) + return rec + } + failTimes := func(n int, params ...string) { + for range n { + Expect(serve(newGetRequest(params...)).Body.String()).To(ContainSubstring(`code="40"`)) + } + } + + It("rejects the correct password exactly like a wrong one", func() { + failTimes(3, "u=admin", "p=WRONG") + + rec := serve(newGetRequest("u=admin", "p=wordpass")) + + Expect(next.called).To(BeFalse()) + Expect(rec.Code).To(Equal(http.StatusOK)) + Expect(rec.Body.String()).To(ContainSubstring(`code="40"`)) + Expect(rec.Header().Get("Retry-After")).To(BeEmpty()) + }) + + It("counts attempts against unknown usernames", func() { + failTimes(3, "u=newuser", "p=secret") + _ = ds.User(context.TODO()).Put(&model.User{UserName: "newuser", NewPassword: "secret"}) + + serve(newGetRequest("u=newuser", "p=secret")) + Expect(next.called).To(BeFalse()) + }) + + It("treats usernames case-insensitively", func() { + failTimes(3, "u=ADMIN", "p=WRONG") + + serve(newGetRequest("u=admin", "p=wordpass")) + Expect(next.called).To(BeFalse()) + }) + + It("does not count successful logins", func() { + for range 10 { + serve(newGetRequest("u=admin", "p=wordpass")) + Expect(next.called).To(BeTrue()) + } + }) + + It("does not count server errors", func() { + userRepo := ds.User(context.TODO()).(*tests.MockedUserRepo) + userRepo.Error = errors.New("db down") + failTimes(5, "u=admin", "p=wordpass") + userRepo.Error = nil + + serve(newGetRequest("u=admin", "p=wordpass")) + Expect(next.called).To(BeTrue()) + }) + + It("does not block other usernames from the same IP", func() { + _ = ds.User(context.TODO()).Put(&model.User{UserName: "other", NewPassword: "otherpass"}) + failTimes(3, "u=admin", "p=WRONG") + + serve(newGetRequest("u=other", "p=otherpass")) + Expect(next.called).To(BeTrue()) + }) + + It("does not block the same username from another IP", func() { + failTimes(3, "u=admin", "p=WRONG") + + r := newGetRequest("u=admin", "p=wordpass") + r.RemoteAddr = "198.51.100.7:1234" + serve(r) + Expect(next.called).To(BeTrue()) + }) + + It("does not limit reverse proxy authentication", func() { + conf.Server.ExtAuth.TrustedSources = "192.168.1.1/24" + conf.Server.ExtAuth.UserHeader = "Remote-User" + failTimes(3, "u=admin", "p=WRONG") + + r := newGetRequest() + r.Header.Add("Remote-User", "admin") + r = r.WithContext(request.WithReverseProxyIp(r.Context(), "192.168.1.1")) + serve(r) + Expect(next.called).To(BeTrue()) + }) + + It("is disabled when AuthRequestLimit is 0", func() { + conf.Server.AuthRequestLimit = 0 + cp = authenticate(ds)(next) + failTimes(10, "u=admin", "p=WRONG") + + serve(newGetRequest("u=admin", "p=wordpass")) + Expect(next.called).To(BeTrue()) + }) + }) + + When("valid requests overlap", func() { + var gate *gatedUserRepo + var gatedDS model.DataStore + + BeforeEach(func() { + DeferCleanup(configtest.SetupConfig()) + conf.Server.AuthRequestLimit = 5 + conf.Server.AuthWindowLength = time.Minute + gate = &gatedUserRepo{ + UserRepository: ds.User(context.TODO()), + entered: make(chan struct{}, 64), + proceed: make(chan struct{}), + } + gatedDS = &gatedDataStore{DataStore: ds, users: gate} + }) + + It("lets every valid request through while checks are in flight", func() { + const burst = 6 + cp := authenticate(gatedDS)(&countingHandler{}) + var passed atomic.Int32 + var wg sync.WaitGroup + for range burst { + wg.Go(func() { + rec := httptest.NewRecorder() + cp.ServeHTTP(rec, newGetRequest("u=admin", "p=wordpass")) + if !strings.Contains(rec.Body.String(), `code="40"`) { + passed.Add(1) + } + }) + } + for range conf.Server.AuthRequestLimit { + Eventually(gate.entered).Should(Receive()) + } + close(gate.proceed) + wg.Wait() + + Expect(passed.Load()).To(Equal(int32(burst))) + }) + + It("caps concurrent credential checks for wrong passwords", func() { + cp := authenticate(gatedDS)(&countingHandler{}) + var wg sync.WaitGroup + for i := range 100 { + wg.Go(func() { + cp.ServeHTTP(httptest.NewRecorder(), newGetRequest("u=admin", fmt.Sprintf("p=wrong%d", i))) + }) + } + + limit := int32(conf.Server.AuthRequestLimit) + Eventually(gate.lookups.Load).Should(Equal(limit)) + Consistently(gate.lookups.Load, 100*time.Millisecond).Should(Equal(limit)) + close(gate.proceed) + wg.Wait() + }) + }) }) Describe("AdminOnly", func() { @@ -560,3 +722,28 @@ func (mp *mockPlayers) Register(ctx context.Context, id, client, typ, ip string) } return &model.Player{ID: id}, mp.transcoding, nil } + +type gatedDataStore struct { + model.DataStore + users model.UserRepository +} + +func (g *gatedDataStore) User(context.Context) model.UserRepository { return g.users } + +type gatedUserRepo struct { + model.UserRepository + entered chan struct{} + proceed chan struct{} + lookups atomic.Int32 +} + +func (g *gatedUserRepo) FindByUsernameWithPassword(username string) (*model.User, error) { + g.lookups.Add(1) + g.entered <- struct{}{} + <-g.proceed + return g.UserRepository.FindByUsernameWithPassword(username) +} + +type countingHandler struct{ calls atomic.Int32 } + +func (c *countingHandler) ServeHTTP(http.ResponseWriter, *http.Request) { c.calls.Add(1) } diff --git a/server/subsonic/stream.go b/server/subsonic/stream.go index 28b4585f0..b4a6b821c 100644 --- a/server/subsonic/stream.go +++ b/server/subsonic/stream.go @@ -3,10 +3,8 @@ package subsonic import ( "context" "errors" - "fmt" "net/http" "strconv" - "strings" "github.com/navidrome/navidrome/conf" "github.com/navidrome/navidrome/core/stream" @@ -15,6 +13,7 @@ import ( "github.com/navidrome/navidrome/model/request" "github.com/navidrome/navidrome/server/subsonic/responses" "github.com/navidrome/navidrome/utils/req" + "github.com/navidrome/navidrome/utils/str" ) func (api *Router) Stream(w http.ResponseWriter, r *http.Request) (*responses.Subsonic, error) { @@ -94,9 +93,7 @@ func (api *Router) Download(w http.ResponseWriter, r *http.Request) (*responses. } setHeaders := func(name string) { - name = strings.ReplaceAll(name, ",", "_") - disposition := fmt.Sprintf("attachment; filename=\"%s.zip\"", name) - w.Header().Set("Content-Disposition", disposition) + w.Header().Set("Content-Disposition", str.ContentDispositionAttachment(name+".zip")) w.Header().Set("Content-Type", "application/zip") } @@ -115,8 +112,7 @@ func (api *Router) Download(w http.ResponseWriter, r *http.Request) (*responses. } }() - disposition := fmt.Sprintf("attachment; filename=\"%s\"", stream.Name()) - w.Header().Set("Content-Disposition", disposition) + w.Header().Set("Content-Disposition", str.ContentDispositionAttachment(stream.Name())) _, err = stream.Serve(ctx, w, r) return nil, err diff --git a/tests/dns_stub.go b/tests/dns_stub.go new file mode 100644 index 000000000..7cd7faf6d --- /dev/null +++ b/tests/dns_stub.go @@ -0,0 +1,54 @@ +package tests + +import ( + "context" + "encoding/binary" + "io" + "net" + "net/netip" + + "golang.org/x/net/dns/dnsmessage" +) + +// StubResolver returns a net.Resolver that answers A queries from records (absolute name to IPv4, +// e.g. "localhost." to "127.0.0.1") instead of the system DNS. Other names do not resolve. +func StubResolver(records map[string]string) *net.Resolver { + return &net.Resolver{ + PreferGo: true, + Dial: func(context.Context, string, string) (net.Conn, error) { + client, server := net.Pipe() + go serveStubDNS(server, records) + return client, nil + }, + } +} + +// net.Pipe is not a PacketConn, so the resolver sends one query framed with a 2-byte length prefix. +func serveStubDNS(conn net.Conn, records map[string]string) { + defer conn.Close() + var size [2]byte + if _, err := io.ReadFull(conn, size[:]); err != nil { + return + } + buf := make([]byte, binary.BigEndian.Uint16(size[:])) + if _, err := io.ReadFull(conn, buf); err != nil { + return + } + var msg dnsmessage.Message + if err := msg.Unpack(buf); err != nil || len(msg.Questions) != 1 { + return + } + msg.Response = true + q := msg.Questions[0] + if ip, err := netip.ParseAddr(records[q.Name.String()]); err == nil && q.Type == dnsmessage.TypeA { + msg.Answers = []dnsmessage.Resource{{ + Header: dnsmessage.ResourceHeader{Name: q.Name, Type: q.Type, Class: q.Class}, + Body: &dnsmessage.AResource{A: ip.As4()}, + }} + } + resp, err := msg.Pack() + if err != nil { + return + } + _, _ = conn.Write(append(binary.BigEndian.AppendUint16(nil, uint16(len(resp))), resp...)) +} diff --git a/tests/fixtures/lastfm.artist.page.challenge.html b/tests/fixtures/lastfm.artist.page.challenge.html new file mode 100644 index 000000000..f431891bd --- /dev/null +++ b/tests/fixtures/lastfm.artist.page.challenge.html @@ -0,0 +1,88 @@ + + + + + + + + Client Challenge + + + + + + + + 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" ) diff --git a/tests/mock_data_store.go b/tests/mock_data_store.go index 32f56a4f0..6a0ebbb31 100644 --- a/tests/mock_data_store.go +++ b/tests/mock_data_store.go @@ -329,6 +329,10 @@ func (db *MockDataStore) WithTxImmediate(block func(tx model.DataStore) error, l return block(db) } +func (db *MockDataStore) WithTxRetry(ctx context.Context, block func(ctx context.Context, tx model.DataStore) error, label ...string) error { + return block(ctx, db) +} + func (db *MockDataStore) Resource(ctx context.Context, m any) model.ResourceRepository { switch m.(type) { case model.MediaFile, *model.MediaFile: diff --git a/tests/mock_mediafile_repo.go b/tests/mock_mediafile_repo.go index a365bd2bd..2093a007d 100644 --- a/tests/mock_mediafile_repo.go +++ b/tests/mock_mediafile_repo.go @@ -37,6 +37,7 @@ type MockMediaFileRepo struct { FindRecentFilesByPropertiesFunc func(missing model.MediaFile, since time.Time) (model.MediaFiles, error) MatchesCriteriaValue bool MatchesCriteriaErr error + BookmarksAdded []string } func (m *MockMediaFileRepo) SetError(err bool) { @@ -76,6 +77,14 @@ func (m *MockMediaFileRepo) Get(id string) (*model.MediaFile, error) { return nil, model.ErrNotFound } +func (m *MockMediaFileRepo) AddBookmark(id, _ string, _ int64) error { + if m.Err { + return errors.New("error") + } + m.BookmarksAdded = append(m.BookmarksAdded, id) + return nil +} + func (m *MockMediaFileRepo) GetWithParticipants(id string) (*model.MediaFile, error) { if m.Err { return nil, errors.New("error") diff --git a/ui/src/authProvider.js b/ui/src/authProvider.js index 18badbc9c..f93114ded 100644 --- a/ui/src/authProvider.js +++ b/ui/src/authProvider.js @@ -64,7 +64,8 @@ const authProvider = { logout: () => { removeItems() - if (config.extAuthLogoutURL) { + // Only proxy-authenticated sessions go to the IdP; others (e.g. direct LAN access) get the login form + if (config.extAuthLogoutURL && config.auth) { window.location.href = config.extAuthLogoutURL return Promise.resolve(false) } diff --git a/ui/src/authProvider.test.js b/ui/src/authProvider.test.js new file mode 100644 index 000000000..b16a39815 --- /dev/null +++ b/ui/src/authProvider.test.js @@ -0,0 +1,47 @@ +import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest' +import config from './config' +import authProvider from './authProvider' + +vi.mock('./config', () => ({ default: {} })) + +describe('authProvider.logout', () => { + const logoutURL = 'https://auth.example.com/signout' + + beforeEach(() => { + vi.stubGlobal('location', { href: '' }) + localStorage.setItem('is-authenticated', 'true') + localStorage.setItem('token', 'abc') + }) + + afterEach(() => { + vi.unstubAllGlobals() + localStorage.clear() + delete config.extAuthLogoutURL + delete config.auth + }) + + it('clears the stored session', async () => { + await authProvider.logout() + expect(localStorage.getItem('is-authenticated')).toBeNull() + expect(localStorage.getItem('token')).toBeNull() + }) + + it('does not redirect when no logout URL is configured', async () => { + config.auth = { id: '1' } + await expect(authProvider.logout()).resolves.toBeUndefined() + expect(window.location.href).toBe('') + }) + + it('redirects to the logout URL when the page was authenticated by the proxy', async () => { + config.extAuthLogoutURL = logoutURL + config.auth = { id: '1' } + await expect(authProvider.logout()).resolves.toBe(false) + expect(window.location.href).toBe(logoutURL) + }) + + it('does not redirect when the page was not authenticated by the proxy', async () => { + config.extAuthLogoutURL = logoutURL + await expect(authProvider.logout()).resolves.toBeUndefined() + expect(window.location.href).toBe('') + }) +}) diff --git a/ui/src/setupTests.js b/ui/src/setupTests.js index ddb999f3c..7cb46e09c 100644 --- a/ui/src/setupTests.js +++ b/ui/src/setupTests.js @@ -14,6 +14,9 @@ const localStorageMock = (function () { setItem: function (key, value) { store[key] = value.toString() }, + removeItem: function (key) { + delete store[key] + }, clear: function () { store = {} }, diff --git a/utils/httpclient/httpclient.go b/utils/httpclient/httpclient.go index 7fb48f36d..b0e7b5681 100644 --- a/utils/httpclient/httpclient.go +++ b/utils/httpclient/httpclient.go @@ -3,10 +3,15 @@ package httpclient import ( + "context" + "net" "net/http" + "net/netip" + "net/url" "time" "github.com/navidrome/navidrome/consts" + "github.com/navidrome/navidrome/utils/netguard" ) type uaTransport struct { @@ -33,3 +38,47 @@ func NewTransport(base http.RoundTripper) http.RoundTripper { func New(timeout time.Duration) *http.Client { return &http.Client{Timeout: timeout, Transport: NewTransport(nil)} } + +// proxyFunc resolves the proxy for a request; tests replace it. +var proxyFunc = http.ProxyFromEnvironment + +type proxyAddrKey struct{} + +// NewExternal is New for URLs from untrusted sources: it refuses to dial private, loopback, +// link-local and unspecified addresses, except those covered by allowed. +func NewExternal(timeout time.Duration, allowed ...netip.Prefix) *http.Client { + t := http.DefaultTransport.(*http.Transport).Clone() + t.Proxy = proxyFunc + direct := net.Dialer{Timeout: 30 * time.Second, KeepAlive: 30 * time.Second} + guarded := direct + guarded.Control = netguard.DialControl(allowed...) + t.DialContext = func(ctx context.Context, network, addr string) (net.Conn, error) { + // Exempt the hop to the proxy, which relays the request and is operator config. A URL + // aimed at the proxy's own address is not proxied, so it stays guarded. + if proxy, ok := ctx.Value(proxyAddrKey{}).(string); ok && proxy == addr { + return direct.DialContext(ctx, network, addr) + } + return guarded.DialContext(ctx, network, addr) + } + return &http.Client{Timeout: timeout, Transport: NewTransport(&proxyTagger{base: t})} +} + +// proxyTagger records the proxy each request resolves to, so the dialer can tell a hop to the +// proxy from a dial to the URL's own host. +type proxyTagger struct{ base http.RoundTripper } + +func (p *proxyTagger) RoundTrip(req *http.Request) (*http.Response, error) { + if u, err := proxyFunc(req); err == nil && u != nil { + req = req.WithContext(context.WithValue(req.Context(), proxyAddrKey{}, proxyAddr(u))) + } + return p.base.RoundTrip(req) +} + +// proxyAddr mirrors how net/http addresses a proxy connection. +func proxyAddr(u *url.URL) string { + port := u.Port() + if port == "" { + port = map[string]string{"http": "80", "https": "443", "socks5": "1080", "socks5h": "1080"}[u.Scheme] + } + return net.JoinHostPort(u.Hostname(), port) +} diff --git a/utils/httpclient/httpclient_proxy_internal_test.go b/utils/httpclient/httpclient_proxy_internal_test.go new file mode 100644 index 000000000..2d542d8fe --- /dev/null +++ b/utils/httpclient/httpclient_proxy_internal_test.go @@ -0,0 +1,68 @@ +package httpclient + +import ( + "net/http" + "net/http/httptest" + "net/url" + "sync/atomic" + "time" + + "github.com/navidrome/navidrome/utils/netguard" + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" +) + +var _ = Describe("NewExternal with a proxy", func() { + var proxy *httptest.Server + var proxied atomic.Int32 + var proxyURL *url.URL + + BeforeEach(func() { + proxied.Store(0) + proxy = httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + proxied.Add(1) + _, _ = w.Write([]byte("via proxy")) + })) + DeferCleanup(proxy.Close) + proxyURL, _ = url.Parse(proxy.URL) + prev := proxyFunc + DeferCleanup(func() { proxyFunc = prev }) + }) + + It("dials the configured proxy even though it listens on a private address", func() { + proxyFunc = func(*http.Request) (*url.URL, error) { return proxyURL, nil } + + resp, err := NewExternal(time.Second).Get("http://navidrome.example.com/cover.jpg") + Expect(err).ToNot(HaveOccurred()) + defer resp.Body.Close() + Expect(proxied.Load()).To(Equal(int32(1))) + }) + + // net/http never proxies loopback targets, so a URL aimed at a loopback proxy is dialed directly. + It("refuses a direct dial to the proxy's own address when the request is not proxied", func() { + proxyFunc = func(r *http.Request) (*url.URL, error) { + if r.URL.Hostname() == "127.0.0.1" { + return nil, nil + } + return proxyURL, nil + } + + _, err := NewExternal(time.Second).Get(proxy.URL + "/secret") + Expect(err).To(MatchError(netguard.ErrPrivateAddress)) + Expect(proxied.Load()).To(BeZero()) + }) + + It("still refuses a direct dial to a private address", func() { + proxyFunc = func(r *http.Request) (*url.URL, error) { + if r.URL.Scheme == "https" { + return proxyURL, nil + } + return nil, nil + } + target := httptest.NewServer(http.HandlerFunc(func(http.ResponseWriter, *http.Request) {})) + DeferCleanup(target.Close) + + _, err := NewExternal(time.Second).Get(target.URL) + Expect(err).To(MatchError(netguard.ErrPrivateAddress)) + }) +}) diff --git a/utils/httpclient/httpclient_test.go b/utils/httpclient/httpclient_test.go index c86b51165..7b1e58797 100644 --- a/utils/httpclient/httpclient_test.go +++ b/utils/httpclient/httpclient_test.go @@ -3,10 +3,12 @@ package httpclient_test import ( "net/http" "net/http/httptest" + "net/netip" "time" "github.com/navidrome/navidrome/consts" "github.com/navidrome/navidrome/utils/httpclient" + "github.com/navidrome/navidrome/utils/netguard" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" ) @@ -16,6 +18,7 @@ var _ = Describe("httpclient", func() { var receivedUA string BeforeEach(func() { + receivedUA = "" server = httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { receivedUA = r.Header.Get("User-Agent") })) @@ -48,6 +51,34 @@ var _ = Describe("httpclient", func() { }) }) + Describe("NewExternal", func() { + It("refuses to connect to a loopback server", func() { + c := httpclient.NewExternal(time.Second) + _, err := c.Get(server.URL) + Expect(err).To(MatchError(netguard.ErrPrivateAddress)) + Expect(receivedUA).To(BeEmpty()) + }) + + It("refuses a redirect from an allowed host to a private address", func() { + redirector := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + http.Redirect(w, r, "http://169.254.169.254/latest/meta-data/", http.StatusFound) + })) + DeferCleanup(redirector.Close) + + c := httpclient.NewExternal(time.Second, netip.MustParsePrefix("127.0.0.1/32")) + _, err := c.Get(redirector.URL) + Expect(err).To(MatchError(netguard.ErrPrivateAddress)) + }) + + It("connects to addresses covered by an allowed prefix and sets the User-Agent", func() { + c := httpclient.NewExternal(time.Second, netip.MustParsePrefix("127.0.0.0/8")) + resp, err := c.Get(server.URL) + Expect(err).ToNot(HaveOccurred()) + resp.Body.Close() + Expect(receivedUA).To(Equal(consts.HTTPUserAgent)) + }) + }) + Describe("NewTransport", func() { It("uses the default transport when base is nil", func() { c := &http.Client{Transport: httpclient.NewTransport(nil)} diff --git a/utils/netguard/netguard.go b/utils/netguard/netguard.go new file mode 100644 index 000000000..43d80ecc9 --- /dev/null +++ b/utils/netguard/netguard.go @@ -0,0 +1,38 @@ +// Package netguard keeps outbound connections driven by untrusted input away from internal addresses. +package netguard + +import ( + "errors" + "fmt" + "net" + "net/netip" + "syscall" +) + +var ErrPrivateAddress = errors.New("dial to private/loopback address blocked") + +// IsPrivateIP reports whether ip is loopback, private, link-local or unspecified. +func IsPrivateIP(ip net.IP) bool { + return ip.IsLoopback() || ip.IsUnspecified() || ip.IsPrivate() || ip.IsLinkLocalUnicast() || ip.IsLinkLocalMulticast() +} + +// DialControl returns a net.Dialer Control hook that rejects private addresses not covered by allowed. +// It sees the resolved IP, so DNS names, redirects and rebinding cannot get around it. +func DialControl(allowed ...netip.Prefix) func(network, address string, c syscall.RawConn) error { + return func(_, address string, _ syscall.RawConn) error { + ap, err := netip.ParseAddrPort(address) + if err != nil { + return fmt.Errorf("%w: unparseable address %q: %w", ErrPrivateAddress, address, err) + } + addr := ap.Addr().Unmap() + if !IsPrivateIP(addr.AsSlice()) { + return nil + } + for _, p := range allowed { + if p.Contains(addr) { + return nil + } + } + return fmt.Errorf("%w: %s", ErrPrivateAddress, address) + } +} diff --git a/utils/netguard/netguard_suite_test.go b/utils/netguard/netguard_suite_test.go new file mode 100644 index 000000000..7769e0c82 --- /dev/null +++ b/utils/netguard/netguard_suite_test.go @@ -0,0 +1,17 @@ +package netguard_test + +import ( + "testing" + + "github.com/navidrome/navidrome/log" + "github.com/navidrome/navidrome/tests" + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" +) + +func TestNetguard(t *testing.T) { + tests.Init(t, false) + log.SetLevel(log.LevelFatal) + RegisterFailHandler(Fail) + RunSpecs(t, "Netguard Suite") +} diff --git a/utils/netguard/netguard_test.go b/utils/netguard/netguard_test.go new file mode 100644 index 000000000..733204226 --- /dev/null +++ b/utils/netguard/netguard_test.go @@ -0,0 +1,65 @@ +package netguard_test + +import ( + "net" + "net/netip" + + "github.com/navidrome/navidrome/utils/netguard" + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" +) + +var _ = Describe("netguard", func() { + DescribeTable("IsPrivateIP", + func(addr string, expected bool) { + Expect(netguard.IsPrivateIP(net.ParseIP(addr))).To(Equal(expected)) + }, + Entry("IPv4 loopback", "127.0.0.1", true), + Entry("IPv4 loopback range", "127.0.0.2", true), + Entry("IPv6 loopback", "::1", true), + Entry("IPv4-mapped IPv6 loopback", "::ffff:127.0.0.1", true), + Entry("10.x", "10.1.2.3", true), + Entry("172.16.x", "172.16.0.1", true), + Entry("192.168.x", "192.168.1.10", true), + Entry("IPv6 unique local", "fd00::1", true), + Entry("link-local (cloud metadata)", "169.254.169.254", true), + Entry("IPv6 link-local", "fe80::1", true), + Entry("link-local multicast", "224.0.0.1", true), + Entry("IPv4 unspecified", "0.0.0.0", true), + Entry("IPv6 unspecified", "::", true), + Entry("public IPv4", "93.184.216.34", false), + Entry("172.32.x is outside the private block", "172.32.0.1", false), + Entry("public IPv6", "2606:4700:4700::1111", false), + ) + + Describe("DialControl", func() { + It("rejects private and loopback addresses", func() { + control := netguard.DialControl() + for _, addr := range []string{"127.0.0.1:80", "[::1]:443", "169.254.169.254:80", "10.0.0.1:8080", "0.0.0.0:80"} { + Expect(control("tcp", addr, nil)).To(MatchError(netguard.ErrPrivateAddress), addr) + } + }) + + It("allows public addresses", func() { + control := netguard.DialControl() + Expect(control("tcp", "93.184.216.34:443", nil)).To(Succeed()) + Expect(control("tcp6", "[2606:4700:4700::1111]:443", nil)).To(Succeed()) + }) + + It("allows private addresses covered by an allowed prefix, and only those", func() { + control := netguard.DialControl(netip.MustParsePrefix("127.0.0.1/32")) + Expect(control("tcp", "127.0.0.1:8080", nil)).To(Succeed()) + Expect(control("tcp", "127.0.0.2:8080", nil)).To(MatchError(netguard.ErrPrivateAddress)) + }) + + It("matches IPv4-mapped IPv6 addresses against IPv4 prefixes", func() { + control := netguard.DialControl(netip.MustParsePrefix("127.0.0.0/8")) + Expect(control("tcp", "[::ffff:127.0.0.1]:80", nil)).To(Succeed()) + }) + + It("fails closed when the address is not an IP", func() { + Expect(netguard.DialControl()("tcp", "localhost:80", nil)).To(HaveOccurred()) + Expect(netguard.DialControl()("tcp", "garbage", nil)).To(HaveOccurred()) + }) + }) +}) diff --git a/utils/str/content_disposition.go b/utils/str/content_disposition.go new file mode 100644 index 000000000..34ae8a5c6 --- /dev/null +++ b/utils/str/content_disposition.go @@ -0,0 +1,80 @@ +package str + +import ( + "cmp" + "fmt" + "mime" + "path" + "strings" + "unicode" + "unicode/utf8" + + "github.com/deluan/sanitize" +) + +const ( + maxFilenameBytes = 255 + maxExtensionBytes = 16 + fallbackFilename = "download" +) + +// ContentDispositionAttachment builds an RFC 6266 attachment header value for a user-controlled filename. +// Non-ASCII names also get a filename* parameter, which clients prefer over the ASCII fallback. +func ContentDispositionAttachment(filename string) string { + stem, ext := splitFilename(filename) + header := fmt.Sprintf("attachment; filename=%q", joinFilename(toASCII(stem), toASCII(ext))) + name := joinFilename(stem, ext) + if isASCII(name) { + return header + } + // FormatMediaType emits a percent-encoded filename* for non-ASCII values + extended := mime.FormatMediaType("attachment", map[string]string{"filename": name}) + return header + strings.TrimPrefix(extended, "attachment") +} + +func splitFilename(filename string) (stem, ext string) { + name := strings.Map(func(r rune) rune { + if unicode.IsControl(r) || unicode.Is(unicode.Bidi_Control, r) { + return -1 + } + return r + }, SanitizeFilename(strings.ToValidUTF8(filename, "_"))) + ext = path.Ext(name) + if len(ext) > maxExtensionBytes { + ext = "" + } + stem = strings.TrimSuffix(name, ext) + if limit := maxFilenameBytes - len(ext); len(stem) > limit { + for limit > 0 && !utf8.RuneStart(stem[limit]) { + limit-- + } + stem = stem[:limit] + } + return stem, ext +} + +func joinFilename(stem, ext string) string { + stem = cmp.Or(strings.TrimSpace(stem), fallbackFilename) + return cmp.Or(strings.TrimRight(stem+ext, " ."), fallbackFilename) +} + +func toASCII(s string) string { + return strings.Map(func(r rune) rune { + switch { + case r == ',': + return '_' + case r >= ' ' && r <= '~': + return r + } + return -1 + }, sanitize.Accents(SanitizeFilename(Clear(s)))) +} + +func isASCII(s string) bool { + for i := 0; i < len(s); i++ { + if s[i] >= utf8.RuneSelf { + return false + } + } + return true +} diff --git a/utils/str/content_disposition_test.go b/utils/str/content_disposition_test.go new file mode 100644 index 000000000..944392a4e --- /dev/null +++ b/utils/str/content_disposition_test.go @@ -0,0 +1,120 @@ +package str_test + +import ( + "mime" + "regexp" + "strings" + + "github.com/navidrome/navidrome/utils/str" + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" +) + +var asciiFilenameRe = regexp.MustCompile(`(?:^|; )filename="([^"]*)"`) + +// asciiName returns the quoted filename parameter, the one used by clients that ignore filename*. +func asciiName(filename string) string { + m := asciiFilenameRe.FindAllStringSubmatch(str.ContentDispositionAttachment(filename), -1) + ExpectWithOffset(1, m).To(HaveLen(1), "expected exactly one quoted filename parameter") + return m[0][1] +} + +// decodedName returns the name an RFC 6266 client picks, preferring filename* when present. +func decodedName(filename string) string { + disp, params, err := mime.ParseMediaType(str.ContentDispositionAttachment(filename)) + ExpectWithOffset(1, err).ToNot(HaveOccurred()) + ExpectWithOffset(1, disp).To(Equal("attachment")) + return params["filename"] +} + +var _ = Describe("ContentDispositionAttachment", func() { + Describe("parameter injection", func() { + const attack = `party"; filename="evil.html.m3u` + + It("does not let a quote in the name open a second parameter", func() { + Expect(str.ContentDispositionAttachment(attack)).To(Equal(`attachment; filename="party_; filename=_evil.html.m3u"`)) + }) + + It("keeps the header parseable", func() { + Expect(decodedName(attack)).To(Equal("party_; filename=_evil.html.m3u")) + }) + + It("does not let a quote in a non-ASCII name inject either", func() { + const utf8Attack = `東京"; filename="evil.html.m3u` + header := str.ContentDispositionAttachment(utf8Attack) + Expect(strings.Count(header, `"`)).To(Equal(2)) + Expect(decodedName(utf8Attack)).To(Equal("東京_; filename=_evil.html.m3u")) + }) + }) + + Describe("ASCII names", func() { + It("sends only the quoted filename, unchanged", func() { + Expect(str.ContentDispositionAttachment("My Playlist.m3u")).To(Equal(`attachment; filename="My Playlist.m3u"`)) + }) + + It("keeps leading dots", func() { + Expect(asciiName("...And Justice for All.zip")).To(Equal("...And Justice for All.zip")) + }) + + It("trims surrounding spaces and trailing dots", func() { + Expect(asciiName(" Greatest Hits .zip")).To(Equal("Greatest Hits.zip")) + Expect(asciiName("Loose End. ")).To(Equal("Loose End")) + }) + + It("falls back to a placeholder when only dots are left", func() { + Expect(str.ContentDispositionAttachment("...")).To(Equal(`attachment; filename="download"`)) + }) + + It("caps a long name whose last dot is not an extension", func() { + name := asciiName("a." + strings.Repeat("x", 300)) + Expect(len(name)).To(BeNumerically("<=", 255)) + Expect(name).To(HavePrefix("a.xxx")) + }) + + It("replaces path separators and reserved characters", func() { + Expect(asciiName("AC/DC: Live, 1979?.zip")).To(Equal("AC_DC_ Live_ 1979_.zip")) + }) + + It("drops control characters", func() { + Expect(asciiName("line\r\nbreak\x00\t.mp3")).To(Equal("linebreak.mp3")) + }) + }) + + Describe("non-ASCII names", func() { + It("transliterates accents in the ASCII fallback", func() { + Expect(asciiName("Legião Urbana.zip")).To(Equal("Legiao Urbana.zip")) + }) + + It("converts typographic punctuation in the ASCII fallback", func() { + Expect(asciiName("She’s a Woman — Live.mp3")).To(Equal("She's a Woman - Live.mp3")) + Expect(asciiName("“Heroes”.mp3")).To(Equal("_Heroes_.mp3")) + Expect(decodedName("She’s a Woman.mp3")).To(Equal("She’s a Woman.mp3")) + }) + + It("keeps the extension when no ASCII letters survive", func() { + Expect(asciiName("東京.mp3")).To(Equal("download.mp3")) + Expect(asciiName("Кино.m3u")).To(Equal("download.m3u")) + Expect(asciiName("東京")).To(Equal("download")) + }) + + DescribeTable("filename* carries the sanitized UTF-8 name", + func(filename, expected string) { + Expect(decodedName(filename)).To(Equal(expected)) + }, + Entry("accented", "Legião Urbana.zip", "Legião Urbana.zip"), + Entry("CJK", "東京.zip", "東京.zip"), + Entry("emoji", "🎵 mix.zip", "🎵 mix.zip"), + Entry("comma and percent", "Sigur Rós, 100%.zip", "Sigur Rós, 100%.zip"), + Entry("path separators", "Sigur Rós/Live: Heima.zip", "Sigur Rós_Live_ Heima.zip"), + Entry("control characters", "Sigur Rós\r\n\x00.zip", "Sigur Rós.zip"), + Entry("bidi override", "Björk\u202Eexe.mp3", "Björkexe.mp3"), + Entry("invalid UTF-8", "Bj\xf6rk Café.mp3", "Bj_rk Café.mp3"), + ) + + It("caps the name length and keeps the extension", func() { + name := decodedName(strings.Repeat("東", 300) + ".zip") + Expect(len(name)).To(BeNumerically("<=", 255)) + Expect(name).To(HaveSuffix("東.zip")) + }) + }) +})