fleetd #439: pin the caller at the fleet_list call site, not just the gate
Review of PR #462 found M2: the coordinatorVisibleTo gate (then an inline principal(exchange).isPrimary() check) could survive a mutation that replaced the argument with a literal true at the one production call site, because every existing test drove listFleet directly and supplied the boolean itself -- nothing exercised the handler's own call. - Name the decision: FleetMcp.coordinatorVisibleTo(Principal), a small package-private predicate next to denyFor/recordPrimarySingleton. The fleet_list handler now calls coordinatorVisibleTo(principal(exchange)) instead of inlining .isPrimary(). - Pin the predicate's role table in FleetMcpAuthzTest (onlyThePrimaryMaySeeTheCoordinatorRow), covering primary/worker/ architect and, newly, anonymous. - Add a source-reading detector at the boundary (theFleetListHandlerActuallyConsultsCoordinatorVisibleTo), same idiom as toolsTheServerRegisters/everyRegisteredToolHasItsHandlerActionPinned: it reads FleetMcp.java, isolates the listHandler block, asserts (as a control) that the block actually contains a listFleet( call, then asserts the call's trailing boolean argument is exactly coordinatorVisibleTo(principal(exchange)) -- not a literal true/false. Both mutations from the review were reproduced and killed by these tests, then reverted; see the PR body for the full break-and-restore transcript.
This commit is contained in:
@@ -427,7 +427,7 @@ public final class FleetMcp {
|
|||||||
leadSeats, callers == null ? Map.of() : callers.leads(),
|
leadSeats, callers == null ? Map.of() : callers.leads(),
|
||||||
callerTerminal(exchange),
|
callerTerminal(exchange),
|
||||||
new CoordinationSource(leadChannel, peers),
|
new CoordinationSource(leadChannel, peers),
|
||||||
principal(exchange).isPrimary());
|
coordinatorVisibleTo(principal(exchange)));
|
||||||
};
|
};
|
||||||
BiFunction<McpSyncServerExchange, McpSchema.CallToolRequest, McpSchema.CallToolResult> stopHandler =
|
BiFunction<McpSyncServerExchange, McpSchema.CallToolRequest, McpSchema.CallToolResult> stopHandler =
|
||||||
(exchange, req) -> {
|
(exchange, req) -> {
|
||||||
@@ -580,6 +580,20 @@ public final class FleetMcp {
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/**
|
||||||
|
* fleetd #439: only the primary may read {@code fleet_list}'s {@code coordinator} row —
|
||||||
|
* lead-to-lead coordination state (coord-ids, mailbox facts, held-message previews), never the
|
||||||
|
* roster. Split out of the {@code fleet_list} handler, same reason as {@link #denyFor} and
|
||||||
|
* {@link #recordPrimarySingleton}: the decision must be unit-testable without fabricating an
|
||||||
|
* SDK {@code McpSyncServerExchange}, and the handler must call this named predicate rather than
|
||||||
|
* inlining the check, so a future edit cannot silently pass a literal instead of asking who
|
||||||
|
* called ({@code FleetMcpAuthzTest.theFleetListHandlerActuallyConsultsCoordinatorVisibleTo}
|
||||||
|
* reads the source and asserts the handler calls this method by name, not a literal).
|
||||||
|
*/
|
||||||
|
static boolean coordinatorVisibleTo(Principal caller) {
|
||||||
|
return caller.isPrimary();
|
||||||
|
}
|
||||||
|
|
||||||
/** The worker identity resolved from this call's connection, or {@code null} if the primary. */
|
/** The worker identity resolved from this call's connection, or {@code null} if the primary. */
|
||||||
private static String callerTerminal(McpSyncServerExchange exchange) {
|
private static String callerTerminal(McpSyncServerExchange exchange) {
|
||||||
Object v = exchange.transportContext().get(CALLER_TERMINAL);
|
Object v = exchange.transportContext().get(CALLER_TERMINAL);
|
||||||
|
|||||||
@@ -187,6 +187,71 @@ class FleetMcpAuthzTest {
|
|||||||
"no CallerResolver supplied ⇒ authorization not enforced (legacy behaviour)");
|
"no CallerResolver supplied ⇒ authorization not enforced (legacy behaviour)");
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// --- fleetd #439: who may see fleet_list's coordinator row ----------------------------------
|
||||||
|
|
||||||
|
/**
|
||||||
|
* fleetd #439: {@link FleetMcp#coordinatorVisibleTo} is the whole policy decision for
|
||||||
|
* {@code fleet_list}'s {@code coordinator} row — lead-to-lead coordination state, not roster
|
||||||
|
* observation. Only the primary may see it; a worker, an architect, and (the case the previous
|
||||||
|
* pass of this ticket did not cover) an anonymous caller must all be refused.
|
||||||
|
*/
|
||||||
|
@Test
|
||||||
|
void onlyThePrimaryMaySeeTheCoordinatorRow() {
|
||||||
|
assertTrue(FleetMcp.coordinatorVisibleTo(PRIMARY), "the primary must see its own coordination state");
|
||||||
|
assertFalse(FleetMcp.coordinatorVisibleTo(WORKER_A), "a worker must not see lead-to-lead coordination state");
|
||||||
|
assertFalse(FleetMcp.coordinatorVisibleTo(ARCH_DESIGN),
|
||||||
|
"an architect holds READ today, but that must not extend to coordinator");
|
||||||
|
assertFalse(FleetMcp.coordinatorVisibleTo(ANON), "authenticated as nothing must not see it either");
|
||||||
|
}
|
||||||
|
|
||||||
|
/**
|
||||||
|
* fleetd #439 / PR #462 review finding M2: the predicate above can be perfectly correct while
|
||||||
|
* the one production call site (the {@code fleet_list} MCP handler) never actually asks it —
|
||||||
|
* a literal {@code true} compiles, and the whole suite stayed green under that mutation because
|
||||||
|
* every existing test drives {@link FleetMcp#listFleet} directly and supplies the boolean
|
||||||
|
* itself. This test reads {@code FleetMcp.java}'s own source (same idiom as {@link
|
||||||
|
* #toolsTheServerRegisters()} / {@link #everyRegisteredToolHasItsHandlerActionPinned()}) and
|
||||||
|
* asserts the handler's call passes {@code coordinatorVisibleTo(principal(exchange))} — not a
|
||||||
|
* literal {@code true} or {@code false} — as {@code listFleet}'s trailing argument.
|
||||||
|
*
|
||||||
|
* <p>Anchored on argument position, not a bare substring search: {@code true} appears many
|
||||||
|
* times elsewhere in this file for unrelated reasons, so a plain {@code contains("true")}
|
||||||
|
* check would prove nothing. The pattern requires the literal text immediately before the
|
||||||
|
* closing {@code );} of the {@code listFleet(} call inside the handler block to be exactly
|
||||||
|
* {@code coordinatorVisibleTo(principal(exchange))}.
|
||||||
|
*/
|
||||||
|
@Test
|
||||||
|
void theFleetListHandlerActuallyConsultsCoordinatorVisibleTo() throws Exception {
|
||||||
|
String source = Files.readString(MCP_SOURCE);
|
||||||
|
|
||||||
|
// Isolate the fleet_list handler block: from its declaration up to the next handler's
|
||||||
|
// declaration. A change to variable naming would break this scrape loudly (see the control
|
||||||
|
// assertion just below), rather than silently reporting "no violation found".
|
||||||
|
int start = source.indexOf("listHandler =");
|
||||||
|
assertTrue(start >= 0, "could not find the fleet_list handler (listHandler) in " + MCP_SOURCE
|
||||||
|
+ " -- the scrape has stopped matching, fix the anchor before trusting this test");
|
||||||
|
int end = source.indexOf("stopHandler =", start);
|
||||||
|
assertTrue(end > start, "could not find the handler declared after listHandler to bound the scrape");
|
||||||
|
String handlerBlock = source.substring(start, end);
|
||||||
|
|
||||||
|
// CONTROL: the block we scraped really does contain a call to listFleet(...) -- if this
|
||||||
|
// fails, the anchors above moved and the assertion below would otherwise pass on nothing.
|
||||||
|
assertTrue(handlerBlock.contains("listFleet("),
|
||||||
|
"control failed: the scraped listHandler block contains no listFleet( call at all -- "
|
||||||
|
+ "the anchors have drifted, this test is not testing what it claims to");
|
||||||
|
|
||||||
|
Pattern trailingArg = Pattern.compile(
|
||||||
|
"listFleet\\([^;]*?,\\s*(coordinatorVisibleTo\\(principal\\(exchange\\)\\)|true|false)\\s*\\)\\s*;",
|
||||||
|
Pattern.DOTALL);
|
||||||
|
Matcher m = trailingArg.matcher(handlerBlock);
|
||||||
|
assertTrue(m.find(), "could not locate listFleet(...)'s trailing boolean argument in the "
|
||||||
|
+ "listHandler block -- the call shape changed, update this test's anchor: " + handlerBlock);
|
||||||
|
String trailing = m.group(1);
|
||||||
|
assertEquals("coordinatorVisibleTo(principal(exchange))", trailing,
|
||||||
|
"the fleet_list handler must ask coordinatorVisibleTo(principal(exchange)) who is "
|
||||||
|
+ "calling, not pass a literal boolean -- found: " + trailing);
|
||||||
|
}
|
||||||
|
|
||||||
// --- which action each tool hands the gate (fleetd #272) ------------------------------------
|
// --- which action each tool hands the gate (fleetd #272) ------------------------------------
|
||||||
|
|
||||||
/**
|
/**
|
||||||
|
|||||||
Reference in New Issue
Block a user