Clean up orphaned DCR entries when servers are removed from profiles (#411)

## Summary

- **Bug:** When a server with OAuth (DCR) is added to a profile, a DCR entry is registered in Docker Desktop's secure secrets store. When that server is later removed from the profile, the DCR entry is never cleaned up. This causes stale OAuth entries to accumulate in `docker mcp oauth ls` and the Desktop UI OAuth tab. Similarly, `docker mcp oauth revoke` uses a catalog-based `IsRemoteOAuthServer()` check that misses community servers without `oauth.providers` metadata, so their DCR entries are never removed on revoke.
- **Fix (profile removal):** After removing servers from a profile in `RemoveServers()`, check if each removed server still exists in any other profile. If not — and the user is not still authorized — delete its DCR entry via the Desktop API.
- **Fix (revoke):** Replace the catalog-based `IsRemoteOAuthServer` check in `revokeDesktopMode()` with profile-aware cleanup using the same `CleanupOrphanedDCREntries()` logic. This correctly handles both catalog servers and community servers.
- **Changed:** `pkg/workingset/server.go` — added and exported `CleanupOrphanedDCREntries()` called from `RemoveServers()`. `cmd/docker-mcp/oauth/revoke.go` — calls `CleanupOrphanedDCREntries()` after token revocation. Skips CE mode, logs warnings on failure without blocking.

Related pinata change to use mcp CLI: docker/pinata#39417

## Test plan

- [x] Add an OAuth-enabled server to a profile (`docker mcp server add default --server <server>`)
- [x] Verify it appears in `docker mcp oauth ls`
- [x] Remove the server (`docker mcp server remove default <server-name>`)
- [x] Verify the DCR entry is cleaned up — it should no longer appear in `docker mcp oauth ls`
- [x] Add the same server to two profiles, remove from one — verify DCR entry is preserved
- [x] Remove from the second profile — verify DCR entry is now cleaned up
- [x] Authorize a server, then revoke (`docker mcp oauth revoke <server>`) — verify DCR entry is removed if not in any profile
- [x] Authorize a server that is still in a profile, revoke — verify DCR entry is preserved (server still in profile)

```
# After adding sigma-remote to default profile
❯ docker mcp oauth ls
gdrive       | not authorized
github       | not authorized
sigma-remote | not authorized

# Authorize sigma-remote
❯ docker mcp oauth authorize sigma-remote
Opening your browser for authentication. If it doesn't open automatically, please visit: ...

# Confirm sigma-remote is authorized
❯ docker mcp oauth ls
gdrive       | not authorized
github       | not authorized
sigma-remote | authorized

# Remove sigma-remote from default profile
❯ docker mcp profile server remove default --name sigma-remote
Removed 1 server(s) from profile default

# Confirm sigma-remote still shows as authorized
❯ docker mcp oauth ls
gdrive       | not authorized
github       | not authorized
sigma-remote | authorized

# Revoke sigma-remote oauth. Note that is has already been removed from default profile
❯ docker mcp oauth revoke sigma-remote
Revoking OAuth access for sigma-remote...
OAuth access revoked for sigma-remote

# Confirm sigma-remote no longer appears in `docker mcp oauth ls`
❯ docker mcp oauth ls
gdrive | not authorized
github | not authorized
```

Author: jchangx

cmd/docker-mcp/oauth/revoke.go

@@ -4,9 +4,10 @@ import (
 	"context"
 	"fmt"
 
-	"github.com/docker/mcp-gateway/pkg/catalog"
+	"github.com/docker/mcp-gateway/pkg/db"
 	"github.com/docker/mcp-gateway/pkg/desktop"
 	pkgoauth "github.com/docker/mcp-gateway/pkg/oauth"
+	"github.com/docker/mcp-gateway/pkg/workingset"
 )
 
 func Revoke(ctx context.Context, app string) error {
@@ -25,28 +26,21 @@ func Revoke(ctx context.Context, app string) error {
 func revokeDesktopMode(ctx context.Context, app string) error {
 	client := desktop.NewAuthClient()
 
-	// Get catalog to check if this is a remote OAuth server
-	catalogData, err := catalog.GetWithOptions(ctx, true, nil)
-	if err != nil {
-		return fmt.Errorf("failed to get catalog: %w", err)
-	}
-
-	server, found := catalogData.Servers[app]
-	isRemoteOAuth := found && server.IsRemoteOAuthServer()
-
 	// Revoke tokens
 	if err := client.DeleteOAuthApp(ctx, app); err != nil {
 		return fmt.Errorf("failed to revoke OAuth access: %w", err)
 	}
 
-	// For remote OAuth servers, also delete DCR client
-	if isRemoteOAuth {
-		if err := client.DeleteDCRClient(ctx, app); err != nil {
-			return fmt.Errorf("failed to remove DCR client: %w", err)
-		}
+	fmt.Printf("OAuth access revoked for %s\n", app)
+
+	// Clean up DCR entry if the server is not in any profile.
+	// If the server is still in a profile, keep the DCR entry so it
+	// remains visible in the OAuth UI for re-authorization.
+	dao, err := db.New()
+	if err == nil {
+		workingset.CleanupOrphanedDCREntries(ctx, dao, []string{app})
 	}
 
-	fmt.Printf("OAuth access revoked for %s\n", app)
 	return nil
 }
 

pkg/workingset/dcr_cleanup_test.go

@@ -0,0 +1,242 @@
+package workingset
+
+import (
+	"context"
+	"fmt"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+
+	"github.com/docker/mcp-gateway/pkg/catalog"
+	"github.com/docker/mcp-gateway/pkg/db"
+	"github.com/docker/mcp-gateway/pkg/desktop"
+)
+
+// mockDCRClient tracks GetOAuthApp, GetDCRClient, and DeleteDCRClient calls for testing.
+type mockDCRClient struct {
+	// registered holds the set of apps that have DCR entries
+	registered map[string]bool
+	// authorized holds the set of apps where the user is still authorized
+	authorized map[string]bool
+	// deleted tracks which apps had DeleteDCRClient called
+	deleted []string
+}
+
+func newMockDCRClient(apps ...string) *mockDCRClient {
+	m := &mockDCRClient{
+		registered: make(map[string]bool),
+		authorized: make(map[string]bool),
+	}
+	for _, app := range apps {
+		m.registered[app] = true
+	}
+	return m
+}
+
+func (m *mockDCRClient) withAuthorized(apps ...string) *mockDCRClient {
+	for _, app := range apps {
+		m.authorized[app] = true
+	}
+	return m
+}
+
+func (m *mockDCRClient) GetOAuthApp(_ context.Context, app string) (*desktop.OAuthApp, error) {
+	if m.registered[app] {
+		return &desktop.OAuthApp{
+			App:        app,
+			Authorized: m.authorized[app],
+		}, nil
+	}
+	return nil, fmt.Errorf("not found")
+}
+
+func (m *mockDCRClient) GetDCRClient(_ context.Context, app string) (*desktop.DCRClient, error) {
+	if m.registered[app] {
+		return &desktop.DCRClient{ServerName: app, State: "registered"}, nil
+	}
+	return nil, fmt.Errorf("not found")
+}
+
+func (m *mockDCRClient) DeleteDCRClient(_ context.Context, app string) error {
+	delete(m.registered, app)
+	m.deleted = append(m.deleted, app)
+	return nil
+}
+
+func TestCleanupOrphanedDCREntries_DeletesOrphanedAndRevoked(t *testing.T) {
+	dao := setupTestDB(t)
+	ctx := t.Context()
+
+	// Create a profile with one server
+	err := dao.CreateWorkingSet(ctx, db.WorkingSet{
+		ID:   "profile-1",
+		Name: "Profile 1",
+		Servers: db.ServerList{
+			{
+				Type: "remote",
+				Snapshot: &db.ServerSnapshot{
+					Server: catalog.Server{Name: "server-a"},
+				},
+			},
+		},
+		Secrets: db.SecretMap{},
+	})
+	require.NoError(t, err)
+
+	// DCR entries exist for both; server-b is not authorized
+	mock := newMockDCRClient("server-a", "server-b")
+
+	// Remove server-b (not in any profile, not authorized) → should be deleted
+	doCleanupOrphanedDCREntries(ctx, dao, mock, []string{"server-b"})
+
+	assert.Equal(t, []string{"server-b"}, mock.deleted)
+	assert.True(t, mock.registered["server-a"], "server-a should not be deleted")
+}
+
+func TestCleanupOrphanedDCREntries_SkipsAuthorized(t *testing.T) {
+	dao := setupTestDB(t)
+	ctx := t.Context()
+
+	// Empty profile (server was removed)
+	err := dao.CreateWorkingSet(ctx, db.WorkingSet{
+		ID:      "profile-1",
+		Name:    "Profile 1",
+		Servers: db.ServerList{},
+		Secrets: db.SecretMap{},
+	})
+	require.NoError(t, err)
+
+	// server-a has DCR entry and is still authorized
+	mock := newMockDCRClient("server-a").withAuthorized("server-a")
+
+	// server-a is not in any profile but IS authorized → should NOT be deleted
+	doCleanupOrphanedDCREntries(ctx, dao, mock, []string{"server-a"})
+
+	assert.Empty(t, mock.deleted)
+	assert.True(t, mock.registered["server-a"])
+}
+
+func TestCleanupOrphanedDCREntries_SkipsInUse(t *testing.T) {
+	dao := setupTestDB(t)
+	ctx := t.Context()
+
+	// Create two profiles, both containing server-a
+	for _, id := range []string{"profile-1", "profile-2"} {
+		err := dao.CreateWorkingSet(ctx, db.WorkingSet{
+			ID:   id,
+			Name: id,
+			Servers: db.ServerList{
+				{
+					Type: "remote",
+					Snapshot: &db.ServerSnapshot{
+						Server: catalog.Server{Name: "server-a"},
+					},
+				},
+			},
+			Secrets: db.SecretMap{},
+		})
+		require.NoError(t, err)
+	}
+
+	mock := newMockDCRClient("server-a")
+
+	// server-a is still in profile-2, should NOT be deleted
+	doCleanupOrphanedDCREntries(ctx, dao, mock, []string{"server-a"})
+
+	assert.Empty(t, mock.deleted)
+	assert.True(t, mock.registered["server-a"])
+}
+
+func TestCleanupOrphanedDCREntries_SkipsNoDCREntry(t *testing.T) {
+	dao := setupTestDB(t)
+	ctx := t.Context()
+
+	// Empty profile
+	err := dao.CreateWorkingSet(ctx, db.WorkingSet{
+		ID:      "profile-1",
+		Name:    "Profile 1",
+		Servers: db.ServerList{},
+		Secrets: db.SecretMap{},
+	})
+	require.NoError(t, err)
+
+	// No DCR entries registered
+	mock := newMockDCRClient()
+
+	// server-x has no DCR entry → GetDCRClient returns error → skip delete
+	doCleanupOrphanedDCREntries(ctx, dao, mock, []string{"server-x"})
+
+	assert.Empty(t, mock.deleted)
+}
+
+func TestCleanupOrphanedDCREntries_MultipleServers(t *testing.T) {
+	dao := setupTestDB(t)
+	ctx := t.Context()
+
+	// Profile with server-a only
+	err := dao.CreateWorkingSet(ctx, db.WorkingSet{
+		ID:   "profile-1",
+		Name: "Profile 1",
+		Servers: db.ServerList{
+			{
+				Type: "remote",
+				Snapshot: &db.ServerSnapshot{
+					Server: catalog.Server{Name: "server-a"},
+				},
+			},
+		},
+		Secrets: db.SecretMap{},
+	})
+	require.NoError(t, err)
+
+	// DCR entries for all three; server-c is still authorized
+	mock := newMockDCRClient("server-a", "server-b", "server-c").withAuthorized("server-c")
+
+	// Remove server-a (still in profile), server-b (revoked), server-c (authorized)
+	doCleanupOrphanedDCREntries(ctx, dao, mock, []string{"server-a", "server-b", "server-c"})
+
+	// Only server-b should be deleted (server-a in profile, server-c authorized)
+	assert.Equal(t, []string{"server-b"}, mock.deleted)
+	assert.True(t, mock.registered["server-a"])
+	assert.True(t, mock.registered["server-c"])
+}
+
+func TestCleanupOrphanedDCREntries_ServerInDifferentProfile(t *testing.T) {
+	dao := setupTestDB(t)
+	ctx := t.Context()
+
+	// Profile 1 is now empty (server-a and server-b were already removed by RemoveServers)
+	err := dao.CreateWorkingSet(ctx, db.WorkingSet{
+		ID:      "profile-1",
+		Name:    "Profile 1",
+		Servers: db.ServerList{},
+		Secrets: db.SecretMap{},
+	})
+	require.NoError(t, err)
+
+	// Profile 2 still has server-b
+	err = dao.CreateWorkingSet(ctx, db.WorkingSet{
+		ID:   "profile-2",
+		Name: "Profile 2",
+		Servers: db.ServerList{
+			{
+				Type: "remote",
+				Snapshot: &db.ServerSnapshot{
+					Server: catalog.Server{Name: "server-b"},
+				},
+			},
+		},
+		Secrets: db.SecretMap{},
+	})
+	require.NoError(t, err)
+
+	mock := newMockDCRClient("server-a", "server-b")
+
+	// Cleanup after removing server-a and server-b from profile-1
+	// server-b is still in profile-2, so only server-a should be deleted
+	doCleanupOrphanedDCREntries(ctx, dao, mock, []string{"server-a", "server-b"})
+
+	assert.Equal(t, []string{"server-a"}, mock.deleted)
+	assert.True(t, mock.registered["server-b"])
+}

pkg/workingset/server.go

@@ -13,6 +13,9 @@ import (
 
 	"github.com/docker/mcp-gateway/cmd/docker-mcp/secret-management/formatting"
 	"github.com/docker/mcp-gateway/pkg/db"
+	"github.com/docker/mcp-gateway/pkg/desktop"
+	"github.com/docker/mcp-gateway/pkg/log"
+	"github.com/docker/mcp-gateway/pkg/oauth"
 	"github.com/docker/mcp-gateway/pkg/oci"
 	"github.com/docker/mcp-gateway/pkg/policy"
 	policycli "github.com/docker/mcp-gateway/pkg/policy/cli"
@@ -117,17 +120,18 @@ func RemoveServers(ctx context.Context, dao db.DAO, id string, serverNames []str
 		namesToRemove[name] = true
 	}
 
-	originalCount := len(workingSet.Servers)
+	removedNames := make([]string, 0)
 	filtered := make([]Server, 0, len(workingSet.Servers))
 	for _, server := range workingSet.Servers {
 		// TODO: Remove when Snapshot is required
-		if server.Snapshot == nil || !namesToRemove[server.Snapshot.Server.Name] {
+		if server.Snapshot != nil && namesToRemove[server.Snapshot.Server.Name] {
+			removedNames = append(removedNames, server.Snapshot.Server.Name)
+		} else {
 			filtered = append(filtered, server)
 		}
 	}
 
-	removedCount := originalCount - len(filtered)
-	if removedCount == 0 {
+	if len(removedNames) == 0 {
 		return fmt.Errorf("no matching servers found to remove")
 	}
 
@@ -142,11 +146,71 @@ func RemoveServers(ctx context.Context, dao db.DAO, id string, serverNames []str
 		return fmt.Errorf("failed to update profile: %w", err)
 	}
 
-	fmt.Printf("Removed %d server(s) from profile %s\n", removedCount, id)
+	fmt.Printf("Removed %d server(s) from profile %s\n", len(removedNames), id)
+
+	// Clean up DCR entries for removed servers not in any other profile
+	cleanupDCREntriesFunc(ctx, dao, removedNames)
 
 	return nil
 }
 
+// cleanupDCREntriesFunc is called by RemoveServers for DCR cleanup.
+// Tests can override this to verify the call without requiring Docker Desktop.
+var cleanupDCREntriesFunc = CleanupOrphanedDCREntries
+
+// dcrClient abstracts the Desktop API operations needed for cleanup,
+// allowing tests to substitute a mock implementation.
+type dcrClient interface {
+	GetOAuthApp(ctx context.Context, app string) (*desktop.OAuthApp, error)
+	GetDCRClient(ctx context.Context, app string) (*desktop.DCRClient, error)
+	DeleteDCRClient(ctx context.Context, app string) error
+}
+
+// CleanupOrphanedDCREntries removes DCR entries for servers that no longer
+// exist in any profile. This prevents stale OAuth entries from accumulating.
+func CleanupOrphanedDCREntries(ctx context.Context, dao db.DAO, serverNames []string) {
+	if oauth.IsCEMode() {
+		return
+	}
+	doCleanupOrphanedDCREntries(ctx, dao, desktop.NewAuthClient(), serverNames)
+}
+
+func doCleanupOrphanedDCREntries(ctx context.Context, dao db.DAO, client dcrClient, serverNames []string) {
+	allSets, err := dao.ListWorkingSets(ctx)
+	if err != nil {
+		log.Logf("Warning: Failed to list profiles for DCR cleanup: %v", err)
+		return
+	}
+
+	// Build set of all server names still in use across all profiles
+	inUse := make(map[string]bool)
+	for _, ws := range allSets {
+		for _, server := range ws.Servers {
+			if server.Snapshot != nil && server.Snapshot.Server.Name != "" {
+				inUse[server.Snapshot.Server.Name] = true
+			}
+		}
+	}
+
+	for _, name := range serverNames {
+		if inUse[name] {
+			continue
+		}
+		// Only delete if a DCR entry actually exists
+		if _, err := client.GetDCRClient(ctx, name); err != nil {
+			continue
+		}
+		// Keep the DCR entry if the user is still authorized — they may
+		// re-add the server to a profile without needing to re-authorize.
+		if app, err := client.GetOAuthApp(ctx, name); err == nil && app.Authorized {
+			continue
+		}
+		if err := client.DeleteDCRClient(ctx, name); err != nil {
+			log.Logf("Warning: Failed to clean up DCR entry for %s: %v", name, err)
+		}
+	}
+}
+
 type SearchResult struct {
 	ID      string   `json:"id" yaml:"id"`
 	Name    string   `json:"name" yaml:"name"`