diff --git a/internal/secrets/file_keyring_safe.go b/internal/secrets/file_keyring_safe.go index 1424d26..f213e6d 100644 --- a/internal/secrets/file_keyring_safe.go +++ b/internal/secrets/file_keyring_safe.go @@ -3,6 +3,7 @@ package secrets import ( "encoding/base64" "errors" + "fmt" "io/fs" "sort" "strings" @@ -61,8 +62,9 @@ func (k *fileSafeKeyring) Get(key string) (keyring.Item, error) { item.Key = key return item, nil } + if !fileKeyNotFound(err) { - return keyring.Item{}, err + return keyring.Item{}, fmt.Errorf("read encoded file keyring item: %w", err) } item, legacyErr := k.inner.Get(key) @@ -70,10 +72,12 @@ func (k *fileSafeKeyring) Get(key string) (keyring.Item, error) { if fileKeyNotFound(legacyErr) { return keyring.Item{}, keyring.ErrKeyNotFound } - return keyring.Item{}, legacyErr + + return keyring.Item{}, fmt.Errorf("read legacy file keyring item: %w", legacyErr) } item.Key = key + return item, nil } @@ -82,8 +86,9 @@ func (k *fileSafeKeyring) GetMetadata(key string) (keyring.Metadata, error) { if err == nil { return meta, nil } + if !fileKeyNotFound(err) { - return keyring.Metadata{}, err + return keyring.Metadata{}, fmt.Errorf("read encoded file keyring metadata: %w", err) } meta, legacyErr := k.inner.GetMetadata(key) @@ -91,7 +96,8 @@ func (k *fileSafeKeyring) GetMetadata(key string) (keyring.Metadata, error) { if fileKeyNotFound(legacyErr) { return keyring.Metadata{}, keyring.ErrKeyNotFound } - return keyring.Metadata{}, legacyErr + + return keyring.Metadata{}, fmt.Errorf("read legacy file keyring metadata: %w", legacyErr) } return meta, nil @@ -99,7 +105,11 @@ func (k *fileSafeKeyring) GetMetadata(key string) (keyring.Metadata, error) { func (k *fileSafeKeyring) Set(item keyring.Item) error { item.Key = fileSafeKey(item.Key) - return k.inner.Set(item) + if err := k.inner.Set(item); err != nil { + return fmt.Errorf("store file keyring item: %w", err) + } + + return nil } func (k *fileSafeKeyring) Remove(key string) error { @@ -109,22 +119,26 @@ func (k *fileSafeKeyring) Remove(key string) error { if encodedErr == nil || legacyErr == nil { return nil } + if fileKeyNotFound(encodedErr) && fileKeyNotFound(legacyErr) { return keyring.ErrKeyNotFound } + if !fileKeyNotFound(encodedErr) { - return encodedErr + return fmt.Errorf("remove encoded file keyring item: %w", encodedErr) } - return legacyErr + + return fmt.Errorf("remove legacy file keyring item: %w", legacyErr) } func (k *fileSafeKeyring) Keys() ([]string, error) { keys, err := k.inner.Keys() if err != nil { - return nil, err + return nil, fmt.Errorf("list file keyring keys: %w", err) } out := make([]string, 0, len(keys)) + seen := make(map[string]struct{}, len(keys)) for _, key := range keys { decoded := decodeFileSafeKey(key) @@ -134,6 +148,7 @@ func (k *fileSafeKeyring) Keys() ([]string, error) { seen[decoded] = struct{}{} out = append(out, decoded) } + sort.Strings(out) return out, nil diff --git a/internal/secrets/file_keyring_safe_test.go b/internal/secrets/file_keyring_safe_test.go index d0e0275..c4f3e93 100644 --- a/internal/secrets/file_keyring_safe_test.go +++ b/internal/secrets/file_keyring_safe_test.go @@ -27,9 +27,11 @@ func TestFileSafeKeyRoundTrip(t *testing.T) { if encoded == key { t.Fatalf("expected encoded key for %q", key) } + if strings.ContainsAny(encoded, `<>:"/\|?*`) { t.Fatalf("encoded key %q still contains a Windows filename separator/reserved char", encoded) } + if got := decodeFileSafeKey(encoded); got != key { t.Fatalf("decodeFileSafeKey(%q)=%q, want %q", encoded, got, key) } @@ -39,6 +41,7 @@ func TestFileSafeKeyRoundTrip(t *testing.T) { if got := decodeFileSafeKey(rawPrefixKey); got != "test" { t.Fatalf("expected canonical encoded key to decode, got %q", got) } + if got := decodeFileSafeKey(fileKeyPrefix + "not valid"); got != fileKeyPrefix+"not valid" { t.Fatalf("expected invalid encoded key to remain raw, got %q", got) } @@ -46,6 +49,7 @@ func TestFileSafeKeyRoundTrip(t *testing.T) { func TestFileSafeKeyringRoundTripWithFileBackend(t *testing.T) { dir := t.TempDir() + inner, err := keyring.Open(keyring.Config{ ServiceName: keyringServiceName(), AllowedBackends: []keyring.BackendType{keyring.FileBackend}, @@ -58,14 +62,16 @@ func TestFileSafeKeyringRoundTripWithFileBackend(t *testing.T) { ring := newFileSafeKeyring(inner) key := "token:default:user@example.com" - if err := ring.Set(keyring.Item{Key: key, Data: []byte("secret")}); err != nil { - t.Fatalf("Set: %v", err) + + if setErr := ring.Set(keyring.Item{Key: key, Data: []byte("secret")}); setErr != nil { + t.Fatalf("Set: %v", setErr) } item, err := ring.Get(key) if err != nil { t.Fatalf("Get: %v", err) } + if item.Key != key || string(item.Data) != "secret" { t.Fatalf("unexpected item: key=%q data=%q", item.Key, item.Data) } @@ -74,6 +80,7 @@ func TestFileSafeKeyringRoundTripWithFileBackend(t *testing.T) { if err != nil { t.Fatalf("Keys: %v", err) } + if !slices.Contains(keys, key) { t.Fatalf("expected decoded key %q in %v", key, keys) } @@ -82,9 +89,11 @@ func TestFileSafeKeyringRoundTripWithFileBackend(t *testing.T) { if err != nil { t.Fatalf("ReadDir: %v", err) } + if len(entries) != 1 { t.Fatalf("expected one keyring file, got %d", len(entries)) } + if name := entries[0].Name(); strings.ContainsAny(name, `<>:"/\|?*`) { t.Fatalf("keyring filename %q contains a Windows filename separator/reserved char", name) } @@ -92,6 +101,7 @@ func TestFileSafeKeyringRoundTripWithFileBackend(t *testing.T) { if err := ring.Remove(key); err != nil { t.Fatalf("Remove: %v", err) } + if _, err := ring.Get(key); !errors.Is(err, keyring.ErrKeyNotFound) { t.Fatalf("expected not found after remove, got %v", err) } @@ -110,24 +120,28 @@ func TestFileSafeKeyringReadsAndRemovesLegacyRawKeys(t *testing.T) { if err != nil { t.Fatalf("Get legacy key: %v", err) } + if item.Key != key || string(item.Data) != "legacy" { t.Fatalf("unexpected legacy item: key=%q data=%q", item.Key, item.Data) } - if err := ring.Set(keyring.Item{Key: key, Data: []byte("new")}); err != nil { - t.Fatalf("Set encoded key: %v", err) + if setErr := ring.Set(keyring.Item{Key: key, Data: []byte("new")}); setErr != nil { + t.Fatalf("Set encoded key: %v", setErr) } keys, err := ring.Keys() if err != nil { t.Fatalf("Keys: %v", err) } + got := 0 + for _, listedKey := range keys { if listedKey == key { got++ } } + if got != 1 { t.Fatalf("expected one decoded key in %v, got count %d", keys, got) } @@ -135,9 +149,11 @@ func TestFileSafeKeyringReadsAndRemovesLegacyRawKeys(t *testing.T) { if err := ring.Remove(key); err != nil { t.Fatalf("Remove: %v", err) } + if _, err := inner.Get(key); !errors.Is(err, keyring.ErrKeyNotFound) { t.Fatalf("expected legacy key removed, got %v", err) } + if _, err := inner.Get(fileSafeKey(key)); !errors.Is(err, keyring.ErrKeyNotFound) { t.Fatalf("expected encoded key removed, got %v", err) } @@ -146,6 +162,7 @@ func TestFileSafeKeyringReadsAndRemovesLegacyRawKeys(t *testing.T) { func TestFileSafeKeyringTreatsInvalidLegacyFilenameAsNotFound(t *testing.T) { orig := isInvalidFileKeyError isInvalidFileKeyError = func(err error) bool { return errors.Is(err, errInvalidTestFilename) } + t.Cleanup(func() { isInvalidFileKeyError = orig }) ring := newFileSafeKeyring(&invalidFilenameKeyring{}) @@ -165,6 +182,7 @@ func TestOpenKeyringWrapsExplicitFileBackend(t *testing.T) { keyringOpenFunc = func(_ keyring.Config) (keyring.Keyring, error) { return keyring.NewArrayKeyring(nil), nil } + t.Cleanup(func() { keyringOpenFunc = origOpen }) store, err := OpenDefault() @@ -176,6 +194,7 @@ func TestOpenKeyringWrapsExplicitFileBackend(t *testing.T) { if !ok { t.Fatalf("expected *KeyringStore, got %T", store) } + if _, ok := keyringStore.ring.(*fileSafeKeyring); !ok { t.Fatalf("expected file-safe keyring, got %T", keyringStore.ring) } diff --git a/internal/secrets/store.go b/internal/secrets/store.go index 53f8922..cac0653 100644 --- a/internal/secrets/store.go +++ b/internal/secrets/store.go @@ -157,25 +157,29 @@ func keyringServiceName() string { // keyringOpenTimeout is the maximum time to wait for keyring.Open() to complete. // On headless Linux, D-Bus SecretService can hang indefinitely if gnome-keyring // is installed but not running. -const keyringOpenTimeout = 10 * time.Second +const ( + keyringOpenTimeout = 10 * time.Second + goosDarwin = "darwin" + goosLinux = "linux" +) func shouldForceFileBackend(goos string, backendInfo KeyringBackendInfo, dbusAddr string) bool { - return goos == "linux" && backendInfo.Value == keyringBackendAuto && dbusAddr == "" + return goos == goosLinux && backendInfo.Value == keyringBackendAuto && dbusAddr == "" } func shouldUseKeyringTimeout(goos string, backendInfo KeyringBackendInfo, dbusAddr string) bool { - return goos == "linux" && backendInfo.Value == "auto" && dbusAddr != "" + return goos == goosLinux && backendInfo.Value == "auto" && dbusAddr != "" } func shouldUseKeyringOperationTimeout(goos string, backendInfo KeyringBackendInfo) bool { - return goos == "darwin" && (backendInfo.Value == keyringBackendAuto || backendInfo.Value == "keychain") + return goos == goosDarwin && (backendInfo.Value == keyringBackendAuto || backendInfo.Value == "keychain") } func keyringTimeoutHint(goos string) string { switch goos { - case "darwin": + case goosDarwin: return "macOS Keychain may be waiting for a permission prompt; run `gog auth list` from a terminal and click \"Always Allow\" when prompted" - case "linux": + case goosLinux: return "D-Bus SecretService may be unresponsive" default: return "keyring backend may be unresponsive" @@ -237,11 +241,12 @@ func openKeyring() (keyring.Keyring, error) { // is unresponsive (e.g., gnome-keyring installed but not running). // Use a timeout as a safety net. if shouldUseKeyringTimeout(runtime.GOOS, backendInfo, dbusAddr) { - ring, err := openKeyringWithTimeout(cfg, keyringOpenTimeout) - if err != nil { - return nil, err + timeoutRing, timeoutErr := openKeyringWithTimeout(cfg, keyringOpenTimeout) + if timeoutErr != nil { + return nil, timeoutErr } - return prepareKeyring(ring, backendInfo, wrapFileKeys), nil + + return prepareKeyring(timeoutRing, backendInfo, wrapFileKeys), nil } ring, err := keyringOpenFunc(cfg) @@ -256,6 +261,7 @@ func prepareKeyring(ring keyring.Keyring, backendInfo KeyringBackendInfo, wrapFi if wrapFileKeys || isFileKeyring(ring) { ring = newFileSafeKeyring(ring) } + if shouldUseKeyringOperationTimeout(runtime.GOOS, backendInfo) { ring = newTimeoutKeyring(ring, keyringOpenTimeout, keyringTimeoutHint(runtime.GOOS)) } diff --git a/internal/secrets/timeout_keyring.go b/internal/secrets/timeout_keyring.go index 115db9c..59454e0 100644 --- a/internal/secrets/timeout_keyring.go +++ b/internal/secrets/timeout_keyring.go @@ -28,6 +28,7 @@ func withKeyringTimeout[T any](timeout time.Duration, operation string, hint str } ch := make(chan result, 1) + go func() { value, err := fn() ch <- result{value: value, err: err} @@ -64,6 +65,7 @@ func (k *timeoutKeyring) Set(item keyring.Item) error { _, err := withKeyringTimeout(k.timeout, "storing keyring item", k.hint, func() (struct{}, error) { return struct{}{}, k.inner.Set(item) }) + return err } @@ -71,6 +73,7 @@ func (k *timeoutKeyring) Remove(key string) error { _, err := withKeyringTimeout(k.timeout, "removing keyring item", k.hint, func() (struct{}, error) { return struct{}{}, k.inner.Remove(key) }) + return err } diff --git a/internal/secrets/timeout_keyring_test.go b/internal/secrets/timeout_keyring_test.go index 7a9b054..014e946 100644 --- a/internal/secrets/timeout_keyring_test.go +++ b/internal/secrets/timeout_keyring_test.go @@ -55,6 +55,7 @@ func TestKeyringTimeoutHint(t *testing.T) { func TestTimeoutKeyringTimesOutOperations(t *testing.T) { block := make(chan struct{}) + t.Cleanup(func() { close(block) }) ring := newTimeoutKeyring(&blockingKeyring{block: block}, 10*time.Millisecond, keyringTimeoutHint("darwin")) @@ -63,6 +64,7 @@ func TestTimeoutKeyringTimesOutOperations(t *testing.T) { if !errors.Is(err, errKeyringTimeout) { t.Fatalf("expected timeout error, got %v", err) } + if !strings.Contains(err.Error(), "listing keyring items") || !strings.Contains(err.Error(), "Always Allow") { t.Fatalf("expected operation and macOS hint in timeout, got %v", err) }