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
Draft
wants to merge 4 commits into
base: main
Choose a base branch
from
Draft

Adding invalidateCredentials() to OAuthClientProvider #570

wants to merge 4 commits into from

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
Open
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)),
      };
    }
  }

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