From 6edcbb4f664d2aae6c88692afc1ec2a98f793106 Mon Sep 17 00:00:00 2001 From: Algis Dumbris Date: Mon, 5 Oct 2026 20:29:01 +0300 Subject: [PATCH 1/5] fix(clients): revoke the client credential on disconnect DELETE /api/v1/connect/{client} and mcpproxy disconnect (daemon and offline) now revoke the client credential after removing the entry, as approved by the maintainer. A revoke failure stays HTTP 200 and is reported in credential_revoke_error. The Web UI confirmation says so; docs, swagger and CHANGELOG updated. fix(runtime): audit and announce the binding restore when a rotating connect fails to finalize (compensating profile_change record plus client.binding_changed), and log a restore failure. Closes #1435 Refs #1451 --- CHANGELOG.md | 8 ++ cmd/mcpproxy/connect_cmd.go | 39 +++++++- cmd/mcpproxy/connect_credential.go | 66 +++++++++++++ .../connect_disconnect_revoke_test.go | 95 +++++++++++++++++++ docs/api/rest-api.md | 10 +- docs/cli/profile-commands.md | 4 +- frontend/src/components/ClientConnectList.vue | 4 + frontend/src/types/api.ts | 4 +- internal/connect/connect.go | 11 ++- internal/httpapi/connect.go | 43 +++++++++ .../httpapi/connect_client_credential_test.go | 71 ++++++++++++++ internal/httpapi/server.go | 27 +++--- internal/runtime/clients_service_connect.go | 37 +++++++- .../clients_service_restore_audit_test.go | 58 +++++++++++ oas/docs.go | 4 +- oas/swagger.yaml | 15 ++- 16 files changed, 468 insertions(+), 28 deletions(-) create mode 100644 cmd/mcpproxy/connect_disconnect_revoke_test.go create mode 100644 internal/runtime/clients_service_restore_audit_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index 08a974b0a..b3eb0f269 100644 --- a/CHANGELOG.md +++ b/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-`), 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 `). 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) diff --git a/cmd/mcpproxy/connect_cmd.go b/cmd/mcpproxy/connect_cmd.go index 4b025f54a..3f71f50e4 100644 --- a/cmd/mcpproxy/connect_cmd.go +++ b/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-): 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 " retries it. + Examples: mcpproxy disconnect claude-code mcpproxy disconnect cursor --name my-proxy`, @@ -142,8 +149,6 @@ 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 { @@ -151,11 +156,18 @@ func runDisconnect(cmd *cobra.Command, args []string) error { } 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. diff --git a/cmd/mcpproxy/connect_credential.go b/cmd/mcpproxy/connect_credential.go index 14be2b976..4b2423f99 100644 --- a/cmd/mcpproxy/connect_credential.go +++ b/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- is held by a diff --git a/cmd/mcpproxy/connect_disconnect_revoke_test.go b/cmd/mcpproxy/connect_disconnect_revoke_test.go new file mode 100644 index 000000000..86117775e --- /dev/null +++ b/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) +} diff --git a/docs/api/rest-api.md b/docs/api/rest-api.md index 774b4512f..2e25e2f36 100644 --- a/docs/api/rest-api.md +++ b/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-` 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). diff --git a/docs/cli/profile-commands.md b/docs/cli/profile-commands.md index 3642d510a..2d126ec30 100644 --- a/docs/cli/profile-commands.md +++ b/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 [--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 [--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 [--disconnect]` | Revoke the credential; with `--disconnect` also remove the config entry of a supported client | +| `client forget [--disconnect]` | Revoke the credential without touching the client's config; with `--disconnect` also remove the config entry of a supported client | + +`mcpproxy disconnect ` is the reverse of `connect`: it removes the config entry **and revokes the client's credential** (`client-`), 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 `. Undoing a disconnect restores the config file only; the credential stays revoked and a later `connect` mints a new one. The credential belongs to the client, so `--name ` for a non-default entry revokes it too. A client without an active client credential exits 1 with `mcpproxy connect --profile

` as the fix. diff --git a/frontend/src/components/ClientConnectList.vue b/frontend/src/components/ClientConnectList.vue index db29fc3fb..85f50a0e4 100644 --- a/frontend/src/components/ClientConnectList.vue +++ b/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.

+

+ The client's credential is revoked too. Restoring the file does not bring it back; + connect again to issue a new one. +