diff --git a/docs/features/security-quarantine.md b/docs/features/security-quarantine.md index c28f1343e..a08d8d626 100644 --- a/docs/features/security-quarantine.md +++ b/docs/features/security-quarantine.md @@ -30,6 +30,66 @@ 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. + +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:** @@ -213,6 +273,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 997d211dc..bd55883a7 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" @@ -467,6 +468,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 +1640,102 @@ 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: 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 +// 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 { + 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 + } + 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 +// 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 a6999edd5..9115c5ff3 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 000000000..9734fa427 --- /dev/null +++ b/internal/config/quarantine_presence_test.go @@ -0,0 +1,175 @@ +package config + +import ( + "encoding/json" + "os" + "path/filepath" + "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, + }, + { + // 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: false, + 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") +} + +// 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 new file mode 100644 index 000000000..c5634594c --- /dev/null +++ b/internal/runtime/config_load_admission_gate.go @@ -0,0 +1,357 @@ +package runtime + +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. +// +// 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 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 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 +// 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 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, 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 +// +// 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. + +// 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 cfg, false + } + + 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 + } + + 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.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 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 + 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 000000000..91d42baec --- /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 new file mode 100644 index 000000000..0eab25915 --- /dev/null +++ b/internal/runtime/config_load_admission_gate_test.go @@ -0,0 +1,228 @@ +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() + 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") + + 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, logger) + require.NoError(t, err) + t.Cleanup(func() { _ = rt.Close() }) + + return rt, cfg, p +} + +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, 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 +// 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, rt.Config().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, rt.Config().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/configsvc/service.go b/internal/runtime/configsvc/service.go index 0a189a26a..da1b63788 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 46cdcaf3c..ac5680f8b 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,14 +948,30 @@ 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 } + // 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. + // + // 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 // a later re-enable doesn't re-quarantine their previously-approved tools. @@ -1138,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), @@ -1416,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 3d94480ef..a34f55b92 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/activity_result_status.go b/internal/server/activity_result_status.go new file mode 100644 index 000000000..6743487e7 --- /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 000000000..5614070e4 --- /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/code_execution_result_status_test.go b/internal/server/code_execution_result_status_test.go new file mode 100644 index 000000000..651e4e1ed --- /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/e2e_iserror_activity_test.go b/internal/server/e2e_iserror_activity_test.go new file mode 100644 index 000000000..db3417a47 --- /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 efb99a20c..3ec241170 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_code_execution.go b/internal/server/mcp_code_execution.go index e3ff025be..eaf480e44 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 diff --git a/internal/server/mcp_routing.go b/internal/server/mcp_routing.go index cb8e1be0a..4031980e2 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/serveredition/api/server_response_json_test.go b/internal/serveredition/api/server_response_json_test.go new file mode 100644 index 000000000..b9770f5f6 --- /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 399494601..ae3e2db04 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"` diff --git a/internal/storage/async_ops_test.go b/internal/storage/async_ops_test.go index 0d5e52b79..807b441c9 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) }