From 86775e3665c220272a5bce9532e07cc87fda9883 Mon Sep 17 00:00:00 2001 From: Dean Sheather Date: Tue, 29 Jul 2025 22:01:58 +1000 Subject: [PATCH 1/2] Revert "fix: opt-in tailscale vpn loop prevention (#148)" This reverts commit 814b412756dcb4b81c38bc0e2b3c750e51f435e5. --- App/App.xaml.cs | 2 +- App/Models/Settings.cs | 13 ++--------- App/Services/CredentialManager.cs | 10 +++++---- App/Services/RpcController.cs | 6 +----- App/ViewModels/SettingsViewModel.cs | 21 +----------------- App/Views/Pages/SettingsMainPage.xaml | 6 ------ App/Views/SettingsWindow.xaml | 4 ++-- App/Views/TrayWindow.xaml.cs | 24 +++++++++++++++------ Tests.App/Services/CredentialManagerTest.cs | 7 ++---- Vpn.Proto/vpn.proto | 3 +-- 10 files changed, 34 insertions(+), 62 deletions(-) diff --git a/App/App.xaml.cs b/App/App.xaml.cs index 40c0f26..a07af43 100644 --- a/App/App.xaml.cs +++ b/App/App.xaml.cs @@ -92,7 +92,6 @@ public App() services.AddSingleton(_ => this); services.AddSingleton(_ => this); - services.AddSingleton, SettingsManager>(); services.AddSingleton(_ => new WindowsCredentialBackend(WindowsCredentialBackend.CoderCredentialsTargetName)); services.AddSingleton(); @@ -121,6 +120,7 @@ public App() // FileSyncListMainPage is created by FileSyncListWindow. services.AddTransient(); + services.AddSingleton, SettingsManager>(); services.AddSingleton(); // SettingsWindow views and view models services.AddTransient(); diff --git a/App/Models/Settings.cs b/App/Models/Settings.cs index f1c89ce..ec4c61b 100644 --- a/App/Models/Settings.cs +++ b/App/Models/Settings.cs @@ -34,11 +34,6 @@ public class CoderConnectSettings : ISettings /// public bool ConnectOnLaunch { get; set; } - /// - /// When this is true Coder Connect will not attempt to protect against Tailscale loopback issues. - /// - public bool EnableCorporateVpnSupport { get; set; } - /// /// CoderConnect current settings version. Increment this when the settings schema changes. /// In future iterations we will be able to handle migrations when the user has @@ -51,21 +46,17 @@ public CoderConnectSettings() Version = VERSION; ConnectOnLaunch = false; - - EnableCorporateVpnSupport = false; } - public CoderConnectSettings(int? version, bool connectOnLaunch, bool enableCorporateVpnSupport) + public CoderConnectSettings(int? version, bool connectOnLaunch) { Version = version ?? VERSION; ConnectOnLaunch = connectOnLaunch; - - EnableCorporateVpnSupport = enableCorporateVpnSupport; } public CoderConnectSettings Clone() { - return new CoderConnectSettings(Version, ConnectOnLaunch, EnableCorporateVpnSupport); + return new CoderConnectSettings(Version, ConnectOnLaunch); } } diff --git a/App/Services/CredentialManager.cs b/App/Services/CredentialManager.cs index c8030c3..6868ae7 100644 --- a/App/Services/CredentialManager.cs +++ b/App/Services/CredentialManager.cs @@ -223,9 +223,8 @@ private async Task LoadCredentialsInner(CancellationToken ct) }; } - // Grab the lock again so we can update the state. Don't use the CT - // here as it may have already been canceled. - using (await _opLock.LockAsync(TimeSpan.FromSeconds(5), CancellationToken.None)) + // Grab the lock again so we can update the state. + using (await _opLock.LockAsync(ct)) { // Prevent new LoadCredentials calls from returning this task. if (_loadCts != null) @@ -243,8 +242,11 @@ private async Task LoadCredentialsInner(CancellationToken ct) if (latestCreds is not null) return latestCreds; } - UpdateState(model); + // If there aren't any latest credentials after a cancellation, we + // most likely timed out and should throw. ct.ThrowIfCancellationRequested(); + + UpdateState(model); return model; } } diff --git a/App/Services/RpcController.cs b/App/Services/RpcController.cs index f7abe79..532c878 100644 --- a/App/Services/RpcController.cs +++ b/App/Services/RpcController.cs @@ -69,7 +69,6 @@ public interface IRpcController : IAsyncDisposable public class RpcController : IRpcController { private readonly ICredentialManager _credentialManager; - private readonly ISettingsManager _settingsManager; private readonly RaiiSemaphoreSlim _operationLock = new(1, 1); private Speaker? _speaker; @@ -77,10 +76,9 @@ public class RpcController : IRpcController private readonly RaiiSemaphoreSlim _stateLock = new(1, 1); private readonly RpcModel _state = new(); - public RpcController(ICredentialManager credentialManager, ISettingsManager settingsManager) + public RpcController(ICredentialManager credentialManager) { _credentialManager = credentialManager; - _settingsManager = settingsManager; } public event EventHandler? StateChanged; @@ -158,7 +156,6 @@ public async Task StartVpn(CancellationToken ct = default) using var _ = await AcquireOperationLockNowAsync(); AssertRpcConnected(); - var coderConnectSettings = await _settingsManager.Read(ct); var credentials = _credentialManager.GetCachedCredentials(); if (credentials.State != CredentialState.Valid) throw new RpcOperationException( @@ -178,7 +175,6 @@ public async Task StartVpn(CancellationToken ct = default) { CoderUrl = credentials.CoderUrl?.ToString(), ApiToken = credentials.ApiToken, - TunnelUseSoftNetIsolation = coderConnectSettings.EnableCorporateVpnSupport, }, }, ct); } diff --git a/App/ViewModels/SettingsViewModel.cs b/App/ViewModels/SettingsViewModel.cs index 58b31f2..721ea95 100644 --- a/App/ViewModels/SettingsViewModel.cs +++ b/App/ViewModels/SettingsViewModel.cs @@ -13,9 +13,6 @@ public partial class SettingsViewModel : ObservableObject [ObservableProperty] public partial bool ConnectOnLaunch { get; set; } - [ObservableProperty] - public partial bool DisableTailscaleLoopProtection { get; set; } - [ObservableProperty] public partial bool StartOnLoginDisabled { get; set; } @@ -34,7 +31,6 @@ public SettingsViewModel(ILogger logger, ISettingsManager logger, ISettingsManager - - - diff --git a/App/Views/SettingsWindow.xaml b/App/Views/SettingsWindow.xaml index a2fe886..512f0f5 100644 --- a/App/Views/SettingsWindow.xaml +++ b/App/Views/SettingsWindow.xaml @@ -9,8 +9,8 @@ xmlns:winuiex="using:WinUIEx" mc:Ignorable="d" Title="Coder Settings" - Width="600" Height="500" - MinWidth="600" MinHeight="500"> + Width="600" Height="350" + MinWidth="600" MinHeight="350"> diff --git a/App/Views/TrayWindow.xaml.cs b/App/Views/TrayWindow.xaml.cs index de944c2..72ab6cc 100644 --- a/App/Views/TrayWindow.xaml.cs +++ b/App/Views/TrayWindow.xaml.cs @@ -5,9 +5,11 @@ using Coder.Desktop.App.Views.Pages; using CommunityToolkit.Mvvm.Input; using Microsoft.UI; +using Microsoft.UI.Input; using Microsoft.UI.Windowing; using Microsoft.UI.Xaml; using Microsoft.UI.Xaml.Controls; +using Microsoft.UI.Xaml.Documents; using Microsoft.UI.Xaml.Media.Animation; using System; using System.Collections.Generic; @@ -17,7 +19,6 @@ using Windows.Graphics; using Windows.System; using Windows.UI.Core; -using Microsoft.UI.Input; using WinRT.Interop; using WindowActivatedEventArgs = Microsoft.UI.Xaml.WindowActivatedEventArgs; @@ -40,6 +41,7 @@ public sealed partial class TrayWindow : Window private readonly IRpcController _rpcController; private readonly ICredentialManager _credentialManager; + private readonly ISyncSessionController _syncSessionController; private readonly IUpdateController _updateController; private readonly IUserNotifier _userNotifier; private readonly TrayWindowLoadingPage _loadingPage; @@ -49,13 +51,15 @@ public sealed partial class TrayWindow : Window public TrayWindow( IRpcController rpcController, ICredentialManager credentialManager, - IUpdateController updateController, IUserNotifier userNotifier, + ISyncSessionController syncSessionController, IUpdateController updateController, + IUserNotifier userNotifier, TrayWindowLoadingPage loadingPage, TrayWindowDisconnectedPage disconnectedPage, TrayWindowLoginRequiredPage loginRequiredPage, TrayWindowMainPage mainPage) { _rpcController = rpcController; _credentialManager = credentialManager; + _syncSessionController = syncSessionController; _updateController = updateController; _userNotifier = userNotifier; _loadingPage = loadingPage; @@ -70,7 +74,9 @@ public TrayWindow( _rpcController.StateChanged += RpcController_StateChanged; _credentialManager.CredentialsChanged += CredentialManager_CredentialsChanged; - SetPageByState(_rpcController.GetState(), _credentialManager.GetCachedCredentials()); + _syncSessionController.StateChanged += SyncSessionController_StateChanged; + SetPageByState(_rpcController.GetState(), _credentialManager.GetCachedCredentials(), + _syncSessionController.GetState()); // Setting these directly in the .xaml doesn't seem to work for whatever reason. TrayIcon.OpenCommand = Tray_OpenCommand; @@ -121,7 +127,8 @@ public TrayWindow( }; } - private void SetPageByState(RpcModel rpcModel, CredentialModel credentialModel) + private void SetPageByState(RpcModel rpcModel, CredentialModel credentialModel, + SyncSessionControllerStateModel syncSessionModel) { if (credentialModel.State == CredentialState.Unknown) { @@ -194,13 +201,18 @@ private void MaybeNotifyUser(RpcModel rpcModel) private void RpcController_StateChanged(object? _, RpcModel model) { - SetPageByState(model, _credentialManager.GetCachedCredentials()); + SetPageByState(model, _credentialManager.GetCachedCredentials(), _syncSessionController.GetState()); MaybeNotifyUser(model); } private void CredentialManager_CredentialsChanged(object? _, CredentialModel model) { - SetPageByState(_rpcController.GetState(), model); + SetPageByState(_rpcController.GetState(), model, _syncSessionController.GetState()); + } + + private void SyncSessionController_StateChanged(object? _, SyncSessionControllerStateModel model) + { + SetPageByState(_rpcController.GetState(), _credentialManager.GetCachedCredentials(), model); } // Sadly this is necessary because Window.Content.SizeChanged doesn't diff --git a/Tests.App/Services/CredentialManagerTest.cs b/Tests.App/Services/CredentialManagerTest.cs index 3f9c4cb..9f1b0df 100644 --- a/Tests.App/Services/CredentialManagerTest.cs +++ b/Tests.App/Services/CredentialManagerTest.cs @@ -317,10 +317,7 @@ public async Task SetDuringLoad(CancellationToken ct) var loadTask = manager.LoadCredentials(ct); // Then fully perform a set. await manager.SetCredentials(TestServerUrl, TestApiToken, ct).WaitAsync(ct); - - // The load should complete with the new valid credentials - var result = await loadTask; - Assert.That(result.State, Is.EqualTo(CredentialState.Valid)); - Assert.That(result.CoderUrl?.ToString(), Is.EqualTo(TestServerUrl)); + // The load should have been cancelled. + Assert.ThrowsAsync(() => loadTask); } } diff --git a/Vpn.Proto/vpn.proto b/Vpn.Proto/vpn.proto index 61c9978..11a481c 100644 --- a/Vpn.Proto/vpn.proto +++ b/Vpn.Proto/vpn.proto @@ -62,7 +62,7 @@ message ServiceMessage { StartResponse start = 2; StopResponse stop = 3; Status status = 4; // either in reply to a StatusRequest or broadcasted - StartProgress start_progress = 5; // broadcasted during startup (used exclusively by Windows) + StartProgress start_progress = 5; // broadcasted during startup } } @@ -214,7 +214,6 @@ message NetworkSettingsResponse { // StartResponse. message StartRequest { int32 tunnel_file_descriptor = 1; - bool tunnel_use_soft_net_isolation = 8; string coder_url = 2; string api_token = 3; // Additional HTTP headers added to all requests From ce225d0ff351742697b48fc40202371d9aa8f2aa Mon Sep 17 00:00:00 2001 From: Dean Sheather Date: Tue, 29 Jul 2025 22:05:49 +1000 Subject: [PATCH 2/2] Restore fixes --- App/Services/CredentialManager.cs | 10 ++++----- App/Views/TrayWindow.xaml.cs | 24 ++++++--------------- Tests.App/Services/CredentialManagerTest.cs | 7 ++++-- 3 files changed, 15 insertions(+), 26 deletions(-) diff --git a/App/Services/CredentialManager.cs b/App/Services/CredentialManager.cs index 6868ae7..c8030c3 100644 --- a/App/Services/CredentialManager.cs +++ b/App/Services/CredentialManager.cs @@ -223,8 +223,9 @@ private async Task LoadCredentialsInner(CancellationToken ct) }; } - // Grab the lock again so we can update the state. - using (await _opLock.LockAsync(ct)) + // Grab the lock again so we can update the state. Don't use the CT + // here as it may have already been canceled. + using (await _opLock.LockAsync(TimeSpan.FromSeconds(5), CancellationToken.None)) { // Prevent new LoadCredentials calls from returning this task. if (_loadCts != null) @@ -242,11 +243,8 @@ private async Task LoadCredentialsInner(CancellationToken ct) if (latestCreds is not null) return latestCreds; } - // If there aren't any latest credentials after a cancellation, we - // most likely timed out and should throw. - ct.ThrowIfCancellationRequested(); - UpdateState(model); + ct.ThrowIfCancellationRequested(); return model; } } diff --git a/App/Views/TrayWindow.xaml.cs b/App/Views/TrayWindow.xaml.cs index 72ab6cc..de944c2 100644 --- a/App/Views/TrayWindow.xaml.cs +++ b/App/Views/TrayWindow.xaml.cs @@ -5,11 +5,9 @@ using Coder.Desktop.App.Views.Pages; using CommunityToolkit.Mvvm.Input; using Microsoft.UI; -using Microsoft.UI.Input; using Microsoft.UI.Windowing; using Microsoft.UI.Xaml; using Microsoft.UI.Xaml.Controls; -using Microsoft.UI.Xaml.Documents; using Microsoft.UI.Xaml.Media.Animation; using System; using System.Collections.Generic; @@ -19,6 +17,7 @@ using Windows.Graphics; using Windows.System; using Windows.UI.Core; +using Microsoft.UI.Input; using WinRT.Interop; using WindowActivatedEventArgs = Microsoft.UI.Xaml.WindowActivatedEventArgs; @@ -41,7 +40,6 @@ public sealed partial class TrayWindow : Window private readonly IRpcController _rpcController; private readonly ICredentialManager _credentialManager; - private readonly ISyncSessionController _syncSessionController; private readonly IUpdateController _updateController; private readonly IUserNotifier _userNotifier; private readonly TrayWindowLoadingPage _loadingPage; @@ -51,15 +49,13 @@ public sealed partial class TrayWindow : Window public TrayWindow( IRpcController rpcController, ICredentialManager credentialManager, - ISyncSessionController syncSessionController, IUpdateController updateController, - IUserNotifier userNotifier, + IUpdateController updateController, IUserNotifier userNotifier, TrayWindowLoadingPage loadingPage, TrayWindowDisconnectedPage disconnectedPage, TrayWindowLoginRequiredPage loginRequiredPage, TrayWindowMainPage mainPage) { _rpcController = rpcController; _credentialManager = credentialManager; - _syncSessionController = syncSessionController; _updateController = updateController; _userNotifier = userNotifier; _loadingPage = loadingPage; @@ -74,9 +70,7 @@ public TrayWindow( _rpcController.StateChanged += RpcController_StateChanged; _credentialManager.CredentialsChanged += CredentialManager_CredentialsChanged; - _syncSessionController.StateChanged += SyncSessionController_StateChanged; - SetPageByState(_rpcController.GetState(), _credentialManager.GetCachedCredentials(), - _syncSessionController.GetState()); + SetPageByState(_rpcController.GetState(), _credentialManager.GetCachedCredentials()); // Setting these directly in the .xaml doesn't seem to work for whatever reason. TrayIcon.OpenCommand = Tray_OpenCommand; @@ -127,8 +121,7 @@ public TrayWindow( }; } - private void SetPageByState(RpcModel rpcModel, CredentialModel credentialModel, - SyncSessionControllerStateModel syncSessionModel) + private void SetPageByState(RpcModel rpcModel, CredentialModel credentialModel) { if (credentialModel.State == CredentialState.Unknown) { @@ -201,18 +194,13 @@ private void MaybeNotifyUser(RpcModel rpcModel) private void RpcController_StateChanged(object? _, RpcModel model) { - SetPageByState(model, _credentialManager.GetCachedCredentials(), _syncSessionController.GetState()); + SetPageByState(model, _credentialManager.GetCachedCredentials()); MaybeNotifyUser(model); } private void CredentialManager_CredentialsChanged(object? _, CredentialModel model) { - SetPageByState(_rpcController.GetState(), model, _syncSessionController.GetState()); - } - - private void SyncSessionController_StateChanged(object? _, SyncSessionControllerStateModel model) - { - SetPageByState(_rpcController.GetState(), _credentialManager.GetCachedCredentials(), model); + SetPageByState(_rpcController.GetState(), model); } // Sadly this is necessary because Window.Content.SizeChanged doesn't diff --git a/Tests.App/Services/CredentialManagerTest.cs b/Tests.App/Services/CredentialManagerTest.cs index 9f1b0df..3f9c4cb 100644 --- a/Tests.App/Services/CredentialManagerTest.cs +++ b/Tests.App/Services/CredentialManagerTest.cs @@ -317,7 +317,10 @@ public async Task SetDuringLoad(CancellationToken ct) var loadTask = manager.LoadCredentials(ct); // Then fully perform a set. await manager.SetCredentials(TestServerUrl, TestApiToken, ct).WaitAsync(ct); - // The load should have been cancelled. - Assert.ThrowsAsync(() => loadTask); + + // The load should complete with the new valid credentials + var result = await loadTask; + Assert.That(result.State, Is.EqualTo(CredentialState.Valid)); + Assert.That(result.CoderUrl?.ToString(), Is.EqualTo(TestServerUrl)); } }