fix(auth): prevent stalled keyrings from hanging Linux commands

SECURITY.md

@@ -26,7 +26,25 @@ If system keyring is unavailable (headless servers, containers), set:
 export BASECAMP_NO_KEYRING=1
 ```
 
-Credentials will be stored in `~/.config/basecamp/credentials.json` with `0600` permissions.
+Credentials will be stored in `~/.config/basecamp/credentials.json` with `0600` permissions
+(or in the configured XDG config directory). This is plaintext storage, not encryption.
+Any non-empty `BASECAMP_NO_KEYRING` value bypasses the keyring before it is probed.
+
+On Linux, the availability probe and each later keyring operation are bounded
+by 10 seconds, including desktop sessions. An initial probe timeout uses the
+file fallback and prints a warning on the first credential read or write.
+Existing keyring credentials are not copied to the file: a fallback file may
+be absent or stale.
+
+After a successful probe, a later timeout returns an error; it never silently
+switches to plaintext or serves stale file credentials. The keyring library
+cannot cancel a started operation, so a timed-out write or delete may still
+complete. That store refuses further keyring operations for the rest of the
+process. Do not automatically retry a write whose outcome is unknown.
+
+On macOS and Windows, only a headless session (no terminal on any standard
+stream and no GUI session) bounds the availability probe; interactive sessions
+leave it unbounded so an unlock prompt is not cut off mid-answer.
 
 ## Supported Versions
 

go.mod

@@ -7,7 +7,7 @@ require (
 	charm.land/bubbletea/v2 v2.0.9
 	charm.land/lipgloss/v2 v2.0.6
 	github.com/basecamp/basecamp-sdk/go v0.19.0
-	github.com/basecamp/cli v0.2.2-0.20260828230226-767413fc712d
+	github.com/basecamp/cli v0.2.2-0.20260930131253-86242a69567b
 	github.com/basecamp/mcp v0.0.0-20260828100356-2d6f44b51e9d
 	github.com/basecamp/surfguard/go v0.1.0
 	github.com/charmbracelet/bubbles v1.0.0

go.sum

@@ -89,8 +89,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.19.0 h1:byygVVbJnWCZsyBNeAlztlUAV23ytLEhPx98WakNy+c=
 github.com/basecamp/basecamp-sdk/go v0.19.0/go.mod h1:kIBDYwPMMD59PadNGxpH0YTQuI+blFPZ8MelGI0RK5Q=
-github.com/basecamp/cli v0.2.2-0.20260828230226-767413fc712d h1:jAzDrCCzDpIwhbFT1xVVs0z2xpXoDEkomHfKB2bUUp8=
-github.com/basecamp/cli v0.2.2-0.20260828230226-767413fc712d/go.mod h1:iTBTaWvsPEFIcZfkxQHEfISyJ6sZ7036K6bNx0RY3EE=
+github.com/basecamp/cli v0.2.2-0.20260930131253-86242a69567b h1:R4M5+NKaDPlC6xvQYBPtIpI3L+iMKhentoMVdiveBd0=
+github.com/basecamp/cli v0.2.2-0.20260930131253-86242a69567b/go.mod h1:iTBTaWvsPEFIcZfkxQHEfISyJ6sZ7036K6bNx0RY3EE=
 github.com/basecamp/mcp v0.0.0-20260828100356-2d6f44b51e9d h1:zEQVGq1x1nhKMZ2TudFAcSJ32CHT8richI1vQakIKz4=
 github.com/basecamp/mcp v0.0.0-20260828100356-2d6f44b51e9d/go.mod h1:Ee2c/q1/pg+5T5741PIuA3s6VJMQC7I0XBNXIHIujzA=
 github.com/basecamp/surfguard/go v0.1.0 h1:JMo+MZQEOBRqnUylzD0/jS9S42Fp+XKWMG+4LWfu/XE=

internal/auth/issue800_keyring_linux_test.go

@@ -0,0 +1,125 @@
+//go:build linux
+
+package auth
+
+import (
+	"bufio"
+	"context"
+	"fmt"
+	"net"
+	"os"
+	"os/exec"
+	"path/filepath"
+	"strings"
+	"testing"
+	"time"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+
+	"github.com/basecamp/cli/credstore"
+)
+
+// Exercise the real credstore -> go-keyring -> godbus path, without accessing
+// the desktop's actual keyring. A subprocess isolates godbus's shared connection
+// and lets the test kill an unbounded handshake rather than hang the test suite.
+func TestIssue800DesktopKeyringHandshakeMustBeBounded(t *testing.T) {
+	t.Run("desktop", func(t *testing.T) { issue800ReadWithStalledBus(t, false, false) })
+	t.Run("headless_control", func(t *testing.T) { issue800ReadWithStalledBus(t, true, false) })
+	t.Run("no_keyring_control", func(t *testing.T) { issue800ReadWithStalledBus(t, false, true) })
+}
+
+func issue800ReadWithStalledBus(t *testing.T, headless, disable bool) {
+	t.Helper()
+	dir := t.TempDir()
+	fileStore := credstore.NewStore(credstore.StoreOptions{ForceFile: true, FallbackDir: dir})
+	require.NoError(t, fileStore.Save("profile:issue800", []byte(`{"access_token":"fallback-token"}`)))
+
+	// Use a short socket path: Unix socket addresses have a small length limit.
+	socketDir, err := os.MkdirTemp("", "issue800-")
+	require.NoError(t, err)
+	t.Cleanup(func() { _ = os.RemoveAll(socketDir) })
+	listener, err := (&net.ListenConfig{}).Listen(context.Background(), "unix", filepath.Join(socketDir, "bus"))
+	require.NoError(t, err)
+	defer listener.Close()
+
+	handshake := make(chan error, 1)
+	release := make(chan struct{})
+	defer close(release)
+	go func() {
+		conn, err := listener.Accept()
+		if err != nil {
+			handshake <- err
+			return
+		}
+		defer conn.Close()
+		_ = conn.SetDeadline(time.Now().Add(5 * time.Second))
+		reader := bufio.NewReader(conn)
+		line, err := reader.ReadString('\n')
+		if err == nil && line != "\x00AUTH\r\n" {
+			err = fmt.Errorf("unexpected initial authentication: %q", line)
+		}
+		if err == nil {
+			_, err = fmt.Fprint(conn, "REJECTED EXTERNAL\r\n")
+		}
+		if err == nil {
+			line, err = reader.ReadString('\n')
+			if err == nil && !strings.HasPrefix(line, "AUTH EXTERNAL") {
+				err = fmt.Errorf("unexpected authentication mechanism: %q", line)
+			}
+		}
+		handshake <- err
+		// Deliberately never send OK or an error. Holding the connection open
+		// recreates a stalled SASL/EXTERNAL exchange, not a connection failure.
+		<-release
+	}()
+
+	t.Setenv("ISSUE800_CREDENTIAL_HELPER", dir)
+	t.Setenv("DBUS_SESSION_BUS_ADDRESS", "unix:path="+filepath.Join(socketDir, "bus"))
+	t.Setenv("BASECAMP_NO_KEYRING", "")
+	t.Setenv("DISPLAY", "")
+	t.Setenv("WAYLAND_DISPLAY", "")
+	if !headless {
+		t.Setenv("WAYLAND_DISPLAY", "wayland-issue800")
+	}
+	if disable {
+		t.Setenv("BASECAMP_NO_KEYRING", "1")
+	}
+
+	executable, err := os.Executable()
+	require.NoError(t, err)
+	// Allow the existing ten-second headless probe budget plus scheduling
+	// margin. The desktop must also have a finite bound; currently it has none.
+	budget := headlessProbeTimeout + 5*time.Second
+	ctx, cancel := context.WithTimeout(context.Background(), budget)
+	defer cancel()
+	cmd := exec.CommandContext(ctx, executable, "-test.run=^TestIssue800CredentialReadHelper$", "-test.v")
+	out, runErr := cmd.CombinedOutput()
+	if !disable {
+		select {
+		case handshakeErr := <-handshake:
+			require.NoError(t, handshakeErr)
+		default:
+			t.Fatal("credential read never reached the fake D-Bus EXTERNAL handshake")
+		}
+	}
+	require.NoError(t, ctx.Err(), "credential read remained blocked in D-Bus authentication after %s; child output: %s", budget, out)
+	require.NoError(t, runErr, "%s", out)
+	assert.Contains(t, string(out), "fallback-token loaded")
+	if !disable {
+		assert.Contains(t, string(out), "keyring probe timed out")
+	} else {
+		assert.NotContains(t, string(out), "warning:")
+	}
+}
+
+func TestIssue800CredentialReadHelper(t *testing.T) {
+	dir := os.Getenv("ISSUE800_CREDENTIAL_HELPER")
+	if dir == "" {
+		t.Skip("subprocess helper")
+	}
+	creds, err := NewStore(dir).Load("profile:issue800")
+	require.NoError(t, err)
+	require.Equal(t, "fallback-token", creds.AccessToken)
+	t.Log("fallback-token loaded")
+}

internal/auth/keyring.go

@@ -6,6 +6,7 @@ import (
 	"errors"
 	"fmt"
 	"os"
+	"runtime"
 	"strings"
 	"sync"
 	"time"
@@ -114,9 +115,10 @@ var sessionIsHeadless = func() bool {
 // 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.
+// incident class. Linux bounds probes and later operations even with a GUI
+// or TTY: the Secret Service D-Bus exchange can stall independently of any
+// unlock prompt (#800). Other platforms keep their interactive probe
+// unbounded so an unlock prompt is not cut off mid-answer.
 const headlessProbeTimeout = 10 * time.Second
 
 // NewStore creates a credential store. The OS keyring is not touched until
@@ -135,7 +137,10 @@ func (s *Store) ensure() credStore {
 			DisableEnvVar: "BASECAMP_NO_KEYRING",
 			FallbackDir:   s.fallbackDir,
 		}
-		if sessionIsHeadless() {
+		if runtime.GOOS == "linux" {
+			opts.ProbeTimeout = headlessProbeTimeout
+			opts.OperationTimeout = headlessProbeTimeout
+		} else if sessionIsHeadless() {
 			opts.ProbeTimeout = headlessProbeTimeout
 		}
 		s.inner = newCredStore(opts)
@@ -187,7 +192,7 @@ func (s *Store) load(origin string, req lockRequest) (*Credentials, error) {
 		if isMissingCredential(err) {
 			return nil, fmt.Errorf("%w: %w", ErrNoCredential, err)
 		}
-		return nil, err
+		return nil, s.operationError(err)
 	}
 	var creds Credentials
 	if err := json.Unmarshal(data, &creds); err != nil {
@@ -240,13 +245,13 @@ func (s *Store) save(under func(func() error) error, origin string, creds *Crede
 	if err != nil {
 		return err
 	}
-	return under(func() error { return s.ensure().Save(origin, data) })
+	return s.operationError(under(func() error { return s.ensure().Save(origin, data) }))
 }
 
 // Delete removes credentials for the given origin. Locked for the same
 // reason Save is: the file backend rewrites the whole document.
 func (s *Store) Delete(origin string) error {
-	return s.withStoreLock(func() error { return s.ensure().Delete(origin) })
+	return s.operationError(s.withStoreLock(func() error { return s.ensure().Delete(origin) }))
 }
 
 // MigrateToKeyring migrates credentials from file to keyring. It reads
@@ -264,8 +269,29 @@ func (s *Store) Delete(origin string) error {
 // migration can therefore be re-saved from the file copy — one stale
 // credential, one login to repair, against a deadlock in the common path.
 func (s *Store) MigrateToKeyring() error {
-	return s.withStoreFileLock(func() error { return s.ensure().MigrateToKeyring() })
+	return s.operationError(s.withStoreFileLock(func() error { return s.ensure().MigrateToKeyring() }))
 }
 
 // UsingKeyring returns true if the store is using the system keyring.
 func (s *Store) UsingKeyring() bool { return s.ensure().UsingKeyring() }
+
+// operationError offers the file-storage remedy only while the keyring is the
+// store in use. After a failed initial probe the store is already on the file
+// backend, whose errors carry the probe's timeout but which the remedy
+// cannot fix.
+func (s *Store) operationError(err error) error {
+	if err == nil || !s.ensure().UsingKeyring() {
+		return err
+	}
+	return keyringOperationError(err)
+}
+
+// A keyring operation timeout is not a missing login. Keep the wrapped error
+// and offer the explicit, warned choice of file storage rather than silently
+// switching away from a keyring whose write may still complete.
+func keyringOperationError(err error) error {
+	if errors.Is(err, context.DeadlineExceeded) && strings.Contains(err.Error(), "keyring") {
+		return fmt.Errorf("%w; to use plaintext credential storage explicitly, set BASECAMP_NO_KEYRING=1", err)
+	}
+	return err
+}

internal/auth/keyring_test.go

@@ -1,8 +1,11 @@
 package auth
 
 import (
+	"context"
+	"fmt"
 	"io"
 	"os"
+	"runtime"
 	"strings"
 	"testing"
 
@@ -73,13 +76,22 @@ func ensureOptions(t *testing.T, headless bool) credstore.StoreOptions {
 	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)
+// Linux must bound a D-Bus stall independently of GUI/TTY availability.
+// Other platforms retain the headless-only probe bound and do not opt in
+// to operation timeouts, so their interactive unlock prompts are unchanged.
+func TestEnsureBoundsLinuxKeyringAndHeadlessProbes(t *testing.T) {
+	headless := ensureOptions(t, true)
+	interactive := ensureOptions(t, false)
+	assert.Equal(t, headlessProbeTimeout, headless.ProbeTimeout)
+	if runtime.GOOS == "linux" {
+		assert.Equal(t, headlessProbeTimeout, interactive.ProbeTimeout)
+		assert.Equal(t, headlessProbeTimeout, headless.OperationTimeout)
+		assert.Equal(t, headlessProbeTimeout, interactive.OperationTimeout)
+	} else {
+		assert.Zero(t, interactive.ProbeTimeout)
+		assert.Zero(t, headless.OperationTimeout)
+		assert.Zero(t, interactive.OperationTimeout)
+	}
 }
 
 // fallenBackStore stands in for a credstore.Store whose keyring probe failed
@@ -93,6 +105,52 @@ func (f *fallenBackStore) MigrateToKeyring() error     { return nil }
 func (f *fallenBackStore) UsingKeyring() bool          { return false }
 func (f *fallenBackStore) FallbackWarning() string     { return f.warning }
 
+type timedOutKeyringStore struct{ fallenBackStore }
+
+func (*timedOutKeyringStore) Load(string) ([]byte, error) { return nil, keyringTimeoutForTest() }
+func (*timedOutKeyringStore) Save(string, []byte) error   { return keyringTimeoutForTest() }
+func (*timedOutKeyringStore) Delete(string) error         { return keyringTimeoutForTest() }
+func (*timedOutKeyringStore) MigrateToKeyring() error     { return keyringTimeoutForTest() }
+func (*timedOutKeyringStore) UsingKeyring() bool          { return true }
+
+func keyringTimeoutForTest() error {
+	return fmt.Errorf("keyring operation timed out: %w", context.DeadlineExceeded)
+}
+
+func TestKeyringTimeoutIsAnErrorWithAnExplicitFileStorageRemedy(t *testing.T) {
+	swapNewCredStore(t, func(credstore.StoreOptions) credStore { return &timedOutKeyringStore{} })
+	store := NewStore(t.TempDir())
+	load := func() error { _, err := store.Load("work"); return err }
+	save := func() error { return store.Save("work", &Credentials{AccessToken: "token"}) }
+	del := func() error { return store.Delete("work") }
+	for _, operation := range []func() error{load, save, del, store.MigrateToKeyring} {
+		err := operation()
+		require.ErrorIs(t, err, context.DeadlineExceeded)
+		assert.NotErrorIs(t, err, ErrNoCredential)
+		assert.ErrorContains(t, err, "BASECAMP_NO_KEYRING=1")
+		assert.ErrorContains(t, err, "plaintext")
+	}
+	assert.True(t, store.UsingKeyring())
+	assert.ErrorIs(t, keyringOperationError(context.DeadlineExceeded), context.DeadlineExceeded)
+	assert.NotContains(t, keyringOperationError(context.DeadlineExceeded).Error(), "BASECAMP_NO_KEYRING")
+}
+
+// timedOutProbeFileStore is the file backend after a keyring probe timeout:
+// its failures carry the probe's deadline, but the keyring is not in use.
+type timedOutProbeFileStore struct{ fallenBackStore }
+
+func (*timedOutProbeFileStore) Load(string) ([]byte, error) {
+	return nil, fmt.Errorf("reading credentials.json: permission denied (%w)", keyringTimeoutForTest())
+}
+
+func TestFileStoreErrorsAfterAProbeTimeoutOmitTheFileStorageRemedy(t *testing.T) {
+	swapNewCredStore(t, func(credstore.StoreOptions) credStore { return &timedOutProbeFileStore{} })
+	store := NewStore(t.TempDir())
+	_, err := store.Load("work")
+	require.ErrorIs(t, err, context.DeadlineExceeded)
+	assert.NotContains(t, err.Error(), "BASECAMP_NO_KEYRING")
+}
+
 // captureStderr returns what the callback wrote to os.Stderr.
 func captureStderr(t *testing.T, fn func()) string {
 	t.Helper()

internal/cli/help.go

@@ -215,6 +215,14 @@ func renderRootHelp(w io.Writer, cmd *cobra.Command) {
 		}
 	}
 
+	// CREDENTIAL STORAGE — the escape hatch must be visible before a
+	// stalled keyring prevents doctor or auth status from answering.
+	b.WriteString("\n")
+	b.WriteString(r.Header.Render("CREDENTIAL STORAGE"))
+	b.WriteString("\n")
+	b.WriteString("  Set BASECAMP_NO_KEYRING=1 to bypass the system keyring.\n")
+	b.WriteString("  Uses plaintext credentials.json in the config directory (mode 0600).\n")
+
 	// EXAMPLES
 	b.WriteString("\n")
 	b.WriteString(r.Header.Render("EXAMPLES"))

internal/cli/help_test.go

@@ -48,6 +48,23 @@ func TestRootHelpContainsCategoryHeaders(t *testing.T) {
 	assert.Contains(t, out, "FLAGS")
 }
 
+func TestKeyringBypassIsDiscoverableInHelp(t *testing.T) {
+	for _, args := range [][]string{{"--help"}, {"auth", "--help"}, {"doctor", "--help"}} {
+		t.Run(strings.Join(args, " "), func(t *testing.T) {
+			isolateHelpTest(t)
+			var buf bytes.Buffer
+			cmd := NewRootCmd()
+			cmd.AddCommand(commands.NewAuthCmd(), commands.NewDoctorCmd())
+			cmd.SetOut(&buf)
+			cmd.SetArgs(args)
+			require.NoError(t, cmd.Execute())
+			assert.Contains(t, buf.String(), "BASECAMP_NO_KEYRING=1")
+			assert.Contains(t, buf.String(), "plaintext")
+			assert.Contains(t, buf.String(), "0600")
+		})
+	}
+}
+
 func TestRootHelpContainsExamples(t *testing.T) {
 	isolateHelpTest(t)