From 80e9921d45ae455bc27a455852c96635a7620369 Mon Sep 17 00:00:00 2001 From: Deluan Date: Wed, 4 Feb 2026 18:08:02 -0500 Subject: [PATCH 1/2] fix(subsonic): return error 70 when scrobble contains invalid IDs Implements OpenSubsonic API clarification from PR #202: the scrobble endpoint now validates all media file IDs before processing and returns error code 70 (data not found) if any ID is invalid. This ensures the entire operation fails atomically - no scrobbles are submitted when any ID in the batch is invalid, matching the OpenSubsonic specification. Changes include updating MediaFileRepository.Exists() to accept variadic IDs for efficient batch validation via a single COUNT query. See: https://github.com/opensubsonic/open-subsonic-api/pull/202 Signed-off-by: Deluan --- model/mediafile.go | 2 +- persistence/mediafile_repository.go | 15 ++++++++-- server/subsonic/media_annotation.go | 11 ++++++++ server/subsonic/media_annotation_test.go | 35 +++++++++++++++++++++++- tests/mock_mediafile_repo.go | 10 +++++-- 5 files changed, 66 insertions(+), 7 deletions(-) diff --git a/model/mediafile.go b/model/mediafile.go index 1d34cd76e..1a21da10b 100644 --- a/model/mediafile.go +++ b/model/mediafile.go @@ -354,7 +354,7 @@ type MediaFileCursor iter.Seq2[MediaFile, error] type MediaFileRepository interface { CountAll(options ...QueryOptions) (int64, error) CountBySuffix(options ...QueryOptions) (map[string]int64, error) - Exists(id string) (bool, error) + Exists(ids ...string) (bool, error) Put(m *MediaFile) error Get(id string) (*MediaFile, error) GetWithParticipants(id string) (*MediaFile, error) diff --git a/persistence/mediafile_repository.go b/persistence/mediafile_repository.go index 2054a34b6..18d2cfec0 100644 --- a/persistence/mediafile_repository.go +++ b/persistence/mediafile_repository.go @@ -143,8 +143,19 @@ func (r *mediaFileRepository) CountBySuffix(options ...model.QueryOptions) (map[ return counts, nil } -func (r *mediaFileRepository) Exists(id string) (bool, error) { - return r.exists(Eq{"media_file.id": id}) +// Exists checks if all given media file IDs exist in the database. If no IDs are provided, it returns true. +// If any of the IDs do not exist, it returns false. It returns an error if the database query fails. +func (r *mediaFileRepository) Exists(ids ...string) (bool, error) { + if len(ids) == 0 { + return true, nil + } + existsQuery := Select("count(*) as exist").From("media_file").Where(Eq{"media_file.id": ids}) + var res struct{ Exist int64 } + err := r.queryOne(existsQuery, &res) + if err != nil { + return false, err + } + return res.Exist == int64(len(ids)), nil } func (r *mediaFileRepository) Put(m *model.MediaFile) error { diff --git a/server/subsonic/media_annotation.go b/server/subsonic/media_annotation.go index 39bc83fa9..718f9921c 100644 --- a/server/subsonic/media_annotation.go +++ b/server/subsonic/media_annotation.go @@ -168,15 +168,26 @@ func (api *Router) Scrobble(r *http.Request) (*responses.Subsonic, error) { position := p.IntOr("position", 0) ctx := r.Context() + // Validate all IDs exist before processing (OpenSubsonic compliance) + exists, err := api.ds.MediaFile(ctx).Exists(ids...) + if err != nil { + return nil, err + } + if !exists { + return nil, newError(responses.ErrorDataNotFound, "Media file not found") + } + if submission { err := api.scrobblerSubmit(ctx, ids, times) if err != nil { log.Error(ctx, "Error registering scrobbles", "ids", ids, "times", times, err) + return nil, err } } else { err := api.scrobblerNowPlaying(ctx, ids[0], position) if err != nil { log.Error(ctx, "Error setting NowPlaying", "id", ids[0], err) + return nil, err } } diff --git a/server/subsonic/media_annotation_test.go b/server/subsonic/media_annotation_test.go index 6f09f5349..880106257 100644 --- a/server/subsonic/media_annotation_test.go +++ b/server/subsonic/media_annotation_test.go @@ -31,6 +31,12 @@ var _ = Describe("MediaAnnotationController", func() { }) Describe("Scrobble", func() { + BeforeEach(func() { + // Populate mock with valid media files + _ = ds.MediaFile(ctx).Put(&model.MediaFile{ID: "12"}) + _ = ds.MediaFile(ctx).Put(&model.MediaFile{ID: "34"}) + }) + It("submit all scrobbles with only the id", func() { submissionTime := time.Now() r := newGetRequest("id=12", "id=34") @@ -71,10 +77,27 @@ var _ = Describe("MediaAnnotationController", func() { Expect(playTracker.Submissions).To(BeEmpty()) }) + It("returns error when any id is invalid", func() { + r := newGetRequest("id=invalid") + + _, err := router.Scrobble(r) + + Expect(err).To(MatchError(ContainSubstring("not found"))) + Expect(playTracker.Submissions).To(BeEmpty()) + }) + + It("returns error and does not scrobble when mix of valid and invalid ids", func() { + r := newGetRequest("id=12", "id=invalid") + + _, err := router.Scrobble(r) + + Expect(err).To(MatchError(ContainSubstring("not found"))) + Expect(playTracker.Submissions).To(BeEmpty()) + }) + Context("submission=false", func() { var req *http.Request BeforeEach(func() { - _ = ds.MediaFile(ctx).Put(&model.MediaFile{ID: "12"}) ctx = request.WithPlayer(ctx, model.Player{ID: "player-1"}) req = newGetRequest("id=12", "submission=false") req = req.WithContext(ctx) @@ -94,6 +117,16 @@ var _ = Describe("MediaAnnotationController", func() { Expect(playTracker.Playing).To(HaveLen(1)) Expect(playTracker.Playing).To(HaveKey("player-1")) }) + + It("returns error when id is invalid", func() { + req = newGetRequest("id=invalid", "submission=false") + req = req.WithContext(ctx) + + _, err := router.Scrobble(req) + + Expect(err).To(MatchError(ContainSubstring("not found"))) + Expect(playTracker.Playing).To(BeEmpty()) + }) }) }) }) diff --git a/tests/mock_mediafile_repo.go b/tests/mock_mediafile_repo.go index 8542fc8d5..4cd011dd9 100644 --- a/tests/mock_mediafile_repo.go +++ b/tests/mock_mediafile_repo.go @@ -44,12 +44,16 @@ func (m *MockMediaFileRepo) SetData(mfs model.MediaFiles) { } } -func (m *MockMediaFileRepo) Exists(id string) (bool, error) { +func (m *MockMediaFileRepo) Exists(ids ...string) (bool, error) { if m.Err { return false, errors.New("error") } - _, found := m.Data[id] - return found, nil + for _, id := range ids { + if _, found := m.Data[id]; !found { + return false, nil + } + } + return true, nil } func (m *MockMediaFileRepo) Get(id string) (*model.MediaFile, error) { From 0e93ebfc73eea68d56ddf23ff6febec820b56b91 Mon Sep 17 00:00:00 2001 From: Deluan Date: Wed, 4 Feb 2026 18:32:01 -0500 Subject: [PATCH 2/2] fix(subsonic): add library filter and dedupe IDs in Exists Addresses code review feedback: apply library-level access control via applyLibraryFilter and handle duplicate IDs correctly using slice.Unique. This prevents users from verifying existence of tracks outside their accessible libraries and ensures duplicate IDs don't cause false negatives. Signed-off-by: Deluan --- persistence/mediafile_repository.go | 26 ++++++++++++++++++-------- 1 file changed, 18 insertions(+), 8 deletions(-) diff --git a/persistence/mediafile_repository.go b/persistence/mediafile_repository.go index 18d2cfec0..9e041f561 100644 --- a/persistence/mediafile_repository.go +++ b/persistence/mediafile_repository.go @@ -143,19 +143,29 @@ func (r *mediaFileRepository) CountBySuffix(options ...model.QueryOptions) (map[ return counts, nil } -// Exists checks if all given media file IDs exist in the database. If no IDs are provided, it returns true. -// If any of the IDs do not exist, it returns false. It returns an error if the database query fails. +// Exists checks if all given media file IDs exist in the database and are accessible to the current user. +// If no IDs are provided, it returns true. Duplicate IDs are handled correctly. +// If any of the IDs do not exist or are not accessible, it returns false. func (r *mediaFileRepository) Exists(ids ...string) (bool, error) { if len(ids) == 0 { return true, nil } - existsQuery := Select("count(*) as exist").From("media_file").Where(Eq{"media_file.id": ids}) - var res struct{ Exist int64 } - err := r.queryOne(existsQuery, &res) - if err != nil { - return false, err + uniqueIds := slice.Unique(ids) + + // Process in batches to avoid hitting SQLITE_MAX_VARIABLE_NUMBER limit (default 999) + const batchSize = 300 + var totalCount int64 + for batch := range slices.Chunk(uniqueIds, batchSize) { + existsQuery := Select("count(*) as exist").From("media_file").Where(Eq{"media_file.id": batch}) + existsQuery = r.applyLibraryFilter(existsQuery) + var res struct{ Exist int64 } + err := r.queryOne(existsQuery, &res) + if err != nil { + return false, err + } + totalCount += res.Exist } - return res.Exist == int64(len(ids)), nil + return totalCount == int64(len(uniqueIds)), nil } func (r *mediaFileRepository) Put(m *model.MediaFile) error {