[peppescg]

Summary

When an embedded auth server uses DCR against an upstream ToolHive auth server, the two sides disagree about how long a registration lives. The downstream caches it forever. The upstream evicts DCR clients after DefaultDCRClientTTL (30 days) without a token exchange, while advertising client_secret_expires_at: 0. After an eviction, or any loss of the upstream's client storage, every authorization is rejected with invalid_client. Only the browser sees that error, so nothing recovers until someone deletes the cached row by hand.

• On a cache hit, ResolveCredentials now checks the cached client against the upstream token endpoint, when the credential store implements the new optional dcr.Replacer interface. It redeems a random authorization code with the cached credentials. Servers that authenticate the client before they validate the grant answer:

  • invalid_client when the registration is gone, so the resolver re-registers;
  • invalid_grant when it is alive, so the cached client is kept.

  ToolHive's own auth server (fosite) behaves this way, for public and confidential clients alike. Anything else (5xx, 429, non-OAuth bodies, network errors, redirects) is inconclusive and keeps the cached client, so an unreachable upstream never blocks startup.

• On invalid_client the resolver registers a new client and replaces the stale row through Replacer.ReplaceIfUnchanged. That method is built on UpdateDCRCredentialsIfUnchanged (Add compare-and-set update for DCR credentials #6758), so concurrent replicas converge on one registration and never overwrite a newer one.

• dcr.CredentialStore itself is unchanged. Replacer is a separate interface, like http.Flusher, implemented by both in-tree stores. A store that does not implement it keeps today's cache-hit behaviour: no probe, no request to the upstream.

• The cache read and the check run inside the existing singleflight, so concurrent callers in one process share a single read, check and registration.

• Registration code is split out of registerAndCache into registerClient / checkAuthoritativeResolution, so the cache-miss and cache-hit paths share it.

Fixes #6821

Type of change

• Bug fix
• New feature
• Refactoring (no behavior change)
• Dependency update
• Documentation
• Other (describe):

Test plan

• Unit tests (task test)
• E2E tests (task test-e2e)
• Linting (task lint-fix)
• Manual testing (describe below)

task test passes except TestGetOverlayMounts (pkg/ignore, not touched here). It failed once in the full run and passes on repeated isolated runs with and without this change.

New tests:

• pkg/authserver/runner/dcr_evicted_registration_test.go is an end-to-end reproduction. It chains two in-process auth servers, each with its own miniredis. The downstream registers on the upstream via DCR, the upstream's Redis is fast-forwarded past DefaultDCRClientTTL, and the downstream restarts on its persisted cache. Before the fix the restarted downstream presents the evicted client_id and the upstream answers 401 invalid_client. With the fix the upstream accepts the new client.
• The same file has a public-client variant (token_endpoint_auth_method=none, driven through the resolver as the CLI flow does) against the same ToolHive upstream: a live client is kept, an evicted one is replaced. It fails without the probe.
• validate_test.go is table-driven over every probe outcome (invalid_grant, invalid_client, other OAuth errors, 5xx, non-OAuth bodies, refused redirects). It covers each auth method (client_secret_basic, client_secret_post, none, plus private_key_jwt, which is skipped) and a failed re-registration. It also checks that the probe uses the configured token endpoint rather than the cached one, that a rejected row is never returned, that a store without Replacer (including one that embeds CredentialStore) is never probed, that a second caller arriving after the first has read the rejected row causes exactly one new registration (two if the cache is read outside the flight), and that the probe's log attributes carry only the HTTP status and OAuth error code, never the server's response text.
• store_test.go tests ReplaceIfUnchanged on both stores: replace, keep a concurrent winner, claim a vanished row, reject nil input.

Changes

File	Change
pkg/auth/dcr/validate.go	Token-endpoint probe and outcome classification (reuses oautherr and the guarded DCR HTTP client)
pkg/auth/dcr/resolver.go	Read the cache inside the singleflight; verify on cache hit when the store is a Replacer, re-register on rejection; registration extracted into registerClient
pkg/auth/dcr/store.go	Optional Replacer interface (ReplaceIfUnchanged on top of UpdateDCRCredentialsIfUnchanged), implemented by both stores
pkg/authserver/runner/embeddedauthserver_test.go	Cache-hit test now expects exactly one verification request
rotationKey docs (pkg/authserver/config.go, CRD type, request.go, storage/types.go, docs/arch) + regenerated CRDs, CRD reference, swagger, Go SDK	Say which dropped registrations the probe replaces on its own and which still need a rotation

Does this introduce a user-facing change?

An embedded auth server whose cached upstream DCR client was deleted upstream now registers a new client at startup instead of failing every authorization with invalid_client. Each resolution of a cached DCR registration through a store that implements Replacer makes one extra token-endpoint request; for the embedded auth server that is once per upstream per start.

Special notes for reviewers

• The probe only decides when the upstream authenticates the client first. RFC 6749 §4.1.3 requires confidential clients to authenticate at the token endpoint. For a public client (none) there is nothing to authenticate, and some servers validate the code first: they answer invalid_grant for any client_id, live, deleted or made up. Examples are the vendor reported in Cached upstream DCR registration never recovers after the upstream evicts it #6821 and better-auth. Against those servers a deleted registration still reads as alive, so the probe is a no-op and behavior is unchanged from today, not a regression. Add dcrConfig.rotationKey to re-register upstream DCR clients #6825 (dcrConfig.rotationKey) is the complementary operator lever for that case. Longer term, Client ID Metadata Documents avoid the problem entirely, since the upstream keeps no stored registration; the MCP spec now deprecates DCR in favour of CIMD. That is out of scope here.
• Startup only. For the embedded auth server the resolver runs when the auth server is constructed, so a registration dropped while the process is running is recovered at the next restart. Recovering in place would need mutable upstream credentials. That is a possible follow-up, kept out of this PR.
• Upstream logs. The probe shows up on the upstream as a failed token request (invalid_grant), logged at Debug by ToolHive. No token can be issued for the random code.
• Probe target. The probe goes to the operator-configured token endpoint when one is set, not the endpoint stored in the cached row. A tampered cache therefore cannot redirect the client secret. Redirects are refused, and the SSRF policy matches registration's.
• No interface change. CredentialStore keeps Get and PutIfAbsent, so stores implemented outside this repo keep compiling. A wrapper that embeds CredentialStore does not pick up Replacer either, so it can never inherit a replacement that skips what it does around writes; to opt in, it implements and forwards ReplaceIfUnchanged on purpose. Callers that resolve often (on a poll rather than at start) stay unprobed until they do.

🤖 Generated with Claude Code

[codecov]

Codecov Report

❌ Patch coverage is 91.60305% with 11 lines in your changes missing coverage. Please review.
✅ Project coverage is 79.63%. Comparing base (14c6066) to head (80ffcca).
⚠️ Report is 2 commits behind head on main.

Files with missing lines	Patch %	Lines
pkg/auth/dcr/validate.go	91.22%	5 Missing ⚠️
pkg/auth/dcr/store.go	80.95%	4 Missing ⚠️
pkg/auth/dcr/resolver.go	96.22%	2 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff            @@
##             main    #6822    +/-   ##
========================================
  Coverage   79.62%   79.63%            
========================================
  Files         815      816     +1     
  Lines       82955    83072   +117     
========================================
+ Hits        66056    66157   +101     
- Misses      16894    16910    +16     
  Partials        5        5            

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[jhrozek]

Looks good overall. The probe is a sensible way to catch this, and the e2e test covering the chained case is a nice touch. I left a few inline notes, none blocking. Below are some things that aren't tied to a changed line.

Docs that the probe now contradicts. The rotationKey docs from #6825 say a dropped registration is "only visible in the user's browser, so the resolver cannot detect it" (pkg/authserver/config.go:906, mcpexternalauthconfig_types.go:1862, and the generated CRD, swagger and docs/operator/crd-api.md copies). With this PR that's only true when the upstream answers invalid_grant for any client, or the client uses private_key_jwt. Something like "at start ToolHive probes the cached client at the token endpoint and replaces it when the upstream answers invalid_client; where it can't tell, the rejection is only visible in the user's browser" would be accurate. Two similar spots: request.go:95-102 and storage/types.go:239-248 call rotation the way to retire a dropped registration. The stale-age warning in resolver.go ("consider rotating via RFC 7592 deregistration and re-registering at next startup") was never true on main, since a hit didn't re-register. It becomes true with this PR, and it could mention rotationKey too. If regenerating the docs makes this PR bigger than you'd like, a follow-up is fine.

Two definitions of "gone". This PR infers it from a token-endpoint invalid_client, and #6825 leaves it to the operator, who is usually reacting to the browser's authorize-endpoint error. It would help for the merged comment and the docs to say which situations the probe catches and which need a rotation.

Tests and the issue link. The e2e test only covers a confidential client (client_secret_basic). A public-client variant would cover the path the fosite reading relies on, which is the case #6821's reporter hit. And with rotationKey as the lever for upstreams where the probe can't tell, Refs #6821 might read more honestly than Fixes. Your call.

[peppescg]

Thanks for the review, I pushed two commits. cb686b0 covers the inline points, see the replies there.

80ffcca updates the rotationKey docs (config.go, the CRD type, request.go, storage/types.go, docs/arch) and regenerates the CRDs, crd-api.md, swagger and the SDK. I kept it in this PR since it's mostly regenerated text. They now say that at start ToolHive checks the cached client at the token endpoint and replaces it on invalid_client, and that rotationKey is the remedy where that check can't tell, which is an upstream that answers a dropped public client like a live one, or a private_key_jwt client. That should also cover the point about the two definitions of "gone". The stale-age warning now points at rotationKey instead of RFC 7592.

I added a public-client variant of the e2e test. It drives the resolver with PublicClient the way the CLI does, since the embedded server always registers a confidential client, and it fails without the probe.

On Fixes vs Refs I'd keep Fixes. The scenario in #6821 is a ToolHive upstream evicting an idle client, and both e2e tests now cover it. The vendor case from the comment is the one rotationKey covers. Happy to switch if you feel strongly.

[jhrozek]

Thanks, all of it looks good. Keeping Fixes is fine with me given both e2e tests cover the #6821 scenario.