diff --git a/CHANGELOG.md b/CHANGELOG.md index b027914ca..e2f394460 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -167,6 +167,51 @@ Releases follow [Semantic Versioning](https://semver.org/). ### Bug Fixes +- **security/quarantine:** a server added by editing `mcp_config.json` (no `quarantined` key, + first seen) stayed held for review only until its first restart. Restarting it (by hand, after a + secret change, or through the automatic baseline scan's connect) wrote the raw file entry over + the recorded quarantine, so the next config write or core restart admitted it and auto-approved + its tools. The restart path now runs the entry through the config-load admission gate, and storage + never lowers a recorded quarantine unless the operator sets `"quarantined": false` or approves the + server. Present since v0.53.0. ([#1463](https://github.com/smart-mcp-proxy/mcpproxy-go/pull/1463)) +- **security/cli:** `mcpproxy doctor` no longer prints the admin API key (it was embedded in the Web + UI URL) in any output format, and URL credentials are masked in its output. ([#1472](https://github.com/smart-mcp-proxy/mcpproxy-go/pull/1472)) +- **review:** the Review screen starts fail-closed: read tools are pre-selected, write, destructive + and unannotated tools are not, the button states the exact count ("Approve server (3 of 9 + tools)"), and approving everything is an explicit action. The same defaults apply on macOS and + in `mcpproxy review approve`. ([#1481](https://github.com/smart-mcp-proxy/mcpproxy-go/pull/1481)) +- **review:** the scan banner no longer says "clean" for definitions that were never scanned or + changed after the scan, an approved server shows its approved state instead of per-tool + Approve/Reject buttons, and tool definitions are captured automatically when a quarantined + server is imported. ([#1470](https://github.com/smart-mcp-proxy/mcpproxy-go/pull/1470)) +- **catalog:** search finds the server you mean: a typed query fetches enough results from each + registry, versions of one server collapse to a single entry, an exact owner or name match ranks + first (searching "github" now returns GitHub's own server first), and "Verified" means the + publisher owns the source repository. ([#1469](https://github.com/smart-mcp-proxy/mcpproxy-go/pull/1469)) +- **telemetry:** when telemetry is disabled by `MCPPROXY_TELEMETRY=false`, `DO_NOT_TRACK` or `CI`, + the setup wizard, Home banner, Settings and the macOS app say so and lock the control instead of + showing the opt-out notice; `GET /api/v1/status` reports the effective state and its source. + macOS Settings shows the listen address of the core it is connected to. ([#1471](https://github.com/smart-mcp-proxy/mcpproxy-go/pull/1471)) +- **cli:** the global `-c/--config` and `-d/--data-dir` flags apply to every management command + (`upstream`, `registry`, `catalog` included) in any position, and no command silently creates a + default config in `~/.mcpproxy` when one was given; errors are printed once; a locked client + credential calling `set_profile` is told that profile changes are locked. ([#1472](https://github.com/smart-mcp-proxy/mcpproxy-go/pull/1472)) +- **web:** first-run fixes — the setup wizard shows one completion state after importing servers, + previewing an empty client config no longer fails, secret values are masked while you type, + the status pill says "awaiting review" for quarantined servers instead of "0 online", and the + profile editor's Try it uses your unsaved edits and shows readable reasons. ([#1473](https://github.com/smart-mcp-proxy/mcpproxy-go/pull/1473)) +- **profiles:** a call refused inside `code_execution` records the same `block_reason` as a + top-level refusal, and `mcpproxy access explain` names the destination profile in its move-client + fix. ([#1468](https://github.com/smart-mcp-proxy/mcpproxy-go/pull/1468)) +- **web:** token savings are labelled as an estimate on Home and in the macOS app, Activity's + "Clear filters" on a server/tool conflict reloads the list, "Needs review" health badges are + orange, and a brand-new install shows a getting-started card instead of "All clear". ([#1467](https://github.com/smart-mcp-proxy/mcpproxy-go/pull/1467)) +- **logging:** an upgrade start no longer repeats the "predate the config-load admission gate" + advisory on every config pass (once per server per process), and the benign startup race + "connection already in progress or established" is logged at debug instead of error. +- **profiles:** `set_profile` from a switchable client credential reports the servers of the + profile it just selected instead of the binding's narrower set. + - **web/settings:** toggles for nullable settings (`quarantine_enabled`, `telemetry.enabled` and the `audit_log.*` booleans) show the value the core actually applies instead of OFF when the key is absent, and the page header now says to press Save changes instead of "Changes save instantly". @@ -235,6 +280,10 @@ Releases follow [Semantic Versioning](https://semver.org/). ### Documentation +- **docs:** profiles are described as optional (without one, nothing profile-level restricts a + caller), and the `quarantined` default for servers added by editing `mcp_config.json` is + explained, with the full admission rules in Security Quarantine and a first-run note in the Quick + Start. - **051:** README hero — frosted-tiles banner + demo GIF (#488) ([#488](https://github.com/smart-mcp-proxy/mcpproxy-go/pull/488)) ([`25731da`](https://github.com/smart-mcp-proxy/mcpproxy-go/commit/25731da5e1a26753ee90a173a8ea03a317e82666)) ## [0.33.1] - 2026-05-20 diff --git a/docs/configuration.md b/docs/configuration.md index fa96fc428..8f290f1c3 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -415,7 +415,7 @@ it and none can double-report it. | `oauth` | object | No | OAuth configuration (see [OAuth Configuration](#oauth-configuration)) | | `isolation` | object | No | Per-server Docker isolation settings (see [Docker Isolation](#docker-isolation)) | | `enabled` | boolean | No | Enable/disable server (default: `true`) | -| `quarantined` | boolean | No | Security quarantine status (default: `false` for manually added servers, `true` for LLM-added servers) | +| `quarantined` | boolean | No | Security quarantine status. The default depends on how the server arrives. A server added through the UI, CLI, REST API or an AI agent follows its [trust mode](features/security-quarantine.md#trust-modes-auto--scan--manual) at add time (quarantined under the default `manual` mode). A server you add by editing this file, with no `quarantined` key and no prior `config.db` record, is **held for review** on first load when quarantine is enabled and its trust mode is not `auto` (no `trust_mode` means `manual`, unless a legacy `auto_approve_tool_changes: true` or `skip_quarantine: true` resolves it to `auto`). An explicit `"quarantined": false` admits it, and an explicit `true` holds it. A server that is already recorded in `config.db` keeps its recorded state. See [Servers added by hand-editing `mcp_config.json`](features/security-quarantine.md#servers-added-by-hand-editing-mcp_configjson) for the full admission rules. | | `reconnect_on_use` | boolean | No | When `true`, tool calls to a disconnected server trigger an immediate reconnect attempt (15s timeout) before failing (default: `false`) | | `expose_prompts` | boolean | No | Per-server override for whether this server's MCP prompts are aggregated into mcpproxy's `prompts/list`. Only takes effect when the global `aggregate_upstream_prompts` master switch is on. Omit to expose prompts whenever the server advertises `Capabilities.Prompts`; `false` opts this server out even if it does. | | `toon_output` | string | No | Per-server override for the global [`toon_output`](#toon-output-adaptive-result-encoding): `off`, `adaptive`, or `always`. Non-empty value wins over the global for this server's tools; omit to inherit. See [TOON Output](features/toon-output.md). | diff --git a/docs/features/profiles.md b/docs/features/profiles.md index d6bd1ef1a..bd01b29fc 100644 --- a/docs/features/profiles.md +++ b/docs/features/profiles.md @@ -6,6 +6,10 @@ description: "Named views over your upstream servers with a tool policy: tier ca # Profiles +:::note Profiles are optional +You do not need a profile to use MCPProxy. Without an effective profile (no profiles configured, or a caller that none of them applies to), no profile-level restriction applies to that caller: it can reach every configured server, unless its own credential is scoped (for example an agent token with an allowed-servers list). Use profiles when you need to scope access, for example to give one client or token read-only access to a few servers. +::: + A **profile** is a named view over your upstream servers plus a **tool policy**. It decides which servers a caller reaches, which of their tools the caller can discover and call, and whether the caller gets code execution and the management tools. The same profile is used by every surface that lets you work with MCPProxy: the config file, the Web UI and macOS app (**Profiles** in the sidebar), the CLI (`mcpproxy profile ...`), the MCP `profiles` tool and the REST API. A profile is enforced by the core, on every path a tool can be discovered or called through. "Work Read-only" really is read-only: a session under it cannot find, describe or call a write, destructive, denied or unclassified tool, whichever way it connected. @@ -18,6 +22,8 @@ Three ideas fit together: ## Quick start +This quick start is for users who need scoped access. If you do not, you can skip profiles entirely. + ```json { "require_mcp_auth": true, diff --git a/docs/features/security-quarantine.md b/docs/features/security-quarantine.md index 44e6446ac..f89116e12 100644 --- a/docs/features/security-quarantine.md +++ b/docs/features/security-quarantine.md @@ -44,6 +44,13 @@ A config-file server is held for review when **both** of the following are true: is an explicit operator statement and is obeyed), **and** - the server is not yet recorded in `config.db`. +Quarantine must also be enabled (`quarantine_enabled`, on by default), and the +server's [trust mode](#trust-modes-auto--scan--manual) must not be `auto`. A +server with no `trust_mode` is `manual` unless a legacy setting resolves to +`auto` (`"auto_approve_tool_changes": true`, or the older +`"skip_quarantine": true`), so by default a first-seen config-file server is +held; a server whose trust mode is `auto` is admitted. + The second condition is what makes upgrading safe: every server you are already running has a `config.db` record, so **upgrading never re-quarantines a server you have already vetted**. The boundary is that a server present in a diff --git a/docs/getting-started/quick-start.mdx b/docs/getting-started/quick-start.mdx index 3dadcf123..eac8cf94e 100644 --- a/docs/getting-started/quick-start.mdx +++ b/docs/getting-started/quick-start.mdx @@ -233,6 +233,10 @@ Edit `~/.mcpproxy/mcp_config.json`: } ``` +:::note First-time config-file servers are held for review +A server you add by editing this file for the first time is **held for review** (quarantined) until you approve it in the **Review queue**. The examples above omit the `quarantined` key on purpose. Writing `"quarantined": false` admits a server right away, so use it only for servers you have already vetted. A server that MCPProxy already knows keeps its recorded state. See [Servers added by hand-editing `mcp_config.json`](/features/security-quarantine#servers-added-by-hand-editing-mcp_configjson). +::: + ## 5. Approve It Out of Quarantine However you added it, the server is now **quarantined** - its tools are withheld diff --git a/internal/runtime/config_load_admission_gate.go b/internal/runtime/config_load_admission_gate.go index c2076382a..0c43dac16 100644 --- a/internal/runtime/config_load_admission_gate.go +++ b/internal/runtime/config_load_admission_gate.go @@ -224,6 +224,25 @@ func (r *Runtime) reportPreFixAdmissions(names []string) { if len(names) == 0 { return } + // The gate runs several times per process (gateInitialConfig, then + // LoadConfiguredServers, then the pre-publish hook on every publish); name + // each affected server once per process. + r.preFixMu.Lock() + if r.preFixReported == nil { + r.preFixReported = make(map[string]struct{}) + } + fresh := names[:0:0] + for _, n := range names { + if _, seen := r.preFixReported[n]; !seen { + r.preFixReported[n] = struct{}{} + fresh = append(fresh, n) + } + } + r.preFixMu.Unlock() + if len(fresh) == 0 { + return + } + names = fresh r.logger.Warn("Configured servers predate the config-load admission gate and have never been explicitly reviewed", zap.Strings("servers", names), zap.String("action", "review them in the quarantine UI, or record the decision with an explicit \"quarantined\" value in mcp_config.json (issue #937)")) diff --git a/internal/runtime/config_load_admission_gate_dedupe_test.go b/internal/runtime/config_load_admission_gate_dedupe_test.go new file mode 100644 index 000000000..60596f109 --- /dev/null +++ b/internal/runtime/config_load_admission_gate_dedupe_test.go @@ -0,0 +1,45 @@ +package runtime + +import ( + "testing" + + "github.com/stretchr/testify/require" + "go.uber.org/zap" + "go.uber.org/zap/zaptest/observer" + + "github.com/smart-mcp-proxy/mcpproxy-go/internal/config" + "github.com/smart-mcp-proxy/mcpproxy-go/internal/runtime/configsvc" +) + +// Item 4a: on an upgrade start the pre-fix advisory must be emitted once per +// process, not once per gate pass. Startup runs the gate in gateInitialConfig +// (before the supervisor starts) and again in LoadConfiguredServers +// (backgroundInitialization), and every later configsvc publish runs it again +// through the pre-publish hook. None of those passes changes the affected set. +func TestConfigLoadAdmissionGate_PreFixAdvisoryOncePerProcess(t *testing.T) { + core, logs := observer.New(zap.WarnLevel) + rt, _, _ := gateEnvAt(t, []map[string]any{ + {"name": "pre-fix", "command": "./poison", "protocol": "stdio", "enabled": true}, + {"name": "reviewed", "command": "./ok", "protocol": "stdio", "enabled": true, "quarantined": false}, + }, nil, zap.New(core)) + + // What an older release left behind: both servers known to config.db, live. + for _, name := range []string{"pre-fix", "reviewed"} { + require.NoError(t, rt.storageManager.SaveUpstreamServer(&config.ServerConfig{ + Name: name, Command: "./x", Protocol: "stdio", Enabled: true, + })) + } + + // Production startup order (lifecycle.go): hook + initial gate, then + // backgroundInitialization -> LoadConfiguredServers(nil). + rt.installAdmissionGateHook() + rt.gateInitialConfig() + require.NoError(t, rt.LoadConfiguredServers(nil)) + + // An unrelated publish (e.g. a hot reload of an unchanged file). + cur := rt.configSvc.Current().Config + require.NoError(t, rt.configSvc.Update(cur, configsvc.UpdateTypeReload, "test_reload")) + + require.Len(t, logs.FilterMessageSnippet("predate").All(), 1, + "the same pre-fix set must be reported once per process, not once per gate pass") +} diff --git a/internal/runtime/runtime.go b/internal/runtime/runtime.go index 4b3f3e3a6..ebfe9c4cb 100644 --- a/internal/runtime/runtime.go +++ b/internal/runtime/runtime.go @@ -127,6 +127,12 @@ type Runtime struct { selfWriteMu sync.Mutex recentSelfWrites []selfWriteEntry + // preFixReported holds the servers the "predate the config-load admission + // gate" advisory has already named in this process, so the gate passes at + // startup and on every later publish do not repeat it. + preFixMu sync.Mutex + preFixReported map[string]struct{} + statusMu sync.RWMutex status Status statusCh chan Status diff --git a/internal/server/profile_resolver.go b/internal/server/profile_resolver.go index cb00adaa5..9a9f97a24 100644 --- a/internal/server/profile_resolver.go +++ b/internal/server/profile_resolver.go @@ -336,7 +336,16 @@ func (p *MCPProxyServer) resolveActiveProfileWithSourceFromIndex(ctx context.Con // review round 2). idx.position is the O(1) existence check the pin/slug // tiers actually need. func (p *MCPProxyServer) resolveEffectiveProfileForJustSetSlug(ctx context.Context, idx *profileIndex, slug string) string { - if pin := profilePinFromContext(ctx); pin != "" { + // A SWITCHABLE client credential's ProfilePin is its BINDING (the base for + // switchable_to), which FR-020 ranks BELOW the url and session tiers. It is + // authoritative only while no selection is in effect, so a just-written + // slug must be reported instead of the binding. A locked credential and a + // regular agent-token pin stay authoritative. + switchableBinding := "" + if pin, mode, ok := clientCredentialFromContext(ctx); ok && mode != auth.ProfileModeLocked { + switchableBinding = pin + } + if pin := profilePinFromContext(ctx); pin != "" && pin != switchableBinding { if idx != nil && idx.position(pin) >= 0 { return pin } @@ -354,10 +363,12 @@ func (p *MCPProxyServer) resolveEffectiveProfileForJustSetSlug(ctx context.Conte if urlScope := profile.ProfileScopeFromContext(ctx); urlScope != nil { return urlScope.Name } - if slug != "" && idx != nil && idx.position(slug) >= 0 { + if slug != "" && idx != nil && idx.position(slug) >= 0 && (switchableBinding == "" || idx.position(switchableBinding) >= 0) { return slug } - return "" + // No selection in effect: a switchable credential falls back to its binding + // (a dangling binding stays deny-all, as resolveV3Base resolves it). + return switchableBinding } // profileScopeFromIndex builds the ProfileScope for slug's FULL membership diff --git a/internal/server/set_profile_switchable_servers_test.go b/internal/server/set_profile_switchable_servers_test.go new file mode 100644 index 000000000..d472e8fc1 --- /dev/null +++ b/internal/server/set_profile_switchable_servers_test.go @@ -0,0 +1,51 @@ +package server + +import ( + "encoding/json" + "testing" + + "github.com/mark3labs/mcp-go/mcp" + "github.com/stretchr/testify/require" +) + +// set_profile must report the servers the session can actually reach after the +// switch. For a SWITCHABLE client credential the bound profile is only the base +// (profile.SourceBinding): the session selection outranks it, so the reported +// list is the selected profile's reach, not the binding's. +func TestSetProfileV3SwitchableClientReportsSelectedProfileServers(t *testing.T) { + proxy, _ := newProfilesV3Fixture(t) + + setProfile := func(t *testing.T, sid, bindingMode, slug string) (active string, servers []string) { + t.Helper() + ctx := sessionCtx(clientCtx("laptop", "work-readonly", bindingMode), sid) + request := mcp.CallToolRequest{} + request.Params.Arguments = map[string]interface{}{"profile": slug} + result, err := proxy.handleSetProfile(ctx, request) + require.NoError(t, err) + require.False(t, result.IsError, resultText(t, result)) + var payload struct { + Active string `json:"active_profile"` + Servers []string `json:"servers"` + } + require.NoError(t, json.Unmarshal([]byte(resultText(t, result)), &payload)) + return payload.Active, payload.Servers + } + + t.Run("switching to a wider profile reports the wider reach", func(t *testing.T) { + active, servers := setProfile(t, "sw-wide", "switchable", "work-full") + require.Equal(t, "work-full", active) + require.ElementsMatch(t, []string{"github", "notion", "filesystem"}, servers) + }) + + t.Run("re-selecting the bound profile reports the binding's reach", func(t *testing.T) { + active, servers := setProfile(t, "sw-base", "switchable", "work-readonly") + require.Equal(t, "work-readonly", active) + require.ElementsMatch(t, []string{"github", "notion"}, servers) + }) + + t.Run("clearing the selection falls back to the binding", func(t *testing.T) { + active, servers := setProfile(t, "sw-clear", "switchable", "") + require.Equal(t, "", active) + require.ElementsMatch(t, []string{"github", "notion"}, servers) + }) +} diff --git a/internal/upstream/addserver_connecting_race_test.go b/internal/upstream/addserver_connecting_race_test.go new file mode 100644 index 000000000..14494bbd8 --- /dev/null +++ b/internal/upstream/addserver_connecting_race_test.go @@ -0,0 +1,40 @@ +package upstream + +import ( + "testing" + + "github.com/stretchr/testify/require" + "go.uber.org/zap" + + "github.com/smart-mcp-proxy/mcpproxy-go/internal/config" + "github.com/smart-mcp-proxy/mcpproxy-go/internal/secret" + "github.com/smart-mcp-proxy/mcpproxy-go/internal/upstream/types" +) + +// Item 4b: at startup the supervisor's reconcile (actor pool) starts the +// connect, and backgroundInitialization -> LoadConfiguredServers then calls +// AddServer for the same unchanged server. managed.Client.Connect correctly +// refuses a second concurrent connect, but AddServer surfaced that refusal as +// an error, which LoadConfiguredServers logged at ERROR ("connection already +// in progress or established (state: Connecting)") and which also skipped +// RegisterServerIdentity for that server. An in-flight connect owned by +// someone else is not a failure of AddServer. +func TestAddServer_UnchangedServerAlreadyConnectingIsNotAnError(t *testing.T) { + m := NewManager(zap.NewNop(), &config.Config{}, nil, secret.NewResolver(), nil) + sc := &config.ServerConfig{ + Name: "fx", Command: "/nonexistent/never-run", Protocol: "stdio", Enabled: true, + } + + require.NoError(t, m.AddServerConfig("fx", sc)) + client, ok := m.GetClient("fx") + require.True(t, ok) + + // The supervisor's connect is in flight. + client.StateManager.TransitionTo(types.StateConnecting) + + // LoadConfiguredServers' AddServer for the same, unchanged config. + require.NoError(t, m.AddServer("fx", sc), + "a connect already in progress for an unchanged server must not be reported as a failure") + require.Equal(t, types.StateConnecting, client.StateManager.GetState(), + "the in-flight connect must be left alone (no second attempt, no reset)") +} diff --git a/internal/upstream/managed/client.go b/internal/upstream/managed/client.go index dffd2df13..96d6b34f4 100644 --- a/internal/upstream/managed/client.go +++ b/internal/upstream/managed/client.go @@ -419,6 +419,10 @@ func NewClient(id string, serverConfig *config.ServerConfig, logger *zap.Logger, return mc, nil } +// ErrConnectAlreadyActive is returned by Connect when another caller's connect +// is in flight or the client is already Ready. It is a guard, not a failure. +var ErrConnectAlreadyActive = errors.New("connection already in progress or established") + // Connect establishes connection with state management. // IMPORTANT: mc.mu is only held briefly for state checks/transitions, NOT during the // potentially slow coreClient.Connect() call (which may involve OAuth flows taking minutes). @@ -430,7 +434,7 @@ func (mc *Client) Connect(ctx context.Context) error { // Check if already connecting or connected if mc.StateManager.IsConnecting() || mc.StateManager.IsReady() { mc.mu.Unlock() - return fmt.Errorf("connection already in progress or established (state: %s)", mc.StateManager.GetState().String()) + return fmt.Errorf("%w (state: %s)", ErrConnectAlreadyActive, mc.StateManager.GetState().String()) } // Snapshot the server name while mc.mu is held. Phase 3 below runs WITHOUT diff --git a/internal/upstream/manager.go b/internal/upstream/manager.go index 3b3a2c5e2..4996f23d1 100644 --- a/internal/upstream/manager.go +++ b/internal/upstream/manager.go @@ -596,6 +596,16 @@ func (m *Manager) AddServer(id string, serverConfig *config.ServerConfig) error ctx, cancel := context.WithTimeout(context.Background(), m.resolveConnectTimeout(serverConfig, client.DependsOnDocker())) defer cancel() if err := client.Connect(ctx); err != nil { + // The supervisor's reconcile usually owns the connect at startup; + // LoadConfiguredServers' AddServer for the same unchanged server then + // hits the in-flight guard. Nothing failed. + if errors.Is(err, managed.ErrConnectAlreadyActive) { + m.logger.Debug("Connect already in progress or established, not starting another", + zap.String("id", id), + zap.String("name", serverConfig.Name), + zap.String("state", client.GetState().String())) + return nil + } // Check if this is an OAuth error - don't fail AddServer for OAuth errStr := err.Error() isOAuthError := strings.Contains(errStr, "OAuth") ||