fix: address PR #1889 review feedback for vision support

- Guard against decompression bombs in ResizeImage by rejecting decoded
  images exceeding 20000x20000 pixels before processing
- Fix image stripping for models with unknown capabilities: only strip
  images when modalities are explicitly known and exclude image input
- Check file existence before calling IsImageFile in read_file handler
  to provide clearer errors and avoid type detection on missing files

Assisted-By: cagent

Author: dgageot

pkg/app/app.go

@@ -408,8 +408,10 @@ func (a *App) processFileAttachment(ctx context.Context, att messages.Attachment
 			}
 			resized, resizeErr := chat.ResizeImage(imgData, mimeType)
 			if resizeErr != nil {
-				slog.Warn("image resize failed, sending original", "path", absPath, "error", resizeErr)
-				resized = &chat.ImageResizeResult{Data: imgData, MimeType: mimeType}
+				// Don't bypass security checks - reject the file if resize failed
+				slog.Warn("skipping attachment: image resize failed", "path", absPath, "error", resizeErr)
+				a.sendEvent(ctx, runtime.Warning(fmt.Sprintf("Skipped attachment %s: %s", att.Name, resizeErr), ""))
+				return
 			}
 			dataURL := fmt.Sprintf("data:%s;base64,%s", resized.MimeType, base64.StdEncoding.EncodeToString(resized.Data))
 			*binaryParts = append(*binaryParts, chat.MessagePart{

pkg/chat/image.go

@@ -22,6 +22,10 @@ const (
 	// MaxImageBytes is the maximum file size for images sent to LLM providers (4.5MB,
 	// below Anthropic's 5MB limit).
 	MaxImageBytes = 4_500_000
+	// maxDecodedDimension is the absolute upper bound on decoded image dimensions.
+	// Images exceeding this are rejected before processing to guard against
+	// decompression bombs (small files that expand to huge pixel buffers).
+	maxDecodedDimension = 20_000
 	// jpegQuality is the default JPEG encoding quality.
 	jpegQuality = 80
 )
@@ -75,6 +79,13 @@ func ResizeImage(data []byte, mimeType string) (*ImageResizeResult, error) {
 	bounds := img.Bounds()
 	origW, origH := bounds.Dx(), bounds.Dy()
 
+	// Guard against decompression bombs: reject images whose decoded
+	// dimensions are absurdly large. A small compressed file can expand
+	// to hundreds of megabytes in memory (e.g. 20000×20000×4 ≈ 1.6 GB).
+	if origW > maxDecodedDimension || origH > maxDecodedDimension {
+		return nil, fmt.Errorf("image dimensions too large: %dx%d (max %d)", origW, origH, maxDecodedDimension)
+	}
+
 	// If the image already fits within all limits, return unchanged.
 	if origW <= MaxImageDimension && origH <= MaxImageDimension && len(data) <= MaxImageBytes {
 		return &ImageResizeResult{
@@ -103,6 +114,7 @@ func ResizeImage(data []byte, mimeType string) (*ImageResizeResult, error) {
 		for _, q := range []int{70, 55, 40} {
 			encoded, err := encodeJPEG(resized, q)
 			if err != nil {
+				slog.Debug("JPEG encoding failed", "quality", q, "error", err)
 				continue
 			}
 
@@ -131,6 +143,7 @@ func ResizeImage(data []byte, mimeType string) (*ImageResizeResult, error) {
 			for _, q := range []int{80, 55, 40} {
 				encoded, err := encodeJPEG(smaller, q)
 				if err != nil {
+					slog.Debug("JPEG encoding failed", "quality", q, "scale", scale, "error", err)
 					continue
 				}
 
@@ -150,8 +163,7 @@ func ResizeImage(data []byte, mimeType string) (*ImageResizeResult, error) {
 	}
 
 	if len(best) > MaxImageBytes {
-		slog.Warn("Image still exceeds size limit after all resize attempts",
-			"original_size", len(data), "final_size", len(best), "limit", MaxImageBytes)
+		return nil, fmt.Errorf("image exceeds size limit after all resize attempts: %d bytes (limit %d)", len(best), MaxImageBytes)
 	}
 
 	return &ImageResizeResult{
@@ -166,34 +178,51 @@ func ResizeImage(data []byte, mimeType string) (*ImageResizeResult, error) {
 }
 
 // ResizeImageBase64 is a convenience wrapper around ResizeImage that accepts
-// and returns base64-encoded image data.
-func ResizeImageBase64(b64Data, mimeType string) (*ImageResizeResult, error) {
+// and returns base64-encoded image data. The base64-encoded result is returned
+// separately to avoid mutating the ImageResizeResult.Data field.
+func ResizeImageBase64(b64Data, mimeType string) (b64Result string, metadata *ImageResizeResult, err error) {
 	raw, err := base64.StdEncoding.DecodeString(b64Data)
 	if err != nil {
-		return nil, fmt.Errorf("decode base64: %w", err)
+		return "", nil, fmt.Errorf("decode base64: %w", err)
 	}
 	result, err := ResizeImage(raw, mimeType)
 	if err != nil {
-		return nil, err
+		return "", nil, err
 	}
-	result.Data = []byte(base64.StdEncoding.EncodeToString(result.Data))
-	return result, nil
+	return base64.StdEncoding.EncodeToString(result.Data), result, nil
 }
 
 // FormatDimensionNote produces a human-readable note describing the resize mapping.
 // This helps the model translate coordinates from the resized image back to the original.
+//
+// Because ResizeImage uses fitDimensions (which preserves aspect ratio), the X and
+// Y scale factors are always equal in practice. If they ever differ (e.g. because
+// the function is called with a manually constructed ImageResizeResult), we emit
+// separate per-axis factors so that coordinate mapping remains correct.
 func FormatDimensionNote(r *ImageResizeResult) string {
 	if !r.Resized {
 		return ""
 	}
 	scaleX := float64(r.OriginalWidth) / float64(r.Width)
 	scaleY := float64(r.OriginalHeight) / float64(r.Height)
-	scale := scaleX
-	if scaleY > scaleX {
-		scale = scaleY
+
+	// Uniform scaling (the normal path): a single factor suffices.
+	const epsilon = 0.01
+	if abs(scaleX-scaleY) < epsilon {
+		return fmt.Sprintf("[Image: original %dx%d, displayed at %dx%d. Multiply coordinates by %.2f to map to original image.]",
+			r.OriginalWidth, r.OriginalHeight, r.Width, r.Height, scaleX)
+	}
+
+	// Non-uniform scaling: provide separate X and Y factors.
+	return fmt.Sprintf("[Image: original %dx%d, displayed at %dx%d. Multiply X coordinates by %.2f and Y coordinates by %.2f to map to original image.]",
+		r.OriginalWidth, r.OriginalHeight, r.Width, r.Height, scaleX, scaleY)
+}
+
+func abs(x float64) float64 {
+	if x < 0 {
+		return -x
 	}
-	return fmt.Sprintf("[Image: original %dx%d, displayed at %dx%d. Multiply coordinates by %.2f to map to original image.]",
-		r.OriginalWidth, r.OriginalHeight, r.Width, r.Height, scale)
+	return x
 }
 
 // fitDimensions returns new dimensions that fit within maxW×maxH while

pkg/chat/image_test.go

@@ -146,7 +146,7 @@ func TestFormatDimensionNote(t *testing.T) {
 		assert.Empty(t, FormatDimensionNote(result))
 	})
 
-	t.Run("resized", func(t *testing.T) {
+	t.Run("uniform scaling", func(t *testing.T) {
 		t.Parallel()
 		result := &ImageResizeResult{
 			Resized:        true,
@@ -158,7 +158,26 @@ func TestFormatDimensionNote(t *testing.T) {
 		note := FormatDimensionNote(result)
 		assert.Contains(t, note, "original 4000x3000")
 		assert.Contains(t, note, "displayed at 2000x1500")
-		assert.Contains(t, note, "2.00")
+		assert.Contains(t, note, "Multiply coordinates by 2.00")
+		// Should NOT contain separate X/Y factors.
+		assert.NotContains(t, note, "X coordinates")
+	})
+
+	t.Run("non-uniform scaling", func(t *testing.T) {
+		t.Parallel()
+		// Manually constructed result with different X and Y scale factors.
+		result := &ImageResizeResult{
+			Resized:        true,
+			OriginalWidth:  4000,
+			OriginalHeight: 2000,
+			Width:          2000,
+			Height:         2000,
+		}
+		note := FormatDimensionNote(result)
+		assert.Contains(t, note, "original 4000x2000")
+		assert.Contains(t, note, "displayed at 2000x2000")
+		assert.Contains(t, note, "Multiply X coordinates by 2.00")
+		assert.Contains(t, note, "Y coordinates by 1.00")
 	})
 }
 
@@ -168,9 +187,11 @@ func TestResizeImageBase64(t *testing.T) {
 	data := createTestPNG(t, 100, 100)
 	b64 := base64.StdEncoding.EncodeToString(data)
 
-	result, err := ResizeImageBase64(b64, "image/png")
+	b64Result, result, err := ResizeImageBase64(b64, "image/png")
 	require.NoError(t, err)
 	assert.False(t, result.Resized)
-	// Result.Data is base64-encoded
-	assert.Equal(t, b64, string(result.Data))
+	// Result data stays as raw bytes
+	assert.Equal(t, data, result.Data)
+	// Base64 result is returned separately
+	assert.Equal(t, b64, b64Result)
 }

pkg/runtime/runtime.go

@@ -1121,7 +1121,7 @@ func (r *LocalRuntime) RunStream(ctx context.Context, sess *session.Session) <-c
 			// Strip image content from messages if the model doesn't support image input.
 			// This prevents API errors when conversation history contains images (e.g. from
 			// tool results or user attachments) but the current model is text-only.
-			if m != nil && !slices.Contains(m.Modalities.Input, "image") {
+			if m != nil && len(m.Modalities.Input) > 0 && !slices.Contains(m.Modalities.Input, "image") {
 				messages = stripImageContent(messages)
 			}
 

pkg/tools/builtin/filesystem.go

@@ -520,12 +520,8 @@ func (t *FilesystemTool) handleListDirectory(_ context.Context, args ListDirecto
 func (t *FilesystemTool) handleReadFile(_ context.Context, args ReadFileArgs) (*tools.ToolCallResult, error) {
 	resolvedPath := t.resolvePath(args.Path)
 
-	// Check if the file is an image
-	if chat.IsImageFile(resolvedPath) {
-		return t.readImageFile(resolvedPath, args.Path)
-	}
-
-	content, err := os.ReadFile(resolvedPath)
+	// Check if the file exists before any type detection.
+	info, err := os.Stat(resolvedPath)
 	if err != nil {
 		var errMsg string
 		if os.IsNotExist(err) {
@@ -543,6 +539,22 @@ func (t *FilesystemTool) handleReadFile(_ context.Context, args ReadFileArgs) (*
 		}, nil
 	}
 
+	// Only check for image files on regular files (not directories, etc.)
+	if info.Mode().IsRegular() && chat.IsImageFile(resolvedPath) {
+		return t.readImageFile(resolvedPath, args.Path)
+	}
+
+	content, err := os.ReadFile(resolvedPath)
+	if err != nil {
+		return &tools.ToolCallResult{
+			Output:  err.Error(),
+			IsError: true,
+			Meta: ReadFileMeta{
+				Error: err.Error(),
+			},
+		}, nil
+	}
+
 	return &tools.ToolCallResult{
 		Output: string(content),
 		Meta: ReadFileMeta{
@@ -575,8 +587,16 @@ func (t *FilesystemTool) readImageFile(resolvedPath, originalPath string) (*tool
 	// Resize the image if it exceeds provider limits (max 2000×2000, max 4.5MB).
 	resized, err := chat.ResizeImage(data, mimeType)
 	if err != nil {
-		// If resize fails, fall back to sending the original.
-		slog.Warn("Image resize failed, sending original", "path", originalPath, "error", err)
+		// Check if the original exceeds limits before falling back
+		if len(data) > chat.MaxImageBytes {
+			return &tools.ToolCallResult{
+				Output:  fmt.Sprintf("Error: Image file too large (%d bytes, max %d bytes)", len(data), chat.MaxImageBytes),
+				IsError: true,
+				Meta:    ReadFileMeta{Path: originalPath, Error: "image too large"},
+			}, nil
+		}
+		// Original is within limits, proceed with fallback
+		slog.Warn("Image resize failed, sending original (within limits)", "path", originalPath, "error", err)
 		encoded := base64.StdEncoding.EncodeToString(data)
 		return &tools.ToolCallResult{
 			Output: fmt.Sprintf("Read image file %s [%s] (%d bytes)", originalPath, mimeType, len(data)),