fix(connections): drain password-source command output without a reader race

Author: datlechin

CHANGELOG.md

@@ -9,6 +9,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ### Fixed
 
+- Reading a connection password from a command, 1Password, Vault, or AWS Secrets Manager no longer occasionally returns corrupted output from a race while reading the command's output.
 - The Structure tab filter and column sort now update the grid instead of leaving stale rows on screen.
 - The row details inspector now shows the selected row's values, including JSON, when a column value filter is active, and a JSON or serialized value you open now follows the selected row as you move between rows. (#1837)
 - Copying, duplicating, and deleting rows now act on the rows you selected when a column value filter is active, instead of the rows sitting at those positions in the unfiltered result. (#1837)

TablePro/Core/Utilities/Connection/PasswordSourceResolver.swift

@@ -156,20 +156,26 @@ enum PasswordSourceResolver {
 
             let stdoutCollector = PipeDataCollector(maxBytes: maxOutputBytes)
             let stderrCollector = PipeDataCollector(maxBytes: maxOutputBytes)
-            stdoutPipe.fileHandleForReading.readabilityHandler = { handle in
-                let chunk = handle.availableData
-                guard !chunk.isEmpty else { return }
-                stdoutCollector.append(chunk)
-                if stdoutCollector.overflowed, process.isRunning {
+            try process.run()
+
+            let drainGroup = DispatchGroup()
+            let drainQueue = DispatchQueue(label: "com.TablePro.PasswordSourceResolver.pipe-drain", attributes: .concurrent)
+            drainPipe(
+                stdoutPipe.fileHandleForReading,
+                into: stdoutCollector,
+                using: drainGroup,
+                queue: drainQueue
+            ) {
+                if process.isRunning {
                     process.terminate()
                 }
             }
-            stderrPipe.fileHandleForReading.readabilityHandler = { handle in
-                let chunk = handle.availableData
-                if !chunk.isEmpty { stderrCollector.append(chunk) }
-            }
-
-            try process.run()
+            drainPipe(
+                stderrPipe.fileHandleForReading,
+                into: stderrCollector,
+                using: drainGroup,
+                queue: drainQueue
+            )
 
             let didTimeout = AtomicFlag()
             let timeoutTask = Task.detached {
@@ -182,14 +188,7 @@ enum PasswordSourceResolver {
 
             process.waitUntilExit()
             timeoutTask.cancel()
-
-            stdoutPipe.fileHandleForReading.readabilityHandler = nil
-            stderrPipe.fileHandleForReading.readabilityHandler = nil
-
-            let remainingStdout = stdoutPipe.fileHandleForReading.readDataToEndOfFile()
-            if !remainingStdout.isEmpty { stdoutCollector.append(remainingStdout) }
-            let remainingStderr = stderrPipe.fileHandleForReading.readDataToEndOfFile()
-            if !remainingStderr.isEmpty { stderrCollector.append(remainingStderr) }
+            drainGroup.wait()
 
             if stdoutCollector.overflowed {
                 throw ResolutionError.outputTooLarge
@@ -212,6 +211,27 @@ enum PasswordSourceResolver {
         return try nonEmpty(output.trimmingCharacters(in: .whitespacesAndNewlines))
     }
 
+    private static func drainPipe(
+        _ handle: FileHandle,
+        into collector: PipeDataCollector,
+        using group: DispatchGroup,
+        queue: DispatchQueue,
+        onOverflow: (() -> Void)? = nil
+    ) {
+        group.enter()
+        queue.async {
+            defer { group.leave() }
+            while true {
+                let chunk = handle.readData(ofLength: 8_192)
+                guard !chunk.isEmpty else { return }
+                collector.append(chunk)
+                if collector.overflowed {
+                    onOverflow?()
+                }
+            }
+        }
+    }
+
     private static func augmentedEnvironment() -> [String: String] {
         var environment = ProcessInfo.processInfo.environment
         let toolPaths = ["/usr/local/bin", "/opt/homebrew/bin", "/usr/bin", "/bin", "/usr/sbin", "/sbin"]

TableProTests/Core/Utilities/Connection/PasswordSourceResolverTests.swift

@@ -0,0 +1,82 @@
+//
+//  PasswordSourceResolverTests.swift
+//  TableProTests
+//
+
+import Foundation
+import Testing
+
+@testable import TablePro
+
+@Suite("PasswordSourceResolver command output")
+struct PasswordSourceResolverTests {
+    @Test("Command stdout is returned trimmed")
+    func returnsTrimmedStdout() async throws {
+        let output = try await PasswordSourceResolver.resolveCommand(
+            shell: "printf '  hunter2\\n'",
+            timeoutSeconds: 5
+        )
+        #expect(output == "hunter2")
+    }
+
+    @Test("Large stdout drains in full and in order while stderr is also draining")
+    func drainsLargeStdoutWithoutTruncation() async throws {
+        let shell = """
+        head -c 300000 /dev/zero | tr '\\0' 'a'
+        head -c 300000 /dev/zero | tr '\\0' 'b' >&2
+        """
+        let output = try await PasswordSourceResolver.resolveCommand(shell: shell, timeoutSeconds: 30)
+        #expect(output.count == 300_000)
+        #expect(output.allSatisfy { $0 == "a" })
+    }
+
+    @Test("A failing command surfaces the exit code and stderr")
+    func failingCommandSurfacesStderr() async throws {
+        do {
+            _ = try await PasswordSourceResolver.resolveCommand(
+                shell: "printf boom >&2; exit 7",
+                timeoutSeconds: 5
+            )
+            Issue.record("Expected resolveCommand to throw")
+        } catch let PasswordSourceResolver.ResolutionError.commandFailed(exitCode, stderr) {
+            #expect(exitCode == 7)
+            #expect(stderr.contains("boom"))
+        }
+    }
+
+    @Test("Output over the size cap fails as too large")
+    func overflowFailsAsTooLarge() async throws {
+        do {
+            _ = try await PasswordSourceResolver.resolveCommand(
+                shell: "yes | head -c 1200000",
+                timeoutSeconds: 30
+            )
+            Issue.record("Expected outputTooLarge")
+        } catch PasswordSourceResolver.ResolutionError.outputTooLarge {
+        }
+    }
+
+    @Test("A command that exceeds the timeout is terminated")
+    func timeoutTerminatesCommand() async throws {
+        do {
+            _ = try await PasswordSourceResolver.resolveCommand(
+                shell: "sleep 30",
+                timeoutSeconds: 1
+            )
+            Issue.record("Expected commandTimedOut")
+        } catch PasswordSourceResolver.ResolutionError.commandTimedOut {
+        }
+    }
+
+    @Test("Empty output fails as an empty password")
+    func emptyOutputFailsAsEmpty() async throws {
+        do {
+            _ = try await PasswordSourceResolver.resolveCommand(
+                shell: "true",
+                timeoutSeconds: 5
+            )
+            Issue.record("Expected emptyPassword")
+        } catch PasswordSourceResolver.ResolutionError.emptyPassword {
+        }
+    }
+}