Skip to content

Commit edfd414

Browse files
fix(mcp): persist token_received_at so OAuth refresh fires before expiry (#8863) (#9460)
## Description Fixes #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 #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 (#8863). --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent 6289aec commit edfd414

1 file changed

Lines changed: 262 additions & 4 deletions

File tree

  • app/src/ai/mcp/templatable_manager

app/src/ai/mcp/templatable_manager/oauth.rs

Lines changed: 262 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
use std::collections::HashMap;
2+
use std::time::{SystemTime, UNIX_EPOCH};
23

34
use anyhow::{anyhow, bail};
45
use oauth2::{RefreshToken, TokenResponse as _};
@@ -41,6 +42,29 @@ pub struct PersistedCredentials {
4142
client_id: String,
4243
client_secret: Option<String>,
4344
token_response: OAuthTokenResponse,
45+
/// Unix timestamp (seconds) when this token was received from the OAuth server.
46+
///
47+
/// Required for rmcp's pre-emptive refresh: it computes remaining lifetime as
48+
/// `expires_in - (now - token_received_at)` and refreshes when below the buffer.
49+
/// Without this value the check is skipped, the cached token stays in use past
50+
/// its TTL, and the next request fails with 401 (see #8863).
51+
///
52+
/// `None` for credentials persisted by older Warp versions; the next refresh
53+
/// will populate it via the credential-store save path.
54+
#[serde(default)]
55+
token_received_at: Option<u64>,
56+
}
57+
58+
/// Returns the current Unix timestamp in seconds.
59+
///
60+
/// Mirrors rmcp's internal `now_epoch_secs` (which is private) so we can stamp
61+
/// `token_received_at` consistently with the timestamps rmcp itself emits during
62+
/// refresh.
63+
fn now_epoch_secs() -> u64 {
64+
SystemTime::now()
65+
.duration_since(UNIX_EPOCH)
66+
.unwrap_or_default()
67+
.as_secs()
4468
}
4569

4670
/// Maps cloud MCP installation UUID to its OAuth credentials in secure storage.
@@ -108,13 +132,17 @@ impl CredentialStore for PersistingCredentialStore {
108132
self.apply_refresh_token_carry_forward(&mut credentials)
109133
.await;
110134

135+
// Capture before `credentials` is consumed below.
136+
let token_received_at = credentials.token_received_at;
137+
111138
self.inner.save(credentials.clone()).await?;
112139

113140
if let Some(token_response) = credentials.token_response {
114141
let _ = self.persist_tx.try_send(PersistedCredentials {
115142
client_id: credentials.client_id,
116143
client_secret: self.client_secret.clone(),
117144
token_response,
145+
token_received_at,
118146
});
119147
}
120148
Ok(())
@@ -141,6 +169,7 @@ async fn install_persisting_credential_store(
141169
client_secret: Option<String>,
142170
spawner: ModelSpawner<TemplatableMCPServerManager>,
143171
installation_uuid: Uuid,
172+
token_received_at: Option<u64>,
144173
) {
145174
let (persist_tx, persist_rx) = async_channel::unbounded();
146175
let store = PersistingCredentialStore {
@@ -150,15 +179,19 @@ async fn install_persisting_credential_store(
150179
};
151180

152181
// Seed the new store with the current credentials so that subsequent
153-
// get_access_token() calls can find them.
182+
// get_access_token() calls can find them. `token_received_at` must be
183+
// preserved across the seed; otherwise rmcp's pre-emptive refresh check
184+
// (which requires both `expires_in` and `token_received_at`) is skipped
185+
// for the lifetime of this store, and the cached token will be used past
186+
// its TTL until a request fails with 401 (#8863).
154187
if let Ok((client_id, Some(token_response))) = auth_manager.get_credentials().await {
155188
let _ = store
156189
.inner
157190
.save(StoredCredentials {
158191
client_id,
159192
token_response: Some(token_response),
160193
granted_scopes: Vec::new(),
161-
token_received_at: None,
194+
token_received_at,
162195
})
163196
.await;
164197
}
@@ -233,6 +266,7 @@ pub async fn make_authenticated_client(
233266
let client_secret = credentials
234267
.client_secret
235268
.or_else(|| provider.as_ref().map(|p| p.client_secret.to_string()));
269+
let cached_token_received_at = credentials.token_received_at;
236270
oauth_state
237271
.set_credentials(&credentials.client_id, credentials.token_response)
238272
.await?;
@@ -262,11 +296,15 @@ pub async fn make_authenticated_client(
262296
//
263297
// Install the persisting credential store before refreshing so that
264298
// the refresh result is automatically written back to secure storage.
299+
// Pass the cached `token_received_at` so rmcp's pre-emptive refresh
300+
// check has the data it needs after this connect-time refresh — see
301+
// #8863 for what happens when this is `None`.
265302
install_persisting_credential_store(
266303
&mut auth_manager,
267304
client_secret,
268305
spawner.clone(),
269306
uuid,
307+
cached_token_received_at,
270308
)
271309
.await;
272310
match auth_manager.refresh_token().await {
@@ -407,13 +445,18 @@ pub async fn make_authenticated_client(
407445
// Handle the callback with the received authorization code and CSRF token.
408446
oauth_state.handle_callback(code, csrf_token).await?;
409447

410-
// Save the credentials to secure storage.
448+
// Save the credentials to secure storage. Stamp `token_received_at` so the
449+
// pre-emptive refresh check in rmcp has a timestamp to compute remaining
450+
// lifetime against on subsequent connects (without it, the cached token
451+
// would be used past its TTL — see #8863).
452+
let token_received_at = now_epoch_secs();
411453
let (client_id, token_response) = oauth_state.get_credentials().await?;
412454
if let Some(token_response) = token_response {
413455
let credentials = PersistedCredentials {
414456
client_id,
415457
client_secret: client_secret.clone(),
416458
token_response,
459+
token_received_at: Some(token_received_at),
417460
};
418461
spawner
419462
.spawn(move |manager, ctx| {
@@ -427,7 +470,14 @@ pub async fn make_authenticated_client(
427470
AuthError::InternalError("Failed to create authorization manager".to_string())
428471
})?;
429472

430-
install_persisting_credential_store(&mut am, client_secret, spawner, uuid).await;
473+
install_persisting_credential_store(
474+
&mut am,
475+
client_secret,
476+
spawner,
477+
uuid,
478+
Some(token_received_at),
479+
)
480+
.await;
431481

432482
Ok((AuthClient::new(reqwest::Client::new(), am), true))
433483
}
@@ -569,3 +619,211 @@ pub(crate) fn write_to_secure_storage<T: Serialize>(
569619
}
570620
}
571621
}
622+
623+
#[cfg(test)]
624+
mod tests {
625+
use super::*;
626+
use rmcp::transport::auth::OAuthTokenResponse;
627+
628+
/// Builds a minimal `OAuthTokenResponse` for tests, optionally with a refresh token.
629+
fn make_test_token_response(refresh_token: Option<&str>) -> OAuthTokenResponse {
630+
let mut json = serde_json::json!({
631+
"access_token": "test_access_token",
632+
"token_type": "bearer",
633+
"expires_in": 3600,
634+
});
635+
if let Some(rt) = refresh_token {
636+
json["refresh_token"] = serde_json::Value::String(rt.to_string());
637+
}
638+
serde_json::from_value(json).expect("OAuthTokenResponse deserialization")
639+
}
640+
641+
/// Constructs a fresh `PersistingCredentialStore` plus the receiver side of its
642+
/// persist channel so tests can observe what would be written to secure storage.
643+
fn make_test_store(
644+
client_secret: Option<String>,
645+
) -> (
646+
PersistingCredentialStore,
647+
async_channel::Receiver<PersistedCredentials>,
648+
) {
649+
let (tx, rx) = async_channel::unbounded();
650+
let store = PersistingCredentialStore {
651+
inner: InMemoryCredentialStore::new(),
652+
client_secret,
653+
persist_tx: tx,
654+
};
655+
(store, rx)
656+
}
657+
658+
#[test]
659+
fn persisted_credentials_round_trip_through_serde_preserves_received_at() {
660+
let original = PersistedCredentials {
661+
client_id: "client-abc".to_string(),
662+
client_secret: Some("shh".to_string()),
663+
token_response: make_test_token_response(Some("refresh-1")),
664+
token_received_at: Some(1_700_000_000),
665+
};
666+
667+
let json = serde_json::to_string(&original).expect("serialize");
668+
let parsed: PersistedCredentials = serde_json::from_str(&json).expect("deserialize");
669+
670+
assert_eq!(parsed.client_id, original.client_id);
671+
assert_eq!(parsed.client_secret, original.client_secret);
672+
assert_eq!(parsed.token_received_at, Some(1_700_000_000));
673+
}
674+
675+
/// Backward compatibility: credentials persisted by older Warp versions do not
676+
/// have the `token_received_at` field. Deserializing them must succeed and
677+
/// default to `None` so the next refresh can populate it. Failing this test
678+
/// would mean every existing user loses their MCP OAuth tokens on upgrade.
679+
#[test]
680+
fn persisted_credentials_deserializes_legacy_format_without_received_at() {
681+
// Legacy format: no `token_received_at` field.
682+
let legacy_json = r#"{
683+
"client_id": "client-abc",
684+
"client_secret": null,
685+
"token_response": {
686+
"access_token": "old_access",
687+
"token_type": "bearer",
688+
"expires_in": 3600,
689+
"refresh_token": "old_refresh"
690+
}
691+
}"#;
692+
693+
let parsed: PersistedCredentials =
694+
serde_json::from_str(legacy_json).expect("legacy format must deserialize");
695+
696+
assert_eq!(parsed.client_id, "client-abc");
697+
assert_eq!(parsed.token_received_at, None);
698+
}
699+
700+
/// Regression test for #8863. When rmcp persists refreshed credentials via
701+
/// `CredentialStore::save`, the `token_received_at` must be forwarded into
702+
/// the channel so the persisted (secure-storage) representation can stamp
703+
/// it. Without this, a restart would lose the timestamp and rmcp's
704+
/// pre-emptive refresh check would be permanently disabled for the cached
705+
/// session.
706+
#[tokio::test]
707+
async fn save_forwards_token_received_at_to_persist_channel() {
708+
let (store, rx) = make_test_store(Some("client_secret_xyz".to_string()));
709+
710+
let credentials = StoredCredentials {
711+
client_id: "client-id".to_string(),
712+
token_response: Some(make_test_token_response(Some("refresh-1"))),
713+
granted_scopes: Vec::new(),
714+
token_received_at: Some(1_700_000_500),
715+
};
716+
717+
store.save(credentials).await.expect("save succeeds");
718+
719+
let persisted = rx.try_recv().expect("persist channel received credentials");
720+
assert_eq!(persisted.token_received_at, Some(1_700_000_500));
721+
assert_eq!(persisted.client_id, "client-id");
722+
assert_eq!(
723+
persisted.client_secret.as_deref(),
724+
Some("client_secret_xyz")
725+
);
726+
}
727+
728+
/// Defensive: if rmcp ever calls `save` without a `token_received_at`
729+
/// (e.g., during initial credential set-up before refresh), we must
730+
/// propagate `None` rather than silently substituting a value.
731+
#[tokio::test]
732+
async fn save_forwards_none_when_received_at_is_none() {
733+
let (store, rx) = make_test_store(None);
734+
735+
let credentials = StoredCredentials {
736+
client_id: "c".to_string(),
737+
token_response: Some(make_test_token_response(None)),
738+
granted_scopes: Vec::new(),
739+
token_received_at: None,
740+
};
741+
742+
store.save(credentials).await.expect("save succeeds");
743+
744+
let persisted = rx.try_recv().expect("persist channel received credentials");
745+
assert_eq!(persisted.token_received_at, None);
746+
}
747+
748+
/// `save` only forwards a credentials snapshot to the persist channel when
749+
/// `token_response` is `Some`. This guards the existing branch from regression.
750+
#[tokio::test]
751+
async fn save_skips_persist_when_token_response_absent() {
752+
let (store, rx) = make_test_store(None);
753+
754+
let credentials = StoredCredentials {
755+
client_id: "c".to_string(),
756+
token_response: None,
757+
granted_scopes: Vec::new(),
758+
token_received_at: Some(1_700_000_500),
759+
};
760+
761+
store.save(credentials).await.expect("save succeeds");
762+
763+
assert!(
764+
rx.try_recv().is_err(),
765+
"no PersistedCredentials should be sent when token_response is absent"
766+
);
767+
}
768+
769+
/// The carry-forward of refresh tokens (when the OAuth server omits one
770+
/// from a refresh response) must not interfere with `token_received_at`
771+
/// propagation. Tests both behaviors in one save: the new credentials get
772+
/// the prior refresh token AND the new `token_received_at`.
773+
#[tokio::test]
774+
async fn save_carries_forward_refresh_token_and_preserves_received_at() {
775+
let (store, rx) = make_test_store(None);
776+
777+
// Seed the inner store with prior credentials that have a refresh token.
778+
store
779+
.inner
780+
.save(StoredCredentials {
781+
client_id: "c".to_string(),
782+
token_response: Some(make_test_token_response(Some("prior-refresh-token"))),
783+
granted_scopes: Vec::new(),
784+
token_received_at: Some(1_699_000_000),
785+
})
786+
.await
787+
.expect("seed succeeds");
788+
789+
// Now save NEW credentials that omit a refresh token, simulating a
790+
// refresh response from a server that does not rotate refresh tokens.
791+
let new_credentials = StoredCredentials {
792+
client_id: "c".to_string(),
793+
token_response: Some(make_test_token_response(None)),
794+
granted_scopes: Vec::new(),
795+
token_received_at: Some(1_700_000_500),
796+
};
797+
798+
store.save(new_credentials).await.expect("save succeeds");
799+
800+
let persisted = rx.try_recv().expect("persist channel received credentials");
801+
assert_eq!(
802+
persisted.token_received_at,
803+
Some(1_700_000_500),
804+
"newer received_at preserved"
805+
);
806+
assert_eq!(
807+
persisted
808+
.token_response
809+
.refresh_token()
810+
.map(|rt| rt.secret().to_string()),
811+
Some("prior-refresh-token".to_string()),
812+
"prior refresh token carried forward"
813+
);
814+
}
815+
816+
/// `now_epoch_secs` returns a non-zero monotonic-ish value. Sanity check
817+
/// that the helper does what it claims and matches rmcp's own clock domain.
818+
#[test]
819+
fn now_epoch_secs_returns_recent_unix_time() {
820+
let now = now_epoch_secs();
821+
// Sanity: any timestamp produced by the running test process must be
822+
// after the OSS-release date (2026-04-28) and before the year 2200.
823+
assert!(now > 1_745_000_000, "epoch seconds must be a real time");
824+
assert!(
825+
now < 7_258_118_400,
826+
"epoch seconds must be before year 2200"
827+
);
828+
}
829+
}

0 commit comments

Comments
 (0)