Feat/generic sdk #4
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>
…support Signed-off-by: Tommy Nguyen <tuannvm@hotmail.com>
|
Caution Review failedThe pull request is closed. WalkthroughRefactors the project into an SDK-agnostic oauth core and two SDK-specific adapters (mark3labs and official mcp), adds a thread-safe token cache and context helpers, moves WithOAuth into adapter packages, updates middleware to use cached validation, and expands docs and examples for dual-SDK usage. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client (mark3labs)
participant MCP as MCP Server
participant Middleware as mark3labs Middleware
participant OAuth as oauth Core
participant Cache as Token Cache
Client->>MCP: Tool request (token in context)
MCP->>Middleware: Invoke handler
Middleware->>OAuth: ValidateTokenCached(token)
OAuth->>Cache: Check cache (token hash)
alt Cache hit
Cache-->>OAuth: Cached user
else Cache miss
OAuth->>OAuth: Validate token with provider
OAuth->>Cache: Store user (5m TTL)
end
OAuth-->>Middleware: User
Middleware->>MCP: Call tool with user in context
MCP-->>Client: Response
sequenceDiagram
participant Client as Client (HTTP)
participant HTTP as mcp.WithOAuth handler
participant OAuth as oauth Core
participant Cache as Token Cache
participant MCP as MCP Stream Handler
Client->>HTTP: HTTP Request + Authorization: Bearer <token>
HTTP->>HTTP: Extract Bearer token
HTTP->>OAuth: ValidateTokenCached(token)
OAuth->>Cache: Check cache
alt Cache hit
Cache-->>OAuth: Cached user
else Cache miss
OAuth->>OAuth: Validate with provider
OAuth->>Cache: Store user (5m)
end
HTTP->>HTTP: Attach token/user to request context
HTTP->>MCP: Delegate to MCP handler
MCP-->>Client: Response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
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)
README.md (1)
124-134: Remove raw token preview logging in middleware.go
Inmiddleware.goat line 135,log.Printf("OAuth: Invalid Authorization header format: %s", preview)exposes parts of the bearer token. Remove or mask this log to prevent token leakage.go.mod (1)
3-3: Fix invalid Go version directive.go.mod’s
godirective must use major.minor only. Apply:-go 1.24.0 +go 1.24Then run:
go mod tidy go mod verifyand confirm any indirect modules (e.g., github.com/google/jsonschema-go) are still required.
oauth.go (1)
239-242: Avoid logging raw Authorization header inCreateHTTPContextFunc
Inmiddleware.go, the invalid‐header branch logs a raw header preview (first 30 chars), which can expose un-hashed tokens. Change this to log only a hashed preview or token length per the “no raw token logs” rule. [middleware.go:122–132]
🧹 Nitpick comments (16)
cache.go (2)
35-39: Avoid spawning a goroutine per expired read.Deleting synchronously is simpler and avoids goroutine churn.
Apply this diff:
- if time.Now().After(cached.ExpiresAt) { - tc.mu.RUnlock() - go tc.deleteExpiredToken(tokenHash) - return nil, false - } + if time.Now().After(cached.ExpiresAt) { + tc.mu.RUnlock() + tc.deleteExpiredToken(tokenHash) + return nil, false + }Also applies to: 45-53
25-43: Avoid mutating the returned CachedTokengetCachedToken returns a pointer to an internal CachedToken after unlocking; callers (middleware.go :45–47, oauth.go :115–117) must treat it as read-only or return a defensive copy to prevent data races.
docs/generic-plan.md (2)
25-45: Add language identifiers to fenced code blocks.Helps readability and satisfies MD040.
Example fix:
-``` +```text oauth-mcp-proxy/ ├── [core package - SDK-agnostic] ...And for import examples: ```diff -``` +```go import ( "github.com/modelcontextprotocol/go-sdk/mcp" mcpoauth "github.com/tuannvm/oauth-mcp-proxy/mcp" )Also applies to: 51-56 --- `212-216`: **Use headings instead of emphasis for section titles.** Convert emphasized lines to proper `###` headings (MD036). Example fix: ```diff -**Overall Complexity: MEDIUM** +### Overall Complexity: MEDIUM -**Total Estimated Effort: 1 day of focused work** +### Total Estimated Effort -**Option 1: Quick Update (mark3labs users)** +### Option 1: Quick Update (mark3labs users)Also applies to: 229-237
README.md (2)
12-16: Fix heading levels (MD001).Increment by one: use
##for section heads.Apply:
-### mark3labs/mcp-go +## mark3labs/mcp-go ... -### Official SDK +## Official SDK ... -### Using mark3labs/mcp-go +## Using mark3labs/mcp-go ... -### Using Official SDK +## Using Official SDKAlso applies to: 25-33, 84-86, 138-146
34-35: Hyphenate “2‑line change.”Minor copy edit.
-> **📢 Migrating from v1.x?** See [MIGRATION-V2.md](./MIGRATION-V2.md) (2 line change, ~5 min) +> **📢 Migrating from v1.x?** See [MIGRATION-V2.md](./MIGRATION-V2.md) (2-line change, ~5 min)docs/generic-implementation.md (1)
31-36: Add language to fenced blocks.Specify
text,bash, orgoas appropriate (MD040).Examples:
-``` +```text === RUN TestOfficialSDKContextPropagation ...-```bash +```bash go build ./... go test ./... -v-```go +```go // ValidateTokenCached validates a token with caching support.Also applies to: 101-106, 150-156, 371-377, 781-805
context.go (1)
8-27: LGTM. Context helpers are minimal and type‑safe.Good unexported keys and clear API. Add a short caution to not log tokens retrieved via
GetOAuthTokento align with project policy.Add to comments:
// Do not log raw tokens; prefer hashed previews if absolutely necessary.As per coding guidelines
Also applies to: 29-45
examples/advanced/main.go (1)
94-112: Unify TLS detection logic.You set
WithTLSbased on cert presence only (Line 24), but later require both cert and key (Line 94). This can drift config and runtime.Use the same condition in both places:
- WithTLS(getEnv("HTTPS_CERT_FILE", "") != ""). + WithTLS(getEnv("HTTPS_CERT_FILE", "") != "" && getEnv("HTTPS_KEY_FILE", "") != "")And keep the same check for
useTLS.Also applies to: 16-25
oauth.go (2)
120-126: Avoid leaking validator internals; add hashed token context to error log.Log a hashed preview to correlate failures without exposing internals; keep client message generic.
- s.logger.Error("Token validation failed: %v", err) + s.logger.Error("Token validation failed (hash: %s...): %v", tokenHash[:16], err)
102-133: Concurrency-safe TTL enforcement in TokenCache verified. TokenCache uses sync.RWMutex for safe concurrent access and deletes expired entries on reads. Optionally, if the provider.User struct exposes its own expiration, cap the cache TTL to the shorter duration.mark3labs/oauth.go (1)
11-39: Doc note: clarify root oauth.WithOAuth deprecation/back-compat.Docs say WithOAuth moved; add a brief note that root oauth.WithOAuth remains for back-compat or mark deprecated to avoid confusion.
mcp/oauth.go (1)
52-76: Consider reusing Server.WrapHandler to avoid duplication.Once WrapHandler validates tokens, wrap mcpHandler with it for a single source of truth.
- wrappedHandler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - ... - mcpHandler.ServeHTTP(w, r) - }) - return oauthServer, wrappedHandler, nil + return oauthServer, s.WrapHandler(mcpHandler), nilmark3labs/middleware.go (1)
25-28: Return a typed auth error for better handling (optional).Define a sentinel error (e.g., ErrUnauthenticated) to allow callers to map errors uniformly.
CLAUDE.md (2)
41-62: Add fenced code languages to satisfy markdownlint and improve rendering.Use “text” for trees/flows and keep “go” for snippets.
-``` +```text oauth-mcp-proxy/ @@ -```text +```text 1. HTTP request with "Authorization: Bearer <token>" header @@ -```text +```text 1. HTTP request with "Authorization: Bearer <token>" headerAlso applies to: 66-77, 88-111, 147-170
145-146: Clarify WithOAuth move vs. back-compat.State that root oauth.WithOAuth remains for backward compatibility or is deprecated, to prevent confusion with the new adapter entry points.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (14)
CLAUDE.md(3 hunks)README.md(5 hunks)cache.go(1 hunks)context.go(1 hunks)docs/generic-implementation.md(1 hunks)docs/generic-plan.md(1 hunks)examples/advanced/main.go(2 hunks)examples/simple/main.go(1 hunks)go.mod(1 hunks)mark3labs/middleware.go(1 hunks)mark3labs/oauth.go(1 hunks)mcp/oauth.go(1 hunks)middleware.go(0 hunks)oauth.go(2 hunks)
💤 Files with no reviewable changes (1)
- middleware.go
🧰 Additional context used
📓 Path-based instructions (2)
**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
Never log raw OAuth tokens; only log hashed previews (e.g., truncated SHA-256)
Files:
mark3labs/oauth.gomark3labs/middleware.goexamples/simple/main.gocontext.gooauth.gomcp/oauth.goexamples/advanced/main.gocache.go
{oauth.go,middleware.go,provider/**/*.go}
📄 CodeRabbit inference engine (CLAUDE.md)
Keep the server instance-scoped: no package-level globals for token caches or validators
Files:
oauth.go
🧠 Learnings (3)
📚 Learning: 2025-10-20T05:43:12.537Z
Learnt from: CR
PR: tuannvm/oauth-mcp-proxy#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-20T05:43:12.537Z
Learning: Always use GetUserFromContext(ctx) in tool handlers to access the authenticated user
Applied to files:
README.md
📚 Learning: 2025-10-20T05:43:12.537Z
Learnt from: CR
PR: tuannvm/oauth-mcp-proxy#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-20T05:43:12.537Z
Learning: Applies to middleware.go : Use a 5-minute token cache with SHA-256 of the token as the cache key
Applied to files:
oauth.go
📚 Learning: 2025-10-20T05:43:12.537Z
Learnt from: CR
PR: tuannvm/oauth-mcp-proxy#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-20T05:43:12.537Z
Learning: Applies to {oauth.go,middleware.go,provider/**/*.go} : Keep the server instance-scoped: no package-level globals for token caches or validators
Applied to files:
CLAUDE.mdcache.go
🧬 Code graph analysis (7)
mark3labs/oauth.go (2)
oauth.go (3)
WithOAuth(318-327)Server(21-27)NewServer(43-76)mark3labs/middleware.go (1)
NewMiddleware(22-40)
mark3labs/middleware.go (2)
oauth.go (1)
Server(21-27)context.go (2)
GetOAuthToken(19-22)WithUser(25-27)
examples/simple/main.go (2)
oauth.go (1)
WithOAuth(318-327)config.go (1)
Config(10-34)
context.go (1)
cache.go (1)
User(11-11)
oauth.go (2)
cache.go (1)
User(11-11)provider/provider.go (1)
User(17-21)
mcp/oauth.go (3)
mark3labs/oauth.go (1)
WithOAuth(39-48)oauth.go (3)
WithOAuth(318-327)Server(21-27)NewServer(43-76)context.go (2)
WithOAuthToken(14-16)WithUser(25-27)
examples/advanced/main.go (1)
oauth.go (1)
WithOAuth(318-327)
🪛 LanguageTool
README.md
[grammar] ~34-~34: Use a hyphen to join words.
Context: ... MIGRATION-V2.md (2 line change, ~5 min) [![GitHub Workflow...
(QB_NEW_EN_HYPHEN)
docs/generic-implementation.md
[style] ~25-~25: Consider using a different adverb to strengthen your wording.
Context: .... If context doesn't propagate, we need a completely different approach. Test Created: ...
(COMPLETELY_ENTIRELY)
[style] ~454-~454: To form a complete sentence, be sure to include a subject.
Context: ...Status*: Skipped for initial release. Can be added later. Files to Create: -...
(MISSING_IT_THERE)
🪛 markdownlint-cli2 (0.18.1)
README.md
12-12: Heading levels should only increment by one level at a time
Expected: h2; Actual: h3
(MD001, heading-increment)
docs/generic-plan.md
25-25: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
51-51: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
167-167: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
212-212: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
229-229: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
CLAUDE.md
43-43: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
docs/generic-implementation.md
31-31: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (8)
examples/advanced/main.go (3)
35-36: Adapter switch looks correct.mark3labs.WithOAuth usage matches the new adapter pattern.
82-91: LGTM: user access via context.Correctly uses
oauth.GetUserFromContext(ctx)in tool handler.
64-66: No changes required WrapHandlerFunc and GetHTTPServerOptions remain defined on oauth.Server.examples/simple/main.go (2)
20-26: Adapter switch looks good.mark3labs.WithOAuth replaces root
oauth.WithOAuthas intended.
50-55: ConfirmCreateHTTPContextFuncremains exported inoauth/middleware.go(line 120); no import or usage updates required.oauth.go (1)
61-75: Instance-scoped cache/validator: LGTM.No package-level globals; aligns with server instance scope. Based on learnings.
mark3labs/oauth.go (1)
39-48: Adapter wiring looks correct.Creates server, registers handlers, returns middleware via NewMiddleware; matches intended flow.
mark3labs/middleware.go (1)
22-39: Middleware flow: LGTM.Reads token from context, validates via cache, attaches user, calls next. Clean.
| // setCachedToken stores a token validation result | ||
| func (tc *TokenCache) setCachedToken(tokenHash string, user *User, expiresAt time.Time) { | ||
| tc.mu.Lock() | ||
| defer tc.mu.Unlock() | ||
|
|
||
| tc.cache[tokenHash] = &CachedToken{ | ||
| User: user, | ||
| ExpiresAt: expiresAt, | ||
| } | ||
| } |
There was a problem hiding this comment.
Make TokenCache zero‑value safe (prevent map panic).
tc.cache may be nil, leading to a panic on first write.
Apply this diff:
func (tc *TokenCache) setCachedToken(tokenHash string, user *User, expiresAt time.Time) {
tc.mu.Lock()
defer tc.mu.Unlock()
+ if tc.cache == nil {
+ tc.cache = make(map[string]*CachedToken)
+ }
tc.cache[tokenHash] = &CachedToken{
User: user,
ExpiresAt: expiresAt,
}
}Optionally add:
// NewTokenCache constructs a ready-to-use cache.
func NewTokenCache() *TokenCache { return &TokenCache{cache: make(map[string]*CachedToken)} }🤖 Prompt for AI Agents
In cache.go around lines 55 to 64, the setCachedToken method writes to tc.cache
without ensuring the map is initialized, which can panic if tc is a zero value;
modify setCachedToken to check if tc.cache == nil and initialize it with
make(map[string]*CachedToken) before assigning, and keep the mutex semantics
(lock/unlock) as-is; optionally add a NewTokenCache constructor that returns
&TokenCache{cache: make(map[string]*CachedToken)} so callers can obtain a
ready-to-use cache.
| authHeader := r.Header.Get("Authorization") | ||
| if authHeader == "" || len(authHeader) < 7 || authHeader[:7] != "Bearer " { | ||
| http.Error(w, "Missing or invalid Authorization header", http.StatusUnauthorized) | ||
| return | ||
| } |
There was a problem hiding this comment.
Return RFC 6750-compliant 401s and avoid exposing internal errors.
Add WWW-Authenticate with resource metadata and JSON body; keep client message generic.
@@
- if authHeader == "" || len(authHeader) < 7 || authHeader[:7] != "Bearer " {
- http.Error(w, "Missing or invalid Authorization header", http.StatusUnauthorized)
- return
- }
+ if authHeader == "" || len(authHeader) < 7 || authHeader[:7] != "Bearer " {
+ metadataURL := oauthServer.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)
+ _ = json.NewEncoder(w).Encode(map[string]string{
+ "error": "invalid_token",
+ "error_description": "Missing or invalid access token",
+ })
+ return
+ }
@@
- user, err := oauthServer.ValidateTokenCached(r.Context(), token)
- if err != nil {
- http.Error(w, fmt.Sprintf("Authentication failed: %v", err), http.StatusUnauthorized)
- return
- }
+ user, err := oauthServer.ValidateTokenCached(r.Context(), token)
+ if err != nil {
+ metadataURL := oauthServer.GetProtectedResourceMetadataURL()
+ w.Header().Add("WWW-Authenticate", `Bearer realm="OAuth", error="invalid_token", error_description="Authentication failed"`)
+ w.Header().Add("WWW-Authenticate", fmt.Sprintf(`resource_metadata="%s"`, metadataURL))
+ w.Header().Set("Content-Type", "application/json")
+ w.WriteHeader(http.StatusUnauthorized)
+ _ = json.NewEncoder(w).Encode(map[string]string{
+ "error": "invalid_token",
+ "error_description": "Authentication failed",
+ })
+ return
+ }Note: add imports for encoding/json if not present.
Also applies to: 65-69
Signed-off-by: Tommy Nguyen <tuannvm@hotmail.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
examples/mark3labs/simple/main.go (1)
78-83: LGTM! Simple and effective helper.The
getEnvhelper function is clean and serves its purpose well for this example.Minor note on implementation variance: The advanced example uses
os.LookupEnv(lines 122-127 in advanced/main.go) while this usesos.Getenv. Both are functionally equivalent for this use case, butLookupEnvis slightly more idiomatic as it distinguishes between unset and empty values. For consistency across examples, consider aligning the implementations.examples/official/advanced/main.go (3)
18-21: Prefer full Okta issuer URL over bare domain.Okta issuers are typically https://{domain}/oauth2/{authServerId} (often /default). Using only the domain commonly fails discovery/validation.
Apply this diff:
- // export OKTA_DOMAIN="dev-12345.okta.com" - // export OKTA_AUDIENCE="api://my-mcp-server" + // export OKTA_ISSUER="https://dev-12345.okta.com/oauth2/default" + // export OKTA_AUDIENCE="api://my-mcp-server"- WithIssuer(fmt.Sprintf("https://%s", getEnv("OKTA_DOMAIN", "dev-12345.okta.com"))). + WithIssuer(getEnv("OKTA_ISSUER", "https://dev-12345.okta.com/oauth2/default")).- log.Println("\nMake sure to set your Okta environment variables:") - log.Println(" export OKTA_DOMAIN=dev-12345.okta.com") + log.Println("\nMake sure to set your Okta environment variables:") + log.Println(" export OKTA_ISSUER=https://dev-12345.okta.com/oauth2/default") log.Println(" export OKTA_AUDIENCE=api://my-mcp-server")Please confirm your proxy’s Okta adapter accepts a full issuer and whether WithProvider("okta") has any special domain handling. If it mandates domain input, we can adapt accordingly.
Also applies to: 23-24, 134-138
52-52: Avoid logging PII in production.You log usernames. For examples it’s fine; in real deployments, gate behind a DEBUG flag or redact.
Also applies to: 77-77, 100-100
129-142: Graceful shutdown and timeouts.Consider http.Server with ReadHeaderTimeout/IdleTimeout and OS signal handling for clean shutdown.
Minimal sketch:
srv := &http.Server{ Addr: addr, Handler: handler, ReadTimeout: 10 * time.Second, WriteTimeout: 10 * time.Second, IdleTimeout: 60 * time.Second, } go func() { log.Fatal(srv.ListenAndServe()) }() /* handle SIGINT/SIGTERM, then: srv.Shutdown(ctx) */
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
examples/README.md(1 hunks)examples/mark3labs/advanced/main.go(2 hunks)examples/mark3labs/simple/main.go(1 hunks)examples/official/advanced/main.go(1 hunks)examples/official/simple/main.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
Never log raw OAuth tokens; only log hashed previews (e.g., truncated SHA-256)
Files:
examples/mark3labs/simple/main.goexamples/mark3labs/advanced/main.goexamples/official/advanced/main.goexamples/official/simple/main.go
🧬 Code graph analysis (4)
examples/mark3labs/simple/main.go (2)
context.go (1)
GetUserFromContext(42-45)middleware.go (1)
CreateHTTPContextFunc(120-139)
examples/mark3labs/advanced/main.go (1)
config.go (1)
NewConfigBuilder(164-170)
examples/official/advanced/main.go (3)
config.go (1)
NewConfigBuilder(164-170)oauth.go (1)
NewServer(43-76)context.go (1)
GetUserFromContext(42-45)
examples/official/simple/main.go (2)
oauth.go (1)
NewServer(43-76)context.go (1)
GetUserFromContext(42-45)
🪛 Gitleaks (8.28.0)
examples/README.md
[high] 298-298: Discovered a potential authorization token provided in a curl command header, which could compromise the curl accessed resource.
(curl-auth-header)
🪛 markdownlint-cli2 (0.18.1)
examples/README.md
7-7: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
69-69: Bare URL used
(MD034, no-bare-urls)
88-88: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
98-98: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
365-365: Bare URL used
(MD034, no-bare-urls)
366-366: Bare URL used
(MD034, no-bare-urls)
🔇 Additional comments (14)
examples/official/simple/main.go (4)
1-13: LGTM! Clean import structure.The imports are well-organized with standard library packages followed by external dependencies.
15-45: LGTM! Proper OAuth context usage.The example correctly demonstrates:
- MCP server initialization with official SDK
- Tool registration with OAuth-protected context
- Proper user extraction from context with error handling
- Returning user information to the authenticated user
47-83: LGTM! OAuth configuration follows best practices.The example demonstrates:
- Proper Okta configuration with environment variables
- Comprehensive error handling for OAuth setup
- Helpful startup logging without exposing sensitive data (complies with coding guidelines)
- Clear instructions for users to set required environment variables
86-91: LGTM! Clean environment variable helper.Simple and effective helper function for retrieving environment variables with default fallbacks.
examples/mark3labs/simple/main.go (5)
3-14: LGTM! Well-organized imports.The imports are cleanly organized and include all necessary packages for the OAuth-enabled MCP server example.
16-33: LGTM! Clear OAuth configuration with good documentation.The OAuth setup is well-structured with helpful comments documenting the required environment variables. The use of
getEnvwith example defaults makes the code runnable out-of-the-box for demonstration purposes while clearly indicating where real values should be provided.
35-52: LGTM! Proper OAuth context usage in tool handler.The tool correctly retrieves the authenticated user from context using
oauth.GetUserFromContextand handles the unauthenticated case appropriately. This is a clear example of how to access user information in OAuth-protected tools.
54-60: LGTM! Correct HTTP context integration.The endpoint setup properly integrates OAuth by using
CreateHTTPContextFunc()to extract and propagate the OAuth token through the request context. The implementation adheres to security guidelines by not logging raw tokens.
62-76: LGTM! Helpful startup logging and guidance.The server startup includes clear logging of configuration details and helpful guidance on required environment variables. This makes the example easy to follow and debug.
examples/mark3labs/advanced/main.go (4)
13-13: LGTM! Necessary import for mark3labs adapter.The addition of the mark3labs adapter package import is correct and supports the refactoring to use SDK-specific adapters.
18-24: LGTM! Improved configuration flexibility.The refactoring to use environment variables for OAuth configuration makes the example more flexible and production-ready. The updated comments clearly document the required variables.
38-38: LGTM! Correct adapter usage.The change from
oauth.WithOAuthtomark3labs.WithOAuthproperly uses the SDK-specific adapter, which aligns with this PR's objective of supporting multiple SDKs through adapter packages.
122-127: LGTM! Idiomatic environment variable helper.The use of
os.LookupEnvis the more idiomatic approach as it distinguishes between unset and empty values. This implementation is clean and appropriate.examples/official/advanced/main.go (1)
42-59: Verify AddTool API; handle errors if the SDK returns them.If mcp.AddTool returns an error in this SDK version, capture and handle it; otherwise ignore this.
Example if it does return error:
if err := mcp.AddTool(mcpServer, tool, handler); err != nil { log.Fatalf("register tool %q: %v", tool.Name, err) }Also applies to: 63-85, 88-107
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: 0
🧹 Nitpick comments (1)
examples/official/simple/main.go (1)
68-87: Server startup and logging are well-structured.The startup sequence correctly logs configuration without exposing sensitive data. The detailed comments about PORT vs SERVER_URL (lines 69-72) are helpful for users deploying behind reverse proxies. No raw OAuth tokens are logged, complying with coding guidelines.
Optional: Consider extracting the issuer URL construction.
The issuer URL is constructed twice (lines 54 and 78). For slightly improved maintainability, you could extract it to a variable:
+ issuer := fmt.Sprintf("https://%s", getEnv("OKTA_DOMAIN", "dev-12345.okta.com")) cfg := &oauth.Config{ Provider: "okta", - Issuer: fmt.Sprintf("https://%s", getEnv("OKTA_DOMAIN", "dev-12345.okta.com")), + Issuer: issuer, Audience: getEnv("OKTA_AUDIENCE", "api://my-mcp-server"), ServerURL: getEnv("SERVER_URL", "http://localhost:8080"), } // ... later in logging ... - log.Printf("Issuer: https://%s", getEnv("OKTA_DOMAIN", "dev-12345.okta.com")) + log.Printf("Issuer: %s", issuer)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
examples/mark3labs/simple/main.go(1 hunks)examples/official/advanced/main.go(1 hunks)examples/official/simple/main.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- examples/official/advanced/main.go
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
Never log raw OAuth tokens; only log hashed previews (e.g., truncated SHA-256)
Files:
examples/official/simple/main.goexamples/mark3labs/simple/main.go
🧬 Code graph analysis (2)
examples/official/simple/main.go (1)
context.go (1)
GetUserFromContext(42-45)
examples/mark3labs/simple/main.go (2)
context.go (1)
GetUserFromContext(42-45)middleware.go (1)
CreateHTTPContextFunc(120-139)
🔇 Additional comments (4)
examples/official/simple/main.go (3)
25-45: LGTM! Clean example of context-based user retrieval.The tool correctly uses
oauth.GetUserFromContext(ctx)to retrieve authenticated user data and handles the case where authentication is missing. The implementation aligns well with the new context helpers introduced in the PR.
47-57: Configuration setup looks good.The OAuth configuration correctly uses environment variables with sensible placeholder defaults for an example. The issuer URL construction from OKTA_DOMAIN is appropriate.
90-95: Standard helper implementation.The
getEnvfunction follows a common pattern for environment variable access with defaults. Implementation is correct.examples/mark3labs/simple/main.go (1)
1-87: LGTM! Well-structured OAuth integration example.This example file effectively demonstrates OAuth integration with the mark3labs MCP SDK. The code is correct, secure, and compliant with the coding guidelines:
- No raw OAuth tokens are logged (only token length in the middleware, which is compliant)
- Environment variable configuration with sensible defaults
- Clear error handling for OAuth setup and server startup
- Proper authentication checks in the tool handler
- Comprehensive user guidance with environment setup instructions
- Helpful comments explaining PORT vs SERVER_URL relationship
The example values (dev-12345.okta.com) are clearly placeholders, and the startup logs provide excellent guidance for users to configure their environment.
Signed-off-by: Tommy Nguyen <tuannvm@hotmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
examples/README.md (1)
325-325: Replace non-existent Go version in Dockerfile.Go 1.24 doesn't exist yet. Use an available version like
golang:1.23orgolang:latestinstead.-FROM golang:1.24 AS builder +FROM golang:1.23 AS builder
🧹 Nitpick comments (2)
examples/README.md (1)
296-299: Add clarification to curl example about token placeholder.The curl example shows
$TOKENwhich will expand to a real token value. Add a comment clarifying this should be replaced with an actual test token from the setup steps above.# Save your Okta token TOKEN="<your-okta-access-token>" # Test with curl +# Replace TOKEN with an actual token from the setup steps above curl -H "Authorization: Bearer $TOKEN" \docs/generic-implementation.md (1)
1-50: Implementation tracking documentation is well-structured and comprehensive.The phases are clearly organized with verification checkpoints and status indicators. Test results are properly documented. This serves as a good implementation guide.
Minor style notes (from linters):
- Lines 25, 462: Minor grammar suggestions (optional)
- Line 31: Consider adding language to code fence for markdown compliance (optional)
These are documentation style issues, not blockers.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
.github/workflows/test.yml(1 hunks)README.md(5 hunks)docs/CLIENT-SETUP.md(3 hunks)docs/MIGRATION.md(0 hunks)docs/TROUBLESHOOTING.md(1 hunks)docs/generic-implementation.md(1 hunks)docs/providers/AZURE.md(1 hunks)docs/providers/GOOGLE.md(1 hunks)docs/providers/HMAC.md(4 hunks)docs/providers/OKTA.md(6 hunks)examples/README.md(1 hunks)
💤 Files with no reviewable changes (1)
- docs/MIGRATION.md
✅ Files skipped from review due to trivial changes (2)
- docs/providers/AZURE.md
- docs/providers/GOOGLE.md
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-20T05:43:12.537Z
Learnt from: CR
PR: tuannvm/oauth-mcp-proxy#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-20T05:43:12.537Z
Learning: Always use GetUserFromContext(ctx) in tool handlers to access the authenticated user
Applied to files:
README.md
🪛 Gitleaks (8.28.0)
examples/README.md
[high] 298-298: Discovered a potential authorization token provided in a curl command header, which could compromise the curl accessed resource.
(curl-auth-header)
🪛 LanguageTool
docs/generic-implementation.md
[style] ~25-~25: Consider using a different adverb to strengthen your wording.
Context: .... If context doesn't propagate, we need a completely different approach. Test Created: ...
(COMPLETELY_ENTIRELY)
[style] ~462-~462: To form a complete sentence, be sure to include a subject.
Context: ...Status*: Skipped for initial release. Can be added later. Files to Create: -...
(MISSING_IT_THERE)
🪛 markdownlint-cli2 (0.18.1)
README.md
12-12: Heading levels should only increment by one level at a time
Expected: h2; Actual: h3
(MD001, heading-increment)
docs/generic-implementation.md
31-31: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (6)
docs/TROUBLESHOOTING.md (1)
477-477: Documentation correctly reflects dual-SDK example structure.The update from single example path to dual SDK examples is accurate and helps users understand both pathways are available.
.github/workflows/test.yml (1)
50-54: Improved build approach for dynamic example discovery.Using
findto discover examples is better than hardcoded paths. This scales automatically as new examples are added to the directory structure.docs/CLIENT-SETUP.md (1)
163-240: Dual-SDK client configuration documentation is clear and consistent.Both mark3labs and official SDK paths are properly documented with parallel examples for Native and Proxy modes. Import paths and initialization sequences are correct.
docs/providers/HMAC.md (1)
29-69: HMAC provider configuration correctly documents both SDKs.The parallel examples for mark3labs and official SDKs are clear, imports are accurate, and the configuration patterns are consistent. Token generation guidance is appropriately provider-agnostic.
docs/providers/OKTA.md (1)
80-142: Okta provider configuration correctly documents both SDKs.Native and Proxy modes are clearly documented with parallel examples for both mark3labs and official SDKs. Import paths and configuration patterns are consistent and accurate.
README.md (1)
5-32: Excellent dual-SDK documentation in README.The introduction clearly highlights support for both SDKs, and the parallel code examples immediately show how each SDK integrates differently. Migration guidance link at line 34 is helpful for existing users. The examples table and quick start sections effectively demonstrate both pathways.
| **One-time setup:** Configure provider + add `WithOAuth()` to your server. | ||
| **Result:** All tools automatically protected with token validation and caching. | ||
|
|
||
| ### mark3labs/mcp-go |
There was a problem hiding this comment.
Fix heading hierarchy: change h3 to h2.
Line 12 is an h3 heading (###) but should be h2 (##) as a direct child of the "Quick Start" h2 section. Heading levels should increment by one level at a time per markdown linting rules.
-### Using mark3labs/mcp-go
+## Using mark3labs/mcp-goAnd similarly for the "Using Official SDK" section at line 138.
Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
12-12: Heading levels should only increment by one level at a time
Expected: h2; Actual: h3
(MD001, heading-increment)
🤖 Prompt for AI Agents
In README.md around lines 12 and 138, the headings use h3 (###) but must be h2
(##) to maintain proper incremental hierarchy under the "Quick Start" h2 section
and the "Using Official SDK" section; change the two headings from `###` to `##`
so each is a direct child one level below their parent h2, and verify adjacent
headings follow the same incremental rule.
…ver startup Signed-off-by: Tommy Nguyen <tuannvm@hotmail.com>
Summary by CodeRabbit
New Features
Breaking Changes
Documentation