From ce3c052df6cc7b2cbadfc766d672c43189577c36 Mon Sep 17 00:00:00 2001 From: Konrad Kollnig <5175206+kasnder@users.noreply.github.com> Date: Wed, 5 Aug 2026 13:48:45 +0200 Subject: [PATCH 1/2] Notice a pinned Private DNS when it happens, and say so in the app MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The warning only re-evaluated when the tunnel was built, so the common sequence — VPN already running, user pins a resolver in Android settings — broke name resolution and stayed silent until some unrelated network change rebuilt it. onLinkPropertiesChanged already fires for this and LinkProperties already carries the resolver name; the reload policy simply discarded it by comparing DNS servers alone, which a pinned resolver leaves untouched. Comparing the name too costs no new registration, and the reason is deliberately absent from shouldRestartWireGuard: the tunnel is fine, only the warning is stale. The notification is also the sole surface, and Util.notify() is silent without POST_NOTIFICATIONS — so a user who declined it saw nothing at all in the one state where nothing loads. A row on the main screen, alongside the local-network one and re-evaluated in the same onResume, reaches them, and covers merely reopening the app (which reloads nothing). It is gated on the VPN actually running, since port 853 is only blocked while it is, and it revives msg_private_dns, which lost its last caller when the onboarding slide went. The condition both surfaces share now lives in Util.isPrivateDnsBlocked and is covered by PrivateDnsBlockedTest, including that an unreadable specifier must not silence the warning. --- .../eu/faircode/netguard/ActivityMain.java | 25 ++++ .../netguard/NetworkReloadPolicy.java | 10 +- .../eu/faircode/netguard/ServiceSinkhole.java | 16 ++- .../main/java/eu/faircode/netguard/Util.java | 13 +++ app/src/main/res/layout/main.xml | 20 ++++ .../netguard/NetworkReloadPolicyTest.java | 59 +++++++++- .../netguard/PrivateDnsBlockedTest.java | 110 ++++++++++++++++++ 7 files changed, 245 insertions(+), 8 deletions(-) create mode 100644 app/src/test/java/eu/faircode/netguard/PrivateDnsBlockedTest.java diff --git a/app/src/main/java/eu/faircode/netguard/ActivityMain.java b/app/src/main/java/eu/faircode/netguard/ActivityMain.java index d67fb5896..f3ea778dd 100644 --- a/app/src/main/java/eu/faircode/netguard/ActivityMain.java +++ b/app/src/main/java/eu/faircode/netguard/ActivityMain.java @@ -467,6 +467,21 @@ public void onClick(View v) { } }); + // Private DNS pinned to a hostname while we block DoT: name resolution + // fails outright, so the user may never see the notification that says + // so — nothing they tap can load anything either. + TextView tvPrivateDns = findViewById(R.id.tvPrivateDns); + tvPrivateDns.setVisibility(View.GONE); + tvPrivateDns.setOnClickListener(new View.OnClickListener() { + @Override + public void onClick(View v) { + Intent settings = new Intent(Settings.ACTION_WIRELESS_SETTINGS); + if (settings.resolveActivity(getPackageManager()) == null) + settings = new Intent(Settings.ACTION_WIFI_SETTINGS); + startActivity(settings); + } + }); + // Application list RecyclerView rvApplication = findViewById(R.id.rvApplication); rvApplication.setHasFixedSize(false); @@ -567,6 +582,16 @@ protected void onResume() { tvLocalNetwork.setVisibility( LocalNetworkAccess.isMissing(this) ? View.VISIBLE : View.GONE); + // Only while we are actually filtering: with the VPN off, port 853 is + // not blocked and a pinned resolver works fine. + TextView tvPrivateDns = findViewById(R.id.tvPrivateDns); + if (tvPrivateDns != null) { + boolean vpnEnabled = PreferenceManager.getDefaultSharedPreferences(this) + .getBoolean("enabled", false); + tvPrivateDns.setVisibility( + vpnEnabled && Util.isPrivateDnsBlocked(this) ? View.VISIBLE : View.GONE); + } + super.onResume(); } diff --git a/app/src/main/java/eu/faircode/netguard/NetworkReloadPolicy.java b/app/src/main/java/eu/faircode/netguard/NetworkReloadPolicy.java index 3df0bda47..b3e526bfd 100644 --- a/app/src/main/java/eu/faircode/netguard/NetworkReloadPolicy.java +++ b/app/src/main/java/eu/faircode/netguard/NetworkReloadPolicy.java @@ -9,6 +9,7 @@ final class NetworkReloadPolicy { static final String REASON_NETWORK_CHANGED = "Network changed"; static final String REASON_CONNECTED_CHANGED = "Connected state changed"; static final String REASON_LINK_PROPERTIES_CHANGED = "link properties changed"; + static final String REASON_PRIVATE_DNS_CHANGED = "private DNS changed"; static final String REASON_METERED_CHANGED = "Metered state changed"; static final String REASON_CONNECTIVITY_CHANGED = "connectivity changed"; @@ -30,10 +31,17 @@ static String onConnectivityChanged() { } static String onLinkPropertiesChanged(List lastDns, List currentDns, - boolean compareDns, boolean reloadOnConnectivity) { + boolean compareDns, boolean reloadOnConnectivity, + String lastPrivateDns, String currentPrivateDns) { if (compareDns ? !same(lastDns, currentDns) : reloadOnConnectivity) return REASON_LINK_PROPERTIES_CHANGED; + // Pinning Private DNS to a hostname leaves the resolver list alone, so + // the comparison above never sees it — yet it decides whether blocking + // DoT stops name resolution outright, which the user has to be told. + if (!Objects.equals(lastPrivateDns, currentPrivateDns)) + return REASON_PRIVATE_DNS_CHANGED; + return null; } diff --git a/app/src/main/java/eu/faircode/netguard/ServiceSinkhole.java b/app/src/main/java/eu/faircode/netguard/ServiceSinkhole.java index 1f060e805..32123ab71 100644 --- a/app/src/main/java/eu/faircode/netguard/ServiceSinkhole.java +++ b/app/src/main/java/eu/faircode/netguard/ServiceSinkhole.java @@ -1693,7 +1693,7 @@ private Builder getBuilder(List listAllowed, List listRule) { // In "hostname" mode it does not fall back — the user picked that // resolver explicitly, so DNS simply fails and nothing resolves. That // looks like TC broke the connection, with no hint of why, so say it. - if (prefs.getBoolean("block_dot", true) && Util.isPrivateDnsHostnameMode(this)) { + if (Util.isPrivateDnsBlocked(this)) { Log.w(TAG, "Private DNS set to a hostname: DoT is blocked and Android will not" + " fall back to plaintext DNS, so name resolution fails"); showPrivateDnsNotification(Util.getPrivateDnsSpecifier(this)); @@ -3146,6 +3146,7 @@ private void listenNetworkChanges() { private Boolean last_connected = null; private Boolean last_metered = null; private List last_dns = null; + private String last_private_dns = null; @Override public void onAvailable(Network network) { @@ -3168,17 +3169,26 @@ public void onLinkPropertiesChanged(Network network, LinkProperties linkProperti // Make sure the right DNS servers are being used List dns = linkProperties.getDnsServers(); + // Non-null only when Private DNS is pinned to a hostname, which + // leaves the resolver list untouched — so this is the only part + // of the properties that reveals the change. + String private_dns = (Build.VERSION.SDK_INT < Build.VERSION_CODES.P + ? null : linkProperties.getPrivateDnsServerName()); SharedPreferences prefs = PreferenceManager.getDefaultSharedPreferences(ServiceSinkhole.this); String reason = NetworkReloadPolicy.onLinkPropertiesChanged( last_dns, dns, Build.VERSION.SDK_INT >= Build.VERSION_CODES.O, - prefs.getBoolean("reload_onconnectivity", false)); + prefs.getBoolean("reload_onconnectivity", false), + last_private_dns, + private_dns); if (reason != null) { Log.i(TAG, "Changed link properties=" + linkProperties + "DNS cur=" + TextUtils.join(",", dns) + - "DNS prv=" + (last_dns == null ? null : TextUtils.join(",", last_dns))); + "DNS prv=" + (last_dns == null ? null : TextUtils.join(",", last_dns)) + + " private DNS cur=" + private_dns + " prv=" + last_private_dns); last_dns = dns; + last_private_dns = private_dns; reloadAfterNetworkChange(reason); } } diff --git a/app/src/main/java/eu/faircode/netguard/Util.java b/app/src/main/java/eu/faircode/netguard/Util.java index 5ed4b2405..4a7fb0fb3 100644 --- a/app/src/main/java/eu/faircode/netguard/Util.java +++ b/app/src/main/java/eu/faircode/netguard/Util.java @@ -57,6 +57,7 @@ import androidx.core.app.ActivityCompat; import androidx.core.app.NotificationManagerCompat; import androidx.core.net.ConnectivityManagerCompat; +import androidx.preference.PreferenceManager; import net.kollnig.missioncontrol.BuildConfig; import net.kollnig.missioncontrol.R; @@ -572,6 +573,18 @@ public static String getPrivateDnsSpecifier(Context context) { return Settings.Global.getString(context.getContentResolver(), "private_dns_specifier"); } + /** + * Whether the device is left with no working resolver: Private DNS pinned + * to a hostname while we block DoT. Deliberately keyed on the mode rather + * than on the resolver name, which is only decoration for the warning — + * a name that reads back empty must not turn the warning off. + */ + public static boolean isPrivateDnsBlocked(Context context) { + return PreferenceManager.getDefaultSharedPreferences(context) + .getBoolean("block_dot", true) + && isPrivateDnsHostnameMode(context); + } + public interface DoubtListener { void onSure(); } diff --git a/app/src/main/res/layout/main.xml b/app/src/main/res/layout/main.xml index ac8183220..326f2deb0 100644 --- a/app/src/main/res/layout/main.xml +++ b/app/src/main/res/layout/main.xml @@ -64,6 +64,26 @@ android:textColor="?attr/colorOff" android:visibility="gone" /> + + + . + */ + +package eu.faircode.netguard; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertNull; +import static org.junit.Assert.assertTrue; + +import android.content.Context; +import android.content.SharedPreferences; +import android.provider.Settings; + +import androidx.preference.PreferenceManager; + +import org.junit.Before; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.robolectric.RobolectricTestRunner; +import org.robolectric.RuntimeEnvironment; + +/** + * Which Private DNS configurations leave the device with no working resolver. + * + *

Only "hostname" mode does: Android falls back to plaintext DNS when a + * resolver it picked itself cannot be reached over DoT, but not when the user + * named one explicitly, so blocking port 853 stops resolution outright. + */ +@RunWith(RobolectricTestRunner.class) +public class PrivateDnsBlockedTest { + private Context context; + private SharedPreferences prefs; + + @Before + public void setUp() { + context = RuntimeEnvironment.getApplication(); + prefs = PreferenceManager.getDefaultSharedPreferences(context); + prefs.edit().clear().commit(); + setPrivateDns("off", null); + } + + private void setPrivateDns(String mode, String specifier) { + Settings.Global.putString(context.getContentResolver(), "private_dns_mode", mode); + Settings.Global.putString(context.getContentResolver(), "private_dns_specifier", specifier); + } + + @Test + public void pinnedResolverWithDotBlockingBreaksResolution() { + setPrivateDns("hostname", "dns.google"); + assertTrue(Util.isPrivateDnsBlocked(context)); + } + + @Test + public void automaticModeDoesNotBreakResolution() { + // Android falls back to plaintext on its own, which is the whole reason + // the onboarding slide could go. + setPrivateDns("opportunistic", null); + assertFalse(Util.isPrivateDnsBlocked(context)); + } + + @Test + public void privateDnsOffDoesNotBreakResolution() { + setPrivateDns("off", null); + assertFalse(Util.isPrivateDnsBlocked(context)); + } + + @Test + public void researchModeLeavesPinnedResolversAlone() { + // Research mode turns DoT blocking off, so a pinned resolver still works. + setPrivateDns("hostname", "dns.google"); + prefs.edit().putBoolean("block_dot", false).commit(); + assertFalse(Util.isPrivateDnsBlocked(context)); + } + + /** + * The decision must not hinge on reading the specifier: if that read comes + * back empty while the mode says "hostname", resolution is still broken and + * staying silent would leave the user with no explanation at all. + */ + @Test + public void pinnedResolverWithoutReadableNameStillBreaksResolution() { + setPrivateDns("hostname", null); + assertTrue(Util.isPrivateDnsBlocked(context)); + assertNull(Util.getPrivateDnsSpecifier(context)); + } + + @Test + public void specifierIsOnlyReportedForPinnedResolvers() { + setPrivateDns("hostname", "dns.google"); + assertEquals("dns.google", Util.getPrivateDnsSpecifier(context)); + + setPrivateDns("opportunistic", "dns.google"); + assertNull(Util.getPrivateDnsSpecifier(context)); + } +} From 5f158151b4b6583c539af7ed36caf76f60f7ca8c Mon Sep 17 00:00:00 2001 From: Konrad Kollnig <5175206+kasnder@users.noreply.github.com> Date: Wed, 5 Aug 2026 13:58:20 +0200 Subject: [PATCH 2/2] Keep the WireGuard rebind when a burst ends on a private DNS change The debounce collapses a burst of connectivity callbacks to its last reason, which was safe only while every reason restarted WireGuard. The new private-DNS reason does not, so a burst that ends on it now drops the rebind an earlier reason in the same 1500ms window required. Accumulate the need for a rebind across the burst instead of reading it off the surviving reason. Co-Authored-By: Claude Opus 5 --- .../netguard/NetworkReloadPolicy.java | 9 +++++++++ .../eu/faircode/netguard/ServiceSinkhole.java | 14 ++++++++++---- .../netguard/NetworkReloadPolicyTest.java | 19 +++++++++++++++++++ 3 files changed, 38 insertions(+), 4 deletions(-) diff --git a/app/src/main/java/eu/faircode/netguard/NetworkReloadPolicy.java b/app/src/main/java/eu/faircode/netguard/NetworkReloadPolicy.java index b3e526bfd..ded42060b 100644 --- a/app/src/main/java/eu/faircode/netguard/NetworkReloadPolicy.java +++ b/app/src/main/java/eu/faircode/netguard/NetworkReloadPolicy.java @@ -70,6 +70,15 @@ static boolean shouldRestartWireGuard(String reason) { REASON_CONNECTIVITY_CHANGED.equals(reason); } + /** + * The same decision across a coalesced burst of callbacks, which keeps only + * the last reason. The need for a rebind is sticky: once any reason in the + * burst required one, a later reason that does not must not cancel it. + */ + static boolean shouldRestartWireGuard(boolean pendingRestart, String reason) { + return pendingRestart || shouldRestartWireGuard(reason); + } + static boolean same(List last, List current) { if (last == null || current == null || last.size() != current.size()) return false; diff --git a/app/src/main/java/eu/faircode/netguard/ServiceSinkhole.java b/app/src/main/java/eu/faircode/netguard/ServiceSinkhole.java index 32123ab71..7f4532f11 100644 --- a/app/src/main/java/eu/faircode/netguard/ServiceSinkhole.java +++ b/app/src/main/java/eu/faircode/netguard/ServiceSinkhole.java @@ -119,6 +119,7 @@ import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.ExecutorService; import java.util.concurrent.Executors; +import java.util.concurrent.atomic.AtomicBoolean; import java.util.concurrent.locks.ReentrantReadWriteLock; import java.util.zip.GZIPInputStream; @@ -3238,19 +3239,24 @@ public void onLost(Network network) { // ConnectivityManager callbacks within milliseconds of each other. Each // reload is a foreground-service update + wakelock + native VPN restart + // WireGuard rebind, so bursts are coalesced into a single reload using the - // last reason once the burst settles. Every reason string currently in use - // maps to the same shouldRestartWireGuard()==true branch, so collapsing to - // the latest reason changes no behaviour beyond the log line. + // last reason once the burst settles. Not every reason needs the rebind, so + // the need for one is accumulated across the burst rather than read off the + // surviving reason: a reason that does not need it must not cancel one that + // did, or the tunnel keeps a socket bound to a network that is gone. private static final long NETWORK_RELOAD_DEBOUNCE_MS = 1500L; private static final Object NETWORK_RELOAD_TOKEN = new Object(); private final Handler networkReloadDebounceHandler = new Handler(Looper.getMainLooper()); + private final AtomicBoolean pendingWireGuardRestart = new AtomicBoolean(false); private void reloadAfterNetworkChange(final String reason) { + // Callbacks arrive off the main thread; the reload runs on it. + if (NetworkReloadPolicy.shouldRestartWireGuard(reason)) + pendingWireGuardRestart.set(true); networkReloadDebounceHandler.removeCallbacksAndMessages(NETWORK_RELOAD_TOKEN); networkReloadDebounceHandler.postAtTime(new Runnable() { @Override public void run() { - if (NetworkReloadPolicy.shouldRestartWireGuard(reason)) + if (pendingWireGuardRestart.getAndSet(false)) net.kollnig.missioncontrol.wg.WgEgress.INSTANCE.onUnderlyingNetworkChanged(); reload(reason, ServiceSinkhole.this, false); } diff --git a/app/src/test/java/eu/faircode/netguard/NetworkReloadPolicyTest.java b/app/src/test/java/eu/faircode/netguard/NetworkReloadPolicyTest.java index 9928386c4..905a97f86 100644 --- a/app/src/test/java/eu/faircode/netguard/NetworkReloadPolicyTest.java +++ b/app/src/test/java/eu/faircode/netguard/NetworkReloadPolicyTest.java @@ -155,6 +155,25 @@ public void privateDnsChangeDoesNotRestartWireGuard() { assertFalse(NetworkReloadPolicy.shouldRestartWireGuard("private DNS changed")); } + /** + * A burst of callbacks is collapsed to its last reason, but the rebind it + * needs is not a property of that reason alone: a private DNS change + * landing right after a genuine network change must not cancel the rebind + * that change required, or the tunnel keeps a socket bound to a gone + * network until some later event. + */ + @Test + public void privateDnsChangeDoesNotCancelAPendingRestart() { + boolean pending = NetworkReloadPolicy.shouldRestartWireGuard(false, "Network changed"); + assertTrue(pending); + assertTrue(NetworkReloadPolicy.shouldRestartWireGuard(pending, "private DNS changed")); + } + + @Test + public void privateDnsChangeAloneStillDoesNotRestartWireGuard() { + assertFalse(NetworkReloadPolicy.shouldRestartWireGuard(false, "private DNS changed")); + } + @Test public void physicalConnectivityReloadsRestartWireGuard() { assertTrue(NetworkReloadPolicy.shouldRestartWireGuard("network available"));