Skip to content

Commit b9de33e

Browse files
author
matthew
committed
wip(fix/1.6.0-mcp-security-findings): in-flight agent work, committed for safety
Battery-forced pause. This is an agent mid-task, committed so nothing is lost rather than because the unit is finished. It has NOT been verified, NOT been red-proven, and must not be merged as-is. Resume or discard.
1 parent 313c60e commit b9de33e

9 files changed

Lines changed: 203 additions & 5 deletions

File tree

crates/busbar/src/ingress/tests/tests.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@ fn minimal_app() -> Arc<App> {
2323
Arc::new(App {
2424
mcp_catalogue: Arc::new(crate::mcp::catalogue::Catalogue::default()),
2525
mcp_sightings: Default::default(),
26+
mcp_spent_approvals: Default::default(),
2627
mcp_pool: Default::default(),
2728
mcp_servers: Arc::new(Default::default()),
2829
// Not an MCP server: the plane is absent and the dispatch table empty, which is what every

crates/busbar/src/main.rs

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3987,6 +3987,13 @@ pub(crate) fn build_app_from_config(
39873987
|| Arc::new(crate::mcp::client::catalogue::CatalogueCache::new()),
39883988
|p| p.mcp_sightings.clone(),
39893989
),
3990+
// CARRIED ACROSS THE APPLY for the same reason, and it is the same class of mistake: an
3991+
// approval already spent is evidence, not intent, and a config apply that forgot it would
3992+
// hand every outstanding confirmation back to whoever still holds it.
3993+
mcp_spent_approvals: prior.map_or_else(
3994+
|| Arc::new(crate::mcp::askstate::SpentAskStates::new()),
3995+
|p| p.mcp_spent_approvals.clone(),
3996+
),
39903997
credential_cache: prior.map_or_else(
39913998
|| Arc::new(auth_cache::CredentialCache::new()),
39923999
|p| p.credential_cache.clone(),

crates/busbar/src/mcp/askstate.rs

Lines changed: 75 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -134,6 +134,9 @@ pub(crate) enum Rejected {
134134
WrongRequest,
135135
/// Sealed under a catalogue generation that is no longer live — an approval moved underneath it.
136136
WrongGeneration,
137+
/// ALREADY REDEEMED. Perfectly valid, for this caller, for this request, inside its window — and
138+
/// already spent on the call it was minted to approve. See [`SpentAskStates`].
139+
AlreadySpent,
137140
}
138141

139142
impl Rejected {
@@ -146,6 +149,7 @@ impl Rejected {
146149
Rejected::WrongPrincipal => "state_wrong_principal",
147150
Rejected::WrongRequest => "state_wrong_request",
148151
Rejected::WrongGeneration => "state_wrong_generation",
152+
Rejected::AlreadySpent => "state_already_spent",
149153
}
150154
}
151155
}
@@ -291,6 +295,77 @@ pub(crate) fn digest_arguments(arguments: &serde_json::Value) -> String {
291295
hex::encode(h.finalize())
292296
}
293297

298+
/// THE SPENT-APPROVAL LEDGER — what makes an approval SINGLE-USE.
299+
///
300+
/// ## What the seal could not do on its own
301+
///
302+
/// Everything else about this module is a statement the seal itself can carry: who it was minted
303+
/// for, what request, which round, until when. Single use is the one property that cannot ride
304+
/// inside the blob, because a caller presenting the identical blob a second time presents an
305+
/// identical, perfectly valid blob. The only thing that can tell the second presentation from the
306+
/// first is a RECORD THAT THE FIRST HAPPENED — and until this existed there was none, so an operator
307+
/// who gated a money-moving tool behind a confirmation got confirm-once-execute-many.
308+
///
309+
/// ## Keyed on the nonce, and only the terminal redemption is recorded
310+
///
311+
/// The nonce already exists and is already unique per mint (`mrtr`'s multi-round scenario requires
312+
/// it), so it is the natural handle and nothing new has to be sealed. What is recorded is the ONE
313+
/// redemption that dispatches: an intermediate round's state is answered with a fresh ask and a
314+
/// fresh state, so burning it would refuse the ordinary case of a client retrying a request whose
315+
/// answer it never saw. The spend therefore happens exactly where the exchange COMPLETES.
316+
///
317+
/// ## What a restart does to it, and why that is the right trade
318+
///
319+
/// This is PROCESS-LOCAL, and deliberately so rather than for want of a durable store.
320+
///
321+
/// - The window a restart reopens is bounded by the state's own life: a state that has lapsed is
322+
/// already refused by [`Sealer::open`], so the most a restart can restore is the unredeemed
323+
/// remainder of one [`DEFAULT_TTL_SECS`] window. It is not a standing hole; it closes by itself.
324+
/// - It is not attacker-triggerable. A caller cannot restart the process, and a caller who could
325+
/// has a larger primitive than double-spending one confirmation.
326+
/// - The alternative is a durable spent-nonce table, which means a new `busbar_api::Store` method,
327+
/// which means the plugin ABI — a substantial change to buy the residual, and one this tree is
328+
/// the wrong place to spend: it is scheduled for deletion and rebuild on the `rmcp` SDK, and the
329+
/// rebuilt tree gets to decide where its state lives. The BEHAVIOUR is pinned by test either way,
330+
/// so the decision can be revisited without the property being lost.
331+
///
332+
/// A fleet is the same trade one hop out: two nodes sharing a signing key share the seal but not
333+
/// this ledger, so a redemption on node A does not stop one on node B. That is a real limit and it
334+
/// is written down here rather than discovered; closing it needs shared state, which is the same
335+
/// durable-store decision.
336+
///
337+
/// ## The size of it
338+
///
339+
/// An entry lives at most as long as the state it records, and every call evicts what has lapsed,
340+
/// so the table holds at most the approvals minted in one TTL window. Minting one costs the caller
341+
/// a metered, budget-charged round, so the rate is bounded by governance rather than by this map.
342+
#[derive(Debug, Default)]
343+
pub(crate) struct SpentAskStates {
344+
/// nonce ⇒ the instant after which the entry is meaningless, because the state it records can
345+
/// no longer be opened anyway.
346+
seen: std::sync::Mutex<std::collections::HashMap<String, u64>>,
347+
}
348+
349+
impl SpentAskStates {
350+
pub(crate) fn new() -> Self {
351+
Self::default()
352+
}
353+
354+
/// SPEND this approval. `true` if it had not been spent before; `false` if it had.
355+
///
356+
/// Test-and-set under one lock, and that is not an optimisation: a caller that fires two
357+
/// redemptions of one approval concurrently is the obvious way to attack a check that reads and
358+
/// then writes, and it is the shape the whole gate exists to refuse.
359+
pub(crate) fn spend(&self, nonce: &str, expires_at: u64, now: u64) -> bool {
360+
// Poison-recovering, like every other request-path lock in this process: the data behind it
361+
// is still valid after a panic, and cascading the poison would turn one stray panic into a
362+
// gate that refuses every confirmation for the life of the process.
363+
let mut seen = self.seen.lock().unwrap_or_else(|e| e.into_inner());
364+
seen.retain(|_, expiry| *expiry >= now);
365+
seen.insert(nonce.to_string(), expires_at).is_none()
366+
}
367+
}
368+
294369
/// A fresh nonce. `getrandom` is the same fail-closed entropy source key secrets use; a failure is
295370
/// not survivable here, because a predictable nonce is a `multi-round` scenario that passes by
296371
/// accident and a replay window that is wider than it looks.

crates/busbar/src/mcp/callerask.rs

Lines changed: 37 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -238,11 +238,16 @@ pub(crate) struct Bind<'a> {
238238
pub(crate) now: u64,
239239
}
240240

241-
/// THE DECISION. Pure: config, caller input, and a clock. It is not in scope of any upstream
242-
/// response and cannot be — see the module header.
241+
/// THE DECISION. Config, caller input, a clock — and the ledger of approvals already spent. It is
242+
/// not in scope of any upstream response and cannot be — see the module header.
243243
///
244244
/// `rounds` is the operator's ordered list of rounds for THIS capability; an empty list means the
245245
/// capability declares no ask, which is the default and is today's behaviour.
246+
///
247+
/// NOT PURE, and only in one place: the arm that COMPLETES an exchange marks the approval it
248+
/// consumed as spent. That effect lives here rather than at the call site on purpose — a caller that
249+
/// had to remember to record the spend is a caller that can forget, and the check and the record
250+
/// have to be one atomic act or two concurrent redemptions both pass it.
246251
pub(crate) fn decide(
247252
rounds: &[AskRoundCfg],
248253
cap: u32,
@@ -251,6 +256,7 @@ pub(crate) fn decide(
251256
bind: Bind<'_>,
252257
args_digest: &str,
253258
sealer: Option<&Sealer>,
259+
spent: &askstate::SpentAskStates,
254260
) -> AskDecision {
255261
// (1) A capability that declares NO ask never asks, and never accepts state either. Accepting
256262
// state here would mean busbar verifying a blob it had no reason to have minted.
@@ -265,6 +271,10 @@ pub(crate) fn decide(
265271

266272
// (2) WHICH ROUND IS THIS? Read from the sealed state, never from a counter busbar holds between
267273
// requests and never from anything the caller can write. A caller with no state has not started one.
274+
// The approval this request is presenting, if it is presenting one: the nonce that identifies it
275+
// and the instant past which it could not be opened anyway. Carried down to the completion arm,
276+
// which is the only place it is spent.
277+
let mut presented: Option<(String, u64)> = None;
268278
let next_round = match retry.state {
269279
None => {
270280
// `mrtr.mdx` client requirements: a client MUST NOT invent state. Responses without one
@@ -290,6 +300,10 @@ pub(crate) fn decide(
290300
) {
291301
return AskDecision::Refuse(Refusal::StateRejected(e));
292302
}
303+
presented = Some((
304+
opened.nonce.clone(),
305+
opened.issued_at.saturating_add(opened.ttl_secs),
306+
));
293307
opened.round.saturating_add(1)
294308
}
295309
};
@@ -307,6 +321,20 @@ pub(crate) fn decide(
307321
// (4) EVERY ROUND ANSWERED ⇒ the exchange is complete and the call proceeds. This is the ONLY
308322
// arm that dispatches, and reaching it requires a verified state for the last configured round.
309323
let Some(this_round) = rounds.get(next_round as usize) else {
324+
// AND THE APPROVAL IS NOW SPENT. Everything above proves the state was busbar's, was this
325+
// caller's, was for this exact request and has not lapsed — all of which is equally true of
326+
// the SECOND presentation of a state already redeemed. Without this, an operator who gated a
327+
// tool because it moves money got confirm-once, execute-many.
328+
//
329+
// `rounds` is non-empty here (the empty case returned at step 1), so an exchange can only
330+
// complete by presenting state, and `presented` is therefore always `Some`. The `else` is
331+
// the fail-closed reading of a shape that cannot occur rather than a case with a meaning.
332+
let Some((nonce, expires_at)) = presented else {
333+
return AskDecision::Refuse(Refusal::StateRejected(askstate::Rejected::AlreadySpent));
334+
};
335+
if !spent.spend(&nonce, expires_at, bind.now) {
336+
return AskDecision::Refuse(Refusal::StateRejected(askstate::Rejected::AlreadySpent));
337+
}
310338
return AskDecision::Proceed;
311339
};
312340

@@ -428,3 +456,10 @@ fn declared(capabilities: &serde_json::Value, key: &str) -> bool {
428456
#[cfg(test)]
429457
#[path = "tests/callerask_tests.rs"]
430458
mod callerask_tests;
459+
460+
// THE GATE JUDGED FROM OUTSIDE IT: one approval, presented twice, against a real upstream that
461+
// records what it was told to do. Separate from the file above because nothing in it may reach into
462+
// the decision — the claim is about what a caller can make happen, not about which arm answered.
463+
#[cfg(test)]
464+
#[path = "tests/confirm_once_tests.rs"]
465+
mod confirm_once_tests;

crates/busbar/src/mcp/method.rs

Lines changed: 37 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1274,12 +1274,46 @@ async fn tools_call(
12741274
// state is sealed over a digest of the arguments AS THE CALLER SENT THEM, so merging first
12751275
// would make a retry's digest disagree with the seal minted on the previous round and every
12761276
// multi-round exchange would fail verification on round two.
1277+
//
1278+
// AND THE MERGE IS BOUNDED BY WHAT THE OPERATOR ASKED FOR. This used to insert every key the
1279+
// caller put in `inputResponses`, overwriting whatever was there — which meant the one thing the
1280+
// seal covers, the arguments the confirmation was DISPLAYED about, could be rewritten on the way
1281+
// past it. A caller shown "approve moving 10 to alice?" answered `{"amount": 1000000}` and the
1282+
// upstream was told to move a million, with the digest check passing the whole way, because the
1283+
// digest is taken over `arguments` and the rewrite arrived in a sibling field. An approval that
1284+
// carries out a different call than the one it described is the same defect as an approval that
1285+
// is not required at all.
1286+
//
1287+
// So: an answer may bind ONLY a key this capability's own `ask_caller:` rounds declared, and may
1288+
// never name an argument the caller already sent. Anything else refuses the call rather than
1289+
// being dropped — a caller whose answer is being ignored has to be told, or the next attacker to
1290+
// try it learns nothing and the next honest client debugs a value that vanished.
12771291
if let Some(responses) = params
12781292
.and_then(|p| p.get("inputResponses"))
12791293
.and_then(|v| v.as_object())
12801294
{
1281-
let merged = arguments.as_object_mut();
1282-
if let Some(merged) = merged {
1295+
let declared: std::collections::BTreeSet<&str> = selected
1296+
.ask_caller
1297+
.iter()
1298+
.flat_map(|round| round.keys().map(String::as_str))
1299+
.collect();
1300+
let sealed: std::collections::BTreeSet<String> = arguments
1301+
.as_object()
1302+
.map(|o| o.keys().cloned().collect())
1303+
.unwrap_or_default();
1304+
if let Some(offending) = responses
1305+
.keys()
1306+
.find(|k| !declared.contains(k.as_str()) || sealed.contains(*k))
1307+
{
1308+
let refusal = DispatchRefusal::NotGranted(format!(
1309+
"the answer named `{offending}`, which is not one of the inputs \
1310+
`{}` requested — an answer may only supply what was asked for, and may never \
1311+
rewrite an argument the confirmation was shown for.",
1312+
selected.namespaced
1313+
));
1314+
return log.refused("caller_ask_answer_undeclared", refuse(ctx, name, &refusal, id));
1315+
}
1316+
if let Some(merged) = arguments.as_object_mut() {
12831317
for (key, value) in responses {
12841318
merged.insert(key.clone(), value.clone());
12851319
}
@@ -2165,6 +2199,7 @@ fn caller_ask_decision(
21652199
},
21662200
&super::askstate::digest_arguments(arguments),
21672201
sealer.as_ref(),
2202+
&ctx.app.mcp_spent_approvals,
21682203
)
21692204
}
21702205

crates/busbar/src/mcp/tests/callerask_tests.rs

Lines changed: 16 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@
1313
//! might be matching nothing at all.
1414
1515
use super::*;
16+
use crate::mcp::askstate::SpentAskStates;
1617
use crate::mcp::config::{AskEntryCfg, AskRoundCfg};
1718

1819
const KEY: [u8; 32] = [3u8; 32];
@@ -69,7 +70,16 @@ fn all_capabilities() -> serde_json::Value {
6970
const DIGEST: &str = "d";
7071

7172
fn decide_with(rounds: &[AskRoundCfg], caps: &serde_json::Value, retry: Retry<'_>) -> AskDecision {
72-
decide(rounds, 3, caps, retry, bind(), DIGEST, Some(&sealer()))
73+
decide(
74+
rounds,
75+
3,
76+
caps,
77+
retry,
78+
bind(),
79+
DIGEST,
80+
Some(&sealer()),
81+
&SpentAskStates::new(),
82+
)
7383
}
7484

7585
/// DENY BY DEFAULT, BY ABSENCE. A capability with no `ask_caller` never asks — which is every
@@ -263,6 +273,7 @@ fn one_callers_state_is_not_redeemable_by_another() {
263273
b,
264274
DIGEST,
265275
Some(&sealer()),
276+
&SpentAskStates::new(),
266277
);
267278
assert!(matches!(
268279
got,
@@ -349,6 +360,7 @@ fn the_round_cap_is_hard_and_cannot_be_reset_by_replaying_an_earlier_state() {
349360
bind(),
350361
DIGEST,
351362
Some(&sealer()),
363+
&SpentAskStates::new(),
352364
)
353365
else {
354366
panic!("round {expected} must be allowed under a cap of 2");
@@ -369,6 +381,7 @@ fn the_round_cap_is_hard_and_cannot_be_reset_by_replaying_an_earlier_state() {
369381
bind(),
370382
DIGEST,
371383
Some(&sealer()),
384+
&SpentAskStates::new(),
372385
);
373386
assert!(
374387
matches!(got, AskDecision::Refuse(Refusal::RoundCapExceeded { .. })),
@@ -389,6 +402,7 @@ fn a_cap_of_zero_never_asks() {
389402
bind(),
390403
DIGEST,
391404
Some(&sealer()),
405+
&SpentAskStates::new(),
392406
);
393407
assert!(matches!(
394408
got,
@@ -453,6 +467,7 @@ fn a_deployment_with_no_signing_key_refuses_rather_than_asking_with_unprotected_
453467
bind(),
454468
DIGEST,
455469
None,
470+
&SpentAskStates::new(),
456471
);
457472
assert!(matches!(got, AskDecision::Refuse(Refusal::NoSealer { .. })));
458473
}

crates/busbar/src/mcp/tests/connect_support.rs

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -43,6 +43,12 @@ pub(crate) struct PeerState {
4343
pub(super) tools: Vec<serde_json::Value>,
4444
/// Methods received, in order. Read to prove a refused dispatch never reached the wire.
4545
pub(super) methods: Vec<String>,
46+
/// The `params.arguments` of every `tools/call` that reached this peer, in order.
47+
///
48+
/// Recorded because "the call carried out after an approval is the call the question was about"
49+
/// is a claim about WHAT THE UPSTREAM WAS TOLD TO DO, and no assertion about busbar's own return
50+
/// value can make it. The peer's own bookkeeping is the only witness that is not the accused.
51+
pub(super) call_arguments: Vec<serde_json::Value>,
4652
/// When set, `tools/list` answers with this JSON-RPC error code instead of a result.
4753
pub(super) list_error: Option<i64>,
4854
}
@@ -112,6 +118,11 @@ impl Peer {
112118
self.methods().iter().filter(|m| *m == "tools/call").count()
113119
}
114120

121+
/// The arguments of every `tools/call` this peer was actually told to run, in order.
122+
pub(crate) fn call_arguments(&self) -> Vec<serde_json::Value> {
123+
self.state.lock().unwrap().call_arguments.clone()
124+
}
125+
115126
/// How many `tools/list` requests reached the wire — i.e. how many times this peer was actually
116127
/// REFRESHED. The load-bearing number for "the refresh timer honours the operator's cadence":
117128
/// a sweep that ignored `refresh_ttl:` would contact every registered upstream on every tick,
@@ -136,6 +147,14 @@ async fn endpoint(
136147
let (tools, list_error) = {
137148
let mut st = shared.0.lock().unwrap();
138149
st.methods.push(method.clone());
150+
if method == "tools/call" {
151+
st.call_arguments.push(
152+
parsed
153+
.pointer("/params/arguments")
154+
.cloned()
155+
.unwrap_or(serde_json::Value::Null),
156+
);
157+
}
139158
(st.tools.clone(), st.list_error)
140159
};
141160
let value = match method.as_str() {

crates/busbar/src/state.rs

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -468,6 +468,16 @@ pub(crate) struct App {
468468
/// runs a refresh has always done; with one, it compares against what the upstream is actually
469469
/// serving, and a schema that moved refuses the call.
470470
pub(crate) mcp_sightings: Arc<crate::mcp::client::catalogue::CatalogueCache>,
471+
/// APPROVALS ALREADY SPENT — the record that makes an operator-configured confirmation
472+
/// single-use.
473+
///
474+
/// Arc-shared ACROSS config applies, for the same correctness reason the sightings cache and the
475+
/// mutation limiter are: this is ACCUMULATED evidence rather than intent, and rebuilding it on
476+
/// every apply would re-open every outstanding approval the instant an operator touched an
477+
/// unrelated section of config — which is the moment a caller holding a spent approval would
478+
/// like it rebuilt. See [`crate::mcp::askstate::SpentAskStates`] for what a RESTART does to it
479+
/// and why that trade was taken.
480+
pub(crate) mcp_spent_approvals: Arc<crate::mcp::askstate::SpentAskStates>,
471481
/// PLANE DISPATCH for this config generation: which plane an inbound path belongs to, and — for
472482
/// an audience-bound plane — what a token presented there must carry and where a refused caller
473483
/// is told to go. Consulted by the auth middleware on every request, which is why it is a

crates/busbar/src/test_support/mod.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1285,6 +1285,7 @@ impl TestApp {
12851285
mcp_servers: std::sync::Arc::new(self.tool_defs.clone()),
12861286
mcp_pool: std::sync::Arc::new(crate::mcp::client::pool::McpConnectionPool::new()),
12871287
mcp_sightings: self.mcp_sightings.clone().unwrap_or_default(),
1288+
mcp_spent_approvals: Default::default(),
12881289
credential_cache: std::sync::Arc::new(crate::auth_cache::CredentialCache::new()),
12891290
auth_scope_caps: std::collections::HashMap::new(),
12901291
role_bindings: self.role_bindings.unwrap_or_default(),

0 commit comments

Comments
 (0)