Repository navigation
FlockScopeGuard's fail-open branch grants account-wide flock access to an unresolved actor #787
Description
FlockScopeGuard.CheckAsync (bottom of src/Cluckwork.Infrastructure/Repositories/UserRoleAssignmentRepository.cs, ~line 64) opens with:
if (!user.IsResolved) return Result.Success();The guard's own comment has been asking for this issue since #500, and says so explicitly:
Closing the gap here means flipping an authorization default from open to closed, which is a behaviour change beyond #500's scope and deserves its own issue rather than a drive-by. Documented rather than silently narrowed.
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) defaults IsUnrestricted = true when 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 TenantResolutionMiddleware 401s an authenticated principal that cannot resolve. The one-shot CLI verbs declare a SystemActors identity.
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 McpCallContext whose 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 ResolveSystemActor currently produces zero roles, which Roles.ResolveEffective reads as a plain Worker, 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.
Activity
docs(mcp): fix 'this PR' references and resolve an answered open ques…
docs(mcp): fix 'this PR' references and resolve an answered open ques…
mforce commentedon Sep 12, 2026
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.
docs(mcp): round 2 attacked the fixes and found two more wrong guards
docs(mcp): round 3 — the #787 backstop claim was wrong for reads
docs(mcp): record the MCP server design and what the spike got wrong (#…
mforce commentedon Sep 13, 2026
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:
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.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.
fix(auth): fail closed on unresolved flock-scope actors (#787) (#868)
chore(main): release 0.1.2 (#802)