-
Notifications
You must be signed in to change notification settings - Fork 918
fix: store resource in session storage #632
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
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.
Hi @jneums , thanks for the PR. Could you resolve the formatting issue? https://github.com/modelcontextprotocol/inspector/actions/runs/16794907055/job/47563830185?pr=632
Thanks!
|
@olaservo I went ahead and ran prettier and it should be good now. |
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.
Looks good to me, thanks!
|
(I think this is ready to merge, just getting another pair of eyes on it since I saw a related conflicting PR come through) |
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.
I still think we should merge #714 to keep the AuthDebuggerState.resource a URL so it matches the type definition regardless of whether we keep this change.
inspector/client/src/lib/auth-types.ts
Line 33 in 16edf53
| resource: URL | null; |
If we keep restoring AuthDebuggerState.resource as a string, that's just asking for more bugs down the road when people reasonably expect the state to match the type definition.
I don't know what makes this better than #714 if we end up having to merge #714 anyway, but I'm happy to approve anything that fixes the resource=undefined issue, and this PR has been open much longer.
Fixes a bug where the
resourceURL was lost during the OAuth redirect, causing aresource parameter mismatcherror during the token exchange.Motivation and Context
During the end-to-end OAuth flow in the Inspector's debug UI, the final token exchange step was failing with a
resource parameter mismatcherror.The root cause was that the
resourceURL, determined during the initial metadata discovery step, was only being stored in the component's volatile in-memory state. When the user is redirected to the authorization server and then back to the Inspector, the page reloads, wiping out this in-memory state.As a result, the final
/tokenrequest was being sent withresource=undefined, which the authorization server correctly rejected. This change fixes the bug by ensuring theresourceURL is persisted across the redirect.How Has This Been Tested?
This has been tested by running the full end-to-end OAuth flow within the MCP Inspector's debug UI against a live resource server.
resource parameter mismatcherror.resourceURL is correctly retrieved fromsessionStorageand included in the/tokenrequest, and valid tokens are received.Breaking Changes
None. This is a bug fix internal to the Inspector's state management and does not alter any public APIs or user-facing configurations.
Types of changes
Checklist
Additional context
The fix follows the existing pattern for persisting OAuth state in the
DebugInspectorOAuthClientProvider.RESOURCE_URLkey was added toconstants.ts.saveResourceandgetResourcemethods were added to theDebugInspectorOAuthClientProvider, usingsessionStorageas the persistence layer.saveResourceafter discovery andgetResourcebefore initiating the token exchange.This ensures the
resourceURL reliably survives the page reload inherent in the OAuth redirect flow.