Skip to content

Conversation

@tuannvm
Copy link
Owner

@tuannvm tuannvm commented Nov 3, 2025

Summary by CodeRabbit

  • New Features

    • MCP endpoints can be wrapped to enforce OAuth Bearer validation with automatic 401 responses and CORS-friendly OPTIONS passthrough.
  • Improvements

    • Strict, case-insensitive Bearer token parsing and clearer 401 error types for missing, malformed, or invalid tokens.
    • Token and user info are propagated to downstream handlers after validation.
  • Documentation

    • Examples and docs updated to show the new wrapping usage for MCP endpoints.

Signed-off-by: Tommy Nguyen <tuannvm@hotmail.com>
@coderabbitai
Copy link

coderabbitai bot commented Nov 3, 2025

Walkthrough

Adds a new MCP-specific OAuth wrapper and 401 helpers, tightens Bearer-token parsing and CORS preflight handling, and updates example apps to use the new wrapper signature and call sites.

Changes

Cohort / File(s) Change Summary
OAuth API & helpers
oauth.go
Added WrapMCPEndpoint(handler http.Handler) http.HandlerFunc, Return401, and Return401InvalidToken; added strings import; implemented standardized 401 JSON responses with WWW-Authenticate and resource metadata URL.
MCP OAuth handling
mcp/oauth.go
OPTIONS (CORS) passthrough; case-insensitive Bearer detection; enforces Bearer scheme, rejects malformed/non-Bearer auth appropriately; extracts token, injects token/user into request context; returns OAuth-specific 401s via server helpers.
Middleware token extraction tweaks
middleware.go
Made Authorization header prefix check case-insensitive, adjusted token extraction logic and logging for malformed headers/token lengths; no public signature changes.
Examples wiring updates
Advanced <br> examples/mark3labs/advanced/main.go
Simple <br> examples/mark3labs/simple/main.go
Capture oauthServer from WithOAuth and switch MCP registration to oauthServer.WrapMCPEndpoint(...) (now accepts http.Handler and returns http.HandlerFunc); update comments to mention automatic 401 handling with CORS support.
Docs / examples in package
mark3labs/oauth.go
Expanded WithOAuth documentation and examples to demonstrate streamable server usage and WrapMCPEndpoint pattern.

Sequence Diagram(s)

sequenceDiagram
    autonumber
    participant Client
    participant Wrapper as WrapMCPEndpoint
    participant OAuth as OAuth Server
    participant MCP as MCP Handler

    Client->>Wrapper: OPTIONS /mcp (CORS preflight)
    Wrapper-->>Client: 200 OK (passthrough)

    Client->>Wrapper: POST /mcp (no Authorization)
    Wrapper->>OAuth: no header -> Return401
    OAuth-->>Wrapper: 401 response
    Wrapper-->>Client: 401 + WWW-Authenticate (invalid_request)

    Client->>Wrapper: POST /mcp (Authorization: Basic ...)
    Wrapper->>OAuth: non-Bearer scheme -> treat as passthrough
    Wrapper->>MCP: forward request unchanged
    MCP-->>Client: handler response

    Client->>Wrapper: POST /mcp (Authorization: Bearer token)
    Wrapper->>OAuth: parse & validate token
    alt token valid
        OAuth-->>Wrapper: inject token/user into context
        Wrapper->>MCP: call handler with enriched context
        MCP-->>Client: 200 OK
    else token invalid
        OAuth-->>Wrapper: Return401InvalidToken
        Wrapper-->>Client: 401 + WWW-Authenticate (invalid_token)
    end
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

  • Review token parsing and header case-insensitive handling in mcp/oauth.go and middleware.go.
  • Verify 401 helper responses (WWW-Authenticate header format, JSON payload, resource metadata URL) in oauth.go.
  • Confirm examples updated to the new WrapMCPEndpoint signature and that call sites use http.Handler correctly.

Possibly related PRs

  • Feat/generic sdk #4 — Refactors OAuth/MCP wiring and token handling; overlaps with MCP wrapper and auth flow changes.
  • Refactor/user friendliness #2 — API refactor that introduced similar Server/WithOAuth and endpoint-wrapping patterns adjusted here.
  • Feat/init #1 — Initial MCP endpoint wrapping patterns and handler signatures that this PR modifies.

Poem

🐰 I hopped the MCP gate with care,
I sniffed the header, checked the Bearer.
I let the good ones skip along,
Sent 401s when things were wrong.
A rabbit's hop, secure and fair. 🥕

Pre-merge checks and finishing touches

✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the main change: adding WrapMCPEndpoint method for automatic 401 handling in OAuth, which is the primary feature across multiple files.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch feat/handle-401

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

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Found a spec gap in the new 401 helper.


w.Header().Add("WWW-Authenticate", `Bearer realm="OAuth", error="invalid_request", error_description="Bearer token required"`)
w.Header().Add("WWW-Authenticate", fmt.Sprintf(`resource_metadata="%s"`, metadataURL))
w.Header().Set("Content-Type", "application/json")
Copy link

Choose a reason for hiding this comment

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

⚠️ Logic This second WWW-Authenticate header omits the auth scheme, so clients treat it as invalid and ignore the resource_metadata hint. Please include it in the Bearer header (and do the same in Return401InvalidToken) so OAuth discovery works.

Copy link

@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 33bddcf and 5bf4fba.

📒 Files selected for processing (5)
  • examples/mark3labs/advanced/main.go (2 hunks)
  • examples/mark3labs/simple/main.go (2 hunks)
  • mark3labs/oauth.go (1 hunks)
  • mcp/oauth.go (3 hunks)
  • oauth.go (2 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
mcp/oauth.go (1)
context.go (2)
  • WithOAuthToken (14-16)
  • WithUser (25-27)
examples/mark3labs/simple/main.go (3)
mark3labs/oauth.go (1)
  • WithOAuth (43-52)
oauth.go (1)
  • WithOAuth (436-445)
middleware.go (1)
  • CreateHTTPContextFunc (120-139)
oauth.go (1)
middleware.go (1)
  • CreateHTTPContextFunc (120-139)
⏰ 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: code-review
🔇 Additional comments (7)
mark3labs/oauth.go (1)

26-39: LGTM! Clear documentation updates.

The documentation improvements effectively guide users on how to use the new WrapMCPEndpoint method with proper example code and method references.

examples/mark3labs/simple/main.go (1)

25-60: LGTM! Correct usage of the new wrapper pattern.

The example properly demonstrates the use of WrapMCPEndpoint for automatic 401 handling, correctly capturing the oauthServer instance and wrapping the MCP endpoint handler.

mcp/oauth.go (2)

106-118: LGTM! Token validation and context setup are correct.

The token validation logic properly validates the token, handles errors with appropriate 401 responses, and correctly adds both the token and user to the request context for downstream handlers.


68-98: Critical: Case-sensitivity mismatch in Bearer token extraction.

The code performs case-insensitive Bearer token detection (line 70, 79) but uses case-sensitive extraction (line 93-98). This creates a critical bug:

  1. Line 70 converts header to lowercase: authLower := strings.ToLower(authHeader)
  2. Line 79 checks using lowercase: if strings.HasPrefix(authLower, "bearer")
  3. Line 93 defines prefix as capitalized: const bearerPrefix = "Bearer "
  4. Line 98 extracts using original header: token := authHeader[len(bearerPrefix):]

Problem: If a client sends "bearer <token>" (lowercase), the prefix check passes, but the slice operation uses the wrong offset length (7 characters for "Bearer " vs actual lowercase "bearer " in the header), resulting in incorrect token extraction.

Impact: OAuth 2.0 RFC 6750 requires case-insensitive scheme recognition ("bearer" and "Bearer" must be treated identically), so this bug will cause authentication failures for compliant clients using lowercase.

Apply this fix:

 	// Check Authorization header
 	authHeader := r.Header.Get("Authorization")
-	authLower := strings.ToLower(authHeader)
 
 	// Return 401 if Bearer token missing
 	if authHeader == "" {
 		oauthServer.Return401(w)
 		return
 	}
 
-	// Check if it's a Bearer token (case-insensitive per OAuth 2.0 spec)
-	if strings.HasPrefix(authLower, "bearer") {
-		// Malformed Bearer token (no space after "Bearer")
-		if !strings.HasPrefix(authLower, "bearer ") {
-			oauthServer.Return401InvalidToken(w)
-			return
-		}
-		// Valid Bearer format, continue to validation
-	} else {
+	// Check if it's a Bearer token (case-insensitive per RFC 6750)
+	if len(authHeader) < 7 || !strings.EqualFold(authHeader[:6], "bearer") {
 		// Pass through non-Bearer schemes (e.g., Basic auth)
 		mcpHandler.ServeHTTP(w, r)
 		return
 	}
 
-	// Extract and validate token (safe slice operation)
-	const bearerPrefix = "Bearer "
-	if len(authHeader) < len(bearerPrefix)+1 {
+	// Check for space after Bearer
+	if authHeader[6] != ' ' {
 		oauthServer.Return401InvalidToken(w)
 		return
 	}
-	token := authHeader[len(bearerPrefix):]
+	
+	// Extract token (skip "Bearer " or "bearer " - 7 chars)
+	token := authHeader[7:]
 
 	// Validate token is not empty
 	if token == "" {
 		oauthServer.Return401InvalidToken(w)
 		return
 	}

Likely an incorrect or invalid review comment.

oauth.go (2)

374-412: LGTM! RFC 6750-compliant 401 response helpers.

The Return401 and Return401InvalidToken methods correctly implement OAuth 2.0 Bearer token error responses per RFC 6750 §3.1:

  • invalid_request for missing tokens
  • invalid_token for malformed/expired tokens
  • Proper WWW-Authenticate headers with realm, error, and error_description
  • Resource metadata URL for OAuth discovery

316-330: Verify: Documentation of token validation responsibility is correct, but could be clearer.

The token validation split is intentional and working as designed:

  • WrapMCPEndpoint extracts the Bearer token to context and passes through OPTIONS/non-Bearer requests
  • The Middleware() function validates tokens from context via ValidateTokenCached() with caching
  • This pattern supports multiple integration frameworks (mark3labs, mcp-go, etc.)

The inline comment at line 357 correctly documents this, and the middleware integration tests confirm the pattern works. However, the function docstring (lines 316-330) focuses on behavior rather than explaining the validation responsibility split.

Suggestion: Add one sentence to the docstring clarifying that token validation occurs in Middleware(), or reference it directly:

// Token validation happens in the configured tool handler middleware (see Middleware()).
// If wrapping without middleware, validation must occur in the upstream HTTP layer.

This makes the responsibility division explicit without requiring readers to find the inline comment at line 357.

examples/mark3labs/advanced/main.go (1)

53-67: LGTM! Correct usage of WrapMCPEndpoint.

The example properly demonstrates wrapping the MCP endpoint handler with the new WrapMCPEndpoint method, including the necessary http.HandlerFunc cast for the function-typed handler.

Signed-off-by: Tommy Nguyen <tuannvm@hotmail.com>
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Security regression: non-Bearer auth bypasses OAuth checks.

} else {
// Pass through non-Bearer schemes (e.g., Basic auth)
mcpHandler.ServeHTTP(w, r)
return
Copy link

Choose a reason for hiding this comment

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

🔒 Letting non-Bearer Authorization headers fall through means anyone can send Authorization: Basic ... and reach the MCP handler without an OAuth token, undoing the previous 401 guard and creating an auth bypass. Please keep rejecting any scheme that isn't a properly formatted Bearer token before invoking the handler.

} else {
// Pass through non-Bearer schemes (e.g., Basic auth)
handler.ServeHTTP(w, r)
return
Copy link

Choose a reason for hiding this comment

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

🔒 Passing through non-Bearer Authorization headers here lets a client hit the MCP endpoint with Authorization: Basic ... and skip OAuth entirely, which regressed from the prior 401 guard. Please continue to reject any non-Bearer scheme before calling the wrapped handler.

Copy link

@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: 2

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (3)
middleware.go (1)

122-139: Don’t log Authorization header contents; guard empty tokens.

Logging header previews can leak credentials; also avoid setting empty tokens into context.

-		authLower := strings.ToLower(authHeader)
-
-		if strings.HasPrefix(authLower, "bearer ") {
-			// Extract token (skip "Bearer " or "bearer " prefix)
-			token := authHeader[7:]
-			// Clean any whitespace
-			token = strings.TrimSpace(token)
-			ctx = WithOAuthToken(ctx, token)
-			log.Printf("OAuth: Token extracted from request (length: %d)", len(token))
-		} else if authHeader != "" {
-			preview := authHeader
-			if len(authHeader) > 30 {
-				preview = authHeader[:30] + "..."
-			}
-			log.Printf("OAuth: Invalid Authorization header format: %s", preview)
-		}
+		// Robust, case-insensitive scheme and 1+ SP handling
+		fields := strings.Fields(authHeader)
+		if len(fields) >= 2 && strings.EqualFold(fields[0], "Bearer") {
+			token := strings.TrimSpace(fields[1])
+			if token == "" {
+				log.Printf("OAuth: Empty Bearer token")
+				return ctx
+			}
+			ctx = WithOAuthToken(ctx, token)
+			log.Printf("OAuth: Token extracted from request (length: %d)", len(token))
+		} else if authHeader != "" {
+			// Log only the scheme, never the credentials
+			scheme := fields[0]
+			log.Printf("OAuth: Non-Bearer Authorization scheme received: %s", scheme)
+		}
oauth.go (2)

264-286: Fix case sensitivity and header formatting in WrapHandler; delegate to Return401/InvalidToken.

Current code is case-sensitive ("Bearer ") and sends a second WWW-Authenticate without a scheme. Use EqualFold + 1+ SP and reuse Return401 helpers.

-		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
-		}
-
-		token := authHeader[7:]
+		authHeader := r.Header.Get("Authorization")
+		if authHeader == "" {
+			s.Return401(w)
+			return
+		}
+		fields := strings.Fields(authHeader)
+		if len(fields) < 2 || !strings.EqualFold(fields[0], "Bearer") {
+			s.Return401(w)
+			return
+		}
+		token := strings.TrimSpace(fields[1])
+		if token == "" {
+			s.Return401InvalidToken(w)
+			return
+		}

269-292: Fix WWW-Authenticate header formatting to comply with RFC 6750.

Lines 269–270 and 290–291 use multiple Add() calls for WWW-Authenticate headers, where the second header lacks the Bearer scheme. According to RFC 6750, all parameters must be combined in a single Bearer header. Use Set() instead of Add() and consolidate all parameters into one header, following the pattern at lines 383–385 and 405–407.

  • Lines 269–270: Combine into single Set() call with Bearer scheme and all parameters
  • Lines 290–291: Combine into single Set() call with Bearer scheme and all parameters
♻️ Duplicate comments (2)
oauth.go (2)

250-308: WrapHandler still sends a second WWW-Authenticate without a scheme.

This repeats the earlier bug; switch to a single Bearer header with resource_metadata (or call Return401/Return401InvalidToken).

Apply the diff above (Lines 264-286) to resolve.


316-372: WrapMCPEndpoint: normalize Bearer parsing and avoid CreateHTTPContextFunc path.

Parse with Fields/EqualFold, reject empty token, and set context directly to avoid extra logging and duplicate parsing.

-		// Check Authorization header
-		authHeader := r.Header.Get("Authorization")
-		authLower := strings.ToLower(authHeader)
+		// Check Authorization header
+		authHeader := r.Header.Get("Authorization")
@@
-		// Check if it's a Bearer token (case-insensitive per OAuth 2.0 spec)
-		if strings.HasPrefix(authLower, "bearer") {
-			// Malformed Bearer token (no space after "Bearer")
-			if !strings.HasPrefix(authLower, "bearer ") {
-				s.Return401InvalidToken(w)
-				return
-			}
-			// Valid Bearer format, extract to context
-			// (validation happens in downstream middleware)
-		} else {
+		// Distinguish Bearer vs non-Bearer (case-insensitive, 1+ SP tolerant)
+		fields := strings.Fields(authHeader)
+		if len(fields) < 2 || !strings.EqualFold(fields[0], "Bearer") {
 			// Pass through non-Bearer schemes (e.g., Basic auth)
 			handler.ServeHTTP(w, r)
 			return
 		}
@@
-		// Extract token to context
-		contextFunc := CreateHTTPContextFunc()
-		ctx := contextFunc(r.Context(), r)
-		r = r.WithContext(ctx)
+		// Extract token to context
+		token := strings.TrimSpace(fields[1])
+		if token == "" {
+			s.Return401InvalidToken(w)
+			return
+		}
+		ctx := WithOAuthToken(r.Context(), token)
+		r = r.WithContext(ctx)
🧹 Nitpick comments (6)
middleware.go (2)

41-55: Avoid duplicate SHA-256 and reuse computed hash.

You compute the token hash twice. Reuse the first value.

-			// Log token hash for debugging (prevents sensitive data exposure)
-			tokenHashFull := fmt.Sprintf("%x", sha256.Sum256([]byte(tokenString)))
-			tokenHashPreview := tokenHashFull[:16] + "..."
+			// Log token hash preview (prevents sensitive data exposure)
+			tokenHashPreview := tokenHash[:16] + "..."

63-66: Extract TTL into a shared constant.

The 5-minute TTL is hard-coded here and elsewhere. Define a package-level const to keep it consistent.

+// consider placing once per package (e.g., in oauth.go) and reusing:
+const tokenCacheTTL = 5 * time.Minute
-			expiresAt := time.Now().Add(5 * time.Minute)
+			expiresAt := time.Now().Add(tokenCacheTTL)
oauth.go (4)

380-394: Add no-store headers to 401 responses.

Prevent caching of error responses as recommended by OAuth best practices.

 	w.Header().Set("WWW-Authenticate", fmt.Sprintf(
 		`Bearer realm="OAuth", error="invalid_request", error_description="Bearer token required", resource_metadata="%s"`,
 		metadataURL))
+	w.Header().Set("Cache-Control", "no-store")
+	w.Header().Set("Pragma", "no-cache")

402-416: Add no-store headers to invalid_token responses.

 	w.Header().Set("WWW-Authenticate", fmt.Sprintf(
 		`Bearer realm="OAuth", error="invalid_token", error_description="Authentication failed", resource_metadata="%s"`,
 		metadataURL))
+	w.Header().Set("Cache-Control", "no-store")
+	w.Header().Set("Pragma", "no-cache")

239-243: Expose HTTPContextFunc via options is good. Consider unifying parsing.

Bearer parsing exists in three places. Extract a shared helper (e.g., parseBearerToken) to avoid drift.


121-134: Minor: consistent TTL constant.

Same 5-minute TTL here as elsewhere; consider using a shared const.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 5bf4fba and 7131d3b.

📒 Files selected for processing (3)
  • mcp/oauth.go (3 hunks)
  • middleware.go (1 hunks)
  • oauth.go (2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
oauth.go (1)
middleware.go (1)
  • CreateHTTPContextFunc (120-142)
mcp/oauth.go (1)
context.go (2)
  • WithOAuthToken (14-16)
  • WithUser (25-27)
⏰ 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: code-review
🔇 Additional comments (3)
mcp/oauth.go (2)

62-67: OPTIONS passthrough LGTM.


115-121: Context injection LGTM.

middleware.go (1)

45-49: Token cache concurrency safety verified—no issues found.

The TokenCache implementation properly uses sync.RWMutex with correct locking patterns:

  • getCachedToken acquires RLock() and releases it in all code paths (lines 28, 33, 37)
  • setCachedToken and deleteExpiredToken use Lock() with defer for write operations
  • Goroutine launch in getCachedToken is safe (lock released before launching; spawned function acquires its own lock)

No concurrency safety issues or improper locking detected.

Comment on lines 72 to 91
// Return 401 if Bearer token missing
if authHeader == "" {
oauthServer.Return401(w)
return
}

// Check if it's a Bearer token (case-insensitive per OAuth 2.0 spec)
if strings.HasPrefix(authLower, "bearer") {
// Malformed Bearer token (no space after "Bearer")
if !strings.HasPrefix(authLower, "bearer ") {
oauthServer.Return401InvalidToken(w)
return
}
// Valid Bearer format, continue to validation
} else {
// Pass through non-Bearer schemes (e.g., Basic auth)
mcpHandler.ServeHTTP(w, r)
return
}

Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion | 🟠 Major

Make Bearer parsing tolerant (case-insensitive, 1+ spaces).

Avoid strict lowercase/prefix + single-space checks. Use Fields/EqualFold; return invalid_token if malformed.

-		// Check Authorization header
-		authHeader := r.Header.Get("Authorization")
-		authLower := strings.ToLower(authHeader)
+		// Check Authorization header
+		authHeader := r.Header.Get("Authorization")
@@
-		// Check if it's a Bearer token (case-insensitive per OAuth 2.0 spec)
-		if strings.HasPrefix(authLower, "bearer") {
-			// Malformed Bearer token (no space after "Bearer")
-			if !strings.HasPrefix(authLower, "bearer ") {
-				oauthServer.Return401InvalidToken(w)
-				return
-			}
-			// Valid Bearer format, continue to validation
-		} else {
+		// Distinguish Bearer vs non-Bearer (case-insensitive, tolerate 1+ SP)
+		fields := strings.Fields(authHeader)
+		if len(fields) < 2 || !strings.EqualFold(fields[0], "Bearer") {
 			// Pass through non-Bearer schemes (e.g., Basic auth)
 			mcpHandler.ServeHTTP(w, r)
 			return
 		}

Committable suggestion skipped: line range outside the PR's diff.

🤖 Prompt for AI Agents
mcp/oauth.go around lines 72 to 91: the current logic uses lowercase prefix
checks and a strict single-space to detect Bearer tokens; change to parse the
Authorization header by splitting into fields (or SplitN by spaces), perform a
case-insensitive comparison (strings.EqualFold) of the scheme to "Bearer", allow
one or more spaces between scheme and token, and if the scheme is "Bearer" but
there is no token (malformed) call oauthServer.Return401InvalidToken(w); if the
scheme is not Bearer, pass through to mcpHandler.ServeHTTP(w, r).

Signed-off-by: Tommy Nguyen <tuannvm@hotmail.com>
Copy link

@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: 0

🧹 Nitpick comments (2)
mcp/oauth.go (1)

68-106: Consider using strings.Fields for more robust Bearer token parsing.

The current implementation works but involves multiple string manipulations and length checks. Using strings.Fields() or strings.SplitN() would simplify the logic and handle edge cases (multiple spaces, tabs) more cleanly.

Example refactor:

-		// Check Authorization header
-		authHeader := r.Header.Get("Authorization")
-		authLower := strings.ToLower(authHeader)
-
-		// Return 401 if Bearer token missing
-		if authHeader == "" {
-			oauthServer.Return401(w)
-			return
-		}
-
-		// Check if it's a Bearer token (case-insensitive per OAuth 2.0 spec)
-		if !strings.HasPrefix(authLower, "bearer") {
-			// Reject non-Bearer schemes (OAuth endpoints require Bearer tokens only)
-			oauthServer.Return401(w)
-			return
-		}
-
-		// Malformed Bearer token (no space after "Bearer")
-		if !strings.HasPrefix(authLower, "bearer ") {
-			oauthServer.Return401InvalidToken(w)
-			return
-		}
-
-		// Extract and validate token (safe slice operation)
-		const bearerPrefix = "Bearer "
-		if len(authHeader) < len(bearerPrefix)+1 {
-			oauthServer.Return401InvalidToken(w)
-			return
-		}
-		token := authHeader[len(bearerPrefix):]
-
-		// Clean any whitespace (e.g., "Bearer token ")
-		token = strings.TrimSpace(token)
-
-		// Validate token is not empty
-		if token == "" {
-			oauthServer.Return401InvalidToken(w)
-			return
-		}
+		// Check Authorization header
+		authHeader := r.Header.Get("Authorization")
+		
+		// Return 401 if Bearer token missing
+		if authHeader == "" {
+			oauthServer.Return401(w)
+			return
+		}
+		
+		// Parse authorization scheme and token
+		fields := strings.Fields(authHeader)
+		if len(fields) < 2 {
+			oauthServer.Return401InvalidToken(w)
+			return
+		}
+		
+		// Check if it's a Bearer token (case-insensitive per OAuth 2.0 spec)
+		if !strings.EqualFold(fields[0], "Bearer") {
+			// Reject non-Bearer schemes (OAuth endpoints require Bearer tokens only)
+			oauthServer.Return401(w)
+			return
+		}
+		
+		token := fields[1]
+		if token == "" {
+			oauthServer.Return401InvalidToken(w)
+			return
+		}

This also applies to oauth.go WrapMCPEndpoint (lines 339-365) which has identical logic.

oauth.go (1)

339-365: Consider extracting shared Bearer validation logic.

The Bearer token validation logic (lines 339-360) is duplicated from mcp/oauth.go (lines 68-89). Consider extracting this into a shared helper function to improve maintainability and ensure consistent behavior across both wrappers.

Additionally, the same strings.Fields refactor suggested for mcp/oauth.go would apply here for more robust parsing.

Example structure:

// parseBearer parses and validates Bearer token from Authorization header.
// Returns token string and error (use appropriate 401 helper based on error type).
func parseBearer(authHeader string) (token string, err error) {
    if authHeader == "" {
        return "", errMissingToken
    }
    
    fields := strings.Fields(authHeader)
    if len(fields) < 2 {
        return "", errInvalidToken
    }
    
    if !strings.EqualFold(fields[0], "Bearer") {
        return "", errNotBearer
    }
    
    return fields[1], nil
}

Then both WrapMCPEndpoint implementations can use this shared logic.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 7131d3b and e0b2354.

📒 Files selected for processing (2)
  • mcp/oauth.go (3 hunks)
  • oauth.go (2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
mcp/oauth.go (1)
context.go (2)
  • WithOAuthToken (14-16)
  • WithUser (25-27)
oauth.go (1)
middleware.go (1)
  • CreateHTTPContextFunc (120-142)
🔇 Additional comments (7)
mcp/oauth.go (3)

62-66: LGTM! OPTIONS passthrough is correct.

Passing OPTIONS requests through for CORS pre-flight is the right approach and follows standard practice.


78-83: Security fix verified: non-Bearer schemes correctly rejected.

This properly addresses the auth bypass vulnerability flagged in previous reviews. Any non-Bearer Authorization header now returns 401, ensuring only Bearer tokens can access the MCP endpoint.


108-121: LGTM! Token validation and context enrichment flow is correct.

The implementation properly validates tokens, enriches the request context with both token and user information, and delegates to the MCP handler. Error handling with appropriate 401 responses is in place.

oauth.go (4)

333-337: LGTM! OPTIONS passthrough for CORS is correct.

Implementation matches mcp/oauth.go and properly handles CORS pre-flight requests.


349-354: Security fix verified: non-Bearer schemes correctly rejected.

This properly addresses the auth bypass concern from previous reviews. Non-Bearer Authorization headers are now rejected with a 401 response.


377-392: LGTM! RFC 6750 compliant 401 response.

The implementation correctly includes all parameters (realm, error, error_description, resource_metadata) in a single WWW-Authenticate header, which addresses the past concern about split headers. This follows RFC 6750 §3 requirements.


399-414: LGTM! Proper invalid_token error response.

Correctly uses error="invalid_token" per RFC 6750 §3.1 for authentication failures, and maintains the same single-header approach as Return401.

@tuannvm tuannvm merged commit d71ecb7 into main Nov 4, 2025
10 checks passed
@tuannvm tuannvm deleted the feat/handle-401 branch November 4, 2025 00:36
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