[PM-27126] Acquire & inject cookies from ServerCommunicationConfigClient#872
Open
addisonbeck wants to merge 21 commits intomainfrom
Open
[PM-27126] Acquire & inject cookies from ServerCommunicationConfigClient#872addisonbeck wants to merge 21 commits intomainfrom
addisonbeck wants to merge 21 commits intomainfrom
Conversation
Contributor
|
New Issues (1)Checkmarx found the following issues in this Pull Request
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #872 +/- ##
==========================================
+ Coverage 82.39% 82.43% +0.03%
==========================================
Files 361 363 +2
Lines 43069 43288 +219
==========================================
+ Hits 35488 35684 +196
- Misses 7581 7604 +23 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Contributor
🔍 SDK Breaking Change Detection ResultsSDK Version:
Breaking change detection completed. View SDK workflow |
addisonbeck
added
the
ai-review-vnext
Contributor
Bitwarden Claude Code ReviewOverall Assessment: APPROVE This PR introduces SSO load balancer cookie middleware for self-hosted Bitwarden deployments requiring session affinity. The implementation adds a Code Review DetailsNo actionable findings. Previous review findings (Notify race condition, silent cookie header encoding) have been addressed in subsequent commits. Dependency Changes
All dependencies are existing workspace dependencies or internal crates. No new external dependencies introduced. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Comment on lines
+140
to
+142
| if let Some(notify) = notify_to_wait { | ||
| // Wait for the in-flight acquisition to complete. | ||
| notify.notified().await; |
There was a problem hiding this comment.
Notify-based deduplication can cause permanent deadlock on multi-threaded runtimes
Details and fix
notify_waiters() only wakes tasks that are already registered as waiters. The Notified future only registers when first polled (at the .await point). There is a window between releasing the in_flight mutex lock (line 138) and polling notify.notified().await (line 142) where Task A (the acquirer) can complete, call notify_waiters() with zero registered waiters, and remove the Notify from the map. Task B then calls .notified().await on a Notify that will never fire again, blocking forever.
Timeline on a multi-threaded tokio runtime:
- Task B: locks
in_flight, clones Arc to Notify, unlocks - Task A (other thread): finishes
acquire_cookie, locksin_flight, removes Notify, callsnotify_waiters()(zero waiters registered), unlocks - Task B: calls
notify.notified().await-- permanent hang
Fix: Register the waiter while still holding the lock, then await it after dropping the lock:
let future_to_wait = {
let mut in_flight = self.in_flight.lock().await;
if let Some(notify) = in_flight.get(&hostname) {
let future = notify.notified();
// Pin and enable so it registers as a waiter now
let mut future = std::pin::pin!(future);
future.as_mut().enable();
Some(future)
} else {
in_flight.insert(hostname.clone(), Arc::new(tokio::sync::Notify::new()));
None
}
};
if let Some(future) = future_to_wait {
future.await;
} else {
// ... acquire and notify as before
}Alternatively, consider tokio::sync::watch or tokio::sync::broadcast which store the last value and don't have this race.
addisonbeck
added a commit
that referenced
this pull request
Mar 31, 2026
Notified futures only register as waiters when first polled. On a multi-threaded tokio runtime, the acquirer could call notify_waiters() between the lock release and the first poll of .notified(), causing the waiter to block forever. Fix by cloning the Arc<Notify> and calling .enable() on the pinned Notified future while still holding the lock, so the waiter is registered before notify_waiters() can fire. The lock is then explicitly dropped before .await, preserving the existing non-blocking hold semantics. Identified by Claude's inline review comment on PR #872.
addisonbeck
added a commit
that referenced
this pull request
Mar 31, 2026
Notified futures only register as waiters when first polled. On a multi-threaded tokio runtime, the acquirer could call notify_waiters() between the lock release and the first poll of .notified(), causing the waiter to block forever. Fix by cloning the Arc<Notify> and calling .enable() on the pinned Notified future while still holding the lock, so the waiter is registered before notify_waiters() can fire. The lock is then explicitly dropped before .await, preserving the existing non-blocking hold semantics. Identified by Claude's inline review comment on PR #872.
addisonbeck
commented
Mar 31, 2026
| @@ -15,9 +15,7 @@ license-file.workspace = true | |||
| keywords.workspace = true | |||
|
|
|||
| [features] | |||
| no-memory-hardening = [ | |||
| "bitwarden-core/no-memory-hardening", | |||
| ] # Disable memory hardening features | |||
Contributor
Author
There was a problem hiding this comment.
These got adjusted during some formatting command. The comments seem superfluous to me, so I left the change even though it's unrelated to this work.
addisonbeck
added a commit
that referenced
this pull request
Apr 7, 2026
Notified futures only register as waiters when first polled. On a multi-threaded tokio runtime, the acquirer could call notify_waiters() between the lock release and the first poll of .notified(), causing the waiter to block forever. Fix by cloning the Arc<Notify> and calling .enable() on the pinned Notified future while still holding the lock, so the waiter is registered before notify_waiters() can fire. The lock is then explicitly dropped before .await, preserving the existing non-blocking hold semantics. Identified by Claude's inline review comment on PR #872.
addisonbeck
added a commit
that referenced
this pull request
Apr 7, 2026
Notified futures only register as waiters when first polled. On a multi-threaded tokio runtime, the acquirer could call notify_waiters() between the lock release and the first poll of .notified(), causing the waiter to block forever. Fix by cloning the Arc<Notify> and calling .enable() on the pinned Notified future while still holding the lock, so the waiter is registered before notify_waiters() can fire. The lock is then explicitly dropped before .await, preserving the existing non-blocking hold semantics. Identified by Claude's inline review comment on PR #872.
PM-27126 introduces a ServerCommunicationConfigMiddleware that injects SSO load balancer cookies and retries on 302/307 redirects. This middleware requires reqwest-middleware (Middleware trait), reqwest (Request/Response types), and http (header injection). Adding these workspace dependencies is the prerequisite for all subsequent middleware implementation commits.
PM-27126 requires the middleware to hold Arc<dyn CookieProvider> so that bitwarden-core can inject cookies without taking a direct dependency on bitwarden-server-communication-config. Introduces the CookieProvider trait (abstracting cookies() and acquire_cookie()) and its implementation for ServerCommunicationConfigClient<R,P>. The ?Send async_trait variant was required because the underlying repository futures are not Send; the trait itself retains Send + Sync bounds keeping Arc<dyn CookieProvider> thread-safe. 36 tests pass including 3 new ones.
…eware PM-27126 requires a middleware that injects SSO load balancer cookies and retries on 302/307 redirects. Introduces ServerCommunicationConfigMiddleware implementing reqwest_middleware::Middleware: injects stored cookies as a Cookie header before forwarding, and on 302/307 calls acquire_cookie() (best-effort) then retries the original request with fresh cookies. Two prerequisite fixes are bundled: (1) repository.rs RPITIT bounds gain + Send so futures are Send on non-wasm targets; (2) cookie_provider.rs switches to platform-conditional async_trait (cfg_attr(not(wasm32))) matching the existing bitwarden-auth middleware pattern. 39 tests pass including 3 new ones for middleware clone and inject_cookies behaviour.
ServerCommunicationConfigMiddleware (commit 3) uses tracing::debug! to log 302/307 intercepts without exposing cookie values. The tracing workspace dependency was omitted from the initial Cargo.toml change and is added here as a follow-up.
PR #902 introduced ClientBuilder to fix constructor telescoping, but commit b6a23ee (pm-27126) added middleware injection via a private new_internal() function and a standalone constructor on Client, creating a rebase conflict. This resolution integrates middleware support into ClientBuilder: - ClientBuilder gains a `middleware` field and `with_middleware()` method - build() handles conditional redirect policy (disabled when middleware is present so outer middleware can observe raw 3xx responses) and WASM cfg-gating for the API HTTP client - new_with_token_handler_and_middleware() is preserved as public API (called by bitwarden-pm) but now delegates to ClientBuilder
PM-27126 requires a public entry point for callers to construct a PasswordManagerClient with SSO cookie middleware. Adds PasswordManagerClient::new_with_server_communication_config(): accepts Arc<dyn CookieProvider>, wraps it in ServerCommunicationConfigMiddleware, and passes it to Client::new_with_token_handler_and_middleware() (commit 5). Existing constructors are unchanged. bitwarden-server-communication-config and reqwest-middleware are added as workspace dependencies. The crate boundary invariant is preserved: bitwarden-core/Cargo.toml does not gain a dependency on bitwarden-server-communication-config.
Lockfile update for bitwarden-pm dependency additions committed in feat(bitwarden-pm): async-trait, bitwarden-server-communication-config, and reqwest-middleware added to workspace lock.
CI Check Style failed because nightly cargo fmt enforces grouped std
imports. Merges separate `use std::collections::HashMap` and
`use std::sync::Arc` into a single `use std::{collections::HashMap,
sync::Arc}` in the test module.
…middleware WASM targets require ?Send futures. The plain async_trait macro adds + Send, causing a type mismatch against the Middleware trait's WASM-compatible future signature. Replaces bare #[async_trait::async_trait] with platform-conditional cfg_attr pair on the impl Middleware block, matching the existing convention in bitwarden-auth token management middleware.
…rustdoc CI Check Style runs rustdoc with -D warnings, which treats unescaped angle brackets as unclosed HTML tags. Wraps `Arc<dyn CookieProvider>` in backticks in the cookie_provider module doc comment to suppress the lint.
… on acquisition failure Three gaps vs the clients implementation were identified review prep. Clients checks needsBootstrap$ before triggering SSO acquisition to avoid spurious flows on redirects unrelated to cookie bootstrapping. The SDK middleware was always acquiring on any 302/307 regardless. Adds needs_bootstrap(&self, hostname: &str) -> bool to CookieProvider trait with a blanket impl on ServerCommunicationConfigClient that delegates to the existing client.needs_bootstrap(). Middleware now skips acquisition and returns the redirect response when needs_bootstrap returns false. Silent acquisition failure: `let _ = acquire_cookie(...).await` produced zero log output on failure, making production SSO misconfigurations invisible. Replaced with `if let Err(e) = ...` + tracing::warn!. Updates three CookieProvider test mocks with the new method.
Concurrent requests that simultaneously receive a 302 during SSO bootstrap would each independently call `acquire_cookie`, which triggers a platform-specific SSO acquisition flow (WebView on mobile, browser window on desktop). Without deduplication, N concurrent requests could open N authentication windows. Adds an `in_flight` map (Mutex<HashMap<String, Arc<Notify>>>) to ServerCommunicationConfigMiddleware. When the first task for a hostname receives a 302, it inserts a Notify into the map and proceeds with acquisition. Subsequent tasks for the same hostname find the existing Notify, skip acquisition, and wait via notify.notified(). Once acquisition completes, the acquirer calls notify_waiters() and removes the entry; all waiters then proceed to retry with the freshly acquired cookies. Mirrors the `pendingAcquisition` deduplication pattern in the clients implementation.
CI nightly `cargo fmt` requires `use std::{collections::HashMap, sync::Arc};`
(grouped) under `imports_granularity = Crate`. The local stable toolchain
cannot apply this nightly-only setting, so the fix is applied manually to
keep CI green.
reqwest::redirect::Policy is unavailable on WASM and the browser auto-follows redirects, so the existing reactive 302/307 detection strategy cannot run on WASM targets. This left ServerCommunicationConfigMiddleware entirely inactive on WASM, blocking auth work that moves network requests into the SDK for WASM clients connecting to ALB-protected self-hosted servers. Add a #[cfg(target_arch = "wasm32")] proactive handle() branch that checks needs_bootstrap() before each request, acquires the SSO cookie if needed, injects it, then sends. The non-WASM reactive strategy is preserved exactly. The in_flight deduplication field and its usages in new() and Clone are cfg-gated to non-WASM only (single-threaded WASM has no concurrent acquisition races to deduplicate). Also correct a misleading comment in bitwarden-core client.rs that claimed additional_middleware is always empty on WASM.
Nightly rustfmt requires cfg-gated imports to precede unconditional imports within the same group. The `#[cfg(not(target_arch = "wasm32"))] use std::collections::HashMap` must appear before `use std::sync::Arc` to satisfy the formatter's ordering rules. No logic change — import ordering only.
Notified futures only register as waiters when first polled. On a multi-threaded tokio runtime, the acquirer could call notify_waiters() between the lock release and the first poll of .notified(), causing the waiter to block forever. Fix by cloning the Arc<Notify> and calling .enable() on the pinned Notified future while still holding the lock, so the waiter is registered before notify_waiters() can fire. The lock is then explicitly dropped before .await, preserving the existing non-blocking hold semantics. Identified by Claude's inline review comment on PR #872.
PR #902 introduced ClientBuilder to eliminate constructor telescoping and named this branch as an example of the problem. This commit completes the migration. Removes Client::new_with_token_handler_and_middleware() from bitwarden-core -- it duplicated what ClientBuilder already provides via with_token_handler() + with_middleware(). Migrates the sole caller, PasswordManagerClient::new_with_server_ communication_config() in bitwarden-pm, to use Client::builder() directly. No new constructors remain on this branch.
cargo +nightly fmt detected a trailing space after the period in a doc comment on the reqwest::Client sentence in middleware.rs. This is a pre-existing formatting issue with no functional impact.
Co-authored-by: claude[bot] <209825114+claude[bot]@users.noreply.github.com>
PR #902 introduced PasswordManagerClientBuilder to eliminate constructor telescoping. This commit migrates SSO cookie middleware support off the standalone new_with_server_communication_config() constructor and onto a new builder method, with_server_communication_config(), making it composable with other builder options. Removes the standalone constructor from lib.rs and adds with_server_communication_config() to builder.rs. The existing lib.rs test is updated to use the builder path; a parallel builder-level test is added.
|
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
PM-27126 — [SDK] Acquire & inject cookies from ServerCommunicationConfigClient into all requests
Sibling clients implementation
This PR was generated as part of a Claude Agent Teams test. Lots of supplementary docs can be found at this gist including:
The prompt provided was
To get this particular PR production ready after this initial implementation I did some comparative analysis between this implementation and the
clientsimplementation, correcting for things like de-duplication, how to handle wasm clients, and other later-in-the-log commits.I skipped the mentioned quality gates because this was a test I ran upwards of 14 times, so most of my tweaks to the upfront requirements landed in Jira and I knew generally what the questions and default answers would be.
The Claude Code configuration with agent definitions, skills, hooks, etc can all be read in my nix config
📔 Objective
Introduces a
CookieProvidertrait andServerCommunicationConfigMiddlewarethat acquires SSO load balancer cookies viaServerCommunicationConfigClientand injects them into every outbound request.On 302/307 redirect responses, the middleware checks
needs_bootstrap(), then acquires cookies best-effort with concurrent requests for the same hostname serialized viatokio::sync::Notify. The original request retries with fresh cookies injected explicitly, unlike the clients implementation, which relies on the browser's session to inject cookies automatically.Non-WASM: On 302/307 redirect responses, the middleware checks needs_bootstrap(), acquires cookies best-effort with concurrent requests serialized via tokio::sync::Notify, then retries with fresh cookies injected explicitly.
WASM: Browser redirects are not observable from the SDK, so the middleware uses a proactive strategy — checking needs_bootstrap() before each request, acquiring cookies if needed, and injecting them before sending. The in_flight deduplication field is cfg-gated to non-WASM only.
Wasm gets special treatment because we can't detect browser redirects in the sdk. The approach taken is to proactively check bootstrapping and apply cookies instead of waiting to be asked to.
A new
PasswordManagerClient::builder().with_middleware()accepts anArc<dyn CookieProvider>so callers can opt in.bitwarden-core
does not gain a dependency onbitwarden-server-communication-config. Cookie injection is threaded throughbitwarden-coreandbitwarden-pm` via trait objects.Design Decisions
-
clientsstores the in-flight acquisition as a Promise inpendingAcquisition; concurrent callers receive and await the samePromiseobject. The SDK can't share a singleFutureacross tasks the same way, so the middleware usestokio::sync::Notify:. The first task acquires and callsnotify_waiters(), waiters independently re-read cookies after waking. Functionally equivalent outcomes, structurally different.reqwesthas no equivalent session-level cookie store, so injection must happen at the middleware layer on both the initial request and the retry.🚨 Breaking Changes
None. All existing constructors are unchanged. The new constructor is additive.