From 8eac9aa88d04f4b7ffb0d7d98af370323fe09a54 Mon Sep 17 00:00:00 2001 From: euxaristia <25621994+euxaristia@users.noreply.github.com> Date: Tue, 14 Jul 2026 04:36:21 -0400 Subject: [PATCH] fix(oauth): store keyring tokens as one entry per provider The keyring backend combined every provider and MCP token into a single JSON blob under one keyring entry. On macOS, add-generic-password now writes through security -i, whose command parser caps a single write at 4095 bytes (#574). The combined blob has no such bound: three or more logged-in providers routinely exceeds it, so Set() starts failing for every provider, not just the one that pushed it over. Split storage into one keyring entry per token key, plus a small index entry listing which keys exist (KeyringClient has no list operation). Each write is now bounded to a single token's size, well under the line cap regardless of how many providers are logged in. Installs on the old combined-entry format keep reading correctly via a legacy fallback, and get migrated to per-key entries on the next save. --- internal/oauth/store.go | 145 +++++++++++++++++++++++++-- internal/oauth/store_keyring_test.go | 112 ++++++++++++++++++++- 2 files changed, 244 insertions(+), 13 deletions(-) diff --git a/internal/oauth/store.go b/internal/oauth/store.go index 951e9616f..f111c8a4e 100644 --- a/internal/oauth/store.go +++ b/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. diff --git a/internal/oauth/store_keyring_test.go b/internal/oauth/store_keyring_test.go index 8931dc6de..d79294be0 100644 --- a/internal/oauth/store_keyring_test.go +++ b/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: "user@example.com", + 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) {