fix: Add OAuth Provider Presets to support GitHub OAuth #676
Conversation
WalkthroughAdds an explicit OAuth provider model and GitHub preset, re-exports provider aliases at the client level, extends OAuthConfig with a Provider field, changes discovery and registration logic to honor configured providers, and updates the OAuth example and README for GitHub usage. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Repository UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
🔇 Additional comments (6)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
examples/oauth_client/main.go (3)
35-35: Clarify PKCE comment accuracy.The comment states "Disable PKCE for GitHub (not supported)" but GitHub does support PKCE—it's optional, not unsupported. Consider updating the comment to reflect that PKCE is disabled for simplicity or that it's optional for GitHub OAuth Apps, rather than implying it's unsupported.
🔎 Suggested comment clarification
- PKCEEnabled: false, // Disable PKCE for GitHub (not supported) + PKCEEnabled: false, // PKCE is optional for GitHub OAuth Apps
18-18: Consider aligning serverURL with GitHub endpoint.The README shows
serverURL = "https://api.githubcopilot.com/v1/mcp"but this file still uses the generic"https://api.example.com/v1/mcp". Since this example is now GitHub-specific (with GitHubProvider configured), aligning the serverURL with the documented GitHub endpoint would make the example more immediately useful.🔎 Suggested alignment
- serverURL = "https://api.example.com/v1/mcp" + serverURL = "https://api.githubcopilot.com/mcp/"
120-125: Consider conditional PKCE generation.PKCE code verifier and challenge are generated even though
PKCEEnabledis set to false on Line 35. While this doesn't cause errors (the values are simply ignored), it's unnecessary computation. Consider generating these values only when PKCE is enabled.🔎 Suggested conditional generation
- // Generate PKCE code verifier and challenge - codeVerifier, err := client.GenerateCodeVerifier() - if err != nil { - log.Fatalf("Failed to generate code verifier: %v", err) - } - codeChallenge := client.GenerateCodeChallenge(codeVerifier) + // Generate PKCE code verifier and challenge if enabled + var codeVerifier, codeChallenge string + // Note: You'd need to check if PKCE is enabled in the handler + // This is a simplified example - in practice, the handler would know its config + codeVerifier, err := client.GenerateCodeVerifier() + if err != nil { + log.Fatalf("Failed to generate code verifier: %v", err) + } + codeChallenge = client.GenerateCodeChallenge(codeVerifier)Note: Since the example doesn't expose PKCEEnabled from the handler, this refactor may require additional changes to check the configuration state.
client/transport/oauth.go (1)
560-563: Consider enhancing error message with actionable guidance.The error message correctly prevents dynamic registration when the provider doesn't support it. Consider making it more helpful by suggesting what the user should do instead (e.g., manually register an OAuth app with the provider).
🔎 Suggested error message enhancement
- return errors.New("dynamic client registration is not supported by this provider") + return errors.New("dynamic client registration is not supported by this provider; please manually register an OAuth application and provide ClientID/ClientSecret")client/transport/oauth_provider.go (2)
13-13: Minor wording suggestion in comment.The comment says "uses split domains for API and generic OAuth" which is slightly unclear. Consider "uses split domains for API and OAuth authorization" or "uses split domains (API domain ≠ authorization domain)" for clarity.
🔎 Suggested comment clarification
-// GitHub does not support RFC 7591 dynamic registration and uses split domains for API and generic OAuth. +// GitHub does not support RFC 7591 dynamic registration and uses split domains for API and OAuth authorization.
14-18: Consider protecting GitHubProvider from accidental modification.GitHubProvider is declared as a package-level
var, making it mutable. If user code accidentally modifies its fields, it would affect all subsequent users of the preset. Consider one of these approaches:
- Document that the preset should not be modified
- Provide a function that returns a new instance each time
- Use a private variable and expose it through a getter function
🔎 Suggested protection using a getter function
-var GitHubProvider = &OAuthProvider{ +var githubProvider = &OAuthProvider{ AuthorizationEndpoint: "https://github.com/login/oauth/authorize", TokenEndpoint: "https://github.com/login/oauth/access_token", SupportsDynamicRegistration: false, } + +// GitHubProvider returns the preset configuration for GitHub OAuth. +// It returns a pointer to a shared instance; do not modify the returned value. +func GitHubProvider() *OAuthProvider { + return githubProvider +}Alternatively, return a new copy each time:
-var GitHubProvider = &OAuthProvider{ - AuthorizationEndpoint: "https://github.com/login/oauth/authorize", - TokenEndpoint: "https://github.com/login/oauth/access_token", - SupportsDynamicRegistration: false, -} +// GitHubProvider returns the preset configuration for GitHub OAuth. +func GitHubProvider() *OAuthProvider { + return &OAuthProvider{ + AuthorizationEndpoint: "https://github.com/login/oauth/authorize", + TokenEndpoint: "https://github.com/login/oauth/access_token", + SupportsDynamicRegistration: false, + } +}Note: This would require updating the re-export in client/oauth.go and the example usage.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
client/oauth.go(1 hunks)client/transport/oauth.go(3 hunks)client/transport/oauth_provider.go(1 hunks)examples/oauth_client/README.md(3 hunks)examples/oauth_client/main.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_provider.goclient/transport/oauth.goclient/oauth.goexamples/oauth_client/main.go
🧬 Code graph analysis (4)
client/transport/oauth_provider.go (1)
client/oauth.go (1)
OAuthProvider(26-26)
client/transport/oauth.go (2)
client/oauth.go (1)
OAuthProvider(26-26)client/transport/oauth_provider.go (1)
OAuthProvider(6-10)
client/oauth.go (1)
client/transport/oauth_provider.go (2)
OAuthProvider(6-10)GitHubProvider(14-18)
examples/oauth_client/main.go (3)
client/oauth.go (2)
TokenStore(17-17)GitHubProvider(29-29)client/transport/oauth.go (1)
TokenStore(54-65)client/transport/oauth_provider.go (1)
GitHubProvider(14-18)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test
🔇 Additional comments (6)
client/oauth.go (1)
25-29: LGTM! Clean re-exports.The OAuthProvider type alias and GitHubProvider variable re-exports follow Go conventions and provide convenient access to transport package entities. GoDoc comments properly start with the identifier names as required by the coding guidelines.
client/transport/oauth.go (2)
40-42: LGTM! Provider field is well-documented.The optional Provider field is properly documented and uses a pointer type to allow nil for the default discovery behavior. This follows Go best practices for optional configuration.
364-373: Reconsider settingIssuerto the API domain when tokens may validate issuer claims.Per OpenID Connect standards, the issuer identifier MUST exactly match the iss claim value in the token. Setting
Issuertoh.baseURL(the API domain likehttps://api.githubcopilot.com) creates a mismatch if tokens are issued with the actual OAuth issuer (https://github.com).This matters because issuer validation occurs after signature verification, where the iss claim is compared to the issuer property from discovery. If this
AuthServerMetadata.Issuerfield is used downstream for token validation or if it's exposed in discovery documents, it could cause legitimate tokens to be rejected during validation.The comment notes RFC 8414 issuers don't match API domains for split-domain providers like GitHub—which is true. However, the solution should preserve the correct OAuth issuer value. Consider:
- Setting
Issuerto the actual OAuth authority (https://github.comfor GitHub)- Documenting why the endpoints differ from the issuer
- Verifying that token validation code uses the actual token's iss claim rather than relying on this metadata field
client/transport/oauth_provider.go (1)
3-10: LGTM! Well-designed provider configuration type.The OAuthProvider struct is clean, well-documented, and appropriately scoped. The GoDoc clearly explains the purpose for providers that don't support RFC 8414 or have split-domain architectures. All fields follow Go naming conventions.
examples/oauth_client/README.md (2)
3-19: LGTM! Clear documentation of provider configuration.The introduction and provider configuration section clearly explains the RFC 8414 discovery mechanism and when manual provider configuration is needed. The GitHub-specific focus aligns well with the code changes and provides helpful context for users.
53-66: LGTM! Configuration section is clear and helpful.The configuration section provides accurate GitHub-specific endpoint information and includes a helpful reminder about configuring the OAuth App callback URL. The scope documentation is also clear and appropriate.
|
Closing in favor of #637. I didn't see there was already an active draft for this feature. |
1 participant
Description
This PR fixes issues with OAuth providers that utilize a split-domain architecture (where the API domain differs from the Authorization domain) and do not support Dynamic Client Registration, as of now, for GitHub.
The Problem:
Currently, the library relies on standard discovery mechanisms that fail for GitHub:
404 Not Foundas it requires manual app creation.api.githubcopilot.com/.well-known/...). Since GitHub's Authorization Server is hosted ongithub.com, this discovery fails, causing the library to incorrectly guess the auth URL ashttps://api.githubcopilot.com/authorize(which is invalid).The Solution:
I have introduced a Provider Preset pattern to
transport/oauth.go:OAuthProviderstruct to define known endpoint configurations and capabilities (e.g.,SupportsDynamicRegistration).client.GitHubProvideras a built-in preset.getServerMetadatato bypass HTTP discovery and use the preset endpoints immediately if configured.RegisterClientto fail gracefully if the provider declares that dynamic registration is unsupported.examples/oauth_client/main.goto demonstrate a working connection to the GitHub MCP server.examples/oauth_client/README.mdfor documentation.Fixes #643
Type of Change
Checklist
examples/oauth_clientwith valid GitHub credentials)Additional Information
Tested locally using a registered GitHub OAuth App. The fix correctly redirects to
github.com/login/oauth/authorizeinstead of the brokenapi.githubcopilot.comURL, and successfully completes the token exchange.Summary by CodeRabbit
New Features
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.