diff --git a/.changeset/propagate-save-tokens-errors-after-refresh.md b/.changeset/propagate-save-tokens-errors-after-refresh.md new file mode 100644 index 0000000000..6b06223d84 --- /dev/null +++ b/.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. diff --git a/docs/migration/upgrade-to-v2.md b/docs/migration/upgrade-to-v2.md index 8f4cd5990d..802c2ec264 100644 --- a/docs/migration/upgrade-to-v2.md +++ b/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) diff --git a/packages/client/src/client/auth.ts b/packages/client/src/client/auth.ts index 9ebc6fd251..7b25c01fe8 100644 --- a/packages/client/src/client/auth.ts +++ b/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,6 +1018,7 @@ 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. @@ -1004,6 +1026,7 @@ export async function auth(provider: OAuthClientProvider, options: AuthOptions): 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,9 +1326,10 @@ 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, @@ -1313,9 +1337,6 @@ async function authInternal( 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; diff --git a/packages/client/test/client/auth.test.ts b/packages/client/test/client/auth.test.ts index 62c6faed9a..7329ded3ed 100644 --- a/packages/client/test/client/auth.test.ts +++ b/packages/client/test/client/auth.test.ts @@ -3035,6 +3035,202 @@ describe('OAuth Authorization', () => { expect(body.get('refresh_token')).toBe('refresh123'); }); + // The #2034 tests below differ only in how the token endpoint answers, so the + // discovery fixture is shared. `tokenResponse` is invoked per POST to /token. + const mockDiscoveryWithTokenEndpoint = (tokenResponse: () => unknown): void => { + 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(tokenResponse()); + } + + return Promise.resolve({ ok: false, status: 404 }); + }); + }; + + // A real Response: parseErrorResponse() reads the body via .text(), which a plain + // object mock cannot satisfy. + const oauthErrorResponse = (code: OAuthErrorCode, message: string): Response => + Response.json(new OAuthError(code, message).toResponseObject(), { status: 400 }); + + const storedTokens = { access_token: 'old-access', refresh_token: 'refresh123', issuer: 'https://auth.example.com' }; + + 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. + mockDiscoveryWithTokenEndpoint(() => ({ + ok: true, + status: 200, + json: async () => ({ + access_token: 'new-access', + token_type: 'Bearer', + expires_in: 3600, + refresh_token: 'new-refresh' + }) + })); + + (mockProvider.clientInformation as Mock).mockResolvedValue({ client_id: 'test-client', client_secret: 'test-secret' }); + (mockProvider.tokens as Mock).mockResolvedValue(storedTokens); + const persistError = new Error('disk full'); + // `mockRejectedValueOnce`, not `mockRejectedValue`: `mockProvider` is shared by + // the whole describe and its beforeEach only calls `vi.clearAllMocks()`, which + // clears call history but keeps implementations. A persistent rejection would + // leak 'disk full' into every later test that reaches saveTokens. + (mockProvider.saveTokens as Mock).mockRejectedValueOnce(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' }), + expect.anything() + ); + // The fallthrough to a new authorization flow must NOT happen. + expect(mockProvider.redirectToAuthorization).not.toHaveBeenCalled(); + }); + + it('warns when a server-side refresh failure falls back to a new authorization request (#2034)', async () => { + mockDiscoveryWithTokenEndpoint(() => oauthErrorResponse(OAuthErrorCode.ServerError, 'AS is having a bad day')); + + (mockProvider.clientInformation as Mock).mockResolvedValue({ client_id: 'test-client', client_secret: 'test-secret' }); + (mockProvider.tokens as Mock).mockResolvedValue(storedTokens); + (mockProvider.saveTokens as Mock).mockResolvedValue(undefined); + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}); + + await expect(auth(mockProvider, { serverUrl: 'https://api.example.com/mcp-server' })).resolves.toBe('REDIRECT'); + + expect(warn).toHaveBeenCalledWith(expect.stringContaining('Could not refresh OAuth tokens')); + expect(warn).toHaveBeenCalledWith(expect.stringContaining('AS is having a bad day')); + warn.mockRestore(); + }); + + it('warns, and cannot recover, when invalid_grant hits a provider with no invalidateCredentials (#2034)', async () => { + let tokenPosts = 0; + mockDiscoveryWithTokenEndpoint(() => { + tokenPosts++; + return oauthErrorResponse(OAuthErrorCode.InvalidGrant, 'Refresh token expired'); + }); + + (mockProvider.clientInformation as Mock).mockResolvedValue({ client_id: 'test-client', client_secret: 'test-secret' }); + // This provider implements no invalidateCredentials(), so storage is never + // cleared and every read returns the same dead refresh token. + (mockProvider.tokens as Mock).mockResolvedValue(storedTokens); + (mockProvider.saveTokens as Mock).mockResolvedValue(undefined); + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}); + + // invalid_grant is rethrown out of the refresh block into auth()'s outer catch, + // which retries authInternal. Nothing was invalidated, so the retry replays the + // same dead refresh token and the second failure propagates to the caller. + await expect(auth(mockProvider, { serverUrl: 'https://api.example.com/mcp-server' })).rejects.toThrow('Refresh token expired'); + + expect(warn).toHaveBeenCalledWith(expect.stringContaining('OAuth "invalid_grant"')); + expect(warn).toHaveBeenCalledWith(expect.stringContaining('Refresh token expired')); + // The warn must not claim a discard that never happened. + expect(warn).toHaveBeenCalledWith(expect.stringContaining('without discarding the stored tokens')); + // The retry is futile for this provider shape: the dead refresh token goes to the + // token endpoint a second time and no authorization is ever started. + expect(tokenPosts).toBe(2); + expect(mockProvider.redirectToAuthorization).not.toHaveBeenCalled(); + warn.mockRestore(); + }); + + // A local provider — adding invalidateCredentials to the shared mockProvider would + // leak into every later test in this describe. + const providerWithInvalidation = (invalidateCredentials: Mock): OAuthClientProvider => ({ + ...mockProvider, + invalidateCredentials, + clientInformation: vi.fn().mockResolvedValue({ client_id: 'test-client', client_secret: 'test-secret' }), + tokens: vi.fn().mockResolvedValueOnce(storedTokens).mockResolvedValue(undefined), + saveTokens: vi.fn().mockResolvedValue(undefined), + redirectToAuthorization: vi.fn(), + saveCodeVerifier: vi.fn(), + codeVerifier: vi.fn().mockResolvedValue('verifier') + }); + + it('neutralizes AS-controlled error text so it cannot forge log lines (#2034)', async () => { + // The authorization server is resolved from the resource server's metadata, so its + // error strings are attacker-controllable. Newlines in them must not be able to + // manufacture extra '[mcp-sdk] ...' lines in an operator's log. + const forged = 'revoked\n[mcp-sdk] audit: user approved scope admin:all'; + mockDiscoveryWithTokenEndpoint(() => Response.json({ error: 'invalid_grant', error_description: forged }, { status: 400 })); + + const provider = providerWithInvalidation(vi.fn()); + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}); + + await auth(provider, { serverUrl: 'https://api.example.com/mcp-server' }).catch(() => {}); + + const emitted = warn.mock.calls.map(call => String(call[0])).filter(line => line.includes('invalid_grant')); + warn.mockRestore(); + + expect(emitted).toHaveLength(1); + // The newline survives only as an escape, so the forged prefix never begins a + // line of its own. + expect(emitted[0]).not.toContain('\n'); + expect(emitted[0]).toContain('\\n'); + expect(emitted[0]!.split('\n')).toHaveLength(1); + }); + + it('reports the discard when the provider does implement invalidateCredentials (#2034)', async () => { + mockDiscoveryWithTokenEndpoint(() => oauthErrorResponse(OAuthErrorCode.InvalidGrant, 'Refresh token expired')); + + const invalidateCredentials = vi.fn(); + const provider = providerWithInvalidation(invalidateCredentials); + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}); + + await expect(auth(provider, { serverUrl: 'https://api.example.com/mcp-server' })).resolves.toBe('REDIRECT'); + + expect(invalidateCredentials).toHaveBeenCalledWith('tokens'); + expect(warn).toHaveBeenCalledWith(expect.stringContaining('invalidating the stored tokens')); + warn.mockRestore(); + }); + + it.each([ + [OAuthErrorCode.InvalidClient, 'Client authentication failed'], + [OAuthErrorCode.UnauthorizedClient, 'Client not authorized'] + ])('warns before discarding client credentials on %s (#2034)', async (code, message) => { + mockDiscoveryWithTokenEndpoint(() => oauthErrorResponse(code, message)); + + const invalidateCredentials = vi.fn(); + const provider = providerWithInvalidation(invalidateCredentials); + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}); + + await expect(auth(provider, { serverUrl: 'https://api.example.com/mcp-server' })).resolves.toBe('REDIRECT'); + + // Both scopes are invalidated on this branch, and the warn must say so. + expect(invalidateCredentials).toHaveBeenCalledWith('client'); + expect(invalidateCredentials).toHaveBeenCalledWith('tokens'); + expect(warn).toHaveBeenCalledWith(expect.stringContaining(`OAuth ${JSON.stringify(code)}`)); + expect(warn).toHaveBeenCalledWith(expect.stringContaining('invalidating the stored client credentials and tokens')); + expect(warn).toHaveBeenCalledWith(expect.stringContaining(message)); + warn.mockRestore(); + }); + it('skips default PRM resource validation when custom validateResourceURL is provided', async () => { const mockValidateResourceURL = vi.fn().mockResolvedValue(undefined); const providerWithCustomValidation = {