Skip to content

Commit 8a8743c

Browse files
committed
fix(039): resolve 13 QA findings in security scanner plugin system
Bundled fix for bugs surfaced by a full QA pass of Spec 039. Each finding has a one-line code reference and a regression test where applicable. Critical: * F-01 security approve now actually unquarantines the server. The scanner Service gets a new ServerUnquarantiner dependency injected from server.Server.UnquarantineServer; ApproveServer calls it after persisting the integrity baseline. Tool indexing and quarantine-bucket removal follow the existing unquarantine path. Critical-findings guard still blocks the call before anything mutates. TestServiceApproveServerCallsUnquarantiner and TestServiceApproveServerCriticalDoesNotUnquarantine pin the contract. * F-02 Source resolver no longer misreads a server's positional data-dir argument as source code. For package-runner commands (npx, uvx, pipx, pnpm dlx, bunx, yarn dlx) resolveFromPackageCache runs first; arg-scan fallback only accepts directories containing a source marker (package.json, pyproject.toml, setup.py, Cargo.toml, go.mod, or a source file). Regression tests cover the filesystem-server false-positive case and the legitimate python-script arg case. High: * F-03 macOS keychain probe no longer pops the "Keychain Not Found" modal. KeyringProvider.IsAvailable uses a read-only Get probe with a 2s timeout and a headless-environment fast path instead of keyring.Set. Public Get, Store, Delete, List, and both registry helpers are wrapped in a new runWithTimeout helper (3s hard deadline) backed by ErrKeyringTimeout. Scanner ConfigureScanner already falls back to in-config storage on Store errors. TestKeyringProvider_IsAvailable_HangingBackend proves the bail-out. * F-04 UI Approve buttons are now scanner-aware. ServerCard, ServerDetail and ScanReport call the new securityApproveServer store action. A custom modal gates force-approve on servers with no scan or with critical findings; the legacy unquarantineServer path is retained only for admin and the Scan First action. * F-05 CLI security scan no longer hangs after the job reports completed. The poll loop now terminates on completed|failed|cancelled and prints live progress lines (N run, M running, F failed). Medium: * F-06 --dry-run prints a structured plan (source method, source path, scanner images, commands, timeouts) and exits without invoking the scan endpoint. No containers start. * F-09 Scanner status vocabulary unified between table and JSON output (available | pulling | installed | configured | error). * F-10 security report text output now contains "Scanners: X run, Y failed (names) of Z" and a coverage warning when any scanner did not run, so a user cannot mistake scanner crashes for "clean". * F-11 Scan history endpoint returns the aggregated risk_score instead of always zero. UX: * F-12 Dashboard "Security Scan" chip is now a live router-link to /security with no "soon" placeholder; matches adjacent Docker / Quarantine chips. * F-14 security overview shows "Last scan: never" instead of 0001-01-01 00:00:00, and JSON emits last_scan_at: null. * F-15 security configure returns in ~3s (was 60s+), bumped client timeout from 10s to 60s as additional safety, added existence prefetch so typos return 404 fast. * F-16 scan --all redraws the progress table in place on TTY via ANSI cursor escapes; falls back to per-line output when piped; FINDINGS column now populated from findings_count. Extra: * mcpproxy security subcommands now honor --config and --data-dir global flags (newSecurityCLIClient checks the package-level configFile/dataDir before calling config.LoadFromFile, mirroring main.go). Not fixed (documented): * F-07 ramparts scanner container ships a GLIBC_2.39-linked binary that fails on arm64 macOS. Upstream scanner image issue, not mcpproxy code. * F-13 cisco-mcp-scanner stdout contains a hardcoded server_url header. Cosmetic upstream scanner output quirk. Post-fix revalidation: 6 of 7 scanners run end-to-end on arm64 macOS against the three test servers (everything, fetch-test, filesystem-test). The QA report in docs/qa/security-scanners-2026-04-10.html now contains a §11 "After-Fix Revalidation" section with per-finding verification evidence, updated code-landscape table, and reproduction instructions. Tests: internal/secret, internal/security, internal/security/scanner, cmd/mcpproxy, frontend unit tests all pass. golangci-lint: 0 issues. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
1 parent 6fbf914 commit 8a8743c

17 files changed

Lines changed: 3039 additions & 148 deletions

cmd/mcpproxy/security_cmd.go

Lines changed: 591 additions & 76 deletions
Large diffs are not rendered by default.

cmd/mcpproxy/security_cmd_test.go

Lines changed: 150 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,150 @@
1+
package main
2+
3+
import (
4+
"testing"
5+
"time"
6+
7+
"github.com/smart-mcp-proxy/mcpproxy-go/internal/config"
8+
)
9+
10+
// TestScannerDisplayStatus verifies F-09: scanner status vocabulary is
11+
// consistent and rich enough to distinguish "available" / "pulling" /
12+
// "installed" / "configured" / "error" in BOTH table and JSON outputs.
13+
func TestScannerDisplayStatus(t *testing.T) {
14+
cases := []struct {
15+
in string
16+
want string
17+
}{
18+
{"available", "available"},
19+
{"pulling", "pulling"},
20+
{"installed", "installed"},
21+
{"configured", "configured"},
22+
{"error", "error"},
23+
{"", "unknown"},
24+
// Future / unexpected values pass through unchanged so they don't
25+
// silently get hidden behind a hard-coded mapping.
26+
{"some-new-state", "some-new-state"},
27+
}
28+
for _, c := range cases {
29+
got := scannerDisplayStatus(c.in)
30+
if got != c.want {
31+
t.Errorf("scannerDisplayStatus(%q) = %q, want %q", c.in, got, c.want)
32+
}
33+
}
34+
}
35+
36+
// TestComputeScanHardTimeout verifies F-05: the per-scanner timeout is
37+
// extrapolated into a sensible whole-scan timeout that won't return early
38+
// nor hang for the duration of the universe.
39+
func TestComputeScanHardTimeout(t *testing.T) {
40+
// Nil config -> 15-minute fallback.
41+
if got := computeScanHardTimeout(nil, ""); got != 15*time.Minute {
42+
t.Errorf("nil cfg: got %s, want 15m", got)
43+
}
44+
45+
// Config with no security section -> fallback.
46+
cfg := &config.Config{}
47+
if got := computeScanHardTimeout(cfg, ""); got != 15*time.Minute {
48+
t.Errorf("nil security: got %s, want 15m", got)
49+
}
50+
51+
// Config with explicit per-scanner timeout, with explicit scanner list:
52+
// 60s * 3 + 30s = 3m30s, but we floor at 15m for sanity.
53+
cfg = &config.Config{
54+
Security: &config.SecurityConfig{
55+
ScanTimeoutDefault: config.Duration(60 * time.Second),
56+
},
57+
}
58+
if got := computeScanHardTimeout(cfg, "a,b,c"); got != 15*time.Minute {
59+
t.Errorf("60s*3 with floor: got %s, want 15m", got)
60+
}
61+
62+
// Per-scanner 5m, no flag (default 8 scanners): 5m*8 + 30s = 40m30s,
63+
// capped at 30m.
64+
cfg = &config.Config{
65+
Security: &config.SecurityConfig{
66+
ScanTimeoutDefault: config.Duration(5 * time.Minute),
67+
},
68+
}
69+
if got := computeScanHardTimeout(cfg, ""); got != 30*time.Minute {
70+
t.Errorf("5m*8 cap: got %s, want 30m", got)
71+
}
72+
73+
// Per-scanner 4m, 6 scanners: 4m*6 + 30s = 24m30s — within bounds.
74+
cfg = &config.Config{
75+
Security: &config.SecurityConfig{
76+
ScanTimeoutDefault: config.Duration(4 * time.Minute),
77+
},
78+
}
79+
got := computeScanHardTimeout(cfg, "s1,s2,s3,s4,s5,s6")
80+
want := 4*time.Minute*6 + 30*time.Second
81+
if got != want {
82+
t.Errorf("4m*6: got %s, want %s", got, want)
83+
}
84+
}
85+
86+
// TestNormalizeOverviewLastScan verifies F-14: Go zero-time `last_scan_at`
87+
// values are scrubbed to JSON null in both table and JSON outputs.
88+
func TestNormalizeOverviewLastScan(t *testing.T) {
89+
cases := []struct {
90+
name string
91+
in map[string]interface{}
92+
// We assert nil-ness via key presence and value.
93+
wantPresent bool
94+
wantNil bool
95+
wantValue interface{}
96+
}{
97+
{
98+
name: "missing key inserted as nil",
99+
in: map[string]interface{}{},
100+
wantPresent: true,
101+
wantNil: true,
102+
},
103+
{
104+
name: "explicit empty string -> nil",
105+
in: map[string]interface{}{"last_scan_at": ""},
106+
wantPresent: true,
107+
wantNil: true,
108+
},
109+
{
110+
name: "Go zero-time RFC3339 -> nil",
111+
in: map[string]interface{}{"last_scan_at": "0001-01-01T00:00:00Z"},
112+
wantPresent: true,
113+
wantNil: true,
114+
},
115+
{
116+
name: "real timestamp preserved",
117+
in: map[string]interface{}{"last_scan_at": "2025-01-15T10:30:00Z"},
118+
wantPresent: true,
119+
wantNil: false,
120+
wantValue: "2025-01-15T10:30:00Z",
121+
},
122+
}
123+
for _, c := range cases {
124+
t.Run(c.name, func(t *testing.T) {
125+
normalizeOverviewLastScan(c.in)
126+
v, present := c.in["last_scan_at"]
127+
if present != c.wantPresent {
128+
t.Errorf("present=%v, want %v", present, c.wantPresent)
129+
}
130+
if c.wantNil && v != nil {
131+
t.Errorf("expected nil value, got %v (%T)", v, v)
132+
}
133+
if !c.wantNil && c.wantValue != nil && v != c.wantValue {
134+
t.Errorf("value = %v, want %v", v, c.wantValue)
135+
}
136+
})
137+
}
138+
139+
// Nil map should not panic.
140+
normalizeOverviewLastScan(nil)
141+
}
142+
143+
// TestClearPreviousLines verifies F-16: passing 0 or negative values is a
144+
// safe no-op (so the first redraw cycle doesn't blow up the terminal).
145+
func TestClearPreviousLines(t *testing.T) {
146+
// We can't easily capture stdout here without restructuring; just verify
147+
// the function doesn't panic on edge cases.
148+
clearPreviousLines(0)
149+
clearPreviousLines(-1)
150+
}

0 commit comments

Comments
 (0)