fix(mcp): refresh OAuth tokens for discovered endpoints and registered clients

[PierrunoYT]

Summary

Login discovers the token endpoint (RFC 9728 / 8414) and can obtain a client_id/client_secret through dynamic registration, but none of it was persisted. On a 401, storeTokenSource.Refresh read only the config-file OAuthConfig, whose TokenEndpoint and ClientID are empty for the standard auth: oauth setup. Refresh then failed with oauth: no token endpoint configured for refresh, and users had to re-run zero mcp oauth login every time the access token expired.

Changes:

• oauth.Token / mcp.StoredToken gain TokenEndpoint, ProtectedTokenEndpoint, ClientID and ClientSecret (all omitempty, so existing stored tokens and provider logins are unchanged on disk).
• Login records the discovered token endpoint (and whether it was server-advertised) and the dynamically registered client. It only records what config did not already supply, so configured secrets are not copied into the token store.
• Refresh falls back to the stored endpoint and client through the new refreshSettings. Explicit config always wins. A configured secret is kept when only the client id comes from the store.
• A stored endpoint is re-validated before credentials are posted to it: oauth.ValidateEndpointURL always, and for a server-advertised endpoint also validateProtectedDiscoveredEndpoints with the same pinned public-network client Login used.
• refreshAccessToken carries the stored fields over to the refreshed token; the token response never contains them, so otherwise the second refresh would fail again.

Linked issue

Fixes #1101

Note: #1101 does not currently carry the issue-approved label. Opening this anyway at the author's request; it can be held until the issue is approved.

Verification

New tests in internal/mcp/oauth_refresh_stored_test.go:

• TestStoreTokenSourceRefreshUsesStoredDiscoveryAndClient runs a real discovery login (protected-resource metadata, dynamic registration, token endpoint), saves the token, then refreshes through storeTokenSource with no configured endpoint or client. It checks the refresh form used the registered client, that the stored fields survive the refresh, and that a second refresh works. With the refresh wiring reverted it fails with the issue's exact error (oauth: no token endpoint configured for refresh); with Login's recording reverted it fails because the client id is empty.
• TestLoginRecordsDiscoveredEndpointAndRegisteredClient and TestLoginDoesNotCopyConfiguredClientOrEndpointIntoStore.
• TestRefreshSettingsConfigOverridesStored, TestRefreshSettingsKeepsConfiguredSecretWithStoredClientID.
• TestRefreshSettingsRejectsUnsafeStoredEndpoint (cleartext http, advertised loopback, advertised private address) and TestStoreTokenSourceRefreshDoesNotPostToUnsafeStoredEndpoint, which checks a public MCP resource cannot send refresh to a loopback server (zero requests received; fails on the old code).
• TestStoredTokenPersistsDiscoveryFields covers the store round trip and that Status() output does not include the secret or endpoint.

Checks: go build ./..., go vet for internal/mcp, internal/oauth, internal/cli, gofmt -l internal/mcp internal/oauth and git diff HEAD --check are clean; go test ./internal/mcp/... ./internal/oauth/... passes.

Not run locally or not clean here:

• go test ./internal/cli/ has failures on this Windows machine unrelated to this change (for example TestRunExecLogsMCPRuntimeCloseError); I confirmed three of them fail the same way on unmodified main.
• go test -race (cgo unavailable here), full go test ./..., smoke and make targets are left to CI.

Checklist

• The linked issue already has the issue-approved label. (not yet)
• go build ./..., go vet ./..., and go test ./... pass locally. (build passes; vet and tests only for the affected packages)
• gofmt clean.
• Tests added/updated for the change (and run under -race where relevant). (tests added; -race not run locally)
• UI changes include screenshots or a short recording where possible. (no UI change)

🤖 Generated with Claude Code

Summary by CodeRabbit

• Bug Fixes
  • OAuth token refresh now reuses endpoint and client details learned during sign-in, helping sessions refresh successfully when those settings were discovered or dynamically registered.
  • Explicitly configured refresh settings take precedence over saved settings. Discovered protected endpoints are checked before credentials are sent, and unsafe saved endpoints are rejected.
  • Refreshed token details are retained when refresh tokens rotate.

[Copilot]

Copilot review overview

🟡 Changes recommended

The implementation appears sound, but issue #1101 lacks the required issue-approved label for a community contribution.

Review effort: Balanced
Findings: None

What changed in this PR

Fixes MCP OAuth refreshes by persisting and securely reusing discovered endpoints and dynamically registered clients.

Changes:

• Persists discovered OAuth endpoint and client credentials.
• Validates stored endpoints and preserves metadata across refreshes.
• Adds end-to-end, persistence, precedence, and security regression tests.

File	Description
internal/​oauth/​oauth.go	Extends the shared token format with refresh metadata.
internal/​mcp/​oauth.go	Records, validates, merges, and preserves discovered settings.
internal/​mcp/​oauth_store.go	Persists the new MCP token fields.
internal/​mcp/​oauth_refresh_stored_test.go	Tests refresh behavior and endpoint safety.
internal/​mcp/​network_client.go	Applies stored settings before refreshing.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[coderabbitai]

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Repository: Twigpine/zero/.coderabbit.yaml

Review profile: CHILL

Plan: Advanced

Run ID: 0b96dc4c-84c9-405a-a0df-74e5db79b77e

📥 Commits

Reviewing files that changed from the base of the PR and between 99721c7 and dea2295.

📒 Files selected for processing (5)

• internal/mcp/network_client.go
• internal/mcp/oauth.go
• internal/mcp/oauth_refresh_stored_test.go
• internal/mcp/oauth_store.go
• internal/oauth/oauth.go

Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 0 remain after this review.

---

Walkthrough

MCP OAuth tokens now retain discovered token endpoints and dynamically registered client credentials. Refresh resolves configured or stored values, validates stored endpoints, and preserves discovery details after token rotation. Tests cover login, persistence, refresh settings, unsafe endpoints, and status output redaction.

Changes

MCP OAuth refresh

Layer / File(s)	Summary
Capture and persist OAuth discovery values internal/oauth/oauth.go, internal/mcp/oauth_store.go, internal/mcp/oauth.go, internal/mcp/oauth_refresh_stored_test.go	OAuth tokens and stored tokens carry endpoint and client details. Login records discovered endpoints and dynamically registered credentials. Tests cover stored values and status output redaction.
Resolve and validate refresh settings internal/mcp/network_client.go, internal/mcp/oauth.go, internal/mcp/oauth_refresh_stored_test.go	Refresh resolves configured or stored settings, validates stored endpoints, and preserves discovery details after refresh. Tests cover settings precedence, token rotation, and unsafe endpoint rejection.

Priority: ➖ Normal

Estimated code review effort: 3 (Moderate) | ~25 minutes

Change: Bug fix · Severity of issue fixed: Medium

Sequence Diagram(s)

sequenceDiagram
  participant storeTokenSource
  participant refreshSettings
  participant refreshAccessToken
  participant TokenEndpoint
  storeTokenSource->>refreshSettings: Resolve config and client using server URL, context, and stored token
  refreshSettings-->>storeTokenSource: Return resolved config and HTTP client
  storeTokenSource->>refreshAccessToken: Pass resolved config, client, and stored token
  refreshAccessToken->>TokenEndpoint: Submit refresh request
  TokenEndpoint-->>refreshAccessToken: Return refreshed token
  refreshAccessToken-->>storeTokenSource: Return token with discovery fields preserved

Loading

Suggested reviewers: gnanam1990

Merge Risk: ⚪ Minimal · up to dea22

Discovery-based OAuth refresh appears ready to merge after normal checks; no actionable issue remains from this review.

Security Architecture Review

Security architecture risk: 🟡 Moderate · up to dea22

Protected-resource discovery retains strong destination checks during refresh. However, legacy discovery can now establish a persistent refresh destination without those public-network checks. This creates a conditional network-boundary concern, not a demonstrated exploit.

Retained concerns

• Medium · security · inferred: Legacy fallback discovery can now grant remote metadata durable refresh-destination authority without public-network revalidation. When protected-resource metadata is absent, the discovered endpoint is persisted with ProtectedTokenEndpoint false and later receives refresh credentials through the ordinary client. This extends the pre-existing interactive-login exposure into subsequent refreshes. Effects on private services remain conditional on successful initial login, destination configuration and TLS verification.

Security review details

Security Blast Radius

• inferred — The new authority concerns credentials for a selected MCP server and outbound requests from the user's process. Server identity includes URL and OAuth configuration, limiting ordinary credential reuse across changed server definitions. Private-network reachability depends on the endpoint's provenance, scheme, TLS verification and the host's network access; cross-tenant or infrastructure privileges are not established.

Security Findings and Attack Paths

• inferred — A controller of legacy discovery metadata can influence the endpoint retained after successful login. Later refresh submits the stored refresh token and applicable client credentials to that endpoint without protected-discovery address checks. Metadata influence is supported by the producer path, but an end-to-end attack against a private service was not demonstrated. Initial login already used the weaker destination policy; the added exposure is durable credential-bearing reuse.

Trust Boundaries and Controls

• observed — Protected-resource discovery persists endpoint provenance and rechecks it before refresh. The guarded transport rejects disallowed literal hosts and resolved addresses, pins accepted addresses while preserving TLS hostname verification, and rejects unsupported custom transports. Loopback is permitted only when the selected MCP resource explicitly authorizes it.
• observed — Explicit OAuth configuration takes precedence over stored settings. All token submissions retain HTTPS-or-loopback-HTTP validation and refuse redirects, so a redirect cannot replay refresh credentials to another origin. These controls also constrain the legacy-discovery concern, although they do not enforce public-address validation.

Resilience and Maintainability Implications

• observed — Individual file publications use temporary files and atomic rename, but refresh retains the pre-existing load, remote refresh, then save ordering. A persistence failure after provider-side token rotation cannot undo the remote transition. The inspected comparison does not show a newly weakened recovery mechanism; concurrent-refresh and interruption recovery were not demonstrated by runtime evidence.

Hardening Proposals

• proposed — Distinguish locally authorized endpoints from remotely learned legacy endpoints in the persisted provenance contract, and apply public-network validation to the latter during refresh unless an explicit local exception authorizes private or loopback destinations.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 40.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 15 functions across 5 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely describes the main change: enabling MCP OAuth token refresh for discovered endpoints and dynamically registered clients.
Linked Issues check	✅ Passed	The changes meet the coding requirements in [#1101]. Login persists discovered token endpoints and dynamically registered client credentials. Refresh uses stored values when configuration omits them…
Out of Scope Changes check	✅ Passed	The changed production files implement the [#1101] refresh behavior and token persistence. The added tests directly verify the requested behavior, security validation, configuration precedence, and se…

• Fix all pre-merge checks with AI

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create a new PR

• Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Autopilot is currently an internal CodeRabbit preview.

---

_{Comment @coderabbitai help to get the list of available commands.}