Skip to content

Adding invalidateCredentials() to OAuthClientProvider #570

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Adding invalidateCredentials() to OAuthClientProvider #570

wants to merge 1 commit into from
+502 −82

Conversation

geelen
Copy link
Contributor

@geelen geelen commented May 30, 2025

From working on mcp-remote, I'm seeing a lot of cases where the local state and the server state drift apart. It's especially common when iterating locally (see geelen/mcp-remote#36), but seems to be happening with production servers too.

The issue was that while the SDK defines a series of specific OAuthError types, they're only used on the server side of things. At the crucial point, where POST /token is being called and a invalid_client or invalid_grant are being received, the client simply logs that the request failed and continues: https://github.com/modelcontextprotocol/typescript-sdk/blob/main/src/client/auth.ts#L166

This PR addresses this by looking for those particular error codes and invoking a new method on the OAuthClientProvider (if present): invalidateCredentials. This takes an argument of 'all' | 'client' | 'tokens' | 'verifier', but currently only all and tokens are used.

How Has This Been Tested?

Some tests have been added (generated by Amp, I'm not entirely happy with them but ran out of time and wanted to submit for feedback).

mcp-remote has been released in preview under https://pkg.pr.new/mcp-remote@96 with these changes and has confirmed that the errors in geelen/mcp-remote#36 are fixed.

Breaking Changes

Any error other than invalid_client or invalid_grant are now re-thrown, rather than silently swallowed. But those errors were likely unrecoverable anyway so this would arguably just change the kind of crash message.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation update

Checklist

  • I have read the MCP Documentation
  • My code follows the repository's style guidelines
  • New and existing tests pass locally
  • I have added appropriate error handling
  • I have added or updated documentation as needed

Additional context

Marked as draft as main has moved on so merging isn't possible yet. Would love reviews on the approach though. I will clean up and merge next week.

This was referenced May 30, 2025
Merged
Open
@pcarleton
Copy link
Contributor

I like this idea! a few notes on the approach:

  • the nested try/catch + passing the error in as an arg is a bit tough to follow, might be time to refactor that method into some sub-methods. put an alternative structure below, lmkwyt
  • if you can add an example in the examples repo, that'd be great to show what you expect folks to do in the invalidate method.

alternative to avoid recursion + nested try/catch:

  type AuthSuccess = {
    success: true;
    result: AuthResult;
  };

  type AuthError = {
    success: false;
    error: Error;
  };

  type AuthResponse = AuthSuccess | AuthError;

  export async function auth(
    provider: OAuthClientProvider,
    options: { serverUrl: string | URL, authorizationCode?: string },
  ): Promise<AuthResult> {
    const response = await authInternal(provider, options);

    if (response.success) {
      return response.result;
    }

    // Handle specific error types
    if (response.error instanceof InvalidClientError ||
         response.error instanceof UnauthorizedClientError) {
      await provider.invalidateCredentials?.('all');
      const retryResponse = await authInternal(provider, options);
      if (retryResponse.success) {
        return retryResponse.result;
      }
    } else if (response.error instanceof InvalidGrantError) {
      await provider.invalidateCredentials?.('tokens');
      const retryResponse = await authInternal(provider, options);
      if (retryResponse.success) {
        return retryResponse.result;
      }
    }

    // Couldn't recover
    throw response.error;
  }

  async function authInternal(
    provider: OAuthClientProvider,
    options: { serverUrl: string | URL, authorizationCode?: string },
  ): Promise<AuthResponse> {
    try {
      // Normal auth flow implementation
      // ...existing implementation...

      return { success: true, result: resultValue };
    } catch (error) {
      return {
        success: false,
        error: error instanceof Error ? error : new Error(String(error)),
      };
    }
  }

@geelen geelen force-pushed the invalidate-credentials branch from e63c5ba to 3d1aaa6 Compare June 6, 2025 07:23
@geelen
Refactoring OAuthErrors
ecb41f5 
This makes it possible to parse them from JSON, using OAUTH_ERRORS

Invalidating credentials & retrying when server OAuth errors occur

Updated existing tests

Added some initial test coverage

refactored to avoid recursion as recommended
@geelen geelen force-pushed the invalidate-credentials branch from 3d1aaa6 to ecb41f5 Compare June 6, 2025 08:26
@geelen geelen marked this pull request as ready for review June 6, 2025 08:32
@geelen
Copy link
Contributor Author

geelen commented Jun 6, 2025

Updated. Available for testing on https://pkg.pr.new/geelen/typescript-sdk/@modelcontextprotocol/sdk@cdf3508

I'm going to cut an mcp-remote release using this now, since it seems to make a big difference to have any invalid data automatically cleaned

geelen added a commit to geelen/mcp-remote that referenced this pull request Jun 6, 2025
@geelen
Implementing invalidateCredentials
c7a376a 
See modelcontextprotocol/typescript-sdk#570
geelen added a commit to geelen/mcp-remote that referenced this pull request Jun 6, 2025
@geelen
Implementing invalidateCredentials
b366dec 
See modelcontextprotocol/typescript-sdk#570
geelen added a commit to geelen/mcp-remote that referenced this pull request Jun 6, 2025
@geelen
Implementing invalidateCredentials (#96)
36ed05e 
See modelcontextprotocol/typescript-sdk#570
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@geelen @pcarleton