From 18aecbfe674eff59e9ab2375ee2787734389fb11 Mon Sep 17 00:00:00 2001 From: Dai Ha Date: Fri, 4 Sep 2026 09:54:52 +0700 Subject: [PATCH] #272: fleet_poll{target} is a drain, so gate it as one MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .../java/dev/ltms/fleet/mcp/FleetMcp.java | 36 ++++++++++++-- .../dev/ltms/fleet/mcp/FleetMcpAuthzTest.java | 47 +++++++++++++++++++ 2 files changed, 80 insertions(+), 3 deletions(-) diff --git a/fleetd/src/main/java/dev/ltms/fleet/mcp/FleetMcp.java b/fleetd/src/main/java/dev/ltms/fleet/mcp/FleetMcp.java index 1e1d7a9..fab8fbc 100644 --- a/fleetd/src/main/java/dev/ltms/fleet/mcp/FleetMcp.java +++ b/fleetd/src/main/java/dev/ltms/fleet/mcp/FleetMcp.java @@ -319,10 +319,12 @@ public final class FleetMcp { }; BiFunction pollHandler = (exchange, req) -> { - McpSchema.CallToolResult denied = deny(exchange, Authz.Action.READ, null); - if (denied != null) return denied; Map 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). + * + *

{@code fleet_poll} is two operations behind one tool name. 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}. + * + *

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. + * + *

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)) { diff --git a/fleetd/src/test/java/dev/ltms/fleet/mcp/FleetMcpAuthzTest.java b/fleetd/src/test/java/dev/ltms/fleet/mcp/FleetMcpAuthzTest.java index 61e31a4..03082f0 100644 --- a/fleetd/src/test/java/dev/ltms/fleet/mcp/FleetMcpAuthzTest.java +++ b/fleetd/src/test/java/dev/ltms/fleet/mcp/FleetMcpAuthzTest.java @@ -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. + * + *

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