-
Notifications
You must be signed in to change notification settings - Fork 14
Improve OAuth diagnostics and error visibility #156
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
Improve OAuth diagnostics and error visibility #156
Conversation
Investigation findings: - OAuth provider requires RFC 8707 resource parameter - MCPProxy successfully detects OAuth and performs discovery - Authorization fails due to missing 'resource' query parameter - Root cause: mcp-go v0.42.0 lacks ExtraParams support Implementation plan structure: - Phase 0 (Priority): OAuth diagnostics and error reporting (2 days) * Fix auth status to show OAuth-configured servers * Parse OAuth error responses for actionable messages * Add OAuth diagnostics to doctor command - Phases 1-5: Config layer, wrapper pattern, integration, tests, docs - Phase 6: Upstream contribution to mcp-go Documentation includes: - runlayer-oauth-investigation.md: Comprehensive investigation findings - 2025-11-27-phase0-oauth-diagnostics.md: Detailed Phase 0 implementation - 2025-11-27-oauth-extra-params.md: Full 6-phase implementation plan - upstream-issue-draft.md: GitHub issue for mcp-go contribution All sensitive data has been sanitized and replaced with generic examples. Next step: Implement Phase 0 to provide OAuth diagnostics
… 1-3)
This commit implements the first batch of Phase 0 OAuth diagnostics
improvements to increase visibility into OAuth authentication failures.
Changes:
- Serialize OAuth configuration in Runtime.GetAllServers() to include
client_id, scopes, auth_url, and token_url in server status
- Extract OAuth config in ManagementService.ListServers() and populate
contracts.OAuthConfig struct for API responses
- Add last_error and authenticated fields to server status
- Implement parseOAuthError() to extract structured error information
from FastAPI validation errors and RFC 6749 OAuth responses
- Add OAuthParameterError type for missing/invalid OAuth parameters
Tests:
- Add unit test for OAuth config serialization in ListServers
- Add comprehensive unit tests for OAuth error parsing covering:
- FastAPI validation errors (Runlayer format)
- RFC 6749 OAuth error responses
- Multiple missing parameters
- Unknown formats and edge cases
- All tests pass
API Changes:
GET /api/v1/servers now returns:
{
"oauth": {
"client_id": "...",
"scopes": [],
"auth_url": "",
"token_url": ""
},
"last_error": "OAuth provider requires 'resource' parameter",
"authenticated": false
}
Related: docs/plans/2025-11-27-phase0-oauth-diagnostics.md
…hase 0 Tasks 4-5)
This commit completes Phase 0 OAuth diagnostics by adding user-facing
error visibility and actionable guidance for OAuth failures.
Changes:
Auth Status Command (auth_cmd.go):
- Add last_error display with clear status indicators
- Show ❌ Authentication Failed, ✅ Authenticated, or ⏳ Pending
- Detect parameter-related OAuth errors
- Provide contextual suggestions for missing parameters
- Link to RFC 8707 documentation and issue tracker
Doctor Command (doctor_cmd.go):
- Add new "OAuth Configuration Issues" section
- Display OAuth parameter mismatches with detailed context
- Show server name, issue type, error message, and impact
- Provide resolution steps and documentation links
Contracts (types.go):
- Add OAuthIssue struct for structured OAuth diagnostics
- Include fields: ServerName, Issue, Error, MissingParams, Resolution, DocumentationURL
- Update Diagnostics struct to include OAuthIssues array
- Update TotalIssues calculation to include OAuth issues
Management Service (diagnostics.go):
- Implement detectOAuthIssues() to identify parameter mismatches
- Add extractParameterName() helper to parse error messages
- Detect "requires 'X' parameter" pattern in OAuth errors
- Generate actionable diagnostics with RFC 8707 context
User Experience:
- Users now see clear error messages instead of generic failures
- Guidance points to upcoming extra_params support
- Links to RFC documentation for technical context
- Consistent messaging across auth status and doctor commands
Example Output:
Server: slack
Status: ❌ Authentication Failed
Error: OAuth provider requires 'resource' parameter
💡 Suggestion:
This OAuth provider requires additional parameters that
MCPProxy doesn't currently support...
Related: docs/plans/2025-11-27-phase0-oauth-diagnostics.md
|
This fixes some parts of what I described in #155 (comment) ... there is a server with |
Dumbris
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.
Code Review: Future Enhancement Suggestion
Excellent work on the OAuth diagnostics implementation! The error parsing and structured output are well done.
Future Enhancement
Populate OAuth Metadata URLs from Discovery
Currently auth_url and token_url are hardcoded to empty strings in internal/runtime/runtime.go:1510-1512. The OAuth metadata discovery (via .well-known/oauth-authorization-server) should already have these URLs available. Consider extracting these from the OAuth client's discovered metadata so users can see which endpoints MCPProxy is attempting to authenticate against in the auth status output. This would make debugging OAuth issues even easier by showing the exact authorization and token URLs being used.
Overall: Great PR, ready to merge! ✅
Fixes the regression from PR #156 where `auth status` wasn't detecting OAuth-configured servers despite the OAuth field being added to contracts. The bug was in ConvertGenericServersToTyped() which is used by the HTTP API to serialize servers. It extracted all server fields (env, headers, timestamps) but completely missed the OAuth configuration. Changes: - Added OAuth config extraction in ConvertGenericServersToTyped() - Extracts all OAuth fields: auth_url, token_url, client_id, scopes, extra_params, redirect_port - Added comprehensive tests for OAuth extraction scenarios Before fix: $ mcpproxy auth status ℹ️ No servers with OAuth configuration found. After fix: $ mcpproxy auth status Server: sentry Status: ❌ Authentication Failed Error: OAuth authorization required - deferred for background processing Resolves: #155 Related: #156
…164) * fix: properly extract OAuth config in ConvertGenericServersToTyped Fixes the regression from PR #156 where `auth status` wasn't detecting OAuth-configured servers despite the OAuth field being added to contracts. The bug was in ConvertGenericServersToTyped() which is used by the HTTP API to serialize servers. It extracted all server fields (env, headers, timestamps) but completely missed the OAuth configuration. Changes: - Added OAuth config extraction in ConvertGenericServersToTyped() - Extracts all OAuth fields: auth_url, token_url, client_id, scopes, extra_params, redirect_port - Added comprehensive tests for OAuth extraction scenarios Before fix: $ mcpproxy auth status ℹ️ No servers with OAuth configuration found. After fix: $ mcpproxy auth status Server: sentry Status: ❌ Authentication Failed Error: OAuth authorization required - deferred for background processing Resolves: #155 Related: #156 * feat: preserve empty OAuth config and add auto-detection - Remove omitempty from OAuth field to prevent config file overwrites - Add DetectOAuthAvailability() function for automatic OAuth detection - Empty OAuth configs now persist through save/load cycles This ensures that when users add 'oauth: {}' to their config, it won't be removed when mcpproxy saves the config back to disk (e.g., when toggling server enable/disable). The auto-detection function will be used in a follow-up commit to automatically detect OAuth availability for HTTP/SSE servers. * chore: regenerate OpenAPI spec after OAuth field changes Updated OpenAPI artifacts to reflect the removal of omitempty from the OAuth field in config.ServerConfig. This ensures the API documentation accurately reflects that OAuth configs are always serialized, even when empty.
This commit addresses UX issues identified during zero-config OAuth testing where OAuth-required servers were displaying confusing or alarming states instead of clear "authentication needed" feedback. ## Changes ### Dashboard.vue - Fixed "ErrorsUnknown" text concatenation by wrapping diagnostics in separate divs - Fixed backend field name mapping (server_name → server, error_message → message) - Added comprehensive diagnostics detail modal with OAuth login buttons - Added colored badges for different diagnostic types (red errors, yellow warnings) ### ServerCard.vue - Changed OAuth servers from red "Disconnected" to blue "Needs Auth" badge - Split error display: red alerts for real errors, blue info for OAuth required - Enhanced OAuth detection to include "deferred for tray UI" pattern - Added clear call-to-action messaging for authentication ### Tray managers.go - Added OAuth detection in getServerStatusDisplay() matching web UI logic - Changed tray icon from 🔴 red to 🔐 blue for OAuth servers - Ensures consistent status display between tray and web UI ### Documentation - Created docs/oauth-ui-feedback-fixes.md with comprehensive analysis - Documented all fixes, remaining issues, and future improvements - Included testing instructions and implementation details ## Remaining Issues (Documented) - OAuth login flow panic in GetAuthorizationURL (backend bug) - Transport error messages need better categorization and truncation Fixes smart-mcp-proxy#156 (if applicable - OAuth UI feedback)
Description
Improves visibility into OAuth authentication failures by surfacing structured error information through CLI commands and API responses. Users experiencing OAuth issues can now see specific error details (like missing provider parameters) instead of generic connection failures, making it easier to diagnose and report problems.
Key improvements:
auth statuscommand now displays OAuth-configured servers with clear status indicators (❌ Failed, ✅ Authenticated, ⏳ Pending) and shows specific error messages when authentication failsdoctorcommand detects OAuth parameter mismatches and provides actionable resolution guidance with RFC documentation linksUser experience example:
Before:
After:
Testing
Test coverage:
auth statusanddoctorcommands confirms diagnostic output