feat: full image support across all channels - #725
Conversation
github-actions
Bot
added
scope: agent
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly expands the agent's multimodal capabilities by integrating full image support across all communication channels. It allows users to seamlessly upload images, leverage advanced AI tools for image generation, editing, and analysis, and ensures that all image-related interactions are consistently processed and displayed, enhancing the overall user experience and the agent's utility in visual tasks. Highlights
Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces comprehensive image handling capabilities across all channels, a significant and well-executed feature. The changes include new tools for image generation, editing, and analysis, along with the necessary infrastructure in the web gateway, dispatcher, and various channels to support attachments and display generated images. The code is generally of high quality, with good test coverage for the new components. My review focuses on opportunities to improve maintainability by reducing code duplication and ensuring documentation accurately reflects implementation.
Note: Security Review did not run due to the size of the PR.
| fn download_and_store_images(attachments: &[InboundAttachment]) { | ||
| for att in attachments { | ||
| if !att.mime_type.starts_with("image/") { | ||
| continue; | ||
| } | ||
|
|
||
| match download_telegram_file(&att.id) { | ||
| Ok(bytes) => { | ||
| channel_host::log( | ||
| channel_host::LogLevel::Info, | ||
| &format!("Downloaded image file: {} bytes", bytes.len()), | ||
| ); | ||
| if let Err(e) = channel_host::store_attachment_data(&att.id, &bytes) { | ||
| channel_host::log( | ||
| channel_host::LogLevel::Error, | ||
| &format!("Failed to store image data: {}", e), | ||
| ); | ||
| } | ||
| } | ||
| Err(e) => { | ||
| channel_host::log( | ||
| channel_host::LogLevel::Error, | ||
| &format!("Failed to download image file: {}", e), | ||
| ); | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
This function is very similar to download_and_store_voice and download_and_store_documents. Having separate functions for each attachment type leads to code duplication and multiple iterations over the attachments list in handle_message.
To improve maintainability and performance, consider consolidating the download logic into a single function or a single loop in handle_message that dispatches based on attachment type. This would avoid iterating over the attachments multiple times.
References
- Consolidate related sequences of operations, such as creating, persisting, and scheduling a job, into a single reusable method to improve code consistency and maintainability.
| fn media_type_from_path(path: &str) -> &'static str { | ||
| let lower = path.to_lowercase(); | ||
| if lower.ends_with(".png") { | ||
| "image/png" | ||
| } else if lower.ends_with(".gif") { | ||
| "image/gif" | ||
| } else if lower.ends_with(".webp") { | ||
| "image/webp" | ||
| } else { | ||
| "image/jpeg" | ||
| } | ||
| } |
There was a problem hiding this comment.
The function media_type_from_path is duplicated in image_analyze.rs and image_edit.rs. To improve maintainability and avoid potential inconsistencies in the future, this helper function should be extracted to a shared module, for example in src/tools/builtin/path_utils.rs, and called from both tools.
References
- Consolidate related sequences of operations, such as creating, persisting, and scheduling a job, into a single reusable method to improve code consistency and maintainability.
| /// Fallback: use the generation API with the source image described in the prompt. | ||
| async fn fallback_chat_edit( | ||
| &self, | ||
| prompt: &str, | ||
| _b64_image: &str, | ||
| _media_type: &str, | ||
| start: std::time::Instant, | ||
| ) -> Result<ToolOutput, ToolError> { |
There was a problem hiding this comment.
The doc comment for fallback_chat_edit states "use the generation API with the source image described in the prompt." However, the implementation does not seem to do this. It only uses the user-provided prompt for generation, and ignores the source image (_b64_image). This could lead to unexpected results for the user, as the context of the original image is lost in the fallback.
Please either update the comment to reflect the actual behavior (generation from prompt only), or consider enhancing the implementation to incorporate the source image. For example, by first using a vision model to describe it and then prepending that description to the user's prompt.
There was a problem hiding this comment.
Pull request overview
Adds end-to-end image support across IronClaw by introducing image tools (generate/edit/analyze), model capability detection helpers, and channel/UI plumbing to accept inbound images and render tool-generated images across channels.
Changes:
- Add
image_generate,image_edit, andimage_analyzebuilt-in tools plus registry wiring. - Extend web gateway (SSE/WS + frontend) to upload images (base64-in-JSON) and render generated images via a sentinel event.
- Add image/vision model detection helpers and propagate a new
StatusUpdate::ImageGeneratedto channels.
Reviewed changes
Copilot reviewed 24 out of 26 changed files in this pull request and generated 16 comments.
Show a summary per file
| File | Description |
|---|---|
| src/tools/registry.rs | Protect image tool names; add helper registration methods for image/vision tools. |
| src/tools/builtin/mod.rs | Export new image tool modules and tool types. |
| src/tools/builtin/image_gen.rs | New image generation tool calling /v1/images/generations and returning sentinel JSON. |
| src/tools/builtin/image_edit.rs | New image edit tool using multipart /v1/images/edits with generation fallback. |
| src/tools/builtin/image_analyze.rs | New vision analysis tool sending image content via chat-completions multimodal payload. |
| src/llm/vision_models.rs | Add vision-capable model name detection and suggestion. |
| src/llm/mod.rs | Export new image_models and vision_models modules. |
| src/llm/image_models.rs | Add image-generation model name detection and suggestion. |
| src/channels/web/ws.rs | Accept images in WS messages and convert to attachments. |
| src/channels/web/types.rs | Add ImageData, include images on requests, and add SSE image_generated event type. |
| src/channels/web/static/style.css | Add styles for upload button, preview strip, and generated image card. |
| src/channels/web/static/index.html | Add hidden file input, attach button, and preview strip in chat input. |
| src/channels/web/static/app.js | Implement image staging (file + paste), previews, sending, and rendering generated images from SSE. |
| src/channels/web/sse.rs | Add SSE event name mapping for image_generated. |
| src/channels/web/server.rs | Convert base64 images to IncomingAttachments and attach to chat messages. |
| src/channels/web/mod.rs | Map StatusUpdate::ImageGenerated to SSE ImageGenerated. |
| src/channels/wasm/wrapper.rs | Represent ImageGenerated as a status message for WASM channels. |
| src/channels/repl.rs | Print [image] status for generated images in REPL. |
| src/channels/http.rs | Add webhook attachments with base64 decode + size validation; increase body limit. |
| src/channels/channel.rs | Add StatusUpdate::ImageGenerated. |
| src/app.rs | Register image/vision tools opportunistically based on API credentials and suggested models. |
| src/agent/dispatcher.rs | Detect sentinel JSON from tool output and broadcast ImageGenerated status. |
| channels-src/telegram/src/lib.rs | Download/store inbound Telegram images for host-side processing. |
| channels-src/slack/src/lib.rs | Download/store Slack files via url_private using host-injected credentials. |
| channels-src/slack/Cargo.lock | Bump slack-channel version in lockfile. |
| .gitignore | Ignore trace JSON artifacts. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let request_body = serde_json::json!({ | ||
| "model": self.model, | ||
| "messages": [{ | ||
| "role": "user", | ||
| "content": [ | ||
| { | ||
| "type": "text", | ||
| "text": question | ||
| }, | ||
| { | ||
| "type": "image_url", | ||
| "image_url": { | ||
| "url": data_url | ||
| } | ||
| } | ||
| ] | ||
| }], | ||
| "max_tokens": 2048 | ||
| }); |
There was a problem hiding this comment.
serde_json::json! is attempting to move self.model out of &self ("model": self.model), which won’t compile. Use &self.model or self.model.clone() for the request payload.
| /// Convert web gateway `ImageData` to `IncomingAttachment` objects. | ||
| pub(crate) fn images_to_attachments(images: &[ImageData]) -> Vec<crate::channels::IncomingAttachment> { | ||
| use base64::Engine; | ||
| images | ||
| .iter() | ||
| .enumerate() | ||
| .filter_map(|(i, img)| { | ||
| let data = base64::engine::general_purpose::STANDARD | ||
| .decode(&img.data) | ||
| .ok()?; | ||
| Some(crate::channels::IncomingAttachment { | ||
| id: format!("web-image-{i}"), | ||
| kind: crate::channels::AttachmentKind::Image, | ||
| mime_type: img.media_type.clone(), | ||
| filename: Some(format!("image-{i}.{}", mime_to_ext(&img.media_type))), | ||
| size_bytes: Some(data.len() as u64), | ||
| source_url: None, | ||
| storage_key: None, | ||
| extracted_text: None, | ||
| data, | ||
| duration_secs: None, | ||
| }) | ||
| }) | ||
| .collect() |
There was a problem hiding this comment.
images_to_attachments silently drops images with invalid base64 (decode(..).ok()?), meaning the request succeeds but attachments disappear with no client-visible error. It also applies no attachment count/size/MIME validation, which makes the gateway susceptible to large-memory allocations and oversized multimodal payloads. Consider validating and returning a 4xx on decode failure, enforcing limits (count + total decoded bytes), and restricting MIME types similar to the WASM channel attachment validation.
| function handleImageFiles(files) { | ||
| Array.from(files).forEach(file => { | ||
| if (!file.type.startsWith('image/')) return; | ||
| const reader = new FileReader(); | ||
| reader.onload = function(e) { | ||
| const dataUrl = e.target.result; | ||
| const commaIdx = dataUrl.indexOf(','); | ||
| const meta = dataUrl.substring(0, commaIdx); // e.g. "data:image/png;base64" | ||
| const base64 = dataUrl.substring(commaIdx + 1); | ||
| const mediaType = meta.replace('data:', '').replace(';base64', ''); | ||
| stagedImages.push({ media_type: mediaType, data: base64, dataUrl: dataUrl }); | ||
| renderImagePreviews(); | ||
| }; | ||
| reader.readAsDataURL(file); | ||
| }); |
There was a problem hiding this comment.
handleImageFiles stages images with no client-side limits (count, per-file size, total size). Large images or many pasted files can cause high memory usage in the browser and will likely exceed server body limits (gateway is currently limited to 1 MB). Consider enforcing reasonable caps and showing a user-visible error when exceeded.
| } | ||
|
|
||
| let media_type = Self::media_type_from_path(image_path); | ||
| let b64_image = base64::engine::general_purpose::STANDARD.encode(image_bytes); |
There was a problem hiding this comment.
b64_image is computed but never used (the fallback method currently ignores the base64 image argument). This is unnecessary work and allocates a full base64 copy of the image; either remove the encoding or update the fallback to actually use it.
| let b64_image = base64::engine::general_purpose::STANDARD.encode(image_bytes); |
| let sentinel = serde_json::json!({ | ||
| "type": "image_generated", | ||
| "data": format!("data:image/png;base64,{}", edited_data), | ||
| "media_type": "image/png", | ||
| "prompt": prompt, | ||
| "source_path": image_path | ||
| }); | ||
|
|
||
| Ok(ToolOutput::text(sentinel.to_string(), start.elapsed())) |
There was a problem hiding this comment.
The tool returns a full base64 data URL inside the tool output string. Since tool outputs are recorded in thread history and broadcast as ToolResult previews, this will inflate storage and status payload sizes. Prefer returning a small sentinel payload (e.g., {type, path/storage_key, media_type}) and keep the raw image bytes out of the tool result channel.
| // Return sentinel JSON for image display | ||
| let sentinel = serde_json::json!({ | ||
| "type": "image_generated", | ||
| "data": format!("data:image/png;base64,{}", image_data), | ||
| "media_type": "image/png", | ||
| "prompt": prompt, | ||
| "size": size | ||
| }); | ||
|
|
||
| Ok(ToolOutput::text(sentinel.to_string(), start.elapsed())) |
There was a problem hiding this comment.
The tool returns a full base64 data URL inside the tool output string. Tool outputs are recorded into the thread history (record_tool_result) and also broadcast as ToolResult previews, so this will significantly bloat persisted conversation state and status traffic. Consider storing the generated image in attachment/blob storage (or workspace) and returning a small sentinel/pointer (e.g., path/storage key) instead of embedding the full base64 payload in the tool output.
| /// Download file bytes and store them via the host for processing. | ||
| /// | ||
| /// Downloads all file types (images, documents, etc.) so the host-side | ||
| /// middleware can process them (vision pipeline for images, text extraction | ||
| /// for documents, transcription for audio, etc.). | ||
| fn download_and_store_slack_files(attachments: &[InboundAttachment]) { | ||
| for att in attachments { | ||
| let Some(ref url) = att.source_url else { | ||
| continue; | ||
| }; | ||
|
|
||
| match download_slack_file(url) { | ||
| Ok(bytes) => { | ||
| channel_host::log( | ||
| channel_host::LogLevel::Info, | ||
| &format!( | ||
| "Downloaded Slack file: {} bytes, mime={}", | ||
| bytes.len(), | ||
| att.mime_type | ||
| ), | ||
| ); | ||
| if let Err(e) = channel_host::store_attachment_data(&att.id, &bytes) { | ||
| channel_host::log( | ||
| channel_host::LogLevel::Error, | ||
| &format!("Failed to store Slack file data: {}", e), | ||
| ); | ||
| } | ||
| } | ||
| Err(e) => { | ||
| channel_host::log( | ||
| channel_host::LogLevel::Error, | ||
| &format!("Failed to download Slack file: {}", e), | ||
| ); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Slack attachments are downloaded unconditionally; if att.size_bytes is present and exceeds the host’s per-attachment storage limit, this will still download the entire file only to have store_attachment_data fail. Consider skipping downloads above the maximum storable size (or adding a conservative cap when size is unknown) to reduce bandwidth/memory risk inside the WASM component.
| // Convert uploaded images to IncomingAttachments | ||
| if !req.images.is_empty() { | ||
| let attachments = images_to_attachments(&req.images); | ||
| msg = msg.with_attachments(attachments); | ||
| } |
There was a problem hiding this comment.
chat_send_handler now accepts base64 images in the JSON body, but the web gateway router is still configured with a 1 MB DefaultBodyLimit (see earlier in this file). Even a single modest image will often exceed that once base64-encoded, causing 413 responses. The body limit (and/or upload approach) needs to be adjusted to make this feature usable.
| /// Download image file bytes and store them via the host for the vision pipeline. | ||
| /// | ||
| /// Separated from `extract_attachments` so that function stays pure (no host | ||
| /// calls) and remains testable in native unit tests. | ||
| fn download_and_store_images(attachments: &[InboundAttachment]) { | ||
| for att in attachments { | ||
| if !att.mime_type.starts_with("image/") { | ||
| continue; | ||
| } | ||
|
|
||
| match download_telegram_file(&att.id) { | ||
| Ok(bytes) => { | ||
| channel_host::log( | ||
| channel_host::LogLevel::Info, | ||
| &format!("Downloaded image file: {} bytes", bytes.len()), | ||
| ); | ||
| if let Err(e) = channel_host::store_attachment_data(&att.id, &bytes) { | ||
| channel_host::log( | ||
| channel_host::LogLevel::Error, | ||
| &format!("Failed to store image data: {}", e), | ||
| ); | ||
| } | ||
| } | ||
| Err(e) => { | ||
| channel_host::log( | ||
| channel_host::LogLevel::Error, | ||
| &format!("Failed to download image file: {}", e), | ||
| ); | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Telegram image attachments are downloaded unconditionally. If att.size_bytes is available (or can be derived from Telegram metadata), consider skipping downloads above the host’s per-attachment storage limit to avoid unnecessary bandwidth/memory use when the host will reject oversized store_attachment_data calls anyway.
| let request_body = serde_json::json!({ | ||
| "model": self.model, | ||
| "prompt": prompt, | ||
| "size": "1024x1024", | ||
| "response_format": "b64_json", | ||
| "n": 1 | ||
| }); |
There was a problem hiding this comment.
serde_json::json! is attempting to move self.model out of &self ("model": self.model), which won’t compile because String is not Copy. Use a borrowed value (e.g., &self.model) or clone it for the request body.
zmanian
left a comment
zmanian
left a comment
There was a problem hiding this comment.
Overview
Good coverage across channels (web, webhook, WASM/Telegram/Slack, REPL) with backwards-compatible serialization (#[serde(default)] on all new fields). The model detection modules (image_models.rs, vision_models.rs) are well-tested.
However, there are two functional bugs and several security concerns that should be addressed.
Must Fix
1. Binary image read via String is broken. ImageAnalyzeTool and ImageEditTool read images via workspace.read(), which returns MemoryDocument.content: String. Binary image data gets corrupted by UTF-8 encoding -- these tools are non-functional for real images. Either add binary storage to workspace, or read images directly via tokio::fs::read() with path validation.
2. API keys stored as plain String. All three image tools store api_key: String instead of secrecy::SecretString. The app.rs wiring already calls expose_secret() to extract it, but the tools should use SecretString internally to prevent keys appearing in debug logs or core dumps.
Should Fix
3. Sentinel JSON string-matching is fragile. The dispatcher parses every tool output as JSON checking for {"type": "image_generated"}. Any tool that happens to return matching JSON triggers image broadcast. A typed ToolOutput variant (e.g., ToolOutput::Image { data_url, path }) would be more robust and better aligned with the project's security posture.
4. No approval/rate-limiting on image generation. image_generate and image_edit have ApprovalRequirement::Never, but image generation APIs are expensive. Consider UnlessAutoApproved or adding rate limiting.
5. Webhook MAX_BODY_BYTES raised from 64KB to 10MB. This applies to the entire HTTP webhook body, not just attachments. Significantly increases DoS surface. Consider validating attachment presence before allowing large bodies.
6. No size validation on data URLs in SSE broadcast. A generated 1024x1024 PNG could be 2-4MB of base64 flowing through the SSE broadcast system to all connected clients. Add a size cap.
7. image_edit fallback silently generates a new image. When /v1/images/edits returns 404, fallback_chat_edit ignores the source image entirely (_b64_image unused) and generates a completely new image from the prompt. Should at minimum warn the user.
What's Good
- Backwards-compatible serialization across all message types
- No new Cargo dependencies
- Good model detection with comprehensive pattern matching
- WASM channels properly use
channel_host::store_attachment_data()for persistence - Signal channel compiles fine (uses
if let, not exhaustive match) - 17 unit tests across the new modules
- SecretString for API keys in all image tools (image_gen, image_edit, image_analyze) - Binary image read via tokio::fs::read instead of DB-backed workspace.read() - Replace Arc<Workspace> with Option<PathBuf> base_dir (workspace has no filesystem API) - ApprovalRequirement::UnlessAutoApproved for cost-sensitive image tools - Scope sentinel detection to image_generate/image_edit tool names only - Skip ToolResult preview broadcast for image sentinels (avoids multi-MB base64 in SSE) - Extract shared media_type_from_path() to builtin/mod.rs - Rename fallback_chat_edit → fallback_generate with tracing::warn - Increase gateway body limit from 1MB to 10MB for image uploads - Increase webhook body limit to 15MB (base64 overhead) - Log warning on invalid base64 in images_to_attachments - Client-side image size limits (5MB/file, 5 images max) in app.js - aria-label on attach button for accessibility - Update body_too_large test for new 10MB limit [skip-regression-check] Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 29 out of 31 changed files in this pull request and generated 10 comments.
Comments suppressed due to low confidence (1)
src/agent/dispatcher.rs:706
is_image_sentinelonly suppresses the ToolResult preview, but the full tool output is still recorded in the thread, stashed injob_ctx.tool_output_stash, and (after sanitization) added to LLM context. For multi‑MB base64 image payloads this can cause significant memory growth and makes the data retrievable via thejsontool. Consider stripping/replacing the base64 field before recording/stashing (store bytes out-of-band and keep only metadata/path in the recorded result).
// Send ToolResult preview (skip for image sentinels to avoid
// broadcasting multi-MB base64 data as a preview)
if !is_image_sentinel
&& let Ok(ref output) = tool_result
&& !output.is_empty()
{
let _ = self
.channels
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| const img = document.createElement('img'); | ||
| img.className = 'generated-image'; | ||
| img.src = dataUrl; |
There was a problem hiding this comment.
The generated image <img> element is created without an alt attribute, which makes the new inline image rendering inaccessible to screen readers. Provide a meaningful alt (e.g., derived from prompt/path or a generic 'Generated image') and consider similar treatment for preview thumbnails.
| img.src = dataUrl; | |
| img.src = dataUrl; | |
| img.alt = path ? ('Generated image: ' + path) : 'Generated image'; |
| let lower = path.to_lowercase(); | ||
| if lower.ends_with(".png") { | ||
| "image/png" | ||
| } else if lower.ends_with(".gif") { | ||
| "image/gif" | ||
| } else if lower.ends_with(".webp") { | ||
| "image/webp" | ||
| } else if lower.ends_with(".svg") { | ||
| "image/svg+xml" | ||
| } else { | ||
| "image/jpeg" | ||
| } |
There was a problem hiding this comment.
media_type_from_path hardcodes a small extension map and falls back to image/jpeg for unknown types (e.g., .bmp), which can mislabel uploads and cause API errors. The repo already uses mime_guess for extension-based MIME detection; prefer that here (optionally with an allowlist for supported image types) instead of guessing JPEG.
| let lower = path.to_lowercase(); | |
| if lower.ends_with(".png") { | |
| "image/png" | |
| } else if lower.ends_with(".gif") { | |
| "image/gif" | |
| } else if lower.ends_with(".webp") { | |
| "image/webp" | |
| } else if lower.ends_with(".svg") { | |
| "image/svg+xml" | |
| } else { | |
| "image/jpeg" | |
| } | |
| if let Some(mime) = mime_guess::from_path(path).first() { | |
| // Only accept image/* types; fall back to JPEG otherwise. | |
| if mime.type_().as_str() == "image" { | |
| return mime.essence_str(); | |
| } | |
| } | |
| // Default to JPEG when the type cannot be determined as an image. | |
| "image/jpeg" |
| fn download_and_store_images(attachments: &[InboundAttachment]) { | ||
| for att in attachments { | ||
| if !att.mime_type.starts_with("image/") { | ||
| continue; | ||
| } | ||
|
|
||
| match download_telegram_file(&att.id) { | ||
| Ok(bytes) => { | ||
| channel_host::log( | ||
| channel_host::LogLevel::Info, | ||
| &format!("Downloaded image file: {} bytes", bytes.len()), | ||
| ); | ||
| if let Err(e) = channel_host::store_attachment_data(&att.id, &bytes) { | ||
| channel_host::log( | ||
| channel_host::LogLevel::Error, | ||
| &format!("Failed to store image data: {}", e), | ||
| ); | ||
| } |
There was a problem hiding this comment.
download_and_store_images downloads every image/* attachment without checking size_bytes first. Large Telegram images can cause high memory use in the WASM runtime before host-side limits reject them. Add a size pre-check using att.size_bytes (and/or a post-download bytes.len() check) with a clear max (similar to Slack’s 20MB cap) to avoid OOM/slowdowns.
| tools.register_image_tools( | ||
| api_base.clone(), | ||
| api_key.clone(), | ||
| gen_model, | ||
| None, | ||
| ); | ||
|
|
||
| // Check for vision models | ||
| let vision_model = crate::llm::vision_models::suggest_vision_model(&models) | ||
| .unwrap_or(&model_name) | ||
| .to_string(); | ||
| tools.register_vision_tools(api_base, api_key, vision_model, None); | ||
| } |
There was a problem hiding this comment.
register_image_tools / register_vision_tools are called with base_dir=None, which (with the current image_edit/image_analyze implementations) effectively allows arbitrary filesystem reads via image_path. Even if you keep the tools sandboxed internally, wiring a concrete base directory here is important so path validation has a well-defined root (e.g., IronClaw base dir / a dedicated attachments dir).
| images | ||
| .iter() | ||
| .enumerate() | ||
| .filter_map(|(i, img)| { | ||
| let data = match base64::engine::general_purpose::STANDARD.decode(&img.data) { | ||
| Ok(d) => d, | ||
| Err(e) => { | ||
| tracing::warn!("Skipping image {i}: invalid base64 data: {e}"); | ||
| return None; | ||
| } | ||
| }; | ||
| Some(crate::channels::IncomingAttachment { | ||
| id: format!("web-image-{i}"), | ||
| kind: crate::channels::AttachmentKind::Image, | ||
| mime_type: img.media_type.clone(), | ||
| filename: Some(format!("image-{i}.{}", mime_to_ext(&img.media_type))), | ||
| size_bytes: Some(data.len() as u64), | ||
| source_url: None, | ||
| storage_key: None, | ||
| extracted_text: None, | ||
| data, | ||
| duration_secs: None, | ||
| }) | ||
| }) | ||
| .collect() |
There was a problem hiding this comment.
images_to_attachments trusts client-provided media_type and unconditionally sets AttachmentKind::Image, with no server-side limits on image count or decoded size. This allows non-image MIME types (or very large base64 blobs) to enter the attachment pipeline and can cause memory/CPU spikes during base64 decode. Add validation (media_type must start with image/), enforce limits (e.g., max 5 images, 5MB each, 10MB total decoded), and return a 4xx error instead of silently accepting/dropping data.
| images | |
| .iter() | |
| .enumerate() | |
| .filter_map(|(i, img)| { | |
| let data = match base64::engine::general_purpose::STANDARD.decode(&img.data) { | |
| Ok(d) => d, | |
| Err(e) => { | |
| tracing::warn!("Skipping image {i}: invalid base64 data: {e}"); | |
| return None; | |
| } | |
| }; | |
| Some(crate::channels::IncomingAttachment { | |
| id: format!("web-image-{i}"), | |
| kind: crate::channels::AttachmentKind::Image, | |
| mime_type: img.media_type.clone(), | |
| filename: Some(format!("image-{i}.{}", mime_to_ext(&img.media_type))), | |
| size_bytes: Some(data.len() as u64), | |
| source_url: None, | |
| storage_key: None, | |
| extracted_text: None, | |
| data, | |
| duration_secs: None, | |
| }) | |
| }) | |
| .collect() | |
| // Safety limits for image attachments coming from the web gateway. | |
| // These are intentionally conservative to avoid excessive memory/CPU usage. | |
| const MAX_IMAGES: usize = 5; | |
| const MAX_IMAGE_BYTES: usize = 5 * 1024 * 1024; // 5MB per image | |
| const MAX_TOTAL_BYTES: usize = 10 * 1024 * 1024; // 10MB across all images | |
| let mut attachments = Vec::new(); | |
| let mut total_decoded_bytes: usize = 0; | |
| for (i, img) in images.iter().enumerate() { | |
| if attachments.len() >= MAX_IMAGES { | |
| tracing::warn!( | |
| "Reached max image count ({}); skipping remaining images", | |
| MAX_IMAGES | |
| ); | |
| break; | |
| } | |
| // Only allow image media types. | |
| if !img.media_type.starts_with("image/") { | |
| tracing::warn!( | |
| "Skipping image {i}: unsupported media_type '{}'; only image/* allowed", | |
| img.media_type | |
| ); | |
| continue; | |
| } | |
| // Rough estimate of decoded size from base64 length: decoded <= 3/4 of input length. | |
| let estimated_decoded_len = img.data.len().saturating_mul(3) / 4; | |
| if estimated_decoded_len > MAX_IMAGE_BYTES { | |
| tracing::warn!( | |
| "Skipping image {i}: estimated decoded size {} bytes exceeds per-image limit {} bytes", | |
| estimated_decoded_len, | |
| MAX_IMAGE_BYTES | |
| ); | |
| continue; | |
| } | |
| if total_decoded_bytes.saturating_add(estimated_decoded_len) > MAX_TOTAL_BYTES { | |
| tracing::warn!( | |
| "Skipping image {i}: estimated decoded size would exceed total limit ({} bytes)", | |
| MAX_TOTAL_BYTES | |
| ); | |
| continue; | |
| } | |
| let data = match base64::engine::general_purpose::STANDARD.decode(&img.data) { | |
| Ok(d) => d, | |
| Err(e) => { | |
| tracing::warn!("Skipping image {i}: invalid base64 data: {e}"); | |
| continue; | |
| } | |
| }; | |
| if data.len() > MAX_IMAGE_BYTES { | |
| tracing::warn!( | |
| "Skipping image {i}: decoded size {} bytes exceeds per-image limit {} bytes", | |
| data.len(), | |
| MAX_IMAGE_BYTES | |
| ); | |
| continue; | |
| } | |
| if total_decoded_bytes.saturating_add(data.len()) > MAX_TOTAL_BYTES { | |
| tracing::warn!( | |
| "Skipping image {i}: decoded size would exceed total limit ({} bytes)", | |
| MAX_TOTAL_BYTES | |
| ); | |
| continue; | |
| } | |
| total_decoded_bytes = total_decoded_bytes.saturating_add(data.len()); | |
| attachments.push(crate::channels::IncomingAttachment { | |
| id: format!("web-image-{i}"), | |
| kind: crate::channels::AttachmentKind::Image, | |
| mime_type: img.media_type.clone(), | |
| filename: Some(format!("image-{i}.{}", mime_to_ext(&img.media_type))), | |
| size_bytes: Some(data.len() as u64), | |
| source_url: None, | |
| storage_key: None, | |
| extracted_text: None, | |
| data, | |
| duration_secs: None, | |
| }); | |
| } | |
| attachments |
| let resolved = if let Some(base) = &self.base_dir { | ||
| let candidate = base.join(image_path); | ||
| if candidate.exists() { | ||
| candidate | ||
| } else { | ||
| PathBuf::from(image_path) | ||
| } | ||
| } else { | ||
| PathBuf::from(image_path) | ||
| }; | ||
|
|
||
| let canonical = resolved.canonicalize().map_err(|e| { | ||
| ToolError::ExecutionFailed(format!("Image path not found: {e}")) | ||
| })?; | ||
|
|
There was a problem hiding this comment.
read_image_bytes resolves paths by joining base_dir but then canonicalizes without verifying the canonical path stays within the sandbox. This permits .. traversal (and absolute paths when base_dir is None), letting the tool read arbitrary host files and upload them to the image API. Reuse crate::tools::builtin::path_utils::validate_path (like the file/message tools) and enforce that reads are restricted to an allowed base directory (and/or /tmp).
| async fn read_image_bytes(&self, image_path: &str) -> Result<Vec<u8>, ToolError> { | ||
| let resolved = if let Some(base) = &self.base_dir { | ||
| let candidate = base.join(image_path); | ||
| if candidate.exists() { | ||
| candidate | ||
| } else { | ||
| PathBuf::from(image_path) | ||
| } | ||
| } else { | ||
| PathBuf::from(image_path) | ||
| }; | ||
|
|
||
| let canonical = resolved.canonicalize().map_err(|e| { | ||
| ToolError::ExecutionFailed(format!("Image path not found: {e}")) | ||
| })?; | ||
|
|
There was a problem hiding this comment.
read_image_bytes uses canonicalize() but does not enforce that the resulting path is under base_dir, and when base_dir is None it allows reading any relative/absolute path on the host. Since this tool sends the bytes to an external vision API, this is a high-risk exfiltration vector. Use the existing path_utils::validate_path sandboxing approach and reject paths outside the sandbox.
| fn requires_approval(&self, _params: &serde_json::Value) -> ApprovalRequirement { | ||
| ApprovalRequirement::Never | ||
| } | ||
|
|
||
| fn requires_sanitization(&self) -> bool { | ||
| true | ||
| } |
There was a problem hiding this comment.
ImageAnalyzeTool::requires_approval is Never, but the tool uploads local image bytes to an external API endpoint. This is inconsistent with other outbound-network tools (e.g., HttpTool) and can leak sensitive data without an approval gate. Change this to at least ApprovalRequirement::UnlessAutoApproved (or stronger) so image uploads require explicit user consent by default.
| let data_url = sentinel.get("data").and_then(|v| v.as_str()).unwrap_or_default().to_string(); | ||
| let path = sentinel.get("path").and_then(|v| v.as_str()).map(String::from); | ||
| let _ = self | ||
| .channels | ||
| .send_status( | ||
| &message.channel, | ||
| StatusUpdate::ImageGenerated { data_url, path }, | ||
| &message.metadata, | ||
| ) | ||
| .await; | ||
| true |
There was a problem hiding this comment.
When parsing the image sentinel, data_url is built with unwrap_or_default(). If the sentinel is malformed/missing data, this will broadcast an ImageGenerated status with an empty URL, leading to broken UI rendering. Treat missing/empty data as an error (or skip sending ImageGenerated) rather than emitting an invalid event.
| let data_url = sentinel.get("data").and_then(|v| v.as_str()).unwrap_or_default().to_string(); | |
| let path = sentinel.get("path").and_then(|v| v.as_str()).map(String::from); | |
| let _ = self | |
| .channels | |
| .send_status( | |
| &message.channel, | |
| StatusUpdate::ImageGenerated { data_url, path }, | |
| &message.metadata, | |
| ) | |
| .await; | |
| true | |
| if let Some(data_url_str) = sentinel | |
| .get("data") | |
| .and_then(|v| v.as_str()) | |
| .filter(|s| !s.is_empty()) | |
| { | |
| let data_url = data_url_str.to_string(); | |
| let path = sentinel | |
| .get("path") | |
| .and_then(|v| v.as_str()) | |
| .map(String::from); | |
| let _ = self | |
| .channels | |
| .send_status( | |
| &message.channel, | |
| StatusUpdate::ImageGenerated { data_url, path }, | |
| &message.metadata, | |
| ) | |
| .await; | |
| true | |
| } else { | |
| // Missing or empty `data` field; treat as non-image sentinel. | |
| false | |
| } |
| // Skip files that exceed the size limit | ||
| if let Some(size) = att.size_bytes { | ||
| if size > MAX_DOWNLOAD_SIZE_BYTES { | ||
| channel_host::log( | ||
| channel_host::LogLevel::Warn, | ||
| &format!( | ||
| "Skipping Slack file download: {} bytes exceeds {} MB limit (id={})", | ||
| size, | ||
| MAX_DOWNLOAD_SIZE_BYTES / (1024 * 1024), | ||
| att.id | ||
| ), | ||
| ); | ||
| continue; | ||
| } | ||
| } | ||
|
|
||
| match download_slack_file(url) { | ||
| Ok(bytes) => { | ||
| channel_host::log( | ||
| channel_host::LogLevel::Info, | ||
| &format!( | ||
| "Downloaded Slack file: {} bytes, mime={}", | ||
| bytes.len(), | ||
| att.mime_type | ||
| ), | ||
| ); | ||
| if let Err(e) = channel_host::store_attachment_data(&att.id, &bytes) { |
There was a problem hiding this comment.
download_and_store_slack_files only skips downloads when att.size_bytes is present and over the limit. If Slack omits size (or reports incorrectly), this may download arbitrarily large files into WASM memory. Add a post-download check on bytes.len() (and skip/log if it exceeds the limit) to ensure the cap is enforced even when metadata is missing.
End-to-end image handling: upload, generation, analysis, editing, and rendering across web gateway, HTTP webhook, WASM (Telegram/Slack), and REPL channels. Builds on the attachment infrastructure from #596 and draws inspiration from PR #641's image pipeline approach — credit to that PR's author for the sentinel JSON pattern and base64-in-JSON upload design. Key changes: - Image upload in web UI (file picker, paste, preview strip) - Image generation tool (FLUX/DALL-E via /v1/images/generations) - Image edit tool (multipart /v1/images/edits with fallback) - Image analysis tool (vision model for workspace images) - Model detection utilities (image_models.rs, vision_models.rs) - Sentinel JSON detection in dispatcher for generated image rendering - StatusUpdate::ImageGenerated → SSE/WS/REPL/WASM broadcast - HTTP webhook attachment support (base64, 5MB/file, 10MB total) - WASM channel image download (Telegram via file API, Slack via host HTTP) - Tool registration wiring in app.rs [skip-regression-check] Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- SecretString for API keys in all image tools (image_gen, image_edit, image_analyze) - Binary image read via tokio::fs::read instead of DB-backed workspace.read() - Replace Arc<Workspace> with Option<PathBuf> base_dir (workspace has no filesystem API) - ApprovalRequirement::UnlessAutoApproved for cost-sensitive image tools - Scope sentinel detection to image_generate/image_edit tool names only - Skip ToolResult preview broadcast for image sentinels (avoids multi-MB base64 in SSE) - Extract shared media_type_from_path() to builtin/mod.rs - Rename fallback_chat_edit → fallback_generate with tracing::warn - Increase gateway body limit from 1MB to 10MB for image uploads - Increase webhook body limit to 15MB (base64 overhead) - Log warning on invalid base64 in images_to_attachments - Client-side image size limits (5MB/file, 5 images max) in app.js - aria-label on attach button for accessibility - Update body_too_large test for new 10MB limit [skip-regression-check] Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Skip downloading files larger than 20 MB in the Slack WASM channel to avoid excessive memory use and slow downloads in the WASM runtime. Logs a warning when a file is skipped. Also bumps channel versions for Slack and Telegram (prior branch changes). [skip-regression-check] Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
4347695 to
dc44466
Compare
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 29 out of 32 changed files in this pull request and generated 7 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| async fn read_image_bytes(&self, image_path: &str) -> Result<Vec<u8>, ToolError> { | ||
| let resolved = if let Some(base) = &self.base_dir { | ||
| let candidate = base.join(image_path); | ||
| if candidate.exists() { | ||
| candidate | ||
| } else { | ||
| PathBuf::from(image_path) | ||
| } | ||
| } else { | ||
| PathBuf::from(image_path) | ||
| }; | ||
|
|
||
| let canonical = resolved | ||
| .canonicalize() | ||
| .map_err(|e| ToolError::ExecutionFailed(format!("Image path not found: {e}")))?; | ||
|
|
||
| tokio::fs::read(&canonical) | ||
| .await | ||
| .map_err(|e| ToolError::ExecutionFailed(format!("Failed to read image file: {e}"))) | ||
| } |
There was a problem hiding this comment.
ImageEditTool::read_image_bytes() doesn’t enforce that image_path stays within base_dir when provided. Because it canonicalizes whatever path exists, inputs like ../.. (or symlink escapes) can still resolve outside the intended sandbox. Consider switching to path_utils::validate_path(image_path, self.base_dir.as_deref()) (same approach used by file/message tools) and rejecting paths that escape the base directory.
| async fn read_image_bytes(&self, image_path: &str) -> Result<Vec<u8>, ToolError> { | ||
| let resolved = if let Some(base) = &self.base_dir { | ||
| let candidate = base.join(image_path); | ||
| if candidate.exists() { | ||
| candidate | ||
| } else { | ||
| PathBuf::from(image_path) | ||
| } | ||
| } else { | ||
| PathBuf::from(image_path) | ||
| }; | ||
|
|
||
| let canonical = resolved | ||
| .canonicalize() | ||
| .map_err(|e| ToolError::ExecutionFailed(format!("Image path not found: {e}")))?; | ||
|
|
||
| tokio::fs::read(&canonical) | ||
| .await | ||
| .map_err(|e| ToolError::ExecutionFailed(format!("Failed to read image file: {e}"))) | ||
| } |
There was a problem hiding this comment.
read_image_bytes() canonicalizes and reads whatever path the tool is given, but it doesn’t validate that the resolved path stays within base_dir when one is configured (and it also falls back to interpreting the input as an absolute path). This creates a path-traversal/arbitrary-file-read risk. Use crate::tools::builtin::path_utils::validate_path(image_path, self.base_dir.as_deref()) (or equivalent containment check) before reading.
| "data": format!("data:image/png;base64,{}", edited_data), | ||
| "media_type": "image/png", | ||
| "prompt": prompt, | ||
| "source_path": image_path |
There was a problem hiding this comment.
The sentinel JSON emitted by image_edit uses the field source_path, but the dispatcher’s image-sentinel handler looks for path. As a result the UI/status updates will never show the path for edited images. Align on one field name (e.g., emit path here, or have the dispatcher also read source_path).
| "source_path": image_path | |
| "path": image_path |
| // Register image/vision tools if we have a workspace and LLM API credentials | ||
| if workspace.is_some() { | ||
| let (api_base, api_key_opt) = if let Some(ref provider) = self.config.llm.provider { | ||
| ( | ||
| provider.base_url.clone(), | ||
| provider.api_key.as_ref().map(|s| { | ||
| use secrecy::ExposeSecret; | ||
| s.expose_secret().to_string() | ||
| }), | ||
| ) | ||
| } else { | ||
| ( | ||
| self.config.llm.nearai.base_url.clone(), | ||
| self.config.llm.nearai.api_key.as_ref().map(|s| { | ||
| use secrecy::ExposeSecret; | ||
| s.expose_secret().to_string() | ||
| }), | ||
| ) | ||
| }; | ||
|
|
||
| if let Some(api_key) = api_key_opt { | ||
| // Check for image generation models | ||
| let model_name = self | ||
| .config | ||
| .llm | ||
| .provider | ||
| .as_ref() | ||
| .map(|p| p.model.clone()) | ||
| .unwrap_or_else(|| self.config.llm.nearai.model.clone()); | ||
| let models = vec![model_name.clone()]; | ||
| let gen_model = crate::llm::image_models::suggest_image_model(&models) | ||
| .unwrap_or("flux-1.1-pro") | ||
| .to_string(); | ||
| tools.register_image_tools(api_base.clone(), api_key.clone(), gen_model, None); | ||
|
|
||
| // Check for vision models | ||
| let vision_model = crate::llm::vision_models::suggest_vision_model(&models) | ||
| .unwrap_or(&model_name) | ||
| .to_string(); | ||
| tools.register_vision_tools(api_base, api_key, vision_model, None); | ||
| } |
There was a problem hiding this comment.
register_image_tools() / register_vision_tools() are called with base_dir = None, but both ImageEditTool and ImageAnalyzeTool accept an optional base directory and currently interpret relative paths against the process CWD. This is both confusing (schema says “workspace path”) and increases the risk of unintended file reads. Consider passing an explicit sandbox base dir (and making the tools enforce it) or removing the filesystem-path feature entirely.
| images | ||
| .iter() | ||
| .enumerate() | ||
| .filter_map(|(i, img)| { | ||
| let data = match base64::engine::general_purpose::STANDARD.decode(&img.data) { | ||
| Ok(d) => d, | ||
| Err(e) => { | ||
| tracing::warn!("Skipping image {i}: invalid base64 data: {e}"); | ||
| return None; | ||
| } | ||
| }; | ||
| Some(crate::channels::IncomingAttachment { | ||
| id: format!("web-image-{i}"), | ||
| kind: crate::channels::AttachmentKind::Image, | ||
| mime_type: img.media_type.clone(), | ||
| filename: Some(format!("image-{i}.{}", mime_to_ext(&img.media_type))), | ||
| size_bytes: Some(data.len() as u64), | ||
| source_url: None, | ||
| storage_key: None, | ||
| extracted_text: None, | ||
| data, | ||
| duration_secs: None, | ||
| }) | ||
| }) | ||
| .collect() |
There was a problem hiding this comment.
The gateway now accepts base64 images but images_to_attachments() has no server-side enforcement of attachment count, per-image size, total decoded size, or even that media_type is actually an image/*. Client-side limits are easy to bypass, and base64 decode allocates eagerly. Consider adding the same limits/pattern used by the HTTP webhook channel (max 5 files, 5MB each, 10MB total decoded) and reject/return an error instead of silently skipping/accepting oversized inputs.
| images | |
| .iter() | |
| .enumerate() | |
| .filter_map(|(i, img)| { | |
| let data = match base64::engine::general_purpose::STANDARD.decode(&img.data) { | |
| Ok(d) => d, | |
| Err(e) => { | |
| tracing::warn!("Skipping image {i}: invalid base64 data: {e}"); | |
| return None; | |
| } | |
| }; | |
| Some(crate::channels::IncomingAttachment { | |
| id: format!("web-image-{i}"), | |
| kind: crate::channels::AttachmentKind::Image, | |
| mime_type: img.media_type.clone(), | |
| filename: Some(format!("image-{i}.{}", mime_to_ext(&img.media_type))), | |
| size_bytes: Some(data.len() as u64), | |
| source_url: None, | |
| storage_key: None, | |
| extracted_text: None, | |
| data, | |
| duration_secs: None, | |
| }) | |
| }) | |
| .collect() | |
| // Limits aligned with HTTP webhook channel: | |
| // - max 5 files | |
| // - max 5MB per file | |
| // - max 10MB total decoded | |
| const MAX_IMAGES: usize = 5; | |
| const MAX_IMAGE_BYTES: usize = 5 * 1024 * 1024; | |
| const MAX_TOTAL_BYTES: usize = 10 * 1024 * 1024; | |
| let mut attachments = Vec::new(); | |
| let mut total_bytes: usize = 0; | |
| for (i, img) in images.iter().enumerate() { | |
| if attachments.len() >= MAX_IMAGES { | |
| tracing::warn!( | |
| "Skipping image {i}: maximum number of images ({MAX_IMAGES}) already processed" | |
| ); | |
| break; | |
| } | |
| // Enforce that media_type is an image. | |
| if !img.media_type.starts_with("image/") { | |
| tracing::warn!( | |
| "Skipping image {i}: unsupported media_type (expected image/*, got {media_type})", | |
| media_type = img.media_type | |
| ); | |
| continue; | |
| } | |
| // Rough upper bound estimate of decoded size from base64 length. | |
| // Base64 inflates by ~4/3; conservatively estimate decoded size as: | |
| let estimated_len = img.data.len().saturating_mul(3) / 4; | |
| if estimated_len > MAX_IMAGE_BYTES { | |
| tracing::warn!( | |
| "Skipping image {i}: estimated decoded size {estimated_len} bytes exceeds per-image limit {MAX_IMAGE_BYTES} bytes" | |
| ); | |
| continue; | |
| } | |
| if total_bytes.saturating_add(estimated_len) > MAX_TOTAL_BYTES { | |
| tracing::warn!( | |
| "Skipping image {i}: estimated decoded size would exceed total limit {MAX_TOTAL_BYTES} bytes" | |
| ); | |
| continue; | |
| } | |
| let data = match base64::engine::general_purpose::STANDARD.decode(&img.data) { | |
| Ok(d) => d, | |
| Err(e) => { | |
| tracing::warn!("Skipping image {i}: invalid base64 data: {e}"); | |
| continue; | |
| } | |
| }; | |
| // Final check on actual decoded size (in case estimate was low). | |
| if data.len() > MAX_IMAGE_BYTES { | |
| tracing::warn!( | |
| "Skipping image {i}: decoded size {} bytes exceeds per-image limit {MAX_IMAGE_BYTES} bytes", | |
| data.len() | |
| ); | |
| continue; | |
| } | |
| if total_bytes.saturating_add(data.len()) > MAX_TOTAL_BYTES { | |
| tracing::warn!( | |
| "Skipping image {i}: decoded size would exceed total limit {MAX_TOTAL_BYTES} bytes" | |
| ); | |
| continue; | |
| } | |
| total_bytes = total_bytes.saturating_add(data.len()); | |
| attachments.push(crate::channels::IncomingAttachment { | |
| id: format!("web-image-{i}"), | |
| kind: crate::channels::AttachmentKind::Image, | |
| mime_type: img.media_type.clone(), | |
| filename: Some(format!("image-{i}.{}", mime_to_ext(&img.media_type))), | |
| size_bytes: Some(data.len() as u64), | |
| source_url: None, | |
| storage_key: None, | |
| extracted_text: None, | |
| data, | |
| duration_secs: None, | |
| }); | |
| } | |
| attachments |
| .merge(projects) | ||
| .merge(protected) | ||
| .layer(DefaultBodyLimit::max(1024 * 1024)) // 1 MB max request body | ||
| .layer(DefaultBodyLimit::max(10 * 1024 * 1024)) // 10 MB max request body (image uploads) |
There was a problem hiding this comment.
DefaultBodyLimit was raised to 10MB for image uploads, but the frontend allows up to 5 images × 5MB each and base64 adds ~33% overhead. In practice many “allowed” client uploads will fail with 413, and decoded bytes can be far less than expected. Consider aligning server body limit + server-side validation with the client limits (or lowering the client limits to match).
| .layer(DefaultBodyLimit::max(10 * 1024 * 1024)) // 10 MB max request body (image uploads) | |
| .layer(DefaultBodyLimit::max(40 * 1024 * 1024)) // 40 MB max request body (up to 5 × 5MB base64-encoded images) |
| const MAX_IMAGE_SIZE_BYTES = 5 * 1024 * 1024; // 5 MB per image | ||
| const MAX_STAGED_IMAGES = 5; | ||
|
|
||
| function handleImageFiles(files) { | ||
| Array.from(files).forEach(file => { | ||
| if (!file.type.startsWith('image/')) return; | ||
| if (file.size > MAX_IMAGE_SIZE_BYTES) { | ||
| alert(`Image "${file.name}" exceeds 5 MB limit (${(file.size / 1024 / 1024).toFixed(1)} MB)`); | ||
| return; | ||
| } | ||
| if (stagedImages.length >= MAX_STAGED_IMAGES) { | ||
| alert(`Maximum ${MAX_STAGED_IMAGES} images allowed per message`); | ||
| return; |
There was a problem hiding this comment.
Frontend staging limits allow 5 images at 5MB each (MAX_STAGED_IMAGES/MAX_IMAGE_SIZE_BYTES), but the server request DefaultBodyLimit is 10MB and base64 inflation makes the JSON payload even larger. This will cause confusing 413 failures for users even when the UI says the selection is valid. Consider enforcing a total-bytes cap in the UI that matches the server (or increase the server limit to match the UI).
zmanian
left a comment
zmanian
left a comment
There was a problem hiding this comment.
Re-review: REQUEST CHANGES
The fix commits addressed many of the original 16 issues (SecretString, binary reads via tokio::fs, sentinel scoping, body limits, etc.) -- good progress. However, several security and correctness issues remain, some pre-existing from the original commit and some newly flagged by Copilot that were not addressed. I have grouped them by severity.
BLOCKING: Security Issues
1. Path traversal in read_image_bytes (image_analyze.rs, image_edit.rs)
Both ImageAnalyzeTool::read_image_bytes() and ImageEditTool::read_image_bytes() allow arbitrary file reads. The path is joined with base_dir and canonicalized, but there is no containment check -- .. traversal or absolute paths escape the sandbox. Worse, base_dir is None when registered in app.rs, so any absolute or relative path works.
This is a high-risk exfiltration vector: the LLM can be prompt-injected to call image_analyze with image_path: "/etc/passwd" or "~/.ironclaw/secrets.json" and the file bytes get sent to an external API.
Fix required: Use crate::tools::builtin::path_utils::validate_path() (already exists in the codebase and is used by file/message tools). Pass a real base_dir in app.rs when registering (e.g., the workspace root or ~/.ironclaw). Both tools must reject paths outside the sandbox.
2. ImageAnalyzeTool::requires_approval returns Never
This tool reads local files and uploads them to an external API endpoint. Every other tool that makes outbound network calls (HttpTool, image_generate, image_edit) requires approval. requires_approval returning Never means the agent can exfiltrate file contents to the vision API without any user consent gate.
Fix required: Change to ApprovalRequirement::UnlessAutoApproved (consistent with image_generate and image_edit).
Important: Correctness Issues
3. Slack file size check is bypassable
In download_and_store_slack_files(), the size check only triggers when att.size_bytes is Some. If Slack omits the size metadata (or it is zero), the download proceeds unconditionally. A post-download bytes.len() check should be added as a safety net.
4. Telegram image downloads have no size check at all
download_and_store_images() in telegram/src/lib.rs downloads every image/* attachment without any size check (unlike the Slack channel which has the 20MB cap). Large Telegram images could cause high memory use or OOM in the WASM runtime.
5. data_url sentinel with unwrap_or_default()
In dispatcher.rs, when parsing the image sentinel, if data is missing the code uses unwrap_or_default() which produces an empty string. This broadcasts an ImageGenerated event with an empty data_url, resulting in a broken <img> tag in the web UI. Either skip broadcasting or treat as an error.
Minor / Nit
6. media_type_from_path hardcoding
The codebase has mime_guess available. The hardcoded match (which maps .bmp to image/jpeg) is fragile. Not blocking but worth switching.
7. Missing alt attribute on generated images
addGeneratedImage() in app.js creates <img> elements without alt. Low priority but simple accessibility fix.
8. images_to_attachments trusts client-provided media_type
In server.rs, the gateway's images_to_attachments() does not validate that media_type actually starts with image/. Non-image MIME types could enter the attachment pipeline. The function already has validation for base64 decode errors but should also validate the MIME type.
Summary
Items 1-2 are security blockers that must be fixed before merge. Items 3-5 are correctness issues that should be addressed. Items 6-8 are lower priority.
The pattern fix from item 1 is important: read_image_bytes appears in both image_analyze.rs and image_edit.rs with identical vulnerable code. Both must be fixed.
…tools Add sandbox path validation via validate_path() to both ImageAnalyzeTool and ImageEditTool to prevent path traversal attacks that could exfiltrate arbitrary files through external vision/edit APIs. Also fix ImageAnalyzeTool::requires_approval to return UnlessAutoApproved, consistent with ImageEditTool and ImageGenerateTool. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Slack: add post-download size check on actual bytes when metadata size_bytes is absent, preventing bypass of the 20MB limit - Telegram: add 20MB download size limit (matching Slack) enforced in download_telegram_file() after receiving response bytes - Dispatcher: skip broadcasting ImageGenerated SSE event when data_url is empty from unwrap_or_default(), log warning instead Closes correctness issues #3, #4, #5 from PR #725 review. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…_type validation - Replace hardcoded media type mapping with mime_guess crate (already in deps) - Add alt attributes to img elements in web UI for accessibility - Validate media_type starts with "image/" in images_to_attachments() - Update bmp test assertion to match mime_guess behavior Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
zmanian
left a comment
zmanian
left a comment
There was a problem hiding this comment.
All 8 review items addressed:
- Path traversal →
validate_path()added to both image tools requires_approval→UnlessAutoApprovedon all three image tools- Slack file size → post-download
bytes.len()guard added - Telegram size → download size cap added
- Empty
data_urlsentinel → checked before broadcast - Media type detection → switched to
mime_guesscrate - Alt attributes → added to both
<img>elements - Media type validation →
images_to_attachments()rejects non-image/types
CI is green (27/27). Ready to merge.
CLAUDE.md:200 still documented the pre-nearai#725 body limit of 1 MB, but server.rs:354 was changed to 10 MB in nearai#725 (image upload support). Update the documentation to match the actual production value.
* fix(test): stabilize openai compat oversized-body regression * docs(web): fix stale body limit in CLAUDE.md (1 MB → 10 MB) CLAUDE.md:200 still documented the pre-#725 body limit of 1 MB, but server.rs:354 was changed to 10 MB in #725 (image upload support). Update the documentation to match the actual production value.
* feat: full image support across all channels End-to-end image handling: upload, generation, analysis, editing, and rendering across web gateway, HTTP webhook, WASM (Telegram/Slack), and REPL channels. Builds on the attachment infrastructure from nearai#596 and draws inspiration from PR nearai#641's image pipeline approach — credit to that PR's author for the sentinel JSON pattern and base64-in-JSON upload design. Key changes: - Image upload in web UI (file picker, paste, preview strip) - Image generation tool (FLUX/DALL-E via /v1/images/generations) - Image edit tool (multipart /v1/images/edits with fallback) - Image analysis tool (vision model for workspace images) - Model detection utilities (image_models.rs, vision_models.rs) - Sentinel JSON detection in dispatcher for generated image rendering - StatusUpdate::ImageGenerated → SSE/WS/REPL/WASM broadcast - HTTP webhook attachment support (base64, 5MB/file, 10MB total) - WASM channel image download (Telegram via file API, Slack via host HTTP) - Tool registration wiring in app.rs [skip-regression-check] Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: address PR nearai#725 review comments (16 issues) - SecretString for API keys in all image tools (image_gen, image_edit, image_analyze) - Binary image read via tokio::fs::read instead of DB-backed workspace.read() - Replace Arc<Workspace> with Option<PathBuf> base_dir (workspace has no filesystem API) - ApprovalRequirement::UnlessAutoApproved for cost-sensitive image tools - Scope sentinel detection to image_generate/image_edit tool names only - Skip ToolResult preview broadcast for image sentinels (avoids multi-MB base64 in SSE) - Extract shared media_type_from_path() to builtin/mod.rs - Rename fallback_chat_edit → fallback_generate with tracing::warn - Increase gateway body limit from 1MB to 10MB for image uploads - Increase webhook body limit to 15MB (base64 overhead) - Log warning on invalid base64 in images_to_attachments - Client-side image size limits (5MB/file, 5 images max) in app.js - aria-label on attach button for accessibility - Update body_too_large test for new 10MB limit [skip-regression-check] Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: add Slack file size check before download (PR review item nearai#15) Skip downloading files larger than 20 MB in the Slack WASM channel to avoid excessive memory use and slow downloads in the WASM runtime. Logs a warning when a file is skipped. Also bumps channel versions for Slack and Telegram (prior branch changes). [skip-regression-check] Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * style: cargo fmt Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(security): add path validation and approval requirement to image tools Add sandbox path validation via validate_path() to both ImageAnalyzeTool and ImageEditTool to prevent path traversal attacks that could exfiltrate arbitrary files through external vision/edit APIs. Also fix ImageAnalyzeTool::requires_approval to return UnlessAutoApproved, consistent with ImageEditTool and ImageGenerateTool. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: post-download size guards and empty data_url sentinel check - Slack: add post-download size check on actual bytes when metadata size_bytes is absent, preventing bypass of the 20MB limit - Telegram: add 20MB download size limit (matching Slack) enforced in download_telegram_file() after receiving response bytes - Dispatcher: skip broadcasting ImageGenerated SSE event when data_url is empty from unwrap_or_default(), log warning instead Closes correctness issues nearai#3, nearai#4, nearai#5 from PR nearai#725 review. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: use mime_guess for media type detection, add alt attrs and media_type validation - Replace hardcoded media type mapping with mime_guess crate (already in deps) - Add alt attributes to img elements in web UI for accessibility - Validate media_type starts with "image/" in images_to_attachments() - Update bmp test assertion to match mime_guess behavior Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com> Co-authored-by: Zaki <zaki@iqlusion.io>
) * fix(test): stabilize openai compat oversized-body regression * docs(web): fix stale body limit in CLAUDE.md (1 MB → 10 MB) CLAUDE.md:200 still documented the pre-nearai#725 body limit of 1 MB, but server.rs:354 was changed to 10 MB in nearai#725 (image upload support). Update the documentation to match the actual production value.
* feat: full image support across all channels End-to-end image handling: upload, generation, analysis, editing, and rendering across web gateway, HTTP webhook, WASM (Telegram/Slack), and REPL channels. Builds on the attachment infrastructure from nearai#596 and draws inspiration from PR nearai#641's image pipeline approach — credit to that PR's author for the sentinel JSON pattern and base64-in-JSON upload design. Key changes: - Image upload in web UI (file picker, paste, preview strip) - Image generation tool (FLUX/DALL-E via /v1/images/generations) - Image edit tool (multipart /v1/images/edits with fallback) - Image analysis tool (vision model for workspace images) - Model detection utilities (image_models.rs, vision_models.rs) - Sentinel JSON detection in dispatcher for generated image rendering - StatusUpdate::ImageGenerated → SSE/WS/REPL/WASM broadcast - HTTP webhook attachment support (base64, 5MB/file, 10MB total) - WASM channel image download (Telegram via file API, Slack via host HTTP) - Tool registration wiring in app.rs [skip-regression-check] Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: address PR nearai#725 review comments (16 issues) - SecretString for API keys in all image tools (image_gen, image_edit, image_analyze) - Binary image read via tokio::fs::read instead of DB-backed workspace.read() - Replace Arc<Workspace> with Option<PathBuf> base_dir (workspace has no filesystem API) - ApprovalRequirement::UnlessAutoApproved for cost-sensitive image tools - Scope sentinel detection to image_generate/image_edit tool names only - Skip ToolResult preview broadcast for image sentinels (avoids multi-MB base64 in SSE) - Extract shared media_type_from_path() to builtin/mod.rs - Rename fallback_chat_edit → fallback_generate with tracing::warn - Increase gateway body limit from 1MB to 10MB for image uploads - Increase webhook body limit to 15MB (base64 overhead) - Log warning on invalid base64 in images_to_attachments - Client-side image size limits (5MB/file, 5 images max) in app.js - aria-label on attach button for accessibility - Update body_too_large test for new 10MB limit [skip-regression-check] Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: add Slack file size check before download (PR review item nearai#15) Skip downloading files larger than 20 MB in the Slack WASM channel to avoid excessive memory use and slow downloads in the WASM runtime. Logs a warning when a file is skipped. Also bumps channel versions for Slack and Telegram (prior branch changes). [skip-regression-check] Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * style: cargo fmt Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(security): add path validation and approval requirement to image tools Add sandbox path validation via validate_path() to both ImageAnalyzeTool and ImageEditTool to prevent path traversal attacks that could exfiltrate arbitrary files through external vision/edit APIs. Also fix ImageAnalyzeTool::requires_approval to return UnlessAutoApproved, consistent with ImageEditTool and ImageGenerateTool. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: post-download size guards and empty data_url sentinel check - Slack: add post-download size check on actual bytes when metadata size_bytes is absent, preventing bypass of the 20MB limit - Telegram: add 20MB download size limit (matching Slack) enforced in download_telegram_file() after receiving response bytes - Dispatcher: skip broadcasting ImageGenerated SSE event when data_url is empty from unwrap_or_default(), log warning instead Closes correctness issues nearai#3, nearai#4, nearai#5 from PR nearai#725 review. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: use mime_guess for media type detection, add alt attrs and media_type validation - Replace hardcoded media type mapping with mime_guess crate (already in deps) - Add alt attributes to img elements in web UI for accessibility - Validate media_type starts with "image/" in images_to_attachments() - Update bmp test assertion to match mime_guess behavior Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com> Co-authored-by: Zaki <zaki@iqlusion.io>
) * fix(test): stabilize openai compat oversized-body regression * docs(web): fix stale body limit in CLAUDE.md (1 MB → 10 MB) CLAUDE.md:200 still documented the pre-nearai#725 body limit of 1 MB, but server.rs:354 was changed to 10 MB in nearai#725 (image upload support). Update the documentation to match the actual production value.
Summary
End-to-end image handling across every IronClaw channel. Builds on the attachment infrastructure from #596 and draws inspiration from PR #641's image pipeline design — credit to that PR's author for the sentinel JSON pattern and base64-in-JSON upload approach.
image_generate): calls/v1/images/generations(FLUX/DALL-E), returns sentinel JSON rendered as inline<img>in the web UIimage_edit): multipart/v1/images/editswith fallback to generation endpointimage_analyze): sends workspace images to a vision model for description/analysisimage_models.rsandvision_models.rsdetect generation/vision capabilities from model names{"type":"image_generated"}triggersStatusUpdate::ImageGenerated→ broadcast to all channelsattachmentsfield onWebhookRequestwith base64 decode, size validation (5MB/file, 10MB total, max 5)ImageGenerated: web (SSE/WS inline image), REPL ([image] path), WASM (status message)register_image_tools()andregister_vision_tools()called inapp.rswhen API credentials are availableFiles changed (26 files, +1700/-20)
image_gen.rs,image_edit.rs,image_analyze.rsimage_models.rs,vision_models.rschannel.rs,dispatcher.rs,repl.rs,wrapper.rstypes.rs,server.rs,ws.rs,sse.rs,mod.rsindex.html,app.js,style.csshttp.rstelegram/src/lib.rs,slack/src/lib.rsapp.rs,registry.rs,llm/mod.rs,tools/builtin/mod.rsTest plan
cargo clippy --all --all-features— zero warningscargo test— 2600+ tests pass (0 failures)wasm32-wasip1and pass testscurlwith base64 image attachment to HTTP webhook → agent describes itRelates to #641, #596
🤖 Generated with Claude Code