diff --git a/codex-rs/core/src/tools/handlers/view_image.rs b/codex-rs/core/src/tools/handlers/view_image.rs index ca32af5b1a..78cb2f0720 100644 --- a/codex-rs/core/src/tools/handlers/view_image.rs +++ b/codex-rs/core/src/tools/handlers/view_image.rs @@ -49,6 +49,8 @@ impl ViewImageHandler { const VIEW_IMAGE_UNSUPPORTED_MESSAGE: &str = "view_image is not allowed because you do not support image inputs"; +const VIEW_IMAGE_INVALID_MESSAGE: &str = + "unable to process image: invalid or unsupported image data"; #[derive(Deserialize)] struct ViewImageArgs { @@ -173,6 +175,11 @@ impl ViewImageHandler { "unable to read image at `{model_visible_path}`: {error}" )) })?; + // Reject non-images before their bytes can reach code mode without changing + // valid image bytes, metadata, or centralized image preparation. + image::load_from_memory(&file_bytes).map_err(|_| { + FunctionCallError::RespondToModel(VIEW_IMAGE_INVALID_MESSAGE.to_string()) + })?; let can_request_original_detail = can_request_original_image_detail(&turn.model_info); let use_original_detail = self.options.unified_image_budget @@ -183,7 +190,7 @@ impl ViewImageHandler { DEFAULT_IMAGE_DETAIL }; - // The history insertion path owns image decoding and resizing. + // The history insertion path owns image preparation and resizing. let image_url = data_url_from_bytes("application/octet-stream", &file_bytes); let item = TurnItem::ImageView(ImageViewItem { @@ -262,8 +269,12 @@ mod tests { use codex_utils_absolute_path::AbsolutePathBuf; use codex_utils_path_uri::PathUri; use core_test_support::TempDirExt; + use image::ImageBuffer; + use image::ImageFormat; + use image::Rgba; use pretty_assertions::assert_eq; use serde_json::json; + use std::io::Cursor; use std::sync::Arc; use tokio::sync::Mutex; @@ -284,6 +295,19 @@ mod tests { )); } + fn tiny_png() -> Vec { + let image = ImageBuffer::from_pixel( + /*width*/ 1, + /*height*/ 1, + Rgba([255u8, 0, 0, 255]), + ); + let mut bytes = Vec::new(); + image + .write_to(&mut Cursor::new(&mut bytes), ImageFormat::Png) + .expect("encode test image"); + bytes + } + #[test] fn log_preview_omits_image_data() { let output = ViewImageOutput { @@ -324,7 +348,7 @@ mod tests { replace_primary_environment_cwd(&mut turn, image_cwd.clone()); let image_path = image_cwd.join("image.png"); - std::fs::write(image_path.as_path(), b"not a real image").expect("write test image"); + std::fs::write(image_path.as_path(), tiny_png()).expect("write test image"); Arc::make_mut(&mut turn.config) .permissions .set_permission_profile(PermissionProfile::Disabled) @@ -400,7 +424,7 @@ mod tests { replace_primary_environment_cwd(&mut turn, image_cwd.clone()); let image_path = image_cwd.join("image.png"); - std::fs::write(image_path.as_path(), b"not a real image").expect("write test image"); + std::fs::write(image_path.as_path(), tiny_png()).expect("write test image"); let TurnEnvironmentState::Ready(environment) = &mut turn.environments.environments[0] else { panic!("primary environment should be ready"); @@ -427,4 +451,47 @@ mod tests { result.expect("explicit high detail should be accepted"); } + + #[tokio::test(flavor = "multi_thread")] + async fn handle_rejects_invalid_image_before_returning_output_to_code_mode() { + let (session, mut turn) = make_session_and_context().await; + let image_dir = tempfile::tempdir().expect("create image temp dir"); + let image_cwd = image_dir.abs(); + + replace_primary_environment_cwd(&mut turn, image_cwd.clone()); + let image_path = image_cwd.join("not-an-image.txt"); + std::fs::write(image_path.as_path(), b"arbitrary file contents") + .expect("write invalid image"); + let TurnEnvironmentState::Ready(environment) = &mut turn.environments.environments[0] + else { + panic!("primary environment should be ready"); + }; + environment.config.permission_profile = + PermissionProfileSnapshot::legacy(PermissionProfile::Disabled); + let turn = Arc::new(turn); + + let result = ViewImageHandler::default() + .handle(ToolInvocation { + session: Arc::new(session), + step_context: StepContext::for_test(Arc::clone(&turn)), + turn, + cancellation_token: tokio_util::sync::CancellationToken::new(), + tracker: Arc::new(Mutex::new(TurnDiffTracker::new())), + call_id: "call-view-image".to_string(), + tool_name: codex_tools::ToolName::plain("view_image"), + source: ToolCallSource::CodeMode { + cell_id: "cell-1".to_string(), + runtime_tool_call_id: "tool-1".to_string(), + }, + payload: ToolPayload::Function { + arguments: json!({ "path": "not-an-image.txt" }).to_string(), + }, + }) + .await; + + let Err(FunctionCallError::RespondToModel(message)) = result else { + panic!("expected invalid image error"); + }; + assert_eq!(message, VIEW_IMAGE_INVALID_MESSAGE); + } } diff --git a/codex-rs/core/tests/suite/code_mode.rs b/codex-rs/core/tests/suite/code_mode.rs index 270abe18d1..bb7688ebac 100644 --- a/codex-rs/core/tests/suite/code_mode.rs +++ b/codex-rs/core/tests/suite/code_mode.rs @@ -76,7 +76,13 @@ use core_test_support::wait_for_mcp_server; use image::DynamicImage; use image::GenericImageView; use image::ImageBuffer; +use image::ImageDecoder; +use image::ImageEncoder; +use image::ImageFormat; +use image::ImageReader; use image::Rgba; +use image::codecs::png::PngEncoder; +use image::metadata::Orientation; use pretty_assertions::assert_eq; use serde_json::Value; use std::collections::HashMap; @@ -4038,6 +4044,53 @@ image(s.trim(), "original"); Ok(()) } +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn code_mode_view_image_rejects_invalid_file_without_exposing_contents() -> Result<()> { + skip_if_no_network!(Ok(())); + + const INVALID_IMAGE_CONTENTS: &str = "private-file-contents-must-not-be-exposed"; + + let server = responses::start_mock_server().await; + let builder = test_codex() + .with_model("gpt-5.4") + .with_config(|config| { + let _ = config.features.enable(Feature::CodeMode); + }) + .with_workspace_setup(|cwd, _fs| async move { + fs::write( + cwd.join("not-an-image.txt").as_path(), + INVALID_IMAGE_CONTENTS, + )?; + Ok(()) + }); + let (_test, second_mock) = run_code_mode_turn_with_builder( + &server, + "use exec to call view_image on a non-image file", + r#"await tools.view_image({ path: "not-an-image.txt" });"#, + builder, + ) + .await?; + + let request = second_mock.single_request(); + let (output, success) = custom_tool_output_body_and_success(&request, "call-1"); + assert_ne!( + success, + Some(true), + "code-mode view_image unexpectedly accepted a non-image file" + ); + assert!( + output.contains("unable to process image: invalid or unsupported image data"), + "unexpected code-mode failure: {output}" + ); + let model_visible_output = serde_json::to_string(&request.custom_tool_call_output("call-1"))?; + assert!( + !model_visible_output.contains(INVALID_IMAGE_CONTENTS), + "invalid file contents leaked into model-visible output: {model_visible_output}" + ); + + Ok(()) +} + #[test_case(false; "legacy detail")] #[test_case(true; "unified image budget")] #[tokio::test(flavor = "multi_thread", worker_threads = 2)] @@ -4057,11 +4110,26 @@ async fn code_mode_can_use_view_image_result_with_image_helper( }); let test = builder.build(&server).await?; - let image_bytes = BASE64_STANDARD.decode( - "iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAYAAAAfFcSJAAAADUlEQVR4nGP4z8DwHwAFAAH/iZk9HQAAAABJRU5ErkJggg==", + let image = ImageBuffer::from_pixel( + /*width*/ 2, + /*height*/ 1, + Rgba([255u8, 0, 0, 255]), + ); + let rotate_90_exif = vec![ + 0x49, 0x49, 0x2a, 0x00, 0x08, 0x00, 0x00, 0x00, 0x01, 0x00, 0x12, 0x01, 0x03, 0x00, 0x01, + 0x00, 0x00, 0x00, 0x06, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, + ]; + let mut image_bytes = Vec::new(); + let mut encoder = PngEncoder::new(&mut image_bytes); + encoder.set_exif_metadata(rotate_90_exif.clone())?; + encoder.write_image( + image.as_raw(), + image.width(), + image.height(), + image::ColorType::Rgba8.into(), )?; let image_path = test.cwd_path().join("code_mode_view_image.png"); - fs::write(&image_path, image_bytes)?; + fs::write(&image_path, &image_bytes)?; let image_path_json = serde_json::to_string(&image_path.to_string_lossy().to_string())?; let expected_output_keys = if unified_image_budget { @@ -4129,6 +4197,21 @@ image(out); .and_then(Value::as_str) .expect("image helper should emit an input_image item with image_url"); assert!(emitted_image_url.starts_with("data:image/png;base64,")); + let (_, emitted_image_base64) = emitted_image_url + .split_once(',') + .expect("emitted image should contain a data URL prefix"); + let emitted_image_bytes = BASE64_STANDARD.decode(emitted_image_base64)?; + assert_eq!(emitted_image_bytes, image_bytes); + let mut decoder = ImageReader::with_format(Cursor::new(&emitted_image_bytes), ImageFormat::Png) + .into_decoder()?; + assert_eq!( + ( + decoder.dimensions(), + decoder.orientation()?, + decoder.exif_metadata()? + ), + ((2, 1), Orientation::Rotate90, Some(rotate_90_exif)) + ); assert_eq!( items[1].get("detail").and_then(Value::as_str), Some("original") diff --git a/codex-rs/core/tests/suite/view_image.rs b/codex-rs/core/tests/suite/view_image.rs index ef12dd54c7..fe9928e198 100644 --- a/codex-rs/core/tests/suite/view_image.rs +++ b/codex-rs/core/tests/suite/view_image.rs @@ -1443,7 +1443,7 @@ async fn view_image_tool_errors_when_path_is_directory() -> anyhow::Result<()> { } #[tokio::test(flavor = "multi_thread", worker_threads = 2)] -async fn view_image_tool_turns_invalid_image_into_placeholder() -> anyhow::Result<()> { +async fn view_image_tool_rejects_invalid_image_before_tool_output() -> anyhow::Result<()> { skip_if_no_network!(Ok(())); let server = start_mock_server().await; @@ -1496,12 +1496,13 @@ async fn view_image_tool_turns_invalid_image_into_placeholder() -> anyhow::Resul .await; let request = second_mock.single_request(); + let output_text = request + .function_call_output_content_and_success(call_id) + .and_then(|(content, _)| content) + .context("invalid view_image error text present")?; assert_eq!( - request.function_call_output(call_id).get("output"), - Some(&serde_json::json!([{ - "type": "input_text", - "text": "image content omitted because it could not be processed" - }])) + output_text, + "unable to process image: invalid or unsupported image data" ); Ok(()) }