From d71f057bde060003104a708ec2a23c3ce9238967 Mon Sep 17 00:00:00 2001
From: Jaap-Jan de Wit | DodoTech <jaap-jan@dodotech.dev>
Date: Fri, 16 Oct 2026 15:09:46 +0200
Subject: [PATCH] SFTP: stop waiting for ever on a host whose login prints
 text, and draw the file panes' icons whole
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

SSH.NET 2026.0.0 runs the SFTP handshake synchronously inside ConnectAsync, with an infinite
operation timeout and without reading the cancellation token. A host whose login script prints
anything (an echo in .bashrc) puts text where the first SFTP packet belongs; SSH.NET reads its first
four bytes as a length of about a gigabyte and waits for the rest, so the file browser said
"Connecting…" for ever. The SFTP client now uses the connect timeout as its operation timeout for
the handshake and resets it to infinite once connected, so transfers on slow links are unchanged. A
timeout after authentication becomes a TimeoutException naming the likely cause.

The SSH test server gains a second port whose sessions print a banner before sftp-server, and
SftpSessionTests proves the open fails within a 3 s connect timeout with that message. Without the
fix the test runs into its 30 s guard.

Button.paneicon is a fixed 26x26 but kept Fluent's 8,5,8,6 padding, which left a 10px-wide content
box, so every 15-16px Material icon on the SFTP panes was drawn with its sides cut off. Its padding
is now 0. Rename on the host pane used U+E9AD, an italic T (text formatting); it now uses U+E9A2,
the rename glyph the phone's files screen already uses. The host pane's "Up one directory" drew the
design's arrow_downward; both panes now use arrow_upward.

SSH tests 123/123, layout tests 203/203, app tests 744/749 with the 5 Linux TPM skips.
---
 src/DodoSSH.Client.App/App.axaml              |  4 ++
 .../Views/TransfersScreen.axaml               | 15 +++----
 .../SshNetConnectionFactory.cs                | 45 +++++++++++++++++--
 .../SftpSessionTests.cs                       | 25 +++++++++++
 .../SshServerFixture.cs                       | 23 ++++++++++
 5 files changed, 101 insertions(+), 11 deletions(-)

diff --git a/src/DodoSSH.Client.App/App.axaml b/src/DodoSSH.Client.App/App.axaml
index e36fcd4..c6b4ce5 100644
--- a/src/DodoSSH.Client.App/App.axaml
+++ b/src/DodoSSH.Client.App/App.axaml
@@ -1223,10 +1223,14 @@
       The two buttons in the drawer's header: the overflow menu and the arrow that puts the drawer away.
       Chrome rather than controls — they are the frame around whichever panel is showing, which is why they
       are flat and quiet until the pointer finds them.
+
+      No padding, because the box is fixed: Fluent's own 8,5,8,6 left a glyph ten pixels of width inside
+      twenty-six, and every 15px Material icon on the SFTP panes was drawn with its sides cut off.
     -->
     <Style Selector="Button.paneicon">
       <Setter Property="Width" Value="26" />
       <Setter Property="Height" Value="26" />
+      <Setter Property="Padding" Value="0" />
       <Setter Property="CornerRadius" Value="6" />
       <Setter Property="FontSize" Value="13" />
       <Setter Property="Foreground" Value="{StaticResource TextFaint}" />
diff --git a/src/DodoSSH.Client.App/Views/TransfersScreen.axaml b/src/DodoSSH.Client.App/Views/TransfersScreen.axaml
index 34b3ee6..7a7e0ce 100644
--- a/src/DodoSSH.Client.App/Views/TransfersScreen.axaml
+++ b/src/DodoSSH.Client.App/Views/TransfersScreen.axaml
@@ -209,10 +209,9 @@
           design's, unlike HOST/BUCKET across the arrow column, which keeps its own words for a reason
           recorded where it is drawn.
 
-          UP is arrow_upward here and arrow_downward on the host/bucket pane below, matching the design's own
-          two glyphs exactly rather than picking one "up a directory" icon for both — both buttons do the same
-          thing (go to the parent directory), and the pair reading differently is the design's own choice
-          faithfully carried over rather than a functional difference invented to justify it.
+          UP is arrow_upward on both panes. The design drew arrow_downward on the host/bucket pane, and that
+          was carried over until it was reported as a wrong icon — which it is: both buttons go to the parent
+          directory, and a down arrow under the tooltip "Up one directory" says the opposite of what it does.
         -->
         <!--
           ◆ THE PATH SITS IN THE STAR COLUMN, ALONE, AND THAT PLACEMENT IS LOAD-BEARING.
@@ -398,8 +397,8 @@
           directory tree and a flat namespace with inferred folders — and that reason still holds, so only
           the style moves to the design's 10px tracked label, not the wording.
 
-          UP is arrow_downward here, matching arrow_upward on the local pane above per the design's own two
-          distinct glyphs; see that pane's own remark on why the pair differs without the actions differing.
+          UP is arrow_upward, the same as the local pane's; see that pane's own remark for why it is no longer
+          the design's arrow_downward.
         -->
         <!--
           ◆ THE PATH IS TYPED HERE, and that is what the star column is now holding.
@@ -446,7 +445,7 @@
             <StackPanel Grid.Column="2" Orientation="Horizontal" Spacing="4" Margin="8,0,0,0">
               <Button Classes="paneicon" Command="{Binding RemoteUpCommand}"
                       IsEnabled="{Binding IsConnected}" ToolTip.Tip="Up one directory">
-                <TextBlock FontFamily="{StaticResource IconFont}" FontSize="16" Text="&#xE5DB;" />
+                <TextBlock FontFamily="{StaticResource IconFont}" FontSize="16" Text="&#xE5D8;" />
               </Button>
               <Button Classes="paneicon" Command="{Binding RefreshRemoteCommand}"
                       IsEnabled="{Binding IsConnected}" ToolTip.Tip="Refresh">
@@ -546,7 +545,7 @@
               </Button>
               <Button Classes="paneicon" Command="{Binding RenameRemoteCommand}"
                       IsEnabled="{Binding CanRenameRemote}" ToolTip.Tip="Rename on the host">
-                <TextBlock FontFamily="{StaticResource IconFont}" FontSize="15" Text="&#xE9AD;" />
+                <TextBlock FontFamily="{StaticResource IconFont}" FontSize="15" Text="&#xE9A2;" />
               </Button>
               <Button Classes="paneicon" Command="{Binding MoveRemoteCommand}"
                       IsEnabled="{Binding CanMoveRemote}" ToolTip.Tip="Move to another directory">
diff --git a/src/DodoSSH.Client.Ssh/SshNetConnectionFactory.cs b/src/DodoSSH.Client.Ssh/SshNetConnectionFactory.cs
index d56950f..27f43c6 100644
--- a/src/DodoSSH.Client.Ssh/SshNetConnectionFactory.cs
+++ b/src/DodoSSH.Client.Ssh/SshNetConnectionFactory.cs
@@ -139,20 +139,41 @@ public sealed class SshNetConnectionFactory(IKnownHostStore knownHosts)
         IAsyncDisposable? lifetime,
         CancellationToken cancellationToken)
     {
+        var info = BuildConnectionInfo(request);
+
         // Keep-alive here for the reason it is on the shell client: a transfer connection left open while
         // somebody browses is idle in exactly the same way, and a file listing against a machine that has
         // since rebooted would otherwise wait on a socket nothing will ever answer.
-        var client = new SftpClient(BuildConnectionInfo(request))
+        //
+        // ◆ The operation timeout is the connect timeout for the handshake, and nothing afterwards. SSH.NET's
+        // default is to wait for ever, and its ConnectAsync runs the SFTP handshake — the version exchange
+        // and the home directory lookup — synchronously after the login, where the cancellation token is not
+        // read. A host whose login prints anything into the channel (an echo in .bashrc is the classic) sends
+        // text SSH.NET reads as the length of a packet a gigabyte long, and the file browser then said
+        // "Connecting…" for ever. Reset once connected, so a slow write on a slow link is not cut off.
+        var client = new SftpClient(info)
         {
             BufferSize = SftpBufferSize,
             KeepAliveInterval = KeepAliveInterval,
+            OperationTimeout = info.Timeout,
         };
 
         // No progress for the file-transfer path. The screen that waits on one is the file browser, which
         // reports itself, and a second connection opened behind an already-open shell has nothing the user
         // is watching a step list for.
-        var gate = await ConnectThroughHostKeyGateAsync(client, request, progress: null, cancellationToken)
-            .ConfigureAwait(false);
+        HostKeyGate gate;
+
+        try
+        {
+            gate = await ConnectThroughHostKeyGateAsync(client, request, progress: null, cancellationToken)
+                .ConfigureAwait(false);
+        }
+        catch (SshOperationTimeoutException exception) when (info.IsAuthenticated)
+        {
+            throw SftpHandshakeTimedOut(request, info, exception);
+        }
+
+        client.OperationTimeout = Timeout.InfiniteTimeSpan;
 
         // Read once, here, rather than per call. SftpClient.WorkingDirectory canonicalises against the server
         // on first read, so leaving it to the property would put a round trip behind something that reads
@@ -172,6 +193,24 @@ public sealed class SshNetConnectionFactory(IKnownHostStore knownHosts)
         return new SshNetSftpSession(client, gate.Presented!, home, lifetime);
     }
 
+    /// <summary>Says what a timeout after the login means for a file-transfer connection.</summary>
+    /// <remarks>
+    /// Only after the login: a timeout before it is the network's or the server's, and SSH.NET's own words
+    /// for it are left alone. One after it is the SFTP handshake's — the host let this account in and then
+    /// never answered as a file server — and SSH.NET's "Operation has timed out" would send somebody looking
+    /// at the network for what is nearly always a line of text in a login script on the host.
+    /// </remarks>
+    private static TimeoutException SftpHandshakeTimedOut(
+        SshConnectionRequest request,
+        ConnectionInfo info,
+        SshOperationTimeoutException exception) =>
+        new(
+            $"{request.Host} accepted the login, but its SFTP service did not answer within "
+            + $"{info.Timeout.TotalSeconds:0} seconds. This is usually a login script on the host that "
+            + "prints text (an echo in .bashrc, say): SFTP needs the remote shell to stay silent when "
+            + "it is not interactive.",
+            exception);
+
     /// <summary>
     /// Runs the handshake with host key trust attached, and translates a refusal this factory caused.
     /// </summary>
diff --git a/tests/DodoSSH.Client.Ssh.Tests/SftpSessionTests.cs b/tests/DodoSSH.Client.Ssh.Tests/SftpSessionTests.cs
index 2498f0f..8bf0a85 100644
--- a/tests/DodoSSH.Client.Ssh.Tests/SftpSessionTests.cs
+++ b/tests/DodoSSH.Client.Ssh.Tests/SftpSessionTests.cs
@@ -211,6 +211,31 @@ public sealed class SftpSessionTests(SshServerFixture fixture)
         refusal.Presentation.Fingerprint.ShouldStartWith("SHA256:");
     }
 
+    [Fact]
+    public async Task AHostWhoseLoginPrintsText_FailsWithinTheConnectTimeout_AndSaysWhy()
+    {
+        var knownHosts = new InMemoryKnownHostStore();
+        var factory = new SshNetConnectionFactory(knownHosts);
+
+        var request = Request() with { Port = fixture.ChattyPort, ConnectTimeout = TimeSpan.FromSeconds(3) };
+
+        var unknown = await Should.ThrowAsync<SshHostKeyUnknownException>(async () =>
+            await factory.OpenSftpAsync(request, Token));
+        await knownHosts.TrustAsync(unknown.Presentation, Token);
+
+        // The banner's first four bytes read as the length of a packet about a gigabyte long, and SSH.NET's
+        // own default is to wait for the rest of it for ever — which the file browser drew as "Connecting…"
+        // with no end. The outer bound is only there so that regression fails this test rather than hangs it,
+        // and it is a cancellation rather than a timeout so that it cannot be mistaken for the answer.
+        using var bound = CancellationTokenSource.CreateLinkedTokenSource(Token);
+        bound.CancelAfter(TimeSpan.FromSeconds(30));
+
+        var failure = await Should.ThrowAsync<TimeoutException>(async () =>
+            await factory.OpenSftpAsync(request, Token).WaitAsync(bound.Token));
+
+        failure.Message.ShouldContain("SFTP service did not answer within 3 seconds");
+    }
+
     private SshConnectionRequest Request() => new(
         fixture.Host,
         fixture.Port,
diff --git a/tests/DodoSSH.Client.Ssh.Tests/SshServerFixture.cs b/tests/DodoSSH.Client.Ssh.Tests/SshServerFixture.cs
index a06acfc..c7ca360 100644
--- a/tests/DodoSSH.Client.Ssh.Tests/SshServerFixture.cs
+++ b/tests/DodoSSH.Client.Ssh.Tests/SshServerFixture.cs
@@ -42,6 +42,17 @@ public sealed class SshServerFixture : IAsyncLifetime
 
     private const int SshPort = 2222;
 
+    /// <summary>
+    /// A second port on the same sshd, where every session starts with a line of text before SFTP.
+    /// </summary>
+    /// <remarks>
+    /// What a host with an <c>echo</c> in its login script looks like to a file-transfer client: the text
+    /// arrives where the server's first SFTP packet should be. A port rather than an account because this
+    /// image's sshd runs unprivileged and cannot log in as anybody but <see cref="Username"/>, and a port
+    /// rather than a change to the main one so the shell and SFTP suites keep a server that behaves.
+    /// </remarks>
+    private const int ChattySshPort = 2223;
+
     /// <summary>
     /// How many connections in a row the server has to answer before this fixture calls it ready.
     /// </summary>
@@ -65,6 +76,9 @@ public sealed class SshServerFixture : IAsyncLifetime
     /// <summary>Host port the container's sshd is published on.</summary>
     public ushort Port => container!.GetMappedPublicPort(SshPort);
 
+    /// <summary>Host port of the same sshd whose sessions print a banner first; see <see cref="ChattySshPort"/>.</summary>
+    public ushort ChattyPort => container!.GetMappedPublicPort(ChattySshPort);
+
     /// <summary>Host the container is reachable at.</summary>
     public string Host => container!.Hostname;
 
@@ -83,6 +97,7 @@ public sealed class SshServerFixture : IAsyncLifetime
             .WithEnvironment("SUDO_ACCESS", "false")
             .WithEnvironment("PUBLIC_KEY", ExportOpenSshPublicKey(ClientKey))
             .WithPortBinding(SshPort, assignRandomHostPort: true)
+            .WithPortBinding(ChattySshPort, assignRandomHostPort: true)
 
             // The entrypoint generates host keys, rewrites sshd_config and installs the authorized
             // key before sshd is usable, so a published port is not readiness. Both conditions are
@@ -176,6 +191,10 @@ public sealed class SshServerFixture : IAsyncLifetime
     /// been touched — the same trap as the wrong file, one layer up. Measured from the log, after a version
     /// of this fixture did exactly that and failed twenty-eight tests.
     /// </para>
+    /// <para>
+    /// The <see cref="ChattySshPort"/> block goes last because a <c>Match</c> runs to the end of the file:
+    /// anything appended after it would apply only to that port.
+    /// </para>
     /// </remarks>
     private async Task ReconfigureAsync()
     {
@@ -185,6 +204,10 @@ public sealed class SshServerFixture : IAsyncLifetime
             "sed -i 's/^AllowTcpForwarding no/AllowTcpForwarding yes/' /config/sshd/sshd_config"
                 + " && sed -i 's/^#*MaxStartups .*/MaxStartups 200/' /config/sshd/sshd_config"
                 + " && printf '\\nPerSourcePenalties no\\n' >> /config/sshd/sshd_config"
+                + $" && sed -i 's/^Port {SshPort}$/Port {SshPort}\\nPort {ChattySshPort}/' /config/sshd/sshd_config"
+                + $" && printf '\\nMatch LocalPort {ChattySshPort}\\n"
+                + "    ForceCommand /bin/sh -c \"echo Welcome to the test server; exec /usr/lib/ssh/sftp-server\"\\n'"
+                + " >> /config/sshd/sshd_config"
                 + " && pkill -HUP sshd",
         ]);