From 9f010a59fbf5e4fe0e44106a07184ee7132e601f Mon Sep 17 00:00:00 2001 From: Bryce Lelbach Date: Sat, 18 Apr 2026 07:17:01 +0000 Subject: [PATCH] fix(auth): refresh on 401 and stop writing bogus refresh tokens MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Users report being logged out of brev-cli multiple times per hour, forcing repeated `brev login --token`. Two bugs in the auth flow caused this: 1. `LoginWithToken` stored the literal string "auto-login" as the RefreshToken whenever it was handed a valid JWT access token (pkg/auth/auth.go). When that short-lived access token later expired, the refresh path in `GetFreshAccessTokenOrNil` tried to exchange "auto-login" with the IdP, which always failed, so the user was prompted to re-login every time the access token aged out — roughly hourly with typical NVIDIA KAS token lifetimes. 2. The auth-failure retry handler in pkg/store/http.go only fired on HTTP 403 Forbidden. APIs returning 401 Unauthorized (the more standard response for expired credentials) bypassed the refresh hook entirely and surfaced as a hard failure on the first such response, even when the refresh token was still good. This change: - Stores an empty RefreshToken when LoginWithToken receives a JWT, so the refresh path correctly recognizes there is nothing to refresh and falls through without a failed IdP round-trip. Emits a one-line notice on stderr so the user knows this session cannot be auto-renewed. - Normalizes the legacy "auto-login" sentinel to "" on read, so users with existing credentials.json files from older CLI versions benefit from the fix without re-running `brev login --token`. - Changes `GetFreshAccessTokenOrNil` to return "" (rather than an expired AccessToken) when there is no way to refresh, so callers prompt for re-login cleanly instead of shipping an expired JWT to the API. - Extends the refresh-and-retry handler in AuthHTTPStore to fire on both 401 and 403. Longer-term the website at brev.nvidia.com/settings/cli should hand out a real refresh token (or a short-lived exchange code) rather than a raw access JWT so `brev login --token` can produce a refreshable session. This PR is the CLI-only stop-the-bleeding fix. Co-Authored-By: Claude Opus 4.7 --- pkg/auth/auth.go | 38 +++++++++++++++++++++++++++++++++----- pkg/store/http.go | 12 ++++++++++-- 2 files changed, 43 insertions(+), 7 deletions(-) diff --git a/pkg/auth/auth.go b/pkg/auth/auth.go index 3138fd034..7c9846309 100644 --- a/pkg/auth/auth.go +++ b/pkg/auth/auth.go @@ -146,6 +146,13 @@ func (t Auth) GetFreshAccessTokenOrNil() (string, error) { return "", nil } + // Older CLI versions stored the literal string "auto-login" when + // `brev login --token` had no real refresh token to save. Treat it as + // absent so we do not attempt to exchange it with the IdP and fail. + if tokens.RefreshToken == autoLoginSentinel { + tokens.RefreshToken = "" + } + // should always at least have access token? if tokens.AccessToken == "" { breverrors.GetDefaultErrorReporter().ReportMessage("access token is an empty string but shouldn't be") @@ -154,7 +161,13 @@ func (t Auth) GetFreshAccessTokenOrNil() (string, error) { if err != nil { return "", breverrors.WrapAndTrace(err) } - if !isAccessTokenValid && tokens.RefreshToken != "" { + if !isAccessTokenValid { + if tokens.RefreshToken == "" { + // Access token is expired and we have no refresh token. Returning + // the expired token here would just cause a 401 on the next API + // call; return empty so callers can prompt for re-login instead. + return "", nil + } tokens, err = t.getNewTokensWithRefreshOrNil(tokens.RefreshToken) if err != nil { return "", breverrors.WrapAndTrace(err) @@ -162,8 +175,6 @@ func (t Auth) GetFreshAccessTokenOrNil() (string, error) { if tokens == nil { return "", nil } - } else if tokens.RefreshToken == "" && tokens.AccessToken == "" { - return "", nil } return tokens.AccessToken, nil } @@ -197,22 +208,39 @@ func shouldLogin() (bool, error) { return trimmed == "y" || trimmed == "", nil } +// autoLoginSentinel is a legacy value older CLI versions stored in place of +// a real token when `brev login --token` was used. It is not a valid token of +// any kind; treat it as "absent" on read. +const autoLoginSentinel = "auto-login" + func (t Auth) LoginWithToken(token string) error { valid, err := isAccessTokenValid(token) if err != nil { return breverrors.WrapAndTrace(err) } if valid { + // The token is a self-contained JWT access token with no accompanying + // refresh token. Previously we stored the string "auto-login" in the + // RefreshToken slot; when the access token expired the refresh path + // then attempted to exchange that sentinel with the IdP, which always + // failed, logging the user out every time the short-lived access + // token aged out. Store an empty RefreshToken instead so the refresh + // path correctly recognizes there is nothing to refresh and prompts + // for a fresh login exactly once. + fmt.Fprintln(os.Stderr, "Note: tokens from --token cannot be refreshed; re-run `brev login` when the session expires.") err := t.authStore.SaveAuthTokens(entity.AuthTokens{ AccessToken: token, - RefreshToken: "auto-login", + RefreshToken: "", }) if err != nil { return breverrors.WrapAndTrace(err) } } else { + // The token is not a JWT, assume it is a refresh token. The access + // token slot is filled with the sentinel so the first API call + // triggers a refresh to populate a real access token. err := t.authStore.SaveAuthTokens(entity.AuthTokens{ - AccessToken: "auto-login", + AccessToken: autoLoginSentinel, RefreshToken: token, }) if err != nil { diff --git a/pkg/store/http.go b/pkg/store/http.go index 60884f810..1bb264971 100644 --- a/pkg/store/http.go +++ b/pkg/store/http.go @@ -104,7 +104,7 @@ func (s *AuthHTTPStore) SetForbiddenStatusRetryHandler(handler func() error) err } attemptsThresh := 1 s.authHTTPClient.restyClient.OnAfterResponse(func(c *resty.Client, r *resty.Response) error { - if r.StatusCode() == http.StatusForbidden && r.Request.Attempt < attemptsThresh+1 { + if isAuthFailure(r.StatusCode()) && r.Request.Attempt < attemptsThresh+1 { err := handler() if err != nil { return breverrors.WrapAndTrace(err) @@ -117,7 +117,7 @@ func (s *AuthHTTPStore) SetForbiddenStatusRetryHandler(handler func() error) err if e != nil { return false } - return r.StatusCode() == http.StatusForbidden + return isAuthFailure(r.StatusCode()) }) s.authHTTPClient.restyClient.SetRetryCount(attemptsThresh) @@ -125,6 +125,14 @@ func (s *AuthHTTPStore) SetForbiddenStatusRetryHandler(handler func() error) err return nil } +// isAuthFailure reports whether an HTTP status code indicates the caller's +// credentials are missing, invalid, or expired. Both 401 Unauthorized and +// 403 Forbidden can signal an expired access token from Brev's APIs, so we +// treat both as triggers for the refresh-and-retry path. +func isAuthFailure(code int) bool { + return code == http.StatusUnauthorized || code == http.StatusForbidden +} + type AuthHTTPClient struct { restyClient *resty.Client auth Auth