From 56c2d8d9018a32c40b8c83eb5c5e8b3c79399f60 Mon Sep 17 00:00:00 2001 From: Erwan Leboucher Date: Thu, 1 Oct 2026 21:50:37 +0200 Subject: [PATCH] refactor(media): Simplify preview image handling --- Cargo.lock | 20 +--- changelog.d/2308.bugfix | 2 +- src/service/Cargo.toml | 5 - src/service/media/preview.rs | 41 +++---- src/service/media/tests.rs | 213 ----------------------------------- 5 files changed, 16 insertions(+), 265 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index c34032c5a..449d1cc25 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1137,7 +1137,6 @@ dependencies = [ "askama", "assign", "async-trait", - "axum", "base64 0.23.1", "bytes", "conduwuit_core", @@ -1168,7 +1167,6 @@ dependencies = [ "resolvematrix", "ruma", "ruminuwuity", - "rustls", "rustyline-async", "sd-notify", "serde", @@ -1176,7 +1174,6 @@ dependencies = [ "serde_json", "serde_urlencoded", "sha2 0.11.0", - "tempfile", "termimad", "tokio", "tracing", @@ -3794,7 +3791,7 @@ version = "5.0.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "51e219e79014df21a225b1860a479e2dcd7cbd9130f4defd4bd0e191ea31d67d" dependencies = [ - "base64 0.22.1", + "base64 0.21.7", "chrono", "getrandom 0.2.17", "http", @@ -5585,7 +5582,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "b8050abb251097357e24aff63ba2c52a6309ecb7d23a5474023df960a02694d8" dependencies = [ "annotate-snippets", - "base64 0.23.1", + "base64 0.21.7", "encoding_rs_io", "granit-parser", "nohash-hasher", @@ -6085,19 +6082,6 @@ version = "0.2.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "7b2093cf4c8eb1e67749a6762251bc9cd836b6fc171623bd0a9d324d37af2417" -[[package]] -name = "tempfile" -version = "3.27.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "32497e9a4c7b38532efcdebeef879707aa9f794296a4f0244f6f69e9bc8574bd" -dependencies = [ - "fastrand", - "getrandom 0.4.3", - "once_cell", - "rustix", - "windows-sys 0.61.2", -] - [[package]] name = "tendril" version = "0.4.3" diff --git a/changelog.d/2308.bugfix b/changelog.d/2308.bugfix index 70e19e733..92bfc74c0 100644 --- a/changelog.d/2308.bugfix +++ b/changelog.d/2308.bugfix @@ -1 +1 @@ -Fixed missing MIME types on URL preview images and rejected failed or invalid image downloads. Contributed by @eleboucher. +Fixed URL preview image MIME types and rejected failed downloads. Contributed by @eleboucher. diff --git a/src/service/Cargo.toml b/src/service/Cargo.toml index 6e04b166a..60a362608 100644 --- a/src/service/Cargo.toml +++ b/src/service/Cargo.toml @@ -124,11 +124,6 @@ serde_urlencoded.workspace = true openidconnect.workspace = true rand.workspace = true -[dev-dependencies] -axum.workspace = true -rustls = { workspace = true, features = ["ring"] } -tempfile = "3" - [target.'cfg(all(unix, target_os = "linux"))'.dependencies] sd-notify.workspace = true sd-notify.optional = true diff --git a/src/service/media/preview.rs b/src/service/media/preview.rs index 967ba2443..ac10fa40c 100644 --- a/src/service/media/preview.rs +++ b/src/service/media/preview.rs @@ -155,32 +155,19 @@ impl Service { ) .await?; - let needs_dimensions = - preview_data.image_width.is_none() || preview_data.image_height.is_none(); - let (image, content_type, width, height) = self - .services - .server - .runtime() - .spawn_blocking(move || -> Result<_> { - let reader = - ImageReader::new(std::io::Cursor::new(&image)).with_guessed_format()?; - let content_type = reader - .format() - .ok_or_else(|| { - err!(Request(Unknown("Downloaded file is not a supported image"))) - })? - .to_mime_type(); - let (width, height) = if needs_dimensions { - let (width, height) = reader.into_dimensions().map_err(|error| { - err!(Request(Unknown("Failed to read preview image dimensions: {error}"))) - })?; - (Some(width), Some(height)) - } else { - (None, None) - }; - Ok((image, content_type, width, height)) - }) - .await??; + let reader = ImageReader::new(std::io::Cursor::new(&image)).with_guessed_format()?; + let content_type = reader + .format() + .ok_or_else(|| err!(Request(Unknown("Downloaded file is not a supported image"))))? + .to_mime_type(); + + if preview_data.image_height.is_none() || preview_data.image_width.is_none() { + let (width, height) = reader.into_dimensions().map_err(|error| { + err!(Request(Unknown("Failed to read preview image dimensions: {error}"))) + })?; + preview_data.image_width.get_or_insert(width); + preview_data.image_height.get_or_insert(height); + } let mxc = Mxc { server_name: self.services.globals.server_name(), @@ -191,8 +178,6 @@ impl Service { .await?; preview_data.image = Some(mxc.to_string()); - preview_data.image_width = preview_data.image_width.or(width); - preview_data.image_height = preview_data.image_height.or(height); Ok(preview_data) } diff --git a/src/service/media/tests.rs b/src/service/media/tests.rs index 2987938ab..651e0ade3 100644 --- a/src/service/media/tests.rs +++ b/src/service/media/tests.rs @@ -1,218 +1,5 @@ #![cfg(test)] -#[cfg(feature = "url_preview")] -mod url_preview { - use std::{io::Cursor, sync::Arc}; - - use axum::{Router, body::Body, http::StatusCode, routing::get}; - use conduwuit::{ - Config, Server, - log::{Log, LogLevelReloadHandles}, - }; - use image::{DynamicImage, ImageFormat}; - use serde_json::json; - use tokio::net::TcpListener; - - use crate::{ - Services, - media::{mxc::Mxc, preview::UrlPreviewData}, - }; - - async fn services() -> (tempfile::TempDir, Arc) { - _ = rustls::crypto::ring::default_provider().install_default(); - let directory = tempfile::tempdir().unwrap(); - let config: Config = serde_json::from_value(json!({ - "server_name": "example.com", - "database_path": directory.path(), - "db_pool_affinity": false, - "db_pool_workers": 1, - "rocksdb_parallelism_threads": 1, - "db_cache_capacity_mb": 8, - "db_write_buffer_capacity_mb": 8, - "max_request_size": 4096, - })) - .unwrap(); - let server = - Arc::new(Server::new(config, Some(tokio::runtime::Handle::current()), Log { - reload: LogLevelReloadHandles::default(), - capture: Arc::default(), - })); - let services = Services::build(server).await.unwrap(); - services.media.create_media_dir().await.unwrap(); - (directory, services) - } - - fn image(format: ImageFormat) -> Vec { - let mut bytes = Cursor::new(Vec::new()); - DynamicImage::new_rgb8(2, 3) - .write_to(&mut bytes, format) - .unwrap(); - bytes.into_inner() - } - - struct ImageServer { - url: String, - task: tokio::task::JoinHandle<()>, - } - - impl ImageServer { - async fn new( - services: &Services, - status: StatusCode, - content_type: Option<&str>, - body: Vec, - ) -> Self { - let listener = TcpListener::bind("127.0.0.1:0").await.unwrap(); - let url = format!("http://{}/image", listener.local_addr().unwrap()); - let mut response = http::Response::builder().status(status); - if let Some(content_type) = content_type { - response = response.header(http::header::CONTENT_TYPE, content_type); - } - let response = response.body(body).unwrap(); - let app = Router::new().route( - "/image", - get(move || { - let response = response.clone(); - async move { response.map(Body::from) } - }), - ); - let task = services - .server - .runtime() - .spawn(async move { axum::serve(listener, app).await.unwrap() }); - Self { url, task } - } - } - - impl Drop for ImageServer { - fn drop(&mut self) { self.task.abort(); } - } - - #[tokio::test] - async fn preview_images_store_mime_type_and_dimensions() { - let (_directory, services) = services().await; - for format in [ImageFormat::Png, ImageFormat::Jpeg, ImageFormat::Gif, ImageFormat::WebP] { - for content_type in [ - Some(format.to_mime_type()), - None, - Some("application/octet-stream"), - Some("image/svg+xml"), - ] { - let bytes = image(format); - let server = - ImageServer::new(&services, StatusCode::OK, content_type, bytes.clone()) - .await; - let preview = services - .media - .download_image( - &server.url, - Some(UrlPreviewData { - title: Some("Example preview".into()), - ..UrlPreviewData::default() - }), - ) - .await - .unwrap(); - assert_eq!((preview.image_width, preview.image_height), (Some(2), Some(3))); - assert_eq!(preview.title.as_deref(), Some("Example preview")); - let uri = preview.image.unwrap(); - let mxc = Mxc::try_from(uri.as_str()).unwrap(); - let stored = services.media.get(&mxc).await.unwrap().unwrap(); - assert_eq!(stored.content_type.as_deref(), Some(format.to_mime_type())); - assert_eq!(stored.content.unwrap(), bytes); - } - } - } - - #[tokio::test] - async fn preview_images_preserve_remote_dimensions() { - let (_directory, services) = services().await; - let server = ImageServer::new( - &services, - StatusCode::OK, - Some("image/png"), - image(ImageFormat::Png), - ) - .await; - for (width, height) in [(Some(99), Some(98)), (Some(99), None), (None, Some(98))] { - let preview = services - .media - .download_image( - &server.url, - Some(UrlPreviewData { - image_width: width, - image_height: height, - ..UrlPreviewData::default() - }), - ) - .await - .unwrap(); - assert_eq!(preview.image_width, width.or(Some(2))); - assert_eq!(preview.image_height, height.or(Some(3))); - } - } - - #[tokio::test] - async fn preview_images_reject_failed_or_invalid_downloads_before_storage() { - let (_directory, services) = services().await; - let png = image(ImageFormat::Png); - let cases = [ - (StatusCode::FOUND, png.clone()), - (StatusCode::FORBIDDEN, png.clone()), - (StatusCode::NOT_FOUND, b"Not found".to_vec()), - (StatusCode::INTERNAL_SERVER_ERROR, png.clone()), - (StatusCode::OK, b"Access denied".to_vec()), - (StatusCode::OK, Vec::new()), - (StatusCode::OK, png[..33].to_vec()), - (StatusCode::OK, vec![0; 4097]), - ]; - for (status, bytes) in cases { - let server = ImageServer::new(&services, status, Some("image/png"), bytes).await; - assert!( - services - .media - .download_image(&server.url, None) - .await - .is_err(), - "accepted failed or invalid image with status {status}", - ); - assert!(services.media.get_all_mxcs().await.unwrap().is_empty()); - } - } - - #[tokio::test] - async fn preview_images_read_dimensions_without_decoding_pixels() { - let (_directory, services) = services().await; - let mut png = image(ImageFormat::Png); - png.truncate(png.len() - 20); - assert!(image::load_from_memory(&png).is_err()); - let server = ImageServer::new(&services, StatusCode::OK, Some("image/png"), png).await; - let preview = services - .media - .download_image(&server.url, None) - .await - .unwrap(); - assert_eq!((preview.image_width, preview.image_height), (Some(2), Some(3))); - - for (width, height) in [(8193_u16, 1_u16), (5000, 5000)] { - let mut bytes = image(ImageFormat::Gif); - bytes[6..8].copy_from_slice(&width.to_le_bytes()); - bytes[8..10].copy_from_slice(&height.to_le_bytes()); - let server = - ImageServer::new(&services, StatusCode::OK, Some("image/gif"), bytes).await; - let preview = services - .media - .download_image(&server.url, None) - .await - .unwrap(); - assert_eq!( - (preview.image_width, preview.image_height), - (Some(u32::from(width)), Some(u32::from(height))), - ); - } - } -} - #[tokio::test] #[cfg(disable)] //TODO: fixme async fn long_file_names_works() {