fix: validate trust_mode, surface the TPA signature bundle, show hold state (#938) - #946
Conversation
… state Three findings from one QA pass, all "the runtime is right but the operator is misled". 1. trust_mode was accepted verbatim from REST/MCP and from a hand-edited config. EffectiveTrustMode() fails closed to manual on anything unrecognized, so a typo'd "Scan" silently behaved as manual while every read surface echoed the typo back as a real mode. Unknown values are now rejected with a 400 (POST/PATCH /api/v1/servers), a tool error (upstream_servers), a CLI error (--trust-mode), and a config validation error naming the accepted vocabulary. 2. FR-019 (config-driven signature-DB location) was unimplemented and the corpus was invisible. Adds security.tpa_bundle_path (env override MCPPROXY_TPA_BUNDLE_PATH), filesystem loading with fail-closed fallback to the last-known-good corpus, hot-reload via ApplySecurityConfig, and a signature_bundle descriptor (source/version/generated_at/fingerprint/ runnable+skipped counts/load_error) on GET /api/v1/security/overview, `mcpproxy security overview`, and the Web UI Security tab. The one load report no longer goes to the unconfigured global zap.L(). 3. Hold state now reaches the surfaces that hid it: `tools list --server` gains APPROVAL/HELD columns and escapes upstream-controlled descriptions (the global view's byte-slice truncation is fixed too), `upstream list` names held/pending/blocked tools instead of a green "Connected", the server card no longer reads a plain green "Clean" while tools are held, and `upstream add` gains --trust-mode. Related #938
Deploying mcpproxy-docs with
|
| Latest commit: |
ba0cf7d
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://74f5dcd4.mcpproxy-docs.pages.dev |
| Branch Preview URL: | https://fix-938-trust-mode-validatio.mcpproxy-docs.pages.dev |
|
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
📦 Build ArtifactsWorkflow Run: View Run Available Artifacts
How to DownloadOption 1: GitHub Web UI (easiest)
Option 2: GitHub CLI gh run download 30687315912 --repo smart-mcp-proxy/mcpproxy-go
|
P1 — an empty-but-valid signature bundle silently disabled all TPA coverage. loadBundleCheck returned a live BundleCheck with zero rules whenever a bundle parsed, passed the version gate, and yielded no runnable rule (empty rules[], or rules that are all structural_diff / non-tool_description). The scan gate computes bundlePresent from that check, so coverageOK stayed true and a trust_mode:scan server could be auto-approved on a rug-pulled description with zero signatures running. Now a zero-runnable corpus fails the load, which routes it through the existing fail-closed fallback (last-known-good corpus stays live, reason in signature_bundle.load_error). `security overview` and the Web UI stat also render a zero count as a warning/error tone instead of a neutral number. P1 — upgrade regression: rejecting a bogus trust_mode at LOAD time bricked installs. A config carrying the value a previous release persisted through the REST API made config.LoadFromFile fail, so the daemon refused to start and every hot-reload was blocked. Rejection stays at the write seams (REST POST/PATCH, upstream_servers, --trust-mode, and now add-json); the load path and /api/v1/config/apply normalize the value to manual — the tier the runtime already applied to it — and warn. P2 — the terminal-escaping fix was bypassable via the tool NAME. Names are upstream-controlled and were rendered raw on all three paths (server-scoped, global, no-daemon), so `"\x1b[2J\x1b[1;1Happroved"` wrote ANSI straight to the operator's tty. Names now go through the same render-safe escape + cap. P2 — tpa_bundle_path was ignored in stdio mode. ConfigureBundle was only reachable from startCustomHTTPServer, a branch Start() skips when listen is empty, while the scan gate itself is transport-independent. The corpus is now installed at server construction and re-applied on hot-reload in every transport. P2 — `upstream add --trust-mode auto` behaved differently with and without a daemon: the config path hardcoded quarantined=true instead of deriving it from the trust tier like the REST path does. P2 — `upstream add-json` silently discarded trust_mode (no field in the decode struct). P2 — MCPPROXY_TPA_BUNDLE_PATH was only applied inside config.Load, so a config posted to /api/v1/config/apply defeated the env override. The precedence now lives in SecurityConfig.EffectiveTPABundlePath, so every path resolves it the same way. Related #938
Cross-model review round — all 7 findings fixed (
|
| # | Finding | Fix | Test |
|---|---|---|---|
| P1 | Empty-but-valid bundle disables all TPA coverage while the gate reports full coverage | loadBundleCheck now fails the load when zero rules are runnable → the existing fail-closed fallback keeps the last-known-good corpus and records load_error |
internal/security/scanner/tpa_bundle_empty_test.go |
| P1 | A config carrying the bogus trust_mode the previous release persisted blocks daemon startup |
Reject at the write seams (unchanged); normalize-and-warn on the load path + /api/v1/config/apply (config.NormalizeTrustModes) |
internal/config/trust_mode_normalize_test.go |
| P2 | Escaping fix bypassed via the tool NAME | sanitizeName on all three render paths (server-scoped, global, no-daemon) + a 60-rune cap |
cmd/mcpproxy/tool_name_escape_test.go |
| P2 | tpa_bundle_path silently ignored in stdio mode |
configureTPABundle at server construction + on hot-reload — transport-independent, matching the gate |
internal/server/tpa_bundle_transport_test.go |
| P2 | upstream add --trust-mode auto differs with/without a daemon |
the config path now uses Config.QuarantineDefaultForServer, same as the REST path |
cmd/mcpproxy/upstream_add_config_mode_test.go |
| P2 | upstream add-json discards trust_mode |
decoded + validated in the extracted parseAddJSONRequest |
same file |
| P2 | /api/v1/config/apply bypasses MCPPROXY_TPA_BUNDLE_PATH precedence |
precedence moved into SecurityConfig.EffectiveTPABundlePath, so every path resolves it identically |
internal/config/tpa_bundle_env_precedence_test.go |
Two extra visibility fixes fall out of finding 1: mcpproxy security overview prints an explicit WARNING: no TPA signatures are running and the Web UI stat renders text-error when runnable_rules == 0 (previously a neutral 0).
Red evidence (representative)
--- FAIL: TestServerScopedToolRowsEscapesName
Error: "\x1b[2J\x1b[1;1Happroved" should not contain "\x1b"
--- FAIL: TestGlobalToolRowsEscapesName (same)
--- FAIL: TestStandaloneToolRowsEscapesName (same)
--- FAIL: TestToolNameIsBounded
Error: "500" is not less than or equal to "60"
--- FAIL: TestUpstreamAddConfigModeQuarantineFollowsTrustMode
Error: Should be false
Messages: trust_mode auto must not be quarantined on add (matches the daemon path)
--- FAIL: TestConfigureBundle_EmptyCorpusKeepsLastKnownGood
expected: "embedded" actual: "file"
"" does not contain "no runnable rules"
--- FAIL: TestTPABundleConfiguredInStdioMode
expected: "<tmp>/scanner-bundle.json" actual: ""
expected: 1 actual: 7 (the embedded corpus was running)
# compile-level red (missing symbols)
internal/config/trust_mode_normalize_test.go:27: undefined: NormalizeTrustModes
cmd/mcpproxy/tool_name_escape_test.go:40: undefined: globalToolRows
cmd/mcpproxy/upstream_add_config_mode_test.go:63: undefined: parseAddJSONRequest
# frontend
AssertionError: expected '' to be 'text-error'
AssertionError: expected 'text-warning' to be 'text-error'
Green gates
$ ./scripts/run-linter.sh
Running golangci-lint...
0 issues.
$ go test ./internal/config/... ./internal/security/... ./internal/runtime/... ./internal/httpapi/... ./cmd/mcpproxy/... -count=1
ok .../internal/config 2.008s
ok .../internal/security/scanner 2.894s
ok .../internal/security/detect 0.967s
ok .../internal/runtime 103.770s
ok .../internal/runtime/configsvc 1.614s
ok .../internal/httpapi 1.968s
ok .../cmd/mcpproxy 2.400s
$ go test ./internal/server/ -count=1
ok .../internal/server 526.397s
$ cd frontend && npx vitest run
Test Files 58 passed (58)
Tests 576 passed (576)
No REST annotations or DTOs changed, so the OpenAPI artifacts are untouched (the pre-push hook's "Verify OpenAPI spec is up to date" passed). No Swift changes.
Deliberately deferred
Nothing from the review. One judgement call worth flagging: /api/v1/config/apply normalizes a legacy bogus trust_mode rather than rejecting it, because a full-config apply usually round-trips whatever is already on disk and would otherwise be permanently blocked by a value it did not introduce. The per-server write seams (POST/PATCH /api/v1/servers, upstream_servers, --trust-mode, add-json) still reject with the accepted vocabulary — that is where an operator actually types one.
There was a problem hiding this comment.
Live QA verified: trust_mode validation rejects bogus values at every write seam (REST add/patch, upstream_servers tool add/update/patch, CLI add/add-json, config validate) with nothing bogus reaching storage; signature bundle surfaced; hold state visible. All 35 checks green.
Addresses all three findings in #938. Each is "the runtime is right but the operator is misled", so every fix is about what a surface says.
1. MEDIUM —
trust_modewas accepted verbatimEffectiveTrustMode()fails closed tomanualon anything unrecognized, so a typo'd"Scan"behaved asmanualwhile every read surface echoed the typo back as though it were a real mode.Unknown values are now refused, with the accepted vocabulary in the message, at every write seam:
POST/PATCH /api/v1/servers→400, controller never calledupstream_serversMCP tool (add + update/patch) → tool errormcpproxy upstream add --trust-mode→ CLI error before anything is writtenValidateDetailederror (servers[N].trust_mode)Matching is deliberately case-sensitive;
config.IsValidTrustMode/config.ValidTrustModesare the single source of truth. Note the config-load half is fatal, consistent with the existing per-serverprotocolandtoon_outputvalidation — a hand-edited bogus mode stops the load rather than quietly running asmanual.Verified against a live core (port 18142, scratch data-dir + config):
2. MEDIUM — FR-019 implemented, and the bundle is now visible
All three parts of the finding:
Config-driven path (FR-019). New
security.tpa_bundle_path(env overrideMCPPROXY_TPA_BUNDLE_PATH, env wins over file). Empty = the corpus embedded in the build. The path is re-read on everyconfig.reloadedviaApplySecurityConfig, so it hot-reloads without a restart. A bundle that fails to read/parse/version-check/compile is refused: the previously active corpus stays live (fail-closed, never fail-empty) and the reason is logged + surfaced.The status report now reaches a log. It went through the unconfigured global
zap.L(); it now goes through the injected scanner logger.Freshness signal.
signature_bundleonGET /api/v1/security/overview:source,path,bundle_version,schema_version,signature_count,runnable_rules/skipped_rules/declared_skipped,generated_at,fingerprint(sha256 prefix),loaded_at,load_error.generated_atreads the bundle's own optional key, falling back to the file's mtime for file-sourced corpora — the v0.1.0 format emits no stamp of its own. Rendered bymcpproxy security overviewand by a "Signatures (runnable)" stat on the Web UI Security tab.Verified on the live core:
Hot-reload + fail-closed, same running process, no restart (env-configured file bundle → corrupt the file → touch config → fix the file → touch config):
3. LOW — hold state on the surfaces that hid it
tools list --server <name>gainsAPPROVALandHELDcolumns (the per-server REST payload always carriedapproval_status/held_*; only the renderer dropped them) and now escapes the description throughdetect.CapEvidence. The global view shared the byte-slice truncation bug (splits multi-byte runes) — both views now use onesanitizeCellhelper, and the standalone (no-daemon) path escapes too.upstream listno longer prints a green✅ Connected (1 tool)over a held tool: the status names the hold (… · 1 changed held), the emoji drops to⚠️, and the action hint points attools list --server=<name>. The counts were already in the payload (contracts.QuarantineStats).cleanfull-server scan verdict with tools held now readsClean scan · N tools heldin warning tone (green check-mark path suppressed, tooltip explains the two independent gates). Awarnings/dangerous/failedverdict is untouched — it already out-ranks the hold.upstream add --trust-modeadded (validated as above), so a server can be added straight intoscanmode.Tests
TDD throughout — every test was written first and observed failing for the right reason (missing symbol or wrong status/render).
internal/config:trust_mode_validation_test.go(vocabulary +ValidateDetailed),tpa_bundle_path_test.go(accessor + env override + precedence)internal/httpapi:trust_mode_validation_test.go(400 on POST/PATCH incl.Scan/SCAN, controller never reached; valid modes still pass)internal/security/scanner:tpa_bundle_source_test.go(embedded default, file load replaces the corpus and its rule is the one that fires, bad path keeps last-known-good, unsupported version refused, logs through the injected logger, hot-reload),bundle_overview_test.go(ApplySecurityConfigwiring +signature_bundlein the overview)cmd/mcpproxy:hold_visibility_test.go(server-scoped columns, escaping, rune-boundary truncation, overview bundle lines),upstream_hold_test.go,upstream_add_trust_mode_test.gofrontend/tests/unit:security-badge-hold.spec.ts,signature-bundle.spec.tsDeliberately deferred
security bundle/security scannersfreshness surface. The descriptor is on the overview only (REST + CLI + Web UI).mcpproxy security scannersstill lists plugins without corpus metadata.generated_atfrom tpa-db. The v0.1.0 bundle format carries no stamp; the loader reads it when present and otherwise falls back to file mtime. Adding it to the producer belongs in tpa-db.upstream list. The hold is folded into the existing STATUS column rather than widening the table.Related #938