-
Notifications
You must be signed in to change notification settings - Fork 327
Fix OAuth callback handling and add HOST auto-detection #495
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
Conversation
MyAgent was creating its own MCPClientManager instance, overriding the parent's instance. This prevented OAuth callbacks from working since the parent Agent class registers callbacks with its own manager.
Callback URLs were stored in memory and lost when Durable Objects hibernated, causing OAuth callbacks to fail with "Not found" errors. Now callback URLs are persisted in the database and restored on wake. - Add registerCallbackUrl() and unregisterCallbackUrl() methods - Restore callback URLs from database in onStart() - Clean up callback URLs when removing MCP servers
The connect() method was always creating a new MCPClientConnection during OAuth reconnect, losing the existing connection state and causing "client isn't in authenticating state" errors. Now reuses the existing connection when reconnecting with OAuth code.
Auto-fallback between transports can consume OAuth codes, causing authentication to fail. Now explicitly detect transport type from URL patterns to prevent unnecessary fallback attempts. - URLs ending with /sse use SSE transport - URLs ending with /mcp use StreamableHTTP transport - Others continue using auto-detection
The callbackHost parameter in addMcpServer() is now optional and will be automatically derived from the current request when not provided. This simplifies MCP server configuration by removing the need for manual HOST environment variable setup in most cases. - Make callbackHost parameter optional in addMcpServer() - Auto-detect host from request.url when in request context - Update examples to reflect HOST as optional - Maintain backward compatibility for existing code
🦋 Changeset detectedLatest commit: b6608fd The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
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: |
- Normalize all URL formats to base URL for consistent storage - Append transport endpoints (/mcp, /sse) at runtime - Track OAuth transport to prevent authorization code consumption - Preserve PKCE verifier across transport attempts - Save transport type when OAuth is required (401 Unauthorized) - Always use auto transport for consistent fallback behavior This ensures OAuth works correctly when auto-fallback occurs between transports, preventing "authorization code already used" errors.
02e326e to
720f0ca
Compare
threepointone
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.
tentatively looks good, think we can add a test?
also pass CI and add a changeset, then I'll review fully and let's land it soon
Yep, working on tests! There were quite a lot of minute issues that had to be fixed, which took a lot of time to even discover manually. Need tests. |
|
no rush then! |
Ensures consistent URL handling regardless of how connections are created and fixes broken auto transport tests.
threepointone
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.
did a quick review, this is looking great (and I'm so happy we're fixing auth)
|
Fixing CI |
Summary
This PR fixes OAuth callback handling for MCP servers and adds automatic HOST detection to simplify configuration.
Changes
OAuth Fixes
Enhancements
callbackHostparameter optional inaddMcpServer()- automatically derives from request when not providedTesting
Test Plan
url,url/,url/mcp,url/sse