fleet/fleetd
3
0
Fork 0
Code Issues 73 Pull Requests Actions Packages Projects Releases 1 Wiki Activity

fleet_poll{target} drains another session's inbox behind a READ gate — authorization fails open #272

New Issue
Closed
opened 2026-09-04 04:52:41 +02:00 by ltms · 2 comments
ltms commented 2026-09-04 04:52:41 +02:00
Owner
Copy Link
Copy Source

What is wrong

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(target), which removes the replies. A second call returns nothing.

The MCP handler gated both branches with a single constant Authz.Action.READ, and did not even pass the target:

// FleetMcp.java, before this fix
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"));
};

Authz.permits opens READ to every authenticated role:

case SPAWN, STOP, DRAIN -> caller.isPrimary();
case READ, METRICS      -> caller.isPrimary() || caller.isWorker() || caller.isArchitect();

So any worker could read a peer's sessionId out of fleet_list and call fleet_poll{target: <peer>}. That destroys the replies the peer had queued for the primary. The gate failed open, and the loss is not recoverable — a drained reply is gone.

Why this is a real defect and not a paper one

Three independent sources say the tight gate is the intended one:

  1. The sibling tool, four lines below, already gets it right — with a comment stating the exact reasoning that was missed here: 
    // Acking removes a reply from the inbox, so it is a drain, not a read.
deny(exchange, Authz.Action.DRAIN, str(a, "target"));
  2. The REST equivalent is correct. FleetApp.drainReplies checks Authz.Action.DRAIN. The two entry paths disagreed, and CB-505 claims authorization is enforced on both.
  3. The docs already describe it as lead-only. wiki/2-Message-Server.md:101 lists fleet_poll under role lead, and the tool's own schema says "drain that worker's inbox". The code was wrong, not the documentation.

Nothing that works today breaks: the documented workflow is fleet_poll{target} then fleet_ack{target, msgId}, and fleet_ack is already DRAIN (primary-only). A worker could never finish that workflow anyway — it could only destroy the first half of it.

Origin

9daf1ec (Stage 5 hardening) added the gate. Authz.Action.READ's own javadoc reads "Read-only observation: status, roster, profiles, task polling" — it describes the ticket branch only. The drain branch was never considered.

The fix

The required action is a function of the arguments, but the handler chose it before looking at them. Make that choice explicit and testable:

static Authz.Action pollAction(String target) {
    return isBlank(target) ? Authz.Action.READ : Authz.Action.DRAIN;
}

The ticket branch stays READ on purpose: an architect may fleet_send, so it owns tickets and must be able to poll them. DRAIN excludes architects — matching fleet_ack.

Why the test suite did not catch it

This is the sharpest part. FleetMcpAuthzTest checks every Authz.Action against every Role, including "a worker may not DRAIN". It passed every day the defect was live. The policy table was correct; the defect was in which action the caller handed it, and no test looked at that.

That file was itself written to close a coverage hole, and its own doc comment says it "drives the policy half of the gate directly". The other half — the per-tool mapping — stayed untested.

The new tests therefore assert against FleetMcp.pollAction, the same method the handler calls, 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 stays open" test green.

Follow-up worth doing separately

Audit every other MCP handler for the same shape — an action picked from the tool name when the arguments decide what the call actually does. fleet_send is the obvious next candidate: sessionId, turnId and coordId are three different operations under one gate.

## What is wrong `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(target)`, which **removes** the replies. A second call returns nothing. The MCP handler gated both branches with a single constant `Authz.Action.READ`, and did not even pass the target: ```java // FleetMcp.java, before this fix 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")); }; ``` `Authz.permits` opens `READ` to every authenticated role: ```java case SPAWN, STOP, DRAIN -> caller.isPrimary(); case READ, METRICS -> caller.isPrimary() || caller.isWorker() || caller.isArchitect(); ``` So any worker could read a peer's `sessionId` out of `fleet_list` and call `fleet_poll{target: <peer>}`. That destroys the replies the peer had queued for the primary. The gate failed **open**, and the loss is not recoverable — a drained reply is gone. ## Why this is a real defect and not a paper one Three independent sources say the tight gate is the intended one: 1. **The sibling tool, four lines below, already gets it right** — with a comment stating the exact reasoning that was missed here: ```java // Acking removes a reply from the inbox, so it is a drain, not a read. deny(exchange, Authz.Action.DRAIN, str(a, "target")); ``` 2. **The REST equivalent is correct.** `FleetApp.drainReplies` checks `Authz.Action.DRAIN`. The two entry paths disagreed, and CB-505 claims authorization is enforced on both. 3. **The docs already describe it as lead-only.** `wiki/2-Message-Server.md:101` lists `fleet_poll` under role `lead`, and the tool's own schema says "drain that worker's inbox". The code was wrong, not the documentation. Nothing that works today breaks: the documented workflow is `fleet_poll{target}` then `fleet_ack{target, msgId}`, and `fleet_ack` is already `DRAIN` (primary-only). A worker could never finish that workflow anyway — it could only destroy the first half of it. ## Origin `9daf1ec` (Stage 5 hardening) added the gate. `Authz.Action.READ`'s own javadoc reads *"Read-only observation: status, roster, profiles, task polling"* — it describes the **ticket** branch only. The drain branch was never considered. ## The fix The required action is a function of the arguments, but the handler chose it before looking at them. Make that choice explicit and testable: ```java static Authz.Action pollAction(String target) { return isBlank(target) ? Authz.Action.READ : Authz.Action.DRAIN; } ``` The ticket branch stays `READ` on purpose: an architect may `fleet_send`, so it owns tickets and must be able to poll them. `DRAIN` excludes architects — matching `fleet_ack`. ## Why the test suite did not catch it This is the sharpest part. `FleetMcpAuthzTest` checks **every** `Authz.Action` against **every** `Role`, including "a worker may not DRAIN". It passed every day the defect was live. The policy table was correct; the defect was in *which action the caller handed it*, and no test looked at that. That file was itself written to close a coverage hole, and its own doc comment says it "drives the policy half of the gate directly". The other half — the per-tool mapping — stayed untested. The new tests therefore assert against `FleetMcp.pollAction`, the same method the handler calls, 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 stays open" test green. ## Follow-up worth doing separately Audit every other MCP handler for the same shape — an action picked from the tool name when the arguments decide what the call actually does. `fleet_send` is the obvious next candidate: `sessionId`, `turnId` and `coordId` are three different operations under one gate.
ltms commented 2026-09-04 05:01:45 +02:00
Author
Owner
Copy Link
Copy Source

Fixed in 18aecbf on main. Full suite green (1274 tests).

The follow-up audit, done

I said the other handlers should be checked for the same shape — an action chosen from the tool name when the arguments decide what the call does. I read all eleven. fleet_poll was the only one. No new findings, and I am not going to manufacture any:

handler action verdict
send SEND + sessionId three modes (sessionId/turnId/coordId), but all three are the same operation, and Authz ignores the target for SEND. Not a privilege gap.
reply, ask REPLY/ASK + self correct — only-as-itself, target is the caller's own pane
ack DRAIN + target correct, and the model the poll fix copied
spawn, stop SPAWN/STOP correct, primary-only
status, list, profiles, whoami READ genuinely read-only, single-branch bodies

fleet_poll was structurally different from all of them: its body is the only one that branches on an argument into a different kind of operation. That is the tell to look for, and it is worth stating as the general rule — a handler whose body has an if on an argument that selects between operations needs its gate chosen inside that if, not above it.

One thing I noticed but did not change, because it is a logging nit and not a defect: sendHandler always passes sessionId as the audit target, so a send made with turnId or coordId is audited with target=null. The authorization decision is unaffected (SEND ignores the target). Recording it here rather than filing it.

Fixed in `18aecbf` on `main`. Full suite green (1274 tests). ## The follow-up audit, done I said the other handlers should be checked for the same shape — an action chosen from the tool name when the arguments decide what the call does. I read all eleven. **`fleet_poll` was the only one.** No new findings, and I am not going to manufacture any: | handler | action | verdict | |---|---|---| | `send` | `SEND` + `sessionId` | three modes (`sessionId`/`turnId`/`coordId`), but all three are the same operation, and `Authz` ignores the target for `SEND`. Not a privilege gap. | | `reply`, `ask` | `REPLY`/`ASK` + `self` | correct — only-as-itself, target is the caller's own pane | | `ack` | `DRAIN` + `target` | correct, and the model the poll fix copied | | `spawn`, `stop` | `SPAWN`/`STOP` | correct, primary-only | | `status`, `list`, `profiles`, `whoami` | `READ` | genuinely read-only, single-branch bodies | `fleet_poll` was structurally different from all of them: its body is the only one that branches on an argument into a *different kind of operation*. That is the tell to look for, and it is worth stating as the general rule — **a handler whose body has an `if` on an argument that selects between operations needs its gate chosen inside that `if`, not above it.** One thing I noticed but did not change, because it is a logging nit and not a defect: `sendHandler` always passes `sessionId` as the audit target, so a send made with `turnId` or `coordId` is audited with `target=null`. The authorization decision is unaffected (`SEND` ignores the target). Recording it here rather than filing it.
ltms closed this issue 2026-09-04 05:01:53 +02:00
ltms commented 2026-09-04 05:27:35 +02:00
Author
Owner
Copy Link
Copy Source

Proven on the live daemon, not just merged. fleetd was redeployed at 10:25 (pid 53061, jar e53017e0d7d7, build green at 1283 tests). I then spawned a real worker against the running daemon and had it make both calls itself. Its raw results:

fleet_poll{target: "<the lead's own sessionId>"} →

forbidden: worker:term_65a9fd3dd938287 may not DRAIN

fleet_poll{ticket: "does-not-exist"} →

unknown ticket: does-not-exist (never issued, or expired)

Both halves hold. The by-target form is now gated as DRAIN and refused for a worker; the by-ticket form still passes the gate as READ and reaches the ticket lookup. Before this fix the first call handed back another session's inbox.

The worker also noted, correctly, that both come back as tool-call errors — the difference is the wording, not the error/non-error shape. Worth writing down: do not tell these two apart by whether the call errored.

**Proven on the live daemon, not just merged.** fleetd was redeployed at 10:25 (pid 53061, jar `e53017e0d7d7`, build green at 1283 tests). I then spawned a real worker against the running daemon and had it make both calls itself. Its raw results: `fleet_poll{target: "<the lead's own sessionId>"}` → ``` forbidden: worker:term_65a9fd3dd938287 may not DRAIN ``` `fleet_poll{ticket: "does-not-exist"}` → ``` unknown ticket: does-not-exist (never issued, or expired) ``` Both halves hold. The by-target form is now gated as `DRAIN` and refused for a worker; the by-ticket form still passes the gate as `READ` and reaches the ticket lookup. Before this fix the first call handed back another session's inbox. The worker also noted, correctly, that both come back as tool-call errors — the difference is the wording, not the error/non-error shape. Worth writing down: do not tell these two apart by whether the call errored.
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
blocked
Cannot start until something else lands. The body says what.
 needs-live-proof
Merged and green, but never shown working on the running daemon. Not the same as done.
 ready-to-delegate
Scope, files and acceptance criteria are written. A worker can be briefed from the body alone.
 silent-default
A feature that compiles, passes tests, and ships turned off. Nine recurrences and counting.
No Label
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
agent kevin ltms
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: fleet/fleetd#272
Delete Branch "%!s()"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?