fix(auth): repair duplicate token alias writes (#721)

TurboTheTurtle · web-flow · commit ebc8218b609a · 2026-06-09T21:07:41.000-07:00
Fixes #718. Co-authored-by: Andy Ye <35905412+TurboTheTurtle@users.noreply.github.com>
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -10,6 +10,7 @@
 ### Fixed
 
 - Docs: avoid duplicate empty paragraphs adjacent to Markdown headings while preserving body paragraph spacing. (#717, #720) — thanks @TurboTheTurtle.
+- Auth: repair duplicate macOS Keychain writes for legacy and subject token aliases without weakening primary token persistence. (#718, #721) — thanks @TurboTheTurtle.
 
 ## 0.23.0 - 2026-06-09
 
diff --git a/internal/secrets/store.go b/internal/secrets/store.go
@@ -505,13 +505,13 @@ func (s *KeyringStore) setTokenNoLock(client string, email string, tok Token) er
 	}
 
 	if normalizedClient == config.DefaultClientName {
-		if err := verifiedSet(s.ring, legacyTokenKey(email), payload, "legacy token"); err != nil {
+		if err := verifiedSetAlias(s.ring, legacyTokenKey(email), payload, "legacy token"); err != nil {
 			return wrapKeychainError(fmt.Errorf("store legacy token: %w", err))
 		}
 	}
 
 	if tok.Subject != "" {
-		if err := verifiedSet(s.ring, subjectTokenKey(normalizedClient, tok.Subject), payload, "subject token"); err != nil {
+		if err := verifiedSetAlias(s.ring, subjectTokenKey(normalizedClient, tok.Subject), payload, "subject token"); err != nil {
 			return wrapKeychainError(fmt.Errorf("store subject token: %w", err))
 		}
 	}
@@ -1017,3 +1017,33 @@ func verifiedSet(ring keyring.Keyring, key string, data []byte, label string) er
 
 	return nil
 }
+
+func verifiedSetAlias(ring keyring.Keyring, key string, data []byte, label string) error {
+	if err := verifiedSet(ring, key, data, label); err != nil {
+		if !isDuplicateKeyringItemError(err) {
+			return err
+		}
+
+		if removeErr := ring.Remove(key); removeErr != nil && !errors.Is(removeErr, keyring.ErrKeyNotFound) {
+			return fmt.Errorf("replace duplicate %s: remove stale item: %w", label, removeErr)
+		}
+
+		if retryErr := verifiedSet(ring, key, data, label); retryErr != nil {
+			return fmt.Errorf("replace duplicate %s: %w", label, retryErr)
+		}
+	}
+
+	return nil
+}
+
+func isDuplicateKeyringItemError(err error) bool {
+	if err == nil {
+		return false
+	}
+
+	msg := strings.ToLower(err.Error())
+
+	return strings.Contains(msg, "-25299") ||
+		strings.Contains(msg, "errsecduplicateitem") ||
+		strings.Contains(msg, "specified item already exists")
+}
diff --git a/internal/secrets/store_more_test.go b/internal/secrets/store_more_test.go
@@ -1,6 +1,7 @@
 package secrets
 
 import (
+	"bytes"
 	"encoding/json"
 	"errors"
 	"fmt"
@@ -15,8 +16,9 @@ import (
 )
 
 var (
-	errTestKeychain = errors.New("test -25308 error")
-	errTestReadBack = errors.New("test read-back failure")
+	errTestDuplicateKeychain = errors.New("failed to update item in keychain: the specified item already exists in the keychain. (-25299)")
+	errTestKeychain          = errors.New("test -25308 error")
+	errTestReadBack          = errors.New("test read-back failure")
 )
 
 func TestKeyringStore_ListDeleteDefault(t *testing.T) {
@@ -321,6 +323,111 @@ func TestKeyringStoreTokenAccessTokenRoundTrip(t *testing.T) {
 	}
 }
 
+func TestKeyringStoreSetTokenRepairsDuplicateAliasWrites(t *testing.T) {
+	email := "a@b.com"
+	subject := "google-sub-123"
+	expires := time.Date(2026, 6, 9, 16, 0, 42, 0, time.UTC)
+
+	ring := &duplicateOnceKeyring{
+		ArrayKeyring: keyring.NewArrayKeyring(nil),
+		duplicateKeys: map[string]int{
+			legacyTokenKey(email):                              1,
+			subjectTokenKey(config.DefaultClientName, subject): 1,
+		},
+		removedKeys: map[string]int{},
+	}
+
+	for _, key := range []string{
+		legacyTokenKey(email),
+		subjectTokenKey(config.DefaultClientName, subject),
+	} {
+		if err := ring.ArrayKeyring.Set(keyringItem(key, []byte("stale"))); err != nil {
+			t.Fatalf("seed stale alias %q: %v", key, err)
+		}
+	}
+
+	store := &KeyringStore{ring: ring}
+
+	err := store.SetToken(config.DefaultClientName, email, Token{
+		Subject:              subject,
+		RefreshToken:         "rt",
+		AccessToken:          "at",
+		AccessTokenExpiresAt: expires,
+	})
+	if err != nil {
+		t.Fatalf("SetToken: %v", err)
+	}
+
+	primary, err := ring.Get(tokenKey(config.DefaultClientName, email))
+	if err != nil {
+		t.Fatalf("read primary token: %v", err)
+	}
+
+	for _, key := range []string{
+		legacyTokenKey(email),
+		subjectTokenKey(config.DefaultClientName, subject),
+	} {
+		item, getErr := ring.Get(key)
+		if getErr != nil {
+			t.Fatalf("expected key %q persisted after duplicate repair: %v", key, getErr)
+		}
+
+		if !bytes.Equal(item.Data, primary.Data) {
+			t.Fatalf("alias %q was not replaced with primary payload", key)
+		}
+
+		if ring.removedKeys[key] != 1 {
+			t.Fatalf("alias %q remove count = %d, want 1", key, ring.removedKeys[key])
+		}
+	}
+
+	got, err := store.GetToken(config.DefaultClientName, email)
+	if err != nil {
+		t.Fatalf("GetToken: %v", err)
+	}
+
+	if got.AccessToken != "at" || !got.AccessTokenExpiresAt.Equal(expires) {
+		t.Fatalf("refreshed access metadata was not preserved: %#v", got)
+	}
+}
+
+func TestKeyringStoreSetTokenKeepsPrimaryDuplicateStrict(t *testing.T) {
+	email := "a@b.com"
+	primaryKey := tokenKey(config.DefaultClientName, email)
+
+	ring := &duplicateOnceKeyring{
+		ArrayKeyring: keyring.NewArrayKeyring(nil),
+		duplicateKeys: map[string]int{
+			primaryKey: 1,
+		},
+		removedKeys: map[string]int{},
+	}
+
+	if err := ring.ArrayKeyring.Set(keyringItem(primaryKey, []byte("stale-primary"))); err != nil {
+		t.Fatalf("seed stale primary: %v", err)
+	}
+
+	store := &KeyringStore{ring: ring}
+
+	err := store.SetToken(config.DefaultClientName, email, Token{RefreshToken: "rt"})
+	if err == nil || !isDuplicateKeyringItemError(err) {
+		t.Fatalf("expected primary duplicate error, got %v", err)
+	}
+
+	if ring.removedKeys[primaryKey] != 0 {
+		t.Fatalf("primary token was removed during strict write")
+	}
+
+	item, getErr := ring.Get(primaryKey)
+	if getErr != nil {
+		t.Fatalf("read primary token: %v", getErr)
+	}
+
+	if string(item.Data) != "stale-primary" {
+		t.Fatalf("primary token changed after failed strict write: %q", item.Data)
+	}
+}
+
 func TestKeyringStoreDeleteTokenAliasPreservesSubjectKey(t *testing.T) {
 	ring := keyring.NewArrayKeyring(nil)
 	store := &KeyringStore{ring: ring}
@@ -494,6 +601,35 @@ func (r *readBackErrorKeyring) Get(_ string) (keyring.Item, error) {
 }
 func (r *readBackErrorKeyring) Keys() ([]string, error) { return nil, nil }
 
+type duplicateOnceKeyring struct {
+	*keyring.ArrayKeyring
+	duplicateKeys map[string]int
+	removedKeys   map[string]int
+}
+
+func (d *duplicateOnceKeyring) Set(item keyring.Item) error {
+	if remaining := d.duplicateKeys[item.Key]; remaining > 0 {
+		d.duplicateKeys[item.Key] = remaining - 1
+		return errTestDuplicateKeychain
+	}
+
+	if err := d.ArrayKeyring.Set(item); err != nil {
+		return fmt.Errorf("set array keyring item: %w", err)
+	}
+
+	return nil
+}
+
+func (d *duplicateOnceKeyring) Remove(key string) error {
+	d.removedKeys[key]++
+
+	if err := d.ArrayKeyring.Remove(key); err != nil {
+		return fmt.Errorf("remove array keyring item: %w", err)
+	}
+
+	return nil
+}
+
 func TestSetTokenVerifyCatchesReadBackError(t *testing.T) {
 	store := &KeyringStore{ring: &readBackErrorKeyring{}}
 	client := config.DefaultClientName
