fix(library): scope a library listing to that library

Opening a library that is not Music, Movies or TV served whatever
happened to be cached — films under Books, albums under Photos — rather
than the library's own contents.

The cached-browse query matched a library parent with an EXISTS that
never referenced the item:

    OR EXISTS (SELECT 1 FROM libraries l
               WHERE l.id = ? AND l.server_id = i.server_id)

It asks only whether a library with the requested id exists, so it is
true for every cached row the moment the parent is any library. The three
typed libraries concealed it because their landing pages pass
include_item_types, which narrowed the result to albums or films or
series; the generic library page passes none, so nothing narrowed it at
all.

`library_id` now decides wherever the cache kept one. That is the
server's own answer, and the only thing that can scope a library whose
type has no mapping (Books, Photos, Collections) or none at all — a
mixed library, where Jellyfin sends CollectionType null. The
collection_type/item_type taxonomy stays as the fallback for rows written
before the link was stored, and a library with neither matches nothing
and falls through to the server, which does know what is in it.

The taxonomy is now one macro shared with the downloaded listing. That
listing had the identical defect and it was fixed there alone (DR-167) —
the comment there even says the mapping "is needed in two places that
must agree", which was true of a third place nobody looked at.

One existing assertion changed rather than being worked around:
UT-206 expected a lib-2 album back from a lib-1 listing, which only held
because of this bug. It is about parameter binding order, so it keeps
testing exactly that, now with an album that is really in lib-1.
This commit is contained in:
2026-09-07 00:11:20 +02:00
parent c44070e720
commit 368935e6f4
2 changed files with 181 additions and 11 deletions
+4 -1
View File
@@ -474,6 +474,7 @@ Internal architecture, components, and application logic.
| DR-274 | Startup shows the picker only when the last-used profile has a PIN, or when more than one profile exists and the setting asks for it; otherwise it resumes exactly as before. The feature is invisible to a single-account install, which is what makes it safe to ship without a migration anyone has to think about | Auth | UR-082 | Proposed | | DR-274 | Startup shows the picker only when the last-used profile has a PIN, or when more than one profile exists and the setting asks for it; otherwise it resumes exactly as before. The feature is invisible to a single-account install, which is what makes it safe to ship without a migration anyone has to think about | Auth | UR-082 | Proposed |
| DR-275 | Idle re-lock separates "the UI is locked" from "who is the active profile", so audio keeps playing and keeps reporting as the account that started it while the screen is locked. Lockscreen transport controls keep working untouched, because nothing on a lockscreen browses or starts new content — the locked UI refuses only what reaches past the current queue. The timer starts when playback stops rather than when the UI goes quiet, and unlocking to a *different* profile stops playback. It lives in Rust beside the player state machine: it needs authoritative playback state, and a frontend timer dies with the WebView on Android | Player | UR-083 | Proposed | | DR-275 | Idle re-lock separates "the UI is locked" from "who is the active profile", so audio keeps playing and keeps reporting as the account that started it while the screen is locked. Lockscreen transport controls keep working untouched, because nothing on a lockscreen browses or starts new content — the locked UI refuses only what reaches past the current queue. The timer starts when playback stops rather than when the UI goes quiet, and unlocking to a *different* profile stops playback. It lives in Rust beside the player state machine: it needs authoritative playback state, and a frontend timer dies with the WebView on Android | Player | UR-083 | Proposed |
| DR-276 | The picker and PIN pad render an opaque `unlock_method` and an `UnlockOutcome` union the backend returns; the frontend never compares a PIN, counts an attempt, or infers that an account without a PIN is a child's. "Child account" is not modelled at all — a child profile is simply one with no PIN — so no role taxonomy is invented on either side of a boundary that has leaked taxonomy before | Frontend | UR-082, UR-083 | Proposed | | DR-276 | The picker and PIN pad render an opaque `unlock_method` and an `UnlockOutcome` union the backend returns; the frontend never compares a PIN, counts an attempt, or infers that an account without a PIN is a child's. "Child account" is not modelled at all — a child profile is simply one with no PIN — so no role taxonomy is invented on either side of a boundary that has leaked taxonomy before | Frontend | UR-082, UR-083 | Proposed |
| DR-277 | A library listing is scoped to that library. The cached-browse query matched a library parent with an `EXISTS` that never referenced the item — it asked only whether a library with the requested id existed — so the clause was true for every cached row on the server. Music, Movies and TV concealed it because their landing pages pass `include_item_types`, which narrowed the result; the generic library page passes none, so opening Books, Photos, Collections or a mixed library served whatever happened to be cached. The stored `library_id` now decides wherever the cache kept one, because that is the server's own answer and the only thing able to scope a library whose type has no mapping or none at all; the `collection_type``item_type` taxonomy is the fallback for rows written before it was stored, and a library with neither matches nothing and falls through to the server. The taxonomy itself is now a single macro shared with the downloaded listing, which had the identical defect fixed in isolation (DR-167) while this path kept it | Repository | UR-007 | Done |
| DR-198 | The webview runs under a real Content-Security-Policy, and the asset protocol is scoped to the one directory it still serves. `csp` was `null`, which disables CSP entirely: any script that reached the web layer — through a future `{@html}`, a dependency, or a devtools paste — would have inherited the whole IPC surface, and with it the user's session. `script-src 'self'` (Tauri injects a nonce for SvelteKit's inline bootstrap script at build time, so no `'unsafe-inline'` is needed) plus `object-src`/`frame-src 'none'` and `base-uri 'self'` is the part that is genuinely restrictive. `img-src`/`media-src`/`connect-src` cannot be: the Jellyfin origin is typed in by the user at run time and is commonly plain `http` on a LAN, so they allow `http:`/`https:` — a wide grant for *data*, but one that still bars `file:`, `filesystem:` and scripting schemes, and leaves `script-src` untouched. `style-src` keeps `'unsafe-inline'` because Svelte compiles `style="…"` attributes (including `app.html`'s `display: contents` wrapper) into markup; this is safe only while no `<style>` element survives into `index.html`, since a nonce there would make Tauri's injection outrank — and therefore void — `'unsafe-inline'`. `worker-src blob:` and `media-src blob:` are hls.js: it demuxes in a worker built from a blob and attaches MSE through `URL.createObjectURL`. `asset:` and `http://asset.localhost` are the same protocol under the two naming schemes `convertFileSrc` emits (custom scheme on Linux/macOS, `http` host on Windows/Android); `ipc:`/`http://ipc.localhost` is the invoke transport, which would otherwise be blocked by `connect-src`. A run-time CSP naming the server origin exactly was rejected: Tauri computes the header from immutable config when it serves the HTML, so it would mean rebuilding config and reloading the webview on every server change, for a policy the user can already point anywhere. The asset-protocol scope narrows from `$APPDATA/**` to `$APPDATA/thumbnails/**` — since DR-137 moved downloaded media to the loopback server, `imageCache` is the only `convertFileSrc` caller left, so the database and the encrypted-token fallback file no longer sit inside the grant | Security | UR-012, UR-071 | Done | | DR-198 | The webview runs under a real Content-Security-Policy, and the asset protocol is scoped to the one directory it still serves. `csp` was `null`, which disables CSP entirely: any script that reached the web layer — through a future `{@html}`, a dependency, or a devtools paste — would have inherited the whole IPC surface, and with it the user's session. `script-src 'self'` (Tauri injects a nonce for SvelteKit's inline bootstrap script at build time, so no `'unsafe-inline'` is needed) plus `object-src`/`frame-src 'none'` and `base-uri 'self'` is the part that is genuinely restrictive. `img-src`/`media-src`/`connect-src` cannot be: the Jellyfin origin is typed in by the user at run time and is commonly plain `http` on a LAN, so they allow `http:`/`https:` — a wide grant for *data*, but one that still bars `file:`, `filesystem:` and scripting schemes, and leaves `script-src` untouched. `style-src` keeps `'unsafe-inline'` because Svelte compiles `style="…"` attributes (including `app.html`'s `display: contents` wrapper) into markup; this is safe only while no `<style>` element survives into `index.html`, since a nonce there would make Tauri's injection outrank — and therefore void — `'unsafe-inline'`. `worker-src blob:` and `media-src blob:` are hls.js: it demuxes in a worker built from a blob and attaches MSE through `URL.createObjectURL`. `asset:` and `http://asset.localhost` are the same protocol under the two naming schemes `convertFileSrc` emits (custom scheme on Linux/macOS, `http` host on Windows/Android); `ipc:`/`http://ipc.localhost` is the invoke transport, which would otherwise be blocked by `connect-src`. A run-time CSP naming the server origin exactly was rejected: Tauri computes the header from immutable config when it serves the HTML, so it would mean rebuilding config and reloading the webview on every server change, for a policy the user can already point anywhere. The asset-protocol scope narrows from `$APPDATA/**` to `$APPDATA/thumbnails/**` — since DR-137 moved downloaded media to the loopback server, `imageCache` is the only `convertFileSrc` caller left, so the database and the encrypted-token fallback file no longer sit inside the grant | Security | UR-012, UR-071 | Done |
--- ---
@@ -490,7 +491,7 @@ Internal architecture, components, and application logic.
| UR-004 | IR-003, IR-004, IR-008, IR-011 | DR-002, DR-004, DR-006, DR-129, DR-171, DR-176, DR-177, DR-181, DR-182, DR-183, DR-185, DR-188, DR-203, DR-265 | | UR-004 | IR-003, IR-004, IR-008, IR-011 | DR-002, DR-004, DR-006, DR-129, DR-171, DR-176, DR-177, DR-181, DR-182, DR-183, DR-185, DR-188, DR-203, DR-265 |
| UR-005 | - | DR-001, DR-005, DR-009, DR-178, DR-179, DR-186, DR-193, DR-195 | | UR-005 | - | DR-001, DR-005, DR-009, DR-178, DR-179, DR-186, DR-193, DR-195 |
| UR-006 | IR-005, IR-006, IR-007, IR-008 | DR-200, DR-201 | | UR-006 | IR-005, IR-006, IR-007, IR-008 | DR-200, DR-201 |
| UR-007 | IR-010 | DR-007, DR-008, DR-016, DR-257, DR-262 | | UR-007 | IR-010 | DR-007, DR-008, DR-016, DR-257, DR-262, DR-277 |
| UR-008 | IR-010 | DR-007, DR-011 | | UR-008 | IR-010 | DR-007, DR-011 |
| UR-009 | IR-009, IR-010, IR-011 | - | | UR-009 | IR-009, IR-010, IR-011 | - |
| UR-010 | IR-012, IR-021 | DR-037, DR-059 | | UR-010 | IR-012, IR-021 | DR-037, DR-059 |
@@ -816,6 +817,8 @@ Internal architecture, components, and application logic.
| UT-245 | A `timeupdate` is applied while the video is playing — the case that froze the position behind a PiP window — and still yields to an in-flight seek, a seek-bar drag, and an element with no current data | DR-265 | Done | | UT-245 | A `timeupdate` is applied while the video is playing — the case that froze the position behind a PiP window — and still yields to an in-flight seek, a seek-bar drag, and an element with no current data | DR-265 | Done |
| UT-246 | Opening a PiP window disarms background audio, and a background signal arriving with the native PiP flag false is still treated as PiP while the frontend's latch says the window is open — without resurrecting one it has already seen close | DR-266 | Done | | UT-246 | Opening a PiP window disarms background audio, and a background signal arriving with the native PiP flag false is still treated as PiP while the frontend's latch says the window is open — without resurrecting one it has already seen close | DR-266 | Done |
| UT-247 | A library whose `collection_type` has no mapping — Books, Photos, a mixed library — does not return the server's films, albums and shows from cache | DR-277 | Done |
| UT-248 | Narrowing the library clause does not starve the libraries that do have landing pages: music, movies and TV each still list their own media and none of the others | DR-277 | Done |
### Integration Tests ### Integration Tests
| Test ID | Test Description | Traces To | Status | | Test ID | Test Description | Traces To | Status |
+176 -9
View File
@@ -84,6 +84,30 @@ fn build_fts_prefix_query(query: &str) -> Option<String> {
) )
} }
/// The Jellyfin taxonomy half of "does cached item `i` belong to library `l`":
/// the library's `collection_type` against the item's `item_type`.
///
/// A macro rather than a `const` because both callers need it *inside* a larger
/// SQL string literal, and `concat!` cannot take a const. One definition, so the
/// two sites cannot drift — they did once already, and opening any downloaded
/// library then listed every downloaded item on the server (DR-167).
///
/// Deliberately has **no fall-open arm**. Adding "…or the type is unknown"
/// makes the clause true for every row, which is precisely the defect it exists
/// to prevent; callers that want that behaviour must say so themselves and
/// justify it, as `LIBRARY_HOLDS_ITEM` does.
///
/// TRACES: UR-007, UR-055 | DR-167, DR-277
macro_rules! library_type_matches_item {
() => {
"(
(l.collection_type = 'music' AND i.item_type IN ('MusicAlbum', 'MusicArtist', 'Audio'))
OR (l.collection_type = 'movies' AND i.item_type = 'Movie')
OR (l.collection_type = 'tvshows' AND i.item_type IN ('Series', 'Season', 'Episode'))
)"
};
}
pub struct OfflineRepository { pub struct OfflineRepository {
db_service: Arc<RusqliteService>, db_service: Arc<RusqliteService>,
server_id: String, server_id: String,
@@ -856,13 +880,13 @@ impl OfflineRepository {
/// no mapping to narrow it by and hiding its contents would be worse. /// no mapping to narrow it by and hiding its contents would be worse.
/// ///
/// TRACES: UR-055 | DR-082, DR-167 /// TRACES: UR-055 | DR-082, DR-167
const LIBRARY_HOLDS_ITEM: &'static str = "( const LIBRARY_HOLDS_ITEM: &'static str = concat!(
(l.collection_type = 'music' AND i.item_type IN ('MusicAlbum', 'MusicArtist', 'Audio')) "(",
OR (l.collection_type = 'movies' AND i.item_type = 'Movie') library_type_matches_item!(),
OR (l.collection_type = 'tvshows' AND i.item_type IN ('Series', 'Season', 'Episode')) " OR l.collection_type IS NULL
OR l.collection_type IS NULL
OR l.collection_type NOT IN ('music', 'movies', 'tvshows') OR l.collection_type NOT IN ('music', 'movies', 'tvshows')
)"; )"
);
/// TRACES: UR-055 | DR-082, DR-083 /// TRACES: UR-055 | DR-082, DR-083
const DOWNLOADED_ITEMS_CTE: &'static str = " const DOWNLOADED_ITEMS_CTE: &'static str = "
@@ -1358,14 +1382,43 @@ impl MediaRepository for OfflineRepository {
-- so match every item on the server and let the type filter -- so match every item on the server and let the type filter
-- (e.g. MusicAlbum / Movie / Series) narrow it. This is what -- (e.g. MusicAlbum / Movie / Series) narrow it. This is what
-- makes library landing pages show albums/movies/shows offline. -- makes library landing pages show albums/movies/shows offline.
--
-- The type correlation is NOT optional. Without it this
-- EXISTS never mentions the item, so it is true for every
-- cached row as soon as the requested parent is any library.
-- Music/Movies/TV got away with that because their landing
-- pages pass `include_item_types`, which narrowed the result;
-- the generic library page passes none, so a Books or Photos
-- library served the entire cached server (DR-277).
--
-- `library_id` wins wherever it survived the cache write:
-- it is the server's own answer, and it is the only thing
-- that can scope a library whose type has no mapping (Books,
-- Photos, Collections) or none at all (a mixed library, where
-- Jellyfin sends CollectionType null). The taxonomy is the
-- fallback for rows that predate it being stored.
--
-- A library with neither a stored link nor a mapped type now
-- matches nothing here and falls through to the server, which
-- does know what is in it. Showing nothing briefly beats
-- showing somebody else's films with confidence.
OR EXISTS ( OR EXISTS (
SELECT 1 FROM libraries l SELECT 1 FROM libraries l
WHERE l.id = ? AND l.server_id = i.server_id WHERE l.id = ? AND l.server_id = i.server_id
AND (
i.library_id = l.id
OR (i.library_id IS NULL AND {})
)
) )
){}{} ){}{}
ORDER BY {} ORDER BY {}
LIMIT {} OFFSET {}", LIMIT {} OFFSET {}",
type_filter, favorites_filter, order_by, limit, start_index library_type_matches_item!(),
type_filter,
favorites_filter,
order_by,
limit,
start_index
); );
// The requested id is compared against every hierarchy-linkage column // The requested id is compared against every hierarchy-linkage column
@@ -4587,6 +4640,27 @@ mod tests {
let db_service = create_test_db(); let db_service = create_test_db();
seed_favorites(&db_service).await; seed_favorites(&db_service).await;
// A favourite album *in lib-1*. The fixture's `album-fav` lives in
// lib-2, and this test used to expect it back from a lib-1 listing —
// which only held because the library clause matched every cached row
// regardless of which library it was in (DR-277). The assertions below
// still span both requested types, which is what UT-206 is really about;
// they now do it with an album that is actually in the library.
db_service
.execute(Query::new(
"INSERT INTO items (id, server_id, name, item_type, library_id, synced_at, sort_name) \
VALUES ('album-lib1', 'test-server', 'Album In Lib One', 'MusicAlbum', 'lib-1', '2026-01-01', 'Album In Lib One')",
))
.await
.unwrap();
db_service
.execute(Query::new(
"INSERT INTO user_data (user_id, item_id, is_favorite) VALUES ('test-user', 'album-lib1', 1)",
))
.await
.unwrap();
let repo = OfflineRepository::new( let repo = OfflineRepository::new(
db_service, db_service,
"test-server".to_string(), "test-server".to_string(),
@@ -4605,7 +4679,11 @@ mod tests {
.unwrap(); .unwrap();
let mut ids: Vec<&str> = both.items.iter().map(|i| i.id.as_str()).collect(); let mut ids: Vec<&str> = both.items.iter().map(|i| i.id.as_str()).collect();
ids.sort(); ids.sort();
assert_eq!(ids, vec!["album-fav", "movie-fav", "movie-plain"]); assert_eq!(ids, vec!["album-lib1", "movie-fav", "movie-plain"]);
assert!(
!ids.contains(&"album-fav"),
"album-fav belongs to lib-2 and must not appear in a lib-1 listing"
);
// Two type placeholders *and* the favourites parameter after them. // Two type placeholders *and* the favourites parameter after them.
let favourites = repo let favourites = repo
@@ -4621,7 +4699,7 @@ mod tests {
.unwrap(); .unwrap();
let mut ids: Vec<&str> = favourites.items.iter().map(|i| i.id.as_str()).collect(); let mut ids: Vec<&str> = favourites.items.iter().map(|i| i.id.as_str()).collect();
ids.sort(); ids.sort();
assert_eq!(ids, vec!["album-fav", "movie-fav"]); assert_eq!(ids, vec!["album-lib1", "movie-fav"]);
} }
/// UT-102 — caching a server result mirrors its favourite state locally, /// UT-102 — caching a server result mirrors its favourite state locally,
@@ -4937,4 +5015,93 @@ mod tests {
"a position with no favourite flag must still be mirrored" "a position with no favourite flag must still be mirrored"
); );
} }
/// A library whose `collection_type` is not one of the three the app has
/// landing pages for — Books, Photos, Home Videos, Collections, or a mixed
/// library — must not show the entire server.
///
/// The cache has no item→library link at all (`library_id`/`parent_id` are
/// NULL, see [[offline-libraries-never-cached]]), so `get_items` matched a
/// library parent with an EXISTS that never referenced the item:
///
/// OR EXISTS (SELECT 1 FROM libraries l WHERE l.id = ? AND ...)
///
/// True for every cached row the moment the requested id is any library.
/// The music/movies/TV landing pages got away with it because each passes
/// `include_item_types`, which narrowed the result; the generic library page
/// passes none, so opening a Books library served whatever happened to be
/// cached — films, albums, episodes. Same defect the downloaded listing had
/// in DR-167, in the path nobody re-checked.
///
/// TRACES: UR-007, UR-055 | DR-277 | UT-247
#[tokio::test]
async fn test_get_items_unknown_library_type_does_not_return_whole_server() {
let db = create_test_db();
seed_library(&db, "books-lib", "books").await;
insert_item(&db, "movie-1", "Movie", None, None, None).await;
insert_item(&db, "album-1", "MusicAlbum", None, None, None).await;
insert_item(&db, "series-1", "Series", None, None, None).await;
let repo = make_repo(&db);
let ids: Vec<String> = repo
.get_items("books-lib", None)
.await
.unwrap()
.items
.iter()
.map(|i| i.id.clone())
.collect();
assert!(
!ids.contains(&"movie-1".to_string())
&& !ids.contains(&"album-1".to_string())
&& !ids.contains(&"series-1".to_string()),
"a Books library must not serve the server's films, albums and shows; got {:?}",
ids
);
}
/// The narrowing must not break the libraries that *do* have landing pages:
/// they reach the same query and must keep returning their own media.
///
/// TRACES: UR-007 | DR-277 | UT-248
#[tokio::test]
async fn test_get_items_typed_libraries_still_return_their_own_media() {
let db = create_test_db();
seed_library(&db, "music-lib", "music").await;
seed_library(&db, "movie-lib", "movies").await;
seed_library(&db, "tv-lib", "tvshows").await;
insert_item(&db, "album-1", "MusicAlbum", None, None, None).await;
insert_item(&db, "movie-1", "Movie", None, None, None).await;
insert_item(&db, "series-1", "Series", None, None, None).await;
let repo = make_repo(&db);
for (lib, expected, forbidden) in [
("music-lib", "album-1", "movie-1"),
("movie-lib", "movie-1", "album-1"),
("tv-lib", "series-1", "album-1"),
] {
let ids: Vec<String> = repo
.get_items(lib, None)
.await
.unwrap()
.items
.iter()
.map(|i| i.id.clone())
.collect();
assert!(
ids.contains(&expected.to_string()),
"{lib} should list {expected}; got {:?}",
ids
);
assert!(
!ids.contains(&forbidden.to_string()),
"{lib} must not list {forbidden}; got {:?}",
ids
);
}
}
} }