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..208482e5 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,30 @@ type Store struct { // newCredStore is replaceable in tests to avoid real keyring access. var newCredStore = credstore.NewStore +// 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()) && + !guiSessionAvailable() +} + +// headlessProbeTimeout bounds the keyring availability probe when no +// 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 // the first credential operation. func NewStore(fallbackDir string) *Store { @@ -49,11 +75,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 sessionIsHeadless() { + 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..22b6701c 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") } + +// swapSessionIsHeadless replaces the interactivity seam for the test. +func swapSessionIsHeadless(t *testing.T, headless bool) { + t.Helper() + 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, headless 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) + }) + swapSessionIsHeadless(t, headless) + + 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, true).ProbeTimeout) + assert.Zero(t, ensureOptions(t, false).ProbeTimeout) +} 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..ddbd20b9 --- /dev/null +++ b/internal/auth/session_other.go @@ -0,0 +1,11 @@ +//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. +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..9e61c6ce --- /dev/null +++ b/internal/auth/session_other_test.go @@ -0,0 +1,22 @@ +//go:build !darwin && !windows + +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()) + + 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 +}