diff --git a/internal/oauth/flow.go b/internal/oauth/flow.go index 9ff94cccd..42da93986 100644 --- a/internal/oauth/flow.go +++ b/internal/oauth/flow.go @@ -177,10 +177,11 @@ func Refresh(ctx context.Context, client *http.Client, cfg Config, current Token if len(cfg.Scopes) > 0 { form.Set("scope", strings.Join(cfg.Scopes, " ")) } - // Carry the existing token_type forward: a refresh response commonly omits it, - // and PostToken only overwrites TokenType when the response supplies one, so - // without seeding it here the type would be silently lost across refreshes (L15). - base := Token{Scopes: current.Scopes, RefreshToken: refresh, Account: current.Account, IDToken: current.IDToken, TokenType: current.TokenType} + scopes := current.Scopes + if len(scopes) == 0 { + scopes = cfg.Scopes + } + base := Token{Scopes: scopes, RefreshToken: refresh, Account: current.Account, IDToken: current.IDToken, TokenType: current.TokenType} return PostToken(ctx, client, cfg.TokenEndpoint, form, base, now) } diff --git a/internal/oauth/flow_test.go b/internal/oauth/flow_test.go index 09ef01251..4b1da98d5 100644 --- a/internal/oauth/flow_test.go +++ b/internal/oauth/flow_test.go @@ -299,3 +299,18 @@ func TestRefreshPreservesTokenTypeWhenOmitted(t *testing.T) { t.Fatalf("refresh should carry the existing token_type forward, got %q", tok.TokenType) } } + +func TestRefreshPreservesScopesWhenOmitted(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + _, _ = w.Write([]byte(`{"access_token":"new-at","expires_in":3600}`)) // no scope in response + })) + defer server.Close() + cfg := Config{ClientID: "c", TokenEndpoint: server.URL, Scopes: []string{"fallback-scope"}} + tok, err := Refresh(context.Background(), server.Client(), cfg, Token{RefreshToken: "keep-me", Scopes: []string{"custom-scope"}}, nil) + if err != nil { + t.Fatalf("Refresh: %v", err) + } + if len(tok.Scopes) != 1 || tok.Scopes[0] != "custom-scope" { + t.Fatalf("refresh should carry existing scopes forward, got %v", tok.Scopes) + } +} diff --git a/internal/oauth/lock.go b/internal/oauth/lock.go index d1dd344d2..0d192a2ed 100644 --- a/internal/oauth/lock.go +++ b/internal/oauth/lock.go @@ -23,6 +23,12 @@ var lockSeq atomic.Uint64 // file is older than fileLockStaleAfter (so a crashed holder cannot deadlock the // store). Release is ownership-aware: it removes the lock only if it still holds // our token, so a stale-broken holder cannot delete a newer holder's lock. +// +// The acquisition deadline is always measured against the real wall clock, never +// the now parameter: now is StoreOptions.Now, which callers may legitimately fix +// (e.g. a test or an embedded clock), and deadline := now().Add(fileLockTimeout) +// followed by now().After(deadline) would then never become true, turning lock +// contention into an infinite retry loop instead of a timeout error. func acquireFileLock(lockPath string, now func() time.Time) (func(), error) { if now == nil { now = time.Now @@ -31,7 +37,7 @@ func acquireFileLock(lockPath string, now func() time.Time) (func(), error) { return nil, err } token := fmt.Sprintf("%d-%d-%d", os.Getpid(), now().UnixNano(), lockSeq.Add(1)) - deadline := now().Add(fileLockTimeout) + deadline := time.Now().Add(fileLockTimeout) for { f, err := os.OpenFile(lockPath, os.O_CREATE|os.O_EXCL|os.O_WRONLY, 0o600) if err == nil { @@ -88,7 +94,7 @@ func acquireFileLock(lockPath string, now func() time.Time) (func(), error) { // Lost the reclaim race (or it was actually fresh) — fall through to the // bounded wait rather than hot-spinning on a reclaim that never wins. } - if now().After(deadline) { + if time.Now().After(deadline) { return nil, fmt.Errorf("oauth: timed out acquiring token lock %s", filepath.Base(lockPath)) } time.Sleep(10 * time.Millisecond) diff --git a/internal/oauth/store.go b/internal/oauth/store.go index 951e9616f..c062f56c7 100644 --- a/internal/oauth/store.go +++ b/internal/oauth/store.go @@ -5,6 +5,8 @@ import ( "encoding/json" "errors" "fmt" + "log" + "net/url" "os" "path/filepath" "regexp" @@ -106,10 +108,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, @@ -139,13 +154,9 @@ func ResolveStorePath(env map[string]string) (string, error) { } configHome := strings.TrimSpace(envValue(env, "XDG_CONFIG_HOME")) if configHome == "" { - home := strings.TrimSpace(firstNonEmpty(envValue(env, "HOME"), envValue(env, "USERPROFILE"))) - if home == "" { - var err error - home, err = os.UserHomeDir() - if err != nil { - return "", fmt.Errorf("oauth: resolve user home: %w", err) - } + home, err := resolveHomeDir(env) + if err != nil { + return "", err } configHome = filepath.Join(home, ".config") } else if !filepath.IsAbs(configHome) { @@ -158,6 +169,24 @@ func ResolveStorePath(env map[string]string) (string, error) { return filepath.Join(configHome, "zero", "oauth-tokens.json"), nil } +// resolveHomeDir returns the user's home directory, honoring HOME/USERPROFILE +// hermetically (via env) before falling back to os.UserHomeDir(). Shared by +// ResolveStorePath's config-root fallback and by keyringLockPath, which +// anchors on this same identity so the keyring lock never varies with a +// per-process override like XDG_CACHE_HOME/XDG_CONFIG_HOME/TMPDIR that two +// processes of the same real user commonly set differently (sandboxes, CI, +// per-shell env). +func resolveHomeDir(env map[string]string) (string, error) { + if home := strings.TrimSpace(firstNonEmpty(envValue(env, "HOME"), envValue(env, "USERPROFILE"))); home != "" { + return home, nil + } + home, err := os.UserHomeDir() + if err != nil { + return "", fmt.Errorf("oauth: resolve user home: %w", err) + } + return home, nil +} + // NewStore builds a token store with the configured backend (file by default, // or the OS keyring when Storage/ZERO_OAUTH_STORAGE selects it). func NewStore(options StoreOptions) (*Store, error) { @@ -196,14 +225,20 @@ func NewStore(options StoreOptions) (*Store, error) { } kr = osKeyring } - // Serialize the keyring's read-modify-write across processes with a lock - // file beside where the file backend would live. Best-effort: if no config - // location resolves, fall back to in-process serialization only. - lockPath := "" - 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 + // lockPath serializes this binary's own keyring read-modify-write across + // processes, keyed off the keyring identity itself (service + index + // account) and anchored on the user's home directory (see + // keyringLockPath), never off a per-process cache/temp/config override: + // two processes with different roots but pointed at the SAME OS keyring + // entry (the service/account is fixed per binary, not per config root) + // must still serialize against each other, or they can race a + // read-modify-write on the shared keyring index and silently drop one + // process's token write. legacyLockPath additionally coordinates with a + // still-running pre-PR binary during the supported mixed-version window + // (see legacyKeyringLockPath). + lockPath := keyringLockPath(options.Env, keyringService, keyringIndexAccount) + legacyLockPath := legacyKeyringLockPath(options.Env) + return &Store{blob: keyringBlob{kr: kr, service: keyringService, legacyAccount: keyringLegacyAccount, indexAccount: keyringIndexAccount, lockPath: lockPath, legacyLockPath: legacyLockPath}, now: now}, nil default: return nil, fmt.Errorf("oauth: unknown storage %q (want \"file\", \"encrypted-file\", or \"keyring\")", storage) } @@ -228,6 +263,76 @@ func resolveStoreFilePath(options StoreOptions) (string, error) { return filepath.Clean(filePath), nil } +// keyringLockPath returns the cross-process lock file location for the +// keyring backend's read-modify-write, derived from the keyring identity +// itself (the service/account the index is stored under) and anchored on the +// user's home directory (see resolveHomeDir) rather than os.UserCacheDir() or +// os.TempDir(): those pick XDG_CACHE_HOME/TMPDIR per PROCESS, so two +// processes of the SAME real user with different cache/temp roots (a common +// case: sandboxes, CI, per-shell overrides) computed different lock files, +// both read the same fixed keyring index, and could publish competing +// updates that silently hid one process's token. HOME does not vary this way +// for a given real user. A single shared ${TMPDIR}/zero-oauth-keyring.lockfile +// would also let any other account on a multi-user host pre-create or keep +// refreshing the victim's lock and time out their Load/Status/Save/Delete, +// even though each user has a separate OS keychain, so only when even the +// home directory can't be resolved does this fall back to a temp file scoped +// by uid so two different users never collide on one path. +func keyringLockPath(env map[string]string, service, account string) string { + name := keyringLockFileName(service, account) + if home, err := resolveHomeDir(env); err == nil && strings.TrimSpace(home) != "" { + return filepath.Join(home, ".cache", "zero", name) + } + return filepath.Join(os.TempDir(), keyringTempLockName(service, account)) +} + +// legacyKeyringLockPath returns the lock file a pre-PR binary acquires around +// its own read-modify-write of the single combined keyring entry, beside +// wherever ResolveStorePath resolves the file-backend location for that +// process's env. A new binary must take this SAME lock (not just its own +// keyringLockPath) around any write that reconciles or deletes the legacy +// entry: an old binary observes no other lock, so only sharing its exact +// lock file stops it from writing a fresh legacy login/refresh in the window +// between this binary's reconciliation read and its legacy-blob delete, +// which would otherwise discard that write permanently. Best-effort: "" +// when the file-backend location can't be resolved at all, matching the +// legacy code's own best-effort fallback (no cross-process lock at all). +func legacyKeyringLockPath(env map[string]string) string { + home, err := resolveHomeDir(nil) + if err != nil { + return "" + } + return filepath.Join(home, ".config", "zero", "oauth-keyring.lockfile") +} + +// keyringLockFileName names the lock file after the keyring identity it +// guards, so distinct (service, account) pairs never share a lock and the +// same pair always resolves to the same lock regardless of caller config. +func keyringLockFileName(service, account string) string { + return fmt.Sprintf("oauth-keyring-%s-%s.lockfile", sanitizeLockComponent(url.QueryEscape(service)), sanitizeLockComponent(url.QueryEscape(account))) +} + +// lockComponentSafe keeps a service/account string safe as one path segment: +// alphanumerics, dot, underscore, and hyphen pass through; anything else +// (a path separator, especially) is replaced so a crafted identity can never +// escape the lock directory. +var lockComponentSafe = regexp.MustCompile(`[^A-Za-z0-9._-]+`) + +func sanitizeLockComponent(s string) string { + return lockComponentSafe.ReplaceAllString(s, "_") +} + +// keyringTempLockName names the last-resort temp lock file, scoping it by uid so +// concurrently running different users do not share one path. os.Getuid returns +// -1 where uids do not apply (Windows), where os.TempDir is already per-user. +func keyringTempLockName(service, account string) string { + name := keyringLockFileName(service, account) + if uid := os.Getuid(); uid >= 0 { + return fmt.Sprintf("zero-%d-%s", uid, name) + } + return "zero-" + name +} + // FilePath returns the resolved token store location (a path for the file // backend, or a "keyring:..." identifier for the keyring backend). func (s *Store) FilePath() string { return s.blob.location() } @@ -256,7 +361,19 @@ func (s *Store) Load(key string) (Token, bool, error) { } s.mu.Lock() defer s.mu.Unlock() - state, err := s.readState() + // Through blob.withReadLock: the keyring backend's read is several + // separate Get calls (index, then each entry), not one atomic snapshot, + // so an unguarded Load could run concurrently with another process's + // Save/Delete mid write and observe a torn state. The file backend's + // withReadLock is a no-op: its writes are atomic renames, so lock-free + // reads keep their crash tolerance (a crashed writer's fresh lock file + // must not block reads of the last complete file). + var state storeFile + err := s.blob.withReadLock(s.now, func() error { + var readErr error + state, readErr = s.readState() + return readErr + }) if err != nil { return Token{}, false, err } @@ -292,7 +409,15 @@ func (s *Store) Delete(key string) (bool, error) { func (s *Store) Status(prefix string) ([]Status, error) { s.mu.Lock() defer s.mu.Unlock() - state, err := s.readState() + // Same reasoning as Load: run the read under blob.withReadLock so the + // keyring's multi-entry read can't observe another process's Save/Delete + // mid write, while file-backend reads stay lock-free. + var state storeFile + err := s.blob.withReadLock(s.now, func() error { + var readErr error + state, readErr = s.readState() + return readErr + }) if err != nil { return nil, err } @@ -386,6 +511,13 @@ type blobStore interface { // (a lock file for the file backend; none for the keyring, which is the // authoritative store and is serialized within the process by Store.mu). withLock(now func() time.Time, fn func() error) error + // withReadLock guards a read-only pass. The file backend's writes are + // atomic renames, so its reads stay lock-free: a crashed writer's fresh + // lock file must not turn into ~30s of read failures when the last + // complete file is perfectly readable. The keyring backend's read is + // several separate Get calls (index, then each entry), not one atomic + // snapshot, so it takes the same cross-process lock as its writes. + withReadLock(now func() time.Time, fn func() error) error // location is a human-readable identifier for diagnostics/errors. location() string } @@ -429,21 +561,122 @@ func (b fileBlob) withLock(now func() time.Time, fn func() error) error { return fn() } +// withReadLock is deliberately lock-free: write() replaces the file with an +// atomic rename, so a reader always sees a complete file, and a crashed +// writer's leftover lock file must not turn readable state into ~30 seconds +// of Load/Status failures while the stale threshold runs out. +func (b fileBlob) withReadLock(now func() time.Time, fn func() error) error { + return fn() +} + 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 + // legacyLockPath, when set, is the lock file a pre-PR binary acquires around + // its own read-modify-write of the legacy combined entry (see + // legacyKeyringLockPath). write() holds it too, so a live old binary can't + // write a fresh legacy credential in the window between this write's legacy + // reconciliation and its legacy-blob delete. + legacyLockPath string } func (b keyringBlob) read() ([]byte, bool, error) { - enc, ok, err := b.kr.Get(b.service, b.account) + keys, ok, _, err := b.readKeyIndex() + if err != nil { + return nil, false, err + } + if !ok { + return b.readLegacy() + } + // The legacy combined entry is consulted lazily (below) only when an indexed + // key's own entry is missing. write() publishes the index before the per-key + // entries and deletes the legacy blob only after every entry is written, so a + // crash partway through the initial legacy->indexed migration can leave a + // pre-existing credential readable solely in the still-present legacy blob. + // In steady state (all entries present) the legacy blob is never read. + var legacyTokens map[string]Token + legacyLoaded := false + 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 { + // The index lists this key but its own entry is missing. Recover it + // from the legacy blob when a migration is still in flight; otherwise + // (a steady-state index/entry desync whose legacy blob is already + // gone) skip rather than fail the whole read, since the next + // Save/Delete will reconcile the index. + if !legacyLoaded { + // A best-effort recovery source for a read: a transient failure + // here must not fail the whole Load/Status, only skip recovering + // this particular desynced key. The next Save/Delete will + // reconcile it once the legacy blob is legible again (and, + // unlike here, will refuse to delete the legacy blob until it + // can actually read it — see write()). + if lt, lerr := b.readLegacyTokens(); lerr == nil { + legacyTokens = lt + } + legacyLoaded = true + } + if token, has := legacyTokens[key]; has { + tokens[key] = token + } + 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 + } + + if !legacyLoaded { + if lt, lerr := b.readLegacyTokens(); lerr == nil { + legacyTokens = lt + } + } + for key, legacyToken := range legacyTokens { + if ValidateKey(key) != nil { + continue + } + if _, exists := tokens[key]; !exists { + tokens[key] = legacyToken + } + } + + 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 } @@ -454,26 +687,462 @@ func (b keyringBlob) read() ([]byte, bool, error) { return data, true, nil } -func (b keyringBlob) write(data []byte) error { - return b.kr.Set(b.service, b.account, base64.StdEncoding.EncodeToString(data)) +// readLegacyTokens returns the tokens held in the legacy combined entry. A +// nil map with a nil error means the entry genuinely does not exist (readLegacy +// returned ok=false, err=nil) — the one case callers may treat as "no tokens" +// and proceed. Any other failure (a transient keyring read error, undecodable +// base64, invalid JSON) is returned as err and must NOT be collapsed into "no +// tokens": write() deletes the legacy blob once it believes reconciliation is +// complete, so mistaking a transient read failure for an empty blob would +// delete a still-unread, still-live credential that belongs to an older, +// still-installed zero binary. +func (b keyringBlob) readLegacyTokens() (map[string]Token, error) { + data, ok, err := b.readLegacy() + if err != nil { + return nil, err + } + if !ok { + return nil, nil + } + var legacyState storeFile + if err := json.Unmarshal(data, &legacyState); err != nil { + return nil, fmt.Errorf("oauth: invalid legacy keyring token blob: %w", err) + } + return legacyState.Tokens, nil } -// withLock serializes the keyring's read-modify-write. Store.mu covers the -// in-process case; lockPath (when set) adds cross-process exclusion so two -// processes can't both read the blob, modify, and write — dropping a token. -func (b keyringBlob) withLock(now func() time.Time, fn func() error) error { - if b.lockPath == "" { - return fn() +// legacyIsFresher reports whether the legacy copy of an already-indexed key +// should win over the indexed copy. An old binary running alongside the new one +// refreshes tokens only in the legacy combined entry, and a refresh pushes the +// expiry later, so a strictly later expiry on the legacy side is the +// signal that it holds a newer credential. Zero (unknown) expiries are valid. +func legacyIsFresher(legacy, current Token) bool { + if legacy.ExpiresAt.After(current.ExpiresAt) { + return true + } + if legacy.ExpiresAt.Equal(current.ExpiresAt) { + return legacy.AccessToken != current.AccessToken || legacy.RefreshToken != current.RefreshToken + } + return false +} + +// write replaces the keyring's token entries with state, ordered so that +// every interruption boundary leaves a recoverable store. The invariant is +// that any token entry existing in the keyring at any instant is listed in +// the published index: the union index is published before entries are +// written, entries are deleted before the index shrinks, and the index +// header is only updated after the chunks it references exist. A crash at +// any step therefore leaves either an index over-listing keys whose entries +// are missing (read() recovers those from the legacy blob during a migration, +// or skips them once it is gone) or entries that a later read/write can still +// see and reconcile, never an invisible credential stranded in the OS keychain. +// The legacy combined entry is the durable fallback for the initial migration +// and is deleted only after every per-key entry is written, while the union +// index still lists removed keys; a failure of that delete is returned so a +// logout is never reported successful with the stale blob still resident. +func (b keyringBlob) write(data []byte) error { + var state storeFile + if err := json.Unmarshal(data, &state); err != nil { + return fmt.Errorf("oauth: encode keyring token blob: %w", err) } - unlock, err := acquireFileLock(b.lockPath, now) + priorKeys, indexExisted, priorChunks, err := b.readKeyIndex() if err != nil { return err } - defer unlock() - return fn() + prior := make(map[string]bool, len(priorKeys)) + for _, key := range priorKeys { + prior[key] = true + } + + // An older binary running alongside this one still reads and writes only the + // legacy combined entry. If that entry exists even though the index has + // already been published, an old binary wrote it after migration, so + // reconcile it into state before it is deleted below rather than blindly + // overwriting it: + // - a key the indexed schema has never seen is a fresh old-binary login; + // merge it so it is not lost; + // - a key already present in state that the legacy blob refreshed (a + // strictly later expiry) takes the legacy value, so a concurrent + // old-binary refresh is not discarded in favor of the stale indexed one; + // - a key that was in the prior index but is absent from this write was + // deliberately removed (a logout); it is left removed, not resurrected. + if indexExisted { + // Unlike read()'s best-effort fallback, a failure here must abort the + // whole write rather than proceed as though the legacy blob were empty: + // step 4 below deletes it, and a transient read error (the legacy blob + // genuinely exists but couldn't be read right now) must never be + // mistaken for "nothing to reconcile," or a still-live credential from + // an older, still-installed zero binary is destroyed irrecoverably. + legacyTokens, err := b.readLegacyTokens() + if err != nil { + return fmt.Errorf("oauth: read legacy keyring token blob for reconciliation: %w", err) + } + for key, legacyToken := range legacyTokens { + if ValidateKey(key) != nil { + continue + } + if current, exists := state.Tokens[key]; exists { + if legacyIsFresher(legacyToken, current) { + state.Tokens[key] = legacyToken + } + continue + } + if prior[key] { + continue + } + state.Tokens[key] = legacyToken + } + } + + keys := make([]string, 0, len(state.Tokens)) + for key := range state.Tokens { + keys = append(keys, key) + } + sort.Strings(keys) + + // 1. Publish the union of the prior and new key sets first, so every + // entry that exists at any point during this update is indexed. + union := keys + if len(priorKeys) > 0 { + merged := make(map[string]bool, len(keys)+len(priorKeys)) + for _, key := range append(append([]string{}, keys...), priorKeys...) { + merged[key] = true + } + union = make([]string, 0, len(merged)) + for key := range merged { + union = append(union, key) + } + sort.Strings(union) + } + unionChunks, err := b.writeKeyIndex(union, priorChunks) + if err != nil { + return err + } + // 2. Write each token entry. + for _, key := range keys { + raw, err := json.Marshal(state.Tokens[key]) + if err != nil { + return err + } + if err := b.kr.Set(b.service, key, base64.StdEncoding.EncodeToString(raw)); err != nil { + return err + } + } + // 3. Delete removed entries while the union index still lists them, so a + // failed Delete leaves a visible (re-deletable) entry, never an orphan. + for _, key := range priorKeys { + if _, ok := state.Tokens[key]; !ok { + if _, err := b.kr.Delete(b.service, key); err != nil { + return err + } + } + } + // 4. Drop the legacy entry: the index now exists and is authoritative, + // and its fresh writes were merged above. This must happen while the + // union index still lists any removed keys and its failure must surface: + // if a stale legacy blob survived a logout whose index shrink already + // completed, the next save would classify its keys as fresh old-binary + // logins and silently resurrect the logged-out credential. + if _, err := b.kr.Delete(b.service, b.legacyAccount); err != nil { + return err + } + // 5. Shrink the index to the exact new key set. + if _, err := b.writeKeyIndex(keys, unionChunks); err != nil { + return err + } + return nil +} + +// maxKeyringIndexChunkBytes bounds one index chunk's raw JSON payload so its +// base64 encoding plus command framing stays well under the macOS +// `security -i` 4095-byte line cap (see internal/keyring): 2700 raw bytes +// expand to 3600 base64 bytes, leaving ~490 bytes for the add-generic-password +// syntax, service, and account. The old single-entry index hit that cap at +// roughly 22 maximum-length keys even when every token was tiny. +const maxKeyringIndexChunkBytes = 2700 + +// maxKeyringIndexChunks caps how many chunk entries a stored index header may +// claim before readKeyIndex issues one OS-keyring lookup per chunk. Each chunk +// holds up to maxKeyringIndexChunkBytes of keys (dozens to ~150 keys), so this +// bound admits far more logins than any real install while refusing to fan a +// corrupt header (e.g. {"v":1,"chunks":1000000000}) out into a billion blocking +// lookups that would wedge every OAuth operation under the store lock. +const maxKeyringIndexChunks = 128 + +// maxKeyringIndexKeys bounds how many keys readKeyIndex will ever return, across +// the header and every chunk (and the legacy bare-array format), before read() +// and write() fan them out into one kr.Get per key while holding the store +// lock. maxKeyringIndexChunks only bounds the number of chunk entries fetched; +// it does not bound how many keys a single chunk's JSON can claim, so a +// corrupted index with an oversized keys array (or many chunks each stuffed +// with keys) could still drive an unbounded number of blocking lookups. The +// bound here is generous relative to what chunkIndexKeys ever legitimately +// produces (short namespaced keys cost at least ~18 bytes each, so one +// maxKeyringIndexChunkBytes chunk holds on the order of a hundred, times +// maxKeyringIndexChunks) while still rejecting a damaged index promptly. +const maxKeyringIndexKeys = 512 + +// errKeyringIndexTooManyKeys is returned when a decoded index (or one of its +// chunks) claims more keys than maxKeyringIndexKeys. +func errKeyringIndexTooManyKeys(count int) error { + log.Printf("warning: oauth: keyring token index lists %d keys, over the %d-key cap", count, maxKeyringIndexKeys) + return fmt.Errorf("oauth: keyring token index lists %d keys, over the %d-key cap", count, maxKeyringIndexKeys) +} + +// keyIndexHeader is chunk 0 of the key index. Chunks 1..Chunks-1 live under +// "-" as plain JSON string arrays. The pre-chunking format +// (a bare JSON array at indexAccount) is still read transparently. +type keyIndexHeader struct { + Version int `json:"v"` + Chunks int `json:"chunks"` + Keys []string `json:"keys"` +} + +func (b keyringBlob) chunkAccount(index int) string { + return fmt.Sprintf("%s-%d", b.indexAccount, index) +} + +// readKeyIndex returns the indexed keys, whether an index exists at all, and +// how many chunk entries it currently occupies. A chunk listed by the header +// but missing from the keyring (a torn write) is skipped, mirroring how +// read() skips an indexed key whose entry is missing. +func (b keyringBlob) readKeyIndex() ([]string, bool, int, error) { + enc, ok, err := b.kr.Get(b.service, b.indexAccount) + if err != nil { + return nil, false, 0, err + } + if !ok { + return nil, false, 0, nil + } + raw, err := base64.StdEncoding.DecodeString(strings.TrimSpace(enc)) + if err != nil { + return nil, false, 0, fmt.Errorf("oauth: decode keyring token index: %w", err) + } + trimmed := strings.TrimSpace(string(raw)) + if strings.HasPrefix(trimmed, "[") { + var keys []string + if err := json.Unmarshal(raw, &keys); err != nil { + return nil, false, 0, fmt.Errorf("oauth: decode keyring token index: %w", err) + } + if len(keys) > maxKeyringIndexKeys { + return nil, false, 0, errKeyringIndexTooManyKeys(len(keys)) + } + return dedupeValidKeys(keys), true, 1, nil + } + var header keyIndexHeader + if err := json.Unmarshal(raw, &header); err != nil { + return nil, false, 0, fmt.Errorf("oauth: decode keyring token index: %w", err) + } + // Reject an unsupported or corrupt header before looping: an out-of-range + // Chunks would otherwise drive up to that many blocking keyring lookups + // (each up to the 10s command timeout) while the store lock is held, wedging + // every Load/Status/Save/Delete instead of failing promptly. + if header.Version != 1 { + return nil, false, 0, fmt.Errorf("oauth: unsupported keyring token index version %d", header.Version) + } + if header.Chunks < 1 || header.Chunks > maxKeyringIndexChunks { + return nil, false, 0, fmt.Errorf("oauth: keyring token index advertises %d chunks (want 1..%d)", header.Chunks, maxKeyringIndexChunks) + } + if len(header.Keys) > maxKeyringIndexKeys { + return nil, false, 0, errKeyringIndexTooManyKeys(len(header.Keys)) + } + keys := header.Keys + for i := 1; i < header.Chunks; i++ { + chunkEnc, ok, err := b.kr.Get(b.service, b.chunkAccount(i)) + if err != nil { + return nil, false, 0, err + } + if !ok { + continue + } + chunkRaw, err := base64.StdEncoding.DecodeString(strings.TrimSpace(chunkEnc)) + if err != nil { + return nil, false, 0, fmt.Errorf("oauth: decode keyring token index chunk %d: %w", i, err) + } + var more []string + if err := json.Unmarshal(chunkRaw, &more); err != nil { + return nil, false, 0, fmt.Errorf("oauth: decode keyring token index chunk %d: %w", i, err) + } + if len(keys)+len(more) > maxKeyringIndexKeys { + return nil, false, 0, errKeyringIndexTooManyKeys(len(keys) + len(more)) + } + keys = append(keys, more...) + } + return dedupeValidKeys(keys), true, header.Chunks, nil +} + +// dedupeValidKeys drops duplicates and malformed entries from a decoded +// index's key list before it is fanned out into one keyring lookup per key by +// read()/write() (via Load/Status/Save/Delete). maxKeyringIndexKeys already +// bounds the raw decode, but that bound does nothing against a corrupted or +// adversarially crafted index that packs its budget with repeats of the same +// key (or garbage that was never a real ValidateKey-shaped entry): every +// duplicate or malformed key would otherwise still cost its own blocking +// keyring lookup (up to the 10s command timeout) while the store lock is +// held, reintroducing the fan-out DoS the index cap was meant to close. +// Order is preserved (first occurrence wins) so callers that sort or display +// keys see stable results. +func dedupeValidKeys(keys []string) []string { + seen := make(map[string]bool, len(keys)) + out := make([]string, 0, len(keys)) + for _, key := range keys { + if seen[key] { + continue + } + if ValidateKey(key) != nil { + continue + } + seen[key] = true + out = append(out, key) + } + return out +} + +// writeKeyIndex persists keys as a chunked index and reports how many chunk +// entries it used. Continuation chunks are written before the header that +// references them, so the authoritative chunk 0 never advertises a chunk that +// does not exist yet; stale chunks from a previously larger index are removed +// only after the header stops referencing them (best-effort: an unreferenced +// chunk is never read). +func (b keyringBlob) writeKeyIndex(keys []string, priorChunks int) (int, error) { + // Refuse to publish an index the reader would reject: readKeyIndex caps both + // total keys and chunk count, and a header beyond either would make every + // later Load/Status/Save/Delete fail before it could recover. Check the key + // count before chunking so a large set of short keys that still fit under + // maxKeyringIndexChunks cannot strand the store unreadable. + if len(keys) > maxKeyringIndexKeys { + return 0, errKeyringIndexTooManyKeys(len(keys)) + } + chunks := chunkIndexKeys(keys) + if len(chunks) > maxKeyringIndexChunks { + return 0, fmt.Errorf("oauth: keyring key index needs %d chunks, over the %d-chunk cap readers accept; too many stored credentials", len(chunks), maxKeyringIndexChunks) + } + for i := 1; i < len(chunks); i++ { + chunkData, err := json.Marshal(chunks[i]) + if err != nil { + return 0, err + } + if err := b.kr.Set(b.service, b.chunkAccount(i), base64.StdEncoding.EncodeToString(chunkData)); err != nil { + return 0, err + } + } + headerData, err := json.Marshal(keyIndexHeader{Version: 1, Chunks: len(chunks), Keys: chunks[0]}) + if err != nil { + return 0, err + } + if err := b.kr.Set(b.service, b.indexAccount, base64.StdEncoding.EncodeToString(headerData)); err != nil { + return 0, err + } + for i := len(chunks); i < priorChunks; i++ { + _, _ = b.kr.Delete(b.service, b.chunkAccount(i)) + } + return len(chunks), nil +} + +// chunkIndexKeys packs keys into chunks whose marshaled JSON stays under +// maxKeyringIndexChunkBytes. Always returns at least one (possibly empty) +// chunk. +func chunkIndexKeys(keys []string) [][]string { + chunks := [][]string{{}} + size := 0 + for _, key := range keys { + // Per-key JSON cost: quotes, comma, and headroom for escaping. + cost := len(key) + 8 + if size+cost > maxKeyringIndexChunkBytes && len(chunks[len(chunks)-1]) > 0 { + chunks = append(chunks, []string{}) + size = 0 + } + chunks[len(chunks)-1] = append(chunks[len(chunks)-1], key) + size += cost + } + return chunks +} + +// fileLockRefreshInterval is how often a held keyring lock's mtime is +// refreshed while its critical section runs. It must stay comfortably under +// fileLockStaleAfter (30s): one external keyring command may legitimately +// take up to its 10s timeout and a multi-entry pass runs several, so without +// refreshing, a healthy slow holder would look stale and another process +// could reclaim the live lock and resume the token-loss race the lock +// exists to prevent. A var so tests can shorten it. +var fileLockRefreshInterval = 10 * time.Second + +// withLeasedLocks acquires every non-empty path in order, refreshes all of +// their mtimes on a ticker while fn runs (so a legitimately slow multi-entry +// keyring pass never looks like a crashed holder to another process), and +// releases them in reverse order once fn returns. +func withLeasedLocks(paths []string, now func() time.Time, fn func() error) error { + var unlocks []func() + var held []string + for _, p := range paths { + if p == "" { + continue + } + unlock, err := acquireFileLock(p, now) + if err != nil { + for i := len(unlocks) - 1; i >= 0; i-- { + unlocks[i]() + } + return err + } + unlocks = append(unlocks, unlock) + held = append(held, p) + } + if len(held) == 0 { + return fn() + } + stop := make(chan struct{}) + done := make(chan struct{}) + go func() { + defer close(done) + ticker := time.NewTicker(fileLockRefreshInterval) + defer ticker.Stop() + for { + select { + case <-stop: + return + case <-ticker.C: + // Lease with wall-clock time, never the injectable now: acquireFileLock + // judges staleness with real time.Since(mtime), so a fixed or stale + // StoreOptions.Now would stamp a live lock with an old mtime that + // another process would immediately reclaim, reviving the token-loss + // race these locks prevent. + at := time.Now() + for _, p := range held { + _ = os.Chtimes(p, at, at) + } + } + } + }() + err := fn() + close(stop) + <-done + for i := len(unlocks) - 1; i >= 0; i-- { + unlocks[i]() + } + return err +} + +// withLock serializes the keyring's read-modify-write. Store.mu covers the +// in-process case; lockPath adds cross-process exclusion between this +// binary's own instances so two of them can't both read the blob, modify, +// and write — dropping a token. legacyLockPath is held for the same +// duration so a live pre-PR binary, which only ever locks there (see +// legacyKeyringLockPath), can't write a fresh legacy credential in the +// window between this write's legacy reconciliation and its legacy-blob +// delete. +func (b keyringBlob) withLock(now func() time.Time, fn func() error) error { + return withLeasedLocks([]string{b.lockPath, b.legacyLockPath}, now, fn) +} + +// withReadLock only takes lockPath: a pre-PR binary never locks for a read +// (see legacyKeyringLockPath), so a read here has nothing to coordinate with +// on the legacy side. +func (b keyringBlob) withReadLock(now func() time.Time, fn func() error) error { + return withLeasedLocks([]string{b.lockPath}, now, 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..5cba1981d 100644 --- a/internal/oauth/store_keyring_test.go +++ b/internal/oauth/store_keyring_test.go @@ -1,8 +1,14 @@ package oauth import ( + "encoding/base64" + "encoding/json" + "fmt" + "os" + "path/filepath" "strings" "testing" + "time" ) // fakeKR is an in-memory KeyringClient for exercising the keyring backend @@ -49,13 +55,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 +75,427 @@ 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) + } + } +} + +// TestStoreKeyringSkipsIndexedKeyMissingItsEntry covers read()'s recovery from +// an index/entry desync: a key listed in the index whose own entry is +// missing (e.g. a process killed between writing the entry and updating the +// index, or between updating the index and deleting a removed entry). read() +// must skip that key rather than fail the whole read, since the next +// Save/Delete reconciles the index against what's actually there. +func TestStoreKeyringSkipsIndexedKeyMissingItsEntry(t *testing.T) { + t.Setenv("XDG_CONFIG_HOME", t.TempDir()) + kr := newFakeKR() + + present := Token{AccessToken: "present-a", RefreshToken: "present-r"} + raw, err := json.Marshal(present) + if err != nil { + t.Fatal(err) + } + kr.data[keyringService+"/"+ProviderKey("present")] = base64.StdEncoding.EncodeToString(raw) + + // The index references both keys, but "missing"'s own entry was never + // written (or was already deleted) — the desync this test targets. + index, err := json.Marshal([]string{ProviderKey("missing"), ProviderKey("present")}) + if err != nil { + t.Fatal(err) + } + kr.data[keyringService+"/"+keyringIndexAccount] = base64.StdEncoding.EncodeToString(index) + + s, err := NewStore(StoreOptions{Storage: "keyring", Keyring: kr}) + if err != nil { + t.Fatal(err) + } + + if _, ok, err := s.Load(ProviderKey("missing")); err != nil || ok { + t.Fatalf("Load(missing): ok=%v err=%v, want ok=false err=nil", ok, err) + } + got, ok, err := s.Load(ProviderKey("present")) + if err != nil || !ok { + t.Fatalf("Load(present): ok=%v err=%v", ok, err) + } + if got.AccessToken != present.AccessToken { + t.Fatalf("Load(present) = %#v", got) + } + + statuses, err := s.Status("") + if err != nil { + t.Fatalf("Status: %v", err) + } + if len(statuses) != 1 || statuses[0].Key != ProviderKey("present") { + t.Fatalf("Status = %#v, want only the present key", statuses) + } +} + +// failingKR wraps fakeKR and fails the Nth mutating operation (Set/Delete), +// for exercising every interruption boundary of the multi-step write. +type failingKR struct { + *fakeKR + failAt int // 1-based mutating-operation number to fail; 0 disables + ops int +} + +func (f *failingKR) Set(service, account, secret string) error { + f.ops++ + if f.failAt != 0 && f.ops == f.failAt { + return errKRInjected + } + return f.fakeKR.Set(service, account, secret) +} + +func (f *failingKR) Delete(service, account string) (bool, error) { + f.ops++ + if f.failAt != 0 && f.ops == f.failAt { + return false, errKRInjected + } + return f.fakeKR.Delete(service, account) +} + +var errKRInjected = errKR("injected keyring failure") + +type errKR string + +func (e errKR) Error() string { return string(e) } + +// indexedKeysOf parses the (possibly chunked) index in kr and returns every +// listed key. +func indexedKeysOf(t *testing.T, kr *fakeKR) map[string]bool { + t.Helper() + blob := keyringBlob{kr: kr, service: keyringService, legacyAccount: keyringLegacyAccount, indexAccount: keyringIndexAccount} + keys, _, _, err := blob.readKeyIndex() + if err != nil { + t.Fatalf("readKeyIndex: %v", err) + } + out := make(map[string]bool, len(keys)) + for _, k := range keys { + out[k] = true + } + return out +} + +// TestStoreKeyringIndexStaysUnderEntryLimit is the regression test for the +// index itself hitting the same macOS `security -i` line cap the per-token +// split fixed for token entries: with enough maximum-length keys, a single +// index entry base64-expands past 4095 bytes even when every token is tiny. +// The index must therefore be bounded per entry (chunked) like everything +// else, and still round-trip. +func TestStoreKeyringIndexStaysUnderEntryLimit(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) + } + // 40 keys near ValidateKey's cap: an unchunked index of these would + // serialize to ~5.5KB before base64. + names := make([]string, 0, 40) + for i := 0; i < 40; i++ { + names = append(names, strings.Repeat("p", 100)+"-"+strings.Repeat("0123456789", 2)+string(rune('a'+i%26))+string(rune('a'+i/26))) + } + for _, name := range names { + if err := s.Save(ProviderKey(name), Token{AccessToken: "a"}); err != nil { + t.Fatalf("Save(%s): %v", name, err) + } + } + // Every keyring value, index entries included, must stay under the cap + // with generous framing margin. + const entryCeiling = 3800 + for k, v := range kr.data { + if len(v) > entryCeiling { + t.Fatalf("keyring entry %q is %d bytes, want <= %d (index cap regression)", k, len(v), entryCeiling) + } + } + // The index actually chunked (otherwise the ceiling check proves nothing). + if _, ok := kr.data[keyringService+"/"+keyringIndexAccount+"-1"]; !ok { + t.Fatal("expected the index to split into continuation chunks") + } + for _, name := range names { + if _, ok, err := s.Load(ProviderKey(name)); err != nil || !ok { + t.Fatalf("Load(%s): ok=%v err=%v", name, ok, err) + } + } + // Shrinking back to one token must also shrink the index and drop the + // stale continuation chunks. + for _, name := range names[1:] { + if _, err := s.Delete(ProviderKey(name)); err != nil { + t.Fatalf("Delete(%s): %v", name, err) + } + } + if _, ok := kr.data[keyringService+"/"+keyringIndexAccount+"-1"]; ok { + t.Fatal("stale index continuation chunk left behind after shrink") + } +} + +// TestStoreKeyringWriteInterruptionsLeaveNoInvisibleTokens drives a write +// through an injected failure at every mutating operation in turn and checks +// the recoverable-store invariant at each boundary: every token entry present +// in the keyring is listed in the published index (so no credential is ever +// stranded invisibly), and a subsequent unimpeded write fully reconciles. +func TestStoreKeyringWriteInterruptionsLeaveNoInvisibleTokens(t *testing.T) { + t.Setenv("XDG_CONFIG_HOME", t.TempDir()) + for failAt := 1; ; failAt++ { + kr := &failingKR{fakeKR: newFakeKR()} + s, err := NewStore(StoreOptions{Storage: "keyring", Keyring: kr}) + if err != nil { + t.Fatal(err) + } + // Seed two tokens cleanly, then fail the Nth mutating operation of a + // write that both adds a token and (via the later delete pass of a + // Delete call) removes one. + if err := s.Save(ProviderKey("alpha"), Token{AccessToken: "a"}); err != nil { + t.Fatal(err) + } + if err := s.Save(ProviderKey("beta"), Token{AccessToken: "b"}); err != nil { + t.Fatal(err) + } + kr.ops = 0 + kr.failAt = failAt + saveErr := s.Save(ProviderKey("gamma"), Token{AccessToken: "c"}) + opsUsed := kr.ops + kr.failAt = 0 + + // Invariant at the interruption boundary: nothing invisible. + indexed := indexedKeysOf(t, kr.fakeKR) + for entry := range kr.data { + account := strings.TrimPrefix(entry, keyringService+"/") + if account == keyringIndexAccount || strings.HasPrefix(account, keyringIndexAccount+"-") || account == keyringLegacyAccount { + continue + } + if !indexed[account] { + t.Fatalf("failAt=%d: token entry %q exists but is not listed in the index (invisible credential)", failAt, account) + } + } + + // The tokens this write didn't touch must stay readable at the + // interruption boundary itself, not just after a later reconciling + // write papers over an incorrect intermediate state. + for _, name := range []string{"alpha", "beta"} { + if _, ok, err := s.Load(ProviderKey(name)); err != nil || !ok { + t.Fatalf("failAt=%d: Load(%s) before reconcile: ok=%v err=%v", failAt, name, ok, err) + } + } + + // A later unimpeded write must reconcile completely. + if err := s.Save(ProviderKey("gamma"), Token{AccessToken: "c"}); err != nil { + t.Fatalf("failAt=%d: reconciling Save: %v", failAt, err) + } + for _, name := range []string{"alpha", "beta", "gamma"} { + if _, ok, err := s.Load(ProviderKey(name)); err != nil || !ok { + t.Fatalf("failAt=%d: Load(%s) after reconcile: ok=%v err=%v", failAt, name, ok, err) + } + } + // Every mutating boundary of the write path now surfaces its failure, + // including the legacy-entry delete (a swallowed failure there could + // let a later save resurrect logged-out credentials). + if opsUsed >= failAt && saveErr == nil { + t.Fatalf("failAt=%d: injected keyring failure was swallowed", failAt) + } + if opsUsed < failAt { + // The write used fewer mutating ops than failAt, so the injection + // never fired and every boundary has been covered. + break + } + } +} + +// TestStoreKeyringDeleteInterruptionsLeaveNoInvisibleTokens is the Delete +// counterpart: a logout interrupted at any boundary must not leave a +// logged-out credential invisibly resident in the OS keychain (the index is +// only shrunk after the entry deletion), and a repeated delete reconciles. +func TestStoreKeyringDeleteInterruptionsLeaveNoInvisibleTokens(t *testing.T) { + t.Setenv("XDG_CONFIG_HOME", t.TempDir()) + for failAt := 1; ; failAt++ { + kr := &failingKR{fakeKR: newFakeKR()} + s, err := NewStore(StoreOptions{Storage: "keyring", Keyring: kr}) + if err != nil { + t.Fatal(err) + } + if err := s.Save(ProviderKey("alpha"), Token{AccessToken: "a"}); err != nil { + t.Fatal(err) + } + if err := s.Save(ProviderKey("beta"), Token{AccessToken: "b"}); err != nil { + t.Fatal(err) + } + kr.ops = 0 + kr.failAt = failAt + _, deleteErr := s.Delete(ProviderKey("beta")) + opsUsed := kr.ops + kr.failAt = 0 + + indexed := indexedKeysOf(t, kr.fakeKR) + for entry := range kr.data { + account := strings.TrimPrefix(entry, keyringService+"/") + if account == keyringIndexAccount || strings.HasPrefix(account, keyringIndexAccount+"-") || account == keyringLegacyAccount { + continue + } + if !indexed[account] { + t.Fatalf("failAt=%d: token entry %q exists but is not listed in the index (invisible credential)", failAt, account) + } + } + + // Retrying the delete must fully reconcile: beta gone from both the + // index and the keyring, alpha intact. + if _, err := s.Delete(ProviderKey("beta")); err != nil { + t.Fatalf("failAt=%d: reconciling Delete: %v", failAt, err) + } + if _, ok := kr.data[keyringService+"/"+ProviderKey("beta")]; ok { + t.Fatalf("failAt=%d: logged-out credential still resident after reconcile", failAt) + } + if _, ok, err := s.Load(ProviderKey("alpha")); err != nil || !ok { + t.Fatalf("failAt=%d: Load(alpha): ok=%v err=%v", failAt, ok, err) + } + // Every mutating boundary of the delete path must surface its failure, + // mirroring the Save-interruption assertion: a swallowed error here + // would let a caller believe a logout succeeded when it didn't. + if opsUsed >= failAt && deleteErr == nil { + t.Fatalf("failAt=%d: injected keyring failure was swallowed", failAt) + } + if opsUsed < failAt { + break + } + } +} + +// TestStoreKeyringMergesFreshLegacyWriteFromOldBinary covers the mixed-version +// window: after migration to the indexed format, an old binary still running +// can save a token into the legacy combined entry. The next new-binary write +// must merge that fresh token instead of deleting the legacy entry over it. +func TestStoreKeyringMergesFreshLegacyWriteFromOldBinary(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) + } + if err := s.Save(ProviderKey("alpha"), Token{AccessToken: "a"}); err != nil { + t.Fatal(err) + } + + // An old binary saves token "carol" through the legacy combined entry. + legacy := storeFile{SchemaVersion: storeSchemaVersion, Tokens: map[string]Token{ + ProviderKey("carol"): {AccessToken: "c", RefreshToken: "cr"}, + }} + data, err := json.Marshal(legacy) + if err != nil { + t.Fatal(err) + } + kr.data[keyringService+"/"+keyringLegacyAccount] = base64.StdEncoding.EncodeToString(data) + + // The next new-binary save must keep carol, not silently lose it. + if err := s.Save(ProviderKey("beta"), Token{AccessToken: "b"}); err != nil { + t.Fatal(err) + } + for _, name := range []string{"alpha", "beta", "carol"} { + if _, ok, err := s.Load(ProviderKey(name)); err != nil || !ok { + t.Fatalf("Load(%s): ok=%v err=%v (fresh legacy write lost)", name, ok, err) + } + } + // Presence alone doesn't rule out the merge corrupting carol's credential + // material; check the actual values survived the legacy->indexed merge. + if got, _, err := s.Load(ProviderKey("carol")); err != nil || got.AccessToken != "c" || got.RefreshToken != "cr" { + t.Fatalf("Load(carol) = %#v, err=%v, want the legacy access/refresh tokens intact", got, err) + } + if _, ok := kr.data[keyringService+"/"+keyringLegacyAccount]; ok { + t.Fatal("legacy entry should be removed once its fresh writes are merged") + } } func TestNewStoreStorageSelection(t *testing.T) { @@ -93,6 +524,81 @@ func TestNewStoreStorageSelection(t *testing.T) { } } +// TestStoreKeyringWithLockRefreshesLease guards the stale-reclaim race: one +// keyring command can take up to 10s and a multi-entry pass runs several, so +// a lock held for a legitimately slow operation can outlive the fixed 30s +// stale threshold. withLock must keep the lock file's mtime fresh while its +// critical section runs, so only a genuinely crashed holder ever looks stale. +func TestStoreKeyringWithLockRefreshesLease(t *testing.T) { + lockPath := filepath.Join(t.TempDir(), "oauth-keyring.lockfile") + blob := keyringBlob{kr: newFakeKR(), service: "zero-test", indexAccount: "idx", lockPath: lockPath} + + previous := fileLockRefreshInterval + fileLockRefreshInterval = 20 * time.Millisecond + defer func() { fileLockRefreshInterval = previous }() + + var first, second time.Time + err := blob.withLock(time.Now, func() error { + info, err := os.Stat(lockPath) + if err != nil { + return err + } + first = info.ModTime() + time.Sleep(150 * time.Millisecond) + info, err = os.Stat(lockPath) + if err != nil { + return err + } + second = info.ModTime() + return nil + }) + if err != nil { + t.Fatalf("withLock: %v", err) + } + if !second.After(first) { + t.Fatalf("lock mtime was not refreshed during the critical section: %v then %v", first, second) + } + if _, err := os.Stat(lockPath); !os.IsNotExist(err) { + t.Fatalf("lock file not released: %v", err) + } +} + +// TestStoreFileLoadToleratesCrashedWriterLock: file-backend reads must stay +// lock-free. A writer that crashed after taking the lock leaves a fresh lock +// file behind; the store file itself is always complete (writes are atomic +// renames), so Load must read it rather than waiting out the lock and +// failing for the ~30 seconds the stale threshold takes to expire. +func TestStoreFileLoadToleratesCrashedWriterLock(t *testing.T) { + path := filepath.Join(t.TempDir(), "oauth-tokens.json") + s, err := NewStore(StoreOptions{FilePath: path}) + if err != nil { + t.Fatal(err) + } + if err := s.Save(ProviderKey("demo"), Token{AccessToken: "a"}); err != nil { + t.Fatal(err) + } + // Simulate the crashed writer: a fresh, never-released lock file. + if err := os.WriteFile(path+".lockfile", []byte("someone-else"), 0o600); err != nil { + t.Fatal(err) + } + start := time.Now() + got, ok, err := s.Load(ProviderKey("demo")) + if err != nil || !ok || got.AccessToken != "a" { + t.Fatalf("Load behind a crashed writer's lock: ok=%v err=%v token=%#v", ok, err, got) + } + if elapsed := time.Since(start); elapsed > 2*time.Second { + t.Fatalf("Load waited on the write lock (%v); reads must be lock-free", elapsed) + } + statusStart := time.Now() + statuses, err := s.Status("") + if err != nil || len(statuses) != 1 { + t.Fatalf("Status behind a crashed writer's lock: %v (%d entries)", err, len(statuses)) + } + if elapsed := time.Since(statusStart); elapsed > 2*time.Second { + t.Fatalf("Status waited on the write lock (%v); reads must be lock-free", elapsed) + } +} + func TestStoreKeyringStatus(t *testing.T) { t.Setenv("XDG_CONFIG_HOME", t.TempDir()) kr := newFakeKR() @@ -111,3 +617,737 @@ func TestStoreKeyringStatus(t *testing.T) { t.Fatalf("status = %#v", statuses) } } + +// TestStoreKeyringMigrationInterruptionsPreserveLegacyTokens drives the initial +// legacy->indexed migration through an injected failure at every mutating +// operation and checks that no pre-existing legacy credential is ever lost. +// write() publishes the index before the per-key entries, so a crash after the +// index appears but before an entry is written must still leave that token +// readable in the not-yet-deleted legacy blob; read() recovers it, and a +// following unimpeded save completes the migration. +func TestStoreKeyringMigrationInterruptionsPreserveLegacyTokens(t *testing.T) { + t.Setenv("XDG_CONFIG_HOME", t.TempDir()) + seeded := map[string]Token{ + ProviderKey("demo"): {AccessToken: "demo-a", RefreshToken: "demo-r"}, + ProviderKey("other"): {AccessToken: "other-a"}, + } + for failAt := 1; ; failAt++ { + kr := &failingKR{fakeKR: newFakeKR()} + // A legacy-only install: one combined entry, no index yet. + legacyData, err := json.Marshal(storeFile{SchemaVersion: storeSchemaVersion, Tokens: seeded}) + if err != nil { + t.Fatal(err) + } + kr.data[keyringService+"/"+keyringLegacyAccount] = base64.StdEncoding.EncodeToString(legacyData) + + s, err := NewStore(StoreOptions{Storage: "keyring", Keyring: kr}) + if err != nil { + t.Fatal(err) + } + kr.ops = 0 + kr.failAt = failAt + _ = s.Save(ProviderKey("new"), Token{AccessToken: "new-c"}) + opsUsed := kr.ops + kr.failAt = 0 + + // Regardless of where the migration was interrupted, a subsequent + // unimpeded save must complete it with every token intact. + if err := s.Save(ProviderKey("new"), Token{AccessToken: "new-c"}); err != nil { + t.Fatalf("failAt=%d: reconciling Save: %v", failAt, err) + } + for key, want := range seeded { + got, ok, err := s.Load(key) + if err != nil || !ok { + t.Fatalf("failAt=%d: Load(%s) after migration: ok=%v err=%v (legacy token lost)", failAt, key, ok, err) + } + if got.AccessToken != want.AccessToken { + t.Fatalf("failAt=%d: Load(%s) = %q, want %q", failAt, key, got.AccessToken, want.AccessToken) + } + } + if got, ok, err := s.Load(ProviderKey("new")); err != nil || !ok || got.AccessToken != "new-c" { + t.Fatalf("failAt=%d: Load(new): ok=%v err=%v token=%#v", failAt, ok, err, got) + } + // The completed migration drops the legacy entry. + if _, ok := kr.data[keyringService+"/"+keyringLegacyAccount]; ok { + t.Fatalf("failAt=%d: legacy entry not removed after migration completed", failAt) + } + if opsUsed < failAt { + break + } + } +} + +// TestStoreKeyringMergesFreshLegacyRefreshOfIndexedKey covers the mixed-version +// window for a key that already exists in the index: an old binary refreshes +// provider:alpha in the legacy combined entry (a strictly later expiry). The +// next new-binary save must keep that fresher refresh instead of overwriting it +// with the stale indexed value and then deleting the legacy entry. +func TestStoreKeyringMergesFreshLegacyRefreshOfIndexedKey(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) + } + stale := time.Now().Add(1 * time.Hour) + if err := s.Save(ProviderKey("alpha"), Token{AccessToken: "a-old", RefreshToken: "r-old", ExpiresAt: stale}); err != nil { + t.Fatal(err) + } + + // An old binary refreshes alpha through the legacy combined entry, pushing + // the expiry later than the indexed copy. + fresh := stale.Add(1 * time.Hour) + legacy := storeFile{SchemaVersion: storeSchemaVersion, Tokens: map[string]Token{ + ProviderKey("alpha"): {AccessToken: "a-new", RefreshToken: "r-new", ExpiresAt: fresh}, + }} + legacyData, err := json.Marshal(legacy) + if err != nil { + t.Fatal(err) + } + kr.data[keyringService+"/"+keyringLegacyAccount] = base64.StdEncoding.EncodeToString(legacyData) + + // A new-binary save of an unrelated key must reconcile alpha, not clobber it. + if err := s.Save(ProviderKey("beta"), Token{AccessToken: "b"}); err != nil { + t.Fatal(err) + } + got, ok, err := s.Load(ProviderKey("alpha")) + if err != nil || !ok { + t.Fatalf("Load(alpha): ok=%v err=%v", ok, err) + } + if got.AccessToken != "a-new" || got.RefreshToken != "r-new" || !got.ExpiresAt.Equal(fresh) { + t.Fatalf("Load(alpha) = %#v, want the refreshed legacy value (tokens and expiry) with ExpiresAt=%v", got, fresh) + } + if _, ok, _ := s.Load(ProviderKey("beta")); !ok { + t.Fatal("Load(beta): not stored") + } + if _, ok := kr.data[keyringService+"/"+keyringLegacyAccount]; ok { + t.Fatal("legacy entry should be removed once its refresh is merged") + } +} + +// TestAcquireFileLockDeadlineUsesWallClockNotInjectedClock guards the other +// half of the same hazard as the lease test below: acquireFileLock's own +// acquisition deadline must be measured against the real wall clock, not the +// injectable now parameter. StoreOptions.Now may legitimately be a fixed +// clock (as this test uses), and deadline := now().Add(fileLockTimeout) +// followed by now().After(deadline) would then never become true, so a +// contested lock would retry forever instead of returning a timeout error. +func TestAcquireFileLockDeadlineUsesWallClockNotInjectedClock(t *testing.T) { + lockPath := filepath.Join(t.TempDir(), "test.lockfile") + // A fresh (non-stale) lock held by someone else: acquireFileLock must not + // reclaim it, only wait out fileLockTimeout and report a timeout. + if err := os.WriteFile(lockPath, []byte("someone-else"), 0o600); err != nil { + t.Fatal(err) + } + fixed := time.Date(2000, 1, 1, 0, 0, 0, 0, time.UTC) + start := time.Now() + _, err := acquireFileLock(lockPath, func() time.Time { return fixed }) + if err == nil { + t.Fatal("expected a timeout error acquiring an already-held, non-stale lock") + } + if elapsed := time.Since(start); elapsed > 10*time.Second { + t.Fatalf("acquireFileLock took %v with a fixed clock; the deadline must use the wall clock, not now()", elapsed) + } +} + +// TestStoreKeyringLeaseUsesWallClockNotStoreClock guards the lock lease against +// a fixed or stale StoreOptions.Now. acquireFileLock judges staleness with real +// time.Since(mtime), so the lease must stamp the live lock with wall-clock time; +// leasing with an old injectable clock would let a peer immediately reclaim the +// held lock and re-enter the keyring read-modify-write concurrently. +func TestStoreKeyringLeaseUsesWallClockNotStoreClock(t *testing.T) { + lockPath := filepath.Join(t.TempDir(), "oauth-keyring.lockfile") + blob := keyringBlob{kr: newFakeKR(), service: "zero-test", indexAccount: "idx", lockPath: lockPath} + + previous := fileLockRefreshInterval + fileLockRefreshInterval = 20 * time.Millisecond + defer func() { fileLockRefreshInterval = previous }() + + // A deliberately stale, fixed clock: if the lease used it, the lock mtime + // would land decades in the past and look stale immediately. + fixed := time.Date(2000, 1, 1, 0, 0, 0, 0, time.UTC) + var mtime time.Time + err := blob.withLock(func() time.Time { return fixed }, func() error { + time.Sleep(150 * time.Millisecond) + info, statErr := os.Stat(lockPath) + if statErr != nil { + return statErr + } + mtime = info.ModTime() + return nil + }) + if err != nil { + t.Fatalf("withLock: %v", err) + } + if time.Since(mtime) > fileLockStaleAfter { + t.Fatalf("lease stamped the lock with the store clock (%v); a peer would reclaim the live lock", mtime) + } +} + +// countingKR counts Get calls so a test can prove a corrupt index is rejected +// before it fans out into a keyring lookup per advertised chunk. +type countingKR struct { + *fakeKR + gets int +} + +func (c *countingKR) Get(service, account string) (string, bool, error) { + c.gets++ + return c.fakeKR.Get(service, account) +} + +// TestStoreKeyringReadIndexRejectsCorruptHeader is the regression test for an +// index header whose advertised chunk count is unbounded: readKeyIndex must +// reject an out-of-range or unsupported header up front rather than issue up to +// that many blocking keyring lookups while holding the store lock. +func TestStoreKeyringReadIndexRejectsCorruptHeader(t *testing.T) { + ckr := &countingKR{fakeKR: newFakeKR()} + blob := keyringBlob{kr: ckr, service: keyringService, legacyAccount: keyringLegacyAccount, indexAccount: keyringIndexAccount} + + oversized, err := json.Marshal(keyIndexHeader{Version: 1, Chunks: 1_000_000_000, Keys: []string{ProviderKey("demo")}}) + if err != nil { + t.Fatal(err) + } + ckr.data[keyringService+"/"+keyringIndexAccount] = base64.StdEncoding.EncodeToString(oversized) + ckr.gets = 0 + if _, _, _, err := blob.readKeyIndex(); err == nil { + t.Fatal("expected an oversized chunk count to be rejected") + } + if ckr.gets != 1 { + t.Fatalf("readKeyIndex issued %d keyring gets on a corrupt header; it must reject before fanning out over chunks", ckr.gets) + } + + unsupported, err := json.Marshal(keyIndexHeader{Version: 2, Chunks: 1, Keys: []string{}}) + if err != nil { + t.Fatal(err) + } + ckr.data[keyringService+"/"+keyringIndexAccount] = base64.StdEncoding.EncodeToString(unsupported) + ckr.gets = 0 + if _, _, _, err := blob.readKeyIndex(); err == nil { + t.Fatal("expected an unsupported index version to be rejected") + } + if ckr.gets != 1 { + t.Fatalf("readKeyIndex issued %d gets for an unsupported version; want header lookup only", ckr.gets) + } +} + +// TestStoreKeyringReadIndexRejectsOversizedKeyList regresses a corrupt index +// that claims more keys than maxKeyringIndexKeys: maxKeyringIndexChunks alone +// bounds only how many chunk entries are fetched, not how many keys a single +// chunk's JSON (or the legacy bare-array format) can claim, so without this +// check readKeyIndex would hand read()/write() an oversized key list to fan +// out into one blocking kr.Get per key while holding the store lock. +func TestStoreKeyringReadIndexRejectsOversizedKeyList(t *testing.T) { + ckr := &countingKR{fakeKR: newFakeKR()} + blob := keyringBlob{kr: ckr, service: keyringService, legacyAccount: keyringLegacyAccount, indexAccount: keyringIndexAccount} + + tooMany := make([]string, maxKeyringIndexKeys+1) + for i := range tooMany { + tooMany[i] = ProviderKey(fmt.Sprintf("p%d", i)) + } + + header, err := json.Marshal(keyIndexHeader{Version: 1, Chunks: 1, Keys: tooMany}) + if err != nil { + t.Fatal(err) + } + ckr.data[keyringService+"/"+keyringIndexAccount] = base64.StdEncoding.EncodeToString(header) + ckr.gets = 0 + if _, _, _, err := blob.readKeyIndex(); err == nil { + t.Fatal("expected an oversized key list in a chunk-0 header to be rejected") + } + if ckr.gets != 1 { + t.Fatalf("readKeyIndex issued %d gets for an oversized header keys list; want header lookup only", ckr.gets) + } + + // The pre-chunking bare-array format must be capped the same way. + legacyArray, err := json.Marshal(tooMany) + if err != nil { + t.Fatal(err) + } + ckr.data[keyringService+"/"+keyringIndexAccount] = base64.StdEncoding.EncodeToString(legacyArray) + ckr.gets = 0 + if _, _, _, err := blob.readKeyIndex(); err == nil { + t.Fatal("expected an oversized legacy-format key array to be rejected") + } + if ckr.gets != 1 { + t.Fatalf("readKeyIndex issued %d gets for an oversized legacy array; want header lookup only", ckr.gets) + } + + // Accumulation across continuation chunks must hit the same total cap: a + // small header plus an oversized chunk-1 would otherwise fan out past the + // bound after the per-header check has already passed. + headerOK, err := json.Marshal(keyIndexHeader{Version: 1, Chunks: 2, Keys: []string{ProviderKey("seed")}}) + if err != nil { + t.Fatal(err) + } + chunk1, err := json.Marshal(tooMany) + if err != nil { + t.Fatal(err) + } + ckr.data[keyringService+"/"+keyringIndexAccount] = base64.StdEncoding.EncodeToString(headerOK) + ckr.data[keyringService+"/"+keyringIndexAccount+"-1"] = base64.StdEncoding.EncodeToString(chunk1) + ckr.gets = 0 + if _, _, _, err := blob.readKeyIndex(); err == nil { + t.Fatal("expected an oversized key list accumulated across chunks to be rejected") + } + if ckr.gets != 2 { + t.Fatalf("readKeyIndex issued %d gets for a header+oversize-chunk; want header and chunk-1 only", ckr.gets) + } +} + +// TestKeyringLockPathIsPerUser covers the lock path used for every keyring +// Store regardless of file-backend config. It must not be the single shared +// temp path that any account on a multi-user host could pre-create or hold, +// and the last-resort temp name must be scoped by uid so different users +// never collide on one lock file. +func TestKeyringLockPathIsPerUser(t *testing.T) { + got := keyringLockPath(nil, keyringService, keyringIndexAccount) + name := keyringLockFileName(keyringService, keyringIndexAccount) + if got == filepath.Join(os.TempDir(), "zero-"+name) { + t.Fatalf("lock path is the shared temp path %q; a co-tenant could grief it", got) + } + if home, err := os.UserHomeDir(); err == nil && strings.TrimSpace(home) != "" { + if want := filepath.Join(home, ".cache", "zero", name); got != want { + t.Fatalf("lock path = %q, want per-user home-anchored path %q", got, want) + } + } + tempName := keyringTempLockName(keyringService, keyringIndexAccount) + if uid := os.Getuid(); uid >= 0 { + if !strings.Contains(tempName, fmt.Sprintf("%d", uid)) { + t.Fatalf("temp lock name %q is not scoped by uid %d", tempName, uid) + } + } else if tempName == "" { + t.Fatal("temp lock name is empty") + } +} + +// TestKeyringLockPathIndependentOfCacheAndTempRoots is the regression test +// for [P1] Make the keyring lock path independent of XDG_CACHE_HOME +// (2026-07-22): os.UserCacheDir() (and its os.TempDir() fallback) is chosen +// per PROCESS from XDG_CACHE_HOME/TMPDIR, so two zero processes belonging to +// the SAME real user but with different cache/temp roots (sandboxes, CI, +// per-shell overrides) computed different lock files, both read the shared +// keyring index, and could publish competing updates that hid one process's +// token. The lock must resolve identically regardless of those roots, since +// it is anchored on the user's home directory instead. +func TestKeyringLockPathIndependentOfCacheAndTempRoots(t *testing.T) { + home := t.TempDir() + envA := map[string]string{ + "HOME": home, + "XDG_CACHE_HOME": filepath.Join(t.TempDir(), "cache-a"), + "TMPDIR": filepath.Join(t.TempDir(), "tmp-a"), + } + envB := map[string]string{ + "HOME": home, + "XDG_CACHE_HOME": filepath.Join(t.TempDir(), "cache-b"), + "TMPDIR": filepath.Join(t.TempDir(), "tmp-b"), + } + + storeA, err := NewStore(StoreOptions{Storage: "keyring", Keyring: newFakeKR(), Env: envA}) + if err != nil { + t.Fatal(err) + } + storeB, err := NewStore(StoreOptions{Storage: "keyring", Keyring: newFakeKR(), Env: envB}) + if err != nil { + t.Fatal(err) + } + blobA, okA := storeA.blob.(keyringBlob) + blobB, okB := storeB.blob.(keyringBlob) + if !okA || !okB { + t.Fatal("Store.blob is not keyringBlob") + } + if blobA.lockPath == "" || blobB.lockPath == "" { + t.Fatal("lockPath should never be empty for the keyring backend") + } + if blobA.lockPath != blobB.lockPath { + t.Fatalf("two same-user processes with different cache/temp roots got different lock paths: %q vs %q (they can race the shared keyring index)", blobA.lockPath, blobB.lockPath) + } + if strings.Contains(blobA.lockPath, "cache-a") || strings.Contains(blobA.lockPath, "cache-b") || + strings.Contains(blobA.lockPath, "tmp-a") || strings.Contains(blobA.lockPath, "tmp-b") { + t.Fatalf("lock path %q is still derived from XDG_CACHE_HOME/TMPDIR", blobA.lockPath) + } +} + +// TestStoreKeyringWriteWaitsForLegacyLockDuringReconciliation is the +// regression test for [P1] Coordinate migration with the legacy lock used by +// old binaries (2026-07-22): a pre-PR binary locks beside ResolveStorePath +// around its own read-modify-write of the legacy combined entry, but this +// binary's write() used to lock only under the cache-derived keyring path, so +// the two never excluded each other. A new save could reconcile the legacy +// blob, an old process could then save a fresh legacy credential, and the new +// save would unconditionally delete that blob without ever having observed +// the old write, losing it permanently. +// +// This simulates a live old binary by holding the exact lock file +// legacyKeyringLockPath computes (the same one a pre-PR binary's Save takes) +// and asserting that Store.Save — which must reconcile and then drop the +// legacy entry here, since one is seeded below — blocks until that lock is +// released, and that the seeded legacy token survives the reconciliation. +func TestStoreKeyringWriteWaitsForLegacyLockDuringReconciliation(t *testing.T) { + dir := t.TempDir() + t.Setenv("XDG_CONFIG_HOME", dir) + kr := newFakeKR() + legacy := storeFile{SchemaVersion: storeSchemaVersion, Tokens: map[string]Token{ + ProviderKey("alpha"): {AccessToken: "legacy-alpha"}, + }} + 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) + } + blob, ok := s.blob.(keyringBlob) + if !ok || blob.legacyLockPath == "" { + t.Fatal("keyring store has no legacy-compat lock path") + } + + // Simulate an old binary holding the legacy lock for its own in-flight Save. + unlock, err := acquireFileLock(blob.legacyLockPath, s.now) + if err != nil { + t.Fatalf("acquire simulated legacy lock: %v", err) + } + + saveDone := make(chan error, 1) + go func() { + saveDone <- s.Save(ProviderKey("beta"), Token{AccessToken: "beta"}) + }() + + select { + case err := <-saveDone: + t.Fatalf("Save proceeded (err=%v) while an old binary held the legacy lock; it can race the reconcile-then-delete window and lose a concurrent legacy write", err) + case <-time.After(200 * time.Millisecond): + // Still blocked, as expected. + } + + unlock() + if err := <-saveDone; err != nil { + t.Fatalf("Save after the legacy lock was released: %v", err) + } + + got, ok, err := s.Load(ProviderKey("alpha")) + if err != nil || !ok || got.AccessToken != "legacy-alpha" { + t.Fatalf("legacy alpha token lost across reconciliation: ok=%v err=%v got=%#v", ok, err, got) + } +} + +// TestKeyringLockPathDerivedFromKeyringIdentityNotFileConfig is the +// regression test for the cross-process lock racing bug: the lock guarding +// the shared keyring index must be keyed off the keyring's own identity +// (service + index account), never off the file-backend path config +// (ZERO_OAUTH_TOKENS_PATH / XDG_CONFIG_HOME). Two zero processes with +// different config roots but pointed at the SAME keyring entry (the service +// and account are fixed per binary, not per config root) must resolve to the +// identical lock path, or they can race a read-modify-write on the shared +// index and silently drop one process's token write. +func TestKeyringLockPathDerivedFromKeyringIdentityNotFileConfig(t *testing.T) { + dirA := filepath.Join(t.TempDir(), "config-root-a") + dirB := filepath.Join(t.TempDir(), "config-root-b") + + storeA, err := NewStore(StoreOptions{Storage: "keyring", Keyring: newFakeKR(), Env: map[string]string{"XDG_CONFIG_HOME": dirA}}) + if err != nil { + t.Fatal(err) + } + storeB, err := NewStore(StoreOptions{Storage: "keyring", Keyring: newFakeKR(), Env: map[string]string{"XDG_CONFIG_HOME": dirB}}) + if err != nil { + t.Fatal(err) + } + blobA, okA := storeA.blob.(keyringBlob) + blobB, okB := storeB.blob.(keyringBlob) + if !okA || !okB { + t.Fatal("Store.blob is not keyringBlob") + } + if blobA.lockPath == "" || blobB.lockPath == "" { + t.Fatal("lockPath should never be empty for the keyring backend") + } + if blobA.lockPath != blobB.lockPath { + t.Fatalf("two processes on the SAME keyring entry got different lock paths for different config roots: %q vs %q (they can race the shared keyring index)", blobA.lockPath, blobB.lockPath) + } + // And it must not be derived from either config root's resolved store path. + if strings.Contains(blobA.lockPath, dirA) || strings.Contains(blobA.lockPath, dirB) { + t.Fatalf("lock path %q is still derived from file-backend config, not the keyring identity", blobA.lockPath) + } +} + +// legacyDeleteFailKR fails Delete for the legacy combined entry only, to +// exercise the boundary where a logout has already rewritten the indexed +// state but the stale legacy blob cannot be removed. +type legacyDeleteFailKR struct { + *fakeKR + fail bool +} + +func (f *legacyDeleteFailKR) Delete(service, account string) (bool, error) { + if f.fail && account == keyringLegacyAccount { + return false, errKRInjected + } + return f.fakeKR.Delete(service, account) +} + +// TestStoreKeyringLogoutSurfacesLegacyBlobDeleteFailure: when the final +// legacy-blob delete fails during a logout, the operation must report the +// failure (not success with the secret still resident), and after a clean +// retry the logged-out credential must not be resurrected by a later save. +func TestStoreKeyringLogoutSurfacesLegacyBlobDeleteFailure(t *testing.T) { + t.Setenv("XDG_CONFIG_HOME", t.TempDir()) + kr := &legacyDeleteFailKR{fakeKR: newFakeKR()} + s, err := NewStore(StoreOptions{Storage: "keyring", Keyring: kr}) + if err != nil { + t.Fatal(err) + } + if err := s.Save(ProviderKey("alpha"), Token{AccessToken: "a"}); err != nil { + t.Fatal(err) + } + // A leftover legacy blob still carries alpha (e.g. written by an old + // binary during the upgrade window). + legacy := storeFile{SchemaVersion: storeSchemaVersion, Tokens: map[string]Token{ + ProviderKey("alpha"): {AccessToken: "stale-a"}, + }} + data, err := json.Marshal(legacy) + if err != nil { + t.Fatal(err) + } + kr.data[keyringService+"/"+keyringLegacyAccount] = base64.StdEncoding.EncodeToString(data) + + kr.fail = true + if _, err := s.Delete(ProviderKey("alpha")); err == nil { + t.Fatal("Delete reported success although the stale legacy blob could not be removed") + } + + // A clean retry succeeds, and a later save must not classify the stale + // legacy alpha as a fresh old-binary login. + kr.fail = false + if _, err := s.Delete(ProviderKey("alpha")); err != nil { + t.Fatalf("retried Delete: %v", err) + } + if err := s.Save(ProviderKey("beta"), Token{AccessToken: "b"}); err != nil { + t.Fatal(err) + } + if _, ok, err := s.Load(ProviderKey("alpha")); err != nil || ok { + t.Fatalf("logged-out credential resurrected: ok=%v err=%v", ok, err) + } +} + +// TestStoreKeyringWriteIndexRejectsOverCapChunks: writeKeyIndex must refuse +// to publish an index header that readKeyIndex would reject, instead of +// persisting a store no later operation can open. +func TestStoreKeyringWriteIndexRejectsOverCapChunks(t *testing.T) { + kr := newFakeKR() + b := keyringBlob{kr: kr, service: keyringService, legacyAccount: keyringLegacyAccount, indexAccount: keyringIndexAccount} + // Each key exceeds the per-chunk byte budget on its own, forcing one + // chunk per key. + long := strings.Repeat("k", maxKeyringIndexChunkBytes) + keys := make([]string, maxKeyringIndexChunks+1) + for i := range keys { + keys[i] = fmt.Sprintf("%s-%d", long, i) + } + if _, err := b.writeKeyIndex(keys, 0); err == nil { + t.Fatal("writeKeyIndex published an index readKeyIndex would refuse") + } + if len(kr.data) != 0 { + t.Fatalf("over-cap index write must publish nothing, found %d entries", len(kr.data)) + } +} + +// TestStoreKeyringWriteIndexRejectsOverCapKeys: short keys can still fit under +// the chunk-count cap while exceeding maxKeyringIndexKeys. writeKeyIndex must +// refuse that set before publishing, matching the reader-side total key cap. +func TestStoreKeyringWriteIndexRejectsOverCapKeys(t *testing.T) { + kr := newFakeKR() + b := keyringBlob{kr: kr, service: keyringService, legacyAccount: keyringLegacyAccount, indexAccount: keyringIndexAccount} + keys := make([]string, maxKeyringIndexKeys+1) + for i := range keys { + // Short keys pack densely into chunks so the chunk-count check alone + // would not catch this over-cap set. + keys[i] = fmt.Sprintf("p%d", i) + } + if _, err := b.writeKeyIndex(keys, 0); err == nil { + t.Fatal("writeKeyIndex published a key count readKeyIndex would refuse") + } + if len(kr.data) != 0 { + t.Fatalf("over-cap key write must publish nothing, found %d entries", len(kr.data)) + } +} + +// TestStoreKeyringReadIndexDedupesDuplicateKeys is the regression test for the +// index fan-out DoS: a corrupted or adversarially crafted index that repeats +// the same key many times must collapse to its distinct, valid keys before +// read()/write() fan them out into one blocking keyring lookup per key. +func TestStoreKeyringReadIndexDedupesDuplicateKeys(t *testing.T) { + ckr := &countingKR{fakeKR: newFakeKR()} + blob := keyringBlob{kr: ckr, service: keyringService, legacyAccount: keyringLegacyAccount, indexAccount: keyringIndexAccount} + + dup := ProviderKey("demo") + many := make([]string, 2000) + for i := range many { + many[i] = dup + } + header, err := json.Marshal(keyIndexHeader{Version: 1, Chunks: 1, Keys: many}) + if err != nil { + t.Fatal(err) + } + ckr.data[keyringService+"/"+keyringIndexAccount] = base64.StdEncoding.EncodeToString(header) + + keys, ok, _, err := blob.readKeyIndex() + if err != nil { + t.Fatalf("readKeyIndex: %v", err) + } + if !ok { + t.Fatal("readKeyIndex: expected an index to be found") + } + if len(keys) != 1 { + t.Fatalf("readKeyIndex returned %d keys for a 2000-duplicate index, want 1 (deduplicated)", len(keys)) + } + + // A malformed (non-ValidateKey-shaped) entry must also be dropped rather + // than fanned out into a lookup. + mixed, err := json.Marshal(keyIndexHeader{Version: 1, Chunks: 1, Keys: []string{dup, dup, "not a valid key", ProviderKey("other")}}) + if err != nil { + t.Fatal(err) + } + ckr.data[keyringService+"/"+keyringIndexAccount] = base64.StdEncoding.EncodeToString(mixed) + keys, _, _, err = blob.readKeyIndex() + if err != nil { + t.Fatalf("readKeyIndex: %v", err) + } + want := map[string]bool{dup: true, ProviderKey("other"): true} + if len(keys) != len(want) { + t.Fatalf("readKeyIndex = %v, want exactly %v", keys, want) + } + for _, k := range keys { + if !want[k] { + t.Fatalf("readKeyIndex returned unexpected key %q", k) + } + } +} + +// TestStoreKeyringDuplicateIndexDoesNotFanOutPerEntry is the end-to-end +// regression test for the same DoS: a duplicate-heavy index must not drive +// one keyring Get per listed entry. Before the index is deduplicated at its +// source, a corrupted index holding thousands of copies of the same key would +// hold the store lock for one blocking keyring lookup (each up to the 10s +// command timeout) per copy, even though the cap on total distinct credentials +// this bug was meant to bound was never actually exceeded. +func TestStoreKeyringDuplicateIndexDoesNotFanOutPerEntry(t *testing.T) { + t.Setenv("XDG_CONFIG_HOME", t.TempDir()) + ckr := &countingKR{fakeKR: newFakeKR()} + + dup := ProviderKey("demo") + raw, err := json.Marshal(Token{AccessToken: "a"}) + if err != nil { + t.Fatal(err) + } + ckr.data[keyringService+"/"+dup] = base64.StdEncoding.EncodeToString(raw) + + many := make([]string, 3000) + for i := range many { + many[i] = dup + } + header, err := json.Marshal(keyIndexHeader{Version: 1, Chunks: 1, Keys: many}) + if err != nil { + t.Fatal(err) + } + ckr.data[keyringService+"/"+keyringIndexAccount] = base64.StdEncoding.EncodeToString(header) + + s, err := NewStore(StoreOptions{Storage: "keyring", Keyring: ckr}) + if err != nil { + t.Fatal(err) + } + + ckr.gets = 0 + statuses, err := s.Status("") + if err != nil { + t.Fatalf("Status: %v", err) + } + if len(statuses) != 1 { + t.Fatalf("Status returned %d entries for a 3000-duplicate index of one key, want 1", len(statuses)) + } + // One Get for the index header, one Get for the single deduplicated key's + // own entry — not one per (duplicate) index entry. + if ckr.gets > 2 { + t.Fatalf("Status issued %d keyring gets for a 3000-entry duplicate index, want <= 2 (fan-out DoS regression)", ckr.gets) + } +} + +// legacyGetFailKR fails Get for the legacy combined entry only, simulating a +// transient keyring read error (as opposed to the entry genuinely not +// existing, which fakeKR's normal Get reports as ok=false, err=nil). +type legacyGetFailKR struct { + *fakeKR + fail bool +} + +func (f *legacyGetFailKR) Get(service, account string) (string, bool, error) { + if f.fail && account == keyringLegacyAccount { + return "", false, errKRInjected + } + return f.fakeKR.Get(service, account) +} + +// TestStoreKeyringWriteRefusesToDeleteLegacyBlobOnTransientReadError is the +// regression test for finding #2: during mixed-version reconciliation (an old +// zero binary's legacy blob alongside the new keyring-based index), a +// transient error reading the legacy blob must not be treated as "the legacy +// blob is empty." write() must refuse to reconcile-and-delete it in that case, +// or an unread credential belonging to an older, still-installed zero binary +// is destroyed irrecoverably. +func TestStoreKeyringWriteRefusesToDeleteLegacyBlobOnTransientReadError(t *testing.T) { + t.Setenv("XDG_CONFIG_HOME", t.TempDir()) + kr := &legacyGetFailKR{fakeKR: newFakeKR()} + s, err := NewStore(StoreOptions{Storage: "keyring", Keyring: kr}) + if err != nil { + t.Fatal(err) + } + // A first save publishes the index, so indexExisted is true for the next + // write and exercises the mixed-version reconciliation path in write() + // where the bug lived. + if err := s.Save(ProviderKey("alpha"), Token{AccessToken: "a"}); err != nil { + t.Fatal(err) + } + + // A legacy blob still carries a credential written by an older, + // still-installed binary. + legacy := storeFile{SchemaVersion: storeSchemaVersion, Tokens: map[string]Token{ + ProviderKey("stale-binary-login"): {AccessToken: "still-live"}, + }} + data, err := json.Marshal(legacy) + if err != nil { + t.Fatal(err) + } + kr.data[keyringService+"/"+keyringLegacyAccount] = base64.StdEncoding.EncodeToString(data) + + // A transient failure reading it: Get returns a real error, not + // ok=false/err=nil ("doesn't exist"). + kr.fail = true + if err := s.Save(ProviderKey("beta"), Token{AccessToken: "b"}); err == nil { + t.Fatal("Save succeeded despite a transient legacy-blob read failure; it must refuse rather than silently treat the blob as empty") + } + // The legacy blob must still be present, not deleted out from under a + // read failure. + if _, ok := kr.data[keyringService+"/"+keyringLegacyAccount]; !ok { + t.Fatal("legacy blob was deleted despite a transient read failure (data loss)") + } + // Nothing else the aborted write touched should be visible either. + if _, ok, _ := s.Load(ProviderKey("beta")); ok { + t.Fatal("beta should not be visible: the write should have aborted entirely, not partially applied") + } + if _, ok, err := s.Load(ProviderKey("alpha")); err != nil || !ok { + t.Fatalf("alpha lost after an aborted write: ok=%v err=%v", ok, err) + } + + // Once the transient failure clears, the legacy credential is recovered + // and the blob is cleaned up normally. + kr.fail = false + if err := s.Save(ProviderKey("beta"), Token{AccessToken: "b"}); err != nil { + t.Fatalf("retried Save: %v", err) + } + if _, ok, err := s.Load(ProviderKey("stale-binary-login")); err != nil || !ok { + t.Fatalf("Load(stale-binary-login): ok=%v err=%v (legacy credential lost)", ok, err) + } + if _, ok := kr.data[keyringService+"/"+keyringLegacyAccount]; ok { + t.Fatal("legacy blob should be removed once it was actually read and reconciled") + } +}