Skip to content

Conversation

@technicalpickles
Copy link
Contributor

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 status command now displays OAuth-configured servers with clear status indicators (❌ Failed, ✅ Authenticated, ⏳ Pending) and shows specific error messages when authentication fails
  • doctor command detects OAuth parameter mismatches and provides actionable resolution guidance with RFC documentation links
  • API responses include OAuth configuration details (client_id, scopes, auth_url, token_url) for better debugging
  • Structured error parsing extracts parameter names from provider error responses (supports both FastAPI validation errors and RFC 6749 OAuth error format)

User experience example:

Before:

$ mcpproxy auth status
ℹ️  No servers with OAuth configuration found.

After:

$ mcpproxy auth status

━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
🔐 OAuth Authentication Status
━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━

Server: slack
  Status: ❌ Authentication Failed
  Auth URL: https://oauth.example.com/authorize
  Token URL: https://oauth.example.com/token
  Error: OAuth provider requires 'resource' parameter

  💡 Suggestion:
     This OAuth provider requires additional parameters that
     MCPProxy doesn't currently support. Support for custom
     OAuth parameters (extra_params) is coming soon.

     For more information:
     - RFC 8707: https://www.rfc-editor.org/rfc/rfc8707.html
     - Track progress: https://github.com/smart-mcp-proxy/mcpproxy-go/issues

Testing

  • I have tested these changes locally
  • I have added/updated tests that prove my fix is effective or my feature works
  • All existing tests pass

Test coverage:

  • 7 unit tests for OAuth error parsing covering FastAPI validation errors, RFC 6749 OAuth errors, and edge cases
  • Unit test for OAuth config serialization through management service
  • Manual testing with auth status and doctor commands confirms diagnostic output

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
@technicalpickles
Copy link
Contributor Author

This fixes some parts of what I described in #155 (comment) ... there is a server with oauth but it wasn't showing up.

Copy link
Contributor

@Dumbris Dumbris left a 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!

@Dumbris Dumbris merged commit fd0e042 into smart-mcp-proxy:main Nov 28, 2025
19 of 20 checks passed
@technicalpickles technicalpickles deleted the oauth-diagnostics-phase0 branch November 29, 2025 20:23
Dumbris added a commit that referenced this pull request Nov 30, 2025
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
Dumbris added a commit that referenced this pull request Nov 30, 2025
…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.
technicalpickles added a commit to technicalpickles/mcpproxy-go that referenced this pull request Dec 1, 2025
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)
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