fix: classify upstream isError results as activity errors and gate config-load admission (#935, #937)

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

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

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
 }
 

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"]))
+}