From 32408d1e642df98b8b0ff1ca2f0e06f81e56cacc Mon Sep 17 00:00:00 2001 From: Dai Ha Date: Sat, 12 Sep 2026 10:58:09 +0700 Subject: [PATCH] fleetd #509: pin the pane-scan completeness fold, and stop legacyPrincipal handing out primary MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Unit 1 — PaneLocator.terminalForPid's completeness fold across herdr clients (PaneLocator.java:117) had no test that varied the number of clients, so a mutation that keeps only the last client's Lookup.complete() instead of ANDing every client's outcome survived: 14 of 15 existing tests agree with the mutant on a single client. Added a two-client test where the lead client errors on the pane that would have owned the pid (an incomplete, negative scan) and the member client cleanly finds no panes (a complete, negative scan) — the real fold ANDs these to false, a last-wins fold reads it as true. Proved against MUTANTC (complete = outcome.complete();): the new test fails with "expected: but was: ", the file was restored byte-identical (sha256 unchanged), and the control run is green. Unit 2 — FleetMcp.legacyPrincipal's else-branch returned Principal.primary for ANY caller the connection did not resolve to a worker pane, with none of CallerResolver.java:254's isLoopback/scanComplete guards. Measured that no production caller passes null callers (Fleetd.java:696 always constructs a real CallerResolver) but FleetMcpAuthzTest.mcp(false) legitimately does, for its "legacy constructor leaves the gate open" test — so the null-callers path is not dead code to delete (option a), it is a documented legacy mode (option b). Changed the else-branch to Principal.anonymous() and widened legacyPrincipal to package-private (like denyFor) so a new test pins the behavior directly, since it only ever ran inside a contextExtractor closure no existing test triggers. --- .../java/dev/ltms/fleet/mcp/FleetMcp.java | 22 +++++++++++++--- .../dev/ltms/fleet/herdr/PaneLocatorTest.java | 26 +++++++++++++++++++ .../dev/ltms/fleet/mcp/FleetMcpAuthzTest.java | 21 +++++++++++++++ 3 files changed, 65 insertions(+), 4 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 6817169..5f0a76b 100644 --- a/fleetd/src/main/java/dev/ltms/fleet/mcp/FleetMcp.java +++ b/fleetd/src/main/java/dev/ltms/fleet/mcp/FleetMcp.java @@ -527,14 +527,28 @@ public final class FleetMcp { } /** - * Pre-CB-501 identity: worker if the connection maps to a pane, otherwise the primary. Used - * only by the legacy constructor, where authorization is not enforced anyway. + * Pre-CB-501 identity: worker if the connection maps to a pane, otherwise anonymous. Used + * only by the legacy constructor ({@code callers == null}), where authorization is not + * enforced anyway — but the resolved {@link Principal} still reaches non-authz logic (e.g. + * {@code markSpawnedMemberPresent}, {@code recordPrimarySingleton}), so it must not be trusted + * with a role it did not earn. + * + *

fleetd #509: this used to fall back to {@link Principal#primary}, unconditionally, for + * every caller the connection did not resolve to a worker pane — with none of + * {@code CallerResolver.java:254}'s two guards ({@code isLoopback}, {@code scanComplete}). + * That is the exact shape #317 and #505 each closed on the enforced path; this branch was the + * same trap, left open on the legacy one. It now returns {@link Principal#anonymous} instead, + * so an unresolved legacy caller earns no authority rather than the primary's. + * + *

Package-private (was {@code private}) so this is unit-testable directly, the same reason + * {@link #denyFor} was split out — it runs inside a contextExtractor closure that only fires on + * a real MCP request, so nothing else could pin this behaviour. */ - private static Principal legacyPrincipal(ConnectionIdentity identity, String addr, int port) { + static Principal legacyPrincipal(ConnectionIdentity identity, String addr, int port) { ConnectionIdentity.Caller c = identity.resolve(addr, port); return c.terminal() != null ? Principal.worker(c.terminal(), c.pid()) - : Principal.primary(c.pid()); + : Principal.anonymous(); } /** The caller reconstructed from the transport context. */ diff --git a/fleetd/src/test/java/dev/ltms/fleet/herdr/PaneLocatorTest.java b/fleetd/src/test/java/dev/ltms/fleet/herdr/PaneLocatorTest.java index 9818fae..e3fce0c 100644 --- a/fleetd/src/test/java/dev/ltms/fleet/herdr/PaneLocatorTest.java +++ b/fleetd/src/test/java/dev/ltms/fleet/herdr/PaneLocatorTest.java @@ -180,6 +180,32 @@ class PaneLocatorTest { assertTrue(outcome.complete(), "a positive match elsewhere in the scan is definitive"); } + // --- fleetd #509: the completeness fold across clients must not collapse to "last wins" ---- + + @Test + void anEarlierClientsErrorSurvivesALaterClientsCleanNegative() { + // terminalForPid folds each client's Lookup.complete() with + // complete = complete && outcome.complete(); + // (PaneLocator.java:117). With a SINGLE client, a fold that keeps only the last outcome + // (dropping the "complete &&" prefix) agrees with the real fold — which is why 14 of the + // 15 pre-existing tests never catch that mutation: none of them vary the number of clients. + // Here the LEAD client errors on exactly the pane that would have owned the pid (so its + // scan is incomplete AND finds no match), and the MEMBER client cleanly reports no panes + // at all (a complete, negative scan). The real fold ANDs the two into false. A fold that + // just keeps the last client's outcome would read this as a clean true — the earlier + // error is erased, and CallerResolver.java:254 would read scanComplete() as true and + // promote an unverified caller to the primary. + HerdrClient lead = new FakeHerdr().processInfoFailsForPane("w2:p7", "transient"); + HerdrClient member = new FakeHerdr().withNoPanes(); + PaneLocator two = new PaneLocator(lead, member); + + PaneLocator.Lookup outcome = two.terminalForPid(FakeHerdr.WORKER_PID); + + assertNull(outcome.terminal(), "the pane that could have owned the pid was never checked"); + assertFalse(outcome.complete(), + "an earlier client's error must survive a later client's clean negative"); + } + /** Minimal single-pane {@link HerdrClient} fake, purpose-built for the ancestry tests above. */ private static final class OnePaneHerdr implements HerdrClient { private final ObjectMapper mapper = new ObjectMapper(); 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 38a26ab..d540948 100644 --- a/fleetd/src/test/java/dev/ltms/fleet/mcp/FleetMcpAuthzTest.java +++ b/fleetd/src/test/java/dev/ltms/fleet/mcp/FleetMcpAuthzTest.java @@ -190,6 +190,27 @@ class FleetMcpAuthzTest { "no CallerResolver supplied ⇒ authorization not enforced (legacy behaviour)"); } + /** + * fleetd #509: {@code legacyPrincipal} (used only when {@code callers == null}, i.e. the + * legacy constructor above) used to fall back to {@link Principal#primary} for ANY caller the + * connection did not resolve to a worker pane — no {@code isLoopback} check, no + * {@code scanComplete} check, unlike the enforced path's {@code CallerResolver.java:254}. A + * non-loopback caller (an off-host client) is exactly the case that must never earn the + * primary's authority, and authorization being disabled in legacy mode does not make that + * safe: the resolved {@link Principal} still reaches non-authz logic such as + * {@code markSpawnedMemberPresent} and {@code recordPrimarySingleton}. + */ + @Test + void legacyPrincipalIsAnonymousNotPrimaryForAnUnresolvedCaller() { + ConnectionIdentity identity = new ConnectionIdentity(new PaneLocator(herdr), _ -> 999_999); + // A non-loopback address never even reaches the pane scan — resolve() short-circuits it + // to Caller(null, -1, true), the same "no terminal" shape a genuine primary's connection + // produces. legacyPrincipal must not conflate the two. + Principal p = FleetMcp.legacyPrincipal(identity, "8.8.8.8", 1234); + assertEquals(Principal.anonymous(), p, + "an unresolved legacy caller must earn no authority, not the primary's"); + } + // --- fleetd #439: who may see fleet_list's coordinator row ---------------------------------- /** -- 2.52.0