feat: derive token cache expiry from JWT claim#25
Open
drombosky wants to merge 1 commit into
Open
Conversation
|
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (9)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (8)
WalkthroughValidators now return a token expiry; caching uses each token's JWT Changes
Sequence DiagramsequenceDiagram
participant Client as Client
participant Middleware as Middleware
participant ValidatorCache as ValidateTokenCached
participant Validator as TokenValidator
participant Cache as Cache
Client->>Middleware: HTTP request with Bearer token
Middleware->>ValidatorCache: ValidateTokenCached(ctx, token)
alt Cache hit & not expired
ValidatorCache->>Cache: lookup(token)
Cache-->>ValidatorCache: cached user
ValidatorCache-->>Middleware: return user
else Cache miss or expired
ValidatorCache->>Validator: ValidateToken(ctx, token)
Validator-->>ValidatorCache: user, expiry, err
alt validation succeeded
ValidatorCache->>ValidatorCache: expiryAdj = expiry - TokenExpiryBuffer
alt time.Now() < expiryAdj
ValidatorCache->>Cache: set(token, user, ttl = expiry - now)
ValidatorCache-->>Middleware: return user
else token expired/expiring too soon
ValidatorCache-->>Middleware: return auth error
end
else validation failed
ValidatorCache-->>Middleware: return auth error
end
end
Middleware-->>Client: respond with user in context or auth error
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
Author
|
This is my first time dealing with Cursor Code Review. It looks like there is a CI error. If there's something wrong with my PR, please feel free to point me in the right direction and I can fix it :) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
provider/provider_test.go (1)
26-53: Cover the no-expfallback in this suite.This only locks in the explicit-
exppath. The interface change also relies on validators returning a non-zero fallback expiry whenexpis absent, andValidateTokenCachednow depends on that contract.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@provider/provider_test.go` around lines 26 - 53, Add a subtest to cover the no-`exp` fallback path: inside the existing t.Run("ValidAudience", ...) or as a sibling test, create a JWT with the same valid audience and no "exp" claim and call validator.ValidateToken (and optionally ValidateTokenCached) to assert it returns a non-nil user and a non-zero expiration that equals the expected fallback expiry used by the validator; reference validator.ValidateToken and validator.ValidateTokenCached and assert expiration.Unix() (or expiration.IsZero()) matches the fallback contract the validator implements.provider/provider.go (1)
130-135: PreferMapClaims.GetExpirationTime()over rawfloat64cast.This keeps cache-expiry extraction aligned with
jwt/v5claim parsing, includes proper type validation and error handling, and safely handles bothfloat64andjson.Numbervalues instead of silently falling back to the 5-minute default.♻️ Proposed change
- expiry := time.Now().Add(5 * time.Minute) - if expVal, ok := claims["exp"].(float64); ok { - expiry = time.Unix(int64(expVal), 0) - } + expiry := time.Now().Add(5 * time.Minute) + if expTime, err := claims.GetExpirationTime(); err == nil && expTime != nil { + expiry = expTime.Time + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@provider/provider.go` around lines 130 - 135, Replace the manual float64 cast extraction of "exp" with jwt/v5's MapClaims.GetExpirationTime() API: assert or convert your claims to jwt.MapClaims (the variable named claims) and call claims.GetExpirationTime(); if it returns a valid time use that for expiry, otherwise fall back to time.Now().Add(5*time.Minute) and log or handle the parsing error as needed; update the expiry assignment logic where expiry is currently set to ensure proper type validation and error handling instead of the raw cast.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@api_test.go`:
- Around line 713-783: The test never verifies the cached entry is stored with
the expected TTL/ExpiresAt because it only calls ValidateTokenCached once; after
filling the cache you must either call srv.cache.getCachedToken(tokenString) and
assert the cached item's ExpiresAt is near time.Now().Add(tt.expirationFromNow)
minus TokenExpiryBuffer (use Config.TokenExpiryBuffer) or invoke
ValidateTokenCached a second time after the original token's exp time window to
ensure the cache TTL prevented reuse (and assert error when within the buffer).
Update TestTokenExpiryBuffer to, for each success case, retrieve the cached
entry via srv.cache.getCachedToken or perform a second validation and assert the
stored ExpiresAt / resulting behavior matches the expected TokenExpiryBuffer
logic.
In `@config.go`:
- Around line 46-49: Reject negative TokenExpiryBuffer values by adding a
validation check where configuration is validated (e.g., the config validation
function that runs before using ValidateTokenCached). Specifically, ensure
TokenExpiryBuffer (the TokenExpiryBuffer field) must be >= 0 and return a clear
error if it's negative so ValidateTokenCached never receives a negative
duration; update any config parsing/initialization code to enforce this and
include the field name in the error message.
In `@docs/SECURITY.md`:
- Around line 203-204: Update the "Cache TTL" statement to explicitly document
two behaviors: when a JWT includes an exp claim use that expiry (capped by the
configurable expiry buffer), and when a JWT has no exp fall back to a 5-minute
cache TTL; also state that tokens whose expiry falls inside the configured
buffer are rejected (not cached). Reference the "Cache TTL" section and the JWT
`exp` claim wording and include mention of the configurable expiry buffer so
readers know where to change the buffer behavior.
In `@middleware.go`:
- Around line 39-42: The middleware is double-wrapping authentication errors
because ValidateTokenCached already returns errors prefixed with "authentication
failed"; update the handler around s.ValidateTokenCached(ctx, tokenString) to
log the error (using s.logger.Error with context req.Params.Name and err) but
return the original err instead of wrapping it (i.e., do not
fmt.Errorf("authentication failed: %w", err)); keep the existing logging call
and ensure the returned error is the one from ValidateTokenCached so callers
don't see duplicated prefixes.
In `@provider/provider.go`:
- Around line 85-86: The code uses strings.TrimPrefix(tokenString, "Bearer ")
which only matches canonical casing; update both places that call
strings.TrimPrefix(tokenString, "Bearer ") (the one in ValidateToken and the
other validator that also manipulates tokenString) to perform a case-insensitive
Bearer removal: detect the "bearer " prefix using a case-insensitive check
(e.g., strings.HasPrefix(strings.ToLower(tokenString), "bearer ")) and then
slice off the prefix so tokenString contains only the token; ensure you replace
both occurrences of strings.TrimPrefix(tokenString, "Bearer ") to keep behavior
consistent.
---
Nitpick comments:
In `@provider/provider_test.go`:
- Around line 26-53: Add a subtest to cover the no-`exp` fallback path: inside
the existing t.Run("ValidAudience", ...) or as a sibling test, create a JWT with
the same valid audience and no "exp" claim and call validator.ValidateToken (and
optionally ValidateTokenCached) to assert it returns a non-nil user and a
non-zero expiration that equals the expected fallback expiry used by the
validator; reference validator.ValidateToken and validator.ValidateTokenCached
and assert expiration.Unix() (or expiration.IsZero()) matches the fallback
contract the validator implements.
In `@provider/provider.go`:
- Around line 130-135: Replace the manual float64 cast extraction of "exp" with
jwt/v5's MapClaims.GetExpirationTime() API: assert or convert your claims to
jwt.MapClaims (the variable named claims) and call claims.GetExpirationTime();
if it returns a valid time use that for expiry, otherwise fall back to
time.Now().Add(5*time.Minute) and log or handle the parsing error as needed;
update the expiry assignment logic where expiry is currently set to ensure
proper type validation and error handling instead of the raw cast.
🪄 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: 00a0ca9e-8dcd-4b56-8e06-49658959cc56
📒 Files selected for processing (9)
api_test.goconfig.gocontext_propagation_test.godocs/SECURITY.mdintegration_test.gomiddleware.gooauth.goprovider/provider.goprovider/provider_test.go
Comment on lines
85
to
86
| // Remove Bearer prefix if present | ||
| tokenString = strings.TrimPrefix(tokenString, "Bearer ") |
There was a problem hiding this comment.
Handle the Bearer prefix case-insensitively in both validators.
Authorization schemes are case-insensitive. strings.TrimPrefix(..., "Bearer ") only strips the canonical casing, so direct callers that pass bearer <token> into ValidateToken will fail verification even though the header is valid. Apply the same normalization in both implementations.
🩹 Suggested change
- tokenString = strings.TrimPrefix(tokenString, "Bearer ")
+ if len(tokenString) >= 7 && strings.EqualFold(tokenString[:7], "Bearer ") {
+ tokenString = tokenString[7:]
+ }
+ tokenString = strings.TrimSpace(tokenString)Also applies to: 234-235
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@provider/provider.go` around lines 85 - 86, The code uses
strings.TrimPrefix(tokenString, "Bearer ") which only matches canonical casing;
update both places that call strings.TrimPrefix(tokenString, "Bearer ") (the one
in ValidateToken and the other validator that also manipulates tokenString) to
perform a case-insensitive Bearer removal: detect the "bearer " prefix using a
case-insensitive check (e.g., strings.HasPrefix(strings.ToLower(tokenString),
"bearer ")) and then slice off the prefix so tokenString contains only the
token; ensure you replace both occurrences of strings.TrimPrefix(tokenString,
"Bearer ") to keep behavior consistent.
Author
There was a problem hiding this comment.
Not fixing since this is pre-existing code.
There was a problem hiding this comment.
@drombosky, understood — keeping the scope of this PR focused makes sense. Would you like me to open a follow-up GitHub issue to track the case-insensitive Bearer prefix handling so it isn't lost?
🧠 Learnings used
Learnt from: CR
Repo: tuannvm/oauth-mcp-proxy PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-13T17:03:53.298Z
Learning: Providers implement TokenValidator interface and all token validation happens in provider/provider.go
Learnt from: CR
Repo: tuannvm/oauth-mcp-proxy PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-13T17:03:53.298Z
Learning: Applies to cache.go : Token caching uses sync.RWMutex with 5-minute TTL and background cleanup via deleteExpiredToken() goroutine to avoid lock contention
Author
There was a problem hiding this comment.
No, let's leave that decision for the maintainers.
There was a problem hiding this comment.
@drombosky, understood — I'll leave that to the maintainers. Thanks for the clarification!
🧠 Learnings used
Learnt from: CR
Repo: tuannvm/oauth-mcp-proxy PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-13T17:03:53.298Z
Learning: Providers implement TokenValidator interface and all token validation happens in provider/provider.go
Learnt from: CR
Repo: tuannvm/oauth-mcp-proxy PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-13T17:03:53.298Z
Learning: Applies to cache.go : Token caching uses sync.RWMutex with 5-minute TTL and background cleanup via deleteExpiredToken() goroutine to avoid lock contention
There was a problem hiding this comment.
🧹 Nitpick comments (3)
oauth.go (2)
129-134: Consider logging when rejecting tokens within the buffer window.The validation-failure path (line 125) logs before returning, but the buffer-rejection path silently returns an error. This could make debugging harder when tokens are rejected for being too close to expiration.
💡 Suggested improvement
if time.Now().After(expiry) { + s.logger.Info("Token rejected: expires within buffer (hash: %s...)", tokenHash[:16]) return nil, fmt.Errorf("authentication failed: token expired or expiring too soon") }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@oauth.go` around lines 129 - 134, The token buffer-rejection branch silently returns an error; update the code around the expiry check (use symbols expiry, s.config, TokenExpiryBuffer, and time.Now()) to log a descriptive message (including expiry, TokenExpiryBuffer and current time) before returning the fmt.Errorf so token rejections within the buffer window are recorded for debugging; keep the existing error return value but ensure the log uses the same logger/context used elsewhere in this module.
129-131: The nil-config guard is appropriate for the deprecated OAuthMiddleware path only.The guard at lines 129-131 is not inconsistent with other methods—it's intentionally protective of the only method called on nil-config Servers. The
OAuthMiddlewarelegacy function (middleware.go:64-68) creates a Server without config and exclusively callsMiddleware(), which in turn only callsValidateTokenCached(). Methods likeGetAuthorizationServerMetadataURL()are never invoked on nil-config Servers; they're only called on properly configured instances from the modernWithOAuth()path.The design is safe as-is. However, if the codebase plans to support other methods being called on nil-config Servers, consider adding nil guards to all config-accessing methods for defensive programming. The current deprecation notice on
OAuthMiddlewaresufficiently documents this as a legacy-only pattern.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@oauth.go` around lines 129 - 131, The nil-config guard in the expiry adjustment is acceptable because the legacy OAuthMiddleware path creates Servers without config and only calls Middleware()→ValidateTokenCached(); leave the existing guard in expiry calculation as-is, but if you intend to support calling other methods on nil-config Servers in future, add consistent nil checks to all config-accessing methods (e.g., GetAuthorizationServerMetadataURL, any methods that read s.config) to early-return a clear error or no-op; locate and update those methods (and constructors like WithOAuth and OAuthMiddleware) so they either require non-nil config or defensively handle nil s.config.api_test.go (1)
763-763: Ignored error from token signing.The error from
SignedStringis discarded. While unlikely to fail in tests with a valid key, a signing failure would cause cryptic test failures downstream.💡 Suggested fix
- tokenString, _ := token.SignedString(cfg.JWTSecret) + tokenString, err := token.SignedString(cfg.JWTSecret) + if err != nil { + t.Fatalf("SignedString: %v", err) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api_test.go` at line 763, The call to token.SignedString(cfg.JWTSecret) currently ignores the returned error; change it to capture both returns (tokenString, err := token.SignedString(cfg.JWTSecret)), check err and fail the test immediately (e.g., t.Fatalf or require.NoError) so a signing failure surfaces clearly; update any surrounding test helper to propagate or handle the error accordingly so tokenString is only used when signing succeeded.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@api_test.go`:
- Line 763: The call to token.SignedString(cfg.JWTSecret) currently ignores the
returned error; change it to capture both returns (tokenString, err :=
token.SignedString(cfg.JWTSecret)), check err and fail the test immediately
(e.g., t.Fatalf or require.NoError) so a signing failure surfaces clearly;
update any surrounding test helper to propagate or handle the error accordingly
so tokenString is only used when signing succeeded.
In `@oauth.go`:
- Around line 129-134: The token buffer-rejection branch silently returns an
error; update the code around the expiry check (use symbols expiry, s.config,
TokenExpiryBuffer, and time.Now()) to log a descriptive message (including
expiry, TokenExpiryBuffer and current time) before returning the fmt.Errorf so
token rejections within the buffer window are recorded for debugging; keep the
existing error return value but ensure the log uses the same logger/context used
elsewhere in this module.
- Around line 129-131: The nil-config guard in the expiry adjustment is
acceptable because the legacy OAuthMiddleware path creates Servers without
config and only calls Middleware()→ValidateTokenCached(); leave the existing
guard in expiry calculation as-is, but if you intend to support calling other
methods on nil-config Servers in future, add consistent nil checks to all
config-accessing methods (e.g., GetAuthorizationServerMetadataURL, any methods
that read s.config) to early-return a clear error or no-op; locate and update
those methods (and constructors like WithOAuth and OAuthMiddleware) so they
either require non-nil config or defensively handle nil s.config.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 04a076b0-d495-4f08-9007-6d4f6f7953d7
📒 Files selected for processing (9)
api_test.goconfig.gocontext_propagation_test.godocs/SECURITY.mdintegration_test.gomiddleware.gooauth.goprovider/provider.goprovider/provider_test.go
✅ Files skipped from review due to trivial changes (1)
- docs/SECURITY.md
🚧 Files skipped from review as they are similar to previous changes (6)
- context_propagation_test.go
- integration_test.go
- middleware.go
- provider/provider_test.go
- provider/provider.go
- config.go
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@oauth.go`:
- Around line 132-137: The current check uses time.Now().After(expiry) which
permits caching when expiry == now; change the conditional to be inclusive so
tokens with expiry equal to now are rejected (e.g., use if
!time.Now().Before(expiry) or if time.Now().After(expiry) ||
time.Now().Equal(expiry)) so the s.logger.Info message and return error happen
when expiry <= now, and only call s.cache.setCachedToken(tokenHash, user,
expiry) for tokens with strictly positive remaining lifetime.
🪄 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: 03b08b88-03b7-42e1-a493-405b67417720
📒 Files selected for processing (9)
api_test.goconfig.gocontext_propagation_test.godocs/SECURITY.mdintegration_test.gomiddleware.gooauth.goprovider/provider.goprovider/provider_test.go
✅ Files skipped from review due to trivial changes (2)
- docs/SECURITY.md
- integration_test.go
🚧 Files skipped from review as they are similar to previous changes (5)
- provider/provider_test.go
- context_propagation_test.go
- config.go
- api_test.go
- provider/provider.go
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
middleware.go (1)
15-28:⚠️ Potential issue | 🟡 MinorStale doc comment: "5-minute TTL" is no longer accurate.
The doc comment at line 20 still references a fixed "5-minute TTL", but the cache TTL is now derived from the JWT
expclaim minusTokenExpiryBuffer.📝 Proposed fix
// Middleware returns an authentication middleware for MCP tools. // Validates OAuth tokens, caches results, and adds authenticated user to context. // // The middleware: // 1. Extracts OAuth token from context (set by CreateHTTPContextFunc) -// 2. Checks token cache (5-minute TTL) +// 2. Checks token cache (TTL derived from JWT exp minus TokenExpiryBuffer) // 3. Validates token using configured provider if not cached // 4. Adds User to context via userContextKey // 5. Passes request to tool handler with authenticated context🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@middleware.go` around lines 15 - 28, Update the stale doc comment that says "5-minute TTL" to describe the current behavior: the cache TTL is computed from the JWT exp claim minus TokenExpiryBuffer (instead of a fixed 5 minutes). Mention the relevant symbols so readers can find the implementation: TokenExpiryBuffer, userContextKey, GetUserFromContext, WithOAuth, and CreateHTTPContextFunc; keep the rest of the comment about extracting token, checking cache, validating with provider, adding User to context, and passing to handler unchanged.context_propagation_test.go (1)
154-158:⚠️ Potential issue | 🟡 MinorStale comment: signature documentation is outdated.
The comments at lines 157-158 still document the old signature
ValidateToken(ctx context.Context, token string) (*User, error), but the method now returns(*User, time.Time, error).📝 Proposed fix
// The key point: interface method signature - // ValidateToken(ctx context.Context, token string) (*User, error) + // ValidateToken(ctx context.Context, token string) (*User, time.Time, error) t.Logf("✅ OIDCValidator implements TokenValidator with context.Context") - t.Logf(" Signature: ValidateToken(ctx context.Context, token string) (*User, error)") + t.Logf(" Signature: ValidateToken(ctx context.Context, token string) (*User, time.Time, error)") t.Logf(" Context flow: HTTP → MCP → Middleware → ValidateToken(ctx) → OIDC Provider")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@context_propagation_test.go` around lines 154 - 158, Update the stale comment in context_propagation_test.go that documents the ValidateToken signature: change the old signature "ValidateToken(ctx context.Context, token string) (*User, error)" to the current "ValidateToken(ctx context.Context, token string) (*User, time.Time, error)" and ensure any accompanying descriptive text (e.g., context flow lines) remains accurate for the updated return values; locate the comment near the tests referencing ValidateToken and adjust it to reflect the new return types.
🧹 Nitpick comments (1)
config.go (1)
349-364: Consider addingTOKEN_EXPIRY_BUFFERenvironment variable support.
FromEnv()doesn't parse aTOKEN_EXPIRY_BUFFER(or similar) environment variable, so users relying on environment-based configuration cannot configure this feature without code changes.💡 Proposed enhancement
+// parseDurationEnv parses a duration environment variable +func parseDurationEnv(key string, defaultVal time.Duration) time.Duration { + val := getEnv(key, "") + if val == "" { + return defaultVal + } + parsed, err := time.ParseDuration(val) + if err != nil { + return defaultVal + } + return parsed +}Then in
FromEnv():return NewConfigBuilder(). // ... existing calls ... WithValidatorIssuer(getEnv("OIDC_VALIDATOR_ISSUER", "")). + WithTokenExpiryBuffer(parseDurationEnv("TOKEN_EXPIRY_BUFFER", 0)). WithServerURL(serverURL).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@config.go` around lines 349 - 364, FromEnv() doesn't read a TOKEN_EXPIRY_BUFFER env var; update the env loading chain to parse that variable and pass it into the config builder. Specifically, parse TOKEN_EXPIRY_BUFFER (e.g. via a new or existing helper like parseDurationEnv("TOKEN_EXPIRY_BUFFER", 0) or parseIntEnv("TOKEN_EXPIRY_BUFFER", 0)) and add .WithTokenExpiryBuffer(parsedValue) to the NewConfigBuilder() chain (and implement WithTokenExpiryBuffer on the builder and the underlying Config struct if missing) so environment-based configuration can control the token expiry buffer.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@context_propagation_test.go`:
- Around line 154-158: Update the stale comment in context_propagation_test.go
that documents the ValidateToken signature: change the old signature
"ValidateToken(ctx context.Context, token string) (*User, error)" to the current
"ValidateToken(ctx context.Context, token string) (*User, time.Time, error)" and
ensure any accompanying descriptive text (e.g., context flow lines) remains
accurate for the updated return values; locate the comment near the tests
referencing ValidateToken and adjust it to reflect the new return types.
In `@middleware.go`:
- Around line 15-28: Update the stale doc comment that says "5-minute TTL" to
describe the current behavior: the cache TTL is computed from the JWT exp claim
minus TokenExpiryBuffer (instead of a fixed 5 minutes). Mention the relevant
symbols so readers can find the implementation: TokenExpiryBuffer,
userContextKey, GetUserFromContext, WithOAuth, and CreateHTTPContextFunc; keep
the rest of the comment about extracting token, checking cache, validating with
provider, adding User to context, and passing to handler unchanged.
---
Nitpick comments:
In `@config.go`:
- Around line 349-364: FromEnv() doesn't read a TOKEN_EXPIRY_BUFFER env var;
update the env loading chain to parse that variable and pass it into the config
builder. Specifically, parse TOKEN_EXPIRY_BUFFER (e.g. via a new or existing
helper like parseDurationEnv("TOKEN_EXPIRY_BUFFER", 0) or
parseIntEnv("TOKEN_EXPIRY_BUFFER", 0)) and add
.WithTokenExpiryBuffer(parsedValue) to the NewConfigBuilder() chain (and
implement WithTokenExpiryBuffer on the builder and the underlying Config struct
if missing) so environment-based configuration can control the token expiry
buffer.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 83189017-8c6f-4d2c-9466-0c50721c46eb
📒 Files selected for processing (9)
api_test.goconfig.gocontext_propagation_test.godocs/SECURITY.mdintegration_test.gomiddleware.gooauth.goprovider/provider.goprovider/provider_test.go
✅ Files skipped from review due to trivial changes (3)
- integration_test.go
- docs/SECURITY.md
- provider/provider_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- provider/provider.go
Currently the code will cache a JWT for 5 minutes. This creates a timing bug and forces unnecessary caching. Since tokens are cached for 5 minutes, it is possible that a token with 1 second of expiration left is cached. The cache code pulls the token out of the cache without any validation, the expired token will be used for the next 4 minutes and 59 seconds. This work sets the cache expiration time based on the token expiration time. This results in each token being cached once and evicted from the cache at expiration. In cases where the JWT needs to be valid beyond this check (e.g., for downstream requests), users can optionally choose to expire the cache before the JWT expiration time with `TokenExpiryBuffer` option. This option also ensures JWTs that are within the expiration window are treated as expired. Signed-off-by: Tyler Drombosky <bowsky@gmail.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
Currently the code will cache a JWT for 5 minutes. This creates a timing bug and forces unnecessary caching.
Since tokens are cached for 5 minutes, it is possible that a token with 1 second of expiration left is cached. The cache code pulls the token out of the cache without any validation, the expired token will be used for the next 4 minutes and 59 seconds.
This work sets the cache expiration time based on the token expiration time. This results in each token being cached once and evicted from the cache at expiration.
In cases where the JWT needs to be valid beyond this check (e.g., for downstream requests), users can optionally choose to expire the cache before the JWT expiration time with
TokenExpiryBufferoption. This option also ensures JWTs that are within the expiration window are treated as expired.Changes
TokenExpiryBuffer time.Durationoption to ensure tokens have a minimum lifetime after validation.ValidateTokeninTokenValidatorto return the expiry on the JWT (5 minutes if one is not present).server.Middleware()to useserver.ValidateTokenCachedto unify caching tokens.Testing
Summary by CodeRabbit
New Features
Behavior Changes
Documentation
Tests