From ae5a2abea9d9c2dec5ca3bfb563800bf2d8a9162 Mon Sep 17 00:00:00 2001 From: Ngo Quoc Dat Date: Thu, 9 Jul 2026 13:18:56 +0700 Subject: [PATCH] fix(connections): drain password-source command output without a reader race --- CHANGELOG.md | 1 + .../Connection/PasswordSourceResolver.swift | 58 ++++++++----- .../PasswordSourceResolverTests.swift | 82 +++++++++++++++++++ 3 files changed, 122 insertions(+), 19 deletions(-) create mode 100644 TableProTests/Core/Utilities/Connection/PasswordSourceResolverTests.swift diff --git a/CHANGELOG.md b/CHANGELOG.md index 2658f3de1..922dfa5c9 100644 --- a/CHANGELOG.md +++ b/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. (#1841) - 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) diff --git a/TablePro/Core/Utilities/Connection/PasswordSourceResolver.swift b/TablePro/Core/Utilities/Connection/PasswordSourceResolver.swift index 1f928df55..4173cde27 100644 --- a/TablePro/Core/Utilities/Connection/PasswordSourceResolver.swift +++ b/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"] diff --git a/TableProTests/Core/Utilities/Connection/PasswordSourceResolverTests.swift b/TableProTests/Core/Utilities/Connection/PasswordSourceResolverTests.swift new file mode 100644 index 000000000..5fc4efc60 --- /dev/null +++ b/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 { + } + } +}