diff --git a/docs/features/security-quarantine.md b/docs/features/security-quarantine.md index 4d887b428..daf015776 100644 --- a/docs/features/security-quarantine.md +++ b/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:** diff --git a/internal/httpapi/patch_server_test.go b/internal/httpapi/patch_server_test.go index 611c118d7..9fc5b8c9a 100644 --- a/internal/httpapi/patch_server_test.go +++ b/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") + }) +} diff --git a/internal/httpapi/server.go b/internal/httpapi/server.go index 88017a437..d7a160e3c 100644 --- a/internal/httpapi/server.go +++ b/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 diff --git a/internal/runtime/config_load_admission_gate.go b/internal/runtime/config_load_admission_gate.go index 63b42d3ab..c2076382a 100644 --- a/internal/runtime/config_load_admission_gate.go +++ b/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. diff --git a/internal/runtime/lifecycle.go b/internal/runtime/lifecycle.go index 1ef1186c6..2970692c3 100644 --- a/internal/runtime/lifecycle.go +++ b/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 } } } diff --git a/internal/runtime/restart_quarantine_test.go b/internal/runtime/restart_quarantine_test.go new file mode 100644 index 000000000..072cd7c1a --- /dev/null +++ b/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") +} diff --git a/internal/server/binding_guard_mcp_test.go b/internal/server/binding_guard_mcp_test.go index b4d2af0c8..fd4c49d65 100644 --- a/internal/server/binding_guard_mcp_test.go +++ b/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) diff --git a/internal/server/e2e_content_forward_test.go b/internal/server/e2e_content_forward_test.go index 546f4f1dc..8515f4803 100644 --- a/internal/server/e2e_content_forward_test.go +++ b/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) diff --git a/internal/server/e2e_iserror_activity_test.go b/internal/server/e2e_iserror_activity_test.go index db3417a47..b9983e9c1 100644 --- a/internal/server/e2e_iserror_activity_test.go +++ b/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() diff --git a/internal/server/e2e_sensitive_data_test.go b/internal/server/e2e_sensitive_data_test.go index 992946ed3..ea0404a09 100644 --- a/internal/server/e2e_sensitive_data_test.go +++ b/internal/server/e2e_sensitive_data_test.go @@ -108,6 +108,7 @@ func TestE2E_SensitiveData_AWSAccessKey(t *testing.T) { serverConfig, err := env.proxyServer.runtime.StorageManager().GetUpstreamServer("testserver") 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) @@ -252,6 +253,7 @@ func TestE2E_SensitiveData_FilePath(t *testing.T) { serverConfig, err := env.proxyServer.runtime.StorageManager().GetUpstreamServer("fileserver") 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) @@ -644,6 +646,7 @@ func TestE2E_SensitiveData_CreditCard(t *testing.T) { serverConfig, err := env.proxyServer.runtime.StorageManager().GetUpstreamServer("paymentserver") 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) diff --git a/internal/server/e2e_test.go b/internal/server/e2e_test.go index f72474219..cdf3f68f3 100644 --- a/internal/server/e2e_test.go +++ b/internal/server/e2e_test.go @@ -400,6 +400,7 @@ func TestE2E_ToolDiscovery(t *testing.T) { serverConfig, err := env.proxyServer.runtime.StorageManager().GetUpstreamServer("testserver") 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) @@ -508,6 +509,7 @@ func TestE2E_ToolCalling(t *testing.T) { serverConfig, err := env.proxyServer.runtime.StorageManager().GetUpstreamServer("echoserver") 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) @@ -1573,6 +1575,7 @@ func TestE2E_IntentDeclarationToolVariants(t *testing.T) { serverConfig, err := env.proxyServer.runtime.StorageManager().GetUpstreamServer("dataserver") 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) @@ -2782,6 +2785,7 @@ func TestE2E_DisableServerRemovesToolsFromSearch(t *testing.T) { serverConfig, err := env.proxyServer.runtime.StorageManager().GetUpstreamServer(serverName) 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) @@ -2966,6 +2970,7 @@ func TestE2E_ServerDeleteReaddDifferentTools(t *testing.T) { serverConfig, err := env.proxyServer.runtime.StorageManager().GetUpstreamServer(serverName) 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) @@ -3090,6 +3095,7 @@ func TestE2E_ServerDeleteReaddDifferentTools(t *testing.T) { serverConfigB, err := env.proxyServer.runtime.StorageManager().GetUpstreamServer(serverName) require.NoError(t, err) serverConfigB.Quarantined = false + serverConfigB.MarkQuarantineExplicitlySet(true) // explicit decision; SaveUpstreamServer refuses to lower quarantine otherwise err = env.proxyServer.runtime.StorageManager().SaveUpstreamServer(serverConfigB) require.NoError(t, err) @@ -3270,6 +3276,7 @@ func TestE2E_RetrieveToolsAnnotationsAndCallWith(t *testing.T) { serverConfig, err := env.proxyServer.runtime.StorageManager().GetUpstreamServer("annotated") 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) @@ -3420,6 +3427,7 @@ func TestE2E_SelfHealingInvalidParams(t *testing.T) { serverConfig, err := env.proxyServer.runtime.StorageManager().GetUpstreamServer(serverName) require.NoError(t, err) serverConfig.Quarantined = false + serverConfig.MarkQuarantineExplicitlySet(true) // explicit decision; SaveUpstreamServer refuses to lower quarantine otherwise require.NoError(t, env.proxyServer.runtime.StorageManager().SaveUpstreamServer(serverConfig)) servers, err := env.proxyServer.runtime.StorageManager().ListUpstreamServers() @@ -3569,6 +3577,7 @@ func TestE2E_ToolResponseModeToggle(t *testing.T) { serverConfig, err := env.proxyServer.runtime.StorageManager().GetUpstreamServer(serverName) require.NoError(t, err) serverConfig.Quarantined = false + serverConfig.MarkQuarantineExplicitlySet(true) // explicit decision; SaveUpstreamServer refuses to lower quarantine otherwise require.NoError(t, env.proxyServer.runtime.StorageManager().SaveUpstreamServer(serverConfig)) servers, err := env.proxyServer.runtime.StorageManager().ListUpstreamServers() diff --git a/internal/server/forward_headers_record_test.go b/internal/server/forward_headers_record_test.go index a57f786ed..de39c852e 100644 --- a/internal/server/forward_headers_record_test.go +++ b/internal/server/forward_headers_record_test.go @@ -115,6 +115,7 @@ func TestForwardedHeaderSuccessEchoScrubbedFromRecords(t *testing.T) { require.NoError(t, err) require.Equal(t, []string{"X-Tenant-Id"}, sc.ForwardHeaders) sc.Quarantined = false + sc.MarkQuarantineExplicitlySet(true) // explicit decision; SaveUpstreamServer refuses to lower quarantine otherwise require.NoError(t, rt.StorageManager().SaveUpstreamServer(sc)) servers, err := rt.StorageManager().ListUpstreamServers() require.NoError(t, err) diff --git a/internal/server/mcp_identity_skew_test.go b/internal/server/mcp_identity_skew_test.go index 8de4cf4bb..37f280f71 100644 --- a/internal/server/mcp_identity_skew_test.go +++ b/internal/server/mcp_identity_skew_test.go @@ -557,6 +557,7 @@ func TestCallTool_PersistedServerVerdictOutranksLaggingStateView(t *testing.T) { // the StateView ever caught up. The never-listed name is // Unresolved again on both paths; the listed sibling // dispatches, certified on the live generation. + stored.MarkQuarantineExplicitlySet(true) // the fixture's revert is an explicit decision; SaveUpstreamServer otherwise keeps a recorded quarantine require.NoError(t, proxy.storage.SaveUpstreamServer(stored)) require.True(t, proxy.resolveExactToolIdentity("a", "ghost").Unresolved(), "control: ghost is unresolved once the record is restored") _, text := callToolReadResult(t, proxy, ctx, "a:ghost") @@ -753,6 +754,7 @@ func TestCallTool_GateRecordIsTheOnlyPersistedReadOfADispatch(t *testing.T) { // Nested: same invariant on the sandbox bridge. Reset the // record so the bridge's gate admits, and flip it again in // the seam. + stored.MarkQuarantineExplicitlySet(true) // the fixture's revert is an explicit decision; SaveUpstreamServer otherwise keeps a recorded quarantine require.NoError(t, proxy.storage.SaveUpstreamServer(stored)) st, ok := rt.Supervisor().StateView().GetServer("a") require.True(t, ok) @@ -863,6 +865,7 @@ func TestCodeExecution_PreflightGateIsTheDispatchGate(t *testing.T) { // the captured gate's verdict — unresolved identity — and the // NEXT call answers the server-level verdict for the same name // (SC-005 parity with its listed sibling), zero upstream calls. + stored.MarkQuarantineExplicitlySet(true) // the fixture's revert is an explicit decision; SaveUpstreamServer otherwise keeps a recorded quarantine require.NoError(t, proxy.storage.SaveUpstreamServer(stored)) paused.Store(0) proxy.sandboxPreflightPause = func(string, string) { diff --git a/internal/server/server.go b/internal/server/server.go index 30873af11..e740e7466 100644 --- a/internal/server/server.go +++ b/internal/server/server.go @@ -2035,11 +2035,18 @@ func (s *Server) UpdateServer(ctx context.Context, serverName string, updates *c if updates.Protocol != "" { existing.Protocol = updates.Protocol } - // Booleans are always applied since the handler only calls UpdateServer - // when the caller explicitly provided these fields + // Enabled and ReconnectOnUse are always applied: the REST handler resolves + // them against the existing server before calling UpdateServer. existing.Enabled = updates.Enabled - existing.Quarantined = updates.Quarantined existing.ReconnectOnUse = updates.ReconnectOnUse + // Quarantine is applied only when the caller stated it (the REST PATCH + // handler marks the explicit bit when the body carries `quarantined`). + // Otherwise the stored value stands: `updates.Quarantined` can be a stale + // false copied from a config snapshot, which must never un-quarantine. + if updates.QuarantineExplicitlySet() { + existing.Quarantined = updates.Quarantined + existing.MarkQuarantineExplicitlySet(true) + } // AutoApproveToolChanges is a tri-state *bool (MCP-2940): nil means // "leave unchanged" so callers that don't touch it (e.g. config-to-secret) diff --git a/internal/server/toon_e2e_test.go b/internal/server/toon_e2e_test.go index 701edf7f6..3f259b243 100644 --- a/internal/server/toon_e2e_test.go +++ b/internal/server/toon_e2e_test.go @@ -209,6 +209,7 @@ func TestE2E_ToonOutputModeFlip(t *testing.T) { serverConfig, err := env.proxyServer.runtime.StorageManager().GetUpstreamServer("toonserver") require.NoError(t, err) serverConfig.Quarantined = false + serverConfig.MarkQuarantineExplicitlySet(true) // explicit decision; SaveUpstreamServer refuses to lower quarantine otherwise require.NoError(t, env.proxyServer.runtime.StorageManager().SaveUpstreamServer(serverConfig)) servers, err := env.proxyServer.runtime.StorageManager().ListUpstreamServers() diff --git a/internal/server/update_server_quarantine_test.go b/internal/server/update_server_quarantine_test.go new file mode 100644 index 000000000..76b37e6e8 --- /dev/null +++ b/internal/server/update_server_quarantine_test.go @@ -0,0 +1,71 @@ +package server + +import ( + "context" + "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" +) + +// UpdateServer is the REST PATCH / config-to-secret write path. It starts from +// the stored record, so an update that never mentioned `quarantined` must keep +// the stored value instead of copying whatever false the caller's struct holds. + +func updateServerFixture(t *testing.T) (*Server, string) { + t.Helper() + proxy, rt := createTestProxyWithRuntime(t, []*config.ServerConfig{{ + Name: "victim", Command: "true", Protocol: "stdio", Enabled: true, Quarantined: true, + }}) + srv := proxy.mainServer + srv.logger = zap.NewNop() + + // Make sure storage holds the quarantined record, and point the runtime at a + // config file so SaveConfiguration has somewhere to write. + require.NoError(t, rt.StorageManager().SaveUpstreamServer(&config.ServerConfig{ + Name: "victim", Command: "true", Protocol: "stdio", Enabled: true, Quarantined: true, + })) + cfgPath := filepath.Join(t.TempDir(), "mcp_config.json") + require.NoError(t, config.SaveConfig(rt.Config(), cfgPath)) + rt.UpdateConfig(rt.Config(), cfgPath) + return srv, cfgPath +} + +func TestUpdateServer_OmittedQuarantineKeepsStoredValue(t *testing.T) { + srv, _ := updateServerFixture(t) + + updates := &config.ServerConfig{Args: []string{"--x"}, Enabled: true} // Quarantined=false, never stated + require.NoError(t, srv.UpdateServer(context.Background(), "victim", updates)) + + got, err := srv.runtime.StorageManager().GetUpstreamServer("victim") + require.NoError(t, err) + assert.True(t, got.Quarantined, "an update that did not state quarantine must keep the stored value") + assert.Equal(t, []string{"--x"}, got.Args) +} + +func TestUpdateServer_ExplicitFalseUnquarantines(t *testing.T) { + srv, cfgPath := updateServerFixture(t) + + updates := &config.ServerConfig{Enabled: true, Quarantined: false} + updates.MarkQuarantineExplicitlySet(true) + require.NoError(t, srv.UpdateServer(context.Background(), "victim", updates)) + + got, err := srv.runtime.StorageManager().GetUpstreamServer("victim") + require.NoError(t, err) + assert.False(t, got.Quarantined, "an explicit operator decision is applied") + + data, err := os.ReadFile(cfgPath) + require.NoError(t, err) + var doc struct { + Servers []map[string]any `json:"mcpServers"` + } + require.NoError(t, json.Unmarshal(data, &doc)) + require.Len(t, doc.Servers, 1) + assert.Equal(t, false, doc.Servers[0]["quarantined"], "the saved file must carry the stated value") +} diff --git a/internal/storage/async_ops.go b/internal/storage/async_ops.go index 4f98073a9..9ef3d82e2 100644 --- a/internal/storage/async_ops.go +++ b/internal/storage/async_ops.go @@ -213,7 +213,14 @@ func (am *AsyncManager) saveServerSync(serverConfig *config.ServerConfig) error QueueTimeout: serverConfig.QueueTimeout, ExposePrompts: serverConfig.ExposePrompts, } - return am.db.SaveUpstream(record) + // Same invariant as Manager.SaveUpstreamServer: never lower a recorded + // quarantine without an explicit decision (only quarantineServerSync may). + kept, err := am.db.SaveUpstreamKeepingQuarantine(record, serverConfig.QuarantineExplicitlySet()) + if kept { + am.logger.Warnw("Refusing to lower a recorded quarantine without an explicit decision", + "server", serverConfig.Name) + } + return err } func (am *AsyncManager) deleteServerSync(name string) error { diff --git a/internal/storage/async_ops_test.go b/internal/storage/async_ops_test.go index 5f49324be..c839be496 100644 --- a/internal/storage/async_ops_test.go +++ b/internal/storage/async_ops_test.go @@ -540,3 +540,49 @@ func TestForwardHeadersRoundTrip(t *testing.T) { t.Error("quarantined server not listed") } } + +// saveServerSync is a second writer of the upstream record and must honour the +// same invariant as Manager.SaveUpstreamServer: it never lowers a recorded +// quarantine without an explicit decision. +func TestSaveServerSync_DoesNotLowerRecordedQuarantine(t *testing.T) { + logger := zaptest.NewLogger(t).Sugar() + manager, err := NewManager(t.TempDir(), logger) + if err != nil { + t.Fatalf("Failed to create storage manager: %v", err) + } + defer manager.Close() + + am := NewAsyncManager(manager.db, logger) + am.Start() + defer am.Stop() + + sc := &config.ServerConfig{Name: "s", Command: "true", Protocol: "stdio", Enabled: true, Quarantined: true, Created: time.Now()} + if err := am.saveServerSync(sc); err != nil { + t.Fatalf("save: %v", err) + } + + lower := &config.ServerConfig{Name: "s", Command: "true", Protocol: "stdio", Enabled: true, Created: time.Now()} + if err := am.saveServerSync(lower); err != nil { + t.Fatalf("save: %v", err) + } + rec, err := manager.db.GetUpstream("s") + if err != nil { + t.Fatalf("get: %v", err) + } + if !rec.Quarantined { + t.Fatal("saveServerSync lowered a recorded quarantine without an explicit decision") + } + + explicit := &config.ServerConfig{Name: "s", Command: "true", Protocol: "stdio", Enabled: true, Created: time.Now()} + explicit.MarkQuarantineExplicitlySet(true) + if err := am.saveServerSync(explicit); err != nil { + t.Fatalf("save: %v", err) + } + rec, err = manager.db.GetUpstream("s") + if err != nil { + t.Fatalf("get: %v", err) + } + if rec.Quarantined { + t.Fatal("an explicit decision must lower the quarantine") + } +} diff --git a/internal/storage/bbolt.go b/internal/storage/bbolt.go index a2dd6d845..39b988984 100644 --- a/internal/storage/bbolt.go +++ b/internal/storage/bbolt.go @@ -227,6 +227,39 @@ func (b *BoltDB) SaveUpstream(record *UpstreamRecord) error { }) } +// SaveUpstreamKeepingQuarantine is SaveUpstream with the quarantine-lowering +// guard: if a record already exists with Quarantined=true and the incoming +// record would clear it without an explicit decision (explicit=false), the +// stored quarantine is kept. The read-check-write runs in one bbolt Update +// transaction so a concurrent QuarantineUpstreamServer cannot be lost between +// the read and the write. It reports whether the guard kept the quarantine. +// An undecodable previous record is overwritten, as SaveUpstream would. +func (b *BoltDB) SaveUpstreamKeepingQuarantine(record *UpstreamRecord, explicit bool) (kept bool, err error) { + record.Updated = time.Now() + + err = b.db.Update(func(tx *bbolt.Tx) error { + bucket := tx.Bucket([]byte(UpstreamsBucket)) + if !record.Quarantined && !explicit { + if prevData := bucket.Get([]byte(record.ID)); prevData != nil { + prev := &UpstreamRecord{} + if prev.UnmarshalBinary(prevData) == nil && prev.Quarantined { + record.Quarantined = true + kept = true + } + } + } + data, marshalErr := record.MarshalBinary() + if marshalErr != nil { + return marshalErr + } + return bucket.Put([]byte(record.ID), data) + }) + if err != nil { + kept = false + } + return kept, err +} + // GetUpstream retrieves an upstream server record by ID func (b *BoltDB) GetUpstream(id string) (*UpstreamRecord, error) { var record *UpstreamRecord diff --git a/internal/storage/manager.go b/internal/storage/manager.go index 6254fada8..c4ef59f62 100644 --- a/internal/storage/manager.go +++ b/internal/storage/manager.go @@ -113,7 +113,15 @@ func (m *Manager) GetBoltDB() *BoltDB { // Upstream operations -// SaveUpstreamServer saves an upstream server configuration +// SaveUpstreamServer saves an upstream server configuration. +// +// Invariant: it never lowers a recorded Quarantined=true. Only +// QuarantineUpstreamServer (the review/approve door) or a config that carries an +// explicit operator decision (QuarantineExplicitlySet) may clear it. A Go-built +// or file-decoded ServerConfig that merely says Quarantined=false is +// indistinguishable from "never stated", so it must not erase a quarantine the +// admission gate recorded. The guard changes only the persisted record, never +// the caller's struct. func (m *Manager) SaveUpstreamServer(serverConfig *config.ServerConfig) error { m.mu.Lock() defer m.mu.Unlock() @@ -154,7 +162,13 @@ func (m *Manager) SaveUpstreamServer(serverConfig *config.ServerConfig) error { ExposePrompts: serverConfig.ExposePrompts, } - return m.db.SaveUpstream(record) + kept, err := m.db.SaveUpstreamKeepingQuarantine(record, serverConfig.QuarantineExplicitlySet()) + if kept { + m.logger.Warnw("Refusing to lower a recorded quarantine without an explicit decision", + "server", serverConfig.Name, + "action", "use the quarantine review (QuarantineServer) or state \"quarantined\": false in mcp_config.json") + } + return err } // GetUpstreamServer retrieves an upstream server by name diff --git a/internal/storage/quarantine_guard_test.go b/internal/storage/quarantine_guard_test.go new file mode 100644 index 000000000..f689a9d6a --- /dev/null +++ b/internal/storage/quarantine_guard_test.go @@ -0,0 +1,143 @@ +package storage + +import ( + "encoding/json" + "sync" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "go.uber.org/zap/zaptest" + + "github.com/smart-mcp-proxy/mcpproxy-go/internal/config" +) + +// Invariant: SaveUpstreamServer never lowers a recorded Quarantined=true unless +// the incoming config carries an explicit operator decision. Only +// QuarantineUpstreamServer (the review/approve door) may clear it otherwise. + +func newGuardManager(t *testing.T) *Manager { + t.Helper() + m, err := NewManager(t.TempDir(), zaptest.NewLogger(t).Sugar()) + require.NoError(t, err) + t.Cleanup(func() { _ = m.Close() }) + return m +} + +func guardServer(name string, quarantined bool) *config.ServerConfig { + return &config.ServerConfig{ + Name: name, + Command: "true", + Protocol: "stdio", + Enabled: true, + Quarantined: quarantined, + Created: time.Now(), + } +} + +func storedQuarantined(t *testing.T, m *Manager, name string) bool { + t.Helper() + sc, err := m.GetUpstreamServer(name) + require.NoError(t, err) + return sc.Quarantined +} + +func TestSaveUpstreamServer_DoesNotLowerRecordedQuarantine(t *testing.T) { + m := newGuardManager(t) + require.NoError(t, m.SaveUpstreamServer(guardServer("s", true))) + + update := guardServer("s", false) + update.Args = []string{"--changed"} + update.Env = map[string]string{"K": "V"} + require.NoError(t, m.SaveUpstreamServer(update)) + + got, err := m.GetUpstreamServer("s") + require.NoError(t, err) + assert.True(t, got.Quarantined, "an unstated false must not lower the recorded quarantine") + assert.Equal(t, []string{"--changed"}, got.Args, "other fields still update") + assert.Equal(t, "V", got.Env["K"]) +} + +func TestSaveUpstreamServer_ExplicitFalseLowersQuarantine(t *testing.T) { + m := newGuardManager(t) + require.NoError(t, m.SaveUpstreamServer(guardServer("s", true))) + + update := guardServer("s", false) + update.MarkQuarantineExplicitlySet(true) + require.NoError(t, m.SaveUpstreamServer(update)) + + assert.False(t, storedQuarantined(t, m, "s")) +} + +func TestSaveUpstreamServer_ExplicitFalseFromJSON(t *testing.T) { + m := newGuardManager(t) + require.NoError(t, m.SaveUpstreamServer(guardServer("x", true))) + + var sc config.ServerConfig + require.NoError(t, json.Unmarshal([]byte(`{"name":"x","command":"true","protocol":"stdio","enabled":true,"quarantined":false}`), &sc)) + require.True(t, sc.QuarantineExplicitlySet()) + require.NoError(t, m.SaveUpstreamServer(&sc)) + + assert.False(t, storedQuarantined(t, m, "x"), "an operator-written quarantined:false is obeyed") +} + +func TestSaveUpstreamServer_NewServerUnquarantinedIsSaved(t *testing.T) { + m := newGuardManager(t) + require.NoError(t, m.SaveUpstreamServer(guardServer("fresh", false))) + assert.False(t, storedQuarantined(t, m, "fresh")) +} + +func TestSaveUpstreamServer_RaiseAlwaysAllowed(t *testing.T) { + m := newGuardManager(t) + require.NoError(t, m.SaveUpstreamServer(guardServer("s", false))) + require.NoError(t, m.SaveUpstreamServer(guardServer("s", true))) + assert.True(t, storedQuarantined(t, m, "s")) +} + +func TestSaveUpstreamServer_DoesNotMutateCaller(t *testing.T) { + m := newGuardManager(t) + require.NoError(t, m.SaveUpstreamServer(guardServer("s", true))) + + update := guardServer("s", false) + require.NoError(t, m.SaveUpstreamServer(update)) + + assert.False(t, update.Quarantined, "the guard changes the persisted record, never the caller's struct") + assert.False(t, update.QuarantineExplicitlySet()) +} + +func TestQuarantineUpstreamServer_StillUnquarantines(t *testing.T) { + m := newGuardManager(t) + require.NoError(t, m.SaveUpstreamServer(guardServer("s", true))) + + require.NoError(t, m.QuarantineUpstreamServer("s", false)) + + assert.False(t, storedQuarantined(t, m, "s"), "the sanctioned approve door must still work") +} + +func TestSaveUpstreamServer_ConcurrentQuarantineNotLost(t *testing.T) { + m := newGuardManager(t) + require.NoError(t, m.SaveUpstreamServer(guardServer("s", false))) + + var wg sync.WaitGroup + stop := make(chan struct{}) + wg.Add(1) + go func() { + defer wg.Done() + for { + select { + case <-stop: + return + default: + _ = m.SaveUpstreamServer(guardServer("s", false)) + } + } + }() + + require.NoError(t, m.QuarantineUpstreamServer("s", true)) + time.Sleep(50 * time.Millisecond) + close(stop) + wg.Wait() + + assert.True(t, storedQuarantined(t, m, "s"), "a concurrent quarantine must not be lost to a stale save") +}