You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
fix(jellyfin): clamp negative StartIndex to zero - #6341 - #6366
GET /Items?IncludeItemTypes=Audio,MusicAlbum&StartIndex=-1&Limit=5 panics with runtime error: slice bounds out of range [-1:] (recovered as an HTTP 500), even on an empty library.
Root cause:StartIndex is read with no lower bound and reaches paginate, whose offset >= len(items) check only guards the upper bound before items[offset:] slices. A negative StartIndex also under-widens the per-type merge window (window := offset + q.limit) and is echoed back verbatim in the response's StartIndex field.
The same unguarded read reaches paginate from two other endpoints — /Genres (via listGenres) and /Studios — so StartIndex=-1 panics there too. /Playlists/{id}/Items and single-type /Items push the offset down to SQL instead, so they cannot panic, but they still echoed the negative value.
Fix: clamp StartIndex to >= 0 at every startindex read (listParams, getGenres, getStudios, getPlaylistItems) — the same clamp idiom the codebase already uses for limit — and guard paginate itself so no current or future caller can slice with a negative offset. A negative StartIndex now behaves exactly like StartIndex=0 on all of these endpoints.
Prior agreement is not needed here — this falls under the small-change exception: a small, unambiguous fix restoring established behavior (a panic recovered as HTTP 500), with reproduction evidence under How to Test.
Type of Change
Bug fix
Checklist
My code follows the project's coding style
I have tested the changes locally
I have added or updated documentation as needed
I have added tests that prove my fix/feature works (or explain why not)
All existing and new tests pass
How to Test
New regression specs fail before the fix and pass after:
server/jellyfin/items_test.go: multi-type /Items?IncludeItemTypes=Audio,MusicAlbum&StartIndex=-1&Limit=2 (the reported panic), plus a single-type /Items?...StartIndex=-1 control
server/jellyfin/browsing_test.go: /Genres?StartIndex=-1 and /Studios?StartIndex=-1
Before: all 5 new specs failed — 4 panicked with slice bounds out of range [-1:], the 5th showed Offset/StartIndex propagating as -1.
After: go test ./server/jellyfin/ → ok (510 specs); go test -tags netgo,sqlite_fts5 ./... → all packages ok; go vet -tags netgo,sqlite_fts5 ./... → clean.
(go test/go vet without build tags fail on clean master too — the project requires the make test tags for main.go/FTS5; unrelated to this change.)
Screenshots / Demos (if applicable)
N/A — backend/API change, no UI.
Additional Notes
Open PR fix(jellyfin): list the user's libraries at the /Items user root #6214 touches the same files but clamps StartIndex only inside its new userRoot branch; it does not touch listParams, paginate, or these handlers, so this change is disjoint — a rebase after it merges should be clean.
The SQL-offset paths could not panic because the persistence layer only applies Offset when it is > 0; clamping there keeps semantics consistent and makes the echoed StartIndex honest.
chained's skip loop already behaved like offset 0 for a negative skip (n < skip is never true), so the unbounded multi-type merge path was safe.
Assistance
AI use (check one):
Yes, AI helped generate or modify code or tests, draft substantive PR text, or create this PR
No
AI tools helped draft this change and this description; the verification steps were actually run.
Summary by CodeRabbit
Bug Fixes
Fixed Jellyfin browsing requests for genres, studios, items, and playlists that use a negative start index.
Negative start indexes are now normalized to zero, so requests return results from the beginning instead of failing.
Pagination handles very large start indexes safely for item searches, while preserving the requested result limit.
These adjustments make pagination behavior more consistent across the supported browsing requests.
PR summary: Clamps a negative client-supplied StartIndex to zero in the Jellyfin pagination handlers (items, genres, studios, playlist items), adds a guard in the in-memory paginate helper, and makes the multi-type /Items search window overflow-safe, with regression specs for each endpoint.
Review summary
The PR clamps a client-supplied negative StartIndex to zero across the Jellyfin /Items, /Genres, /Studios and /Playlists/{id}/Items pagination reads and adds a defensive non-negative guard inside paginate. Verified that this closes the previously reported StartIndex-overflow panic on the multi-type search merge (offset is capped before the sum and search limits are clamped to [1, maxSearchLimit]), so that finding is resolved. One issue remains: the new shared pageParams helper still forwards Limit unvalidated, so on the non-search multi-type /Items merge a negative Limit or an overflowing StartIndex+Limit wraps the per-type window to a non-positive value, which persistence treats as "no LIMIT" — every row of every merged type is then drained and materialised in memory instead of taking the streaming path. This is reported as "Unvalidated Limit and StartIndex+Limit overflow force full-library materialisation on multi-type /Items". Library scoping for the edited /Studios handler and the SQL-offset paths (single-type /Items, playlists, genres) were traced and found sound.
Ruled out (1)
Does the edited /Studios handler return record labels from libraries the caller cannot access? The tag repository always applies its own user_library INNER JOIN, so an empty library grant yields zero rows rather than every library's labels.
Review history (4)
Review 4 · ef563a8 · Nov 10 21:07 UTC · Nothing new
Review 3 · 872a296 · Nov 10 20:49 UTC · Found 1 new Medium Vulnerability
Review 2 · a57467e · Nov 8 22:27 UTC · Nothing new
Review 1 · 8cc6178 · Oct 21 12:13 UTC · Nothing new
Review 4 on this PR · ef563a8 · requested Nov 10 20:56 UTC on a new commit
No actionable comments were generated in the recent review.
Walkthrough
Jellyfin item queries, genre and studio listings, and playlist item requests now normalize negative StartIndex values to zero. Regression tests cover these requests and large multi-type query offsets.
Changes: Jellyfin pagination
Layer / File(s)
Summary
Item query and pagination server/jellyfin/items.go, server/jellyfin/items_test.go
Item query parameters and pagination clamp negative offsets to zero. Multi-type search windows cap the offset before adding the limit. Tests cover negative offsets and large offsets, including math.MaxInt.
Browsing and playlist pagination server/jellyfin/browsing.go, server/jellyfin/browsing_test.go, server/jellyfin/playlists.go, server/jellyfin/playlists_test.go
Genre, studio, and playlist item endpoints use the shared pagination helper. Tests verify successful responses and normalized indexes or query offsets.
The pagination change is mergeable after normal checks; no new blocking risk was established.
Pre-merge checks: 5
Check name
Status
Explanation
Title check
The title clearly summarizes the main change: clamping negative Jellyfin StartIndex values to zero.
Description check
The description includes the change, root cause, related issue, bug-fix type, testing steps and results, and relevant notes. It follows the template and provides sufficient detail.
Linked Issues check
Issue #6341 requires negative StartIndex on multi-type /Items to behave as zero or return HTTP 400, without panicking. listParams now gets pagination values from pageParams, and paginate cla…
Out of Scope Changes check
The changes support the pagination fix in #6341. The maxSearchLimit window cap and its large-offset tests protect the same multi-type pagination path. The endpoint changes and regression tests cover…
Docstring Coverage
Docstring coverage is inconclusive: files from earlier reviews still need to be rechecked within the check's analysis limits. Not blocking.
Full details: Linked Issues check
Explanation
Issue #6341 requires negative StartIndex on multi-type /Items to behave as zero or return HTTP 400, without panicking. listParams now gets pagination values from pageParams, and paginate clamps negative offsets before slicing. The summaries also show the shared helper is used by /Genres, /Studios, and /Playlists/{id}/Items. Regression tests cover these paths and single-type /Items, including normalized offsets. The change meets the linked issue requirement.
Full details: Out of Scope Changes check
Explanation
The changes support the pagination fix in #6341. The maxSearchLimit window cap and its large-offset tests protect the same multi-type pagination path. The endpoint changes and regression tests cover related paths identified in the issue and PR description. No unrelated change is demonstrated.
Finishing Touches
Generate unit tests (beta)
Create a new PR
Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
File: No configuration file from this repository applied
Commit: Unavailable
Layers: defaults
Review profile: CHILL
Plan: Advanced
Run ID: 10a79ee3-26e8-465f-a5ea-9c85fa4beb7e
Commits
Reviewing files that changed from the base of the PR and between 872a296 and ef563a8.
Files selected for processing: 2
server/jellyfin/items.go
server/jellyfin/items_test.go
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 8 remain after this review.
… helper
The negative-StartIndex clamp was repeated in getItems (listParams), getGenres,
getStudios and getPlaylistItems. Move the StartIndex/Limit parsing into one
pageParams helper so a new paginated endpoint gets the clamp by calling it,
instead of having to copy max(..., 0). Behavior is unchanged: Limit is read
as before, and the existing negative-StartIndex specs cover every caller.
Thanks for the fix and the thorough specs, @santhiprakash! I pushed a small follow-up commit that moves the StartIndex/Limit parsing into a shared pageParams helper. That way new paginated endpoints get the clamp without having to copy max(..., 0). Behavior is the same, and your specs cover every caller.
…ge window
On a multi-type /Items search, mergeTypesPaged computed the per-type window as
offset+limit and only then capped it at maxSearchLimit. A StartIndex near the
int limit wrapped that sum negative, the cap kept the negative value, and
searchPage sliced rows[:Max] with it inside an errgroup worker, where the HTTP
Recoverer cannot catch the panic, so one request stopped the server.
Cap the offset before adding the limit. The search limit is already bounded by
clampLimit, so the sum stays small and the window is unchanged for every
in-range request.
Resolve the semantic conflicts with master. navidrome#6414 replaced artistId with
artistIds, so hasFilters now checks len(q.artistIds). navidrome#6414 also pairs each
SortOrder value with its SortBy key, so sortViews uses listParts and
sortDirection the same way applySort does, and a new spec covers the pairing.
navidrome#6366 clamps StartIndex in pageParams, so the user-root branch drops its own
max(q.offset, 0).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
![strix-security[bot]](https://avatars.githubusercontent.com/in/2749804?s=60&v=4){kind=small}
Description
GET /Items?IncludeItemTypes=Audio,MusicAlbum&StartIndex=-1&Limit=5panics withruntime error: slice bounds out of range [-1:](recovered as an HTTP 500), even on an empty library.Root cause:
StartIndexis read with no lower bound and reachespaginate, whoseoffset >= len(items)check only guards the upper bound beforeitems[offset:]slices. A negativeStartIndexalso under-widens the per-type merge window (window := offset + q.limit) and is echoed back verbatim in the response'sStartIndexfield.The same unguarded read reaches
paginatefrom two other endpoints —/Genres(vialistGenres) and/Studios— soStartIndex=-1panics there too./Playlists/{id}/Itemsand single-type/Itemspush the offset down to SQL instead, so they cannot panic, but they still echoed the negative value.Fix: clamp
StartIndexto >= 0 at everystartindexread (listParams,getGenres,getStudios,getPlaylistItems) — the same clamp idiom the codebase already uses forlimit— and guardpaginateitself so no current or future caller can slice with a negative offset. A negativeStartIndexnow behaves exactly likeStartIndex=0on all of these endpoints.Related Issues
Closes #6341
Prior agreement is not needed here — this falls under the small-change exception: a small, unambiguous fix restoring established behavior (a panic recovered as HTTP 500), with reproduction evidence under How to Test.
Type of Change
Checklist
How to Test
New regression specs fail before the fix and pass after:
server/jellyfin/items_test.go: multi-type/Items?IncludeItemTypes=Audio,MusicAlbum&StartIndex=-1&Limit=2(the reported panic), plus a single-type/Items?...StartIndex=-1controlserver/jellyfin/browsing_test.go:/Genres?StartIndex=-1and/Studios?StartIndex=-1server/jellyfin/playlists_test.go:/Playlists/{id}/Items?StartIndex=-1Verified on current
master(df77c462):slice bounds out of range [-1:], the 5th showedOffset/StartIndexpropagating as -1.go test ./server/jellyfin/→ ok (510 specs);go test -tags netgo,sqlite_fts5 ./...→ all packages ok;go vet -tags netgo,sqlite_fts5 ./...→ clean.go test/go vetwithout build tags fail on cleanmastertoo — the project requires themake testtags formain.go/FTS5; unrelated to this change.)Screenshots / Demos (if applicable)
N/A — backend/API change, no UI.
Additional Notes
StartIndexonly inside its newuserRootbranch; it does not touchlistParams,paginate, or these handlers, so this change is disjoint — a rebase after it merges should be clean.Offsetwhen it is> 0; clamping there keeps semantics consistent and makes the echoedStartIndexhonest.chained's skip loop already behaved like offset 0 for a negativeskip(n < skipis never true), so the unbounded multi-type merge path was safe.Assistance
AI use (check one):
AI tools helped draft this change and this description; the verification steps were actually run.
Summary by CodeRabbit