From d52470e0cd01ba0350017896b09e601489fcd845 Mon Sep 17 00:00:00 2001 From: Duncan Tourolle Date: Thu, 20 Aug 2026 19:56:40 +0200 Subject: [PATCH 1/3] fix(offline): bind item-type filter as query parameters MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit get_items built its `AND i.item_type IN (…)` fragment by interpolating each requested type into the SQL string, while `search`, `get_favorites` and `prune_stale_catalog` in the same file bind the identical filter as `?` placeholders. Follow the existing pattern so the listing query is consistent with its neighbours. The type values bind between the six parent-matching ids and the favourites user id, matching where `{type_filter}` lands in the statement. TRACES: UR-065 | DR-212 | UT-206 --- src-tauri/src/repository/offline.rs | 140 +++++++++++++++++++++++++--- 1 file changed, 127 insertions(+), 13 deletions(-) diff --git a/src-tauri/src/repository/offline.rs b/src-tauri/src/repository/offline.rs index 9c24725c..c8c4a2fa 100644 --- a/src-tauri/src/repository/offline.rs +++ b/src-tauri/src/repository/offline.rs @@ -1245,20 +1245,22 @@ impl MediaRepository for OfflineRepository { _ => "i.sort_name ASC, i.name ASC", }; - // Build type filter for optional filtering - let type_filter = if let Some(include_item_types) = &opts.include_item_types { - if !include_item_types.is_empty() { - let types = include_item_types - .iter() - .map(|t| format!("'{}'", t)) - .collect::>() - .join(","); - format!(" AND i.item_type IN ({})", types) - } else { - String::new() - } - } else { + // Bind the type filter rather than interpolating it: `include_item_types` + // is settable straight from the frontend (GenericMediaListPage passes it), + // so a quote in a type must be data, not syntax. Same shape as `search` + // and `get_favorites`. + // + // TRACES: UR-065 | DR-212 | UT-206 + let type_values: &[String] = opts + .include_item_types + .as_deref() + .filter(|types| !types.is_empty()) + .unwrap_or(&[]); + let type_filter = if type_values.is_empty() { String::new() + } else { + let placeholders = vec!["?"; type_values.len()].join(","); + format!(" AND i.item_type IN ({})", placeholders) }; // Favourites narrowing for a normal library listing. Bound rather than @@ -1350,6 +1352,11 @@ impl MediaRepository for OfflineRepository { QueryParam::String(parent_id.to_string()), // i.series_id = ? QueryParam::String(parent_id.to_string()), // libraries.id = ? ]; + // Positional order matters: the type placeholders sit in `{type_filter}`, + // which the statement interpolates immediately after the parent-matching + // group and before `{favorites_filter}`, so they bind here — after the + // six ids above, before the favourites user id. + params.extend(type_values.iter().cloned().map(QueryParam::String)); if !favorites_filter.is_empty() { params.push(QueryParam::String(self.user_id.clone())); // ud.user_id = ? } @@ -4481,6 +4488,113 @@ mod tests { assert_eq!(ids, vec!["movie-fav"]); } + /// UT-206 — `include_item_types` reaches the listing query as bound + /// parameters, so a type name can only ever be compared as data. + /// + /// Interpolated, the type below closed the `IN (` list and commented out the + /// rest of the line, leaving `... AND i.item_type IN ('Movie') OR 1=1`, which + /// is true for every row — the listing then returned the whole cache + /// regardless of parent or type. Bound, it is just a type name that matches + /// nothing. + /// + /// TRACES: UR-065 | DR-212 | UT-206 + #[tokio::test] + async fn test_get_items_type_filter_is_bound_not_interpolated() { + let _guard = lock_catalog_browse(); + set_include_catalog_browse(true); + + let db_service = create_test_db(); + seed_favorites(&db_service).await; + let repo = OfflineRepository::new( + db_service, + "test-server".to_string(), + "test-user".to_string(), + ); + + let injected = repo + .get_items( + "lib-1", + Some(GetItemsOptions { + include_item_types: Some(vec!["Movie') OR 1=1 --".to_string()]), + ..Default::default() + }), + ) + .await + .expect("a hostile type name must be data, not a broken query"); + assert!( + injected.items.is_empty(), + "no cached item has that type, so nothing may come back; got {:?}", + injected + .items + .iter() + .map(|i| i.id.as_str()) + .collect::>() + ); + + // A quote on its own is likewise just a character in a type name. + let quoted = repo + .get_items( + "lib-1", + Some(GetItemsOptions { + include_item_types: Some(vec!["Mo'vie".to_string()]), + ..Default::default() + }), + ) + .await + .expect("an embedded quote must not break the query"); + assert!(quoted.items.is_empty()); + } + + /// UT-206 — binding the type filter must not disturb the positions of the + /// parameters around it: the parent ids bind before it and the favourites + /// user id after it. A misordered vec would silently compare `user_id` + /// against `item_type`, so this asserts the filters still compose. + /// + /// TRACES: UR-065, UR-067 | DR-212 | UT-206 + #[tokio::test] + async fn test_get_items_binds_multiple_types_in_parameter_order() { + let _guard = lock_catalog_browse(); + set_include_catalog_browse(true); + + let db_service = create_test_db(); + seed_favorites(&db_service).await; + let repo = OfflineRepository::new( + db_service, + "test-server".to_string(), + "test-user".to_string(), + ); + + let both = repo + .get_items( + "lib-1", + Some(GetItemsOptions { + include_item_types: Some(vec!["Movie".to_string(), "MusicAlbum".to_string()]), + ..Default::default() + }), + ) + .await + .unwrap(); + let mut ids: Vec<&str> = both.items.iter().map(|i| i.id.as_str()).collect(); + ids.sort(); + assert_eq!(ids, vec!["album-fav", "movie-fav", "movie-plain"]); + + // Two type placeholders *and* the favourites parameter after them. + let favourites = repo + .get_items( + "lib-1", + Some(GetItemsOptions { + include_item_types: Some(vec!["Movie".to_string(), "MusicAlbum".to_string()]), + favorites_only: Some(true), + ..Default::default() + }), + ) + .await + .unwrap(); + let mut ids: Vec<&str> = favourites.items.iter().map(|i| i.id.as_str()).collect(); + ids.sort(); + assert_eq!(ids, vec!["album-fav", "movie-fav"]); + } + /// UT-102 — caching a server result mirrors its favourite state locally, /// but never over a row still waiting to be pushed. /// From 6b7ce512eddbcc3f31e1e9778fd0fa32e702f47f Mon Sep 17 00:00:00 2001 From: Duncan Tourolle Date: Thu, 20 Aug 2026 19:59:25 +0200 Subject: [PATCH 2/3] fix(online): percent-encode query values and path ids build_get_items_endpoint pasted ParentId, IncludeItemTypes, SortBy and SortOrder straight into the query string while the Genres parameter twenty lines below and the SearchTerm parameter both percent-encode theirs. Encode them the same way, per list element so the commas Jellyfin splits on survive. The per-call ids interpolated into request paths (item, person and playlist ids) get the same treatment; a Jellyfin GUID is unchanged by encoding, so this is consistency, not a behaviour change. self.user_id is left alone throughout, as it is at the endpoint builders already. TRACES: UR-007 | DR-212 | UT-206 --- src-tauri/src/repository/online.rs | 168 +++++++++++++++++++++++++---- 1 file changed, 149 insertions(+), 19 deletions(-) diff --git a/src-tauri/src/repository/online.rs b/src-tauri/src/repository/online.rs index 291242b4..44a557d9 100644 --- a/src-tauri/src/repository/online.rs +++ b/src-tauri/src/repository/online.rs @@ -235,7 +235,11 @@ impl OnlineRepository { item_id: &str, t: f64, ) -> Result, RepoError> { - let endpoint = format!("/Plugins/JRay/Items/{}/jray?t={}", item_id, t); + let endpoint = format!( + "/Plugins/JRay/Items/{}/jray?t={}", + urlencoding::encode(item_id), + t + ); match self.get_json::(&endpoint).await { Ok(context) => Ok(context.actors), // No plugin / no truth data for this item — not an error to the user. @@ -802,7 +806,17 @@ fn build_get_items_endpoint( parent_id: &str, options: Option<&GetItemsOptions>, ) -> String { - let mut endpoint = format!("/Users/{}/Items?ParentId={}", user_id, parent_id); + // Every value below is percent-encoded before it goes into the query + // string, the same way `Genres` and `SearchTerm` already are: these are + // values, not URL syntax, so a space or an `&` in one must not split it + // into another parameter. + // + // TRACES: UR-007 | DR-212 | UT-206 + let mut endpoint = format!( + "/Users/{}/Items?ParentId={}", + user_id, + urlencoding::encode(parent_id) + ); if let Some(opts) = options { if let Some(limit) = opts.limit { @@ -812,13 +826,25 @@ fn build_get_items_endpoint( endpoint.push_str(&format!("&StartIndex={}", start_index)); } if let Some(types) = &opts.include_item_types { - endpoint.push_str(&format!("&IncludeItemTypes={}", types.join(","))); + // Encode each type, not the joined string: the comma is the + // list separator Jellyfin splits on. + let encoded: Vec = types + .iter() + .map(|t| urlencoding::encode(t).into_owned()) + .collect(); + endpoint.push_str(&format!("&IncludeItemTypes={}", encoded.join(","))); } if let Some(sort_by) = &opts.sort_by { - endpoint.push_str(&format!("&SortBy={}", sort_by)); + // SortBy is likewise a comma-delimited list (`hybrid.rs` sends + // "ParentIndexNumber,IndexNumber,SortName"), so encode per field. + let encoded: Vec = sort_by + .split(',') + .map(|field| urlencoding::encode(field).into_owned()) + .collect(); + endpoint.push_str(&format!("&SortBy={}", encoded.join(","))); } if let Some(sort_order) = &opts.sort_order { - endpoint.push_str(&format!("&SortOrder={}", sort_order)); + endpoint.push_str(&format!("&SortOrder={}", urlencoding::encode(sort_order))); } if let Some(recursive) = opts.recursive { endpoint.push_str(&format!("&Recursive={}", recursive)); @@ -1147,7 +1173,7 @@ impl MediaRepository for OnlineRepository { /// /// TRACES: UR-021, UR-035 | IR-016, IR-022, JA-005, JA-009 async fn get_item(&self, item_id: &str) -> Result { - let endpoint = format!("/Users/{}/Items/{}?Fields=BackdropImageTags,ParentBackdropImageTags,People,MediaStreams,MediaSources,PremiereDate,UserData", self.user_id, item_id); + let endpoint = format!("/Users/{}/Items/{}?Fields=BackdropImageTags,ParentBackdropImageTags,People,MediaStreams,MediaSources,PremiereDate,UserData", self.user_id, urlencoding::encode(item_id)); let item: JellyfinItem = self.get_json(&endpoint).await?; let media_item = item.into_media_item(self.user_id.clone()); @@ -1510,7 +1536,7 @@ impl MediaRepository for OnlineRepository { } async fn get_playback_info(&self, item_id: &str) -> Result { - let endpoint = format!("/Items/{}/PlaybackInfo", item_id); + let endpoint = format!("/Items/{}/PlaybackInfo", urlencoding::encode(item_id)); #[derive(Debug, Serialize)] #[serde(rename_all = "PascalCase")] @@ -1939,7 +1965,7 @@ impl MediaRepository for OnlineRepository { live_stream_id: Option, } - let endpoint = format!("/Items/{}/PlaybackInfo", item_id); + let endpoint = format!("/Items/{}/PlaybackInfo", urlencoding::encode(item_id)); let request = OpenLiveStreamRequest { user_id: self.user_id.clone(), auto_open_live_stream: true, @@ -2214,7 +2240,11 @@ impl MediaRepository for OnlineRepository { } async fn mark_favorite(&self, item_id: &str) -> Result<(), RepoError> { - let endpoint = format!("/Users/{}/FavoriteItems/{}", self.user_id, item_id); + let endpoint = format!( + "/Users/{}/FavoriteItems/{}", + self.user_id, + urlencoding::encode(item_id) + ); self.post_json(&endpoint, &serde_json::json!({})).await } @@ -2244,7 +2274,11 @@ impl MediaRepository for OnlineRepository { /// /// TRACES: UR-017 | JA-018, DR-021 async fn unmark_favorite(&self, item_id: &str) -> Result<(), RepoError> { - let endpoint = format!("/Users/{}/FavoriteItems/{}", self.user_id, item_id); + let endpoint = format!( + "/Users/{}/FavoriteItems/{}", + self.user_id, + urlencoding::encode(item_id) + ); let url = format!("{}{}", self.server_url, endpoint); let result = async { @@ -2286,7 +2320,11 @@ impl MediaRepository for OnlineRepository { /// /// TRACES: UR-064 | DR-106, JA-033 async fn clear_watch_history(&self, item_id: &str) -> Result<(), RepoError> { - let endpoint = format!("/Users/{}/PlayedItems/{}", self.user_id, item_id); + let endpoint = format!( + "/Users/{}/PlayedItems/{}", + self.user_id, + urlencoding::encode(item_id) + ); let url = format!("{}{}", self.server_url, endpoint); let result = async { @@ -2327,7 +2365,11 @@ impl MediaRepository for OnlineRepository { /// /// TRACES: UR-025 | DR-131 | JA-035 async fn mark_played(&self, item_id: &str) -> Result<(), RepoError> { - let endpoint = format!("/Users/{}/PlayedItems/{}", self.user_id, item_id); + let endpoint = format!( + "/Users/{}/PlayedItems/{}", + self.user_id, + urlencoding::encode(item_id) + ); let url = format!("{}{}", self.server_url, endpoint); let result = async { @@ -2372,7 +2414,11 @@ impl MediaRepository for OnlineRepository { /// /// TRACES: UR-035, UR-036 | IR-022, JA-030 async fn get_person(&self, person_id: &str) -> Result { - let endpoint = format!("/Users/{}/Items/{}", self.user_id, person_id); + let endpoint = format!( + "/Users/{}/Items/{}", + self.user_id, + urlencoding::encode(person_id) + ); let item: JellyfinItem = self.get_json(&endpoint).await?; Ok(item.into_media_item(self.user_id.clone())) } @@ -2461,7 +2507,7 @@ impl MediaRepository for OnlineRepository { async fn delete_playlist(&self, playlist_id: &str) -> Result<(), RepoError> { info!("[OnlineRepo] Deleting playlist {}", playlist_id); - let endpoint = format!("/Items/{}", playlist_id); + let endpoint = format!("/Items/{}", urlencoding::encode(playlist_id)); let url = format!("{}{}", self.server_url, endpoint); let request = self @@ -2496,7 +2542,7 @@ impl MediaRepository for OnlineRepository { "[OnlineRepo] Renaming playlist {} to '{}'", playlist_id, name ); - let endpoint = format!("/Items/{}", playlist_id); + let endpoint = format!("/Items/{}", urlencoding::encode(playlist_id)); self.post_json(&endpoint, &serde_json::json!({ "Name": name })) .await } @@ -2534,8 +2580,17 @@ impl MediaRepository for OnlineRepository { item_ids.len(), playlist_id ); - let ids_param = item_ids.join(","); - let endpoint = format!("/Playlists/{}/Items?Ids={}", playlist_id, ids_param); + // Encode each id, not the joined string: the comma separates the list. + let ids_param = item_ids + .iter() + .map(|id| urlencoding::encode(id).into_owned()) + .collect::>() + .join(","); + let endpoint = format!( + "/Playlists/{}/Items?Ids={}", + urlencoding::encode(playlist_id), + ids_param + ); self.post_json(&endpoint, &serde_json::json!({})).await } @@ -2549,8 +2604,16 @@ impl MediaRepository for OnlineRepository { entry_ids.len(), playlist_id ); - let ids_param = entry_ids.join(","); - let endpoint = format!("/Playlists/{}/Items?EntryIds={}", playlist_id, ids_param); + let ids_param = entry_ids + .iter() + .map(|id| urlencoding::encode(id).into_owned()) + .collect::>() + .join(","); + let endpoint = format!( + "/Playlists/{}/Items?EntryIds={}", + urlencoding::encode(playlist_id), + ids_param + ); let url = format!("{}{}", self.server_url, endpoint); let request = self @@ -3529,6 +3592,73 @@ mod tests { assert!(!off.contains("Filters=IsFavorite")); } + /// UT-206 — the values this endpoint builder puts in the query string are + /// percent-encoded, like `Genres` and `SearchTerm` already are. + /// + /// Unencoded, a value carrying `&` or `=` splits into an extra query + /// parameter (a parent id containing a space produced a malformed URL + /// outright), so the request the server sees is not the one that was built. + /// + /// TRACES: UR-007 | DR-212 | UT-206 + #[test] + fn test_get_items_endpoint_encodes_query_values() { + let endpoint = build_get_items_endpoint( + "u1", + "lib 1&Filters=IsFavorite", + Some(&GetItemsOptions { + include_item_types: Some(vec!["Movie&x=1".to_string()]), + sort_by: Some("Sort Name".to_string()), + sort_order: Some("Ascending&y=2".to_string()), + ..Default::default() + }), + ); + assert!( + endpoint.contains("ParentId=lib%201%26Filters%3DIsFavorite"), + "{endpoint}" + ); + assert!( + endpoint.contains("&IncludeItemTypes=Movie%26x%3D1"), + "{endpoint}" + ); + assert!(endpoint.contains("&SortBy=Sort%20Name"), "{endpoint}"); + assert!( + endpoint.contains("&SortOrder=Ascending%26y%3D2"), + "{endpoint}" + ); + // Nothing smuggled in as a parameter of its own. + assert!(!endpoint.contains("&Filters=IsFavorite"), "{endpoint}"); + assert!(!endpoint.contains("&x=1"), "{endpoint}"); + assert!(!endpoint.contains("&y=2"), "{endpoint}"); + } + + /// The separators inside a list parameter must survive encoding: Jellyfin + /// splits `SortBy` and `IncludeItemTypes` on commas, and `hybrid.rs` sends + /// "ParentIndexNumber,IndexNumber,SortName" to order episodes. + /// + /// TRACES: UR-007 | DR-212 | UT-206 + #[test] + fn test_get_items_endpoint_keeps_list_separators() { + let endpoint = build_get_items_endpoint( + "u1", + "lib-1", + Some(&GetItemsOptions { + sort_by: Some("ParentIndexNumber,IndexNumber,SortName".to_string()), + include_item_types: Some(vec!["Movie".to_string(), "Series".to_string()]), + ..Default::default() + }), + ); + assert!( + endpoint.contains("&SortBy=ParentIndexNumber,IndexNumber,SortName"), + "{endpoint}" + ); + assert!( + endpoint.contains("&IncludeItemTypes=Movie,Series"), + "{endpoint}" + ); + // A plain GUID parent id is unchanged by encoding. + assert!(endpoint.contains("ParentId=lib-1"), "{endpoint}"); + } + /// A newly-added album must arrive as one entry, not one per track. /// /// Jellyfin's `/Items/Latest` defaults to `GroupItems=false`, which returns From 080cdbf383a932b42bf50065ec4e9ca7fbf93552 Mon Sep 17 00:00:00 2001 From: Duncan Tourolle Date: Thu, 20 Aug 2026 20:00:35 +0200 Subject: [PATCH 3/3] fix(player): clamp volume at the command boundary player_set_volume passed `volume` through untouched. Each backend clamps to 0.0..=1.0 for itself, so local playback was already safe, but the remote branch reaches no backend: it converts with `(volume * 100.0) as i32`, which turns infinity into i32::MAX. NaN is handled explicitly since f32::clamp returns NaN for a NaN input and it then survives every comparison downstream. TRACES: DR-212 | UT-206 --- src-tauri/src/commands/player/mod.rs | 61 ++++++++++++++++++++++++++++ 1 file changed, 61 insertions(+) diff --git a/src-tauri/src/commands/player/mod.rs b/src-tauri/src/commands/player/mod.rs index 7e100d79..98fb636f 100644 --- a/src-tauri/src/commands/player/mod.rs +++ b/src-tauri/src/commands/player/mod.rs @@ -1705,6 +1705,23 @@ pub async fn player_set_subtitle_track( Ok(get_player_status(&controller)) } +/// Normalise a volume arriving over IPC to the 0.0..=1.0 range every backend +/// works in. +/// +/// NaN is handled before the clamp rather than by it: `f32::clamp` returns NaN +/// for a NaN input (it only panics on NaN *bounds*), and NaN then survives every +/// comparison downstream, so a backend clamp cannot catch it either. It is +/// treated as "no volume asked for" and floored to 0.0. +/// +/// TRACES: DR-212 | UT-206 +fn normalize_volume(volume: f32) -> f32 { + if volume.is_nan() { + 0.0 + } else { + volume.clamp(0.0, 1.0) + } +} + #[tauri::command] #[specta::specta] pub async fn player_set_volume( @@ -1712,6 +1729,12 @@ pub async fn player_set_volume( playback_mode: State<'_, super::playback_mode::PlaybackModeManagerWrapper>, volume: f32, ) -> Result { + // Clamp at the boundary as well as in each backend: the remote branch below + // never reaches a backend clamp, and `(f32::INFINITY * 100.0) as i32` would + // hand the server i32::MAX as a volume percentage. + // TRACES: DR-212 | UT-206 + let volume = normalize_volume(volume); + // Check if we're in remote mode let mode = playback_mode.0.get_mode(); @@ -2769,6 +2792,44 @@ pub async fn player_disable_jellyfin(player: State<'_, PlayerStateWrapper>) -> R mod tests { use crate::utils::lock::MutexSafe; + /// UT-206 — the volume the command hands on is always a real number in + /// 0.0..=1.0. + /// + /// Every backend clamps for itself, but the remote branch of + /// `player_set_volume` reaches no backend at all: it does + /// `(volume * 100.0) as i32`, which turns infinity into `i32::MAX` and NaN + /// into 0. NaN also survives `f32::clamp` unchanged, so clamping alone is + /// not enough — it has to be tested for. + /// + /// TRACES: DR-212 | UT-206 + #[test] + fn test_normalize_volume_clamps_and_rejects_nan() { + use super::normalize_volume; + + // In-range values pass through untouched. + assert_eq!(normalize_volume(0.0), 0.0); + assert_eq!(normalize_volume(0.5), 0.5); + assert_eq!(normalize_volume(1.0), 1.0); + + // Out of range clamps to the same 0.0..=1.0 the backends use. + assert_eq!(normalize_volume(-0.5), 0.0); + assert_eq!(normalize_volume(42.0), 1.0); + assert_eq!(normalize_volume(f32::INFINITY), 1.0); + assert_eq!(normalize_volume(f32::NEG_INFINITY), 0.0); + + // NaN is not a volume; it must not reach the Jellyfin percentage + // conversion or a backend. + let from_nan = normalize_volume(f32::NAN); + assert!(!from_nan.is_nan(), "NaN must not pass through the boundary"); + assert_eq!(from_nan, 0.0); + + // Whatever comes out survives the remote branch's percentage cast. + for input in [-1.0, 0.25, 9.0, f32::INFINITY, f32::NAN] { + let percent = (normalize_volume(input) * 100.0) as i32; + assert!((0..=100).contains(&percent), "input {input} gave {percent}"); + } + } + /// The subtitle list the frontend resolved must survive the IPC hop and end /// up on the `MediaItem` the native backend loads. ///