fix: apply the per-user library filter to bookmarks, playlists and now-playing (#6179)

On a multi-library instance, a few reads and writes built their own queries
without the per-user library filter that every other media read applies. A
user granted only some libraries could see, and store, tracks from libraries
they had no access to.

- getBookmarks now filters the query. It has to be the query and not the
  result: the loop below it pre-sizes the response from the bookmark count,
  so a row dropped afterwards would emit an empty bookmark entry.
- createBookmark rejects an id the caller cannot read, returning error 70 to
  match getSong. Stored rows are left alone rather than purged, so a
  temporary revoke does not lose saved playback positions.
- playlistTrackRepository Read, Count and GetAlbumIDs get the filter their
  siblings CountAll and GetMediaFileIDs already had. Read is the one that
  mattered most: its id is the integer playlist position, so it needed no
  track id at all.
- Playlist track writes are filtered in playlistRepository.addTracks, the
  only writer of playlist_tracks rows apart from smart playlists, so Add,
  Insert, AddAlbums/AddArtists/AddDiscs and a full replace through Put all
  go through it. Insert reserves a slot per requested id, so when the filter
  drops one it renumbers to close the hole.
- playTracker.GetNowPlaying honours its context instead of discarding it.
  The cache is process-global, so the filter belongs in the tracker rather
  than in the Subsonic handler, and any future caller inherits it.

Admins and single-library installs are unaffected: applyLibraryFilter and
HasLibraryAccess both short-circuit for them. Scanner playlist sync runs as
admin, and M3U and CLI imports already resolve tracks through FindByPaths as
the same user, so neither changes.
This commit is contained in:
Deluan Quintão 2026-09-20 12:37:18 -04:00 • committed by GitHub
commit 8e784b6af7
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
12 changed files with 401 additions and 12 deletions

View file

@ -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

View file

@ -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"))
})
})
})

View file

@ -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() {