Fix auth token refresh by passing client credentials

internal/auth/auth.go

@@ -176,17 +176,63 @@ func (m *Manager) refreshLocked(ctx context.Context, origin string, creds *Crede
 		return output.ErrAuth("No refresh token available")
 	}
 
-	// Use stored token endpoint (survives discovery failures)
+	// Migrate old credentials missing OAuthType
+	if creds.OAuthType == "" {
+		creds.OAuthType = "launchpad"
+	}
+
+	// Migrate old credentials missing TokenEndpoint
+	if creds.TokenEndpoint == "" {
+		if creds.OAuthType == "bc3" {
+			return output.ErrAuth("Stored credentials missing token endpoint — please re-authenticate: basecamp auth login")
+		}
+		lpURL, lpErr := m.launchpadURL()
+		if lpErr != nil {
+			return lpErr
+		}
+		creds.TokenEndpoint = lpURL + "/authorization/token"
+	}
+
 	tokenEndpoint := creds.TokenEndpoint
-	if tokenEndpoint == "" {
-		return output.ErrAuth("No token endpoint stored")
+
+	// Resolve client credentials for the refresh request
+	var clientID, clientSecret string
+	switch creds.OAuthType {
+	case "bc3":
+		cc, err := m.loadBC3Client()
+		if err != nil {
+			if os.IsNotExist(err) {
+				// DCR credentials from custom-redirect logins are intentionally
+				// not persisted (see registerBC3Client). After a process restart
+				// the client.json won't exist and refresh is impossible.
+				return output.ErrAuth("Cannot load BC3 client credentials for token refresh. " +
+					"This can happen after a custom-redirect login (credentials are session-only). " +
+					"Please re-authenticate: basecamp auth login")
+			}
+			return output.ErrAuth(fmt.Sprintf("Cannot load BC3 client credentials for token refresh: %v", err))
+		}
+		clientID = cc.ClientID
+		clientSecret = cc.ClientSecret
+	default:
+		// Launchpad (or old credentials defaulted to launchpad)
+		if envCreds, err := resolveClientCredentials(func(string) {}); err != nil {
+			return err
+		} else if envCreds != nil {
+			clientID = envCreds.ClientID
+			clientSecret = envCreds.ClientSecret
+		} else {
+			clientID = launchpadClientID
+			clientSecret = launchpadClientSecret
+		}
 	}
 
 	exchanger := oauth.NewExchanger(m.httpClient)
 
 	req := oauth.RefreshRequest{
 		TokenEndpoint:   tokenEndpoint,
 		RefreshToken:    creds.RefreshToken,
+		ClientID:        clientID,
+		ClientSecret:    clientSecret,
 		UseLegacyFormat: creds.OAuthType == "launchpad",
 	}
 
@@ -201,6 +247,11 @@ func (m *Manager) refreshLocked(ctx context.Context, origin string, creds *Crede
 	}
 	if !token.ExpiresAt.IsZero() {
 		creds.ExpiresAt = token.ExpiresAt.Unix()
+	} else {
+		// Server didn't return expiry — clear to zero. The existing
+		// contract (auth.go:93) treats ExpiresAt==0 as non-expiring,
+		// so this won't re-trigger refresh on the next call.
+		creds.ExpiresAt = 0
 	}
 
 	return m.store.Save(origin, creds)

internal/auth/auth_test.go

@@ -1342,6 +1342,209 @@ func TestLoginBC3DefaultsToRead(t *testing.T) {
 	assert.Equal(t, "read", creds.Scope, "stored scope should be 'read' for BC3 default")
 }
 
+func TestRefreshLocked_LaunchpadSendsClientID(t *testing.T) {
+	t.Setenv("BASECAMP_OAUTH_CLIENT_ID", "")
+	t.Setenv("BASECAMP_OAUTH_CLIENT_SECRET", "")
+
+	var mu sync.Mutex
+	var capturedBody string
+	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		body, _ := io.ReadAll(r.Body)
+		mu.Lock()
+		capturedBody = string(body)
+		mu.Unlock()
+		w.Header().Set("Content-Type", "application/json")
+		fmt.Fprint(w, `{"access_token":"new-token","refresh_token":"new-refresh"}`)
+	}))
+	defer srv.Close()
+
+	tmpDir := t.TempDir()
+	cfg := config.Default()
+	cfg.BaseURL = srv.URL
+
+	m := &Manager{
+		cfg:        cfg,
+		httpClient: srv.Client(),
+		store:      newTestStore(t, tmpDir),
+	}
+
+	creds := &Credentials{
+		AccessToken:   "old-token",
+		RefreshToken:  "old-refresh",
+		OAuthType:     "launchpad",
+		TokenEndpoint: srv.URL + "/authorization/token",
+		ExpiresAt:     time.Now().Add(-1 * time.Hour).Unix(),
+	}
+	require.NoError(t, m.store.Save(srv.URL, creds))
+
+	err := m.refreshLocked(context.Background(), srv.URL, creds)
+	require.NoError(t, err)
+
+	mu.Lock()
+	body := capturedBody
+	mu.Unlock()
+	assert.Contains(t, body, "client_id="+launchpadClientID)
+	assert.Contains(t, body, "client_secret="+launchpadClientSecret)
+}
+
+func TestRefreshLocked_BC3SendsClientID(t *testing.T) {
+	var mu sync.Mutex
+	var capturedBody string
+	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		body, _ := io.ReadAll(r.Body)
+		mu.Lock()
+		capturedBody = string(body)
+		mu.Unlock()
+		w.Header().Set("Content-Type", "application/json")
+		fmt.Fprint(w, `{"access_token":"new-token","refresh_token":"new-refresh","expires_in":3600}`)
+	}))
+	defer srv.Close()
+
+	tmpDir := t.TempDir()
+	t.Setenv("XDG_CONFIG_HOME", tmpDir)
+
+	cfg := config.Default()
+	cfg.BaseURL = srv.URL
+
+	m := &Manager{
+		cfg:        cfg,
+		httpClient: srv.Client(),
+		store:      newTestStore(t, tmpDir),
+	}
+
+	// Pre-populate client.json
+	require.NoError(t, m.saveBC3Client(&ClientCredentials{
+		ClientID:     "bc3-client-id",
+		ClientSecret: "bc3-client-secret",
+	}))
+
+	creds := &Credentials{
+		AccessToken:   "old-token",
+		RefreshToken:  "old-refresh",
+		OAuthType:     "bc3",
+		TokenEndpoint: srv.URL + "/token",
+		ExpiresAt:     time.Now().Add(-1 * time.Hour).Unix(),
+	}
+	require.NoError(t, m.store.Save(srv.URL, creds))
+
+	err := m.refreshLocked(context.Background(), srv.URL, creds)
+	require.NoError(t, err)
+
+	mu.Lock()
+	body := capturedBody
+	mu.Unlock()
+	assert.Contains(t, body, "client_id=bc3-client-id")
+	assert.Contains(t, body, "client_secret=bc3-client-secret")
+}
+
+func TestRefreshLocked_BC3WithoutClientJSON(t *testing.T) {
+	tmpDir := t.TempDir()
+	t.Setenv("XDG_CONFIG_HOME", tmpDir)
+
+	cfg := config.Default()
+	m := &Manager{
+		cfg:        cfg,
+		httpClient: http.DefaultClient,
+		store:      newTestStore(t, tmpDir),
+	}
+
+	creds := &Credentials{
+		AccessToken:   "old-token",
+		RefreshToken:  "old-refresh",
+		OAuthType:     "bc3",
+		TokenEndpoint: "https://example.com/token",
+		ExpiresAt:     time.Now().Add(-1 * time.Hour).Unix(),
+	}
+
+	err := m.refreshLocked(context.Background(), "test", creds)
+	require.Error(t, err)
+	assert.Contains(t, err.Error(), "Cannot load BC3 client credentials")
+	assert.Contains(t, err.Error(), "custom-redirect")
+}
+
+func TestRefreshLocked_ClearsExpiresAtWhenServerOmits(t *testing.T) {
+	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		w.Header().Set("Content-Type", "application/json")
+		// No expires_in in response
+		fmt.Fprint(w, `{"access_token":"new-token","refresh_token":"new-refresh"}`)
+	}))
+	defer srv.Close()
+
+	tmpDir := t.TempDir()
+	cfg := config.Default()
+	cfg.BaseURL = srv.URL
+
+	m := &Manager{
+		cfg:        cfg,
+		httpClient: srv.Client(),
+		store:      newTestStore(t, tmpDir),
+	}
+
+	creds := &Credentials{
+		AccessToken:   "old-token",
+		RefreshToken:  "old-refresh",
+		OAuthType:     "launchpad",
+		TokenEndpoint: srv.URL + "/authorization/token",
+		ExpiresAt:     time.Now().Add(-1 * time.Hour).Unix(),
+	}
+	require.NoError(t, m.store.Save(srv.URL, creds))
+
+	err := m.refreshLocked(context.Background(), srv.URL, creds)
+	require.NoError(t, err)
+
+	// Reload and verify ExpiresAt is 0 (non-expiring)
+	reloaded, err := m.store.Load(srv.URL)
+	require.NoError(t, err)
+	assert.Equal(t, int64(0), reloaded.ExpiresAt,
+		"ExpiresAt should be 0 when server omits expires_in")
+}
+
+func TestRefreshLocked_EmptyOAuthTypeDefaultsToLaunchpad(t *testing.T) {
+	t.Setenv("BASECAMP_OAUTH_CLIENT_ID", "")
+	t.Setenv("BASECAMP_OAUTH_CLIENT_SECRET", "")
+
+	var mu sync.Mutex
+	var capturedBody string
+	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		body, _ := io.ReadAll(r.Body)
+		mu.Lock()
+		capturedBody = string(body)
+		mu.Unlock()
+		w.Header().Set("Content-Type", "application/json")
+		fmt.Fprint(w, `{"access_token":"new-token","refresh_token":"new-refresh"}`)
+	}))
+	defer srv.Close()
+
+	tmpDir := t.TempDir()
+	cfg := config.Default()
+	cfg.BaseURL = srv.URL
+
+	m := &Manager{
+		cfg:        cfg,
+		httpClient: srv.Client(),
+		store:      newTestStore(t, tmpDir),
+	}
+
+	creds := &Credentials{
+		AccessToken:   "old-token",
+		RefreshToken:  "old-refresh",
+		OAuthType:     "", // Old credentials with no OAuthType
+		TokenEndpoint: srv.URL + "/authorization/token",
+		ExpiresAt:     time.Now().Add(-1 * time.Hour).Unix(),
+	}
+	require.NoError(t, m.store.Save(srv.URL, creds))
+
+	err := m.refreshLocked(context.Background(), srv.URL, creds)
+	require.NoError(t, err)
+
+	mu.Lock()
+	body := capturedBody
+	mu.Unlock()
+	// Should have used launchpad legacy format (type=refresh, not grant_type=refresh_token)
+	assert.Contains(t, body, "type=refresh")
+	assert.Contains(t, body, "client_id="+launchpadClientID)
+}
+
 func TestLoginRejectsInvalidScope(t *testing.T) {
 	tmpDir := t.TempDir()
 	cfg := &config.Config{BaseURL: "https://3.basecampapi.com"}