fix(client/auth): propagate saveTokens errors after refresh - #2053
Conversation
🦋 Changeset detectedLatest commit: 0754542 The changes in this PR will be included in the next version bump. This PR includes changesets to release 6 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
@modelcontextprotocol/client
@modelcontextprotocol/codemod
@modelcontextprotocol/core
@modelcontextprotocol/server
@modelcontextprotocol/server-legacy
@modelcontextprotocol/express
@modelcontextprotocol/fastify
@modelcontextprotocol/hono
@modelcontextprotocol/node
commit: |
|
The |
Closes modelcontextprotocol#2034 Signed-off-by: SAY-5 <say.apm35@gmail.com> Signed-off-by: Sai Asish Y <say.apm35@gmail.com>
39747b1 to
5be12c5
Compare
Signed-off-by: Sai Asish Y <say.apm35@gmail.com>
Signed-off-by: Sai Asish Y <say.apm35@gmail.com>
|
@claude review |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
…add changeset Addresses review feedback on modelcontextprotocol#2053: - document the propagation change in docs/migration/upgrade-to-v2.md - warn on the refresh-failure fallthrough instead of swallowing silently (modelcontextprotocol#2034) - scope the rejecting saveTokens stub with mockRejectedValueOnce - add the missing @modelcontextprotocol/client changeset
|
@claude review |
There was a problem hiding this comment.
Beyond the inline finding, I also checked whether the new "persistence failures must always propagate" invariant survives the outer auth() retry wrapper (auth.ts:996-1014) — it does: a saveTokens rejection is not an OAuthError, so the wrapper's catch rethrows it instead of triggering a silent credential-invalidation retry.
Extended reasoning...
This run's remaining candidate concern was that the outer auth() wrapper might undo the fix by swallowing saveTokens rejections the way it swallows recoverable OAuth errors. Reading auth.ts:996-1014, the catch only invalidates credentials and retries for OAuthError with codes invalid_client, unauthorized_client, or invalid_grant; any other thrown value — including a storage error from provider.saveTokens — reaches the throw error fallthrough and propagates to the caller, so the PR's invariant holds end-to-end. Recording this so the scope boundary of the inline finding (which concerns the warn's coverage of refresh-failure fallthrough paths, not persistence propagation) is clear. The hunt exited on max_rounds and a confirmed finding is posted inline, so approval is not on the table this run.
…paths The observability warn added in c77fdc2 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.
| 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. | ||
| await provider.invalidateCredentials?.('client'); | ||
| 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); | ||
| } |
There was a problem hiding this comment.
🟣 Pre-existing (in code this diff touches and now advertises via the new warns): auth()'s recoverable-error retry is not gated on the callback leg — when the error comes from the authorization-code exchange (finishAuth), invalidate-and-retry is destructive or self-defeating. The new warnCredentialInvalidation calls surface and legitimize this broken path.
Extended reasoning...
Scenario 1 (invalid_grant, common): user completes auth; finishAuth() exchanges the code and saveTokens persists a good token set. The browser replays the callback URL (refresh/back button) and the app calls finishAuth() again with the now-consumed code. The AS returns invalid_grant; auth()'s catch (line 1020-1023) logs the new warn, calls invalidateCredentials('tokens') — wiping the freshly saved VALID tokens — then retries authInternal with the SAME dead authorizationCode, which re-POSTs it and rejects with invalid_grant. A duplicate callback logs the user out. Scenario 2 (invalid_client, e.g. expired DCR client secret at token exchange): the catch invalidates client credentials (line 1017), retries, and the retry hits authInternal line 1238-1240 — clientInformation is now undefined while authorizationCode is set — throwing the generic 'Existing OAuth client information is required when exchanging an authorization code', masking the real invalid_client. This defeats the adjacent comment's stated goal (lines 1014-1016) of not masking 'the real invalid_client'. Fix at one place: skip
Verification: pre-existing — the failure path is real and reachable, and the diff touches the exact lines. Mechanics verified: finishAuth (packages/client/src/client/streamableHttp.ts:897-905) calls auth(provider, { ..., authorizationCode }). In authInternal, the callback leg exchanges the code at packages/client/src/client/auth.ts:1293-1300 (fetchToken → parseErrorResponse at line 2125 throws an `OAu
…r 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.
|
@claude review |
| // 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); |
There was a problem hiding this comment.
🟡 nit, pre-existing interaction: the saveTokens rejection this PR deliberately propagates out of auth() is collapsed back into a generic UnauthorizedError by the withOAuth fetch middleware (packages/client/src/client/middleware.ts:81-86), which discards the original error object (no cause) and re-types a storage failure as "authentication required" — unlike both transports, which preserve the raw error via markAuthSeamEscape(error) (streamableHttp.ts:571-575, sse.ts:382-386).
Extended reasoning...
A consumer uses createFetchWithMiddleware/withOAuth (the SDK's documented general-purpose OAuth fetch wrapper). Their provider's saveTokens() throws 'disk full' after a successful refresh against a rotating-refresh-token AS — exactly the scenario this PR fixes. auth() rejects with the raw Error as intended, but middleware.ts:81-86 catches it (error instanceof UnauthorizedError is false) and throws new UnauthorizedError('Failed to re-authenticate: disk full') with no cause. The consumer's handler, following the SDK's own guidance that UnauthorizedError means 'redirect the user to authorize', starts a fresh interactive OAuth flow instead of surfacing the storage failure; programmatic discrimination (error instanceof, error === persistError, inspecting .cause) is impossible — only the message substring survives. On this first-party path the failure class the changeset promises to surface ('that rejection is the failure that was being discarded') is still effectively discarded. Fix at middleware.ts: rethrow non-OAuth-flow errors unwrapped, or construct the UnauthorizedError with `{
Verification: nit — every factual element of the candidate checks out. (1) The PR makes auth() reject with the raw saveTokens error: the diff moves persistence out of the try ("if (newTokens) { await provider.saveTokens({ ...newTokens, issuer }, infoCtx); return 'AUTHORIZED'; }", packages/client/src/client/auth.ts:1360-1366), and the changeset explicitly promises "a provider's I/O error propagates to the cal
…ontextprotocol#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 modelcontextprotocol#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.
|
@claude review |
…s 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.
|
@claude review |
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.
|
@claude review |
There was a problem hiding this comment.
This revision addresses everything raised in my earlier rounds and this run found no new bugs — since the change alters auth() control flow in the OAuth client, a human look is still worthwhile as a final step.
What was reviewed:
- Verified the log-forging fix (ab7831c): both new warns now JSON-stringify the AS-supplied error code/message (raw response body on the non-OAuth path), pinned by the forged-newline test.
- Re-checked the narrowed try/catch: only the
saveTokensrejection newly propagates; ServerError/unknown fallthrough, non-ServerError rethrow, andInsecureTokenEndpointErrorhandling are unchanged. - Confirmed the new tests avoid shared-mock leakage (
mockRejectedValueOnce, local provider clones) and that the invalid_grant test now asserts the real rejection outcome for providers withoutinvalidateCredentials.
Extended reasoning...
Overview
The PR fixes issue #2034 in packages/client/src/client/auth.ts: previously one try/catch wrapped both refreshAuthorization() and the provider.saveTokens() persisting its result, so a provider persistence error was silently swallowed and auth() fell through to a fresh authorization flow — destructive under refresh-token rotation, since the new token set is dropped after the old refresh token is already invalidated server-side. The final diff narrows the guard to the refresh call only, moves persistence to an unguarded path, and adds console.warn diagnostics on both silent re-authorization routes (the refresh fallthrough and the outer invalid_grant/invalid_client/unauthorized_client recovery). It ships a changeset, a migration-guide entry under the OAuth behavioral-changes section, and six new tests behind a shared discovery fixture.
Security risks
The code is OAuth client auth — inherently sensitive. The one security issue found across the review rounds (my prior red finding: AS-controlled error.code/error.message/raw response body interpolated unneutralized into console.warn, enabling log-line forgery from a server-directed authorization server) was fixed in commit ab7831c by JSON-stringifying the interpolated values, matching the existing convention in authErrors.ts, and is pinned by a test asserting a forged \n[mcp-sdk]... payload cannot start a new log line. No token values are logged. The propagation change itself does not widen any attack surface; it converts a silent credential drop into a caller-visible rejection.
Level of scrutiny
High scrutiny was applied over seven review rounds: the diff has been re-read in full at each push, and this final run's hunt exited on a dry streak with zero findings. Every previously raised item is now resolved in code: migration-guide documentation, the missing warn on the invalid_grant path, the false "invalidating" claim for providers without invalidateCredentials, test coverage for the invalid_client/unauthorized_client warn, the duplicated 40-line discovery fixture, the unrealistic invalid_grant test (now asserting the real rejection with tokenPosts === 2), the shared-mock mockRejectedValue leak (now mockRejectedValueOnce plus local provider clones), and the log-injection neutralization. Two items I flagged as explicitly pre-existing (the destructive callback-leg retry and the middleware UnauthorizedError collapse) are outside this PR's scope. Despite the clean final state, I am not approving because the guidelines exclude auth-path changes from bot approval — a behavioral change to auth()'s public error contract deserves a human maintainer's sign-off.
Other factors
The author responded promptly and substantively to every round, with each fix commit directly traceable to a finding and accompanied by tests. Test coverage of the new behavior is thorough, including error paths and the neutralization property. The changeset and migration text were re-read against the final implementation and now match what the code ships (propagation, warn coverage on all three recovery codes, unchanged refresh-failure control flow). I could not execute the vitest suite in this sandbox (permission denied), so my confidence rests on reading the code and tests rather than a green local run — one more reason defer is the right decision over approve.
Resolves conflicts with #2053 (propagate saveTokens errors after refresh), which moved the post-refresh saveTokens call out of the try block so persistence failures surface instead of falling through to a new authorization request. This branch's scope preservation moves with it: the conditional spread now applies at the relocated save site, so a refresh response without scope still keeps the stored grant and the payload never carries a present-but-undefined scope key. In the tests, both sides had added a discovery fixture for the auth() refresh path: main's mockDiscoveryWithTokenEndpoint (per-call token endpoint factory, needed by the #2034 regression tests) is kept as the shared base, and this branch's mockRefreshFetchWithTokenResponse is now a thin wrapper over it. All four scope-preservation tests and all of main's #2034 tests are retained. Co-Authored-By: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KXnNnp3fxQYR5HF9BUVhYP
2 participants
Closes #2034
The
try { refreshAuthorization(...); saveTokens(...); } catch (error)block inauth()was wide enough to swallowsaveTokens()failures (any non-OAuthError, plusServerError). With rotating refresh tokens, that loses the freshly minted refresh token while the AS has already invalidated the old one, leaving the client unable to recover.Split the block: the
try/catchnow wraps onlyrefreshAuthorization, where fall-through to a fresh authorization flow is the intended recovery.saveTokensruns after the catch on a separate, unguarded path so its errors propagate to the caller.Added a vitest regression test under
OAuth Authorization > auth functionthat mockssaveTokensto reject, asserts the rejection bubbles to theauth()caller, and assertsredirectToAuthorizationis never reached. The test fails onmainand passes with this change. Fullpackages/clientsuite (365 tests) passes;pnpm lintclean.