feat(secrets): make keyring open timeout configurable, default 30s (#845)

malob · claude · steipete · web-flow · commit b3c285613b74 · 2026-06-19T09:42:53.000+02:00
* feat(secrets): make keyring open timeout configurable, default 30s

Raise the keyring open/operation timeout from 10s to 30s and make it
configurable via GOG_KEYRING_OPEN_TIMEOUT (Go duration), mirroring the
existing GOG_KEYRING_LOCK_TIMEOUT.

10s is too short to satisfy an interactive macOS Keychain permission prompt
(password entry plus "Always Allow"), so a legitimate prompt times out
before it can be approved. 30s still bounds the indefinite hang the timeout
was originally added to prevent. The timeout error now points at
GOG_KEYRING_OPEN_TIMEOUT.

Co-Authored-By: Claude Opus 4.8 &lt;noreply@anthropic.com&gt;

* fix(secrets): scope longer keyring timeout to macOS

---------

Co-authored-by: Claude Opus 4.8 &lt;noreply@anthropic.com&gt;
Co-authored-by: Peter Steinberger &lt;steipete@gmail.com&gt;
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 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,6 +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` 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
@@ -16,6 +16,7 @@ const (
 	keyringPasswordEnv    = "GOG_KEYRING_PASSWORD" //nolint:gosec // env var name, not a credential
 	keyringBackendEnv     = "GOG_KEYRING_BACKEND"  //nolint:gosec // env var name, not a credential
 	keyringServiceNameEnv = "GOG_KEYRING_SERVICE_NAME"
+	keyringOpenTimeoutEnv = "GOG_KEYRING_OPEN_TIMEOUT"
 )
 
 var (
@@ -68,6 +69,7 @@ func OpenOptionsFromLookup(
 	password, passwordSet := lookup(keyringPasswordEnv)
 	serviceName, _ := lookup(keyringServiceNameEnv)
 	dbusAddress, _ := lookup("DBUS_SESSION_BUS_ADDRESS")
+	openTimeoutRaw, _ := lookup(keyringOpenTimeoutEnv)
 	lockTimeoutRaw, _ := lookup(keyringLockTimeoutEnv)
 
 	return OpenOptions{
@@ -80,7 +82,7 @@ func OpenOptionsFromLookup(
 		GOOS:        goos,
 		DBusAddress: dbusAddress,
 		IsTTY:       isTTY,
-		OpenTimeout: keyringOpenTimeout,
+		OpenTimeout: parseKeyringOpenTimeout(openTimeoutRaw, goos),
 		LockTimeout: parseKeyringLockTimeout(lockTimeoutRaw),
 	}
 }
@@ -161,15 +163,39 @@ func serviceNameFor(options OpenOptions) string {
 	return config.AppName
 }
 
-// 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.
+// Keyring timeouts guard against unresponsive backends. macOS gets longer for
+// interactive permission prompts; other platforms retain the existing limit.
 const (
-	keyringOpenTimeout = 10 * time.Second
-	goosDarwin         = "darwin"
-	goosLinux          = "linux"
+	keyringOpenTimeout       = 10 * time.Second
+	darwinKeyringOpenTimeout = 30 * time.Second
+	goosDarwin               = "darwin"
+	goosLinux                = "linux"
 )
 
+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 fallback
+	}
+
+	timeout, err := time.ParseDuration(raw)
+	if err != nil || timeout <= 0 {
+		return fallback
+	}
+
+	return timeout
+}
+
 func shouldForceFileBackend(goos string, backendInfo KeyringBackendInfo, dbusAddr string) bool {
 	return goos == goosLinux && backendInfo.Value == keyringBackendAuto && dbusAddr == ""
 }
@@ -249,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
@@ -295,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
@@ -76,6 +76,55 @@ func TestOpenOptionsFromLookupCapturesEnvironment(t *testing.T) {
 	}
 }
 
+func TestOpenOptionsFromLookupOpenTimeout(t *testing.T) {
+	t.Parallel()
+
+	values := map[string]string{keyringOpenTimeoutEnv: "45s"}
+	options := OpenOptionsFromLookup(
+		config.Layout{ConfigDir: "/config", DataDir: "/data"},
+		config.NewConfigStore(config.Layout{ConfigDir: "/config"}),
+		func(key string) (string, bool) {
+			value, ok := values[key]
+			return value, ok
+		},
+		"darwin",
+		true,
+	)
+
+	if options.OpenTimeout != 45*time.Second {
+		t.Fatalf("OpenTimeout = %v, want 45s", options.OpenTimeout)
+	}
+}
+
+func TestParseKeyringOpenTimeout(t *testing.T) {
+	t.Parallel()
+
+	tests := []struct {
+		name string
+		raw  string
+		goos string
+		want time.Duration
+	}{
+		{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, tt.goos); got != tt.want {
+				t.Fatalf("parseKeyringOpenTimeout(%q, %q) = %v, want %v", tt.raw, tt.goos, got, tt.want)
+			}
+		})
+	}
+}
+
 func TestOpenUsesInjectedOptions(t *testing.T) {
 	t.Parallel()
 
diff --git a/internal/secrets/timeout_keyring.go b/internal/secrets/timeout_keyring.go
@@ -47,8 +47,8 @@ func withKeyringTimeout[T any](timeout time.Duration, operation string, hint str
 
 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=<password> to use encrypted file storage instead",
-		errKeyringTimeout, timeout, operation, hint)
+		"set %s to allow more time, or set GOG_KEYRING_BACKEND=file and GOG_KEYRING_PASSWORD=<password> to use encrypted file storage instead",
+		errKeyringTimeout, timeout, operation, hint, keyringOpenTimeoutEnv)
 }
 
 func IsKeyringTimeout(err error) bool {
diff --git a/internal/secrets/timeout_keyring_test.go b/internal/secrets/timeout_keyring_test.go
@@ -68,8 +68,9 @@ func TestTimeoutKeyringTimesOutOperations(t *testing.T) {
 		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)
+	if !strings.Contains(err.Error(), "listing keyring items") || !strings.Contains(err.Error(), "Always Allow") ||
+		!strings.Contains(err.Error(), "GOG_KEYRING_OPEN_TIMEOUT") {
+		t.Fatalf("expected operation, macOS hint, and timeout env in error, got %v", err)
 	}
 }
 

Original file line number	Diff line number	Diff line change
`@@ -47,8 +47,8 @@ func withKeyringTimeout[T any](timeout time.Duration, operation string, hint str`
`47`	`47`
`48`	`48`	`func keyringTimeoutError(operation string, timeout time.Duration, hint string) error {`
`49`	`49`	`return fmt.Errorf("%w after %v while %s (%s); "+`
`50`		`- "set GOG_KEYRING_BACKEND=file and GOG_KEYRING_PASSWORD=<password> to use encrypted file storage instead",`
`51`		`- errKeyringTimeout, timeout, operation, hint)`
	`50`	`+ "set %s to allow more time, or set GOG_KEYRING_BACKEND=file and GOG_KEYRING_PASSWORD=<password> to use encrypted file storage instead",`
	`51`	`+ errKeyringTimeout, timeout, operation, hint, keyringOpenTimeoutEnv)`
`52`	`52`	`}`
`53`	`53`
`54`	`54`	`func IsKeyringTimeout(err error) bool {`
Original file line number	Diff line number	Diff line change
`@@ -68,8 +68,9 @@ func TestTimeoutKeyringTimesOutOperations(t *testing.T) {`
`68`	`68`	`t.Fatalf("expected timeout error, got %v", err)`
`69`	`69`	`}`
`70`	`70`
`71`		`- if !strings.Contains(err.Error(), "listing keyring items") \|\| !strings.Contains(err.Error(), "Always Allow") {`
`72`		`- t.Fatalf("expected operation and macOS hint in timeout, got %v", err)`
	`71`	`+ if !strings.Contains(err.Error(), "listing keyring items") \|\| !strings.Contains(err.Error(), "Always Allow") \|\|`
	`72`	`+ !strings.Contains(err.Error(), "GOG_KEYRING_OPEN_TIMEOUT") {`
	`73`	`+ t.Fatalf("expected operation, macOS hint, and timeout env in error, got %v", err)`
`73`	`74`	`}`
`74`	`75`	`}`
`75`	`76`