-
Notifications
You must be signed in to change notification settings - Fork 897
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.
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
base: main
Are you sure you want to change the base?
Conversation
I like this idea! a few notes on the approach:
alternative to avoid recursion + nested try/catch:
|
e63c5ba
to
3d1aaa6
Compare
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
3d1aaa6
to
ecb41f5
Compare
Updated. Available for testing on 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this looks good to me, one open question about the double errorCode
declaration, and I'm wondering if there's a more idiomatic way to do that. I'll try to chase that down and if we don't come up with anything, merge as is.
static errorCode: string; | ||
public errorCode: string; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this and the this.constructor
below seem odd, and I'm wondering if there's another way to do this.
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 ainvalid_client
orinvalid_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#L166This 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 onlyall
andtokens
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 underhttps://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
orinvalid_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
Checklist
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.