fix(secrets): scope longer keyring timeout to macOS

steipete · steipete · commit e380be2ec407 · 2026-06-19T08:28:47.000+01:00
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -4,6 +4,7 @@
 
 ### Added
 
+- Auth: make keyring open/operation timeouts configurable with `GOG_KEYRING_OPEN_TIMEOUT`, using a `30s` macOS default for permission prompts while retaining `10s` elsewhere. (#845) — thanks @malob.
 - Calendar: add `create --timezone`/`--tz` to apply one IANA timezone to both event endpoints while retaining granular start/end timezone flags. (#844) — thanks @malob.
 - Docs: add non-destructive `insert-image --before` and `--after` anchors while clarifying that `--at` replaces its placeholder. (#839) — thanks @sebsnyk.
 - Slides: allow `insert-image` and `replace-slide` to use public HTTPS image URLs without temporary Drive sharing. (#825) — thanks @sebsnyk.
diff --git a/docs/spec.md b/docs/spec.md
@@ -112,7 +112,7 @@ Implementation: `internal/config/*`.
 - Legacy key format: `token:<email>` (migrated on first read)
 - Stored payload is JSON (refresh token + metadata like OIDC subject, current email, selected services/scopes).
 - Email is treated as display/contact state; Google's OIDC `sub` is used to detect the same account after an email rename and migrate aliases/defaults/client mappings on reauthorization.
-- macOS Keychain operations are bounded by a timeout (default `30s`, configurable via `GOG_KEYRING_OPEN_TIMEOUT`) so non-surfacing permission prompts return actionable guidance instead of hanging indefinitely.
+- Keyring operations are bounded by a timeout (default `30s` on macOS and `10s` elsewhere, configurable via `GOG_KEYRING_OPEN_TIMEOUT`) so non-surfacing permission prompts and unresponsive backends 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`
@@ -169,7 +169,7 @@ Environment:
 - `GOG_KEYRING_PASSWORD=...` (used when keyring falls back to encrypted file backend in non-interactive environments)
 - `GOG_KEYRING_BACKEND={auto|keychain|file}` (force backend; use `file` to avoid Keychain prompts and pair with `GOG_KEYRING_PASSWORD` for non-interactive)
 - `GOG_KEYRING_SERVICE_NAME=...` (override keyring namespace/service name; default `gogcli`)
-- `GOG_KEYRING_OPEN_TIMEOUT=30s` (max time to wait for a keyring open/operation — e.g. a macOS Keychain permission prompt — before failing; Go duration, default `30s`)
+- `GOG_KEYRING_OPEN_TIMEOUT=30s` (max time to wait for a keyring open/operation — e.g. a macOS Keychain permission prompt — before failing; Go duration, default `30s` on macOS and `10s` elsewhere)
 - `GOG_TIMEZONE=America/New_York` (default output timezone; IANA name or `UTC`; `local` forces local timezone)
 - `GOG_ENABLE_COMMANDS=calendar,tasks,gmail.search` (optional prefix allowlist; dot paths allowed; parent paths allow children)
 - `GOG_ENABLE_COMMANDS_EXACT=calendar.events,gmail.search` (optional exact allowlist; dot paths allowed; parent paths do not allow children)
diff --git a/internal/secrets/backend.go b/internal/secrets/backend.go
@@ -82,7 +82,7 @@ func OpenOptionsFromLookup(
 		GOOS:        goos,
 		DBusAddress: dbusAddress,
 		IsTTY:       isTTY,
-		OpenTimeout: parseKeyringOpenTimeout(openTimeoutRaw),
+		OpenTimeout: parseKeyringOpenTimeout(openTimeoutRaw, goos),
 		LockTimeout: parseKeyringLockTimeout(lockTimeoutRaw),
 	}
 }
@@ -163,28 +163,34 @@ func serviceNameFor(options OpenOptions) string {
 	return config.AppName
 }
 
-// keyringOpenTimeout is the default maximum time to wait for a keyring open or
-// operation to complete; override with GOG_KEYRING_OPEN_TIMEOUT. It guards
-// against backends that can hang indefinitely (headless-Linux D-Bus
-// SecretService when gnome-keyring is installed but not running; an unresponsive
-// macOS Keychain) while still leaving room for an interactive macOS Keychain
-// permission prompt (password entry plus "Always Allow").
+// Keyring timeouts guard against unresponsive backends. macOS gets longer for
+// interactive permission prompts; other platforms retain the existing limit.
 const (
-	keyringOpenTimeout = 30 * time.Second
-	goosDarwin         = "darwin"
-	goosLinux          = "linux"
+	keyringOpenTimeout       = 10 * time.Second
+	darwinKeyringOpenTimeout = 30 * time.Second
+	goosDarwin               = "darwin"
+	goosLinux                = "linux"
 )
 
-// parseKeyringOpenTimeout resolves GOG_KEYRING_OPEN_TIMEOUT, falling back to the
-// default when unset, unparseable, or non-positive.
-func parseKeyringOpenTimeout(raw string) time.Duration {
+func defaultKeyringOpenTimeout(goos string) time.Duration {
+	if goos == goosDarwin {
+		return darwinKeyringOpenTimeout
+	}
+
+	return keyringOpenTimeout
+}
+
+// parseKeyringOpenTimeout resolves GOG_KEYRING_OPEN_TIMEOUT, falling back to
+// the platform default when unset, unparseable, or non-positive.
+func parseKeyringOpenTimeout(raw, goos string) time.Duration {
+	fallback := defaultKeyringOpenTimeout(goos)
 	if raw == "" {
-		return keyringOpenTimeout
+		return fallback
 	}
 
 	timeout, err := time.ParseDuration(raw)
 	if err != nil || timeout <= 0 {
-		return keyringOpenTimeout
+		return fallback
 	}
 
 	return timeout
@@ -269,7 +275,7 @@ func openKeyringWithOptions(options OpenOptions) (keyring.Keyring, error) {
 
 	openTimeout := options.OpenTimeout
 	if openTimeout <= 0 {
-		openTimeout = keyringOpenTimeout
+		openTimeout = defaultKeyringOpenTimeout(options.GOOS)
 	}
 
 	open := options.openKeyringFn
@@ -315,7 +321,7 @@ func prepareKeyring(
 	if shouldUseKeyringOperationTimeout(options.GOOS, backendInfo, options.DBusAddress) {
 		timeout := options.OpenTimeout
 		if timeout <= 0 {
-			timeout = keyringOpenTimeout
+			timeout = defaultKeyringOpenTimeout(options.GOOS)
 		}
 		ring = newTimeoutKeyring(ring, timeout, keyringTimeoutHint(options.GOOS))
 	}
diff --git a/internal/secrets/store_test.go b/internal/secrets/store_test.go
@@ -102,20 +102,24 @@ func TestParseKeyringOpenTimeout(t *testing.T) {
 	tests := []struct {
 		name string
 		raw  string
+		goos string
 		want time.Duration
 	}{
-		{name: "empty falls back to default", raw: "", want: keyringOpenTimeout},
-		{name: "valid duration", raw: "1m", want: time.Minute},
-		{name: "unparseable falls back to default", raw: "nonsense", want: keyringOpenTimeout},
-		{name: "non-positive falls back to default", raw: "-5s", want: keyringOpenTimeout},
+		{name: "darwin default", goos: "darwin", want: darwinKeyringOpenTimeout},
+		{name: "linux default", goos: "linux", want: keyringOpenTimeout},
+		{name: "other default", goos: "windows", want: keyringOpenTimeout},
+		{name: "valid duration overrides darwin", raw: "1m", goos: "darwin", want: time.Minute},
+		{name: "valid duration overrides linux", raw: "45s", goos: "linux", want: 45 * time.Second},
+		{name: "invalid uses darwin default", raw: "nonsense", goos: "darwin", want: darwinKeyringOpenTimeout},
+		{name: "non-positive uses linux default", raw: "-5s", goos: "linux", want: keyringOpenTimeout},
 	}
 
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
 			t.Parallel()
 
-			if got := parseKeyringOpenTimeout(tt.raw); got != tt.want {
-				t.Fatalf("parseKeyringOpenTimeout(%q) = %v, want %v", tt.raw, got, tt.want)
+			if got := parseKeyringOpenTimeout(tt.raw, tt.goos); got != tt.want {
+				t.Fatalf("parseKeyringOpenTimeout(%q, %q) = %v, want %v", tt.raw, tt.goos, got, tt.want)
 			}
 		})
 	}
