From 285989eadd3eb244e0795358c1c92a524180482d Mon Sep 17 00:00:00 2001 From: euxaristia Date: Fri, 21 Aug 2026 22:59:03 -0400 Subject: [PATCH 1/9] fix(oauth): split an oversized keyring token blob across entries Store every provider and MCP token in one keyring entry and the store stops working once the logins outgrow it. On macOS the secret rides inside a `security -i` command line capped at 4095 bytes, which leaves 3027 bytes of JSON for all logins combined, so a second OIDC login fails to save and every write after it fails too. Split a blob that does not fit across numbered entries and put a manifest in the anchor account. Chunks live in two alternating generations: a write fills the one that is not live, then replaces the manifest, so that single write is the commit point and a crash partway through still reads the previous generation. `zc1:` cannot prefix base64, so an entry written by an existing build is still recognised and read without a migration step. Reserve the range a write will occupy before occupying it. Without that, a write interrupted while filling a longer generation leaves chunks above the count the manifest records, and no later cleanup knows to delete them: a fragment of a token blob would stay in the keychain for good. Expose the per-entry budget from internal/keyring rather than hardcoding the macOS figure in the oauth store, sharing one line builder with Set so the budget and the boundary it describes cannot drift. Backends with no limit report so and keep the single-entry layout, so Linux is untouched. Refs #937 --- internal/keyring/keyring.go | 37 +- internal/keyring/keyring_test.go | 43 ++ internal/oauth/store.go | 288 ++++++++++++- internal/oauth/store_keyring_chunked_test.go | 417 +++++++++++++++++++ internal/oauth/store_keyring_test.go | 60 ++- 5 files changed, 834 insertions(+), 11 deletions(-) create mode 100644 internal/oauth/store_keyring_chunked_test.go 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/store.go b/internal/oauth/store.go index 1bc7f1dc8..268110c58 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" @@ -104,14 +108,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 @@ -469,8 +502,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 +534,196 @@ 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) read() ([]byte, bool, error) { - enc, ok, err := b.kr.Get(b.service, b.account) + head, ok, err := b.kr.Get(b.service, b.account) if err != nil || !ok { return nil, ok, err } - data, err := base64.StdEncoding.DecodeString(strings.TrimSpace(enc)) + head = strings.TrimSpace(head) + if !strings.HasPrefix(head, keyringManifestPrefix) { + data, err := decodeKeyringBlob(head) + return data, err == nil, err + } + manifest, err := parseKeyringManifest(head) if err != nil { - return nil, false, fmt.Errorf("oauth: decode keyring token blob: %w", err) + return nil, false, err + } + count := manifest.counts[manifest.live] + var encoded strings.Builder + for index := range count { + part, ok, err := b.kr.Get(b.service, b.chunkAccount(manifest.live, index)) + if err != nil { + return nil, false, err + } + if !ok { + return nil, false, fmt.Errorf("oauth: keyring token blob at %s is missing chunk %d of %d", b.location(), index+1, count) + } + encoded.WriteString(strings.TrimSpace(part)) + } + data, err := decodeKeyringBlob(encoded.String()) + if err != nil { + return nil, false, err + } + // 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 { + return nil, false, fmt.Errorf("oauth: keyring token blob at %s failed its integrity check; the entries are inconsistent, so log in again", b.location()) } return data, true, nil } func (b keyringBlob) write(data []byte) error { - return b.kr.Set(b.service, b.account, base64.StdEncoding.EncodeToString(data)) + 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. +func (b keyringBlob) writeWhole(encoded string, previous keyringManifest) error { + if err := b.kr.Set(b.service, b.account, encoded); err != nil { + return err + } + 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) error { + 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 the target + // generation 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. + if err := b.deleteChunkRange(family, count, keyringMaxChunks, nil); 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 + } + + 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 + } + } + // 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 +} + +// 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 keyringManifest{counts: map[string]int{}}, err + } + head = strings.TrimSpace(head) + if !strings.HasPrefix(head, keyringManifestPrefix) { + return keyringManifest{counts: map[string]int{}}, nil + } + return parseKeyringManifest(head) +} + +// 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. func (b keyringBlob) withLock(now func() time.Time, fn func() error) error { if b.lockPath == "" { return fn() @@ -513,6 +738,55 @@ 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") + } + 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..f02ca9969 --- /dev/null +++ b/internal/oauth/store_keyring_chunked_test.go @@ -0,0 +1,417 @@ +package oauth + +import ( + "errors" + "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() + t.Setenv("XDG_CONFIG_HOME", t.TempDir()) + 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) + } +} + +// 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", + } { + 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} { + stored := kr.chunkAccounts(family) + if len(stored) > manifest.counts[family] { + t.Errorf("generation %q holds %d chunks (%v) but the manifest counts %d", family, len(stored), stored, manifest.counts[family]) + } + } + 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) + } + } +} diff --git a/internal/oauth/store_keyring_test.go b/internal/oauth/store_keyring_test.go index 8931dc6de..8245ce5f5 100644 --- a/internal/oauth/store_keyring_test.go +++ b/internal/oauth/store_keyring_test.go @@ -1,22 +1,56 @@ 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 +} + +func newFakeKR() *fakeKR { return &fakeKR{data: map[string]string{}, sets: 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) { @@ -25,6 +59,28 @@ func (f *fakeKR) Delete(service, account string) (bool, error) { 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. From f9aa4b22d305d46b115236e9ee3223932da76e77 Mon Sep 17 00:00:00 2001 From: euxaristia Date: Sat, 22 Aug 2026 16:58:40 -0400 Subject: [PATCH 2/9] fix(oauth): lock keyring reads, validate manifest digest, and harden chunk tests --- internal/oauth/store.go | 24 +++- internal/oauth/store_keyring_chunked_test.go | 132 ++++++++++++++++++- 2 files changed, 150 insertions(+), 6 deletions(-) diff --git a/internal/oauth/store.go b/internal/oauth/store.go index 268110c58..decda7b7b 100644 --- a/internal/oauth/store.go +++ b/internal/oauth/store.go @@ -289,11 +289,21 @@ func (s *Store) Load(key string) (Token, bool, error) { } s.mu.Lock() defer s.mu.Unlock() - state, err := s.readState() + var ( + token Token + ok bool + ) + err := s.blob.withLock(s.now, func() error { + state, err := s.readState() + if err != nil { + return err + } + token, ok = state.Tokens[key] + return nil + }) if err != nil { return Token{}, false, err } - token, ok := state.Tokens[key] return token, ok, nil } @@ -325,7 +335,12 @@ 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() + var state storeFile + err := s.blob.withLock(s.now, func() error { + var err error + state, err = s.readState() + return err + }) if err != nil { return nil, err } @@ -784,6 +799,9 @@ func parseKeyringManifest(head string) (keyringManifest, error) { 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 } diff --git a/internal/oauth/store_keyring_chunked_test.go b/internal/oauth/store_keyring_chunked_test.go index f02ca9969..1a5823bf1 100644 --- a/internal/oauth/store_keyring_chunked_test.go +++ b/internal/oauth/store_keyring_chunked_test.go @@ -2,6 +2,7 @@ package oauth import ( "errors" + "strconv" "strings" "testing" "time" @@ -200,6 +201,55 @@ func TestStoreKeyringWriteCommitsOnlyAtTheManifest(t *testing.T) { } } +// 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 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. @@ -318,6 +368,7 @@ func TestParseKeyringManifestRejectsMalformed(t *testing.T) { "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) @@ -332,9 +383,22 @@ func TestParseKeyringManifestRejectsMalformed(t *testing.T) { func assertNoStrayChunks(t *testing.T, kr *fakeKR, manifest keyringManifest) { t.Helper() for _, family := range []string{keyringChunkFamilyA, keyringChunkFamilyB} { - stored := kr.chunkAccounts(family) - if len(stored) > manifest.counts[family] { - t.Errorf("generation %q holds %d chunks (%v) but the manifest counts %d", family, len(stored), stored, manifest.counts[family]) + 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] { @@ -415,3 +479,65 @@ func TestStoreKeyringSweepsStrayChunksOnFirstGrowth(t *testing.T) { } } } + +// TestStoreKeyringReadSerializedWithLockDuringChunkedWrite verifies that readers +// executing concurrently with a chunked writer hold the cross-process lock so they +// do not observe a torn state (such as reading an old manifest after the writer +// has deleted old-generation chunks). +func TestStoreKeyringReadSerializedWithLockDuringChunkedWrite(t *testing.T) { + dir := t.TempDir() + 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) + } + + // While lock is held, Load and Status should block. + loaded := make(chan Token, 1) + loadErr := make(chan error, 1) + go func() { + tok, _, err := reader.Load(ProviderKey("first")) + loadErr <- err + loaded <- tok + }() + + // Ensure reader is waiting on lock. + select { + case <-loaded: + t.Fatal("reader.Load returned while lock was held") + case <-time.After(50 * time.Millisecond): + } + + // Release lock, allowing reader to complete. + unlock() + + select { + case err := <-loadErr: + if err != nil { + t.Fatalf("reader.Load failed after unlock: %v", err) + } + tok := <-loaded + if tok.AccessToken != bigToken("a").AccessToken { + t.Errorf("reader.Load returned unexpected token: %v", tok) + } + case <-time.After(2 * time.Second): + t.Fatal("reader.Load timed out waiting for lock release") + } +} From 340ca470693ef4361ac898025a85f0debe45c48d Mon Sep 17 00:00:00 2001 From: euxaristia Date: Sat, 22 Aug 2026 18:05:56 -0400 Subject: [PATCH 3/9] test(oauth): assert Status blocks during keyring lock regression test --- internal/oauth/store_keyring_chunked_test.go | 23 ++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/internal/oauth/store_keyring_chunked_test.go b/internal/oauth/store_keyring_chunked_test.go index 1a5823bf1..1d9e6e14a 100644 --- a/internal/oauth/store_keyring_chunked_test.go +++ b/internal/oauth/store_keyring_chunked_test.go @@ -518,10 +518,20 @@ func TestStoreKeyringReadSerializedWithLockDuringChunkedWrite(t *testing.T) { loaded <- tok }() + statused := make(chan []Status, 1) + statusErr := make(chan error, 1) + go func() { + st, err := reader.Status(KeyPrefixProvider) + statusErr <- err + statused <- st + }() + // Ensure reader is waiting on lock. select { case <-loaded: t.Fatal("reader.Load returned while lock was held") + case <-statused: + t.Fatal("reader.Status returned while lock was held") case <-time.After(50 * time.Millisecond): } @@ -540,4 +550,17 @@ func TestStoreKeyringReadSerializedWithLockDuringChunkedWrite(t *testing.T) { case <-time.After(2 * time.Second): t.Fatal("reader.Load timed out waiting for lock release") } + + select { + case err := <-statusErr: + if err != nil { + t.Fatalf("reader.Status failed after unlock: %v", err) + } + st := <-statused + if len(st) != 2 { + t.Errorf("reader.Status returned %d entries, want 2", len(st)) + } + case <-time.After(2 * time.Second): + t.Fatal("reader.Status timed out waiting for lock release") + } } From fa6ceda5c612fe90176b80cef473d44c92c25e73 Mon Sep 17 00:00:00 2001 From: euxaristia Date: Mon, 24 Aug 2026 09:07:19 -0400 Subject: [PATCH 4/9] fix(oauth): sweep both chunk generations when a store regrows MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A shrink writes the blob back under the anchor, which replaces the manifest and takes the per-generation chunk counts with it. From then on nothing can name the chunks a failed cleanup left behind, so the growth branch's sweep of the target generation is the only one that will ever reach them — and it only ever targets family A. A keychain that refused one delete during the shrink therefore kept a superseded generation of access, ID and refresh tokens indefinitely, with no way for the user to know. Sweep the other generation alongside the target, and document that the reclaim waits for the next growth: a store that shrinks once and never grows again keeps the residue, which sweeping on every whole write would close at a cost of 128 `security` invocations per save on macOS. Also record why Load and Status take the cross-process lock, and give the missing-chunk error the same "log in again" advice the digest failure carries. Refs #937 --- internal/oauth/store.go | 36 ++++++++++-- internal/oauth/store_keyring_chunked_test.go | 59 ++++++++++++++++++++ internal/oauth/store_keyring_test.go | 8 +++ 3 files changed, 99 insertions(+), 4 deletions(-) diff --git a/internal/oauth/store.go b/internal/oauth/store.go index decda7b7b..27782af5f 100644 --- a/internal/oauth/store.go +++ b/internal/oauth/store.go @@ -579,7 +579,7 @@ func (b keyringBlob) read() ([]byte, bool, error) { return nil, false, err } if !ok { - return nil, false, fmt.Errorf("oauth: keyring token blob at %s is missing chunk %d of %d", b.location(), index+1, count) + return nil, false, fmt.Errorf("oauth: keyring token blob at %s is missing chunk %d of %d; the entries are incomplete, so log in again", b.location(), index+1, count) } encoded.WriteString(strings.TrimSpace(part)) } @@ -613,6 +613,14 @@ func (b keyringBlob) write(data []byte) error { // 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 @@ -651,11 +659,24 @@ func (b keyringBlob) writeChunked(data []byte, encoded string, previous keyringM // 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 the target - // generation instead. This runs once, when a store first outgrows a + // 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. - if err := b.deleteChunkRange(family, count, keyringMaxChunks, nil); err != nil { + // + // 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] { @@ -739,6 +760,13 @@ func (b keyringBlob) chunkAccount(family string, index int) string { // 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() diff --git a/internal/oauth/store_keyring_chunked_test.go b/internal/oauth/store_keyring_chunked_test.go index 1d9e6e14a..97e7c6417 100644 --- a/internal/oauth/store_keyring_chunked_test.go +++ b/internal/oauth/store_keyring_chunked_test.go @@ -564,3 +564,62 @@ func TestStoreKeyringReadSerializedWithLockDuringChunkedWrite(t *testing.T) { t.Fatal("reader.Status timed out waiting for lock release") } } + +// 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") + } +} diff --git a/internal/oauth/store_keyring_test.go b/internal/oauth/store_keyring_test.go index 8245ce5f5..b03dc88dd 100644 --- a/internal/oauth/store_keyring_test.go +++ b/internal/oauth/store_keyring_test.go @@ -24,6 +24,9 @@ type fakeKR struct { // sets counts successful writes per account, so a test can assert the order // a write publishes in. sets 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{}} } @@ -54,6 +57,11 @@ func (f *fakeKR) Set(service, account, secret string) error { return nil } func (f *fakeKR) Delete(service, account string) (bool, error) { + 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) From f8530da82166c516e27f6da9ecdc9b7d64c3b962 Mon Sep 17 00:00:00 2001 From: euxaristia Date: Mon, 31 Aug 2026 03:38:30 -0400 Subject: [PATCH 5/9] fix(oauth): serialize keyring operations under user-level lock Derive the keyring backend lock file path from the user's home directory rather than file store configuration, ensuring processes with distinct store paths share the same lock domain for the OS keychain. Refs #938 --- internal/oauth/store.go | 47 +++++++- internal/oauth/store_keyring_chunked_test.go | 114 ++++++++++++++++++- internal/oauth/store_keyring_test.go | 10 +- 3 files changed, 162 insertions(+), 9 deletions(-) diff --git a/internal/oauth/store.go b/internal/oauth/store.go index 27782af5f..72e024bda 100644 --- a/internal/oauth/store.go +++ b/internal/oauth/store.go @@ -100,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 @@ -191,6 +195,35 @@ 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 := 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 for keyring lock: %w", err) + } + } else if !filepath.IsAbs(home) { + resolved, err := filepath.Abs(home) + if err != nil { + return "", err + } + 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) { @@ -230,11 +263,15 @@ 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 == "" { + if lp, perr := ResolveKeyringLockPath(options.Env); perr == nil { + lockPath = lp + } } return &Store{blob: keyringBlob{kr: kr, service: keyringService, account: keyringAccount, lockPath: lockPath}, now: now}, nil default: diff --git a/internal/oauth/store_keyring_chunked_test.go b/internal/oauth/store_keyring_chunked_test.go index 97e7c6417..830d58f7a 100644 --- a/internal/oauth/store_keyring_chunked_test.go +++ b/internal/oauth/store_keyring_chunked_test.go @@ -2,6 +2,7 @@ package oauth import ( "errors" + "path/filepath" "strconv" "strings" "testing" @@ -30,7 +31,10 @@ func bigToken(seed string) Token { func newCappedKeyringStore(t *testing.T, kr KeyringClient) *Store { t.Helper() - t.Setenv("XDG_CONFIG_HOME", t.TempDir()) + 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) @@ -486,6 +490,8 @@ func TestStoreKeyringSweepsStrayChunksOnFirstGrowth(t *testing.T) { // has deleted old-generation chunks). func TestStoreKeyringReadSerializedWithLockDuringChunkedWrite(t *testing.T) { dir := t.TempDir() + t.Setenv("HOME", dir) + t.Setenv("USERPROFILE", dir) t.Setenv("XDG_CONFIG_HOME", dir) kr := newCappedFakeKR(macOSLikeBudget) @@ -565,6 +571,112 @@ func TestStoreKeyringReadSerializedWithLockDuringChunkedWrite(t *testing.T) { } } +// 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 reads, 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")) + + // 1. Force overlapping save/read: hold the lock manually to simulate an + // in-flight multi-step write by storeA, and verify storeB's Load / Status blocks. + unlock, err := acquireFileLock(lockA, time.Now) + if err != nil { + t.Fatalf("acquireFileLock: %v", err) + } + + loaded := make(chan Token, 1) + loadErr := make(chan error, 1) + go func() { + tok, _, err := storeB.Load(ProviderKey("keyA")) + loadErr <- err + loaded <- tok + }() + + select { + case <-loaded: + t.Fatal("storeB.Load completed while storeA's lock was held") + case <-time.After(50 * time.Millisecond): + } + + unlock() + + select { + case err := <-loadErr: + if err != nil { + t.Fatalf("storeB.Load failed after unlock: %v", err) + } + tok := <-loaded + if tok.AccessToken != bigToken("a").AccessToken { + t.Errorf("storeB.Load got unexpected token: %v", tok) + } + case <-time.After(2 * time.Second): + t.Fatal("storeB.Load timed out waiting for lock release") + } + + // 2. 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") + } +} + // 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 diff --git a/internal/oauth/store_keyring_test.go b/internal/oauth/store_keyring_test.go index b03dc88dd..ae62dee34 100644 --- a/internal/oauth/store_keyring_test.go +++ b/internal/oauth/store_keyring_test.go @@ -91,8 +91,10 @@ func (f *fakeKR) chunkAccounts(family string) []string { } 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 { @@ -158,7 +160,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 { From 46f2392be1bcf380fa06ab569a9b84c518d9599a Mon Sep 17 00:00:00 2001 From: euxaristia Date: Tue, 1 Sep 2026 16:35:46 -0400 Subject: [PATCH 6/9] Address review feedback on chunked keyring token store and recovery Refs #938 --- internal/cli/auth.go | 40 ++- internal/cli/auth_test.go | 31 +- internal/oauth/manager.go | 8 + internal/oauth/store.go | 162 ++++++---- internal/oauth/store_keyring_chunked_test.go | 307 +++++++++++++------ 5 files changed, 401 insertions(+), 147 deletions(-) diff --git a/internal/cli/auth.go b/internal/cli/auth.go index f3ecdcc42..348cedf72 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. All stored credentials and entries cleared."); 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/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 72e024bda..c3fe0fdb4 100644 --- a/internal/oauth/store.go +++ b/internal/oauth/store.go @@ -326,21 +326,11 @@ func (s *Store) Load(key string) (Token, bool, error) { } s.mu.Lock() defer s.mu.Unlock() - var ( - token Token - ok bool - ) - err := s.blob.withLock(s.now, func() error { - state, err := s.readState() - if err != nil { - return err - } - token, ok = state.Tokens[key] - return nil - }) + state, err := s.readState() if err != nil { return Token{}, false, err } + token, ok := state.Tokens[key] return token, ok, nil } @@ -367,17 +357,21 @@ 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) { s.mu.Lock() defer s.mu.Unlock() - var state storeFile - err := s.blob.withLock(s.now, func() error { - var err error - state, err = s.readState() - return err - }) + state, err := s.readState() if err != nil { return nil, err } @@ -467,6 +461,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). @@ -479,6 +475,17 @@ 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) + } + return errors.Join(errs...) +} + func (b fileBlob) read() ([]byte, bool, error) { data, err := os.ReadFile(b.path) if err != nil { @@ -594,43 +601,69 @@ type keyringManifest struct { 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 and %q.[A|B].0..%d to recover", + b.location(), b.account, detail, b.account, b.account, keyringMaxChunks-1) +} + func (b keyringBlob) read() ([]byte, bool, error) { - 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) - return data, err == nil, err - } - manifest, err := parseKeyringManifest(head) - if err != nil { - return nil, false, err - } - count := manifest.counts[manifest.live] - var encoded strings.Builder - for index := range count { - part, ok, err := b.kr.Get(b.service, b.chunkAccount(manifest.live, index)) + 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 { - return nil, false, err + 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 !ok { - return nil, false, fmt.Errorf("oauth: keyring token blob at %s is missing chunk %d of %d; the entries are incomplete, so log in again", b.location(), index+1, count) + if chunkErr != nil { + lastErr = chunkErr + continue } - encoded.WriteString(strings.TrimSpace(part)) - } - data, err := decodeKeyringBlob(encoded.String()) - if err != nil { - return nil, false, err - } - // 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 { - return nil, false, fmt.Errorf("oauth: keyring token blob at %s failed its integrity check; the entries are inconsistent, so log in again", b.location()) + 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 data, true, nil + return nil, false, lastErr } func (b keyringBlob) write(data []byte) error { @@ -669,7 +702,7 @@ func (b keyringBlob) writeWhole(encoded string, previous keyringManifest) error return nil } -func (b keyringBlob) writeChunked(data []byte, encoded string, previous keyringManifest) error { +func (b keyringBlob) writeChunked(data []byte, encoded string, previous keyringManifest) (retErr error) { family := keyringChunkFamilyA if previous.live == keyringChunkFamilyA { family = keyringChunkFamilyB @@ -725,11 +758,22 @@ func (b keyringBlob) writeChunked(data []byte, encoded string, previous keyringM 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. + _ = b.deleteChunkRange(family, 0, writtenChunks, nil) + } + }() + 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 @@ -759,6 +803,16 @@ func (b keyringBlob) writeChunked(data []byte, encoded string, previous keyringM return nil } +func (b keyringBlob) reset() error { + var err error + err = b.deleteChunkRange(keyringChunkFamilyA, 0, keyringMaxChunks, nil) + err = b.deleteChunkRange(keyringChunkFamilyB, 0, keyringMaxChunks, err) + if _, kerr := b.kr.Delete(b.service, b.account); kerr != nil && err == nil { + err = fmt.Errorf("remove %s: %w", b.account, kerr) + } + return err +} + // 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 @@ -772,7 +826,11 @@ func (b keyringBlob) readManifest() (keyringManifest, error) { if !strings.HasPrefix(head, keyringManifestPrefix) { return keyringManifest{counts: map[string]int{}}, nil } - return parseKeyringManifest(head) + m, err := parseKeyringManifest(head) + if err != nil { + return keyringManifest{counts: map[string]int{}}, b.corruptError("has a malformed manifest (" + err.Error() + ")") + } + return m, nil } // deleteChunkRange removes chunks [from, to) of family, joining onto prior. It diff --git a/internal/oauth/store_keyring_chunked_test.go b/internal/oauth/store_keyring_chunked_test.go index 830d58f7a..7565614c2 100644 --- a/internal/oauth/store_keyring_chunked_test.go +++ b/internal/oauth/store_keyring_chunked_test.go @@ -235,6 +235,12 @@ func TestStoreKeyringWriteFailsOnFinalManifestPublication(t *testing.T) { 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") @@ -484,11 +490,10 @@ func TestStoreKeyringSweepsStrayChunksOnFirstGrowth(t *testing.T) { } } -// TestStoreKeyringReadSerializedWithLockDuringChunkedWrite verifies that readers -// executing concurrently with a chunked writer hold the cross-process lock so they -// do not observe a torn state (such as reading an old manifest after the writer -// has deleted old-generation chunks). -func TestStoreKeyringReadSerializedWithLockDuringChunkedWrite(t *testing.T) { +// 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) @@ -515,67 +520,37 @@ func TestStoreKeyringReadSerializedWithLockDuringChunkedWrite(t *testing.T) { t.Fatalf("acquireFileLock: %v", err) } - // While lock is held, Load and Status should block. - loaded := make(chan Token, 1) - loadErr := make(chan error, 1) - go func() { - tok, _, err := reader.Load(ProviderKey("first")) - loadErr <- err - loaded <- tok - }() - - statused := make(chan []Status, 1) - statusErr := make(chan error, 1) - go func() { - st, err := reader.Status(KeyPrefixProvider) - statusErr <- err - statused <- st - }() - - // Ensure reader is waiting on lock. - select { - case <-loaded: - t.Fatal("reader.Load returned while lock was held") - case <-statused: - t.Fatal("reader.Status returned while lock was held") - case <-time.After(50 * time.Millisecond): - } - - // Release lock, allowing reader to complete. - unlock() + // While lock is held by writer, reader operations (Load, Status, FirstStored) + // should complete immediately without blocking or timing out. + tok, ok, err := reader.Load(ProviderKey("first")) + if err != nil { + t.Fatalf("reader.Load failed while writer held lock: %v", err) + } + if !ok || tok.AccessToken != bigToken("a").AccessToken { + t.Fatalf("reader.Load returned unexpected token while writer held lock: ok=%v, token=%v", ok, tok) + } - select { - case err := <-loadErr: - if err != nil { - t.Fatalf("reader.Load failed after unlock: %v", err) - } - tok := <-loaded - if tok.AccessToken != bigToken("a").AccessToken { - t.Errorf("reader.Load returned unexpected token: %v", 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) } - select { - case err := <-statusErr: - if err != nil { - t.Fatalf("reader.Status failed after unlock: %v", err) - } - st := <-statused - if len(st) != 2 { - t.Errorf("reader.Status returned %d entries, want 2", len(st)) - } - case <-time.After(2 * time.Second): - t.Fatal("reader.Status timed out waiting for lock release") + st, err := reader.Status(KeyPrefixProvider) + if err != nil { + t.Fatalf("reader.Status failed while writer held lock: %v", err) + } + if len(st) != 2 { + t.Fatalf("reader.Status returned %d entries, want 2", len(st)) } + + unlock() } // 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 reads, writes, and manifest rotations remain strictly serialized. +// concurrent writes and manifest rotations remain strictly serialized. func TestStoreKeyringSharedLockAcrossDifferentEnvironmentRoots(t *testing.T) { homeDir := t.TempDir() t.Setenv("HOME", homeDir) @@ -622,43 +597,7 @@ func TestStoreKeyringSharedLockAcrossDifferentEnvironmentRoots(t *testing.T) { mustSave(t, storeA, "keyA", bigToken("a")) - // 1. Force overlapping save/read: hold the lock manually to simulate an - // in-flight multi-step write by storeA, and verify storeB's Load / Status blocks. - unlock, err := acquireFileLock(lockA, time.Now) - if err != nil { - t.Fatalf("acquireFileLock: %v", err) - } - - loaded := make(chan Token, 1) - loadErr := make(chan error, 1) - go func() { - tok, _, err := storeB.Load(ProviderKey("keyA")) - loadErr <- err - loaded <- tok - }() - - select { - case <-loaded: - t.Fatal("storeB.Load completed while storeA's lock was held") - case <-time.After(50 * time.Millisecond): - } - - unlock() - - select { - case err := <-loadErr: - if err != nil { - t.Fatalf("storeB.Load failed after unlock: %v", err) - } - tok := <-loaded - if tok.AccessToken != bigToken("a").AccessToken { - t.Errorf("storeB.Load got unexpected token: %v", tok) - } - case <-time.After(2 * time.Second): - t.Fatal("storeB.Load timed out waiting for lock release") - } - - // 2. Force overlapping save/save: write from storeB and storeA consecutively, + // 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) @@ -677,6 +616,188 @@ func TestStoreKeyringSharedLockAcrossDifferentEnvironmentRoots(t *testing.T) { } } +// 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 From f5ce8edbaca92648f85d0823a17223e31f6b5f58 Mon Sep 17 00:00:00 2001 From: euxaristia Date: Tue, 1 Sep 2026 17:54:50 -0400 Subject: [PATCH 7/9] Refine auth reset message and chunk format notation Refs #938 --- internal/cli/auth.go | 2 +- internal/oauth/store.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/cli/auth.go b/internal/cli/auth.go index 348cedf72..7b484c3e9 100644 --- a/internal/cli/auth.go +++ b/internal/cli/auth.go @@ -600,7 +600,7 @@ func runAuthReset(args []string, stdout io.Writer, stderr io.Writer, deps appDep } return exitSuccess } - if _, err := fmt.Fprintln(stdout, "Reset OAuth token store. All stored credentials and entries cleared."); err != nil { + if _, err := fmt.Fprintln(stdout, "Reset OAuth token store."); err != nil { return exitCrash } return exitSuccess diff --git a/internal/oauth/store.go b/internal/oauth/store.go index c3fe0fdb4..b2d114677 100644 --- a/internal/oauth/store.go +++ b/internal/oauth/store.go @@ -602,7 +602,7 @@ type keyringManifest struct { } 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 and %q.[A|B].0..%d to recover", + return fmt.Errorf("oauth: keyring token data at %s (account %q) %s; run `zero auth reset` or remove entries %q and %q.[a|b].0..%d to recover", b.location(), b.account, detail, b.account, b.account, keyringMaxChunks-1) } From 3142213f1a97c151aa8b7107c498aa9c7d941e57 Mon Sep 17 00:00:00 2001 From: euxaristia Date: Wed, 2 Sep 2026 04:53:44 -0400 Subject: [PATCH 8/9] Harden OAuth store reset lifecycle, keyring migration recovery, and completions. Refs #938 --- internal/cli/completions.go | 2 +- internal/cli/completions_test.go | 1 + internal/oauth/store.go | 103 ++++++++++++--- internal/oauth/store_keyring_chunked_test.go | 128 +++++++++++++++++++ internal/oauth/store_keyring_test.go | 10 +- internal/oauth/store_test.go | 63 +++++++++ 6 files changed, 284 insertions(+), 23 deletions(-) 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/oauth/store.go b/internal/oauth/store.go index b2d114677..fc2670ce6 100644 --- a/internal/oauth/store.go +++ b/internal/oauth/store.go @@ -207,18 +207,11 @@ func ResolveKeyringLockPath(env map[string]string) (string, error) { } return filepath.Abs(override) } - 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 for keyring lock: %w", err) - } - } else if !filepath.IsAbs(home) { - resolved, err := filepath.Abs(home) - if err != nil { - return "", err - } + 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 @@ -483,6 +476,21 @@ func (b fileBlob) reset() error { 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 { + 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...) } @@ -602,8 +610,8 @@ type keyringManifest struct { } 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 and %q.[a|b].0..%d to recover", - b.location(), b.account, detail, b.account, b.account, keyringMaxChunks-1) + 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) { @@ -695,6 +703,7 @@ 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) @@ -703,6 +712,7 @@ func (b keyringBlob) writeWhole(encoded string, previous keyringManifest) error } func (b keyringBlob) writeChunked(data []byte, encoded string, previous keyringManifest) (retErr error) { + b.sweepCleanupAccount() family := keyringChunkFamilyA if previous.live == keyringChunkFamilyA { family = keyringChunkFamilyB @@ -764,7 +774,12 @@ func (b keyringBlob) writeChunked(data []byte, encoded string, previous keyringM // 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. - _ = b.deleteChunkRange(family, 0, writtenChunks, nil) + 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()) + } } }() @@ -804,13 +819,59 @@ func (b keyringBlob) writeChunked(data []byte, encoded string, previous keyringM } func (b keyringBlob) reset() error { - var err error - err = b.deleteChunkRange(keyringChunkFamilyA, 0, keyringMaxChunks, nil) - err = b.deleteChunkRange(keyringChunkFamilyB, 0, keyringMaxChunks, err) - if _, kerr := b.kr.Delete(b.service, b.account); kerr != nil && err == nil { - err = fmt.Errorf("remove %s: %w", b.account, kerr) + 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 count, err := strconv.Atoi(parts[1]); err == nil && count > 0 { + _ = b.deleteChunkRange(family, 0, count, nil) + } + } } - return err + _, _ = b.kr.Delete(b.service, b.cleanupAccount()) } // readManifest returns the live manifest, or a zero manifest when the anchor diff --git a/internal/oauth/store_keyring_chunked_test.go b/internal/oauth/store_keyring_chunked_test.go index 7565614c2..75541caa6 100644 --- a/internal/oauth/store_keyring_chunked_test.go +++ b/internal/oauth/store_keyring_chunked_test.go @@ -856,3 +856,131 @@ func TestStoreKeyringShrinkResidueIsReclaimedOnRegrowth(t *testing.T) { 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) + } + }) +} diff --git a/internal/oauth/store_keyring_test.go b/internal/oauth/store_keyring_test.go index ae62dee34..e49a10731 100644 --- a/internal/oauth/store_keyring_test.go +++ b/internal/oauth/store_keyring_test.go @@ -24,12 +24,16 @@ type fakeKR struct { // 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{}} } +func newFakeKR() *fakeKR { + return &fakeKR{data: map[string]string{}, sets: map[string]int{}, deletes: map[string]int{}} +} // newCappedFakeKR returns a fake whose entries hold at most budget bytes once // the account name is charged, mimicking the macOS keychain. @@ -57,6 +61,10 @@ func (f *fakeKR) Set(service, account, secret string) error { 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 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) + } +} From 95732af39fa9e5a5d997d00f0afcd0dc8241436d Mon Sep 17 00:00:00 2001 From: euxaristia Date: Fri, 4 Sep 2026 14:53:02 -0400 Subject: [PATCH 9/9] Validate keyring cleanup markers, propagate lock resolution errors, and test reader concurrency. Refs #938 --- internal/oauth/store.go | 33 ++++--- internal/oauth/store_keyring_chunked_test.go | 90 +++++++++++++++++--- 2 files changed, 103 insertions(+), 20 deletions(-) diff --git a/internal/oauth/store.go b/internal/oauth/store.go index fc2670ce6..a941e9e36 100644 --- a/internal/oauth/store.go +++ b/internal/oauth/store.go @@ -262,9 +262,11 @@ func NewStore(options StoreOptions) (*Store, error) { // home location resolves, fall back to in-process serialization only. lockPath := strings.TrimSpace(options.KeyringLockPath) if lockPath == "" { - if lp, perr := ResolveKeyringLockPath(options.Env); perr == nil { - lockPath = lp + 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: @@ -478,12 +480,16 @@ func (b fileBlob) reset() error { } for _, dir := range []string{b.path + ".publish", b.path + ".secret.publish"} { entries, err := os.ReadDir(dir) - if err == nil { - 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 != 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) } } } @@ -866,8 +872,15 @@ func (b keyringBlob) sweepCleanupAccount() { parts := strings.Split(raw, ":") if len(parts) == 2 { family := parts[0] - if count, err := strconv.Atoi(parts[1]); err == nil && count > 0 { - _ = b.deleteChunkRange(family, 0, count, nil) + 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 + } + } } } } diff --git a/internal/oauth/store_keyring_chunked_test.go b/internal/oauth/store_keyring_chunked_test.go index 75541caa6..81c20f0a8 100644 --- a/internal/oauth/store_keyring_chunked_test.go +++ b/internal/oauth/store_keyring_chunked_test.go @@ -520,14 +520,33 @@ func TestStoreKeyringReaderDoesNotBlockOrMissDuringSlowWriter(t *testing.T) { t.Fatalf("acquireFileLock: %v", err) } - // While lock is held by writer, reader operations (Load, Status, FirstStored) - // should complete immediately without blocking or timing out. - tok, ok, err := reader.Load(ProviderKey("first")) - if err != nil { - t.Fatalf("reader.Load failed while writer held lock: %v", err) + type loadResult struct { + tok Token + ok bool + err error } - if !ok || tok.AccessToken != bigToken("a").AccessToken { - t.Fatalf("reader.Load returned unexpected token while writer held lock: ok=%v, token=%v", ok, tok) + 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"}) @@ -537,13 +556,11 @@ func TestStoreKeyringReaderDoesNotBlockOrMissDuringSlowWriter(t *testing.T) { st, err := reader.Status(KeyPrefixProvider) if err != nil { - t.Fatalf("reader.Status failed while writer held lock: %v", err) + t.Fatalf("reader.Status failed: %v", err) } if len(st) != 2 { t.Fatalf("reader.Status returned %d entries, want 2", len(st)) } - - unlock() } // TestStoreKeyringSharedLockAcrossDifferentEnvironmentRoots is the regression for @@ -984,3 +1001,56 @@ func TestStoreKeyringResetIsBoundedByBackendAndManifest(t *testing.T) { } }) } + +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) +}