sec(auth): account scope can be laundered through the group-membership endpoint (join a wider group, or leave the scoping one)

[cristim]

Found by independent adversarial review of PR #1737. Live on main. PR #1737 bounds what an actor may grant given their account scope; this is how an actor raises their own scope in a single self-edit, through a neighbouring endpoint that #1737 already guards on the permission dimension but not the account dimension.

Two independent routes, opposite directions.

Route 1 — join a wider group

guardSelfEscalation gates self-added groups on update:users, which the acting user already holds. It then calls guardSelfCarvedOutGrant, which checks only the carved-out money verbs of the group being joined.

Account scope is never checked. So joining any broader-scoped group is permitted, and a default deployment ships exactly one: Administrators, seeded allowed_accounts = ARRAY[*] by migrations 000024/000057.

Route 2 — leave the scoping group

guardGroupChange returns early unless addsNewGroup(prior, next). A self-edit that only removes groups is therefore not guarded at all.

Dropping the group carrying the account restriction leaves AllowedAccounts = [], which IsUnrestrictedAccess reads as all accounts.

Removing the restriction grants the restriction. That inversion is the reason this is worth fixing at the representation and not only at the guard.

Why the existing guards do not catch it

Both routes exercise the permission dimensions guard correctly. The defect is that the account dimension has no equivalent on this endpoint — the same asymmetry PR #1737 was written to close on the group-write path, still open on the membership path.

Fix

Apply an account-scope ceiling to self-membership changes, in both directions:

• Adding a group must not grant the actor accounts beyond their current scope.
• Removing a group must not widen the actors effective scope. This is the counter-intuitive half and the one an implementation will miss: the guard must compare resulting scope against prior scope, not merely check what is being added.

Note that guardGroupChanges addsNewGroup early return has to go, or the removal route stays unguarded no matter what is added inside it.

Verification

Both directions, and a control — a refusal-only test proves nothing here, since a fix refusing all membership edits would pass it:

• scoped actor joins Administrators (allowed_accounts = [*]) -> refused
• scoped actor drops the group carrying their restriction -> refused
• actor makes a membership change that does not widen scope -> still allowed
• actor joins a group whose scope is a subset of their own -> still allowed

Mutation-verify per test, run individually: an aggregate package run misreports this because a mock panic kills the binary.

Related

• sec(auth): enforce a grant ceiling and system-managed guard on group writes #1737 — bounds granting on the group-write path. This is the same asymmetry on the membership path. #1737s claim should be read as covering group writes, not the account dimension generally.
• sec(auth): account scoping fails OPEN when a user's groups cannot be resolved (unrestricted access to all accounts) #1748 / PR sec(auth): fail closed when a user's account scope cannot be established #1752 — account scope failing open when groups cannot be resolved. Different mechanism, same representation hazard: empty means unrestricted.
• sec(auth): the #923 money separation-of-duties carve-out is voidable by any admin in one request #1550 — the original grant-ceiling issue.

[cristim]

Correction to this issue body: it describes code that does not exist on main

I wrote this issue from an independent review of PR #1737, and described the fix in terms of that PR's structure without checking the names against main. Correcting it before anyone acts on it.

Does not exist on main:

guardSelfEscalation      NO   (added by #1737)
guardSelfCarvedOutGrant  NO   (added by #1737)
addedGroups              NO   (added by #1737)

Does exist on main: guardGroupChange, addsNewGroup. On main, guardGroupChange still carries the old inline self-escalation branch — a direct HasPermission(update:users) check, no extracted helper, and no prior-membership resolution.

That last point matters for the fix: "compare resulting scope against prior scope" needs a prior-membership snapshot, which #1737 introduces and main does not have.

The defect itself is confirmed, on current main

Reproduced live at 9bfd6822c, both routes:

ROUTE1  join wider group          ACCEPTED   scope acct-A -> ALL ACCOUNTS
ROUTE2  leave scoping group       ACCEPTED   scope acct-A -> [] = ALL ACCOUNTS

So the issue is real and the severity stands. Only the implementation guidance was written against the wrong tree.

Sequencing decision: this stacks on #1737

Implementing against main's current structure would mean building the prior-scope resolution that #1737 already introduces, then reconciling two versions of the same guard at merge — two divergent implementations of one security guard, merged by hand, which is how a bypass gets introduced by the fix for a bypass.

So this waits on #1737 and reuses its accountScopeGap rather than duplicating it. #1737 is rebased, MERGEABLE, and with a reviewer.

Two implementation traps, both learned on #1737

• Test the prior scope with len(...) == 0, not IsUnrestrictedAccess(...). A scope containing the wildcard is already maximally wide, so no membership change can widen it — and all seven seeded groups carry the wildcard. Getting this wrong refuses every Administrators member. This exact predicate error blocked PR sec(auth): fail closed when a user's account scope cannot be established #1752.
• Edit by function body, not first match. guardGroupChange and guardSelfEscalation are neighbours and share repeating mock-setup anchors; a first-match replace lands in the wrong function and the test still passes. That happened twice on sec(auth): enforce a grant ceiling and system-managed guard on group writes #1737 and was caught only by mutation-verifying that the kill was assertion-driven rather than panic-driven.

Verification bar unchanged

Four directions, because a refusal-only suite passes against a fix that refuses every membership edit:

• scoped actor joins Administrators (wildcard scope) -> refused
• scoped actor drops the group carrying their restriction -> refused
• membership change that does not widen scope -> still allowed
• actor joins a group whose scope is a subset of their own -> still allowed

[cristim]

Three corrections to this issue's diagnosis, found while fixing it (PR #1808)

1. "A default deployment ships exactly one" broader group is wrong, and the real number is worse.

Every seeded group ships allowed_accounts = ARRAY['*'], not just Administrators: migrations 000024, 000057, 000059/000064, 000096. So Route 1 has several viable laundering targets rather than the single one this issue named.

That does not change the fix, because the guard keys off the scope value rather than a group ID, so all of them are covered by construction. But anyone reading this issue to assess exposure would have undercounted the attack surface, and anyone tempted to fix it by special-casing the Administrators group would have shipped a guard with several holes left open.

2. "The addsNewGroup early return has to go" was right about the effect, wrong about the disposition.

Deleting it outright would also have forced removal-only self-edits to hold update:users inherited from prior groups, which is a behaviour change beyond what this issue asks for. It was instead moved inside guardSelfEscalation, scoping it to the permission axis, where a removal genuinely can only shrink the union. The account axis is now evaluated unconditionally, which is what actually closes Route 2.

3. The realistic attacker is narrower than this issue implies.

PUT /api/users/{id} is an AuthAdmin route requiring {admin, *}. The actor is therefore a scoped administrator, not any holder of update:users.

I am leaving severity/critical in place. A scoped administrator reaching unrestricted access still crosses the exact boundary that scoping exists to enforce, in a single self-edit, and the fact that they already hold admin within their scope is what makes the escalation plausible rather than exotic. But the issue as written reads as a broader-reachability bug than it is, and anyone re-triaging should have the accurate precondition.

Note on the fix's own review

The first implementation refused on any unresolved group, which would have refused legitimate edits in ordinary operation: DeleteGroup drops the groups row without purging users.group_ids (an array column with no FK), so a dangling membership ID is the normal state after any custom group is deleted. Under refuse-on-any-skip, an already-unrestricted admin could not remove that dangling ID from their own membership - a change widening nothing, by a principal already at maximum scope, in a single-admin deployment with nobody able to clean up.

That was corrected without failing open. A skipped group can only widen the union when what survives is empty, because empty is the value read as unrestricted. The condition is now len(AllowedAccounts) == 0 && SkippedGroups > 0 plus len(Groups) == 0, matching grantCeilingAccounts and ResolveAllowedAccounts character for character. The ambiguous empty-with-a-skip case is still refused, so #1748 is preserved rather than reintroduced, and it is pinned from both directions: one mutant tightening back to refuse-on-any-skip fails the dangling-group control, another dropping the emptiness condition fails the fail-closed test.