From 983f0f5ef3a0a7f5a2ef54ec563626004d15453d Mon Sep 17 00:00:00 2001 From: GlassOnTin Date: Sun, 5 Apr 2026 12:05:07 +0100 Subject: [PATCH 01/10] Add SFTP client support (draft-ietf-secsh-filexfer) Implements a full SFTPv3 client on top of the existing SSH subsystem channel support, providing file transfer operations over SSH. Public API: - SftpClient interface with file I/O, stat, directory, and path operations - SftpAttributes, SftpFileHandle, SftpDirectoryEntry data types - SftpOpenFlag and SftpStatusCode enums - SftpException for protocol errors - SshClient.openSftp() and BlockingSshClient.openSftp() entry points Internal implementation: - SftpPacketIO: SFTP packet framing with chunk accumulation over SSH channel - SftpDispatcher: request ID sequencing and coroutine-based response routing - SftpFileAttributes: SFTPv3 ATTRS parser/serializer with flags-based fields - SftpClientImpl: full protocol implementation with INIT/VERSION handshake All operations are suspend functions supporting concurrent pipelined requests via ConcurrentHashMap + CompletableDeferred response matching. Handles OpenSSH symlink argument order quirk (reversed from spec). Includes integration tests against OpenSSH 9.9p2 via testcontainers. Refs: #96 --- sshlib/api.txt | 121 +++++ .../org/connectbot/sshlib/SftpClient.kt | 144 ++++++ .../org/connectbot/sshlib/SftpException.kt | 31 ++ .../kotlin/org/connectbot/sshlib/SftpTypes.kt | 92 ++++ .../kotlin/org/connectbot/sshlib/SshClient.kt | 30 ++ .../sshlib/blocking/BlockingSshClient.kt | 6 + .../sshlib/client/sftp/SftpClientImpl.kt | 437 ++++++++++++++++++ .../sshlib/client/sftp/SftpDispatcher.kt | 142 ++++++ .../sshlib/client/sftp/SftpFileAttributes.kt | 107 +++++ .../sshlib/client/sftp/SftpPacketIO.kt | 125 +++++ .../client/sftp/SftpClientIntegrationTest.kt | 331 +++++++++++++ 11 files changed, 1566 insertions(+) create mode 100644 sshlib/src/main/kotlin/org/connectbot/sshlib/SftpClient.kt create mode 100644 sshlib/src/main/kotlin/org/connectbot/sshlib/SftpException.kt create mode 100644 sshlib/src/main/kotlin/org/connectbot/sshlib/SftpTypes.kt create mode 100644 sshlib/src/main/kotlin/org/connectbot/sshlib/client/sftp/SftpClientImpl.kt create mode 100644 sshlib/src/main/kotlin/org/connectbot/sshlib/client/sftp/SftpDispatcher.kt create mode 100644 sshlib/src/main/kotlin/org/connectbot/sshlib/client/sftp/SftpFileAttributes.kt create mode 100644 sshlib/src/main/kotlin/org/connectbot/sshlib/client/sftp/SftpPacketIO.kt create mode 100644 sshlib/src/test/kotlin/org/connectbot/sshlib/client/sftp/SftpClientIntegrationTest.kt diff --git a/sshlib/api.txt b/sshlib/api.txt index fd09a04..717055b 100644 --- a/sshlib/api.txt +++ b/sshlib/api.txt @@ -230,6 +230,125 @@ package org.connectbot.sshlib { property public String type; } + public final class SftpAttributes { + ctor public SftpAttributes(); + ctor public SftpAttributes(optional java.lang.Long? size, optional java.lang.Integer? uid, optional java.lang.Integer? gid, optional java.lang.Integer? permissions, optional java.lang.Integer? atime, optional java.lang.Integer? mtime); + method public java.lang.Long? component1(); + method public java.lang.Integer? component2(); + method public java.lang.Integer? component3(); + method public java.lang.Integer? component4(); + method public java.lang.Integer? component5(); + method public java.lang.Integer? component6(); + method public org.connectbot.sshlib.SftpAttributes copy(optional java.lang.Long? size, optional java.lang.Integer? uid, optional java.lang.Integer? gid, optional java.lang.Integer? permissions, optional java.lang.Integer? atime, optional java.lang.Integer? mtime); + method public boolean equals(java.lang.Object? other); + method @InaccessibleFromKotlin public java.lang.Integer? getAtime(); + method @InaccessibleFromKotlin public java.lang.Integer? getGid(); + method @InaccessibleFromKotlin public java.lang.Integer? getMtime(); + method @InaccessibleFromKotlin public java.lang.Integer? getPermissions(); + method @InaccessibleFromKotlin public java.lang.Long? getSize(); + method @InaccessibleFromKotlin public java.lang.Integer? getUid(); + method public int hashCode(); + method public java.lang.String toString(); + property public Integer? atime; + property public Integer? gid; + property public Integer? mtime; + property public Integer? permissions; + property public Long? size; + property public Integer? uid; + field public static final org.connectbot.sshlib.SftpAttributes.Companion Companion; + } + + public static final class SftpAttributes.Companion { + method @InaccessibleFromKotlin public org.connectbot.sshlib.SftpAttributes getEMPTY(); + property public org.connectbot.sshlib.SftpAttributes EMPTY; + } + + public interface SftpClient { + method public void close(); + method public suspend java.lang.Object? close(org.connectbot.sshlib.SftpFileHandle handle, kotlin.coroutines.Continuation); + method public suspend java.lang.Object? fsetstat(org.connectbot.sshlib.SftpFileHandle handle, org.connectbot.sshlib.SftpAttributes attrs, kotlin.coroutines.Continuation); + method public suspend java.lang.Object? fstat(org.connectbot.sshlib.SftpFileHandle handle, kotlin.coroutines.Continuation); + method @InaccessibleFromKotlin public int getProtocolVersion(); + method @InaccessibleFromKotlin public boolean isOpen(); + method public default suspend java.lang.Object? listdir(java.lang.String path, kotlin.coroutines.Continuation>); + method public suspend java.lang.Object? lstat(java.lang.String path, kotlin.coroutines.Continuation); + method public suspend java.lang.Object? mkdir(java.lang.String path, optional org.connectbot.sshlib.SftpAttributes attrs, kotlin.coroutines.Continuation); + method public suspend java.lang.Object? open(java.lang.String path, java.util.Set flags, optional org.connectbot.sshlib.SftpAttributes attrs, kotlin.coroutines.Continuation); + method public suspend java.lang.Object? opendir(java.lang.String path, kotlin.coroutines.Continuation); + method public suspend java.lang.Object? read(org.connectbot.sshlib.SftpFileHandle handle, long offset, int length, kotlin.coroutines.Continuation); + method public suspend java.lang.Object? readdir(org.connectbot.sshlib.SftpFileHandle handle, kotlin.coroutines.Continuation?>); + method public suspend java.lang.Object? readlink(java.lang.String path, kotlin.coroutines.Continuation); + method public suspend java.lang.Object? realpath(java.lang.String path, kotlin.coroutines.Continuation); + method public suspend java.lang.Object? remove(java.lang.String path, kotlin.coroutines.Continuation); + method public suspend java.lang.Object? rename(java.lang.String oldPath, java.lang.String newPath, kotlin.coroutines.Continuation); + method public suspend java.lang.Object? rmdir(java.lang.String path, kotlin.coroutines.Continuation); + method public suspend java.lang.Object? setstat(java.lang.String path, org.connectbot.sshlib.SftpAttributes attrs, kotlin.coroutines.Continuation); + method public suspend java.lang.Object? stat(java.lang.String path, kotlin.coroutines.Continuation); + method public suspend java.lang.Object? symlink(java.lang.String targetPath, java.lang.String linkPath, kotlin.coroutines.Continuation); + method public suspend java.lang.Object? write(org.connectbot.sshlib.SftpFileHandle handle, long offset, byte[] data, kotlin.coroutines.Continuation); + property public abstract boolean isOpen; + property public abstract int protocolVersion; + } + + public final class SftpDirectoryEntry { + ctor public SftpDirectoryEntry(java.lang.String filename, java.lang.String longname, org.connectbot.sshlib.SftpAttributes attrs); + method public java.lang.String component1(); + method public java.lang.String component2(); + method public org.connectbot.sshlib.SftpAttributes component3(); + method public org.connectbot.sshlib.SftpDirectoryEntry copy(optional java.lang.String filename, optional java.lang.String longname, optional org.connectbot.sshlib.SftpAttributes attrs); + method public boolean equals(java.lang.Object? other); + method @InaccessibleFromKotlin public org.connectbot.sshlib.SftpAttributes getAttrs(); + method @InaccessibleFromKotlin public java.lang.String getFilename(); + method @InaccessibleFromKotlin public java.lang.String getLongname(); + method public int hashCode(); + method public java.lang.String toString(); + property public org.connectbot.sshlib.SftpAttributes attrs; + property public String filename; + property public String longname; + } + + public final class SftpException extends org.connectbot.sshlib.SshException { + ctor public SftpException(org.connectbot.sshlib.SftpStatusCode statusCode, java.lang.String message, optional java.lang.Throwable? cause); + method @InaccessibleFromKotlin public org.connectbot.sshlib.SftpStatusCode getStatusCode(); + method public java.lang.String toString(); + property public org.connectbot.sshlib.SftpStatusCode statusCode; + } + + public final class SftpFileHandle { + method public boolean equals(java.lang.Object? other); + method public int hashCode(); + } + + public enum SftpOpenFlag { + method @InaccessibleFromKotlin public int getValue(); + property public int value; + enum_constant public static final org.connectbot.sshlib.SftpOpenFlag APPEND; + enum_constant public static final org.connectbot.sshlib.SftpOpenFlag CREATE; + enum_constant public static final org.connectbot.sshlib.SftpOpenFlag EXCLUDE; + enum_constant public static final org.connectbot.sshlib.SftpOpenFlag READ; + enum_constant public static final org.connectbot.sshlib.SftpOpenFlag TRUNCATE; + enum_constant public static final org.connectbot.sshlib.SftpOpenFlag WRITE; + } + + public enum SftpStatusCode { + method @InaccessibleFromKotlin public int getCode(); + property public int code; + enum_constant public static final org.connectbot.sshlib.SftpStatusCode BAD_MESSAGE; + enum_constant public static final org.connectbot.sshlib.SftpStatusCode CONNECTION_LOST; + enum_constant public static final org.connectbot.sshlib.SftpStatusCode EOF; + enum_constant public static final org.connectbot.sshlib.SftpStatusCode FAILURE; + enum_constant public static final org.connectbot.sshlib.SftpStatusCode NO_CONNECTION; + enum_constant public static final org.connectbot.sshlib.SftpStatusCode NO_SUCH_FILE; + enum_constant public static final org.connectbot.sshlib.SftpStatusCode OK; + enum_constant public static final org.connectbot.sshlib.SftpStatusCode OP_UNSUPPORTED; + enum_constant public static final org.connectbot.sshlib.SftpStatusCode PERMISSION_DENIED; + field public static final org.connectbot.sshlib.SftpStatusCode.Companion Companion; + } + + public static final class SftpStatusCode.Companion { + method public org.connectbot.sshlib.SftpStatusCode fromCode(int code); + } + public interface Socks5Authenticator { method public boolean authenticate(java.lang.String username, java.lang.String password); } @@ -254,6 +373,7 @@ package org.connectbot.sshlib { method public suspend java.lang.Object? localPortForward(int bindPort, java.lang.String remoteHost, int remotePort, kotlin.coroutines.Continuation); method public org.connectbot.sshlib.transport.TransportFactory? openDirectTcpipTransport(java.lang.String remoteHost, int remotePort, optional java.lang.String originAddr, optional int originPort); method public suspend java.lang.Object? openSession(kotlin.coroutines.Continuation); + method public suspend java.lang.Object? openSftp(kotlin.coroutines.Continuation); method public suspend java.lang.Object? remotePortForward(java.lang.String remoteBindAddress, int remoteBindPort, java.lang.String localHost, int localPort, kotlin.coroutines.Continuation); property public kotlinx.coroutines.flow.SharedFlow disconnectedFlow; property public boolean isAuthenticated; @@ -419,6 +539,7 @@ package org.connectbot.sshlib.blocking { method public org.connectbot.sshlib.transport.TransportFactory? openDirectTcpipTransport(java.lang.String remoteHost, int remotePort, optional java.lang.String originAddr); method public org.connectbot.sshlib.transport.TransportFactory? openDirectTcpipTransport(java.lang.String remoteHost, int remotePort, optional java.lang.String originAddr, optional int originPort); method public org.connectbot.sshlib.SshSession? openSession(); + method public org.connectbot.sshlib.SftpClient? openSftp(); method public org.connectbot.sshlib.PortForwarder? remotePortForward(java.lang.String remoteBindAddress, int remoteBindPort, java.lang.String localHost, int localPort); property public kotlinx.coroutines.flow.SharedFlow disconnectedFlow; property public boolean isAuthenticated; diff --git a/sshlib/src/main/kotlin/org/connectbot/sshlib/SftpClient.kt b/sshlib/src/main/kotlin/org/connectbot/sshlib/SftpClient.kt new file mode 100644 index 0000000..d30dcbb --- /dev/null +++ b/sshlib/src/main/kotlin/org/connectbot/sshlib/SftpClient.kt @@ -0,0 +1,144 @@ +/* + * Copyright 2025 Kenny Root + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.connectbot.sshlib + +/** + * SFTP client for file transfer over SSH (draft-ietf-secsh-filexfer). + * + * Obtain an instance via [SshClient.openSftp]. All methods are suspend functions + * for use with Kotlin coroutines. Multiple concurrent operations are supported + * via SFTP request pipelining. + * + * Usage: + * ```kotlin + * val sftp = client.openSftp() ?: error("Failed to open SFTP") + * try { + * val entries = sftp.listdir("/home/user") + * entries.forEach { println(it.filename) } + * } finally { + * sftp.close() + * } + * ``` + */ +interface SftpClient : AutoCloseable { + /** The negotiated SFTP protocol version (typically 3). */ + val protocolVersion: Int + + /** Whether this SFTP session is still open. */ + val isOpen: Boolean + + // --- File I/O --- + + /** + * Open a file. Returns a handle for subsequent read/write/close operations. + * + * @throws SftpException on server error (e.g. NO_SUCH_FILE, PERMISSION_DENIED) + */ + suspend fun open( + path: String, + flags: Set, + attrs: SftpAttributes = SftpAttributes.EMPTY, + ): SftpFileHandle + + /** Close a file or directory handle. */ + suspend fun close(handle: SftpFileHandle) + + /** + * Read data from an open file at the given offset. + * + * @return File data, or null if at EOF + */ + suspend fun read(handle: SftpFileHandle, offset: Long, length: Int): ByteArray? + + /** Write data to an open file at the given offset. */ + suspend fun write(handle: SftpFileHandle, offset: Long, data: ByteArray) + + // --- Stat operations --- + + /** Get file attributes, following symlinks. */ + suspend fun stat(path: String): SftpAttributes + + /** Get file attributes without following symlinks. */ + suspend fun lstat(path: String): SftpAttributes + + /** Get attributes of an open file handle. */ + suspend fun fstat(handle: SftpFileHandle): SftpAttributes + + /** Set file attributes by path. */ + suspend fun setstat(path: String, attrs: SftpAttributes) + + /** Set attributes of an open file handle. */ + suspend fun fsetstat(handle: SftpFileHandle, attrs: SftpAttributes) + + // --- Directory operations --- + + /** Open a directory for reading. */ + suspend fun opendir(path: String): SftpFileHandle + + /** + * Read the next batch of directory entries. + * + * @return List of entries, or null if end of directory reached + */ + suspend fun readdir(handle: SftpFileHandle): List? + + /** + * List all entries in a directory. Convenience method that handles + * opendir/readdir/close internally. + */ + suspend fun listdir(path: String): List { + val handle = opendir(path) + try { + val entries = mutableListOf() + while (true) { + val batch = readdir(handle) ?: break + entries.addAll(batch) + } + return entries + } finally { + close(handle) + } + } + + /** Create a directory. */ + suspend fun mkdir(path: String, attrs: SftpAttributes = SftpAttributes.EMPTY) + + /** Remove an empty directory. */ + suspend fun rmdir(path: String) + + // --- File management --- + + /** Delete a file. */ + suspend fun remove(path: String) + + /** Rename or move a file. */ + suspend fun rename(oldPath: String, newPath: String) + + // --- Path operations --- + + /** Resolve a path to its canonical absolute form. */ + suspend fun realpath(path: String): String + + /** Read the target of a symbolic link. */ + suspend fun readlink(path: String): String + + /** Create a symbolic link. */ + suspend fun symlink(targetPath: String, linkPath: String) + + /** Close this SFTP session and the underlying SSH channel. */ + override fun close() +} diff --git a/sshlib/src/main/kotlin/org/connectbot/sshlib/SftpException.kt b/sshlib/src/main/kotlin/org/connectbot/sshlib/SftpException.kt new file mode 100644 index 0000000..a7f0f67 --- /dev/null +++ b/sshlib/src/main/kotlin/org/connectbot/sshlib/SftpException.kt @@ -0,0 +1,31 @@ +/* + * Copyright 2025 Kenny Root + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.connectbot.sshlib + +/** + * Exception thrown for SFTP protocol errors. + * + * @param statusCode The SFTP status code from the server + * @param message Human-readable error message + */ +class SftpException( + val statusCode: SftpStatusCode, + message: String, + cause: Throwable? = null, +) : SshException(message, cause) { + override fun toString(): String = "SftpException(${statusCode.name}): $message" +} diff --git a/sshlib/src/main/kotlin/org/connectbot/sshlib/SftpTypes.kt b/sshlib/src/main/kotlin/org/connectbot/sshlib/SftpTypes.kt new file mode 100644 index 0000000..4b2ea81 --- /dev/null +++ b/sshlib/src/main/kotlin/org/connectbot/sshlib/SftpTypes.kt @@ -0,0 +1,92 @@ +/* + * Copyright 2025 Kenny Root + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.connectbot.sshlib + +/** + * Opaque file or directory handle returned by SFTP open/opendir operations. + * + * Handles are server-assigned and should be closed when no longer needed. + */ +class SftpFileHandle internal constructor(internal val handle: ByteArray) { + override fun equals(other: Any?): Boolean = + other is SftpFileHandle && handle.contentEquals(other.handle) + + override fun hashCode(): Int = handle.contentHashCode() +} + +/** + * File attributes for SFTP operations (SFTPv3 ATTRS structure). + * + * All fields are optional — only fields with non-null values are transmitted. + */ +data class SftpAttributes( + val size: Long? = null, + val uid: Int? = null, + val gid: Int? = null, + val permissions: Int? = null, + val atime: Int? = null, + val mtime: Int? = null, +) { + companion object { + val EMPTY = SftpAttributes() + } +} + +/** + * Entry from an SFTP directory listing. + * + * @param filename Short filename (e.g. "file.txt") + * @param longname Long-format listing (e.g. "-rw-r--r-- 1 user group 1234 Jan 1 00:00 file.txt") + * @param attrs File attributes + */ +data class SftpDirectoryEntry( + val filename: String, + val longname: String, + val attrs: SftpAttributes, +) + +/** + * Flags for SFTP file open operations. + */ +enum class SftpOpenFlag(val value: Int) { + READ(0x00000001), + WRITE(0x00000002), + APPEND(0x00000004), + CREATE(0x00000008), + TRUNCATE(0x00000010), + EXCLUDE(0x00000020), +} + +/** + * SFTP status codes (draft-ietf-secsh-filexfer-02 section 7). + */ +enum class SftpStatusCode(val code: Int) { + OK(0), + EOF(1), + NO_SUCH_FILE(2), + PERMISSION_DENIED(3), + FAILURE(4), + BAD_MESSAGE(5), + NO_CONNECTION(6), + CONNECTION_LOST(7), + OP_UNSUPPORTED(8); + + companion object { + fun fromCode(code: Int): SftpStatusCode = + entries.firstOrNull { it.code == code } ?: FAILURE + } +} diff --git a/sshlib/src/main/kotlin/org/connectbot/sshlib/SshClient.kt b/sshlib/src/main/kotlin/org/connectbot/sshlib/SshClient.kt index e7b435a..c4079c1 100644 --- a/sshlib/src/main/kotlin/org/connectbot/sshlib/SshClient.kt +++ b/sshlib/src/main/kotlin/org/connectbot/sshlib/SshClient.kt @@ -401,6 +401,36 @@ class SshClient private constructor( } } + /** + * Open an SFTP session for file transfer. + * + * Opens a new session channel, starts the "sftp" subsystem, and performs + * SFTP version negotiation. + * + * @return SftpClient instance, or null if not authenticated or connection fails + */ + suspend fun openSftp(): SftpClient? { + val conn = connection + if (conn == null || !authenticated) { + logger.error("Not authenticated - call connect() and authenticate first") + return null + } + + return try { + logger.info("Opening SFTP session") + val session = conn.openSessionChannel() + ?: throw SshException("Failed to open session channel for SFTP") + if (!session.requestSubsystem("sftp")) { + session.close() + throw SshException("Server rejected SFTP subsystem request") + } + org.connectbot.sshlib.client.sftp.SftpClientImpl.create(session) + } catch (e: Exception) { + logger.error("Failed to open SFTP session", e) + null + } + } + /** * Start local port forwarding (RFC 4254 section 7.2). * diff --git a/sshlib/src/main/kotlin/org/connectbot/sshlib/blocking/BlockingSshClient.kt b/sshlib/src/main/kotlin/org/connectbot/sshlib/blocking/BlockingSshClient.kt index 3a7e011..3221ebb 100644 --- a/sshlib/src/main/kotlin/org/connectbot/sshlib/blocking/BlockingSshClient.kt +++ b/sshlib/src/main/kotlin/org/connectbot/sshlib/blocking/BlockingSshClient.kt @@ -27,6 +27,7 @@ import org.connectbot.sshlib.ConnectResult import org.connectbot.sshlib.HostKeyVerifier import org.connectbot.sshlib.KeyboardInteractiveCallback import org.connectbot.sshlib.PortForwarder +import org.connectbot.sshlib.SftpClient import org.connectbot.sshlib.Socks5Authenticator import org.connectbot.sshlib.SshClient import org.connectbot.sshlib.SshClientConfig @@ -238,6 +239,11 @@ class BlockingSshClient internal constructor( */ fun openSession(): SshSession? = runBlocking { client.openSession() } + /** + * Open an SFTP session for file transfer (blocking wrapper). + */ + fun openSftp(): SftpClient? = runBlocking { client.openSftp() } + /** * Start local port forwarding. */ diff --git a/sshlib/src/main/kotlin/org/connectbot/sshlib/client/sftp/SftpClientImpl.kt b/sshlib/src/main/kotlin/org/connectbot/sshlib/client/sftp/SftpClientImpl.kt new file mode 100644 index 0000000..750ac76 --- /dev/null +++ b/sshlib/src/main/kotlin/org/connectbot/sshlib/client/sftp/SftpClientImpl.kt @@ -0,0 +1,437 @@ +/* + * Copyright 2025 Kenny Root + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.connectbot.sshlib.client.sftp + +import kotlinx.coroutines.CoroutineScope +import kotlinx.coroutines.Dispatchers +import kotlinx.coroutines.Job +import kotlinx.coroutines.SupervisorJob +import org.connectbot.sshlib.SftpAttributes +import org.connectbot.sshlib.SftpClient +import org.connectbot.sshlib.SftpDirectoryEntry +import org.connectbot.sshlib.SftpException +import org.connectbot.sshlib.SftpFileHandle +import org.connectbot.sshlib.SftpOpenFlag +import org.connectbot.sshlib.SftpStatusCode +import org.connectbot.sshlib.SshSession +import org.slf4j.LoggerFactory +import java.nio.ByteBuffer +import java.nio.charset.StandardCharsets + +/** + * Internal implementation of [SftpClient]. + * + * SFTP message types (draft-ietf-secsh-filexfer-02 section 3): + */ +internal class SftpClientImpl private constructor( + private val session: SshSession, + private val dispatcher: SftpDispatcher, + private val readJob: Job, + override val protocolVersion: Int, +) : SftpClient { + + private val scope = CoroutineScope(Dispatchers.IO + SupervisorJob()) + private var closed = false + + override val isOpen: Boolean get() = !closed && session.isOpen + + // --- File I/O --- + + override suspend fun open(path: String, flags: Set, attrs: SftpAttributes): SftpFileHandle { + val pflags = flags.fold(0) { acc, flag -> acc or flag.value } + val pathBytes = path.toByteArray(StandardCharsets.UTF_8) + val attrsBytes = SftpFileAttributes.encode(attrs) + + val payload = ByteBuffer.allocate(4 + pathBytes.size + 4 + attrsBytes.size) + putString(payload, pathBytes) + payload.putInt(pflags) + payload.put(attrsBytes) + + val response = dispatcher.request(SSH_FXP_OPEN, payload.array()) + return when (response.type) { + SSH_FXP_HANDLE -> SftpFileHandle(extractString(ByteBuffer.wrap(response.payload))) + SSH_FXP_STATUS -> throw decodeStatusException(response.payload) + else -> throw SftpProtocolException("Unexpected response type ${response.type} for OPEN") + } + } + + override suspend fun close(handle: SftpFileHandle) { + val payload = ByteBuffer.allocate(4 + handle.handle.size) + putString(payload, handle.handle) + + val response = dispatcher.request(SSH_FXP_CLOSE, payload.array()) + if (response.type == SSH_FXP_STATUS) { + val status = decodeStatus(response.payload) + if (status != SftpStatusCode.OK) throw decodeStatusException(response.payload) + } + } + + override suspend fun read(handle: SftpFileHandle, offset: Long, length: Int): ByteArray? { + val payload = ByteBuffer.allocate(4 + handle.handle.size + 8 + 4) + putString(payload, handle.handle) + payload.putLong(offset) + payload.putInt(length) + + val response = dispatcher.request(SSH_FXP_READ, payload.array()) + return when (response.type) { + SSH_FXP_DATA -> extractString(ByteBuffer.wrap(response.payload)) + SSH_FXP_STATUS -> { + val status = decodeStatus(response.payload) + if (status == SftpStatusCode.EOF) null + else throw decodeStatusException(response.payload) + } + else -> throw SftpProtocolException("Unexpected response type ${response.type} for READ") + } + } + + override suspend fun write(handle: SftpFileHandle, offset: Long, data: ByteArray) { + val payload = ByteBuffer.allocate(4 + handle.handle.size + 8 + 4 + data.size) + putString(payload, handle.handle) + payload.putLong(offset) + putString(payload, data) + + val response = dispatcher.request(SSH_FXP_WRITE, payload.array()) + if (response.type == SSH_FXP_STATUS) { + val status = decodeStatus(response.payload) + if (status != SftpStatusCode.OK) throw decodeStatusException(response.payload) + } + } + + // --- Stat operations --- + + override suspend fun stat(path: String): SftpAttributes { + return statRequest(SSH_FXP_STAT, path) + } + + override suspend fun lstat(path: String): SftpAttributes { + return statRequest(SSH_FXP_LSTAT, path) + } + + private suspend fun statRequest(type: Int, path: String): SftpAttributes { + val pathBytes = path.toByteArray(StandardCharsets.UTF_8) + val payload = ByteBuffer.allocate(4 + pathBytes.size) + putString(payload, pathBytes) + + val response = dispatcher.request(type, payload.array()) + return when (response.type) { + SSH_FXP_ATTRS -> SftpFileAttributes.decode(ByteBuffer.wrap(response.payload)) + SSH_FXP_STATUS -> throw decodeStatusException(response.payload) + else -> throw SftpProtocolException("Unexpected response type ${response.type} for STAT") + } + } + + override suspend fun fstat(handle: SftpFileHandle): SftpAttributes { + val payload = ByteBuffer.allocate(4 + handle.handle.size) + putString(payload, handle.handle) + + val response = dispatcher.request(SSH_FXP_FSTAT, payload.array()) + return when (response.type) { + SSH_FXP_ATTRS -> SftpFileAttributes.decode(ByteBuffer.wrap(response.payload)) + SSH_FXP_STATUS -> throw decodeStatusException(response.payload) + else -> throw SftpProtocolException("Unexpected response type ${response.type} for FSTAT") + } + } + + override suspend fun setstat(path: String, attrs: SftpAttributes) { + val pathBytes = path.toByteArray(StandardCharsets.UTF_8) + val attrsBytes = SftpFileAttributes.encode(attrs) + val payload = ByteBuffer.allocate(4 + pathBytes.size + attrsBytes.size) + putString(payload, pathBytes) + payload.put(attrsBytes) + + val response = dispatcher.request(SSH_FXP_SETSTAT, payload.array()) + if (response.type == SSH_FXP_STATUS) { + val status = decodeStatus(response.payload) + if (status != SftpStatusCode.OK) throw decodeStatusException(response.payload) + } + } + + override suspend fun fsetstat(handle: SftpFileHandle, attrs: SftpAttributes) { + val attrsBytes = SftpFileAttributes.encode(attrs) + val payload = ByteBuffer.allocate(4 + handle.handle.size + attrsBytes.size) + putString(payload, handle.handle) + payload.put(attrsBytes) + + val response = dispatcher.request(SSH_FXP_FSETSTAT, payload.array()) + if (response.type == SSH_FXP_STATUS) { + val status = decodeStatus(response.payload) + if (status != SftpStatusCode.OK) throw decodeStatusException(response.payload) + } + } + + // --- Directory operations --- + + override suspend fun opendir(path: String): SftpFileHandle { + val pathBytes = path.toByteArray(StandardCharsets.UTF_8) + val payload = ByteBuffer.allocate(4 + pathBytes.size) + putString(payload, pathBytes) + + val response = dispatcher.request(SSH_FXP_OPENDIR, payload.array()) + return when (response.type) { + SSH_FXP_HANDLE -> SftpFileHandle(extractString(ByteBuffer.wrap(response.payload))) + SSH_FXP_STATUS -> throw decodeStatusException(response.payload) + else -> throw SftpProtocolException("Unexpected response type ${response.type} for OPENDIR") + } + } + + override suspend fun readdir(handle: SftpFileHandle): List? { + val payload = ByteBuffer.allocate(4 + handle.handle.size) + putString(payload, handle.handle) + + val response = dispatcher.request(SSH_FXP_READDIR, payload.array()) + return when (response.type) { + SSH_FXP_NAME -> decodeName(response.payload) + SSH_FXP_STATUS -> { + val status = decodeStatus(response.payload) + if (status == SftpStatusCode.EOF) null + else throw decodeStatusException(response.payload) + } + else -> throw SftpProtocolException("Unexpected response type ${response.type} for READDIR") + } + } + + override suspend fun mkdir(path: String, attrs: SftpAttributes) { + val pathBytes = path.toByteArray(StandardCharsets.UTF_8) + val attrsBytes = SftpFileAttributes.encode(attrs) + val payload = ByteBuffer.allocate(4 + pathBytes.size + attrsBytes.size) + putString(payload, pathBytes) + payload.put(attrsBytes) + + val response = dispatcher.request(SSH_FXP_MKDIR, payload.array()) + if (response.type == SSH_FXP_STATUS) { + val status = decodeStatus(response.payload) + if (status != SftpStatusCode.OK) throw decodeStatusException(response.payload) + } + } + + override suspend fun rmdir(path: String) { + simplePathRequest(SSH_FXP_RMDIR, path) + } + + // --- File management --- + + override suspend fun remove(path: String) { + simplePathRequest(SSH_FXP_REMOVE, path) + } + + override suspend fun rename(oldPath: String, newPath: String) { + val oldBytes = oldPath.toByteArray(StandardCharsets.UTF_8) + val newBytes = newPath.toByteArray(StandardCharsets.UTF_8) + val payload = ByteBuffer.allocate(4 + oldBytes.size + 4 + newBytes.size) + putString(payload, oldBytes) + putString(payload, newBytes) + + val response = dispatcher.request(SSH_FXP_RENAME, payload.array()) + if (response.type == SSH_FXP_STATUS) { + val status = decodeStatus(response.payload) + if (status != SftpStatusCode.OK) throw decodeStatusException(response.payload) + } + } + + // --- Path operations --- + + override suspend fun realpath(path: String): String { + val pathBytes = path.toByteArray(StandardCharsets.UTF_8) + val payload = ByteBuffer.allocate(4 + pathBytes.size) + putString(payload, pathBytes) + + val response = dispatcher.request(SSH_FXP_REALPATH, payload.array()) + return when (response.type) { + SSH_FXP_NAME -> { + val entries = decodeName(response.payload) + entries.firstOrNull()?.filename + ?: throw SftpProtocolException("REALPATH returned empty NAME") + } + SSH_FXP_STATUS -> throw decodeStatusException(response.payload) + else -> throw SftpProtocolException("Unexpected response type ${response.type} for REALPATH") + } + } + + override suspend fun readlink(path: String): String { + val pathBytes = path.toByteArray(StandardCharsets.UTF_8) + val payload = ByteBuffer.allocate(4 + pathBytes.size) + putString(payload, pathBytes) + + val response = dispatcher.request(SSH_FXP_READLINK, payload.array()) + return when (response.type) { + SSH_FXP_NAME -> { + val entries = decodeName(response.payload) + entries.firstOrNull()?.filename + ?: throw SftpProtocolException("READLINK returned empty NAME") + } + SSH_FXP_STATUS -> throw decodeStatusException(response.payload) + else -> throw SftpProtocolException("Unexpected response type ${response.type} for READLINK") + } + } + + override suspend fun symlink(targetPath: String, linkPath: String) { + // Note: OpenSSH has a known bug where symlink arguments are reversed + // from the spec. The spec says (targetpath, linkpath) but OpenSSH + // expects (linkpath, targetpath). We follow the OpenSSH convention + // since it's the most common server implementation. + val linkBytes = linkPath.toByteArray(StandardCharsets.UTF_8) + val targetBytes = targetPath.toByteArray(StandardCharsets.UTF_8) + val payload = ByteBuffer.allocate(4 + linkBytes.size + 4 + targetBytes.size) + putString(payload, linkBytes) + putString(payload, targetBytes) + + val response = dispatcher.request(SSH_FXP_SYMLINK, payload.array()) + if (response.type == SSH_FXP_STATUS) { + val status = decodeStatus(response.payload) + if (status != SftpStatusCode.OK) throw decodeStatusException(response.payload) + } + } + + override fun close() { + if (closed) return + closed = true + dispatcher.stop() + session.close() + } + + // --- Helpers --- + + private suspend fun simplePathRequest(type: Int, path: String) { + val pathBytes = path.toByteArray(StandardCharsets.UTF_8) + val payload = ByteBuffer.allocate(4 + pathBytes.size) + putString(payload, pathBytes) + + val response = dispatcher.request(type, payload.array()) + if (response.type == SSH_FXP_STATUS) { + val status = decodeStatus(response.payload) + if (status != SftpStatusCode.OK) throw decodeStatusException(response.payload) + } + } + + companion object { + private val logger = LoggerFactory.getLogger(SftpClientImpl::class.java) + + // SFTP message types + private const val SSH_FXP_INIT = 1 + private const val SSH_FXP_VERSION = 2 + private const val SSH_FXP_OPEN = 3 + private const val SSH_FXP_CLOSE = 4 + private const val SSH_FXP_READ = 5 + private const val SSH_FXP_WRITE = 6 + private const val SSH_FXP_LSTAT = 7 + private const val SSH_FXP_FSTAT = 8 + private const val SSH_FXP_SETSTAT = 9 + private const val SSH_FXP_FSETSTAT = 10 + private const val SSH_FXP_OPENDIR = 11 + private const val SSH_FXP_READDIR = 12 + private const val SSH_FXP_REMOVE = 13 + private const val SSH_FXP_MKDIR = 14 + private const val SSH_FXP_RMDIR = 15 + private const val SSH_FXP_REALPATH = 16 + private const val SSH_FXP_STAT = 17 + private const val SSH_FXP_RENAME = 18 + private const val SSH_FXP_READLINK = 19 + private const val SSH_FXP_SYMLINK = 20 + + private const val SSH_FXP_STATUS = 101 + private const val SSH_FXP_HANDLE = 102 + private const val SSH_FXP_DATA = 103 + private const val SSH_FXP_NAME = 104 + private const val SSH_FXP_ATTRS = 105 + + private const val SFTP_VERSION = 3 + + /** + * Create an SFTP client by performing the INIT/VERSION handshake. + */ + suspend fun create(session: SshSession): SftpClient { + val packetIO = SftpPacketIO(session) + val dispatcher = SftpDispatcher(packetIO) + + // Send SSH_FXP_INIT + val initPayload = ByteBuffer.allocate(4) + initPayload.putInt(SFTP_VERSION) + dispatcher.writeRaw(SSH_FXP_INIT, initPayload.array()) + + // Read SSH_FXP_VERSION + val versionPacket = dispatcher.readRaw() + if (versionPacket.type != SSH_FXP_VERSION) { + throw SftpProtocolException( + "Expected SSH_FXP_VERSION (2), got ${versionPacket.type}" + ) + } + if (versionPacket.payload.size < 4) { + throw SftpProtocolException("SSH_FXP_VERSION payload too short") + } + val serverVersion = ByteBuffer.wrap(versionPacket.payload, 0, 4).int + val negotiatedVersion = minOf(SFTP_VERSION, serverVersion) + logger.info("SFTP version negotiated: {} (server: {})", negotiatedVersion, serverVersion) + + // Start the background read loop + val scope = CoroutineScope(Dispatchers.IO + SupervisorJob()) + val readJob = dispatcher.startReadLoop(scope) + + return SftpClientImpl(session, dispatcher, readJob, negotiatedVersion) + } + + // --- Wire format helpers --- + + /** Write a length-prefixed string/byte array to a ByteBuffer. */ + private fun putString(buf: ByteBuffer, data: ByteArray) { + buf.putInt(data.size) + buf.put(data) + } + + /** Read a length-prefixed string/byte array from a ByteBuffer. */ + private fun extractString(buf: ByteBuffer): ByteArray { + val len = buf.int + val data = ByteArray(len) + buf.get(data) + return data + } + + /** Decode a STATUS response to get the status code. */ + private fun decodeStatus(payload: ByteArray): SftpStatusCode { + if (payload.size < 4) return SftpStatusCode.FAILURE + val code = ByteBuffer.wrap(payload, 0, 4).int + return SftpStatusCode.fromCode(code) + } + + /** Decode a STATUS response into an SftpException. */ + private fun decodeStatusException(payload: ByteArray): SftpException { + val buf = ByteBuffer.wrap(payload) + val code = if (buf.remaining() >= 4) buf.int else 4 + val statusCode = SftpStatusCode.fromCode(code) + val message = if (buf.remaining() >= 4) { + val msgBytes = extractString(buf) + String(msgBytes, StandardCharsets.UTF_8) + } else { + statusCode.name + } + return SftpException(statusCode, message) + } + + /** Decode a NAME response (used by readdir, realpath, readlink). */ + private fun decodeName(payload: ByteArray): List { + val buf = ByteBuffer.wrap(payload) + val count = buf.int + val entries = mutableListOf() + repeat(count) { + val filename = String(extractString(buf), StandardCharsets.UTF_8) + val longname = String(extractString(buf), StandardCharsets.UTF_8) + val attrs = SftpFileAttributes.decode(buf) + entries.add(SftpDirectoryEntry(filename, longname, attrs)) + } + return entries + } + } +} diff --git a/sshlib/src/main/kotlin/org/connectbot/sshlib/client/sftp/SftpDispatcher.kt b/sshlib/src/main/kotlin/org/connectbot/sshlib/client/sftp/SftpDispatcher.kt new file mode 100644 index 0000000..142ddd4 --- /dev/null +++ b/sshlib/src/main/kotlin/org/connectbot/sshlib/client/sftp/SftpDispatcher.kt @@ -0,0 +1,142 @@ +/* + * Copyright 2025 Kenny Root + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.connectbot.sshlib.client.sftp + +import kotlinx.coroutines.CompletableDeferred +import kotlinx.coroutines.CoroutineScope +import kotlinx.coroutines.Job +import kotlinx.coroutines.launch +import kotlinx.coroutines.sync.Mutex +import kotlinx.coroutines.sync.withLock +import kotlinx.coroutines.withTimeout +import org.slf4j.LoggerFactory +import java.nio.ByteBuffer +import java.util.concurrent.ConcurrentHashMap +import java.util.concurrent.atomic.AtomicInteger + +/** + * Dispatches SFTP requests and routes responses back to waiting coroutines. + * + * Each outbound request gets a unique request ID. A background read loop + * parses incoming SFTP packets and completes the matching deferred. + */ +internal class SftpDispatcher(private val packetIO: SftpPacketIO) { + private val nextRequestId = AtomicInteger(1) + private val pending = ConcurrentHashMap>() + private val writeMutex = Mutex() + private var readJob: Job? = null + + /** + * Send an SFTP request and wait for the matching response. + * + * @param type SFTP message type + * @param payload Request payload (without request ID — it will be prepended) + * @param timeoutMs Maximum time to wait for a response + * @return The raw response packet + */ + suspend fun request(type: Int, payload: ByteArray, timeoutMs: Long = 30_000L): SftpRawPacket { + val requestId = nextRequestId.getAndIncrement() + val deferred = CompletableDeferred() + pending[requestId] = deferred + + try { + // Prepend request ID to payload + val fullPayload = ByteBuffer.allocate(4 + payload.size) + fullPayload.putInt(requestId) + fullPayload.put(payload) + + writeMutex.withLock { + packetIO.writePacket(type, fullPayload.array()) + } + + return withTimeout(timeoutMs) { + deferred.await() + } + } catch (e: Exception) { + pending.remove(requestId) + throw e + } + } + + /** + * Send an SFTP packet without a request ID (used for INIT). + */ + suspend fun writeRaw(type: Int, payload: ByteArray) { + writeMutex.withLock { + packetIO.writePacket(type, payload) + } + } + + /** + * Read a single raw packet (used for VERSION response during init). + */ + suspend fun readRaw(): SftpRawPacket = packetIO.readPacket() + + /** + * Start the background read loop that routes responses to waiting callers. + */ + fun startReadLoop(scope: CoroutineScope): Job { + val job = scope.launch { + try { + while (true) { + val packet = packetIO.readPacket() + + // Extract request ID from first 4 bytes of payload + if (packet.payload.size < 4) { + logger.warn("SFTP packet type {} with payload too short for request ID", packet.type) + continue + } + + val requestId = ByteBuffer.wrap(packet.payload, 0, 4).int + val responsePayload = packet.payload.copyOfRange(4, packet.payload.size) + val responsePacket = SftpRawPacket(packet.type, responsePayload) + + val deferred = pending.remove(requestId) + if (deferred != null) { + deferred.complete(responsePacket) + } else { + logger.warn("SFTP response for unknown request ID {}", requestId) + } + } + } catch (e: SftpProtocolException) { + logger.debug("SFTP channel closed: {}", e.message) + // Complete all pending requests with the error + val error = e + pending.values.forEach { it.completeExceptionally(error) } + pending.clear() + } catch (e: Exception) { + logger.debug("SFTP read loop ended: {}", e.message) + pending.values.forEach { it.completeExceptionally(e) } + pending.clear() + } + } + readJob = job + return job + } + + fun stop() { + readJob?.cancel() + pending.values.forEach { + it.completeExceptionally(SftpProtocolException("SFTP session closed")) + } + pending.clear() + } + + companion object { + private val logger = LoggerFactory.getLogger(SftpDispatcher::class.java) + } +} diff --git a/sshlib/src/main/kotlin/org/connectbot/sshlib/client/sftp/SftpFileAttributes.kt b/sshlib/src/main/kotlin/org/connectbot/sshlib/client/sftp/SftpFileAttributes.kt new file mode 100644 index 0000000..683f3c2 --- /dev/null +++ b/sshlib/src/main/kotlin/org/connectbot/sshlib/client/sftp/SftpFileAttributes.kt @@ -0,0 +1,107 @@ +/* + * Copyright 2025 Kenny Root + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.connectbot.sshlib.client.sftp + +import org.connectbot.sshlib.SftpAttributes +import java.nio.ByteBuffer + +/** + * Internal parser/serializer for SFTPv3 ATTRS structure. + * + * The ATTRS structure uses a flags field to indicate which subsequent + * fields are present (draft-ietf-secsh-filexfer-02 section 5). + * + * Flags: + * - SSH_FILEXFER_ATTR_SIZE (0x00000001): size is present + * - SSH_FILEXFER_ATTR_UIDGID (0x00000002): uid and gid are present + * - SSH_FILEXFER_ATTR_PERMISSIONS(0x00000004): permissions is present + * - SSH_FILEXFER_ATTR_ACMODTIME (0x00000008): atime and mtime are present + * - SSH_FILEXFER_ATTR_EXTENDED (0x80000000): extended attributes present + */ +internal object SftpFileAttributes { + private const val SSH_FILEXFER_ATTR_SIZE = 0x00000001 + private const val SSH_FILEXFER_ATTR_UIDGID = 0x00000002 + private const val SSH_FILEXFER_ATTR_PERMISSIONS = 0x00000004 + private const val SSH_FILEXFER_ATTR_ACMODTIME = 0x00000008 + private const val SSH_FILEXFER_ATTR_EXTENDED = 0x80000000.toInt() + + /** + * Parse ATTRS from a ByteBuffer at its current position. + */ + fun decode(buf: ByteBuffer): SftpAttributes { + val flags = buf.int + val size = if (flags and SSH_FILEXFER_ATTR_SIZE != 0) buf.long else null + val uid = if (flags and SSH_FILEXFER_ATTR_UIDGID != 0) buf.int else null + val gid = if (flags and SSH_FILEXFER_ATTR_UIDGID != 0) buf.int else null + val permissions = if (flags and SSH_FILEXFER_ATTR_PERMISSIONS != 0) buf.int else null + val atime = if (flags and SSH_FILEXFER_ATTR_ACMODTIME != 0) buf.int else null + val mtime = if (flags and SSH_FILEXFER_ATTR_ACMODTIME != 0) buf.int else null + + // Skip extended attributes if present + if (flags and SSH_FILEXFER_ATTR_EXTENDED != 0) { + val extCount = buf.int + repeat(extCount) { + val typeLen = buf.int + buf.position(buf.position() + typeLen) // skip type string + val dataLen = buf.int + buf.position(buf.position() + dataLen) // skip data string + } + } + + return SftpAttributes( + size = size, + uid = uid, + gid = gid, + permissions = permissions, + atime = atime, + mtime = mtime, + ) + } + + /** + * Encode ATTRS to a byte array. + */ + fun encode(attrs: SftpAttributes): ByteArray { + var flags = 0 + if (attrs.size != null) flags = flags or SSH_FILEXFER_ATTR_SIZE + if (attrs.uid != null || attrs.gid != null) flags = flags or SSH_FILEXFER_ATTR_UIDGID + if (attrs.permissions != null) flags = flags or SSH_FILEXFER_ATTR_PERMISSIONS + if (attrs.atime != null || attrs.mtime != null) flags = flags or SSH_FILEXFER_ATTR_ACMODTIME + + // Calculate size + var size = 4 // flags + if (flags and SSH_FILEXFER_ATTR_SIZE != 0) size += 8 + if (flags and SSH_FILEXFER_ATTR_UIDGID != 0) size += 8 + if (flags and SSH_FILEXFER_ATTR_PERMISSIONS != 0) size += 4 + if (flags and SSH_FILEXFER_ATTR_ACMODTIME != 0) size += 8 + + val buf = ByteBuffer.allocate(size) + buf.putInt(flags) + if (attrs.size != null) buf.putLong(attrs.size) + if (flags and SSH_FILEXFER_ATTR_UIDGID != 0) { + buf.putInt(attrs.uid ?: 0) + buf.putInt(attrs.gid ?: 0) + } + if (attrs.permissions != null) buf.putInt(attrs.permissions) + if (flags and SSH_FILEXFER_ATTR_ACMODTIME != 0) { + buf.putInt(attrs.atime ?: 0) + buf.putInt(attrs.mtime ?: 0) + } + + return buf.array() + } +} diff --git a/sshlib/src/main/kotlin/org/connectbot/sshlib/client/sftp/SftpPacketIO.kt b/sshlib/src/main/kotlin/org/connectbot/sshlib/client/sftp/SftpPacketIO.kt new file mode 100644 index 0000000..69a53e8 --- /dev/null +++ b/sshlib/src/main/kotlin/org/connectbot/sshlib/client/sftp/SftpPacketIO.kt @@ -0,0 +1,125 @@ +/* + * Copyright 2025 Kenny Root + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.connectbot.sshlib.client.sftp + +import org.connectbot.sshlib.SshSession +import java.io.ByteArrayOutputStream +import java.nio.ByteBuffer + +/** + * SFTP packet framing over an SSH session channel. + * + * SFTP packets are length-prefixed: `uint32 length + byte type + payload`. + * The length field counts everything after itself (type + payload). + * + * SSH channel data arrives in arbitrary chunks that may not align with SFTP + * packet boundaries. This class accumulates bytes until a complete packet + * is available. + */ +internal class SftpPacketIO(private val session: SshSession) { + private val buffer = ByteArrayOutputStream() + private var bufferedBytes = ByteArray(0) + private var bufferedOffset = 0 + private var bufferedLength = 0 + + /** + * Read a complete SFTP packet. Blocks (suspends) until enough data arrives. + * + * @return Parsed SFTP packet (type + payload, without the length prefix) + * @throws SftpProtocolException if the channel closes before a complete packet + */ + suspend fun readPacket(): SftpRawPacket { + // Read the 4-byte length prefix + val lengthBytes = readExact(4) + val length = ByteBuffer.wrap(lengthBytes).int + if (length < 1 || length > MAX_PACKET_SIZE) { + throw SftpProtocolException("Invalid SFTP packet length: $length") + } + + // Read the packet body (type + payload) + val body = readExact(length) + val type = body[0].toInt() and 0xFF + val payload = body.copyOfRange(1, body.size) + return SftpRawPacket(type, payload) + } + + /** + * Write an SFTP packet with the given type and payload. + */ + suspend fun writePacket(type: Int, payload: ByteArray) { + val length = 1 + payload.size // type byte + payload + val packet = ByteBuffer.allocate(4 + length) + packet.putInt(length) + packet.put(type.toByte()) + packet.put(payload) + session.write(packet.array()) + } + + /** + * Read exactly [count] bytes from the session, accumulating across + * multiple channel data chunks as needed. + */ + private suspend fun readExact(count: Int): ByteArray { + val result = ByteArray(count) + var filled = 0 + + // Drain any leftover buffered data first + if (bufferedLength > 0) { + val toCopy = minOf(count, bufferedLength) + System.arraycopy(bufferedBytes, bufferedOffset, result, 0, toCopy) + bufferedOffset += toCopy + bufferedLength -= toCopy + filled += toCopy + } + + // Read from the session until we have enough + while (filled < count) { + val data = session.read() + ?: throw SftpProtocolException("SSH channel closed before complete SFTP packet") + + val toCopy = minOf(count - filled, data.size) + System.arraycopy(data, 0, result, filled, toCopy) + filled += toCopy + + // Buffer any leftover bytes for the next readExact call + if (toCopy < data.size) { + bufferedBytes = data + bufferedOffset = toCopy + bufferedLength = data.size - toCopy + } + } + + return result + } + + companion object { + /** Maximum SFTP packet size (256KB — generous limit). */ + private const val MAX_PACKET_SIZE = 256 * 1024 + } +} + +/** + * Raw SFTP packet with type byte and payload (without the length prefix). + */ +internal data class SftpRawPacket(val type: Int, val payload: ByteArray) { + override fun equals(other: Any?): Boolean = + other is SftpRawPacket && type == other.type && payload.contentEquals(other.payload) + + override fun hashCode(): Int = 31 * type + payload.contentHashCode() +} + +internal class SftpProtocolException(message: String) : Exception(message) diff --git a/sshlib/src/test/kotlin/org/connectbot/sshlib/client/sftp/SftpClientIntegrationTest.kt b/sshlib/src/test/kotlin/org/connectbot/sshlib/client/sftp/SftpClientIntegrationTest.kt new file mode 100644 index 0000000..edda4c8 --- /dev/null +++ b/sshlib/src/test/kotlin/org/connectbot/sshlib/client/sftp/SftpClientIntegrationTest.kt @@ -0,0 +1,331 @@ +/* + * Copyright 2025 Kenny Root + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.connectbot.sshlib.client.sftp + +import kotlinx.coroutines.runBlocking +import org.connectbot.sshlib.HostKeyVerifier +import org.connectbot.sshlib.PublicKey +import org.connectbot.sshlib.SftpAttributes +import org.connectbot.sshlib.SftpClient +import org.connectbot.sshlib.SftpException +import org.connectbot.sshlib.SftpOpenFlag +import org.connectbot.sshlib.SftpStatusCode +import org.connectbot.sshlib.SshClient +import org.connectbot.sshlib.SshClientConfig +import org.junit.jupiter.api.Assertions.assertEquals +import org.junit.jupiter.api.Assertions.assertNotNull +import org.junit.jupiter.api.Assertions.assertNull +import org.junit.jupiter.api.Assertions.assertTrue +import org.junit.jupiter.api.Assertions.fail +import org.junit.jupiter.api.Test +import org.slf4j.LoggerFactory +import org.testcontainers.containers.GenericContainer +import org.testcontainers.containers.output.Slf4jLogConsumer +import org.testcontainers.containers.wait.strategy.Wait +import org.testcontainers.images.builder.ImageFromDockerfile +import org.testcontainers.junit.jupiter.Container +import org.testcontainers.junit.jupiter.Testcontainers + +/** + * Integration tests for SFTP client against a real OpenSSH server. + */ +@Testcontainers +class SftpClientIntegrationTest { + + companion object { + private val logger = LoggerFactory.getLogger(SftpClientIntegrationTest::class.java) + private val logConsumer = Slf4jLogConsumer(logger).withPrefix("DOCKER") + + private const val USERNAME = "testuser" + private const val PASSWORD = "testpass" + private const val OPENSSH_VERSION = "V_9_9_P2" + private const val DEBUG_CFLAGS = "" + + @Container + @JvmStatic + val opensshContainer: GenericContainer<*> = GenericContainer( + ImageFromDockerfile("openssh-sftp-test", false) + .withFileFromClasspath(".", "openssh-server") + .withBuildArg("OPENSSH_VERSION", OPENSSH_VERSION) + .withBuildArg("DEBUG_CFLAGS", DEBUG_CFLAGS) + ) + .withExposedPorts(22) + .withLogConsumer(logConsumer) + .waitingFor( + Wait.forLogMessage(".*Server listening.*", 1) + ) + } + + private val acceptAllVerifier = object : HostKeyVerifier { + override suspend fun verify(key: PublicKey): Boolean = true + } + + private suspend fun openSftp(): Pair { + val host = opensshContainer.host + val port = opensshContainer.getMappedPort(22) + + val config = SshClientConfig { + this.host = host + this.port = port + this.hostKeyVerifier = acceptAllVerifier + } + val client = SshClient(config) + + assertTrue(client.connect(), "Should connect to SSH server") + assertTrue(client.authenticatePassword(USERNAME, PASSWORD), "Should authenticate") + + val sftp = client.openSftp() + assertNotNull(sftp, "Should open SFTP session") + return Pair(client, sftp!!) + } + + @Test + fun `should negotiate SFTP version 3`() = runBlocking { + val (client, sftp) = openSftp() + try { + assertTrue(sftp.protocolVersion >= 3, "SFTP version should be >= 3") + assertTrue(sftp.isOpen, "SFTP session should be open") + } finally { + sftp.close() + client.disconnect() + } + } + + @Test + fun `should stat root directory`() = runBlocking { + val (client, sftp) = openSftp() + try { + val attrs = sftp.stat("/") + assertNotNull(attrs.permissions, "Root dir should have permissions") + } finally { + sftp.close() + client.disconnect() + } + } + + @Test + fun `should resolve realpath`() = runBlocking { + val (client, sftp) = openSftp() + try { + val path = sftp.realpath(".") + assertTrue(path.startsWith("/"), "Realpath should return absolute path: $path") + } finally { + sftp.close() + client.disconnect() + } + } + + @Test + fun `should create write read and delete file`() = runBlocking { + val (client, sftp) = openSftp() + try { + val testPath = "/tmp/sftp-test-${System.currentTimeMillis()}.txt" + val testData = "Hello SFTP!\n".toByteArray() + + // Create and write + val handle = sftp.open( + testPath, + setOf(SftpOpenFlag.WRITE, SftpOpenFlag.CREATE, SftpOpenFlag.TRUNCATE), + ) + sftp.write(handle, 0, testData) + sftp.close(handle) + + // Read back + val readHandle = sftp.open(testPath, setOf(SftpOpenFlag.READ)) + val readData = sftp.read(readHandle, 0, testData.size) + sftp.close(readHandle) + + assertNotNull(readData) + assertTrue(testData.contentEquals(readData!!), "Read data should match written data") + + // Stat + val attrs = sftp.stat(testPath) + assertEquals(testData.size.toLong(), attrs.size, "File size should match") + + // Delete + sftp.remove(testPath) + + // Verify deleted + try { + sftp.stat(testPath) + fail("stat should fail after delete") + } catch (e: SftpException) { + assertEquals(SftpStatusCode.NO_SUCH_FILE, e.statusCode) + } + } finally { + sftp.close() + client.disconnect() + } + } + + @Test + fun `should list directory`() = runBlocking { + val (client, sftp) = openSftp() + try { + val entries = sftp.listdir("/tmp") + assertTrue(entries.isNotEmpty(), "Directory listing should not be empty") + // /tmp always has . and .. + assertTrue(entries.any { it.filename == "." }, "Should contain '.'") + assertTrue(entries.any { it.filename == ".." }, "Should contain '..'") + } finally { + sftp.close() + client.disconnect() + } + } + + @Test + fun `should create and remove directory`() = runBlocking { + val (client, sftp) = openSftp() + try { + val dirPath = "/tmp/sftp-dir-${System.currentTimeMillis()}" + + sftp.mkdir(dirPath) + + val attrs = sftp.stat(dirPath) + assertNotNull(attrs.permissions) + + sftp.rmdir(dirPath) + + try { + sftp.stat(dirPath) + fail("stat should fail after rmdir") + } catch (e: SftpException) { + assertEquals(SftpStatusCode.NO_SUCH_FILE, e.statusCode) + } + } finally { + sftp.close() + client.disconnect() + } + } + + @Test + fun `should rename a file`() = runBlocking { + val (client, sftp) = openSftp() + try { + val ts = System.currentTimeMillis() + val oldPath = "/tmp/sftp-old-$ts.txt" + val newPath = "/tmp/sftp-new-$ts.txt" + + // Create file + val handle = sftp.open(oldPath, setOf(SftpOpenFlag.WRITE, SftpOpenFlag.CREATE)) + sftp.write(handle, 0, "rename test".toByteArray()) + sftp.close(handle) + + // Rename + sftp.rename(oldPath, newPath) + + // Old path should not exist + try { + sftp.stat(oldPath) + fail("Old path should not exist after rename") + } catch (e: SftpException) { + assertEquals(SftpStatusCode.NO_SUCH_FILE, e.statusCode) + } + + // New path should exist + sftp.stat(newPath) + + // Cleanup + sftp.remove(newPath) + } finally { + sftp.close() + client.disconnect() + } + } + + @Test + fun `should handle file not found`() = runBlocking { + val (client, sftp) = openSftp() + try { + try { + sftp.stat("/nonexistent/path/that/does/not/exist") + fail("Should throw SftpException for nonexistent path") + } catch (e: SftpException) { + assertEquals(SftpStatusCode.NO_SUCH_FILE, e.statusCode) + } + } finally { + sftp.close() + client.disconnect() + } + } + + @Test + fun `should handle large file transfer`() = runBlocking { + val (client, sftp) = openSftp() + try { + val testPath = "/tmp/sftp-large-${System.currentTimeMillis()}.bin" + // 64KB — exercises SSH channel window adjustment + val testData = ByteArray(64 * 1024) { (it % 256).toByte() } + + val writeHandle = sftp.open( + testPath, + setOf(SftpOpenFlag.WRITE, SftpOpenFlag.CREATE, SftpOpenFlag.TRUNCATE), + ) + sftp.write(writeHandle, 0, testData) + sftp.close(writeHandle) + + // Read back in chunks + val readHandle = sftp.open(testPath, setOf(SftpOpenFlag.READ)) + val readBuffer = mutableListOf() + var offset = 0L + while (true) { + val chunk = sftp.read(readHandle, offset, 16384) ?: break + readBuffer.addAll(chunk.toList()) + offset += chunk.size + } + sftp.close(readHandle) + + assertEquals(testData.size, readBuffer.size, "Should read back all bytes") + assertTrue( + testData.contentEquals(readBuffer.toByteArray()), + "Read data should match written data", + ) + + sftp.remove(testPath) + } finally { + sftp.close() + client.disconnect() + } + } + + @Test + fun `should set file attributes`() = runBlocking { + val (client, sftp) = openSftp() + try { + val testPath = "/tmp/sftp-attrs-${System.currentTimeMillis()}.txt" + + val handle = sftp.open(testPath, setOf(SftpOpenFlag.WRITE, SftpOpenFlag.CREATE)) + sftp.close(handle) + + // Set permissions to 0644 + sftp.setstat(testPath, SftpAttributes(permissions = 0b110_100_100)) // 0644 + + val attrs = sftp.stat(testPath) + // Check lower 9 bits (rwx for user/group/other) + assertEquals( + 0b110_100_100, + (attrs.permissions ?: 0) and 0x1FF, + "Permissions should be 0644", + ) + + sftp.remove(testPath) + } finally { + sftp.close() + client.disconnect() + } + } +} From d3e2353267fb8ed3114efa317a82d7c69b69752e Mon Sep 17 00:00:00 2001 From: GlassOnTin Date: Sun, 5 Apr 2026 12:48:20 +0100 Subject: [PATCH 02/10] Fix spotless formatting --- .../kotlin/org/connectbot/sshlib/SftpTypes.kt | 9 +++--- .../sshlib/client/sftp/SftpClientImpl.kt | 30 ++++++++++++------- .../sshlib/client/sftp/SftpFileAttributes.kt | 2 +- .../sshlib/client/sftp/SftpPacketIO.kt | 3 +- .../client/sftp/SftpClientIntegrationTest.kt | 8 ++--- 5 files changed, 30 insertions(+), 22 deletions(-) diff --git a/sshlib/src/main/kotlin/org/connectbot/sshlib/SftpTypes.kt b/sshlib/src/main/kotlin/org/connectbot/sshlib/SftpTypes.kt index 4b2ea81..369cab9 100644 --- a/sshlib/src/main/kotlin/org/connectbot/sshlib/SftpTypes.kt +++ b/sshlib/src/main/kotlin/org/connectbot/sshlib/SftpTypes.kt @@ -22,8 +22,7 @@ package org.connectbot.sshlib * Handles are server-assigned and should be closed when no longer needed. */ class SftpFileHandle internal constructor(internal val handle: ByteArray) { - override fun equals(other: Any?): Boolean = - other is SftpFileHandle && handle.contentEquals(other.handle) + override fun equals(other: Any?): Boolean = other is SftpFileHandle && handle.contentEquals(other.handle) override fun hashCode(): Int = handle.contentHashCode() } @@ -83,10 +82,10 @@ enum class SftpStatusCode(val code: Int) { BAD_MESSAGE(5), NO_CONNECTION(6), CONNECTION_LOST(7), - OP_UNSUPPORTED(8); + OP_UNSUPPORTED(8), + ; companion object { - fun fromCode(code: Int): SftpStatusCode = - entries.firstOrNull { it.code == code } ?: FAILURE + fun fromCode(code: Int): SftpStatusCode = entries.firstOrNull { it.code == code } ?: FAILURE } } diff --git a/sshlib/src/main/kotlin/org/connectbot/sshlib/client/sftp/SftpClientImpl.kt b/sshlib/src/main/kotlin/org/connectbot/sshlib/client/sftp/SftpClientImpl.kt index 750ac76..aa4e53e 100644 --- a/sshlib/src/main/kotlin/org/connectbot/sshlib/client/sftp/SftpClientImpl.kt +++ b/sshlib/src/main/kotlin/org/connectbot/sshlib/client/sftp/SftpClientImpl.kt @@ -89,11 +89,16 @@ internal class SftpClientImpl private constructor( val response = dispatcher.request(SSH_FXP_READ, payload.array()) return when (response.type) { SSH_FXP_DATA -> extractString(ByteBuffer.wrap(response.payload)) + SSH_FXP_STATUS -> { val status = decodeStatus(response.payload) - if (status == SftpStatusCode.EOF) null - else throw decodeStatusException(response.payload) + if (status == SftpStatusCode.EOF) { + null + } else { + throw decodeStatusException(response.payload) + } } + else -> throw SftpProtocolException("Unexpected response type ${response.type} for READ") } } @@ -113,13 +118,9 @@ internal class SftpClientImpl private constructor( // --- Stat operations --- - override suspend fun stat(path: String): SftpAttributes { - return statRequest(SSH_FXP_STAT, path) - } + override suspend fun stat(path: String): SftpAttributes = statRequest(SSH_FXP_STAT, path) - override suspend fun lstat(path: String): SftpAttributes { - return statRequest(SSH_FXP_LSTAT, path) - } + override suspend fun lstat(path: String): SftpAttributes = statRequest(SSH_FXP_LSTAT, path) private suspend fun statRequest(type: Int, path: String): SftpAttributes { val pathBytes = path.toByteArray(StandardCharsets.UTF_8) @@ -195,11 +196,16 @@ internal class SftpClientImpl private constructor( val response = dispatcher.request(SSH_FXP_READDIR, payload.array()) return when (response.type) { SSH_FXP_NAME -> decodeName(response.payload) + SSH_FXP_STATUS -> { val status = decodeStatus(response.payload) - if (status == SftpStatusCode.EOF) null - else throw decodeStatusException(response.payload) + if (status == SftpStatusCode.EOF) { + null + } else { + throw decodeStatusException(response.payload) + } } + else -> throw SftpProtocolException("Unexpected response type ${response.type} for READDIR") } } @@ -256,7 +262,9 @@ internal class SftpClientImpl private constructor( entries.firstOrNull()?.filename ?: throw SftpProtocolException("REALPATH returned empty NAME") } + SSH_FXP_STATUS -> throw decodeStatusException(response.payload) + else -> throw SftpProtocolException("Unexpected response type ${response.type} for REALPATH") } } @@ -273,7 +281,9 @@ internal class SftpClientImpl private constructor( entries.firstOrNull()?.filename ?: throw SftpProtocolException("READLINK returned empty NAME") } + SSH_FXP_STATUS -> throw decodeStatusException(response.payload) + else -> throw SftpProtocolException("Unexpected response type ${response.type} for READLINK") } } diff --git a/sshlib/src/main/kotlin/org/connectbot/sshlib/client/sftp/SftpFileAttributes.kt b/sshlib/src/main/kotlin/org/connectbot/sshlib/client/sftp/SftpFileAttributes.kt index 683f3c2..c113988 100644 --- a/sshlib/src/main/kotlin/org/connectbot/sshlib/client/sftp/SftpFileAttributes.kt +++ b/sshlib/src/main/kotlin/org/connectbot/sshlib/client/sftp/SftpFileAttributes.kt @@ -68,7 +68,7 @@ internal object SftpFileAttributes { gid = gid, permissions = permissions, atime = atime, - mtime = mtime, + mtime = mtime ) } diff --git a/sshlib/src/main/kotlin/org/connectbot/sshlib/client/sftp/SftpPacketIO.kt b/sshlib/src/main/kotlin/org/connectbot/sshlib/client/sftp/SftpPacketIO.kt index 69a53e8..528f93f 100644 --- a/sshlib/src/main/kotlin/org/connectbot/sshlib/client/sftp/SftpPacketIO.kt +++ b/sshlib/src/main/kotlin/org/connectbot/sshlib/client/sftp/SftpPacketIO.kt @@ -116,8 +116,7 @@ internal class SftpPacketIO(private val session: SshSession) { * Raw SFTP packet with type byte and payload (without the length prefix). */ internal data class SftpRawPacket(val type: Int, val payload: ByteArray) { - override fun equals(other: Any?): Boolean = - other is SftpRawPacket && type == other.type && payload.contentEquals(other.payload) + override fun equals(other: Any?): Boolean = other is SftpRawPacket && type == other.type && payload.contentEquals(other.payload) override fun hashCode(): Int = 31 * type + payload.contentHashCode() } diff --git a/sshlib/src/test/kotlin/org/connectbot/sshlib/client/sftp/SftpClientIntegrationTest.kt b/sshlib/src/test/kotlin/org/connectbot/sshlib/client/sftp/SftpClientIntegrationTest.kt index edda4c8..98d6266 100644 --- a/sshlib/src/test/kotlin/org/connectbot/sshlib/client/sftp/SftpClientIntegrationTest.kt +++ b/sshlib/src/test/kotlin/org/connectbot/sshlib/client/sftp/SftpClientIntegrationTest.kt @@ -139,7 +139,7 @@ class SftpClientIntegrationTest { // Create and write val handle = sftp.open( testPath, - setOf(SftpOpenFlag.WRITE, SftpOpenFlag.CREATE, SftpOpenFlag.TRUNCATE), + setOf(SftpOpenFlag.WRITE, SftpOpenFlag.CREATE, SftpOpenFlag.TRUNCATE) ) sftp.write(handle, 0, testData) sftp.close(handle) @@ -273,7 +273,7 @@ class SftpClientIntegrationTest { val writeHandle = sftp.open( testPath, - setOf(SftpOpenFlag.WRITE, SftpOpenFlag.CREATE, SftpOpenFlag.TRUNCATE), + setOf(SftpOpenFlag.WRITE, SftpOpenFlag.CREATE, SftpOpenFlag.TRUNCATE) ) sftp.write(writeHandle, 0, testData) sftp.close(writeHandle) @@ -292,7 +292,7 @@ class SftpClientIntegrationTest { assertEquals(testData.size, readBuffer.size, "Should read back all bytes") assertTrue( testData.contentEquals(readBuffer.toByteArray()), - "Read data should match written data", + "Read data should match written data" ) sftp.remove(testPath) @@ -319,7 +319,7 @@ class SftpClientIntegrationTest { assertEquals( 0b110_100_100, (attrs.permissions ?: 0) and 0x1FF, - "Permissions should be 0644", + "Permissions should be 0644" ) sftp.remove(testPath) From 0536bc6d7e132e6471323f23bf22ba2363ff295b Mon Sep 17 00:00:00 2001 From: GlassOnTin Date: Mon, 6 Apr 2026 10:34:51 +0100 Subject: [PATCH 03/10] Refactor SFTP API to use sealed SftpResult instead of exceptions Replace thrown SftpException/SftpProtocolException with a sealed SftpResult interface (Success, ServerError, ProtocolError, IoError). All SftpClient methods now return SftpResult, making error handling structural rather than exception-based. - SftpResult.getOrThrow() provided for blocking API interop - SftpResult.getOrNull() for optional-style access - SftpClientImpl wraps dispatcher exceptions as IoError/ProtocolError - Updated integration tests to use getOrThrow() and check ServerError - Fixed fully qualified SftpClientImpl reference in SshClient (now imported) --- .../org/connectbot/sshlib/SftpClient.kt | 82 +++--- .../org/connectbot/sshlib/SftpResult.kt | 52 ++++ .../kotlin/org/connectbot/sshlib/SshClient.kt | 3 +- .../sshlib/client/sftp/SftpClientImpl.kt | 266 +++++++++--------- .../client/sftp/SftpClientIntegrationTest.kt | 108 +++---- 5 files changed, 282 insertions(+), 229 deletions(-) create mode 100644 sshlib/src/main/kotlin/org/connectbot/sshlib/SftpResult.kt diff --git a/sshlib/src/main/kotlin/org/connectbot/sshlib/SftpClient.kt b/sshlib/src/main/kotlin/org/connectbot/sshlib/SftpClient.kt index d30dcbb..9f5a880 100644 --- a/sshlib/src/main/kotlin/org/connectbot/sshlib/SftpClient.kt +++ b/sshlib/src/main/kotlin/org/connectbot/sshlib/SftpClient.kt @@ -23,12 +23,19 @@ package org.connectbot.sshlib * for use with Kotlin coroutines. Multiple concurrent operations are supported * via SFTP request pipelining. * + * All operations return [SftpResult] instead of throwing exceptions, so errors + * can be handled structurally. Use [getOrNull] or [getOrThrow] for convenience. + * * Usage: * ```kotlin * val sftp = client.openSftp() ?: error("Failed to open SFTP") * try { - * val entries = sftp.listdir("/home/user") - * entries.forEach { println(it.filename) } + * when (val result = sftp.listdir("/home/user")) { + * is SftpResult.Success -> result.value.forEach { println(it.filename) } + * is SftpResult.ServerError -> println("Error: ${result.message}") + * is SftpResult.ProtocolError -> println("Protocol error: ${result.message}") + * is SftpResult.IoError -> println("I/O error: ${result.cause}") + * } * } finally { * sftp.close() * } @@ -43,101 +50,108 @@ interface SftpClient : AutoCloseable { // --- File I/O --- - /** - * Open a file. Returns a handle for subsequent read/write/close operations. - * - * @throws SftpException on server error (e.g. NO_SUCH_FILE, PERMISSION_DENIED) - */ + /** Open a file. Returns a handle for subsequent read/write/close operations. */ suspend fun open( path: String, flags: Set, attrs: SftpAttributes = SftpAttributes.EMPTY, - ): SftpFileHandle + ): SftpResult /** Close a file or directory handle. */ - suspend fun close(handle: SftpFileHandle) + suspend fun close(handle: SftpFileHandle): SftpResult /** * Read data from an open file at the given offset. - * - * @return File data, or null if at EOF + * Returns [SftpResult.Success] with data, or with null at EOF. */ - suspend fun read(handle: SftpFileHandle, offset: Long, length: Int): ByteArray? + suspend fun read(handle: SftpFileHandle, offset: Long, length: Int): SftpResult /** Write data to an open file at the given offset. */ - suspend fun write(handle: SftpFileHandle, offset: Long, data: ByteArray) + suspend fun write(handle: SftpFileHandle, offset: Long, data: ByteArray): SftpResult // --- Stat operations --- /** Get file attributes, following symlinks. */ - suspend fun stat(path: String): SftpAttributes + suspend fun stat(path: String): SftpResult /** Get file attributes without following symlinks. */ - suspend fun lstat(path: String): SftpAttributes + suspend fun lstat(path: String): SftpResult /** Get attributes of an open file handle. */ - suspend fun fstat(handle: SftpFileHandle): SftpAttributes + suspend fun fstat(handle: SftpFileHandle): SftpResult /** Set file attributes by path. */ - suspend fun setstat(path: String, attrs: SftpAttributes) + suspend fun setstat(path: String, attrs: SftpAttributes): SftpResult /** Set attributes of an open file handle. */ - suspend fun fsetstat(handle: SftpFileHandle, attrs: SftpAttributes) + suspend fun fsetstat(handle: SftpFileHandle, attrs: SftpAttributes): SftpResult // --- Directory operations --- /** Open a directory for reading. */ - suspend fun opendir(path: String): SftpFileHandle + suspend fun opendir(path: String): SftpResult /** * Read the next batch of directory entries. - * - * @return List of entries, or null if end of directory reached + * Returns [SftpResult.Success] with entries, or with null at end of directory. */ - suspend fun readdir(handle: SftpFileHandle): List? + suspend fun readdir(handle: SftpFileHandle): SftpResult?> /** * List all entries in a directory. Convenience method that handles * opendir/readdir/close internally. */ - suspend fun listdir(path: String): List { - val handle = opendir(path) + suspend fun listdir(path: String): SftpResult> { + val handleResult = opendir(path) + val handle = when (handleResult) { + is SftpResult.Success -> handleResult.value + is SftpResult.ServerError -> return handleResult + is SftpResult.ProtocolError -> return handleResult + is SftpResult.IoError -> return handleResult + } try { val entries = mutableListOf() while (true) { - val batch = readdir(handle) ?: break - entries.addAll(batch) + when (val batch = readdir(handle)) { + is SftpResult.Success -> { + if (batch.value == null) break + entries.addAll(batch.value) + } + is SftpResult.ServerError -> return batch + is SftpResult.ProtocolError -> return batch + is SftpResult.IoError -> return batch + } } - return entries + return SftpResult.Success(entries) } finally { close(handle) } } /** Create a directory. */ - suspend fun mkdir(path: String, attrs: SftpAttributes = SftpAttributes.EMPTY) + suspend fun mkdir(path: String, attrs: SftpAttributes = SftpAttributes.EMPTY): SftpResult /** Remove an empty directory. */ - suspend fun rmdir(path: String) + suspend fun rmdir(path: String): SftpResult // --- File management --- /** Delete a file. */ - suspend fun remove(path: String) + suspend fun remove(path: String): SftpResult /** Rename or move a file. */ - suspend fun rename(oldPath: String, newPath: String) + suspend fun rename(oldPath: String, newPath: String): SftpResult // --- Path operations --- /** Resolve a path to its canonical absolute form. */ - suspend fun realpath(path: String): String + suspend fun realpath(path: String): SftpResult /** Read the target of a symbolic link. */ - suspend fun readlink(path: String): String + suspend fun readlink(path: String): SftpResult /** Create a symbolic link. */ - suspend fun symlink(targetPath: String, linkPath: String) + suspend fun symlink(targetPath: String, linkPath: String): SftpResult /** Close this SFTP session and the underlying SSH channel. */ override fun close() diff --git a/sshlib/src/main/kotlin/org/connectbot/sshlib/SftpResult.kt b/sshlib/src/main/kotlin/org/connectbot/sshlib/SftpResult.kt new file mode 100644 index 0000000..859de30 --- /dev/null +++ b/sshlib/src/main/kotlin/org/connectbot/sshlib/SftpResult.kt @@ -0,0 +1,52 @@ +/* + * Copyright 2025 Kenny Root + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.connectbot.sshlib + +/** + * Result type for SFTP operations. Replaces thrown exceptions with a + * sealed interface so callers can handle errors structurally. + */ +sealed interface SftpResult { + /** Operation succeeded with [value]. */ + data class Success(val value: T) : SftpResult + + /** SFTP server returned an error status. */ + data class ServerError( + val statusCode: SftpStatusCode, + val message: String, + ) : SftpResult + + /** SFTP protocol violation (unexpected packet type, malformed data). */ + data class ProtocolError(val message: String) : SftpResult + + /** Network or I/O error. */ + data class IoError(val cause: Throwable) : SftpResult +} + +/** Convenience: extract value or null for success, throws nothing. */ +fun SftpResult.getOrNull(): T? = when (this) { + is SftpResult.Success -> value + else -> null +} + +/** Convenience: extract value or throw for interop with blocking APIs. */ +fun SftpResult.getOrThrow(): T = when (this) { + is SftpResult.Success -> value + is SftpResult.ServerError -> throw SftpException(statusCode, message) + is SftpResult.ProtocolError -> throw SftpException(SftpStatusCode.BAD_MESSAGE, message) + is SftpResult.IoError -> throw SftpException(SftpStatusCode.FAILURE, cause.message ?: "I/O error", cause) +} diff --git a/sshlib/src/main/kotlin/org/connectbot/sshlib/SshClient.kt b/sshlib/src/main/kotlin/org/connectbot/sshlib/SshClient.kt index c4079c1..629b6ac 100644 --- a/sshlib/src/main/kotlin/org/connectbot/sshlib/SshClient.kt +++ b/sshlib/src/main/kotlin/org/connectbot/sshlib/SshClient.kt @@ -28,6 +28,7 @@ import kotlinx.coroutines.flow.asSharedFlow import kotlinx.coroutines.launch import org.connectbot.sshlib.client.DynamicPortForwarder import org.connectbot.sshlib.client.LocalPortForwarder +import org.connectbot.sshlib.client.sftp.SftpClientImpl import org.connectbot.sshlib.client.RemotePortForwarder import org.connectbot.sshlib.client.SshConnection import org.connectbot.sshlib.crypto.PrivateKeyReader @@ -424,7 +425,7 @@ class SshClient private constructor( session.close() throw SshException("Server rejected SFTP subsystem request") } - org.connectbot.sshlib.client.sftp.SftpClientImpl.create(session) + SftpClientImpl.create(session) } catch (e: Exception) { logger.error("Failed to open SFTP session", e) null diff --git a/sshlib/src/main/kotlin/org/connectbot/sshlib/client/sftp/SftpClientImpl.kt b/sshlib/src/main/kotlin/org/connectbot/sshlib/client/sftp/SftpClientImpl.kt index aa4e53e..042aa7e 100644 --- a/sshlib/src/main/kotlin/org/connectbot/sshlib/client/sftp/SftpClientImpl.kt +++ b/sshlib/src/main/kotlin/org/connectbot/sshlib/client/sftp/SftpClientImpl.kt @@ -23,9 +23,9 @@ import kotlinx.coroutines.SupervisorJob import org.connectbot.sshlib.SftpAttributes import org.connectbot.sshlib.SftpClient import org.connectbot.sshlib.SftpDirectoryEntry -import org.connectbot.sshlib.SftpException import org.connectbot.sshlib.SftpFileHandle import org.connectbot.sshlib.SftpOpenFlag +import org.connectbot.sshlib.SftpResult import org.connectbot.sshlib.SftpStatusCode import org.connectbot.sshlib.SshSession import org.slf4j.LoggerFactory @@ -51,7 +51,7 @@ internal class SftpClientImpl private constructor( // --- File I/O --- - override suspend fun open(path: String, flags: Set, attrs: SftpAttributes): SftpFileHandle { + override suspend fun open(path: String, flags: Set, attrs: SftpAttributes): SftpResult { val pflags = flags.fold(0) { acc, flag -> acc or flag.value } val pathBytes = path.toByteArray(StandardCharsets.UTF_8) val attrsBytes = SftpFileAttributes.encode(attrs) @@ -61,234 +61,210 @@ internal class SftpClientImpl private constructor( payload.putInt(pflags) payload.put(attrsBytes) - val response = dispatcher.request(SSH_FXP_OPEN, payload.array()) - return when (response.type) { - SSH_FXP_HANDLE -> SftpFileHandle(extractString(ByteBuffer.wrap(response.payload))) - SSH_FXP_STATUS -> throw decodeStatusException(response.payload) - else -> throw SftpProtocolException("Unexpected response type ${response.type} for OPEN") + return dispatchRequest(SSH_FXP_OPEN, payload.array()) { response -> + when (response.type) { + SSH_FXP_HANDLE -> SftpResult.Success(SftpFileHandle(extractString(ByteBuffer.wrap(response.payload)))) + SSH_FXP_STATUS -> decodeStatusError(response.payload) + else -> SftpResult.ProtocolError("Unexpected response type ${response.type} for OPEN") + } } } - override suspend fun close(handle: SftpFileHandle) { + override suspend fun close(handle: SftpFileHandle): SftpResult { val payload = ByteBuffer.allocate(4 + handle.handle.size) putString(payload, handle.handle) - val response = dispatcher.request(SSH_FXP_CLOSE, payload.array()) - if (response.type == SSH_FXP_STATUS) { - val status = decodeStatus(response.payload) - if (status != SftpStatusCode.OK) throw decodeStatusException(response.payload) + return dispatchRequest(SSH_FXP_CLOSE, payload.array()) { response -> + if (response.type == SSH_FXP_STATUS) { + val status = decodeStatus(response.payload) + if (status == SftpStatusCode.OK) SftpResult.Success(Unit) + else decodeStatusError(response.payload) + } else { + SftpResult.Success(Unit) + } } } - override suspend fun read(handle: SftpFileHandle, offset: Long, length: Int): ByteArray? { + override suspend fun read(handle: SftpFileHandle, offset: Long, length: Int): SftpResult { val payload = ByteBuffer.allocate(4 + handle.handle.size + 8 + 4) putString(payload, handle.handle) payload.putLong(offset) payload.putInt(length) - val response = dispatcher.request(SSH_FXP_READ, payload.array()) - return when (response.type) { - SSH_FXP_DATA -> extractString(ByteBuffer.wrap(response.payload)) - - SSH_FXP_STATUS -> { - val status = decodeStatus(response.payload) - if (status == SftpStatusCode.EOF) { - null - } else { - throw decodeStatusException(response.payload) + return dispatchRequest(SSH_FXP_READ, payload.array()) { response -> + when (response.type) { + SSH_FXP_DATA -> SftpResult.Success(extractString(ByteBuffer.wrap(response.payload))) + SSH_FXP_STATUS -> { + val status = decodeStatus(response.payload) + if (status == SftpStatusCode.EOF) SftpResult.Success(null) + else decodeStatusError(response.payload) } + else -> SftpResult.ProtocolError("Unexpected response type ${response.type} for READ") } - - else -> throw SftpProtocolException("Unexpected response type ${response.type} for READ") } } - override suspend fun write(handle: SftpFileHandle, offset: Long, data: ByteArray) { + override suspend fun write(handle: SftpFileHandle, offset: Long, data: ByteArray): SftpResult { val payload = ByteBuffer.allocate(4 + handle.handle.size + 8 + 4 + data.size) putString(payload, handle.handle) payload.putLong(offset) putString(payload, data) - val response = dispatcher.request(SSH_FXP_WRITE, payload.array()) - if (response.type == SSH_FXP_STATUS) { - val status = decodeStatus(response.payload) - if (status != SftpStatusCode.OK) throw decodeStatusException(response.payload) - } + return dispatchStatusRequest(SSH_FXP_WRITE, payload.array()) } // --- Stat operations --- - override suspend fun stat(path: String): SftpAttributes = statRequest(SSH_FXP_STAT, path) + override suspend fun stat(path: String): SftpResult = statRequest(SSH_FXP_STAT, path) - override suspend fun lstat(path: String): SftpAttributes = statRequest(SSH_FXP_LSTAT, path) + override suspend fun lstat(path: String): SftpResult = statRequest(SSH_FXP_LSTAT, path) - private suspend fun statRequest(type: Int, path: String): SftpAttributes { + private suspend fun statRequest(type: Int, path: String): SftpResult { val pathBytes = path.toByteArray(StandardCharsets.UTF_8) val payload = ByteBuffer.allocate(4 + pathBytes.size) putString(payload, pathBytes) - val response = dispatcher.request(type, payload.array()) - return when (response.type) { - SSH_FXP_ATTRS -> SftpFileAttributes.decode(ByteBuffer.wrap(response.payload)) - SSH_FXP_STATUS -> throw decodeStatusException(response.payload) - else -> throw SftpProtocolException("Unexpected response type ${response.type} for STAT") + return dispatchRequest(type, payload.array()) { response -> + when (response.type) { + SSH_FXP_ATTRS -> SftpResult.Success(SftpFileAttributes.decode(ByteBuffer.wrap(response.payload))) + SSH_FXP_STATUS -> decodeStatusError(response.payload) + else -> SftpResult.ProtocolError("Unexpected response type ${response.type} for STAT") + } } } - override suspend fun fstat(handle: SftpFileHandle): SftpAttributes { + override suspend fun fstat(handle: SftpFileHandle): SftpResult { val payload = ByteBuffer.allocate(4 + handle.handle.size) putString(payload, handle.handle) - val response = dispatcher.request(SSH_FXP_FSTAT, payload.array()) - return when (response.type) { - SSH_FXP_ATTRS -> SftpFileAttributes.decode(ByteBuffer.wrap(response.payload)) - SSH_FXP_STATUS -> throw decodeStatusException(response.payload) - else -> throw SftpProtocolException("Unexpected response type ${response.type} for FSTAT") + return dispatchRequest(SSH_FXP_FSTAT, payload.array()) { response -> + when (response.type) { + SSH_FXP_ATTRS -> SftpResult.Success(SftpFileAttributes.decode(ByteBuffer.wrap(response.payload))) + SSH_FXP_STATUS -> decodeStatusError(response.payload) + else -> SftpResult.ProtocolError("Unexpected response type ${response.type} for FSTAT") + } } } - override suspend fun setstat(path: String, attrs: SftpAttributes) { + override suspend fun setstat(path: String, attrs: SftpAttributes): SftpResult { val pathBytes = path.toByteArray(StandardCharsets.UTF_8) val attrsBytes = SftpFileAttributes.encode(attrs) val payload = ByteBuffer.allocate(4 + pathBytes.size + attrsBytes.size) putString(payload, pathBytes) payload.put(attrsBytes) - val response = dispatcher.request(SSH_FXP_SETSTAT, payload.array()) - if (response.type == SSH_FXP_STATUS) { - val status = decodeStatus(response.payload) - if (status != SftpStatusCode.OK) throw decodeStatusException(response.payload) - } + return dispatchStatusRequest(SSH_FXP_SETSTAT, payload.array()) } - override suspend fun fsetstat(handle: SftpFileHandle, attrs: SftpAttributes) { + override suspend fun fsetstat(handle: SftpFileHandle, attrs: SftpAttributes): SftpResult { val attrsBytes = SftpFileAttributes.encode(attrs) val payload = ByteBuffer.allocate(4 + handle.handle.size + attrsBytes.size) putString(payload, handle.handle) payload.put(attrsBytes) - val response = dispatcher.request(SSH_FXP_FSETSTAT, payload.array()) - if (response.type == SSH_FXP_STATUS) { - val status = decodeStatus(response.payload) - if (status != SftpStatusCode.OK) throw decodeStatusException(response.payload) - } + return dispatchStatusRequest(SSH_FXP_FSETSTAT, payload.array()) } // --- Directory operations --- - override suspend fun opendir(path: String): SftpFileHandle { + override suspend fun opendir(path: String): SftpResult { val pathBytes = path.toByteArray(StandardCharsets.UTF_8) val payload = ByteBuffer.allocate(4 + pathBytes.size) putString(payload, pathBytes) - val response = dispatcher.request(SSH_FXP_OPENDIR, payload.array()) - return when (response.type) { - SSH_FXP_HANDLE -> SftpFileHandle(extractString(ByteBuffer.wrap(response.payload))) - SSH_FXP_STATUS -> throw decodeStatusException(response.payload) - else -> throw SftpProtocolException("Unexpected response type ${response.type} for OPENDIR") + return dispatchRequest(SSH_FXP_OPENDIR, payload.array()) { response -> + when (response.type) { + SSH_FXP_HANDLE -> SftpResult.Success(SftpFileHandle(extractString(ByteBuffer.wrap(response.payload)))) + SSH_FXP_STATUS -> decodeStatusError(response.payload) + else -> SftpResult.ProtocolError("Unexpected response type ${response.type} for OPENDIR") + } } } - override suspend fun readdir(handle: SftpFileHandle): List? { + override suspend fun readdir(handle: SftpFileHandle): SftpResult?> { val payload = ByteBuffer.allocate(4 + handle.handle.size) putString(payload, handle.handle) - val response = dispatcher.request(SSH_FXP_READDIR, payload.array()) - return when (response.type) { - SSH_FXP_NAME -> decodeName(response.payload) - - SSH_FXP_STATUS -> { - val status = decodeStatus(response.payload) - if (status == SftpStatusCode.EOF) { - null - } else { - throw decodeStatusException(response.payload) + return dispatchRequest(SSH_FXP_READDIR, payload.array()) { response -> + when (response.type) { + SSH_FXP_NAME -> SftpResult.Success(decodeName(response.payload)) + SSH_FXP_STATUS -> { + val status = decodeStatus(response.payload) + if (status == SftpStatusCode.EOF) SftpResult.Success(null) + else decodeStatusError(response.payload) } + else -> SftpResult.ProtocolError("Unexpected response type ${response.type} for READDIR") } - - else -> throw SftpProtocolException("Unexpected response type ${response.type} for READDIR") } } - override suspend fun mkdir(path: String, attrs: SftpAttributes) { + override suspend fun mkdir(path: String, attrs: SftpAttributes): SftpResult { val pathBytes = path.toByteArray(StandardCharsets.UTF_8) val attrsBytes = SftpFileAttributes.encode(attrs) val payload = ByteBuffer.allocate(4 + pathBytes.size + attrsBytes.size) putString(payload, pathBytes) payload.put(attrsBytes) - val response = dispatcher.request(SSH_FXP_MKDIR, payload.array()) - if (response.type == SSH_FXP_STATUS) { - val status = decodeStatus(response.payload) - if (status != SftpStatusCode.OK) throw decodeStatusException(response.payload) - } + return dispatchStatusRequest(SSH_FXP_MKDIR, payload.array()) } - override suspend fun rmdir(path: String) { - simplePathRequest(SSH_FXP_RMDIR, path) - } + override suspend fun rmdir(path: String): SftpResult = simplePathRequest(SSH_FXP_RMDIR, path) // --- File management --- - override suspend fun remove(path: String) { - simplePathRequest(SSH_FXP_REMOVE, path) - } + override suspend fun remove(path: String): SftpResult = simplePathRequest(SSH_FXP_REMOVE, path) - override suspend fun rename(oldPath: String, newPath: String) { + override suspend fun rename(oldPath: String, newPath: String): SftpResult { val oldBytes = oldPath.toByteArray(StandardCharsets.UTF_8) val newBytes = newPath.toByteArray(StandardCharsets.UTF_8) val payload = ByteBuffer.allocate(4 + oldBytes.size + 4 + newBytes.size) putString(payload, oldBytes) putString(payload, newBytes) - val response = dispatcher.request(SSH_FXP_RENAME, payload.array()) - if (response.type == SSH_FXP_STATUS) { - val status = decodeStatus(response.payload) - if (status != SftpStatusCode.OK) throw decodeStatusException(response.payload) - } + return dispatchStatusRequest(SSH_FXP_RENAME, payload.array()) } // --- Path operations --- - override suspend fun realpath(path: String): String { + override suspend fun realpath(path: String): SftpResult { val pathBytes = path.toByteArray(StandardCharsets.UTF_8) val payload = ByteBuffer.allocate(4 + pathBytes.size) putString(payload, pathBytes) - val response = dispatcher.request(SSH_FXP_REALPATH, payload.array()) - return when (response.type) { - SSH_FXP_NAME -> { - val entries = decodeName(response.payload) - entries.firstOrNull()?.filename - ?: throw SftpProtocolException("REALPATH returned empty NAME") + return dispatchRequest(SSH_FXP_REALPATH, payload.array()) { response -> + when (response.type) { + SSH_FXP_NAME -> { + val entries = decodeName(response.payload) + val filename = entries.firstOrNull()?.filename + if (filename != null) SftpResult.Success(filename) + else SftpResult.ProtocolError("REALPATH returned empty NAME") + } + SSH_FXP_STATUS -> decodeStatusError(response.payload) + else -> SftpResult.ProtocolError("Unexpected response type ${response.type} for REALPATH") } - - SSH_FXP_STATUS -> throw decodeStatusException(response.payload) - - else -> throw SftpProtocolException("Unexpected response type ${response.type} for REALPATH") } } - override suspend fun readlink(path: String): String { + override suspend fun readlink(path: String): SftpResult { val pathBytes = path.toByteArray(StandardCharsets.UTF_8) val payload = ByteBuffer.allocate(4 + pathBytes.size) putString(payload, pathBytes) - val response = dispatcher.request(SSH_FXP_READLINK, payload.array()) - return when (response.type) { - SSH_FXP_NAME -> { - val entries = decodeName(response.payload) - entries.firstOrNull()?.filename - ?: throw SftpProtocolException("READLINK returned empty NAME") + return dispatchRequest(SSH_FXP_READLINK, payload.array()) { response -> + when (response.type) { + SSH_FXP_NAME -> { + val entries = decodeName(response.payload) + val filename = entries.firstOrNull()?.filename + if (filename != null) SftpResult.Success(filename) + else SftpResult.ProtocolError("READLINK returned empty NAME") + } + SSH_FXP_STATUS -> decodeStatusError(response.payload) + else -> SftpResult.ProtocolError("Unexpected response type ${response.type} for READLINK") } - - SSH_FXP_STATUS -> throw decodeStatusException(response.payload) - - else -> throw SftpProtocolException("Unexpected response type ${response.type} for READLINK") } } - override suspend fun symlink(targetPath: String, linkPath: String) { + override suspend fun symlink(targetPath: String, linkPath: String): SftpResult { // Note: OpenSSH has a known bug where symlink arguments are reversed // from the spec. The spec says (targetpath, linkpath) but OpenSSH // expects (linkpath, targetpath). We follow the OpenSSH convention @@ -299,11 +275,7 @@ internal class SftpClientImpl private constructor( putString(payload, linkBytes) putString(payload, targetBytes) - val response = dispatcher.request(SSH_FXP_SYMLINK, payload.array()) - if (response.type == SSH_FXP_STATUS) { - val status = decodeStatus(response.payload) - if (status != SftpStatusCode.OK) throw decodeStatusException(response.payload) - } + return dispatchStatusRequest(SSH_FXP_SYMLINK, payload.array()) } override fun close() { @@ -313,18 +285,48 @@ internal class SftpClientImpl private constructor( session.close() } - // --- Helpers --- + // --- Internal helpers --- + + /** + * Send a request and map the response, catching transport-level errors + * as [SftpResult.IoError]. + */ + private suspend fun dispatchRequest( + type: Int, + payload: ByteArray, + map: (SftpRawPacket) -> SftpResult, + ): SftpResult { + return try { + val response = dispatcher.request(type, payload) + map(response) + } catch (e: SftpProtocolException) { + SftpResult.ProtocolError(e.message ?: "Protocol error") + } catch (e: Exception) { + SftpResult.IoError(e) + } + } - private suspend fun simplePathRequest(type: Int, path: String) { + /** + * Send a request that expects SSH_FXP_STATUS with OK. + */ + private suspend fun dispatchStatusRequest(type: Int, payload: ByteArray): SftpResult { + return dispatchRequest(type, payload) { response -> + if (response.type == SSH_FXP_STATUS) { + val status = decodeStatus(response.payload) + if (status == SftpStatusCode.OK) SftpResult.Success(Unit) + else decodeStatusError(response.payload) + } else { + SftpResult.Success(Unit) + } + } + } + + private suspend fun simplePathRequest(type: Int, path: String): SftpResult { val pathBytes = path.toByteArray(StandardCharsets.UTF_8) val payload = ByteBuffer.allocate(4 + pathBytes.size) putString(payload, pathBytes) - val response = dispatcher.request(type, payload.array()) - if (response.type == SSH_FXP_STATUS) { - val status = decodeStatus(response.payload) - if (status != SftpStatusCode.OK) throw decodeStatusException(response.payload) - } + return dispatchStatusRequest(type, payload.array()) } companion object { @@ -416,8 +418,8 @@ internal class SftpClientImpl private constructor( return SftpStatusCode.fromCode(code) } - /** Decode a STATUS response into an SftpException. */ - private fun decodeStatusException(payload: ByteArray): SftpException { + /** Decode a STATUS response into an [SftpResult.ServerError]. */ + private fun decodeStatusError(payload: ByteArray): SftpResult.ServerError { val buf = ByteBuffer.wrap(payload) val code = if (buf.remaining() >= 4) buf.int else 4 val statusCode = SftpStatusCode.fromCode(code) @@ -427,7 +429,7 @@ internal class SftpClientImpl private constructor( } else { statusCode.name } - return SftpException(statusCode, message) + return SftpResult.ServerError(statusCode, message) } /** Decode a NAME response (used by readdir, realpath, readlink). */ diff --git a/sshlib/src/test/kotlin/org/connectbot/sshlib/client/sftp/SftpClientIntegrationTest.kt b/sshlib/src/test/kotlin/org/connectbot/sshlib/client/sftp/SftpClientIntegrationTest.kt index 98d6266..de87cd0 100644 --- a/sshlib/src/test/kotlin/org/connectbot/sshlib/client/sftp/SftpClientIntegrationTest.kt +++ b/sshlib/src/test/kotlin/org/connectbot/sshlib/client/sftp/SftpClientIntegrationTest.kt @@ -19,18 +19,16 @@ package org.connectbot.sshlib.client.sftp import kotlinx.coroutines.runBlocking import org.connectbot.sshlib.HostKeyVerifier import org.connectbot.sshlib.PublicKey -import org.connectbot.sshlib.SftpAttributes import org.connectbot.sshlib.SftpClient -import org.connectbot.sshlib.SftpException import org.connectbot.sshlib.SftpOpenFlag +import org.connectbot.sshlib.SftpResult import org.connectbot.sshlib.SftpStatusCode import org.connectbot.sshlib.SshClient import org.connectbot.sshlib.SshClientConfig +import org.connectbot.sshlib.getOrThrow import org.junit.jupiter.api.Assertions.assertEquals import org.junit.jupiter.api.Assertions.assertNotNull -import org.junit.jupiter.api.Assertions.assertNull import org.junit.jupiter.api.Assertions.assertTrue -import org.junit.jupiter.api.Assertions.fail import org.junit.jupiter.api.Test import org.slf4j.LoggerFactory import org.testcontainers.containers.GenericContainer @@ -109,7 +107,7 @@ class SftpClientIntegrationTest { fun `should stat root directory`() = runBlocking { val (client, sftp) = openSftp() try { - val attrs = sftp.stat("/") + val attrs = sftp.stat("/").getOrThrow() assertNotNull(attrs.permissions, "Root dir should have permissions") } finally { sftp.close() @@ -121,7 +119,7 @@ class SftpClientIntegrationTest { fun `should resolve realpath`() = runBlocking { val (client, sftp) = openSftp() try { - val path = sftp.realpath(".") + val path = sftp.realpath(".").getOrThrow() assertTrue(path.startsWith("/"), "Realpath should return absolute path: $path") } finally { sftp.close() @@ -140,32 +138,29 @@ class SftpClientIntegrationTest { val handle = sftp.open( testPath, setOf(SftpOpenFlag.WRITE, SftpOpenFlag.CREATE, SftpOpenFlag.TRUNCATE) - ) - sftp.write(handle, 0, testData) - sftp.close(handle) + ).getOrThrow() + sftp.write(handle, 0, testData).getOrThrow() + sftp.close(handle).getOrThrow() // Read back - val readHandle = sftp.open(testPath, setOf(SftpOpenFlag.READ)) - val readData = sftp.read(readHandle, 0, testData.size) - sftp.close(readHandle) + val readHandle = sftp.open(testPath, setOf(SftpOpenFlag.READ)).getOrThrow() + val readData = sftp.read(readHandle, 0, testData.size).getOrThrow() + sftp.close(readHandle).getOrThrow() assertNotNull(readData) assertTrue(testData.contentEquals(readData!!), "Read data should match written data") // Stat - val attrs = sftp.stat(testPath) + val attrs = sftp.stat(testPath).getOrThrow() assertEquals(testData.size.toLong(), attrs.size, "File size should match") // Delete - sftp.remove(testPath) + sftp.remove(testPath).getOrThrow() // Verify deleted - try { - sftp.stat(testPath) - fail("stat should fail after delete") - } catch (e: SftpException) { - assertEquals(SftpStatusCode.NO_SUCH_FILE, e.statusCode) - } + val result = sftp.stat(testPath) + assertTrue(result is SftpResult.ServerError, "stat should fail after delete") + assertEquals(SftpStatusCode.NO_SUCH_FILE, (result as SftpResult.ServerError).statusCode) } finally { sftp.close() client.disconnect() @@ -176,9 +171,8 @@ class SftpClientIntegrationTest { fun `should list directory`() = runBlocking { val (client, sftp) = openSftp() try { - val entries = sftp.listdir("/tmp") + val entries = sftp.listdir("/tmp").getOrThrow() assertTrue(entries.isNotEmpty(), "Directory listing should not be empty") - // /tmp always has . and .. assertTrue(entries.any { it.filename == "." }, "Should contain '.'") assertTrue(entries.any { it.filename == ".." }, "Should contain '..'") } finally { @@ -193,19 +187,16 @@ class SftpClientIntegrationTest { try { val dirPath = "/tmp/sftp-dir-${System.currentTimeMillis()}" - sftp.mkdir(dirPath) + sftp.mkdir(dirPath).getOrThrow() - val attrs = sftp.stat(dirPath) + val attrs = sftp.stat(dirPath).getOrThrow() assertNotNull(attrs.permissions) - sftp.rmdir(dirPath) + sftp.rmdir(dirPath).getOrThrow() - try { - sftp.stat(dirPath) - fail("stat should fail after rmdir") - } catch (e: SftpException) { - assertEquals(SftpStatusCode.NO_SUCH_FILE, e.statusCode) - } + val result = sftp.stat(dirPath) + assertTrue(result is SftpResult.ServerError, "stat should fail after rmdir") + assertEquals(SftpStatusCode.NO_SUCH_FILE, (result as SftpResult.ServerError).statusCode) } finally { sftp.close() client.disconnect() @@ -221,26 +212,23 @@ class SftpClientIntegrationTest { val newPath = "/tmp/sftp-new-$ts.txt" // Create file - val handle = sftp.open(oldPath, setOf(SftpOpenFlag.WRITE, SftpOpenFlag.CREATE)) - sftp.write(handle, 0, "rename test".toByteArray()) - sftp.close(handle) + val handle = sftp.open(oldPath, setOf(SftpOpenFlag.WRITE, SftpOpenFlag.CREATE)).getOrThrow() + sftp.write(handle, 0, "rename test".toByteArray()).getOrThrow() + sftp.close(handle).getOrThrow() // Rename - sftp.rename(oldPath, newPath) + sftp.rename(oldPath, newPath).getOrThrow() // Old path should not exist - try { - sftp.stat(oldPath) - fail("Old path should not exist after rename") - } catch (e: SftpException) { - assertEquals(SftpStatusCode.NO_SUCH_FILE, e.statusCode) - } + val result = sftp.stat(oldPath) + assertTrue(result is SftpResult.ServerError, "Old path should not exist after rename") + assertEquals(SftpStatusCode.NO_SUCH_FILE, (result as SftpResult.ServerError).statusCode) // New path should exist - sftp.stat(newPath) + sftp.stat(newPath).getOrThrow() // Cleanup - sftp.remove(newPath) + sftp.remove(newPath).getOrThrow() } finally { sftp.close() client.disconnect() @@ -251,12 +239,9 @@ class SftpClientIntegrationTest { fun `should handle file not found`() = runBlocking { val (client, sftp) = openSftp() try { - try { - sftp.stat("/nonexistent/path/that/does/not/exist") - fail("Should throw SftpException for nonexistent path") - } catch (e: SftpException) { - assertEquals(SftpStatusCode.NO_SUCH_FILE, e.statusCode) - } + val result = sftp.stat("/nonexistent/path/that/does/not/exist") + assertTrue(result is SftpResult.ServerError, "Should return ServerError for nonexistent path") + assertEquals(SftpStatusCode.NO_SUCH_FILE, (result as SftpResult.ServerError).statusCode) } finally { sftp.close() client.disconnect() @@ -274,20 +259,20 @@ class SftpClientIntegrationTest { val writeHandle = sftp.open( testPath, setOf(SftpOpenFlag.WRITE, SftpOpenFlag.CREATE, SftpOpenFlag.TRUNCATE) - ) - sftp.write(writeHandle, 0, testData) - sftp.close(writeHandle) + ).getOrThrow() + sftp.write(writeHandle, 0, testData).getOrThrow() + sftp.close(writeHandle).getOrThrow() // Read back in chunks - val readHandle = sftp.open(testPath, setOf(SftpOpenFlag.READ)) + val readHandle = sftp.open(testPath, setOf(SftpOpenFlag.READ)).getOrThrow() val readBuffer = mutableListOf() var offset = 0L while (true) { - val chunk = sftp.read(readHandle, offset, 16384) ?: break + val chunk = sftp.read(readHandle, offset, 16384).getOrThrow() ?: break readBuffer.addAll(chunk.toList()) offset += chunk.size } - sftp.close(readHandle) + sftp.close(readHandle).getOrThrow() assertEquals(testData.size, readBuffer.size, "Should read back all bytes") assertTrue( @@ -295,7 +280,7 @@ class SftpClientIntegrationTest { "Read data should match written data" ) - sftp.remove(testPath) + sftp.remove(testPath).getOrThrow() } finally { sftp.close() client.disconnect() @@ -308,21 +293,20 @@ class SftpClientIntegrationTest { try { val testPath = "/tmp/sftp-attrs-${System.currentTimeMillis()}.txt" - val handle = sftp.open(testPath, setOf(SftpOpenFlag.WRITE, SftpOpenFlag.CREATE)) - sftp.close(handle) + val handle = sftp.open(testPath, setOf(SftpOpenFlag.WRITE, SftpOpenFlag.CREATE)).getOrThrow() + sftp.close(handle).getOrThrow() // Set permissions to 0644 - sftp.setstat(testPath, SftpAttributes(permissions = 0b110_100_100)) // 0644 + sftp.setstat(testPath, org.connectbot.sshlib.SftpAttributes(permissions = 0b110_100_100)).getOrThrow() - val attrs = sftp.stat(testPath) - // Check lower 9 bits (rwx for user/group/other) + val attrs = sftp.stat(testPath).getOrThrow() assertEquals( 0b110_100_100, (attrs.permissions ?: 0) and 0x1FF, "Permissions should be 0644" ) - sftp.remove(testPath) + sftp.remove(testPath).getOrThrow() } finally { sftp.close() client.disconnect() From f8dbd410a3e407442b0e7f7e2e6ff147466504fb Mon Sep 17 00:00:00 2001 From: GlassOnTin Date: Mon, 6 Apr 2026 10:56:28 +0100 Subject: [PATCH 04/10] Fix spotless formatting --- .../org/connectbot/sshlib/SftpClient.kt | 3 + .../kotlin/org/connectbot/sshlib/SshClient.kt | 2 +- .../sshlib/client/sftp/SftpClientImpl.kt | 74 ++++++++++++------- 3 files changed, 52 insertions(+), 27 deletions(-) diff --git a/sshlib/src/main/kotlin/org/connectbot/sshlib/SftpClient.kt b/sshlib/src/main/kotlin/org/connectbot/sshlib/SftpClient.kt index 9f5a880..522ea20 100644 --- a/sshlib/src/main/kotlin/org/connectbot/sshlib/SftpClient.kt +++ b/sshlib/src/main/kotlin/org/connectbot/sshlib/SftpClient.kt @@ -117,8 +117,11 @@ interface SftpClient : AutoCloseable { if (batch.value == null) break entries.addAll(batch.value) } + is SftpResult.ServerError -> return batch + is SftpResult.ProtocolError -> return batch + is SftpResult.IoError -> return batch } } diff --git a/sshlib/src/main/kotlin/org/connectbot/sshlib/SshClient.kt b/sshlib/src/main/kotlin/org/connectbot/sshlib/SshClient.kt index 629b6ac..515fa51 100644 --- a/sshlib/src/main/kotlin/org/connectbot/sshlib/SshClient.kt +++ b/sshlib/src/main/kotlin/org/connectbot/sshlib/SshClient.kt @@ -28,9 +28,9 @@ import kotlinx.coroutines.flow.asSharedFlow import kotlinx.coroutines.launch import org.connectbot.sshlib.client.DynamicPortForwarder import org.connectbot.sshlib.client.LocalPortForwarder -import org.connectbot.sshlib.client.sftp.SftpClientImpl import org.connectbot.sshlib.client.RemotePortForwarder import org.connectbot.sshlib.client.SshConnection +import org.connectbot.sshlib.client.sftp.SftpClientImpl import org.connectbot.sshlib.crypto.PrivateKeyReader import org.connectbot.sshlib.transport.ForwardingChannelTransport import org.connectbot.sshlib.transport.Transport diff --git a/sshlib/src/main/kotlin/org/connectbot/sshlib/client/sftp/SftpClientImpl.kt b/sshlib/src/main/kotlin/org/connectbot/sshlib/client/sftp/SftpClientImpl.kt index 042aa7e..0461021 100644 --- a/sshlib/src/main/kotlin/org/connectbot/sshlib/client/sftp/SftpClientImpl.kt +++ b/sshlib/src/main/kotlin/org/connectbot/sshlib/client/sftp/SftpClientImpl.kt @@ -77,8 +77,11 @@ internal class SftpClientImpl private constructor( return dispatchRequest(SSH_FXP_CLOSE, payload.array()) { response -> if (response.type == SSH_FXP_STATUS) { val status = decodeStatus(response.payload) - if (status == SftpStatusCode.OK) SftpResult.Success(Unit) - else decodeStatusError(response.payload) + if (status == SftpStatusCode.OK) { + SftpResult.Success(Unit) + } else { + decodeStatusError(response.payload) + } } else { SftpResult.Success(Unit) } @@ -94,11 +97,16 @@ internal class SftpClientImpl private constructor( return dispatchRequest(SSH_FXP_READ, payload.array()) { response -> when (response.type) { SSH_FXP_DATA -> SftpResult.Success(extractString(ByteBuffer.wrap(response.payload))) + SSH_FXP_STATUS -> { val status = decodeStatus(response.payload) - if (status == SftpStatusCode.EOF) SftpResult.Success(null) - else decodeStatusError(response.payload) + if (status == SftpStatusCode.EOF) { + SftpResult.Success(null) + } else { + decodeStatusError(response.payload) + } } + else -> SftpResult.ProtocolError("Unexpected response type ${response.type} for READ") } } @@ -188,11 +196,16 @@ internal class SftpClientImpl private constructor( return dispatchRequest(SSH_FXP_READDIR, payload.array()) { response -> when (response.type) { SSH_FXP_NAME -> SftpResult.Success(decodeName(response.payload)) + SSH_FXP_STATUS -> { val status = decodeStatus(response.payload) - if (status == SftpStatusCode.EOF) SftpResult.Success(null) - else decodeStatusError(response.payload) + if (status == SftpStatusCode.EOF) { + SftpResult.Success(null) + } else { + decodeStatusError(response.payload) + } } + else -> SftpResult.ProtocolError("Unexpected response type ${response.type} for READDIR") } } @@ -236,10 +249,15 @@ internal class SftpClientImpl private constructor( SSH_FXP_NAME -> { val entries = decodeName(response.payload) val filename = entries.firstOrNull()?.filename - if (filename != null) SftpResult.Success(filename) - else SftpResult.ProtocolError("REALPATH returned empty NAME") + if (filename != null) { + SftpResult.Success(filename) + } else { + SftpResult.ProtocolError("REALPATH returned empty NAME") + } } + SSH_FXP_STATUS -> decodeStatusError(response.payload) + else -> SftpResult.ProtocolError("Unexpected response type ${response.type} for REALPATH") } } @@ -255,10 +273,15 @@ internal class SftpClientImpl private constructor( SSH_FXP_NAME -> { val entries = decodeName(response.payload) val filename = entries.firstOrNull()?.filename - if (filename != null) SftpResult.Success(filename) - else SftpResult.ProtocolError("READLINK returned empty NAME") + if (filename != null) { + SftpResult.Success(filename) + } else { + SftpResult.ProtocolError("READLINK returned empty NAME") + } } + SSH_FXP_STATUS -> decodeStatusError(response.payload) + else -> SftpResult.ProtocolError("Unexpected response type ${response.type} for READLINK") } } @@ -295,29 +318,28 @@ internal class SftpClientImpl private constructor( type: Int, payload: ByteArray, map: (SftpRawPacket) -> SftpResult, - ): SftpResult { - return try { - val response = dispatcher.request(type, payload) - map(response) - } catch (e: SftpProtocolException) { - SftpResult.ProtocolError(e.message ?: "Protocol error") - } catch (e: Exception) { - SftpResult.IoError(e) - } + ): SftpResult = try { + val response = dispatcher.request(type, payload) + map(response) + } catch (e: SftpProtocolException) { + SftpResult.ProtocolError(e.message ?: "Protocol error") + } catch (e: Exception) { + SftpResult.IoError(e) } /** * Send a request that expects SSH_FXP_STATUS with OK. */ - private suspend fun dispatchStatusRequest(type: Int, payload: ByteArray): SftpResult { - return dispatchRequest(type, payload) { response -> - if (response.type == SSH_FXP_STATUS) { - val status = decodeStatus(response.payload) - if (status == SftpStatusCode.OK) SftpResult.Success(Unit) - else decodeStatusError(response.payload) - } else { + private suspend fun dispatchStatusRequest(type: Int, payload: ByteArray): SftpResult = dispatchRequest(type, payload) { response -> + if (response.type == SSH_FXP_STATUS) { + val status = decodeStatus(response.payload) + if (status == SftpStatusCode.OK) { SftpResult.Success(Unit) + } else { + decodeStatusError(response.payload) } + } else { + SftpResult.Success(Unit) } } From 712dd043b4ee6b7adbbe83317d633197b248e9ea Mon Sep 17 00:00:00 2001 From: GlassOnTin Date: Mon, 6 Apr 2026 23:41:26 +0100 Subject: [PATCH 05/10] Address review: sealed results for SFTP, fix imports Per kruton's review: - SshClient.openSftp() returns SftpResult instead of nullable - SftpDispatcher.request/writeRaw/readRaw return SftpResult instead of throwing - SftpClientImpl.create() returns SftpResult instead of throwing - BlockingSshClient.openSftp() uses getOrThrow() with @Throws annotation - Fix fully qualified StreamForwarder reference (internal, can't import) - Update integration tests for new return types - Regenerate Metalava API signature --- sshlib/api.txt | 97 +++++++++++++++---- .../kotlin/org/connectbot/sshlib/SshClient.kt | 12 +-- .../sshlib/blocking/BlockingSshClient.kt | 5 +- .../sshlib/client/sftp/SftpClientImpl.kt | 39 +++++--- .../sshlib/client/sftp/SftpDispatcher.kt | 36 +++++-- .../client/sftp/SftpClientIntegrationTest.kt | 7 +- 6 files changed, 142 insertions(+), 54 deletions(-) diff --git a/sshlib/api.txt b/sshlib/api.txt index 717055b..230946a 100644 --- a/sshlib/api.txt +++ b/sshlib/api.txt @@ -265,27 +265,27 @@ package org.connectbot.sshlib { public interface SftpClient { method public void close(); - method public suspend java.lang.Object? close(org.connectbot.sshlib.SftpFileHandle handle, kotlin.coroutines.Continuation); - method public suspend java.lang.Object? fsetstat(org.connectbot.sshlib.SftpFileHandle handle, org.connectbot.sshlib.SftpAttributes attrs, kotlin.coroutines.Continuation); - method public suspend java.lang.Object? fstat(org.connectbot.sshlib.SftpFileHandle handle, kotlin.coroutines.Continuation); + method public suspend java.lang.Object? close(org.connectbot.sshlib.SftpFileHandle handle, kotlin.coroutines.Continuation>); + method public suspend java.lang.Object? fsetstat(org.connectbot.sshlib.SftpFileHandle handle, org.connectbot.sshlib.SftpAttributes attrs, kotlin.coroutines.Continuation>); + method public suspend java.lang.Object? fstat(org.connectbot.sshlib.SftpFileHandle handle, kotlin.coroutines.Continuation>); method @InaccessibleFromKotlin public int getProtocolVersion(); method @InaccessibleFromKotlin public boolean isOpen(); - method public default suspend java.lang.Object? listdir(java.lang.String path, kotlin.coroutines.Continuation>); - method public suspend java.lang.Object? lstat(java.lang.String path, kotlin.coroutines.Continuation); - method public suspend java.lang.Object? mkdir(java.lang.String path, optional org.connectbot.sshlib.SftpAttributes attrs, kotlin.coroutines.Continuation); - method public suspend java.lang.Object? open(java.lang.String path, java.util.Set flags, optional org.connectbot.sshlib.SftpAttributes attrs, kotlin.coroutines.Continuation); - method public suspend java.lang.Object? opendir(java.lang.String path, kotlin.coroutines.Continuation); - method public suspend java.lang.Object? read(org.connectbot.sshlib.SftpFileHandle handle, long offset, int length, kotlin.coroutines.Continuation); - method public suspend java.lang.Object? readdir(org.connectbot.sshlib.SftpFileHandle handle, kotlin.coroutines.Continuation?>); - method public suspend java.lang.Object? readlink(java.lang.String path, kotlin.coroutines.Continuation); - method public suspend java.lang.Object? realpath(java.lang.String path, kotlin.coroutines.Continuation); - method public suspend java.lang.Object? remove(java.lang.String path, kotlin.coroutines.Continuation); - method public suspend java.lang.Object? rename(java.lang.String oldPath, java.lang.String newPath, kotlin.coroutines.Continuation); - method public suspend java.lang.Object? rmdir(java.lang.String path, kotlin.coroutines.Continuation); - method public suspend java.lang.Object? setstat(java.lang.String path, org.connectbot.sshlib.SftpAttributes attrs, kotlin.coroutines.Continuation); - method public suspend java.lang.Object? stat(java.lang.String path, kotlin.coroutines.Continuation); - method public suspend java.lang.Object? symlink(java.lang.String targetPath, java.lang.String linkPath, kotlin.coroutines.Continuation); - method public suspend java.lang.Object? write(org.connectbot.sshlib.SftpFileHandle handle, long offset, byte[] data, kotlin.coroutines.Continuation); + method public default suspend java.lang.Object? listdir(java.lang.String path, kotlin.coroutines.Continuation>>); + method public suspend java.lang.Object? lstat(java.lang.String path, kotlin.coroutines.Continuation>); + method public suspend java.lang.Object? mkdir(java.lang.String path, optional org.connectbot.sshlib.SftpAttributes attrs, kotlin.coroutines.Continuation>); + method public suspend java.lang.Object? open(java.lang.String path, java.util.Set flags, optional org.connectbot.sshlib.SftpAttributes attrs, kotlin.coroutines.Continuation>); + method public suspend java.lang.Object? opendir(java.lang.String path, kotlin.coroutines.Continuation>); + method public suspend java.lang.Object? read(org.connectbot.sshlib.SftpFileHandle handle, long offset, int length, kotlin.coroutines.Continuation>); + method public suspend java.lang.Object? readdir(org.connectbot.sshlib.SftpFileHandle handle, kotlin.coroutines.Continuation?>>); + method public suspend java.lang.Object? readlink(java.lang.String path, kotlin.coroutines.Continuation>); + method public suspend java.lang.Object? realpath(java.lang.String path, kotlin.coroutines.Continuation>); + method public suspend java.lang.Object? remove(java.lang.String path, kotlin.coroutines.Continuation>); + method public suspend java.lang.Object? rename(java.lang.String oldPath, java.lang.String newPath, kotlin.coroutines.Continuation>); + method public suspend java.lang.Object? rmdir(java.lang.String path, kotlin.coroutines.Continuation>); + method public suspend java.lang.Object? setstat(java.lang.String path, org.connectbot.sshlib.SftpAttributes attrs, kotlin.coroutines.Continuation>); + method public suspend java.lang.Object? stat(java.lang.String path, kotlin.coroutines.Continuation>); + method public suspend java.lang.Object? symlink(java.lang.String targetPath, java.lang.String linkPath, kotlin.coroutines.Continuation>); + method public suspend java.lang.Object? write(org.connectbot.sshlib.SftpFileHandle handle, long offset, byte[] data, kotlin.coroutines.Continuation>); property public abstract boolean isOpen; property public abstract int protocolVersion; } @@ -330,6 +330,61 @@ package org.connectbot.sshlib { enum_constant public static final org.connectbot.sshlib.SftpOpenFlag WRITE; } + public sealed exhaustive interface SftpResult { + } + + public static final class SftpResult.IoError implements org.connectbot.sshlib.SftpResult { + ctor public SftpResult.IoError(java.lang.Throwable cause); + method public java.lang.Throwable component1(); + method public org.connectbot.sshlib.SftpResult.IoError copy(optional java.lang.Throwable cause); + method public boolean equals(java.lang.Object? other); + method @InaccessibleFromKotlin public java.lang.Throwable getCause(); + method public int hashCode(); + method public java.lang.String toString(); + property public Throwable cause; + } + + public static final class SftpResult.ProtocolError implements org.connectbot.sshlib.SftpResult { + ctor public SftpResult.ProtocolError(java.lang.String message); + method public java.lang.String component1(); + method public org.connectbot.sshlib.SftpResult.ProtocolError copy(optional java.lang.String message); + method public boolean equals(java.lang.Object? other); + method @InaccessibleFromKotlin public java.lang.String getMessage(); + method public int hashCode(); + method public java.lang.String toString(); + property public String message; + } + + public static final class SftpResult.ServerError implements org.connectbot.sshlib.SftpResult { + ctor public SftpResult.ServerError(org.connectbot.sshlib.SftpStatusCode statusCode, java.lang.String message); + method public org.connectbot.sshlib.SftpStatusCode component1(); + method public java.lang.String component2(); + method public org.connectbot.sshlib.SftpResult.ServerError copy(optional org.connectbot.sshlib.SftpStatusCode statusCode, optional java.lang.String message); + method public boolean equals(java.lang.Object? other); + method @InaccessibleFromKotlin public java.lang.String getMessage(); + method @InaccessibleFromKotlin public org.connectbot.sshlib.SftpStatusCode getStatusCode(); + method public int hashCode(); + method public java.lang.String toString(); + property public String message; + property public org.connectbot.sshlib.SftpStatusCode statusCode; + } + + public static final class SftpResult.Success implements org.connectbot.sshlib.SftpResult { + ctor public SftpResult.Success(T value); + method public T component1(); + method public org.connectbot.sshlib.SftpResult.Success copy(optional T value); + method public boolean equals(java.lang.Object? other); + method @InaccessibleFromKotlin public T getValue(); + method public int hashCode(); + method public java.lang.String toString(); + property public T value; + } + + public final class SftpResultKt { + method public static T? getOrNull(org.connectbot.sshlib.SftpResult); + method public static T getOrThrow(org.connectbot.sshlib.SftpResult); + } + public enum SftpStatusCode { method @InaccessibleFromKotlin public int getCode(); property public int code; @@ -373,7 +428,7 @@ package org.connectbot.sshlib { method public suspend java.lang.Object? localPortForward(int bindPort, java.lang.String remoteHost, int remotePort, kotlin.coroutines.Continuation); method public org.connectbot.sshlib.transport.TransportFactory? openDirectTcpipTransport(java.lang.String remoteHost, int remotePort, optional java.lang.String originAddr, optional int originPort); method public suspend java.lang.Object? openSession(kotlin.coroutines.Continuation); - method public suspend java.lang.Object? openSftp(kotlin.coroutines.Continuation); + method public suspend java.lang.Object? openSftp(kotlin.coroutines.Continuation>); method public suspend java.lang.Object? remotePortForward(java.lang.String remoteBindAddress, int remoteBindPort, java.lang.String localHost, int localPort, kotlin.coroutines.Continuation); property public kotlinx.coroutines.flow.SharedFlow disconnectedFlow; property public boolean isAuthenticated; @@ -539,7 +594,7 @@ package org.connectbot.sshlib.blocking { method public org.connectbot.sshlib.transport.TransportFactory? openDirectTcpipTransport(java.lang.String remoteHost, int remotePort, optional java.lang.String originAddr); method public org.connectbot.sshlib.transport.TransportFactory? openDirectTcpipTransport(java.lang.String remoteHost, int remotePort, optional java.lang.String originAddr, optional int originPort); method public org.connectbot.sshlib.SshSession? openSession(); - method public org.connectbot.sshlib.SftpClient? openSftp(); + method @kotlin.jvm.Throws(exceptionClasses=SftpException::class) public org.connectbot.sshlib.SftpClient openSftp() throws org.connectbot.sshlib.SftpException; method public org.connectbot.sshlib.PortForwarder? remotePortForward(java.lang.String remoteBindAddress, int remoteBindPort, java.lang.String localHost, int localPort); property public kotlinx.coroutines.flow.SharedFlow disconnectedFlow; property public boolean isAuthenticated; diff --git a/sshlib/src/main/kotlin/org/connectbot/sshlib/SshClient.kt b/sshlib/src/main/kotlin/org/connectbot/sshlib/SshClient.kt index 515fa51..bfe96e6 100644 --- a/sshlib/src/main/kotlin/org/connectbot/sshlib/SshClient.kt +++ b/sshlib/src/main/kotlin/org/connectbot/sshlib/SshClient.kt @@ -408,27 +408,27 @@ class SshClient private constructor( * Opens a new session channel, starts the "sftp" subsystem, and performs * SFTP version negotiation. * - * @return SftpClient instance, or null if not authenticated or connection fails + * @return [SftpResult.Success] with the client, or an error variant */ - suspend fun openSftp(): SftpClient? { + suspend fun openSftp(): SftpResult { val conn = connection if (conn == null || !authenticated) { logger.error("Not authenticated - call connect() and authenticate first") - return null + return SftpResult.IoError(IllegalStateException("Not authenticated")) } return try { logger.info("Opening SFTP session") val session = conn.openSessionChannel() - ?: throw SshException("Failed to open session channel for SFTP") + ?: return SftpResult.ProtocolError("Failed to open session channel for SFTP") if (!session.requestSubsystem("sftp")) { session.close() - throw SshException("Server rejected SFTP subsystem request") + return SftpResult.ProtocolError("Server rejected SFTP subsystem request") } SftpClientImpl.create(session) } catch (e: Exception) { logger.error("Failed to open SFTP session", e) - null + SftpResult.IoError(e) } } diff --git a/sshlib/src/main/kotlin/org/connectbot/sshlib/blocking/BlockingSshClient.kt b/sshlib/src/main/kotlin/org/connectbot/sshlib/blocking/BlockingSshClient.kt index 3221ebb..c00bb38 100644 --- a/sshlib/src/main/kotlin/org/connectbot/sshlib/blocking/BlockingSshClient.kt +++ b/sshlib/src/main/kotlin/org/connectbot/sshlib/blocking/BlockingSshClient.kt @@ -28,6 +28,8 @@ import org.connectbot.sshlib.HostKeyVerifier import org.connectbot.sshlib.KeyboardInteractiveCallback import org.connectbot.sshlib.PortForwarder import org.connectbot.sshlib.SftpClient +import org.connectbot.sshlib.SftpException +import org.connectbot.sshlib.getOrThrow import org.connectbot.sshlib.Socks5Authenticator import org.connectbot.sshlib.SshClient import org.connectbot.sshlib.SshClientConfig @@ -242,7 +244,8 @@ class BlockingSshClient internal constructor( /** * Open an SFTP session for file transfer (blocking wrapper). */ - fun openSftp(): SftpClient? = runBlocking { client.openSftp() } + @Throws(SftpException::class) + fun openSftp(): SftpClient = runBlocking { client.openSftp().getOrThrow() } /** * Start local port forwarding. diff --git a/sshlib/src/main/kotlin/org/connectbot/sshlib/client/sftp/SftpClientImpl.kt b/sshlib/src/main/kotlin/org/connectbot/sshlib/client/sftp/SftpClientImpl.kt index 0461021..f239acb 100644 --- a/sshlib/src/main/kotlin/org/connectbot/sshlib/client/sftp/SftpClientImpl.kt +++ b/sshlib/src/main/kotlin/org/connectbot/sshlib/client/sftp/SftpClientImpl.kt @@ -311,20 +311,19 @@ internal class SftpClientImpl private constructor( // --- Internal helpers --- /** - * Send a request and map the response, catching transport-level errors - * as [SftpResult.IoError]. + * Send a request and map the response. */ private suspend fun dispatchRequest( type: Int, payload: ByteArray, map: (SftpRawPacket) -> SftpResult, - ): SftpResult = try { - val response = dispatcher.request(type, payload) - map(response) - } catch (e: SftpProtocolException) { - SftpResult.ProtocolError(e.message ?: "Protocol error") - } catch (e: Exception) { - SftpResult.IoError(e) + ): SftpResult { + return when (val result = dispatcher.request(type, payload)) { + is SftpResult.Success -> map(result.value) + is SftpResult.ServerError -> result + is SftpResult.ProtocolError -> result + is SftpResult.IoError -> result + } } /** @@ -387,24 +386,34 @@ internal class SftpClientImpl private constructor( /** * Create an SFTP client by performing the INIT/VERSION handshake. */ - suspend fun create(session: SshSession): SftpClient { + suspend fun create(session: SshSession): SftpResult { val packetIO = SftpPacketIO(session) val dispatcher = SftpDispatcher(packetIO) // Send SSH_FXP_INIT val initPayload = ByteBuffer.allocate(4) initPayload.putInt(SFTP_VERSION) - dispatcher.writeRaw(SSH_FXP_INIT, initPayload.array()) + when (val w = dispatcher.writeRaw(SSH_FXP_INIT, initPayload.array())) { + is SftpResult.Success -> {} + is SftpResult.ServerError -> return w + is SftpResult.ProtocolError -> return w + is SftpResult.IoError -> return w + } // Read SSH_FXP_VERSION - val versionPacket = dispatcher.readRaw() + val versionPacket = when (val r = dispatcher.readRaw()) { + is SftpResult.Success -> r.value + is SftpResult.ServerError -> return r + is SftpResult.ProtocolError -> return r + is SftpResult.IoError -> return r + } if (versionPacket.type != SSH_FXP_VERSION) { - throw SftpProtocolException( + return SftpResult.ProtocolError( "Expected SSH_FXP_VERSION (2), got ${versionPacket.type}" ) } if (versionPacket.payload.size < 4) { - throw SftpProtocolException("SSH_FXP_VERSION payload too short") + return SftpResult.ProtocolError("SSH_FXP_VERSION payload too short") } val serverVersion = ByteBuffer.wrap(versionPacket.payload, 0, 4).int val negotiatedVersion = minOf(SFTP_VERSION, serverVersion) @@ -414,7 +423,7 @@ internal class SftpClientImpl private constructor( val scope = CoroutineScope(Dispatchers.IO + SupervisorJob()) val readJob = dispatcher.startReadLoop(scope) - return SftpClientImpl(session, dispatcher, readJob, negotiatedVersion) + return SftpResult.Success(SftpClientImpl(session, dispatcher, readJob, negotiatedVersion)) } // --- Wire format helpers --- diff --git a/sshlib/src/main/kotlin/org/connectbot/sshlib/client/sftp/SftpDispatcher.kt b/sshlib/src/main/kotlin/org/connectbot/sshlib/client/sftp/SftpDispatcher.kt index 142ddd4..d234d37 100644 --- a/sshlib/src/main/kotlin/org/connectbot/sshlib/client/sftp/SftpDispatcher.kt +++ b/sshlib/src/main/kotlin/org/connectbot/sshlib/client/sftp/SftpDispatcher.kt @@ -23,6 +23,7 @@ import kotlinx.coroutines.launch import kotlinx.coroutines.sync.Mutex import kotlinx.coroutines.sync.withLock import kotlinx.coroutines.withTimeout +import org.connectbot.sshlib.SftpResult import org.slf4j.LoggerFactory import java.nio.ByteBuffer import java.util.concurrent.ConcurrentHashMap @@ -48,12 +49,12 @@ internal class SftpDispatcher(private val packetIO: SftpPacketIO) { * @param timeoutMs Maximum time to wait for a response * @return The raw response packet */ - suspend fun request(type: Int, payload: ByteArray, timeoutMs: Long = 30_000L): SftpRawPacket { + suspend fun request(type: Int, payload: ByteArray, timeoutMs: Long = 30_000L): SftpResult { val requestId = nextRequestId.getAndIncrement() val deferred = CompletableDeferred() pending[requestId] = deferred - try { + return try { // Prepend request ID to payload val fullPayload = ByteBuffer.allocate(4 + payload.size) fullPayload.putInt(requestId) @@ -63,28 +64,47 @@ internal class SftpDispatcher(private val packetIO: SftpPacketIO) { packetIO.writePacket(type, fullPayload.array()) } - return withTimeout(timeoutMs) { + val packet = withTimeout(timeoutMs) { deferred.await() } + SftpResult.Success(packet) + } catch (e: SftpProtocolException) { + pending.remove(requestId) + SftpResult.ProtocolError(e.message ?: "Protocol error") } catch (e: Exception) { pending.remove(requestId) - throw e + SftpResult.IoError(e) } } /** * Send an SFTP packet without a request ID (used for INIT). */ - suspend fun writeRaw(type: Int, payload: ByteArray) { - writeMutex.withLock { - packetIO.writePacket(type, payload) + suspend fun writeRaw(type: Int, payload: ByteArray): SftpResult { + return try { + writeMutex.withLock { + packetIO.writePacket(type, payload) + } + SftpResult.Success(Unit) + } catch (e: SftpProtocolException) { + SftpResult.ProtocolError(e.message ?: "Protocol error") + } catch (e: Exception) { + SftpResult.IoError(e) } } /** * Read a single raw packet (used for VERSION response during init). */ - suspend fun readRaw(): SftpRawPacket = packetIO.readPacket() + suspend fun readRaw(): SftpResult { + return try { + SftpResult.Success(packetIO.readPacket()) + } catch (e: SftpProtocolException) { + SftpResult.ProtocolError(e.message ?: "Protocol error") + } catch (e: Exception) { + SftpResult.IoError(e) + } + } /** * Start the background read loop that routes responses to waiting callers. diff --git a/sshlib/src/test/kotlin/org/connectbot/sshlib/client/sftp/SftpClientIntegrationTest.kt b/sshlib/src/test/kotlin/org/connectbot/sshlib/client/sftp/SftpClientIntegrationTest.kt index de87cd0..a48816d 100644 --- a/sshlib/src/test/kotlin/org/connectbot/sshlib/client/sftp/SftpClientIntegrationTest.kt +++ b/sshlib/src/test/kotlin/org/connectbot/sshlib/client/sftp/SftpClientIntegrationTest.kt @@ -86,9 +86,10 @@ class SftpClientIntegrationTest { assertTrue(client.connect(), "Should connect to SSH server") assertTrue(client.authenticatePassword(USERNAME, PASSWORD), "Should authenticate") - val sftp = client.openSftp() - assertNotNull(sftp, "Should open SFTP session") - return Pair(client, sftp!!) + val sftpResult = client.openSftp() + assertTrue(sftpResult is SftpResult.Success, "Should open SFTP session, got: $sftpResult") + val sftp = (sftpResult as SftpResult.Success).value + return Pair(client, sftp) } @Test From 811a145b5d23689fa34fd9834de2926536b5ee27 Mon Sep 17 00:00:00 2001 From: GlassOnTin Date: Sat, 25 Apr 2026 21:18:53 +0100 Subject: [PATCH 06/10] Address review: convert SftpPacketIO to sealed-result return MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Per @kruton's feedback on PR #112: Kotlin library APIs shouldn't throw for things they can manage themselves (network errors, malformed packets). Throws are reserved for the BlockingSshClient wrapper layer (Java interop, @Throws-annotated). The previous "address review" commit converted the SftpClient and SshClient surfaces but left SftpPacketIO still throwing — SftpDispatcher caught the throws to translate to SftpResult, which kept the contract-on-the-wire correct but missed the design intent of keeping throws out of the suspend layer entirely. Changes: - SftpPacketIO.readPacket() now returns SftpResult instead of throwing. The two error paths (invalid packet length, channel closed mid-packet) become ProtocolError / IoError variants. The private readExact() helper still throws a new internal ChannelClosedException; readPacket catches and translates. Keeps the throw scoped to a single private call chain that never escapes the file. - SftpPacketIO.writePacket() now returns SftpResult; underlying session.write() exceptions become IoError. - SftpDispatcher.request/writeRaw/readRaw now propagate the SftpResult from the framing layer directly instead of catching exceptions. Cleaner: only the timeout/await path keeps its catch (the Coroutines API throws on cancel / timeout, no result type to consume). - SftpDispatcher.startReadLoop's read loop uses a `when` on the SftpResult to handle each variant — IoError / ProtocolError drain pending requests with the matching exception, success case routes to the requestId match. - SftpClientIntegrationTest: pre-existing breakage on the branch (connect()/authenticatePassword() now return sealed types upstream). Updated test assertions to check for the Success variant. --- .../sshlib/client/sftp/SftpDispatcher.kt | 94 +++++++++++-------- .../sshlib/client/sftp/SftpPacketIO.kt | 73 +++++++++----- .../client/sftp/SftpClientIntegrationTest.kt | 6 +- 3 files changed, 109 insertions(+), 64 deletions(-) diff --git a/sshlib/src/main/kotlin/org/connectbot/sshlib/client/sftp/SftpDispatcher.kt b/sshlib/src/main/kotlin/org/connectbot/sshlib/client/sftp/SftpDispatcher.kt index d234d37..1a0c450 100644 --- a/sshlib/src/main/kotlin/org/connectbot/sshlib/client/sftp/SftpDispatcher.kt +++ b/sshlib/src/main/kotlin/org/connectbot/sshlib/client/sftp/SftpDispatcher.kt @@ -54,23 +54,36 @@ internal class SftpDispatcher(private val packetIO: SftpPacketIO) { val deferred = CompletableDeferred() pending[requestId] = deferred - return try { - // Prepend request ID to payload - val fullPayload = ByteBuffer.allocate(4 + payload.size) - fullPayload.putInt(requestId) - fullPayload.put(payload) + // Prepend request ID to payload + val fullPayload = ByteBuffer.allocate(4 + payload.size) + fullPayload.putInt(requestId) + fullPayload.put(payload) - writeMutex.withLock { - packetIO.writePacket(type, fullPayload.array()) - } + // The framing layer ([packetIO]) now returns sealed SftpResult + // rather than throwing — propagate any error from the write + // straight back to the caller. Timeout / await are still + // exception-paths so they keep the catch. + val writeResult = writeMutex.withLock { + packetIO.writePacket(type, fullPayload.array()) + } + if (writeResult is SftpResult.IoError) { + pending.remove(requestId) + return writeResult + } + if (writeResult is SftpResult.ProtocolError) { + pending.remove(requestId) + return writeResult + } + if (writeResult is SftpResult.ServerError) { + pending.remove(requestId) + return writeResult + } + return try { val packet = withTimeout(timeoutMs) { deferred.await() } SftpResult.Success(packet) - } catch (e: SftpProtocolException) { - pending.remove(requestId) - SftpResult.ProtocolError(e.message ?: "Protocol error") } catch (e: Exception) { pending.remove(requestId) SftpResult.IoError(e) @@ -78,33 +91,20 @@ internal class SftpDispatcher(private val packetIO: SftpPacketIO) { } /** - * Send an SFTP packet without a request ID (used for INIT). + * Send an SFTP packet without a request ID (used for INIT). Just + * forwards the framing-layer result. */ suspend fun writeRaw(type: Int, payload: ByteArray): SftpResult { - return try { - writeMutex.withLock { - packetIO.writePacket(type, payload) - } - SftpResult.Success(Unit) - } catch (e: SftpProtocolException) { - SftpResult.ProtocolError(e.message ?: "Protocol error") - } catch (e: Exception) { - SftpResult.IoError(e) + return writeMutex.withLock { + packetIO.writePacket(type, payload) } } /** * Read a single raw packet (used for VERSION response during init). + * Just forwards the framing-layer result. */ - suspend fun readRaw(): SftpResult { - return try { - SftpResult.Success(packetIO.readPacket()) - } catch (e: SftpProtocolException) { - SftpResult.ProtocolError(e.message ?: "Protocol error") - } catch (e: Exception) { - SftpResult.IoError(e) - } - } + suspend fun readRaw(): SftpResult = packetIO.readPacket() /** * Start the background read loop that routes responses to waiting callers. @@ -112,8 +112,29 @@ internal class SftpDispatcher(private val packetIO: SftpPacketIO) { fun startReadLoop(scope: CoroutineScope): Job { val job = scope.launch { try { - while (true) { - val packet = packetIO.readPacket() + loop@ while (true) { + val packet = when (val result = packetIO.readPacket()) { + is SftpResult.Success -> result.value + is SftpResult.IoError -> { + logger.debug("SFTP read loop ended: {}", result.cause.message) + pending.values.forEach { it.completeExceptionally(result.cause) } + pending.clear() + break@loop + } + is SftpResult.ProtocolError -> { + logger.debug("SFTP channel closed: {}", result.message) + val err = SftpProtocolException(result.message) + pending.values.forEach { it.completeExceptionally(err) } + pending.clear() + break@loop + } + is SftpResult.ServerError -> { + // Framing layer never produces ServerError, but the + // sealed exhaustiveness check requires this branch. + logger.warn("Unexpected ServerError from packet framing: code={}", result.statusCode) + break@loop + } + } // Extract request ID from first 4 bytes of payload if (packet.payload.size < 4) { @@ -132,14 +153,9 @@ internal class SftpDispatcher(private val packetIO: SftpPacketIO) { logger.warn("SFTP response for unknown request ID {}", requestId) } } - } catch (e: SftpProtocolException) { - logger.debug("SFTP channel closed: {}", e.message) - // Complete all pending requests with the error - val error = e - pending.values.forEach { it.completeExceptionally(error) } - pending.clear() } catch (e: Exception) { - logger.debug("SFTP read loop ended: {}", e.message) + // Coroutine cancellation or other unexpected error. + logger.debug("SFTP read loop ended unexpectedly: {}", e.message) pending.values.forEach { it.completeExceptionally(e) } pending.clear() } diff --git a/sshlib/src/main/kotlin/org/connectbot/sshlib/client/sftp/SftpPacketIO.kt b/sshlib/src/main/kotlin/org/connectbot/sshlib/client/sftp/SftpPacketIO.kt index 528f93f..16627cf 100644 --- a/sshlib/src/main/kotlin/org/connectbot/sshlib/client/sftp/SftpPacketIO.kt +++ b/sshlib/src/main/kotlin/org/connectbot/sshlib/client/sftp/SftpPacketIO.kt @@ -16,6 +16,7 @@ package org.connectbot.sshlib.client.sftp +import org.connectbot.sshlib.SftpResult import org.connectbot.sshlib.SshSession import java.io.ByteArrayOutputStream import java.nio.ByteBuffer @@ -39,39 +40,58 @@ internal class SftpPacketIO(private val session: SshSession) { /** * Read a complete SFTP packet. Blocks (suspends) until enough data arrives. * - * @return Parsed SFTP packet (type + payload, without the length prefix) - * @throws SftpProtocolException if the channel closes before a complete packet + * Returns a sealed [SftpResult] rather than throwing — network errors and + * malformed packets are normal failure modes that callers should handle + * explicitly. (Reviewed by @kruton on PR #112: Kotlin library APIs + * shouldn't throw for things they can manage themselves.) */ - suspend fun readPacket(): SftpRawPacket { - // Read the 4-byte length prefix - val lengthBytes = readExact(4) - val length = ByteBuffer.wrap(lengthBytes).int - if (length < 1 || length > MAX_PACKET_SIZE) { - throw SftpProtocolException("Invalid SFTP packet length: $length") - } + suspend fun readPacket(): SftpResult { + return try { + // Read the 4-byte length prefix + val lengthBytes = readExact(4) + val length = ByteBuffer.wrap(lengthBytes).int + if (length < 1 || length > MAX_PACKET_SIZE) { + return SftpResult.ProtocolError("Invalid SFTP packet length: $length") + } - // Read the packet body (type + payload) - val body = readExact(length) - val type = body[0].toInt() and 0xFF - val payload = body.copyOfRange(1, body.size) - return SftpRawPacket(type, payload) + // Read the packet body (type + payload) + val body = readExact(length) + val type = body[0].toInt() and 0xFF + val payload = body.copyOfRange(1, body.size) + SftpResult.Success(SftpRawPacket(type, payload)) + } catch (e: ChannelClosedException) { + SftpResult.IoError(e) + } catch (e: Exception) { + SftpResult.IoError(e) + } } /** * Write an SFTP packet with the given type and payload. + * + * Returns [SftpResult.Success] on send or [SftpResult.IoError] if the + * underlying SSH session write fails. */ - suspend fun writePacket(type: Int, payload: ByteArray) { - val length = 1 + payload.size // type byte + payload - val packet = ByteBuffer.allocate(4 + length) - packet.putInt(length) - packet.put(type.toByte()) - packet.put(payload) - session.write(packet.array()) + suspend fun writePacket(type: Int, payload: ByteArray): SftpResult { + return try { + val length = 1 + payload.size // type byte + payload + val packet = ByteBuffer.allocate(4 + length) + packet.putInt(length) + packet.put(type.toByte()) + packet.put(payload) + session.write(packet.array()) + SftpResult.Success(Unit) + } catch (e: Exception) { + SftpResult.IoError(e) + } } /** * Read exactly [count] bytes from the session, accumulating across - * multiple channel data chunks as needed. + * multiple channel data chunks as needed. Throws [ChannelClosedException] + * if the channel closes mid-packet — callers (only [readPacket]) catch + * and translate to [SftpResult.IoError]. Kept private so the throw + * doesn't leak past the API surface. */ private suspend fun readExact(count: Int): ByteArray { val result = ByteArray(count) @@ -89,7 +109,7 @@ internal class SftpPacketIO(private val session: SshSession) { // Read from the session until we have enough while (filled < count) { val data = session.read() - ?: throw SftpProtocolException("SSH channel closed before complete SFTP packet") + ?: throw ChannelClosedException("SSH channel closed before complete SFTP packet") val toCopy = minOf(count - filled, data.size) System.arraycopy(data, 0, result, filled, toCopy) @@ -112,6 +132,13 @@ internal class SftpPacketIO(private val session: SshSession) { } } +/** + * Internal exception used by [SftpPacketIO.readExact] to signal a closed + * channel mid-packet. Caught by [SftpPacketIO.readPacket] and translated + * into [SftpResult.IoError]; never escapes this file. + */ +internal class ChannelClosedException(message: String) : Exception(message) + /** * Raw SFTP packet with type byte and payload (without the length prefix). */ diff --git a/sshlib/src/test/kotlin/org/connectbot/sshlib/client/sftp/SftpClientIntegrationTest.kt b/sshlib/src/test/kotlin/org/connectbot/sshlib/client/sftp/SftpClientIntegrationTest.kt index a48816d..3a40f40 100644 --- a/sshlib/src/test/kotlin/org/connectbot/sshlib/client/sftp/SftpClientIntegrationTest.kt +++ b/sshlib/src/test/kotlin/org/connectbot/sshlib/client/sftp/SftpClientIntegrationTest.kt @@ -83,8 +83,10 @@ class SftpClientIntegrationTest { } val client = SshClient(config) - assertTrue(client.connect(), "Should connect to SSH server") - assertTrue(client.authenticatePassword(USERNAME, PASSWORD), "Should authenticate") + val connectResult = client.connect() + assertTrue(connectResult is org.connectbot.sshlib.ConnectResult.Success, "Should connect to SSH server, got: $connectResult") + val authResult = client.authenticatePassword(USERNAME, PASSWORD) + assertTrue(authResult is org.connectbot.sshlib.AuthResult.Success, "Should authenticate, got: $authResult") val sftpResult = client.openSftp() assertTrue(sftpResult is SftpResult.Success, "Should open SFTP session, got: $sftpResult") From 06f8d697a83756bf22e73d498916b1d2cae31372 Mon Sep 17 00:00:00 2001 From: GlassOnTin Date: Sat, 25 Apr 2026 21:47:13 +0100 Subject: [PATCH 07/10] Run spotlessApply CI failed on the previous push because of formatting violations across files I edited (SftpPacketIO, SftpDispatcher, the test) and a few that pre-existed on the branch (BlockingSshClient, SftpClientImpl, SftpFileAttributes). --- .../sshlib/blocking/BlockingSshClient.kt | 2 +- .../sshlib/client/sftp/SftpClientImpl.kt | 14 +++++------- .../sshlib/client/sftp/SftpDispatcher.kt | 9 ++++---- .../sshlib/client/sftp/SftpFileAttributes.kt | 2 +- .../sshlib/client/sftp/SftpPacketIO.kt | 22 +++++++++---------- .../client/sftp/SftpClientIntegrationTest.kt | 12 +++++----- 6 files changed, 29 insertions(+), 32 deletions(-) diff --git a/sshlib/src/main/kotlin/org/connectbot/sshlib/blocking/BlockingSshClient.kt b/sshlib/src/main/kotlin/org/connectbot/sshlib/blocking/BlockingSshClient.kt index c00bb38..40b181c 100644 --- a/sshlib/src/main/kotlin/org/connectbot/sshlib/blocking/BlockingSshClient.kt +++ b/sshlib/src/main/kotlin/org/connectbot/sshlib/blocking/BlockingSshClient.kt @@ -29,13 +29,13 @@ import org.connectbot.sshlib.KeyboardInteractiveCallback import org.connectbot.sshlib.PortForwarder import org.connectbot.sshlib.SftpClient import org.connectbot.sshlib.SftpException -import org.connectbot.sshlib.getOrThrow import org.connectbot.sshlib.Socks5Authenticator import org.connectbot.sshlib.SshClient import org.connectbot.sshlib.SshClientConfig import org.connectbot.sshlib.SshException import org.connectbot.sshlib.SshSession import org.connectbot.sshlib.StreamForwarder +import org.connectbot.sshlib.getOrThrow import org.connectbot.sshlib.transport.TransportFactory import java.net.InetSocketAddress diff --git a/sshlib/src/main/kotlin/org/connectbot/sshlib/client/sftp/SftpClientImpl.kt b/sshlib/src/main/kotlin/org/connectbot/sshlib/client/sftp/SftpClientImpl.kt index f239acb..1bb19b0 100644 --- a/sshlib/src/main/kotlin/org/connectbot/sshlib/client/sftp/SftpClientImpl.kt +++ b/sshlib/src/main/kotlin/org/connectbot/sshlib/client/sftp/SftpClientImpl.kt @@ -317,13 +317,11 @@ internal class SftpClientImpl private constructor( type: Int, payload: ByteArray, map: (SftpRawPacket) -> SftpResult, - ): SftpResult { - return when (val result = dispatcher.request(type, payload)) { - is SftpResult.Success -> map(result.value) - is SftpResult.ServerError -> result - is SftpResult.ProtocolError -> result - is SftpResult.IoError -> result - } + ): SftpResult = when (val result = dispatcher.request(type, payload)) { + is SftpResult.Success -> map(result.value) + is SftpResult.ServerError -> result + is SftpResult.ProtocolError -> result + is SftpResult.IoError -> result } /** @@ -409,7 +407,7 @@ internal class SftpClientImpl private constructor( } if (versionPacket.type != SSH_FXP_VERSION) { return SftpResult.ProtocolError( - "Expected SSH_FXP_VERSION (2), got ${versionPacket.type}" + "Expected SSH_FXP_VERSION (2), got ${versionPacket.type}", ) } if (versionPacket.payload.size < 4) { diff --git a/sshlib/src/main/kotlin/org/connectbot/sshlib/client/sftp/SftpDispatcher.kt b/sshlib/src/main/kotlin/org/connectbot/sshlib/client/sftp/SftpDispatcher.kt index 1a0c450..7d67107 100644 --- a/sshlib/src/main/kotlin/org/connectbot/sshlib/client/sftp/SftpDispatcher.kt +++ b/sshlib/src/main/kotlin/org/connectbot/sshlib/client/sftp/SftpDispatcher.kt @@ -94,10 +94,8 @@ internal class SftpDispatcher(private val packetIO: SftpPacketIO) { * Send an SFTP packet without a request ID (used for INIT). Just * forwards the framing-layer result. */ - suspend fun writeRaw(type: Int, payload: ByteArray): SftpResult { - return writeMutex.withLock { - packetIO.writePacket(type, payload) - } + suspend fun writeRaw(type: Int, payload: ByteArray): SftpResult = writeMutex.withLock { + packetIO.writePacket(type, payload) } /** @@ -115,12 +113,14 @@ internal class SftpDispatcher(private val packetIO: SftpPacketIO) { loop@ while (true) { val packet = when (val result = packetIO.readPacket()) { is SftpResult.Success -> result.value + is SftpResult.IoError -> { logger.debug("SFTP read loop ended: {}", result.cause.message) pending.values.forEach { it.completeExceptionally(result.cause) } pending.clear() break@loop } + is SftpResult.ProtocolError -> { logger.debug("SFTP channel closed: {}", result.message) val err = SftpProtocolException(result.message) @@ -128,6 +128,7 @@ internal class SftpDispatcher(private val packetIO: SftpPacketIO) { pending.clear() break@loop } + is SftpResult.ServerError -> { // Framing layer never produces ServerError, but the // sealed exhaustiveness check requires this branch. diff --git a/sshlib/src/main/kotlin/org/connectbot/sshlib/client/sftp/SftpFileAttributes.kt b/sshlib/src/main/kotlin/org/connectbot/sshlib/client/sftp/SftpFileAttributes.kt index c113988..683f3c2 100644 --- a/sshlib/src/main/kotlin/org/connectbot/sshlib/client/sftp/SftpFileAttributes.kt +++ b/sshlib/src/main/kotlin/org/connectbot/sshlib/client/sftp/SftpFileAttributes.kt @@ -68,7 +68,7 @@ internal object SftpFileAttributes { gid = gid, permissions = permissions, atime = atime, - mtime = mtime + mtime = mtime, ) } diff --git a/sshlib/src/main/kotlin/org/connectbot/sshlib/client/sftp/SftpPacketIO.kt b/sshlib/src/main/kotlin/org/connectbot/sshlib/client/sftp/SftpPacketIO.kt index 16627cf..673e988 100644 --- a/sshlib/src/main/kotlin/org/connectbot/sshlib/client/sftp/SftpPacketIO.kt +++ b/sshlib/src/main/kotlin/org/connectbot/sshlib/client/sftp/SftpPacketIO.kt @@ -72,18 +72,16 @@ internal class SftpPacketIO(private val session: SshSession) { * Returns [SftpResult.Success] on send or [SftpResult.IoError] if the * underlying SSH session write fails. */ - suspend fun writePacket(type: Int, payload: ByteArray): SftpResult { - return try { - val length = 1 + payload.size // type byte + payload - val packet = ByteBuffer.allocate(4 + length) - packet.putInt(length) - packet.put(type.toByte()) - packet.put(payload) - session.write(packet.array()) - SftpResult.Success(Unit) - } catch (e: Exception) { - SftpResult.IoError(e) - } + suspend fun writePacket(type: Int, payload: ByteArray): SftpResult = try { + val length = 1 + payload.size // type byte + payload + val packet = ByteBuffer.allocate(4 + length) + packet.putInt(length) + packet.put(type.toByte()) + packet.put(payload) + session.write(packet.array()) + SftpResult.Success(Unit) + } catch (e: Exception) { + SftpResult.IoError(e) } /** diff --git a/sshlib/src/test/kotlin/org/connectbot/sshlib/client/sftp/SftpClientIntegrationTest.kt b/sshlib/src/test/kotlin/org/connectbot/sshlib/client/sftp/SftpClientIntegrationTest.kt index 3a40f40..99bf76b 100644 --- a/sshlib/src/test/kotlin/org/connectbot/sshlib/client/sftp/SftpClientIntegrationTest.kt +++ b/sshlib/src/test/kotlin/org/connectbot/sshlib/client/sftp/SftpClientIntegrationTest.kt @@ -59,12 +59,12 @@ class SftpClientIntegrationTest { ImageFromDockerfile("openssh-sftp-test", false) .withFileFromClasspath(".", "openssh-server") .withBuildArg("OPENSSH_VERSION", OPENSSH_VERSION) - .withBuildArg("DEBUG_CFLAGS", DEBUG_CFLAGS) + .withBuildArg("DEBUG_CFLAGS", DEBUG_CFLAGS), ) .withExposedPorts(22) .withLogConsumer(logConsumer) .waitingFor( - Wait.forLogMessage(".*Server listening.*", 1) + Wait.forLogMessage(".*Server listening.*", 1), ) } @@ -140,7 +140,7 @@ class SftpClientIntegrationTest { // Create and write val handle = sftp.open( testPath, - setOf(SftpOpenFlag.WRITE, SftpOpenFlag.CREATE, SftpOpenFlag.TRUNCATE) + setOf(SftpOpenFlag.WRITE, SftpOpenFlag.CREATE, SftpOpenFlag.TRUNCATE), ).getOrThrow() sftp.write(handle, 0, testData).getOrThrow() sftp.close(handle).getOrThrow() @@ -261,7 +261,7 @@ class SftpClientIntegrationTest { val writeHandle = sftp.open( testPath, - setOf(SftpOpenFlag.WRITE, SftpOpenFlag.CREATE, SftpOpenFlag.TRUNCATE) + setOf(SftpOpenFlag.WRITE, SftpOpenFlag.CREATE, SftpOpenFlag.TRUNCATE), ).getOrThrow() sftp.write(writeHandle, 0, testData).getOrThrow() sftp.close(writeHandle).getOrThrow() @@ -280,7 +280,7 @@ class SftpClientIntegrationTest { assertEquals(testData.size, readBuffer.size, "Should read back all bytes") assertTrue( testData.contentEquals(readBuffer.toByteArray()), - "Read data should match written data" + "Read data should match written data", ) sftp.remove(testPath).getOrThrow() @@ -306,7 +306,7 @@ class SftpClientIntegrationTest { assertEquals( 0b110_100_100, (attrs.permissions ?: 0) and 0x1FF, - "Permissions should be 0644" + "Permissions should be 0644", ) sftp.remove(testPath).getOrThrow() From f5a122679a1708ffbcb635d6b055b487bf613a25 Mon Sep 17 00:00:00 2001 From: GlassOnTin Date: Sat, 25 Apr 2026 21:56:11 +0100 Subject: [PATCH 08/10] =?UTF-8?q?Fix=20SFTP=20integration=20test=20OPENSSH?= =?UTF-8?q?=5FVERSION=20(tag=20name=20=E2=86=92=20tarball=20name)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The test was passing `V_9_9_P2` (the github-tag format) as the build arg to Dockerfile, which used it verbatim in `wget openssh-${VERSION}.tar.gz`. URL doesn't exist → docker build fails → testcontainers reports `initializationError`. The renovate annotation in the Dockerfile already documents the mapping (`V_9_9_P2` → `9.9p2`) but no code did the conversion. Pass the tarball name directly. Renovate bot can be configured to write either form via its `extractVersion` setting if it needs the tag form for tracking. This is what's been failing the PR's CI ever since the original push (712dd04, 2026-04-06) — not anything to do with the sealed-result review feedback. --- .../sshlib/client/sftp/SftpClientIntegrationTest.kt | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/sshlib/src/test/kotlin/org/connectbot/sshlib/client/sftp/SftpClientIntegrationTest.kt b/sshlib/src/test/kotlin/org/connectbot/sshlib/client/sftp/SftpClientIntegrationTest.kt index 99bf76b..1f397c5 100644 --- a/sshlib/src/test/kotlin/org/connectbot/sshlib/client/sftp/SftpClientIntegrationTest.kt +++ b/sshlib/src/test/kotlin/org/connectbot/sshlib/client/sftp/SftpClientIntegrationTest.kt @@ -50,7 +50,11 @@ class SftpClientIntegrationTest { private const val USERNAME = "testuser" private const val PASSWORD = "testpass" - private const val OPENSSH_VERSION = "V_9_9_P2" + + // Tarball-name format expected by wget in the test Dockerfile + // (`openssh-${OPENSSH_VERSION}.tar.gz`). The renovate annotation + // there extracts this from the github tag `V_9_9_P2`. + private const val OPENSSH_VERSION = "9.9p2" private const val DEBUG_CFLAGS = "" @Container From 4ac6898591527ac9834050b56203f01f551affe0 Mon Sep 17 00:00:00 2001 From: GlassOnTin Date: Sat, 25 Apr 2026 22:12:46 +0100 Subject: [PATCH 09/10] Grow Kaitai serialization buffer on overflow MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The fixed 16 KiB buffer in KaitaiUtils.toByteArray() overflows when SSH_MSG_CHANNEL_DATA carries near-maxPacketSize (32 KiB) of data — exposed by the new SFTP large-file integration test. Start at 16 KiB and double on BufferOverflowException up to a 1 MiB ceiling. Most SSH messages still fit on the first try; only large channel-data writes (SFTP, large port-forward chunks) hit the retry path. --- .../connectbot/sshlib/protocol/KaitaiUtils.kt | 29 +++++++++++++++---- 1 file changed, 24 insertions(+), 5 deletions(-) diff --git a/protocol/src/main/kotlin/org/connectbot/sshlib/protocol/KaitaiUtils.kt b/protocol/src/main/kotlin/org/connectbot/sshlib/protocol/KaitaiUtils.kt index 0f5ff84..e8bb73a 100644 --- a/protocol/src/main/kotlin/org/connectbot/sshlib/protocol/KaitaiUtils.kt +++ b/protocol/src/main/kotlin/org/connectbot/sshlib/protocol/KaitaiUtils.kt @@ -21,12 +21,31 @@ import io.kaitai.struct.KaitaiStruct /** * Serialize a Kaitai struct to a byte array. + * + * Kaitai's [ByteBufferKaitaiStream] is fixed-capacity, so the underlying + * `ByteBuffer.put` throws [java.nio.BufferOverflowException] if the + * pre-allocated buffer is too small. We don't have a cheap way to know + * the encoded size up front, so start at 16 KiB and double on overflow + * until the message fits or we cross [MAX_BUFFER]. Most SSH messages + * encode in well under 16 KiB; this only matters for [SshMsgChannelData] + * carrying near-`maxPacketSize` (32 KiB) of data — e.g. SFTP transfers. */ fun KaitaiStruct.ReadWrite.toByteArray(): ByteArray { _check() - val io = ByteBufferKaitaiStream(1024 * 16) - _write(io) - val size = io.pos() - io.seek(0) - return io.readBytes(size.toLong()) + var capacity = INITIAL_BUFFER + while (true) { + try { + val io = ByteBufferKaitaiStream(capacity) + _write(io) + val size = io.pos() + io.seek(0) + return io.readBytes(size.toLong()) + } catch (_: java.nio.BufferOverflowException) { + if (capacity >= MAX_BUFFER) throw IllegalStateException("Kaitai message exceeds $MAX_BUFFER byte serialization limit") + capacity = minOf(capacity * 2, MAX_BUFFER) + } + } } + +private const val INITIAL_BUFFER = 1024L * 16 +private const val MAX_BUFFER = 1024L * 1024 From 9057e91cffc30315b0761f4f91f80d21ea87ee52 Mon Sep 17 00:00:00 2001 From: GlassOnTin Date: Sun, 26 Apr 2026 12:25:09 +0100 Subject: [PATCH 10/10] Retrigger CI MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Last run had a flaky timing failure on Java 21 (RekeyTest's 'rekey triggered after interval elapses' hit the 5000ms timeout). RekeyTest passes on Java 17 and isn't touched by this PR — same test, same code, different JVM = environmental flake. Empty commit to retrigger and confirm.