Fixing desktop cleanup + other issues

Author: cutecatfann

cmd/docker-mcp/oauth/auth.go

@@ -173,14 +173,6 @@ func authorizeCommunityMode(ctx context.Context, serverName string, scopes strin
 		return fmt.Errorf("docker pass required for community server OAuth: %w", err)
 	}
 
-	// Clean any stale Desktop Secrets Engine entries for this server. If the
-	// server was previously authorized via Desktop OAuth (flag OFF), the
-	// docker-desktop-mcp-oauth plugin holds a token with a more specific
-	// pattern match (docker/mcp/oauth/**) than docker-pass (**), causing
-	// GetSecret to return the stale Desktop value instead of the fresh
-	// docker pass value written below.
-	cleanStaleDesktopEntriesFunc(ctx, serverName)
-
 	// Step 1: Create callback server first — we need its localhost URL for DCR
 	// registration. Community servers reject mcp.docker.com/oauth/callback and
 	// only accept localhost redirect URIs.
@@ -310,6 +302,15 @@ func authorizeCommunityMode(ctx context.Context, serverName string, scopes strin
 		return fmt.Errorf("failed to store token: %w", err)
 	}
 
+	// Step 8: Clean stale Desktop Secrets Engine entries now that the fresh
+	// token is safely stored in docker pass. We defer this until after storage
+	// succeeds so that if the authorize flow fails at any earlier step, the
+	// user retains their existing Desktop authorization as a fallback.
+	// The docker-desktop-mcp-oauth plugin (pattern docker/mcp/oauth/**) has
+	// higher priority than docker-pass (**), so stale Desktop entries would
+	// shadow the fresh token if not removed.
+	cleanStaleDesktopEntriesFunc(ctx, serverName)
+
 	fmt.Printf("Authorization successful! Token stored securely.\n")
 	fmt.Printf("You can now use: docker mcp server start %s\n", serverName)
 

cmd/docker-mcp/oauth/auth_test.go

@@ -141,10 +141,13 @@ func TestAuthorize_CEMode_CommunityServer(t *testing.T) {
 	assert.Equal(t, "ce", *called)
 }
 
-// TestAuthorizeCommunityMode_CleansDesktopEntries verifies that the real
-// authorizeCommunityMode function cleans stale Desktop Secrets Engine
-// entries before proceeding with the Gateway OAuth flow.
-func TestAuthorizeCommunityMode_CleansDesktopEntries(t *testing.T) {
+// TestAuthorizeCommunityMode_NoCleanupOnFailure verifies that
+// authorizeCommunityMode does NOT clean stale Desktop entries when the
+// authorize flow fails before token storage. This ensures the user
+// retains their existing Desktop authorization as a fallback if the
+// community flow fails mid-way (port conflict, user closes browser, etc.).
+// Cleanup only runs after the fresh token is safely stored in docker pass.
+func TestAuthorizeCommunityMode_NoCleanupOnFailure(t *testing.T) {
 	// Save and restore all function pointers touched by this test.
 	oldDesktopCleanup := cleanStaleDesktopEntriesFunc
 	oldCheckPass := checkHasDockerPassFunc
@@ -158,21 +161,20 @@ func TestAuthorizeCommunityMode_CleansDesktopEntries(t *testing.T) {
 	// Mock docker pass check to succeed.
 	checkHasDockerPassFunc = func(_ context.Context) error { return nil }
 
-	// Mock callback server creation to fail — this stops execution right
-	// after cleanup runs, avoiding the need to mock DCR, PKCE, etc.
+	// Mock callback server creation to fail — simulates a mid-flow failure.
 	newCallbackServerFunc = func() (*pkgoauth.CallbackServer, error) {
-		return nil, fmt.Errorf("test: stop after cleanup")
+		return nil, fmt.Errorf("test: port conflict")
 	}
 
-	var desktopCleanupCalled string
-	cleanStaleDesktopEntriesFunc = func(_ context.Context, app string) {
-		desktopCleanupCalled = app
+	var desktopCleanupCalled bool
+	cleanStaleDesktopEntriesFunc = func(_ context.Context, _ string) {
+		desktopCleanupCalled = true
 	}
 
 	// Call the real authorizeCommunityMode directly.
 	err := authorizeCommunityMode(t.Context(), "my-community-server", "")
 	require.Error(t, err)
 	assert.Contains(t, err.Error(), "failed to create callback server")
-	assert.Equal(t, "my-community-server", desktopCleanupCalled,
-		"community authorize should clean stale Desktop entries before proceeding")
+	assert.False(t, desktopCleanupCalled,
+		"community authorize should NOT clean Desktop entries when flow fails before token storage")
 }

cmd/docker-mcp/oauth/cleanup.go

@@ -5,6 +5,7 @@ import (
 
 	"github.com/docker/mcp-gateway/cmd/docker-mcp/secret-management/secret"
 	"github.com/docker/mcp-gateway/pkg/desktop"
+	"github.com/docker/mcp-gateway/pkg/log"
 )
 
 // Function pointers for testability.
@@ -21,7 +22,9 @@ var (
 // (normal case for first-time authorizations or single-store workflows).
 func cleanStaleDesktopEntries(ctx context.Context, app string) {
 	client := desktop.NewAuthClient()
-	_ = client.DeleteOAuthApp(ctx, app)
+	if err := client.DeleteOAuthApp(ctx, app); err != nil {
+		log.Logf("Warning: failed to clean stale Desktop entry for %s: %v", app, err)
+	}
 }
 
 // cleanStaleDockerPassEntries removes OAuth token and DCR client entries
@@ -39,10 +42,14 @@ func cleanStaleDockerPassEntries(ctx context.Context, app string) {
 	dcrKey := secret.GetDCRKey(app)
 	for _, k := range keys {
 		if k == oauthKey {
-			_ = secret.DeleteOAuthToken(ctx, app)
+			if err := secret.DeleteOAuthToken(ctx, app); err != nil {
+				log.Logf("Warning: failed to clean stale docker pass OAuth token for %s: %v", app, err)
+			}
 		}
 		if k == dcrKey {
-			_ = secret.DeleteDCRClient(ctx, app)
+			if err := secret.DeleteDCRClient(ctx, app); err != nil {
+				log.Logf("Warning: failed to clean stale docker pass DCR client for %s: %v", app, err)
+			}
 		}
 	}
 }