fix(oauth): store keyring tokens as one entry per provider

internal/oauth/store.go

@@ -106,10 +106,23 @@ type KeyringClient interface {
 	Delete(service, account string) (bool, error)
 }
 
-// Keyring storage stores the whole token blob under one fixed entry.
+// Keyring storage splits the token blob into one keyring entry per token key,
+// plus a small index entry listing which keys exist. A single combined entry
+// (the original design) grows with every additional provider/MCP login and,
+// on macOS, add-generic-password now goes through `security -i`'s line-based
+// command parser (see internal/keyring), which caps a single write at 4095
+// bytes; three or more logged-in providers routinely exceeds that. Splitting
+// by key bounds each write to one token, which stays well under the cap
+// regardless of how many providers are logged in.
 const (
 	keyringService = "zero"
-	keyringAccount = "oauth-tokens"
+	// keyringLegacyAccount held the whole blob as one entry in the original
+	// design. New writes never use it; it is only read once, to migrate
+	// existing installs into the per-key format.
+	keyringLegacyAccount = "oauth-tokens"
+	// keyringIndexAccount holds a JSON array of the token keys that currently
+	// have their own keyring entry, since KeyringClient has no "list" operation.
+	keyringIndexAccount = "oauth-tokens-index"
 )
 
 // Store persists OAuth tokens (provider + MCP namespaces) as one JSON blob,
@@ -203,7 +216,7 @@ func NewStore(options StoreOptions) (*Store, error) {
 		if storePath, perr := ResolveStorePath(options.Env); perr == nil {
 			lockPath = filepath.Join(filepath.Dir(storePath), "oauth-keyring.lockfile")
 		}
-		return &Store{blob: keyringBlob{kr: kr, service: keyringService, account: keyringAccount, lockPath: lockPath}, now: now}, nil
+		return &Store{blob: keyringBlob{kr: kr, service: keyringService, legacyAccount: keyringLegacyAccount, indexAccount: keyringIndexAccount, lockPath: lockPath}, now: now}, nil
 	default:
 		return nil, fmt.Errorf("oauth: unknown storage %q (want \"file\", \"encrypted-file\", or \"keyring\")", storage)
 	}
@@ -431,19 +444,70 @@ func (b fileBlob) withLock(now func() time.Time, fn func() error) error {
 
 func (b fileBlob) location() string { return b.path }
 
-// keyringBlob persists the blob in the OS keyring as a single base64 entry
-// (base64 keeps the multi-line JSON a single, control-character-free value).
+// keyringBlob persists tokens in the OS keyring as one base64 entry per token
+// key (account = key), plus an index entry listing which keys exist (base64
+// keeps every value a single, control-character-free string; see keyringService
+// for why a single combined entry doesn't work). read/write still present the
+// same whole-blob shape (a marshaled storeFile) that Store expects, fanning it
+// out to/in from the individual entries internally.
 type keyringBlob struct {
 	kr      KeyringClient
 	service string
-	account string
+	// legacyAccount is the pre-migration whole-blob entry; read only, to pick up
+	// tokens saved by older versions the first time this runs.
+	legacyAccount string
+	indexAccount  string
 	// lockPath, when set, is a cross-process lock file serializing the keyring's
 	// read-modify-write so concurrent processes don't clobber each other's tokens.
 	lockPath string
 }
 
 func (b keyringBlob) read() ([]byte, bool, error) {
-	enc, ok, err := b.kr.Get(b.service, b.account)
+	indexEnc, ok, err := b.kr.Get(b.service, b.indexAccount)
+	if err != nil {
+		return nil, false, err
+	}
+	if !ok {
+		return b.readLegacy()
+	}
+	keys, err := decodeKeyIndex(indexEnc)
+	if err != nil {
+		return nil, false, fmt.Errorf("oauth: decode keyring token index: %w", err)
+	}
+	tokens := make(map[string]Token, len(keys))
+	for _, key := range keys {
+		enc, ok, err := b.kr.Get(b.service, key)
+		if err != nil {
+			return nil, false, err
+		}
+		if !ok {
+			// Index and entries fell out of sync (e.g. a killed process between
+			// writing an entry and updating the index); skip rather than fail the
+			// whole read, since the next Save/Delete will reconcile the index.
+			continue
+		}
+		raw, err := base64.StdEncoding.DecodeString(strings.TrimSpace(enc))
+		if err != nil {
+			return nil, false, fmt.Errorf("oauth: decode keyring token entry %q: %w", key, err)
+		}
+		var token Token
+		if err := json.Unmarshal(raw, &token); err != nil {
+			return nil, false, fmt.Errorf("oauth: invalid keyring token entry %q: %w", key, err)
+		}
+		tokens[key] = token
+	}
+	data, err := json.Marshal(storeFile{SchemaVersion: storeSchemaVersion, Tokens: tokens})
+	if err != nil {
+		return nil, false, err
+	}
+	return data, true, nil
+}
+
+// readLegacy reads the pre-migration whole-blob entry, for installs that
+// haven't written since upgrading. The next write() migrates them: it writes
+// per-key entries and an index, then deletes this entry.
+func (b keyringBlob) readLegacy() ([]byte, bool, error) {
+	enc, ok, err := b.kr.Get(b.service, b.legacyAccount)
 	if err != nil || !ok {
 		return nil, ok, err
 	}
@@ -455,7 +519,70 @@ func (b keyringBlob) read() ([]byte, bool, error) {
 }
 
 func (b keyringBlob) write(data []byte) error {
-	return b.kr.Set(b.service, b.account, base64.StdEncoding.EncodeToString(data))
+	var state storeFile
+	if err := json.Unmarshal(data, &state); err != nil {
+		return fmt.Errorf("oauth: encode keyring token blob: %w", err)
+	}
+	priorKeys, err := b.indexedKeys()
+	if err != nil {
+		return err
+	}
+	keys := make([]string, 0, len(state.Tokens))
+	for key, token := range state.Tokens {
+		raw, err := json.Marshal(token)
+		if err != nil {
+			return err
+		}
+		if err := b.kr.Set(b.service, key, base64.StdEncoding.EncodeToString(raw)); err != nil {
+			return err
+		}
+		keys = append(keys, key)
+	}
+	sort.Strings(keys)
+	indexData, err := json.Marshal(keys)
+	if err != nil {
+		return err
+	}
+	if err := b.kr.Set(b.service, b.indexAccount, base64.StdEncoding.EncodeToString(indexData)); err != nil {
+		return err
+	}
+	for _, key := range priorKeys {
+		if _, ok := state.Tokens[key]; !ok {
+			if _, err := b.kr.Delete(b.service, key); err != nil {
+				return err
+			}
+		}
+	}
+	// The index now exists and is authoritative; drop the legacy entry so a
+	// future read never falls back to it.
+	_, _ = b.kr.Delete(b.service, b.legacyAccount)
+	return nil
+}
+
+// indexedKeys returns the keys currently listed in the index, or nil if there
+// is no index yet (first write, or still on the legacy format).
+func (b keyringBlob) indexedKeys() ([]string, error) {
+	enc, ok, err := b.kr.Get(b.service, b.indexAccount)
+	if err != nil || !ok {
+		return nil, err
+	}
+	keys, err := decodeKeyIndex(enc)
+	if err != nil {
+		return nil, fmt.Errorf("oauth: decode keyring token index: %w", err)
+	}
+	return keys, nil
+}
+
+func decodeKeyIndex(enc string) ([]string, error) {
+	data, err := base64.StdEncoding.DecodeString(strings.TrimSpace(enc))
+	if err != nil {
+		return nil, err
+	}
+	var keys []string
+	if err := json.Unmarshal(data, &keys); err != nil {
+		return nil, err
+	}
+	return keys, nil
 }
 
 // withLock serializes the keyring's read-modify-write. Store.mu covers the
@@ -473,7 +600,7 @@ func (b keyringBlob) withLock(now func() time.Time, fn func() error) error {
 	return fn()
 }
 
-func (b keyringBlob) location() string { return "keyring:" + b.service + "/" + b.account }
+func (b keyringBlob) location() string { return "keyring:" + b.service + "/" + b.indexAccount }
 
 // FormatStatuses renders a human-readable status table without leaking token
 // material.

internal/oauth/store_keyring_test.go

@@ -1,6 +1,8 @@
 package oauth
 
 import (
+	"encoding/base64"
+	"encoding/json"
 	"strings"
 	"testing"
 )
@@ -49,13 +51,17 @@ func TestStoreKeyringBackendRoundTrip(t *testing.T) {
 		t.Fatalf("Load = %#v", got)
 	}
 
-	// The blob is stored base64-encoded, so the raw JSON field names never appear.
-	raw := kr.data[keyringService+"/"+keyringAccount]
+	// The token lives under its own entry (account = key), not one combined
+	// blob, and is base64-encoded so the raw JSON field names never appear.
+	raw := kr.data[keyringService+"/"+ProviderKey("demo")]
 	if raw == "" {
-		t.Fatal("nothing stored in keyring")
+		t.Fatal("nothing stored under the token's own keyring entry")
 	}
 	if strings.Contains(raw, "access_token") {
-		t.Fatalf("keyring blob is not encoded: %s", raw)
+		t.Fatalf("keyring entry is not encoded: %s", raw)
+	}
+	if raw := kr.data[keyringService+"/"+keyringLegacyAccount]; raw != "" {
+		t.Fatalf("legacy combined entry should not be written by new code: %s", raw)
 	}
 
 	removed, err := s.Delete(ProviderKey("demo"))
@@ -65,6 +71,104 @@ func TestStoreKeyringBackendRoundTrip(t *testing.T) {
 	if _, ok, _ := s.Load(ProviderKey("demo")); ok {
 		t.Fatal("token still present after delete")
 	}
+	// Delete must also drop the now-unused entry, not just remove it from the
+	// index, or a stale keyring item accumulates for every logout.
+	if _, ok := kr.data[keyringService+"/"+ProviderKey("demo")]; ok {
+		t.Fatal("deleted token's keyring entry was not removed")
+	}
+}
+
+// TestStoreKeyringManyProvidersStayUnderEntryLimit is the regression test for
+// the bug this backend originally shipped with: every provider's tokens were
+// combined into one keyring entry, and on macOS that entry is written through
+// `security -i`, whose command parser caps a single write around 4KB. Three or
+// more logged-in providers routinely exceeded it, so Set() would start failing
+// for every provider, not just the one pushing it over. Splitting into one
+// entry per key bounds each individual write to a single token regardless of
+// how many providers are logged in.
+func TestStoreKeyringManyProvidersStayUnderEntryLimit(t *testing.T) {
+	t.Setenv("XDG_CONFIG_HOME", t.TempDir())
+	kr := newFakeKR()
+	s, err := NewStore(StoreOptions{Storage: "keyring", Keyring: kr})
+	if err != nil {
+		t.Fatal(err)
+	}
+	// A realistically large single token: JWT-shaped access/ID tokens plus an
+	// opaque refresh token, comparable to what OIDC providers actually issue.
+	big := Token{
+		AccessToken:  "eyJhbGciOiJSUzI1NiJ9." + strings.Repeat("QUJDRA", 60) + ".sig",
+		RefreshToken: "rt_" + strings.Repeat("x", 80),
+		TokenType:    "Bearer",
+		Scopes:       []string{"openid", "profile", "email", "offline_access"},
+		Account:      "[email protected]",
+		IDToken:      "eyJhbGciOiJSUzI1NiJ9." + strings.Repeat("QUJDRA", 70) + ".sig",
+	}
+	providers := []string{"anthropic", "openai", "minimax", "zai", "google"}
+	for _, name := range providers {
+		if err := s.Save(ProviderKey(name), big); err != nil {
+			t.Fatalf("Save(%s): %v", name, err)
+		}
+	}
+	// Each individual keyring value must stay small even with 5 providers
+	// logged in: no entry aggregates more than one provider's tokens.
+	const singleTokenCeiling = 3000 // generous margin under the ~4095-byte line cap
+	for k, v := range kr.data {
+		if len(v) > singleTokenCeiling {
+			t.Fatalf("keyring entry %q is %d bytes, want < %d (aggregation regression)", k, len(v), singleTokenCeiling)
+		}
+	}
+	for _, name := range providers {
+		got, ok, err := s.Load(ProviderKey(name))
+		if err != nil || !ok {
+			t.Fatalf("Load(%s): ok=%v err=%v", name, ok, err)
+		}
+		if got.AccessToken != big.AccessToken {
+			t.Fatalf("Load(%s) = %#v", name, got)
+		}
+	}
+}
+
+// TestStoreKeyringMigratesLegacyCombinedEntry ensures installs upgrading from
+// the original single-blob format keep reading their existing tokens, and get
+// migrated to per-key entries (with the legacy entry removed) the next time
+// anything is saved.
+func TestStoreKeyringMigratesLegacyCombinedEntry(t *testing.T) {
+	t.Setenv("XDG_CONFIG_HOME", t.TempDir())
+	kr := newFakeKR()
+	legacy := storeFile{SchemaVersion: storeSchemaVersion, Tokens: map[string]Token{
+		ProviderKey("demo"): {AccessToken: "legacy-a", RefreshToken: "legacy-r"},
+	}}
+	data, err := json.Marshal(legacy)
+	if err != nil {
+		t.Fatal(err)
+	}
+	kr.data[keyringService+"/"+keyringLegacyAccount] = base64.StdEncoding.EncodeToString(data)
+
+	s, err := NewStore(StoreOptions{Storage: "keyring", Keyring: kr})
+	if err != nil {
+		t.Fatal(err)
+	}
+	got, ok, err := s.Load(ProviderKey("demo"))
+	if err != nil || !ok {
+		t.Fatalf("Load legacy token: ok=%v err=%v", ok, err)
+	}
+	if got.AccessToken != "legacy-a" {
+		t.Fatalf("Load = %#v", got)
+	}
+
+	// Saving a second provider must migrate: the legacy entry is dropped, and
+	// both tokens end up as their own entries.
+	if err := s.Save(ProviderKey("other"), Token{AccessToken: "other-a"}); err != nil {
+		t.Fatal(err)
+	}
+	if _, ok := kr.data[keyringService+"/"+keyringLegacyAccount]; ok {
+		t.Fatal("legacy combined entry should be removed after migration")
+	}
+	for _, name := range []string{"demo", "other"} {
+		if _, ok, err := s.Load(ProviderKey(name)); err != nil || !ok {
+			t.Fatalf("Load(%s) after migration: ok=%v err=%v", name, ok, err)
+		}
+	}
 }
 
 func TestNewStoreStorageSelection(t *testing.T) {