From 3c4ff411d219526aa9ba952418ad5bc9b5e77a6c Mon Sep 17 00:00:00 2001 From: Algis Dumbris Date: Sat, 1 Aug 2026 08:24:59 +0300 Subject: [PATCH 1/3] fix: classify upstream isError results as activity errors and gate config-load admission MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two independent HIGH-severity core fixes. activity: an upstream answering isError:true is an error (Related #935) A tool result carrying isError:true — bad argument value, unknown tool, server-side validation failure — was recorded with status=success, because the transport hop returned err==nil and every post-dispatch emit site hardcoded "success". Only mcpproxy's own pre-dispatch validation produced status=error, so every failure consumer under-reported: the tray glance error series and its red row markers never fired for the most common real failure, and the 24h error counts were correspondingly low. activityStatusForResult() now classifies the dispatched result and supplies an error_message drawn from the result's text blocks (or its structured content), capped at 512 bytes on a rune boundary. Wired into all three dispatch paths: handleCallToolVariant, the legacy handleCallTool, and the direct-routing handler. The paired internal_tool_call record carries the same classification. Usage aggregates and the timeline derive from ActivityRecord.Status, so they become correct with no further change. What is returned to the caller is untouched — the isError result is still forwarded verbatim. The pre-dispatch error path is unchanged. security: apply the trust-mode admission gate on the config-load path (Related #937) A server written directly into mcp_config.json never passed through Config.QuarantineDefaultForServer, which was invoked only from the three add-time sites. It was admitted unquarantined with its tools auto-approved, serving poisoned tool descriptions verbatim under the default manual trust mode with quarantine_enabled:true — contradicting the trust-mode table in docs/features/security-quarantine.md. ServerConfig.UnmarshalJSON now records whether the document carried a "quarantined" key, making absent distinguishable from explicit false, and LoadConfiguredServers applies the gate before its storage-save phase. A server is gated only when the key was absent AND the server is not yet in config.db. Every server an existing user runs already has a config.db record, so upgrading re-quarantines nothing. The gate only ever adds quarantine, never removes it. --- docs/features/security-quarantine.md | 33 +++ internal/config/config.go | 50 ++++ internal/config/merge.go | 5 + internal/config/quarantine_presence_test.go | 94 ++++++++ .../runtime/config_load_admission_gate.go | 102 ++++++++ .../config_load_admission_gate_test.go | 221 ++++++++++++++++++ internal/runtime/lifecycle.go | 5 + internal/server/activity_result_status.go | 118 ++++++++++ .../server/activity_result_status_test.go | 182 +++++++++++++++ internal/server/e2e_iserror_activity_test.go | 193 +++++++++++++++ internal/server/mcp.go | 35 ++- internal/server/mcp_routing.go | 10 +- internal/storage/async_ops_test.go | 10 + 13 files changed, 1049 insertions(+), 9 deletions(-) create mode 100644 internal/config/quarantine_presence_test.go create mode 100644 internal/runtime/config_load_admission_gate.go create mode 100644 internal/runtime/config_load_admission_gate_test.go create mode 100644 internal/server/activity_result_status.go create mode 100644 internal/server/activity_result_status_test.go create mode 100644 internal/server/e2e_iserror_activity_test.go diff --git a/docs/features/security-quarantine.md b/docs/features/security-quarantine.md index c28f1343..44938db4 100644 --- a/docs/features/security-quarantine.md +++ b/docs/features/security-quarantine.md @@ -30,6 +30,34 @@ When a new server is added via an AI client (using the `upstream_servers` tool): 2. Tool calls to quarantined servers return a **security analysis** instead of executing 3. Server remains quarantined until **manually approved** +### Servers added by hand-editing `mcp_config.json` + +Since issue #937, the same admission gate applies to servers written directly +into the configuration file — hand-editing is a normal workflow, and config +files get shared, templated and copied between machines, so an entry from a +config edit is admitted on exactly the same terms as one from +`mcpproxy upstream add`. + +A config-file server is held for review when **both** of the following are true: + +- the entry does **not** contain a `quarantined` key (writing `"quarantined": false` + is an explicit operator statement and is obeyed), **and** +- the server is not yet recorded in `config.db`. + +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 +hand-written config but absent from `config.db` — after a wiped data directory, +or on a machine that has never seen that config before — is treated as +first-seen and held for review. + +The decision is durable: once recorded, a quarantine is not reverted by a config +file that never mentions the key. Un-quarantining stays a user action, and +writes both the config file and `config.db`. + +No TPA scan runs at config-load admission; quarantine-by-default already keeps +the tools away from agents, and the scan is what the human review step performs. + ### Tool Discovery and Search Isolation **Quarantined servers are completely isolated from the tool discovery and search system:** @@ -213,6 +241,11 @@ new-server admission and tool-change approval (superseding the binary Unrecognized values fail closed to `manual`. Config field: per-server `trust_mode`; REST: `trust_mode` on `POST/PATCH/GET /api/v1/servers`. +The "Add time" column applies to every admission path — the `upstream_servers` +tool, the REST API, a registry add, and (since issue #937) a first-seen server +in `mcp_config.json`. See +[Servers added by hand-editing `mcp_config.json`](#servers-added-by-hand-editing-mcp_configjson). + **Web UI (spec 088)**: the server's Configuration tab has a tri-mode selector (choosing `auto` asks for confirmation and explains the risk); the add-server form chooses the mode instead of a raw quarantine checkbox diff --git a/internal/config/config.go b/internal/config/config.go index 997d211d..556dbb40 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -467,6 +467,13 @@ type ServerConfig struct { OAuth *OAuthConfig `json:"oauth" mapstructure:"oauth"` // OAuth configuration (keep even when empty to signal OAuth requirement) Enabled bool `json:"enabled" mapstructure:"enabled"` Quarantined bool `json:"quarantined" mapstructure:"quarantined"` // Security quarantine status + // quarantineExplicitlySet records whether the decoded JSON actually carried a + // "quarantined" key (issue #937). Quarantined is a plain bool, so an ABSENT + // key and an explicit `false` are otherwise indistinguishable — which is what + // let a hand-written mcp_config.json entry bypass the trust-mode admission + // gate that every add path applies. Set only by UnmarshalJSON; read via + // QuarantineExplicitlySet(). + quarantineExplicitlySet bool // SkipQuarantine is DEPRECATED (MCP-2930): use AutoApproveToolChanges instead. // Kept for back-compat parsing; on config load a legacy skip_quarantine:true is // migrated to auto_approve_tool_changes:true only when the new field is unset @@ -1632,6 +1639,49 @@ func (c *Config) DefaultQuarantineForNewServer() bool { return c.IsQuarantineEnabled() } +// UnmarshalJSON decodes a ServerConfig and additionally records whether the +// "quarantined" key was present at all (issue #937). +// +// The presence bit is what distinguishes "the operator opted out" from "the +// operator never said anything", and it is the only thing the config-load +// admission gate keys off. It is deliberately unexported and never serialized: +// it describes the DOCUMENT that was parsed, not the server. +// +// Note this only fires for JSON decoding. Structs built in Go (REST handlers, +// the add paths, tests) leave the bit false — i.e. "unstated" — which is the +// safe default: the gate can only ever ADD quarantine, never remove it. +func (sc *ServerConfig) UnmarshalJSON(data []byte) error { + type Alias ServerConfig + if err := json.Unmarshal(data, (*Alias)(sc)); err != nil { + return err + } + + var probe map[string]json.RawMessage + if err := json.Unmarshal(data, &probe); err != nil { + // A non-object payload cannot carry the key; the decode above would + // already have failed on anything we care about. + return nil //nolint:nilerr // presence detection is best-effort + } + _, sc.quarantineExplicitlySet = probe["quarantined"] + return nil +} + +// QuarantineExplicitlySet reports whether the parsed configuration document +// stated a "quarantined" value for this server. False means the key was absent +// (or the struct was built in Go rather than decoded), which the config-load +// admission gate treats as "never been through admission". +func (sc *ServerConfig) QuarantineExplicitlySet() bool { + return sc != nil && sc.quarantineExplicitlySet +} + +// MarkQuarantineExplicitlySet stamps the presence bit on a struct that did not +// come from JSON. Used by copy helpers so the bit survives a config round-trip. +func (sc *ServerConfig) MarkQuarantineExplicitlySet(explicit bool) { + if sc != nil { + sc.quarantineExplicitlySet = explicit + } +} + // QuarantineDefaultForServer resolves the add-time Quarantined default for a // specific new server from its trust_mode (spec 086 stage 3, FR-011). Secure by // default: a server is admitted UNQUARANTINED only under trust_mode auto; scan diff --git a/internal/config/merge.go b/internal/config/merge.go index a6999edd..9115c5ff 100644 --- a/internal/config/merge.go +++ b/internal/config/merge.go @@ -627,6 +627,11 @@ func CopyServerConfig(src *ServerConfig) *ServerConfig { dst.Isolation = copyIsolationConfig(src.Isolation) dst.OAuth = copyOAuthConfig(src.OAuth) + // Carry the unexported "quarantined key was present" bit (issue #937). + // Dropping it here would make an explicitly-opted-out server look un-stated + // to the config-load admission gate on the next sync. + dst.quarantineExplicitlySet = src.quarantineExplicitlySet + return dst } diff --git a/internal/config/quarantine_presence_test.go b/internal/config/quarantine_presence_test.go new file mode 100644 index 00000000..7a2f3356 --- /dev/null +++ b/internal/config/quarantine_presence_test.go @@ -0,0 +1,94 @@ +package config + +import ( + "encoding/json" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// Issue #937: `Quarantined` is a plain bool, so an ABSENT "quarantined" key is +// indistinguishable from an explicit `false`. That is what let a hand-written +// mcp_config.json entry be admitted unquarantined while the identical server +// added via `upstream add` was correctly gated: config load read "not +// quarantined" for a server that had simply never said anything. +// +// The presence bit is what the admission gate keys off, so it has to survive +// JSON decoding. +func TestServerConfig_QuarantineExplicitlySet(t *testing.T) { + tests := []struct { + name string + raw string + wantExplicit bool + wantValue bool + }{ + { + name: "absent key is not explicit", + raw: `{"name":"handwritten","command":"./poison"}`, + wantExplicit: false, + wantValue: false, + }, + { + name: "explicit false is explicit", + raw: `{"name":"handwritten","command":"./poison","quarantined":false}`, + wantExplicit: true, + wantValue: false, + }, + { + name: "explicit true is explicit", + raw: `{"name":"handwritten","command":"./poison","quarantined":true}`, + wantExplicit: true, + wantValue: true, + }, + { + name: "explicit null counts as stated", + raw: `{"name":"handwritten","command":"./poison","quarantined":null}`, + wantExplicit: true, + wantValue: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + var sc ServerConfig + require.NoError(t, json.Unmarshal([]byte(tt.raw), &sc)) + assert.Equal(t, tt.wantExplicit, sc.QuarantineExplicitlySet()) + assert.Equal(t, tt.wantValue, sc.Quarantined) + // The rest of the struct must still decode normally — the custom + // unmarshaler must not shadow any other field. + assert.Equal(t, "handwritten", sc.Name) + assert.Equal(t, "./poison", sc.Command) + }) + } +} + +// The presence bit is unexported, so it is invisible to reflection-based copies. +// CopyServerConfig is on the config-apply path; losing the bit there would make +// an explicitly-opted-out server look un-stated and get gated on the next sync. +func TestCopyServerConfig_PreservesQuarantinePresence(t *testing.T) { + var sc ServerConfig + require.NoError(t, json.Unmarshal([]byte(`{"name":"s","quarantined":false}`), &sc)) + require.True(t, sc.QuarantineExplicitlySet()) + + assert.True(t, CopyServerConfig(&sc).QuarantineExplicitlySet()) +} + +// A server list parsed from a whole config file must carry the bit too — the +// admission gate runs on Config.Servers, not on individually-decoded structs. +func TestConfigLoad_ServersCarryQuarantinePresence(t *testing.T) { + raw := `{ + "listen": "127.0.0.1:0", + "mcpServers": [ + {"name":"stated","command":"a","quarantined":false}, + {"name":"unstated","command":"b"} + ] + }` + + var cfg Config + require.NoError(t, json.Unmarshal([]byte(raw), &cfg)) + require.Len(t, cfg.Servers, 2) + + assert.True(t, cfg.Servers[0].QuarantineExplicitlySet(), "stated") + assert.False(t, cfg.Servers[1].QuarantineExplicitlySet(), "unstated") +} diff --git a/internal/runtime/config_load_admission_gate.go b/internal/runtime/config_load_admission_gate.go new file mode 100644 index 00000000..653d8c78 --- /dev/null +++ b/internal/runtime/config_load_admission_gate.go @@ -0,0 +1,102 @@ +package runtime + +import ( + "go.uber.org/zap" + + "github.com/smart-mcp-proxy/mcpproxy-go/internal/config" +) + +// Issue #937 — the trust-mode admission gate on the CONFIG-LOAD path. +// +// Before this, Config.QuarantineDefaultForServer() was invoked from exactly +// three ADD-time sites (registry add, the upstream_servers MCP tool, the REST +// handler). A server written straight into mcp_config.json reached the upstream +// manager without passing any of them: it was admitted unquarantined, its tools +// auto-approved, and a poisoned tool description served verbatim to agents — +// under the DEFAULT `manual` trust mode with quarantine_enabled:true. That +// contradicts the trust-mode table published in +// docs/features/security-quarantine.md, which states that `manual` means +// "Quarantined for human review". +// +// # What is gated +// +// A server is gated only when BOTH hold: +// +// - the parsed config document never stated a `quarantined` value +// (ServerConfig.QuarantineExplicitlySet() — an operator who wrote the key, +// either way, has spoken and is obeyed), AND +// - the server is not yet known to config.db. +// +// # The upgrade boundary (deliberate, and the reason for the second condition) +// +// "Not yet known to config.db" is the proxy for "has never been through +// admission". Every server an existing user is running already has a config.db +// record, so an upgrade re-quarantines nothing — which is the whole point: +// a server the user has already vetted must not suddenly disappear behind a +// quarantine wall on a version bump. +// +// The boundary this leaves is precise and worth stating: a server that is in a +// hand-written config but NOT in config.db is treated as first-seen. That is +// true after `rm -rf ~/.mcpproxy/*.db`, or when a config file is moved to a +// machine whose data dir has never seen it — both of which are exactly the +// "config files get shared, templated, copied between machines" case the issue +// calls out, so gating them is the intended behaviour rather than a regression. +// +// # Durability +// +// The gate decision is persisted by the caller's SaveUpstreamServer. On the +// next start the server IS in config.db while the config file still says +// nothing, so the second branch below re-reads the stored value instead of +// letting an absent key silently resolve back to false. Without that, the hole +// would reopen one restart later. +// +// # Direction +// +// The gate can only ever ADD quarantine. Both branches are guarded on +// !sc.Quarantined, so a config (or an in-flight struct from an add path) that +// already asks for quarantine is never relaxed by a stale storage record. +// Un-quarantining stays exclusively a user action, which goes through +// Runtime.QuarantineServer and writes both stores. +// +// # Not done here +// +// No TPA scan runs at config-load admission. Quarantine-by-default already +// prevents the tools from reaching an agent, and the scan is what the human +// review step in the quarantine UI performs. Running a synchronous scan on the +// startup path would block boot on every first-seen server. +func (r *Runtime) applyConfigLoadAdmissionGate(cfg *config.Config, stored map[string]*config.ServerConfig) { + if cfg == nil { + return + } + + for _, sc := range cfg.Servers { + if sc == nil || sc.Quarantined || sc.QuarantineExplicitlySet() { + continue + } + + if prev, known := stored[sc.Name]; known { + // Already been through admission. Inherit whatever mcpproxy itself + // recorded, so a gate decision from a previous run survives a config + // file that never mentions the key. + if prev != nil && prev.Quarantined { + sc.Quarantined = true + r.logger.Info("Retaining recorded quarantine for server with no explicit config value", + zap.String("server", sc.Name)) + } + continue + } + + if !cfg.QuarantineDefaultForServer(sc) { + continue + } + + sc.Quarantined = true + r.logger.Warn("Quarantining first-seen server from configuration file", + zap.String("server", sc.Name), + zap.String("trust_mode", string(sc.EffectiveTrustMode())), + zap.String("reason", "config-load admission gate: server has not been reviewed (issue #937)")) + r.EmitActivityQuarantineChange(sc.Name, true, + "first-seen server from configuration file held for review by the "+ + string(sc.EffectiveTrustMode())+" trust mode") + } +} diff --git a/internal/runtime/config_load_admission_gate_test.go b/internal/runtime/config_load_admission_gate_test.go new file mode 100644 index 00000000..827d6e5a --- /dev/null +++ b/internal/runtime/config_load_admission_gate_test.go @@ -0,0 +1,221 @@ +package runtime + +import ( + "encoding/json" + "os" + "path/filepath" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "go.uber.org/zap" + + "github.com/smart-mcp-proxy/mcpproxy-go/internal/config" +) + +// Issue #937 — the trust-mode admission gate must apply on the CONFIG-LOAD path, +// not only on the add paths (`upstream_servers` tool, REST, registry add). +// +// Before the fix, a server written straight into mcp_config.json was admitted +// unquarantined with its tools auto-approved, serving a poisoned tool +// description verbatim — under the DEFAULT `manual` trust mode with +// quarantine_enabled:true. `mcpproxy upstream add` of the very same server was +// correctly quarantined. +// +// These tests drive the production path: file → LoadFromFile → runtime.New → +// LoadConfiguredServers, and assert on what lands in storage (which is what the +// supervisor, the REST /servers endpoint and the tool index all read). + +// gateEnv writes a config file containing the given server entries and returns a +// runtime built from it plus the parsed config. +func gateEnv(t *testing.T, servers []map[string]any, extra map[string]any) (*Runtime, *config.Config) { + t.Helper() + + dir := t.TempDir() + p := filepath.Join(dir, "mcp_config.json") + + raw := map[string]any{ + "listen": "127.0.0.1:0", + "data_dir": dir, + "api_key": "k", + "mcpServers": servers, + } + for k, v := range extra { + raw[k] = v + } + + cfgJSON, err := json.Marshal(raw) + require.NoError(t, err) + require.NoError(t, os.WriteFile(p, cfgJSON, 0600)) + + cfg, err := config.LoadFromFile(p) + require.NoError(t, err) + + rt, err := New(cfg, p, zap.NewNop()) + require.NoError(t, err) + t.Cleanup(func() { _ = rt.Close() }) + + return rt, cfg +} + +func storedServer(t *testing.T, rt *Runtime, name string) *config.ServerConfig { + t.Helper() + stored, err := rt.storageManager.ListUpstreamServers() + require.NoError(t, err) + for _, s := range stored { + if s.Name == name { + return s + } + } + t.Fatalf("server %q not found in storage", name) + return nil +} + +// The headline case from the issue: a hand-written entry with NO `quarantined` +// key and NO `trust_mode`. EffectiveTrustMode() resolves to manual, which the +// docs publish as "Quarantined for human review". +func TestConfigLoadAdmissionGate_QuarantinesFirstSeenServer(t *testing.T) { + rt, cfg := gateEnv(t, []map[string]any{ + {"name": "handwritten", "command": "./poison", "protocol": "stdio", "enabled": true}, + }, nil) + + require.NoError(t, rt.LoadConfiguredServers(cfg)) + + assert.True(t, storedServer(t, rt, "handwritten").Quarantined, + "a first-seen config-file server under the default manual trust mode must be quarantined (issue #937)") + assert.True(t, cfg.Servers[0].Quarantined, + "the in-memory config must agree, or the supervisor will still connect it unquarantined") +} + +// trust_mode:auto is the documented opt-out. It must keep working, or the gate +// would quarantine servers the operator deliberately trusted. +func TestConfigLoadAdmissionGate_TrustModeAutoIsNotGated(t *testing.T) { + rt, cfg := gateEnv(t, []map[string]any{ + {"name": "trusted", "command": "./ok", "protocol": "stdio", "enabled": true, "trust_mode": "auto"}, + }, nil) + + require.NoError(t, rt.LoadConfiguredServers(cfg)) + + assert.False(t, storedServer(t, rt, "trusted").Quarantined, + "trust_mode:auto is an explicit opt-out and must not be gated") +} + +// Quarantine turned off globally disables the whole feature; the gate must not +// resurrect it. +func TestConfigLoadAdmissionGate_QuarantineDisabledGlobally(t *testing.T) { + rt, cfg := gateEnv(t, []map[string]any{ + {"name": "anything", "command": "./ok", "protocol": "stdio", "enabled": true}, + }, map[string]any{"quarantine_enabled": false}) + + require.NoError(t, rt.LoadConfiguredServers(cfg)) + + assert.False(t, storedServer(t, rt, "anything").Quarantined, + "quarantine_enabled:false must disable the gate entirely") +} + +// An operator who wrote `"quarantined": false` said something. The gate keys off +// ABSENCE, not off the value, so an explicit opt-out is honoured. +func TestConfigLoadAdmissionGate_ExplicitFalseIsHonoured(t *testing.T) { + rt, cfg := gateEnv(t, []map[string]any{ + {"name": "vetted", "command": "./ok", "protocol": "stdio", "enabled": true, "quarantined": false}, + }, nil) + + require.NoError(t, rt.LoadConfiguredServers(cfg)) + + assert.False(t, storedServer(t, rt, "vetted").Quarantined, + "an explicit quarantined:false is an operator statement and must be honoured") +} + +// UPGRADE SAFETY — the property that makes this shippable. +// +// A server an existing user has been running for months has an entry in +// config.db. It must not be re-quarantined just because their hand-written +// config never spelled out `quarantined`. Storage presence is the "has already +// been through admission" signal. +func TestConfigLoadAdmissionGate_KnownServerIsNotRequarantined(t *testing.T) { + rt, cfg := gateEnv(t, []map[string]any{ + {"name": "longstanding", "command": "./ok", "protocol": "stdio", "enabled": true}, + }, nil) + + // Simulate the pre-upgrade state: the server is already known to config.db, + // unquarantined. + require.NoError(t, rt.storageManager.SaveUpstreamServer(&config.ServerConfig{ + Name: "longstanding", Command: "./ok", Protocol: "stdio", Enabled: true, Quarantined: false, + })) + + require.NoError(t, rt.LoadConfiguredServers(cfg)) + + assert.False(t, storedServer(t, rt, "longstanding").Quarantined, + "a server already known to config.db has been through admission and must not be re-quarantined on upgrade") + assert.False(t, cfg.Servers[0].Quarantined) +} + +// DURABILITY — the gate decision has to survive a restart. +// +// After the first gated load, config.db says quarantined while the config file +// still says nothing. On the next start the un-stated key must inherit +// mcpproxy's own state rather than silently reverting to false — otherwise the +// hole reopens one restart later. +func TestConfigLoadAdmissionGate_QuarantineSurvivesRestart(t *testing.T) { + rt, cfg := gateEnv(t, []map[string]any{ + {"name": "handwritten", "command": "./poison", "protocol": "stdio", "enabled": true}, + }, nil) + + // Storage remembers the gate decision from a previous run. + require.NoError(t, rt.storageManager.SaveUpstreamServer(&config.ServerConfig{ + Name: "handwritten", Command: "./poison", Protocol: "stdio", Enabled: true, Quarantined: true, + })) + + require.NoError(t, rt.LoadConfiguredServers(cfg)) + + assert.True(t, storedServer(t, rt, "handwritten").Quarantined, + "a quarantine recorded in config.db must not be reverted by a config file that never mentions the key") + assert.True(t, cfg.Servers[0].Quarantined) +} + +// The issue's comparison table: the same server, same binary, same +// quarantine_enabled — reached by `upstream add` versus by a hand-written +// config — must land in the same place. The add paths call +// Config.QuarantineDefaultForServer; this asserts the config-load path now +// agrees with it for every trust mode, rather than re-deriving its own answer. +func TestConfigLoadAdmissionGate_MatchesAddPathDecision(t *testing.T) { + for _, trustMode := range []string{"", "manual", "scan", "auto"} { + t.Run("trust_mode="+trustMode, func(t *testing.T) { + entry := map[string]any{ + "name": "same", "command": "./x", "protocol": "stdio", "enabled": true, + } + if trustMode != "" { + entry["trust_mode"] = trustMode + } + + rt, cfg := gateEnv(t, []map[string]any{entry}, nil) + + // What `upstream add` / the REST handler would have decided. + addPath := cfg.QuarantineDefaultForServer(&config.ServerConfig{TrustMode: trustMode}) + + require.NoError(t, rt.LoadConfiguredServers(cfg)) + + assert.Equal(t, addPath, storedServer(t, rt, "same").Quarantined, + "config-load admission must reach the same verdict as the add path (issue #937)") + }) + } +} + +// The gate only ever ADDS quarantine. A config that already asks for quarantine +// (in memory or on disk) must never be relaxed by the storage fallback — every +// admission path sets the flag before it reaches here, and un-quarantining is +// the user's decision alone. +func TestConfigLoadAdmissionGate_NeverUnquarantines(t *testing.T) { + rt, cfg := gateEnv(t, []map[string]any{ + {"name": "held", "command": "./x", "protocol": "stdio", "enabled": true, "quarantined": true}, + }, nil) + + require.NoError(t, rt.storageManager.SaveUpstreamServer(&config.ServerConfig{ + Name: "held", Command: "./x", Protocol: "stdio", Enabled: true, Quarantined: false, + })) + + require.NoError(t, rt.LoadConfiguredServers(cfg)) + + assert.True(t, storedServer(t, rt, "held").Quarantined, + "config asking for quarantine must win over a stale unquarantined storage record") +} diff --git a/internal/runtime/lifecycle.go b/internal/runtime/lifecycle.go index 46cdcaf3..5e42b329 100644 --- a/internal/runtime/lifecycle.go +++ b/internal/runtime/lifecycle.go @@ -942,6 +942,11 @@ func (r *Runtime) LoadConfiguredServers(cfg *config.Config) error { storedServerMap[storedServer.Name] = storedServer } + // Issue #937: apply the trust-mode admission gate to servers that arrived by + // config edit. Runs BEFORE the storage save loop below so the gated value is + // what gets persisted, connected, and reported. + r.applyConfigLoadAdmissionGate(cfg, storedServerMap) + // GC orphaned tool-approval records (MCP-1002): drop approvals whose server // is no longer configured. Configured-but-disabled servers are preserved so // a later re-enable doesn't re-quarantine their previously-approved tools. diff --git a/internal/server/activity_result_status.go b/internal/server/activity_result_status.go new file mode 100644 index 00000000..6743487e --- /dev/null +++ b/internal/server/activity_result_status.go @@ -0,0 +1,118 @@ +package server + +import ( + "encoding/json" + "strings" + + "github.com/mark3labs/mcp-go/mcp" +) + +// Issue #935 — classifying a DISPATCHED tool result for the activity log. +// +// A tool call has two independent failure surfaces: +// +// 1. the call never happened — pre-dispatch validation, quarantine, transport +// or protocol failure. These return a non-nil error and were always +// recorded as status="error". +// 2. the call happened and the upstream ANSWERED "this failed" by setting +// isError:true on the result — a bad argument value, an unknown tool, a +// server-side validation failure. The MCP round-trip succeeded, so err is +// nil, and every post-dispatch emit site used to hardcode status="success". +// +// (2) is by far the most common real-world failure, and recording it as a +// success made every consumer of activity status under-report: the tray glance +// error series and its red row markers never fired, and the 24h error counts in +// the usage timeline were correspondingly low (both derive from +// ActivityRecord.Status — see runtime.UsageAggregate.Apply). +// +// Deliberately NOT done here: minting a third status value (e.g. +// "upstream_error") to tell (1) and (2) apart. Every consumer — the usage +// aggregate, the timeline, the tray, the REST filters, the frontend — switches +// on the two known values today, and a new one would be silently dropped by all +// of them. The distinction is left for a follow-up that can migrate the +// consumers together; the error_message already carries the upstream's own +// words, which is what a human debugging the call needs. +// +// This classification affects the ACTIVITY RECORD ONLY. What is returned to the +// caller is untouched: an isError result is still forwarded verbatim, because +// the client is entitled to the upstream's own answer. + +const ( + // activityErrorMessageLimit caps the recorded error text. Activity records + // are persisted and streamed to the UI, so an upstream that answers with a + // multi-megabyte error body must not be able to bloat the log. + activityErrorMessageLimit = 512 + + // activityErrorMessageEllipsis marks a message the cap above shortened. + activityErrorMessageEllipsis = "…" + + // upstreamErrorResultMessage is recorded when the upstream flagged the + // result as an error but supplied nothing readable to explain it. An empty + // error_message would render as a blank row in the activity UI. + upstreamErrorResultMessage = "upstream returned an error result (isError=true)" +) + +// activityStatusForResult classifies a successfully dispatched tool result for +// the activity log, returning the (status, error_message) pair to record. +// +// It returns ("success", "") for anything that is not an MCP result flagged +// isError — including nil and non-CallToolResult values, which several call +// paths can produce — and ("error", ) otherwise. +func activityStatusForResult(result interface{}) (status, errorMsg string) { + ctr := asCallToolResult(result) + if ctr == nil || !ctr.IsError { + return "success", "" + } + return "error", upstreamErrorText(ctr) +} + +// asCallToolResult normalises the interface{} that upstream.Manager.CallTool +// returns. Both the pointer and the value form are accepted; anything else +// (string, map, nil) yields nil. +func asCallToolResult(result interface{}) *mcp.CallToolResult { + switch v := result.(type) { + case *mcp.CallToolResult: + return v + case mcp.CallToolResult: + return &v + default: + return nil + } +} + +// upstreamErrorText extracts a human-readable reason from an isError result: +// its text blocks first (that is where MCP servers put the message), then its +// structured content, then a fixed fallback. +func upstreamErrorText(ctr *mcp.CallToolResult) string { + parts := make([]string, 0, len(ctr.Content)) + for _, c := range ctr.Content { + var text string + switch tc := c.(type) { + case mcp.TextContent: + text = tc.Text + case *mcp.TextContent: + if tc != nil { + text = tc.Text + } + } + if trimmed := strings.TrimSpace(text); trimmed != "" { + parts = append(parts, trimmed) + } + } + + msg := strings.Join(parts, "\n") + if msg == "" && ctr.StructuredContent != nil { + if encoded, err := json.Marshal(ctr.StructuredContent); err == nil { + msg = strings.TrimSpace(string(encoded)) + } + } + if msg == "" { + return upstreamErrorResultMessage + } + if len(msg) > activityErrorMessageLimit { + // safeTruncateBytes backs the cut up to a rune boundary, so the stored + // message is always valid UTF-8. + msg = msg[:safeTruncateBytes(msg, activityErrorMessageLimit)] + activityErrorMessageEllipsis + } + return msg +} diff --git a/internal/server/activity_result_status_test.go b/internal/server/activity_result_status_test.go new file mode 100644 index 00000000..5614070e --- /dev/null +++ b/internal/server/activity_result_status_test.go @@ -0,0 +1,182 @@ +package server + +import ( + "go/ast" + "go/parser" + "go/token" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/mark3labs/mcp-go/mcp" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// Issue #935: an upstream that answers with isError:true has FAILED. The +// transport call succeeded, so err == nil and every post-dispatch emit site +// used to hardcode status="success" — which silently hid the single most +// common real failure (bad argument value, unknown tool, server-side +// validation) from the tray glance, the 24h error counts and the activity log. +func TestActivityStatusForResult(t *testing.T) { + tests := []struct { + name string + result interface{} + wantStatus string + wantErrMsg string + }{ + { + name: "nil result is a success", + result: nil, + wantStatus: "success", + wantErrMsg: "", + }, + { + name: "ordinary result is a success", + result: mcp.NewToolResultText("42"), + wantStatus: "success", + wantErrMsg: "", + }, + { + name: "non-CallToolResult value is a success", + result: map[string]any{"ok": true}, + wantStatus: "success", + wantErrMsg: "", + }, + { + name: "isError result is an error carrying the upstream text", + result: mcp.NewToolResultError("Invalid timezone: 'Mars/Olympus'"), + wantStatus: "error", + wantErrMsg: "Invalid timezone: 'Mars/Olympus'", + }, + { + name: "isError result by value is classified too", + result: mcp.CallToolResult{ + IsError: true, + Content: []mcp.Content{mcp.NewTextContent("unknown tool: nope")}, + }, + wantStatus: "error", + wantErrMsg: "unknown tool: nope", + }, + { + name: "multiple text blocks are joined", + result: &mcp.CallToolResult{ + IsError: true, + Content: []mcp.Content{ + mcp.NewTextContent("validation failed:"), + mcp.NewTextContent("field 'timezone' is required"), + }, + }, + wantStatus: "error", + wantErrMsg: "validation failed:\nfield 'timezone' is required", + }, + { + name: "isError with no usable content falls back to a fixed message", + result: &mcp.CallToolResult{ + IsError: true, + Content: []mcp.Content{mcp.NewImageContent("Zm9v", "image/png")}, + }, + wantStatus: "error", + wantErrMsg: upstreamErrorResultMessage, + }, + { + name: "isError with only structured content serialises it", + result: &mcp.CallToolResult{ + IsError: true, + StructuredContent: map[string]any{"code": "E_BAD_ARG"}, + }, + wantStatus: "error", + wantErrMsg: `{"code":"E_BAD_ARG"}`, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + status, errMsg := activityStatusForResult(tt.result) + assert.Equal(t, tt.wantStatus, status) + assert.Equal(t, tt.wantErrMsg, errMsg) + }) + } +} + +// The activity log is not a place to dump a multi-megabyte upstream error +// body: every record is persisted and streamed to the UI. The message is +// capped, and the cap must never split a multi-byte rune. +func TestActivityStatusForResult_TruncatesLongErrors(t *testing.T) { + long := strings.Repeat("é", activityErrorMessageLimit) + status, errMsg := activityStatusForResult(mcp.NewToolResultError(long)) + + require.Equal(t, "error", status) + assert.LessOrEqual(t, len(errMsg), activityErrorMessageLimit+len(activityErrorMessageEllipsis)) + assert.True(t, strings.HasSuffix(errMsg, activityErrorMessageEllipsis), + "a truncated message must be marked as truncated, got %q", errMsg) + assert.True(t, strings.HasPrefix(long, strings.TrimSuffix(errMsg, activityErrorMessageEllipsis)), + "truncation must keep a valid prefix of the original text") +} + +// Guard against the regression coming back through a new call path (same +// technique as TestActivityIDsAreNeverMintedInline). A post-dispatch emit site +// that hardcodes the literal "success" cannot have consulted the result, so it +// will mis-classify an isError answer no matter how good the classifier is. +// +// statusArgIndex maps an emit helper to the 0-based position of its `status` +// parameter. +// +// Only emitActivityToolCallCompleted is covered: every one of its call sites +// reports an UPSTREAM dispatch, so every one of them must consult the result. +// emitActivityInternalToolCall is deliberately excluded — most of its call +// sites report on mcpproxy's OWN handlers (retrieve_tools, describe_tool, +// upstream_servers …), where a literal "success" is correct. The one site that +// mirrors an upstream dispatch (call_tool_*) is covered by the end-to-end test +// TestE2E_UpstreamIsErrorRecordedAsActivityError instead. +var statusArgIndex = map[string]int{ + // (serverName, toolName, sessionID, requestID, source, status, ...) + "emitActivityToolCallCompleted": 5, +} + +func TestActivityCompletionNeverHardcodesSuccess(t *testing.T) { + entries, err := os.ReadDir(".") + require.NoError(t, err) + + fset := token.NewFileSet() + var offenders []string + + for _, entry := range entries { + name := entry.Name() + if entry.IsDir() || !strings.HasSuffix(name, ".go") || strings.HasSuffix(name, "_test.go") { + continue + } + + file, err := parser.ParseFile(fset, filepath.Join(".", name), nil, 0) + require.NoErrorf(t, err, "parsing %s", name) + + ast.Inspect(file, func(node ast.Node) bool { + call, ok := node.(*ast.CallExpr) + if !ok { + return true + } + sel, ok := call.Fun.(*ast.SelectorExpr) + if !ok { + return true + } + idx, tracked := statusArgIndex[sel.Sel.Name] + if !tracked || len(call.Args) <= idx { + return true + } + lit, ok := call.Args[idx].(*ast.BasicLit) + if !ok || lit.Kind != token.STRING || lit.Value != `"success"` { + return true + } + offenders = append(offenders, + fset.Position(lit.Pos()).String()+" ("+sel.Sel.Name+")") + return true + }) + } + + require.Emptyf(t, offenders, + "these emit a hardcoded success status and therefore record an "+ + "isError:true upstream answer as a success (issue #935); classify "+ + "the dispatched result with activityStatusForResult instead:\n%s", + strings.Join(offenders, "\n")) +} diff --git a/internal/server/e2e_iserror_activity_test.go b/internal/server/e2e_iserror_activity_test.go new file mode 100644 index 00000000..db3417a4 --- /dev/null +++ b/internal/server/e2e_iserror_activity_test.go @@ -0,0 +1,193 @@ +package server + +import ( + "context" + "fmt" + "net" + "net/http" + "testing" + "time" + + "github.com/mark3labs/mcp-go/mcp" + mcpserver "github.com/mark3labs/mcp-go/server" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "go.uber.org/zap" + + "github.com/smart-mcp-proxy/mcpproxy-go/internal/storage" +) + +// CreateMockIsErrorServer starts a mock MCP server exposing two tools: one that +// answers normally and one that answers with isError:true — the shape an +// upstream uses to report a bad argument value, an unknown tool, or a +// server-side validation failure. The transport call succeeds in both cases. +func (env *TestEnvironment) CreateMockIsErrorServer(name string) *MockUpstreamServer { + mcpServer := mcpserver.NewMCPServer(name, "1.0.0-test", mcpserver.WithToolCapabilities(true)) + mockServer := &MockUpstreamServer{server: mcpServer} + + okTool := mcp.Tool{ + Name: "get_time", + Description: "Returns a fixed time", + InputSchema: mcp.ToolInputSchema{Type: "object", Properties: map[string]interface{}{}}, + } + failTool := mcp.Tool{ + Name: "bad_timezone", + Description: "Always answers with isError:true, like a real server rejecting an argument", + InputSchema: mcp.ToolInputSchema{Type: "object", Properties: map[string]interface{}{}}, + } + mockServer.tools = []mcp.Tool{okTool, failTool} + + mcpServer.AddTool(okTool, func(_ context.Context, _ mcp.CallToolRequest) (*mcp.CallToolResult, error) { + return mcp.NewToolResultText("2026-01-01T00:00:00Z"), nil + }) + mcpServer.AddTool(failTool, func(_ context.Context, _ mcp.CallToolRequest) (*mcp.CallToolResult, error) { + return mcp.NewToolResultError("Invalid timezone: 'Mars/Olympus'"), nil + }) + + streamableServer := mcpserver.NewStreamableHTTPServer(mcpServer) + + ln, err := net.Listen("tcp", ":0") + require.NoError(env.t, err) + port := ln.Addr().(*net.TCPAddr).Port + ln.Close() + + mockServer.addr = fmt.Sprintf("http://localhost:%d", port) + httpServer := &http.Server{ + Addr: fmt.Sprintf(":%d", port), + Handler: streamableServer, + ReadHeaderTimeout: 5 * time.Second, + } + mockServer.httpServer = httpServer + mockServer.stopFunc = httpServer.Close + + go func() { + if err := httpServer.ListenAndServe(); err != nil && err != http.ErrServerClosed { + env.logger.Error("Mock isError server error", zap.Error(err)) + } + }() + + time.Sleep(200 * time.Millisecond) + + env.mockServers[name] = mockServer + return mockServer +} + +// TestE2E_UpstreamIsErrorRecordedAsActivityError is the regression test for +// issue #935. +// +// Before the fix, a call whose upstream answered isError:true landed in the +// activity log with status="success" — the transport hop had worked, so err was +// nil and the emit site hardcoded "success". That hid the most common real +// failure from the tray glance error markers and from the 24h error counts. +// +// The test drives the real dispatch path (client → /mcp → call_tool_read → +// upstream) and asserts on what is actually persisted, so it fails if any layer +// between the classifier and the activity store drops the classification. +func TestE2E_UpstreamIsErrorRecordedAsActivityError(t *testing.T) { + env := NewTestEnvironment(t) + defer env.Cleanup() + + mockServer := env.CreateMockIsErrorServer("flaky") + + mcpClient := env.CreateProxyClient() + defer mcpClient.Close() + env.ConnectClient(mcpClient) + + ctx := context.Background() + + addRequest := mcp.CallToolRequest{} + addRequest.Params.Name = "upstream_servers" + addRequest.Params.Arguments = map[string]interface{}{ + "operation": "add", + "name": "flaky", + "url": mockServer.addr, + "protocol": "streamable-http", + "enabled": true, + } + _, err := mcpClient.CallTool(ctx, addRequest) + require.NoError(t, err) + + rt := env.proxyServer.runtime + + serverConfig, err := rt.StorageManager().GetUpstreamServer("flaky") + require.NoError(t, err) + serverConfig.Quarantined = false + require.NoError(t, rt.StorageManager().SaveUpstreamServer(serverConfig)) + + servers, err := rt.StorageManager().ListUpstreamServers() + require.NoError(t, err) + cfg := rt.Config() + cfg.Servers = servers + require.NoError(t, rt.LoadConfiguredServers(cfg)) + + time.Sleep(3 * time.Second) + _ = rt.DiscoverAndIndexTools(ctx) + time.Sleep(3 * time.Second) + + callRequest := mcp.CallToolRequest{} + callRequest.Params.Name = "call_tool_read" + callRequest.Params.Arguments = map[string]interface{}{ + "name": "flaky:bad_timezone", + "args": map[string]interface{}{}, + "intent": map[string]interface{}{"operation_type": "read"}, + } + + callResult, err := mcpClient.CallTool(ctx, callRequest) + require.NoError(t, err) + require.NotNil(t, callResult) + + // The caller's answer is untouched: the upstream's own isError result is + // still forwarded verbatim. Only the activity classification changes. + assert.True(t, callResult.IsError, + "the upstream's isError result must still reach the caller unchanged") + + record := awaitToolCallActivity(t, rt, "flaky", "bad_timezone") + assert.Equal(t, "error", record.Status, + "an isError:true upstream answer must be recorded as an error (issue #935)") + assert.Contains(t, record.ErrorMessage, "Mars/Olympus", + "the recorded error_message must carry the upstream's own explanation") + + // And a genuinely successful call on the same server is still a success — + // the classifier must not paint everything red. + okRequest := mcp.CallToolRequest{} + okRequest.Params.Name = "call_tool_read" + okRequest.Params.Arguments = map[string]interface{}{ + "name": "flaky:get_time", + "args": map[string]interface{}{}, + "intent": map[string]interface{}{"operation_type": "read"}, + } + okResult, err := mcpClient.CallTool(ctx, okRequest) + require.NoError(t, err) + require.False(t, okResult.IsError) + + okRecord := awaitToolCallActivity(t, rt, "flaky", "get_time") + assert.Equal(t, "success", okRecord.Status) + assert.Empty(t, okRecord.ErrorMessage) +} + +// awaitToolCallActivity polls the activity store for the tool_call record of a +// given server/tool. Activity is written asynchronously off the event bus, so a +// single immediate read races the writer. +func awaitToolCallActivity(t *testing.T, rt interface { + ListActivities(storage.ActivityFilter) ([]*storage.ActivityRecord, int, error) +}, serverName, toolName string) *storage.ActivityRecord { + t.Helper() + + deadline := time.Now().Add(10 * time.Second) + for { + records, _, err := rt.ListActivities(storage.ActivityFilter{ + Types: []string{string(storage.ActivityTypeToolCall)}, + Server: serverName, + Tool: toolName, + Limit: 50, + }) + require.NoError(t, err) + if len(records) > 0 { + return records[0] + } + if time.Now().After(deadline) { + t.Fatalf("no tool_call activity recorded for %s:%s within 10s", serverName, toolName) + } + time.Sleep(100 * time.Millisecond) + } +} diff --git a/internal/server/mcp.go b/internal/server/mcp.go index efb99a20..3ec24117 100644 --- a/internal/server/mcp.go +++ b/internal/server/mcp.go @@ -2212,8 +2212,19 @@ func (p *MCPProxyServer) handleCallToolVariant(ctx context.Context, request mcp. // activation flag means "succeeded", not merely "attempted". p.recordRealToolCallSuccess() - // Record successful response + // Issue #935: the transport hop succeeded, but the upstream may still have + // ANSWERED "this failed" via isError:true (bad argument value, unknown + // tool, server-side validation). Classify here — before encodeToonBlocks / + // spotlighting mutate the result in place — so the recorded message is the + // upstream's own words. This governs the ACTIVITY RECORD ONLY; the result + // itself is still forwarded to the caller verbatim below. + activityStatus, activityErrMsg := activityStatusForResult(result) + + // Record the response. An isError answer is still a response — it is stored + // as one, with the upstream's explanation mirrored into Error so the tool + // call history agrees with the activity log. toolCallRecord.Response = result + toolCallRecord.Error = activityErrMsg // Count output tokens for successful response if tokenMetrics != nil && p.mainServer != nil && p.mainServer.runtime != nil { @@ -2316,11 +2327,14 @@ func (p *MCPProxyServer) handleCallToolVariant(ctx context.Context, request mcp. if intent != nil { intentMap = intent.ToMap() } - p.emitActivityToolCallCompleted(serverName, actualToolName, sessionID, requestID, activitySource, "success", "", duration.Milliseconds(), activityArgs, response, responseTruncated, toolVariant, intentMap, contentTrust, profileSlug, activityRequestBytes, activityResponseBytes, toonDetectionText, toonDecisions) + p.emitActivityToolCallCompleted(serverName, actualToolName, sessionID, requestID, activitySource, activityStatus, activityErrMsg, duration.Milliseconds(), activityArgs, response, responseTruncated, toolVariant, intentMap, contentTrust, profileSlug, activityRequestBytes, activityResponseBytes, toonDetectionText, toonDecisions) - // Spec 024: Emit internal tool call event for success + // Spec 024: Emit internal tool call event. It carries the SAME classification + // as the tool_call record above (issue #935) — the two describe one dispatch, + // and a "success" wrapper around a failed call is exactly what made the + // failure invisible. internalToolName := "call_tool_" + intent.OperationType // e.g., "call_tool_read" - p.emitActivityInternalToolCall(internalToolName, serverName, actualToolName, toolVariant, sessionID, requestID, "success", "", time.Since(internalStartTime).Milliseconds(), activityArgs, result, intentMap, "") + p.emitActivityInternalToolCall(internalToolName, serverName, actualToolName, toolVariant, sessionID, requestID, activityStatus, activityErrMsg, time.Since(internalStartTime).Milliseconds(), activityArgs, result, intentMap, "") return forwarded, nil } @@ -2632,8 +2646,14 @@ func (p *MCPProxyServer) handleCallTool(ctx context.Context, request mcp.CallToo return p.createDetailedErrorResponse(err, serverName, actualToolName), nil } - // Record successful response + // Issue #935: an upstream that answered isError:true failed, even though the + // transport hop did not. Classified before the result is truncated/forwarded. + activityStatus, activityErrMsg := activityStatusForResult(result) + + // Record the response (an isError answer is still a response; its + // explanation is mirrored into Error so history agrees with activity). toolCallRecord.Response = result + toolCallRecord.Error = activityErrMsg // Count output tokens for successful response if tokenMetrics != nil && p.mainServer != nil && p.mainServer.runtime != nil { @@ -2714,9 +2734,10 @@ func (p *MCPProxyServer) handleCallTool(ctx context.Context, request mcp.CallToo p.sessionStore.UpdateSessionStats(sessionID, tokenMetrics.TotalTokens) } - // Emit activity completed event for success with determined source (legacy - no intent) + // Emit activity completed event with determined source (legacy - no intent). + // Status comes from the upstream result, not from err alone (issue #935). responseTruncated := tokenMetrics != nil && tokenMetrics.WasTruncated - p.emitActivityToolCallCompleted(serverName, actualToolName, sessionID, requestID, activitySource, "success", "", duration.Milliseconds(), activityArgs, response, responseTruncated, "", nil, "", "", legacyRequestBytes, legacyResponseBytes, "", nil) + p.emitActivityToolCallCompleted(serverName, actualToolName, sessionID, requestID, activitySource, activityStatus, activityErrMsg, duration.Milliseconds(), activityArgs, response, responseTruncated, "", nil, "", "", legacyRequestBytes, legacyResponseBytes, "", nil) return forwarded, nil } diff --git a/internal/server/mcp_routing.go b/internal/server/mcp_routing.go index cb8e1be0..4031980e 100644 --- a/internal/server/mcp_routing.go +++ b/internal/server/mcp_routing.go @@ -229,6 +229,11 @@ func (p *MCPProxyServer) makeDirectModeHandler(serverName, toolName string, anno // Determine tool variant for activity logging toolVariant := contracts.DeriveCallWith(annotations) + // Issue #935: direct mode reaches the same upstreams as call_tool_*, so + // it must classify an isError:true answer as a failure too. Read from + // the raw result, before the truncation loop below rewrites it. + activityStatus, activityErrMsg := activityStatusForResult(result) + // Forward content blocks (preserving ImageContent, AudioContent, etc.) // while applying truncation only to TextContent. See issue #368. // @@ -292,11 +297,12 @@ func (p *MCPProxyServer) makeDirectModeHandler(serverName, toolName string, anno forwarded = mcp.NewToolResultText(responseText) } - // Emit success activity + // Emit completion activity (success, or error when the upstream itself + // reported one — issue #935). // Spec 069 A1: pre-truncation sizes; result was measured before the truncation loop above. routingResponseBytes := rawByteSize(result) routingRequestBytes := rawByteSize(enrichedArgs) - p.emitActivityToolCallCompleted(serverName, toolName, sessionID, requestID, "mcp", "success", "", durationMs, enrichedArgs, responseText, truncated, toolVariant, nil, directContentTrust, "", routingRequestBytes, routingResponseBytes, "", nil) + p.emitActivityToolCallCompleted(serverName, toolName, sessionID, requestID, "mcp", activityStatus, activityErrMsg, durationMs, enrichedArgs, responseText, truncated, toolVariant, nil, directContentTrust, "", routingRequestBytes, routingResponseBytes, "", nil) return forwarded, nil } diff --git a/internal/storage/async_ops_test.go b/internal/storage/async_ops_test.go index 0d5e52b7..807b441c 100644 --- a/internal/storage/async_ops_test.go +++ b/internal/storage/async_ops_test.go @@ -361,6 +361,12 @@ func TestSaveServerSyncFieldCoverage(t *testing.T) { // UpstreamRecord so a REST/UI-set override survives a restart and a // SaveConfiguration rebuild of the JSON server list. "ToonOutput": true, + // Issue #937: unexported parse-time bit recording whether the JSON + // document carried a "quarantined" key. It describes the DOCUMENT that + // was parsed, not the server, so it is deliberately NOT persisted — a + // record read back from BBolt is not a config file and must not claim + // the operator stated anything. + "quarantineExplicitlySet": true, } // Get all fields from ServerConfig @@ -400,6 +406,10 @@ func TestSaveServerSyncFieldCoverage(t *testing.T) { // Spec 074: server-edition JSON-config field, not persisted to BBolt continue } + if fieldName == "quarantineExplicitlySet" { + // Issue #937: parse-time-only bit, never persisted (see above) + continue + } if !upstreamFields[fieldName] { t.Errorf("Expected field %q in UpstreamRecord but not found", fieldName) } From 0d9617f24361bc2933f5487a283273a44eda3b71 Mon Sep 17 00:00:00 2001 From: Algis Dumbris Date: Sat, 1 Aug 2026 09:07:36 +0300 Subject: [PATCH 2/3] fix: close the review gaps in the #937 config-load admission gate MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Cross-model review of PR #944 found six ways the gate could be bypassed, inverted, or raced. All six are fixed with a failing test first. P1 — any mcpproxy-initiated config save permanently disarmed the gate. ServerConfig.Quarantined had no omitempty and SaveConfig is a plain MarshalIndent, so one write of the config file stamped "quarantined": false onto every server; reading that back set the presence bit and the gate skipped admission forever — including for servers it had just quarantined, whose config.db record the next start then overwrote with false. ServerConfig now has a MarshalJSON that omits the key when the value is false and no document ever stated it, and always writes it when true. P2 — a transient storage read failure mass-quarantined everything. A failed ListUpstreamServers() was flattened into "no servers are known", so every configured server looked first-seen and was walled off, permanently. The gate now takes an explicit storage-readable flag and abstains when it is false. P2 — the gate mutated an already-published config snapshot. It wrote through the *config.ServerConfig pointers the supervisor and serverEligibleForIndexing read concurrently, and both hot paths published before gating. The gate now returns a copy and never writes through its input; configsvc gained a pre-publish hook so every configuration is gated before any subscriber can observe it; ApplyConfig gates before its disk write (covering the RequiresRestart branch, which never reaches LoadConfiguredServers); and the startup snapshot is gated before supervisor.Start(). P2 — installs already hit by #937 stay live after upgrading, because the buggy admission left exactly the config.db record the gate reads as vetted. That tradeoff stays (upgrade safety), but it is no longer silent: startup names the affected servers in a warning, and the docs carry an "action required" upgrade section. A human quarantine toggle now records itself as an explicit value in the config document, so a reviewed server is never reported. P2 — "quarantined": null counted as an operator statement, contradicting the codebase's own RFC 7396 null-means-unset convention and letting a templating tool admit a first-seen server. Null is now absence. P2 — code_execution is a fourth upstream dispatch path and still classified on err == nil alone, so an upstream CallToolResult{IsError:true} was stored in tool-call history as a clean success while the identical call through call_tool_read recorded the upstream's message. Nested calls now share the issue #935 classifier; the code_execution wrapper deliberately keeps its own result.Ok outcome, documented inline. Related #937 Related #935 --- docs/features/security-quarantine.md | 32 ++ internal/config/config.go | 51 ++- internal/config/quarantine_presence_test.go | 85 ++++- .../runtime/config_load_admission_gate.go | 305 ++++++++++++++++-- .../config_load_admission_gate_review_test.go | 242 ++++++++++++++ .../config_load_admission_gate_test.go | 19 +- internal/runtime/configsvc/service.go | 40 +++ internal/runtime/lifecycle.go | 60 +++- internal/runtime/runtime.go | 9 + .../code_execution_result_status_test.go | 105 ++++++ internal/server/mcp_code_execution.go | 70 +++- 11 files changed, 961 insertions(+), 57 deletions(-) create mode 100644 internal/runtime/config_load_admission_gate_review_test.go create mode 100644 internal/server/code_execution_result_status_test.go diff --git a/docs/features/security-quarantine.md b/docs/features/security-quarantine.md index 44938db4..a08d8d62 100644 --- a/docs/features/security-quarantine.md +++ b/docs/features/security-quarantine.md @@ -58,6 +58,38 @@ writes both the config file and `config.db`. No TPA scan runs at config-load admission; quarantine-by-default already keeps the tools away from agents, and the scan is what the human review step performs. +If mcpproxy writes the configuration file itself (any API apply, a quarantine +toggle, an `upstream add`), it does **not** stamp `"quarantined": false` onto +servers that never stated it — a value mcpproxy invented would otherwise read +back as an operator statement and disable the gate for that server. + +#### Upgrading from a release affected by #937 — action required + +The gate treats "present in `config.db`" as "has already been through +admission". That is what keeps upgrades safe, but it also means an install that +was **already** hit by #937 is not remediated by upgrading: the buggy admission +left exactly the `config.db` record the gate now reads as vetted, so a server +that was admitted unquarantined stays live. + +At startup mcpproxy logs a warning naming any server that looks like this — +configured, running unquarantined, never explicitly reviewed, and with a +`trust_mode` that would have held it: + +``` +Configured servers predate the config-load admission gate and have never been +explicitly reviewed servers=["suspicious-server"] +``` + +For each server named, either: + +- review it and record the decision — quarantining and then releasing it from + the quarantine UI writes an explicit `"quarantined"` value to + `mcp_config.json`, which silences the warning; or +- write the decision by hand: add `"quarantined": true` (hold it) or + `"quarantined": false` (you have vetted it) to that server's entry. + +Adding the key by hand is enough — the gate obeys an explicit value either way. + ### Tool Discovery and Search Isolation **Quarantined servers are completely isolated from the tool discovery and search system:** diff --git a/internal/config/config.go b/internal/config/config.go index 556dbb40..edcc1f25 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -1,6 +1,7 @@ package config import ( + "bytes" "crypto/rand" "encoding/hex" "encoding/json" @@ -1644,8 +1645,14 @@ func (c *Config) DefaultQuarantineForNewServer() bool { // // The presence bit is what distinguishes "the operator opted out" from "the // operator never said anything", and it is the only thing the config-load -// admission gate keys off. It is deliberately unexported and never serialized: -// it describes the DOCUMENT that was parsed, not the server. +// admission gate keys off. It is deliberately unexported: it describes the +// DOCUMENT that was parsed, not the server. MarshalJSON below round-trips it by +// OMITTING the key rather than by writing the bit out. +// +// A JSON `null` does NOT count as a statement. Everywhere else in mcpproxy null +// means "unset" (RFC 7396 merge-patch semantics — see the env_json handling in +// internal/server/mcp.go), and a templating tool or serializer that emits null +// for an unset field must not be able to silently admit a first-seen server. // // Note this only fires for JSON decoding. Structs built in Go (REST handlers, // the add paths, tests) leave the bit false — i.e. "unstated" — which is the @@ -1662,10 +1669,48 @@ func (sc *ServerConfig) UnmarshalJSON(data []byte) error { // already have failed on anything we care about. return nil //nolint:nilerr // presence detection is best-effort } - _, sc.quarantineExplicitlySet = probe["quarantined"] + raw, present := probe["quarantined"] + sc.quarantineExplicitlySet = present && !isJSONNull(raw) return nil } +// isJSONNull reports whether a raw JSON value is the literal `null`. +func isJSONNull(raw json.RawMessage) bool { + return string(bytes.TrimSpace(raw)) == "null" +} + +// MarshalJSON writes a ServerConfig, omitting "quarantined" when the value is +// false AND no configuration document ever stated it (issue #937, review P1). +// +// Without this, mcpproxy's own SaveConfig fabricated an operator statement: +// `Quarantined` is a plain bool with no omitempty, so every save stamped +// `"quarantined": false` onto servers that had never mentioned the key. Reading +// that back set the presence bit, and the config-load admission gate then +// skipped the server permanently — including servers the gate had itself just +// quarantined, whose config.db record the next start would overwrite with +// false. One API-triggered save disarmed the gate for good. +// +// A true value is ALWAYS written: a gate decision persisted to disk must be +// readable back, and quarantine is never expressed by absence. +func (sc *ServerConfig) MarshalJSON() ([]byte, error) { + type Alias ServerConfig + + // The shallower field dominates the embedded alias's "quarantined" tag + // (encoding/json resolves name conflicts by depth), so a nil pointer here + // drops the key entirely instead of emitting it twice. + aux := struct { + Quarantined *bool `json:"quarantined,omitempty"` + *Alias + }{Alias: (*Alias)(sc)} + + if sc.Quarantined || sc.quarantineExplicitlySet { + v := sc.Quarantined + aux.Quarantined = &v + } + + return json.Marshal(aux) +} + // QuarantineExplicitlySet reports whether the parsed configuration document // stated a "quarantined" value for this server. False means the key was absent // (or the struct was built in Go rather than decoded), which the config-load diff --git a/internal/config/quarantine_presence_test.go b/internal/config/quarantine_presence_test.go index 7a2f3356..9734fa42 100644 --- a/internal/config/quarantine_presence_test.go +++ b/internal/config/quarantine_presence_test.go @@ -2,6 +2,8 @@ package config import ( "encoding/json" + "os" + "path/filepath" "testing" "github.com/stretchr/testify/assert" @@ -42,9 +44,14 @@ func TestServerConfig_QuarantineExplicitlySet(t *testing.T) { wantValue: true, }, { - name: "explicit null counts as stated", + // Review finding: `null` is the codebase's "unset" marker elsewhere + // (RFC 7396 merge-patch semantics on env_json, mcp.go). Counting it + // as an operator statement let a templating tool or a serializer + // that emits null for an unset field silently admit a first-seen + // server unquarantined. + name: "explicit null is NOT a statement", raw: `{"name":"handwritten","command":"./poison","quarantined":null}`, - wantExplicit: true, + wantExplicit: false, wantValue: false, }, } @@ -92,3 +99,77 @@ func TestConfigLoad_ServersCarryQuarantinePresence(t *testing.T) { assert.True(t, cfg.Servers[0].QuarantineExplicitlySet(), "stated") assert.False(t, cfg.Servers[1].QuarantineExplicitlySet(), "unstated") } + +// Review finding (P1) — mcpproxy's OWN config save must not fabricate an +// operator statement. +// +// `Quarantined` carries `json:"quarantined"` with no omitempty and SaveConfig is +// a plain json.MarshalIndent, so the moment mcpproxy wrote the config file every +// server gained a literal `"quarantined": false`. Reading that back set +// quarantineExplicitlySet=true, and the admission gate skipped the server +// forever — including servers the gate had itself just quarantined, whose +// config.db record was then overwritten with false on the next start. +// +// A save must round-trip the PRESENCE bit, not invent one. +func TestSaveConfig_DoesNotFabricateQuarantineStatement(t *testing.T) { + dir := t.TempDir() + p := filepath.Join(dir, "mcp_config.json") + + raw := `{ + "listen": "127.0.0.1:0", + "api_key": "k", + "mcpServers": [ + {"name":"unstated","command":"a","protocol":"stdio","enabled":true}, + {"name":"stated-false","command":"b","protocol":"stdio","enabled":true,"quarantined":false}, + {"name":"stated-true","command":"c","protocol":"stdio","enabled":true,"quarantined":true} + ] + }` + require.NoError(t, os.WriteFile(p, []byte(raw), 0600)) + + cfg, err := LoadFromFile(p) + require.NoError(t, err) + require.NoError(t, SaveConfig(cfg, p)) + + // The saved document itself must not carry a key the operator never wrote. + saved, err := os.ReadFile(p) + require.NoError(t, err) + var doc struct { + Servers []map[string]json.RawMessage `json:"mcpServers"` + } + require.NoError(t, json.Unmarshal(saved, &doc)) + require.Len(t, doc.Servers, 3) + _, present := doc.Servers[0]["quarantined"] + assert.False(t, present, + "mcpproxy must not write a quarantined key for a server that never stated one") + _, present = doc.Servers[1]["quarantined"] + assert.True(t, present, "an explicit quarantined:false must survive the save") + _, present = doc.Servers[2]["quarantined"] + assert.True(t, present, "quarantined:true must always be written") + + // And the reload must agree, since that is what the admission gate reads. + reloaded, err := LoadFromFile(p) + require.NoError(t, err) + require.Len(t, reloaded.Servers, 3) + assert.False(t, reloaded.Servers[0].QuarantineExplicitlySet(), + "a save/reload round-trip must not turn an unstated server into a stated one") + assert.True(t, reloaded.Servers[1].QuarantineExplicitlySet()) + assert.True(t, reloaded.Servers[2].QuarantineExplicitlySet()) + assert.True(t, reloaded.Servers[2].Quarantined) +} + +// A quarantined server must always serialize the flag even when the config file +// it came from never mentioned it — otherwise a gate decision written to disk +// would evaporate on the next load. +func TestServerConfig_MarshalJSON_AlwaysWritesTrue(t *testing.T) { + var sc ServerConfig + require.NoError(t, json.Unmarshal([]byte(`{"name":"s","command":"c"}`), &sc)) + require.False(t, sc.QuarantineExplicitlySet()) + + sc.Quarantined = true + encoded, err := json.Marshal(&sc) + require.NoError(t, err) + + var probe map[string]json.RawMessage + require.NoError(t, json.Unmarshal(encoded, &probe)) + assert.Equal(t, "true", string(probe["quarantined"])) +} diff --git a/internal/runtime/config_load_admission_gate.go b/internal/runtime/config_load_admission_gate.go index 653d8c78..c5634594 100644 --- a/internal/runtime/config_load_admission_gate.go +++ b/internal/runtime/config_load_admission_gate.go @@ -4,6 +4,7 @@ import ( "go.uber.org/zap" "github.com/smart-mcp-proxy/mcpproxy-go/internal/config" + "github.com/smart-mcp-proxy/mcpproxy-go/internal/runtime/configsvc" ) // Issue #937 — the trust-mode admission gate on the CONFIG-LOAD path. @@ -20,14 +21,15 @@ import ( // // # What is gated // -// A server is gated only when BOTH hold: +// A server is gated only when ALL of these hold: // +// - config.db was readable (see "Storage failures" below), AND // - the parsed config document never stated a `quarantined` value // (ServerConfig.QuarantineExplicitlySet() — an operator who wrote the key, // either way, has spoken and is obeyed), AND // - the server is not yet known to config.db. // -// # The upgrade boundary (deliberate, and the reason for the second condition) +// # The upgrade boundary (deliberate, and the reason for the last condition) // // "Not yet known to config.db" is the proxy for "has never been through // admission". Every server an existing user is running already has a config.db @@ -35,20 +37,51 @@ import ( // a server the user has already vetted must not suddenly disappear behind a // quarantine wall on a version bump. // -// The boundary this leaves is precise and worth stating: a server that is in a +// The cost of that choice is that an install ALREADY hit by #937 keeps its +// poisoned server live, because the buggy admission left exactly the config.db +// record this gate reads as "vetted". Nothing distinguishes the two at load +// time, so instead of guessing, reportPreFixAdmissions below names those +// servers in a WARN so an operator can review them. See the "Upgrading" section +// of docs/features/security-quarantine.md. +// +// The other boundary is precise and worth stating: a server that is in a // hand-written config but NOT in config.db is treated as first-seen. That is // true after `rm -rf ~/.mcpproxy/*.db`, or when a config file is moved to a // machine whose data dir has never seen it — both of which are exactly the // "config files get shared, templated, copied between machines" case the issue // calls out, so gating them is the intended behaviour rather than a regression. // +// # Storage failures +// +// A failed ListUpstreamServers() must NOT be flattened into "no servers are +// known" — that would make every configured server look first-seen and wall off +// the user's entire server list behind a quarantine, permanently, because the +// next boot would inherit the decision. A single bbolt lock contention at boot +// is not a security event. When storage is unreadable the gate abstains +// entirely: unknown is not the same as new. +// // # Durability // -// The gate decision is persisted by the caller's SaveUpstreamServer. On the -// next start the server IS in config.db while the config file still says -// nothing, so the second branch below re-reads the stored value instead of -// letting an absent key silently resolve back to false. Without that, the hole -// would reopen one restart later. +// The gate decision is persisted by the caller's SaveUpstreamServer, and the +// second branch below re-reads it, so a config file that stays silent about +// `quarantined` inherits mcpproxy's own recorded state instead of resolving an +// absent key back to false. +// +// That durability depends on mcpproxy never FABRICATING an operator statement +// when it writes the config file. ServerConfig.MarshalJSON omits `quarantined` +// for a server that is not quarantined and never stated the key; without that, +// a single save (any `PUT /api/v1/config`) stamped `"quarantined": false` on +// every server, the presence check below skipped them forever, and the save +// loop in LoadConfiguredServers then wrote false over the config.db quarantine. +// +// # Immutability +// +// The gate NEVER writes through a *config.ServerConfig it was handed. Those +// pointers live in the snapshot published by configsvc, which the supervisor's +// reconcile loop and serverEligibleForIndexing read concurrently and which +// internal/server/server.go documents as immutable. A gated server is a COPY in +// a fresh Servers slice, and the caller republishes the whole config so +// subscribers observe the decision through an atomic snapshot swap. // // # Direction // @@ -64,39 +97,261 @@ import ( // prevents the tools from reaching an agent, and the scan is what the human // review step in the quarantine UI performs. Running a synchronous scan on the // startup path would block boot on every first-seen server. -func (r *Runtime) applyConfigLoadAdmissionGate(cfg *config.Config, stored map[string]*config.ServerConfig) { + +// admissionDecision is one server the gate wants to change, plus the activity +// text to publish once the decision has actually been committed. +type admissionDecision struct { + server string + reason string + newly bool +} + +// applyConfigLoadAdmissionGate resolves the admission decision for every server +// in cfg and returns the config to use. +// +// It never mutates cfg. When nothing changes it returns (cfg, false); otherwise +// it returns a copy whose gated entries are copies too, so an already-published +// snapshot is never written behind a subscriber's back. +// +// storageOK reports whether `stored` is a trustworthy view of config.db. When +// it is false the gate abstains: an unreadable database means "unknown", not +// "everything is new". +func (r *Runtime) applyConfigLoadAdmissionGate(cfg *config.Config, stored map[string]*config.ServerConfig, storageOK bool) (*config.Config, bool) { if cfg == nil { - return + return cfg, false } - for _, sc := range cfg.Servers { - if sc == nil || sc.Quarantined || sc.QuarantineExplicitlySet() { + if !storageOK { + r.logger.Warn("Skipping config-load admission gate: server storage is unreadable", + zap.String("reason", "an unreadable config.db cannot distinguish a first-seen server from a known one (issue #937)")) + return cfg, false + } + + decisions := make(map[int]admissionDecision) + var preFix []string + + for i, sc := range cfg.Servers { + if sc == nil || sc.Quarantined { continue } - if prev, known := stored[sc.Name]; known { + prev, known := stored[sc.Name] + if known { // Already been through admission. Inherit whatever mcpproxy itself // recorded, so a gate decision from a previous run survives a config // file that never mentions the key. - if prev != nil && prev.Quarantined { - sc.Quarantined = true - r.logger.Info("Retaining recorded quarantine for server with no explicit config value", - zap.String("server", sc.Name)) + if prev != nil && prev.Quarantined && !sc.QuarantineExplicitlySet() { + decisions[i] = admissionDecision{ + server: sc.Name, + reason: "retaining the quarantine recorded in config.db for a server whose config states nothing", + } + continue + } + // Known, live, never explicitly reviewed, and its trust mode says it + // should have been held: this is what an install hit by #937 before + // the fix looks like. Upgrade safety says leave it alone; honesty + // says say so. + if !sc.QuarantineExplicitlySet() && cfg.QuarantineDefaultForServer(sc) { + preFix = append(preFix, sc.Name) } continue } - if !cfg.QuarantineDefaultForServer(sc) { + if sc.QuarantineExplicitlySet() || !cfg.QuarantineDefaultForServer(sc) { continue } + decisions[i] = admissionDecision{ + server: sc.Name, + reason: "first-seen server from configuration file held for review by the " + + string(sc.EffectiveTrustMode()) + " trust mode", + newly: true, + } + } + + r.reportPreFixAdmissions(preFix) + + if len(decisions) == 0 { + return cfg, false + } + + gated := *cfg + gated.Servers = make([]*config.ServerConfig, len(cfg.Servers)) + copy(gated.Servers, cfg.Servers) + + for i, d := range decisions { + sc := config.CopyServerConfig(cfg.Servers[i]) sc.Quarantined = true - r.logger.Warn("Quarantining first-seen server from configuration file", - zap.String("server", sc.Name), - zap.String("trust_mode", string(sc.EffectiveTrustMode())), - zap.String("reason", "config-load admission gate: server has not been reviewed (issue #937)")) - r.EmitActivityQuarantineChange(sc.Name, true, - "first-seen server from configuration file held for review by the "+ - string(sc.EffectiveTrustMode())+" trust mode") + gated.Servers[i] = sc + + if d.newly { + r.logger.Warn("Quarantining first-seen server from configuration file", + zap.String("server", d.server), + zap.String("trust_mode", string(sc.EffectiveTrustMode())), + zap.String("reason", "config-load admission gate: server has not been reviewed (issue #937)")) + } else { + r.logger.Info("Retaining recorded quarantine for server with no explicit config value", + zap.String("server", d.server)) + } + } + + // Activity is emitted off the caller's goroutine: this runs inside + // configsvc's publish lock on the hot path, and an event subscriber must + // never be able to stall a config update. + pending := make([]admissionDecision, 0, len(decisions)) + for _, d := range decisions { + if d.newly { + pending = append(pending, d) + } + } + if len(pending) > 0 { + go func() { + for _, d := range pending { + r.EmitActivityQuarantineChange(d.server, true, d.reason) + } + }() + } + + return &gated, true +} + +// reportPreFixAdmissions warns about servers that are live, unreviewed, and +// would have been held by their trust mode — the signature of an install that +// was admitted by the #937 bug before it was fixed. The gate deliberately does +// not re-quarantine them (that would break upgrade safety for every legitimately +// vetted server), so this is the only signal an affected operator gets. +func (r *Runtime) reportPreFixAdmissions(names []string) { + if len(names) == 0 { + return + } + 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)")) +} + +// storedServersForAdmission reads config.db's view of the configured servers. +// The second return value reports whether that view is trustworthy; callers +// must not treat a failed read as an empty database. +func (r *Runtime) storedServersForAdmission() (map[string]*config.ServerConfig, bool) { + if r.storageManager == nil { + return nil, false + } + stored, err := r.storageManager.ListUpstreamServers() + if err != nil { + r.logger.Error("Failed to read stored servers for the admission gate", zap.Error(err)) + return nil, false + } + byName := make(map[string]*config.ServerConfig, len(stored)) + for _, s := range stored { + if s != nil { + byName[s.Name] = s + } + } + return byName, true +} + +// gateConfigForAdmission is the one-call form used by paths that hold a config +// they have not published yet (ApplyConfig before its disk write, and the +// configsvc pre-publish hook). It reads storage itself. +func (r *Runtime) gateConfigForAdmission(cfg *config.Config) *config.Config { + stored, ok := r.storedServersForAdmission() + gated, _ := r.applyConfigLoadAdmissionGate(cfg, stored, ok) + return gated +} + +// installAdmissionGateHook wires the gate into configsvc so that EVERY +// configuration published to subscribers has already been through admission. +// +// Without it there is a real exposure window: ReloadConfiguration publishes the +// freshly-parsed file (waking the supervisor's reconcile loop) and only then +// calls LoadConfiguredServers, so a poisoned server hand-added to a running +// mcpproxy could be connected and its tools indexed before the gate flipped the +// bit. +func (r *Runtime) installAdmissionGateHook() { + if r.configSvc == nil { + return + } + r.configSvc.SetPrePublishHook(func(cfg *config.Config) *config.Config { + return r.gateConfigForAdmission(cfg) + }) +} + +// gateInitialConfig applies admission to the snapshot configsvc was constructed +// with. NewService stores that snapshot directly, so it never passes through the +// pre-publish hook; this must run before anything can observe it (i.e. before +// supervisor.Start()). +func (r *Runtime) gateInitialConfig() { + if r.configSvc == nil { + return + } + current := r.configSvc.Current() + if current == nil || current.Config == nil { + return + } + if gated := r.gateConfigForAdmission(current.Config); gated != current.Config { + if err := r.configSvc.Update(gated, configsvc.UpdateTypeModify, "config_load_admission_gate"); err != nil { + r.logger.Error("Failed to publish admission-gated startup configuration", zap.Error(err)) + return + } + r.mu.Lock() + r.cfg = gated + r.mu.Unlock() + } +} + +// markQuarantineDecisionExplicit records, in the published configuration, that +// a quarantine value has been stated for this server. Called when a human +// toggles quarantine: config.db cannot carry the presence bit, so without this +// the next SaveConfiguration would drop the key and the server would look +// never-reviewed again. +// +// Republished as a copy for the same reason the gate is: the snapshot's +// ServerConfig pointers are read concurrently by the supervisor. +func (r *Runtime) markQuarantineDecisionExplicit(serverName string) { + if r.configSvc == nil { + return + } + current := r.configSvc.Current() + if current == nil || current.Config == nil { + return + } + + for i, sc := range current.Config.Servers { + if sc == nil || sc.Name != serverName || sc.QuarantineExplicitlySet() { + continue + } + updated := *current.Config + updated.Servers = make([]*config.ServerConfig, len(current.Config.Servers)) + copy(updated.Servers, current.Config.Servers) + stamped := config.CopyServerConfig(sc) + stamped.MarkQuarantineExplicitlySet(true) + updated.Servers[i] = stamped + r.publishAdmissionGatedConfig(current.Config, &updated) + return + } +} + +// publishAdmissionGatedConfig swaps a gated config in for the one it was +// derived from, atomically, so subscribers see the decision. +// +// It is a no-op when `previous` is no longer the published config: someone +// committed a newer one while we were gating, and that config went through the +// pre-publish hook on its own way in. Clobbering it here would revert a +// concurrent apply. +func (r *Runtime) publishAdmissionGatedConfig(previous, gated *config.Config) { + if r.configSvc == nil || previous == gated { + return + } + if current := r.configSvc.Current(); current == nil || current.Config != previous { + return + } + if err := r.configSvc.Update(gated, configsvc.UpdateTypeModify, "config_load_admission_gate"); err != nil { + r.logger.Error("Failed to publish admission-gated configuration", zap.Error(err)) + return + } + r.mu.Lock() + if r.cfg == previous { + r.cfg = gated } + r.mu.Unlock() } diff --git a/internal/runtime/config_load_admission_gate_review_test.go b/internal/runtime/config_load_admission_gate_review_test.go new file mode 100644 index 00000000..91d42bae --- /dev/null +++ b/internal/runtime/config_load_admission_gate_review_test.go @@ -0,0 +1,242 @@ +package runtime + +import ( + "encoding/json" + "os" + "path/filepath" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "go.uber.org/zap" + "go.uber.org/zap/zaptest/observer" + + "github.com/smart-mcp-proxy/mcpproxy-go/internal/config" +) + +// Cross-model review findings on the #937 admission gate. + +// P1 — mcpproxy's own config save must not disarm the gate. +// +// `ServerConfig.Quarantined` had no omitempty and SaveConfig is a plain +// MarshalIndent, so any save (e.g. PUT /api/v1/config) stamped +// `"quarantined": false` onto every server. Reading that back set the presence +// bit, the gate skipped admission forever, and the save loop in +// LoadConfiguredServers then wrote false over the config.db quarantine — the +// poisoned server came back live one restart later. +func TestConfigLoadAdmissionGate_SurvivesMcpproxyOwnSave(t *testing.T) { + rt, cfg, path := gateEnvAt(t, []map[string]any{ + {"name": "poison", "command": "./evil", "protocol": "stdio", "enabled": true}, + }, nil, zap.NewNop()) + + // Run 1: the gate quarantines the first-seen server and it is persisted. + require.NoError(t, rt.LoadConfiguredServers(cfg)) + require.True(t, storedServer(t, rt, "poison").Quarantined) + + // mcpproxy writes the config file itself — exactly what ApplyConfig does + // BEFORE the gate ever runs on that apply. + require.NoError(t, config.SaveConfig(rt.Config(), path)) + + // Run 2: reload from that self-written file. + reloaded, err := config.LoadFromFile(path) + require.NoError(t, err) + require.NoError(t, rt.LoadConfiguredServers(reloaded)) + + assert.True(t, storedServer(t, rt, "poison").Quarantined, + "a config file mcpproxy wrote itself must not disarm the admission gate") +} + +// P1 variant — a server that is BOTH first-seen and only ever described by a +// config mcpproxy saved. Simulates PUT /api/v1/config adding a server whose +// apply required a restart, so the gate never ran in that process. +func TestConfigLoadAdmissionGate_GatesAfterSelfSaveOfNeverAdmittedServer(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "mcp_config.json") + raw, err := json.Marshal(map[string]any{ + "listen": "127.0.0.1:0", + "data_dir": dir, + "api_key": "k", + "mcpServers": []map[string]any{ + {"name": "poison", "command": "./evil", "protocol": "stdio", "enabled": true}, + }, + }) + require.NoError(t, err) + require.NoError(t, os.WriteFile(path, raw, 0600)) + + cfg, err := config.LoadFromFile(path) + require.NoError(t, err) + + // Disk gets the config before any gating (ApplyConfig saves first, and on + // the RequiresRestart branch the gate never runs in that process at all). + require.NoError(t, config.SaveConfig(cfg, path)) + + reloaded, err := config.LoadFromFile(path) + require.NoError(t, err) + require.False(t, reloaded.Servers[0].QuarantineExplicitlySet(), + "a save must not fabricate an operator statement") + + rt2, err := New(reloaded, path, zap.NewNop()) + require.NoError(t, err) + t.Cleanup(func() { _ = rt2.Close() }) + + require.NoError(t, rt2.LoadConfiguredServers(reloaded)) + assert.True(t, storedServer(t, rt2, "poison").Quarantined, + "a never-admitted server must be gated even after mcpproxy rewrote the config file") +} + +// P2 — a transient storage read failure must not mass-quarantine everything. +// +// ListUpstreamServers() errors were swallowed and `stored` became empty, so the +// gate saw every configured server as first-seen and walled off the user's +// entire server list — permanently, because the next boot inherits it. +func TestConfigLoadAdmissionGate_SkippedWhenStorageUnreadable(t *testing.T) { + rt, cfg := gateEnv(t, []map[string]any{ + {"name": "a", "command": "./a", "protocol": "stdio", "enabled": true}, + {"name": "b", "command": "./b", "protocol": "stdio", "enabled": true}, + }, nil) + + gated, changed := rt.applyConfigLoadAdmissionGate(cfg, nil, false) + + assert.False(t, changed, + "an unreadable config.db means 'unknown', not 'first-seen' — the gate must abstain") + for i, sc := range gated.Servers { + assert.False(t, sc.Quarantined, "server %d must not be quarantined on a storage read failure", i) + } +} + +// Control for the test above: with storage readable and empty, the same servers +// ARE first-seen and the gate fires. Proves the abstention is keyed on the +// failure, not on the empty map. +func TestConfigLoadAdmissionGate_FiresWhenStorageReadableAndEmpty(t *testing.T) { + rt, cfg := gateEnv(t, []map[string]any{ + {"name": "a", "command": "./a", "protocol": "stdio", "enabled": true}, + }, nil) + + gated, changed := rt.applyConfigLoadAdmissionGate(cfg, map[string]*config.ServerConfig{}, true) + + assert.True(t, changed) + assert.True(t, gated.Servers[0].Quarantined) +} + +// P2 — the gate must not write into a config snapshot that has already been +// published to subscribers (supervisor reconcile, serverEligibleForIndexing). +// server.go:1414 states the contract: the live snapshot is immutable and +// mutating it in place is a data race. +func TestConfigLoadAdmissionGate_DoesNotMutatePublishedSnapshot(t *testing.T) { + rt, cfg := gateEnv(t, []map[string]any{ + {"name": "handwritten", "command": "./poison", "protocol": "stdio", "enabled": true}, + }, nil) + + published := rt.Config() + require.Same(t, cfg, published) + publishedServer := published.Servers[0] + + require.NoError(t, rt.LoadConfiguredServers(cfg)) + + assert.False(t, publishedServer.Quarantined, + "the gate must not write into ServerConfig pointers already handed to subscribers") + assert.True(t, rt.Config().Servers[0].Quarantined, + "the gated decision must reach subscribers by REPLACING the snapshot, not by mutating it") + assert.NotSame(t, publishedServer, rt.Config().Servers[0], + "the gated server must be a copy") +} + +// P2 — installs already hit by #937 keep a config.db record, so the gate's +// "known means admitted" rule leaves them live. That tradeoff is deliberate +// (upgrade safety), but it must not be silent: the operator gets a named, +// actionable warning so the population the issue was filed about can be found. +func TestConfigLoadAdmissionGate_WarnsAboutServersAdmittedBeforeTheFix(t *testing.T) { + core, logs := observer.New(zap.WarnLevel) + rt, cfg, _ := gateEnvAt(t, []map[string]any{ + {"name": "pre-fix", "command": "./poison", "protocol": "stdio", "enabled": true}, + {"name": "reviewed", "command": "./ok", "protocol": "stdio", "enabled": true, "quarantined": false}, + {"name": "trusted", "command": "./ok", "protocol": "stdio", "enabled": true, "trust_mode": "auto"}, + }, nil, zap.New(core)) + + for _, name := range []string{"pre-fix", "reviewed", "trusted"} { + require.NoError(t, rt.storageManager.SaveUpstreamServer(&config.ServerConfig{ + Name: name, Command: "./x", Protocol: "stdio", Enabled: true, Quarantined: false, + })) + } + + require.NoError(t, rt.LoadConfiguredServers(cfg)) + + // Upgrade safety is unchanged — nothing gets re-quarantined. + assert.False(t, storedServer(t, rt, "pre-fix").Quarantined) + + entries := logs.FilterMessageSnippet("predate").All() + require.Len(t, entries, 1, "exactly one advisory, listing the affected servers") + assert.Equal(t, []interface{}{"pre-fix"}, entries[0].ContextMap()["servers"], + "only an unreviewed server whose trust mode would gate it is reported") +} + +// Sanity: the advisory is a diagnostic, not a behaviour change — a clean +// install with nothing pre-existing must not emit it. +func TestConfigLoadAdmissionGate_NoAdvisoryOnCleanInstall(t *testing.T) { + core, logs := observer.New(zap.WarnLevel) + rt, cfg, _ := gateEnvAt(t, []map[string]any{ + {"name": "fresh", "command": "./x", "protocol": "stdio", "enabled": true}, + }, nil, zap.New(core)) + + require.NoError(t, rt.LoadConfiguredServers(cfg)) + + assert.Empty(t, logs.FilterMessageSnippet("predate").All()) +} + +// The gate must be applied to a config BEFORE it is written to disk and before +// it is published, so the RequiresRestart branch of ApplyConfig (which returns +// without ever calling LoadConfiguredServers) cannot leave an ungated server on +// disk for the next process to trust. +func TestApplyConfig_GatesFirstSeenServerBeforeSavingToDisk(t *testing.T) { + rt, _, path := gateEnvAt(t, []map[string]any{}, nil, zap.NewNop()) + + newCfg, err := config.LoadFromFile(path) + require.NoError(t, err) + newCfg.Servers = append(newCfg.Servers, &config.ServerConfig{ + Name: "poison", Command: "./evil", Protocol: "stdio", Enabled: true, + }) + + _, err = rt.ApplyConfig(newCfg, path) + require.NoError(t, err) + + saved, err := os.ReadFile(path) + require.NoError(t, err) + var doc struct { + Servers []map[string]json.RawMessage `json:"mcpServers"` + } + require.NoError(t, json.Unmarshal(saved, &doc)) + require.Len(t, doc.Servers, 1) + assert.Equal(t, "true", string(doc.Servers[0]["quarantined"]), + "a first-seen server added through the API must be gated before it reaches disk") +} + +// A human toggling quarantine is an operator statement about that server. It +// must land in the config document, because config.db's UpstreamRecord has no +// field for the presence bit — without this the next save erased the decision +// and the server looked never-reviewed again (which would make the advisory +// above cry wolf on every legitimately reviewed server). +func TestQuarantineServer_RecordsTheDecisionInTheConfigDocument(t *testing.T) { + core, logs := observer.New(zap.WarnLevel) + rt, cfg, path := gateEnvAt(t, []map[string]any{ + {"name": "reviewed", "command": "./x", "protocol": "stdio", "enabled": true}, + }, nil, zap.New(core)) + + require.NoError(t, rt.LoadConfiguredServers(cfg)) + require.True(t, storedServer(t, rt, "reviewed").Quarantined, "gated on first sight") + + // The human reviews it and lets it out. + require.NoError(t, rt.QuarantineServer("reviewed", false)) + + saved, err := os.ReadFile(path) + require.NoError(t, err) + var doc struct { + Servers []map[string]json.RawMessage `json:"mcpServers"` + } + require.NoError(t, json.Unmarshal(saved, &doc)) + require.Len(t, doc.Servers, 1) + assert.Equal(t, "false", string(doc.Servers[0]["quarantined"]), + "the review decision must be written to the config file, not merely to config.db") + + // A reviewed server is not part of the #937 population. + assert.Empty(t, logs.FilterMessageSnippet("predate").All()) +} diff --git a/internal/runtime/config_load_admission_gate_test.go b/internal/runtime/config_load_admission_gate_test.go index 827d6e5a..0eab2591 100644 --- a/internal/runtime/config_load_admission_gate_test.go +++ b/internal/runtime/config_load_admission_gate_test.go @@ -30,6 +30,13 @@ import ( // runtime built from it plus the parsed config. func gateEnv(t *testing.T, servers []map[string]any, extra map[string]any) (*Runtime, *config.Config) { t.Helper() + rt, cfg, _ := gateEnvAt(t, servers, extra, zap.NewNop()) + return rt, cfg +} + +// gateEnvAt is gateEnv plus the config file path and an injectable logger. +func gateEnvAt(t *testing.T, servers []map[string]any, extra map[string]any, logger *zap.Logger) (*Runtime, *config.Config, string) { + t.Helper() dir := t.TempDir() p := filepath.Join(dir, "mcp_config.json") @@ -51,11 +58,11 @@ func gateEnv(t *testing.T, servers []map[string]any, extra map[string]any) (*Run cfg, err := config.LoadFromFile(p) require.NoError(t, err) - rt, err := New(cfg, p, zap.NewNop()) + rt, err := New(cfg, p, logger) require.NoError(t, err) t.Cleanup(func() { _ = rt.Close() }) - return rt, cfg + return rt, cfg, p } func storedServer(t *testing.T, rt *Runtime, name string) *config.ServerConfig { @@ -83,8 +90,8 @@ func TestConfigLoadAdmissionGate_QuarantinesFirstSeenServer(t *testing.T) { assert.True(t, storedServer(t, rt, "handwritten").Quarantined, "a first-seen config-file server under the default manual trust mode must be quarantined (issue #937)") - assert.True(t, cfg.Servers[0].Quarantined, - "the in-memory config must agree, or the supervisor will still connect it unquarantined") + assert.True(t, rt.Config().Servers[0].Quarantined, + "the PUBLISHED snapshot must agree, or the supervisor will still connect it unquarantined") } // trust_mode:auto is the documented opt-out. It must keep working, or the gate @@ -147,7 +154,7 @@ func TestConfigLoadAdmissionGate_KnownServerIsNotRequarantined(t *testing.T) { assert.False(t, storedServer(t, rt, "longstanding").Quarantined, "a server already known to config.db has been through admission and must not be re-quarantined on upgrade") - assert.False(t, cfg.Servers[0].Quarantined) + assert.False(t, rt.Config().Servers[0].Quarantined) } // DURABILITY — the gate decision has to survive a restart. @@ -170,7 +177,7 @@ func TestConfigLoadAdmissionGate_QuarantineSurvivesRestart(t *testing.T) { assert.True(t, storedServer(t, rt, "handwritten").Quarantined, "a quarantine recorded in config.db must not be reverted by a config file that never mentions the key") - assert.True(t, cfg.Servers[0].Quarantined) + assert.True(t, rt.Config().Servers[0].Quarantined) } // The issue's comparison table: the same server, same binary, same diff --git a/internal/runtime/configsvc/service.go b/internal/runtime/configsvc/service.go index 0a189a26..da1b6378 100644 --- a/internal/runtime/configsvc/service.go +++ b/internal/runtime/configsvc/service.go @@ -47,6 +47,13 @@ type Service struct { version int64 subscribers []chan Update subMu sync.RWMutex + + // prePublish runs on an incoming config while it is still exclusively owned + // by the updater — before the snapshot is stored and therefore before any + // subscriber can observe it. It returns the config to publish. Used by the + // #937 admission gate so a poisoned server can never be reconciled and + // indexed in the window between "config parsed" and "config gated". + prePublish atomic.Value // func(*config.Config) *config.Config } // NewService creates a new configuration service with the given initial config. @@ -84,12 +91,45 @@ func (s *Service) Current() *Snapshot { return s.snapshot.Load().(*Snapshot) } +// SetPrePublishHook installs a function invoked on every configuration on its +// way into the snapshot, before subscribers are notified. The hook receives a +// config nothing else can observe yet and returns the config to publish +// (itself, or a modified copy — it must not mutate the one it is given, which +// may still be the currently published snapshot). +// +// Passing nil removes the hook. Safe to call at any time. +func (s *Service) SetPrePublishHook(hook func(*config.Config) *config.Config) { + if hook == nil { + s.prePublish.Store((func(*config.Config) *config.Config)(nil)) + return + } + s.prePublish.Store(hook) +} + +// runPrePublishHook applies the installed hook, if any. +func (s *Service) runPrePublishHook(cfg *config.Config) *config.Config { + v := s.prePublish.Load() + if v == nil { + return cfg + } + hook, _ := v.(func(*config.Config) *config.Config) + if hook == nil { + return cfg + } + if gated := hook(cfg); gated != nil { + return gated + } + return cfg +} + // Update atomically updates the configuration and notifies all subscribers. // This operation is serialized to ensure consistency. func (s *Service) Update(newConfig *config.Config, updateType UpdateType, source string) error { s.updateMu.Lock() defer s.updateMu.Unlock() + newConfig = s.runPrePublishHook(newConfig) + current := s.Current() s.version++ diff --git a/internal/runtime/lifecycle.go b/internal/runtime/lifecycle.go index 5e42b329..ac5680f8 100644 --- a/internal/runtime/lifecycle.go +++ b/internal/runtime/lifecycle.go @@ -77,6 +77,15 @@ func (r *Runtime) StartBackgroundInitialization() { r.logger.Info("Token saved callback registered for proactive refresh") } + // Issue #937 (review): admission must be settled BEFORE the supervisor can + // reconcile. installAdmissionGateHook gates every future publication inside + // configsvc; gateInitialConfig covers the one snapshot that never goes + // through Update — the one NewService was constructed with. Both run while + // the supervisor is still stopped, so the startup gate cannot race a + // reconcile that would connect an ungated server and index its tools. + r.installAdmissionGateHook() + r.gateInitialConfig() + // Phase 6: Start Supervisor for state reconciliation and lock-free reads if r.supervisor != nil { r.supervisor.Start() @@ -925,6 +934,11 @@ func (r *Runtime) LoadConfiguredServers(cfg *config.Config) error { currentUpstreams := r.upstreamManager.GetAllServerNames() storedServers, err := r.storageManager.ListUpstreamServers() + // A failed read is NOT an empty database. Flattening it into one made the + // admission gate below see every configured server as first-seen and + // quarantine the lot (review P2); the rest of the sync already tolerates an + // empty stored view, so only the gate needs to know the difference. + storageReadable := err == nil if err != nil { r.logger.Error("Failed to get stored servers for sync", zap.Error(err)) storedServers = []*config.ServerConfig{} @@ -934,10 +948,6 @@ func (r *Runtime) LoadConfiguredServers(cfg *config.Config) error { storedServerMap := make(map[string]*config.ServerConfig) var changed bool - for _, serverCfg := range cfg.Servers { - configuredServers[serverCfg.Name] = serverCfg - } - for _, storedServer := range storedServers { storedServerMap[storedServer.Name] = storedServer } @@ -945,7 +955,22 @@ func (r *Runtime) LoadConfiguredServers(cfg *config.Config) error { // Issue #937: apply the trust-mode admission gate to servers that arrived by // config edit. Runs BEFORE the storage save loop below so the gated value is // what gets persisted, connected, and reported. - r.applyConfigLoadAdmissionGate(cfg, storedServerMap) + // + // The gate returns a COPY rather than writing through cfg.Servers: those + // pointers are the ones configsvc published, and the supervisor reads them + // concurrently. publishAdmissionGatedConfig swaps the whole config in so the + // decision reaches subscribers atomically. Normally this is already a no-op + // because configsvc's pre-publish hook gated the config on its way in; it + // still fires when storage changed after publication (e.g. a restart + // inheriting a recorded quarantine). + if gated, gateChanged := r.applyConfigLoadAdmissionGate(cfg, storedServerMap, storageReadable); gateChanged { + r.publishAdmissionGatedConfig(cfg, gated) + cfg = gated + } + + for _, serverCfg := range cfg.Servers { + configuredServers[serverCfg.Name] = serverCfg + } // GC orphaned tool-approval records (MCP-1002): drop approvals whose server // is no longer configured. Configured-but-disabled servers are preserved so @@ -1143,6 +1168,24 @@ func (r *Runtime) SaveConfiguration() error { // Update servers with latest from storage configCopy.Servers = latestServers + // config.db's UpstreamRecord has no field for the "a quarantine value was + // stated" bit (issue #937), so servers that came back from storage look + // un-stated again. Carry the bit across the round-trip from the snapshot, + // or an operator's explicit `"quarantined": false` — and the decision the + // user made in the quarantine UI, which QuarantineServer stamps — would be + // erased from the config file by the next save. + stated := make(map[string]bool, len(configCopy.Servers)) + for _, sc := range snapshot.Config.Servers { + if sc != nil && sc.QuarantineExplicitlySet() { + stated[sc.Name] = true + } + } + for _, sc := range latestServers { + if sc != nil && stated[sc.Name] { + sc.MarkQuarantineExplicitlySet(true) + } + } + r.logger.Debug("Saving configuration to disk", zap.Int("server_count", len(latestServers)), zap.String("config_path", snapshot.Path), @@ -1421,6 +1464,13 @@ func (r *Runtime) QuarantineServer(serverName string, quarantined bool) error { } } + // A human toggling quarantine IS a statement about this server (issue #937). + // Record it in the config document so the admission gate — and the + // "predates the gate" advisory — can tell a reviewed server from one that + // merely happens to have a config.db row. Must happen before the save below, + // which is what writes the file. + r.markQuarantineDecisionExplicit(serverName) + // Save configuration synchronously to ensure changes are persisted before returning if err := r.SaveConfiguration(); err != nil { r.logger.Error("Failed to save configuration after quarantine state change", zap.Error(err)) diff --git a/internal/runtime/runtime.go b/internal/runtime/runtime.go index 3d94480e..a34f55b9 100644 --- a/internal/runtime/runtime.go +++ b/internal/runtime/runtime.go @@ -1427,6 +1427,15 @@ func (r *Runtime) ApplyConfig(newCfg *config.Config, cfgPath string) (*ConfigApp // Idempotent + nil-safe. config.MigrateDeepScanConfig(newCfg) + // Issue #937 (review P1): gate BEFORE the diff and BEFORE the disk write + // below. ApplyConfig saves first and only reaches LoadConfiguredServers at + // the very end — and not at all when result.RequiresRestart — so a + // `PUT /api/v1/config` that adds a first-seen server used to put it on disk + // unquarantined and, on the restart branch, never gate it in this process at + // all. Gating here also means the diff reports the value that is actually + // persisted. The gate returns a copy, so the caller's config is untouched. + newCfg = r.gateConfigForAdmission(newCfg) + // Detect changes and determine if restart is required result := DetectConfigChanges(r.cfg, newCfg) if !result.Success { diff --git a/internal/server/code_execution_result_status_test.go b/internal/server/code_execution_result_status_test.go new file mode 100644 index 00000000..651e4e1e --- /dev/null +++ b/internal/server/code_execution_result_status_test.go @@ -0,0 +1,105 @@ +package server + +import ( + "testing" + "time" + + "github.com/mark3labs/mcp-go/mcp" + "github.com/stretchr/testify/assert" + "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/storage" +) + +// Review finding — `code_execution` is a FOURTH upstream dispatch path, and it +// was still classifying on `err == nil` alone. +// +// A tool invoked from the JS sandbox goes through upstreamToolCaller.CallTool, +// which recorded `success: err == nil` and only ever set record.Error from a Go +// error. An upstream answering `CallToolResult{IsError:true}` — the most common +// real failure — was therefore stored in tool-call history as a clean success, +// while the identical call through call_tool_read now records the upstream's +// own message (issue #935, mcp.go). + +func isErrorResult(text string) *mcp.CallToolResult { + return mcp.NewToolResultError(text) +} + +// The in-memory record is what the code_execution response reports back as the +// list of tool calls the script made. +func TestUpstreamToolCaller_RecordsIsErrorResultAsFailure(t *testing.T) { + u := &upstreamToolCaller{logger: zap.NewNop()} + + start := time.Now() + u.recordUpstreamCall("time", "get_current_time", start, time.Millisecond, + isErrorResult("Invalid timezone: 'Mars/Olympus'"), nil) + u.recordUpstreamCall("time", "get_current_time", start, time.Millisecond, + mcp.NewToolResultText("2026-01-01T00:00:00Z"), nil) + + calls := u.getToolCalls() + require.Len(t, calls, 2) + + assert.False(t, calls[0].Success, + "an upstream that answered isError:true did not succeed, even though the transport hop did") + assert.Contains(t, calls[0].Error, "Mars/Olympus", + "the recorded error must carry the upstream's own explanation") + + assert.True(t, calls[1].Success, "a normal answer must still be a success") + assert.Empty(t, calls[1].Error) +} + +// A transport/dispatch error still wins: its message is the useful one. +func TestUpstreamToolCaller_TransportErrorStillRecorded(t *testing.T) { + u := &upstreamToolCaller{logger: zap.NewNop()} + + u.recordUpstreamCall("time", "get_current_time", time.Now(), time.Millisecond, + nil, assert.AnError) + + calls := u.getToolCalls() + require.Len(t, calls, 1) + assert.False(t, calls[0].Success) + assert.Equal(t, assert.AnError.Error(), calls[0].Error) +} + +// The persisted history row is what the activity/usage consumers read. Before +// the fix it had an empty Error for an isError answer, so a failed nested call +// was indistinguishable from a successful one. +func TestUpstreamToolCaller_StoresUpstreamErrorInHistory(t *testing.T) { + dir := t.TempDir() + store, err := storage.NewManager(dir, zap.NewNop().Sugar()) + require.NoError(t, err) + t.Cleanup(func() { _ = store.Close() }) + + serverCfg := &config.ServerConfig{ + Name: "time", URL: "http://localhost:1/mcp", Protocol: "streamable-http", Enabled: true, + } + require.NoError(t, store.SaveUpstreamServer(serverCfg)) + + u := &upstreamToolCaller{ + logger: zap.NewNop(), + storage: store, + executionID: "exec-1", + } + + start := time.Now() + u.storeToolCallInHistory("time", "get_current_time", + map[string]interface{}{"timezone": "Mars/Olympus"}, + isErrorResult("Invalid timezone: 'Mars/Olympus'"), nil, start, time.Millisecond) + u.storeToolCallInHistory("time", "get_current_time", + map[string]interface{}{"timezone": "UTC"}, + mcp.NewToolResultText("ok"), nil, start.Add(time.Second), time.Millisecond) + + records, err := store.GetServerToolCalls(storage.GenerateServerID(serverCfg), 10) + require.NoError(t, err) + require.Len(t, records, 2) + + byErr := map[bool]*storage.ToolCallRecord{} + for _, rec := range records { + byErr[rec.Error != ""] = rec + } + require.Contains(t, byErr, true, "the isError call must persist an error message") + assert.Contains(t, byErr[true].Error, "Mars/Olympus") + require.Contains(t, byErr, false, "the successful call must stay clean") +} diff --git a/internal/server/mcp_code_execution.go b/internal/server/mcp_code_execution.go index e3ff025b..eaf480e4 100644 --- a/internal/server/mcp_code_execution.go +++ b/internal/server/mcp_code_execution.go @@ -360,7 +360,15 @@ func (p *MCPProxyServer) handleCodeExecution(ctx context.Context, request mcp.Ca return nil, fmt.Errorf("failed to serialize result: %w", err) } - // Spec 024: Emit internal tool call event for code_execution + // Spec 024: Emit internal tool call event for code_execution. + // + // This is the WRAPPER's own outcome and stays keyed on the JS runtime's + // result: a script that called a tool, got an isError answer and handled it + // has succeeded. The nested dispatches carry their own classification — + // upstreamToolCaller.CallTool records an isError:true answer as a failed + // tool call with the upstream's message (issue #935) — so a failure inside + // the sandbox is visible in the tool-call history rather than being folded + // into the wrapper. var status, errorMsg string if result.Ok { status = "success" @@ -447,7 +455,7 @@ func (u *upstreamToolCaller) CallTool(ctx context.Context, serverName, toolName if !exists { err := fmt.Errorf("server not found: %s", serverName) duration := time.Since(startTime) - u.recordToolCall(serverName, toolName, startTime, duration, false, err) + u.recordToolCall(serverName, toolName, startTime, duration, false, err.Error()) u.storeToolCallInHistory(serverName, toolName, args, nil, err, startTime, duration) return nil, err } @@ -456,8 +464,11 @@ func (u *upstreamToolCaller) CallTool(ctx context.Context, serverName, toolName result, err := client.CallTool(ctx, toolName, args) duration := time.Since(startTime) - // Record the tool call with timing and result - u.recordToolCall(serverName, toolName, startTime, duration, err == nil, err) + // Record the tool call with timing and result. Issue #935: code_execution + // is a fourth upstream dispatch path, so it classifies an isError:true + // answer as a failure exactly like call_tool_* does — otherwise the same + // upstream rejection is a clean success here and an error there. + u.recordUpstreamCall(serverName, toolName, startTime, duration, result, err) u.storeToolCallInHistory(serverName, toolName, args, result, err, startTime, duration) u.logger.Debug("upstream tool call completed", @@ -465,7 +476,7 @@ func (u *upstreamToolCaller) CallTool(ctx context.Context, serverName, toolName zap.String("server", serverName), zap.String("tool", toolName), zap.Duration("duration", duration), - zap.Bool("success", err == nil), + zap.Bool("success", err == nil && !upstreamAnsweredWithError(result)), ) if err != nil { @@ -475,24 +486,48 @@ func (u *upstreamToolCaller) CallTool(ctx context.Context, serverName, toolName return result, nil } +// upstreamAnsweredWithError reports whether a dispatched result is an MCP +// answer flagged isError:true — a failure the upstream reported over a +// successful transport hop (issue #935). +func upstreamAnsweredWithError(result interface{}) bool { + status, _ := activityStatusForResult(result) + return status != "success" +} + +// nestedCallFailure classifies one nested dispatch from the JS sandbox, +// returning (success, message). A Go error wins over the upstream's own text: +// when the call never completed, that is the useful explanation. +func nestedCallFailure(result interface{}, err error) (bool, string) { + if err != nil { + return false, err.Error() + } + status, msg := activityStatusForResult(result) + if status == "success" { + return true, "" + } + return false, msg +} + +// recordUpstreamCall records a nested tool call with timing and its classified +// outcome (thread-safe). +func (u *upstreamToolCaller) recordUpstreamCall(serverName, toolName string, startTime time.Time, duration time.Duration, result interface{}, err error) { + success, errMsg := nestedCallFailure(result, err) + u.recordToolCall(serverName, toolName, startTime, duration, success, errMsg) +} + // recordToolCall records a tool call with timing and result information (thread-safe) -func (u *upstreamToolCaller) recordToolCall(serverName, toolName string, startTime time.Time, duration time.Duration, success bool, err error) { +func (u *upstreamToolCaller) recordToolCall(serverName, toolName string, startTime time.Time, duration time.Duration, success bool, errMsg string) { u.mu.Lock() defer u.mu.Unlock() - record := toolCallRecord{ + u.toolCalls = append(u.toolCalls, toolCallRecord{ ServerName: serverName, ToolName: toolName, StartTime: startTime, Duration: duration, Success: success, - } - - if err != nil { - record.Error = err.Error() - } - - u.toolCalls = append(u.toolCalls, record) + Error: errMsg, + }) } // getToolCalls returns all recorded tool calls (thread-safe) @@ -596,8 +631,11 @@ func (u *upstreamToolCaller) storeToolCallInHistory(serverName, toolName string, Metrics: tokenMetrics, } - if callErr != nil { - record.Error = callErr.Error() + // Issue #935: an upstream that answered isError:true failed, even though the + // transport hop did not. Without this the nested history row was clean while + // the identical call through call_tool_read recorded the upstream's message. + if _, errMsg := nestedCallFailure(result, callErr); errMsg != "" { + record.Error = errMsg } // Store in database From c64f128f684f28676a720c7346195907ce0e3ec6 Mon Sep 17 00:00:00 2001 From: Algis Dumbris Date: Sat, 1 Aug 2026 09:51:30 +0300 Subject: [PATCH 3/3] fix(serveredition): stop ServerConfig JSON methods hijacking ServerResponse MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit config.ServerConfig gained custom MarshalJSON/UnmarshalJSON in the #937 config-load admission gate work. Go promotes those methods to any struct that embeds it, and api.ServerResponse embeds *config.ServerConfig, so encoding/json treated the whole wrapper as a Marshaler/Unmarshaler and delegated to the embedded config: - decode failed with "json: Unmarshal(nil *config.Alias)" (the embedded pointer is nil before decoding starts), turning every test in internal/serveredition/api red; - encode silently DROPPED "ownership" and "user_enabled" from every server-edition API response, and panicked outright on a nil embedded config. No test covered that, so it would have shipped as a silent API regression. Give ServerResponse explicit JSON methods that marshal the embedded config through its own MarshalJSON — preserving the #937 quarantined presence semantics exactly — and splice the wrapper fields on top. ServerResponse is the only type in the repo that embeds ServerConfig; document the promotion hazard on the config methods so the next one does not repeat it. Related #937 --- internal/config/config.go | 9 + .../api/server_response_json_test.go | 157 ++++++++++++++++++ internal/serveredition/api/user_handlers.go | 80 +++++++++ 3 files changed, 246 insertions(+) create mode 100644 internal/serveredition/api/server_response_json_test.go diff --git a/internal/config/config.go b/internal/config/config.go index edcc1f25..bd55883a 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -1657,6 +1657,15 @@ func (c *Config) DefaultQuarantineForNewServer() bool { // Note this only fires for JSON decoding. Structs built in Go (REST handlers, // the add paths, tests) leave the bit false — i.e. "unstated" — which is the // safe default: the gate can only ever ADD quarantine, never remove it. +// +// WARNING — do not EMBED ServerConfig (by pointer or value) in another struct +// that adds JSON fields of its own. Go promotes these methods to the outer +// type, so encoding/json treats the wrapper as a Marshaler/Unmarshaler and +// delegates the whole value here: the wrapper's own fields are silently dropped +// on encode, and decode fails with "json: Unmarshal(nil *config.Alias)" while +// the embedded pointer is still nil. A wrapper that needs to embed this must +// declare its own MarshalJSON/UnmarshalJSON — see +// internal/serveredition/api.ServerResponse. func (sc *ServerConfig) UnmarshalJSON(data []byte) error { type Alias ServerConfig if err := json.Unmarshal(data, (*Alias)(sc)); err != nil { diff --git a/internal/serveredition/api/server_response_json_test.go b/internal/serveredition/api/server_response_json_test.go new file mode 100644 index 00000000..b9770f5f --- /dev/null +++ b/internal/serveredition/api/server_response_json_test.go @@ -0,0 +1,157 @@ +//go:build server + +package api + +import ( + "encoding/json" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/smart-mcp-proxy/mcpproxy-go/internal/config" +) + +// Regression for the #937 embedding landmine. +// +// config.ServerConfig grew custom MarshalJSON/UnmarshalJSON to record whether a +// parsed document carried a "quarantined" key. Go promotes those methods to any +// struct that EMBEDS it, so ServerResponse — which embeds *config.ServerConfig +// and adds `ownership` / `user_enabled` — was handed wholesale to the embedded +// config by encoding/json: +// +// - encode silently dropped `ownership` and `user_enabled` from every +// server-edition API response (a silent API regression, no test caught it); +// - decode failed with "json: Unmarshal(nil *config.Alias)" because the +// embedded pointer is nil before decoding starts (this one turned every test +// in this package red). +// +// This test fails if the promotion ever comes back — e.g. if ServerResponse's +// explicit JSON methods are removed, or if new wrapper fields are added to the +// struct without being added to the methods. +func TestServerResponse_JSONRoundTrip(t *testing.T) { + userEnabled := true + resp := &ServerResponse{ + ServerConfig: &config.ServerConfig{ + Name: "shared-srv", + URL: "https://example.test/mcp", + Protocol: "http", + Enabled: true, + Shared: true, + }, + Ownership: "shared", + UserEnabled: &userEnabled, + } + + encoded, err := json.Marshal(resp) + require.NoError(t, err) + + // The wire form must be FLAT and must carry all three groups of fields. + var wire map[string]json.RawMessage + require.NoError(t, json.Unmarshal(encoded, &wire)) + assert.Contains(t, wire, "name", "embedded server fields must survive") + assert.Contains(t, wire, "url") + assert.Contains(t, wire, "enabled") + assert.Contains(t, wire, "ownership", "wrapper field must not be dropped by method promotion") + assert.Contains(t, wire, "user_enabled", "wrapper field must not be dropped by method promotion") + + // And it must decode back into a fully populated value. + var decoded ServerResponse + require.NoError(t, json.Unmarshal(encoded, &decoded)) + require.NotNil(t, decoded.ServerConfig, "embedded config must be allocated by UnmarshalJSON") + assert.Equal(t, "shared-srv", decoded.Name) + assert.Equal(t, "https://example.test/mcp", decoded.URL) + assert.Equal(t, "http", decoded.Protocol) + assert.True(t, decoded.Enabled) + assert.True(t, decoded.Shared) + assert.Equal(t, "shared", decoded.Ownership) + require.NotNil(t, decoded.UserEnabled) + assert.True(t, *decoded.UserEnabled) +} + +// A nil embedded config must not panic or lose the wrapper fields, and +// `user_enabled` must stay omitted when there is no per-user preference. +func TestServerResponse_MarshalJSON_NilConfigAndOmittedPreference(t *testing.T) { + encoded, err := json.Marshal(&ServerResponse{Ownership: "personal"}) + require.NoError(t, err) + + var wire map[string]json.RawMessage + require.NoError(t, json.Unmarshal(encoded, &wire)) + assert.Equal(t, `"personal"`, string(wire["ownership"])) + assert.NotContains(t, wire, "user_enabled", "omitempty must still apply") +} + +// The #937 presence semantics of the embedded config must survive the wrapper: +// an unstated `quarantined` stays absent, an explicit one is written through. +func TestServerResponse_PreservesQuarantinePresenceSemantics(t *testing.T) { + t.Run("unstated quarantined:false is omitted", func(t *testing.T) { + encoded, err := json.Marshal(&ServerResponse{ + ServerConfig: &config.ServerConfig{Name: "s", Command: "c"}, + Ownership: "personal", + }) + require.NoError(t, err) + + var wire map[string]json.RawMessage + require.NoError(t, json.Unmarshal(encoded, &wire)) + assert.NotContains(t, wire, "quarantined", + "the wrapper must not fabricate an operator statement") + }) + + t.Run("quarantined:true is always written", func(t *testing.T) { + encoded, err := json.Marshal(&ServerResponse{ + ServerConfig: &config.ServerConfig{Name: "s", Command: "c", Quarantined: true}, + Ownership: "personal", + }) + require.NoError(t, err) + + var wire map[string]json.RawMessage + require.NoError(t, json.Unmarshal(encoded, &wire)) + assert.Equal(t, "true", string(wire["quarantined"])) + }) + + t.Run("decode records the presence bit on the embedded config", func(t *testing.T) { + var stated ServerResponse + require.NoError(t, json.Unmarshal( + []byte(`{"name":"s","command":"c","quarantined":false,"ownership":"personal"}`), &stated)) + require.NotNil(t, stated.ServerConfig) + assert.True(t, stated.QuarantineExplicitlySet()) + + var unstated ServerResponse + require.NoError(t, json.Unmarshal( + []byte(`{"name":"s","command":"c","ownership":"personal"}`), &unstated)) + require.NotNil(t, unstated.ServerConfig) + assert.False(t, unstated.QuarantineExplicitlySet()) + }) +} + +// Lists of responses (the actual endpoint payload) must round-trip too — this is +// the shape that was returning ownership-less objects to the Web UI. +func TestServerListResponse_RoundTrip(t *testing.T) { + userEnabled := false + payload := ServerListResponse{ + Personal: []*ServerResponse{{ + ServerConfig: &config.ServerConfig{Name: "p1", Command: "c", Enabled: true}, + Ownership: "personal", + }}, + Shared: []*ServerResponse{{ + ServerConfig: &config.ServerConfig{Name: "s1", URL: "https://x.test", Shared: true}, + Ownership: "shared", + UserEnabled: &userEnabled, + }}, + } + + encoded, err := json.Marshal(payload) + require.NoError(t, err) + + var decoded ServerListResponse + require.NoError(t, json.Unmarshal(encoded, &decoded)) + require.Len(t, decoded.Personal, 1) + require.Len(t, decoded.Shared, 1) + assert.Equal(t, "p1", decoded.Personal[0].Name) + assert.Equal(t, "personal", decoded.Personal[0].Ownership) + assert.Nil(t, decoded.Personal[0].UserEnabled) + assert.Equal(t, "s1", decoded.Shared[0].Name) + assert.Equal(t, "shared", decoded.Shared[0].Ownership) + require.NotNil(t, decoded.Shared[0].UserEnabled) + assert.False(t, *decoded.Shared[0].UserEnabled) +} diff --git a/internal/serveredition/api/user_handlers.go b/internal/serveredition/api/user_handlers.go index 39949460..ae3e2db0 100644 --- a/internal/serveredition/api/user_handlers.go +++ b/internal/serveredition/api/user_handlers.go @@ -3,6 +3,7 @@ package api import ( + "bytes" "encoding/json" "fmt" "net/http" @@ -110,12 +111,91 @@ type EnableServerRequest struct { } // ServerResponse wraps a ServerConfig with ownership information. +// +// NOTE (issue #937 fallout): config.ServerConfig carries custom +// MarshalJSON/UnmarshalJSON methods, and Go PROMOTES those to any struct that +// embeds it. Without the explicit methods below, encoding/json saw +// ServerResponse as a json.Marshaler/Unmarshaler and delegated the whole value +// to the embedded config — silently dropping `ownership` and `user_enabled` +// from every response, and failing every decode with +// "json: Unmarshal(nil *config.Alias)" because the embedded pointer is nil +// before decoding starts. Keep the methods in sync with the fields here; +// TestServerResponse_JSONRoundTrip pins the behaviour. type ServerResponse struct { *config.ServerConfig Ownership string `json:"ownership"` // "personal" or "shared" UserEnabled *bool `json:"user_enabled,omitempty"` // Per-user preference for shared servers (nil = no preference, defaults to enabled) } +// serverResponseWrapperFields holds only the fields ServerResponse adds on top +// of the embedded config, so both JSON methods share one definition. +type serverResponseWrapperFields struct { + Ownership string `json:"ownership"` + UserEnabled *bool `json:"user_enabled,omitempty"` +} + +// MarshalJSON flattens the embedded *config.ServerConfig and the wrapper fields +// into a single JSON object. +// +// The embedded config is marshaled through its OWN MarshalJSON so the #937 +// "quarantined" presence semantics (omit an unstated false, always write true) +// are preserved verbatim; the wrapper fields are then spliced on top. +func (r ServerResponse) MarshalJSON() ([]byte, error) { + fields := map[string]json.RawMessage{} + + if r.ServerConfig != nil { + raw, err := json.Marshal(r.ServerConfig) + if err != nil { + return nil, err + } + if err := json.Unmarshal(raw, &fields); err != nil { + return nil, err + } + } + + wrapper, err := json.Marshal(serverResponseWrapperFields{ + Ownership: r.Ownership, + UserEnabled: r.UserEnabled, + }) + if err != nil { + return nil, err + } + var wrapperFields map[string]json.RawMessage + if err := json.Unmarshal(wrapper, &wrapperFields); err != nil { + return nil, err + } + // Wrapper fields win: they are what the endpoint promises. + for k, v := range wrapperFields { + fields[k] = v + } + + return json.Marshal(fields) +} + +// UnmarshalJSON decodes a flattened ServerResponse, allocating the embedded +// config first so the config's own UnmarshalJSON (and its "quarantined" +// presence detection) runs against a non-nil target. +func (r *ServerResponse) UnmarshalJSON(data []byte) error { + if string(bytes.TrimSpace(data)) == "null" { + return nil + } + + sc := &config.ServerConfig{} + if err := json.Unmarshal(data, sc); err != nil { + return err + } + + var wrapper serverResponseWrapperFields + if err := json.Unmarshal(data, &wrapper); err != nil { + return err + } + + r.ServerConfig = sc + r.Ownership = wrapper.Ownership + r.UserEnabled = wrapper.UserEnabled + return nil +} + // ServerListResponse contains personal and shared servers for a user. type ServerListResponse struct { Personal []*ServerResponse `json:"personal"`