-
Notifications
You must be signed in to change notification settings - Fork 1.6k
fix: persist resource metadata URL across OAuth redirects #1350
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?
fix: persist resource metadata URL across OAuth redirects #1350
Conversation
In browser OAuth flows, when the user is redirected to the authorization server and back, the resourceMetadataUrl discovered from the WWW-Authenticate header was lost. This caused token exchange to fail because the SDK couldn't locate the correct token endpoint. This commit adds two optional methods to OAuthClientProvider: - saveResourceMetadataUrl(url): Saves the URL before redirect - resourceMetadataUrl(): Loads the saved URL after redirect The SDK now: 1. Loads resourceMetadataUrl from provider if not passed in options 2. Saves resourceMetadataUrl before calling redirectToAuthorization() This change is fully backwards-compatible as both methods are optional. Providers that don't implement them will continue to work as before. Fixes modelcontextprotocol#1234
🦋 Changeset detectedLatest commit: a2d30d7 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
commit: |
pcarleton
left a comment
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.
Thanks @hassan123789
I generally like this direction and think it will be useful. I wanted to propose a slight tweak to make it more extensible in the future. what do you think about this:
interface DiscoveryState {
resourceMetadataUrl?: string;
// Future additions as needed:
// authorizationServerUrl?: string;
// authorizationServerMetadata?: AuthorizationServerMetadata;
// resource?: string;
}
interface OAuthClientProvider {
// ... existing methods ...
/**
* Persists discovery state before redirecting to authorization.
* Called alongside saveCodeVerifier() during authorization flow.
*/
saveDiscoveryState?(state: DiscoveryState): void | Promise<void>;
/**
* Retrieves discovery state after redirect.
* Returns undefined if not persisted or not implemented.
*/
discoveryState?(): DiscoveryState | undefined | Promise<DiscoveryState | undefined>;
}
We can start with just resourceMetadataUrl, but if we decide to extend it in the future (to e.g. skip rediscovering auth endpoints, or if for some reason need to provide that data out of band), we can without needing to add new methods.
Motivation and Context
Fixes #1234
In browser OAuth flows, when the user is redirected to the authorization server and back, the
resourceMetadataUrldiscovered from theWWW-Authenticateheader is lost. This causes token exchange to fail because the SDK cannot locate the correct token endpoint.This is a critical issue for any browser-based MCP client implementing OAuth.
Solution
Added two optional methods to
OAuthClientProvider:saveResourceMetadataUrl(url: URL): Saves the URL before redirectresourceMetadataUrl(): URL | undefined | Promise<URL | undefined>: Loads the saved URL after redirectThe SDK now:
resourceMetadataUrlfrom provider if not passed in options (post-redirect)resourceMetadataUrlbefore callingredirectToAuthorization()(pre-redirect)This follows the existing pattern used by
saveCodeVerifier()/codeVerifier().How Has This Been Tested?
pnpm typecheck:allpassesBreaking Changes
None. Both methods are optional. Existing providers will continue to work without modification.
Types of changes
Checklist
Additional context
Example browser implementation: