fix(security): keep implicit quarantine across server restarts and config writes (Spec fix-quarantine-restart)

docs/features/security-quarantine.md

@@ -90,6 +90,18 @@ For each server named, either:
 
 Adding the key by hand is enough — the gate obeys an explicit value either way.
 
+**Restarts also cleared the quarantine.** In the same affected releases,
+restarting a server that the gate had quarantined silently cleared its
+quarantine: a restart from the REST API, the CLI, the tray or an MCP client,
+"restart all", a secret change that restarts the servers using it, and a security
+scan (including the baseline scan that runs shortly after startup) all re-read the
+server from `mcp_config.json` and wrote the un-gated entry over the recorded
+quarantine. Such a server shows up in the same "predate the config-load admission
+gate" warning and should be reviewed the same way. From this fix on, a restart
+runs the file entry through the admission gate, and `config.db` refuses to lower a
+recorded quarantine unless the operator states `"quarantined": false` or the
+server is released from the quarantine review.
+
 ### Tool Discovery and Search Isolation
 
 **Quarantined servers are completely isolated from the tool discovery and search system:**

internal/httpapi/patch_server_test.go

@@ -1154,3 +1154,47 @@ func TestHandlePatchServer_IsolationPreservesUnexposedFields(t *testing.T) {
 	assert.Equal(t, sandbox, *iso.Mode)
 	assert.Equal(t, "local", iso.LogDriver)
 }
+
+// TestHandlePatchServer_QuarantinedFieldMarksExplicit pins that only a PATCH body
+// that actually carries `quarantined` is an operator decision. Without the
+// explicit bit, UpdateServer must not apply (or storage lower) the quarantine.
+func TestHandlePatchServer_QuarantinedFieldMarksExplicit(t *testing.T) {
+	patch := func(t *testing.T, existingQuarantined bool, body map[string]any) *config.ServerConfig {
+		t.Helper()
+		mockCtrl := &mockPatchServerController{
+			apiKey: "test-key",
+			existingServer: &config.ServerConfig{
+				Name:        "github",
+				Protocol:    "stdio",
+				Enabled:     true,
+				Quarantined: existingQuarantined,
+			},
+		}
+		srv := NewServer(mockCtrl, zap.NewNop().Sugar(), nil)
+		raw, _ := json.Marshal(body)
+		req := httptest.NewRequest(http.MethodPatch, "/api/v1/servers/github", bytes.NewReader(raw))
+		req.Header.Set("Content-Type", "application/json")
+		req.Header.Set("X-API-Key", "test-key")
+		w := httptest.NewRecorder()
+		srv.ServeHTTP(w, req)
+		require.Equal(t, http.StatusOK, w.Code, "body=%s", w.Body.String())
+		require.NotNil(t, mockCtrl.capturedUpdates)
+		return mockCtrl.capturedUpdates
+	}
+
+	t.Run("explicit false", func(t *testing.T) {
+		got := patch(t, true, map[string]any{"quarantined": false})
+		assert.True(t, got.QuarantineExplicitlySet())
+		assert.False(t, got.Quarantined)
+	})
+	t.Run("explicit true", func(t *testing.T) {
+		got := patch(t, false, map[string]any{"quarantined": true})
+		assert.True(t, got.QuarantineExplicitlySet())
+		assert.True(t, got.Quarantined)
+	})
+	t.Run("omitted", func(t *testing.T) {
+		got := patch(t, true, map[string]any{"args": []string{"x"}})
+		assert.False(t, got.QuarantineExplicitlySet(), "an unrelated PATCH must not carry an operator decision")
+		assert.True(t, got.Quarantined, "existing value is preserved")
+	})
+}

internal/httpapi/server.go

@@ -2815,6 +2815,9 @@ func (s *Server) handlePatchServer(w http.ResponseWriter, r *http.Request) {
 	}
 	if req.Quarantined != nil {
 		updates.Quarantined = *req.Quarantined
+		// Only a body that carries the field is an operator decision; UpdateServer
+		// and the storage guard lower a recorded quarantine only for this case.
+		updates.MarkQuarantineExplicitlySet(true)
 		hasUpdates = true
 	} else if existingSrv != nil {
 		updates.Quarantined = existingSrv.Quarantined

internal/runtime/config_load_admission_gate.go

@@ -250,6 +250,68 @@ func (r *Runtime) storedServersForAdmission() (map[string]*config.ServerConfig,
 	return byName, true
 }
 
+// gateServerForRestart runs one server entry that was just re-read from
+// mcp_config.json (the #467 disk-first restart) through the admission gate and
+// reports whether the result may be persisted to config.db.
+//
+// Why: the raw file entry of a server that never stated `quarantined` decodes as
+// Quarantined=false. Persisting or using it as-is silently erased the quarantine
+// the gate recorded at load, and every later config write, reload or reboot then
+// saw a known, live, un-stated server and admitted it. Only the restarted server
+// is gated (not the whole file) so other servers do not re-log the "predate the
+// gate" warning or emit duplicate first-seen activity on every restart.
+//
+// Like the gate itself this can only ever ADD quarantine.
+func (r *Runtime) gateServerForRestart(diskCfg *config.Config, srv *config.ServerConfig) (*config.ServerConfig, bool) {
+	stored, ok := r.storedServersForAdmission()
+	return r.admitServerForRestart(diskCfg, srv, stored, ok)
+}
+
+// admitServerForRestart is gateServerForRestart with the storage view injected.
+//
+// When storage is readable the disk entry goes through applyConfigLoadAdmissionGate
+// as a one-server config. When it is not, the gate would abstain, but a restart
+// must not then trust the raw file (fail closed): a server that states nothing
+// inherits the quarantine of the currently published (already gated) entry, or
+// the trust-mode default if the runtime has never seen it. An unreadable storage
+// is never written to. An explicit operator `quarantined` value is always obeyed.
+func (r *Runtime) admitServerForRestart(diskCfg *config.Config, srv *config.ServerConfig, stored map[string]*config.ServerConfig, storageOK bool) (*config.ServerConfig, bool) {
+	if storageOK {
+		one := *diskCfg
+		one.Servers = []*config.ServerConfig{srv}
+		gated, _ := r.applyConfigLoadAdmissionGate(&one, stored, true)
+		return gated.Servers[0], true
+	}
+
+	out := config.CopyServerConfig(srv)
+	if out.QuarantineExplicitlySet() || out.Quarantined {
+		return out, false
+	}
+	published := r.Config()
+	if r.configSvc != nil {
+		if snap := r.configSvc.Current(); snap != nil && snap.Config != nil {
+			published = snap.Config
+		}
+	}
+	known := false
+	if published != nil {
+		for _, sc := range published.Servers {
+			if sc != nil && sc.Name == srv.Name {
+				out.Quarantined = sc.Quarantined
+				known = true
+				break
+			}
+		}
+	}
+	if !known && diskCfg.QuarantineDefaultForServer(out) {
+		out.Quarantined = true
+	}
+	r.logger.Warn("Server storage unreadable during restart; keeping the published quarantine decision instead of trusting the config file",
+		zap.String("server", srv.Name),
+		zap.Bool("quarantined", out.Quarantined))
+	return out, false
+}
+
 // 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.

internal/runtime/lifecycle.go

@@ -2230,6 +2230,10 @@ func (r *Runtime) BulkEnableServers(serverNames []string, enabled bool) (map[str
 // see the same value. Without this, only the synchronous restart that did
 // the disk read would see the edit; the next one would replay storage and
 // regress. See issue #467 for context.
+//
+// The disk entry is admission-gated (issue #937) before it is persisted or
+// returned: a server whose file entry never stated `quarantined` keeps the
+// quarantine recorded for it instead of being reset to unquarantined.
 func (r *Runtime) lookupServerConfigForRestart(serverName string) *config.ServerConfig {
 	r.mu.RLock()
 	cfgPath := r.cfgPath
@@ -2245,14 +2249,15 @@ func (r *Runtime) lookupServerConfigForRestart(serverName string) *config.Server
 		} else {
 			for _, srv := range diskCfg.Servers {
 				if srv != nil && srv.Name == serverName {
-					if r.storageManager != nil {
-						if saveErr := r.storageManager.SaveUpstreamServer(srv); saveErr != nil {
+					gated, persist := r.gateServerForRestart(diskCfg, srv)
+					if persist && r.storageManager != nil {
+						if saveErr := r.storageManager.SaveUpstreamServer(gated); saveErr != nil {
 							r.logger.Warn("Failed to persist disk-loaded config to storage during restart",
 								zap.String("server", serverName),
 								zap.Error(saveErr))
 						}
 					}
-					return srv
+					return gated
 				}
 			}
 		}

internal/runtime/restart_quarantine_test.go

@@ -0,0 +1,194 @@
+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"
+	"github.com/smart-mcp-proxy/mcpproxy-go/internal/storage"
+)
+
+// A server written into mcp_config.json with no `quarantined` key is held by
+// the config-load admission gate (#937). RestartServer re-reads the file from
+// disk (#467); it must run that entry through the same gate instead of writing
+// the raw, un-gated entry over the recorded quarantine in config.db.
+
+func restartQuarantineEnv(t *testing.T, extra map[string]any, servers ...map[string]any) (*Runtime, string) {
+	t.Helper()
+	rt, _, cfgPath := gateEnvAt(t, servers, extra, zap.NewNop())
+	require.NoError(t, rt.LoadConfiguredServers(nil))
+	return rt, cfgPath
+}
+
+func silentServer(name string) map[string]any {
+	return map[string]any{"name": name, "command": "true", "protocol": "stdio", "enabled": true}
+}
+
+// rewriteServersOnDisk replaces the mcpServers array of the config file.
+func rewriteServersOnDisk(t *testing.T, cfgPath string, servers ...map[string]any) {
+	t.Helper()
+	raw, err := os.ReadFile(cfgPath)
+	require.NoError(t, err)
+	var doc map[string]any
+	require.NoError(t, json.Unmarshal(raw, &doc))
+	list := make([]any, 0, len(servers))
+	for _, s := range servers {
+		list = append(list, s)
+	}
+	doc["mcpServers"] = list
+	out, err := json.Marshal(doc)
+	require.NoError(t, err)
+	require.NoError(t, os.WriteFile(cfgPath, out, 0600))
+}
+
+func TestRestartServer_KeepsImplicitQuarantine(t *testing.T) {
+	rt, cfgPath := restartQuarantineEnv(t, nil, silentServer("victim"), silentServer("approved"))
+	require.True(t, storedServer(t, rt, "victim").Quarantined, "precondition: gate quarantined the first-seen server")
+
+	_ = rt.RestartServer("victim")
+
+	assert.True(t, storedServer(t, rt, "victim").Quarantined,
+		"RestartServer must not write the raw config file entry (quarantined=false) over the recorded quarantine")
+
+	// Approving a DIFFERENT server must not change the victim.
+	require.NoError(t, rt.QuarantineServer("approved", false))
+	assert.True(t, storedServer(t, rt, "victim").Quarantined, "approving another server un-quarantined 'victim'")
+	assert.False(t, storedServer(t, rt, "approved").Quarantined)
+
+	for _, sc := range rt.Config().Servers {
+		if sc.Name == "victim" {
+			assert.True(t, sc.Quarantined, "published config must still hold victim")
+		}
+	}
+
+	// The file written by SaveConfiguration must carry the quarantine.
+	data, err := os.ReadFile(cfgPath)
+	require.NoError(t, err)
+	var raw struct {
+		Servers []map[string]any `json:"mcpServers"`
+	}
+	require.NoError(t, json.Unmarshal(data, &raw))
+	found := false
+	for _, s := range raw.Servers {
+		if s["name"] == "victim" {
+			found = true
+			assert.Equal(t, true, s["quarantined"], "config file must record victim as quarantined")
+		}
+	}
+	assert.True(t, found)
+}
+
+func TestRestartServer_KeepsImplicitQuarantineAcrossReload(t *testing.T) {
+	dir := t.TempDir()
+	cfgPath := filepath.Join(dir, "mcp_config.json")
+	fileCfg := config.DefaultConfig()
+	fileCfg.Listen = "127.0.0.1:0"
+	fileCfg.DataDir = dir
+	fileCfg.Servers = []*config.ServerConfig{
+		{Name: "victim", Command: "true", Protocol: "stdio", Enabled: true},
+		{Name: "approved", Command: "true", Protocol: "stdio", Enabled: true},
+	}
+	require.NoError(t, config.SaveConfig(fileCfg, cfgPath))
+
+	boot := func() *Runtime {
+		cfg, err := config.LoadFromFile(cfgPath)
+		require.NoError(t, err)
+		rt, err := New(cfg, cfgPath, zap.NewNop())
+		require.NoError(t, err)
+		require.NoError(t, rt.LoadConfiguredServers(nil))
+		return rt
+	}
+
+	rt := boot()
+	require.True(t, storedServer(t, rt, "victim").Quarantined)
+	_ = rt.RestartServer("victim")
+	require.NoError(t, rt.QuarantineServer("approved", false))
+	require.NoError(t, rt.Close())
+
+	// Simulate a reboot: fresh runtime over the same data dir and config file.
+	rt2 := boot()
+	t.Cleanup(func() { _ = rt2.Close() })
+
+	assert.True(t, storedServer(t, rt2, "victim").Quarantined, "victim must stay quarantined after a reboot")
+	approvals, err := rt2.ListToolApprovals("victim")
+	require.NoError(t, err)
+	for _, a := range approvals {
+		assert.NotEqual(t, storage.ToolApprovalStatusApproved, a.Status, "victim tools must not be baseline-approved")
+	}
+}
+
+func TestRestartServer_FirstSeenServerIsGated(t *testing.T) {
+	rt, cfgPath := restartQuarantineEnv(t, nil, silentServer("existing"))
+
+	// Hand-add a server to the file; the watcher has not fired, so config.db
+	// has never seen it.
+	rewriteServersOnDisk(t, cfgPath, silentServer("existing"), silentServer("fresh"))
+
+	_ = rt.RestartServer("fresh")
+
+	assert.True(t, storedServer(t, rt, "fresh").Quarantined, "first-seen server restarted from disk must be gated")
+}
+
+func TestRestartServer_ExplicitFalseOnDiskIsHonoured(t *testing.T) {
+	rt, cfgPath := restartQuarantineEnv(t, nil, silentServer("victim"))
+	require.True(t, storedServer(t, rt, "victim").Quarantined)
+
+	// The operator states quarantined:false by hand.
+	explicit := silentServer("victim")
+	explicit["quarantined"] = false
+	rewriteServersOnDisk(t, cfgPath, explicit)
+
+	_ = rt.RestartServer("victim")
+
+	assert.False(t, storedServer(t, rt, "victim").Quarantined, "an explicit operator false on disk is obeyed")
+}
+
+func TestRestartServer_TrustModeAutoNotGated(t *testing.T) {
+	auto := func(name string) map[string]any {
+		s := silentServer(name)
+		s["trust_mode"] = "auto"
+		return s
+	}
+	rt, cfgPath := restartQuarantineEnv(t, nil, auto("existing"))
+	require.False(t, storedServer(t, rt, "existing").Quarantined, "precondition: auto trust mode does not gate")
+
+	rewriteServersOnDisk(t, cfgPath, auto("existing"), auto("fresh"))
+
+	_ = rt.RestartServer("fresh")
+
+	assert.False(t, storedServer(t, rt, "fresh").Quarantined, "auto trust mode must not over-quarantine on restart")
+}
+
+func TestRestartAdmission_StorageUnreadableFailsClosed(t *testing.T) {
+	rt, cfgPath := restartQuarantineEnv(t, nil, silentServer("victim"))
+	require.True(t, storedServer(t, rt, "victim").Quarantined)
+
+	diskCfg, err := config.LoadFromFile(cfgPath)
+	require.NoError(t, err)
+	disk := diskCfg.Servers[0]
+	require.False(t, disk.Quarantined)
+
+	// Storage unreadable: the published (gated) entry says quarantined.
+	got, persist := rt.admitServerForRestart(diskCfg, disk, nil, false)
+	assert.True(t, got.Quarantined, "fail closed: inherit the published quarantine")
+	assert.False(t, persist, "never write to an unreadable storage")
+	assert.False(t, disk.Quarantined, "the caller's struct must not be mutated")
+
+	// Absent from the published config: fall back to the trust-mode default.
+	other := &config.ServerConfig{Name: "unknown-to-runtime", Command: "true", Protocol: "stdio", Enabled: true}
+	got, persist = rt.admitServerForRestart(diskCfg, other, nil, false)
+	assert.True(t, got.Quarantined, "fail closed: manual trust mode default")
+	assert.False(t, persist)
+
+	// An explicit operator false is still obeyed.
+	stated := &config.ServerConfig{Name: "victim", Command: "true", Protocol: "stdio", Enabled: true}
+	stated.MarkQuarantineExplicitlySet(true)
+	got, _ = rt.admitServerForRestart(diskCfg, stated, nil, false)
+	assert.False(t, got.Quarantined, "explicit false is obeyed even when storage is unreadable")
+}

internal/server/binding_guard_mcp_test.go

@@ -76,7 +76,11 @@ func TestBindingGuardRuntime_EndToEnd(t *testing.T) {
 	require.True(t, found, "anonymous_denied_by_binding_guard must be reported")
 
 	// a fix lifts the guard: anonymous_profile equal to the binding
-	proxy.currentConfig().AnonymousProfile = "work-readonly"
+	// Publish a new snapshot instead of mutating the live one: the attention
+	// subscriber reads the current config concurrently (data race under -race).
+	fixed := *proxy.currentConfig()
+	fixed.AnonymousProfile = "work-readonly"
+	rt.UpdateConfig(&fixed, "")
 	idx = proxy.profileIndexFor(proxy.currentConfig())
 	res = proxy.ResolveProfileV3(anonCtx(), idx)
 	require.False(t, res.BindingGuarded)

internal/server/e2e_content_forward_test.go

@@ -119,6 +119,7 @@ func TestE2E_ImageContentPreservation(t *testing.T) {
 	serverConfig, err := env.proxyServer.runtime.StorageManager().GetUpstreamServer("imgserver")
 	require.NoError(t, err)
 	serverConfig.Quarantined = false
+	serverConfig.MarkQuarantineExplicitlySet(true) // explicit decision; SaveUpstreamServer refuses to lower quarantine otherwise
 	err = env.proxyServer.runtime.StorageManager().SaveUpstreamServer(serverConfig)
 	require.NoError(t, err)
 

internal/server/e2e_iserror_activity_test.go

@@ -112,6 +112,7 @@ func TestE2E_UpstreamIsErrorRecordedAsActivityError(t *testing.T) {
 	serverConfig, err := rt.StorageManager().GetUpstreamServer("flaky")
 	require.NoError(t, err)
 	serverConfig.Quarantined = false
+	serverConfig.MarkQuarantineExplicitlySet(true) // explicit decision; SaveUpstreamServer refuses to lower quarantine otherwise
 	require.NoError(t, rt.StorageManager().SaveUpstreamServer(serverConfig))
 
 	servers, err := rt.StorageManager().ListUpstreamServers()