diff --git a/CHANGELOG.md b/CHANGELOG.md index 74c0467..0a33fa3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,7 @@ - Backup: expand `gog backup push --services all` with Drive content export/download, Gmail settings, native Workspace Docs/Sheets/Slides/Form data, Apps Script projects, Chat, Classroom, best-effort optional service error shards, and plaintext Drive file export. ### Fixed +- Secrets: time out macOS Keychain read/write/list operations with a clear recovery hint instead of hanging indefinitely when a permission prompt cannot surface. (#515, #513) — thanks @sardoru. - Secrets: encode file-backend key names so stored tokens work on Windows, while still reading/removing legacy raw entries. (#527, #502) — thanks @solomonneas. - Drive: include `driveId` in `drive ls`, `drive search`, and `drive get` field masks so Shared Drive files can be identified in JSON output. (#524) — thanks @LeanSheng. - Gmail: expose reply threading headers in default `gmail get --format metadata` output and fail explicit reply targets that cannot provide a `Message-ID`. (#528, #512) — thanks @solomonneas. diff --git a/README.md b/README.md index 0cc5440..b42b0cf 100644 --- a/README.md +++ b/README.md @@ -258,6 +258,8 @@ Backends: - `keychain`: macOS Keychain (recommended on macOS; avoids password management). - `file`: encrypted on-disk keyring (requires a password). +On macOS, Keychain operations time out with a recovery hint if a permission prompt cannot surface; run the command from Terminal and choose "Always Allow", or switch to the file backend for headless runs. + Set backend via command (writes `keyring_backend` into `config.json`): ```bash diff --git a/docs/spec.md b/docs/spec.md index ab96bf4..3d794a7 100644 --- a/docs/spec.md +++ b/docs/spec.md @@ -93,6 +93,7 @@ Implementation: `internal/config/*`. - Key format: `token::` (default client uses `token:default:`) - Legacy key format: `token:` (migrated on first read) - Stored payload is JSON (refresh token + metadata like selected services/scopes). +- macOS Keychain operations are bounded by a timeout so non-surfacing permission prompts return actionable guidance instead of hanging indefinitely. - Fallback: if no OS credential store is available, keyring may use its encrypted "file" backend: - Directory: `$(os.UserConfigDir())/gogcli/keyring/` (one file per key; gog-managed key names are encoded for portable filenames) - Password: prompts on TTY; for non-interactive runs set `GOG_KEYRING_PASSWORD` diff --git a/internal/secrets/store.go b/internal/secrets/store.go index 0775615..53f8922 100644 --- a/internal/secrets/store.go +++ b/internal/secrets/store.go @@ -5,6 +5,7 @@ import ( "errors" "fmt" "os" + "reflect" "runtime" "strings" "time" @@ -156,7 +157,7 @@ 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 = 5 * time.Second +const keyringOpenTimeout = 10 * time.Second func shouldForceFileBackend(goos string, backendInfo KeyringBackendInfo, dbusAddr string) bool { return goos == "linux" && backendInfo.Value == keyringBackendAuto && dbusAddr == "" @@ -166,6 +167,29 @@ func shouldUseKeyringTimeout(goos string, backendInfo KeyringBackendInfo, dbusAd return goos == "linux" && backendInfo.Value == "auto" && dbusAddr != "" } +func shouldUseKeyringOperationTimeout(goos string, backendInfo KeyringBackendInfo) bool { + return goos == "darwin" && (backendInfo.Value == keyringBackendAuto || backendInfo.Value == "keychain") +} + +func keyringTimeoutHint(goos string) string { + switch goos { + case "darwin": + 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": + return "D-Bus SecretService may be unresponsive" + default: + return "keyring backend may be unresponsive" + } +} + +func isFileKeyring(ring keyring.Keyring) bool { + if ring == nil { + return false + } + + return reflect.TypeOf(ring).String() == "*keyring.fileKeyring" +} + func openKeyring() (keyring.Keyring, error) { // On Linux/WSL/containers, OS keychains (secret-service/kwallet) may be unavailable. // In that case github.com/99designs/keyring falls back to the "file" backend, @@ -217,10 +241,7 @@ func openKeyring() (keyring.Keyring, error) { if err != nil { return nil, err } - if wrapFileKeys { - return newFileSafeKeyring(ring), nil - } - return ring, nil + return prepareKeyring(ring, backendInfo, wrapFileKeys), nil } ring, err := keyringOpenFunc(cfg) @@ -228,11 +249,18 @@ func openKeyring() (keyring.Keyring, error) { return nil, fmt.Errorf("open keyring: %w", err) } - if wrapFileKeys { - return newFileSafeKeyring(ring), nil + return prepareKeyring(ring, backendInfo, wrapFileKeys), nil +} + +func prepareKeyring(ring keyring.Keyring, backendInfo KeyringBackendInfo, wrapFileKeys bool) keyring.Keyring { + if wrapFileKeys || isFileKeyring(ring) { + ring = newFileSafeKeyring(ring) + } + if shouldUseKeyringOperationTimeout(runtime.GOOS, backendInfo) { + ring = newTimeoutKeyring(ring, keyringOpenTimeout, keyringTimeoutHint(runtime.GOOS)) } - return ring, nil + return ring } type keyringResult struct { @@ -263,9 +291,7 @@ func openKeyringWithTimeout(cfg keyring.Config, timeout time.Duration) (keyring. return res.ring, nil case <-time.After(timeout): - return nil, fmt.Errorf("%w after %v (D-Bus SecretService may be unresponsive); "+ - "set GOG_KEYRING_BACKEND=file and GOG_KEYRING_PASSWORD= to use encrypted file storage instead", - errKeyringTimeout, timeout) + return nil, keyringTimeoutError("opening keyring", timeout, keyringTimeoutHint(runtime.GOOS)) } } diff --git a/internal/secrets/store_test.go b/internal/secrets/store_test.go index 2e6cea1..dcfedbd 100644 --- a/internal/secrets/store_test.go +++ b/internal/secrets/store_test.go @@ -172,6 +172,14 @@ func TestKeyringDbusGuards(t *testing.T) { wantForce: false, wantTimeout: false, }, + { + name: "darwin auto no open timeout", + goos: "darwin", + backend: "auto", + dbusAddr: "", + wantForce: false, + wantTimeout: false, + }, { name: "linux explicit file no dbus", goos: "linux", diff --git a/internal/secrets/timeout_keyring.go b/internal/secrets/timeout_keyring.go new file mode 100644 index 0000000..115db9c --- /dev/null +++ b/internal/secrets/timeout_keyring.go @@ -0,0 +1,79 @@ +package secrets + +import ( + "fmt" + "time" + + "github.com/99designs/keyring" +) + +type timeoutKeyring struct { + inner keyring.Keyring + timeout time.Duration + hint string +} + +func newTimeoutKeyring(inner keyring.Keyring, timeout time.Duration, hint string) keyring.Keyring { + return &timeoutKeyring{ + inner: inner, + timeout: timeout, + hint: hint, + } +} + +func withKeyringTimeout[T any](timeout time.Duration, operation string, hint string, fn func() (T, error)) (T, error) { + type result struct { + value T + err error + } + + ch := make(chan result, 1) + go func() { + value, err := fn() + ch <- result{value: value, err: err} + }() + + select { + case res := <-ch: + return res.value, res.err + case <-time.After(timeout): + var zero T + return zero, keyringTimeoutError(operation, timeout, hint) + } +} + +func keyringTimeoutError(operation string, timeout time.Duration, hint string) error { + return fmt.Errorf("%w after %v while %s (%s); "+ + "set GOG_KEYRING_BACKEND=file and GOG_KEYRING_PASSWORD= to use encrypted file storage instead", + errKeyringTimeout, timeout, operation, hint) +} + +func (k *timeoutKeyring) Get(key string) (keyring.Item, error) { + return withKeyringTimeout(k.timeout, "reading keyring item", k.hint, func() (keyring.Item, error) { + return k.inner.Get(key) + }) +} + +func (k *timeoutKeyring) GetMetadata(key string) (keyring.Metadata, error) { + return withKeyringTimeout(k.timeout, "reading keyring metadata", k.hint, func() (keyring.Metadata, error) { + return k.inner.GetMetadata(key) + }) +} + +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 +} + +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 +} + +func (k *timeoutKeyring) Keys() ([]string, error) { + return withKeyringTimeout(k.timeout, "listing keyring items", k.hint, k.inner.Keys) +} diff --git a/internal/secrets/timeout_keyring_test.go b/internal/secrets/timeout_keyring_test.go new file mode 100644 index 0000000..7a9b054 --- /dev/null +++ b/internal/secrets/timeout_keyring_test.go @@ -0,0 +1,102 @@ +package secrets + +import ( + "errors" + "strings" + "testing" + "time" + + "github.com/99designs/keyring" +) + +func TestKeyringOperationTimeoutGuards(t *testing.T) { + tests := []struct { + name string + goos string + backend string + wantWrap bool + }{ + {name: "darwin auto", goos: "darwin", backend: "auto", wantWrap: true}, + {name: "darwin keychain", goos: "darwin", backend: "keychain", wantWrap: true}, + {name: "darwin file", goos: "darwin", backend: "file", wantWrap: false}, + {name: "linux auto", goos: "linux", backend: "auto", wantWrap: false}, + {name: "windows auto", goos: "windows", backend: "auto", wantWrap: false}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := shouldUseKeyringOperationTimeout(tt.goos, KeyringBackendInfo{Value: tt.backend}) + if got != tt.wantWrap { + t.Fatalf("shouldUseKeyringOperationTimeout=%v, want %v", got, tt.wantWrap) + } + }) + } +} + +func TestKeyringTimeoutHint(t *testing.T) { + tests := []struct { + goos string + wantSubstr string + }{ + {"darwin", "Always Allow"}, + {"linux", "D-Bus SecretService"}, + {"windows", "keyring backend"}, + } + + for _, tt := range tests { + t.Run(tt.goos, func(t *testing.T) { + hint := keyringTimeoutHint(tt.goos) + if !strings.Contains(hint, tt.wantSubstr) { + t.Fatalf("keyringTimeoutHint(%q)=%q, want substring %q", tt.goos, hint, tt.wantSubstr) + } + }) + } +} + +func TestTimeoutKeyringTimesOutOperations(t *testing.T) { + block := make(chan struct{}) + t.Cleanup(func() { close(block) }) + + ring := newTimeoutKeyring(&blockingKeyring{block: block}, 10*time.Millisecond, keyringTimeoutHint("darwin")) + + _, err := ring.Keys() + 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) + } +} + +type blockingKeyring struct { + block <-chan struct{} +} + +func (k *blockingKeyring) wait() { + <-k.block +} + +func (k *blockingKeyring) Get(string) (keyring.Item, error) { + k.wait() + return keyring.Item{}, keyring.ErrKeyNotFound +} + +func (k *blockingKeyring) GetMetadata(string) (keyring.Metadata, error) { + k.wait() + return keyring.Metadata{}, keyring.ErrKeyNotFound +} + +func (k *blockingKeyring) Set(keyring.Item) error { + k.wait() + return nil +} + +func (k *blockingKeyring) Remove(string) error { + k.wait() + return nil +} + +func (k *blockingKeyring) Keys() ([]string, error) { + k.wait() + return nil, nil +}