From 89dc7bedc5a9ef36e344ca1fc6936c231b1d6b72 Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Fri, 23 Oct 2026 12:17:48 -0700 Subject: [PATCH 1/3] Refuse credentials from a newer CLI, and stop refreshing a refused client A credential whose oauth_type this version doesn't know was refreshed as Launchpad: an older CLI reading a newer login sent Launchpad's client to that login's token endpoint, which answered invalid_client on every command, indefinitely. Only "" and "launchpad" take the Launchpad path now; any other unknown type is refused before anything is sent, with the upgrade as the remedy. invalid_client on a refresh is terminal: asking again as the same client cannot succeed, and each ask is charged to the abuse tracker. The refusal is held on the stored login with no recheck, for that refresh token and client, so later commands answer locally and say to log in again. A login or a changed BASECAMP_OAUTH_CLIENT_ID/SECRET is not held to it. --- internal/auth/agent.go | 2 +- internal/auth/auth.go | 43 +++++- internal/auth/refresh_client_test.go | 198 +++++++++++++++++++++++++++ internal/auth/renewal_hold.go | 79 ++++++++++- internal/auth/renewal_hold_test.go | 6 +- 5 files changed, 316 insertions(+), 12 deletions(-) create mode 100644 internal/auth/refresh_client_test.go diff --git a/internal/auth/agent.go b/internal/auth/agent.go index 5e9217e58..551b7f89f 100644 --- a/internal/auth/agent.go +++ b/internal/auth/agent.go @@ -192,7 +192,7 @@ func (m *Manager) mintAgentCredential(ctx context.Context, origin string, creds token, hold, err := m.mintAgentToken(ctx, mint) if err != nil { if hold != nil { - m.rememberRenewalHold(origin, hold) + m.rememberRenewalHold(origin, hold.Client, hold) } return err } diff --git a/internal/auth/auth.go b/internal/auth/auth.go index 06199b6bd..bd5dccc6f 100644 --- a/internal/auth/auth.go +++ b/internal/auth/auth.go @@ -505,6 +505,27 @@ func invalidGrant(err error) (string, bool) { return "", false } +// invalidClient reports whether a refresh was refused with invalid_client: +// the token endpoint does not accept the client the refresh was sent as. +// Read off the SDK's typed refusal, as invalidGrant is, and only from the +// statuses RFC 6749 §5.2 gives that refusal: a 429 or a server fault naming +// the client is not a verdict on it. +func invalidClient(err error) bool { + var refusal *basecamp.Error + return errors.As(err, &refusal) && refusal.OAuthError == "invalid_client" && + (refusal.HTTPStatus == http.StatusBadRequest || refusal.HTTPStatus == http.StatusUnauthorized) +} + +// errNewerCredential is the refusal for a stored credential whose OAuth type +// this version does not know. The type is quoted, so a damaged store cannot +// put terminal controls into the message. +func (m *Manager) errNewerCredential(oauthType string) *output.Error { + e := m.errAuth(fmt.Sprintf("These credentials were written by a newer basecamp CLI (OAuth type %q) and this version cannot refresh them", oauthType)) + command, _ := m.loginRemedy() + e.Hint = "Run: basecamp upgrade (or, to keep this version, log in again with it: " + command + ")" + return e +} + // forgetRefusedGrant deletes the stored credential only while it still // carries the refresh token the server just refused. Two processes used to // be able to enter the refresh window together — the first rotating and @@ -575,6 +596,17 @@ func (m *Manager) prepareRefresh(creds *Credentials) (oauth.RefreshRequest, *oau creds.OAuthType = oauthTypeLaunchpad } + // Only the types this version knows how to refresh go any further. A + // type it has never heard of was written by a newer CLI, whose client + // and token endpoint this one cannot know: refreshing it as Launchpad + // sends Launchpad's client to someone else's token endpoint, which + // answers invalid_client on every command, forever. + switch creds.OAuthType { + case "bc3", oauthTypeBC5, oauthTypeLaunchpad: + default: + return oauth.RefreshRequest{}, nil, m.errNewerCredential(creds.OAuthType) + } + // Migrate old credentials missing TokenEndpoint if creds.TokenEndpoint == "" { if creds.OAuthType == "bc3" || creds.OAuthType == oauthTypeBC5 { @@ -608,7 +640,7 @@ func (m *Manager) prepareRefresh(creds *Credentials) (oauth.RefreshRequest, *oau case oauthTypeBC5: // Pre-registered public client: no secret. clientID = bc5ClientID - default: + case oauthTypeLaunchpad: // Launchpad (or old credentials defaulted to launchpad) if envCreds, err := resolveClientCredentials(func(string) {}); err != nil { return oauth.RefreshRequest{}, nil, err @@ -633,6 +665,12 @@ func (m *Manager) prepareRefresh(creds *Credentials) (oauth.RefreshRequest, *oau return oauth.RefreshRequest{}, nil, laneErr } + // A client the token endpoint already refused for this refresh token + // is refused again here, before anything is sent. + if err := m.refusedRefreshClient(creds, clientID, clientSecret); err != nil { + return oauth.RefreshRequest{}, nil, err + } + // A rate limit the token endpoint already put on this refresh token is // answered last, once everything local has resolved and only the // request is left — which is also what lets a report say the next @@ -669,6 +707,9 @@ func (m *Manager) refreshCredential(ctx context.Context, origin string, creds *C if held := m.holdRateLimitedRefresh(origin, creds, err); held != nil { return held } + if invalidClient(err) { + return m.holdRefusedRefreshClient(origin, creds, req.ClientID, req.ClientSecret, err) + } desc, dead := invalidGrant(err) if !dead { return wrapOAuthError("token refresh failed", err) diff --git a/internal/auth/refresh_client_test.go b/internal/auth/refresh_client_test.go new file mode 100644 index 000000000..44452fe82 --- /dev/null +++ b/internal/auth/refresh_client_test.go @@ -0,0 +1,198 @@ +package auth + +import ( + "context" + "fmt" + "net/http" + "net/http/httptest" + "sync/atomic" + "testing" + "time" + + "github.com/basecamp/basecamp-sdk/go/pkg/basecamp" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/basecamp/basecamp-cli/internal/config" + "github.com/basecamp/basecamp-cli/internal/output" +) + +// A credential of a type this version has never heard of was written by a +// newer CLI. It is refused before anything is sent — not refreshed as +// Launchpad, which sent Launchpad's client to the newer login's token +// endpoint and drew invalid_client on every command — and the refusal says +// to upgrade. +func TestRefresh_UnknownOAuthTypeRefusesWithoutSending(t *testing.T) { + for name, endpoint := range map[string]string{ + "with its token endpoint": "https://3.basecampapi.com/oauth/tokens", + // Launchpad's migration would have filled one in. + "without one": "", + } { + t.Run(name, func(t *testing.T) { + t.Setenv("BASECAMP_OAUTH_CLIENT_ID", "") + t.Setenv("BASECAMP_OAUTH_CLIENT_SECRET", "") + transport := &recordingTransport{} + cfg := config.Default() + cfg.ActiveProfile = "work" + m := &Manager{cfg: cfg, httpClient: &http.Client{Transport: transport}, store: newTestStore(t, t.TempDir())} + creds := &Credentials{ + AccessToken: "old-tok", + RefreshToken: "old-ref", + OAuthType: "bc6\x1b[31m", + TokenEndpoint: endpoint, + ExpiresAt: time.Now().Add(-time.Hour).Unix(), + } + key := m.credentialKey() + require.NoError(t, m.store.Save(key, creds)) + + 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, "newer basecamp CLI") + assert.Contains(t, cliErr.Message, `"bc6\x1b[31m"`, "the type is quoted, never written raw") + assert.NotContains(t, cliErr.Message, "\x1b") + assert.Equal(t, "Run: basecamp upgrade (or, to keep this version, log in again with it: basecamp auth login -P work)", cliErr.Hint) + assert.False(t, transport.attempted.Load(), "an unknown type must be refused without any network request") + + stored, loadErr := m.store.Load(key) + require.NoError(t, loadErr, "the newer CLI's credential is left for the newer CLI") + assert.Equal(t, "bc6\x1b[31m", stored.OAuthType) + assert.Equal(t, endpoint, stored.TokenEndpoint, "nothing is migrated onto it") + + refusal := m.RefreshRefusal(stored) + require.Error(t, refusal) + assert.Contains(t, output.AsError(refusal).Message, "newer basecamp CLI", "the report says what the next command will do") + }) + } +} + +// invalidClientRefresh is a token endpoint that answers every refresh with +// status and an invalid_client body, counting what reaches it, and a Manager +// whose active profile's credential of oauthType refreshes against it. +func invalidClientRefresh(t *testing.T, status int, oauthType string) (*Manager, string, *atomic.Int32) { + t.Helper() + var hits atomic.Int32 + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + hits.Add(1) + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(status) + fmt.Fprint(w, `{"error":"invalid_client","error_description":"Client authentication failed"}`) + })) + t.Cleanup(srv.Close) + + cfg := config.Default() + cfg.ActiveProfile = "work" + cfg.BaseURL = srv.URL + 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: oauthType, + TokenEndpoint: srv.URL + "/oauth/tokens", + ExpiresAt: time.Now().Add(-time.Hour).Unix(), + })) + return m, key, &hits +} + +// invalid_client on a refresh is terminal: the token endpoint does not know +// the client, and asking again as the same client cannot change that. The +// first refusal tells the person to log in again; every later command, in +// this process or another, answers from the store and sends nothing — each +// resend is charged to the abuse tracker that blocks the address. +func TestRefresh_InvalidClientIsTerminal(t *testing.T) { + for _, oauthType := range []string{oauthTypeBC5, oauthTypeLaunchpad} { + t.Run(oauthType, func(t *testing.T) { + t.Setenv("BASECAMP_OAUTH_CLIENT_ID", "") + t.Setenv("BASECAMP_OAUTH_CLIENT_SECRET", "") + t.Setenv("BASECAMP_TOKEN", "") + m, key, hits := invalidClientRefresh(t, http.StatusUnauthorized, oauthType) + + 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_client") + assert.Contains(t, cliErr.Message, "will not be sent again") + assert.Equal(t, "Run: basecamp auth login -P work", cliErr.Hint) + var refusal *basecamp.Error + require.ErrorAs(t, err, &refusal, "the SDK's refusal stays in the chain") + assert.Equal(t, int32(1), hits.Load()) + + stored, loadErr := m.store.Load(key) + require.NoError(t, loadErr, "the credential is kept: a hold is undone by a login, a delete is not undone") + require.NotNil(t, stored.RenewalHold) + assert.Equal(t, renewalHoldRefused, stored.RenewalHold.Kind) + assert.Zero(t, stored.RenewalHold.Until, "invalid_client is never rechecked") + + for range 3 { + _, err = m.AccessToken(context.Background()) + require.ErrorAs(t, err, &cliErr) + assert.Equal(t, output.CodeAuth, cliErr.Code) + assert.Contains(t, cliErr.Message, "remembered") + assert.Equal(t, "Run: basecamp auth login -P work", cliErr.Hint) + } + fresh := &Manager{cfg: m.cfg, httpClient: m.httpClient, store: m.store} + require.Error(t, fresh.Refresh(context.Background())) + assert.Equal(t, int32(1), hits.Load(), "a refused client was sent again") + + status := m.RenewalHoldStatus(stored, m.RefreshRefusal(stored)) + require.NotNil(t, status, "the report shows the hold") + assert.True(t, status.Permanent()) + + // A login writes a new refresh token, which the hold is not about. + stored.RefreshToken = "new-ref" + require.NoError(t, m.store.Save(key, stored)) + require.Error(t, m.Refresh(context.Background())) + assert.Equal(t, int32(2), hits.Load(), "a new login was held to the old one's refusal") + }) + } +} + +// A Launchpad refresh is sent as whatever client the environment names, so +// a refusal is held for that client only: fixing BASECAMP_OAUTH_CLIENT_ID +// sends the refresh again, without a login. +func TestRefresh_InvalidClientHoldIsForTheClientItRefused(t *testing.T) { + t.Setenv("BASECAMP_OAUTH_CLIENT_ID", "wrong-client") + t.Setenv("BASECAMP_OAUTH_CLIENT_SECRET", "wrong-secret") + m, _, hits := invalidClientRefresh(t, http.StatusUnauthorized, oauthTypeLaunchpad) + + err := m.Refresh(context.Background()) + require.Error(t, err) + assert.Contains(t, err.Error(), "BASECAMP_OAUTH_CLIENT_ID/SECRET") + require.Error(t, m.Refresh(context.Background())) + assert.Equal(t, int32(1), hits.Load()) + + t.Setenv("BASECAMP_OAUTH_CLIENT_ID", "") + t.Setenv("BASECAMP_OAUTH_CLIENT_SECRET", "") + require.Error(t, m.Refresh(context.Background())) + assert.Equal(t, int32(2), hits.Load(), "a refresh as another client was held to this one's refusal") +} + +// The status decides, as it does for an agent's mint: a server fault or a +// rate limit that happens to name invalid_client is no verdict on the +// client, and holds nothing of the kind. +func TestRefresh_InvalidClientFromAServerFaultIsNotTerminal(t *testing.T) { + m, key, hits := invalidClientRefresh(t, http.StatusServiceUnavailable, oauthTypeBC5) + + err := m.Refresh(context.Background()) + require.Error(t, err) + assert.NotContains(t, err.Error(), "will not be sent again") + stored, loadErr := m.store.Load(key) + require.NoError(t, loadErr) + assert.Nil(t, stored.RenewalHold) + + _ = m.Refresh(context.Background()) + assert.Equal(t, int32(2), hits.Load(), "a server fault is tried again") +} + +// invalidClient is read off the typed refusal and its status, never the text. +func TestInvalidClient_ReadsTheTypedCodeAndStatus(t *testing.T) { + assert.True(t, invalidClient(fmt.Errorf("wrapped: %w", &basecamp.Error{HTTPStatus: 401, OAuthError: "invalid_client"}))) + assert.True(t, invalidClient(&basecamp.Error{HTTPStatus: 400, OAuthError: "invalid_client"})) + assert.False(t, invalidClient(&basecamp.Error{HTTPStatus: 429, OAuthError: "invalid_client"})) + assert.False(t, invalidClient(&basecamp.Error{HTTPStatus: 500, OAuthError: "invalid_client"})) + assert.False(t, invalidClient(&basecamp.Error{HTTPStatus: 401, OAuthError: "invalid_grant", Message: "invalid_client"})) + assert.False(t, invalidClient(fmt.Errorf("token error: invalid_client"))) +} diff --git a/internal/auth/renewal_hold.go b/internal/auth/renewal_hold.go index b4e5b5d9c..10f40e6c5 100644 --- a/internal/auth/renewal_hold.go +++ b/internal/auth/renewal_hold.go @@ -110,8 +110,9 @@ const MaxServerWait = maxAgentConnectLifetime // RenewalHold is the token endpoint's last refusal of a credential's renewal // — an agent's mint, or anyone else's refresh — remembered so the next -// renewal can answer it without asking again. A refresh is only ever held -// for a rate limit (holdRateLimitedRefresh). +// renewal can answer it without asking again. A refresh is held for a rate +// limit (holdRateLimitedRefresh) or for an invalid_client refusal of the +// client it was sent as (holdRefusedRefreshClient). // // Nothing in it came from the server as free text: Detail is the same fixed // vocabulary agentMintRefusal renders (see oauthErrorCodes). @@ -184,7 +185,7 @@ func (m *Manager) holdRateLimitedRefresh(origin string, creds *Credentials, err Client: refreshTokenFingerprint(creds.RefreshToken), Until: ceilUnix(now.Add(min(wait, MaxServerWait))), } - m.rememberRenewalHold(origin, hold) + m.rememberRenewalHold(origin, hold.Client, hold) held := time.Unix(hold.Until, 0).UTC().Format(time.RFC3339) var msg string @@ -209,6 +210,68 @@ func (m *Manager) holdRateLimitedRefresh(origin string, creds *Credentials, err return e } +// refreshClientFingerprint names a refresh token together with the client it +// is presented as, for the one hold that is about the pair: an invalid_client +// refusal. A Launchpad refresh is sent as whatever client +// BASECAMP_OAUTH_CLIENT_ID/SECRET name, so the same refresh token can be +// refused as one client and accepted as another; a hold on the token alone +// would outlive the fix. +func refreshClientFingerprint(refreshToken, clientID, clientSecret string) string { + sum := sha256.Sum256([]byte("basecamp-cli refresh client hold\x00" + refreshToken + "\x00" + clientID + "\x00" + clientSecret)) + return hex.EncodeToString(sum[:16]) +} + +// holdRefusedRefreshClient remembers an invalid_client answer to a refresh +// and returns the auth error it is answered with. +// +// invalid_client is not a refusal that waits out: the token endpoint does not +// know the client the refresh was sent as, and asking again as the same +// client cannot change that. Every command that finds the access token near +// expiry would otherwise send the same refresh, and a scheduled job sends it +// on every run — each one charged to the abuse tracker that blocks the +// address after a handful. So it is held with no recheck, for exactly this +// refresh token and client (refreshClientFingerprint); a login writes a +// credential with no hold, and a changed client is not held. +func (m *Manager) holdRefusedRefreshClient(origin string, creds *Credentials, clientID, clientSecret string, err error) error { + m.rememberRenewalHold(origin, refreshTokenFingerprint(creds.RefreshToken), &RenewalHold{ + Kind: renewalHoldRefused, + Detail: invalidClientDetail, + Client: refreshClientFingerprint(creds.RefreshToken, clientID, clientSecret), + }) + e := m.errAuth(refusedRefreshClientMessage(creds) + "; it will not be sent again") + // The SDK's refusal stays in the chain, so its request id and OAuth + // error still reach the error envelope. + e.Cause = err + return e +} + +// refusedRefreshClient is the error a remembered invalid_client answers a +// refresh with, or nil when the refresh may be sent. A hold for another +// refresh token or another client holds nothing. +func (m *Manager) refusedRefreshClient(creds *Credentials, clientID, clientSecret string) error { + hold := creds.RenewalHold + if hold == nil || creds.OAuthType == oauthTypeAgent || hold.Kind != renewalHoldRefused || + hold.Until != 0 || hold.Detail != invalidClientDetail || + hold.Client != refreshClientFingerprint(creds.RefreshToken, clientID, clientSecret) { + return nil + } + e := m.errAuth(refusedRefreshClientMessage(creds) + "; the refusal is remembered, and this refresh will not be sent again") + e.Cause = errRenewalHeld + return e +} + +// refusedRefreshClientMessage says what an invalid_client refusal of creds' +// refresh means. A BC5 refresh is sent as the fixed public client, so the +// login itself is the remedy; a Launchpad one may have been sent as a client +// the environment names. +func refusedRefreshClientMessage(creds *Credentials) string { + msg := "The token endpoint refused the OAuth client this login refreshes as (" + invalidClientDetail + ")" + if creds.OAuthType == oauthTypeLaunchpad { + msg += ", either because the login is not one this client can refresh or because BASECAMP_OAUTH_CLIENT_ID/SECRET name a different client" + } + return msg +} + // refreshTokenFingerprint names a refresh token the way // agentClientFingerprint names a client: a truncated SHA-256 under a label of // its own, so a hold can say which refresh token it is about without @@ -375,8 +438,10 @@ func (hold *RenewalHold) holds(creds *Credentials, now time.Time) bool { if hold == nil || hold.Client != renewalSubject(creds) { return false } - // A refresh is only ever held for a rate limit: a refused refresh token - // is forgotten outright rather than held (refreshCredential). + // A refresh is held here only for a rate limit. A refused grant is + // forgotten outright rather than held (refreshCredential), and a refused + // client is held for the client it was sent as, which only + // refusedRefreshClient can read. if creds.OAuthType != oauthTypeAgent && hold.Kind != renewalHoldRateLimited { return false } @@ -411,7 +476,7 @@ func (hold *RenewalHold) holds(creds *Credentials, now time.Time) bool { // // A failure to write is reported and otherwise changes nothing — the // refusal is what the caller sees either way. -func (m *Manager) rememberRenewalHold(origin string, hold *RenewalHold) { +func (m *Manager) rememberRenewalHold(origin, about string, hold *RenewalHold) { current, err := m.store.Load(origin) if err != nil { // A credential removed since the mint (a logout) has nothing to @@ -421,7 +486,7 @@ func (m *Manager) rememberRenewalHold(origin string, hold *RenewalHold) { } return } - if renewalSubject(current) != hold.Client { + if renewalSubject(current) != about { return } current.RenewalHold = hold diff --git a/internal/auth/renewal_hold_test.go b/internal/auth/renewal_hold_test.go index c52736e19..355b22c3f 100644 --- a/internal/auth/renewal_hold_test.go +++ b/internal/auth/renewal_hold_test.go @@ -360,7 +360,7 @@ func TestAStaleRefusalNeverLandsOnANewSecret(t *testing.T) { relogged.ClientSecret = "a-new-secret" require.NoError(t, m.store.Save(key, relogged)) - m.rememberRenewalHold(key, stale) + m.rememberRenewalHold(key, stale.Client, stale) stored, err := m.store.Load(key) require.NoError(t, err) @@ -489,12 +489,12 @@ func TestAFailedReadIsReportedWhenARefusalCannotBeRemembered(t *testing.T) { var warned []string m.Warnf = func(format string, args ...any) { warned = append(warned, fmt.Sprintf(format, args...)) } - m.rememberRenewalHold("nowhere", &RenewalHold{Kind: renewalHoldRefused}) + m.rememberRenewalHold("nowhere", "", &RenewalHold{Kind: renewalHoldRefused}) assert.Empty(t, warned, "a credential that is gone has nothing to remember a refusal on") m.store = &Store{inner: unreadableStore{}} m.store.initOnce.Do(func() {}) - m.rememberRenewalHold(key, &RenewalHold{Kind: renewalHoldRefused}) + m.rememberRenewalHold(key, "", &RenewalHold{Kind: renewalHoldRefused}) require.Len(t, warned, 1) assert.Contains(t, warned[0], "could not read the credential") } From bffbd21a6d16eb77937b75f3f80b32a5d6d64ddc Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Fri, 23 Oct 2026 12:39:05 -0700 Subject: [PATCH 2/3] Name a newer credential's type before asking it for a refresh token A newer type need not renew by refresh token, and one without was reported as "No refresh token available" with a login as the remedy, rather than as a newer credential to upgrade for. --- internal/auth/auth.go | 12 +++++++----- internal/auth/refresh_client_test.go | 12 ++++++++---- 2 files changed, 15 insertions(+), 9 deletions(-) diff --git a/internal/auth/auth.go b/internal/auth/auth.go index bd5dccc6f..7c06d0a19 100644 --- a/internal/auth/auth.go +++ b/internal/auth/auth.go @@ -587,10 +587,6 @@ func (m *Manager) refreshLocked(ctx context.Context, origin string, creds *Crede // the credential's missing fields in place, checks what it holds, and // resolves the client and lane the request would go out through. func (m *Manager) prepareRefresh(creds *Credentials) (oauth.RefreshRequest, *oauth.Exchanger, error) { - if creds.RefreshToken == "" { - return oauth.RefreshRequest{}, nil, m.errAuth("No refresh token available") - } - // Migrate old credentials missing OAuthType if creds.OAuthType == "" { creds.OAuthType = oauthTypeLaunchpad @@ -600,13 +596,19 @@ func (m *Manager) prepareRefresh(creds *Credentials) (oauth.RefreshRequest, *oau // type it has never heard of was written by a newer CLI, whose client // and token endpoint this one cannot know: refreshing it as Launchpad // sends Launchpad's client to someone else's token endpoint, which - // answers invalid_client on every command, forever. + // answers invalid_client on every command, forever. Checked before + // anything the credential holds, since a newer type need not hold what + // this version's refresh asks for. switch creds.OAuthType { case "bc3", oauthTypeBC5, oauthTypeLaunchpad: default: return oauth.RefreshRequest{}, nil, m.errNewerCredential(creds.OAuthType) } + if creds.RefreshToken == "" { + return oauth.RefreshRequest{}, nil, m.errAuth("No refresh token available") + } + // Migrate old credentials missing TokenEndpoint if creds.TokenEndpoint == "" { if creds.OAuthType == "bc3" || creds.OAuthType == oauthTypeBC5 { diff --git a/internal/auth/refresh_client_test.go b/internal/auth/refresh_client_test.go index 44452fe82..b3d20177b 100644 --- a/internal/auth/refresh_client_test.go +++ b/internal/auth/refresh_client_test.go @@ -23,11 +23,15 @@ import ( // endpoint and drew invalid_client on every command — and the refusal says // to upgrade. func TestRefresh_UnknownOAuthTypeRefusesWithoutSending(t *testing.T) { - for name, endpoint := range map[string]string{ - "with its token endpoint": "https://3.basecampapi.com/oauth/tokens", + for name, tc := range map[string]struct{ endpoint, refreshToken string }{ + "with its token endpoint": {"https://3.basecampapi.com/oauth/tokens", "old-ref"}, // Launchpad's migration would have filled one in. - "without one": "", + "without one": {"", "old-ref"}, + // A newer type need not renew by refresh token at all, and is + // still named for what it is rather than as a missing token. + "without a refresh token": {"https://3.basecampapi.com/oauth/tokens", ""}, } { + endpoint := tc.endpoint t.Run(name, func(t *testing.T) { t.Setenv("BASECAMP_OAUTH_CLIENT_ID", "") t.Setenv("BASECAMP_OAUTH_CLIENT_SECRET", "") @@ -37,7 +41,7 @@ func TestRefresh_UnknownOAuthTypeRefusesWithoutSending(t *testing.T) { m := &Manager{cfg: cfg, httpClient: &http.Client{Transport: transport}, store: newTestStore(t, t.TempDir())} creds := &Credentials{ AccessToken: "old-tok", - RefreshToken: "old-ref", + RefreshToken: tc.refreshToken, OAuthType: "bc6\x1b[31m", TokenEndpoint: endpoint, ExpiresAt: time.Now().Add(-time.Hour).Unix(), From 9219f45749d8f30df433ece18ba1eb3d36201e91 Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Fri, 23 Oct 2026 12:58:10 -0700 Subject: [PATCH 3/3] Promise no resend only when the hold was stored, and name a login's remedy A refused refresh said it would not be sent again even when the hold that keeps that promise could not be stored. And auth status told a login held for invalid_client to log in with a new secret, the remedy for an agent's refused secret, rather than to log in again. --- internal/auth/refresh_client_test.go | 15 +++++++ internal/auth/renewal_hold.go | 19 ++++++--- internal/commands/auth.go | 8 +++- internal/commands/auth_status_agent_test.go | 46 +++++++++++++++++++++ 4 files changed, 81 insertions(+), 7 deletions(-) diff --git a/internal/auth/refresh_client_test.go b/internal/auth/refresh_client_test.go index b3d20177b..fedbbf0c6 100644 --- a/internal/auth/refresh_client_test.go +++ b/internal/auth/refresh_client_test.go @@ -200,3 +200,18 @@ func TestInvalidClient_ReadsTheTypedCodeAndStatus(t *testing.T) { assert.False(t, invalidClient(&basecamp.Error{HTTPStatus: 401, OAuthError: "invalid_grant", Message: "invalid_client"})) assert.False(t, invalidClient(fmt.Errorf("token error: invalid_client"))) } + +// The refusal promises that the refresh will not be sent again only when the +// hold that keeps that promise was stored. +func TestRefresh_InvalidClientPromisesOnlyAStoredHold(t *testing.T) { + m := &Manager{cfg: config.Default(), store: newTestStore(t, t.TempDir())} + creds := &Credentials{RefreshToken: "old-ref", OAuthType: oauthTypeBC5} + refusal := &basecamp.Error{HTTPStatus: 401, OAuthError: "invalid_client"} + + err := m.holdRefusedRefreshClient("nowhere", creds, bc5ClientID, "", refusal) + var cliErr *output.Error + require.ErrorAs(t, err, &cliErr) + assert.Equal(t, output.CodeAuth, cliErr.Code) + assert.Contains(t, cliErr.Message, "invalid_client") + assert.NotContains(t, cliErr.Message, "will not be sent again", "nothing was stored to keep that promise") +} diff --git a/internal/auth/renewal_hold.go b/internal/auth/renewal_hold.go index 10f40e6c5..f477818a4 100644 --- a/internal/auth/renewal_hold.go +++ b/internal/auth/renewal_hold.go @@ -233,12 +233,16 @@ func refreshClientFingerprint(refreshToken, clientID, clientSecret string) strin // refresh token and client (refreshClientFingerprint); a login writes a // credential with no hold, and a changed client is not held. func (m *Manager) holdRefusedRefreshClient(origin string, creds *Credentials, clientID, clientSecret string, err error) error { - m.rememberRenewalHold(origin, refreshTokenFingerprint(creds.RefreshToken), &RenewalHold{ + stored := m.rememberRenewalHold(origin, refreshTokenFingerprint(creds.RefreshToken), &RenewalHold{ Kind: renewalHoldRefused, Detail: invalidClientDetail, Client: refreshClientFingerprint(creds.RefreshToken, clientID, clientSecret), }) - e := m.errAuth(refusedRefreshClientMessage(creds) + "; it will not be sent again") + msg := refusedRefreshClientMessage(creds) + if stored { + msg += "; it will not be sent again" + } + e := m.errAuth(msg) // The SDK's refusal stays in the chain, so its request id and OAuth // error still reach the error envelope. e.Cause = err @@ -475,8 +479,9 @@ func (hold *RenewalHold) holds(creds *Credentials, now time.Time) bool { // instead would leave exactly those hosts minting on every poll. // // A failure to write is reported and otherwise changes nothing — the -// refusal is what the caller sees either way. -func (m *Manager) rememberRenewalHold(origin, about string, hold *RenewalHold) { +// refusal is what the caller sees either way. It reports whether the hold +// was stored, for a caller whose message promises what later commands do. +func (m *Manager) rememberRenewalHold(origin, about string, hold *RenewalHold) (stored bool) { current, err := m.store.Load(origin) if err != nil { // A credential removed since the mint (a logout) has nothing to @@ -484,15 +489,17 @@ func (m *Manager) rememberRenewalHold(origin, about string, hold *RenewalHold) { if !errors.Is(err, ErrNoCredential) { m.warnf("warning: could not read the credential for %s to remember the token endpoint's refusal, so the next command will ask again: %v", origin, err) } - return + return false } if renewalSubject(current) != about { - return + return false } current.RenewalHold = hold if err := m.store.Save(origin, current); err != nil { m.warnf("warning: could not remember the token endpoint's refusal for %s, so the next command will ask again: %v", origin, err) + return false } + return true } // now is the Manager's clock: the clock field when a test set one. diff --git a/internal/commands/auth.go b/internal/commands/auth.go index e2f85e288..c561fe0dc 100644 --- a/internal/commands/auth.go +++ b/internal/commands/auth.go @@ -458,7 +458,13 @@ func authStatusReport(ctx context.Context, app *appctx.App) (*authStatus, error) case !expired && hold != nil: // A stored hold that never ends: the next renewal will be // refused, and the line says so before the token runs out. - expiry = "expires in " + coarseDuration(expiresIn) + "; renewal would be refused: " + hold.Detail + "; log in with a new secret" + // An agent's refusal is of its secret; anyone else's is of the + // client a refresh is sent as, which a login replaces. + remedy := "log in again" + if creds.OAuthType == "agent" { + remedy = "log in with a new secret" + } + expiry = "expires in " + coarseDuration(expiresIn) + "; renewal would be refused: " + hold.Detail + "; " + remedy report.hint = remedyFor(app, refusal) case !expired: expiry = "expires in " + coarseDuration(expiresIn) diff --git a/internal/commands/auth_status_agent_test.go b/internal/commands/auth_status_agent_test.go index 2c9cac0aa..c8372a2f1 100644 --- a/internal/commands/auth_status_agent_test.go +++ b/internal/commands/auth_status_agent_test.go @@ -224,6 +224,52 @@ func TestAuthStatusNamesAPermanentHoldWhileTheTokenIsLive(t *testing.T) { assert.Contains(t, report.hint, "--with-client-credentials") } +// TestAuthStatusNamesARefusedRefreshClientsRemedy: a login's refresh held +// for invalid_client is a refusal of the client the refresh is sent as, not +// of an agent's secret, so the line names a login, not a new secret. +func TestAuthStatusNamesARefusedRefreshClientsRemedy(t *testing.T) { + t.Setenv("BASECAMP_NO_KEYRING", "1") + t.Setenv("BASECAMP_TOKEN", "") + t.Setenv("XDG_CONFIG_HOME", t.TempDir()) + + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusUnauthorized) + _, _ = w.Write([]byte(`{"error":"invalid_client"}`)) + })) + t.Cleanup(srv.Close) + + cfg := &config.Config{BaseURL: srv.URL, ActiveProfile: "work", Sources: map[string]string{}} + authMgr := auth.NewManager(cfg, srv.Client()) + store := auth.NewStore(config.GlobalConfigDir()) + authMgr.SetStore(store) + require.NoError(t, store.Save("profile:work", &auth.Credentials{ + AccessToken: "spent", + RefreshToken: "refused-ref", + OAuthType: "bc5", + Scope: "full", + TokenEndpoint: srv.URL + "/oauth/tokens", + ExpiresAt: time.Now().Add(-time.Minute).Unix(), + })) + _, err := authMgr.AccessToken(context.Background()) + require.Error(t, err) + + creds, err := store.Load("profile:work") + require.NoError(t, err) + require.NotNil(t, creds.RenewalHold) + creds.AccessToken = "still-good" + creds.ExpiresAt = time.Now().Add(time.Hour).Unix() + require.NoError(t, store.Save("profile:work", creds)) + + app := &appctx.App{Config: cfg, Auth: authMgr} + report, err := authStatusReport(context.Background(), app) + require.NoError(t, err) + joined := strings.Join(report.details, "\n") + assert.Contains(t, joined, "renewal would be refused: token error: invalid_client; log in again") + assert.NotContains(t, joined, "new secret") + assert.Contains(t, report.hint, "basecamp auth login -P work") +} + // TestAuthStatusReportsATimeLimitedHoldAsRefreshable: a hold that ends on // its own — a 429 until its Retry-After, a refusal rechecked hourly — is // what a renewal sent now would meet, and nothing more. The credential is