Repository navigation
fix: durable server tool baseline so replaced inventories never auto-approve (Spec 117 PR-1; #1567) - #1604
Merged
Conversation
…approve (Spec 117 PR-1) A trusted server's baseline was inferred only from approved/changed tool records. A replaced inventory or an authoritative empty refresh deletes those records, so the next discovery pass treated the server as new and approved the replacement by auto-baseline; call_tool_destructive then reached the upstream. - storage: tool_baseline_decisions bucket (one marker per server), removed only together with the server's approval records (same bbolt tx). - toolreview: new leaf package with the BaselineDecision value type. - runtime: serverHasBaseline = marker OR approved/changed record; the inferred marker is back-filled before the pass's first write and a marker or listing failure aborts the pass before anything is published or written; the discovery marker is written after a pass that approved tools. - runtime/scanner/server: every security approval commits through Runtime.CommitServerApproval, which records the marker under the per-server approval lock before the integrity baseline (empty and blind reviews too). - copy: the blind-approval dialog and CLI prompt say later tools wait for review; docs Durable tool baseline section; Spec 117 design docs. Related #1567
Deploying mcpproxy-docs with
|
| Latest commit: |
6f593e7
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://0bf50ccf.mcpproxy-docs.pages.dev |
| Branch Preview URL: | https://fix-117-pr1-durable-baseline.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 38291075769 --repo smart-mcp-proxy/mcpproxy-go
|
… the StateView; correct the marker-failure docs (Spec 117 PR-1 review r1) - New TestToolBaselineMarkerFault_CaptureIsNotPublished drives commitDiscoveredToolsAt with a published seed capture and a non-nil capture plus an injected marker write failure (hint update and replaced inventory): published=false and the StateView tools, ToolsDiscovered, DiscoveryEpoch, records, last-good snapshot and capture stamp are all unchanged. The existing marker-fault tests passed a nil capture and never reached the publication branch. - docs/features/security-quarantine.md and CLAUDE.md: a failed marker write on a server's first trusted discovery is not an abort (its approved records establish the baseline and the next pass marks it); read/list failures and the back-fill write on a server with approved records abort the pass.
|
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
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-1 only (#1567): a durable per-server tool baseline, so a replaced inventory, an authoritative empty refresh or an empty or blind review never re-triggers the first-baseline auto-approval of tools nobody reviewed. Review binding /
expected_hashes(PR-2, #1566) and capture ordering (PR-3, #1565) are not in this PR.The Spec 117 design (
specs/117-review-bound-approvals/, from draft #1580) ships with this PR.ROADMAP.mdis regenerated.The bug on main. A trusted server auto-baselines
read_note. Its inventory is then replaced by the destructivewipe_all_notes. The removal pass deletesread_note's record, so the next pass finds no approved record. It treats the server as new and approveswipe_all_notesbyauto-baseline, andcall_tool_destructivethen reaches the upstream. An authoritative empty refresh, or a server approval of an empty review, leads to the same result.Design
tool_baseline_decisionskeyed by server name. Each value is{decided_at, source: discovery|server_approval|inferred, by}.decided_atis kept, and only aninferredsource is upgraded.DeleteServerToolApprovals(server removal) orPruneToolApprovalsNotIn(orphan prune, which also covers markers of servers that have no records).internal/toolreview: a new leaf package (stdlib only) that holdsBaselineDecision. It avoids the storage → scanner import cycle.serverHasBaseline= marker OR an approved/changed record. The record inference is kept, so no migration is needed and a downgrade is safe.resolveToolBaselineHeldlists the prior records and reads the marker. On an older store (records but no marker) it back-fillsinferredbefore the pass's first record write.errBaselineMarker. The pass is then aborted with no write of any kind. On main, a listing error was read as "no baseline", which auto-baselined the whole inventory.discoveryafter its loop. A failure there is logged and is not an abort: the records it just approved establish the baseline by inference, and the next pass back-fills the marker. An empty first inventory writes no marker (FR-025).Runtime.CommitServerApproval(server, blocks, expected, by, commit)is the PR-1 form of the Spec 117 commit section. Under the per-server approval lock it writes the marker (server_approval,by) and then runs the scanner's integrity-baseline save.ServerApprovalCommittercapability of its unquarantiner adapter, for both the plain branch and the…WithBlocksbranch. This covers empty and blind reviews andscan-auto.expectedis rejected; binding is PR-2.docs/features/security-quarantine.md, plusdocs/cli/review-commands.mdand a CLAUDE.md Recent Changes line.Deviations from the spec
These are recorded in
research.md, "PR-1 implementation notes". Main gained Spec 116 D15 after the design: discovery now publishes the snapshot, stamp and StateView inside the approval-lock hold, throughheld.held. When it fails, nothing of the capture is published, so T105's "restore the last-good snapshot and stamp" is unnecessary.commitDiscoveredToolsAtreports such a pass as eligible but not published, and the sweep re-lists it. Spec 116's per-server sweep publish replaces the batchRefreshToolsFromDiscoveryomission the design described.byis theApprovedByof the first approved record:auto-baseline, orauto,scan-approvedorauto-approve-changesunder a lifted gate.testHooks.toolBaselineFault), becauseRuntime.storageManageris the concrete*storage.Managerrather than an interface.Lock order
No new lock edges:
captureSnapMu→ per-server approval lock → storage (the marker read/back-fill happens inside the existing hold, beforeheld).WithToolApprovalLockhold, which covered the with-blocks branch only.QuarantineServer(false)→Runtime.Commit) still runs after unlock, as before.This composes with the per-server approval lock (#1571), Spec 116's commit section (#1590) and the TPA RT-01..07 holds (#1596). No RT path changed. Scan mode still never auto-approves a changed contract, and a new tool in scan mode still goes through the scan.
Compatibility
inferredon its first pass (verified live on an older store).unquarantine, the MCP op, config edits) are unchanged (A7).scan-autorecord the marker too. New tools then follow the scan-mode rule (a clean scan still auto-approves them), not the first-baseline rule.Verification
internal/storage/tool_baseline_decision_test.go(T102)internal/runtime/tool_baseline_durable_test.go(T104 a–e, T106)internal/runtime/tool_baseline_marker_fault_test.go(T104f: replacement and authoritative-empty aborts, a hint update that writes nothing, a listing fault, a read fault, a discovery-marker fault that is not an abort)internal/server/tool_baseline_durable_dispatch_test.go(T104s). On origin/main this test fails: refreshes 2 and 3 approvewipe_all_notesbyauto-baseline, the call returns "ok" and the stub upstream records the dispatch.internal/security/scanner/service_baseline_commit_test.go(T106)frontend/tests/unit/review-screen-blind-copy.spec.tsand the CLI prompt test (T108)go test -race -tags server -timeout 30m -skip "E2E|Binary|MCPProtocol|TestInfoEndpoint|TestGracefulShutdownNoPanic|TestSocketInfoEndpoint" ./internal/...passes in all 80 packages. The new tests pass with-race -count=5.go test ./cmd/mcpproxy/passes, and the vitest review specs pass 32/32.go test ./tests/tpa-attack-lab/passes../scripts/test-api-e2e.shon scratch ports 19008/19009: 70 passed, 0 failed.--build-tags server, reports nothing in the changed files.make buildsucceeds.wipe_all_noteswas approved byauto-baseline, indexed and dispatched (1 upstream call, 2 after a restart). The authoritative-empty variant and the empty-review variant were also auto-approved.wipe_all_notesstayed pending after 3 refreshes and after a restart. It was absent from the index search, andcall_tool_destructivewas refused with 0 upstream calls. The authoritative-empty and empty-review variants also stayed pending and were refused.large-reviewkeeps 180 pending tools and none are indexed, and the trusted 320-tool server stays approved.inferred.Fixes #1567
Supersedes #1580 (spec)
🤖 Generated with Claude Code