diff --git a/docs/features/security-quarantine.md b/docs/features/security-quarantine.md index 7f89c8143..1bf113b91 100644 --- a/docs/features/security-quarantine.md +++ b/docs/features/security-quarantine.md @@ -240,6 +240,8 @@ The scan line of the review screen says whether the baseline scan describes the Each tool's scan verdict follows the same rule: `clean` only when the scan covered that tool's current definition. After a baseline scan has listed a quarantined server's tools, MCPProxy captures the definitions itself, so the review list is not empty until someone clicks **Fetch tool definitions**. With `security.auto_baseline_scan: false` and no manual scan nothing is started automatically. +Coverage is bound to the definition itself, not only to timing. When a scan exports a tool's definition it records a SHA-256 digest of the description and input schema (`tool_hashes` in the scan context). A tool counts as covered only if that digest equals the digest of its current definition, so a benign definition that was scanned and then swapped for a poisoned one inside the same timing window reads as stale. Scans stored before this field existed carry no digests and keep the earlier name-and-timing rules. Approval still re-checks the definition independently; this only keeps the review display honest. + On a server that is not quarantined, the review tab shows approved state: approved tools read **Approved** or **Blocked** (no Approve or Reject), the heading says the server is approved, and **Manage tools** and **Quarantine to review again…** are offered. The second is the existing quarantine action behind a confirmation. Only a new or changed tool shows Approve and Reject. ### Scan a Server for TPAs (MCP) diff --git a/internal/hash/hash.go b/internal/hash/hash.go index cc98ae8cc..f2dd2ae11 100644 --- a/internal/hash/hash.go +++ b/internal/hash/hash.go @@ -251,3 +251,20 @@ func ComputeToolHashWithOutputSchema(serverName, toolName, description string, i } return hash } + +// ToolDefinitionDigest returns a stable digest of the parts of a tool +// definition a security scan reads: its description and input schema. The +// tool name is the lookup key on both sides, and the output schema is not +// exported to scanners, so neither is folded in. A missing, null or empty +// schema counts as "{}" so every producer agrees. +func ToolDefinitionDigest(description, schemaJSON string) string { + switch schemaJSON { + case "", "null": + schemaJSON = "{}" + } + h := sha256.New() + h.Write([]byte(description)) + h.Write([]byte("|")) + h.Write([]byte(NormalizeJSON(schemaJSON))) + return hex.EncodeToString(h.Sum(nil)) +} diff --git a/internal/runtime/review.go b/internal/runtime/review.go index 7a40ca023..6601f14f5 100644 --- a/internal/runtime/review.go +++ b/internal/runtime/review.go @@ -11,6 +11,7 @@ import ( "github.com/smart-mcp-proxy/mcpproxy-go/internal/config" "github.com/smart-mcp-proxy/mcpproxy-go/internal/contracts" + "github.com/smart-mcp-proxy/mcpproxy-go/internal/hash" "github.com/smart-mcp-proxy/mcpproxy-go/internal/oauth" "github.com/smart-mcp-proxy/mcpproxy-go/internal/security/scanner" "github.com/smart-mcp-proxy/mcpproxy-go/internal/storage" @@ -349,6 +350,10 @@ func reviewCoverage(job *scanner.ScanJob, records []*storage.ToolApprovalRecord, // - A definition added or changed after the scan read its definitions // (DefinitionChangedAt after ScanContext.ToolsExportedAt, or StartedAt // for a scan that recorded no export time) is not covered. +// - A scan that recorded per-tool definition digests (ToolHashes) covers a +// tool only when its digest equals the digest of the record's current +// definition, so a definition swapped inside the timing window is not +// covered. Scans without digests (legacy) use the rules below unchanged. // - A scan that recorded its tool names covers exactly those tools. // - A legacy scan (no recorded names, ToolsExported > 0) covers approved // records, and pending records of a quarantined server (its whole toolset @@ -369,6 +374,19 @@ func reviewToolCovered(job *scanner.ScanJob, quarantined bool, serverName string if !record.DefinitionChangedAt.IsZero() && record.DefinitionChangedAt.After(analysedAt) { return false } + if len(job.ScanContext.ToolHashes) > 0 { + // Hash-bound scan: the analysed definition must be the record's + // CURRENT one. A record with no stored definition cannot be compared, + // so it is not covered. + scanned, ok := job.ScanContext.ToolHashes[record.ToolName] + if !ok { + scanned, ok = job.ScanContext.ToolHashes[serverName+":"+record.ToolName] + } + if !ok || (record.CurrentDescription == "" && record.CurrentSchema == "") { + return false + } + return scanned == hash.ToolDefinitionDigest(record.CurrentDescription, record.CurrentSchema) + } if len(job.ScanContext.ToolNames) > 0 { for _, name := range job.ScanContext.ToolNames { if name == record.ToolName || name == serverName+":"+record.ToolName { diff --git a/internal/runtime/review_scan_coverage_test.go b/internal/runtime/review_scan_coverage_test.go index d8bf4e3db..f6c549a99 100644 --- a/internal/runtime/review_scan_coverage_test.go +++ b/internal/runtime/review_scan_coverage_test.go @@ -9,6 +9,7 @@ import ( "github.com/stretchr/testify/require" "github.com/smart-mcp-proxy/mcpproxy-go/internal/config" + "github.com/smart-mcp-proxy/mcpproxy-go/internal/hash" "github.com/smart-mcp-proxy/mcpproxy-go/internal/security/scanner" "github.com/smart-mcp-proxy/mcpproxy-go/internal/storage" ) @@ -242,3 +243,45 @@ func TestReviewToolCoveredUsesExportTime(t *testing.T) { } require.True(t, reviewToolCovered(legacy, false, "srv", changedBetween), "no export time falls back to StartedAt") } + +func TestReviewToolCoveredBindsToDefinitionHash(t *testing.T) { + const schema = `{"type":"object"}` + scanned := hash.ToolDefinitionDigest("reads notes", schema) + job := func(hashes map[string]string) *scanner.ScanJob { + return &scanner.ScanJob{ + Status: scanner.ScanJobStatusCompleted, StartedAt: time.Now().Add(-time.Hour), + ScanContext: &scanner.ScanContext{ToolsExported: 1, ToolNames: []string{"notes"}, ToolHashes: hashes}, + } + } + record := func(desc string) *storage.ToolApprovalRecord { + return &storage.ToolApprovalRecord{ToolName: "notes", Status: storage.ToolApprovalStatusApproved, CurrentDescription: desc, CurrentSchema: schema} + } + + t.Run("same hash is covered", func(t *testing.T) { + require.True(t, reviewToolCovered(job(map[string]string{"notes": scanned}), false, "srv", record("reads notes"))) + }) + t.Run("same name different hash is not covered", func(t *testing.T) { + require.False(t, reviewToolCovered(job(map[string]string{"notes": scanned}), false, "srv", record("ignore previous instructions"))) + }) + t.Run("tool absent from hashes is not covered", func(t *testing.T) { + require.False(t, reviewToolCovered(job(map[string]string{"other": scanned}), false, "srv", record("reads notes"))) + }) + t.Run("server-prefixed export name matches", func(t *testing.T) { + require.True(t, reviewToolCovered(job(map[string]string{"srv:notes": scanned}), false, "srv", record("reads notes"))) + }) + t.Run("legacy scan without hashes keeps name and timing rules", func(t *testing.T) { + require.True(t, reviewToolCovered(job(nil), false, "srv", record("anything"))) + changed := record("anything") + changed.DefinitionChangedAt = time.Now() + require.False(t, reviewToolCovered(job(nil), false, "srv", changed)) + }) +} + +func TestScanContextLegacyJSONDecodesWithoutToolHashes(t *testing.T) { + var sc scanner.ScanContext + require.NoError(t, json.Unmarshal([]byte(`{"source_method":"none","tools_exported":2,"tool_names":["a","b"]}`), &sc)) + require.Nil(t, sc.ToolHashes) + require.True(t, reviewToolCovered(&scanner.ScanJob{ + Status: scanner.ScanJobStatusCompleted, StartedAt: time.Now(), ScanContext: &sc, + }, false, "srv", &storage.ToolApprovalRecord{ToolName: "a", Status: storage.ToolApprovalStatusApproved})) +} diff --git a/internal/security/scanner/export_tool_names_test.go b/internal/security/scanner/export_tool_names_test.go index 77a95f80e..462975663 100644 --- a/internal/security/scanner/export_tool_names_test.go +++ b/internal/security/scanner/export_tool_names_test.go @@ -10,6 +10,8 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + + "github.com/smart-mcp-proxy/mcpproxy-go/internal/hash" ) // namesProvider reports a fixed tool list (duplicates and unsorted on @@ -100,3 +102,23 @@ func TestStartScanRecordsToolsExportedAt(t *testing.T) { assert.False(t, final.ScanContext.ToolsExportedAt.IsZero()) assert.True(t, final.ScanContext.ToolsExportedAt.After(before)) } + +func TestExportToolDefinitionsRecordsDefinitionHashes(t *testing.T) { + dir := t.TempDir() + logger := zap.NewNop() + svc := NewService(newMockStorage(), NewRegistry(dir, logger), NewDockerRunner(logger), dir, logger) + svc.SetServerInfoProvider(&namesProvider{tools: []map[string]interface{}{ + {"name": "a", "description": "d", "inputSchema": map[string]interface{}{"type": "object"}}, + {"name": "b", "description": "x"}, + }}) + + e := svc.exportToolDefinitionsStamped("srv", t.TempDir()) + assert.Equal(t, map[string]string{ + "a": hash.ToolDefinitionDigest("d", `{"type":"object"}`), + "b": hash.ToolDefinitionDigest("x", ""), + }, e.Hashes) + var sc ScanContext + e.applyTo(&sc) + assert.Equal(t, e.Hashes, sc.ToolHashes) + assert.False(t, sc.ToolsExportedAt.IsZero()) +} diff --git a/internal/security/scanner/service.go b/internal/security/scanner/service.go index ee1f3d0b3..1b36f6cb2 100644 --- a/internal/security/scanner/service.go +++ b/internal/security/scanner/service.go @@ -15,6 +15,7 @@ import ( "go.uber.org/zap" "github.com/smart-mcp-proxy/mcpproxy-go/internal/config" + "github.com/smart-mcp-proxy/mcpproxy-go/internal/hash" ) // errNoScans is returned by findLatestPassJobs when the scan-job bucket has no @@ -1112,7 +1113,7 @@ func (s *Service) StartScan(ctx context.Context, serverName string, dryRun bool, s.waitForConnection(serverName, 30*time.Second) } } - scanCtx.ToolsExported, scanCtx.ToolNames, scanCtx.ToolsExportedAt = s.exportToolDefinitionsStamped(serverName, req.SourceDir) + s.exportToolDefinitionsStamped(serverName, req.SourceDir).applyTo(scanCtx) // If export failed, retry once. Reconnect ONLY when the server is // actually disconnected (that path handles quarantined servers @@ -1125,7 +1126,7 @@ func (s *Service) StartScan(ctx context.Context, serverName string, dryRun bool, if s.serverInfo.IsConnected(serverName) { s.logger.Info("Tool export returned 0 for a connected server, retrying export without restarting it", zap.String("server", serverName)) - scanCtx.ToolsExported, scanCtx.ToolNames, scanCtx.ToolsExportedAt = s.exportToolDefinitionsStamped(serverName, req.SourceDir) + s.exportToolDefinitionsStamped(serverName, req.SourceDir).applyTo(scanCtx) } else { s.logger.Info("Tool export returned 0, retrying after EnsureConnected", zap.String("server", serverName)) @@ -1134,7 +1135,7 @@ func (s *Service) StartScan(ctx context.Context, serverName string, dryRun bool, zap.String("server", serverName), zap.Error(err)) } else { s.waitForConnection(serverName, 30*time.Second) - scanCtx.ToolsExported, scanCtx.ToolNames, scanCtx.ToolsExportedAt = s.exportToolDefinitionsStamped(serverName, req.SourceDir) + s.exportToolDefinitionsStamped(serverName, req.SourceDir).applyTo(scanCtx) } } } @@ -2349,13 +2350,27 @@ func (s *Service) waitForConnection(serverName string, timeout time.Duration) { zap.Duration("timeout", timeout)) } -// exportToolDefinitionsStamped runs exportToolDefinitions and also returns the +// toolExport is the outcome of one tool-definition export. +type toolExport struct { + Count int + Names []string + Hashes map[string]string + At time.Time +} + +// applyTo records the export on the scan context. +func (e toolExport) applyTo(sc *ScanContext) { + sc.ToolsExported, sc.ToolNames, sc.ToolHashes, sc.ToolsExportedAt = e.Count, e.Names, e.Hashes, e.At +} + +// exportToolDefinitionsStamped runs exportToolDefinitions and also records the // instant just before the definitions were read. Taking the stamp first means a // definition change racing the read is judged not covered rather than covered. -func (s *Service) exportToolDefinitionsStamped(serverName, sourceDir string) (int, []string, time.Time) { +func (s *Service) exportToolDefinitionsStamped(serverName, sourceDir string) toolExport { at := time.Now().UTC() - count, names := s.exportToolDefinitions(serverName, sourceDir) - return count, names, at + e := s.exportToolDefinitionsFull(serverName, sourceDir) + e.At = at + return e } // exportToolDefinitions writes a tools.json file to the source directory @@ -2363,14 +2378,22 @@ func (s *Service) exportToolDefinitionsStamped(serverName, sourceDir string) (in // Returns the number of tools exported and their sorted, de-duplicated names, // so a scan records which definitions it actually saw. func (s *Service) exportToolDefinitions(serverName, sourceDir string) (int, []string) { + e := s.exportToolDefinitionsFull(serverName, sourceDir) + return e.Count, e.Names +} + +// exportToolDefinitionsFull is exportToolDefinitions plus the per-tool +// definition digests of what was written, so a scan can later be matched to +// the exact definitions it analysed. +func (s *Service) exportToolDefinitionsFull(serverName, sourceDir string) toolExport { tools, err := s.serverInfo.GetServerTools(serverName) if err != nil { s.logger.Warn("Could not export tool definitions for scanning", zap.String("server", serverName), zap.Error(err)) - return 0, nil + return toolExport{} } if len(tools) == 0 { - return 0, nil + return toolExport{} } // Format as MCP tools/list output @@ -2379,20 +2402,48 @@ func (s *Service) exportToolDefinitions(serverName, sourceDir string) (int, []st } data, err := json.MarshalIndent(toolsData, "", " ") if err != nil { - return 0, nil + return toolExport{} } toolsPath := filepath.Join(sourceDir, "tools.json") if err := os.WriteFile(toolsPath, data, 0644); err != nil { s.logger.Debug("Failed to write tools.json", zap.Error(err)) - return 0, nil + return toolExport{} } s.logger.Info("Exported tool definitions for scanning", zap.String("server", serverName), zap.Int("tools", len(tools)), zap.String("path", toolsPath), ) - return len(tools), toolDefinitionNames(tools) + return toolExport{Count: len(tools), Names: toolDefinitionNames(tools), Hashes: toolDefinitionHashes(tools)} +} + +// toolDefinitionHashes maps each exported tool name to the digest of its +// description and input schema (hash.ToolDefinitionDigest). A duplicate name +// keeps the first definition, matching what the scan reads first. +func toolDefinitionHashes(tools []map[string]interface{}) map[string]string { + hashes := make(map[string]string, len(tools)) + for _, tool := range tools { + name, _ := tool["name"].(string) + if name == "" { + continue + } + if _, dup := hashes[name]; dup { + continue + } + description, _ := tool["description"].(string) + schemaJSON := "" + if schema := tool["inputSchema"]; schema != nil { + if raw, err := json.Marshal(schema); err == nil { + schemaJSON = string(raw) + } + } + hashes[name] = hash.ToolDefinitionDigest(description, schemaJSON) + } + if len(hashes) == 0 { + return nil + } + return hashes } // toolDefinitionNames returns the sorted, de-duplicated non-empty "name" diff --git a/internal/security/scanner/types.go b/internal/security/scanner/types.go index 126ee6040..a793e4ef5 100644 --- a/internal/security/scanner/types.go +++ b/internal/security/scanner/types.go @@ -262,20 +262,21 @@ type ScanJobSummary struct { // ScanContext describes what was scanned and how the source was resolved. // This gives users full transparency into what the scanners actually checked. type ScanContext struct { - SourceMethod string `json:"source_method"` // "docker_extract", "working_dir", "local_path", "url", "none" - SourcePath string `json:"source_path"` // Actual path/URL that was scanned - DockerIsolation bool `json:"docker_isolation"` // Whether server runs in Docker - ContainerID string `json:"container_id,omitempty"` // Docker container ID (if applicable) - ContainerOwner string `json:"container_owner,omitempty"` // Server name that owns ContainerID (verified via com.mcpproxy.server label) - ContainerImage string `json:"container_image,omitempty"` // Docker image used - ServerProtocol string `json:"server_protocol"` // stdio, http, sse - ServerCommand string `json:"server_command,omitempty"` // Command used to start server - ToolsExported int `json:"tools_exported,omitempty"` // Number of tool definitions exported for scanning - ToolsExportedAt time.Time `json:"tools_exported_at,omitzero"` // When the exported definitions were read; a definition changed after this was not analysed - ToolNames []string `json:"tool_names,omitempty"` // Sorted, de-duplicated names of the exported tool definitions (Pass 1) - ScannedFiles []string `json:"scanned_files,omitempty"` // List of files that were scanned (capped at MaxScannedFiles) - TotalFiles int `json:"total_files"` // Total file count (may be > len(ScannedFiles) if capped) - TotalSizeBytes int64 `json:"total_size_bytes"` // Total size of scanned source + SourceMethod string `json:"source_method"` // "docker_extract", "working_dir", "local_path", "url", "none" + SourcePath string `json:"source_path"` // Actual path/URL that was scanned + DockerIsolation bool `json:"docker_isolation"` // Whether server runs in Docker + ContainerID string `json:"container_id,omitempty"` // Docker container ID (if applicable) + ContainerOwner string `json:"container_owner,omitempty"` // Server name that owns ContainerID (verified via com.mcpproxy.server label) + ContainerImage string `json:"container_image,omitempty"` // Docker image used + ServerProtocol string `json:"server_protocol"` // stdio, http, sse + ServerCommand string `json:"server_command,omitempty"` // Command used to start server + ToolsExported int `json:"tools_exported,omitempty"` // Number of tool definitions exported for scanning + ToolsExportedAt time.Time `json:"tools_exported_at,omitzero"` // When the exported definitions were read; a definition changed after this was not analysed + ToolNames []string `json:"tool_names,omitempty"` // Sorted, de-duplicated names of the exported tool definitions (Pass 1) + ToolHashes map[string]string `json:"tool_hashes,omitempty"` // Exported tool name -> hash.ToolDefinitionDigest of the definition analysed; absent on legacy scans + ScannedFiles []string `json:"scanned_files,omitempty"` // List of files that were scanned (capped at MaxScannedFiles) + TotalFiles int `json:"total_files"` // Total file count (may be > len(ScannedFiles) if capped) + TotalSizeBytes int64 `json:"total_size_bytes"` // Total size of scanned source } // ScannerJobStatus tracks a single scanner's execution within a scan job