diff --git a/CHANGELOG.md b/CHANGELOG.md index d17723c6e..3510dca44 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -217,6 +217,7 @@ Releases follow [Semantic Versioning](https://semver.org/). ### Bug Fixes +- **Core no longer burns a CPU core while idle on installs with many held tool changes (#1724).** The per-tool change-gate scan (`trust_mode: scan` approval, and the hold evidence in `manual`) fed every other connected server's tools to the detector as cross-server context and then ran all checks, phrase/regex detection included, over that whole fleet for each held tool, discarding the peer findings afterwards. With a few hundred tools and a few dozen held changes each 5-minute discovery pass re-scanned tools × fleet (one report: 471 tools, 108 held, ~11 minutes at 100% CPU per pass). The detector now inspects only the scanned server's tools; peers stay in the cross-server index so the shadowing and impersonation checks see the same context and the verdict for the scanned tool is unchanged. One held-tool scan against a 470-tool fleet went from ~3 s to ~5 ms. - **`serve --data-dir` (and `MCPPROXY_DATA_DIR`) now reads `/mcp_config.json`.** Until now `serve` looked for its config only in the working directory and `$HOME/.mcpproxy` and applied the data dir afterwards, so a config on a mounted volume was ignored and a fresh one (new listen address, new API key) was written under `$HOME`, while every management command already read the config from the data dir. `serve` now resolves it the same way: an explicit `--config` wins, then `/mcp_config.json` if it exists, and on a first run with no config anywhere the default is seeded inside the data dir. `MCPPROXY_DATA_DIR`, documented but never implemented, now works as an alias of `--data-dir` (the legacy `MCPPROXY_DATA` keeps relocating only the data directory, and loses when both are set). If you already export `MCPPROXY_DATA_DIR`, it takes effect for the first time on this upgrade: the data directory, and so the database, search index and socket, move to it. **Migration:** an instance started with `--data-dir X` that relied on the config under `$HOME/.mcpproxy` while a stale `X/mcp_config.json` also exists will now read the one in `X`. - **audit: a profile tool-policy refusal now names its rule (#1703).** The `authz` audit line for a call refused by a profile's tier cap, deny rule or unannotated-tool policy used to read `reason: other`; it now reads `profile_tier`, `profile_rule` or `profile_unannotated` (`disclosed: true`), matching the `block_reason` of the same call in the activity log. A refusal inside a `code_execution` script used to be written as `token_scope` with `disclosed: false` and now uses the same three reasons. `profile_scope` is unchanged, and `profile_code_execution` / `profile_management` are not audit reasons because those built-in tools are not audited per call. The `reason` enum in `docs/schemas/audit-line-v1.schema.json` gains the three members, so a consumer that validates audit lines against the enum must accept them. (refs #1703) diff --git a/internal/security/detect/scoped_view_test.go b/internal/security/detect/scoped_view_test.go new file mode 100644 index 000000000..1dde85593 --- /dev/null +++ b/internal/security/detect/scoped_view_test.go @@ -0,0 +1,94 @@ +package detect + +import ( + "reflect" + "sort" + "testing" +) + +// countingCheck records the (server, name) of every tool the engine hands it +// and consults the registry like Shadowing does (by ToolsByName). +type countingCheck struct { + id string + inspected *[]string +} + +func (c countingCheck) ID() string { return c.id } +func (c countingCheck) Inspect(t ToolView, reg RegistryView) []Signal { + *c.inspected = append(*c.inspected, t.Server+":"+t.Name) + for _, other := range reg.ToolsByName[t.Name] { + if other.Server != t.Server { + return []Signal{{CheckID: c.id, Tier: TierHard, ThreatType: ThreatToolPoisoning, Confidence: 0.9, Detail: "collides with " + other.Server}} + } + } + return nil +} + +// A scoped view inspects ONLY the target tools; peers are index context for +// the cross-tool checks (ToolsByName / ToolNames) and are never handed to a +// check themselves. This is what keeps a per-tool change-gate scan O(1 tool) +// instead of O(fleet): before it, every held tool rescanned the whole fleet +// with every check and threw the peer findings away. +func TestNewScopedRegistryView_InspectsTargetsOnly(t *testing.T) { + targets := []ToolView{ + {Server: "evil", Name: "send_email", Description: "Send mail."}, + {Server: "evil", Name: "list_pages", Description: "List pages."}, + } + peers := []ToolView{ + {Server: "mailer", Name: "send_email", Description: "Send an email to a recipient."}, + {Server: "docs", Name: "search_docs", Description: "Search the docs."}, + } + reg := NewScopedRegistryView(targets, peers) + + if len(reg.Tools) != 2 { + t.Fatalf("Tools = %d, want 2 (targets only)", len(reg.Tools)) + } + for _, tv := range reg.Tools { + if tv.NormalizedText == "" || tv.PhraseText == "" { + t.Errorf("target %s:%s not normalized", tv.Server, tv.Name) + } + } + if _, ok := reg.ToolNames["search_docs"]; !ok { + t.Error("peer name search_docs missing from ToolNames") + } + if got := len(reg.ToolsByName["send_email"]); got != 2 { + t.Errorf("ToolsByName[send_email] = %d entries, want 2 (target + peer)", got) + } + + var inspected []string + e := NewEngine(Options{Checks: []Check{countingCheck{id: "count", inspected: &inspected}}}) + res := e.Scan(reg) + + sort.Strings(inspected) + if want := []string{"evil:list_pages", "evil:send_email"}; !reflect.DeepEqual(inspected, want) { + t.Errorf("inspected = %v, want %v (peers must not be inspected)", inspected, want) + } + if len(res.Findings) != 1 || res.Findings[0].Location != "evil:send_email" { + t.Errorf("findings = %+v, want exactly the cross-server collision on evil:send_email", res.Findings) + } +} + +// Scoped and unscoped views agree on the target server's findings: scoping +// drops peer inspection, never target evidence. +func TestNewScopedRegistryView_TargetFindingsMatchFullView(t *testing.T) { + targets := []ToolView{{Server: "evil", Name: "send_email", Description: "Send mail."}} + peers := []ToolView{{Server: "mailer", Name: "send_email", Description: "Send an email."}} + var a, b []string + e := NewEngine(Options{Checks: []Check{countingCheck{id: "count", inspected: &a}}}) + full := e.Scan(NewRegistryView(append(append([]ToolView{}, targets...), peers...))) + e2 := NewEngine(Options{Checks: []Check{countingCheck{id: "count", inspected: &b}}}) + scoped := e2.Scan(NewScopedRegistryView(targets, peers)) + + var fullEvil []Finding + for _, f := range full.Findings { + if f.Location == "evil:send_email" { + fullEvil = append(fullEvil, f) + } + } + if !reflect.DeepEqual(fullEvil, scoped.Findings) { + t.Errorf("target findings differ:\n full=%+v\n scoped=%+v", fullEvil, scoped.Findings) + } + if len(a) != 2 || len(b) != 1 { + t.Errorf("inspections full=%d scoped=%d, want 2 and 1", len(a), len(b)) + } +} diff --git a/internal/security/detect/signal.go b/internal/security/detect/signal.go index 0d80c23b8..9ab410630 100644 --- a/internal/security/detect/signal.go +++ b/internal/security/detect/signal.go @@ -121,6 +121,38 @@ func NewRegistryView(tools []ToolView) RegistryView { return RegistryView{Tools: out, ToolsByName: byName, ToolNames: names} } +// NewScopedRegistryView builds a view whose scan set (Tools) is targets only, +// while the cross-tool indexes (ToolsByName, ToolNames) cover targets AND +// peers. The engine inspects only Tools, so a check never runs on a peer: +// peers are pure context for the cross-server checks (Shadowing reads the +// indexes, nothing reads a peer's normalized text). Only targets are +// normalized. Target findings are identical to those of a full +// NewRegistryView over targets+peers; what is dropped is the work (and the +// findings) on the peers themselves. This is the shape the per-tool change +// gate needs: one held tool against a fleet of hundreds must cost one tool's +// worth of checks, not the fleet's. +func NewScopedRegistryView(targets, peers []ToolView) RegistryView { + byName := make(map[string][]ToolView, len(targets)+len(peers)) + names := make(map[string]struct{}, len(targets)+len(peers)) + out := make([]ToolView, len(targets)) + for i, tv := range targets { + if tv.NormalizedText == "" { + tv.NormalizedText = Normalize(combinedText(tv)) + } + if tv.PhraseText == "" { + tv.PhraseText = NormalizeForPhrase(combinedText(tv)) + } + out[i] = tv + byName[tv.Name] = append(byName[tv.Name], tv) + names[tv.Name] = struct{}{} + } + for _, tv := range peers { + byName[tv.Name] = append(byName[tv.Name], tv) + names[tv.Name] = struct{}{} + } + return RegistryView{Tools: out, ToolsByName: byName, ToolNames: names} +} + // combinedText concatenates the description with the raw schema text so the // soft checks see parameter names/descriptions too. func combinedText(tv ToolView) string { diff --git a/internal/security/scanner/inprocess.go b/internal/security/scanner/inprocess.go index 88275a2da..4764d1df7 100644 --- a/internal/security/scanner/inprocess.go +++ b/internal/security/scanner/inprocess.go @@ -89,12 +89,13 @@ func ScanToolMetadataVerdict(serverName string, tools []*config.ToolMetadata, pe } } sort.Strings(peerNames) + var peerViews []detect.ToolView for _, name := range peerNames { for _, t := range peerTools[name] { if t == nil { continue } - views = append(views, toolView(name, toolDef{ + peerViews = append(peerViews, toolView(name, toolDef{ Name: t.Name, Description: t.Description, InputSchema: json.RawMessage(t.ParamsJSON), @@ -119,7 +120,13 @@ func ScanToolMetadataVerdict(serverName string, tools []*config.ToolMetadata, pe ScannerID: inProcessTPAScannerID, Checks: engineChecks, }) - result := engine.Scan(detect.NewRegistryView(views)) + // Scoped view: only serverName's tools are inspected; peers are index + // context for the cross-server checks. The change gate runs this once per + // held tool on every discovery pass, so inspecting the whole fleet each + // time (and discarding the peer findings below) is what pinned a core at + // 100% CPU on installs with a few hundred tools and a few dozen held + // changes. + result := engine.Scan(detect.NewScopedRegistryView(views, peerViews)) prefix := serverName + ":" out := make([]ScanFinding, 0, len(result.Findings)) @@ -214,9 +221,10 @@ func detectEngineFindings(tools []toolDef, serverName string, peerTools map[stri } } sort.Strings(peerNames) + var peerViews []detect.ToolView for _, name := range peerNames { for _, t := range peerTools[name] { - views = append(views, toolView(name, t)) + peerViews = append(peerViews, toolView(name, t)) } } @@ -234,7 +242,9 @@ func detectEngineFindings(tools []toolDef, serverName string, peerTools map[stri ScannerID: scannerID, Checks: engineChecks, }) - result := engine.Scan(detect.NewRegistryView(views)) + // Peers are index context only (see NewScopedRegistryView): their own + // findings were never reported from here. + result := engine.Scan(detect.NewScopedRegistryView(views, peerViews)) prefix := serverName + ":" out := make([]ScanFinding, 0, len(result.Findings)) diff --git a/internal/security/scanner/inprocess_scope_bench_test.go b/internal/security/scanner/inprocess_scope_bench_test.go new file mode 100644 index 000000000..660079efb --- /dev/null +++ b/internal/security/scanner/inprocess_scope_bench_test.go @@ -0,0 +1,58 @@ +package scanner + +import ( + "fmt" + "testing" + + "github.com/smart-mcp-proxy/mcpproxy-go/internal/config" +) + +// benchPeerFleet builds a fleet shaped like the install behind the "high CPU +// while idle" report: 11 servers, ~471 tools, realistic descriptions and +// schemas. The first server is the one whose changed tools are scanned. +func benchPeerFleet(servers, toolsPerServer int) (string, []*config.ToolMetadata, map[string][]*config.ToolMetadata) { + mk := func(server string, i int) *config.ToolMetadata { + return &config.ToolMetadata{ + ServerName: server, + Name: fmt.Sprintf("%s_tool_%d", server, i), + RawName: fmt.Sprintf("%s_tool_%d", server, i), + Description: fmt.Sprintf("Retrieve, list or update %s records number %d. Returns a paginated list of items with their identifiers, titles, "+ + "owners, timestamps and status. Use the cursor parameter to fetch the next page. Supports filtering by workspace, "+ + "project and label; sorting by created or updated time.", server, i), + ParamsJSON: fmt.Sprintf(`{"type":"object","properties":{"query":{"type":"string","description":"Free-text search for %s %d"},`+ + `"cursor":{"type":"string","description":"Pagination cursor from the previous response"},`+ + `"limit":{"type":"integer","minimum":1,"maximum":100,"default":25},`+ + `"filter":{"type":"object","properties":{"workspace":{"type":"string"},"project":{"type":"string"},"labels":{"type":"array","items":{"type":"string"}}}},`+ + `"sort":{"type":"string","enum":["created_asc","created_desc","updated_asc","updated_desc"]}},"required":["query"]}`, server, i), + OutputSchemaJSON: `{"type":"object","properties":{"items":{"type":"array","items":{"type":"object","properties":{"id":{"type":"string"},"title":{"type":"string"},"status":{"type":"string"}}}},"next_cursor":{"type":"string"}}}`, + } + } + target := "notion" + var targetTools []*config.ToolMetadata + for i := 0; i < toolsPerServer; i++ { + targetTools = append(targetTools, mk(target, i)) + } + peers := make(map[string][]*config.ToolMetadata) + for s := 1; s < servers; s++ { + name := fmt.Sprintf("peer%d", s) + for i := 0; i < toolsPerServer; i++ { + peers[name] = append(peers[name], mk(name, i)) + } + } + return target, targetTools, peers +} + +// BenchmarkScanToolMetadataVerdict_PerToolWithPeers is one discovery-pass scan +// of a single held tool with the full peer fleet as context, which the runtime +// repeats once per held tool on every pass. +func BenchmarkScanToolMetadataVerdict_PerToolWithPeers(b *testing.B) { + target, tools, peers := benchPeerFleet(11, 43) + b.ReportAllocs() + b.ResetTimer() + for i := 0; i < b.N; i++ { + verdict, _, ok := ScanToolMetadataVerdict(target, tools[i%len(tools):i%len(tools)+1], peers) + if verdict != "clean" || !ok { + b.Fatalf("verdict=%s ok=%v", verdict, ok) + } + } +}