Repository navigation
sec(auth): account scope can be laundered through the group-membership endpoint (join a wider group, or leave the scoping one) #1756
Description
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.
Activity
cristim commentedon Aug 8, 2026
Correction to this issue body: it describes code that does not exist on
mainI 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:Does exist on
main:guardGroupChange,addsNewGroup. Onmain,guardGroupChangestill carries the old inline self-escalation branch — a directHasPermission(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
maindoes not have.The defect itself is confirmed, on current
mainReproduced live at
9bfd6822c, both routes: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
accountScopeGaprather than duplicating it. #1737 is rebased, MERGEABLE, and with a reviewer.Two implementation traps, both learned on #1737
len(...) == 0, notIsUnrestrictedAccess(...). 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.guardGroupChangeandguardSelfEscalationare 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:
cristim commentedon Aug 12, 2026
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: migrations000024,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
addsNewGroupearly 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:usersinherited from prior groups, which is a behaviour change beyond what this issue asks for. It was instead moved insideguardSelfEscalation, 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 anAuthAdminroute requiring{admin, *}. The actor is therefore a scoped administrator, not any holder ofupdate:users.I am leaving
severity/criticalin 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:
DeleteGroupdrops thegroupsrow without purgingusers.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 > 0pluslen(Groups) == 0, matchinggrantCeilingAccountsandResolveAllowedAccountscharacter 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.