From b8f1a182cb4a775ef9e7aec61bf1b693450859db Mon Sep 17 00:00:00 2001 From: Konrad Kollnig <5175206+kasnder@users.noreply.github.com> Date: Sat, 11 Jul 2026 23:58:36 +0200 Subject: [PATCH] Handle external VPN revocation policy --- .../eu/faircode/netguard/ServiceSinkhole.java | 59 ++++++++++--------- .../netguard/VpnRevokePolicyTest.java | 36 +++++++++++ 2 files changed, 68 insertions(+), 27 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 8c1bc9daf..c70b601ee 100644 --- a/app/src/main/java/eu/faircode/netguard/ServiceSinkhole.java +++ b/app/src/main/java/eu/faircode/netguard/ServiceSinkhole.java @@ -3291,34 +3291,33 @@ 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). + // 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. // - // 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. + // Correctness in both directions depends on the OS reporting the + // always-on designation correctly at revoke time. On pre-Q devices the + // hidden setting may be unavailable, so unknown is biased toward keeping + // the existing state; only a positively-known false disables TC. + 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); + } + + 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. @@ -3328,6 +3327,12 @@ public void onRevoke() { super.onRevoke(); } + static boolean resolveEnabledAfterRevoke(boolean enabled, Boolean alwaysOn) { + 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 new file mode 100644 index 000000000..5b4ac4b20 --- /dev/null +++ b/app/src/test/java/eu/faircode/netguard/VpnRevokePolicyTest.java @@ -0,0 +1,36 @@ +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() { + assertTrue(ServiceSinkhole.resolveEnabledAfterRevoke(true, null)); + } + + @Test + public void unknownAlwaysOnStateNeverEnablesAnAlreadyDisabledService() { + assertFalse(ServiceSinkhole.resolveEnabledAfterRevoke(false, null)); + } +}