From 17dea6ad795b03769bf58bb40403900627dcfc98 Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Sun, 8 Jun 2025 08:16:39 +0100 Subject: [PATCH] fix: Prevent security vulnerability from malformed app targets MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses critical edge case where malformed app targets with multiple leading colons (e.g., "::::::::::::::::Finder") created empty app names that would match ALL system processes. This could potentially expose sensitive information or cause unintended system-wide captures. Key improvements: - Enhanced app target parsing to validate non-empty app names - Added fallback logic to extract valid app names from malformed inputs - Default to screen mode when all parts are empty (security-first approach) - Comprehensive test coverage for edge cases - Improved backward compatibility with hidden path parameters 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude --- .../Sources/peekaboo/FileNameGenerator.swift | 38 +++++ .../Sources/peekaboo/ImageSaver.swift | 47 ++++++ .../peekaboo/PermissionErrorDetector.swift | 35 ++++ src/tools/analyze.ts | 25 +-- src/tools/image.ts | 20 +-- src/tools/list.ts | 10 +- src/utils/image-cli-args.ts | 69 +++++--- tests/unit/tools/analyze.test.ts | 46 ++++++ tests/unit/tools/image.test.ts | 42 +++++ tests/unit/utils/malformed-app-target.test.ts | 149 ++++++++++++++++++ 10 files changed, 436 insertions(+), 45 deletions(-) create mode 100644 peekaboo-cli/Sources/peekaboo/FileNameGenerator.swift create mode 100644 peekaboo-cli/Sources/peekaboo/ImageSaver.swift create mode 100644 peekaboo-cli/Sources/peekaboo/PermissionErrorDetector.swift create mode 100644 tests/unit/utils/malformed-app-target.test.ts diff --git a/peekaboo-cli/Sources/peekaboo/FileNameGenerator.swift b/peekaboo-cli/Sources/peekaboo/FileNameGenerator.swift new file mode 100644 index 0000000..c4f844f --- /dev/null +++ b/peekaboo-cli/Sources/peekaboo/FileNameGenerator.swift @@ -0,0 +1,38 @@ +import Foundation + +struct FileNameGenerator { + static func generateFileName( + displayIndex: Int? = nil, + appName: String? = nil, + windowIndex: Int? = nil, + windowTitle: String? = nil, + format: ImageFormat + ) -> String { + let timestamp = DateFormatter.timestamp.string(from: Date()) + let ext = format.rawValue + + if let displayIndex { + return "screen_\(displayIndex + 1)_\(timestamp).\(ext)" + } else if let appName { + let cleanAppName = appName.replacingOccurrences(of: " ", with: "_") + if let windowIndex { + return "\(cleanAppName)_window_\(windowIndex)_\(timestamp).\(ext)" + } else if let windowTitle { + let cleanTitle = windowTitle.replacingOccurrences(of: " ", with: "_").prefix(20) + return "\(cleanAppName)_\(cleanTitle)_\(timestamp).\(ext)" + } else { + return "\(cleanAppName)_\(timestamp).\(ext)" + } + } else { + return "capture_\(timestamp).\(ext)" + } + } +} + +extension DateFormatter { + static let timestamp: DateFormatter = { + let formatter = DateFormatter() + formatter.dateFormat = "yyyyMMdd_HHmmss" + return formatter + }() +} diff --git a/peekaboo-cli/Sources/peekaboo/ImageSaver.swift b/peekaboo-cli/Sources/peekaboo/ImageSaver.swift new file mode 100644 index 0000000..3d6b3ab --- /dev/null +++ b/peekaboo-cli/Sources/peekaboo/ImageSaver.swift @@ -0,0 +1,47 @@ +import Foundation +import CoreGraphics +import ImageIO +import UniformTypeIdentifiers + +struct ImageSaver { + static func saveImage(_ image: CGImage, to path: String, format: ImageFormat) throws(CaptureError) { + let url = URL(fileURLWithPath: path) + + // Check if the parent directory exists + let directory = url.deletingLastPathComponent() + var isDirectory: ObjCBool = false + if !FileManager.default.fileExists(atPath: directory.path, isDirectory: &isDirectory) { + let error = NSError( + domain: NSCocoaErrorDomain, + code: NSFileNoSuchFileError, + userInfo: [NSLocalizedDescriptionKey: "No such file or directory"] + ) + throw CaptureError.fileWriteError(path, error) + } + + let utType: UTType = format == .png ? .png : .jpeg + guard let destination = CGImageDestinationCreateWithURL( + url as CFURL, + utType.identifier as CFString, + 1, + nil + ) else { + // Try to create a more specific error for common cases + if !FileManager.default.isWritableFile(atPath: directory.path) { + let error = NSError( + domain: NSPOSIXErrorDomain, + code: Int(EACCES), + userInfo: [NSLocalizedDescriptionKey: "Permission denied"] + ) + throw CaptureError.fileWriteError(path, error) + } + throw CaptureError.fileWriteError(path, nil) + } + + CGImageDestinationAddImage(destination, image, nil) + + guard CGImageDestinationFinalize(destination) else { + throw CaptureError.fileWriteError(path, nil) + } + } +} diff --git a/peekaboo-cli/Sources/peekaboo/PermissionErrorDetector.swift b/peekaboo-cli/Sources/peekaboo/PermissionErrorDetector.swift new file mode 100644 index 0000000..d2b7a77 --- /dev/null +++ b/peekaboo-cli/Sources/peekaboo/PermissionErrorDetector.swift @@ -0,0 +1,35 @@ +import Foundation + +struct PermissionErrorDetector { + static func isScreenRecordingPermissionError(_ error: Error) -> Bool { + let errorString = error.localizedDescription.lowercased() + + // Check for specific screen recording related errors + if errorString.contains("screen recording") { + return true + } + + // Check for NSError codes specific to screen capture permissions + if let nsError = error as NSError? { + // ScreenCaptureKit specific error codes + if nsError.domain == "com.apple.screencapturekit" && nsError.code == -3801 { + // SCStreamErrorUserDeclined = -3801 + return true + } + + // CoreGraphics error codes for screen capture + if nsError.domain == "com.apple.coregraphics" && nsError.code == 1002 { + // kCGErrorCannotComplete when permissions are denied + return true + } + } + + // Only consider it a permission error if it mentions both "permission" and capture-related terms + if errorString.contains("permission") && + (errorString.contains("capture") || errorString.contains("recording") || errorString.contains("screen")) { + return true + } + + return false + } +} diff --git a/src/tools/analyze.ts b/src/tools/analyze.ts index 34940d7..7e07756 100644 --- a/src/tools/analyze.ts +++ b/src/tools/analyze.ts @@ -37,17 +37,22 @@ export const analyzeToolSchema = z.object({ .describe( "Optional. Explicit provider/model. Validated against server's PEEKABOO_AI_PROVIDERS.", ), - // Silent fallback parameter (not advertised in schema) - path: z.string().optional(), -}).refine( - (data) => data.image_path || data.path, - { - message: "image_path is required", - path: ["image_path"], - }, -); +}) + .passthrough() // Allow unknown properties (for the hidden `path` parameter) + .refine( + (data: unknown) => { + const typedData = data as { image_path?: string; path?: string }; + return typedData.image_path || typedData.path; + }, + { + message: "image_path is required", + path: ["image_path"], + }, + ); -export type AnalyzeToolInput = z.infer; +export type AnalyzeToolInput = z.infer & { + path?: string; // Hidden parameter for backward compatibility +}; export async function analyzeToolHandler( input: AnalyzeToolInput, diff --git a/src/tools/image.ts b/src/tools/image.ts index 2ac7315..029ba38 100644 --- a/src/tools/image.ts +++ b/src/tools/image.ts @@ -43,7 +43,7 @@ export async function imageToolHandler( if (effectiveFormat && !validFormats.includes(effectiveFormat)) { logger.warn( { originalFormat: effectiveFormat, fallbackFormat: "png" }, - `Invalid format '${effectiveFormat}' detected, falling back to PNG` + `Invalid format '${effectiveFormat}' detected, falling back to PNG`, ); effectiveFormat = "png"; formatWarning = `Invalid format '${input.format}' was provided. Automatically using PNG format instead.`; @@ -61,28 +61,28 @@ export async function imageToolHandler( // Create a corrected input object if format or path needs to be adjusted let correctedInput = input; - + // If format was corrected and we have a path, update the file extension to match the actual format if (input.format && input.format !== effectiveFormat && input.path) { const originalPath = input.path; const parsedPath = path.parse(originalPath); - + // Map format to appropriate extension const extensionMap: { [key: string]: string } = { "png": ".png", - "jpg": ".jpg", + "jpg": ".jpg", "jpeg": ".jpg", - "data": ".png" // data format saves as PNG + "data": ".png", // data format saves as PNG }; - + const newExtension = extensionMap[effectiveFormat || "png"] || ".png"; const correctedPath = path.join(parsedPath.dir, parsedPath.name + newExtension); - + logger.debug( { originalPath, correctedPath, originalFormat: input.format, correctedFormat: effectiveFormat }, - "Correcting file extension to match format" + "Correcting file extension to match format", ); - + correctedInput = { ...input, path: correctedPath }; } @@ -297,7 +297,7 @@ export async function imageToolHandler( } if (!analysisSucceeded && analysisAttempted) { result.isError = true; - result._meta = { ...result._meta, analysis_error: analysisText }; + result._meta = { ...(result._meta || {}), analysis_error: analysisText }; } return result; diff --git a/src/tools/list.ts b/src/tools/list.ts index 160adfe..eb26422 100644 --- a/src/tools/list.ts +++ b/src/tools/list.ts @@ -43,12 +43,12 @@ export const listToolSchema = z if (val === "" || val === null || val === undefined) { return undefined; } - + // If it's already an array, return as-is if (Array.isArray(val)) { return val; } - + // If it's a string that looks like JSON, try to parse it if (typeof val === "string") { try { @@ -59,16 +59,16 @@ export const listToolSchema = z } catch { // Not valid JSON, treat as single item } - + // If it's a comma-separated string, split it if (val.includes(",")) { return val.split(",").map(s => s.trim()); } - + // Single string value, wrap in array return [val.trim()]; } - + return val; }, z diff --git a/src/utils/image-cli-args.ts b/src/utils/image-cli-args.ts index d6b9c4f..e0f3d56 100644 --- a/src/utils/image-cli-args.ts +++ b/src/utils/image-cli-args.ts @@ -81,7 +81,7 @@ export function buildSwiftCliArgs( } else { args.push("--mode", "screen", "--screen-index", screenIndex.toString()); } - } else if (input.app_target === "frontmost") { + } else if (input.app_target.toLowerCase() === "frontmost") { // 'frontmost': All windows of the frontmost app log.warn( "'frontmost' target requires determining current frontmost app, defaulting to screen mode", @@ -91,31 +91,60 @@ export function buildSwiftCliArgs( // 'AppName:WINDOW_TITLE:Title' or 'AppName:WINDOW_INDEX:Index' const parts = input.app_target.split(":"); if (parts.length >= 3) { - const appName = parts[0]; - const specifierType = parts[1]; + const appName = parts[0].trim(); + const specifierType = parts[1].trim(); const specifierValue = parts.slice(2).join(":"); // Handle colons in window titles - args.push("--app", appName.trim()); - args.push("--mode", "window"); - - if (specifierType === "WINDOW_TITLE") { - args.push("--window-title", specifierValue); - } else if (specifierType === "WINDOW_INDEX") { - args.push("--window-index", specifierValue); - } else { + // Validate that we have a non-empty app name + if (!appName) { log.warn( - { specifierType }, - "Unknown window specifier type, defaulting to main window", + { app_target: input.app_target }, + "Empty app name detected in app_target, treating as malformed", ); + // Try to find the first non-empty part as the app name + const nonEmptyParts = parts.filter(part => part.trim()); + if (nonEmptyParts.length > 0) { + args.push("--app", nonEmptyParts[0].trim()); + args.push("--mode", "multi"); + } else { + // All parts are empty, default to screen mode + log.warn("All parts of app_target are empty, defaulting to screen mode"); + args.push("--mode", "screen"); + } + } else { + args.push("--app", appName); + args.push("--mode", "window"); + + if (specifierType.toUpperCase() === "WINDOW_TITLE") { + args.push("--window-title", specifierValue); + } else if (specifierType.toUpperCase() === "WINDOW_INDEX") { + args.push("--window-index", specifierValue); + } else { + log.warn( + { specifierType }, + "Unknown window specifier type, defaulting to main window", + ); + } } } else { - // Malformed: treat as app name - log.warn( - { app_target: input.app_target }, - "Malformed window specifier, treating as app name", - ); - args.push("--app", input.app_target.trim()); - args.push("--mode", "multi"); + // Malformed: treat as app name, but validate it's not empty + const cleanAppTarget = input.app_target.trim(); + if (!cleanAppTarget || cleanAppTarget === ":".repeat(cleanAppTarget.length)) { + log.warn( + { app_target: input.app_target }, + "Malformed app_target with only colons or empty, defaulting to screen mode", + ); + args.push("--mode", "screen"); + } else { + log.warn( + { app_target: input.app_target }, + "Malformed window specifier, treating as app name", + ); + // Remove trailing colons from app name + const appName = cleanAppTarget.replace(/:+$/, ""); + args.push("--app", appName); + args.push("--mode", "multi"); + } } } else { // 'AppName': All windows of that app diff --git a/tests/unit/tools/analyze.test.ts b/tests/unit/tools/analyze.test.ts index 87da06a..f51708a 100644 --- a/tests/unit/tools/analyze.test.ts +++ b/tests/unit/tools/analyze.test.ts @@ -645,5 +645,51 @@ describe("Analyze Tool", () => { expect(analyzeToolSchema.safeParse(inputWithoutProvider).success).toBe(true); expect(analyzeToolSchema.safeParse(inputWithProvider).success).toBe(true); }); + + it("should accept hidden path parameter for backward compatibility", () => { + // Test that the hidden 'path' parameter is accepted and works + const inputWithPath = { + path: "/path/to/image.png", + question: "What is this?", + }; + + const inputWithBothPaths = { + image_path: "/path/to/primary.png", + path: "/path/to/fallback.png", + question: "What is this?", + }; + + // Both should pass validation + expect(analyzeToolSchema.safeParse(inputWithPath).success).toBe(true); + expect(analyzeToolSchema.safeParse(inputWithBothPaths).success).toBe(true); + + // When both are provided, image_path should take precedence + const parsedWithBoth = analyzeToolSchema.parse(inputWithBothPaths); + expect(parsedWithBoth.image_path).toBe("/path/to/primary.png"); + expect((parsedWithBoth as any).path).toBe("/path/to/fallback.png"); + }); + + it("should handle hidden path parameter in tool handler", async () => { + process.env.PEEKABOO_AI_PROVIDERS = "ollama/llava"; + mockParseAIProviders.mockReturnValue([ + { provider: "ollama", model: "llava" }, + ]); + mockDetermineProviderAndModel.mockResolvedValue({ + provider: "ollama", + model: "llava", + }); + mockAnalyzeImageWithProvider.mockResolvedValue("Analysis complete"); + + // Test with only path parameter (should use fallback) + const inputWithOnlyPath: any = { + path: "/path/to/fallback.png", + question: "What is this?", + }; + + const result = await analyzeToolHandler(inputWithOnlyPath, mockContext); + + expect(mockReadImageAsBase64).toHaveBeenCalledWith("/path/to/fallback.png"); + expect(result.isError).toBeUndefined(); + }); }); }); \ No newline at end of file diff --git a/tests/unit/tools/image.test.ts b/tests/unit/tools/image.test.ts index bcd2aa2..66e7d91 100644 --- a/tests/unit/tools/image.test.ts +++ b/tests/unit/tools/image.test.ts @@ -1044,6 +1044,48 @@ describe("Image Tool", () => { expect(loggerWarnSpy).toHaveBeenCalled(); }); + it("should handle app_target: 'frontmost' case-insensitively", () => { + const loggerWarnSpy = vi.spyOn(mockLogger, "warn"); + + // Test uppercase + const argsUpper = buildSwiftCliArgs({ app_target: "FRONTMOST" }, undefined, undefined, mockLogger); + expect(argsUpper).toEqual( + expect.arrayContaining(["--mode", "screen"]), + ); + expect(argsUpper).not.toContain("--app"); + + // Test mixed case + const argsMixed = buildSwiftCliArgs({ app_target: "Frontmost" }, undefined, undefined, mockLogger); + expect(argsMixed).toEqual( + expect.arrayContaining(["--mode", "screen"]), + ); + expect(argsMixed).not.toContain("--app"); + + expect(loggerWarnSpy).toHaveBeenCalledTimes(2); + }); + + it("should handle window specifiers case-insensitively", () => { + // Test lowercase window_title + const argsLowerTitle = buildSwiftCliArgs({ app_target: "Safari:window_title:Apple Website" }, undefined); + expect(argsLowerTitle).toEqual( + expect.arrayContaining([ + "--app", "Safari", + "--mode", "window", + "--window-title", "Apple Website" + ]), + ); + + // Test mixed case Window_Index + const argsMixedIndex = buildSwiftCliArgs({ app_target: "Terminal:Window_Index:2" }, undefined); + expect(argsMixedIndex).toEqual( + expect.arrayContaining([ + "--app", "Terminal", + "--mode", "window", + "--window-index", "2" + ]), + ); + }); + it("should handle simple app name", () => { const args = buildSwiftCliArgs({ app_target: "Safari" }, undefined); expect(args).toEqual( diff --git a/tests/unit/utils/malformed-app-target.test.ts b/tests/unit/utils/malformed-app-target.test.ts new file mode 100644 index 0000000..b82edfc --- /dev/null +++ b/tests/unit/utils/malformed-app-target.test.ts @@ -0,0 +1,149 @@ +import { describe, it, expect } from "vitest"; +import { buildSwiftCliArgs } from "../../../src/utils/image-cli-args"; + +describe("Malformed App Target Parsing", () => { + it("should handle multiple leading colons correctly", () => { + const input = { + app_target: "::::::::::::::::Finder", + format: "png" as const + }; + + const args = buildSwiftCliArgs(input, "/tmp/test.png"); + + // Should not result in empty app name + expect(args).toContain("--app"); + const appIndex = args.indexOf("--app"); + expect(args[appIndex + 1]).not.toBe(""); + + // Should either treat as malformed and use multi mode, or properly extract "Finder" + expect(args).toContain("--mode"); + const modeIndex = args.indexOf("--mode"); + const mode = args[modeIndex + 1]; + expect(["multi", "window"]).toContain(mode); + }); + + it("should handle empty parts between colons", () => { + const input = { + app_target: "::Chrome::WINDOW_TITLE::google.com", + format: "png" as const + }; + + const args = buildSwiftCliArgs(input, "/tmp/test.png"); + + // Should not result in empty app name + expect(args).toContain("--app"); + const appIndex = args.indexOf("--app"); + expect(args[appIndex + 1]).not.toBe(""); + }); + + it("should handle app target that starts with colon", () => { + const input = { + app_target: ":Chrome", + format: "png" as const + }; + + const args = buildSwiftCliArgs(input, "/tmp/test.png"); + + // Should handle gracefully, not create empty app name + expect(args).toContain("--app"); + const appIndex = args.indexOf("--app"); + expect(args[appIndex + 1]).not.toBe(""); + }); + + it("should handle app target that ends with colon", () => { + const input = { + app_target: "Chrome:", + format: "png" as const + }; + + const args = buildSwiftCliArgs(input, "/tmp/test.png"); + + // Should treat as simple app name + expect(args).toContain("--app"); + const appIndex = args.indexOf("--app"); + expect(args[appIndex + 1]).toBe("Chrome"); + + expect(args).toContain("--mode"); + const modeIndex = args.indexOf("--mode"); + expect(args[modeIndex + 1]).toBe("multi"); + }); + + it("should handle multiple consecutive colons in middle", () => { + const input = { + app_target: "Chrome:::WINDOW_TITLE:::google.com", + format: "png" as const + }; + + const args = buildSwiftCliArgs(input, "/tmp/test.png"); + + expect(args).toContain("--app"); + const appIndex = args.indexOf("--app"); + expect(args[appIndex + 1]).toBe("Chrome"); + + // Should handle the malformed specifier type gracefully + expect(args).toContain("--mode"); + }); + + it("should reject completely empty app target", () => { + const input = { + app_target: "", + format: "png" as const + }; + + const args = buildSwiftCliArgs(input, "/tmp/test.png"); + + // Should default to screen mode for empty target + expect(args).toContain("--mode"); + const modeIndex = args.indexOf("--mode"); + expect(args[modeIndex + 1]).toBe("screen"); + + // Should not contain app argument + expect(args).not.toContain("--app"); + }); + + it("should handle only colons", () => { + const input = { + app_target: ":::::", + format: "png" as const + }; + + const args = buildSwiftCliArgs(input, "/tmp/test.png"); + + // Should not result in empty app name being passed + if (args.includes("--app")) { + const appIndex = args.indexOf("--app"); + expect(args[appIndex + 1]).not.toBe(""); + } + }); + + it("should handle whitespace-only app names", () => { + const input = { + app_target: " :WINDOW_TITLE:test", + format: "png" as const + }; + + const args = buildSwiftCliArgs(input, "/tmp/test.png"); + + // Should handle whitespace-only app names + if (args.includes("--app")) { + const appIndex = args.indexOf("--app"); + const appName = args[appIndex + 1]; + expect(appName.trim()).not.toBe(""); + } + }); + + it("should demonstrate current behavior for debugging", () => { + // This test documents what currently happens with the problematic input + const input = { + app_target: "::::::::::::::::Finder", + format: "png" as const + }; + + const args = buildSwiftCliArgs(input, "/tmp/test.png"); + + console.log("Args for '::::::::::::::::Finder':", args); + + // Document current behavior - this will help us verify the fix + expect(args).toContain("--mode"); + }); +}); \ No newline at end of file