From c703b1f5e4a5558f50e989dcbb0d28e381c07fde Mon Sep 17 00:00:00 2001 From: Algis Dumbris Date: Fri, 2 Oct 2026 11:52:22 +0300 Subject: [PATCH 1/5] fix(runtime): gate the disk entry on server restart so implicit quarantine survives RestartServer re-reads mcp_config.json (#467) and persisted the raw entry of a server that never stated quarantined, erasing the quarantine the config-load admission gate recorded. The entry now goes through the same gate before it is persisted or used, and fails closed when storage is unreadable. --- .../runtime/config_load_admission_gate.go | 62 ++++++ internal/runtime/lifecycle.go | 11 +- internal/runtime/restart_quarantine_test.go | 194 ++++++++++++++++++ 3 files changed, 264 insertions(+), 3 deletions(-) create mode 100644 internal/runtime/restart_quarantine_test.go 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") +} From a931ce7c7c3689f2cb5a7b5fb33aba8c9ec76734 Mon Sep 17 00:00:00 2001 From: Algis Dumbris Date: Fri, 2 Oct 2026 11:52:25 +0300 Subject: [PATCH 2/5] fix(storage): never lower a recorded quarantine without an explicit decision SaveUpstreamServer and the async saveServerSync now keep a stored Quarantined=true unless the incoming config states quarantined explicitly. The read-check-write runs in one bbolt transaction so a concurrent QuarantineUpstreamServer is not lost. Tests that un-quarantined through a plain save now mark the decision explicit. --- internal/server/e2e_content_forward_test.go | 1 + internal/server/e2e_iserror_activity_test.go | 1 + internal/server/e2e_sensitive_data_test.go | 3 + internal/server/e2e_test.go | 9 ++ .../server/forward_headers_record_test.go | 1 + internal/server/mcp_identity_skew_test.go | 3 + internal/server/toon_e2e_test.go | 1 + internal/storage/async_ops.go | 9 +- internal/storage/async_ops_test.go | 46 ++++++ internal/storage/bbolt.go | 33 ++++ internal/storage/manager.go | 18 ++- internal/storage/quarantine_guard_test.go | 143 ++++++++++++++++++ 12 files changed, 265 insertions(+), 3 deletions(-) create mode 100644 internal/storage/quarantine_guard_test.go 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/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/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") +} From d0bd601662f009cbab4393d229ba78d25e19f18a Mon Sep 17 00:00:00 2001 From: Algis Dumbris Date: Fri, 2 Oct 2026 11:52:26 +0300 Subject: [PATCH 3/5] fix(httpapi): only an explicit PATCH quarantined value changes quarantine The REST PATCH handler marks the explicit bit when the body carries quarantined, and UpdateServer applies the value only then, so an unrelated PATCH can no longer copy a stale false over the stored record. --- internal/httpapi/patch_server_test.go | 44 ++++++++++++ internal/httpapi/server.go | 3 + internal/server/server.go | 13 +++- .../server/update_server_quarantine_test.go | 71 +++++++++++++++++++ 4 files changed, 128 insertions(+), 3 deletions(-) create mode 100644 internal/server/update_server_quarantine_test.go 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/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/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") +} From 73e03bcc23d538527d46b62980aa7d00297366fb Mon Sep 17 00:00:00 2001 From: Algis Dumbris Date: Fri, 2 Oct 2026 11:52:26 +0300 Subject: [PATCH 4/5] docs(quarantine): note restarts cleared quarantine in releases affected by #937 --- docs/features/security-quarantine.md | 12 ++++++++++++ 1 file changed, 12 insertions(+) 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:** From 0edf3da47a2c464052562f37c1f47fca0fe97540 Mon Sep 17 00:00:00 2001 From: Algis Dumbris Date: Fri, 2 Oct 2026 13:13:59 +0300 Subject: [PATCH 5/5] test(server): publish a config snapshot instead of mutating the live one in binding guard e2e --- internal/server/binding_guard_mcp_test.go | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) 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)