latest changes and bug fixes post v055z - #2961
Conversation
fix: celzero#2956, celzero#2946 avoid accessing DNS proxy details from the main context fix: celzero#2955 show IP protocol mismatch in a separate notification channel fix: celzero#2948 ensure the notification channel exists before posting notifications fix: celzero#2944 fix port rule checks in the upstream answer flow refactor: split BraveVPNService into TunDnsManager, TunFirewallManager, and TunFlowManager
fix: celzero#2947 remove crash logging from getResolver() related: celzero#2945, celzero#2940 ensure Plus resolvers are added in parallel fix: celzero#2942 prevent disabling the local blocklist setting when local blocklist files are available refactor: add APIs to support GlobalProxyHandler
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe pull request adds firewall and VPN flow managers, proxy recovery, lockdown enforcement, WireGuard feedback, SAF memory-profile export, billing error classification, DNS host pinning, and related UI, database, test, and platform updates. ChangesApplication updates
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 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 |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 17
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/src/full/java/com/celzero/bravedns/service/ProxyManager.kt (1)
130-152: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winProxy rows are created for tombstoned apps in both bulk-assignment functions. Both loops read from
FirewallManager.getAllApps(), which returns tombstoned entries as well (seeFirewallManager.ktline 410). Tombstoned entries keep a negated uid inappInfos, sogetAppInfoByUidAndPackageresolves them andensureBaseRowmakes uninstalled apps visible in the "All Apps" pager.
app/src/full/java/com/celzero/bravedns/service/ProxyManager.kt#L130-L152: insetProxyIdForAllApps, skip the app whenai.tombstoneTs > 0L, before callingensureBaseRow.app/src/full/java/com/celzero/bravedns/service/ProxyManager.kt#L164-L178: insetProxyIdForUnselectedApps, apply the sameai.tombstoneTs > 0Lguard beforeensureBaseRowand thepamSet.addcall.🤖 Prompt for 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. In `@app/src/full/java/com/celzero/bravedns/service/ProxyManager.kt` around lines 130 - 152, The bulk-assignment loops must skip tombstoned apps before creating proxy rows. In app/src/full/java/com/celzero/bravedns/service/ProxyManager.kt lines 130-152, update setProxyIdForAllApps to guard on ai.tombstoneTs > 0L before ensureBaseRow; apply the same guard in setProxyIdForUnselectedApps at lines 164-178 before ensureBaseRow and pamSet.add.
🟡 Minor comments (7)
app/src/main/res/values/strings.xml-2099-2108 (1)
2099-2108: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRemove the literal backslashes from the lockdown message.
\"renders as\"in the UI. Use"without the backslash.Proposed fix
- <string name="lockdown_check_ac_desc" translatable="false">Only the \"Hybrid\" dial strategy with \"never retry\" is compatible; disable anti-censorship otherwise.</string> + <string name="lockdown_check_ac_desc" translatable="false">Only the "Hybrid" dial strategy with "never retry" is compatible; disable anti-censorship otherwise.</string>🤖 Prompt for 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. In `@app/src/main/res/values/strings.xml` around lines 2099 - 2108, Update the lockdown_check_ac_desc string to use " directly for the quoted terms, removing the literal backslashes while preserving the displayed message.app/src/full/java/com/celzero/bravedns/adapter/ConnectionLogAdapter.kt-315-315 (1)
315-315: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse the effective blocked state for the RTT marker.
The callers explicitly override the raw blocked value for
RULE12without a proxy. The duration uses the effectiveblockedparameter, but the laterisRoundTripShortercall still receiveslog.isBlockedorct.isBlocked. This can show the rocket marker on an entry that the same method treats as blocked.
app/src/full/java/com/celzero/bravedns/adapter/ConnectionLogAdapter.kt#L315-L315: passblockedtoisRoundTripShorter.app/src/full/java/com/celzero/bravedns/adapter/ConnectionTrackerAdapter.kt#L271-L271: passblockedtoisRoundTripShorter.🤖 Prompt for 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. In `@app/src/full/java/com/celzero/bravedns/adapter/ConnectionLogAdapter.kt` at line 315, The RTT marker must use the effective blocked state rather than each log’s raw state. In ConnectionLogAdapter.kt lines 315-315, update displaySummaryDetails to pass blocked to isRoundTripShorter; make the same change in ConnectionTrackerAdapter.kt lines 271-271, passing blocked there as well.app/src/full/java/com/celzero/bravedns/adapter/ConnectionLogAdapter.kt-338-344 (1)
338-344: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMake minimal blocked summaries visible.
The new branches set the red duration marker but do not reliably show
connectionSummaryLl. Inapp/src/full/java/com/celzero/bravedns/adapter/ConnectionTrackerAdapter.kt,hasMinSummaryremains false, so Line [324]-[326] hides the container. Inapp/src/full/java/com/celzero/bravedns/adapter/ConnectionLogAdapter.kt, a recycled holder can retainView.GONE.
app/src/full/java/com/celzero/bravedns/adapter/ConnectionLogAdapter.kt#L338-L344: setconnectionSummaryLltoView.VISIBLEwhenblockedis true.app/src/full/java/com/celzero/bravedns/adapter/ConnectionTrackerAdapter.kt#L292-L297: sethasMinSummary = trueand makeconnectionSummaryLlvisible.Add a regression test for a blocked minimal row after a row with no summary.
🤖 Prompt for 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. In `@app/src/full/java/com/celzero/bravedns/adapter/ConnectionLogAdapter.kt` around lines 338 - 344, Make blocked minimal summaries visible in both affected adapters: in app/src/full/java/com/celzero/bravedns/adapter/ConnectionLogAdapter.kt lines 338-344, set connectionSummaryLl to View.VISIBLE when blocked is true to override recycled-holder state; in app/src/full/java/com/celzero/bravedns/adapter/ConnectionTrackerAdapter.kt lines 292-297, set hasMinSummary = true and make connectionSummaryLl visible. Add a regression test covering a blocked minimal row rendered after a row with no summary.app/src/full/res/layout/fragment_home_screen.xml-97-104 (1)
97-104: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReplace auto-sizing on these one-line titles with a fixed overflow-safe layout.
Android auto-sizing needs fixed boundaries and is warned against with
wrap_content, especially whenmaxLines="1"is set. These titles can overflow or produce broken sizing behavior on narrow / long-locale cards. Use a fixed height or constraint-based card height, then set eitherandroid:ellipsize="end"or a second-line overflow fallback consistently.Apply the same layout contract to every home-card title in both
fragment_home_screen.xmlandlayout-sw600dp/fragment_home_screen.xml.🤖 Prompt for 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. In `@app/src/full/res/layout/fragment_home_screen.xml` around lines 97 - 104, Replace auto-sizing on every home-card title in app/src/full/res/layout/fragment_home_screen.xml at lines 97-104, 200-207, 314-321, 387-394, and 488-495, and app/src/full/res/layout-sw600dp/fragment_home_screen.xml at lines 107-114, 209-216, 321-328, 393-400, and 495-502. Use a fixed or constraint-based title height with consistent end ellipsizing or a second-line overflow fallback, while preserving each title’s existing text and styling.app/src/main/java/com/celzero/bravedns/service/DomainRulesManager.kt-237-245 (1)
237-245: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse
Locale.ROOTfor domain case folding.Every other lowercase call in this object uses
Locale.ROOT(see lines 122, 128, 166, 178, 324).Locale.getDefault()applies locale-sensitive case mapping. In Turkish or Azeri locales,Imaps toı, so a domain such asINDEX.example.combecomesındex.example.com. The matched domain is then written back toconnInfo.querybyTunFirewallManager, which propagates the altered string to the connection log.🐛 Proposed fix
- val domains = if (isAtleastR()) { + val domains = if (isAtleastR()) { // on Android R and above, go will give the first domain as the accurate domain so // no need to check further domains - val d = domain.lowercase(Locale.getDefault()).split(",").firstOrNull() + val d = domain.lowercase(Locale.ROOT).split(",").firstOrNull() if (d.isNullOrEmpty()) return Pair(Status.NONE, "") listOf(d) } else { - domain.lowercase(Locale.getDefault()).split(",") + domain.lowercase(Locale.ROOT).split(",") }🤖 Prompt for 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. In `@app/src/main/java/com/celzero/bravedns/service/DomainRulesManager.kt` around lines 237 - 245, Update the lowercase conversion in the Android R-and-above branch of the domain extraction logic to use Locale.ROOT instead of Locale.getDefault(). Keep the existing split, first-domain selection, and empty-domain handling unchanged, matching the other lowercase calls in DomainRulesManager.app/src/main/java/com/celzero/bravedns/net/go/GoVpnAdapter.kt-914-916 (1)
914-916: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
GlobalProxyHandler.trackruns without confirming that the tunnel add succeeded. Every add site records the id unconditionally, either before the tunnel call or while ignoring anullresult. The handler then monitors proxies that were never added, spends the attempt budget, and finally emits a HIGH-severityproxy re-add failedevent for a proxy that was never present.
app/src/main/java/com/celzero/bravedns/net/go/GoVpnAdapter.kt#L914-L916: callGlobalProxyHandler.track(id)only whenres != null.app/src/main/java/com/celzero/bravedns/net/go/GoVpnAdapter.kt#L1318-L1321: moveGlobalProxyHandler.track(id)belowaddProxyand guard it onp != null.app/src/main/java/com/celzero/bravedns/net/go/GoVpnAdapter.kt#L2859-L2861: moveGlobalProxyHandler.track(Backend.RpnWin)below therpn.registerWin(...)call.app/src/main/java/com/celzero/bravedns/net/go/GoVpnAdapter.kt#L3216-L3218: moveGlobalProxyHandler.track(Backend.RpnWin + key)below thewin.fork(key)call.🤖 Prompt for 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. In `@app/src/main/java/com/celzero/bravedns/net/go/GoVpnAdapter.kt` around lines 914 - 916, Only track proxies after their tunnel add succeeds: in GoVpnAdapter.kt lines 914-916, guard GlobalProxyHandler.track(id) on a non-null getProxies()?.addProxy result; in lines 1318-1321, move tracking after addProxy and guard it on p != null; in lines 2859-2861, move tracking below rpn.registerWin(...); and in lines 3216-3218, move tracking below win.fork(key). These four sites in the same file must no longer record unsuccessful additions.app/src/main/java/com/celzero/bravedns/service/GlobalProxyHandler.kt-114-146 (1)
114-146: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
startis not atomic, and theuntrackRpnpredicate has a redundant clause.
- Lines 126-127 perform a check-then-set on
job. Two concurrentstartcalls can both observejob?.isActive != trueand launch two checker coroutines. The KDoc claims idempotence. Guard the body withsynchronized(this).- Line 116:
it == Backend.RpnWinis already covered byit.startsWith(Backend.RpnWin). Drop the first clause.♻️ Proposed change
fun untrackRpn() { - proxies.keys.removeAll { - it == Backend.RpnWin || it.startsWith(Backend.RpnWin) - } + proxies.keys.removeAll { it.startsWith(Backend.RpnWin) } Logger.v(LOG_TAG_PROXY, "$TAG untracked rpn proxies, total: ${proxies.size}") } - fun start(scope: CoroutineScope) { + `@Synchronized` + fun start(scope: CoroutineScope) { if (job?.isActive == true) return🤖 Prompt for 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. In `@app/src/main/java/com/celzero/bravedns/service/GlobalProxyHandler.kt` around lines 114 - 146, Update untrackRpn to remove the redundant equality check and retain only the startsWith predicate. Make start atomic by synchronizing the check-and-launch section on this, preserving the existing idempotent behavior and checker setup.
🧹 Nitpick comments (11)
app/src/full/java/com/celzero/bravedns/ui/fragment/AboutFragment.kt (3)
786-794: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
DocumentsContract.Document.COLUMN_DISPLAY_NAMEfor a document URI.
childUriis a document URI. The documented column for its name isDocumentsContract.Document.COLUMN_DISPLAY_NAME. Both constants resolve to"_display_name", so behavior does not change, but the sibling methodisTreeUriAccessiblealready queriesDocumentsContract.Documentcolumns. Using the same constant family removes the inconsistency and drops theOpenableColumnsimport.♻️ Proposed refactor
var name = displayName contentResolver.query( - childUri, arrayOf(OpenableColumns.DISPLAY_NAME), null, null, null + childUri, arrayOf(DocumentsContract.Document.COLUMN_DISPLAY_NAME), null, null, null )?.use { c -> if (c.moveToFirst()) { - val idx = c.getColumnIndex(OpenableColumns.DISPLAY_NAME) + val idx = c.getColumnIndex(DocumentsContract.Document.COLUMN_DISPLAY_NAME) if (idx >= 0) name = c.getString(idx) ?: displayName } }🤖 Prompt for 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. In `@app/src/full/java/com/celzero/bravedns/ui/fragment/AboutFragment.kt` around lines 786 - 794, Update the query projection in the childUri handling block to use DocumentsContract.Document.COLUMN_DISPLAY_NAME instead of OpenableColumns.DISPLAY_NAME, matching the document-URI column family used by isTreeUriAccessible. Remove the now-unused OpenableColumns import and keep the existing name fallback behavior unchanged.
189-206: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueReuse
ctxinstead ofrequireContext()in the result callback.The callback captures
ctx = contextand null-checks it for the toast path. It then callsrequireContext()for the permission call. If the fragment is detached when the result arrives,requireContext()throwsIllegalStateException. Use the already-resolved context for both paths.♻️ Proposed refactor
- try { - requireContext().contentResolver.takePersistableUriPermission( + val resolverCtx = ctx ?: requireActivity().applicationContext + try { + resolverCtx.contentResolver.takePersistableUriPermission( uri, Intent.FLAG_GRANT_READ_URI_PERMISSION or Intent.FLAG_GRANT_WRITE_URI_PERMISSION ) } catch (e: SecurityException) { Logger.w(LOG_TAG_UI, "Could not persist URI permission for $uri", e) }🤖 Prompt for 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. In `@app/src/full/java/com/celzero/bravedns/ui/fragment/AboutFragment.kt` around lines 189 - 206, Update the result callback around ctx and takePersistableUriPermission to reuse the already-resolved ctx instead of calling requireContext(). Preserve the existing null handling and ensure the permission request uses the captured context without throwing if the fragment is detached.
618-629: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low value
isTreeUriAccessibleruns a SAF query on the main thread.
openMemoryProfileis called from a click listener.isTreeUriAccessibleperforms aContentResolver.queryagainst an external DocumentsProvider, which can block. The path is DEBUG-only, so the impact is limited. If you want to avoid a possible frame drop, move the validation intoprofileScopeand fall back to the picker on the main thread.🤖 Prompt for 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. In `@app/src/full/java/com/celzero/bravedns/ui/fragment/AboutFragment.kt` around lines 618 - 629, Update openMemoryProfile’s stored-URI validation so isTreeUriAccessible runs inside profileScope rather than on the click listener’s main-thread path; marshal the result back to the main thread, reusing the accessible URI for performMemoryProfileCapture and clearing the persisted URI before launching the picker when validation fails.app/src/main/java/com/celzero/bravedns/service/TunFlowManager.kt (2)
1403-1410: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the empty
ifblock.Lines 1406-1408 contain a condition with a comment-only body. The following line already handles the null case with a safe call.
♻️ Proposed fix
private fun deviceLocked(ctx: FlowContext): Boolean { if (!persistentState.getBlockWhenDeviceLocked()) return false - - if (ctx.keyguardManager == null) { - // keyguardManager is provided by BVS via FlowContext - } return (ctx.keyguardManager?.isKeyguardLocked == true) }🤖 Prompt for 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. In `@app/src/main/java/com/celzero/bravedns/service/TunFlowManager.kt` around lines 1403 - 1410, Remove the empty null-check block from deviceLocked; retain the existing persistent-state guard and the safe-call return using ctx.keyguardManager?.isKeyguardLocked.
640-653: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the no-op early-return statement.
Line 643 evaluates
emptyList<String>()and discards it. It does not exit the lambda. The behaviour is still correct because the snapshot of an empty set is empty, but the line implies a guard that does not exist.♻️ Proposed fix
val cidsToClose: List<String> = activeClosableCidsMutex.withLock { - if (activeClosableCids.isEmpty()) emptyList<String>() - val snapshot = activeClosableCids.toList() activeClosableCids.clear() snapshot }🤖 Prompt for 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. In `@app/src/main/java/com/celzero/bravedns/service/TunFlowManager.kt` around lines 640 - 653, Remove the discarded emptyList<String>() expression from the activeClosableCidsMutex.withLock block in closeTrackedConnsOnDeviceLock; retain the snapshot, clear, and return flow so empty sets still produce an empty cidsToClose list.app/src/full/java/com/celzero/bravedns/service/ProxyManager.kt (1)
410-419: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider independent checks instead of
when.
whenselects at most one branch. If more than one of Orbot, custom SOCKS5, and custom HTTP can be enabled at the same time, the statistics omit the others. If the app configuration guarantees mutual exclusion, the current code is correct and no change is needed.♻️ Proposed refactor
- when { - appConfig.isOrbotProxyEnabled() -> add(ID_ORBOT_BASE) - appConfig.isCustomSocks5Enabled() -> add(ID_S5_BASE) - appConfig.isCustomHttpProxyEnabled() -> add(ID_HTTP_BASE) - } + if (appConfig.isOrbotProxyEnabled()) add(ID_ORBOT_BASE) + if (appConfig.isCustomSocks5Enabled()) add(ID_S5_BASE) + if (appConfig.isCustomHttpProxyEnabled()) add(ID_HTTP_BASE)🤖 Prompt for 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. In `@app/src/full/java/com/celzero/bravedns/service/ProxyManager.kt` around lines 410 - 419, Verify whether isOrbotProxyEnabled, isCustomSocks5Enabled, and isCustomHttpProxyEnabled are mutually exclusive. If multiple settings may be enabled simultaneously, replace the when in the proxies buildList with independent checks so every enabled proxy identifier is added; otherwise leave the current implementation unchanged.app/src/main/java/com/celzero/bravedns/service/TunFirewallManager.kt (1)
174-204: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the redundant re-split of
ips.Line 177 already reduces
destIPto a single element whenisAlgis false, then line 178 splits that single element again. The same pattern repeats at line 262. The result is correct, but the intent is unclear to a reader.♻️ Proposed refactor
- val ips = if (params.isAlg) params.connInfo.destIP else params.connInfo.destIP.split(",").first() - ips.split(",").forEach { ip -> + val ips = + if (params.isAlg) params.connInfo.destIP.split(",") + else listOf(params.connInfo.destIP.split(",").first()) + ips.forEach { ip ->Apply the same change at line 262.
🤖 Prompt for 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. In `@app/src/main/java/com/celzero/bravedns/service/TunFirewallManager.kt` around lines 174 - 204, Remove the redundant split in the IP-rules loop by iterating directly over the `ips` value computed from `params.connInfo.destIP`; preserve splitting only in the non-ALG branch. Apply the same adjustment to the corresponding IP-processing loop near the second occurrence around the firewall rule handling.app/src/main/java/com/celzero/bravedns/tunnel/TunDnsManager.kt (3)
78-84: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMark the test seam with
@VisibleForTesting.
setDnsOptsFactoryForTestis public production API. Annotate it so lint flags accidental production use.♻️ Proposed change
+ `@androidx.annotation.VisibleForTesting` fun setDnsOptsFactoryForTest(factory: () -> DNSOpts) { dnsOptsFactory = factory }🤖 Prompt for 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. In `@app/src/main/java/com/celzero/bravedns/tunnel/TunDnsManager.kt` around lines 78 - 84, Annotate setDnsOptsFactoryForTest with `@VisibleForTesting` so its test-only purpose is explicit and lint can flag production use; leave dnsOptsFactory and the setter behavior unchanged.
819-830: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCorrect the misleading log text.
The log says "bypass tid", but the branch blocks the query.
tidcsvisBlockAllandtidseccsvis empty at that point. Rename the fields in the message.♻️ Proposed change
Logger.vv( LOG_TAG_VPN, - "onUpstreamAnswer: block dns for unknown app, original tid: $id, bypass tid: [$tidcsv, $tidseccsv], ipcsv: ${params.ipcsv}" + "onUpstreamAnswer: block dns for unknown app, original tid: $id, blocked tid: [$tidcsv, $tidseccsv], ipcsv: ${params.ipcsv}" )🤖 Prompt for 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. In `@app/src/main/java/com/celzero/bravedns/tunnel/TunDnsManager.kt` around lines 819 - 830, Update the Logger.vv message in the uid == INVALID_UID blocking branch to replace the misleading “bypass tid” label with labels indicating the blocked tid values, specifically tidcsv and tidseccsv. Keep the logged values and surrounding behavior unchanged.
323-343: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConfirm the intent of the flipped
skipGlobalRulesbranch.
skipGlobalRulesis a compile-timetrue, so the whole block at Lines 325-343 is now unreachable. Before this change the same block ran. This removes global (UID_EVERYBODY) TRUST/BLOCK domain evaluation fromonQueryfor the uid-present path and defers it toonUpstreamAnswer. The comment states this is intentional.If it is intentional, delete the dead block and keep only the comment. Dead code with an inverted flag is easy to re-enable by accident.
🤖 Prompt for 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. In `@app/src/main/java/com/celzero/bravedns/tunnel/TunDnsManager.kt` around lines 323 - 343, Remove the unreachable skipGlobalRules conditional and its globalDomainRule evaluation from the onQuery flow, leaving only the existing comment documenting that global rules are handled in onUpstreamAnswer. Preserve the remaining query handling unchanged.app/src/test/java/com/celzero/bravedns/tunnel/TunDnsManagerTest.kt (1)
337-365: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRename this test to match what it asserts.
The name says "unknown app blocked returns BlockAll", but the inline comment states the
blockDnsForUnknownAppdelegate cannot be stubbed, so the block branch is never taken. The assertion only verifies thatTunFirewallManager.firewallruns. Rename to something likeINVALID_UID falls through to firewall evaluationso the intent stays clear.🤖 Prompt for 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. In `@app/src/test/java/com/celzero/bravedns/tunnel/TunDnsManagerTest.kt` around lines 337 - 365, Rename the test function `handleOnUpstreamAnswer - unknown app blocked returns BlockAll` to reflect that INVALID_UID proceeds to firewall evaluation rather than returning BlockAll. Preserve the existing test setup and assertion verifying `TunFirewallManager.firewall` is invoked.
🤖 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/full/java/com/celzero/bravedns/service/WireguardManager.kt`:
- Around line 395-406: Update disableOtherOneWireGuardConfigs to use a
synchronous suspend disable path, such as disableConfigAndAwait, for every
replaced configuration before returning. Ensure that path performs the mapping
update, db.disableConfig, and VpnController.removeWireGuardProxy in order and
awaits live tunnel removal, then retain the One-WireGuard state update without
allowing the selected configuration to enable first.
In
`@app/src/full/java/com/celzero/bravedns/ui/activity/TunnelSettingsActivity.kt`:
- Line 1360: Update the lockdown handling near settingsActivityLanTrafficRl so
settingsActivityLanTrafficSwitch is also disabled when isSystemLockdown is true.
Ensure its listener cannot update persistentState.privateIps during lockdown,
while preserving normal LAN traffic behavior when lockdown is inactive.
- Around line 1225-1230: The lockdown compatibility check around isHybridDial
and hasConflict incorrectly conflates TCP_PROXY.mode with SPLIT_AUTO.mode.
Distinguish the strategies using the appropriate persistent-state value or enum
identity, require persistentState.autoProxyEnabled for the TCP Proxy hybrid
case, and preserve the incompatibility outcome for both Split Auto + Never Retry
and TCP Proxy + Never Retry when the required proxy setting is absent. Add
coverage for both combinations.
In `@app/src/full/java/com/celzero/bravedns/ui/fragment/AboutFragment.kt`:
- Around line 659-688: Update the completion block to pass appCtx, not ctx, to
showMemoryProfileNotification. Ensure the notification is posted even when
profileScope is canceled during capture by wrapping the notification’s
Main-thread completion in a NonCancellable context, adding the required
kotlinx.coroutines.NonCancellable import; keep the fragment-attached check
limited to the toast.
In `@app/src/main/java/com/celzero/bravedns/database/RefreshDatabase.kt`:
- Around line 547-556: Make the candidate filtering in findPackagesToAdd compare
entries by the full (uid, packageName) pair rather than packageName alone, so
sibling UIDs remain eligible while existing mappings are excluded; verify this
preserves the expected behavior for all other callers, then retain the uid-aware
snapshot lookup in the add flow.
In `@app/src/main/java/com/celzero/bravedns/net/go/GoVpnAdapter.kt`:
- Around line 3572-3603: The addMultipleDnsAsPlus flow should await all
transport-add jobs instead of launching unbounded fire-and-forget coroutines:
use a structured coroutine scope with async jobs and a bounded semaphore,
preserving per-endpoint success/failure handling. Update the failure logEvent
message from “Smart DNS transports added” to a failure-specific message such as
“Smart DNS transport add failed”, while keeping the existing error details.
- Around line 2962-2967: Update the trackedId calculation near
GlobalProxyHandler.track so any iid beginning with Backend.RpnWin is used
unchanged, while unprefixed ids receive the prefix. Replace the exact-equality
check with startsWith and preserve the existing special handling for the bare
Backend.RpnWin value.
- Around line 181-183: Update the addMultipleDnsAsPlus() call in
initResolverProxiesPcap to run only when appConfig.isSmartDnsEnabled() is true,
while preserving the existing null and CLIENT_ERROR status checks. This prevents
repeated additions during updateTun for non-Smart DNS configurations.
- Around line 2418-2435: Update replaceHostWithIp so the DoH URL authority
remains the original hostname even when ips is provided; do not substitute
firstIp into the URL host or bracket IPv6 values. Preserve the original scheme,
port, path, query, and fragment while leaving the ips argument available only
for bootstrap/fallback handling by Intra.addDoHTransport.
In
`@app/src/main/java/com/celzero/bravedns/rpnproxy/SubscriptionStateMachineV2.kt`:
- Line 766: Resolve the JVM getter clash between the open val currentState
property and the getCurrentState() method in SubscriptionStateMachineV2 by
assigning the property getter a distinct JVM name or renaming one declaration.
Preserve the existing Kotlin API and state access behavior while ensuring both
declarations compile with unique JVM signatures.
In `@app/src/main/java/com/celzero/bravedns/service/GlobalProxyHandler.kt`:
- Around line 235-255: Update readdProxy to handle ID_S5_BASE, ID_HTTP_BASE, and
ID_ORBOT_BASE by invoking VpnController.readdSocks5Proxy(), readdHttpProxy(),
and readdCustomProxy() respectively, before the unknown-id fallback. Remove the
now-used imports if applicable, while preserving existing handling for RPN and
WireGuard proxies.
In `@app/src/main/java/com/celzero/bravedns/service/TunFirewallManager.kt`:
- Around line 472-493: Update testWithBackoff to suspend for each computed
backoff interval before retrying test(), using the existing remainingWaitMs and
exponentialBackoff flow. Ensure the new-app check genuinely waits up to
durationSec for asynchronous firewall tracking, while preserving the immediate
success return when test() succeeds.
In `@app/src/main/java/com/celzero/bravedns/service/TunFlowManager.kt`:
- Around line 1163-1178: Update the blocked-result check in the flow handling
around WireguardManager.getAllPossibleConfigIdsForApp: replace the invalid
List.equals(String) comparison with the intended block detection, confirming
whether it should test wgs.contains(Backend.Block) or wgs.first() ==
Backend.Block. Ensure blocked WireGuard results set connTracker.isBlocked and
RULE17, rather than being returned as a normal proxy list.
- Around line 587-592: Update the proxy classification flow around
isNotLocalAndRpnProxy so comma-separated mark.pidcsv values are evaluated by
their individual proxy IDs rather than as one CSV string. Apply the same
correction at the containsMultipleProxy path near cm.isBlocked, ensuring local
pseudo-proxy combinations do not set FirewallRuleset.RULE12 while any genuinely
non-local proxy still is handled correctly.
In `@app/src/main/java/com/celzero/bravedns/tunnel/TunDnsManager.kt`:
- Around line 858-884: Update the FirewallParameters construction in the
upstream-answer flow so multi-address DNS responses are evaluated correctly by
IP rules: either set isAlg to true to split params.ipcsv and check every
address, or pass the existing firstDestIp as connInfo.destIP. Preserve
single-address behavior and ensure IP TRUST/BLOCK rules are applied to
multi-answer responses.
In `@app/src/play/java/com/celzero/bravedns/iab/InAppBillingHandler.kt`:
- Around line 1968-1974: Ensure product query results are treated as successful
only when both INAPP and subscription response codes are OK: in
app/src/play/java/com/celzero/bravedns/iab/InAppBillingHandler.kt#L1968-L1974,
retain the prior complete cache and call productResult(false, emptyList()) on
any failure, otherwise replace the cache and notify billingListener with the
merged catalog; apply the same all-queries-successful behavior at
app/src/website/java/com/celzero/bravedns/iab/InAppBillingHandler.kt#L1968-L1974.
In
`@app/src/test/java/com/celzero/bravedns/service/IpRulesManagerWildcardTest.kt`:
- Around line 105-196: Replace the simulated tests in IpRulesManagerWildcardTest
with a test that executes production behavior and fails when padAndNormalize
reverts to .orEmpty(). Prefer exposing padAndNormalize for testing with internal
and `@VisibleForTesting`, then assert it preserves "*.255.255.255"; alternatively
verify through addIpRule that the persisted ipAddress is non-empty. Retain the
assignPrefixForSingleBlock contract tests, but remove tests that only validate
local Elvis, formatting, or conditional expressions.
---
Outside diff comments:
In `@app/src/full/java/com/celzero/bravedns/service/ProxyManager.kt`:
- Around line 130-152: The bulk-assignment loops must skip tombstoned apps
before creating proxy rows. In
app/src/full/java/com/celzero/bravedns/service/ProxyManager.kt lines 130-152,
update setProxyIdForAllApps to guard on ai.tombstoneTs > 0L before
ensureBaseRow; apply the same guard in setProxyIdForUnselectedApps at lines
164-178 before ensureBaseRow and pamSet.add.
---
Minor comments:
In `@app/src/full/java/com/celzero/bravedns/adapter/ConnectionLogAdapter.kt`:
- Line 315: The RTT marker must use the effective blocked state rather than each
log’s raw state. In ConnectionLogAdapter.kt lines 315-315, update
displaySummaryDetails to pass blocked to isRoundTripShorter; make the same
change in ConnectionTrackerAdapter.kt lines 271-271, passing blocked there as
well.
- Around line 338-344: Make blocked minimal summaries visible in both affected
adapters: in
app/src/full/java/com/celzero/bravedns/adapter/ConnectionLogAdapter.kt lines
338-344, set connectionSummaryLl to View.VISIBLE when blocked is true to
override recycled-holder state; in
app/src/full/java/com/celzero/bravedns/adapter/ConnectionTrackerAdapter.kt lines
292-297, set hasMinSummary = true and make connectionSummaryLl visible. Add a
regression test covering a blocked minimal row rendered after a row with no
summary.
In `@app/src/full/res/layout/fragment_home_screen.xml`:
- Around line 97-104: Replace auto-sizing on every home-card title in
app/src/full/res/layout/fragment_home_screen.xml at lines 97-104, 200-207,
314-321, 387-394, and 488-495, and
app/src/full/res/layout-sw600dp/fragment_home_screen.xml at lines 107-114,
209-216, 321-328, 393-400, and 495-502. Use a fixed or constraint-based title
height with consistent end ellipsizing or a second-line overflow fallback, while
preserving each title’s existing text and styling.
In `@app/src/main/java/com/celzero/bravedns/net/go/GoVpnAdapter.kt`:
- Around line 914-916: Only track proxies after their tunnel add succeeds: in
GoVpnAdapter.kt lines 914-916, guard GlobalProxyHandler.track(id) on a non-null
getProxies()?.addProxy result; in lines 1318-1321, move tracking after addProxy
and guard it on p != null; in lines 2859-2861, move tracking below
rpn.registerWin(...); and in lines 3216-3218, move tracking below win.fork(key).
These four sites in the same file must no longer record unsuccessful additions.
In `@app/src/main/java/com/celzero/bravedns/service/DomainRulesManager.kt`:
- Around line 237-245: Update the lowercase conversion in the Android
R-and-above branch of the domain extraction logic to use Locale.ROOT instead of
Locale.getDefault(). Keep the existing split, first-domain selection, and
empty-domain handling unchanged, matching the other lowercase calls in
DomainRulesManager.
In `@app/src/main/java/com/celzero/bravedns/service/GlobalProxyHandler.kt`:
- Around line 114-146: Update untrackRpn to remove the redundant equality check
and retain only the startsWith predicate. Make start atomic by synchronizing the
check-and-launch section on this, preserving the existing idempotent behavior
and checker setup.
In `@app/src/main/res/values/strings.xml`:
- Around line 2099-2108: Update the lockdown_check_ac_desc string to use "
directly for the quoted terms, removing the literal backslashes while preserving
the displayed message.
---
Nitpick comments:
In `@app/src/full/java/com/celzero/bravedns/service/ProxyManager.kt`:
- Around line 410-419: Verify whether isOrbotProxyEnabled,
isCustomSocks5Enabled, and isCustomHttpProxyEnabled are mutually exclusive. If
multiple settings may be enabled simultaneously, replace the when in the proxies
buildList with independent checks so every enabled proxy identifier is added;
otherwise leave the current implementation unchanged.
In `@app/src/full/java/com/celzero/bravedns/ui/fragment/AboutFragment.kt`:
- Around line 786-794: Update the query projection in the childUri handling
block to use DocumentsContract.Document.COLUMN_DISPLAY_NAME instead of
OpenableColumns.DISPLAY_NAME, matching the document-URI column family used by
isTreeUriAccessible. Remove the now-unused OpenableColumns import and keep the
existing name fallback behavior unchanged.
- Around line 189-206: Update the result callback around ctx and
takePersistableUriPermission to reuse the already-resolved ctx instead of
calling requireContext(). Preserve the existing null handling and ensure the
permission request uses the captured context without throwing if the fragment is
detached.
- Around line 618-629: Update openMemoryProfile’s stored-URI validation so
isTreeUriAccessible runs inside profileScope rather than on the click listener’s
main-thread path; marshal the result back to the main thread, reusing the
accessible URI for performMemoryProfileCapture and clearing the persisted URI
before launching the picker when validation fails.
In `@app/src/main/java/com/celzero/bravedns/service/TunFirewallManager.kt`:
- Around line 174-204: Remove the redundant split in the IP-rules loop by
iterating directly over the `ips` value computed from `params.connInfo.destIP`;
preserve splitting only in the non-ALG branch. Apply the same adjustment to the
corresponding IP-processing loop near the second occurrence around the firewall
rule handling.
In `@app/src/main/java/com/celzero/bravedns/service/TunFlowManager.kt`:
- Around line 1403-1410: Remove the empty null-check block from deviceLocked;
retain the existing persistent-state guard and the safe-call return using
ctx.keyguardManager?.isKeyguardLocked.
- Around line 640-653: Remove the discarded emptyList<String>() expression from
the activeClosableCidsMutex.withLock block in closeTrackedConnsOnDeviceLock;
retain the snapshot, clear, and return flow so empty sets still produce an empty
cidsToClose list.
In `@app/src/main/java/com/celzero/bravedns/tunnel/TunDnsManager.kt`:
- Around line 78-84: Annotate setDnsOptsFactoryForTest with `@VisibleForTesting`
so its test-only purpose is explicit and lint can flag production use; leave
dnsOptsFactory and the setter behavior unchanged.
- Around line 819-830: Update the Logger.vv message in the uid == INVALID_UID
blocking branch to replace the misleading “bypass tid” label with labels
indicating the blocked tid values, specifically tidcsv and tidseccsv. Keep the
logged values and surrounding behavior unchanged.
- Around line 323-343: Remove the unreachable skipGlobalRules conditional and
its globalDomainRule evaluation from the onQuery flow, leaving only the existing
comment documenting that global rules are handled in onUpstreamAnswer. Preserve
the remaining query handling unchanged.
In `@app/src/test/java/com/celzero/bravedns/tunnel/TunDnsManagerTest.kt`:
- Around line 337-365: Rename the test function `handleOnUpstreamAnswer -
unknown app blocked returns BlockAll` to reflect that INVALID_UID proceeds to
firewall evaluation rather than returning BlockAll. Preserve the existing test
setup and assertion verifying `TunFirewallManager.firewall` is invoked.
🪄 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: 98425e8a-5263-40da-8e50-608d1ab2bc7d
📒 Files selected for processing (45)
app/src/full/java/com/celzero/bravedns/adapter/ConnectionLogAdapter.ktapp/src/full/java/com/celzero/bravedns/adapter/ConnectionTrackerAdapter.ktapp/src/full/java/com/celzero/bravedns/adapter/OneWgConfigAdapter.ktapp/src/full/java/com/celzero/bravedns/adapter/WgConfigAdapter.ktapp/src/full/java/com/celzero/bravedns/service/ProxyManager.ktapp/src/full/java/com/celzero/bravedns/service/WireguardManager.ktapp/src/full/java/com/celzero/bravedns/ui/activity/AntiCensorshipActivity.ktapp/src/full/java/com/celzero/bravedns/ui/activity/ProxySettingsActivity.ktapp/src/full/java/com/celzero/bravedns/ui/activity/TunnelSettingsActivity.ktapp/src/full/java/com/celzero/bravedns/ui/activity/WgConfigDetailActivity.ktapp/src/full/java/com/celzero/bravedns/ui/fragment/AboutFragment.ktapp/src/full/java/com/celzero/bravedns/ui/fragment/DnsSettingsFragment.ktapp/src/full/res/layout-sw600dp/fragment_home_screen.xmlapp/src/full/res/layout/fragment_home_screen.xmlapp/src/main/java/com/celzero/bravedns/data/AppConfig.ktapp/src/main/java/com/celzero/bravedns/database/AppDatabase.ktapp/src/main/java/com/celzero/bravedns/database/RefreshDatabase.ktapp/src/main/java/com/celzero/bravedns/net/go/GoVpnAdapter.ktapp/src/main/java/com/celzero/bravedns/rpnproxy/SubscriptionStateMachineV2.ktapp/src/main/java/com/celzero/bravedns/scheduler/RpnProxyUpdateWorker.ktapp/src/main/java/com/celzero/bravedns/service/BraveVPNService.ktapp/src/main/java/com/celzero/bravedns/service/DomainRulesManager.ktapp/src/main/java/com/celzero/bravedns/service/FirewallManager.ktapp/src/main/java/com/celzero/bravedns/service/GlobalProxyHandler.ktapp/src/main/java/com/celzero/bravedns/service/IpRulesManager.ktapp/src/main/java/com/celzero/bravedns/service/PersistentState.ktapp/src/main/java/com/celzero/bravedns/service/TunFirewallManager.ktapp/src/main/java/com/celzero/bravedns/service/TunFlowManager.ktapp/src/main/java/com/celzero/bravedns/service/VpnController.ktapp/src/main/java/com/celzero/bravedns/tunnel/TunDnsManager.ktapp/src/main/java/com/celzero/bravedns/util/OrbotHelper.ktapp/src/main/res/layout/fragment_configure.xmlapp/src/main/res/layout/fragment_rethink_plus_premium.xmlapp/src/main/res/values/strings.xmlapp/src/play/java/com/celzero/bravedns/iab/InAppBillingHandler.ktapp/src/play/java/com/celzero/bravedns/ui/fragment/RethinkPlusFragment.ktapp/src/play/java/com/celzero/bravedns/viewmodel/RethinkPlusViewModel.ktapp/src/test/java/com/celzero/bravedns/service/IpRulesManagerWildcardTest.ktapp/src/test/java/com/celzero/bravedns/service/TunFirewallManagerTest.ktapp/src/test/java/com/celzero/bravedns/tunnel/TunDnsManagerTest.ktapp/src/website/java/com/celzero/bravedns/iab/InAppBillingHandler.ktapp/src/website/java/com/celzero/bravedns/sponsor/billing/SponsorBillingManagerImpl.ktapp/src/website/java/com/celzero/bravedns/ui/fragment/RethinkPlusFragment.ktapp/src/website/java/com/celzero/bravedns/viewmodel/RethinkPlusViewModel.ktgradle.properties
📜 Review details
⏰ Context from checks skipped due to timeout. (3)
- GitHub Check: Analyze (java-kotlin)
- GitHub Check: build
- GitHub Check: 🧭 OCR
⚠️ CI failures not shown inline (1)
GitHub Check: mobsfscan: 2 new alerts including 1 error
Conclusion: failure
### New alerts in code changed by this pull request
* 1 error
* 1 note
_Alerts not introduced by this pull request might have been detected because the code changes were too large._
See annotations below for details.
[View all branch alerts](/celzero/rethink-app/security/code-scanning?query=pr%3A2961+tool%3Amobsfscan+is%3Aopen).
🧰 Additional context used
🪛 GitHub Check: mobsfscan
app/src/full/java/com/celzero/bravedns/ui/activity/TunnelSettingsActivity.kt
[failure] 1337-1337:
Hidden elements in view can be used to hide data from user. But this data can be leaked.
|
🔍 OpenCodeReview found 79 issue(s) in this PR.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
app/src/main/res/values/strings.xml (1)
2099-2099: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRemove the literal backslashes from this message.
The UI will show backslashes before the quotation marks. Use
"Hybrid"and"never retry"without preceding backslashes.🤖 Prompt for 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. In `@app/src/main/res/values/strings.xml` at line 2099, Update the lockdown_check_ac_desc string to remove the backslashes preceding both quoted phrases, while retaining the "Hybrid" and "never retry" entities.app/src/full/java/com/celzero/bravedns/adapter/ConnectionLogAdapter.kt (1)
340-354: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReset alpha for active connection summaries.
If a recycled row was blocked,
connectionDuration.alpharemains0.7fon thehasCidpath. Set it to1fwhen rendering the green active indicator.🤖 Prompt for 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. In `@app/src/full/java/com/celzero/bravedns/adapter/ConnectionLogAdapter.kt` around lines 340 - 354, Reset connectionDuration.alpha to 1f in the hasCid branch of the connection summary rendering before showing the green active indicator, so recycled rows do not retain the blocked-state alpha. Keep the existing blocked and non-blocked handling unchanged.app/src/full/java/com/celzero/bravedns/ui/fragment/AboutFragment.kt (4)
659-666: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winSerialize memory-profile captures.
Every call to
performMemoryProfileCapture()starts a new child ofprofileScope. Repeated taps can run several JVM and Go heap dumps and SAF copies at the same time. This can exhaust memory and I/O resources and can post duplicate results. Track one active capture job or disableaboutMemoryProfileuntil completion.🤖 Prompt for 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. In `@app/src/full/java/com/celzero/bravedns/ui/fragment/AboutFragment.kt` around lines 659 - 666, Serialize calls to performMemoryProfileCapture by tracking the active profileScope capture job and returning or ignoring subsequent requests while it is running. Clear the tracked job when the capture completes or fails so a later capture can start, while preserving the existing capture and UI behavior.
788-795: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winDo not report export failure after the document was written.
writtenbecomestruebefore the display-name query. If the query throws,copyFileToTree()exits while the destination document remains, and the caller reports success = false. A retry can create a duplicate file. Make read-only display-name lookup best-effort after a successful write.🤖 Prompt for 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. In `@app/src/full/java/com/celzero/bravedns/ui/fragment/AboutFragment.kt` around lines 788 - 795, Update the post-write display-name lookup in copyFileToTree so failures from contentResolver.query or cursor processing cannot propagate after written is set true. Keep the document creation and write result successful, and use the existing displayName fallback when the best-effort lookup fails, preventing the caller from reporting export failure or retrying.Source: MCP tools
774-783: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winCheck the Boolean returned by
deleteDocument().
DocumentsContract.deleteDocument()returns success as a Boolean; false can also indicate deletion failure. Ifwrittenremains false because the provider returns false, the partial child stays in the selected folder. UpdatewritableTree()to return the result and log false as a cleanup failure.🤖 Prompt for 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. In `@app/src/full/java/com/celzero/bravedns/ui/fragment/AboutFragment.kt` around lines 774 - 783, Update writableTree() and its orphan cleanup around DocumentsContract.deleteDocument(): capture and return the Boolean result instead of ignoring it, and treat false as a cleanup failure by logging it alongside thrown exceptions. Ensure callers use the returned result so an unsuccessful deletion is not reported as successful.Source: MCP tools
619-624: 🚀 Performance & Scalability | 🟠 Major | ⚡ Quick winMove persisted-URI validation off the UI thread.
isTreeUriAccessible()queries the selected URI synchronously in the click handler. Document providers can be cloud-backed, so this SAF call can block the UI; run theContentResolver.query()validation onDispatchers.IO, then make the capture or picker decision onDispatchers.Main.🤖 Prompt for 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. In `@app/src/full/java/com/celzero/bravedns/ui/fragment/AboutFragment.kt` around lines 619 - 624, Update the memory-profile directory flow around isTreeUriAccessible() so its synchronous ContentResolver.query() validation runs on Dispatchers.IO rather than the click handler’s UI thread. After validation completes, switch to Dispatchers.Main before invoking performMemoryProfileCapture() or deciding to launch the directory picker, while preserving the existing stored-URI accessibility behavior.Source: MCP tools
🧹 Nitpick comments (1)
app/src/full/java/com/celzero/bravedns/ui/fragment/AboutFragment.kt (1)
193-195: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMove new memory-profile messages to string resources.
The picker errors, capture toast, result text, notification titles, and channel description are hardcoded in
AboutFragment.kt. Add resource-backed strings with placeholders for labels, filenames, sizes, and error messages. This keeps the new flow localizable and consistent with the rest of the UI.Also applies to: 647-650, 665-665, 733-737, 823-824, 840-854, 876-882
🤖 Prompt for 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. In `@app/src/full/java/com/celzero/bravedns/ui/fragment/AboutFragment.kt` around lines 193 - 195, Replace the hardcoded memory-profile UI text in AboutFragment, including picker errors, capture toast, result text, notification titles, and channel description, with string-resource lookups. Add the required localized resources and placeholders for labels, filenames, sizes, and error messages, then format them at each affected call site while preserving the existing messages and flow.
🤖 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/full/java/com/celzero/bravedns/adapter/ConnectionLogAdapter.kt`:
- Around line 238-245: Update the package lookup in the getOrPut block so
failures from FirewallManager.getPackageNamesByUid are not inserted into
packageNameCache. Cache and return the package list only when the lookup
succeeds; on failure, return an uncached empty list so subsequent binds retry
the lookup.
In `@app/src/main/java/com/celzero/bravedns/database/RefreshDatabase.kt`:
- Around line 632-641: Update the validRpnIds and purgeGhostMappings flow so a
failed RpnProxyManager.getCachedWinServerKeys() read does not produce an empty
valid-RPN set or purge RPN mappings. Track read success separately, and invoke
ProxyManager.purgeGhostMappings with RPN mappings eligible for purging only
after a successful key read while preserving existing WireGuard cleanup
behavior.
---
Outside diff comments:
In `@app/src/full/java/com/celzero/bravedns/adapter/ConnectionLogAdapter.kt`:
- Around line 340-354: Reset connectionDuration.alpha to 1f in the hasCid branch
of the connection summary rendering before showing the green active indicator,
so recycled rows do not retain the blocked-state alpha. Keep the existing
blocked and non-blocked handling unchanged.
In `@app/src/full/java/com/celzero/bravedns/ui/fragment/AboutFragment.kt`:
- Around line 659-666: Serialize calls to performMemoryProfileCapture by
tracking the active profileScope capture job and returning or ignoring
subsequent requests while it is running. Clear the tracked job when the capture
completes or fails so a later capture can start, while preserving the existing
capture and UI behavior.
- Around line 788-795: Update the post-write display-name lookup in
copyFileToTree so failures from contentResolver.query or cursor processing
cannot propagate after written is set true. Keep the document creation and write
result successful, and use the existing displayName fallback when the
best-effort lookup fails, preventing the caller from reporting export failure or
retrying.
- Around line 774-783: Update writableTree() and its orphan cleanup around
DocumentsContract.deleteDocument(): capture and return the Boolean result
instead of ignoring it, and treat false as a cleanup failure by logging it
alongside thrown exceptions. Ensure callers use the returned result so an
unsuccessful deletion is not reported as successful.
- Around line 619-624: Update the memory-profile directory flow around
isTreeUriAccessible() so its synchronous ContentResolver.query() validation runs
on Dispatchers.IO rather than the click handler’s UI thread. After validation
completes, switch to Dispatchers.Main before invoking
performMemoryProfileCapture() or deciding to launch the directory picker, while
preserving the existing stored-URI accessibility behavior.
In `@app/src/main/res/values/strings.xml`:
- Line 2099: Update the lockdown_check_ac_desc string to remove the backslashes
preceding both quoted phrases, while retaining the "Hybrid" and
"never retry" entities.
---
Nitpick comments:
In `@app/src/full/java/com/celzero/bravedns/ui/fragment/AboutFragment.kt`:
- Around line 193-195: Replace the hardcoded memory-profile UI text in
AboutFragment, including picker errors, capture toast, result text, notification
titles, and channel description, with string-resource lookups. Add the required
localized resources and placeholders for labels, filenames, sizes, and error
messages, then format them at each affected call site while preserving the
existing messages and flow.
🪄 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: 4a66af20-a47a-40f7-9d8e-2c83cf288c33
📒 Files selected for processing (22)
app/src/full/java/com/celzero/bravedns/adapter/ConnectionLogAdapter.ktapp/src/full/java/com/celzero/bravedns/adapter/ConnectionTrackerAdapter.ktapp/src/full/java/com/celzero/bravedns/customdownloader/RetrofitManager.ktapp/src/full/java/com/celzero/bravedns/service/ProxyManager.ktapp/src/full/java/com/celzero/bravedns/service/WireguardManager.ktapp/src/full/java/com/celzero/bravedns/ui/activity/AntiCensorshipActivity.ktapp/src/full/java/com/celzero/bravedns/ui/activity/ProxySettingsActivity.ktapp/src/full/java/com/celzero/bravedns/ui/activity/TunnelSettingsActivity.ktapp/src/full/java/com/celzero/bravedns/ui/fragment/AboutFragment.ktapp/src/full/java/com/celzero/bravedns/ui/fragment/DnsSettingsFragment.ktapp/src/main/java/com/celzero/bravedns/database/RefreshDatabase.ktapp/src/main/java/com/celzero/bravedns/net/go/GoVpnAdapter.ktapp/src/main/java/com/celzero/bravedns/rpnproxy/RpnProxyManager.ktapp/src/main/java/com/celzero/bravedns/service/DomainRulesManager.ktapp/src/main/java/com/celzero/bravedns/service/FirewallManager.ktapp/src/main/java/com/celzero/bravedns/service/TunFlowManager.ktapp/src/main/java/com/celzero/bravedns/tunnel/TunDnsManager.ktapp/src/main/res/values/server_table.xmlapp/src/main/res/values/servers.xmlapp/src/main/res/values/strings.xmlapp/src/test/java/com/celzero/bravedns/service/ProxyManagerTest.ktgradle.properties
💤 Files with no reviewable changes (1)
- app/src/full/java/com/celzero/bravedns/ui/fragment/DnsSettingsFragment.kt
🚧 Files skipped from review as they are similar to previous changes (7)
- app/src/main/java/com/celzero/bravedns/service/DomainRulesManager.kt
- app/src/full/java/com/celzero/bravedns/adapter/ConnectionTrackerAdapter.kt
- app/src/full/java/com/celzero/bravedns/ui/activity/ProxySettingsActivity.kt
- app/src/main/java/com/celzero/bravedns/tunnel/TunDnsManager.kt
- app/src/main/java/com/celzero/bravedns/net/go/GoVpnAdapter.kt
- app/src/main/java/com/celzero/bravedns/service/TunFlowManager.kt
- app/src/full/java/com/celzero/bravedns/ui/activity/TunnelSettingsActivity.kt
📜 Review details
⏰ Context from checks skipped due to timeout. (3)
- GitHub Check: build
- GitHub Check: Analyze (java-kotlin)
- GitHub Check: 🧭 OCR
⚠️ CI failures not shown inline (1)
GitHub Check: mobsfscan: 2 new alerts including 1 error
Conclusion: failure
### New alerts in code changed by this pull request
* 1 error
* 1 note
_Alerts not introduced by this pull request might have been detected because the code changes were too large._
See annotations below for details.
[View all branch alerts](/celzero/rethink-app/security/code-scanning?query=pr%3A2961+tool%3Amobsfscan+is%3Aopen).
🔇 Additional comments (25)
app/src/full/java/com/celzero/bravedns/customdownloader/RetrofitManager.kt (2)
18-20: LGTM!Also applies to: 35-40, 135-135, 195-217
225-225: 🩺 Stability & AvailabilityDo not memoize the pre-Koin fallback.
Line 225 caches
emptyMap()whenGlobalContext.get()fails. If the first DNS lookup occurs beforestartKoin, host pinning stays disabled for the process lifetime after Koin becomes available.Initialize pinned hosts after Koin starts, or retry after this failure.
#!/bin/bash set -euo pipefail # Map the changed resolver before tracing its initialization and call sites. ast-grep outline app/src/full/java/com/celzero/bravedns/customdownloader/RetrofitManager.kt --items all # Verify that Koin starts before any path can create and use this custom DNS resolver. rg -nP --type kt -C 4 '\b(startKoin|stopKoin)\s*\(|\bGlobalContext\b|\bRetrofitManager\b|\b(okHttpClient|getBlocklistBaseBuilder|getRpnBaseBuilder|getIpInfoBaseBuilder)\s*\(' appapp/src/main/res/values/server_table.xml (1)
24-25: LGTM!Also applies to: 48-49
app/src/main/res/values/servers.xml (1)
45-48: LGTM!app/src/test/java/com/celzero/bravedns/service/ProxyManagerTest.kt (1)
1250-1272: LGTM!Also applies to: 1274-1292, 1294-1310, 1312-1331, 1333-1348, 1350-1372, 1374-1392
app/src/full/java/com/celzero/bravedns/service/ProxyManager.kt (2)
169-177: Skip tombstoned apps in the unselected-app path.
FirewallManager.getAllApps()includes tombstoned apps. This path can recreate base and proxy mappings for uninstalled apps because it does not checkai.tombstoneTs.
34-34: LGTM!Also applies to: 142-145, 237-244, 279-281, 412-427, 485-511
app/src/full/java/com/celzero/bravedns/ui/activity/AntiCensorshipActivity.kt (2)
113-128: Apply a lockdown-compatible persisted state.
handleProxyLockdownRestrictions()only changes row alpha. An incompatible persisted dial or retry strategy remains active until the user changes it.
59-61: LGTM!Also applies to: 231-315, 341-345, 365-365
app/src/full/java/com/celzero/bravedns/service/WireguardManager.kt (2)
403-405: Make One-WireGuard replacement durable before enabling another configuration.
disableConfig()starts detached database and tunnel operations. It also returns without state changes when the parsed config is absent. The old configuration can remain active during replacement.
533-579: LGTM!Also applies to: 637-639
app/src/main/java/com/celzero/bravedns/database/RefreshDatabase.kt (1)
524-560: LGTM!Also applies to: 603-612
app/src/main/java/com/celzero/bravedns/rpnproxy/RpnProxyManager.kt (1)
1813-1832: LGTM!app/src/main/java/com/celzero/bravedns/service/FirewallManager.kt (1)
49-49: LGTM!Also applies to: 299-310, 425-429, 651-661
gradle.properties (1)
33-34: LGTM!app/src/main/res/values/strings.xml (1)
527-527: LGTM!Also applies to: 612-612, 697-698, 844-844, 1696-1700, 2107-2108, 2221-2221, 2251-2260
app/src/full/java/com/celzero/bravedns/adapter/ConnectionLogAdapter.kt (1)
66-78: LGTM!Also applies to: 172-182, 389-397
app/src/full/java/com/celzero/bravedns/ui/fragment/AboutFragment.kt (8)
197-207: Previously reported: persist the URI only after permission succeeds.
persistentState.memoryProfileDirUriis still assigned aftertakePersistableUriPermission()catchesSecurityException. The app can store a temporary grant and reuse it after process restart. This remains the unresolved issue from the previous review.
660-688: The stale Activity context issue remains in the toast path.
ctxis captured before the coroutine starts and is used at Line 688. A long capture can retain the old Activity and pass it to the toast after reattachment. UseappCtxor resolve the current context inside the Main-thread block. This is the same lifecycle issue previously reported for the notification path.
738-740: Previously reported: preserve the staging file after a failed copy.The
finallyblock deletesstagingFilewhencopyFileToTree()returnsnullor throws. This remains the unresolved data-loss issue from the previous review.
755-755: Previously reported: distinguish empty captures from copy failures.
copyFileToTree()returnsnullfor an empty source, andpublishStagedProfile()maps that result to"Failed to save to selected folder". The user still receives an incorrect storage error for an empty capture.
28-32: LGTM!Also applies to: 49-50, 115-115, 141-146, 166-169
184-192: LGTM!
635-645: LGTM!Also applies to: 667-680, 682-684, 694-704, 828-837
803-815: 🗄️ Data Integrity & IntegrationManual verification needed for URI-revocation behavior.
The repository could not be inspected in this environment; confirm whether transient
DocumentsProviderfailures can clearpersistentState.memoryProfileDirUriand whether clear should only happen after confirmed revocation/deletion.
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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/build.gradle`:
- Around line 421-426: Update the runtime Glide dependency near the
fullImplementation okhttp3-integration and kspFull compiler declarations to
version 5.0.9, aligning it with the integration and compiler artifacts.
In
`@app/src/full/java/com/celzero/bravedns/ui/activity/ServerOrderHistoryActivity.kt`:
- Around line 229-237: Update the expiry display in ServerOrderHistoryActivity
using the value from VpnController.getWinExpiryTs() as a timestamp, formatting
it into a localized user-readable date or relative expiry string before
assigning b.tvHeroExpiry.text; preserve the existing null handling and
visibility behavior.
In `@app/src/full/java/com/celzero/bravedns/ui/fragment/HomeScreenFragment.kt`:
- Around line 1792-1800: Before returning from the API 37+ missing-permission
branch in the auto-start flow, clear the active VPN state in the controller and
persisted activation state so the home-screen button reflects that the VPN is
off. Preserve the existing skip log and return behavior, ensuring the next user
tap requests permission and starts the VPN.
- Around line 1974-1982: Remove the local-network permission hard gate from the
VPN startup path around VpnController.start(), including the
hasLocalNetworkPermission() check, handleLocalNetworkPermissionDenied(), and
early return. Keep VPN startup governed by its existing protections, and enforce
ACCESS_LOCAL_NETWORK only in the direct LAN feature that requires it.
In `@app/src/main/java/com/celzero/bravedns/util/Utilities.kt`:
- Around line 620-622: Annotate Utilities.isAtleast37() with
`@ChecksSdkIntAtLeast`(api = Build.VERSION_CODES.CINNAMON_BUN) and add the
corresponding annotation import. Keep the existing SDK comparison unchanged so
lint recognizes this method as an API-level guard.
🪄 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: 090b1be8-0b81-422b-a6ed-c09a34fedd71
📒 Files selected for processing (11)
app/build.gradleapp/src/full/java/com/celzero/bravedns/ui/activity/ServerOrderHistoryActivity.ktapp/src/full/java/com/celzero/bravedns/ui/fragment/HomeScreenFragment.ktapp/src/main/AndroidManifest.xmlapp/src/main/java/com/celzero/bravedns/util/Utilities.ktapp/src/main/res/layout/activity_rethink_plus_dashboard.xmlapp/src/main/res/layout/activity_server_order_history.xmlapp/src/main/res/layout/bottomsheet_server_settings.xmlapp/src/main/res/values-v35/values.xmlapp/src/main/res/values/strings.xmlgradle.properties
💤 Files with no reviewable changes (1)
- app/src/main/res/values-v35/values.xml
🚧 Files skipped from review as they are similar to previous changes (1)
- gradle.properties
📜 Review details
⏰ Context from checks skipped due to timeout. (3)
- GitHub Check: 🧭 OCR
- GitHub Check: Analyze (java-kotlin)
- GitHub Check: build
⚠️ CI failures not shown inline (1)
GitHub Check: mobsfscan: 2 new alerts including 1 error
Conclusion: failure
### New alerts in code changed by this pull request
* 1 error
* 1 note
_Alerts not introduced by this pull request might have been detected because the code changes were too large._
See annotations below for details.
[View all branch alerts](/celzero/rethink-app/security/code-scanning?query=pr%3A2961+tool%3Amobsfscan+is%3Aopen).
🧰 Additional context used
🪛 GitHub Check: mobsfscan
app/src/full/java/com/celzero/bravedns/ui/activity/ServerOrderHistoryActivity.kt
[failure] 234-234:
Hidden elements in view can be used to hide data from user. But this data can be leaked.
🔇 Additional comments (5)
app/src/main/AndroidManifest.xml (1)
4-5: LGTM!Also applies to: 36-37, 50-51
app/src/main/res/layout/activity_rethink_plus_dashboard.xml (1)
40-45: LGTM!Also applies to: 57-63
app/src/main/res/layout/activity_server_order_history.xml (1)
39-66: LGTM!app/src/main/res/layout/bottomsheet_server_settings.xml (1)
58-62: LGTM!app/src/main/res/values/strings.xml (1)
527-527: LGTM!Also applies to: 612-612, 697-698, 799-801, 847-847, 1699-1703, 2102-2111, 2224-2224, 2254-2263, 2304-2305, 2428-2428
| # too expensive, commenting it for now. | ||
| # pull_request_target: | ||
| # types: [opened, synchronize, reopened] |
There was a problem hiding this comment.
[maintainability · low]
With the pull_request_target trigger disabled, the job-level if: condition (line ~69) and the concurrency group (line ~22) still test github.event_name == 'pull_request_target'. These branches can never evaluate to true now (only issue_comment can fire this workflow), so they are dead code and can mislead future maintainers into thinking the trigger is still active. Consider removing the pull_request_target branch from the job if: condition and the concurrency group for consistency.
| runCatching { FirewallManager.getPackageNamesByUid(log.uid) } | ||
| .getOrDefault(emptyList()) |
There was a problem hiding this comment.
[bug · low]
The comment states "On failure the entry is not cached, so the next bind retries", but runCatching { ... }.getOrDefault(emptyList()) swallows the exception and returns normally, so getOrPut DOES cache emptyList(). A transient iterator fault therefore permanently caches the empty list and future binds never retry — the uid keeps the default icon for the adapter's lifetime, which contradicts the documented intent. To match the retry behavior, wrap the whole getOrPut in runCatching so a failed compute never gets stored.
Suggestion:
| runCatching { FirewallManager.getPackageNamesByUid(log.uid) } | |
| .getOrDefault(emptyList()) | |
| val apps = runCatching { | |
| packageNameCache.getOrPut(log.uid) { | |
| FirewallManager.getPackageNamesByUid(log.uid) | |
| } | |
| }.getOrDefault(emptyList()) |
| if (blocked) { | ||
| b.connectionDuration.text = context.getString(R.string.symbol_red_circle) | ||
| b.connectionDuration.alpha = 0.7f | ||
| hasMinSummary = true | ||
| } else { | ||
| b.connectionDuration.text = "" | ||
| b.connectionDuration.alpha = 1f | ||
| } |
There was a problem hiding this comment.
[bug · medium]
The blocked branch sets hasMinSummary = true so the summary won't be hidden, but it never sets connectionSummaryLl.visibility = View.VISIBLE. Because update() doesn't clear stale view state when a holder is recycled (only clear() does, and it's only called for null items), a holder previously showing a non-blocked minimal summary (visibility = GONE) will keep the layout hidden — so the red-circle blocked indicator won't be displayed. The hasCid branch above explicitly sets VISIBLE; mirror that here.
Suggestion:
| if (blocked) { | |
| b.connectionDuration.text = context.getString(R.string.symbol_red_circle) | |
| b.connectionDuration.alpha = 0.7f | |
| hasMinSummary = true | |
| } else { | |
| b.connectionDuration.text = "" | |
| b.connectionDuration.alpha = 1f | |
| } | |
| if (blocked) { | |
| b.connectionDuration.text = context.getString(R.string.symbol_red_circle) | |
| b.connectionDuration.alpha = 0.7f | |
| b.connectionSummaryLl.visibility = View.VISIBLE | |
| hasMinSummary = true | |
| } else { | |
| b.connectionDuration.text = "" | |
| b.connectionDuration.alpha = 1f | |
| } |
| var replacedOthers = false | ||
| if (WireguardManager.isAnyOtherOneWgEnabled(config.id)) { | ||
| Logger.i(LOG_TAG_PROXY, "another one-wg config is active; replacing it") | ||
| WireguardManager.disableOtherOneWireGuardConfigs(config.id) | ||
| replacedOthers = true | ||
| } |
There was a problem hiding this comment.
[bug · high]
The new "replace" flow permanently disables the previously active one-wg config before enabling the newly selected one. disableOtherOneWireGuardConfigs() writes isActive=false/oneWireGuard=false to the DB for the old config, then enableConfig(...) runs. If enableConfig fails — either silently (early return when the mapping/config is missing) or by throwing (e.g. db.update/VpnController.addWireGuardProxy) — the old config is already disabled with no rollback, leaving the user with no active WireGuard tunnel and no error surfaced (the switch stays checked). The previous behavior blocked this case with an error, keeping the existing config intact. Consider enabling the new config first and disabling the others only on success, or wrapping the sequence in try/catch to restore the previous config and reset the switch state on failure.
| // available; otherwise falls back to a toast so the error is never silently dropped. | ||
| private fun showInvalidConfigDialog() { | ||
| val ctx = context | ||
| if (ctx is android.app.Activity && !ctx.isFinishing) { |
There was a problem hiding this comment.
[bug · medium]
showInvalidConfigDialog() only checks !ctx.isFinishing, not !ctx.isDestroyed. This code runs on the main thread after an IO round-trip, so the activity may already be destroyed (e.g. configuration change or navigating away) while not finishing, and calling .show() on a destroyed activity can throw WindowManager.BadTokenException. Add && !ctx.isDestroyed to match the existing lifecycle guards in the codebase (e.g. WgMainActivity.showOneWgToggle checks isDestroyed).
Suggestion:
| if (ctx is android.app.Activity && !ctx.isFinishing) { | |
| if (ctx is android.app.Activity && !ctx.isFinishing && !ctx.isDestroyed) { |
| fun untrackRpn() { | ||
| proxies.keys.removeAll { | ||
| it == Backend.RpnWin || it.startsWith(Backend.RpnWin) | ||
| } |
There was a problem hiding this comment.
[bug · low]
proxies.keys.removeAll { ... } on the ConcurrentHashMap key set is not atomic: the weakly-consistent iterator can race with concurrent track()/untrack() calls (e.g. a country server tracked between the hasNext/remove calls may be lost, or a stale key may survive). Also, it == Backend.RpnWin is redundant since it.startsWith(Backend.RpnWin) already covers the exact match. Consider iterating under a remove loop that only removes matching keys (proxies.keys.removeIf { it == Backend.RpnWin || it.startsWith(Backend.RpnWin) } on the CHM view, or dropping the redundant == clause) if strict consistency matters.
| try { | ||
| processProxy(id, entry, now) | ||
| } catch (e: Exception) { | ||
| Logger.w(LOG_TAG_PROXY, "$TAG err processing $id: ${e.message}") | ||
| } |
There was a problem hiding this comment.
[bug · low]
Broad catch (Exception) here (and in start()'s loop) will also swallow CancellationException thrown by nested suspend calls (hasProxy/hasRpnProxy/registerProxy etc.) when the coroutine is cancelled. Although the while (isActive) guard eventually terminates the loop, swallowing the cancellation signal is fragile and can delay/break cooperative cancellation. Prefer catching CancellationException first and rethrowing it (e.g. catch (e: CancellationException) { throw e }), or narrow the catch to the expected failure types.
| id.startsWith(ID_WG_BASE) -> { | ||
| val proxyId = id.substring(ID_WG_BASE.length).toIntOrNull() | ||
| proxyId != null && WireguardManager.getConfigById(proxyId) != null | ||
| } |
There was a problem hiding this comment.
[bug · high]
getConfigById() searches the configs set, which retains configs even after they are disabled (disableConfig only flips isActive in mappings; the entry is removed from configs only on delete). So a disabled-but-not-deleted WireGuard config is still considered "wanted" here. Combined with GoVpnAdapter.removeWgProxy returning early WITHOUT untrack() when the tunnel is not connected, a config disabled while the tunnel is down remains tracked and will be re-added via VpnController.addWireGuardProxy(id, force = true) once the tunnel is back — silently routing traffic through a proxy the user disabled (privacy regression). Use the active-state check (e.g. WireguardManager.isConfigActive(id)) instead of mere existence.
| else -> { | ||
| Logger.w(LOG_TAG_PROXY, "$TAG unknown proxy id, skip re-add: $id") | ||
| } |
There was a problem hiding this comment.
[bug · high]
S5/HTTP/Orbot proxies are tracked (GoVpnAdapter.setSocks5Proxy/setHttpProxy call track() for ID_S5_BASE/ID_HTTP_BASE/ID_ORBOT_BASE) and GoVpnAdapter even exposes readdSocks5Proxy()/readdHttpProxy()/readdCustomProxy() explicitly documented as "used by the global proxy handler when these proxies go missing". However this else-branch only logs "unknown proxy id, skip re-add" for those ids, so the handler never actually heals them. They burn the attempt budget and are eventually dropped with a HIGH-severity 'proxy re-add failed' event. Moreover, none of the S5/HTTP/Orbot removal paths call untrack(), so a user-disabled proxy stays tracked and produces a misleading HIGH event. Suggest dispatching these ids to VpnController.readdSocks5Proxy()/readdHttpProxy()/readdCustomProxy() (or not tracking them at all).
| // IPv4 VPN constants | ||
| // changing the below ip should require a changes in ConnectionTracer, RethinkLogAdapter | ||
| private const val IPV4_TEMPLATE: String = "10.111.222.%d" | ||
| private const val IPV4_PREFIX_LENGTH: Int = 24 |
There was a problem hiding this comment.
[maintainability · low]
These constants are dead code in this file: IPV4_PREFIX_LENGTH, IPV6_PREFIX_LENGTH, MIN_MTU, and MAX_MTU are never referenced inside TunFlowManager. The live copies are in BraveVPNService (lines 248–260) and ConnectionMonitor (line 926), and the unit test references BraveVPNService.MIN_MTU. Keeping a second, unused copy here invites future drift (e.g. editing one MTU constant and not the other). Remove the unused declarations.
| private var lastRethinkBlockReason: Int = -1 | ||
| private var lastRxTrafficTime: Long = elapsedRealtime() |
There was a problem hiding this comment.
[bug · medium]
These fields are written/read from different threads without synchronization: handlePostflow() writes lastRxTrafficTime from Go/JNI callback threads, while BraveVPNService.maybeNetworkStall() reads it (via getLastRxTrafficTime()) on the serializer/looper context. lastRethinkBlockReason is also mutated from flow threads. In the original BraveVPNService these fields were @Volatile; after the move they are plain mutable vars, so cross-thread visibility is no longer guaranteed — a stale read can make the 30s data-stall check falsely trigger a VPN restart, and duplicate/suppressed Rethink-block notifications are possible. Restore @Volatile on both.
Suggestion:
| private var lastRethinkBlockReason: Int = -1 | |
| private var lastRxTrafficTime: Long = elapsedRealtime() | |
| @Volatile | |
| private var lastRethinkBlockReason: Int = -1 | |
| @Volatile | |
| private var lastRxTrafficTime: Long = elapsedRealtime() |
| cm.isBlocked = if (proxyRule.isEmpty()) true else cm.isBlocked | ||
| cm.blockedByRule = proxyRule.ifEmpty { FirewallRuleset.RULE18.id } |
There was a problem hiding this comment.
[bug · medium]
For connections tracked via trackedConnMetaData (i.e. multi-proxy conns that were allowed), a postflow whose s.pid is empty or a local proxy (Base/Exit — isNotLocalAndRpnProxy is false) forces isBlocked = true and overwrites blockedByRule with RULE18. If such a connection legitimately completed via the local/Exit fallback (e.g. RPN list [rpn1, exit] where the RPN failed), the traffic was allowed but is now logged as "blocked by rule 18", corrupting network logs, per-app stats, and UI-reported block reasons — and any more specific earlier firewall decision is lost. Please confirm this is intentional; otherwise only mark blocked when the connection truly failed to establish.
| } | ||
|
|
||
| // if `d` is blocked, then at least one of the real ips is unspecified | ||
| val anyRealIpBlocked = !ips.none { isUnspecifiedIp(it.trim()) } |
There was a problem hiding this comment.
[style · low]
Minor readability: !ips.none { isUnspecifiedIp(it.trim()) } is a double negative; ips.any { ... } expresses the same intent more directly.
Suggestion:
| val anyRealIpBlocked = !ips.none { isUnspecifiedIp(it.trim()) } | |
| val anyRealIpBlocked = ips.any { isUnspecifiedIp(it.trim()) } |
| val lastColonIndex = trimmed.lastIndexOf(':') | ||
| if (lastColonIndex > 0) { | ||
| val potentialPort = trimmed.substring(lastColonIndex + 1) | ||
| if (potentialPort.toIntOrNull() != null) { | ||
| val ipPart = trimmed.substring(0, lastColonIndex) | ||
| // avoid cutting an IPv6 without port (like 2001:db8::1) | ||
| val colonCount = ipPart.count { it == ':' } | ||
| val ip = if (colonCount >= 2) ipPart else trimmed | ||
| val port = if (colonCount >= 2) potentialPort.toIntOrNull() ?: 0 else 0 | ||
| if (colonCount >= 2) return ip to port |
There was a problem hiding this comment.
[bug · medium]
Port-less unbracketed IPv6 endpoints are misparsed as "ip:port". For 2001:db8::1 (or the VPN's fake-DNS IPv6 fd66:f83a:c650::3), lastColonIndex points at the final ':' before the numeric hextet, so ipPart = "2001:db8::" with colonCount = 3 >= 2 — the branch returns host "2001:db8::" with port 1, corrupting the endpoint. The colonCount >= 2 guard does not distinguish "IPv6 with port" from plain IPv6 (every valid IPv6 has ≥2 colons), defeating the comment's stated intent. Since this parser feeds getUid() and firewall/metered decisions in preflow/flow/inflow for every endpoint, any port-less IPv6 flow (e.g. ICMP) will get a corrupted host/port. Consider validating ipPart as an actual IPv6 address (e.g. IPAddressString(ipPart).isIPv6()) before treating the trailing segment as a port, or let HostName handle it in a try/catch.
| val ipv4 = HostName(ipv4Parts).toString() | ||
| val ipv6 = HostName(ipv6Parts).toString() |
There was a problem hiding this comment.
[bug · low]
HostName(ipv4Parts) / HostName(ipv6Parts) are invoked without any try/catch on persistentState.customLanDnsIpv4/customLanDnsIpv6. Unlike parseIpAndPort() (which defensively wraps the same call), a malformed/empty preference value here would throw from inside the flow() data path, propagating through the Go callback and potentially disrupting the VPN session. Defaults are well-formed, but a defensive try/catch (falling back to the non-custom comparison) would make this robust against bad user input.
| if (duration == 60) { | ||
| binding.savingsText.text = | ||
| context.getString(R.string.save_percentage, "45%") | ||
| } else { | ||
| binding.savingsText.visibility = View.GONE | ||
| binding.savingsText.text = | ||
| context.getString(R.string.save_percentage, "35%") | ||
| } |
There was a problem hiding this comment.
[bug · medium]
The else branch also matches duration == 0, which getInAppDurationMonths returns for any unrecognized plan ID (its else -> 0). For such an INAPP plan the badge would still claim "Save 35%" even though the actual duration/savings is unknown and the figure is likely inaccurate. Consider handling duration == 0 explicitly (hide the savings badge or fall back to calculateSavings).
Suggestion:
| if (duration == 60) { | |
| binding.savingsText.text = | |
| context.getString(R.string.save_percentage, "45%") | |
| } else { | |
| binding.savingsText.visibility = View.GONE | |
| binding.savingsText.text = | |
| context.getString(R.string.save_percentage, "35%") | |
| } | |
| if (duration == 60) { | |
| binding.savingsText.text = | |
| context.getString(R.string.save_percentage, "45%") | |
| } else if (duration > 0) { | |
| binding.savingsText.text = | |
| context.getString(R.string.save_percentage, "35%") | |
| } else { | |
| binding.savingsText.visibility = View.GONE | |
| } |
| @Volatile | ||
| var lastProductQueryResponseCode: Int = BillingResponseCode.OK |
There was a problem hiding this comment.
[bug · medium]
lastProductQueryResponseCode is shared mutable state on a singleton that is read by RethinkPlusFragment.productResult() only after the listener notification is dispatched, and it is never reset at the start of a query run. queryProductDetails/queryProductDetailsWithTimeout can be triggered concurrently from several paths (onBillingSetupFinished at line 556, purchase flows at 2188/2267, and multiple ViewModel triggers). If run A times out and posts its failure notification to the main thread, but run B (started meanwhile) completes first and overwrites this field, run A's productResult(false, emptyList()) will read run B's code — e.g. OK → the user sees EMPTY_RESPONSE instead of REQUEST_TIMEOUT. Conversely the timeout path can overwrite a fresh OK before run B's own notification is read. Suggest resetting lastProductQueryResponseCode = BillingResponseCode.OK at the start of each query run, and ideally passing the response code through the productResult callback instead of reading a shared global after the fact.
| loge(mname, "queryEntitlement server business error for token=${pt.take(8)}; no linkedPurchaseId, preserving local purchase (fail-safe)") | ||
| } | ||
| result.purchase |
There was a problem hiding this comment.
[bug · medium]
This changes the entitlement semantics for a server business error without a linkedPurchaseId: previously the local purchase was expired (payload cleared, expiry zeroed, i.e. treated like Expired); now result.purchase (the original, unchanged purchase) is preserved in both branches. Per QueryEntitlementResult.Failure, this result means the server was reachable and explicitly reported the purchase as not valid (e.g. revoked/refunded). Preserving it locally means a server-confirmed-invalid purchase continues to grant entitlement — and for one-time INAPP purchases (no advancing expiry) that is indefinite. If the fail-safe intent is to only guard against transient/ambiguous errors, consider still expiring when the server reports a definitive revoke/refund with no linked purchase, or at least bounding the preserved entitlement to the local billing window.
| isRetryable = error.isRetryable, | ||
| reason = getApplication<Application>().getString(error.reasonRes) |
There was a problem hiding this comment.
[other · medium]
The hardcoded message "Unable to load plans. Please try again." is now shown even when error.isRetryable is false (e.g. BILLING_UNAVAILABLE or AUTHENTICATION_FAILURE), while the fragment hides the retry button for non-retryable states (b.retryButton.isVisible = isRetryable). The primary message contradicts the retry policy and can confuse users who are told to "try again" but given no retry affordance. Consider selecting the message based on error.isRetryable (e.g., a non-retryable wording like "Unable to load plans" without the "try again" instruction) so the text matches the state.
| BillingResponseCode.USER_CANCELED -> SERVER_ERROR | ||
| BillingResponseCode.ITEM_NOT_OWNED -> SERVER_ERROR |
There was a problem hiding this comment.
[maintainability · low]
USER_CANCELED, ITEM_ALREADY_OWNED, ITEM_NOT_OWNED, FEATURE_NOT_SUPPORTED and ITEM_UNAVAILABLE are purchase/entitlement-flow codes that are semantically unrelated to a product-details query failure. Mapping them to SERVER_ERROR would render the misleading reason "Google Play is temporarily unavailable" if any of these ever surfaces during queryProductDetails. These codes cannot occur during a product query; prefer mapping them to UNKNOWN (or a dedicated value) so the shown reason is not misleading.
2 participants
Summary by CodeRabbit