Skip to content

Commit 527097b

Browse files
Dumbrisclaude
andauthored
fix(health): stop judging header-auth servers by a stale OAuth token record (#1262)
* fix(health): stop judging header-auth servers by a stale OAuth token record A server that once logged in with OAuth (autodiscovery, no `oauth` block) and was then switched to a static `Authorization` header keeps its old record in the oauth_tokens bucket: nothing removes it because the config never carried an OAuth block whose removal could have cleared it. The server-list projection built an `oauth` object from that dead record and, once its expiry passed, health reported the connected, tool-serving server as unhealthy / "Token expired" / login forever. After logout the in-memory refresh schedule outlived the token and the same server flipped to "Refresh token expired". - config: add ServerConfig.HasStaticAuthorizationHeader (case-insensitive name, non-blank value). OAuth populates that very header and the headers-auth strategy runs first, so the two are mutually exclusive. - runtime: a server with no `oauth` block and a static Authorization header no longer consults the stored token for its projection. The record is left alone (logout still removes it); it is simply not evidence for such a server. - health: refresh state only applies when OAuthRequired — it is a property of the token being refreshed. - runtime: TriggerOAuthLogout drops the refresh schedule alongside the token, as server removal already did. Closes #1172 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(health): let the live auth strategy override the static-header gate Review round 1 (opencode/astra): a rejected static Authorization header can fall through to the OAuth strategy, which then authenticates the connection with the stored token — and the header-based gate would have hidden that token's state. Record the strategy that won each HTTP/SSE connect attempt (core.Client.AuthStrategy, via a shared runAuthStrategies loop replacing the two identical ones), expose it as managed.Client.ConnectedWithOAuth, and let the runtime consult the stored token whenever the live connection was made with OAuth. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(health): withhold stale refresh state through one runtime seam, not the calculator Review round 2 (opencode/astra): guarding the calculator's refresh-state branch on OAuthRequired hid genuine refresh failures on the two surfaces (Server.GetAllServers for the tray, the MCP upstream_servers list) that only learn OAuthRequired from an explicit `oauth` block — an autodiscovery OAuth server with a failed refresh would have read healthy there. Revert the calculator change and move the decision to where the evidence is: Runtime.StoredOAuthTokenInPlay(name, cfg) answers "is the oauth_tokens record (and the RefreshManager schedule built from it) evidence about this server", and Runtime.HealthRefreshState wraps GetRefreshState with it. All three CalculateHealth call sites now read refresh state through that seam, so a header-authenticated server's stale schedule is withheld everywhere and an autodiscovery server's genuine failure is kept. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
1 parent fe3f4e3 commit 527097b

12 files changed

Lines changed: 555 additions & 78 deletions

‎internal/config/config.go‎

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2151,6 +2151,29 @@ func (sc *ServerConfig) IsAutoApproveToolChanges() bool {
21512151
return sc.EffectiveTrustMode() == TrustModeAuto
21522152
}
21532153

2154+
// HasStaticAuthorizationHeader reports whether the server is configured to
2155+
// authenticate with a static credential in the `Authorization` header (name
2156+
// matched case-insensitively, value non-blank).
2157+
//
2158+
// OAuth and a static Authorization header are mutually exclusive on the wire:
2159+
// the OAuth transport populates that very header, and the headers-auth
2160+
// strategy runs before OAuth is ever attempted. A config that declares one
2161+
// has therefore stopped using the other, which lets the server projection
2162+
// ignore an OAuth token record left behind by a previous OAuth login
2163+
// (GH #1172) without a heuristic on other header names. A nil receiver has no
2164+
// headers.
2165+
func (sc *ServerConfig) HasStaticAuthorizationHeader() bool {
2166+
if sc == nil {
2167+
return false
2168+
}
2169+
for name, value := range sc.Headers {
2170+
if strings.EqualFold(name, "Authorization") && strings.TrimSpace(value) != "" {
2171+
return true
2172+
}
2173+
}
2174+
return false
2175+
}
2176+
21542177
// IsToolAllowedByConfig reports whether toolName passes the server's static
21552178
// enabled_tools / disabled_tools filter. Returns true when neither list is set.
21562179
func (sc *ServerConfig) IsToolAllowedByConfig(toolName string) bool {
Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,41 @@
1+
package config
2+
3+
import (
4+
"testing"
5+
6+
"github.com/stretchr/testify/assert"
7+
)
8+
9+
// GH #1172: the server-list projection must be able to tell "this server
10+
// authenticates with a static Authorization header" apart from "this server
11+
// may use OAuth via autodiscovery" without consulting stored tokens.
12+
func TestServerConfig_HasStaticAuthorizationHeader(t *testing.T) {
13+
cases := []struct {
14+
name string
15+
headers map[string]string
16+
want bool
17+
}{
18+
{"nil headers", nil, false},
19+
{"empty headers", map[string]string{}, false},
20+
{"canonical Authorization", map[string]string{"Authorization": "Bearer abc"}, true},
21+
{"lower-case authorization", map[string]string{"authorization": "Bearer abc"}, true},
22+
{"mixed-case AUTHORIZATION", map[string]string{"AUTHORIZATION": "Basic xyz"}, true},
23+
{"empty Authorization value is not a credential", map[string]string{"Authorization": ""}, false},
24+
{"blank Authorization value is not a credential", map[string]string{"Authorization": " "}, false},
25+
{"unrelated header only", map[string]string{"X-Client-Version": "1.0"}, false},
26+
{"api-key style header is not Authorization", map[string]string{"X-API-Key": "k"}, false},
27+
{"Proxy-Authorization is not the resource credential", map[string]string{"Proxy-Authorization": "Basic x"}, false},
28+
}
29+
30+
for _, tc := range cases {
31+
t.Run(tc.name, func(t *testing.T) {
32+
sc := &ServerConfig{Name: "s", URL: "https://example.invalid/mcp", Headers: tc.headers}
33+
assert.Equal(t, tc.want, sc.HasStaticAuthorizationHeader())
34+
})
35+
}
36+
37+
t.Run("nil receiver", func(t *testing.T) {
38+
var sc *ServerConfig
39+
assert.False(t, sc.HasStaticAuthorizationHeader())
40+
})
41+
}

‎internal/runtime/runtime.go‎

Lines changed: 68 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -2364,11 +2364,16 @@ func (r *Runtime) GetAllServers() ([]map[string]interface{}, error) {
23642364
}
23652365
}
23662366

2367+
// GH #1172: a token record left behind by an earlier OAuth login
2368+
// is not evidence about a server that now authenticates with a
2369+
// static Authorization header — see StoredOAuthTokenInPlay.
2370+
tokenInPlay := r.StoredOAuthTokenInPlay(serverStatus.Name, serverStatus.Config)
2371+
23672372
// Check if server has valid OAuth token in storage
23682373
// IMPORTANT: This runs for ALL servers with a URL, including autodiscovery servers
23692374
// PersistentTokenStore uses serverKey (name + URL hash), not just server name
23702375
// We need to generate the same key format: "servername_hash16"
2371-
if url != "" && r.storageManager != nil {
2376+
if url != "" && r.storageManager != nil && tokenInPlay {
23722377
r.logger.Debug("Checking OAuth token in storage",
23732378
zap.String("server", serverStatus.Name),
23742379
// #1158: the configured upstream URL routinely carries a
@@ -2655,13 +2660,11 @@ func (r *Runtime) GetAllServers() ([]map[string]interface{}, error) {
26552660
}
26562661

26572662
// T032: Wire refresh state into health calculation (Spec 023)
2658-
if r.refreshManager != nil {
2659-
if refreshState := r.refreshManager.GetRefreshState(serverStatus.Name); refreshState != nil {
2660-
healthInput.RefreshState = health.RefreshState(refreshState.State)
2661-
healthInput.RefreshRetryCount = refreshState.RetryCount
2662-
healthInput.RefreshLastError = refreshState.LastError
2663-
healthInput.RefreshNextAttempt = refreshState.NextAttempt
2664-
}
2663+
if refreshState := r.HealthRefreshState(serverStatus.Name, serverStatus.Config); refreshState != nil {
2664+
healthInput.RefreshState = health.RefreshState(refreshState.State)
2665+
healthInput.RefreshRetryCount = refreshState.RetryCount
2666+
healthInput.RefreshLastError = refreshState.LastError
2667+
healthInput.RefreshNextAttempt = refreshState.NextAttempt
26652668
}
26662669

26672670
healthStatus := health.CalculateHealth(healthInput, healthConfig)
@@ -2914,6 +2917,55 @@ func (r *Runtime) TriggerOAuthLoginQuick(serverName string) (*core.OAuthStartRes
29142917
return result, nil
29152918
}
29162919

2920+
// StoredOAuthTokenInPlay reports whether a record in the oauth_tokens bucket
2921+
// (and the RefreshManager schedule built from it) is evidence about the named
2922+
// server's authentication state (GH #1172).
2923+
//
2924+
// It is not when the server has no `oauth` block and carries a static
2925+
// Authorization header: the headers-auth strategy runs first and OAuth is
2926+
// never attempted while it works, and OAuth would populate that very header,
2927+
// so a token left behind by an earlier OAuth login (autodiscovery servers have
2928+
// no `oauth` block whose removal could have cleared it) is stale. Consulting
2929+
// it used to report a connected, tool-serving upstream as unhealthy / "Token
2930+
// expired" / login for as long as the record existed, and its failed refresh
2931+
// schedule as "Refresh token expired". The record is left alone (logout still
2932+
// removes it); it is simply not consulted.
2933+
//
2934+
// The live connection has the final say: if the configured header was
2935+
// rejected and the OAuth strategy rescued the connection with that very
2936+
// token, the token IS in play and is reported as for any other OAuth server.
2937+
// A nil config (server known only to the state view) keeps the historical
2938+
// behaviour of consulting the record.
2939+
func (r *Runtime) StoredOAuthTokenInPlay(name string, cfg *config.ServerConfig) bool {
2940+
if cfg == nil || cfg.OAuth != nil || !cfg.HasStaticAuthorizationHeader() {
2941+
return true
2942+
}
2943+
return r.serverConnectedWithOAuth(name)
2944+
}
2945+
2946+
// HealthRefreshState returns the RefreshManager state to feed into the health
2947+
// calculator for the named server, or nil when there is none or when the token
2948+
// it rides on is not in play for this server (StoredOAuthTokenInPlay). Every
2949+
// CalculateHealth call site must read refresh state through this, or the
2950+
// REST, MCP and tray surfaces disagree about a header-authenticated server.
2951+
func (r *Runtime) HealthRefreshState(name string, cfg *config.ServerConfig) *oauth.RefreshStateInfo {
2952+
if r.refreshManager == nil || !r.StoredOAuthTokenInPlay(name, cfg) {
2953+
return nil
2954+
}
2955+
return r.refreshManager.GetRefreshState(name)
2956+
}
2957+
2958+
// serverConnectedWithOAuth reports whether the named server's live connection
2959+
// was authenticated by the OAuth strategy (GH #1172). False when the upstream
2960+
// manager or the client is absent, or the client is not connected that way.
2961+
func (r *Runtime) serverConnectedWithOAuth(name string) bool {
2962+
if r.upstreamManager == nil {
2963+
return false
2964+
}
2965+
client, exists := r.upstreamManager.GetClient(name)
2966+
return exists && client != nil && client.ConnectedWithOAuth()
2967+
}
2968+
29172969
// TriggerOAuthLogout implements RuntimeOperations interface for management service.
29182970
// Clears OAuth token and disconnects a specific server.
29192971
func (r *Runtime) TriggerOAuthLogout(serverName string) error {
@@ -2938,6 +2990,14 @@ func (r *Runtime) TriggerOAuthLogout(serverName string) error {
29382990
return fmt.Errorf("failed to clear OAuth token: %w", err)
29392991
}
29402992

2993+
// GH #1172: the refresh schedule rides on the token that was just removed.
2994+
// Left behind, a failed schedule kept reporting the logged-out server as
2995+
// "Refresh token expired" instead of "Logged out" — the server-removal path
2996+
// already does this, logout did not.
2997+
if r.refreshManager != nil {
2998+
r.refreshManager.OnTokenCleared(serverName)
2999+
}
3000+
29413001
// Disconnect the server to force re-authentication
29423002
if err := r.upstreamManager.DisconnectServer(serverName); err != nil {
29433003
r.logger.Warn("Failed to disconnect server after OAuth logout",

0 commit comments

Comments
 (0)