Re-review of the hardening found a bug the hardening introduced, and a second one it left behind. The fix for "don't echo a credential into an error" bounded the read rather than the message, so any response longer than 512 bytes was truncated before parsing. A real token response is ~550 bytes — two ~120-char tokens plus organization and account objects — so refresh would have failed every time against the live endpoint, and the "not a token response" branch would then have printed the truncated body, which contains both tokens, to the screen. Every test passed because every fixture was about sixty bytes. There is now a full-size fixture, and it fails if the old bound is put back. The second: after a rotation the file kept the old token's refreshTokenExpiresAt beside the new refreshToken, so the next process start read a stale expiry, concluded the refresh token was dead, and refused to send a request the server would have honoured. Rather than guess at a value the exchange never returns, refreshTokenExpiresAt is no longer modelled at all — not parsed, not written, preserved like any other key we do not understand. Source asks the server once and remembers a refusal instead. An answer beats a timestamp we would have had to invent, and the dead-token case still costs exactly one round trip. Also: ErrStoreChanged no longer triggers the write-back warning. It means someone else logged in with a newer credential, which is good news, not a broken login. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Showing 5 changed files with 128 additions and 154 deletions.
| @@ -70,16 +70,15 @@ One package, one concern, no ambient state: | ||
| 70 | 70 | |
| 71 | 71 | ```go |
| 72 | 72 | type Credentials struct { |
| 73 | - AccessToken, RefreshToken string | |
| 74 | - ExpiresAt, RefreshExpiresAt time.Time | |
| 75 | - Scopes []string | |
| 76 | - SubscriptionType string | |
| 73 | + AccessToken, RefreshToken string | |
| 74 | + ExpiresAt time.Time | |
| 75 | + Scopes []string | |
| 76 | + SubscriptionType string | |
| 77 | 77 | } |
| 78 | 78 | |
| 79 | - func Load(home string) (*Credentials, error) // parse. no network | |
| 80 | - func Save(home string, c *Credentials) error // read-modify-write, atomic | |
| 81 | - func (c *Credentials) Expired(now time.Time) bool // with skew | |
| 82 | - func (c *Credentials) RefreshExpired(now time.Time) bool // terminal | |
| 79 | + func Load(home string) (*Credentials, error) // parse. no network | |
| 80 | + func Save(home string, c *Credentials, expect string) error // read-modify-write, atomic, CAS | |
| 81 | + func (c *Credentials) Expired(now time.Time) bool // with skew | |
| 83 | 82 | func Refresh(context.Context, *http.Client, *Credentials) (*Credentials, error) |
| 84 | 83 | |
| 85 | 84 | // Source is the policy: refresh on expiry, write back only on rotation. |
| @@ -87,6 +86,10 @@ func NewSource(c *Credentials, h *http.Client, home string) *Source | ||
| 87 | 86 | func (s *Source) Token(context.Context) (string, error) |
| 88 | 87 | ``` |
| 89 | 88 | |
| 89 | + `refreshTokenExpiresAt` is conspicuously absent from `Credentials`. That is | |
| 90 | + deliberate and the reasoning is in the refresh section below: a field we cannot | |
| 91 | + keep accurate after a rotation is worse than no field. | |
| 92 | + | |
| 90 | 93 | `Load` takes `home` rather than calling `os.UserHomeDir` so tests point it at a |
| 91 | 94 | fixture directory, matching how `skills.Discover` already takes its roots. |
| 92 | 95 | `Refresh` returns new credentials and writes nothing; `Source` decides whether |
| @@ -212,12 +215,15 @@ Four more things the write has to survive, none of them hypothetical: | ||
| 212 | 215 | our own making against an endpoint that already only answers 429. It is |
| 213 | 216 | rejected as malformed instead. |
| 214 | 217 | - **A rotated token's unknown lifetime.** The exchange never says how long the |
| 215 | - new refresh token lives. In memory that is the zero time, and `RefreshExpired` | |
| 216 | - reads zero as *unknown* rather than *expired* — otherwise we would refuse to | |
| 217 | - send a request the server would have honoured. On disk the stale key is left | |
| 218 | - alone rather than deleted: Claude Code reads this file, we cannot see what it | |
| 219 | - does with a missing key, and a too-early expiry merely makes it renew sooner. | |
| 220 | - Between annoying a tool we do not own and possibly breaking it, annoying wins. | |
| 218 | + new refresh token lives, so `refreshTokenExpiresAt` is not modelled at all — | |
| 219 | + not parsed, not written, preserved only as one more key we leave alone. The | |
| 220 | + alternative was worse in both directions: write a guess and we corrupt the | |
| 221 | + meaning of a field Claude Code relies on; keep the old token's expiry beside | |
| 222 | + a rotated token and our own next startup reads it, decides the refresh token | |
| 223 | + is dead, and refuses to send a request the server would have honoured. | |
| 224 | + Instead `Source` asks once and remembers the answer — a refusal sets a flag | |
| 225 | + and every later call returns `ErrExpired` without another round trip. An | |
| 226 | + answer from the server beats a timestamp we would have had to invent. | |
| 221 | 227 | |
| 222 | 228 | Because the endpoint has never once answered, none of this is verified against |
| 223 | 229 | the live service. It is verified against `httptest` and against the real file's |
| @@ -34,7 +34,6 @@ type Credentials struct { | ||
| 34 | 34 | AccessToken string |
| 35 | 35 | RefreshToken string |
| 36 | 36 | ExpiresAt time.Time |
| 37 | - RefreshExpiresAt time.Time | |
| 38 | 37 | Scopes []string |
| 39 | 38 | SubscriptionType string |
| 40 | 39 | } |
| @@ -42,12 +41,11 @@ type Credentials struct { | ||
| 42 | 41 | // stored mirrors the on-disk JSON. Timestamps are epoch milliseconds. |
| 43 | 42 | type stored struct { |
| 44 | 43 | ClaudeAIOAuth struct { |
| 45 | - AccessToken string `json:"accessToken"` | |
| 46 | - RefreshToken string `json:"refreshToken"` | |
| 47 | - ExpiresAt int64 `json:"expiresAt"` | |
| 48 | - RefreshTokenExpiresAt int64 `json:"refreshTokenExpiresAt"` | |
| 49 | - Scopes []string `json:"scopes"` | |
| 50 | - SubscriptionType string `json:"subscriptionType"` | |
| 44 | + AccessToken string `json:"accessToken"` | |
| 45 | + RefreshToken string `json:"refreshToken"` | |
| 46 | + ExpiresAt int64 `json:"expiresAt"` | |
| 47 | + Scopes []string `json:"scopes"` | |
| 48 | + SubscriptionType string `json:"subscriptionType"` | |
| 51 | 49 | } `json:"claudeAiOauth"` |
| 52 | 50 | } |
| 53 | 51 | |
| @@ -73,19 +71,13 @@ func Load(home string) (*Credentials, error) { | ||
| 73 | 71 | if o.ExpiresAt == 0 { |
| 74 | 72 | return nil, fmt.Errorf("claudecode: %s has no expiresAt", path) |
| 75 | 73 | } |
| 76 | - c := &Credentials{ | |
| 74 | + return &Credentials{ | |
| 77 | 75 | AccessToken: o.AccessToken, |
| 78 | 76 | RefreshToken: o.RefreshToken, |
| 79 | 77 | ExpiresAt: time.UnixMilli(o.ExpiresAt), |
| 80 | 78 | Scopes: o.Scopes, |
| 81 | 79 | SubscriptionType: o.SubscriptionType, |
| 82 | - } | |
| 83 | - // Absent means unknown, and unknown must stay the zero Time: UnixMilli(0) | |
| 84 | - // is 1970, which reads as long expired and would refuse to refresh at all. | |
| 85 | - if o.RefreshTokenExpiresAt != 0 { | |
| 86 | - c.RefreshExpiresAt = time.UnixMilli(o.RefreshTokenExpiresAt) | |
| 87 | - } | |
| 88 | - return c, nil | |
| 80 | + }, nil | |
| 89 | 81 | } |
| 90 | 82 | |
| 91 | 83 | // Expired reports whether the access token is past its expiry, or close enough |
| @@ -93,18 +85,3 @@ func Load(home string) (*Credentials, error) { | ||
| 93 | 85 | func (c *Credentials) Expired(now time.Time) bool { |
| 94 | 86 | return !now.Before(c.ExpiresAt.Add(-skew)) |
| 95 | 87 | } |
| 96 | - | |
| 97 | - // RefreshExpired reports whether the refresh token is past its expiry. That is | |
| 98 | - // terminal: no amount of refreshing fixes it, only logging in again. | |
| 99 | - // | |
| 100 | - // A zero expiry means unknown, not expired — the store may not carry the key, | |
| 101 | - // and a rotated token's lifetime is never returned by the exchange. Treating | |
| 102 | - // unknown as dead would refuse to send a request the server would have | |
| 103 | - // honoured, and turn any schema drift into a false "your login has expired". | |
| 104 | - // When we do not know, let the server be the authority: it refuses with a 400. | |
| 105 | - func (c *Credentials) RefreshExpired(now time.Time) bool { | |
| 106 | - if c.RefreshExpiresAt.IsZero() { | |
| 107 | - return false | |
| 108 | - } | |
| 109 | - return !now.Before(c.RefreshExpiresAt) | |
| 110 | - } | |
| @@ -62,9 +62,6 @@ func TestLoadTreatsTimestampsAsMilliseconds(t *testing.T) { | ||
| 62 | 62 | if !c.ExpiresAt.Equal(want) { |
| 63 | 63 | t.Errorf("ExpiresAt = %s, want %s", c.ExpiresAt, want) |
| 64 | 64 | } |
| 65 | - if want := time.UnixMilli(1791404074460); !c.RefreshExpiresAt.Equal(want) { | |
| 66 | - t.Errorf("RefreshExpiresAt = %s, want %s", c.RefreshExpiresAt, want) | |
| 67 | - } | |
| 68 | 65 | } |
| 69 | 66 | |
| 70 | 67 | // Not logged in is not a failure. It is a fall-through to the next source. |
| @@ -128,17 +125,6 @@ func TestExpiredUsesASkewMargin(t *testing.T) { | ||
| 128 | 125 | } |
| 129 | 126 | } |
| 130 | 127 | |
| 131 | - func TestRefreshExpiredIsTerminal(t *testing.T) { | |
| 132 | - base := time.Unix(1_700_000_000, 0) | |
| 133 | - c := &Credentials{RefreshExpiresAt: base} | |
| 134 | - if c.RefreshExpired(base.Add(-24 * time.Hour)) { | |
| 135 | - t.Error("a refresh token with a day left is not expired") | |
| 136 | - } | |
| 137 | - if !c.RefreshExpired(base.Add(time.Hour)) { | |
| 138 | - t.Error("a refresh token past its expiry is expired") | |
| 139 | - } | |
| 140 | - } | |
| 141 | - | |
| 142 | 128 | // A token that has not lapsed is handed straight back, with no refresh and no |
| 143 | 129 | // network of any kind. |
| 144 | 130 | func TestSourceReturnsTheAccessToken(t *testing.T) { |
| @@ -29,12 +29,18 @@ const defaultTokenURL = "https://console.anthropic.com/v1/oauth/token" | ||
| 29 | 29 | // refresh runs under a lock every other request is queued behind. |
| 30 | 30 | const refreshTimeout = 30 * time.Second |
| 31 | 31 | |
| 32 | - // maxErrorBody is how much of a failed response is quoted back. Unbounded, a | |
| 33 | - // wrong endpoint returning a large page becomes the error message; and the | |
| 34 | - // body of a token-endpoint error is a place a submitted credential could | |
| 35 | - // plausibly be echoed, so it is truncated as well as capped. | |
| 32 | + // maxErrorBody is how much of a response is quoted back in an error. The body | |
| 33 | + // of a token-endpoint error is a place a submitted credential could plausibly | |
| 34 | + // be echoed, and a wrong endpoint can return a whole HTML page, so error text | |
| 35 | + // is clipped. This bounds the *message*, never the read: clipping the body we | |
| 36 | + // parse would truncate a perfectly good token response. | |
| 36 | 37 | const maxErrorBody = 512 |
| 37 | 38 | |
| 39 | + // maxBody bounds the read itself, so a misrouted request to something that | |
| 40 | + // streams forever cannot exhaust memory. A token response is a few hundred | |
| 41 | + // bytes; this is four orders of magnitude of headroom. | |
| 42 | + const maxBody = 1 << 20 | |
| 43 | + | |
| 38 | 44 | // tokenURL is the endpoint a refresh is sent to. |
| 39 | 45 | func tokenURL() string { |
| 40 | 46 | if u := os.Getenv(tokenURLEnv); u != "" { |
| @@ -43,9 +49,9 @@ func tokenURL() string { | ||
| 43 | 49 | return defaultTokenURL |
| 44 | 50 | } |
| 45 | 51 | |
| 46 | - // ErrExpired means the credential has lapsed and cannot be renewed here — | |
| 47 | - // the refresh token is missing, itself expired, or was refused. Only logging | |
| 48 | - // in again fixes it, and the tool that owns the credential is the one to do it. | |
| 52 | + // ErrExpired means the credential has lapsed and cannot be renewed here — the | |
| 53 | + // refresh token is missing, or the server refused it. Only logging in again | |
| 54 | + // fixes it, and the tool that owns the credential is the one to do it. | |
| 49 | 55 | var ErrExpired = errors.New("claudecode: your Claude Code login has expired, run `claude` to refresh it") |
| 50 | 56 | |
| 51 | 57 | // Refresh exchanges the refresh token for a new access token. It returns new |
| @@ -81,7 +87,7 @@ func Refresh(ctx context.Context, h *http.Client, c *Credentials) (*Credentials, | ||
| 81 | 87 | return nil, fmt.Errorf("claudecode: refreshing: %w", err) |
| 82 | 88 | } |
| 83 | 89 | defer resp.Body.Close() |
| 84 | - raw, err := io.ReadAll(io.LimitReader(resp.Body, maxErrorBody+1)) | |
| 90 | + raw, err := io.ReadAll(io.LimitReader(resp.Body, maxBody)) | |
| 85 | 91 | if err != nil { |
| 86 | 92 | return nil, fmt.Errorf("claudecode: reading refresh response: %w", err) |
| 87 | 93 | } |
| @@ -114,12 +120,8 @@ func Refresh(ctx context.Context, h *http.Client, c *Credentials) (*Credentials, | ||
| 114 | 120 | next.ExpiresAt = time.Now().Add(time.Duration(out.ExpiresIn) * time.Second) |
| 115 | 121 | // An omitted refresh_token means the old one is still good. Blanking it |
| 116 | 122 | // would throw away the only thing that can refresh again. |
| 117 | - if out.RefreshToken != "" && out.RefreshToken != c.RefreshToken { | |
| 123 | + if out.RefreshToken != "" { | |
| 118 | 124 | next.RefreshToken = out.RefreshToken |
| 119 | - // The response says nothing about how long the NEW refresh token | |
| 120 | - // lives, and carrying the old one's expiry forward would eventually | |
| 121 | - // make a perfectly good token look dead. Unknown is the truth. | |
| 122 | - next.RefreshExpiresAt = time.Time{} | |
| 123 | 125 | } |
| 124 | 126 | return &next, nil |
| 125 | 127 | } |
| @@ -190,16 +192,9 @@ func Save(home string, c *Credentials, expectRefresh string) error { | ||
| 190 | 192 | oauth["accessToken"] = c.AccessToken |
| 191 | 193 | oauth["refreshToken"] = c.RefreshToken |
| 192 | 194 | oauth["expiresAt"] = json.Number(strconv.FormatInt(c.ExpiresAt.UnixMilli(), 10)) |
| 193 | - // A rotated refresh token has an unknown lifetime: the exchange does not | |
| 194 | - // say. In memory that is the zero time, but the stale key is left in place | |
| 195 | - // here rather than deleted. Deleting would be more honest and is the wrong | |
| 196 | - // trade — Claude Code reads this file and we cannot see what it does with | |
| 197 | - // a missing key, whereas a too-early expiry only makes it renew or ask for | |
| 198 | - // a login sooner than it needed to. Between annoying a tool we do not own | |
| 199 | - // and possibly breaking it, annoying wins. | |
| 200 | - if !c.RefreshExpiresAt.IsZero() { | |
| 201 | - oauth["refreshTokenExpiresAt"] = json.Number(strconv.FormatInt(c.RefreshExpiresAt.UnixMilli(), 10)) | |
| 202 | - } | |
| 195 | + // refreshTokenExpiresAt is deliberately not touched. We do not model it and | |
| 196 | + // the exchange never states a rotated token's lifetime, so it survives as | |
| 197 | + // one more key we preserve rather than one we guess at. | |
| 203 | 198 | |
| 204 | 199 | if blob, err = json.Marshal(oauth); err != nil { |
| 205 | 200 | return fmt.Errorf("claudecode: encoding claudeAiOauth: %w", err) |
| @@ -264,6 +259,12 @@ type Source struct { | ||
| 264 | 259 | // home is where a rotated credential is written back. Empty disables |
| 265 | 260 | // write-back entirely. |
| 266 | 261 | home string |
| 262 | + // refused records that the server has already rejected this refresh token. | |
| 263 | + // There is no stored refresh-token expiry to consult — the file's is not | |
| 264 | + // updated when a token rotates, so trusting it would refuse a request the | |
| 265 | + // server would have honoured. Asking once and remembering the answer is | |
| 266 | + // both more accurate and cheaper than guessing from a timestamp. | |
| 267 | + refused bool | |
| 267 | 268 | |
| 268 | 269 | // OnWriteBackFailure is called when the server rotated the refresh token |
| 269 | 270 | // but the new one could not be stored. That combination is the one this |
| @@ -301,17 +302,22 @@ func (s *Source) Token(ctx context.Context) (string, error) { | ||
| 301 | 302 | s.mu.Lock() |
| 302 | 303 | defer s.mu.Unlock() |
| 303 | 304 | |
| 304 | - now := time.Now() | |
| 305 | - if !s.creds.Expired(now) { | |
| 305 | + if !s.creds.Expired(time.Now()) { | |
| 306 | 306 | return s.creds.AccessToken, nil |
| 307 | 307 | } |
| 308 | - if s.creds.RefreshToken == "" || s.creds.RefreshExpired(now) { | |
| 308 | + // Once the server has refused this refresh token, it will refuse it again. | |
| 309 | + // Remembering that is what keeps a dead login from spending a round trip | |
| 310 | + // on every subsequent request. | |
| 311 | + if s.refused || s.creds.RefreshToken == "" { | |
| 309 | 312 | return "", ErrExpired |
| 310 | 313 | } |
| 311 | 314 | |
| 312 | 315 | from := s.creds.RefreshToken |
| 313 | 316 | next, err := Refresh(ctx, s.http, s.creds) |
| 314 | 317 | if err != nil { |
| 318 | + if errors.Is(err, ErrExpired) { | |
| 319 | + s.refused = true | |
| 320 | + } | |
| 315 | 321 | return "", err |
| 316 | 322 | } |
| 317 | 323 | rotated := next.RefreshToken != from |
| @@ -323,7 +329,13 @@ func (s *Source) Token(ctx context.Context) (string, error) { | ||
| 323 | 329 | // file. What would be worse still is saying nothing, because the |
| 324 | 330 | // user's Claude Code login is now broken in a way only this message |
| 325 | 331 | // explains. |
| 326 | - if err := Save(s.home, s.creds, from); err != nil && s.OnWriteBackFailure != nil { | |
| 332 | + // | |
| 333 | + // ErrStoreChanged is the exception, and it is not a failure: it means | |
| 334 | + // something else wrote a newer credential while we ran. Nothing is | |
| 335 | + // broken, nothing is lost, and warning about it would be alarming | |
| 336 | + // about someone else's perfectly good login. | |
| 337 | + err := Save(s.home, s.creds, from) | |
| 338 | + if err != nil && !errors.Is(err, ErrStoreChanged) && s.OnWriteBackFailure != nil { | |
| 327 | 339 | s.OnWriteBackFailure(err) |
| 328 | 340 | } |
| 329 | 341 | } |
| @@ -19,10 +19,9 @@ import ( | ||
| 19 | 19 | // which is the only state in which a refresh should be attempted. |
| 20 | 20 | func liveCreds() *Credentials { |
| 21 | 21 | return &Credentials{ |
| 22 | - AccessToken: "stale", | |
| 23 | - RefreshToken: "r1", | |
| 24 | - ExpiresAt: time.Now().Add(-time.Hour), | |
| 25 | - RefreshExpiresAt: time.Now().Add(24 * time.Hour), | |
| 22 | + AccessToken: "stale", | |
| 23 | + RefreshToken: "r1", | |
| 24 | + ExpiresAt: time.Now().Add(-time.Hour), | |
| 26 | 25 | } |
| 27 | 26 | } |
| 28 | 27 | |
| @@ -169,20 +168,26 @@ func TestSourceConcurrentTokensRefreshOnce(t *testing.T) { | ||
| 169 | 168 | } |
| 170 | 169 | } |
| 171 | 170 | |
| 172 | - // An expired refresh token cannot be refreshed. Sending the request anyway | |
| 173 | - // spends a round trip to be told what we already knew. | |
| 174 | - func TestSourceExpiredRefreshTokenDoesNotCallOut(t *testing.T) { | |
| 171 | + // A refusal is terminal, and asking again on every request would spend a round | |
| 172 | + // trip to be told the same thing. The answer is remembered instead. | |
| 173 | + func TestSourceRemembersARefusal(t *testing.T) { | |
| 174 | + var hits int | |
| 175 | 175 | srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { |
| 176 | - t.Error("no request should be sent for a dead refresh token") | |
| 176 | + hits++ | |
| 177 | + w.WriteHeader(http.StatusBadRequest) | |
| 178 | + io.WriteString(w, `{"error":"invalid_grant"}`) | |
| 177 | 179 | })) |
| 178 | 180 | defer srv.Close() |
| 179 | 181 | t.Setenv(tokenURLEnv, srv.URL) |
| 180 | 182 | |
| 181 | - c := liveCreds() | |
| 182 | - c.RefreshExpiresAt = time.Now().Add(-time.Hour) | |
| 183 | - s := NewSource(c, srv.Client(), "") | |
| 184 | - if _, err := s.Token(context.Background()); !errors.Is(err, ErrExpired) { | |
| 185 | - t.Fatalf("err = %v, want ErrExpired", err) | |
| 183 | + s := NewSource(liveCreds(), srv.Client(), "") | |
| 184 | + for i := 0; i < 4; i++ { | |
| 185 | + if _, err := s.Token(context.Background()); !errors.Is(err, ErrExpired) { | |
| 186 | + t.Fatalf("call %d: err = %v, want ErrExpired", i, err) | |
| 187 | + } | |
| 188 | + } | |
| 189 | + if hits != 1 { | |
| 190 | + t.Errorf("asked the server %d times, want 1 — a refusal should be remembered", hits) | |
| 186 | 191 | } |
| 187 | 192 | } |
| 188 | 193 | |
| @@ -475,29 +480,6 @@ func TestRefreshErrorBodyIsBounded(t *testing.T) { | ||
| 475 | 480 | } |
| 476 | 481 | } |
| 477 | 482 | |
| 478 | - // Rotation gives us a token whose lifetime the response never states. Carrying | |
| 479 | - // the old one's expiry forward would eventually refuse to refresh a perfectly | |
| 480 | - // good token. | |
| 481 | - func TestRotationClearsTheRefreshExpiry(t *testing.T) { | |
| 482 | - srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | |
| 483 | - io.WriteString(w, `{"access_token":"a","refresh_token":"r2","expires_in":28800}`) | |
| 484 | - })) | |
| 485 | - defer srv.Close() | |
| 486 | - t.Setenv(tokenURLEnv, srv.URL) | |
| 487 | - | |
| 488 | - got, err := Refresh(context.Background(), srv.Client(), liveCreds()) | |
| 489 | - if err != nil { | |
| 490 | - t.Fatal(err) | |
| 491 | - } | |
| 492 | - if !got.RefreshExpiresAt.IsZero() { | |
| 493 | - t.Errorf("RefreshExpiresAt = %s, want zero (unknown) after rotation", got.RefreshExpiresAt) | |
| 494 | - } | |
| 495 | - // Unknown must not read as expired, or the next lapse refuses to refresh. | |
| 496 | - if got.RefreshExpired(time.Now()) { | |
| 497 | - t.Error("an unknown refresh expiry must not count as expired") | |
| 498 | - } | |
| 499 | - } | |
| 500 | - | |
| 501 | 483 | // If the user runs `claude` and logs in afresh while we are running, their new |
| 502 | 484 | // credential is strictly better than ours. Overwriting it would undo a login |
| 503 | 485 | // they just performed. |
| @@ -588,45 +570,56 @@ func TestNoRotationReportsNothing(t *testing.T) { | ||
| 588 | 570 | } |
| 589 | 571 | } |
| 590 | 572 | |
| 591 | - // A store with no refreshTokenExpiresAt must not read as permanently expired: | |
| 592 | - // the package doc admits the schema can drift, and drift should not silently | |
| 593 | - // turn refresh into a false "your login has expired". | |
| 594 | - func TestMissingRefreshExpiryIsUnknownNotExpired(t *testing.T) { | |
| 595 | - home := t.TempDir() | |
| 596 | - if err := os.MkdirAll(filepath.Join(home, ".claude"), 0o700); err != nil { | |
| 597 | - t.Fatal(err) | |
| 598 | - } | |
| 599 | - body := `{"claudeAiOauth":{"accessToken":"a","refreshToken":"r","expiresAt":1789668269460}}` | |
| 600 | - if err := os.WriteFile(filepath.Join(home, Filename), []byte(body), 0o600); err != nil { | |
| 601 | - t.Fatal(err) | |
| 573 | + // A real token response is far bigger than any fixture above: two ~120-char | |
| 574 | + // opaque tokens plus whatever else the server includes. An earlier version | |
| 575 | + // bounded the read at the error-clip size, so a realistic body was truncated, | |
| 576 | + // every refresh failed to parse, and the truncated body — containing both | |
| 577 | + // tokens — was printed into the error. Small fixtures hid it completely. | |
| 578 | + func TestRefreshHandlesARealisticallyLargeResponse(t *testing.T) { | |
| 579 | + access := "sk-ant-oat01-" + strings.Repeat("x", 108) | |
| 580 | + refresh := "sk-ant-ort01-" + strings.Repeat("y", 108) | |
| 581 | + body := `{"access_token":"` + access + `","refresh_token":"` + refresh + | |
| 582 | + `","expires_in":28800,"token_type":"Bearer","scope":"user:inference user:profile",` + | |
| 583 | + `"organization":{"uuid":"` + strings.Repeat("0", 36) + `","name":"Some Organisation Name"},` + | |
| 584 | + `"account":{"uuid":"` + strings.Repeat("1", 36) + `","email_address":"someone@example.com"}}` | |
| 585 | + // The bug was that the read was bounded at the error-clip size, so this | |
| 586 | + // fixture is only a regression test while it exceeds that. | |
| 587 | + if len(body) <= maxErrorBody { | |
| 588 | + t.Fatalf("fixture is %d bytes, not more than the %d-byte clip", len(body), maxErrorBody) | |
| 602 | 589 | } |
| 603 | - c, err := Load(home) | |
| 590 | + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | |
| 591 | + io.WriteString(w, body) | |
| 592 | + })) | |
| 593 | + defer srv.Close() | |
| 594 | + t.Setenv(tokenURLEnv, srv.URL) | |
| 595 | + | |
| 596 | + got, err := Refresh(context.Background(), srv.Client(), &Credentials{RefreshToken: "old"}) | |
| 604 | 597 | if err != nil { |
| 605 | - t.Fatal(err) | |
| 598 | + t.Fatalf("a full-size token response must parse: %v", err) | |
| 606 | 599 | } |
| 607 | - if c.RefreshExpired(time.Now()) { | |
| 608 | - t.Error("an absent refreshTokenExpiresAt must mean unknown, not expired") | |
| 600 | + if got.AccessToken != access { | |
| 601 | + t.Errorf("access token was truncated: got %d chars, want %d", len(got.AccessToken), len(access)) | |
| 602 | + } | |
| 603 | + if got.RefreshToken != refresh { | |
| 604 | + t.Errorf("refresh token was truncated: got %d chars, want %d", len(got.RefreshToken), len(refresh)) | |
| 609 | 605 | } |
| 610 | 606 | } |
| 611 | 607 | |
| 612 | - // Write-back must not remove a key from a file Claude Code reads, even one we | |
| 613 | - // can no longer describe accurately after a rotation. | |
| 614 | - func TestSaveNeverDeletesTheRefreshExpiryKey(t *testing.T) { | |
| 615 | - home := storeWith(t, "r1") | |
| 616 | - c, err := Load(home) | |
| 617 | - if err != nil { | |
| 618 | - t.Fatal(err) | |
| 619 | - } | |
| 620 | - c.RefreshToken = "r2-rotated" | |
| 621 | - c.RefreshExpiresAt = time.Time{} // unknown, as after a rotation | |
| 622 | - if err := Save(home, c, "r1"); err != nil { | |
| 623 | - t.Fatalf("Save: %v", err) | |
| 624 | - } | |
| 625 | - oauth := readStore(t, home)["claudeAiOauth"].(map[string]any) | |
| 626 | - if _, ok := oauth["refreshTokenExpiresAt"]; !ok { | |
| 627 | - t.Error("refreshTokenExpiresAt was deleted; a key we did not add is not ours to remove") | |
| 608 | + // And when a large body IS an error, the token must not ride along into the | |
| 609 | + // message wholesale. | |
| 610 | + func TestRefreshErrorDoesNotDumpTheWholeBody(t *testing.T) { | |
| 611 | + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | |
| 612 | + w.WriteHeader(http.StatusBadRequest) | |
| 613 | + io.WriteString(w, `{"error":"invalid_grant","echo":"`+strings.Repeat("Z", 5000)+`"}`) | |
| 614 | + })) | |
| 615 | + defer srv.Close() | |
| 616 | + t.Setenv(tokenURLEnv, srv.URL) | |
| 617 | + | |
| 618 | + _, err := Refresh(context.Background(), srv.Client(), &Credentials{RefreshToken: "r"}) | |
| 619 | + if err == nil { | |
| 620 | + t.Fatal("expected an error") | |
| 628 | 621 | } |
| 629 | - if oauth["refreshToken"] != "r2-rotated" { | |
| 630 | - t.Errorf("refreshToken = %v, want the rotated one", oauth["refreshToken"]) | |
| 622 | + if strings.Count(err.Error(), "Z") > maxErrorBody { | |
| 623 | + t.Errorf("error carried %d bytes of body; it should be clipped to %d", strings.Count(err.Error(), "Z"), maxErrorBody) | |
| 631 | 624 | } |
| 632 | 625 | } |