From 7703e50a7f593c2d61dec2c07005918ccbdbe140 Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Sat, 12 Sep 2026 18:21:31 -0700 Subject: [PATCH 1/8] Treat a rejected refresh token as an auth failure and forget it A refresh the server refuses with invalid_grant means the refresh token is expired, revoked, or reused. The CLI surfaced it as api_error (exit 7) carrying the raw "token error: invalid_grant" string, and left the dead credential in the store, so every later command re-tried the same refresh. Basecamp's abuse tracker bans the client and address after a handful of invalid_grant failures, which turned one expired session into a lockout. Now the refusal is auth_required (exit 3) with the login to run, and the credential is deleted on the spot. Only invalid_grant deletes: invalid_request, network faults, and 5xx keep the credential and their existing classes. Detection matches the SDK's untyped error message; the coupling is named beside the constant. Every auth_required error also carries a remedy now. A 401 from the API arrives without a hint, so output.AsError and convertSDKError default one; the auth errors the credential manager raises name the active profile, since a bare login would store the new credential under the base URL instead. A regression test pins that a refresh preserves user_id, user_email, scope, and resource across the token rotation. --- internal/auth/auth.go | 81 ++++++++++++++--- internal/auth/auth_test.go | 135 +++++++++++++++++++++++++++++ internal/auth/device_test.go | 5 +- internal/commands/projects.go | 4 +- internal/commands/projects_test.go | 31 +++++++ internal/output/errors.go | 21 ++++- 6 files changed, 260 insertions(+), 17 deletions(-) diff --git a/internal/auth/auth.go b/internal/auth/auth.go index 5c2f0fe2e..2e2c27e39 100644 --- a/internal/auth/auth.go +++ b/internal/auth/auth.go @@ -128,6 +128,28 @@ func (m *Manager) credentialKey() string { return config.NormalizeBaseURL(m.cfg.BaseURL) } +// LoginCommand is the command that re-establishes the active credential: +// addressed to the active profile when there is one, since a bare login +// would store the new credential under the base URL instead. +func (m *Manager) LoginCommand() string { + if m.cfg.ActiveProfile != "" { + return "basecamp auth login -P " + m.cfg.ActiveProfile + } + return "basecamp auth login" +} + +// LoginHint is LoginCommand as an error hint. +func (m *Manager) LoginHint() string { + return "Run: " + m.LoginCommand() +} + +// errAuth is an auth_required error whose remedy names the active profile. +func (m *Manager) errAuth(msg string) *output.Error { + e := output.ErrAuth(msg) + e.Hint = m.LoginHint() + return e +} + // AccessToken returns a valid access token, refreshing if needed. // If BASECAMP_TOKEN env var is set, it's used directly without OAuth. func (m *Manager) AccessToken(ctx context.Context) (string, error) { @@ -142,7 +164,7 @@ func (m *Manager) AccessToken(ctx context.Context) (string, error) { credKey := m.credentialKey() creds, err := m.store.Load(credKey) if err != nil { - return "", output.ErrAuth(fmt.Sprintf("Not authenticated for %s: %v", credKey, err)) + return "", m.errAuth(fmt.Sprintf("Not authenticated for %s: %v", credKey, err)) } // Check if token is expired (with 5 minute buffer). @@ -155,12 +177,12 @@ func (m *Manager) AccessToken(ctx context.Context) (string, error) { // Reload refreshed credentials creds, err = m.store.Load(credKey) if err != nil { - return "", output.ErrAuth(fmt.Sprintf("Failed to load refreshed credentials for %s: %v", credKey, err)) + return "", m.errAuth(fmt.Sprintf("Failed to load refreshed credentials for %s: %v", credKey, err)) } } if creds.AccessToken == "" { - return "", output.ErrAuth(fmt.Sprintf("Stored credentials for %s have empty access token", credKey)) + return "", m.errAuth(fmt.Sprintf("Stored credentials for %s have empty access token", credKey)) } return creds.AccessToken, nil @@ -176,7 +198,7 @@ func (m *Manager) StoredAccessToken(ctx context.Context) (string, error) { credKey := m.credentialKey() creds, err := m.store.Load(credKey) if err != nil { - return "", output.ErrAuth(fmt.Sprintf("No stored credentials for %s: %v", credKey, err)) + return "", m.errAuth(fmt.Sprintf("No stored credentials for %s: %v", credKey, err)) } // Check if token is expired (with the refresh-window buffer) @@ -188,12 +210,12 @@ func (m *Manager) StoredAccessToken(ctx context.Context) (string, error) { // Reload refreshed credentials creds, err = m.store.Load(credKey) if err != nil { - return "", output.ErrAuth(fmt.Sprintf("Failed to load refreshed credentials for %s: %v", credKey, err)) + return "", m.errAuth(fmt.Sprintf("Failed to load refreshed credentials for %s: %v", credKey, err)) } } if creds.AccessToken == "" { - return "", output.ErrAuth(fmt.Sprintf("Stored credentials for %s have empty access token", credKey)) + return "", m.errAuth(fmt.Sprintf("Stored credentials for %s have empty access token", credKey)) } return creds.AccessToken, nil @@ -223,15 +245,37 @@ func (m *Manager) Refresh(ctx context.Context) error { credKey := m.credentialKey() creds, err := m.store.Load(credKey) if err != nil { - return output.ErrAuth(fmt.Sprintf("Not authenticated for %s: %v", credKey, err)) + return m.errAuth(fmt.Sprintf("Not authenticated for %s: %v", credKey, err)) } return m.refreshLocked(ctx, credKey, creds) } +// invalidGrantPrefix is how the SDK's token exchanger renders an RFC 6749 +// token-endpoint error: it returns the response as an untyped error, so the +// OAuth error code is recoverable only from the message. Coupled to +// basecamp-sdk oauth.Exchanger; a re-pin that types the error can replace +// the string match. +const invalidGrantPrefix = "token error: invalid_grant" + +// invalidGrant reports whether a refresh was refused with invalid_grant — +// the refresh token is expired, revoked, or reused — and returns the +// server's error_description when it sent one. +func invalidGrant(err error) (string, bool) { + rest, ok := strings.CutPrefix(err.Error(), invalidGrantPrefix) + switch { + case !ok: + return "", false + case rest == "": + return "", true + default: + return strings.CutPrefix(rest, " - ") + } +} + func (m *Manager) refreshLocked(ctx context.Context, origin string, creds *Credentials) error { if creds.RefreshToken == "" { - return output.ErrAuth("No refresh token available") + return m.errAuth("No refresh token available") } // Migrate old credentials missing OAuthType @@ -242,7 +286,7 @@ func (m *Manager) refreshLocked(ctx context.Context, origin string, creds *Crede // Migrate old credentials missing TokenEndpoint if creds.TokenEndpoint == "" { if creds.OAuthType == "bc3" || creds.OAuthType == oauthTypeBC5 { - return output.ErrAuth("Stored credentials missing token endpoint — please re-authenticate: basecamp auth login") + return m.errAuth("Stored credentials are missing their token endpoint and cannot be refreshed") } lpURL, lpErr := m.launchpadURL() if lpErr != nil { @@ -268,7 +312,7 @@ func (m *Manager) refreshLocked(ctx context.Context, origin string, creds *Crede case "bc3": // DCR-era development flow, removed. Its per-install dynamic clients // can't be resolved anymore, so the refresh token is unusable. - return output.ErrAuth("Stored credentials are from a removed development flow — please re-authenticate: basecamp auth login") + return m.errAuth("Stored credentials are from a removed development flow and cannot be refreshed") case oauthTypeBC5: // Pre-registered public client: no secret. clientID = bc5ClientID @@ -311,7 +355,22 @@ func (m *Manager) refreshLocked(ctx context.Context, origin string, creds *Crede token, err := exchanger.Refresh(ctx, req) if err != nil { - return wrapOAuthError("token refresh failed", err) + desc, dead := invalidGrant(err) + if !dead { + return wrapOAuthError("token refresh failed", err) + } + // The grant is gone for good, so the credential is forgotten now + // rather than re-tried by every later command: Basecamp's abuse + // tracker bans the client and address after a handful of + // invalid_grant failures, which would turn one expired session + // into a lockout. The delete's own outcome cannot change the + // answer — the session is over either way. + _ = m.store.Delete(origin) + msg := "Your session has expired or was revoked" + if desc = strings.TrimSpace(richtext.SanitizeSingleLine(desc)); desc != "" { + msg += " (" + desc + ")" + } + return m.errAuth(msg) } creds.AccessToken = token.AccessToken diff --git a/internal/auth/auth_test.go b/internal/auth/auth_test.go index 10c842123..1cc224523 100644 --- a/internal/auth/auth_test.go +++ b/internal/auth/auth_test.go @@ -1884,3 +1884,138 @@ func TestLoginLaunchpadVerifyRunsBeforeStore(t *testing.T) { _, loadErr := m.store.Load(credKey) assert.Error(t, loadErr, "a rejected token is never stored") } + +// refreshRefusedBy is a token endpoint that answers every refresh with the +// given RFC 6749 error body, and a Manager whose active profile's credential +// refreshes against it. +func refreshRefusedBy(t *testing.T, status int, body string) (*Manager, string) { + t.Helper() + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(status) + fmt.Fprint(w, body) + })) + t.Cleanup(srv.Close) + + cfg := config.Default() + cfg.ActiveProfile = "work" + m := &Manager{cfg: cfg, httpClient: srv.Client(), store: newTestStore(t, t.TempDir())} + key := m.credentialKey() + require.NoError(t, m.store.Save(key, &Credentials{ + AccessToken: "old-tok", + RefreshToken: "old-ref", + OAuthType: "bc5", + TokenEndpoint: srv.URL + "/oauth/tokens", + Scope: "full", + ExpiresAt: time.Now().Add(-time.Hour).Unix(), + })) + return m, key +} + +// TestRefresh_InvalidGrantForgetsTheCredential: a refresh token the server +// no longer honors is an auth failure with the login to run, and the dead +// credential is deleted so later commands do not keep re-trying it. +func TestRefresh_InvalidGrantForgetsTheCredential(t *testing.T) { + m, key := refreshRefusedBy(t, http.StatusBadRequest, + `{"error":"invalid_grant","error_description":"The refresh token was revoked\u001b[31m"}`) + + err := m.Refresh(context.Background()) + require.Error(t, err) + + var cliErr *output.Error + require.ErrorAs(t, err, &cliErr) + assert.Equal(t, output.CodeAuth, cliErr.Code) + assert.True(t, strings.HasPrefix(cliErr.Message, "Your session has expired or was revoked (The refresh token was revoked"), cliErr.Message) + assert.NotContains(t, cliErr.Message, "\x1b", "the server's description is sanitized for the terminal") + assert.Equal(t, "Run: basecamp auth login -P work", cliErr.Hint) + + _, loadErr := m.store.Load(key) + assert.Error(t, loadErr, "the dead credential must be forgotten") + assert.False(t, m.IsAuthenticated()) +} + +// TestRefresh_InvalidGrantWithoutDescription: the server may send the bare +// error code; that is still the session-over answer. +func TestRefresh_InvalidGrantWithoutDescription(t *testing.T) { + m, key := refreshRefusedBy(t, http.StatusBadRequest, `{"error":"invalid_grant"}`) + + err := m.Refresh(context.Background()) + var cliErr *output.Error + require.ErrorAs(t, err, &cliErr) + assert.Equal(t, output.CodeAuth, cliErr.Code) + assert.Equal(t, "Your session has expired or was revoked", cliErr.Message) + _, loadErr := m.store.Load(key) + assert.Error(t, loadErr) +} + +// TestRefresh_InvalidRequestKeepsTheCredential: any other refusal keeps its +// existing class and leaves the credential in place — a malformed request +// or a server fault says nothing about the grant. +func TestRefresh_InvalidRequestKeepsTheCredential(t *testing.T) { + m, key := refreshRefusedBy(t, http.StatusBadRequest, + `{"error":"invalid_request","error_description":"resource is required"}`) + + err := m.Refresh(context.Background()) + require.Error(t, err) + + var cliErr *output.Error + require.ErrorAs(t, err, &cliErr) + assert.Equal(t, output.CodeAPI, cliErr.Code) + assert.True(t, strings.HasPrefix(cliErr.Message, "token refresh failed: "), cliErr.Message) + + creds, loadErr := m.store.Load(key) + require.NoError(t, loadErr) + assert.Equal(t, "old-ref", creds.RefreshToken) +} + +// TestAccessToken_InvalidGrantHintsTheProfile: the automatic refresh on an +// ordinary command takes the same path as `auth refresh`. +func TestAccessToken_InvalidGrantHintsTheProfile(t *testing.T) { + m, key := refreshRefusedBy(t, http.StatusBadRequest, `{"error":"invalid_grant"}`) + t.Setenv("BASECAMP_TOKEN", "") + + _, err := m.AccessToken(context.Background()) + var cliErr *output.Error + require.ErrorAs(t, err, &cliErr) + assert.Equal(t, output.CodeAuth, cliErr.Code) + assert.Equal(t, "Run: basecamp auth login -P work", cliErr.Hint) + _, loadErr := m.store.Load(key) + assert.Error(t, loadErr) +} + +// TestRefresh_PreservesIdentityAndBinding: a rotation replaces the tokens +// and nothing else — the stored user, scope, and account binding survive a +// token response that does not repeat them. +func TestRefresh_PreservesIdentityAndBinding(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + fmt.Fprint(w, `{"access_token":"new-tok","refresh_token":"new-ref","expires_in":3600}`) + })) + defer srv.Close() + + m := &Manager{cfg: config.Default(), httpClient: srv.Client(), store: newTestStore(t, t.TempDir())} + key := m.credentialKey() + require.NoError(t, m.store.Save(key, &Credentials{ + AccessToken: "old-tok", + RefreshToken: "old-ref", + OAuthType: "bc5", + TokenEndpoint: srv.URL + "/oauth/tokens", + Scope: "full", + UserID: "51177542", + UserEmail: "bot@example.com", + Resource: "urn:bc:account:999", + ExpiresAt: time.Now().Add(-time.Hour).Unix(), + })) + + require.NoError(t, m.Refresh(context.Background())) + + creds, err := m.store.Load(key) + require.NoError(t, err) + assert.Equal(t, "new-tok", creds.AccessToken) + assert.Equal(t, "new-ref", creds.RefreshToken) + assert.Equal(t, "51177542", creds.UserID) + assert.Equal(t, "bot@example.com", creds.UserEmail) + assert.Equal(t, "full", creds.Scope) + assert.Equal(t, "urn:bc:account:999", creds.Resource) + assert.Equal(t, "bc5", creds.OAuthType) +} diff --git a/internal/auth/device_test.go b/internal/auth/device_test.go index 0fbea4844..f48b81ae0 100644 --- a/internal/auth/device_test.go +++ b/internal/auth/device_test.go @@ -800,7 +800,10 @@ func TestRefreshLocked_LegacyBC3RequiresReauth(t *testing.T) { err := m.refreshLocked(context.Background(), "test", creds) require.Error(t, err) - assert.Contains(t, err.Error(), "re-authenticate") + var cliErr *output.Error + require.ErrorAs(t, err, &cliErr) + assert.Equal(t, output.CodeAuth, cliErr.Code) + assert.Equal(t, "Run: basecamp auth login", cliErr.Hint) assert.False(t, transport.attempted.Load(), "legacy bc3 refresh must fail without any network request") } diff --git a/internal/commands/projects.go b/internal/commands/projects.go index 70f4748bd..4cf270ddc 100644 --- a/internal/commands/projects.go +++ b/internal/commands/projects.go @@ -467,14 +467,14 @@ func convertSDKError(err error) error { // Handle structured SDK errors var sdkErr *basecamp.Error if errors.As(err, &sdkErr) { - return &output.Error{ + return output.WithAuthHint(&output.Error{ Code: sdkErr.Code, Message: sdkErr.Message, Hint: sdkErr.Hint, HTTPStatus: sdkErr.HTTPStatus, Retryable: sdkErr.Retryable, Cause: sdkErr, - } + }) } return err } diff --git a/internal/commands/projects_test.go b/internal/commands/projects_test.go index d3a812f1a..9274a3e11 100644 --- a/internal/commands/projects_test.go +++ b/internal/commands/projects_test.go @@ -217,3 +217,34 @@ func TestConvertSDKErrorCarriesTheGateMessageAndHint(t *testing.T) { assert.Equal(t, "Re-run, or lower parallelism.", outErr.Hint) assert.True(t, outErr.Retryable) } + +// unauthorizedTransport answers every request with a bare 401. +type unauthorizedTransport struct{} + +func (unauthorizedTransport) RoundTrip(*http.Request) (*http.Response, error) { + return jsonResponse(http.StatusUnauthorized, `{"error":"authentication required"}`, http.Header{"Content-Type": []string{"application/json"}}), nil +} + +// TestProjectsList401CarriesTheLoginHint: a 401 from the API arrives without +// a remedy, and the rendered error must still say what to run. +func TestProjectsList401CarriesTheLoginHint(t *testing.T) { + app, out := setupProjectsMockApp(t, unauthorizedTransport{}) + + err := executeCommand(NewProjectsCmd(), app, "list") + require.Error(t, err) + + converted := output.AsError(err) + assert.Equal(t, output.CodeAuth, converted.Code) + assert.Equal(t, "Run: basecamp auth login", converted.Hint) + + require.NoError(t, app.Err(err)) + var envelope struct { + OK bool `json:"ok"` + Code string `json:"code"` + Hint string `json:"hint"` + } + require.NoError(t, json.Unmarshal(out.Bytes(), &envelope), out.String()) + assert.False(t, envelope.OK) + assert.Equal(t, "auth_required", envelope.Code) + assert.Equal(t, "Run: basecamp auth login", envelope.Hint) +} diff --git a/internal/output/errors.go b/internal/output/errors.go index d9ec24c57..ffee6a8cb 100644 --- a/internal/output/errors.go +++ b/internal/output/errors.go @@ -46,16 +46,31 @@ func AsError(err error) *Error { if message == "" { message = sdkErr.Message } - return &Error{ + return WithAuthHint(&Error{ Code: sdkErr.Code, Message: message, Hint: sdkErr.Hint, HTTPStatus: sdkErr.HTTPStatus, Retryable: sdkErr.Retryable, Cause: sdkErr, - } + }) + } + return WithAuthHint(clioutput.AsError(err)) +} + +// WithAuthHint gives an auth_required error that names no remedy the login +// command. A 401 from the API and the SDK's own auth errors arrive without +// one, and "authentication required" alone leaves the reader to guess what +// to run. Errors that already carry a hint, or are not auth errors, pass +// through untouched; a hinted copy is returned so the caller's error is not +// rewritten under it. +func WithAuthHint(e *Error) *Error { + if e.Code != CodeAuth || e.Hint != "" { + return e } - return clioutput.AsError(err) + hinted := *e + hinted.Hint = "Run: basecamp auth login" + return &hinted } // AsGateError converts a resilience gate rejection, which arrives through From 9761ef7bd8317b20496b38e58df2154c308690b5 Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Sat, 12 Sep 2026 18:40:31 -0700 Subject: [PATCH 2/8] Forget a refused grant only when it is provably dead, and fit the remedy MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review of the invalid_grant handling raised two ways an unconditional delete could discard a live credential: a Launchpad refresh sends whichever client the environment names, and the server answers invalid_grant for a token issued to another client too; and two processes can refresh at once, so the one refused for reusing the old token would delete the rotated credential the other had just saved. Only a BC5 credential is forgotten now — its client is the fixed public one — and only while the store still holds the refresh token that was refused. The generic "Run: basecamp auth login" hint was not actionable under an active profile (the login would store the credential under the base URL) or under BASECAMP_TOKEN (no login changes what requests send). The app boundary now replaces the generic hint with the profile-aware login command, or names the environment token. For that to survive rendering, output.AsError prefers an *Error already in the chain over the SDK error it wraps, which is also what every command's hand-built error expects. --- internal/appctx/context.go | 27 +++++++++++- internal/auth/auth.go | 25 +++++++++-- internal/auth/auth_test.go | 67 ++++++++++++++++++++++++++++++ internal/commands/projects_test.go | 39 +++++++++++++++++ internal/output/errors.go | 21 ++++++++-- 5 files changed, 171 insertions(+), 8 deletions(-) diff --git a/internal/appctx/context.go b/internal/appctx/context.go index 2004ce78d..e55f3113a 100644 --- a/internal/appctx/context.go +++ b/internal/appctx/context.go @@ -282,7 +282,7 @@ func (a *App) Err(err error) error { } // Print the error response - if outputErr := a.Output.Err(err, opts...); outputErr != nil { + if outputErr := a.Output.Err(a.withAuthRemedy(err), opts...); outputErr != nil { return outputErr } @@ -294,6 +294,31 @@ func (a *App) Err(err error) error { return nil } +// withAuthRemedy replaces the generic login hint on an auth_required error +// with one that fits the credential actually in play. A 401 is classified +// far from the profile and the environment, so the SDK conversion can only +// say "basecamp auth login": under an active profile that command would +// store the new credential somewhere the failing command never reads, and +// under BASECAMP_TOKEN no login changes what requests send. Errors whose +// hint is already specific (the credential manager names the profile +// itself) are left alone. +func (a *App) withAuthRemedy(err error) error { + e := output.AsError(err) + if e.Code != output.CodeAuth || (e.Hint != "" && e.Hint != output.DefaultAuthHint) { + return err + } + hinted := *e + switch { + case os.Getenv("BASECAMP_TOKEN") != "": + hinted.Hint = "BASECAMP_TOKEN is set and every request uses it instead of a stored login; unset it, or export a token the server accepts" + case a.Auth != nil: + hinted.Hint = a.Auth.LoginHint() + default: + return err + } + return &hinted +} + // shouldIncludeStatsInError returns true if stats should be included in the error envelope. func (a *App) shouldIncludeStatsInError() bool { if !a.Flags.Stats || a.Flags.NoStats || a.Collector == nil { diff --git a/internal/auth/auth.go b/internal/auth/auth.go index 2e2c27e39..1639e11c8 100644 --- a/internal/auth/auth.go +++ b/internal/auth/auth.go @@ -273,6 +273,18 @@ func invalidGrant(err error) (string, bool) { } } +// forgetRefusedGrant deletes the stored credential only while it still +// carries the refresh token the server just refused. Each process has its +// own Manager lock, so two of them can enter the refresh window together: +// the first rotates and saves, the second is refused for reusing the old +// token, and an unconditional delete here would throw away the fresh +// credential the first one stored. +func (m *Manager) forgetRefusedGrant(origin, refusedToken string) { + if current, err := m.store.Load(origin); err == nil && current.RefreshToken == refusedToken { + _ = m.store.Delete(origin) + } +} + func (m *Manager) refreshLocked(ctx context.Context, origin string, creds *Credentials) error { if creds.RefreshToken == "" { return m.errAuth("No refresh token available") @@ -363,9 +375,16 @@ func (m *Manager) refreshLocked(ctx context.Context, origin string, creds *Crede // rather than re-tried by every later command: Basecamp's abuse // tracker bans the client and address after a handful of // invalid_grant failures, which would turn one expired session - // into a lockout. The delete's own outcome cannot change the - // answer — the session is over either way. - _ = m.store.Delete(origin) + // into a lockout. Only a BC5 credential is forgotten: its client is + // the fixed public one, so the refusal can only be about the grant. + // A Launchpad refresh sends whatever client the environment names, + // and the server answers invalid_grant for a token issued to a + // different client too, which is not proof the grant is dead. The + // delete's own outcome cannot change the answer — the session is + // over either way. + if creds.OAuthType == oauthTypeBC5 { + m.forgetRefusedGrant(origin, creds.RefreshToken) + } msg := "Your session has expired or was revoked" if desc = strings.TrimSpace(richtext.SanitizeSingleLine(desc)); desc != "" { msg += " (" + desc + ")" diff --git a/internal/auth/auth_test.go b/internal/auth/auth_test.go index 1cc224523..b5173d533 100644 --- a/internal/auth/auth_test.go +++ b/internal/auth/auth_test.go @@ -2019,3 +2019,70 @@ func TestRefresh_PreservesIdentityAndBinding(t *testing.T) { assert.Equal(t, "urn:bc:account:999", creds.Resource) assert.Equal(t, "bc5", creds.OAuthType) } + +// TestRefresh_InvalidGrantKeepsAConcurrentlyRotatedCredential: two +// processes can refresh at once; when the other one has already saved the +// rotated token, the refusal this one gets for reusing the old token must +// not delete the fresh credential. +func TestRefresh_InvalidGrantKeepsAConcurrentlyRotatedCredential(t *testing.T) { + var m *Manager + var key string + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + // The other process wins the race while this request is in flight. + require.NoError(t, m.store.Save(key, &Credentials{ + AccessToken: "rotated-tok", RefreshToken: "rotated-ref", OAuthType: "bc5", + TokenEndpoint: "http://" + r.Host + "/oauth/tokens", ExpiresAt: time.Now().Add(time.Hour).Unix(), + })) + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusBadRequest) + fmt.Fprint(w, `{"error":"invalid_grant"}`) + })) + defer srv.Close() + + m = &Manager{cfg: config.Default(), httpClient: srv.Client(), store: newTestStore(t, t.TempDir())} + key = m.credentialKey() + require.NoError(t, m.store.Save(key, &Credentials{ + AccessToken: "old-tok", RefreshToken: "old-ref", OAuthType: "bc5", + TokenEndpoint: srv.URL + "/oauth/tokens", ExpiresAt: time.Now().Add(-time.Hour).Unix(), + })) + + err := m.Refresh(context.Background()) + var cliErr *output.Error + require.ErrorAs(t, err, &cliErr) + assert.Equal(t, output.CodeAuth, cliErr.Code) + + creds, loadErr := m.store.Load(key) + require.NoError(t, loadErr, "the rotated credential must survive") + assert.Equal(t, "rotated-ref", creds.RefreshToken) +} + +// TestRefresh_InvalidGrantOnLaunchpadKeepsTheCredential: a Launchpad +// refresh sends whichever client the environment names, and the server +// also answers invalid_grant for a token issued to another client, so the +// refusal is reported but the credential is not deleted. +func TestRefresh_InvalidGrantOnLaunchpadKeepsTheCredential(t *testing.T) { + t.Setenv("BASECAMP_OAUTH_CLIENT_ID", "custom-id") + t.Setenv("BASECAMP_OAUTH_CLIENT_SECRET", "custom-secret") + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusBadRequest) + fmt.Fprint(w, `{"error":"invalid_grant"}`) + })) + defer srv.Close() + + m := &Manager{cfg: config.Default(), httpClient: srv.Client(), store: newTestStore(t, t.TempDir())} + key := m.credentialKey() + require.NoError(t, m.store.Save(key, &Credentials{ + AccessToken: "old-tok", RefreshToken: "old-ref", OAuthType: oauthTypeLaunchpad, + TokenEndpoint: srv.URL + "/authorization/token", ExpiresAt: time.Now().Add(-time.Hour).Unix(), + })) + + err := m.Refresh(context.Background()) + var cliErr *output.Error + require.ErrorAs(t, err, &cliErr) + assert.Equal(t, output.CodeAuth, cliErr.Code) + + creds, loadErr := m.store.Load(key) + require.NoError(t, loadErr, "a Launchpad refusal is not proof the grant is dead") + assert.Equal(t, "old-ref", creds.RefreshToken) +} diff --git a/internal/commands/projects_test.go b/internal/commands/projects_test.go index 9274a3e11..e88d85c03 100644 --- a/internal/commands/projects_test.go +++ b/internal/commands/projects_test.go @@ -248,3 +248,42 @@ func TestProjectsList401CarriesTheLoginHint(t *testing.T) { assert.Equal(t, "auth_required", envelope.Code) assert.Equal(t, "Run: basecamp auth login", envelope.Hint) } + +// TestProjectsList401HintNamesTheProfile: the rendered remedy must be the +// login that repairs the credential the command used. +func TestProjectsList401HintNamesTheProfile(t *testing.T) { + t.Setenv("BASECAMP_TOKEN", "") + app, out := setupProjectsMockApp(t, unauthorizedTransport{}) + app.Config.ActiveProfile = "work" + + err := executeCommand(NewProjectsCmd(), app, "list") + require.Error(t, err) + require.NoError(t, app.Err(err)) + + var envelope struct { + Code string `json:"code"` + Hint string `json:"hint"` + } + require.NoError(t, json.Unmarshal(out.Bytes(), &envelope), out.String()) + assert.Equal(t, "auth_required", envelope.Code) + assert.Equal(t, "Run: basecamp auth login -P work", envelope.Hint) +} + +// TestProjectsList401HintUnderEnvToken: a login cannot help while +// BASECAMP_TOKEN shadows every stored credential, so the remedy names the +// variable instead. +func TestProjectsList401HintUnderEnvToken(t *testing.T) { + t.Setenv("BASECAMP_TOKEN", "bc_at_rejected") + app, out := setupProjectsMockApp(t, unauthorizedTransport{}) + + err := executeCommand(NewProjectsCmd(), app, "list") + require.Error(t, err) + require.NoError(t, app.Err(err)) + + var envelope struct { + Hint string `json:"hint"` + } + require.NoError(t, json.Unmarshal(out.Bytes(), &envelope), out.String()) + assert.Contains(t, envelope.Hint, "BASECAMP_TOKEN is set") + assert.NotContains(t, envelope.Hint, "auth login") +} diff --git a/internal/output/errors.go b/internal/output/errors.go index ffee6a8cb..71b58b4d5 100644 --- a/internal/output/errors.go +++ b/internal/output/errors.go @@ -33,10 +33,17 @@ func ErrAmbiguous(resource string, matches []string) *Error { return clioutput.ErrAmbiguous(resource, matches) } +// AsError converts err to the CLI's structured error. An *Error already in +// the chain is the CLI's own verdict and wins over the SDK error it may +// wrap as its cause; only a bare SDK error is converted from its taxonomy. func AsError(err error) *Error { if gateErr := AsGateError(err); gateErr != nil { return gateErr } + var cliErr *Error + if errors.As(err, &cliErr) { + return WithAuthHint(cliErr) + } var sdkErr *basecamp.Error if errors.As(err, &sdkErr) { message := err.Error() @@ -58,9 +65,15 @@ func AsError(err error) *Error { return WithAuthHint(clioutput.AsError(err)) } -// WithAuthHint gives an auth_required error that names no remedy the login -// command. A 401 from the API and the SDK's own auth errors arrive without -// one, and "authentication required" alone leaves the reader to guess what +// DefaultAuthHint is the remedy an auth_required error carries when nothing +// closer to the credential has named one. The app boundary (appctx.App.Err) +// replaces it with a remedy that knows the active profile and whether +// BASECAMP_TOKEN is in play; this is the floor beneath that. +const DefaultAuthHint = "Run: basecamp auth login" + +// WithAuthHint adds the login remedy to an auth_required error that has +// none. A 401 from the API and the SDK's own auth errors arrive without a +// hint, and "authentication required" alone leaves the reader to guess what // to run. Errors that already carry a hint, or are not auth errors, pass // through untouched; a hinted copy is returned so the caller's error is not // rewritten under it. @@ -69,7 +82,7 @@ func WithAuthHint(e *Error) *Error { return e } hinted := *e - hinted.Hint = "Run: basecamp auth login" + hinted.Hint = DefaultAuthHint return &hinted } From 774ee46b3725dacdfd44ba64ba2e63654c8eb810 Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Sat, 12 Sep 2026 18:56:13 -0700 Subject: [PATCH 3/8] Quote the profile in the login remedy and scope the 401 rewrite MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The pasted login command carried the profile name bare, and names from configuration files are not checked at load time, so one with a space or shell syntax produced a command that selected the wrong profile or ran something else; the name is shell-quoted now. The app-boundary rewrite of the auth hint applied to every auth_required error, including the credential manager's own, so `auth refresh` and `auth token --stored` — which ignore BASECAMP_TOKEN by design — were told to unset it. Only errors carrying an SDK error, an API 401, are rewritten now. A failed delete of a refused credential is reported through the manager's warning sink instead of being dropped. --- internal/appctx/context.go | 22 ++++++++++-------- internal/auth/auth.go | 33 +++++++++++++++++++++++---- internal/auth/auth_test.go | 17 ++++++++++++++ internal/commands/auth_remedy_test.go | 31 +++++++++++++++++++++++++ 4 files changed, 89 insertions(+), 14 deletions(-) create mode 100644 internal/commands/auth_remedy_test.go diff --git a/internal/appctx/context.go b/internal/appctx/context.go index e55f3113a..008c22591 100644 --- a/internal/appctx/context.go +++ b/internal/appctx/context.go @@ -3,6 +3,7 @@ package appctx import ( "context" + "errors" "fmt" "net/http" "os" @@ -294,17 +295,20 @@ func (a *App) Err(err error) error { return nil } -// withAuthRemedy replaces the generic login hint on an auth_required error -// with one that fits the credential actually in play. A 401 is classified -// far from the profile and the environment, so the SDK conversion can only -// say "basecamp auth login": under an active profile that command would -// store the new credential somewhere the failing command never reads, and -// under BASECAMP_TOKEN no login changes what requests send. Errors whose -// hint is already specific (the credential manager names the profile -// itself) are left alone. +// withAuthRemedy replaces the generic login hint on an API 401 with one that +// fits the credential the request actually sent. The SDK classifies a 401 +// far from the profile and the environment, so its conversion can only say +// "basecamp auth login": under an active profile that command would store +// the new credential somewhere the failing command never reads, and under +// BASECAMP_TOKEN — which every request sends ahead of any stored login — no +// login changes anything. Only errors carrying an SDK error are rewritten: +// the credential manager's own failures name the profile themselves, and +// come from stored-credential operations (auth refresh, auth token +// --stored) that ignore the environment token by design. func (a *App) withAuthRemedy(err error) error { + var sdkErr *basecamp.Error e := output.AsError(err) - if e.Code != output.CodeAuth || (e.Hint != "" && e.Hint != output.DefaultAuthHint) { + if e.Code != output.CodeAuth || !errors.As(err, &sdkErr) || (e.Hint != "" && e.Hint != output.DefaultAuthHint) { return err } hinted := *e diff --git a/internal/auth/auth.go b/internal/auth/auth.go index 1639e11c8..fdae5797d 100644 --- a/internal/auth/auth.go +++ b/internal/auth/auth.go @@ -130,14 +130,30 @@ func (m *Manager) credentialKey() string { // LoginCommand is the command that re-establishes the active credential: // addressed to the active profile when there is one, since a bare login -// would store the new credential under the base URL instead. +// would store the new credential under the base URL instead. The command +// is meant to be pasted, so the profile name is shell-quoted. func (m *Manager) LoginCommand() string { if m.cfg.ActiveProfile != "" { - return "basecamp auth login -P " + m.cfg.ActiveProfile + return "basecamp auth login -P " + shellQuote(m.cfg.ActiveProfile) } return "basecamp auth login" } +// shellQuote renders s safe to embed in an emitted shell command: a clearly +// inert name passes through bare, anything else is single-quoted — the one +// POSIX form in which nothing substitutes — with embedded single quotes +// spelled '\”. Profile names come from configuration files, which do not +// apply the create-time name check. +func shellQuote(s string) string { + inert := s != "" && strings.IndexFunc(s, func(r rune) bool { + return !(r >= 'a' && r <= 'z' || r >= 'A' && r <= 'Z' || r >= '0' && r <= '9' || strings.ContainsRune("_./:@%+=-", r)) + }) < 0 + if inert { + return s + } + return "'" + strings.ReplaceAll(s, "'", `'\''`) + "'" +} + // LoginHint is LoginCommand as an error hint. func (m *Manager) LoginHint() string { return "Run: " + m.LoginCommand() @@ -278,10 +294,17 @@ func invalidGrant(err error) (string, bool) { // own Manager lock, so two of them can enter the refresh window together: // the first rotates and saves, the second is refused for reusing the old // token, and an unconditional delete here would throw away the fresh -// credential the first one stored. +// credential the first one stored. The re-read closes that window down to +// the gap between this Load and Delete; a rotation landing inside it is +// lost, which costs one login, and a cross-process lock on a store that is +// usually the OS keyring is not a price worth paying for that. func (m *Manager) forgetRefusedGrant(origin, refusedToken string) { - if current, err := m.store.Load(origin); err == nil && current.RefreshToken == refusedToken { - _ = m.store.Delete(origin) + current, err := m.store.Load(origin) + if err != nil || current.RefreshToken != refusedToken { + return + } + if err := m.store.Delete(origin); err != nil { + m.warnf("could not forget the refused credential for %s: %v", origin, err) } } diff --git a/internal/auth/auth_test.go b/internal/auth/auth_test.go index b5173d533..19f4867c6 100644 --- a/internal/auth/auth_test.go +++ b/internal/auth/auth_test.go @@ -2086,3 +2086,20 @@ func TestRefresh_InvalidGrantOnLaunchpadKeepsTheCredential(t *testing.T) { require.NoError(t, loadErr, "a Launchpad refusal is not proof the grant is dead") assert.Equal(t, "old-ref", creds.RefreshToken) } + +// TestLoginCommand_QuotesTheProfile: the remedy is pasted into a shell, and +// profile names loaded from configuration are not checked at load time. +func TestLoginCommand_QuotesTheProfile(t *testing.T) { + for name, want := range map[string]string{ + "": "basecamp auth login", + "work": "basecamp auth login -P work", + "work profile": "basecamp auth login -P 'work profile'", + "it's": `basecamp auth login -P 'it'\''s'`, + "$(rm -rf x)": "basecamp auth login -P '$(rm -rf x)'", + } { + cfg := config.Default() + cfg.ActiveProfile = name + m := &Manager{cfg: cfg} + assert.Equal(t, want, m.LoginCommand(), "profile %q", name) + } +} diff --git a/internal/commands/auth_remedy_test.go b/internal/commands/auth_remedy_test.go new file mode 100644 index 000000000..0de7f5b94 --- /dev/null +++ b/internal/commands/auth_remedy_test.go @@ -0,0 +1,31 @@ +package commands + +import ( + "encoding/json" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/basecamp/basecamp-cli/internal/output" +) + +// TestAuthTokenStoredKeepsTheLoginHintUnderEnvToken: auth token --stored +// deliberately ignores BASECAMP_TOKEN, so its stored-credential failure +// must keep the login remedy rather than be rewritten to talk about the +// environment token. +func TestAuthTokenStoredKeepsTheLoginHintUnderEnvToken(t *testing.T) { + t.Setenv("BASECAMP_TOKEN", "bc_at_env") + app, out := setupProjectsMockApp(t, unauthorizedTransport{}) + + err := executeCommand(NewAuthCmd(), app, "token", "--stored") + require.Error(t, err) + assert.Equal(t, output.CodeAuth, output.AsError(err).Code) + require.NoError(t, app.Err(err)) + + var envelope struct { + Hint string `json:"hint"` + } + require.NoError(t, json.Unmarshal(out.Bytes(), &envelope), out.String()) + assert.Equal(t, "Run: basecamp auth login", envelope.Hint) +} From 552d0caa0023ab8312680caa431272466ef188e8 Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Sat, 12 Sep 2026 18:58:21 -0700 Subject: [PATCH 4/8] Name the shell-inert rune set so staticcheck reads the quoting rule --- internal/auth/auth.go | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/internal/auth/auth.go b/internal/auth/auth.go index fdae5797d..209257f0c 100644 --- a/internal/auth/auth.go +++ b/internal/auth/auth.go @@ -145,15 +145,19 @@ func (m *Manager) LoginCommand() string { // spelled '\”. Profile names come from configuration files, which do not // apply the create-time name check. func shellQuote(s string) string { - inert := s != "" && strings.IndexFunc(s, func(r rune) bool { - return !(r >= 'a' && r <= 'z' || r >= 'A' && r <= 'Z' || r >= '0' && r <= '9' || strings.ContainsRune("_./:@%+=-", r)) - }) < 0 - if inert { + if s != "" && strings.IndexFunc(s, shellActive) < 0 { return s } return "'" + strings.ReplaceAll(s, "'", `'\''`) + "'" } +// shellActive reports whether r can mean anything to a POSIX shell outside +// quotes; letters, digits and a few inert punctuation marks cannot. +func shellActive(r rune) bool { + inert := r >= 'a' && r <= 'z' || r >= 'A' && r <= 'Z' || r >= '0' && r <= '9' || strings.ContainsRune("_./:@%+=-", r) + return !inert +} + // LoginHint is LoginCommand as an error hint. func (m *Manager) LoginHint() string { return "Run: " + m.LoginCommand() From b9b04310cb42018734ae8d6f6b43d15ea037225e Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Sat, 12 Sep 2026 19:12:51 -0700 Subject: [PATCH 5/8] Replace only the default prefix of a composed auth hint A command may append guidance of its own to the default login hint (a partial reorder's rerun note), and an exact match then left the bare login in place under a profile or BASECAMP_TOKEN. The remedy now replaces the default prefix and keeps what was appended. --- internal/appctx/context.go | 13 +++++++++---- internal/commands/auth_remedy_test.go | 26 ++++++++++++++++++++++++++ 2 files changed, 35 insertions(+), 4 deletions(-) diff --git a/internal/appctx/context.go b/internal/appctx/context.go index 008c22591..4dfaa7d29 100644 --- a/internal/appctx/context.go +++ b/internal/appctx/context.go @@ -308,18 +308,23 @@ func (a *App) Err(err error) error { func (a *App) withAuthRemedy(err error) error { var sdkErr *basecamp.Error e := output.AsError(err) - if e.Code != output.CodeAuth || !errors.As(err, &sdkErr) || (e.Hint != "" && e.Hint != output.DefaultAuthHint) { + if e.Code != output.CodeAuth || !errors.As(err, &sdkErr) || (e.Hint != "" && !strings.HasPrefix(e.Hint, output.DefaultAuthHint)) { return err } - hinted := *e + var remedy string switch { case os.Getenv("BASECAMP_TOKEN") != "": - hinted.Hint = "BASECAMP_TOKEN is set and every request uses it instead of a stored login; unset it, or export a token the server accepts" + remedy = "BASECAMP_TOKEN is set and every request uses it instead of a stored login; unset it, or export a token the server accepts" case a.Auth != nil: - hinted.Hint = a.Auth.LoginHint() + remedy = a.Auth.LoginHint() default: return err } + // A command may have appended guidance of its own to the default hint + // (a partial reorder's rerun note); the remedy replaces the default + // and keeps the rest. + hinted := *e + hinted.Hint = remedy + strings.TrimPrefix(e.Hint, output.DefaultAuthHint) return &hinted } diff --git a/internal/commands/auth_remedy_test.go b/internal/commands/auth_remedy_test.go index 0de7f5b94..6f18ffc55 100644 --- a/internal/commands/auth_remedy_test.go +++ b/internal/commands/auth_remedy_test.go @@ -7,6 +7,8 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "github.com/basecamp/basecamp-sdk/go/pkg/basecamp" + "github.com/basecamp/basecamp-cli/internal/output" ) @@ -29,3 +31,27 @@ func TestAuthTokenStoredKeepsTheLoginHintUnderEnvToken(t *testing.T) { require.NoError(t, json.Unmarshal(out.Bytes(), &envelope), out.String()) assert.Equal(t, "Run: basecamp auth login", envelope.Hint) } + +// TestAuthRemedyKeepsGuidanceAppendedToTheDefaultHint: a command that +// appended its own note to the default hint (a partial reorder's rerun +// guidance) keeps that note; only the login prefix is replaced. +func TestAuthRemedyKeepsGuidanceAppendedToTheDefaultHint(t *testing.T) { + t.Setenv("BASECAMP_TOKEN", "") + app, out := setupProjectsMockApp(t, unauthorizedTransport{}) + app.Config.ActiveProfile = "work" + + sdkErr := &basecamp.Error{Code: basecamp.CodeAuth, Message: "authentication required", HTTPStatus: 401} + composed := &output.Error{ + Code: output.CodeAuth, + Message: "Reordered 1 of 3 todolists; failed at #2: authentication required", + Hint: output.DefaultAuthHint + " Rerun the whole command once the cause is fixed.", + Cause: sdkErr, + } + require.NoError(t, app.Err(composed)) + + var envelope struct { + Hint string `json:"hint"` + } + require.NoError(t, json.Unmarshal(out.Bytes(), &envelope), out.String()) + assert.Equal(t, "Run: basecamp auth login -P work Rerun the whole command once the cause is fixed.", envelope.Hint) +} From 5180eac4203557cf2c950b7a06d1960b8951cf11 Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Sat, 12 Sep 2026 19:23:39 -0700 Subject: [PATCH 6/8] Treat a concurrent rotation of the refused credential as success When another process rotated and saved the credential while this refresh was in flight, the refusal for reusing the old token is not a dead session: the store holds a live credential. The refresh now returns success in that case so the callers reload it, instead of failing the command and asking for a login. --- internal/auth/auth.go | 21 ++++++++++++++++----- internal/auth/auth_test.go | 13 ++++++++----- 2 files changed, 24 insertions(+), 10 deletions(-) diff --git a/internal/auth/auth.go b/internal/auth/auth.go index 209257f0c..e259fb381 100644 --- a/internal/auth/auth.go +++ b/internal/auth/auth.go @@ -302,14 +302,22 @@ func invalidGrant(err error) (string, bool) { // the gap between this Load and Delete; a rotation landing inside it is // lost, which costs one login, and a cross-process lock on a store that is // usually the OS keyring is not a price worth paying for that. -func (m *Manager) forgetRefusedGrant(origin, refusedToken string) { +// +// It reports whether the store holds a credential other than the refused +// one — another process's rotation, which is a live credential the caller +// can reload rather than a session that has ended. +func (m *Manager) forgetRefusedGrant(origin, refusedToken string) (rotated bool) { current, err := m.store.Load(origin) - if err != nil || current.RefreshToken != refusedToken { - return + if err != nil { + return false + } + if current.RefreshToken != refusedToken { + return true } if err := m.store.Delete(origin); err != nil { m.warnf("could not forget the refused credential for %s: %v", origin, err) } + return false } func (m *Manager) refreshLocked(ctx context.Context, origin string, creds *Credentials) error { @@ -409,8 +417,11 @@ func (m *Manager) refreshLocked(ctx context.Context, origin string, creds *Crede // different client too, which is not proof the grant is dead. The // delete's own outcome cannot change the answer — the session is // over either way. - if creds.OAuthType == oauthTypeBC5 { - m.forgetRefusedGrant(origin, creds.RefreshToken) + if creds.OAuthType == oauthTypeBC5 && m.forgetRefusedGrant(origin, creds.RefreshToken) { + // Another process rotated the credential while this refresh + // was in flight: the store holds a live one, which the callers + // reload, so this refresh has succeeded by proxy. + return nil } msg := "Your session has expired or was revoked" if desc = strings.TrimSpace(richtext.SanitizeSingleLine(desc)); desc != "" { diff --git a/internal/auth/auth_test.go b/internal/auth/auth_test.go index 19f4867c6..df7669335 100644 --- a/internal/auth/auth_test.go +++ b/internal/auth/auth_test.go @@ -2023,7 +2023,8 @@ func TestRefresh_PreservesIdentityAndBinding(t *testing.T) { // TestRefresh_InvalidGrantKeepsAConcurrentlyRotatedCredential: two // processes can refresh at once; when the other one has already saved the // rotated token, the refusal this one gets for reusing the old token must -// not delete the fresh credential. +// not delete the fresh credential, and is not a failure: the store holds a +// live credential for the caller to reload. func TestRefresh_InvalidGrantKeepsAConcurrentlyRotatedCredential(t *testing.T) { var m *Manager var key string @@ -2046,14 +2047,16 @@ func TestRefresh_InvalidGrantKeepsAConcurrentlyRotatedCredential(t *testing.T) { TokenEndpoint: srv.URL + "/oauth/tokens", ExpiresAt: time.Now().Add(-time.Hour).Unix(), })) - err := m.Refresh(context.Background()) - var cliErr *output.Error - require.ErrorAs(t, err, &cliErr) - assert.Equal(t, output.CodeAuth, cliErr.Code) + require.NoError(t, m.Refresh(context.Background()), "the other process's rotation is this refresh's success") creds, loadErr := m.store.Load(key) require.NoError(t, loadErr, "the rotated credential must survive") assert.Equal(t, "rotated-ref", creds.RefreshToken) + + t.Setenv("BASECAMP_TOKEN", "") + token, err := m.AccessToken(context.Background()) + require.NoError(t, err) + assert.Equal(t, "rotated-tok", token, "the caller reloads the live credential") } // TestRefresh_InvalidGrantOnLaunchpadKeepsTheCredential: a Launchpad From 2af0b1d4f2c157f5f90e1cdc45cd8b6527d8939a Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Sun, 13 Sep 2026 16:12:22 -0700 Subject: [PATCH 7/8] Name the profile in every auth failure the refresh returns MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The profile-aware login remedy was attached at chosen sites inside the refresh, so the failures it did not name — an unsafe stored token endpoint, a half-configured OAuth client, a lane client that could not be built — reached the reader with the bare "basecamp auth login", which under an active profile stores the new credential where the failing command never looks. The refresh now has one exit, and every auth-class error crossing it without a remedy is given the profile's login. --- internal/auth/auth.go | 25 ++++++++++++++++++ internal/auth/auth_test.go | 54 ++++++++++++++++++++++++++++++++++++++ internal/output/errors.go | 2 +- 3 files changed, 80 insertions(+), 1 deletion(-) diff --git a/internal/auth/auth.go b/internal/auth/auth.go index e259fb381..307c14e28 100644 --- a/internal/auth/auth.go +++ b/internal/auth/auth.go @@ -4,6 +4,7 @@ package auth import ( "bufio" "context" + "errors" "fmt" "io" "net" @@ -170,6 +171,21 @@ func (m *Manager) errAuth(msg string) *output.Error { return e } +// hintLogin gives an auth_required error that carries no remedy, or only +// the profile-less default, the login naming the active profile; every +// other error passes through unchanged. The error is copied rather than +// rewritten: a lane client's setup error is cached and returned to every +// later caller. +func (m *Manager) hintLogin(err error) error { + var e *output.Error + if !errors.As(err, &e) || e.Code != output.CodeAuth || (e.Hint != "" && e.Hint != output.DefaultAuthHint) { + return err + } + hinted := *e + hinted.Hint = m.LoginHint() + return &hinted +} + // AccessToken returns a valid access token, refreshing if needed. // If BASECAMP_TOKEN env var is set, it's used directly without OAuth. func (m *Manager) AccessToken(ctx context.Context) (string, error) { @@ -320,7 +336,16 @@ func (m *Manager) forgetRefusedGrant(origin, refusedToken string) (rotated bool) return false } +// refreshLocked rotates the stored credential under the manager lock. The +// credential is the active profile's, so whatever auth-class failure the +// refresh hits — an unusable stored endpoint, a half-configured OAuth +// client, a refused grant — the remedy is the login that writes that +// profile's credential, not the bare one. func (m *Manager) refreshLocked(ctx context.Context, origin string, creds *Credentials) error { + return m.hintLogin(m.refreshCredential(ctx, origin, creds)) +} + +func (m *Manager) refreshCredential(ctx context.Context, origin string, creds *Credentials) error { if creds.RefreshToken == "" { return m.errAuth("No refresh token available") } diff --git a/internal/auth/auth_test.go b/internal/auth/auth_test.go index df7669335..07c984fe2 100644 --- a/internal/auth/auth_test.go +++ b/internal/auth/auth_test.go @@ -2106,3 +2106,57 @@ func TestLoginCommand_QuotesTheProfile(t *testing.T) { assert.Equal(t, want, m.LoginCommand(), "profile %q", name) } } + +// profiledRefresh is a Manager whose active profile holds the given +// credential, for refresh failures that never reach a token endpoint. +func profiledRefresh(t *testing.T, creds *Credentials) (*Manager, string) { + t.Helper() + cfg := config.Default() + cfg.ActiveProfile = "work" + m := &Manager{cfg: cfg, httpClient: &http.Client{}, store: newTestStore(t, t.TempDir())} + key := m.credentialKey() + creds.ExpiresAt = time.Now().Add(-time.Hour).Unix() + require.NoError(t, m.store.Save(key, creds)) + return m, key +} + +// TestRefresh_UnsafeTokenEndpointHintsTheProfile: a stored endpoint the +// refresh refuses to POST to is an auth failure of the profile's +// credential, and its remedy names the profile like the refused-grant path +// does; the credential is kept, since nothing was learned about the grant. +func TestRefresh_UnsafeTokenEndpointHintsTheProfile(t *testing.T) { + m, key := profiledRefresh(t, &Credentials{ + AccessToken: "old-tok", RefreshToken: "old-ref", OAuthType: "bc5", + TokenEndpoint: "https://user@evil.example/oauth/tokens", + }) + + err := m.Refresh(context.Background()) + var cliErr *output.Error + require.ErrorAs(t, err, &cliErr) + assert.Equal(t, output.CodeAuth, cliErr.Code) + assert.Contains(t, cliErr.Message, "invalid token endpoint") + assert.Equal(t, "Run: basecamp auth login -P work", cliErr.Hint) + + creds, loadErr := m.store.Load(key) + require.NoError(t, loadErr) + assert.Equal(t, "old-ref", creds.RefreshToken) +} + +// TestRefresh_HalfConfiguredClientHintsTheProfile: a Launchpad refresh +// with only one of the OAuth client variables set fails before any request, +// and that failure also names the profile. +func TestRefresh_HalfConfiguredClientHintsTheProfile(t *testing.T) { + t.Setenv("BASECAMP_OAUTH_CLIENT_ID", "custom-id") + t.Setenv("BASECAMP_OAUTH_CLIENT_SECRET", "") + m, _ := profiledRefresh(t, &Credentials{ + AccessToken: "old-tok", RefreshToken: "old-ref", OAuthType: oauthTypeLaunchpad, + TokenEndpoint: "https://launchpad.example/authorization/token", + }) + + err := m.Refresh(context.Background()) + var cliErr *output.Error + require.ErrorAs(t, err, &cliErr) + assert.Equal(t, output.CodeAuth, cliErr.Code) + assert.Contains(t, cliErr.Message, "BASECAMP_OAUTH_CLIENT_SECRET is required") + assert.Equal(t, "Run: basecamp auth login -P work", cliErr.Hint) +} diff --git a/internal/output/errors.go b/internal/output/errors.go index 71b58b4d5..d48b0a0f6 100644 --- a/internal/output/errors.go +++ b/internal/output/errors.go @@ -118,7 +118,7 @@ func ErrAuth(msg string) *Error { return &Error{ Code: CodeAuth, Message: msg, - Hint: "Run: basecamp auth login", + Hint: DefaultAuthHint, } } From d2693fcfd332174f70bb17af123b510ff3cd8de0 Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Sun, 13 Sep 2026 16:31:10 -0700 Subject: [PATCH 8/8] Say what a Launchpad refusal can mean instead of declaring the session over A Launchpad refresh sends whichever OAuth client the environment names, and the server answers invalid_grant for a token issued to a different client as well as for a dead grant. The credential is already kept in that case, but the message still said the session had expired, sending the reader to a new login when unsetting or correcting the client variables would have kept the one they had. The Launchpad message now names both readings. --- internal/auth/auth.go | 3 +++ internal/auth/auth_test.go | 2 ++ 2 files changed, 5 insertions(+) diff --git a/internal/auth/auth.go b/internal/auth/auth.go index 307c14e28..f8e72168b 100644 --- a/internal/auth/auth.go +++ b/internal/auth/auth.go @@ -449,6 +449,9 @@ func (m *Manager) refreshCredential(ctx context.Context, origin string, creds *C return nil } msg := "Your session has expired or was revoked" + if creds.OAuthType != oauthTypeBC5 { + msg = "The refresh token was refused: the session has expired or was revoked, or BASECAMP_OAUTH_CLIENT_ID/SECRET name a different OAuth client than the one it was issued to" + } if desc = strings.TrimSpace(richtext.SanitizeSingleLine(desc)); desc != "" { msg += " (" + desc + ")" } diff --git a/internal/auth/auth_test.go b/internal/auth/auth_test.go index 07c984fe2..8689e89fe 100644 --- a/internal/auth/auth_test.go +++ b/internal/auth/auth_test.go @@ -2084,6 +2084,8 @@ func TestRefresh_InvalidGrantOnLaunchpadKeepsTheCredential(t *testing.T) { var cliErr *output.Error require.ErrorAs(t, err, &cliErr) assert.Equal(t, output.CodeAuth, cliErr.Code) + assert.Contains(t, cliErr.Message, "BASECAMP_OAUTH_CLIENT_ID/SECRET name a different OAuth client", "the message names the client mismatch the refusal may mean") + assert.Equal(t, "Run: basecamp auth login", cliErr.Hint) creds, loadErr := m.store.Load(key) require.NoError(t, loadErr, "a Launchpad refusal is not proof the grant is dead")