From 9dda5e5aa554ebd2b54a27c5efffdfe062a47189 Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Sat, 12 Sep 2026 18:26:40 -0700 Subject: [PATCH] Revoke the grant a refused login leaves behind MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A login refused by its Verify hook — an --expect-identity mismatch above all — correctly stored nothing, but the full-scope refresh and access tokens the server had just issued stayed live with no local record of them: an orphan grant only the server's session list could find. The smoke test flagged it as a live orphan grant needing operator action. The refused credential is now revoked before the refusal is returned, on both the device and Launchpad flows (a no-op on Launchpad, which has no revocation endpoint). The refusal stays the error the caller sees; a failed revocation adds one warning line so the operator knows a live token is out there until it expires. --- internal/auth/auth.go | 2 + internal/auth/revoke.go | 17 ++++ internal/auth/revoke_test.go | 147 +++++++++++++++++++++++++++++++++++ 3 files changed, 166 insertions(+) diff --git a/internal/auth/auth.go b/internal/auth/auth.go index b4806104..a404f594 100644 --- a/internal/auth/auth.go +++ b/internal/auth/auth.go @@ -612,6 +612,7 @@ func (m *Manager) loginLaunchpad(ctx context.Context, credKey string, oauthCfg * if opts.Verify != nil { if err := opts.Verify(ctx, creds.AccessToken, oauthTypeLaunchpad); err != nil { + m.discardGrant(ctx, creds, opts.log) return nil, err } } @@ -758,6 +759,7 @@ func (m *Manager) loginDevice(ctx context.Context, credKey string, oauthCfg *oau if opts.Verify != nil { if err := opts.Verify(ctx, creds.AccessToken, oauthTypeBC5); err != nil { + m.discardGrant(ctx, creds, opts.log) return nil, err } } diff --git a/internal/auth/revoke.go b/internal/auth/revoke.go index 979ac503..de1865dc 100644 --- a/internal/auth/revoke.go +++ b/internal/auth/revoke.go @@ -13,6 +13,7 @@ import ( "time" "github.com/basecamp/basecamp-cli/internal/output" + "github.com/basecamp/basecamp-cli/internal/richtext" ) // revokeRequestTimeout bounds each revocation round trip — the metadata @@ -220,6 +221,22 @@ func statusFailure(msg string, resp *http.Response) error { } } +// discardGrant revokes a freshly minted credential the login refused to +// store (a Verify failure). Without it the grant would stay live and +// full-scope with no local record of it. The refusal is the error the +// caller sees; a failed revocation is logged, with what it left usable, +// so the operator knows a live token is out there. +// +// The cleanup runs detached from the login's context: a Verify that failed +// because that context was canceled or timed out is exactly the case where +// the grant would otherwise be orphaned, and the per-request timeouts in +// revoke still bound the calls. +func (m *Manager) discardGrant(ctx context.Context, creds *Credentials, log func(string)) { + if result := m.revokeForDiscard(context.WithoutCancel(ctx), creds, ""); result.Err != nil { + log(richtext.SanitizeSingleLine("warning: could not revoke the refused credential server-side: " + result.Err.Error() + " — " + result.Outstanding())) + } +} + // revokeSkipReason says why creds are not the CLI's to revoke, or "" when // they are: a BC5 credential minted by an OAuth login. func revokeSkipReason(creds *Credentials) string { diff --git a/internal/auth/revoke_test.go b/internal/auth/revoke_test.go index 3fe8cc31..c4ab207b 100644 --- a/internal/auth/revoke_test.go +++ b/internal/auth/revoke_test.go @@ -4,8 +4,12 @@ import ( "context" "errors" "fmt" + "io" "net/http" + "net/http/httptest" "net/url" + "strings" + "sync" "testing" "time" @@ -388,6 +392,149 @@ func TestLoginDevice_RecordsTheIssuer(t *testing.T) { assert.Empty(t, as.revokeCalls(), "an accepted login revokes nothing") } +// A login refused by Verify (an --expect-identity mismatch) stores nothing, +// and now also leaves nothing live: the grant it minted is revoked before +// the refusal is returned. +func TestLoginDevice_VerifyFailureRevokesTheGrant(t *testing.T) { + as := startDeviceAS(t) + resource := startResourceServer(t, as.srv.URL) + m := newDeviceTestManager(t, resource.URL) + credKey := config.NormalizeBaseURL(resource.URL) + + cl := &collectLogger{} + _, err := m.Login(context.Background(), LoginOptions{ + Remote: true, + Logger: cl.log, + deviceOptions: []oauth.DeviceOption{instantSleep()}, + Verify: func(context.Context, string, string) error { return output.ErrAuth("not you") }, + }) + require.Error(t, err) + assert.Contains(t, err.Error(), "not you", "the refusal stays the error the caller sees") + assertRevoked(t, as.revokeCalls(), "dev-ref", "dev-tok") + assert.NotContains(t, cl.joined(), "warning") + _, loadErr := m.store.Load(credKey) + assert.Error(t, loadErr, "a rejected token is never stored") +} + +func TestLoginDevice_VerifyFailureWarnsWhenTheGrantOutlivesIt(t *testing.T) { + as := startDeviceAS(t) + as.revoke = func(int) (int, string) { return http.StatusInternalServerError, `{}` } + resource := startResourceServer(t, as.srv.URL) + m := newDeviceTestManager(t, resource.URL) + + cl := &collectLogger{} + _, err := m.Login(context.Background(), LoginOptions{ + Remote: true, + Logger: cl.log, + deviceOptions: []oauth.DeviceOption{instantSleep()}, + Verify: func(context.Context, string, string) error { return output.ErrAuth("not you") }, + }) + require.Error(t, err) + assert.Contains(t, err.Error(), "not you") + assert.Contains(t, cl.joined(), "warning: could not revoke the refused credential server-side") + assert.Contains(t, cl.joined(), "refresh token stays valid until it is revoked") + assert.NotContains(t, cl.joined(), "dev-ref") + assert.NotContains(t, cl.joined(), "dev-tok") +} + +// A refusal after the login context is gone — Verify timed out or was +// canceled — must still revoke: that is exactly when the grant would +// otherwise be orphaned. +func TestLoginDevice_VerifyFailureRevokesAfterCancellation(t *testing.T) { + as := startDeviceAS(t) + resource := startResourceServer(t, as.srv.URL) + m := newDeviceTestManager(t, resource.URL) + + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + cl := &collectLogger{} + _, err := m.Login(ctx, LoginOptions{ + Remote: true, + Logger: cl.log, + deviceOptions: []oauth.DeviceOption{instantSleep()}, + Verify: func(context.Context, string, string) error { + cancel() + return output.ErrAuth("not you") + }, + }) + require.Error(t, err) + assert.Contains(t, err.Error(), "not you") + assertRevoked(t, as.revokeCalls(), "dev-ref", "dev-tok") + assert.NotContains(t, cl.joined(), "warning") +} + +// A refused Launchpad login stores nothing and revokes nothing: Launchpad +// has no revocation endpoint, so no request leaves and no warning prints. +func TestLoginLaunchpad_VerifyFailureRevokesNothing(t *testing.T) { + var mu sync.Mutex + var paths []string + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + mu.Lock() + paths = append(paths, r.URL.Path) + mu.Unlock() + switch r.URL.Path { + case "/authorization/token": + w.Header().Set("Content-Type", "application/json") + fmt.Fprint(w, `{"access_token":"remote-tok","token_type":"bearer","refresh_token":"remote-refresh"}`) + default: + http.NotFound(w, r) + } + })) + defer srv.Close() + + tmpDir := t.TempDir() + t.Setenv("XDG_CONFIG_HOME", tmpDir) + t.Setenv("BASECAMP_LAUNCHPAD_URL", srv.URL) + t.Setenv("BASECAMP_OAUTH_ISSUER", "") + cfg := &config.Config{BaseURL: srv.URL} + m := NewManager(cfg, srv.Client()) + m.store = newTestStore(t, tmpDir) + credKey := config.NormalizeBaseURL(srv.URL) + + sl := newSyncLogger() + pr, pw := io.Pipe() + defer pr.Close() + errCh := make(chan error, 1) + go func() { + _, err := m.Login(context.Background(), LoginOptions{ + Remote: true, + Logger: sl.log, + InputReader: pr, + Verify: func(context.Context, string, string) error { return output.ErrAuth("not you") }, + }) + errCh <- err + }() + + var authURL string + select { + case authURL = <-sl.authReady: + case <-time.After(5 * time.Second): + t.Fatal("timed out waiting for auth URL to be logged") + } + u, err := url.Parse(authURL) + require.NoError(t, err) + _, err = fmt.Fprintf(pw, "http://127.0.0.1:8976/callback?code=test-code&state=%s\n", u.Query().Get("state")) + require.NoError(t, err) + pw.Close() + + select { + case err := <-errCh: + require.Error(t, err) + assert.Contains(t, err.Error(), "not you") + case <-time.After(5 * time.Second): + t.Fatal("Login timed out") + } + + _, loadErr := m.store.Load(credKey) + assert.Error(t, loadErr, "a rejected token is never stored") + assert.NotContains(t, strings.Join(sl.snapshot(), "\n"), "could not revoke") + mu.Lock() + defer mu.Unlock() + assert.Contains(t, paths, "/authorization/token") + assert.NotContains(t, paths, "/.well-known/oauth-authorization-server", "no revocation metadata is fetched for a Launchpad grant") + assert.NotContains(t, paths, "/oauth/revocations") +} + func TestRevokeStored_RevokesThenDeletes(t *testing.T) { as := startDeviceAS(t) m := newDeviceTestManager(t, as.srv.URL)