You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Introduces packages/bull_tor as a workspace member, the single place that owns Tor for the app. Nothing consumes it in this PR — the following PRs in the stack migrate the two consumers and then delete the legacy integration.
The package carries a domain contract (TorRepository, TorConnectionState, TorRoute, TorSession, TorTransport, TorFailure) and two backends behind it: OnionTorBackend, an embedded arti client driven through the onion plugin, and an external SOCKS backend for a user-provided proxy such as Orbot. A lifecycle controller puts the client dormant in the background and wakes it on resume, so a long-lived embedded Tor does not keep circuits alive while the app is not in use.
Sessions are isolated per consumer rather than shared: each caller opens its own session so RecoverBull traffic and Electrum traffic cannot be linked to one another by the exit side.
bull_sdk moves from 62977e2f to 88e05c9, the merge commit of SatoshiPortal/bull_sdk#19, which is what ships the onion package. The same rev is pinned in packages/bull_tor and packages/bull_payjoin, so every entry point resolves to one bull_sdk revision. That merge also carries the cargokit fix that stops unrequested Android ABIs being built, so a debug flutter run on an arm64 device compiles the Rust once instead of three times.
Known limitation, deliberately not addressed here. In automatic mode the switch from direct to snowflake is triggered only by a terminal failure of the direct attempt — see _shouldUseSnowflake in lib/src/data/tor_repository_impl.dart. Arti reports a censorship-suggesting blockage on the status stream well before it gives up, and the UI already reacts to that signal, but the repository does not: it waits for the attempt to fail, which in the worst case means our own 120s BOOTSTRAP_TIMEOUT in onion. A censored user can therefore wait up to two minutes before Snowflake is even attempted. How early arti actually errors out under real filtering has not been measured. Reacting to the settled stream diagnostic instead — cancelling the direct attempt, which stop() plus the generation counter already make possible — is the fix, and it belongs in its own PR.
Verified: flutter analyze --fatal-warnings --fatal-infos clean, dart fix --dry-run reports nothing, dart format reports no changes, and the package's own suites pass (tor_repository_test.dart, tor_lifecycle_controller_test.dart, external_socks_tor_backend_test.dart), including the automatic-mode fallback and the transport-memory cases.
Review for correctness bugs, security issues, regressions
Post findings
Note: I couldn't post inline review comments — the gh api write call needs an approval this workflow doesn't grant — so all findings are in this comment with file/line links. I also couldn't fetch the onion package source from this environment, so finding 1 is caveated on its stop() semantics.
The package is carefully built: generation counters and lifecycle serialization are reasoned about explicitly, the SOCKS policy narrowing is well documented, and the tests cover the racy paths (stale generations, mid-session mode change, non-monotonic status). I found no security issues. The findings below are correctness/robustness, roughly in severity order.
1. Unguarded second service.stop() in _cleanup() can make ensureReady()/retry()/close() reject with a raw exception — onion_tor_backend.dart:317-321
OnionTorBackend.stop() guards its direct service.stop() in try/catch (line 268-273) — showing a throw is anticipated — then queues _stop → _cleanup(), which calls await service?.stop() on the same, already-stopped service with no guard. The existence of onion.TorFailureKind.notRunning suggests stopping a dead service can throw. If it does, the exception escapes _cleanup and propagates in two damaging ways:
TorRepositoryImpl._connect line 168:await _embeddedTor.stop() sits outside the attempt loop's try/catch. Everything else in _connect resolves to a TorConnectionState, but a throw here makes ensureReady()/retry() reject with a raw exception and no TorUnavailable is ever emitted — consumers awaiting the state get an unhandled error instead.
_start's catch blocks (lines 154-164):await _cleanup() runs inside each catch; if it throws, the original mapped failure is replaced by the raw error. A TorBootstrapTimeoutFailure then surfaces as TorUnexpectedFailure in the repository, and _shouldUseSnowflake no longer matches — the automatic → Snowflake fallback is silently defeated in exactly the censored-network case it exists for.
close(): repository close() awaits _embeddedTor.close() before _changes.close(); a throw leaks the state stream.
Cheap fix regardless of onion's actual semantics: wrap service?.stop() in _cleanup in the same try/log pattern stop() already uses (keeping the finally for the Snowflake lease). If onion's stop() is verified idempotent-and-non-throwing, this drops to a hardening nit — but the guard costs three lines. Fix this →
state != AppLifecycleState.resumed treats inactive as dormant. On iOS, inactive fires for transient interruptions that don't mean "app not in use": biometric prompts, system alerts, the notification/control-center pull, the app switcher. This app shows biometric prompts in sensitive flows — a RecoverBull request in flight when Face ID appears would have arti put dormant mid-request. Whether that stalls existing circuits depends on arti's soft-dormancy semantics (which I couldn't verify from here), but at minimum it churns dormancy on every notification-shade peek. Consider going dormant only on paused/hidden/detached, or debouncing the transition.
The design accepts non-monotonic states (the "forwards non-monotonic embedded state without latching ready" test), so _current can be TorConnecting while the client is healthy and _inFlight is null. An ensureReady() call in that window skips the cached-ready path and goes to _begin → _connect, whose first act is _embeddedTor.stop() — a full teardown and re-bootstrap of a client arti was merely reporting a hiccup for. The backend's adopt-existing path in _start (lines 78-86) is unreachable from this flow because _connect always stops first. Stop-first is right for retry() (the doc comment says why), but for a plain ensureReady() it converts a transient dip into a multi-second re-bootstrap. Worth distinguishing the two entry points, or waiting out an in-progress TorConnecting when the listener is still alive.
The repository emits TorConnecting (tor_repository_impl.dart:199), then the backend publishes EmbeddedTorStopped followed by EmbeddedTorConnecting(0), so every start/retry emits Connecting → Stopped → Connecting. A UI that renders TorStopped as "Tor is off" will flicker or log a misleading transition. Minor, but easy to trip on when the consumers migrate in the next PRs.
InternetAddress(endpoint.host) requires an IP literal, but TorProxyEndpoint deliberately accepts free-form hosts — tryParse's own doc says values are hand-typed in the Electrum advanced options, so localhost:9050 is a valid endpoint that crashes the factory with an unmodeled ArgumentError. Latent today (the only consumer uses the embedded loopback endpoint), but the public types allow the bad combination. Either constrain TorProxyEndpoint.host to IP literals, or handle non-literals in the factory.
Each read calls _socket.listen, and Socket is single-subscription, so a second read on the same connection throws StateError. Fine for the current one-greeting use, but nothing in the SocketPort contract says so — a doc line on the port would prevent a future SOCKS-handshake extension (e.g. reading the method-select and connect replies) from hitting it at runtime.
Security: no issues found, and several deliberate positives worth keeping: SocksPolicy.onionOnly narrows what the unauthenticated loopback listener is worth to a hostile local app (and the tradeoff is honestly documented); per-consumer sessions prevent exit-side linkage of RecoverBull and Electrum traffic; nothing secret is logged (ports, percentages, transport names only); Tor state is excluded from iCloud backup on iOS and Android already has allowBackup="false"; the non-nullable endpoint in TorHttpClientFactory compile-time-blocks the clearnet-to-onion footgun.
Nits / notes:
ARCHITECTURE.md "Current state" says packages/tor (bull_tor) — the directory is packages/bull_tor; the Tor-package paragraph below it has it right.
The iOS deployment floor moves from 14.0 to 15.0 (Podfile, pbxproj, AppFrameworkInfo). That drops iOS 14 devices and isn't mentioned in the PR description — worth a line there confirming it's forced by the onion pod.
The known Snowflake-fallback latency limitation is clearly documented in the PR description and deferred deliberately — not re-flagged here.
I couldn't run make analyze/make unit-test in this runner (no Flutter toolchain); the CI checks job covers both.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Introduces
packages/bull_toras a workspace member, the single place that owns Tor for the app. Nothing consumes it in this PR — the following PRs in the stack migrate the two consumers and then delete the legacy integration.The package carries a domain contract (
TorRepository,TorConnectionState,TorRoute,TorSession,TorTransport,TorFailure) and two backends behind it:OnionTorBackend, an embedded arti client driven through theonionplugin, and an external SOCKS backend for a user-provided proxy such as Orbot. A lifecycle controller puts the client dormant in the background and wakes it on resume, so a long-lived embedded Tor does not keep circuits alive while the app is not in use.Sessions are isolated per consumer rather than shared: each caller opens its own session so RecoverBull traffic and Electrum traffic cannot be linked to one another by the exit side.
bull_sdkmoves from62977e2fto88e05c9, the merge commit of SatoshiPortal/bull_sdk#19, which is what ships theonionpackage. The same rev is pinned inpackages/bull_torandpackages/bull_payjoin, so every entry point resolves to onebull_sdkrevision. That merge also carries the cargokit fix that stops unrequested Android ABIs being built, so a debugflutter runon an arm64 device compiles the Rust once instead of three times.Known limitation, deliberately not addressed here. In automatic mode the switch from
directtosnowflakeis triggered only by a terminal failure of the direct attempt — see_shouldUseSnowflakeinlib/src/data/tor_repository_impl.dart. Arti reports a censorship-suggesting blockage on the status stream well before it gives up, and the UI already reacts to that signal, but the repository does not: it waits for the attempt to fail, which in the worst case means our own 120sBOOTSTRAP_TIMEOUTinonion. A censored user can therefore wait up to two minutes before Snowflake is even attempted. How early arti actually errors out under real filtering has not been measured. Reacting to the settled stream diagnostic instead — cancelling the direct attempt, whichstop()plus the generation counter already make possible — is the fix, and it belongs in its own PR.Verified:
flutter analyze --fatal-warnings --fatal-infosclean,dart fix --dry-runreports nothing,dart formatreports no changes, and the package's own suites pass (tor_repository_test.dart,tor_lifecycle_controller_test.dart,external_socks_tor_backend_test.dart), including the automatic-mode fallback and the transport-memory cases.