diff --git a/adapters/lastfm/auth_router.go b/adapters/lastfm/auth_router.go index 411bf069a..ec1bb9290 100644 --- a/adapters/lastfm/auth_router.go +++ b/adapters/lastfm/auth_router.go @@ -132,7 +132,7 @@ func (s *Router) callback(w http.ResponseWriter, r *http.Request) { func (s *Router) fetchSessionKey(ctx context.Context, uid, token string) error { sessionKey, err := s.client.getSession(ctx, token) if err != nil { - log.Error(ctx, "Could not fetch LastFM session key", "userId", uid, "token", token, + log.Error(ctx, "Could not fetch LastFM session key", "userId", uid, "requestId", middleware.GetReqID(ctx), err) return err } diff --git a/cmd/wire_gen.go b/cmd/wire_gen.go index de4c55a1e..fd04c44c5 100644 --- a/cmd/wire_gen.go +++ b/cmd/wire_gen.go @@ -70,7 +70,7 @@ func CreateNativeAPIRouter(ctx context.Context) *nativeapi.Router { insights := metrics.GetInstance(dataStore) broker := events.GetBroker() metricsMetrics := metrics.GetPrometheusInstance(dataStore) - modelScanner := scanner.New(ctx, dataStore, broker, playlistsPlaylists, metricsMetrics) + modelScanner := scanner.GetInstance(ctx, dataStore, broker, playlistsPlaylists, metricsMetrics) watcher := scanner.GetWatcher(dataStore, modelScanner) manager := plugins.GetManager(dataStore, broker, metricsMetrics) library := core.NewLibrary(dataStore, modelScanner, watcher, broker, manager) @@ -103,7 +103,7 @@ func CreateSubsonicAPIRouter(ctx context.Context) *subsonic.Router { provider := external.NewProvider(dataStore, agentsAgents, matcherMatcher, broker) uploader := artwork.NewUploader(dataStore) playlistsPlaylists := playlists.NewPlaylists(dataStore, uploader) - modelScanner := scanner.New(ctx, dataStore, broker, playlistsPlaylists, metricsMetrics) + modelScanner := scanner.GetInstance(ctx, dataStore, broker, playlistsPlaylists, metricsMetrics) playTracker := scrobbler.GetPlayTracker(dataStore, broker, manager) playbackServer := playback.GetInstance(dataStore) lyricsLyrics := lyrics.NewLyrics(dataStore, manager) @@ -189,7 +189,7 @@ func CreateScanner(ctx context.Context) model.Scanner { uploader := artwork.NewUploader(dataStore) playlistsPlaylists := playlists.NewPlaylists(dataStore, uploader) metricsMetrics := metrics.GetPrometheusInstance(dataStore) - modelScanner := scanner.New(ctx, dataStore, broker, playlistsPlaylists, metricsMetrics) + modelScanner := scanner.GetInstance(ctx, dataStore, broker, playlistsPlaylists, metricsMetrics) return modelScanner } @@ -200,7 +200,7 @@ func CreateScanWatcher(ctx context.Context) scanner.Watcher { uploader := artwork.NewUploader(dataStore) playlistsPlaylists := playlists.NewPlaylists(dataStore, uploader) metricsMetrics := metrics.GetPrometheusInstance(dataStore) - modelScanner := scanner.New(ctx, dataStore, broker, playlistsPlaylists, metricsMetrics) + modelScanner := scanner.GetInstance(ctx, dataStore, broker, playlistsPlaylists, metricsMetrics) watcher := scanner.GetWatcher(dataStore, modelScanner) return watcher } @@ -249,7 +249,7 @@ func getPluginManager() *plugins.Manager { // wire_injectors.go: -var allProviders = wire.NewSet(core.Set, artwork.Set, server.New, subsonic.New, jellyfin.New, nativeapi.New, public.New, persistence.New, lastfm.NewRouter, listenbrainz.NewRouter, events.GetBroker, scanner.New, scanner.GetWatcher, metrics.GetPrometheusInstance, db.Db, plugins.GetManager, sonic.New, wire.Bind(new(agents.PluginLoader), new(*plugins.Manager)), wire.Bind(new(scrobbler.PluginLoader), new(*plugins.Manager)), wire.Bind(new(lyrics.PluginLoader), new(*plugins.Manager)), wire.Bind(new(sonic.PluginLoader), new(*plugins.Manager)), wire.Bind(new(sonic.Engine), new(*sonic.Sonic)), wire.Bind(new(nativeapi.PluginManager), new(*plugins.Manager)), wire.Bind(new(core.PluginUnloader), new(*plugins.Manager)), wire.Bind(new(plugins.PluginMetricsRecorder), new(metrics.Metrics)), wire.Bind(new(core.Watcher), new(scanner.Watcher)), wire.Bind(new(playlists.ImageUploadService), new(artwork.Uploader))) +var allProviders = wire.NewSet(core.Set, artwork.Set, server.New, subsonic.New, jellyfin.New, nativeapi.New, public.New, persistence.New, lastfm.NewRouter, listenbrainz.NewRouter, events.GetBroker, scanner.GetInstance, scanner.GetWatcher, metrics.GetPrometheusInstance, db.Db, plugins.GetManager, sonic.New, wire.Bind(new(agents.PluginLoader), new(*plugins.Manager)), wire.Bind(new(scrobbler.PluginLoader), new(*plugins.Manager)), wire.Bind(new(lyrics.PluginLoader), new(*plugins.Manager)), wire.Bind(new(sonic.PluginLoader), new(*plugins.Manager)), wire.Bind(new(sonic.Engine), new(*sonic.Sonic)), wire.Bind(new(nativeapi.PluginManager), new(*plugins.Manager)), wire.Bind(new(core.PluginUnloader), new(*plugins.Manager)), wire.Bind(new(plugins.PluginMetricsRecorder), new(metrics.Metrics)), wire.Bind(new(core.Watcher), new(scanner.Watcher)), wire.Bind(new(playlists.ImageUploadService), new(artwork.Uploader))) func GetPluginManager(ctx context.Context) *plugins.Manager { manager := getPluginManager() diff --git a/cmd/wire_injectors.go b/cmd/wire_injectors.go index 8bc404fb1..a54d4ae1b 100644 --- a/cmd/wire_injectors.go +++ b/cmd/wire_injectors.go @@ -42,7 +42,7 @@ var allProviders = wire.NewSet( lastfm.NewRouter, listenbrainz.NewRouter, events.GetBroker, - scanner.New, + scanner.GetInstance, scanner.GetWatcher, metrics.GetPrometheusInstance, db.Db, diff --git a/conf/configuration.go b/conf/configuration.go index ff119417a..e549e2a61 100644 --- a/conf/configuration.go +++ b/conf/configuration.go @@ -407,7 +407,7 @@ func Load(noConfigDump bool) { if mkErr := os.MkdirAll(filepath.Dir(Server.LogFile), os.ModePerm); mkErr != nil { logFatal(fmt.Sprintf("Error creating log file directory: %s", mkErr.Error())) } - out, err = os.OpenFile(Server.LogFile, os.O_APPEND|os.O_CREATE|os.O_WRONLY, 0644) + out, err = os.OpenFile(Server.LogFile, os.O_APPEND|os.O_CREATE|os.O_WRONLY, 0600) if err != nil { logFatal(fmt.Sprintf("Error opening log file %s: %s", Server.LogFile, err.Error())) } diff --git a/conf/configuration_test.go b/conf/configuration_test.go index 2c7f8edaa..9404f01bc 100644 --- a/conf/configuration_test.go +++ b/conf/configuration_test.go @@ -5,6 +5,7 @@ import ( "fmt" "os" "path/filepath" + "runtime" "testing" "time" @@ -329,6 +330,21 @@ var _ = Describe("Configuration", func() { }).To(PanicWith(ContainSubstring("Error creating log file directory"))) }) + It("creates the log file readable only by the owner", func() { + if runtime.GOOS == "windows" { + Skip("file modes are not enforced on Windows") + } + logFile := filepath.Join(GinkgoT().TempDir(), "navidrome.log") + viper.SetDefault("datafolder", GinkgoT().TempDir()) + viper.SetDefault("logfile", logFile) + DeferCleanup(log.SetOutput, os.Stderr) + conf.Load(true) + + info, err := os.Stat(logFile) + Expect(err).ToNot(HaveOccurred()) + Expect(info.Mode().Perm()).To(Equal(os.FileMode(0600))) + }) + It("is called when BaseURL is invalid", func() { viper.SetDefault("datafolder", GinkgoT().TempDir()) viper.SetDefault("baseurl", "://invalid") diff --git a/core/artwork/resize.go b/core/artwork/resize.go index 9536c3bb1..9b3ff46e5 100644 --- a/core/artwork/resize.go +++ b/core/artwork/resize.go @@ -76,7 +76,7 @@ func toFastScaleType(img image.Image) image.Image { } func resizeStaticImage(data []byte, size int, square bool) (io.Reader, int, error) { - original, format, err := image.Decode(bytes.NewReader(data)) + original, format, err := decodeCapped(data) if err != nil { return nil, 0, err } diff --git a/core/artwork/resize_test.go b/core/artwork/resize_test.go new file mode 100644 index 000000000..76b95974d --- /dev/null +++ b/core/artwork/resize_test.go @@ -0,0 +1,13 @@ +package artwork + +import ( + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" +) + +var _ = Describe("resizeStaticImage", func() { + It("rejects images whose declared dimensions exceed the pixel cap before decoding", func() { + _, _, err := resizeStaticImage(pngHeaderWithDims(9000, 9000), 300, false) + Expect(err).To(MatchError(ContainSubstring("exceed pixel cap"))) + }) +}) diff --git a/core/share.go b/core/share.go index b2f32ba39..5ee1a5667 100644 --- a/core/share.go +++ b/core/share.go @@ -2,6 +2,8 @@ package core import ( "context" + "fmt" + "slices" "strings" "time" @@ -98,27 +100,19 @@ func (r *shareRepositoryWrapper) Save(entity any) (string, error) { s.ExpiresAt = new(time.Now().Add(conf.Server.DefaultShareExpiration)) } - firstId, _, _ := strings.Cut(s.ResourceIDs, ",") - v, err := model.GetEntityByID(r.ctx, r.ds, firstId) + s.ResourceType, err = r.resourceType(s.ResourceIDs) if err != nil { return "", err } - switch v.(type) { - case *model.Artist: - s.ResourceType = "artist" + switch s.ResourceType { + case "artist": s.Contents = r.contentsLabelFromArtist(s.ID, s.ResourceIDs) - case *model.Album: - s.ResourceType = "album" + case "album": s.Contents = r.contentsLabelFromAlbums(s.ID, s.ResourceIDs) - case *model.Playlist: - s.ResourceType = "playlist" + case "playlist": s.Contents = r.contentsLabelFromPlaylist(s.ID, s.ResourceIDs) - case *model.MediaFile: - s.ResourceType = "media_file" + case "media_file": s.Contents = r.contentsLabelFromMediaFiles(s.ID, s.ResourceIDs) - default: - log.Error(r.ctx, "Invalid Resource ID", "id", firstId) - return "", model.ErrNotFound } s.Contents = str.TruncateRunes(s.Contents, 30, "...") @@ -126,6 +120,29 @@ func (r *shareRepositoryWrapper) Save(entity any) (string, error) { return r.Persistable.Save(s) } +var shareableKinds = []model.Kind{model.KindArtistArtwork, model.KindAlbumArtwork, model.KindPlaylistArtwork, model.KindMediaFileArtwork} + +// resourceType resolves every ID as the current user, so an entity they cannot see cannot +// ride along behind a valid first one, and requires all IDs to be of the same kind. +func (r *shareRepositoryWrapper) resourceType(resourceIDs string) (string, error) { + resourceType := "" + for _, id := range strings.Split(resourceIDs, ",") { + kind, err := model.GetEntityKindByID(r.ctx, r.ds, id) + if err != nil { + return "", err + } + if !slices.Contains(shareableKinds, kind) { + log.Error(r.ctx, "Invalid Resource ID", "id", id) + return "", model.ErrNotFound + } + if resourceType != "" && kind.String() != resourceType { + return "", fmt.Errorf("%w: share mixes %s and %s resources", model.ErrValidation, resourceType, kind) + } + resourceType = kind.String() + } + return resourceType, nil +} + func (r *shareRepositoryWrapper) Update(id string, entity any, _ ...string) error { cols := []string{"description", "downloadable"} diff --git a/core/share_test.go b/core/share_test.go index 475d40ec9..a5ee0428b 100644 --- a/core/share_test.go +++ b/core/share_test.go @@ -70,6 +70,19 @@ var _ = Describe("Share", func() { Expect(err).ToNot(HaveOccurred()) Expect(entity.Contents).To(Equal("私の中の幻想的世界観及びその顕現を想起させたある現実で...")) }) + + It("fails when any of the resource IDs does not exist", func() { + entity := &model.Share{Description: "test", ResourceIDs: "123,missing"} + _, err := repo.Save(entity) + Expect(err).To(MatchError(model.ErrNotFound)) + }) + + It("fails when the resource IDs are of mixed types", func() { + _ = ds.MediaFile(ctx).Put(&model.MediaFile{ID: "456", Title: "Example Media File"}) + entity := &model.Share{Description: "test", ResourceIDs: "123,456"} + _, err := repo.Save(entity) + Expect(err).To(HaveOccurred()) + }) }) Describe("Update", func() { diff --git a/core/storage/local/watcher.go b/core/storage/local/watcher.go index 1b8a4e0c8..ef404d497 100644 --- a/core/storage/local/watcher.go +++ b/core/storage/local/watcher.go @@ -3,6 +3,7 @@ package local import ( "context" "errors" + "fmt" "path/filepath" "strings" @@ -18,22 +19,18 @@ func (s *localStorage) Start(ctx context.Context) (<-chan string, error) { return nil, errors.New("watcher already started") } input := make(chan notify.EventInfo, 500) - output := make(chan string, 500) + libPath := filepath.Join(s.u.Path, "...") + log.Debug(ctx, "Starting watcher", "lib", libPath) + if err := notify.Watch(libPath, input, WatchEvents); err != nil { + s.watching.Store(false) + return nil, fmt.Errorf("starting watcher on %s: %w", libPath, err) + } - started := make(chan struct{}) + output := make(chan string, 500) go func() { defer close(input) defer close(output) - - libPath := filepath.Join(s.u.Path, "...") - log.Debug(ctx, "Starting watcher", "lib", libPath) - err := notify.Watch(libPath, input, WatchEvents) - if err != nil { - log.Error("Error starting watcher", "lib", libPath, err) - return - } defer notify.Stop(input) - close(started) // signals the main goroutine we have started for { select { @@ -49,9 +46,5 @@ func (s *localStorage) Start(ctx context.Context) (<-chan string, error) { } } }() - select { - case <-started: - case <-ctx.Done(): - } return output, nil } diff --git a/core/storage/local/watcher_test.go b/core/storage/local/watcher_test.go index 8d2d31367..37387c76a 100644 --- a/core/storage/local/watcher_test.go +++ b/core/storage/local/watcher_test.go @@ -137,3 +137,18 @@ type noopExtractor struct{} func (s noopExtractor) Parse(files ...string) (map[string]metadata.Info, error) { return nil, nil } func (s noopExtractor) Version() string { return "0" } + +var _ = Describe("Watcher.Start", func() { + It("returns an error instead of hanging when the path cannot be watched", func() { + local.RegisterExtractor("noop", func(fs fs.FS, path string) local.Extractor { return noopExtractor{} }) + conf.Server.Scanner.Extractor = "noop" + ls, err := storage.For(filepath.Join(GinkgoT().TempDir(), "does-not-exist")) + Expect(err).ToNot(HaveOccurred()) + lsw := ls.(storage.Watcher) + + ctx, cancel := context.WithTimeout(context.Background(), 3*time.Second) + defer cancel() + _, err = lsw.Start(ctx) + Expect(err).To(HaveOccurred()) + }) +}) diff --git a/db/migrations/20240511220020_add_library_table.go b/db/migrations/20240511220020_add_library_table.go index 55b521ca9..d7b4aa5c2 100644 --- a/db/migrations/20240511220020_add_library_table.go +++ b/db/migrations/20240511220020_add_library_table.go @@ -3,7 +3,6 @@ package migrations import ( "context" "database/sql" - "fmt" "github.com/navidrome/navidrome/conf" "github.com/pressly/goose/v3" @@ -28,10 +27,10 @@ func upAddLibraryTable(ctx context.Context, tx *sql.Tx) error { return err } - _, err = tx.ExecContext(ctx, fmt.Sprintf(` - insert into library(id, name, path) values(1, 'Music Library', '%s'); - delete from property where id like 'LastScan-%%'; -`, conf.Server.MusicFolder)) + _, err = tx.ExecContext(ctx, ` + insert into library(id, name, path) values(1, 'Music Library', ?); + delete from property where id like 'LastScan-%'; +`, conf.Server.MusicFolder) if err != nil { return err } diff --git a/model/share.go b/model/share.go index ce0846d60..cf1f4cb34 100644 --- a/model/share.go +++ b/model/share.go @@ -32,9 +32,6 @@ type Share struct { func (s Share) CoverArtID() ArtworkID { ids := strings.SplitN(s.ResourceIDs, ",", 2) - if len(ids) == 0 { - return ArtworkID{} - } switch s.ResourceType { case "album": return Album{ID: ids[0]}.CoverArtID() @@ -43,6 +40,10 @@ func (s Share) CoverArtID() ArtworkID { case "artist": return Artist{ID: ids[0]}.CoverArtID() } + // Tracks can be empty when they went missing or the owner lost access to their library. + if len(s.Tracks) == 0 { + return ArtworkID{} + } rnd := random.Int64N(len(s.Tracks)) return s.Tracks[rnd].CoverArtID() } diff --git a/model/share_test.go b/model/share_test.go new file mode 100644 index 000000000..b5e38af59 --- /dev/null +++ b/model/share_test.go @@ -0,0 +1,19 @@ +package model_test + +import ( + "github.com/navidrome/navidrome/model" + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" +) + +var _ = Describe("Share.CoverArtID", func() { + It("returns an empty artwork ID for a media file share with no visible tracks", func() { + s := model.Share{ResourceType: "media_file", ResourceIDs: "mf-1"} + Expect(s.CoverArtID()).To(Equal(model.ArtworkID{})) + }) + + It("picks a track's cover for a media file share", func() { + s := model.Share{ResourceType: "media_file", ResourceIDs: "mf-1", Tracks: model.MediaFiles{{ID: "mf-1"}}} + Expect(s.CoverArtID()).To(Equal(model.MediaFile{ID: "mf-1"}.CoverArtID())) + }) +}) diff --git a/persistence/share_repository.go b/persistence/share_repository.go index 4b6dc9240..92c696688 100644 --- a/persistence/share_repository.go +++ b/persistence/share_repository.go @@ -72,7 +72,6 @@ func (r *shareRepository) GetAll(options ...model.QueryOptions) (model.Shares, e } func (r *shareRepository) loadMedia(share *model.Share) error { - var err error ids := strings.Split(share.ResourceIDs, ",") if len(ids) == 0 { return nil @@ -80,15 +79,15 @@ func (r *shareRepository) loadMedia(share *model.Share) error { noMissing := func(cond Sqlizer) Sqlizer { return And{cond, Eq{"missing": false}} } + // Load as the share owner so their library access is applied, whoever renders the share. + ctx, err := r.ownerContext(share) + if err != nil { + return err + } switch share.ResourceType { case "artist": // Match by album-artist participation, not the deprecated album_artist_id // column (first album artist only), so co-album-artists are included too. - // Load as the share owner so their library access is applied. - ctx, err := r.ownerContext(share) - if err != nil { - return err - } albumRepo := NewAlbumRepository(ctx, r.db) share.Albums, err = albumRepo.GetAll(model.QueryOptions{Filters: noMissing(ParticipantIDFilter("album", ids, model.RoleAlbumArtist)), Sort: "artist"}) if err != nil { @@ -98,20 +97,15 @@ func (r *shareRepository) loadMedia(share *model.Share) error { share.Tracks, err = mfRepo.GetAll(model.QueryOptions{Filters: noMissing(ParticipantIDFilter("media_file", ids, model.RoleAlbumArtist)), Sort: "artist"}) return err case "album": - albumRepo := NewAlbumRepository(r.ctx, r.db) + albumRepo := NewAlbumRepository(ctx, r.db) share.Albums, err = albumRepo.GetAll(model.QueryOptions{Filters: noMissing(Eq{"album.id": ids})}) if err != nil { return err } - mfRepo := NewMediaFileRepository(r.ctx, r.db) + mfRepo := NewMediaFileRepository(ctx, r.db) share.Tracks, err = mfRepo.GetAll(model.QueryOptions{Filters: noMissing(Eq{"album_id": ids}), Sort: "album"}) return err case "playlist": - // Load tracks as the share owner so their library access is applied. - ctx, err := r.ownerContext(share) - if err != nil { - return err - } plsRepo := NewPlaylistRepository(ctx, r.db) // Tracks returns nil when the playlist is no longer visible to the owner // (e.g. it was made private after the share was created); leave the share @@ -127,7 +121,7 @@ func (r *shareRepository) loadMedia(share *model.Share) error { share.Tracks = tracks.MediaFiles() return nil case "media_file": - mfRepo := NewMediaFileRepository(r.ctx, r.db) + mfRepo := NewMediaFileRepository(ctx, r.db) tracks, err := mfRepo.GetAll(model.QueryOptions{Filters: noMissing(Eq{"media_file.id": ids})}) share.Tracks = sortByIdPosition(tracks, ids) return err diff --git a/persistence/share_repository_test.go b/persistence/share_repository_test.go index 3af91b2af..02d8a7386 100644 --- a/persistence/share_repository_test.go +++ b/persistence/share_repository_test.go @@ -228,7 +228,7 @@ var _ = Describe("ShareRepository", func() { }) }) - Describe("Artist share library scoping", func() { + Describe("Artist, album and media file share library scoping", func() { var otherLib model.Library var owner model.User const primaryID = "share-aa-primary" @@ -267,20 +267,26 @@ var _ = Describe("ShareRepository", func() { Expect(ur.Put(&owner)).To(Succeed()) Expect(ur.SetUserLibraries(owner.ID, []int{1})).To(Succeed()) - _, err := b.NewQuery(` - INSERT INTO share (id, user_id, description, resource_type, resource_ids, created_at, updated_at) - VALUES ({:id}, {:user}, {:desc}, {:type}, {:ids}, {:created}, {:updated}) - `).Bind(map[string]any{ - "id": "art-share", "user": owner.ID, "desc": "Artist scope share", - "type": "artist", "ids": secondaryID, "created": time.Now(), "updated": time.Now(), - }).Execute() - Expect(err).ToNot(HaveOccurred()) + for _, s := range []struct{ id, typ, ids string }{ + {"art-share", "artist", secondaryID}, + {"art-album-share", "album", "art-album-ok,art-album-other"}, + {"art-mf-share", "media_file", "art-ok,art-other"}, + } { + _, err := b.NewQuery(` + INSERT INTO share (id, user_id, description, resource_type, resource_ids, created_at, updated_at) + VALUES ({:id}, {:user}, {:desc}, {:type}, {:ids}, {:created}, {:updated}) + `).Bind(map[string]any{ + "id": s.id, "user": owner.ID, "desc": "Scope share", + "type": s.typ, "ids": s.ids, "created": time.Now(), "updated": time.Now(), + }).Execute() + Expect(err).ToNot(HaveOccurred()) + } }) AfterEach(func() { adminCtx := request.WithUser(log.NewContext(GinkgoT().Context()), adminUser) b := GetDBXBuilder() - _, _ = b.NewQuery(`DELETE FROM share WHERE id = 'art-share'`).Execute() + _, _ = b.NewQuery(`DELETE FROM share WHERE id IN ('art-share', 'art-album-share', 'art-mf-share')`).Execute() mr := NewMediaFileRepository(adminCtx, b).(*mediaFileRepository) _, _ = mr.executeSQL(squirrel.Delete("media_file").Where(squirrel.Eq{"id": []string{"art-ok", "art-other"}})) alr := NewAlbumRepository(adminCtx, b).(*albumRepository) @@ -309,6 +315,23 @@ var _ = Describe("ShareRepository", func() { Expect(share.Albums).ToNot(ContainElement(HaveField("ID", "art-album-other")), "an album outside the owner's libraries must not appear in the share") }) + + It("excludes albums and their tracks outside the owner's libraries from an album share", func() { + // Public share rendering has no user in the context. + share, err := NewShareRepository(log.NewContext(GinkgoT().Context()), GetDBXBuilder()).Get("art-album-share") + Expect(err).ToNot(HaveOccurred()) + Expect(share.Albums).To(ContainElement(HaveField("ID", "art-album-ok"))) + Expect(share.Albums).ToNot(ContainElement(HaveField("ID", "art-album-other"))) + Expect(share.Tracks).To(ContainElement(HaveField("ID", "art-ok"))) + Expect(share.Tracks).ToNot(ContainElement(HaveField("ID", "art-other"))) + }) + + It("excludes tracks outside the owner's libraries from a media file share", func() { + share, err := NewShareRepository(log.NewContext(GinkgoT().Context()), GetDBXBuilder()).Get("art-mf-share") + Expect(err).ToNot(HaveOccurred()) + Expect(share.Tracks).To(ContainElement(HaveField("ID", "art-ok"))) + Expect(share.Tracks).ToNot(ContainElement(HaveField("ID", "art-other"))) + }) }) Describe("Ownership Checks", func() { diff --git a/scanner/controller.go b/scanner/controller.go index df5aeb6f9..bfb396c6d 100644 --- a/scanner/controller.go +++ b/scanner/controller.go @@ -20,6 +20,7 @@ import ( "github.com/navidrome/navidrome/model/request" "github.com/navidrome/navidrome/server/events" "github.com/navidrome/navidrome/utils/pl" + "github.com/navidrome/navidrome/utils/singleton" "golang.org/x/time/rate" ) @@ -387,3 +388,12 @@ func (s *controller) trackProgress(ctx context.Context, progress <-chan *Progres func (s *controller) sendMessage(ctx context.Context, status *events.ScanStatus) { s.broker.SendBroadcastMessage(ctx, status) } + +// GetInstance returns the scanner singleton: Status reads the progress counters of the controller +// running the scan, and scheduler, watcher and signal scans do not start from the API's injector. +func GetInstance(rootCtx context.Context, ds model.DataStore, broker events.Broker, + pls playlists.Playlists, m metrics.Metrics) model.Scanner { + return singleton.GetInstance(func() *controller { + return New(rootCtx, ds, broker, pls, m).(*controller) + }) +} diff --git a/scanner/controller_test.go b/scanner/controller_test.go index 7ffdd69d4..45d202904 100644 --- a/scanner/controller_test.go +++ b/scanner/controller_test.go @@ -92,3 +92,13 @@ var _ = Describe("EffectiveFullScan", func() { Expect(scanner.EffectiveFullScan(context.Background(), ds, false, targets)).To(BeFalse()) }) }) + +var _ = Describe("GetInstance", func() { + It("returns the same controller to every caller", func() { + ds := &tests.MockDataStore{} + pls := playlists.NewPlaylists(ds, artwork.NewUploader(ds)) + a := scanner.GetInstance(context.Background(), ds, events.NoopBroker(), pls, metrics.NewNoopInstance()) + b := scanner.GetInstance(context.Background(), ds, events.NoopBroker(), pls, metrics.NewNoopInstance()) + Expect(a).To(BeIdenticalTo(b)) + }) +}) diff --git a/server/auth.go b/server/auth.go index 37a318a83..fdeb8c455 100644 --- a/server/auth.go +++ b/server/auth.go @@ -96,6 +96,16 @@ func buildAuthPayload(user *model.User) map[string]any { return payload } +// MaxLoginBodySize bounds the payload of unauthenticated login routes across all APIs. +const MaxLoginBodySize = 8 << 10 + +func LimitLoginBody(next http.Handler) http.Handler { + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + r.Body = http.MaxBytesReader(w, r.Body, MaxLoginBodySize) + next.ServeHTTP(w, r) + }) +} + func getCredentialsFromBody(r *http.Request) (username string, password string, err error) { data := make(map[string]string) decoder := json.NewDecoder(r.Body) @@ -150,6 +160,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) + return fmt.Errorf("creating initial user: %w", err) } return nil } diff --git a/server/auth_test.go b/server/auth_test.go index e021c82a8..af9ffcaeb 100644 --- a/server/auth_test.go +++ b/server/auth_test.go @@ -4,6 +4,7 @@ import ( "context" "crypto/md5" "encoding/json" + "errors" "fmt" "net/http" "net/http/httptest" @@ -64,6 +65,14 @@ var _ = Describe("Auth", func() { }) }) + Describe("createAdminUser", func() { + It("returns the error when the user cannot be saved", func() { + ds = &tests.MockDataStore{MockedUser: &tests.MockedUserRepo{Error: errors.New("db is down")}} + err := createAdminUser(context.Background(), ds, "johndoe", "secret") + Expect(err).To(MatchError(ContainSubstring("db is down"))) + }) + }) + Describe("Login from HTTP headers", func() { const ( trustedIpv4 = "192.168.0.42" @@ -200,6 +209,13 @@ var _ = Describe("Auth", func() { Expect(resp.Code).To(Equal(http.StatusUnauthorized)) }) + It("rejects a request body larger than the limit", func() { + body := `{"username":"janedoe", "password":"abc123", "padding":"` + strings.Repeat("x", MaxLoginBodySize) + `"}` + req = httptest.NewRequest("POST", "/login", strings.NewReader(body)) + LimitLoginBody(http.HandlerFunc(login(ds))).ServeHTTP(resp, req) + Expect(resp.Code).To(Equal(http.StatusUnprocessableEntity)) + }) + It("logs in successfully if user exists", func() { usr := ds.User(context.Background()) _ = usr.Put(&model.User{ID: "111", UserName: "janedoe", NewPassword: "abc123", Name: "Jane", IsAdmin: false}) diff --git a/server/jellyfin/api.go b/server/jellyfin/api.go index b0a5350b3..971f7794a 100644 --- a/server/jellyfin/api.go +++ b/server/jellyfin/api.go @@ -78,12 +78,11 @@ func (api *Router) routes() http.Handler { inner.Get("/quickconnect/enabled", api.quickConnectEnabled) // Rate-limit the password login, mirroring the native /auth/login: it's an unauthenticated // brute-force surface, so it must share the same per-client throttle when one is configured. + login := inner.With(server.LimitLoginBody) if conf.Server.AuthRequestLimit > 0 { - limiter := server.ClientIPRateLimiter(conf.Server.AuthRequestLimit, conf.Server.AuthWindowLength) - inner.With(limiter).Post("/users/authenticatebyname", api.authenticateByName) - } else { - inner.Post("/users/authenticatebyname", api.authenticateByName) + login = login.With(server.ClientIPRateLimiter(conf.Server.AuthRequestLimit, conf.Server.AuthWindowLength)) } + login.Post("/users/authenticatebyname", api.authenticateByName) inner.Get("/users/public", api.getPublicUsers) // Images are intentionally public: artwork isn't sensitive, matching Jellyfin's image handling. diff --git a/server/jellyfin/auth_test.go b/server/jellyfin/auth_test.go index 5a613f828..577677643 100644 --- a/server/jellyfin/auth_test.go +++ b/server/jellyfin/auth_test.go @@ -9,6 +9,7 @@ import ( "github.com/navidrome/navidrome/core/auth" "github.com/navidrome/navidrome/model" + "github.com/navidrome/navidrome/server" "github.com/navidrome/navidrome/server/jellyfin/dto" "github.com/navidrome/navidrome/tests" . "github.com/onsi/ginkgo/v2" @@ -101,3 +102,15 @@ var _ = Describe("AuthenticateByName", func() { Expect(w.Code).To(Equal(http.StatusUnauthorized)) }) }) + +var _ = Describe("AuthenticateByName body limit", func() { + It("rejects a request body larger than the limit", func() { + ds := &tests.MockDataStore{} + api := &Router{ds: ds} + w := httptest.NewRecorder() + body := `{"Username":"alice","Pw":"secret","Padding":"` + strings.Repeat("x", server.MaxLoginBodySize) + `"}` + r := httptest.NewRequest("POST", "/Users/AuthenticateByName", strings.NewReader(body)) + server.LimitLoginBody(http.HandlerFunc(api.authenticateByName)).ServeHTTP(w, r) + Expect(w.Code).To(Equal(http.StatusBadRequest)) + }) +}) diff --git a/server/server.go b/server/server.go index 7e7cfa3ea..7e7e2f6bd 100644 --- a/server/server.go +++ b/server/server.go @@ -204,6 +204,7 @@ func (s *Server) initRoutes() { func (s *Server) mountAuthenticationRoutes() chi.Router { r := s.router return r.Route(path.Join(conf.Server.BasePath, "/auth"), func(r chi.Router) { + r.Use(LimitLoginBody) if conf.Server.AuthRequestLimit > 0 { log.Info("Login rate limit set", "requestLimit", conf.Server.AuthRequestLimit, "windowLength", conf.Server.AuthWindowLength)