Skip to content

security: tool/server approvals are not bound to the reviewed definitions (add expected_hashes / review fingerprint) #1566

Closed

Description

@Dumbris

Problem

Approval is not bound to what the reviewer saw. Runtime.ApproveTools (internal/runtime/tool_quarantine.go:2019) sets ApprovedHash = CurrentHash at the moment it runs, approveBaselineToolsForServer (:2423) promotes every pending record, and the HTTP/CLI/Web/tray approve APIs (internal/httpapi/server.go:6850-6900, cmd/mcpproxy/review_cmd.go) accept only tool names / a block list - there is no expected_hashes or review revision in the contract.

Failure scenarios

  • Operator reviews definition A; discovery persists B (rug-pull) before the approval executes; B is approved.
  • A tool discovered after the GET (not in the submitted block list) is promoted by server approval; a selected pending tool whose definition changed is approved at its newer hash, and the response can report zero holds.
  • Annotations are excluded from the tool hash (tool_quarantine.go hash, CurrentAnnotations updated at :828-853 while status stays approved), so a tool reviewed as read-only can become destructive without invalidating the approval; this also applies to approved tools on a re-quarantined server.
  • Empty review: mcpproxy review approve --force on an empty inventory sends an unbound request, and a destructive tool captured while the confirmation prompt is open is promoted.
  • Scanner server approval validates at commit but unquarantines in a separate step (internal/security/scanner/service.go:1697, UnquarantineServer :127), leaving a commit-to-unquarantine window for annotation changes.

Suggested direction

Additive contract: clients send expected_hashes (name -> review fingerprint including safety annotations; {} for an empty review) and the core validates inside the per-server approval lock, rejecting with 409/stale-review and leaving additions/mismatches pending; unbound legacy requests stay accepted but keep additions held. Revalidate across the unquarantine step or perform it inside the same commit. Keep the rug-pull hash policy unchanged (separate review fingerprint).

Where Sol found it

ux02-r1-A.txt #1, ux02-r1-B.txt #1, ux02-r2-A.txt #2, ux02-r4-A.txt #2, ux02-r7-c2.txt #1, ux02-r10-backend.txt #1.

Related: #1567, #1569, #1568


Found by OpenCode Sol 6.1 cross-model review of the (now split) oversized PRs #1553 (UX-01) and #1555 (UX-02); verified still present on origin/main (704795c) by reading the cited sites. Pre-existing, deliberately out of scope of the minimal core PRs. Review transcripts: docs/qa/0.70.0-ux-2026-10-10/remediation/reviews/.

Metadata

Metadata

Assignees

No one assigned

    Labels

    area/proxyCore MCP proxy engine and tool routingkind/bugSomething isn't workingkind/securitySecurity fix or hardeningpriority/highShould be addressed in the current or next release

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions