From 23d5659691f3033f79265348fe79a45d88f9c183 Mon Sep 17 00:00:00 2001 From: Ryanmello07 <67509637+Ryanmello07@users.noreply.github.com> Date: Mon, 7 Sep 2026 21:55:23 -0700 Subject: [PATCH] persist the dns resolver settings in the app's own local state The same shape as the split rules: the device persists its resolver settings, but into the network extension's container, which the app process cannot read. DnsSettingsStore kept no copy of its own, so everything configured on the DNS page died with the app process. The editor makes it worse than a lost edit. DnsSettingsView opens on whatever the device reports, and with the rpc down that is the remote's own empty memory -- so a tunnel-down launch shows the blank never-configured form over the resolver the extension still holds on disk, and applying from that blank base replaces it on the next connect. So the store takes the app's local state, mirrors every reading it can vouch for, and DeviceManager.initDevice seeds the next device from that mirror. One difference from the rules, and it decides the design. DeviceLocal.SetDnsResolverSettings returns WITHOUT persisting on nil settings and when the mux is disabled, so the app cannot treat its own write as accepted -- the mirror is written from the read-back rather than from the value just pushed. Connected, that is the extension's answer to the edit, so a dropped edit mirrors as the settings still in force; disconnected, it is the edit the remote queued, which is what the next connect applies. The mux-disabled case is self-consistent rather than a hazard: the same condition that makes the setter bail makes the getter report nothing, and a device that reports nothing mirrors nothing. nil is never written: it takes the store's delete branch, and a deleted store reads back as never configured, which is what the seed reads as "leave the extension's resolver alone". Settings with everything turned off are a real value -- the user emptied the form -- and are mirrored and seeded as one. The store also re-reads once the rpc is up. The settings the extension replays on connect arrive over the reverse sync, before the remote publishes its service, so that read is unvouched; without the re-read a build upgrading from before this fix would keep a nil mirror indefinitely and sit one edit away from the destructive path above. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01AWV4MaF9JBcQppSX9UxATa --- app/network/NetworkApp.swift | 8 +- .../Shared/ViewModels/DeviceManager.swift | 13 ++ .../Shared/ViewModels/DnsSettingsStore.swift | 95 ++++++++++++- .../DnsResolverSettingsMirrorTests.swift | 125 ++++++++++++++++++ 4 files changed, 233 insertions(+), 8 deletions(-) create mode 100644 app/networkTests/DnsResolverSettingsMirrorTests.swift diff --git a/app/network/NetworkApp.swift b/app/network/NetworkApp.swift index c97d7c00..1a8203b0 100644 --- a/app/network/NetworkApp.swift +++ b/app/network/NetworkApp.swift @@ -169,11 +169,11 @@ struct NetworkApp: App { private func setupDeviceStores(_ device: SdkDeviceRemote) { let localState = deviceManager.asyncLocalState?.getLocalState() throughputStore.setup(device) - // the app local state mirrors the split rule and transport settings - // edits so they survive an app relaunch while the tunnel is down - // (see the stores) + // the app local state mirrors the split rule, dns and transport + // settings edits so they survive an app relaunch while the tunnel is + // down (see the stores) blockActionsStore.setup(device, localState: localState) - dnsSettingsStore.setup(device) + dnsSettingsStore.setup(device, localState: localState) transportSettingsStore.setup(device, localState: localState) networkPeersStore.setup(device) reliabilityStore.setup(device) diff --git a/app/network/Shared/ViewModels/DeviceManager.swift b/app/network/Shared/ViewModels/DeviceManager.swift index f4f691b5..f84b8273 100644 --- a/app/network/Shared/ViewModels/DeviceManager.swift +++ b/app/network/Shared/ViewModels/DeviceManager.swift @@ -1238,6 +1238,19 @@ extension DeviceManager { device.setBlockActionOverrides(blockActionOverrides) } + // the dns resolver settings cross the same way, mirrored by + // DnsSettingsStore. The editor opens on whatever the device reports, so + // without the seed a tunnel-down launch shows the blank + // never-configured form over the resolver the extension has on disk, + // and applying an edit from that blank base replaces it on the next + // connect. Nothing stored means never mirrored -- leave the + // extension's resolver alone; settings with everything turned off are + // a real value and are seeded as one. Above setDevice for the same + // reason as the rules. + if let dnsResolverSettings = localState.getDnsResolverSettings() { + device.setDnsResolverSettings(dnsResolverSettings) + } + self.setDevice(device: device) return true } diff --git a/app/network/Shared/ViewModels/DnsSettingsStore.swift b/app/network/Shared/ViewModels/DnsSettingsStore.swift index 2b9c7f0a..4f8642a5 100644 --- a/app/network/Shared/ViewModels/DnsSettingsStore.swift +++ b/app/network/Shared/ViewModels/DnsSettingsStore.swift @@ -113,8 +113,27 @@ private class DnsResolverSettingsListener: NSObject, SdkDnsResolverSettingsChang } } +private class DnsSettingsRemoteListener: NSObject, SdkRemoteChangeListenerProtocol { + private let callback: (Bool) -> Void + init(callback: @escaping (Bool) -> Void) { + self.callback = callback + } + func remoteChanged(_ remoteConnected: Bool) { + callback(remoteConnected) + } +} + /** - * Publishes the device dns resolver settings and applies edits + * Publishes the device dns resolver settings and applies edits. + * + * The device persists the settings in its own local state, but in the network + * extension's container, which the app process cannot read. The store mirrors + * every reading it can vouch for into the app's own local state, and + * `DeviceManager.initDevice` seeds the next device from that mirror. Without + * it the editor opens on the blank never-configured form whenever the tunnel + * is down -- and an edit applied from that blank base is not merely lost with + * the app process, it replaces the resolver the extension still holds on the + * next connect. */ @MainActor class DnsSettingsStore: ObservableObject { @@ -122,17 +141,34 @@ class DnsSettingsStore: ObservableObject { @Published private(set) var settings: DnsSettings? = nil private var device: SdkDeviceRemote? + // the app's own store, which the resolver settings are mirrored into; the + // extension keeps its copy in a container this process cannot read + private var localState: SdkLocalState? private var settingsSub: SdkSubProtocol? + private var remoteSub: SdkSubProtocol? - func setup(_ device: SdkDeviceRemote) { + func setup(_ device: SdkDeviceRemote, localState: SdkLocalState?) { reset() self.device = device + self.localState = localState self.settingsSub = device.add(DnsResolverSettingsListener { [weak self] in DispatchQueue.main.async { self?.update() } }) + // the settings the extension replays on connect arrive over the + // reverse sync, which runs before the remote publishes its service, so + // that read still comes out of the remote's own memory. Re-read once + // the rpc is up so the mirror is written from the extension's resolver + // rather than from this process's guess at it + self.remoteSub = device.add(DnsSettingsRemoteListener { [weak self] remoteConnected in + DispatchQueue.main.async { + if remoteConnected { + self?.update() + } + } + }) update() } @@ -140,26 +176,77 @@ class DnsSettingsStore: ObservableObject { func reset() { settingsSub?.close() settingsSub = nil + remoteSub?.close() + remoteSub = nil device = nil + // nothing is persisted from here: reset runs on every backgrounding + // and the mirror is already current by then + localState = nil settings = nil } - private func update() { + /** + * Re-reads the settings from the device and, when the read is one this + * process can vouch for, mirrors it. + * + * Only two reads qualify: one taken off a connected device, which is the + * extension's own resolver, and one taken right after an edit made here. + * With the rpc down and nothing seeded the device answers out of its own + * empty memory instead, and mirroring that would replace the app's only + * durable copy with settings it never read. + * + * The mirror is written from the read-back and never from the value just + * pushed: `DeviceLocal.SetDnsResolverSettings` returns without persisting + * when the settings are nil or the mux is disabled, so the app cannot take + * its own write as accepted. Connected, the read-back is the extension's + * answer to the edit -- a dropped edit mirrors as the settings still in + * force; disconnected, it is the edit the remote queued, which is what the + * next connect applies. + * + * `getConnected()` is GetRemoteConnected; swift's importer strips the + * redundant "Remote". + */ + private func update(afterEdit: Bool = false) { guard let device = self.device else { return } if let sdkSettings = device.getDnsResolverSettings() { settings = DnsSettings(sdkSettings) + if afterEdit || device.getConnected() { + persist(sdkSettings) + } } else { settings = nil } } + /** + * Writes the settings to the app's own local state. + * + * Always the sdk object read back from the device rather than the + * `DnsSettings` projection, so the fields this app does not model (the + * upgrade mask address) survive the mirror. + * + * nil is never written: it takes the store's delete branch, and a deleted + * store reads back as never configured, which is what + * `DeviceManager.initDevice` reads as "leave the extension's resolver + * alone". A device that reports no settings therefore mirrors nothing, + * while settings with everything turned off are a real value -- the user + * emptied the form -- and are mirrored as one. + */ + private func persist(_ sdkSettings: SdkDnsResolverSettings) { + do { + try localState?.setDnsResolverSettings(sdkSettings) + } catch { + print("[DnsSettingsStore]failed to persist dns resolver settings: \(error.localizedDescription)") + } + } + func apply(_ newSettings: DnsSettings) { guard let device = self.device else { return } device.setDnsResolverSettings(newSettings.toSdk()) - update() + update(afterEdit: true) } } diff --git a/app/networkTests/DnsResolverSettingsMirrorTests.swift b/app/networkTests/DnsResolverSettingsMirrorTests.swift new file mode 100644 index 00000000..a9111e18 --- /dev/null +++ b/app/networkTests/DnsResolverSettingsMirrorTests.swift @@ -0,0 +1,125 @@ +// +// DnsResolverSettingsMirrorTests.swift +// networkTests +// +// Covers the store the dns resolver settings are mirrored into. +// +// The settings themselves live in the network extension's container, which +// the app process cannot read, so this mirror is the app's only durable copy +// -- it is what the editor opens on with the tunnel down and what +// DeviceManager.initDevice seeds the next device from. +// +// The distinction the fix would be silently wrong without: writing nil +// DELETES the store, and a deleted store reads back exactly like one that was +// never written, so "the user turned everything off" and "this install has +// never mirrored" are the same value unless an emptied form is written as a +// settings object. Read as never-mirrored it would restore the resolver the +// user just cleared; read as emptied it would push a blank resolver over the +// one the extension still holds. DnsSettingsStore.persist and the seed's +// `if let` both rest on that line. +// + +import Testing +import Foundation +import URnetworkSdk +@testable import URnetwork + +struct DnsResolverSettingsMirrorTests { + + /// Each test gets its own storage home; the SDK roots the store at + /// `/.by`, so a fresh temporary directory is a fresh store. + private func withLocalState(_ body: (SdkLocalState) throws -> Void) rethrows { + let home = FileManager.default.temporaryDirectory + .appendingPathComponent("DnsResolverSettingsMirrorTests-\(UUID().uuidString)") + try? FileManager.default.createDirectory(at: home, withIntermediateDirectories: true) + defer { try? FileManager.default.removeItem(at: home) } + + guard let asyncLocalState = SdkAsyncLocalState(home.path()), + let localState = asyncLocalState.getLocalState() else { + Issue.record("could not open a local state under \(home.path())") + return + } + defer { asyncLocalState.close() } + try body(localState) + } + + /// a configuration with both a toggle set and servers in more than one + /// list, so a round trip that drops either shows up + private var configured: DnsSettings { + var settings = DnsSettings() + settings.enableRemoteDoh = true + settings.enableLocalDns = true + settings.enableFallback = true + settings.remoteDohUrlsIpv4 = ["https://1.1.1.1/dns-query", "https://8.8.8.8/dns-query"] + settings.remoteDohUrlsIpv6 = ["https://[2606:4700:4700::1111]/dns-query"] + settings.localDnsIpv4 = ["192.168.1.1"] + settings.localDnsIpv6 = ["fe80::1"] + return settings + } + + /// Every field the editor writes and the connect drawer summarizes -- + /// settings that round-trip without them render as a resolver the user + /// never configured. + @Test func settingsRoundTripWithTheirTogglesAndServers() throws { + try withLocalState { localState in + let configured = self.configured + + try localState.setDnsResolverSettings(configured.toSdk()) + + let read = try #require(localState.getDnsResolverSettings()) + #expect(DnsSettings(read) == configured) + } + } + + /// The mirror copies the device's own settings object rather than the + /// DnsSettings projection the editor renders, so fields the app never + /// models have to survive it too. + @Test func fieldsTheAppDoesNotModelSurviveTheRoundTrip() throws { + try withLocalState { localState in + let sdkSettings = configured.toSdk() + sdkSettings.dnsUpgradeMaskAddress = "10.64.0.1" + + try localState.setDnsResolverSettings(sdkSettings) + + let read = try #require(localState.getDnsResolverSettings()) + #expect(read.dnsUpgradeMaskAddress == "10.64.0.1") + } + } + + /// "I turned everything off" -- an emptied form is a real value and has to + /// come back as one, or the seed skips it and the extension's resolver + /// stands. + @Test func settingsWithNothingEnabledRoundTripAsAValue() throws { + try withLocalState { localState in + try localState.setDnsResolverSettings(configured.toSdk()) + try localState.setDnsResolverSettings(DnsSettings().toSdk()) + + let read = try #require(localState.getDnsResolverSettings()) + #expect(DnsSettings(read) == DnsSettings()) + } + } + + /// Writing nil is the delete branch. DnsSettingsStore.persist must never + /// reach it: the result is indistinguishable from an install that never + /// mirrored. + @Test func writingNilDeletesTheStore() throws { + try withLocalState { localState in + try localState.setDnsResolverSettings(configured.toSdk()) + #expect(localState.getDnsResolverSettings() != nil) + + try localState.setDnsResolverSettings(nil) + + #expect(localState.getDnsResolverSettings() == nil) + } + } + + /// The upgrade guard. A build that has never mirrored reads nil, so + /// DeviceManager.initDevice queues nothing and the resolver a pre-fix build + /// left in the extension is still there on the first launch after this + /// ships -- rather than being replaced by a blank one. + @Test func aStoreThatWasNeverWrittenReadsNilRatherThanEmptySettings() throws { + try withLocalState { localState in + #expect(localState.getDnsResolverSettings() == nil) + } + } +}