From 82c5fec2f1b494e2c198e094e097565959acf535 Mon Sep 17 00:00:00 2001 From: Konrad Kollnig <5175206+kasnder@users.noreply.github.com> Date: Sat, 11 Jul 2026 10:09:04 +0200 Subject: [PATCH 1/3] Fix weekly review regressions --- .../eu/faircode/netguard/ServiceSinkhole.java | 52 +++++++++---------- .../missioncontrol/data/BlockingMode.java | 19 ++++--- app/src/main/res/values-sl-rSI/strings.xml | 2 +- .../netguard/VpnRevokePolicyTest.java | 26 ++++++++++ .../data/BlockingModeAndroidTest.java | 8 +++ 5 files changed, 71 insertions(+), 36 deletions(-) create mode 100644 app/src/test/java/eu/faircode/netguard/VpnRevokePolicyTest.java diff --git a/app/src/main/java/eu/faircode/netguard/ServiceSinkhole.java b/app/src/main/java/eu/faircode/netguard/ServiceSinkhole.java index 5aa8560fd..735d53465 100644 --- a/app/src/main/java/eu/faircode/netguard/ServiceSinkhole.java +++ b/app/src/main/java/eu/faircode/netguard/ServiceSinkhole.java @@ -3208,34 +3208,26 @@ private void set(Intent intent) { public void onRevoke() { Log.i(TAG, "Revoke"); - // Android does not tell us *why* onRevoke() fired. The system calls it - // identically for (a) a deliberate user disable from the system VPN - // settings, (b) another VPN app taking the single VPN slot (#331), and - // (c) an OS-initiated teardown/re-auth of the always-on slot — Doze, - // memory pressure, an OS update — which is the "always-on VPN turns - // itself off after 3-5 days" report (#526). - // - // We deliberately do NOT persist enabled=false here. Every genuine - // in-app disable path (the main toggle in ActivityMain, the quick- - // settings tile in ServiceTileMain, and the widget in WidgetAdmin) - // already sets enabled=false *itself* before stopping the service, and - // tearing down our own tunnel does not trigger onRevoke(). So a revoke - // reaching this method is always externally initiated. Clearing - // "enabled" here used to turn every such external teardown into a - // permanent self-disable with no recovery: the service is START_STICKY - // and onStartCommand() keys off "enabled", so once it is false every - // subsequent (re)start immediately stops again. - // - // Leaving "enabled" untouched lets the existing recovery mechanisms - // re-establish the tunnel once TC reacquires the VPN slot: the OS - // restarting the always-on service, ReceiverAutostart on boot, the - // watchdog alarm, and reopening ActivityMain (which calls start() when - // enabled). We do not add any retry loop here, so a second VPN that - // legitimately holds the slot is not thrashed — recovery is driven only - // by those OS-paced triggers. The remaining tradeoff is that a user who - // disables TC via the *system* VPN settings (rather than the in-app - // toggle) will see it come back on the next such trigger; the in-app - // toggle remains the supported way to turn TC off. + // Preserve the enabled state only while Android still designates us as + // the always-on VPN. In that case a transient system teardown should be + // recoverable. A deliberate revoke in system settings, or another VPN + // taking the slot, clears the always-on designation and must disable TC; + // otherwise the repeating watchdog would keep trying to reclaim the VPN. + boolean alwaysOn = false; + try { + if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.Q) + alwaysOn = isAlwaysOn(); + else + alwaysOn = getPackageName().equals(Settings.Secure.getString( + getContentResolver(), "always_on_vpn_app")); + } catch (Throwable ex) { + Log.w(TAG, "Could not determine always-on VPN state", ex); + } + + SharedPreferences prefs = PreferenceManager.getDefaultSharedPreferences(this); + boolean enabled = resolveEnabledAfterRevoke(prefs.getBoolean("enabled", false), alwaysOn); + prefs.edit().putBoolean("enabled", enabled).apply(); + Log.i(TAG, "Revoke always-on=" + alwaysOn + " enabled=" + enabled); // Feedback: the tunnel really did drop, so inform the user (mirrors // Android's own "VPN disconnected" notice). This does not disable TC. @@ -3245,6 +3237,10 @@ public void onRevoke() { super.onRevoke(); } + static boolean resolveEnabledAfterRevoke(boolean enabled, boolean alwaysOn) { + return enabled && alwaysOn; + } + @Override public void onDestroy() { synchronized (this) { diff --git a/app/src/main/java/net/kollnig/missioncontrol/data/BlockingMode.java b/app/src/main/java/net/kollnig/missioncontrol/data/BlockingMode.java index 489d96d71..3aadcfa4f 100644 --- a/app/src/main/java/net/kollnig/missioncontrol/data/BlockingMode.java +++ b/app/src/main/java/net/kollnig/missioncontrol/data/BlockingMode.java @@ -80,17 +80,22 @@ public static boolean isStrictMode(Context c) { } /** - * Minimal mode normally forces tracker protection on for apps that are in - * the VPN route. Browsers are the exception: they stay routed for VPN / - * WireGuard privacy, but app-level tracker blocking defaults off because - * browser-native blocking is usually more precise. + * Tracker protection defaults on for ordinary apps and off for browsers, + * where browser-native blocking is usually more precise. An explicit + * per-package value always wins, including in Minimal mode, so bulk + * include/exclude actions have the behavior promised by the settings UI. */ public static boolean isTrackerProtectionEnabled(Context c, SharedPreferences trackerProtectPrefs, String packageName) { - if (isBrowserApp(c, packageName)) - return trackerProtectPrefs.getBoolean(packageName, false); - return isMinimalMode(c) || trackerProtectPrefs.getBoolean(packageName, true); + Boolean configured = trackerProtectPrefs.contains(packageName) + ? trackerProtectPrefs.getBoolean(packageName, true) + : null; + return resolveTrackerProtection(isBrowserApp(c, packageName), configured); + } + + static boolean resolveTrackerProtection(boolean browser, Boolean configured) { + return configured == null ? !browser : configured; } /** diff --git a/app/src/main/res/values-sl-rSI/strings.xml b/app/src/main/res/values-sl-rSI/strings.xml index d5c0a2f0d..abd39a11e 100644 --- a/app/src/main/res/values-sl-rSI/strings.xml +++ b/app/src/main/res/values-sl-rSI/strings.xml @@ -204,7 +204,7 @@ TrackerControl ustvari WireGuardov profil Mulvad. Številka računa se shrani krajevno za bodoče profile Mulvad. Žetoni se ne shranjujejo in in izbrisa tukajšnjega profila ne odstrani naprave iz Mullvada. Ustvarjanje profila WireGuard … Nastavitev Mullvada je spodletela: %s - + Držav Mullvad ni bilo mogoče naložiti: %s %s Mullvad - %s Mullvad diff --git a/app/src/test/java/eu/faircode/netguard/VpnRevokePolicyTest.java b/app/src/test/java/eu/faircode/netguard/VpnRevokePolicyTest.java new file mode 100644 index 000000000..4a92a2e36 --- /dev/null +++ b/app/src/test/java/eu/faircode/netguard/VpnRevokePolicyTest.java @@ -0,0 +1,26 @@ +package eu.faircode.netguard; + +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertTrue; + +import org.junit.Test; +import org.junit.runner.RunWith; +import org.robolectric.RobolectricTestRunner; + +@RunWith(RobolectricTestRunner.class) +public class VpnRevokePolicyTest { + @Test + public void alwaysOnRevokeKeepsEnabledStateForRecovery() { + assertTrue(ServiceSinkhole.resolveEnabledAfterRevoke(true, true)); + } + + @Test + public void ordinaryRevokeDisablesWatchdogRecovery() { + assertFalse(ServiceSinkhole.resolveEnabledAfterRevoke(true, false)); + } + + @Test + public void revokeNeverEnablesAnAlreadyDisabledService() { + assertFalse(ServiceSinkhole.resolveEnabledAfterRevoke(false, true)); + } +} diff --git a/app/src/test/java/net/kollnig/missioncontrol/data/BlockingModeAndroidTest.java b/app/src/test/java/net/kollnig/missioncontrol/data/BlockingModeAndroidTest.java index 7c1dc2d1d..c2c232d7c 100644 --- a/app/src/test/java/net/kollnig/missioncontrol/data/BlockingModeAndroidTest.java +++ b/app/src/test/java/net/kollnig/missioncontrol/data/BlockingModeAndroidTest.java @@ -87,6 +87,14 @@ public void explicitVpnInclusionWithRealExcludedAppsStopsAutoManagement() throws assertFalse(result.autoExcludedApps.contains("com.whatsapp")); } + @Test + public void explicitTrackerProtectionOverridesMinimalModeDefaults() { + assertTrue(BlockingMode.resolveTrackerProtection(false, null)); + assertFalse(BlockingMode.resolveTrackerProtection(true, null)); + assertFalse(BlockingMode.resolveTrackerProtection(false, false)); + assertTrue(BlockingMode.resolveTrackerProtection(true, true)); + } + private static String readExcludedAppsAsset() throws IOException { return new String(Files.readAllBytes(assetPath("ddg-excluded-apps.json")), StandardCharsets.UTF_8); } From afdfed6fe594a79bbbb2694bc41060ced4ad8976 Mon Sep 17 00:00:00 2001 From: Konrad Kollnig <5175206+kasnder@users.noreply.github.com> Date: Sat, 11 Jul 2026 13:15:48 +0200 Subject: [PATCH 2/3] Bias onRevoke toward keeping VPN enabled on unknown always-on state The pre-Q revoke path reads the @hide "always_on_vpn_app" Settings.Secure key, which returns null on some OEM/OS builds even when TC is the configured always-on VPN. Treating that null (or a throwing lookup) as "not always-on" would persist enabled=false and permanently self-disable TC on every revoke for those users. Make the always-on lookup nullable and have resolveEnabledAfterRevoke keep the existing enabled state when the always-on designation is unknown, only persisting a disable when always-on is positively known to be off. Positively-known true/false behavior is unchanged. Adds truth- table tests for the null/unknown case and documents the timing assumption inline. Co-Authored-By: Claude Opus 4.8 --- .../eu/faircode/netguard/ServiceSinkhole.java | 27 +++++++++++++++---- .../netguard/VpnRevokePolicyTest.java | 14 ++++++++++ 2 files changed, 36 insertions(+), 5 deletions(-) diff --git a/app/src/main/java/eu/faircode/netguard/ServiceSinkhole.java b/app/src/main/java/eu/faircode/netguard/ServiceSinkhole.java index 735d53465..4ec85b59c 100644 --- a/app/src/main/java/eu/faircode/netguard/ServiceSinkhole.java +++ b/app/src/main/java/eu/faircode/netguard/ServiceSinkhole.java @@ -3213,15 +3213,27 @@ public void onRevoke() { // recoverable. A deliberate revoke in system settings, or another VPN // taking the slot, clears the always-on designation and must disable TC; // otherwise the repeating watchdog would keep trying to reclaim the VPN. - boolean alwaysOn = false; + // + // Correctness in both directions (staying on for #526 recovery, and + // honoring a deliberate system-settings disable) depends on the OS + // reporting the always-on designation correctly right here at revoke + // time. On pre-Q devices the "always_on_vpn_app" Settings.Secure key is + // @hide and some OEM/OS builds return null even when TC *is* the + // configured always-on VPN, or the lookup can throw. That null/unknown + // case is deliberately biased toward keeping the VPN enabled: only a + // positively-known alwaysOn=false is allowed to persist enabled=false. + Boolean alwaysOn = null; try { if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.Q) alwaysOn = isAlwaysOn(); - else - alwaysOn = getPackageName().equals(Settings.Secure.getString( - getContentResolver(), "always_on_vpn_app")); + else { + String alwaysOnPackage = Settings.Secure.getString( + getContentResolver(), "always_on_vpn_app"); + alwaysOn = (alwaysOnPackage == null) ? null : getPackageName().equals(alwaysOnPackage); + } } catch (Throwable ex) { Log.w(TAG, "Could not determine always-on VPN state", ex); + alwaysOn = null; } SharedPreferences prefs = PreferenceManager.getDefaultSharedPreferences(this); @@ -3237,7 +3249,12 @@ public void onRevoke() { super.onRevoke(); } - static boolean resolveEnabledAfterRevoke(boolean enabled, boolean alwaysOn) { + static boolean resolveEnabledAfterRevoke(boolean enabled, Boolean alwaysOn) { + // alwaysOn == null means the always-on state could not be positively + // determined (see onRevoke comment above). Do not force a disable in + // that case: keep whatever enabled state was already persisted. + if (alwaysOn == null) + return enabled; return enabled && alwaysOn; } diff --git a/app/src/test/java/eu/faircode/netguard/VpnRevokePolicyTest.java b/app/src/test/java/eu/faircode/netguard/VpnRevokePolicyTest.java index 4a92a2e36..ca4b11de3 100644 --- a/app/src/test/java/eu/faircode/netguard/VpnRevokePolicyTest.java +++ b/app/src/test/java/eu/faircode/netguard/VpnRevokePolicyTest.java @@ -23,4 +23,18 @@ public void ordinaryRevokeDisablesWatchdogRecovery() { public void revokeNeverEnablesAnAlreadyDisabledService() { assertFalse(ServiceSinkhole.resolveEnabledAfterRevoke(false, true)); } + + @Test + public void unknownAlwaysOnStateKeepsEnabledStateForRecovery() { + // A null/unreadable always-on lookup (e.g. the hidden pre-Q + // "always_on_vpn_app" setting returning null on some OEM builds even + // though TC is the configured always-on VPN) must not force a + // disable: the state is left as-is, biased toward staying enabled. + assertTrue(ServiceSinkhole.resolveEnabledAfterRevoke(true, null)); + } + + @Test + public void unknownAlwaysOnStateNeverEnablesAnAlreadyDisabledService() { + assertFalse(ServiceSinkhole.resolveEnabledAfterRevoke(false, null)); + } } From 30d96bf6da5c217ef1e7ac9322fdc1ec3c76601a Mon Sep 17 00:00:00 2001 From: Konrad Kollnig <5175206+kasnder@users.noreply.github.com> Date: Sat, 11 Jul 2026 23:54:38 +0200 Subject: [PATCH 3/3] Split VPN revoke policy from weekly fixes --- .../eu/faircode/netguard/ServiceSinkhole.java | 67 ++++++++----------- .../netguard/VpnRevokePolicyTest.java | 40 ----------- 2 files changed, 27 insertions(+), 80 deletions(-) delete mode 100644 app/src/test/java/eu/faircode/netguard/VpnRevokePolicyTest.java diff --git a/app/src/main/java/eu/faircode/netguard/ServiceSinkhole.java b/app/src/main/java/eu/faircode/netguard/ServiceSinkhole.java index 4ec85b59c..5aa8560fd 100644 --- a/app/src/main/java/eu/faircode/netguard/ServiceSinkhole.java +++ b/app/src/main/java/eu/faircode/netguard/ServiceSinkhole.java @@ -3208,38 +3208,34 @@ private void set(Intent intent) { public void onRevoke() { Log.i(TAG, "Revoke"); - // Preserve the enabled state only while Android still designates us as - // the always-on VPN. In that case a transient system teardown should be - // recoverable. A deliberate revoke in system settings, or another VPN - // taking the slot, clears the always-on designation and must disable TC; - // otherwise the repeating watchdog would keep trying to reclaim the VPN. + // Android does not tell us *why* onRevoke() fired. The system calls it + // identically for (a) a deliberate user disable from the system VPN + // settings, (b) another VPN app taking the single VPN slot (#331), and + // (c) an OS-initiated teardown/re-auth of the always-on slot — Doze, + // memory pressure, an OS update — which is the "always-on VPN turns + // itself off after 3-5 days" report (#526). // - // Correctness in both directions (staying on for #526 recovery, and - // honoring a deliberate system-settings disable) depends on the OS - // reporting the always-on designation correctly right here at revoke - // time. On pre-Q devices the "always_on_vpn_app" Settings.Secure key is - // @hide and some OEM/OS builds return null even when TC *is* the - // configured always-on VPN, or the lookup can throw. That null/unknown - // case is deliberately biased toward keeping the VPN enabled: only a - // positively-known alwaysOn=false is allowed to persist enabled=false. - Boolean alwaysOn = null; - try { - if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.Q) - alwaysOn = isAlwaysOn(); - else { - String alwaysOnPackage = Settings.Secure.getString( - getContentResolver(), "always_on_vpn_app"); - alwaysOn = (alwaysOnPackage == null) ? null : getPackageName().equals(alwaysOnPackage); - } - } catch (Throwable ex) { - Log.w(TAG, "Could not determine always-on VPN state", ex); - alwaysOn = null; - } - - SharedPreferences prefs = PreferenceManager.getDefaultSharedPreferences(this); - boolean enabled = resolveEnabledAfterRevoke(prefs.getBoolean("enabled", false), alwaysOn); - prefs.edit().putBoolean("enabled", enabled).apply(); - Log.i(TAG, "Revoke always-on=" + alwaysOn + " enabled=" + enabled); + // We deliberately do NOT persist enabled=false here. Every genuine + // in-app disable path (the main toggle in ActivityMain, the quick- + // settings tile in ServiceTileMain, and the widget in WidgetAdmin) + // already sets enabled=false *itself* before stopping the service, and + // tearing down our own tunnel does not trigger onRevoke(). So a revoke + // reaching this method is always externally initiated. Clearing + // "enabled" here used to turn every such external teardown into a + // permanent self-disable with no recovery: the service is START_STICKY + // and onStartCommand() keys off "enabled", so once it is false every + // subsequent (re)start immediately stops again. + // + // Leaving "enabled" untouched lets the existing recovery mechanisms + // re-establish the tunnel once TC reacquires the VPN slot: the OS + // restarting the always-on service, ReceiverAutostart on boot, the + // watchdog alarm, and reopening ActivityMain (which calls start() when + // enabled). We do not add any retry loop here, so a second VPN that + // legitimately holds the slot is not thrashed — recovery is driven only + // by those OS-paced triggers. The remaining tradeoff is that a user who + // disables TC via the *system* VPN settings (rather than the in-app + // toggle) will see it come back on the next such trigger; the in-app + // toggle remains the supported way to turn TC off. // Feedback: the tunnel really did drop, so inform the user (mirrors // Android's own "VPN disconnected" notice). This does not disable TC. @@ -3249,15 +3245,6 @@ public void onRevoke() { super.onRevoke(); } - static boolean resolveEnabledAfterRevoke(boolean enabled, Boolean alwaysOn) { - // alwaysOn == null means the always-on state could not be positively - // determined (see onRevoke comment above). Do not force a disable in - // that case: keep whatever enabled state was already persisted. - if (alwaysOn == null) - return enabled; - return enabled && alwaysOn; - } - @Override public void onDestroy() { synchronized (this) { diff --git a/app/src/test/java/eu/faircode/netguard/VpnRevokePolicyTest.java b/app/src/test/java/eu/faircode/netguard/VpnRevokePolicyTest.java deleted file mode 100644 index ca4b11de3..000000000 --- a/app/src/test/java/eu/faircode/netguard/VpnRevokePolicyTest.java +++ /dev/null @@ -1,40 +0,0 @@ -package eu.faircode.netguard; - -import static org.junit.Assert.assertFalse; -import static org.junit.Assert.assertTrue; - -import org.junit.Test; -import org.junit.runner.RunWith; -import org.robolectric.RobolectricTestRunner; - -@RunWith(RobolectricTestRunner.class) -public class VpnRevokePolicyTest { - @Test - public void alwaysOnRevokeKeepsEnabledStateForRecovery() { - assertTrue(ServiceSinkhole.resolveEnabledAfterRevoke(true, true)); - } - - @Test - public void ordinaryRevokeDisablesWatchdogRecovery() { - assertFalse(ServiceSinkhole.resolveEnabledAfterRevoke(true, false)); - } - - @Test - public void revokeNeverEnablesAnAlreadyDisabledService() { - assertFalse(ServiceSinkhole.resolveEnabledAfterRevoke(false, true)); - } - - @Test - public void unknownAlwaysOnStateKeepsEnabledStateForRecovery() { - // A null/unreadable always-on lookup (e.g. the hidden pre-Q - // "always_on_vpn_app" setting returning null on some OEM builds even - // though TC is the configured always-on VPN) must not force a - // disable: the state is left as-is, biased toward staying enabled. - assertTrue(ServiceSinkhole.resolveEnabledAfterRevoke(true, null)); - } - - @Test - public void unknownAlwaysOnStateNeverEnablesAnAlreadyDisabledService() { - assertFalse(ServiceSinkhole.resolveEnabledAfterRevoke(false, null)); - } -}