fix: Treat and send images that can't be decoded as Viewtype::File

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<String>,
-        maybe_sticker: &mut bool,
+        viewtype: &mut Viewtype,
     ) -> Result<String> {
         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<String>,
-        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<String> = 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)

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",

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);
             }
         }

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<()> {

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"),

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<MimePart<'s
         _ => 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.

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};

src/mimeparser.rs

@@ -1018,16 +1018,25 @@ impl MimeMessage {
         is_related: bool,
     ) -> Result<bool> {
         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
             inside multipart/alternative, we use this (happens eg in
             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<bool> {
         // 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<String>,
+    is_chat_msg: bool,
 ) -> Result<(Mime, Viewtype)> {
     let mimetype = mail.ctype.mimetype.parse::<Mime>()?;
 
@@ -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,
         },