fix(mcp): persist token_received_at so OAuth refresh fires before expiry (#8863)#9460
Conversation
`PersistedCredentials` did not record when its token was received, and `install_persisting_credential_store` hardcoded `token_received_at: None` when seeding the rmcp credential store from cached credentials. The result: rmcp's pre-emptive refresh check at `AuthorizationManager::get_access_token` requires both `expires_in` and `token_received_at`, so it was skipped for the lifetime of every OAuth-authenticated session that originated from a fresh login. After the user's first authorization, the cached access token was used past its TTL until the next request returned 401, at which point rmcp's `handle_response` returns `AuthorizationRequired` without attempting recovery — the user sees their MCP server "disconnect" and is prompted to re-authenticate. This matches the report in warpdotdev#8863. Fix: - Add `token_received_at: Option<u64>` to `PersistedCredentials` (with `#[serde(default)]` for backward compat — credentials persisted by older Warp versions deserialize cleanly with `None`, and the next refresh repopulates it). - Forward `token_received_at` from `StoredCredentials` through `PersistingCredentialStore::save` to the persist channel. - Thread `token_received_at` as a parameter into `install_persisting_credential_store` and seed the inner store with it instead of hardcoded `None`. - In the cached-creds path, pass the value loaded from `PersistedCredentials`. In the fresh-OAuth path, stamp it to the current Unix time at the same point we save the credentials. Adds six unit tests covering serde round-trip, legacy-format deserialization (regression-protects existing users on upgrade), the save-forwards-received_at fix, the absent-token-response branch, the refresh-token carry-forward + received_at interaction, and the `now_epoch_secs` helper. Fixes warpdotdev#8863 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
/oz-review |
|
I'm re-reviewing this pull request in response to a review request. I reviewed this pull request and requested human review from: @Legoben. I left feedback as a comment so a maintainer can approve. Comment I'm starting a first review of this pull request. I reviewed this pull request and requested human review from: @Legoben. I left feedback as a comment so a maintainer can approve. Comment I completed the review and posted feedback on this pull request. Comment I reviewed this pull request and requested human review from: @Legoben. Comment You can view the conversation on Warp. I reviewed this pull request and requested human review from: @Legoben. Comment I reviewed this pull request and requested human review from: @Legoben. Comment I reviewed this pull request and requested human review from: @Legoben. 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 persists token_received_at for MCP OAuth credentials, forwards it through the rmcp credential-store save path, and seeds the in-memory store with the timestamp so pre-emptive token refresh can run before expiry. It also preserves legacy deserialization behavior for existing stored credentials and adds targeted regression tests.
Concerns
- No blocking correctness, security, or error-handling concerns found in the changed diff.
Verdict
Found: 0 critical, 0 important, 0 suggestions
Approve
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
There was a problem hiding this comment.
Overview
This PR persists token_received_at for MCP OAuth credentials, forwards it through the rmcp credential-store save path, and seeds the in-memory store with the timestamp so pre-emptive token refresh can run before expiry. It also preserves legacy deserialization behavior for existing stored credentials and adds targeted regression tests.
Concerns
- No blocking correctness, security, or error-handling concerns found in the changed diff.
Verdict
Found: 0 critical, 0 important, 0 suggestions
Approve
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
…h-token-received-at
There was a problem hiding this comment.
Overview
This PR persists token_received_at for MCP OAuth credentials, forwards it through the rmcp credential-store save path, and seeds the in-memory store with the timestamp so rmcp can perform pre-emptive refresh before access-token expiry.
Concerns
- No blocking correctness or security concerns found in the inlined diff.
Verdict
Found: 0 critical, 0 important, 0 suggestions
Approve
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
|
I'd rather refresh before expiration instead of after a failed call, let me know if this should be adjusted |
vorporeal
requested review from
peicodes
and removed request for
Legoben and
vorporeal
|
hey @peicodes - can you take a look at this? |
peicodes
left a comment
peicodes
left a comment
There was a problem hiding this comment.
This is a great fix, thank you!
The part about the pre-emptive refresh_token request being now redundant is accurate, but yes, OOS
captainsafia
added
the
external-contributor
Warp Dev Github Integration
CI's `cargo fmt --check` flagged two single-line asserts in the test module that exceed the project's max line width. Splitting them across multiple lines per rustfmt's preferred form. No behavior change. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
/oz-review |
There was a problem hiding this comment.
Overview
This PR persists and restores token_received_at for MCP OAuth credentials so rmcp can perform pre-emptive token refresh before access tokens expire. It updates both cached-credential and fresh-OAuth paths, forwards the timestamp through the persisting credential store, and adds regression coverage for legacy deserialization and save propagation.
Concerns
- No blocking correctness or security concerns found in the changed lines. The supplemental security pass did not identify new credential exposure or token-handling regressions.
Verdict
Found: 0 critical, 0 important, 0 suggestions
Approve
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
There was a problem hiding this comment.
Overview
This PR persists token_received_at for MCP OAuth credentials and threads it through the persisting credential store so rmcp can perform pre-emptive access-token refreshes before expiry. The security pass found no additional issues in the changed code.
Concerns
- The added test module appears to call
OAuthTokenResponse::refresh_token()without importing theoauth2::TokenResponsetrait in that module, which will prevent the new tests from compiling.
Verdict
Found: 0 critical, 1 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
There was a problem hiding this comment.
Overview
This PR persists and restores token_received_at for MCP OAuth credentials so rmcp can perform pre-emptive token refresh before access tokens expire. It updates both cached-credential and fresh-OAuth paths, forwards the timestamp through the persisting credential store, and adds regression coverage for legacy deserialization and save propagation.
Concerns
- No blocking correctness or security concerns found in the changed lines. The supplemental security pass did not identify new credential exposure or token-handling regressions.
Verdict
Found: 0 critical, 0 important, 0 suggestions
Approve
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
The test at `save_carries_forward_refresh_token_and_preserves_received_at` calls `.refresh_token()` on `OAuthTokenResponse`, which is exclusively a trait method on `oauth2::TokenResponse` (no inherent method by that name). The parent module's anonymous import (`use oauth2::TokenResponse as _;`) does not propagate into child modules, even via `use super::*;` — the glob does not pick up anonymous imports. Add the explicit import in the test module so trait method resolution works robustly, regardless of any quirks in the previous CI run that may have masked this. Per @oz-for-oss review on warpdotdev#9460. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
/oz-review |
There was a problem hiding this comment.
Overview
This PR persists token_received_at for MCP OAuth credentials, threads it through the rmcp credential-store seed path, and adds unit coverage for serde compatibility, persistence forwarding, refresh-token carry-forward, and timestamp generation.
Concerns
- No blocking correctness, error-handling, or security concerns were found in the changed lines. A repository-local
security-review-prskill file was not present in the workspace, so a manual supplemental security pass was applied.
Verdict
Found: 0 critical, 0 important, 0 suggestions
Approve
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
There was a problem hiding this comment.
Overview
This PR persists token_received_at alongside MCP OAuth credentials, forwards the value through the rmcp credential-store save path, and seeds rmcp's in-memory store with the persisted timestamp so pre-emptive refresh can run before token expiry.
Concerns
- No blocking correctness or security concerns found in the changed lines.
Verdict
Found: 0 critical, 0 important, 0 suggestions
Approve
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
…h-token-received-at
oz-for-oss
Bot
dismissed
their stale review
Oz no longer requests changes for this pull request after the latest automated review.
There was a problem hiding this comment.
Overview
This PR persists token_received_at with MCP OAuth credentials, seeds rmcp's credential store with that timestamp, and adds regression coverage for legacy credentials, refresh-token carry-forward, and timestamp propagation.
Concerns
- No blocking correctness concerns found in the inlined diff.
- No security findings from the supplemental security pass.
Verdict
Found: 0 critical, 0 important, 0 suggestions
Approve
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
There was a problem hiding this comment.
Overview
This PR persists token_received_at with MCP OAuth credentials, seeds rmcp's credential store with that timestamp, and adds regression coverage for legacy credentials, refresh-token carry-forward, and timestamp propagation.
Concerns
- No blocking correctness concerns found in the inlined diff.
- No security findings from the supplemental security pass.
Verdict
Found: 0 critical, 0 important, 0 suggestions
Approve
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
Earlier review feedback claimed `use oauth2::TokenResponse as _;` was
needed in the test module so `.refresh_token()` would resolve. Verified
locally: the parent module's anonymous `use oauth2::{RefreshToken,
TokenResponse as _};` already brings the trait into scope for method
resolution in `mod tests` via Rust's parent-scope lookup, so the
explicit import in the test module is redundant and triggers the
`-D warnings` lint as `unused_imports`.
Removing the duplicate; the 7 OAuth tests still pass locally:
cargo nextest run -p warp -E 'test(oauth::)'
→ 7/7 pass
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…h-token-received-at
|
This looks good! Let's stop making changes to it - I'll wait for CI to pass and merge. |
Sorry about that, I wasnt sure how the keeping up to master flow was handled, thought it was my responsibility |
…iry (warpdotdev#8863) (warpdotdev#9460) ## Description Fixes warpdotdev#8863. `PersistedCredentials` did not record when its token was received, and `install_persisting_credential_store` hardcoded `token_received_at: None` when seeding the rmcp credential store. The result: rmcp's pre-emptive refresh check at [`AuthorizationManager::get_access_token`](https://github.com/warpdotdev/rmcp/blob/c0f65dc/crates/rmcp/src/transport/auth.rs#L617-L633) — which requires both `expires_in` and `token_received_at` — was skipped for the lifetime of every OAuth-authenticated session. After the first authorization, the cached access token was used past its TTL. When a request finally hit a 401, rmcp's [`handle_response`](https://github.com/warpdotdev/rmcp/blob/c0f65dc/crates/rmcp/src/transport/auth.rs#L710-L720) returns `AuthorizationRequired` without attempting recovery — the user sees their MCP server disconnect and is forced to re-authenticate. That matches the reproducer in warpdotdev#8863 exactly: "after logging in the first time if you wait 1hr… the MCP server disconnects and requires you to login again." The cached-credentials path partially masked the bug because of the unconditional `auth_manager.refresh_token()` workaround at [`oauth.rs:272`](app/src/ai/mcp/templatable_manager/oauth.rs#L272) — that call writes back fresh credentials with `received_at` properly set, papering over the missing seed value for the duration of that session. The fresh-OAuth path has no such fallback, so users hit the bug on their first session. ### Fix 1. Add `token_received_at: Option<u64>` to `PersistedCredentials`. `#[serde(default)]` keeps existing on-disk credentials deserializable — they come back with `None` and the next refresh populates the field. 2. Forward `token_received_at` from `StoredCredentials` through `PersistingCredentialStore::save` to the persist channel so the value reaches secure storage. 3. Thread `token_received_at` as a parameter into `install_persisting_credential_store` and seed the inner store with it instead of hardcoded `None`. 4. Cached-creds call site (`make_authenticated_client`): pass the value loaded from `PersistedCredentials`. 5. Fresh-OAuth call site: stamp `token_received_at` to `now_epoch_secs()` at the point we save the credentials, and pass the same value into the seed. Spec reference: [RFC 6749 §5.1 `expires_in`](https://datatracker.ietf.org/doc/html/rfc6749#section-5.1) and [§6 refresh-token semantics](https://datatracker.ietf.org/doc/html/rfc6749#section-6); rmcp's pre-emptive refresh implements the recommended client behavior of refreshing before expiry to avoid in-flight 401s. ### Things deliberately left alone - The unconditional connect-time `auth_manager.refresh_token()` call at [oauth.rs:272](app/src/ai/mcp/templatable_manager/oauth.rs#L272) and the stale comment above it claiming rmcp's fork lacks expiry detection. The fork at the pinned revision (c0f65dc, "Fixes for oauth expiry and scopes") *does* now have proper detection per upstream [rust-sdk#680](modelcontextprotocol/rust-sdk#680), so the workaround is now redundant. Removing it is a behavior change worth its own PR — out of scope here. - 401-on-active-request retry. rmcp's `handle_response` still bubbles up `AuthorizationRequired` rather than refresh-and-retry; pre-emptive refresh covers the common case but not surprise expiries (clock skew, server-side revocation). A reactive layer is a follow-up. ## Testing Six new unit tests in `app/src/ai/mcp/templatable_manager/oauth.rs`: - `persisted_credentials_round_trip_through_serde_preserves_received_at` — value preservation. - `persisted_credentials_deserializes_legacy_format_without_received_at` — **regression-protects existing users on upgrade**: credentials persisted by older Warp must still deserialize, with `received_at` defaulting to `None`. Failing this test would mean existing users lose their MCP OAuth tokens. - `save_forwards_token_received_at_to_persist_channel` — the core regression test for the fix. - `save_forwards_none_when_received_at_is_none` — defensive: don't substitute a fake value if rmcp passes `None`. - `save_skips_persist_when_token_response_absent` — no regression of the existing `if let Some(token_response)` branch. - `save_carries_forward_refresh_token_and_preserves_received_at` — combined check that the existing refresh-token carry-forward (RFC 6749 §6) and the new `received_at` propagation don't interfere. - `now_epoch_secs_returns_recent_unix_time` — sanity check on the helper. I was not able to run `./script/presubmit` locally (`warpui` build needs `xcrun metal` which requires full Xcode.app, only CommandLineTools is installed here). Trusting CI for `cargo clippy --workspace --all-targets --tests -- -D warnings` and `cargo nextest run --workspace`. ## Server API dependencies N/A — client-only change. - [x] Is this change necessary to make the client compatible with a desired [server API breaking change](https://www.notion.so/warpdev/How-to-safely-introduce-server-API-breaking-changes-0aa805ff5d5d41fd8834f3c95caba0b4): **No** ## Agent Mode - [ ] Warp Agent Mode - This PR was created via Warp's AI Agent Mode ## Changelog Entries for Stable CHANGELOG-BUG-FIX: Fix MCP OAuth servers disconnecting after access-token TTL expires; refresh tokens are now used to obtain new access tokens before expiry (warpdotdev#8863). --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
4 participants
Description
Fixes #8863.
PersistedCredentialsdid not record when its token was received, andinstall_persisting_credential_storehardcodedtoken_received_at: Nonewhen seeding the rmcp credential store. The result: rmcp's pre-emptive refresh check atAuthorizationManager::get_access_token— which requires bothexpires_inandtoken_received_at— was skipped for the lifetime of every OAuth-authenticated session.After the first authorization, the cached access token was used past its TTL. When a request finally hit a 401, rmcp's
handle_responsereturnsAuthorizationRequiredwithout attempting recovery — the user sees their MCP server disconnect and is forced to re-authenticate. That matches the reproducer in #8863 exactly: "after logging in the first time if you wait 1hr… the MCP server disconnects and requires you to login again."The cached-credentials path partially masked the bug because of the unconditional
auth_manager.refresh_token()workaround atoauth.rs:272— that call writes back fresh credentials withreceived_atproperly set, papering over the missing seed value for the duration of that session. The fresh-OAuth path has no such fallback, so users hit the bug on their first session.Fix
token_received_at: Option<u64>toPersistedCredentials.#[serde(default)]keeps existing on-disk credentials deserializable — they come back withNoneand the next refresh populates the field.token_received_atfromStoredCredentialsthroughPersistingCredentialStore::saveto the persist channel so the value reaches secure storage.token_received_atas a parameter intoinstall_persisting_credential_storeand seed the inner store with it instead of hardcodedNone.make_authenticated_client): pass the value loaded fromPersistedCredentials.token_received_attonow_epoch_secs()at the point we save the credentials, and pass the same value into the seed.Spec reference: RFC 6749 §5.1
expires_inand §6 refresh-token semantics; rmcp's pre-emptive refresh implements the recommended client behavior of refreshing before expiry to avoid in-flight 401s.Things deliberately left alone
auth_manager.refresh_token()call at oauth.rs:272 and the stale comment above it claiming rmcp's fork lacks expiry detection. The fork at the pinned revision (c0f65dc, "Fixes for oauth expiry and scopes") does now have proper detection per upstream rust-sdk#680, so the workaround is now redundant. Removing it is a behavior change worth its own PR — out of scope here.handle_responsestill bubbles upAuthorizationRequiredrather than refresh-and-retry; pre-emptive refresh covers the common case but not surprise expiries (clock skew, server-side revocation). A reactive layer is a follow-up.Testing
Six new unit tests in
app/src/ai/mcp/templatable_manager/oauth.rs:persisted_credentials_round_trip_through_serde_preserves_received_at— value preservation.persisted_credentials_deserializes_legacy_format_without_received_at— regression-protects existing users on upgrade: credentials persisted by older Warp must still deserialize, withreceived_atdefaulting toNone. Failing this test would mean existing users lose their MCP OAuth tokens.save_forwards_token_received_at_to_persist_channel— the core regression test for the fix.save_forwards_none_when_received_at_is_none— defensive: don't substitute a fake value if rmcp passesNone.save_skips_persist_when_token_response_absent— no regression of the existingif let Some(token_response)branch.save_carries_forward_refresh_token_and_preserves_received_at— combined check that the existing refresh-token carry-forward (RFC 6749 §6) and the newreceived_atpropagation don't interfere.now_epoch_secs_returns_recent_unix_time— sanity check on the helper.I was not able to run
./script/presubmitlocally (warpuibuild needsxcrun metalwhich requires full Xcode.app, only CommandLineTools is installed here). Trusting CI forcargo clippy --workspace --all-targets --tests -- -D warningsandcargo nextest run --workspace.Server API dependencies
N/A — client-only change.
Agent Mode
Changelog Entries for Stable
CHANGELOG-BUG-FIX: Fix MCP OAuth servers disconnecting after access-token TTL expires; refresh tokens are now used to obtain new access tokens before expiry (#8863).