diff --git a/changelog.d/2448.bugfix.md b/changelog.d/2448.bugfix.md new file mode 100644 index 000000000..557e68c4a --- /dev/null +++ b/changelog.d/2448.bugfix.md @@ -0,0 +1 @@ +URL previews no longer completely fail if downloading auxiliary content (images/videos/audio) fails. Contributed by @nex. diff --git a/src/api/client/media.rs b/src/api/client/media.rs index 669cda35d..4f780cd99 100644 --- a/src/api/client/media.rs +++ b/src/api/client/media.rs @@ -2,7 +2,7 @@ use std::time::Duration; use axum::extract::State; use conduwuit::{ - Err, Result, err, + Err, Result, err, info, utils::{self, content_disposition::make_content_disposition, math::ruma_from_usize}, }; use conduwuit_core::error; @@ -219,10 +219,15 @@ pub(crate) async fn get_media_preview_route( ))); } + info!("Fetching a URL preview for {url} on behalf of {sender_user}"); let preview = services .media .get_url_preview(&url) .await + .inspect(|_| info!(user=%sender_user, %url, "Finished fetching a URL preview")) + .inspect_err( + |e| info!(user=%sender_user, %url, error=?e, "Failed to fetch a URL preview"), + ) .map_err(|error| { err!(Request(Unknown( debug_error!(%sender_user, %url, "Failed to fetch URL preview: {error}") diff --git a/src/service/client/mod.rs b/src/service/client/mod.rs index 39f00b40c..4d668b72d 100644 --- a/src/service/client/mod.rs +++ b/src/service/client/mod.rs @@ -111,7 +111,7 @@ impl crate::Service for Service { .local_address(url_preview_bind_addr) .timeout(Duration::from_secs(config.url_preview_timeout)) .redirect(redirect::Policy::limited(3)) - .user_agent(url_preview_user_agent) + .user_agent(url_preview_user_agent.replace("", conduwuit::version_ua())) .build()?, external_resource: base(config, reqwest_dns_resolver.clone())? diff --git a/src/service/media/data.rs b/src/service/media/data.rs index 7416464f8..c0c3cc1f0 100644 --- a/src/service/media/data.rs +++ b/src/service/media/data.rs @@ -248,6 +248,8 @@ impl Data { x => x, };*/ + // TODO(nex): Oh my god this is awful when can we kill it + let title = match values .next() .and_then(|b| String::from_utf8(b.to_vec()).ok()) @@ -346,6 +348,11 @@ impl Data { video_height, audio, audio_size, + ..Default::default() /* TODO(nex,silversys): This weird manual field + * de/serialisation just needs replacing with JSON storage to + * avoid this mess. This is consequently intentionally + * dropping information added later (like image alt text). + * I'll do it later. */ }) } } diff --git a/src/service/media/preview.rs b/src/service/media/preview.rs index 084aa9a1b..2bb6ac93e 100644 --- a/src/service/media/preview.rs +++ b/src/service/media/preview.rs @@ -7,8 +7,9 @@ use std::time::SystemTime; -use conduwuit::{Err, Result, debug, err, utils::response::LimitReadExt}; +use conduwuit::{Err, Result, debug, err, info, utils::response::LimitReadExt}; use ipaddress::IPAddress; +use lettre::message::header::ContentType; use ruma::OwnedMxcUri; use serde::Serialize; use url::Url; @@ -16,20 +17,54 @@ use url::Url; use super::Service; use crate::media::mxc::Mxc; -#[derive(Serialize, Default)] +#[derive(Serialize, Default, Debug)] pub struct UrlPreviewData { + // From https://ogp.me/ + /// The title of your object as it should appear within the graph + /// "The Rock". #[serde(skip_serializing_if = "Option::is_none", rename(serialize = "og:title"))] pub title: Option, + + /// The type of your object, e.g., "video.movie". + #[serde(skip_serializing_if = "String::is_empty", rename(serialize = "og:type"))] + pub site_type: String, + /// The canonical URL of your object that will be used as its permanent ID + /// in the graph. + #[serde(skip_serializing_if = "Option::is_none", rename(serialize = "og:url"))] + pub url: Option, + /// A one to two sentence description of your object. #[serde(skip_serializing_if = "Option::is_none", rename(serialize = "og:description"))] pub description: Option, + /// The word that appears before this object's title in a sentence. + #[serde(skip_serializing_if = "Option::is_none", rename(serialize = "og:determiner"))] + pub determiner: Option, + /// The locale these tags are marked up in + #[serde(skip_serializing_if = "Option::is_none", rename(serialize = "og:locale"))] + pub locale: Option, + /// If your object is part of a larger web site, the name which should be + /// displayed for the overall site. + #[serde(skip_serializing_if = "Option::is_none", rename(serialize = "og:site_name"))] + pub site_name: Option, + + /// An image URL which should represent your object within the graph. #[serde(skip_serializing_if = "Option::is_none", rename(serialize = "og:image"))] pub image: Option, + /// A MIME type for this image. + #[serde(skip_serializing_if = "Option::is_none", rename(serialize = "og:image:type"))] + pub image_type: Option, #[serde(skip_serializing_if = "Option::is_none", rename(serialize = "matrix:image:size"))] pub image_size: Option, + /// The number of pixels wide. #[serde(skip_serializing_if = "Option::is_none", rename(serialize = "og:image:width"))] pub image_width: Option, + /// The number of pixels high. #[serde(skip_serializing_if = "Option::is_none", rename(serialize = "og:image:height"))] pub image_height: Option, + /// A description of what is in the image (not a caption). + #[serde(skip_serializing_if = "Option::is_none", rename(serialize = "og:image:alt"))] + pub image_alt: Option, + + /// A URL to a video file that complements this object. #[serde(skip_serializing_if = "Option::is_none", rename(serialize = "og:video"))] pub video: Option, #[serde(skip_serializing_if = "Option::is_none", rename(serialize = "matrix:video:size"))] @@ -38,6 +73,11 @@ pub struct UrlPreviewData { pub video_width: Option, #[serde(skip_serializing_if = "Option::is_none", rename(serialize = "og:video:height"))] pub video_height: Option, + #[serde(skip_serializing_if = "Option::is_none", rename(serialize = "og:video:type"))] + pub video_type: Option, + #[serde(skip_serializing_if = "Option::is_none", rename(serialize = "og:video:alt"))] + pub video_alt: Option, + /// A URL to an audio file to accompany this object. #[serde(skip_serializing_if = "Option::is_none", rename(serialize = "og:audio"))] pub audio: Option, #[serde(skip_serializing_if = "Option::is_none", rename(serialize = "matrix:audio:size"))] @@ -100,7 +140,9 @@ impl Service { } let Some(content_type) = response.headers().get(reqwest::header::CONTENT_TYPE) else { - return Err!(Request(Unknown("Unknown or invalid Content-Type header"))); + return Err!(Request(Unknown( + "Response contains unknown, invalid, or missing Content-Type header" + ))); }; let content_type = content_type @@ -109,12 +151,10 @@ impl Service { let data = match content_type { | html if html.starts_with("text/html") => self.download_html(url.as_str()).await?, - | img if img.starts_with("image/") => self.download_image(url.as_str(), None).await?, - | video if video.starts_with("video/") => - self.download_video(url.as_str(), None).await?, - | audio if audio.starts_with("audio/") => - self.download_audio(url.as_str(), None).await?, - | _ => return Err!(Request(Unknown("Unsupported Content-Type"))), + | img if img.starts_with("image/") => self.download_image(url.as_str()).await?, + | video if video.starts_with("video/") => self.download_video(url.as_str()).await?, + | audio if audio.starts_with("audio/") => self.download_audio(url.as_str()).await?, + | other => return Err!(Request(Unknown("Unsupported Content-Type: {other}"))), }; self.set_url_preview(url.as_str(), &data).await?; @@ -123,17 +163,13 @@ impl Service { } #[cfg(feature = "url_preview")] - pub async fn download_image( - &self, - url: &str, - preview_data: Option, - ) -> Result { + pub async fn download_image(&self, url: &str) -> Result { use conduwuit::utils::random_string; use image::ImageReader; use crate::media::mxc::Mxc; - let mut preview_data = preview_data.unwrap_or_default(); + let mut preview_data = UrlPreviewData::default(); let response = self.services.client.url_preview.get(url).send().await?; if !response.status().is_success() { @@ -157,13 +193,12 @@ impl Service { .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 (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); + preview_data.image_type = content_type.parse().ok(); let mxc = Mxc { server_name: self.services.globals.server_name(), @@ -179,12 +214,9 @@ impl Service { } #[cfg(feature = "url_preview")] - pub async fn download_video( - &self, - url: &str, - preview_data: Option, - ) -> Result { - let mut preview_data = preview_data.unwrap_or_default(); + #[tracing::instrument(skip(self))] + pub async fn download_video(&self, url: &str) -> Result { + let mut preview_data = UrlPreviewData::default(); if self.services.globals.url_preview_allow_audio_video() { let (url, size) = self.download_media(url).await?; @@ -196,12 +228,9 @@ impl Service { } #[cfg(feature = "url_preview")] - pub async fn download_audio( - &self, - url: &str, - preview_data: Option, - ) -> Result { - let mut preview_data = preview_data.unwrap_or_default(); + #[tracing::instrument(skip(self))] + pub async fn download_audio(&self, url: &str) -> Result { + let mut preview_data = UrlPreviewData::default(); if self.services.globals.url_preview_allow_audio_video() { let (url, size) = self.download_media(url).await?; @@ -212,6 +241,7 @@ impl Service { Ok(preview_data) } + #[tracing::instrument(skip(self))] pub async fn download_media(&self, url: &str) -> Result<(OwnedMxcUri, usize)> { use conduwuit::utils::random_string; use http::header::CONTENT_TYPE; @@ -269,6 +299,7 @@ impl Service { } #[cfg(feature = "url_preview")] + #[tracing::instrument(skip(self))] async fn download_html(&self, url: &str) -> Result { use webpage::HTML; @@ -286,32 +317,75 @@ impl Service { .expect("u64 should fit in usize"), ) .await?; - let Ok(html) = HTML::from_string(body.clone(), Some(url.to_owned())) else { + let Ok(html) = HTML::from_string(body.clone(), Some(url.to_owned())) + .inspect_err(|e| debug!(error=?e, "Failed to parse HTML data")) + else { return Err!(Request(Unknown("Failed to parse HTML"))); }; - let mut preview_data = UrlPreviewData::default(); + let mut preview_data = UrlPreviewData::from(&html.opengraph); + debug!(parsed=?preview_data, deserialised=?html.opengraph, "Parsed opengraph data"); if let Some(obj) = html.opengraph.images.first() { - preview_data = self.download_image(&obj.url, Some(preview_data)).await?; + info!(asset_url=%obj.url, "Downloading preview image"); + if let Ok(image_data) = self.download_image(&obj.url).await.inspect_err( + |e| info!(error=?e, asset_url=%obj.url, "Could not download URL preview image"), + ) { + preview_data.image = image_data.image; + preview_data.image_size = image_data.image_size; + // Prioritise what the top-level site specified, falling back to + // detected params. + preview_data.image_width = preview_data.image_width.or(image_data.image_width); + preview_data.image_height = preview_data.image_height.or(image_data.image_height); + preview_data.image_type = preview_data.image_type.or(image_data.image_type); + } } if let Some(obj) = html.opengraph.videos.first() { - preview_data = self.download_video(&obj.url, Some(preview_data)).await?; - preview_data.video_width = obj.properties.get("width").and_then(|v| v.parse().ok()); - preview_data.video_height = obj.properties.get("height").and_then(|v| v.parse().ok()); + info!(asset_url=%obj.url, "Downloading preview video"); + if let Ok(video_data) = self.download_video(&obj.url).await.inspect_err( + |e| info!(error=?e, asset_url=%obj.url, "Could not download URL preview video"), + ) { + preview_data.video = video_data.video; + preview_data.video_size = video_data.video_size; + } } if let Some(obj) = html.opengraph.audios.first() { - preview_data = self.download_audio(&obj.url, Some(preview_data)).await?; + info!(asset_url=%obj.url, "Downloading URL preview audio"); + if let Ok(audio_data) = self.download_audio(&obj.url).await.inspect_err( + |e| info!(error=?e, asset_url=%obj.url, "Could not download URL preview audio"), + ) { + preview_data.audio = audio_data.audio; + preview_data.audio_size = audio_data.audio_size; + } } - let props = html.opengraph.properties; + // Provide defaults for some params + preview_data.title = preview_data.title.or(html.title); + preview_data.description = preview_data.description.or(html.description); + preview_data.url = preview_data.url.or_else(|| Some(url.to_owned())); - /* use OpenGraph title/description, but fall back to HTML if not - * available */ - preview_data.title = props.get("title").cloned().or(html.title); - preview_data.description = props.get("description").cloned().or(html.description); + // We remove og:image:*, og:video:*, and og:audio:* if their respective + // prior-level properties are not present. + // We do this here instead of in the From<> impl because we can't know + // if we'll be able to fetch the media to convert it to an mxc that + // point. + if preview_data.image.is_none() { + preview_data.image_type = None; + preview_data.image_alt = None; + preview_data.image_height = None; + preview_data.image_width = None; + } + if preview_data.video.is_none() { + preview_data.video_type = None; + preview_data.video_alt = None; + preview_data.video_height = None; + preview_data.video_width = None; + } + if preview_data.audio.is_none() { + preview_data.audio_size = None; + } Ok(preview_data) } @@ -443,3 +517,114 @@ impl Service { false } } + +impl From<&webpage::Opengraph> for UrlPreviewData { + fn from(og: &webpage::Opengraph) -> Self { + Self { + title: og.properties.get("title").cloned(), + description: og.properties.get("description").cloned(), + site_type: og.og_type.clone(), + url: og.properties.get("url").cloned(), + determiner: og.properties.get("determiner").cloned(), + locale: og.properties.get("locale").cloned(), + site_name: og.properties.get("site_name").cloned(), + image_type: og + .images + .first() + .and_then(|img| img.properties.get("type").and_then(|t| t.parse().ok())), + image_width: og + .images + .first() + .and_then(|img| img.properties.get("width").and_then(|w| w.parse().ok())), + image_height: og + .images + .first() + .and_then(|img| img.properties.get("height").and_then(|h| h.parse().ok())), + image_alt: og + .images + .first() + .and_then(|img| img.properties.get("alt").cloned()), + video_type: og + .videos + .first() + .and_then(|vid| vid.properties.get("type")) + .and_then(|t| t.parse().ok()), + video_width: og + .videos + .first() + .and_then(|vid| vid.properties.get("width")) + .and_then(|w| w.parse().ok()), + video_height: og + .videos + .first() + .and_then(|vid| vid.properties.get("height")) + .and_then(|h| h.parse().ok()), + video_alt: og + .videos + .first() + .and_then(|vid| vid.properties.get("alt").cloned()), + ..Default::default() + } + } +} + +#[cfg(test)] +mod tests { + use webpage::HTML; + + use super::UrlPreviewData; + + #[test] + fn test_urlpreviewdata_parses_correctly() { + let html = r#" + + + + + + + + + + + + + + + + + + "#; + + let parsed_html = + HTML::from_string(html.to_owned(), Some("https://example.com".to_owned())) + .expect("Failed to parse HTML"); + + let data = UrlPreviewData::from(&parsed_html.opengraph); + + assert_eq!(data.title, Some("Test Title".to_owned())); + assert_eq!(data.description, Some("Test Description".to_owned())); + assert_eq!( + data.image, None, + "UrlPreviewData should not inherit HTTP/S URL from URL preview" + ); + assert_eq!( + data.video, None, + "UrlPreviewData should not inherit HTTP/S URL from URL preview" + ); + assert_eq!( + data.audio, None, + "UrlPreviewData should not inherit HTTP/S URL from URL preview" + ); + assert_eq!(data.url, Some("https://example.com/index.html".to_owned())); + assert_eq!(data.site_type, "org.continuwuity.other".to_owned()); + assert_eq!(data.site_name, Some("Example Site".to_owned())); + assert_eq!(data.image_alt, Some("An example image".to_owned())); + assert_eq!(data.image_type, Some("image/png".parse().unwrap())); + assert_eq!(data.image_height, Some(69)); + assert_eq!(data.image_width, Some(420)); + assert_eq!(data.image_size, None, "*_size must be None when pulling from opengraph"); + assert_eq!(data.video_size, None, "*_size must be None when pulling from opengraph"); + assert_eq!(data.audio_size, None, "*_size must be None when pulling from opengraph"); + } +}