Skip to content

Commit e45042d

Browse files
authored
feat(quarantine): baseline-approve pending tools on server unquarantine (#640)
When a server is unquarantined/approved, promote its status=pending tool-approval records to approved (baseline trust): approving a server means trusting its current tool snapshot. Tool-level quarantine is then reserved for status=changed (rug-pull) records only. Adds Runtime.approveBaselineToolsForServer, which promotes pending ONLY (never changed — deliberately does NOT reuse ApproveAllTools, so re-approving a server later never silently clears a genuine rug-pull flag). Called from the !quarantined branch of QuarantineServer after persist+reload and before re-index, so newly-trusted tools become immediately searchable. This also covers the scan-approve path (ApproveServer -> UnquarantineServer -> QuarantineServer(false)). Related MCP-2100, MCP-2081 (Spec 032).
1 parent bda8157 commit e45042d

4 files changed

Lines changed: 230 additions & 0 deletions

File tree

docs/features/security-quarantine.md

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -55,6 +55,11 @@ When a server is unquarantined (approved):
5555
1. The server connects to discover its tools
5656
2. Tools are **indexed and become searchable**
5757
3. Tool calls are allowed to execute normally
58+
4. Any **pending** (newly-discovered, never-reviewed) tool-approval records for
59+
the server are **auto-promoted to approved** — approving a server means you
60+
trust its current tool snapshot (baseline trust). Tools whose description or
61+
schema later **changes** (`changed`, i.e. rug-pull) are *not* affected and
62+
stay blocked until you re-approve them explicitly.
5863

5964
### Security Analysis
6065

internal/runtime/lifecycle.go

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1164,6 +1164,21 @@ func (r *Runtime) QuarantineServer(serverName string, quarantined bool) error {
11641164
return fmt.Errorf("failed to reload configuration: %w", err)
11651165
}
11661166

1167+
// On unquarantine/approval, baseline-trust the server's CURRENT tool
1168+
// snapshot: promote its pending (never-reviewed) tool records to approved.
1169+
// Tool-level quarantine then guards only status=changed (rug-pull) records.
1170+
// (Spec 032, MCP-2100; trust model confirmed in MCP-2081.) Done before the
1171+
// re-index in HandleUpstreamServerChange so the newly-trusted tools become
1172+
// immediately searchable. Best-effort: a promotion failure must not abort
1173+
// the unquarantine the user already requested and that is already persisted.
1174+
if !quarantined {
1175+
if err := r.approveBaselineToolsForServer(serverName); err != nil {
1176+
r.logger.Warn("Failed to baseline-approve tools on server unquarantine",
1177+
zap.String("server", serverName),
1178+
zap.Error(err))
1179+
}
1180+
}
1181+
11671182
r.emitServersChanged("quarantine_toggle", map[string]any{
11681183
"server": serverName,
11691184
"quarantined": quarantined,

internal/runtime/tool_quarantine.go

Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -962,6 +962,54 @@ func (r *Runtime) collectKnownToolNames(serverName string) ([]string, error) {
962962
return out, nil
963963
}
964964

965+
// approveBaselineToolsForServer promotes a server's pending tool-approval
966+
// records to approved as baseline trust when the server itself is
967+
// approved/unquarantined.
968+
//
969+
// Trust model (Spec 032, MCP-2081/MCP-2100, request_confirmation 7cfce731):
970+
// approving/unquarantining a server == trusting its CURRENT tool snapshot. So
971+
// pending (newly-discovered, never-reviewed) tools inherit that baseline trust
972+
// automatically. Tool-level quarantine is then reserved for status=changed
973+
// (rug-pull) records only.
974+
//
975+
// CRITICAL: this promotes status=pending ONLY. status=changed records are left
976+
// untouched so that re-approving a server later never silently clears a genuine
977+
// rug-pull flag (preserves Spec 032's rug-pull guarantee). This is precisely why
978+
// it does NOT reuse ApproveAllTools, which promotes both pending AND changed.
979+
func (r *Runtime) approveBaselineToolsForServer(serverName string) error {
980+
if r.storageManager == nil {
981+
return nil
982+
}
983+
984+
records, err := r.storageManager.ListToolApprovals(serverName)
985+
if err != nil {
986+
return err
987+
}
988+
989+
var pendingTools []string
990+
for _, record := range records {
991+
if record.Status == storage.ToolApprovalStatusPending {
992+
pendingTools = append(pendingTools, record.ToolName)
993+
}
994+
}
995+
996+
if len(pendingTools) == 0 {
997+
return nil
998+
}
999+
1000+
// ApproveTools sets ApprovedHash=CurrentHash, runs enforceInvariant
1001+
// (pending→approved is permitted), and emits activity + a single SSE event.
1002+
if err := r.ApproveTools(serverName, pendingTools, "system:server-approval-baseline"); err != nil {
1003+
return err
1004+
}
1005+
1006+
r.logger.Info("Baseline-approved pending tools on server approval",
1007+
zap.String("server", serverName),
1008+
zap.Int("count", len(pendingTools)))
1009+
1010+
return nil
1011+
}
1012+
9651013
// ApproveAllTools approves all pending/changed tools for a server.
9661014
func (r *Runtime) ApproveAllTools(serverName string, approvedBy string) (int, error) {
9671015
if r.storageManager == nil {
Lines changed: 162 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,162 @@
1+
package runtime
2+
3+
import (
4+
"path/filepath"
5+
"testing"
6+
7+
"github.com/stretchr/testify/assert"
8+
"github.com/stretchr/testify/require"
9+
"go.uber.org/zap"
10+
11+
"github.com/smart-mcp-proxy/mcpproxy-go/internal/config"
12+
"github.com/smart-mcp-proxy/mcpproxy-go/internal/storage"
13+
)
14+
15+
// TestApproveBaselineToolsForServer_PromotesPendingOnly verifies the core
16+
// baseline-trust rule (Spec 032, MCP-2100): when a server is approved, its
17+
// pending (never-reviewed) tool records inherit baseline trust and are promoted
18+
// to approved.
19+
func TestApproveBaselineToolsForServer_PromotesPendingOnly(t *testing.T) {
20+
rt := setupQuarantineRuntime(t, nil, []*config.ServerConfig{
21+
{Name: "github", Enabled: true, Quarantined: true},
22+
})
23+
24+
// Seed two pending tools (the state after first discovery under quarantine).
25+
tools := []*config.ToolMetadata{
26+
{ServerName: "github", Name: "create_issue", Description: "Creates issues", ParamsJSON: `{}`},
27+
{ServerName: "github", Name: "list_repos", Description: "Lists repos", ParamsJSON: `{}`},
28+
}
29+
result, err := rt.checkToolApprovals("github", tools)
30+
require.NoError(t, err)
31+
require.Equal(t, 2, result.PendingCount, "precondition: both tools pending")
32+
33+
// Approve the server's baseline tool snapshot.
34+
require.NoError(t, rt.approveBaselineToolsForServer("github"))
35+
36+
records, err := rt.storageManager.ListToolApprovals("github")
37+
require.NoError(t, err)
38+
require.Len(t, records, 2)
39+
for _, rec := range records {
40+
assert.Equal(t, storage.ToolApprovalStatusApproved, rec.Status,
41+
"tool %s must be approved after baseline approval", rec.ToolName)
42+
assert.Equal(t, "system:server-approval-baseline", rec.ApprovedBy)
43+
assert.Equal(t, rec.CurrentHash, rec.ApprovedHash,
44+
"approved hash must equal current hash (baseline trusts current snapshot)")
45+
}
46+
47+
// Re-checking discovery must now block nothing.
48+
result, err = rt.checkToolApprovals("github", tools)
49+
require.NoError(t, err)
50+
assert.Equal(t, 0, len(result.BlockedTools))
51+
assert.Equal(t, 0, result.PendingCount)
52+
}
53+
54+
// TestApproveBaselineToolsForServer_LeavesChangedUntouched is the critical
55+
// correctness constraint: baseline approval must promote pending ONLY, never
56+
// status=changed (rug pull). Re-approving a server later must not silently
57+
// clear a genuine rug-pull flag (preserves Spec 032's guarantee).
58+
func TestApproveBaselineToolsForServer_LeavesChangedUntouched(t *testing.T) {
59+
rt := setupQuarantineRuntime(t, nil, []*config.ServerConfig{
60+
{Name: "github", Enabled: true, Quarantined: true},
61+
})
62+
63+
// One pending tool (should be promoted).
64+
require.NoError(t, rt.storageManager.SaveToolApproval(&storage.ToolApprovalRecord{
65+
ServerName: "github",
66+
ToolName: "create_issue",
67+
CurrentHash: "hash-pending",
68+
Status: storage.ToolApprovalStatusPending,
69+
CurrentDescription: "Creates issues",
70+
CurrentSchema: `{}`,
71+
}))
72+
73+
// One changed (rug-pull) tool (must remain changed).
74+
require.NoError(t, rt.storageManager.SaveToolApproval(&storage.ToolApprovalRecord{
75+
ServerName: "github",
76+
ToolName: "list_repos",
77+
ApprovedHash: "hash-approved-old",
78+
CurrentHash: "hash-changed-new",
79+
Status: storage.ToolApprovalStatusChanged,
80+
CurrentDescription: "MALICIOUS: exfiltrate secrets",
81+
PreviousDescription: "Lists repos",
82+
CurrentSchema: `{}`,
83+
}))
84+
85+
require.NoError(t, rt.approveBaselineToolsForServer("github"))
86+
87+
pending, err := rt.storageManager.GetToolApproval("github", "create_issue")
88+
require.NoError(t, err)
89+
assert.Equal(t, storage.ToolApprovalStatusApproved, pending.Status,
90+
"pending tool must be promoted to approved")
91+
92+
changed, err := rt.storageManager.GetToolApproval("github", "list_repos")
93+
require.NoError(t, err)
94+
assert.Equal(t, storage.ToolApprovalStatusChanged, changed.Status,
95+
"changed (rug-pull) tool must NOT be silently cleared by baseline approval")
96+
assert.Equal(t, "hash-approved-old", changed.ApprovedHash,
97+
"changed tool's approved hash must be left untouched")
98+
}
99+
100+
// TestQuarantineServer_Unquarantine_BaselineApprovesPending exercises the
101+
// end-to-end path the user actually triggers: unquarantining a server promotes
102+
// its pending tools to approved (baseline trust) while leaving changed records
103+
// blocked.
104+
func TestQuarantineServer_Unquarantine_BaselineApprovesPending(t *testing.T) {
105+
tmpDir := t.TempDir()
106+
cfgPath := filepath.Join(tmpDir, "mcp_config.json")
107+
108+
cfg := config.DefaultConfig()
109+
cfg.Listen = "127.0.0.1:0"
110+
cfg.DataDir = tmpDir
111+
cfg.Servers = []*config.ServerConfig{
112+
{
113+
Name: "github",
114+
Command: "this-command-does-not-exist",
115+
Protocol: "stdio",
116+
Enabled: true,
117+
Quarantined: true,
118+
},
119+
}
120+
require.NoError(t, config.SaveConfig(cfg, cfgPath))
121+
122+
rt, err := New(cfg, cfgPath, zap.NewNop())
123+
require.NoError(t, err)
124+
t.Cleanup(func() { _ = rt.Close() })
125+
126+
// Persist the server so QuarantineServer's storage lookups succeed.
127+
require.NoError(t, rt.storageManager.SaveUpstreamServer(cfg.Servers[0]))
128+
129+
// Seed two pending tools and one changed (rug-pull) tool.
130+
require.NoError(t, rt.storageManager.SaveToolApproval(&storage.ToolApprovalRecord{
131+
ServerName: "github", ToolName: "create_issue",
132+
CurrentHash: "h1", Status: storage.ToolApprovalStatusPending,
133+
CurrentDescription: "Creates issues", CurrentSchema: `{}`,
134+
}))
135+
require.NoError(t, rt.storageManager.SaveToolApproval(&storage.ToolApprovalRecord{
136+
ServerName: "github", ToolName: "search_code",
137+
CurrentHash: "h2", Status: storage.ToolApprovalStatusPending,
138+
CurrentDescription: "Searches code", CurrentSchema: `{}`,
139+
}))
140+
require.NoError(t, rt.storageManager.SaveToolApproval(&storage.ToolApprovalRecord{
141+
ServerName: "github", ToolName: "list_repos",
142+
ApprovedHash: "old", CurrentHash: "new",
143+
Status: storage.ToolApprovalStatusChanged,
144+
CurrentDescription: "MALICIOUS", PreviousDescription: "Lists repos", CurrentSchema: `{}`,
145+
}))
146+
147+
// Unquarantine via the real entrypoint.
148+
require.NoError(t, rt.QuarantineServer("github", false))
149+
150+
for _, name := range []string{"create_issue", "search_code"} {
151+
rec, err := rt.storageManager.GetToolApproval("github", name)
152+
require.NoError(t, err)
153+
assert.Equal(t, storage.ToolApprovalStatusApproved, rec.Status,
154+
"pending tool %s must be baseline-approved on unquarantine", name)
155+
assert.Equal(t, "system:server-approval-baseline", rec.ApprovedBy)
156+
}
157+
158+
changed, err := rt.storageManager.GetToolApproval("github", "list_repos")
159+
require.NoError(t, err)
160+
assert.Equal(t, storage.ToolApprovalStatusChanged, changed.Status,
161+
"changed (rug-pull) tool must stay blocked after server unquarantine")
162+
}

0 commit comments

Comments
 (0)