Merge pull request #2251 from code-yeongyu/fix/pr-1906-image-conversion

fix(look-at): temp dir cleanup, Windows compat, argument injection prevention

src/tools/look-at/image-converter.test.ts

@@ -1,11 +1,11 @@
 import { describe, expect, test, mock, beforeEach } from "bun:test"
 import { existsSync, mkdtempSync, writeFileSync, unlinkSync, rmSync } from "node:fs"
 import { tmpdir } from "node:os"
-import { join } from "node:path"
+import { dirname, join } from "node:path"
 
 const originalChildProcess = await import("node:child_process")
 
-const execFileSyncMock = mock((_command: string, _args: string[]) => "")
+const execFileSyncMock = mock((_command: string, _args: string[], _options?: unknown) => "")
 const execSyncMock = mock(() => {
   throw new Error("execSync should not be called")
 })
@@ -16,7 +16,44 @@ mock.module("node:child_process", () => ({
   execSync: execSyncMock,
 }))
 
-const { convertImageToJpeg } = await import("./image-converter")
+const { convertImageToJpeg, cleanupConvertedImage } = await import("./image-converter")
+
+function writeConvertedOutput(command: string, args: string[]): void {
+  if (command === "sips") {
+    const outIndex = args.indexOf("--out")
+    const outputPath = outIndex >= 0 ? args[outIndex + 1] : undefined
+    if (outputPath) {
+      writeFileSync(outputPath, "jpeg")
+    }
+    return
+  }
+
+  if (command === "convert") {
+    writeFileSync(args[2], "jpeg")
+    return
+  }
+
+  if (command === "magick") {
+    writeFileSync(args[2], "jpeg")
+  }
+}
+
+function withMockPlatform<TValue>(platform: NodeJS.Platform, run: () => TValue): TValue {
+  const originalPlatform = process.platform
+  Object.defineProperty(process, "platform", {
+    value: platform,
+    configurable: true,
+  })
+
+  try {
+    return run()
+  } finally {
+    Object.defineProperty(process, "platform", {
+      value: originalPlatform,
+      configurable: true,
+    })
+  }
+}
 
 describe("image-converter command execution safety", () => {
   beforeEach(() => {
@@ -30,13 +67,7 @@ describe("image-converter command execution safety", () => {
     writeFileSync(inputPath, "fake-heic-data")
 
     execFileSyncMock.mockImplementation((command: string, args: string[]) => {
-      if (command === "sips") {
+      writeConvertedOutput(command, args)
-        const outIndex = args.indexOf("--out")
-        const outputPath = outIndex >= 0 ? args[outIndex + 1] : undefined
-        if (outputPath) writeFileSync(outputPath, "jpeg")
-      } else if (command === "convert") {
-        writeFileSync(args[1], "jpeg")
-      }
       return ""
     })
 
@@ -48,7 +79,10 @@ describe("image-converter command execution safety", () => {
     const [firstCommand, firstArgs] = execFileSyncMock.mock.calls[0] as [string, string[]]
     expect(typeof firstCommand).toBe("string")
     expect(Array.isArray(firstArgs)).toBe(true)
+    expect(["sips", "convert", "magick"]).toContain(firstCommand)
+    expect(firstArgs).toContain("--")
     expect(firstArgs).toContain(inputPath)
+    expect(firstArgs.indexOf("--") < firstArgs.indexOf(inputPath)).toBe(true)
     expect(firstArgs.join(" ")).not.toContain(`\"${inputPath}\"`)
 
     expect(existsSync(outputPath)).toBe(true)
@@ -57,4 +91,102 @@ describe("image-converter command execution safety", () => {
     if (existsSync(inputPath)) unlinkSync(inputPath)
     rmSync(testDir, { recursive: true, force: true })
   })
+
+  test("removes temporary conversion directory during cleanup", () => {
+    const testDir = mkdtempSync(join(tmpdir(), "img-converter-cleanup-test-"))
+    const inputPath = join(testDir, "photo.heic")
+    writeFileSync(inputPath, "fake-heic-data")
+
+    execFileSyncMock.mockImplementation((command: string, args: string[]) => {
+      writeConvertedOutput(command, args)
+      return ""
+    })
+
+    const outputPath = convertImageToJpeg(inputPath, "image/heic")
+    const conversionDirectory = dirname(outputPath)
+
+    expect(existsSync(conversionDirectory)).toBe(true)
+
+    cleanupConvertedImage(outputPath)
+
+    expect(existsSync(conversionDirectory)).toBe(false)
+
+    if (existsSync(inputPath)) unlinkSync(inputPath)
+    rmSync(testDir, { recursive: true, force: true })
+  })
+
+  test("uses magick command on non-darwin platforms to avoid convert.exe collision", () => {
+    withMockPlatform("linux", () => {
+      const testDir = mkdtempSync(join(tmpdir(), "img-converter-platform-test-"))
+      const inputPath = join(testDir, "photo.heic")
+      writeFileSync(inputPath, "fake-heic-data")
+
+      execFileSyncMock.mockImplementation((command: string, args: string[]) => {
+        if (command === "magick") {
+          writeFileSync(args[2], "jpeg")
+        }
+        return ""
+      })
+
+      const outputPath = convertImageToJpeg(inputPath, "image/heic")
+
+      const [command, args] = execFileSyncMock.mock.calls[0] as [string, string[]]
+      expect(command).toBe("magick")
+      expect(args).toContain("--")
+      expect(args.indexOf("--") < args.indexOf(inputPath)).toBe(true)
+      expect(existsSync(outputPath)).toBe(true)
+
+      cleanupConvertedImage(outputPath)
+      if (existsSync(inputPath)) unlinkSync(inputPath)
+      rmSync(testDir, { recursive: true, force: true })
+    })
+  })
+
+  test("applies timeout when executing conversion commands", () => {
+    const testDir = mkdtempSync(join(tmpdir(), "img-converter-timeout-test-"))
+    const inputPath = join(testDir, "photo.heic")
+    writeFileSync(inputPath, "fake-heic-data")
+
+    execFileSyncMock.mockImplementation((command: string, args: string[]) => {
+      writeConvertedOutput(command, args)
+      return ""
+    })
+
+    const outputPath = convertImageToJpeg(inputPath, "image/heic")
+
+    const options = execFileSyncMock.mock.calls[0]?.[2] as { timeout?: number } | undefined
+    expect(options).toBeDefined()
+    expect(typeof options?.timeout).toBe("number")
+    expect((options?.timeout ?? 0) > 0).toBe(true)
+
+    cleanupConvertedImage(outputPath)
+    if (existsSync(inputPath)) unlinkSync(inputPath)
+    rmSync(testDir, { recursive: true, force: true })
+  })
+
+  test("attaches temporary output path to conversion errors", () => {
+    withMockPlatform("linux", () => {
+      const testDir = mkdtempSync(join(tmpdir(), "img-converter-failure-test-"))
+      const inputPath = join(testDir, "photo.heic")
+      writeFileSync(inputPath, "fake-heic-data")
+
+      execFileSyncMock.mockImplementation(() => {
+        throw new Error("conversion process failed")
+      })
+
+      const runConversion = () => convertImageToJpeg(inputPath, "image/heic")
+      expect(runConversion).toThrow("No image conversion tool available")
+
+      try {
+        runConversion()
+      } catch (error) {
+        const conversionError = error as Error & { temporaryOutputPath?: string }
+        expect(conversionError.temporaryOutputPath).toBeDefined()
+        expect(conversionError.temporaryOutputPath?.endsWith("converted.jpg")).toBe(true)
+      }
+
+      if (existsSync(inputPath)) unlinkSync(inputPath)
+      rmSync(testDir, { recursive: true, force: true })
+    })
+  })
 })

src/tools/look-at/image-converter.ts

@@ -1,7 +1,7 @@
 import { execFileSync } from "node:child_process"
-import { existsSync, mkdtempSync, unlinkSync, writeFileSync, readFileSync } from "node:fs"
+import { existsSync, mkdtempSync, readFileSync, rmSync, unlinkSync, writeFileSync } from "node:fs"
 import { tmpdir } from "node:os"
-import { join } from "node:path"
+import { dirname, join } from "node:path"
 import { log } from "../../shared"
 
 const SUPPORTED_FORMATS = new Set([
@@ -32,6 +32,8 @@ const UNSUPPORTED_FORMATS = new Set([
   "image/x-photoshop",
 ])
 
+const CONVERSION_TIMEOUT_MS = 30_000
+
 export function needsConversion(mimeType: string): boolean {
   if (SUPPORTED_FORMATS.has(mimeType)) {
     return false
@@ -57,9 +59,10 @@ export function convertImageToJpeg(inputPath: string, mimeType: string): string
   try {
     if (process.platform === "darwin") {
       try {
-        execFileSync("sips", ["-s", "format", "jpeg", inputPath, "--out", outputPath], {
+        execFileSync("sips", ["-s", "format", "jpeg", "--", inputPath, "--out", outputPath], {
           stdio: "pipe",
           encoding: "utf-8",
+          timeout: CONVERSION_TIMEOUT_MS,
         })
         
         if (existsSync(outputPath)) {
@@ -72,9 +75,11 @@ export function convertImageToJpeg(inputPath: string, mimeType: string): string
     }
 
     try {
-      execFileSync("convert", [inputPath, outputPath], {
+      const imagemagickCommand = process.platform === "darwin" ? "convert" : "magick"
+      execFileSync(imagemagickCommand, ["--", inputPath, outputPath], {
         stdio: "pipe",
         encoding: "utf-8",
+        timeout: CONVERSION_TIMEOUT_MS,
       })
       
       if (existsSync(outputPath)) {
@@ -97,6 +102,11 @@ export function convertImageToJpeg(inputPath: string, mimeType: string): string
         unlinkSync(outputPath)
       }
     } catch {}
+
+    if (error instanceof Error) {
+      const conversionError = error as Error & { temporaryOutputPath?: string }
+      conversionError.temporaryOutputPath = outputPath
+    }
     
     throw error
   }
@@ -104,10 +114,15 @@ export function convertImageToJpeg(inputPath: string, mimeType: string): string
 
 export function cleanupConvertedImage(filePath: string): void {
   try {
+    const tempDirectory = dirname(filePath)
     if (existsSync(filePath)) {
       unlinkSync(filePath)
       log(`[image-converter] Cleaned up temporary file: ${filePath}`)
     }
+    if (existsSync(tempDirectory)) {
+      rmSync(tempDirectory, { recursive: true, force: true })
+      log(`[image-converter] Cleaned up temporary directory: ${tempDirectory}`)
+    }
   } catch (error) {
     log(`[image-converter] Failed to cleanup ${filePath}: ${error}`)
   }

src/tools/look-at/tools.ts

@@ -20,6 +20,24 @@ import {
   cleanupConvertedImage,
 } from "./image-converter"
 
+function getTemporaryConversionPath(error: unknown): string | null {
+  if (!(error instanceof Error)) {
+    return null
+  }
+
+  const temporaryOutputPath = Reflect.get(error, "temporaryOutputPath")
+  if (typeof temporaryOutputPath === "string" && temporaryOutputPath.length > 0) {
+    return temporaryOutputPath
+  }
+
+  const temporaryDirectory = Reflect.get(error, "temporaryDirectory")
+  if (typeof temporaryDirectory === "string" && temporaryDirectory.length > 0) {
+    return temporaryDirectory
+  }
+
+  return null
+}
+
 export { normalizeArgs, validateArgs } from "./look-at-arguments"
 
 export function createLookAt(ctx: PluginInput): ToolDefinition {
@@ -48,6 +66,7 @@ export function createLookAt(ctx: PluginInput): ToolDefinition {
       let mimeType: string
       let filePart: { type: "file"; mime: string; url: string; filename: string }
       let tempFilePath: string | null = null
+      let tempConversionPath: string | null = null
       let tempFilesToCleanup: string[] = []
 
       try {
@@ -85,10 +104,15 @@ export function createLookAt(ctx: PluginInput): ToolDefinition {
           log(`[look_at] Detected unsupported format: ${mimeType}, converting to JPEG...`)
           try {
             tempFilePath = convertImageToJpeg(filePath, mimeType)
+            tempConversionPath = tempFilePath
             actualFilePath = tempFilePath
             mimeType = "image/jpeg"
             log(`[look_at] Conversion successful: ${tempFilePath}`)
           } catch (conversionError) {
+            const failedConversionPath = getTemporaryConversionPath(conversionError)
+            if (failedConversionPath) {
+              tempConversionPath = failedConversionPath
+            }
             log(`[look_at] Conversion failed: ${conversionError}`)
             return `Error: Failed to convert image format. ${conversionError}`
           }
@@ -194,10 +218,14 @@ Original error: ${createResult.error}`
         log(`[look_at] Got response, length: ${responseText.length}`)
         return responseText
       } finally {
-        if (tempFilePath) {
+        if (tempConversionPath) {
+          cleanupConvertedImage(tempConversionPath)
+        } else if (tempFilePath) {
           cleanupConvertedImage(tempFilePath)
         }
-        tempFilesToCleanup.forEach(file => cleanupConvertedImage(file))
+        tempFilesToCleanup.forEach(file => {
+          cleanupConvertedImage(file)
+        })
       }
     },
   })