Repository navigation
fix(oauth): serialize token refresh per server and classify refresh errors (Spec 113-a) - #1498
Merged
Merged
Conversation
…rrors (Spec 113-a) Serialize every OAuth refresh flight per server key across mcp-go's reactive refresh (the token store it reads before each request) and the RefreshManager's proactive path, including the stored-DCR-credentials sub-path. A rotating authorization server now sees one refresh request per expiry instead of one per concurrent caller, which on reuse-detecting providers revoked the whole grant. - oauth.RefreshCoordinator: single-flight per server key on a detached 30 s context; re-reads the record and skips the network when another flight already rotated the token; waiters share the token or the error; terminal latch and transient cooldown keyed by the grant generation; FR-006a compare-and-swap so a login that supersedes a flight always wins. - PersistentTokenStore (and the CLI in-memory store) refresh through the coordinator once core binds the live handler, and hand mcp-go a token with a zero ExpiresAt so its unlocked refreshToken can never run. - SaveToken and the manual refresh path persist via one BBolt read-modify-write (storage.UpdateOAuthToken), fixing the lost DCR-credential update. - ClassifyRefreshError: RFC 6749 section 5.2 error code (transport.OAuthError), HTTP status (typed RefreshHTTPError / mcp-go's fixed text), network error types; substring matching only as the last fallback. invalid_client clears a DCR registration once (compare-and-clear) and is terminal; static clients are terminal with an actionable message; 5xx/429/network keep the backoff. - RefreshManager reads token records by server key (refreshed event fired never before), handles all terminal classes, and learns about reactive terminal outcomes through the coordinator completion hook.
Deploying mcpproxy-docs with
|
| Latest commit: |
a245f4a
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://25aa9efb.mcpproxy-docs.pages.dev |
| Branch Preview URL: | https://113-a-oauth-refresh-robustne.mcpproxy-docs.pages.dev |
|
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
Contributor
📦 Build ArtifactsWorkflow Run: View Run Available Artifacts
How to DownloadOption 1: GitHub Web UI (easiest)
Option 2: GitHub CLI gh run download 37326870262 --repo smart-mcp-proxy/mcpproxy-go
|
…ave (Spec 113-a SC-001) The shuffle CI run saw two token requests per expiry in TestOAuthRefreshRace_OneTokenRequestPerExpiry. Cause: RefreshOAuthTokenDirect reads the record when it runs, not when the RefreshManager timer fired. When a reactive flight rotated the token first, the proactive caller observed the NEW refresh token, so the FR-003 "already rotated" check could not tell, and a second flight rotated the just-minted grant again. The coordinator now records when a token was last saved for a key (a successful flight or NoteTokenSaved from a login/persist). A proactive request within MinRefreshInterval (5 s, the RefreshManager's own scheduling floor) of that save, for a non-expired token, is answered from storage without a network request. Reactive callers are unaffected. Adds a deterministic reproduction (reactive refresh, then a late proactive refresh) for both refresh sub-paths, plus coordinator unit tests.
…cts, DCR recovery, secret scrubbing) - Terminal latch: a login that saved a new grant between the flight's post-refresh read and the latch read supersedes the flight; no latch, no completion hook (FR-006a). - DCR compare-and-clear also requires the stored refresh token to be the one the failed request used, so a login that reused the client id keeps it. - Drop the "login flow active" exemption from the invalid_client clear: a connect attempt (tryOAuthAuth holds a flow) running its own refresh left the rejected registration in place and every sign-in reloaded it. The CAS protects registrations a login saved. A code exchange that still uses the cleared client id is told to sign in again, not that the AS rejects new registrations. - RefreshManager only fails the schedule it recorded the failure on; a schedule replaced by a login's OnTokenSaved keeps running. - ScrubRefreshError: mcp-go embeds error_description / raw bodies in refresh errors; the handler sub-path now returns a scrubbed, capped message with the original chain kept for classification. - CLI in-memory store records carry the save time, so short tokens are not refreshed on every request. - A client id given only via oauth.extra_params refreshes through the handler (OAuthTransportWrapper injects it) instead of ErrNoClientCredentials. - Tests: the race-test AS holds the token request 50 ms so callers overlap the flight; the DCR lost-update test checks write errors and token fields.
…ential echo, capped classification) - Terminal outcomes are applied only to the schedule they belong to: a proactive attempt only fails the schedule it started from, and a reactive completion hook carries the failed record's expiry and only fails a schedule built for that token. A login whose OnTokenSaved replaced the schedule after the flight's last generation check keeps running (FR-006a). - Refresh errors remove the exact credentials the request sent (refresh token, client secret) before pattern scrubbing, on both sub-paths; an opaque value echoed without a key=value shape was left in logs/events. - Classification reads the unscrubbed, uncapped text under a scrubbed error, so a long non-JSON body does not hide invalid_grant from the fallback.
…extra_params secret, classify long bodies) - The stored-DCR refresh redacts the credentials it sent from the error code field as well as the detail, and caps it. - A client_secret given via oauth.extra_params (injected into every token request by OAuthTransportWrapper) is redacted from refresh errors on both sub-paths. - A non-JSON error body longer than the detail cap takes its RFC 6749 code from the whole body, so invalid_grant still latches instead of retrying.
…3-a review round 4) The RFC 6749 code is only inferred from a non-JSON error body for non-5xx, non-429 responses; a gateway page that merely mentions invalid_grant stays transient instead of latching the grant.
…short-token proactive, stale bookkeeping) - A flight whose record was deleted (logout) or emptied while it ran hands out no token: its persist was discarded, so its result is too. - The proactive fresh-window skip applies only while the stored token has more than the window left; a very short token scheduled at expiry minus MinRefreshInterval is genuinely due. - The DCR "re-registration used" mark is only recorded when no token was saved since the flight read the state, so a login that re-registered in between is not marked as rejected. - A transient retry only reschedules the schedule that failed; a login's new schedule keeps its own timer. - "Still valid" in the reactive path is judged when the flight returns, so a token that expired during a slow failed refresh is not handed out. - The live handler's DCR client secret is redacted from refresh errors too.
…fresh, escaped credential echo) - The reactive "access token still valid" fallback only serves the token while the store still holds it; a logout (or login) during the flight yields an error instead of the pre-logout token. - Refresh errors redact the sent credentials in their raw, JSON-escaped and URL-escaped spellings (an AS may echo them inside a JSON error_description or a form-encoded body).
…a review round 7)
- The stored-DCR refresh renders a JSON error body from its decoded
error / error_description fields, so whatever JSON escaping the AS chose
is undone before the sent credentials are matched.
- SentValueSpellings also covers the plain (non-HTML-escaping) and
PHP-style ("\/") JSON spellings for the handler sub-path's raw bodies.
Dumbris
added a commit
that referenced
this pull request
Oct 6, 2026
#1498 drops the 'unless a login flow is active' exemption for the DCR clear: tryOAuthAuth holds a connection-wide flow during its own reactive refresh, so the exemption kept a rejected registration forever. The compare-and-clear on client id and refresh token protects anything a login saved.
…l (Spec 113-a review round 8)
Dumbris
enabled auto-merge (squash)
October 6, 2026 17:24
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
2 participants
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.
Problem
Gaps G1 and G2 of the Anthropic MCP-proxy talk audit ("How Anthropic uses MCP for its own product", https://www.youtube.com/watch?v=e8DMLtP5ibk), re-verified in the Spec 113 research:
OAuthHandler.getValidTokenrefreshes with no lock whenever the token store reports an expired token, and the RefreshManager's proactive path (RefreshOAuthTokenDirect, two sub-paths) refreshes independently. With an authorization server that rotates refresh tokens, N concurrent callers at expiry sent N refresh requests with the same refresh token. Providers with reuse detection revoke the whole grant, so the user has to sign in again.PersistentTokenStore.SaveTokenalso read the record and wrote it in two transactions, so a concurrent DCR write was lost. The new test reproduces both bugs onmain.GenerateServerKey(name, url). The token-age log and theoauth.token_refreshedevent never fired.invalid_clientfell through tofailed_other, so a dead DCR client was retried up to 50 times. 5xx and 429 were not told apart from other errors.What changed
oauth.RefreshCoordinator(new, process-wide): runs at most one flight per server key. The flight runs on a detached context with a 30 s timeout. Inside the flight it first re-reads the record, and if the refresh token was already rotated it returns the stored token with no network call. All waiters get the same token or the same error. After a terminal failure it latches, and after a transient failure reactive callers get a cooldown that mirrors the RefreshManager backoff. The latch and cooldown are keyed by grant generation (refresh-token and client-id fingerprint), so a login clears them. A flight whose generation was superseded during the request (for example by a login) has its result discarded and causes no side effects (FR-006a). Each flight logs one Info line with server, trigger, outcome class and whether the network call was skipped.Reactive path: core binds the live handler's refresh function to the token store handed to mcp-go (
c.bindOAuthRefresher(oauthConfig)after each OAuthc.client =). A boundGetToken:ExpiresAtzeroed, so mcp-go's unlockedrefreshTokencan never run;transport.ErrOAuthAuthorizationRequired;oauth.ErrTokenRefreshTransient.Unbound stores (read-only probes) behave as before. The CLI in-memory store uses the same coordinator, keyed by server name.
Proactive path:
RefreshOAuthTokenDirectruns a coordinator flight with the same refresh function. That function useshandler.RefreshTokenwhen the handler has a usable client id and the stored DCR credentials otherwise, and never sends an emptyclient_id.Storage:
BoltDB.UpdateOAuthToken(one read-modify-write transaction) andClearOAuthClientCredentialsIf(compare-and-clear).SaveTokenand the manual refresh persist throughUpdateOAuthToken.Classification:
ClassifyRefreshErrorreads the RFC 6749 §5.2 code fromtransport.OAuthErrorand the HTTP status from the typedRefreshHTTPErroror mcp-go's fixed non-JSON text. It also recognises network error types. Substring matching is kept only as the last fallback, and each pattern is tested.invalid_grantinvalid_client, DCR clientinvalid_client, static clientoauth.client_id/oauth.client_secretunauthorized_client,unsupported_grant_type,invalid_scopeserver_error,temporarily_unavailable, 5xx, 429, networkNew metric labels:
failed_invalid_client,failed_server_error. Existing labels are unchanged.RefreshManager: looks up records by server key, treats every terminal class as terminal and reports each one once, and learns about terminal outcomes of reactive flights through the coordinator completion hook.
Docs:
docs/features/oauth-authentication.mdcovers refresh serialization, the error classes and the metric labels.Tests
refresh_errors_test.go(table covering every class and fallback pattern),refresh_coordinator_test.go(single flight with N=50, stale observed token, shared failure, cancelled initiator, parallel keys, terminal latch, transient cooldown, stale flight after login,invalid_clientDCR/static/connect-flow/compare-and-clear, login-exchange rule),persistent_token_store_refresh_test.go,persistent_token_store_race_test.go,memory_token_store_refresh_test.go,refresh_manager_invalid_client_test.go,storage/bbolt_oauth_update_test.go.internal/upstream/core/oauth_refresh_race_test.gouses a rotatinghttptestAS that revokes the grant on reuse, a protected Streamable HTTP MCP server, and a real mcp-go client. 50 concurrentCallTools plus one concurrentRefreshOAuthTokenDirecton an expired token produce exactly 1 token request, all calls succeed, and the request carries a non-emptyclient_id. It covers both sub-paths and passes under-race -count=20. With the binding disabled, the same test fails: the grant is revoked and the calls get "authorization required".go test -racepassed forinternal/oauth/...,internal/storage/...,internal/upstream/...,internal/observability/...,internal/runtime/...andinternal/appctx/.... The coordinator, store and storage tests also pass with-count=20.go build ./cmd/mcpproxyandgo build -tags server -o /dev/null ./cmd/mcpproxyboth succeed..github/.golangci.yml, bare and with--build-tags server. Neither run reports anything in changed or new files. Both runs do report findings in files this PR does not touch (for exampleredactview.go,connection_stdio.goandtests/oauthserver/jwks.go)../scripts/test-api-e2e.shagainst an isolated instance (LISTEN_PORT=18347): passed. On the first run the launcher-lifecycle reconnect check timed out once; the rerun passed fully, as didorigin/mainon the same port.make swaggerwas not needed (FR-013).Assumptions
Updatedtime.GetTokenkeeps serving it. The terminal or transient error is returned only once the token has actually expired, so an AS outage does not fail calls early. The RefreshManager is told about terminal outcomes at once through the hook.MinRefreshInterval(5 s, the RefreshManager's own scheduling floor) of a token save, for a token with more than 5 s left, is answered from storage (see the CI note below).tryOAuthAuthholds a connection-wide OAuth flow while its own reactive refresh runs, so the exemption kept a rejected registration forever (every sign-in reloaded it and skipped DCR). The compare-and-clear on client id plus refresh token already protects any registration or grant a login saved. A code exchange that still uses the cleared client is told to sign in again, not that the AS rejects new registrations.DefaultMaxRetries50).ErrNoClientCredentials) and never sends an emptyclient_id.connection_oauth.go(one helper call each), not at a single site as the plan suggested, because there is no shared post-creation point. The three authorization-code exchange sites call a one-line annotation helper, which implements the FR-009 login-flow rule.RefreshManagerknows only the display name, so FR-010 matches the record byDisplayName(most recently updated wins), after trying a legacy record keyed by the bare name.upstream.Manager.RefreshOAuthTokenis unchanged and keeps its reconnect fallback on failure.CI shuffle failure (run 37307361769)
TestOAuthRefreshRace_OneTokenRequestPerExpiry/stored_DCR_credentials_sub-pathsaw 2 token requests, both valid (rt-0thenrt-1, no replay). Root cause was in production code:RefreshOAuthTokenDirectreads the record when it runs, not when the RefreshManager timer fired. When the reactive flight had already rotated the token, the proactive caller observed the new refresh token, so the FR-003 "already rotated" check could not tell, and a second flight rotated the just-minted grant. In production that is the timer firing while a reactive refresh (or a login) completes, beforeOnTokenSavedreschedules. Fix: the coordinator records the last token save per key and answers a proactive request within the fresh window from storage.TestOAuthRefreshRace_LateProactiveAfterReactiveFlightreproduces the CI ordering deterministically for both sub-paths (it failed before the fix). The race test's AS now holds the token request for 50 ms so callers overlap the flight. Verified with the CI seed (-shuffle=1791202369572320301 -count=20),GOMAXPROCS=1 -count=50and-count=50 -cpu 1,2,4under-race.Review fixes (opencode gpt-6.1-sol)
Stale-flight side effects (latch re-read, DCR compare-and-clear now also on the refresh token, schedule binding for terminal outcomes and retries, logout during a flight or a grace refresh hands out no token, DCR bookkeeping after a concurrent login); DCR recovery during a connect attempt; secret redaction of refresh errors (pattern scrub plus the exact sent credentials in raw, JSON- and URL-escaped spellings, decoded JSON error fields, error code field); short-token handling (CLI memory store save time, proactive window);
oauth.extra_params.client_idgrants stay refreshable; long non-JSON bodies keep their RFC 6749 code, while 5xx/429 stay transient.Spec: #1495
Review: opencode github-copilot/gpt-6.1-sol, 8 rounds, clean (round 8 VERDICT: CLEAN); follow-ups: proactive refreshes are not held by the reactive transient cooldown (by design, RefreshManager has its own backoff); the still-valid access token is served during the refresh margin even after a terminal refresh error (the hook applies the terminal outcome to the schedule at once).