From 676d040c460cd3d71c9f6daf313a84215079b154 Mon Sep 17 00:00:00 2001
From: SAY-5 <say.apm35@gmail.com>
Date: Mon, 11 May 2026 21:47:07 -0700
Subject: [PATCH] fix(client/auth): propagate saveTokens errors after refresh

Closes #2034

Signed-off-by: SAY-5 <say.apm35@gmail.com>
---
 packages/client/src/client/auth.ts       | 15 +++--
 packages/client/test/client/auth.test.ts | 72 ++++++++++++++++++++++++
 2 files changed, 83 insertions(+), 4 deletions(-)

diff --git a/packages/client/src/client/auth.ts b/packages/client/src/client/auth.ts
index 5f55fb7a0..814179a76 100644
--- a/packages/client/src/client/auth.ts
+++ b/packages/client/src/client/auth.ts
@@ -763,9 +763,10 @@ async function authInternal(
 
     // Handle token refresh or new authorization
     if (tokens?.refresh_token) {
+        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,
@@ -773,9 +774,6 @@ async function authInternal(
                 addClientAuthentication: provider.addClientAuthentication,
                 fetchFn
             });
-
-            await provider.saveTokens(newTokens);
-            return 'AUTHORIZED';
         } catch (error) {
             // 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) {
@@ -785,6 +783,15 @@ async function authInternal(
                 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);
+            return 'AUTHORIZED';
+        }
     }
 
     const state = provider.state ? await provider.state() : undefined;
diff --git a/packages/client/test/client/auth.test.ts b/packages/client/test/client/auth.test.ts
index 04d7f4a3f..7e32a903e 100644
--- a/packages/client/test/client/auth.test.ts
+++ b/packages/client/test/client/auth.test.ts
@@ -2591,6 +2591,78 @@ describe('OAuth Authorization', () => {
             expect(body.get('refresh_token')).toBe('refresh123');
         });
 
+        it('propagates saveTokens errors after a successful refresh (#2034)', async () => {
+            // Regression test: previously the catch block that wraps
+            // refreshAuthorization() also wrapped saveTokens(), silently
+            // swallowing any non-OAuthError thrown while persisting the new
+            // tokens and falling through to startAuthorization(). With
+            // rotating refresh tokens, that loses the freshly minted refresh
+            // token while invalidating the old one server-side.
+            mockFetch.mockImplementation(url => {
+                const urlString = url.toString();
+
+                if (urlString.includes('/.well-known/oauth-protected-resource')) {
+                    return Promise.resolve({
+                        ok: true,
+                        status: 200,
+                        json: async () => ({
+                            resource: 'https://api.example.com/mcp-server',
+                            authorization_servers: ['https://auth.example.com']
+                        })
+                    });
+                } else if (urlString.includes('/.well-known/oauth-authorization-server')) {
+                    return Promise.resolve({
+                        ok: true,
+                        status: 200,
+                        json: async () => ({
+                            issuer: 'https://auth.example.com',
+                            authorization_endpoint: 'https://auth.example.com/authorize',
+                            token_endpoint: 'https://auth.example.com/token',
+                            response_types_supported: ['code'],
+                            code_challenge_methods_supported: ['S256']
+                        })
+                    });
+                } else if (urlString.includes('/token')) {
+                    return Promise.resolve({
+                        ok: true,
+                        status: 200,
+                        json: async () => ({
+                            access_token: 'new-access',
+                            token_type: 'Bearer',
+                            expires_in: 3600,
+                            refresh_token: 'new-refresh'
+                        })
+                    });
+                }
+
+                return Promise.resolve({ ok: false, status: 404 });
+            });
+
+            (mockProvider.clientInformation as Mock).mockResolvedValue({
+                client_id: 'test-client',
+                client_secret: 'test-secret'
+            });
+            (mockProvider.tokens as Mock).mockResolvedValue({
+                access_token: 'old-access',
+                refresh_token: 'refresh123'
+            });
+            const persistError = new Error('disk full');
+            (mockProvider.saveTokens as Mock).mockRejectedValue(persistError);
+
+            await expect(
+                auth(mockProvider, {
+                    serverUrl: 'https://api.example.com/mcp-server'
+                })
+            ).rejects.toBe(persistError);
+
+            // saveTokens was called with the new tokens before throwing.
+            expect(mockProvider.saveTokens).toHaveBeenCalledWith(
+                expect.objectContaining({ access_token: 'new-access', refresh_token: 'new-refresh' })
+            );
+            // The fallthrough to a new authorization flow must NOT happen.
+            expect(mockProvider.redirectToAuthorization).not.toHaveBeenCalled();
+        });
+
         it('skips default PRM resource validation when custom validateResourceURL is provided', async () => {
             const mockValidateResourceURL = vi.fn().mockResolvedValue(undefined);
             const providerWithCustomValidation = {