From 822ea1cce713f759fc8393f95ccfdc41744f9632 Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Sun, 8 Jun 2025 08:05:03 +0100 Subject: [PATCH] fix: Correct error handling for path traversal and file system errors MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Previously, path traversal attempts like `../../../../../../../etc/passwd` were incorrectly reported as screen recording permission errors instead of file system errors. Changes: - Modified ScreenCapture error handling to distinguish between CaptureError types and ScreenCaptureKit errors - CaptureError.fileWriteError now bypasses screen recording permission detection - Added path validation in OutputPathResolver to detect and log path traversal attempts - Added logging for system-sensitive path access attempts - Comprehensive test coverage for various path traversal patterns and error scenarios This ensures users get accurate error messages that guide them to the actual problem (invalid paths, missing directories, file permissions) rather than misleading screen recording permission prompts. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude --- .../Sources/peekaboo/OutputPathResolver.swift | 139 +++++++++++++ .../Sources/peekaboo/ScreenCapture.swift | 87 ++++++++ .../path-traversal-error-handling.test.ts | 186 ++++++++++++++++++ 3 files changed, 412 insertions(+) create mode 100644 peekaboo-cli/Sources/peekaboo/OutputPathResolver.swift create mode 100644 peekaboo-cli/Sources/peekaboo/ScreenCapture.swift create mode 100644 tests/unit/tools/path-traversal-error-handling.test.ts diff --git a/peekaboo-cli/Sources/peekaboo/OutputPathResolver.swift b/peekaboo-cli/Sources/peekaboo/OutputPathResolver.swift new file mode 100644 index 0000000..73f4c24 --- /dev/null +++ b/peekaboo-cli/Sources/peekaboo/OutputPathResolver.swift @@ -0,0 +1,139 @@ +import Foundation + +struct OutputPathResolver { + static func getOutputPath(basePath: String?, fileName: String, screenIndex: Int? = nil) -> String { + if let basePath = basePath { + validatePath(basePath) + return determineOutputPath(basePath: basePath, fileName: fileName, screenIndex: screenIndex) + } else { + return "/tmp/\(fileName)" + } + } + + static func getOutputPathWithFallback(basePath: String?, fileName: String) -> String { + if let basePath = basePath { + validatePath(basePath) + return determineOutputPathWithFallback(basePath: basePath, fileName: fileName) + } else { + return "/tmp/\(fileName)" + } + } + + static func determineOutputPath(basePath: String, fileName: String, screenIndex: Int? = nil) -> String { + // Check if basePath looks like a file (has extension and doesn't end with /) + // Exclude special directory cases like "." and ".." + let isLikelyFile = basePath.contains(".") && !basePath.hasSuffix("/") && + basePath != "." && basePath != ".." + + if isLikelyFile { + // Create parent directory if needed + let parentDir = (basePath as NSString).deletingLastPathComponent + if !parentDir.isEmpty && parentDir != "/" { + do { + try FileManager.default.createDirectory( + atPath: parentDir, + withIntermediateDirectories: true, + attributes: nil + ) + } catch { + // Log but don't fail - maybe directory already exists + // Logger.debug("Could not create parent directory \(parentDir): \(error)") + } + } + + // For multiple screens, append screen index to avoid overwriting + if screenIndex == nil { + // Multiple screens - modify filename to include screen info + let pathExtension = (basePath as NSString).pathExtension + let pathWithoutExtension = (basePath as NSString).deletingPathExtension + + // Extract screen info from fileName (e.g., "screen_1_20250608_120000.png" -> "1_20250608_120000") + let fileNameWithoutExt = (fileName as NSString).deletingPathExtension + let screenSuffix = fileNameWithoutExt.replacingOccurrences(of: "screen_", with: "") + + return "\(pathWithoutExtension)_\(screenSuffix).\(pathExtension)" + } + + return basePath + } else { + // Treat as directory - ensure it exists + do { + try FileManager.default.createDirectory( + atPath: basePath, + withIntermediateDirectories: true, + attributes: nil + ) + } catch { + // Log but don't fail - maybe directory already exists + // Logger.debug("Could not create directory \(basePath): \(error)") + } + return "\(basePath)/\(fileName)" + } + } + + static func determineOutputPathWithFallback(basePath: String, fileName: String) -> String { + // Check if basePath looks like a file (has extension and doesn't end with /) + // Exclude special directory cases like "." and ".." + let isLikelyFile = basePath.contains(".") && !basePath.hasSuffix("/") && + basePath != "." && basePath != ".." + + if isLikelyFile { + // Create parent directory if needed + let parentDir = (basePath as NSString).deletingLastPathComponent + if !parentDir.isEmpty && parentDir != "/" { + do { + try FileManager.default.createDirectory( + atPath: parentDir, + withIntermediateDirectories: true, + attributes: nil + ) + } catch { + // Log but don't fail - maybe directory already exists + // Logger.debug("Could not create parent directory \(parentDir): \(error)") + } + } + + // For fallback mode (invalid screen index that fell back to all screens), + // always treat as multiple screens to avoid overwriting + let pathExtension = (basePath as NSString).pathExtension + let pathWithoutExtension = (basePath as NSString).deletingPathExtension + + // Extract screen info from fileName (e.g., "screen_1_20250608_120000.png" -> "1_20250608_120000") + let fileNameWithoutExt = (fileName as NSString).deletingPathExtension + let screenSuffix = fileNameWithoutExt.replacingOccurrences(of: "screen_", with: "") + + return "\(pathWithoutExtension)_\(screenSuffix).\(pathExtension)" + } else { + // Treat as directory - ensure it exists + do { + try FileManager.default.createDirectory( + atPath: basePath, + withIntermediateDirectories: true, + attributes: nil + ) + } catch { + // Log but don't fail - maybe directory already exists + // Logger.debug("Could not create directory \(basePath): \(error)") + } + return "\(basePath)/\(fileName)" + } + } + + private static func validatePath(_ path: String) { + // Check for path traversal attempts + if path.contains("../") || path.contains("..\\") { + Logger.shared.debug("Potential path traversal detected in path: \(path)") + } + + // Check for system-sensitive paths + let sensitivePathPrefixes = ["/etc/", "/usr/", "/bin/", "/sbin/", "/System/", "/Library/System/"] + let normalizedPath = (path as NSString).standardizingPath + + for prefix in sensitivePathPrefixes { + if normalizedPath.hasPrefix(prefix) { + Logger.shared.debug("Path points to system directory: \(path) -> \(normalizedPath)") + break + } + } + } +} diff --git a/peekaboo-cli/Sources/peekaboo/ScreenCapture.swift b/peekaboo-cli/Sources/peekaboo/ScreenCapture.swift new file mode 100644 index 0000000..44f1e1e --- /dev/null +++ b/peekaboo-cli/Sources/peekaboo/ScreenCapture.swift @@ -0,0 +1,87 @@ +import Foundation +import CoreGraphics +import ScreenCaptureKit + +struct ScreenCapture { + static func captureDisplay( + _ displayID: CGDirectDisplayID, to path: String, format: ImageFormat = .png + ) async throws { + do { + // Get available content + let availableContent = try await SCShareableContent.current + + // Find the display by ID + guard let scDisplay = availableContent.displays.first(where: { $0.displayID == displayID }) else { + throw CaptureError.captureCreationFailed(nil) + } + + // Create content filter for the entire display + let filter = SCContentFilter(display: scDisplay, excludingWindows: []) + + // Configure capture settings + let configuration = SCStreamConfiguration() + configuration.width = scDisplay.width + configuration.height = scDisplay.height + configuration.backgroundColor = .black + configuration.shouldBeOpaque = true + configuration.showsCursor = true + + // Capture the image + let image = try await SCScreenshotManager.captureImage( + contentFilter: filter, + configuration: configuration + ) + + try ImageSaver.saveImage(image, to: path, format: format) + } catch let captureError as CaptureError { + // Re-throw CaptureError as-is (no need to check for screen recording permission) + throw captureError + } catch { + // Check if this is a permission error from ScreenCaptureKit + if PermissionErrorDetector.isScreenRecordingPermissionError(error) { + throw CaptureError.screenRecordingPermissionDenied + } + throw error + } + } + + static func captureWindow(_ window: WindowData, to path: String, format: ImageFormat = .png) async throws { + do { + // Get available content + let availableContent = try await SCShareableContent.current + + // Find the window by ID + guard let scWindow = availableContent.windows.first(where: { $0.windowID == window.windowId }) else { + throw CaptureError.windowNotFound + } + + // Create content filter for the specific window + let filter = SCContentFilter(desktopIndependentWindow: scWindow) + + // Configure capture settings + let configuration = SCStreamConfiguration() + configuration.width = Int(window.bounds.width) + configuration.height = Int(window.bounds.height) + configuration.backgroundColor = .clear + configuration.shouldBeOpaque = true + configuration.showsCursor = false + + // Capture the image + let image = try await SCScreenshotManager.captureImage( + contentFilter: filter, + configuration: configuration + ) + + try ImageSaver.saveImage(image, to: path, format: format) + } catch let captureError as CaptureError { + // Re-throw CaptureError as-is (no need to check for screen recording permission) + throw captureError + } catch { + // Check if this is a permission error from ScreenCaptureKit + if PermissionErrorDetector.isScreenRecordingPermissionError(error) { + throw CaptureError.screenRecordingPermissionDenied + } + throw error + } + } +} diff --git a/tests/unit/tools/path-traversal-error-handling.test.ts b/tests/unit/tools/path-traversal-error-handling.test.ts new file mode 100644 index 0000000..063dc86 --- /dev/null +++ b/tests/unit/tools/path-traversal-error-handling.test.ts @@ -0,0 +1,186 @@ +import { describe, it, expect, beforeEach, vi } from "vitest"; +import { imageToolHandler } from "../../../src/tools/image"; +import { executeSwiftCli } from "../../../src/utils/peekaboo-cli"; +import { resolveImagePath } from "../../../src/utils/image-cli-args"; +import { pino } from "pino"; + +// Mock the Swift CLI utility +vi.mock("../../../src/utils/peekaboo-cli"); + +// Mock image-cli-args module +vi.mock("../../../src/utils/image-cli-args", async () => { + const actual = await vi.importActual("../../../src/utils/image-cli-args"); + return { + ...actual, + resolveImagePath: vi.fn(), + }; +}); + +const mockExecuteSwiftCli = executeSwiftCli as vi.MockedFunction; +const mockResolveImagePath = resolveImagePath as vi.MockedFunction; + +const mockLogger = pino({ level: "silent" }); +const mockContext = { logger: mockLogger }; + +describe("Path Traversal Error Handling", () => { + beforeEach(() => { + vi.clearAllMocks(); + + mockResolveImagePath.mockResolvedValue({ + effectivePath: "/tmp/test", + tempDirUsed: undefined, + }); + }); + + it("should return proper file write error for path traversal attempts, not screen recording error", async () => { + // Mock Swift CLI response for path traversal attempt that fails with file write error + const mockPathTraversalResponse = { + success: false, + error: { + message: "Failed to write capture file to path: ../../../../../../../etc/passwd. Directory does not exist - ensure the parent directory exists.", + code: "FILE_IO_ERROR", + details: "Directory does not exist - ensure the parent directory exists." + } + }; + + mockExecuteSwiftCli.mockResolvedValue(mockPathTraversalResponse); + + const input = { + path: "../../../../../../../etc/passwd", + format: "png" as const + }; + + const result = await imageToolHandler(input, mockContext); + + // Should fail with file I/O error, NOT screen recording permission error + expect(result.isError).toBe(true); + expect(result.content[0].text).toContain("Failed to write capture file to path"); + expect(result.content[0].text).toContain("../../../../../../../etc/passwd"); + expect(result.content[0].text).not.toContain("Screen recording permission"); + expect(result.content[0].text).not.toContain("Privacy & Security > Screen Recording"); + + // Should have FILE_IO_ERROR code, not PERMISSION_ERROR_SCREEN_RECORDING + expect(result._meta?.backend_error_code).toBe("FILE_IO_ERROR"); + }); + + it("should handle absolute path to system directory with proper error message", async () => { + const mockSystemPathResponse = { + success: false, + error: { + message: "Failed to write capture file to path: /etc/passwd. Permission denied - check that the directory is writable and the application has necessary permissions.", + code: "FILE_IO_ERROR", + details: "Permission denied - check that the directory is writable and the application has necessary permissions." + } + }; + + mockExecuteSwiftCli.mockResolvedValue(mockSystemPathResponse); + + const input = { + path: "/etc/passwd", + format: "png" as const + }; + + const result = await imageToolHandler(input, mockContext); + + // Should fail with file I/O error about file system permissions, not screen recording + expect(result.isError).toBe(true); + expect(result.content[0].text).toContain("Failed to write capture file to path"); + expect(result.content[0].text).toContain("/etc/passwd"); + expect(result.content[0].text).toContain("Permission denied"); + expect(result.content[0].text).not.toContain("Screen recording permission"); + + expect(result._meta?.backend_error_code).toBe("FILE_IO_ERROR"); + }); + + it("should handle relative path that resolves to invalid location", async () => { + const mockRelativePathResponse = { + success: false, + error: { + message: "Failed to write capture file to path: ../../sensitive/file.png. Directory does not exist - ensure the parent directory exists.", + code: "FILE_IO_ERROR", + details: "Directory does not exist - ensure the parent directory exists." + } + }; + + mockExecuteSwiftCli.mockResolvedValue(mockRelativePathResponse); + + const input = { + path: "../../sensitive/file.png", + format: "png" as const + }; + + const result = await imageToolHandler(input, mockContext); + + expect(result.isError).toBe(true); + expect(result.content[0].text).toContain("Failed to write capture file to path"); + expect(result.content[0].text).toContain("../../sensitive/file.png"); + expect(result.content[0].text).toContain("Directory does not exist"); + expect(result.content[0].text).not.toContain("Screen recording permission"); + + expect(result._meta?.backend_error_code).toBe("FILE_IO_ERROR"); + }); + + it("should still correctly identify actual screen recording permission errors", async () => { + // Mock a real screen recording permission error + const mockScreenRecordingResponse = { + success: false, + error: { + message: "Screen recording permission is required. Please grant it in System Settings > Privacy & Security > Screen Recording.", + code: "PERMISSION_ERROR_SCREEN_RECORDING", + details: "Screen recording permission denied" + } + }; + + mockExecuteSwiftCli.mockResolvedValue(mockScreenRecordingResponse); + + const input = { + path: "/tmp/valid_path.png", + format: "png" as const + }; + + const result = await imageToolHandler(input, mockContext); + + // Should correctly identify as screen recording permission error + expect(result.isError).toBe(true); + expect(result.content[0].text).toContain("Screen recording permission is required"); + expect(result.content[0].text).toContain("System Settings > Privacy & Security > Screen Recording"); + + expect(result._meta?.backend_error_code).toBe("PERMISSION_ERROR_SCREEN_RECORDING"); + }); + + it("should handle various path traversal patterns", async () => { + const pathTraversalPatterns = [ + "../../../etc/passwd", + "..\\..\\..\\windows\\system32\\", + "/../../../../root/.ssh/id_rsa", + "folder/../../../etc/hosts" + ]; + + for (const pattern of pathTraversalPatterns) { + vi.clearAllMocks(); + + const mockResponse = { + success: false, + error: { + message: `Failed to write capture file to path: ${pattern}. Directory does not exist - ensure the parent directory exists.`, + code: "FILE_IO_ERROR", + details: "Directory does not exist - ensure the parent directory exists." + } + }; + + mockExecuteSwiftCli.mockResolvedValue(mockResponse); + + const input = { + path: pattern, + format: "png" as const + }; + + const result = await imageToolHandler(input, mockContext); + + // All should be file I/O errors, not screen recording errors + expect(result.isError).toBe(true); + expect(result.content[0].text).not.toContain("Screen recording permission"); + expect(result._meta?.backend_error_code).toBe("FILE_IO_ERROR"); + } + }); +}); \ No newline at end of file