fix(clients): revoke the client credential on disconnect

CHANGELOG.md

@@ -7,6 +7,14 @@ Releases follow [Semantic Versioning](https://semver.org/).
 
 ### Breaking Changes
 
+- **connect / disconnect:** `mcpproxy disconnect`, `DELETE /api/v1/connect/{client}`, and the Web UI
+  and tray Disconnect now also revoke the client's credential (`client-<id>`), so a secret copied out
+  of the removed config stops authenticating. The result reports `credential_revoked`, or
+  `credential_revoke_error` when the revoke failed after the entry was removed (still HTTP 200 / exit
+  0; retry with `mcpproxy client forget <id>`). Undoing a disconnect restores the file only; a
+  reconnect mints a fresh credential. **Migration:** a script that disconnected a client and kept
+  using its old credential must connect again. (refs #1435, #1451)
+
 - **profiles / tool refusals:** a profile's tool refusal now names the profile to a caller whose
   effective profile is its own (a pin, a client binding, the URL or `set_profile`):
   `blocked by profile: github:create_issue is a write tool; profile "Work Read-only" (work-readonly)

cmd/mcpproxy/connect_cmd.go

@@ -69,6 +69,13 @@ func GetDisconnectCommand() *cobra.Command {
 		Long: `Remove the MCPProxy entry from the specified client's configuration file.
 A backup of the original config file is created before any modification.
 
+Disconnect also revokes the client's credential (token client-<id>): it stops
+authenticating at once, so a secret copied out of the config is dead. Undoing
+a disconnect restores the config entry only; a later "mcpproxy connect" mints
+a new credential. A client with no credential just loses its entry. If the
+entry is removed but the revoke fails, the command says so and
+"mcpproxy client forget <client>" retries it.
+
 Examples:
   mcpproxy disconnect claude-code
   mcpproxy disconnect cursor --name my-proxy`,
@@ -142,20 +149,25 @@ func runDisconnect(cmd *cobra.Command, args []string) error {
 		return fmt.Errorf("failed to load config: %w", err)
 	}
 
-	svc := connect.NewService(cfg.Listen, cfg.APIKey).WithRequireMCPAuth(cfg.RequireMCPAuth)
-
 	format := clioutput.ResolveFormat(globalOutputFormat, globalJSONOutput)
 	formatter, err := clioutput.NewFormatter(format)
 	if err != nil {
 		return err
 	}
 
 	clientID := args[0]
-	result, err := svc.Disconnect(clientID, connectServerName)
+	// Same daemon/offline split as connect: the daemon revokes the credential
+	// and records presence; offline revokes over config.db directly.
+	backend, err := newConnectBackend(cfg)
+	if err != nil {
+		return describeConnectFailure(err, clientID)
+	}
+	defer backend.close()
+	result, err := backend.disconnect(clientID, connectServerName)
 	if err != nil {
 		return err
 	}
-	if result.Success {
+	if result.Success && !backend.viaDaemon() {
 		notifyClientDisconnected(cfg, clientID)
 	}
 
@@ -332,6 +344,9 @@ func printConnectResult(result *connect.ConnectResult, formatter clioutput.Outpu
 			if line := connectCredentialLine(result); line != "" {
 				fmt.Println(line)
 			}
+			if line := disconnectCredentialLine(result); line != "" {
+				fmt.Println(line)
+			}
 			// FR-037: Config shows the home-shortened display_path; the full
 			// path is still available via -o json's config_path.
 			fmt.Printf("Config: %s\n", connectResultDisplayPath(result))
@@ -355,6 +370,22 @@ func printConnectResult(result *connect.ConnectResult, formatter clioutput.Outpu
 	return connectResultError(result)
 }
 
+// disconnectCredentialLine reports what a disconnect did to the client
+// credential. Empty for a connect result (CredentialRevoked is also set by an
+// undo, which prints its own line elsewhere) and for a not-found disconnect.
+func disconnectCredentialLine(r *connect.ConnectResult) string {
+	switch {
+	case r.Action != "removed":
+		return ""
+	case r.CredentialRevokeError != "":
+		return fmt.Sprintf("Credential: NOT revoked (%s); retry with: mcpproxy client forget %s", r.CredentialRevokeError, r.Client)
+	case r.CredentialRevoked != "":
+		return fmt.Sprintf("Credential: revoked (token %s)", r.CredentialRevoked)
+	default:
+		return "Credential: none to revoke"
+	}
+}
+
 // connectResultError turns a refused connect/disconnect result into a non-nil
 // error so the exit code is 1; stdout is already printed unchanged. nil for a
 // successful result or an already_exists no-op.

cmd/mcpproxy/connect_credential.go

@@ -77,6 +77,9 @@ func connectIntentFromFlags(cmd *cobra.Command) (connect.CredentialIntent, error
 // connectBackend performs a connect write for the CLI.
 type connectBackend interface {
 	connect(clientID, serverName string, force bool, intent connect.CredentialIntent) (*connect.ConnectResult, error)
+	// disconnect removes the entry and revokes the client's credential
+	// (CredentialRevoked / CredentialRevokeError on the result).
+	disconnect(clientID, serverName string) (*connect.ConnectResult, error)
 	close()
 	// viaDaemon reports whether the daemon already recorded the connection
 	// (so the CLI must not relay it again).
@@ -128,6 +131,41 @@ func (d *daemonConnectBackend) connect(clientID, serverName string, force bool,
 	return parseConnectResponse(resp.StatusCode, raw, clientID)
 }
 
+// disconnect asks the daemon to remove the entry; the daemon also revokes the
+// client credential and records the presence change itself.
+func (d *daemonConnectBackend) disconnect(clientID, serverName string) (*connect.ConnectResult, error) {
+	body, err := json.Marshal(connectRequestBody{ServerName: serverName})
+	if err != nil {
+		return nil, err
+	}
+	ctx, cancel := context.WithTimeout(context.Background(), 60*time.Second)
+	defer cancel()
+	resp, err := d.client.DoRaw(ctx, http.MethodDelete, "/api/v1/connect/"+clientID, body)
+	if err != nil {
+		return nil, fmt.Errorf("disconnect request failed: %w", err)
+	}
+	defer resp.Body.Close()
+	raw, err := io.ReadAll(resp.Body)
+	if err != nil {
+		return nil, err
+	}
+	return parseDisconnectResponse(resp.StatusCode, raw, clientID)
+}
+
+// parseDisconnectResponse maps a DELETE /api/v1/connect/{client} answer. A 404
+// for a known client is the "entry not found" result (exit 0, like the local
+// path); a 404 for an unknown client stays an error.
+func parseDisconnectResponse(status int, raw []byte, clientID string) (*connect.ConnectResult, error) {
+	if status == http.StatusNotFound && connect.FindClient(clientID) != nil {
+		var env struct {
+			Error string `json:"error"`
+		}
+		_ = json.Unmarshal(raw, &env)
+		return &connect.ConnectResult{Client: clientID, Action: "not_found", Message: env.Error}, nil
+	}
+	return parseConnectResponse(status, raw, clientID)
+}
+
 // parseConnectResponse maps a POST /api/v1/connect/{client} answer to a result
 // or to the same typed errors the local path returns, so both render alike.
 func parseConnectResponse(status int, raw []byte, clientID string) (*connect.ConnectResult, error) {
@@ -217,6 +255,34 @@ func (o *offlineConnectBackend) connect(clientID, serverName string, force bool,
 	return o.svc.ConnectWithOptions(clientID, serverName, connect.ConnectOptions{Force: force, Intent: intent})
 }
 
+// disconnect removes the entry, then revokes the client credential. Like the
+// daemon, a revoke failure never turns a completed disconnect into an error.
+func (o *offlineConnectBackend) disconnect(clientID, serverName string) (*connect.ConnectResult, error) {
+	result, err := o.svc.Disconnect(clientID, serverName)
+	if err != nil || result == nil || !result.Success || !auth.ValidClientID(clientID) {
+		return result, err
+	}
+	cred, gerr := o.clients.Get(clientID)
+	if gerr != nil {
+		result.CredentialRevokeError = gerr.Error()
+		return result, nil
+	}
+	if cred == nil {
+		return result, nil
+	}
+	actor := runtime.Actor{Kind: "cli_offline", Surface: profile.SurfaceCLI}
+	view, ferr := o.clients.Forget(context.Background(), actor, clientID, true)
+	var none *runtime.NoClientCredentialError
+	switch {
+	case ferr == nil:
+		result.CredentialRevoked = view.TokenName
+	case errors.As(ferr, &none):
+	default:
+		result.CredentialRevokeError = ferr.Error()
+	}
+	return result, nil
+}
+
 // --- errors and rendering -------------------------------------------------
 
 // connectConflictError is the name-conflict refusal: client-<id> is held by a

cmd/mcpproxy/connect_disconnect_revoke_test.go

@@ -0,0 +1,95 @@
+package main
+
+import (
+	"io"
+	"net/http"
+	"net/http/httptest"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+	"go.uber.org/zap"
+
+	"github.com/smart-mcp-proxy/mcpproxy-go/internal/cliclient"
+	"github.com/smart-mcp-proxy/mcpproxy-go/internal/storage"
+)
+
+func runDisconnectArgs(t *testing.T, args ...string) (string, error) {
+	t.Helper()
+	var runErr error
+	out := captureStdout(t, func() {
+		cmd := GetDisconnectCommand()
+		cmd.SetArgs(args)
+		cmd.SilenceUsage, cmd.SilenceErrors = true, true
+		runErr = cmd.Execute()
+	})
+	return out, runErr
+}
+
+func clientCredentialRevoked(t *testing.T, dataDir string) bool {
+	t.Helper()
+	sm, err := storage.NewManager(dataDir, zap.NewNop().Sugar())
+	require.NoError(t, err)
+	defer func() { _ = sm.Close() }()
+	toks, err := sm.ListAgentTokens()
+	require.NoError(t, err)
+	for _, tk := range toks {
+		if tk.Name == "client-cursor" {
+			return tk.Revoked
+		}
+	}
+	return true // no record at all: nothing live
+}
+
+// Offline (no daemon) disconnect removes the entry and revokes the client
+// credential over config.db (#1435).
+func TestDisconnect_OfflineRevokesTheClientCredential(t *testing.T) {
+	home, cfg := connectTestEnv(t, true)
+	seedCursor(t, home)
+	out, err := runConnectArgs(t, "cursor", "--profile", "ro")
+	require.NoError(t, err, out)
+	resetConnectFlagValues()
+	require.False(t, clientCredentialRevoked(t, cfg.DataDir), "connect minted a live credential")
+
+	out, err = runDisconnectArgs(t, "cursor")
+	require.NoError(t, err, out)
+	resetConnectFlagValues()
+	assert.Contains(t, out, "Credential: revoked (token client-cursor)")
+	assert.True(t, clientCredentialRevoked(t, cfg.DataDir))
+
+	// Disconnecting again: nothing to remove, exit 0, no credential line.
+	out, err = runDisconnectArgs(t, "cursor")
+	require.NoError(t, err, out)
+	resetConnectFlagValues()
+	assert.NotContains(t, out, "Credential:")
+}
+
+// With a daemon, disconnect goes through DELETE /api/v1/connect/{client} so the
+// daemon revokes and records presence itself.
+func TestDisconnect_DaemonBackendIssuesDelete(t *testing.T) {
+	var method, path, body string
+	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		raw, _ := io.ReadAll(r.Body)
+		method, path, body = r.Method, r.URL.Path, string(raw)
+		w.Header().Set("Content-Type", "application/json")
+		_, _ = w.Write([]byte(`{"success":true,"data":{"success":true,"client":"cursor","action":"removed","message":"ok","credential_revoked":"client-cursor"}}`))
+	}))
+	defer srv.Close()
+
+	b := &daemonConnectBackend{client: cliclient.NewClientWithAPIKey(srv.URL, "k", zap.NewNop().Sugar())}
+	res, err := b.disconnect("cursor", "my-proxy")
+	require.NoError(t, err)
+	assert.Equal(t, http.MethodDelete, method)
+	assert.Equal(t, "/api/v1/connect/cursor", path)
+	assert.JSONEq(t, `{"server_name":"my-proxy"}`, body)
+	assert.Equal(t, "client-cursor", res.CredentialRevoked)
+	assert.Equal(t, "Credential: revoked (token client-cursor)", disconnectCredentialLine(res))
+}
+
+func TestParseDisconnectResponse_NotFoundIsAResultForAKnownClient(t *testing.T) {
+	res, err := parseDisconnectResponse(http.StatusNotFound, []byte(`{"error":"no entry"}`), "cursor")
+	require.NoError(t, err)
+	assert.Equal(t, "not_found", res.Action)
+	_, err = parseDisconnectResponse(http.StatusNotFound, []byte(`{"error":"unknown client"}`), "nope")
+	require.Error(t, err)
+}

docs/api/rest-api.md

@@ -1127,7 +1127,15 @@ curl "http://127.0.0.1:8080/api/v1/connect/claude-desktop?apikey=your-api-key"
 
 #### POST/DELETE /api/v1/connect/{client}
 
-Connect/disconnect are unchanged except that a permission-denied config access
+`DELETE` removes the entry **and revokes the client's credential** (the
+`client-<id>` token stops authenticating at once, a `forget` change record with
+`disconnected: true` is written). The result names it in `credential_revoked`;
+a client with no credential leaves the field empty. If the entry was removed but
+the revoke failed, the response is still `200`: `credential_revoke_error` carries
+the reason and `DELETE /api/v1/clients/{client}` retries the revoke. Undoing a
+disconnect (below) restores the file only: the credential stays revoked and a
+later connect mints a new one. Apart from that, connect/disconnect are unchanged
+except that a permission-denied config access
 now returns **`403 Forbidden`** whose error body carries the remediation text
 (distinct from a generic `400` or a `404` not-found).
 
@@ -1165,8 +1173,9 @@ and threat model.
 
 **Client credential (Spec 108).** The body also accepts `profile` (a profile
 name; `""` is All servers; omitted means All servers for a fresh credential and
-the existing binding on a reconnect, including over an expired credential; a
-revoked one restarts at All servers), `mode` (`locked` or `switchable`) and
+the existing binding on a reconnect, including over an expired or revoked
+credential: a revoked record keeps its prior pin and mode, so a profile-locked
+client is never silently widened to All servers), `mode` (`locked` or `switchable`) and
 `keyless`. The write embeds a per-client `mcp_cli_` credential, never the admin
 API key, and the result carries `credential` (masked), `token_name`, `profile`,
 `mode`, `keyless` and, for a reconnect over an active credential, `rotation`

docs/cli/profile-commands.md

@@ -74,7 +74,9 @@ A write that would let a client bound to a profile escape it while `require_mcp_
 | `client add <id> [--display-name N] [--profile P] [--lock\|--switchable] [--expires-in 90d]` | A custom client (a script, a CI job) with its own credential, printed once with a header snippet |
 | `client rotate <id> [--yes]` / `--finalize` | Replace a credential without cutting the client off. A supported client previews the config change and asks to confirm (`--yes` for scripts; a non-interactive run without it exits 1); a custom client gets the new secret once and stays pending until `--finalize` or 24 hours |
 | `client upgrade-admin-key-holders [--profile P\|all] [--lock\|--switchable] [--yes]` | Replace the admin API key in every supported client's config with a per-client credential, after a preview. With `--profile` and `require_mcp_auth` off the guard can refuse: the command prints the preview and the refusal, exits 1 and changes nothing. It ends with the step that remains: rotate the admin API key |
-| `client forget <id> [--disconnect]` | Revoke the credential; with `--disconnect` also remove the config entry of a supported client |
+| `client forget <id> [--disconnect]` | Revoke the credential without touching the client's config; with `--disconnect` also remove the config entry of a supported client |
+
+`mcpproxy disconnect <client>` is the reverse of `connect`: it removes the config entry **and revokes the client's credential** (`client-<id>`), so a secret copied out of the config stops authenticating at once. With a running daemon the command calls `DELETE /api/v1/connect/{client}`; without one it revokes over `config.db`. It prints `Credential: revoked (token client-cursor)`, or `Credential: none to revoke` for a client that had none. If the entry was removed but the revoke failed, the command still exits 0, prints `Credential: NOT revoked (...)` and the fix is `mcpproxy client forget <id>`. Undoing a disconnect restores the config file only; the credential stays revoked and a later `connect` mints a new one. The revoked record keeps the client's prior profile pin and mode, so a `connect` that names no profile re-mints with that binding (a profile-locked client is never silently widened to All servers, and the reconnect preview shows it); pass `--profile` to choose a different one. With `require_mcp_auth` off, a named-profile binding is refused by the bypass guard (`binding_bypassable_without_auth`); enable `require_mcp_auth` or choose `--profile all` explicitly. The credential belongs to the client, so `--name <entry>` for a non-default entry revokes it too.
 
 A client without an active client credential exits 1 with `mcpproxy connect <id> --profile <p>` as the fix.
 

frontend/src/components/ClientConnectList.vue

@@ -502,6 +502,10 @@
             A timestamped backup of the file is written first, and the path is shown afterwards
             so you can restore it.
           </p>
+          <p class="text-sm text-base-content/70 mt-2" data-test="connect-disconnect-revokes">
+            The client's credential is revoked too. Restoring the file does not bring it back;
+            connect again to issue a new one.
+          </p>
           <div class="modal-action">
             <button class="btn btn-ghost btn-sm" data-test="connect-disconnect-cancel" @click="disconnectTarget = null">Cancel</button>
             <button
@@ -1108,16 +1112,27 @@ async function disconnect(clientId: string) {
     const client = clients.value.find(c => c.id === clientId)
     const response = await api.disconnectClient(clientId, client?.server_name || 'mcpproxy')
     if (response.success && response.data) {
+      const revokeError = response.data.credential_revoke_error
       resultMessage.value = response.data.message || `Disconnected from ${clientId}`
-      resultSuccess.value = true
+      if (revokeError) {
+        // The entry is gone but the credential is still live: say so, with the retry.
+        resultMessage.value += `. Its credential was NOT revoked (${revokeError}); retry with: mcpproxy client forget ${clientId}`
+      }
+      resultSuccess.value = !revokeError
       resultBackupPath.value = response.data.backup_path || null
       resultReloadHint.value = response.data.reload_hint || ''
       await refreshAfterWrite(clientId)
-      systemStore.addToast({
-        type: 'info',
-        title: 'Client Disconnected',
-        message: `MCPProxy removed from ${clientId}`,
-      })
+      systemStore.addToast(revokeError
+        ? {
+            type: 'warning',
+            title: 'Disconnected, credential still active',
+            message: `MCPProxy removed from ${clientId}, but its credential was not revoked`,
+          }
+        : {
+            type: 'info',
+            title: 'Client Disconnected',
+            message: `MCPProxy removed from ${clientId}`,
+          })
     } else {
       resultMessage.value = response.error || 'Failed to disconnect'
       resultSuccess.value = false

frontend/src/types/api.ts

@@ -1317,14 +1317,16 @@ export interface ConnectResult {
   // (`mcp_cli_••••`, never the secret), its token name and binding. Empty for a
   // keyless entry. `rotation` is 'finalized' when a reconnect replaced an
   // active credential's secret; `credential_revoked` names the credential an
-  // undo revoked.
+  // undo or a disconnect revoked; `credential_revoke_error` is set when a
+  // disconnect removed the entry but the revoke failed.
   credential?: string
   token_name?: string
   profile?: string
   mode?: 'locked' | 'switchable'
   keyless?: boolean
   rotation?: string
   credential_revoked?: string
+  credential_revoke_error?: string
 }
 
 // Spec 078 US1: the exact change a connect would make, returned WITHOUT writing