Skip to content

fix!: deal with huge incoming image pixelsizes #6825 #6935

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 8 commits into
base: pre-main
Choose a base branch
from

Conversation

ivan-cx
Copy link

@ivan-cx ivan-cx commented Jun 24, 2025

This pull request attempts to fix #6825

BREAKING CHANGE: messages with invalid images, images of unknown size, huge images, will have Viewtype::File

After changing the logic of Viewtype selection, I had to fix 3 old tests that used invalid Base64 image data.

ivan-cx added 3 commits June 24, 2025 23:58
 - test_huge_image_becomes_file: message with huge image should have Viewtype::File
 - test_4k_image_stays_image: message with 4k image should have Viewtype::Image

Refs: chatmail#6825
BREAKING CHANGE: messages with invalid images, images of unknown size,
huge images, will have Viewtype::File
Refs: chatmail#6825
 - parse_inline_image
 - parse_thunderbird_html_embedded_image
 - parse_outlook_html_embedded_image

The tests were failing because messages with images of unknown size
(invalid images) now have Viewtype::File and not Viewtype::Image

Refs: chatmail#6825
@ivan-cx ivan-cx changed the title deal with huge incoming image pixelsizes #6825 fix!: deal with huge incoming image pixelsizes #6825 Jun 25, 2025
@ivan-cx
Copy link
Author

ivan-cx commented Jun 25, 2025

The CI tests are failing, I assume, due to unset CHATMAIL_DOMAIN repository variable in my forked repo. I've added the variable now, let's see if it works?

@ivan-cx
Copy link
Author

ivan-cx commented Jun 25, 2025

Ok, I do not know how to fix
"CAN NOT RUN COVERAGE correctly: Missing CHATMAIL_DOMAIN environment variable!"
and similar issues in tests. My forked repo has CHATMAIL_DOMAIN repository variable AND environment variable, but it does not affect the runs in this PR.
Local Python tests work fine for me on Ubuntu, I set CHATMAIL_DOMAIN manually when I run them.

}
// image is too big or size is unknown, display as file:
_ => Viewtype::File,
}
Copy link
Author

@ivan-cx ivan-cx Jun 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This "else if" block is the main change of the PR.
For Viewtype::Image and Viewtype::Gif we check image size and set width and height for valid images.
Otherwise, we change msg_type to Viewtype::File - for all images/gifs of unknown size, or too big.
Setting of width and height previously was done not only for Viewtype::Image and Viewtype::Gif, but for any mime::IMAGE, which is not the same (at least, SVG is mime::IMAGE, but Viewtype::File)

@iequidoo iequidoo self-requested a review June 25, 2025 18:31
Copy link
Collaborator

@iequidoo iequidoo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

JSON-RPC tests failing with Missing CHATMAIL_DOMAIN environment variable! is a known problem for PRs from other repos.

ivan-cx added 2 commits June 25, 2025 23:54
 - treat Viewtype::Sticker the same way as Image and Gif
 - use safe `set_i64()` and `.into()` instead of unsafe `as i32`

Reviewed-by: iequidoo
@iequidoo iequidoo changed the base branch from main to pre-main June 26, 2025 21:54
@iequidoo
Copy link
Collaborator

Everything else looks good. I created a pre-main branch so that the PR can be merged (probably, squash-merged) there and then pre-main can be merged to main additionally checking that JSON-RPC tests are working. Meanwhile other contributors may take a look at the PR (particularly at this 8.3 Mpx limit which is nearly optimal for me personally). Many thanks!

@iequidoo iequidoo requested a review from r10s June 26, 2025 22:00
Comment on lines +1992 to +2033
async fn test_huge_image_becomes_file() {
let t = TestContext::new_alice().await;
receive_imf(
&t.ctx,
include_bytes!("../../test-data/message/image_huge_36M.eml"),
false,
)
.await
.unwrap();
let chats = Chatlist::try_load(&t.ctx, 0, None, None).await.unwrap();
let msg_id = chats.get_msg_id(0).unwrap().unwrap();
let msg = Message::load_from_db(&t.ctx, msg_id).await.unwrap();
// Huge image should be treated as file:
assert_eq!(msg.viewtype, Viewtype::File);
assert!(msg.get_file(&t).is_some());
assert_eq!(msg.get_filename().unwrap(), "huge_image.png");
assert_eq!(msg.get_filemime().unwrap(), "image/png");
// File has no width or height
assert_eq!(msg.param.get_int(Param::Width).unwrap_or_default(), 0);
assert_eq!(msg.param.get_int(Param::Height).unwrap_or_default(), 0);
}

#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
async fn test_4k_image_stays_image() {
let t = TestContext::new_alice().await;
receive_imf(
&t.ctx,
include_bytes!("../../test-data/message/image_4k.eml"),
false,
)
.await
.unwrap();
let chats = Chatlist::try_load(&t.ctx, 0, None, None).await.unwrap();
let msg_id = chats.get_msg_id(0).unwrap().unwrap();
let msg = Message::load_from_db(&t.ctx, msg_id).await.unwrap();
// 4K image should be treated as image:
assert_eq!(msg.viewtype, Viewtype::Image);
assert!(msg.get_file(&t).is_some());
assert_eq!(msg.get_filename().unwrap(), "4k_image.png");
assert_eq!(msg.get_filemime().unwrap(), "image/png");
assert_eq!(msg.param.get_int(Param::Width).unwrap_or_default(), 3840);
assert_eq!(msg.param.get_int(Param::Height).unwrap_or_default(), 2160);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
async fn test_huge_image_becomes_file() {
let t = TestContext::new_alice().await;
receive_imf(
&t.ctx,
include_bytes!("../../test-data/message/image_huge_36M.eml"),
false,
)
.await
.unwrap();
let chats = Chatlist::try_load(&t.ctx, 0, None, None).await.unwrap();
let msg_id = chats.get_msg_id(0).unwrap().unwrap();
let msg = Message::load_from_db(&t.ctx, msg_id).await.unwrap();
// Huge image should be treated as file:
assert_eq!(msg.viewtype, Viewtype::File);
assert!(msg.get_file(&t).is_some());
assert_eq!(msg.get_filename().unwrap(), "huge_image.png");
assert_eq!(msg.get_filemime().unwrap(), "image/png");
// File has no width or height
assert_eq!(msg.param.get_int(Param::Width).unwrap_or_default(), 0);
assert_eq!(msg.param.get_int(Param::Height).unwrap_or_default(), 0);
}
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
async fn test_4k_image_stays_image() {
let t = TestContext::new_alice().await;
receive_imf(
&t.ctx,
include_bytes!("../../test-data/message/image_4k.eml"),
false,
)
.await
.unwrap();
let chats = Chatlist::try_load(&t.ctx, 0, None, None).await.unwrap();
let msg_id = chats.get_msg_id(0).unwrap().unwrap();
let msg = Message::load_from_db(&t.ctx, msg_id).await.unwrap();
// 4K image should be treated as image:
assert_eq!(msg.viewtype, Viewtype::Image);
assert!(msg.get_file(&t).is_some());
assert_eq!(msg.get_filename().unwrap(), "4k_image.png");
assert_eq!(msg.get_filemime().unwrap(), "image/png");
assert_eq!(msg.param.get_int(Param::Width).unwrap_or_default(), 3840);
assert_eq!(msg.param.get_int(Param::Height).unwrap_or_default(), 2160);
async fn test_huge_image_becomes_file() -> Result<()> {
let t = TestContext::new_alice().await;
let msg_id = receive_imf(
&t,
include_bytes!("../../test-data/message/image_huge_36M.eml"),
false,
)
.await?
.unwrap()
.msg_ids[0];
let msg = Message::load_from_db(&t.ctx, msg_id).await.unwrap();
// Huge image should be treated as file:
assert_eq!(msg.viewtype, Viewtype::File);
assert!(msg.get_file(&t).is_some());
assert_eq!(msg.get_filename().unwrap(), "huge_image.png");
assert_eq!(msg.get_filemime().unwrap(), "image/png");
// File has no width or height
assert_eq!(msg.param.get_int(Param::Width).unwrap_or_default(), 0);
assert_eq!(msg.param.get_int(Param::Height).unwrap_or_default(), 0);
Ok(())
}
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
async fn test_4k_image_stays_image() -> Result<()> {
let t = TestContext::new_alice().await;
let msg_id = receive_imf(
&t,
include_bytes!("../../test-data/message/image_4k.eml"),
false,
)
.await?
.unwrap()
.msg_ids[0];
let msg = Message::load_from_db(&t.ctx, msg_id).await.unwrap();
// 4K image should be treated as image:
assert_eq!(msg.viewtype, Viewtype::Image);
assert!(msg.get_file(&t).is_some());
assert_eq!(msg.get_filename().unwrap(), "4k_image.png");
assert_eq!(msg.get_filemime().unwrap(), "image/png");
assert_eq!(msg.param.get_int(Param::Width).unwrap_or_default(), 3840);
assert_eq!(msg.param.get_int(Param::Height).unwrap_or_default(), 2160);
Ok(())

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

deal with huge incoming image pixelsizes
2 participants