From b4abd5cbab6216781892a827e85bf89ebac66048 Mon Sep 17 00:00:00 2001 From: Shane Neuville Date: Sun, 15 Mar 2026 16:50:42 -0500 Subject: [PATCH 1/2] fix: restore Direct Sharing persistence on Mac Catalyst MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PR #341 moved ServerPassword, RemoteToken, and LanToken to SecureStorage (Keychain) on Mac Catalyst via [JsonIgnore] and #if IOS || ANDROID || MACCATALYST guards. However, Mac Catalyst runs without app sandbox (disabled in Entitlements.plist), making Keychain unreliable. The password was silently lost on restart, so StartDirectSharingIfEnabled() would skip due to empty password. Fix: - Change SecureStorage guards from IOS || ANDROID || MACCATALYST to IOS || ANDROID only — Mac Catalyst is a desktop platform - Add one-time RecoverSecretsFromSecureStorage() for MACCATALYST to recover any passwords already migrated to Keychain by PR 341 - Only clean up Keychain entries after verifying JSON was written - Add 4 regression tests for secret serialization on desktop Verified via MauiDevFlow: enabled Direct Sharing, relaunched app, confirmed bridge auto-started with 'Stop Direct Sharing' visible. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- PolyPilot.Tests/ConnectionSettingsTests.cs | 73 +++++++++++++++++++++ PolyPilot/Models/ConnectionSettings.cs | 75 ++++++++++++++++++++-- 2 files changed, 143 insertions(+), 5 deletions(-) diff --git a/PolyPilot.Tests/ConnectionSettingsTests.cs b/PolyPilot.Tests/ConnectionSettingsTests.cs index ddc1e517f6..aab9f19576 100644 --- a/PolyPilot.Tests/ConnectionSettingsTests.cs +++ b/PolyPilot.Tests/ConnectionSettingsTests.cs @@ -483,6 +483,79 @@ public void VsCodeVariant_DisplayName_ReturnsCorrectLabel() Assert.Equal("VS Code Insiders", VsCodeVariant.Insiders.DisplayName()); } + [Fact] + public void ServerPassword_SerializesToJson_OnDesktop() + { + // Regression test for PR 341: ServerPassword must be included in JSON serialization + // on desktop platforms (including Mac Catalyst). Previously, [JsonIgnore] on MACCATALYST + // caused the password to be silently dropped, breaking Direct Sharing persistence. + var settings = new ConnectionSettings + { + ServerPassword = "test-password-123", + DirectSharingEnabled = true + }; + + var json = JsonSerializer.Serialize(settings, new JsonSerializerOptions { WriteIndented = true }); + Assert.Contains("\"ServerPassword\"", json); + Assert.Contains("test-password-123", json); + } + + [Fact] + public void ServerPassword_DeserializesFromJson_OnDesktop() + { + // Regression test: password round-trips through JSON on desktop platforms + var json = """{"Mode":0,"Host":"localhost","Port":4321,"ServerPassword":"my-secret","DirectSharingEnabled":true}"""; + var loaded = JsonSerializer.Deserialize(json); + + Assert.NotNull(loaded); + Assert.Equal("my-secret", loaded!.ServerPassword); + Assert.True(loaded.DirectSharingEnabled); + } + + [Fact] + public void DirectSharing_RequiresServerPassword_ForAutoStart() + { + // Validates the auto-start guard logic from Dashboard.StartDirectSharingIfEnabled: + // DirectSharingEnabled=true with empty password should NOT auto-start. + var settings = new ConnectionSettings + { + DirectSharingEnabled = true, + ServerPassword = null + }; + bool shouldAutoStart = settings.DirectSharingEnabled && !string.IsNullOrEmpty(settings.ServerPassword); + Assert.False(shouldAutoStart, "Should not auto-start without password"); + + settings.ServerPassword = ""; + shouldAutoStart = settings.DirectSharingEnabled && !string.IsNullOrEmpty(settings.ServerPassword); + Assert.False(shouldAutoStart, "Should not auto-start with empty password"); + + settings.ServerPassword = "real-password"; + shouldAutoStart = settings.DirectSharingEnabled && !string.IsNullOrEmpty(settings.ServerPassword); + Assert.True(shouldAutoStart, "Should auto-start with password set"); + } + + [Fact] + public void AllSecretFields_PresentInJson_OnDesktop() + { + // Ensures RemoteToken, LanToken, and ServerPassword all serialize on desktop. + // On iOS/Android they get [JsonIgnore] and go to SecureStorage, + // but on desktop (including Mac Catalyst) they must be in plain JSON. + var settings = new ConnectionSettings + { + RemoteToken = "remote-tok", + LanToken = "lan-tok", + ServerPassword = "srv-pass" + }; + + var json = JsonSerializer.Serialize(settings); + Assert.Contains("\"RemoteToken\"", json); + Assert.Contains("\"LanToken\"", json); + Assert.Contains("\"ServerPassword\"", json); + Assert.Contains("remote-tok", json); + Assert.Contains("lan-tok", json); + Assert.Contains("srv-pass", json); + } + private void Dispose() { try { Directory.Delete(_testDir, true); } catch { } diff --git a/PolyPilot/Models/ConnectionSettings.cs b/PolyPilot/Models/ConnectionSettings.cs index 0425e04b37..6a17f2ddd4 100644 --- a/PolyPilot/Models/ConnectionSettings.cs +++ b/PolyPilot/Models/ConnectionSettings.cs @@ -70,8 +70,9 @@ public class ConnectionSettings public bool AutoStartServer { get; set; } = false; public string? RemoteUrl { get; set; } - // Secrets: stored in SecureStorage on iOS/Android/MacCatalyst; plain JSON on other desktop. -#if IOS || ANDROID || MACCATALYST + // Secrets: stored in SecureStorage on iOS/Android; plain JSON on desktop (incl. Mac Catalyst). + // Mac Catalyst runs without app sandbox, making Keychain unreliable for SecureStorage. +#if IOS || ANDROID private string? _remoteToken; [System.Text.Json.Serialization.JsonIgnore] public string? RemoteToken @@ -215,7 +216,11 @@ public static ConnectionSettings Load() if (settings.Theme == UiTheme.InternationalWomensDay) settings.Theme = UiTheme.System; -#if IOS || ANDROID || MACCATALYST +#if MACCATALYST + // Reverse migration: PR 341 moved secrets to SecureStorage on Mac Catalyst, + // but Keychain is unreliable without app sandboxing. Recover secrets to plain JSON. + RecoverSecretsFromSecureStorage(settings); +#elif IOS || ANDROID settings.MigrateAndLoadMobileSecrets(rawJson); #endif @@ -260,7 +265,7 @@ public void Save() { var dir = Path.GetDirectoryName(SettingsPath)!; Directory.CreateDirectory(dir); -#if IOS || ANDROID || MACCATALYST +#if IOS || ANDROID SaveMobileSecretsIfDirty(); var json = JsonSerializer.Serialize(this, new JsonSerializerOptions { WriteIndented = true }); #else @@ -271,7 +276,67 @@ public void Save() catch { } } -#if IOS || ANDROID || MACCATALYST +#if MACCATALYST + /// + /// One-time reverse migration: PR 341 moved ServerPassword/RemoteToken/LanToken to + /// SecureStorage on Mac Catalyst. Since Mac Catalyst runs without sandbox, Keychain + /// is unreliable. This recovers any values and writes them back to plain JSON. + /// + private static void RecoverSecretsFromSecureStorage(ConnectionSettings settings) + { + try + { + bool needsSave = false; + + if (string.IsNullOrEmpty(settings.RemoteToken)) + { + var val = ReadSecureStorage("polypilot.connection.remoteToken"); + if (!string.IsNullOrEmpty(val)) { settings.RemoteToken = val; needsSave = true; } + } + if (string.IsNullOrEmpty(settings.LanToken)) + { + var val = ReadSecureStorage("polypilot.connection.lanToken"); + if (!string.IsNullOrEmpty(val)) { settings.LanToken = val; needsSave = true; } + } + if (string.IsNullOrEmpty(settings.ServerPassword)) + { + var val = ReadSecureStorage("polypilot.connection.serverPassword"); + if (!string.IsNullOrEmpty(val)) { settings.ServerPassword = val; needsSave = true; } + } + + if (needsSave) + { + settings.Save(); + + // Only clean up Keychain after verifying the JSON file was actually written. + // If Save() failed silently, leave Keychain intact so migration retries next launch. + if (File.Exists(SettingsPath)) + { + try + { + var verify = File.ReadAllText(SettingsPath); + if (verify.Contains("ServerPassword")) + { + try { SecureStorage.Default.Remove("polypilot.connection.remoteToken"); } catch { } + try { SecureStorage.Default.Remove("polypilot.connection.lanToken"); } catch { } + try { SecureStorage.Default.Remove("polypilot.connection.serverPassword"); } catch { } + } + } + catch { } + } + } + } + catch { } + } + + private static string? ReadSecureStorage(string key) + { + try { return Task.Run(() => SecureStorage.Default.GetAsync(key)).GetAwaiter().GetResult(); } + catch { return null; } + } +#endif + +#if IOS || ANDROID private const string RemoteTokenKey = "polypilot.connection.remoteToken"; private const string LanTokenKey = "polypilot.connection.lanToken"; private const string ServerPasswordKey = "polypilot.connection.serverPassword"; From 8ae6dbc5e4899d71fa128126828ce8d1abf1d603 Mon Sep 17 00:00:00 2001 From: Shane Neuville Date: Sun, 15 Mar 2026 17:44:53 -0500 Subject: [PATCH 2/2] fix: per-key Keychain cleanup to prevent data loss on transient read failure Addresses PR review finding #1 (critical): if ReadSecureStorage fails transiently for one secret but succeeds for another, the blanket SecureStorage.Remove() would destroy the unrecovered secret. Now each Keychain entry is only removed if that specific value was successfully recovered. Also removes the no-op verify.Contains() guard (finding #2) since per-key tracking makes it unnecessary. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- PolyPilot/Models/ConnectionSettings.cs | 30 ++++++++++++-------------- 1 file changed, 14 insertions(+), 16 deletions(-) diff --git a/PolyPilot/Models/ConnectionSettings.cs b/PolyPilot/Models/ConnectionSettings.cs index 6a17f2ddd4..1fd653b60b 100644 --- a/PolyPilot/Models/ConnectionSettings.cs +++ b/PolyPilot/Models/ConnectionSettings.cs @@ -281,48 +281,46 @@ public void Save() /// One-time reverse migration: PR 341 moved ServerPassword/RemoteToken/LanToken to /// SecureStorage on Mac Catalyst. Since Mac Catalyst runs without sandbox, Keychain /// is unreliable. This recovers any values and writes them back to plain JSON. + /// Only removes each Keychain entry after confirming that specific value was recovered. /// private static void RecoverSecretsFromSecureStorage(ConnectionSettings settings) { try { bool needsSave = false; + bool recoveredRemote = false, recoveredLan = false, recoveredPass = false; if (string.IsNullOrEmpty(settings.RemoteToken)) { var val = ReadSecureStorage("polypilot.connection.remoteToken"); - if (!string.IsNullOrEmpty(val)) { settings.RemoteToken = val; needsSave = true; } + if (!string.IsNullOrEmpty(val)) { settings.RemoteToken = val; needsSave = true; recoveredRemote = true; } } if (string.IsNullOrEmpty(settings.LanToken)) { var val = ReadSecureStorage("polypilot.connection.lanToken"); - if (!string.IsNullOrEmpty(val)) { settings.LanToken = val; needsSave = true; } + if (!string.IsNullOrEmpty(val)) { settings.LanToken = val; needsSave = true; recoveredLan = true; } } if (string.IsNullOrEmpty(settings.ServerPassword)) { var val = ReadSecureStorage("polypilot.connection.serverPassword"); - if (!string.IsNullOrEmpty(val)) { settings.ServerPassword = val; needsSave = true; } + if (!string.IsNullOrEmpty(val)) { settings.ServerPassword = val; needsSave = true; recoveredPass = true; } } if (needsSave) { settings.Save(); - // Only clean up Keychain after verifying the JSON file was actually written. - // If Save() failed silently, leave Keychain intact so migration retries next launch. + // Per-key cleanup: only remove a Keychain entry if that specific value was recovered + // and Save() wrote the file. Prevents data loss if Keychain read fails transiently + // for one secret but succeeds for another. if (File.Exists(SettingsPath)) { - try - { - var verify = File.ReadAllText(SettingsPath); - if (verify.Contains("ServerPassword")) - { - try { SecureStorage.Default.Remove("polypilot.connection.remoteToken"); } catch { } - try { SecureStorage.Default.Remove("polypilot.connection.lanToken"); } catch { } - try { SecureStorage.Default.Remove("polypilot.connection.serverPassword"); } catch { } - } - } - catch { } + if (recoveredRemote) + try { SecureStorage.Default.Remove("polypilot.connection.remoteToken"); } catch { } + if (recoveredLan) + try { SecureStorage.Default.Remove("polypilot.connection.lanToken"); } catch { } + if (recoveredPass) + try { SecureStorage.Default.Remove("polypilot.connection.serverPassword"); } catch { } } } }