Skip to content

Commit 9f010a5

Browse files
brycelelbachclaude
andcommitted
fix(auth): refresh on 401 and stop writing bogus refresh tokens
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 <noreply@anthropic.com>
1 parent 9e0e34b commit 9f010a5

2 files changed

Lines changed: 43 additions & 7 deletions

File tree

pkg/auth/auth.go

Lines changed: 33 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -146,6 +146,13 @@ func (t Auth) GetFreshAccessTokenOrNil() (string, error) {
146146
return "", nil
147147
}
148148

149+
// Older CLI versions stored the literal string "auto-login" when
150+
// `brev login --token` had no real refresh token to save. Treat it as
151+
// absent so we do not attempt to exchange it with the IdP and fail.
152+
if tokens.RefreshToken == autoLoginSentinel {
153+
tokens.RefreshToken = ""
154+
}
155+
149156
// should always at least have access token?
150157
if tokens.AccessToken == "" {
151158
breverrors.GetDefaultErrorReporter().ReportMessage("access token is an empty string but shouldn't be")
@@ -154,16 +161,20 @@ func (t Auth) GetFreshAccessTokenOrNil() (string, error) {
154161
if err != nil {
155162
return "", breverrors.WrapAndTrace(err)
156163
}
157-
if !isAccessTokenValid && tokens.RefreshToken != "" {
164+
if !isAccessTokenValid {
165+
if tokens.RefreshToken == "" {
166+
// Access token is expired and we have no refresh token. Returning
167+
// the expired token here would just cause a 401 on the next API
168+
// call; return empty so callers can prompt for re-login instead.
169+
return "", nil
170+
}
158171
tokens, err = t.getNewTokensWithRefreshOrNil(tokens.RefreshToken)
159172
if err != nil {
160173
return "", breverrors.WrapAndTrace(err)
161174
}
162175
if tokens == nil {
163176
return "", nil
164177
}
165-
} else if tokens.RefreshToken == "" && tokens.AccessToken == "" {
166-
return "", nil
167178
}
168179
return tokens.AccessToken, nil
169180
}
@@ -197,22 +208,39 @@ func shouldLogin() (bool, error) {
197208
return trimmed == "y" || trimmed == "", nil
198209
}
199210

211+
// autoLoginSentinel is a legacy value older CLI versions stored in place of
212+
// a real token when `brev login --token` was used. It is not a valid token of
213+
// any kind; treat it as "absent" on read.
214+
const autoLoginSentinel = "auto-login"
215+
200216
func (t Auth) LoginWithToken(token string) error {
201217
valid, err := isAccessTokenValid(token)
202218
if err != nil {
203219
return breverrors.WrapAndTrace(err)
204220
}
205221
if valid {
222+
// The token is a self-contained JWT access token with no accompanying
223+
// refresh token. Previously we stored the string "auto-login" in the
224+
// RefreshToken slot; when the access token expired the refresh path
225+
// then attempted to exchange that sentinel with the IdP, which always
226+
// failed, logging the user out every time the short-lived access
227+
// token aged out. Store an empty RefreshToken instead so the refresh
228+
// path correctly recognizes there is nothing to refresh and prompts
229+
// for a fresh login exactly once.
230+
fmt.Fprintln(os.Stderr, "Note: tokens from --token cannot be refreshed; re-run `brev login` when the session expires.")
206231
err := t.authStore.SaveAuthTokens(entity.AuthTokens{
207232
AccessToken: token,
208-
RefreshToken: "auto-login",
233+
RefreshToken: "",
209234
})
210235
if err != nil {
211236
return breverrors.WrapAndTrace(err)
212237
}
213238
} else {
239+
// The token is not a JWT, assume it is a refresh token. The access
240+
// token slot is filled with the sentinel so the first API call
241+
// triggers a refresh to populate a real access token.
214242
err := t.authStore.SaveAuthTokens(entity.AuthTokens{
215-
AccessToken: "auto-login",
243+
AccessToken: autoLoginSentinel,
216244
RefreshToken: token,
217245
})
218246
if err != nil {

pkg/store/http.go

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -104,7 +104,7 @@ func (s *AuthHTTPStore) SetForbiddenStatusRetryHandler(handler func() error) err
104104
}
105105
attemptsThresh := 1
106106
s.authHTTPClient.restyClient.OnAfterResponse(func(c *resty.Client, r *resty.Response) error {
107-
if r.StatusCode() == http.StatusForbidden && r.Request.Attempt < attemptsThresh+1 {
107+
if isAuthFailure(r.StatusCode()) && r.Request.Attempt < attemptsThresh+1 {
108108
err := handler()
109109
if err != nil {
110110
return breverrors.WrapAndTrace(err)
@@ -117,14 +117,22 @@ func (s *AuthHTTPStore) SetForbiddenStatusRetryHandler(handler func() error) err
117117
if e != nil {
118118
return false
119119
}
120-
return r.StatusCode() == http.StatusForbidden
120+
return isAuthFailure(r.StatusCode())
121121
})
122122
s.authHTTPClient.restyClient.SetRetryCount(attemptsThresh)
123123

124124
s.isRefreshTokenHandlerSet = true
125125
return nil
126126
}
127127

128+
// isAuthFailure reports whether an HTTP status code indicates the caller's
129+
// credentials are missing, invalid, or expired. Both 401 Unauthorized and
130+
// 403 Forbidden can signal an expired access token from Brev's APIs, so we
131+
// treat both as triggers for the refresh-and-retry path.
132+
func isAuthFailure(code int) bool {
133+
return code == http.StatusUnauthorized || code == http.StatusForbidden
134+
}
135+
128136
type AuthHTTPClient struct {
129137
restyClient *resty.Client
130138
auth Auth

0 commit comments

Comments
 (0)