From 03a72696d44293bdac84671b1e010c7d900b2d00 Mon Sep 17 00:00:00 2001 From: Algis Dumbris Date: Fri, 30 Oct 2026 13:22:10 +0200 Subject: [PATCH 1/2] fix(quarantine): server admission stays human under the default trust tier Spec 119 made scan the default trust mode, so the spec 086 clean-scan admission gate began releasing every server added without a trust_mode (TestE2E_QuarantineConfigApply has failed on main since #1740). Server release is meant to stay with a person. - ServerConfig.AdmitsOnCleanScan: only an explicit trust_mode "scan" lets a clean admission scan unquarantine a server. The default tier still auto-approves clean tool changes. - maybeAutoApproveScanSettled, maybeStartAdmissionScan and scanModeAdmissionOwns use it; default-tier servers get the informational (non-gating) baseline scan instead. - The approval commit re-checks AdmitsOnCleanScan for scan-auto approvals under the commit lock, so a trust_mode changed mid-scan cannot release the server. - Web UI: the add form sends trust_mode only when one is picked, and the selector checks no radio for an unset mode so Scan can be chosen explicitly. - Docs, CLI help and CHANGELOG describe the admission rule. --- CHANGELOG.md | 8 +++-- cmd/mcpproxy/upstream_cmd.go | 2 +- docs/configuration.md | 2 +- docs/features/security-quarantine.md | 2 +- docs/intro.md | 2 +- frontend/src/components/ManualServerForm.vue | 9 +++-- frontend/src/components/TrustModeSelector.vue | 13 ++++++-- frontend/src/utils/trustMode.ts | 7 ++-- frontend/tests/unit/add-server-manual.spec.ts | 19 +++++++++-- .../unit/server-detail-auto-approve.spec.ts | 4 +-- .../tests/unit/trust-mode-selector.spec.ts | 12 +++++-- internal/config/config.go | 20 +++++++++-- .../runtime/server_approval_commit_test.go | 33 +++++++++++++++++++ internal/runtime/tool_approval_review.go | 9 +++++ internal/server/scan_admission_test.go | 21 ++++++++++++ internal/server/scan_informational.go | 8 +++-- internal/server/scan_informational_test.go | 6 ++-- internal/server/server.go | 12 ++++--- 18 files changed, 154 insertions(+), 35 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1a4ec9e0d..d17723c6e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -28,9 +28,11 @@ Releases follow [Semantic Versioning](https://semver.org/). changes behind an unchanged definition. **Migration:** set `trust_mode: "manual"` on any server whose tool changes you want a person to review; nothing changes for servers with an explicit `trust_mode` or `auto_approve_tool_changes`. A record whose stored annotation snapshot differs from the live hints in - dispatch tier only (a server last seen before hints were captured) is held once for review. New servers - added with no trust mode are quarantined, scanned and released automatically on a clean verdict, as - `scan` always did. + dispatch tier only (a server last seen before hints were captured) is held once for review. Server + admission stays with a person: a new server added with no trust mode is quarantined and stays + quarantined until someone approves it. Only an explicit `trust_mode: "scan"` lets a clean admission + scan release a server, and the approval re-checks that under the commit lock. The Web UI add form sends + a trust mode only when one is picked. - **connect / disconnect:** `mcpproxy disconnect`, `DELETE /api/v1/connect/{client}`, and the Web UI and tray Disconnect now also revoke the client's credential (`client-`), so a secret copied out diff --git a/cmd/mcpproxy/upstream_cmd.go b/cmd/mcpproxy/upstream_cmd.go index 80dcbc852..4b9edd38d 100644 --- a/cmd/mcpproxy/upstream_cmd.go +++ b/cmd/mcpproxy/upstream_cmd.go @@ -379,7 +379,7 @@ func init() { upstreamAddCmd.Flags().StringVar(&upstreamAddTransport, "transport", "", "Transport type: http or stdio (auto-detected if not specified)") upstreamAddCmd.Flags().BoolVar(&upstreamAddIfNotExists, "if-not-exists", false, "Don't error if server already exists") upstreamAddCmd.Flags().BoolVar(&upstreamAddNoQuarantine, "no-quarantine", false, "Don't quarantine the new server (use with caution)") - upstreamAddCmd.Flags().StringVar(&upstreamAddTrustMode, "trust-mode", "", "Per-server trust tier governing admission AND tool-change approval: auto (approve without scanning), scan (new tools and changed tools are auto-approved only when the offline TPA scan is clean; findings are held for review), manual (human reviews every change). Unset inherits the default (scan)") + upstreamAddCmd.Flags().StringVar(&upstreamAddTrustMode, "trust-mode", "", "Per-server trust tier governing admission AND tool-change approval: auto (approve without scanning), scan (new tools and changed tools are auto-approved only when the offline TPA scan is clean; findings are held for review), manual (human reviews every change). Unset inherits the default (scan for tool changes; the server stays quarantined until approved — only an explicit scan releases it on a clean scan)") upstreamAddCmd.Flags().StringArrayVar(&upstreamAddSecretEnvs, "secret-env", nil, "Environment variable to store in the OS keyring instead of the config, in KEY=value format (repeatable, FR-065)") upstreamAddCmd.Flags().StringArrayVar(&upstreamAddSecretHeaders, "secret-header", nil, "HTTP header to store in the OS keyring instead of the config, in 'Name: value' format (repeatable, FR-065)") diff --git a/docs/configuration.md b/docs/configuration.md index c7ebcb24c..289448c6f 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -453,7 +453,7 @@ the error names the field. | `oauth` | object | No | OAuth configuration (see [OAuth Configuration](#oauth-configuration)) | | `isolation` | object | No | Per-server Docker isolation settings (see [Docker Isolation](#docker-isolation)) | | `enabled` | boolean | No | Enable/disable server (default: `true`) | -| `quarantined` | boolean | No | Security quarantine status. The default depends on how the server arrives. A server added through the UI, CLI, REST API or an AI agent follows its [trust mode](features/security-quarantine.md#trust-modes-auto--scan--manual) at add time (quarantined under the default `scan` mode, then released automatically on a clean scan). A server you add by editing this file, with no `quarantined` key and no prior `config.db` record, is **held for review** on first load when quarantine is enabled and its trust mode is not `auto` (no `trust_mode` means `scan`, unless a legacy `auto_approve_tool_changes` resolves it — `true` to `auto`, `false` to `manual` — or `skip_quarantine: true` resolves it to `auto`). An explicit `"quarantined": false` admits it, and an explicit `true` holds it. A server that is already recorded in `config.db` keeps its recorded state. See [Servers added by hand-editing `mcp_config.json`](features/security-quarantine.md#servers-added-by-hand-editing-mcp_configjson) for the full admission rules. | +| `quarantined` | boolean | No | Security quarantine status. The default depends on how the server arrives. A server added through the UI, CLI, REST API or an AI agent follows its [trust mode](features/security-quarantine.md#trust-modes-auto--scan--manual) at add time (quarantined under the default tier until you approve it; an explicit `trust_mode: scan` releases it automatically on a clean scan). A server you add by editing this file, with no `quarantined` key and no prior `config.db` record, is **held for review** on first load when quarantine is enabled and its trust mode is not `auto` (no `trust_mode` means `scan`, unless a legacy `auto_approve_tool_changes` resolves it — `true` to `auto`, `false` to `manual` — or `skip_quarantine: true` resolves it to `auto`). An explicit `"quarantined": false` admits it, and an explicit `true` holds it. A server that is already recorded in `config.db` keeps its recorded state. See [Servers added by hand-editing `mcp_config.json`](features/security-quarantine.md#servers-added-by-hand-editing-mcp_configjson) for the full admission rules. | | `reconnect_on_use` | boolean | No | When `true`, tool calls to a disconnected server trigger an immediate reconnect attempt (15s timeout) before failing (default: `false`) | | `expose_prompts` | boolean | No | Per-server override for whether this server's MCP prompts are aggregated into mcpproxy's `prompts/list`. Only takes effect when the global `aggregate_upstream_prompts` master switch is on. Omit to expose prompts whenever the server advertises `Capabilities.Prompts`; `false` opts this server out even if it does. | | `toon_output` | string | No | Per-server override for the global [`toon_output`](#toon-output-adaptive-result-encoding): `off`, `adaptive`, or `always`. Non-empty value wins over the global for this server's tools; omit to inherit. See [TOON Output](features/toon-output.md). | diff --git a/docs/features/security-quarantine.md b/docs/features/security-quarantine.md index cd248f028..d5f55740f 100644 --- a/docs/features/security-quarantine.md +++ b/docs/features/security-quarantine.md @@ -593,7 +593,7 @@ new-server admission and tool-change approval (superseding the binary | Mode | Add time | Tool changes | |------|----------|--------------| | `auto` | Admitted without quarantine or scanning | Trusted without scanning (rug-pull risk) | -| `scan` (default) | Quarantined, then a fail-closed automatic TPA scan admits it on a clean verdict | New tools and **changed** tool contracts are auto-approved when the offline scan verdict is clean with full coverage (spec 119) **and** the structural gate finds nothing (spec 120): any safety-hint change in either direction, a new required parameter, a type change, an enum member added or an enum dropped, a loosened or removed constraint, a default or const added or changed, a new parameter that carries a default, const or enum, an output-schema change, or a dispatch-tier change with an equal hash is held whatever the verdict ([details](#structural-gate)). A finding or an uncovered tool is held for review with the evidence attached. A paraphrased rug pull that scans clean and changes nothing structural is admitted: that is the accepted residual risk of the default tier — use `manual` on servers where it is not acceptable | +| `scan` (default) | Quarantined. Set **explicitly**, a fail-closed automatic TPA scan admits it on a clean verdict. Left unset (the default tier), the server stays quarantined until a person approves it: server release stays human unless you opt in | New tools and **changed** tool contracts are auto-approved when the offline scan verdict is clean with full coverage (spec 119) **and** the structural gate finds nothing (spec 120): any safety-hint change in either direction, a new required parameter, a type change, an enum member added or an enum dropped, a loosened or removed constraint, a default or const added or changed, a new parameter that carries a default, const or enum, an output-schema change, or a dispatch-tier change with an equal hash is held whatever the verdict ([details](#structural-gate)). A finding or an uncovered tool is held for review with the evidence attached. A paraphrased rug pull that scans clean and changes nothing structural is admitted: that is the accepted residual risk of the default tier — use `manual` on servers where it is not acceptable | | `manual` | Quarantined for human review | Every change held for review, including input-schema-only changes, an added output schema, an added description and annotation-hint flips (see [What quarantine does and does not cover](#what-quarantine-does-and-does-not-cover)); a clean scan verdict is recorded as evidence and approves nothing | Config field: per-server `trust_mode`; REST: `trust_mode` on diff --git a/docs/intro.md b/docs/intro.md index 4a500b22b..fd359284f 100644 --- a/docs/intro.md +++ b/docs/intro.md @@ -32,7 +32,7 @@ Instead of loading all tools from all servers into your AI client's context, MCP ### Security Quarantine -New MCP servers are quarantined on add and their tools are withheld from agents in the meantime. Under the default `scan` trust mode a server is released automatically when its offline scan is clean; under `manual` it waits for your approval. This reduces exposure to Tool Poisoning Attacks, where a malicious server tries to manipulate AI behavior through crafted tool descriptions. A clean scan is not proof of safety: `scan` and `auto` admit servers or changes without a human review, so use `manual` on servers where that is not acceptable. +New MCP servers are quarantined on add and their tools are withheld from agents in the meantime. By default a new server waits for your approval; set `trust_mode: scan` on a server to release it automatically when its offline scan is clean. This reduces exposure to Tool Poisoning Attacks, where a malicious server tries to manipulate AI behavior through crafted tool descriptions. A clean scan is not proof of safety: an explicit `scan` and `auto` admit servers, and the default tier admits clean tool changes, without a human review, so use `manual` on servers where that is not acceptable. ### Docker Isolation diff --git a/frontend/src/components/ManualServerForm.vue b/frontend/src/components/ManualServerForm.vue index 858b677f3..f2c97e50f 100644 --- a/frontend/src/components/ManualServerForm.vue +++ b/frontend/src/components/ManualServerForm.vue @@ -115,7 +115,7 @@ import TrustModeSelector from '@/components/TrustModeSelector.vue' import { resolveSecretFields, rollbackSecrets } from '@/composables/useSecretFields' import { useServersStore } from '@/stores/servers' import { serverDetailPath } from '@/utils/serverRoute' -import { DEFAULT_TRUST_MODE, type TrustMode } from '@/utils/trustMode' +import type { TrustMode } from '@/utils/trustMode' const props = withDefaults(defineProps<{ navigateAfterAdd?: boolean @@ -140,7 +140,10 @@ const errorEl = ref(null) const name = ref('') const protocol = ref<'stdio' | 'http'>('stdio') -const trustMode = ref(DEFAULT_TRUST_MODE) +// Unset until the operator picks a mode: an unset trust_mode is the default +// tier, which never releases the server from quarantine on a clean scan; an +// explicit Scan does. +const trustMode = ref(undefined) const trustModeConfirmationPending = ref(false) const url = ref('') const command = ref('') @@ -191,8 +194,8 @@ async function handleSubmit() { name: name.value, protocol: protocol.value, enabled: true, - trust_mode: trustMode.value, } + if (trustMode.value) serverData.trust_mode = trustMode.value if (protocol.value === 'http') { serverData.url = url.value if (Object.keys(resolved.headers).length > 0) serverData.headers_json = JSON.stringify(resolved.headers) diff --git a/frontend/src/components/TrustModeSelector.vue b/frontend/src/components/TrustModeSelector.vue index 2fd9e0ffb..2b835ab01 100644 --- a/frontend/src/components/TrustModeSelector.vue +++ b/frontend/src/components/TrustModeSelector.vue @@ -17,7 +17,7 @@ class="radio radio-primary radio-sm mt-0.5 shrink-0" :name="groupName" :value="m.mode" - :checked="displayedMode === m.mode" + :checked="checkedMode === m.mode" @change="onSelect(m.mode)" /> @@ -118,6 +118,15 @@ const pendingWarnMode = ref(null) /** Tentative selection while a warning is open; otherwise the effective mode. */ const displayedMode = computed(() => pendingWarnMode.value ?? state.value.effective) +/** + * The checked radio. An unset value (the default tier) checks none: it is not + * the same as an explicit Scan, which also releases a quarantined server on a + * clean scan, so the operator must be able to pick Scan to opt in. + */ +const checkedMode = computed(() => + pendingWarnMode.value ?? (state.value.isDefault ? null : state.value.effective) +) + // A value arriving from the parent (save, re-load, SSE refresh) supersedes any // selection still awaiting acknowledgement. watch( @@ -129,7 +138,7 @@ watch( ) function onSelect(mode: TrustMode) { - if (mode === state.value.effective) { + if (mode === state.value.effective && !state.value.isDefault) { // Already in effect — nothing to warn about and nothing to save. pendingWarnMode.value = null emit('confirmation-pending', false) diff --git a/frontend/src/utils/trustMode.ts b/frontend/src/utils/trustMode.ts index 411d6eed1..04e7ac16e 100644 --- a/frontend/src/utils/trustMode.ts +++ b/frontend/src/utils/trustMode.ts @@ -6,8 +6,9 @@ * contracts auto-approved only on a clean offline scan verdict (otherwise * held with the findings), manual = every change held; * - new-server admission: auto = admitted without quarantine or scan, - * scan = quarantined on add with a fail-closed automatic scan (the - * default), manual = quarantined on add for human review. + * scan = quarantined on add and released by a clean automatic scan, but + * only when set EXPLICITLY; an unset mode (the default tier) is quarantined + * until a person approves it, manual = quarantined on add for human review. * * The raw value delivered by `GET /api/v1/servers` may be absent (unset), a * value migrated from the legacy `auto_approve_tool_changes` / `skip_quarantine` @@ -94,7 +95,7 @@ export const TRUST_MODES: readonly TrustModeMeta[] = [ description: 'New tools and changes to approved tools are approved automatically when the offline security scan returns a clean verdict. Anything the scan flags — or cannot cover — is held for your review with the findings attached.', admissionNote: - 'At add time the server is quarantined and scanned automatically; a scan that is not clean — or cannot complete — keeps it quarantined (fail closed). The default.', + 'At add time the server is quarantined and scanned automatically. Chosen explicitly, a clean scan releases it; a scan that is not clean — or cannot complete — keeps it quarantined (fail closed). Left unset (the default), the server stays quarantined until you approve it.', warnsOnSelect: false, }, { diff --git a/frontend/tests/unit/add-server-manual.spec.ts b/frontend/tests/unit/add-server-manual.spec.ts index f663fc2bc..d93545f53 100644 --- a/frontend/tests/unit/add-server-manual.spec.ts +++ b/frontend/tests/unit/add-server-manual.spec.ts @@ -91,7 +91,6 @@ describe('ManualServerForm', () => { name: 'fs-server', protocol: 'stdio', enabled: true, - trust_mode: 'scan', command: 'npx', args_json: JSON.stringify(['-y', '@scope/server', '/tmp']), }) @@ -115,7 +114,6 @@ describe('ManualServerForm', () => { name: 'remote', protocol: 'http', enabled: true, - trust_mode: 'scan', url: 'https://api.example.com/mcp', }) expect(wrapper.emitted('added')).toEqual([['remote']]) @@ -146,6 +144,23 @@ describe('ManualServerForm', () => { expect(JSON.parse(payload.env_json as string)).toEqual({ API_TOKEN: 'fake-secret-value' }) }) + it('sends trust_mode scan only when the operator picks it', async () => { + vi.mocked(api.callTool).mockResolvedValue({ success: true, data: {} }) + vi.mocked(api.getServers).mockResolvedValue({ success: true, data: { servers: [] } }) + const wrapper = await mountManual() + + await wrapper.find('[data-test="manual-name-input"]').setValue('scan-server') + await wrapper.find('[data-test="manual-command-input"]').setValue('npx') + await wrapper.find('[data-test="trust-mode-option-scan"] input').setValue(true) + await wrapper.find('[data-test="manual-server-form"]').trigger('submit') + await flushPromises() + + expect(api.callTool).toHaveBeenCalledWith('upstream_servers', expect.objectContaining({ + name: 'scan-server', + trust_mode: 'scan', + })) + }) + it('requires confirmation before adding with auto trust mode and submits the chosen mode', async () => { vi.mocked(api.callTool).mockResolvedValue({ success: true, data: {} }) vi.mocked(api.getServers).mockResolvedValue({ success: true, data: { servers: [] } }) diff --git a/frontend/tests/unit/server-detail-auto-approve.spec.ts b/frontend/tests/unit/server-detail-auto-approve.spec.ts index 093b249bf..ac897880c 100644 --- a/frontend/tests/unit/server-detail-auto-approve.spec.ts +++ b/frontend/tests/unit/server-detail-auto-approve.spec.ts @@ -104,9 +104,9 @@ describe('ServerDetail — trust-mode control (Spec 088 US1, T009)', () => { expect(wrapper.findAll('[data-test^="trust-mode-option-"]')).toHaveLength(3) }) - it('presents scan as the current default when no trust mode is configured (spec 119)', async () => { + it('marks scan as the current default, with no mode checked, when no trust mode is configured (spec 119)', async () => { const { wrapper } = await mountDetail('config') - expect(isChecked(wrapper, 'scan')).toBe(true) + expect(isChecked(wrapper, 'scan')).toBe(false) expect(wrapper.find('[data-test="trust-mode-default-note"]').exists()).toBe(true) }) diff --git a/frontend/tests/unit/trust-mode-selector.spec.ts b/frontend/tests/unit/trust-mode-selector.spec.ts index 263dacf19..b4462a053 100644 --- a/frontend/tests/unit/trust-mode-selector.spec.ts +++ b/frontend/tests/unit/trust-mode-selector.spec.ts @@ -70,9 +70,9 @@ describe('TrustModeSelector — rendering (FR-002)', () => { }) describe('TrustModeSelector — current selection from the raw value (FR-001)', () => { - it('unset (field omitted) selects scan and indicates it is the default (spec 119)', () => { + it('unset (field omitted) checks no mode and marks scan as the current default (spec 119)', () => { const wrapper = mountSelector(undefined) - expect(isChecked(wrapper, 'scan')).toBe(true) + expect(isChecked(wrapper, 'scan')).toBe(false) expect(isChecked(wrapper, 'auto')).toBe(false) expect(isChecked(wrapper, 'manual')).toBe(false) const note = wrapper.find('[data-test="trust-mode-default-note"]') @@ -83,7 +83,7 @@ describe('TrustModeSelector — current selection from the raw value (FR-001)', it('empty string is also treated as unset (default scan)', () => { const wrapper = mountSelector('') - expect(isChecked(wrapper, 'scan')).toBe(true) + expect(isChecked(wrapper, 'scan')).toBe(false) expect(wrapper.find('[data-test="trust-mode-default-note"]').exists()).toBe(true) expect(wrapper.find('[data-test="trust-mode-invalid-note"]').exists()).toBe(false) }) @@ -155,6 +155,12 @@ describe('TrustModeSelector — selecting a safe mode emits immediately', () => expect(wrapper.find('[data-test="trust-mode-auto-confirm"]').exists()).toBe(false) }) + it('picking scan while unset emits scan (an explicit opt-in to clean-scan admission)', async () => { + const wrapper = mountSelector(undefined) + await select(wrapper, 'scan') + expect(wrapper.emitted('update:modelValue')![0]).toEqual(['scan']) + }) + it('re-selecting the mode already in effect does not emit', async () => { const wrapper = mountSelector('scan') await select(wrapper, 'scan') diff --git a/internal/config/config.go b/internal/config/config.go index 8e47b1e24..c594e8d7e 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -2234,6 +2234,20 @@ func (sc *ServerConfig) TrustModeDefaulted() bool { return sc != nil && sc.TrustMode == "" && sc.AutoApproveToolChanges == nil && !sc.SkipQuarantine } +// AdmitsOnCleanScan reports whether a clean admission scan may release this +// server from quarantine (spec 086 FR-011). Only an EXPLICIT trust_mode: scan +// opts in. The default tier resolves to scan for tool changes (spec 119) but +// server admission stays with a person: a server with no trust_mode stays +// quarantined until someone approves it. +// ScanAutoApprovedBy is the approver recorded when a clean admission scan +// releases an explicit trust_mode: scan server. The approval commit re-checks +// AdmitsOnCleanScan for it under the commit lock. +const ScanAutoApprovedBy = "scan-auto" + +func (sc *ServerConfig) AdmitsOnCleanScan() bool { + return sc != nil && TrustMode(sc.TrustMode) == TrustModeScan +} + func (sc *ServerConfig) EffectiveTrustMode() TrustMode { switch TrustMode(sc.TrustMode) { case TrustModeAuto, TrustModeScan, TrustModeManual: @@ -2258,8 +2272,10 @@ func (sc *ServerConfig) EffectiveTrustMode() TrustMode { return TrustModeAuto } // Spec 119: an unset trust_mode with no legacy flags is the DEFAULT tier, - // scan — quarantined and scanned on add, released on a clean verdict, tool - // changes approved on a clean verdict and held on findings. Operators who + // scan — tool changes approved on a clean verdict and held on findings. + // Server admission is NOT part of the default: only an explicit + // trust_mode: scan releases a quarantined server on a clean verdict + // (AdmitsOnCleanScan). Operators who // want every change reviewed by a person set trust_mode: manual explicitly. return TrustModeScan } diff --git a/internal/runtime/server_approval_commit_test.go b/internal/runtime/server_approval_commit_test.go index ba4430a12..93e5b0e16 100644 --- a/internal/runtime/server_approval_commit_test.go +++ b/internal/runtime/server_approval_commit_test.go @@ -232,3 +232,36 @@ func TestCommitServerApproval_BoundMatchFlipsStorageUnderTheLock(t *testing.T) { } assert.True(t, rvRecord(t, rt, "x").Disabled) } + +// A clean-scan admission (approver config.ScanAutoApprovedBy) re-checks the +// trust mode inside the commit: only an explicit trust_mode: scan server is +// released; a manual or default-tier server stays quarantined even if the scan +// settled while it was still scan. +func TestCommitServerApproval_ScanAutoRequiresExplicitScan(t *testing.T) { + rt := newReviewRuntime(t, true) + setTrustMode := func(mode string) { + t.Helper() + _, err := rt.Commit("test_trust_mode", func(tx *CommitTx) error { + return tx.UpdateServer(rvServer, func(cur *config.ServerConfig) error { + cur.TrustMode = mode + return nil + }) + }) + require.NoError(t, err) + } + + for _, mode := range []string{string(config.TrustModeManual), ""} { + setTrustMode(mode) + save, calls := baselineSave(t, rt) + _, err := rt.CommitServerApproval(rvServer, nil, nil, config.ScanAutoApprovedBy, save) + require.Error(t, err, "trust_mode %q must not admit on a clean scan", mode) + requireNothingApproved(t, rt, calls) + } + + setTrustMode(string(config.TrustModeScan)) + save, calls := baselineSave(t, rt) + _, err := rt.CommitServerApproval(rvServer, nil, nil, config.ScanAutoApprovedBy, save) + require.NoError(t, err) + assert.Equal(t, int32(1), calls.Load()) + assert.False(t, rt.serverQuarantinedInStorage(rvServer)) +} diff --git a/internal/runtime/tool_approval_review.go b/internal/runtime/tool_approval_review.go index 8daf564df..0b0081df5 100644 --- a/internal/runtime/tool_approval_review.go +++ b/internal/runtime/tool_approval_review.go @@ -6,6 +6,7 @@ import ( "go.uber.org/zap" + "github.com/smart-mcp-proxy/mcpproxy-go/internal/config" "github.com/smart-mcp-proxy/mcpproxy-go/internal/storage" "github.com/smart-mcp-proxy/mcpproxy-go/internal/toolreview" ) @@ -277,6 +278,14 @@ func (r *Runtime) commitServerApproval(serverName string, inc int64, bound bool, promoted := 0 _, err := r.Commit("security_approve", func(tx *CommitTx) error { tx.EventExtra(map[string]any{"server": serverName, "quarantined": false}) + // A clean-scan admission was decided before this commit; the trust mode + // may have changed since. Re-check it here, under the same commit as the + // release, so a server that no longer opts in stays quarantined. + if by == config.ScanAutoApprovedBy { + if sc, ok := tx.Server(serverName); ok && (!sc.AdmitsOnCleanScan() || !sc.Quarantined) { + return fmt.Errorf("server %q no longer admits on a clean scan; it stays quarantined", serverName) + } + } if err := tx.SetQuarantined(serverName, false); err != nil { // A bound approval of a server removed since it was reviewed is a // replaced server (409 server_replaced), the same answer the diff --git a/internal/server/scan_admission_test.go b/internal/server/scan_admission_test.go index 2d118a976..030fea2d9 100644 --- a/internal/server/scan_admission_test.go +++ b/internal/server/scan_admission_test.go @@ -332,6 +332,18 @@ func TestMaybeAutoApproveScanSettled(t *testing.T) { assert.Equal(t, []string{"srv"}, fake.approvedServers()) }) + t.Run("default tier (no trust_mode) + clean: never approves (server release stays human)", func(t *testing.T) { + fake := newFakeSecurityScanner() + fake.summaries["srv"] = &scanner.ScanSummary{Status: "clean"} + defaulted := &config.ServerConfig{Name: "srv", Quarantined: true} + require.Equal(t, config.TrustModeScan, defaulted.EffectiveTrustMode(), "precondition: the default tier is scan") + s := newAdmissionTestServer(t, fake, defaulted) + + s.maybeAutoApproveScanSettled(context.Background(), "srv") + + assert.Empty(t, fake.approvedServers(), "only an explicit trust_mode: scan admits a server on a clean scan") + }) + t.Run("missing config entry: never approves", func(t *testing.T) { fake := newFakeSecurityScanner() fake.summaries["ghost"] = &scanner.ScanSummary{Status: "clean"} @@ -419,6 +431,15 @@ func TestMaybeStartAdmissionScans(t *testing.T) { assert.Empty(t, fake.startedScans()) }) + t.Run("default tier (no trust_mode): no admission scan", func(t *testing.T) { + fake := newFakeSecurityScanner() + s := newAdmissionTestServer(t, fake, &config.ServerConfig{Name: "srv", Quarantined: true}) + + s.maybeStartAdmissionScans(context.Background()) + time.Sleep(50 * time.Millisecond) + assert.Empty(t, fake.startedScans()) + }) + t.Run("already scanned: never kicks a scan", func(t *testing.T) { fake := newFakeSecurityScanner() fake.summaries["srv"] = &scanner.ScanSummary{Status: "scanning"} diff --git a/internal/server/scan_informational.go b/internal/server/scan_informational.go index fada1c0df..3967178d2 100644 --- a/internal/server/scan_informational.go +++ b/internal/server/scan_informational.go @@ -84,8 +84,10 @@ func isTerminalScanStatus(status string) bool { // gate — and the settle-driven auto-approval behind it — is responsible for this // server. Those servers must NOT be picked up by the informational path. // -// The rule is deliberately the blunt one: EVERY trust_mode:"scan" server belongs -// to the gating path, whatever its quarantine state or approval history. +// The rule is deliberately the blunt one: EVERY explicit trust_mode:"scan" server +// belongs to the gating path, whatever its quarantine state or approval history. +// A server with no trust_mode (the default tier, which resolves to scan for tool +// changes) is never admitted by a scan, so it is the informational path's. // // Narrower predicates were tried and are unsafe, because every input the settle // handler gates on is MUTABLE while a scan is in flight, and @@ -132,7 +134,7 @@ func scanModeAdmissionOwns(sc *config.ServerConfig) bool { if sc == nil { return false } - return sc.EffectiveTrustMode() == config.TrustModeScan + return sc.AdmitsOnCleanScan() } // informationalScansEnabled resolves the security.auto_baseline_scan kill switch diff --git a/internal/server/scan_informational_test.go b/internal/server/scan_informational_test.go index 531f005a5..15e15d05f 100644 --- a/internal/server/scan_informational_test.go +++ b/internal/server/scan_informational_test.go @@ -583,9 +583,9 @@ func TestScanModeAdmissionOwns(t *testing.T) { {"scan+not quarantined+baseline", &config.ServerConfig{TrustMode: "scan"}, true, true}, {"manual+quarantined", &config.ServerConfig{TrustMode: "manual", Quarantined: true}, false, false}, {"auto+quarantined", &config.ServerConfig{TrustMode: "auto", Quarantined: true}, false, false}, - // Spec 119: an unset trust_mode is the default tier, scan, so the - // admission gate owns it. - {"empty trust mode (scan, the default) + quarantined", &config.ServerConfig{Quarantined: true}, false, true}, + // An unset trust_mode is the default tier: scan for tool changes, but a + // scan never admits the server, so the informational path owns it. + {"empty trust mode (the default) + quarantined", &config.ServerConfig{Quarantined: true}, false, false}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { diff --git a/internal/server/server.go b/internal/server/server.go index 2d01762e4..0c8e6a23f 100644 --- a/internal/server/server.go +++ b/internal/server/server.go @@ -988,9 +988,11 @@ func (s *Server) maybeAutoApproveScanSettled(ctx context.Context, serverName str return } mode := sc.EffectiveTrustMode() - // Cheap trust-mode gate before the (cached) verdict read: only scan-mode, - // still-quarantined servers can possibly auto-approve. - if mode != config.TrustModeScan || !sc.Quarantined { + // Cheap trust-mode gate before the (cached) verdict read: only an explicit + // trust_mode: scan, still-quarantined server can possibly auto-approve. The + // default tier (no trust_mode) also resolves to scan, but its admission + // stays with a person. + if !sc.AdmitsOnCleanScan() || !sc.Quarantined { return } // Spec 101 US3: a post-activation re-protection job analyses the stored @@ -1027,7 +1029,7 @@ func (s *Server) maybeAutoApproveScanSettled(ctx context.Context, serverName str zap.String("verdict", verdict)) return } - if err := scanSvc.ApproveServer(ctx, serverName, false, "scan-auto"); err != nil { + if err := scanSvc.ApproveServer(ctx, serverName, false, config.ScanAutoApprovedBy); err != nil { // ApproveServer's own hard-tier/missing-report gate can reject; that is the // intended fail-closed outcome, not a fatal error. Log and leave quarantined. s.logger.Warn("auto-approve of scan-mode server after green scan was rejected", @@ -1118,7 +1120,7 @@ func (s *Server) maybeStartAdmissionScan(ctx context.Context, sc *config.ServerC if sc == nil || scanSvc == nil { return } - if sc.EffectiveTrustMode() != config.TrustModeScan || !sc.Quarantined { + if !sc.AdmitsOnCleanScan() || !sc.Quarantined { return } // Already scanned (scanning/clean/failed/…): the admission scan already ran From 91d1d364afacfe53423ec367d971595f1a3fd686 Mon Sep 17 00:00:00 2001 From: Algis Dumbris Date: Fri, 30 Oct 2026 15:37:34 +0200 Subject: [PATCH 2/2] test(cli): best-effort cleanup of the chdir dir in TestLoadersOffLinuxKeepCreatingHomeConfig Windows CI failed TempDir's RemoveAll on the former cwd ("being used by another process") twice on #1802. --- cmd/mcpproxy/packaged_install_test.go | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/cmd/mcpproxy/packaged_install_test.go b/cmd/mcpproxy/packaged_install_test.go index 7abad64f9..88e1ff81d 100644 --- a/cmd/mcpproxy/packaged_install_test.go +++ b/cmd/mcpproxy/packaged_install_test.go @@ -172,7 +172,14 @@ func TestLoadersOffLinuxKeepCreatingHomeConfig(t *testing.T) { home := t.TempDir() t.Setenv("HOME", home) t.Setenv("USERPROFILE", home) // os.UserHomeDir on Windows - t.Chdir(t.TempDir()) + // Not t.TempDir: on Windows CI the former cwd can stay locked for a moment + // after t.Chdir restores it, and TempDir's RemoveAll then fails the test. + cwd, err := os.MkdirTemp("", "mcpproxy-cwd-") + if err != nil { + t.Fatal(err) + } + t.Cleanup(func() { _ = os.RemoveAll(cwd) }) // best effort; runs after the chdir is undone + t.Chdir(cwd) if _, err := loadUpstreamConfig(); err != nil { t.Fatalf("load: %v", err)