Codebase Audit and Bug Fixes for VPN State, Detection, and Lifecycle - #34
Conversation
- Implement explicit ServiceState enum in TunnelGuardVpnService (NO_VPN, TUNNELGUARD_STARTING, TUNNELGUARD_ACTIVE, TUNNELGUARD_STOPPING, UPSTREAM_VPN, VPN_CONFLICT, ERROR) to track and log transitions safely. - Enhance upstream VPN detection in TunnelGuardConfig to utilize ownerUid checks on API 30+ (avoiding fragile hardcoded IP addresses) and isTunnelEstablished checks on older platforms. - Optimize active app monitoring loop to avoid starting or to stop immediately when disabled or missing permissions, preventing CPU/battery drain. - Guarantee zero coroutine leaks by canceling the serviceScope context job in onDestroy(). - Add comprehensive unit tests verifying the state machine and dual-path detection.
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
|
Warning Review limit reached
Next review available in: 31 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe VPN service now exposes lifecycle states, records state transitions across routing and shutdown paths, cancels its service scope on destruction, and improves self-VPN detection across Android versions with tests. ChangesVPN state and detection
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant TunnelGuardVpnService
participant TunnelGuardConfig
participant ConnectivityManager
TunnelGuardVpnService->>TunnelGuardConfig: detectRealVpnCapabilities
TunnelGuardConfig->>ConnectivityManager: inspect active network capabilities
ConnectivityManager-->>TunnelGuardConfig: VPN networks and owner information
alt Android 30+
TunnelGuardConfig->>TunnelGuardConfig: exclude current-process VPN
else Older Android
TunnelGuardConfig->>TunnelGuardVpnService: check isTunnelEstablished
TunnelGuardVpnService-->>TunnelGuardConfig: tunnel state
end
TunnelGuardConfig-->>TunnelGuardVpnService: real VPN capabilities
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/src/main/java/com/tunnelguard/app/TunnelGuardConfig.kt`:
- Around line 61-75: The Android R+ branch in detectRealVpnCapabilities must
return true immediately when caps.ownerUid differs from Process.myUid(), without
calling isOurOurVpn; retain address matching only for pre-R devices. Update
app/src/main/java/com/tunnelguard/app/TunnelGuardConfig.kt lines 61-75
accordingly, and add the requested R+ regression test in
app/src/test/java/com/tunnelguard/app/TunnelGuardConfigRobolectricTest.kt lines
120-126 using a non-own UID and a matching LinkAddress.
In `@app/src/main/java/com/tunnelguard/app/TunnelGuardVpnService.kt`:
- Around line 107-113: Replace the unsynchronized
updateServiceState/currentServiceState mutation with a shared lock used by
transitionTo(), the ACTION_STOP teardown path, and checkAndRunVpnRouting(). Hold
that lock across state changes and stopVpn() so routing cannot publish an active
state or recreate an interface after shutdown, and keep the companion state
write protected by the same synchronization.
- Around line 161-169: Update the monitoring loop started by startMonitoring so
it rechecks app-monitor, Usage Access, and overlay permissions while running;
when the condition becomes false, cancel and clear monitorJob and exit the loop
or coroutine. Preserve the existing detection behavior while all permissions
remain available, and ensure the existing startMonitoring guard remains
consistent.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: f5cab381-774f-467b-9e83-7787dced9e62
📒 Files selected for processing (3)
app/src/main/java/com/tunnelguard/app/TunnelGuardConfig.ktapp/src/main/java/com/tunnelguard/app/TunnelGuardVpnService.ktapp/src/test/java/com/tunnelguard/app/TunnelGuardConfigRobolectricTest.kt
… dynamic app monitoring checks - Update detectRealVpnCapabilities on Android R+ to immediately return true when caps.ownerUid differs from Process.myUid(), bypassing link address checks. - Add companion stateLock in TunnelGuardVpnService and synchronize all ServiceState updates, stopVpn(), ACTION_STOP teardown paths, and checkAndRunVpnRouting() using the shared lock. - Refactor startMonitoring loop to dynamically re-evaluate app-monitor status and required permissions, stopping cleanly on failure. - Add regression unit test in TunnelGuardConfigRobolectricTest verifying correct R+ ownerUid detection and bypass behavior.
…nce detectRealVpnCapabilities - Resolve SuspiciousIndentation lint failure in startMonitoring loop inside TunnelGuardVpnService.kt. - Ensure the Android R+ branch of detectRealVpnCapabilities in TunnelGuardConfig.kt immediately returns true when caps.ownerUid differs from Process.myUid(), bypassing link address comparisons. - Implement thread safety using shared companion stateLock monitor across all ServiceState updates, stopVpn(), ACTION_STOP teardown paths, and checkAndRunVpnRouting() executions. - Enhance startMonitoring loop to dynamically re-evaluate app-monitoring configurations and system-overlay/Usage Stats permissions, cleanly exiting if revoked. - Include robust Robolectric unit tests for R+ regression and legacy detection fallback scenarios.
1 participant
Comprehensive codebase audit and bug fixes addressing VPN lifecycle state tracking, upstream VPN detection/distinction, and background coroutine resource leaks in TunnelGuard.
PR created automatically by Jules for task 4469103161600589584 started by @DisabledAbel
Summary by CodeRabbit
Bug Fixes
Tests