fix: defer response body consumption; require onUnauthorized on OAuthClientProvider

Addresses two review comments from the second claude[bot] pass:

1. response.text() was called before passing response to
   onUnauthorized(), so custom implementations calling
   ctx.response.text()/.json() got empty results. Moved text()
   consumption after the onUnauthorized branch in all three
   401-handling sites — it's only needed for the fallthrough
   error message.

2. onUnauthorized() was optional on OAuthClientProvider (inherited
   from AuthProvider), but OAuth providers that omit it lose ALL
   401 recovery — no token refresh, no redirect. The migration
   docs said 'optional but recommended' which understates the
   footgun. Now required on OAuthClientProvider (still optional on
   base AuthProvider). TypeScript enforces at compile time; the
   delegation to handleOAuthUnauthorized is a one-liner.

Author: felixweinberger

docs/migration-SKILL.md

@@ -217,7 +217,7 @@ Transport `authProvider` options now accept the minimal `AuthProvider` interface
 | N/A                                                   | `isOAuthClientProvider(provider)` — type guard                              |
 | N/A                                                   | `UnauthorizedContext` — `{ response, serverUrl, fetchFn }`                  |
 
-**For custom `OAuthClientProvider` implementations**, add:
+**For custom `OAuthClientProvider` implementations**, add both methods (both required — TypeScript enforces this):
 
 ```typescript
 async token(): Promise<string | undefined> {

docs/migration.md

@@ -667,7 +667,7 @@ class MyProvider implements OAuthClientProvider {
         return (await this.tokens())?.access_token;
     }
 
-    // Optional but recommended: runs the OAuth flow on 401
+    // Required: runs the OAuth flow on 401 — without this, 401 throws with no recovery
     async onUnauthorized(ctx: UnauthorizedContext): Promise<void> {
         await handleOAuthUnauthorized(this, ctx);
     }

packages/client/src/client/auth.examples.ts

@@ -9,8 +9,8 @@
 
 import type { AuthorizationServerMetadata } from '@modelcontextprotocol/core';
 
-import type { OAuthClientProvider } from './auth.js';
-import { fetchToken } from './auth.js';
+import type { OAuthClientProvider, UnauthorizedContext } from './auth.js';
+import { fetchToken, handleOAuthUnauthorized } from './auth.js';
 
 /**
  * Base class providing no-op implementations of required OAuthClientProvider methods.
@@ -32,6 +32,9 @@ abstract class MyProviderBase implements OAuthClientProvider {
     async token(): Promise<string | undefined> {
         return undefined;
     }
+    async onUnauthorized(ctx: UnauthorizedContext): Promise<void> {
+        await handleOAuthUnauthorized(this, ctx);
+    }
     saveTokens() {
         return Promise.resolve();
     }

packages/client/src/client/auth.ts

@@ -117,10 +117,20 @@ export async function handleOAuthUnauthorized(provider: OAuthClientProvider, ctx
  * code verifiers should not cross different sessions.
  *
  * Extends {@linkcode AuthProvider} — implementations must provide `token()`
- * (typically `return (await this.tokens())?.access_token`) and may provide
- * `onUnauthorized()` (typically `return handleOAuthUnauthorized(this, ctx)`).
+ * (typically `return (await this.tokens())?.access_token`) and `onUnauthorized()`
+ * (typically `return handleOAuthUnauthorized(this, ctx)`). Without `onUnauthorized()`,
+ * 401 responses throw immediately with no token refresh or reauth.
  */
 export interface OAuthClientProvider extends AuthProvider {
+    /**
+     * Runs the OAuth re-authentication flow on 401. Required on `OAuthClientProvider`
+     * (optional on the base `AuthProvider`) because OAuth providers that omit this lose
+     * all 401 recovery — no token refresh, no redirect to authorization.
+     *
+     * Most implementations should delegate: `return handleOAuthUnauthorized(this, ctx)`.
+     */
+    onUnauthorized(ctx: UnauthorizedContext): Promise<void>;
+
     /**
      * The URL to redirect the user agent to after authorization.
      * Return `undefined` for non-interactive flows that don't require user interaction

packages/client/src/client/sse.ts

@@ -255,8 +255,6 @@ export class SSEClientTransport implements Transport {
 
             const response = await (this._fetch ?? fetch)(this._endpoint, init);
             if (!response.ok) {
-                const text = await response.text?.().catch(() => null);
-
                 if (response.status === 401) {
                     const { resourceMetadataUrl, scope } = extractWWWAuthenticateParams(response);
                     this._resourceMetadataUrl = resourceMetadataUrl;
@@ -273,10 +271,12 @@ export class SSEClientTransport implements Transport {
                         return this.send(message);
                     }
                     if (this._authProvider) {
+                        await response.text?.().catch(() => {});
                         throw new UnauthorizedError();
                     }
                 }
 
+                const text = await response.text?.().catch(() => null);
                 throw new Error(`Error POSTing to endpoint (HTTP ${response.status}): ${text}`);
             }
 

packages/client/src/client/streamableHttp.ts

@@ -204,8 +204,6 @@ export class StreamableHTTPClientTransport implements Transport {
             });
 
             if (!response.ok) {
-                await response.text?.().catch(() => {});
-
                 if (response.status === 401) {
                     if (this._authProvider?.onUnauthorized && !this._authRetryInFlight) {
                         this._authRetryInFlight = true;
@@ -221,10 +219,13 @@ export class StreamableHTTPClientTransport implements Transport {
                         }
                     }
                     if (this._authProvider) {
+                        await response.text?.().catch(() => {});
                         throw new UnauthorizedError();
                     }
                 }
 
+                await response.text?.().catch(() => {});
+
                 // 405 indicates that the server does not offer an SSE stream at GET endpoint
                 // This is an expected case that should not trigger an error
                 if (response.status === 405) {
@@ -481,8 +482,6 @@ export class StreamableHTTPClientTransport implements Transport {
             }
 
             if (!response.ok) {
-                const text = await response.text?.().catch(() => null);
-
                 if (response.status === 401) {
                     // Store WWW-Authenticate params for interactive finishAuth() path
                     const { resourceMetadataUrl, scope } = extractWWWAuthenticateParams(response);
@@ -500,10 +499,13 @@ export class StreamableHTTPClientTransport implements Transport {
                         return this.send(message);
                     }
                     if (this._authProvider) {
+                        await response.text?.().catch(() => {});
                         throw new UnauthorizedError();
                     }
                 }
 
+                const text = await response.text?.().catch(() => null);
+
                 if (response.status === 403 && isOAuthClientProvider(this._authProvider)) {
                     const { resourceMetadataUrl, scope, error } = extractWWWAuthenticateParams(response);
 

packages/client/test/client/auth.test.ts

@@ -1039,6 +1039,7 @@ describe('OAuth Authorization', () => {
                 }),
                 tokens: vi.fn().mockResolvedValue(undefined),
                 token: vi.fn(async () => undefined),
+                onUnauthorized: vi.fn(async () => {}),
                 saveTokens: vi.fn(),
                 redirectToAuthorization: vi.fn(),
                 saveCodeVerifier: vi.fn(),
@@ -1985,6 +1986,7 @@ describe('OAuth Authorization', () => {
             clientInformation: vi.fn(),
             tokens: vi.fn(),
             token: vi.fn(async () => undefined),
+            onUnauthorized: vi.fn(async () => {}),
             saveTokens: vi.fn(),
             redirectToAuthorization: vi.fn(),
             saveCodeVerifier: vi.fn(),
@@ -2059,6 +2061,7 @@ describe('OAuth Authorization', () => {
                 }),
                 tokens: vi.fn().mockResolvedValue(undefined),
                 token: vi.fn(async () => undefined),
+                onUnauthorized: vi.fn(async () => {}),
                 saveTokens: vi.fn().mockResolvedValue(undefined),
                 redirectToAuthorization: vi.fn(),
                 saveCodeVerifier: vi.fn(),
@@ -2975,6 +2978,7 @@ describe('OAuth Authorization', () => {
                 }),
                 tokens: vi.fn().mockResolvedValue(undefined),
                 token: vi.fn(async () => undefined),
+                onUnauthorized: vi.fn(async () => {}),
                 saveTokens: vi.fn(),
                 redirectToAuthorization: vi.fn(),
                 saveCodeVerifier: vi.fn(),
@@ -3429,6 +3433,7 @@ describe('OAuth Authorization', () => {
             saveClientInformation: vi.fn().mockResolvedValue(undefined),
             tokens: vi.fn().mockResolvedValue(undefined),
             token: vi.fn(async () => undefined),
+            onUnauthorized: vi.fn(async () => {}),
             saveTokens: vi.fn().mockResolvedValue(undefined),
             redirectToAuthorization: vi.fn().mockResolvedValue(undefined),
             saveCodeVerifier: vi.fn().mockResolvedValue(undefined),

packages/client/test/client/middleware.test.ts

@@ -34,6 +34,7 @@ describe('withOAuth', () => {
             },
             tokens: vi.fn(),
             token: vi.fn(async () => undefined),
+            onUnauthorized: vi.fn(async () => {}),
             saveTokens: vi.fn(),
             clientInformation: vi.fn(),
             redirectToAuthorization: vi.fn(),
@@ -761,6 +762,7 @@ describe('Integration Tests', () => {
             },
             tokens: vi.fn(),
             token: vi.fn(async () => undefined),
+            onUnauthorized: vi.fn(async () => {}),
             saveTokens: vi.fn(),
             clientInformation: vi.fn(),
             redirectToAuthorization: vi.fn(),