Skip to content

Restore AuthDebuggerState.resource as URL #714

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

halter73
Copy link

@halter73 halter73 commented Aug 13, 2025

Not doing so prevents the resource URI from getting sent to the OAuth server's token endpoint during the "Quick OAuth Flow"

Not doing so prevents the resource URI from getting sent to the
OAuth server's token endpoint during the "Quick OAuth flow"
@halter73 halter73 requested a review from localden August 13, 2025 21:00
Copy link

github-actions bot commented Aug 13, 2025

🎭 Playwright E2E Test Results

✅  24 passed

Details

24 tests across 3 suites
 30.8 seconds
 f2865a3
ℹ️  Test Environment: Ubuntu Latest, Node.js v22.18.0
Browsers: Chromium, Firefox

📊 View Detailed HTML Report (download artifacts)

@olaservo
Copy link
Member

Hi @halter73 @dend I think this PR which I was about to merge would invalidate the need for this one? Could you take a look before I merge it? Thanks!

@halter73
Copy link
Author

@olaservo I approved the other PR, because my main motivation is to fix the resource=undefined issue with the /token request, but I don't think it invalidates the need for this one. Either way, it's wrong to restore the AuthDebuggerState in a way that violates the type definition:

resource: URL | null;

Even if we decide to store the resource in a separate session state key, we should fix the AuthDebuggerState restore behavior so that it creates a valid object. And at that point, I don't know what the advantages of the 30 line change is over the 3 line change when you need to make the 3 line change anyway. Personally, I would probably just merge this PR over the other one for simplicities sake, but they're not mutually exclusive.

@olaservo
Copy link
Member

Thanks for the explanation @halter73 , looks like there is just a formatting failure in the CI: https://github.com/modelcontextprotocol/inspector/actions/runs/16999856050/job/48199242030?pr=714

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants