Merge pull request 'SFTP: stop waiting for ever on a host whose login prints text, and draw the file panes' icons whole' (#25) from claude/sftp-connection-icons-5ad752 into main

Reviewed-on: #25

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}" />

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
+          UP is arrow_upward on both panes. The design drew arrow_downward on the host/bucket pane, and that
-          two glyphs exactly rather than picking one "up a directory" icon for both — both buttons do the same
+          was carried over until it was reported as a wrong icon — which it is: both buttons go to the parent
-          thing (go to the parent directory), and the pair reading differently is the design's own choice
+          directory, and a down arrow under the tooltip "Up one directory" says the opposite of what it does.
-          faithfully carried over rather than a functional difference invented to justify it.
         -->
         <!--
           ◆ 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
+          UP is arrow_upward, the same as the local pane's; see that pane's own remark for why it is no longer
-          distinct glyphs; see that pane's own remark on why the pair differs without the actions differing.
+          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">

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)
+        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>

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,

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",
         ]);