fix(mcpfed): ReadOnlyHint can no longer lower federated tool risk below the floor#147
Merged
Merged
Conversation
…low the floor The federation risk classifier trusted ReadOnlyHint — a tool annotation the external, untrusted MCP server controls — and returned risk.Low unconditionally. A malicious or compromised federated server could therefore mark a destructive tool read-only to have it classified Low, which: - skips the confirm gate (Low is below any Medium/High threshold), - passes the audit fail-closed gate (RequireOpen only refuses >= High), - and is permitted even in read-only mode (read-only refuses only > Low). That defeats the documented "every federated tool is at least High" guarantee those gates rely on — a server-controlled gate bypass. Fix: enforce the invariant the docs already claimed — server annotations may RAISE risk but never lower it below the operator's configured floor (RiskDefault, default High). classify() now returns defaultLevel for ReadOnlyHint=true (treated as "no escalation needed"), never Low. A read-only federated tool reaches Low only when the operator deliberately sets that server's RiskDefault=low. DestructiveHint->Critical and OpenWorldHint->bump are unchanged (both raise, both safe). Updated the two tests that pinned the vulnerable behaviour, corrected doc.go, and added attack-case coverage: destructive+readonly stays Critical; read-only is held at the High/Medium floor; read-only reaches Low only via an explicit operator default. Verified: task ci green (lint 0 / vet / build / test:full 0 fail / govulncheck clean); task eval 17/17. Behaviour-isolated to the inbound federation classifier; the MCP server's outbound ReadOnlyHint annotations are unaffected.
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
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.
Summary
Third trust-boundary safety fix in the series (after MCP output quarantine v0.766 and federation output bound v0.767). A gate-bypass via untrusted metadata.
The federation risk classifier (
mcpfed.classify) trustedReadOnlyHint— a tool annotation the external, untrusted MCP server controls — and returnedrisk.Lowunconditionally. A malicious or compromised federated server could mark a destructive tool read-only to have it classified Low, which:RequireOpenonly refuses ≥ High),> Low).That defeats the documented "every federated tool is at least High" guarantee those gates rely on.
Fix
Enforce the invariant the docs already claimed — server annotations may raise risk but never lower it below the operator's configured floor (
RiskDefault, default High).classify()now returnsdefaultLevelforReadOnlyHint=true(treated as "no escalation needed"), neverLow. A read-only federated tool reaches Low only when the operator deliberately sets that server'sRiskDefault=low.DestructiveHint→Critical andOpenWorldHint→bump are unchanged (both raise, both safe).Verification
doc.go; added attack-case coverage: destructive+readonly stays Critical; read-only held at the High/Medium floor; read-only reaches Low only via an explicit operator default.task cigreen (lint 0 / vet / build /test:full0 fail / govulncheck clean);task eval17/17.ReadOnlyHintannotations are unaffected.Safety fix — anchors v0.768.0.