From 0ba623640ad394a8c23cfd4277f991699baa2da7 Mon Sep 17 00:00:00 2001 From: Algis Dumbris Date: Sun, 14 Jun 2026 17:16:10 +0300 Subject: [PATCH] fix(scanner): always surface cisco static-analysis coverage caveat MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The bundled cisco-mcp-scanner runs 'static --tools tools.json': it analyzes the exported tool definitions with YARA + readiness rules and never probes the live server endpoint or makes a network request. An is_safe/SAFE result therefore reflects the tool definitions, not the server's runtime behavior — so a clean Cisco result for a remote/URL server must not be over-trusted as live coverage. Previously the only caveat ('no network request was made') was emitted by sanitizeCiscoStdout solely when the upstream output happened to contain the hardcoded deepwiki placeholder line. If a future cisco-ai-mcp-scanner release changes or drops that placeholder, the caveat would silently vanish while the static-only limitation remains. Prepend a permanent coverage caveat to every Cisco execution log, independent of the deepwiki string, leading the output so it survives MaxLogBytes truncation. Still strip the deepwiki placeholder line when present. Update docs to frame the limitation as a coverage caveat rather than a purely cosmetic note. Resolves the residual coverage-honesty concern from gh #383 (cosmetic leak already fixed in #528). Related #383 --- docs/features/security-scanner-plugins.md | 2 +- internal/security/scanner/engine.go | 35 ++++++++++++++------ internal/security/scanner/engine_test.go | 39 +++++++++++++++++++++-- 3 files changed, 62 insertions(+), 14 deletions(-) diff --git a/docs/features/security-scanner-plugins.md b/docs/features/security-scanner-plugins.md index 91364e448..10576f90e 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 e64fd595e..64c18333e 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 115297632..bc42fa3ae 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) } }