Repository navigation
fix(security): keep implicit quarantine across server restarts and config writes (Spec fix-quarantine-restart) - #1463
Merged
Conversation
…ntine survives RestartServer re-reads mcp_config.json (#467) and persisted the raw entry of a server that never stated quarantined, erasing the quarantine the config-load admission gate recorded. The entry now goes through the same gate before it is persisted or used, and fails closed when storage is unreadable.
…ecision SaveUpstreamServer and the async saveServerSync now keep a stored Quarantined=true unless the incoming config states quarantined explicitly. The read-check-write runs in one bbolt transaction so a concurrent QuarantineUpstreamServer is not lost. Tests that un-quarantined through a plain save now mark the decision explicit.
…tine The REST PATCH handler marks the explicit bit when the body carries quarantined, and UpdateServer applies the value only then, so an unrelated PATCH can no longer copy a stale false over the stored record.
Deploying mcpproxy-docs with
|
| Latest commit: |
0edf3da
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://89fd7812.mcpproxy-docs.pages.dev |
| Branch Preview URL: | https://fix-quarantine-restart-bypas.mcpproxy-docs.pages.dev |
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Contributor
📦 Build ArtifactsWorkflow Run: View Run Available Artifacts
How to DownloadOption 1: GitHub Web UI (easiest)
Option 2: GitHub CLI gh run download 36994351547 --repo smart-mcp-proxy/mcpproxy-go
|
…one in binding guard e2e
4 tasks
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
github-actions Bot
pushed a commit
that referenced
this pull request
Oct 3, 2026
…le reach (#1482) Docs, changelog and verified fixes from a first-run, upgrade and user-test pass on `main` (335477e). ## Docs - `docs/features/profiles.md`: profiles are optional. Without an effective profile, no profile-level restriction applies (a scoped credential's own grant still does). The quick start is presented as being for scoped access. - `docs/configuration.md` (`quarantined` row): explains the real default. A server added through the UI, CLI or API follows its trust mode. A first-seen config-file server with no `quarantined` key and no `config.db` record is held for review. An explicit value wins, and a recorded server keeps its state. Links to the admission rules. - `docs/features/security-quarantine.md`: the hand-editing section names the extra conditions: quarantine is enabled, and the trust mode is not `auto` (including legacy `auto_approve_tool_changes`/`skip_quarantine` resolving to `auto`), matching `ServerConfig.EffectiveTrustMode`. - `docs/getting-started/quick-start.mdx`: a plain note that a server added by editing the file is held for review until approved. - `CHANGELOG.md`: the security, review, catalog, telemetry, CLI and first-run fixes merged since v0.69.0 (#1463, #1467–#1473, #1481) plus this PR's. ## Fixes (each reproduced in an isolated instance, regression test fails on main) - **Upgrade start advisory repeated.** `reportPreFixAdmissions` ran on every gate pass (initial gate, `LoadConfiguredServers`, every publish), so an upgrade start logged the "predate the config-load admission gate" advisory 2–5 times. It is now reported once per server per process. Gate decisions are unchanged. - **Benign startup race logged as ERROR.** Both the supervisor reconcile and `LoadConfiguredServers` → `AddServer` asked a not-yet-ready client to connect. The second call was correctly refused with "connection already in progress or established (state: Connecting)" but logged at ERROR, and server identity registration was skipped. A new `managed.ErrConnectAlreadyActive` sentinel (same message text) lets `AddServer` treat it as success at debug level. - **`set_profile` under-reported a switchable client's reach.** After a successful switch, `resolveEffectiveProfileForJustSetSlug` treated the client binding as authoritative and reported the binding's servers instead of the selected profile's. ## Verification - Cross-model review: codex gpt-6.1-sol, chunked. Three low findings: two doc-wording findings are applied, and the third (the per-process advisory set is never cleared) is a deliberate trade-off. - Live re-verification on isolated instances: each symptom was reproduced on a main build and is gone on this branch. The upgrade case was run over a v0.52.1 data dir.
3 tasks done
github-actions Bot
pushed a commit
that referenced
this pull request
Oct 3, 2026
…de (RC-UPG-001) (#1485) # Pull Request ## Description Fixes RC-UPG-001, the release blocker in the v0.70.0-rc.2 test report. On upgrade from v0.69, a server that v0.69 held in quarantine was admitted, and all of its tools were auto-approved. **Root cause** - v0.69's restart path (`lookupServerConfigForRestart`) re-read `mcp_config.json`. A server with no `quarantined` key reads as `false`, and that path wrote the entry to config.db **ungated**. - The baseline security scan shortly after startup triggers such a restart. So does any manual or secret-driven restart. - v0.69 kept the server quarantined in memory, which is why it looked held. config.db now said `quarantined:false`. - The RC already gates the restart path (#1463). On upgrade, though, the config-load admission gate reads that stale record as "known to config.db = already admitted", and auto-baseline then approves every tool. **Fix** (`internal/runtime/config_load_admission_gate.go`) - Being known to config.db is no longer enough by itself. A known, unquarantined server is held again when all of these are true: - its config never states `quarantined`; - its trust mode would hold it; - it has **no approval baseline**. A baseline means at least one tool record that is approved, or that carries an `ApprovedHash`, such as a rug-pulled "changed" record. - Servers with approved tools are unaffected. They still get only the existing pre-fix advisory. - If approval records can't be read, the gate keeps the old rule: unknown is not the same as never admitted. - The re-quarantine is logged ("Quarantining known server with no approved tool baseline"), emitted as activity, and persisted. The next start takes the existing "retain recorded quarantine" branch. **Deliberate trade-off (fail-closed).** Some vetted servers have no tool records, and nothing in storage distinguishes them from the stale v0.69 record. They are held once on upgrade: - zero-tool servers (prompts or resources only); - servers never discovered while live, such as disabled ones; - servers last vetted before v0.21. One approval releases each, and so does an explicit `"quarantined": false` set before upgrading. This is documented in `docs/features/security-quarantine.md` and the changelog. **Tests** - New `config_load_admission_gate_upgrade_test.go` covers: - a known server with only pending records, and one with no records, is held; - an approved record, or a changed-after-approval record, counts as vetted; - explicit `false` and `trust_mode: auto` stay live; - the hold survives a reload. - Three existing tests modeled a "vetted" server without the approval baseline that every real vetted server has. They now seed one. ## Testing - [x] I have tested these changes locally - [x] I have added/updated tests that prove my fix is effective or my feature works - [x] All existing tests pass Upgrade replay with the real binaries, on an isolated instance: v0.69 with a keyless server and a vetted server, then a restart of the keyless one, then an upgrade. | | keyless server after upgrade | vetted server | |---|---|---| | v0.70.0-rc.2 | live, 9 tools auto-approved (bug) | live | | this branch | quarantined, 0 tools | live | The race suite with `-tags server` passes for runtime, config, storage, server and httpapi, and lint is clean. The cross-model review (zcode) found one MEDIUM, the population above. It is kept as a documented trade-off, and round 2 came back clean. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
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
Security fix. Restarting a server that the config-load admission gate (#937) had quarantined silently cleared its quarantine, and the next unrelated config write, reload or process restart then admitted it and auto-approved its tools. This is not a Spec 108/109 task, so no spec documents change and no names or contracts change on any surface (REST, CLI, MCP, Web UI, macOS app are unaffected apart from the corrected behavior).
Root cause
lookupServerConfigForRestartininternal/runtime/lifecycle.gois the #467 "disk-first restart" helper. It re-readsmcp_config.json, writes the raw file entry to config.db withSaveUpstreamServer, and hands the same entry to the upstream manager. A server entry that never statedquarantineddecodes as not quarantined with no explicit bit. The admission gate had quarantined it at load and recorded that in config.db, so the restart overwrote the recorded quarantine with false. The gate's durability depends on config.db: its "known server" branch only re-holds a server when the stored record is quarantined and the config states nothing. After the overwrite, the nextSaveConfiguration(approving or disabling a different server, a PATCH, enable/disable) rebuilt the file and snapshot from storage without the quarantine, and any later reload or reboot treated the server as known, live and vetted. The restarted client was also created as trusted until the next reconcile.Affected paths
Everything that reaches
Runtime.RestartServer: REST restart and restart_all,mcpproxy upstream restart(including--all), MCPupstream_servers operation=restart, the tray restart, secret changes that restart affected servers, the scanner'sEnsureConnected(manual scan and the automatic baseline sweep shortly after startup), and the server-edition admin handler. The bug has existed since the gate was added in #944, which did not gate this disk read.Fix
SaveUpstreamServerand the asyncsaveServerSyncnever lower a stored quarantine unless the incoming config statesquarantinedexplicitly. The read-check-write happens inside one bbolt transaction, so a concurrentQuarantineUpstreamServercannot be lost. The guard changes only the persisted record, never the caller's struct (which may be a published snapshot pointer), and logs a warning naming the server.quarantined, andUpdateServerapplies the value only then. Previously an unrelated PATCH could copy a stale false from a config snapshot over the stored record.Decisions
"quarantined": falsein the file, or an explicit PATCH, is still obeyed (operator statement keeps its bug: mcp_config.json file-watcher hot-reload doesn't propagate env (and likely headers) changes to running upstreams #467 and security: servers loaded from mcp_config.json bypass the trust-mode admission gate entirely #937 meaning).Runtime.QuarantineServer(name, false)(security approve, tray, REST unquarantine, scan-mode auto-approve) goes throughQuarantineUpstreamServer, which the guard does not touch.docs/features/security-quarantine.mdnow says so.Tests
internal/runtime/restart_quarantine_test.go: restart keeps implicit quarantine (storage, published config and the written file, including after approving a different server), keeps it across a simulated reboot with no tool approvals promoted, gates a first-seen server restarted from disk, obeys an explicit false on disk, does not over-quarantine undertrust_mode: auto, and fails closed when storage is unreadable.internal/storage/quarantine_guard_test.goandasync_ops_test.go: the guard never lowers without an explicit decision (Go-built and JSON-decoded explicit false both lower), new servers and raises are unaffected, the caller's struct is not mutated,QuarantineUpstreamServerstill un-quarantines, and a concurrent quarantine is not lost under-race.internal/httpapi/patch_server_test.go: the explicit bit is set only when the body carriesquarantined.internal/server/update_server_quarantine_test.go: an update that omits quarantine keeps the stored value; an explicit false un-quarantines and is written to the file.go test -raceon runtime, storage, httpapi, server, management, security and config, the server-edition lane with the CI skip regex, the server E2E tests, and golangci-lint (bare and--build-tags server).Follow-up (medium/low, not in this PR)
REST PATCH
quarantined:falsestill bypasses the baseline-approve and activity event thatQuarantineServerperforms; consider routing it throughQuarantineServer.