From 5be12c55fb6df5410abcb1e1c4731cf3c7f760f2 Mon Sep 17 00:00:00 2001 From: SAY-5 Date: Mon, 11 May 2026 21:47:07 -0700 Subject: [PATCH 1/8] fix(client/auth): propagate saveTokens errors after refresh Closes #2034 Signed-off-by: SAY-5 Signed-off-by: Sai Asish Y --- 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 5f55fb7a08..814179a761 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 04d7f4a3fb..7e32a903e0 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 = { From 54e0f8d62adda4a22402f356cd4f1ee0cbdb0e12 Mon Sep 17 00:00:00 2001 From: Sai Asish Y Date: Sun, 16 Aug 2026 03:08:15 -0700 Subject: [PATCH 2/8] test(client/auth): assert saveTokens call without indexing mock.calls Signed-off-by: Sai Asish Y --- packages/client/test/client/auth.test.ts | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/packages/client/test/client/auth.test.ts b/packages/client/test/client/auth.test.ts index 01fea55ac0..cbb007a265 100644 --- a/packages/client/test/client/auth.test.ts +++ b/packages/client/test/client/auth.test.ts @@ -3101,8 +3101,9 @@ describe('OAuth Authorization', () => { ).rejects.toBe(persistError); // saveTokens was called with the new tokens before throwing. - expect((mockProvider.saveTokens as Mock).mock.calls[0][0]).toEqual( - expect.objectContaining({ access_token: 'new-access', refresh_token: 'new-refresh' }) + 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(); From c77fdc288fe95bf8ff5cf91236b9cd6dba4900e1 Mon Sep 17 00:00:00 2001 From: Konstantin Konstantinov Date: Mon, 17 Aug 2026 22:44:13 +0300 Subject: [PATCH 3/8] fix(client/auth): log refresh fallthrough, document behavior change, add changeset Addresses review feedback on #2053: - document the propagation change in docs/migration/upgrade-to-v2.md - warn on the refresh-failure fallthrough instead of swallowing silently (#2034) - scope the rejecting saveTokens stub with mockRejectedValueOnce - add the missing @modelcontextprotocol/client changeset --- ...pagate-save-tokens-errors-after-refresh.md | 32 +++++++++++++++++++ docs/migration/upgrade-to-v2.md | 13 ++++++++ packages/client/src/client/auth.ts | 8 ++++- packages/client/test/client/auth.test.ts | 6 +++- 4 files changed, 57 insertions(+), 2 deletions(-) create mode 100644 .changeset/propagate-save-tokens-errors-after-refresh.md 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..b9270f6985 --- /dev/null +++ b/.changeset/propagate-save-tokens-errors-after-refresh.md @@ -0,0 +1,32 @@ +--- +'@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. That fallthrough +no longer happens in total silence, though — it now emits a `console.warn` naming the +cause, so the re-authorization prompt a user sees can be traced back to the failed refresh. + +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..333c2d1889 100644 --- a/docs/migration/upgrade-to-v2.md +++ b/docs/migration/upgrade-to-v2.md @@ -1116,6 +1116,19 @@ 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 are unchanged: a `ServerError` or an unknown error still falls + through to a new authorization request, now with a `console.warn` naming the cause. #### OAuth client flow errors (new) diff --git a/packages/client/src/client/auth.ts b/packages/client/src/client/auth.ts index d8b921eb9c..cf13780da9 100644 --- a/packages/client/src/client/auth.ts +++ b/packages/client/src/client/auth.ts @@ -1323,7 +1323,13 @@ 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. + console.warn( + `[mcp-sdk] Could not refresh OAuth tokens; falling back to a new authorization request. ` + + `Cause: ${error instanceof Error ? error.message : String(error)}` + ); } else { // Refresh failed for another reason, re-throw throw error; diff --git a/packages/client/test/client/auth.test.ts b/packages/client/test/client/auth.test.ts index cbb007a265..616f71c075 100644 --- a/packages/client/test/client/auth.test.ts +++ b/packages/client/test/client/auth.test.ts @@ -3092,7 +3092,11 @@ describe('OAuth Authorization', () => { issuer: 'https://auth.example.com' }); const persistError = new Error('disk full'); - (mockProvider.saveTokens as Mock).mockRejectedValue(persistError); + // `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, { From 52f492bfc8925efb7dc7e255ed32bd2bf52ceca9 Mon Sep 17 00:00:00 2001 From: Konstantin Konstantinov Date: Tue, 18 Aug 2026 09:29:05 +0300 Subject: [PATCH 4/8] fix(client/auth): warn on the silent credential-invalidation re-auth paths The observability warn added in c77fdc28 only covered the in-place fallthrough for ServerError/unknown refresh failures. invalid_grant -- an expired, revoked, or rotation-reuse-detected refresh token, and the state a dropped token set leaves behind for the next call -- is rethrown and recovered by auth()'s outer wrapper, which discarded credentials and re-authorized with no diagnostic at all. Warn from both recovery branches, cover each path with a test, and correct the changeset and migration guide, which claimed coverage the implementation did not have. --- ...pagate-save-tokens-errors-after-refresh.md | 12 +- docs/migration/upgrade-to-v2.md | 7 +- packages/client/src/client/auth.ts | 15 +++ packages/client/test/client/auth.test.ts | 121 ++++++++++++++++++ 4 files changed, 150 insertions(+), 5 deletions(-) diff --git a/.changeset/propagate-save-tokens-errors-after-refresh.md b/.changeset/propagate-save-tokens-errors-after-refresh.md index b9270f6985..6b06223d84 100644 --- a/.changeset/propagate-save-tokens-errors-after-refresh.md +++ b/.changeset/propagate-save-tokens-errors-after-refresh.md @@ -23,9 +23,15 @@ after it, on an unguarded path, so a provider's I/O error propagates to the call 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. That fallthrough -no longer happens in total silence, though — it now emits a `console.warn` naming the -cause, so the re-authorization prompt a user sees can be traced back to the failed refresh. +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 diff --git a/docs/migration/upgrade-to-v2.md b/docs/migration/upgrade-to-v2.md index 333c2d1889..802c2ec264 100644 --- a/docs/migration/upgrade-to-v2.md +++ b/docs/migration/upgrade-to-v2.md @@ -1127,8 +1127,11 @@ OAuth `onUnauthorized` behavior, for composing your own adapter). 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 are unchanged: a `ServerError` or an unknown error still falls - through to a new authorization request, now with a `console.warn` naming the cause. + 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 cf13780da9..c04f6e3e80 100644 --- a/packages/client/src/client/auth.ts +++ b/packages/client/src/client/auth.ts @@ -984,6 +984,19 @@ 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(error: OAuthError, invalidated: string): void { + console.warn( + `[mcp-sdk] OAuth '${error.code}' — invalidating stored ${invalidated} and retrying authorization. Cause: ${error.message}` + ); +} + /** * Orchestrates the full auth flow with a server. * @@ -997,6 +1010,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(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 +1018,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(error, 'tokens'); await provider.invalidateCredentials?.('tokens'); return await authInternal(provider, options); } diff --git a/packages/client/test/client/auth.test.ts b/packages/client/test/client/auth.test.ts index 616f71c075..fe301eb1b1 100644 --- a/packages/client/test/client/auth.test.ts +++ b/packages/client/test/client/auth.test.ts @@ -3113,6 +3113,127 @@ describe('OAuth Authorization', () => { expect(mockProvider.redirectToAuthorization).not.toHaveBeenCalled(); }); + it('warns when a server-side refresh failure falls back to a new authorization request (#2034)', async () => { + 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')) { + // A real Response: parseErrorResponse() reads the body via .text(), + // which a plain object mock cannot satisfy. + return Promise.resolve( + Response.json(new OAuthError(OAuthErrorCode.ServerError, 'AS is having a bad day').toResponseObject(), { + status: 400 + }) + ); + } + + 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', + issuer: 'https://auth.example.com' + }); + (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 before invalidating tokens and retrying when a refresh fails with invalid_grant (#2034)', async () => { + 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')) { + // A real Response: parseErrorResponse() reads the body via .text(), + // which a plain object mock cannot satisfy. + return Promise.resolve( + Response.json(new OAuthError(OAuthErrorCode.InvalidGrant, 'Refresh token expired').toResponseObject(), { + status: 400 + }) + ); + } + + return Promise.resolve({ ok: false, status: 404 }); + }); + + (mockProvider.clientInformation as Mock).mockResolvedValue({ + client_id: 'test-client', + client_secret: 'test-secret' + }); + // The retry runs against invalidated storage, so the second read has no tokens — + // otherwise the retry would re-POST the dead refresh token and reject. + (mockProvider.tokens as Mock) + .mockResolvedValueOnce({ + access_token: 'old-access', + refresh_token: 'refresh123', + issuer: 'https://auth.example.com' + }) + .mockResolvedValue(undefined); + (mockProvider.saveTokens as Mock).mockResolvedValue(undefined); + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}); + + // invalid_grant is rethrown out of the refresh block and recovered by auth()'s + // outer wrapper, which invalidates tokens and re-authorizes — silently, before. + await expect(auth(mockProvider, { serverUrl: 'https://api.example.com/mcp-server' })).resolves.toBe('REDIRECT'); + + expect(warn).toHaveBeenCalledWith(expect.stringContaining("OAuth 'invalid_grant'")); + expect(warn).toHaveBeenCalledWith(expect.stringContaining('Refresh token expired')); + expect(mockProvider.redirectToAuthorization).toHaveBeenCalled(); + warn.mockRestore(); + }); + it('skips default PRM resource validation when custom validateResourceURL is provided', async () => { const mockValidateResourceURL = vi.fn().mockResolvedValue(undefined); const providerWithCustomValidation = { From 70ba9a0337a11e56fc1a48c223efb6aab20d2fdc Mon Sep 17 00:00:00 2001 From: Konstantin Konstantinov Date: Tue, 18 Aug 2026 11:30:46 +0300 Subject: [PATCH 5/8] fix(client/auth): only claim credential invalidation when the provider supports it invalidateCredentials is optional on OAuthClientProvider, and it is invoked as provider.invalidateCredentials?.(...). For a provider that omits it the new warn claimed stored credentials had been discarded when nothing was touched -- pointing debugging at cleared storage while the stale credential is still there and will be replayed on the next call. Pick the wording from whether the provider actually implements it, and pin both branches with tests. --- packages/client/src/client/auth.ts | 17 +++--- packages/client/test/client/auth.test.ts | 68 ++++++++++++++++++++++++ 2 files changed, 79 insertions(+), 6 deletions(-) diff --git a/packages/client/src/client/auth.ts b/packages/client/src/client/auth.ts index c04f6e3e80..5db35db649 100644 --- a/packages/client/src/client/auth.ts +++ b/packages/client/src/client/auth.ts @@ -991,10 +991,15 @@ export interface AuthOptions { * common case is `invalid_grant` — an expired, revoked, or rotation-reuse-detected refresh * token. See issue #2034. */ -function warnCredentialInvalidation(error: OAuthError, invalidated: string): void { - console.warn( - `[mcp-sdk] OAuth '${error.code}' — invalidating stored ${invalidated} and retrying authorization. Cause: ${error.message}` - ); +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`; + console.warn(`[mcp-sdk] OAuth '${error.code}' — ${action}. Cause: ${error.message}`); } /** @@ -1010,7 +1015,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(error, 'client credentials and tokens'); + 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. @@ -1018,7 +1023,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(error, 'tokens'); + warnCredentialInvalidation(provider, error, 'tokens'); await provider.invalidateCredentials?.('tokens'); return await authInternal(provider, options); } diff --git a/packages/client/test/client/auth.test.ts b/packages/client/test/client/auth.test.ts index fe301eb1b1..e29945941c 100644 --- a/packages/client/test/client/auth.test.ts +++ b/packages/client/test/client/auth.test.ts @@ -3230,10 +3230,78 @@ describe('OAuth Authorization', () => { expect(warn).toHaveBeenCalledWith(expect.stringContaining("OAuth 'invalid_grant'")); expect(warn).toHaveBeenCalledWith(expect.stringContaining('Refresh token expired')); + // This provider implements no invalidateCredentials(), so the warn must not + // claim anything was discarded. + expect(warn).toHaveBeenCalledWith(expect.stringContaining('without discarding the stored tokens')); expect(mockProvider.redirectToAuthorization).toHaveBeenCalled(); warn.mockRestore(); }); + it('reports the discard when the provider does implement invalidateCredentials (#2034)', async () => { + 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( + Response.json(new OAuthError(OAuthErrorCode.InvalidGrant, 'Refresh token expired').toResponseObject(), { + status: 400 + }) + ); + } + + return Promise.resolve({ ok: false, status: 404 }); + }); + + // A local provider — adding invalidateCredentials to the shared mockProvider + // would leak into every later test in this describe. + const invalidateCredentials = vi.fn(); + const provider: OAuthClientProvider = { + ...mockProvider, + invalidateCredentials, + clientInformation: vi.fn().mockResolvedValue({ client_id: 'test-client', client_secret: 'test-secret' }), + tokens: vi + .fn() + .mockResolvedValueOnce({ + access_token: 'old-access', + refresh_token: 'refresh123', + issuer: 'https://auth.example.com' + }) + .mockResolvedValue(undefined), + saveTokens: vi.fn().mockResolvedValue(undefined), + redirectToAuthorization: vi.fn(), + saveCodeVerifier: vi.fn(), + codeVerifier: vi.fn().mockResolvedValue('verifier') + }; + 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('skips default PRM resource validation when custom validateResourceURL is provided', async () => { const mockValidateResourceURL = vi.fn().mockResolvedValue(undefined); const providerWithCustomValidation = { From ced05447a02d31b57558b2f734e72d54d71c89c6 Mon Sep 17 00:00:00 2001 From: Konstantin Konstantinov Date: Tue, 18 Aug 2026 13:10:48 +0300 Subject: [PATCH 6/8] test(client/auth): cover the invalid_client warn and share the #2034 discovery fixture The warn on auth()'s InvalidClient/UnauthorizedClient recovery branch had no test -- the branch is exercised by existing transport tests, but none assert the warning, so the call site could be dropped without a failure. Cover both codes via it.each, asserting the scopes invalidated and the wording. The four #2034 tests also each inlined the same ~40-line discovery fixture, differing only in the token endpoint response. Extract mockDiscoveryWithTokenEndpoint and oauthErrorResponse so the variation under test is what is visible. --- packages/client/test/client/auth.test.ts | 249 +++++++---------------- 1 file changed, 74 insertions(+), 175 deletions(-) diff --git a/packages/client/test/client/auth.test.ts b/packages/client/test/client/auth.test.ts index e29945941c..3ce0ce19e0 100644 --- a/packages/client/test/client/auth.test.ts +++ b/packages/client/test/client/auth.test.ts @@ -3035,13 +3035,9 @@ 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. + // 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(); @@ -3067,30 +3063,40 @@ describe('OAuth Authorization', () => { }) }); } 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(tokenResponse()); } 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', - issuer: 'https://auth.example.com' - }); + // 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 @@ -3098,11 +3104,7 @@ describe('OAuth Authorization', () => { // 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); + 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( @@ -3114,52 +3116,10 @@ describe('OAuth Authorization', () => { }); it('warns when a server-side refresh failure falls back to a new authorization request (#2034)', async () => { - mockFetch.mockImplementation(url => { - const urlString = url.toString(); + mockDiscoveryWithTokenEndpoint(() => oauthErrorResponse(OAuthErrorCode.ServerError, 'AS is having a bad day')); - 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')) { - // A real Response: parseErrorResponse() reads the body via .text(), - // which a plain object mock cannot satisfy. - return Promise.resolve( - Response.json(new OAuthError(OAuthErrorCode.ServerError, 'AS is having a bad day').toResponseObject(), { - status: 400 - }) - ); - } - - 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', - issuer: 'https://auth.example.com' - }); + (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(() => {}); @@ -3171,56 +3131,12 @@ describe('OAuth Authorization', () => { }); it('warns before invalidating tokens and retrying when a refresh fails with invalid_grant (#2034)', async () => { - mockFetch.mockImplementation(url => { - const urlString = url.toString(); + mockDiscoveryWithTokenEndpoint(() => oauthErrorResponse(OAuthErrorCode.InvalidGrant, 'Refresh token expired')); - 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')) { - // A real Response: parseErrorResponse() reads the body via .text(), - // which a plain object mock cannot satisfy. - return Promise.resolve( - Response.json(new OAuthError(OAuthErrorCode.InvalidGrant, 'Refresh token expired').toResponseObject(), { - status: 400 - }) - ); - } - - return Promise.resolve({ ok: false, status: 404 }); - }); - - (mockProvider.clientInformation as Mock).mockResolvedValue({ - client_id: 'test-client', - client_secret: 'test-secret' - }); + (mockProvider.clientInformation as Mock).mockResolvedValue({ client_id: 'test-client', client_secret: 'test-secret' }); // The retry runs against invalidated storage, so the second read has no tokens — // otherwise the retry would re-POST the dead refresh token and reject. - (mockProvider.tokens as Mock) - .mockResolvedValueOnce({ - access_token: 'old-access', - refresh_token: 'refresh123', - issuer: 'https://auth.example.com' - }) - .mockResolvedValue(undefined); + (mockProvider.tokens as Mock).mockResolvedValueOnce(storedTokens).mockResolvedValue(undefined); (mockProvider.saveTokens as Mock).mockResolvedValue(undefined); const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}); @@ -3237,68 +3153,51 @@ describe('OAuth Authorization', () => { 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('reports the discard when the provider does implement invalidateCredentials (#2034)', async () => { - mockFetch.mockImplementation(url => { - const urlString = url.toString(); + mockDiscoveryWithTokenEndpoint(() => oauthErrorResponse(OAuthErrorCode.InvalidGrant, 'Refresh token expired')); - 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( - Response.json(new OAuthError(OAuthErrorCode.InvalidGrant, 'Refresh token expired').toResponseObject(), { - status: 400 - }) - ); - } + const invalidateCredentials = vi.fn(); + const provider = providerWithInvalidation(invalidateCredentials); + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}); - return Promise.resolve({ ok: false, status: 404 }); - }); + 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)); - // A local provider — adding invalidateCredentials to the shared mockProvider - // would leak into every later test in this describe. const invalidateCredentials = vi.fn(); - const provider: OAuthClientProvider = { - ...mockProvider, - invalidateCredentials, - clientInformation: vi.fn().mockResolvedValue({ client_id: 'test-client', client_secret: 'test-secret' }), - tokens: vi - .fn() - .mockResolvedValueOnce({ - access_token: 'old-access', - refresh_token: 'refresh123', - issuer: 'https://auth.example.com' - }) - .mockResolvedValue(undefined), - saveTokens: vi.fn().mockResolvedValue(undefined), - redirectToAuthorization: vi.fn(), - saveCodeVerifier: vi.fn(), - codeVerifier: vi.fn().mockResolvedValue('verifier') - }; + 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('invalidating the stored tokens')); + expect(warn).toHaveBeenCalledWith(expect.stringContaining(`OAuth '${code}'`)); + expect(warn).toHaveBeenCalledWith(expect.stringContaining('invalidating the stored client credentials and tokens')); + expect(warn).toHaveBeenCalledWith(expect.stringContaining(message)); warn.mockRestore(); }); From f11b20762411fa5727c0cccd9531b6e9de0077fa Mon Sep 17 00:00:00 2001 From: Konstantin Konstantinov Date: Tue, 18 Aug 2026 14:04:04 +0300 Subject: [PATCH 7/8] test(client/auth): assert the real invalid_grant outcome for providers without invalidateCredentials The test mocked tokens() to return undefined on the retry read, simulating an invalidation that cannot happen: mockProvider implements no invalidateCredentials, so auth()'s recovery clears nothing. It then asserted 'REDIRECT', which is unreachable for that provider shape. What actually happens: the retry re-reads the same stored tokens, replays the dead refresh token at the token endpoint, and the second invalid_grant propagates -- so auth() rejects and no authorization is ever started. Assert that instead, including the second token POST, alongside the warn wording it already covered. --- packages/client/test/client/auth.test.ts | 29 +++++++++++++++--------- 1 file changed, 18 insertions(+), 11 deletions(-) diff --git a/packages/client/test/client/auth.test.ts b/packages/client/test/client/auth.test.ts index 3ce0ce19e0..07924f5a6b 100644 --- a/packages/client/test/client/auth.test.ts +++ b/packages/client/test/client/auth.test.ts @@ -3130,26 +3130,33 @@ describe('OAuth Authorization', () => { warn.mockRestore(); }); - it('warns before invalidating tokens and retrying when a refresh fails with invalid_grant (#2034)', async () => { - mockDiscoveryWithTokenEndpoint(() => oauthErrorResponse(OAuthErrorCode.InvalidGrant, 'Refresh token expired')); + 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' }); - // The retry runs against invalidated storage, so the second read has no tokens — - // otherwise the retry would re-POST the dead refresh token and reject. - (mockProvider.tokens as Mock).mockResolvedValueOnce(storedTokens).mockResolvedValue(undefined); + // 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 and recovered by auth()'s - // outer wrapper, which invalidates tokens and re-authorizes — silently, before. - await expect(auth(mockProvider, { serverUrl: 'https://api.example.com/mcp-server' })).resolves.toBe('REDIRECT'); + // 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')); - // This provider implements no invalidateCredentials(), so the warn must not - // claim anything was discarded. + // The warn must not claim a discard that never happened. expect(warn).toHaveBeenCalledWith(expect.stringContaining('without discarding the stored tokens')); - expect(mockProvider.redirectToAuthorization).toHaveBeenCalled(); + // 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(); }); From ab7831c604c787bcc069fa41d975a961d38c00fc Mon Sep 17 00:00:00 2001 From: Konstantin Konstantinov Date: Tue, 18 Aug 2026 15:56:06 +0300 Subject: [PATCH 8/8] fix(client/auth): neutralize AS-controlled text in the new warn output The warns added here interpolated OAuth error strings raw. Those are echoed verbatim from the authorization server -- which is resolved from the resource server's protected-resource metadata, so a malicious MCP server can point a client at one it controls -- and on the non-OAuth-shaped path the message carries the entire raw response body. A newline in error_description therefore let the server manufacture extra '[mcp-sdk] ...' lines in an operator's log, forging SDK statements. JSON-stringify the interpolated values, matching the convention this module already applies to attacker-controllable issuer values in authErrors.ts, and pin it with a test. --- packages/client/src/client/auth.ts | 9 ++++++-- packages/client/test/client/auth.test.ts | 27 ++++++++++++++++++++++-- 2 files changed, 32 insertions(+), 4 deletions(-) diff --git a/packages/client/src/client/auth.ts b/packages/client/src/client/auth.ts index 5db35db649..7b25c01fe8 100644 --- a/packages/client/src/client/auth.ts +++ b/packages/client/src/client/auth.ts @@ -999,7 +999,10 @@ function warnCredentialInvalidation(provider: OAuthClientProvider, error: OAuthE provider.invalidateCredentials === undefined ? `retrying authorization without discarding the stored ${invalidated} (provider implements no invalidateCredentials())` : `invalidating the stored ${invalidated} and retrying authorization`; - console.warn(`[mcp-sdk] OAuth '${error.code}' — ${action}. Cause: ${error.message}`); + // 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)}`); } /** @@ -1346,9 +1349,11 @@ async function authInternal( // 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: ${error instanceof Error ? error.message : String(error)}` + `Cause: ${JSON.stringify(error instanceof Error ? error.message : String(error))}` ); } else { // Refresh failed for another reason, re-throw diff --git a/packages/client/test/client/auth.test.ts b/packages/client/test/client/auth.test.ts index 07924f5a6b..7329ded3ed 100644 --- a/packages/client/test/client/auth.test.ts +++ b/packages/client/test/client/auth.test.ts @@ -3149,7 +3149,7 @@ describe('OAuth Authorization', () => { // 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('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')); @@ -3173,6 +3173,29 @@ describe('OAuth Authorization', () => { 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')); @@ -3202,7 +3225,7 @@ describe('OAuth Authorization', () => { // 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 '${code}'`)); + 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();