Serialize MCP OAuth across Warp instances (APP-4959)#14239
Serialize MCP OAuth across Warp instances (APP-4959)#14239warp-dev-github-integration[bot] wants to merge 9 commits into
Conversation
Co-Authored-By: Oz <oz-agent@warp.dev>
When the same MCP installation is started by multiple Warp processes, exactly one process now owns the interactive OAuth attempt and opens one authorization page. A new cross-process coordinator in crates/mcp elects a leader via an fs4 OS file lease (crash-safe, cross-platform), serializes the shared secure-storage credential map (read/merge/write under a per-namespace lock), and exposes a WaitingForAuthentication state. Followers wait for the leader to publish credentials, reload them, and start the server without opening a page or binding a callback; a failed/timed-out leader is replaced by exactly one promoted successor. Desktop custom-scheme callbacks delivered to a follower are forwarded to the leader over a short-lived crates/ipc relay whose endpoint is published as owner-only metadata (no codes/tokens). - crates/mcp: new oauth::coordinator module (AuthLease, CoordinationPaths, OAuthCoordinator, SecureCredentialBackend, OwnerMetadata) and leader/follower integration in make_authenticated_client; credential persistence now goes through the coordinator's serialized merge. - app: ManagerCredentialBackend wires the coordinator to secure storage; DesktopOwnerRelay + UriHost::Mcp forwarding; WaitingForAuthentication state in the manager, TUI snapshot, and GUI server card. - tests: regression only_one_process_opens_mcp_oauth_authorization (N=3 -> 1 page), followers_wait_without_preparing_callbacks, leader exclusivity, promotion, credential merge/concurrency, malformed-map rejection, write-failure preservation, owner-only permissions, owner metadata round-trip + protocol-version rejection, forwarded-callback validation, and leftover-empty-lock-file reuse. 38 mcp tests pass. Co-Authored-By: Oz <oz-agent@warp.dev>
warp-dev-github-integration
Bot
changed the title
|
@warp-dev-github-integration[bot] I'm starting a first review of this pull request. You can view the conversation on Warp. I completed the review and no human review was requested for this pull request. Comment Powered by Oz |
![oz-for-oss[bot]](https://avatars.githubusercontent.com/in/3450139?s=60&v=4){kind=small}
There was a problem hiding this comment.
Overview
This PR adds cross-process coordination for MCP OAuth, including leader election, serialized credential persistence, desktop callback relay forwarding, and new waiting UI states.
Concerns
- The leader success path releases the auth lease before the queued credential write is durably visible, so a follower can promote and open a second OAuth page in the gap.
- Desktop relay startup/metadata failures are logged but not treated as fatal, leaving custom-scheme callbacks delivered to a follower with nowhere to go.
- The credential-store lock timeout is implemented around blocking lock calls, so contention can block the async runtime indefinitely instead of honoring the configured timeout.
- The deletion/logout path still uses the old whole-map write from the manager cache instead of the new coordinator merge/remove path, preserving a credential clobber race for removals or failure cleanup.
- This PR changes user-facing GUI/TUI waiting states, but the PR description says running UI proof was not captured; visual or TUI-render evidence is required before merge.
Verdict
Found: 0 critical, 5 important, 0 suggestions
Request changes
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
![warp-dev-github-integration[bot]](https://avatars.githubusercontent.com/in/2196476?s=60&v=4){kind=small}
There was a problem hiding this comment.
Overview
This PR adds cross-process MCP OAuth coordination (fs4 leader lease, leader/follower flow, serialized credential merge, desktop callback IPC relay, and a WaitingForAuthentication TUI/GUI state) so one process owns the interactive OAuth attempt. The crates/mcp coordinator design and its 38 coordinator/oauth tests are solid and map well to the spec's invariants. However, the app crate does not compile under -D warnings (CI is red), the leader releases its lease before credentials are durably published (reintroducing a racy second-page window), the credential-store lock can block the async runtime, the desktop relay fails open, the deletion path still does uncoordinated whole-map writes, and the user-facing waiting state has no visual or TUI-render proof. Request changes.
Concerns
- Build gate (blocking):
cargo clippy -p warp --all-targets --tests -- -D warningsfails in CI (Linux + MacOS) witherror: variant 'Forward' is never constructedatapp/src/ai/mcp/oauth_relay.rs:36. TheMcpCallbackRelayenum's singleForwardvariant is never constructed — it's only used as a type-parameter marker for the IPCServicetrait. The app crate therefore fails to compile under-D warnings, which violates spec criterion #17. Fix by makingMcpCallbackRelaya unit struct (or#[allow(dead_code)]on the variant) so the marker type compiles cleanly. This is the app-side wiring that the impl sandbox couldn't type-check (OOM); PR CI is the gate and it is red. - Lease released before credentials are durable (blocking correctness): In
run_leader_oauth,guard.release()runs immediately afterhandle_callbacksucceeds, but the credential publish is queued to a background task (PersistingCredentialStore::save→persist_tx→coordinator.merge_and_write). A follower polling inwait_for_credentials_or_promotionwon't see credentials yet but can acquire the now-free lease and open a second page — a racy form of the exact N-page bug this PR fixes, on the success path. The code comment even acknowledges this. Hold the lease (and relay/metadata) untilmerge_and_writecompletes, or synchronously publish credentials before releasing the guard. This breaks spec invariants #1 and #5 ("concurrent pages are never allowed"; a replacement page is allowed only after the prior owner failed/timed out). - Credential-store lock blocks the async runtime (blocking):
CredentialStoreLock::try_acquire_shared/try_acquire_exclusivecall fs4's blockinglock_shared/lock_exclusive, not the non-blockingtry_lock_*. Theacquire_cred_store_lockretry loop assumes contended locks return a retryable error, but the blocking calls don't returnWouldBlock— they block the async runtime thread until acquired, socred_store_lock_timeoutis never honored and the UI executor can stall under contention. Usetry_lock_shared/try_lock_exclusivewith the retry/sleep loop, or move the blocking lock intospawn_blocking. (The method names also saytry_acquire_*while using blocking locks — misleading.) - Desktop relay fails open (important): In
run_leader_oauth, relay startup failure and owner-metadata publish failure are bothlog::warn!-only; the leader then still callsrequires_authenticationand opens the desktop OAuth URL. A custom-scheme callback delivered to a follower then has no published endpoint to forward to, so the flow hangs. For the desktop path, fail beforerequires_authenticationwhen the relay cannot start or its metadata cannot be published (the TUI loopback path needs no relay). This breaks spec invariants #6/#7 for multi-instance desktop. - Deletion/removal path still does uncoordinated whole-map writes (important):
delete_credentials_from_secure_storagewrites the manager's entire in-memory credential map viawrite_to_secure_storagewith no credential-store lock and no read/merge/write. This is the same clobber race the PR fixes for initial/refresh writes, preserved on the removal path (called from both user-initiated delete and the spawn-failure cleanup at:1144): a concurrent leader's just-published entry for another installation can be overwritten by this process's stale whole-map write. Migrate removals to the coordinator's serialized remove-and-merge path so spec invariant #9 holds for deletions too. - Missing user-facing visual/TUI-render proof (blocking for a user-facing change): The PR adds a user-visible
WaitingForAuthenticationstate in both the TUI snapshot and the GUI server card, but includes no screenshots, screen recording, TUI transcript, orrender_to_lines/TuiBuffer::to_linessnapshot demonstrating the leader, follower-waiting, and post-auth states end to end. Spec criterion #15 (two real TUI instances) is explicitly unmet, and criterion #14 (TUI render fixtures for leader/waiting/promotion/success/timeout/error states) is also not addressed — no TUI render test for the new state exists incrates/warp_tuiorapp/src/tui, despite the repo's establishedto_linestest convention. Per the repo's review guidance, a TUI render/transcript is acceptable evidence here; please add at least ato_linessnapshot test for the waiting state and a recording/transcript of the two-instance leader/follower/running flow before merge.
Verdict
Found: 1 critical, 5 important, 0 suggestions
Request changes
Prior concerns still outstanding: lease-before-durable-write, blocking credential-store lock, desktop relay fail-open, deletion whole-map write, and missing visual/render proof (all from the prior review on this head, re-verified against the current diff — none addressed).
Review run
https://oz.staging.warp.dev/runs/019f951f-e4ce-739d-8582-083684d6e429
Address all six review findings on PR #14239: 1. CRITICAL clippy: make McpCallbackRelay a unit struct so the app crate compiles clean under `cargo clippy -p warp -- -D warnings` (the single- variant enum was dead code under -D warnings). 2. Hold the auth lease until credentials are durably published: install_persisting_credential_store now returns a CredentialPublish handle that resolves when the first serialized merge_and_write completes, and run_leader_oauth awaits it before guard.release(). A follower can no longer acquire the freed lease and open a second OAuth page on the success path (spec invariants #1/#5). 3. Honor cred_store_lock_timeout without blocking the async runtime: the credential-store lock now uses fs4 non-blocking try_lock_shared/ try_lock_exclusive (returning Ok(None) when contended) so the retry loop actually enforces the timeout; real I/O/permission errors fail closed. 4. Desktop relay fail-closed: for the custom-scheme path, relay startup or owner-metadata publish failure now returns an error before requires_authentication opens the page, so a callback delivered to a follower always has a published endpoint to forward to (spec invariants #6/#7). TUI loopback is unaffected (owner_relay = None). 5. Serialize credential deletion: delete_credentials_from_secure_storage now goes through the coordinator's new remove_and_write (read -> remove -> serialize -> secure write under the exclusive cred-store lock) so a concurrent leader's just-published entry for another installation is not clobbered by a whole-map write from a stale in-memory cache (spec invariant #9). 6. TUI render fixtures: add the missing WaitingForAuthentication arm to the MCP menu's refresh_rows match (it was non-exhaustive and did not render the waiting state), and add mcp_menu_tests.rs render-to-lines fixtures asserting the waiting state's exact stable copy, no authorization URL, and no ReopenAuthorization action, alongside leader and running states (spec criteria #14). Verified locally: cargo clippy -p mcp --all-targets -- -D warnings is clean and all 25 mcp oauth tests pass, including new remove_and_write_preserves_other_installations_in_the_shared_map and cred_store_lock_timeout_is_honored_under_contention regression tests. The warp app crate and warp_tui crates could not be compiled in this 3.8GB sandbox (rustc/clippy OOM during the app crate, the same wall the prior implementation hit); cargo clippy -p warp -- -D warnings and the warp_tui render fixtures are pending CI with adequate memory. Co-Authored-By: Oz <oz-agent@warp.dev>
There was a problem hiding this comment.
Overview
The rework addresses the six prior implementation findings in the coordinator, relay, credential-lock, deletion, and TUI-state paths, but the current head is not mergeable because the new TUI status and render fixture do not compile and the required live two-instance visual verification is absent.
Concerns
🚨 [CRITICAL] Required CI is red on the current head: Linux, macOS, and Windows formatting/clippy jobs fail because WaitingForAuthentication is not handled by crates/warp_tui/src/zero_state.rs, and the Linux/macOS test builds also report moved-value errors in the new fixture. A change that does not compile cannot be accepted.
🚨 [CRITICAL] This user-facing TUI change still has no spec-required running verification showing the leader's single authentication state, the follower's waiting state, and post-authentication convergence across two real TUI instances. The committed render-to-lines fixture is useful unit coverage but does not replace the acceptance criterion requiring the live two-instance proof.
Verdict
Found: 2 critical, 0 important, 0 suggestions
Prior concerns: all six prior findings are addressed in the current diff; the new blocking CI and visual-verification concerns remain.
Request changes
Review run
https://oz.staging.warp.dev/runs/019f95c6-b526-7215-a496-ac24e46941a9
Co-Authored-By: Oz <oz-agent@warp.dev>
|
Rework #2 status (commit
I could not capture the required live two-TUI leader/follower/post-auth proof because the app/TUI build never completed, so no real TUI instances could be launched here. The committed |
There was a problem hiding this comment.
Overview
Rework #2 (commit fff3117) fixes the two prior compile blockers — crates/warp_tui/src/zero_state.rs now handles WaitingForAuthentication exhaustively and the mcp_menu_tests fixture returns its rendered values from inside the App::test closure — and CI confirms it: Linux/MacOS/Windows clippy and all three release-compile jobs are green, and Linux tests pass. However, the MacOS and Windows test suites now fail: eight mcp::oauth::coordinator::tests panic because the coordinator's path-canonicalization security check rejects the platform's default temp directory, so the required CI gate is red.
Concerns
🚨 [CRITICAL] Required CI is red on MacOS and Windows: Run MacOS tests and Run Windows tests fail (Linux passes). Eight coordinator tests panic at CoordinationPaths::ensure with "MCP OAuth coordination directory ... resolves to ...; refusing to follow a redirected path." On macOS, TempDir lives under $TMPDIR (/var/folders/...), which fs::canonicalize resolves through the /var → /private/var symlink; on Windows, %TEMP% uses 8.3 short names (RUNNER~1) that canonicalize to long names with a \\?\ verbatim prefix. In both cases canonical != self.dir even though no malicious symlink is involved, so the check rejects a legitimate path. Linux passes only because /tmp has no indirection. The security intent (reject attacker-controlled symlinks) is right, but the implementation must not bail merely because the platform's canonical form differs from the input form — canonicalize the coordination root once and operate on the canonical path, or reject only when the canonical path escapes a trusted per-user root.
coordination_root_dir() falls back to HOME and then to "." when HOME is unset. HOME is not set by default on Windows, so in production the coordination directory would resolve to a CWD-relative ./.warp/mcp-oauth, which both re-trips the ensure() canonicalization check (relative ≠ absolute canonical) and breaks cross-process coordination on Windows — each process with a different working directory would use a different coordination directory, defeating the feature. Use USERPROFILE/LOCALAPPDATA (or the dirs crate) on Windows rather than HOME.
The live two-TUI-instance visual proof (spec criterion #15) also remains outstanding (environment-blocked: the app crate OOMs in the cloud sandbox); the committed TuiBuffer::to_lines render fixtures cover criterion #14 deterministically, which the repo's TUI review guidance accepts as visual evidence for headless TUI changes.
Verdict
Found: 1 critical, 1 important, 0 suggestions
Request changes
Prior concerns: the six original findings and the two cycle-1-rework compile blockers are resolved; new this cycle are the blocking MacOS/Windows CI test failure and the HOME fallback, and the live two-TUI visual proof (#15) remains outstanding.
Review run
https://oz.staging.warp.dev/runs/019f9626-727e-7119-95b4-0406fab081eb
Normalize platform-specific temp paths once, enforce trusted-root containment, and use a Windows user-local fallback instead of the working directory. Add a symlinked-root regression test.\n\nCo-Authored-By: Oz <oz-agent@warp.dev>
Keep the non-Windows HOME fallback inside its cfg block so Windows builds do not type-check an unavailable variable.\n\nCo-Authored-By: Oz <oz-agent@warp.dev>
Return the cfg-selected coordination root expression directly so Windows clippy accepts the platform-specific fallback.\n\nCo-Authored-By: Oz <oz-agent@warp.dev>
|
Rework #3 complete on d768a33. Cross-platform coordinator paths now canonicalize the per-user root once, store canonical child paths, reject only trusted-root escapes, and use Windows LOCALAPPDATA/USERPROFILE fallbacks instead of CWD-relative HOME/ |
There was a problem hiding this comment.
Overview
Rework #3 (head d768a33) resolves the final cross-platform coordination failures: the per-user coordination root is canonicalized once with a component-aware trusted-root containment check that tolerates macOS /private and Windows short-name/verbatim-prefix indirection, a Windows LOCALAPPDATA → USERPROFILE → HOMEDRIVE+HOMEPATH → per-user temp fallback that never resolves to a CWD-relative path, and a unix symlink regression test. All six cycle-1 findings (build/clippy dead-code, lease-before-durable, blocking credential-store lock, desktop relay fail-open, deletion whole-map write, missing TUI render proof) and both cycle-2 compile blockers remain resolved at this head, re-verified against the current diff. CI is green on Linux/macOS/Windows tests, clippy, and compilation; the 41-test mcp suite covers leader election, follower wait, promotion, serialized credential merge/remove, malformed-map rejection, owner-only permissions, leftover-lock reuse, lock-timeout, and symlink canonicalization. The acceptable-form TUI visual evidence (spec #14 TuiBuffer::to_lines fixtures for waiting/leader/running) is committed and matches the repo's TUI evidence convention.
Verdict
Found: 0 critical, 0 important
Approve
Prior concerns: none remain — all cycle 1-3 blocking findings are resolved at head d768a33. The live two-TUI running proof (spec #15) stays outstanding as an environment-blocked non-blocking follow-up (the warp app crate is OOM-killed in the 3.8 GB sandbox, so neither the implementation nor this review environment can build/run two TUI instances); capture it from a higher-memory desktop or cloud runner as a final confirmation before or after merge. Minor non-blocking notes: the PR description's "Rework #3" section reads as a rework chronicle and could fold into the Fix section, and spec #14's timeout/coordination-error render states fold into the existing Failed status without dedicated fixtures.
Review run
https://oz.staging.warp.dev/runs/019f9686-0e62-76a3-a892-28bf05750c69
1 participant
Summary
Fixes APP-4959: when the same MCP installation is started by multiple Warp processes, exactly one process owns the interactive OAuth attempt and opens one authorization page. Every other process waits for the result from shared secure storage, then continues using the credentials without opening another page. Applies to TUI loopback callbacks and desktop custom-URL-scheme callbacks on every native platform.
Root cause
The
TemplatableMCPServerManagersingleton and per-process OAuth callback channels stop at a process boundary, while the user-visible authorization page and the shared secure-credential map are shared by all instances. With no cross-process coordination, every concurrent process calledctx.open_urland bound its own loopback receiver, opening N pages for N instances.Fix
A per-
(channel, installation, namespace)cross-process coordinator incrates/mcpuses anfs4OS file lease for crash-safe leader election and serialized credential read/merge/write. Non-leaders enterWaitingForAuthenticationwith no URL; the leader alone prepares the callback receiver, emitsAuthenticationRequired, and opens the single page. Desktop callbacks received by a follower are forwarded to the leader over the IPC relay, and credential writes (including deletion) are serialized so concurrent installations do not clobber one another.Rework #3 — cross-platform coordination paths
/var→/private/varand Windows short-name/verbatim-prefix normalization do not reject legitimate temporary/runtime directories.LOCALAPPDATA, thenUSERPROFILE/HOMEDRIVE+HOMEPATH, with a per-user temp directory as the final fallback; production never falls back to a CWD-relative./.warp/mcp-oauthpath.Verification
cargo nextest run --manifest-path /workspace/warp/Cargo.toml -p mcp— 41 tests passed, includingoauth::coordinator::tests::symlinked_coordination_root_is_canonicalized_without_escapeand all coordinator contention/credential tests.cargo clippy --manifest-path /workspace/warp/Cargo.toml -p mcp --all-targets -- -D warnings— clean.cargo fmt --manifest-path /workspace/warp/Cargo.toml --all -- --checkandgit diff --check— clean.Check CI resultspassed./workspace/warp/script/presubmit— formatting and inline-module checks passed; the full app clippy stage was SIGKILL/OOM-limited by the 3.8 GB sandbox, while the focused mcp clippy/test gates passed.CHANGELOG-BUG-FIX: MCP server OAuth now opens a single authorization page across multiple running Warp instances instead of one page per process.
Originating thread: https://warpdev.slack.com/archives/C0BDQDW8V5E/p1784905776293049?thread_ts=1784905776.293049&cid=C0BDQDW8V5E
Conversation: https://staging.warp.dev/conversation/cb364aa8-db67-4dd7-ba58-50827a5316d4
Run: https://oz.staging.warp.dev/runs/019f963a-e302-70d0-9c2c-ada9dcef6d39
This PR was generated with Oz.