[Security 4/9] MCP tools require confirmation unless positively known-safe

[amal66]

[Security 4/9] MCP tools require confirmation unless positively known-safe

Part of the split of #227 into single-topic PRs. Index: tracking comment on #227.

TL;DR

Flip the tool-confirmation default. Previously a connector tool ran without confirmation unless it declared itself destructive. Now a tool requires confirmation unless it is positively known-safe — i.e. it explicitly claims readOnlyHint: true, is not flagged destructive, and is not open-world. Anything absent or ambiguous is gated.

Risk to user data

Severity: medium–high, and this is a key defense-in-depth layer behind prompt injection (see [Security 5/9], spotlighting). Tool annotations (readOnlyHint / destructiveHint / openWorldHint) are advisory and controlled by the external MCP server — a hint, not a guarantee. If the LLM is tricked (or a connector is malicious/mis-annotated) into calling a side-effecting tool, the cost in a legal product is severe: exfiltrating a privileged document, mutating a matter, hitting an unknown external system. Requiring positive proof-of-safety means a bad or missing annotation fails toward a confirmation click, not silent execution.

Flows affected

• MCP tool dispatch confirmation gating (toolRequiresConfirmation in mcp/client.ts).
• User sees a confirmation prompt for any tool not provably read-only+closed-world.

Attack precedent

This is the "human in the loop for consequential actions" mitigation from the OWASP Top 10 for LLM Applications (excessive agency / insecure output handling). The general lesson — never trust a security decision to a field the other side controls — is the same failure behind capability-confused deputies.

Possible fixes, and what we chose

Option	Verdict
Trust destructiveHint (old policy)	A poorly- or maliciously-annotated tool omits the flag and runs unconfirmed. Rejected.
Gate on openWorldHint	Almost every useful connector (Gmail, Slack, GitHub) is "open world", so this disables everything. Rejected as the sole signal.
Confirm everything, always	Safe but so much friction that users habituate and click through — which trains them to approve blindly. Rejected.
Known-safe allowlist: confirm unless readOnlyHint===true && !destructive && !openWorld	Chosen. The default flips toward safety; genuinely read-only, closed-world tools stay frictionless.

flowchart TD
    T["tool call requested"] --> Q{readOnlyHint === true<br/>AND not destructive<br/>AND not open-world?}
    Q -- yes --> Run["run without confirmation"]
    Q -- "no / missing / ambiguous" --> Ask["require user confirmation"]
    Ask -- approved --> Run
    Ask -- denied --> Stop["do not run"]

Loading

The subtlety encoded in the tests: readOnlyHint must be explicitly true — merely absent is treated as untrusted, so a tool that says nothing is gated, not trusted.

What's in this PR

• backend/src/lib/mcp/client.ts — toolRequiresConfirmation inverted to known-safe.
• Tests: confirmation.test.ts (9 tests covering each annotation combination, incl. the missing-hint case).

Reading

OWASP Top 10 for LLM Applications · Confused deputy problem

[CLAassistant]

All committers have signed the CLA.

[amal66]

Converting to draft — an adversarial re-review I ran found a confirmed regression: since no runtime confirmation flow exists yet, requires_confirmation=true acts as a permanent lockout (sync force-disables flagged tools, the toggle rejects enabling them), so defaulting to it would silently disable most existing connector tools on their next sync, with no backfill for already-synced rows. I'll rework this either as an explicit "default-disable unknown tools" policy or with a real confirmation flow before asking for review. Please skip this one for now.

[willchen96]

I don’t think this should merge in its current form. I found three blocking issues:

1. There is no user-confirmation flow. toolRequiresConfirmation() sets requires_confirmation, but tools with that value are then disabled during refresh, their UI toggle is disabled, the backend rejects attempts to enable them, and both tool discovery and execution filter them out. The user never receives an approval prompt and has no way to approve a call. In practice, “confirmation required” currently means “permanently unavailable.”

2. The MCP defaults are interpreted incorrectly. The MCP specification says an omitted openWorldHint defaults to true. However, the new implementation treats a missing value like false, so { readOnlyHint: true } is allowed to run automatically. If the policy requires a positively known closed-world tool, it must require openWorldHint === false explicitly, and the corresponding test should be reversed.

3. External annotations are not proof that a tool is safe. The MCP server controls readOnlyHint, destructiveHint, and openWorldHint, and a malicious or incorrectly implemented server can lie. Custom or untrusted connectors should require per-call approval regardless of their annotations. Automatic execution should additionally require a locally controlled trust decision or reviewed allowlist.

To match the PR description, the implementation needs a real pending-call approval flow: store the exact proposed tool and arguments, show them to the user, bind approval to that user/chat/payload, make it short-lived and single-use, and execute only after approval.

Until that exists, the behavior should be described as “blocked by policy,” not “confirmation required.” The narrower classification fix should also require an explicit closed-world declaration:

const annotationSafe =
    annotations?.readOnlyHint === true &&
    annotations?.openWorldHint === false &&
    annotations?.destructiveHint !== true;

The backend build and test suite pass, but the tests currently validate the incorrect missing-openWorldHint behavior and do not test an end-to-end user approval because no such path exists.