Repository navigation
FlockScopeGuard's fail-open branch grants account-wide flock access to an unresolved actor #787
Description
Activity
Scoped as a prerequisite for #789 (EPIC: MCP server support), slice 6 — the write tool. Not a blocker for the epic as a whole.
The reasoning, so the boundary is not lost:
FlockScopeGuardis consulted byRecordDailyEntry, which is slice 6. The MCP read slices do not reach it — they are scoped by theFlockScopeEF query filter instead. So this gates the write and nothing else in that epic.It became a prerequisite rather than a nice-to-have because an adversarial review of the MCP design found that the mechanism meant to make this branch unreachable from MCP has a hole: a tool injecting a helper that opens a secondary DI scope leaves
CurrentUserContextunresolved in that scope, landing exactly onif (!user.IsResolved) return Result.Success(). The MCP design's own fix (a dependency-graph walk plus a causal test) is the primary control; flipping this branch fail-closed is the backstop that turns a silent grant into a refusal if that control is ever weakened.Deliberately not made an epic-wide blocker: this change touches both seeders, four one-shot CLI verbs and every non-HTTP caller, and the guard's own comment says it "deserves its own issue rather than a drive-by". Making MCP's read slices wait on it would buy no safety, since those slices never reach this code.
Triaged in the 2026-09-13 issue cleanup. Kept open at
priority:tier3, tied to #789 slice 6 — no change. Recording the reasoning so the tier is a decision rather than an omission.Why it is not being promoted: no unresolved caller exists today. Both seeders declare an actor before every feed/water-usage call (#500), every HTTP route behind the guard requires authentication, and
TenantResolutionMiddleware401s an authenticated principal that cannot resolve. So this is a latent default, not a live hole, and it moves when the thing that would make it live moves.Why it is not being closed either. Two properties make it worth keeping on the books:
- The protection on the audited handlers is an accident, not a design.
RecordDailyEntryandSubmitDailyEntryare safe only because they write an audit event andIAuditWriterfails closed on an unresolved actor — they throw before reachingif (!user.IsResolved) return Result.Success().RecordFeedUsageandRecordWaterUsageaudit nothing and reach it. Any new flock-scoped handler that does not audit inherits the hole by default, and nothing will tell its author. - Two fail-open defaults compound.
FlockScope(Worker flock assignment is enforced on writes but not on reads — a restricted worker can enumerate and read unassigned flocks #388) independently defaultsIsUnrestricted = truewhen unresolved. An unresolved caller is therefore both unscoped by the guard and unrestricted by the query filter.
The trigger that promotes this: MCP slice 6 (#809, the write tool), or any new non-HTTP caller of a flock-scoped handler. The MCP design's adversarial review found the concrete route — a tool injecting a helper that opens a secondary DI scope leaves
CurrentUserContextunresolved in that scope, landing exactly on this branch. That design's dependency-graph walk plus causal test is the primary control; flipping this branch fail-closed is the backstop.A note for whoever eventually does it: this is a behaviour change from open to closed on an authorization default, which is exactly what #500 declined to do as a drive-by. It needs every non-HTTP caller audited for a declared actor first, and a causal test that goes red when the branch is restored — not just a flipped boolean.
- The protection on the audited handlers is an accident, not a design.
- added a commit that references this issue
on Sep 14, 2026 - added a commit that references this issue
on Sep 16, 2026
FlockScopeGuard.CheckAsync(bottom ofsrc/Cluckwork.Infrastructure/Repositories/UserRoleAssignmentRepository.cs, ~line 64) opens with:The guard's own comment has been asking for this issue since #500, and says so explicitly:
This is that issue. Surfaced again while designing #770 (MCP server support), where an unresolved actor was a live possibility rather than a hypothetical.
The gap, precisely
The comment already draws the distinction and it is worth preserving exactly, because half of it is easy to get wrong:
RecordDailyEntryandSubmitDailyEntrydo write an audit event, andIAuditWriterfails closed on an unresolved actor (fix(seed): seeded audit events carry "(unresolved)" as the actor, now visible on record History columns #500), so an unresolved caller throws before reaching the fail-open line. Loud.RecordFeedUsageandRecordWaterUsageaudit nothing. An unresolved caller reaches the branch and is granted account-wide flock access, silently. Nothing downstream catches it.So the protection on the two audited handlers is an accident of which handlers happen to audit — not a property of the guard. Any new flock-scoped handler that does not audit inherits the hole by default.
Note also
FlockScope(#388,src/Cluckwork.Infrastructure/Persistence/FlockScope.cs) defaultsIsUnrestricted = truewhen unresolved, deliberately, for the same non-HTTP callers. The two defaults compound: an unresolved caller is both unscoped by the guard and unrestricted by the query filter.Why it is not presently a defect
No such caller exists today. Both seeders declare an actor before every feed/water-usage call (#500), every HTTP route behind the guard requires authentication, and
TenantResolutionMiddleware401s an authenticated principal that cannot resolve. The one-shot CLI verbs declare aSystemActorsidentity.It is a live gap for a future non-HTTP caller, and the number of those is growing.
Why the MCP design did not fix it
#770's design makes the branch unreachable from MCP — a scoped
McpCallContextwhose factory throws unless tenant, actor and flock scope are all resolved, enforced by an assembly walk so a future tool inherits it. That is deliberately narrower than fixing the guard: flipping the default is an authorization behaviour change affecting both seeders, the one-shot verbs and every non-HTTP caller, which is exactly the drive-by the comment warns against.What a fix probably looks like
Flip the branch to fail closed, and make every legitimate non-HTTP caller declare an actor explicitly — most already do, post-#500. The work is in finding the ones that do not, and in deciding what a system actor that should be account-wide declares instead, since
ResolveSystemActorcurrently produces zero roles, whichRoles.ResolveEffectivereads as a plainWorker, which with zero assignment rows is the account-wide case. That interaction is the subtle part: a system actor today has more effective flock reach than a restricted worker, not less.Worth pairing with a guard that walks flock-scoped handlers and asserts each is unreachable with an unresolved actor, rather than relying on the audit writer's incidental coverage.