diff --git a/docs/features/security-scanner-plugins.md b/docs/features/security-scanner-plugins.md index 91364e44..10576f90 100644 --- a/docs/features/security-scanner-plugins.md +++ b/docs/features/security-scanner-plugins.md @@ -317,7 +317,7 @@ The Security page at `/security` in the Web UI mirrors the CLI and provides: ## Known limitations - **Ramparts on arm64 macOS** — the upstream scanner image ships a binary linked against a newer GLIBC than the image base and fails every run on arm64. Track the [scanner-ramparts image rebuild](https://github.com/smart-mcp-proxy/mcpproxy-go/issues) for a fix. Other 6 of 7 scanners work out of the box on arm64 macOS. -- **Cisco scanner output has a hardcoded `server_url`** header in its stdout (`https://mcp.deepwiki.com/mcp`). This is cosmetic and does not affect findings. Since #383, mcpproxy strips this line from the user-visible execution log and replaces it with an annotation explaining no network request was made. +- **Cisco scanner is static-analysis only — coverage caveat.** The bundled `cisco-mcp-scanner` runs `static --tools tools.json` (YARA + readiness rules over the exported tool definitions). It **never connects to or probes the live server endpoint and makes no network request**, so an `is_safe`/`SAFE` result reflects the analyzed tool definitions, not the server's live runtime behavior — do not read a clean Cisco result for a remote/URL server as proof the live endpoint was exercised. mcpproxy prepends this caveat to every Cisco execution log. Relatedly, the upstream tool hardcodes a placeholder `server_url` header (`https://mcp.deepwiki.com/mcp`) in its stdout; this is cosmetic and does not affect findings, and since #383 mcpproxy strips that line from the user-visible execution log and replaces it with an annotation explaining no network request was made. - **Pass 2 (supply-chain audit)** currently requires Docker isolation to be enabled, otherwise it fails source resolution. The UI doesn't yet surface this precondition. ## Related reading diff --git a/internal/security/scanner/engine.go b/internal/security/scanner/engine.go index e64fd595..64c18333 100644 --- a/internal/security/scanner/engine.go +++ b/internal/security/scanner/engine.go @@ -1073,19 +1073,34 @@ var ciscoServerURLPattern = regexp.MustCompile( `(?m)^[ \t]*"server_url"[ \t]*:[ \t]*"https?://[^"]*deepwiki[^"]*"[ \t]*,?[ \t]*\r?\n?`, ) -// sanitizeCiscoStdout replaces the upstream-hardcoded deepwiki placeholder -// URL line with a short annotation referencing the tracking issue. The rest -// of the raw output is preserved for debugging. +// ciscoCoverageCaveat is prepended to every cisco-mcp-scanner execution log so +// the scanner's coverage is never over-trusted. The bundled cisco scanner runs +// `static --tools tools.json` (registry_bundled.go): it analyzes the exported +// tool definitions with YARA + readiness rules and never connects to the live +// server endpoint, so an is_safe/SAFE result reflects the tool definitions, not +// the server's runtime behavior. Surfacing this unconditionally (not only when +// the upstream deepwiki placeholder happens to appear) keeps the caveat honest +// even if a future upstream release changes its raw output. See issue #383 / MCP-2399. +const ciscoCoverageCaveat = "// [mcpproxy] coverage caveat: cisco-mcp-scanner performs STATIC analysis of " + + "the exported tool definitions only. It does not connect to or probe the live server endpoint and " + + "makes no network request, so a \"safe\"/is_safe result reflects the analyzed tool definitions, not " + + "the server's live runtime behavior (see issue #383).\n" + +// sanitizeCiscoStdout prepends a static-analysis coverage caveat and replaces +// the upstream-hardcoded deepwiki placeholder URL line with a short annotation +// referencing the tracking issue. The rest of the raw output is preserved for +// debugging. The caveat leads the output so it survives MaxLogBytes truncation. // // Assumes pretty-printed multi-line output; minified single-line JSON // bypasses this filter (acceptable since the cisco scanner emits multi-line). func sanitizeCiscoStdout(stdout string) string { - if !strings.Contains(stdout, "deepwiki") { - return stdout + sanitized := stdout + if strings.Contains(sanitized, "deepwiki") { + sanitized = ciscoServerURLPattern.ReplaceAllString( + sanitized, + "// [mcpproxy] upstream cisco-ai-mcp-scanner emits a hardcoded "+ + "placeholder server_url; no network request was made (see issue #383)\n", + ) } - return ciscoServerURLPattern.ReplaceAllString( - stdout, - "// [mcpproxy] upstream cisco-ai-mcp-scanner emits a hardcoded "+ - "placeholder server_url; no network request was made (see issue #383)\n", - ) + return ciscoCoverageCaveat + sanitized } diff --git a/internal/security/scanner/engine_test.go b/internal/security/scanner/engine_test.go index 11529763..bc42fa3a 100644 --- a/internal/security/scanner/engine_test.go +++ b/internal/security/scanner/engine_test.go @@ -841,10 +841,43 @@ func TestSanitizeCiscoStdout_PreservesScanResults(t *testing.T) { } } -func TestSanitizeCiscoStdout_NoOpWithoutDeepwiki(t *testing.T) { +// TestSanitizeCiscoStdout_SurfacesCoverageCaveatWithoutDeepwiki asserts the +// static-analysis coverage caveat is surfaced even when the upstream output +// carries no deepwiki placeholder line. The caveat must not depend on the +// upstream placeholder string, which a future cisco-ai-mcp-scanner release may +// change or drop — the static-only limitation is permanent regardless. See MCP-2399. +func TestSanitizeCiscoStdout_SurfacesCoverageCaveatWithoutDeepwiki(t *testing.T) { in := `{"scan_results": [{"tool_name": "x", "is_safe": true}]}` - if got := sanitizeCiscoStdout(in); got != in { - t.Errorf("expected no-op, got diff: %s", got) + out := sanitizeCiscoStdout(in) + if !strings.Contains(out, ciscoCoverageCaveat) { + t.Errorf("expected static-analysis coverage caveat to be surfaced, got:\n%s", out) + } + // Original payload must be preserved verbatim after the caveat header. + if !strings.Contains(out, in) { + t.Errorf("expected original output preserved, got:\n%s", out) + } + // Caveat must state the key facts: static-only and no network request. + if !strings.Contains(ciscoCoverageCaveat, "static") && !strings.Contains(ciscoCoverageCaveat, "STATIC") { + t.Error("caveat must state the analysis is static") + } + if !strings.Contains(ciscoCoverageCaveat, "network request") { + t.Error("caveat must state no network request is made") + } +} + +// TestSanitizeCiscoStdout_CaveatAlwaysLeadsOutput asserts the caveat is the +// first thing in the sanitized output so it survives MaxLogBytes truncation. +func TestSanitizeCiscoStdout_CaveatAlwaysLeadsOutput(t *testing.T) { + withDeepwiki := `{ + "server_url": "https://mcp.deepwiki.com/mcp", + "scan_results": [{"tool_name": "x", "is_safe": true}] +}` + out := sanitizeCiscoStdout(withDeepwiki) + if !strings.HasPrefix(out, ciscoCoverageCaveat) { + t.Errorf("expected caveat to lead the output, got:\n%s", out) + } + if strings.Contains(out, "deepwiki") { + t.Errorf("expected deepwiki line still stripped, got:\n%s", out) } }