From edb710faf55e42cdc54adaab7fdc7c0233ea6c7f Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Mon, 27 Jul 2026 19:41:19 -0700 Subject: [PATCH 1/4] Bound the keyring probe on headless sessions (Refs #568) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Bumps github.com/basecamp/cli to the post-#56 credstore (v0.2.2-0.20260728023309-04e401b12c6c), which adds StoreOptions.ProbeTimeout: a bounded availability probe that kills the hung `security` child on darwin and falls back to file storage as if the probe had failed. The CLI wires it interactivity-aware: when stderr is not a terminal (CI, piped installers, ssh without a TTY) the probe is bounded at ten seconds — a headless session can never answer a keychain unlock prompt, so hanging forever in an uncancellable child was the only alternative (the #568 incident class, previously mitigated only by lazy construction and the BASECAMP_NO_KEYRING escape hatch). Interactive sessions keep the unbounded probe: a locked keychain there raises an unlock prompt, and cutting it off mid-answer would silently degrade the user to plaintext file storage. ForceFile is deliberately not wired: credstore's internal timeout-to-file fallback covers the CLI's need, and the env-var path already provides forced file mode. --- go.mod | 2 +- go.sum | 4 ++-- internal/auth/keyring.go | 27 +++++++++++++++++++++++++-- internal/auth/keyring_test.go | 34 ++++++++++++++++++++++++++++++++++ 4 files changed, 62 insertions(+), 5 deletions(-) diff --git a/go.mod b/go.mod index 2485c3b5..bf3e2bda 100644 --- a/go.mod +++ b/go.mod @@ -7,7 +7,7 @@ require ( charm.land/bubbletea/v2 v2.0.8 charm.land/lipgloss/v2 v2.0.5 github.com/basecamp/basecamp-sdk/go v0.9.1-0.20260727173625-bb363c847b92 - github.com/basecamp/cli v0.2.1 + github.com/basecamp/cli v0.2.2-0.20260728023309-04e401b12c6c github.com/charmbracelet/bubbles v1.0.0 github.com/charmbracelet/glamour v1.0.0 github.com/charmbracelet/huh v1.0.0 diff --git a/go.sum b/go.sum index 68a29485..11a120dd 100644 --- a/go.sum +++ b/go.sum @@ -25,8 +25,8 @@ github.com/aymerick/douceur v0.2.0 h1:Mv+mAeH1Q+n9Fr+oyamOlAkUNPWPlA8PPGR0QAaYuP github.com/aymerick/douceur v0.2.0/go.mod h1:wlT5vV2O3h55X9m7iVYN0TBM0NH/MmbLnd30/FjWUq4= github.com/basecamp/basecamp-sdk/go v0.9.1-0.20260727173625-bb363c847b92 h1:4gQJTR8e5kBvXHDHLvef+Q00XY7KzWTxnQSLWMSPcyA= github.com/basecamp/basecamp-sdk/go v0.9.1-0.20260727173625-bb363c847b92/go.mod h1:r83ralDQ0q9vbAby5qQ5x9hgCgUdJLDLHYpiU6jaFjE= -github.com/basecamp/cli v0.2.1 h1:8GyehPVtsTXla0oOPu4QgXRjwwzJ99prlByvyi+0HRQ= -github.com/basecamp/cli v0.2.1/go.mod h1:p8tt/DatJ2LAzWO6N6tNfV8x3gF5T3IxDTo+U8FfWPo= +github.com/basecamp/cli v0.2.2-0.20260728023309-04e401b12c6c h1:+5sQBl8sqYoD1Qhwsibn8sBCKWPyZ9NDez6mnuo9Afo= +github.com/basecamp/cli v0.2.2-0.20260728023309-04e401b12c6c/go.mod h1:EK1Dba6DEw8ZAilVBpf/jri3ONDV7LQkLACSDe73f/c= github.com/bmatcuk/doublestar v1.1.1/go.mod h1:UD6OnuiIn0yFxxA2le/rnRU1G4RaI4UvFv1sNto9p6w= github.com/catppuccin/go v0.3.0 h1:d+0/YicIq+hSTo5oPuRi5kOpqkVA5tAsU6dNhvRu+aY= github.com/catppuccin/go v0.3.0/go.mod h1:8IHJuMGaUUjQM82qBrGNBv7LFq6JI3NnQCF6MOlZjpc= diff --git a/internal/auth/keyring.go b/internal/auth/keyring.go index 6fcc52a4..8c6d1c52 100644 --- a/internal/auth/keyring.go +++ b/internal/auth/keyring.go @@ -5,8 +5,10 @@ import ( "fmt" "os" "sync" + "time" "github.com/basecamp/cli/credstore" + "github.com/charmbracelet/x/term" ) // Credentials holds OAuth tokens and metadata. @@ -38,6 +40,23 @@ type Store struct { // newCredStore is replaceable in tests to avoid real keyring access. var newCredStore = credstore.NewStore +// stderrIsTerminal reports whether a human is attached. Stderr, not stdout: +// interactive use routinely pipes stdout (JSON to jq) while prompts and +// warnings still reach the terminal. Replaceable in tests. +var stderrIsTerminal = func() bool { + return term.IsTerminal(os.Stderr.Fd()) +} + +// headlessProbeTimeout bounds the keyring availability probe when no +// terminal is attached. A headless session (CI, piped installers, ssh +// without a TTY) can never answer a keychain unlock prompt, so an +// unavailable keyring must fall back to file storage instead of hanging +// forever in an uncancellable `security` child — the #568 incident class. +// Interactive sessions keep the unbounded probe: a locked keychain there +// raises an unlock prompt, and cutting it off mid-answer would silently +// degrade the user to plaintext file storage. +const headlessProbeTimeout = 10 * time.Second + // NewStore creates a credential store. The OS keyring is not touched until // the first credential operation. func NewStore(fallbackDir string) *Store { @@ -49,11 +68,15 @@ func NewStore(fallbackDir string) *Store { // sync.Once provides the synchronization. func (s *Store) ensure() *credstore.Store { s.initOnce.Do(func() { - s.inner = newCredStore(credstore.StoreOptions{ + opts := credstore.StoreOptions{ ServiceName: "basecamp", DisableEnvVar: "BASECAMP_NO_KEYRING", FallbackDir: s.fallbackDir, - }) + } + if !stderrIsTerminal() { + opts.ProbeTimeout = headlessProbeTimeout + } + s.inner = newCredStore(opts) }) return s.inner } diff --git a/internal/auth/keyring_test.go b/internal/auth/keyring_test.go index 08a08f26..60b34abc 100644 --- a/internal/auth/keyring_test.go +++ b/internal/auth/keyring_test.go @@ -44,3 +44,37 @@ func TestNewStoreIsLazy(t *testing.T) { store.UsingKeyring() assert.Equal(t, 1, calls, "construction happens once") } + +// swapStderrIsTerminal replaces the interactivity seam for the test. +func swapStderrIsTerminal(t *testing.T, interactive bool) { + t.Helper() + orig := stderrIsTerminal + stderrIsTerminal = func() bool { return interactive } + t.Cleanup(func() { stderrIsTerminal = orig }) +} + +// ensureOptions constructs a store's credstore through the seams and returns +// the options it was built with. +func ensureOptions(t *testing.T, interactive bool) credstore.StoreOptions { + t.Helper() + t.Setenv("BASECAMP_NO_KEYRING", "1") // keep the delegated construction off the real keyring + + var got credstore.StoreOptions + swapNewCredStore(t, func(opts credstore.StoreOptions) *credstore.Store { + got = opts + return credstore.NewStore(opts) + }) + swapStderrIsTerminal(t, interactive) + + NewStore(t.TempDir()).UsingKeyring() + return got +} + +// Headless sessions can never answer a keychain unlock prompt, so the probe +// must be bounded there — the #568 incident class. Interactive sessions keep +// the unbounded probe so a legitimate unlock prompt is never cut off +// mid-answer (which would silently degrade to plaintext file storage). +func TestEnsureBoundsProbeOnlyWhenHeadless(t *testing.T) { + assert.Equal(t, headlessProbeTimeout, ensureOptions(t, false).ProbeTimeout) + assert.Zero(t, ensureOptions(t, true).ProbeTimeout) +} From b2b72bfc9e22f108ad4216ed91ff352657114f91 Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Mon, 27 Jul 2026 19:54:58 -0700 Subject: [PATCH 2/4] Treat only fully detached sessions as headless (Refs #568) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Stderr alone misclassified routine interactive use: `2>auth.log` from a terminal would have bounded the probe and, on a >10s unlock answer, silently degraded to plaintext file storage with the warning hidden in the redirected log. Headless now requires stdin, stdout, and stderr to all be non-terminals — any attached stream means a human can answer the unlock prompt on the controlling terminal or the GUI. Review feedback on #581. --- internal/auth/keyring.go | 30 +++++++++++++++++------------- internal/auth/keyring_test.go | 18 +++++++++--------- 2 files changed, 26 insertions(+), 22 deletions(-) diff --git a/internal/auth/keyring.go b/internal/auth/keyring.go index 8c6d1c52..701f0984 100644 --- a/internal/auth/keyring.go +++ b/internal/auth/keyring.go @@ -40,21 +40,25 @@ type Store struct { // newCredStore is replaceable in tests to avoid real keyring access. var newCredStore = credstore.NewStore -// stderrIsTerminal reports whether a human is attached. Stderr, not stdout: -// interactive use routinely pipes stdout (JSON to jq) while prompts and -// warnings still reach the terminal. Replaceable in tests. -var stderrIsTerminal = func() bool { - return term.IsTerminal(os.Stderr.Fd()) +// sessionIsHeadless reports that no human is attached: stdin, stdout, and +// stderr are all non-terminals. Any one being a terminal counts as +// interactive — redirecting a stream or two (`2>auth.log`, `| jq`) is +// routine interactive use where a keychain unlock prompt can still be +// answered, on the controlling terminal or the GUI. Replaceable in tests. +var sessionIsHeadless = func() bool { + return !term.IsTerminal(os.Stdin.Fd()) && + !term.IsTerminal(os.Stdout.Fd()) && + !term.IsTerminal(os.Stderr.Fd()) } // headlessProbeTimeout bounds the keyring availability probe when no -// terminal is attached. A headless session (CI, piped installers, ssh -// without a TTY) can never answer a keychain unlock prompt, so an -// unavailable keyring must fall back to file storage instead of hanging -// forever in an uncancellable `security` child — the #568 incident class. -// Interactive sessions keep the unbounded probe: a locked keychain there -// raises an unlock prompt, and cutting it off mid-answer would silently -// degrade the user to plaintext file storage. +// terminal is attached to any standard stream. A headless session (CI, +// piped installers, ssh without a TTY) can never answer a keychain unlock +// prompt, so an unavailable keyring must fall back to file storage instead +// of hanging forever in an uncancellable `security` child — the #568 +// incident class. Interactive sessions keep the unbounded probe: a locked +// keychain there raises an unlock prompt, and cutting it off mid-answer +// would silently degrade the user to plaintext file storage. const headlessProbeTimeout = 10 * time.Second // NewStore creates a credential store. The OS keyring is not touched until @@ -73,7 +77,7 @@ func (s *Store) ensure() *credstore.Store { DisableEnvVar: "BASECAMP_NO_KEYRING", FallbackDir: s.fallbackDir, } - if !stderrIsTerminal() { + if sessionIsHeadless() { opts.ProbeTimeout = headlessProbeTimeout } s.inner = newCredStore(opts) diff --git a/internal/auth/keyring_test.go b/internal/auth/keyring_test.go index 60b34abc..22b6701c 100644 --- a/internal/auth/keyring_test.go +++ b/internal/auth/keyring_test.go @@ -45,17 +45,17 @@ func TestNewStoreIsLazy(t *testing.T) { assert.Equal(t, 1, calls, "construction happens once") } -// swapStderrIsTerminal replaces the interactivity seam for the test. -func swapStderrIsTerminal(t *testing.T, interactive bool) { +// swapSessionIsHeadless replaces the interactivity seam for the test. +func swapSessionIsHeadless(t *testing.T, headless bool) { t.Helper() - orig := stderrIsTerminal - stderrIsTerminal = func() bool { return interactive } - t.Cleanup(func() { stderrIsTerminal = orig }) + orig := sessionIsHeadless + sessionIsHeadless = func() bool { return headless } + t.Cleanup(func() { sessionIsHeadless = orig }) } // ensureOptions constructs a store's credstore through the seams and returns // the options it was built with. -func ensureOptions(t *testing.T, interactive bool) credstore.StoreOptions { +func ensureOptions(t *testing.T, headless bool) credstore.StoreOptions { t.Helper() t.Setenv("BASECAMP_NO_KEYRING", "1") // keep the delegated construction off the real keyring @@ -64,7 +64,7 @@ func ensureOptions(t *testing.T, interactive bool) credstore.StoreOptions { got = opts return credstore.NewStore(opts) }) - swapStderrIsTerminal(t, interactive) + swapSessionIsHeadless(t, headless) NewStore(t.TempDir()).UsingKeyring() return got @@ -75,6 +75,6 @@ func ensureOptions(t *testing.T, interactive bool) credstore.StoreOptions { // the unbounded probe so a legitimate unlock prompt is never cut off // mid-answer (which would silently degrade to plaintext file storage). func TestEnsureBoundsProbeOnlyWhenHeadless(t *testing.T) { - assert.Equal(t, headlessProbeTimeout, ensureOptions(t, false).ProbeTimeout) - assert.Zero(t, ensureOptions(t, true).ProbeTimeout) + assert.Equal(t, headlessProbeTimeout, ensureOptions(t, true).ProbeTimeout) + assert.Zero(t, ensureOptions(t, false).ProbeTimeout) } From ced0cbcd10216f2f12443f4e4498d5642722538e Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Mon, 27 Jul 2026 20:08:26 -0700 Subject: [PATCH 3/4] Recognize GUI sessions before bounding the probe (Refs #568) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit All three stdio streams being non-terminals is not proof of headlessness: a macOS app or IDE task runner launches the CLI fully detached while the user can still answer the keychain unlock dialog on the WindowServer. Headless now additionally requires no GUI session — on darwin via `launchctl managername` != Aqua (bounded, cannot touch the keychain, failures count as no GUI so true headless keeps the bounded probe); elsewhere via DISPLAY/WAYLAND_DISPLAY, with Windows always false since Credential Manager never prompts. Review feedback on #581. --- internal/auth/keyring.go | 15 +++++++++------ internal/auth/session_darwin.go | 23 +++++++++++++++++++++++ internal/auth/session_other.go | 13 +++++++++++++ internal/auth/session_other_test.go | 18 ++++++++++++++++++ 4 files changed, 63 insertions(+), 6 deletions(-) create mode 100644 internal/auth/session_darwin.go create mode 100644 internal/auth/session_other.go create mode 100644 internal/auth/session_other_test.go diff --git a/internal/auth/keyring.go b/internal/auth/keyring.go index 701f0984..208482e5 100644 --- a/internal/auth/keyring.go +++ b/internal/auth/keyring.go @@ -40,15 +40,18 @@ type Store struct { // newCredStore is replaceable in tests to avoid real keyring access. var newCredStore = credstore.NewStore -// sessionIsHeadless reports that no human is attached: stdin, stdout, and -// stderr are all non-terminals. Any one being a terminal counts as -// interactive — redirecting a stream or two (`2>auth.log`, `| jq`) is -// routine interactive use where a keychain unlock prompt can still be -// answered, on the controlling terminal or the GUI. Replaceable in tests. +// sessionIsHeadless reports that no human can answer a keyring unlock +// prompt: stdin, stdout, and stderr are all non-terminals AND no GUI +// session is available. Any attached stream counts as interactive — +// redirecting a stream or two (`2>auth.log`, `| jq`) is routine interactive +// use — and a GUI session (an app or IDE task runner launching the CLI with +// all streams detached) can still present the unlock dialog. Replaceable in +// tests. var sessionIsHeadless = func() bool { return !term.IsTerminal(os.Stdin.Fd()) && !term.IsTerminal(os.Stdout.Fd()) && - !term.IsTerminal(os.Stderr.Fd()) + !term.IsTerminal(os.Stderr.Fd()) && + !guiSessionAvailable() } // headlessProbeTimeout bounds the keyring availability probe when no diff --git a/internal/auth/session_darwin.go b/internal/auth/session_darwin.go new file mode 100644 index 00000000..0fe2e206 --- /dev/null +++ b/internal/auth/session_darwin.go @@ -0,0 +1,23 @@ +package auth + +import ( + "context" + "os/exec" + "strings" + "time" +) + +// guiSessionAvailable reports whether the process runs inside a macOS Aqua +// (GUI) session, where the keychain unlock prompt appears as a WindowServer +// dialog even with every standard stream detached — GUI-launched apps and +// IDE task runners. `launchctl managername` prints "Aqua" there; ssh and CI +// run under StandardIO or Background. launchctl only inspects the launchd +// session — it cannot touch the keychain — but is bounded anyway, and any +// failure counts as no GUI so genuinely headless sessions keep the bounded +// probe. +func guiSessionAvailable() bool { + ctx, cancel := context.WithTimeout(context.Background(), 2*time.Second) + defer cancel() + out, err := exec.CommandContext(ctx, "/bin/launchctl", "managername").Output() + return err == nil && strings.TrimSpace(string(out)) == "Aqua" +} diff --git a/internal/auth/session_other.go b/internal/auth/session_other.go new file mode 100644 index 00000000..3a5e9af0 --- /dev/null +++ b/internal/auth/session_other.go @@ -0,0 +1,13 @@ +//go:build !darwin + +package auth + +import "os" + +// guiSessionAvailable reports a display server the keyring's unlock prompt +// could use even with every standard stream detached. On Windows this is +// always false: Credential Manager operates without prompting, so a bounded +// probe cannot cut off an interaction there. +func guiSessionAvailable() bool { + return os.Getenv("DISPLAY") != "" || os.Getenv("WAYLAND_DISPLAY") != "" +} diff --git a/internal/auth/session_other_test.go b/internal/auth/session_other_test.go new file mode 100644 index 00000000..09e41b6a --- /dev/null +++ b/internal/auth/session_other_test.go @@ -0,0 +1,18 @@ +//go:build !darwin + +package auth + +import ( + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestGUISessionAvailableTracksDisplayEnv(t *testing.T) { + t.Setenv("DISPLAY", "") + t.Setenv("WAYLAND_DISPLAY", "") + assert.False(t, guiSessionAvailable()) + + t.Setenv("DISPLAY", ":0") + assert.True(t, guiSessionAvailable()) +} From 4eb07792bfe03bc6996c0f1f00ad30b39f0c65b5 Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Mon, 27 Jul 2026 20:21:13 -0700 Subject: [PATCH 4/4] Split the Windows GUI-session stub into its own file (Refs #568) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The !darwin implementation's claim that Windows is always non-GUI was false — that build returned true on any platform with DISPLAY set. A dedicated session_windows.go hard-codes false with the rationale, the unix implementation drops the stale claim, and the env test gains the Wayland-only branch. Review feedback on #581. --- internal/auth/session_other.go | 6 ++---- internal/auth/session_other_test.go | 6 +++++- internal/auth/session_windows.go | 9 +++++++++ 3 files changed, 16 insertions(+), 5 deletions(-) create mode 100644 internal/auth/session_windows.go diff --git a/internal/auth/session_other.go b/internal/auth/session_other.go index 3a5e9af0..ddbd20b9 100644 --- a/internal/auth/session_other.go +++ b/internal/auth/session_other.go @@ -1,13 +1,11 @@ -//go:build !darwin +//go:build !darwin && !windows package auth import "os" // guiSessionAvailable reports a display server the keyring's unlock prompt -// could use even with every standard stream detached. On Windows this is -// always false: Credential Manager operates without prompting, so a bounded -// probe cannot cut off an interaction there. +// could use even with every standard stream detached. func guiSessionAvailable() bool { return os.Getenv("DISPLAY") != "" || os.Getenv("WAYLAND_DISPLAY") != "" } diff --git a/internal/auth/session_other_test.go b/internal/auth/session_other_test.go index 09e41b6a..9e61c6ce 100644 --- a/internal/auth/session_other_test.go +++ b/internal/auth/session_other_test.go @@ -1,4 +1,4 @@ -//go:build !darwin +//go:build !darwin && !windows package auth @@ -15,4 +15,8 @@ func TestGUISessionAvailableTracksDisplayEnv(t *testing.T) { t.Setenv("DISPLAY", ":0") assert.True(t, guiSessionAvailable()) + + t.Setenv("DISPLAY", "") + t.Setenv("WAYLAND_DISPLAY", "wayland-0") + assert.True(t, guiSessionAvailable(), "Wayland-only sessions have no DISPLAY") } diff --git a/internal/auth/session_windows.go b/internal/auth/session_windows.go new file mode 100644 index 00000000..0e41782e --- /dev/null +++ b/internal/auth/session_windows.go @@ -0,0 +1,9 @@ +package auth + +// guiSessionAvailable is always false on Windows: Credential Manager +// operates without prompting, so a bounded probe cannot cut off an +// interaction, and misclassifying a GUI-launched session as headless is +// harmless here. +func guiSessionAvailable() bool { + return false +}