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>

Author: claude

cmd/mcpproxy/security_cmd.go

@@ -0,0 +1,150 @@
+package main
+
+import (
+	"testing"
+	"time"
+
+	"github.com/smart-mcp-proxy/mcpproxy-go/internal/config"
+)
+
+// TestScannerDisplayStatus verifies F-09: scanner status vocabulary is
+// consistent and rich enough to distinguish "available" / "pulling" /
+// "installed" / "configured" / "error" in BOTH table and JSON outputs.
+func TestScannerDisplayStatus(t *testing.T) {
+	cases := []struct {
+		in   string
+		want string
+	}{
+		{"available", "available"},
+		{"pulling", "pulling"},
+		{"installed", "installed"},
+		{"configured", "configured"},
+		{"error", "error"},
+		{"", "unknown"},
+		// Future / unexpected values pass through unchanged so they don't
+		// silently get hidden behind a hard-coded mapping.
+		{"some-new-state", "some-new-state"},
+	}
+	for _, c := range cases {
+		got := scannerDisplayStatus(c.in)
+		if got != c.want {
+			t.Errorf("scannerDisplayStatus(%q) = %q, want %q", c.in, got, c.want)
+		}
+	}
+}
+
+// TestComputeScanHardTimeout verifies F-05: the per-scanner timeout is
+// extrapolated into a sensible whole-scan timeout that won't return early
+// nor hang for the duration of the universe.
+func TestComputeScanHardTimeout(t *testing.T) {
+	// Nil config -> 15-minute fallback.
+	if got := computeScanHardTimeout(nil, ""); got != 15*time.Minute {
+		t.Errorf("nil cfg: got %s, want 15m", got)
+	}
+
+	// Config with no security section -> fallback.
+	cfg := &config.Config{}
+	if got := computeScanHardTimeout(cfg, ""); got != 15*time.Minute {
+		t.Errorf("nil security: got %s, want 15m", got)
+	}
+
+	// Config with explicit per-scanner timeout, with explicit scanner list:
+	// 60s * 3 + 30s = 3m30s, but we floor at 15m for sanity.
+	cfg = &config.Config{
+		Security: &config.SecurityConfig{
+			ScanTimeoutDefault: config.Duration(60 * time.Second),
+		},
+	}
+	if got := computeScanHardTimeout(cfg, "a,b,c"); got != 15*time.Minute {
+		t.Errorf("60s*3 with floor: got %s, want 15m", got)
+	}
+
+	// Per-scanner 5m, no flag (default 8 scanners): 5m*8 + 30s = 40m30s,
+	// capped at 30m.
+	cfg = &config.Config{
+		Security: &config.SecurityConfig{
+			ScanTimeoutDefault: config.Duration(5 * time.Minute),
+		},
+	}
+	if got := computeScanHardTimeout(cfg, ""); got != 30*time.Minute {
+		t.Errorf("5m*8 cap: got %s, want 30m", got)
+	}
+
+	// Per-scanner 4m, 6 scanners: 4m*6 + 30s = 24m30s — within bounds.
+	cfg = &config.Config{
+		Security: &config.SecurityConfig{
+			ScanTimeoutDefault: config.Duration(4 * time.Minute),
+		},
+	}
+	got := computeScanHardTimeout(cfg, "s1,s2,s3,s4,s5,s6")
+	want := 4*time.Minute*6 + 30*time.Second
+	if got != want {
+		t.Errorf("4m*6: got %s, want %s", got, want)
+	}
+}
+
+// TestNormalizeOverviewLastScan verifies F-14: Go zero-time `last_scan_at`
+// values are scrubbed to JSON null in both table and JSON outputs.
+func TestNormalizeOverviewLastScan(t *testing.T) {
+	cases := []struct {
+		name string
+		in   map[string]interface{}
+		// We assert nil-ness via key presence and value.
+		wantPresent bool
+		wantNil     bool
+		wantValue   interface{}
+	}{
+		{
+			name:        "missing key inserted as nil",
+			in:          map[string]interface{}{},
+			wantPresent: true,
+			wantNil:     true,
+		},
+		{
+			name:        "explicit empty string -> nil",
+			in:          map[string]interface{}{"last_scan_at": ""},
+			wantPresent: true,
+			wantNil:     true,
+		},
+		{
+			name:        "Go zero-time RFC3339 -> nil",
+			in:          map[string]interface{}{"last_scan_at": "0001-01-01T00:00:00Z"},
+			wantPresent: true,
+			wantNil:     true,
+		},
+		{
+			name:        "real timestamp preserved",
+			in:          map[string]interface{}{"last_scan_at": "2025-01-15T10:30:00Z"},
+			wantPresent: true,
+			wantNil:     false,
+			wantValue:   "2025-01-15T10:30:00Z",
+		},
+	}
+	for _, c := range cases {
+		t.Run(c.name, func(t *testing.T) {
+			normalizeOverviewLastScan(c.in)
+			v, present := c.in["last_scan_at"]
+			if present != c.wantPresent {
+				t.Errorf("present=%v, want %v", present, c.wantPresent)
+			}
+			if c.wantNil && v != nil {
+				t.Errorf("expected nil value, got %v (%T)", v, v)
+			}
+			if !c.wantNil && c.wantValue != nil && v != c.wantValue {
+				t.Errorf("value = %v, want %v", v, c.wantValue)
+			}
+		})
+	}
+
+	// Nil map should not panic.
+	normalizeOverviewLastScan(nil)
+}
+
+// TestClearPreviousLines verifies F-16: passing 0 or negative values is a
+// safe no-op (so the first redraw cycle doesn't blow up the terminal).
+func TestClearPreviousLines(t *testing.T) {
+	// We can't easily capture stdout here without restructuring; just verify
+	// the function doesn't panic on edge cases.
+	clearPreviousLines(0)
+	clearPreviousLines(-1)
+}

cmd/mcpproxy/security_cmd_test.go

@@ -113,7 +113,7 @@
         <!-- Primary action button based on health.action -->
         <button
           v-if="healthAction === 'approve'"
-          @click="unquarantine"
+          @click="handleApproveClick"
           :disabled="loading"
           class="btn btn-sm btn-warning"
         >
@@ -232,6 +232,51 @@
       </div>
     </div>
 
+    <!-- Approve Confirmation Modal (F-04: security scanner gated) -->
+    <div v-if="showApproveConfirmation" class="modal modal-open">
+      <div class="modal-box">
+        <h3 class="font-bold text-lg mb-4">
+          {{ approveDialogMode === 'no_scan' ? 'No Security Scan Run' : 'Critical Findings Detected' }}
+        </h3>
+        <p v-if="approveDialogMode === 'critical'" class="mb-4">
+          <strong>{{ server.name }}</strong> has
+          <span class="text-error font-semibold">{{ criticalFindingCount }} critical finding{{ criticalFindingCount === 1 ? '' : 's' }}</span>
+          in its most recent security scan. Approving this server will allow it to run despite these warnings.
+        </p>
+        <p v-else class="mb-4">
+          No security scan has been run for <strong>{{ server.name }}</strong>. We strongly recommend running a scan first.
+        </p>
+        <p class="text-sm text-base-content/70 mb-6">
+          The security scanner is an experimental heuristic. Force-approving a server bypasses the scanner gate and is irreversible from this dialog.
+        </p>
+        <div class="modal-action">
+          <button
+            @click="showApproveConfirmation = false"
+            :disabled="loading"
+            class="btn btn-outline"
+          >
+            Cancel
+          </button>
+          <router-link
+            v-if="approveDialogMode === 'no_scan'"
+            :to="`/servers/${server.name}?tab=security`"
+            class="btn btn-primary"
+            @click="showApproveConfirmation = false"
+          >
+            Scan First
+          </router-link>
+          <button
+            @click="confirmForceApprove"
+            :disabled="loading"
+            class="btn btn-error"
+          >
+            <span v-if="loading" class="loading loading-spinner loading-xs"></span>
+            Force Approve
+          </button>
+        </div>
+      </div>
+    </div>
+
     <!-- Delete Confirmation Modal -->
     <div v-if="showDeleteConfirmation" class="modal modal-open">
       <div class="modal-box">
@@ -282,6 +327,8 @@ const systemStore = useSystemStore()
 const { hasEnabledScanners } = useSecurityScannerStatus()
 const loading = ref(false)
 const showDeleteConfirmation = ref(false)
+const showApproveConfirmation = ref(false)
+const approveDialogMode = ref<'no_scan' | 'critical'>('no_scan')
 
 const isHttpProtocol = computed(() => {
   return props.server.protocol === 'http' || props.server.protocol === 'streamable-http'
@@ -585,26 +632,67 @@ async function triggerLogout() {
   }
 }
 
-async function unquarantine() {
+// Counts critical findings from the scan summary if available. Used to gate
+// the Approve button behind an extra confirmation (F-04).
+const criticalFindingCount = computed(() => {
+  const scan = props.server.security_scan as any
+  if (!scan) return 0
+  // finding_counts.critical is populated from the latest report summary.
+  const fc = scan.finding_counts as Record<string, number> | undefined
+  if (fc && typeof fc.critical === 'number') return fc.critical
+  return 0
+})
+
+// True when a scan has actually been run (has a last_scan_at timestamp).
+const hasCompletedScan = computed(() => {
+  const scan = props.server.security_scan
+  if (!scan) return false
+  return !!scan.last_scan_at
+})
+
+// Primary approve click handler. Chooses the right flow based on scan state:
+//   1. No scan run yet → open "Scan first / Force approve" dialog
+//   2. Scan run with critical findings → open "Force approve?" dialog
+//   3. Clean scan → call securityApproveServer directly
+function handleApproveClick() {
+  if (!hasCompletedScan.value) {
+    approveDialogMode.value = 'no_scan'
+    showApproveConfirmation.value = true
+    return
+  }
+  if (criticalFindingCount.value > 0) {
+    approveDialogMode.value = 'critical'
+    showApproveConfirmation.value = true
+    return
+  }
+  void doSecurityApprove(false)
+}
+
+async function doSecurityApprove(force: boolean) {
   loading.value = true
   try {
-    await serversStore.unquarantineServer(props.server.name)
+    await serversStore.securityApproveServer(props.server.name, force)
     systemStore.addToast({
       type: 'success',
-      title: 'Server Unquarantined',
-      message: `${props.server.name} has been removed from quarantine`,
+      title: 'Server Approved',
+      message: `${props.server.name} has been approved and unquarantined`,
     })
+    showApproveConfirmation.value = false
   } catch (error) {
     systemStore.addToast({
       type: 'error',
-      title: 'Unquarantine Failed',
+      title: 'Approve Failed',
       message: error instanceof Error ? error.message : 'Unknown error',
     })
   } finally {
     loading.value = false
   }
 }
 
+function confirmForceApprove() {
+  void doSecurityApprove(true)
+}
+
 async function confirmDelete() {
   loading.value = true
   try {

frontend/src/components/ServerCard.vue

@@ -13,6 +13,8 @@ import api from '@/services/api'
 const enabledCount = ref<number | null>(null)
 const dockerAvailable = ref<boolean>(true)
 const loaded = ref(false)
+const totalFindings = ref<number>(0)
+const totalScans = ref<number>(0)
 let inflight: Promise<void> | null = null
 
 export async function refreshSecurityScannerStatus(): Promise<void> {
@@ -28,6 +30,12 @@ export async function refreshSecurityScannerStatus(): Promise<void> {
       const enabled = data.scanners_enabled ?? data.scanners_installed ?? 0
       enabledCount.value = typeof enabled === 'number' ? enabled : 0
       dockerAvailable.value = data.docker_available !== false
+      // Aggregated finding count from the /security/overview endpoint used by
+      // the Dashboard security chip (F-12). The backend returns this under
+      // `findings_by_severity.total`.
+      const fbs = data.findings_by_severity ?? {}
+      totalFindings.value = typeof fbs.total === 'number' ? fbs.total : 0
+      totalScans.value = typeof data.total_scans === 'number' ? data.total_scans : 0
       loaded.value = true
     } catch {
       // On error, keep current values; the UI will fall back to dockerAvailable
@@ -48,8 +56,12 @@ export function useSecurityScannerStatus() {
     enabledCount,
     dockerAvailable,
     loaded,
+    totalFindings,
+    totalScans,
     /** True when at least one scanner is enabled and docker is available. */
     hasEnabledScanners: () => (enabledCount.value ?? 0) > 0,
+    /** True when at least one scan has completed (ever). */
+    hasAnyScans: () => totalScans.value > 0,
     refresh: refreshSecurityScannerStatus,
   }
 }

frontend/src/composables/useSecurityScannerStatus.ts

@@ -277,6 +277,31 @@ export const useServersStore = defineStore('servers', () => {
     }
   }
 
+  // Security-aware approval path (Spec 039 / F-04). Goes through
+  // POST /api/v1/servers/{name}/security/approve which enforces the
+  // scanner gate before unquarantining the server. Use this — not
+  // unquarantineServer — for all user-facing "Approve" buttons.
+  async function securityApproveServer(serverName: string, force = false) {
+    try {
+      const response = await api.securityApprove(serverName, force)
+      if (response.success) {
+        // Optimistic update: the backend will also unquarantine the server
+        // on a successful approve, so reflect that in local state. SSE
+        // refresh will reconcile any remaining fields shortly after.
+        const server = servers.value.find(s => s.name === serverName)
+        if (server) {
+          server.quarantined = false
+        }
+        return true
+      } else {
+        throw new Error(response.error || 'Failed to approve server')
+      }
+    } catch (error) {
+      console.error('Failed to approve server via security scanner:', error)
+      throw error
+    }
+  }
+
   async function deleteServer(serverName: string) {
     try {
       const response = await api.deleteServer(serverName)
@@ -371,10 +396,11 @@ export const useServersStore = defineStore('servers', () => {
     triggerOAuthLogout,
     quarantineServer,
     unquarantineServer,
+    securityApproveServer,
     deleteServer,
     updateServerStatus,
     getServerByName,
     addServer,
     cleanupEventListeners,
   }
-})
+})