Skip to content

Commit 0719fae

Browse files
authored
fix(quarantine): server admission stays human under the default trust tier (#1802)
## Summary Since #1740 (Spec 119) made `scan` the default trust mode, the Spec 086 clean-scan admission gate has been releasing **every** server added without a `trust_mode`, including one added with `quarantined: true`. `TestE2E_QuarantineConfigApply` has failed on main since then (bisected to #1740). Maintainer decision: server admission stays human. Only an explicit `trust_mode: "scan"` auto-admits; the default tier only auto-approves clean tool changes. - `ServerConfig.AdmitsOnCleanScan()` is true only for an explicit `trust_mode: "scan"`. - `maybeAutoApproveScanSettled`, `maybeStartAdmissionScan` and `scanModeAdmissionOwns` use it. Default-tier servers get the informational (non-gating) baseline scan instead of the admission one. - The approval commit re-checks `AdmitsOnCleanScan` (and that the server is still quarantined) for `scan-auto` approvals inside the same commit, so a `trust_mode` changed while the scan was running cannot release the server. - Web UI: the manual add form sends `trust_mode` only when one is picked. The selector checks no radio for an unset mode (Scan keeps the "current default" badge), so Scan can be chosen explicitly. Previously the form always sent `scan`. - Docs (`intro.md`, `configuration.md`, `security-quarantine.md`), CLI `--trust-mode` help and the CHANGELOG Spec 119 entry describe the rule. ## Tests - New: default-tier server is never auto-approved and gets no admission scan (`scan_admission_test.go`); `scanModeAdmissionOwns` hands it to the informational path; `TestCommitServerApproval_ScanAutoRequiresExplicitScan` (manual and unset refused, explicit scan released). Each fails without the fix. - Frontend: explicit Scan is sent when picked; unset form omits `trust_mode`; selector emits `scan` from the unset state. - Locally: `internal/server` (server tag, CI skip regex), `internal/runtime`, `internal/config`, `cmd/...`, `internal/qapilot/...`, `httpapi`; `TestE2E_QuarantineConfigApply` passes again; `./scripts/test-api-e2e.sh` all passed; vitest 2791 passed (the 2 `activity-views.spec.ts` failures also fail on main locally); `vue-tsc`; both golangci-lint configs report 0 new issues. - Cross-reviewed with opencode Sol 6.1: round 1 found the commit-time race and the add-form opt-in gap (both fixed), round 2 CLEAN.
1 parent 5a116b7 commit 0719fae

19 files changed

Lines changed: 162 additions & 36 deletions

‎CHANGELOG.md‎

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -28,9 +28,11 @@ Releases follow [Semantic Versioning](https://semver.org/).
2828
changes behind an unchanged definition. **Migration:** set `trust_mode: "manual"` on any server whose
2929
tool changes you want a person to review; nothing changes for servers with an explicit `trust_mode` or
3030
`auto_approve_tool_changes`. A record whose stored annotation snapshot differs from the live hints in
31-
dispatch tier only (a server last seen before hints were captured) is held once for review. New servers
32-
added with no trust mode are quarantined, scanned and released automatically on a clean verdict, as
33-
`scan` always did.
31+
dispatch tier only (a server last seen before hints were captured) is held once for review. Server
32+
admission stays with a person: a new server added with no trust mode is quarantined and stays
33+
quarantined until someone approves it. Only an explicit `trust_mode: "scan"` lets a clean admission
34+
scan release a server, and the approval re-checks that under the commit lock. The Web UI add form sends
35+
a trust mode only when one is picked.
3436

3537
- **connect / disconnect:** `mcpproxy disconnect`, `DELETE /api/v1/connect/{client}`, and the Web UI
3638
and tray Disconnect now also revoke the client's credential (`client-<id>`), so a secret copied out

‎cmd/mcpproxy/packaged_install_test.go‎

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -172,7 +172,14 @@ func TestLoadersOffLinuxKeepCreatingHomeConfig(t *testing.T) {
172172
home := t.TempDir()
173173
t.Setenv("HOME", home)
174174
t.Setenv("USERPROFILE", home) // os.UserHomeDir on Windows
175-
t.Chdir(t.TempDir())
175+
// Not t.TempDir: on Windows CI the former cwd can stay locked for a moment
176+
// after t.Chdir restores it, and TempDir's RemoveAll then fails the test.
177+
cwd, err := os.MkdirTemp("", "mcpproxy-cwd-")
178+
if err != nil {
179+
t.Fatal(err)
180+
}
181+
t.Cleanup(func() { _ = os.RemoveAll(cwd) }) // best effort; runs after the chdir is undone
182+
t.Chdir(cwd)
176183

177184
if _, err := loadUpstreamConfig(); err != nil {
178185
t.Fatalf("load: %v", err)

‎cmd/mcpproxy/upstream_cmd.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -379,7 +379,7 @@ func init() {
379379
upstreamAddCmd.Flags().StringVar(&upstreamAddTransport, "transport", "", "Transport type: http or stdio (auto-detected if not specified)")
380380
upstreamAddCmd.Flags().BoolVar(&upstreamAddIfNotExists, "if-not-exists", false, "Don't error if server already exists")
381381
upstreamAddCmd.Flags().BoolVar(&upstreamAddNoQuarantine, "no-quarantine", false, "Don't quarantine the new server (use with caution)")
382-
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)")
382+
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)")
383383
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)")
384384
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)")
385385

‎docs/configuration.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -453,7 +453,7 @@ the error names the field.
453453
| `oauth` | object | No | OAuth configuration (see [OAuth Configuration](#oauth-configuration)) |
454454
| `isolation` | object | No | Per-server Docker isolation settings (see [Docker Isolation](#docker-isolation)) |
455455
| `enabled` | boolean | No | Enable/disable server (default: `true`) |
456-
| `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. |
456+
| `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. |
457457
| `reconnect_on_use` | boolean | No | When `true`, tool calls to a disconnected server trigger an immediate reconnect attempt (15s timeout) before failing (default: `false`) |
458458
| `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. |
459459
| `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). |

‎docs/features/security-quarantine.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -593,7 +593,7 @@ new-server admission and tool-change approval (superseding the binary
593593
| Mode | Add time | Tool changes |
594594
|------|----------|--------------|
595595
| `auto` | Admitted without quarantine or scanning | Trusted without scanning (rug-pull risk) |
596-
| `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 |
596+
| `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 |
597597
| `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 |
598598

599599
Config field: per-server `trust_mode`; REST: `trust_mode` on

‎docs/intro.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -32,7 +32,7 @@ Instead of loading all tools from all servers into your AI client's context, MCP
3232

3333
### Security Quarantine
3434

35-
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.
35+
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.
3636

3737
### Docker Isolation
3838

‎frontend/src/components/ManualServerForm.vue‎

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -115,7 +115,7 @@ import TrustModeSelector from '@/components/TrustModeSelector.vue'
115115
import { resolveSecretFields, rollbackSecrets } from '@/composables/useSecretFields'
116116
import { useServersStore } from '@/stores/servers'
117117
import { serverDetailPath } from '@/utils/serverRoute'
118-
import { DEFAULT_TRUST_MODE, type TrustMode } from '@/utils/trustMode'
118+
import type { TrustMode } from '@/utils/trustMode'
119119
120120
const props = withDefaults(defineProps<{
121121
navigateAfterAdd?: boolean
@@ -140,7 +140,10 @@ const errorEl = ref<HTMLElement | null>(null)
140140
141141
const name = ref('')
142142
const protocol = ref<'stdio' | 'http'>('stdio')
143-
const trustMode = ref<TrustMode>(DEFAULT_TRUST_MODE)
143+
// Unset until the operator picks a mode: an unset trust_mode is the default
144+
// tier, which never releases the server from quarantine on a clean scan; an
145+
// explicit Scan does.
146+
const trustMode = ref<TrustMode | undefined>(undefined)
144147
const trustModeConfirmationPending = ref(false)
145148
const url = ref('')
146149
const command = ref('')
@@ -191,8 +194,8 @@ async function handleSubmit() {
191194
name: name.value,
192195
protocol: protocol.value,
193196
enabled: true,
194-
trust_mode: trustMode.value,
195197
}
198+
if (trustMode.value) serverData.trust_mode = trustMode.value
196199
if (protocol.value === 'http') {
197200
serverData.url = url.value
198201
if (Object.keys(resolved.headers).length > 0) serverData.headers_json = JSON.stringify(resolved.headers)

‎frontend/src/components/TrustModeSelector.vue‎

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,7 @@
1717
class="radio radio-primary radio-sm mt-0.5 shrink-0"
1818
:name="groupName"
1919
:value="m.mode"
20-
:checked="displayedMode === m.mode"
20+
:checked="checkedMode === m.mode"
2121
@change="onSelect(m.mode)"
2222
/>
2323
<span class="flex-1 min-w-0">
@@ -118,6 +118,15 @@ const pendingWarnMode = ref<TrustMode | null>(null)
118118
/** Tentative selection while a warning is open; otherwise the effective mode. */
119119
const displayedMode = computed<TrustMode>(() => pendingWarnMode.value ?? state.value.effective)
120120
121+
/**
122+
* The checked radio. An unset value (the default tier) checks none: it is not
123+
* the same as an explicit Scan, which also releases a quarantined server on a
124+
* clean scan, so the operator must be able to pick Scan to opt in.
125+
*/
126+
const checkedMode = computed<TrustMode | null>(() =>
127+
pendingWarnMode.value ?? (state.value.isDefault ? null : state.value.effective)
128+
)
129+
121130
// A value arriving from the parent (save, re-load, SSE refresh) supersedes any
122131
// selection still awaiting acknowledgement.
123132
watch(
@@ -129,7 +138,7 @@ watch(
129138
)
130139
131140
function onSelect(mode: TrustMode) {
132-
if (mode === state.value.effective) {
141+
if (mode === state.value.effective && !state.value.isDefault) {
133142
// Already in effect — nothing to warn about and nothing to save.
134143
pendingWarnMode.value = null
135144
emit('confirmation-pending', false)

‎frontend/src/utils/trustMode.ts‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -6,8 +6,9 @@
66
* contracts auto-approved only on a clean offline scan verdict (otherwise
77
* held with the findings), manual = every change held;
88
* - new-server admission: auto = admitted without quarantine or scan,
9-
* scan = quarantined on add with a fail-closed automatic scan (the
10-
* default), manual = quarantined on add for human review.
9+
* scan = quarantined on add and released by a clean automatic scan, but
10+
* only when set EXPLICITLY; an unset mode (the default tier) is quarantined
11+
* until a person approves it, manual = quarantined on add for human review.
1112
*
1213
* The raw value delivered by `GET /api/v1/servers` may be absent (unset), a
1314
* value migrated from the legacy `auto_approve_tool_changes` / `skip_quarantine`
@@ -94,7 +95,7 @@ export const TRUST_MODES: readonly TrustModeMeta[] = [
9495
description:
9596
'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.',
9697
admissionNote:
97-
'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.',
98+
'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.',
9899
warnsOnSelect: false,
99100
},
100101
{

‎frontend/tests/unit/add-server-manual.spec.ts‎

Lines changed: 17 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -91,7 +91,6 @@ describe('ManualServerForm', () => {
9191
name: 'fs-server',
9292
protocol: 'stdio',
9393
enabled: true,
94-
trust_mode: 'scan',
9594
command: 'npx',
9695
args_json: JSON.stringify(['-y', '@scope/server', '/tmp']),
9796
})
@@ -115,7 +114,6 @@ describe('ManualServerForm', () => {
115114
name: 'remote',
116115
protocol: 'http',
117116
enabled: true,
118-
trust_mode: 'scan',
119117
url: 'https://api.example.com/mcp',
120118
})
121119
expect(wrapper.emitted('added')).toEqual([['remote']])
@@ -146,6 +144,23 @@ describe('ManualServerForm', () => {
146144
expect(JSON.parse(payload.env_json as string)).toEqual({ API_TOKEN: 'fake-secret-value' })
147145
})
148146

147+
it('sends trust_mode scan only when the operator picks it', async () => {
148+
vi.mocked(api.callTool).mockResolvedValue({ success: true, data: {} })
149+
vi.mocked(api.getServers).mockResolvedValue({ success: true, data: { servers: [] } })
150+
const wrapper = await mountManual()
151+
152+
await wrapper.find('[data-test="manual-name-input"]').setValue('scan-server')
153+
await wrapper.find('[data-test="manual-command-input"]').setValue('npx')
154+
await wrapper.find('[data-test="trust-mode-option-scan"] input').setValue(true)
155+
await wrapper.find('[data-test="manual-server-form"]').trigger('submit')
156+
await flushPromises()
157+
158+
expect(api.callTool).toHaveBeenCalledWith('upstream_servers', expect.objectContaining({
159+
name: 'scan-server',
160+
trust_mode: 'scan',
161+
}))
162+
})
163+
149164
it('requires confirmation before adding with auto trust mode and submits the chosen mode', async () => {
150165
vi.mocked(api.callTool).mockResolvedValue({ success: true, data: {} })
151166
vi.mocked(api.getServers).mockResolvedValue({ success: true, data: { servers: [] } })

0 commit comments

Comments
 (0)