Refactor/user friendliness #2
Conversation
Signed-off-by: Tommy Nguyen <tuannvm@hotmail.com>
Signed-off-by: Tommy Nguyen <tuannvm@hotmail.com>
Signed-off-by: Tommy Nguyen <tuannvm@hotmail.com>
Signed-off-by: Tommy Nguyen <tuannvm@hotmail.com>
Signed-off-by: Tommy Nguyen <tuannvm@hotmail.com>
Signed-off-by: Tommy Nguyen <tuannvm@hotmail.com>
Signed-off-by: Tommy Nguyen <tuannvm@hotmail.com>
|
Warning Rate limit exceeded@tuannvm has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 6 minutes and 18 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
WalkthroughThis PR adds a fluent ConfigBuilder, extends WithOAuth to return the OAuth Server instance (changing its signature), adds server helper methods/endpoints and HTTP-server options, restructures and consolidates tests, updates and adds examples to the new API, and removes several planning and long-form documentation files. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant App as Application
participant WithOAuth as WithOAuth()
participant OAuth as OAuth Server
participant MCP as MCP Server
participant HTTP as HTTP Server
App->>WithOAuth: WithOAuth(mux, cfg)
WithOAuth->>OAuth: initialize Server & register endpoints
WithOAuth-->>App: return (server, option, err)
App->>MCP: NewMCPServer(name, option)
MCP->>MCP: attach OAuth middleware
App->>OAuth: GetHTTPServerOptions()
OAuth-->>App: return StreamableHTTP options
App->>HTTP: Start server (with options)
App->>OAuth: LogStartup(useTLS)
OAuth-->>App: log endpoints & status
sequenceDiagram
autonumber
participant Dev as Developer
participant Builder as ConfigBuilder
participant Config as Config
Dev->>Builder: NewConfigBuilder()
Dev->>Builder: WithProvider(...)/WithIssuer(...)/WithHost(...)/WithTLS(...)
Builder->>Builder: set fields (fluent)
Dev->>Builder: Build()
Builder->>Config: auto-detect ServerURL if unset
Builder-->>Dev: return (*Config, nil) or (nil, error)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
docs/CONFIGURATION.md (2)
539-557: Align environment variable names with FromEnv() (OIDC_ and MCP_URL)*The sample .env uses OAUTH_ISSUER/AUDIENCE/CLIENT_* and OAUTH_SERVER_URL, but FromEnv() expects OIDC_* and MCP_URL. Update to prevent misconfiguration.
Apply:
- OAUTH_ISSUER=https://yourcompany.okta.com - OAUTH_AUDIENCE=api://my-mcp-server + OIDC_ISSUER=https://yourcompany.okta.com + OIDC_AUDIENCE=api://my-mcp-server ... - OAUTH_CLIENT_ID=your-client-id - OAUTH_CLIENT_SECRET=your-client-secret - OAUTH_SERVER_URL=https://your-server.com - OAUTH_REDIRECT_URIS=https://your-server.com/oauth/callback + OIDC_CLIENT_ID=your-client-id + OIDC_CLIENT_SECRET=your-client-secret + MCP_URL=https://your-server.com + OAUTH_REDIRECT_URIS=https://your-server.com/oauth/callback
562-577: Fix code sample to read the correct env varsMatch FromEnv()/docs list to avoid confusion.
- Provider: os.Getenv("OAUTH_PROVIDER"), - Issuer: os.Getenv("OAUTH_ISSUER"), - Audience: os.Getenv("OAUTH_AUDIENCE"), - ClientID: os.Getenv("OAUTH_CLIENT_ID"), - ClientSecret: os.Getenv("OAUTH_CLIENT_SECRET"), - ServerURL: os.Getenv("OAUTH_SERVER_URL"), - RedirectURIs: os.Getenv("OAUTH_REDIRECT_URIS"), + Provider: os.Getenv("OAUTH_PROVIDER"), + Issuer: os.Getenv("OIDC_ISSUER"), + Audience: os.Getenv("OIDC_AUDIENCE"), + ClientID: os.Getenv("OIDC_CLIENT_ID"), + ClientSecret: os.Getenv("OIDC_CLIENT_SECRET"), + ServerURL: os.Getenv("MCP_URL"), + RedirectURIs: os.Getenv("OAUTH_REDIRECT_URIS"),docs/providers/OKTA.md (1)
131-147: Correct audience configuration for Okta—do not use custom 'aud' claimThe 'aud' claim is a standard OIDC reserved claim. Replace the custom claim steps with proper Authorization Server configuration:
- Navigate to Security → API → Authorization Servers
- Select your authorization server → Settings tab
- Set the Audience field to your resource server URI (e.g.,
https://api.example.com)This Audience value becomes the
audclaim in all access tokens from that authorization server. Do not add a custom claim named 'aud'.
🧹 Nitpick comments (13)
config_test.go (1)
221-230: Use t.Setenv instead of manual Setenv/Unsetenv in testsSimplifies setup/teardown and avoids leaks if a test bails early.
Example:
- for k, v := range tt.envVars { - os.Setenv(k, v) - } - defer func() { - for k := range tt.envVars { - os.Unsetenv(k) - } - }() + for k, v := range tt.envVars { + t.Setenv(k, v) + }security_test.go (2)
295-303: Don’t ignore decode/unmarshal errors in tampering testsSwallowing errors can mask regressions. Assert decode/unmarshal succeed before tampering.
- decoded, _ := base64.URLEncoding.DecodeString(signedState) + decoded, err := base64.URLEncoding.DecodeString(signedState) + if err != nil { t.Fatalf("decode signed state: %v", err) } var tamperedData map[string]string - _ = json.Unmarshal(decoded, &tamperedData) + if err := json.Unmarshal(decoded, &tamperedData); err != nil { + t.Fatalf("unmarshal signed state: %v", err) + } ... - unsignedJSON, _ := json.Marshal(unsignedData) + unsignedJSON, err := json.Marshal(unsignedData) + if err != nil { t.Fatalf("marshal unsigned state: %v", err) }Also applies to: 317-321
399-427: Extend HTTPS enforcement tests to cover handler behaviorCurrent test only reimplements logic. Add endpoint-level tests (authorize/callback) ensuring HTTP non-localhost redirects are rejected, and HTTPS/localhost are accepted.
examples/simple/main.go (1)
18-24: Remove unnecessary ServerURL in native-mode exampleServerURL isn’t needed unless running in proxy mode. Dropping it clarifies defaults.
_, oauthOption, err := oauth.WithOAuth(mux, &oauth.Config{ Provider: "okta", // or "hmac", "google", "azure" Issuer: "https://your-company.okta.com", Audience: "api://your-mcp-server", - ServerURL: "https://your-server.com", })docs/MIGRATION.md (1)
366-368: Unify version references across docsThis file mentions v0.1.0, while CONFIGURATION.md labels features as v0.2.0+. Recommend aligning or removing version qualifiers to avoid confusion.
config.go (1)
36-90: Optional: strengthen ServerURL validation (non-breaking proposal)Consider validating ServerURL format and enforcing HTTPS for non-localhost in Validate() (only warn or gate in proxy mode). This prevents accidental HTTP on public hosts and catches malformed URLs early.
oauth.go (5)
63-66: Avoid hard-coded protocol version in handler creationPassing "1.0.0" here will drift. Make it a const or plumb from a package/version var.
- handler := CreateOAuth2Handler(cfg, "1.0.0", logger) + handler := CreateOAuth2Handler(cfg, Version, logger) // expose oauth.Version or pass from caller
89-97: Gate proxy-only endpoints by Mode to avoid exposing unused routesYou register proxy endpoints even in native mode. Gate them for clarity and surface minimization.
func (s *Server) RegisterHandlers(mux *http.ServeMux) { mux.HandleFunc("/.well-known/oauth-authorization-server", s.handler.HandleAuthorizationServerMetadata) mux.HandleFunc("/.well-known/oauth-protected-resource", s.handler.HandleProtectedResourceMetadata) mux.HandleFunc("/.well-known/jwks.json", s.handler.HandleJWKS) - mux.HandleFunc("/oauth/authorize", s.handler.HandleAuthorize) - mux.HandleFunc("/oauth/callback", s.handler.HandleCallback) - mux.HandleFunc("/oauth/token", s.handler.HandleToken) - mux.HandleFunc("/oauth/register", s.handler.HandleRegister) mux.HandleFunc("/.well-known/openid-configuration", s.handler.HandleOIDCDiscovery) + if s.config.Mode == "proxy" { + mux.HandleFunc("/oauth/authorize", s.handler.HandleAuthorize) + mux.HandleFunc("/oauth/callback", s.handler.HandleCallback) + mux.HandleFunc("/oauth/token", s.handler.HandleToken) + mux.HandleFunc("/oauth/register", s.handler.HandleRegister) + } }
99-133: Normalize base URL to avoid double slashesIf Config.ServerURL ends with “/”, helpers will emit “//path”. Trim trailing “/” once.
-func (s *Server) GetAuthorizationServerMetadataURL() string { - return fmt.Sprintf("%s/.well-known/oauth-authorization-server", s.config.ServerURL) -} +func (s *Server) GetAuthorizationServerMetadataURL() string { + base := strings.TrimRight(s.config.ServerURL, "/") + return base + "/.well-known/oauth-authorization-server" +}Repeat similarly for GetProtectedResourceMetadataURL, GetOIDCDiscoveryURL, GetCallbackURL, GetAuthorizeURL, GetTokenURL, GetRegisterURL.
Additional required import:
import ( "encoding/json" "fmt" "net/http" + "strings"
160-180: Include JWKS in startup logs; keep logs spec-alignedAdd JWKS to the startup list for parity with registered handlers.
s.logger.Info(" - OIDC discovery: %s", s.GetOIDCDiscoveryURL()) + s.logger.Info(" - JWKS keys: %s", fmt.Sprintf("%s/.well-known/jwks.json", strings.TrimRight(s.config.ServerURL, "/")))
134-139: Name fits better as URL, not PathEndpoint.Path actually holds a full URL. Consider renaming to URL before this public API ships to avoid confusion.
-type Endpoint struct { - Path string +type Endpoint struct { + URL string Description string }And update GetAllEndpoints builders accordingly.
examples/advanced/main.go (2)
66-74: Unify TLS detection; reuse one boolean for logs, status, and server startStatus uses only CERT presence; startup/server use CERT+KEY. Make them consistent.
- // Feature 5: GetStatusString - Human-readable OAuth status - useTLS := getEnv("HTTPS_CERT_FILE", "") != "" - fmt.Fprintf(w, `{"status":"ok","oauth":"%s"}`, oauthServer.GetStatusString(useTLS)) + // Feature 5: GetStatusString - Human-readable OAuth status + useTLS := tlsEnabled() + _ = json.NewEncoder(w).Encode(map[string]string{ + "status": "ok", + "oauth": oauthServer.GetStatusString(useTLS), + }) ... - useTLS := getEnv("HTTPS_CERT_FILE", "") != "" && getEnv("HTTPS_KEY_FILE", "") != "" + useTLS := tlsEnabled() ... - if useTLS { + if useTLS {Add helper and import:
+import "encoding/json" ... +func tlsEnabled() bool { + return getEnv("HTTPS_CERT_FILE", "") != "" && getEnv("HTTPS_KEY_FILE", "") != "" +}Also applies to: 93-99, 101-116
50-63: CORS example is permissive; document risksUsing Access-Control-Allow-Origin: * with Authorization is okay only when not using credentials. Consider noting this in comments to avoid copy-paste into prod.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (27)
CHANGELOG.md(0 hunks)README.md(2 hunks)api_test.go(8 hunks)attack_scenarios_test.go(0 hunks)config.go(1 hunks)config_test.go(1 hunks)docs/CLIENT-SETUP.md(16 hunks)docs/CONFIGURATION.md(18 hunks)docs/MIGRATION.md(12 hunks)docs/RELEASING.md(0 hunks)docs/SECURITY.md(11 hunks)docs/TROUBLESHOOTING.md(20 hunks)docs/implementation.md(0 hunks)docs/oauth.md(0 hunks)docs/plan-standalone.md(0 hunks)docs/plan.md(0 hunks)docs/providers/AZURE.md(5 hunks)docs/providers/GOOGLE.md(4 hunks)docs/providers/HMAC.md(1 hunks)docs/providers/OKTA.md(3 hunks)examples/README.md(0 hunks)examples/advanced/main.go(1 hunks)examples/embedded/main.go(0 hunks)examples/simple/main.go(1 hunks)oauth.go(2 hunks)security_scenarios_test.go(0 hunks)security_test.go(2 hunks)
💤 Files with no reviewable changes (10)
- docs/implementation.md
- docs/RELEASING.md
- attack_scenarios_test.go
- examples/embedded/main.go
- examples/README.md
- docs/plan.md
- security_scenarios_test.go
- docs/oauth.md
- docs/plan-standalone.md
- CHANGELOG.md
🧰 Additional context used
🧬 Code graph analysis (7)
security_test.go (1)
handlers.go (2)
OAuth2Handler(26-30)OAuth2Config(38-63)
config_test.go (1)
config.go (3)
NewConfigBuilder(164-170)AutoDetectServerURL(262-268)FromEnv(271-299)
api_test.go (1)
oauth.go (2)
WithOAuth(282-291)NewServer(40-73)
config.go (2)
provider/provider.go (2)
Config(32-38)Logger(24-29)logger.go (1)
Logger(22-27)
oauth.go (2)
middleware.go (1)
CreateHTTPContextFunc(217-236)config.go (1)
Config(10-34)
examples/advanced/main.go (3)
config.go (1)
NewConfigBuilder(164-170)oauth.go (1)
WithOAuth(282-291)middleware.go (1)
GetUserFromContext(199-202)
examples/simple/main.go (3)
oauth.go (1)
WithOAuth(282-291)config.go (1)
Config(10-34)middleware.go (2)
GetUserFromContext(199-202)CreateHTTPContextFunc(217-236)
🪛 markdownlint-cli2 (0.18.1)
docs/SECURITY.md
346-346: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (8)
api_test.go (3)
34-42: LGTM - Proper handling of new return signature.The test correctly captures the new
oauthServerreturn value and validates it's non-nil, which is appropriate for verifying the API change.
253-324: LGTM - Comprehensive WrapHandler test coverage.The new test cases properly validate:
- 401 response when token is missing
- WWW-Authenticate header presence
- Token extraction from Authorization header
The test structure is clear and covers the essential authentication middleware behavior.
326-426: LGTM - Thorough helper method validation.The test systematically validates:
- All URL discovery endpoints with expected values
- Mode-specific endpoint counts (3 for native, 7 for proxy)
- Status string generation with HTTPS warnings
- HTTP server options integration with StreamableHTTPServer
Excellent coverage of the new Server helper methods.
README.md (2)
1-18: LGTM - Clear documentation of new API.The README effectively communicates:
- The one-time setup pattern
- The new three-value return signature from
WithOAuth- The automatic protection of all tools
The example is concise and demonstrates the simplified integration approach.
75-112: LGTM - Comprehensive quick start guide.The quick start section provides:
- Clear installation instructions
- Complete working example with the new API
- User context access pattern
- Proper integration with StreamableHTTPServer
The code examples are practical and ready to use.
config.go (1)
121-127: Review comment is incorrect—no compile-time type mismatch exists.Both
oauth.Loggerandprovider.Loggerhave identical method signatures (Debug, Info, Warn, Error). In Go, interfaces are structurally typed: two interfaces with the same methods are implicitly assignable. The assignment at line 126 (Logger: logger) will compile without any adapter. No changes are required.Likely an incorrect or invalid review comment.
oauth.go (1)
280-291: Great: returns server + middleware optionThe revised WithOAuth signature is clean and ergonomic. Registering handlers then returning the middleware keeps call sites simple.
examples/advanced/main.go (1)
33-48: Nice end-to-end wiringUsing oauth.WithOAuth + GetHTTPServerOptions + WrapHandlerFunc yields a clear, reproducible setup. Good example flow.
| ``` | ||
| INFO: Validating token (hash: a7bc40a987f35871...) | ||
| ``` |
There was a problem hiding this comment.
Specify language for fenced code block.
The fenced code block is missing a language identifier, which impacts syntax highlighting and accessibility.
Apply this diff to add the language identifier:
-```
+```text
INFO: Validating token (hash: a7bc40a987f35871...)
<details>
<summary>🧰 Tools</summary>
<details>
<summary>🪛 markdownlint-cli2 (0.18.1)</summary>
346-346: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
</details>
</details>
<details>
<summary>🤖 Prompt for AI Agents</summary>
In docs/SECURITY.md around lines 346 to 348, the fenced code block lacks a
language identifier; update the block to include a language (use "text" per the
review) so it reads as a triple-backtick block annotated with "text" before the
content, preserving the inner log line unchanged.
</details>
<!-- This is an auto-generated comment by CodeRabbit -->
| // GetAllEndpoints returns all OAuth endpoints with descriptions | ||
| func (s *Server) GetAllEndpoints() []Endpoint { | ||
| endpoints := []Endpoint{ | ||
| {Path: s.GetAuthorizationServerMetadataURL(), Description: "OAuth metadata"}, | ||
| {Path: s.GetProtectedResourceMetadataURL(), Description: "Resource metadata"}, | ||
| {Path: s.GetOIDCDiscoveryURL(), Description: "OIDC discovery"}, | ||
| } | ||
|
|
||
| if s.config.Mode == "proxy" { | ||
| endpoints = append(endpoints, | ||
| Endpoint{Path: s.GetAuthorizeURL(), Description: "Authorization endpoint"}, | ||
| Endpoint{Path: s.GetCallbackURL(), Description: "OAuth callback"}, | ||
| Endpoint{Path: s.GetTokenURL(), Description: "Token endpoint"}, | ||
| Endpoint{Path: s.GetRegisterURL(), Description: "Client registration"}, | ||
| ) | ||
| } | ||
|
|
||
| return endpoints | ||
| } |
There was a problem hiding this comment.
JWKS endpoint missing from GetAllEndpoints list
You register /.well-known/jwks.json but don’t include it in discovery helpers.
func (s *Server) GetAllEndpoints() []Endpoint {
endpoints := []Endpoint{
{Path: s.GetAuthorizationServerMetadataURL(), Description: "OAuth metadata"},
{Path: s.GetProtectedResourceMetadataURL(), Description: "Resource metadata"},
{Path: s.GetOIDCDiscoveryURL(), Description: "OIDC discovery"},
+ {Path: fmt.Sprintf("%s/.well-known/jwks.json", strings.TrimRight(s.config.ServerURL, "/")), Description: "JWKS keys"},
}(Requires fmt/strings already imported as per prior comment.)
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // GetAllEndpoints returns all OAuth endpoints with descriptions | |
| func (s *Server) GetAllEndpoints() []Endpoint { | |
| endpoints := []Endpoint{ | |
| {Path: s.GetAuthorizationServerMetadataURL(), Description: "OAuth metadata"}, | |
| {Path: s.GetProtectedResourceMetadataURL(), Description: "Resource metadata"}, | |
| {Path: s.GetOIDCDiscoveryURL(), Description: "OIDC discovery"}, | |
| } | |
| if s.config.Mode == "proxy" { | |
| endpoints = append(endpoints, | |
| Endpoint{Path: s.GetAuthorizeURL(), Description: "Authorization endpoint"}, | |
| Endpoint{Path: s.GetCallbackURL(), Description: "OAuth callback"}, | |
| Endpoint{Path: s.GetTokenURL(), Description: "Token endpoint"}, | |
| Endpoint{Path: s.GetRegisterURL(), Description: "Client registration"}, | |
| ) | |
| } | |
| return endpoints | |
| } | |
| // GetAllEndpoints returns all OAuth endpoints with descriptions | |
| func (s *Server) GetAllEndpoints() []Endpoint { | |
| endpoints := []Endpoint{ | |
| {Path: s.GetAuthorizationServerMetadataURL(), Description: "OAuth metadata"}, | |
| {Path: s.GetProtectedResourceMetadataURL(), Description: "Resource metadata"}, | |
| {Path: s.GetOIDCDiscoveryURL(), Description: "OIDC discovery"}, | |
| {Path: fmt.Sprintf("%s/.well-known/jwks.json", strings.TrimRight(s.config.ServerURL, "/")), Description: "JWKS keys"}, | |
| } | |
| if s.config.Mode == "proxy" { | |
| endpoints = append(endpoints, | |
| Endpoint{Path: s.GetAuthorizeURL(), Description: "Authorization endpoint"}, | |
| Endpoint{Path: s.GetCallbackURL(), Description: "OAuth callback"}, | |
| Endpoint{Path: s.GetTokenURL(), Description: "Token endpoint"}, | |
| Endpoint{Path: s.GetRegisterURL(), Description: "Client registration"}, | |
| ) | |
| } | |
| return endpoints | |
| } |
🤖 Prompt for AI Agents
In oauth.go around lines 140 to 158, the JWKS endpoint (/.well-known/jwks.json)
is registered but not listed in GetAllEndpoints; update the initial endpoints
slice to include Endpoint{Path: s.GetJwksURL(), Description: "JWKS (JSON Web Key
Set)"} so the JWKS URL appears in discovery helpers (no change to import lines
required as fmt/strings are already present).
| return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||
| authHeader := r.Header.Get("Authorization") | ||
| if authHeader == "" || len(authHeader) < 7 || authHeader[:7] != "Bearer " { | ||
| s.logger.Info("OAuth: No bearer token provided, returning 401 with discovery info") | ||
|
|
||
| metadataURL := s.GetProtectedResourceMetadataURL() | ||
| w.Header().Add("WWW-Authenticate", `Bearer realm="OAuth", error="invalid_token", error_description="Missing or invalid access token"`) | ||
| w.Header().Add("WWW-Authenticate", fmt.Sprintf(`resource_metadata="%s"`, metadataURL)) | ||
| w.Header().Set("Content-Type", "application/json") | ||
| w.WriteHeader(http.StatusUnauthorized) | ||
|
|
||
| if err := json.NewEncoder(w).Encode(oauthErrorResponse{ | ||
| Error: "invalid_token", | ||
| ErrorDescription: "Missing or invalid access token", | ||
| }); err != nil { | ||
| s.logger.Error("Error encoding OAuth error response: %v", err) | ||
| } | ||
| return | ||
| } | ||
|
|
||
| contextFunc := CreateHTTPContextFunc() | ||
| ctx := contextFunc(r.Context(), r) | ||
| r = r.WithContext(ctx) | ||
|
|
||
| next.ServeHTTP(w, r) | ||
| }) | ||
| } |
There was a problem hiding this comment.
Harden Bearer parsing and headers; add no-store; use error_uri
- Accept case-insensitive “Bearer” per RFC 7235.
- Add Cache-Control: no-store on 401.
- Prefer a single WWW-Authenticate with error_uri (RFC 6750) pointing to metadata.
return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
authHeader := r.Header.Get("Authorization")
- if authHeader == "" || len(authHeader) < 7 || authHeader[:7] != "Bearer " {
+ parts := strings.SplitN(authHeader, " ", 2)
+ if authHeader == "" || len(parts) != 2 || !strings.EqualFold(parts[0], "Bearer") || strings.TrimSpace(parts[1]) == "" {
- s.logger.Info("OAuth: No bearer token provided, returning 401 with discovery info")
+ s.logger.Info("OAuth: No/invalid bearer token, returning 401 with discovery info")
- metadataURL := s.GetProtectedResourceMetadataURL()
- w.Header().Add("WWW-Authenticate", `Bearer realm="OAuth", error="invalid_token", error_description="Missing or invalid access token"`)
- w.Header().Add("WWW-Authenticate", fmt.Sprintf(`resource_metadata="%s"`, metadataURL))
+ metadataURL := s.GetProtectedResourceMetadataURL()
+ w.Header().Set("WWW-Authenticate",
+ fmt.Sprintf(`Bearer realm="OAuth", error="invalid_token", error_description="Missing or invalid access token", error_uri="%s"`, metadataURL))
+ w.Header().Set("Cache-Control", "no-store")
+ w.Header().Set("Pragma", "no-cache")
w.Header().Set("Content-Type", "application/json")
w.WriteHeader(http.StatusUnauthorized)Also add:
import (
"encoding/json"
"fmt"
"net/http"
+ "strings"🤖 Prompt for AI Agents
In oauth.go around lines 226 to 252, the bearer token parsing is fragile and
multiple WWW-Authenticate headers are emitted; update to accept case-insensitive
"Bearer" per RFC 7235 (e.g., use strings.HasPrefix on
strings.ToLower(authHeader) or strings.HasPrefix(strings.ToLower(authHeader),
"bearer ")), avoid fixed-length slicing for token checks, add Cache-Control:
no-store to the 401 response, and emit a single WWW-Authenticate header using
RFC 6750 form including realm, error, error_description and error_uri pointing
to s.GetProtectedResourceMetadataURL() (use Header().Set to ensure only one
header). Ensure Content-Type and WriteHeader remain, and log encoding errors
unchanged.
Signed-off-by: Tommy Nguyen <tuannvm@hotmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
api_test.go (2)
83-83: Consider validating oauthServer in additional tests.For consistency and to ensure the API contract (WithOAuth returns a non-nil server when err is nil), consider adding nil checks for
oauthServerin the other test cases that currently discard it, similar to the check inBasicUsage_NativeMode(lines 39-41).Example pattern to apply:
- _, oauthOption, err := WithOAuth(mux, cfg) + oauthServer, oauthOption, err := WithOAuth(mux, cfg) if err != nil { t.Fatalf("WithOAuth failed in proxy mode: %v", err) } + if oauthServer == nil { + t.Fatal("Expected OAuth server, got nil") + }Also applies to: 102-102, 124-124, 209-209, 235-235
364-386: Consider making endpoint count expectations more maintainable.The test uses hard-coded expected endpoint counts (3 for native, 7 for proxy). If endpoints are added or removed in the future, these tests will need manual updates.
Consider verifying specific endpoints by name/path or extracting the expected counts as named constants.
Example:
t.Run("GetAllEndpoints", func(t *testing.T) { endpoints := server.GetAllEndpoints() - if len(endpoints) != 3 { - t.Errorf("Expected 3 endpoints in native mode, got %d", len(endpoints)) + // Verify expected endpoints are present + expectedNative := []string{ + "/.well-known/oauth-authorization-server", + "/.well-known/oauth-protected-resource", + "/.well-known/openid-configuration", + } + if len(endpoints) != len(expectedNative) { + t.Errorf("Expected %d endpoints in native mode, got %d", len(expectedNative), len(endpoints)) }config.go (1)
261-268: Consider omitting standard ports for cleaner URLs.The function always includes the port in the URL, which produces non-canonical URLs for standard ports (e.g.,
https://example.com:443instead ofhttps://example.com). This could cause issues with OAuth redirect URI validation and URL matching.Apply this diff to omit standard ports:
func AutoDetectServerURL(host, port string, useTLS bool) string { scheme := "http" + standardPort := "80" if useTLS { scheme = "https" + standardPort = "443" } - return fmt.Sprintf("%s://%s:%s", scheme, host, port) + if port == standardPort { + return fmt.Sprintf("%s://%s", scheme, host) + } + return fmt.Sprintf("%s://%s:%s", scheme, host, port) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
api_test.go(8 hunks)config.go(1 hunks)config_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- config_test.go
🧰 Additional context used
🧬 Code graph analysis (2)
api_test.go (2)
oauth.go (2)
WithOAuth(282-291)NewServer(40-73)config.go (1)
Config(10-34)
config.go (1)
provider/provider.go (2)
Config(32-38)Logger(24-29)
🔇 Additional comments (4)
config.go (4)
154-170: LGTM! Clean builder pattern implementation.The ConfigBuilder struct and constructor follow the fluent builder pattern well. Default values (localhost:8080, HTTP) are sensible for development. The separation of host/port/useTLS from ServerURL enables flexible configuration while maintaining clarity.
172-248: LGTM! Fluent setters follow idiomatic patterns.All setter methods correctly implement the fluent builder pattern by returning
*ConfigBuilderfor method chaining. Good choice to accept[]byteforWithJWTSecretto avoid unnecessary string conversions and maintain security best practices.
250-259: LGTM! Build method correctly validates configuration.The method properly auto-detects ServerURL when not explicitly set and validates the resulting configuration. The validation ensures all required fields are present based on mode and provider.
270-294: LGTM! FromEnv provides clean environment-driven configuration.The function properly reads environment variables and uses the builder pattern for configuration. The auto-detection logic for TLS (checking for cert/key files) and ServerURL is sensible. Validation happens in
Build(), which will catch any missing required fields.
| t.Run("ExtractsTokenWithBearer", func(t *testing.T) { | ||
| cfg := &Config{ | ||
| Provider: "hmac", | ||
| Issuer: "https://test.example.com", | ||
| Audience: "api://test", | ||
| ServerURL: "https://test-server.com", | ||
| JWTSecret: []byte("test-secret-key-must-be-32-bytes-long!"), | ||
| } | ||
|
|
||
| server, err := NewServer(cfg) | ||
| if err != nil { | ||
| t.Fatalf("NewServer failed: %v", err) | ||
| } | ||
|
|
||
| handler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||
| w.WriteHeader(http.StatusOK) | ||
| }) | ||
|
|
||
| wrappedHandler := server.WrapHandler(handler) | ||
|
|
||
| req := httptest.NewRequest("GET", "/protected", nil) | ||
| req.Header.Set("Authorization", "Bearer some-token") | ||
| w := httptest.NewRecorder() | ||
|
|
||
| wrappedHandler.ServeHTTP(w, req) | ||
|
|
||
| if w.Code == http.StatusUnauthorized { | ||
| t.Log("Token validation works") | ||
| } | ||
| }) |
There was a problem hiding this comment.
Clarify or enhance the ExtractsTokenWithBearer test.
The test passes an invalid token ("some-token") which will fail validation and return 401. The test then logs "Token validation works" but doesn't verify that token extraction actually occurred.
Consider one of these approaches:
- Use a valid JWT token (similar to lines 160-169 in the EndToEndWithHTTPContextFunc test) to verify successful extraction and handler execution
- Rename the test to reflect what it actually tests (e.g.,
"Returns401WithInvalidToken") - Add assertions to verify the token was extracted into the request context before validation failed
Example for approach 1:
t.Run("ExtractsTokenWithBearer", func(t *testing.T) {
cfg := &Config{
Provider: "hmac",
Issuer: "https://test.example.com",
Audience: "api://test",
ServerURL: "https://test-server.com",
JWTSecret: []byte("test-secret-key-must-be-32-bytes-long!"),
}
server, err := NewServer(cfg)
if err != nil {
t.Fatalf("NewServer failed: %v", err)
}
+ // Create a valid JWT token
+ token := jwt.NewWithClaims(jwt.SigningMethodHS256, jwt.MapClaims{
+ "sub": "test-user",
+ "aud": cfg.Audience,
+ "exp": time.Now().Add(time.Hour).Unix(),
+ "iat": time.Now().Unix(),
+ })
+ tokenString, _ := token.SignedString(cfg.JWTSecret)
+
handler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+ // Verify token was extracted to context
+ if token, ok := GetOAuthToken(r.Context()); !ok || token != tokenString {
+ t.Error("Token not properly extracted to context")
+ }
w.WriteHeader(http.StatusOK)
})
wrappedHandler := server.WrapHandler(handler)
req := httptest.NewRequest("GET", "/protected", nil)
- req.Header.Set("Authorization", "Bearer some-token")
+ req.Header.Set("Authorization", "Bearer "+tokenString)
w := httptest.NewRecorder()
wrappedHandler.ServeHTTP(w, req)
- if w.Code == http.StatusUnauthorized {
- t.Log("Token validation works")
+ if w.Code != http.StatusOK {
+ t.Errorf("Expected 200 OK, got %d", w.Code)
}
})Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In api_test.go around lines 294 to 323, the test "ExtractsTokenWithBearer" sends
an invalid token ("some-token") and only logs success on a 401 rather than
asserting token extraction or handler execution; change the test to either (A)
generate and use a valid JWT (mirror the creation used in
EndToEndWithHTTPContextFunc around lines 160-169), set that token as the Bearer
header, call wrappedHandler and assert the response is 200 and the handler ran,
or (B) if you intend to test invalid tokens, rename the test to
"Returns401WithInvalidToken" and add an explicit assertion that the response
code is 401 (no logging), or (C) add assertions that the token value was placed
into the request context before validation failed (mock or inspect the
middleware behavior) so the test actually verifies extraction; implement one of
these fixes and remove the ambiguous t.Log usage.
Signed-off-by: Tommy Nguyen <tuannvm@hotmail.com>
Signed-off-by: Tommy Nguyen <tuannvm@hotmail.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
examples/advanced/main.go (4)
82-89: Return MCP error results instead of Go errors in tool handlersBetter align with mcp-go handler pattern by returning an MCP error result (so clients receive structured error payloads) rather than a non-nil Go error.
- user, ok := oauth.GetUserFromContext(ctx) - if !ok { - return nil, fmt.Errorf("authentication required") - } + user, ok := oauth.GetUserFromContext(ctx) + if !ok { + return mcp.NewToolResultError("authentication required"), nil + }Based on learnings.
50-58: Optional: tighten CORS and add Vary headerFor production, avoid wildcard origins and set Vary: Origin so proxies don’t cache a response across origins. Consider making allowed origin configurable via env.
- w.Header().Set("Access-Control-Allow-Origin", "*") + origin := r.Header.Get("Origin") + allowed := getEnv("CORS_ALLOWED_ORIGIN", "") + if allowed != "" && origin == allowed { + w.Header().Set("Access-Control-Allow-Origin", origin) + } else { + w.Header().Set("Access-Control-Allow-Origin", "*") // safe default for example + } + w.Header().Set("Vary", "Origin") - w.Header().Set("Access-Control-Allow-Methods", "GET, POST, OPTIONS") - w.Header().Set("Access-Control-Allow-Headers", "Content-Type, Authorization") + w.Header().Set("Access-Control-Allow-Methods", "GET, POST, OPTIONS") + w.Header().Set("Access-Control-Allow-Headers", "Accept, Content-Type, Authorization")If the client sends cookies, also set Access-Control-Allow-Credentials: true and avoid “*”.
105-115: Recommended: use http.Server with timeoutsAdd sensible timeouts to mitigate slowloris and resource exhaustion. Example:
- if useTLS { - certFile := getEnv("HTTPS_CERT_FILE", "") - keyFile := getEnv("HTTPS_KEY_FILE", "") - if err := http.ListenAndServeTLS(addr, certFile, keyFile, mux); err != nil { - log.Fatalf("HTTPS server failed: %v", err) - } - } else { - if err := http.ListenAndServe(addr, mux); err != nil { - log.Fatalf("HTTP server failed: %v", err) - } - } + srv := &http.Server{ + Addr: addr, + Handler: mux, + ReadHeaderTimeout: 10 * time.Second, + ReadTimeout: 30 * time.Second, + WriteTimeout: 60 * time.Second, + IdleTimeout: 120 * time.Second, + MaxHeaderBytes: 1 << 20, // 1MB + } + if useTLS { + certFile := getEnv("HTTPS_CERT_FILE", "") + keyFile := getEnv("HTTPS_KEY_FILE", "") + if err := srv.ListenAndServeTLS(certFile, keyFile); err != nil { + log.Fatalf("HTTPS server failed: %v", err) + } + } else { + if err := srv.ListenAndServe(); err != nil { + log.Fatalf("HTTP server failed: %v", err) + } + }Remember to add: import "time".
76-81: Optional: define tool via mcp.NewTool for typed args/schemaUsing mcp.NewTool yields better metadata and JSON schema generation for clients.
- mcpServer.AddTool( - mcp.Tool{ - Name: "get_user_info", - Description: "Returns authenticated user information", - }, + mcpServer.AddTool( + mcp.NewTool( + "get_user_info", + mcp.WithDescription("Returns authenticated user information"), + ), func(ctx context.Context, req mcp.CallToolRequest) (*mcp.CallToolResult, error) {Based on learnings.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/test.yml(1 hunks)examples/advanced/main.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
examples/advanced/main.go (3)
config.go (1)
NewConfigBuilder(164-170)oauth.go (1)
WithOAuth(282-291)middleware.go (1)
GetUserFromContext(199-202)
🔇 Additional comments (1)
examples/advanced/main.go (1)
42-46: No changes required — option name is correct.The mcp-go v0.41.1 server package correctly exposes
WithStateLess(capital L) as shown in the code. The concern raised in the review comment is unfounded; the code is already using the correct option name.
| go build ./examples/simple/main.go | ||
| go build ./examples/advanced/main.go |
There was a problem hiding this comment.
Build examples by package, not by single file, to avoid missing files in multi-file packages
Using go build on specific files skips other files in the same package. If examples grow beyond a single file, builds can fail unexpectedly. Build the package directories instead.
- go build ./examples/simple/main.go
- go build ./examples/advanced/main.go
+ go build ./examples/simple
+ go build ./examples/advanced📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| go build ./examples/simple/main.go | |
| go build ./examples/advanced/main.go | |
| go build ./examples/simple | |
| go build ./examples/advanced |
🤖 Prompt for AI Agents
.github/workflows/test.yml around lines 51-52: the workflow currently runs "go
build" on individual example source files which will omit other files in the
same package; change the steps to run "go build" on the package directories
(e.g., ./examples/simple and ./examples/advanced) so the entire package is built
and additional files are included.
| WithHost(getEnv("MCP_HOST", "localhost")). | ||
| WithPort(getEnv("MCP_PORT", "8080")). | ||
| WithTLS(getEnv("HTTPS_CERT_FILE", "") != ""). | ||
| Build() |
There was a problem hiding this comment.
Unify TLS detection to prevent HTTPS/HTTP mismatch (wrong ServerURL/callbacks)
TLS is computed inconsistently:
- Config uses cert-only check.
- Status endpoint uses cert-only check.
- Server start uses cert AND key.
This can produce https:// in cfg.ServerURL while the server actually runs over HTTP, breaking OAuth redirects.
Apply these changes to derive TLS once and use cfg.UseTLS everywhere:
- WithTLS(getEnv("HTTPS_CERT_FILE", "") != "").
+ WithTLS(getEnv("HTTPS_CERT_FILE", "") != "" && getEnv("HTTPS_KEY_FILE", "") != "").- useTLS := getEnv("HTTPS_CERT_FILE", "") != ""
- _, _ = fmt.Fprintf(w, `{"status":"ok","oauth":"%s"}`, oauthServer.GetStatusString(useTLS))
+ useTLS := cfg.UseTLS
+ _, _ = fmt.Fprintf(w, `{"status":"ok","oauth":"%s"}`, oauthServer.GetStatusString(useTLS))- useTLS := getEnv("HTTPS_CERT_FILE", "") != "" && getEnv("HTTPS_KEY_FILE", "") != ""
- oauthServer.LogStartup(useTLS)
+ useTLS := cfg.UseTLS
+ oauthServer.LogStartup(useTLS)
@@
- if useTLS {
+ if useTLS {
certFile := getEnv("HTTPS_CERT_FILE", "")
keyFile := getEnv("HTTPS_KEY_FILE", "")
if err := http.ListenAndServeTLS(addr, certFile, keyFile, mux); err != nil {
log.Fatalf("HTTPS server failed: %v", err)
}
} else {
if err := http.ListenAndServe(addr, mux); err != nil {
log.Fatalf("HTTP server failed: %v", err)
}
}If Config doesn’t expose UseTLS, compute once up top and pass the same boolean into both WithTLS and later uses.
Also applies to: 71-73, 93-110
🤖 Prompt for AI Agents
In examples/advanced/main.go around lines 21-24 (and also apply similar changes
at 71-73 and 93-110), TLS/HTTPS is being detected inconsistently (cert-only in
some places, cert+key in others) which can yield a cfg.ServerURL with https://
while the server actually runs over HTTP; compute a single boolean UseTLS once
at the top (e.g. useCert := getEnv("HTTPS_CERT_FILE","") != "" and useKey :=
getEnv("HTTPS_KEY_FILE","") != "" then UseTLS := useCert && useKey or, if Config
exposes UseTLS, set that field), pass that same UseTLS into WithTLS(...) when
building the config and reuse it everywhere the server start, status endpoint,
and callback URL generation need TLS so all code reads the same boolean instead
of re-checking files in multiple ways.
Summary by CodeRabbit
New Features
Documentation
Tests