fix(mcp): gate MCP write tools behind the user confirmation prompt - #2846
Conversation
github-actions
Bot
added
documentation
|
Verdict: Approve This closes a real confirmation-gate bypass: the gate matched a static set of tool-name strings, but MCP tools register under server-chosen The design is sound and honest about its own limits — the code and docs both state plainly this defends against untrusted content, not a malicious server (which could just name its write tool innocuously). Vetting the server stays the control for that. Fail-closed defaults, unattended-mode denial, and unprefixed-alias resolution are all covered by tests. No blocking issues. One nit below (a misleading test docstring). No security concern to escalate — this is the security hardening. Real-world evidence
Evidence supports merge. 🔍 Technical detailsStrengths
🟢 Minor — inaccurate test docstring re: plural verbs (
|
MCP tools bypassed the confirmation guardrail entirely. The gate matches tool names against a fixed set, but MCP tools register under names chosen by the connected server (mcp_<server>_<tool>), so no MCP tool could ever match it. Every write- and destructive-capable MCP tool executed with no prompt, which turned untrusted content the agent ingests into a path to an unconfirmed write. The classification now travels with the registry entry instead of being looked up by name. MCPTool captures the MCP-spec annotations, to_gaia_format stamps a requires_confirmation flag, and the gate unions the static name set with that flag. A tool is exempt only when the server declares readOnlyHint true and the name carries no mutating verb; a missing, false, or non-boolean hint all require confirmation. Entries under the mcp_ prefix carrying no flag fail closed, so a registration path that forgets to classify cannot silently reopen the gate. Resolving an unprefixed alias to its canonical name already happened before the gate, so calling write_file instead of mcp_fs_write_file no longer skips the check either. Reported via responsible disclosure.
e9e8e2f to
eb21f07
Compare
|
🟡 One spec example block wasn't updated when The PR correctly removes 🔍 Technical details
# current (stale)
{
"name": "mcp_filesystem_read_file",
...
"atomic": True,
"_mcp_server": "filesystem",
"_mcp_tool_name": "read_file" # ← removed from code; no longer emitted
}Should match what # correct
{
"name": "mcp_filesystem_read_file",
"display_name": "read_file (filesystem)",
...
"atomic": True,
"requires_confirmation": True, # ← always present now
"_mcp_server": "filesystem"
# _mcp_tool_name gone
}The |
The _TOOL_REGISTRY example block still listed _mcp_tool_name, which to_gaia_format has not emitted for some time, and omitted the new requires_confirmation key. Two blocks in the same file described the same structure differently.
|
Good catch — fixed in a4a6a93. The |
…n note The tool-annotations link pointed at an unversioned spec path that 404s; CI's external-link check caught it. Also corrects the note that said the CLI proceeds automatically — since #2210 it prompts on a TTY and denies when there is no terminal to ask.
The name-resolution fixtures hand-build mcp_-prefixed registry entries. Those now fail closed without an explicit verdict, so every call was denied before resolution was reached. Stub them the way to_gaia_format stamps a tool the server proved read-only, keeping these tests about naming.
#2853) CodeAgent's orchestrator ran tools by pulling the callable straight out of `_TOOL_REGISTRY`, skipping `Agent._execute_tool` and with it the user-confirmation guardrail — so `run_shell_command`, `write_file` and any MCP tool registered by a co-resident agent in the same process all executed with no prompt during orchestrated runs. Orchestrated calls now take the same path as the agent loop's, so the same tools prompt whether they are invoked directly or through a checklist. Same vulnerability class as #2846, which fixes it for MCP tools in the base agent; this is the CodeAgent half. Reported via responsible disclosure. A denial is treated as a user decision rather than a transient fault: it is never retried, it stops the checklist, and the orchestrator stops replanning instead of queuing another prompt for the same work. Result parsing now reads the base agent's `status` field alongside the legacy `success` key, so a denied or errored call can no longer be mistaken for success by tools that return a bare payload dict. Also fixes warnings being dropped when a checklist exits early — a stopped run now still reports what the completed items produced. ## Test plan - [ ] `python -m pytest hub/agents/code/python/tests/test_tool_executor_confirmation.py -q` — 16 tests, 19 subtests - [ ] Confirm a denied gated tool does not execute (asserted via side-effect, not just the return value) - [ ] Confirm a denied item stops the checklist and is not retried by the error handler - [ ] Confirm the orchestrator halts replanning after a denial rather than re-prompting - [ ] Run a normal `gaia-code` generation task through the orchestrator and confirm non-gated tools are unaffected - [ ] `python util/lint.py --all` --------- Co-authored-by: Ovtcharov <kovtchar@amd.com>
…#2851) Every MCP tool in the C++ SDK ran with no confirmation. `connectMcpServer()` stamped the registry's default policy — `ALLOW` — onto each discovered tool, so the user-confirmation guardrail could never fire for them, and untrusted content the agent ingests could drive an `mcp_*_delete_file` or `mcp_*_start_process` call straight to execution. Discovered MCP tools are now classified fail-closed and registered `CONFIRM` unless the server proves the tool read-only, so the C++ SDK matches the guardrail the Python SDK enforces. Same vulnerability class as amd#2846, which fixes it in the Python SDK; this is the C++ half. Reported via responsible disclosure. A tool is exempt only when `annotations.readOnlyHint` is the JSON boolean `true` **and** its name carries no state-changing verb. Missing annotations, a non-object value, the string `"true"`, or a read-only claim contradicted by a name like `delete_file` all require confirmation. The registry default can raise that verdict but never lower it. Note for headless hosts: agents in `silentMode` have no confirmation callback, so gated MCP tools are denied until the host installs one or pre-approves the tool. That is deliberate — nothing can answer for an absent human — but it is a behaviour change for unattended C++ hosts using MCP. ## Test plan - [ ] Build the C++ SDK and run `tests_mock` — 483 tests pass, including 24 in `MCPConfirmationTest` - [ ] `tests_mock --gtest_filter="*Confirm*:*Annotation*:*ReadOnly*"` passes - [ ] Confirm a denied MCP tool does not execute (asserted via side-effect, not just return value) - [ ] Confirm a server declaring `readOnlyHint: true` on a read-shaped tool still runs unprompted - [ ] Confirm `readOnlyHint: "true"` (string) and a non-object `annotations` both still require confirmation Co-authored-by: Ovtcharov <kovtchar@amd.com>
…r re-verification Merging current main brought 41 more commits (now 331 since v0.22.0). Re-verified every headline against the new pin on real hardware: all six hold. Folded in what that delta added and verified live — the confirmation gate now covers the terminal, the local API, and MCP tool calls (#2846/#2854, both proven fail-closed model-free); the security section gains enforced MCP --auth-token, shell-less MCP launch, the ~/.gaia write guard, and the CWE-89 SQL block (#2844/#2847/#2860); and the skills section reflects the verified create/import/sign/audit/migrate lifecycle, stated as opt-in since no agent loads skills by default (#2848). The C++ SDK and a skills-by- default claim were deliberately excluded — the former ships no user artifact, the latter is false.
2 participants
Connecting any MCP server to GAIA gave every one of its tools a free pass on the confirmation prompt — including file writes, repo deletes and shell execution. The guardrail matches a fixed list of tool names, and MCP tool names are supplied by the connected server, so no MCP tool could ever match it. That made a prompt injection in a document, email or web page the agent was asked to read a direct path to a write the user never approved. MCP tools now carry their own risk classification and prompt unless the server proves the tool is read-only, so they are gated the same way GAIA's own write tools are.
Reported via responsible disclosure. Reproduced on
mainbefore the fix and confirmed closed after, against a live MCP server.Builds directly on #2210: that change made consoles deny rather than answer for an absent human, and this one makes MCP tools reach that gate in the first place. The two together mean an injected MCP write is refused on the CLI and prompts in the Agent UI, instead of running silently on both.
A tool is exempt only when its server sets the MCP-spec
readOnlyHinttotrueand its name carries no mutating verb — so a server that ships no annotations gets no benefit of the doubt, and areadOnlyHint: trueon a tool nameddelete_fileis ignored. Note this defends against servers that annotate incorrectly, not against a malicious server, which could simply name its write tool something innocuous; vetting the server remains the control for that.Test plan
python -m pytest tests/unit/mcp/client/test_mcp_tool_risk_classification.py tests/unit/agents/test_mcp_tool_confirmation_gate.py -q(65 tests)python -m pytest tests/unit/mcp/ tests/unit/agents/ -q— no new failures vs. the pre-existing baselinereadOnlyHint: trueon a read tool, confirm it still runs with no promptwrite_filerather thanmcp_<server>_write_file) is also gatedpython util/lint.py --all