fix(client/auth): propagate saveTokens errors after refresh (#2053)

Signed-off-by: SAY-5 <say.apm35@gmail.com>
Signed-off-by: Sai Asish Y <say.apm35@gmail.com>
Co-authored-by: Konstantin Konstantinov <KKonstantinov@users.noreply.github.com>
Co-authored-by: Konstantin Konstantinov <konstantin@mach5technology.com>

Author: SAY-5

.changeset/propagate-save-tokens-errors-after-refresh.md

@@ -0,0 +1,38 @@
+---
+'@modelcontextprotocol/client': patch
+---
+
+Let `saveTokens` failures surface after a successful token refresh. In `auth()`, one `try`
+wrapped both `refreshAuthorization()` and the `provider.saveTokens()` that persists its
+result, and the `catch` deliberately swallows anything that is not an `OAuthError` — plus
+`ServerError` — so that a failed refresh falls through to a fresh authorization request.
+A persistence error thrown by the provider landed in that same branch: it was discarded
+with no log and no rethrow, and `auth()` continued to `startAuthorization()` and returned
+`'REDIRECT'`.
+
+Against an authorization server that rotates refresh tokens (the OAuth 2.1 default, and
+Keycloak's) this loses credentials rather than merely hiding an error. The exchange has
+already succeeded server-side, so the old refresh token is invalidated at the moment the
+new one is issued; dropping the new token set leaves nothing usable on either side. On a
+headless or CLI client, where `redirectToAuthorization` is typically a no-op, the fallthrough
+is silent and the client is left with stale tokens and no indication of why.
+
+The `try`/`catch` now covers only `refreshAuthorization()`. Persisting the result happens
+after it, on an unguarded path, so a provider's I/O error propagates to the caller.
+
+Refresh-request failures keep their existing control flow exactly: a `ServerError` or an
+unknown error still falls through to a new authorization flow, a non-`ServerError`
+`OAuthError` is still rethrown, and `InsecureTokenEndpointError` is still surfaced. The
+SEP-2352 `issuer` stamp written with the refreshed tokens is unchanged.
+
+Those fallbacks no longer happen in silence, though. Both routes to an unexplained
+re-authorization now emit a `console.warn` naming the cause: the in-place fallthrough in
+the refresh block, and `auth()`'s outer recovery for `invalid_grant`, `invalid_client`,
+and `unauthorized_client`, which discards stored credentials and retries. The second one
+matters most in practice — an expired, revoked, or rotation-reuse-detected refresh token
+is reported as `invalid_grant`, which is precisely the state a dropped token set leaves
+behind for the next call.
+
+Consumers whose `OAuthClientProvider.saveTokens` can reject should note that `auth()` may
+now reject where it previously returned `'REDIRECT'` — that rejection is the failure that
+was being discarded.

docs/migration/upgrade-to-v2.md

@@ -1116,6 +1116,22 @@ OAuth `onUnauthorized` behavior, for composing your own adapter).
   discovery state so the callback-leg check on retry does not mask the original error.
   A provider whose `invalidateCredentials()` implementation special-cases the `'all'`
   scope must handle the split calls.
+- **Token persistence failures after a refresh now propagate.** v1 wrapped both
+  `refreshAuthorization()` and the `saveTokens()` that persists its result in one
+  `try`/`catch`, so a provider's persistence error was discarded alongside AS-side refresh
+  failures and `auth()` fell through to a fresh authorization request, returning
+  `'REDIRECT'`. Only the refresh call is guarded now — persisting runs after it and rejects
+  to the caller. Against an AS that rotates refresh tokens this was destructive rather than
+  merely quiet: the exchange has already succeeded, so the old refresh token is invalidated
+  server-side the moment the new one is issued, and dropping the new token set leaves
+  nothing usable on either side. A provider whose `saveTokens()` can throw (transient
+  storage errors, file-lock contention) must handle the rejection from `auth()` — and from
+  the transport 401-retry paths built on it — where v1 silently re-authorized. Refresh
+  failures themselves keep their control flow: a `ServerError` or an unknown error still
+  falls through to a new authorization request, and `invalid_grant` / `invalid_client` /
+  `unauthorized_client` are still recovered by discarding stored credentials and retrying.
+  Both routes now emit a `console.warn` naming the cause, so an unexplained re-auth prompt
+  can be traced to the failure that triggered it.
 
 #### OAuth client flow errors (new)
 

packages/client/src/client/auth.ts

@@ -984,6 +984,27 @@ export interface AuthOptions {
     forceReauthorization?: boolean;
 }
 
+/**
+ * Recovering from a recoverable OAuth error discards stored credentials and silently starts a
+ * fresh authorization. On a headless client whose `redirectToAuthorization()` is a no-op that
+ * recovery is indistinguishable from nothing happening at all, so name the cause. The most
+ * common case is `invalid_grant` — an expired, revoked, or rotation-reuse-detected refresh
+ * token. See issue #2034.
+ */
+function warnCredentialInvalidation(provider: OAuthClientProvider, error: OAuthError, invalidated: string): void {
+    // `invalidateCredentials` is optional. When a provider omits it nothing is actually
+    // discarded, so do not claim otherwise — the stale credential is still in storage and
+    // will be replayed on the next call, which is the thing worth telling the operator.
+    const action =
+        provider.invalidateCredentials === undefined
+            ? `retrying authorization without discarding the stored ${invalidated} (provider implements no invalidateCredentials())`
+            : `invalidating the stored ${invalidated} and retrying authorization`;
+    // JSON-stringify the AS-supplied values so attacker-supplied control characters cannot
+    // forge log lines — the authorization server is resolved from the resource server's
+    // metadata, and both `code` and `message` are echoed from its response verbatim.
+    console.warn(`[mcp-sdk] OAuth ${JSON.stringify(error.code)} — ${action}. Cause: ${JSON.stringify(error.message)}`);
+}
+
 /**
  * Orchestrates the full auth flow with a server.
  *
@@ -997,13 +1018,15 @@ export async function auth(provider: OAuthClientProvider, options: AuthOptions):
         // Handle recoverable error types by invalidating credentials and retrying
         if (error instanceof OAuthError) {
             if (error.code === OAuthErrorCode.InvalidClient || error.code === OAuthErrorCode.UnauthorizedClient) {
+                warnCredentialInvalidation(provider, error, 'client credentials and tokens');
                 // Not 'all' — preserve discoveryState so the callback-leg gate on retry doesn't
                 // fire a false 'discoveryState was not available on the callback leg' AuthorizationServerMismatchError that masks the
                 // real invalid_client.
                 await provider.invalidateCredentials?.('client');
                 await provider.invalidateCredentials?.('tokens');
                 return await authInternal(provider, options);
             } else if (error.code === OAuthErrorCode.InvalidGrant) {
+                warnCredentialInvalidation(provider, error, 'tokens');
                 await provider.invalidateCredentials?.('tokens');
                 return await authInternal(provider, options);
             }
@@ -1303,19 +1326,17 @@ async function authInternal(
     // current token's granted scope — refreshing would not widen it (RFC 6749
     // §6), so skip straight to a fresh authorization request.
     if (tokens?.refresh_token && !forceReauthorization) {
+        let newTokens: OAuthTokens | undefined;
         try {
             // Attempt to refresh the token
-            const newTokens = await refreshAuthorization(authorizationServerUrl, {
+            newTokens = await refreshAuthorization(authorizationServerUrl, {
                 metadata,
                 clientInformation,
                 refreshToken: tokens.refresh_token,
                 resource,
                 addClientAuthentication: provider.addClientAuthentication,
                 fetchFn
             });
-
-            await provider.saveTokens({ ...newTokens, issuer }, infoCtx);
-            return 'AUTHORIZED';
         } catch (error) {
             // A non-TLS token endpoint is a configuration error — re-authorizing cannot
             // fix it. Surface it so the consumer sees the misconfiguration instead of an
@@ -1325,12 +1346,29 @@ async function authInternal(
             }
             // If this is a ServerError, or an unknown type, log it out and try to continue. Otherwise, escalate so we can fix things and retry.
             if (!(error instanceof OAuthError) || error.code === OAuthErrorCode.ServerError) {
-                // Could not refresh OAuth tokens
+                // Could not refresh OAuth tokens. The fallthrough to a fresh authorization
+                // request is deliberate, but it is invisible on a headless client whose
+                // redirectToAuthorization() is a no-op — so say why it happened.
+                // JSON-stringify the cause: on the non-OAuth-shaped path it carries the raw
+                // response body, so it is arbitrary attacker-supplied bytes.
+                console.warn(
+                    `[mcp-sdk] Could not refresh OAuth tokens; falling back to a new authorization request. ` +
+                        `Cause: ${JSON.stringify(error instanceof Error ? error.message : String(error))}`
+                );
             } else {
                 // Refresh failed for another reason, re-throw
                 throw error;
             }
         }
+
+        // Persist any newly minted tokens. Persistence failures must always
+        // propagate: the authorization server may have rotated the refresh
+        // token, so silently dropping the new tokens would leave the client
+        // with credentials that are already invalid server-side.
+        if (newTokens) {
+            await provider.saveTokens({ ...newTokens, issuer }, infoCtx);
+            return 'AUTHORIZED';
+        }
     }
 
     const state = provider.state ? await provider.state() : undefined;