From 2d95366552b387445a8fa8b5440d0bc9b97c389e Mon Sep 17 00:00:00 2001 From: Jaap-Jan de Wit | DodoTech Date: Mon, 14 Sep 2026 10:18:58 +0200 Subject: [PATCH] Stop a reconnect dialling a different machine of the same name MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A tab remembered what its host was called and nothing else, and the reconnect under a stopped tab looked the host up by that name: HostFor took the first row in the keychain whose label matched. 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 the database server gets called in both — so the button dialled whichever namesake sat earliest in the list. A dropped session on one machine came back as a live shell on another, with the tab, the address and the status bar all still naming the first. There was nothing on screen to read the substitution off. The name match was not an oversight; it was an argument, written into the remark on HostFor and repeated on RefreshConnectedHosts: a tab outlives the vault that opened it, so it "cannot hold an entity id that would still mean anything after a lock". That is not so. EntityId 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 every row around the same ids. The one handle that was rejected as too fragile to survive a lock is the handle that survives it. So the id now travels with the connection. ConnectionAttemptEventArgs and TerminalSessionEventArgs carry it, taken from ConnectionTarget.HostId, which the connect path has had all along for the log; TerminalTabViewModel keeps it; and HostFor matches on it. A host renamed since the tab opened is found too, which a name match by definition cannot. 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 refused a reconnect rather than offered one that would dial a stranger sharing its name. Three other lookups had grown their own copy of the same match and so had the same fault, where it is read rather than dialled: the green connected dot, which lit on every machine answering to the name and claimed a shell on hosts nobody had connected to; the pinned paths QUICK ACCESS offers, which come from whichever row that same match found; and the sidebar's "Pin folder" and the SFTP tab row, which would have edited and browsed the wrong host. All four now go through HostFor, so "which machine is this tab" has one answer. Both new tests assert 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. --- .../ViewModels/MainWindowViewModel.cs | 64 +++++++------ .../ViewModels/TerminalTabViewModel.cs | 25 +++++ .../ViewModels/VaultViewModel.cs | 33 ++++++- .../ShellFlowTests.cs | 91 ++++++++++++++++++- 4 files changed, 177 insertions(+), 36 deletions(-) diff --git a/src/DodoSSH.Client.Shell/ViewModels/MainWindowViewModel.cs b/src/DodoSSH.Client.Shell/ViewModels/MainWindowViewModel.cs index bc345f1..afda117 100644 --- a/src/DodoSSH.Client.Shell/ViewModels/MainWindowViewModel.cs +++ b/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 } /// - /// 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. /// /// /// - /// Matched on the label, which is the same handle 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. + /// ◆ Matched on the keychain item's id, never on the name. 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 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 . + /// + /// + /// It outlives a lock, which is what the name match was here to survive. 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. /// /// /// 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 /// /// 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); /// /// Whether the selected tab has stopped and there is something to dial again. @@ -4382,10 +4391,15 @@ internal sealed partial class MainWindowViewModel : ObservableObject, IAsyncDisp /// /// /// - /// 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 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. + /// + /// + /// 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. /// /// /// 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 's plumbing — the same /// refusal, the same re-found row, the same password-sheet branch for a host /// that cannot be dialled unattended — through , and the host is found by - /// the same label match used to decide the row is there to click. + /// the same match 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. /// [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; } diff --git a/src/DodoSSH.Client.Shell/ViewModels/TerminalTabViewModel.cs b/src/DodoSSH.Client.Shell/ViewModels/TerminalTabViewModel.cs index c415559..b99c28e 100644 --- a/src/DodoSSH.Client.Shell/ViewModels/TerminalTabViewModel.cs +++ b/src/DodoSSH.Client.Shell/ViewModels/TerminalTabViewModel.cs @@ -229,6 +229,31 @@ internal sealed partial class TerminalTabViewModel : ObservableObject internal string Label { get; } + /// + /// The keychain item this tab was opened from, or null for a machine reached by typing an address. + /// + /// + /// + /// ◆ What the shell matches this tab back to a host row with, because the name will not do it. + /// 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 can answer with a different machine that happens to + /// share the name. The reconnect under a stopped tab dials whatever that lookup finds, which is + /// the one place being approximately right is dialling a stranger. See + /// MainWindowViewModel.HostFor. + /// + /// + /// It survives a lock, which the row objects do not. This is the item's sync identity rather than + /// an index into the list on screen — see VaultItem<TSecret>.EntityId — 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. + /// + /// + /// 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. + /// + /// + internal Guid? HostId { get; init; } + /// The account and endpoint, for the pane header and the status bar. internal string Address { get; } diff --git a/src/DodoSSH.Client.Shell/ViewModels/VaultViewModel.cs b/src/DodoSSH.Client.Shell/ViewModels/VaultViewModel.cs index d75c868..aa38bdc 100644 --- a/src/DodoSSH.Client.Shell/ViewModels/VaultViewModel.cs +++ b/src/DodoSSH.Client.Shell/ViewModels/VaultViewModel.cs @@ -947,6 +947,7 @@ internal static class ItemBadge /// /// The display name of the key or credential that authenticated, or null when a typed password did. /// +/// The keychain item this was dialled from, or null for a typed-in address. 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 { /// Which attempt this session came out of. /// @@ -971,19 +973,27 @@ internal sealed class TerminalSessionEventArgs( internal string HostKeyAlgorithm { get; } = hostKeyAlgorithm; internal string? IdentityLabel { get; } = identityLabel; + + /// + internal Guid? HostId { get; } = hostId; } /// A connection that has been asked for, and has not answered yet. /// Identifies this attempt for the whole of its life. /// The host's name, as the vault has it. /// Who this will be logged in as, and where. +/// The keychain item this is being dialled from, or null for a typed-in address. /// /// 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. /// -internal sealed class ConnectionAttemptEventArgs(Guid attemptId, string label, string address) : EventArgs +internal sealed class ConnectionAttemptEventArgs( + Guid attemptId, + string label, + string address, + Guid? hostId) : EventArgs { /// Identifies this attempt for the whole of its life. /// @@ -995,6 +1005,19 @@ internal sealed class ConnectionAttemptEventArgs(Guid attemptId, string label, s internal string Label { get; } = label; internal string Address { get; } = address; + + /// + /// The keychain item this connection was made from, or null for a machine that is not in the keychain. + /// + /// + /// ◆ The name is not an identifier and this is. 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 TerminalTabViewModel.HostId. + /// + internal Guid? HostId { get; } = hostId; } /// A connection that has got as far as a named step. @@ -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. diff --git a/tests/DodoSSH.Client.App.Tests/ShellFlowTests.cs b/tests/DodoSSH.Client.App.Tests/ShellFlowTests.cs index 2973a77..564c804 100644 --- a/tests/DodoSSH.Client.App.Tests/ShellFlowTests.cs +++ b/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"); } + /// + /// + /// ◆ 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. + /// + /// + /// 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. + /// + /// + [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"); + } + + /// + /// 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. + /// + [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"); + } + /// /// 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)); + /// The host row for a given machine, for the tests whose rows deliberately share a name. + /// + /// 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. + /// + private static HostRowViewModel Machine(VaultViewModel vault, string hostname) => + vault.Hosts.Single(row => string.Equals(row.Host.Hostname, hostname, StringComparison.Ordinal)); + /// Deletes the selected host: the question, and then the answer to it. /// /// Both halves, because both are what deleting anything now takes — arming on its own changes nothing,