From 79117d9930a47f8ffa15f4f982873e72240069cb Mon Sep 17 00:00:00 2001 From: Rob Zolkos Date: Sat, 19 Sep 2026 21:17:07 -0400 Subject: [PATCH 1/2] Persist refused refresh-token suppression --- internal/auth/auth.go | 18 +++++++-- internal/auth/auth_test.go | 79 ++++++++++++++++++++++++-------------- 2 files changed, 65 insertions(+), 32 deletions(-) diff --git a/internal/auth/auth.go b/internal/auth/auth.go index a2a1d59a..1957e64a 100644 --- a/internal/auth/auth.go +++ b/internal/auth/auth.go @@ -486,10 +486,22 @@ func (m *Manager) accountForRefreshFailure(err error, sentRefreshToken string) e // 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 credential is still on disk for the next command to load, so the - // refusal is remembered here instead. + // Keep the process-local guard even if the durable fallback also fails. m.refusedRefreshToken = sentRefreshToken - return errRefusedGrant(delErr) + + // The keyring can occasionally refuse a delete while still allowing a + // write. Replace the dead grant with a signed-out record so another + // process cannot load and submit it again. A later login overwrites this + // through the ordinary save path. + tombstone := &Credentials{} + if saveErr := m.store.save(m.baseURL, tombstone); saveErr != nil { + return errRefusedGrant(errors.Join(delErr, fmt.Errorf("replace refused credential: %w", saveErr))) + } + m.cachedCredentials = cloneCredentials(tombstone) + if m.credentialCleared != nil { + m.credentialCleared() + } + return errRefusedGrant(nil) } m.cachedCredentials = nil // Cached mail must not outlive the credential that fetched it, here as much diff --git a/internal/auth/auth_test.go b/internal/auth/auth_test.go index 2550dd52..e65b40ed 100644 --- a/internal/auth/auth_test.go +++ b/internal/auth/auth_test.go @@ -982,51 +982,65 @@ func TestRefreshForgetsAGrantTheServerRefused(t *testing.T) { } // 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) { +// When the store will not let go of it, an empty credential takes its place so a +// later process cannot load and resend the same dead token. +func TestARefusedGrantIsNotResentByANewManagerWhenItCannotBeDeleted(t *testing.T) { calls := 0 server := httptest.NewServer(invalidGrantHandler(&calls)) defer server.Close() t.Setenv("HEY_TOKEN", "") t.Setenv("HEY_NO_KEYRING", "") - mgr := NewManager(server.URL, server.Client(), t.TempDir()) - - // A keyring that stores and reads but refuses to delete. + configDir := 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) { return stored, nil }, - delete: func(_, _ string) error { return errors.New("keyring is locked") }, - } - - saveExpiredCredential(t, mgr) - - for range 4 { - if _, err := mgr.AccessToken(t.Context()); err == nil { - t.Fatal("AccessToken succeeded with a refused grant") + newManager := func() *Manager { + mgr := NewManager(server.URL, server.Client(), configDir) + 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") }, } + return mgr } - if calls != 1 { - t.Errorf("refresh requests = %d, want 1 — the refusal has to outlive a delete that failed", calls) + first := newManager() + saveExpiredCredential(t, first) + if _, err := first.AccessToken(t.Context()); err == nil { + t.Fatal("first manager accepted a refused grant") } - _, err := mgr.AccessToken(t.Context()) + second := newManager() + _, err := second.AccessToken(t.Context()) var authErr *apierr.Error if !errors.As(err, &authErr) || authErr.Code != apierr.CodeAuth { - t.Errorf("error = %v, want one coded %q", err, apierr.CodeAuth) + t.Fatalf("second manager error = %v, want one coded %q", err, apierr.CodeAuth) + } + if calls != 1 { + t.Errorf("refresh requests = %d, want one across both managers", calls) + } + authenticated, statusErr := second.AuthenticationStatus() + if statusErr != nil { + t.Fatalf("AuthenticationStatus: %v", statusErr) + } + if authenticated { + t.Error("the refused credential still reports authenticated") + } + + if err := second.LoginWithToken("fresh-access"); err != nil { + t.Fatalf("LoginWithToken: %v", err) + } + if token, err := second.AccessToken(t.Context()); err != nil || token != "fresh-access" { + t.Fatalf("AccessToken after login = %q, %v", token, err) } } // 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. +// that fetched it. The hook runs when the credential is deleted or replaced by a +// signed-out record. 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 @@ -1036,7 +1050,7 @@ func TestTheClearedCredentialHookRunsOnlyWhenTheCredentialWent(t *testing.T) { 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: "refused grant replaced after delete fails", status: http.StatusBadRequest, body: `{"error":"invalid_grant"}`, refuseDelete: true, wantRuns: 1}, {name: "rate limited", status: http.StatusTooManyRequests, body: `{"error":"rate_limit_exceeded"}`, wantRuns: 0}, {name: "origin failure", status: http.StatusBadGateway, body: "upstream unavailable", wantRuns: 0}, } @@ -1083,8 +1097,15 @@ func TestTheClearedCredentialHookRunsOnlyWhenTheCredentialWent(t *testing.T) { 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) + var remaining Credentials + if stored != "" { + if err := json.Unmarshal([]byte(stored), &remaining); err != nil { + t.Fatalf("stored credential: %v", err) + } + } + kept := remaining.AccessToken != "" || remaining.SessionCookie != "" + if kept != (tt.wantRuns == 0) { + t.Errorf("usable credential kept = %v; the hook has to run exactly when it is gone", kept) } }) } From c5323cbc0a09785db4ea3d2c274d8b37f42940db Mon Sep 17 00:00:00 2001 From: Rob Zolkos Date: Sat, 19 Sep 2026 21:37:15 -0400 Subject: [PATCH 2/2] Recognize signed-out refresh tombstones --- internal/auth/auth.go | 16 ++++++++++------ internal/auth/auth_test.go | 6 +++++- 2 files changed, 15 insertions(+), 7 deletions(-) diff --git a/internal/auth/auth.go b/internal/auth/auth.go index 1957e64a..afa6fcff 100644 --- a/internal/auth/auth.go +++ b/internal/auth/auth.go @@ -48,9 +48,10 @@ type Manager struct { // 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 runs after the manager has deleted a credential, or + // replaced it with a signed-out record, on its own verdict. The owner of the + // response cache can then drop what that credential fetched. Logout is not + // that: its callers already clear the cache themselves. credentialCleared func() } @@ -312,9 +313,9 @@ func (m *Manager) LoginWithCookie(cookie string) error { } // 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. +// on its own — today, when the server has refused the refresh token. Replacing a +// credential with a signed-out record after deletion fails counts as clearing it. +// Logout does not: its callers clear the cache themselves. func (m *Manager) OnCredentialCleared(fn func()) { m.mu.Lock() defer m.mu.Unlock() @@ -401,6 +402,9 @@ func (m *Manager) refreshLocked(ctx context.Context, creds *Credentials) error { return nil } creds = cloneCredentials(stored) + if creds.AccessToken == "" && creds.RefreshToken == "" && creds.SessionCookie == "" { + return errNoCredential("stored credentials are signed out", nil) + } // Cookie-based auth has nothing to refresh. The fresh read above still lets // a 401 adopt a cookie another process stored before the SDK retries. diff --git a/internal/auth/auth_test.go b/internal/auth/auth_test.go index e65b40ed..b2dac97b 100644 --- a/internal/auth/auth_test.go +++ b/internal/auth/auth_test.go @@ -1007,11 +1007,15 @@ func TestARefusedGrantIsNotResentByANewManagerWhenItCannotBeDeleted(t *testing.T first := newManager() saveExpiredCredential(t, first) + second := newManager() + if authenticated, err := second.AuthenticationStatus(); err != nil || !authenticated { + t.Fatalf("second manager did not cache the original credential: authenticated = %t, error = %v", authenticated, err) + } + if _, err := first.AccessToken(t.Context()); err == nil { t.Fatal("first manager accepted a refused grant") } - second := newManager() _, err := second.AccessToken(t.Context()) var authErr *apierr.Error if !errors.As(err, &authErr) || authErr.Code != apierr.CodeAuth {