Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[BUG] - Problem with app: Excluded Apps get reset #465

Open
Uinsart opened this issue Nov 27, 2024 · 11 comments
Open

[BUG] - Problem with app: Excluded Apps get reset #465

Uinsart opened this issue Nov 27, 2024 · 11 comments
Assignees
Labels
bug Something isn't working

Comments

@Uinsart
Copy link

Uinsart commented Nov 27, 2024

Describe the bug
If I exclude apps from the tunnel via the edit tunnel options and save them, the setting gets reset by the next connection change.
So, I exclude app, save the change, switch from WiFi to mobile, the tunnel gets activated. Apps are still excluded. When I switch back to WiFi and the tunnel shuts off, the excluded apps are reset.

Smartphone (please complete the following information):

  • Device: Google Pixel 6
  • Android Version: Android 15
  • App Version 3.6.1
  • Backend: Kernel

To Reproduce
Steps to reproduce the behavior:

  1. Go to Tunnel Settings
  2. Click on edit
  3. Exclude apps
  4. Save
  5. Go back to main screen
  6. Switch to mobile data. Tunnel gets activated
  7. Check excluded apps, still there
  8. Switch to WiFi, tunnel gets deactivated
  9. Check excluded apps, all apps are getting tunneled.

Expected behavior
I expect the settings to stay after a connection change.

Screenshots (Only if necessary)

Additional context
IMHO it worked till a few days ago.

@Uinsart Uinsart added the bug Something isn't working label Nov 27, 2024
@zaneschepke
Copy link
Owner

Hello! Thank you for the report. I'll try to reproduce this.

@Uinsart
Copy link
Author

Uinsart commented Nov 29, 2024

Hello! Thank you for the report. I'll try to reproduce this.

Hmu if you need more info

@bmonjoie
Copy link

bmonjoie commented Dec 8, 2024

I have the exact same bug.

Device: Google Pixel 8 Pro
Android Version: Android 15
App Version 3.6.1

@Dentic89
Copy link

Dentic89 commented Dec 8, 2024

I can confirm this on a Google Pixel 7 Pro. I excluded Android Auto because it doesn't work with VPN. After switching networks it resets to "Tunneling apps: all"

@zaneschepke
Copy link
Owner

Hello all! Thank you for the report and the info. I'll look into a fix for this.

@bmonjoie
Copy link

bmonjoie commented Dec 11, 2024

Hello @zaneschepke, I might be completely wrong, but I believe I have found the issue.

Unless I misunderstood, the AutoTunnelService keeps a list of configured tunnels in its autoTunnelStateFlow StateFlow fed by a Flow coming from the database.
This list of configured tunnels is then used to save which one is active or not in the database.
Yet, the Flow coming from the database has a distinctUntilChange that only checks for whether the tunnel is active or not: https://github.com/zaneschepke/wgtunnel/blob/main/app/src/main/java/com/zaneschepke/wireguardautotunnel/service/foreground/autotunnel/AutoTunnelService.kt#L273

Therefore, if the user creates a tunnel without excluding any applications, it is added to the autoTunnelStateFlow as is. If the user adds excluded applications, the autoTunnelStateFlow is not updated (because of the distinctUntilChanged). Finally, when the AutoTunnelService sets the tunnel's value to active, it uses its cached configuration (without excluded applications), which overrides the excluded applications because the method used to change the value to active is a complete update of the TunnelConfig object.

Hopefully, what I have said makes sense, and the solution would be to change the following code from:

appDataRepository.get().tunnels.getTunnelConfigsFlow().distinctUntilChanged { old, new ->
				old.map { it.isActive } != new.map { it.isActive }
			},

to:

appDataRepository.get().tunnels.getTunnelConfigsFlow().distinctUntilChanged(),

Edit: Another solution would be to change those lines:

To only update the isActive in the database rather than save everything.

@Dentic89
Copy link

Hello @zaneschepke, I might be completely wrong, but I believe I have found the issue.

Unless I misunderstood, the AutoTunnelService keeps a list of configured tunnels in its autoTunnelStateFlow StateFlow fed by a Flow coming from the database. This list of configured tunnels is then used to save which one is active or not in the database. Yet, the Flow coming from the database has a distinctUntilChange that only checks for whether the tunnel is active or not: https://github.com/zaneschepke/wgtunnel/blob/main/app/src/main/java/com/zaneschepke/wireguardautotunnel/service/foreground/autotunnel/AutoTunnelService.kt#L273

Therefore, if the user creates a tunnel without excluding any applications, it is added to the autoTunnelStateFlow as is. If the user adds excluded applications, the autoTunnelStateFlow is not updated (because of the distinctUntilChanged). Finally, when the AutoTunnelService sets the tunnel's value to active, it uses its cached configuration (without excluded applications), which overrides the excluded applications because the method used to change the value to active is a complete update of the TunnelConfig object.

Hopefully, what I have said makes sense, and the solution would be to change the following code from:

appDataRepository.get().tunnels.getTunnelConfigsFlow().distinctUntilChanged { old, new ->
				old.map { it.isActive } != new.map { it.isActive }
			},

to:

appDataRepository.get().tunnels.getTunnelConfigsFlow().distinctUntilChanged(),

Edit: Another solution would be to change those lines:

* https://github.com/zaneschepke/wgtunnel/blob/main/app/src/main/java/com/zaneschepke/wireguardautotunnel/service/tunnel/WireGuardTunnel.kt#L126

* https://github.com/zaneschepke/wgtunnel/blob/main/app/src/main/java/com/zaneschepke/wireguardautotunnel/service/tunnel/WireGuardTunnel.kt#L247

To only update the isActive in the database rather than save everything.

Thank you very much for the detailed explanation!
So If a understand it correctly, the problem occurs when changing the tunnel configuration. Therefore you can bypass the error by deleting the tunnel configuration, create a new one and set the exclusion from the beginning? I'll give this a try tomorrow.

@bmonjoie
Copy link

bmonjoie commented Dec 11, 2024

If I am right, you can configure your tunnel correctly, then force close the application and launch it again to fix the issue. All of that while staying on the same network not to trigger the bug, of course.
That should force the application to load the proper configuration in the StateFlow and therefore fix the issue.

@Dentic89
Copy link

If I am right, you can configure your tunnel correctly, then force close the application and launch it again to fix the issue. All of that while staying on the same network not to trigger the bug, of course. That should force the application to load the proper configuration in the StateFlow and therefore fix the issue.

You are right, I can confirm that as workaround! Thank you very much! It's important to force close the app in app settings and not just end it in the recent apps screen.

@zaneschepke
Copy link
Owner

Thank you @bmonjoie for the detailed explanation and solution! This is a bug then as it should actually be doing the opposite. This flow should be ignoring changes to isActive state and collecting states for all other tunnelConfig changes. The reason we want to ignore the isActive state is to allow users the ability to manually toggle tunnels off even while auto-tunneling (until the next network event change). Otherwise, the tunnel will just immediately toggle back on.

@bmonjoie
Copy link

@zaneschepke I was wrong about the root cause of this bug. I didn't understand the distrinctUntilChanged condition correctly, and I realize now that I was not looking at the correct version of the code. The code that was in production for 3.6.1 (where the bug happens) is this one: https://github.com/zaneschepke/wgtunnel/blob/cab29459300bb9087b4cb3cf90ab9c57f15d99e6/app/src/main/java/com/zaneschepke/wireguardautotunnel/service/foreground/AutoTunnelService.kt

I believe that the issue is the one I described: the content of the tunnel configs was not updated correctly in the flow, and when the AutoTunnel was turned on, it would override the content in the database. But this is for a different reason than I thought. I believe the reason this occurred is because of the use of emit here:

emit is suspending, and when a flow is collected or combined without collectLatest or combineLatest, it could be that something is holding the collect or combine, and the emit is never executed. At least, that's the only explanation I see as to why the list was not updated.

I believe this bug is fixed in 3.6.3. Sorry for misleading you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

4 participants