diff --git a/internal/cli/auth.go b/internal/cli/auth.go index f3ecdcc42..7b484c3e9 100644 --- a/internal/cli/auth.go +++ b/internal/cli/auth.go @@ -74,6 +74,8 @@ func runAuth(args []string, stdout io.Writer, stderr io.Writer, deps appDeps) in return runAuthStatus(args[1:], stdout, stderr, deps) case "refresh": return runAuthRefresh(args[1:], stdout, stderr, deps) + case "reset": + return runAuthReset(args[1:], stdout, stderr, deps) case "openrouter": return runAuthOpenRouter(args[1:], stdout, stderr, deps) case "chatgpt": @@ -321,6 +323,7 @@ func validateAuthFlags(sub string, a authArgs) error { "logout": {"json": true}, "status": {"json": true}, "refresh": {"watch": true}, + "reset": {"json": true}, }[sub] bad := func(name string) error { return fmt.Errorf("zero auth %s does not accept %s", sub, name) } if a.json && !allowed["json"] { @@ -569,6 +572,40 @@ func runAuthRefreshWatch(manager *oauth.Manager, key, provider string, stdout io return exitSuccess } +func runAuthReset(args []string, stdout io.Writer, stderr io.Writer, deps appDeps) int { + parsed, err := parseAuthArgs("reset", args) + if err != nil { + return writeExecUsageError(stderr, err.Error()) + } + if parsed.help { + _ = writeAuthHelp(stdout) + return exitSuccess + } + if len(parsed.positional) > 0 { + return writeExecUsageError(stderr, fmt.Sprintf("zero auth reset takes no arguments (got %q)", parsed.positional[0])) + } + manager, err := newAuthManager(deps, stdout) + if err != nil { + return writeAppError(stderr, redaction.ErrorMessage(err, redaction.Options{}), exitCrash) + } + if err := manager.Reset(); err != nil { + return writeAppError(stderr, redaction.ErrorMessage(err, redaction.Options{}), exitCrash) + } + if parsed.json { + payload := struct { + Reset bool `json:"reset"` + }{Reset: true} + if err := writePrettyJSON(stdout, payload); err != nil { + return exitCrash + } + return exitSuccess + } + if _, err := fmt.Fprintln(stdout, "Reset OAuth token store."); err != nil { + return exitCrash + } + return exitSuccess +} + func filterAuthStatuses(statuses []oauth.Status, provider string) []oauth.Status { want := oauth.ProviderKey(provider) filtered := make([]oauth.Status, 0, 1) @@ -589,6 +626,7 @@ Commands: logout Delete a provider's stored login status [provider] Show login presence/expiry (never the token) refresh [--watch] Force a token refresh (--watch keeps it fresh) + reset Reset the OAuth token store (clears corrupted entries and tokens) openrouter Log in to OpenRouter in the browser; mints an API key chatgpt Log in to ChatGPT in the browser (Codex backend, ChatGPT Plus/Pro) @@ -615,7 +653,7 @@ Flags: --device Use the device-code flow (headless/SSH; no browser) --scope Add an OAuth scope (repeatable) --watch Keep the token fresh in the foreground (refresh only) - --json Print result as JSON (status/logout) + --json Print result as JSON (status/logout/reset) -h, --help Show this help `) return err diff --git a/internal/cli/auth_test.go b/internal/cli/auth_test.go index 9b1ba0fb5..c895f6a8e 100644 --- a/internal/cli/auth_test.go +++ b/internal/cli/auth_test.go @@ -196,13 +196,42 @@ func TestRunAuthHelp(t *testing.T) { if code := runWithDeps([]string{"auth", "--help"}, &stdout, &stderr, appDeps{}); code != exitSuccess { t.Fatalf("exit = %d", code) } - for _, want := range []string{"zero auth", "login", "logout", "status", "refresh", "--device"} { + for _, want := range []string{"zero auth", "login", "logout", "status", "refresh", "reset", "--device"} { if !strings.Contains(stdout.String(), want) { t.Fatalf("help missing %q:\n%s", want, stdout.String()) } } } +func TestRunAuthReset(t *testing.T) { + path := withAuthStore(t) + store, err := oauth.NewStore(oauth.StoreOptions{FilePath: path}) + if err != nil { + t.Fatalf("NewStore: %v", err) + } + if err := store.Save(oauth.ProviderKey("demo"), oauth.Token{AccessToken: "secret"}); err != nil { + t.Fatalf("Save: %v", err) + } + + var stdout, stderr bytes.Buffer + if code := runWithDeps([]string{"auth", "reset"}, &stdout, &stderr, appDeps{}); code != exitSuccess { + t.Fatalf("exit = %d stderr=%s", code, stderr.String()) + } + if !strings.Contains(stdout.String(), "Reset OAuth token store") { + t.Fatalf("stdout = %q", stdout.String()) + } + + // Verify store is now empty. + stdout.Reset() + stderr.Reset() + if code := runWithDeps([]string{"auth", "status"}, &stdout, &stderr, appDeps{}); code != exitSuccess { + t.Fatalf("status exit = %d", code) + } + if !strings.Contains(stdout.String(), "No OAuth provider logins are stored.") { + t.Fatalf("expected empty store after reset, got: %q", stdout.String()) + } +} + // TestRunAuthLoginChatGPTRoutesToDedicatedFlow verifies `zero auth login // chatgpt` reaches the dedicated ChatGPT login (fixed-port loopback + mandatory // authorize params), not the generic manager path. The generic login accepts diff --git a/internal/cli/completions.go b/internal/cli/completions.go index 5f9058946..a5077e96a 100644 --- a/internal/cli/completions.go +++ b/internal/cli/completions.go @@ -78,7 +78,7 @@ var completionRoot = completionNode{ {names: []string{"tools"}, children: leafNodes("list")}, {names: []string{"oauth"}, children: leafNodes("login", "logout", "status")}, }}, - {names: []string{"auth"}, children: leafNodes("openrouter", "chatgpt", "login", "logout", "status", "refresh")}, + {names: []string{"auth"}, children: leafNodes("openrouter", "chatgpt", "login", "logout", "status", "refresh", "reset")}, {names: []string{"sandbox"}, children: []completionNode{ {names: []string{"policy"}}, {names: []string{"setup"}}, {names: []string{"check"}}, {names: []string{"grants"}, children: leafNodes("list", "allow", "deny", "revoke", "clear")}, diff --git a/internal/cli/completions_test.go b/internal/cli/completions_test.go index b7eb059b7..a3300bd96 100644 --- a/internal/cli/completions_test.go +++ b/internal/cli/completions_test.go @@ -159,6 +159,7 @@ func TestCompletionTreeCoversAliasesNestingAndCommonFlags(t *testing.T) { assertCandidates(t, byPath["worktree"], "prepare", "release") assertCandidates(t, byPath["daemon"], "start", "stop", "status", "run", "attach") assertCandidates(t, byPath["mcp oauth"], "login", "logout", "status") + assertCandidates(t, byPath["auth"], "openrouter", "chatgpt", "login", "logout", "status", "refresh", "reset") assertCandidates(t, byPath["sandbox grants"], "list", "allow", "deny", "revoke", "clear") assertCandidates(t, byPath["completions"], "bash", "zsh", "fish", "powershell", "elvish") assertCandidates(t, byPath["plugins"], "list", "add", "info", "remove", "rm") diff --git a/internal/keyring/keyring.go b/internal/keyring/keyring.go index 3c9f41166..0386bc31e 100644 --- a/internal/keyring/keyring.go +++ b/internal/keyring/keyring.go @@ -9,7 +9,9 @@ // through `security -i` (interactive mode), whose command parser is line-based // with a fixed 4096-byte line buffer, so Set rejects secrets containing // newlines or exceeding that budget rather than silently corrupting them; -// Linux's secret-tool has no such restriction. +// Linux's secret-tool has no such restriction. MaxSecretLen reports that budget +// so a caller holding a larger value can split it across entries instead of +// being turned away. package keyring import ( @@ -82,7 +84,7 @@ func (k *Keyring) Set(service, account, secret string) error { } } // -U updates the item if it already exists rather than failing. - line := "add-generic-password -U -s " + securityQuote(service) + " -a " + securityQuote(account) + " -w " + securityQuote(secret) + "\n" + line := securityAddLine(service, account, secret) if len(line) > securityMaxLine { return wrap("set", fmt.Errorf("secret too large for the macOS security tool's %d-byte command line; use the file backend instead", securityMaxLine)) } @@ -162,11 +164,42 @@ func (k *Keyring) Delete(service, account string) (bool, error) { } } +// MaxSecretLen reports the largest secret, in bytes, that Set accepts under +// (service, account) on this platform. ok is false when the backend imposes no +// practical limit, which is the case for Linux's secret-tool: it reads the +// secret from stdin rather than a command line. On macOS the secret shares the +// `security -i` command line with the service and account, so the budget +// depends on both and a caller that splits an oversized value must size each +// part against the exact account name it will be stored under. +// +// The budget assumes the secret needs no quote escaping. securityQuote only +// expands `\` and `"`, neither of which appears in base64 or hex, so a caller +// storing encoded data gets the exact figure. +func (k *Keyring) MaxSecretLen(service, account string) (int, bool) { + if k.goos != "darwin" { + return 0, false + } + // Measuring the real command with an empty secret counts the surrounding + // quotes too, so a secret of exactly this length lands on securityMaxLine. + budget := securityMaxLine - len(securityAddLine(service, account, "")) + if budget < 0 { + budget = 0 + } + return budget, true +} + // securityMaxLine is the most `security -i` can read as one command: its line // buffer is MAX_LINE_LEN (4096) bytes in Apple's SecurityTool, one of which the // terminating NUL consumes. const securityMaxLine = 4095 +// securityAddLine renders the `security -i` command that stores secret under +// (service, account). Set writes it and MaxSecretLen measures it, so the two +// cannot disagree about how much of the line the overhead consumes. +func securityAddLine(service, account, secret string) string { + return "add-generic-password -U -s " + securityQuote(service) + " -a " + securityQuote(account) + " -w " + securityQuote(secret) + "\n" +} + // securityQuote wraps s for one argument of a `security -i` command line. The // tool's parser (split_line in Apple's SecurityTool) treats a backslash inside // double quotes as escaping the next character and the matching quote as the diff --git a/internal/keyring/keyring_test.go b/internal/keyring/keyring_test.go index 430d402ed..8f2be3aad 100644 --- a/internal/keyring/keyring_test.go +++ b/internal/keyring/keyring_test.go @@ -375,3 +375,46 @@ func TestAvailable(t *testing.T) { t.Fatal("linux should be available") } } + +// TestMaxSecretLenMatchesTheDarwinSetBoundary pins the budget to the boundary +// Set actually enforces. A caller that splits an oversized value depends on the +// two agreeing exactly: one byte of drift either wastes an entry or produces a +// chunk security silently splits into two garbage commands. +func TestMaxSecretLenMatchesTheDarwinSetBoundary(t *testing.T) { + k := newFake("darwin").keyring() + budget, bounded := k.MaxSecretLen("zero", "oauth-tokens") + if !bounded { + t.Fatal("MaxSecretLen reported darwin as unbounded") + } + if err := newFake("darwin").keyring().Set("zero", "oauth-tokens", strings.Repeat("a", budget)); err != nil { + t.Errorf("Set(budget=%d bytes): %v, want acceptance at the boundary", budget, err) + } + if err := newFake("darwin").keyring().Set("zero", "oauth-tokens", strings.Repeat("a", budget+1)); err == nil { + t.Errorf("Set(%d bytes) succeeded, want rejection one byte past the budget", budget+1) + } +} + +// TestMaxSecretLenShrinksWithTheAccountName covers why the budget takes the +// account: on macOS the account and the secret share one command line, so a +// longer account name leaves less room for the secret. +func TestMaxSecretLenShrinksWithTheAccountName(t *testing.T) { + k := newFake("darwin").keyring() + short, _ := k.MaxSecretLen("zero", "oauth-tokens") + long, _ := k.MaxSecretLen("zero", "oauth-tokens.a.63") + if want := short - len(".a.63"); long != want { + t.Fatalf("MaxSecretLen for the longer account = %d, want %d", long, want) + } + if err := newFake("darwin").keyring().Set("zero", "oauth-tokens.a.63", strings.Repeat("a", long+1)); err == nil { + t.Error("Set one byte past the longer account's budget succeeded") + } +} + +// TestMaxSecretLenUnboundedOffDarwin keeps the limit platform-specific: +// secret-tool reads the secret from stdin, so there is no command line to fill. +func TestMaxSecretLenUnboundedOffDarwin(t *testing.T) { + for _, goos := range []string{"linux", "windows"} { + if n, bounded := newFake(goos).keyring().MaxSecretLen("zero", "oauth-tokens"); bounded || n != 0 { + t.Errorf("MaxSecretLen on %s = (%d, %v), want (0, false)", goos, n, bounded) + } + } +} diff --git a/internal/oauth/manager.go b/internal/oauth/manager.go index dbdf7817e..865a5062a 100644 --- a/internal/oauth/manager.go +++ b/internal/oauth/manager.go @@ -416,6 +416,14 @@ func (m *Manager) Logout(name string) (bool, error) { return m.store.Delete(ProviderKey(name)) } +// Reset clears all persistent state in the underlying store. +func (m *Manager) Reset() error { + if m == nil || m.store == nil { + return nil + } + return m.store.Reset() +} + // StatusAll returns the status of every provider login. func (m *Manager) StatusAll() ([]Status, error) { return m.store.Status(KeyPrefixProvider) diff --git a/internal/oauth/store.go b/internal/oauth/store.go index 1bc7f1dc8..a941e9e36 100644 --- a/internal/oauth/store.go +++ b/internal/oauth/store.go @@ -1,15 +1,19 @@ package oauth import ( + "crypto/sha256" "encoding/base64" + "encoding/hex" "encoding/json" "errors" "fmt" + "maps" "os" "path/filepath" "regexp" "runtime" "sort" + "strconv" "strings" "sync" "time" @@ -96,6 +100,10 @@ type StoreOptions struct { // Keyring is the client used when Storage=="keyring"; nil => keyring.New(). // Injected by tests to avoid touching a real keychain. Keyring KeyringClient + // KeyringLockPath overrides the cross-process lock file used by the keyring + // backend. When empty, it is derived from the keyring storage identity + // via ResolveKeyringLockPath(Env). + KeyringLockPath string } // KeyringClient is the minimal OS-keyring surface the store needs. *keyring.Keyring @@ -104,14 +112,43 @@ type KeyringClient interface { Get(service, account string) (string, bool, error) Set(service, account, secret string) error Delete(service, account string) (bool, error) + // MaxSecretLen reports the largest secret, in bytes, Set accepts under + // (service, account); ok is false when the backend has no practical limit. + // keyringBlob needs it because macOS caps a keychain write well below the + // size of a multi-provider token blob (see the keyringBlob doc). + MaxSecretLen(service, account string) (int, bool) } -// Keyring storage stores the whole token blob under one fixed entry. +// Keyring storage anchors the token blob at one fixed entry, which holds either +// the whole blob or, once that no longer fits, the manifest naming the chunks +// that do (see keyringBlob). const ( keyringService = "zero" keyringAccount = "oauth-tokens" ) +// Chunked keyring layout. +// +// keyringManifestPrefix marks the anchor entry as a manifest rather than a +// whole blob. ":" is outside the base64 alphabet, so a stored blob can never +// begin with this prefix and the two layouts are distinguishable without a +// schema migration. +// +// keyringChunkFamilyA and B name the two generations a write alternates +// between; keyringMaxChunks bounds both the accounts a read will probe and the +// blob size the backend will accept. +const ( + keyringManifestPrefix = "zc1:" + keyringChunkFamilyA = "a" + keyringChunkFamilyB = "b" + keyringMaxChunks = 64 + // keyringMinChunkLen is the smallest per-entry budget worth splitting into. + // Below it the service and account names have eaten the command line, so no + // chunk count would fit the blob and the store says so instead of writing + // hundreds of tiny entries. + keyringMinChunkLen = 256 +) + // Store persists OAuth tokens (provider + MCP namespaces) as one JSON blob, // written atomically through a pluggable backend (a 0600 file guarded by a // cross-process lock, or the OS keyring). When crypter is non-nil the file blob @@ -158,6 +195,28 @@ func ResolveStorePath(env map[string]string) (string, error) { return filepath.Join(configHome, "zero", "oauth-tokens.json"), nil } +// ResolveKeyringLockPath determines the on-disk cross-process lock file for the +// keyring backend. Unlike file-backed storage, keyring entries are anchored to +// the user's OS keychain (zero/oauth-tokens) and are shared across all processes +// for that OS user, regardless of file-store overrides such as +// ZERO_OAUTH_TOKENS_PATH, FilePath, or XDG_CONFIG_HOME. +func ResolveKeyringLockPath(env map[string]string) (string, error) { + if override := strings.TrimSpace(envValue(env, "ZERO_OAUTH_KEYRING_LOCK_PATH")); override != "" { + if filepath.IsAbs(override) { + return filepath.Clean(override), nil + } + return filepath.Abs(override) + } + home, err := os.UserHomeDir() + if err != nil { + return "", fmt.Errorf("oauth: resolve user home for keyring lock: %w", err) + } + if resolved, err := filepath.EvalSymlinks(home); err == nil { + home = resolved + } + return filepath.Join(home, ".zero", "oauth-keyring.lockfile"), 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) { @@ -197,11 +256,17 @@ 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") + // file derived from the keyring storage identity ("zero"/"oauth-tokens"). + // Unlike the file backend, the keyring does not vary with file configuration + // (ZERO_OAUTH_TOKENS_PATH, FilePath, or XDG_CONFIG_HOME). Best-effort: if no + // home location resolves, fall back to in-process serialization only. + lockPath := strings.TrimSpace(options.KeyringLockPath) + if lockPath == "" { + lp, perr := ResolveKeyringLockPath(options.Env) + if perr != nil { + return nil, perr + } + lockPath = lp } return &Store{blob: keyringBlob{kr: kr, service: keyringService, account: keyringAccount, lockPath: lockPath}, now: now}, nil default: @@ -287,6 +352,15 @@ func (s *Store) Delete(key string) (bool, error) { return removed, err } +// Reset clears all persistent state in the store (removing files or deleting all keyring entries). +func (s *Store) Reset() error { + s.mu.Lock() + defer s.mu.Unlock() + return s.blob.withLock(s.now, func() error { + return s.blob.reset() + }) +} + // Status returns redaction-safe summaries of every stored token, sorted by key. // An optional prefix filters to one namespace (e.g. KeyPrefixProvider). func (s *Store) Status(prefix string) ([]Status, error) { @@ -382,6 +456,8 @@ type blobStore interface { read() (data []byte, ok bool, err error) // write replaces the stored blob. write(data []byte) error + // reset removes all stored state and entries. + reset() 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). @@ -394,6 +470,36 @@ type blobStore interface { // by a cross-process lock file. Behavior matches the original file store. type fileBlob struct{ path string } +func (b fileBlob) reset() error { + var errs []error + if err := os.Remove(b.path); err != nil && !errors.Is(err, os.ErrNotExist) { + errs = append(errs, err) + } + if err := os.Remove(b.path + ".secret"); err != nil && !errors.Is(err, os.ErrNotExist) { + errs = append(errs, err) + } + for _, dir := range []string{b.path + ".publish", b.path + ".secret.publish"} { + entries, err := os.ReadDir(dir) + if err != nil { + if !errors.Is(err, os.ErrNotExist) { + errs = append(errs, err) + } + continue + } + for _, entry := range entries { + if strings.HasPrefix(entry.Name(), "publish-") { + if err := os.Remove(filepath.Join(dir, entry.Name())); err != nil && !errors.Is(err, os.ErrNotExist) { + errs = append(errs, err) + } + } + } + } + if err := os.Remove(b.path + ".secret.lock"); err != nil && !errors.Is(err, os.ErrNotExist) { + errs = append(errs, err) + } + return errors.Join(errs...) +} + func (b fileBlob) read() ([]byte, bool, error) { data, err := os.ReadFile(b.path) if err != nil { @@ -469,8 +575,29 @@ func (b fileBlob) withLock(now func() time.Time, fn func() error) error { func (b fileBlob) location() string { return b.path } -// keyringBlob persists the blob in the OS keyring as a single base64 entry -// (base64 keeps the multi-line JSON a single, control-character-free value). +// keyringBlob persists the blob in the OS keyring, base64-encoded (base64 keeps +// the multi-line JSON a single, control-character-free value). +// +// macOS caps a keychain write at securityMaxLine bytes, because the secret +// rides inside a `security -i` command line, and the blob holds every provider +// and MCP login at once. That budget is 4039 bytes of base64 under the anchor +// account, which is 3027 bytes of JSON: one OIDC login carrying an access +// token, an ID token and a refresh token can fill it on its own, and two +// reliably do. Storing the blob as one entry therefore turned a second login +// into a hard "secret too large" failure with nothing saved. +// +// So a blob that does not fit is split across numbered entries and the anchor +// account holds a manifest instead. Chunks live under two alternating +// generations ("families"): a write fills the family that is NOT live, then +// replaces the manifest. That single Set is the commit point, so until it lands +// read() still returns the previous generation intact and a crash mid-write +// loses nothing. Cleanup of the retired generation runs after the commit and +// correctness never depends on it: the manifest states how many chunks each +// family holds, and it is only ever written with a count it has already made +// true, so a failed cleanup over-states and the next write deletes the excess. +// +// Backends with no size limit (Linux secret-tool reads the secret from stdin) +// never reach the chunked layout, so their stored form is unchanged. type keyringBlob struct { kr KeyringClient service string @@ -480,25 +607,335 @@ type keyringBlob struct { lockPath string } +// keyringManifest describes which chunk generation is live and how many entries +// each generation currently holds. +type keyringManifest struct { + live string + counts map[string]int + digest string +} + +func (b keyringBlob) corruptError(detail string) error { + return fmt.Errorf("oauth: keyring token data at %s (account %q) %s; run `zero auth reset` or remove entries %q, %q.a.0..%d, and %q.b.0..%d to recover", + b.location(), b.account, detail, b.account, b.account, keyringMaxChunks-1, b.account, keyringMaxChunks-1) +} + func (b keyringBlob) read() ([]byte, bool, error) { - enc, ok, err := b.kr.Get(b.service, b.account) + var lastErr error + for attempt := 0; attempt < 3; attempt++ { + if attempt > 0 { + time.Sleep(10 * time.Millisecond) + } + head, ok, err := b.kr.Get(b.service, b.account) + if err != nil || !ok { + return nil, ok, err + } + head = strings.TrimSpace(head) + if !strings.HasPrefix(head, keyringManifestPrefix) { + data, err := decodeKeyringBlob(head) + if err != nil { + lastErr = b.corruptError("contains invalid base64 data: " + err.Error()) + continue + } + return data, true, nil + } + manifest, err := parseKeyringManifest(head) + if err != nil { + lastErr = b.corruptError("has a malformed manifest (" + err.Error() + ")") + continue + } + count := manifest.counts[manifest.live] + var encoded strings.Builder + var chunkErr error + for index := range count { + part, ok, err := b.kr.Get(b.service, b.chunkAccount(manifest.live, index)) + if err != nil { + chunkErr = err + break + } + if !ok { + chunkErr = b.corruptError(fmt.Sprintf("is missing chunk %d of %d", index+1, count)) + break + } + encoded.WriteString(strings.TrimSpace(part)) + } + if chunkErr != nil { + lastErr = chunkErr + continue + } + data, err := decodeKeyringBlob(encoded.String()) + if err != nil { + lastErr = b.corruptError("contains invalid chunk data: " + err.Error()) + continue + } + // The corruption this guards against is the one that motivated chunking: + // `security -i` splits an overlong line into two garbage commands rather + // than refusing it, so a stored chunk can come back truncated. + if sum := sha256.Sum256(data); hex.EncodeToString(sum[:]) != manifest.digest { + lastErr = b.corruptError("failed its integrity check") + continue + } + return data, true, nil + } + return nil, false, lastErr +} + +func (b keyringBlob) write(data []byte) error { + encoded := base64.StdEncoding.EncodeToString(data) + // Read the live manifest before overwriting anything: it names the + // generation this write must avoid, and the counts cleanup needs afterwards. + previous, err := b.readManifest() + if err != nil { + return err + } + if budget, bounded := b.kr.MaxSecretLen(b.service, b.account); !bounded || len(encoded) <= budget { + return b.writeWhole(encoded, previous) + } + return b.writeChunked(data, encoded, previous) +} + +// writeWhole stores the blob under the anchor account, the layout every backend +// without a size limit uses and the one a shrinking store returns to. The Set +// is the commit point; the chunks it retires are deleted after it lands. +// +// A delete that fails here leaves residue the manifest can no longer describe, +// because the anchor now holds the blob rather than the counts. Nothing +// reclaims it until the store next outgrows a single entry, where writeChunked +// sweeps both generations; a store that shrinks once and never grows again +// keeps it. Sweeping on every whole write would close that, but it costs a +// keyringMaxChunks-wide probe per save — 128 `security` invocations on macOS — +// for residue that only an already-failed delete can produce. +func (b keyringBlob) writeWhole(encoded string, previous keyringManifest) error { + if err := b.kr.Set(b.service, b.account, encoded); err != nil { + return err + } + b.sweepCleanupAccount() + err := b.deleteChunkRange(keyringChunkFamilyA, 0, previous.counts[keyringChunkFamilyA], nil) + if err = b.deleteChunkRange(keyringChunkFamilyB, 0, previous.counts[keyringChunkFamilyB], err); err != nil { + return fmt.Errorf("oauth: tokens were saved, but a superseded keyring entry at %s could not be removed: %w", b.location(), err) + } + return nil +} + +func (b keyringBlob) writeChunked(data []byte, encoded string, previous keyringManifest) (retErr error) { + b.sweepCleanupAccount() + family := keyringChunkFamilyA + if previous.live == keyringChunkFamilyA { + family = keyringChunkFamilyB + } + // Size every chunk against the LONGEST account name the family can produce. + // On macOS the account shares the command line with the secret, so a budget + // derived from chunk 0 would overflow once the index grew a digit; pinning + // it to the highest index keeps the size independent of the final count. + budget, bounded := b.kr.MaxSecretLen(b.service, b.chunkAccount(family, keyringMaxChunks-1)) + if !bounded || budget < keyringMinChunkLen { + return fmt.Errorf("oauth: keyring entries at %s hold at most %d bytes, too small to store the token blob; use file storage", b.location(), budget) + } + if len(encoded) > budget*keyringMaxChunks { + return fmt.Errorf("oauth: token blob is %d encoded bytes, over the %d the keyring at %s can hold; use file storage", len(encoded), budget*keyringMaxChunks, b.location()) + } + + count := (len(encoded) + budget - 1) / budget + // Make the manifest cover the range this write is about to occupy BEFORE + // occupying it. Without that, a write that dies partway through leaves + // chunks above the count the manifest records, and no later cleanup would + // know to delete them: a fragment of a token blob would sit in the keychain + // for good. Raising the count of a generation that is not live changes + // nothing a reader looks at, so the reservation is invisible until the + // commit below. + if previous.live == "" { + // Nothing to reserve against: the anchor still holds the whole blob, so + // writing a manifest here would destroy the only copy. Sweep both + // generations instead. This runs once, when a store first outgrows a + // single entry, and only has anything to find if an earlier shrink was + // interrupted before its cleanup finished. + // + // Both, not just the target: writeWhole replaced the anchor with the + // blob, so the counts that named the chunks a failed cleanup left + // behind are gone. The manifest cannot state the range any more and no + // later write derives it, so this is the only sweep that will ever + // reach them. Sweeping just the target would leave the other + // generation's chunks, and the token material in them, unreferenced + // for good. + other := keyringChunkFamilyB + if family == keyringChunkFamilyB { + other = keyringChunkFamilyA + } + err := b.deleteChunkRange(family, count, keyringMaxChunks, nil) + if err = b.deleteChunkRange(other, 0, keyringMaxChunks, err); err != nil { + return err + } + } else if count > previous.counts[family] { + reservation := keyringManifest{live: previous.live, counts: maps.Clone(previous.counts), digest: previous.digest} + reservation.counts[family] = count + if err := b.kr.Set(b.service, b.account, formatKeyringManifest(reservation)); err != nil { + return err + } + previous.counts[family] = count + } + + writtenChunks := 0 + defer func() { + if retErr != nil && previous.live == "" && writtenChunks > 0 { + // During the first migration from whole-entry to chunked layout, + // a failure before the manifest commit point must clean up any chunks + // written so far to avoid leaving orphaned credential material in the keychain. + if err := b.deleteChunkRange(family, 0, writtenChunks, nil); err != nil { + _ = b.kr.Set(b.service, b.cleanupAccount(), fmt.Sprintf("%s:%d", family, writtenChunks)) + retErr = errors.Join(retErr, fmt.Errorf("cleanup orphaned migration chunks: %w", err)) + } else { + _, _ = b.kr.Delete(b.service, b.cleanupAccount()) + } + } + }() + + for index := range count { + offset := index * budget + if err := b.kr.Set(b.service, b.chunkAccount(family, index), encoded[offset:min(offset+budget, len(encoded))]); err != nil { + return err + } + writtenChunks++ + } + // This generation is not live yet, so trimming a longer previous one of it + // now is safe, and it makes the count the manifest is about to publish true + // before the manifest claims it. + if err := b.deleteChunkRange(family, count, previous.counts[family], nil); err != nil { + return err + } + + next := keyringManifest{live: family, counts: maps.Clone(previous.counts), digest: hexDigest(data)} + next.counts[family] = count + if err := b.kr.Set(b.service, b.account, formatKeyringManifest(next)); err != nil { + return err + } + + // Committed. The retired generation is now unreferenced, so deleting it is + // secret hygiene rather than correctness. Its count deliberately stays in + // the manifest afterwards: over-stating is the safe direction, it saves the + // next write a reservation, and if a delete here failed the next cleanup + // retries the same range. + retired := previous.live + if retired == "" || next.counts[retired] == 0 { + return nil + } + if err := b.deleteChunkRange(retired, 0, next.counts[retired], nil); err != nil { + return fmt.Errorf("oauth: tokens were saved, but a superseded keyring entry at %s could not be removed: %w", b.location(), err) + } + return nil +} + +func (b keyringBlob) reset() error { + b.sweepCleanupAccount() + _, bounded := b.kr.MaxSecretLen(b.service, b.account) + if !bounded { + var errs []error + if _, err := b.kr.Delete(b.service, b.account); err != nil { + errs = append(errs, fmt.Errorf("remove %s: %w", b.account, err)) + } + _, _ = b.kr.Delete(b.service, b.cleanupAccount()) + return errors.Join(errs...) + } + + manifest, err := b.readManifest() + if err == nil && (manifest.live != "" || len(manifest.counts) > 0) { + var delErr error + delErr = b.deleteChunkRange(keyringChunkFamilyA, 0, manifest.counts[keyringChunkFamilyA], nil) + delErr = b.deleteChunkRange(keyringChunkFamilyB, 0, manifest.counts[keyringChunkFamilyB], delErr) + if _, kerr := b.kr.Delete(b.service, b.account); kerr != nil && delErr == nil { + delErr = fmt.Errorf("remove %s: %w", b.account, kerr) + } + _, _ = b.kr.Delete(b.service, b.cleanupAccount()) + return delErr + } + + var delErr error + delErr = b.deleteChunkRange(keyringChunkFamilyA, 0, keyringMaxChunks, nil) + delErr = b.deleteChunkRange(keyringChunkFamilyB, 0, keyringMaxChunks, delErr) + if _, kerr := b.kr.Delete(b.service, b.account); kerr != nil && delErr == nil { + delErr = fmt.Errorf("remove %s: %w", b.account, kerr) + } + _, _ = b.kr.Delete(b.service, b.cleanupAccount()) + return delErr +} + +func (b keyringBlob) cleanupAccount() string { + return b.account + ".cleanup" +} + +func (b keyringBlob) sweepCleanupAccount() { + raw, ok, err := b.kr.Get(b.service, b.cleanupAccount()) + if err != nil || !ok { + return + } + raw = strings.TrimSpace(raw) + if raw != "" { + parts := strings.Split(raw, ":") + if len(parts) == 2 { + family := parts[0] + if family == keyringChunkFamilyA || family == keyringChunkFamilyB { + if count, err := strconv.Atoi(parts[1]); err == nil && count > 0 { + if count > keyringMaxChunks { + count = keyringMaxChunks + } + if err := b.deleteChunkRange(family, 0, count, nil); err != nil { + return + } + } + } + } + } + _, _ = b.kr.Delete(b.service, b.cleanupAccount()) +} + +// readManifest returns the live manifest, or a zero manifest when the anchor +// holds a whole blob or nothing at all. A malformed manifest is an error: the +// chunks it names are unreachable without it, and overwriting it would strand +// them in the keychain. +func (b keyringBlob) readManifest() (keyringManifest, error) { + head, ok, err := b.kr.Get(b.service, b.account) if err != nil || !ok { - return nil, ok, err + return keyringManifest{counts: map[string]int{}}, err + } + head = strings.TrimSpace(head) + if !strings.HasPrefix(head, keyringManifestPrefix) { + return keyringManifest{counts: map[string]int{}}, nil } - data, err := base64.StdEncoding.DecodeString(strings.TrimSpace(enc)) + m, err := parseKeyringManifest(head) if err != nil { - return nil, false, fmt.Errorf("oauth: decode keyring token blob: %w", err) + return keyringManifest{counts: map[string]int{}}, b.corruptError("has a malformed manifest (" + err.Error() + ")") } - return data, true, nil + return m, nil } -func (b keyringBlob) write(data []byte) error { - return b.kr.Set(b.service, b.account, base64.StdEncoding.EncodeToString(data)) +// deleteChunkRange removes chunks [from, to) of family, joining onto prior. It +// takes prior so the two-family cleanup in writeWhole reports the first failure +// without either delete being skipped. +func (b keyringBlob) deleteChunkRange(family string, from, to int, prior error) error { + for index := from; index < to; index++ { + account := b.chunkAccount(family, index) + if _, err := b.kr.Delete(b.service, account); err != nil && prior == nil { + prior = fmt.Errorf("remove %s: %w", account, err) + } + } + return prior +} + +func (b keyringBlob) chunkAccount(family string, index int) string { + return b.account + "." + family + "." + strconv.Itoa(index) } // 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. +// The chunked layout needs it for a second reason: it makes a write's chunk +// fills and its manifest commit one indivisible sequence to other processes. +// +// Readers (Load, Status) take it as well, which is deliberate and not a +// leftover: the generational layout already hands them a consistent view +// without it, but holding it keeps a read from observing a torn manifest on a +// backend whose Set is not atomic, and it bounds a reader behind at most one +// writer. acquireFileLock reclaims after fileLockStaleAfter, so a crashed +// holder cannot wedge the hot path. func (b keyringBlob) withLock(now func() time.Time, fn func() error) error { if b.lockPath == "" { return fn() @@ -513,6 +950,58 @@ func (b keyringBlob) withLock(now func() time.Time, fn func() error) error { func (b keyringBlob) location() string { return "keyring:" + b.service + "/" + b.account } +func decodeKeyringBlob(encoded string) ([]byte, error) { + data, err := base64.StdEncoding.DecodeString(encoded) + if err != nil { + return nil, fmt.Errorf("oauth: decode keyring token blob: %w", err) + } + return data, nil +} + +func hexDigest(data []byte) string { + sum := sha256.Sum256(data) + return hex.EncodeToString(sum[:]) +} + +// formatKeyringManifest renders "zc1::::". +func formatKeyringManifest(m keyringManifest) string { + return keyringManifestPrefix + m.live + + ":" + strconv.Itoa(m.counts[keyringChunkFamilyA]) + + ":" + strconv.Itoa(m.counts[keyringChunkFamilyB]) + + ":" + m.digest +} + +func parseKeyringManifest(head string) (keyringManifest, error) { + malformed := func(detail string) (keyringManifest, error) { + return keyringManifest{}, fmt.Errorf("oauth: keyring token manifest %s", detail) + } + fields := strings.Split(strings.TrimPrefix(head, keyringManifestPrefix), ":") + if len(fields) != 4 { + return malformed("is malformed") + } + m := keyringManifest{live: fields[0], counts: map[string]int{}, digest: fields[3]} + if m.live != keyringChunkFamilyA && m.live != keyringChunkFamilyB { + return malformed(fmt.Sprintf("names unknown generation %q", m.live)) + } + for i, family := range []string{keyringChunkFamilyA, keyringChunkFamilyB} { + count, err := strconv.Atoi(fields[i+1]) + if err != nil || count < 0 || count > keyringMaxChunks { + return malformed(fmt.Sprintf("has invalid chunk count %q", fields[i+1])) + } + m.counts[family] = count + } + if m.counts[m.live] == 0 { + return malformed("names a generation with no chunks") + } + if len(m.digest) != hex.EncodedLen(sha256.Size) { + return malformed("has an invalid digest") + } + if _, err := hex.DecodeString(m.digest); err != nil { + return malformed("has an invalid digest") + } + return m, nil +} + // FormatStatuses renders a human-readable status table without leaking token // material. func FormatStatuses(statuses []Status) string { diff --git a/internal/oauth/store_keyring_chunked_test.go b/internal/oauth/store_keyring_chunked_test.go new file mode 100644 index 000000000..81c20f0a8 --- /dev/null +++ b/internal/oauth/store_keyring_chunked_test.go @@ -0,0 +1,1056 @@ +package oauth + +import ( + "errors" + "path/filepath" + "strconv" + "strings" + "testing" + "time" +) + +// macOSLikeBudget is the real macOS ceiling: `security -i` reads at most 4095 +// bytes of command line, and the fake charges the account name against it the +// way the real command line does. +const macOSLikeBudget = 4095 + +// bigToken builds a credential the size of a real OIDC login (a JWT access +// token, an ID token, and an opaque refresh token), which is what pushes the +// combined blob past a single keychain entry. +func bigToken(seed string) Token { + return Token{ + AccessToken: strings.Repeat(seed, 1200), + IDToken: strings.Repeat(seed, 900), + RefreshToken: strings.Repeat(seed, 300), + TokenType: "Bearer", + Scopes: []string{"openid", "profile", "email", "offline_access"}, + ExpiresAt: time.Unix(1_800_000_000, 0).UTC(), + Account: seed + "@example.com", + } +} + +func newCappedKeyringStore(t *testing.T, kr KeyringClient) *Store { + t.Helper() + dir := t.TempDir() + t.Setenv("HOME", dir) + t.Setenv("USERPROFILE", dir) + t.Setenv("XDG_CONFIG_HOME", dir) + s, err := NewStore(StoreOptions{Storage: "keyring", Keyring: kr}) + if err != nil { + t.Fatalf("NewStore(keyring): %v", err) + } + return s +} + +func mustSave(t *testing.T, s *Store, name string, token Token) { + t.Helper() + if err := s.Save(ProviderKey(name), token); err != nil { + t.Fatalf("Save(%s): %v", name, err) + } +} + +func mustLoad(t *testing.T, s *Store, name string) Token { + t.Helper() + token, ok, err := s.Load(ProviderKey(name)) + if err != nil { + t.Fatalf("Load(%s): %v", name, err) + } + if !ok { + t.Fatalf("Load(%s): no token stored", name) + } + return token +} + +func manifestOf(t *testing.T, kr *fakeKR) keyringManifest { + t.Helper() + head, ok, _ := kr.Get(keyringService, keyringAccount) + if !ok { + t.Fatal("no anchor entry stored") + } + if !strings.HasPrefix(head, keyringManifestPrefix) { + t.Fatalf("anchor entry is a whole blob, not a manifest: %.32s...", head) + } + manifest, err := parseKeyringManifest(head) + if err != nil { + t.Fatalf("parseKeyringManifest(%q): %v", head, err) + } + return manifest +} + +// TestStoreKeyringSavesSecondLoginOverEntryLimit is the regression: two OIDC +// logins do not fit one macOS keychain entry, and storing the blob as a single +// entry made the second Save fail outright with nothing persisted. +func TestStoreKeyringSavesSecondLoginOverEntryLimit(t *testing.T) { + kr := newCappedFakeKR(macOSLikeBudget) + s := newCappedKeyringStore(t, kr) + + mustSave(t, s, "first", bigToken("a")) + mustSave(t, s, "second", bigToken("b")) + + if got := mustLoad(t, s, "first"); got.AccessToken != bigToken("a").AccessToken { + t.Error("first login did not survive the second save") + } + if got := mustLoad(t, s, "second"); got.AccessToken != bigToken("b").AccessToken { + t.Error("second login did not round-trip") + } + + manifest := manifestOf(t, kr) + if manifest.counts[manifest.live] < 2 { + t.Fatalf("blob fit in %d chunk(s); the test no longer exercises splitting", manifest.counts[manifest.live]) + } + for _, account := range kr.chunkAccounts(manifest.live) { + if got := len(kr.data[keyringService+"/"+account]); got > macOSLikeBudget-len(account) { + t.Errorf("chunk %s is %d bytes, over its own entry budget", account, got) + } + } +} + +// TestStoreKeyringChunkSizingSurvivesLongerChunkAccounts pins the reason chunks +// are sized against the highest possible index: on macOS the account shares the +// command line with the secret, so a budget taken from chunk 0 overflows once +// the index grows a digit. +func TestStoreKeyringChunkSizingSurvivesLongerChunkAccounts(t *testing.T) { + // A tiny budget forces enough chunks that the index reaches two digits. + kr := newCappedFakeKR(keyringMinChunkLen + len(keyringAccount) + 8) + s := newCappedKeyringStore(t, kr) + + mustSave(t, s, "first", bigToken("a")) + + manifest := manifestOf(t, kr) + if manifest.counts[manifest.live] < 10 { + t.Fatalf("only %d chunks; the test needs a two-digit index", manifest.counts[manifest.live]) + } + if got := mustLoad(t, s, "first"); got.AccessToken != bigToken("a").AccessToken { + t.Error("blob did not round-trip across two-digit chunk indices") + } +} + +// TestStoreKeyringChunkedReadRejectsMissingChunk covers a torn read: a chunk the +// manifest names is gone, which must be reported rather than decoded as a +// truncated blob. +func TestStoreKeyringChunkedReadRejectsMissingChunk(t *testing.T) { + kr := newCappedFakeKR(macOSLikeBudget) + s := newCappedKeyringStore(t, kr) + mustSave(t, s, "first", bigToken("a")) + mustSave(t, s, "second", bigToken("b")) + + manifest := manifestOf(t, kr) + accounts := kr.chunkAccounts(manifest.live) + delete(kr.data, keyringService+"/"+accounts[len(accounts)-1]) + + _, _, err := s.Load(ProviderKey("first")) + if err == nil || !strings.Contains(err.Error(), "missing chunk") { + t.Fatalf("Load after losing a chunk: err = %v, want a missing-chunk error", err) + } +} + +// TestStoreKeyringChunkedReadRejectsTruncatedChunk covers the corruption that +// motivated chunking in the first place: `security -i` splits an overlong line +// into two commands instead of refusing it, so a chunk can come back short. The +// digest has to catch that even when the result is still valid base64. +func TestStoreKeyringChunkedReadRejectsTruncatedChunk(t *testing.T) { + kr := newCappedFakeKR(macOSLikeBudget) + s := newCappedKeyringStore(t, kr) + mustSave(t, s, "first", bigToken("a")) + mustSave(t, s, "second", bigToken("b")) + + manifest := manifestOf(t, kr) + account := kr.chunkAccounts(manifest.live)[0] + key := keyringService + "/" + account + // Drop a whole base64 quantum so the concatenation still decodes cleanly. + kr.data[key] = kr.data[key][:len(kr.data[key])-4] + + _, _, err := s.Load(ProviderKey("first")) + if err == nil { + t.Fatal("Load of a truncated chunk succeeded; the digest did not catch it") + } + if !strings.Contains(err.Error(), "integrity check") && !strings.Contains(err.Error(), "invalid token store") { + t.Fatalf("Load of a truncated chunk: err = %v, want an integrity or parse failure", err) + } +} + +// TestStoreKeyringWriteCommitsOnlyAtTheManifest asserts the commit point: a +// write that dies while filling chunks must leave the previous generation +// readable, because the manifest still names it. +func TestStoreKeyringWriteCommitsOnlyAtTheManifest(t *testing.T) { + kr := newCappedFakeKR(macOSLikeBudget) + s := newCappedKeyringStore(t, kr) + mustSave(t, s, "first", bigToken("a")) + mustSave(t, s, "second", bigToken("b")) + + before := manifestOf(t, kr) + boom := errors.New("keychain is locked") + // Fail the second chunk of whichever generation the next write targets, so + // the write dies partway through filling it. + kr.failSet = func(account string) error { + if strings.HasSuffix(account, ".1") { + return boom + } + return nil + } + + if err := s.Save(ProviderKey("third"), bigToken("c")); !errors.Is(err, boom) { + t.Fatalf("Save with a failing chunk write: err = %v, want %v", err, boom) + } + + kr.failSet = nil + if got := mustLoad(t, s, "first"); got.AccessToken != bigToken("a").AccessToken { + t.Error("a failed write damaged the committed blob") + } + if _, ok, _ := s.Load(ProviderKey("third")); ok { + t.Error("a write that never reached the manifest was visible anyway") + } + if after := manifestOf(t, kr); after.live != before.live || after.digest != before.digest { + t.Errorf("manifest moved on a failed write: %+v -> %+v", before, after) + } +} + +// TestStoreKeyringWriteFailsOnFinalManifestPublication asserts that when the +// final manifest publication fails after all chunk writes succeed, the previous +// manifest and committed blob remain readable, the new login is not visible, +// and the target generation's written accounts remain tracked for cleanup. +func TestStoreKeyringWriteFailsOnFinalManifestPublication(t *testing.T) { + kr := newCappedFakeKR(macOSLikeBudget) + s := newCappedKeyringStore(t, kr) + mustSave(t, s, "first", bigToken("a")) + mustSave(t, s, "second", bigToken("b")) + + before := manifestOf(t, kr) + boom := errors.New("keychain is locked on manifest publish") + + // Allow reservation and chunk writes to succeed, but fail when publishing + // the final manifest with next.live. + anchorSets := 0 + kr.failSet = func(account string) error { + if account == keyringAccount { + anchorSets++ + if anchorSets > 1 { + return boom + } + } + return nil + } + + if err := s.Save(ProviderKey("third"), bigToken("c")); !errors.Is(err, boom) { + t.Fatalf("Save with failing final manifest publication: err = %v, want %v", err, boom) + } + kr.failSet = nil + if anchorSets != 2 { + t.Errorf("anchor writes = %d, want 2", anchorSets) + } + if kr.sets[keyringAccount] < 1 { + t.Errorf("successful anchor sets = %d, want at least 1", kr.sets[keyringAccount]) + } + + if got := mustLoad(t, s, "first"); got.AccessToken != bigToken("a").AccessToken { + t.Error("a failed final manifest publication damaged the committed blob") + } + if _, ok, _ := s.Load(ProviderKey("third")); ok { + t.Error("a write that failed final manifest publication was visible anyway") + } + + after := manifestOf(t, kr) + if after.live != before.live { + t.Fatalf("live generation moved on failed final manifest publication: %q -> %q", before.live, after.live) + } + assertNoStrayChunks(t, kr, after) + + // A subsequent write successfully cleans up the orphaned generation chunks + mustSave(t, s, "fourth", bigToken("d")) + assertNoStrayChunks(t, kr, manifestOf(t, kr)) +} + +// TestStoreKeyringAlternatesGenerationsAndRetiresTheOld covers the ping-pong: +// each write lands in the generation that is not live, and the retired one is +// removed so a previous login's tokens do not linger in the keychain. +func TestStoreKeyringAlternatesGenerationsAndRetiresTheOld(t *testing.T) { + kr := newCappedFakeKR(macOSLikeBudget) + s := newCappedKeyringStore(t, kr) + mustSave(t, s, "first", bigToken("a")) + mustSave(t, s, "second", bigToken("b")) + + seen := map[string]bool{} + for round := range 3 { + mustSave(t, s, "second", bigToken(string(rune('c'+round)))) + manifest := manifestOf(t, kr) + seen[manifest.live] = true + + retired := keyringChunkFamilyA + if manifest.live == keyringChunkFamilyA { + retired = keyringChunkFamilyB + } + if left := kr.chunkAccounts(retired); len(left) != 0 { + t.Errorf("round %d: retired generation %q still holds %v", round, retired, left) + } + // The retired count deliberately stays in the manifest. Over-stating is + // the safe direction, so the invariant to hold is the one-sided one. + assertNoStrayChunks(t, kr, manifest) + } + if len(seen) != 2 { + t.Errorf("writes stayed in generation(s) %v; they must alternate", seen) + } +} + +// TestStoreKeyringGrowsIntoChunksAndShrinksBack covers both layout transitions, +// including that shrinking below the cap removes the chunks rather than +// stranding a logged-out provider's tokens in the keychain. +func TestStoreKeyringGrowsIntoChunksAndShrinksBack(t *testing.T) { + kr := newCappedFakeKR(macOSLikeBudget) + s := newCappedKeyringStore(t, kr) + + mustSave(t, s, "small", Token{AccessToken: "short", RefreshToken: "r"}) + if head, _, _ := kr.Get(keyringService, keyringAccount); strings.HasPrefix(head, keyringManifestPrefix) { + t.Fatal("a blob that fits one entry was chunked anyway") + } + + mustSave(t, s, "first", bigToken("a")) + mustSave(t, s, "second", bigToken("b")) + manifest := manifestOf(t, kr) + if len(kr.chunkAccounts(manifest.live)) == 0 { + t.Fatal("blob outgrew one entry but no chunks were written") + } + + if _, err := s.Delete(ProviderKey("first")); err != nil { + t.Fatalf("Delete(first): %v", err) + } + if _, err := s.Delete(ProviderKey("second")); err != nil { + t.Fatalf("Delete(second): %v", err) + } + + head, ok, _ := kr.Get(keyringService, keyringAccount) + if !ok || strings.HasPrefix(head, keyringManifestPrefix) { + t.Fatalf("store did not return to a whole entry after shrinking: %.32s...", head) + } + for _, family := range []string{keyringChunkFamilyA, keyringChunkFamilyB} { + if left := kr.chunkAccounts(family); len(left) != 0 { + t.Errorf("generation %q still holds %v after shrinking", family, left) + } + } + if got := mustLoad(t, s, "small"); got.AccessToken != "short" { + t.Errorf("small token lost across the layout transitions: %#v", got) + } +} + +// TestStoreKeyringUnboundedBackendNeverChunks pins that a backend without a +// size limit (Linux secret-tool reads the secret from stdin) keeps the original +// single-entry form, so nothing about its stored layout changes. +func TestStoreKeyringUnboundedBackendNeverChunks(t *testing.T) { + kr := newFakeKR() + s := newCappedKeyringStore(t, kr) + mustSave(t, s, "first", bigToken("a")) + mustSave(t, s, "second", bigToken("b")) + + head, ok, _ := kr.Get(keyringService, keyringAccount) + if !ok || strings.HasPrefix(head, keyringManifestPrefix) { + t.Fatalf("unbounded backend used the chunked layout: %.32s...", head) + } + if got := mustLoad(t, s, "second"); got.AccessToken != bigToken("b").AccessToken { + t.Error("unbounded backend did not round-trip") + } +} + +// TestStoreKeyringReadsLegacyWholeEntry covers the upgrade path: a blob written +// by a build that only knew the single-entry layout is still read, without a +// migration step. +func TestStoreKeyringReadsLegacyWholeEntry(t *testing.T) { + kr := newFakeKR() + legacy := newCappedKeyringStore(t, kr) + mustSave(t, legacy, "first", Token{AccessToken: "legacy", RefreshToken: "r"}) + + kr.budget = macOSLikeBudget + s := newCappedKeyringStore(t, kr) + if got := mustLoad(t, s, "first"); got.AccessToken != "legacy" { + t.Fatalf("legacy entry did not load: %#v", got) + } + mustSave(t, s, "second", bigToken("b")) + if got := mustLoad(t, s, "first"); got.AccessToken != "legacy" { + t.Error("legacy token lost when the store grew into chunks") + } +} + +func TestParseKeyringManifestRejectsMalformed(t *testing.T) { + digest := strings.Repeat("0", 64) + for name, head := range map[string]string{ + "too few fields": keyringManifestPrefix + "a:1:" + digest, + "unknown generation": keyringManifestPrefix + "c:1:0:" + digest, + "negative count": keyringManifestPrefix + "a:-1:0:" + digest, + "count over the cap": keyringManifestPrefix + "a:65:0:" + digest, + "non-numeric count": keyringManifestPrefix + "a:x:0:" + digest, + "live has no chunks": keyringManifestPrefix + "a:0:2:" + digest, + "short digest": keyringManifestPrefix + "a:1:0:beef", + "non-hex digest": keyringManifestPrefix + "a:1:0:" + strings.Repeat("g", 64), + } { + if _, err := parseKeyringManifest(head); err == nil { + t.Errorf("%s: parseKeyringManifest(%q) succeeded, want an error", name, head) + } + } +} + +// assertNoStrayChunks checks the invariant the whole cleanup design rests on: +// the manifest may over-state how many chunks a generation holds (a later write +// deletes the excess) but must never under-state it, because an uncounted chunk +// is a fragment of a token blob nothing will ever delete. +func assertNoStrayChunks(t *testing.T, kr *fakeKR, manifest keyringManifest) { + t.Helper() + for _, family := range []string{keyringChunkFamilyA, keyringChunkFamilyB} { + count := manifest.counts[family] + for index := 0; index < keyringMaxChunks; index++ { + account := keyringAccount + "." + family + "." + strconv.Itoa(index) + _, exists := kr.data[keyringService+"/"+account] + if family == manifest.live { + if index < count && !exists { + t.Errorf("live generation %q is missing expected chunk index %d", family, index) + } + if index >= count && exists { + t.Errorf("live generation %q has stray chunk at index %d (count %d)", family, index, count) + } + } else { + if index >= count && exists { + t.Errorf("generation %q has stray chunk at index %d (count %d)", family, index, count) + } + } + } + } + if got := len(kr.chunkAccounts(manifest.live)); got != manifest.counts[manifest.live] { + t.Errorf("live generation %q holds %d chunks, manifest says %d", manifest.live, got, manifest.counts[manifest.live]) + } +} + +// TestStoreKeyringReservesChunkRangeBeforeFilling covers the failure path the +// reservation exists for: a write that dies partway through filling a longer +// generation must still leave those chunks counted, so the next write deletes +// them instead of stranding token material in the keychain. +func TestStoreKeyringReservesChunkRangeBeforeFilling(t *testing.T) { + // A small per-entry budget makes the token blob span many chunks, so a + // failure can land in the middle of one generation. + kr := newCappedFakeKR(keyringMinChunkLen + len(keyringAccount) + 40) + s := newCappedKeyringStore(t, kr) + mustSave(t, s, "first", bigToken("a")) + + before := manifestOf(t, kr) + target := keyringChunkFamilyB + if before.live == keyringChunkFamilyB { + target = keyringChunkFamilyA + } + + boom := errors.New("keychain is locked") + kr.failSet = func(account string) error { + if account == keyringAccount+"."+target+".5" { + return boom + } + return nil + } + if err := s.Save(ProviderKey("second"), bigToken("b")); !errors.Is(err, boom) { + t.Fatalf("Save with a failing chunk write: err = %v, want %v", err, boom) + } + kr.failSet = nil + + interrupted := manifestOf(t, kr) + if interrupted.live != before.live { + t.Fatalf("a failed write moved the live generation: %q -> %q", before.live, interrupted.live) + } + if len(kr.chunkAccounts(target)) == 0 { + t.Fatal("the failed write left no chunks; the test no longer exercises the reservation") + } + assertNoStrayChunks(t, kr, interrupted) + + // A later, much smaller write into the same generation must reclaim every + // chunk the interrupted one left behind. + if _, err := s.Delete(ProviderKey("first")); err != nil { + t.Fatalf("Delete(first): %v", err) + } + mustSave(t, s, "small", Token{AccessToken: strings.Repeat("s", 900)}) + assertNoStrayChunks(t, kr, manifestOf(t, kr)) +} + +// TestStoreKeyringSweepsStrayChunksOnFirstGrowth covers the one transition the +// reservation cannot protect: while the anchor still holds the whole blob there +// is no manifest to reserve into, because writing one would destroy the only +// copy. Chunks left by an interrupted shrink are swept instead. +func TestStoreKeyringSweepsStrayChunksOnFirstGrowth(t *testing.T) { + kr := newCappedFakeKR(macOSLikeBudget) + s := newCappedKeyringStore(t, kr) + mustSave(t, s, "small", Token{AccessToken: "short"}) + + // Stand in for a shrink whose cleanup was interrupted: the anchor holds a + // whole blob, but a previous chunked generation is still on disk. + strays := []string{keyringAccount + ".a.3", keyringAccount + ".a.7"} + for _, account := range strays { + kr.data[keyringService+"/"+account] = "c3RhbGUtdG9rZW4tbWF0ZXJpYWw=" + } + + mustSave(t, s, "first", bigToken("a")) + mustSave(t, s, "second", bigToken("b")) + + assertNoStrayChunks(t, kr, manifestOf(t, kr)) + for _, account := range strays { + if _, ok := kr.data[keyringService+"/"+account]; ok { + t.Errorf("stray chunk %s survived the growth into the chunked layout", account) + } + } +} + +// TestStoreKeyringReaderDoesNotBlockOrMissDuringSlowWriter verifies that readers +// (Load, Status, FirstStored) executing concurrently with a slow writer holding +// the cross-process lock do not block or treat contention as a missing credential. +func TestStoreKeyringReaderDoesNotBlockOrMissDuringSlowWriter(t *testing.T) { + dir := t.TempDir() + t.Setenv("HOME", dir) + t.Setenv("USERPROFILE", dir) + t.Setenv("XDG_CONFIG_HOME", dir) + + kr := newCappedFakeKR(macOSLikeBudget) + writer, err := NewStore(StoreOptions{Storage: "keyring", Keyring: kr}) + if err != nil { + t.Fatalf("NewStore(writer): %v", err) + } + reader, err := NewStore(StoreOptions{Storage: "keyring", Keyring: kr}) + if err != nil { + t.Fatalf("NewStore(reader): %v", err) + } + + mustSave(t, writer, "first", bigToken("a")) + mustSave(t, writer, "second", bigToken("b")) + + // Manually acquire the lock to simulate writer holding the lock across + // manifest commit and chunk rotation. + lockPath := writer.blob.(keyringBlob).lockPath + unlock, err := acquireFileLock(lockPath, time.Now) + if err != nil { + t.Fatalf("acquireFileLock: %v", err) + } + + type loadResult struct { + tok Token + ok bool + err error + } + done := make(chan loadResult, 1) + go func() { + tok, ok, err := reader.Load(ProviderKey("first")) + done <- loadResult{tok: tok, ok: ok, err: err} + }() + + // Brief pause to ensure reader goroutine has entered acquireFileLock contention loop. + time.Sleep(50 * time.Millisecond) + + // Release lock; reader must acquire lock and complete. + unlock() + + select { + case res := <-done: + if res.err != nil { + t.Fatalf("reader.Load failed after lock release: %v", res.err) + } + if !res.ok || res.tok.AccessToken != bigToken("a").AccessToken { + t.Fatalf("reader.Load returned unexpected token: ok=%v, token=%v", res.ok, res.tok) + } + case <-time.After(2 * time.Second): + t.Fatal("reader.Load timed out waiting for lock release") + } + + firstTok, key, found := FirstStored(reader, []string{"first", "second"}) + if !found || key != ProviderKey("first") || firstTok.AccessToken != bigToken("a").AccessToken { + t.Fatalf("FirstStored returned (%v, %q, %v), want valid 'first' token", firstTok, key, found) + } + + st, err := reader.Status(KeyPrefixProvider) + if err != nil { + t.Fatalf("reader.Status failed: %v", err) + } + if len(st) != 2 { + t.Fatalf("reader.Status returned %d entries, want 2", len(st)) + } +} + +// TestStoreKeyringSharedLockAcrossDifferentEnvironmentRoots is the regression for +// split lock domains when processes run with different ZERO_OAUTH_TOKENS_PATH, +// FilePath, or XDG_CONFIG_HOME. Because the OS keychain is single-domain for the +// user ("zero"/"oauth-tokens"), all stores must share the same lock file so +// concurrent writes and manifest rotations remain strictly serialized. +func TestStoreKeyringSharedLockAcrossDifferentEnvironmentRoots(t *testing.T) { + homeDir := t.TempDir() + t.Setenv("HOME", homeDir) + t.Setenv("USERPROFILE", homeDir) + + dirA := t.TempDir() + dirB := t.TempDir() + + kr := newCappedFakeKR(macOSLikeBudget) + storeA, err := NewStore(StoreOptions{ + Storage: "keyring", + Keyring: kr, + FilePath: filepath.Join(dirA, "custom-tokens.json"), + Env: map[string]string{ + "ZERO_OAUTH_TOKENS_PATH": filepath.Join(dirA, "custom-tokens.json"), + "XDG_CONFIG_HOME": dirA, + }, + }) + if err != nil { + t.Fatalf("NewStore(storeA): %v", err) + } + + storeB, err := NewStore(StoreOptions{ + Storage: "keyring", + Keyring: kr, + FilePath: filepath.Join(dirB, "custom-tokens.json"), + Env: map[string]string{ + "ZERO_OAUTH_TOKENS_PATH": filepath.Join(dirB, "custom-tokens.json"), + "XDG_CONFIG_HOME": dirB, + }, + }) + if err != nil { + t.Fatalf("NewStore(storeB): %v", err) + } + + lockA := storeA.blob.(keyringBlob).lockPath + lockB := storeB.blob.(keyringBlob).lockPath + if lockA == "" || lockB == "" { + t.Fatalf("lockPath should not be empty: lockA=%q lockB=%q", lockA, lockB) + } + if lockA != lockB { + t.Fatalf("stores with distinct env roots must share the keyring lock: lockA=%q != lockB=%q", lockA, lockB) + } + + mustSave(t, storeA, "keyA", bigToken("a")) + + // Force overlapping save/save: write from storeB and storeA consecutively, + // verifying both updates are preserved and readable by either store. + if err := storeB.Save(ProviderKey("keyB"), bigToken("b")); err != nil { + t.Fatalf("storeB.Save: %v", err) + } + if err := storeA.Save(ProviderKey("keyC"), bigToken("c")); err != nil { + t.Fatalf("storeA.Save: %v", err) + } + + gotA := mustLoad(t, storeB, "keyA") + gotB := mustLoad(t, storeA, "keyB") + gotC := mustLoad(t, storeB, "keyC") + if gotA.AccessToken != bigToken("a").AccessToken || + gotB.AccessToken != bigToken("b").AccessToken || + gotC.AccessToken != bigToken("c").AccessToken { + t.Errorf("concurrent state was not preserved across stores") + } +} + +// TestStoreKeyringFirstMigrationFailureCleansUpWrittenChunks is the regression for +// P1: during the first migration from a whole-entry layout to a chunked layout, +// if a chunk write fails partway through, the written chunks must be cleaned up +// so future small saves (which do not sweep chunk ranges) do not leave orphaned +// token material in the keychain. +func TestStoreKeyringFirstMigrationFailureCleansUpWrittenChunks(t *testing.T) { + kr := newCappedFakeKR(macOSLikeBudget) + s := newCappedKeyringStore(t, kr) + + smallToken := Token{ + AccessToken: "small-secret-token", + TokenType: "Bearer", + Account: "user@example.com", + } + mustSave(t, s, "small", smallToken) + + // Verify initial store is a single whole entry (not chunked). + head, ok, err := kr.Get(keyringService, keyringAccount) + if err != nil || !ok { + t.Fatalf("initial Get failed: %v", err) + } + if strings.HasPrefix(head, keyringManifestPrefix) { + t.Fatal("initial small save unexpectedly created a manifest") + } + + // Inject failure on writing the second chunk during the first oversized save. + boom := errors.New("keychain write failed on chunk 1") + kr.failSet = func(account string) error { + if strings.HasSuffix(account, "."+keyringChunkFamilyA+".1") { + return boom + } + return nil + } + + // Try saving an oversized token (triggering first migration to chunked layout). + hugeToken := bigToken("x") + hugeToken.AccessToken = strings.Repeat("x", 4000) + if err := s.Save(ProviderKey("big"), hugeToken); !errors.Is(err, boom) { + t.Fatalf("Save with chunk write failure: got %v, want %v", err, boom) + } + kr.failSet = nil + + // Verify the original small token remains intact and readable. + if got := mustLoad(t, s, "small"); got.AccessToken != smallToken.AccessToken { + t.Fatalf("small token was damaged by failed migration: %v", got) + } + + // Verify no chunks survived in either chunk family. + if chunksA := kr.chunkAccounts(keyringChunkFamilyA); len(chunksA) != 0 { + t.Fatalf("chunks for family A survived failed first migration: %v", chunksA) + } + if chunksB := kr.chunkAccounts(keyringChunkFamilyB); len(chunksB) != 0 { + t.Fatalf("chunks for family B survived failed first migration: %v", chunksB) + } + + // A subsequent small save succeeds and stays whole without strays. + mustSave(t, s, "small2", smallToken) + if chunksA := kr.chunkAccounts(keyringChunkFamilyA); len(chunksA) != 0 { + t.Fatalf("chunks for family A found after subsequent small save: %v", chunksA) + } + if got := mustLoad(t, s, "small"); got.AccessToken != smallToken.AccessToken { + t.Errorf("small token not readable: %v", got) + } + if got := mustLoad(t, s, "small2"); got.AccessToken != smallToken.AccessToken { + t.Errorf("small2 token not readable: %v", got) + } +} + +// TestStoreKeyringFirstMigrationFinalManifestFailureCleansUpWrittenChunks verifies that +// if the final manifest write fails during first migration, written chunks are removed. +func TestStoreKeyringFirstMigrationFinalManifestFailureCleansUpWrittenChunks(t *testing.T) { + kr := newCappedFakeKR(macOSLikeBudget) + s := newCappedKeyringStore(t, kr) + + smallToken := Token{ + AccessToken: "small-secret-token", + TokenType: "Bearer", + Account: "user@example.com", + } + mustSave(t, s, "small", smallToken) + + boom := errors.New("keychain write failed on anchor manifest commit") + kr.failSet = func(account string) error { + if account == keyringAccount { + return boom + } + return nil + } + + hugeToken := bigToken("x") + hugeToken.AccessToken = strings.Repeat("x", 4000) + if err := s.Save(ProviderKey("big"), hugeToken); !errors.Is(err, boom) { + t.Fatalf("Save with final manifest commit failure: got %v, want %v", err, boom) + } + kr.failSet = nil + + // Verify original small token is still intact. + if got := mustLoad(t, s, "small"); got.AccessToken != smallToken.AccessToken { + t.Fatalf("small token was damaged by failed migration: %v", got) + } + + // Verify all chunks in family A were cleaned up. + if chunksA := kr.chunkAccounts(keyringChunkFamilyA); len(chunksA) != 0 { + t.Fatalf("chunks for family A survived failed first migration: %v", chunksA) + } +} + +// TestStoreKeyringCorruptStateActionableErrorAndRecovery verifies that corrupt +// manifests, missing chunks, and digest mismatches fail closed with actionable +// error messages naming the anchor and remediation command, and that Reset() +// provides a full recovery path. +func TestStoreKeyringCorruptStateActionableErrorAndRecovery(t *testing.T) { + kr := newCappedFakeKR(macOSLikeBudget) + s := newCappedKeyringStore(t, kr) + + mustSave(t, s, "first", bigToken("a")) + mustSave(t, s, "second", bigToken("b")) + + manifest := manifestOf(t, kr) + + // 1. Missing chunk error test + firstChunk := keyringAccount + "." + manifest.live + ".0" + savedChunk := kr.data[keyringService+"/"+firstChunk] + delete(kr.data, keyringService+"/"+firstChunk) + + _, _, err := s.Load(ProviderKey("first")) + if err == nil || !strings.Contains(err.Error(), "zero auth reset") || !strings.Contains(err.Error(), keyringAccount) { + t.Fatalf("Load with missing chunk did not return actionable recovery error: %v", err) + } + if err := s.Save(ProviderKey("third"), bigToken("c")); err == nil || !strings.Contains(err.Error(), "zero auth reset") { + t.Fatalf("Save with missing chunk did not fail closed with actionable recovery error: %v", err) + } + if _, err := s.Status(KeyPrefixProvider); err == nil || !strings.Contains(err.Error(), "zero auth reset") { + t.Fatalf("Status with missing chunk did not return actionable recovery error: %v", err) + } + if _, err := s.Delete(ProviderKey("first")); err == nil || !strings.Contains(err.Error(), "zero auth reset") { + t.Fatalf("Delete with missing chunk did not return actionable recovery error: %v", err) + } + + // Restore chunk + kr.data[keyringService+"/"+firstChunk] = savedChunk + + // 2. Digest mismatch test: modify one character in the chunk while keeping it valid base64 + mutated := []byte(savedChunk) + if mutated[0] == 'A' { + mutated[0] = 'B' + } else { + mutated[0] = 'A' + } + kr.data[keyringService+"/"+firstChunk] = string(mutated) + _, _, err = s.Load(ProviderKey("first")) + if err == nil || !strings.Contains(err.Error(), "failed its integrity check") || !strings.Contains(err.Error(), "zero auth reset") { + t.Fatalf("Load with digest mismatch did not return actionable recovery error: %v", err) + } + + // 3. Malformed manifest test + kr.data[keyringService+"/"+keyringAccount] = "zc1:invalid-manifest-data" + _, _, err = s.Load(ProviderKey("first")) + if err == nil || !strings.Contains(err.Error(), "malformed manifest") || !strings.Contains(err.Error(), "zero auth reset") { + t.Fatalf("Load with malformed manifest did not return actionable recovery error: %v", err) + } + if err := s.Save(ProviderKey("fourth"), bigToken("d")); err == nil || !strings.Contains(err.Error(), "zero auth reset") { + t.Fatalf("Save with malformed manifest did not fail closed: %v", err) + } + + // 4. Test recovery via Reset() + if err := s.Reset(); err != nil { + t.Fatalf("Reset() failed: %v", err) + } + + // Verify all entries in fake keyring were purged. + if len(kr.data) != 0 { + t.Fatalf("Reset did not clear all keyring entries, remaining: %v", kr.data) + } + + // 5. Fresh save and load after reset succeeds + mustSave(t, s, "fresh", bigToken("e")) + if got := mustLoad(t, s, "fresh"); got.AccessToken != bigToken("e").AccessToken { + t.Fatalf("fresh load after reset returned %v", got) + } +} + +// TestStoreKeyringShrinkResidueIsReclaimedOnRegrowth is the regression for a +// retired generation orphaned permanently. Cleanup is hygiene rather than +// correctness only while a manifest exists to state the counts; writeWhole +// replaces the anchor with the blob and the counts go with it. A shrink whose +// delete of the retired generation fails therefore leaves chunks that the next +// growth would never sweep, because that growth only ever targets family A. +// The token material in them would sit in the keychain for good. +func TestStoreKeyringShrinkResidueIsReclaimedOnRegrowth(t *testing.T) { + kr := newCappedFakeKR(macOSLikeBudget) + s := newCappedKeyringStore(t, kr) + + // Grow until family B is the live generation, so a shrink from here is the + // one that retires it. + mustSave(t, s, "first", bigToken("a")) + mustSave(t, s, "second", bigToken("b")) + mustSave(t, s, "third", bigToken("c")) + if live := manifestOf(t, kr).live; live != keyringChunkFamilyB { + t.Fatalf("live generation = %q, want %q", live, keyringChunkFamilyB) + } + + // Shrink back to a whole entry with a keychain that refuses to remove + // family B, the generation being retired on the way down. + kr.failDelete = func(account string) error { + if strings.HasPrefix(account, keyringAccount+"."+keyringChunkFamilyB+".") { + return errors.New("keychain busy") + } + return nil + } + for _, key := range []string{"second", "third"} { + if _, err := s.Delete(ProviderKey(key)); err != nil && !strings.Contains(err.Error(), "could not be removed") { + t.Fatalf("Delete(%s): %v", key, err) + } + } + kr.failDelete = nil + if head, _, _ := kr.Get(keyringService, keyringAccount); strings.HasPrefix(head, keyringManifestPrefix) { + t.Fatal("store did not shrink back to a whole entry, so the orphaning path was never taken") + } + orphans := kr.chunkAccounts(keyringChunkFamilyB) + if len(orphans) == 0 { + t.Fatal("setup did not strand any family-B chunks") + } + + // Grow back into the chunked layout. This is the only sweep that can still + // reach the stranded generation, because the manifest that named it is gone. + mustSave(t, s, "fourth", bigToken("d")) + + manifest := manifestOf(t, kr) + if manifest.live != keyringChunkFamilyA { + t.Fatalf("live generation after regrowth = %q, want %q", manifest.live, keyringChunkFamilyA) + } + if got := kr.chunkAccounts(keyringChunkFamilyB); len(got) != 0 { + t.Errorf("family B still holds %d unreferenced chunks after regrowth: %v", len(got), got) + } + assertNoStrayChunks(t, kr, manifest) + if got := mustLoad(t, s, "fourth"); got.AccessToken != bigToken("d").AccessToken { + t.Error("token stored across the regrowth did not survive") + } +} + +// TestStoreKeyringFirstMigrationRollbackFailurePreservesReclaimableCleanup tests that +// if first-migration chunk write fails and compensating deletion also fails, the +// cleanup error is joined into the return error and cleanup state is durably recorded +// such that a subsequent small save reclaims the stranded chunks. +func TestStoreKeyringFirstMigrationRollbackFailurePreservesReclaimableCleanup(t *testing.T) { + kr := newCappedFakeKR(macOSLikeBudget) + s := newCappedKeyringStore(t, kr) + + smallToken := Token{ + AccessToken: "small-secret-token", + TokenType: "Bearer", + Account: "user@example.com", + } + mustSave(t, s, "small", smallToken) + + // Inject failure on writing chunk 1, AND failure on deleting chunk 0 during rollback. + writeBoom := errors.New("keychain write failed on chunk 1") + deleteBoom := errors.New("keychain delete failed on chunk 0 during rollback") + kr.failSet = func(account string) error { + if strings.HasSuffix(account, "."+keyringChunkFamilyA+".1") { + return writeBoom + } + return nil + } + kr.failDelete = func(account string) error { + if strings.HasSuffix(account, "."+keyringChunkFamilyA+".0") { + return deleteBoom + } + return nil + } + + hugeToken := bigToken("x") + hugeToken.AccessToken = strings.Repeat("x", 4000) + err := s.Save(ProviderKey("big"), hugeToken) + if err == nil { + t.Fatal("expected Save to fail") + } + if !errors.Is(err, writeBoom) { + t.Errorf("expected error to wrap writeBoom: %v", err) + } + if !strings.Contains(err.Error(), "cleanup orphaned migration chunks") { + t.Errorf("expected error to join rollback cleanup failure: %v", err) + } + + // Verify the original small token is still intact and readable. + if got := mustLoad(t, s, "small"); got.AccessToken != smallToken.AccessToken { + t.Fatalf("small token was damaged by failed migration: %v", got) + } + + // Clear failure hooks. + kr.failSet = nil + kr.failDelete = nil + + // Verify chunk 0 was stranded initially. + if chunksA := kr.chunkAccounts(keyringChunkFamilyA); len(chunksA) != 1 { + t.Fatalf("expected 1 stranded chunk in family A before cleanup, got %v", chunksA) + } + + // Perform a subsequent small save (which uses writeWhole). + mustSave(t, s, "small2", smallToken) + + // Verify both families are now completely empty of chunks. + if chunksA := kr.chunkAccounts(keyringChunkFamilyA); len(chunksA) != 0 { + t.Fatalf("chunks for family A survived subsequent small save: %v", chunksA) + } + if chunksB := kr.chunkAccounts(keyringChunkFamilyB); len(chunksB) != 0 { + t.Fatalf("chunks for family B found after subsequent small save: %v", chunksB) + } + if _, ok, _ := kr.Get(keyringService, keyringAccount+".cleanup"); ok { + t.Fatal(".cleanup marker was not removed after sweep") + } +} + +// TestStoreKeyringResetIsBoundedByBackendAndManifest verifies that Reset only +// issues the necessary delete operations according to backend capabilities and +// known manifest layout. +func TestStoreKeyringResetIsBoundedByBackendAndManifest(t *testing.T) { + t.Run("unbounded backend issues only anchor delete", func(t *testing.T) { + kr := newFakeKR() + s := newCappedKeyringStore(t, kr) + + mustSave(t, s, "first", bigToken("a")) + mustSave(t, s, "second", bigToken("b")) + + kr.deletes = map[string]int{} + if err := s.Reset(); err != nil { + t.Fatalf("Reset: %v", err) + } + + totalDeletes := 0 + for _, count := range kr.deletes { + totalDeletes += count + } + // On unbounded backend (Linux), at most anchor + cleanup marker = 2 deletes, not 129 + if totalDeletes > 2 { + t.Errorf("unbounded reset performed %d delete calls, want <= 2", totalDeletes) + } + }) + + t.Run("bounded backend with valid manifest issues bounded chunk deletes", func(t *testing.T) { + kr := newCappedFakeKR(macOSLikeBudget) + s := newCappedKeyringStore(t, kr) + + mustSave(t, s, "first", bigToken("a")) + mustSave(t, s, "second", bigToken("b")) + + manifest := manifestOf(t, kr) + expectedChunks := manifest.counts[keyringChunkFamilyA] + manifest.counts[keyringChunkFamilyB] + if expectedChunks == 0 { + t.Fatal("expected non-zero chunks in manifest") + } + + kr.deletes = map[string]int{} + if err := s.Reset(); err != nil { + t.Fatalf("Reset: %v", err) + } + + totalDeletes := 0 + for _, count := range kr.deletes { + totalDeletes += count + } + // Chunk count + anchor + cleanup marker + if totalDeletes > expectedChunks+2 { + t.Errorf("manifest-bounded reset performed %d delete calls, want <= %d", totalDeletes, expectedChunks+2) + } + }) +} + +func TestStoreKeyringSweepCleanupAccountBoundsAndValidatesMarker(t *testing.T) { + kr := newCappedFakeKR(macOSLikeBudget) + blob := keyringBlob{kr: kr, service: keyringService, account: keyringAccount} + + // 1. Invalid family: must not delete any chunks, and marker is removed + kr.data[keyringService+"/"+blob.cleanupAccount()] = "invalid_family:5" + kr.deletes = map[string]int{} + blob.sweepCleanupAccount() + if len(kr.deletes) != 1 || kr.deletes[blob.cleanupAccount()] != 1 { + t.Errorf("expected only cleanup marker delete for invalid family, got %v", kr.deletes) + } + + // 2. Count larger than keyringMaxChunks: must be clamped to keyringMaxChunks + kr.data[keyringService+"/"+blob.cleanupAccount()] = keyringChunkFamilyA + ":1000000" + kr.deletes = map[string]int{} + blob.sweepCleanupAccount() + // Should delete chunks 0..keyringMaxChunks-1 plus the cleanup marker + expectedDeletes := keyringMaxChunks + 1 + totalDeletes := 0 + for _, c := range kr.deletes { + totalDeletes += c + } + if totalDeletes != expectedDeletes { + t.Errorf("expected %d deletes (clamped to keyringMaxChunks + marker), got %d", expectedDeletes, totalDeletes) + } + + // 3. Failed delete preserves the cleanup marker + failKR := &erroringFakeKR{ + fakeKR: newCappedFakeKR(macOSLikeBudget), + deleteErr: errors.New("keychain delete failure"), + } + blobWithErr := keyringBlob{kr: failKR, service: keyringService, account: keyringAccount} + failKR.data[keyringService+"/"+blobWithErr.cleanupAccount()] = keyringChunkFamilyA + ":3" + blobWithErr.sweepCleanupAccount() + + // Marker must still be in keychain + if val, ok := failKR.data[keyringService+"/"+blobWithErr.cleanupAccount()]; !ok || val != keyringChunkFamilyA+":3" { + t.Errorf("expected cleanup marker preserved after delete failure, got %q, ok=%v", val, ok) + } +} + +type erroringFakeKR struct { + *fakeKR + deleteErr error +} + +func (e *erroringFakeKR) Delete(service, account string) (bool, error) { + if strings.Contains(account, ".a.") && e.deleteErr != nil { + return false, e.deleteErr + } + return e.fakeKR.Delete(service, account) +} diff --git a/internal/oauth/store_keyring_test.go b/internal/oauth/store_keyring_test.go index 8931dc6de..e49a10731 100644 --- a/internal/oauth/store_keyring_test.go +++ b/internal/oauth/store_keyring_test.go @@ -1,34 +1,108 @@ package oauth import ( + "fmt" + "strconv" "strings" "testing" ) // fakeKR is an in-memory KeyringClient for exercising the keyring backend // without touching a real OS keychain. -type fakeKR struct{ data map[string]string } +type fakeKR struct { + data map[string]string + // budget mimics a backend that caps a single entry, as macOS does by + // carrying the secret on the `security -i` command line. 0 leaves the + // backend unbounded, which is Linux's secret-tool. The account is charged + // against the budget for the same reason the real one charges it: both + // share one command line, so a longer account name leaves less for the + // secret. + budget int + // failSet, when non-nil, decides whether a write to account fails, standing + // in for a locked or full keychain. + failSet func(account string) error + // sets counts successful writes per account, so a test can assert the order + // a write publishes in. + sets map[string]int + // deletes counts delete calls per account. + deletes map[string]int + // failDelete, when non-nil, decides whether removing account fails, standing + // in for a keychain that refuses a delete while it is busy or locked. + failDelete func(account string) error +} + +func newFakeKR() *fakeKR { + return &fakeKR{data: map[string]string{}, sets: map[string]int{}, deletes: map[string]int{}} +} -func newFakeKR() *fakeKR { return &fakeKR{data: map[string]string{}} } +// newCappedFakeKR returns a fake whose entries hold at most budget bytes once +// the account name is charged, mimicking the macOS keychain. +func newCappedFakeKR(budget int) *fakeKR { + f := newFakeKR() + f.budget = budget + return f +} func (f *fakeKR) Get(service, account string) (string, bool, error) { v, ok := f.data[service+"/"+account] return v, ok, nil } func (f *fakeKR) Set(service, account, secret string) error { + if f.failSet != nil { + if err := f.failSet(account); err != nil { + return err + } + } + if limit, bounded := f.MaxSecretLen(service, account); bounded && len(secret) > limit { + return fmt.Errorf("keyring: secret too large (%d > %d)", len(secret), limit) + } f.data[service+"/"+account] = secret + f.sets[account]++ return nil } func (f *fakeKR) Delete(service, account string) (bool, error) { + if f.deletes == nil { + f.deletes = map[string]int{} + } + f.deletes[account]++ + if f.failDelete != nil { + if err := f.failDelete(account); err != nil { + return false, err + } + } key := service + "/" + account _, ok := f.data[key] delete(f.data, key) return ok, nil } +func (f *fakeKR) MaxSecretLen(_, account string) (int, bool) { + if f.budget == 0 { + return 0, false + } + limit := f.budget - len(account) + if limit < 0 { + limit = 0 + } + return limit, true +} + +// chunkAccounts returns the stored chunk accounts for family, in index order. +func (f *fakeKR) chunkAccounts(family string) []string { + var accounts []string + for index := 0; index < keyringMaxChunks; index++ { + account := keyringAccount + "." + family + "." + strconv.Itoa(index) + if _, ok := f.data[keyringService+"/"+account]; ok { + accounts = append(accounts, account) + } + } + return accounts +} func TestStoreKeyringBackendRoundTrip(t *testing.T) { - // Keep the cross-process keyring lock file inside a temp config dir. - t.Setenv("XDG_CONFIG_HOME", t.TempDir()) + // Keep the cross-process keyring lock file inside a temp dir. + dir := t.TempDir() + t.Setenv("HOME", dir) + t.Setenv("USERPROFILE", dir) kr := newFakeKR() s, err := NewStore(StoreOptions{Storage: "keyring", Keyring: kr}) if err != nil { @@ -94,7 +168,9 @@ func TestNewStoreStorageSelection(t *testing.T) { } func TestStoreKeyringStatus(t *testing.T) { - t.Setenv("XDG_CONFIG_HOME", t.TempDir()) + dir := t.TempDir() + t.Setenv("HOME", dir) + t.Setenv("USERPROFILE", dir) kr := newFakeKR() s, err := NewStore(StoreOptions{Storage: "keyring", Keyring: kr}) if err != nil { diff --git a/internal/oauth/store_test.go b/internal/oauth/store_test.go index 494f35a02..a603d6887 100644 --- a/internal/oauth/store_test.go +++ b/internal/oauth/store_test.go @@ -197,3 +197,66 @@ func TestProviderKeyNormalizesCase(t *testing.T) { t.Fatalf("candidate \"xai\" must find the mixed-case login, ok=%v key=%q", ok, key) } } + +func TestStoreFileResetCleansPublicationResiduesAndSecretLock(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "tokens.json") + s, err := NewStore(StoreOptions{FilePath: path, Encrypted: true}) + if err != nil { + t.Fatalf("NewStore: %v", err) + } + + // 1. Perform initial save to establish encrypted store and .secret + if err := s.Save(ProviderKey("svc"), Token{AccessToken: "initial"}); err != nil { + t.Fatalf("Save: %v", err) + } + + // 2. Simulate crash residues: publish-* in .publish and .secret.publish, and stale .secret.lock + pubDir := path + ".publish" + secPubDir := path + ".secret.publish" + if err := os.MkdirAll(pubDir, 0o700); err != nil { + t.Fatalf("MkdirAll pubDir: %v", err) + } + if err := os.MkdirAll(secPubDir, 0o700); err != nil { + t.Fatalf("MkdirAll secPubDir: %v", err) + } + strandedToken := filepath.Join(pubDir, "publish-stranded-token") + strandedSecret := filepath.Join(secPubDir, "publish-stranded-secret") + staleLock := path + ".secret.lock" + if err := os.WriteFile(strandedToken, []byte("token-residue"), 0o600); err != nil { + t.Fatalf("write strandedToken: %v", err) + } + if err := os.WriteFile(strandedSecret, []byte("secret-residue"), 0o600); err != nil { + t.Fatalf("write strandedSecret: %v", err) + } + if err := os.WriteFile(staleLock, []byte("stale-lock"), 0o600); err != nil { + t.Fatalf("write staleLock: %v", err) + } + + // 3. Reset the store + if err := s.Reset(); err != nil { + t.Fatalf("Reset: %v", err) + } + + // 4. Verify all secret-bearing artifacts and locks are removed + for _, p := range []string{path, path + ".secret", strandedToken, strandedSecret, staleLock} { + if _, err := os.Stat(p); !os.IsNotExist(err) { + t.Errorf("file %s should be removed by Reset, stat err=%v", p, err) + } + } + // Publication dirs themselves should be preserved + for _, d := range []string{pubDir, secPubDir} { + if _, err := os.Stat(d); err != nil { + t.Errorf("publication directory %s should be preserved, stat err=%v", d, err) + } + } + + // 5. Verify fresh encrypted Save/Load succeeds + if err := s.Save(ProviderKey("svc2"), Token{AccessToken: "fresh"}); err != nil { + t.Fatalf("Save after Reset: %v", err) + } + tok, ok, err := s.Load(ProviderKey("svc2")) + if err != nil || !ok || tok.AccessToken != "fresh" { + t.Fatalf("Load after Reset = (%v, %v, %v), want fresh token", tok, ok, err) + } +}