From e12f0065a6739c5f53c9dc05fabb828cebfdc0d6 Mon Sep 17 00:00:00 2001 From: Duncan Tourolle Date: Thu, 20 Aug 2026 19:56:23 +0200 Subject: [PATCH] fix(thumbnails): confine cache writes to the cache directory The thumbnail cache built its filename from `item_id`, `image_type` and `tag`, but only sanitised the tag. `Path::join` neither folds `..` nor keeps its base when handed an absolute path, so a malformed id could place a cache write outside the cache directory. Sanitise all three parts through one helper using the rule the tag already used (non-alphanumerics become `_`), so ids and types that were already safe keep producing exactly the same filename, and resolve the result against the cache dir with a lexical `..` fold plus a `starts_with` check, modelled on `media_server::resolve_path`. The database still stores the raw key and the resolved path, so the lookup in `get_cached_path` keeps matching what the caller asks for. --- src-tauri/src/thumbnail/cache.rs | 226 ++++++++++++++++++++++++++++++- 1 file changed, 221 insertions(+), 5 deletions(-) diff --git a/src-tauri/src/thumbnail/cache.rs b/src-tauri/src/thumbnail/cache.rs index a91b320e..e170663b 100644 --- a/src-tauri/src/thumbnail/cache.rs +++ b/src-tauri/src/thumbnail/cache.rs @@ -1,7 +1,7 @@ //! Thumbnail cache manager with LRU eviction use log::error; -use std::path::PathBuf; +use std::path::{Path, PathBuf}; use std::sync::Arc; use std::sync::Mutex; @@ -28,6 +28,19 @@ impl Default for CacheConfig { } } +/// Make one part of a cache filename safe to put in a path. +/// +/// Every part of the name comes from the caller — the item id and image type are +/// taken verbatim from Jellyfin JSON — so none of them may contribute a path +/// separator or a `..`. The rule is the one the image tag has always used +/// (non-alphanumerics become `_`), applied to all three parts, so values that +/// were already safe keep producing exactly the filename they did before. +/// +/// TRACES: | DR-210 | UT-204 +fn safe_component(value: &str) -> String { + value.replace(|c: char| !c.is_alphanumeric(), "_") +} + /// Thumbnail cache with LRU eviction pub struct ThumbnailCache { config: Arc>, @@ -50,6 +63,34 @@ impl ThumbnailCache { } } + /// Resolve a cache filename against the cache directory, refusing anything + /// that lands outside it. + /// + /// `..` is folded away lexically rather than through `canonicalize`, so a + /// file that does not exist yet still resolves — the same approach as + /// `media_server::resolve_path`. `safe_component` should already have made an + /// escape impossible; this is the check at the point of use. + /// + /// TRACES: | DR-210 | UT-204 + fn resolve_in_cache_dir(&self, filename: &str) -> Result { + let mut resolved = self.cache_dir.clone(); + for part in Path::new(filename).components() { + match part { + std::path::Component::ParentDir => { + resolved.pop(); + } + std::path::Component::CurDir => {} + other => resolved.push(other), + } + } + + if resolved.starts_with(&self.cache_dir) { + Ok(resolved) + } else { + Err("Thumbnail path escapes the cache directory".to_string()) + } + } + /// Check if caching is enabled pub fn is_enabled(&self) -> bool { self.config.lock().map(|c| c.enabled).unwrap_or(true) @@ -155,6 +196,8 @@ impl ThumbnailCache { } /// Save thumbnail to cache + /// + /// TRACES: | DR-210 | UT-204 // The arguments are the cache key (item/type/tag) plus the payload and its // dimensions — all independent scalars borrowed from the caller. A parameter // struct would only move the same list one level down. @@ -173,10 +216,16 @@ impl ThumbnailCache { return Err("Thumbnail caching is disabled".to_string()); } - // Generate safe filename - let safe_tag = tag.replace(|c: char| !c.is_alphanumeric(), "_"); - let filename = format!("{}_{}_{}.jpg", item_id, image_type, safe_tag); - let file_path = self.cache_dir.join(&filename); + // Generate safe filename. The database keeps the *raw* key below, so the + // lookup in `get_cached_path` still matches what the caller asks for; only + // the on-disk name is sanitised, and the row records where it landed. + let filename = format!( + "{}_{}_{}.jpg", + safe_component(item_id), + safe_component(image_type), + safe_component(tag) + ); + let file_path = self.resolve_in_cache_dir(&filename)?; // Ensure we have space (evict LRU items if needed) self.ensure_space(db.clone(), data.len() as u64).await?; @@ -571,4 +620,171 @@ mod tests { let size = cache.get_cache_size(conn.clone()).await; assert!(size <= 60); } + + /// A traversal-style `item_id` must not steer a cache write out of the cache + /// directory. The id reaches `save_thumbnail` verbatim from Jellyfin JSON, so + /// it is not ours to trust. + /// + /// TRACES: | DR-210 | UT-204 + #[tokio::test] + async fn test_save_thumbnail_confines_traversal_item_id() { + let (conn, temp_dir) = setup_test_db(); + let cache = ThumbnailCache::new(temp_dir.path().to_path_buf(), CacheConfig::default()); + + let result = cache + .save_thumbnail( + conn.clone(), + "../evil", + "Primary", + "tag1", + b"fake image data", + None, + None, + ) + .await; + + // Where `../evil` lands if `..` is honoured: the cache dir's parent. + let escaped = temp_dir.path().join("evil_Primary_tag1.jpg"); + assert!( + !escaped.exists(), + "wrote outside the cache directory: {}", + escaped.display() + ); + + // Refusing is acceptable; succeeding is too, as long as it stayed inside. + if let Ok(path) = result { + assert!( + path.starts_with(&cache.cache_dir) && !path.to_string_lossy().contains(".."), + "returned a path outside the cache directory: {}", + path.display() + ); + assert!(path.exists()); + } + } + + /// `Path::join` discards the base when handed an absolute path, so an + /// absolute `item_id` would otherwise pick the write location outright. + /// + /// TRACES: | DR-210 | UT-204 + #[tokio::test] + async fn test_save_thumbnail_confines_absolute_item_id() { + let (conn, temp_dir) = setup_test_db(); + let outside = TempDir::new().unwrap(); + let cache = ThumbnailCache::new(temp_dir.path().to_path_buf(), CacheConfig::default()); + + let absolute_id = outside.path().join("evil").to_string_lossy().to_string(); + let result = cache + .save_thumbnail( + conn.clone(), + &absolute_id, + "Primary", + "tag1", + b"fake image data", + None, + None, + ) + .await; + + let escaped = outside.path().join("evil_Primary_tag1.jpg"); + assert!( + !escaped.exists(), + "wrote outside the cache directory: {}", + escaped.display() + ); + + if let Ok(path) = result { + assert!( + path.starts_with(&cache.cache_dir), + "returned a path outside the cache directory: {}", + path.display() + ); + } + } + + /// `image_type` is equally unsanitised, and equally caller-supplied. + /// + /// TRACES: | DR-210 | UT-204 + #[tokio::test] + async fn test_save_thumbnail_confines_traversal_image_type() { + let (conn, temp_dir) = setup_test_db(); + let cache = ThumbnailCache::new(temp_dir.path().to_path_buf(), CacheConfig::default()); + + let path = cache + .save_thumbnail( + conn.clone(), + "item1", + "../Primary", + "tag1", + b"fake image data", + None, + None, + ) + .await + .expect("a malformed image_type should be sanitised, not break caching"); + + // The file belongs directly in the cache dir — no separator from the + // image type may survive into the filename. + assert_eq!(path.parent(), Some(cache.cache_dir.as_path())); + assert!(path.exists()); + } + + /// Ids, types and tags that were already filesystem-safe — the overwhelming + /// majority — keep producing exactly the filename they did before, so + /// sanitising does not orphan existing cache entries. + /// + /// TRACES: | DR-210 | UT-204 + #[tokio::test] + async fn test_save_thumbnail_filename_unchanged_for_safe_values() { + let (conn, temp_dir) = setup_test_db(); + let cache = ThumbnailCache::new(temp_dir.path().to_path_buf(), CacheConfig::default()); + + let path = cache + .save_thumbnail( + conn.clone(), + "a1b2c3d4e5f60718293a4b5c6d7e8f90", + "Primary", + "abcdef0123456789", + b"fake image data", + None, + None, + ) + .await + .unwrap(); + + assert_eq!( + path, + cache + .cache_dir + .join("a1b2c3d4e5f60718293a4b5c6d7e8f90_Primary_abcdef0123456789.jpg") + ); + } + + /// Sanitising the filename must not desynchronise the write path from the + /// read path: the database keeps the raw key and the resolved path, so a + /// lookup after a save still finds the file that was written. + /// + /// TRACES: | DR-210 | UT-204 + #[tokio::test] + async fn test_traversal_item_id_still_round_trips() { + let (conn, temp_dir) = setup_test_db(); + let cache = ThumbnailCache::new(temp_dir.path().to_path_buf(), CacheConfig::default()); + + let saved = cache + .save_thumbnail( + conn.clone(), + "../evil", + "Primary", + "tag1", + b"fake image data", + None, + None, + ) + .await + .unwrap(); + + let cached = cache + .get_cached_path(conn.clone(), "../evil", "Primary", "tag1") + .await; + assert_eq!(cached, Some(saved)); + } }