modelcontextprotocol/typescript-sdk
Watch
1
0
Fork
You've already forked typescript-sdk
 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions 1

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

Browse source 
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>
This commit is contained in:
Sai Asish Y 2026-08-18 12:09:20 -07:00 • committed by GitHub
commit 3924de99df
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
4 changed files with 293 additions and 5 deletions
Download patch file Download diff file Expand all files Collapse all files

38
.changeset/propagate-save-tokens-errors-after-refresh.md Normal file
View file

@ -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.

16
docs/migration/upgrade-to-v2.md
View file

@ -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)

48
packages/client/src/client/auth.ts
View file

@ -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;

196
packages/client/test/client/auth.test.ts
View file

@ -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 = {