fix: persist OAuth client credentials for token refresh

The silent token refresh flow was broken because clientID and clientSecret
were not stored with the OAuthToken. When getValidToken attempted to refresh
an expired token, it passed empty strings for both credentials, causing OAuth
servers to reject the request.

- Add ClientID and ClientSecret fields to OAuthToken struct
- Store client credentials when exchanging authorization codes for tokens
- Preserve client credentials through token refresh cycles
- Pass stored credentials in getValidToken silent refresh path
- Add tests covering credential persistence and refresh flow

Assisted-By: docker-agent

Author: dgageot

pkg/tools/mcp/oauth.go

@@ -237,7 +237,7 @@ func (t *oauthTransport) getValidToken(ctx context.Context) *OAuthToken {
 		return nil
 	}
 
-	newToken, err := RefreshAccessToken(ctx, metadata.TokenEndpoint, token.RefreshToken, "", "")
+	newToken, err := RefreshAccessToken(ctx, metadata.TokenEndpoint, token.RefreshToken, token.ClientID, token.ClientSecret)
 	if err != nil {
 		slog.Debug("Token refresh failed, will require interactive auth", "error", err)
 		return nil
@@ -389,6 +389,9 @@ func (t *oauthTransport) handleManagedOAuthFlow(ctx context.Context, authServer,
 		return fmt.Errorf("failed to exchange code for token: %w", err)
 	}
 
+	token.ClientID = clientID
+	token.ClientSecret = clientSecret
+
 	if err := t.tokenStore.StoreToken(t.baseURL, token); err != nil {
 		return fmt.Errorf("failed to store token: %w", err)
 	}

pkg/tools/mcp/oauth_helpers.go

@@ -79,6 +79,9 @@ func ExchangeCodeForToken(ctx context.Context, tokenEndpoint, code, codeVerifier
 		token.ExpiresAt = time.Now().Add(time.Duration(token.ExpiresIn) * time.Second)
 	}
 
+	token.ClientID = clientID
+	token.ClientSecret = clientSecret
+
 	return &token, nil
 }
 
@@ -202,5 +205,9 @@ func RefreshAccessToken(ctx context.Context, tokenEndpoint, refreshToken, client
 		token.RefreshToken = refreshToken
 	}
 
+	// Preserve client credentials so subsequent refreshes work
+	token.ClientID = clientID
+	token.ClientSecret = clientSecret
+
 	return &token, nil
 }

pkg/tools/mcp/oauth_test.go

@@ -0,0 +1,265 @@
+package mcp
+
+import (
+	"encoding/json"
+	"net/http"
+	"net/http/httptest"
+	"net/url"
+	"testing"
+	"time"
+)
+
+// TestExchangeCodeForToken_PreservesClientCredentials verifies that
+// ExchangeCodeForToken stores the client_id and client_secret on the
+// returned OAuthToken so they are available for subsequent refresh calls.
+func TestExchangeCodeForToken_PreservesClientCredentials(t *testing.T) {
+	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		if err := r.ParseForm(); err != nil {
+			t.Fatal(err)
+		}
+		if got := r.FormValue("client_id"); got != "my-client" {
+			t.Errorf("client_id = %q, want %q", got, "my-client")
+		}
+		if got := r.FormValue("client_secret"); got != "my-secret" {
+			t.Errorf("client_secret = %q, want %q", got, "my-secret")
+		}
+
+		w.Header().Set("Content-Type", "application/json")
+		_ = json.NewEncoder(w).Encode(map[string]any{
+			"access_token":  "at-new",
+			"token_type":    "Bearer",
+			"expires_in":    3600,
+			"refresh_token": "rt-new",
+		})
+	}))
+	defer srv.Close()
+
+	token, err := ExchangeCodeForToken(t.Context(), srv.URL, "code", "verifier", "my-client", "my-secret", "http://localhost/callback")
+	if err != nil {
+		t.Fatalf("ExchangeCodeForToken: %v", err)
+	}
+
+	if token.ClientID != "my-client" {
+		t.Errorf("ClientID = %q, want %q", token.ClientID, "my-client")
+	}
+	if token.ClientSecret != "my-secret" {
+		t.Errorf("ClientSecret = %q, want %q", token.ClientSecret, "my-secret")
+	}
+}
+
+// TestRefreshAccessToken_PreservesClientCredentials verifies that
+// RefreshAccessToken carries the client credentials through to the new token.
+func TestRefreshAccessToken_PreservesClientCredentials(t *testing.T) {
+	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		if err := r.ParseForm(); err != nil {
+			t.Fatal(err)
+		}
+		if got := r.FormValue("client_id"); got != "cid" {
+			t.Errorf("client_id = %q, want %q", got, "cid")
+		}
+		if got := r.FormValue("client_secret"); got != "csec" {
+			t.Errorf("client_secret = %q, want %q", got, "csec")
+		}
+
+		w.Header().Set("Content-Type", "application/json")
+		_ = json.NewEncoder(w).Encode(map[string]any{
+			"access_token": "at-refreshed",
+			"token_type":   "Bearer",
+			"expires_in":   7200,
+			// Server does NOT return a new refresh_token – old one should be preserved.
+		})
+	}))
+	defer srv.Close()
+
+	token, err := RefreshAccessToken(t.Context(), srv.URL, "old-rt", "cid", "csec")
+	if err != nil {
+		t.Fatalf("RefreshAccessToken: %v", err)
+	}
+
+	if token.AccessToken != "at-refreshed" {
+		t.Errorf("AccessToken = %q, want %q", token.AccessToken, "at-refreshed")
+	}
+	if token.RefreshToken != "old-rt" {
+		t.Errorf("RefreshToken = %q, want %q (should be preserved)", token.RefreshToken, "old-rt")
+	}
+	if token.ClientID != "cid" {
+		t.Errorf("ClientID = %q, want %q", token.ClientID, "cid")
+	}
+	if token.ClientSecret != "csec" {
+		t.Errorf("ClientSecret = %q, want %q", token.ClientSecret, "csec")
+	}
+}
+
+// TestGetValidToken_UsesStoredCredentialsForRefresh verifies that the
+// oauthTransport.getValidToken method sends the stored client credentials
+// when silently refreshing an expired token.
+func TestGetValidToken_UsesStoredCredentialsForRefresh(t *testing.T) {
+	var receivedClientID, receivedClientSecret string
+
+	// Use a mux so we can reference srv.URL in closures (srv is assigned before handlers run).
+	mux := http.NewServeMux()
+	srv := httptest.NewServer(mux)
+	defer srv.Close()
+
+	mux.HandleFunc("/.well-known/oauth-authorization-server", func(w http.ResponseWriter, _ *http.Request) {
+		w.Header().Set("Content-Type", "application/json")
+		_ = json.NewEncoder(w).Encode(map[string]any{
+			"issuer":                 srv.URL,
+			"token_endpoint":         srv.URL + "/token",
+			"authorization_endpoint": srv.URL + "/authorize",
+		})
+	})
+	mux.HandleFunc("/token", func(w http.ResponseWriter, r *http.Request) {
+		if err := r.ParseForm(); err != nil {
+			t.Fatal(err)
+		}
+		receivedClientID = r.FormValue("client_id")
+		receivedClientSecret = r.FormValue("client_secret")
+
+		w.Header().Set("Content-Type", "application/json")
+		_ = json.NewEncoder(w).Encode(map[string]any{
+			"access_token":  "fresh-at",
+			"token_type":    "Bearer",
+			"expires_in":    3600,
+			"refresh_token": "fresh-rt",
+		})
+	})
+
+	// Pre-populate an expired token with stored client credentials.
+	store := NewInMemoryTokenStore()
+	expiredToken := &OAuthToken{
+		AccessToken:  "old-at",
+		TokenType:    "Bearer",
+		RefreshToken: "old-rt",
+		ExpiresAt:    time.Now().Add(-1 * time.Hour), // expired
+		ClientID:     "stored-cid",
+		ClientSecret: "stored-csec",
+	}
+	if err := store.StoreToken(srv.URL, expiredToken); err != nil {
+		t.Fatal(err)
+	}
+
+	transport := &oauthTransport{
+		base:       http.DefaultTransport,
+		tokenStore: store,
+		baseURL:    srv.URL,
+	}
+
+	got := transport.getValidToken(t.Context())
+	if got == nil {
+		t.Fatal("getValidToken returned nil, expected refreshed token")
+	}
+	if got.AccessToken != "fresh-at" {
+		t.Errorf("AccessToken = %q, want %q", got.AccessToken, "fresh-at")
+	}
+	if receivedClientID != "stored-cid" {
+		t.Errorf("token endpoint received client_id = %q, want %q", receivedClientID, "stored-cid")
+	}
+	if receivedClientSecret != "stored-csec" {
+		t.Errorf("token endpoint received client_secret = %q, want %q", receivedClientSecret, "stored-csec")
+	}
+
+	// Verify the refreshed token also carries the credentials forward.
+	updated, err := store.GetToken(srv.URL)
+	if err != nil {
+		t.Fatalf("GetToken after refresh: %v", err)
+	}
+	if updated.ClientID != "stored-cid" {
+		t.Errorf("stored ClientID = %q, want %q", updated.ClientID, "stored-cid")
+	}
+	if updated.ClientSecret != "stored-csec" {
+		t.Errorf("stored ClientSecret = %q, want %q", updated.ClientSecret, "stored-csec")
+	}
+}
+
+// TestOAuthTokenClientCredentials_JSONRoundTrip verifies that ClientID and
+// ClientSecret survive JSON serialization (important for keyring storage).
+func TestOAuthTokenClientCredentials_JSONRoundTrip(t *testing.T) {
+	token := &OAuthToken{
+		AccessToken:  "at",
+		TokenType:    "Bearer",
+		RefreshToken: "rt",
+		ExpiresIn:    3600,
+		ExpiresAt:    time.Date(2025, 6, 1, 0, 0, 0, 0, time.UTC),
+		ClientID:     "cid",
+		ClientSecret: "csec",
+	}
+
+	data, err := json.Marshal(token)
+	if err != nil {
+		t.Fatalf("Marshal: %v", err)
+	}
+
+	var got OAuthToken
+	if err := json.Unmarshal(data, &got); err != nil {
+		t.Fatalf("Unmarshal: %v", err)
+	}
+
+	if got.ClientID != "cid" {
+		t.Errorf("ClientID = %q, want %q", got.ClientID, "cid")
+	}
+	if got.ClientSecret != "csec" {
+		t.Errorf("ClientSecret = %q, want %q", got.ClientSecret, "csec")
+	}
+}
+
+// TestOAuthTokenClientCredentials_OmittedWhenEmpty verifies the omitempty
+// tag works so tokens without client credentials don't leak empty fields.
+func TestOAuthTokenClientCredentials_OmittedWhenEmpty(t *testing.T) {
+	token := &OAuthToken{
+		AccessToken: "at",
+		TokenType:   "Bearer",
+	}
+
+	data, err := json.Marshal(token)
+	if err != nil {
+		t.Fatalf("Marshal: %v", err)
+	}
+
+	var raw map[string]any
+	if err := json.Unmarshal(data, &raw); err != nil {
+		t.Fatal(err)
+	}
+
+	if _, ok := raw["client_id"]; ok {
+		t.Error("client_id should be omitted when empty")
+	}
+	if _, ok := raw["client_secret"]; ok {
+		t.Error("client_secret should be omitted when empty")
+	}
+}
+
+// TestRefreshAccessToken_SendsEmptyClientIDWhenNotStored ensures that when
+// no client credentials were stored (legacy tokens), the refresh still
+// sends whatever was provided (empty string), matching the old behavior.
+func TestRefreshAccessToken_SendsEmptyClientIDWhenNotStored(t *testing.T) {
+	var receivedForm url.Values
+
+	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		if err := r.ParseForm(); err != nil {
+			t.Fatal(err)
+		}
+		receivedForm = r.Form
+
+		w.Header().Set("Content-Type", "application/json")
+		_ = json.NewEncoder(w).Encode(map[string]any{
+			"access_token": "new-at",
+			"token_type":   "Bearer",
+		})
+	}))
+	defer srv.Close()
+
+	_, err := RefreshAccessToken(t.Context(), srv.URL, "rt", "", "")
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	// client_id is always sent (even empty) per the current implementation.
+	if got := receivedForm.Get("client_id"); got != "" {
+		t.Errorf("client_id = %q, want empty", got)
+	}
+	// client_secret should NOT be sent when empty.
+	if receivedForm.Has("client_secret") {
+		t.Error("client_secret should not be sent when empty")
+	}
+}

pkg/tools/mcp/tokenstore.go

@@ -24,6 +24,8 @@ type OAuthToken struct {
 	RefreshToken string    `json:"refresh_token,omitempty"`
 	Scope        string    `json:"scope,omitempty"`
 	ExpiresAt    time.Time `json:"expires_at"`
+	ClientID     string    `json:"client_id,omitempty"`
+	ClientSecret string    `json:"client_secret,omitempty"`
 }
 
 // IsExpired checks if the token is expired