A listing other than /Items never receives the account's rating limit or blocked tags #128
Loading…
Reference in a new issue
No description provided.
Delete branch "%!s()"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
/Items/Latest,/Shows/NextUp, the by-name listings, the discover routes and the tv_shows routes all build anItemQuerycarryinguser_idand nomax_parental_ratingand noblocked_tags. Upstream constructsnew InternalItemsQuery(user), whose constructor callsSetUser— so every one of those listings is narrowed by the account's rating limit and blocked tags before it runs.Found while implementing #111, which closed the by-id half. The result is that a by-id read is now stricter than a listing: an account that is refused an item at
/Items/{id}can still be shown that item in Recently Added, Next Up, a genre listing or a search.That is safe in the direction that matters least and unsafe in the direction that matters: nothing leaks a body the account may not fetch, but the item's title, artwork and metadata are all in the listing.
Where to start
Every construction of
ItemQueryoutsidecrates/jellyfin-http/src/routes/items.rs. #111 addedrestrict_to_what_the_account_may_seethere andjellyfin_app::visibility::of, which builds aViewerfrom the user row, permissions and preferences — so the pieces exist and what is missing is the other callers using them.The shape worth copying is the one #111 chose for by-id reads: make the narrowing impossible to forget rather than remembered at each site. A listing that takes a user and does not narrow by that user is the bug, so the type that carries the user should be the thing that applies it.
Upstream:
InternalItemsQuery.SetUser, and theIsParentalAllowed/IsVisibleViaTagsrules underBaseItem.IsVisibleStandalone.How to know it is done
A restricted account gets the same listing from both servers on every route above — measured on a pair with real accounts, since the shared reference has one account and creating another there is a write. #111's
visibility.jsonand itsaccountfield in a harness request are the machinery for this; extend them rather than writing a second thing.House rules
Tests run under
cargo nextest; upstream's source is the checkout inupstream/at the pin; generated files are regenerated rather than edited; comments say what the code does, not how it got there. Anything needing a write to the reference wants its own container rather than the shared one —#111did that and it worked: the pinned image over asqlite3 .backupcopy ofreference/var/config/data/jellyfin.db, on ports of its own.Done, on
main— seven commits,ec8e778throughd92ad66.The shape
ItemQuerylostuser_id,max_parental_ratingandblocked_tags. ThirteenItemRepositorymethods take a&Viewerinstead —query,count,any_search_candidate,count_by_type,named_in,values_in,count_under_each,count_directly_under_each,years_in,official_ratings_in,languages_in,people_page,count_people. Whose marks a played or favourite filter reads is now the viewer's account, so a listing cannot carry an account's playback state without being narrowed by that account.Listingis the only thingnarrowing()accepts, and a reviewer confirmed no path reaches rows without one.That is M11's rule for by-id reads applied to listings: make the unchecked read impossible to express rather than remembered at each of sixty call sites.
/Shows/{id}/Seasonsand/Shows/{id}/Episodesnamed no account at all — no narrowing and no marks filter. They do now.Measured
Nine accounts against a pair built for it —
restricted,unrated,showkid,ratingkid,tagkid,allowkid,unratedkid,unratedall, and the administrator — over 40 routes, comparing names andTotalRecordCount:BlockUnratedItemsMovie, SeriesMaxParentalRating7BlockedTagsAllowedTagsBlockUnratedItemsMovieFifteen of the seventeen are
/Items/Suggestionsand/Search/Hints, which the administrator sees too — filed as #138 and #139, neither a visibility defect. The other two are the/Yearswalk-versus-query residual on an edited pair; on the fixture as provisioned/Yearsagrees for all three of its accounts.The question this closes: an independent reviewer enumerated every id our server puts into any of 34 listings for each of 8 restricted accounts and then asked for each of those ids — 270 by-id reads, zero status disagreements with upstream, and every row still visible in a listing is one upstream shows too.
#129's two rules, verified against source
IsVisibleViaTagstests blocked tags first, then exempts a row whose first parent is aUserRootFolder,AggregateFolderorUserView, then applies the allowlist — so the exemption cannot leak into the blocked half, and the port keeps them separate. The Person exemption in the SQL is upstream's|| e.Type == personTypeName, not an invention.BlockUnratedItemsis per kind, and which kinds answer for themselves is the whole rule: the default returns false for a folder and anIItemByName;Series,MusicAlbumandBoxSetoverride it; andSeasonexplicitly returns false, so a season is refused by the ancestor chain rather than by its own bucket. Measured for an account blocking all nine kinds: every season, every episode and the series 404, the music album stays 200 because it answers only for theMusicbucket, and the artist, the year and the library folders stay 200 — matching upstream exactly.What this round found beyond the two issues
A regression the previous batch shipped, and I signed off on.
ff48166invented a named/unnamed distinction in/Items/Latest's grouping. Upstream'sUserViewManager.cs:369-392groups onrequest.GroupItemsalone and collects the root's own children rather than the views. The administrator agrees either way only because their first root child is the empty box-set library, which is why my verification of that batch passed. For a movies-only account it was a live divergence. Fixed in95be42b, and the test that asserted the wrong rule is rewritten./Yearscounted every library the server holds rather than the ones the account may see — a #128-class defect one function deeper than the audit reached.restrictedwent 9 → 6 against upstream's 6.Viewerhad picked up#[derive(Default)], where the default isanyone()— a silent constructor for "sees everything" on the one type whose purpose is that this cannot be reached by omission. Removed, with a doc comment saying why it is absent.The cost was measured rather than assumed. The viewer was being rebuilt at every call site — seven
Preferencesreads inside one/Items/Latest. It is memoized per request now, in atask_localentered bycanonical::rewrite, which every request already passes through:/Items/Latestends below where it started./Personsand/Shows/NextUpstay above because they now do work they did not do — the people access filter, and the account read that is #128 itself. Nothing needs invalidating: no route writes a policy and then answers with a row, and the memo dies with the request.Replay
visibility.jsonis 57 responses, 0 differences — and 356 differences across 15 entries against the pre-change binary, which is the measure of whether a scenario bites. It could not run at all until the reference was rebuilt with arestrictedaccount, and #129 became replayable only once the fixture grew anunratedone (2dec147), since every row indefaultis unrated and that is the one policy that refuses anything there.Fifteen new entries, including the three that must stay
200— an album, an artist and a year read by id — which are the over-blocking guard rather than passengers./Personsnow returns a row forunrated, so it is no longer an empty-versus-empty comparison anywhere in the file.Regression guard, all at their baselines:
handshake,items,browsing,playback,library,serving,bundleat 0,portable6,refusals8.JELLYFIN_RS_REQUIRE_FIXTURES=1 cargo nextest run --workspaceonmain: 2234 passed, 0 skipped. Clippy clean. No ignore-list entry added or touched.Named rather than left to be found
under_the_rootreads withViewer::anyone()and narrows only by the library permission; upstream'sGetChildren(user, true)asks the whole rule, and theUserRootFolderexemption fires after the blocked-tag test. Unmeasurable on this fixture — no library folder carries a tag — and the comment says so rather than claiming parity.named_in's second pass applies the listing form of the unrated rule to the name row itself, so a genre or studio row could be refused where the by-id form would exempt it as a folder. Only/Artistsreaches that path here;/Genres,/MusicGenresand/Studiosare empty for every account./Yearsstill walks where upstream prunes: it takes a row the account may see under a folder it may not. Nothing in the fixture is rated, so nothing triggers it today.All three are in the harness README's "what this fixture cannot ask".
The risk row
MILESTONES.mdsaid this was half-corrected and named these two issues. What remains true is now narrower and is recorded there.