From a71e661e6f3dd721ff53d787b2188e20f3de292b Mon Sep 17 00:00:00 2001 From: Zacharias Dyna Knudsen Date: Tue, 15 Sep 2026 10:35:07 +0200 Subject: [PATCH 1/4] Forget a refresh token the server has refused MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A refresh that failed left the credential on disk whatever the failure was. When the failure was invalid_grant — the grant expired, revoked, or already spent, as every session is when a password changes — the dead token stayed put and went back out on the next command, and the next. The token endpoint's rate limit counts refusals as well as successes and is keyed on the address, so this is self-inflicted: `hey watch` re-authenticates on every ActionCable dial and redials on a fifteen-second timeout, so one dead grant spends the whole hourly allowance in minutes and goes on spending each new one — including the allowance a fresh sign-in needs. The Omarchy bar plugin restarts `hey watch` after any non-auth exit, which kept that going indefinitely. Split the token endpoint's answers into the one that is a verdict on the grant and the rest that are not. invalid_grant on a 4xx clears the credential and reports an auth error, so the next command asks for a login instead of resending a token that will never be accepted. A transport failure, a 5xx, an unparseable body, a different OAuth error code, and a 429 all leave the credential alone — signing someone out over a blip is the worse failure. A 429 is transient, but asking again during one cannot get through and only spends what is left, so it also parks refreshes in this process until the limit can have cleared: Retry-After when the server sends one, fifteen minutes otherwise. While parked, a token that has not actually expired yet is used as-is rather than failing the command — the refresh window opens five minutes early, so it usually still works. Two processes can queue on the credential lock holding the same dead grant, so a re-read that finds the credential gone now stops rather than falling back to the copy it came in with: another process has just had that grant refused. --- internal/auth/auth.go | 152 ++++++++++++- internal/auth/auth_test.go | 421 +++++++++++++++++++++++++++++++++++++ internal/auth/oauth.go | 111 +++++++++- internal/auth/store.go | 18 +- 4 files changed, 682 insertions(+), 20 deletions(-) diff --git a/internal/auth/auth.go b/internal/auth/auth.go index b50e7e1a..fb027189 100644 --- a/internal/auth/auth.go +++ b/internal/auth/auth.go @@ -12,6 +12,8 @@ import ( "strings" "sync" "time" + + "github.com/basecamp/hey-cli/internal/apierr" ) // Built-in OAuth client ID for the CLI app. @@ -32,8 +34,21 @@ type Manager struct { callbackWait callbackWaiter listen listenerFactory mu sync.Mutex + + // refreshHoldUntil is when this process will next send a refresh, after the + // token endpoint rate-limited one. Guarded by mu, which every path into + // refreshLocked already holds. + refreshHoldUntil time.Time } +// defaultRefreshHold is how long to sit out a rate limit the server did not put a +// Retry-After on. The token endpoint's limit is a fixed window an hour wide, so a +// process pausing this long spends only a few of the allowance per window however +// long it runs — where `hey watch`, which re-authenticates on every ActionCable +// dial and redials on a fifteen-second timeout, spends all of it in minutes and +// then keeps it spent. +const defaultRefreshHold = 15 * time.Minute + // NewManager creates a new auth manager. func NewManager(baseURL string, httpClient *http.Client, configDir string) *Manager { listenConfig := &net.ListenConfig{} @@ -62,7 +77,7 @@ func (m *Manager) AccessToken(ctx context.Context) (string, error) { creds, err := m.store.Load(m.baseURL) if err != nil { - return "", fmt.Errorf("not authenticated: %w", err) + return "", errNoCredential(fmt.Sprintf("not authenticated: %v", err), err) } // Check if token is expired (with 5-minute buffer) @@ -85,7 +100,7 @@ func (m *Manager) AccessToken(ctx context.Context) (string, error) { return creds.SessionCookie, nil } - return "", fmt.Errorf("no access token or session cookie available") + return "", errNoCredential("no access token or session cookie available", nil) } // AuthenticateRequest sets the appropriate auth header on an HTTP request. @@ -101,7 +116,7 @@ func (m *Manager) AuthenticateRequest(ctx context.Context, req *http.Request) er creds, err := m.store.Load(m.baseURL) if err != nil { - return fmt.Errorf("not authenticated: %w", err) + return errNoCredential(fmt.Sprintf("not authenticated: %v", err), err) } if creds.AccessToken != "" { @@ -124,7 +139,7 @@ func (m *Manager) AuthenticateRequest(ctx context.Context, req *http.Request) er return nil } - return fmt.Errorf("no access token or session cookie available") + return errNoCredential("no access token or session cookie available", nil) } // IsAuthenticated checks if there are valid credentials. @@ -258,7 +273,7 @@ func (m *Manager) Refresh(ctx context.Context) error { creds, err := m.store.Load(m.baseURL) if err != nil { - return fmt.Errorf("not authenticated: %w", err) + return errNoCredential(fmt.Sprintf("not authenticated: %v", err), err) } // Cookie-based auth doesn't support refresh; treat as no-op. @@ -281,17 +296,37 @@ func (m *Manager) refreshLocked(ctx context.Context, creds *Credentials) error { } defer unlock() - if stored, loadErr := m.store.load(m.baseURL); loadErr == nil { - if stored.AccessToken != "" && stored.AccessToken != creds.AccessToken { - return nil - } - creds = stored + stored, loadErr := m.store.load(m.baseURL) + if loadErr != nil { + // The credential went away while we waited for the lock, which is what + // happens when another process just had this same grant refused and + // forgot it. Falling back to the copy we came in with would send a token + // already known to be dead, and spend one more of an allowance that is + // counted per address and shared by every process here. + return errNoCredential(fmt.Sprintf("not authenticated: %v", loadErr), loadErr) } + if stored.AccessToken != "" && stored.AccessToken != creds.AccessToken { + return nil + } + creds = stored if creds.RefreshToken == "" { return fmt.Errorf("no refresh token available") } + if held, until := m.refreshHeld(); held { + // The endpoint rate-limited us recently. Its limit counts refusals as + // well as successes, so asking again now cannot get through and only + // spends the allowance a fresh sign-in will need. + if creds.ExpiresAt > time.Now().Unix() { + // The five-minute buffer opened this refresh early and the access + // token has not actually expired yet, so the command carries on with + // the one we hold and tries again later. + return nil + } + return errRefreshHeld(until) + } + tokenEndpoint := creds.TokenEndpoint if tokenEndpoint == "" { tokenEndpoint = m.baseURL + "/oauth/tokens" @@ -304,8 +339,9 @@ func (m *Manager) refreshLocked(ctx context.Context, creds *Credentials) error { token, err := refreshOAuthToken(ctx, m.httpClient, tokenEndpoint, creds.RefreshToken, oauthClientID, installID) if err != nil { - return fmt.Errorf("token refresh failed: %w", err) + return m.accountForRefreshFailure(err) } + m.refreshHoldUntil = time.Time{} // A 200 without an access token is not a refresh. Storing the empty string would // take the working token with it and leave nothing to authenticate with. if token.AccessToken == "" { @@ -325,6 +361,100 @@ func (m *Manager) refreshLocked(ctx context.Context, creds *Credentials) error { return m.store.save(m.baseURL, creds) } +// accountForRefreshFailure decides what a failed refresh costs the stored credential. +// Only the server refusing the grant itself clears it; every other failure leaves it +// exactly where it was. +// +// Getting this split wrong is expensive in both directions. Keeping a refused grant is +// the bug this replaces: the credential stayed on disk, so every later command re-sent +// a token the server had already killed, and because the token endpoint's limit counts +// refusals as well as successes, a background `hey watch` could spend an hour's +// allowance in minutes and go on spending it — locking the address out of the login it +// needed to recover. Clearing on anything softer is the opposite failure: a flaky +// network or a passing 502 would sign people out of a session that was fine. +func (m *Manager) accountForRefreshFailure(err error) error { + var refusal *tokenEndpointError + if !errors.As(err, &refusal) { + // A transport failure — no answer from the server at all, so no verdict on + // the grant. + return fmt.Errorf("token refresh failed: %w", err) + } + + if refusal.rateLimited() { + // Says nothing about the credential: the server declined to look at it. + m.holdRefreshes(refusal.RetryAfter) + return errRefreshHeld(m.refreshHoldUntil) + } + + if !refusal.grantRefused() { + return fmt.Errorf("token refresh failed: %w", err) + } + + // invalid_grant. The refresh token is expired, revoked or already spent, and it + // will be refused the same way forever. Forget it here, under the lock that + // already spans this load-refresh-save, so the next command asks for a login + // instead of re-sending it. + if delErr := m.store.delete(m.baseURL); delErr != nil { + // The grant is dead whether or not the store would let go of it, and the + // credential is still on disk for the next command to find. Hold refreshes + // so a cleanup that failed cannot become the same hammering by another + // name, and still report it as the auth failure it is. + m.holdRefreshes(0) + return &apierr.Error{ + Code: apierr.CodeAuth, + Message: fmt.Sprintf("HEY refused the stored refresh token: the session has expired or was revoked. The stored credentials could not be cleared: %v", delErr), + Hint: "Run: hey auth logout, then: hey auth login", + HTTPStatus: 401, + Cause: delErr, + } + } + return apierr.ErrAuth("HEY refused the stored refresh token: the session has expired or was revoked. Changing your HEY password ends every session, including this one. The stored credentials have been cleared") +} + +// refreshHeld reports whether this process is sitting out a rate limit, and until when. +func (m *Manager) refreshHeld() (bool, time.Time) { + if m.refreshHoldUntil.IsZero() || !time.Now().Before(m.refreshHoldUntil) { + return false, time.Time{} + } + return true, m.refreshHoldUntil +} + +// holdRefreshes parks refreshes until the rate limit has had time to clear, honoring +// the server's Retry-After when it sends one. The hold only ever moves later, so a +// short hint cannot shorten a longer wait already in force. +func (m *Manager) holdRefreshes(retryAfter time.Duration) { + if retryAfter <= 0 { + retryAfter = defaultRefreshHold + } + if until := time.Now().Add(retryAfter); until.After(m.refreshHoldUntil) { + m.refreshHoldUntil = until + } +} + +// errNoCredential reports a missing or unusable credential. It carries the auth code +// so the exit status and the login hint survive the trip out through the SDK: the auth +// strategy is ours and the SDK hands our errors back untouched, so an unclassified one +// would reach the envelope as a generic API failure — exiting 7 where a script, and the +// service that restarts `hey watch`, are both watching for the auth exit. +func errNoCredential(msg string, cause error) *apierr.Error { + return &apierr.Error{ + Code: apierr.CodeAuth, + Message: msg, + Hint: "Run: hey auth login", + HTTPStatus: 401, + Cause: cause, + } +} + +func errRefreshHeld(until time.Time) error { + return &apierr.Error{ + Code: apierr.CodeRateLimit, + Message: fmt.Sprintf("HEY is rate-limiting token requests — not asking again for %s", time.Until(until).Round(time.Second)), + Hint: "Asking sooner cannot get through, and spends the allowance a fresh sign-in needs. Wait for the limit to clear, then run the command again", + HTTPStatus: 429, + } +} + // GetStore returns the credential store. func (m *Manager) GetStore() *Store { return m.store diff --git a/internal/auth/auth_test.go b/internal/auth/auth_test.go index 488d18c1..346746bd 100644 --- a/internal/auth/auth_test.go +++ b/internal/auth/auth_test.go @@ -14,6 +14,8 @@ import ( "strings" "testing" "time" + + "github.com/basecamp/hey-cli/internal/apierr" ) func testManager(t *testing.T, server *httptest.Server) *Manager { @@ -829,3 +831,422 @@ func TestLoginOptionsLoggerReceivesProgress(t *testing.T) { t.Errorf("stderr should stay silent when a Logger is set, got %q", captured) } } + +// The refusal HEY sends when a refresh token has been killed — by a password change, +// a revoked session, or a token already spent. +func invalidGrantHandler(calls *int) http.HandlerFunc { + return func(w http.ResponseWriter, _ *http.Request) { + *calls++ + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusBadRequest) + _, _ = fmt.Fprint(w, `{"error":"invalid_grant","error_description":"The refresh token is invalid"}`) + } +} + +func TestRefreshForgetsAGrantTheServerRefused(t *testing.T) { + calls := 0 + server := httptest.NewServer(invalidGrantHandler(&calls)) + defer server.Close() + + t.Setenv("HEY_TOKEN", "") + mgr := testManager(t, server) + if err := mgr.GetStore().Save(mgr.CredentialKey(), &Credentials{ + AccessToken: "dead-access", + RefreshToken: "dead-refresh", + ExpiresAt: time.Now().Add(-time.Hour).Unix(), + }); err != nil { + t.Fatalf("Save: %v", err) + } + + _, err := mgr.AccessToken(t.Context()) + if err == nil { + t.Fatal("AccessToken succeeded against a refused grant") + } + + var authErr *apierr.Error + if !errors.As(err, &authErr) || authErr.Code != apierr.CodeAuth { + t.Errorf("error = %v, want one coded %q so the caller stops rather than retries", err, apierr.CodeAuth) + } + if _, loadErr := mgr.GetStore().Load(mgr.CredentialKey()); loadErr == nil { + t.Error("the refused credential is still stored; every later command will re-send it") + } + if mgr.IsAuthenticated() { + t.Error("IsAuthenticated still true after the session was refused") + } +} + +// The bug itself. `hey watch` re-authenticates on every ActionCable dial, so a refresh +// token the server has already killed used to go back out every fifteen seconds for as +// long as the process lived — and the token endpoint's limit counts refusals, so the +// client spent the allowance it needed to log back in. +func TestARefusedGrantIsNeverSentTwice(t *testing.T) { + calls := 0 + server := httptest.NewServer(invalidGrantHandler(&calls)) + defer server.Close() + + t.Setenv("HEY_TOKEN", "") + mgr := testManager(t, server) + if err := mgr.GetStore().Save(mgr.CredentialKey(), &Credentials{ + AccessToken: "dead-access", + RefreshToken: "dead-refresh", + ExpiresAt: time.Now().Add(-time.Hour).Unix(), + }); err != nil { + t.Fatalf("Save: %v", err) + } + + for range 5 { + req, reqErr := http.NewRequestWithContext(t.Context(), http.MethodGet, server.URL, nil) + if reqErr != nil { + t.Fatalf("NewRequest: %v", reqErr) + } + if err := mgr.AuthenticateRequest(t.Context(), req); err == nil { + t.Fatal("AuthenticateRequest succeeded with a refused grant") + } + } + + if calls != 1 { + t.Errorf("refresh requests = %d, want 1 — the grant is dead, so only the attempt that learned that should reach the server", calls) + } +} + +func TestRefreshKeepsCredentialsTheServerNeverJudged(t *testing.T) { + tests := []struct { + name string + status int + body string + }{ + {name: "rate limited", status: http.StatusTooManyRequests, body: `{"error":"rate_limit_exceeded"}`}, + {name: "origin failure", status: http.StatusBadGateway, body: "upstream unavailable"}, + {name: "unparseable refusal", status: http.StatusBadRequest, body: "nope"}, + {name: "a different oauth error", status: http.StatusBadRequest, body: `{"error":"invalid_request"}`}, + {name: "invalid_grant from a broken origin", status: http.StatusInternalServerError, body: `{"error":"invalid_grant"}`}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(tt.status) + _, _ = fmt.Fprint(w, tt.body) + })) + defer server.Close() + + t.Setenv("HEY_TOKEN", "") + mgr := testManager(t, server) + if err := mgr.GetStore().Save(mgr.CredentialKey(), &Credentials{ + AccessToken: "access", + RefreshToken: "refresh", + ExpiresAt: time.Now().Add(-time.Hour).Unix(), + }); err != nil { + t.Fatalf("Save: %v", err) + } + + if _, err := mgr.AccessToken(t.Context()); err == nil { + t.Fatal("AccessToken succeeded against a failed refresh") + } + + stored, err := mgr.GetStore().Load(mgr.CredentialKey()) + if err != nil { + t.Fatalf("the credential was cleared on a failure that is no verdict on the grant: %v", err) + } + if stored.RefreshToken != "refresh" { + t.Errorf("stored refresh token = %q, want it left alone", stored.RefreshToken) + } + }) + } +} + +func TestRateLimitedRefreshStopsAskingUntilTheLimitCanHaveCleared(t *testing.T) { + calls := 0 + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + calls++ + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusTooManyRequests) + _, _ = fmt.Fprint(w, `{"error":"rate_limit_exceeded"}`) + })) + defer server.Close() + + t.Setenv("HEY_TOKEN", "") + mgr := testManager(t, server) + if err := mgr.GetStore().Save(mgr.CredentialKey(), &Credentials{ + AccessToken: "expired-access", + RefreshToken: "refresh", + ExpiresAt: time.Now().Add(-time.Hour).Unix(), + }); err != nil { + t.Fatalf("Save: %v", err) + } + + for range 4 { + if _, err := mgr.AccessToken(t.Context()); err == nil { + t.Fatal("AccessToken succeeded while rate limited") + } + } + + if calls != 1 { + t.Errorf("refresh requests = %d, want 1 — asking again inside the window cannot get through and only spends the allowance", calls) + } + + _, err := mgr.AccessToken(t.Context()) + var rateErr *apierr.Error + if !errors.As(err, &rateErr) || rateErr.Code != apierr.CodeRateLimit { + t.Errorf("error = %v, want one coded %q while the hold is in force", err, apierr.CodeRateLimit) + } +} + +// Being rate-limited is not being logged out. The refresh window opens five minutes +// before expiry, so a token turned away there is usually still good, and the command +// should run on it rather than fail. +func TestRateLimitedRefreshFallsBackToTheTokenItStillHolds(t *testing.T) { + calls := 0 + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + calls++ + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusTooManyRequests) + _, _ = fmt.Fprint(w, `{"error":"rate_limit_exceeded"}`) + })) + defer server.Close() + + t.Setenv("HEY_TOKEN", "") + mgr := testManager(t, server) + if err := mgr.GetStore().Save(mgr.CredentialKey(), &Credentials{ + AccessToken: "still-good", + RefreshToken: "refresh", + ExpiresAt: time.Now().Add(2 * time.Minute).Unix(), + }); err != nil { + t.Fatalf("Save: %v", err) + } + + if _, err := mgr.AccessToken(t.Context()); err == nil { + t.Fatal("the first refresh should surface the rate limit") + } + + token, err := mgr.AccessToken(t.Context()) + if err != nil { + t.Fatalf("AccessToken: %v — a rate limit is not a logout", err) + } + if token != "still-good" { + t.Errorf("token = %q, want the unexpired one already stored", token) + } + if calls != 1 { + t.Errorf("refresh requests = %d, want 1", calls) + } +} + +func TestRefreshHoldHonorsRetryAfter(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Retry-After", "42") + w.WriteHeader(http.StatusTooManyRequests) + _, _ = fmt.Fprint(w, `{"error":"rate_limit_exceeded"}`) + })) + defer server.Close() + + t.Setenv("HEY_TOKEN", "") + mgr := testManager(t, server) + if err := mgr.GetStore().Save(mgr.CredentialKey(), &Credentials{ + AccessToken: "access", + RefreshToken: "refresh", + ExpiresAt: time.Now().Add(-time.Hour).Unix(), + }); err != nil { + t.Fatalf("Save: %v", err) + } + + if _, err := mgr.AccessToken(t.Context()); err == nil { + t.Fatal("AccessToken succeeded while rate limited") + } + + held, until := mgr.refreshHeld() + if !held { + t.Fatal("no hold after a 429") + } + if wait := time.Until(until); wait > 42*time.Second || wait < 30*time.Second { + t.Errorf("hold = %s, want about the 42 seconds the server asked for", wait.Round(time.Second)) + } +} + +func TestParseRetryAfter(t *testing.T) { + tests := []struct { + name string + value string + want time.Duration + }{ + {name: "absent", value: "", want: 0}, + {name: "seconds", value: "90", want: 90 * time.Second}, + {name: "zero", value: "0", want: 0}, + {name: "negative", value: "-5", want: 0}, + {name: "nonsense", value: "soon", want: 0}, + {name: "capped", value: "86400", want: maxRetryAfter}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := parseRetryAfter(tt.value); got != tt.want { + t.Errorf("parseRetryAfter(%q) = %s, want %s", tt.value, got, tt.want) + } + }) + } +} + +func TestASuccessfulRefreshLiftsTheHold(t *testing.T) { + t.Setenv("HEY_TOKEN", "") + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "application/json") + _, _ = fmt.Fprint(w, `{"access_token":"fresh","refresh_token":"rotated","expires_in":3600}`) + })) + defer server.Close() + + mgr := testManager(t, server) + if err := mgr.GetStore().Save(mgr.CredentialKey(), &Credentials{ + AccessToken: "old", + RefreshToken: "refresh", + ExpiresAt: time.Now().Add(-time.Hour).Unix(), + }); err != nil { + t.Fatalf("Save: %v", err) + } + mgr.holdRefreshes(time.Minute) + + // The hold is in force, so this one is skipped and the expired token reported. + if _, err := mgr.AccessToken(t.Context()); err == nil { + t.Fatal("AccessToken succeeded while held") + } + + mgr.refreshHoldUntil = time.Time{} + if _, err := mgr.AccessToken(t.Context()); err != nil { + t.Fatalf("AccessToken: %v", err) + } + if held, _ := mgr.refreshHeld(); held { + t.Error("a successful refresh left the hold in place") + } +} + +// Two processes can queue on the credential lock holding the same dead grant. The +// first is refused and forgets it; the second must not go on to send its own copy, +// or the allowance is spent twice over for one dead session. +func TestRefreshStopsWhenAnotherProcessForgotTheCredential(t *testing.T) { + calls := 0 + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + calls++ + w.WriteHeader(http.StatusBadRequest) + _, _ = fmt.Fprint(w, `{"error":"invalid_grant"}`) + })) + defer server.Close() + + t.Setenv("HEY_TOKEN", "") + mgr := testManager(t, server) + + // What the losing process is still holding after the winner cleared the store. + stale := &Credentials{ + AccessToken: "dead-access", + RefreshToken: "dead-refresh", + ExpiresAt: time.Now().Add(-time.Hour).Unix(), + } + + err := mgr.refreshLocked(t.Context(), stale) + if err == nil { + t.Fatal("refreshLocked succeeded with no credential in the store") + } + var authErr *apierr.Error + if !errors.As(err, &authErr) || authErr.Code != apierr.CodeAuth { + t.Errorf("error = %v, want one coded %q", err, apierr.CodeAuth) + } + if calls != 0 { + t.Errorf("refresh requests = %d, want none — the credential was already gone", calls) + } +} + +// A 429 whose body never arrives is still a 429: the hold has to come from the +// status, not from parsing what followed it. +func TestRateLimitHoldSurvivesABodyThatNeverArrives(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Length", "64") + w.Header().Set("Retry-After", "30") + w.WriteHeader(http.StatusTooManyRequests) + // Fewer bytes than promised, then hang up: ReadAll fails. + _, _ = fmt.Fprint(w, "trunc") + if flusher, ok := w.(http.Flusher); ok { + flusher.Flush() + } + if hijacker, ok := w.(http.Hijacker); ok { + conn, _, hijackErr := hijacker.Hijack() + if hijackErr == nil { + _ = conn.Close() + } + } + })) + defer server.Close() + + t.Setenv("HEY_TOKEN", "") + mgr := testManager(t, server) + if err := mgr.GetStore().Save(mgr.CredentialKey(), &Credentials{ + AccessToken: "access", + RefreshToken: "refresh", + ExpiresAt: time.Now().Add(-time.Hour).Unix(), + }); err != nil { + t.Fatalf("Save: %v", err) + } + + if _, err := mgr.AccessToken(t.Context()); err == nil { + t.Fatal("AccessToken succeeded against a truncated 429") + } + + if held, _ := mgr.refreshHeld(); !held { + t.Error("no hold after a 429 whose body could not be read") + } + if _, err := mgr.GetStore().Load(mgr.CredentialKey()); err != nil { + t.Errorf("the credential was cleared on a rate limit: %v", err) + } +} + +func TestTheFirstRateLimitIsAlreadyClassified(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusTooManyRequests) + _, _ = fmt.Fprint(w, `{"error":"rate_limit_exceeded"}`) + })) + defer server.Close() + + t.Setenv("HEY_TOKEN", "") + mgr := testManager(t, server) + if err := mgr.GetStore().Save(mgr.CredentialKey(), &Credentials{ + AccessToken: "access", + RefreshToken: "refresh", + ExpiresAt: time.Now().Add(-time.Hour).Unix(), + }); err != nil { + t.Fatalf("Save: %v", err) + } + + _, err := mgr.AccessToken(t.Context()) + var rateErr *apierr.Error + if !errors.As(err, &rateErr) || rateErr.Code != apierr.CodeRateLimit { + t.Errorf("error = %v, want the first rate limit coded %q too", err, apierr.CodeRateLimit) + } +} + +// The exit code and the login hint have to survive the trip out: `hey watch` is +// restarted by a shell service on any non-auth exit, so an auth failure that reads +// as a generic API error is a restart loop. +func TestACredentialFailureKeepsItsAuthCode(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + t.Fatal("no request should be made without a credential") + })) + defer server.Close() + + t.Setenv("HEY_TOKEN", "") + mgr := testManager(t, server) + + req, err := http.NewRequestWithContext(t.Context(), http.MethodGet, server.URL, nil) + if err != nil { + t.Fatalf("NewRequest: %v", err) + } + + authErr := mgr.AuthenticateRequest(t.Context(), req) + if authErr == nil { + t.Fatal("AuthenticateRequest succeeded with no credential") + } + + mapped := apierr.AsError(apierr.FromSDK(authErr)) + if mapped.Code != apierr.CodeAuth { + t.Errorf("code after the SDK adapter = %q, want %q", mapped.Code, apierr.CodeAuth) + } + if mapped.Hint == "" { + t.Error("the login hint was lost on the way out") + } +} diff --git a/internal/auth/oauth.go b/internal/auth/oauth.go index 199a3710..c60de797 100644 --- a/internal/auth/oauth.go +++ b/internal/auth/oauth.go @@ -10,12 +10,110 @@ import ( "io" "net/http" "net/url" + "strconv" "strings" "time" "github.com/basecamp/hey-cli/internal/version" ) +// tokenEndpointError is a non-200 answer from the OAuth token endpoint, kept typed +// so a caller can tell the two kinds apart: the server refusing the grant we sent, +// which no retry will ever fix, and the server refusing to answer at all, which a +// later attempt may well get through. +// +// RFC 6749 §5.2 gives the refusal a machine-readable code in a JSON body. Some +// refusals carry no body worth parsing (a proxy's 502, a plain 401), and those +// leave Code empty — which is the point: an unrecognized failure is treated as +// transient, never as proof that the credential is dead. +type tokenEndpointError struct { + Op string // "token exchange" or "token refresh", for the message + StatusCode int + Code string // RFC 6749 §5.2 error code, empty when the body carried none + Description string // error_description, when the server sent one + Body string // the response body, verbatim, as the message has always shown it + RetryAfter time.Duration +} + +func (e *tokenEndpointError) Error() string { + return fmt.Sprintf("%s failed (status %d): %s", e.Op, e.StatusCode, e.Body) +} + +// grantRefused reports whether the server refused the grant itself. invalid_grant is +// the single answer RFC 6749 gives for a refresh token that is expired, revoked or +// already spent, and it is the only one that proves re-sending it can never work — +// so it is the only one we act on by forgetting the credential. Everything else, +// including a bare 4xx with no code, stays transient: the cost of being wrong the +// other way is signing someone out over a blip. +// +// The status is checked alongside the code because a 5xx that happens to echo an +// error code is an origin failing, not a grant decision. +func (e *tokenEndpointError) grantRefused() bool { + return e.Code == "invalid_grant" && e.StatusCode >= 400 && e.StatusCode < 500 +} + +// rateLimited reports whether the server declined to evaluate the grant at all. It +// says nothing about whether the credential is good, so the credential is kept. +func (e *tokenEndpointError) rateLimited() bool { + return e.StatusCode == http.StatusTooManyRequests +} + +// newTokenEndpointError reads what the refusal is willing to say. A body that is not +// the JSON of RFC 6749 §5.2 is not an error here — it just leaves Code empty, and an +// empty code is transient. +func newTokenEndpointError(op string, resp *http.Response, body []byte) *tokenEndpointError { + err := &tokenEndpointError{ + Op: op, + StatusCode: resp.StatusCode, + Body: string(body), + RetryAfter: parseRetryAfter(resp.Header.Get("Retry-After")), + } + + var payload struct { + Error string `json:"error"` + Description string `json:"error_description"` + } + if jsonErr := json.Unmarshal(body, &payload); jsonErr == nil { + err.Code = payload.Error + err.Description = payload.Description + } + return err +} + +// parseRetryAfter reads the delay-seconds and HTTP-date forms of RFC 9110 §10.2.3, +// and returns 0 for anything else. A negative or absurd value is dropped rather than +// honored: the header is the server's suggestion, not a lever to pin a client with. +func parseRetryAfter(value string) time.Duration { + value = strings.TrimSpace(value) + if value == "" { + return 0 + } + + if seconds, err := strconv.Atoi(value); err == nil { + return clampRetryAfter(time.Duration(seconds) * time.Second) + } + if when, err := http.ParseTime(value); err == nil { + return clampRetryAfter(time.Until(when)) + } + return 0 +} + +// maxRetryAfter caps how long a server may park the client. An hour is the whole +// window the token endpoint's limit is measured over, so nothing beyond it can be +// about this limit. +const maxRetryAfter = time.Hour + +func clampRetryAfter(d time.Duration) time.Duration { + switch { + case d <= 0: + return 0 + case d > maxRetryAfter: + return maxRetryAfter + default: + return d + } +} + // OAuthToken represents the token response from the HEY OAuth server. type OAuthToken struct { AccessToken string `json:"access_token"` //nolint:gosec // G117: legitimate OAuth field @@ -51,11 +149,17 @@ func exchangeCode(ctx context.Context, httpClient *http.Client, tokenEndpoint, c body, err := io.ReadAll(io.LimitReader(resp.Body, 64<<10)) if err != nil { + if resp.StatusCode != http.StatusOK { + // The status and its headers are already in hand. A body that + // truncates on the way in is no reason to lose the verdict with it — + // least of all a 429, whose whole value here is the Retry-After. + return nil, newTokenEndpointError("token exchange", resp, nil) + } return nil, fmt.Errorf("reading token response: %w", err) } if resp.StatusCode != http.StatusOK { - return nil, fmt.Errorf("token exchange failed (status %d): %s", resp.StatusCode, string(body)) + return nil, newTokenEndpointError("token exchange", resp, body) } var token OAuthToken @@ -94,11 +198,14 @@ func refreshOAuthToken(ctx context.Context, httpClient *http.Client, tokenEndpoi body, err := io.ReadAll(io.LimitReader(resp.Body, 64<<10)) if err != nil { + if resp.StatusCode != http.StatusOK { + return nil, newTokenEndpointError("token refresh", resp, nil) + } return nil, fmt.Errorf("reading refresh response: %w", err) } if resp.StatusCode != http.StatusOK { - return nil, fmt.Errorf("token refresh failed (status %d): %s", resp.StatusCode, string(body)) + return nil, newTokenEndpointError("token refresh", resp, body) } var token OAuthToken diff --git a/internal/auth/store.go b/internal/auth/store.go index 3dc03265..3e71f31f 100644 --- a/internal/auth/store.go +++ b/internal/auth/store.go @@ -100,15 +100,11 @@ func (s *Store) Delete(origin string) error { } defer unlock() - s.ensureInit() - if s.useKeyring { - return s.keyring.delete(serviceName, key(origin)) - } - return s.deleteFile(origin) + return s.delete(origin) } -// load and save are the unlocked pair, for a caller already holding the lock over a -// whole read-modify-write. Everything else goes through Load and Save. +// load, save and delete are the unlocked set, for a caller already holding the lock over +// a whole read-modify-write. Everything else goes through Load, Save and Delete. func (s *Store) load(origin string) (*Credentials, error) { s.ensureInit() if s.useKeyring { @@ -125,6 +121,14 @@ func (s *Store) save(origin string, creds *Credentials) error { return s.saveToFile(origin, creds) } +func (s *Store) delete(origin string) error { + s.ensureInit() + if s.useKeyring { + return s.keyring.delete(serviceName, key(origin)) + } + return s.deleteFile(origin) +} + func (s *Store) loadFromKeyring(origin string) (*Credentials, error) { data, err := s.keyring.get(serviceName, key(origin)) if err != nil { From 2278b6b8ef818486713fd3bc83898911a20ad9b3 Mon Sep 17 00:00:00 2001 From: Zacharias Dyna Knudsen Date: Tue, 15 Sep 2026 10:45:06 +0200 Subject: [PATCH 2/4] Keep a credential failure classified on the way out MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The CLI's auth strategy runs inside SDK calls, and the SDK returns whatever it hands back untouched. So a credential failure arrives at apierr.FromSDK already classified — but hey.AsError only recognizes the SDK's own error type, and read everything else as a generic API failure. An auth error came out of the envelope as "api" and exit 7, without the hint that says how to fix it. That matters most where the auth exit is what stops a loop. `hey watch` treats a read error as permanent only for usage and auth, and permanentReadError asked the same classifier, so a session the server had ended read as a retryable API error and the watch redialled every fifteen seconds. The Omarchy bar plugin restarts `hey watch` after any non-auth exit, so the wrong code kept it being restarted. Preserve an already-classified CLI error in both places, and report a missing or unusable credential as the auth failure it is rather than a bare error. The messages are unchanged. --- internal/apierr/sdk.go | 11 +++++++++++ internal/apierr/sdk_test.go | 33 +++++++++++++++++++++++++++++++++ internal/cmd/watch.go | 9 +++++++++ internal/cmd/watch_test.go | 31 +++++++++++++++++++++++++++++++ 4 files changed, 84 insertions(+) diff --git a/internal/apierr/sdk.go b/internal/apierr/sdk.go index 5b1b0081..fa209e90 100644 --- a/internal/apierr/sdk.go +++ b/internal/apierr/sdk.go @@ -1,6 +1,7 @@ package apierr import ( + "errors" "fmt" hey "github.com/basecamp/hey-sdk/go/pkg/hey" @@ -15,6 +16,16 @@ func FromSDK(err error) error { return nil } + // Not every error that comes back from an SDK call was made by the SDK. The + // auth strategy is ours, and the SDK returns what it hands back untouched, so + // a credential failure arrives here already classified. hey.AsError only + // recognizes the SDK's own type and would flatten it to "api" — losing the + // auth exit code and the hint that says how to fix it. + var cliErr *Error + if errors.As(err, &cliErr) { + return cliErr + } + sdkErr := hey.AsError(err) switch sdkErr.Code { case hey.CodeAuth: diff --git a/internal/apierr/sdk_test.go b/internal/apierr/sdk_test.go index f96f5bbe..b4b14f95 100644 --- a/internal/apierr/sdk_test.go +++ b/internal/apierr/sdk_test.go @@ -2,6 +2,7 @@ package apierr import ( "errors" + "fmt" "testing" hey "github.com/basecamp/hey-sdk/go/pkg/hey" @@ -132,3 +133,35 @@ func TestFromSDKKeepsTheCauseReachable(t *testing.T) { t.Error("the SDK error a validation failure came from must stay reachable through errors.Is") } } + +// The CLI's own auth strategy runs inside SDK calls, and the SDK hands back what it +// returns untouched. Such an error arrives here already classified; flattening it to +// "api" would cost the auth exit code and the hint that says how to fix it. +func TestFromSDKKeepsAnAlreadyClassifiedError(t *testing.T) { + tests := []struct { + name string + err error + }{ + {name: "bare", err: ErrAuth("not authenticated")}, + {name: "wrapped", err: fmt.Errorf("reading changes: %w", ErrAuth("not authenticated"))}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := AsError(FromSDK(tt.err)) + if got.Code != CodeAuth { + t.Errorf("code = %q, want %q", got.Code, CodeAuth) + } + if got.Hint == "" { + t.Error("the login hint was dropped") + } + }) + } +} + +func TestFromSDKStillMapsRateLimitedCLIErrors(t *testing.T) { + got := AsError(FromSDK(&Error{Code: CodeRateLimit, Message: "rate limited", HTTPStatus: 429})) + if got.Code != CodeRateLimit { + t.Errorf("code = %q, want %q", got.Code, CodeRateLimit) + } +} diff --git a/internal/cmd/watch.go b/internal/cmd/watch.go index cc4f005c..37855d21 100644 --- a/internal/cmd/watch.go +++ b/internal/cmd/watch.go @@ -706,6 +706,15 @@ func (w *postingsWatch) classify(box *watchedBox, posting generated.Posting) *bo // cursor or credentials the server won't take doesn't get better by waiting two minutes, // and a watch that retried it silently would sit there for hours and still exit 0. func permanentReadError(err error) bool { + // A credential failure comes from our own auth strategy, which the SDK passes + // back untouched, so it is already classified and hey.AsError would read it as + // a generic API error. A watch that retried a session the server has ended + // would redial every fifteen seconds for as long as it was left running. + var cliErr *apierr.Error + if errors.As(err, &cliErr) { + return cliErr.Code == apierr.CodeUsage || cliErr.Code == apierr.CodeAuth + } + switch hey.AsError(err).Code { case hey.CodeUsage, hey.CodeAuth: return true diff --git a/internal/cmd/watch_test.go b/internal/cmd/watch_test.go index 8fab476b..5b4412f6 100644 --- a/internal/cmd/watch_test.go +++ b/internal/cmd/watch_test.go @@ -17,7 +17,9 @@ import ( actioncable "github.com/basecamp/actioncable-go" "github.com/basecamp/hey-sdk/go/pkg/generated" + hey "github.com/basecamp/hey-sdk/go/pkg/hey" + "github.com/basecamp/hey-cli/internal/apierr" "github.com/basecamp/hey-cli/internal/auth" ) @@ -1027,3 +1029,32 @@ func TestWatchLineDescribesTheWatchsOwnNews(t *testing.T) { t.Errorf("line = %q, want ready described without a box", line) } } + +// A watch whose credentials the server has ended must stop, not redial. The auth +// failure comes from the CLI's own auth strategy, which the SDK returns untouched, +// so it is not a *hey.Error and the SDK's classifier reads it as a generic API +// error — the kind this retries every two minutes, for as long as the shell service +// keeps restarting it. +func TestPermanentReadErrorRecognizesACLIAuthFailure(t *testing.T) { + tests := []struct { + name string + err error + want bool + }{ + {name: "cli auth", err: apierr.ErrAuth("not authenticated"), want: true}, + {name: "cli auth wrapped", err: fmt.Errorf("reading changes: %w", apierr.ErrAuth("not authenticated")), want: true}, + {name: "cli usage", err: apierr.ErrUsage("bad cursor"), want: true}, + {name: "cli rate limit", err: apierr.ErrRateLimit(30), want: false}, + {name: "cli network", err: apierr.ErrNetwork(io.EOF), want: false}, + {name: "sdk auth", err: &hey.Error{Code: hey.CodeAuth, Message: "not authenticated"}, want: true}, + {name: "sdk server error", err: &hey.Error{Code: hey.CodeAPI, Message: "boom"}, want: false}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := permanentReadError(tt.err); got != tt.want { + t.Errorf("permanentReadError(%v) = %t, want %t", tt.err, got, tt.want) + } + }) + } +} From 8527c5756f874ed6392f8a4b3d994ed94c770f95 Mon Sep 17 00:00:00 2001 From: Zacharias Dyna Knudsen Date: Tue, 15 Sep 2026 12:15:38 +0200 Subject: [PATCH 3/4] Tidy the refusal errors, and remember one we could not delete MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review follow-up. errNotAuthenticated builds the message from its cause, so the four call sites that were each spelling out the same Sprintf no longer do. errRefreshHeld returns *apierr.Error like the others rather than a bare error. errRefusedGrant is the one place the invalid_grant sentence is written, with or without a cleanup failure to report. tokenEndpointError drops Description: nothing read it, and the server's error_description already reaches the reader through Body, which the message prints verbatim. Removing the field also removes an accident — a non-string error_description used to fail the whole unmarshal, leaving the code unread and a dead grant treated as transient. The verdict is the error code; what the server puts beside it does not change whether the grant is dead, and a test now pins that for a string, a number, an object and nothing at all. Deleting the credential is what makes a refused grant stop being sent. When the store will not let go of it the credential is still there for the next command to load, so the refused token is now remembered for the life of the process and not sent again. That replaces the rate-limit hold the failed delete used to borrow, which only postponed the resend by fifteen minutes. The transient-failure cases move into TestRefreshFailuresPreserveCredentials rather than sitting in a table of their own. Every status and body is still covered, the credential assertion now checks both tokens, and the rate-limited case additionally pins its classification. --- internal/auth/auth.go | 138 ++++++++---------- internal/auth/auth_test.go | 279 +++++++++++++++--------------------- internal/auth/oauth.go | 92 ++++-------- internal/auth/oauth_test.go | 4 +- 4 files changed, 206 insertions(+), 307 deletions(-) diff --git a/internal/auth/auth.go b/internal/auth/auth.go index fb027189..9ad00249 100644 --- a/internal/auth/auth.go +++ b/internal/auth/auth.go @@ -35,18 +35,18 @@ type Manager struct { listen listenerFactory mu sync.Mutex - // refreshHoldUntil is when this process will next send a refresh, after the - // token endpoint rate-limited one. Guarded by mu, which every path into - // refreshLocked already holds. + // refreshHoldUntil parks refreshes after the token endpoint rate-limited one. + // Guarded by mu, which every path into refreshLocked already holds. refreshHoldUntil time.Time + + // refusedRefreshToken is a grant the server refused that the store could not + // delete, so this process remembers not to send it again. Guarded by mu. + refusedRefreshToken string } -// defaultRefreshHold is how long to sit out a rate limit the server did not put a -// Retry-After on. The token endpoint's limit is a fixed window an hour wide, so a -// process pausing this long spends only a few of the allowance per window however -// long it runs — where `hey watch`, which re-authenticates on every ActionCable -// dial and redials on a fifteen-second timeout, spends all of it in minutes and -// then keeps it spent. +// defaultRefreshHold is how long to sit out a rate limit that came without a +// Retry-After. The token endpoint's window is an hour and it counts refusals, so a +// `hey watch` redialling every fifteen seconds would spend it in minutes. const defaultRefreshHold = 15 * time.Minute // NewManager creates a new auth manager. @@ -77,7 +77,7 @@ func (m *Manager) AccessToken(ctx context.Context) (string, error) { creds, err := m.store.Load(m.baseURL) if err != nil { - return "", errNoCredential(fmt.Sprintf("not authenticated: %v", err), err) + return "", errNotAuthenticated(err) } // Check if token is expired (with 5-minute buffer) @@ -116,7 +116,7 @@ func (m *Manager) AuthenticateRequest(ctx context.Context, req *http.Request) er creds, err := m.store.Load(m.baseURL) if err != nil { - return errNoCredential(fmt.Sprintf("not authenticated: %v", err), err) + return errNotAuthenticated(err) } if creds.AccessToken != "" { @@ -273,7 +273,7 @@ func (m *Manager) Refresh(ctx context.Context) error { creds, err := m.store.Load(m.baseURL) if err != nil { - return errNoCredential(fmt.Sprintf("not authenticated: %v", err), err) + return errNotAuthenticated(err) } // Cookie-based auth doesn't support refresh; treat as no-op. @@ -298,12 +298,9 @@ func (m *Manager) refreshLocked(ctx context.Context, creds *Credentials) error { stored, loadErr := m.store.load(m.baseURL) if loadErr != nil { - // The credential went away while we waited for the lock, which is what - // happens when another process just had this same grant refused and - // forgot it. Falling back to the copy we came in with would send a token - // already known to be dead, and spend one more of an allowance that is - // counted per address and shared by every process here. - return errNoCredential(fmt.Sprintf("not authenticated: %v", loadErr), loadErr) + // Another process had this same grant refused and forgot it while we + // waited for the lock. The copy we came in with is the same dead token. + return errNotAuthenticated(loadErr) } if stored.AccessToken != "" && stored.AccessToken != creds.AccessToken { return nil @@ -314,14 +311,16 @@ func (m *Manager) refreshLocked(ctx context.Context, creds *Credentials) error { return fmt.Errorf("no refresh token available") } + if m.refusedRefreshToken != "" && creds.RefreshToken == m.refusedRefreshToken { + // Refused once already, and the store would not let go of it. + return errRefusedGrant(nil) + } + if held, until := m.refreshHeld(); held { - // The endpoint rate-limited us recently. Its limit counts refusals as - // well as successes, so asking again now cannot get through and only - // spends the allowance a fresh sign-in will need. + // Asking again inside the rate limit cannot get through. A token that has + // not actually expired is still good, so carry on with it: the SDK's own + // retry after a 401 costs one request at most. if creds.ExpiresAt > time.Now().Unix() { - // The five-minute buffer opened this refresh early and the access - // token has not actually expired yet, so the command carries on with - // the one we hold and tries again later. return nil } return errRefreshHeld(until) @@ -339,7 +338,7 @@ func (m *Manager) refreshLocked(ctx context.Context, creds *Credentials) error { token, err := refreshOAuthToken(ctx, m.httpClient, tokenEndpoint, creds.RefreshToken, oauthClientID, installID) if err != nil { - return m.accountForRefreshFailure(err) + return m.accountForRefreshFailure(err, creds.RefreshToken) } m.refreshHoldUntil = time.Time{} // A 200 without an access token is not a refresh. Storing the empty string would @@ -362,26 +361,16 @@ func (m *Manager) refreshLocked(ctx context.Context, creds *Credentials) error { } // accountForRefreshFailure decides what a failed refresh costs the stored credential. -// Only the server refusing the grant itself clears it; every other failure leaves it -// exactly where it was. -// -// Getting this split wrong is expensive in both directions. Keeping a refused grant is -// the bug this replaces: the credential stayed on disk, so every later command re-sent -// a token the server had already killed, and because the token endpoint's limit counts -// refusals as well as successes, a background `hey watch` could spend an hour's -// allowance in minutes and go on spending it — locking the address out of the login it -// needed to recover. Clearing on anything softer is the opposite failure: a flaky -// network or a passing 502 would sign people out of a session that was fine. -func (m *Manager) accountForRefreshFailure(err error) error { +// Only invalid_grant clears it: that is the one answer that proves re-sending can never +// work. Anything softer keeps it, or a passing 502 would sign people out. +func (m *Manager) accountForRefreshFailure(err error, sentRefreshToken string) error { var refusal *tokenEndpointError if !errors.As(err, &refusal) { - // A transport failure — no answer from the server at all, so no verdict on - // the grant. + // No answer from the server, so no verdict on the grant. return fmt.Errorf("token refresh failed: %w", err) } if refusal.rateLimited() { - // Says nothing about the credential: the server declined to look at it. m.holdRefreshes(refusal.RetryAfter) return errRefreshHeld(m.refreshHoldUntil) } @@ -390,25 +379,15 @@ func (m *Manager) accountForRefreshFailure(err error) error { return fmt.Errorf("token refresh failed: %w", err) } - // invalid_grant. The refresh token is expired, revoked or already spent, and it - // will be refused the same way forever. Forget it here, under the lock that - // already spans this load-refresh-save, so the next command asks for a login - // instead of re-sending it. + // Forget it under the lock that already spans this load-refresh-save, so the + // next command asks for a login instead of re-sending it. if delErr := m.store.delete(m.baseURL); delErr != nil { - // The grant is dead whether or not the store would let go of it, and the - // credential is still on disk for the next command to find. Hold refreshes - // so a cleanup that failed cannot become the same hammering by another - // name, and still report it as the auth failure it is. - m.holdRefreshes(0) - return &apierr.Error{ - Code: apierr.CodeAuth, - Message: fmt.Sprintf("HEY refused the stored refresh token: the session has expired or was revoked. The stored credentials could not be cleared: %v", delErr), - Hint: "Run: hey auth logout, then: hey auth login", - HTTPStatus: 401, - Cause: delErr, - } + // The credential is still on disk for the next command to load, so the + // refusal is remembered here instead. + m.refusedRefreshToken = sentRefreshToken + return errRefusedGrant(delErr) } - return apierr.ErrAuth("HEY refused the stored refresh token: the session has expired or was revoked. Changing your HEY password ends every session, including this one. The stored credentials have been cleared") + return errRefusedGrant(nil) } // refreshHeld reports whether this process is sitting out a rate limit, and until when. @@ -420,8 +399,7 @@ func (m *Manager) refreshHeld() (bool, time.Time) { } // holdRefreshes parks refreshes until the rate limit has had time to clear, honoring -// the server's Retry-After when it sends one. The hold only ever moves later, so a -// short hint cannot shorten a longer wait already in force. +// the server's Retry-After when it sends one. The hold only ever moves later. func (m *Manager) holdRefreshes(retryAfter time.Duration) { if retryAfter <= 0 { retryAfter = defaultRefreshHold @@ -431,28 +409,36 @@ func (m *Manager) holdRefreshes(retryAfter time.Duration) { } } -// errNoCredential reports a missing or unusable credential. It carries the auth code -// so the exit status and the login hint survive the trip out through the SDK: the auth -// strategy is ours and the SDK hands our errors back untouched, so an unclassified one -// would reach the envelope as a generic API failure — exiting 7 where a script, and the -// service that restarts `hey watch`, are both watching for the auth exit. +// errNoCredential is an auth failure with the store's reason attached. It has to be +// classified here: the SDK hands our errors back untouched, so an unclassified one +// would reach the envelope as a generic API failure without the login hint. func errNoCredential(msg string, cause error) *apierr.Error { - return &apierr.Error{ - Code: apierr.CodeAuth, - Message: msg, - Hint: "Run: hey auth login", - HTTPStatus: 401, - Cause: cause, - } + err := apierr.ErrAuth(msg) + err.Cause = cause + return err } -func errRefreshHeld(until time.Time) error { - return &apierr.Error{ - Code: apierr.CodeRateLimit, - Message: fmt.Sprintf("HEY is rate-limiting token requests — not asking again for %s", time.Until(until).Round(time.Second)), - Hint: "Asking sooner cannot get through, and spends the allowance a fresh sign-in needs. Wait for the limit to clear, then run the command again", - HTTPStatus: 429, +func errNotAuthenticated(cause error) *apierr.Error { + return errNoCredential(fmt.Sprintf("not authenticated: %v", cause), cause) +} + +// errRefusedGrant reports a refresh token the server answered invalid_grant for. +// cleanupErr is the failure to clear it from the store, when there was one. +func errRefusedGrant(cleanupErr error) *apierr.Error { + if cleanupErr != nil { + err := errNoCredential(fmt.Sprintf("HEY refused the stored refresh token: the session has expired or was revoked. The stored credentials could not be cleared: %v", cleanupErr), cleanupErr) + err.Hint = "Run: hey auth logout, then: hey auth login" + return err } + return apierr.ErrAuth("HEY refused the stored refresh token: the session has expired or was revoked. Changing your HEY password ends every session, including this one. The stored credentials have been cleared") +} + +func errRefreshHeld(until time.Time) *apierr.Error { + wait := time.Until(until).Round(time.Second) + err := apierr.ErrRateLimit(int(wait / time.Second)) + err.Message = fmt.Sprintf("HEY is rate-limiting token requests — not asking again for %s", wait) + err.Hint = "Wait for the limit to clear, then run the command again" + return err } // GetStore returns the credential store. diff --git a/internal/auth/auth_test.go b/internal/auth/auth_test.go index 346746bd..326d8023 100644 --- a/internal/auth/auth_test.go +++ b/internal/auth/auth_test.go @@ -441,6 +441,10 @@ func TestMissingCredentialsDoNotModifyRequest(t *testing.T) { if err == nil || !strings.Contains(err.Error(), tt.want) { t.Fatalf("error = %v, want substring %q", err, tt.want) } + var authErr *apierr.Error + if !errors.As(err, &authErr) || authErr.Code != apierr.CodeAuth { + t.Errorf("error = %v, want one coded %q so the exit code and login hint survive the SDK", err, apierr.CodeAuth) + } if got := req.Header.Get("Authorization"); got != "original" { t.Errorf("Authorization = %q, want original header preserved", got) } @@ -572,16 +576,29 @@ func TestRefreshUsesStoredEndpointAndRotatesToken(t *testing.T) { } func TestRefreshFailuresPreserveCredentials(t *testing.T) { + // Every refusal that is not the server's verdict on the grant itself. The + // credential has to come through each of them untouched: signing someone out + // over a blip is worse than the resend this fix exists to stop. + refusable := func() *Credentials { + return &Credentials{AccessToken: "access", RefreshToken: "refresh"} + } + tests := []struct { name string creds *Credentials status int + body string want string wantCalls int }{ {name: "not authenticated", want: "not authenticated", wantCalls: 0}, {name: "no refresh token", creds: &Credentials{AccessToken: "access"}, want: "no refresh token", wantCalls: 0}, - {name: "server failure", creds: &Credentials{AccessToken: "access", RefreshToken: "refresh"}, status: http.StatusUnauthorized, want: "token refresh failed", wantCalls: 1}, + {name: "server failure", creds: refusable(), status: http.StatusUnauthorized, body: "denied", want: "token refresh failed", wantCalls: 1}, + {name: "rate limited", creds: refusable(), status: http.StatusTooManyRequests, body: `{"error":"rate_limit_exceeded"}`, want: "rate-limiting", wantCalls: 1}, + {name: "origin failure", creds: refusable(), status: http.StatusBadGateway, body: "upstream unavailable", want: "token refresh failed", wantCalls: 1}, + {name: "unparseable refusal", creds: refusable(), status: http.StatusBadRequest, body: "nope", want: "token refresh failed", wantCalls: 1}, + {name: "a different oauth error", creds: refusable(), status: http.StatusBadRequest, body: `{"error":"invalid_request"}`, want: "token refresh failed", wantCalls: 1}, + {name: "invalid_grant from a broken origin", creds: refusable(), status: http.StatusInternalServerError, body: `{"error":"invalid_grant"}`, want: "token refresh failed", wantCalls: 1}, } for _, tt := range tests { @@ -590,7 +607,7 @@ func TestRefreshFailuresPreserveCredentials(t *testing.T) { server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { calls++ w.WriteHeader(tt.status) - _, _ = fmt.Fprint(w, "denied") + _, _ = fmt.Fprint(w, tt.body) })) defer server.Close() @@ -832,6 +849,19 @@ func TestLoginOptionsLoggerReceivesProgress(t *testing.T) { } } +// saveExpiredCredential stores a credential whose access token has run out, so the +// next use has to refresh. +func saveExpiredCredential(t *testing.T, mgr *Manager) { + t.Helper() + if err := mgr.GetStore().Save(mgr.CredentialKey(), &Credentials{ + AccessToken: "expired-access", + RefreshToken: "refresh", + ExpiresAt: time.Now().Add(-time.Hour).Unix(), + }); err != nil { + t.Fatalf("Save: %v", err) + } +} + // The refusal HEY sends when a refresh token has been killed — by a password change, // a revoked session, or a token already spent. func invalidGrantHandler(calls *int) http.HandlerFunc { @@ -844,34 +874,86 @@ func invalidGrantHandler(calls *int) http.HandlerFunc { } func TestRefreshForgetsAGrantTheServerRefused(t *testing.T) { + // The verdict is the error code. What the server puts alongside it — a + // description, a malformed one, or nothing — does not change whether the grant + // is dead, so none of these may change whether the credential is cleared. + bodies := map[string]string{ + "with a description": `{"error":"invalid_grant","error_description":"The refresh token is invalid"}`, + "bare": `{"error":"invalid_grant"}`, + "malformed description": `{"error":"invalid_grant","error_description":123}`, + "description as an object": `{"error":"invalid_grant","error_description":{"detail":"gone"}}`, + } + + for name, body := range bodies { + t.Run(name, func(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusBadRequest) + _, _ = fmt.Fprint(w, body) + })) + defer server.Close() + + t.Setenv("HEY_TOKEN", "") + mgr := testManager(t, server) + saveExpiredCredential(t, mgr) + + _, err := mgr.AccessToken(t.Context()) + if err == nil { + t.Fatal("AccessToken succeeded against a refused grant") + } + + var authErr *apierr.Error + if !errors.As(err, &authErr) || authErr.Code != apierr.CodeAuth { + t.Errorf("error = %v, want one coded %q so the caller stops rather than retries", err, apierr.CodeAuth) + } + if _, loadErr := mgr.GetStore().Load(mgr.CredentialKey()); loadErr == nil { + t.Error("the refused credential is still stored; every later command will re-send it") + } + if mgr.IsAuthenticated() { + t.Error("IsAuthenticated still true after the session was refused") + } + }) + } +} + +// Deleting the credential is what normally stops a refused grant being sent again. +// When the store will not let go of it, the refusal has to be remembered instead, or +// the next command loads the same dead token and spends another attempt on it. +func TestARefusedGrantIsNotResentWhenItCannotBeDeleted(t *testing.T) { calls := 0 server := httptest.NewServer(invalidGrantHandler(&calls)) defer server.Close() t.Setenv("HEY_TOKEN", "") - mgr := testManager(t, server) - if err := mgr.GetStore().Save(mgr.CredentialKey(), &Credentials{ - AccessToken: "dead-access", - RefreshToken: "dead-refresh", - ExpiresAt: time.Now().Add(-time.Hour).Unix(), - }); err != nil { - t.Fatalf("Save: %v", err) + t.Setenv("HEY_NO_KEYRING", "") + mgr := NewManager(server.URL, server.Client(), t.TempDir()) + + // A keyring that stores and reads but refuses to delete. + stored := "" + mgr.GetStore().useKeyring = true + mgr.GetStore().initOnce.Do(func() {}) + mgr.GetStore().keyring = credentialKeyring{ + set: func(_, _, password string) error { stored = password; return nil }, + get: func(_, _ string) (string, error) { return stored, nil }, + delete: func(_, _ string) error { return errors.New("keyring is locked") }, } - _, err := mgr.AccessToken(t.Context()) - if err == nil { - t.Fatal("AccessToken succeeded against a refused grant") + saveExpiredCredential(t, mgr) + + for range 4 { + if _, err := mgr.AccessToken(t.Context()); err == nil { + t.Fatal("AccessToken succeeded with a refused grant") + } } + if calls != 1 { + t.Errorf("refresh requests = %d, want 1 — the refusal has to outlive a delete that failed", calls) + } + + _, err := mgr.AccessToken(t.Context()) var authErr *apierr.Error if !errors.As(err, &authErr) || authErr.Code != apierr.CodeAuth { - t.Errorf("error = %v, want one coded %q so the caller stops rather than retries", err, apierr.CodeAuth) - } - if _, loadErr := mgr.GetStore().Load(mgr.CredentialKey()); loadErr == nil { - t.Error("the refused credential is still stored; every later command will re-send it") - } - if mgr.IsAuthenticated() { - t.Error("IsAuthenticated still true after the session was refused") + t.Errorf("error = %v, want one coded %q", err, apierr.CodeAuth) } } @@ -886,13 +968,7 @@ func TestARefusedGrantIsNeverSentTwice(t *testing.T) { t.Setenv("HEY_TOKEN", "") mgr := testManager(t, server) - if err := mgr.GetStore().Save(mgr.CredentialKey(), &Credentials{ - AccessToken: "dead-access", - RefreshToken: "dead-refresh", - ExpiresAt: time.Now().Add(-time.Hour).Unix(), - }); err != nil { - t.Fatalf("Save: %v", err) - } + saveExpiredCredential(t, mgr) for range 5 { req, reqErr := http.NewRequestWithContext(t.Context(), http.MethodGet, server.URL, nil) @@ -909,53 +985,6 @@ func TestARefusedGrantIsNeverSentTwice(t *testing.T) { } } -func TestRefreshKeepsCredentialsTheServerNeverJudged(t *testing.T) { - tests := []struct { - name string - status int - body string - }{ - {name: "rate limited", status: http.StatusTooManyRequests, body: `{"error":"rate_limit_exceeded"}`}, - {name: "origin failure", status: http.StatusBadGateway, body: "upstream unavailable"}, - {name: "unparseable refusal", status: http.StatusBadRequest, body: "nope"}, - {name: "a different oauth error", status: http.StatusBadRequest, body: `{"error":"invalid_request"}`}, - {name: "invalid_grant from a broken origin", status: http.StatusInternalServerError, body: `{"error":"invalid_grant"}`}, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { - w.Header().Set("Content-Type", "application/json") - w.WriteHeader(tt.status) - _, _ = fmt.Fprint(w, tt.body) - })) - defer server.Close() - - t.Setenv("HEY_TOKEN", "") - mgr := testManager(t, server) - if err := mgr.GetStore().Save(mgr.CredentialKey(), &Credentials{ - AccessToken: "access", - RefreshToken: "refresh", - ExpiresAt: time.Now().Add(-time.Hour).Unix(), - }); err != nil { - t.Fatalf("Save: %v", err) - } - - if _, err := mgr.AccessToken(t.Context()); err == nil { - t.Fatal("AccessToken succeeded against a failed refresh") - } - - stored, err := mgr.GetStore().Load(mgr.CredentialKey()) - if err != nil { - t.Fatalf("the credential was cleared on a failure that is no verdict on the grant: %v", err) - } - if stored.RefreshToken != "refresh" { - t.Errorf("stored refresh token = %q, want it left alone", stored.RefreshToken) - } - }) - } -} - func TestRateLimitedRefreshStopsAskingUntilTheLimitCanHaveCleared(t *testing.T) { calls := 0 server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { @@ -968,28 +997,23 @@ func TestRateLimitedRefreshStopsAskingUntilTheLimitCanHaveCleared(t *testing.T) t.Setenv("HEY_TOKEN", "") mgr := testManager(t, server) - if err := mgr.GetStore().Save(mgr.CredentialKey(), &Credentials{ - AccessToken: "expired-access", - RefreshToken: "refresh", - ExpiresAt: time.Now().Add(-time.Hour).Unix(), - }); err != nil { - t.Fatalf("Save: %v", err) + saveExpiredCredential(t, mgr) + + _, err := mgr.AccessToken(t.Context()) + var rateErr *apierr.Error + if !errors.As(err, &rateErr) || rateErr.Code != apierr.CodeRateLimit { + t.Fatalf("error = %v, want the first rate limit coded %q", err, apierr.CodeRateLimit) } for range 4 { - if _, err := mgr.AccessToken(t.Context()); err == nil { - t.Fatal("AccessToken succeeded while rate limited") + _, err = mgr.AccessToken(t.Context()) + if !errors.As(err, &rateErr) || rateErr.Code != apierr.CodeRateLimit { + t.Fatalf("error = %v, want one coded %q while the hold is in force", err, apierr.CodeRateLimit) } } if calls != 1 { - t.Errorf("refresh requests = %d, want 1 — asking again inside the window cannot get through and only spends the allowance", calls) - } - - _, err := mgr.AccessToken(t.Context()) - var rateErr *apierr.Error - if !errors.As(err, &rateErr) || rateErr.Code != apierr.CodeRateLimit { - t.Errorf("error = %v, want one coded %q while the hold is in force", err, apierr.CodeRateLimit) + t.Errorf("refresh requests = %d, want 1 — asking again inside the window cannot get through", calls) } } @@ -1042,13 +1066,7 @@ func TestRefreshHoldHonorsRetryAfter(t *testing.T) { t.Setenv("HEY_TOKEN", "") mgr := testManager(t, server) - if err := mgr.GetStore().Save(mgr.CredentialKey(), &Credentials{ - AccessToken: "access", - RefreshToken: "refresh", - ExpiresAt: time.Now().Add(-time.Hour).Unix(), - }); err != nil { - t.Fatalf("Save: %v", err) - } + saveExpiredCredential(t, mgr) if _, err := mgr.AccessToken(t.Context()); err == nil { t.Fatal("AccessToken succeeded while rate limited") @@ -1095,13 +1113,7 @@ func TestASuccessfulRefreshLiftsTheHold(t *testing.T) { defer server.Close() mgr := testManager(t, server) - if err := mgr.GetStore().Save(mgr.CredentialKey(), &Credentials{ - AccessToken: "old", - RefreshToken: "refresh", - ExpiresAt: time.Now().Add(-time.Hour).Unix(), - }); err != nil { - t.Fatalf("Save: %v", err) - } + saveExpiredCredential(t, mgr) mgr.holdRefreshes(time.Minute) // The hold is in force, so this one is skipped and the expired token reported. @@ -1176,13 +1188,7 @@ func TestRateLimitHoldSurvivesABodyThatNeverArrives(t *testing.T) { t.Setenv("HEY_TOKEN", "") mgr := testManager(t, server) - if err := mgr.GetStore().Save(mgr.CredentialKey(), &Credentials{ - AccessToken: "access", - RefreshToken: "refresh", - ExpiresAt: time.Now().Add(-time.Hour).Unix(), - }); err != nil { - t.Fatalf("Save: %v", err) - } + saveExpiredCredential(t, mgr) if _, err := mgr.AccessToken(t.Context()); err == nil { t.Fatal("AccessToken succeeded against a truncated 429") @@ -1195,58 +1201,3 @@ func TestRateLimitHoldSurvivesABodyThatNeverArrives(t *testing.T) { t.Errorf("the credential was cleared on a rate limit: %v", err) } } - -func TestTheFirstRateLimitIsAlreadyClassified(t *testing.T) { - server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { - w.WriteHeader(http.StatusTooManyRequests) - _, _ = fmt.Fprint(w, `{"error":"rate_limit_exceeded"}`) - })) - defer server.Close() - - t.Setenv("HEY_TOKEN", "") - mgr := testManager(t, server) - if err := mgr.GetStore().Save(mgr.CredentialKey(), &Credentials{ - AccessToken: "access", - RefreshToken: "refresh", - ExpiresAt: time.Now().Add(-time.Hour).Unix(), - }); err != nil { - t.Fatalf("Save: %v", err) - } - - _, err := mgr.AccessToken(t.Context()) - var rateErr *apierr.Error - if !errors.As(err, &rateErr) || rateErr.Code != apierr.CodeRateLimit { - t.Errorf("error = %v, want the first rate limit coded %q too", err, apierr.CodeRateLimit) - } -} - -// The exit code and the login hint have to survive the trip out: `hey watch` is -// restarted by a shell service on any non-auth exit, so an auth failure that reads -// as a generic API error is a restart loop. -func TestACredentialFailureKeepsItsAuthCode(t *testing.T) { - server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { - t.Fatal("no request should be made without a credential") - })) - defer server.Close() - - t.Setenv("HEY_TOKEN", "") - mgr := testManager(t, server) - - req, err := http.NewRequestWithContext(t.Context(), http.MethodGet, server.URL, nil) - if err != nil { - t.Fatalf("NewRequest: %v", err) - } - - authErr := mgr.AuthenticateRequest(t.Context(), req) - if authErr == nil { - t.Fatal("AuthenticateRequest succeeded with no credential") - } - - mapped := apierr.AsError(apierr.FromSDK(authErr)) - if mapped.Code != apierr.CodeAuth { - t.Errorf("code after the SDK adapter = %q, want %q", mapped.Code, apierr.CodeAuth) - } - if mapped.Hint == "" { - t.Error("the login hint was lost on the way out") - } -} diff --git a/internal/auth/oauth.go b/internal/auth/oauth.go index c60de797..e307273c 100644 --- a/internal/auth/oauth.go +++ b/internal/auth/oauth.go @@ -17,22 +17,16 @@ import ( "github.com/basecamp/hey-cli/internal/version" ) -// tokenEndpointError is a non-200 answer from the OAuth token endpoint, kept typed -// so a caller can tell the two kinds apart: the server refusing the grant we sent, -// which no retry will ever fix, and the server refusing to answer at all, which a -// later attempt may well get through. -// -// RFC 6749 §5.2 gives the refusal a machine-readable code in a JSON body. Some -// refusals carry no body worth parsing (a proxy's 502, a plain 401), and those -// leave Code empty — which is the point: an unrecognized failure is treated as -// transient, never as proof that the credential is dead. +// tokenEndpointError is a non-200 answer from the OAuth token endpoint, typed so a +// caller can tell a dead grant from a server that would not answer. Code is the RFC +// 6749 §5.2 error code, and stays empty when the body carried none: an unrecognized +// failure is transient, never proof that the credential is dead. type tokenEndpointError struct { - Op string // "token exchange" or "token refresh", for the message - StatusCode int - Code string // RFC 6749 §5.2 error code, empty when the body carried none - Description string // error_description, when the server sent one - Body string // the response body, verbatim, as the message has always shown it - RetryAfter time.Duration + Op string // "token exchange" or "token refresh", for the message + StatusCode int + Code string + Body string // verbatim, as the message has always shown it + RetryAfter time.Duration } func (e *tokenEndpointError) Error() string { @@ -40,27 +34,17 @@ func (e *tokenEndpointError) Error() string { } // grantRefused reports whether the server refused the grant itself. invalid_grant is -// the single answer RFC 6749 gives for a refresh token that is expired, revoked or -// already spent, and it is the only one that proves re-sending it can never work — -// so it is the only one we act on by forgetting the credential. Everything else, -// including a bare 4xx with no code, stays transient: the cost of being wrong the -// other way is signing someone out over a blip. -// -// The status is checked alongside the code because a 5xx that happens to echo an -// error code is an origin failing, not a grant decision. +// the one answer RFC 6749 gives for a refresh token that is expired, revoked or spent. +// The status is checked too: a 5xx that echoes the code is an origin failing. func (e *tokenEndpointError) grantRefused() bool { return e.Code == "invalid_grant" && e.StatusCode >= 400 && e.StatusCode < 500 } -// rateLimited reports whether the server declined to evaluate the grant at all. It -// says nothing about whether the credential is good, so the credential is kept. +// rateLimited reports whether the server declined to look at the grant at all. func (e *tokenEndpointError) rateLimited() bool { return e.StatusCode == http.StatusTooManyRequests } -// newTokenEndpointError reads what the refusal is willing to say. A body that is not -// the JSON of RFC 6749 §5.2 is not an error here — it just leaves Code empty, and an -// empty code is transient. func newTokenEndpointError(op string, resp *http.Response, body []byte) *tokenEndpointError { err := &tokenEndpointError{ Op: op, @@ -70,12 +54,10 @@ func newTokenEndpointError(op string, resp *http.Response, body []byte) *tokenEn } var payload struct { - Error string `json:"error"` - Description string `json:"error_description"` + Error string `json:"error"` } if jsonErr := json.Unmarshal(body, &payload); jsonErr == nil { err.Code = payload.Error - err.Description = payload.Description } return err } @@ -147,31 +129,7 @@ func exchangeCode(ctx context.Context, httpClient *http.Client, tokenEndpoint, c } defer resp.Body.Close() - body, err := io.ReadAll(io.LimitReader(resp.Body, 64<<10)) - if err != nil { - if resp.StatusCode != http.StatusOK { - // The status and its headers are already in hand. A body that - // truncates on the way in is no reason to lose the verdict with it — - // least of all a 429, whose whole value here is the Retry-After. - return nil, newTokenEndpointError("token exchange", resp, nil) - } - return nil, fmt.Errorf("reading token response: %w", err) - } - - if resp.StatusCode != http.StatusOK { - return nil, newTokenEndpointError("token exchange", resp, body) - } - - var token OAuthToken - if err := json.Unmarshal(body, &token); err != nil { - return nil, fmt.Errorf("parsing token response: %w", err) - } - - if token.ExpiresIn > 0 { - token.ExpiresAt = time.Now().Add(time.Duration(token.ExpiresIn) * time.Second) - } - - return &token, nil + return readTokenResponse("token exchange", resp) } // refreshOAuthToken refreshes an access token using a refresh token. @@ -196,21 +154,25 @@ func refreshOAuthToken(ctx context.Context, httpClient *http.Client, tokenEndpoi } defer resp.Body.Close() - body, err := io.ReadAll(io.LimitReader(resp.Body, 64<<10)) - if err != nil { - if resp.StatusCode != http.StatusOK { - return nil, newTokenEndpointError("token refresh", resp, nil) - } - return nil, fmt.Errorf("reading refresh response: %w", err) - } + return readTokenResponse("token refresh", resp) +} +// readTokenResponse reads the token endpoint's answer, typing a refusal so the caller +// can tell a dead grant from a server that would not answer. +func readTokenResponse(op string, resp *http.Response) (*OAuthToken, error) { + body, err := io.ReadAll(io.LimitReader(resp.Body, 64<<10)) if resp.StatusCode != http.StatusOK { - return nil, newTokenEndpointError("token refresh", resp, body) + // A body that truncates on the way in does not lose the verdict with it: + // the status and its Retry-After are already in hand. + return nil, newTokenEndpointError(op, resp, body) + } + if err != nil { + return nil, fmt.Errorf("reading %s response: %w", op, err) } var token OAuthToken if err := json.Unmarshal(body, &token); err != nil { - return nil, fmt.Errorf("parsing refresh response: %w", err) + return nil, fmt.Errorf("parsing %s response: %w", op, err) } if token.ExpiresIn > 0 { diff --git a/internal/auth/oauth_test.go b/internal/auth/oauth_test.go index 245313b6..c5ce7e89 100644 --- a/internal/auth/oauth_test.go +++ b/internal/auth/oauth_test.go @@ -100,9 +100,9 @@ func TestOAuthTokenResponseFailures(t *testing.T) { exchange bool }{ {name: "exchange status", status: http.StatusUnauthorized, body: "denied", want: "token exchange failed (status 401): denied", exchange: true}, - {name: "exchange invalid JSON", status: http.StatusOK, body: "not-json", want: "parsing token response", exchange: true}, + {name: "exchange invalid JSON", status: http.StatusOK, body: "not-json", want: "parsing token exchange response", exchange: true}, {name: "refresh status", status: http.StatusBadGateway, body: "upstream unavailable", want: "token refresh failed (status 502): upstream unavailable"}, - {name: "refresh invalid JSON", status: http.StatusOK, body: "not-json", want: "parsing refresh response"}, + {name: "refresh invalid JSON", status: http.StatusOK, body: "not-json", want: "parsing token refresh response"}, } for _, tt := range tests { From 2890c5faf97ab443d39dd5282aceb801c21fff92 Mon Sep 17 00:00:00 2001 From: Zacharias Dyna Knudsen Date: Wed, 16 Sep 2026 09:37:54 +0200 Subject: [PATCH 4/4] Keep a rate limit classified, and drop the cache with a refused grant MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review follow-up. `hey auth refresh` and `hey auth token` wrapped every manager failure as ErrAuth. The manager classifies its own refusals now — a refused grant is auth, a throttled token endpoint is rate_limit with how long to wait — so a 429 came out of both commands as exit 3 with "Run: hey auth login", advice that spends another request on the same limit. authFailure keeps an already classified error as it is, code, hint and status, and only puts the command's context in front of the message. Anything unclassified is still ErrAuth, as before. A refresh token the server refused is forgotten by the manager itself, in the middle of whatever command sent it, and that path skipped the cache. Logout clears the HTTP response cache because cached mail must not outlive the credential that fetched it; the same holds when the credential goes on the server's verdict. The manager gains OnCredentialCleared, which runs after it has deleted a credential of its own accord and nowhere else — not on Logout, whose callers clear the cache themselves, and not when the store refused the delete, since the credential is then still there to be used. root.go wires it to clearHTTPCache, best-effort like the logout path. Tests pin both: a 429 keeps its code and exit through either command with the credential intact, a refused grant clears the credential and the cache together, and a 429 or a 502 clears neither. The hook is tested on its own to run exactly when the credential went. The cache tests share one seed helper now instead of four copies of the same files. --- internal/auth/auth.go | 20 +++++ internal/auth/auth_test.go | 68 +++++++++++++++ internal/cmd/auth.go | 20 ++++- internal/cmd/auth_commands_test.go | 53 +++++++++++ internal/cmd/root.go | 4 + internal/cmd/sdk_cache_test.go | 136 +++++++++++++++++++++++------ 6 files changed, 273 insertions(+), 28 deletions(-) diff --git a/internal/auth/auth.go b/internal/auth/auth.go index 9ad00249..73789402 100644 --- a/internal/auth/auth.go +++ b/internal/auth/auth.go @@ -42,6 +42,11 @@ type Manager struct { // refusedRefreshToken is a grant the server refused that the store could not // delete, so this process remembers not to send it again. Guarded by mu. refusedRefreshToken string + + // credentialCleared runs after the manager has deleted a credential on its own + // verdict, so the owner of the response cache can drop what that credential + // fetched. Logout is not that: its callers already clear the cache themselves. + credentialCleared func() } // defaultRefreshHold is how long to sit out a rate limit that came without a @@ -261,6 +266,16 @@ func (m *Manager) LoginWithCookie(cookie string) error { return m.store.Save(m.baseURL, creds) } +// OnCredentialCleared registers what to run when the manager clears a credential +// on its own — today, when the server has refused the refresh token. It does not +// run for Logout, whose callers clear the cache themselves, and not when the store +// refused the deletion, because the credential is then still there to be used. +func (m *Manager) OnCredentialCleared(fn func()) { + m.mu.Lock() + defer m.mu.Unlock() + m.credentialCleared = fn +} + // Logout removes stored credentials. func (m *Manager) Logout() error { return m.store.Delete(m.baseURL) @@ -387,6 +402,11 @@ func (m *Manager) accountForRefreshFailure(err error, sentRefreshToken string) e m.refusedRefreshToken = sentRefreshToken return errRefusedGrant(delErr) } + // Cached mail must not outlive the credential that fetched it, here as much + // as on an explicit logout. + if m.credentialCleared != nil { + m.credentialCleared() + } return errRefusedGrant(nil) } diff --git a/internal/auth/auth_test.go b/internal/auth/auth_test.go index 326d8023..a142d8df 100644 --- a/internal/auth/auth_test.go +++ b/internal/auth/auth_test.go @@ -957,6 +957,74 @@ func TestARefusedGrantIsNotResentWhenItCannotBeDeleted(t *testing.T) { } } +// Forgetting the credential is the manager's call, so whoever owns the response +// cache has to hear about it from here: cached mail must not outlive the credential +// that fetched it. The hook runs only when the credential actually went — a refusal +// the store would not delete, or a failure that is no verdict on the grant, keeps +// the credential and so keeps the cache. +func TestTheClearedCredentialHookRunsOnlyWhenTheCredentialWent(t *testing.T) { + tests := []struct { + name string + status int + body string + refuseDelete bool + wantRuns int + }{ + {name: "refused grant", status: http.StatusBadRequest, body: `{"error":"invalid_grant"}`, wantRuns: 1}, + {name: "refused grant the store keeps", status: http.StatusBadRequest, body: `{"error":"invalid_grant"}`, refuseDelete: true, wantRuns: 0}, + {name: "rate limited", status: http.StatusTooManyRequests, body: `{"error":"rate_limit_exceeded"}`, wantRuns: 0}, + {name: "origin failure", status: http.StatusBadGateway, body: "upstream unavailable", wantRuns: 0}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(tt.status) + _, _ = fmt.Fprint(w, tt.body) + })) + defer server.Close() + + t.Setenv("HEY_TOKEN", "") + t.Setenv("HEY_NO_KEYRING", "") + mgr := NewManager(server.URL, server.Client(), t.TempDir()) + stored := "" + mgr.GetStore().useKeyring = true + mgr.GetStore().initOnce.Do(func() {}) + mgr.GetStore().keyring = credentialKeyring{ + set: func(_, _, password string) error { stored = password; return nil }, + get: func(_, _ string) (string, error) { + if stored == "" { + return "", errors.New("no credential") + } + return stored, nil + }, + delete: func(_, _ string) error { + if tt.refuseDelete { + return errors.New("keyring is locked") + } + stored = "" + return nil + }, + } + runs := 0 + mgr.OnCredentialCleared(func() { runs++ }) + saveExpiredCredential(t, mgr) + + if _, err := mgr.AccessToken(t.Context()); err == nil { + t.Fatal("AccessToken succeeded against a failing token endpoint") + } + + if runs != tt.wantRuns { + t.Errorf("hook ran %d times, want %d", runs, tt.wantRuns) + } + if kept := stored != ""; kept != (tt.wantRuns == 0) { + t.Errorf("credential kept = %v; the hook has to run exactly when it is gone", kept) + } + }) + } +} + // The bug itself. `hey watch` re-authenticates on every ActionCable dial, so a refresh // token the server has already killed used to go back out every fifteen seconds for as // long as the process lived — and the token endpoint's limit counts refusals, so the diff --git a/internal/cmd/auth.go b/internal/cmd/auth.go index 50c2403d..1758ad08 100644 --- a/internal/cmd/auth.go +++ b/internal/cmd/auth.go @@ -2,6 +2,7 @@ package cmd import ( "context" + "errors" "fmt" "os" "strings" @@ -277,7 +278,7 @@ func newAuthRefreshCommand() *cobra.Command { Short: "Force token refresh", RunE: func(cmd *cobra.Command, args []string) error { if err := authMgr.Refresh(cmd.Context()); err != nil { - return apierr.ErrAuth(fmt.Sprintf("refresh failed: %v", err)) + return authFailure("refresh failed", err) } return writeMutation(cmd, "Token refreshed", nil) }, @@ -310,7 +311,7 @@ Cookie header, so it is not a bearer token and this command refuses to print it. token, err := authMgr.AccessToken(cmd.Context()) if err != nil { - return apierr.ErrAuth(fmt.Sprintf("could not get token: %v", err)) + return authFailure("could not get token", err) } fmt.Fprint(cmd.OutOrStdout(), token) return nil @@ -322,6 +323,21 @@ Cookie header, so it is not a bearer token and this command refuses to print it. return cmd } +// authFailure reports a manager failure with the command's context in front of it. +// The manager classifies its own refusals — a refused grant is auth, a throttled +// token endpoint is rate_limit with how long to wait — and wrapping every one as +// ErrAuth turned a 429 into exit 3 and "Run: hey auth login". A classified error +// keeps its code, hint and status; only its message gains the context. +func authFailure(context string, err error) error { + var classified *apierr.Error + if !errors.As(err, &classified) { + return apierr.ErrAuth(fmt.Sprintf("%s: %v", context, err)) + } + prefixed := *classified + prefixed.Message = fmt.Sprintf("%s: %s", context, classified.Message) + return &prefixed +} + // refuseSessionCookieAsToken stops `hey auth token` from printing a session cookie. // AccessToken falls back to one, and a cookie sent as a bearer token 401s with // nothing to explain it — besides leaving the cookie in the caller's shell history. diff --git a/internal/cmd/auth_commands_test.go b/internal/cmd/auth_commands_test.go index 1cd7819b..d5fc8028 100644 --- a/internal/cmd/auth_commands_test.go +++ b/internal/cmd/auth_commands_test.go @@ -3,6 +3,7 @@ package cmd import ( "bytes" "encoding/json" + "errors" "io" "net/http" "net/http/httptest" @@ -12,6 +13,7 @@ import ( "testing" "time" + "github.com/basecamp/hey-cli/internal/apierr" "github.com/basecamp/hey-cli/internal/auth" "github.com/basecamp/hey-cli/internal/output" ) @@ -206,6 +208,57 @@ func TestAuthRefreshFailure(t *testing.T) { } } +// A throttled token endpoint is not a missing login. Both commands used to wrap every +// manager failure as ErrAuth, so a 429 came out as exit 3 with "Run: hey auth login" — +// advice that spends another request on the same limit. +func TestAuthCommandsKeepARateLimitClassified(t *testing.T) { + commands := map[string][]string{ + "refresh": {"auth", "refresh"}, + "token": {"auth", "token", "--stored"}, + } + + for name, args := range commands { + t.Run(name, func(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Retry-After", "42") + w.WriteHeader(http.StatusTooManyRequests) + })) + defer server.Close() + configHome := t.TempDir() + t.Setenv("HEY_NO_KEYRING", "1") + manager := auth.NewManager(server.URL, server.Client(), filepath.Join(configHome, "hey-cli")) + // Expired, so `auth token` has to refresh rather than print what it holds. + expired := &auth.Credentials{AccessToken: "old-access", RefreshToken: "old-refresh", ExpiresAt: time.Now().Add(-time.Hour).Unix()} + if err := manager.GetStore().Save(manager.CredentialKey(), expired); err != nil { + t.Fatalf("seed credentials: %v", err) + } + + _, _, err := runAuthCommand(t, configHome, server.URL, "", true, args...) + + var classified *apierr.Error + if !errors.As(err, &classified) { + t.Fatalf("error = %v, want an *apierr.Error", err) + } + if classified.Code != apierr.CodeRateLimit { + t.Errorf("code = %q, want %q", classified.Code, apierr.CodeRateLimit) + } + if got := output.ExitCodeFor(err); got != output.ExitRateLimit { + t.Errorf("exit code = %d, want %d", got, output.ExitRateLimit) + } + if !strings.Contains(classified.Message, "rate-limiting") || !strings.Contains(classified.Message, "42s") { + t.Errorf("message = %q, want the rate limit and its wait", classified.Message) + } + if strings.Contains(classified.Hint, "hey auth login") { + t.Errorf("hint = %q; logging in again spends the same limit", classified.Hint) + } + creds, loadErr := manager.GetStore().Load(manager.CredentialKey()) + if loadErr != nil || creds.RefreshToken != "old-refresh" { + t.Errorf("credentials after a rate limit = %#v, %v; want them kept", creds, loadErr) + } + }) + } +} + func TestDoctorCommandReportsEnvironment(t *testing.T) { server := httptest.NewServer(http.NotFoundHandler()) defer server.Close() diff --git a/internal/cmd/root.go b/internal/cmd/root.go index 3c1206cf..9d543fdd 100644 --- a/internal/cmd/root.go +++ b/internal/cmd/root.go @@ -116,6 +116,10 @@ func newRootCmd() *cobra.Command { configDir := config.ConfigDir() httpClient := &http.Client{Timeout: 30 * time.Second} authMgr = auth.NewManager(cfg.BaseURL, httpClient, configDir) + // A refresh token HEY refuses is forgotten by the manager itself, on + // whatever command happened to send it. Cached mail must not outlive + // that credential any more than it outlives a logout. + authMgr.OnCredentialCleared(func() { clearHTTPCache(cmd.ErrOrStderr()) }) initSDK(authMgr, cfg.BaseURL) // The agent-local setup subcommands and skill commands never read diff --git a/internal/cmd/sdk_cache_test.go b/internal/cmd/sdk_cache_test.go index ef5255d1..c0209cf7 100644 --- a/internal/cmd/sdk_cache_test.go +++ b/internal/cmd/sdk_cache_test.go @@ -1,10 +1,16 @@ package cmd import ( + "errors" + "io" + "net/http" + "net/http/httptest" "os" "path/filepath" "testing" + "time" + "github.com/basecamp/hey-cli/internal/apierr" "github.com/basecamp/hey-cli/internal/auth" ) @@ -26,18 +32,7 @@ func TestInitSDKEnablesTheRevalidationCache(t *testing.T) { // ones cached: the replacement clears the cache without waiting for a logout. func TestLoginReplacingCredentialsClearsTheHTTPCache(t *testing.T) { configHome := t.TempDir() - responses := filepath.Join(configHome, "hey-cli", "http", "responses") - if err := os.MkdirAll(responses, 0o700); err != nil { - t.Fatal(err) - } - for name, content := range map[string]string{ - filepath.Join(responses, "abc123.body"): `{"cached":"mail"}`, - filepath.Join(configHome, "hey-cli", "http", "etags.json"): `{"abc123":"\"v1\""}`, - } { - if err := os.WriteFile(name, []byte(content), 0o600); err != nil { - t.Fatal(err) - } - } + responses, etags := seedHTTPCache(t, configHome) if _, _, err := runAuthCommand(t, configHome, "https://app.hey.com", "", true, "auth", "login", "--cookie", "replacement-cookie"); err != nil { t.Fatalf("auth login: %v", err) @@ -46,25 +41,14 @@ func TestLoginReplacingCredentialsClearsTheHTTPCache(t *testing.T) { if _, err := os.Stat(responses); !os.IsNotExist(err) { t.Error("expected login to drop the previous credentials' cached responses") } - if _, err := os.Stat(filepath.Join(configHome, "hey-cli", "http", "etags.json")); !os.IsNotExist(err) { + if _, err := os.Stat(etags); !os.IsNotExist(err) { t.Error("expected login to drop the previous credentials' cached ETags") } } func TestLogoutClearsTheHTTPCache(t *testing.T) { configHome := t.TempDir() - responses := filepath.Join(configHome, "hey-cli", "http", "responses") - if err := os.MkdirAll(responses, 0o700); err != nil { - t.Fatal(err) - } - for name, content := range map[string]string{ - filepath.Join(responses, "abc123.body"): `{"cached":"mail"}`, - filepath.Join(configHome, "hey-cli", "http", "etags.json"): `{"abc123":"\"v1\""}`, - } { - if err := os.WriteFile(name, []byte(content), 0o600); err != nil { - t.Fatal(err) - } - } + responses, etags := seedHTTPCache(t, configHome) if _, _, err := runAuthCommand(t, configHome, "https://app.hey.com", "", true, "auth", "login", "--cookie", "session-cookie"); err != nil { t.Fatalf("auth login: %v", err) @@ -76,7 +60,107 @@ func TestLogoutClearsTheHTTPCache(t *testing.T) { if _, err := os.Stat(responses); !os.IsNotExist(err) { t.Error("expected logout to drop the cached responses") } - if _, err := os.Stat(filepath.Join(configHome, "hey-cli", "http", "etags.json")); !os.IsNotExist(err) { + if _, err := os.Stat(etags); !os.IsNotExist(err) { t.Error("expected logout to drop the cached ETags") } } + +// A refresh token HEY has refused is forgotten by the manager itself, in the middle +// of whatever command sent it. Cached mail must not outlive that credential any more +// than it outlives an explicit logout. +func TestARefusedGrantClearsTheHTTPCache(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusBadRequest) + _, _ = io.WriteString(w, `{"error":"invalid_grant","error_description":"The refresh token is invalid"}`) + })) + defer server.Close() + configHome := t.TempDir() + responses, etags := seedHTTPCache(t, configHome) + manager := seedExpiredCredential(t, configHome, server) + + _, _, err := runAuthCommand(t, configHome, server.URL, "", true, "auth", "refresh") + var classified *apierr.Error + if !errors.As(err, &classified) || classified.Code != apierr.CodeAuth { + t.Fatalf("error = %v, want one coded %q", err, apierr.CodeAuth) + } + + if _, loadErr := manager.GetStore().Load(manager.CredentialKey()); loadErr == nil { + t.Error("the refused credential is still stored") + } + if _, statErr := os.Stat(responses); !os.IsNotExist(statErr) { + t.Error("expected the refused grant to drop the cached responses") + } + if _, statErr := os.Stat(etags); !os.IsNotExist(statErr) { + t.Error("expected the refused grant to drop the cached ETags") + } +} + +// Anything softer than a refused grant is no verdict on the credential, so it keeps +// both the credential and the mail it fetched. +func TestATransientRefreshFailureKeepsTheCredentialAndTheHTTPCache(t *testing.T) { + statuses := map[string]int{ + "rate limited": http.StatusTooManyRequests, + "origin failure": http.StatusBadGateway, + } + + for name, status := range statuses { + t.Run(name, func(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(status) + })) + defer server.Close() + configHome := t.TempDir() + responses, etags := seedHTTPCache(t, configHome) + manager := seedExpiredCredential(t, configHome, server) + + if _, _, err := runAuthCommand(t, configHome, server.URL, "", true, "auth", "refresh"); err == nil { + t.Fatal("auth refresh succeeded against a failing token endpoint") + } + + creds, loadErr := manager.GetStore().Load(manager.CredentialKey()) + if loadErr != nil || creds.RefreshToken != "old-refresh" { + t.Errorf("credentials = %#v, %v; want them kept", creds, loadErr) + } + if _, statErr := os.Stat(filepath.Join(responses, "abc123.body")); statErr != nil { + t.Errorf("cached response: %v; want it kept", statErr) + } + if _, statErr := os.Stat(etags); statErr != nil { + t.Errorf("cached ETags: %v; want them kept", statErr) + } + }) + } +} + +// seedHTTPCache plants one cached response and its ETag under configHome, the way +// the SDK's cache lays them out, and answers where each is. +func seedHTTPCache(t *testing.T, configHome string) (responses, etags string) { + t.Helper() + responses = filepath.Join(configHome, "hey-cli", "http", "responses") + etags = filepath.Join(configHome, "hey-cli", "http", "etags.json") + if err := os.MkdirAll(responses, 0o700); err != nil { + t.Fatal(err) + } + for name, content := range map[string]string{ + filepath.Join(responses, "abc123.body"): `{"cached":"mail"}`, + etags: `{"abc123":"\"v1\""}`, + } { + if err := os.WriteFile(name, []byte(content), 0o600); err != nil { + t.Fatal(err) + } + } + return responses, etags +} + +// seedExpiredCredential stores a credential whose access token has expired, so the +// next command has to send the refresh token to server. +func seedExpiredCredential(t *testing.T, configHome string, server *httptest.Server) *auth.Manager { + t.Helper() + t.Setenv("HEY_NO_KEYRING", "1") + manager := auth.NewManager(server.URL, server.Client(), filepath.Join(configHome, "hey-cli")) + expired := &auth.Credentials{AccessToken: "old-access", RefreshToken: "old-refresh", ExpiresAt: time.Now().Add(-time.Hour).Unix()} + if err := manager.GetStore().Save(manager.CredentialKey(), expired); err != nil { + t.Fatalf("seed credentials: %v", err) + } + return manager +}