Skip to content

Conversation

@staugaard
Copy link

@staugaard staugaard commented Dec 15, 2025

Description

Try OAuth Authorization Server Metadata discovery first, before falling back to OpenID Connect.

Type of Change

  • Bug fix (non-breaking change that fixes an issue)
  • New feature (non-breaking change that adds functionality)
  • MCP spec compatibility implementation
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation update
  • Code refactoring (no functional changes)
  • Performance improvement
  • Tests only (no functional changes)
  • Other (please describe):

Checklist

  • My code follows the code style of this project
  • I have performed a self-review of my own code
  • I have added tests that prove my fix is effective or that my feature works
  • I have updated the documentation accordingly

MCP Spec Compliance

  • This PR implements a feature defined in the MCP specification
  • Link to relevant spec section: Authorization
  • Implementation follows the specification exactly

Additional Information

Summary by CodeRabbit

Bug Fixes

  • Improved OAuth authentication server discovery to enhance reliability and performance. The system now attempts authentication protocol metadata sources in an optimized sequence, providing faster connection establishment and more consistent authentication experiences across various identity providers and server configurations.

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Dec 15, 2025

Walkthrough

The change modifies OAuth server metadata discovery in client/transport/oauth.go by reversing the discovery attempt order. The function now attempts OAuth Authorization Server Metadata discovery first, then falls back to OpenID Connect discovery if that fails, with early return on successful OAuth metadata fetch.

Changes

Cohort / File(s) Summary
OAuth Metadata Discovery Reordering
client/transport/oauth.go
Swaps discovery order in getServerMetadata: attempts OAuth Authorization Server Metadata endpoint first (/.well-known/oauth-authorization-server), falls back to OpenID Connect discovery (/.well-known/openid-configuration) on failure, with early return logic.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

  • Verify the OAuth/OIDC specification compliance for this discovery order change
  • Confirm fallback logic and error handling correctly implement early return behavior
  • Ensure no regressions in scenarios where only one discovery endpoint is available

Possibly related PRs

Suggested labels

type: bug

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately summarizes the main change: swapping OAuth Authorization Server Metadata discovery to attempt first before OpenID Connect fallback.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Description check ✅ Passed The PR description is comprehensive and follows the template well. It includes a clear summary, appropriate type of change selections, completed checklist items, and MCP spec compliance details with a valid specification link.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 919c4bf and 72f5235.

📒 Files selected for processing (1)
  • client/transport/oauth.go (1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go

📄 CodeRabbit inference engine (AGENTS.md)

**/*.go: Order imports: standard library first, then third-party, then local packages (goimports enforces this)
Follow Go naming conventions: exported identifiers in PascalCase; unexported in camelCase; acronyms uppercase (HTTP, JSON, MCP)
Error handling: return sentinel errors, wrap with fmt.Errorf("context: %w", err), and check with errors.Is/As
Prefer explicit types and strongly-typed structs; avoid using any except where protocol flexibility is required (e.g., Arguments any)
All exported types and functions must have GoDoc comments starting with the identifier name; avoid inline comments unless necessary
Functions that are handlers or long-running must accept context.Context as the first parameter
Ensure thread safety for shared state using sync.Mutex and document thread-safety requirements in comments
For JSON: use json struct tags with omitempty for optional fields; use json.RawMessage for flexible/deferred parsing

Files:

  • client/transport/oauth.go
🔇 Additional comments (1)
client/transport/oauth.go (1)

418-422: LGTM! Correct discovery order per RFC 8414.

Trying OAuth Authorization Server Metadata (RFC 8414) first is the correct approach for pure OAuth 2.0 implementations. The early return pattern is appropriate and efficient.

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.

1 participant