#272: fleet_poll{target} is a drain, so gate it as one
fleet_poll is two operations behind one tool name. With `ticket` it observes
an async delegation and changes nothing. With `target` it calls
MessageService.drainReplies, which REMOVES the replies — a second call
returns nothing.
The handler gated both branches with a constant Authz.Action.READ, and did
not pass the target at all. READ is open to every authenticated role, so any
worker could read a peer's sessionId out of fleet_list and destroy the
replies that peer had queued for the primary. The gate failed open, and a
drained reply is not recoverable.
Three things already said the tight gate was intended:
- fleet_ack, four lines below, gates the same drain as DRAIN, with a
comment giving the exact reasoning missed here ("Acking removes a reply
from the inbox, so it is a drain, not a read").
- the REST path checks DRAIN in FleetApp.drainReplies.
- wiki/2-Message-Server.md lists fleet_poll as lead-only, and the tool
schema says "drain that worker's inbox".
Nothing that works today breaks: the documented flow is fleet_poll{target}
then fleet_ack{target,msgId}, and fleet_ack is already primary-only. A
worker could never complete that flow — only destroy its first half.
The required action is a function of the arguments, but the handler chose it
before looking at them. pollAction(target) makes that choice explicit. The
ticket branch stays READ on purpose: an architect may fleet_send, so it owns
tickets and must be able to poll them.
Why the suite missed it: FleetMcpAuthzTest checks every Action against every
Role, including "a worker may not DRAIN", and passed the whole time. The
policy table was right; the action fed to it was wrong, and nothing tested
that mapping. The new tests assert against pollAction itself, so the handler
keeps no private copy of the rule.
Mutation-proved: reverting pollAction to a constant READ turns exactly the
two new defect tests red and leaves the ticket-branch test green.
Introduced in 9daf1ec, where Authz.READ's own javadoc ("...task polling")
describes only the ticket half.
This commit is contained in:
@@ -319,10 +319,12 @@ public final class FleetMcp {
|
||||
};
|
||||
BiFunction<McpSyncServerExchange, McpSchema.CallToolRequest, McpSchema.CallToolResult> pollHandler =
|
||||
(exchange, req) -> {
|
||||
McpSchema.CallToolResult denied = deny(exchange, Authz.Action.READ, null);
|
||||
if (denied != null) return denied;
|
||||
Map<String, Object> a = req.arguments();
|
||||
return poll(messages, str(a, "ticket"), str(a, "target"));
|
||||
String target = str(a, "target");
|
||||
// The action depends on the ARGUMENTS, not on the tool name -- see pollAction.
|
||||
McpSchema.CallToolResult denied = deny(exchange, pollAction(target), target);
|
||||
if (denied != null) return denied;
|
||||
return poll(messages, str(a, "ticket"), target);
|
||||
};
|
||||
// CB-307 Increment 3: per-msgId ack (not needed in v1 but supported by the inbox).
|
||||
// Acking removes a reply from the inbox, so it is a drain, not a read.
|
||||
@@ -724,6 +726,34 @@ public final class FleetMcp {
|
||||
return text("delivered to peer lead " + coordId + " (msgId " + msg.msgId() + ")");
|
||||
}
|
||||
|
||||
/**
|
||||
* Which authorization action a {@code fleet_poll} call needs, decided by its arguments
|
||||
* (fleetd #272).
|
||||
*
|
||||
* <p>{@code fleet_poll} is <strong>two operations behind one tool name</strong>. With {@code
|
||||
* ticket} it observes an async delegation and changes nothing, which is a {@link
|
||||
* Authz.Action#READ}. With {@code target} it calls {@link MessageService#drainReplies} on that
|
||||
* session -- the replies are removed from the inbox and a second call returns nothing -- so it
|
||||
* is a {@link Authz.Action#DRAIN}, the same gate {@code fleet_ack} already uses for removing a
|
||||
* single message, and the same one the REST path uses at {@code FleetApp.drainReplies}.
|
||||
*
|
||||
* <p>Until this method existed the handler passed a constant {@code READ} for both branches.
|
||||
* {@code READ} is open to every authenticated role, so any worker could read a peer's id out of
|
||||
* {@code fleet_list} and destroy the replies that peer had queued for the primary. The gate
|
||||
* failed open, and it did so because the required action is a function of the arguments while
|
||||
* the handler chose it before looking at them.
|
||||
*
|
||||
* <p>The choice lives in this method, and not inline in the handler, so that a test can assert
|
||||
* the mapping the handler actually uses. {@code FleetMcpAuthzTest} already checked every
|
||||
* {@link Authz.Action} against every {@link Role} and passed throughout -- it tested the policy
|
||||
* table, which was correct, while the defect was in which action the caller handed it.
|
||||
*
|
||||
* @param target the {@code target} argument of the call, or {@code null}/blank when absent
|
||||
*/
|
||||
static Authz.Action pollAction(String target) {
|
||||
return isBlank(target) ? Authz.Action.READ : Authz.Action.DRAIN;
|
||||
}
|
||||
|
||||
/** {@code fleet_poll}: check an async delegation by ticket, or drain a worker's inbox by target. */
|
||||
static McpSchema.CallToolResult poll(MessageService messages, String ticket, String target) {
|
||||
if (!isBlank(target)) {
|
||||
|
||||
@@ -179,6 +179,53 @@ class FleetMcpAuthzTest {
|
||||
"no CallerResolver supplied ⇒ authorization not enforced (legacy behaviour)");
|
||||
}
|
||||
|
||||
// --- which action each tool hands the gate (fleetd #272) ------------------------------------
|
||||
|
||||
/**
|
||||
* fleetd #272: {@code fleet_poll{target}} drains a session's reply inbox, so it needs
|
||||
* {@link Authz.Action#DRAIN} -- not the {@link Authz.Action#READ} the handler passed for both
|
||||
* of its branches until this ticket.
|
||||
*
|
||||
* <p>This asserts against {@link FleetMcp#pollAction}, the method the handler itself calls, so
|
||||
* the handler holds no separate copy of the rule that this test could miss. Every other test in
|
||||
* this class checks the policy table (is a worker allowed to DRAIN?) and all of them passed for
|
||||
* the whole time the defect was live -- the table was right, the action fed to it was wrong.
|
||||
*/
|
||||
@Test
|
||||
void pollingByTargetIsADrainAndPollingByTicketIsARead() {
|
||||
assertEquals(Authz.Action.DRAIN, FleetMcp.pollAction("term_b"),
|
||||
"poll by target removes the replies — that is a drain, not an observation");
|
||||
assertEquals(Authz.Action.READ, FleetMcp.pollAction(null),
|
||||
"poll by ticket changes nothing");
|
||||
assertEquals(Authz.Action.READ, FleetMcp.pollAction(" "),
|
||||
"a blank target is an absent target");
|
||||
}
|
||||
|
||||
@Test
|
||||
void aWorkerMayNotDrainAnotherSessionsInboxByPolling() {
|
||||
FleetMcp m = mcp(true);
|
||||
|
||||
assertNotNull(m.denyFor(WORKER_A, FleetMcp.pollAction("term_b"), "term_b"),
|
||||
"a worker draining a peer's inbox would destroy replies queued for the primary");
|
||||
assertNotNull(m.denyFor(ARCH_DESIGN, FleetMcp.pollAction("term_b"), "term_b"),
|
||||
"an architect has no lifecycle rights either — same gate as fleet_ack");
|
||||
assertNull(m.denyFor(PRIMARY, FleetMcp.pollAction("term_b"), "term_b"),
|
||||
"collecting a held reply is the primary's job");
|
||||
}
|
||||
|
||||
/**
|
||||
* The tightening must not close the branch that legitimately serves non-primary callers: an
|
||||
* architect may {@code fleet_send}, so it owns tickets and must be able to poll them.
|
||||
*/
|
||||
@Test
|
||||
void pollingAnOwnTicketStaysOpenToWorkersAndArchitects() {
|
||||
FleetMcp m = mcp(true);
|
||||
|
||||
assertNull(m.denyFor(WORKER_A, FleetMcp.pollAction(null), null));
|
||||
assertNull(m.denyFor(ARCH_DESIGN, FleetMcp.pollAction(null), null),
|
||||
"an architect delegates with wait:false, so it must be able to poll its ticket");
|
||||
}
|
||||
|
||||
// --- identity reconstruction from the transport context ------------------------------------
|
||||
|
||||
@Test
|
||||
|
||||
Reference in New Issue
Block a user