Skip to content

fix(auth): propagate saveTokens errors during token refresh#2035

Open
jbsky wants to merge 1 commit intomodelcontextprotocol:mainfrom
jbsky:fix/auth-silent-swallow-save-tokens
Open

fix(auth): propagate saveTokens errors during token refresh#2035
jbsky wants to merge 1 commit intomodelcontextprotocol:mainfrom
jbsky:fix/auth-silent-swallow-save-tokens

Conversation

@jbsky
Copy link
Copy Markdown

@jbsky jbsky commented May 8, 2026

Summary

Fixes #2034

saveTokens() errors during token refresh are silently caught and discarded, because saveTokens() sits inside the same try/catch that handles refreshAuthorization() network errors. When an authorization server uses rotating refresh tokens, this is destructive: the AS has already invalidated the old RT, so losing the new tokens leaves the client permanently locked out.

Changes

  • Move newTokens declaration outside the try block
  • Move saveTokens(newTokens) after the catch block so it is no longer swallowed by the refreshAuthorization error handler
  • Add a test that verifies saveTokens errors propagate to the caller

Root cause

// BEFORE (lines 766-786)
try {
    const newTokens = await refreshAuthorization(...);
    await provider.saveTokens(newTokens);  // ← inside try
    return 'AUTHORIZED';
} catch (error) {
    // This catches BOTH refreshAuthorization AND saveTokens errors
    if (!(error instanceof OAuthError) || ...) {
        // silently swallowed — saveTokens failure lost here
    }
}
// AFTER
let newTokens;
try {
    newTokens = await refreshAuthorization(...);
} catch (error) {
    // Only catches refreshAuthorization errors now
}

if (newTokens) {
    await provider.saveTokens(newTokens);  // ← propagates naturally
    return 'AUTHORIZED';
}

Test

Added test case: "should propagate saveTokens errors after successful token refresh" — verifies that a saveTokens rejection is not caught by the refresh error handler.

All 365 existing tests continue to pass.

Previously, auth() wrapped both refreshAuthorization() and
saveTokens() in the same try/catch. Non-OAuthError exceptions
(including I/O errors from saveTokens) were silently discarded.

This is destructive when the AS uses rotating refresh tokens:
the old RT is invalidated server-side upon issuing a new one,
but since saveTokens failure was swallowed, the new tokens were
lost. The client was left with an invalid refresh token and had
to fully re-authenticate.

Fix: move saveTokens() outside the try/catch so its errors
propagate naturally. Only refreshAuthorization() failures are
caught and allow fallthrough to a new authorization flow.

Add test verifying saveTokens errors are propagated.
@jbsky jbsky requested a review from a team as a code owner May 8, 2026 19:03
@changeset-bot
Copy link
Copy Markdown

changeset-bot Bot commented May 8, 2026

⚠️ No Changeset found

Latest commit: b156881

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@pkg-pr-new
Copy link
Copy Markdown

pkg-pr-new Bot commented May 8, 2026

Open in StackBlitz

@modelcontextprotocol/client

npm i https://pkg.pr.new/@modelcontextprotocol/client@2035

@modelcontextprotocol/server

npm i https://pkg.pr.new/@modelcontextprotocol/server@2035

@modelcontextprotocol/express

npm i https://pkg.pr.new/@modelcontextprotocol/express@2035

@modelcontextprotocol/fastify

npm i https://pkg.pr.new/@modelcontextprotocol/fastify@2035

@modelcontextprotocol/hono

npm i https://pkg.pr.new/@modelcontextprotocol/hono@2035

@modelcontextprotocol/node

npm i https://pkg.pr.new/@modelcontextprotocol/node@2035

commit: b156881

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

auth() silently swallows non-OAuthError exceptions from refreshAuthorization / saveTokens, preventing token persistence

1 participant