feat: OAuth refresh token grant with configurable TTLs#49
Conversation
- ScopedToken: add RefreshTokenHash and RefreshExpiresAt fields - TokenRegistry: add CreateWithRefresh() for access+refresh token pairs - TokenRegistry: add RotateViaRefreshToken() with single-use rotation - TokenRegistryFile: version 1->2 (backward-compatible Load) - handleOAuthToken: support grant_type=refresh_token - handleOAuthRegister/wellknown: advertise refresh_token support - Config: OAuthConfig with AccessTokenTTL (default 24h) and RefreshTokenTTL (default 720h/30d), file config + merge support - Full test coverage: refresh flow, rotation, expiry, config roundtrip Fixes #46
There was a problem hiding this comment.
Pull request overview
Adds OAuth 2.0 refresh-token grant support (RFC 6749 §6) to the MCP server. The token registry is extended with a refresh-token hash and refresh expiry, exposed through new CreateWithRefresh / RotateViaRefreshToken APIs (single-use rotation). The token endpoint, dynamic client registration response, and .well-known metadata are updated to advertise/handle refresh_token, and a new OAuthConfig block exposes configurable access/refresh TTLs (defaults 24h / 720h).
Changes:
- Extend
ScopedToken/TokenRegistryEntrywithRefreshTokenHashandRefreshExpiresAt, bump registry file version 1→2, addCreateWithRefreshandRotateViaRefreshToken. - Wire
grant_type=refresh_tokenintohandleOAuthToken, plumb TTLs fromMCPConfig.OAuththroughRunHTTPServerOnListener, and advertiserefresh_tokenin registration and well-known responses. - Introduce
OAuthConfig/fileOAuthConfigwith defaults and merge logic, plus tests for the OAuth flow and config roundtrip.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| internal/mcp/token.go | Adds refresh-token fields, lookup, creation, and single-use rotation; bumps file version to 2. |
| internal/mcp/token_test.go | Loosens version assertions to accept 1 or 2. |
| internal/mcp/serverbootstrap/oauth.go | Refactors handleOAuthToken into a grant-type switch and adds refresh_token handling; advertises new grant in registration. |
| internal/mcp/serverbootstrap/wellknown.go | Adds refresh_token to grant_types_supported. |
| internal/mcp/serverbootstrap/http.go | Reads OAuth TTLs from config and passes them to the token handler. |
| internal/mcp/serverbootstrap/oauth_refresh_test.go | New tests for full refresh flow, expired-refresh denial, registration/well-known advertising, and error paths. |
| internal/config/schema.go | Adds OAuthConfig, default TTLs, file-config pointer struct, and merge logic. |
| internal/config/schema_oauth_test.go | New tests for OAuth defaults and merge behavior (including partial/no overrides). |
Comments suppressed due to low confidence (3)
internal/mcp/token.go:446
- After rotation, the new access and refresh TTLs are computed as
time.Until(oldEntry.ExpiresAt)/time.Until(oldEntry.RefreshExpiresAt), i.e. the remaining lifetime of the old tokens. This causes refresh tokens to monotonically shrink with every rotation — after enough single-use rotations the refresh window collapses, requiring the user to re-run the full authorization-code flow even though they have been actively using the system. Most OAuth implementations either issue a fresh full-TTL refresh token on each rotation or implement a sliding window. Consider passing the configuredaccessTokenTTL/refreshTokenTTLintoRotateViaRefreshToken(or storing the TTLs on theScopedToken) so each rotation produces tokens with the intended lifetime.
var accessTTL, refreshTTL time.Duration
if oldEntry.ExpiresAt != nil {
accessTTL = time.Until(*oldEntry.ExpiresAt)
if accessTTL < 0 {
accessTTL = 0
}
}
if oldEntry.RefreshExpiresAt != nil {
refreshTTL = time.Until(*oldEntry.RefreshExpiresAt)
if refreshTTL < 0 {
refreshTTL = 0
}
}
newTok, rawAccess, rawRefresh, err := r.CreateWithRefresh(
oldEntry.Label,
oldEntry.AllowedTools,
oldEntry.AgentName,
accessTTL,
refreshTTL,
)
internal/mcp/token.go:446
oldEntry.ExpiresAtandoldEntry.RefreshExpiresAtare read here afterr.mu.Unlock()on line 424, whileCreateWithRefreshis then called which re-acquires the lock and may concurrently mutate the entries map. Although the pointer values on this specific entry are unlikely to change (it was just revoked), reading shared registry state outside the lock is fragile and inconsistent with the surrounding code. Capture the needed values (expiresAt,refreshExpiresAt,label,allowedTools,agentName) into local variables before unlocking on line 424.
oldEntry.Revoked = true
now := time.Now().UTC()
oldEntry.RevokedAt = &now
r.mu.Unlock()
var accessTTL, refreshTTL time.Duration
if oldEntry.ExpiresAt != nil {
accessTTL = time.Until(*oldEntry.ExpiresAt)
if accessTTL < 0 {
accessTTL = 0
}
}
if oldEntry.RefreshExpiresAt != nil {
refreshTTL = time.Until(*oldEntry.RefreshExpiresAt)
if refreshTTL < 0 {
refreshTTL = 0
}
}
newTok, rawAccess, rawRefresh, err := r.CreateWithRefresh(
oldEntry.Label,
oldEntry.AllowedTools,
oldEntry.AgentName,
accessTTL,
refreshTTL,
)
internal/mcp/token.go:334
- The refresh token hash is compared with a plain string
==equality (t.RefreshTokenHash == refreshHash). The PR description and linked issue both call out "constant-time-compare der Hashes" as part of the in-scope work. Although both sides are SHA-256 hex digests (already a one-way function, so timing leakage of the hash itself has limited security value), to satisfy the stated acceptance criterion and to be defensive, usecrypto/subtle.ConstantTimeComparefor the comparison.
func (r *TokenRegistry) getByRefreshTokenHash(refreshHash string) *ScopedToken {
for _, t := range r.entries {
if t.RefreshTokenHash == refreshHash && !t.Revoked && !t.IsExpired() {
return t
}
}
return nil
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // getByRefreshTokenHash looks up a token by its refresh token hash. Returns | ||
| // nil if not found, revoked, or expired. | ||
| func (r *TokenRegistry) getByRefreshTokenHash(refreshHash string) *ScopedToken { | ||
| for _, t := range r.entries { | ||
| if t.RefreshTokenHash == refreshHash && !t.Revoked && !t.IsExpired() { | ||
| return t | ||
| } | ||
| } | ||
| return nil | ||
| } |
Fixes #46
What
Describe the change in a few sentences.
Why
Explain the problem, user need, or maintenance reason.
Testing
make fmtmake vetmake lintGOWORK=off go test ./...Safety