Repository navigation
fix: bind approvals to the reviewed tool definitions (Spec 117 PR-2; #1566) - #1605
Merged
Merged
Conversation
…ons (Spec 117 PR-2) The review payload reports a per-tool review fingerprint (current_hash: rv1- over server, tool, the approval-hashed contract and the four raw safety hints; the rug-pull hash is unchanged). ApproveToolsReviewed and ApproveAllToolsReviewed validate expected_hashes on records read under the per-server approval lock and return toolreview.StaleToolReviewError with nothing written on a mismatch. CommitServerApproval is now one Spec 116 config commit: its approval-lock hook validates the binding, records the baseline marker, runs the integrity baseline + blocks commit and promotes the pending records present at that moment, and the same lock hold covers the storage quarantine flip. FinishServerApproval runs the unlocked tail without the post-unquarantine promote-all, so a tool filed after the commit stays pending (FR-012). Related #1566
…review_out_of_date (Spec 117 PR-2)
The scanner's ApproveServerReviewed passes the binding to the runtime's
one-section server approval through the new ReviewedServerApprover
capability (the dangerous-scan gate still answers first); ApproveServer and
ApproveServerWithBlocks are its unbound forms. A binding is never dropped:
without the capability or a runtime it fails closed.
Both REST approve handlers accept expected_hashes (absent/null = unbound,
{} = bound to the empty snapshot) and map StaleToolReviewError to HTTP 409
{error_code: review_out_of_date, stale_tools: {changed, unreviewed,
missing_blocks}}; the message says "out of date" and never "dangerous",
as the macOS tray (#1572) expects. A bound duplicate tool name is counted
once. Swagger regenerated.
Related #1566
…view (Spec 117 PR-2)
mcpproxy review approve sends expected_hashes built from the review the
operator saw: every reviewed tool on a quarantined server ({} for an empty
review), the selected or held tools on a trusted server, unbound only when
the core reports no fingerprints. --from-review binds to a saved
'review show -o json' of the same server (checked before any request); the
interactive prompt lists the tools the approval makes callable. A 409 out of
date exits 1 with the core's message and a hint; -o json prints the
envelope. Refusals no longer print cobra's usage text.
Related #1566
… review (Spec 117 PR-2)
The review screen sends the click-time fingerprints of every reviewed tool
with a server approval ({} for an empty or not-captured review, unbound
only for a review without fingerprints) and a per-tool approve sends that
tool's fingerprint. The force retry re-sends the binding of the attempt that
answered dangerous. On 409 out of date it reloads the review, keeps the
operator's explicit choices that still apply and shows the core's message
beside the controls, never the error panel or the force dialog.
Related #1566
REST (current_hash, expected_hashes, 409 contract, compatibility matrix), CLI review approve (--from-review, binding, exit codes), the security quarantine page, CLAUDE.md Recent Changes, ROADMAP, and the Spec 117 tasks/research notes for PR-2 (implementation notes and deviations). Related #1566
Deploying mcpproxy-docs with
|
| Latest commit: |
c0866fb
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://16d8dc04.mcpproxy-docs.pages.dev |
| Branch Preview URL: | https://fix-117-pr2-review-binding.mcpproxy-docs.pages.dev |
Contributor
📦 Build ArtifactsWorkflow Run: View Run Available Artifacts
How to DownloadOption 1: GitHub Web UI (easiest)
Option 2: GitHub CLI gh run download 38322049766 --repo smart-mcp-proxy/mcpproxy-go
|
|
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
…2 step 8 status (Spec 117 PR-2, Sol r1)
expectedHashes built the binding in a plain {} keyed by upstream-controlled
tool names; a tool named __proto__ hit the inherited setter and dropped out
of the JSON, so the core refused every approval of it as out of date. Build
it on a null prototype. T218: L2 step 8 ran through the tray client code
against the fixed core; the rendered-sheet run stays open.
…rds follow colliding and raw-hint captures (Spec 117 PR-2, Sol r2) - reviewFingerprint hashes server, tool, CurrentHash, description, normalized input/output schema and raw hints, each length-prefixed: two contracts that share the |-joined storage hash no longer share a fingerprint. The rug-pull hash is unchanged (its own collision is #1606). - refreshHeldContractSnapshot keeps a changed record only when hash AND every field match, and compares the raw four hints (FR-017). - Tests: colliding contracts (named, approve-all, server approval, changed record), explicit-default hint, approve-all over a changed record, and a capture injected between validation and the writes (reviewBindingValidated seam). - Spec/data-model/research/contract/tasks and REST docs: rv2.
…request schemas (Spec 117 PR-2, Sol r3) swag v2 cannot emit a JSON Schema type union, so the two expected_hashes fields carry an x-nullable marker and make swagger runs cmd/oas-nullable to rewrite it into type: [object, "null"] in swagger.yaml and docs.go. A schema-validating client now accepts the explicit null the handlers already treat as an unbound request, and still rejects strings/arrays.
…nding # Conflicts: # CLAUDE.md # ROADMAP.md # docs/api/rest-api.md # frontend/src/components/ReviewScreen.vue # frontend/src/services/api.ts # internal/httpapi/security_scanner.go # internal/httpapi/server.go # internal/runtime/tool_baseline.go # internal/security/scanner/service.go # internal/server/server.go # oas/docs.go # oas/swagger.yaml
…n capture (single locked approval section); resync evidence
…eplaced, not not-found (Spec 117 PR-2 review)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Scope
Spec 117 PR-2 (#1566): approvals are bound to the tool definitions the operator reviewed. PR-1 (#1604, durable baseline) is already on main; capture ordering (PR-3, #1565) is not in this PR.
GET /api/v1/servers/{id}/reviewreportscurrent_hashper tool:rv1-+ sha256 over the server, the tool, the approval-hashed contract (description, input and output schema) and the four safety hints by raw value. The rug-pull hash is unchanged. Golden vectors are pinned ininternal/runtime/testdata/review_fingerprint_vectors.json.POST .../security/approveandPOST .../tools/approve(toolsandapprove_all) acceptexpected_hashes. Absent ornullmeans unbound (legacy).{}binds to the empty snapshot. The check runs on records read under the per-server approval lock. A mismatch returns HTTP 409 witherror_code: review_out_of_date,stale_tools: {changed, unreviewed, missing_blocks}, and a message that contains "out of date" and never "dangerous". Nothing is written.mcpproxy review approve(new--from-review <file>) send the binding. The macOS tray (fix(tray): per-server review sheet, click-time approval decisions, review-bound approvals (#1569) #1572) already does, and its contract is honoured unchanged.Design decisions and deviations from the spec
Runtime.CommitServerApprovalrunsCommit("security_approve")withSetQuarantined(false)and anUnderApprovalLockhook, the integration point Spec 116 left for this spec. The hook does validate → marker → integrity baseline + blocks → promote. The commit then writes the config file and the storage row before it releases the lock. The spec's D5 asked only for the storage flip inside the section. With Spec 116 the whole flip, including the config file and the live config, is one step.FinishServerApprovalis what remains: the activity record andHandleUpstreamServerChange. The plannedQuarantineServersplit (finishQuarantineChange) is not needed.QuarantineServeris unchanged and keeps its promote-pending behaviour for the non-review unquarantine paths (A7).approveToolsLockedalways did.refreshHeldContractSnapshotkeeps a changed record on the live definition. The output-schema backfill also refreshes the hints. T205b and T205c are therefore regression tests, not failing-first tests. The T205c wording "still approved after the backfill" predates RT-03: a hint flip now goes down the rug-pull path. The test asserts what FR-017a protects: the review shows the captured hints, and a binding to the old hints is refused.--from-review. It accepts the data object thatreview show -o jsonprints (the CLI drops the envelope) as well as the REST envelope. A file for another server, an error envelope or a malformed file exits 1 before any request. The trusted path binds--toolsto the named tools and the defaultapprove_allto every pending or changed tool. An empty review sends{}.review approveno longer prints cobra's usage text for a refusal from the core.Lock order
configCommitMu(L0, Spec 116 commit) → per-server approval lock (L1, taken by the commit for the server; the hook runs under it) → storage / bbolt.ApproveToolsReviewedandApproveAllToolsReviewedtake only L1. Nothing under L1 takes L0 orcaptureSnapMu, or callsSaveConfiguration,HandleUpstreamServerChangeor a reindex. Events, the reindex andFinishServerApprovalrun after the lock is released.Compatibility
mcpproxy security approve, the Tools page bulk approve and the MCPquarantine_securityapproval operations stay unbound (follow-up O3). Both editions behave the same.Verification
--from-reviewand REST binding tests fail on main by construction. The live main run shows the field ignored with HTTP 200.go test -race -tags server -timeout 30m -skip "E2E|Binary|MCPProtocol|TestInfoEndpoint|TestGracefulShutdownNoPanic|TestSocketInfoEndpoint" ./internal/...: 81 packages ok.go test -race ./cmd/mcpproxy/ok. The new runtime tests pass with-count=5.go test ./tests/tpa-attack-lab/ok.make buildok. golangci-lint v2, bare and with--build-tags server: no findings in the changed code../scripts/test-api-e2e.shon a scratch port: 69 passed, 0 failed, 1 skipped (an external registry was unreachable).security/approvereturned 200 and activated a definition that had turned destructive. Unreviewed tools and staletools/approve/approve_allwere approved too.{}.--from-reviewexited 1 with the hint, and-o jsonprinted the 409 envelope.tools/call.Fixes #1566
🤖 Generated with Claude Code