feat(security): scan_server/get_scan_report in quarantine_security + fix Scan-All visibility and Docker gating - #1032
Merged
Conversation
…fix Scan-All visibility and Docker gating TPA scanning existed but was unreachable from where people actually are. MCP: quarantine_security — the most-used security surface — never mentioned scanning and had no way to run one. It gains two operations, `scan_server` (runs the offline in-process baseline scan; returns the verdict when it settles, otherwise the job id) and `get_scan_report` (latest verdict, counts, findings). list_quarantined / inspect_quarantined / inspect_tools now carry a one-line scan status, so an unscanned server reads as "never scanned — run scan_server first" instead of looking clean. Neither operation adds a parameter (both reuse `name`); the three frozen tool-surface goldens were regenerated deliberately and their enumerated-delta gates widened to name quarantine_security. Web UI: the Security page's "Scan All Servers" button never rendered on a fresh install (the overview counted only BBolt-persisted scanners and missed the always-on in-process baseline, reporting 0) and was disabled without Docker — the same false gate already fixed for the per-server Scan Now button. The overview now counts registry in-process scanners, and the button is no longer gated on Docker; the tooltip and the Docker alert say what Docker actually affects (optional deep scanners). CLI: `mcpproxy security --help` opened with "Security scanners run as Docker containers", which is only true of the optional layer. Also fixes dispatch for the advertised `quarantine_server` operation name, which only accepted the historical "quarantine" spelling.
Deploying mcpproxy-docs with
|
| Latest commit: |
4a3d74d
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://ee452137.mcpproxy-docs.pages.dev |
| Branch Preview URL: | https://feat-quarantine-security-sca.mcpproxy-docs.pages.dev |
📦 Build ArtifactsWorkflow Run: View Run Available Artifacts
How to DownloadOption 1: GitHub Web UI (easiest)
Option 2: GitHub CLI gh run download 32690759109 --repo smart-mcp-proxy/mcpproxy-go
|
|
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
…ace-free Two defects found in cross-model review of the scan surface. 1. awaitScanVerdict trusted the terminal job status. The scanner engine sets job.Status to a terminal value BEFORE its completion callback persists the report, and the job only leaves the engine's active map after the callback returns. A summary read inside that window still answers "scanning", so scan_server could reply status=completed + verdict=scanning — a non-verdict that reads like a clean-ish result. The wait now keeps polling until the settled job's own verdict is visible and otherwise falls back to the honest async answer. 2. GetOverview counted the in-process baseline by reading ScannerPlugin.Status from the pointers Registry.List() hands out. Those are the registry's live records, and UpdateStatus mutates Status on exactly them under the registry lock, so the count raced with any concurrent scanner install/pull. The predicate now resolves inside Registry.InProcessRunnableIDs(), under the same lock as the write — and that helper is nil-receiver safe, so a Service built without a registry no longer panics in the one call that had stopped guarding for it. Tests: the completion window (both halves), a -race loop of GetOverview against concurrent UpdateStatus, and the nil-registry path.
…us scan The summary and the aggregated report are two independent latest-by-server reads. While a new scan is running the summary says "scanning" but the report is still the PREVIOUS job's, so the response carried that older job's id, findings_total and findings with nothing saying they were not the running scan's result — an agent reading it would attribute stale findings to the scan it just started. The response now names whose findings they are.
…ort holds Two more defects from cross-model review, both in how the new scan surface reads the scanner service's latest-by-server state. 1. scan_server polled GetScanStatus, which answers with whatever job is ACTIVE for the server. With deep scan enabled a completed Pass 1 auto-starts the Pass-2 supply-chain audit, so the active job becomes Pass 2 — an id that never matches the one scan_server started. The wait then ran out its full budget and answered "scan started" while the baseline verdict was already sitting there. It now polls Pass 1 specifically, via a new GetScanStatusByPass on the scanner interface. 2. get_scan_report labelled every report attached to a "scanning" summary as the previous scan's. Per-scanner reports are persisted as each scanner finishes, so the report can equally be the RUNNING job's partial output — which the response also reported as job_id, contradicting the label. The label now distinguishes the two by comparing the report's job id with the running Pass-1 job.
…n reject Two defects on the paths quarantine_security's new scan_server / get_scan_report now expose to agents. Lost cache invalidation. GetScanSummary reads BBolt with no lock held and stores the result afterwards, while the scan callbacks invalidate. A reader that sampled the pre-scan state could land its store on top of a starting scan's invalidation, leaving a stale "clean" summary cached for a server being scanned right now — and scan_server waits for the job to settle and then reports that summary verbatim. Each server now carries an invalidation generation: the reader samples it on the cache miss and cacheScanSummary drops the write if it moved. Temp-dir leak. Service.StartScan exports tool definitions into an os.MkdirTemp dir and gives the cleanup to the scan callback, but the engine rejects a scan (already in progress, no scanners resolved) before it ever calls that callback, so the directory survived for the life of the process. Release it on the reject path — every engine error path returns before OnScanStarted, so it cannot double-free. Reachable in a loop now that scan_server is an agent-callable MCP operation.
…meters assertQuarantineSecurityDelta claimed every non-`operation` schema stays byte-equal, but it only compared `type` on `name` and only the enum on `operation`. A later golden regeneration could therefore slip a new `pattern`, `enum` or `default` onto `name`, or change `operation.type`, and the frozen-golden gate would still pass — on the two parameters this feature is allowed to touch, i.e. the widest blind spot the helper could have. Compare both schemas minus only the keys the feature may move (`description` on `name`; `enum` + `description` on `operation`). Verified by injecting a `pattern` on `name` and confirming the gate now fails on every surface.
The test pinned os.TempDir() to TMPDIR, which only holds on unix — Windows resolves TMP/TEMP, so the precondition failed the windows-amd64 build job. Set all three variables, read os.TempDir() back instead of asserting which one won, and count only the "mcpproxy-scan-tools-" prefix so the assertion stays exact even when the process temp dir is shared. Re-verified it still fails when the cleanup on the engine-reject path is removed.
net.Listen creates the socket file with umask-default permissions before listener_unix.go chmods it to 0600; the readiness poll only waited for the file to exist, so a stat landing inside that window asserted against 0755 (observed twice on the ubuntu -race CI runner). Wait for the hardened mode before asserting.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
…n accessor + test fake union - server.go: keep main's mutex-guarded securityScannerSvc() + setSecurityScanner publisher, drop the duplicate plain accessor. All #1032 call sites already nil-check the returned service. - scanner/service.go: identical StartScan cleanup fix on both sides; keep main's comment, single cleanup call. - scan_admission_test.go: union fake — main's startScanTries/startScanErrByServer/ scanResult plus #1032's jobs/passJobs/reports and per-pass lookups. StartScan records every attempt, honours per-server errors, runs the onStartScan hook, and returns the pinned Pass-1 job.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
TPA scanning exists but is unreachable from the surfaces people actually use.
MCP.
quarantine_securityis the most-used security surface (64 installs vs 12 for the scanning tools) and it never mentioned scanning, let alone offered it. It gains:scan_server(name) — runs the always-on offline baseline scan in-process (no Docker); returns the verdict when the scan settles, otherwise the job id and"status": "scan started".get_scan_report(name) — latest verdict, risk score, counts, deep-scan descriptor, and up to 10 findings.list_quarantined/inspect_quarantined/inspect_toolsnow carry a one-linescan_status, so an unscanned server reads asnever scanned — run scan_server firstrather than looking clean.Neither operation adds a parameter — both reuse
name— so the delta is the operation enum plus prose. The three frozen tool-surface goldens were regenerated deliberately; the enumerated-delta gates now namequarantine_security(andcode_execution_modegot the frozen pre-feature copy it never needed before, a byte-for-byte snapshot of its previous golden).Web UI. "Scan All Servers" did not render on a fresh install:
GetOverviewcounted only BBolt-persisted scanners and missed the in-process baseline in the in-memory registry, soscanners_enabledwas 0. It was also disabled without Docker ("Docker is required to run security scanners") — the same false gate already fixed for the per-server Scan Now button (spec 088 FR-016). The overview now counts registry in-process scanners, the button is ungated, and the tooltip plus the Docker alert say what Docker actually affects: the optional deep scanners.CLI.
mcpproxy security --helpopened with "Security scanners run as Docker containers", true only of the optional layer. Rewritten: baseline is built in and needs no Docker; deep scanners are the Docker-backed extra.Also fixes dispatch of the advertised
quarantine_serveroperation name, which previously only accepted the historicalquarantinespelling and returned "Unknown quarantine operation".Key decisions:
scan_serverwaits up to 8s for the scan to settle before answering async, so the common in-process case returns a verdict in one call without holding a slow one open.findings_truncated) — the full report stays on REST/web UI.