Wire the dormant allowBypass pref into the VPN builder; add phone-UI toggle (fixes wireless Android Auto) - #3057
kazimurtaza wants to merge 5 commits into
Conversation
…toggle The allowBypass persistent preference and its settings strings exist, and the TV build exposes a toggle, but BraveVPNService never calls VpnService.Builder.allowBypass() — the pref is dead on phones. Wireless Android Auto refuses to negotiate its Wi-Fi-Direct transport when the default network is a non-bypassable VPN, so Rethink users cannot use wireless AA at all (workarounds like app-level Exclude don't help; the check inspects the network object, not tunnel membership). - BraveVPNService.newBuilder(): call builder.allowBypass() when the pref is set and VPN is not in lockdown mode (platform forbids bypass in lockdown) - BraveVPNService.onSharedPreferenceChanged: restart the tunnel on change, like the privateIps toggle - TunnelSettingsActivity + layout: 'Enable network visibility' toggle using the already-shipped settings_allow_bypass_heading/_desc strings Fixes celzero#2685
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 8 included reviews per hour; 4 remain after this review. 📜 Recent review details🔇 Additional comments (1)
📝 WalkthroughWalkthroughAdds a tunnel settings row and switch for app bypass. The preference is persisted in ChangesTunnel bypass setting
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to This change enables an opt-in VPN bypass setting while preserving the existing default behavior, with no actionable merge-blocking risk remaining. Sequence Diagram(s)sequenceDiagram
participant TunnelSettingsActivity
participant PersistentState
participant BraveVPNService
TunnelSettingsActivity->>PersistentState: Save allowBypass
PersistentState->>BraveVPNService: Report ALLOW_BYPASS change
BraveVPNService->>BraveVPNService: Request debounced VPN restart
BraveVPNService->>PersistentState: Read allowBypass
BraveVPNService->>BraveVPNService: Call allowBypass when lockdown is inactive
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Full details: Linked Issues checkExplanation The changes directly address issue Full details: Out of Scope Changes checkExplanation The changes remain within scope. The layout, preference key, VPN builder behavior, preference-change restart, and phone UI toggle all support the Android Auto VPN-bypass objective. Documentation comments and restored declarations are supporting maintenance changes. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Addresses the docstring-coverage pre-merge check on PR celzero#3057. Refs celzero#2685
Brings docstring coverage over the pre-merge threshold on PR celzero#3057. Refs celzero#2685
Completes docstring coverage for the diff context on PR celzero#3057. Refs celzero#2685
Previous commit's scripted insertion mangled the declarations; restored them and re-verified the build compiles. Refs celzero#2685
|
@coderabbitai resume |
✅ Action performedReviews resumed. |
|
Wouldn't this allow any app to initiate a VPN bypass? Is there a way to only allow it from certain callers that the user selects in RethinkDNS? So they can allow Android Auto to bypass VPN, but not e.g. Facebook. |
|
Good question. Three parts to the answer. What allowBypass actually changes: bypass stays on-demand and per socket. An app only side-steps the tunnel if it explicitly binds to a non-VPN network, for example via The honest trade-off: yes, any app that does implement those calls can then use the underlying network directly. That is inherent to the platform API: On selecting specific apps: the stronger per-app form already exists as Configure, Apps, Exclude, which removes an app from the tunnel entirely ( One further limitation worth naming: once the flag is on, Rethink cannot see or log which apps bypass, because bypassed traffic never enters the tunnel. So a per-app prompt or audit log is not something this change could layer on top; that would need platform support that does not exist today. |
What
Wires the dormant
allowBypasspersistent preference into the VPN builder and adds the corresponding phone-UI toggle (the TV build already ships a toggle for this pref; the stringssettings_allow_bypass_heading/_deschave been invalues/strings.xmlall along — only the wire-up was missing).Fixes #2685.
Root cause
PersistentState.allowBypassexists (allow_bypass, defaultfalse) andSettingsScreen.kt(TV) writes it, butBraveVPNServicenever reads it —VpnService.Builder.allowBypass()is never called, so the established VPN is always non-bypassable:Wireless Android Auto negotiates its transport over Wi-Fi-Direct and checks the default network object: when it is a non-bypassable VPN, the session is refused regardless of per-app configuration:
App-level workarounds (Exclude / Bypass DNS & Firewall for
com.google.android.projection.gearhead) do not help because the check inspects the network, not tunnel membership. Withbuilder.allowBypass()called at establish, the platform surfacesbypassable=trueand apps that explicitly opt out (bindProcessToNetwork, as AA's transport does) can use the underlying network — the documented purpose ofVpnService.Builder.allowBypass().Changes
BraveVPNService.newBuilder(): callbuilder.allowBypass()when the pref is enabled, skipped under VPN lockdown (the platform disallows bypass in lockdown mode — mirrors the existing excluded-apps guard).BraveVPNService.onSharedPreferenceChanged(): restart the tunnel when the pref changes (same one-liner as thePRIVATE_IPSbranch —allowBypass()is additive, so disabling requires a rebuild of the builder).TunnelSettingsActivity+activity_tunnel_settings.xml: new "Enable network visibility" toggle row cloned from the lan-traffic row, using the existing strings.No behavior change by default (
allowBypassdefaults tofalse; opt-in via the new toggle).Note: with this change the TV build's toggle becomes functional too (it currently writes a pref nothing reads). A follow-up could switch its hardcoded labels to
settings_allow_bypass_heading.Testing
Built
assembleFdroidFullReleaseDebug, installed on a Pixel 8 Pro (Android 17), Rethink active:ALLOW_BYPASSpref branch).dumpsys connectivityshows the VPNbypassable=trueafter the restart.doubleclick.net→127.0.0.1).Summary by CodeRabbit
New Features
Bug Fixes