diff --git a/SECURITY.md b/SECURITY.md index 7e51d05ec..8acfada0d 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. Don't copy that file between machines or containers. A login refreshes by replacing its token, and Basecamp signs out every copy once two of them diff --git a/go.mod b/go.mod index 0fc9c7173..6ad7e9f21 100644 --- a/go.mod +++ b/go.mod @@ -9,7 +9,7 @@ require ( charm.land/bubbletea/v2 v2.1.0 charm.land/lipgloss/v2 v2.0.6 github.com/basecamp/basecamp-sdk/go v0.27.1-0.20261022230746-63115e7978c9 - github.com/basecamp/cli v0.2.2-0.20260828230226-767413fc712d + github.com/basecamp/cli v0.2.2-0.20261023171439-c9bce8206df9 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 aa0908eee..bf81fac46 100644 --- a/go.sum +++ b/go.sum @@ -91,8 +91,8 @@ github.com/basecamp/actioncable-go v1.2.0 h1:t0MxlfMTcmaGXZ5ztgKRs7ZiY1MwXtBRwSS github.com/basecamp/actioncable-go v1.2.0/go.mod h1:ezaV5z1GXQAsqyejqTs6wCFl2D8Wj+COLQkHc/kwoRs= github.com/basecamp/basecamp-sdk/go v0.27.1-0.20261022230746-63115e7978c9 h1:ZcW7c/cnQTgFkNHMcnkxPqHIko2d0v2MrOgyykBrOj0= github.com/basecamp/basecamp-sdk/go v0.27.1-0.20261022230746-63115e7978c9/go.mod h1:mUm/MQGMyg1E1cddyxdG7tQHiq0NsYee2zUeAUT5VjU= -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.20261023171439-c9bce8206df9 h1:3NfGs+zx23Tp6cf+97+UewrJHlzZMLzmvEXOmOJyQAk= +github.com/basecamp/cli v0.2.2-0.20261023171439-c9bce8206df9/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 efd662c7d..463a76c77 100644 --- a/internal/auth/keyring.go +++ b/internal/auth/keyring.go @@ -6,6 +6,7 @@ import ( "errors" "fmt" "os" + "runtime" "strings" "sync" "time" @@ -123,9 +124,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 @@ -144,7 +146,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) @@ -196,7 +201,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 { @@ -249,13 +254,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 @@ -273,8 +278,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 f31a7a3bd..5890d78a2 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 ecab70202..e2f85e288 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 462da74ca..2c9cac0aa 100644 --- a/internal/commands/auth_status_agent_test.go +++ b/internal/commands/auth_status_agent_test.go @@ -307,7 +307,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") @@ -321,10 +321,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 d64922332..2976a8ebd 100644 --- a/internal/commands/doctor.go +++ b/internal/commands/doctor.go @@ -6,6 +6,7 @@ import ( "context" "debug/pe" "encoding/json" + "errors" "fmt" "io" "net/http" @@ -22,6 +23,7 @@ import ( "github.com/basecamp/basecamp-sdk/go/pkg/basecamp" "github.com/basecamp/basecamp-cli/internal/appctx" + "github.com/basecamp/basecamp-cli/internal/auth" "github.com/basecamp/basecamp-cli/internal/config" "github.com/basecamp/basecamp-cli/internal/harness" "github.com/basecamp/basecamp-cli/internal/output" @@ -88,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 @@ -150,7 +157,7 @@ func runDoctorChecks(ctx context.Context, app *appctx.App, verbose bool) []Check checks = append(checks, checkConfigFiles(app, verbose)...) // 6. Credentials check - credCheck := checkCredentials(app, verbose) + credCheck, unreadable := checkStoredCredentials(ctx, app, verbose) checks = append(checks, credCheck) // 7. Authentication check (only if credentials exist) @@ -159,6 +166,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", @@ -208,7 +221,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) } @@ -695,7 +708,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", } @@ -704,26 +725,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" @@ -749,11 +784,17 @@ func checkCredentials(app *appctx.App, verbose bool) Check { check.Message = credsPath } } - return check + return check, false } // checkAuthentication checks token validity. func checkAuthentication(ctx context.Context, app *appctx.App, verbose bool) Check { + return checkAuthenticationWith(ctx, app, verbose, app.Auth.GetStore().LoadContext, app.Auth.Refresh) +} + +func checkAuthenticationWith(ctx context.Context, app *appctx.App, verbose bool, + load func(context.Context, string) (*auth.Credentials, error), refresh func(context.Context) error, +) Check { check := Check{ Name: "Authentication", } @@ -767,12 +808,15 @@ func checkAuthentication(ctx context.Context, app *appctx.App, verbose bool) Che // Load credentials to check expiration credKey := app.Auth.CredentialKey() - store := app.Auth.GetStore() - creds, err := store.Load(credKey) + creds, err := load(ctx, credKey) if err != nil { + problem := output.AsError(err) check.Status = "fail" - check.Message = "Cannot load credentials" - check.Hint = app.Auth.LoginHint() + check.Message = "Could not read stored credentials: " + problem.Message + check.Hint = problem.Hint + if errors.Is(err, auth.ErrNoCredential) || errors.Is(err, auth.ErrInvalidCredentials) { + check.Hint = app.Auth.LoginHint() + } return check } @@ -782,14 +826,16 @@ func checkAuthentication(ctx context.Context, app *appctx.App, verbose bool) Che if expiresIn < 0 { // Token expired - try to refresh - if err := app.Auth.Refresh(ctx); err != nil { + if err := refresh(ctx); err != nil { + problem := output.AsError(err) check.Status = "fail" - check.Message = "Token expired and refresh failed" - // The failure's own remedy, not a blanket login: for an - // agent credential `basecamp auth login` signs a PERSON - // in, and doctor is read by people (and agents) looking - // for exactly the command to run. - check.Hint = remedyFor(app, err) + check.Message = "Token expired and refresh failed: " + problem.Message + check.Hint = problem.Hint + // Only a refused credential calls for login. A storage or + // network failure must retain its own cause and remedy. + if problem.Code == output.CodeAuth { + check.Hint = remedyFor(app, err) + } return check } check.Status = "pass" @@ -1120,6 +1166,11 @@ func buildDoctorBreadcrumbs(checks []Check, login string) []output.Breadcrumb { switch c.Name { case "Credentials", "Authentication": + // Both checks must name login as the remedy before offering it + // as a next step. Do not invent it for storage or network errors. + if !strings.Contains(c.Hint, login) { + continue + } breadcrumbs = append(breadcrumbs, output.Breadcrumb{ Action: "login", Cmd: login, @@ -1304,7 +1355,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 @@ -1348,12 +1399,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::*)") } } @@ -1368,3 +1415,40 @@ func checkLegacyInstall() *Check { Hint: "Run: basecamp migrate", } } + +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 + } +} diff --git a/internal/commands/doctor_test.go b/internal/commands/doctor_test.go index 11daa5c06..8cf201512 100644 --- a/internal/commands/doctor_test.go +++ b/internal/commands/doctor_test.go @@ -5,12 +5,16 @@ import ( "context" "debug/pe" "encoding/binary" + "errors" + "fmt" "net/http" "os" "path/filepath" "runtime" "strings" + "sync/atomic" "testing" + "time" "github.com/spf13/cobra" "github.com/stretchr/testify/assert" @@ -298,7 +302,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 +317,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 +383,150 @@ 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 TestDoctorAuthenticationPreservesLaterStoreTimeouts(t *testing.T) { + for _, failure := range []string{"authentication read", "refresh read", "refresh write"} { + t.Run(failure, func(t *testing.T) { + app, _ := setupDoctorTestApp(t, "12345") + t.Setenv("BASECAMP_TOKEN", "") + creds := &auth.Credentials{ + AccessToken: "test-token", RefreshToken: "test-refresh", + OAuthType: "bc5", ExpiresAt: time.Now().Add(time.Hour).Unix(), + } + if failure != "authentication read" { + creds.ExpiresAt = time.Now().Add(-time.Hour).Unix() + } + require.NoError(t, app.Auth.GetStore().Save(app.Auth.CredentialKey(), creds)) + initial, unreadable := checkStoredCredentials(context.Background(), app, false) + require.False(t, unreadable) + require.Equal(t, "pass", initial.Status) + + operation := "read" + if failure == "refresh write" { + operation = "write" + } + timeout := fmt.Errorf("keyring %s timed out: %w; set BASECAMP_NO_KEYRING=1", operation, context.DeadlineExceeded) + loads, refreshes := 0, 0 + load := func(context.Context, string) (*auth.Credentials, error) { + loads++ + if failure == "authentication read" || (failure == "refresh read" && loads == 2) { + return nil, timeout + } + return creds, nil + } + refresh := func(ctx context.Context) error { + refreshes++ + if _, err := load(ctx, app.Auth.CredentialKey()); err != nil { + return err + } + return timeout // The refresh read succeeded, but saving its result timed out. + } + check := checkAuthenticationWith(context.Background(), app, false, load, refresh) + assert.Equal(t, "fail", check.Status) + assert.Contains(t, check.Message, "keyring "+operation+" timed out") + assert.Contains(t, check.Message, "BASECAMP_NO_KEYRING=1") + assert.NotContains(t, check.Hint, app.Auth.LoginCommand()) + assert.Empty(t, buildDoctorBreadcrumbs([]Check{initial, check}, app.Auth.LoginCommand())) + if failure == "authentication read" { + assert.Equal(t, 1, loads) + assert.Zero(t, refreshes) + } else { + assert.Equal(t, 2, loads) + assert.Equal(t, 1, refreshes) + } + }) + } +} + +func TestDoctorAuthenticationKeepsGenuineLoginRemedies(t *testing.T) { + app, _ := setupDoctorTestApp(t, "12345") + t.Setenv("BASECAMP_TOKEN", "") + t.Run("missing credential", func(t *testing.T) { + check := checkAuthentication(context.Background(), app, false) + assert.Equal(t, "fail", check.Status) + assert.Equal(t, app.Auth.LoginHint(), check.Hint) + require.Len(t, buildDoctorBreadcrumbs([]Check{check}, app.Auth.LoginCommand()), 1) + }) + t.Run("expired without refresh token", func(t *testing.T) { + require.NoError(t, app.Auth.GetStore().Save(app.Auth.CredentialKey(), &auth.Credentials{ + AccessToken: "test-token", OAuthType: "bc5", ExpiresAt: time.Now().Add(-time.Hour).Unix(), + })) + check := checkAuthentication(context.Background(), app, false) + assert.Equal(t, "fail", check.Status) + assert.Contains(t, check.Message, "No refresh token") + assert.Equal(t, app.Auth.LoginHint(), check.Hint) + require.Len(t, buildDoctorBreadcrumbs([]Check{check}, app.Auth.LoginCommand()), 1) + }) +} + +func TestDoctorAuthenticationBreadcrumbRequiresLoginRemedy(t *testing.T) { + for _, status := range []string{"fail", "warn"} { + check := Check{Name: "Authentication", Status: status, Hint: "Try again later"} + assert.Empty(t, buildDoctorBreadcrumbs([]Check{check}, "basecamp auth login")) + } + check := Check{Name: "Authentication", Status: "fail", Hint: "Run: basecamp auth login"} + crumbs := buildDoctorBreadcrumbs([]Check{check}, "basecamp auth login") + require.Len(t, crumbs, 1) + assert.Equal(t, "basecamp auth login", crumbs[0].Cmd) +} + +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 +614,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 +631,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 +647,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 +658,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 +677,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 +1004,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 +1019,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 +1036,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 +1076,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..3b767aa64 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,118 @@ 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 { - // Already migrated — just clean up the legacy key - _ = keyringOps.delete(legacyServiceName, legacyKey) + _, 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 copied — cleanup must finish before marking migration complete. + if err := ops.delete(legacyServiceName, legacyKey); errors.Is(err, context.DeadlineExceeded) { + result.KeyringErrors = append(result.KeyringErrors, + fmt.Sprintf("failed to delete legacy credential for %s: %v", origin, err)) + return + } 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) + // Ordinary deletion errors remain best-effort, but a timed-out + // cleanup must remain visible and withhold the completion marker. + if err := ops.delete(legacyServiceName, legacyKey); errors.Is(err, context.DeadlineExceeded) { + result.KeyringErrors = append(result.KeyringErrors, + fmt.Sprintf("failed to delete legacy credential for %s: %v", origin, err)) + return + } result.KeyringMigrated++ } diff --git a/internal/commands/migrate_test.go b/internal/commands/migrate_test.go index 885334782..71da084e6 100644 --- a/internal/commands/migrate_test.go +++ b/internal/commands/migrate_test.go @@ -1,14 +1,20 @@ package commands import ( + "bytes" "encoding/json" "fmt" "os" "path/filepath" + "strings" + "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 +389,161 @@ 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 TestMigrateDeletionTimeoutDoesNotMarkCompletion(t *testing.T) { + for _, alreadyCopied := range []bool{false, true} { + t.Run(fmt.Sprintf("destination_exists=%t", alreadyCopied), func(t *testing.T) { + t.Setenv("HOME", t.TempDir()) + t.Setenv("BASECAMP_NO_KEYRING", "") + t.Setenv("XDG_CONFIG_HOME", t.TempDir()) + cacheBase := t.TempDir() + t.Setenv("XDG_CACHE_HOME", cacheBase) + require.NoError(t, os.MkdirAll(filepath.Join(cacheBase, "bcq"), 0700)) + + fk := newFakeKeyring() + fk.store[fk.key("bcq", "bcq::https://3.basecampapi.com")] = "legacy-value" + if alreadyCopied { + fk.store[fk.key("basecamp", "basecamp::https://3.basecampapi.com")] = "current-value" + } + fk.install(t) + originalTimeout := legacyKeyringTimeout + legacyKeyringTimeout = 40 * time.Millisecond + t.Cleanup(func() { legacyKeyringTimeout = originalTimeout }) + release, finished := make(chan struct{}), make(chan struct{}) + keyringOps.delete = func(string, string) error { + defer close(finished) + <-release + return nil + } + t.Cleanup(func() { close(release); <-finished }) + + cmd := NewMigrateCmd() + var out bytes.Buffer + cmd.SetOut(&out) + cmd.SetArgs(nil) + require.NoError(t, cmd.Execute()) + var result MigrateResult + require.NoError(t, json.Unmarshal(out.Bytes(), &result)) + assert.True(t, result.CacheMoved, "even a successful cache move must not hide incomplete keyring cleanup") + assert.Zero(t, result.KeyringMigrated) + assert.Len(t, result.KeyringErrors, 1) + failure := strings.Join(result.KeyringErrors, "; ") + assert.Contains(t, failure, "delete") + assert.Contains(t, failure, "timed out") + assert.Contains(t, failure, "BASECAMP_NO_KEYRING=1") + _, err := os.Stat(filepath.Join(config.GlobalConfigDir(), migratedMarker)) + assert.True(t, os.IsNotExist(err), "migration must stay discoverable after a deletion timeout") + value, err := fk.get("basecamp", "basecamp::https://3.basecampapi.com") + require.NoError(t, err) + if alreadyCopied { + assert.Equal(t, "current-value", value) + } else { + assert.Equal(t, "legacy-value", value) + } + }) + } +} + +func TestMigrateOrdinaryDeletionErrorsRemainBestEffort(t *testing.T) { + for _, alreadyCopied := range []bool{false, true} { + t.Run(fmt.Sprintf("destination_exists=%t", alreadyCopied), func(t *testing.T) { + fk := newFakeKeyring() + fk.store[fk.key("bcq", "bcq::https://3.basecampapi.com")] = "legacy-value" + if alreadyCopied { + fk.store[fk.key("basecamp", "basecamp::https://3.basecampapi.com")] = "current-value" + } + fk.install(t) + keyringOps.delete = func(string, string) error { return fmt.Errorf("delete refused") } + result := &MigrateResult{} + migrateKeyring(result, t.TempDir()) + assert.Equal(t, 1, result.KeyringMigrated) + assert.Empty(t, result.KeyringErrors) + }) + } +} + +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 5473eaec8..650dca96b 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-hun/i3MPdFGgzl0qYgal3ur2UQVCS+qnnJdFBreVDDY="; + vendorHash = "sha256-TqjNLGbA3/mruCH8TA4HaGHYLnsXLK66fUuXp4ye9eQ="; subPackages = [ "cmd/basecamp" ];