feat(security): offline TPA scanner + trust-tiered approval modes (spec 086)#919
Open
Dumbris wants to merge 13 commits into
Open
feat(security): offline TPA scanner + trust-tiered approval modes (spec 086)#919Dumbris wants to merge 13 commits into
Dumbris wants to merge 13 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: |
f3639ff
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://be631e5b.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 30192756971 --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
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