Feat/init #1
Merged
Feat/init #1
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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>
Signed-off-by: Tommy Nguyen <tuannvm@hotmail.com>
…ation Signed-off-by: Tommy Nguyen <tuannvm@hotmail.com>
… usage Signed-off-by: Tommy Nguyen <tuannvm@hotmail.com>
Signed-off-by: Tommy Nguyen <tuannvm@hotmail.com>
Signed-off-by: Tommy Nguyen <tuannvm@hotmail.com>
WalkthroughIntroduces a complete OAuth MCP Proxy library with native and proxy authentication modes, pluggable provider support (HMAC, OIDC, Okta, Google, Azure), HTTP handlers for OAuth flows, token validation with caching, MCP middleware integration, comprehensive documentation, deployment tooling, and extensive test coverage. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant MCP as MCP Server/<br/>OAuth Handler
participant Provider as OAuth Provider<br/>(Okta/Google/etc)
rect rgb(200, 220, 255)
Note over Client,Provider: OAuth 2.0 Authorization Flow (Proxy Mode)
Client->>MCP: GET /oauth2/authorize<br/>?redirect_uri=...&state=...
MCP->>MCP: Validate redirect_uri<br/>Sign state (fixed-redirect)
MCP->>Provider: Redirect to provider<br/>authorization endpoint
Provider->>Client: Show login/consent
Client->>Provider: User grants consent
Provider->>MCP: Redirect callback<br/>?code=...&state=...
end
rect rgb(220, 255, 220)
Note over MCP,Provider: Token Exchange
MCP->>MCP: Verify signed state<br/>Validate redirect_uri
MCP->>Provider: POST /token<br/>code + code_verifier (PKCE)
Provider->>MCP: access_token + id_token
end
rect rgb(255, 240, 200)
Note over Client,MCP: Token Validation & Tool Access
Client->>MCP: Call MCP tool<br/>Authorization: Bearer {token}
MCP->>MCP: Middleware extracts token<br/>Check cache (5min TTL)
alt Token cached
MCP->>MCP: Use cached User
else Token not cached
MCP->>Provider: Verify token/validate claims<br/>(audience, signature, exp)
Provider->>MCP: User claims
MCP->>MCP: Store in cache
end
MCP->>Client: Tool executes with<br/>authenticated User context
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Rationale: This is a substantial, foundational pull request introducing an entire OAuth library with intricate security-critical logic. The heterogeneous scope spans CI/CD configuration, comprehensive documentation, core cryptographic and OAuth flow implementations, pluggable provider architecture, middleware integration with MCP, and 40+ test files. Key complexity drivers include: (1) multi-layer OAuth2/OIDC flows with PKCE and state signing; (2) defense-in-depth security model (localhost validation, HMAC state integrity, redirect URI validation); (3) context propagation through HTTP and MCP middleware; (4) instance-scoped token caching; (5) provider abstraction with multiple validators; (6) metadata endpoint implementations. Each major functional area demands separate security and correctness reasoning, with particular attention to token handling, state validation, and context flow. Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
Signed-off-by: Tommy Nguyen <tuannvm@hotmail.com>
Signed-off-by: Tommy Nguyen <tuannvm@hotmail.com>
…ames Signed-off-by: Tommy Nguyen <tuannvm@hotmail.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 18
🧹 Nitpick comments (50)
state_test.go (3)
10-11: Check for error from crypto/rand.Read.While ignoring the error from
crypto/rand.Readis unlikely to cause issues in tests (crypto/rand rarely fails), it's better practice to check the error to catch potential test environment issues.Apply this diff:
key := make([]byte, 32) - _, _ = rand.Read(key) + if _, err := rand.Read(key); err != nil { + t.Fatalf("Failed to generate random key: %v", err) + }
83-84: Check for error from crypto/rand.Read.Same recommendation as line 10-11.
110-111: Check for error from crypto/rand.Read.Same recommendation as previous instances.
attack_scenarios_test.go (1)
9-10: Check for error from crypto/rand.Read.Same recommendation as in state_test.go - check the error for robustness.
oauth.go (2)
63-71: Avoid hard‑coding version string in handler setup.Passing "1.0.0" makes versioning brittle. Thread a single source of truth (const/env/ldflags) to NewServer/CreateOAuth2Handler.
Apply one of:
- handler := CreateOAuth2Handler(cfg, "1.0.0", logger) + handler := CreateOAuth2Handler(cfg, Version(), logger)And add:
+// version.go +package oauth + +var version = "dev" +func Version() string { return version }Then set with -ldflags "-X github.com/tuannvm/oauth-mcp-proxy/oauth.version=$(git describe --tags --always)".
112-124: Good integration surface. Add explicit note about HTTPContextFunc.WithOAuth returns only middleware; token extraction relies on HTTPContextFunc. Consider returning both or panic/log when HTTPContextFunc is nil to catch misconfig at startup.
fixed_redirect_test.go (2)
12-68: Test isLocalhostURI only checks host; expectedError suggests broader validation.You’re mixing error expectations unrelated to isLocalhostURI. Either:
- test validateRedirectURI (full validator returning those errors), or
- drop expectedError and assert only host classification here.
Apply one:
- expectedError string + // only assert host classification hereOr add a separate test exercising the full redirect validator that returns those specific errors.
70-85: Tighten negative assertions.The exception logic makes failures easy to miss. Prefer explicit wantIsLocalhost and assert equality.
- if !tt.shouldPass && isLocalhost && tt.expectedError != "must not contain fragment" && tt.expectedError != "Invalid redirect_uri scheme" { - t.Errorf("Expected localhost detection to fail for %s", tt.clientURI) - } + if got := isLocalhost; got != tt.shouldPass { + t.Errorf("isLocalhostURI(%q) = %v, want %v", tt.clientURI, got, tt.shouldPass) + } ```<!-- review_comment_end --> </blockquote></details> <details> <summary>config.go (1)</summary><blockquote> `36-90`: **Config.Validate: good guardrails; consider optional native-mode relaxations.** Requiring Audience for all modes may be too strict for pure ID-token–only native flows. If intended, keep; else, make Audience required only when used by the chosen validator. <!-- review_comment_end --> </blockquote></details> <details> <summary>provider/provider.go (3)</summary><blockquote> `295-325`: **Add clock skew leeway to standard claim checks.** Strict second-for-second checks can spuriously fail with small clock drift. Add a configurable leeway (e.g., 60s). ```diff -func validateTokenClaims(claims jwt.MapClaims) error { +func validateTokenClaims(claims jwt.MapClaims) error { + const leeway = int64(60) now := time.Now().Unix() if exp, ok := claims["exp"].(float64); ok { - if now > int64(exp) { + if now > int64(exp)+leeway { return fmt.Errorf("token expired") } } if nbf, ok := claims["nbf"].(float64); ok { - if now < int64(nbf) { + if now+leeway < int64(nbf) { return fmt.Errorf("token not yet valid") } } if iat, ok := claims["iat"].(float64); ok { - if now < int64(iat) { + if now+leeway < int64(iat) { return fmt.Errorf("token issued in the future") } } return nil } ```<!-- review_comment_end --> --- `198-205`: **Supported algs list may be too narrow.** Limiting to RS256 and ES256 could reject PS256/PS384/RS512 used by some providers. Consider letting go-oidc defaults or make this configurable. <!-- review_comment_end --> --- `131-158`: **Duplicate audience validation logic.** HMACValidator.validateAudience and OIDCValidator.validateAudience are identical. Extract a shared helper to reduce duplication. <!-- review_comment_end --> Also applies to: 266-293 </blockquote></details> <details> <summary>metadata.go (2)</summary><blockquote> `133-141`: **Make resource_signing_alg_values_supported provider‑aware** Hardcoded RS256 may be wrong for HMAC deployments. Compute based on provider mode. ```diff - "resource_signing_alg_values_supported": []string{"RS256"}, + "resource_signing_alg_values_supported": func() []string { + if h.config.Provider == "hmac" { return []string{"HS256"} } + return []string{"RS256"} + }(),
53-70: Legacy metadata JWKS path is provider‑incorrectFor okta/google/azure you set jwks_uri = issuer + "/.well-known/jwks.json". Okta and Azure use non‑well‑known JWKS paths; Google uses a fixed URL. Align with GetAuthorizationServerMetadata provider logic for JWKS to avoid misleading clients.
metadata_test.go (2)
183-231: Add tests for mode/JWKS consistency and CORS headersGiven JWKS is disabled in native mode, add a test that discovery JWKS matches a reachable endpoint per mode, and verify Access-Control-Allow-Headers echo behavior on OPTIONS. This guards against the discovery/JWKS mismatch noted in implementation.
93-144: HEAD/OPTIONS coverage is good; also assert Content-Type and JSON error shapeFor negative methods (POST), assert Content-Type stays application/json and the body is a JSON error object once error handling is refactored.
handlers.go (4)
235-240: Reinforce JWKS proxy transport and timeoutsJWKS client lacks TLS MinVersion and fine‑grained timeouts. Add TLS1.3, TLSHandshakeTimeout, IdleConnTimeout, MaxIdleConns/PerHost.
client := &http.Client{ - Timeout: 10 * time.Second, - Transport: &http.Transport{ - TLSClientConfig: &tls.Config{InsecureSkipVerify: false}, - }, + Timeout: 10 * time.Second, + Transport: &http.Transport{ + TLSClientConfig: &tls.Config{ + InsecureSkipVerify: false, + MinVersion: tls.VersionTLS13, + }, + TLSHandshakeTimeout: 10 * time.Second, + IdleConnTimeout: 30 * time.Second, + MaxIdleConns: 10, + MaxIdleConnsPerHost: 2, + }, }Also applies to: 242-265
792-804: Wire up validation and security headersvalidateOAuthParams and addSecurityHeaders are unused. Invoke them in HandleAuthorize/HandleToken (after ParseForm) and add headers to responses for consistency.
Example:
func (h *OAuth2Handler) HandleToken(w http.ResponseWriter, r *http.Request) { + h.addSecurityHeaders(w) ... - if err := r.ParseForm(); err != nil { ... } + if err := r.ParseForm(); err != nil { ... } + if err := h.validateOAuthParams(r); err != nil { + h.logger.Warn("SECURITY: %v", err) + writeJSONError(w, http.StatusBadRequest, "invalid_request", err.Error()) + return + }Also applies to: 806-812
647-681: PKCE transport: broaden detection and preserve headersMatching only URL paths containing "/token" is fragile. Token endpoints may vary. Add code_verifier to any POST with form grant_type=authorization_code, and preserve Content-Type explicitly.
- if req.Method == "POST" && strings.Contains(req.URL.Path, "/token") { + if req.Method == "POST" && strings.Contains(req.Header.Get("Content-Type"), "application/x-www-form-urlencoded") { ... - req.ContentLength = int64(len(values.Encode())) + req.ContentLength = int64(len(values.Encode())) + req.Header.Set("Content-Type", "application/x-www-form-urlencoded") }
771-789: Redirect allowlist comparison: normalize URIsPlain string equality is brittle (trailing slashes, case, default ports). Normalize before compare (scheme, host, path).
Sketch:
- if allowed != "" && uri == allowed { + if allowed != "" && urlsEqual(uri, allowed) { return true }Helper:
func urlsEqual(a, b string) bool { au, err1 := url.Parse(strings.TrimSpace(a)) bu, err2 := url.Parse(strings.TrimSpace(b)) if err1 != nil || err2 != nil { return false } return strings.EqualFold(au.Scheme, bu.Scheme) && strings.EqualFold(au.Host, bu.Host) && strings.TrimRight(au.Path, "/") == strings.TrimRight(bu.Path, "/") }.goreleaser.yml (1)
6-10: Consider adding static analysis in pre‑hooksIncluding go vet/staticcheck before release improves signal. Optional but useful.
before: hooks: - go mod tidy - go test ./... + - go vet ./... + # - staticcheck ./... # if added to tools.github/workflows/test.yml (3)
38-45: Avoid bc dependency in coverage checkbc may not be present. Use awk only.
- COVERAGE=$(go tool cover -func=coverage.out | grep total | awk '{print $3}' | sed 's/%//') - echo "Total coverage: ${COVERAGE}%" - if (( $(echo "$COVERAGE < 30" | bc -l) )); then + COVERAGE=$(go tool cover -func=coverage.out | awk '/total:/ {gsub("%","",$3); print $3}') + echo "Total coverage: ${COVERAGE}%" + awk -v c="$COVERAGE" 'BEGIN{ if (c+0 < 30) print "Warning: Coverage below 30%"}'
49-53: Guard example builds if directories are optionalFailing builds if examples are absent makes PRs brittle.
- cd examples/simple && go build - cd ../embedded && go build + [ -d examples/simple ] && (cd examples/simple && go build) + [ -d examples/embedded ] && (cd examples/embedded && go build)
12-14: Use go-version-file to sync workflow with go.modGo 1.25 is a valid, released version, so there's no risk of toolchain resolution failures. However, using
go-version-file: go.modis a best practice that keeps your workflow automatically in sync with your declared version (currently 1.25.1), avoiding manual maintenance.env: - GO_VERSION: "1.25" + GO_VERSION: "" ... - name: Set up Go uses: actions/setup-go@v5 with: - go-version: ${{ env.GO_VERSION }} + go-version-file: go.mod cache: trueAlso applies to: 23-28
docs/SECURITY.md (1)
156-156: Add language specification to fenced code blocks.Lines 156, 245, 325, 330, and 383 contain fenced code blocks without a language identifier. Per markdownlint (MD040), all code blocks should specify a language for syntax highlighting and consistency. These appear to show plain text output, HTTP headers, and log formats.
Apply this diff to add language specifications:
# Line 156 (HTTP output format) -``` +```text Service A: Audience = "api://service-a" Service B: Audience = "api://service-b" -``` +``` # Line 245 (URI examples) -``` +```text ✅ http://localhost:8080/callback ✅ http://127.0.0.1:3000/callback -``` +``` # Line 325 (log output) -``` +```text INFO: Validating token (hash: a7bc40a987f35871...) -``` +``` # Line 330 (log output - bad example) -``` +```text ERROR: Token xyz123... invalid // SECURITY VIOLATION! -``` +``` # Line 383 (HTTP headers output) -``` +```text X-Content-Type-Options: nosniff X-Frame-Options: DENY -``` +```Also applies to: 245-245, 325-325, 330-330, 383-383
docs/CONFIGURATION.md (1)
346-346: Add language specification to fenced code block.Line 346 contains a fenced code block without a language identifier for the default logging output format. Per markdownlint (MD040), specify the language for consistency and syntax highlighting.
Apply this diff:
**Default behavior:** -``` +```text [INFO] OAuth2: Authorization request - client_id: ... [WARN] SECURITY: Invalid redirect URI ... [ERROR] OAuth2: Token validation failed: ... -``` +```docs/providers/HMAC.md (1)
44-61: Consider showing error handling in the example.The example function ignores the error from
token.SignedString(secret)on Line 58. While acceptable for documentation brevity, consider adding a comment or showing proper error handling to avoid promoting bad practices.Apply this diff to improve the example:
- tokenString, _ := token.SignedString(secret) - return tokenString + tokenString, err := token.SignedString(secret) + if err != nil { + panic(err) // Handle appropriately in production + } + return tokenStringdocs/TROUBLESHOOTING.md (1)
44-50: Base64 decoding command may fail on some systems.The JWT decode command uses
base64 -d, but JWT tokens use base64url encoding (not standard base64), which can cause decoding issues. Additionally, base64 command syntax differs between systems (macOS uses-D, Linux uses-d).Consider adding a note about potential decoding issues or using a more robust approach:
# Note: JWT uses base64url encoding. This may produce errors but often works for inspection. # For production use, use a proper JWT library or online decoder. echo "<token>" | cut -d. -f2 | base64 -d 2>/dev/null | jq .expOr reference a JWT debugging tool like jwt.io for more reliable decoding.
middleware_compatibility_test.go (1)
70-81: Consider handling the SignedString error in tests.Line 80 ignores the error from
token.SignedString(cfg.JWTSecret). While acceptable in test code for brevity, handling the error would make test failures clearer if token signing fails unexpectedly.- tokenString, _ := token.SignedString(cfg.JWTSecret) + tokenString, err := token.SignedString(cfg.JWTSecret) + if err != nil { + t.Fatalf("Failed to sign token: %v", err) + }security_scenarios_test.go (1)
119-132: HTTPS enforcement test validates local logic, not production pathThis test recomputes requiresHTTPS rather than invoking the function that enforces scheme (e.g., redirect/validation in handlers). Prefer exercising the real function to catch regressions.
- isLocalhost := isLocalhostURI(tt.uri) - parsed, _ := url.Parse(tt.uri) - requiresHTTPS := !isLocalhost && parsed.Scheme == "http" + isLocalhost := isLocalhostURI(tt.uri) + parsed, err := url.Parse(tt.uri) + if err != nil { + t.Fatalf("failed to parse URI: %v", err) + } + requiresHTTPS := !isLocalhost && parsed.Scheme == "http" + // TODO: call the real enforcement/validation function used in handlers, + // e.g., validateRedirectURI(parsed) or similar, and assert on its error/result.Point me to the enforcement function and I’ll wire the assertions.
examples/embedded/main.go (2)
90-107: Reuse provided HTTP context extractorYou reimplemented Bearer extraction. Use oauth.CreateHTTPContextFunc() to reduce duplication and keep behavior consistent.
- // Setup MCP endpoint with OAuth context extraction - oauthContextFunc := func(ctx context.Context, r *http.Request) context.Context { - authHeader := r.Header.Get("Authorization") - if authHeader != "" { - token := authHeader - if len(authHeader) > 7 && authHeader[:7] == "Bearer " { - token = authHeader[7:] - } - ctx = oauth.WithOAuthToken(ctx, token) - } - return ctx - } + // Setup MCP endpoint with OAuth context extraction + oauthContextFunc := oauth.CreateHTTPContextFunc()
21-28: Security note: demo secretStatic HS256 secret is fine for examples; add a brief comment warning not to use static/shared secrets in production and to source from env/secret manager.
cfg := &oauth.Config{ Mode: "native", Provider: "hmac", Issuer: "https://test.example.com", Audience: "api://test-mcp-server", - JWTSecret: []byte("test-secret-key-must-be-32-bytes-long!"), + // Demo only — do not hardcode in production; load from a secret manager/env. + JWTSecret: []byte("test-secret-key-must-be-32-bytes-long!"), }examples/simple/main.go (2)
72-76: Return an MCP error result for unauthenticated caseMinor consistency with other example; return a tool error result instead of a Go error.
- if !ok { - // This should never happen if OAuth is working correctly - return nil, fmt.Errorf("authentication required") - } + if !ok { + // This should never happen if OAuth is working correctly + return mcp.NewToolResultError("authentication required"), nil + }
35-41: Example config noteAdd a short comment reminding that HMAC is for local/testing, and OIDC providers should be used in prod.
oauthOption, err := oauth.WithOAuth(mux, &oauth.Config{ - Provider: "hmac", // or "okta", "google", "azure" + Provider: "hmac", // For local/testing; prefer OIDC ("okta","google","azure") in prodphase3_test.go (3)
52-60: Assert success status for metadata endpointChecking only NotFound can miss failures. Assert StatusOK.
- if w.Code == http.StatusNotFound { - t.Error("OAuth metadata endpoint not registered") - } + if w.Code != http.StatusOK { + t.Errorf("OAuth metadata endpoint unexpected status: got %d, want %d", w.Code, http.StatusOK) + }
160-163: Check SignedString errorDon’t ignore signing errors; fail fast for clarity.
- tokenString, _ := token.SignedString(cfg.JWTSecret) + tokenString, err := token.SignedString(cfg.JWTSecret) + if err != nil { + t.Fatalf("Failed to sign token: %v", err) + }
63-75: Proxy mode provider choiceTest uses Mode "proxy" with Provider "hmac". If proxy mode expects OIDC providers, please confirm this combination is valid or add a negative test expecting error.
provider/provider_test.go (3)
36-49: Reduce brittleness: assert errors by type/contains, not exact stringComparing full error strings makes tests fragile. Prefer sentinel errors or substring checks.
- if err != nil { - t.Errorf("Expected valid token to pass, got error: %v", err) - } + if err != nil { + t.Fatalf("expected valid token, got error: %v", err) + }Apply similar tightening across this file; for negative cases, prefer strings.Contains or errors.Is.
60-73: Use helper for token signing and check errorsAvoid repeating token creation and ignored errors; create a mustSign helper.
+// test helper +func mustSignHS256(t *testing.T, claims jwt.MapClaims, secret []byte) string { + t.Helper() + token := jwt.NewWithClaims(jwt.SigningMethodHS256, claims) + s, err := token.SignedString(secret) + if err != nil { + t.Fatalf("failed to sign token: %v", err) + } + return s +}Then:
- tokenString, err := token.SignedString([]byte(cfg.JWTSecret)) - if err != nil { - t.Fatalf("Failed to sign token: %v", err) - } + tokenString := mustSignHS256(t, jwt.MapClaims{ + "sub": "test-user", + "aud": "test-service-audience", + "exp": time.Now().Add(time.Hour).Unix(), + "iat": time.Now().Unix(), + "email": "test@example.com", + }, cfg.JWTSecret)
70-73: Error message equality checksReplace exact equality with substring checks to tolerate minor wording changes.
- if err != nil && err.Error() != "audience validation failed: invalid audience: expected test-service-audience, got wrong.audience.com" { + if err != nil && !strings.Contains(err.Error(), "audience validation failed") { t.Errorf("Expected specific audience error, got: %v", err) }Repeat similarly for other exact matches.
Also applies to: 141-144, 162-165, 180-183
phase2_integration_test.go (3)
45-46: Check SignedString errors (multiple sites)Several tests ignore token.SignedString errors; fail fast for clarity.
- tokenString, _ := token.SignedString(cfg.JWTSecret) + tokenString, err := token.SignedString(cfg.JWTSecret) + if err != nil { + t.Fatalf("failed to sign token: %v", err) + }Repeat similarly at Lines 89-90 and 191-191.
Also applies to: 89-90, 191-191
20-58: Reduce duplication with a shared token helper and t.ParallelIntroduce a mustSign helper and consider t.Parallel for subtests to speed CI.
+func mustSignHS256(t *testing.T, claims jwt.MapClaims, secret []byte) string { + t.Helper() + token := jwt.NewWithClaims(jwt.SigningMethodHS256, claims) + s, err := token.SignedString(secret) + if err != nil { + t.Fatalf("sign failed: %v", err) + } + return s +}Then replace repeated token creation blocks with mustSignHS256 calls; add t.Parallel() at the start of independent t.Run bodies.
Also applies to: 60-101, 103-131, 133-225, 227-248, 250-348
156-163: Capture user deterministicallycapturedUser is set inside handler; race-free in tests, but add assertion that middleware injected user by verifying result content or by returning an error when user missing to tighten the contract.
middleware.go (5)
120-133: Avoid double hashing; reuse tokenHash for preview.You compute SHA‑256 twice. Reuse the first hash and slice for preview.
- // Create token hash for caching - tokenHash := fmt.Sprintf("%x", sha256.Sum256([]byte(tokenString))) + // Create token hash for caching + tokenHash := fmt.Sprintf("%x", sha256.Sum256([]byte(tokenString))) // Check cache first if cached, exists := s.cache.getCachedToken(tokenHash); exists { s.logger.Info("Using cached authentication for tool: %s (user: %s)", req.Params.Name, cached.User.Username) ctx = context.WithValue(ctx, userContextKey, cached.User) return next(ctx, req) } // Log token hash for debugging (prevents sensitive data exposure) - tokenHashFull := fmt.Sprintf("%x", sha256.Sum256([]byte(tokenString))) - tokenHashPreview := tokenHashFull[:16] + "..." + tokenHashPreview := tokenHash[:16] + "..." s.logger.Info("Validating token for tool %s (hash: %s)", req.Params.Name, tokenHashPreview)
63-67: Delete expired cache entries synchronously to avoid goroutine storms.Spawning a goroutine per expired check can create unbounded goroutines under load. Delete inline after releasing the read lock.
- if time.Now().After(cached.ExpiresAt) { - tc.mu.RUnlock() - // Schedule expired token deletion in a separate operation - go tc.deleteExpiredToken(tokenHash) - return nil, false - } + if time.Now().After(cached.ExpiresAt) { + tc.mu.RUnlock() + // Delete expired token synchronously + tc.deleteExpiredToken(tokenHash) + return nil, false + }Also applies to: 75-83
142-149: Make cache TTL configurable and align with token expiry if available.Hardcoding 5m may over-accept revoked/expired tokens or cause unnecessary revalidations.
- Add Config.CacheTTL time.Duration (default 5m).
- If the validator can surface token exp, set ExpiresAt = min(now+CacheTTL, tokenExp).
If changing the interface is deferred, at least thread TTL through Server.
Example:
- // Cache the validation result (expire in 5 minutes) - expiresAt := time.Now().Add(5 * time.Minute) + ttl := 5 * time.Minute + if s.config != nil && s.config.CacheTTL > 0 { + ttl = s.config.CacheTTL + } + expiresAt := time.Now().Add(ttl) s.cache.setCachedToken(tokenHash, user, expiresAt)
21-27: Prefer struct keys for context to avoid accidental collisions.Typed strings are okay, but unexported empty-struct keys are the common pattern.
-type contextKey string +type contextKey struct{} const ( - oauthTokenKey contextKey = "oauth_token" - userContextKey contextKey = "user" + oauthTokenKey contextKey = contextKey{} + userContextKey contextKey = contextKey{} )
218-236: Optional: use pluggable Logger here for consistency.This file otherwise uses Server.logger, but this helper uses log.Printf. Consider a variant that accepts a Logger.
Example addition (outside diff above):
func CreateHTTPContextFuncWithLogger(l Logger) func(context.Context, *http.Request) context.Context { return func(ctx context.Context, r *http.Request) context.Context { // identical parsing; replace log.Printf with l.Info(...) } }docs/implementation.md (2)
596-603: Fix markdownlint issues (headings and fenced code languages).
- Add language to final fenced block (MD040).
- Replace emphasis-as-heading with proper headings at lines 206, 213, 220, 227 (MD036).
Example:
-``` +```text # OAuth MCP Proxy - Implementation Log ...And convert emphasized headings to `###` where appropriate. --- `468-476`: **Nit: naming consistency in examples (mcpserver vs server).** Examples refer to mcpserver.WithHTTPContextFunc while code imports server. Align naming to avoid confusion. </blockquote></details> </blockquote></details> <details> <summary>📜 Review details</summary> **Configuration used**: CodeRabbit UI **Review profile**: CHILL **Plan**: Pro <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between f7bed0558e03958ba21d0120c23b7e158694c0ac and 389a1a33dbd44bd337924b76d984ea4f6a17e994. </details> <details> <summary>⛔ Files ignored due to path filters (1)</summary> * `go.sum` is excluded by `!**/*.sum` </details> <details> <summary>📒 Files selected for processing (44)</summary> * `.github/workflows/release.yml` (1 hunks) * `.github/workflows/test.yml` (1 hunks) * `.gitignore` (1 hunks) * `.goreleaser.yml` (1 hunks) * `CHANGELOG.md` (1 hunks) * `LICENSE` (1 hunks) * `Makefile` (1 hunks) * `README.md` (1 hunks) * `attack_scenarios_test.go` (1 hunks) * `config.go` (1 hunks) * `context_propagation_test.go` (1 hunks) * `docs/CLIENT-SETUP.md` (1 hunks) * `docs/CONFIGURATION.md` (1 hunks) * `docs/MIGRATION.md` (1 hunks) * `docs/RELEASING.md` (1 hunks) * `docs/SECURITY.md` (1 hunks) * `docs/TROUBLESHOOTING.md` (1 hunks) * `docs/implementation.md` (1 hunks) * `docs/oauth.md` (1 hunks) * `docs/plan-standalone.md` (1 hunks) * `docs/plan.md` (1 hunks) * `docs/providers/AZURE.md` (1 hunks) * `docs/providers/GOOGLE.md` (1 hunks) * `docs/providers/HMAC.md` (1 hunks) * `docs/providers/OKTA.md` (1 hunks) * `examples/README.md` (1 hunks) * `examples/embedded/main.go` (1 hunks) * `examples/simple/main.go` (1 hunks) * `fixed_redirect_test.go` (1 hunks) * `go.mod` (1 hunks) * `handlers.go` (1 hunks) * `logger.go` (1 hunks) * `metadata.go` (1 hunks) * `metadata_test.go` (1 hunks) * `middleware.go` (1 hunks) * `middleware_compatibility_test.go` (1 hunks) * `oauth.go` (1 hunks) * `phase2_integration_test.go` (1 hunks) * `phase3_test.go` (1 hunks) * `provider/provider.go` (1 hunks) * `provider/provider_test.go` (1 hunks) * `security_scenarios_test.go` (1 hunks) * `security_test.go` (1 hunks) * `state_test.go` (1 hunks) </details> <details> <summary>🧰 Additional context used</summary> <details> <summary>🧬 Code graph analysis (18)</summary> <details> <summary>metadata_test.go (1)</summary><blockquote> <details> <summary>handlers.go (2)</summary> * `OAuth2Config` (38-63) * `OAuth2Handler` (26-30) </details> </blockquote></details> <details> <summary>state_test.go (1)</summary><blockquote> <details> <summary>handlers.go (2)</summary> * `OAuth2Handler` (26-30) * `OAuth2Config` (38-63) </details> </blockquote></details> <details> <summary>middleware_compatibility_test.go (3)</summary><blockquote> <details> <summary>config.go (1)</summary> * `Config` (10-34) </details> <details> <summary>oauth.go (1)</summary> * `NewServer` (39-72) </details> <details> <summary>middleware.go (2)</summary> * `GetUserFromContext` (199-202) * `WithOAuthToken` (42-44) </details> </blockquote></details> <details> <summary>context_propagation_test.go (4)</summary><blockquote> <details> <summary>config.go (1)</summary> * `Config` (10-34) </details> <details> <summary>provider/provider.go (3)</summary> * `Config` (32-38) * `TokenValidator` (41-44) * `OIDCValidator` (54-59) </details> <details> <summary>oauth.go (1)</summary> * `NewServer` (39-72) </details> <details> <summary>middleware.go (2)</summary> * `WithOAuthToken` (42-44) * `GetUserFromContext` (199-202) </details> </blockquote></details> <details> <summary>attack_scenarios_test.go (1)</summary><blockquote> <details> <summary>handlers.go (2)</summary> * `OAuth2Handler` (26-30) * `OAuth2Config` (38-63) </details> </blockquote></details> <details> <summary>examples/embedded/main.go (2)</summary><blockquote> <details> <summary>oauth.go (1)</summary> * `NewServer` (39-72) </details> <details> <summary>middleware.go (2)</summary> * `GetUserFromContext` (199-202) * `WithOAuthToken` (42-44) </details> </blockquote></details> <details> <summary>metadata.go (1)</summary><blockquote> <details> <summary>handlers.go (1)</summary> * `OAuth2Handler` (26-30) </details> </blockquote></details> <details> <summary>config.go (3)</summary><blockquote> <details> <summary>provider/provider.go (5)</summary> * `Logger` (24-29) * `Config` (32-38) * `TokenValidator` (41-44) * `HMACValidator` (47-51) * `OIDCValidator` (54-59) </details> <details> <summary>logger.go (1)</summary> * `Logger` (22-27) </details> <details> <summary>handlers.go (3)</summary> * `OAuth2Handler` (26-30) * `NewOAuth2ConfigFromConfig` (164-196) * `NewOAuth2Handler` (66-128) </details> </blockquote></details> <details> <summary>middleware.go (2)</summary><blockquote> <details> <summary>provider/provider.go (2)</summary> * `User` (17-21) * `TokenValidator` (41-44) </details> <details> <summary>oauth.go (1)</summary> * `Server` (17-23) </details> </blockquote></details> <details> <summary>security_test.go (1)</summary><blockquote> <details> <summary>handlers.go (2)</summary> * `OAuth2Config` (38-63) * `OAuth2Handler` (26-30) </details> </blockquote></details> <details> <summary>provider/provider.go (2)</summary><blockquote> <details> <summary>middleware.go (1)</summary> * `User` (19-19) </details> <details> <summary>config.go (1)</summary> * `Config` (10-34) </details> </blockquote></details> <details> <summary>phase2_integration_test.go (4)</summary><blockquote> <details> <summary>config.go (1)</summary> * `Config` (10-34) </details> <details> <summary>provider/provider.go (4)</summary> * `Config` (32-38) * `HMACValidator` (47-51) * `User` (17-21) * `OIDCValidator` (54-59) </details> <details> <summary>oauth.go (1)</summary> * `NewServer` (39-72) </details> <details> <summary>middleware.go (3)</summary> * `User` (19-19) * `GetUserFromContext` (199-202) * `WithOAuthToken` (42-44) </details> </blockquote></details> <details> <summary>provider/provider_test.go (2)</summary><blockquote> <details> <summary>config.go (1)</summary> * `Config` (10-34) </details> <details> <summary>provider/provider.go (3)</summary> * `Config` (32-38) * `HMACValidator` (47-51) * `OIDCValidator` (54-59) </details> </blockquote></details> <details> <summary>oauth.go (5)</summary><blockquote> <details> <summary>config.go (2)</summary> * `Config` (10-34) * `CreateOAuth2Handler` (147-153) </details> <details> <summary>provider/provider.go (3)</summary> * `Config` (32-38) * `TokenValidator` (41-44) * `Logger` (24-29) </details> <details> <summary>middleware.go (2)</summary> * `TokenCache` (30-33) * `CachedToken` (36-39) </details> <details> <summary>handlers.go (1)</summary> * `OAuth2Handler` (26-30) </details> <details> <summary>logger.go (1)</summary> * `Logger` (22-27) </details> </blockquote></details> <details> <summary>phase3_test.go (2)</summary><blockquote> <details> <summary>oauth.go (1)</summary> * `WithOAuth` (112-124) </details> <details> <summary>middleware.go (3)</summary> * `GetUserFromContext` (199-202) * `CreateHTTPContextFunc` (217-236) * `GetOAuthToken` (47-50) </details> </blockquote></details> <details> <summary>examples/simple/main.go (4)</summary><blockquote> <details> <summary>oauth.go (1)</summary> * `WithOAuth` (112-124) </details> <details> <summary>config.go (1)</summary> * `Config` (10-34) </details> <details> <summary>provider/provider.go (1)</summary> * `Config` (32-38) </details> <details> <summary>middleware.go (2)</summary> * `GetUserFromContext` (199-202) * `CreateHTTPContextFunc` (217-236) </details> </blockquote></details> <details> <summary>handlers.go (2)</summary><blockquote> <details> <summary>config.go (1)</summary> * `Config` (10-34) </details> <details> <summary>logger.go (1)</summary> * `Logger` (22-27) </details> </blockquote></details> <details> <summary>security_scenarios_test.go (1)</summary><blockquote> <details> <summary>handlers.go (2)</summary> * `OAuth2Handler` (26-30) * `OAuth2Config` (38-63) </details> </blockquote></details> </details><details> <summary>🪛 ast-grep (0.39.6)</summary> <details> <summary>handlers.go</summary> [warning] 237-237: MinVersion` is missing from this TLS configuration. By default, TLS 1.2 is currently used as the minimum when acting as a client, and TLS 1.0 when acting as a server. General purpose web applications should default to TLS 1.3 with all other protocols disabled. Only where it is known that a web server must support legacy clients with unsupported an insecure browsers (such as Internet Explorer 10), it may be necessary to enable TLS 1.0 to provide support. Add `MinVersion: tls.VersionTLS13' to the TLS configuration to bump the minimum version to TLS 1.3. Context: tls.Config{InsecureSkipVerify: false} Note: [CWE-327]: Use of a Broken or Risky Cryptographic Algorithm [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES] https://owasp.org/Top10/A02_2021-Cryptographic_Failures (missing-ssl-minversion-go) </details> </details> <details> <summary>🪛 checkmake (0.2.2)</summary> <details> <summary>Makefile</summary> [warning] 52-52: Target body for "help" exceeds allowed length of 5 (13). (maxbodylength) --- [warning] 1-1: Missing required phony target "all" (minphony) </details> </details> <details> <summary>🪛 LanguageTool</summary> <details> <summary>README.md</summary> [style] ~253-~253: Using many exclamation marks might seem excessive (in this case: 6 exclamation marks for a text that’s 4111 characters long) Context: ...er v0.1.0 release, contributions welcome! **Report issues:** [GitHub Issues](htt... (EN_EXCESSIVE_EXCLAMATION) </details> <details> <summary>docs/TROUBLESHOOTING.md</summary> [grammar] ~374-~374: Ensure spelling is correct Context: ... 3. **Provider can't reach callback URL:** - ServerURL must be publicly accessible - Firewall ... (QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1) </details> <details> <summary>docs/plan.md</summary> [style] ~53-~53: Consider using a different adjective in this context to strengthen your wording. Context: ...c:** Industry-standard OIDC library (no good alternatives) - **jwt:** Standard Go JW... (GOOD_ALTERNATIVE) --- [grammar] ~405-~405: Ensure spelling is correct Context: ...gle, Azure AD - Token caching with TTL (5min default) - **OAuth modes: native + prox... (QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1) </details> </details> <details> <summary>🪛 markdownlint-cli2 (0.18.1)</summary> <details> <summary>docs/TROUBLESHOOTING.md</summary> 30-30: Fenced code blocks should have a language specified (MD040, fenced-code-language) --- 200-200: Fenced code blocks should have a language specified (MD040, fenced-code-language) --- 249-249: Fenced code blocks should have a language specified (MD040, fenced-code-language) </details> <details> <summary>docs/plan.md</summary> 206-206: Emphasis used instead of a heading (MD036, no-emphasis-as-heading) --- 213-213: Emphasis used instead of a heading (MD036, no-emphasis-as-heading) --- 220-220: Emphasis used instead of a heading (MD036, no-emphasis-as-heading) --- 227-227: Emphasis used instead of a heading (MD036, no-emphasis-as-heading) </details> <details> <summary>docs/oauth.md</summary> 206-206: Emphasis used instead of a heading (MD036, no-emphasis-as-heading) --- 213-213: Emphasis used instead of a heading (MD036, no-emphasis-as-heading) --- 220-220: Emphasis used instead of a heading (MD036, no-emphasis-as-heading) --- 227-227: Emphasis used instead of a heading (MD036, no-emphasis-as-heading) --- 599-599: Fenced code blocks should have a language specified (MD040, fenced-code-language) --- 659-659: Emphasis used instead of a heading (MD036, no-emphasis-as-heading) --- 665-665: Emphasis used instead of a heading (MD036, no-emphasis-as-heading) --- 671-671: Emphasis used instead of a heading (MD036, no-emphasis-as-heading) --- 677-677: Emphasis used instead of a heading (MD036, no-emphasis-as-heading) --- 683-683: Emphasis used instead of a heading (MD036, no-emphasis-as-heading) --- 745-745: Emphasis used instead of a heading (MD036, no-emphasis-as-heading) --- 753-753: Emphasis used instead of a heading (MD036, no-emphasis-as-heading) --- 761-761: Emphasis used instead of a heading (MD036, no-emphasis-as-heading) --- 768-768: Emphasis used instead of a heading (MD036, no-emphasis-as-heading) </details> <details> <summary>docs/CONFIGURATION.md</summary> 346-346: Fenced code blocks should have a language specified (MD040, fenced-code-language) </details> <details> <summary>docs/SECURITY.md</summary> 156-156: Fenced code blocks should have a language specified (MD040, fenced-code-language) --- 245-245: Fenced code blocks should have a language specified (MD040, fenced-code-language) --- 325-325: Fenced code blocks should have a language specified (MD040, fenced-code-language) --- 330-330: Fenced code blocks should have a language specified (MD040, fenced-code-language) --- 383-383: Fenced code blocks should have a language specified (MD040, fenced-code-language) </details> <details> <summary>docs/implementation.md</summary> 206-206: Emphasis used instead of a heading (MD036, no-emphasis-as-heading) --- 213-213: Emphasis used instead of a heading (MD036, no-emphasis-as-heading) --- 220-220: Emphasis used instead of a heading (MD036, no-emphasis-as-heading) --- 227-227: Emphasis used instead of a heading (MD036, no-emphasis-as-heading) --- 599-599: Fenced code blocks should have a language specified (MD040, fenced-code-language) </details> </details> </details> <details> <summary>🔇 Additional comments (48)</summary><blockquote> <details> <summary>LICENSE (1)</summary><blockquote> `1-21`: **LGTM! Standard MIT License.** The MIT License text is properly formatted with the correct copyright holder and year. </blockquote></details> <details> <summary>docs/plan.md (1)</summary><blockquote> `1-502`: **Comprehensive implementation plan.** This planning document provides clear phases, architecture decisions, and success criteria for the OAuth MCP Proxy library. The document is well-structured and aligns with the PR's objectives. A few minor observations: - The phased approach (Phase 0-6) provides a clear roadmap - The distinction between native and proxy modes is well-explained - The decision to fix critical architecture issues (global state, logging, validation) in Phase 1.5 is sound </blockquote></details> <details> <summary>.gitignore (1)</summary><blockquote> `1-37`: **LGTM! Comprehensive gitignore patterns.** The patterns appropriately cover Go binaries, test outputs, IDE artifacts, OS files, and environment variables. Project-specific binaries from examples are correctly excluded. </blockquote></details> <details> <summary>state_test.go (1)</summary><blockquote> `134-158`: **LGTM! Comprehensive localhost detection tests.** The test cases cover all relevant localhost formats (localhost, 127.0.0.1, IPv6 [::1]) and important edge cases like subdomains and invalid URIs. </blockquote></details> <details> <summary>examples/README.md (4)</summary><blockquote> `5-14`: **LGTM! Clear and concise quick start example.** The 3-line example effectively demonstrates the simplicity of integrating OAuth protection. --- `20-64`: **LGTM! Complete working example with clear instructions.** The Simple API example demonstrates all necessary steps including OAuth setup, MCP server creation, and endpoint configuration. The test command is helpful. --- `66-92`: **LGTM! Clear guidance on API choices.** The distinction between production usage (simple API) and learning internals (advanced API) is well-explained. The comparison table helps users make informed choices. --- `94-160`: **LGTM! Comprehensive provider examples and logging guidance.** The provider configurations cover all supported types with correct parameters. The custom logging section clearly demonstrates the Logger interface implementation and documents what gets logged. </blockquote></details> <details> <summary>security_test.go (5)</summary><blockquote> `10-68`: **LGTM! Comprehensive redirect URI validation tests.** The tests cover critical security scenarios including empty allowlists, exact matches, and partial match rejection. This is essential for preventing open redirect vulnerabilities. --- `70-143`: **LGTM! Important parameter validation tests.** These tests verify protection against oversized OAuth parameters, which is important for preventing DoS attacks and protocol violations. --- `145-164`: **LGTM! Security headers validation.** The test verifies essential security headers are set correctly (X-Content-Type-Options, X-Frame-Options, Cache-Control), which helps prevent XSS, clickjacking, and caching of sensitive OAuth data. --- `166-212`: **LGTM! Native mode enforcement tests.** The tests verify that OAuth proxy endpoints correctly return 404 with appropriate messaging in native mode, ensuring the mode distinction is properly enforced. --- `214-267`: **LGTM! JWKS endpoint behavior tests.** The tests verify correct JWKS endpoint behavior: native mode blocks access with appropriate messaging, and HMAC provider returns empty key set (as HMAC doesn't use public keys). </blockquote></details> <details> <summary>logger.go (2)</summary><blockquote> `22-27`: **LGTM! Clean and well-documented Logger interface.** The Logger interface follows standard logging patterns and provides clear documentation with usage examples. The four levels (Debug, Info, Warn, Error) are appropriate for OAuth logging needs. --- `29-46`: **LGTM! Simple and effective default logger.** The defaultLogger implementation appropriately delegates to the standard library `log` package with level prefixes, providing sensible default behavior when no custom logger is provided. </blockquote></details> <details> <summary>attack_scenarios_test.go (2)</summary><blockquote> `8-117`: **LGTM! Excellent security scenario coverage.** The four attack scenarios provide comprehensive coverage of the security model: 1. Authorization-time rejection of malicious redirects 2. Defense-in-depth against leaked signing keys 3. Legitimate localhost flow validation 4. Subdomain attack prevention These tests effectively demonstrate the multi-layered security approach. --- `119-144`: **LGTM! Valuable security documentation.** This test serves as executable documentation of the defense-in-depth security layers. While not a functional test, it provides valuable context for understanding the security architecture and can help with security audits. </blockquote></details> <details> <summary>oauth.go (1)</summary><blockquote> `86-94`: **Ensure HTTP method checks inside handlers.** http.ServeMux doesn’t enforce methods. Confirm s.handler.* reject unexpected methods (e.g., only GET for discovery/JWKS, POST for token) to reduce attack surface. Would you like a quick grep script to scan handlers for method checks?<!-- review_comment_end --> </blockquote></details> <details> <summary>config.go (1)</summary><blockquote> `146-153`: **CreateOAuth2Handler: default logger path OK.** Looks correct and consistent with handlers.go usage. <!-- review_comment_end --> </blockquote></details> <details> <summary>context_propagation_test.go (2)</summary><blockquote> `140-159`: **Interface assertion test looks fine.** Good lightweight compile-time guard that OIDCValidator satisfies TokenValidator signature. <!-- review_comment_end --> --- `195-206`: **Nice end-to-end context capture.** Middleware-to-handler propagation and user injection check is solid. <!-- review_comment_end --> </blockquote></details> <details> <summary>provider/provider.go (1)</summary><blockquote> `197-206`: **The review comment is based on an incorrect assumption about go-oidc semantics and misses a critical bug.** The core issue: go-oidc's `oidc.Config.ClientID` field is used to validate the ID token's `aud` (audience) claim. oidc.Config.ClientID is the expected audience (aud) for ID tokens — the verifier checks that the token's aud array contains that ClientID. Since the `Config` struct only has an `Audience` field (not a separate `ClientID` field), passing `cfg.Audience` to `ClientID` is **semantically correct** for validating that the token's audience matches the expected value. However, there is a **critical bug** the original review missed: on line 206, `v.logger.Info(...)` is called, but `v.logger` is **never initialized** in the `Initialize()` method. The `Config` struct has a `Logger` field, but it is never assigned to `v.logger`. This causes a nil pointer dereference when the log statement executes. **Required fix (line ~206):** Add `v.logger = cfg.Logger` in the `Initialize()` method after the issuer/audience validation checks, before calling `v.logger.Info()`. > Likely an incorrect or invalid review comment. </blockquote></details> <details> <summary>docs/SECURITY.md (1)</summary><blockquote> `1-468`: **Comprehensive security documentation aligns with OAuth 2.1 best practices.** This file provides excellent coverage of production security considerations including secrets management, JWT validation, HTTPS/TLS deployment, audience validation, PKCE, redirect URI security, token handling, logging, rate limiting, and security headers. The examples are concrete and actionable. Based on retrieved learnings, the recommendations align with current industry standards for jwt/v5.3.0, oauth2 v0.32.0, and OIDC v3.16.0 usage patterns. </blockquote></details> <details> <summary>README.md (2)</summary><blockquote> `227-227`: **Verify release version consistency.** Line 227 states "Current Release: v0.0.1 (Preview)" but docs/RELEASING.md and examples throughout the PR reference v0.1.0 as the first stable release target. Clarify which version is correct for this PR: - If v0.0.1 is an intermediate preview release, confirm v0.1.0 is still the planned stable release. - If v0.1.0 is the actual current version, update line 227 accordingly. --- `1-261`: **Well-structured README with clear architecture and comprehensive guidance.** The entry point effectively guides users through setup with good visual aids (Mermaid diagrams), clear architecture explanation, and prominent links to all necessary documentation. Dependency versions are correctly pinned and align with current, well-maintained libraries. </blockquote></details> <details> <summary>docs/providers/GOOGLE.md (1)</summary><blockquote> `1-220`: **Provider guide is well-structured and comprehensive.** The Google provider guide follows a clear pattern with setup instructions, configuration examples for both native and proxy modes, testing guidance, user claims mapping, and troubleshooting. Configuration examples properly reference environment variables and highlight key differences from other providers (Client ID as audience). The checklist items are appropriate for production deployment. </blockquote></details> <details> <summary>docs/RELEASING.md (1)</summary><blockquote> `1-278`: **Clear and comprehensive release process documentation for maintainers.** The guide effectively explains both automated and manual release workflows, pkg.go.dev publishing mechanics, versioning strategy, and rollback procedures. The pre/release/post checklists are thorough, and the warning about pkg.go.dev immutability is important. Support policy is clearly defined for different release cycles. The release workflow referenced aligns well with the actual .github/workflows/release.yml. </blockquote></details> <details> <summary>docs/providers/AZURE.md (1)</summary><blockquote> `1-255`: **Comprehensive Azure AD provider guide with helpful multi-tenant coverage.** The guide clearly explains Azure AD setup, configuration for native and proxy modes, and audience options. The multi-tenant support section (lines 204-216) and AADSTS troubleshooting section are particularly valuable. Production checklist includes Azure-specific security considerations like Key Vault, Conditional Access, and monitoring. Structure and clarity align well with other provider guides in the documentation suite. </blockquote></details> <details> <summary>docs/MIGRATION.md (1)</summary><blockquote> `1-357`: **Excellent migration guide with clear before/after examples and helpful rollback plan.** The guide effectively communicates breaking changes (import path, API redesign) while demonstrating significant API simplification (from ~40 to ~20 lines). Configuration mapping table, new features documentation, and pragmatic rollback guidance make migration low-risk. The transition from internal mcp-trino OAuth to public oauth-mcp-proxy library is well-framed as a positive upgrade with maintained token validation logic but improved packaging and usability. </blockquote></details> <details> <summary>docs/CONFIGURATION.md (1)</summary><blockquote> `1-496`: **Comprehensive configuration reference covering all options and use cases.** The guide effectively documents all Config fields with clear descriptions, examples, and security notes. Mode auto-detection logic is well-explained, Logger interface examples aid integration with popular logging libraries, and the environment variable pattern provides a ready-to-use template. Complete examples for each provider and mode combination (HMAC, Okta native/proxy, Google, Azure) cover realistic deployment scenarios. </blockquote></details> <details> <summary>.github/workflows/release.yml (3)</summary><blockquote> `20-20`: **Verify Go version 1.25 against go.mod compatibility.** Line 20 sets `GO_VERSION: "1.25"` for the release workflow. Based on retrieved learnings, coreos/go-oidc v3.16.0 requires Go 1.24 minimum. Verify that 1.25 is compatible with all dependencies in go.mod and is the intended minimum version for releases. If you need to support Go 1.24 or earlier, adjust this version accordingly. --- `44-50`: **Verify anothrNick/github-tag-action@v1 is actively maintained.** Line 44 uses `anothrNick/github-tag-action@v1` for version bumping and tagging. This action is not from an official GitHub organization. Verify that: 1. The action is actively maintained and receives security updates. 2. Version `v1` is pinned to a specific commit SHA for supply-chain security. 3. This is the intended action for your release process (alternatives: google-github-actions/release-please or similar). Consider using a more recent pinning strategy if available. --- `1-82`: **Release workflow structure is sound with good resilience patterns.** The workflow follows CI/CD best practices: full history checkout for changelog generation, race-detector tests, conditional execution on actual tag creation, non-fatal pkg.go.dev indexing request, and visibility through GitHub step summary. The use of `steps.tag.outputs.new_tag` to gate downstream steps prevents unnecessary goreleaser invocation. The `|| true` on the curl indexing request ensures workflow success even if pkg.go.dev is temporarily unavailable. </blockquote></details> <details> <summary>Makefile (1)</summary><blockquote> `1-65`: **LGTM! Well-structured development tooling.** The Makefile provides a comprehensive set of standard development targets with good defaults. The auto-installation of linting and security tools (golangci-lint, govulncheck) is convenient for onboarding new developers. </blockquote></details> <details> <summary>docs/providers/HMAC.md (2)</summary><blockquote> `77-120`: **LGTM! Strong security guidance.** The security recommendations are excellent: - Appropriate minimum secret length (32 bytes) for HS256 - Crypto/rand usage for secure generation - Clear warnings against weak secrets - Sensible token expiration recommendations --- `122-166`: **LGTM! Clear testing instructions and honest limitations.** The testing section provides practical step-by-step guidance, and the limitations section honestly addresses when HMAC authentication is not appropriate (e.g., public-facing applications, user authentication). </blockquote></details> <details> <summary>docs/providers/OKTA.md (1)</summary><blockquote> `1-223`: **LGTM! Comprehensive Okta integration guide.** Excellent documentation covering all aspects of Okta integration: - Clear setup instructions in the Okta portal - Configuration examples for both native and proxy modes - Audience configuration with multiple approaches - Provider-specific troubleshooting - Production deployment checklist The documentation aligns well with the provider patterns established in the codebase. </blockquote></details> <details> <summary>docs/CLIENT-SETUP.md (1)</summary><blockquote> `1-396`: **LGTM! Excellent client-side integration guide.** This is a comprehensive and well-structured guide that covers: - Auto-discovery with clear sequence diagrams - Multiple configuration approaches (auto, manual, proxy) - OAuth metadata endpoint details (RFC 8414 compliance) - Deployment configurations for Kubernetes and Docker - Practical testing and troubleshooting steps The guide makes it easy for developers to integrate MCP clients with OAuth-enabled servers. </blockquote></details> <details> <summary>docs/TROUBLESHOOTING.md (1)</summary><blockquote> `95-494`: **LGTM! Comprehensive troubleshooting coverage.** The guide covers all major error categories with: - Clear symptom descriptions - Root cause analysis - Step-by-step solutions - Debug commands and code examples - Development vs production considerations This is an excellent resource for developers debugging OAuth issues. </blockquote></details> <details> <summary>CHANGELOG.md (1)</summary><blockquote> `1-42`: **LGTM! Well-formatted changelog.** The changelog follows Keep a Changelog and Semantic Versioning standards correctly. The v0.0.1 preview release is appropriately documented with comprehensive Added, Fixed, and Security sections. </blockquote></details> <details> <summary>middleware_compatibility_test.go (1)</summary><blockquote> `1-226`: **LGTM! Comprehensive middleware compatibility tests.** The test suite effectively validates: - Middleware integration with mcp-go v0.41.1's WithToolHandlerMiddleware - Server-wide middleware application - OAuth validation and user context propagation - Invalid token rejection The test coverage ensures the middleware API is compatible with the expected mcp-go signatures. </blockquote></details> <details> <summary>docs/oauth.md (3)</summary><blockquote> `1-275`: **LGTM! Excellent OAuth architecture documentation.** This section provides a thorough and accurate explanation of: - Native vs proxy operational modes with clear sequence diagrams - Security requirements and guarantees - Redirect URI configuration modes (fixed, allowlist, security default) - Defense-in-depth security model with four independent layers The documentation clearly explains when to use each mode and the security implications. --- `297-419`: **LGTM! Thorough attack prevention and flow documentation.** Excellent coverage of: - Attack scenarios (state tampering, open redirect, code theft) with clear prevention mechanisms - Metadata endpoint behavior in different modes - Complete OAuth flow with all security layers The documentation accurately represents the security architecture and OAuth 2.0/2.1 compliance. --- `420-814`: **LGTM! Comprehensive deployment and operational guidance.** The remaining sections provide excellent coverage of: - Production-ready configuration examples with Helm values - Detailed security model implementation (HMAC signing, localhost detection) - Kubernetes deployment architecture with multi-pod considerations - Real bug fixes with symptoms, causes, and solutions - OAuth 2.0/2.1 compliance mapping - Client compatibility matrix - Production deployment checklist This is a thorough operational guide that addresses real-world deployment scenarios and troubleshooting. </blockquote></details> <details> <summary>docs/plan-standalone.md (1)</summary><blockquote> `397-399`: **Doc freshness** Doc date/status look current and consistent with PR scope. No changes needed. </blockquote></details> <details> <summary>docs/implementation.md (2)</summary><blockquote> `39-44`: **Dependency toolchain is compatible; review comment requires correction.** The codebase specifies Go 1.25.1, which is compatible with the pinned dependencies. However, the review comment contains unverified or inaccurate claims: - The golang-jwt/jwt library aligns support with Go's version release policy, supporting a major version until two newer major releases exist—not a hard requirement for Go 1.21+. - No documentation confirms that go-oidc v3.16.0 requires Go ≥ 1.24; the claim cannot be verified. The go.mod configuration (Go 1.25.1) exceeds any reasonable minimum for both libraries and requires no changes. --- `355-395`: **Documentation line number error in middleware.go context reference; code implementation is correct.** The review comment correctly directs verification of all `ValidateToken()` call sites. All are properly updated with context parameters: - **provider/provider_test.go**: 6 call sites (lines 41, 65, 88, 112, 136, 239) — all pass `context.Background()` ✅ - **phase2_integration_test.go**: 3 call sites (lines 48, 91, 276) — all pass `context.Background()` ✅ - **middleware.go**: 1 call site (line 136) — passes request `ctx` to validator ✅ - **Interface & implementations**: Both `HMACValidator` and `OIDCValidator` properly accept context parameter; `OIDCValidator` correctly uses incoming context with timeout (line 220: `ctx, cancel := context.WithTimeout(ctx, 10*time.Second)`) ✅ **Documentation error**: Docs state middleware passes context at "line 123," but the actual call is at **line 136**. </blockquote></details> <details> <summary>middleware.go (1)</summary><blockquote> `125-127`: **Remove unnecessary nil guard—ValidateToken contract guarantees non-nil user on success.** The review comment misses that both `HMACValidator` and `OIDCValidator` return non-nil `User` pointers on success and `nil` only on error. Line 138-141 in middleware.go ensures we only proceed to line 144 (which calls `setCachedToken`) when validation succeeds, guaranteeing the cached user is never nil. This is why line 149 (`user.Username` access) is safely unguarded—the same contract applies to line 126. > Likely an incorrect or invalid review comment. </blockquote></details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
Comment on lines
+118
to
+127
| // createValidator creates the appropriate token validator based on configuration | ||
| func createValidator(cfg *Config, logger Logger) (provider.TokenValidator, error) { | ||
| // Convert root Config to provider.Config | ||
| providerCfg := &provider.Config{ | ||
| Provider: cfg.Provider, | ||
| Issuer: cfg.Issuer, | ||
| Audience: cfg.Audience, | ||
| JWTSecret: cfg.JWTSecret, | ||
| Logger: logger, | ||
| } |
There was a problem hiding this comment.
Compile-time type mismatch: oauth.Logger vs provider.Logger in createValidator.
provider.Config.Logger expects provider.Logger, but logger is oauth.Logger. This won’t compile.
One minimal fix: add an adapter type in package oauth and wrap logger when filling provider.Config.
+// logger_adapter.go (in package oauth)
+package oauth
+import "github.com/tuannvm/oauth-mcp-proxy/provider"
+
+type providerLoggerAdapter struct{ l Logger }
+func (a providerLoggerAdapter) Debug(msg string, args ...interface{}) { a.l.Debug(msg, args...) }
+func (a providerLoggerAdapter) Info(msg string, args ...interface{}) { a.l.Info(msg, args...) }
+func (a providerLoggerAdapter) Warn(msg string, args ...interface{}) { a.l.Warn(msg, args...) }
+func (a providerLoggerAdapter) Error(msg string, args ...interface{}) { a.l.Error(msg, args...) }And use it:
- providerCfg := &provider.Config{
+ pLogger := provider.Logger(nil)
+ if logger != nil {
+ if pl, ok := any(logger).(provider.Logger); ok {
+ pLogger = pl
+ } else {
+ pLogger = providerLoggerAdapter{l: logger}
+ }
+ }
+ providerCfg := &provider.Config{
Provider: cfg.Provider,
Issuer: cfg.Issuer,
Audience: cfg.Audience,
JWTSecret: cfg.JWTSecret,
- Logger: logger,
+ Logger: pLogger,
}Alternatively, dedupe the Logger interface (single shared package) if you prefer.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In config.go around lines 118 to 127, createValidator is assigning an
oauth.Logger to provider.Config.Logger which expects provider.Logger causing a
compile-time mismatch; fix by adding an adapter in the oauth package that
implements provider.Logger by delegating to the existing oauth.Logger (or
alternatively move/dedupe the Logger interface into a shared package), then wrap
the passed logger with that adapter when constructing providerCfg.Logger so
provider.Config receives a provider.Logger-compatible value.
Comment on lines
+40
to
+41
| tokenString, _ := token.SignedString(cfg.JWTSecret) | ||
|
|
There was a problem hiding this comment.
Don’t ignore errors in tests.
Unchecked errors can mask real issues.
- tokenString, _ := token.SignedString(cfg.JWTSecret)
+ tokenString, err := token.SignedString(cfg.JWTSecret)
+ if err != nil { t.Fatalf("SignedString failed: %v", err) }And check NewServer errors where currently ignored.
Also applies to: 68-69, 78-79, 112-113, 122-123, 208-210
🤖 Prompt for AI Agents
In context_propagation_test.go around lines 40-41 (and also at 68-69, 78-79,
112-113, 122-123, 208-210) you are currently calling functions like
token.SignedString and NewServer while discarding returned errors; update each
call to capture the error and handle it in the test (e.g., err :=
token.SignedString(cfg.JWTSecret); if err != nil { t.Fatalf("SignedString
failed: %v", err) } or use your test helper like require.NoError(t, err)) and do
the same for NewServer invocations so tests fail loudly on unexpected errors.
Comment on lines
+172
to
+179
| **Logging Migration Status:** | ||
| - ✅ middleware.go: Uses logger (Server.logger) - 100% migrated | ||
| - ✅ providers.go: Uses logger (validator.logger) - 100% migrated | ||
| - ⚠️ handlers.go: Still has 38 log.Printf calls (deferred to v0.2.0) | ||
| - ⚠️ metadata.go: Still has 11 log.Printf calls (deferred to v0.2.0) | ||
| - **Rationale:** Middleware is hot path (every request), handlers are infrequent (OAuth flow) | ||
|
|
||
| **Files Modified:** |
There was a problem hiding this comment.
Docs claim “middleware.go: 100% migrated to Logger” but file still uses log.Printf.
CreateHTTPContextFunc and CreateRequestAuthHook use log.Printf. Update docs or migrate those logs.
🤖 Prompt for AI Agents
In docs/implementation.md around lines 172 to 179: the doc asserts middleware.go
is 100% migrated but CreateHTTPContextFunc and CreateRequestAuthHook still call
log.Printf; either update the docs or migrate those calls. To fix, open
middleware.go, locate the log.Printf calls in CreateHTTPContextFunc and
CreateRequestAuthHook, and replace them with the package/server logger (e.g.,
Server.logger.Infof/Errorf or middleware.logger if available), ensuring the
logger is in scope (add a logger parameter or access the Server instance as
needed) and preserve the original message formatting and log level;
alternatively, if you choose not to change code, update docs/implementation.md
to remove the “100% migrated” claim or mark those functions as remaining work.
| log.Println("📋 Testing Instructions:") | ||
| log.Println() | ||
| log.Println("1. Start the server:") | ||
| log.Println(" go run examples/embedded.go") |
There was a problem hiding this comment.
Fix run command in instructions
Path should reference the example directory.
- log.Println(" go run examples/embedded.go")
+ log.Println(" go run ./examples/embedded")📝 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.
Suggested change
| log.Println(" go run examples/embedded.go") | |
| log.Println(" go run ./examples/embedded") |
🤖 Prompt for AI Agents
In examples/embedded/main.go around line 118, the printed run command currently
says "go run examples/embedded.go" which points to a non-existent file; update
the log message to reference the example directory (for example "go run
./examples/embedded" or "go run ./examples/embedded/main.go") so the instruction
correctly points to the example path.
| @@ -0,0 +1,23 @@ | |||
| module github.com/tuannvm/oauth-mcp-proxy | |||
|
|
|||
| go 1.25.1 | |||
There was a problem hiding this comment.
Invalid go directive format; fix to major.minor
The go directive should be major.minor (no patch). Use toolchain for patch if needed.
-go 1.25.1
+go 1.25
+toolchain go1.25.1As per learnings, coreos/go-oidc v3.16.0 requires Go ≥1.24, so 1.25 is fine. Based on learnings.
📝 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.
Suggested change
| go 1.25.1 | |
| go 1.25 | |
| toolchain go1.25.1 |
🤖 Prompt for AI Agents
In go.mod around line 3, the go directive uses a patch version "1.25.1" which is
invalid; change it to the major.minor form "1.25". Edit the go.mod file to
replace the go directive with "go 1.25" (no patch), then run your usual go
toolchain commands (e.g., go mod tidy) to verify modules and toolchain
compatibility.
Comment on lines
+206
to
+212
| v.logger.Info("OAuth: OIDC validator initialized with audience validation: %s", cfg.Audience) | ||
|
|
||
| v.provider = provider | ||
| v.verifier = verifier | ||
| v.audience = cfg.Audience | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Nil interface method call: v.logger.Info may panic.
v.logger is never set in Initialize, so Info on a nil interface will panic.
func (v *OIDCValidator) Initialize(cfg *Config) error {
+ v.logger = cfg.Logger // may be nil
...
- v.logger.Info("OAuth: OIDC validator initialized with audience validation: %s", cfg.Audience)
+ if v.logger != nil {
+ v.logger.Info("OAuth: OIDC validator initialized with audience validation: %s", cfg.Audience)
+ }
v.provider = provider
v.verifier = verifier
v.audience = cfg.Audience
return nil
}
```<!-- review_comment_end -->
> Committable suggestion skipped: line range outside the PR's diff.
<details>
<summary>🤖 Prompt for AI Agents</summary>
In provider/provider.go around lines 206 to 212, v.logger.Info is called but
v.logger is never initialized in Initialize so calling Info can panic; either
ensure v.logger is assigned a non-nil logger before this call (e.g., set
v.logger = cfg.Logger or a default logger at start of Initialize) or guard the
call with a nil check (if v.logger != nil { v.logger.Info(...) }) and prefer
initializing a sensible default logger so subsequent methods can safely use
v.logger.
</details>
<!-- fingerprinting:phantom:medusa:chinchilla -->
<!-- This is an auto-generated comment by CodeRabbit -->
Comment on lines
+12
to
+20
| key := make([]byte, 32) | ||
| _, _ = rand.Read(key) | ||
|
|
||
| handler := &OAuth2Handler{ | ||
| config: &OAuth2Config{ | ||
| stateSigningKey: key, | ||
| RedirectURIs: "https://mcp-server.com/oauth/callback", | ||
| }, | ||
| } |
There was a problem hiding this comment.
Seed key generation: check errors
rand.Read is ignored. Verify bytes read and err to avoid weak keys in tests.
- key := make([]byte, 32)
- _, _ = rand.Read(key)
+ key := make([]byte, 32)
+ if n, err := rand.Read(key); err != nil || n != len(key) {
+ t.Fatalf("failed to read random key: n=%d err=%v", n, err)
+ }📝 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.
Suggested change
| key := make([]byte, 32) | |
| _, _ = rand.Read(key) | |
| handler := &OAuth2Handler{ | |
| config: &OAuth2Config{ | |
| stateSigningKey: key, | |
| RedirectURIs: "https://mcp-server.com/oauth/callback", | |
| }, | |
| } | |
| key := make([]byte, 32) | |
| if n, err := rand.Read(key); err != nil || n != len(key) { | |
| t.Fatalf("failed to read random key: n=%d err=%v", n, err) | |
| } | |
| handler := &OAuth2Handler{ | |
| config: &OAuth2Config{ | |
| stateSigningKey: key, | |
| RedirectURIs: "https://mcp-server.com/oauth/callback", | |
| }, | |
| } |
🤖 Prompt for AI Agents
In security_scenarios_test.go around lines 12 to 20, the test ignores rand.Read
results which can produce partial fills or errors and lead to weak keys; update
the test to capture (n, err) := rand.Read(key), verify err == nil and n ==
len(key), and fail the test immediately (e.g., t.Fatalf or equivalent) if either
condition is not met so the test never proceeds with a partially filled or
errored key.
Comment on lines
+35
to
+44
| decoded, _ := base64.URLEncoding.DecodeString(signedState) | ||
| var tamperedData map[string]string | ||
| _ = json.Unmarshal(decoded, &tamperedData) | ||
|
|
||
| // Change redirect to evil site | ||
| tamperedData["redirect"] = "https://evil.com/steal-codes" | ||
|
|
||
| // Re-encode (but signature is now invalid) | ||
| tamperedJSON, _ := json.Marshal(tamperedData) | ||
| tamperedState := base64.URLEncoding.EncodeToString(tamperedJSON) |
There was a problem hiding this comment.
🧩 Analysis chain
State tampering test can panic; handle decode/unmarshal errors
Assumes signState returns base64-encoded JSON. If format differs, json.Unmarshal fails and tamperedData writes will panic. Check errors (or adapt to actual format).
- decoded, _ := base64.URLEncoding.DecodeString(signedState)
- var tamperedData map[string]string
- _ = json.Unmarshal(decoded, &tamperedData)
+ decoded, err := base64.URLEncoding.DecodeString(signedState)
+ if err != nil {
+ t.Fatalf("unexpected signedState format (base64 decode failed): %v", err)
+ }
+ var tamperedData map[string]string
+ if err := json.Unmarshal(decoded, &tamperedData); err != nil {
+ t.Fatalf("unexpected state payload (json unmarshal failed): %v", err)
+ }If signState is JWS/compact or another envelope, please confirm its format so we can tamper the correct portion (e.g., payload segment). Based on learnings.
I need to examine the codebase to verify the concerns about error handling and state format. Let me gather the necessary information.
---
🏁 Script executed:
```shell
#!/bin/bash
# Find the security_scenarios_test.go file and inspect the context
fd -e go -i '*security*scenarios*test' --exec cat {} \;
Length of output: 303
🏁 Script executed:
#!/bin/bash
# Search for how signedState is generated/obtained in test setup
rg -n -B 5 -A 2 'signedState\s*:=' --type goLength of output: 467
🏁 Script executed:
#!/bin/bash
# Search for state signing implementation to understand the format
rg -n -B 3 -A 10 'func.*[Ss]ign.*[Ss]tate|func.*[Ee]ncodeState|func.*[Cc]reateState' --type goLength of output: 642
🏁 Script executed:
#!/bin/bash
# Look for any comments or documentation about state format
rg -n -C 3 'base64|JWS|compact|payload' --type go | head -50Length of output: 2217
I found important context. Let me examine the signing implementation more closely and see the complete test file to understand the actual state format.
---
🏁 Script executed:
```shell
#!/bin/bash
# Get the complete signState function implementation
rg -n -A 30 'func.*\(h \*OAuth2Handler\) signState' --type go
Length of output: 1515
🏁 Script executed:
#!/bin/bash
# Get the complete security_scenarios_test.go file
cat security_scenarios_test.goLength of output: 3791
🏁 Script executed:
#!/bin/bash
# Check verifyState to understand expected format
rg -n -B 3 -A 30 'func.*\(h \*OAuth2Handler\) verifyState' --type goLength of output: 1571
Add error handling to test's decode/unmarshal operations for better failure diagnostics
The signedState format is confirmed as base64-encoded JSON (from signState() implementation in handlers.go:715). The tampering logic is correct, but ignoring decode and unmarshal errors means test failures become silent instead of providing clear diagnostics if the test setup changes.
- decoded, _ := base64.URLEncoding.DecodeString(signedState)
- var tamperedData map[string]string
- _ = json.Unmarshal(decoded, &tamperedData)
+ decoded, err := base64.URLEncoding.DecodeString(signedState)
+ if err != nil {
+ t.Fatalf("setup error: could not decode signed state: %v", err)
+ }
+ var tamperedData map[string]string
+ if err := json.Unmarshal(decoded, &tamperedData); err != nil {
+ t.Fatalf("setup error: could not unmarshal state payload: %v", err)
+ }📝 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.
Suggested change
| decoded, _ := base64.URLEncoding.DecodeString(signedState) | |
| var tamperedData map[string]string | |
| _ = json.Unmarshal(decoded, &tamperedData) | |
| // Change redirect to evil site | |
| tamperedData["redirect"] = "https://evil.com/steal-codes" | |
| // Re-encode (but signature is now invalid) | |
| tamperedJSON, _ := json.Marshal(tamperedData) | |
| tamperedState := base64.URLEncoding.EncodeToString(tamperedJSON) | |
| decoded, err := base64.URLEncoding.DecodeString(signedState) | |
| if err != nil { | |
| t.Fatalf("setup error: could not decode signed state: %v", err) | |
| } | |
| var tamperedData map[string]string | |
| if err := json.Unmarshal(decoded, &tamperedData); err != nil { | |
| t.Fatalf("setup error: could not unmarshal state payload: %v", err) | |
| } | |
| // Change redirect to evil site | |
| tamperedData["redirect"] = "https://evil.com/steal-codes" | |
| // Re-encode (but signature is now invalid) | |
| tamperedJSON, _ := json.Marshal(tamperedData) | |
| tamperedState := base64.URLEncoding.EncodeToString(tamperedJSON) |
🤖 Prompt for AI Agents
In security_scenarios_test.go around lines 35 to 44, the base64.DecodeString and
json.Unmarshal/json.Marshal calls ignore errors which silences setup failures;
update the test to check and handle each error (decoded, unmarshal, and marshal)
and fail the test immediately with a clear message (e.g., t.Fatalf or
require.NoError) including the error value so failures surface useful
diagnostics.
Comment on lines
+64
to
+66
| unsignedJSON, _ := json.Marshal(unsignedData) | ||
| unsignedState := base64.URLEncoding.EncodeToString(unsignedJSON) | ||
|
|
There was a problem hiding this comment.
Unsigned state test: check marshal error
Avoid ignoring json.Marshal error.
- unsignedJSON, _ := json.Marshal(unsignedData)
+ unsignedJSON, err := json.Marshal(unsignedData)
+ if err != nil {
+ t.Fatalf("failed to marshal unsigned state: %v", err)
+ }📝 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.
Suggested change
| unsignedJSON, _ := json.Marshal(unsignedData) | |
| unsignedState := base64.URLEncoding.EncodeToString(unsignedJSON) | |
| unsignedJSON, err := json.Marshal(unsignedData) | |
| if err != nil { | |
| t.Fatalf("failed to marshal unsigned state: %v", err) | |
| } | |
| unsignedState := base64.URLEncoding.EncodeToString(unsignedJSON) |
🤖 Prompt for AI Agents
In security_scenarios_test.go around lines 64 to 66, the json.Marshal error for
unsignedData is being ignored; update this to capture the error, check it, and
fail the test on error (e.g., if err != nil {
t.Fatalf("json.Marshal(unsignedData) failed: %v", err) }) before using
unsignedJSON and then continue to base64 encode the result.
Comment on lines
+103
to
+107
| // Verify the original signed state works correctly | ||
| _, err = handler.verifyState(signed) | ||
| if err != nil { | ||
| t.Logf("Good: Original state verification works: %v", err) | ||
| } |
There was a problem hiding this comment.
Fix verification logic.
The test logs "Good" when there's an error, but this seems incorrect. The test should verify that the original state verification succeeds, not that it fails.
Apply this diff:
// Verify the original signed state works correctly
_, err = handler.verifyState(signed)
if err != nil {
- t.Logf("Good: Original state verification works: %v", err)
+ t.Fatalf("Original state verification should succeed: %v", err)
}
+ t.Logf("Good: Original state verification succeeded")📝 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.
Suggested change
| // Verify the original signed state works correctly | |
| _, err = handler.verifyState(signed) | |
| if err != nil { | |
| t.Logf("Good: Original state verification works: %v", err) | |
| } | |
| // Verify the original signed state works correctly | |
| _, err = handler.verifyState(signed) | |
| if err != nil { | |
| t.Fatalf("Original state verification should succeed: %v", err) | |
| } | |
| t.Logf("Good: Original state verification succeeded") |
🤖 Prompt for AI Agents
In state_test.go around lines 103 to 107, the test currently logs "Good" when
verifyState returns an error; change the logic to fail the test when
verification fails. Replace the current if block so that if err != nil you call
t.Fatalf with a message like "Original state verification failed: %v" (including
err); if you still want a success message, use t.Logf when err == nil, otherwise
don't log success.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Release Notes
New Features
Documentation
Infrastructure