Skip to content

Commit e121fce

Browse files
authored
Refuse credentials from a newer CLI, and stop refreshing a refused client (#959)
* 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. * 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. * 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.
1 parent d74f1d5 commit e121fce

7 files changed

Lines changed: 404 additions & 20 deletions

File tree

‎internal/auth/agent.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -192,7 +192,7 @@ func (m *Manager) mintAgentCredential(ctx context.Context, origin string, creds
192192
token, hold, err := m.mintAgentToken(ctx, mint)
193193
if err != nil {
194194
if hold != nil {
195-
m.rememberRenewalHold(origin, hold)
195+
m.rememberRenewalHold(origin, hold.Client, hold)
196196
}
197197
return err
198198
}

‎internal/auth/auth.go‎

Lines changed: 48 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -505,6 +505,27 @@ func invalidGrant(err error) (string, bool) {
505505
return "", false
506506
}
507507

508+
// invalidClient reports whether a refresh was refused with invalid_client:
509+
// the token endpoint does not accept the client the refresh was sent as.
510+
// Read off the SDK's typed refusal, as invalidGrant is, and only from the
511+
// statuses RFC 6749 §5.2 gives that refusal: a 429 or a server fault naming
512+
// the client is not a verdict on it.
513+
func invalidClient(err error) bool {
514+
var refusal *basecamp.Error
515+
return errors.As(err, &refusal) && refusal.OAuthError == "invalid_client" &&
516+
(refusal.HTTPStatus == http.StatusBadRequest || refusal.HTTPStatus == http.StatusUnauthorized)
517+
}
518+
519+
// errNewerCredential is the refusal for a stored credential whose OAuth type
520+
// this version does not know. The type is quoted, so a damaged store cannot
521+
// put terminal controls into the message.
522+
func (m *Manager) errNewerCredential(oauthType string) *output.Error {
523+
e := m.errAuth(fmt.Sprintf("These credentials were written by a newer basecamp CLI (OAuth type %q) and this version cannot refresh them", oauthType))
524+
command, _ := m.loginRemedy()
525+
e.Hint = "Run: basecamp upgrade (or, to keep this version, log in again with it: " + command + ")"
526+
return e
527+
}
528+
508529
// forgetRefusedGrant deletes the stored credential only while it still
509530
// carries the refresh token the server just refused. Two processes used to
510531
// be able to enter the refresh window together — the first rotating and
@@ -566,15 +587,28 @@ func (m *Manager) refreshLocked(ctx context.Context, origin string, creds *Crede
566587
// the credential's missing fields in place, checks what it holds, and
567588
// resolves the client and lane the request would go out through.
568589
func (m *Manager) prepareRefresh(creds *Credentials) (oauth.RefreshRequest, *oauth.Exchanger, error) {
569-
if creds.RefreshToken == "" {
570-
return oauth.RefreshRequest{}, nil, m.errAuth("No refresh token available")
571-
}
572-
573590
// Migrate old credentials missing OAuthType
574591
if creds.OAuthType == "" {
575592
creds.OAuthType = oauthTypeLaunchpad
576593
}
577594

595+
// Only the types this version knows how to refresh go any further. A
596+
// type it has never heard of was written by a newer CLI, whose client
597+
// and token endpoint this one cannot know: refreshing it as Launchpad
598+
// sends Launchpad's client to someone else's token endpoint, which
599+
// answers invalid_client on every command, forever. Checked before
600+
// anything the credential holds, since a newer type need not hold what
601+
// this version's refresh asks for.
602+
switch creds.OAuthType {
603+
case "bc3", oauthTypeBC5, oauthTypeLaunchpad:
604+
default:
605+
return oauth.RefreshRequest{}, nil, m.errNewerCredential(creds.OAuthType)
606+
}
607+
608+
if creds.RefreshToken == "" {
609+
return oauth.RefreshRequest{}, nil, m.errAuth("No refresh token available")
610+
}
611+
578612
// Migrate old credentials missing TokenEndpoint
579613
if creds.TokenEndpoint == "" {
580614
if creds.OAuthType == "bc3" || creds.OAuthType == oauthTypeBC5 {
@@ -608,7 +642,7 @@ func (m *Manager) prepareRefresh(creds *Credentials) (oauth.RefreshRequest, *oau
608642
case oauthTypeBC5:
609643
// Pre-registered public client: no secret.
610644
clientID = bc5ClientID
611-
default:
645+
case oauthTypeLaunchpad:
612646
// Launchpad (or old credentials defaulted to launchpad)
613647
if envCreds, err := resolveClientCredentials(func(string) {}); err != nil {
614648
return oauth.RefreshRequest{}, nil, err
@@ -633,6 +667,12 @@ func (m *Manager) prepareRefresh(creds *Credentials) (oauth.RefreshRequest, *oau
633667
return oauth.RefreshRequest{}, nil, laneErr
634668
}
635669

670+
// A client the token endpoint already refused for this refresh token
671+
// is refused again here, before anything is sent.
672+
if err := m.refusedRefreshClient(creds, clientID, clientSecret); err != nil {
673+
return oauth.RefreshRequest{}, nil, err
674+
}
675+
636676
// A rate limit the token endpoint already put on this refresh token is
637677
// answered last, once everything local has resolved and only the
638678
// request is left — which is also what lets a report say the next
@@ -669,6 +709,9 @@ func (m *Manager) refreshCredential(ctx context.Context, origin string, creds *C
669709
if held := m.holdRateLimitedRefresh(origin, creds, err); held != nil {
670710
return held
671711
}
712+
if invalidClient(err) {
713+
return m.holdRefusedRefreshClient(origin, creds, req.ClientID, req.ClientSecret, err)
714+
}
672715
desc, dead := invalidGrant(err)
673716
if !dead {
674717
return wrapOAuthError("token refresh failed", err)
Lines changed: 217 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,217 @@
1+
package auth
2+
3+
import (
4+
"context"
5+
"fmt"
6+
"net/http"
7+
"net/http/httptest"
8+
"sync/atomic"
9+
"testing"
10+
"time"
11+
12+
"github.com/basecamp/basecamp-sdk/go/pkg/basecamp"
13+
"github.com/stretchr/testify/assert"
14+
"github.com/stretchr/testify/require"
15+
16+
"github.com/basecamp/basecamp-cli/internal/config"
17+
"github.com/basecamp/basecamp-cli/internal/output"
18+
)
19+
20+
// A credential of a type this version has never heard of was written by a
21+
// newer CLI. It is refused before anything is sent — not refreshed as
22+
// Launchpad, which sent Launchpad's client to the newer login's token
23+
// endpoint and drew invalid_client on every command — and the refusal says
24+
// to upgrade.
25+
func TestRefresh_UnknownOAuthTypeRefusesWithoutSending(t *testing.T) {
26+
for name, tc := range map[string]struct{ endpoint, refreshToken string }{
27+
"with its token endpoint": {"https://3.basecampapi.com/oauth/tokens", "old-ref"},
28+
// Launchpad's migration would have filled one in.
29+
"without one": {"", "old-ref"},
30+
// A newer type need not renew by refresh token at all, and is
31+
// still named for what it is rather than as a missing token.
32+
"without a refresh token": {"https://3.basecampapi.com/oauth/tokens", ""},
33+
} {
34+
endpoint := tc.endpoint
35+
t.Run(name, func(t *testing.T) {
36+
t.Setenv("BASECAMP_OAUTH_CLIENT_ID", "")
37+
t.Setenv("BASECAMP_OAUTH_CLIENT_SECRET", "")
38+
transport := &recordingTransport{}
39+
cfg := config.Default()
40+
cfg.ActiveProfile = "work"
41+
m := &Manager{cfg: cfg, httpClient: &http.Client{Transport: transport}, store: newTestStore(t, t.TempDir())}
42+
creds := &Credentials{
43+
AccessToken: "old-tok",
44+
RefreshToken: tc.refreshToken,
45+
OAuthType: "bc6\x1b[31m",
46+
TokenEndpoint: endpoint,
47+
ExpiresAt: time.Now().Add(-time.Hour).Unix(),
48+
}
49+
key := m.credentialKey()
50+
require.NoError(t, m.store.Save(key, creds))
51+
52+
err := m.Refresh(context.Background())
53+
var cliErr *output.Error
54+
require.ErrorAs(t, err, &cliErr)
55+
assert.Equal(t, output.CodeAuth, cliErr.Code)
56+
assert.Contains(t, cliErr.Message, "newer basecamp CLI")
57+
assert.Contains(t, cliErr.Message, `"bc6\x1b[31m"`, "the type is quoted, never written raw")
58+
assert.NotContains(t, cliErr.Message, "\x1b")
59+
assert.Equal(t, "Run: basecamp upgrade (or, to keep this version, log in again with it: basecamp auth login -P work)", cliErr.Hint)
60+
assert.False(t, transport.attempted.Load(), "an unknown type must be refused without any network request")
61+
62+
stored, loadErr := m.store.Load(key)
63+
require.NoError(t, loadErr, "the newer CLI's credential is left for the newer CLI")
64+
assert.Equal(t, "bc6\x1b[31m", stored.OAuthType)
65+
assert.Equal(t, endpoint, stored.TokenEndpoint, "nothing is migrated onto it")
66+
67+
refusal := m.RefreshRefusal(stored)
68+
require.Error(t, refusal)
69+
assert.Contains(t, output.AsError(refusal).Message, "newer basecamp CLI", "the report says what the next command will do")
70+
})
71+
}
72+
}
73+
74+
// invalidClientRefresh is a token endpoint that answers every refresh with
75+
// status and an invalid_client body, counting what reaches it, and a Manager
76+
// whose active profile's credential of oauthType refreshes against it.
77+
func invalidClientRefresh(t *testing.T, status int, oauthType string) (*Manager, string, *atomic.Int32) {
78+
t.Helper()
79+
var hits atomic.Int32
80+
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
81+
hits.Add(1)
82+
w.Header().Set("Content-Type", "application/json")
83+
w.WriteHeader(status)
84+
fmt.Fprint(w, `{"error":"invalid_client","error_description":"Client authentication failed"}`)
85+
}))
86+
t.Cleanup(srv.Close)
87+
88+
cfg := config.Default()
89+
cfg.ActiveProfile = "work"
90+
cfg.BaseURL = srv.URL
91+
m := &Manager{cfg: cfg, httpClient: srv.Client(), store: newTestStore(t, t.TempDir())}
92+
key := m.credentialKey()
93+
require.NoError(t, m.store.Save(key, &Credentials{
94+
AccessToken: "old-tok",
95+
RefreshToken: "old-ref",
96+
OAuthType: oauthType,
97+
TokenEndpoint: srv.URL + "/oauth/tokens",
98+
ExpiresAt: time.Now().Add(-time.Hour).Unix(),
99+
}))
100+
return m, key, &hits
101+
}
102+
103+
// invalid_client on a refresh is terminal: the token endpoint does not know
104+
// the client, and asking again as the same client cannot change that. The
105+
// first refusal tells the person to log in again; every later command, in
106+
// this process or another, answers from the store and sends nothing — each
107+
// resend is charged to the abuse tracker that blocks the address.
108+
func TestRefresh_InvalidClientIsTerminal(t *testing.T) {
109+
for _, oauthType := range []string{oauthTypeBC5, oauthTypeLaunchpad} {
110+
t.Run(oauthType, func(t *testing.T) {
111+
t.Setenv("BASECAMP_OAUTH_CLIENT_ID", "")
112+
t.Setenv("BASECAMP_OAUTH_CLIENT_SECRET", "")
113+
t.Setenv("BASECAMP_TOKEN", "")
114+
m, key, hits := invalidClientRefresh(t, http.StatusUnauthorized, oauthType)
115+
116+
err := m.Refresh(context.Background())
117+
var cliErr *output.Error
118+
require.ErrorAs(t, err, &cliErr)
119+
assert.Equal(t, output.CodeAuth, cliErr.Code)
120+
assert.Contains(t, cliErr.Message, "invalid_client")
121+
assert.Contains(t, cliErr.Message, "will not be sent again")
122+
assert.Equal(t, "Run: basecamp auth login -P work", cliErr.Hint)
123+
var refusal *basecamp.Error
124+
require.ErrorAs(t, err, &refusal, "the SDK's refusal stays in the chain")
125+
assert.Equal(t, int32(1), hits.Load())
126+
127+
stored, loadErr := m.store.Load(key)
128+
require.NoError(t, loadErr, "the credential is kept: a hold is undone by a login, a delete is not undone")
129+
require.NotNil(t, stored.RenewalHold)
130+
assert.Equal(t, renewalHoldRefused, stored.RenewalHold.Kind)
131+
assert.Zero(t, stored.RenewalHold.Until, "invalid_client is never rechecked")
132+
133+
for range 3 {
134+
_, err = m.AccessToken(context.Background())
135+
require.ErrorAs(t, err, &cliErr)
136+
assert.Equal(t, output.CodeAuth, cliErr.Code)
137+
assert.Contains(t, cliErr.Message, "remembered")
138+
assert.Equal(t, "Run: basecamp auth login -P work", cliErr.Hint)
139+
}
140+
fresh := &Manager{cfg: m.cfg, httpClient: m.httpClient, store: m.store}
141+
require.Error(t, fresh.Refresh(context.Background()))
142+
assert.Equal(t, int32(1), hits.Load(), "a refused client was sent again")
143+
144+
status := m.RenewalHoldStatus(stored, m.RefreshRefusal(stored))
145+
require.NotNil(t, status, "the report shows the hold")
146+
assert.True(t, status.Permanent())
147+
148+
// A login writes a new refresh token, which the hold is not about.
149+
stored.RefreshToken = "new-ref"
150+
require.NoError(t, m.store.Save(key, stored))
151+
require.Error(t, m.Refresh(context.Background()))
152+
assert.Equal(t, int32(2), hits.Load(), "a new login was held to the old one's refusal")
153+
})
154+
}
155+
}
156+
157+
// A Launchpad refresh is sent as whatever client the environment names, so
158+
// a refusal is held for that client only: fixing BASECAMP_OAUTH_CLIENT_ID
159+
// sends the refresh again, without a login.
160+
func TestRefresh_InvalidClientHoldIsForTheClientItRefused(t *testing.T) {
161+
t.Setenv("BASECAMP_OAUTH_CLIENT_ID", "wrong-client")
162+
t.Setenv("BASECAMP_OAUTH_CLIENT_SECRET", "wrong-secret")
163+
m, _, hits := invalidClientRefresh(t, http.StatusUnauthorized, oauthTypeLaunchpad)
164+
165+
err := m.Refresh(context.Background())
166+
require.Error(t, err)
167+
assert.Contains(t, err.Error(), "BASECAMP_OAUTH_CLIENT_ID/SECRET")
168+
require.Error(t, m.Refresh(context.Background()))
169+
assert.Equal(t, int32(1), hits.Load())
170+
171+
t.Setenv("BASECAMP_OAUTH_CLIENT_ID", "")
172+
t.Setenv("BASECAMP_OAUTH_CLIENT_SECRET", "")
173+
require.Error(t, m.Refresh(context.Background()))
174+
assert.Equal(t, int32(2), hits.Load(), "a refresh as another client was held to this one's refusal")
175+
}
176+
177+
// The status decides, as it does for an agent's mint: a server fault or a
178+
// rate limit that happens to name invalid_client is no verdict on the
179+
// client, and holds nothing of the kind.
180+
func TestRefresh_InvalidClientFromAServerFaultIsNotTerminal(t *testing.T) {
181+
m, key, hits := invalidClientRefresh(t, http.StatusServiceUnavailable, oauthTypeBC5)
182+
183+
err := m.Refresh(context.Background())
184+
require.Error(t, err)
185+
assert.NotContains(t, err.Error(), "will not be sent again")
186+
stored, loadErr := m.store.Load(key)
187+
require.NoError(t, loadErr)
188+
assert.Nil(t, stored.RenewalHold)
189+
190+
_ = m.Refresh(context.Background())
191+
assert.Equal(t, int32(2), hits.Load(), "a server fault is tried again")
192+
}
193+
194+
// invalidClient is read off the typed refusal and its status, never the text.
195+
func TestInvalidClient_ReadsTheTypedCodeAndStatus(t *testing.T) {
196+
assert.True(t, invalidClient(fmt.Errorf("wrapped: %w", &basecamp.Error{HTTPStatus: 401, OAuthError: "invalid_client"})))
197+
assert.True(t, invalidClient(&basecamp.Error{HTTPStatus: 400, OAuthError: "invalid_client"}))
198+
assert.False(t, invalidClient(&basecamp.Error{HTTPStatus: 429, OAuthError: "invalid_client"}))
199+
assert.False(t, invalidClient(&basecamp.Error{HTTPStatus: 500, OAuthError: "invalid_client"}))
200+
assert.False(t, invalidClient(&basecamp.Error{HTTPStatus: 401, OAuthError: "invalid_grant", Message: "invalid_client"}))
201+
assert.False(t, invalidClient(fmt.Errorf("token error: invalid_client")))
202+
}
203+
204+
// The refusal promises that the refresh will not be sent again only when the
205+
// hold that keeps that promise was stored.
206+
func TestRefresh_InvalidClientPromisesOnlyAStoredHold(t *testing.T) {
207+
m := &Manager{cfg: config.Default(), store: newTestStore(t, t.TempDir())}
208+
creds := &Credentials{RefreshToken: "old-ref", OAuthType: oauthTypeBC5}
209+
refusal := &basecamp.Error{HTTPStatus: 401, OAuthError: "invalid_client"}
210+
211+
err := m.holdRefusedRefreshClient("nowhere", creds, bc5ClientID, "", refusal)
212+
var cliErr *output.Error
213+
require.ErrorAs(t, err, &cliErr)
214+
assert.Equal(t, output.CodeAuth, cliErr.Code)
215+
assert.Contains(t, cliErr.Message, "invalid_client")
216+
assert.NotContains(t, cliErr.Message, "will not be sent again", "nothing was stored to keep that promise")
217+
}

0 commit comments

Comments
 (0)