[PM-39294] Gate shared unlock handshakes on transport reachability - #1221
Draft
coroiu wants to merge 14 commits into
Draft
[PM-39294] Gate shared unlock handshakes on transport reachability#1221coroiu wants to merge 14 commits into
coroiu wants to merge 14 commits into
Conversation
The crates.io 0.7.x releases of crypto-bigint were yanked after 0.7.5 shipped a bugfix, so a refreshed registry index fails to resolve `crypto-bigint = "^0.7"` (required by rsa 0.10.0-rc.18). Pin crypto-bigint to the published 0.7.5 from git so the workspace resolves regardless of index state, and bump rsa to rc.18 to match. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add a `Reachability { Reachable, Unreachable, Unknown }` signal and a
`CommunicationBackend::reachability(endpoint)` method (default `Unknown`) so a
transport can answer authoritatively when it knows its own connectivity (e.g. a
native-messaging port being connected). The default keeps existing backends
unchanged. Wire the optional `reachability?(endpoint)` method through the WASM
`IpcCommunicationBackendSender` interface, mapping a missing/throwing
implementation to `Unknown`.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add a ReachabilityTracker that hands out scoped ReachabilityHandles per endpoint. A handle reports reachability by pulling the transport-native signal on demand and, only when that is Unknown, falling back to a ping/pong liveness window. Each tracked endpoint runs one adaptive ping loop (pinging only while the transport is Unknown); handles for the same endpoint share it via a weak registry, and the loop stops once the last handle is dropped. Registry entries are cleaned deterministically on Drop so the map stays bounded to currently-tracked endpoints. Reserved control-topic namespace ($bw.control.*) marks ping/pong frames for the crypto bypass. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The IpcClient now owns one ReachabilityTracker, built over a ControlSplitter that wraps the raw backend. The splitter routes reserved control-topic frames to the tracker (a single handler task answers pings and records liveness) and hides them from the crypto layer's receivers, so NoiseCryptoProvider stays unchanged and never decodes a ping as a handshake frame. Expose the tracker via IpcClient::reachability() so consumers can track a peer and gate their own sends; IpcClient::send stays a dumb pipe. The in-file test crypto provider is generalized over the backend type. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The follower now tracks its discovered leader's reachability (holding the handle for its lifetime so the ping loop persists) and skips StartSession and heartbeat sends while the leader is unreachable. This is what stops the Noise handshake churn and "[IPC] Failed to connect to Bitwarden Desktop App" error spam when the desktop app is absent; a leader whose transport answers Unknown becomes reachable once the ping/pong round trip lands. Adds a settable reachability to TestCommunicationBackend (default Reachable) to drive the gating in tests, and guards the wasm-only tsify attribute so the crate's tests compile off-wasm. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add IpcClient.reachability() returning a ReachabilityTracker whose track(endpoint) yields a ReachabilityHandle with an async isReachable(). The handle's generated free() stops tracking immediately; if it is not called, GC finalization eventually drops the handle and the ping loop stops on its own. Also records the web-time lock entry introduced with the tracker module. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add integration tests covering the FFI path: an Unknown transport is probed with ping/pong and becomes reachable once a pong lands; a transport that answers Reachable/Unreachable is trusted directly and never pinged. Extend the mock transport with an optional reachability and have the shared-unlock setups report Reachable so their (now reachability-gated) follower starts sessions immediately. Also feature-detect the optional reachability method on the JS sender before calling it: invoking a missing async method traps, which under panic=abort poisons the whole wasm instance. Absent or rejecting implementations fall back to Unknown. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
coroiu
added
enhancement
Contributor
🤖 Bitwarden Claude Code ReviewOverall Assessment: APPROVE Reviewed the transport-reachability layer for shared-unlock followers: the new Code Review DetailsNo blocking findings. Notes considered and intentionally not raised as findings:
Previously-raised concerns (control-handler cancellation, |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Contributor
🔍 SDK Breaking Change DetectionSDK Version:
Breaking change detection uses the build of the SDK from this branch, including any incompatibities pre-existing on or merged into this branch. Check the workflow logs to confirm. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1221 +/- ##
==========================================
- Coverage 85.10% 85.09% -0.01%
==========================================
Files 467 473 +6
Lines 64408 64816 +408
==========================================
+ Hits 54817 55158 +341
- Misses 9591 9658 +67 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
coroiu
commented
Jun 29, 2026
- Move the reserved control topics and is_control_topic into a dedicated `control` module instead of living in reachability. - Extract a `prune` helper for the registry cleanup duplicated in track() and Drop. - Rewrite the ReachabilityTracker public doc from a consumer's perspective. - Merge the two cfg_attr attributes on Reachability into one. - Move the WASM ReachabilityTracker/Handle wrappers into their own wasm/reachability.rs. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The control-frame handler task previously looped forever and was never tied to the client's lifecycle, so it leaked across a stop/restart (and a restart would spawn a second responder). Pass it the client's CancellationToken and select! on it so it stops cleanly on shutdown. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
CI runs clippy/rustdoc with `-D warnings`, which turned two pre-existing warnings into errors (the local pre-commit hook does not use `-D warnings`, so they slipped through): - `ControlSplitter`/`ControlSplitterReceiver` are `pub(crate)` but appear in the public `IpcClientImpl` trait bounds (`private_interfaces`). Make them `#[doc(hidden)] pub` and re-export them (hidden) so the bound is satisfiable without exposing them as real API. - Remove redundant explicit link targets in the wasm reachability docs (`rustdoc::redundant_explicit_links`). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Replace the `web_time::Instant` timestamp window with a sleep-based liveness check: a control frame from the endpoint sets `live` immediately, and the per-endpoint ping loop clears it after a cycle with no reply. This removes the `web-time` dependency (std's `Instant` panics on wasm, which is why it was added) without pulling in a new crate, and works identically on native and wasm. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Make the ControlSplitter a pure data/control demux: it no longer holds the tracker or spawns anything. It exposes `subscribe_control()` returning a `ControlReceiver` whose `receive()` yields typed `ControlMessage`s (ping/pong), mirroring `CommunicationBackendReceiver`. The crypto-facing `subscribe()` still drops control frames. The ReachabilityTracker now owns its inbound loop: `IpcClient` calls `tracker.start()` with the control receiver + cancellation token, and the tracker consumes the stream internally (recording liveness, answering pings) — so `handle_inbound` is private and the splitter no longer needs to know about reachability. The tracker is also split into a `reachability/` module: `mod.rs` (shared inner state), `handle.rs` (ReachabilityHandle), and `tracker.rs` (ReachabilityTracker + loops). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Rename TrackerInner to ReachabilityHandleInner and treat it as the handle's shared inner state: the per-endpoint ping loop is now a method on it (`spawn_loop`), and the registry is keyed by `Weak<ReachabilityHandleInner>`. Liveness mutation is exposed via `mark_alive`, so the tracker no longer reaches into the inner's fields. The tracker keeps its role as factory + registry + inbound responder (`track`, `start`, `handle_inbound`); the handle (its inner) owns its loop. No public API or behavior change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…pported transports Rename Reachability::Unknown to Reachability::Unsupported: a transport either supports reachability (answers Reachable/Unreachable) or it doesn't (Unsupported), and answers consistently per endpoint. Given that contract, the per-endpoint ping loop now exits as soon as the transport returns a definitive answer — is_reachable() reads a supported transport on demand, so no loop is needed there. Only Unsupported transports keep the ping/pong loop running. Removes POLL_INTERVAL (no more idle polling of authoritative transports). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-39294
Companion clients PR: bitwarden/clients#21551
📔 Objective
Shared-unlock followers repeatedly attempt Noise crypto handshakes against a leader (e.g. the desktop app) that often isn't running. Each attempt hits the 2s handshake timeout and surfaces as error spam plus reconnect churn. This introduces a lightweight reachability layer so a follower only talks to a leader it knows is present.
Design (an alternative to #1213):
CommunicationBackend::reachability(endpoint) -> Reachable | Unreachable | Unknown(defaultUnknown). A definite transport answer wins; onlyUnknownfalls back to a plaintext ping/pong liveness window. So browser↔desktop (native port state is known) never pings, while web↔extension (postMessage, no synchronous signal) uses ping/pong.ReachabilityTracker::track(endpoint)after discovering the peer and hold aReachabilityHandle; dropping the last handle stops that endpoint's ping loop. One loop per endpoint via a weak registry, with deterministic cleanup onDrop.IpcClient::sendstays a dumb pipe; the shared-unlockFollowerchecksis_reachable()beforestart_sessions/heartbeat.ControlSplitterbackend decorator peels reserved control-topic ($bw.control.*) frames off before any crypto receiver sees them and routes them to the tracker, soNoiseCryptoProvideris unchanged — no handshake-abort guard needed.🚨 Breaking Changes
CommunicationBackendgains areachability()method, but it has a default (Unknown) so existing backends are unaffected.⏰ Reminders
bitwarden-ipc(40 tests),bitwarden-shared-unlock(14 tests), and wasm integration tests (reachability round-trip, transport-authoritative, shared-unlock, biometrics).crypto-bigint/rsacommit is a build-resolution fix (yanked crates.io 0.7.x); it can be reviewed/reverted independently.🤖 Generated with Claude Code