Merge pull request 'Stop a reconnect dialling a different machine of the same name' (#17) from claude/reconnect-wrong-server-9cccfb into main

Reviewed-on: #17

src/DodoSSH.Client.Shell/ViewModels/MainWindowViewModel.cs

@@ -3761,7 +3761,7 @@ internal sealed partial class MainWindowViewModel : ObservableObject, IAsyncDisp
             return;
         }
 
-        var tab = new TerminalTabViewModel(e.Label, e.Address);
+        var tab = new TerminalTabViewModel(e.Label, e.Address) { HostId = e.HostId };
         attempts[e.AttemptId] = tab;
 
         AdoptTab(tab);
@@ -3825,6 +3825,7 @@ internal sealed partial class MainWindowViewModel : ObservableObject, IAsyncDisp
             // dropping it would leave a shell running with nothing in the window naming it.
             var adopted = new TerminalTabViewModel(e.SessionId, e.Label, e.Address)
             {
+                HostId = e.HostId,
                 StartedAt = clock.GetUtcNow(),
                 Cipher = NullIfEmpty(e.Cipher),
                 HostKeyAlgorithm = NullIfEmpty(e.HostKeyAlgorithm),
@@ -4036,13 +4037,22 @@ internal sealed partial class MainWindowViewModel : ObservableObject, IAsyncDisp
     }
 
     /// <summary>
-    /// The host this tab was opened against, or null when the keychain has nothing under that name.
+    /// The host this tab was opened against, or null when the keychain no longer holds it.
     /// </summary>
     /// <remarks>
     /// <para>
-    /// Matched on the label, which is the same handle <see cref="RefreshConnectedHosts"/> matches on and for
-    /// the same reason: a tab outlives the vault that opened it, so it cannot hold an entity id that would
-    /// still mean anything after a lock.
+    /// ◆ <b>Matched on the keychain item's id, never on the name.</b> Nothing stops two hosts being called
+    /// the same thing — a team's vault beside a personal one makes it ordinary, and "db" is what people call
+    /// their database server in both — and this is the lookup <see cref="ReconnectTabAsync"/> dials the
+    /// answer to. A name match would take a dropped session on one machine and quietly reopen a shell on
+    /// another, with the tab, the status bar and the address all still naming the first. The id is carried
+    /// from the attempt that opened the tab; see <see cref="TerminalTabViewModel.HostId"/>.
+    /// </para>
+    /// <para>
+    /// <b>It outlives a lock, which is what the name match was here to survive.</b> The id is the item's own
+    /// sync identity rather than a reference to a row object, so a vault that is locked and opened again
+    /// rebuilds its list around the same ids and a tab from before the lock finds its host in it. A host
+    /// renamed since the tab opened is found too.
     /// </para>
     /// <para>
     /// Null is the ordinary answer for two real cases, and both of them are why the reconnect below has to be
@@ -4051,10 +4061,9 @@ internal sealed partial class MainWindowViewModel : ObservableObject, IAsyncDisp
     /// </para>
     /// </remarks>
     private HostRowViewModel? HostFor(TerminalTabViewModel? tab) =>
-        tab is null || Vault is not { } vault
+        tab?.HostId is not { } hostId || Vault is not { } vault
             ? null
-            : vault.Hosts.FirstOrDefault(
-                row => string.Equals(row.Label, tab.Label, StringComparison.Ordinal));
+            : vault.Hosts.FirstOrDefault(row => row.EntityId == hostId);
 
     /// <summary>
     /// Whether the selected tab has stopped and there is something to dial again.
@@ -4382,10 +4391,15 @@ internal sealed partial class MainWindowViewModel : ObservableObject, IAsyncDisp
     /// </summary>
     /// <remarks>
     /// <para>
-    /// Matched on the label, which is what a tab was named after, because that is the only handle the two
-    /// lists share — a tab outlives the vault that opened it, so it cannot hold an entity id that would
-    /// still mean anything after a lock. Two hosts sharing a name would light both dots, which is a smaller
-    /// wrong than a dot that goes dark when the vault is reopened.
+    /// Matched on the keychain item's id, the same handle <see cref="HostFor"/> matches on and for the
+    /// reason given there: two hosts may share a name, and this list is read as "which machines am I on".
+    /// Lighting both dots for a name two machines answer to would say the user has a shell open on a host
+    /// they have never connected to — and it is the same match that decides whose pinned paths the sidebar
+    /// offers, which would then be another host's folders.
+    /// </para>
+    /// <para>
+    /// A tab with no item behind it — a machine reached by typing an address — lights nothing, which is
+    /// correct: there is no row here that it is a session on.
     /// </para>
     /// <para>
     /// <see cref="ActiveTabPinnedPaths"/> is rebuilt here, on the same match, rather than from a subscription
@@ -4408,14 +4422,11 @@ internal sealed partial class MainWindowViewModel : ObservableObject, IAsyncDisp
 
         foreach (var host in vault.Hosts)
         {
-            host.IsConnected = Tabs.Any(
-                tab => tab.IsLive && string.Equals(tab.Label, host.Label, StringComparison.Ordinal));
+            host.IsConnected = Tabs.Any(tab => tab.IsLive && tab.HostId == host.EntityId);
 
             // The active tab's host, and only when it is actually connected: the sidebar is for a session
             // that is open, not for whichever machine's tab happens to be selected while it is still dialling.
-            if (host.IsConnected
-                && SelectedTab is { } selected
-                && string.Equals(host.Label, selected.Label, StringComparison.Ordinal))
+            if (host.IsConnected && SelectedTab is { } selected && selected.HostId == host.EntityId)
             {
                 connectedActiveHost = host;
             }
@@ -4549,15 +4560,14 @@ internal sealed partial class MainWindowViewModel : ObservableObject, IAsyncDisp
     /// is drawn and not what it does. It shares <see cref="OnVaultFilesRequested"/>'s plumbing — the same
     /// <see cref="ShowFiles"/> refusal, the same re-found row, the same password-sheet branch for a host
     /// that cannot be dialled unattended — through <see cref="GoToHostFilesAsync"/>, and the host is found by
-    /// the same label match <see cref="RefreshConnectedHosts"/> used to decide the row is there to click.
+    /// the same <see cref="HostFor"/> match <see cref="RefreshConnectedHosts"/> used to decide the row is
+    /// there to click — which is the point: the pins on screen and the host they are opened on come from one
+    /// answer to "which machine is this tab", and two hosts of the same name cannot pull them apart.
     /// </remarks>
     [RelayCommand]
     private async Task OpenPinnedPathAsync(string path)
     {
-        if (Vault is not { } vault
-            || SelectedTab is not { } tab
-            || vault.Hosts.FirstOrDefault(
-                host => string.Equals(host.Label, tab.Label, StringComparison.Ordinal)) is not { } host)
+        if (HostFor(SelectedTab) is not { } host)
         {
             return;
         }
@@ -4638,10 +4648,7 @@ internal sealed partial class MainWindowViewModel : ObservableObject, IAsyncDisp
     [RelayCommand]
     private void PinFolderFromSidebar()
     {
-        if (Vault is not { } vault
-            || SelectedTab is not { } tab
-            || vault.Hosts.FirstOrDefault(
-                host => string.Equals(host.Label, tab.Label, StringComparison.Ordinal)) is not { } host)
+        if (Vault is not { } vault || HostFor(SelectedTab) is not { } host)
         {
             return;
         }
@@ -4674,10 +4681,7 @@ internal sealed partial class MainWindowViewModel : ObservableObject, IAsyncDisp
     [RelayCommand]
     private async Task SelectFilesHostAsync(TerminalTabViewModel tab)
     {
-        if (Vault is not { } vault
-            || tab is null
-            || vault.Hosts.FirstOrDefault(
-                host => string.Equals(host.Label, tab.Label, StringComparison.Ordinal)) is not { } host)
+        if (HostFor(tab) is not { } host)
         {
             return;
         }

src/DodoSSH.Client.Shell/ViewModels/TerminalTabViewModel.cs

@@ -229,6 +229,31 @@ internal sealed partial class TerminalTabViewModel : ObservableObject
 
     internal string Label { get; }
 
+    /// <summary>
+    /// The keychain item this tab was opened from, or null for a machine reached by typing an address.
+    /// </summary>
+    /// <remarks>
+    /// <para>
+    /// ◆ <b>What the shell matches this tab back to a host row with, because the name will not do it.</b>
+    /// Nothing stops two hosts being called the same thing, and a team's vault beside a personal one makes it
+    /// ordinary — so a lookup on <see cref="Label"/> can answer with a different machine that happens to
+    /// share the name. The reconnect under a stopped tab <em>dials</em> whatever that lookup finds, which is
+    /// the one place being approximately right is dialling a stranger. See
+    /// <c>MainWindowViewModel.HostFor</c>.
+    /// </para>
+    /// <para>
+    /// <b>It survives a lock, which the row objects do not.</b> This is the item's sync identity rather than
+    /// an index into the list on screen — see <c>VaultItem&lt;TSecret&gt;.EntityId</c> — so a vault that is
+    /// locked and opened again rebuilds every row around the same ids, and a tab that outlived the vault
+    /// finds its host again. A rename finds it too, which a name match by definition cannot.
+    /// </para>
+    /// <para>
+    /// Null is not missing data: it is the whole of what makes a typed-in address different, and it is why
+    /// such a tab is honestly refused a reconnect rather than offered one that would dial a namesake.
+    /// </para>
+    /// </remarks>
+    internal Guid? HostId { get; init; }
+
     /// <summary>The account and endpoint, for the pane header and the status bar.</summary>
     internal string Address { get; }
 

src/DodoSSH.Client.Shell/ViewModels/VaultViewModel.cs

@@ -947,6 +947,7 @@ internal static class ItemBadge
 /// <param name="identityLabel">
 /// The display name of the key or credential that authenticated, or null when a typed password did.
 /// </param>
+/// <param name="hostId">The keychain item this was dialled from, or null for a typed-in address.</param>
 internal sealed class TerminalSessionEventArgs(
     Guid attemptId,
     uint sessionId,
@@ -954,7 +955,8 @@ internal sealed class TerminalSessionEventArgs(
     string address,
     string cipher,
     string hostKeyAlgorithm,
-    string? identityLabel) : EventArgs
+    string? identityLabel,
+    Guid? hostId) : EventArgs
 {
     /// <summary>Which attempt this session came out of.</summary>
     /// <inheritdoc cref="ConnectionAttemptEventArgs.AttemptId" path="/remarks" />
@@ -971,19 +973,27 @@ internal sealed class TerminalSessionEventArgs(
     internal string HostKeyAlgorithm { get; } = hostKeyAlgorithm;
 
     internal string? IdentityLabel { get; } = identityLabel;
+
+    /// <inheritdoc cref="ConnectionAttemptEventArgs.HostId" />
+    internal Guid? HostId { get; } = hostId;
 }
 
 /// <summary>A connection that has been asked for, and has not answered yet.</summary>
 /// <param name="attemptId">Identifies this attempt for the whole of its life.</param>
 /// <param name="label">The host's name, as the vault has it.</param>
 /// <param name="address">Who this will be logged in as, and where.</param>
+/// <param name="hostId">The keychain item this is being dialled from, or null for a typed-in address.</param>
 /// <remarks>
 /// The vault says a connection has started before it says whether it worked, so that the shell can put a
 /// tab in the strip at the moment the user asks for one rather than however many seconds later a handshake
 /// takes. Everything a tab needs to name itself is here, because the name is a decrypted item and the shell
 /// has no vault to read it from.
 /// </remarks>
-internal sealed class ConnectionAttemptEventArgs(Guid attemptId, string label, string address) : EventArgs
+internal sealed class ConnectionAttemptEventArgs(
+    Guid attemptId,
+    string label,
+    string address,
+    Guid? hostId) : EventArgs
 {
     /// <summary>Identifies this attempt for the whole of its life.</summary>
     /// <remarks>
@@ -995,6 +1005,19 @@ internal sealed class ConnectionAttemptEventArgs(Guid attemptId, string label, s
     internal string Label { get; } = label;
 
     internal string Address { get; } = address;
+
+    /// <summary>
+    /// The keychain item this connection was made from, or null for a machine that is not in the keychain.
+    /// </summary>
+    /// <remarks>
+    /// ◆ <b>The name is not an identifier and this is.</b> Two hosts may be called the same thing — nothing
+    /// stops it, and a team vault next to a personal one makes it ordinary — so a tab that remembered only
+    /// what it was called could be matched back to the wrong machine. That is not a cosmetic mix-up: the
+    /// reconnect below the tab dials whichever row the match found, so the one thing that must not be
+    /// guessed is which machine a reconnect goes to. Carried from here because a tab has no vault to ask —
+    /// see <c>TerminalTabViewModel.HostId</c>.
+    /// </remarks>
+    internal Guid? HostId { get; } = hostId;
 }
 
 /// <summary>A connection that has got as far as a named step.</summary>
@@ -11326,7 +11349,8 @@ internal sealed partial class VaultViewModel(
         var attempt = new ConnectionAttemptEventArgs(
             Guid.CreateVersion7(),
             target.Label,
-            Dialled(target, authentication));
+            Dialled(target, authentication),
+            target.HostId);
 
         ConnectionStarting?.Invoke(this, attempt);
         Status = $"Connecting to {target.Label}…";
@@ -11784,7 +11808,8 @@ internal sealed partial class VaultViewModel(
                 Dialled(target, authentication),
                 facts?.Cipher ?? string.Empty,
                 facts?.HostKeyAlgorithm ?? string.Empty,
-                authentication.IdentityLabel));
+                authentication.IdentityLabel,
+                target.HostId));
 
         // Last, and after the tab exists: keeping the password is a favour, and the session the user asked
         // for must not wait on a vault write to appear.

tests/DodoSSH.Client.App.Tests/ShellFlowTests.cs

@@ -2099,6 +2099,82 @@ public sealed partial class ShellFlowTests : IAsyncLifetime
             "the session that was replaced should have been released with its pane");
     }
 
+    /// <remarks>
+    /// <para>
+    /// ◆ THE ONE THIS EXISTS FOR: a reconnect that dialled a different machine of the same name. Nothing in
+    /// this application stops two hosts being called the same thing — a team's vault beside a personal one
+    /// makes it ordinary, and "prod-db" is what a database server gets called in both — and a tab used to
+    /// find its host by that name. A dropped session on one machine then came back as a live shell on the
+    /// other, with the tab, the address and the status bar all still naming the first.
+    /// </para>
+    /// <para>
+    /// Asserted on the address actually dialled rather than on anything the tab says about itself. The two
+    /// rows agree on every string a tab carries, which is exactly why this went unnoticed: a test written
+    /// against the label would have passed on the day the wrong machine was being opened.
+    /// </para>
+    /// </remarks>
+    [Fact]
+    public async Task ReconnectingATabWhoseHostHasANamesake_DialsTheMachineTheTabWasOpenedOn()
+    {
+        var vault = await ReadyToConnectAsync();
+
+        await AddKeyAsync(vault, "deploy");
+
+        // A second host under the name the fixture's own already has, on a different machine.
+        await AddHostAsync(vault, "prod-db", "db-replica.internal");
+
+        // Both bound to the key, so neither is diverted to the password sheet: what this asserts is which of
+        // the two gets dialled, not whether either can be. Re-found between the two, because saving a host
+        // rebuilds the list.
+        await BindKeyAsync(vault, Machine(vault, "db.internal"), vault.Keys[0].EntityId);
+        await BindKeyAsync(vault, Machine(vault, "db-replica.internal"), vault.Keys[0].EntityId);
+
+        await using var renderer = await FakeRenderer.AttachAsync(workspace, Token);
+        await vault.ConnectToRowCommand.ExecuteAsync(Machine(vault, "db-replica.internal"));
+
+        var tab = shell.Tabs.ShouldHaveSingleItem();
+        ssh.Requests.ShouldHaveSingleItem().Host.ShouldBe("db-replica.internal");
+
+        ssh.Shells[0].Drop("An established connection was aborted by the server.");
+        await EventuallyAsync(() => !tab.IsLive, "the dropped shell should have ended the session");
+
+        shell.CanReconnectSelectedTab.ShouldBeTrue("the machine this tab was opened on is still there");
+
+        await shell.ReconnectTabCommand.ExecuteAsync(tab);
+
+        tab.IsLive.ShouldBeTrue(tab.Status);
+
+        ssh.Requests.Count.ShouldBe(2);
+        ssh.Requests[1].Host.ShouldBe(
+            "db-replica.internal",
+            "a reconnect dials the machine the tab was opened on, not the one that shares its name");
+    }
+
+    /// <remarks>
+    /// The same mix-up as the test above, where it is read rather than dialled. A green dot is the host list
+    /// answering "am I on this machine", and the name two rows share is not an answer to it — a dot on the
+    /// machine nobody has a shell on claims a session that does not exist, and takes QUICK ACCESS's pins with
+    /// it, since the sidebar offers the pinned folders of whichever row that same match found.
+    /// </remarks>
+    [Fact]
+    public async Task ConnectingToOneOfTwoHostsOfTheSameName_LightsThatOneAlone()
+    {
+        var vault = await ReadyToConnectAsync();
+
+        await AddHostAsync(vault, "prod-db", "db-replica.internal");
+
+        await using var renderer = await FakeRenderer.AttachAsync(workspace, Token);
+
+        vault.SelectedHost = Machine(vault, "db-replica.internal");
+        await vault.ConnectCommand.ExecuteAsync(null);
+
+        shell.Tabs.ShouldHaveSingleItem().IsLive.ShouldBeTrue();
+
+        Machine(vault, "db-replica.internal").IsConnected.ShouldBeTrue();
+        Machine(vault, "db.internal").IsConnected.ShouldBeFalse(
+            "nobody has a shell on this machine; only its name was connected to");
+    }
+
     /// <remarks>
     /// A connection that was refused is worth retrying for the same reason a dropped one is, and the
     /// commonest refusal — a machine that was not up yet — is the one most likely to succeed a minute later.
@@ -10377,11 +10453,14 @@ public sealed partial class ShellFlowTests : IAsyncLifetime
         await vault.SaveHostCommand.ExecuteAsync(null);
     }
 
-    private static async Task AddHostAsync(VaultViewModel vault, string label)
+    private static async Task AddHostAsync(
+        VaultViewModel vault,
+        string label,
+        string hostname = "db.internal")
     {
         vault.NewHostCommand.Execute(null);
         vault.EditorLabel = label;
-        vault.EditorHostname = "db.internal";
+        vault.EditorHostname = hostname;
         vault.EditorUsername = "deploy";
 
         await vault.SaveHostCommand.ExecuteAsync(null);
@@ -10405,6 +10484,14 @@ public sealed partial class ShellFlowTests : IAsyncLifetime
     private static HostRowViewModel Host(VaultViewModel vault, string label) =>
         vault.Hosts.Single(row => string.Equals(row.Label, label, StringComparison.Ordinal));
 
+    /// <summary>The host row for a given machine, for the tests whose rows deliberately share a name.</summary>
+    /// <remarks>
+    /// <see cref="Host"/> cannot be asked when two rows are called the same thing, which is the whole
+    /// situation those tests set up. Re-found rather than held, because saving any host rebuilds the list.
+    /// </remarks>
+    private static HostRowViewModel Machine(VaultViewModel vault, string hostname) =>
+        vault.Hosts.Single(row => string.Equals(row.Host.Hostname, hostname, StringComparison.Ordinal));
+
     /// <summary>Deletes the selected host: the question, and then the answer to it.</summary>
     /// <remarks>
     /// Both halves, because both are what deleting anything now takes — arming on its own changes nothing,