Security hardening: nonce cleanup, rolling deploy compatibility - #28
Merged
Conversation
CRITICAL fixes: - Add nonce replay protection with signature-first validation - Add 1MB body size limit to token endpoint (DoS prevention) - Fix callback URL query injection with proper URL encoding HIGH fixes: - Token cache now respects JWT expiry (min(token.expiry, now+5min)) - Enforce issuer URL validation for OIDC providers (MITM prevention) - Eliminate insecure fallback state-signing key (now panics) - Add 256KB body size limit to registration endpoint - Add timestamp and nonce to OAuth state for replay protection - Include timestamp/nonce in state signature verification - Add generateSecureNonce() for cryptographically secure nonces New features: - Add nonce cache with background cleanup - Add security headers (CSP, X-Frame-Options, X-Content-Type-Options) - Add ValidateIssuerURL() for issuer URL validation - Add rate limiter (standalone, ready for integration) Tests: - Add cache_expiry_test.go for token cache expiry verification - Add validation_test.go for input validation tests - Add error_leakage_test.go for error message sanitization - Add ratelimit_test.go for rate limiter coverage All tests pass. No CRITICAL vulnerabilities remain. Signed-off-by: Tommy Nguyen <tuannvm@hotmail.com>
- Fix errcheck issues for validator.Initialize - Fix staticcheck issue with nil context (use context.TODO()) - Fix errcheck issues for defer r.Body.Close() All lint checks pass now. Signed-off-by: Tommy Nguyen <tuannvm@hotmail.com>
This fix completes the replay protection implementation by making timestamp and nonce REQUIRED fields in verifyState(). Previously, these fields were treated as optional, which meant: - Old signed states from before the upgrade would still validate - Attackers could bypass age checks with missing/non-numeric timestamps Changes: - verifyState() now requires timestamp and nonce fields - Returns clear error message when fields are missing - Validates timestamp format before parsing - Tests updated to include timestamp/nonce when signing states This is a BREAKING CHANGE for security: states signed without timestamp/nonce will no longer validate after this upgrade. Signed-off-by: Tommy Nguyen <tuannvm@hotmail.com>
This fix addresses a P1 issue where validateOAuthParams() was defined but never called from HandleAuthorize or HandleToken. Changes: - Add validateOAuthParams() call to HandleAuthorize after method check - Add validateOAuthParams() call to HandleToken after ParseForm This enforces field length limits on attacker-controlled OAuth parameters (code, state, code_challenge) to prevent DoS attacks via oversized values. Signed-off-by: Tommy Nguyen <tuannvm@hotmail.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds validation helpers, nonce-based state replay protection, JWT-expiry-aware token caching, an in-memory fixed-window rate limiter, request-size limits, centralized security headers, multiple security/validation tests, dependency bumps, and handler/middleware hardening across OAuth flows. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Handler as OAuth Handler
participant RateLimiter
participant Validator as Param Validator
participant StateStore as State & Nonce Store
participant Cache as Token Cache
participant Provider as Token Validator
Client->>Handler: /authorize or /token request
Handler->>RateLimiter: Allow(key)?
alt denied
RateLimiter-->>Handler: deny
Handler-->>Client: HTTP 429
else allowed
RateLimiter-->>Handler: allow
Handler->>Validator: ValidateOAuthParams(request)
alt invalid params
Validator-->>Handler: error
Handler-->>Client: HTTP 400 (sanitized)
else valid
Validator-->>Handler: ok
Handler->>StateStore: VerifyState(signedState)
alt state invalid or replay
StateStore-->>Handler: error (generic)
Handler-->>Client: HTTP 400
else state ok
StateStore-->>Handler: ok
Handler->>Cache: GetCachedToken(key)
alt cache hit
Cache-->>Handler: token
else cache miss
Handler->>Provider: ValidateToken(token)
Provider-->>Handler: User{Expiry: ...}
Handler->>Cache: SetCachedToken(key, token, expiresAt)
end
Handler-->>Client: HTTP 200 (with security headers)
end
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
- Clean up expired nonces before replay check (prevent memory leak) - Make timestamp/nonce optional for backward compatibility with older versions - Allows states from previous versions during rolling deployments Signed-off-by: Tommy Nguyen <tuannvm@hotmail.com>
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Version 0.33.1 no longer exists. Using master branch for latest stable version. Signed-off-by: Tommy Nguyen <tuannvm@hotmail.com>
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
middleware.go (1)
146-152:⚠️ Potential issue | 🟡 MinorPotential token exposure in log output.
Logging the first 30 characters of the Authorization header can expose up to 23 characters of the actual Bearer token (after the 7-character "Bearer " prefix), which may violate the guideline to never log raw OAuth tokens.
Consider logging only the token hash instead:
Proposed fix
} else if authHeader != "" { - preview := authHeader - if len(authHeader) > 30 { - preview = authHeader[:30] + "..." - } - log.Printf("OAuth: Invalid Authorization header format: %s", preview) + // Log only that the format was invalid, not the content (may contain partial tokens) + log.Printf("OAuth: Invalid Authorization header format (length: %d, expected 'Bearer <token>')", len(authHeader)) }Based on learnings: "Never log raw OAuth tokens; only log SHA-256 hash of token formatted as fmt.Sprintf("%x", sha256.Sum256([]byte(token)))[:16]"
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@middleware.go` around lines 146 - 152, The current logging prints a raw preview of authHeader (variable authHeader) which may expose parts of the Bearer token; instead, detect and strip the "Bearer " prefix to extract the token, compute its SHA-256 digest, format it as hex (fmt.Sprintf("%x", sha256.Sum256([]byte(token)))) and log only the first 16 hex characters (no raw token) in the log.Printf call where authHeader is currently used; update the branch that handles non-empty authHeader (the block that builds preview and calls log.Printf("OAuth: Invalid Authorization header format: %s", preview)) to use the hashed preview and import crypto/sha256 and fmt as needed.
🧹 Nitpick comments (6)
cache_expiry_test.go (2)
8-44: Consider using table-driven subtests for test organization.Per coding guidelines, tests should use the table-driven subtests pattern with
t.Run(). This would consolidate related expiry scenarios and improve maintainability.Example refactor for expiry tests
func TestTokenCacheExpiry(t *testing.T) { tests := []struct { name string expiryDelta time.Duration waitDelta time.Duration wantExists bool }{ {"immediate_check", 50 * time.Millisecond, 0, true}, {"after_expiry", 50 * time.Millisecond, 100 * time.Millisecond, false}, {"long_expiry", 1 * time.Hour, 10 * time.Millisecond, true}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { cache := &TokenCache{cache: make(map[string]*CachedToken)} user := &User{Username: "testuser", Email: "test@example.com", Subject: "123"} tokenHash := "test-token-hash" cache.setCachedToken(tokenHash, user, time.Now().Add(tt.expiryDelta)) if tt.waitDelta > 0 { time.Sleep(tt.waitDelta) } _, exists := cache.getCachedToken(tokenHash) if exists != tt.wantExists { t.Errorf("exists = %v, want %v", exists, tt.wantExists) } }) } }As per coding guidelines: "Use table-driven subtests pattern with t.Run() for test organization"
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cache_expiry_test.go` around lines 8 - 44, Refactor TestTokenCacheExpiry into a table-driven subtests pattern: create a slice of test cases (name, expiryDelta, waitDelta, wantExists), and for each case call t.Run(tt.name, func(t *testing.T){ ... }), inside each subtest instantiate a fresh TokenCache and User, call cache.setCachedToken(tokenHash, user, time.Now().Add(tt.expiryDelta)), optionally time.Sleep(tt.waitDelta), then call cache.getCachedToken(tokenHash) and assert that exists == tt.wantExists (and if exists also assert cached.User.Username == "testuser"); keep using the existing TokenCache, setCachedToken and getCachedToken symbols and preserve current semantics for expiry behavior.
46-75: Misleading test name and fragile timing.
TestTokenCacheConcurrentExpirydoesn't test concurrent access—it tests multiple sequential entries expiring. Consider renaming toTestTokenCacheMultipleEntriesExpiry.The timing (10ms expiry, 20ms sleep) may be flaky on slow CI systems. Consider increasing the margins (e.g., 50ms expiry, 100ms sleep) to reduce test flakiness.
Line 59:
string(rune('0'+i))is an unusual pattern for generating token hashes. Consider usingfmt.Sprintf("test-token-hash-%d", i)for clarity.Suggested improvements
-func TestTokenCacheConcurrentExpiry(t *testing.T) { +func TestTokenCacheMultipleEntriesExpiry(t *testing.T) { cache := &TokenCache{ cache: make(map[string]*CachedToken), } user := &User{ Username: "testuser", Email: "test@example.com", Subject: "123", } // Add multiple tokens with short expiry for i := 0; i < 10; i++ { - tokenHash := "test-token-hash-" + string(rune('0'+i)) - expiresAt := time.Now().Add(10 * time.Millisecond) + tokenHash := fmt.Sprintf("test-token-hash-%d", i) + expiresAt := time.Now().Add(50 * time.Millisecond) cache.setCachedToken(tokenHash, user, expiresAt) } // Wait for expiry - time.Sleep(20 * time.Millisecond) + time.Sleep(100 * time.Millisecond) // All should be expired for i := 0; i < 10; i++ { - tokenHash := "test-token-hash-" + string(rune('0'+i)) + tokenHash := fmt.Sprintf("test-token-hash-%d", i) _, exists := cache.getCachedToken(tokenHash) if exists { t.Errorf("Token %s should be expired", tokenHash) } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cache_expiry_test.go` around lines 46 - 75, Rename the test function TestTokenCacheConcurrentExpiry to TestTokenCacheMultipleEntriesExpiry, update the token generation in the loop to use fmt.Sprintf("test-token-hash-%d", i) instead of string(rune('0'+i)), and increase the timing margins by setting expiresAt to time.Now().Add(50 * time.Millisecond) and sleeping for 100 * time.Millisecond before assertions; ensure you still use the TokenCache.setCachedToken and TokenCache.getCachedToken methods in the same places.ratelimit.go (1)
8-10: Comment says sliding-window, implementation is fixed-window.The docs are misleading versus actual behavior (
windowStartreset model). Rename the comment to fixed-window to avoid incorrect expectations.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ratelimit.go` around lines 8 - 10, The struct comment for RateLimiter incorrectly says "sliding window" while the implementation uses a fixed-window via windowStart resets; update the doc comment above type RateLimiter to describe it as a fixed-window/fixed-interval token counter (mentioning windowStart as the fixed window start marker) and adjust wording to reflect that behavior (e.g., "fixed-window rate limiting using windowStart to track current window") so docs match the RateLimiter implementation.ratelimit_test.go (1)
9-221: Refactor tests to table-driven subtests witht.Run().Current tests are split into many standalone functions; please convert to table-driven cases with subtests for consistency and maintainability.
As per coding guidelines, "
**/*_test.go: Use table-driven subtests pattern with t.Run() for test organization".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ratelimit_test.go` around lines 9 - 221, The tests are many standalone functions (e.g., TestRateLimiterBasic, TestRateLimiterWindowReset, TestRateLimiterReset, TestRateLimiterConcurrent, TestRateLimiterCleanup, TestRateLimiterCleanupBackground, TestRateLimiterGetters, TestRateLimiterSlidingWindow); refactor them into a single table-driven set of subtests using t.Run() so each scenario is a case with a name, input (window, maxReqs, sequence of Allow/GetCount/Reset/StartCleanup/cleanupExpiredEntries calls and expected outcomes) and a runner that invokes NewRateLimiter, Allow, GetCount, Reset, StartCleanup, cleanupExpiredEntries, GetWindow and GetMaxReqs as needed; ensure concurrent case uses a subtest with its goroutines and WaitGroup and that cleanup background uses defer to stop the cleanup goroutine inside that subtest.validation_test.go (1)
313-325: Consider usingstrings.Containsfrom standard library.The custom
containsStringandcontainsSubstringhelpers reimplementsstrings.Contains. Using the standard library is more idiomatic and readable.Suggested simplification
-// Helper function to check if string contains substring -func containsString(s, substr string) bool { - return len(s) >= len(substr) && (s == substr || len(s) > len(substr) && containsSubstring(s, substr)) -} - -func containsSubstring(s, substr string) bool { - for i := 0; i <= len(s)-len(substr); i++ { - if s[i:i+len(substr)] == substr { - return true - } - } - return false -} +import "strings" + +// Then use strings.Contains(err.Error(), tt.errMsg) directly in tests🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@validation_test.go` around lines 313 - 325, Replace the custom helpers containsString and containsSubstring with the standard library function strings.Contains: remove both functions (containsString, containsSubstring), add an import for "strings" if missing, and update any calls to containsString(s, substr) to use strings.Contains(s, substr) so behavior remains identical and code is more idiomatic.handlers.go (1)
868-886: Nonce cleanup happens on every verification, which could be optimized.The current implementation cleans expired nonces on every
verifyStatecall while holding the write lock. Under high load, this could create contention. Consider moving cleanup to a background goroutine.That said, for typical OAuth callback rates, this is unlikely to be a bottleneck. The PR objective of fixing the memory leak is achieved.
Alternative: Background cleanup goroutine (optional)
For higher throughput scenarios, cleanup could run periodically in a background goroutine started by
NewOAuth2Handler:go func() { ticker := time.NewTicker(time.Minute) defer ticker.Stop() for range ticker.C { h.seenNonceMu.Lock() now := time.Now() for n, expiry := range h.seenNonces { if expiry.Before(now) { delete(h.seenNonces, n) } } h.seenNonceMu.Unlock() } }()This would require lifecycle management (e.g., context cancellation) to stop the goroutine cleanly.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@handlers.go` around lines 868 - 886, The nonce cleanup loop inside the verifyState block (using h.seenNonces and h.seenNonceMu) should be moved out of the hot path: remove the per-call expired-nonce sweep from the verifyState code so it only checks for existence and inserts the new nonce under the mutex, and instead start a background goroutine in NewOAuth2Handler that periodically (e.g., once per minute via time.Ticker) locks h.seenNonceMu, removes expired entries (expiry.Before(now)), and unlocks; ensure the goroutine uses a context or stop channel from NewOAuth2Handler so it can be cleanly cancelled on shutdown.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@error_leakage_test.go`:
- Around line 86-93: The OAuth2Handler in the test is missing initialization of
the seenNonces map which can cause a nil map write panic; update the test's
OAuth2Handler construction to set seenNonces to a new map (e.g.,
make(map[string]time.Time)) and initialize any required mutex or related fields
consistent with the pattern in handlers.go (refer to seenNonces and its
protecting sync.Mutex usage), and add imports for "sync" and "time" if not
already present so the map value type and mutex can be used.
In `@go.mod`:
- Around line 13-27: The go.mod currently pins github.com/buger/jsonparser at
v1.1.1 which is vulnerable; update the requirement for the module
(github.com/buger/jsonparser) to v1.1.2 and then refresh modules (e.g., run go
get github.com/buger/jsonparser@v1.1.2 and go mod tidy) so the fixed version is
recorded and vendored/checked into the repo.
In `@handlers.go`:
- Around line 989-996: The generateSecureNonce function currently falls back to
a predictable timestamp when crypto/rand.Read fails, weakening replay
protection; change it to propagate the error instead of returning a time-based
nonce by updating generateSecureNonce to return (string, error) and return the
rand.Read error when it occurs (or, if you prefer consistent behavior with init
code, panic on error inside generateSecureNonce) and then update all call sites
that use generateSecureNonce to handle the error (either by propagating,
returning an error, or recovering/panicking where appropriate) so no predictable
nonce is ever emitted.
In `@ratelimit_test.go`:
- Around line 48-53: The tests in ratelimit_test.go use brittle time.Sleep calls
right up against window/ticker boundaries (e.g., the Sleep(60 *
time.Millisecond) followed by rl.Allow("key1")), which can flake in CI; replace
those fixed short sleeps with a stable polling-with-timeout helper that
repeatedly calls rl.Allow (or checks the intended condition) until success or a
generous timeout, or increase the sleep buffers significantly; update all
similar occurrences that use time.Sleep near window expiry or ticker cadence
(including the blocks around rl.Allow at the ranges noted) to use that polling
helper or larger buffers so tests become scheduler-tolerant.
In `@ratelimit.go`:
- Around line 26-31: NewRateLimiter currently accepts non-positive window or
maxReqs which leads to incorrect/unbounded behavior; update NewRateLimiter to
validate inputs (window > 0 and maxReqs > 0), change its signature to return
(*RateLimiter, error), and return a descriptive error if validation fails;
construct and return the RateLimiter only when inputs are valid and update
callers to handle the error (if you prefer to keep the old signature, instead
perform a fail-fast panic with a clear message). Ensure references to
NewRateLimiter and the RateLimiter type are updated accordingly so the runtime
branches that rely on valid window/maxReqs no longer encounter zero/negative
values.
- Around line 95-111: The StartCleanup code must validate the interval and make
stopping safe to call multiple times: check that the passed interval is > 0 and
if not return a no-op stop function without starting the goroutine; when
creating the stop mechanism use a sync.Once (e.g., local once := &sync.Once{})
and return a stop function that calls once.Do(func(){ close(stopCh) }) so
close(stopCh) is executed exactly once; create the ticker only after the
interval check and keep the goroutine calling rl.cleanupExpiredEntries()
unchanged, referencing the existing RateLimiter.StartCleanup,
cleanupExpiredEntries, stopCh and ticker symbols.
---
Outside diff comments:
In `@middleware.go`:
- Around line 146-152: The current logging prints a raw preview of authHeader
(variable authHeader) which may expose parts of the Bearer token; instead,
detect and strip the "Bearer " prefix to extract the token, compute its SHA-256
digest, format it as hex (fmt.Sprintf("%x", sha256.Sum256([]byte(token)))) and
log only the first 16 hex characters (no raw token) in the log.Printf call where
authHeader is currently used; update the branch that handles non-empty
authHeader (the block that builds preview and calls log.Printf("OAuth: Invalid
Authorization header format: %s", preview)) to use the hashed preview and import
crypto/sha256 and fmt as needed.
---
Nitpick comments:
In `@cache_expiry_test.go`:
- Around line 8-44: Refactor TestTokenCacheExpiry into a table-driven subtests
pattern: create a slice of test cases (name, expiryDelta, waitDelta,
wantExists), and for each case call t.Run(tt.name, func(t *testing.T){ ... }),
inside each subtest instantiate a fresh TokenCache and User, call
cache.setCachedToken(tokenHash, user, time.Now().Add(tt.expiryDelta)),
optionally time.Sleep(tt.waitDelta), then call cache.getCachedToken(tokenHash)
and assert that exists == tt.wantExists (and if exists also assert
cached.User.Username == "testuser"); keep using the existing TokenCache,
setCachedToken and getCachedToken symbols and preserve current semantics for
expiry behavior.
- Around line 46-75: Rename the test function TestTokenCacheConcurrentExpiry to
TestTokenCacheMultipleEntriesExpiry, update the token generation in the loop to
use fmt.Sprintf("test-token-hash-%d", i) instead of string(rune('0'+i)), and
increase the timing margins by setting expiresAt to time.Now().Add(50 *
time.Millisecond) and sleeping for 100 * time.Millisecond before assertions;
ensure you still use the TokenCache.setCachedToken and TokenCache.getCachedToken
methods in the same places.
In `@handlers.go`:
- Around line 868-886: The nonce cleanup loop inside the verifyState block
(using h.seenNonces and h.seenNonceMu) should be moved out of the hot path:
remove the per-call expired-nonce sweep from the verifyState code so it only
checks for existence and inserts the new nonce under the mutex, and instead
start a background goroutine in NewOAuth2Handler that periodically (e.g., once
per minute via time.Ticker) locks h.seenNonceMu, removes expired entries
(expiry.Before(now)), and unlocks; ensure the goroutine uses a context or stop
channel from NewOAuth2Handler so it can be cleanly cancelled on shutdown.
In `@ratelimit_test.go`:
- Around line 9-221: The tests are many standalone functions (e.g.,
TestRateLimiterBasic, TestRateLimiterWindowReset, TestRateLimiterReset,
TestRateLimiterConcurrent, TestRateLimiterCleanup,
TestRateLimiterCleanupBackground, TestRateLimiterGetters,
TestRateLimiterSlidingWindow); refactor them into a single table-driven set of
subtests using t.Run() so each scenario is a case with a name, input (window,
maxReqs, sequence of Allow/GetCount/Reset/StartCleanup/cleanupExpiredEntries
calls and expected outcomes) and a runner that invokes NewRateLimiter, Allow,
GetCount, Reset, StartCleanup, cleanupExpiredEntries, GetWindow and GetMaxReqs
as needed; ensure concurrent case uses a subtest with its goroutines and
WaitGroup and that cleanup background uses defer to stop the cleanup goroutine
inside that subtest.
In `@ratelimit.go`:
- Around line 8-10: The struct comment for RateLimiter incorrectly says "sliding
window" while the implementation uses a fixed-window via windowStart resets;
update the doc comment above type RateLimiter to describe it as a
fixed-window/fixed-interval token counter (mentioning windowStart as the fixed
window start marker) and adjust wording to reflect that behavior (e.g.,
"fixed-window rate limiting using windowStart to track current window") so docs
match the RateLimiter implementation.
In `@validation_test.go`:
- Around line 313-325: Replace the custom helpers containsString and
containsSubstring with the standard library function strings.Contains: remove
both functions (containsString, containsSubstring), add an import for "strings"
if missing, and update any calls to containsString(s, substr) to use
strings.Contains(s, substr) so behavior remains identical and code is more
idiomatic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ae948a69-4dbb-49c2-928f-bb3806af3aef
⛔ Files ignored due to path filters (2)
.claude/scheduled_tasks.lockis excluded by!**/*.lockgo.sumis excluded by!**/*.sum
📒 Files selected for processing (15)
cache_expiry_test.goconfig.goerror_leakage_test.gogo.modhandlers.gometadata.gomiddleware.gooauth.goprovider/provider.goratelimit.goratelimit_test.gosecurity_test.gostate_test.govalidation.govalidation_test.go
- GO-2025-XXX: os.ReadDir vulnerability - GO-2026-4601: IPv6 parsing vulnerability in net/url Both are fixed in Go 1.25.8 Signed-off-by: Tommy Nguyen <tuannvm@hotmail.com>
Security fixes: - generateSecureNonce: panic instead of timestamp fallback (weakens replay protection) - middleware: Log only auth header length, not partial tokens - go.mod: Upgrade github.com/buger/jsonparser v1.1.1 -> v1.1.2 (vulnerability fix) Code quality improvements: - error_leakage_test.go: Add missing seenNonces map initialization - ratelimit.go: Fix comment (fixed-window, not sliding-window) - ratelimit.go: Add validation to NewRateLimiter (panic on invalid input) - ratelimit.go: Make StartCleanup safe to call multiple times (sync.Once) - ratelimit.go: Validate interval > 0 before starting cleanup goroutine Signed-off-by: Tommy Nguyen <tuannvm@hotmail.com>
The official go-sdk v1.4.1 requires auth.TokenInfo in the request context for proper session binding and user verification. Without this, the SDK's session hijacking protection does not work. Changes: - Use auth.RequireBearerToken middleware with TokenVerifier wrapper - Populate auth.TokenInfo with user.Subject for session tracking - Populate auth.TokenInfo.Expiration with user.Expiry - Maintain backward compatibility with oauth-mcp-proxy context This fixes the P1 compatibility issue identified by Codex review. Signed-off-by: Tommy Nguyen <tuannvm@hotmail.com>
Fixes P1 issues from Codex review: 1. Return auth.ErrInvalidToken directly for 401 responses - Previously wrapped error caused 500 response instead of 401 - Clients need proper 401 to trigger re-auth flows 2. Pass OPTIONS requests through before auth check - CORS preflight requests don't have Authorization header - Browser-based MCP clients need CORS support 3. Handle zero-expiry tokens with default expiration - HMAC tokens may not have exp claim - Set 1-hour default for tokens without expiry This ensures the official SDK adapter works correctly for: - Proper 401 error responses - CORS preflight requests - All validator types (HMAC, OIDC) Signed-off-by: Tommy Nguyen <tuannvm@hotmail.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
mcp/oauth.go (2)
86-89: Context propagation pattern works but is subtle.The pattern
*req = *req.WithContext(ctx)modifies the request in place so context values are available downstream. This works becausereqis a pointer passed to the verifier. While functional, this is a non-obvious side effect within a verifier function.Consider adding a brief comment explaining why this is necessary (the verifier's return value doesn't propagate context, so in-place modification is required).
📝 Optional: Add explanatory comment
// Also store in oauth-mcp-proxy context for backward compatibility + // Note: We must modify the request in-place because the verifier's + // return value (TokenInfo) doesn't propagate context to the handler. ctx = oauth.WithOAuthToken(ctx, token) ctx = oauth.WithUser(ctx, user) *req = *req.WithContext(ctx)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcp/oauth.go` around lines 86 - 89, Add a short explanatory comment above the in-place context assignment where oauth.WithOAuthToken and oauth.WithUser are applied (the line doing *req = *req.WithContext(ctx)) stating that the verifier cannot return a modified request/context to the caller and therefore we must mutate the incoming *http.Request in-place so downstream handlers receive the updated context; reference oauth.WithOAuthToken, oauth.WithUser and the in-place assignment to make the intent clear.
67-72: Original error context is lost when returningauth.ErrInvalidToken.When token validation fails, the actual error from
ValidateTokenCachedis discarded and replaced withauth.ErrInvalidToken. While this prevents error leakage (which is good for security), consider logging the original error for debugging purposes before returning.🔍 Optional: Log the original error for debugging
verifier := auth.TokenVerifier(func(ctx context.Context, token string, req *http.Request) (*auth.TokenInfo, error) { user, err := oauthServer.ValidateTokenCached(ctx, token) if err != nil { + // Log for debugging, but return generic error to client + // Note: ValidateTokenCached already logs validation failures // Return auth.ErrInvalidToken directly for proper 401 response return nil, auth.ErrInvalidToken }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcp/oauth.go` around lines 67 - 72, When ValidateTokenCached fails inside the TokenVerifier (the anonymous func assigned to verifier), preserve the security behavior of returning auth.ErrInvalidToken but log the original error for debugging first; update the verifier function to call your logger (or processLogger) with a clear message and the err from oauthServer.ValidateTokenCached before returning nil, auth.ErrInvalidToken so the original context is available for diagnostics while the response still returns auth.ErrInvalidToken.handlers.go (1)
826-841: Variable shadowing is intentional but could be clearer.Line 852 introduces a new
timestampvariable (string) that shadows theint64 timestampfrom line 826. While the scoping is correct (the string version is only used for signature verification), this pattern can be confusing during code review.Consider renaming for clarity:
♻️ Optional: Rename for clarity
- if timestamp, ok := stateData["timestamp"]; ok { - dataToSign += "×tamp=" + timestamp + if ts, ok := stateData["timestamp"]; ok { + dataToSign += "×tamp=" + ts }Also applies to: 851-857
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@handlers.go` around lines 826 - 841, The block declares an int64 named `timestamp` and later shadows it with a string `timestamp` during signature verification; rename the inner string variable (e.g., `timestampStr` or `sigTimestamp`) and update all local uses (including the call to `strconv.ParseInt` and any signature verification code that reads the string) so the outer `timestamp int64`, `hasTimestamp` check, `age` calculation, and error handling remain unshadowed and clear.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/build.yml:
- Line 95: Replace the mutable ref in the workflow where the Trivy action is
used (the line containing "uses: aquasecurity/trivy-action@master") with an
immutable reference: update the "uses" entry to point to a specific commit SHA
(or an audited release tag) instead of "@master", commit that change, and ensure
the chosen SHA corresponds to the expected version of aquasecurity/trivy-action
to avoid pulling unreviewed upstream changes.
In `@mcp/oauth.go`:
- Around line 74-79: The code in mcp/oauth.go sets TokenInfo.Expiration to
time.Now().Add(time.Hour) when user.Expiry is zero, which mismatches the 5 *
time.Minute cache TTL fallback used in middleware; either align these values or
make the expiration configurable/shared. Update the logic around
user.Expiry/TokenInfo.Expiration to use the same TTL as the middleware fallback
(e.g., 5 * time.Minute) or pull from a shared constant/config (create or
reference a shared DEFAULT_TOKEN_EXPIRY) so session expiration and cache TTL are
consistent; change the default substitution in the TokenInfo construction
accordingly and/or expose a config option so both mcp/oauth.go and the
middleware use the same source of truth.
- Around line 95-97: Replace the use of the authorization server metadata URL
with the protected resource metadata URL when constructing the auth middleware:
in the authMiddleware creation (auth.RequireBearerToken and its
RequireBearerTokenOptions) call oauthServer.GetProtectedResourceMetadataURL()
instead of oauthServer.GetAuthorizationServerMetadataURL() so the middleware
points to the protected resource metadata endpoint consistent with Return401()
and Return401InvalidToken().
---
Nitpick comments:
In `@handlers.go`:
- Around line 826-841: The block declares an int64 named `timestamp` and later
shadows it with a string `timestamp` during signature verification; rename the
inner string variable (e.g., `timestampStr` or `sigTimestamp`) and update all
local uses (including the call to `strconv.ParseInt` and any signature
verification code that reads the string) so the outer `timestamp int64`,
`hasTimestamp` check, `age` calculation, and error handling remain unshadowed
and clear.
In `@mcp/oauth.go`:
- Around line 86-89: Add a short explanatory comment above the in-place context
assignment where oauth.WithOAuthToken and oauth.WithUser are applied (the line
doing *req = *req.WithContext(ctx)) stating that the verifier cannot return a
modified request/context to the caller and therefore we must mutate the incoming
*http.Request in-place so downstream handlers receive the updated context;
reference oauth.WithOAuthToken, oauth.WithUser and the in-place assignment to
make the intent clear.
- Around line 67-72: When ValidateTokenCached fails inside the TokenVerifier
(the anonymous func assigned to verifier), preserve the security behavior of
returning auth.ErrInvalidToken but log the original error for debugging first;
update the verifier function to call your logger (or processLogger) with a clear
message and the err from oauthServer.ValidateTokenCached before returning nil,
auth.ErrInvalidToken so the original context is available for diagnostics while
the response still returns auth.ErrInvalidToken.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a39261e5-3fc2-4335-a6d7-c8c1ce9e4ce6
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (8)
.github/workflows/build.yml.github/workflows/test.ymlerror_leakage_test.gogo.modhandlers.gomcp/oauth.gomiddleware.goratelimit.go
✅ Files skipped from review due to trivial changes (2)
- .github/workflows/test.yml
- go.mod
🚧 Files skipped from review as they are similar to previous changes (2)
- error_leakage_test.go
- ratelimit.go
README.md: - Add "Security Features" section detailing all built-in protections - Update feature list to highlight security hardening - Document state replay protection, DoS prevention, rate limiting docs/SECURITY.md: - Update rate limiting section to document built-in RateLimiter - Add "Built-in Security Features" section with: - State replay protection (timestamp + nonce, cleanup) - Token cache security (JWT expiry-aware) - Input validation (parameter limits, body size limits) - Issuer URL validation (HTTPS enforcement) - Constant-time HMAC comparison - Secure nonce generation (crypto/rand) - Session management (auth.TokenInfo population) - Update security headers to match actual implementation (CSP added) - Update checklist to mark built-in features as complete docs/CONFIGURATION.md: - Update Config struct documentation with missing fields - Add FixedRedirectURI and AllowedClientRedirectDomains - Add Scopes and SkipAudienceCheck options - Add issuer URL validation notes - Clarify redirect URI configuration options (RedirectURIs vs FixedRedirectURI) - Document mode detection behavior Signed-off-by: Tommy Nguyen <tuannvm@hotmail.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/CONFIGURATION.md`:
- Line 33: The docs incorrectly state Scopes as type "string"; update the
documentation to reflect the real struct field type Config.Scopes as []string (a
slice of strings) and note that OAuth scopes should be provided as a string
slice rather than a single space-separated string so examples match the actual
Config struct and compile correctly.
In `@docs/SECURITY.md`:
- Line 482: The "No raw IP addresses" security bullet overstates actual
behavior; update the SECURITY.md bullet to accurately reflect current issuer
validation (which only checks URL format/host presence and HTTPS except for
localhost) by either removing that bullet or softening it to something like
"Prefer hostnames; raw IPs may be accepted by current validation."
Alternatively, implement explicit raw-IP rejection in the issuer validation path
(the code that enforces URL format/host presence and HTTPS) and then keep the
original bullet. Ensure the docs and the issuer validation logic stay
consistent.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 636494dc-2e96-43a2-9747-bcd92bf809fb
📒 Files selected for processing (3)
README.mddocs/CONFIGURATION.mddocs/SECURITY.md
Signed-off-by: Tommy Nguyen <tuannvm@hotmail.com>
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
- Fix concurrent mutation of oauth2Config.RedirectURL - Create copy of oauth2Config before mutation in HandleAuthorize - Create copy of oauth2Config before mutation in HandleToken - Prevents cross-request redirect mix-up in concurrent scenarios - Add redirect URI validation to Config.Validate() - Validate each URI in RedirectURIS comma-separated list - Validate FixedRedirectURI if set - Enforces HTTPS for non-localhost URIs at startup Signed-off-by: Tommy Nguyen <tuannvm@hotmail.com>
Latest version includes bug fixes and improvements. All tests pass with new version. Signed-off-by: Tommy Nguyen <tuannvm@hotmail.com>
- Fix mcp adapter to use GetProtectedResourceMetadataURL() instead of GetAuthorizationServerMetadataURL() for ResourceMetadataURL option (RFC 9728 compliance) - Update examples/README.md to reflect actual tool count in advanced examples Signed-off-by: Tommy Nguyen <tuannvm@hotmail.com>
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
docs/SECURITY.md (1)
26-30:⚠️ Potential issue | 🟡 MinorRaw-IP rejection is documented as enforced, but this needs confirmation.
Line 29 and Line 551 state raw IP issuer hosts are disallowed. Please confirm
ValidateIssuerURLactually enforces this; otherwise soften/remove this claim.Use this read-only check to confirm behavior:
#!/bin/bash set -euo pipefail echo "Locate validation implementation files" files=$(fd -i "validation.go") echo "$files" echo echo "Inspect ValidateIssuerURL implementation" rg -nP 'func\s+ValidateIssuerURL\s*\(' -C40 $files echo echo "Look for explicit IP-host rejection logic" rg -nP 'net\.ParseIP|netip|ParseAddr|Hostname\(|localhost|IP address|raw IP' -C5 $filesExpected result: an explicit branch that detects IP hosts and returns an error. If absent, docs should be adjusted.
Also applies to: 546-552
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/SECURITY.md` around lines 26 - 30, Validate the documentation claim that raw-IP issuer hosts are disallowed by locating and inspecting the ValidateIssuerURL implementation (search for function ValidateIssuerURL in validation.go or related validation files) and confirm there is an explicit branch that detects IP hosts (e.g., using net.ParseIP, netip, or hostname parsing) and returns an error; if such IP-host rejection logic is missing, update SECURITY.md to soften/remove the assertion about raw-IP issuer hosts (or add an accurate note saying validation does not currently reject IP hosts) and/or implement the IP-host check inside ValidateIssuerURL to enforce the documented behavior.mcp/oauth.go (1)
74-79:⚠️ Potential issue | 🟠 MajorStill open: keep the fallback
TokenInfo.Expirationaligned withValidateTokenCached.When
user.Expiryis zero, this creates a 1-hour SDK expiration, butoauth.ValidateTokenCachedonly gives zero-expiry tokens the normal 5-minute cache window. That can leave the go-sdk auth layer treating a token as live well past the next revalidation point. Please share one fallback source between both paths, or leaveExpirationunset if the SDK supports “unknown expiry”.#!/bin/bash set -euo pipefail # Expect the fallback in mcp/oauth.go to match the proxy's own # zero-expiry handling in oauth.go (and middleware.go if present). echo "=== mcp/oauth.go fallback ===" sed -n '72,90p' mcp/oauth.go echo echo "=== oauth.ValidateTokenCached expiry handling ===" sed -n '103,140p' oauth.go echo if [[ -f middleware.go ]]; then echo "=== middleware.go fallback TTL ===" sed -n '60,85p' middleware.go fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcp/oauth.go` around lines 74 - 79, The TokenInfo expiration fallback in mcp/oauth.go currently sets expiration := user.Expiry then defaults to time.Now().Add(time.Hour) when zero, which diverges from oauth.ValidateTokenCached's 5-minute handling; change this to use a single shared fallback (e.g., a package-level constant like defaultZeroExpiryTTL used by ValidateTokenCached) or avoid setting TokenInfo.Expiration when user.Expiry.IsZero() if the SDK supports unknown expiry; update mcp/oauth.go to reference that shared constant or leave Expiration unset so both TokenInfo and ValidateTokenCached remain aligned (refer to user.Expiry, TokenInfo.Expiration, and ValidateTokenCached to locate the code to change).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@config.go`:
- Around line 98-121: The current validation can pass when c.RedirectURIs is a
comma-only string because you only check raw emptiness and then skip empty
entries; update the validation in the block handling c.RedirectURIs and
FixedRedirectURI so you parse strings.Split(c.RedirectURIs, ","), trim each uri,
validate non-empty entries with ValidateRedirectURI, and keep a counter of
successfully validated redirect URIs; if after processing the list there are
zero valid URIs and FixedRedirectURI is empty or invalid, return an error
("proxy mode requires at least one valid redirect URI"); ensure you still
validate FixedRedirectURI with ValidateRedirectURI and treat it as satisfying
the requirement if valid (use symbols c.RedirectURIs, c.FixedRedirectURI,
ValidateRedirectURI).
In `@docs/SECURITY.md`:
- Around line 62-64: Update the header and occurrences to use the hyphenated
compound adjective form: change the heading "Backwards Compatible Changes" to
"Backward-Compatible Changes" and replace any instances of "backwards
compatible" in the body with "backward-compatible" (and "Backwards" with
"Backward" where capitalized). Ensure consistency across the document, including
any nearby headings or list items that reference the same phrase.
In `@mcp/oauth.go`:
- Around line 41-44: The returned wrapped handler currently doesn't delegate to
the existing mux so OAuth discovery/callback/token routes registered on mux
never get served; update wrappedHandler (the function wrapping handler/mux) to
delegate to mux.ServeHTTP for requests not handled by the protected logic (e.g.,
OPTIONS, discovery/callback/token paths) or alternatively mount the protected
MCP handler onto mux before returning; locate the symbols mux, wrappedHandler
and the protected MCP handler in oauth.go and either call mux.ServeHTTP(w, r)
from wrappedHandler when appropriate or register the protected handler on mux
and return mux as the ready-to-serve handler.
---
Duplicate comments:
In `@docs/SECURITY.md`:
- Around line 26-30: Validate the documentation claim that raw-IP issuer hosts
are disallowed by locating and inspecting the ValidateIssuerURL implementation
(search for function ValidateIssuerURL in validation.go or related validation
files) and confirm there is an explicit branch that detects IP hosts (e.g.,
using net.ParseIP, netip, or hostname parsing) and returns an error; if such
IP-host rejection logic is missing, update SECURITY.md to soften/remove the
assertion about raw-IP issuer hosts (or add an accurate note saying validation
does not currently reject IP hosts) and/or implement the IP-host check inside
ValidateIssuerURL to enforce the documented behavior.
In `@mcp/oauth.go`:
- Around line 74-79: The TokenInfo expiration fallback in mcp/oauth.go currently
sets expiration := user.Expiry then defaults to time.Now().Add(time.Hour) when
zero, which diverges from oauth.ValidateTokenCached's 5-minute handling; change
this to use a single shared fallback (e.g., a package-level constant like
defaultZeroExpiryTTL used by ValidateTokenCached) or avoid setting
TokenInfo.Expiration when user.Expiry.IsZero() if the SDK supports unknown
expiry; update mcp/oauth.go to reference that shared constant or leave
Expiration unset so both TokenInfo and ValidateTokenCached remain aligned (refer
to user.Expiry, TokenInfo.Expiration, and ValidateTokenCached to locate the code
to change).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3076e415-8e6f-4e51-bb68-4425b5a76fa4
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (8)
README.mdconfig.godocs/CONFIGURATION.mddocs/SECURITY.mdexamples/README.mdgo.modhandlers.gomcp/oauth.go
✅ Files skipped from review due to trivial changes (2)
- examples/README.md
- go.mod
🚧 Files skipped from review as they are similar to previous changes (2)
- handlers.go
- docs/CONFIGURATION.md
- Pin trivy-action to v0.33.1 instead of mutable @master ref - Fix docs/CONFIGURATION.md Scopes type from string to []string Signed-off-by: Tommy Nguyen <tuannvm@hotmail.com>
- Replace context.Background() with request-scoped context + 30s timeout in token exchange and refresh token flows (DoS prevention) - Add length validation for redirect_uri, code_verifier, refresh_token, and client_id parameters in validateOAuthParams Signed-off-by: Tommy Nguyen <tuannvm@hotmail.com>
Remove unused rate limiter, verbose doc.go files, and uncalled validation functions. Fix duplicate hash computation in middleware and move nonce cleanup from per-request O(n) to background goroutine. Removed: - ratelimit.go/ratelimit_test.go (never wired into handlers) - doc.go files (root, mark3labs, mcp, provider) duplicating README - ValidateClientID, ValidateClientSecret (never called) - isPrivateIP, isLocalhostIP (over-defensive SSRF check) - .claude/ralph-loop.local.md, .claude/scheduled_tasks.lock Fixed: - Duplicate SHA-256 hash in middleware.go Middleware() - O(n) nonce cleanup per verifyState() call → background goroutine Signed-off-by: Tommy Nguyen <tuannvm@hotmail.com>
- Replace custom containsString/containsSubstring with strings.Contains - Rename TestTokenCacheConcurrentExpiry to TestTokenCacheMultipleEntriesExpiry - Use fmt.Sprintf for test token hash generation instead of string(rune()) - Increase test timing margins (50ms expiry, 100ms sleep) to reduce CI flakiness Signed-off-by: Tommy Nguyen <tuannvm@hotmail.com>
- Fix trivy-action version (0.33.1 not found → v0.35.0) - Validate proxy mode has at least one usable redirect URI after trimming - Fix 'Backwards Compatible' → 'Backward-Compatible' in SECURITY.md Signed-off-by: Tommy Nguyen <tuannvm@hotmail.com>
…t URI validation Signed-off-by: Tommy Nguyen <tuannvm@hotmail.com>
- Update Dockerfile base image from golang:1.24 to golang:1.25 - Document user.Expiry field in user info examples - Add auth.TokenInfoFromContext alternative for official SDK - Show token expiry in official/advanced whoami tool Signed-off-by: Tommy Nguyen <tuannvm@hotmail.com>
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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
This PR addresses P2 security issues identified in code review:
Changes
seenNoncesmap before checking for replaytimestampandnonceoptional for rolling deploy compatibilityTests
All tests pass (
make test)Linter clean (
make lint)Previously Completed
Summary by CodeRabbit
New Features
Security Improvements
Bug Fixes
Tests
Documentation