diff --git a/SECURITY.md b/SECURITY.md index 64d11a981..afbe5619d 100644 --- a/SECURITY.md +++ b/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 diff --git a/go.mod b/go.mod index 14709182e..4b4170253 100644 --- a/go.mod +++ b/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 diff --git a/go.sum b/go.sum index 389256aba..bd8e5dc39 100644 --- a/go.sum +++ b/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= diff --git a/internal/auth/issue800_keyring_linux_test.go b/internal/auth/issue800_keyring_linux_test.go new file mode 100644 index 000000000..ffc78de62 --- /dev/null +++ b/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") +} diff --git a/internal/auth/keyring.go b/internal/auth/keyring.go index d041c3796..c1454066f 100644 --- a/internal/auth/keyring.go +++ b/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 +} diff --git a/internal/auth/keyring_test.go b/internal/auth/keyring_test.go index 3b67f021b..285df30d1 100644 --- a/internal/auth/keyring_test.go +++ b/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() diff --git a/internal/cli/help.go b/internal/cli/help.go index 5ea87c669..ee7604eda 100644 --- a/internal/cli/help.go +++ b/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")) diff --git a/internal/cli/help_test.go b/internal/cli/help_test.go index 3eacb64c2..356a9e078 100644 --- a/internal/cli/help_test.go +++ b/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) diff --git a/internal/commands/auth.go b/internal/commands/auth.go index fef3eedbb..8baa6a53b 100644 --- a/internal/commands/auth.go +++ b/internal/commands/auth.go @@ -33,7 +33,15 @@ func NewAuthCmd() *cobra.Command { cmd := &cobra.Command{ Use: "auth", Short: "Manage authentication", - Long: "Manage Basecamp authentication including login, logout, and status.", + Long: `Manage Basecamp authentication including login, logout, and status. + +Credentials use the system keyring when available. Set BASECAMP_NO_KEYRING=1 +before running a command to bypass the keyring and use plaintext credentials +in the config directory (credentials.json, mode 0600). + +On Linux, keyring probes and operations time out after 10 seconds. An initial +probe failure uses the warned file fallback. A later operation timeout returns +an error without switching storage; a timed-out write may still complete.`, } cmd.AddCommand( diff --git a/internal/commands/auth_status_agent_test.go b/internal/commands/auth_status_agent_test.go index 9b33e3a86..747260c32 100644 --- a/internal/commands/auth_status_agent_test.go +++ b/internal/commands/auth_status_agent_test.go @@ -115,7 +115,7 @@ func TestDoctorOffersTheAgentLoginForABrokenAgent(t *testing.T) { assert.Contains(t, check.Hint, "--with-client-credentials") assert.NotContains(t, check.Hint, "Run: basecamp auth login -P") - crumbs := buildDoctorBreadcrumbs([]Check{{Name: "Credentials", Status: "fail"}}, app.Auth.LoginCommand()) + crumbs := buildDoctorBreadcrumbs([]Check{{Name: "Credentials", Status: "fail", Hint: app.Auth.LoginHint()}}, app.Auth.LoginCommand()) require.NotEmpty(t, crumbs) assert.Contains(t, crumbs[0].Cmd, "--with-client-credentials") assert.Contains(t, crumbs[0].Cmd, "--client-id agent-client") @@ -129,10 +129,22 @@ func TestDoctorOffersTheAgentLoginForABrokenAgent(t *testing.T) { ClientSecret: "agent-secret", TokenEndpoint: srv.URL + "/oauth/tokens", })) - credentials := checkCredentials(app, false) + credentials := checkCredentials(context.Background(), app, false) assert.Equal(t, "fail", credentials.Status) assert.Contains(t, credentials.Hint, "--with-client-credentials") assert.Contains(t, app.Auth.LoginCommand(), "--client-id agent-client") + + // The breadcrumb doctor builds from its own checks carries the agent's + // login too, not only a hand-built Credentials row. + var logins []string + for _, crumb := range buildDoctorBreadcrumbs(runDoctorChecks(context.Background(), app, false), app.Auth.LoginCommand()) { + if crumb.Action == "login" { + logins = append(logins, crumb.Cmd) + } + } + require.Len(t, logins, 1) + assert.Contains(t, logins[0], "--with-client-credentials") + assert.Contains(t, logins[0], "--client-id agent-client") } // TestAuthStatusNamesTheKindEvenWhenItCannotAuthenticate: a credential can diff --git a/internal/commands/doctor.go b/internal/commands/doctor.go index 00a03a3f4..d7d6ccb1b 100644 --- a/internal/commands/doctor.go +++ b/internal/commands/doctor.go @@ -90,6 +90,11 @@ The doctor command helps troubleshoot common issues by checking: - Cache directory health - Shell completion status +If the system keyring stalls, set BASECAMP_NO_KEYRING=1 before running doctor. +This explicitly selects plaintext credential storage (credentials.json, mode +0600); it does not copy credentials out of the keyring. Linux keyring checks +have a 10-second deadline, including the best-effort legacy-install lookup. + Examples: basecamp doctor # Run all diagnostic checks basecamp doctor --json # Output results as JSON @@ -157,7 +162,7 @@ func runDoctorChecks(ctx context.Context, app *appctx.App, verbose bool) []Check } // 6. Credentials check - credCheck := checkCredentials(app, verbose) + credCheck, unreadable := checkStoredCredentials(ctx, app, verbose) checks = append(checks, credCheck) // 7. Authentication check (only if credentials exist) @@ -166,6 +171,12 @@ func runDoctorChecks(ctx context.Context, app *appctx.App, verbose bool) []Check authCheck := checkAuthentication(ctx, app, verbose) checks = append(checks, authCheck) canTestAPI = authCheck.Status == "pass" || authCheck.Status == "warn" + } else if unreadable { + checks = append(checks, Check{ + Name: "Authentication", + Status: "skip", + Message: "Skipped (credentials could not be read)", + }) } else { checks = append(checks, Check{ Name: "Authentication", @@ -215,7 +226,7 @@ func runDoctorChecks(ctx context.Context, app *appctx.App, verbose bool) []Check checks = append(checks, checkShellCompletion(verbose)) // 12. Legacy bcq detection - if legacyCheck := checkLegacyInstall(); legacyCheck != nil { + if legacyCheck := checkLegacyInstall(ctx); legacyCheck != nil { checks = append(checks, *legacyCheck) } @@ -702,7 +713,15 @@ func validateConfigFile(path, name string, verbose bool) Check { } // checkCredentials checks for stored credentials. -func checkCredentials(app *appctx.App, verbose bool) Check { +func checkCredentials(ctx context.Context, app *appctx.App, verbose bool) Check { + check, _ := checkStoredCredentials(ctx, app, verbose) + return check +} + +// checkStoredCredentials is checkCredentials that also reports whether the +// failure was a store that could not be read, as opposed to a missing or +// unusable credential that a login would repair. +func checkStoredCredentials(ctx context.Context, app *appctx.App, verbose bool) (Check, bool) { check := Check{ Name: "Credentials", } @@ -711,26 +730,40 @@ func checkCredentials(app *appctx.App, verbose bool) Check { if envToken := os.Getenv("BASECAMP_TOKEN"); envToken != "" { check.Status = "pass" check.Message = "Using BASECAMP_TOKEN environment variable" - return check + return check, false } - // Check if authenticated (works for both keyring and file storage) - if !app.Auth.IsAuthenticated() { + // An unreadable store is not a missing login: preserve a keyring timeout + // and its explicit file-storage workaround instead of asking for OAuth. + authenticated, authErr := app.Auth.CheckAuthenticated(ctx) + if authErr != nil { + problem := output.AsError(authErr) + check.Status = "fail" + check.Message = "Could not read stored credentials: " + problem.Message + check.Hint = problem.Hint + if problem.Code == output.CodeAuth { + // A stored but unusable credential already carries the remedy + // for its kind; it is not a failure to read the store. + check.Message = problem.Message + return check, false + } + return check, true + } + if !authenticated { check.Status = "fail" check.Message = "No credentials found" check.Hint = app.Auth.LoginHint() - return check + return check, false } // Try to load credentials for details credKey := app.Auth.CredentialKey() store := app.Auth.GetStore() - creds, err := store.Load(credKey) + creds, err := store.LoadContext(ctx, credKey) if err != nil { - // Authenticated but can't load details - still pass but note the issue - check.Status = "pass" - check.Message = "Stored (via system keyring)" - return check + check.Status = "fail" + check.Message = "Could not read stored credentials: " + err.Error() + return check, true } check.Status = "pass" @@ -756,7 +789,7 @@ func checkCredentials(app *appctx.App, verbose bool) Check { check.Message = credsPath } } - return check + return check, false } // checkAuthentication checks token validity. @@ -1127,6 +1160,12 @@ func buildDoctorBreadcrumbs(checks []Check, login string) []output.Breadcrumb { switch c.Name { case "Credentials", "Authentication": + // Credential checks carry a login hint only when the store was + // readable and a login would repair what it reported. Do not + // invent that remedy for a timeout or another read failure. + if c.Name == "Credentials" && !strings.Contains(c.Hint, login) { + continue + } breadcrumbs = append(breadcrumbs, output.Breadcrumb{ Action: "login", Cmd: login, @@ -1311,7 +1350,7 @@ func checkSkillVersion() Check { // checkLegacyInstall detects stale bcq artifacts and suggests migration. // Returns nil if no legacy artifacts are found (to avoid noisy output). -func checkLegacyInstall() *Check { +func checkLegacyInstall(ctx context.Context) *Check { home, err := os.UserHomeDir() if err != nil { return nil @@ -1355,12 +1394,8 @@ func checkLegacyInstall() *Check { // Skip when BASECAMP_NO_KEYRING is set (headless/CI environments) if os.Getenv("BASECAMP_NO_KEYRING") == "" { configDir := filepath.Join(configBase, "basecamp") - for _, origin := range collectKnownOrigins(configDir) { - legacyKey := fmt.Sprintf("bcq::%s", origin) - if _, err := keyring.Get("bcq", legacyKey); err == nil { - found = append(found, "keyring(bcq::*)") - break - } + if legacyKeyringEntryExists(ctx, collectKnownOrigins(configDir), legacyKeyringTimeout) { + found = append(found, "keyring(bcq::*)") } } @@ -1376,6 +1411,43 @@ func checkLegacyInstall() *Check { } } +var legacyKeyringGet = keyring.Get + +// legacyKeyringEntryExists is best-effort: doctor must not hang in its legacy +// probe after the active credential store has already timed out. A single +// budget covers all origins, and a late read never starts the next lookup. +func legacyKeyringEntryExists(ctx context.Context, origins []string, timeout time.Duration) bool { + if timeout > 0 { + var cancel context.CancelFunc + ctx, cancel = context.WithTimeout(ctx, timeout) + defer cancel() + } + get := legacyKeyringGet + lookup := func() bool { + for _, origin := range origins { + if ctx.Err() != nil { + return false + } + legacyKey := "bcq::" + origin + if _, err := get("bcq", legacyKey); err == nil { + return true + } + } + return false + } + if timeout <= 0 { + return lookup() + } + done := make(chan bool, 1) + go func() { done <- lookup() }() + select { + case found := <-done: + return found + case <-ctx.Done(): + return false + } +} + // checkConnectorSessionPaths reports whether a task token's unix socket fits // under the session directory this profile's connector would use. A unix // socket path is 103 bytes at most, and a long home, a deep XDG_RUNTIME_DIR diff --git a/internal/commands/doctor_test.go b/internal/commands/doctor_test.go index 11daa5c06..dc73c7904 100644 --- a/internal/commands/doctor_test.go +++ b/internal/commands/doctor_test.go @@ -5,12 +5,15 @@ import ( "context" "debug/pe" "encoding/binary" + "errors" "net/http" "os" "path/filepath" "runtime" "strings" + "sync/atomic" "testing" + "time" "github.com/spf13/cobra" "github.com/stretchr/testify/assert" @@ -298,7 +301,7 @@ func TestValidateConfigFile(t *testing.T) { func TestBuildDoctorBreadcrumbs(t *testing.T) { checks := []Check{ - {Name: "Credentials", Status: "fail"}, + {Name: "Credentials", Status: "fail", Hint: "Run: basecamp auth login"}, {Name: "Authentication", Status: "fail"}, {Name: "API Connectivity", Status: "pass"}, } @@ -313,7 +316,7 @@ func TestBuildDoctorBreadcrumbs(t *testing.T) { func TestBuildDoctorBreadcrumbsDeduplication(t *testing.T) { // Both Credentials and Authentication fail - should only suggest login once checks := []Check{ - {Name: "Credentials", Status: "fail"}, + {Name: "Credentials", Status: "fail", Hint: "Run: basecamp auth login"}, {Name: "Authentication", Status: "fail"}, } @@ -379,6 +382,64 @@ func executeDoctorCommand(cmd *cobra.Command, app *appctx.App, args ...string) e return cmd.Execute() } +func TestDoctorDoesNotCallAnUnreadableStoreAMissingLogin(t *testing.T) { + app, _ := setupDoctorTestApp(t, "12345") + // A directory where the credential file should be is a read failure, + // not a missing credential. The same branch reports keyring timeouts. + require.NoError(t, os.MkdirAll(filepath.Join(config.GlobalConfigDir(), "credentials.json"), 0700)) + check, unreadable := checkStoredCredentials(context.Background(), app, false) + assert.True(t, unreadable, "doctor must not skip authentication as if there were no credentials") + assert.Equal(t, "fail", check.Status) + assert.Contains(t, check.Message, "Could not read stored credentials") + assert.NotContains(t, check.Message, "No credentials found") + assert.Empty(t, check.Hint, "an unreadable store must not prescribe another OAuth login") + + checks := runDoctorChecks(context.Background(), app, false) + for _, crumb := range buildDoctorBreadcrumbs(checks, app.Auth.LoginCommand()) { + assert.NotEqual(t, "login", crumb.Action, "an unreadable store must not prescribe login through a breadcrumb either") + } +} + +func TestDoctorLegacyKeyringProbeIsBounded(t *testing.T) { + original := legacyKeyringGet + t.Cleanup(func() { legacyKeyringGet = original }) + var calls atomic.Int32 + release, finished := make(chan struct{}), make(chan struct{}) + legacyKeyringGet = func(string, string) (string, error) { + calls.Add(1) + <-release + defer close(finished) + return "", errors.New("unavailable") + } + t.Cleanup(func() { close(release); <-finished }) + result := make(chan bool, 1) + go func() { + result <- legacyKeyringEntryExists(context.Background(), []string{"first", "second"}, 40*time.Millisecond) + }() + select { + case found := <-result: + assert.False(t, found) + case <-time.After(time.Second): + t.Fatal("doctor's legacy keyring lookup ignored its deadline") + } + assert.EqualValues(t, 1, calls.Load()) +} + +func TestDoctorLegacyKeyringProbeFindsHealthyEntries(t *testing.T) { + original := legacyKeyringGet + t.Cleanup(func() { legacyKeyringGet = original }) + legacyKeyringGet = func(service, key string) (string, error) { + assert.Equal(t, "bcq", service) + if key == "bcq::second" { + return "token", nil + } + return "", errors.New("not found") + } + assert.True(t, legacyKeyringEntryExists(context.Background(), []string{"first", "second"}, time.Second)) + assert.True(t, legacyKeyringEntryExists(context.Background(), []string{"first", "second"}, 0)) + assert.False(t, legacyKeyringEntryExists(context.Background(), []string{"first"}, time.Second)) +} + func TestDoctorCommandCreation(t *testing.T) { cmd := NewDoctorCmd() assert.Equal(t, "doctor", cmd.Use) @@ -466,7 +527,7 @@ func TestCheckLegacyInstall_DetectsLegacyCache(t *testing.T) { // Create legacy cache dir require.NoError(t, os.MkdirAll(filepath.Join(cacheBase, "bcq"), 0700)) - check := checkLegacyInstall() + check := checkLegacyInstall(context.Background()) require.NotNil(t, check, "should detect legacy cache dir") assert.Equal(t, "warn", check.Status) assert.Contains(t, check.Message, filepath.Join(cacheBase, "bcq")) @@ -483,7 +544,7 @@ func TestCheckLegacyInstall_DetectsLegacyTheme(t *testing.T) { // Create legacy theme dir require.NoError(t, os.MkdirAll(filepath.Join(configBase, "bcq", "theme"), 0700)) - check := checkLegacyInstall() + check := checkLegacyInstall(context.Background()) require.NotNil(t, check, "should detect legacy theme dir") assert.Equal(t, "warn", check.Status) assert.Contains(t, check.Message, filepath.Join(configBase, "bcq", "theme")) @@ -499,7 +560,7 @@ func TestCheckLegacyInstall_DetectsBothArtifacts(t *testing.T) { require.NoError(t, os.MkdirAll(filepath.Join(cacheBase, "bcq"), 0700)) require.NoError(t, os.MkdirAll(filepath.Join(configBase, "bcq", "theme"), 0700)) - check := checkLegacyInstall() + check := checkLegacyInstall(context.Background()) require.NotNil(t, check) assert.Contains(t, check.Message, "bcq") assert.Contains(t, check.Message, "theme") @@ -510,7 +571,7 @@ func TestCheckLegacyInstall_NilWhenClean(t *testing.T) { t.Setenv("XDG_CACHE_HOME", t.TempDir()) t.Setenv("XDG_CONFIG_HOME", t.TempDir()) - check := checkLegacyInstall() + check := checkLegacyInstall(context.Background()) assert.Nil(t, check, "should return nil when no legacy artifacts exist") } @@ -529,7 +590,7 @@ func TestCheckLegacyInstall_NilWhenAlreadyMigrated(t *testing.T) { require.NoError(t, os.MkdirAll(markerDir, 0700)) require.NoError(t, os.WriteFile(filepath.Join(markerDir, ".migrated"), []byte("migrated\n"), 0600)) - check := checkLegacyInstall() + check := checkLegacyInstall(context.Background()) assert.Nil(t, check, "should return nil when .migrated marker exists") } @@ -856,7 +917,7 @@ func TestCheckLegacyInstall_SkipsKeyringWhenNoKeyring(t *testing.T) { // With BASECAMP_NO_KEYRING set, even if legacy keyring entries existed, // the function should not probe the keyring and should return nil - check := checkLegacyInstall() + check := checkLegacyInstall(context.Background()) assert.Nil(t, check) } @@ -871,7 +932,7 @@ func TestDoctorVerboseHidesLaunchpadScope(t *testing.T) { Scope: "read", })) - check := checkCredentials(app, true) + check := checkCredentials(context.Background(), app, true) assert.Equal(t, "pass", check.Status) assert.NotContains(t, check.Message, "scope:", "Launchpad scope should not appear in verbose output") assert.Contains(t, check.Message, "type: launchpad") @@ -888,7 +949,7 @@ func TestDoctorVerboseShowsBC3Scope(t *testing.T) { Scope: "read", })) - check := checkCredentials(app, true) + check := checkCredentials(context.Background(), app, true) assert.Equal(t, "pass", check.Status) assert.Contains(t, check.Message, "scope: read", "BC3 scope should appear in verbose output") assert.Contains(t, check.Message, "type: bc3") @@ -928,3 +989,21 @@ func TestAttachGitHubAuthFallsBackToGithubToken(t *testing.T) { func buildDoctorBreadcrumbsForTest(checks []Check) []output.Breadcrumb { return buildDoctorBreadcrumbs(checks, "basecamp auth login") } + +// TestDoctorStillPrescribesLoginForMissingCredentials: the unreadable-store +// guard must not swallow the breadcrumb for a store that was read and held +// nothing — that is exactly the case a login repairs. +func TestDoctorStillPrescribesLoginForMissingCredentials(t *testing.T) { + app, _ := setupDoctorTestApp(t, "12345") + t.Setenv("BASECAMP_TOKEN", "") + + checks := runDoctorChecks(context.Background(), app, false) + var logins []string + for _, crumb := range buildDoctorBreadcrumbs(checks, app.Auth.LoginCommand()) { + if crumb.Action == "login" { + logins = append(logins, crumb.Cmd) + } + } + require.Len(t, logins, 1, "a missing credential must still offer exactly one login") + assert.Equal(t, app.Auth.LoginCommand(), logins[0]) +} diff --git a/internal/commands/migrate.go b/internal/commands/migrate.go index c265792a6..4467ee184 100644 --- a/internal/commands/migrate.go +++ b/internal/commands/migrate.go @@ -1,12 +1,17 @@ package commands import ( + "context" "encoding/json" + "errors" "fmt" "io/fs" "os" "path/filepath" + "runtime" "strings" + "sync/atomic" + "time" "github.com/spf13/cobra" "github.com/zalando/go-keyring" @@ -77,8 +82,11 @@ func runMigrate(cmd *cobra.Command, force bool) error { result := &MigrateResult{} - // 1. Migrate keyring entries - migrateKeyring(result, configDir) + // 1. Migrate keyring entries, unless the keyring is bypassed + keyringSkipped := os.Getenv("BASECAMP_NO_KEYRING") != "" + if !keyringSkipped { + migrateKeyring(result, configDir) + } // 2. Migrate cache directory migrateCache(result) @@ -100,11 +108,15 @@ func runMigrate(cmd *cobra.Command, force bool) error { if result.ThemeMoved { parts = append(parts, "theme migrated") } + if keyringSkipped { + parts = append(parts, "keyring skipped (BASECAMP_NO_KEYRING set)") + } - // Only write marker when something actually migrated and no errors occurred + // Only write marker when something actually migrated, no errors occurred, + // and the keyring step ran: a skipped keyring may still hold bcq entries. migrated := result.KeyringMigrated > 0 || result.CacheMoved || result.ThemeMoved hasErrors := len(result.KeyringErrors) > 0 - if migrated && !hasErrors { + if migrated && !hasErrors && !keyringSkipped { if err := os.MkdirAll(configDir, 0700); err == nil { _ = os.WriteFile(markerPath, []byte("migrated\n"), 0600) } @@ -148,38 +160,109 @@ type keyringFuncs struct { delete func(service, key string) error } +// legacyKeyringTimeout bounds the direct go-keyring calls made for the bcq +// legacy entries: each call in migrate, the whole lookup in doctor. A Linux +// Secret Service call can stall on D-Bus forever (#800); other platforms keep +// their interactive unlock prompts unbounded. +var legacyKeyringTimeout = func() time.Duration { + if runtime.GOOS == "linux" { + return 10 * time.Second + } + return 0 +}() + +var errLegacyKeyringTimeout = fmt.Errorf("keyring operation timed out: %w; set BASECAMP_NO_KEYRING=1 to skip the keyring", context.DeadlineExceeded) + +// bounded returns ops whose calls give up after timeout. Once one call times +// out every later call fails at once: a stalled D-Bus connection does not +// recover within the process, and each call would wait out the full budget. +func (ops keyringFuncs) bounded(timeout time.Duration) keyringFuncs { + if timeout <= 0 { + return ops + } + var stalled atomic.Bool + run := func(fn func() error) error { + if stalled.Load() { + return errLegacyKeyringTimeout + } + done := make(chan error, 1) + go func() { done <- fn() }() + timer := time.NewTimer(timeout) + defer timer.Stop() + select { + case err := <-done: + return err + case <-timer.C: + stalled.Store(true) + return errLegacyKeyringTimeout + } + } + return keyringFuncs{ + get: func(service, key string) (string, error) { + var data string + err := run(func() error { + var err error + data, err = ops.get(service, key) + return err + }) + if err != nil { + return "", err + } + return data, nil + }, + set: func(service, key, data string) error { + return run(func() error { return ops.set(service, key, data) }) + }, + delete: func(service, key string) error { + return run(func() error { return ops.delete(service, key) }) + }, + } +} + // migrateKeyring migrates credentials from legacy "bcq" service to "basecamp" service. func migrateKeyring(result *MigrateResult, configDir string) { origins := collectKnownOrigins(configDir) + ops := keyringOps.bounded(legacyKeyringTimeout) for _, origin := range origins { legacyKey := fmt.Sprintf("bcq::%s", origin) newKey := fmt.Sprintf("basecamp::%s", origin) // Read from legacy service - data, err := keyringOps.get(legacyServiceName, legacyKey) + data, err := ops.get(legacyServiceName, legacyKey) + if errors.Is(err, context.DeadlineExceeded) { + result.KeyringErrors = append(result.KeyringErrors, + fmt.Sprintf("failed to read %s: %v", origin, err)) + return + } if err != nil { // No legacy entry for this origin — skip silently continue } // Check if new entry already exists - if _, err := keyringOps.get("basecamp", newKey); err == nil { + _, err = ops.get("basecamp", newKey) + if errors.Is(err, context.DeadlineExceeded) { + result.KeyringErrors = append(result.KeyringErrors, + fmt.Sprintf("failed to read %s: %v", origin, err)) + return + } + if err == nil { // Already migrated — just clean up the legacy key - _ = keyringOps.delete(legacyServiceName, legacyKey) + _ = ops.delete(legacyServiceName, legacyKey) result.KeyringMigrated++ continue } // Write to new service - if err := keyringOps.set("basecamp", newKey, data); err != nil { + if err := ops.set("basecamp", newKey, data); err != nil { result.KeyringErrors = append(result.KeyringErrors, fmt.Sprintf("failed to write %s: %v", origin, err)) continue } // Delete old entry (best-effort) - _ = keyringOps.delete(legacyServiceName, legacyKey) + _ = ops.delete(legacyServiceName, legacyKey) result.KeyringMigrated++ } diff --git a/internal/commands/migrate_test.go b/internal/commands/migrate_test.go index 885334782..680ecc524 100644 --- a/internal/commands/migrate_test.go +++ b/internal/commands/migrate_test.go @@ -1,14 +1,19 @@ package commands import ( + "bytes" "encoding/json" "fmt" "os" "path/filepath" + "sync/atomic" "testing" + "time" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + + "github.com/basecamp/basecamp-cli/internal/config" ) func TestMigrateCache_NoLegacyDir(t *testing.T) { @@ -383,3 +388,89 @@ func TestMigrateKeyring_MultipleOrigins(t *testing.T) { assert.Equal(t, 2, result.KeyringMigrated) assert.Empty(t, result.KeyringErrors) } + +func TestMigrateKeyring_StalledKeyringIsBounded(t *testing.T) { + origTimeout := legacyKeyringTimeout + legacyKeyringTimeout = 40 * time.Millisecond + t.Cleanup(func() { legacyKeyringTimeout = origTimeout }) + + var calls atomic.Int32 + release, finished := make(chan struct{}), make(chan struct{}) + orig := keyringOps + keyringOps = keyringFuncs{ + get: func(string, string) (string, error) { + calls.Add(1) + <-release + defer close(finished) + return `{"token":"secret"}`, nil + }, + set: func(string, string, string) error { t.Error("set after a stalled read"); return nil }, + delete: func(string, string) error { t.Error("delete after a stalled read"); return nil }, + } + t.Cleanup(func() { keyringOps = orig; close(release); <-finished }) + + configDir := t.TempDir() + creds := map[string]any{"https://custom.basecampapi.com": map[string]string{"token": "x"}} + data, _ := json.Marshal(creds) + require.NoError(t, os.WriteFile(filepath.Join(configDir, "credentials.json"), data, 0600)) + + done := make(chan *MigrateResult, 1) + go func() { + result := &MigrateResult{} + migrateKeyring(result, configDir) + done <- result + }() + select { + case result := <-done: + assert.Equal(t, 0, result.KeyringMigrated) + require.Len(t, result.KeyringErrors, 1) + assert.Contains(t, result.KeyringErrors[0], "timed out") + assert.Contains(t, result.KeyringErrors[0], "BASECAMP_NO_KEYRING=1") + case <-time.After(time.Second): + t.Fatal("keyring migration ignored its deadline") + } + assert.EqualValues(t, 1, calls.Load()) +} + +func TestMigrateSkipsKeyringWhenNoKeyring(t *testing.T) { + t.Setenv("BASECAMP_NO_KEYRING", "1") + t.Setenv("XDG_CONFIG_HOME", t.TempDir()) + t.Setenv("XDG_CACHE_HOME", t.TempDir()) + + orig := keyringOps + keyringOps = keyringFuncs{ + get: func(string, string) (string, error) { + t.Error("keyring read with BASECAMP_NO_KEYRING set") + return "", nil + }, + set: func(string, string, string) error { t.Error("keyring write with BASECAMP_NO_KEYRING set"); return nil }, + delete: func(string, string) error { t.Error("keyring delete with BASECAMP_NO_KEYRING set"); return nil }, + } + t.Cleanup(func() { keyringOps = orig }) + + cmd := NewMigrateCmd() + var out bytes.Buffer + cmd.SetOut(&out) + cmd.SetArgs(nil) + require.NoError(t, cmd.Execute()) + assert.Contains(t, out.String(), `"keyring_migrated": 0`) +} + +func TestMigrateMarker_NotWrittenWhenKeyringSkipped(t *testing.T) { + t.Setenv("BASECAMP_NO_KEYRING", "1") + configBase := t.TempDir() + cacheBase := t.TempDir() + t.Setenv("XDG_CONFIG_HOME", configBase) + t.Setenv("XDG_CACHE_HOME", cacheBase) + require.NoError(t, os.MkdirAll(filepath.Join(cacheBase, "bcq"), 0700)) + + cmd := NewMigrateCmd() + var out bytes.Buffer + cmd.SetOut(&out) + cmd.SetArgs(nil) + require.NoError(t, cmd.Execute()) + assert.Contains(t, out.String(), `"cache_moved": true`) + + _, err := os.Stat(filepath.Join(config.GlobalConfigDir(), migratedMarker)) + assert.True(t, os.IsNotExist(err), "marker must not be written while bcq keyring entries may remain") +} diff --git a/nix/package.nix b/nix/package.nix index 82e2a2522..52fa15ce6 100644 --- a/nix/package.nix +++ b/nix/package.nix @@ -8,7 +8,7 @@ buildGoModule.override { go = go_1_26; } (finalAttrs: { src = lib.cleanSource ./..; # To update: set to lib.fakeHash, run `nix build`, use the hash from the error. - vendorHash = "sha256-fbSMybSFUlSHIE8/aqLH/QCu+Q3cvJV/qpBAKz7VAZI="; + vendorHash = "sha256-jSh16aV4PZxztlVEgckzu62517TBPh9EQKvoRDiudrI="; subPackages = [ "cmd/basecamp" ];