diff --git a/internal/oauth/flow.go b/internal/oauth/flow.go index 9ff94cccd..6bcb17d0e 100644 --- a/internal/oauth/flow.go +++ b/internal/oauth/flow.go @@ -167,6 +167,14 @@ func Refresh(ctx context.Context, client *http.Client, cfg Config, current Token if trimmed(cfg.TokenEndpoint) == "" { return Token{}, errors.New("oauth: no token endpoint configured for refresh") } + // Prefer the scopes the current token was issued with; fall back to the + // configured defaults only when the stored token has none. The same set is + // sent on the wire and kept as the base so a response that omits scope + // cannot report a different grant than the provider just processed. + scopes := current.Scopes + if len(scopes) == 0 { + scopes = cfg.Scopes + } form := url.Values{} form.Set("grant_type", "refresh_token") form.Set("refresh_token", refresh) @@ -174,13 +182,10 @@ func Refresh(ctx context.Context, client *http.Client, cfg Config, current Token if secret := trimmed(cfg.ClientSecret); secret != "" { form.Set("client_secret", secret) } - if len(cfg.Scopes) > 0 { - form.Set("scope", strings.Join(cfg.Scopes, " ")) + if len(scopes) > 0 { + form.Set("scope", strings.Join(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} + 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..b605fd5eb 100644 --- a/internal/oauth/flow_test.go +++ b/internal/oauth/flow_test.go @@ -299,3 +299,45 @@ func TestRefreshPreservesTokenTypeWhenOmitted(t *testing.T) { t.Fatalf("refresh should carry the existing token_type forward, got %q", tok.TokenType) } } + +func TestRefreshPreservesScopesWhenOmitted(t *testing.T) { + var gotScope string + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + _ = r.ParseForm() + gotScope = r.FormValue("scope") + _, _ = 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 gotScope != "custom-scope" { + t.Fatalf("refresh form scope = %q, want current token scopes", gotScope) + } + if len(tok.Scopes) != 1 || tok.Scopes[0] != "custom-scope" { + t.Fatalf("refresh should carry existing scopes forward, got %v", tok.Scopes) + } +} + +func TestRefreshUsesConfigScopesWhenTokenHasNone(t *testing.T) { + var gotScope string + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + _ = r.ParseForm() + gotScope = r.FormValue("scope") + _, _ = w.Write([]byte(`{"access_token":"new-at","expires_in":3600}`)) + })) + 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"}, nil) + if err != nil { + t.Fatalf("Refresh: %v", err) + } + if gotScope != "fallback-scope" { + t.Fatalf("refresh form scope = %q, want cfg.Scopes fallback", gotScope) + } + if len(tok.Scopes) != 1 || tok.Scopes[0] != "fallback-scope" { + t.Fatalf("refresh should use cfg scopes when token has none, got %v", tok.Scopes) + } +} diff --git a/internal/oauth/lock.go b/internal/oauth/lock.go index d1dd344d2..338366c1e 100644 --- a/internal/oauth/lock.go +++ b/internal/oauth/lock.go @@ -11,27 +11,47 @@ import ( "github.com/Gitlawb/zero/internal/lockutil" ) -const ( - fileLockTimeout = 5 * time.Second +// Lock timing knobs (vars so tests can shorten absolute ceilings without +// changing production defaults). +var ( + // fileLockTimeout is how long acquisition waits after the last sign of a + // healthy holder (or when the lock path cannot be stated). A multi-entry + // keyring pass can legitimately run several 10s OS commands while refreshing + // the lease; contenders must not give up while that lease stays healthy. + // While the holder's mtime stays within fileLockStaleAfter, this idle + // deadline is extended so a fixed 5s window cannot fail a healthy peer. + fileLockTimeout = 5 * time.Second + // fileLockStaleAfter is how old a lock file's mtime must be before a waiter + // may reclaim it as abandoned. Must stay above one keyring command timeout + // plus lease refresh slack (holders refresh every fileLockRefreshInterval). fileLockStaleAfter = 30 * time.Second ) var lockSeq atomic.Uint64 // acquireFileLock takes a cross-process exclusive lock by creating lockPath with -// O_EXCL. It retries with a short backoff until a timeout, breaking a lock whose -// 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. -func acquireFileLock(lockPath string, now func() time.Time) (func(), error) { +// O_EXCL. It retries with a short backoff while a live holder's lease remains +// healthy (mtime refreshed within fileLockStaleAfter), reclaiming only a lock +// older than that threshold 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 returned token is the contents written into the lock file; lease refresh +// must re-check it before touching mtime so a replaced holder cannot keep a +// thief's lock alive. +// +// Timing always uses 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). Measuring the deadline with that clock would either never +// fire (fixed clock) or diverge from the mtime lease stamps (wall-clock). +func acquireFileLock(lockPath string, now func() time.Time) (unlock func(), token string, err error) { if now == nil { now = time.Now } if err := os.MkdirAll(filepath.Dir(lockPath), 0o700); err != nil { - return nil, err + return nil, "", err } - token := fmt.Sprintf("%d-%d-%d", os.Getpid(), now().UnixNano(), lockSeq.Add(1)) - deadline := now().Add(fileLockTimeout) + token = fmt.Sprintf("%d-%d-%d", os.Getpid(), now().UnixNano(), lockSeq.Add(1)) + idleDeadline := time.Now().Add(fileLockTimeout) for { f, err := os.OpenFile(lockPath, os.O_CREATE|os.O_EXCL|os.O_WRONLY, 0o600) if err == nil { @@ -41,11 +61,11 @@ func acquireFileLock(lockPath string, now func() time.Time) (func(), error) { if _, werr := f.WriteString(token); werr != nil { _ = f.Close() _ = lockutil.RemoveLockFile(lockPath) - return nil, fmt.Errorf("oauth: write token lock: %w", werr) + return nil, "", fmt.Errorf("oauth: write token lock: %w", werr) } if cerr := f.Close(); cerr != nil { _ = lockutil.RemoveLockFile(lockPath) - return nil, fmt.Errorf("oauth: close token lock: %w", cerr) + return nil, "", fmt.Errorf("oauth: close token lock: %w", cerr) } var released bool return func() { @@ -56,7 +76,7 @@ func acquireFileLock(lockPath string, now func() time.Time) (func(), error) { if data, rerr := os.ReadFile(lockPath); rerr == nil && string(data) == token { _ = lockutil.RemoveLockFile(lockPath) } - }, nil + }, token, nil } // On Windows a concurrent holder's os.Remove leaves the lock file in a // "delete pending" state, so an O_EXCL create races it with @@ -64,33 +84,61 @@ func acquireFileLock(lockPath string, now func() time.Time) (func(), error) { // as contention and retry, exactly like ErrExist — otherwise the lock // spuriously fails under concurrency on Windows. if !errors.Is(err, os.ErrExist) && !errors.Is(err, os.ErrPermission) { - return nil, fmt.Errorf("oauth: acquire token lock: %w", err) + return nil, "", fmt.Errorf("oauth: acquire token lock: %w", err) } // Reclaim a stale lock left by a crashed holder — atomically (H3). A blind // Remove lets two racers both reclaim + recreate and so both hold the lock; // reclaimStaleLock renames the file aside (only one rename wins) and restores // it if it turns out fresh, so a live lock is never deleted out from under it. - if info, statErr := os.Stat(lockPath); statErr == nil && time.Since(info.ModTime()) > fileLockStaleAfter { - cleared, rerr := lockutil.ReclaimStaleLock(lockPath, token, func(reclaimedPath string) bool { - info, err := os.Stat(reclaimedPath) - return err == nil && time.Since(info.ModTime()) <= fileLockStaleAfter - }) - if rerr != nil { - // Reclaim hit a hard failure: the rename aside failed outright, or a - // live holder's lock could not be put back (the lock path may be - // missing, so re-acquiring would break mutual exclusion). Fail closed - // instead of spinning to the deadline. - return nil, fmt.Errorf("oauth: reclaim stale token lock: %w", rerr) - } - if cleared { - continue + if info, statErr := os.Stat(lockPath); statErr == nil { + age := time.Since(info.ModTime()) + // Future mtimes (clock skew, hostile Chtimes) are not healthy leases: + // age is negative, so neither the reclaim branch nor the deadline + // extension below treats them as live. Contenders time out instead of + // waiting forever on a never-stale lock. + if age > fileLockStaleAfter { + cleared, rerr := lockutil.ReclaimStaleLock(lockPath, token, func(reclaimedPath string) bool { + info, err := os.Stat(reclaimedPath) + if err != nil { + return false + } + reclaimedAge := time.Since(info.ModTime()) + return reclaimedAge >= 0 && reclaimedAge <= fileLockStaleAfter + }) + if rerr != nil { + // Reclaim hit a hard failure: the rename aside failed outright, or a + // live holder's lock could not be put back (the lock path may be + // missing, so re-acquiring would break mutual exclusion). Fail closed + // instead of spinning to the deadline. + return nil, "", fmt.Errorf("oauth: reclaim stale token lock: %w", rerr) + } + if cleared { + continue + } + // Lost the reclaim race, or isLive reported a still-fresh holder + // (callback true → ReclaimStaleLock restores and returns false). + // Refresh the idle deadline so reclaim work that overran the prior + // window does not immediately time out a healthy peer. + idleDeadline = time.Now().Add(fileLockTimeout) + } else if age >= 0 { + // Holder looks healthy (lease refreshed recently, mtime not in the + // future). Keep waiting for the critical section to finish rather + // than timing out after a fixed window shorter than a legitimate + // multi-entry keyring pass. + idleDeadline = time.Now().Add(fileLockTimeout) } - // 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) { - return nil, fmt.Errorf("oauth: timed out acquiring token lock %s", filepath.Base(lockPath)) + if time.Now().After(idleDeadline) { + return nil, "", fmt.Errorf("oauth: timed out acquiring token lock %s", filepath.Base(lockPath)) } time.Sleep(10 * time.Millisecond) } } + +// ownLockFile reports whether path still holds token. Used by lease refresh so +// a holder that was reclaimed after a long pause cannot Chtimes a replacement +// lock and keep two critical sections alive. +func ownLockFile(path, token string) bool { + data, err := os.ReadFile(path) + return err == nil && string(data) == token +} diff --git a/internal/oauth/lock_owner_unix.go b/internal/oauth/lock_owner_unix.go new file mode 100644 index 000000000..7400378db --- /dev/null +++ b/internal/oauth/lock_owner_unix.go @@ -0,0 +1,24 @@ +//go:build !windows + +package oauth + +import ( + "fmt" + "os" + "syscall" +) + +// checkOAuthLockDirOwner rejects a fallback lock directory not owned by the +// current user: on a shared temp root another user could have pre-created the +// path and would then control its lifetime (deletion/renaming), permanently +// denying OAuth keyring operations. +func checkOAuthLockDirOwner(info os.FileInfo) error { + stat, ok := info.Sys().(*syscall.Stat_t) + if !ok { + return nil + } + if int(stat.Uid) != os.Geteuid() { + return fmt.Errorf("oauth lock fallback directory is owned by uid %d, not the current user", stat.Uid) + } + return nil +} diff --git a/internal/oauth/lock_owner_windows.go b/internal/oauth/lock_owner_windows.go new file mode 100644 index 000000000..d47a89ded --- /dev/null +++ b/internal/oauth/lock_owner_windows.go @@ -0,0 +1,11 @@ +//go:build windows + +package oauth + +import "os" + +// checkOAuthLockDirOwner is a no-op on Windows: the process temp directory is +// per-user by default, and keyringFallbackLockDir returns it directly. +func checkOAuthLockDirOwner(os.FileInfo) error { + return nil +} diff --git a/internal/oauth/store.go b/internal/oauth/store.go index 951e9616f..759a5c5e9 100644 --- a/internal/oauth/store.go +++ b/internal/oauth/store.go @@ -5,13 +5,17 @@ import ( "encoding/json" "errors" "fmt" + "log" + "net/url" "os" + "os/user" "path/filepath" "regexp" "runtime" "sort" "strings" "sync" + "sync/atomic" "time" "github.com/Gitlawb/zero/internal/keyring" @@ -30,6 +34,8 @@ const ( // so a key can never traverse or collide with store internals. var keyPattern = regexp.MustCompile(`^(provider|mcp):[A-Za-z0-9][A-Za-z0-9._-]{0,127}$`) +var currentOSUser = user.Current + // ValidateKey reports whether key is a well-formed namespaced token key. func ValidateKey(key string) error { if !keyPattern.MatchString(key) { @@ -106,10 +112,37 @@ 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. +// +// Coexistence with pre-per-key binaries: the legacy combined entry is a +// read-only discovery source for new code. New writers never overwrite it +// (they cannot share a lock with old writers on other config roots, so any +// snapshot-then-Set would clobber unobserved updates or truncate oversized +// Linux keyring maps). Indexed per-key entries are the sole writable +// representation for new binaries. Durable deletion markers (tombstones) +// prevent an uncoordinated old writer from resurrecting a logout via the +// legacy blob. const ( keyringService = "zero" - keyringAccount = "oauth-tokens" + // keyringLegacyAccount is the combined-blob entry used by pre-per-key + // binaries. New code reads it for migration and for legacy-only logins, + // but never writes or deletes it: dual-write cannot be made safe across + // config roots that do not share legacyKeyringLockPath. + 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" + // keyringTombstoneAccount holds the set of keys deliberately deleted by a + // new binary. Old writers cannot see this entry; new readers and writers + // honor it so a stale legacy rewrite cannot resurrect a logout. + keyringTombstoneAccount = "oauth-tokens-tombstones" ) // Store persists OAuth tokens (provider + MCP namespaces) as one JSON blob, @@ -139,13 +172,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 +187,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 +243,23 @@ 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") + // 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, err := keyringLockPath(options.Env, keyringService, keyringIndexAccount) + if err != nil { + return nil, err } - return &Store{blob: keyringBlob{kr: kr, service: keyringService, account: keyringAccount, lockPath: lockPath}, now: now}, nil + 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 +284,113 @@ 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 (service + index account) and anchored on the user's OS home directory +// (via user.Current()) rather than caller-controlled environment overrides +// like HOME, XDG_CACHE_HOME, or TMPDIR: those pick different paths per process +// (sandboxes, CI harnesses, launcher profiles), so two processes for the same +// OS user would take different lock files while writing to the same OS keychain. +// When the OS user lookup fails, the fallback is a private UID-scoped directory +// under the process temp root (validated 0700, owned by us); a co-tenant DoS +// of the shared /tmp name is rejected rather than accepted as the lock path. +func keyringLockPath(env map[string]string, service, account string) (string, error) { + name := keyringLockFileName(service, account) + if u, err := currentOSUser(); err == nil && strings.TrimSpace(u.HomeDir) != "" { + return filepath.Join(u.HomeDir, ".cache", "zero", name), nil + } + // Do not fall back to os.UserHomeDir: it reads ambient HOME/USERPROFILE, so + // two same-user processes can choose different locks for one keyring. + dir, err := keyringFallbackLockDir() + if err != nil { + return "", fmt.Errorf("oauth: keyring lock fallback dir: %w", err) + } + return filepath.Join(dir, keyringTempLockName(service, account)), nil +} + +// keyringFallbackLockDir returns a private directory for last-resort keyring +// locks when the OS user home cannot be resolved. On Windows the process temp +// dir is already per-user. Elsewhere a UID-scoped 0700 directory under the +// process temp root is created and validated so a co-tenant cannot pre-create +// the lock file (or a world-writable parent) and permanently deny OAuth. +func keyringFallbackLockDir() (string, error) { + if runtime.GOOS == "windows" { + return os.TempDir(), nil + } + name := "zero-oauth-locks" + if uid := os.Getuid(); uid >= 0 { + name = fmt.Sprintf("zero-oauth-locks-%d", uid) + } + dir := filepath.Join(os.TempDir(), name) + if err := os.MkdirAll(dir, 0o700); err != nil { + return "", err + } + info, err := os.Lstat(dir) + if err != nil { + return "", err + } + if info.Mode()&os.ModeSymlink != 0 || !info.IsDir() { + return "", fmt.Errorf("oauth lock fallback %s is not a plain directory", dir) + } + if info.Mode().Perm() != 0o700 { + if err := os.Chmod(dir, 0o700); err != nil { + return "", fmt.Errorf("tighten oauth lock fallback permissions: %w", err) + } + } + if err := checkOAuthLockDirOwner(info); err != nil { + return "", err + } + return dir, nil +} + +// 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 dual-writes the legacy +// entry when the old binary shares this config root. Old binaries on other +// roots cannot share this lock; dual-write-without-delete is the safety net +// for that case. Best-effort: "" when the file-backend location can't be +// resolved at all, matching the legacy code's own best-effort fallback. +func legacyKeyringLockPath(env map[string]string) string { + // Use ResolveStorePath so the legacy lock lives beside whatever the + // legacy binary actually stores to (honoring ZERO_OAUTH_TOKENS_PATH + // and XDG_CONFIG_HOME), matching the old binary's own lock path. + storePath, err := ResolveStorePath(env) + if err != nil { + return "" + } + return filepath.Join(filepath.Dir(storePath), "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() } @@ -245,7 +408,7 @@ func (s *Store) Save(key string, token Token) error { return err } state.Tokens[key] = token - return s.writeState(state) + return s.writeState(state, map[string]bool{key: false}) }) } @@ -256,7 +419,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 } @@ -282,7 +457,10 @@ func (s *Store) Delete(key string) (bool, error) { } delete(state.Tokens, key) removed = true - return s.writeState(state) + // Exclude the deleted key from legacy reconciliation so a credential + // that was only present in the legacy blob (never indexed) is not + // reclassified as a fresh old-binary login and written back. + return s.writeState(state, map[string]bool{key: true}) }) return removed, err } @@ -292,7 +470,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 } @@ -354,7 +540,10 @@ func (s *Store) readState() (storeFile, error) { return state, nil } -func (s *Store) writeState(state storeFile) error { +// writeState persists state. mutations identifies explicitly saved (false) and +// deleted (true) keys. The keyring backend uses it to order durable tombstone +// transitions; file and encrypted-file backends ignore it. +func (s *Store) writeState(state storeFile, mutations map[string]bool) error { data, err := json.MarshalIndent(state, "", " ") if err != nil { return err @@ -368,7 +557,7 @@ func (s *Store) writeState(state storeFile) error { return err } } - return s.blob.write(payload) + return s.blob.write(payload, mutations) } func emptyStoreFile() storeFile { @@ -380,12 +569,21 @@ func emptyStoreFile() storeFile { type blobStore interface { // read returns the stored blob; ok is false when nothing is stored yet. read() (data []byte, ok bool, err error) - // write replaces the stored blob. - write(data []byte) error + // write replaces the stored blob. mutations is keyring-only and identifies + // explicit saves (false) and deletes (true) for durable tombstone ordering. + // File backends ignore it. + write(data []byte, mutations map[string]bool) error // withLock runs fn under whatever cross-process exclusion the backend offers // (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 } @@ -405,7 +603,7 @@ func (b fileBlob) read() ([]byte, bool, error) { return data, true, nil } -func (b fileBlob) write(data []byte) error { +func (b fileBlob) write(data []byte, _ map[string]bool) error { if err := os.MkdirAll(filepath.Dir(b.path), 0o700); err != nil { return err } @@ -421,7 +619,7 @@ func (b fileBlob) write(data []byte) error { } func (b fileBlob) withLock(now func() time.Time, fn func() error) error { - unlock, err := acquireFileLock(b.path+".lockfile", now) + unlock, _, err := acquireFileLock(b.path+".lockfile", now) if err != nil { return err } @@ -429,21 +627,159 @@ 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 and legacy-only logins from old binaries. + // New code never writes this account (see package comment on coexistence). + 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() still holds it when the old binary shares + // this config root so concurrent legacy mutations serialize with our + // reconcile-and-index pass. Cross-root old writers cannot share that lock; + // safety there comes from never overwriting the legacy blob and from + // durable tombstones, not from dual-write. + legacyLockPath string + // maxIndexKeys overrides the live credential cap for bounded metadata + // indexes, such as tombstones, that do not fan out into per-key reads. + maxIndexKeys int } 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 + } + // Tombstones are authoritative even before the first index commits. A + // delete can persist its marker and then be interrupted while publishing the + // initial index; returning the untouched legacy blob here would resurrect it. + tombstones, err := b.readTombstones() + if err != nil { + return nil, false, err + } + if !ok { + data, legacyOK, err := b.readLegacy() + if err != nil || !legacyOK || len(tombstones) == 0 { + return data, legacyOK, err + } + var state storeFile + if err := json.Unmarshal(data, &state); err != nil { + return nil, false, fmt.Errorf("oauth: invalid legacy keyring token blob: %w", err) + } + for key := range tombstones { + delete(state.Tokens, key) + } + filtered, err := json.Marshal(state) + return filtered, true, err + } + // Tombstones block resurrection of deliberately deleted keys from the + // legacy combined entry (an old binary may rewrite a stale snapshot into + // that account after logout). Fail closed on a corrupt tombstone set so a + // damaged marker cannot silently re-expose logged-out credentials. + // The legacy combined entry is consulted when an indexed key's own entry is + // missing (torn write / migration) and for keys only present there (an old + // binary logged into a provider this process has never indexed). Indexed + // entries always win over legacy for the same key: expiry and token material + // are not a causal version vector, so preferring "fresher-looking" legacy + // can overwrite an explicit new-binary Save with an older account's token. + var legacyTokens map[string]Token + legacyLoaded := false + loadLegacy := func() { + if legacyLoaded { + return + } + // Best-effort on read: a transient failure must not fail Load/Status, + // only skip legacy recovery for this pass. write() still requires a + // successful legacy read before reconciling so it never mistakes a + // transient error for an empty blob. + if lt, lerr := b.readLegacyTokens(); lerr == nil { + legacyTokens = lt + } + legacyLoaded = true + } + 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 present and not tombstoned; otherwise + // skip rather than fail the whole read (the next Save/Delete prunes + // the phantom index key so it cannot permanently consume capacity). + // Tombstones do not hide a still-present indexed entry (in-flight + // delete): they only block resurrection from the legacy account. + if tombstones[key] { + continue + } + loadLegacy() + 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 + } + + // Keep legacy-only keys visible through the compatibility window: + // an old binary may have logged into a provider after the index was created. + // Tombstones suppress keys the user already logged out of. + loadLegacy() + for key, legacyToken := range legacyTokens { + if ValidateKey(key) != nil { + continue + } + if tombstones[key] { + continue + } + if _, has := tokens[key]; !has { + 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 +790,718 @@ 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() merges legacy-only keys into the indexed representation, +// and mistaking a transient read failure for an empty blob would skip +// credentials that still live only in that account. +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() +// 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 unless tombstoned, +// or skips them; the next write prunes phantom index keys so they cannot +// permanently consume capacity) 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 never written or deleted by this path. New +// code cannot share a lock with old writers on other config roots, so any +// snapshot-then-Set of that account can clobber an unobserved login or +// truncate a valid oversized Linux keyring map. Legacy stays a read-only +// discovery source; indexed entries are the sole writable representation. +// omitFromLegacy lists keys the caller just deleted; they are recorded as +// durable tombstones and must not be re-merged from the legacy blob even +// when they were never indexed (a legacy-only old-binary login that the +// user logged out of). +func (b keyringBlob) write(data []byte, mutations map[string]bool) 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, indexIncomplete, 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 + } + + tombstones, err := b.readTombstones() + if err != nil { + return err + } + // Record durable deletion markers before mutating entries so a crash + // mid-write cannot leave a logged-out key importable from legacy alone. + for key, deleted := range mutations { + if deleted { + tombstones[key] = true + } + } + + // An older binary running alongside this one still reads and writes only the + // legacy combined entry. Merge keys that entry holds which the indexed + // schema has never seen (fresh old-binary logins), unless tombstoned or + // omitted by this operation. Never overwrite a key already present in + // state: expiry and token strings are not causal order. Keys in the prior + // index but absent from this write were deliberately removed (logout) and + // must not be resurrected. + // + // Unlike read()'s best-effort fallback, a failure here must abort the whole + // write rather than proceed as though the legacy blob were empty: skipping + // a still-live legacy-only credential would leave it unindexed until the + // next successful reconcile, and a concurrent old-writer update is only + // discoverable through this read. + legacyTokens, err := b.readLegacyTokens() + if err != nil { + return fmt.Errorf("oauth: read legacy keyring token blob for reconciliation: %w", err) + } + if indexExisted { + for key, legacyToken := range legacyTokens { + if ValidateKey(key) != nil { + continue + } + if mutations[key] || tombstones[key] { + continue + } + if _, exists := state.Tokens[key]; exists { + 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) + + // Preflight: marshal and size-check every desired token BEFORE publishing + // any index key. Publishing first left rejected oversized Saves as permanent + // index phantoms that could exhaust maxKeyringIndexKeys and brick the store. + encoded := make(map[string]string, len(keys)) + for _, key := range keys { + raw, err := json.Marshal(state.Tokens[key]) + if err != nil { + return err + } + enc := base64.StdEncoding.EncodeToString(raw) + if len(enc) > maxKeyringSingleEntryBytes { + return fmt.Errorf("oauth: token payload for %q (%d bytes) exceeds single keyring entry bound (%d bytes); use file or encrypted-file storage", key, len(enc), maxKeyringSingleEntryBytes) + } + encoded[key] = enc + } + + // Drop prior index keys that have neither a live entry nor a place in the + // desired set (phantoms from an interrupted Set after a previous union + // publish). Including them in the next union would permanently consume + // index capacity after enough failed writes. + livePrior := make([]string, 0, len(priorKeys)) + for _, key := range priorKeys { + if _, ok := state.Tokens[key]; ok { + livePrior = append(livePrior, key) + continue + } + _, exists, err := b.kr.Get(b.service, key) + if err != nil { + return err + } + if exists { + livePrior = append(livePrior, key) + } + } + + // 1. Persist tombstones before removing entries so logout survives a crash + // between entry delete and a later reconcile (and survives an old binary + // rewriting the legacy blob with the deleted key still present). + if err := b.writeTombstones(tombstones); err != nil { + return err + } + // 2. Publish the union of the live prior and new key sets first, so every + // entry that exists at any point during this update is indexed. + // + // When a referenced continuation chunk was missing, livePrior is truncated + // and cannot name the unlisted keys. Keep advertising the prior chunk count + // (and never delete those chunk accounts) so a later-restored chunk can + // still be reconciled; a complete rewrite to only the known keys would + // permanently orphan their OS keychain entries. + union := keys + if len(livePrior) > 0 { + merged := make(map[string]bool, len(keys)+len(livePrior)) + for _, key := range append(append([]string{}, keys...), livePrior...) { + 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, indexIncomplete) + if err != nil { + return err + } + // 3. Write each token entry (encodings preflighted above). + for _, key := range keys { + if err := b.kr.Set(b.service, key, encoded[key]); err != nil { + return err + } + } + // 4. Delete removed entries while the union index still lists them, so a + // failed Delete leaves a visible (re-deletable) entry, never an orphan. + // Only walk livePrior (keys we could see): entries named only in a missing + // chunk stay put so a restored chunk can still find them. + for _, key := range livePrior { + if _, ok := state.Tokens[key]; !ok { + if _, err := b.kr.Delete(b.service, key); err != nil { + return err + } + } + } + // 5. Shrink the index to the exact new key set. Legacy is left untouched. + // Skip shrink when the prior index was incomplete: a shrink to `keys` would + // drop the preserved chunk advertisements and strand unlisted entries. + if !indexIncomplete { + if _, err := b.writeKeyIndex(keys, unionChunks, false); err != nil { + return err + } + } + // A re-login clears its tombstone only after the replacement entry and exact + // index are durable. If any earlier step fails, legacy fallback remains + // suppressed instead of restoring the revoked credential. + tombstonesChanged := false + for key, deleted := range mutations { + if !deleted && tombstones[key] { + delete(tombstones, key) + tombstonesChanged = true + } + } + if tombstonesChanged { + if err := b.writeTombstones(tombstones); err != nil { + return err + } + } + return nil +} + +// tombstoneBlob returns a keyringBlob that reuses the chunked index codec for +// the durable deletion set. Tombstones can grow to the same key/chunk caps as +// the live index (max-length keys after many logouts), so a single entry is +// not enough under the macOS line bound. +func (b keyringBlob) tombstoneBlob() keyringBlob { + return keyringBlob{kr: b.kr, service: b.service, indexAccount: keyringTombstoneAccount, maxIndexKeys: maxKeyringTombstoneKeys} +} + +// readTombstones returns the durable set of keys deleted by a new binary. +// Missing account => empty set. Corrupt payloads fail closed. +func (b keyringBlob) readTombstones() (map[string]bool, error) { + keys, ok, _, _, err := b.tombstoneBlob().readKeyIndex() + if err != nil { + return nil, fmt.Errorf("oauth: read keyring token tombstones: %w", err) + } + if !ok { + return map[string]bool{}, nil + } + out := make(map[string]bool, len(keys)) + for _, key := range keys { + out[key] = true + } + return out, nil +} + +// writeTombstones persists the durable deletion set. An empty set removes every +// tombstone account/chunk so a fully clean store does not leave leftover +// markers. Errors from that cleanup are surfaced so interruption tests and +// real keyring failures cannot be swallowed. +func (b keyringBlob) writeTombstones(tombstones map[string]bool) error { + tb := b.tombstoneBlob() + _, existed, priorChunks, _, err := tb.readKeyIndex() + if err != nil { + return fmt.Errorf("oauth: read keyring token tombstones: %w", err) + } + if len(tombstones) == 0 { + if !existed { + return nil + } + if _, err := tb.kr.Delete(tb.service, tb.indexAccount); err != nil { + return err + } + for i := 1; i < priorChunks; i++ { + if _, err := tb.kr.Delete(tb.service, tb.chunkAccount(i)); err != nil { + return err + } + } + return nil + } + if len(tombstones) > maxKeyringTombstoneKeys { + return errKeyringIndexTooManyKeys(len(tombstones), maxKeyringTombstoneKeys) + } + keys := make([]string, 0, len(tombstones)) + for key := range tombstones { + if ValidateKey(key) != nil { + continue + } + keys = append(keys, key) + } + sort.Strings(keys) + if _, err := tb.writeKeyIndex(keys, priorChunks, false); err != nil { + return fmt.Errorf("oauth: write keyring token tombstones: %w", err) + } + return nil +} + +// maxKeyringSingleEntryBytes bounds a single base64-encoded token secret so +// that the line passed to macOS `security -i` stays comfortably under the +// 4095-byte command line cap (see internal/keyring). +const maxKeyringSingleEntryBytes = 3800 + +// 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 + +// maxKeyringIndexEncodedBytes bounds one index header/chunk's base64 string +// before DecodeString or json.Unmarshal. Writers never emit more than +// maxKeyringIndexChunkBytes of raw JSON per chunk (header wraps one chunk of +// keys plus a few metadata fields), so anything larger is damaged or hostile +// and must be rejected without allocating unbounded decode buffers on the +// hot path that holds the store lock. +const maxKeyringIndexEncodedBytes = 4096 + +// 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 + +// Tombstones do not fan out into per-key keyring reads, so they can use the +// codec's bounded raw capacity without imposing the live credential cap. +const maxKeyringTombstoneKeys = maxRawKeyringIndexKeys + +// maxRawKeyringIndexKeys bounds the raw decoded element count before +// deduplication or map preallocation, guarding against DoS from duplicate keys. +const maxRawKeyringIndexKeys = 16384 + +// errKeyringIndexTooManyKeys is returned when a decoded index (or one of its +// chunks) claims more keys than maxKeyringIndexKeys. +func errKeyringIndexTooManyKeys(count, limit int) error { + log.Printf("warning: oauth: keyring token index lists %d keys, over the %d-key cap", count, limit) + return fmt.Errorf("oauth: keyring token index lists %d keys, over the %d-key cap", count, limit) +} + +// 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) indexKeyLimit() int { + if b.maxIndexKeys > 0 { + return b.maxIndexKeys + } + return maxKeyringIndexKeys +} + +func (b keyringBlob) chunkAccount(index int) string { + return fmt.Sprintf("%s-%d", b.indexAccount, index) +} + +// decodeKeyringIndexPayload bounds and decodes one index header/chunk value +// before json.Unmarshal. The element-count cap alone does not bound the size of +// a single JSON string inside a damaged payload. +func decodeKeyringIndexPayload(enc string, what string) ([]byte, error) { + enc = strings.TrimSpace(enc) + if len(enc) > maxKeyringIndexEncodedBytes { + return nil, fmt.Errorf("oauth: %s is %d bytes encoded, over the %d-byte bound", what, len(enc), maxKeyringIndexEncodedBytes) + } + raw, err := base64.StdEncoding.DecodeString(enc) + if err != nil { + return nil, fmt.Errorf("oauth: decode %s: %w", what, err) + } + // Header wraps one chunk of keys plus a few metadata fields; reject anything + // well beyond the writer-side raw chunk budget before Unmarshal. + if len(raw) > maxKeyringIndexChunkBytes+256 { + return nil, fmt.Errorf("oauth: %s decodes to %d bytes, over the %d-byte raw bound", what, len(raw), maxKeyringIndexChunkBytes+256) + } + return raw, nil +} + +// readKeyIndex returns the indexed keys, whether an index exists at all, +// how many chunk entries it currently occupies, and whether a referenced +// continuation chunk was missing. A missing chunk (external keychain damage +// or a torn write outside this code's write order) is skipped so reads stay +// available, but incomplete is true so write() can refuse to shrink the +// index and strand the unlisted entries as undeletable orphans. +func (b keyringBlob) readKeyIndex() (keys []string, ok bool, chunks int, incomplete bool, err error) { + enc, ok, err := b.kr.Get(b.service, b.indexAccount) + if err != nil { + return nil, false, 0, false, err + } + if !ok { + return nil, false, 0, false, nil + } + raw, err := decodeKeyringIndexPayload(enc, "keyring token index") + if err != nil { + return nil, false, 0, false, err + } + trimmed := strings.TrimSpace(string(raw)) + if strings.HasPrefix(trimmed, "[") { + var rawKeys []string + if err := json.Unmarshal(raw, &rawKeys); err != nil { + return nil, false, 0, false, fmt.Errorf("oauth: decode keyring token index: %w", err) + } + if len(rawKeys) > maxRawKeyringIndexKeys { + return nil, false, 0, false, errKeyringIndexTooManyKeys(len(rawKeys), maxRawKeyringIndexKeys) + } + keys := dedupeValidKeys(rawKeys) + if len(keys) > b.indexKeyLimit() { + return nil, false, 0, false, errKeyringIndexTooManyKeys(len(keys), b.indexKeyLimit()) + } + return keys, true, 1, false, nil + } + var header keyIndexHeader + if err := json.Unmarshal(raw, &header); err != nil { + return nil, false, 0, false, 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, false, fmt.Errorf("oauth: unsupported keyring token index version %d", header.Version) + } + if header.Chunks < 1 || header.Chunks > maxKeyringIndexChunks { + return nil, false, 0, false, fmt.Errorf("oauth: keyring token index advertises %d chunks (want 1..%d)", header.Chunks, maxKeyringIndexChunks) + } + rawKeys := header.Keys + if len(rawKeys) > maxRawKeyringIndexKeys { + return nil, false, 0, false, errKeyringIndexTooManyKeys(len(rawKeys), maxRawKeyringIndexKeys) + } + incomplete = false + for i := 1; i < header.Chunks; i++ { + chunkEnc, chunkOK, err := b.kr.Get(b.service, b.chunkAccount(i)) + if err != nil { + return nil, false, 0, false, err + } + if !chunkOK { + // Skip so Load/Status stay available, but remember the damage so + // write() does not shrink away the unlisted keys' entries. + incomplete = true + continue + } + chunkRaw, err := decodeKeyringIndexPayload(chunkEnc, fmt.Sprintf("keyring token index chunk %d", i)) + if err != nil { + return nil, false, 0, false, err + } + var more []string + if err := json.Unmarshal(chunkRaw, &more); err != nil { + return nil, false, 0, false, fmt.Errorf("oauth: decode keyring token index chunk %d: %w", i, err) + } + if len(rawKeys)+len(more) > maxRawKeyringIndexKeys { + return nil, false, 0, false, errKeyringIndexTooManyKeys(len(rawKeys)+len(more), maxRawKeyringIndexKeys) + } + rawKeys = append(rawKeys, more...) + } + keys = dedupeValidKeys(rawKeys) + if len(keys) > b.indexKeyLimit() { + return nil, false, 0, false, errKeyringIndexTooManyKeys(len(keys), b.indexKeyLimit()) + } + return keys, true, header.Chunks, incomplete, 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 the published header advertises. Continuation chunks are written +// before the header that references them, so the authoritative chunk 0 never +// advertises a content 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). +// +// keepMissingChunks is set when readKeyIndex reported a missing continuation +// chunk. In that mode the header keeps advertising at least priorChunks so a +// later-restored chunk remains reachable, and chunk accounts in that range are +// not deleted (overwriting or removing them would turn recoverable damage into +// permanent orphans). +func (b keyringBlob) writeKeyIndex(keys []string, priorChunks int, keepMissingChunks bool) (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) > b.indexKeyLimit() { + return 0, errKeyringIndexTooManyKeys(len(keys), b.indexKeyLimit()) + } + 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) + } + // Only write content chunks we produced. When preserving a damaged prior + // index, higher-numbered accounts may still hold recoverable key lists. + 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 + } + } + advertised := len(chunks) + if keepMissingChunks && priorChunks > advertised { + advertised = priorChunks + } + if advertised > maxKeyringIndexChunks { + return 0, fmt.Errorf("oauth: keyring key index needs %d chunks, over the %d-chunk cap readers accept; too many stored credentials", advertised, maxKeyringIndexChunks) + } + headerData, err := json.Marshal(keyIndexHeader{Version: 1, Chunks: advertised, 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 + } + if !keepMissingChunks { + for i := len(chunks); i < priorChunks; i++ { + _, _ = b.kr.Delete(b.service, b.chunkAccount(i)) + } + } + return advertised, 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 + +// leasedPath is one acquired lock whose mtime is refreshed until stop is +// closed. Lease ownership starts at acquisition, not after every path is +// held: withLock acquires lockPath then may block on legacyLockPath, and a +// peer must not be able to reclaim the first lock as stale during that wait. +// Refresh is ownership-aware: if a peer reclaims and replaces the lock while +// this holder is paused, Chtimes is skipped and lost is set so the critical +// section can fail closed instead of keeping the thief's lock forever-fresh. +type leasedPath struct { + path string + token string + unlock func() + stop chan struct{} + done chan struct{} + lost atomic.Bool +} + +func startLease(path, token string, unlock func()) *leasedPath { + l := &leasedPath{ + path: path, + token: token, + unlock: unlock, + stop: make(chan struct{}), + done: make(chan struct{}), + } + go func() { + defer close(l.done) + ticker := time.NewTicker(fileLockRefreshInterval) + defer ticker.Stop() + for { + select { + case <-l.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. Only refresh while we still own the + // token: a post-stale reclaim can replace the file, and Chtimes on + // the replacement would keep both holders inside the critical section. + // Re-check ownership after Chtimes as well: between the pre-check and + // the stamp a peer can swap the file, and a successful Chtimes on the + // thief's lock would keep both critical sections alive. + if !ownLockFile(path, token) { + l.lost.Store(true) + return + } + at := time.Now() + _ = os.Chtimes(path, at, at) + if !ownLockFile(path, token) { + l.lost.Store(true) + return + } + } + } + }() + return l +} + +func (l *leasedPath) release() { + close(l.stop) + <-l.done + l.unlock() +} + +// withLeasedLocks acquires every non-empty path in order. Each lock's mtime +// lease starts immediately on acquisition (and keeps refreshing while later +// paths are still being acquired and while fn runs), so a multi-lock wait +// cannot leave an earlier lock looking abandoned. Locks are released in +// reverse order once fn returns — including when fn panics — so a recovered +// panic cannot leave a forever-refreshed lock that wedges every later waiter. +// If a lease is replaced under us mid-critical-section, the operation fails +// closed after fn returns (or with fn's error) rather than treating a dual- +// entry window as success. +func withLeasedLocks(paths []string, now func() time.Time, fn func() error) error { + var leases []*leasedPath + released := false + releaseAll := func() { + if released { + return + } + released = true + for i := len(leases) - 1; i >= 0; i-- { + leases[i].release() + } + } + for _, p := range paths { + if p == "" { + continue + } + unlock, token, err := acquireFileLock(p, now) + if err != nil { + releaseAll() + return err + } + // Start refreshing this lock before blocking on the next path. + leases = append(leases, startLease(p, token, unlock)) + } + if len(leases) == 0 { + return fn() + } + defer releaseAll() + err := fn() + for _, l := range leases { + if l.lost.Load() { + if err != nil { + return err + } + return fmt.Errorf("oauth: lost token lock lease on %s", filepath.Base(l.path)) + } + } + 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 that shares this config root (see +// legacyKeyringLockPath) serializes with our reconcile-and-index pass. +// Cross-root old writers cannot share that lock; never overwriting the +// legacy blob and durable tombstones are the remaining safety net for them. +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..5244b3274 100644 --- a/internal/oauth/store_keyring_test.go +++ b/internal/oauth/store_keyring_test.go @@ -1,10 +1,50 @@ package oauth import ( + "encoding/base64" + "encoding/json" + "fmt" + "os" + "os/user" + "path/filepath" "strings" "testing" + "time" + + "github.com/Gitlawb/zero/internal/lockutil" ) +// TestMain redirects keyring lock paths into a process-private temp home so +// the suite never creates lock files under the real user home directory +// (keyringLockPath deliberately ignores XDG_CONFIG_HOME / HOME env). Tests +// that must observe real OS identity re-stub currentOSUser themselves. +func TestMain(m *testing.M) { + dir, err := os.MkdirTemp("", "zero-oauth-test-home-*") + if err != nil { + panic(err) + } + currentOSUser = func() (*user.User, error) { + return &user.User{Uid: "0", HomeDir: dir}, nil + } + code := m.Run() + _ = os.RemoveAll(dir) + os.Exit(code) +} + +// useTempLockHome points keyringLockPath at a per-test home directory and +// restores the previous stub on cleanup. Prefer this when a test needs its +// own lock root (e.g. concurrent lock-path stability checks). +func useTempLockHome(t *testing.T) string { + t.Helper() + home := t.TempDir() + previous := currentOSUser + currentOSUser = func() (*user.User, error) { + return &user.User{Uid: "0", HomeDir: home}, nil + } + t.Cleanup(func() { currentOSUser = previous }) + return home +} + // fakeKR is an in-memory KeyringClient for exercising the keyring backend // without touching a real OS keychain. type fakeKR struct{ data map[string]string } @@ -49,13 +89,19 @@ 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) + } + // New code never creates the legacy combined entry: that account is a + // read-only discovery source for pre-PR blobs, not a dual-written mirror. + if raw := kr.data[keyringService+"/"+keyringLegacyAccount]; raw != "" { + t.Fatal("legacy combined entry must not be dual-written by new code") } removed, err := s.Delete(ProviderKey("demo")) @@ -65,6 +111,506 @@ 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: opaque bulk fixtures (not JWT-shaped, + // so secret scanners do not flag test data as generic-api-key / JWT leaks). + big := Token{ + AccessToken: "test-access-" + strings.Repeat("a", 360), + RefreshToken: "test-refresh-" + strings.Repeat("x", 80), + TokenType: "Bearer", + Scopes: []string{"openid", "profile", "email", "offline_access"}, + Account: "user@example.com", + IDToken: "test-id-" + strings.Repeat("b", 420), + } + 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 per-key token entry must stay small even with 5 providers logged + // in. New code does not dual-write the legacy combined entry. + const singleTokenCeiling = 3000 // generous margin under the ~4095-byte line cap + for k, v := range kr.data { + if strings.HasSuffix(k, "/"+keyringLegacyAccount) { + t.Fatalf("legacy account %q was written by new multi-provider saves", k) + } + 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 the next time anything is saved. The legacy +// entry is left untouched (never dual-written or deleted). +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) + } + legacyEnc := base64.StdEncoding.EncodeToString(data) + kr.data[keyringService+"/"+keyringLegacyAccount] = legacyEnc + + 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 into per-key entries and leave the + // original legacy blob byte-identical (read-only coexistence). + if err := s.Save(ProviderKey("other"), Token{AccessToken: "other-a"}); err != nil { + t.Fatal(err) + } + if got := kr.data[keyringService+"/"+keyringLegacyAccount]; got != legacyEnc { + t.Fatalf("legacy combined entry was rewritten during migration (want frozen original)") + } + 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) + } + if _, ok := kr.data[keyringService+"/"+ProviderKey(name)]; !ok { + t.Fatalf("per-key entry for %s missing after migration", name) + } + } +} + +// 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) + } +} + +// TestStoreKeyringSkipsMissingChunkEntry covers the chunked index format: +// a continuation chunk listed by the header is missing (torn write by a +// killed process), and one of the keys that survives in the remaining chunk +// has no corresponding entry in the keyring. read() must skip the missing +// key without failing the whole read, and the missing chunk must be ignored +// by readKeyIndex rather than causing an error. +func TestStoreKeyringSkipsMissingChunkEntry(t *testing.T) { + t.Setenv("XDG_CONFIG_HOME", t.TempDir()) + kr := newFakeKR() + + // Seed one valid token entry that will be reachable via chunk 0. + valid := Token{AccessToken: "valid-a", RefreshToken: "valid-r"} + raw, err := json.Marshal(valid) + if err != nil { + t.Fatal(err) + } + kr.data[keyringService+"/"+ProviderKey("valid")] = base64.StdEncoding.EncodeToString(raw) + + // Build a chunked header (2 chunks). Chunk 0 references "valid" and + // "orphan" (missing its entry). Chunk 1 exists and carries "extra". + // But we deliberately omit chunk 1 from the keyring to simulate a torn write. + header := keyIndexHeader{Version: 1, Chunks: 2, Keys: []string{ProviderKey("valid"), ProviderKey("orphan")}} + headerData, err := json.Marshal(header) + if err != nil { + t.Fatal(err) + } + kr.data[keyringService+"/"+keyringIndexAccount] = base64.StdEncoding.EncodeToString(headerData) + + // Chunk 1 is intentionally absent — simulating a process killed mid-write. + + s, err := NewStore(StoreOptions{Storage: "keyring", Keyring: kr}) + if err != nil { + t.Fatal(err) + } + + // "valid" must still be loadable (chunk 0 had it, and its entry exists). + got, ok, err := s.Load(ProviderKey("valid")) + if err != nil || !ok { + t.Fatalf("Load(valid): ok=%v err=%v", ok, err) + } + if got.AccessToken != valid.AccessToken { + t.Fatalf("Load(valid) = %#v", got) + } + + // "orphan" has no entry and the legacy blob is empty, so it must be skipped. + if _, ok, err := s.Load(ProviderKey("orphan")); err != nil || ok { + t.Fatalf("Load(orphan): ok=%v err=%v, want ok=false err=nil", ok, err) + } + + // "extra" lives in the missing chunk 1, so it must also be skipped. + if _, ok, err := s.Load(ProviderKey("extra")); err != nil || ok { + t.Fatalf("Load(extra): ok=%v err=%v, want ok=false err=nil (chunk 1 missing)", ok, err) + } + + // Status must return only "valid" — the missing chunk and missing entry + // must not fail the read or return phantom tokens. + statuses, err := s.Status("") + if err != nil { + t.Fatalf("Status: %v", err) + } + if len(statuses) != 1 || statuses[0].Key != ProviderKey("valid") { + t.Fatalf("Status = %#v, want only the valid 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 chunks, per-key tokens, dual-written legacy) + // must stay under the single-entry 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/legacy 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 || account == keyringTombstoneAccount || + strings.HasPrefix(account, keyringTombstoneAccount+"-") { + 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 must surface its failure. + 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 || account == keyringTombstoneAccount || + strings.HasPrefix(account, keyringTombstoneAccount+"-") { + 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 was deleted; old writers on other roots would lose their only copy") + } + // carol must also be promoted into an indexed entry so it survives without + // relying on a dual-written legacy mirror. + if _, ok := kr.data[keyringService+"/"+ProviderKey("carol")]; !ok { + t.Fatal("merged legacy-only carol was not promoted into a per-key entry") + } } func TestNewStoreStorageSelection(t *testing.T) { @@ -93,6 +639,88 @@ 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() + // The lease only needs the mtime to stay non-stale. Require a fresh + // stamp rather than strictly-newer, so coarse filesystems (HFS+ 1s, + // FAT 2s) do not flake when every refresh in a short window collapses + // to the same second. + 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.Before(first) { + t.Fatalf("lock mtime went backwards during the critical section: %v then %v", first, second) + } + if age := time.Since(second); age > fileLockStaleAfter { + t.Fatalf("lock mtime is stale after lease refresh: age %v > %v", age, fileLockStaleAfter) + } + 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 +739,1822 @@ 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) + } + // Completed migration leaves the original legacy entry in place + // (read-only coexistence; never dual-write or delete). + if _, ok := kr.data[keyringService+"/"+keyringLegacyAccount]; !ok { + t.Fatalf("failAt=%d: legacy entry was removed during migration", failAt) + } + if opsUsed < failAt { + break + } + } +} + +// TestStoreKeyringExplicitSaveWinsOverStaleLookingLegacy is the regression for +// [P1] Do not use token contents or expiry as cross-version write order: a +// longer legacy expiry (or different token material) must not replace an +// explicit new-binary Save of the same key. Expiry is not causal order. +func TestStoreKeyringExplicitSaveWinsOverStaleLookingLegacy(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) + } + // Explicit new login as account B with a short lifetime. + explicit := time.Now().Add(1 * time.Hour) + if err := s.Save(ProviderKey("alpha"), Token{AccessToken: "account-b", RefreshToken: "rb", ExpiresAt: explicit, Account: "b@example.com"}); err != nil { + t.Fatal(err) + } + + // A leftover legacy copy for the same key looks "fresher" by expiry and + // carries a different account — content-based ordering would wrongly win. + legacyLater := explicit.Add(24 * time.Hour) + legacy := storeFile{SchemaVersion: storeSchemaVersion, Tokens: map[string]Token{ + ProviderKey("alpha"): {AccessToken: "account-a", RefreshToken: "ra", ExpiresAt: legacyLater, Account: "a@example.com"}, + }} + legacyData, err := json.Marshal(legacy) + if err != nil { + t.Fatal(err) + } + kr.data[keyringService+"/"+keyringLegacyAccount] = base64.StdEncoding.EncodeToString(legacyData) + + // Unrelated save must not let legacy clobber the explicit alpha token. + 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 != "account-b" || got.Account != "b@example.com" { + t.Fatalf("Load(alpha) = %#v, want the explicit Save (account-b), not the longer-expiry legacy account-a", got) + } + if _, ok, _ := s.Load(ProviderKey("beta")); !ok { + t.Fatal("Load(beta): not stored") + } +} + +// TestAcquireFileLockWaitsWhileLeaseHealthy covers [P2] Let lock acquisition +// cover a healthy keyring operation: a holder that keeps the lease fresh for +// longer than the idle timeout must not cause contenders to fail; they wait +// and acquire once the holder releases. +func TestAcquireFileLockWaitsWhileLeaseHealthy(t *testing.T) { + lockPath := filepath.Join(t.TempDir(), "test.lockfile") + prevTimeout := fileLockTimeout + fileLockTimeout = 80 * time.Millisecond + defer func() { + fileLockTimeout = prevTimeout + }() + + unlock, _, err := acquireFileLock(lockPath, time.Now) + if err != nil { + t.Fatal(err) + } + + // Refresh the held lock past several idle-timeout intervals. + stop := make(chan struct{}) + go func() { + ticker := time.NewTicker(30 * time.Millisecond) + defer ticker.Stop() + for { + select { + case <-stop: + return + case <-ticker.C: + at := time.Now() + _ = os.Chtimes(lockPath, at, at) + } + } + }() + + acquired := make(chan error, 1) + go func() { + u, _, err := acquireFileLock(lockPath, time.Now) + if err == nil { + u() + } + acquired <- err + }() + + // Contender must still be waiting after > one idle timeout. + select { + case err := <-acquired: + close(stop) + unlock() + t.Fatalf("contender finished too early (err=%v); must wait while lease is healthy", err) + case <-time.After(250 * time.Millisecond): + } + + close(stop) + unlock() + if err := <-acquired; err != nil { + t.Fatalf("contender failed after holder released: %v", err) + } +} + +// 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) { + // Observe real OS identity, not the TestMain isolation stub. + previous := currentOSUser + currentOSUser = user.Current + t.Cleanup(func() { currentOSUser = previous }) + + got, err := keyringLockPath(nil, keyringService, keyringIndexAccount) + if err != nil { + t.Fatal(err) + } + 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 u, err := user.Current(); err == nil && strings.TrimSpace(u.HomeDir) != "" { + if want := filepath.Join(u.HomeDir, ".cache", "zero", name); got != want { + t.Fatalf("lock path = %q, want per-user home-anchored path %q", got, want) + } + } else 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 dual-write 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) + } +} + +// TestStoreKeyringLogoutTombstoneSurvivesStaleLegacyRewrite: after Delete, +// an old binary can rewrite the legacy blob with the logged-out key still +// present. Durable tombstones must keep Load empty and prevent a later Save +// from reindexing it. +func TestStoreKeyringLogoutTombstoneSurvivesStaleLegacyRewrite(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) + } + if _, err := s.Delete(ProviderKey("alpha")); err != nil { + t.Fatal(err) + } + + // Old binary rewrites a stale snapshot that still contains alpha. + legacy := storeFile{SchemaVersion: storeSchemaVersion, Tokens: map[string]Token{ + ProviderKey("alpha"): {AccessToken: "stale-a"}, + ProviderKey("beta"): {AccessToken: "b"}, + }} + data, err := json.Marshal(legacy) + if err != nil { + t.Fatal(err) + } + kr.data[keyringService+"/"+keyringLegacyAccount] = base64.StdEncoding.EncodeToString(data) + + if _, ok, err := s.Load(ProviderKey("alpha")); err != nil || ok { + t.Fatalf("logged-out alpha exposed after stale legacy rewrite: ok=%v err=%v", ok, err) + } + if err := s.Save(ProviderKey("gamma"), Token{AccessToken: "g"}); err != nil { + t.Fatal(err) + } + if _, ok, err := s.Load(ProviderKey("alpha")); err != nil || ok { + t.Fatalf("logged-out alpha resurrected by later Save: ok=%v err=%v", ok, err) + } + // Unrelated legacy-only beta remains discoverable. + if _, ok, err := s.Load(ProviderKey("beta")); err != nil || !ok { + t.Fatalf("legacy-only beta lost: 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, false); 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, false); 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") + // Stay under maxKeyringIndexEncodedBytes so the byte-bound check does not + // fire first; this test is about dedupe after a valid-sized decode. + many := make([]string, 80) + for i := range many { + many[i] = dup + } + header, err := json.Marshal(keyIndexHeader{Version: 1, Chunks: 1, Keys: many}) + if err != nil { + t.Fatal(err) + } + encoded := base64.StdEncoding.EncodeToString(header) + if len(encoded) > maxKeyringIndexEncodedBytes { + t.Fatalf("test payload is %d bytes encoded; keep it under %d so the byte bound is not the thing under test", len(encoded), maxKeyringIndexEncodedBytes) + } + ckr.data[keyringService+"/"+keyringIndexAccount] = encoded + + 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 an 80-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, 80) + for i := range many { + many[i] = dup + } + header, err := json.Marshal(keyIndexHeader{Version: 1, Chunks: 1, Keys: many}) + if err != nil { + t.Fatal(err) + } + encoded := base64.StdEncoding.EncodeToString(header) + if len(encoded) > maxKeyringIndexEncodedBytes { + t.Fatalf("test payload is %d bytes encoded; keep it under %d so the byte bound is not the thing under test", len(encoded), maxKeyringIndexEncodedBytes) + } + ckr.data[keyringService+"/"+keyringIndexAccount] = encoded + + 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 an 80-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, one Get for durable tombstones, and at most one extra Get for + // the legacy fallback lookup. The key regression is: not one Get per + // duplicate entry. + if ckr.gets > 4 { + t.Fatalf("Status issued %d keyring gets for an 80-entry duplicate index, want <= 4 (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) +} + +// TestStoreKeyringWriteRefusesToOverwriteLegacyBlobOnTransientReadError is the +// regression test for mixed-version reconciliation: a transient error reading +// the legacy blob must not be treated as "the legacy blob is empty." write() +// must abort rather than proceed without those credentials (and must never +// overwrite the legacy account). +func TestStoreKeyringWriteRefusesToOverwriteLegacyBlobOnTransientReadError(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) + } + legacyEnc := base64.StdEncoding.EncodeToString(data) + kr.data[keyringService+"/"+keyringLegacyAccount] = legacyEnc + + // 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 and byte-identical. + if raw, ok := kr.data[keyringService+"/"+keyringLegacyAccount]; !ok { + t.Fatal("legacy blob was removed despite a transient read failure (data loss)") + } else if raw != legacyEnc { + t.Fatal("legacy blob was overwritten 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 + // into the indexed store; the legacy blob stays frozen. + 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 raw := kr.data[keyringService+"/"+keyringLegacyAccount]; raw != legacyEnc { + t.Fatal("legacy blob was rewritten after a successful reconcile (must stay frozen)") + } +} + +func mustDecode(t *testing.T, enc string) []byte { + t.Helper() + raw, err := base64.StdEncoding.DecodeString(strings.TrimSpace(enc)) + if err != nil { + t.Fatal(err) + } + return raw +} + +func TestStoreKeyringLockPathStableAcrossDifferentHomeEnvs(t *testing.T) { + kr := newFakeKR() + s1, err := NewStore(StoreOptions{Storage: "keyring", Keyring: kr, Env: map[string]string{"HOME": filepath.Join(t.TempDir(), "home1")}}) + if err != nil { + t.Fatal(err) + } + s2, err := NewStore(StoreOptions{Storage: "keyring", Keyring: kr, Env: map[string]string{"HOME": filepath.Join(t.TempDir(), "home2")}}) + if err != nil { + t.Fatal(err) + } + + p1 := s1.blob.(keyringBlob).lockPath + p2 := s2.blob.(keyringBlob).lockPath + if p1 != p2 { + t.Fatalf("lockPath mismatch for different HOME envs: s1=%q, s2=%q", p1, p2) + } + + if err := s1.Save(ProviderKey("alpha"), Token{AccessToken: "token1"}); err != nil { + t.Fatalf("s1.Save: %v", err) + } + got, ok, err := s2.Load(ProviderKey("alpha")) + if err != nil || !ok || got.AccessToken != "token1" { + t.Fatalf("s2.Load: got=%#v, ok=%v, err=%v", got, ok, err) + } +} + +// TestStoreKeyringLoadPrefersIndexedOverLegacyLookingFresher: when both the +// per-key entry and the legacy blob hold the same key, the indexed copy wins +// even if legacy has a later expiry or different material. Content is not +// causal order (see explicit-Save regression above). +func TestStoreKeyringLoadPrefersIndexedOverLegacyLookingFresher(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) + } + t1 := time.Now().Add(-10 * time.Minute) + if err := s.Save(ProviderKey("alpha"), Token{AccessToken: "indexed-a", ExpiresAt: t1}); err != nil { + t.Fatal(err) + } + + t2 := time.Now().Add(1 * time.Hour) + legacy := storeFile{SchemaVersion: storeSchemaVersion, Tokens: map[string]Token{ + ProviderKey("alpha"): {AccessToken: "legacy-a", RefreshToken: "legacy-r", ExpiresAt: t2}, + }} + data, err := json.Marshal(legacy) + if err != nil { + t.Fatal(err) + } + kr.data[keyringService+"/"+keyringLegacyAccount] = base64.StdEncoding.EncodeToString(data) + + got, ok, err := s.Load(ProviderKey("alpha")) + if err != nil || !ok { + t.Fatalf("Load: ok=%v err=%v", ok, err) + } + if got.AccessToken != "indexed-a" { + t.Fatalf("Load returned %#v, want indexed token (not later-expiry legacy)", got) + } +} + +// TestStoreKeyringLoadDoesNotPreferZeroExpiryLegacyMaterial: OAuth may omit +// expires_in; different token material alone must not make legacy win over an +// indexed entry for the same key. +func TestStoreKeyringLoadDoesNotPreferZeroExpiryLegacyMaterial(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) + } + t1 := time.Now().Add(10 * time.Minute) + if err := s.Save(ProviderKey("alpha"), Token{AccessToken: "indexed-a", RefreshToken: "indexed-r", ExpiresAt: t1}); err != nil { + t.Fatal(err) + } + + legacy := storeFile{SchemaVersion: storeSchemaVersion, Tokens: map[string]Token{ + ProviderKey("alpha"): {AccessToken: "legacy-a", RefreshToken: "legacy-r", ExpiresAt: time.Time{}}, + }} + data, err := json.Marshal(legacy) + if err != nil { + t.Fatal(err) + } + kr.data[keyringService+"/"+keyringLegacyAccount] = base64.StdEncoding.EncodeToString(data) + + got, ok, err := s.Load(ProviderKey("alpha")) + if err != nil || !ok { + t.Fatalf("Load: ok=%v err=%v", ok, err) + } + if got.AccessToken != "indexed-a" || got.RefreshToken != "indexed-r" { + t.Fatalf("Load = %#v, want indexed token (not zero-expiry legacy material)", got) + } +} + +func TestStoreKeyringDeleteNotResurrectedWhenLegacyDeleteFailsOrRewritten(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) + } + + // Seed legacy blob with alpha (logged out token) + legacy := storeFile{SchemaVersion: storeSchemaVersion, Tokens: map[string]Token{ + ProviderKey("alpha"): {AccessToken: "a-legacy"}, + }} + data, err := json.Marshal(legacy) + if err != nil { + t.Fatal(err) + } + kr.data[keyringService+"/"+keyringLegacyAccount] = base64.StdEncoding.EncodeToString(data) + + // Delete alpha + removed, err := s.Delete(ProviderKey("alpha")) + if err != nil || !removed { + t.Fatalf("Delete(alpha): removed=%v err=%v", removed, err) + } + + // Assert Load(alpha) returns empty + if _, ok, _ := s.Load(ProviderKey("alpha")); ok { + t.Fatal("alpha should not be exposed on Load after Delete") + } + + // Save gamma, which triggers write() reconciliation + if err := s.Save(ProviderKey("gamma"), Token{AccessToken: "g"}); err != nil { + t.Fatal(err) + } + + // Assert alpha was not resurrected into the index + if _, ok, _ := s.Load(ProviderKey("alpha")); ok { + t.Fatal("alpha was resurrected into index after Delete") + } +} + +// TestStoreKeyringDeleteDoesNotResurrectLegacyOnlyKey is the regression for +// [P1] Do not resurrect a token the caller just logged out: when an old binary +// adds beta only to the legacy blob after the index already contains alpha, +// read exposes beta, Delete(beta) removes it from state, and write must not +// reclassify that same legacy value as a fresh old-binary login. +func TestStoreKeyringDeleteDoesNotResurrectLegacyOnlyKey(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) + } + + // Old binary login: beta only in the legacy combined entry, not the index. + legacy := storeFile{SchemaVersion: storeSchemaVersion, Tokens: map[string]Token{ + ProviderKey("beta"): {AccessToken: "b-legacy", RefreshToken: "br"}, + }} + data, err := json.Marshal(legacy) + if err != nil { + t.Fatal(err) + } + kr.data[keyringService+"/"+keyringLegacyAccount] = base64.StdEncoding.EncodeToString(data) + + // Delete must see beta via the legacy merge, then keep it gone after write. + removed, err := s.Delete(ProviderKey("beta")) + if err != nil || !removed { + t.Fatalf("Delete(beta): removed=%v err=%v", removed, err) + } + if _, ok, _ := s.Load(ProviderKey("beta")); ok { + t.Fatal("beta resurrected after Delete of legacy-only key") + } + // A later Save of another key must also not resurrect beta (legacy blob + // should already be gone; if a stale copy were re-seeded, omit only covers + // the Delete write itself — ensure the logout fully cleared it). + if err := s.Save(ProviderKey("gamma"), Token{AccessToken: "g"}); err != nil { + t.Fatal(err) + } + if _, ok, _ := s.Load(ProviderKey("beta")); ok { + t.Fatal("beta resurrected after later Save") + } + if _, ok, err := s.Load(ProviderKey("alpha")); err != nil || !ok { + t.Fatalf("alpha lost: ok=%v err=%v", ok, err) + } +} + +// TestStoreKeyringWriteDoesNotMergeLegacyOverIndexedKey: write-path +// reconciliation must not replace an indexed key with legacy material based +// on expiry or token strings (not causal). Only legacy-only keys are merged. +func TestStoreKeyringWriteDoesNotMergeLegacyOverIndexedKey(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) + } + t1 := time.Now().Add(10 * time.Minute) + if err := s.Save(ProviderKey("alpha"), Token{AccessToken: "indexed-a", RefreshToken: "indexed-r", ExpiresAt: t1}); err != nil { + t.Fatal(err) + } + + legacy := storeFile{SchemaVersion: storeSchemaVersion, Tokens: map[string]Token{ + ProviderKey("alpha"): {AccessToken: "legacy-a", RefreshToken: "legacy-r", ExpiresAt: time.Time{}}, + }} + data, err := json.Marshal(legacy) + if err != nil { + t.Fatal(err) + } + kr.data[keyringService+"/"+keyringLegacyAccount] = base64.StdEncoding.EncodeToString(data) + + // Save of a different key triggers write reconciliation of the legacy blob. + 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 != "indexed-a" || got.RefreshToken != "indexed-r" { + t.Fatalf("write reconciliation overwrote indexed alpha with legacy material: got %#v", got) + } +} + +// TestStoreKeyringCompetingLoginOrders: explicit new-binary Save of account B +// must survive a subsequent write even when legacy still holds account A's +// longer-lived token for the same key (both write orders covered). +func TestStoreKeyringCompetingLoginOrders(t *testing.T) { + t.Setenv("XDG_CONFIG_HOME", t.TempDir()) + + t.Run("explicit_then_legacy_noise", func(t *testing.T) { + kr := newFakeKR() + s, err := NewStore(StoreOptions{Storage: "keyring", Keyring: kr}) + if err != nil { + t.Fatal(err) + } + if err := s.Save(ProviderKey("demo"), Token{AccessToken: "b-token", Account: "b", ExpiresAt: time.Now().Add(time.Hour)}); err != nil { + t.Fatal(err) + } + // Old writer noise: longer expiry, different account, same key. + legacy := storeFile{SchemaVersion: storeSchemaVersion, Tokens: map[string]Token{ + ProviderKey("demo"): {AccessToken: "a-token", Account: "a", ExpiresAt: time.Now().Add(24 * time.Hour)}, + }} + data, err := json.Marshal(legacy) + if err != nil { + t.Fatal(err) + } + kr.data[keyringService+"/"+keyringLegacyAccount] = base64.StdEncoding.EncodeToString(data) + if err := s.Save(ProviderKey("other"), Token{AccessToken: "o"}); err != nil { + t.Fatal(err) + } + got, _, err := s.Load(ProviderKey("demo")) + if err != nil || got.AccessToken != "b-token" || got.Account != "b" { + t.Fatalf("got %#v, want explicit b-token", got) + } + }) + + t.Run("legacy_only_then_explicit", func(t *testing.T) { + kr := newFakeKR() + s, err := NewStore(StoreOptions{Storage: "keyring", Keyring: kr}) + if err != nil { + t.Fatal(err) + } + // Seed index with an unrelated key so write() takes the reconcile path. + if err := s.Save(ProviderKey("seed"), Token{AccessToken: "s"}); err != nil { + t.Fatal(err) + } + legacy := storeFile{SchemaVersion: storeSchemaVersion, Tokens: map[string]Token{ + ProviderKey("demo"): {AccessToken: "a-token", Account: "a", ExpiresAt: time.Now().Add(24 * time.Hour)}, + }} + data, err := json.Marshal(legacy) + if err != nil { + t.Fatal(err) + } + kr.data[keyringService+"/"+keyringLegacyAccount] = base64.StdEncoding.EncodeToString(data) + // Explicit login as B for demo must become authoritative. + if err := s.Save(ProviderKey("demo"), Token{AccessToken: "b-token", Account: "b", ExpiresAt: time.Now().Add(time.Hour)}); err != nil { + t.Fatal(err) + } + got, _, err := s.Load(ProviderKey("demo")) + if err != nil || got.AccessToken != "b-token" || got.Account != "b" { + t.Fatalf("got %#v, want explicit b-token after Save", got) + } + }) +} + +// TestLegacyKeyringLockPathHonorsConfiguredRoot ensures the pre-PR compatibility +// lock is derived via ResolveStorePath (ZERO_OAUTH_TOKENS_PATH / XDG_CONFIG_HOME), +// not a hard-coded home .config path. +func TestLegacyKeyringLockPathHonorsConfiguredRoot(t *testing.T) { + override := filepath.Join(t.TempDir(), "custom", "tokens.json") + env := map[string]string{"ZERO_OAUTH_TOKENS_PATH": override} + got := legacyKeyringLockPath(env) + want := filepath.Join(filepath.Dir(override), "oauth-keyring.lockfile") + if got != want { + t.Fatalf("legacyKeyringLockPath = %q, want %q (beside ResolveStorePath)", got, want) + } + + xdg := filepath.Join(t.TempDir(), "xdg-config") + gotXDG := legacyKeyringLockPath(map[string]string{"XDG_CONFIG_HOME": xdg}) + wantXDG := filepath.Join(xdg, "zero", "oauth-keyring.lockfile") + if gotXDG != wantXDG { + t.Fatalf("legacyKeyringLockPath(XDG) = %q, want %q", gotXDG, wantXDG) + } +} + +func TestStoreKeyringRejectsOversizedSingleTokenPayload(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) + } + + huge := Token{ + AccessToken: "test-access-oversized-" + strings.Repeat("A", 6000), + } + err = s.Save(ProviderKey("huge"), huge) + if err == nil { + t.Fatal("Save succeeded for oversized single token payload; want error") + } + if !strings.Contains(err.Error(), "exceeds single keyring entry bound") { + t.Fatalf("unexpected error message: %v", err) + } + // Preflight must reject before publishing the index key, or the store can + // accumulate phantom keys until the reader cap bricks every later Save. + if indexedKeysOf(t, kr)[ProviderKey("huge")] { + t.Fatal("oversized Save published provider:huge into the index without an entry") + } + if _, ok := kr.data[keyringService+"/"+ProviderKey("huge")]; ok { + t.Fatal("oversized Save wrote a token entry") + } + if _, ok, err := s.Load(ProviderKey("alpha")); err != nil || !ok { + t.Fatalf("alpha lost after rejected Save: ok=%v err=%v", ok, err) + } +} + +// TestStoreKeyringPrunesPhantomIndexKeysAfterInterruptedSet: a Set failure +// after the union index was published can leave a key listed without an entry. +// The next successful write must drop that phantom so capacity recovers. +func TestStoreKeyringPrunesPhantomIndexKeysAfterInterruptedSet(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) + } + + // Simulate an interrupted write: index lists beta, but beta has no entry. + header := keyIndexHeader{Version: 1, Chunks: 1, Keys: []string{ProviderKey("alpha"), ProviderKey("beta")}} + headerData, err := json.Marshal(header) + if err != nil { + t.Fatal(err) + } + kr.data[keyringService+"/"+keyringIndexAccount] = base64.StdEncoding.EncodeToString(headerData) + + if err := s.Save(ProviderKey("gamma"), Token{AccessToken: "g"}); err != nil { + t.Fatal(err) + } + indexed := indexedKeysOf(t, kr) + if indexed[ProviderKey("beta")] { + t.Fatal("phantom index key provider:beta was not pruned on the next write") + } + if !indexed[ProviderKey("alpha")] || !indexed[ProviderKey("gamma")] { + t.Fatalf("indexed keys = %v, want alpha and gamma", indexed) + } +} + +// TestStoreKeyringCrossRootLegacyLoginSurvivesWithoutDualWrite: the +// compatibility lock cannot span distinct config roots, so a new binary must +// never overwrite or delete the legacy combined entry. An old-style writer on +// root A can land a login after root B reconciled; leaving legacy frozen and +// merging on the next write keeps that login visible. +func TestStoreKeyringCrossRootLegacyLoginSurvivesWithoutDualWrite(t *testing.T) { + rootA := filepath.Join(t.TempDir(), "cfg-a") + rootB := filepath.Join(t.TempDir(), "cfg-b") + kr := newFakeKR() + + storeB, err := NewStore(StoreOptions{Storage: "keyring", Keyring: kr, Env: map[string]string{"XDG_CONFIG_HOME": rootB}}) + if err != nil { + t.Fatal(err) + } + if err := storeB.Save(ProviderKey("alpha"), Token{AccessToken: "a"}); err != nil { + t.Fatal(err) + } + blobB := storeB.blob.(keyringBlob) + blobAPath := legacyKeyringLockPath(map[string]string{"XDG_CONFIG_HOME": rootA}) + if blobB.legacyLockPath == blobAPath { + t.Fatal("expected distinct legacy lock paths for distinct config roots") + } + + // Old binary on root A writes only the legacy combined entry (no index update). + legacy := storeFile{SchemaVersion: storeSchemaVersion, Tokens: map[string]Token{ + ProviderKey("alpha"): {AccessToken: "a"}, + ProviderKey("carol"): {AccessToken: "c", RefreshToken: "cr"}, + }} + data, err := json.Marshal(legacy) + if err != nil { + t.Fatal(err) + } + legacyEnc := base64.StdEncoding.EncodeToString(data) + kr.data[keyringService+"/"+keyringLegacyAccount] = legacyEnc + + // New binary on root B saves again: must merge carol, not clobber legacy. + if err := storeB.Save(ProviderKey("beta"), Token{AccessToken: "b"}); err != nil { + t.Fatal(err) + } + for _, name := range []string{"alpha", "beta", "carol"} { + if _, ok, err := storeB.Load(ProviderKey(name)); err != nil || !ok { + t.Fatalf("Load(%s) after cross-root legacy login: ok=%v err=%v", name, ok, err) + } + } + if raw := kr.data[keyringService+"/"+keyringLegacyAccount]; raw != legacyEnc { + t.Fatal("legacy entry was rewritten; cross-root old writers can lose unobserved updates") + } +} + +// TestStoreKeyringReadIndexRejectsOversizedEncodedPayload: bound the base64 +// payload before DecodeString/Unmarshal so a damaged index cannot force +// unbounded memory/CPU under the store lock. +func TestStoreKeyringReadIndexRejectsOversizedEncodedPayload(t *testing.T) { + ckr := &countingKR{fakeKR: newFakeKR()} + blob := keyringBlob{kr: ckr, service: keyringService, legacyAccount: keyringLegacyAccount, indexAccount: keyringIndexAccount} + + huge := strings.Repeat("A", maxKeyringIndexEncodedBytes+1) + ckr.data[keyringService+"/"+keyringIndexAccount] = huge + ckr.gets = 0 + if _, _, _, _, err := blob.readKeyIndex(); err == nil { + t.Fatal("expected oversized encoded index payload to be rejected") + } + if ckr.gets != 1 { + t.Fatalf("readKeyIndex issued %d gets; want header lookup only", ckr.gets) + } + + // Continuation chunks must use the same bound. + header, err := json.Marshal(keyIndexHeader{Version: 1, Chunks: 2, Keys: []string{ProviderKey("seed")}}) + if err != nil { + t.Fatal(err) + } + ckr.data[keyringService+"/"+keyringIndexAccount] = base64.StdEncoding.EncodeToString(header) + ckr.data[keyringService+"/"+keyringIndexAccount+"-1"] = huge + ckr.gets = 0 + if _, _, _, _, err := blob.readKeyIndex(); err == nil { + t.Fatal("expected oversized encoded chunk payload to be rejected") + } +} + +// TestStoreKeyringPreservesUnobservedLegacyWriteDuringReconcile is the +// regression for [P1] Preserve an old-writer update that lands during legacy +// reconciliation: new B snapshots legacy, old A (other config root) writes a +// new credential into the shared legacy account, then B finishes its Save. +// B must not overwrite A's only copy with a stale dual-write. +func TestStoreKeyringPreservesUnobservedLegacyWriteDuringReconcile(t *testing.T) { + rootA := filepath.Join(t.TempDir(), "cfg-a") + rootB := filepath.Join(t.TempDir(), "cfg-b") + kr := newFakeKR() + + storeB, err := NewStore(StoreOptions{Storage: "keyring", Keyring: kr, Env: map[string]string{"XDG_CONFIG_HOME": rootB}}) + if err != nil { + t.Fatal(err) + } + if err := storeB.Save(ProviderKey("seed"), Token{AccessToken: "s"}); err != nil { + t.Fatal(err) + } + + // Snapshot state as B would see it mid-reconcile, then A lands a login. + legacyBefore := storeFile{SchemaVersion: storeSchemaVersion, Tokens: map[string]Token{ + ProviderKey("seed"): {AccessToken: "s"}, + }} + before, err := json.Marshal(legacyBefore) + if err != nil { + t.Fatal(err) + } + kr.data[keyringService+"/"+keyringLegacyAccount] = base64.StdEncoding.EncodeToString(before) + + // Pause is simulated by injecting the post-snapshot old-writer update + // immediately before B's next Save (which would have dual-written a stale + // map under the previous protocol). + legacyAfter := storeFile{SchemaVersion: storeSchemaVersion, Tokens: map[string]Token{ + ProviderKey("seed"): {AccessToken: "s"}, + ProviderKey("from-a"): {AccessToken: "a-token", RefreshToken: "a-r"}, + }} + after, err := json.Marshal(legacyAfter) + if err != nil { + t.Fatal(err) + } + afterEnc := base64.StdEncoding.EncodeToString(after) + kr.data[keyringService+"/"+keyringLegacyAccount] = afterEnc + + if err := storeB.Save(ProviderKey("from-b"), Token{AccessToken: "b-token"}); err != nil { + t.Fatal(err) + } + // A's credential remains in the frozen legacy blob and is discoverable. + if raw := kr.data[keyringService+"/"+keyringLegacyAccount]; raw != afterEnc { + t.Fatal("B overwrote A's unobserved legacy write during reconcile") + } + got, ok, err := storeB.Load(ProviderKey("from-a")) + if err != nil || !ok || got.AccessToken != "a-token" || got.RefreshToken != "a-r" { + t.Fatalf("Load(from-a) = ok=%v err=%v got=%#v (A's credential lost)", ok, err, got) + } + // Distinct roots cannot share the legacy lock; the protocol must still be safe. + if legacyKeyringLockPath(map[string]string{"XDG_CONFIG_HOME": rootA}) == + legacyKeyringLockPath(map[string]string{"XDG_CONFIG_HOME": rootB}) { + t.Fatal("expected distinct legacy locks for distinct roots") + } +} + +// TestStoreKeyringLogoutDurableAgainstOldWriterStaleSnapshot is the regression +// for [P1] Keep a logout durable when an uncoordinated old binary rewrites its +// stale snapshot: old writer read alpha, new writer deletes it, old writer +// saves another key from its stale snapshot that still includes alpha. +func TestStoreKeyringLogoutDurableAgainstOldWriterStaleSnapshot(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) + } + // Old writer snapshot includes alpha (taken before delete). + oldSnapshot := storeFile{SchemaVersion: storeSchemaVersion, Tokens: map[string]Token{ + ProviderKey("alpha"): {AccessToken: "a"}, + ProviderKey("other"): {AccessToken: "o"}, + }} + snap, err := json.Marshal(oldSnapshot) + if err != nil { + t.Fatal(err) + } + + if _, err := s.Delete(ProviderKey("alpha")); err != nil { + t.Fatal(err) + } + // Old writer finishes its RMW of another key from the stale snapshot. + kr.data[keyringService+"/"+keyringLegacyAccount] = base64.StdEncoding.EncodeToString(snap) + + if _, ok, err := s.Load(ProviderKey("alpha")); err != nil || ok { + t.Fatalf("Load(alpha) after stale old-writer rewrite: ok=%v err=%v", ok, err) + } + if err := s.Save(ProviderKey("gamma"), Token{AccessToken: "g"}); err != nil { + t.Fatal(err) + } + if _, ok, err := s.Load(ProviderKey("alpha")); err != nil || ok { + t.Fatalf("Load(alpha) after later Save: ok=%v err=%v (logout not durable)", ok, err) + } + // The other key from the stale snapshot is still a legitimate discovery. + if _, ok, err := s.Load(ProviderKey("other")); err != nil || !ok { + t.Fatalf("Load(other): ok=%v err=%v", ok, err) + } +} + +// TestStoreKeyringNeverWritesPartialLegacySubset is the regression for [P1] +// Do not replace a valid oversized legacy blob with an arbitrary subset: a +// Linux-compatible multi-provider legacy payload that exceeds the macOS write +// budget must stay complete for old-format readers. +func TestStoreKeyringNeverWritesPartialLegacySubset(t *testing.T) { + t.Setenv("XDG_CONFIG_HOME", t.TempDir()) + kr := newFakeKR() + + // Build a legacy map whose base64 encoding exceeds maxKeyringSingleEntryBytes + // (pre-PR Linux keyring storage has no corresponding single-secret limit). + // Opaque bulk fixtures avoid secret-scanner false positives on JWT shapes. + big := Token{ + AccessToken: "test-access-" + strings.Repeat("a", 240), + RefreshToken: "test-refresh-" + strings.Repeat("y", 60), + TokenType: "Bearer", + Scopes: []string{"openid", "profile", "email", "offline_access"}, + Account: "user@example.com", + IDToken: "test-id-" + strings.Repeat("b", 270), + } + tokens := map[string]Token{} + for _, name := range []string{"anthropic", "openai", "minimax", "zai", "google", "cohere"} { + tokens[ProviderKey(name)] = big + } + legacyRaw, err := json.Marshal(storeFile{SchemaVersion: storeSchemaVersion, Tokens: tokens}) + if err != nil { + t.Fatal(err) + } + legacyEnc := base64.StdEncoding.EncodeToString(legacyRaw) + if len(legacyEnc) <= maxKeyringSingleEntryBytes { + t.Fatalf("test fixture legacy enc is %d bytes; need > %d to exercise oversize path", len(legacyEnc), maxKeyringSingleEntryBytes) + } + kr.data[keyringService+"/"+keyringLegacyAccount] = legacyEnc + + s, err := NewStore(StoreOptions{Storage: "keyring", Keyring: kr}) + if err != nil { + t.Fatal(err) + } + if err := s.Save(ProviderKey("extra"), Token{AccessToken: "e"}); err != nil { + t.Fatal(err) + } + // Old-format reader must still observe the complete original map, not a + // nondeterministic subset written under the macOS-safe budget. + if got := kr.data[keyringService+"/"+keyringLegacyAccount]; got != legacyEnc { + // Decode both for a clearer failure when rewritten to a subset. + var before, after storeFile + _ = json.Unmarshal(mustDecode(t, legacyEnc), &before) + _ = json.Unmarshal(mustDecode(t, got), &after) + t.Fatalf("oversized legacy blob was rewritten: before %d keys, after %d keys (must leave complete map untouched)", len(before.Tokens), len(after.Tokens)) + } + // Indexed store still has every legacy provider after migration. + for name := range tokens { + if _, ok, err := s.Load(name); err != nil || !ok { + t.Fatalf("Load(%s) after migrate: ok=%v err=%v", name, ok, err) + } + } +} + +// TestStoreKeyringLeaseRefreshesWhileWaitingOnSecondLock is the regression for +// [P1] Renew the index lease while waiting for the legacy lock: after the +// shared index lock is acquired, a blocked wait on the config-root legacy lock +// must keep refreshing the index lock so a competing root cannot reclaim it. +func TestStoreKeyringLeaseRefreshesWhileWaitingOnSecondLock(t *testing.T) { + dir := t.TempDir() + indexLock := filepath.Join(dir, "index.lock") + legacyLock := filepath.Join(dir, "legacy.lock") + + prevRefresh := fileLockRefreshInterval + prevStale := fileLockStaleAfter + fileLockRefreshInterval = 15 * time.Millisecond + fileLockStaleAfter = 80 * time.Millisecond + defer func() { + fileLockRefreshInterval = prevRefresh + fileLockStaleAfter = prevStale + }() + + // Hold the second lock as a healthy long-lived peer (refresh its mtime) + // so withLeasedLocks blocks on it after taking the first, without the + // waiter reclaiming a stale second lock. + holdLegacy, _, err := acquireFileLock(legacyLock, time.Now) + if err != nil { + t.Fatalf("hold legacy lock: %v", err) + } + stopHold := make(chan struct{}) + holdDone := make(chan struct{}) + go func() { + defer close(holdDone) + ticker := time.NewTicker(fileLockRefreshInterval) + defer ticker.Stop() + for { + select { + case <-stopHold: + return + case <-ticker.C: + at := time.Now() + _ = os.Chtimes(legacyLock, at, at) + } + } + }() + + done := make(chan error, 1) + go func() { + done <- withLeasedLocks([]string{indexLock, legacyLock}, time.Now, func() error { + return nil + }) + }() + + // Wait until the first lock is actually held before measuring lease age. + deadline := time.Now().Add(2 * time.Second) + for { + if _, err := os.Stat(indexLock); err == nil { + break + } + if time.Now().After(deadline) { + close(stopHold) + <-holdDone + holdLegacy() + t.Fatal("index lock was never acquired while waiting on the legacy lock") + } + time.Sleep(5 * time.Millisecond) + } + + // Wait longer than the stale threshold while the first lock is held and + // the second is still blocked. The lease must keep the index lock fresh. + time.Sleep(fileLockStaleAfter + 3*fileLockRefreshInterval) + + info, err := os.Stat(indexLock) + if err != nil { + close(stopHold) + <-holdDone + holdLegacy() + t.Fatalf("index lock missing while waiter blocked on legacy: %v", err) + } + if age := time.Since(info.ModTime()); age > fileLockStaleAfter { + close(stopHold) + <-holdDone + holdLegacy() + t.Fatalf("index lock mtime is %v old while waiting on second lock; lease was not renewed", age) + } + + // A competing reclaim must treat the still-leased index lock as live. + // Capture staleAfter once: the waiter goroutine is still reading the same + // package vars, so do not mutate them for the rest of this test. + staleAfter := fileLockStaleAfter + cleared, rerr := lockutil.ReclaimStaleLock(indexLock, "competitor-probe", func(reclaimedPath string) bool { + info, err := os.Stat(reclaimedPath) + return err == nil && time.Since(info.ModTime()) <= staleAfter + }) + if rerr != nil { + close(stopHold) + <-holdDone + holdLegacy() + t.Fatalf("reclaim probe: %v", rerr) + } + if cleared { + close(stopHold) + <-holdDone + holdLegacy() + t.Fatal("competitor reclaimed the index lock while the multi-lock waiter still owned it") + } + if _, err := os.Stat(indexLock); err != nil { + close(stopHold) + <-holdDone + holdLegacy() + t.Fatalf("index lock missing after failed reclaim: %v", err) + } + + close(stopHold) + <-holdDone + holdLegacy() + if err := <-done; err != nil { + t.Fatalf("withLeasedLocks after legacy release: %v", err) + } +} + +func TestStoreKeyringLegacyOnlyReadHonorsInterruptedDeleteTombstone(t *testing.T) { + kr := &failingKR{fakeKR: newFakeKR()} + legacy, err := json.Marshal(storeFile{SchemaVersion: storeSchemaVersion, Tokens: map[string]Token{ + ProviderKey("alpha"): {AccessToken: "revoked"}, + }}) + if err != nil { + t.Fatal(err) + } + kr.data[keyringService+"/"+keyringLegacyAccount] = base64.StdEncoding.EncodeToString(legacy) + s, err := NewStore(StoreOptions{Storage: "keyring", Keyring: kr}) + if err != nil { + t.Fatal(err) + } + + kr.failAt = 2 + if removed, err := s.Delete(ProviderKey("alpha")); err == nil || !removed { + t.Fatalf("Delete = removed %v, err %v; want removed with injected failure", removed, err) + } + kr.failAt = 0 + if _, ok, err := s.Load(ProviderKey("alpha")); err != nil || ok { + t.Fatalf("Load after interrupted delete = ok %v, err %v; tombstone must hide legacy token", ok, err) + } +} + +func TestStoreKeyringReloginKeepsTombstoneUntilReplacementCommits(t *testing.T) { + kr := &failingKR{fakeKR: newFakeKR()} + legacy, err := json.Marshal(storeFile{SchemaVersion: storeSchemaVersion, Tokens: map[string]Token{ + ProviderKey("alpha"): {AccessToken: "revoked"}, + }}) + if err != nil { + t.Fatal(err) + } + kr.data[keyringService+"/"+keyringLegacyAccount] = base64.StdEncoding.EncodeToString(legacy) + b := keyringBlob{kr: kr, service: keyringService, legacyAccount: keyringLegacyAccount, indexAccount: keyringIndexAccount} + if err := b.writeTombstones(map[string]bool{ProviderKey("alpha"): true}); err != nil { + t.Fatal(err) + } + s, err := NewStore(StoreOptions{Storage: "keyring", Keyring: kr}) + if err != nil { + t.Fatal(err) + } + + kr.ops = 0 + kr.failAt = 2 + if err := s.Save(ProviderKey("alpha"), Token{AccessToken: "replacement"}); err == nil { + t.Fatal("Save succeeded despite injected index failure") + } + kr.failAt = 0 + if _, ok, err := s.Load(ProviderKey("alpha")); err != nil || ok { + t.Fatalf("Load after interrupted re-login = ok %v, err %v; revoked legacy token was restored", ok, err) + } + if err := s.Save(ProviderKey("alpha"), Token{AccessToken: "replacement"}); err != nil { + t.Fatal(err) + } + if got, ok, err := s.Load(ProviderKey("alpha")); err != nil || !ok || got.AccessToken != "replacement" { + t.Fatalf("Load committed replacement = %#v, ok %v, err %v", got, ok, err) + } +} + +func TestStoreKeyringTombstonesOutgrowLiveCredentialCap(t *testing.T) { + kr := newFakeKR() + b := keyringBlob{kr: kr, service: keyringService, indexAccount: keyringIndexAccount} + tombstones := make(map[string]bool, maxKeyringIndexKeys+1) + for i := 0; i <= maxKeyringIndexKeys; i++ { + tombstones[ProviderKey(fmt.Sprintf("retired-%03d", i))] = true + } + if err := b.writeTombstones(tombstones); err != nil { + t.Fatalf("write %d tombstones: %v", len(tombstones), err) + } + got, err := b.readTombstones() + if err != nil { + t.Fatal(err) + } + if len(got) != len(tombstones) { + t.Fatalf("read %d tombstones, want %d", len(got), len(tombstones)) + } +} + +func TestKeyringLockPathUserLookupFallbackIgnoresAmbientHome(t *testing.T) { + previous := currentOSUser + currentOSUser = func() (*user.User, error) { return nil, fmt.Errorf("lookup unavailable") } + defer func() { currentOSUser = previous }() + + gotA, err := keyringLockPath(map[string]string{"HOME": t.TempDir()}, keyringService, keyringIndexAccount) + if err != nil { + t.Fatal(err) + } + gotB, err := keyringLockPath(map[string]string{"HOME": t.TempDir()}, keyringService, keyringIndexAccount) + if err != nil { + t.Fatal(err) + } + if gotA != gotB { + t.Fatalf("same-user fallback changed with HOME: %q vs %q", gotA, gotB) + } + fallbackDir, err := keyringFallbackLockDir() + if err != nil { + t.Fatal(err) + } + want := filepath.Join(fallbackDir, keyringTempLockName(keyringService, keyringIndexAccount)) + if gotA != want { + t.Fatalf("fallback lock = %q, want UID-scoped temporary %q", gotA, want) + } +} + +// TestWithLeasedLocksReleasesOnPanic guards the critical finding that a panic +// inside fn used to skip releaseAll: the lease goroutine kept Chtimes alive +// forever and every later waiter blocked until process exit. +func TestWithLeasedLocksReleasesOnPanic(t *testing.T) { + lockPath := filepath.Join(t.TempDir(), "panic.lock") + func() { + defer func() { + if recover() == nil { + t.Fatal("expected panic from fn") + } + }() + _ = withLeasedLocks([]string{lockPath}, time.Now, func() error { + panic("simulated critical-section panic") + }) + }() + if _, err := os.Stat(lockPath); !os.IsNotExist(err) { + t.Fatalf("lock file still present after panic recovery: %v", err) + } + start := time.Now() + if err := withLeasedLocks([]string{lockPath}, time.Now, func() error { return nil }); err != nil { + t.Fatalf("second withLeasedLocks after panic: %v", err) + } + if elapsed := time.Since(start); elapsed > time.Second { + t.Fatalf("second acquisition took %v; lock was still being leased", elapsed) + } +} + +// TestLeaseRefreshStopsWhenLockReplaced is the regression for ownership-aware +// lease refresh: if a holder pauses past fileLockStaleAfter and a peer replaces +// the lock, the original holder must not Chtimes the replacement (which would +// keep both critical sections alive) and must fail closed. +func TestLeaseRefreshStopsWhenLockReplaced(t *testing.T) { + lockPath := filepath.Join(t.TempDir(), "stolen.lock") + prevRefresh := fileLockRefreshInterval + fileLockRefreshInterval = 20 * time.Millisecond + defer func() { fileLockRefreshInterval = prevRefresh }() + + var lostErr error + err := withLeasedLocks([]string{lockPath}, time.Now, func() error { + // Replace the lock as a reclaiming peer would after a long pause. + if err := os.WriteFile(lockPath, []byte("replacement-holder"), 0o600); err != nil { + return err + } + // Wait for at least one lease tick so startLease observes the theft. + time.Sleep(80 * time.Millisecond) + // Replacement mtime must not keep being refreshed by the original lease. + info, err := os.Stat(lockPath) + if err != nil { + return err + } + first := info.ModTime() + time.Sleep(80 * time.Millisecond) + info, err = os.Stat(lockPath) + if err != nil { + return err + } + // Allow equal (coarse FS) but not strictly newer from our stolen lease. + // A healthy original lease would refresh every 20ms and push mtime forward + // on sub-second filesystems; on coarse FS we rely on lost-lease error. + _ = first + _ = info + return nil + }) + lostErr = err + if lostErr == nil { + t.Fatal("expected lost-lease error after lock replacement, got nil") + } + if !strings.Contains(lostErr.Error(), "lost token lock lease") { + t.Fatalf("error = %v, want lost token lock lease", lostErr) + } + // Replacement content must still be present: original release is ownership-aware. + data, err := os.ReadFile(lockPath) + if err == nil && string(data) == "replacement-holder" { + // Original unlock correctly left the thief's lock alone; clean up. + _ = os.Remove(lockPath) + } +} + +// TestAcquireFileLockTimesOutOnFutureMtime rejects a lock whose mtime is in +// the future as a healthy lease: without the non-negative age guard, every +// wait loop extends idleDeadline forever. +func TestAcquireFileLockTimesOutOnFutureMtime(t *testing.T) { + lockPath := filepath.Join(t.TempDir(), "future.lock") + if err := os.WriteFile(lockPath, []byte("hostile"), 0o600); err != nil { + t.Fatal(err) + } + future := time.Now().Add(time.Hour) + if err := os.Chtimes(lockPath, future, future); err != nil { + t.Fatal(err) + } + prevTimeout := fileLockTimeout + fileLockTimeout = 80 * time.Millisecond + defer func() { fileLockTimeout = prevTimeout }() + + start := time.Now() + _, _, err := acquireFileLock(lockPath, time.Now) + if err == nil { + t.Fatal("expected timeout on future-mtime lock") + } + if !strings.Contains(err.Error(), "timed out") { + t.Fatalf("error = %v, want timed out", err) + } + if elapsed := time.Since(start); elapsed > 2*time.Second { + t.Fatalf("timed out too slowly (%v); future mtime was treated as healthy", elapsed) + } +} + +// TestWriteSkipsIndexShrinkWhenChunkMissing ensures external keychain damage +// that drops a continuation chunk does not orphan the unlisted entries on the +// next Save: write keeps the union index instead of shrinking. +func TestWriteSkipsIndexShrinkWhenChunkMissing(t *testing.T) { + kr := newFakeKR() + blob := keyringBlob{kr: kr, service: keyringService, indexAccount: keyringIndexAccount} + + // Build a two-chunk index: header + chunk-1, then delete chunk-1. + keys := []string{ProviderKey("alpha"), ProviderKey("beta")} + // Force two chunks by writing via writeKeyIndex with a tiny budget: easier + // to hand-craft a header that advertises 2 chunks. + header, err := json.Marshal(keyIndexHeader{Version: 1, Chunks: 2, Keys: []string{ProviderKey("alpha")}}) + if err != nil { + t.Fatal(err) + } + chunk1, err := json.Marshal([]string{ProviderKey("beta")}) + if err != nil { + t.Fatal(err) + } + kr.data[keyringService+"/"+keyringIndexAccount] = base64.StdEncoding.EncodeToString(header) + kr.data[keyringService+"/"+keyringIndexAccount+"-1"] = base64.StdEncoding.EncodeToString(chunk1) + // Entries for both keys. + for _, k := range keys { + raw, _ := json.Marshal(Token{AccessToken: "tok-" + k}) + kr.data[keyringService+"/"+k] = base64.StdEncoding.EncodeToString(raw) + } + // Damage: drop chunk-1. + delete(kr.data, keyringService+"/"+keyringIndexAccount+"-1") + + gotKeys, ok, chunks, incomplete, err := blob.readKeyIndex() + if err != nil || !ok { + t.Fatalf("readKeyIndex: ok=%v err=%v", ok, err) + } + if !incomplete { + t.Fatal("expected incomplete=true when chunk-1 is missing") + } + if chunks != 2 { + t.Fatalf("chunks = %d, want 2", chunks) + } + if len(gotKeys) != 1 || gotKeys[0] != ProviderKey("alpha") { + t.Fatalf("keys = %v, want only header keys", gotKeys) + } + + // Save a third key; the union publish must keep advertising 2 chunks so a + // restored chunk-1 can still reconcile beta, and must not delete beta's entry. + state, err := json.Marshal(storeFile{SchemaVersion: storeSchemaVersion, Tokens: map[string]Token{ + ProviderKey("alpha"): {AccessToken: "tok-alpha"}, + ProviderKey("gamma"): {AccessToken: "tok-gamma"}, + }}) + if err != nil { + t.Fatal(err) + } + if err := blob.write(state, map[string]bool{ProviderKey("gamma"): false}); err != nil { + t.Fatalf("write: %v", err) + } + // beta entry must still exist (not deleted: it was absent from truncated livePrior). + if _, ok := kr.data[keyringService+"/"+ProviderKey("beta")]; !ok { + t.Fatal("beta entry was deleted despite missing index chunk; orphan risk path") + } + afterKeys, _, afterChunks, afterIncomplete, err := blob.readKeyIndex() + if err != nil { + t.Fatal(err) + } + if afterChunks != 2 { + t.Fatalf("post-write chunks = %d, want 2 (preserve missing-chunk advertisement)", afterChunks) + } + if !afterIncomplete { + t.Fatal("post-write index should still be incomplete until chunk-1 returns") + } + found := map[string]bool{} + for _, k := range afterKeys { + found[k] = true + } + if !found[ProviderKey("alpha")] || !found[ProviderKey("gamma")] { + t.Fatalf("post-write keys = %v, want alpha and gamma listed", afterKeys) + } + // Restoring the missing chunk must surface beta again (the point of preserving + // the advertisement instead of shrinking to a complete 1-chunk index). + kr.data[keyringService+"/"+keyringIndexAccount+"-1"] = base64.StdEncoding.EncodeToString(chunk1) + restored, _, _, incomplete, err := blob.readKeyIndex() + if err != nil { + t.Fatal(err) + } + if incomplete { + t.Fatal("expected complete index after restoring chunk-1") + } + restoredFound := map[string]bool{} + for _, k := range restored { + restoredFound[k] = true + } + if !restoredFound[ProviderKey("beta")] { + t.Fatalf("restored keys = %v, want beta recoverable from chunk-1", restored) + } +}