diff --git a/src/blob.rs b/src/blob.rs index 2a3029abb9..09bf3b2f1d 100644 --- a/src/blob.rs +++ b/src/blob.rs @@ -21,6 +21,7 @@ use crate::constants::{self, MediaQuality}; use crate::context::Context; use crate::events::EventType; use crate::log::{error, info, warn, LogExt}; +use crate::message::Viewtype; use crate::tools::sanitize_filename; /// Represents a file in the blob directory. @@ -267,32 +268,30 @@ impl<'a> BlobObject<'a> { } }; - let maybe_sticker = &mut false; + let viewtype = &mut Viewtype::Image; let is_avatar = true; - self.recode_to_size( - context, - None, // The name of an avatar doesn't matter - maybe_sticker, - img_wh, - max_bytes, - is_avatar, + self.check_or_recode_to_size( + context, None, // The name of an avatar doesn't matter + viewtype, img_wh, max_bytes, is_avatar, )?; Ok(()) } - /// Recodes an image pointed by a [BlobObject] so that it fits into limits on the image width, - /// height and file size specified by the config. + /// Checks or recodes an image pointed by the [BlobObject] so that it fits into limits on the + /// image width, height and file size specified by the config. /// - /// On some platforms images are passed to the core as [`crate::message::Viewtype::Sticker`] in - /// which case `maybe_sticker` flag should be set. We recheck if an image is a true sticker - /// assuming that it must have at least one fully transparent corner, otherwise this flag is - /// reset. - pub async fn recode_to_image_size( + /// Recoding is only done for [`Viewtype::Image`]. For [`Viewtype::File`], if it's a correct + /// image, `*viewtype` is set to [`Viewtype::Image`]. + /// + /// On some platforms images are passed to Core as [`Viewtype::Sticker`]. We recheck if the + /// image is a true sticker assuming that it must have at least one fully transparent corner, + /// otherwise `*viewtype` is set to [`Viewtype::Image`]. + pub async fn check_or_recode_image( &mut self, context: &Context, name: Option, - maybe_sticker: &mut bool, + viewtype: &mut Viewtype, ) -> Result { let (img_wh, max_bytes) = match MediaQuality::from_i32(context.get_config_int(Config::MediaQuality).await?) @@ -305,13 +304,10 @@ impl<'a> BlobObject<'a> { MediaQuality::Worse => (constants::WORSE_IMAGE_SIZE, constants::WORSE_IMAGE_BYTES), }; let is_avatar = false; - let new_name = - self.recode_to_size(context, name, maybe_sticker, img_wh, max_bytes, is_avatar)?; - - Ok(new_name) + self.check_or_recode_to_size(context, name, viewtype, img_wh, max_bytes, is_avatar) } - /// Recodes the image so that it fits into limits on width/height and byte size. + /// Checks or recodes the image so that it fits into limits on width/height and byte size. /// /// If `!is_avatar`, then if `max_bytes` is exceeded, reduces the image to `img_wh` and proceeds /// with the result without rechecking. @@ -322,11 +318,11 @@ impl<'a> BlobObject<'a> { /// then the updated user-visible filename will be returned; /// this may be necessary because the format may be changed to JPG, /// i.e. "image.png" -> "image.jpg". - fn recode_to_size( + fn check_or_recode_to_size( &mut self, context: &Context, name: Option, - maybe_sticker: &mut bool, + viewtype: &mut Viewtype, mut img_wh: u32, max_bytes: usize, is_avatar: bool, @@ -337,6 +333,7 @@ impl<'a> BlobObject<'a> { let no_exif_ref = &mut no_exif; let mut name = name.unwrap_or_else(|| self.name.clone()); let original_name = name.clone(); + let vt = &mut *viewtype; let res: Result = tokio::task::block_in_place(move || { let mut file = std::fs::File::open(self.to_abs_path())?; let (nr_bytes, exif) = image_metadata(&file)?; @@ -355,21 +352,28 @@ impl<'a> BlobObject<'a> { ) } }; - let fmt = imgreader.format().context("No format??")?; + let fmt = imgreader.format().context("Unknown format")?; + if *vt == Viewtype::File { + *vt = Viewtype::Image; + return Ok(name); + } let mut img = imgreader.decode().context("image decode failure")?; let orientation = exif.as_ref().map(|exif| exif_orientation(exif, context)); let mut encoded = Vec::new(); - if *maybe_sticker { + if *vt == Viewtype::Sticker { let x_max = img.width().saturating_sub(1); let y_max = img.height().saturating_sub(1); - *maybe_sticker = img.in_bounds(x_max, y_max) - && (img.get_pixel(0, 0).0[3] == 0 + if !img.in_bounds(x_max, y_max) + || !(img.get_pixel(0, 0).0[3] == 0 || img.get_pixel(x_max, 0).0[3] == 0 || img.get_pixel(0, y_max).0[3] == 0 - || img.get_pixel(x_max, y_max).0[3] == 0); + || img.get_pixel(x_max, y_max).0[3] == 0) + { + *vt = Viewtype::Image; + } } - if *maybe_sticker && exif.is_none() { + if *vt == Viewtype::Sticker && exif.is_none() { return Ok(name); } @@ -504,10 +508,11 @@ impl<'a> BlobObject<'a> { Ok(_) => res, Err(err) => { if !is_avatar && no_exif { - warn!( + error!( context, - "Cannot recode image, using original data: {err:#}.", + "Cannot check/recode image, using original data: {err:#}.", ); + *viewtype = Viewtype::File; Ok(original_name) } else { Err(err) diff --git a/src/blob/blob_tests.rs b/src/blob/blob_tests.rs index 47132baf26..9690531f3e 100644 --- a/src/blob/blob_tests.rs +++ b/src/blob/blob_tests.rs @@ -140,9 +140,9 @@ async fn test_add_white_bg() { let mut blob = BlobObject::create_and_deduplicate(&t, &avatar_src, &avatar_src).unwrap(); let img_wh = 128; - let maybe_sticker = &mut false; + let viewtype = &mut Viewtype::Image; let strict_limits = true; - blob.recode_to_size(&t, None, maybe_sticker, img_wh, 20_000, strict_limits) + blob.check_or_recode_to_size(&t, None, viewtype, img_wh, 20_000, strict_limits) .unwrap(); tokio::task::block_in_place(move || { let img = ImageReader::open(blob.to_abs_path()) @@ -188,9 +188,9 @@ async fn test_selfavatar_outside_blobdir() { ); let mut blob = BlobObject::create_and_deduplicate(&t, avatar_path, avatar_path).unwrap(); - let maybe_sticker = &mut false; + let viewtype = &mut Viewtype::Image; let strict_limits = true; - blob.recode_to_size(&t, None, maybe_sticker, 1000, 3000, strict_limits) + blob.check_or_recode_to_size(&t, None, viewtype, 1000, 3000, strict_limits) .unwrap(); let new_file_size = file_size(&blob.to_abs_path()).await; assert!(new_file_size <= 3000); @@ -400,7 +400,7 @@ async fn test_recode_image_balanced_png() { .await .unwrap(); - // This will be sent as Image, see [`BlobObject::maybe_sticker`] for explanation. + // This will be sent as Image, see [`BlobObject::check_or_recode_image()`] for explanation. SendImageCheckMediaquality { viewtype: Viewtype::Sticker, media_quality_config: "0", diff --git a/src/chat.rs b/src/chat.rs index d79a76ec8f..badb858200 100644 --- a/src/chat.rs +++ b/src/chat.rs @@ -2755,7 +2755,7 @@ async fn prepare_msg_blob(context: &Context, msg: &mut Message) -> Result<()> { .param .get_file_blob(context)? .with_context(|| format!("attachment missing for message of type #{}", msg.viewtype))?; - let send_as_is = msg.viewtype == Viewtype::File; + let mut maybe_image = false; if msg.viewtype == Viewtype::File || msg.viewtype == Viewtype::Image @@ -2773,6 +2773,8 @@ async fn prepare_msg_blob(context: &Context, msg: &mut Message) -> Result<()> { // UIs don't want conversions of `Sticker` to anything other than `Image`. msg.param.set_int(Param::ForceSticker, 1); } + } else if better_type == Viewtype::Image { + maybe_image = true; } else if better_type != Viewtype::Webxdc || context .ensure_sendable_webxdc_file(&blob.to_abs_path()) @@ -2791,25 +2793,27 @@ async fn prepare_msg_blob(context: &Context, msg: &mut Message) -> Result<()> { if msg.viewtype == Viewtype::Vcard { msg.try_set_vcard(context, &blob.to_abs_path()).await?; } - - let mut maybe_sticker = msg.viewtype == Viewtype::Sticker; - if !send_as_is - && (msg.viewtype == Viewtype::Image - || maybe_sticker && !msg.param.exists(Param::ForceSticker)) + if msg.viewtype == Viewtype::File && maybe_image + || msg.viewtype == Viewtype::Image + || msg.viewtype == Viewtype::Sticker && !msg.param.exists(Param::ForceSticker) { let new_name = blob - .recode_to_image_size(context, msg.get_filename(), &mut maybe_sticker) + .check_or_recode_image(context, msg.get_filename(), &mut msg.viewtype) .await?; msg.param.set(Param::Filename, new_name); msg.param.set(Param::File, blob.as_name()); - - if !maybe_sticker { - msg.viewtype = Viewtype::Image; - } } if !msg.param.exists(Param::MimeType) { - if let Some((_, mime)) = message::guess_msgtype_from_suffix(msg) { + if let Some((viewtype, mime)) = message::guess_msgtype_from_suffix(msg) { + // If we unexpectedly didn't recognize the file as image, don't send it as such, + // either the format is unsupported or the image is corrupted. + let mime = match viewtype != Viewtype::Image + || matches!(msg.viewtype, Viewtype::Image | Viewtype::Sticker) + { + true => mime, + false => "application/octet-stream", + }; msg.param.set(Param::MimeType, mime); } } diff --git a/src/chat/chat_tests.rs b/src/chat/chat_tests.rs index 908ac29af2..98e8c0138f 100644 --- a/src/chat/chat_tests.rs +++ b/src/chat/chat_tests.rs @@ -3457,6 +3457,29 @@ async fn test_jpeg_with_png_ext() -> Result<()> { Ok(()) } +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn test_nonimage_with_png_ext() -> Result<()> { + let alice = TestContext::new_alice().await; + let bob = TestContext::new_bob().await; + let alice_chat = alice.create_chat(&bob).await; + + let bytes = include_bytes!("../../test-data/message/thunderbird_with_autocrypt.eml"); + let file = alice.get_blobdir().join("screenshot.png"); + + for vt in [Viewtype::Image, Viewtype::File] { + tokio::fs::write(&file, bytes).await?; + let mut msg = Message::new(vt); + msg.set_file_and_deduplicate(&alice, &file, Some("screenshot.png"), None)?; + let sent_msg = alice.send_msg(alice_chat.get_id(), &mut msg).await; + assert_eq!(msg.viewtype, Viewtype::File); + assert_eq!(msg.get_filemime().unwrap(), "application/octet-stream"); + let msg_bob = bob.recv_msg(&sent_msg).await; + assert_eq!(msg_bob.viewtype, Viewtype::File); + assert_eq!(msg_bob.get_filemime().unwrap(), "application/octet-stream"); + } + Ok(()) +} + /// Tests that info message is ignored when constructing `In-Reply-To`. #[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn test_info_not_referenced() -> Result<()> { diff --git a/src/message.rs b/src/message.rs index 0f1e9da1f0..d399e5a9d3 100644 --- a/src/message.rs +++ b/src/message.rs @@ -1601,7 +1601,7 @@ pub(crate) fn guess_msgtype_from_path_suffix(path: &Path) -> Option<(Viewtype, & "rtf" => (Viewtype::File, "application/rtf"), "spx" => (Viewtype::File, "audio/ogg"), // Ogg Speex Profile "svg" => (Viewtype::File, "image/svg+xml"), - "tgs" => (Viewtype::Sticker, "application/x-tgsticker"), + "tgs" => (Viewtype::File, "application/x-tgsticker"), "tiff" => (Viewtype::File, "image/tiff"), "tif" => (Viewtype::File, "image/tiff"), "ttf" => (Viewtype::File, "font/ttf"), diff --git a/src/mimefactory.rs b/src/mimefactory.rs index 14639b918f..f53673f6df 100644 --- a/src/mimefactory.rs +++ b/src/mimefactory.rs @@ -26,7 +26,7 @@ use crate::ephemeral::Timer as EphemeralTimer; use crate::key::DcKey; use crate::location; use crate::log::{info, warn}; -use crate::message::{self, Message, MsgId, Viewtype}; +use crate::message::{Message, MsgId, Viewtype}; use crate::mimeparser::{is_hidden, SystemMessage}; use crate::param::Param; use crate::peer_channels::create_iroh_header; @@ -1726,18 +1726,11 @@ async fn build_body_file(context: &Context, msg: &Message) -> Result file_name, }; - /* check mimetype */ - let mimetype = match msg.param.get(Param::MimeType) { - Some(mtype) => mtype.to_string(), - None => { - if let Some((_viewtype, res)) = message::guess_msgtype_from_suffix(msg) { - res.to_string() - } else { - "application/octet-stream".to_string() - } - } - }; - + let mimetype = msg + .param + .get(Param::MimeType) + .unwrap_or("application/octet-stream") + .to_string(); let body = fs::read(blob.to_abs_path()).await?; // create mime part, for Content-Disposition, see RFC 2183. diff --git a/src/mimefactory/mimefactory_tests.rs b/src/mimefactory/mimefactory_tests.rs index 22306ddebe..673fe89bea 100644 --- a/src/mimefactory/mimefactory_tests.rs +++ b/src/mimefactory/mimefactory_tests.rs @@ -12,6 +12,7 @@ use crate::chatlist::Chatlist; use crate::constants; use crate::contact::Origin; use crate::headerdef::HeaderDef; +use crate::message; use crate::mimeparser::MimeMessage; use crate::receive_imf::receive_imf; use crate::test_utils::{get_chat_msg, TestContext, TestContextManager}; diff --git a/src/mimeparser.rs b/src/mimeparser.rs index 92728c463d..c22abc832d 100644 --- a/src/mimeparser.rs +++ b/src/mimeparser.rs @@ -1018,7 +1018,12 @@ impl MimeMessage { is_related: bool, ) -> Result { let mut any_part_added = false; - let mimetype = get_mime_type(mail, &get_attachment_filename(context, mail)?)?.0; + let mimetype = get_mime_type( + mail, + &get_attachment_filename(context, mail)?, + self.has_chat_version(), + )? + .0; match (mimetype.type_(), mimetype.subtype().as_str()) { /* Most times, multipart/alternative contains true alternatives as text/plain and text/html. If we find a multipart/mixed @@ -1026,8 +1031,12 @@ impl MimeMessage { apple mail: "plaintext" as an alternative to "html+PDF attachment") */ (mime::MULTIPART, "alternative") => { for cur_data in &mail.subparts { - let mime_type = - get_mime_type(cur_data, &get_attachment_filename(context, cur_data)?)?.0; + let mime_type = get_mime_type( + cur_data, + &get_attachment_filename(context, cur_data)?, + self.has_chat_version(), + )? + .0; if mime_type == "multipart/mixed" || mime_type == "multipart/related" { any_part_added = self .parse_mime_recursive(context, cur_data, is_related) @@ -1038,9 +1047,13 @@ impl MimeMessage { if !any_part_added { /* search for text/plain and add this */ for cur_data in &mail.subparts { - if get_mime_type(cur_data, &get_attachment_filename(context, cur_data)?)? - .0 - .type_() + if get_mime_type( + cur_data, + &get_attachment_filename(context, cur_data)?, + self.has_chat_version(), + )? + .0 + .type_() == mime::TEXT { any_part_added = self @@ -1172,7 +1185,7 @@ impl MimeMessage { ) -> Result { // return true if a part was added let filename = get_attachment_filename(context, mail)?; - let (mime_type, msg_type) = get_mime_type(mail, &filename)?; + let (mime_type, msg_type) = get_mime_type(mail, &filename, self.has_chat_version())?; let raw_mime = mail.ctype.mimetype.to_lowercase(); let old_part_count = self.parts.len(); @@ -2088,6 +2101,7 @@ pub struct Part { fn get_mime_type( mail: &mailparse::ParsedMail<'_>, filename: &Option, + is_chat_msg: bool, ) -> Result<(Mime, Viewtype)> { let mimetype = mail.ctype.mimetype.parse::()?; @@ -2121,13 +2135,13 @@ fn get_mime_type( } mime::APPLICATION => match mimetype.subtype() { mime::OCTET_STREAM => match filename { - Some(filename) => { + Some(filename) if !is_chat_msg => { match message::guess_msgtype_from_path_suffix(Path::new(&filename)) { Some((viewtype, _)) => viewtype, None => Viewtype::File, } } - None => Viewtype::File, + _ => Viewtype::File, }, _ => Viewtype::File, },