fix(secrets): timeout keyring.Open() on macOS to prevent indefinite hang (#513)#515
Closed
sardoru wants to merge 1 commit intosteipete:mainfrom
Closed
fix(secrets): timeout keyring.Open() on macOS to prevent indefinite hang (#513)#515sardoru wants to merge 1 commit intosteipete:mainfrom
sardoru wants to merge 1 commit intosteipete:mainfrom
Conversation
Fixes steipete#513. Previously the keyring.Open() timeout safety net only covered Linux with a D-Bus session. On macOS, if the Security framework waits for a GUI permission prompt that cannot surface (e.g., post-Homebrew-upgrade re-authorization, non-interactive runs like crons/agents), every auth- requiring command hangs indefinitely until the process is killed — with no output and no actionable error. This extends shouldUseKeyringTimeout to include macOS when the backend is 'auto' or 'keychain'. The timeout is also bumped from 5s to 10s to tolerate slower macOS Keychain responses on first post-upgrade access. When the timeout fires, the error now includes platform-specific guidance: - darwin: run from a terminal and click 'Always Allow' when prompted (common after Homebrew upgrades; links to steipete#86) - linux: existing 'D-Bus SecretService may be unresponsive' hint - other: generic 'keyring backend may be unresponsive' hint All cases continue to suggest GOG_KEYRING_BACKEND=file as a fallback. Adds test cases covering darwin auto/keychain/file in TestKeyringDbusGuards and a new TestKeyringTimeoutHint covering the platform-specific hints.
Owner
|
Thanks @sardoru. I landed this as a maintainer rewrite on main in 6430dd1, with lint/test cleanup in c2ea4f5. I changed the implementation from an Closing this as landed. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #513.
The bug
After upgrading gogcli via Homebrew, every command that reads stored tokens (
gog auth list,gog gmail search, etc.) hangs indefinitely on macOS and is eventually SIGKILL'd — no output, no prompt, no actionable error.The root cause is that the
keyring.Open()timeout safety net introduced for headless Linux (#XXX) only fires whengoos == "linux". On macOS, the Security framework waits for a Keychain GUI permission prompt that can't surface in non-interactive contexts (cron jobs, agents, subprocesses) — or, per the existing comment referencing #86, after a Homebrew upgrade installs a new binary hash and the previous "Always Allow" grant no longer applies. The process just hangs.The fix
Extend
shouldUseKeyringTimeoutto includedarwinwhen the backend isautoorkeychain. If the Keychain doesn't respond within the timeout, return a clear error with platform-specific guidance instead of hanging forever.Also bump the timeout from 5s → 10s to tolerate slower macOS Keychain responses on first post-upgrade access (still short enough to feel like a real error, long enough to not false-positive on a cold keychain).
Error message before
Error message after
What's covered
Tests
TestKeyringDbusGuardswith three darwin cases:auto→ timeout,keychain→ timeout,file→ no timeoutTestKeyringTimeoutHintverifies platform-specific hint text for darwin/linux/windowsTestOpenKeyringWithTimeout_Timeoutstill passes (my change kept theGOG_KEYRING_BACKEND=filefallback text the test asserts on)Why 10s instead of 5s
On macOS, Keychain can briefly block when it's cold (first access after login, or first access after a binary-hash change). 5s was occasionally false-positive even when the prompt would eventually surface. 10s is comfortably long enough for a real prompt to complete while still failing fast on a deadlocked Security framework.
Caveat / follow-up
The hang is really in
keyring.Keys()on subsequent calls if the permission grant isn't cached — but in practicekeyring.Open()probes the backend during construction, which is where the GUI prompt is triggered, so timing outOpen()catches almost all real-world cases. A full-coverage fix would also wrapKeys()/Get()with a timeout, but that's more invasive and deserves a separate PR.Discovery
I hit this the night after #513 — upgraded from v0.12.0 to v0.13.0 via
brew upgrade, and everygogcommand that touched auth hung. Rolled back to v0.12.0 (same OAuth tokens in Keychain), everything worked instantly again. Didn't re-upgrade until I could trace it.Tested manually on: