auth: tokens that live 30s or less are refreshed on every request #1442

Activity

1. github-actions

   added

   bugSomething is not workingSomething is not working

   P2Medium: important but non-blocking improvementMedium: important but non-blocking improvement

   T-securitySecurity-related changesSecurity-related changes

   ready for workIssue is well-defined and ready to be picked upIssue is well-defined and ready to be picked up

   on Nov 10, 2026
2. github-actions

   assigned

   jokemanfire

   on Nov 10, 2026
3. github-actions commented on Nov 10, 2026

   

   github-actionsBot

   on Nov 10, 2026 – with GitHub Actions

   Contributor

   More actions

   @jokemanfire is on triage rotation this week and is the reviewer for this one.

   Codex labeled this bug / P2 (T-security). Please confirm or correct those labels.

   _{Automated triage. Labels are a first pass and the assigned maintainer has the final say.}
4. jokemanfire commented on Nov 11, 2026

   

   jokemanfire

   on Nov 11, 2026

   Member

   More actions

   I reproduced this on main (f2b0ddf). Your diagnosis is right, and the fix direction is right — one edge case needs an extra clause.

   Repro (red on main)

   The test you proposed, added next to the existing buffer tests:

   #[tokio::test]
   async fn get_access_token_returns_short_lived_token_without_refreshing() {
       let mut manager = manager_with_metadata(None).await;
       manager.configure_client(test_client_config()).unwrap();
   
       let stored = StoredCredentials {
           client_id: "my-client".to_string(),
           token_response: Some(make_token_response("short-lived", Some(20))),
           granted_scopes: vec![],
           token_received_at: Some(AuthorizationManager::now_epoch_secs()),
           issuer: None,
       };
       manager.credential_store.save(stored).await.unwrap();
   
       let token = manager.get_access_token().await.unwrap();
       assert_eq!(token, "short-lived");
   }

   cargo test -p rmcp --lib --all-features -- get_access_token_returns_short_lived_token_without_refreshing

   → FAILED: called Result::unwrap() on an Err value: AuthorizationRequired (no refresh token configured, so the refresh attempt surfaces as re-auth required). The token was received zero seconds ago.

   Mechanism, for the record: REFRESH_BUFFER_SECS = 30 (crates/rmcp/src/transport/auth.rs:2470), remaining < REFRESH_BUFFER_SECS at :2494, and a refresh writes token_received_at: Some(Self::now_epoch_secs()) at :2618. So every refreshed short-lived token is stored as "received now" and is immediately inside the window again — one token request per call, and with rotation a new refresh token each time.

   On the proposed fix

   buffer = REFRESH_BUFFER_SECS.min(expires_in / 2) is the right shape: 3600 s stays at 30 s, 30 s refreshes at 15 s remaining, 20 s refreshes at 10 s.

   Edge case that needs the extra clause: with expires_in <= 1, the buffer becomes 0, and remaining saturates at 0, so 0 < 0 is false — an expired token is then returned forever and nothing refreshes or re-auths. That would be a worse bug than the one being fixed (silent 401s with no re-auth path). Use:

   let buffer = Self::REFRESH_BUFFER_SECS.min(expires_in.as_secs() / 2);
   if remaining < buffer || remaining == 0 {

   I tried exactly that on main: 20 s token received now returns without refreshing, expires_in = 20 received 25 s ago still attempts a refresh, and the rest of auth::tests stays green (260 passed, 0 failed).

   A max(1) on the buffer would also unbreak the expired case, but it re-introduces "refresh every call" for expires_in = 0; the remaining == 0 clause keeps today's behaviour there, which is the only thing possible when the server issues an unusable token.

   Tests worth shipping with the PR

   1. make_token_response("t", Some(20)) + token_received_at: now → Ok("t") (the test above; red on main).
   2. Same, received_at: now - 25 → refresh is attempted (no refresh token ⇒ AuthorizationRequired) — pins that actual expiry still refreshes.
   3. Some(1) + now - 5 → still refreshes (the remaining == 0 clause).

   Labels: bug / P2 / T-security look right to me — correctness bug, not a vulnerability; the impact is token-endpoint churn and rotation pressure.

   Yes, please send the PR — I'll review it at the pushed head. If you'd rather hand it off, say so and I'll open one.