feat(security): offline TPA scanner + trust-tiered approval modes (spec 086) - #919
Open
Dumbris wants to merge 17 commits into
Open
feat(security): offline TPA scanner + trust-tiered approval modes (spec 086)#919Dumbris wants to merge 17 commits into
Dumbris wants to merge 17 commits into
Conversation
Two spec-kit feature specs (spec.md + plan.md each) plus a cross-cutting NEXT-STEPS sequencing doc, for consuming the tpa-db scanner-bundle.json in mcpproxy's offline detect engine: - 086: bundle-backed detect.Check + per-server 3-mode trust setting (auto/scan/manual) gating BOTH new-server admission and tool-change approval; migrates the AutoApproveToolChanges tri-state. - 087: daily offline-first bundle refresh (embedded default -> file drop -> optional signed fetch), fail-safe to last-known-good, every activation gated by the cmd/scan-eval recall/FP bar; daily release-availability check reusing internal/updatecheck (no auto-installer). Grounded in real symbols (checkToolApprovals, normalizeServerQuarantineFlags, deriveBaselineVerdict, ApproveServer, inprocess.go check slice). Planning only, no code. Related #86
…e 1)
Load the tpa-db scanner-bundle.json (contract v0.1.0) as a deterministic,
fully-offline detect.Check that flags tools whose description matches a
bundled regex rule, driving the same hard-tier quarantine gate as the
built-in checks. Implements spec 086 FR-001..FR-007 (stage 1: loader +
bundle-backed check).
## Changes
- Embed the known-good default bundle at
internal/security/scanner/bundled/scanner-bundle.json via //go:embed. The
embed + parse live in the scanner package, never in detect, which is
offline-import-guarded (imports_test.go forbids os/path/filepath/net/embed).
- internal/security/scanner/tpa_bundle.go:
- loadBundleCheck parses the bundle, verifies bundle_version major.minor
(refuses an unknown major/minor rather than running stale rules; accepts a
differing PATCH; ignores unknown additive keys), and compiles every
engine==regex / target==tool_description rule once under RE2. A single
un-compilable pattern rejects the WHOLE candidate (never a partial load).
- engine==structural_diff and non-tool_description targets are counted as
not-runnable coverage (Skipped), and the bundle's own skipped[] LLM/jsonpath
detectors are tracked separately (Declared) — never counted as clean.
- BundleCheck implements detect.Check: ID()=="tpa.bundle"; Inspect ranges the
pre-sorted compiled rules and emits one TierHard Signal per description hit
with CheckID "tpa.<TPA-id>.<detector>", Confidence=rule.confidence, and a
ThreatType mapped from rule.category. Pure/total/deterministic.
- defaultBundleCheck loads the embedded default once (sync.Once); on a load
failure it logs a warning and returns nil so scanning continues WITHOUT the
bundle — a bundle problem never breaks the scanner.
- internal/security/scanner/inprocess.go: append the bundle check to the
detect.NewEngine check slice so its findings flow through
detectFindingToScanFinding -> ScanFinding -> the verdict/quarantine machinery
with no extra plumbing (FR-005).
## Testing
- go build ./... — passes.
- go test ./internal/security/... — passes (incl. the detect offline import
guard).
- go vet ./internal/security/scanner/... — clean.
- New table-driven tests (tpa_bundle_test.go): embedded bundle yields 7
runnable regex rules with 3 not-runnable rules + 3 declared-skipped; the
check FIRES a TierHard signal (CheckID tpa.TPA-2026-0001.hidden_instruction)
on the hidden-instruction payload; stays SILENT on a benign description; an
unsupported bundle_version is rejected; a bad regex rejects the whole load;
two Inspect calls are byte-identical (determinism).
Related #86
…stage 2) ## Changes - config: add ServerConfig.TrustMode (json:"trust_mode", mapstructure:"trust-mode") and a typed EffectiveTrustMode() accessor (auto|scan|manual) — the single resolution point, failing closed to manual for empty/unknown values. - config: extend normalizeServerQuarantineFlags with a second pass mapping auto_approve_tool_changes onto trust_mode when unset (true->auto, false->manual, nil->empty->manual). Preserves the skip_quarantine->auto_approve pass and never clobbers an explicit trust_mode or the auto_approve_tool_changes pointer. - config: IsQuarantineSkipped()/IsAutoApproveToolChanges() are now thin wrappers over EffectiveTrustMode()==auto; add trust_mode to CopyServerConfig and a MergeServerConfig PATCH branch (REST trust-tier changes). - scanner: add exported ScanToolMetadataVerdict — a synchronous, offline (no Docker/network/fs) verdict over the shared baseline detect engine + tpa-db bundle, surfacing coverage so callers can fail closed. - runtime: derive the mode via EffectiveTrustMode() in checkToolApprovals; under trust_mode: scan the changed-tool and rug-pull seams auto-approve ONLY on a green (clean, full-coverage) verdict, else hold the record (fail closed). Add TransitionReason ReasonScanApproved (provenance "scan-approved") to both assertToolApprovalInvariant allow-lists. auto keeps today's auto-approve; manual always holds. ## Testing - go build ./... ; go vet (config, runtime, scanner) — clean - go test ./internal/config/... ./internal/runtime/... ./internal/security/... — pass - Added: config migration+EffectiveTrustMode+merge tables; scanner verdict table (benign clean / injection non-clean / empty fail-closed); runtime scan-mode benign-approve, malicious-held, still-changed held/cleared, manual-held, auto-approved. Related #86
## Changes - config: add `Config.QuarantineDefaultForServer(sc)` — resolves the add-time Quarantined default from a server's `trust_mode`: auto is admitted UNQUARANTINED, scan|manual are quarantined on add (FR-011). The global `quarantine_enabled=false` escape hatch (#370) still wins; a nil server config fails closed to quarantine. - Route all three add/admission call sites through the new helper (CN-002: the mode comes from config/registry-derived trust_mode, never a request-driven admission override): - REST /api/v1/servers: resolve via `req.TrustMode`; the request `quarantined` boolean (#370) still wins after. - MCP upstream_servers add: parse `trust_mode`, resolve via it, and carry it onto the created ServerConfig. Empty trust_mode resolves to manual, so the omitted case is behavior-identical to the old global default. - add-from-registry: re-resolve `serverCfg.Quarantined` per-server after the pure builder returns. Registry entries carry no client trust_mode, so this stays manual -> quarantine, preserving the CN-002 quarantine-by-default invariant. - server: on `EventTypeSecurityScanSettled`, auto-approve a scan-mode server whose freshly-read baseline verdict is GREEN ("clean") via `ApproveServer(force=false)` (unquarantine + baseline-approve pending tools). Fail closed on every other condition — manual servers never auto-approve, a non-quarantined server is skipped (re-entrancy / stale-replay guard), and any non-"clean" or missing verdict leaves the server quarantined. The settle payload lacks the verdict, so it is re-read via `GetScanSummary`; `ApproveServer(force=false)` re-gates on hard-tier findings as defense in depth. ## Testing - `go build ./...`, `go vet` on the touched packages. - `go test ./internal/config/... ./internal/runtime/... ./internal/security/... ./internal/server/... ./internal/httpapi/... -count=1` — all pass (the internal/server E2E TestBinary*/TestMCPProtocol* suites require a pre-built `mcpproxy` binary at internal/server/mcpproxy; green once built). - New table-driven tests: `TestConfig_QuarantineDefaultForServer` (auto unquarantined; scan|manual quarantined; global-disable escape hatch; nil/empty/unknown fail closed) and `TestShouldAutoApproveScanSettled` (scan+clean approves; dangerous/warnings/failed/not_scanned/scanning/empty stay quarantined; manual never approves; already-unquarantined skipped). Related #86
Spec 086 stage-3 admission gate hardening from code review: - FR-011 "trigger a scan" (high): scan-mode servers were quarantined on add but nothing ever started their baseline scan, so they stayed quarantined forever (the settle handler never fired). Add maybeStartAdmissionScans on the servers.changed event: kick a one-shot baseline scan for every scan-mode, still-quarantined, never-scanned server. StartScan auto-connects the quarantined server, scans, and emits the debounced settle event the existing auto-approve handler consumes. Idempotent via an in-memory kicked-set plus the "already scanned" (non-nil summary) guard. - Admission-window gate (low): maybeAutoApproveScanSettled auto-unquarantined a scan-mode server on ANY clean settle, silently overriding a human's deliberate post-approval re-quarantine. Gate on scanner.HasApprovalBaseline — a prior integrity baseline means the server was already admitted, so a later clean scan never re-approves it. FR-011 is a NEW-server admission gate only. - Testability (medium): introduce a securityScannerService interface seam so the real settle handler and admission-scan trigger can be driven with a fake scanner + live runtime config. New scan_admission_test.go covers the fail-closed branches the pure predicate test bypassed: missing/stale config, manual-never-approve, empty verdict, ApproveServer error, prior-baseline re-quarantine, and the one-shot/deduped scan trigger. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Deploying mcpproxy-docs with
|
| Latest commit: |
0bc76b8
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://5f886e5b.mcpproxy-docs.pages.dev |
| Branch Preview URL: | https://086-tpa-scanner-approval.mcpproxy-docs.pages.dev |
Related to spec 086 REST DTO changes (AddServerRequest/patch trust_mode).
TrustMode (spec 086) was added to ServerConfig but not wired into the storage save-sync path, so TestSaveServerSyncFieldCoverage failed and a REST/UI/MCP-set trust_mode would be wiped on the next SaveConfiguration rebuild. Round-trip it through UpstreamRecord like AutoApproveToolChanges. ## Changes - UpstreamRecord gains a TrustMode string field - ServerConfig<->UpstreamRecord conversions (manager.go x3, async_ops.go) carry it - field-coverage test allowlist updated ## Testing - go test ./internal/storage/... ./internal/config/... ./internal/runtime/... ./internal/contracts/... ./internal/httpapi/... pass - go vet ./... clean
|
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
…y flag, MCP trust_mode ## Changes - runtime: trust_mode:scan now scans NEW post-baseline tool additions and auto-approves only on a green verdict (was: held pending indefinitely). Fails closed on non-green/degraded/absent verdict. (codex P1) - httpapi: new-server admission honors legacy auto_approve_tool_changes when trust_mode is omitted, so an auto server is not left contradictorily quarantined. (codex P2) - mcp: declare trust_mode (auto|scan|manual) on the upstream_servers tool input schema and map it in update/patch, so MCP clients can discover and set it. (codex P2) Skipped (with rationale): treating by-design skipped structural_diff rules as degraded coverage would make scan mode never auto-approve (every bundle carries such rules); runtime-configurable bundle path is spec 087 scope. ## Testing - new tests: scan-mode new-tool addition (benign->scan-approved, malicious->held) - go build ./..., go vet, go test runtime/httpapi/server pass; OpenAPI verify clean
The codex-fix that declares trust_mode on the upstream_servers MCP tool schema is an intended parameter addition; update the pre-feature surface snapshot in all three surfaces so TestMenuSurface_ExactDeltaFromPreFeature reflects it (the tool-name-set delta is unchanged — no new tools).
# Conflicts: # internal/server/server.go # oas/docs.go
The roadmap generator picks up the new specs/086-tpa-scanner-approval and specs/087-tpa-daily-refresh directories added in this branch.
📦 Build ArtifactsWorkflow Run: View Run Available Artifacts
How to DownloadOption 1: GitHub Web UI (easiest)
Option 2: GitHub CLI gh run download 30257172589 --repo smart-mcp-proxy/mcpproxy-go
|
maybeStartAdmissionScans ran on the servers.changed event-loop goroutine and iterated runtime.Config().Servers. Config() returns a shared lock-free snapshot whose Servers slice/structs other goroutines mutate in place, so the background read raced with concurrent config/storage writes (caught by the -race E2E job, e.g. TestE2E_SensitiveData_*). Snapshot servers via StorageManager(). ListUpstreamServers() instead — RLock-guarded, fresh copies, serialized against SaveUpstreamServer by the manager mutex. ## Testing - go test -race ./internal/server -run TestE2E (full suite): 0 races, 0 fails - the two previously-failing tests (AWSAccessKey, FilePath) pass with -race - confirmed the race does NOT occur on origin/main (introduced by this branch)
The race fix made maybeStartAdmissionScans read the server list from StorageManager().ListUpstreamServers() instead of the shared live config, so newAdmissionTestServer must seed storage (not just cfg.Servers) or the sweep sees an empty list. Mirrors production, where an added server is saved to BBolt before servers.changed fires. ## Testing - go test ./internal/server -run 'TestMaybeStartAdmissionScans|TestMaybeAutoApproveScanSettled' passes (with and without -race) - full CI-equivalent scope (go test -tags nogui -skip E2E|Binary|MCPProtocol ./...) exit 0
…o scan-mode admission works Spec 086 FR-011 quarantines a trust_mode=scan server on add and then kicks an admission baseline scan. That scan could never run: the scanner's tool export goes through configServerInfoProvider.GetServerTools -> Server.GetServerTools, which reads the StateView tool cache, and quarantined servers are deliberately excluded from tool discovery/indexing (issue #873). The cache is therefore empty by construction for exactly the servers being admitted, StartScan aborted with "tool export failed (server is connected but returned 0 tools)", and scan mode collapsed into manual: no scan job, no green auto-admit, no TPA finding on a poisoned server (QA ADM-02 / ADM-03 / SRF-06). The scanner-facing provider now falls back to a LIVE tools/list on the managed upstream client - the same direct-client seam the quarantine inspection surface (quarantine_security/inspect_quarantined) already uses - when the cached read yields nothing. Scoped to scanning/inspection: definitions land only in the scan's tools.json, never in StateView or the Bleve index, so quarantined tools stay un-indexed and un-callable and #873 is not reintroduced. General tool discovery and GetAllServerTools (peer snapshot) are untouched. Also stop the retry churn in Service.StartScan: EnsureConnected RESTARTS the server, so calling it on an already-connected server tore the connection down mid-export and turned a recoverable "0 tools" into "server is disconnected". The retry now reconnects only when the server is actually disconnected. Tests (fail before the fix with the exact production error message): - TestConfigServerInfoProviderQuarantinedLiveToolExport: live fallback, plus the disconnected / no-client negative paths. - TestAdmissionScanRealExportPath: real scanner.Service + real provider export path for a quarantined server - green settles clean and auto-unquarantines with an integrity baseline; poisoned settles dangerous, stays quarantined and persists a TPA-YYYY-NNNN finding (SC-006); a manual-mode quarantined server is neither scanned nor live-listed.
PR #919 added contracts.Server.TrustMode and generated an OpenAPI response property documenting it as "surfaced on the GET path so clients can read back the persisted mode", but no production code ever emitted a "trust_mode" key: the field was always the zero value on GET /api/v1/servers, on `mcpproxy upstream list -o json` (the same REST route) and on the MCP upstream_servers list. The write path (POST/PATCH -> MergeServerConfig -> config.json) worked, so the tier was persisted and enforced but unreadable. Wire the field through every server read seam, mirroring how the deprecated predecessor auto_approve_tool_changes (MCP-2940) and init_timeout (MCP-3322) are handled: - internal/runtime/runtime.go: emit trust_mode in both server projections (StateView + getAllServersLegacy), which also feeds the SSE servers.changed embed. - internal/management/service.go: project srvRaw["trust_mode"] onto contracts.Server in ListServers (the GET /api/v1/servers seam). - internal/server/server.go: same for the tray/legacy projections that back the httpapi fallback path (contracts.ConvertGenericServersToTyped's trust_mode branch was dead until now). - internal/server/mcp.go: emit it from handleListUpstreams so an agent can read back the mode it set via operation=add/update/patch. - cmd/mcpproxy/upstream_cmd.go: same for the daemon-less CLI list path. The RAW configured value is emitted and omitted when empty, so the wire keeps distinguishing "never configured" from an explicit tier; resolution of empty/unknown values stays the sole job of config.EffectiveTrustMode(). No contracts/OpenAPI change was needed — swagger already documents the field. Regression coverage: internal/server/trust_mode_readback_test.go drives POST then GET through the real chi router and asserts the MCP list payload, plus a "trust_mode projected" case in internal/management/service_test.go.
…(FR-018)
A tool change (or new tool) held by the trust_mode: scan gate showed only
that it was held — the matched TPA signature was visible on the security-scan
surfaces but never on the tool-approval view an operator actually reviews.
Carry the blocking evidence from the gate onto the approval record:
- storage.ToolApprovalRecord gains held_reason / held_verdict / held_signals
(additive, omitempty) plus SetScanHold/ClearScanHold. Signals are the
deterministic check ids, e.g. "tpa.TPA-2026-0001.hidden_instruction",
deduped and capped at MaxToolHeldSignals.
- scanChangeIsClean now returns the hold evidence; all three gate sites (new
tool -> pending, still-changed, approved -> changed) persist it. Every
transition back to approved clears it, so an approved record can never show
a stale TPA badge. recordScanHold only writes when the evidence changed, so
a steady-state held tool costs no BBolt write per discovery pass.
- Surfaced on contracts.Tool (REST /api/v1/tools and /servers/{id}/tools),
the tool-diff endpoint, the management-service tool projection, and the MCP
quarantine_security inspect_tool_approvals result.
- CLI: `mcpproxy tools list` gains a HELD column rendering the matched
TPA-YYYY-NNNN id (falling back to the raw check id, then the hold reason).
Back-compat: records written before these fields decode with them empty and
render exactly as before ("-" in the table, keys absent from JSON).
Regenerated frontend/src/types/contracts.ts and the OpenAPI artifacts.
… keeps them (FR-018) The scanner emits its heuristic checks (directive.imperative, capability.mismatch, secret.embedded) ahead of the tpa.* checks, so the tool-approval CLI listing's 2-signal cap collapsed the matched TPA-YYYY-NNNN id into the '+N' suffix and never named it for the canonical poison payload. Hoist TPA-matched labels ahead of raw check ids before truncating. JSON/REST/MCP surfaces were already correct.
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.
Summary
Adds a fast, fully-offline Tool-Poisoning-Attack (TPA) scanner to mcpproxy whose knowledge is an updatable signature database (the tpa-db
scanner-bundle.json), plus an opt-in, per-server trust-tiered approval model. Implements spec086-tpa-scanner-approval(spec/plan inspecs/086-tpa-scanner-approval/).A user sets a per-server
trust_mode:autoscanmanualEverything fails closed: a missing/degraded/dangerous verdict never auto-approves, and
manualis never auto-approved even on a clean scan.Changes
internal/security/scanner/tpa_bundle.go): embedsscanner-bundle.json, version-checks it, compiles the regex rules once, skips stateful/structural_diffrules offline, and runs as onedetect.Checkwired into the in-process scanner. Evidence routed throughdetect.CapEvidence(render-safe).trust_modeconfig (internal/config): new per-serverTrustMode(auto/scan/manual) +EffectiveTrustMode(), migrated from the legacyauto_approve_tool_changestri-state without clobbering explicit values; invalid modes fail closed tomanual. Wired through REST create/patch DTOs.internal/runtime/tool_quarantine.go):scanmode consults a synchronous, Docker-free in-process verdict (scanner.ScanToolMetadataVerdict, peer-aware for cross-server shadowing) at the tool-change seams; newReasonScanApprovedtransition registered in the approval invariants.internal/server,internal/config):QuarantineDefaultForServergoverns add-time quarantine by trust mode across all three add paths; scan-mode servers are scanned on connect and auto-approved on a green settle.Testing
go build ./...,go vet, andgo test ./internal/config/... ./internal/runtime/... ./internal/security/... ./internal/server/... ./internal/httpapi/...all pass locally (incl. the offline import guard).manual-never-auto-approves, and admission per mode.codex+ an independent adversarial verifier; findings (scan bypass, fail-open on invalid mode, cross-server shadowing coverage gap, scan-never-triggered) were fixed.Notes / follow-ups
trust_modein theupstream_serversMCP tool input schema; thecmd/scan-evalgate does not yet cover the bundle check (prerequisite for the follow-up daily-refresh spec 087).🤖 Generated with Claude Code