From 652f552f0dcce5053c1fda656b2d7c994304d305 Mon Sep 17 00:00:00 2001 From: Scott Hanselman Date: Sun, 26 Apr 2026 23:47:11 -0700 Subject: [PATCH] feat: harden ssh tunnel command state Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- docs/MISSION_CONTROL.md | 3 +- src/OpenClaw.Shared/SshTunnelCommandLine.cs | 46 +++++++++++++++++++ src/OpenClaw.Tray.WinUI/App.xaml.cs | 5 +- .../Services/SshTunnelService.cs | 45 ++++++++---------- tests/OpenClaw.Shared.Tests/ModelsTests.cs | 22 +++++++++ 5 files changed, 91 insertions(+), 30 deletions(-) create mode 100644 src/OpenClaw.Shared/SshTunnelCommandLine.cs diff --git a/docs/MISSION_CONTROL.md b/docs/MISSION_CONTROL.md index 07e0ad5..b2ca31c 100644 --- a/docs/MISSION_CONTROL.md +++ b/docs/MISSION_CONTROL.md @@ -357,12 +357,13 @@ Risk: low. No protocol changes. Deliverables: -- Mac-equivalent SSH options: +- Mac-equivalent SSH options: **implemented for tunnel startup** - `BatchMode=yes` - `ExitOnForwardFailure=yes` - `ServerAliveInterval=15` - `ServerAliveCountMax=3` - `TCPKeepAlive=yes` +- Explicit tunnel states (`NotConfigured`, `Stopped`, `Starting`, `Up`, `Restarting`, `Failed`): **implemented** - Fast-fail detection. - Optional random local port fallback. - WSL detection helper with timeout/cache. diff --git a/src/OpenClaw.Shared/SshTunnelCommandLine.cs b/src/OpenClaw.Shared/SshTunnelCommandLine.cs new file mode 100644 index 0000000..9880bc7 --- /dev/null +++ b/src/OpenClaw.Shared/SshTunnelCommandLine.cs @@ -0,0 +1,46 @@ +using System.Text; +using System.Text.RegularExpressions; + +namespace OpenClaw.Shared; + +public static class SshTunnelCommandLine +{ + private static readonly Regex s_validSshUser = new(@"^[a-zA-Z0-9._-]+$", RegexOptions.Compiled); + private static readonly Regex s_validSshHost = new(@"^[a-zA-Z0-9._-]+$", RegexOptions.Compiled); + + public static string BuildArguments(string user, string host, int remotePort, int localPort) + { + user = user.Trim(); + host = host.Trim(); + + if (!s_validSshUser.IsMatch(user)) + throw new ArgumentException($"SSH user contains invalid characters: '{user}'", nameof(user)); + if (!s_validSshHost.IsMatch(host)) + throw new ArgumentException($"SSH host contains invalid characters: '{host}'", nameof(host)); + ValidatePort(remotePort, nameof(remotePort)); + ValidatePort(localPort, nameof(localPort)); + + var sb = new StringBuilder(); + sb.Append("-o BatchMode=yes "); + sb.Append("-o ExitOnForwardFailure=yes "); + sb.Append("-o ServerAliveInterval=15 "); + sb.Append("-o ServerAliveCountMax=3 "); + sb.Append("-o TCPKeepAlive=yes "); + sb.Append("-N "); + sb.Append("-L "); + sb.Append(localPort); + sb.Append(":127.0.0.1:"); + sb.Append(remotePort); + sb.Append(' '); + sb.Append(user); + sb.Append('@'); + sb.Append(host); + return sb.ToString(); + } + + private static void ValidatePort(int port, string parameterName) + { + if (port is < 1 or > 65535) + throw new ArgumentOutOfRangeException(parameterName, port, "SSH tunnel ports must be between 1 and 65535."); + } +} diff --git a/src/OpenClaw.Tray.WinUI/App.xaml.cs b/src/OpenClaw.Tray.WinUI/App.xaml.cs index ad46a64..e392dfa 100644 --- a/src/OpenClaw.Tray.WinUI/App.xaml.cs +++ b/src/OpenClaw.Tray.WinUI/App.xaml.cs @@ -2013,8 +2013,8 @@ public partial class App : Application var user = string.IsNullOrWhiteSpace(_sshTunnelService?.CurrentUser) ? _settings.SshTunnelUser : _sshTunnelService!.CurrentUser!; - var status = _sshTunnelService?.IsRunning == true - ? TunnelStatus.Up + var status = _sshTunnelService?.Status is TunnelStatus.Up or TunnelStatus.Starting or TunnelStatus.Restarting or TunnelStatus.Failed + ? _sshTunnelService.Status : string.IsNullOrWhiteSpace(_sshTunnelService?.LastError) ? TunnelStatus.Stopped : TunnelStatus.Failed; @@ -2624,6 +2624,7 @@ public partial class App : Application private async void OnSshTunnelExited(object? sender, int exitCode) { Logger.Warn($"SSH tunnel exited unexpectedly (code {exitCode}); restarting in 3s..."); + _sshTunnelService?.MarkRestarting(exitCode); await Task.Delay(3000); if (_sshTunnelService != null && _settings?.UseSshTunnel == true) { diff --git a/src/OpenClaw.Tray.WinUI/Services/SshTunnelService.cs b/src/OpenClaw.Tray.WinUI/Services/SshTunnelService.cs index 4f1a53b..ba1f2e3 100644 --- a/src/OpenClaw.Tray.WinUI/Services/SshTunnelService.cs +++ b/src/OpenClaw.Tray.WinUI/Services/SshTunnelService.cs @@ -1,8 +1,6 @@ using OpenClaw.Shared; using System; using System.Diagnostics; -using System.Text; -using System.Text.RegularExpressions; namespace OpenClawTray.Services; @@ -31,6 +29,13 @@ public sealed class SshTunnelService : IDisposable public int CurrentLocalPort { get; private set; } public DateTime? StartedAtUtc { get; private set; } public string? LastError { get; private set; } + public TunnelStatus Status { get; private set; } = TunnelStatus.NotConfigured; + + public void MarkRestarting(int exitCode) + { + Status = TunnelStatus.Restarting; + LastError = $"SSH tunnel exited unexpectedly with code {exitCode}; restart is scheduled."; + } public void EnsureStarted(SettingsManager settings) { @@ -38,6 +43,7 @@ public sealed class SshTunnelService : IDisposable { Stop(); LastError = null; + Status = TunnelStatus.NotConfigured; return; } @@ -57,10 +63,12 @@ public sealed class SshTunnelService : IDisposable if (IsRunning && string.Equals(_lastSpec, spec, StringComparison.Ordinal)) { + Status = TunnelStatus.Up; return; } Stop(); + Status = TunnelStatus.Starting; StartProcess(user, host, remotePort, localPort); _lastSpec = spec; } @@ -69,6 +77,8 @@ public sealed class SshTunnelService : IDisposable { if (_process == null) { + if (Status != TunnelStatus.NotConfigured) + Status = TunnelStatus.Stopped; return; } @@ -93,6 +103,8 @@ public sealed class SshTunnelService : IDisposable _process = null; _lastSpec = null; StartedAtUtc = null; + if (Status != TunnelStatus.NotConfigured) + Status = TunnelStatus.Stopped; _stopping = false; } } @@ -102,7 +114,7 @@ public sealed class SshTunnelService : IDisposable var psi = new ProcessStartInfo { FileName = "ssh", - Arguments = BuildArguments(user, host, remotePort, localPort), + Arguments = SshTunnelCommandLine.BuildArguments(user, host, remotePort, localPort), UseShellExecute = false, RedirectStandardOutput = true, RedirectStandardError = true, @@ -143,6 +155,7 @@ public sealed class SshTunnelService : IDisposable _logger.Warn($"SSH tunnel exited unexpectedly (code {exitCode})"); LastError = $"SSH tunnel exited unexpectedly with code {exitCode}."; StartedAtUtc = null; + Status = TunnelStatus.Failed; try { process.Dispose(); } catch { } _process = null; _lastSpec = null; @@ -160,6 +173,7 @@ public sealed class SshTunnelService : IDisposable catch (Exception ex) { LastError = ex.Message; + Status = TunnelStatus.Failed; process.Dispose(); throw new InvalidOperationException("Unable to start SSH tunnel process. Ensure OpenSSH client is installed and available in PATH.", ex); } @@ -173,6 +187,7 @@ public sealed class SshTunnelService : IDisposable CurrentLocalPort = localPort; StartedAtUtc = DateTime.UtcNow; LastError = null; + Status = TunnelStatus.Up; _logger.Info($"SSH tunnel started: 127.0.0.1:{localPort} -> 127.0.0.1:{remotePort} via {user}@{host}"); } @@ -180,30 +195,6 @@ public sealed class SshTunnelService : IDisposable private static string BuildSpec(string user, string host, int remotePort, int localPort) => $"{user}@{host}:{localPort}:{remotePort}"; - // Strict validation for SSH user/host to prevent command injection - private static readonly Regex s_validSshUser = new(@"^[a-zA-Z0-9._-]+$", RegexOptions.Compiled); - private static readonly Regex s_validSshHost = new(@"^[a-zA-Z0-9._-]+$", RegexOptions.Compiled); - - private static string BuildArguments(string user, string host, int remotePort, int localPort) - { - if (!s_validSshUser.IsMatch(user)) - throw new ArgumentException($"SSH user contains invalid characters: '{user}'"); - if (!s_validSshHost.IsMatch(host)) - throw new ArgumentException($"SSH host contains invalid characters: '{host}'"); - - var sb = new StringBuilder(); - sb.Append("-N "); - sb.Append("-L "); - sb.Append(localPort); - sb.Append(":127.0.0.1:"); - sb.Append(remotePort); - sb.Append(' '); - sb.Append(user); - sb.Append('@'); - sb.Append(host); - return sb.ToString(); - } - public void Dispose() { Stop(); diff --git a/tests/OpenClaw.Shared.Tests/ModelsTests.cs b/tests/OpenClaw.Shared.Tests/ModelsTests.cs index 093e3ab..8db1f00 100644 --- a/tests/OpenClaw.Shared.Tests/ModelsTests.cs +++ b/tests/OpenClaw.Shared.Tests/ModelsTests.cs @@ -124,6 +124,28 @@ public class AgentActivityTests } } +public class SshTunnelCommandLineTests +{ + [Fact] + public void BuildArguments_UsesMacParitySshOptions() + { + var args = SshTunnelCommandLine.BuildArguments("scott", "mac-mini.local", 18789, 28789); + + Assert.Equal("-o BatchMode=yes -o ExitOnForwardFailure=yes -o ServerAliveInterval=15 -o ServerAliveCountMax=3 -o TCPKeepAlive=yes -N -L 28789:127.0.0.1:18789 scott@mac-mini.local", args); + } + + [Theory] + [InlineData("bad user", "mac-mini", 18789, 28789)] + [InlineData("scott", "mac mini", 18789, 28789)] + [InlineData("scott", "mac-mini", 0, 28789)] + [InlineData("scott", "mac-mini", 18789, 70000)] + public void BuildArguments_RejectsUnsafeInputs(string user, string host, int remotePort, int localPort) + { + Assert.ThrowsAny(() => + SshTunnelCommandLine.BuildArguments(user, host, remotePort, localPort)); + } +} + public class ChannelHealthTests { [Theory]