Merge pull request #125 from darkroomengineering/port/ipv6-scp-normalization

fix(ssh): bracket IPv6 hosts in our scp upload call sites too

Author: arzafran

Sources/RemoteSSHConnectionPolicy.swift

@@ -86,4 +86,49 @@ enum RemoteSSHConnectionPolicy {
     static func shellSingleQuoted(_ value: String) -> String {
         "'" + value.replacingOccurrences(of: "'", with: "'\"'\"'") + "'"
     }
+
+    /// Rewrites a destination for `scp`'s combined `host:path` argument syntax by
+    /// bracketing a bare IPv6 literal host (`user@2001:db8::1` -> `user@[2001:db8::1]`).
+    ///
+    /// `ssh` takes the destination as its own argument, so a bare IPv6 literal like
+    /// `2001:db8::1` is unambiguous there (see `CLI+SSH.swift`'s `normalizeSSHDestination`,
+    /// which instead *strips* brackets for that call). `scp` glues the destination and the
+    /// remote path together with a colon (`host:path`), so an un-bracketed IPv6 host's own
+    /// colons collide with that separator and scp misparses the path. Only bare/unbracketed
+    /// IPv6 hosts are rewritten — `user@host`, hostnames, IPv4 literals, and already-bracketed
+    /// hosts pass through unchanged (#4948 follow-up: the ssh-only fix in `CLI+SSH.swift`
+    /// didn't cover our scp call sites).
+    static func scpRemoteDestination(_ destination: String) -> String {
+        let trimmedDestination = destination.trimmingCharacters(in: .whitespacesAndNewlines)
+        guard !trimmedDestination.isEmpty else { return destination }
+
+        let parts = trimmedDestination.split(separator: "@", maxSplits: 1, omittingEmptySubsequences: false)
+        let userPart: String?
+        let hostPart: String
+        if parts.count == 2 {
+            userPart = String(parts[0])
+            hostPart = String(parts[1])
+        } else {
+            userPart = nil
+            hostPart = trimmedDestination
+        }
+
+        guard shouldBracketIPv6LiteralForSCP(hostPart) else {
+            return trimmedDestination
+        }
+
+        let bracketedHost = "[\(hostPart)]"
+        if let userPart {
+            return "\(userPart)@\(bracketedHost)"
+        }
+        return bracketedHost
+    }
+
+    private static func shouldBracketIPv6LiteralForSCP(_ host: String) -> Bool {
+        let trimmedHost = host.trimmingCharacters(in: .whitespacesAndNewlines)
+        return !trimmedHost.isEmpty &&
+            trimmedHost.contains(":") &&
+            !trimmedHost.hasPrefix("[") &&
+            !trimmedHost.hasSuffix("]")
+    }
 }

Sources/TerminalSSHSessionDetector.swift

@@ -154,7 +154,7 @@ struct DetectedSSHSession: Equatable {
             args += ["-o", option]
         }
 
-        args += [localPath, "\(Self.scpRemoteDestination(destination)):\(remotePath)"]
+        args += [localPath, "\(RemoteSSHConnectionPolicy.scpRemoteDestination(destination)):\(remotePath)"]
         return args
     }
 
@@ -311,40 +311,6 @@ struct DetectedSSHSession: Equatable {
             .first(where: { !$0.isEmpty })
     }
 
-    private static func scpRemoteDestination(_ destination: String) -> String {
-        let trimmedDestination = destination.trimmingCharacters(in: .whitespacesAndNewlines)
-        guard !trimmedDestination.isEmpty else { return destination }
-
-        let parts = trimmedDestination.split(separator: "@", maxSplits: 1, omittingEmptySubsequences: false)
-        let userPart: String?
-        let hostPart: String
-        if parts.count == 2 {
-            userPart = String(parts[0])
-            hostPart = String(parts[1])
-        } else {
-            userPart = nil
-            hostPart = trimmedDestination
-        }
-
-        guard shouldBracketIPv6Literal(hostPart) else {
-            return trimmedDestination
-        }
-
-        let bracketedHost = "[\(hostPart)]"
-        if let userPart {
-            return "\(userPart)@\(bracketedHost)"
-        }
-        return bracketedHost
-    }
-
-    private static func shouldBracketIPv6Literal(_ host: String) -> Bool {
-        let trimmedHost = host.trimmingCharacters(in: .whitespacesAndNewlines)
-        return !trimmedHost.isEmpty &&
-            trimmedHost.contains(":") &&
-            !trimmedHost.hasPrefix("[") &&
-            !trimmedHost.hasSuffix("]")
-    }
-
 #if DEBUG
     func scpArgumentsForTesting(localPath: String, remotePath: String) -> [String] {
         scpArguments(localPath: localPath, remotePath: remotePath)

Sources/WorkspaceRemoteSSHBatchCommandBuilder.swift

@@ -39,6 +39,32 @@ enum WorkspaceRemoteSSHBatchCommandBuilder {
         return args
     }
 
+    /// Builds the `scp` argument list for staging a local file to `configuration.destination`.
+    /// Shared by the `programad-remote` binary upload and the dropped-file upload so the
+    /// IPv6-bracketing fix (see `RemoteSSHConnectionPolicy.scpRemoteDestination`) only has
+    /// to be applied once.
+    static func scpUploadArguments(
+        configuration: WorkspaceRemoteConfiguration,
+        localPath: String,
+        remotePath: String
+    ) -> [String] {
+        let scpSSHOptions = RemoteSSHConnectionPolicy.backgroundOptions(configuration.sshOptions)
+        var args: [String] = ["-q", "-o", "ControlMaster=no"]
+        args += RemoteSSHConnectionPolicy.strictHostKeyCheckingArguments(unlessSetIn: scpSSHOptions)
+        if let port = configuration.port {
+            args += ["-P", String(port)]
+        }
+        if let identityFile = configuration.identityFile,
+           !identityFile.trimmingCharacters(in: .whitespacesAndNewlines).isEmpty {
+            args += ["-i", identityFile]
+        }
+        for option in scpSSHOptions {
+            args += ["-o", option]
+        }
+        args += [localPath, "\(RemoteSSHConnectionPolicy.scpRemoteDestination(configuration.destination)):\(remotePath)"]
+        return args
+    }
+
     private static func batchArguments(configuration: WorkspaceRemoteConfiguration) -> [String] {
         let effectiveSSHOptions = RemoteSSHConnectionPolicy.backgroundOptions(configuration.sshOptions)
         var args = RemoteSSHConnectionPolicy.keepaliveArguments

Sources/WorkspaceRemoteSessionController+DaemonInstall.swift

@@ -471,21 +471,11 @@ extension WorkspaceRemoteSessionController {
             ])
         }
 
-        let scpSSHOptions = RemoteSSHConnectionPolicy.backgroundOptions(configuration.sshOptions)
-        var scpArgs: [String] = ["-q"]
-        scpArgs += RemoteSSHConnectionPolicy.strictHostKeyCheckingArguments(unlessSetIn: scpSSHOptions)
-        scpArgs += ["-o", "ControlMaster=no"]
-        if let port = configuration.port {
-            scpArgs += ["-P", String(port)]
-        }
-        if let identityFile = configuration.identityFile,
-           !identityFile.trimmingCharacters(in: .whitespacesAndNewlines).isEmpty {
-            scpArgs += ["-i", identityFile]
-        }
-        for option in scpSSHOptions {
-            scpArgs += ["-o", option]
-        }
-        scpArgs += [localBinary.path, "\(configuration.destination):\(remoteTempPath)"]
+        let scpArgs = WorkspaceRemoteSSHBatchCommandBuilder.scpUploadArguments(
+            configuration: configuration,
+            localPath: localBinary.path,
+            remotePath: remoteTempPath
+        )
         let scpResult = try scpExec(arguments: scpArgs, timeout: 45)
         guard scpResult.status == 0 else {
             let detail = Self.bestErrorLine(stderr: scpResult.stderr, stdout: scpResult.stdout) ?? "scp exited \(scpResult.status)"
@@ -512,7 +502,6 @@ extension WorkspaceRemoteSessionController {
         _ fileURLs: [URL],
         operation: TerminalImageTransferOperation
     ) throws -> [String] {
-        let scpSSHOptions = RemoteSSHConnectionPolicy.backgroundOptions(configuration.sshOptions)
         return try performSCPUploadWithCancelCleanup(
             items: fileURLs,
             checkCancelled: { try operation.throwIfCancelled() },
@@ -524,19 +513,11 @@ extension WorkspaceRemoteSessionController {
 
                 let remotePath = Self.remoteDropPath(for: normalizedLocalURL)
                 record(remotePath)
-                var scpArgs: [String] = ["-q", "-o", "ControlMaster=no"]
-                scpArgs += RemoteSSHConnectionPolicy.strictHostKeyCheckingArguments(unlessSetIn: scpSSHOptions)
-                if let port = configuration.port {
-                    scpArgs += ["-P", String(port)]
-                }
-                if let identityFile = configuration.identityFile,
-                   !identityFile.trimmingCharacters(in: .whitespacesAndNewlines).isEmpty {
-                    scpArgs += ["-i", identityFile]
-                }
-                for option in scpSSHOptions {
-                    scpArgs += ["-o", option]
-                }
-                scpArgs += [normalizedLocalURL.path, "\(configuration.destination):\(remotePath)"]
+                let scpArgs = WorkspaceRemoteSSHBatchCommandBuilder.scpUploadArguments(
+                    configuration: configuration,
+                    localPath: normalizedLocalURL.path,
+                    remotePath: remotePath
+                )
 
                 let scpResult = try scpExec(arguments: scpArgs, timeout: 45, operation: operation)
                 guard scpResult.status == 0 else {

programaTests/WorkspaceRemoteConnectionTests.swift

@@ -1229,6 +1229,81 @@ final class WorkspaceRemoteConnectionTests: XCTestCase {
         XCTAssertEqual(scpArgs.last, "lawrence@[2001:db8::1]:/tmp/programa-drop-123.png")
     }
 
+    func testRemoteSSHConnectionPolicyScpRemoteDestinationBracketsBareIPv6Literal() {
+        XCTAssertEqual(
+            RemoteSSHConnectionPolicy.scpRemoteDestination("lawrence@2001:db8::1"),
+            "lawrence@[2001:db8::1]"
+        )
+        XCTAssertEqual(
+            RemoteSSHConnectionPolicy.scpRemoteDestination("2001:db8::1"),
+            "[2001:db8::1]"
+        )
+        XCTAssertEqual(
+            RemoteSSHConnectionPolicy.scpRemoteDestination("::1"),
+            "[::1]"
+        )
+    }
+
+    func testRemoteSSHConnectionPolicyScpRemoteDestinationPassesThroughNonBareIPv6Hosts() {
+        // Already-bracketed hosts are left alone.
+        XCTAssertEqual(
+            RemoteSSHConnectionPolicy.scpRemoteDestination("lawrence@[2001:db8::1]"),
+            "lawrence@[2001:db8::1]"
+        )
+        // IPv4 literals have no ambiguous colon, so they pass through untouched.
+        XCTAssertEqual(
+            RemoteSSHConnectionPolicy.scpRemoteDestination("lawrence@192.168.1.1"),
+            "lawrence@192.168.1.1"
+        )
+        // Plain hostnames and configured SSH aliases pass through untouched.
+        XCTAssertEqual(
+            RemoteSSHConnectionPolicy.scpRemoteDestination("cmux-macmini"),
+            "cmux-macmini"
+        )
+        XCTAssertEqual(
+            RemoteSSHConnectionPolicy.scpRemoteDestination("lawrence@example.com"),
+            "lawrence@example.com"
+        )
+    }
+
+    func testScpUploadArgumentsBracketsIPv6LiteralDestinationForDaemonBinaryUpload() {
+        let configuration = WorkspaceRemoteConfiguration(
+            destination: "2001:db8::1",
+            port: 2222,
+            identityFile: "/Users/test/.ssh/id_ed25519",
+            sshOptions: [
+                "ControlMaster=auto",
+                "ControlPersist=600",
+                "StrictHostKeyChecking=accept-new",
+            ],
+            localProxyPort: nil,
+            relayPort: nil,
+            relayID: nil,
+            relayToken: nil,
+            localSocketPath: nil,
+            terminalStartupCommand: "ssh [2001:db8::1]"
+        )
+
+        // This is the exact call `WorkspaceRemoteSessionController+DaemonInstall.swift`
+        // makes for both the programad-remote binary upload and dropped-file uploads
+        // (#4948 follow-up: the ssh-only fix in CLI+SSH.swift left these scp call
+        // sites choking on bracketless IPv6 hosts).
+        let arguments = WorkspaceRemoteSSHBatchCommandBuilder.scpUploadArguments(
+            configuration: configuration,
+            localPath: "/tmp/programad-remote",
+            remotePath: "/home/lawrence/.programa/remote/programad-remote"
+        )
+
+        XCTAssertEqual(
+            arguments.last,
+            "[2001:db8::1]:/home/lawrence/.programa/remote/programad-remote"
+        )
+        XCTAssertTrue(arguments.contains("-P"))
+        XCTAssertTrue(arguments.contains("2222"))
+        XCTAssertTrue(arguments.contains("-i"))
+        XCTAssertTrue(arguments.contains("/Users/test/.ssh/id_ed25519"))
+    }
+
     func testDetectsForegroundSSHSessionWithLowercaseAgentFlag() {
         let session = TerminalSSHSessionDetector.detectForTesting(
             ttyName: "/dev/ttys004",