From a9b8dcb3c4a8e6f45760a751fdedf64acdc687f1 Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Fri, 28 Aug 2026 13:38:00 -0700 Subject: [PATCH 1/3] auth: warn about the keyring fallback on the first read, not only on login MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The credential store printed its "system keyring unavailable, credentials stored in plaintext" warning only on Save. Every other command merely reads, so a process whose keyring probe failed served whatever an earlier fallback had left in credentials.json — on one machine today, tokens that expired months ago and profiles that no longer existed — with no word about why. The failing probe was invisible until the next login. Store.Load now runs the same once-per-process warning as Save, so the first read after a fallen-back probe says so on stderr, and once credstore is bumped past basecamp/cli#70 the warning and the Load error also name the probe failure itself. Hosts that mean to use file storage set BASECAMP_NO_KEYRING, which skips the probe and never warns. The wrapper's inner store is now the credStore interface rather than the concrete *credstore.Store, so the test can stand in a fallen-back store without failing a real keyring probe. --- internal/auth/keyring.go | 27 +++++++++++++--- internal/auth/keyring_test.go | 61 +++++++++++++++++++++++++++++++++-- 2 files changed, 81 insertions(+), 7 deletions(-) diff --git a/internal/auth/keyring.go b/internal/auth/keyring.go index 8deff4bd0..3a17280a5 100644 --- a/internal/auth/keyring.go +++ b/internal/auth/keyring.go @@ -40,12 +40,24 @@ type Credentials struct { type Store struct { fallbackDir string initOnce sync.Once - inner *credstore.Store + inner credStore warnOnce sync.Once } +// credStore is the slice of credstore.Store this wrapper uses, as an +// interface so tests can stand in a store that fell back to file storage +// without failing a real keyring probe. +type credStore interface { + Load(key string) ([]byte, error) + Save(key string, data []byte) error + Delete(key string) error + MigrateToKeyring() error + UsingKeyring() bool + FallbackWarning() string +} + // newCredStore is replaceable in tests to avoid real keyring access. -var newCredStore = credstore.NewStore +var newCredStore = func(opts credstore.StoreOptions) credStore { return credstore.NewStore(opts) } // sessionIsHeadless reports that no human can answer a keyring unlock // prompt: stdin, stdout, and stderr are all non-terminals AND no GUI @@ -80,7 +92,7 @@ func NewStore(fallbackDir string) *Store { // ensure constructs the underlying store on first use. Callers reach here // from paths that don't hold Manager.mu (e.g. IsAuthenticated), so the // sync.Once provides the synchronization. -func (s *Store) ensure() *credstore.Store { +func (s *Store) ensure() credStore { s.initOnce.Do(func() { opts := credstore.StoreOptions{ ServiceName: "basecamp", @@ -95,7 +107,13 @@ func (s *Store) ensure() *credstore.Store { return s.inner } -// warnFallback prints the keyring fallback warning once, on first credential write. +// warnFallback prints the keyring fallback warning once per process, on the +// first credential read or write. Reads warn too: a fallback read returns +// whatever an earlier fallback left in credentials.json — possibly months +// stale — rather than the credentials the keyring holds, and the probe +// failure behind it would otherwise stay invisible until the next login. +// Hosts that mean to use file storage set BASECAMP_NO_KEYRING, which skips +// the probe and so never warns. func (s *Store) warnFallback() { inner := s.ensure() s.warnOnce.Do(func() { @@ -107,6 +125,7 @@ func (s *Store) warnFallback() { // Load retrieves credentials for the given origin. func (s *Store) Load(origin string) (*Credentials, error) { + s.warnFallback() data, err := s.ensure().Load(origin) if err != nil { return nil, err diff --git a/internal/auth/keyring_test.go b/internal/auth/keyring_test.go index 22b6701c9..3b67f021b 100644 --- a/internal/auth/keyring_test.go +++ b/internal/auth/keyring_test.go @@ -1,6 +1,9 @@ package auth import ( + "io" + "os" + "strings" "testing" "github.com/stretchr/testify/assert" @@ -10,7 +13,7 @@ import ( ) // swapNewCredStore replaces the credstore constructor seam for the test. -func swapNewCredStore(t *testing.T, fn func(credstore.StoreOptions) *credstore.Store) { +func swapNewCredStore(t *testing.T, fn func(credstore.StoreOptions) credStore) { t.Helper() orig := newCredStore newCredStore = fn @@ -28,7 +31,7 @@ func TestNewStoreIsLazy(t *testing.T) { t.Setenv("BASECAMP_NO_KEYRING", "1") // keep the delegated construction off the real keyring calls := 0 - swapNewCredStore(t, func(opts credstore.StoreOptions) *credstore.Store { + swapNewCredStore(t, func(opts credstore.StoreOptions) credStore { calls++ return credstore.NewStore(opts) }) @@ -60,7 +63,7 @@ func ensureOptions(t *testing.T, headless bool) credstore.StoreOptions { t.Setenv("BASECAMP_NO_KEYRING", "1") // keep the delegated construction off the real keyring var got credstore.StoreOptions - swapNewCredStore(t, func(opts credstore.StoreOptions) *credstore.Store { + swapNewCredStore(t, func(opts credstore.StoreOptions) credStore { got = opts return credstore.NewStore(opts) }) @@ -78,3 +81,55 @@ func TestEnsureBoundsProbeOnlyWhenHeadless(t *testing.T) { assert.Equal(t, headlessProbeTimeout, ensureOptions(t, true).ProbeTimeout) assert.Zero(t, ensureOptions(t, false).ProbeTimeout) } + +// fallenBackStore stands in for a credstore.Store whose keyring probe failed +// and which is serving the plaintext file instead. +type fallenBackStore struct{ warning string } + +func (f *fallenBackStore) Load(string) ([]byte, error) { return []byte(`{"access_token":"tok"}`), nil } +func (f *fallenBackStore) Save(string, []byte) error { return nil } +func (f *fallenBackStore) Delete(string) error { return nil } +func (f *fallenBackStore) MigrateToKeyring() error { return nil } +func (f *fallenBackStore) UsingKeyring() bool { return false } +func (f *fallenBackStore) FallbackWarning() string { return f.warning } + +// captureStderr returns what the callback wrote to os.Stderr. +func captureStderr(t *testing.T, fn func()) string { + t.Helper() + r, w, err := os.Pipe() + require.NoError(t, err) + orig := os.Stderr + os.Stderr = w + defer func() { os.Stderr = orig }() + + fn() + + require.NoError(t, w.Close()) + out, err := io.ReadAll(r) + require.NoError(t, err) + return string(out) +} + +// Regression: the fallback warning printed only on Save, so a process that +// merely read credentials — every command but login — could serve a stale +// credentials.json after a failed keyring probe without a word about it. +// The warning must show on the first read too, and still only once. +func TestLoadWarnsOnceWhenKeyringFellBack(t *testing.T) { + warning := "system keyring unavailable (User interaction is not allowed.), credentials stored in plaintext at /tmp/credentials.json" + swapNewCredStore(t, func(credstore.StoreOptions) credStore { return &fallenBackStore{warning: warning} }) + store := NewStore(t.TempDir()) + + reads := captureStderr(t, func() { + for range 2 { + _, err := store.Load("profile:work") + require.NoError(t, err) + } + }) + assert.Equal(t, 1, strings.Count(reads, "warning: "), "the first read warns, the second does not") + assert.Contains(t, reads, "warning: "+warning+"\n") + + write := captureStderr(t, func() { + require.NoError(t, store.Save("profile:work", &Credentials{AccessToken: "tok"})) + }) + assert.Empty(t, write, "a write after the read has already warned stays quiet") +} From 6208afd91b890461134edb983eb616daf142107e Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Fri, 28 Aug 2026 13:41:02 -0700 Subject: [PATCH 2/3] auth: reword the credStore seam comment --- internal/auth/keyring.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/auth/keyring.go b/internal/auth/keyring.go index 3a17280a5..8fb20fcc9 100644 --- a/internal/auth/keyring.go +++ b/internal/auth/keyring.go @@ -45,7 +45,7 @@ type Store struct { } // credStore is the slice of credstore.Store this wrapper uses, as an -// interface so tests can stand in a store that fell back to file storage +// interface so tests can substitute a store that fell back to file storage // without failing a real keyring probe. type credStore interface { Load(key string) ([]byte, error) From 84aaed3e29a9a8b0497285648b4c090eae754e8b Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Fri, 28 Aug 2026 13:57:59 -0700 Subject: [PATCH 3/3] auth: keep a BASECAMP_TOKEN session off the store in SetUserEmail MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `basecamp people me` stores the fetched email through SetUserEmail, the one read path with no BASECAMP_TOKEN guard: IsAuthenticated and AuthorizationEndpoint short-circuit on the env token, then SetUserEmail loads the stored credentials anyway. That was wrong twice over. The email names the env token's user, not whoever the stored credentials belong to, so writing it there mislabeled them. And the load ran the keyring probe — now that a fallback read warns, a CI host with a locked keychain and only BASECAMP_TOKEN warned about plaintext credentials it neither stored nor read. BASECAMP_TOKEN wins, matching AccessToken() and AccountID(): SetUserEmail returns without touching the store. --- internal/auth/auth.go | 10 ++++++++++ internal/auth/auth_test.go | 18 ++++++++++++++++++ 2 files changed, 28 insertions(+) diff --git a/internal/auth/auth.go b/internal/auth/auth.go index 5d5195212..5bd836ed3 100644 --- a/internal/auth/auth.go +++ b/internal/auth/auth.go @@ -1158,7 +1158,17 @@ func (m *Manager) GetUserEmail() string { // SetUserEmail stores the user email for the current credential key // without modifying the stored user ID. +// +// BASECAMP_TOKEN wins — match AccessToken() precedence. The email was +// fetched with the environment token, so it names that token's user, not +// whoever the stored credentials belong to; writing it there would +// mislabel them. Skipping the store also keeps a token session off the +// keyring probe and the fallback warning it can raise. func (m *Manager) SetUserEmail(email string) error { + if os.Getenv("BASECAMP_TOKEN") != "" { + return nil + } + credKey := m.credentialKey() creds, err := m.store.Load(credKey) if err != nil { diff --git a/internal/auth/auth_test.go b/internal/auth/auth_test.go index 923fdf9f2..4019313e3 100644 --- a/internal/auth/auth_test.go +++ b/internal/auth/auth_test.go @@ -16,6 +16,7 @@ import ( "time" "github.com/basecamp/basecamp-sdk/go/pkg/basecamp/oauth" + "github.com/basecamp/cli/credstore" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -260,6 +261,23 @@ func TestSetUserEmail(t *testing.T) { assert.Equal(t, "original-id", loaded.UserID) } +// Regression: `basecamp people me` on BASECAMP_TOKEN stored the fetched +// email through SetUserEmail, the one read path with no env-token guard. +// That both mislabeled any stored credentials with the env token's user and +// ran the keyring probe — so a CI host with a locked keychain now warned +// about plaintext credentials it neither stored nor read. +func TestSetUserEmailSkipsStoreOnEnvToken(t *testing.T) { + t.Setenv("BASECAMP_TOKEN", "bc_at_from_environment") + swapNewCredStore(t, func(credstore.StoreOptions) credStore { + t.Error("a token session must not construct the credential store") + return &fallenBackStore{} + }) + manager := NewManager(&config.Config{BaseURL: "https://3.basecampapi.com"}, http.DefaultClient) + manager.store = NewStore(t.TempDir()) + + require.NoError(t, manager.SetUserEmail("someone@example.com")) +} + func TestSetUserIdentity(t *testing.T) { tmpDir := t.TempDir()