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

Pin the Authz.Action every handler chooses, so the next #272 fails a test instead of shipping #281

New Issue
Closed
opened 2026-09-04 05:34:46 +02:00 by ltms · 1 comment
ltms commented 2026-09-04 05:34:46 +02:00
Owner
Copy Link
Copy Source

Found by the authorization audit that followed #272. This is a test-coverage gap, not a live defect — every handler on both surfaces currently chooses the right action. I checked that myself; see the evidence below.

Why this is worth doing anyway

#272 is the reason. fleet_poll was one tool name over two different operations: by ticket it reads your own task, by target it drains someone else's inbox. The handler picked its Authz.Action from the tool name, above the branch, so both forms were gated as the weaker READ and any worker could drain any session's inbox.

The part that matters here: the policy table was correct the whole time, and FleetMcpAuthzTest — which covers every Action against every Role — passed the whole time. The table was right. The caller handed it the wrong question. Exhaustive coverage of a lookup is not coverage of the control that calls it.

Today exactly one handler has a test that pins which action it chooses: pollingByTargetIsADrainAndPollingByTicketIsARead, added by #272. Every other handler's choice is unpinned. Nothing fails if someone adds an argument-dependent branch to fleet_send or POST /sessions/{id}/message and leaves the gate above it.

The ask

Add a test that pins, for every MCP tool handler and every REST route, the Authz.Action it actually selects — including the argument shapes that could select a different one. Model it on pollingByTargetIsADrainAndPollingByTicketIsARead: assert against the extracted choice, not against a private copy of the rule inside the test.

Two properties matter more than the exact shape:

  1. It must fail when a handler's choice changes, not only when the role table changes. If the test still passes after you move a gate above a branch, it is testing the wrong thing.
  2. It must fail when a new handler is added with no pinned action — otherwise the coverage rots the moment someone adds a tool. An enumeration-driven test that walks the registered handlers gives you that for free; a hand-written list does not.

Property 2 is the harder half and the more valuable one. If you cannot get both, say which you got and why.

What I verified myself, and how

Independently of the audit report, on main at 66e5247:

  • rest/FleetApp.java registers 15 routes (lines 163-179). 14 call allow(ctx, Authz.Action.X, target) as the first statement of their handler; GET /healthz is deliberately open as a liveness probe.
  • mcp/FleetMcp.java registers 11 tools. fleet_poll is the only one whose operation changes with its arguments, and since #272 it chooses via pollAction(target) before dispatch.
  • Matching route/tool pairs use the same action on both surfaces, and Fleetd.main passes one CallerResolver to both.

I also chased the one fail-open in the gate: FleetApp.allow returns true when auth == null, and FleetMcp.deny does the same at line 481. Not reachable in production — Fleetd.main constructs a CallerResolver on both branches (lines 622 and 626), and cfg.validateAuthExposure() throws at startup if the bind is wider than the auth mode can defend, so the dangerous configuration cannot be reached by ignoring a log line. The null path is the legacy/test constructor only. Worth knowing, not worth changing.

Scope

Tests only. Do not change any handler or the role table — there is nothing wrong with either right now. If writing the test makes you want to change production code to make a handler's choice observable, that is fine and expected (that is what pollAction did), but keep the behaviour identical and say what you extracted.

Found by the authorization audit that followed #272. **This is a test-coverage gap, not a live defect** — every handler on both surfaces currently chooses the right action. I checked that myself; see the evidence below. ## Why this is worth doing anyway #272 is the reason. `fleet_poll` was one tool name over two different operations: by `ticket` it reads your own task, by `target` it drains someone else's inbox. The handler picked its `Authz.Action` from the tool name, above the branch, so both forms were gated as the weaker `READ` and any worker could drain any session's inbox. The part that matters here: **the policy table was correct the whole time, and `FleetMcpAuthzTest` — which covers every `Action` against every `Role` — passed the whole time.** The table was right. The caller handed it the wrong question. Exhaustive coverage of a lookup is not coverage of the control that calls it. Today exactly one handler has a test that pins which action it chooses: `pollingByTargetIsADrainAndPollingByTicketIsARead`, added by #272. Every other handler's choice is unpinned. Nothing fails if someone adds an argument-dependent branch to `fleet_send` or `POST /sessions/{id}/message` and leaves the gate above it. ## The ask Add a test that pins, for **every** MCP tool handler and **every** REST route, the `Authz.Action` it actually selects — including the argument shapes that could select a different one. Model it on `pollingByTargetIsADrainAndPollingByTicketIsARead`: assert against the extracted choice, not against a private copy of the rule inside the test. Two properties matter more than the exact shape: 1. **It must fail when a handler's choice changes**, not only when the role table changes. If the test still passes after you move a gate above a branch, it is testing the wrong thing. 2. **It must fail when a new handler is added with no pinned action** — otherwise the coverage rots the moment someone adds a tool. An enumeration-driven test that walks the registered handlers gives you that for free; a hand-written list does not. Property 2 is the harder half and the more valuable one. If you cannot get both, say which you got and why. ## What I verified myself, and how Independently of the audit report, on `main` at `66e5247`: - `rest/FleetApp.java` registers **15** routes (lines 163-179). 14 call `allow(ctx, Authz.Action.X, target)` as the first statement of their handler; `GET /healthz` is deliberately open as a liveness probe. - `mcp/FleetMcp.java` registers **11** tools. `fleet_poll` is the only one whose operation changes with its arguments, and since #272 it chooses via `pollAction(target)` before dispatch. - Matching route/tool pairs use the same action on both surfaces, and `Fleetd.main` passes one `CallerResolver` to both. I also chased the one fail-open in the gate: `FleetApp.allow` returns `true` when `auth == null`, and `FleetMcp.deny` does the same at line 481. **Not reachable in production** — `Fleetd.main` constructs a `CallerResolver` on both branches (lines 622 and 626), and `cfg.validateAuthExposure()` throws at startup if the bind is wider than the auth mode can defend, so the dangerous configuration cannot be reached by ignoring a log line. The null path is the legacy/test constructor only. Worth knowing, not worth changing. ## Scope Tests only. Do not change any handler or the role table — there is nothing wrong with either right now. If writing the test makes you want to change production code to make a handler's choice observable, that is fine and expected (that is what `pollAction` did), but keep the behaviour identical and say what you extracted.
ltms commented 2026-09-04 05:53:01 +02:00
Author
Owner
Copy Link
Copy Source

Merged as 0087645. Both properties hold, and I proved each one myself against the real merge rather than on the worker's word — deliberately using different mutations than the ones it reported.

Property 1 — flipped fleet_ack from DRAIN to READ (the worker had used fleet_poll):

FleetMcpAuthzTest.everyRegisteredToolHasItsHandlerActionPinned:225 expected: <DRAIN> but was: <READ>

Property 2 — registered a real extra route, app.get("/dai-fake-probe", this::healthz), with no pinned action:

GET /dai-fake-probe is registered but has no pinned authorization action
  ==> IllegalArgumentException: route has no authorization gate: GET /dai-fake-probe

Both restored afterwards; the tree is clean and the merge built green at 1285 tests.

What landed

toolAction(String, Map) and routeAction(String) now hold the choice, every handler gates through them, and both throw on a name they do not know. The tests enumerate what is actually registered by scraping tool("fleet_…") from FleetMcp.java and app.<verb>("…") from FleetApp.java — the technique McpContractDocTest already used — instead of a hand-written list. Each scrape carries a denominator guard, so a scrape that quietly stops matching fails loudly instead of passing on an empty set.

GET /healthz stays ungated, but the test now asserts routeAction("GET /healthz") throws. The exemption is a decision someone can see, not a hole.

Note on how this went

The first round delivered property 1 and said plainly that property 2 was not done, because both inventories were hand-declared. That honest report is the only reason this took one more round instead of shipping a test that agreed with itself. The fix was to point at McpContractDocTest, which had solved the same problem in this repo months ago — worth remembering that the precedent existed and neither of us found it first time.

Remaining limits, stated rather than hidden

  • The MCP denominator guard is >= 10 against eleven registered tools, so removing a tool would not trip it. It guards a broken scrape, not a shrinking surface.
  • Both scrapes read source text. A route registered through a variable that is not named app, or a tool registered by something other than a literal tool("…") call, would be invisible to them. Javalin 6.7 can report its own endpoints from the instance build() returns; reading the live registry would close that gap and is the better version if anyone revisits this.
Merged as `0087645`. Both properties hold, and I proved each one myself against the real merge rather than on the worker's word — deliberately using different mutations than the ones it reported. **Property 1** — flipped `fleet_ack` from `DRAIN` to `READ` (the worker had used `fleet_poll`): ``` FleetMcpAuthzTest.everyRegisteredToolHasItsHandlerActionPinned:225 expected: <DRAIN> but was: <READ> ``` **Property 2** — registered a real extra route, `app.get("/dai-fake-probe", this::healthz)`, with no pinned action: ``` GET /dai-fake-probe is registered but has no pinned authorization action ==> IllegalArgumentException: route has no authorization gate: GET /dai-fake-probe ``` Both restored afterwards; the tree is clean and the merge built green at **1285 tests**. ## What landed `toolAction(String, Map)` and `routeAction(String)` now hold the choice, every handler gates through them, and both throw on a name they do not know. The tests enumerate what is **actually registered** by scraping `tool("fleet_…")` from `FleetMcp.java` and `app.<verb>("…")` from `FleetApp.java` — the technique `McpContractDocTest` already used — instead of a hand-written list. Each scrape carries a denominator guard, so a scrape that quietly stops matching fails loudly instead of passing on an empty set. `GET /healthz` stays ungated, but the test now asserts `routeAction("GET /healthz")` **throws**. The exemption is a decision someone can see, not a hole. ## Note on how this went The first round delivered property 1 and said plainly that property 2 was not done, because both inventories were hand-declared. That honest report is the only reason this took one more round instead of shipping a test that agreed with itself. The fix was to point at `McpContractDocTest`, which had solved the same problem in this repo months ago — worth remembering that the precedent existed and neither of us found it first time. ## Remaining limits, stated rather than hidden - The MCP denominator guard is `>= 10` against eleven registered tools, so **removing** a tool would not trip it. It guards a broken scrape, not a shrinking surface. - Both scrapes read source text. A route registered through a variable that is not named `app`, or a tool registered by something other than a literal `tool("…")` call, would be invisible to them. Javalin 6.7 can report its own endpoints from the instance `build()` returns; reading the live registry would close that gap and is the better version if anyone revisits this.
ltms closed this issue 2026-09-04 05:53:04 +02:00
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#281
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?