From a45ef77cc0d1e7207f67e36f49b38648da78cea7 Mon Sep 17 00:00:00 2001 From: a05031113 Date: Tue, 14 Jul 2026 21:31:57 +0800 Subject: [PATCH 1/4] Fix VT-export clipboard capture swallowing concurrent user copies MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The one-shot standardClipboardWriteCapture armed by readTerminalTextFromVTExportForSnapshot (session snapshots, mobile replay, agent naming) diverted the NEXT standard-clipboard write from any source. A user copy (Cmd+C, copy-on-select, or copy mode) racing an in-flight VT export was captured instead of reaching NSPasteboard, silently leaving the clipboard empty. The capture now carries a predicate and only consumes writes that match it; non-matching writes pass through to the real pasteboard with the capture left armed. The VT-export caller matches only a single absolute path (or file URL) to an existing file — the shape write_screen_file / write_active_file produce — so user copies can no longer be swallowed. Also: check the previously ignored setString result (retry once and log on failure, instead of leaving the clipboard cleared with nothing written), log pass-through writes for diagnosis, and inject the standard pasteboard so tests cover the write path without touching the real general pasteboard. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01SwGyYRt9QjDUhimcp2xB7E --- .../TerminalPasteboardService.swift | 98 +++++++++++++++---- .../TerminalPasteboardServiceTests.swift | 35 +++++++ .../Clipboard/TerminalClipboardWriting.swift | 32 ++++-- Sources/TerminalController.swift | 19 +++- cmuxTests/SessionPersistenceTests.swift | 17 ++++ 5 files changed, 175 insertions(+), 26 deletions(-) diff --git a/Packages/macOS/CmuxTerminal/Sources/CmuxTerminal/Services/Pasteboard/TerminalPasteboardService.swift b/Packages/macOS/CmuxTerminal/Sources/CmuxTerminal/Services/Pasteboard/TerminalPasteboardService.swift index dd3ea62fd679..0efc42ce996e 100644 --- a/Packages/macOS/CmuxTerminal/Sources/CmuxTerminal/Services/Pasteboard/TerminalPasteboardService.swift +++ b/Packages/macOS/CmuxTerminal/Sources/CmuxTerminal/Services/Pasteboard/TerminalPasteboardService.swift @@ -1,6 +1,7 @@ public import AppKit public import CmuxTerminalCore public import GhosttyKit +internal import os /// The terminal's pasteboard capability: clipboard reads and writes for the /// ghostty runtime, plus materialization of pasteboard images into owned @@ -23,13 +24,23 @@ public import GhosttyKit /// temp-file set and the one-shot write capture), the sanctioned shape for /// state shared with synchronous callbacks. public final class TerminalPasteboardService: Sendable { - /// One-shot interception slot for ``captureNextStandardClipboardWrite(_:)``. + /// One-shot interception slot for ``captureNextStandardClipboardWrite(matching:_:)``. final class ClipboardWriteCapture: Sendable { private let lock = NSLock() // SAFETY: guarded by `lock`; written by the runtime's write-clipboard // callback thread and read by the capturing caller. nonisolated(unsafe) private var capturedValue: String? + /// Predicate deciding whether a standard-clipboard write belongs to + /// this capture. Writes it rejects pass through to the real + /// pasteboard and leave the capture armed, so an unrelated write + /// (e.g. a user copy racing a VT export) is not swallowed. + let accepts: @Sendable (String) -> Bool + + init(accepts: @escaping @Sendable (String) -> Bool) { + self.accepts = accepts + } + /// Stores the diverted clipboard string. func capture(_ value: String) { lock.lock() @@ -57,6 +68,16 @@ public final class TerminalPasteboardService: Sendable { // ghostty runtime threads. nonisolated(unsafe) private let selectionPasteboard: NSPasteboard + // SAFETY: immutable reference; same argument as `selectionPasteboard`. + // Injectable so tests can exercise standard-location writes without + // touching the real general pasteboard. + nonisolated(unsafe) private let standardPasteboard: NSPasteboard + + private static let logger = Logger( + subsystem: "com.cmuxterm.app", + category: "terminal.pasteboard" + ) + /// The directory that owned temporary image files are written into. let temporaryDirectory: URL @@ -74,11 +95,19 @@ public final class TerminalPasteboardService: Sendable { /// Creates the process's pasteboard service. /// - /// - Parameter temporaryDirectory: Destination for owned temporary image - /// files. Tests inject a scratch directory; the app uses the user's - /// temporary directory. - public init(temporaryDirectory: URL = FileManager.default.temporaryDirectory) { + /// - Parameters: + /// - temporaryDirectory: Destination for owned temporary image files. + /// Tests inject a scratch directory; the app uses the user's + /// temporary directory. + /// - standardPasteboard: The pasteboard backing the standard clipboard + /// location. Tests inject a scratch pasteboard; the app uses + /// `NSPasteboard.general`. + public init( + temporaryDirectory: URL = FileManager.default.temporaryDirectory, + standardPasteboard: NSPasteboard = .general + ) { self.temporaryDirectory = temporaryDirectory + self.standardPasteboard = standardPasteboard self.selectionPasteboard = NSPasteboard( name: NSPasteboard.Name("com.mitchellh.ghostty.selection") ) @@ -88,32 +117,63 @@ public final class TerminalPasteboardService: Sendable { extension TerminalPasteboardService: TerminalClipboardWriting { /// Writes a string to the given ghostty clipboard location, honoring an /// armed one-shot capture for the standard location. + /// + /// An armed capture only consumes writes its predicate accepts; any other + /// standard-location write (e.g. a user copy racing a VT export) passes + /// through to the real pasteboard with the capture left armed. public func writeString(_ string: String, to location: ghostty_clipboard_e) { if location == GHOSTTY_CLIPBOARD_STANDARD { - var capture: ClipboardWriteCapture? standardClipboardWriteCaptureLock.lock() - capture = standardClipboardWriteCapture - if capture != nil { - standardClipboardWriteCapture = nil - } + let armed = standardClipboardWriteCapture standardClipboardWriteCaptureLock.unlock() - if let capture { - capture.capture(string) - return + if let armed { + if armed.accepts(string) { + standardClipboardWriteCaptureLock.lock() + if standardClipboardWriteCapture === armed { + standardClipboardWriteCapture = nil + } + standardClipboardWriteCaptureLock.unlock() + armed.capture(string) + return + } + Self.logger.info( + "standard write passed through armed capture (length \(string.count, privacy: .public))" + ) } } guard let pasteboard = pasteboard(for: location) else { return } pasteboard.clearContents() - pasteboard.setString(string, forType: .string) + if !pasteboard.setString(string, forType: .string) { + // A contended pasteboard can reject the write after clearContents, + // leaving the clipboard empty. Re-declare and retry once so the + // failure is at least not silent data loss. + pasteboard.clearContents() + let retried = pasteboard.setString(string, forType: .string) + Self.logger.error( + "pasteboard setString failed (length \(string.count, privacy: .public)), retry \(retried ? "succeeded" : "failed", privacy: .public)" + ) + } } - /// Arms a one-shot diversion of the next standard-clipboard write that - /// happens while `action` runs, returning the diverted string. + /// Arms a one-shot diversion of the next matching standard-clipboard + /// write that happens while `action` runs, returning the diverted string. + /// + /// - Parameters: + /// - predicate: Decides whether a given standard-clipboard write is the + /// one this capture is waiting for. Non-matching writes reach the + /// real pasteboard and leave the capture armed, so unrelated writers + /// (a user copy, another surface) are not swallowed. Pass a predicate + /// as narrow as the expected payload allows — e.g. "an existing file + /// under the temporary directory" for a VT screen export. + /// - action: The operation expected to trigger the write. @discardableResult - public func captureNextStandardClipboardWrite(_ action: () -> Bool) -> String? { - let capture = ClipboardWriteCapture() + public func captureNextStandardClipboardWrite( + matching predicate: @escaping @Sendable (String) -> Bool = { _ in true }, + _ action: () -> Bool + ) -> String? { + let capture = ClipboardWriteCapture(accepts: predicate) standardClipboardWriteCaptureLock.lock() standardClipboardWriteCapture = capture standardClipboardWriteCaptureLock.unlock() @@ -136,7 +196,7 @@ extension TerminalPasteboardService { public func pasteboard(for location: ghostty_clipboard_e) -> NSPasteboard? { switch location { case GHOSTTY_CLIPBOARD_STANDARD: - return .general + return standardPasteboard case GHOSTTY_CLIPBOARD_SELECTION: return selectionPasteboard default: diff --git a/Packages/macOS/CmuxTerminal/Tests/CmuxTerminalTests/Services/TerminalPasteboardServiceTests.swift b/Packages/macOS/CmuxTerminal/Tests/CmuxTerminalTests/Services/TerminalPasteboardServiceTests.swift index b0a5fb52339e..917ba1f14ca8 100644 --- a/Packages/macOS/CmuxTerminal/Tests/CmuxTerminalTests/Services/TerminalPasteboardServiceTests.swift +++ b/Packages/macOS/CmuxTerminal/Tests/CmuxTerminalTests/Services/TerminalPasteboardServiceTests.swift @@ -123,6 +123,41 @@ struct ClipboardWriteCaptureTests { #expect(board?.string(forType: .string) == marker) #expect(service.hasString(for: GHOSTTY_CLIPBOARD_SELECTION)) } + + @Test func nonMatchingWritePassesThroughAndLeavesCaptureArmed() throws { + let scratchDir = try makeScratchDirectory() + defer { try? FileManager.default.removeItem(at: scratchDir) } + let scratch = ScratchPasteboard() + let service = TerminalPasteboardService(standardPasteboard: scratch.pasteboard) + let exportFile = scratchDir.appendingPathComponent("export.vt") + try Data("screen".utf8).write(to: exportFile) + let userCopy = "user copied text \(UUID().uuidString)" + + let captured = service.captureNextStandardClipboardWrite( + matching: { $0 == exportFile.path } + ) { + // A user copy racing the export must reach the pasteboard... + service.writeString(userCopy, to: GHOSTTY_CLIPBOARD_STANDARD) + // ...and the capture must stay armed for the export's own write. + service.writeString(exportFile.path, to: GHOSTTY_CLIPBOARD_STANDARD) + return true + } + + #expect(captured == exportFile.path) + #expect(scratch.pasteboard.string(forType: .string) == userCopy) + } + + @Test func standardWritesLandOnInjectedPasteboardAfterCaptureDisarms() { + let scratch = ScratchPasteboard() + let service = TerminalPasteboardService(standardPasteboard: scratch.pasteboard) + service.captureNextStandardClipboardWrite { + service.writeString("diverted", to: GHOSTTY_CLIPBOARD_STANDARD) + return true + } + let marker = "after-capture-\(UUID().uuidString)" + service.writeString(marker, to: GHOSTTY_CLIPBOARD_STANDARD) + #expect(scratch.pasteboard.string(forType: .string) == marker) + } } @Suite("Image materialization and temp-file ownership") diff --git a/Packages/macOS/CmuxTerminalCore/Sources/CmuxTerminalCore/Clipboard/TerminalClipboardWriting.swift b/Packages/macOS/CmuxTerminalCore/Sources/CmuxTerminalCore/Clipboard/TerminalClipboardWriting.swift index a75259ce5cc1..66931919498a 100644 --- a/Packages/macOS/CmuxTerminalCore/Sources/CmuxTerminalCore/Clipboard/TerminalClipboardWriting.swift +++ b/Packages/macOS/CmuxTerminalCore/Sources/CmuxTerminalCore/Clipboard/TerminalClipboardWriting.swift @@ -12,14 +12,34 @@ public protocol TerminalClipboardWriting: AnyObject, Sendable { /// Writes a string to the given ghostty clipboard location. /// /// When a one-shot capture is armed via - /// ``captureNextStandardClipboardWrite(_:)``, a standard-location write is - /// diverted into the capture instead of the system pasteboard. + /// ``captureNextStandardClipboardWrite(matching:_:)``, a standard-location + /// write the capture's predicate accepts is diverted into the capture + /// instead of the system pasteboard; a write it rejects reaches the + /// pasteboard and leaves the capture armed. func writeString(_ string: String, to location: ghostty_clipboard_e) - /// Arms a one-shot diversion of the next standard-clipboard write that - /// happens while `action` runs, returning the diverted string. + /// Arms a one-shot diversion of the next matching standard-clipboard + /// write that happens while `action` runs, returning the diverted string. /// - /// Returns `nil` when `action` reports failure or no write occurred. + /// `predicate` decides whether a given write is the one this capture is + /// waiting for; non-matching writes (e.g. a concurrent user copy) pass + /// through to the real pasteboard un-swallowed. + /// + /// Returns `nil` when `action` reports failure or no matching write + /// occurred. + @discardableResult + func captureNextStandardClipboardWrite( + matching predicate: @escaping @Sendable (String) -> Bool, + _ action: () -> Bool + ) -> String? +} + +extension TerminalClipboardWriting { + /// Arms a one-shot diversion of the next standard-clipboard write, + /// accepting any payload. Prefer the `matching:` variant with the + /// narrowest predicate the expected payload allows. @discardableResult - func captureNextStandardClipboardWrite(_ action: () -> Bool) -> String? + public func captureNextStandardClipboardWrite(_ action: () -> Bool) -> String? { + captureNextStandardClipboardWrite(matching: { _ in true }, action) + } } diff --git a/Sources/TerminalController.swift b/Sources/TerminalController.swift index 13efbd01ab91..47a9350c6067 100644 --- a/Sources/TerminalController.swift +++ b/Sources/TerminalController.swift @@ -687,6 +687,21 @@ class TerminalController { return trimmed.hasPrefix("/") ? trimmed : nil } + /// Whether a standard-clipboard write plausibly is the file path a + /// `write_screen_file:copy` / `write_active_file:copy` binding action + /// produces: a single absolute path (or file URL) to an existing file. + /// Gates the VT-export clipboard capture so it cannot swallow an + /// unrelated write (e.g. a user copy) that races the export. + nonisolated static func isPlausibleExportedScreenPath( + _ raw: String, + fileManager: FileManager = .default + ) -> Bool { + guard let path = normalizedExportedScreenPath(raw) else { return false } + var isDirectory = ObjCBool(false) + return fileManager.fileExists(atPath: path, isDirectory: &isDirectory) + && !isDirectory.boolValue + } + nonisolated static func shouldRemoveExportedScreenFile( fileURL: URL, temporaryDirectory: URL = FileManager.default.temporaryDirectory @@ -5440,7 +5455,9 @@ class TerminalController { normalizeLineEndings: Bool = true ) -> String? { var actionSucceeded = false - let exportedPath = GhosttyApp.terminalPasteboard.captureNextStandardClipboardWrite { + let exportedPath = GhosttyApp.terminalPasteboard.captureNextStandardClipboardWrite( + matching: { Self.isPlausibleExportedScreenPath($0) } + ) { let ok = terminalPanel.performInternalBindingAction(bindingAction) actionSucceeded = ok return ok diff --git a/cmuxTests/SessionPersistenceTests.swift b/cmuxTests/SessionPersistenceTests.swift index 1f2146f9eeba..ec4c0f44c758 100644 --- a/cmuxTests/SessionPersistenceTests.swift +++ b/cmuxTests/SessionPersistenceTests.swift @@ -708,6 +708,23 @@ final class SessionPersistenceTests: XCTestCase { XCTAssertNil(TerminalController.normalizedExportedScreenPath(nil)) } + func testIsPlausibleExportedScreenPathRequiresExistingFile() throws { + let directory = FileManager.default.temporaryDirectory + .appendingPathComponent("cmux-export-path-tests-\(UUID().uuidString)", isDirectory: true) + try FileManager.default.createDirectory(at: directory, withIntermediateDirectories: true) + defer { try? FileManager.default.removeItem(at: directory) } + let existing = directory.appendingPathComponent("screen.vt") + try Data("screen".utf8).write(to: existing) + + XCTAssertTrue(TerminalController.isPlausibleExportedScreenPath(existing.path)) + XCTAssertTrue(TerminalController.isPlausibleExportedScreenPath("file://\(existing.path)")) + // Typical user copies must never be mistaken for an export path. + XCTAssertFalse(TerminalController.isPlausibleExportedScreenPath("user copied text")) + XCTAssertFalse(TerminalController.isPlausibleExportedScreenPath(existing.path + "-missing")) + // Directories are not export payloads. + XCTAssertFalse(TerminalController.isPlausibleExportedScreenPath(directory.path)) + } + func testNormalizedMobileVTExportTextSplitsGhosttyCRLFRows() { let normalized = TerminalController.normalizedMobileVTExportText("first\r\nsecond\r\nthird") let rows = normalized.split(separator: "\n", omittingEmptySubsequences: false).map(String.init) From cae1ca147cdeb268ebbfe6a95b94c5fed0295581 Mon Sep 17 00:00:00 2001 From: a05031113 Date: Wed, 15 Jul 2026 00:58:39 +0800 Subject: [PATCH 2/4] Fix Cmd+C swallowed by stale selection validation; guard empty clipboard writes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two more clipboard failures found while dogfooding on a Claude Code (agent TUI) pane, where output redraws continuously: 1. validateUserInterfaceItem gated Copy on ghostty_surface_has_selection, which can report false while the runtime still holds a live selection under constant TUI redraw (the selection highlight stays visible and copy-on-select still produces the full text). The disabled menu item then swallows Cmd+C at key-equivalent dispatch, so the runtime's own copy binding never runs. Repro fingerprint: Cmd+C only worked while the Edit menu was held open (menu tracking lets the key fall through to the surface). Copy is now enabled whenever a surface exists; copying with no selection remains a harmless no-op. 2. writeString cleared the pasteboard before writing even for an empty payload (e.g. copy-on-select firing after the selection was already invalidated), silently destroying whatever the user last copied — paste turns up empty and clipboard managers record nothing. Empty standard writes are now ignored and logged. Also adds DEBUG-level cmuxDebugLog probes on the ghostty write-clipboard callback, mirroring the existing read-side logging. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01SwGyYRt9QjDUhimcp2xB7E --- .../TerminalPasteboardService.swift | 9 ++++++++ .../TerminalPasteboardServiceTests.swift | 13 +++++++++++ Sources/GhosttyTerminalView.swift | 22 ++++++++++++++++--- 3 files changed, 41 insertions(+), 3 deletions(-) diff --git a/Packages/macOS/CmuxTerminal/Sources/CmuxTerminal/Services/Pasteboard/TerminalPasteboardService.swift b/Packages/macOS/CmuxTerminal/Sources/CmuxTerminal/Services/Pasteboard/TerminalPasteboardService.swift index 0efc42ce996e..707f6fb9c8a2 100644 --- a/Packages/macOS/CmuxTerminal/Sources/CmuxTerminal/Services/Pasteboard/TerminalPasteboardService.swift +++ b/Packages/macOS/CmuxTerminal/Sources/CmuxTerminal/Services/Pasteboard/TerminalPasteboardService.swift @@ -143,6 +143,15 @@ extension TerminalPasteboardService: TerminalClipboardWriting { } } + // An empty payload (e.g. copy-on-select firing after a TUI redraw + // already invalidated the selection) must not clear the clipboard: + // clearContents-then-write-nothing silently destroys whatever the + // user last copied. + guard !string.isEmpty else { + Self.logger.info("ignored empty clipboard write") + return + } + guard let pasteboard = pasteboard(for: location) else { return } pasteboard.clearContents() if !pasteboard.setString(string, forType: .string) { diff --git a/Packages/macOS/CmuxTerminal/Tests/CmuxTerminalTests/Services/TerminalPasteboardServiceTests.swift b/Packages/macOS/CmuxTerminal/Tests/CmuxTerminalTests/Services/TerminalPasteboardServiceTests.swift index 917ba1f14ca8..810e9aac960a 100644 --- a/Packages/macOS/CmuxTerminal/Tests/CmuxTerminalTests/Services/TerminalPasteboardServiceTests.swift +++ b/Packages/macOS/CmuxTerminal/Tests/CmuxTerminalTests/Services/TerminalPasteboardServiceTests.swift @@ -147,6 +147,19 @@ struct ClipboardWriteCaptureTests { #expect(scratch.pasteboard.string(forType: .string) == userCopy) } + @Test func emptyStandardWriteDoesNotClearExistingClipboardContent() { + let scratch = ScratchPasteboard() + let service = TerminalPasteboardService(standardPasteboard: scratch.pasteboard) + let marker = "existing-content-\(UUID().uuidString)" + service.writeString(marker, to: GHOSTTY_CLIPBOARD_STANDARD) + + // An empty payload (e.g. copy-on-select firing after the selection + // was already invalidated) must be a no-op, not a destructive clear. + service.writeString("", to: GHOSTTY_CLIPBOARD_STANDARD) + + #expect(scratch.pasteboard.string(forType: .string) == marker) + } + @Test func standardWritesLandOnInjectedPasteboardAfterCaptureDisarms() { let scratch = ScratchPasteboard() let service = TerminalPasteboardService(standardPasteboard: scratch.pasteboard) diff --git a/Sources/GhosttyTerminalView.swift b/Sources/GhosttyTerminalView.swift index e4cd62df46ea..81bf2092ffd2 100644 --- a/Sources/GhosttyTerminalView.swift +++ b/Sources/GhosttyTerminalView.swift @@ -870,13 +870,24 @@ class GhosttyApp { } runtimeConfig.write_clipboard_cb = { _, location, content, len, _ in // Write clipboard - guard let content = content, len > 0 else { return } + guard let content = content, len > 0 else { + #if DEBUG + cmuxDebugLog("terminal.clipboard.write EMPTY payload location=\(location.rawValue) len=\(len)") + #endif + return + } + #if DEBUG + cmuxDebugLog("terminal.clipboard.write location=\(location.rawValue) items=\(len)") + #endif let buffer = UnsafeBufferPointer(start: content, count: Int(len)) var fallback: String? for item in buffer { guard let dataPtr = item.data else { continue } let value = String(cString: dataPtr) + #if DEBUG + cmuxDebugLog("terminal.clipboard.write item mime=\(item.mime.map { String(cString: $0) } ?? "nil") length=\(value.count)") + #endif if let mimePtr = item.mime { let mime = String(cString: mimePtr) @@ -5195,8 +5206,13 @@ class GhosttyNSView: NSView, NSUserInterfaceValidations { func validateUserInterfaceItem(_ item: NSValidatedUserInterfaceItem) -> Bool { switch item.action { case #selector(copy(_:)): - guard let surface = surface else { return false } - return hasCopyableTerminalSelection(surface: surface) + // Enabled whenever a surface exists, not gated on + // ghostty_surface_has_selection: that flag can report false while + // the runtime still holds a live selection (e.g. under constant + // TUI redraw), and a disabled menu item swallows Cmd+C before the + // runtime's own copy binding can handle it. Copy with no + // selection is a harmless no-op. + return surface != nil case #selector(paste(_:)): return GhosttyApp.terminalPasteboard.hasString(for: GHOSTTY_CLIPBOARD_STANDARD) case #selector(pasteAsPlainText(_:)): From 8eb73949dcf5a3613ad0ccce15170c9627237671 Mon Sep 17 00:00:00 2001 From: a05031113 Date: Wed, 15 Jul 2026 01:15:10 +0800 Subject: [PATCH 3/4] Address review: atomic capture claim, changeCount-guarded retry, pure-string export predicate MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - writeString: only the write that atomically clears the one-shot slot captures; a concurrent matching write that loses the claim race falls through to the pasteboard instead of being swallowed (Greptile P1, CodeRabbit critical). - Retry after a failed setString only runs while changeCount still matches our clearContents, so it can never clobber a newer concurrent write (Greptile P1). - isPlausibleExportedScreenPath: replaced the fileExists check with a pure string prefix check against the process temporary directory (accepting both /var and /private/var spellings) — no filesystem I/O inside the ghostty runtime callback, and existing absolute paths a user copies (e.g. /etc/hosts) no longer match (Greptile P1 + P2). Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01SwGyYRt9QjDUhimcp2xB7E --- .../TerminalPasteboardService.swift | 37 +++++++++++++------ Sources/TerminalController.swift | 31 ++++++++++++---- cmuxTests/SessionPersistenceTests.swift | 28 +++++++------- 3 files changed, 63 insertions(+), 33 deletions(-) diff --git a/Packages/macOS/CmuxTerminal/Sources/CmuxTerminal/Services/Pasteboard/TerminalPasteboardService.swift b/Packages/macOS/CmuxTerminal/Sources/CmuxTerminal/Services/Pasteboard/TerminalPasteboardService.swift index 707f6fb9c8a2..2f2dfd94cd35 100644 --- a/Packages/macOS/CmuxTerminal/Sources/CmuxTerminal/Services/Pasteboard/TerminalPasteboardService.swift +++ b/Packages/macOS/CmuxTerminal/Sources/CmuxTerminal/Services/Pasteboard/TerminalPasteboardService.swift @@ -129,17 +129,26 @@ extension TerminalPasteboardService: TerminalClipboardWriting { if let armed { if armed.accepts(string) { + // Claim the one-shot slot atomically: only the write that + // actually clears it may capture. A concurrent matching + // write that loses this race falls through to the real + // pasteboard instead of overwriting the captured value + // and being swallowed. standardClipboardWriteCaptureLock.lock() - if standardClipboardWriteCapture === armed { + let claimed = standardClipboardWriteCapture === armed + if claimed { standardClipboardWriteCapture = nil } standardClipboardWriteCaptureLock.unlock() - armed.capture(string) - return + if claimed { + armed.capture(string) + return + } + } else { + Self.logger.info( + "standard write passed through armed capture (length \(string.count, privacy: .public))" + ) } - Self.logger.info( - "standard write passed through armed capture (length \(string.count, privacy: .public))" - ) } } @@ -153,15 +162,19 @@ extension TerminalPasteboardService: TerminalClipboardWriting { } guard let pasteboard = pasteboard(for: location) else { return } - pasteboard.clearContents() + let clearedChangeCount = pasteboard.clearContents() if !pasteboard.setString(string, forType: .string) { // A contended pasteboard can reject the write after clearContents, - // leaving the clipboard empty. Re-declare and retry once so the - // failure is at least not silent data loss. - pasteboard.clearContents() - let retried = pasteboard.setString(string, forType: .string) + // leaving the clipboard empty. Retry once so the failure is not + // silent data loss — but only while nothing else has written in + // the meantime, so the retry never clobbers a newer value. + var retried = false + if pasteboard.changeCount == clearedChangeCount { + pasteboard.clearContents() + retried = pasteboard.setString(string, forType: .string) + } Self.logger.error( - "pasteboard setString failed (length \(string.count, privacy: .public)), retry \(retried ? "succeeded" : "failed", privacy: .public)" + "pasteboard setString failed (length \(string.count, privacy: .public)), retry \(retried ? "succeeded" : "skipped-or-failed", privacy: .public)" ) } } diff --git a/Sources/TerminalController.swift b/Sources/TerminalController.swift index 47a9350c6067..6ccba3b04a91 100644 --- a/Sources/TerminalController.swift +++ b/Sources/TerminalController.swift @@ -689,17 +689,32 @@ class TerminalController { /// Whether a standard-clipboard write plausibly is the file path a /// `write_screen_file:copy` / `write_active_file:copy` binding action - /// produces: a single absolute path (or file URL) to an existing file. - /// Gates the VT-export clipboard capture so it cannot swallow an - /// unrelated write (e.g. a user copy) that races the export. + /// produces: a single absolute path (or file URL) under the process + /// temporary directory, where ghostty writes screen exports. Gates the + /// VT-export clipboard capture so it cannot swallow an unrelated write + /// (e.g. a user copying some other absolute path) that races the export. + /// + /// Deliberately a pure string check: it runs inside the ghostty + /// write-clipboard runtime callback, where filesystem I/O (a `fileExists` + /// on a slow or network volume) could stall terminal processing. nonisolated static func isPlausibleExportedScreenPath( _ raw: String, - fileManager: FileManager = .default + temporaryDirectory: URL = FileManager.default.temporaryDirectory ) -> Bool { - guard let path = normalizedExportedScreenPath(raw) else { return false } - var isDirectory = ObjCBool(false) - return fileManager.fileExists(atPath: path, isDirectory: &isDirectory) - && !isDirectory.boolValue + guard let path = normalizedExportedScreenPath(raw), + !path.contains("\n") else { + return false + } + // TMPDIR is /var/... which is a symlink to /private/var/...; accept + // either spelling without resolving symlinks on the hot path. + let temporary = temporaryDirectory.standardizedFileURL.path + var prefixes = [temporary] + if temporary.hasPrefix("/var/") { + prefixes.append("/private" + temporary) + } else if temporary.hasPrefix("/private/var/") { + prefixes.append(String(temporary.dropFirst("/private".count))) + } + return prefixes.contains { path.hasPrefix($0.hasSuffix("/") ? $0 : $0 + "/") } } nonisolated static func shouldRemoveExportedScreenFile( diff --git a/cmuxTests/SessionPersistenceTests.swift b/cmuxTests/SessionPersistenceTests.swift index ec4c0f44c758..54829e850926 100644 --- a/cmuxTests/SessionPersistenceTests.swift +++ b/cmuxTests/SessionPersistenceTests.swift @@ -708,21 +708,23 @@ final class SessionPersistenceTests: XCTestCase { XCTAssertNil(TerminalController.normalizedExportedScreenPath(nil)) } - func testIsPlausibleExportedScreenPathRequiresExistingFile() throws { - let directory = FileManager.default.temporaryDirectory - .appendingPathComponent("cmux-export-path-tests-\(UUID().uuidString)", isDirectory: true) - try FileManager.default.createDirectory(at: directory, withIntermediateDirectories: true) - defer { try? FileManager.default.removeItem(at: directory) } - let existing = directory.appendingPathComponent("screen.vt") - try Data("screen".utf8).write(to: existing) - - XCTAssertTrue(TerminalController.isPlausibleExportedScreenPath(existing.path)) - XCTAssertTrue(TerminalController.isPlausibleExportedScreenPath("file://\(existing.path)")) + func testIsPlausibleExportedScreenPathRequiresTemporaryDirectoryPrefix() { + let temporary = FileManager.default.temporaryDirectory + let exportPath = temporary.appendingPathComponent("cmux-screen-\(UUID().uuidString).vt").path + + XCTAssertTrue(TerminalController.isPlausibleExportedScreenPath(exportPath)) + XCTAssertTrue(TerminalController.isPlausibleExportedScreenPath("file://\(exportPath)")) + // Both spellings of the /var -> /private/var symlink are accepted. + if exportPath.hasPrefix("/var/") { + XCTAssertTrue(TerminalController.isPlausibleExportedScreenPath("/private" + exportPath)) + } // Typical user copies must never be mistaken for an export path. XCTAssertFalse(TerminalController.isPlausibleExportedScreenPath("user copied text")) - XCTAssertFalse(TerminalController.isPlausibleExportedScreenPath(existing.path + "-missing")) - // Directories are not export payloads. - XCTAssertFalse(TerminalController.isPlausibleExportedScreenPath(directory.path)) + // Existing absolute paths outside the temporary directory (e.g. a + // copied /etc/hosts) are not export payloads. + XCTAssertFalse(TerminalController.isPlausibleExportedScreenPath("/etc/hosts")) + // Interior newlines mean a multi-line user copy, not a path. + XCTAssertFalse(TerminalController.isPlausibleExportedScreenPath(exportPath + "\n/second/line")) } func testNormalizedMobileVTExportTextSplitsGhosttyCRLFRows() { From 8f81791013b74378d7ed39cfd09e00bbb7fa5f42 Mon Sep 17 00:00:00 2001 From: a05031113 Date: Wed, 15 Jul 2026 02:05:50 +0800 Subject: [PATCH 4/4] Reject overlapping clipboard captures instead of letting them steal writes Arming a capture while another is in flight replaced the process-wide slot: with predicate-scoped captures, the first operation's export write could satisfy the second capture (both predicates accept export paths), handing pane A's export to pane B's snapshot while pane A got nil. captureNextStandardClipboardWrite now returns nil without arming when the slot is already held. Every caller already treats nil as "fall back to a non-capture read", so an overlapping export degrades gracefully instead of cross-delivering content. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01SwGyYRt9QjDUhimcp2xB7E --- .../Pasteboard/TerminalPasteboardService.swift | 15 ++++++++++++++- .../Services/TerminalPasteboardServiceTests.swift | 13 +++++++++++++ .../Clipboard/TerminalClipboardWriting.swift | 6 ++++-- 3 files changed, 31 insertions(+), 3 deletions(-) diff --git a/Packages/macOS/CmuxTerminal/Sources/CmuxTerminal/Services/Pasteboard/TerminalPasteboardService.swift b/Packages/macOS/CmuxTerminal/Sources/CmuxTerminal/Services/Pasteboard/TerminalPasteboardService.swift index 2f2dfd94cd35..3d5b74d94353 100644 --- a/Packages/macOS/CmuxTerminal/Sources/CmuxTerminal/Services/Pasteboard/TerminalPasteboardService.swift +++ b/Packages/macOS/CmuxTerminal/Sources/CmuxTerminal/Services/Pasteboard/TerminalPasteboardService.swift @@ -190,6 +190,12 @@ extension TerminalPasteboardService: TerminalClipboardWriting { /// as narrow as the expected payload allows — e.g. "an existing file /// under the temporary directory" for a VT screen export. /// - action: The operation expected to trigger the write. + /// + /// Returns `nil` without running a capture when another capture is + /// already in flight: replacing the armed slot would let one operation's + /// write satisfy the other's capture (both predicates accept export + /// paths), handing the wrong content to the wrong caller. Callers + /// already treat `nil` as "fall back to a non-capture read". @discardableResult public func captureNextStandardClipboardWrite( matching predicate: @escaping @Sendable (String) -> Bool = { _ in true }, @@ -197,8 +203,15 @@ extension TerminalPasteboardService: TerminalClipboardWriting { ) -> String? { let capture = ClipboardWriteCapture(accepts: predicate) standardClipboardWriteCaptureLock.lock() - standardClipboardWriteCapture = capture + let alreadyArmed = standardClipboardWriteCapture != nil + if !alreadyArmed { + standardClipboardWriteCapture = capture + } standardClipboardWriteCaptureLock.unlock() + guard !alreadyArmed else { + Self.logger.info("clipboard capture rejected: another capture is in flight") + return nil + } defer { standardClipboardWriteCaptureLock.lock() diff --git a/Packages/macOS/CmuxTerminal/Tests/CmuxTerminalTests/Services/TerminalPasteboardServiceTests.swift b/Packages/macOS/CmuxTerminal/Tests/CmuxTerminalTests/Services/TerminalPasteboardServiceTests.swift index 810e9aac960a..53943b79874d 100644 --- a/Packages/macOS/CmuxTerminal/Tests/CmuxTerminalTests/Services/TerminalPasteboardServiceTests.swift +++ b/Packages/macOS/CmuxTerminal/Tests/CmuxTerminalTests/Services/TerminalPasteboardServiceTests.swift @@ -147,6 +147,19 @@ struct ClipboardWriteCaptureTests { #expect(scratch.pasteboard.string(forType: .string) == userCopy) } + @Test func overlappingCaptureIsRejectedAndDoesNotStealTheFirstCapturesWrite() { + let service = TerminalPasteboardService() + let outer = service.captureNextStandardClipboardWrite { () -> Bool in + // A second capture while one is in flight must be rejected... + let inner = service.captureNextStandardClipboardWrite { true } + #expect(inner == nil) + // ...and the original capture still owns the next matching write. + service.writeString("outer-value", to: GHOSTTY_CLIPBOARD_STANDARD) + return true + } + #expect(outer == "outer-value") + } + @Test func emptyStandardWriteDoesNotClearExistingClipboardContent() { let scratch = ScratchPasteboard() let service = TerminalPasteboardService(standardPasteboard: scratch.pasteboard) diff --git a/Packages/macOS/CmuxTerminalCore/Sources/CmuxTerminalCore/Clipboard/TerminalClipboardWriting.swift b/Packages/macOS/CmuxTerminalCore/Sources/CmuxTerminalCore/Clipboard/TerminalClipboardWriting.swift index 66931919498a..58a59ee4f2e9 100644 --- a/Packages/macOS/CmuxTerminalCore/Sources/CmuxTerminalCore/Clipboard/TerminalClipboardWriting.swift +++ b/Packages/macOS/CmuxTerminalCore/Sources/CmuxTerminalCore/Clipboard/TerminalClipboardWriting.swift @@ -25,8 +25,10 @@ public protocol TerminalClipboardWriting: AnyObject, Sendable { /// waiting for; non-matching writes (e.g. a concurrent user copy) pass /// through to the real pasteboard un-swallowed. /// - /// Returns `nil` when `action` reports failure or no matching write - /// occurred. + /// Returns `nil` when `action` reports failure, no matching write + /// occurred, or another capture is already in flight (overlapping + /// captures are rejected rather than allowed to steal each other's + /// writes; callers treat `nil` as "fall back to a non-capture read"). @discardableResult func captureNextStandardClipboardWrite( matching predicate: @escaping @Sendable (String) -> Bool,