Merge origin/main into claude/spelunk-tunnel

Port forwarding took host schema version 8 first, so the Spelunk route moves to 9 and is written after the forwards. A host may carry both.

src/DodoSSH.Client.Domain/HostSecret.cs

@@ -45,6 +45,13 @@ public sealed record HostSecret : IVaultSecret
     /// </remarks>
     public const int MaxPinnedPaths = 32;
 
+    /// <summary>The most port forwards a host may carry.</summary>
+    /// <remarks>
+    /// Each one is a listener opened with every terminal to the host, so the bound is on what one connect
+    /// may open as much as on the payload.
+    /// </remarks>
+    public const int MaxPortForwards = 16;
+
     /// <summary>Display name. The only name this host has anywhere.</summary>
     public required string Label { get; init; }
 
@@ -223,6 +230,15 @@ public sealed record HostSecret : IVaultSecret
     /// </remarks>
     public PinnedPathList PinnedPaths { get; init; } = PinnedPathList.Empty;
 
+    /// <summary>
+    /// The port forwards started with every terminal opened to this host.
+    /// </summary>
+    /// <remarks>
+    /// Here rather than in an item of their own; see <see cref="PortForwardRule"/>. Merged per forward, so two
+    /// machines each adding one both keep theirs, and two editing the same one is a recorded conflict.
+    /// </remarks>
+    public PortForwardList PortForwards { get; init; } = PortForwardList.Empty;
+
     /// <summary>
     /// The group this host is filed under, or null for none.
     /// </summary>
@@ -339,10 +355,32 @@ public sealed record HostSecret : IVaultSecret
         }
 
         return PinnedPathsAreStorable(out reason)
+            && PortForwardsAreStorable(out reason)
             && ReferencesAreStorable(out reason)
             && RouteIsStorable(out reason);
     }
 
+    /// <summary>Checks the forwards saved on this host, each on its own terms and the list as a whole.</summary>
+    private bool PortForwardsAreStorable([NotNullWhen(false)] out string? reason)
+    {
+        if (PortForwards.Count > MaxPortForwards)
+        {
+            reason = $"A host cannot carry more than {MaxPortForwards} port forwards.";
+            return false;
+        }
+
+        foreach (var forward in PortForwards)
+        {
+            if (!forward.TryValidate(out reason))
+            {
+                return false;
+            }
+        }
+
+        reason = null;
+        return true;
+    }
+
     /// <summary>Checks the Spelunk route, if there is one, and that it is the only route.</summary>
     private bool RouteIsStorable([NotNullWhen(false)] out string? reason)
     {

src/DodoSSH.Client.Domain/HostSecretCodec.cs

@@ -112,6 +112,13 @@ public static class HostSecretCodec
     /// </remarks>
     public const int PinnedPathsSchemaVersion = 7;
 
+    /// <summary>The version that introduced <see cref="HostSecret.PortForwards"/>.</summary>
+    /// <remarks>
+    /// Its own version for the reason <see cref="PinnedPathsSchemaVersion"/> gives: a host can carry
+    /// forwards without pinning anything, and should stay editable on a build that knows 7 until it does.
+    /// </remarks>
+    public const int PortForwardsSchemaVersion = 8;
+
     /// <summary>The version that introduced <see cref="HostSecret.Spelunk"/>.</summary>
     /// <remarks>
     /// <para>
@@ -122,10 +129,11 @@ public static class HostSecretCodec
     /// </para>
     /// <para>
     /// Additive, like every version but <see cref="PortInheritSchemaVersion"/>: the route is one more
-    /// property, and a build that does not know it skips it and still reads a valid host.
+    /// property, and a build that does not know it skips it and still reads a valid host. It follows
+    /// <see cref="PortForwardsSchemaVersion"/>, whose forwards reached main first.
     /// </para>
     /// </remarks>
-    public const int SpelunkSchemaVersion = 8;
+    public const int SpelunkSchemaVersion = 9;
 
     /// <summary>The highest schema version this build can write.</summary>
     /// <remarks>
@@ -179,6 +187,10 @@ public static class HostSecretCodec
             // as it did before this field existed.
             PinnedPaths = host.PinnedPaths.Count == 0 ? null : [.. host.PinnedPaths],
 
+            // Last, and null when empty, for the reason both fields above give.
+            PortForwards = host.PortForwards.Count == 0
+                ? null
+                : [.. host.PortForwards.Select(PortForwardDocument.From)],
             // Last of all, and absent for a host that is dialled, so every host without a route encodes
             // exactly as it did before routes existed.
             Spelunk = host.Spelunk is { } route
@@ -264,6 +276,11 @@ public static class HostSecretCodec
             version = Math.Max(version, PinnedPathsSchemaVersion);
         }
 
+        if (host.PortForwards.Count > 0)
+        {
+            version = Math.Max(version, PortForwardsSchemaVersion);
+        }
+
         if (host.Spelunk is not null)
         {
             version = Math.Max(version, SpelunkSchemaVersion);
@@ -326,6 +343,11 @@ public static class HostSecretCodec
             return false;
         }
 
+        if (!TryBuildForwards(parsed.PortForwards, out var forwards))
+        {
+            return false;
+        }
+
         var candidate = new HostSecret
         {
             Label = parsed.Label ?? string.Empty,
@@ -348,6 +370,7 @@ public static class HostSecretCodec
 
             PinnedPaths = PinnedPathList.Create(parsed.PinnedPaths ?? []),
 
+            PortForwards = forwards,
             Spelunk = RouteFrom(parsed.Spelunk),
         };
 
@@ -360,6 +383,45 @@ public static class HostSecretCodec
         return true;
     }
 
+    /// <remarks>
+    /// A missing id, kind or port is a refusal rather than a default. Each would be invented here — a fresh
+    /// id the other machines do not have, a <c>Local</c> nobody chose — and an invented forward is one the
+    /// next save writes back as though somebody had asked for it.
+    /// </remarks>
+    private static bool TryBuildForwards(
+        PortForwardDocument?[]? parsed,
+        [NotNullWhen(true)] out PortForwardList? forwards)
+    {
+        forwards = PortForwardList.Empty;
+
+        if (parsed is null)
+        {
+            return true;
+        }
+
+        var rules = new List<PortForwardRule>(parsed.Length);
+
+        foreach (var entry in parsed)
+        {
+            if (entry?.Id is not { } id || entry.Kind is not { } kind || entry.BindPort is not { } bindPort)
+            {
+                return false;
+            }
+
+            rules.Add(new PortForwardRule
+            {
+                Id = id,
+                Kind = (PortForwardKind)kind,
+                BindPort = bindPort,
+                TargetHost = entry.TargetHost,
+                TargetPort = entry.TargetPort,
+            });
+        }
+
+        forwards = PortForwardList.Create(rules);
+        return true;
+    }
+
     /// <summary>The route as stored, or null when there is none.</summary>
     /// <remarks>
     /// Missing parts come through as empty values rather than making the route vanish, so that
@@ -447,6 +509,9 @@ internal sealed class HostPayloadDocument
     /// </remarks>
     public string[]? PinnedPaths { get; set; }
 
+    /// <remarks>Last, and null rather than <c>[]</c>, for the reasons <see cref="TagIds"/> gives.</remarks>
+    public PortForwardDocument?[]? PortForwards { get; set; }
+
     /// <remarks>
     /// Last, and null for a host that is dialled, for the reason <see cref="TagIds"/> gives. One nested object
     /// rather than three loose properties, because the three are one decision — a server id without its
@@ -465,6 +530,33 @@ internal sealed class SpelunkPayloadDocument
     public Guid? ApiKeyCredentialId { get; set; }
 }
 
+/// <summary>One forward's serialised shape. Nullable throughout, because it models untrusted input.</summary>
+/// <remarks>
+/// The kind is written as its number rather than its name, so renaming a member of
+/// <see cref="PortForwardKind"/> can never change what is on the wire.
+/// </remarks>
+internal sealed class PortForwardDocument
+{
+    public Guid? Id { get; set; }
+
+    public int? Kind { get; set; }
+
+    public int? BindPort { get; set; }
+
+    public string? TargetHost { get; set; }
+
+    public int? TargetPort { get; set; }
+
+    public static PortForwardDocument From(PortForwardRule rule) => new()
+    {
+        Id = rule.Id,
+        Kind = (int)rule.Kind,
+        BindPort = rule.BindPort,
+        TargetHost = rule.TargetHost,
+        TargetPort = rule.TargetPort,
+    };
+}
+
 [JsonSourceGenerationOptions(
     PropertyNamingPolicy = JsonKnownNamingPolicy.CamelCase,
     DefaultIgnoreCondition = JsonIgnoreCondition.WhenWritingNull,

src/DodoSSH.Client.Domain/HostSecretMerge.cs

@@ -99,6 +99,8 @@ public static class HostSecretMerge
             TagIds = MergeTags(ancestor.TagIds, local.TagIds, remote.TagIds, conflicts),
             PinnedPaths = MergePinnedPaths(
                 ancestor.PinnedPaths, local.PinnedPaths, remote.PinnedPaths, conflicts),
+            PortForwards = MergePortForwards(
+                ancestor.PortForwards, local.PortForwards, remote.PortForwards, conflicts),
             RelayEnabled = Field(
                 nameof(HostSecret.RelayEnabled),
                 ancestor.RelayEnabled,
@@ -354,6 +356,46 @@ public static class HostSecretMerge
         return PinnedPathList.Create(merge.Merged.Keys);
     }
 
+    /// <summary>
+    /// Merges the forwards per forward, so two people each adding one both keep theirs.
+    /// </summary>
+    /// <remarks>
+    /// <para>
+    /// Keyed by id and ordered the way <see cref="MergePinnedPaths"/> is: existing forwards, then this
+    /// machine's additions, then the server's.
+    /// </para>
+    /// <para>
+    /// <b>Unlike a tag or a pinned path, a conflict is reachable here.</b> The value under a key is the whole
+    /// rule, so two machines changing one forward differently is a genuine clash. The server's version wins,
+    /// as every scalar's does, and the other is recorded in OpenSSH's notation so the user can read what was
+    /// lost. An edit on one side beats a removal on the other, which is the map merge's standing rule.
+    /// </para>
+    /// </remarks>
+    private static PortForwardList MergePortForwards(
+        PortForwardList ancestor,
+        PortForwardList local,
+        PortForwardList remote,
+        List<HostFieldConflict> conflicts)
+    {
+        var merge = ThreeWayMerge.Map(
+            ancestor.ToIdMap(),
+            local.ToIdMap(),
+            remote.ToIdMap(),
+            EqualityComparer<Guid>.Default);
+
+        foreach (var conflict in merge.Conflicts)
+        {
+            conflicts.Add(new HostFieldConflict(
+                $"{nameof(HostSecret.PortForwards)}[{conflict.Key}]",
+                conflict.DiscardedSide,
+                conflict.Kept?.Notation ?? "removed",
+                conflict.Discarded?.Notation ?? "removed",
+                conflict.DiscardedWasRemoval));
+        }
+
+        return PortForwardList.Create(merge.Merged.Values);
+    }
+
     private static string FormatChain(JumpChain chain) =>
         chain.Count == 0 ? "(none)" : string.Join(" → ", chain);
 }

src/DodoSSH.Client.Domain/PortForwardList.cs

@@ -0,0 +1,140 @@
+using System.Collections;
+using System.Diagnostics.CodeAnalysis;
+
+namespace DodoSSH.Client.Domain;
+
+/// <summary>
+/// The port forwards saved on a host, in the order the user added them.
+/// </summary>
+/// <remarks>
+/// <para>
+/// A dedicated type rather than a list, for the reason <see cref="PinnedPathList"/> gives: a plain list on a
+/// record compares by reference, so every host would read as changed on every sync pass.
+/// </para>
+/// <para>
+/// <b>Ordered and unique by id.</b> Order is how the session sidebar lists them. Uniqueness is by
+/// <see cref="PortForwardRule.Id"/> rather than by value, keeping the first occurrence, because the id is
+/// what the merge keys on — two entries with one id would be one forward the merge could not tell apart.
+/// </para>
+/// <para>
+/// Invalid rules are not rejected at construction, for the reason every collection in this assembly gives:
+/// they arrive from another client's payload, and a throw here would fail the whole sync pass.
+/// <see cref="HostSecret.TryValidate"/> catches them.
+/// </para>
+/// </remarks>
+public sealed class PortForwardList : IReadOnlyList<PortForwardRule>, IEquatable<PortForwardList>
+{
+    private readonly PortForwardRule[] rules;
+    private readonly int hash;
+
+    private PortForwardList(PortForwardRule[] rules)
+    {
+        this.rules = rules;
+        hash = ComputeHash(rules);
+    }
+
+    /// <summary>No forwards.</summary>
+    public static PortForwardList Empty { get; } = new([]);
+
+    /// <inheritdoc />
+    public int Count => rules.Length;
+
+    /// <inheritdoc />
+    public PortForwardRule this[int index] => rules[index];
+
+    /// <summary>Copies a sequence of rules, keeping order and the first rule with any given id.</summary>
+    public static PortForwardList Create(IEnumerable<PortForwardRule> rules)
+    {
+        ArgumentNullException.ThrowIfNull(rules);
+
+        var kept = new List<PortForwardRule>();
+        var seen = new HashSet<Guid>();
+
+        foreach (var rule in rules)
+        {
+            if (rule is null)
+            {
+                throw new ArgumentException("A forward list cannot contain a null rule.", nameof(rules));
+            }
+
+            if (seen.Add(rule.Id))
+            {
+                kept.Add(rule);
+            }
+        }
+
+        return kept.Count == 0 ? Empty : new PortForwardList([.. kept]);
+    }
+
+    /// <summary>
+    /// The list as a map from id to rule, in list order, which is the shape <see cref="ThreeWayMerge.Map"/>
+    /// takes.
+    /// </summary>
+    /// <remarks>
+    /// Built in list order for the reason <see cref="PinnedPathList.ToPathMap"/> is: the merge unions keys
+    /// ancestor first, then local, then remote, so an ordered map is what keeps "existing forwards, then
+    /// additions" without the merge ordering anything itself. Unlike a pinned path, a value here can change
+    /// under its key, so a conflict is reachable: both machines editing the same forward differently.
+    /// </remarks>
+    public IReadOnlyDictionary<Guid, PortForwardRule> ToIdMap()
+    {
+        var map = new Dictionary<Guid, PortForwardRule>(rules.Length);
+
+        foreach (var rule in rules)
+        {
+            map[rule.Id] = rule;
+        }
+
+        return map;
+    }
+
+    /// <inheritdoc />
+    public bool Equals(PortForwardList? other)
+    {
+        if (ReferenceEquals(this, other))
+        {
+            return true;
+        }
+
+        return other is not null
+            && other.hash == hash
+            && rules.AsSpan().SequenceEqual(other.rules);
+    }
+
+    /// <inheritdoc />
+    public override bool Equals(object? obj) => Equals(obj as PortForwardList);
+
+    /// <inheritdoc />
+    public override int GetHashCode() => hash;
+
+    /// <inheritdoc />
+    public IEnumerator<PortForwardRule> GetEnumerator() => ((IEnumerable<PortForwardRule>)rules).GetEnumerator();
+
+    /// <inheritdoc />
+    IEnumerator IEnumerable.GetEnumerator() => rules.GetEnumerator();
+
+    /// <summary>Contents equality, tolerating nulls on either side.</summary>
+    [SuppressMessage(
+        "Usage",
+        "CA2225:Operator overloads have named alternates",
+        Justification = "Equals(PortForwardList) is the named alternate.")]
+    public static bool operator ==(PortForwardList? left, PortForwardList? right) =>
+        left is null ? right is null : left.Equals(right);
+
+    /// <summary>Contents inequality.</summary>
+    public static bool operator !=(PortForwardList? left, PortForwardList? right) => !(left == right);
+
+    private static int ComputeHash(PortForwardRule[] rules)
+    {
+        // Order-sensitive, because order is kept.
+        var accumulator = new HashCode();
+        accumulator.Add(rules.Length);
+
+        foreach (var rule in rules)
+        {
+            accumulator.Add(rule);
+        }
+
+        return accumulator.ToHashCode();
+    }
+}

src/DodoSSH.Client.Domain/PortForwardRule.cs

@@ -0,0 +1,166 @@
+using System.Diagnostics.CodeAnalysis;
+using System.Globalization;
+
+namespace DodoSSH.Client.Domain;
+
+/// <summary>Which way a saved forward carries connections.</summary>
+/// <remarks>
+/// Stored as its number, so the values are part of the payload format and must never be renumbered. The
+/// SSH layer has its own enum with the same three members; the connect path maps one to the other, which
+/// keeps this assembly free of any reference to it.
+/// </remarks>
+public enum PortForwardKind
+{
+    /// <summary>A port on this machine, carried to a target the remote can reach. <c>ssh -L</c>.</summary>
+    Local = 0,
+
+    /// <summary>A port on the remote, carried back to a target this machine can reach. <c>ssh -R</c>.</summary>
+    Remote = 1,
+
+    /// <summary>A SOCKS proxy on this machine, whose connections leave from the remote. <c>ssh -D</c>.</summary>
+    Dynamic = 2,
+}
+
+/// <summary>
+/// A port forward saved on a host, started with every terminal opened to it.
+/// </summary>
+/// <remarks>
+/// <para>
+/// <b>Part of the host's encrypted payload rather than an item of its own</b>, which is what
+/// <c>RelayTicketRequest.PortForwardId</c> has said since the contract was frozen. A forward means nothing
+/// without the host it runs over, it is edited in the host's own editor, and a separate item kind would be a
+/// table, a server kind, a cipher and a repository to buy one thing the host's per-id merge already gives:
+/// two machines each adding a forward both keep theirs. <c>SyncEntityType.PortForward</c> stays reserved,
+/// the way <c>HostTag</c> does for the same reason.
+/// </para>
+/// <para>
+/// <b>There is no bind address.</b> Every listener binds loopback — see <c>SshPortForwardSpec</c> for why a
+/// field another member of the keychain can fill in must not be able to say otherwise.
+/// </para>
+/// <para>
+/// <see cref="Id"/> is what the merge keys on, so editing a forward's port on one machine and its target on
+/// another is one forward changed twice — a conflict the merge records — rather than two forwards.
+/// </para>
+/// </remarks>
+public sealed record PortForwardRule
+{
+    /// <summary>The longest a target host may be, which is the longest a DNS name can be.</summary>
+    public const int MaxTargetHostLength = 253;
+
+    /// <summary>Identifies this forward within its host, for the merge.</summary>
+    public required Guid Id { get; init; }
+
+    /// <summary>Which way it carries connections.</summary>
+    public required PortForwardKind Kind { get; init; }
+
+    /// <summary>
+    /// The port to listen on: on this machine for a local or dynamic forward, on the remote for a remote one.
+    /// </summary>
+    public required int BindPort { get; init; }
+
+    /// <summary>Where each connection goes, or null for a dynamic forward.</summary>
+    /// <remarks>
+    /// Resolved by the remote for a local forward — so it may name a machine only the remote can see — and
+    /// by this machine for a remote one.
+    /// </remarks>
+    public string? TargetHost { get; init; }
+
+    /// <summary>The port on <see cref="TargetHost"/>, or null for a dynamic forward.</summary>
+    public int? TargetPort { get; init; }
+
+    /// <summary>
+    /// The forward in OpenSSH's own notation, which is what anybody who has used one will read fastest.
+    /// </summary>
+    /// <remarks>
+    /// <c>-L 5432:db.internal:5432</c>, <c>-R 8080:localhost:3000</c>, <c>-D 1080</c>. No bind address, because
+    /// there is never one to show.
+    /// </remarks>
+    public string Notation => Kind switch
+    {
+        PortForwardKind.Local => $"-L {Port(BindPort)}:{TargetHost}:{Port(TargetPort)}",
+        PortForwardKind.Remote => $"-R {Port(BindPort)}:{TargetHost}:{Port(TargetPort)}",
+        _ => $"-D {Port(BindPort)}",
+    };
+
+    /// <summary>Checks the fields that must hold before this can be stored.</summary>
+    /// <remarks>
+    /// Deliberately silent about two forwards on one port. Two machines each adding one is exactly what the
+    /// merge produces, and a host that failed validation after a merge could not be written back at all; the
+    /// second listener fails when it starts instead, and says so. The editor refuses the duplicate up front.
+    /// </remarks>
+    public bool TryValidate([NotNullWhen(false)] out string? reason)
+    {
+        if (Id == Guid.Empty)
+        {
+            reason = "A port forward cannot have an empty id.";
+            return false;
+        }
+
+        if (!Enum.IsDefined(Kind))
+        {
+            reason = $"'{(int)Kind}' is not a kind of port forward.";
+            return false;
+        }
+
+        if (BindPort is < 1 or > 65535)
+        {
+            reason = $"A forward's listening port must be between 1 and 65535, not {BindPort}.";
+            return false;
+        }
+
+        if (Kind is PortForwardKind.Dynamic)
+        {
+            if (TargetHost is not null || TargetPort is not null)
+            {
+                reason = "A dynamic forward has no target: each connection through it names its own.";
+                return false;
+            }
+
+            reason = null;
+            return true;
+        }
+
+        return TargetIsStorable(out reason);
+    }
+
+    private static string Port(int? port) => port?.ToString(CultureInfo.InvariantCulture) ?? "?";
+
+    /// <remarks>
+    /// Whitespace and control characters are refused as well as blanks: neither is part of any host name,
+    /// both are what a corrupted payload looks like, and this string is drawn on screen as it stands.
+    /// </remarks>
+    private bool TargetIsStorable([NotNullWhen(false)] out string? reason)
+    {
+        if (string.IsNullOrWhiteSpace(TargetHost))
+        {
+            reason = "A forward needs a host to forward to.";
+            return false;
+        }
+
+        if (TargetHost.Length > MaxTargetHostLength)
+        {
+            reason = $"A forward's target cannot be longer than {MaxTargetHostLength} characters.";
+            return false;
+        }
+
+        foreach (var ch in TargetHost)
+        {
+            if (char.IsControl(ch) || char.IsWhiteSpace(ch))
+            {
+                reason = "A forward's target cannot contain spaces or control characters.";
+                return false;
+            }
+        }
+
+        if (TargetPort is not (>= 1 and <= 65535))
+        {
+            reason = TargetPort is { } port
+                ? $"A forward's target port must be between 1 and 65535, not {port}."
+                : "A forward needs a port to forward to.";
+            return false;
+        }
+
+        reason = null;
+        return true;
+    }
+}